Skip to content

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

Merged
jonalmeida merged 1 commit intomainfrom
FXA-10869
Jan 13, 2025
Merged

fix(settings): Navigate to confirmation if we don't have the recoveryKey#18156
jonalmeida merged 1 commit intomainfrom
FXA-10869

Conversation

@jonalmeida
Copy link
Copy Markdown
Contributor

Because

  • We can sometimes get into a point where the page is unloaded after a new
    recovery key is created. We want to gracefully handle this flow since
    the user is already verified.

This pull request

  • We navigate to /reset_password_confirmed if we don't have the recovery key anymore in the SensitiveDataClient.

Issue that this pull request solves

Closes: FXA-10869

Checklist

Put an x in the boxes that apply

  • My commit is GPG signed.
  • If applicable, I have modified or added tests which pass locally.
  • I have added necessary documentation (if appropriate).
  • I have verified that my changes render correctly in RTL (if appropriate).

Screenshots (Optional)

n/a

Other information (Optional)

Built on top of #18136. Will rebase when the prior PR lands.

SensitiveData.Key.NewRecoveryKey
)?.recoveryKey;

if (!recoveryKey) {
Copy link
Copy Markdown
Contributor

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

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

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

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.

@jonalmeida
Copy link
Copy Markdown
Contributor Author

I'm facing a weird bug with this patch where the page refreshes to a blank one first, but a second refresh does what I want. I'm not sure if this is a bug with my local config or not.

I thought it might have been because a duplication in our routes, but it seems like that is unrelated.

@vpomerleau
Copy link
Copy Markdown
Contributor

I think you might be on the right track - while the route definitions in fxa-content-server include both /reset_password_verified and /reset_password_confirmed, the one we currently use in the React app is /reset_password_verified. The doubling in content-server might be an artifact from previously having different views for these routes (before my time on the team).

Using reset_password_confirmed with a React Router navigation bumps out of the React app and that's likely causing the blank page you initially see, unless you also have the same issue when navigating to /reset_password_verified

@vpomerleau
Copy link
Copy Markdown
Contributor

Following up on the blank page issue - changing the route to /reset_password_verified doesn't fully resolve the issue.

However moving the navigation into a useEffect hook as follows seems to work:

  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) {
    return;
  }

@jonalmeida
Copy link
Copy Markdown
Contributor Author

Following up on the blank page issue - changing the route to /reset_password_verified doesn't fully resolve the issue.

However moving the navigation into a useEffect hook as follows seems to work:

  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) {
    return;
  }

Thank you!

@jonalmeida jonalmeida marked this pull request as ready for review January 8, 2025 08:35
@jonalmeida jonalmeida requested a review from a team as a code owner January 8, 2025 08:35
@jonalmeida jonalmeida requested a review from vpomerleau January 8, 2025 08:36
We can sometimes get into a point where the page is unloaded after a new
recovery key is created. We want to gracefully handle this flow since
the user is already verified.

Fixes FXA-10869
Copy link
Copy Markdown
Contributor

@vpomerleau vpomerleau left a comment

Choose a reason for hiding this comment

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

Thanks for the updates here! :shipit:

@jonalmeida
Copy link
Copy Markdown
Contributor Author

I filed this bug where desktop needs to properly handle the URI correctly since we are fixing this bug now: https://bugzilla.mozilla.org/show_bug.cgi?id=1941390

@jonalmeida jonalmeida merged commit ef0bfab into main Jan 13, 2025
@jonalmeida jonalmeida deleted the FXA-10869 branch January 13, 2025 19:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants