Skip to content

Comments

Do not use deprecated NAMEID_EMAIL_ADDRESS as default for SAML2 logout#5907

Open
jdede wants to merge 2 commits intoBookStackApp:developmentfrom
jdede:fix-SLO
Open

Do not use deprecated NAMEID_EMAIL_ADDRESS as default for SAML2 logout#5907
jdede wants to merge 2 commits intoBookStackApp:developmentfrom
jdede:fix-SLO

Conversation

@jdede
Copy link

@jdede jdede commented Nov 20, 2025

For SAML 2.0 logout, the "NAMEID_EMAIL_ADDRESS" (urn:oasis:names:tc:SAML:1.1:nameid-format:emailAddress) is used as the default value. As the value is set, it can not be overwritten in the onelogin framework for example by setting something like

SAML2_ONELOGIN_OVERRIDES: '{"sp":{"NameIDFormat":"urn:oasis:names:tc:SAML:2.0:nameid-format:persistent"}}'

Further, the urn:oasis:names:tc:SAML:1.1:nameid-format:emailAddress is outdated by IDMs like shibboleth.

By removing this line, the default settings of the underlying framework are being used and users can adapt the value according to their needs by using the overrides.

@ssddanbrown
Copy link
Member

Thanks for the PR @jdede.

Do not use deprecated NAMEID_EMAIL_ADDRESS as default for SAML2 logout

Do you have any relatively official guidance as to how that's been deprecated? From searching I can't find any notice/spec defining that it's been deprecated at all.

I'm not keen on changing the defaults as per this PR, as I'm concerned this will cause breaking changes for existing users.
Plus there are a few other parts of the code which will need to change if we start expecting non-email based IDs, since our logic does expect it to be the email address.

Generally I don't think it'd be worth supporting variation here unless there's a wider proven need, and at the moment it seems like this is a need based on the defaults of one auth provide, which can support NAMEID_EMAIL_ADDRESS with a little configuration as far as I can tell?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants