feat: implement api for "forgot password?" flow #14915
fix: give 20 minutes for one time passcode
- Loading branch information
commit 32a8df4b9b09c3aede8fd02a858a8167baaafb66
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
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. 🤔
There was a problem hiding this comment.
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? 😆
There was a problem hiding this comment.
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
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
There was a problem hiding this comment.
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 👍🏻
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ha! TIL 🥲
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.