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
Brightspace · Oct 21, 2024 · e8c5942 · e8c5942
1 parent f279984
commit e8c5942
Show file tree

Hide file tree

Showing 4 changed files with 94 additions and 44 deletions.
diff --git a/src/D2L.Bmx/BmxEnvironment.cs 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
@@ -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<IBrowser?> LaunchBrowserAsync( bool noSandbox = false ) {
-		string? browserPath = GetPathToBrowser();
-		if( browserPath is null ) {
-			return null;
-		}
-
+	public static async Task<IBrowser> 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
@@ -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<IOktaAuthenticatedClient?> 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<IOktaAuthenticatedClient?> 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<string?>( 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
@@ -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() );
 		}
 	} )