Skip to content

[PM-32594] Add authorization to admin-initiated sponsorship endpoints#7095

Open
cturnbull-bitwarden wants to merge 4 commits intomainfrom
billing/PM-32594-sponsorships-revoke-vulnerability
Open

[PM-32594] Add authorization to admin-initiated sponsorship endpoints#7095
cturnbull-bitwarden wants to merge 4 commits intomainfrom
billing/PM-32594-sponsorships-revoke-vulnerability

Conversation

@cturnbull-bitwarden
Copy link
Contributor

🎟️ Tracking

https://bitwarden.atlassian.net/browse/PM-32594

📔 Objective

Adds authorization checks to the AdminInitiatedRevokeSponsorshipAsync and ResendSponsorshipOffer endpoints in both the cloud and self-hosted sponsorship controllers. Previously, these endpoints only required [Authorize("Application")], meaning any authenticated user could call them regardless of their relationship to the
sponsoring organization.

Now uses [Authorize<ManageUsersRequirement>] to enforce that the caller is an Owner, Admin, or has the ManageUsers permission in the sponsoring organization — consistent with the existing GetSponsoredOrganizations endpoint and CreateSponsorshipCommand validation.

Route parameters on the affected endpoints were renamed from {sponsoringOrgId} to {organizationId} to work with the existing authorization handler infrastructure. The C# parameter names remain unchanged.

@cturnbull-bitwarden cturnbull-bitwarden added the ai-review Request a Claude code review label Feb 26, 2026
@claude
Copy link
Contributor

claude bot commented Feb 26, 2026

Claude finished @cturnbull-bitwarden's task in 2m 20s —— View job


Pull Request Review: PM-32594 Add authorization to admin-initiated sponsorship endpoints

Summary

This PR fixes a security vulnerability (VULN-441) where the AdminInitiatedRevokeSponsorshipAsync and ResendSponsorshipOffer endpoints lacked proper organization-level authorization. Previously, any authenticated user could call these endpoints regardless of their relationship to the sponsoring organization.

Changes Reviewed

File Type Assessment
src/Api/Billing/Controllers/OrganizationSponsorshipsController.cs Security fix Correct
src/Api/Controllers/SelfHosted/SelfHostedOrganizationSponsorshipsController.cs Security fix Correct
test/Api.IntegrationTest/Billing/Controllers/OrganizationSponsorshipsControllerTests.cs Tests Thorough

Analysis

Security fix is correct. The [Authorize<ManageUsersRequirement>] attribute leverages the existing OrganizationRequirementHandler, which extracts the organization ID from the route and verifies the caller is an Owner, Admin, Custom user with ManageUsers permission, or a Provider user for that specific organization. This prevents both unauthorized access and cross-organization attacks.

Route parameter renaming is necessary. The rename from {sponsoringOrgId} to {organizationId} is required because HttpContextExtensions.GetOrganizationId() only recognizes route values named orgId or organizationId. The [FromRoute(Name = "organizationId")] binding preserves the existing C# parameter names, and the actual URL structure remains unchanged for API consumers -- no breaking change.

Both deployment targets are covered. The cloud controller (OrganizationSponsorshipsController) and the self-hosted controller (SelfHostedOrganizationSponsorshipsController) are both updated. The self-hosted controller already has [Authorize("Application")] at the class level, so only the [Authorize<ManageUsersRequirement>] attribute was needed at the method level.

Test coverage is thorough. The 13 new integration tests cover both endpoints across all relevant authorization scenarios: non-member (forbidden), regular member without ManageUsers (forbidden), owner (allowed), admin (allowed), custom user with ManageUsers (allowed), and cross-organization attack (forbidden).

Verdict

No issues found. This is a focused, well-implemented security fix with excellent test coverage.


Reviewed by Claude (Anthropic) | Model: Opus 4.6

@cturnbull-bitwarden cturnbull-bitwarden removed the ai-review Request a Claude code review label Feb 26, 2026
@cturnbull-bitwarden cturnbull-bitwarden marked this pull request as ready for review February 26, 2026 21:47
@cturnbull-bitwarden cturnbull-bitwarden requested a review from a team as a code owner February 26, 2026 21:47
@github-actions
Copy link
Contributor

github-actions bot commented Feb 26, 2026

Logo
Checkmarx One – Scan Summary & Details734f98d5-1c2a-4483-9ae9-356ca712bebe

Great job! No new security vulnerabilities introduced in this pull request

Copy link
Contributor

@kdenney kdenney left a comment

Choose a reason for hiding this comment

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

Looks good; just one question to confirm first.

@codecov
Copy link

codecov bot commented Feb 27, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 56.52%. Comparing base (0c9b97b) to head (53a7d7b).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #7095      +/-   ##
==========================================
+ Coverage   56.45%   56.52%   +0.07%     
==========================================
  Files        2013     2013              
  Lines       88112    88112              
  Branches     7863     7863              
==========================================
+ Hits        49742    49804      +62     
+ Misses      36554    36487      -67     
- Partials     1816     1821       +5     

☔ 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

sonarqubecloud bot commented Mar 2, 2026

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants