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-10741] Refactor biometrics interface & add dynamic status #10973

Draft
wants to merge 22 commits into
base: km/pm-9823/extract-biometric-messaging-service
Choose a base branch
from

Conversation

quexten
Copy link
Contributor

@quexten quexten commented Sep 10, 2024

🎟️ Tracking

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

📔 Objective

Refactor biometrics both in browser and desktop.
This has a few goals, mainly to improve readability, maintainability; since this is a fairly large PR here is a breakdown:

New biometrics service interface

We introduce a new interface for biometrics:

import { UserId } from "@bitwarden/common/types/guid";
import { UserKey } from "@bitwarden/common/types/key";

import { BiometricsStatus } from "./biometrics-status";

/**
 * The biometrics service is used to provide access to the status of and access to biometric functionality on the platforms.
 */
export abstract class BiometricsService {
  abstract authenticateWithBiometrics(): Promise<boolean>;
  abstract getBiometricsStatus(): Promise<BiometricsStatus>;
  abstract unlockWithBiometricsForUser(userId: UserId): Promise<UserKey>;
  abstract getBiometricsStatusForUser(userId: UserId): Promise<BiometricsStatus>;
}

implemented both on desktop and browser. Previously, the user-specific biometrics would be done via the keytar with a special biometric key suffix. This was intuitive. Further, the api for this set the userkey to state as a side-effect. In the new biometrics interface, the user is responsible for setting the userkey, when calling "unlockWithBiometricsForUser". Additionally, this greatly simplifies the keytar code, which previously had many special case exceptions to handle the biometrics related codepaths.

Consumers of the biometrics API no longer need to care about multiple different ways of getting the status or platform support of biometrics. They can query either the general biometrics status, or the status for one specific user (which will include responses such as "unlock is needed" when masterpassword reprompt is used).

Lock screen availability check

Since the biometrics state is not much more easily accessible both on desktop and browser, this PR adds a dynamic availability check to the lock components, which will show the reason why biometrics are unavailable, instead of just hiding the option.

TODO SCREENSHOTS

Moved os biometrics services

The naming for the biometrics services has been clarified to "os-biometrics-{windows/mac/linux}". The os-biometrics* implementation is the lowest layer, and will be later moved to rust.

Consistent naming of ipc methods

All ipc methods follow the same naming of the methods of the biometrics interfaces they implement, message commands are also renamed to follow the same naming scheme, improving readability.

Client keyhalf by userid instead of service + key

The client keyhalf was previously set using a "service" and "key", which was added complexity. It is just a key for each user, and so we store it by userId. In a later refactor, this concept will be moved to the windows implementation entirely, since it is only used there.

Refactored IPC

IPC between desktop and browser has been refactored, both to break out the biometric specific code into other files, but also in order to support concurrent requsests. Previously a single message handler callback would be registered, and if another message was sent before the first response was received, the handler would be overwritten and no response would be received. This PR introduces a map of callbacks and message ids in order to support concurrent requests (f.e for checking availability concurrently to unlocking).

Backward compatibility:

Note: This changes Browser<->desktop IPC, which has backward compatibility implications. The PR handles both directions (browser being older than desktop, and desktop being older than browser). The remaining messages will be removed in 3 releases.

📸 Screenshots

⏰ 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 Sep 10, 2024

Logo
Checkmarx One – Scan Summary & Details3b91d9be-a203-4880-91ae-57addc4be9af

Fixed Issues

Severity Issue Source File / Package
HIGH Client_DOM_XSS /apps/web/src/connectors/redirect.ts: 15
HIGH Client_DOM_XSS /apps/web/src/connectors/redirect.ts: 6
MEDIUM Angular_Improper_Type_Pipe_Usage /apps/browser/src/autofill/popup/fido2/fido2-use-browser-link-v1.component.html: 1
LOW Client_DOM_Open_Redirect /apps/web/src/connectors/redirect.ts: 15
LOW Client_DOM_Open_Redirect /apps/web/src/connectors/redirect.ts: 6

@quexten quexten changed the title Km/pm 10741/refactor biometrics [PM-10741] Refactor biometrics interface & add dynamic status Sep 11, 2024
@quexten quexten force-pushed the km/pm-10741/refactor-biometrics branch from 2e0002a to 62f65aa Compare September 15, 2024 12:37
Copy link

codecov bot commented Sep 16, 2024

Codecov Report

Attention: Patch coverage is 12.53071% with 356 lines in your changes missing coverage. Please review.

Project coverage is 35.05%. Comparing base (1a9dfbc) to head (9703970).
Report is 101 commits behind head on km/pm-9823/extract-biometric-messaging-service.

✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
...owser/src/background/nativeMessaging.background.ts 0.00% 61 Missing ⚠️
...iometrics/background-browser-biometrics.service.ts 0.00% 45 Missing ⚠️
...src/services/biometric-native-messaging.service.ts 0.00% 41 Missing ⚠️
...y-management/biometrics/main-biometrics.service.ts 43.63% 31 Missing ⚠️
libs/angular/src/auth/components/lock.component.ts 0.00% 26 Missing ⚠️
...agement/biometrics/main-biometrics-ipc.listener.ts 0.00% 22 Missing ⚠️
...pps/desktop/src/app/accounts/settings.component.ts 0.00% 15 Missing ⚠️
...gement/biometrics/foreground-browser-biometrics.ts 0.00% 13 Missing ⚠️
...nagement/biometrics/renderer-biometrics.service.ts 0.00% 13 Missing ⚠️
...p/src/platform/services/electron-crypto.service.ts 0.00% 12 Missing ⚠️
... and 16 more
Additional details and impacted files
@@                                Coverage Diff                                 @@
##           km/pm-9823/extract-biometric-messaging-service   #10973      +/-   ##
==================================================================================
+ Coverage                                           33.17%   35.05%   +1.87%     
==================================================================================
  Files                                                2691     2692       +1     
  Lines                                               83005    83777     +772     
  Branches                                            15761    15894     +133     
==================================================================================
+ Hits                                                27537    29367    +1830     
- Misses                                              53301    53443     +142     
+ Partials                                             2167      967    -1200     

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

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.

1 participant