feat: expose the IdP's claim set and the Subject NameID - #20
Open
dougcain wants to merge 1 commit into
Open
Conversation
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
ISSOAuthorizationResponse's typed getters are the intersection of what the 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 Googlehd, a customer's employee-number claim. A consumer that wants one has to re-parsegetRawResponseData()itself, 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.
What
response.getClaims() // { "<claim name>" : [ "value", ... ] } response.getClaim( "hd" ) // first value, or "" response.getClaim( "employeeNumber", "n/a" ) // ...or the caller's default response.getNameId() // the Subject's NameID response.getNameIdFormat() // ...and what it meansFour methods, closed to further growth, and they read 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.
MicrosoftSAMLProviderfills the map from the assertion'sAttributeStatement;GoogleProviderandFacebookProviderfrom the verified id token;GitHubProviderfrom the user info response.A claim always holds an array. A SAML attribute may carry several
AttributeValues — Entra'sauthnmethodsreferencesdoes, and its group claims do — and an IdP may split one claim across repeatedAttributeelements. The existing extraction takes 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 ongetRawResponseData().NameID gets its own getters rather than a slot in the map, because it is not an attribute and its
Formatchanges 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.Claims are set 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.
Also fixes prefixed assertions
Deriving the typed fields from the claim set fixes them for prefixed assertions as a side effect. 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 whole response came back asFailed to extract user information. The claim walk matches onlocal-name(), the same lesson as thesamlp:prefixes in #19.New fixture
prefixedSAMLResponse.xmlcovers it.Deliberately not changed
givenname,surnameorobjectidentifierclaim still fails the whole response, as it does today. 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. There is a spec pinning the current behaviour, including that the claim set is still reported on that path so you can see what the IdP actually sent.Nameonly, notFriendlyName. Shibboleth'surn:oid:names are unusable without a lookup andFriendlyNameis the human-readable key, so this may be worth adding — but two ways to reach one claim is ambiguity, and it seemed better asked than assumed.Notes
ISSOAuthorizationResponse, which is technically breaking for anyone implementing that interface outside the module —SSOAuthorizationResponseis the only implementation here.development, so it is independent of fix: restore OpenSAML class loading and make SAML parsing engine-portable #19. The two touch neighbouring hunks ofSAMLParsingService.cfc; whichever merges second wants a trivial rebase, and I am happy to do it.Testing
29 specs pass on both CI engines — BoxLang 1.17.0-snapshot and Lucee 5.4.8.2 — with
format:checkclean. New coverage: the full claim set from the existing Entra fixture, a multi-valued claim, NameID and Format, a prefixed assertion end to end, the missing-required-claim path, and the normalisation and defaulting rules on the response object.