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

[PM-16985] Fix biometrics not working in firefox or windows #12833

Merged
merged 11 commits into from
Jan 14, 2025
Merged

Conversation

quexten
Copy link
Contributor

@quexten quexten commented Jan 13, 2025

🎟️ Tracking

https://bitwarden.atlassian.net/browse/PM-16985

📔 Objective

Fixes biometrics not working in popup extensions. The biometrics service expected the consumer in the UI code to handle the result - the userkey - and set it to state. However, in the extensions, the UI process gets closed when doing UI depending on the browser - if it is in popup mode. In popout things work fine.

For this reason, we also set the userkey, and the biometrics enabled setting as a side-effect in biometrics background, in order to restore functionality. This will most likely be revisited lated, as it is just a hotfix.

📸 Screenshots

Screen.Recording.2025-01-13.at.18.28.48.mov

⏰ Reminders before review

  • Contributor guidelines followed
  • All formatters and local linters executed and passed
  • Written new unit and / or integration tests where applicable
  • Protected functional changes with optionality (feature flags)
  • Used internationalization (i18n) for all UI strings
  • CI builds passed
  • Communicated to DevOps any deployment requirements
  • Updated any necessary documentation (Confluence, contributing docs) or informed the documentation team

🦮 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

Copy link
Contributor

github-actions bot commented Jan 13, 2025

Logo
Checkmarx One – Scan Summary & Detailsfd6c8c50-11b5-4a0d-8222-81dd9507a9e2

New Issues (3)

Checkmarx found the following issues in this Pull Request

Severity Issue Source File / Package Checkmarx Insight
MEDIUM Client_Privacy_Violation /bitwarden_license/bit-web/src/app/tools/access-intelligence/org-at-risk-members-dialog.component.html: 17
detailsMethod org_at_risk_members_dialog_component at line 17 of /bitwarden_license/bit-web/src/app/tools/access-intelligence/org-at-risk-members-dialog.c...
Attack Vector
MEDIUM Client_Privacy_Violation /bitwarden_license/bit-web/src/app/tools/access-intelligence/all-applications.component.html: 91
detailsMethod all_applications_component at line 91 of /bitwarden_license/bit-web/src/app/tools/access-intelligence/all-applications.component.html sends ...
Attack Vector
MEDIUM Client_Privacy_Violation /bitwarden_license/bit-web/src/app/tools/access-intelligence/all-applications.component.html: 96
detailsMethod all_applications_component at line 96 of /bitwarden_license/bit-web/src/app/tools/access-intelligence/all-applications.component.html sends ...
Attack Vector
Fixed Issues (1)

Great job! The following issues were fixed in this Pull Request

Severity Issue Source File / Package
MEDIUM Client_Privacy_Violation /bitwarden_license/bit-web/src/app/tools/access-intelligence/org-at-risk-members-dialog.component.html: 17

@quexten quexten marked this pull request as ready for review January 13, 2025 17:21
@quexten quexten requested review from a team as code owners January 13, 2025 17:21
@quexten quexten requested review from rr-bw January 13, 2025 17:21
@quexten quexten marked this pull request as draft January 13, 2025 17:24
@quexten quexten changed the title Fix biometrics not working in firefox or windows [PM-16985] Fix biometrics not working in firefox or windows Jan 13, 2025
@quexten quexten marked this pull request as ready for review January 13, 2025 17:55
Copy link

codecov bot commented Jan 13, 2025

Codecov Report

Attention: Patch coverage is 0% with 29 lines in your changes missing coverage. Please review.

Project coverage is 34.27%. Comparing base (a54508a) to head (0f44783).
Report is 14 commits behind head on main.

✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
...iometrics/background-browser-biometrics.service.ts 0.00% 20 Missing ⚠️
.../auth/popup/settings/account-security.component.ts 0.00% 7 Missing ⚠️
apps/browser/src/background/main.background.ts 0.00% 1 Missing ⚠️
...gement/biometrics/foreground-browser-biometrics.ts 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #12833      +/-   ##
==========================================
- Coverage   34.28%   34.27%   -0.01%     
==========================================
  Files        2933     2933              
  Lines       90081    90103      +22     
  Branches    16916    16919       +3     
==========================================
  Hits        30886    30886              
- Misses      56737    56759      +22     
  Partials     2458     2458              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@Thomas-Avery Thomas-Avery left a comment

Choose a reason for hiding this comment

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

LGTM

@quexten quexten merged commit 8e95029 into main Jan 14, 2025
35 of 36 checks passed
@quexten quexten deleted the km/fix-bio branch January 14, 2025 15:20
quexten added a commit that referenced this pull request Jan 14, 2025
* Fix biometrics not working in firefox or windows

* Remove logs

* Update badge after biometric unlock

* Add removal todo note

* Remove debug logging

* Fix type warnings

* Fix userkey typing in background biometrics service

* Simplify types for userkey in foreground-browser-biometrics and runtime.background.ts

(cherry picked from commit 8e95029)
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