BIT-2346: Add inline autofill #645

matt-livefront · 2024-05-29T20:24:47Z

🎟️ Tracking

BIT-2346

📔 Objective

Implements inline autofill so that vault items are exposed as autofill suggestions on the keyboard.

📸 Screenshots

RPReplay_Final1717000513.MP4

⏰ 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-29T20:33:36Z

	4 Warnings
⚠️	Ignoring duplicate libraries: '-lbitwarden_uniffi'
⚠️	BitwardenAutoFillExtension/CredentialProviderViewController.swift has less than 80% code coverage
⚠️	BitwardenShareExtension/ShareViewController.swift has less than 80% code coverage
⚠️	BitwardenShared/UI/Platform/Application/AppExtensionDelegate.swift has less than 80% code coverage

	6 Messages
📖	BitwardenActionExtensionTests: Executed 0 tests, with 0 failures (0 expected) in 0 (0.0) seconds
📖	BitwardenAutoFillExtensionTests: Executed 0 tests, with 0 failures (0 expected) in 0 (0.0) seconds
📖	BitwardenShareExtensionTests: Executed 0 tests, with 0 failures (0 expected) in 0 (0.001) seconds
📖	BitwardenSharedTests: Executed 3150 tests, with 12 failures (0 expected) in 57.586 (59.192) seconds
📖	BitwardenTests: Executed 4 tests, with 0 failures (0 expected) in 0.392 (0.422) seconds
📖	NetworkingTests: Executed 26 tests, with 0 failures (0 expected) in 0.038 (0.045) seconds

Bitwarden code coverage

Total coverage: `86.33%`

File	Coverage
BitwardenAutoFillExtension/CredentialProviderViewController.swift	`0.00%`
BitwardenShareExtension/ShareViewController.swift	`0.00%`
BitwardenShared/Core/Autofill/Services/AutofillCredentialService.swift	`95.73%`
BitwardenShared/Core/Platform/Services/ServiceContainer.swift	`97.30%`
BitwardenShared/Core/Platform/Services/StateService.swift	`97.46%`
BitwardenShared/Core/Tools/Repositories/SendRepository.swift	`86.79%`
BitwardenShared/Core/Vault/Repositories/VaultRepository.swift	`96.28%`
BitwardenShared/Core/Vault/Services/VaultTimeoutService.swift	`100.00%`
BitwardenShared/UI/Platform/Application/AppCoordinator.swift	`82.35%`
BitwardenShared/UI/Platform/Application/AppExtensionDelegate.swift	`0.00%`
BitwardenShared/UI/Platform/Application/AppProcessor.swift	`83.69%`

Powered by Slather

Generated by 🚫 Danger

matt-livefront · 2024-05-29T20:40:33Z

.swiftformat

--varattributes same-line
+--varattributes preserve


This rule was conflicting with SwiftLint, but I think SwiftLint has better handing for this since it allows certain attributes (e.g. @Environment) to be on the same line and others to be on the previous line.

SwiftFormat wanted to change this:

Before After

@available(iOS 17, *)
var credentialIdentity: (any ASCredentialIdentity)? {
passwordCredentialIdentity
}

@available(iOS 17, *) var credentialIdentity: (any ASCredentialIdentity)? {
passwordCredentialIdentity
}

bitwarden-bot · 2024-05-29T20:40:45Z

Checkmarx One – Scan Summary & Details – 24a3ef10-cbfe-40e7-b002-b7bbdc4f5b27

No New Or Fixed Issues Found

fedemkr

🎉 Awesome job! Some improvements I saw and perhaps it's being done elsewhere but is it possible that incremental updates for the ASCredentialIdentityStore is missing? I.e. when a cipher is added/modified/removed then it should update the Store.

MAUI sources:

fedemkr · 2024-05-30T16:06:51Z

BitwardenShared/Core/Autofill/Services/AutofillCredentialService.swift

+        let disableAutoTotpCopy = try await stateService.getDisableAutoTotpCopy()
+        if !disableAutoTotpCopy,
+           let totp = cipher.login?.totp {
+            let codeModel = try await clientService.vault().generateTOTPCode(for: totp, date: nil)
+            pasteboardService.copy(codeModel.code)
+        }


⚠️ This should check if TOTP is enabled at an organizational level or if the user has premium access. MAUI Code here

fedemkr · 2024-05-30T17:25:54Z

BitwardenShared/Core/Vault/Repositories/VaultRepository.swift

