Skip to content
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

Merged
merged 9 commits into from
Aug 5, 2024

Conversation

lmuntaner
Copy link
Collaborator

@lmuntaner lmuntaner commented Jul 30, 2024

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:

  • 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 less than one device.
    (a phrase and pin are not considered a device, only normal devices or recovery devices)
  • The user has not disabled the warning.
    (users can choose to not see the warning again by clicking "do not remind" button)

🟡 Some screens were changed

@lmuntaner
Copy link
Collaborator Author

Scenario: user with one device logs in II and clicks "Remind later"

one-passkey-remind-later.mov

Scenario: user with one device logs in dapp and clicks "Do not remind me"

one-passkey-remind-later.mov

Scenario: user with one device logs in dapp and adds a new device

one-passkey-add-device-auth.mov

Scenario: user with only pin logs in dapp and clicks "Remind later"

pin-only-remind-later-auth.mov

@lmuntaner lmuntaner marked this pull request as ready for review July 31, 2024 09:11
@lmuntaner lmuntaner requested a review from a team as a code owner July 31, 2024 09:11
Copy link

@anedos-dfinity anedos-dfinity left a 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

src/frontend/src/flows/recovery/recoveryWizard.ts Outdated Show resolved Hide resolved
src/frontend/src/flows/recovery/recoveryWizard.ts Outdated Show resolved Hide resolved
@lmuntaner
Copy link
Collaborator Author

LGTM, I've looked mainly at the tests, addDeviceWarning, getDevicesStatus and recoveryWizard, less so at updateMetadataMapV2

That makes sense, those are the new features.

Copy link
Member

@frederikrothenberger frederikrothenberger left a 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)

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") {

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({

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 {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice tests! 👍

@frederikrothenberger frederikrothenberger added this pull request to the merge queue Aug 5, 2024
Merged via the queue into main with commit ce33116 Aug 5, 2024
65 checks passed
@frederikrothenberger frederikrothenberger deleted the lm-use-new-warning-page branch August 5, 2024 13:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants