feat: implement api for "forgot password?" flow by DanielleMaywood · Pull Request #14915 · coder/coder

DanielleMaywood · 2024-10-01T14:12:33Z

Relates to #14232

This implements two endpoints (names subject to change):

/api/v2/users/otp/request
/api/v2/users/otp/change-password

They are both in the same group as /api/v2/users/login so they have the same rate limiting logic applied.

stirby · 2024-10-01T15:40:41Z

What does this look like in the settings? I know we want to avoid adding notification-specific functionality but I'm not sure that this should appear in a "Notification Preferences" table.

Users probably shouldn't be able to disable the flow at all.

DanielleMaywood · 2024-10-01T15:59:29Z

What does this look like in the settings? I know we want to avoid adding notification-specific functionality but I'm not sure that this should appear in a "Notification Preferences" table.

Users probably shouldn't be able to disable the flow at all.

It appears like the rest of the notifications.

I definitely agree that a user shouldn't be able to disable it. I'm happy to look into stopping it from being displayed on this page.

@dannykopping What're your thoughts on this?

dannykopping · 2024-10-01T17:16:47Z

@dannykopping What're your thoughts on this?

This case is easily fixable and diagnosable, so I think special-casing is not worth the extra complexity.

dannykopping · 2024-10-01T18:02:41Z

To add some more context:

In my experience, special-casing ends up rotting over time. I definitely understand the motivation here though @stirby, and it will indeed be quite a simple special-casing, so if you feel strongly about it then let's do it.

I think the back-end should be responsibly for hiding that option, and it should be done based on the template ID (not the name).

dannykopping

I appreciate your security awareness here, and the methodical approach.
There are a few things to fix, but nothing major.

dannykopping · 2024-10-02T07:09:10Z

+
+	if equal, _ = userpassword.Compare(string(user.HashedPassword), req.Password); equal {
+		httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{
+			Message: "New password cannot match old password.",


I'm curious: why not? If they're forgotten their password but have now somehow remembered it, do we have to force a change?

I'm happy to go either way on this. The decision to do this was based on similar logic being performed on the putUserPassword endpoint.

coder/coderd/users.go

Lines 1048 to 1054 in 32a8df4

// Prevent users reusing their old password.

if match, _ := userpassword.Compare(string(user.HashedPassword), params.Password); match {

httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{

Message: "New password cannot match old password.",

})

return

}

WDYT @stirby?

Did we come to a decision on this?

dannykopping · 2024-10-02T07:09:39Z

+			return xerrors.Errorf("update user hashed password: %w", err)
+		}
+
+		//nolint:gocritic // We need to delete all API keys for the user.


Why? API keys are separate authorization contexts to user passwords.

For example, as an operator I might've created an API key to use in CI to do x. It would be pretty weird and unexpected to invalidate that key if I forgot my personal password.

I'm not against this, but the reasons need to be carefully articulated and documented. When in doubt, follow the principle of least surprise.

The putUserPassword endpoint has this behaviour. My assumption is that we'd want the behaviour to match. Unfortunately there are no comments in the immediate area justifying the decision.

coder/coderd/users.go

Line 977 in 32a8df4

func (api *API) putUserPassword(rw http.ResponseWriter, r *http.Request) {

coder/coderd/users.go

Lines 1074 to 1077 in 32a8df4

err = tx.DeleteAPIKeysByUserID(ctx, user.ID)

if err != nil {

return xerrors.Errorf("delete api keys by user ID: %w", err)

}

I'm happy to have a discussion around whether this should stay or go.

Hhmm, do we document this behaviour anywhere?

If we do I am unable to find it.

Given that this behaviour exists, we can't change it now - and I think that it'd be the expectation of operators for this to behave similarly.

I do think we should document this explicitly (could be done out-of-band of this PR), and raise an issue to clarify why we do this and possibly remove it for the reasons I outlined initially.

@DanielleMaywood would you mind creating an issue for follow-up?

mafredri

Nice work! Had a few suggestions, random thoughts and one larger concern.

mafredri · 2024-10-02T17:23:22Z

+	}
+	aReq.Old = user
+
+	passcode := uuid.New()


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

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

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

@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 👍🏻

Ha! TIL 🥲

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

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

mafredri · 2024-10-02T17:43:26Z

+		}
+		aReq.Old = user
+
+		equal, err := userpassword.Compare(string(user.HashedOneTimePasscode), req.OneTimePasscode)


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?

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

I've now added a test to verify that tokens do expire.

mafredri · 2024-10-02T17:43:53Z

