allow interactive prompts when using cached creds #442
Conversation
| if( cache ) { | ||
| if( string.IsNullOrEmpty( account ) || string.IsNullOrEmpty( role ) ) { | ||
| throw new BmxException( "Account & Role must be provided when using cached AWS credentials" ); | ||
| } | ||
|
|
||
| // if using cache, avoid calling Okta at all if possible | ||
| if( cache && !string.IsNullOrEmpty( account ) && !string.IsNullOrEmpty( role ) ) { |
There was a problem hiding this comment.
Real change part 1 of 2:
Don't fail the run if account & role aren't provided via config or CLI flags.
- In my testing, I find erroring out here to be counter intuitive and unnecessarily harsh. It's perfectly fine if users want to get the cached creds early in an interactive run (which will call Okta), and later utilize the cached creds in a script (where account & role are provided via flags and Okta won't be called).
- Erroring out here kinda mixes the purpose of the
cacheandnonInteractiveparameters of this method, which makes extending this method to work with writing credential process command (this propsal) more difficult.
| // try getting from cache again even if calling Okta is inevitable (we still avoid the AWS call) | ||
| if( cache ) { |
There was a problem hiding this comment.
Seems fine, I think the code as originally written would have guarded against valid credentials in BMX that Okta no longer believes are valid (user unassigned from app etc...?)
That doesn't seem like something BMX would be able to care for, it could only be done authoritatively on the AWS side by revoking old sessions altogether.
There was a problem hiding this comment.
I think the code as originally written would have guarded against valid credentials in BMX that Okta no longer believes are valid (user unassigned from app etc...?)
No, there's no material difference here.
If user is unassigned from the app, and
- if they provide everything upfront, we don't call Okta in both worlds, and they can use the creds just fine
- if they don't provide everything upfront
- in the old world BMX fails directly
- in the new world they won't see the account & role they want in the selection prompts
| if( string.IsNullOrEmpty( account ) ) { | ||
| if( !string.IsNullOrEmpty( config.Account ) ) { | ||
| account = config.Account; | ||
| } | ||
| if( string.IsNullOrEmpty( account ) && !string.IsNullOrEmpty( config.Account ) ) { | ||
| account = config.Account; |
There was a problem hiding this comment.
Unrelated nit: combine ifs to reduce nesting
| if( !nonInteractive ) { | ||
| string[] accounts = awsApps.Select( app => app.Label ).ToArray(); | ||
| account = consolePrompter.PromptAccount( accounts ); | ||
| } else { | ||
| if( nonInteractive ) { | ||
| throw new BmxException( "Account value was not provided" ); |
There was a problem hiding this comment.
Unrelated nit: revert condition to early exit & reduce nesting.
cfbao
requested review from
tmanti,
boarnoah,
scowing,
jharoutuniand2l and
gord5500
In my testing, I find the strict behaviour when using
--cacheto be counter intuitive and unnecessarily harsh.It also makes extending code more difficult.
More here: #442 (comment)