Conversation
There was a problem hiding this comment.
I checked out your other PR as well, and after poking around I agree this one is the better approach as it is much simpler and in checking out our code and docs, I don't immediately see why the token has to be single use.
Couple notes:
- On top of the pruning and TTL, the token gets deleted in
resetAccount_19which runs for ARK flow and non-ARK flow, withDELETE FROM passwordForgotTokens WHERE uid = uidArg; - Does this warrant lowering the TTL? It seems reasonable to me right now (
PASSWORD_FORGOT_TOKEN_TTL) in stage/prod, but I could see cutting it a little shorter
We might want Vijay's take too in case he sees something we don't. I didn't test this locally yet but I'm happy to test and r+.
| // Note! This is the start of edge case this test validates. When we provided | ||
| // a recovery key, we took our password forgot token and exchange it for an | ||
| // account reset token, which resulted in the passwordForgotToken becoming | ||
| // invalid. We therefore must use the account reset token for the rest of |
There was a problem hiding this comment.
I think this comment was a copy/paste from the other PR and is no longer correct here.
| inCreatedAt, | ||
| inVerificationMethod | ||
| ); | ||
|
|
There was a problem hiding this comment.
Is it worth having a comment somewhere that the only difference between this migration and the previous one is we no longer delete the token?
LZoog
left a comment
There was a problem hiding this comment.
Tested locally and verified it is working 👍
Because
This pull request
Issue that this pull request solves
Closes: FXA-13162
Checklist
Put an
xin the boxes that applyScreenshots (Optional)
Please attach the screenshots of the changes made in case of change in user interface.
Other information (Optional)
Note this a much cleaner approach than this other draft. The question is whether or not destroying the password forgot token is mandatory. If so maybe that other much more involved approach is necessary...
It looks like that logic was introduced here... and I see no mention for why the change was made. The commit seems more like general refactor / clean up. It's worth noting that
passwordForgotTokensare being pruned by the token pruner, so imo this approach is fine...