Skip to content

[PM-28855] Update credential identities store on cipher changes on iOS extensions #2169

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.

Sign up for GitHub

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
+689 −2
Open

[PM-28855] Update credential identities store on cipher changes on iOS extensions #2169

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7349f76
PM-28855 Added cipher change subscriber to update the credential iden…
fedemkr Nov 28, 2025
c310353
Merge branch 'main' into PM-28855/update-asstore-on-passkey-registration
fedemkr Nov 28, 2025
a7e0017
PM-28855 Fix typo
fedemkr Dec 1, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
184 changes: 183 additions & 1 deletion BitwardenShared/Core/Autofill/Services/AutofillCredentialService+AppExtensionTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ import XCTest
/// initialization to see if the subscription to the `VaultTimeoutService` is necessary. So it's easier to test
/// having a new class test specifically for it.
@MainActor
class AutofillCredentialServiceAppExtensionTests: BitwardenTestCase {
class AutofillCredentialServiceAppExtensionTests: BitwardenTestCase { // swiftlint:disable:this type_body_length
// MARK: Properties

var appContextHelper: MockAppContextHelper!
Expand Down Expand Up @@ -101,6 +101,188 @@ class AutofillCredentialServiceAppExtensionTests: BitwardenTestCase {

// MARK: Tests

/// `subscribeToCipherChanges()` inserts credentials in the store when a cipher is inserted.
func test_subscribeToCipherChanges_insert() async throws {
prepareDataForIdentitiesReplacement()
stateService.activeAccount = .fixture(profile: .fixture(userId: "1"))

try await waitForAsync { [weak self] in
guard let self else { return false }
return subject.hasCipherChangesSubscription
}

// Send an inserted cipher
cipherService.cipherChangesSubject.send(
.inserted(.fixture(
id: "1",
login: .fixture(
password: "password123",
uris: [.fixture(uri: "bitwarden.com")],
username: "[email protected]",
),
)),
)

try await waitForAsync { [weak self] in
guard let self else { return false }
return identityStore.saveCredentialIdentitiesCalled
}

XCTAssertTrue(identityStore.saveCredentialIdentitiesCalled)
XCTAssertEqual(
identityStore.saveCredentialIdentitiesIdentities,
[
.password(PasswordCredentialIdentity(id: "1", uri: "bitwarden.com", username: "[email protected]")),
],
)
}

/// `subscribeToCipherChanges()` updates credentials in the store when a cipher is updated.
func test_subscribeToCipherChanges_update() async throws {
prepareDataForIdentitiesReplacement()
stateService.activeAccount = .fixture(profile: .fixture(userId: "1"))

try await waitForAsync { [weak self] in
guard let self else { return false }
return subject.hasCipherChangesSubscription
}
credentialIdentityFactory.createCredentialIdentitiesMocker
.withResult { cipher in
if cipher.id == "3" {
[
.password(
PasswordCredentialIdentity(
id: "3",
uri: "example.com",
username: "[email protected]",
),
),
]
} else {
[]
}
}

// Send an updated cipher
cipherService.cipherChangesSubject.send(
.updated(.fixture(
id: "3",
login: .fixture(
password: "newpassword",
uris: [.fixture(uri: "example.com")],
username: "[email protected]",
),
)),
)

try await waitForAsync { [weak self] in
guard let self else { return false }
return identityStore.saveCredentialIdentitiesCalled
}

XCTAssertTrue(identityStore.saveCredentialIdentitiesCalled)
XCTAssertEqual(
identityStore.saveCredentialIdentitiesIdentities,
[
.password(PasswordCredentialIdentity(id: "3", uri: "example.com", username: "[email protected]")),
],
)
}

/// `subscribeToCipherChanges()` removes credentials from the store when a cipher is deleted.
func test_subscribeToCipherChanges_delete() async throws {
prepareDataForIdentitiesReplacement()
stateService.activeAccount = .fixture(profile: .fixture(userId: "1"))

try await waitForAsync { [weak self] in
guard let self else { return false }
return subject.hasCipherChangesSubscription
}

// Send a deleted cipher
cipherService.cipherChangesSubject.send(
.deleted(.fixture(
id: "1",
login: .fixture(
password: "password123",
uris: [.fixture(uri: "bitwarden.com")],
username: "[email protected]",
),
)),
)

try await waitForAsync { [weak self] in
guard let self else { return false }
return identityStore.removeCredentialIdentitiesCalled
}

XCTAssertTrue(identityStore.removeCredentialIdentitiesCalled)
XCTAssertEqual(
identityStore.removeCredentialIdentitiesIdentities,
[
.password(PasswordCredentialIdentity(id: "1", uri: "bitwarden.com", username: "[email protected]")),
],
)
}

/// `subscribeToCipherChanges()` does not update the store when identity store is disabled.
func test_subscribeToCipherChanges_storeDisabled() async throws {
prepareDataForIdentitiesReplacement()
stateService.activeAccount = .fixture(profile: .fixture(userId: "1"))
identityStore.state.mockIsEnabled = false

try await waitForAsync { [weak self] in
guard let self else { return false }
return subject.hasCipherChangesSubscription
}

// Send an inserted cipher
cipherService.cipherChangesSubject.send(
.inserted(.fixture(
id: "1",
login: .fixture(
password: "password123",
uris: [.fixture(uri: "bitwarden.com")],
username: "[email protected]",
),
)),
)

// Wait a bit to ensure no changes are processed
try await Task.sleep(nanoseconds: 100_000_000) // 0.1 seconds

XCTAssertFalse(identityStore.saveCredentialIdentitiesCalled)
}

/// `subscribeToCipherChanges()` does not update the store when incremental updates are not supported.
func test_subscribeToCipherChanges_incrementalUpdatesNotSupported() async throws {
prepareDataForIdentitiesReplacement()
stateService.activeAccount = .fixture(profile: .fixture(userId: "1"))
identityStore.state.mockSupportsIncrementalUpdates = false

try await waitForAsync { [weak self] in
guard let self else { return false }
return subject.hasCipherChangesSubscription
}

// Send an inserted cipher
cipherService.cipherChangesSubject.send(
.inserted(.fixture(
id: "1",
login: .fixture(
password: "password123",
uris: [.fixture(uri: "bitwarden.com")],
username: "[email protected]",
),
)),
)

// Wait a bit to ensure no changes are processed
try await Task.sleep(nanoseconds: 100_000_000) // 0.1 seconds

XCTAssertFalse(identityStore.saveCredentialIdentitiesCalled)
}

/// `syncIdentities(vaultLockStatus:)` doesn't update the credential identity store with the identities
/// from the user's vault when the app context is `.appExtension`.
func test_syncIdentities_appExtensionContext() {
Expand Down
124 changes: 124 additions & 0 deletions BitwardenShared/Core/Autofill/Services/AutofillCredentialService.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,11 +86,21 @@ protocol AutofillCredentialService: AnyObject {
/// A default implementation of an `AutofillCredentialService`.
///
class DefaultAutofillCredentialService {
// MARK: Computed properties

/// Whether the cipher changes publisher has been subscribed to. This is useful for tests.
var hasCipherChangesSubscription: Bool {
cipherChangesSubscriptionTask != nil && !(cipherChangesSubscriptionTask?.isCancelled ?? true)
}

// MARK: Private Properties

/// Helper to know about the app context.
private let appContextHelper: AppContextHelper

/// A reference to the task used to track cipher changes.
private var cipherChangesSubscriptionTask: Task<Void, Never>?

/// The service used to manage syncing and updates to the user's ciphers.
private let cipherService: CipherService

Expand Down Expand Up @@ -191,6 +201,11 @@ class DefaultAutofillCredentialService {
self.vaultTimeoutService = vaultTimeoutService

guard appContextHelper.appContext == .mainApp else {
// NOTE: [PM-28855] when in the context of iOS extensions
// subscribe to individual cipher changes to update the local OS store
// to improve memory performance and avoid crashes by not loading
// nor potentially decrypting the whole vault.
subscribeToCipherChanges()
return
}

Expand All @@ -201,8 +216,37 @@ class DefaultAutofillCredentialService {
}
}

/// Deinitializes this service.
deinit {
cipherChangesSubscriptionTask?.cancel()
cipherChangesSubscriptionTask = nil
}

// MARK: Private Methods

/// Subscribes to cipher changes to update the internal `ASCredentialIdentityStore`.
private func subscribeToCipherChanges() {
cipherChangesSubscriptionTask?.cancel()
cipherChangesSubscriptionTask = Task { [weak self] in
guard let self else { return }

do {
for await cipherChange in try await cipherService.cipherChangesPublisher().values {
Comment on lines +228 to +234
Copy link

@claude claude bot Dec 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❌ Critical: If cipherChangesPublisher() throws during initialization, the error is logged but the subscription task completes silently. This means cipher changes won't update the credential store, breaking autofill functionality without any visible indication.

Recommendation: Add retry logic or fail-safe handling:

private func subscribeToCipherChanges() {
    cipherChangesSubscriptionTask?.cancel()
    cipherChangesSubscriptionTask = Task { [weak self] in
        guard let self else { return }
        
        var retryCount = 0
        let maxRetries = 3
        
        while retryCount < maxRetries && !Task.isCancelled {
            do {
                let publisher = try await cipherService.cipherChangesPublisher()
                for await cipherChange in publisher.values {
                    switch cipherChange {
                    case let .inserted(cipher):
                        await upsertCredentialsInStore(for: cipher)
                    case let .updated(cipher):
                        await upsertCredentialsInStore(for: cipher)
                    case let .deleted(cipher):
                        await removeCredentialsInStore(for: cipher)
                    }
                }
                break // Success
            } catch {
                errorReporter.log(error: error)
                retryCount += 1
                if retryCount < maxRetries {
                    try? await Task.sleep(nanoseconds: 1_000_000_000)
                }
            }
        }
    }
}

switch cipherChange {
case let .inserted(cipher):
await upsertCredentialsInStore(for: cipher)
case let .updated(cipher):
await upsertCredentialsInStore(for: cipher)
case let .deleted(cipher):
await removeCredentialsInStore(for: cipher)
}
}
} catch {
errorReporter.log(error: error)
}
}
}

/// Synchronizes the identities in the identity store for the user with the specified lock status.
///
/// - If the user's vault is unlocked, identities in the store will be replaced by the user's identities.
Expand Down Expand Up @@ -467,6 +511,30 @@ extension DefaultAutofillCredentialService: AutofillCredentialService {
return try await clientService.vault().ciphers().decrypt(cipher: encryptedCipher)
}

/// Gets the credential identities for a given cipher.
/// - Parameter cipher: The cipher to get the credential identities from.
/// - Returns: A list of credential identities for the cipher.
@available(iOS 17.0, *)
private func getCredentialIdentities(from cipher: Cipher) async throws -> [ASCredentialIdentity] {
var identities = [ASCredentialIdentity]()
let decryptedCipher = try await clientService.vault().ciphers().decrypt(cipher: cipher)

let newIdentities = await credentialIdentityFactory.createCredentialIdentities(from: decryptedCipher)
identities.append(contentsOf: newIdentities)

let fido2Identities = try await clientService.platform().fido2()
.authenticator(
userInterface: fido2UserInterfaceHelper,
credentialStore: fido2CredentialStore,
)
.credentialsForAutofill()
Comment on lines +525 to +530
Copy link

@claude claude bot Dec 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💭 Memory Concern: Does credentialsForAutofill() load all Fido2 credentials into memory before the .filter operation? If so, this defeats the memory efficiency goal of this PR for large vaults.

Question: Can we fetch Fido2 credentials for a specific cipher ID directly from the SDK, or is credentialsForAutofill() already efficient/lazy?

.filter { $0.cipherId == cipher.id }
.compactMap { $0.toFido2CredentialIdentity() }
identities.append(contentsOf: fido2Identities)

return identities
}

/// Provides a Fido2 credential based for the given request.
/// - Parameters:
/// - request: Request to get the assertion credential.
Expand Down Expand Up @@ -525,6 +593,48 @@ extension DefaultAutofillCredentialService: AutofillCredentialService {
throw error
}
}

/// Removes the credential identities associated with the cipher on the store.
/// - Parameter cipher: The cipher to get the credential identities from.
private func removeCredentialsInStore(for cipher: Cipher) async {
guard #available(iOS 17.0, *),
await identityStore.state().isEnabled,
await identityStore.state().supportsIncrementalUpdates else {
return
}

do {
let identities = try await getCredentialIdentities(from: cipher)
try await identityStore.removeCredentialIdentities(identities)

Logger.application.debug(
"[AutofillCredentialService] Removed \(identities.count) identities from \(cipher.id ?? "nil")",
)
} catch {
errorReporter.log(error: error)
}
}

/// Adds/Updates the credential identities associated with the cipher on the store.
/// - Parameter cipher: The cipher to get the credential identities from.
private func upsertCredentialsInStore(for cipher: Cipher) async {
guard #available(iOS 17.0, *),
await identityStore.state().isEnabled,
await identityStore.state().supportsIncrementalUpdates else {
return
}

do {
let identities = try await getCredentialIdentities(from: cipher)
try await identityStore.saveCredentialIdentities(identities)

Logger.application.debug(
"[AutofillCredentialService] Upserted \(identities.count) identities from \(cipher.id ?? "nil")",
)
} catch {
errorReporter.log(error: error)
}
}
}

// MARK: - CredentialIdentityStore
Expand All @@ -536,6 +646,13 @@ protocol CredentialIdentityStore {
///
func removeAllCredentialIdentities() async throws

/// Remove the given credential identities from the store.
///
/// - Parameter credentialIdentities: A list of credential identities to remove.
///
@available(iOS 17.0, *)
func removeCredentialIdentities(_ credentialIdentities: [any ASCredentialIdentity]) async throws

/// Replaces existing credential identities with new credential identities.
///
/// - Parameter newCredentialIdentities: The new credential identities.
Expand All @@ -549,6 +666,13 @@ protocol CredentialIdentityStore {
///
func replaceCredentialIdentities(with newCredentialIdentities: [ASPasswordCredentialIdentity]) async throws

/// Save the supplied credential identities to the store.
///
/// - Parameter credentialIdentities: A list of credential identities to save.
///
@available(iOS 17.0, *)
func saveCredentialIdentities(_ credentialIdentities: [any ASCredentialIdentity]) async throws

/// Gets the state of the credential identity store.
///
/// - Returns: The state of the credential identity store.
Expand Down
Loading
Loading