fix: restore OpenSAML class loading and make SAML parsing engine-portable

ModuleConfig.cfc

@@ -20,7 +20,12 @@ component {
 	this.entryPoint     = "/cbsso";
 
 	// Dependencies
-	this.dependencies = [ "hyper", "jwtcfml" ];
+	// cbjavaloader is load-order critical, not merely installed: onLoad() hands it this module's /lib so the
+	// bundled OpenSAML jar becomes resolvable. A module cannot place that jar on the classpath itself -
+	// this.javaSettings is an Application.cfc setting, read before any module registers, and ColdBox does
+	// not merge a module's copy of it - so without cbjavaloader every consuming app would have to add
+	// cbsso's own lib path to its Application.cfc.
+	this.dependencies = [ "hyper", "jwtcfml", "cbjavaloader" ];
 
 	routes = [
 		{
@@ -63,7 +68,15 @@ component {
 	 * Fired when the module is registered and activated.
 	 */
 	function onLoad(){
-		// Register all app disks
+		// Must precede registerProviders(): a SAML provider resolves cbsso.opensaml.* out of this path.
+		// Guarded because /lib is a build artifact of the Gradle project in /java and is absent from a
+		// source checkout, where appendPaths would throw "Invalid library path". Skipping it there leaves a
+		// SAML provider to fail on first use with its own error rather than taking the application down.
+		var openSAMLLibPath = modulePath & "/lib";
+		if ( directoryExists( openSAMLLibPath ) ) {
+			wirebox.getInstance( "loader@cbjavaloader" ).appendPaths( openSAMLLibPath );
+		}
+
 		wirebox.getInstance( "ProviderService@cbsso" ).registerProviders();
 
 		if ( settings.enableCBAuthIntegration ) {

changelog.md

@@ -11,6 +11,17 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Changed
 
+- `MicrosoftSAMLProvider.setFederationMetadataURL()` no longer initialises OpenSAML or fetches the IdP's
+  metadata. Every provider setter runs inside `ProviderService.registerProviders()`, i.e. inside the
+  module's `onLoad()`, so doing that work there made application boot load a 17MB jar and make one
+  outbound HTTPS call per configured provider. Both now happen on first use.
+- `MicrosoftSAMLProvider` throws `MicrosoftSAMLProvider.MissingConfiguration` when initialised without a
+  `federationMetadataURL`, rather than letting `cacheCerts()` fail against an empty string. Reachable now
+  that initialisation is lazy: it used to run only from the setter, which by definition had a value.
+- `ProviderService.registerProviders()` isolates each definition, so one provider that cannot be built no
+  longer aborts the remaining providers, `onLoad()`, or the application boot it runs inside. The failure
+  is logged and that provider is left unregistered, where `missing()` reports it and `Auth` redirects to
+  `errorRedirect`.
 - **BREAKING** `SSOAuthorizationResponse.getName()` returned `FirstName` instead of `Name`, so the value
   written by `setName()` could never be read back - `GitHubProvider` sets only `Name`, making its display
   name unreachable. It now returns `Name`, falling back to `FirstName LastName` for providers that set
@@ -21,6 +32,38 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Fixed
 
+- The bundled OpenSAML jar was not placed on any classpath, so `MicrosoftSAMLProvider` could not be
+  built at all: `createObject( "java", "cbsso.opensaml.AuthNRequestGenerator" )` searches the server
+  classpath, while `cbjavaloader` was no longer a module dependency and `onLoad()` no longer handed it
+  this module's `/lib`. `cbjavaloader` is restored to `this.dependencies`, `onLoad()` appends `/lib`
+  again, and the classes resolve through the `javaloader:` DSL, which is the loader those paths were
+  added to. A module cannot substitute `this.javaSettings` for this - it is an `Application.cfc` setting
+  read before any module registers, and ColdBox does not merge a module's copy - so the alternative would
+  be every consuming application adding cbsso's own lib path to its `Application.cfc`.
+- OpenSAML initialisation and signature validation now run with the thread context classloader set to
+  cbjavaloader's, and restore the original afterwards. `InitializationService` discovers its providers
+  through `ServiceLoader`, which reads the thread context classloader rather than the one the classes were
+  loaded from, so on BoxLang discovery found nothing and initialisation failed. Adobe ColdFusion resolves
+  it without help and is unaffected.
+- `initializeOpenSAMLLib()` guarded both the library initialisation and the certificate fetch on
+  `AuthNRequestGenerator` being set, and the generator is published before `cacheCerts()` runs. One
+  transient metadata failure - or an unreachable IdP on first sign-in - therefore satisfied the guard on
+  every later call, leaving a validator holding no certificates that never fetched them again for the life
+  of the application, so every subsequent login failed on a signature that could not be checked. The two
+  conditions are now tracked separately: the library initialises once, and the certificates are re-fetched
+  on each call until a fetch succeeds. `setFederationMetadataURL()` clears the flag, so re-registering a
+  provider against a different IdP refetches rather than validating against the old IdP's certificates.
+- `SAMLParsingService` matched `StatusCode` and `StatusMessage` by the `samlp:` prefix. Because
+  `extractUserInfo()` strips only the default namespace declaration, and BoxLang's `xmlSearch` does not
+  resolve a prefixed XPath against a prefix declared in the document, `detectSuccess()` returned false for
+  a valid, signed, successful assertion - failing every login closed. Both now match on `local-name()`,
+  which behaves identically on every engine.
+- `MicrosoftSAMLProvider.processAuthorizationEvent()` referenced an `xmlData` variable that no longer
+  exists when signature or issuer validation fails, so the one path handling an invalid response threw on
+  an undefined variable and reported that instead of the validation failure. It now reports the IdP's own
+  status message where there is one and the validation error otherwise. The private `extractErrorMessage()`
+  it called is removed: it was unreachable, declared `boolean` while returning a string, and duplicated
+  `SAMLParsingService.extractErrorMessage()`.
 - [#16](https://github.com/coldbox-modules/cbSSO/issues/16) An unregistered provider name threw a
   `KeyNotFoundException` from `ProviderService.get()` before the handler's `isNull()` guard could run,
   so `CBSSOMissingProvider` was never announced from `Auth.start()` or `Auth.authorize()`. The

models/ProviderService.cfc

@@ -10,23 +10,37 @@ component accessors="true" singleton threadsafe {
 
 	variables.providers = {};
 
+	/**
+	 * One provider failing to build no longer aborts the rest, or the module's onLoad(), or the application
+	 * boot that onLoad() runs inside. A definition can fail for reasons wholly outside this module -
+	 * an unreachable IdP, a missing jar, a provider type that is not installed - and losing every other
+	 * provider plus the whole application to it is out of all proportion. The failure is logged and that
+	 * provider is left unregistered, so missing() reports it and Auth redirects to errorRedirect.
+	 */
 	ProviderService function registerProviders(){
 		variables.moduleSettings.providers.each( function( providerDefinition ){
-			var provider = wirebox.getInstance( providerDefinition.type );
-
-			for ( var setting in providerDefinition ) {
-				if ( !structKeyExists( provider, "set#setting#" ) ) {
-					continue;
+			try {
+				var provider = wirebox.getInstance( providerDefinition.type );
+
+				for ( var setting in providerDefinition ) {
+					if ( !structKeyExists( provider, "set#setting#" ) ) {
+						continue;
+					}
+
+					invoke(
+						provider,
+						"set#setting#",
+						[ providerDefinition[ setting ] ]
+					);
 				}
 
-				invoke(
-					provider,
-					"set#setting#",
-					[ providerDefinition[ setting ] ]
+				providers[ provider.getName() ] = provider;
+			} catch ( any e ) {
+				log.error(
+					"Could not register SSO provider [#providerDefinition.name ?: providerDefinition.type ?: "unnamed"#] - it will be unavailable until the next successful registration",
+					{ "error" : e.message, "detail" : e.detail ?: "" }
 				);
 			}
-
-			providers[ provider.getName() ] = provider;
 		} );
 		return this;
 	}

models/providers/MicrosoftSAMLProvider.cfc

@@ -12,24 +12,30 @@ component
 	property name="federationMetadataURL";
 	property name="expectedIssuer";
 
-	property name="wirebox" inject="wirebox";
+	property name="wirebox"    inject="wirebox";
+	property name="javaLoader" inject="loader@cbjavaloader";
 	property name="AuthNRequestGenerator";
 	property name="responseValidator";
 	property name="SAMLParsingService" inject="SAMLParsingService@cbsso";
 
 	variables.name                  = "Entra";
 	variables.federationMetadataURL = "";
+	variables.certificatesCached    = false;
 
 	public string function getName(){
 		return variables.name;
 	}
 
+	/**
+	 * Stores the URL and nothing more. It used to initialise OpenSAML and fetch the IdP's metadata here,
+	 * but every setter on a provider runs inside ProviderService.registerProviders(), i.e. inside the
+	 * module's onLoad() - so that turned application boot into "load a 17MB jar and make an outbound HTTPS
+	 * call per configured provider", and any failure in either took the whole application down before it
+	 * could serve a request. Both now happen on first use, via initializeOpenSAMLLib().
+	 */
 	public any function setFederationMetadataURL( required string federationMetadataURL ){
-		variables.federationMetadataURL = federationMetadataURL;
-
-		initializeOpenSAMLLib();
-
-		responseValidator.cacheCerts( variables.federationMetadataURL );
+		variables.federationMetadataURL = arguments.federationMetadataURL;
+		variables.certificatesCached    = false;
 
 		return this;
 	}
@@ -53,16 +59,26 @@ component
 			authResponse.setRawResponseData( data );
 
 			try {
-				variables.AuthNRequestGenerator.initOpenSAML();
-				variables.responseValidator.parseAndValidate(
-					javacast( "string", data ),
-					variables.expectedIssuer
-				);
+				// initializeOpenSAMLLib() above has already initialised OpenSAML; this needs the classloader
+				// context for verification specifically. SignatureValidator.validate resolves crypto
+				// providers through the thread context classloader, which on BoxLang is not the one the
+				// OpenSAML classes came from. Marshalling alone does not need it - getRawSAMLRequest() goes
+				// through the same OpenSAML registry unwrapped and works - so do not widen this to "any
+				// OpenSAML call" or narrow it away on the assumption that initialisation covered it.
+				runWithClassLoader( function(){
+					variables.responseValidator.parseAndValidate(
+						javacast( "string", data ),
+						variables.expectedIssuer
+					);
+				} );
 			} catch ( any e ) {
+				// A validation failure is our verdict on the response, not the IdP's, so the exception is
+				// what explains it. samlData.errorMessage is only populated when the IdP itself returned a
+				// failure status, and in that case its own wording is the more useful of the two.
 				return authResponse
 					.setWasSuccessful( false )
 					.setRawResponseData( data )
-					.setErrorMessage( extractErrorMessage( xmlData ) )
+					.setErrorMessage( len( samlData.errorMessage ) ? samlData.errorMessage : e.message );
 			}
 
 
@@ -107,20 +123,74 @@ component
 		return binaryEncode( output, "base64" );
 	}
 
-	private boolean function extractErrorMessage( required xmlDoc ){
-		return xmlSearch( xmlDoc, "//samlp:StatusMessage" )[ 1 ].xmlchildren[ 1 ].xmltext;
-	}
-
+	/**
+	 * Resolved through cbjavaloader rather than createObject( "java", ... ): ModuleConfig hands the bundled
+	 * jar to cbjavaloader's URLClassLoader, which createObject does not consult - it searches the server
+	 * classpath and reports "has not been located in the [java] resolver".
+	 *
+	 * Two readiness conditions, guarded separately. The library is initialised once for the life of the
+	 * application, but the certificates come from an outbound fetch that can fail on its own, so guarding
+	 * both on the generator meant one transient metadata failure - which happens after the generator is
+	 * published - left this provider short-circuiting on every later call with a validator holding no
+	 * certificates, and no way back short of an application restart.
+	 */
 	private void function initializeOpenSAMLLib(){
-		if ( !isNull( variables.AuthNRequestGenerator ) ) {
+		if ( isNull( variables.AuthNRequestGenerator ) ) {
+			runWithClassLoader( function(){
+				var generator = wirebox.getInstance( "javaloader:cbsso.opensaml.AuthNRequestGenerator" );
+				var validator = wirebox.getInstance( "javaloader:cbsso.opensaml.AuthResponseValidator" );
+
+				generator.initOpenSAML();
+
+				// Published only once initOpenSAML() has returned. Assigning beforehand would let a failed
+				// initialisation leave a provider whose guard above short-circuits, so it could never
+				// initialise again for the life of the application.
+				variables.AuthNRequestGenerator = generator;
+				variables.responseValidator     = validator;
+			} );
+		}
+
+		if ( variables.certificatesCached ) {
 			return;
 		}
 
-		variables.AuthNRequestGenerator = createObject( "java", "cbsso.opensaml.AuthNRequestGenerator" );
-		variables.responseValidator     = createObject( "java", "cbsso.opensaml.AuthResponseValidator" );
+		// Reached lazily now rather than from setFederationMetadataURL(), so an unset URL arrives here
+		// instead of never getting this far. Named rather than left to fail inside cacheCerts, where an
+		// empty URL surfaces as an opaque fetch error on the user's first sign-in.
+		if ( !len( trim( variables.federationMetadataURL ) ) ) {
+			throw(
+				type    = "MicrosoftSAMLProvider.MissingConfiguration",
+				message = "federationMetadataURL is required but not set",
+				detail  = "Set it on the provider definition; it is the source of the signing certificates."
+			);
+		}
+
+		variables.responseValidator.cacheCerts( variables.federationMetadataURL );
+
+		variables.certificatesCached = true;
+	}
+
+	/**
+	 * OpenSAML's InitializationService discovers its providers through ServiceLoader, which reads the
+	 * *thread context* classloader. On BoxLang that is not cbjavaloader's URLClassLoader, so discovery finds
+	 * nothing and initialisation fails; Adobe ColdFusion resolves it without help. Swapped only for the
+	 * duration of the call, and restored in a finally so a failure cannot leak the wrong loader into the
+	 * request thread.
+	 */
+	private any function runWithClassLoader( required function callback ){
+		if ( !structKeyExists( server, "BoxLang" ) ) {
+			return callback();
+		}
+
+		var currentThread       = createObject( "java", "java.lang.Thread" ).currentThread();
+		var originalClassLoader = currentThread.getContextClassLoader();
 
-		variables.AuthNRequestGenerator.initOpenSAML();
-		responseValidator.cacheCerts( variables.federationMetadataURL );
+		try {
+			currentThread.setContextClassLoader( variables.javaLoader.getURLClassLoader() );
+			return callback();
+		} finally {
+			currentThread.setContextClassLoader( originalClassLoader );
+		}
 	}
 
 }

models/utility/SAMLParsingService.cfc

@@ -42,13 +42,23 @@ component singleton {
 		return data;
 	}
 
+	/**
+	 * Matched on local-name() rather than the `samlp:` prefix. extractUserInfo() strips only the default
+	 * namespace declaration, so `xmlns:samlp` survives on the document - but BoxLang's xmlSearch does not
+	 * resolve a prefixed XPath against a prefix declared in the document, so `//samlp:StatusCode` finds
+	 * nothing there and a valid, signed, successful assertion is reported as a failure. local-name() is
+	 * the form that behaves the same on every engine.
+	 */
 	private boolean function detectSuccess( required xmlDoc ){
-		return xmlSearch( xmlDoc, "//samlp:StatusCode[@Value='urn:oasis:names:tc:SAML:2.0:status:Success']" ).len() == 1;
+		return xmlSearch(
+			xmlDoc,
+			"//*[local-name()='StatusCode' and @Value='urn:oasis:names:tc:SAML:2.0:status:Success']"
+		).len() == 1;
 	}
 
 	private string function extractErrorMessage( required xmlDoc ){
 		try {
-			return xmlSearch( xmlDoc, "//samlp:StatusMessage" )[ 1 ].xmlchildren[ 1 ].xmltext;
+			return xmlSearch( xmlDoc, "//*[local-name()='StatusMessage']" )[ 1 ].xmlchildren[ 1 ].xmltext;
 		} catch ( any e ) {
 			try {
 				var nodes = xmlSearch( xmlDoc, "//*" );