fix(settings): redirect user on AAL mismatch in mfa guard #19599
Conversation
| MfaOtpRequestCache.set(sessionToken, requiredScope); | ||
| await authClient.mfaRequestOtp(sessionToken, requiredScope); | ||
| } catch (err) { | ||
| MfaOtpRequestCache.remove(sessionToken, requiredScope); |
There was a problem hiding this comment.
What's the reason for removing the call to the error boundary? Assuming there's a good reason, we probably want to also clear the MfaOtpRequestCache request cache too so it's consistent with the error-boundaries logic.
There was a problem hiding this comment.
It doesn't make sense to call the error boundary here, since the MFA error boundary handles invalid jwt and does not handle other authorization errors. (In this case it is not even possible for MfaErrorBoundary to differentiate invalid jwt and AAL mismatch, since they use the same errno INVALID_TOKEN)
| setLocalizedErrorBannerMessage(undefined); | ||
| setShowResendSuccessBanner(true); | ||
| } catch (err) { | ||
| MfaOtpRequestCache.remove(sessionToken, requiredScope); |
There was a problem hiding this comment.
See comment above about removing call to error boundary.
| } = await authClient.sessionStatus(sessionToken); | ||
| if (!sessionVerificationMeetsMinimumAAL) { | ||
| console.warn('2FA must be entered to access /settings!'); | ||
| navigateWithQuery('/signin_totp_code', { |
There was a problem hiding this comment.
When testing locally, I see a flash of the child component before navigation - we should render a LoadingSpinner until we've resolved AAL/JWT checks
| const { | ||
| details: { sessionVerificationMeetsMinimumAAL }, | ||
| } = await authClient.sessionStatus(sessionToken); | ||
| if (!sessionVerificationMeetsMinimumAAL) { |
There was a problem hiding this comment.
I feel like this logic really belogns in an error boundary. If we inline it in this component, I feel like we will inevtiably start copying this logic into any component that could potentially hit that states. That's alot of extra checks and code to copy around!
I think the better approach is making sure the error response from the server for an insuffecient AAL is consistent (it should be since this is check is now down the auth-schemes), and then respond to it at the error boundary.
Because: * General Application Error when interacting with site after 2FA enabled This commit: * redirects user on AAL mismatch in mfa guard Closes #FXA-12576
MagentaManifold
force-pushed
the
FXA-12576
branch
from
cb6cba6 to
9e1aac9
Compare
Because
This pull request
Issue that this pull request solves
Closes: FXA-12576
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)
Any other information that is important to this pull request.