okta: add passwordless dsso support #480
Conversation
github-actions
bot
added
language/csharp
size/L
gord5500
changed the title
|
There are still quite a few mentions of DSSO in user-facing messages. |
@cfbao I replaced DSSO with the actual name but I'll just say 'automatically' instead |
|
yeah... I don't think "Desktop Single Sign-On" means anything to most people either. |
| bool nonInteractive, | ||
| bool experimentalBypassBrowserSecurity | ||
| ) { | ||
| await using IBrowser? browser = await Browser.LaunchBrowserAsync( experimentalBypassBrowserSecurity ); |
There was a problem hiding this comment.
nit: for such a side-effect heavy operation (launching a browser), I'd rather not invoke it through a static class.
A class-interface combo with it passed as an explicit dependency through the constructor parameter the DI way would be nice.
No need to change this now though. I have a couple other nits with this OktaAuthenticator class's structure overall. Will perhaps refactor it sometime.
| string sessionLogin = oktaSession.Login.Split( "@" )[0]; | ||
| string providedLogin = user.Split( "@" )[0]; | ||
| if( !sessionLogin.Equals( providedLogin, StringComparison.OrdinalIgnoreCase ) ) { |
There was a problem hiding this comment.
It's technically possible for an Okta org to have two distinct users with logins:
Our logic here won't capture such a mismatch.
I'm fine with our logic for now, but this is something to think about.
|
Please update PR description too |
Co-authored-by: Chenfeng Bao <[email protected]>
Co-authored-by: Chenfeng Bao <[email protected]>
Co-authored-by: Chenfeng Bao <[email protected]>
Co-authored-by: Chenfeng Bao <[email protected]>
Co-authored-by: Chenfeng Bao <[email protected]>
gord5500
changed the title
gord5500
changed the title
gord5500
changed the title
|
@cfbao the review became stale. Do you mind looking over again please? |
src/D2L.Bmx/Browser.cs
Outdated
| foreach( string environmentVariable in WindowsEnvironmentVariables ) { | ||
| string? prefix = Environment.GetEnvironmentVariable( environmentVariable ); | ||
| if( prefix is not null ) { | ||
| string path = prefix + windowsPartialPath; |
There was a problem hiding this comment.
Just noticed this... best to use Path.Join for safe path concatenation.
Also, it'd be good to not start WindowsPartialPaths with leading (back)slashes, because they're relative (not absolute) paths.
src/D2L.Bmx/Browser.cs
Outdated
| return MacPaths.First( File.Exists ); | ||
| } else if( OperatingSystem.IsLinux() ) { | ||
| return LinuxPaths.First( File.Exists ); |
There was a problem hiding this comment.
This doesn't look right. If someone's Mac doesn't have Chrome or Edge installed, this would throw.
.FirstOrDefault should work.
Why
It'll probably make a lot of people happy
This is going to be on by default and if any failure occurs, fall back to the password prompt.
Adding an additional flag
--experimental-bypass-browser-securityonly for using bmx with elevated permissions. Chromium sandbox doesn't work without the--no-sanboxarg as admin. This can only be passed as a command line argFile size jumped from 13.3 MB to 18.6 MB. Doesn't seem too bad?
Ticket
VUL-453