De-serialize Security Question MFA to different type #459
Conversation
| @@ -21,17 +21,68 @@ internal record AuthenticateResponseEmbedded( | |||
| OktaMfaFactor[]? Factors | |||
| ); | |||
|
|
|||
| [JsonPolymorphic( | |||
| TypeDiscriminatorPropertyName = "factorType", | |||
There was a problem hiding this comment.
Unfortunately, STJ doesn't offer a way to include the property in JSON used for type discrimination in the de-serialized object.
I did find a issue asking for this capability (can't find it now). It makes sense since ideally if you are using type discrimination, then the rest of your code should use the C# type system to handle differences in the various types.
In this case, where we only want to treat one of several derived types differently (but include different names for each type), this is incredibly cumbersome.
src/D2L.Bmx/OktaAuthenticator.cs
Outdated
| @@ -57,18 +57,18 @@ bool ignoreCache | |||
| if( authnResponse is AuthenticateResponse.MfaRequired mfaInfo ) { | |||
| OktaMfaFactor mfaFactor = consolePrompter.SelectMfa( mfaInfo.Factors ); | |||
|
|
|||
| if( !IsMfaFactorTypeSupported( mfaFactor.FactorType ) ) { | |||
| if( mfaFactor.FactorName == "Unknown" ) { | |||
There was a problem hiding this comment.
Not a fan of this at all
There was a problem hiding this comment.
This can be a type check against the (maybe renamed) base type, or at least you can make the string a constant so there's no possibility of a typo.
There was a problem hiding this comment.
Doesn't look great IMO. 19413d4
I think we should instead rely on something using UnknownDerivedTypeHandling = JsonUnknownDerivedTypeHandling.FailSerialization
& filtering out failures seems better although definitely more involved code wise (de-serialization call for this API response handling will need rework).
After all we aren't proceeding when unknown factor type is selected, might as well not populate the list with them (maybe a separate message for "x, y factors are not supported by BMX"
There was a problem hiding this comment.
I was thinking of giving "unknown" a special constant, rather than reusing the "FactoryType" thingy we're using elsewhere. Reusing that would be confusing for sure.
What about doing a runtime type check though? We could call the base type something like OktaMfaFactorUnspecified, and check the exact type (rather than using is which considers child types a match).
More involved code change is precisely why I didn't suggest failing serialization directly, but that's an option for sure. We can even make the base type abstract.
There was a problem hiding this comment.
actually, with a better base type name, the constant check wouldn't look so confusing either.
There was a problem hiding this comment.
Think we should go with a standalone const + factor name check,
b57bf15 is confusing in it's own right (since we use the base type name in quite a few contexts, it is awkward to name it "unknown"/"unsupported".
There was a problem hiding this comment.
Maybe BaseOktaMfaFactor or something might help, but it still feels wrong.
There was a problem hiding this comment.
Yeah, I see. Renaming the base type doesn't work. A distinctly named constant is good. The FactorType constant isn't in the type hierarchy anyway.
There was a problem hiding this comment.
Changed to a better named const for this, should revisit if this ends up happening dotnet/runtime#93471
| throw new BmxException( "Selected MFA not supported by BMX" ); | ||
| } | ||
|
|
||
| // TODO: Handle retry | ||
| if( mfaFactor.FactorType is "sms" or "call" or "email" ) { | ||
| if( mfaFactor.RequireChallengeIssue ) { |
There was a problem hiding this comment.
Alternatively if ( mfaFactor is OktaMfaEmailFactor ... ) { etc....?
boarnoah
force-pushed
the
mfa-factors-polymorphic
branch
from
44dbb09 to
7e9e4db
Compare
boarnoah
requested review from
cfbao,
scowing,
jharoutuniand2l,
jkomonen and
gord5500
and removed request for
a team
| } | ||
|
|
||
| internal record OktaMfaQuestionFactor() : OktaMfaFactor() { | ||
| public override string FactorName => "Security Question"; |
There was a problem hiding this comment.
Going with this approach lets us customize this to be different from Okta's raw value in API response, there is a case to be made for leaving it as-is to reduce confusion*.
* See:
token vs token:hardware, I suppose we could map it better to what Okta actually calls it in the UI (Yubikey etc..., but thats awfully vendor specific)
There was a problem hiding this comment.
Having control over a more user friendly name isn't a bad idea.
You can also do something like IMS does here
https://github.com/Brightspace/instance-metadata-store/blob/32251a9a9ff5c23c1fe7dea7faa2da39d02d8a5e/worker/src/D2L.InstanceMetadataStore.Worker/ConfigEtl/Extractor/ConfigEtlExtractorRequest.cs#L5-L10
So the discriminator value is defined in the child type itself to improve locality.
There was a problem hiding this comment.
I did play around with that, didn't find a great solution for keeping the discriminator as a static value but also to be able to use derived type's value for it without downcasting.
There was a problem hiding this comment.
I'm thinking of something like this
record OktaMfaFoo : OktaMfaFactor {
public const string FactorType = "foo";
public override string FactorName => FactorType;
}We use OktaMfaFoo.FactorType for the compile time discriminator declarations, and factor.FactorName in our runtime logic to avoid casting.
Alternatively,
record OktaMfaFoo : OktaMfaFactor {
public const string FactorType = "foo";
public override string FactorName => "Foo;
}purely for the locality benefit, to reduce possibility of a mismatch.
…smatch) - Rename "token" factor to a more appropriate name
boarnoah
force-pushed
the
mfa-factors-polymorphic
branch
from
b57bf15 to
2dcb2b7
Compare
Why
With .NET 9 we can use properties aside from the first in JSON to do type discrimination (in this case using
factorTypein the API).So we can use it de-serialize the different properties in Factor's Profile section (in this case we only care about QuestionText for the Security Question factor).
Ticket
N/A