[PM-14983] Support Optic ID and any future biometric authentication types #1146
Conversation
# Conflicts: # BitwardenShared/UI/Platform/Application/Support/Localizations/en.lproj/Localizable.strings
|
|
|
Thank you for your contribution! We've added this to our internal Community PR board for review. |
BitwardenShared/UI/Auth/VaultUnlockSetup/VaultUnlockSetupProcessorTests.swift
Outdated
Show resolved
Hide resolved
|
Hi! 👋🏻 It looks like there's a conflict in the strings file; can you please resolve that? |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1146 +/- ##
==========================================
+ Coverage 89.42% 89.46% +0.03%
==========================================
Files 724 740 +16
Lines 45773 46452 +679
==========================================
+ Hits 40933 41558 +625
- Misses 4840 4894 +54 ☔ View full report in Codecov by Sentry. |
|
Great job, no security vulnerabilities found in this Pull Request |
There was a problem hiding this comment.
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.
| case opticID | ||
|
|
||
| /// Unknown other biometric authentication | ||
| case biometrics |
There was a problem hiding this comment.
🎨 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.
There was a problem hiding this comment.
I have renamed the case to unknown.
| @@ -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. | |||
There was a problem hiding this comment.
🎨 We prefer tests to be alphabetized if possible
There was a problem hiding this comment.
I have alphabetized the added tests.
| case .opticID: | ||
| Localizations.unlockWith(Localizations.opticID) | ||
| case .biometrics: | ||
| Localizations.unlockWith(Localizations.biometrics) |
There was a problem hiding this comment.
🎨 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:
There was a problem hiding this comment.
I added a new localization string entry for unknownBiometrics that is lowercased.
| case .opticID: | ||
| Localizations.pinRequireBioOrMasterPasswordRestart(Localizations.opticID) | ||
| case .biometrics: | ||
| Localizations.pinRequireBioOrMasterPasswordRestart(Localizations.biometrics) |
There was a problem hiding this comment.
🎨 This is another case where "Biometrics" being capitalized looks weird.
| @@ -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() { | |||
There was a problem hiding this comment.
🎨 We prefer tests to be in alphabetical order
BitwardenShared/UI/Platform/Settings/Settings/AccountSecurity/AccountSecurityView.swift
Show resolved
Hide resolved
| case .opticID: | ||
| return Localizations.unlockWith(Localizations.opticID) | ||
| case .biometrics: | ||
| return Localizations.unlockWith(Localizations.biometrics) |
There was a problem hiding this comment.
🎨 This is another case where the capitalization gets us
# Conflicts: # BitwardenShared/UI/Platform/Application/Support/Localizations/en.lproj/Localizable.strings
BitwardenShared/Core/Auth/Services/Biometrics/BiometricAuthenticationType.swift
Show resolved
Hide resolved
| return .none | ||
| case .touchID: | ||
| return .touchID | ||
| case .faceID: | ||
| return .faceID | ||
| case .opticID: |
There was a problem hiding this comment.
🎨 We prefer cases in a switch to be alphabetized; we must have missed .faceID being in the wrong place before
| @@ -195,6 +195,10 @@ struct VaultUnlockView: View { | |||
| Text(Localizations.useFaceIDToUnlock) | |||
| case .touchID: | |||
| Text(Localizations.useFingerprintToUnlock) | |||
| case .opticID: | |||
There was a problem hiding this comment.
🎨 We prefer cases in a switch to be alphabetized
| @@ -1059,6 +1059,9 @@ | |||
| "CopyPrivateKey" = "Copy private key"; | |||
| "CopyFingerprint" = "Copy fingerprint"; | |||
| "SSHKeys" = "SSH keys"; | |||
| "OpticID" = "Optic ID"; | |||
| "UseOpticIDToUnlock" = "Use Optic ID To Unlock"; | |||
| "UnknownBiometrics" = "biometrics"; | |||
There was a problem hiding this comment.
🎨 I can't find any other good examples of where we have both an uppercased and lowercased version of a string like this, or where we're doing the casing manually. I feel like it would be better for us to have an UnlockWithBiometrics string, and to use that in VaultUnlockSetupState.UnlockMethod.title specifically for the .unknown case instead of Localizations.unlockWith(...). This would also match how UnlockWithPIN works, I feel.
| XCTAssertEqual(subject.alertActions.count, 2) | ||
| XCTAssertEqual(subject.preferredStyle, .alert) | ||
| XCTAssertEqual(subject.title, Localizations.unlockWithPIN) | ||
| XCTAssertEqual(subject.message, Localizations.pinRequireBioOrMasterPasswordRestart(Localizations.unknownBiometrics)) |
There was a problem hiding this comment.
There was a problem hiding this comment.
Sorry about that! Forgot to run the tests. Should be fixed now.
|
👍🏻 Thank you again for your contribution! This looks good to me, and I'm going to start pushing it along to our QA. |
🎟️ Tracking
📔 Objective
Support Optic ID and any future biometric authentication types.
📸 Screenshots
⏰ 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