-
Notifications
You must be signed in to change notification settings - Fork 48
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
feat: use method selector in 2fa login #360
base: main
Are you sure you want to change the base?
feat: use method selector in 2fa login #360
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #360 +/- ##
==========================================
+ Coverage 42.43% 50.61% +8.18%
==========================================
Files 136 138 +2
Lines 2008 2114 +106
Branches 288 316 +28
==========================================
+ Hits 852 1070 +218
+ Misses 1149 1032 -117
- Partials 7 12 +5
|
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.
When code is configured for MFA, the method selector seems to be missing the identifier
payload param, leading to this behavior:
Screen.Recording.2025-02-19.at.10.17.54.AM.mov
"two-step.webauthn.title": "Security Key", | ||
"two-step.webauthn.description": "Use your security key to authenticate", | ||
"two-step.webauthn.title": "Use a hardware key", | ||
"two-step.webauthn.description": "Use the signature of one of your cryptographic hardware keys", |
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 bit too technical. The previous version might be more accurate. This came from figma?
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.
Yes I took it from figma. But I do agree, I think the previous version is better in terms of clarity. I can change it back.
![]() In this case, I think we need to show an error. You might be able to adjust this: https://github.com/ory/elements/blob/master/packages/elements-react/src/components/form/form.tsx#L238-L245 |
Co-authored-by: Jonas Hungershausen <[email protected]>
I think this happens because the This is working in the current AX because Your point about the missing The issue is that not all 2fa methods include the Should I create pull requests in Kratos to address these issues? |
So we would need to take the
I think the identifier button is also not doing what it seems. In the 1fa flows, it allows the user to change their input email address (identifier), but here it just allows changing the method. So IMO, we should actually hide it for now, and implement proper navigation on the 2fa screen in a separate PR. Can we easily hide the identifier button in 2fa flows (should be possible by using the That would require logging the user out and initializing a new flow. And since not all methods return the identifier node, we might need to adjust the API response, or change that button to instead be a text field at the bottom like "Not you? Logout". |
Related Issue or Design Document
Checklist
If this pull request addresses a security vulnerability,
I confirm that I got approval (please contact [email protected]) from the maintainers to push the changes.
Further comments
Multiple 2fa methods
Single 2fa method