[PM-14983] Support Optic ID and any future biometric authentication types #1146

bunnyhero · 2024-11-17T17:46:46Z

🎟️ Tracking

📔 Objective

Support Optic ID and any future biometric authentication types.

📸 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

# Conflicts: # BitwardenShared/UI/Platform/Application/Support/Localizations/en.lproj/Localizable.strings

CLAassistant · 2024-11-17T17:46:52Z

All committers have signed the CLA.

bitwarden-bot · 2024-11-17T17:47:07Z

Thank you for your contribution! We've added this to our internal Community PR board for review.
ID: PM-14983

KatherineInCode

Thank you for your contribution! I've left a quick note about some comments, but will try to give this the fuller attention it deserves when I get a chance.

BitwardenShared/UI/Auth/VaultUnlockSetup/VaultUnlockSetupProcessorTests.swift

KatherineInCode · 2024-12-31T16:05:57Z

Hi! 👋🏻 It looks like there's a conflict in the strings file; can you please resolve that?

codecov · 2024-12-31T16:15:39Z

Codecov Report

Attention: Patch coverage is 58.33333% with 10 lines in your changes missing coverage. Please review.

Project coverage is 89.48%. Comparing base (a495a62) to head (ecbdb5d).
Report is 64 commits behind head on main.

Files with missing lines	Patch %	Lines
...e/Auth/Services/Biometrics/BiometricsService.swift	0.00%	4 Missing ⚠️
.../Auth/VaultUnlockSetup/VaultUnlockSetupState.swift	50.00%	4 Missing ⚠️
...Settings/AccountSecurity/AccountSecurityView.swift	50.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1146      +/-   ##
==========================================
- Coverage   89.49%   89.48%   -0.02%     
==========================================
  Files         688      688              
  Lines       43498    43519      +21     
==========================================
+ Hits        38929    38943      +14     
- Misses       4569     4576       +7

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

github-actions · 2024-12-31T16:25:53Z

Checkmarx One – Scan Summary & Details – 765bba69-2026-4afe-8153-4dd0d59143f9

No New Or Fixed Issues Found

KatherineInCode

Thank you again for your contribution! On the whole this looks good, just a lot of little things to get it in line with our typical code organization.

I'm not sure what the best solution for the capitalization of "biometrics" for the unknown case is, though. I would say it may make more sense to leave it out as a default case, but it looks like that's an error as of Swift 6, which is reasonable. We may need a string specifically for when the generic biometrics is used in a larger string, like this; it's probably worth investigating how we handle this elsewhere in the app, I just don't know it offhand.

KatherineInCode · 2024-12-31T16:41:51Z

BitwardenShared/Core/Auth/Services/Biometrics/BiometricAuthenticationType.swift

+    case opticID
+
+    /// Unknown other biometric authentication
+    case biometrics


🎨 I think this case would be better named unknown, in line with its comment. Based on the type we already know it's biometrics, and it'd be better to be clear that it's a type unknown to us currently. That said, I think keeping it the generic "Use biometrics to unlock" in the strings file is fine.

I have renamed the case to unknown.

KatherineInCode · 2024-12-31T16:52:35Z

BitwardenShared/UI/Auth/VaultUnlockSetup/VaultUnlockSetupProcessorTests.swift

@@ -129,6 +129,30 @@ class VaultUnlockSetupProcessorTests: BitwardenTestCase {
        XCTAssertEqual(subject.state.unlockMethods, [.biometrics(.touchID), .pin])
    }

+    /// `perform(_:)` with `.loadData` fetches the biometrics unlock status for a device with Optic ID.


🎨 We prefer tests to be alphabetized if possible

I have alphabetized the added tests.

KatherineInCode · 2024-12-31T17:10:38Z

BitwardenShared/UI/Auth/VaultUnlockSetup/VaultUnlockSetupState.swift

+                case .opticID:
+                    Localizations.unlockWith(Localizations.opticID)
+                case .biometrics:
+                    Localizations.unlockWith(Localizations.biometrics)


🎨 When this shows up on the Vault Unlock Setup screen, it produces the sentence "Unlock with Biometrics", and I'm not sure that's how we want to capitalize that:

I added a new localization string entry for unknownBiometrics that is lowercased.

KatherineInCode · 2024-12-31T17:22:03Z

BitwardenShared/UI/Platform/Settings/Extensions/Alert+Settings.swift

+        case .opticID:
+            Localizations.pinRequireBioOrMasterPasswordRestart(Localizations.opticID)
+        case .biometrics:
+            Localizations.pinRequireBioOrMasterPasswordRestart(Localizations.biometrics)


🎨 This is another case where "Biometrics" being capitalized looks weird.

KatherineInCode · 2024-12-31T17:22:25Z

BitwardenShared/UI/Platform/Settings/Extensions/AlertSettingsTests.swift

@@ -222,6 +222,28 @@ class AlertSettingsTests: BitwardenTestCase {
        XCTAssertEqual(subject.message, Localizations.pinRequireBioOrMasterPasswordRestart(Localizations.touchID))
    }

