Skip to content

[PM-35393] MasterPasswordService auth integration#7575

Open
enmande wants to merge 10 commits intomainfrom
auth/pm-35393/master-password-service-auth-integration
Open

[PM-35393] MasterPasswordService auth integration#7575
enmande wants to merge 10 commits intomainfrom
auth/pm-35393/master-password-service-auth-integration

Conversation

@enmande
Copy link
Copy Markdown
Contributor

@enmande enmande commented May 1, 2026

🎟️ Tracking

PM-35393

📔 Objective

Integrate Auth-domain callers with MasterPasswordService.
Wires five password-mutation flows to the service added in #7530 :

  • Self-service password change
  • TDE offboarding
  • Emergency Access Takeover
  • Temporary password replacement
  • SSO JIT provisioning.

All relevant request models now accept both payload variants ("new": AuthenticationData + UnlockData, or "legacy": NewMasterPasswordHash + Key) for backward compatibility.
KDF validation is enforced at the request model layer whenever new payloads are present.
Legacy entry points on IUserService are marked [Obsolete] with specific replacement guidance. Legacy fields are tracked for removal in PM-33141.

📸 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

TDE Offboarding

  • User zed belongs to an SSO-required Trusted Device Encryption organization.
  • Organization Owner enmande converts the organization to Master Password Encryption. This is generally described as "TDE Offboarding."
  • User zed is required to set a new Master Password at next login; password is successfully set and allows zed to log in and view their vault.

NOTE: This organization retains SSO requirement throughout; zed remains authenticated to SSO for the duration of this test, so SSO challenge is not represented in this video.

pm-35393__tde-offboarding.mov

Emergency Access Takeover

  • User zed, who is enrolled in Emergency Access Takeover by organizational policy, has their account taken over ("Recovered") by organization Owner enmande.
  • enmande sets zed's new temporary Master Password.
  • New temporary Master Password is accepted at next login for zed's account.
  • Upon login, zed's account requires the setting of a new Master Password.
  • User is logged out on change of the Master Password.
  • New Master Password hash is set, and enabled subsequent login and access to the vault.
pm-35393__account-recovery.mov

@enmande enmande changed the title Auth/pm 35393/master password service auth integration [PM-35393] MasterPasswordService auth integration May 1, 2026
@enmande enmande added the ai-review Request a Claude code review label May 4, 2026
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 4, 2026

🤖 Bitwarden Claude Code Review

Overall Assessment: APPROVE

This PR integrates the new IMasterPasswordService (Prepare/Save/Build × Set/Update) into the existing master-password mutation flows: self-service password change, SSO JIT provision, TDE set/offboarding, admin temp-password replacement, and emergency-access recovery takeover. Each flow accepts both the new dual-payload shape (AuthenticationData + UnlockData) and the legacy shape (NewMasterPasswordHash + Key) via shared validators, with legacy entry points clearly marked [Obsolete] for the PM-33141 cleanup. Security invariants — salt-unchanged, no pre-existing master password where required, KDF parameter validation, Key-Connector exclusion, and the Task.Delay(2000) timing mitigation in PostPassword — are preserved and centrally enforced inside MasterPasswordService/SetInitialPasswordData.ValidateDataForUser. Test coverage is thorough across the new commands, request models, and the extracted MasterPasswordPayloadVariantValidator.

📋 Review Details

Findings

No blocking, important, or debt-level findings. The dual-payload acceptance in MasterPasswordPayloadVariantValidator.ValidateExclusivity is intentional per its docstring (transition period — clients may send both; callers prefer the new shape).

