Skip to content

fix(settings): Navigate to confirmation if we don't have the recoveryKey #18156

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.

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Merged
jonalmeida merged 1 commit into from
Jan 13, 2025
Merged

fix(settings): Navigate to confirmation if we don't have the recoveryKey #18156

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 21 additions & 11 deletions ...s/fxa-settings/src/pages/ResetPassword/ResetPasswordWithRecoveryKeyVerified/container.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
* License, v. 2.0. If a copy of the MPL was not distributed with this
* file, You can obtain one at http://mozilla.org/MPL/2.0/. */

import { useCallback, useState } from 'react';
import { useCallback, useEffect, useState } from 'react';
import { RouteComponentProps, useLocation } from '@reach/router';
import { ResetPasswordWithRecoveryKeyVerifiedProps } from './interfaces';
import { formatRecoveryKey } from '../../../lib/utilities';
Expand Down Expand Up @@ -45,20 +45,30 @@ const ResetPasswordWithRecoveryKeyVerifiedContainer = ({

const account = useAccount();
const { uid, sessionToken } = currentAccount() || {};
Comment thread
jonalmeida marked this conversation as resolved.
const { keyFetchToken, unwrapBKey } =
sensitiveDataClient.getDataType(SensitiveData.Key.AccountReset) || {};
// We keep the previous non-null assertion on 'newRecoveryKey' here because the
// flow dictates we definitely have it. This is not good practice.
// TODO: Fix me with FXA-10869.
const { recoveryKey } = sensitiveDataClient.getDataType(
SensitiveData.Key.NewRecoveryKey
)!;
const newRecoveryKey = formatRecoveryKey(recoveryKey);

const updateRecoveryKeyHint = useCallback(
async (hint: string) => account.updateRecoveryKeyHint(hint),
[account]
);
const { keyFetchToken, unwrapBKey } =
sensitiveDataClient.getDataType(SensitiveData.Key.AccountReset) || {};
const recoveryKey = sensitiveDataClient.getDataType(
SensitiveData.Key.NewRecoveryKey
)?.recoveryKey;

useEffect(() => {
if (!recoveryKey) {
// If we get here, that means a password reset was completed (with a verified account).
// Along the way, we have lost a copy of the recovery key in memory if the page was unloaded.
// This is fine - we navigate to the confirmed page and carry on.
navigateWithQuery('/reset_password_verified', { replace: true });
}
}, [recoveryKey, navigateWithQuery]);

if (!recoveryKey) {
Copy link
Copy Markdown
Contributor

@vpomerleau vpomerleau Dec 19, 2024

Choose a reason for hiding this comment

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

I'm wondering if we should lift the check for sessionToken and uid out of the navigateNext function and above the check for recoveryKey - if that data is not in local storage then we might as well go directly to signin

Copy link
Copy Markdown
Contributor Author

@jonalmeida jonalmeida Jan 7, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Oh, that's a good idea! It would cancel out the password-reset-with-recovery-key flow but that is probably okay if we don't have these bits needed to get started in the first place which they would reach eventually after all when it's too late.

EDIT: So I tested this out and it doesn't seem super useful - since we have already reached a verified and recovered state, I don't think we need to lift it there.

Copy link
Copy Markdown
Contributor

@vpomerleau vpomerleau Jan 8, 2025

Choose a reason for hiding this comment

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

This was a few weeks ago, so I'm no longer sure if this line previously used navigateNext or if I just had a general observation about the session token check in the navigateNext function and couldn't directly comment on the unmodified line.

The general thought was that if we reach this page without a sessionToken, we should immediately redirect to signin on render

Copy link
Copy Markdown
Contributor Author

@jonalmeida jonalmeida Jan 8, 2025

Choose a reason for hiding this comment

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

Yeah, that logic makes sense to me and it would have been nice if we could have early navigated without the gnarliness.

return;
}

const newRecoveryKey = formatRecoveryKey(recoveryKey.buffer);

const navigateToHint = () => setShowHint(true);
const navigateNext = async (continueToAccountEvent: () => void) => {
Expand Down