fix(settings): Nav to signin totp if already enabled #19620
Conversation
vpomerleau
force-pushed
the
polish-totp-navigation
branch
from
bc0eb89 to
9d37fc9
Compare
MagentaManifold
left a comment
MagentaManifold
left a comment
There was a problem hiding this comment.
I have no idea how to actually test these, but the logic and the unit tests you added look solid. I added a comment for potentially making the logic simpler
| totpStatus?.account?.totp.verified === true || | ||
| isTotpCreating.current | ||
| isTotpCreating.current || | ||
| totpStatusLoading === true |
There was a problem hiding this comment.
Nit: totpStatusLoading is a boolean, so there's no need for === true. The other occurrence on line 175 in the same file is probably a mistake
| const handleOAuthRedirectError = async (error: AuthError) => { | ||
| if ( | ||
| error.errno === AuthUiErrors.TOTP_REQUIRED.errno || | ||
| error.errno === AuthUiErrors.INSUFFICIENT_ACR_VALUES.errno |
There was a problem hiding this comment.
If my understanding is correct (lmk if I'm wrong): TOTP_REQUIRED means TOTP is not enabled (but should be), while INSUFFICIENT_ACR_VALUES means TOTP is enabled, but the user doesn't have a session with this ACR (i.e., AAL2). So we can distinguish the two cases just by handling these two errnos separately, saving us an API call. Ditto for Signin/utils.ts
Because: * We were seeing some AAL errors on createTotp This commit: * Ensure that inline_totp_setup waitf totp status to resolve before starting setup process * Add a few more navigation intercepts to navigate to signin_totp_code instead of inline_totp_setup if totp already enabled * Add a couple of tests for inline_totp_setup Closes
vpomerleau
force-pushed
the
polish-totp-navigation
branch
from
9d37fc9 to
b0ff926
Compare
Because
This pull request
Issue that this pull request solves
Closes: FXA-12681
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)
This is to address some edge cases where users may land on inline_totp_setup when they already have 2FA enabled.