Skip to content

[PM-35394] MasterPasswordService Admin Console Integration#7629

Open
enmande wants to merge 13 commits into
auth/pm-35393/master-password-service-auth-integrationfrom
auth/pm-35394/master-password-service-ac-integration
Open

[PM-35394] MasterPasswordService Admin Console Integration#7629
enmande wants to merge 13 commits into
auth/pm-35393/master-password-service-auth-integrationfrom
auth/pm-35394/master-password-service-ac-integration

Conversation

@enmande
Copy link
Copy Markdown
Contributor

@enmande enmande commented May 13, 2026

🎟️ Tracking

PM-35394

📔 Objective

Extend the AdminConsole account-recovery flow to accept the new master password payload shape (UnlockData + AuthenticationData) alongside the existing (NewMasterPasswordHash + Key), wiring the new path through the centralized MasterPasswordService.PrepareSetInitialOrUpdateExistingMasterPasswordAsync tier (source).

Per-request availability of an acceptable payload loadout (hash + key, or authenticationdata + unlockdata) is boundary-enforced at the request-model layer; both the v1 and v2 command paths handle either variant, and OrganizationUsersController.PutRecoverAccount dispatches accordingly so clients that have not yet upgraded continue to work while the in-flight updates roll out.

Part of the PM-33011 story-of-stories to route all password set/change/rotate flows through MasterPasswordService. Depends on PM-35393.

📓 This is currently stacked on #7575 which is in QA.

📸 Screenshots

Admin Account Recovery (password only) with reset-two-factor-account-recovery 🎏

ℹ️ This illustrates flow pathing when the pm-15489-reset-two-factor-account-recovery flag is on.
zed is a just-provisioned user in a TDE encryption org.
Admin enmande recovers zed's account.

  • New Master Password is successfully set
  • Default KDF settings are applied (in range)
  • zed is required to update the password on next login.
PM-35394__tde-takeover-admin-reset-two-factor-on.mov

Admin Account Recovery (2FA only) with reset-two-factor-account-recovery 🎏

ℹ️ This illustrates flow pathing when the pm-15489-reset-two-factor-account-recovery flag is on.
zed is an organization user with 2FA (authenticator).
Admin enmande recovers zed's 2FA.

  • 2FA is successfully reset, no change to Master Password.
PM-35394__recover-2fa-only.mov

Admin Account Recovery (Master Password + 2FA) with reset-two-factor-account-recovery 🎏

ℹ️ This illustrates flow pathing when the pm-15489-reset-two-factor-account-recovery flag is on.
zed is a user in a TDE encryption org with 2FA (authenticator).
Admin enmande recovers zed's account with Master Password + 2FA.

  • 2FA is successfully reset
  • New Master Password is successfully set
  • zed is required to update the password on next login.
PM-35394__recover-password-and-2fa.mov

Admin Account Recovery

ℹ️ This illustrates flow pathing when the pm-15489-reset-two-factor-account-recovery flag is off.
zed is a just-provisioned user in a TDE encryption org.
Admin enmande recovers zed's account.

  • New Master Password is successfully set
  • Default KDF settings are applied (in range)
  • zed is required to update the password on next login.
PM-35394__tde-recover-admin-reset-two-factor-off.mov

@enmande enmande added needs-qa ai-review Request a Claude code review labels May 13, 2026
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 13, 2026

🤖 Bitwarden Claude Code Review

Overall Assessment: APPROVE

This PR extends the AdminConsole account-recovery flow to accept the new UnlockData + AuthenticationData payload variant alongside the legacy NewMasterPasswordHash + Key shape, and routes the new path through the centralized MasterPasswordService.PrepareSetInitialOrUpdateExistingMasterPasswordAsync. Variant presence is enforced upstream at the request-model layer, and both the v1 and v2 command paths handle either variant so clients can roll out gradually. The two previously-raised CRITICAL/IMPORTANT findings (presence-check gated on ResetMasterPassword; four tests missing ResetMasterPassword = true) appear addressed by commits 305b993 and cc8b68b.

