Fixed failing oauth if username case changed #40
Conversation
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.
| @@ -33,7 +33,7 @@ | |||
| // Remove every html tag and useless space on username (to prevent XSS) | |||
| $user=strip_tags(trim($_POST['user'])); | |||
|
|
|||
| $user=$_POST['user']; | |||
| $user=strtolower($_POST['user']); | |||
There was a problem hiding this comment.
Note: I'm unclear why $user is being set to the result of strip_tags(trim()) and immediately re-set to the POST value, but I didn't want to try and make both changes at once since that's not quite relevant to this particular change.
|
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 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 |
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.