Skip to content

PM-40295: bug: Add MP Policy checks on vault unlock - #7165

Open
david-livefront wants to merge 1 commit into
mainfrom
PM-40295-mp-policy-on-unlock
Open

PM-40295: bug: Add MP Policy checks on vault unlock#7165
david-livefront wants to merge 1 commit into
mainfrom
PM-40295-mp-policy-on-unlock

Conversation

@david-livefront

@david-livefront david-livefront commented Jul 14, 2026

Copy link
Copy Markdown
Collaborator

🎟️ Tracking

PM-40295

📔 Objective

This PR moves all the MasterPassword Policy logic into a new class and adds the policy check on vault unlock as well as login.

@david-livefront
david-livefront requested a review from a team as a code owner July 14, 2026 19:25
@david-livefront david-livefront changed the title PM-40295: Add MP Policy checks on vault unlock PM-40295: bug: Add MP Policy checks on vault unlock Jul 14, 2026
@github-actions github-actions Bot added app:password-manager Bitwarden Password Manager app context t:bug Change Type - Bug labels Jul 14, 2026
@github-actions

github-actions Bot commented Jul 14, 2026

Copy link
Copy Markdown
Contributor

🤖 Bitwarden Claude Code Review

Overall Assessment: APPROVE

This PR extracts master password policy logic out of AuthRepositoryImpl into a new PasswordPolicyManager/PasswordPolicyManagerImpl, wires it through DI, and adds the policy check on vault unlock (VaultLockManagerImpl.processMasterPassword) in addition to login. AuthRepository now delegates to PasswordPolicyManager, so existing callers of passwordPolicies, passwordResetReason, getPasswordStrength, and validatePasswordAgainstPolicies remain source-compatible. Tests were moved alongside the new manager and unlock/logout paths gained corresponding storePasswordToCheck/removePasswordToCheck verifications. The extraction is clean and the validation logic is preserved verbatim.

Code Review Details
  • ❓ : Cached plaintext password is no longer cleared on user switch (only on lock/logout), lengthening the in-memory retention window for a backgrounded account
    • app/src/main/kotlin/com/x8bit/bitwarden/data/platform/manager/policy/PasswordPolicyManagerImpl.kt:101

* The password that needs to be checked against any organization policies before
* the user can complete the login flow. This value is stored using the user ID.
*/
private val mutablePasswordsToCheckFlow = MutableStateFlow(value = mapOf<String, String>())

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.

@vvolkgang Once the KeystoreManager is ready, I would like to encrypt this data.

@david-livefront
david-livefront force-pushed the PM-40295-mp-policy-on-unlock branch from b5a621e to b29e1e3 Compare July 14, 2026 19:29
@codecov

codecov Bot commented Jul 14, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 68.80734% with 34 lines in your changes missing coverage. Please review.
✅ Project coverage is 86.51%. Comparing base (b0daf66) to head (d4b422c).

Files with missing lines Patch % Lines
...atform/manager/policy/PasswordPolicyManagerImpl.kt 69.51% 1 Missing and 24 partials ⚠️
...twarden/data/vault/manager/VaultLockManagerImpl.kt 59.09% 5 Missing and 4 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #7165      +/-   ##
==========================================
+ Coverage   86.36%   86.51%   +0.15%     
==========================================
  Files         916      917       +1     
  Lines       65833    65886      +53     
  Branches     9718     9730      +12     
==========================================
+ Hits        56854    57000     +146     
+ Misses       5509     5418      -91     
+ Partials     3470     3468       -2     
Flag Coverage Δ
app-data 17.48% <67.88%> (-0.10%) ⬇️
app-ui-auth-tools 18.74% <0.91%> (-0.02%) ⬇️
app-ui-platform 16.82% <0.91%> (+0.30%) ⬆️
app-ui-vault 27.91% <0.00%> (+0.04%) ⬆️
authenticator 6.09% <0.00%> (-0.02%) ⬇️
lib-core-network-bridge 4.11% <0.00%> (-0.02%) ⬇️
lib-data-ui 1.18% <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 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.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@david-livefront
david-livefront force-pushed the PM-40295-mp-policy-on-unlock branch 3 times, most recently from 68addd6 to bd2e4c2 Compare July 21, 2026 19:35
@david-livefront
david-livefront force-pushed the PM-40295-mp-policy-on-unlock branch 2 times, most recently from 2f21e9f to 4156730 Compare July 27, 2026 18:36
@david-livefront
david-livefront force-pushed the PM-40295-mp-policy-on-unlock branch from 4156730 to d4b422c Compare July 29, 2026 13:40

@vvolkgang vvolkgang 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.

:shipit:

)
}
val password = when (initUserCryptoMethod) {
is InitUserCryptoMethod.MasterPasswordUnlock -> initUserCryptoMethod.password

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.

💭 Perhaps move this to InitUserCryptoMethodExtensions? updateKdfIfNeeded down below is doing the same. Not a blocker and can be dealt with later.


// Cache the password to verify against any password policies after the sync completes.
passwordsToCheckMap.put(userId, it)
storePasswordToCheck(userId = userId, password = it)

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.

Let's circle back on this later and research how we could restructure the sync + policy check process to remove this need.

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

Labels

app:password-manager Bitwarden Password Manager app context t:bug Change Type - Bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants