Skip to content

feat: implement api for "forgot password?" flow #14915

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 25 commits into from
Oct 4, 2024
Merged

feat: implement api for "forgot password?" flow #14915

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
b44adff
feat: add api for forgotten password flow
DanielleMaywood Sep 30, 2024
60fc32c
fix: appease linter
DanielleMaywood Oct 1, 2024
32a8df4
fix: give 20 minutes for one time passcode
DanielleMaywood Oct 1, 2024
e486923
chore: changes from feedback
DanielleMaywood Oct 2, 2024
da87e9b
fix: make ChangePasswordWithOneTimePassword expect StatusOK
DanielleMaywood Oct 2, 2024
39fa0f1
docs: run 'make gen'
DanielleMaywood Oct 2, 2024
b89dfd7
test: add more tests
DanielleMaywood Oct 2, 2024
714e316
test: ensure login fails before changing password
DanielleMaywood Oct 2, 2024
c4ed205
refactor: test and dbmem.UpdateUserHashedOneTimePasscode
DanielleMaywood Oct 2, 2024
fa9d426
fix: do not enqueue notification if user.ID is uuid.Nil
DanielleMaywood Oct 2, 2024
883a231
refactor: wrap GetUserByEmailOrUsername in transaction
DanielleMaywood Oct 2, 2024
bff384b
refactor: error handling
DanielleMaywood Oct 2, 2024
26cc758
fix: check one-time passcode expiry
DanielleMaywood Oct 2, 2024
25581c2
test: one-time passcode becomes invalid after use
DanielleMaywood Oct 2, 2024
764b0cc
Merge branch 'main' into dm-request-and-submit-passcode
DanielleMaywood Oct 2, 2024
6078409
fix: make changes from feedback
DanielleMaywood Oct 3, 2024
3a0946c
refactor: change endpoints
DanielleMaywood Oct 3, 2024
1a7ad7a
fix: update assertSecurityDefined links
DanielleMaywood Oct 3, 2024
6bbcff2
refactor: make validity period an option and test it
DanielleMaywood Oct 3, 2024
dfc32b1
test: refactor tests to reduce duplication
DanielleMaywood Oct 3, 2024
9f80082
test: ensure cannot login after failed password change
DanielleMaywood Oct 4, 2024
fe9b3f8
fix: imports
DanielleMaywood Oct 4, 2024
b42b73c
test: ensure can login with old credentials
DanielleMaywood Oct 4, 2024
034a7d4
chore: apply changes based on feedback
DanielleMaywood Oct 4, 2024
e14d43b
chore: add missing comment
DanielleMaywood Oct 4, 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
Prev Previous commit
Next Next commit
chore: changes from feedback
  • Loading branch information
@DanielleMaywood
DanielleMaywood committed Oct 2, 2024
commit e486923c257ef80cdc4f2023a0e269827de33ad1
4 changes: 2 additions & 2 deletions coderd/database/migrations/000260_notifications_forgot_password.up.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
INSERT INTO notification_templates (id, name, title_template, body_template, "group", actions)
VALUES ('62f86a30-2330-4b61-a26d-311ff3b608cf', 'One Time Passcode', E'Your one time passcode is enclosed.',
E'Hi {{.UserName}},\n\nA request to reset the password for your Coder account has been made. Your one time passcode is:\n\n**{{.Labels.one_time_passcode}}**\n\nIf you did not request to reset your password, you can ignore this message.',
VALUES ('62f86a30-2330-4b61-a26d-311ff3b608cf', 'One-Time Passcode', E'Your one-time passcode is enclosed.',
E'Hi {{.UserName}},\n\nA request to reset the password for your Coder account has been made. Your one-time passcode is:\n\n**{{.Labels.one_time_passcode}}**\n\nIf you did not request to reset your password, you can ignore this message.',
'User Events', '[]'::jsonb);
2 changes: 1 addition & 1 deletion ...fications/testdata/rendered-templates/TemplateUserRequestedOneTimePasscode-body.md.golden
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
Hi Bobby,

A request to reset the password for your Coder account has been made. Your one time passcode is:
A request to reset the password for your Coder account has been made. Your one-time passcode is:

**fad9020b-6562-4cdb-87f1-0486f1bea415**

Expand Down
2 changes: 1 addition & 1 deletion ...ications/testdata/rendered-templates/TemplateUserRequestedOneTimePasscode-title.md.golden
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
Your one time passcode is enclosed.
Your one-time passcode is enclosed.
35 changes: 18 additions & 17 deletions coderd/userauth.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ const (
userAuthLoggerName = "userauth"
OAuthConvertCookieValue = "coder_oauth_convert_jwt"
mergeStateStringPrefix = "convert-"
oneTimePasscodeDuration = 20 * time.Minute
)

type OAuthConvertStateClaims struct {
Expand Down Expand Up @@ -202,13 +203,13 @@ func (api *API) postConvertLoginType(rw http.ResponseWriter, r *http.Request) {
})
}

