add passwordless timeout config value - #529
Conversation
| // 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 }; |
There was a problem hiding this comment.
6 is the min we should wait. Here should be Math.Max
There was a problem hiding this comment.
there's also a 3 sec wait somewhere below that need to be bumped up
There was a problem hiding this comment.
I don't believe the timeout that people actually hit is the 15 total timeout, but rather the 6 and 3 page load timeout.
|
|
||
| internal static class PasswordlessTimeoutDefaults { | ||
| public const int Min = 5; | ||
| public const int Max = 30; |
There was a problem hiding this comment.
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.
Yeaaa I was wondering what to do for this. I'll bump it
There was a problem hiding this comment.
you updated the default not the max?
There was a problem hiding this comment.
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.
we already have a constants class
There was a problem hiding this comment.
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.
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.
There was a problem hiding this comment.
Sure I can remove the limits. For me I just never saw it succeed below 5 seconds
There was a problem hiding this comment.
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
| 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" |
There was a problem hiding this comment.
Do we ever say "DSSO" in user facing messages?
| "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" | ||
| + $" (0 to disable, {PasswordlessTimeoutDefaults.Min}-{PasswordlessTimeoutDefaults.Max}," |
There was a problem hiding this comment.
message not clear to me: 0 to disable the timeout control or to disable passwordless auth?
| || configTimeout < 0 | ||
| || ( configTimeout > 0 && configTimeout < PasswordlessTimeoutDefaults.Min ) |
There was a problem hiding this comment.
only need a single equality check with 0?
There was a problem hiding this comment.
|
|
||
| int? IConsolePrompter.PromptPasswordlessTimeout() { | ||
| Console.Error.Write( | ||
| "Okta passwordless (DSSO) timeout in seconds" |
There was a problem hiding this comment.
There was a problem hiding this comment.
should use ParameterDescriptions here?
| 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.Max( 6, timeoutSeconds / 2 ) ) ) { AutoReset = false }; |
There was a problem hiding this comment.
minor, but might be better to avoid integer arithmetic and truncation behaviour
e.g.
5/2 = 2
vs
5/2.0 = 2.5
25% difference
| // we give the first page 6 sec to load, but 3 sec is probably enough for subsequent pages | ||
| pageTimer.Interval = 3000; | ||
| pageTimer.Interval = Math.Max( 3, timeoutSeconds / 4 ) * 1000; |
There was a problem hiding this comment.
I'm maybe paranoid, but I wonder if any page might load really slow and 7 sec isn't enough... especially like, what if the first page isn't the slowest one, and it's the 3 page that would take bulk of the time??
thinking - remove the distinction between first page load vs subsequent page load, and just always use timeoutSeconds / 2.0 for each page load.
| // we give the first page 6 sec to load, but 3 sec is probably enough for subsequent pages | ||
| pageTimer.Interval = 3000; | ||
| pageTimer.Interval = Math.Max( 3, timeoutSeconds / 2.0 ) * 1000; |
There was a problem hiding this comment.
thinking about these page timeouts again...
I wonder if the 6 vs 3 timeouts minimums are even meaningful now.
Can maybe just set a single page that's half the total timeout when the Timer is created, and not change the Interval here?
There was a problem hiding this comment.
Sure I've got it now to just always be set to half the total timeout
3 participants
Why
Zscaler can be slow for some people. Bump the default timeout from 15 to 30 seconds and add an optional config value to lower timeout or just disable passwordless entirely. Updating the tests in vulcan-scratch repo too to cover a few scenarios
Ticket
HOD-4334 - BMX: support longer timeouts