Skip to content

[PM-40300]: Restrict Provider Users' Account Recovery - #8103

Open
sven-bitwarden wants to merge 1 commit into
mainfrom
ac/pm-40300/fix-provider-vuln
Open

[PM-40300]: Restrict Provider Users' Account Recovery#8103
sven-bitwarden wants to merge 1 commit into
mainfrom
ac/pm-40300/fix-provider-vuln

Conversation

@sven-bitwarden

@sven-bitwarden sven-bitwarden commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

🎟️ Tracking

PM-40300

@sven-bitwarden
sven-bitwarden requested a review from a team as a code owner July 30, 2026 19:20
@github-actions

github-actions Bot commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

🤖 Bitwarden Claude Code Review

Overall Assessment: APPROVE

This bugfix closes two account recovery privilege-escalation paths: it extends RecoverAccountAuthorizationHandler so a provider Service User can no longer act on a Provider Admin in their own provider, and it applies the same per-target authorization to the two endpoints that return ResetPasswordKey material ({id}/reset-password-details and account-recovery-details), which previously only gated on ManageAccountRecoveryRequirement. I verified the claims semantics of ICurrentContext.ProviderUser/ProviderProviderAdmin, confirmed the checks fail closed for non-confirmed provider memberships and for targets with no linked user, and confirmed no other endpoint or response model exposes ResetPasswordKey. Unit and integration coverage matches the behavior changes on both the authorized and unauthorized paths.

Code Review Details
  • 🎨 : Bulk authorization issues one ProviderUser query per member (N+1); GetManyByManyUsersAsync would collapse it to a single query
    • src/Api/AdminConsole/Controllers/OrganizationUsersController.cs:288

Notes considered and not raised as findings: returning NotFoundException instead of the handler's failure reason is the correct non-disclosing response here, and the bulk endpoint silently omitting unauthorized users is consistent with the pre-existing behavior of GetManyAccountRecoveryDetailsByOrganizationUserAsync, which already returns partial results for members without linked users.

@sven-bitwarden sven-bitwarden added the t:bugfix Change Type - Bugfix label Jul 30, 2026
Comment thread src/Api/AdminConsole/Controllers/OrganizationUsersController.cs Dismissed
@sven-bitwarden sven-bitwarden added the ai-review-vnext Request a Claude code review using the vNext workflow label Jul 30, 2026
Comment thread src/Api/AdminConsole/Controllers/OrganizationUsersController.cs
@codecov

codecov Bot commented Jul 30, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 62.74%. Comparing base (561bf01) to head (9d3ef0e).
⚠️ Report is 2 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #8103      +/-   ##
==========================================
+ Coverage   62.71%   62.74%   +0.02%     
==========================================
  Files        2298     2298              
  Lines      100115   100151      +36     
  Branches     9011     9015       +4     
==========================================
+ Hits        62785    62835      +50     
+ Misses      35147    35134      -13     
+ Partials     2183     2182       -1     

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

}

private async Task<bool> CanRecoverAccountAsync(OrganizationUser organizationUser) =>
(await _authorizationService.AuthorizeAsync(

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.

Have you seen BulkAuthorizationHandler (here) and an example of its implementation ? This might an avenue to investigate.

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 t:bugfix Change Type - Bugfix

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants