[PM-28855] Update credential identities store on cipher changes on iOS extensions #2169
Conversation
…tities store when cipher get CUD on iOS extensions.
|
Claude finished @fedemkr's task —— View job Code Review CompleteSummaryThis PR implements memory-efficient credential identity updates for iOS extensions by subscribing to individual cipher changes instead of batch-loading the entire vault. The implementation is architecturally sound and well-tested. FindingsFinding 1: Potential memory leak in deinit with Task cancellationThe Current implementationdeinit {
cipherChangesSubscriptionTask?.cancel()
cipherChangesSubscriptionTask = nil
}Recommendation: Consider if the explicit Finding 2: Typo in DocC comment
/// - Parameter cipher: The cipher to get the credential identiteis from.Should be: /// - Parameter cipher: The cipher to get the credential identities from.Finding 3: Error handling silently logs but doesn't retry💭 Question In } catch {
errorReporter.log(error: error)
}If the publisher fails once (e.g., due to database corruption), the extension will stop receiving updates for the rest of its lifetime. Consider if this is acceptable or if recovery/retry logic is needed. Finding 4: CipherChangePublisher doesn't filter by cipher type💭 Observation
Currently, deleted secure notes, cards, and identities trigger decryption operations that produce no identities. This may be intentional for simplicity, but filtering by Finding 5: iOS 17+ availability guard spans large code blocks🎨 Code Structure Both Consider alternative structure@available(iOS 17.0, *)
private func upsertCredentialsInStore(for cipher: Cipher) async {
guard await identityStore.state().isEnabled,
await identityStore.state().supportsIncrementalUpdates else {
return
}
// ... rest of implementation
}And for iOS 16 and below, provide a no-op stub or handle differently in the calling code. This makes the availability requirements clearer at the method signature level. Finding 6: Test coverage for error scenarios🎨 Suggestion
These error paths exist and are logged, but aren't explicitly tested. Consider adding tests to verify errors are properly logged and don't crash the subscription. Good Practices Observed
Action Items
|
|
Great job! No new security vulnerabilities introduced in this pull request |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2169 +/- ##
==========================================
+ Coverage 85.39% 85.42% +0.03%
==========================================
Files 1731 1732 +1
Lines 145718 146209 +491
==========================================
+ Hits 124434 124899 +465
- Misses 21284 21310 +26 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
🎟️ Tracking
PM-28855
📔 Objective
Add cipher changes subscription to update credential identities store on iOS extensions.
This differs from the current approach where all ciphers/credentials get updated to be done on individual cipher CUD helping memory efficiency.
As you can see the new
CipherChangePublisherpublishes only when "one" cipher operation was done, but it doesn't when there's a batch operation like replacing all ciphers to avoid memory problems on the extensions.Important
This is a solution for iOS 17+ versions as they have incremental updates on the credential identities store. I didn't add the fallback as it'd imply replacing all identities from all ciphers which means loading and decrypting all (or most) ciphers which may crash on large vaults or have the extension use more memory leaving less for other operations like searching before crashing. The user can always force a sync on the main app to update the OS store and have autofill working for that specific credential again.
⏰ 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