choose MFA option if there is only one #288
Conversation
DannyDaoBoYang
requested review from
cfbao,
jharoutuniand2l,
scowing,
boarnoah and
gord5500
as code owners
|
PR #289 created to format .cs files. |
src/D2L.Bmx/Authenticate.cs
Outdated
| if( ( mfaOptions[i].Provider == DefautMFAProvider ) && ( mfaOptions[i].Name == DefaultMFAmethod ) ) { | ||
| return i; | ||
| } |
There was a problem hiding this comment.
| if( ( mfaOptions[i].Provider == DefautMFAProvider ) && ( mfaOptions[i].Name == DefaultMFAmethod ) ) { | |
| return i; | |
| } | |
| return Array.IndexOf(mfaOptions, mfaOptions.FirstOrDefault(option => option.Provider == DefautMFAProvider && option.Name == DefaultMFAmethod)) |
LINQ is pretty op once you start using it a bit. Not saying you should change what you have though
There was a problem hiding this comment.
sure, will use that
|
PR #290 created to format .cs files. |
src/D2L.Bmx/Authenticate.cs
Outdated
| ) | ||
| ); | ||
| } | ||
| private static int PromptMfa( MfaOption[] mfaOptions, string DefautMFAProvider, string DefaultMFAmethod ) { |
There was a problem hiding this comment.
| private static int PromptMfa( MfaOption[] mfaOptions, string DefautMFAProvider, string DefaultMFAmethod ) { | |
| private static int PromptMfa( MfaOption[] mfaOptions, string defaultMfaProvider, string defaultMfaMethod ) { |
Looks like the other areas of code have Mfa instead of MFA all caps
There was a problem hiding this comment.
Also want to use camelCase
There was a problem hiding this comment.
Sure will change that
|
PR #291 created to format .cs files. |
src/D2L.Bmx/Program.cs
Outdated
| @@ -55,6 +55,9 @@ | |||
| var profileOption = new Option<string>( | |||
| name: "--profile", | |||
| description: "aws profile name" ); | |||
| var MFAOption = new Option<string>( | |||
| name: "--dftMFA", | |||
| description: "the defaultMFA in the provider_method format (eg. OKTA_question)" ); | |||
There was a problem hiding this comment.
Looking at the implementation in this PR and Okta's doc on MFA API, I have doubt on how worthwhile this "default MFA option" feature really is.
The description here is cryptic, and hard to make it understandable too. Even if users understand this, it's still hard for users to know what to enter here (or in the config). Who would've thought of entering GOOGLE_token:software:totp for example?
Selecting the only MFA option when appropriate is a nice-to-have and an easy win, but I'm not convinced we should implement default MFA selection in the config or on the command line.
There was a problem hiding this comment.
Agreed, the way Okta exposes factors, how the names are written is really no good.
Since the MFA case is already an edge case for Okta authn the value is further lessened.
DannyDaoBoYang
force-pushed
the
Select_and_Set_MFA_In_Config
branch
from
ea5de90 to
244c3e5
Compare
github-actions
bot
added
size/S
|
Updated branch. So the only change here will be to choose MFA option if there is only one |
Thanks! Can you update the PR title & description accordingly too? |
DannyDaoBoYang
changed the title
Yes. Updated. |
Why