Skip to content

feat: add one time passcode columns to users table #14797

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.

Sign up for GitHub

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

Merged
merged 5 commits into from
Sep 25, 2024
Merged

feat: add one time passcode columns to users table #14797

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
f15b165
feat: add one time passcode columns to users table
DanielleMaywood Sep 25, 2024
e121fc6
fix: grammar
DanielleMaywood Sep 25, 2024
67fb551
fix: track changes
DanielleMaywood Sep 25, 2024
6dd119f
fix: run 'make gen'
DanielleMaywood Sep 25, 2024
9f3a036
fix: add constraint
DanielleMaywood Sep 25, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 10 additions & 1 deletion coderd/database/dump.sql
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 3 additions & 0 deletions coderd/database/migrations/000258_add_otp_reset_to_user.down.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
ALTER TABLE users DROP COLUMN hashed_one_time_passcode;
Copy link
Member

Choose a reason for hiding this comment

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

Did you consider using a token instead of a passcode? Either works I guess but I token is more common.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@johnstcn @dannykopping thoughts on token vs passcode? I'm neutral either way.

Copy link
Member

Choose a reason for hiding this comment

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

Let's stick with what you have for now.

ALTER TABLE users DROP COLUMN one_time_passcode_expires_at;
ALTER TABLE users DROP COLUMN must_reset_password;
8 changes: 8 additions & 0 deletions coderd/database/migrations/000258_add_otp_reset_to_user.up.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
ALTER TABLE users ADD COLUMN hashed_one_time_passcode bytea;
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

We already have the prefix for hashed_password so it does maintain consistency

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The decision to prefix with hashed_ was that the password column also has that prefix. I'm fine with removing it though.

Copy link
Contributor

Choose a reason for hiding this comment

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

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).

Copy link
Member

Choose a reason for hiding this comment

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

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.

COMMENT ON COLUMN users.hashed_one_time_passcode IS 'A hash of the one-time-passcode given to the user.';

ALTER TABLE users ADD COLUMN one_time_passcode_expires_at timestamp with time zone;
COMMENT ON COLUMN users.one_time_passcode_expires_at IS 'The time when the one-time-passcode expires.';

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.';
Comment on lines +12 to +13
Copy link
Member

Choose a reason for hiding this comment

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

Do we need this column? Could we instead say that a user must reset their password if hashed_one_time_passcode IS NOT NULL?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

3 changes: 3 additions & 0 deletions coderd/database/modelqueries.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -364,6 +364,9 @@ func (q *sqlQuerier) GetAuthorizedUsers(ctx context.Context, arg GetUsersParams,
&i.ThemePreference,
&i.Name,
&i.GithubComUserID,
&i.HashedOneTimePasscode,
&i.OneTimePasscodeExpiresAt,
&i.MustResetPassword,
&i.Count,
); err != nil {
return nil, err
Expand Down
6 changes: 6 additions & 0 deletions coderd/database/models.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

97 changes: 68 additions & 29 deletions coderd/database/queries.sql.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading
Loading