From 5eb0c8c7f3a2ba83fff1e748820fd8f79f868407 Mon Sep 17 00:00:00 2001 From: Doug Cain Date: Wed, 5 Aug 2026 00:43:04 +0100 Subject: [PATCH 1/2] feat: expose the IdP's claim set and the Subject NameID The typed getters on ISSOAuthorizationResponse are the intersection of what four providers have in common - email, a name, an id. Anything else an IdP asserts has nowhere to go: an Entra group or role claim, a Google `hd`, a customer's employee-number claim. A consumer wanting one had to re-parse getRawResponseData() itself, which means re-implementing the namespace handling this module already does. Adding a getter per claim would grow the interface without end, so this adds a map instead: getClaims() keyed by the name the IdP used, and getClaim( name, default ) for the single-value case. Closed to further growth, and it reads the same way for a SAML attribute as for an oAuth id token claim, which is what makes it worth putting on the shared interface rather than on one provider. A claim always holds an array. A SAML attribute may carry several AttributeValues - Entra's authnmethodsreferences does, and its group claims do - and an IdP may split one claim across repeated Attribute elements. The existing extraction took the first value of the first element, so the rest were unreachable and nothing said so. Values that are not simple, a nested object in an id token, are left out of the map and stay on getRawResponseData(). NameID gets its own pair of getters rather than a slot in the map, because it is not an attribute and its Format changes what the value means: Entra's default is a pairwise identifier scoped to one app registration, so the same person arrives under a different NameID at a second registration in the same tenant. A caller that cannot see the Format cannot tell a portable identifier from a scoped one. The response could not reach NameID at all before this. MicrosoftSAMLProvider sets the claims on the success path only. An assertion whose signature did not verify has asserted nothing, so a consumer reading a claim off a failed response would be trusting whoever sent it rather than the IdP. Deriving the typed fields from the claim set also fixes them for prefixed assertions. The old extractors matched //Attribute[@Name='...'], which resolves only when the assertion carries the SAML namespace as its default - extractUserInfo() strips default namespace declarations and nothing else. An IdP that prefixes its elements, as ADFS and Shibboleth do and Entra can be configured to, yielded no first name, surname or object identifier, and the response came back as "Failed to extract user information". Covered by a new prefixed fixture. A missing givenname, surname or objectidentifier claim still fails the whole response, as it did before. That looks wrong - a missing display-name claim is a poor reason to refuse a login - but it is a question about required claims, not about reaching them, so it is left alone here. 29 specs pass on both CI engines: BoxLang 1.17.0-snapshot and Lucee 5.4.8.2. --- changelog.md | 29 ++++ models/ISSOAuthorizationResponse.cfc | 4 + models/SSOAuthorizationResponse.cfc | 80 ++++++++++ models/providers/FacebookProvider.cfc | 1 + models/providers/GitHubProvider.cfc | 3 +- models/providers/GoogleProvider.cfc | 1 + models/providers/MicrosoftSAMLProvider.cfc | 5 + models/utility/SAMLParsingService.cfc | 146 +++++++++++++----- .../tests/resources/prefixedSAMLResponse.xml | 47 ++++++ .../tests/specs/SAMLParsingServiceTest.cfc | 56 +++++++ .../specs/SSOAuthorizationResponseSpec.cfc | 62 ++++++++ 11 files changed, 394 insertions(+), 40 deletions(-) create mode 100644 test-harness/tests/resources/prefixedSAMLResponse.xml diff --git a/changelog.md b/changelog.md index 4cae4fc..c80d7db 100644 --- a/changelog.md +++ b/changelog.md @@ -9,6 +9,28 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Added + +- `ISSOAuthorizationResponse.getClaims()` and `getClaim( name, defaultValue )` expose everything the IdP + asserted, keyed by the name the IdP used - the WS-Federation claim URIs for SAML, the id token or user + info keys for oAuth. The typed getters are a lowest common denominator of the four providers, so an + Entra group or role claim, a Google `hd`, or a customer's employee-number claim had nowhere to go and no + way to be read: a consumer had to re-parse `getRawResponseData()` itself. One map means the interface + does not grow a getter per claim, and it reads the same way for a SAML attribute as for an oAuth claim. +- `ISSOAuthorizationResponse.getNameId()` and `getNameIdFormat()` expose the Subject's NameID, which no + attribute can substitute for and which the response could not reach at all. The Format comes with it + because it decides what the value means: Entra's default is a pairwise identifier scoped to one app + registration, so the same person arrives under a different NameID at a second registration in the same + tenant. Treat one as an identifier without reading the Format and you have keyed identity to a value + that is not portable. +- `SAMLParsingService.extractUserInfo()` returns `claims`, `nameId` and `nameIdFormat` alongside the + existing fields. A claim always holds an array, since a SAML attribute may carry several + AttributeValues - Entra's `authnmethodsreferences` and its group claims do - and an IdP may split one + claim across repeated `Attribute` elements. Values are trimmed, which pretty-printed assertions need. +- `MicrosoftSAMLProvider` sets the claims on the success path only. An assertion whose signature did not + verify has asserted nothing, so a consumer reading a claim off a failed response would be trusting + whoever sent it rather than the IdP. + ### Changed - **BREAKING** `SSOAuthorizationResponse.getName()` returned `FirstName` instead of `Name`, so the value @@ -21,6 +43,13 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- `SAMLParsingService` matched `//Attribute[@Name='...']`, which only resolves when the assertion carries + the SAML namespace as its default - `extractUserInfo()` strips default namespace declarations, and + nothing else. An IdP that prefixes its elements, as ADFS and Shibboleth do and Entra can be configured + to, therefore yielded no first name, surname or object identifier, and the whole response was reported + as `Failed to extract user information`. The typed fields are now derived from the claim set, which is + matched on `local-name()`. + - [#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 diff --git a/models/ISSOAuthorizationResponse.cfc b/models/ISSOAuthorizationResponse.cfc index 1c4e68e..2fe1e96 100644 --- a/models/ISSOAuthorizationResponse.cfc +++ b/models/ISSOAuthorizationResponse.cfc @@ -9,5 +9,9 @@ interface { public string function getLastName(); public any function getRawResponseData(); public string function getErrorMessage(); + public struct function getClaims(); + public string function getClaim( required string name, string defaultValue ); + public string function getNameId(); + public string function getNameIdFormat(); } diff --git a/models/SSOAuthorizationResponse.cfc b/models/SSOAuthorizationResponse.cfc index 0045f72..a9b0d04 100644 --- a/models/SSOAuthorizationResponse.cfc +++ b/models/SSOAuthorizationResponse.cfc @@ -9,6 +9,9 @@ component implements="cbsso.models.ISSOAuthorizationResponse" accessors=true { property name="LastName"; property name="RawResponseData"; property name="ErrorMessage"; + property name="Claims"; + property name="NameId"; + property name="NameIdFormat"; /** * Seeds every property, so a response that only ever had its failure fields populated still @@ -23,6 +26,9 @@ component implements="cbsso.models.ISSOAuthorizationResponse" accessors=true { variables.LastName = ""; variables.ErrorMessage = ""; variables.RawResponseData = {}; + variables.Claims = {}; + variables.NameId = ""; + variables.NameIdFormat = ""; return this; } @@ -82,4 +88,78 @@ component implements="cbsso.models.ISSOAuthorizationResponse" accessors=true { return variables.ErrorMessage; } + /** + * Everything the IdP asserted, keyed by the name it used - the WS-Federation claim URIs for SAML, the + * id token or user info keys for oAuth. The typed getters above cover what every provider has in + * common; this is where anything else lives, so reaching a group, role or employee-number claim does + * not need a getter of its own. + */ + public struct function getClaims(){ + return variables.Claims; + } + + /** + * The first value of a claim, which is what a caller wants in all but the multi-valued case. Struct + * keys are case-insensitive, so the name does not have to match the IdP's casing. + */ + public string function getClaim( required string name, string defaultValue = "" ){ + if ( !variables.Claims.keyExists( arguments.name ) || !variables.Claims[ arguments.name ].len() ) { + return arguments.defaultValue; + } + + return variables.Claims[ arguments.name ][ 1 ]; + } + + /** + * Normalised here rather than in each provider, so `getClaims()` reads the same way whatever produced + * it: every claim holds an array, because a SAML attribute and an oAuth claim can both be + * multi-valued. Values that are not simple - a nested object in an id token - are left out, and stay + * reachable on `getRawResponseData()`. + */ + public any function setClaims( required struct claims ){ + var normalised = {}; + + for ( var name in arguments.claims ) { + var value = arguments.claims[ name ]; + + if ( isSimpleValue( value ) ) { + normalised[ name ] = [ toString( value ) ]; + continue; + } + + if ( !isArray( value ) ) { + continue; + } + + normalised[ name ] = []; + + for ( var entry in value ) { + if ( isSimpleValue( entry ) ) { + normalised[ name ].append( toString( entry ) ); + } + } + } + + variables.Claims = normalised; + + return this; + } + + /** + * The Subject's NameID, which SAML always carries and no claim can substitute for. Empty for oAuth + * providers, and for a SAML assertion that identifies its subject by attribute alone. + */ + public string function getNameId(){ + return variables.NameId; + } + + /** + * The NameID's Format. Read it before treating a NameID as an identifier: Entra's default is a + * pairwise value scoped to one app registration, so the same person arrives under a different NameID + * at a second registration in the same tenant. + */ + public string function getNameIdFormat(){ + return variables.NameIdFormat; + } + } diff --git a/models/providers/FacebookProvider.cfc b/models/providers/FacebookProvider.cfc index 265b7bb..b437a2e 100644 --- a/models/providers/FacebookProvider.cfc +++ b/models/providers/FacebookProvider.cfc @@ -74,6 +74,7 @@ component .setLastName( idTokenData.family_name ) .setEmail( idTokenData.email ) .setUserId( idTokenData.sub ) + .setClaims( idTokenData ) } catch ( any e ) { return authResponse.setWasSuccessful( false ).setErrorMessage( e.message ); } diff --git a/models/providers/GitHubProvider.cfc b/models/providers/GitHubProvider.cfc index d66e022..887527a 100644 --- a/models/providers/GitHubProvider.cfc +++ b/models/providers/GitHubProvider.cfc @@ -77,7 +77,8 @@ component .setWasSuccessful( true ) .setName( userData.name ) .setEmail( userData.email ) - .setUserId( userData.id ); + .setUserId( userData.id ) + .setClaims( userData ); } catch ( any e ) { return authResponse.setWasSuccessful( false ).setErrorMessage( e.message ); } diff --git a/models/providers/GoogleProvider.cfc b/models/providers/GoogleProvider.cfc index 7127337..4107afe 100644 --- a/models/providers/GoogleProvider.cfc +++ b/models/providers/GoogleProvider.cfc @@ -69,6 +69,7 @@ component .setLastName( idTokenData.family_name ) .setEmail( idTokenData.email ) .setUserId( idTokenData.sub ) + .setClaims( idTokenData ) } catch ( any e ) { return authResponse.setWasSuccessful( false ).setErrorMessage( e.message ); } diff --git a/models/providers/MicrosoftSAMLProvider.cfc b/models/providers/MicrosoftSAMLProvider.cfc index 50009a2..e22935d 100644 --- a/models/providers/MicrosoftSAMLProvider.cfc +++ b/models/providers/MicrosoftSAMLProvider.cfc @@ -73,12 +73,17 @@ component .setErrorMessage( samlData.errorMessage ); } + // Set only here, not on the failure returns above: an assertion whose signature did not verify + // has asserted nothing, and a consumer reading a claim off it would be trusting the sender. return authResponse .setWasSuccessful( true ) .setFirstName( samlData.firstName ) .setLastName( samlData.lastName ) .setEmail( samlData.email ) .setUserId( samlData.userId ) + .setClaims( samlData.claims ) + .setNameId( samlData.nameId ) + .setNameIdFormat( samlData.nameIdFormat ) .setRawResponseData( data ); } catch ( any e ) { return authResponse.setWasSuccessful( false ).setErrorMessage( e.message ); diff --git a/models/utility/SAMLParsingService.cfc b/models/utility/SAMLParsingService.cfc index f3e6805..fd403ce 100644 --- a/models/utility/SAMLParsingService.cfc +++ b/models/utility/SAMLParsingService.cfc @@ -1,5 +1,17 @@ component singleton { + /** + * The WS-Federation and Microsoft claim URIs the typed fields are derived from. Every other attribute + * the IdP asserted is reachable through `claims`, under the name the IdP used. + */ + variables.claimNames = { + "givenName" : "http://schemas.xmlsoap.org/ws/2005/05/identity/claims/givenname", + "surname" : "http://schemas.xmlsoap.org/ws/2005/05/identity/claims/surname", + "name" : "http://schemas.xmlsoap.org/ws/2005/05/identity/claims/name", + "emailAddress" : "http://schemas.xmlsoap.org/ws/2005/05/identity/claims/emailaddress", + "objectIdentifier" : "http://schemas.microsoft.com/identity/claims/objectidentifier" + }; + public struct function extractUserInfo( required string rawSAMLResponse ){ var data = { "success" : false, @@ -8,7 +20,10 @@ component singleton { "firstName" : "", "lastName" : "", "email" : "", - "userId" : "" + "userId" : "", + "nameId" : "", + "nameIdFormat" : "", + "claims" : {} }; var xmlData = xmlParse( rawSAMLResponse.reReplace( "xmlns="".+?""", "", "all" ) ); @@ -21,10 +36,18 @@ component singleton { } try { - data.firstName = extractFirstName( xmlData ); - data.lastName = extractLastName( xmlData ); - data.email = extractEmail( xmlData ); - data.userId = extractUserId( xmlData ); + var subject = extractSubjectNameId( xmlData ); + + // Populated before the required claims are read, so a response that fails on a missing + // one still reports what the IdP actually asserted. + data.claims = extractClaims( xmlData ); + data.nameId = subject.value; + data.nameIdFormat = subject.format; + + data.firstName = requiredClaim( data.claims, variables.claimNames.givenName ); + data.lastName = requiredClaim( data.claims, variables.claimNames.surname ); + data.email = extractEmail( data.claims ); + data.userId = requiredClaim( data.claims, variables.claimNames.objectIdentifier ); return data; } catch ( any e ) { @@ -42,13 +65,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, "//*" ); @@ -64,47 +97,82 @@ component singleton { } } - private string function extractFirstName( required xmlDoc ){ - return xmlSearch( - xmlDoc, - "//Attribute[@Name='http://schemas.xmlsoap.org/ws/2005/05/identity/claims/givenname']" - )[ 1 ].xmlchildren[ 1 ].xmltext; - } + /** + * Every asserted attribute, keyed by its `Name` and always holding an array - a claim may carry more + * than one AttributeValue (Entra group and role claims routinely do), and an IdP may split one claim + * across repeated Attribute elements. + */ + private struct function extractClaims( required xmlDoc ){ + var claims = {}; - private string function extractLastName( required xmlDoc ){ - return xmlSearch( - xmlDoc, - "//Attribute[@Name='http://schemas.xmlsoap.org/ws/2005/05/identity/claims/surname']" - )[ 1 ].xmlchildren[ 1 ].xmltext; - } + for ( var node in xmlSearch( xmlDoc, "//*[local-name()='Attribute'][@Name]" ) ) { + var name = trim( node.xmlAttributes.Name ); - private string function extractEmail( required xmlDoc ){ - // try emailAddress claim first, then fallback to name claim if emailAddress is not present - var emailNodes = xmlSearch( - xmlDoc, - "//Attribute[@Name='http://schemas.xmlsoap.org/ws/2005/05/identity/claims/emailaddress']" - ); + if ( !len( name ) ) { + continue; + } + + if ( !claims.keyExists( name ) ) { + claims[ name ] = []; + } - if ( arrayLen( emailNodes ) > 0 ) { - return emailNodes[ 1 ].xmlchildren[ 1 ].xmltext; + for ( var valueNode in node.xmlChildren ) { + if ( listLast( valueNode.xmlName, ":" ) == "AttributeValue" ) { + claims[ name ].append( trim( valueNode.xmlText ) ); + } + } } - var nameNodes = xmlSearch( - xmlDoc, - "//Attribute[@Name='http://schemas.xmlsoap.org/ws/2005/05/identity/claims/name']" - ); - if ( arrayLen( nameNodes ) > 0 ) { - return nameNodes[ 1 ].xmlchildren[ 1 ].xmltext; + return claims; + } + + /** + * The Format matters as much as the value: Entra's default is a pairwise identifier scoped to the app + * registration, stable within that registration and meaningless outside it. A consumer cannot tell a + * portable identifier from a scoped one without it. + */ + private struct function extractSubjectNameId( required xmlDoc ){ + var nodes = xmlSearch( xmlDoc, "//*[local-name()='Subject']/*[local-name()='NameID']" ); + + if ( !nodes.len() ) { + return { "value" : "", "format" : "" }; } - return ""; + var attributes = nodes[ 1 ].xmlAttributes; + + return { + "value" : trim( nodes[ 1 ].xmlText ), + "format" : attributes.keyExists( "Format" ) ? trim( attributes.Format ) : "" + }; } - private string function extractUserId( required xmlDoc ){ - return xmlSearch( - xmlDoc, - "//Attribute[@Name='http://schemas.microsoft.com/identity/claims/objectidentifier']" - )[ 1 ].xmlchildren[ 1 ].xmltext; + /** + * Falls back to the `name` claim, which carries the UPN when no email claim is mapped. + */ + private string function extractEmail( required struct claims ){ + var email = claimValue( claims, variables.claimNames.emailAddress ); + + return len( email ) ? email : claimValue( claims, variables.claimNames.name ); + } + + private string function claimValue( required struct claims, required string name ){ + return claims.keyExists( name ) && claims[ name ].len() ? claims[ name ][ 1 ] : ""; + } + + /** + * Still throws when the claim is absent, so an assertion missing one of the values the typed fields + * are built from fails exactly as it did before the claim set was exposed. Whether a missing + * display-name claim should fail a login at all is a separate question from reaching the claims. + */ + private string function requiredClaim( required struct claims, required string name ){ + if ( !claims.keyExists( name ) ) { + throw( + type = "SAMLParsingService.MissingClaim", + message = "The assertion contains no '#name#' claim." + ); + } + + return claimValue( claims, name ); } } diff --git a/test-harness/tests/resources/prefixedSAMLResponse.xml b/test-harness/tests/resources/prefixedSAMLResponse.xml new file mode 100644 index 0000000..3b76c22 --- /dev/null +++ b/test-harness/tests/resources/prefixedSAMLResponse.xml @@ -0,0 +1,47 @@ + + + https://sts.windows.net/2b263285-61e2-49c4-a257-8234f38486a2/ + + + + + https://sts.windows.net/2b263285-61e2-49c4-a257-8234f38486a2/ + + + V3JpdHRlbkJ5T3J0dXNTb2x1dGlvbnM9 + + + + + + + 0c8f4a52-1b7d-4e39-9f6a-3d2c5b8e7a14 + + + Ada + + + Lovelace + + + ada.lovelace@example.com + + + Analysts + Engineering + + + A1B2C3 + + + + diff --git a/test-harness/tests/specs/SAMLParsingServiceTest.cfc b/test-harness/tests/specs/SAMLParsingServiceTest.cfc index 6c2c566..e214a6d 100644 --- a/test-harness/tests/specs/SAMLParsingServiceTest.cfc +++ b/test-harness/tests/specs/SAMLParsingServiceTest.cfc @@ -48,6 +48,62 @@ component extends="coldbox.system.testing.BaseTestCase" { expect( result.email ).toBe( "jbeers@ortussolutions.com" ); } ); + it( "returns every asserted attribute, not only the ones with a typed field", function(){ + var rawSAMLResponse = fileRead( expandPath( "/tests/resources/validSAMLResponse.xml" ) ); + var result = service.extractUserInfo( rawSAMLResponse ); + + expect( result.claims ).toBeStruct(); + expect( result.claims ).toHaveKey( "http://schemas.microsoft.com/identity/claims/tenantid" ); + expect( result.claims[ "http://schemas.microsoft.com/identity/claims/displayname" ] ).toBe( [ "Jacob Beers" ] ); + } ); + + it( "keeps every value of a multi-valued claim", function(){ + var rawSAMLResponse = fileRead( expandPath( "/tests/resources/validSAMLResponse.xml" ) ); + var result = service.extractUserInfo( rawSAMLResponse ); + + // Entra sends three authentication methods here, each pretty-printed onto its own line + expect( result.claims[ "http://schemas.microsoft.com/claims/authnmethodsreferences" ] ).toBe( [ + "http://schemas.microsoft.com/ws/2008/06/identity/authenticationmethod/password", + "http://schemas.microsoft.com/claims/multipleauthn", + "http://schemas.microsoft.com/ws/2008/06/identity/authenticationmethod/unspecified" + ] ); + } ); + + it( "extracts the subject NameID and its format", function(){ + var rawSAMLResponse = fileRead( expandPath( "/tests/resources/validSAMLResponse.xml" ) ); + var result = service.extractUserInfo( rawSAMLResponse ); + + expect( result.nameId ).toBe( "pO+tkeMWqlmQJ6WmA1k2HOVlYfBGf0CnHApnDU9cGTk=" ); + expect( result.nameIdFormat ).toBe( "urn:oasis:names:tc:SAML:2.0:nameid-format:transient" ); + } ); + + it( "extracts from an assertion whose elements are namespace-prefixed", function(){ + var rawSAMLResponse = fileRead( expandPath( "/tests/resources/prefixedSAMLResponse.xml" ) ); + var result = service.extractUserInfo( rawSAMLResponse ); + + expect( result.success ).toBeTrue(); + expect( result.firstName ).toBe( "Ada" ); + expect( result.lastName ).toBe( "Lovelace" ); + expect( result.email ).toBe( "ada.lovelace@example.com" ); + expect( result.userId ).toBe( "0c8f4a52-1b7d-4e39-9f6a-3d2c5b8e7a14" ); + expect( result.nameId ).toBe( "V3JpdHRlbkJ5T3J0dXNTb2x1dGlvbnM9" ); + expect( result.nameIdFormat ).toBe( "urn:oasis:names:tc:SAML:2.0:nameid-format:persistent" ); + expect( result.claims[ "http://schemas.microsoft.com/ws/2008/06/identity/claims/groups" ] ).toBe( [ "Analysts", "Engineering" ] ); + expect( result.claims[ "https://example.com/claims/employeenumber" ] ).toBe( [ "A1B2C3" ] ); + } ); + + it( "reports what was asserted even when a claim a typed field needs is missing", function(){ + var rawSAMLResponse = fileRead( expandPath( "/tests/resources/prefixedSAMLResponse.xml" ) ).replace( + "http://schemas.xmlsoap.org/ws/2005/05/identity/claims/surname", + "https://example.com/claims/notasurname" + ); + var result = service.extractUserInfo( rawSAMLResponse ); + + expect( result.success ).toBeFalse(); + expect( result.errorMessage ).toStartWith( "Failed to extract user information:" ); + expect( result.claims ).toHaveKey( "https://example.com/claims/notasurname" ); + } ); + it( "should return an error message from the xml", function(){ var rawSAMLResponse = fileRead( expandPath( "/tests/resources/errorSAMLResponse.xml" ) ); var result = service.extractUserInfo( rawSAMLResponse ); diff --git a/test-harness/tests/specs/SSOAuthorizationResponseSpec.cfc b/test-harness/tests/specs/SSOAuthorizationResponseSpec.cfc index d6f33c5..9bdf7d2 100644 --- a/test-harness/tests/specs/SSOAuthorizationResponseSpec.cfc +++ b/test-harness/tests/specs/SSOAuthorizationResponseSpec.cfc @@ -44,6 +44,68 @@ component extends="coldbox.system.testing.BaseTestCase" { expect( response.getName() ).toBe( "" ); expect( response.getFirstName() ).toBe( "" ); expect( response.getLastName() ).toBe( "" ); + expect( response.getClaims() ).toBe( {} ); + expect( response.getClaim( "urn:oid:0.9.2342.19200300.100.1.1" ) ).toBe( "" ); + expect( response.getNameId() ).toBe( "" ); + expect( response.getNameIdFormat() ).toBe( "" ); + } ); + + it( "returns the caller's default for a claim the IdP did not assert", function(){ + response.setClaims( { "email" : "jdoe@example.com" } ); + + expect( response.getClaim( "employeeNumber", "unknown" ) ).toBe( "unknown" ); + } ); + + it( "holds every claim as an array, whatever the provider handed it", function(){ + response.setClaims( { + "email" : "jdoe@example.com", + "groups" : [ "Analysts", "Engineering" ] + } ); + + expect( response.getClaims() ).toBe( { + "email" : [ "jdoe@example.com" ], + "groups" : [ "Analysts", "Engineering" ] + } ); + } ); + + it( "returns the first value of a multi-valued claim", function(){ + response.setClaims( { "groups" : [ "Analysts", "Engineering" ] } ); + + expect( response.getClaim( "groups" ) ).toBe( "Analysts" ); + expect( response.getClaims()[ "groups" ] ).toHaveLength( 2 ); + } ); + + it( "reads a claim back under any casing, since the IdP chooses the name", function(){ + response.setClaims( { "employeeNumber" : "A1B2C3" } ); + + expect( response.getClaim( "EMPLOYEENUMBER" ) ).toBe( "A1B2C3" ); + } ); + + it( "stringifies simple values, so a claim always reads as a string", function(){ + response.setClaims( { "emailVerified" : true, "authTime" : 1767225600 } ); + + expect( response.getClaim( "emailVerified" ) ).toBe( "true" ); + expect( response.getClaim( "authTime" ) ).toBe( "1767225600" ); + } ); + + it( "leaves out a claim whose value is not simple - a nested object in an id token", function(){ + response.setClaims( { + "email" : "jdoe@example.com", + "address" : { "locality" : "Houston" } + } ); + + expect( response.getClaims() ).notToHaveKey( "address" ); + expect( response.getClaim( "address" ) ).toBe( "" ); + expect( response.getClaims() ).toHaveKey( "email" ); + } ); + + it( "reads back the NameID and its format", function(){ + response + .setNameId( "V3JpdHRlbkJ5T3J0dXNTb2x1dGlvbnM9" ) + .setNameIdFormat( "urn:oasis:names:tc:SAML:2.0:nameid-format:persistent" ); + + expect( response.getNameId() ).toBe( "V3JpdHRlbkJ5T3J0dXNTb2x1dGlvbnM9" ); + expect( response.getNameIdFormat() ).toBe( "urn:oasis:names:tc:SAML:2.0:nameid-format:persistent" ); } ); it( "reads back a Name set without any FirstName - as GitHubProvider does", function(){ From c305a293beb45815553544041f369ec3a66ee60b Mon Sep 17 00:00:00 2001 From: Doug Cain Date: Wed, 5 Aug 2026 08:29:51 +0100 Subject: [PATCH 2/2] fix: stop requiring claims SAML 2.0 does not require SAMLParsingService treated a missing givenname, surname or objectidentifier claim as fatal and failed the whole response. SAML 2.0 requires none of them. An AttributeStatement is optional throughout Core, and the Web Browser SSO profile asks only for a Subject carrying a bearer SubjectConfirmation (Profiles 4.1.4.2) - even a NameID is not mandatory there, only implied when the SP sends a NameIDPolicy the IdP must honour or fail with InvalidNameIDPolicy. All three names are WS-Federation or Microsoft URIs, which is to say they are Entra's dialect rather than the protocol's. So the module refused a conformant assertion from ADFS, Shibboleth or Okta over a display name, while ignoring the identifier the profile does point at. That is the wrong way round: the display names are the optional part and the subject identifier is not. The display-name claims are now optional and yield empty strings. The subject is identified by the objectidentifier claim where the IdP asserts one, since it is stable across app registrations, and by the Subject's NameID where it does not - which is the only value a minimally conformant assertion carries. A transient NameID is not accepted. The specification defines it as valid for a single session, so keying identity to one enrols the same person again on every login, which is worse than refusing the login. An assertion carrying nothing else fails with SAMLParsingService.NoSubjectIdentifier and a message naming which of the two was missing. Whether the identifier that is returned is portable stays the caller's to judge from nameIdFormat: Entra's persistent NameID is pairwise, scoped to one app registration. Email gains one fallback for the same reason - a NameID whose Format is the emailAddress format, which is what an IdP that federates on email rather than on attributes sends. An empty email is not fatal; it never was, and it is a claim like any other. Breaking for a consumer that assumed a successful response carries a first name: firstName and lastName may now be empty, where before they were populated or the response failed. 33 specs pass on both CI engines: BoxLang 1.17.0-snapshot and Lucee 5.4.8.2. --- changelog.md | 14 ++++ models/utility/SAMLParsingService.cfc | 75 +++++++++++++------ .../tests/specs/SAMLParsingServiceTest.cfc | 74 ++++++++++++++++-- 3 files changed, 136 insertions(+), 27 deletions(-) diff --git a/changelog.md b/changelog.md index c80d7db..392ad19 100644 --- a/changelog.md +++ b/changelog.md @@ -40,6 +40,20 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 will now receive a full name. - `Auth` resolves the requested provider in a `preHandler`, which stores it in `prc.ssoProvider` for every action instead of each action resolving it for itself. +- **BREAKING** `SAMLParsingService` treated a missing `givenname`, `surname` or `objectidentifier` claim as + a fatal error, failing the whole response. SAML 2.0 requires none of them: an `` is + optional throughout Core, the Web Browser SSO profile (Profiles 4.1.4.2) asks only for a `` + carrying a bearer ``, and all three names are WS-Federation or Microsoft URIs that + only an Entra-shaped IdP asserts - so a conformant assertion from ADFS, Shibboleth or Okta was refused + over a display name, while the identifier the profile does point at went unread. The display names are + now optional, and the subject is identified by the `objectidentifier` claim where the IdP asserts one and + by the Subject's NameID where it does not. `firstName` and `lastName` may now be empty on a successful + response, where before they were either populated or the response failed. +- A transient NameID is not accepted as a subject identifier, and an assertion carrying no other is + refused with `SAMLParsingService.NoSubjectIdentifier`. The specification defines a transient identifier + as valid for a single session, so keying identity to one enrols the same person again on every login. + Whether the identifier that *is* returned is portable remains the caller's to judge from `nameIdFormat`: + Entra's persistent NameID is pairwise, scoped to one app registration. ### Fixed diff --git a/models/utility/SAMLParsingService.cfc b/models/utility/SAMLParsingService.cfc index fd403ce..8d98fc4 100644 --- a/models/utility/SAMLParsingService.cfc +++ b/models/utility/SAMLParsingService.cfc @@ -12,6 +12,11 @@ component singleton { "objectIdentifier" : "http://schemas.microsoft.com/identity/claims/objectidentifier" }; + variables.nameIdFormats = { + "transient" : "urn:oasis:names:tc:SAML:2.0:nameid-format:transient", + "emailAddress" : "urn:oasis:names:tc:SAML:1.1:nameid-format:emailAddress" + }; + public struct function extractUserInfo( required string rawSAMLResponse ){ var data = { "success" : false, @@ -38,16 +43,16 @@ component singleton { try { var subject = extractSubjectNameId( xmlData ); - // Populated before the required claims are read, so a response that fails on a missing - // one still reports what the IdP actually asserted. + // Populated before the subject is resolved, so a response that fails to identify one + // still reports what the IdP actually asserted. data.claims = extractClaims( xmlData ); data.nameId = subject.value; data.nameIdFormat = subject.format; - data.firstName = requiredClaim( data.claims, variables.claimNames.givenName ); - data.lastName = requiredClaim( data.claims, variables.claimNames.surname ); - data.email = extractEmail( data.claims ); - data.userId = requiredClaim( data.claims, variables.claimNames.objectIdentifier ); + data.firstName = claimValue( data.claims, variables.claimNames.givenName ); + data.lastName = claimValue( data.claims, variables.claimNames.surname ); + data.email = extractEmail( data.claims, subject ); + data.userId = extractUserId( data.claims, subject ); return data; } catch ( any e ) { @@ -147,32 +152,58 @@ component singleton { } /** - * Falls back to the `name` claim, which carries the UPN when no email claim is mapped. + * Falls back to the `name` claim, which carries the UPN when no email claim is mapped, and then to the + * NameID when its Format says the value is an email address - the format an IdP that federates on email + * rather than on attributes will use. */ - private string function extractEmail( required struct claims ){ + private string function extractEmail( required struct claims, required struct subject ){ var email = claimValue( claims, variables.claimNames.emailAddress ); - return len( email ) ? email : claimValue( claims, variables.claimNames.name ); - } + if ( !len( email ) ) { + email = claimValue( claims, variables.claimNames.name ); + } - private string function claimValue( required struct claims, required string name ){ - return claims.keyExists( name ) && claims[ name ].len() ? claims[ name ][ 1 ] : ""; + if ( !len( email ) && subject.format == variables.nameIdFormats.emailAddress ) { + email = subject.value; + } + + return email; } /** - * Still throws when the claim is absent, so an assertion missing one of the values the typed fields - * are built from fails exactly as it did before the claim set was exposed. Whether a missing - * display-name claim should fail a login at all is a separate question from reaching the claims. + * SAML 2.0 requires none of the attributes the typed fields are read from. An AttributeStatement is + * optional throughout Core, and the Web Browser SSO profile (Profiles 4.1.4.2) asks only for a Subject + * carrying a bearer SubjectConfirmation. The names read here are WS-Federation and Microsoft URIs that + * only an Entra-shaped IdP asserts, so treating one as mandatory rejects a conformant assertion from + * ADFS, Shibboleth or Okta over a display name. + * + * What a consumer cannot do without is something to identify the subject by, so that is the only thing + * this refuses on. The object identifier is preferred because it is stable across app registrations, + * and the NameID stands in when it is absent - unless it is transient, which the specification defines + * as valid for a single session, so keying identity to it would enrol the same person again on every + * login. Whether the value it does return is portable is the caller's to judge from `nameIdFormat`. */ - private string function requiredClaim( required struct claims, required string name ){ - if ( !claims.keyExists( name ) ) { - throw( - type = "SAMLParsingService.MissingClaim", - message = "The assertion contains no '#name#' claim." - ); + private string function extractUserId( required struct claims, required struct subject ){ + var objectIdentifier = claimValue( claims, variables.claimNames.objectIdentifier ); + + if ( len( objectIdentifier ) ) { + return objectIdentifier; + } + + if ( len( subject.value ) && subject.format != variables.nameIdFormats.transient ) { + return subject.value; } - return claimValue( claims, name ); + var reason = len( subject.value ) ? "its NameID is transient" : "it carries no NameID"; + + throw( + type = "SAMLParsingService.NoSubjectIdentifier", + message = "The assertion identifies no subject: it asserts no '#variables.claimNames.objectIdentifier#' claim, and #reason#." + ); + } + + private string function claimValue( required struct claims, required string name ){ + return claims.keyExists( name ) && claims[ name ].len() ? claims[ name ][ 1 ] : ""; } } diff --git a/test-harness/tests/specs/SAMLParsingServiceTest.cfc b/test-harness/tests/specs/SAMLParsingServiceTest.cfc index e214a6d..46f9f9e 100644 --- a/test-harness/tests/specs/SAMLParsingServiceTest.cfc +++ b/test-harness/tests/specs/SAMLParsingServiceTest.cfc @@ -92,16 +92,80 @@ component extends="coldbox.system.testing.BaseTestCase" { expect( result.claims[ "https://example.com/claims/employeenumber" ] ).toBe( [ "A1B2C3" ] ); } ); - it( "reports what was asserted even when a claim a typed field needs is missing", function(){ + it( "accepts an assertion asserting no display-name claims, which SAML never required", function(){ + var rawSAMLResponse = fileRead( expandPath( "/tests/resources/prefixedSAMLResponse.xml" ) ) + .replace( + "http://schemas.xmlsoap.org/ws/2005/05/identity/claims/givenname", + "https://example.com/claims/notagivenname" + ) + .replace( + "http://schemas.xmlsoap.org/ws/2005/05/identity/claims/surname", + "https://example.com/claims/notasurname" + ); + var result = service.extractUserInfo( rawSAMLResponse ); + + expect( result.success ).toBeTrue(); + expect( result.firstName ).toBe( "" ); + expect( result.lastName ).toBe( "" ); + expect( result.userId ).toBe( "0c8f4a52-1b7d-4e39-9f6a-3d2c5b8e7a14" ); + expect( result.claims ).toHaveKey( "https://example.com/claims/notasurname" ); + } ); + + it( "identifies the subject by its NameID when no object identifier is asserted", function(){ var rawSAMLResponse = fileRead( expandPath( "/tests/resources/prefixedSAMLResponse.xml" ) ).replace( - "http://schemas.xmlsoap.org/ws/2005/05/identity/claims/surname", - "https://example.com/claims/notasurname" + "http://schemas.microsoft.com/identity/claims/objectidentifier", + "https://example.com/claims/notanobjectidentifier" + ); + var result = service.extractUserInfo( rawSAMLResponse ); + + expect( result.success ).toBeTrue(); + expect( result.userId ).toBe( "V3JpdHRlbkJ5T3J0dXNTb2x1dGlvbnM9" ); + expect( result.nameIdFormat ).toBe( "urn:oasis:names:tc:SAML:2.0:nameid-format:persistent" ); + } ); + + it( "refuses to identify the subject by a transient NameID", function(){ + var rawSAMLResponse = fileRead( expandPath( "/tests/resources/validSAMLResponse.xml" ) ).replace( + "http://schemas.microsoft.com/identity/claims/objectidentifier", + "https://example.com/claims/notanobjectidentifier" ); var result = service.extractUserInfo( rawSAMLResponse ); expect( result.success ).toBeFalse(); - expect( result.errorMessage ).toStartWith( "Failed to extract user information:" ); - expect( result.claims ).toHaveKey( "https://example.com/claims/notasurname" ); + expect( result.errorMessage ).toInclude( "identifies no subject" ); + expect( result.errorMessage ).toInclude( "transient" ); + } ); + + it( "reports what was asserted even when nothing in it identifies the subject", function(){ + // Renaming the element leaves a Subject holding only its bearer SubjectConfirmation, which + // is all the Web Browser SSO profile requires of one + var rawSAMLResponse = fileRead( expandPath( "/tests/resources/prefixedSAMLResponse.xml" ) ) + .replace( "NameID", "Unidentified", "all" ) + .replace( + "http://schemas.microsoft.com/identity/claims/objectidentifier", + "https://example.com/claims/notanobjectidentifier" + ); + var result = service.extractUserInfo( rawSAMLResponse ); + + expect( result.success ).toBeFalse(); + expect( result.errorMessage ).toInclude( "carries no NameID" ); + expect( result.claims ).toHaveKey( "https://example.com/claims/notanobjectidentifier" ); + } ); + + it( "reads the email from a NameID whose format says it is one", function(){ + var rawSAMLResponse = fileRead( expandPath( "/tests/resources/prefixedSAMLResponse.xml" ) ) + .replace( + "urn:oasis:names:tc:SAML:2.0:nameid-format:persistent", + "urn:oasis:names:tc:SAML:1.1:nameid-format:emailAddress" + ) + .replace( "V3JpdHRlbkJ5T3J0dXNTb2x1dGlvbnM9", "ada@example.com" ) + .replace( + "http://schemas.xmlsoap.org/ws/2005/05/identity/claims/emailaddress", + "https://example.com/claims/notanemail" + ); + var result = service.extractUserInfo( rawSAMLResponse ); + + expect( result.success ).toBeTrue(); + expect( result.email ).toBe( "ada@example.com" ); } ); it( "should return an error message from the xml", function(){