Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

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.

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
DanielleMaywood merged 25 commits intomainfromdm-request-and-submit-passcode
Oct 4, 2024

Conversation

DanielleMaywood
Copy link
Contributor

@DanielleMaywoodDanielleMaywood commentedOct 1, 2024•
edited
Loading

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.

Screenshot 2024-10-01 at 15 46 36

@DanielleMaywoodDanielleMaywood changed the titlefeat: implement api for "forgot password? flowfeat: implement api for "forgot password?" flowOct 1, 2024
@stirby
Copy link
Collaborator

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
Copy link
ContributorAuthor

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.

Screenshot 2024-10-01 at 16 57 18

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
Copy link
Contributor

@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
Copy link
Contributor

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

Copy link
Contributor

@dannykoppingdannykopping left a comment

Choose a reason for hiding this comment

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

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


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.",
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

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

coder/coderd/users.go

Lines 1048 to 1054 in32a8df4

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

dannykopping reacted with thumbs up emoji
Copy link
Contributor

Choose a reason for hiding this comment

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

WDYT@stirby?

Copy link
Contributor

Choose a reason for hiding this comment

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

Did we come to a decision on this?

return xerrors.Errorf("update user hashed password: %w", err)
}

//nolint:gocritic // We need to delete all API keys for the user.
Copy link
Contributor

Choose a reason for hiding this comment

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

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 dox. 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 articulatedand documented. When in doubt, follow the principle of least surprise.

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

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

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

coder/coderd/users.go

Lines 1074 to 1077 in32a8df4

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Hhmm, do we document this behaviour anywhere?

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

If we do I am unable to find it.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

@DanielleMaywoodDanielleMaywood marked this pull request as ready for reviewOctober 2, 2024 15:19
Copy link
Member

@mafredrimafredri left a comment

Choose a reason for hiding this comment

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

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

}
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
ContributorAuthor

Choose a reason for hiding this comment

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

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 byRFC6749, 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
ContributorAuthor

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
ContributorAuthor

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 entropyshould be sufficient. We can always review this in the future if we decide a different approach should be taken.

mtojek reacted with thumbs up emoji
}
aReq.Old = user

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
ContributorAuthor

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
ContributorAuthor

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.

}

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


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

Choose a reason for hiding this comment

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

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

dannykopping reacted with thumbs up emoji
Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

Can do!

}
aReq.Old = user

passcode := uuid.New()
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
Member

@mtojekmtojek left a comment

Choose a reason for hiding this comment

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

Last to the party, but added a few cents 👍

Nice work, Danielle!

Copy link
Member

@mtojekmtojek left a comment

Choose a reason for hiding this comment

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

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

Copy link
Member

@mafredrimafredri left a comment

Choose a reason for hiding this comment

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

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

@DanielleMaywoodDanielleMaywood merged commit4369f2b intomainOct 4, 2024
28 of 30 checks passed
@DanielleMaywoodDanielleMaywood deleted the dm-request-and-submit-passcode branchOctober 4, 2024 10:53
@github-actionsgithub-actionsbot locked and limited conversation to collaboratorsOct 4, 2024
@dannykopping
Copy link
Contributor

Great work@DanielleMaywood!

Sign up for freeto subscribe to this conversation on GitHub. Already have an account?Sign in.
Reviewers

@mafredrimafredrimafredri approved these changes

@mtojekmtojekmtojek approved these changes

@dannykoppingdannykoppingAwaiting requested review from dannykopping

Assignees

@DanielleMaywoodDanielleMaywood

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

5 participants
@DanielleMaywood@stirby@dannykopping@mafredri@mtojek
[8]ページ先頭
©2009-2025 Movatter.jp