Allows to customize SAML attributes #1344
Conversation
There was a problem hiding this comment.
Lgtm.
I don't have the rights to merge. Also I would normally need to setup an environment with SAML.
Would it help you if I do that @dsagal (or anyone from GristLabs)? Or you have everything in place and would like to do that yourself?
I can probably test it with my instance, but I'll just need some pointer on how to build a local |
mclegrand
force-pushed
the
main
branch
2 times, most recently
from
dc45f63 to
a8dd567
Compare
Adds three environment variables: - GRIST_SAML_ATTR_FIRSTNAME - GRIST_SAML_ATTR_LASTNAME - GRIST_SAML_ATTR_EMAIL so that the attributes coming from the IdP can be customized. This allows from a variety of IdP to be used directly, including ones from educational institution with urn:oid (direct or aliased)
| const fnameAttribute = process.env.GRIST_SAML_ATTR_FIRSTNAME || ''; | ||
| const lnameAttribute = process.env.GRIST_SAML_ATTR_LASTNAME || ''; | ||
| const emailAttribute = process.env.GRIST_SAML_ATTR_EMAIL || ''; | ||
| const fname = samlUser.attributes[fnameAttribute] || |
There was a problem hiding this comment.
Let's definitely not lookup the key if the key is falsy. Otherwise, it actually could have a matching key in attributes and the value could be non-falsy. It would be weird to have one, but the kind of weirdness that can lead to exploits.
| const lnameAttribute = process.env.GRIST_SAML_ATTR_LASTNAME || ''; | ||
| const emailAttribute = process.env.GRIST_SAML_ATTR_EMAIL || ''; | ||
| const fname = samlUser.attributes[fnameAttribute] || | ||
| samlUser.given_name || samlUser.attributes.FirstName || ''; |
There was a problem hiding this comment.
Related question: if you set a variable like GRIST_SAML_ATTR_FIRSTNAME, would you expect the just that attribute would get used? If that attribute is empty, would you want it to fall back to looking at other attributes, or just use the empty value?
There was a problem hiding this comment.
I guess in theory "do not fallback" would make sense if the value is returned and returned empty, but I can't think of a case where the fallback values would also exist and differ (I have never seen an empty string saml response value)
Context
As an admin trying to provide grist to people in my department, I do not control the institutional IdP response and would still like to directly use SAML login. Currently, the NameID mechanism for uid is fairly standard, but there is a variety of possible names for firstname, sn, and mail in various IdP, which are not used in saml2-js. saml2-js is currently in "maintenance mode" and does not seem to use env variables to determine custom attribute names (but exposes them all in user.attributes).
Proposed solution
This would allow me to just add the variables with actual values from my IdP into the container env variables.
Has this been tested?