Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allows to customize SAML attributes #1344

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Allows to customize SAML attributes #1344

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 18 additions & 3 deletions app/server/lib/SamlConfig.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,15 @@
* Comma-separated list of paths for certificates from identity provider, PEM format.
* env GRIST_SAML_IDP_UNENCRYPTED
* If set and non-empty, allow unencrypted assertions, relying on https for privacy.
* env GRIST_SAML_ATTR_FIRSTNAME
* If set and non-empty, determines the user's firstname attribute from the IdP response.
* e.g. "urn:oid:2.5.4.4"
* env GRIST_SAML_ATTR_LASTNAME
* If set and non-empty, determines the user's lastname attribute from the IdP response.
* e.g. "urn:oid:1.3.6.1.4.1.5923.1.1.1.6"
* env GRIST_SAML_ATTR_EMAIL
* If set and non-empty, determines the user's email attribute from the IdP response.
* e.g. "urn:oid:0.9.2342.19200300.100.1.3"
*
* This version of SamlConfig has been tested with Auth0 SAML IdP following the instructions
* at:
Expand Down Expand Up @@ -181,9 +190,15 @@ export class SamlConfig {
// An example IdP response is at https://github.com/Clever/saml2#assert_response. Saml2-js
// maps some standard attributes as user.given_name, user.surname, which we use if
// available. Otherwise we use user.attributes which has the form {Name: [Value]}.
const fname = samlUser.given_name || samlUser.attributes.FirstName || '';
const lname = samlUser.surname || samlUser.attributes.LastName || '';
const email = samlUser.email || samlUser.name_id;
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] ||
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

samlUser.given_name || samlUser.attributes.FirstName || '';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Author

@mclegrand mclegrand Dec 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

const lname = samlUser.attributes[lnameAttribute] ||
samlUser.surname || samlUser.attributes.LastName || '';
const email = samlUser.attributes[emailAttribute] ||
samlUser.email || samlUser.name_id;
const profile = {
email,
name: `${fname} ${lname}`.trim(),
Expand Down