fix(react): Password reset unexpected error fixes, with and without recovery key #15392
Conversation
LZoog
changed the title
| @@ -89,7 +89,6 @@ export async function generateRecoveryKey( | |||
| */ | |||
| export async function decryptRecoveryKeyData( | |||
| recoveryKey: Uint8Array, | |||
| recoveryKeyId: string, | |||
There was a problem hiding this comment.
This parameter was unused.
| ) { | ||
| return this.authAPI.accountReset( | ||
| ): Promise<AccountResetPayload> { | ||
| const result = await this.authAPI.accountReset( |
There was a problem hiding this comment.
I'm not 100% certain this was necessary, but I changed this while debugging and it now matches other resolver functions result/returns. We already have tests written for it.
| @@ -14,6 +14,22 @@ export interface OldSettingsData { | |||
| metricsEnabled?: boolean; | |||
| } | |||
|
|
|||
| export function getOldSettingsData({ | |||
There was a problem hiding this comment.
Without this, I was getting errors in content-server after reset around disallowed keys attempting to be set, like authAt and clientMutationId.
| * sessionToken.mustVerify which was added here: https://github.com/mozilla/fxa/pull/7512 | ||
| * The unverified session token returned by a password reset contains `mustVerify` | ||
| * which causes the 'Must verify' error to be thrown and a redirect to occur */ | ||
| async isSessionVerifiedAuthClient() { |
There was a problem hiding this comment.
I like the idea of some differentiator, like ending in AuthClient for methods that use auth client over apollo client. It makes it more clear to me what's happening when we call it and that we need to refactor it.
| function navigateAway() { | ||
| setUserPreference('account-recovery', account.recoveryKey); | ||
| async function navigateAway() { | ||
| // TODO / refactor: the initial account result with useAccount() does not |
There was a problem hiding this comment.
There aren't issues filed for the TODOs in this PR yet, but we will definitely be filing issues out of the settings architecture doc which specifically calls out our API calls in Account, so these will probably be filed whenever the rest of those are.
| // ); | ||
| await waitFor(() => { | ||
| expect(mockNavigate).toHaveBeenCalledWith( | ||
| expect.stringContaining('/account_recovery_confirm_key'), |
There was a problem hiding this comment.
We're currently doing different things regarding parameters in our tests for reset PW at the moment, and it makes it challenging to be consistent. If I mock window.location.search here to more explicitly test what this is being called with, the expired or damaged link is rendered instead due to the test set up. However as-is, window.location renders as undefined so to get this passing I'd have to test .toHaveBeenCalledWith('/account_recovery_confirm_keyundefined' or something like that. It seemed fine to test stringContaining for now (and toContain further down) and revisit our tests later when we do any architecture-related refactoring.
| // TODO / refactor: the initial account result with useAccount() does not | ||
| // contain account data due to no session token. Users receive the session | ||
| // token on PW reset, so we can query here for it. | ||
| const hasTotp = await account.hasTotpAuthClient(); |
There was a problem hiding this comment.
Just realized, this should be in a try/catch here or in Account.ts, I'll add that before merge
There was a problem hiding this comment.
Done. Also added this check in case the check failed, where I'm just returning false as a fallback, but if this is false when the user does have TOTP, they’d get stuck in a weird state where they’d see a flash of reset PW verified and then they’d be taken to /signin_token_code (since that function was getting hit), with an undefined email which I couldn’t get out of without hitting /clear since it was hitting that mustVerify issue.
Issue filed for mustVerify: https://mozilla-hub.atlassian.net/browse/FXA-7626
…ecovery key Because: * There's an error when attempting to reset a password with an account recovery key, and other issues were uncovered during the fix This commit: * Fixes logic around accountResetToken, ensuring we only fetch getRecoveryBundle once with the same token * Requests, receives, and sets sessionToken from account recovery reset and normal PW reset * Uses this session token to perform other requests that must use authClient for now due to 'sessionToken.mustVerify' checks Closes FXA-7189
Because:
This commit:
Closes FXA-7189, FXA-7152
I think we're going to have to unfortunately change all of our other reset PW apollo client calls back to auth client temporarily, which is probably going to fix at least a decent number of bugs. I'm guessing we've been accidentally doing a lot of testing while still authenticated, instead of actually signing out or trying to PW reset in another browser - if you "sign out" before any resets, you'll see lots of problems around us needing a session token to use our GQL API. See the GQL & API calls section of the WIP Settings architecture doc. I'd like us to do this before we convert any other pages personally.Turns out this is not true, Vijay pinged me and let me know he'd done testing without a session token and that we can retrieve it during PW reset. I'll update our doc - the problem is that when we try to access account data viauseAccountand the session token is null, or in the case now, when the session token is returningmustVerify(made notes in code).I've tested:
The only difference with the old version is it this displays a flash of "Password reset" after a reset but before redirect to TOTP. This would be an a11y issue, but seeing as we don't display anything in the old version and we are working on the redesign directly after this is out, I think it's OK to leave.