Skip to content

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

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

[PM-37888] refactor: Remove unused AccountEncryptionKeys wrapper#2861
matt-livefront wants to merge 2 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 struct and its replacement with WrappedAccountCryptographicState used directly across StateService, AuthRepository, AuthService, SyncService, KeyConnectorService, and AuthenticatorSyncService. Also reviewed migration 7 (removal of the legacy masterKeyEncryptedUserKey UserDefaults value) and the removal of the now-unused key field from AccountKeysResponseModelProtocol, ProfileResponseModel, and SetAccountKeysResponseModel. Verified all call sites are updated, no orphaned references to the removed API remain, and IdentityTokenResponseModel.key is correctly retained for the key connector unlock path.

Code Review Details

No findings.

Notes from validation:

  • Migration 7 follows the same hardcoded-key pattern as migration 6 and is registered in the migrations array, so the version counter advances correctly and it runs for any account below version 7. It only removes a UserDefaults value that is no longer read, making it safe cleanup.
  • The WrappedAccountCryptographicState(responseModel:) initializer preserves the original private-key resolution logic; the only behavior change is dropping the unused encryptedUserKey assignment.
  • Removal of the encryptedUserKey accessors from AppSettingsStore and its mock is complete with no remaining consumers.
  • Test coverage is comprehensively updated to match the new API, including new tests for migration 7 and the relocated init(responseModel:).

@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.24%. Comparing base (72273fe) to head (cdfd8b6).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2861      +/-   ##
==========================================
- Coverage   81.25%   81.24%   -0.01%     
==========================================
  Files        1027     1026       -1     
  Lines       66091    66054      -37     
==========================================
- Hits        53699    53668      -31     
+ Misses      12392    12386       -6     

☔ 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 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!

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