// Requests a one-time-passcode for a user.
// Requests a one-time passcode for a user.
//
// @Summary Request one-time-passcode.
// @Summary Request one-time passcode.
// @ID request-one-time-passcode
// @Accept json
// @Tags Authorization
// @Param request body codersdk.RequestOneTimePasscodeRequest true "Request one time passcode request"
// @Param request body codersdk.RequestOneTimePasscodeRequest true "Request one-time passcode request"
// @Success 200
// @Router /users/request-one-time-passcode [post]
func (api *API) postRequestOneTimePasscode(rw http.ResponseWriter, r *http.Request) {
Expand Down Expand Up @@ -243,7 +244,7 @@ func (api *API) postRequestOneTimePasscode(rw http.ResponseWriter, r *http.Reque
rw.WriteHeader(http.StatusOK)
}()

//nolint:gocritic // In order to request a one-time-passcode, we need to get the user first!
//nolint:gocritic // In order to request a one-time passcode, we need to get the user first - and can only do that in the system auth context.
user, err := api.Database.GetUserByEmailOrUsername(dbauthz.AsSystemRestricted(ctx), database.GetUserByEmailOrUsernameParams{
Email: req.Email,
})
Expand All @@ -254,22 +255,22 @@ func (api *API) postRequestOneTimePasscode(rw http.ResponseWriter, r *http.Reque
aReq.Old = user

passcode := uuid.New()
Copy link
Member

Choose a reason for hiding this comment

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

Side-note: I always feel weirded out using plain UUIDs for auth, but we do it elsewhere and UUIDv4 is supposedly OK. There are varying views on this though. I bring this up because I wonder if we should consider using something with more entropy in the future. 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

(According to ChatGPT o1-preview) there are 122 bits of entropy in UUIDv4, isn't that enough for you? 😆

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also worth noting, the UUIDv4 implementation we use does use a cryptographically secure RNG.

coder/go.mod

Line 122 in 764b0cc

github.com/google/uuid v1.6.0

https://github.com/google/uuid/blob/0e97ed3b537927cb4afea366bc4cc36f6eb37e75/uuid.go#L45

https://github.com/google/uuid/blob/0e97ed3b537927cb4afea366bc4cc36f6eb37e75/uuid.go#L9

https://pkg.go.dev/crypto/rand#pkg-variables

Copy link
Member

Choose a reason for hiding this comment

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

@dannykopping exactly, it's a few bits short. I believe 128 or more bits of entropy is the recommendation (required by RFC6749, for example). 😄

@DanielleMaywood yes, definitely worth noting 👍🏻

Copy link
Contributor

Choose a reason for hiding this comment

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

Ha! TIL 🥲

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In that case, should we go for an alternative to using a UUID here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mtojek and I discussed this and have decided that the current solution should be sufficient. The tokens are generated with a cryptographically secure RNG, the rate limiter is pretty strict (60req/s) and tokens do not last very long so the few bits short in entropy should be sufficient. We can always review this in the future if we decide a different approach should be taken.

passcodeExpiresAt := dbtime.Now().Add(20 * time.Minute)
passcodeExpiresAt := dbtime.Now().Add(oneTimePasscodeDuration)

hashedPasscode, err := userpassword.Hash(passcode.String())
if err != nil {
logger.Error(ctx, "unable to hash passcode", slog.Error(err))
return
}

//nolint:gocritic // We need to be able to save the one-time-passcode.
//nolint:gocritic // We need the system auth context to be able to save the one-time passcode.
err = api.Database.UpdateUserHashedOneTimePasscode(dbauthz.AsSystemRestricted(ctx), database.UpdateUserHashedOneTimePasscodeParams{
ID: user.ID,
HashedOneTimePasscode: []byte(hashedPasscode),
OneTimePasscodeExpiresAt: sql.NullTime{Time: passcodeExpiresAt, Valid: true},
})
if err != nil {
logger.Error(ctx, "unable to set user hashed one time passcode", slog.Error(err))
logger.Error(ctx, "unable to set user hashed one-time passcode", slog.Error(err))
return
}

Expand All @@ -278,16 +279,16 @@ func (api *API) postRequestOneTimePasscode(rw http.ResponseWriter, r *http.Reque
newUser.OneTimePasscodeExpiresAt = sql.NullTime{Time: passcodeExpiresAt, Valid: true}
aReq.New = newUser

// Send the one-time-passcode to the user.
// Send the one-time passcode to the user.
err = api.notifyUserRequestedOneTimePasscode(ctx, user, passcode.String())
if err != nil {
logger.Error(ctx, "unable to notify user about one time passcode request", slog.Error(err))
logger.Error(ctx, "unable to notify user about one-time passcode request", slog.Error(err))
}
}

func (api *API) notifyUserRequestedOneTimePasscode(ctx context.Context, user database.User, passcode string) error {
_, err := api.NotificationsEnqueuer.Enqueue(
//nolint:gocritic // We need to be able to send the user their one time passcode.
//nolint:gocritic // We need the system auth context to be able to send the user their one-time passcode.
dbauthz.AsSystemRestricted(ctx),
user.ID,
notifications.TemplateUserRequestedOneTimePasscode,
Expand All @@ -302,9 +303,9 @@ func (api *API) notifyUserRequestedOneTimePasscode(ctx context.Context, user dat
return nil
}

// Change a users password with a one-time-passcode.
// Change a users password with a one-time passcode.
//
// @Summary Change password with a one-time-passcode.
// @Summary Change password with a one-time passcode.
// @ID change-password-with-a-one-time-passcode
// @Accept json
// @Tags Authorization
Expand Down Expand Up @@ -338,7 +339,7 @@ func (api *API) postChangePasswordWithOneTimePasscode(rw http.ResponseWriter, r
return
}

//nolint:gocritic // In order to change a user's password, we need to get the user first!
//nolint:gocritic // In order to change a user's password, we need to get the user first - and can only do that in the system auth context.
user, err := api.Database.GetUserByEmailOrUsername(dbauthz.AsSystemRestricted(ctx), database.GetUserByEmailOrUsernameParams{
Email: req.Email,
})
Expand All @@ -361,7 +362,7 @@ func (api *API) postChangePasswordWithOneTimePasscode(rw http.ResponseWriter, r
}

if !equal {
httpapi.Write(ctx, rw, http.StatusUnauthorized, codersdk.Response{
httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{
Message: "Incorrect email or one-time-passcode.",
})
return
Expand Down Expand Up @@ -398,7 +399,7 @@ func (api *API) postChangePasswordWithOneTimePasscode(rw http.ResponseWriter, r
}

err = api.Database.InTx(func(tx database.Store) error {
//nolint:gocritic // We need to update the user's password.
//nolint:gocritic // We need the system auth context to be able to update the user's password.
err = tx.UpdateUserHashedPassword(dbauthz.AsSystemRestricted(ctx), database.UpdateUserHashedPasswordParams{
Copy link
Member

Choose a reason for hiding this comment

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

I wish we could do this without system context somehow, seems like a potential security risk.

ID: user.ID,
HashedPassword: []byte(newHashedPassword),
Expand All @@ -407,7 +408,7 @@ func (api *API) postChangePasswordWithOneTimePasscode(rw http.ResponseWriter, r
return xerrors.Errorf("update user hashed password: %w", err)
}

//nolint:gocritic // We need to delete all API keys for the user.
//nolint:gocritic // We need the system auth context to be able to delete all API keys for the user.
err = tx.DeleteAPIKeysByUserID(dbauthz.AsSystemRestricted(ctx), user.ID)
if err != nil {
return xerrors.Errorf("delete api keys for user: %w", err)
Expand All @@ -430,7 +431,7 @@ func (api *API) postChangePasswordWithOneTimePasscode(rw http.ResponseWriter, r
newUser.HashedOneTimePasscode = nil
aReq.New = newUser

rw.WriteHeader(http.StatusNoContent)
rw.WriteHeader(http.StatusOK)
}

// Authenticates the user with an email and password.
Expand Down
Loading