Clean up other email tokens #1572
Merged
Conversation
This file contains 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
devinivy
reviewed
Sep 12, 2023
Comment on lines
15
to
16
| // for lookups where we don't know the did | ||
| await db.schema.createIndex('email_token_token_idx').on('token').execute() |
There was a problem hiding this comment.
This may need to be unique when paired with purpose to ensure two dids don't end-up with the same token. I believe the passwordResetToken column dropped below did have this uniqueness constraint on it for that purpose.
(Relatedly, I forget if that index/unique-constraint automatically gets dropped when the column is dropped. If so, I wonder if the DROP INDEX is not done CONCURRENTLY and can cause table locking.)
There was a problem hiding this comment.
took care of this in a previous PR with a unique index on (purpose, token) 👍
devinivy
approved these changes
Sep 12, 2023
devinivy
approved these changes
Sep 28, 2023
mloar
pushed a commit
to mloar/atproto
that referenced
this pull request
Nov 15, 2023
* lexicons * codegen * email templates * request routes * impl * migration * tidy * tests * tidy & bugfixes * format * fix api test * fix auth test * impl * update constraint name * temporarily disable unconfirmed updates * tidy * fix some tests * validate email syntax
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Optional addition on top of #1568
Move account delete & password reset tokens to the new
email_tokentable & logic.This cleans up the routes substantially, removes password reset state from the
user_accounttable and gets some of this sensitive logic all co-located.The migration is a little bit tricky here because we'll end up breaking some folks' deletes/password resets. But it will just be for a short time and it might be worth it. We can do it in off-peak hours to mitigate the impact