-
Notifications
You must be signed in to change notification settings - Fork 17
feat: CP-11530 seedless recovery methods #426
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.
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
base: main
Are you sure you want to change the base?
Conversation
|
Feature flags are missing |
apps/next/src/pages/Settings/components/RecoveryMethods/Authenticator/Authenticator.tsx
Outdated
Show resolved
Hide resolved
apps/next/src/pages/Settings/components/RecoveryMethods/Authenticator/AuthenticatorDetails.tsx
Outdated
Show resolved
Hide resolved
| export enum AuthenticatorState { | ||
| Initial = 'initial', | ||
| Initiated = 'initiated', | ||
| ConfirmChange = 'confirm-change', | ||
| ConfirmRemoval = 'confirm-removal', | ||
| Pending = 'pending', | ||
| Completing = 'completing', | ||
| VerifyCode = 'verify-code', | ||
| Failure = 'failure', | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Out of curiosity: why to even use enums in Typescript?
IMO they are much more cumbersome to use in comparison to string unions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can use them as values which is not the case with simple types. I don't have to compare and calculate the values since we have them in the enum in this particular case.
apps/next/src/pages/Settings/components/RecoveryMethods/FullScreens/FullScreenContent.tsx
Outdated
Show resolved
Hide resolved
apps/next/src/pages/Settings/components/RecoveryMethods/FullScreens/FullScreenContent.tsx
Outdated
Show resolved
Hide resolved
apps/next/src/pages/Settings/components/RecoveryMethods/FullScreens/RemoveTotp.tsx
Outdated
Show resolved
Hide resolved
| const history = useHistory(); | ||
| const { registerBackButtonHandler, setTotal } = useModalPageControl(); | ||
| const { oidcToken, setSeedlessSignerToken } = useOnboardingContext(); | ||
| // TODO: remove |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this will be the last one to be removed, easiest way my seedless account gets deleted
| setIsVerifying(true); | ||
| await completeAuthenticatorChange(totpChallenge.totpId, code); | ||
| toast.success(t('Authenticator added!'), { | ||
| duration: Infinity, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, it means it will be there until leaving that page state.
| export enum AddFIDOState { | ||
| Initial = 'initial', | ||
| Initiated = 'initiated', | ||
| Failure = 'failure', | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We use enums in these cases besides I think the enums are better to understands the value meaning. I mean if I set a state to 'initial' it won't say any specific meaning in addition it is a state but with the AddFIDOState.Initial I will know okay I'm dealing with the Adding FIDO state.
| export enum AuthenticatorState { | ||
| Initial = 'initial', | ||
| Initiated = 'initiated', | ||
| ConfirmChange = 'confirm-change', | ||
| ConfirmRemoval = 'confirm-removal', | ||
| Pending = 'pending', | ||
| Completing = 'completing', | ||
| VerifyCode = 'verify-code', | ||
| Failure = 'failure', | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can use them as values which is not the case with simple types. I don't have to compare and calculate the values since we have them in the enum in this particular case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did not look through the code, only tested:
- Why is this clickable? Also the card looks different than designs (it has some elevation added) + the background is too light it seems like (without elevation, the card barely stands out):
Screen.Recording.2025-10-28.at.12.51.12.mov
- Is it not a bit confusing that after adding a recovery method, I'm shown this almost empty screen instead of some success state?
- It's very easy to get stuck on the settings page where only restarting the service worker helps to exit:
Screen.Recording.2025-10-28.at.13.04.05.mov
Screen.Recording.2025-10-28.at.13.11.58.mov
There is the toaster for the success state. I've added a "Management" page because if the user wants to add another method the extension has to be opened and closed and the UX is not too great about that. I've done a demo with that and there was no feedback about this. |
Description
The seedless MFA management flow.
Changes
Add the screens and the flows.
Testing
Onboard with a seedless account (preferred if there is no MFA setup) -> try to add remove replace the MFA-s
Screenshots:
Onboarding
Screen.Recording.2025-09-12.at.16.54.18.mov
Adding Authenticator first (which means it will happen in the floating extension)
Screen.Recording.2025-09-12.at.16.56.28.mov
Adding FIDO Devices
Screen.Recording.2025-09-12.at.16.59.23.mov
Removing Authenticator (there is a change option as well)
Screen.Recording.2025-09-12.at.17.02.00.mov
Adding Authenticator again (it happens in a full screen mode)
Screen.Recording.2025-09-12.at.17.03.23.mov
Removing FIDO devices
Screen.Recording.2025-09-12.at.17.05.13.mov
Changing Authenticator app
Screen.Recording.2025-09-12.at.17.06.29.mov
When every recovery method feature flag turned off
Checklist for the author
Tick each of them when done or if not applicable.