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

Retry with differen rp id #2753

Merged
merged 2 commits into from
Dec 18, 2024
Merged

Retry with differen rp id #2753

merged 2 commits into from
Dec 18, 2024

Conversation

lmuntaner
Copy link
Collaborator

@lmuntaner lmuntaner commented Dec 17, 2024

Motivation

There are scenarios where the RP ID selected is not the correct one for the device the user is in. When that happens, we want to tell the user to try again, but this time, we will select another RP ID.

In this PR, I introduce the logic to use another RP ID on retry. See the video for a demo of how it looks like.

What I haven't improved in this PR is the UX when retrying. I'm thinking on only improving the toast error for now and if users are confused improve it. But this is an edge case that I hope not so many people will encounter.

Changes

  • New helpers excludeCredentialsFromOrigins and hasCredentialsFromMultipleOrigins.
  • Add instance variable in Connection: _cancelledRpIds to keep track of the cancelled RP IDs.
  • Add logic to use the _cancelledRpIds to filter the devices with the cancelled RP IDs from the calculation to find the next RP ID.

Tests

  • Test the new functionality.
  • Add tests for excludeCredentialsFromOrigins.

🟡 Some screens were changed

@lmuntaner
Copy link
Collaborator Author

lmuntaner commented Dec 17, 2024

Current UX:

The user is logged in the second attempt.
https://github.com/user-attachments/assets/f1b542a9-f945-4a56-a784-fb3ae1969c38

For some reason, github doesn't embed the videos within the comments now...

PENDING: To improve the UX.

@lmuntaner lmuntaner marked this pull request as ready for review December 17, 2024 18:53
@lmuntaner lmuntaner requested review from sea-snake and LXIF December 17, 2024 18:53
@lmuntaner
Copy link
Collaborator Author

@sea-snake @LXIF please review

Copy link
Contributor

@LXIF LXIF left a comment

Choose a reason for hiding this comment

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

LGTM, though would have to spend dedicated time on it if you want me to verify the logic itself more deeply.

@lmuntaner lmuntaner added this pull request to the merge queue Dec 18, 2024
@lmuntaner
Copy link
Collaborator Author

LGTM, though would have to spend dedicated time on it if you want me to verify the logic itself more deeply.

the logic is written in the design doc in the section "Algorithm To Choose RP ID". Is that what you mean?

Merged via the queue into main with commit e3e48d0 Dec 18, 2024
68 checks passed
@lmuntaner lmuntaner deleted the lm-retry-different-rp-id branch December 18, 2024 10:46
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.

2 participants