[PM-41272] Extract organization authorization into a new library - #8095
[PM-41272] Extract organization authorization into a new library#8095Hinton wants to merge 3 commits into
Conversation
|
@eliykat This is just a quick extraction as we discussed briefly in DM. If you want to tweak something, feel free to go ahead and do it directly on this branch. I think since it only moves files we shouldn't need to do any QA testing prior to merge? |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8095 +/- ##
===========================================
+ Coverage 14.99% 62.87% +47.88%
===========================================
Files 1409 2302 +893
Lines 61097 100219 +39122
Branches 4864 9019 +4155
===========================================
+ Hits 9161 63015 +53854
+ Misses 51780 35020 -16760
- Partials 156 2184 +2028 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| ```csharp | ||
| endpoints.MapGroup("/organizations/{orgId:guid}/access-rules") | ||
| .RequireAuthorization(policy => policy.AddRequirements(new ManageAccessRulesRequirement())) | ||
| .MapAccessRuleEndpoints(); | ||
| ``` |
There was a problem hiding this comment.
Non-blocking but I'm interested in feedback (including @justindbaur): we currently expose these as authorization requirements rather than authorization policies, because (a) I dislike magic strings that policies use as identifiers, and (b) there is only 1 requirement per endpoint, whereas a policy represents a collection of requirements + authentication schemes.
However, it looks like RequireAuthorization only accepts policy strings, or policies, or a policy builder callback, all of which is a bit verbose. We could instead package up each requirement into its own authorization policy:
public static class OrganizationPolicy
{
public static AuthorizationPolicy ManageAccessRules
=> new([new ManageAccessRulesRequirement()], []); // let the consumer dictate the authentication scheme
}And then usage becomes:
endpoints.MapGroup("/organizations/{orgId:guid}/access-rules")
.RequireAuthorization(OrganizationPolicy.ManageAccessRules)
.MapAccessRuleEndpoints();which I think is much nicer for consumers and seems to be what ASP.NET wants you to do. The static class also makes the policies easy to find.
PS. "policy" is unfortunate here because we also have a policy domain, but if you're importing from the OrganizationAuthorization library hopefully that context is clear.
There was a problem hiding this comment.
We could call it OrganizationAuthorizationPolicy?
There was a problem hiding this comment.
It seems we can do:
endpoints.MapGroup("/organizations/{orgId:guid}/access-rules")
.RequireAuthorization(new AuthorizeAttribute<ManageAccessRulesRequirement>())
.MapAccessRuleEndpoints();Might be good enough?
🤖 Bitwarden Claude Code ReviewOverall Assessment: APPROVE Reviewed the extraction of organization/provider authorization plumbing from Code Review DetailsNo blocking findings. Notes considered and deliberately not raised as findings:
|
…horization The organization and provider requirement plumbing lived in src/Api, so hosts that cannot reference Api could not use it. Pam is the immediate case: Api already references Pam, so the dependency cannot go the other way, and its endpoint handlers were left doing imperative ICurrentContext checks instead of authorizing through the middleware. Core is not an option here — these types read user claims and depend on how the host authenticates, which is why IOrganizationContext carries an explicit "do not move this into Core" note. A library is the sanctioned alternative, and src/Libraries already has the shape (LIBRARY.md, ADR-0031, ADR-0032) plus two precedents in SsrfProtection and HttpExtensions. What moved: - IOrganizationRequirement and its handler, OrganizationContext, OrganizationClaimsExtensions, and the five organization requirements - The Providers tree — same plumbing, same dependencies - AuthorizeAttribute<T> and HttpContextExtensions What deliberately stayed in Api: Collections, the resource-based handlers that authorize over specific Api domain models, and NoopAuthorizeAttribute (an Api controller convention). The namespaces are unchanged, following the SsrfProtection and Data precedent, so none of the 173 call sites in Api needed touching. Two things worth reviewing: - The handlers and OrganizationContext no longer take IUserService just to call GetProperUserId. UserClaimsExtensions.GetUserId reads the sub claim instead, which is what AddCustomIdentityServices configures UserIdClaimType to and what CurrentContext already reads directly. Each call site's null behaviour is preserved. This is the only behavioural change. - The library sits above Core rather than below it. README.md records every borrowed Core type as debt, per LIBRARY.md. Pam is not wired up here — that belongs with the endpoint changes that consume it, so this stays a pure extraction that can merge on its own.
- Keep IUserService rather than reading the sub claim directly. It's in Core, which this library already depends on, so there's nothing to gain by avoiding it. Every moved file is now byte-identical to its original apart from the Microsoft.AspNetCore.* usings that Api got implicitly from the Web SDK. - Trim the DI comment down to what's relevant to Api. - Drop the trailing explanation from the CODEOWNERS entry. - Record IUserService in the README's Core debt table, and note that depending on Core is the accepted position for now.
Reuses the same requirement-binding mechanism controllers already use via [Authorize<T>], instead of a policy-builder callback, avoiding a named policy. Add a TestHost-backed regression test proving the combined requirement is enforced end-to-end.
b52b8b7 to
51dae70
Compare
Hinton
changed the title
eliykat
left a comment
eliykat
left a comment
There was a problem hiding this comment.
Also a good solution, thanks!
| } | ||
| } | ||
|
|
||
| private static async Task<IHost> BuildHostAsync() => |
3 participants
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-41272
📔 Objective
Move the generic organization/provider requirement plumbing out of
src/Api/AdminConsole/Authorizationand into a newsrc/Libraries/OrganizationAuthorization, so any host can authorize organization-scoped endpoints through the authorization middleware instead of hand-rollingICurrentContextchecks inside handlers.What moved
IOrganizationRequirement+OrganizationRequirementHandler,OrganizationContext,OrganizationClaimsExtensions, and the five organization requirementsProviders/tree — identical plumbing, identical dependencies, no reason to split itAuthorizeAttribute<T>andHttpContextExtensionsWhat deliberately stayed in Api
Collections/*, the resource-based handlers (RecoverAccountAuthorizationHandler,OrgUserLinkedToUserIdHandler,OrganizationCollectionManagementAccessHandler), andNoopAuthorizeAttribute. Those authorize over specific Api domain models rather than over the organization or provider on the route, and have no non-Api consumers.This is a pure move
Following the
SsrfProtectionandDataprecedent, the extracted types keep their existing namespaces. None of the 173 call sites inApineeded touching.Every moved file is byte-identical to its original except for three added
Microsoft.AspNetCore.*usings, whichApiwas getting implicitly from the Web SDK:HttpContextExtensions.csMicrosoft.AspNetCore.Http,Microsoft.AspNetCore.RoutingOrganizations/OrganizationRequirementHandler.csMicrosoft.AspNetCore.HttpProviders/ProviderRequirementHandler.csMicrosoft.AspNetCore.HttpAll 13 moved test files are pure moves with no edits at all. The only genuinely new code is
OrganizationAuthorizationServiceCollectionExtensions(the library'sAddFooentry point per LIBRARY.md), the two.csprojfiles, andREADME.md.Renaming the namespaces to
Bit.OrganizationAuthorizationis a follow-up we can take whenever it suits, independent of this move.👀 One thing worth a look
The library sits above
Core, not below it. LIBRARY.md wants new libraries below Core, soREADME.mdrecords every borrowed Core type. Per discussion below, depending on Core is the accepted position for now — unwinding it would mean extracting the Admin Console data models, identity, and organizations, which is its own effort. The table is there so those pieces are known, not because they are queued up.