Skip to content

[PM-34883] - Add InjectOrganizationUserAttribute#7536

Merged
jrmccannon merged 3 commits intomainfrom
jmccannon/ac/pm-34883-org-membership-attr
Apr 28, 2026
Merged

[PM-34883] - Add InjectOrganizationUserAttribute#7536
jrmccannon merged 3 commits intomainfrom
jmccannon/ac/pm-34883-org-membership-attr

Conversation

@jrmccannon
Copy link
Copy Markdown
Contributor

@jrmccannon jrmccannon commented Apr 23, 2026

🎟️ Tracking

PM-34883

📔 Objective

This adds an attribute that can be used to decorate api endpoints and allow for the target organizationUser (via the route)
to be validated in a more standardized way. This will pull the id out of the route (id or whatever string is provided via the constructor) and will validate the target OrganizationUser belongs to the organization from the route. Since we've fetched the org user already, we'll provide it to the controller method so we don't have to get it again.

I've implemented this on organization/{orgId}/users/{id}/recover-account.

@jrmccannon jrmccannon requested a review from a team as a code owner April 23, 2026 15:01
@jrmccannon jrmccannon requested a review from eliykat April 23, 2026 15:02
@jrmccannon jrmccannon added the ai-review Request a Claude code review label Apr 23, 2026
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Apr 23, 2026

🤖 Bitwarden Claude Code Review

Overall Assessment: APPROVE

This PR introduces InjectOrganizationUserAttribute and the accompanying OrganizationUserModelBinder to standardize loading and org-membership validation of an OrganizationUser from a route parameter, and applies it to the account-recovery endpoints. The change is consistent with the prior reviewer's preference to replace the dual [InjectOrganization]/[BindNever] Billing pattern with a custom model binder, includes a full set of binder unit tests, and correctly removes the now-redundant controller tests for the not-found / org-mismatch paths. Authorization ([Authorize<ManageAccountRecoveryRequirement>]) still runs before the binder, and BadRequestException/NotFoundException are translated to 400/404 by the existing ExceptionHandlerFilterAttribute. No security, correctness, or breaking-change concerns surfaced.

Code Review Details

No findings.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Apr 23, 2026

Logo
Checkmarx One – Scan Summary & Details8fe26b4a-837d-426f-8c32-9b1b5e0c1cd6


New Issues (3) Checkmarx found the following issues in this Pull Request
# Severity Issue Source File / Package Checkmarx Insight
1 MEDIUM CSRF src/Api/AdminConsole/Controllers/OrganizationUsersController.cs: 514
detailsMethod at line 514 of /src/Api/AdminConsole/Controllers/OrganizationUsersController.cs gets a parameter from a user request from model. This par...
Attack Vector
2 MEDIUM CSRF src/Api/Vault/Controllers/CiphersController.cs: 1558
detailsMethod at line 1558 of /src/Api/Vault/Controllers/CiphersController.cs gets a parameter from a user request from id. This parameter value flows ...
Attack Vector
3 MEDIUM CSRF src/Api/Vault/Controllers/CiphersController.cs: 1385
detailsMethod at line 1385 of /src/Api/Vault/Controllers/CiphersController.cs gets a parameter from a user request from id. This parameter value flows ...
Attack Vector

Fixed Issues (1) Great job! The following issues were fixed in this Pull Request
Severity Issue Source File / Package
MEDIUM CSRF src/Api/AdminConsole/Controllers/OrganizationUsersController.cs: 512

@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 23, 2026

Codecov Report

❌ Patch coverage is 93.10345% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 59.26%. Comparing base (53dc0c4) to head (a78389f).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
...sole/Attributes/InjectOrganizationUserAttribute.cs 92.85% 0 Missing and 2 partials ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main    #7536   +/-   ##
=======================================
  Coverage   59.25%   59.26%           
=======================================
  Files        2081     2082    +1     
  Lines       92036    92060   +24     
  Branches     8179     8181    +2     
=======================================
+ Hits        54534    54556   +22     
  Misses      35563    35563           
- Partials     1939     1941    +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.

Copy link
Copy Markdown
Member

@eliykat eliykat 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, I've left some feedback below to explore/consider, but this approach will work just fine as well.

Comment thread src/Api/AdminConsole/Controllers/OrganizationUsersController.cs Outdated
Comment thread src/Api/AdminConsole/Attributes/InjectOrganizationUserAttribute.cs Outdated
@jrmccannon jrmccannon requested a review from eliykat April 27, 2026 19:51
@sonarqubecloud
Copy link
Copy Markdown

@jrmccannon jrmccannon merged commit 9c02f0c into main Apr 28, 2026
40 checks passed
@jrmccannon jrmccannon deleted the jmccannon/ac/pm-34883-org-membership-attr branch April 28, 2026 18:52
jaasen-livefront pushed a commit that referenced this pull request Apr 29, 2026
* Added InjectOrganizationUserAttribute and updated account-recovery put to use it.

* Changes from code review
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.

2 participants