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

Centralize password check as method of 'User' objects #341

Merged
merged 2 commits into from
Apr 19, 2021

Conversation

DasSkelett
Copy link
Member

Motivation

Checking passwords (and setting passwords) is something that must be rock-solid.
For this it makes sense to keep as much logic of it at a single place, so you don't have to rewrite it half a dozen times with the risk of introducing a nasty bug each time.

Also bcrypt 3.1.0 introduced a new bcrypt.checkpw(<input to check>, <hashed password>) function that is way more intuitive than the old bcrypt.hashpw(<input>, <hashed password>) == <hashed password>.
If you are wondering how the old one works, bcrypt.hashpw() reads the salt from the hashed password (bcrypt hashes save the salt in front of the actual pw hash) and hashes the clear text input using this salt. Then we compare the output with the actual hash.
The new method basically still does the same in the background, but we don't need to care about it anymore, it's much more readable.

Changes

Create a new User.check_password(<input>), which takes care of encoding the input strings with UTF-8 to give bcrypt the byte array it wants, and calling bcrypt.checkpw(). It returns True for a matching password and False for incorrect ones.
All places where we had the complicated bcrypt.hashpw() construct now simply call checkpw() on a User object.

If we ever need to change the hash function for passwords, this should simplify the process.

I've also added a basic test to make sure we aren't doing something majorly wrong in this method.

I've specified the minimum version of bcrypt we now need in the requirements-backend.txt.

Please review carefully to make sure I didn't accidentally drop a vital not or something in the password checks.

@DasSkelett DasSkelett added Type: Improvement Area: Backend Related to the Python code that runs inside gunicorn Priority: Low Status: Ready labels Apr 19, 2021
Copy link
Contributor

@HebaruSan HebaruSan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed that logging in still works. 👍

@DasSkelett DasSkelett merged commit e9cdced into KSP-SpaceDock:alpha Apr 19, 2021
@DasSkelett DasSkelett deleted the enh/password-check branch April 19, 2021 15:26
This was referenced May 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Backend Related to the Python code that runs inside gunicorn Priority: Low Status: Ready Type: Improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants