Improve question MFA prompt #458
Conversation
boarnoah
requested review from
cfbao,
scowing,
jharoutuniand2l,
jkomonen and
gord5500
|
|
||
| if( mfaInput is not null ) { | ||
| return mfaInput; | ||
| } | ||
| throw new BmxException( "Invalid MFA Input" ); | ||
| } | ||
|
|
||
| private string GetMaskedInput( string prompt ) { |
There was a problem hiding this comment.
Refactored this masked input read to be re-usable (for GetMfaResponse)
| internal record OktaMfaFactor( | ||
| string Id, | ||
| string FactorType, | ||
| string Provider, | ||
| string VendorName | ||
| string VendorName, | ||
| OktaMfaQuestionProfile Profile |
There was a problem hiding this comment.
This I really do not like.
It isn't straightforward to handle extracting nested value & do type discrimination based on FactorType for de serialization.
Without using a custom convertor at least AFAICS, so went with this approach.
There was a problem hiding this comment.
Isn't this what type discriminator is designed for? I don't see the need for a custom convertor.
There was a problem hiding this comment.
Couple of reasons,
- Out of order discriminator isn't in as of .NET 8 ? [JsonSerializer] Add support for out-of-order metadata reads. dotnet/runtime#97474
- Need a bit more of a code change to differentiate between types without FactorType as a property (to not conflict with it's use as the type discriminator)
- It still looks kind of silly, how even with type discrimination we would still need need to do some additional work to hoist QuestionText up from nested profile, ex:
// Binding to a nested property in polymorphic Profile [EditorBrowsable( EditorBrowsableState.Never )] public required OktaMfaQuestionProfile Profile { get; set; } public string QuestionText => Profile.QuestionText;
All of which to me suggests, a custom convertor is the way to go, if we want to take it all the way (even if we discard the third point)
There was a problem hiding this comment.
- We can use the STJ preview package.
- Yes, which should be straightforward.
- Not following... why?
There was a problem hiding this comment.
Moved to a separate PR targeting this ( #459 )since there is a few ways we can go about this.
re: 3,
Agreed it isn't necessary. Shouldn't really manipulate the Okta API types unnecessarily, after all its all still encapsulated within the context of the Okta authentication related code (no additional leak of implementation details involved).
| // On Windows, Console.ReadKey calls native console API, and will fail without a console attached | ||
| if( Console.IsInputRedirected ) { | ||
| Console.Error.WriteLine( """ | ||
| ====== WARNING ====== |
There was a problem hiding this comment.
It is going to pop this twice once for password + security question, in this case. But meh, that is a very niche edge case
github-actions
bot
added
size/L
boarnoah
force-pushed
the
aw/improve-mfa-prompt
branch
from
643bd10 to
3b50357
Compare
|
Changed separator used in factor list, now that we can't use Factor Type directly & need to use a custom pretty name for Factor Probably shouldn't align factor list vertically with padding since vendor name length has unknown bounds + it remains closer to the original look. |
Why
This MFA factor (which incidentally I thought we didn't allow with D2L's Okta) looked like:
Now, it will mask the input for the security question (which is a static value) + prompt with the question text rather than "Answer:"
Ticket
https://desire2learn.atlassian.net/browse/VUL-400
resolves #446