[PM-8352] Initial Fido2 User verification flow implementation #653

fedemkr · 2024-05-31T14:16:27Z

🎟️ Tracking

PM-8352

📔 Objective

Implement Fido2 User verification flow.

Note: This is an initial PR and the flow will be completed in subsequent PRs to ease

⏰ 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

github-actions · 2024-05-31T14:25:32Z

	2 Warnings
⚠️	Ignoring duplicate libraries: '-lbitwarden_uniffi'
⚠️	BitwardenShared/UI/Auth/Utilities/UserVerificationHelper.swift has less than 80% code coverage

	6 Messages
📖	BitwardenActionExtensionTests: Executed 0 tests, with 0 failures (0 expected) in 0 (0.004) seconds
📖	BitwardenAutoFillExtensionTests: Executed 0 tests, with 0 failures (0 expected) in 0 (0.001) seconds
📖	BitwardenShareExtensionTests: Executed 0 tests, with 0 failures (0 expected) in 0 (0.001) seconds
📖	BitwardenSharedTests: Executed 3219 tests, with 12 failures (0 expected) in 75.208 (76.317) seconds
📖	BitwardenTests: Executed 4 tests, with 0 failures (0 expected) in 0.576 (0.617) seconds
📖	NetworkingTests: Executed 26 tests, with 0 failures (0 expected) in 0.059 (0.067) seconds

Bitwarden code coverage

Total coverage: `86.35%`

File	Coverage
BitwardenShared/Core/Auth/Repositories/AuthRepository.swift	`97.19%`
BitwardenShared/Core/Auth/Services/LocalAuth/LocalAuthService.swift	`100.00%`
BitwardenShared/Core/Platform/Services/ServiceContainer.swift	`97.33%`
BitwardenShared/UI/Auth/Extensions/Alert+Auth.swift	`100.00%`
BitwardenShared/UI/Auth/Utilities/UserVerificationHelper.swift	`79.01%`
BitwardenShared/UI/Auth/Utilities/UserVerificationRunner.swift	`100.00%`
BitwardenShared/UI/Autofill/Utilities/Fido2UserVerificationMediator.swift	`83.56%`
BitwardenShared/UI/Platform/Application/Extensions/Alert+Extensions.swift	`92.31%`
BitwardenShared/UI/Platform/Settings/Extensions/Alert+Settings.swift	`100.00%`
BitwardenShared/UI/Platform/Settings/Settings/AccountSecurity/AccountSecurityProcessor.swift	`98.71%`

Powered by Slather

Generated by 🚫 Danger

github-actions · 2024-05-31T14:28:43Z

Checkmarx One – Scan Summary & Details – 8b2eb545-9a94-47a0-827c-dd490c41fe33

No New Or Fixed Issues Found

KatherineInCode · 2024-06-03T16:47:11Z

BitwardenShared/Core/Auth/Repositories/AuthRepository.swift

+    /// Whether master password reprompt should be performed.
+    ///
+    /// - Parameter reprompt: Cipher reprompt type to check
+    /// - Returns: `True` if master password reprompt should be performed, `False` otherwise.


⛏️ In general we don't capitalize true and false in doc comments

KatherineInCode · 2024-06-03T16:57:27Z

BitwardenShared/Core/Auth/Services/LocalAuth/LocalAuthService.swift

+
+            switch laError.code {
+            case .userCancel:
+                Logger.application.log("User cancel authentication")


⛏️ Maybe "User canceled authentication"?

BitwardenShared/UI/Auth/Utilities/UserVerificationHelper.swift

KatherineInCode · 2024-06-03T17:18:16Z

BitwardenShared/UI/Auth/Utilities/UserVerificationHelper.swift

+    func verifyPin() async throws -> UserVerificationResult
+}
+
+// MARK: DefaultUserVerificationHelper


⛏️ Usually our MARK comments for different top-level options (classes, etc.) are MARK: - to differentiate them from landmarks inside of a class.

KatherineInCode · 2024-06-03T17:19:08Z

BitwardenShared/UI/Auth/Utilities/UserVerificationHelper.swift