Files Reviewed

  • src/Api/Auth/Controllers/AccountsController.cs (PostPassword, PutUpdateTempPasswordAsync, PutUpdateTdePasswordAsync routing)
  • src/Api/Auth/Controllers/EmergencyAccessController.cs (Password dispatch)
  • src/Api/Auth/Models/Request/Accounts/PasswordRequestModel.cs, ChangeKdfRequestModel.cs, UpdateTdeOffboardingPasswordRequestModel.cs, UpdateTempPasswordRequestModel.cs
  • src/Api/Auth/Models/Request/EmergencyAccessRequestModels.cs
  • src/Core/Utilities/MasterPasswordPayloadVariantValidator.cs
  • src/Core/Auth/UserFeatures/UserMasterPassword/SelfServicePasswordChangeCommand.cs
  • src/Core/Auth/UserFeatures/TempPassword/ReplaceAdminSetTemporaryPasswordCommand.cs
  • src/Core/Auth/UserFeatures/UserMasterPassword/FinishSsoJitProvisionMasterPasswordCommand.cs
  • src/Core/Auth/UserFeatures/UserMasterPassword/TdeSetPasswordCommand.cs
  • src/Core/Auth/UserFeatures/TdeOffboardingPassword/TdeOffboardingPasswordCommand.cs
  • src/Core/Auth/UserFeatures/EmergencyAccess/EmergencyAccessService.cs (FinishRecoveryTakeoverAsync)
  • src/Core/Auth/UserFeatures/UserMasterPassword/MasterPasswordService.cs and Data/SetInitialPasswordData.cs
  • src/Core/Auth/UserFeatures/UserMasterPassword/Interfaces/IMasterPasswordService.cs (visibility change)
  • src/Core/Auth/UserFeatures/UserServiceCollectionExtensions.cs (DI registrations)
  • src/Core/Services/IUserService.cs and Implementations/UserService.cs (Obsolete markings)
  • New unit tests: SelfServicePasswordChangeCommandTests, ReplaceAdminSetTemporaryPasswordCommandTests, TdeSetPasswordCommandTests, MasterPasswordPayloadVariantValidatorTests, request-model validation tests
  • Updated integration tests: Api.IntegrationTest/Controllers/AccountsControllerTest.cs

Verification Notes

  • Task.Delay(2000) timing-attack mitigation preserved in AccountsController.PostPassword (legacy branch).
  • Removed inline user.HasMasterPassword() and salt-validation checks in TDE/SSO commands are still enforced inside MasterPasswordService via SetInitialPasswordData.ValidateDataForUser and BuildUpdateUserDelegateSetInitialMasterPassword.
  • IMasterPasswordService visibility change from internal to public is required for cross-assembly consumption from Bit.Api.
  • No new external package dependencies; no AppSec dependency-approval gate needed.
  • Test plan in PR description is comprehensive and includes manual verification across all five flows.

@sonarqubecloud
Copy link
Copy Markdown

sonarqubecloud Bot commented May 4, 2026

@enmande enmande marked this pull request as ready for review May 4, 2026 15:44
@enmande enmande requested a review from a team as a code owner May 4, 2026 15:44
@enmande enmande requested a review from ike-kottlowski May 4, 2026 15:44
@codecov
Copy link
Copy Markdown

codecov Bot commented May 4, 2026

Codecov Report

❌ Patch coverage is 64.77273% with 124 lines in your changes missing coverage. Please review.
✅ Project coverage is 59.62%. Comparing base (5ae8570) to head (b2c9a70).
⚠️ Report is 5 commits behind head on main.

Files with missing lines Patch % Lines
src/Api/Auth/Controllers/AccountsController.cs 17.02% 38 Missing and 1 partial ⚠️
...fboardingPassword/TdeOffboardingPasswordCommand.cs 15.90% 35 Missing and 2 partials ⚠️
...Features/EmergencyAccess/EmergencyAccessService.cs 5.88% 32 Missing ⚠️
.../Api/Auth/Controllers/EmergencyAccessController.cs 0.00% 15 Missing ⚠️
...assword/ReplaceAdminSetTemporaryPasswordCommand.cs 96.29% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@           Coverage Diff            @@
##             main    #7575    +/-   ##
========================================
  Coverage   59.62%   59.62%            
========================================
  Files        2097     2101     +4     
  Lines       92611    92914   +303     
  Branches     8249     8295    +46     
========================================
+ Hits        55215    55397   +182     
- Misses      35443    35562   +119     
- Partials     1953     1955     +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.

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