-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
[PM-16978] Add support for fido2 2fa on mac #12823
base: main
Are you sure you want to change the base?
Conversation
Great job, no security vulnerabilities found in this Pull Request |
Codecov ReportAttention: Patch coverage is
✅ All tests successful. No failed tests found.
Additional details and impacted files@@ Coverage Diff @@
## main #12823 +/- ##
==========================================
- Coverage 34.13% 34.13% -0.01%
==========================================
Files 2936 2936
Lines 90428 90441 +13
Branches 16988 16992 +4
==========================================
+ Hits 30869 30871 +2
- Misses 57102 57112 +10
- Partials 2457 2458 +1 ☔ View full report in Codecov by Sentry. |
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 suspect there are several flows which do trigger prompts. Security Keys can be configured to always perform UV. TouchID might need UV?
It should be clear to the user that they need to change 2fa method as the default tends to be WebAuthn which might not work.
// Temporarily restricted to only Windows until https://github.com/electron/electron/pull/28349 | ||
// has been merged and an updated electron build is available. |
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.
Comment should be updated.
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.
Updated the comment. Also doesn't seem like electron will ever add support here, so I removed the reference to the PR and noted this has to be added in a different way.
18281f2
to
814cee0
Compare
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.
Did we plan on improving the error message for users with touchID? Returning NotAllowedError is suboptimal
Sorry, should have linked that. The notallowederror is only present on the old two-factor component, but I've submitted the fix for that here: #12830; the new component shows: |
@quexten That error is not particularly useful to users with a TouchID 2fa though? They would need to get the error, go to our help page and troubleshoot the problem themselves to identify that the platform doesn't support non security keys? |
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-16978
📔 Objective
Fido2 on electron desktop on mac works, when no pin is required, only the pin is unimplemented. Therefore, we can just enable it for 2fa.
📸 Screenshots
Screen.Recording.2025-01-13.at.13.13.50.mov
⏰ Reminders before review
🦮 Reviewer guidelines
:+1:
) or similar for great changes:memo:
) or ℹ️ (:information_source:
) for notes or general info:question:
) for questions:thinking:
) or 💭 (:thought_balloon:
) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion:art:
) for suggestions / improvements:x:
) or:warning:
) for more significant problems or concerns needing attention:seedling:
) or ♻️ (:recycle:
) for future improvements or indications of technical debt:pick:
) for minor or nitpick changes