-
Notifications
You must be signed in to change notification settings - Fork 142
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
Use new warning page and change the rendering logic #2551
Conversation
Scenario: user with one device logs in II and clicks "Remind later" one-passkey-remind-later.movScenario: user with one device logs in dapp and clicks "Do not remind me" one-passkey-remind-later.movScenario: user with one device logs in dapp and adds a new device one-passkey-add-device-auth.movScenario: user with only pin logs in dapp and clicks "Remind later" pin-only-remind-later-auth.mov |
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.
LGTM, I've looked mainly at the tests, addDeviceWarning
, getDevicesStatus
and recoveryWizard
, less so at updateMetadataMapV2
That makes sense, those are the new features. |
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.
LGTM, but we should revisit the comments I made once you're back, @lmuntaner.
* * Not having seen the recovery page in the last week | ||
* (on registration, the user is not shown the page, but set it as seen to not bother during the onboarding) | ||
* * The user has at most one device. | ||
* (a phrase and pin are not considered a device, only normal devices or recovery devices) |
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 think this is a questionable decision. IMHO, having one device and a recovery phrase should be fine. Otherwise, people owning only one WebAuthn capable device are forced to disable the warning rather than being able to remediate the situation with an appropriate action (namely setting up a recovery phrase).
nowInMillis, | ||
}); | ||
|
||
if (devicesStatus !== "no-warning") { |
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 think the code would be simpler if this condition was flipped (with early return).
doAdd satisfies "ok"; | ||
|
||
await setupRecovery({ userNumber, connection }); | ||
const userChoice = await addDeviceWarning({ |
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.
These warning screens are now inconsistent with the messaging on the manage page when a user only has a pin identity: the manage page strongly suggests setting up a recovery phrase, whereas this screen only nudges with regards to a passkey.
Furthermore, the warning on the management page even persists after having added 2 passkeys. We should reconsider the warning screens on the management page too.
@@ -0,0 +1,204 @@ | |||
import { |
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.
Nice tests! 👍
Motivation is to stop recommending recovery phrase and instead recommend securing the identity with multiple devices.
We added a new warning page in #2550.
In this PR, the recoveryWizard uses the new page and also changes the logic of rendering.
Three conditions must be met for the warning page to be shown:
(on registration, the user is not shown the page, but set it as seen to not bother during the onboarding)
(a phrase and pin are not considered a device, only normal devices or recovery devices)
(users can choose to not see the warning again by clicking "do not remind" button)
🟡 Some screens were changed