feat: add one time passcode columns to users table #14797
Conversation
| @@ -0,0 +1,3 @@ | |||
| ALTER TABLE users DROP COLUMN hashed_one_time_passcode; | |||
There was a problem hiding this comment.
Did you consider using a token instead of a passcode? Either works I guess but I token is more common.
There was a problem hiding this comment.
I hadn't considered token. I'm happy with any name that makes sense so will change it if we feel token makes more sense.
There was a problem hiding this comment.
@johnstcn @dannykopping thoughts on token vs passcode? I'm neutral either way.
There was a problem hiding this comment.
Let's stick with what you have for now.
enterprise/audit/table.go
Outdated
| "one_time_passcode_expires_at": ActionIgnore, | ||
| "must_reset_password": ActionIgnore, |
There was a problem hiding this comment.
I think it could be useful to track these?
There was a problem hiding this comment.
Sure thing, will make that change!
There was a problem hiding this comment.
It would allow folks to then see in the audit logs when someone requested a password change. I think that would be very desirable from a security perspective.
| ALTER TABLE users ADD COLUMN must_reset_password bool NOT NULL DEFAULT false; | ||
| COMMENT ON COLUMN users.must_reset_password IS 'Determines if the user should be forced to change their password.'; |
There was a problem hiding this comment.
Do we need this column? Could we instead say that a user must reset their password if hashed_one_time_passcode IS NOT NULL?
There was a problem hiding this comment.
By doing that you could force someone to change their password by putting someone's email in the "Forgot password?" form (when it exists). That is why it is a separate field.
| @@ -0,0 +1,8 @@ | |||
| ALTER TABLE users ADD COLUMN hashed_one_time_passcode bytea; | |||
There was a problem hiding this comment.
Nit: I feel the hashed_ prefix is unnecessary.
Also I think we could abbreviate these to otp for brevity, and you have helpfully added comments explaining what the fields are.
There was a problem hiding this comment.
We already have the prefix for hashed_password so it does maintain consistency
There was a problem hiding this comment.
The decision to prefix with hashed_ was that the password column also has that prefix. I'm fine with removing it though.
There was a problem hiding this comment.
Consistency is good, I forgot about hashed_password, but I do still like otp as an abbreviation though (but don't feel strongly about it).
There was a problem hiding this comment.
If we use otp then we'll need to add some aguisíní to sqlc.yaml to generate HashedOTP and OTPExpiresAt instead of HashedOtp and OtpExpiresAt.
relates to #14232
This sets the foundation for adding a one-time-passcode flow for a user to reset their password.
This change adds 3 new columns to the
userstable:hashed_one_time_passcodeone_time_passcode_expires_atmust_reset_passwordThe
hashed_one_time_passcodecolumn is at it says, it is a hash of the one-time-passcode that will be sent to the user (over email).The
one_time_passcode_expires_atcolumn is the timestamp of when the one-time-passcode sent to the user will no longer be valid.The
must_reset_passwordcolumn is so we can force a user to reset their password.