From e8c594264e17bb6cb3d674017dfb78601df50a74 Mon Sep 17 00:00:00 2001 From: Chenfeng Bao Date: Mon, 21 Oct 2024 14:20:05 -0700 Subject: [PATCH] tweak DSSO messages for UX (#487) Don't print scary messages by default when passwordless auth (i.e. DSSO) fails for one reason or another. Also split out the initial validation of the DSSO method into a separate synchronous method, so it's easier to track when we need to print certain messages. https://desire2learn.atlassian.net/browse/VUL-506 --- src/D2L.Bmx/BmxEnvironment.cs | 5 ++ src/D2L.Bmx/Browser.cs | 23 ++++--- src/D2L.Bmx/OktaAuthenticator.cs | 108 ++++++++++++++++++++++--------- src/D2L.Bmx/Program.cs | 2 +- 4 files changed, 94 insertions(+), 44 deletions(-) create mode 100644 src/D2L.Bmx/BmxEnvironment.cs diff --git a/src/D2L.Bmx/BmxEnvironment.cs b/src/D2L.Bmx/BmxEnvironment.cs new file mode 100644 index 00000000..56d3aa35 --- /dev/null +++ b/src/D2L.Bmx/BmxEnvironment.cs @@ -0,0 +1,5 @@ +namespace D2L.Bmx; + +internal static class BmxEnvironment { + public static bool IsDebug { get; } = Environment.GetEnvironmentVariable( "BMX_DEBUG" ) == "1"; +} diff --git a/src/D2L.Bmx/Browser.cs b/src/D2L.Bmx/Browser.cs index af86cdc2..26cf9558 100644 --- a/src/D2L.Bmx/Browser.cs +++ b/src/D2L.Bmx/Browser.cs @@ -1,3 +1,4 @@ +using System.Diagnostics.CodeAnalysis; using PuppeteerSharp; namespace D2L.Bmx; @@ -25,12 +26,7 @@ public static class Browser { "/opt/microsoft/msedge/msedge", ]; - public static async Task LaunchBrowserAsync( bool noSandbox = false ) { - string? browserPath = GetPathToBrowser(); - if( browserPath is null ) { - return null; - } - + public static async Task LaunchBrowserAsync( string browserPath, bool noSandbox = false ) { var launchOptions = new LaunchOptions { ExecutablePath = browserPath, Args = noSandbox ? ["--no-sandbox"] : [] @@ -39,24 +35,27 @@ public static class Browser { return await Puppeteer.LaunchAsync( launchOptions ); } - private static string? GetPathToBrowser() { + public static bool TryGetPathToBrowser( [NotNullWhen( returnValue: true )] out string? path ) { + path = null; if( OperatingSystem.IsWindows() ) { foreach( string windowsPartialPath in WindowsPartialPaths ) { foreach( string environmentVariable in WindowsEnvironmentVariables ) { string? prefix = Environment.GetEnvironmentVariable( environmentVariable ); if( prefix is not null ) { - string path = Path.Join( prefix, windowsPartialPath ); + path = Path.Join( prefix, windowsPartialPath ); if( File.Exists( path ) ) { - return path; + return true; } } } } } else if( OperatingSystem.IsMacOS() ) { - return Array.Find( MacPaths, File.Exists ); + path = Array.Find( MacPaths, File.Exists ); + return path is not null; } else if( OperatingSystem.IsLinux() ) { - return Array.Find( LinuxPaths, File.Exists ); + path = Array.Find( LinuxPaths, File.Exists ); + return path is not null; } - return null; + return false; } } diff --git a/src/D2L.Bmx/OktaAuthenticator.cs b/src/D2L.Bmx/OktaAuthenticator.cs index a658a905..7f4f2cfb 100644 --- a/src/D2L.Bmx/OktaAuthenticator.cs +++ b/src/D2L.Bmx/OktaAuthenticator.cs @@ -2,7 +2,6 @@ using System.Diagnostics.CodeAnalysis; using D2L.Bmx.Okta; using D2L.Bmx.Okta.Models; -using PuppeteerSharp; namespace D2L.Bmx; @@ -60,14 +59,21 @@ bool bypassBrowserSecurity return new( Org: org, User: user, Client: oktaAuthenticated ); } - oktaAuthenticated = await GetDssoAuthenticatedClientAsync( - orgUrl, - user, - nonInteractive, - bypassBrowserSecurity - ); - if( oktaAuthenticated is not null ) { - return new( Org: org, User: user, Client: oktaAuthenticated ); + if( TryGetDssoAuthOptions( nonInteractive, bypassBrowserSecurity, out var dssoAuthOptions ) ) { + if( !nonInteractive ) { + Console.Error.WriteLine( "Attempting Okta passwordless authentication..." ); + } + oktaAuthenticated = await GetDssoAuthenticatedClientAsync( + orgUrl, + user, + dssoAuthOptions + ); + if( oktaAuthenticated is not null ) { + return new( Org: org, User: user, Client: oktaAuthenticated ); + } + if( !nonInteractive ) { + Console.Error.WriteLine( "Falling back to Okta password authentication..." ); + } } if( nonInteractive ) { @@ -99,35 +105,64 @@ private bool TryAuthenticateFromCache( return true; } - private async Task GetDssoAuthenticatedClientAsync( - Uri orgUrl, - string user, + private record DssoAuthOptions( string BrowserPath, bool BypassBrowserSecurity, bool PrintDebugMessage ); + + private bool TryGetDssoAuthOptions( bool nonInteractive, - bool bypassBrowserSecurity + bool bypassBrowserSecurity, + [NotNullWhen( returnValue: true )] out DssoAuthOptions? options ) { + options = null; + + bool printDebugMessage = + BmxEnvironment.IsDebug + || ( + // if the user passed --experimental-bypass-browser-security, they've explicitly expressed interest in the browser/DSSO process + bypassBrowserSecurity + // but we still shouldn't print messages in non-interactive mode (it may break credential process) + && !nonInteractive + ); bool hasElevatedPermissions = UserPrivileges.HasElevatedPermissions(); if( hasElevatedPermissions && !bypassBrowserSecurity ) { - consoleWriter.WriteWarning( $""" - BMX is being run with elevated privileges and is unable to use Okta passwordless authentication. - If you want passwordless authentication, and aren't concerned with the security of {orgUrl.Host}, - consider using the '--experimental-bypass-browser-security' flag. - """ - ); - return null; - } else if( !hasElevatedPermissions && bypassBrowserSecurity ) { + if( printDebugMessage ) { + consoleWriter.WriteWarning( + "BMX is being run with elevated privileges and is unable to use Okta passwordless authentication." + ); + } + return false; + } + if( !hasElevatedPermissions && bypassBrowserSecurity ) { // We want to avoid providing '--no-sandbox' to chromium unless absolutely necessary. bypassBrowserSecurity = false; + if( printDebugMessage ) { + consoleWriter.WriteWarning( + "Ignoring '--experimental-bypass-browser-security', " + + "as it's only needed when BMX is run with elevated privileges." + ); + } } - await using IBrowser? browser = await Browser.LaunchBrowserAsync( bypassBrowserSecurity ); - if( browser is null ) { - return null; + if( !Browser.TryGetPathToBrowser( out string? browserPath ) ) { + if( printDebugMessage ) { + consoleWriter.WriteWarning( + "No suitable browser found for Okta passwordless authentication" + ); + } + return false; } - if( !nonInteractive ) { - Console.Error.WriteLine( "Attempting Okta passwordless authentication." ); - } + options = new( browserPath, bypassBrowserSecurity, printDebugMessage ); + return true; + } + + private async Task GetDssoAuthenticatedClientAsync( + Uri orgUrl, + string user, + DssoAuthOptions options + ) { + await using var browser = await Browser.LaunchBrowserAsync( options.BrowserPath, options.BypassBrowserSecurity ); + using var cancellationTokenSource = new CancellationTokenSource( TimeSpan.FromSeconds( 15 ) ); var sessionIdTcs = new TaskCompletionSource( TaskCreationOptions.RunContinuationsAsynchronously ); string? sessionId = null; @@ -151,8 +186,15 @@ async Task GetSessionCookieAsync() { attempt++; await page.GoToAsync( orgUrl.AbsoluteUri ).WaitAsync( cancellationTokenSource.Token ); } else { - consoleWriter.WriteWarning( - "Okta passwordless authentication failed" ); + if( options.PrintDebugMessage ) { + if( url.AbsolutePath == "/" ) { + consoleWriter.WriteWarning( + "Okta passwordless authentication is not available." + ); + } else { + consoleWriter.WriteWarning( "Okta passwordless authentication failed" ); + } + } sessionIdTcs.SetResult( null ); } return; @@ -164,9 +206,13 @@ async Task GetSessionCookieAsync() { } } } catch( TaskCanceledException ) { - consoleWriter.WriteWarning( "Okta passwordless authentication timed out." ); + if( options.PrintDebugMessage ) { + consoleWriter.WriteWarning( "Okta passwordless authentication timed out." ); + } } catch( Exception ) { - consoleWriter.WriteWarning( "Unknown error occurred while trying Okta passwordless authentication." ); + if( options.PrintDebugMessage ) { + consoleWriter.WriteWarning( "Unknown error occurred while trying Okta passwordless authentication." ); + } } if( sessionId is null ) { diff --git a/src/D2L.Bmx/Program.cs b/src/D2L.Bmx/Program.cs index 1d68b8d1..210b3708 100644 --- a/src/D2L.Bmx/Program.cs +++ b/src/D2L.Bmx/Program.cs @@ -276,7 +276,7 @@ } else { consoleWriter.WriteError( "BMX exited with unexpected internal error" ); } - if( Environment.GetEnvironmentVariable( "BMX_DEBUG" ) == "1" ) { + if( BmxEnvironment.IsDebug ) { consoleWriter.WriteError( exception.ToString() ); } } )