+		}
+
+		//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{


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

mafredri · 2024-10-02T17:47:15Z

+
+		notif := notifyEnq.Sent[0]
+		require.NotEqual(t, notifications.TemplateUserRequestedOneTimePasscode, notif.TemplateID)
+	})


A few more test-cases could be good, like expired token and resetting the pw of a user that hasn't requested one.

dannykopping · 2024-10-03T06:33:51Z

+	}
+	aReq.Old = user
+
+	passcode := uuid.New()


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

mtojek

Last to the party, but added a few cents 👍

Nice work, Danielle!

mtojek

Perfect 👍 I think it is in good shape to merge, but let's wait for @mafredri 's approval as Danny is out.

mafredri

Looking great! Approving since I think the remaining stuff is pretty minor mainly documenting the reasoning and adding one more test.

dannykopping · 2024-10-04T11:46:22Z

Great work @DanielleMaywood!

DanielleMaywood added 3 commits October 1, 2024 10:59

feat: add api for forgotten password flow

b44adff

fix: appease linter

60fc32c

fix: give 20 minutes for one time passcode

32a8df4

github-actions Bot assigned DanielleMaywood Oct 1, 2024

DanielleMaywood changed the title ~~feat: implement api for "forgot password? flow~~ feat: implement api for "forgot password?" flow Oct 1, 2024

dannykopping reviewed Oct 2, 2024

View reviewed changes

DanielleMaywood added 9 commits October 2, 2024 08:56

chore: changes from feedback

e486923

fix: make ChangePasswordWithOneTimePassword expect StatusOK

da87e9b

docs: run 'make gen'

39fa0f1

test: add more tests

b89dfd7

test: ensure login fails before changing password

714e316

refactor: test and dbmem.UpdateUserHashedOneTimePasscode

c4ed205

fix: do not enqueue notification if user.ID is uuid.Nil

fa9d426

refactor: wrap GetUserByEmailOrUsername in transaction

883a231

refactor: error handling

bff384b

DanielleMaywood marked this pull request as ready for review October 2, 2024 15:19

DanielleMaywood requested review from dannykopping, mafredri and mtojek October 2, 2024 15:19

mafredri reviewed Oct 2, 2024

View reviewed changes

DanielleMaywood added 3 commits October 2, 2024 18:31

fix: check one-time passcode expiry

26cc758

test: one-time passcode becomes invalid after use

25581c2

Merge branch 'main' into dm-request-and-submit-passcode

764b0cc

dannykopping reviewed Oct 3, 2024

View reviewed changes

mtojek reviewed Oct 3, 2024

View reviewed changes

fix: make changes from feedback

6078409

DanielleMaywood added 5 commits October 3, 2024 09:42

refactor: change endpoints

3a0946c

fix: update assertSecurityDefined links

1a7ad7a

refactor: make validity period an option and test it

6bbcff2

test: refactor tests to reduce duplication

dfc32b1

test: ensure cannot login after failed password change

9f80082

DanielleMaywood requested review from dannykopping, mafredri and mtojek October 4, 2024 08:20

mtojek approved these changes Oct 4, 2024

View reviewed changes

Comment thread coderd/userauth.go

fix: imports

fe9b3f8

mafredri approved these changes Oct 4, 2024

View reviewed changes

DanielleMaywood added 3 commits October 4, 2024 10:12

test: ensure can login with old credentials

b42b73c

chore: apply changes based on feedback

034a7d4

chore: add missing comment

e14d43b

DanielleMaywood merged commit 4369f2b into main Oct 4, 2024

DanielleMaywood deleted the dm-request-and-submit-passcode branch October 4, 2024 10:53

github-actions Bot locked and limited conversation to collaborators Oct 4, 2024

	// Prevent users reusing their old password.
	if match, _ := userpassword.Compare(string(user.HashedPassword), params.Password); match {
	httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{
	Message: "New password cannot match old password.",
	})
	return
	}

	err = tx.DeleteAPIKeysByUserID(ctx, user.ID)
	if err != nil {
	return xerrors.Errorf("delete api keys by user ID: %w", err)
	}

Conversation

DanielleMaywood commented Oct 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stirby commented Oct 1, 2024

Uh oh!

DanielleMaywood commented Oct 1, 2024

Uh oh!

dannykopping commented Oct 1, 2024

Uh oh!

dannykopping commented Oct 1, 2024

Uh oh!

dannykopping left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mafredri left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

DanielleMaywood commented Oct 1, 2024 •

edited

Loading