[PM-38443] feat: Add FillAssistRepository and sync integration#2812
Conversation
github-actions
Bot
added
app:password-manager
🤖 Bitwarden Claude Code ReviewOverall Assessment: REQUEST CHANGES Reviewed the Code Review Details
Notes on existing reviewer threads (no new action from me):
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
This PR introduces a new Fill-Assist sync orchestration layer (FillAssistRepository + DefaultFillAssistRepository) and wires it into the app’s dependency graph and vault sync flow so Fill-Assist rules can be fetched, validated, cached per-account, and consulted by hostname.
Changes:
- Added
FillAssistRepository/DefaultFillAssistRepositoryimplementing the fill-assist sync + caching pipeline. - Integrated the repository into
ServiceContainer/Servicesand injected it intoDefaultSyncService. - Updated mocks/tests wiring to accommodate the new dependency.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| BitwardenShared/Core/Vault/Services/SyncServiceTests.swift | Injects a MockFillAssistRepository into DefaultSyncService test setup. |
| BitwardenShared/Core/Vault/Services/SyncService.swift | Calls fill-assist sync during vault sync completion path. |
| BitwardenShared/Core/Platform/Services/TestHelpers/ServiceContainer+Mocks.swift | Adds fillAssistRepository to mock container factory. |
| BitwardenShared/Core/Platform/Services/Services.swift | Adds HasFillAssistRepository to the Services composition. |
| BitwardenShared/Core/Platform/Services/ServiceContainer.swift | Instantiates DefaultFillAssistRepository and injects it into DefaultSyncService and container. |
| BitwardenShared/Core/Autofill/Repositories/FillAssistRepository.swift | New repository implementing manifest fetch, schema check, integrity check, parsing, and per-account caching. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Sync fill-assist rules alongside vault data. Failures must not block vault sync. | ||
| await fillAssistRepository.syncFillAssistRules() | ||
|
|
| typealias Services = HasConfigService | ||
| & HasEnvironmentService | ||
| & HasFillAssistAPIService | ||
| & HasStateService | ||
|
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2812 +/- ##
==========================================
- Coverage 81.26% 78.88% -2.38%
==========================================
Files 1028 1158 +130
Lines 66273 74157 +7884
==========================================
+ Hits 53858 58502 +4644
- Misses 12415 15655 +3240 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| /// Fetches and caches fill-assist rules for the active account. | ||
| /// | ||
| func syncFillAssistRules() async | ||
|
|
||
| /// Returns the cached fill-assist rules for a given hostname, or `nil` if unavailable. | ||
| /// | ||
| /// - Parameter hostname: The hostname to look up. | ||
| /// - Returns: The cached `FillAssistHostRules`, or `nil`. | ||
| /// | ||
| func fillAssistRules(for hostname: String) async -> FillAssistHostRules? | ||
|
|
||
| /// Clears all cached fill-assist data for the active account. | ||
| /// | ||
| func clearFillAssistRules() async throws |
There was a problem hiding this comment.
⛏️ Given that this is the "Fill Assist" repository, I think we can remove the use of "FillAssist" in the function names as it's a bit repetitive IMO.
| let sourceUrl = environmentService.fillAssistRulesURL | ||
| let userId = try await stateService.getActiveAccountId() | ||
| let lastFetch = appSettingsStore.fillAssistLastFetchTimestamp(userId: userId) | ||
| if let lastFetch, Date().timeIntervalSince(lastFetch) < Constants.fillAssistUpdateInterval { |
There was a problem hiding this comment.
🎨 Use TimeProvider instead of Date() so you can better mock the date on the tests.
This applies to all Date() in this file.
| ) | ||
| } | ||
|
|
||
| // Sync fill-assist rules alongside vault data. Failures must not block vault sync. |
There was a problem hiding this comment.
⛏️ I'd remove "Failures must not block vault sync." as the function doesn't throw any errors so I think it's a direct assumption.
| } | ||
|
|
||
| // Sync fill-assist rules alongside vault data. Failures must not block vault sync. | ||
| await fillAssistRepository.syncFillAssistRules() |
| private func makeManifest(cid: String) throws -> FillAssistManifestResponseModel { | ||
| let json = """ | ||
| { | ||
| "buildId": "v1", | ||
| "gitSha": "abc", | ||
| "maps": { "forms": { "v1": { | ||
| "cid": "\(cid)", | ||
| "filename": "forms.v1.json", | ||
| "schema": "forms.v1.schema.json" | ||
| }}}, | ||
| "timestamp": "2026-06-11T14:29:32.184Z" | ||
| } | ||
| """ | ||
| return try JSONDecoder.pascalOrSnakeCaseDecoder.decode( | ||
| FillAssistManifestResponseModel.self, | ||
| from: Data(json.utf8), | ||
| ) | ||
| } | ||
|
|
||
| private func makeFormsMapWithPathnames() throws -> FormsMapResponseModel { | ||
| let json = """ | ||
| { | ||
| "schemaVersion": "1.0.0", | ||
| "hosts": { | ||
| "example.com": { | ||
| "forms": [{ "category": "account-login", "fields": { "username": ["input#user1"] } }], | ||
| "pathnames": { | ||
| "/login": { | ||
| "forms": [{ "category": "account-login", "fields": { "username": ["input#user2"] } }] | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| """ | ||
| return try JSONDecoder.pascalOrSnakeCaseDecoder.decode( | ||
| FormsMapResponseModel.self, | ||
| from: Data(json.utf8), | ||
| ) | ||
| } | ||
|
|
||
| private func makeFormsMap(schemaVersion: String = "1.0.0") throws -> FormsMapResponseModel { | ||
| let json = """ | ||
| { | ||
| "schemaVersion": "\(schemaVersion)", | ||
| "hosts": { | ||
| "example.com": { | ||
| "forms": [{ "category": "account-login", "fields": { "username": ["input#user"] } }] | ||
| } | ||
| } | ||
| } | ||
| """ | ||
| return try JSONDecoder.pascalOrSnakeCaseDecoder.decode( | ||
| FormsMapResponseModel.self, | ||
| from: Data(json.utf8), | ||
| ) | ||
| } |
There was a problem hiding this comment.
🤔 Can't we directly initialize the structs instead of using JSON decoding?
…red for fill-assist CDN
…ling tests to FillAssistRepository
…der, move helpers to model layer
…nd add coverage for edge cases
…epositoryTests - Add @mainactor so featureFlagsBool mutations compile in Swift Testing - Move helper methods to a private extension to satisfy type_body_length
…t fillAssistClient for tests - Filter pooled entries with empty attribute arrays before writing host rules, so hosts with only unsupported selectors (e.g. shadow DOM) are correctly excluded - Add fillAssistClient parameter to APIService.init (defaults to URLSession.shared) so FillAssistAPIServiceTests can inject MockHTTPClient for download calls
6b5d552 to
4d9842c
Compare
…uct initialization Add memberwise inits to FormsMapResponseModel and FormsMapHostEntry so test helpers can construct fixtures without JSON string construction.
| func makeManifest(cid: String) -> FillAssistManifestResponseModel { | ||
| let entry = FillAssistManifestEntryModel( | ||
| cid: cid, | ||
| deprecated: false, | ||
| filename: "forms.v1.json", | ||
| schema: "forms.v1.schema.json", | ||
| ) | ||
| return FillAssistManifestResponseModel( | ||
| buildId: "v1", | ||
| gitSha: "abc", | ||
| maps: ["forms": ["v1": entry]], | ||
| timestamp: Date(timeIntervalSinceReferenceDate: 0), | ||
| ) | ||
| } | ||
|
|
||
| private func makeFormsMap( | ||
| schemaVersion: String = "1.0.0", | ||
| usernameSelector: String = "input#user", | ||
| hosts: [String: String]? = nil, | ||
| ) -> FormsMapResponseModel { | ||
| let content = { (selector: String) in | ||
| FormsMapContent( | ||
| category: "account-login", | ||
| container: nil, | ||
| fields: ["username": [.single(selector)]], | ||
| actions: nil, | ||
| ) | ||
| } | ||
| let resolvedHosts: [String: FormsMapHostEntry] = if let hosts { | ||
| hosts.mapValues { FormsMapHostEntry(forms: [content($0)]) } | ||
| } else { | ||
| ["example.com": FormsMapHostEntry(forms: [content(usernameSelector)])] | ||
| } | ||
| return FormsMapResponseModel(hosts: resolvedHosts, schemaVersion: schemaVersion) | ||
| } | ||
|
|
||
| private func makeFormsMapWithPathnames() -> FormsMapResponseModel { | ||
| let loginContent = FormsMapContent( | ||
| category: "account-login", | ||
| container: nil, | ||
| fields: ["username": [.single("input#user1")]], | ||
| actions: nil, | ||
| ) | ||
| let pathnameContent = FormsMapContent( | ||
| category: "account-login", | ||
| container: nil, | ||
| fields: ["username": [.single("input#user2")]], | ||
| actions: nil, | ||
| ) | ||
| return FormsMapResponseModel( | ||
| hosts: [ | ||
| "example.com": FormsMapHostEntry( | ||
| forms: [loginContent], | ||
| pathnames: ["/login": FormsMapPathnameEntry(forms: [pathnameContent])], | ||
| ), | ||
| ], | ||
| schemaVersion: "1.0.0", | ||
| ) | ||
| } |
…its and test helpers
github-actions
Bot
added
the
app:authenticator
… timestamp is fresh
… timestamp is fresh
…thod and its test
…c-skip behavior syncRules_withinUpdateInterval now also stubs cached data to match the updated skip condition, and a new test covers a fresh timestamp with no readable cache. Fixture builders moved to a separate file to stay under the file length limit.
Adds setFillAssistEnabledError to MockStateService and a test verifying AutoFillProcessor reverts the toggle and shows an alert when persisting the preference fails.
…uests The tests still mocked the old download(filename:)-based HTTPClient API, but the production code was migrated to fillAssistService.send(_:) in an earlier commit. Update the mock setup and assertions to match.
The reprompt handler only proceeded when the app extension's canAutofill was true, blocking Fill Assist-driven autofill from the main app context.
fedemkr
left a comment
fedemkr
left a comment
There was a problem hiding this comment.
Looks good, approving as forceSync should be handled in a different PR.
| cached != nil, | ||
| timeProvider.presentTime.timeIntervalSince(lastFetch) < Constants.FillAssist.updateInterval { | ||
| return | ||
| } |
There was a problem hiding this comment.
🤔 I believe we should consider checking forceSync here to bypass this check. Otherwise users will need to wait up to 6 hours when a change is done in map-the-web.
I think it can be done in a different PR so not to keep adding stuff to this one.
…e flag The autofill-capability bypass only checked the persisted user toggle, not the live fillAssistTargetingRules flag, so disabling the flag after a user had opted in wouldn't revert the bypass. Also strengthens the flagOff test to actually exercise canAutofill == false.
| public func fingerprint(for data: Encodable) throws -> String { | ||
| let encoder = JSONEncoder() | ||
| encoder.outputFormatting = [.sortedKeys] | ||
| return try encoder.encode(data).generatedHash(using: SHA256.self) | ||
| } |
| appSettingsStore.setDefaultUriMatchType(nil, userId: knownUserId) | ||
| appSettingsStore.setDisableAutoTotpCopy(nil, userId: knownUserId) | ||
| appSettingsStore.setEncryptedUserKey(key: nil, userId: knownUserId) | ||
| appSettingsStore.setFillAssistCachedData(nil, userId: knownUserId) | ||
| appSettingsStore.setFillAssistLastFetchTimestamp(nil, userId: knownUserId) | ||
| appSettingsStore.setHasPerformedSyncAfterLogin(nil, userId: knownUserId) | ||
| appSettingsStore.setLastSyncTime(nil, userId: knownUserId) |
There was a problem hiding this comment.
I'm calling Clear rules, which clears that cache, here: https://github.com/bitwarden/ios/pull/2812/changes/BASE..49ad11ab3c1f39a8f024ef68796e26136df376be#diff-cb8846480d62aaf308bd0e3a3cc1f018f98ec0031d052faf961fe8e90982fc1bL868
| appSettingsStore.setEncryptedUserKey(key: nil, userId: knownUserId) | ||
| appSettingsStore.setFillAssistCachedData(nil, userId: knownUserId) | ||
| appSettingsStore.setFillAssistLastFetchTimestamp(nil, userId: knownUserId) | ||
| appSettingsStore.setHasPerformedSyncAfterLogin(nil, userId: knownUserId) |
There was a problem hiding this comment.
logoutAccount leaves cache behind on account deletion.
Details and fix
This PR removed setFillAssistCachedData(nil) and setFillAssistLastFetchTimestamp(nil) from logoutAccount and relocated that clearing into AuthRepository.logout (via fillAssistRepository.clearRules). However, AuthRepository.deleteAccount does not call clearRules — it goes deleteAccount → stateService.deleteAccount() → logoutAccount(userInitiated: true).
Before this change, logoutAccount cleared the fill-assist cache for both the logout and the delete-account flows. Now the delete-account flow leaves fillAssistCachedData_<userId>, fillAssistLastFetchTimestamp_<userId>, and the Keychain fingerprint (fillAssistRulesFingerprint_<userId>) behind after the account is deleted.
Fix: call fillAssistRepository.clearRules(userId:) in AuthRepository.deleteAccount as well (before stateService.deleteAccount()), mirroring logout.
AuthRepositoryTests.test_deleteAccount has no clearRulesCalled assertion, so this gap is currently untested.
There was a problem hiding this comment.
I'm calling Clear rules, which clears that cache, here: https://github.com/bitwarden/ios/pull/2812/changes/BASE..49ad11ab3c1f39a8f024ef68796e26136df376be#diff-cb8846480d62aaf308bd0e3a3cc1f018f98ec0031d052faf961fe8e90982fc1bL868
| appSettingsStore.setEncryptedUserKey(key: nil, userId: knownUserId) | ||
| appSettingsStore.setFillAssistCachedData(nil, userId: knownUserId) | ||
| appSettingsStore.setFillAssistLastFetchTimestamp(nil, userId: knownUserId) | ||
| appSettingsStore.setHasPerformedSyncAfterLogin(nil, userId: knownUserId) |
There was a problem hiding this comment.
logoutAccount leaves cache behind on account deletion.
Details and fix
This PR removed setFillAssistCachedData(nil) and setFillAssistLastFetchTimestamp(nil) from logoutAccount and relocated that clearing into AuthRepository.logout (via fillAssistRepository.clearRules). However, AuthRepository.deleteAccount does not call clearRules — it goes deleteAccount → stateService.deleteAccount() → logoutAccount(userInitiated: true).
Before this change, logoutAccount cleared the fill-assist cache for both the logout and the delete-account flows. Now the delete-account flow leaves fillAssistCachedData_<userId>, fillAssistLastFetchTimestamp_<userId>, and the Keychain fingerprint (fillAssistRulesFingerprint_<userId>) behind after the account is deleted.
Fix: call fillAssistRepository.clearRules(userId:) in AuthRepository.deleteAccount as well (before stateService.deleteAccount()), mirroring logout.
AuthRepositoryTests.test_deleteAccount has no clearRulesCalled assertion, so this gap is currently untested.
🗒️ Note
To avoid further conflicts, this PR contains the following stacked approved PRs:
#2876
#2882
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-38443
📔 Objective
Adds
FillAssistRepository(protocol +DefaultFillAssistRepository) which orchestrates the full fill-assist sync pipeline: feature flag check, 6-hour interval guard, manifest fetch, cid/sourceUrl cache comparison, forms download, schema version validation, CSS selector parsing, and per-account storage. Wires the repository intoSyncService(non-blocking) andServiceContainer.Also fixes
APIService.fillAssistServiceto use a plainHTTPService(client: URLSession.shared)— the standardCertificateHTTPClientdeliberately blocks 302 redirects for SSO, which broke the GitHub Releases CDN redirect chain.