fix(settings): Redirect users from /settings if session is unverified #19546
Conversation
| } | ||
|
|
||
| // If we're on settings route but user is not signed in, redirect immediately | ||
| if (window.location.pathname?.includes('/settings') && !isSignedIn) { |
There was a problem hiding this comment.
We already have this same check for isSignedIn, under <SettingsRoutes />, so this seemed redundant. I'm not even sure if we need that isSignedIn check, but I'll look at that in the same follow up related to this stuff that I need to do.
I went ahead and removed hardNavigate here as well since we don't need it any longer.
LZoog
force-pushed
the
FXA-12472
branch
2 times, most recently
from
6924d57 to
e98f275
Compare
|
|
||
| async signout() { | ||
| const button = await this.element.waitForSelector( | ||
| const button = this.element.locator( |
There was a problem hiding this comment.
I had to make these functional-test changes due to these waitForSelectors. There was a race condition where this was showing as "hidden", I guess due to the new isSessionVerified call. waitFor is the recommendation for Playwright but it won't work on multiple elements.
There was a problem hiding this comment.
I'm curious what you mean by it "won't work on multiple elements". Is the selector here returning multiple elements?
PW should internally wait for visible as part of their "actionable" checks. I wonder if you'd be able to get away with removing the waitFor() all together?
There was a problem hiding this comment.
Hmmm, possibly. My PR fails with waitForSelector due to it being "hidden", but I mistakenly used page.locator, before I realized we have a ConnectedServicesRow to check the page and that this was intended to be a single element. I reverted back to element.selector so I'll take a look at this again.
There was a problem hiding this comment.
Ran a couple of tests locally that were previously failing and they passed, so pushed the update 🤞
There was a problem hiding this comment.
@nshirley they passed! Woo. Thanks for reviewing.
Because: * Users should not be able to directly bypass /signin_token_code This commit: * Uses session.isSessionVerified to check if the user should be redirected away closes FXA-12472
Because:
This commit:
closes FXA-12472
We can now use
session.isSessionVerifiedfor this as of my last PR.You can test this with the
SIGNIN_CONFIRMATION_TOKEN_VERIFICATIONflag to get in an unverified state. Try to access settings directly after you're taken to/signin_token_code, and see you'll be redirected away from Settings.