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
fix: make changes from feedback
  • Loading branch information
@DanielleMaywood
DanielleMaywood committed Oct 3, 2024
commit 6078409140f294fd49e61b5cb69d16e8ff3f69da
8 changes: 4 additions & 4 deletions coderd/apidoc/docs.go
View file
Open in desktop

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

8 changes: 4 additions & 4 deletions coderd/apidoc/swagger.json
View file
Open in desktop

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

2 changes: 1 addition & 1 deletion coderd/database/migrations/000261_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.',
VALUES ('62f86a30-2330-4b61-a26d-311ff3b608cf', 'One-Time Passcode', E'Your One-Time Passcode for Coder.',
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 ...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 for Coder.
38 changes: 20 additions & 18 deletions coderd/userauth.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,10 +43,10 @@ import (
)

const (
userAuthLoggerName = "userauth"
OAuthConvertCookieValue = "coder_oauth_convert_jwt"
mergeStateStringPrefix = "convert-"
oneTimePasscodeDuration = 20 * time.Minute
userAuthLoggerName = "userauth"
OAuthConvertCookieValue = "coder_oauth_convert_jwt"
mergeStateStringPrefix = "convert-"
oneTimePasscodeValidityPeriod = 20 * time.Minute
)

type OAuthConvertStateClaims struct {
Expand Down Expand Up @@ -210,7 +210,7 @@ func (api *API) postConvertLoginType(rw http.ResponseWriter, r *http.Request) {
// @Accept json
// @Tags Authorization
// @Param request body codersdk.RequestOneTimePasscodeRequest true "Request one-time passcode request"
// @Success 200
// @Success 204
// @Router /users/request-one-time-passcode [post]
func (api *API) postRequestOneTimePasscode(rw http.ResponseWriter, r *http.Request) {
var (
Expand Down Expand Up @@ -241,7 +241,7 @@ func (api *API) postRequestOneTimePasscode(rw http.ResponseWriter, r *http.Reque
defer func() {
// We always send the same response. If we give a more detailed response
// it would open us up to an enumeration attack.
rw.WriteHeader(http.StatusOK)
rw.WriteHeader(http.StatusNoContent)
}()

//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.
Expand All @@ -255,7 +255,7 @@ 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(oneTimePasscodeDuration)
passcodeExpiresAt := dbtime.Now().Add(oneTimePasscodeValidityPeriod)

hashedPasscode, err := userpassword.Hash(passcode.String())
if err != nil {
Expand All @@ -279,13 +279,15 @@ func (api *API) postRequestOneTimePasscode(rw http.ResponseWriter, r *http.Reque
auditUser.OneTimePasscodeExpiresAt = sql.NullTime{Time: passcodeExpiresAt, Valid: true}
aReq.New = auditUser

if user.ID != uuid.Nil {
// 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))
go func() {
if user.ID != uuid.Nil {
// Send the one-time passcode to the user.
err = api.notifyUserRequestedOneTimePasscode(context.Background(), user, passcode.String())
if err != nil {
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 {
Expand All @@ -312,7 +314,7 @@ func (api *API) notifyUserRequestedOneTimePasscode(ctx context.Context, user dat
// @Accept json
// @Tags Authorization
// @Param request body codersdk.ChangePasswordWithOneTimePasscodeRequest true "Change password request"
// @Success 200
// @Success 204
// @Router /users/change-password-with-one-time-passcode [post]
func (api *API) postChangePasswordWithOneTimePasscode(rw http.ResponseWriter, r *http.Request) {
var (
Expand Down Expand Up @@ -354,14 +356,14 @@ func (api *API) postChangePasswordWithOneTimePasscode(rw http.ResponseWriter, r

equal, err := userpassword.Compare(string(user.HashedOneTimePasscode), req.OneTimePasscode)
Copy link
Member

Choose a reason for hiding this comment

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

What happens here when you give a valid user email, but the token is null? And where do we verify the expiry of the token?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh I've completely forgotten to check the expiry 🤦‍♀️ Thank you for spotting that. Will definitely get that added.

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've now added a test to verify that tokens do expire.

if err != nil {
logger.Error(ctx, "unable to compare one time passcode", slog.Error(err))
return xerrors.Errorf("compare one time passcode: %w", err)
logger.Error(ctx, "unable to compare one-time passcode", slog.Error(err))
return xerrors.Errorf("compare one-time passcode: %w", err)
}

now := dbtime.Now()
if !equal || now.After(user.OneTimePasscodeExpiresAt.Time) {
httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{
Message: "Incorrect email or one-time-passcode.",
Message: "Incorrect email or one-time passcode.",
})
return nil
}
Expand Down Expand Up @@ -415,7 +417,7 @@ func (api *API) postChangePasswordWithOneTimePasscode(rw http.ResponseWriter, r
auditUser.HashedOneTimePasscode = nil
aReq.New = auditUser

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

return nil
}, nil)
Expand Down
26 changes: 16 additions & 10 deletions coderd/userauth_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1666,10 +1666,10 @@ func TestUserForgotPassword(t *testing.T) {
}

t.Run("CanChangePassword", func(t *testing.T) {
const newPassword = "SomeNewSecurePassword!"

t.Parallel()

const newPassword = "SomeNewSecurePassword!"

notifyEnq := &testutil.FakeNotificationsEnqueuer{}

client := coderdtest.New(t, &coderdtest.Options{
Expand All @@ -1688,7 +1688,10 @@ func TestUserForgotPassword(t *testing.T) {
Email: anotherUser.Email,
Password: newPassword,
})
require.Error(t, err)
var apiErr *codersdk.Error
require.ErrorAs(t, err, &apiErr)
require.Equal(t, http.StatusUnauthorized, apiErr.StatusCode())
require.Contains(t, apiErr.Message, "Incorrect email or password.")

err = anotherClient.RequestOneTimePasscode(ctx, codersdk.RequestOneTimePasscodeRequest{
Email: anotherUser.Email,
Expand Down Expand Up @@ -1722,7 +1725,9 @@ func TestUserForgotPassword(t *testing.T) {
OneTimePasscode: oneTimePasscode,
Password: "SomeDifferentSecurePassword!",
})
require.Error(t, err)
require.ErrorAs(t, err, &apiErr)
require.Equal(t, http.StatusBadRequest, apiErr.StatusCode())
require.Contains(t, apiErr.Message, "Incorrect email or one-time passcode.")
})

t.Run("CannotChangePasswordWithInvalidOneTimePasscode", func(t *testing.T) {
Expand Down Expand Up @@ -1755,11 +1760,10 @@ func TestUserForgotPassword(t *testing.T) {
OneTimePasscode: uuid.New().String(), // Use a different UUID to the one expected
Password: "SomeNewSecurePassword!",
})
require.Error(t, err)

var apiErr *codersdk.Error
require.ErrorAs(t, err, &apiErr)
require.Equal(t, http.StatusBadRequest, apiErr.StatusCode())
require.Contains(t, apiErr.Message, "Incorrect email or one-time passcode")
})

t.Run("CannotChangePasswordWithNoOneTimePasscode", func(t *testing.T) {
Expand Down Expand Up @@ -1792,11 +1796,12 @@ func TestUserForgotPassword(t *testing.T) {
OneTimePasscode: "",
Password: "SomeNewSecurePassword!",
})
require.Error(t, err)

var apiErr *codersdk.Error
require.ErrorAs(t, err, &apiErr)
require.Equal(t, http.StatusBadRequest, apiErr.StatusCode())
require.Contains(t, apiErr.Message, "Validation failed.")
require.Equal(t, 1, len(apiErr.Validations))
require.Equal(t, "one_time_passcode", apiErr.Validations[0].Field)
})

t.Run("CannotChangePasswordWithWeakPassword", func(t *testing.T) {
Expand Down Expand Up @@ -1831,11 +1836,12 @@ func TestUserForgotPassword(t *testing.T) {
OneTimePasscode: oneTimePasscode,
Password: "notstrong",
})
require.Error(t, err)

var apiErr *codersdk.Error
require.ErrorAs(t, err, &apiErr)
require.Equal(t, http.StatusBadRequest, apiErr.StatusCode())
require.Contains(t, apiErr.Message, "Invalid password.")
require.Equal(t, 1, len(apiErr.Validations))
require.Equal(t, "password", apiErr.Validations[0].Field)
})

t.Run("GivenOKResponseWithInvalidEmail", func(t *testing.T) {
Expand Down
4 changes: 2 additions & 2 deletions codersdk/users.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -569,7 +569,7 @@ func (c *Client) RequestOneTimePasscode(ctx context.Context, req RequestOneTimeP
}
defer res.Body.Close()

if res.StatusCode != http.StatusOK {
if res.StatusCode != http.StatusNoContent {
return ReadBodyAsError(res)
}

Expand All @@ -583,7 +583,7 @@ func (c *Client) ChangePasswordWithOneTimePasscode(ctx context.Context, req Chan
}
defer res.Body.Close()

if res.StatusCode != http.StatusOK {
if res.StatusCode != http.StatusNoContent {
return ReadBodyAsError(res)
}

Expand Down
12 changes: 6 additions & 6 deletions docs/reference/api/authorization.md
View file
Open in desktop

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

Loading