+    /// `unlockWithPINCodeAlert(action)` constructs an `Alert` with the correct title, message, Yes and No buttons
+    /// when `biometricType` is `opticID`.
+    func test_unlockWithPINAlert_opticID() {


🎨 We prefer tests to be in alphabetical order

KatherineInCode · 2024-12-31T17:22:43Z

BitwardenShared/UI/Platform/Settings/Extensions/Alert+Settings.swift

@@ -251,6 +251,10 @@ extension Alert {
            Localizations.pinRequireBioOrMasterPasswordRestart(Localizations.faceID)
        case .touchID:
            Localizations.pinRequireBioOrMasterPasswordRestart(Localizations.touchID)
+        case .opticID:


🎨 We prefer cases in a switch statement be alphabetized

KatherineInCode · 2024-12-31T17:23:12Z

BitwardenShared/UI/Auth/VaultUnlockSetup/VaultUnlockSetupState.swift

@@ -43,6 +43,10 @@ struct VaultUnlockSetupState: Equatable {
                    "FaceID"
                case .touchID:
                    "TouchID"
+                case .opticID:


🎨 We prefer cases in a switch be alphabetized

KatherineInCode · 2024-12-31T17:27:44Z

BitwardenShared/UI/Platform/Settings/Settings/AccountSecurity/AccountSecurityView.swift

@@ -270,6 +270,10 @@ struct AccountSecurityView: View {
            return Localizations.unlockWith(Localizations.faceID)
        case .touchID:
            return Localizations.unlockWith(Localizations.touchID)
+        case .opticID:


🎨 We prefer cases in a switch statement to be alphabetized

KatherineInCode · 2024-12-31T17:28:06Z

BitwardenShared/UI/Platform/Settings/Settings/AccountSecurity/AccountSecurityView.swift

+        case .opticID:
+            return Localizations.unlockWith(Localizations.opticID)
+        case .biometrics:
+            return Localizations.unlockWith(Localizations.biometrics)


🎨 This is another case where the capitalization gets us

# Conflicts: # BitwardenShared/UI/Platform/Application/Support/Localizations/en.lproj/Localizable.strings

bunnyhero added 2 commits November 15, 2024 22:44

Add opticID and generic biometrics types

9ae3258

# Conflicts: # BitwardenShared/UI/Platform/Application/Support/Localizations/en.lproj/Localizable.strings

Expand existing biometrics tests

760a778

bunnyhero requested review from matt-livefront, phil-livefront, brant-livefront and a team as code owners November 17, 2024 17:46

bitwarden-bot changed the title ~~Optic~~ [PM-14983] Optic Nov 17, 2024

bitwarden-bot added the community-pr label Nov 17, 2024

bunnyhero changed the title ~~[PM-14983] Optic~~ [PM-14983] Support Optic ID and any future biometric authentication types Nov 17, 2024

KatherineInCode reviewed Nov 18, 2024

View reviewed changes

BitwardenShared/UI/Auth/VaultUnlockSetup/VaultUnlockSetupProcessorTests.swift Outdated Show resolved Hide resolved

Fix comments to match code.

ecbdb5d

KatherineInCode requested changes Dec 31, 2024

View reviewed changes

bunnyhero added 5 commits January 4, 2025 19:20

Merge branch 'main' into optic-id

f6cc63f

# Conflicts: # BitwardenShared/UI/Platform/Application/Support/Localizations/en.lproj/Localizable.strings

rename unknown case to "unknown"

35a8877

Add new string for unknown/generic biometrics

bdb08c9

Alphabetize tests and switch cases

272a37b

Merge branch 'main' into optic-id

260dd2a

bunnyhero requested a review from KatherineInCode January 9, 2025 23:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[PM-14983] Support Optic ID and any future biometric authentication types #1146

[PM-14983] Support Optic ID and any future biometric authentication types #1146

bunnyhero commented Nov 17, 2024

CLAassistant commented Nov 17, 2024 •

edited

Loading

bitwarden-bot commented Nov 17, 2024

KatherineInCode left a comment

KatherineInCode commented Dec 31, 2024

codecov bot commented Dec 31, 2024

github-actions bot commented Dec 31, 2024

KatherineInCode left a comment

KatherineInCode Dec 31, 2024

bunnyhero Jan 9, 2025

KatherineInCode Dec 31, 2024

bunnyhero Jan 9, 2025

KatherineInCode Dec 31, 2024

bunnyhero Jan 9, 2025

KatherineInCode Dec 31, 2024

KatherineInCode Dec 31, 2024

KatherineInCode Dec 31, 2024

KatherineInCode Dec 31, 2024

KatherineInCode Dec 31, 2024

KatherineInCode Dec 31, 2024

[PM-14983] Support Optic ID and any future biometric authentication types #1146

Are you sure you want to change the base?

[PM-14983] Support Optic ID and any future biometric authentication types #1146

Conversation

bunnyhero commented Nov 17, 2024

🎟️ Tracking

📔 Objective

📸 Screenshots

⏰ Reminders before review

🦮 Reviewer guidelines

CLAassistant commented Nov 17, 2024 • edited Loading

bitwarden-bot commented Nov 17, 2024

KatherineInCode left a comment

Choose a reason for hiding this comment

KatherineInCode commented Dec 31, 2024

codecov bot commented Dec 31, 2024

Codecov Report

github-actions bot commented Dec 31, 2024

No New Or Fixed Issues Found

KatherineInCode left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CLAassistant commented Nov 17, 2024 •

edited

Loading