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.

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

Add firstName and lastName variables for LDAP #5288

Conversation

MatthieuLeboeuf
Copy link
Contributor

Hello !

To resolve #1684

I added two fields, LDAP_FIRST_NAME_ATTRIBUTE and LDAP_LAST_NAME_ATTRIBUTE, which, if both are filled, allow replacing LDAP_DISPLAY_NAME_ATTRIBUTE.

I did not set default values to avoid breaking existing installations.

@ssddanbrown
Copy link
Member

Hi @MatthieuLeboeuf,
Thanks for offering this.

I'm not too keen on expanding our support of LDAP options, based on such little demand, but we do support something similar for SAML2/OIDC so it maybe makes sense to align functionality.

Rather than supporting three variables just for the name, could this instead follow the convention of our SAML2/OIDC implementation and allow the attributes to be defined under the single option, split via pipe, for example:

LDAP_DISPLAY_NAME_ATTRIBUTE=firstname|lastname

With found attributed values joined using a space. That way it's aligned with our other auth options, and allows any number of attribute values to be joined.

Also, if possible, would be good to have a test to cover any added/changed functionality here.

@MatthieuLeboeuf
Copy link
Contributor Author

Hello :)

For SAML2, SAML2_DISPLAY_NAME_ATTRIBUTES is used, and for OIDC, OIDC_DISPLAY_NAME_CLAIMS is used.

Should we rename LDAP_DISPLAY_NAME_ATTRIBUTE to LDAP_DISPLAY_NAME_ATTRIBUTES ?
However, this would introduce a breaking change.

@ssddanbrown
Copy link
Member

@MatthieuLeboeuf No, just keep it setting name as-is, we'll just live with the misaligned pluralisation.
I checked via a quick search that | should not be part of an existing value LDAP attribute name, so we should be able to add this to the existing option without breaking any instances.

@MatthieuLeboeuf
Copy link
Contributor Author

Okay 👍

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.

LDAP_DISPLAY_NAME_ATTRIBUTE merge multiple values
2 participants