Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

allow interactive prompts when using cached creds #442

Merged
merged 1 commit into from
Jan 15, 2024
Merged

allow interactive prompts when using cached creds #442

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 27 additions & 22 deletions src/D2L.Bmx/AwsCredsCreator.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,16 +23,12 @@ bool cache
);
}

if( string.IsNullOrEmpty( account ) ) {
if( !string.IsNullOrEmpty( config.Account ) ) {
account = config.Account;
}
if( string.IsNullOrEmpty( account ) && !string.IsNullOrEmpty( config.Account ) ) {
account = config.Account;
Comment on lines -26 to +27
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unrelated nit: combine ifs to reduce nesting

}

if( string.IsNullOrEmpty( role ) ) {
if( !string.IsNullOrEmpty( config.Role ) ) {
role = config.Role;
}
if( string.IsNullOrEmpty( role ) && !string.IsNullOrEmpty( config.Role ) ) {
role = config.Role;
}

if( duration is null or 0 ) {
Expand All @@ -43,11 +39,8 @@ bool cache
}
}

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 ) ) {
Comment on lines -46 to +43
Copy link
Member Author

@cfbao cfbao Jan 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 cache and nonInteractive parameters of this method, which makes extending this method to work with writing credential process command (this propsal) more difficult.

var cachedCredentials = awsCredentialCache.GetCredentials(
org: oktaApi.Org,
user: oktaApi.User,
Expand All @@ -61,16 +54,14 @@ bool cache
}
}

//cache this one?
OktaApp[] awsApps = await oktaApi.Api.GetAwsAccountAppsAsync();

if( string.IsNullOrEmpty( account ) ) {
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" );
Comment on lines -68 to 61
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unrelated nit: revert condition to early exit & reduce nesting.

}
string[] accounts = awsApps.Select( app => app.Label ).ToArray();
account = consolePrompter.PromptAccount( accounts );
}

OktaApp selectedAwsApp = Array.Find(
Expand All @@ -83,12 +74,26 @@ bool cache
AwsRole[] rolesData = HtmlXmlHelper.GetRolesFromSamlResponse( samlResponse );

if( string.IsNullOrEmpty( role ) ) {
if( !nonInteractive ) {
string[] roles = rolesData.Select( r => r.RoleName ).ToArray();
role = consolePrompter.PromptRole( roles );
} else {
if( nonInteractive ) {
throw new BmxException( "Role value was not provided" );
}
string[] roles = rolesData.Select( r => r.RoleName ).ToArray();
role = consolePrompter.PromptRole( roles );
}

// try getting from cache again even if calling Okta is inevitable (we still avoid the AWS call)
if( cache ) {
Comment on lines +84 to +85
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Real change part 2 of 2.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

var cachedCredentials = awsCredentialCache.GetCredentials(
org: oktaApi.Org,
user: oktaApi.User,
accountName: account,
roleName: role,
duration: duration.Value
);

if( cachedCredentials is not null ) {
return cachedCredentials;
}
}

AwsRole selectedRoleData = Array.Find(
Expand Down