Skip to content

[PM-37888] refactor: Remove unused AccountEncryptionKeys wrapper - #2861

Open
matt-livefront wants to merge 4 commits into
mainfrom
matt/PM-37888-remove-account-encryption-keys
Open

[PM-37888] refactor: Remove unused AccountEncryptionKeys wrapper#2861
matt-livefront wants to merge 4 commits into
mainfrom
matt/PM-37888-remove-account-encryption-keys

Conversation

@matt-livefront

@matt-livefront matt-livefront commented Jul 6, 2026

Copy link
Copy Markdown
Collaborator

🎟️ Tracking

PM-37888

📔 Objective

Follow-up to #2780 and #2764, which moved all real consumers off of AccountEncryptionKeys.encryptedUserKey. This removes the AccountEncryptionKeys wrapper struct since every explicit constructor already passed encryptedUserKey: nil, and the sole consumer of an AccountEncryptionKeys value (initializeUserCrypto) only ever read cryptographicState. WrappedAccountCryptographicState can be used directly wherever AccountEncryptionKeys was used.

  • Delete AccountEncryptionKeys; replace StateService.get/setAccountEncryptionKeys with get/setAccountCryptographicState returning/taking WrappedAccountCryptographicState directly
  • Move the responseModel:-based factory onto WrappedAccountCryptographicState itself (WrappedAccountCryptographicState+Extensions.swift)
  • Update all call sites in AuthRepository, AuthService, SyncService, KeyConnectorService, and AuthenticatorSyncService
  • Remove the legacy encryptedUserKey storage in AppSettingsStore (unused since the master-password-unlock key moved to MasterPasswordUnlockResponseModel in [PM-37888] refactor: Read masterKeyWrappedUserKey from masterPasswordUnlock #2780)
  • Remove the now-unused key: String? from AccountKeysResponseModelProtocol, ProfileResponseModel, and SetAccountKeysResponseModel. IdentityTokenResponseModel.key is kept — it's still used in AuthService.unlockMethod(for:) for the key connector unlock path

@matt-livefront
matt-livefront requested a review from a team as a code owner July 6, 2026 21:22
@matt-livefront matt-livefront added ai-review Request a Claude code review t:tech-debt Change Type - Tech debt labels Jul 6, 2026
@github-actions github-actions Bot added the app:password-manager Bitwarden Password Manager app context label Jul 6, 2026
@github-actions

github-actions Bot commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

🤖 Bitwarden Claude Code Review

Overall Assessment: APPROVE

Reviewed the removal of the AccountEncryptionKeys wrapper in favor of using WrappedAccountCryptographicState directly across StateService, AuthRepository, AuthService, SyncService, KeyConnectorService, AuthenticatorSyncService, and the CryptoClientProtocol extension. Verified that the legacy masterKeyEncryptedUserKey storage had no remaining readers on main before removal, that new migration 7 matches the storage-key format (bwPreferencesStorage:masterKeyEncryptedUserKey_<userId>) and the pattern established by migration 6, and that the responseModel: factory moved to WrappedAccountCryptographicState preserves the original accountKeysprivateKey fallback and nil-return behavior with equivalent test coverage. Also confirmed IdentityTokenResponseModel.key is correctly retained for the key connector unlock path after key was dropped from AccountKeysResponseModelProtocol, ProfileResponseModel, and SetAccountKeysResponseModel.

Code Review Details
  • ❓ : Fill-assist cache clearing added to logoutAccount appears to be an unintended merge artifact — it doesn't exist on main and duplicates FillAssistRepository.clearRules(userId:)
    • BitwardenShared/Core/Platform/Services/StateService.swift:1995-1996

@codecov

codecov Bot commented Jul 6, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 81.72%. Comparing base (55c8d84) to head (21de392).
⚠️ Report is 4 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2861      +/-   ##
==========================================
+ Coverage   79.45%   81.72%   +2.26%     
==========================================
  Files        1172     1048     -124     
  Lines       75100    67768    -7332     
==========================================
- Hits        59670    55382    -4288     
+ Misses      15430    12386    -3044     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

LRNcardozoWDF
LRNcardozoWDF previously approved these changes Jul 7, 2026

@LRNcardozoWDF LRNcardozoWDF left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM!

Comment on lines +1995 to +1996
appSettingsStore.setFillAssistCachedData(nil, userId: knownUserId)
appSettingsStore.setFillAssistLastFetchTimestamp(nil, userId: knownUserId)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

QUESTION: These two fill-assist lines look like a merge artifact — were they intended in this PR?

Details

The diff replaces appSettingsStore.setEncryptedUserKey(key: nil, userId: knownUserId) with two fill-assist clears that don't exist on main. main's logoutAccount has no fill-assist cleanup — PM-38443 (#2811 added it here, #2812 moved it out) relocated that responsibility to FillAssistRepository.clearRules(userId:), which AuthRepository.logout already calls before stateService.logoutAccount.

So on the normal logout path this is now duplicated, and it also isn't mentioned in the PR description for an otherwise pure AccountEncryptionKeys refactor. If it wasn't intentional, dropping both lines restores parity with main.

(Note: deleteAccount() calls logoutAccount directly without going through AuthRepository.logout, so if the intent was to cover that path, a comment or a call to clearRules would make it explicit — and the keychain fillAssistRulesFingerprint is still left behind either way.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ai-review Request a Claude code review app:password-manager Bitwarden Password Manager app context t:tech-debt Change Type - Tech debt

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants