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

Make username case-insensitive #677

Merged
merged 1 commit into from
Aug 22, 2017
Merged

Make username case-insensitive #677

merged 1 commit into from
Aug 22, 2017

Conversation

MaxLeiter
Copy link
Contributor

No description provided.

@coveralls
Copy link

coveralls commented Jul 21, 2017

Coverage Status

Coverage remained the same at 73.554% when pulling 645c257 on MaxLeiter:case-sensitive into 3d533b3 on devsnd:devel.

@devsnd
Copy link
Owner

devsnd commented Jul 21, 2017

Yes, we should make them case insensitive, but before I can accept the PR we also have to make sure that changePassword, addUser and deleteUser are all also case insensitive. Otherwise bad things can happen.

If we're really careful we would check if there any users that would not be able to log in any more in that case (e.g. devsnd and DEVSND) on startup, but that's probably overkill.

@MaxLeiter
Copy link
Contributor Author

MaxLeiter commented Jul 21, 2017

I'll go ahead and put the lowers elsewhere, but this shouldn't stop anyone from logging in -- it just lowercases the username entered and the username it's fetching from the database

No point in making addUser case-insensitive, as it doesn't lookup usernames.

@MaxLeiter
Copy link
Contributor Author

Updated, added to changePassword

@devsnd
Copy link
Owner

devsnd commented Jul 21, 2017

I'll go ahead and put the lowers elsewhere, but this shouldn't stop anyone from logging in -- it just lowercases the username entered and the username it's fetching from the database

Well, if anyone would create two users MAX and max then both could not log in anymore, because of line 108: assert len(rows) <= 1. So it's important to make sure that it's impossible to create this scenario by making all accesses lower-cased.

@MaxLeiter
Copy link
Contributor Author

Good point -- done

@coveralls
Copy link

coveralls commented Jul 21, 2017

Coverage Status

Coverage remained the same at 73.554% when pulling bcf3395 on MaxLeiter:case-sensitive into 3f969ef on devsnd:devel.

@coveralls
Copy link

coveralls commented Jul 21, 2017

Coverage Status

Coverage decreased (-0.004%) to 73.551% when pulling 1a2de1b on MaxLeiter:case-sensitive into 3f969ef on devsnd:devel.

@devsnd devsnd merged commit fca95ae into devsnd:devel Aug 22, 2017
@devsnd
Copy link
Owner

devsnd commented Aug 22, 2017

Thanks, this time you get a dolphin and a fish, so you can feed it.

🐬 🐟

@MaxLeiter MaxLeiter deleted the case-sensitive branch August 22, 2017 13:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants