Skip to content

[PM-35393] Master Password Service Self-service#7587

Closed
enmande wants to merge 11 commits intomainfrom
auth/pm-35393/mp-service-self-service
Closed

[PM-35393] Master Password Service Self-service#7587
enmande wants to merge 11 commits intomainfrom
auth/pm-35393/mp-service-self-service

Conversation

@enmande
Copy link
Copy Markdown
Contributor

@enmande enmande commented May 5, 2026

🎟️ Tracking

PM-35393

📔 Objective

Implements Auth domain integrations of the MasterPasswordService.
1/4: Connects Master Password Self-Service functions.

📸 Screenshots

Master Password Self-Service

User enmande updates their Master Password using self-service.

  • Current Master Password is required and passes validation.
  • New Master Password is hashed and set.
  • User is logged out on success (behavioral parity with main).
  • User can log in with the new Master Password.
pm-35393__mp-self-service.mov

@enmande enmande added the ai-review Request a Claude code review label May 5, 2026
@enmande enmande requested a review from ike-kottlowski May 5, 2026 20:58
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 5, 2026

Bitwarden Claude Code Review

Overall Assessment: APPROVE

This PR introduces ISelfServicePasswordChangeCommand as the new dispatch target for the /accounts/password endpoint, splits PasswordRequestModel from its old SecretVerificationRequestModel base, adds a sibling ChangeKdfRequestModel, and centralizes the dual-shape exclusivity check in MasterPasswordPayloadVariantValidator. The new command verifies the current password via IUserService.CheckPasswordAsync, delegates persistence to IMasterPasswordService.SaveUpdateExistingMasterPasswordAsync, and emits the change-password event plus logout push. The previously-flagged IMPORTANT finding (KDF iteration-minimum enforcement blocking legacy users on change-password) was addressed in commit 22e6f7fb0 exactly as suggested — PasswordRequestModel now only enforces KDF/salt equivalence, while ChangeKdfRequestModel retains full KDF validation appropriate to its endpoint, and a regression test was added back to guard the invariant.

Code Review Details

No new findings.

The split between PasswordRequestModel (change-password, no KDF minimum enforcement) and ChangeKdfRequestModel (change-KDF, full KDF validation) correctly aligns with the documented invariant on KdfSettingsValidator.ValidateAuthenticationAndUnlockData and IMasterPasswordService. Timing protection (Task.Delay(2000) on failure) is preserved. Test coverage is comprehensive across the new command, both request models, and the variant validator.

Comment thread src/Api/Auth/Models/Request/Accounts/PasswordRequestModel.cs
@codecov
Copy link
Copy Markdown

codecov Bot commented May 5, 2026

Codecov Report

❌ Patch coverage is 88.76404% with 10 lines in your changes missing coverage. Please review.
✅ Project coverage is 59.76%. Comparing base (f835212) to head (22e6f7f).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
src/Api/Auth/Controllers/AccountsController.cs 40.00% 8 Missing and 1 partial ⚠️
...th/Models/Request/Accounts/PasswordRequestModel.cs 92.85% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #7587      +/-   ##
==========================================
+ Coverage   59.73%   59.76%   +0.02%     
==========================================
  Files        2102     2105       +3     
  Lines       92691    92762      +71     
  Branches     8257     8268      +11     
==========================================
+ Hits        55371    55438      +67     
- Misses      35366    35368       +2     
- Partials     1954     1956       +2     

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

@sonarqubecloud
Copy link
Copy Markdown

sonarqubecloud Bot commented May 5, 2026

@enmande enmande changed the title Auth/pm 35393/mp service self service [PM-35393] Master Password Service Self-service May 6, 2026
@enmande enmande closed this May 6, 2026
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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant