-
Notifications
You must be signed in to change notification settings - Fork 6
add passwordless timeout config value #529
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.
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
Changes from 2 commits
e680241
2d50657
5d9989f
b3ce521
4b331fb
fa45664
78607ae
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,10 +1,17 @@ | ||
| namespace D2L.Bmx; | ||
|
|
||
| internal static class PasswordlessTimeoutDefaults { | ||
| public const int Min = 5; | ||
| public const int Max = 30; | ||
| public const int Default = 30; | ||
| } | ||
|
|
||
| internal record BmxConfig( | ||
| string? Org, | ||
| string? User, | ||
| string? Account, | ||
| string? Role, | ||
| string? Profile, | ||
| int? Duration | ||
| int? Duration, | ||
| int? PasswordlessTimeout | ||
| ); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -44,13 +44,28 @@ public BmxConfig GetConfiguration() { | |
| duration = configDuration; | ||
| } | ||
|
|
||
| int? passwordlessTimeout = null; | ||
| if( !string.IsNullOrEmpty( data.Global["passwordlessTimeout"] ) ) { | ||
| if( !int.TryParse( data.Global["passwordlessTimeout"], out int configTimeout ) | ||
| || configTimeout < 0 | ||
| || ( configTimeout > 0 && configTimeout < PasswordlessTimeoutDefaults.Min ) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. only need a single equality check with 0?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
| || configTimeout > PasswordlessTimeoutDefaults.Max ) { | ||
| throw new BmxException( | ||
| "Invalid passwordlessTimeout in config." | ||
| + $" Must be 0 (disabled) or between {PasswordlessTimeoutDefaults.Min}" | ||
| + $" and {PasswordlessTimeoutDefaults.Max} seconds." ); | ||
| } | ||
| passwordlessTimeout = configTimeout; | ||
| } | ||
|
|
||
| return new BmxConfig( | ||
| Org: data.Global["org"], | ||
| User: data.Global["user"], | ||
| Account: data.Global["account"], | ||
| Role: data.Global["role"], | ||
| Profile: data.Global["profile"], | ||
| Duration: duration | ||
| Duration: duration, | ||
| PasswordlessTimeout: passwordlessTimeout | ||
| ); | ||
| } | ||
|
|
||
|
|
@@ -75,6 +90,9 @@ public void SaveConfiguration( BmxConfig config ) { | |
| if( config.Duration.HasValue ) { | ||
| data.Global["duration"] = $"{config.Duration}"; | ||
| } | ||
| if( config.PasswordlessTimeout.HasValue ) { | ||
| data.Global["passwordlessTimeout"] = $"{config.PasswordlessTimeout}"; | ||
| } | ||
|
|
||
| fs.Position = 0; | ||
| fs.SetLength( 0 ); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -9,6 +9,7 @@ internal interface IConsolePrompter { | |
| string PromptUser( bool allowEmptyInput ); | ||
| string PromptPassword(); | ||
| int? PromptDuration(); | ||
| int? PromptPasswordlessTimeout(); | ||
| string PromptAccount( string[] accounts ); | ||
| string PromptRole( string[] roles ); | ||
| OktaMfaFactor SelectMfa( OktaMfaFactor[] mfaOptions ); | ||
|
|
@@ -63,6 +64,25 @@ string IConsolePrompter.PromptPassword() { | |
| return duration; | ||
| } | ||
|
|
||
| int? IConsolePrompter.PromptPasswordlessTimeout() { | ||
| Console.Error.Write( | ||
| "Okta passwordless (DSSO) timeout in seconds" | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should use |
||
| + " (optional, 0 to disable," | ||
| + $" {PasswordlessTimeoutDefaults.Min}-{PasswordlessTimeoutDefaults.Max}," | ||
| + $" default: {PasswordlessTimeoutDefaults.Default}): " ); | ||
| string? input = Console.ReadLine(); | ||
| if( input is null || string.IsNullOrWhiteSpace( input ) ) { | ||
| return null; | ||
| } | ||
| if( int.TryParse( input, out int timeout ) | ||
| && ( timeout == 0 | ||
| || ( timeout >= PasswordlessTimeoutDefaults.Min | ||
| && timeout <= PasswordlessTimeoutDefaults.Max ) ) ) { | ||
| return timeout; | ||
| } | ||
| return null; | ||
| } | ||
|
|
||
| string IConsolePrompter.PromptAccount( string[] accounts ) { | ||
| if( accounts.Length == 0 ) { | ||
| throw new BmxException( "No AWS account available" ); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -23,7 +23,8 @@ public async Task<OktaAuthenticatedContext> AuthenticateAsync( | |
| string? org, | ||
| string? user, | ||
| bool nonInteractive, | ||
| bool ignoreCache | ||
| bool ignoreCache, | ||
| int? passwordlessTimeout | ||
| ) { | ||
| var orgSource = ParameterSource.CliArg; | ||
| if( string.IsNullOrEmpty( org ) && !string.IsNullOrEmpty( config.Org ) ) { | ||
|
|
@@ -65,19 +66,30 @@ bool ignoreCache | |
| OperatingSystem.IsWindows() | ||
| && browserLauncher.TryGetPathToBrowser( out string? browserPath ) | ||
| ) { | ||
| if( !nonInteractive ) { | ||
| Console.Error.WriteLine( "Attempting Okta passwordless authentication..." ); | ||
| } | ||
| oktaAuthenticated = await GetDssoAuthenticatedClientAsync( | ||
| orgUrl, | ||
| user, | ||
| browserPath | ||
| ); | ||
| if( oktaAuthenticated is not null ) { | ||
| return new( Org: org, User: user, Client: oktaAuthenticated ); | ||
| } | ||
| if( !nonInteractive ) { | ||
| Console.Error.WriteLine( "Falling back to Okta password authentication..." ); | ||
| int resolvedTimeout = passwordlessTimeout | ||
| ?? config.PasswordlessTimeout | ||
| ?? PasswordlessTimeoutDefaults.Default; | ||
|
|
||
| if( resolvedTimeout == 0 ) { | ||
| if( BmxEnvironment.IsDebug ) { | ||
| messageWriter.WriteWarning( "Okta passwordless authentication disabled via configuration" ); | ||
| } | ||
| } else { | ||
| if( !nonInteractive ) { | ||
| Console.Error.WriteLine( "Attempting Okta passwordless authentication..." ); | ||
| } | ||
| oktaAuthenticated = await GetDssoAuthenticatedClientAsync( | ||
| orgUrl, | ||
| user, | ||
| browserPath, | ||
| resolvedTimeout | ||
| ); | ||
| if( oktaAuthenticated is not null ) { | ||
| return new( Org: org, User: user, Client: oktaAuthenticated ); | ||
| } | ||
| if( !nonInteractive ) { | ||
| Console.Error.WriteLine( "Falling back to Okta password authentication..." ); | ||
| } | ||
| } | ||
| } else if( BmxEnvironment.IsDebug ) { | ||
| messageWriter.WriteWarning( "No suitable browser found for Okta passwordless authentication" ); | ||
|
|
@@ -115,12 +127,13 @@ private bool TryAuthenticateFromCache( | |
| private async Task<IOktaAuthenticatedClient?> GetDssoAuthenticatedClientAsync( | ||
| Uri orgUrl, | ||
| string user, | ||
| string browserPath | ||
| string browserPath, | ||
| int timeoutSeconds | ||
| ) { | ||
| string? sessionId = null; | ||
|
|
||
| try { | ||
| sessionId = await GetSessionIdFromBrowserAsync( browserPath, orgUrl ); | ||
| sessionId = await GetSessionIdFromBrowserAsync( browserPath, orgUrl, timeoutSeconds ); | ||
| } catch( TaskCanceledException ex ) { | ||
| if( BmxEnvironment.IsDebug ) { | ||
| messageWriter.WriteWarning( $"Okta passwordless authentication timed out. \n{ex}" ); | ||
|
|
@@ -158,20 +171,21 @@ The provided Okta user '{providedLogin}' does not match the system configured pa | |
| return oktaAuthenticatedClient; | ||
| } | ||
|
|
||
| private async Task<string?> GetSessionIdFromBrowserAsync( string browserPath, Uri orgUrl ) { | ||
| private async Task<string?> GetSessionIdFromBrowserAsync( string browserPath, Uri orgUrl, int timeoutSeconds ) { | ||
| if( BmxEnvironment.IsDebug ) { | ||
| messageWriter.WriteWarning( $"Launching browser: {browserPath}" ); | ||
| } | ||
| await using var browser = await browserLauncher.LaunchAsync( browserPath ); | ||
|
|
||
| var sessionIdTcs = new TaskCompletionSource<string?>( TaskCreationOptions.RunContinuationsAsynchronously ); | ||
|
|
||
| // cancel if the total time exceeds 15 seconds, including all page loads and retries | ||
| using var cancellationTokenSource = new CancellationTokenSource( TimeSpan.FromSeconds( 15 ) ); | ||
| // cancel if the total time exceeds the configured timeout, including all page loads and retries | ||
| using var cancellationTokenSource = new CancellationTokenSource( TimeSpan.FromSeconds( timeoutSeconds ) ); | ||
| cancellationTokenSource.Token.Register( () => sessionIdTcs.TrySetCanceled() ); | ||
|
|
||
| // cancel if we can't load the first page for 6 seconds | ||
| using var pageTimer = new System.Timers.Timer( TimeSpan.FromSeconds( 6 ) ) { AutoReset = false }; | ||
| // cancel if we can't load the first page within a derived timeout | ||
| using var pageTimer = new System.Timers.Timer( | ||
| TimeSpan.FromSeconds( Math.Min( 6, timeoutSeconds / 2 ) ) ) { AutoReset = false }; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 6 is the min we should wait. Here should be
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. there's also a 3 sec wait somewhere below that need to be bumped up
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't believe the timeout that people actually hit is the 15 total timeout, but rather the 6 and 3 page load timeout. |
||
| pageTimer.Elapsed += ( _, _ ) => cancellationTokenSource.Cancel(); | ||
| pageTimer.Start(); | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -14,6 +14,10 @@ internal static class ParameterDescriptions { | |
| public const string NonInteractive = "Run non-interactively without showing any prompts"; | ||
| public const string CacheAwsCredentials = | ||
| "Enables Cache for AWS tokens. Implied if '--use-credential-process' is supplied"; | ||
| public static readonly string PasswordlessTimeout = | ||
| "Timeout for Okta passwordless (DSSO) authentication in seconds" | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we ever say "DSSO" in user facing messages? |
||
| + $" (0 to disable, {PasswordlessTimeoutDefaults.Min}-{PasswordlessTimeoutDefaults.Max}," | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. message not clear to me: 0 to disable the timeout control or to disable passwordless auth? |
||
| + $" default: {PasswordlessTimeoutDefaults.Default})"; | ||
| public const string UseCredentialProcess = """ | ||
| Write BMX command to AWS profile, so that AWS tools & SDKs using the profile will source credentials from BMX. | ||
| See https://docs.aws.amazon.com/cli/latest/userguide/cli-configure-sourcing-external.html. | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wouldn't necessarily cap max at 30. If people want to wait a minute I wouldn't stop them
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeaaa I was wondering what to do for this. I'll bump it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you updated the default not the max?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also, nit, and I'm not too sure either, but this seems more like "constants" rather than "config"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we already have a constants class
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
was just looking at the config parsing logic - I'm not sure there's much value in setting a non-zero min either?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess what I'm leaning towards is no min or max.
We can keep the "0 = disable" behaviour (which is a logical conclusion from the config) and check timeout >=0 as a sanity check.
But I'm not seeing value in a non-zero min or any max.
Keep things simpler.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure I can remove the limits. For me I just never saw it succeed below 5 seconds
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay I removed the limits and tried rewording the prompt for setting the value to 0 means disabling passwordless auth. Also the timing changes to always be timeout / 2.0