kscalelabs / store

K-Scale Labs store
https://kscale.store
MIT License
25 stars 24 forks source link

[ IMPR ] hashed passwords #237

Closed Winston-Hsiao closed 1 month ago

Winston-Hsiao commented 1 month ago

Notable changes/additions:

  1. created_at and updated_at fields to user (set on User creation)
  2. email_verified_at field for when email verification is completed (has to be set in email verification api route, should be set automatically by default for standard auth methods)
  3. OAuth providers (Google and Github) normalized with standard User model. Signing up with Google and Github now creates a new User, leaving hashed_password empty and instead setting the linked provider on the User object itself
  4. Password strength check/validation

New npm packages:

  1. zxcvbn password strength

New pip packages:

  1. bcrypt: bcrypt pip page recommends argon2id or scrypt as alternatives to bcrypt itself
  2. email-validator required for EmailStr pydantic type (validate user input emails)
Winston-Hsiao commented 1 month ago

@codekansas My local environment isn't properly set up yet to test this unfortunately, will need to take some time to set up local dynamo db instance, etc. But this should be the general extent to what we need added.

  1. store hashed_password on user model
  2. hash password input by user and store on account creation
  3. on sign in use verify_password to check if input password is correct.
Winston-Hsiao commented 1 month ago

looks good - only thing is, not sure what we will do for the password if the user logs in with google / github oauth instead. open to suggestions

Approaches I've seen in the past is having a nested OAuth account type related to the User model that can be defined or undefined (in the case that someone signs up using google/github oauth, we still make a User object like with standard sign up. We would also have to make hashed_password optional for the User model in that case. So all Users are lumped in that one type/collection which is good, and we can distinguish login/auth method based on if they have a linked account or a hashed_password set.

We'll have to adjust the various models we have now to get User data more normalized and then support the account type variants, rather than having them all be separate.

chennisden commented 1 month ago

Relatedly, we should run some checks, both client-side and server-side, that the password the user inputs is actually secure (e.g. we ought to reject 123456).

I am no fan of "must have an uppercase character/number/etc" requirements and think zxcvbn is a much better solution.

Winston-Hsiao commented 1 month ago

Relatedly, we should run some checks, both client-side and server-side, that the password the user inputs is actually secure (e.g. we ought to reject 123456).

I am no fan of "must have an uppercase character/number/etc" requirements and think zxcvbn is a much better solution.

agreed on both client and server side validation for password strength.

also agreed, I've implemented "must have an uppercase character/number/etc" myself in my own project and worry sometimes it's too much/annoying for new users, zxcvbn looks like a great solution, great suggestion.