Skip to content

PM-38358: Chore: Remove user key#7004

Open
david-livefront wants to merge 2 commits into
mainfrom
PM-38358-remove-user-key
Open

PM-38358: Chore: Remove user key#7004
david-livefront wants to merge 2 commits into
mainfrom
PM-38358-remove-user-key

Conversation

@david-livefront
Copy link
Copy Markdown
Collaborator

@david-livefront david-livefront commented Jun 1, 2026

🎟️ Tracking

PM-38358

📔 Objective

This PR removes the stored encrypted user key from shared preferences and updates the remaining location that relied on it.

@david-livefront david-livefront requested a review from a team as a code owner June 1, 2026 20:20
@david-livefront david-livefront added the ai-review-vnext Request a Claude code review using the vNext workflow label Jun 1, 2026
@github-actions github-actions Bot added app:password-manager Bitwarden Password Manager app context t:tech-debt Change Type - Tech debt labels Jun 1, 2026
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Jun 1, 2026

🤖 Bitwarden Claude Code Review

Overall Assessment: APPROVE

This PR removes the legacy stored encrypted user key (MASTER_KEY_ENCRYPTION_USER_KEY in shared preferences) and migrates the remaining consumers — validatePassword and removePassword — to read masterKeyWrappedUserKey from profile.userDecryptionOptions.masterPasswordUnlock. It also adds a one-time removeLegacyUserKeys() cleanup during AuthDiskSourceImpl init, updates toRemovedPasswordUserStateJson to clear masterPasswordUnlock, and updates toUpdatedUserStateJson to keep hasMasterPassword in sync with masterPasswordUnlock != null across both code paths. Tests were updated accordingly: the legacy assertUserKey / storeUserKey / getUserKey test helpers are removed, and a new init-level test verifies the legacy key removal.

Code Review Details

No findings. The removed-key fallback path is consistent with unlockVaultWithMasterPassword, which already reads from userDecryptionOptions.masterPasswordUnlock. removeWithPrefix correctly targets unencrypted sharedPreferences (where the legacy key was stored), and the new AuthDiskSourceTest seeding now matches that store. The previously-raised concern about the test not actually validating the cleanup has been addressed by the author.

@david-livefront david-livefront force-pushed the PM-38358-remove-user-key branch from 526a537 to cd9dc77 Compare June 1, 2026 20:22
?.copy(hasMasterPassword = false)
?.copy(
hasMasterPassword = false,
masterPasswordUnlock = null,
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

This change ensures that masterPasswordUnlock is cleared as well to avoid any future confusion.

?.copy(masterPasswordUnlock = syncUserDecryption.masterPasswordUnlock)
?.copy(
hasMasterPassword = syncUserDecryption.masterPasswordUnlock != null,
masterPasswordUnlock = syncUserDecryption.masterPasswordUnlock,
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

And this ensures that both code pathways are consistent regardless of whether the userDecryptionOptions are present or not.

Comment on lines +39 to +68
@@ -56,6 +60,13 @@ class AuthDiskSourceTest {
verify(exactly = 1) { legacySecureStorageMigrator.migrateIfNecessary() }
}

@Test
fun `initialization should trigger a legacy user key removal`() {
assertNull(
fakeSharedPreferences.getString("bwPreferencesStorage:masterKeyEncryptedUserKey", null),
)
}
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.

⚠️ IMPORTANT: This test does not actually validate that removeLegacyUserKeys() runs — it would pass even if the cleanup were deleted.

Details and fix

The legacy user key was stored in the non-encrypted sharedPreferences (see MASTER_KEY_ENCRYPTION_USER_KEY in the "should not be encrypted" group, and getUserKey/storeUserKey previously used getString/putString). removeLegacyUserKeys() calls removeWithPrefix(...), which only iterates sharedPreferences — not the encrypted one.

However, the seed here puts the key into fakeEncryptedSharedPreferences (which is wired as encryptedSharedPreferences), and the assertion then reads from fakeSharedPreferences, which was never populated. The assertion would be vacuously true even if removeLegacyUserKeys() were never called.

Suggested fix — seed the key into the same prefs object that removeWithPrefix operates on:

private val fakeEncryptedSharedPreferences = FakeSharedPreferences()
private val fakeSharedPreferences = FakeSharedPreferences().apply {
    edit(commit = true) {
        putString("bwPreferencesStorage:masterKeyEncryptedUserKey_userId", "testUserKey")
    }
}

And then the assertion at line 64-68 will actually verify the removal happened.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

@claude this has been addressed.

@codecov
Copy link
Copy Markdown

codecov Bot commented Jun 1, 2026

Codecov Report

❌ Patch coverage is 62.50000% with 9 lines in your changes missing coverage. Please review.
✅ Project coverage is 86.06%. Comparing base (c18dd58) to head (c4e42f3).

Files with missing lines Patch % Lines
...twarden/data/auth/repository/AuthRepositoryImpl.kt 50.00% 0 Missing and 8 partials ⚠️
...ta/auth/repository/util/UserStateJsonExtensions.kt 83.33% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #7004      +/-   ##
==========================================
- Coverage   86.49%   86.06%   -0.43%     
==========================================
  Files         873      953      +80     
  Lines       63852    65902    +2050     
  Branches     9248     9298      +50     
==========================================
+ Hits        55229    56719    +1490     
- Misses       5439     5979     +540     
- Partials     3184     3204      +20     
Flag Coverage Δ
app-data 17.10% <62.50%> (-0.36%) ⬇️
app-ui-auth-tools 19.16% <0.00%> (+0.19%) ⬆️
app-ui-platform 16.96% <0.00%> (+0.51%) ⬆️
app-ui-vault 28.29% <0.00%> (+0.55%) ⬆️
authenticator 6.20% <0.00%> (+<0.01%) ⬆️
lib-core-network-bridge 4.07% <0.00%> (+<0.01%) ⬆️
lib-data-ui 1.14% <0.00%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 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.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@david-livefront david-livefront force-pushed the PM-38358-remove-user-key branch from cd9dc77 to c4e42f3 Compare June 1, 2026 20:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ai-review-vnext Request a Claude code review using the vNext workflow 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.

1 participant