-
-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
Case-insensitive login #8610
Case-insensitive login #8610
Conversation
Looks like Piwik always used the token auth and not the password: https://github.com/piwik/piwik/blob/0da8fa6a7f4f77412eb0f8cda966914416fad977/plugins/Login/Auth.php Not sure if it confers any security benefits, going to investigate further... |
Found initial use of token_auth for logging in: 62ff9b5 Before this, it used the My guess is that it was put in in preparation for API authentication. Since that uses only token auth, it may have been easier to have one code path instead of two. I cannot think of any security benefits of using only the token auth. Someone could potentially brute force the password just as easily as when authenticating against the hashed password. |
I can see a couple issues w/ merging this as is:
|
One more thing: can you make the case insensitivity explicit in |
Forgot to mention it but for the record another motivation for this solution (i.e. login using the password, not always going through the auth token) is to help a tiny bit getting rid of
Done. Comment + test should do it.
Yep a test for that will be good. Will do it.
I hope that currently it's not possible to register with the same login and different case. Else this will be a mess. We'll see tomorrow ;) |
@diosmosis It should be good with the last 2 commits. Check also b4c52c7, it's quite ugly but if you think that it's possible that some users could exist somewhere with same login/different case then… |
As the initial request was re email address (which in another plugin may be used as login), does it maybe make sense to update methods like Also wondering if there was a possible attack to create a new account with an existing username but different case, eg first letter uppercase, and then when logging in, I'm getting a different users identity or cannot log in anymore? Eg there is a user with username Guess all I'm saying is can we prevent the creation of new users with the same username but different case? I think it's already the case and it is not even possible to have same username twice with different case so please ignore it all if someone can confirm this. |
No behavior change.
Refactored the way password login is done: instead of computing the token for the given login/password and then checking the token, login/password is now directly checked against the database. Since DB string comparison is case insensitive, it allows logging in with case-insensitive login (when logging in through the login/password form). Added more tests to cover this new behavior + existing "log in with password" behavior.
…d to use a query that hits the primary key index of the user table.
Fixes #8548, only allow case-insensitive login (while maintaining BC for 2.15 LTS)
Fixes #8548
Refactored the way password login is done: instead of computing the token for the given login/password and then checking the token, login/password is now directly checked against the database. Since DB string comparison is case insensitive, it allows logging in with case-insensitive login (when logging in through the login/password form).
Added more tests to cover this new behavior + existing "log in with password" behavior.
Please review extensively. Also I have no idea why the login was working the way it was working before, maybe I just missed a use case…
(tip for reviewing: the first commit is a small refactoring, review them separately it's easier)