Code Review Details

No new findings.

Verified during this pass:

  • OrganizationUserResetPasswordRequestModel.Validate now short-circuits when !ResetMasterPassword, so 2FA-only requests under the v2 (flag-on) path are no longer rejected by model validation.
  • The four impacted tests in OrganizationUserResetPasswordRequestModelTests.cs (Validate_UnlockAndAuthenticationDataOnly_NoErrors, Validate_UnlockAndAuthenticationDataOnly_WithMismatchedKdfSettings_ReturnsKdfValidationError, Validate_OnlyOneOfUnlockAndAuthenticationData_ReturnsError, Validate_WhenBothAuthAndUnlockPresent_WithBelowMinimumKdf_NoError) all set ResetMasterPassword = true.
  • IValidatableObject errors surface at the controller boundary via the existing ModelStateValidationFilterAttribute, so the bang-asserted dereferences in PutRecoverAccount are safe under the validator's contract.
  • MasterPasswordService.PrepareSetInitialOrUpdateExistingMasterPasswordAsync routes set-initial vs update-existing internally and sets revision/last-password-change timestamps on both branches; v2's new-payload branch correctly omits the manual timestamp updates that the legacy branch retains.
  • The KDF/salt agreement check at the request-model layer uses ValidateKdfAndSaltAgreement (no range enforcement), aligned with admin-recovery's documented contract that range is enforced upstream at registration/KDF change.
  • Duplicated guard logic in the v1 AdminRecoverAccountCommand overloads and dual dispatch in v2 are transitional and tied to PM-33141 cleanup; not flagged.

Comment thread src/Api/Models/Request/Organizations/OrganizationUserResetPasswordRequestModel.cs Outdated
@codecov
Copy link
Copy Markdown

codecov Bot commented May 13, 2026

Codecov Report

❌ Patch coverage is 88.39286% with 13 lines in your changes missing coverage. Please review.
✅ Project coverage is 59.90%. Comparing base (eeb3dd5) to head (cc8b68b).

Files with missing lines Patch % Lines
...ures/AccountRecovery/AdminRecoverAccountCommand.cs 70.00% 8 Missing and 4 partials ⚠️
...tions/OrganizationUserResetPasswordRequestModel.cs 96.29% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@                                    Coverage Diff                                     @@
##           auth/pm-35393/master-password-service-auth-integration    #7629      +/-   ##
==========================================================================================
+ Coverage                                                   59.87%   59.90%   +0.02%     
==========================================================================================
  Files                                                        2124     2124              
  Lines                                                       93469    93566      +97     
  Branches                                                     8307     8326      +19     
==========================================================================================
+ Hits                                                        55965    56048      +83     
- Misses                                                      35527    35535       +8     
- Partials                                                     1977     1983       +6     

☔ 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

@enmande enmande changed the title Auth/pm 35394/master password service ac integration [PM-35394] MasterPasswordService Admin Console Integration May 13, 2026
@enmande enmande marked this pull request as ready for review May 13, 2026 20:28
@enmande enmande requested a review from a team as a code owner May 13, 2026 20:28
@enmande enmande requested a review from BTreston May 13, 2026 20:28
Copy link
Copy Markdown
Contributor

@BTreston BTreston left a comment

Choose a reason for hiding this comment

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

LGTM, non blocking comments. I'll have another look when its ready for main

IMasterPasswordService masterPasswordService,
TimeProvider timeProvider) : IAdminRecoverAccountCommand
{
public async Task<IdentityResult> RecoverAccountAsync(Guid orgId,
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.

⛏️ Can we also [Obsolete] this?

@@ -21,4 +22,20 @@ public interface IAdminRecoverAccountCommand
/// <exception cref="NotFoundException">When the user does not exist.</exception>
Task<IdentityResult> RecoverAccountAsync(Guid orgId, OrganizationUser organizationUser,
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.

⛏️ same [Obsolete] here

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 needs-qa

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants