Crivaledaz / Mattermost-LDAP

This module provides an external LDAP authentication in Mattermost for the Team Edition (free).
MIT License
359 stars 71 forks source link

Fixed failing oauth if username case changed #40

Closed parnic-sks closed 4 years ago

parnic-sks commented 4 years ago

If the user had previously logged into mattermost as "user" and then tried to connect another device as "User", a new set of entries would be created in the database which caused user IDs to not match up which caused Mattermost to throw an error about the account already being registered under a different service. This change ensures usernames are always processed in lowercase so that this can't happen.

Note that manual fixups of any existing entries in the mattermost-ldap database may need to be performed to fully fix everything. This just prevents the issue from occurring again in a very simple way. A more thorough fix would be to make the db queries check in a case-insensitive matter, though multiple entries in the users database would still exist.

This could be made into a config setting if desired, though for our use case (Microsoft AD server) the usernames themselves are always case-insensitive.

Crivaledaz commented 4 years ago

You are right, username should be forced in lowercase to avoid multi entries created for the same user, and associated Mattermost errors. By default, LDAP/AD attributes and DNs are case insensitive, so the best way is to keep it lowercase.

Furthermore, I am also surprised about the $user escaping. It is completely useless if $user is set again just after. I don't know how this error has been introduced.

I have integrated these changes in the V2, available here : https://github.com/Crivaledaz/Mattermost-LDAP/tree/v2

I am sorry for this late answer. Thank you for your work and your contribution. I merge this PR on the V1.

Regards