clients icon indicating copy to clipboard operation
clients copied to clipboard

[PM-6296] Fix biometrics error prompt when biometrics are temporarily unavailable in browser extension

Open quexten opened this issue 1 year ago â€ĸ 3 comments

đŸŽŸī¸ Tracking

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

📔 Objective

We currently only have two ways of "detecting biometric availability", one is osSupported which is static per OS on browser, and dynamically detects availability on mac. The other way is biometrics failing during unlock.

Since biometrics can become temporarily unavailable (f.e closing the lid on a mac disables touchid), we need a way to dynamically detect availability in the browser extension.

This PR extends the IPC protocol with an availability check message, and the extension checks for availability before triggering the prompt. If this is due to a manual, intentional user action, a warning dialog is shown. If this is due to automatic "touchid on extension open", then no warning is shown.

📸 Screenshots

https://github.com/bitwarden/clients/assets/11866552/650ce95a-8e84-4a24-b588-4d06da3c51e7

⏰ 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

quexten avatar Jun 27 '24 11:06 quexten

@MGibson1 This PR re-uses "osSupportsBiometrics". Do you feel like it would make sense to split this function into one actually returning "os supports" and one returning "is currently available", or rename this into "isBiometricUnlockAvailable", since "osSupports" suggests this is about the operating system, not about whether the device is (temporarily) unavailable?

quexten avatar Jun 27 '24 11:06 quexten

Codecov Report

Attention: Patch coverage is 6.86275% with 95 lines in your changes missing coverage. Please review.

Project coverage is 31.85%. Comparing base (4772943) to head (50a1611). Report is 31 commits behind head on main.

Files Patch % Lines
...latform/services/background-browser-biometrics..ts 0.00% 14 Missing :warning:
apps/browser/src/auth/popup/lock.component.ts 0.00% 10 Missing :warning:
...s/desktop/src/services/native-messaging.service.ts 0.00% 10 Missing :warning:
...platform/services/foreground-browser-biometrics.ts 0.00% 9 Missing :warning:
...rc/platform/services/browser-biometrics.service.ts 0.00% 8 Missing :warning:
...c/platform/services/electron-biometrics.service.ts 0.00% 6 Missing :warning:
libs/angular/src/auth/components/lock.component.ts 0.00% 6 Missing :warning:
...owser/src/background/nativeMessaging.background.ts 0.00% 5 Missing :warning:
.../auth/popup/settings/account-security.component.ts 0.00% 3 Missing :warning:
apps/browser/src/background/runtime.background.ts 0.00% 3 Missing :warning:
... and 13 more
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #9851      +/-   ##
==========================================
- Coverage   31.90%   31.85%   -0.05%     
==========================================
  Files        2644     2667      +23     
  Lines       78889    79329     +440     
  Branches    14793    14828      +35     
==========================================
+ Hits        25169    25272     +103     
- Misses      51809    52147     +338     
+ Partials     1911     1910       -1     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar Jun 27 '24 11:06 codecov[bot]

Logo Checkmarx One – Scan Summary & Details – 8a90b3bc-3118-430a-adbc-9645ba28d434

No New Or Fixed Issues Found

github-actions[bot] avatar Jun 27 '24 12:06 github-actions[bot]

Since biometricsservice was a desktop only concept, I had to create a lot of the plumbing on the platforms respectively, but all biometrics functionality are pulled out of platformService now.

quexten avatar Jul 04 '24 16:07 quexten

Fixed merge conflicts.

quexten avatar Jul 23 '24 13:07 quexten

re-approved, but it looks like there are some failed tests

jprusik avatar Jul 23 '24 15:07 jprusik

Tests failing is unrelated to the PR and is fixed by https://github.com/bitwarden/clients/pull/10215

quexten avatar Jul 23 '24 15:07 quexten

Set to automerge since QA needs to test on main (biometrics in dev mode requires some hacks to set up).

quexten avatar Jul 23 '24 16:07 quexten

Re-enabling auto-merge after rc cut.

quexten avatar Jul 29 '24 13:07 quexten