BookStackApp / BookStack

A platform to create documentation/wiki content built with PHP & Laravel
https://www.bookstackapp.com/
MIT License
15.43k stars 1.94k forks source link

Add firstName and lastName variables for LDAP #5288

Closed MatthieuLeboeuf closed 3 weeks ago

MatthieuLeboeuf commented 4 weeks ago

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 commented 3 weeks ago

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 commented 3 weeks ago

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 commented 3 weeks ago

@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 commented 3 weeks ago

Okay :+1: