fix: stop requiring claims SAML 2.0 does not require - #21
Open
dougcain wants to merge 2 commits 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.
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.
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
1 participant
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.
The problem
SAMLParsingServicetreats a missinggivenname,surnameorobjectidentifierclaim as fatal and failsthe whole response. SAML 2.0 requires none of them:
<Subject>is[Optional]on<Assertion>, and<AttributeStatement>is optionalthroughout.
<Subject>, if present, needs either an identifier (BaseID/NameID/EncryptedID) or a<SubjectConfirmation>. Either one satisfies it.<Subject>carrying a bearer<SubjectConfirmation>withRecipient/NotOnOrAfter, plus an<AudienceRestriction>and an<AuthnStatement>. Still nothing requiring aNameID, let alone an attribute.NameIDbecomes obligatory is<NameIDPolicy>on the AuthnRequest: ask for a Format theIdP cannot produce and it must fail with
…status:InvalidNameIDPolicyrather than substitute one. It isthe SP's request that makes it mandatory, not the protocol.
And all three names the module insists on are WS-Federation or Microsoft URIs — Entra's dialect, not the
protocol's. So a conformant assertion from ADFS, Shibboleth or Okta is refused over a display name, while
the identifier the profile actually points at goes unread. That is the wrong way round.
The change
firstNameandlastNameyield""when the IdP asserts nogivenname/surname, rather than failing the login.getName()already copes with a missing first name.objectidentifierwhere the IdP asserts one — preferred because it isstable across app registrations — and by the Subject's
NameIDwhere it does not. That is the onlyidentifier a minimally conformant assertion carries.
single session, so keying identity to one enrols the same person again on every login — worse than
refusing. An assertion carrying nothing else fails with
SAMLParsingService.NoSubjectIdentifierand amessage naming which of the two was missing, so the admin reading the log knows what to map in the IdP.
nameIdFormat(feat: expose the IdP's claim set and the Subject NameID #20). Entra's persistent NameID is pairwise, scoped to one app registration.
Formatisurn:oasis:names:tc:SAML:1.1:nameid-format:emailAddress, which is what an IdP federating on email ratherthan on attributes sends. An empty email is not fatal — it never was, and it is a claim like any other.
Net effect: the only thing that now fails a signature-valid,
Success-status assertion is having nothingin it that identifies the subject.
Breaking
firstNameandlastNamemay be empty on a successful response, where before they were either populatedor the response failed. A consumer that assumed a successful response carries a first name will now see
"". Noted as BREAKING in the changelog.Deliberately not changed
BaseIDandEncryptedIDare still not read.EncryptedIDneeds the SP's decryption key, which themodule has no configuration for, so honouring it is a feature rather than a fix.
BaseIDis abstract andhas no in-the-wild use I can point at.
…/identity/claims/displaynameor…/claims/name.Splitting a single display name into a first and last name is guesswork that gets Chinese and Spanish
naming orders wrong, and the whole name is reachable via
getClaim()now. Worth doing properly one dayas a
Nameon the response, not smuggled into this.NameIDPolicyis not sent on the AuthnRequest. Requestingpersistentexplicitly would be thematching change on the request side, but it alters what IdPs return for existing installs.
Testing
33 specs pass on both CI engines — BoxLang 1.17.0-snapshot and Lucee 5.4.8.2 — with
format:checkclean.New coverage: an assertion with no display-name claims succeeds; the NameID identifies the subject when no
objectidentifieris asserted; a transient NameID with noobjectidentifieris refused; an assertion whoseSubject holds only its bearer
SubjectConfirmationis refused and still reports its claim set; and emailis read from an
emailAddress-format NameID. The spec that pinned the old fatal-claim behaviour isreplaced rather than deleted.