@@ -1024,6 +1031,12 @@ extension DefaultVaultRepository: VaultRepository {
        await vaultTimeoutService.remove(userId: userId)
    }

+    func repromptRequiredForCipher(id: String) async throws -> Bool {
+        guard try await stateService.getUserHasMasterPassword() else { return false }


⚠️ This should check if the user has master password hash, because the user might have a master password set up but may not have logged in with master password and used Trust Device Encryption. With the hash we make sure we are able to verify the master password.
🤔 I think there's an issue for this, so not sure if the idea is to keep using stateService.getUserHasMasterPassword and then when the issue is grabbed change all occurrences to use the hash but making a comment just in case.

The SDK has a method validatePasswordUserKey to validate the user's password using the user's key without the master password hash. We use that if the master password hash doesn't exist. I believe that was added for this use case. Wouldn't that work here if the user has a master password but no master password hash?

ios/BitwardenShared/Core/Auth/Repositories/AuthRepository.swift

Lines 717 to 728 in 6904487

func validatePassword(_ password: String) async throws -> Bool {

if let passwordHash = try await stateService.getMasterPasswordHash() {

return try await clientService.auth().validatePassword(password: password, passwordHash: passwordHash)

} else {

let encryptionKeys = try await stateService.getAccountEncryptionKeys()

guard let encUserKey = encryptionKeys.encryptedUserKey else { throw StateServiceError.noEncUserKey }

do {

let passwordHash = try await clientService.auth().validatePasswordUserKey(

password: password,

encryptedUserKey: encUserKey

)

try await stateService.setMasterPasswordHash(passwordHash)

🤔 AFAIK it wouldn't work given that you don't have the hash to compare against the password string the user is entering in the prompt and/or the UserKey may not be based on the password thus you can't compare it. However, I'll ask the SDK team to see if there's something I don't know and that's a valid use case.

Talked with the SDK team and they said it should work as you describe but we'd need to test it in order to ensure that's the case. So let's keep it like you did it and if then doesn't work we'll change it. Thank you Matt for the feedback!!

fedemkr · 2024-05-30T17:28:34Z

BitwardenShared/Core/Vault/Repositories/VaultRepositoryTests.swift

@@ -1196,6 +1196,32 @@ class VaultRepositoryTests: BitwardenTestCase { // swiftlint:disable:this type_b
        }
    }

+    /// `repromptRequiredForCipher(id:)` returns `true` if reprompt is required for a cipher.


🎨 Could a test be added for the case cipher is nil

fedemkr · 2024-05-30T18:51:41Z

BitwardenShared/UI/Platform/Application/AppProcessorTests.swift

+
+    /// `repromptForCredentialIfNecessary(for:)` reprompts the user for their master password if
+    /// reprompt is enabled for the cipher.
+    func test_repromptForCredentialIfNecessary() throws {


⛏️ Could a test be added to take into account an error thrown that is logged by the error reporter?

matt-livefront · 2024-06-03T20:50:11Z

Some improvements I saw and perhaps it's being done elsewhere but is it possible that incremental updates for the ASCredentialIdentityStore is missing? I.e. when a cipher is added/modified/removed then it should update the Store.

Adding/modifying/removing a cipher does update the store with the current approach. That's handled by the cipher's publisher. It's not using incremental updates though, I wasn't sure how critical that was and this was already getting pretty large. We could consider that as a future enhancement.

fedemkr

Great! I agree incremental store can be done in a separate PR as an improvement.
Just noticed some small things

fedemkr · 2024-06-03T21:25:07Z

BitwardenShared/Core/Autofill/Services/AutofillCredentialServiceTests.swift

+    }
+
+    /// `provideCredential(for:)` doesn't copy the cipher's TOTP code if the user doesn't have premium access.
+    func test_provideCredential_totpCopyNotPremium() async throws {


⛏️ I think a test should be added when active account doesn't have premium but cipher.organizationUseTotp is true

fedemkr · 2024-06-03T21:27:57Z

BitwardenShared/Core/Platform/Services/StateServiceTests.swift

+    func test_doesActiveAccountHavePremium_personalFalse_organizationTrue() async throws {
+        await subject.addAccount(.fixture(profile: .fixture(hasPremiumPersonally: false)))
+        try await dataStore.replaceOrganizations([.fixture(usersGetPremium: true)], userId: "1")
+        let organizations = try await dataStore.fetchAllOrganizations(userId: "1")


🤔 Can this line be removed?

fedemkr · 2024-06-03T21:29:32Z

BitwardenShared/Core/Platform/Services/StateServiceTests.swift

+
+    /// `doesActiveAccountHavePremium()` with no premium personally and an organization with premium
+    /// but disabled returns false.
+    func test_doesActiveAccountHavePremium_personalFalse_organizationTrueDisabled() async throws {


🤔 Maybe I'm missing them but shouldn't tests be added to test the case where there are organizations but no one matches the current userId?

fedemkr

Approving in case this needs to be merged for the beta release and we can improve the tests later.

fedemkr

LGTM 🎉

BIT-2346: Add inline autofill

bf33160

matt-livefront requested review from fedemkr, jubie-livefront and KatherineInCode May 29, 2024 20:24

matt-livefront commented May 29, 2024

View reviewed changes

fedemkr reviewed May 30, 2024

View reviewed changes

matt-livefront added 2 commits June 3, 2024 14:32

BIT-2346: Add additional tests

7e114a0

BIT-2346: TOTP copy premium check

90bf07f

Merge branch 'main' into matt/BIT-2346-autofill

67ccf6c

matt-livefront requested a review from fedemkr June 3, 2024 21:10

fedemkr reviewed Jun 3, 2024

View reviewed changes

fedemkr previously approved these changes Jun 4, 2024

View reviewed changes

BIT-2346: Additional tests and feedback

60beb7d

matt-livefront dismissed fedemkr’s stale review via 60beb7d June 4, 2024 13:58

matt-livefront requested a review from fedemkr June 4, 2024 14:00

fedemkr approved these changes Jun 4, 2024

View reviewed changes

matt-livefront merged commit 7710cbb into main Jun 4, 2024
5 checks passed

matt-livefront deleted the matt/BIT-2346-autofill branch June 4, 2024 14:29

matt-livefront mentioned this pull request Jun 4, 2024

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

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BIT-2346: Add inline autofill #645

BIT-2346: Add inline autofill #645

matt-livefront commented May 29, 2024

github-actions bot commented May 29, 2024 •

edited

Loading

matt-livefront May 29, 2024

bitwarden-bot commented May 29, 2024 •

edited by github-actions bot

Loading

fedemkr left a comment

fedemkr May 30, 2024

fedemkr May 30, 2024

matt-livefront Jun 3, 2024

fedemkr Jun 3, 2024 •

edited

Loading

fedemkr Jun 3, 2024

fedemkr May 30, 2024

fedemkr May 30, 2024

matt-livefront commented Jun 3, 2024

fedemkr left a comment

fedemkr Jun 3, 2024

fedemkr Jun 3, 2024

fedemkr Jun 3, 2024

fedemkr left a comment

fedemkr left a comment

	func validatePassword(_ password: String) async throws -> Bool {
	if let passwordHash = try await stateService.getMasterPasswordHash() {
	return try await clientService.auth().validatePassword(password: password, passwordHash: passwordHash)
	} else {
	let encryptionKeys = try await stateService.getAccountEncryptionKeys()
	guard let encUserKey = encryptionKeys.encryptedUserKey else { throw StateServiceError.noEncUserKey }
	do {
	let passwordHash = try await clientService.auth().validatePasswordUserKey(
	password: password,
	encryptedUserKey: encUserKey
	)
	try await stateService.setMasterPasswordHash(passwordHash)

BIT-2346: Add inline autofill #645

BIT-2346: Add inline autofill #645

Conversation

matt-livefront commented May 29, 2024

🎟️ Tracking

📔 Objective

📸 Screenshots

⏰ Reminders before review

🦮 Reviewer guidelines

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

Bitwarden code coverage

Total coverage: 86.33%

Choose a reason for hiding this comment

bitwarden-bot commented May 29, 2024 • edited by github-actions bot Loading

No New Or Fixed Issues Found

fedemkr 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

fedemkr Jun 3, 2024 • edited Loading

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 commented Jun 3, 2024

fedemkr 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

fedemkr left a comment

Choose a reason for hiding this comment

fedemkr left a comment

Choose a reason for hiding this comment

github-actions bot commented May 29, 2024 •

edited

Loading

Total coverage: `86.33%`

bitwarden-bot commented May 29, 2024 •

edited by github-actions bot

Loading

fedemkr Jun 3, 2024 •

edited

Loading