+///
+protocol UserVerificationHelper {
+    /// Performs OS local auth, e.g. biometrics or pin/pattern
+    /// - Parameter because: The  reason to be displayed to the user when evaluating the policy if needed


⛏️

Suggested change

/// - Parameter because: The reason to be displayed to the user when evaluating the policy if needed

/// - Parameter because: The reason to be displayed to the user when evaluating the policy if needed

🤔 I also kinda feel like this parameter would better be called reason, to match what we call similar things elsewhere in the app

KatherineInCode · 2024-06-03T17:26:13Z

BitwardenShared/UI/Auth/Utilities/UserVerificationHelperTests.swift

+    /// `verifyDeviceLocalAuth()` with device status not authorized.
+    func test_verifyDeviceLocalAuth_cant_perform_when_not_authorized() async throws {


🎨 I feel like our usual pattern with tests is to make the test name somewhat more terse, generally of the test_function_condition variety, with each unit camelcased—so test_verifyDeviceLocalAuth_notAuthorized here—and to put the fuller explanation of the Given/When/Then in the comment, since that's easier to read.

KatherineInCode · 2024-06-03T17:30:51Z

BitwardenShared/UI/Auth/Utilities/UserVerificationRunner.swift

+    func verifyWithAttempts(
+        verifyFunction: () async throws -> UserVerificationResult
+    ) async throws -> UserVerificationResult {
+        var attempts: Int8 = 0


❓ Why Int8 instead of Int for just counting? The Swift language guide indicates that we should always use Int unless there's a good reason to do otherwise.

I just thought that given it counts "attempts", one would never use hundreds or thousands of attempts so Int8 (maximum 127) made sense. Maybe I could even have used UInt8 to indicate that attempts shouldn't be negative. I can make it Int if performance is better or something like that. IMO even though is negligible for performance I often try to use the necessary data type instead of the broader one.

It's a good instinct, and one that I had coming from Objective-C, but Swift really pushes to use the base types (Int, Double, etc.) unless absolutely necessary. I assume there's some sort of compiler optimization that happens around those base types, and I've also occasionally run into issues doing math when I had competing types (though that was also a Very Long Time Ago so it might not be an issue now). It's relatively moot here, however.

Given that in this case it's pretty much the same and the performance gain is negligible, I'll just change it to avoid confusions.

KatherineInCode · 2024-06-03T17:32:58Z

BitwardenShared/UI/Auth/Utilities/UserVerificationHelper.swift

+/// An enum with the possible results when verifying a user
+///
+enum UserVerificationResult {
+    case cantPerform


🎨 Using a contraction here feels weird to me. I think a more idiomatic way would be unableToPerform

BitwardenShared/UI/Autofill/Utilities/Fido2UserVerificationMediator.swift

Co-authored-by: Katherine Bertelsen <[email protected]>

…iator.swift Co-authored-by: Katherine Bertelsen <[email protected]>

…ing.

…nfusions.

KatherineInCode · 2024-06-04T18:34:08Z

BitwardenShared/Core/Auth/Services/LocalAuth/LocalAuthServiceTests.swift

+        XCTAssertEqual(result, .passcodeNotSet)
+    }
+
+    /// `evaluateDeviceOwnerPolicy(suppliedContext:,deviceAuthStatus:,localizedReason:)`


⛏️ Generally when giving a Swift function that takes multiple parameters, just the colons are used: evaluateDeviceOwnerPolicy(suppliedContext:deviceAuthStatus:localizedReason:). It's a little weird, but it's a holdover from the Objective-C days (when there weren't commas between function arguments like that).

Tip: you can get the symbol name by right clicking the function > Copy > Copy Symbol Name. Or ^⇧⌘C is the default shortcut.

BitwardenShared/UI/Auth/Extensions/Alert+Auth.swift

BitwardenShared/Core/Auth/Repositories/AuthRepository.swift

BitwardenShared/Core/Auth/Services/LocalAuth/LocalAuthService.swift

BitwardenShared/UI/Auth/Extensions/Alert+Auth.swift

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

GlobalTestHelpers/Support/BitwardenTestCase.swift

Co-authored-by: Matt Czech <[email protected]>

…g, updated tests symbol names

matt-livefront

Looks good to me!

…d logic for checking if can verify master password.

matt-livefront

👍

PM-8352 Started Fido2 User verification flow implementation

efd9737

fedemkr requested review from a team and matt-livefront May 31, 2024 14:16

PM-8352 Added missing unit tests for AuthRepository

cbcba98

KatherineInCode reviewed Jun 3, 2024

View reviewed changes

fedemkr and others added 4 commits June 3, 2024 16:14

Update BitwardenShared/UI/Auth/Utilities/UserVerificationHelper.swift

8093425

Co-authored-by: Katherine Bertelsen <[email protected]>

Update BitwardenShared/UI/Autofill/Utilities/Fido2UserVerificationMed…

8af9023

…iator.swift Co-authored-by: Katherine Bertelsen <[email protected]>

PM-8352 Address PR feedback: Marks, naming, capitalization, tests nam…

dde6ea5

…ing.

PM-8352 Addressed PR feedback: removed specific Int8 type to avoid co…

8ed21d8

…nfusions.

fedemkr requested a review from KatherineInCode June 3, 2024 20:17

Merge branch 'main' into mobiletf/pm-8352/fido2-user-verification-flow

c549d4e

KatherineInCode reviewed Jun 4, 2024

View reviewed changes

matt-livefront reviewed Jun 4, 2024

View reviewed changes

fedemkr and others added 3 commits June 4, 2024 17:35

Update BitwardenShared/UI/Auth/Extensions/Alert+Auth.swift

bfff433

Co-authored-by: Matt Czech <[email protected]>

Merge branch 'main' into mobiletf/pm-8352/fido2-user-verification-flow

d43f442

PM-8352 Addressed PR feedback: completion closure order to be trailin…

cf18285

…g, updated tests symbol names

fedemkr requested review from matt-livefront and KatherineInCode June 4, 2024 21:42

matt-livefront previously approved these changes Jun 5, 2024

View reviewed changes

fedemkr added 3 commits June 11, 2024 08:31

Merge branch 'main' into mobiletf/pm-8352/fido2-user-verification-flow

f2b2bdf

PM-8352 Updated to use SDK objects for user verification. Also update…

b3ab097

…d logic for checking if can verify master password.

Merge branch 'main' into mobiletf/pm-8352/fido2-user-verification-flow

e9138e4

fedemkr dismissed matt-livefront’s stale review via e9138e4 June 11, 2024 16:50

fedemkr requested a review from matt-livefront June 11, 2024 16:50

matt-livefront approved these changes Jun 12, 2024

View reviewed changes

Merge branch 'main' into mobiletf/pm-8352/fido2-user-verification-flow

67e12f6

fedemkr merged commit c0de781 into main Jun 18, 2024
5 checks passed

fedemkr deleted the mobiletf/pm-8352/fido2-user-verification-flow branch June 18, 2024 18:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[PM-8352] Initial Fido2 User verification flow implementation #653

[PM-8352] Initial Fido2 User verification flow implementation #653

fedemkr commented May 31, 2024 •

edited by jira bot

Loading

github-actions bot commented May 31, 2024 •

edited

Loading

github-actions bot commented May 31, 2024 •

edited

Loading

KatherineInCode Jun 3, 2024

KatherineInCode Jun 3, 2024

KatherineInCode Jun 3, 2024

KatherineInCode Jun 3, 2024

KatherineInCode Jun 3, 2024

KatherineInCode Jun 3, 2024

KatherineInCode Jun 3, 2024

fedemkr Jun 3, 2024

KatherineInCode Jun 3, 2024

fedemkr Jun 3, 2024

KatherineInCode Jun 3, 2024

KatherineInCode Jun 4, 2024

matt-livefront Jun 4, 2024

matt-livefront left a comment

matt-livefront left a comment

	/// - Parameter because: The reason to be displayed to the user when evaluating the policy if needed
	/// - Parameter because: The reason to be displayed to the user when evaluating the policy if needed

		/// `verifyDeviceLocalAuth()` with device status not authorized.
		func test_verifyDeviceLocalAuth_cant_perform_when_not_authorized() async throws {

[PM-8352] Initial Fido2 User verification flow implementation #653

[PM-8352] Initial Fido2 User verification flow implementation #653

Conversation

fedemkr commented May 31, 2024 • edited by jira bot Loading

🎟️ Tracking

📔 Objective

⏰ Reminders before review

🦮 Reviewer guidelines

github-actions bot commented May 31, 2024 • edited Loading

Bitwarden code coverage

Total coverage: 86.35%

github-actions bot commented May 31, 2024 • edited Loading

No New Or Fixed Issues Found

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

matt-livefront left a comment

Choose a reason for hiding this comment

matt-livefront left a comment

Choose a reason for hiding this comment

fedemkr commented May 31, 2024 •

edited by jira bot

Loading

github-actions bot commented May 31, 2024 •

edited

Loading

Total coverage: `86.35%`

github-actions bot commented May 31, 2024 •

edited

Loading