From 0356bd48f8b722f41c69267b8c81e951990bf06c Mon Sep 17 00:00:00 2001 From: Hinton Date: Fri, 7 Aug 2026 12:16:39 +0200 Subject: [PATCH] [PM-40211] Authorize Access Rule endpoints with IOrganizationRequirement MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Declarative authorization on the access-rule routes (ADR-0022): the group requires organization membership, and writes additionally require authority over rule authorship via the new ManageAccessRulesRequirement. ASP.NET combines the group and endpoint policies, so a write has to satisfy both. Deviation from the story: no named-policy bridge. The story specifies policy-name constants in Policies.cs registered in Api/Startup.cs, because Pam.csproj cannot reference Api and so could not reach AuthorizeAttribute. PM-41272 has since extracted that authorization code into src/Libraries/OrganizationAuthorization, which depends only on Core — so Pam references the library directly and attaches AuthorizeAttribute to the group and to the write routes. Policies.cs and Startup.cs are untouched. The requirements are carried as endpoint metadata, which AuthorizationMiddleware combines with the group-level Policies.Application rather than replacing it; a test asserts both are present on every route. OrganizationRequirementHandler resolves the organization from the {orgId:guid} group prefix and is already DI-registered by AddOrganizationAuthorization. Providers are excluded from the resource entirely. The group gate is deliberately MemberRequirement and not MemberOrProviderRequirement: providers manage an organization's billing and configuration, but access rules gate who can lease credentials out of it, which is not theirs to read or change. That gate is load-bearing rather than decorative — ManageAccessRulesRequirement derives from BasePermissionRequirement, whose final arm authorizes any provider for the organization, so the write routes would admit providers on the permission alone. Two tests pin this: every write carries MemberRequirement alongside the permission, and no access-rule route carries MemberOrProviderRequirement. Scope: this covers the Access Rule half of the story only. The remaining half — the usage gate on access-request submission, which must check both the member's AccessPam and the organization's UsePam, plus the CipherLeaseGate parity decision — cannot land yet. SubmitAccessRequestCommand does not exist on main: the access-request and cipher-lease handlers are still NotImplementedException scaffolds, so there is no submission path to gate. That gate belongs with the request/lease behaviour slices. For the same reason there are no imperative EnsureMemberAsync/EnsureAdminAsync checks to delete — AccessRuleEndpointsHandler never had them on main. Note the observable change the story flags (breakdown review note 8) holds here: unauthorized calls now return 403 from the authorization middleware. --- .../Pam/Api/Endpoints/AccessRuleEndpoints.cs | 28 ++++++- bitwarden_license/src/Services/Pam/Pam.csproj | 1 + .../src/Services/Pam/packages.lock.json | 6 ++ .../Api/Endpoints/AccessRuleEndpointsTests.cs | 78 ++++++++++++++++++- .../test/Services/Pam.Test/packages.lock.json | 9 ++- src/Api/packages.lock.json | 3 +- .../Requirements/PermissionRequirements.cs | 1 + test/Api.IntegrationTest/packages.lock.json | 3 +- test/Api.Test/packages.lock.json | 3 +- .../packages.lock.json | 3 +- .../PermissionRequirementsTests.cs | 1 + 11 files changed, 126 insertions(+), 10 deletions(-) diff --git a/bitwarden_license/src/Services/Pam/Api/Endpoints/AccessRuleEndpoints.cs b/bitwarden_license/src/Services/Pam/Api/Endpoints/AccessRuleEndpoints.cs index d5cc8aaa2f1b..4c47f38ef87a 100644 --- a/bitwarden_license/src/Services/Pam/Api/Endpoints/AccessRuleEndpoints.cs +++ b/bitwarden_license/src/Services/Pam/Api/Endpoints/AccessRuleEndpoints.cs @@ -1,4 +1,6 @@ -using Bit.Services.Pam.Api.Endpoints.Handlers; +using Bit.Api.AdminConsole.Authorization; +using Bit.Api.AdminConsole.Authorization.Requirements; +using Bit.Services.Pam.Api.Endpoints.Handlers; using Bit.Services.Pam.Api.Models.Request; namespace Bit.Services.Pam.Api.Endpoints; @@ -7,11 +9,26 @@ namespace Bit.Services.Pam.Api.Endpoints; /// The organizations/{orgId}/access-rules resource: rule CRUD scoped to an organization. orgId is /// bound from the group's route prefix. /// +/// +/// Authorization runs in the middleware rather than the handler. The group requires organization membership and the +/// write endpoints additionally require ; ASP.NET combines the group and +/// endpoint policies, so a write has to satisfy both. Both requirements read orgId off the route, which the +/// group prefix supplies. +/// +/// The group requirement is deliberately and not +/// MemberOrProviderRequirement: providers manage an organization's billing and configuration, but access +/// rules gate who can lease credentials out of it, which is not theirs to change. Note this group gate is the only +/// thing keeping providers out — derives from +/// BasePermissionRequirement, which falls back to authorizing a provider for the organization. Removing or +/// weakening the group requirement would silently readmit them to the write endpoints. +/// +/// internal static class AccessRuleEndpoints { public static RouteGroupBuilder MapAccessRuleEndpoints(this RouteGroupBuilder group) { group.WithTags("AccessRules"); + group.RequireAuthorization(new AuthorizeAttribute()); group.MapGet("", (Guid orgId, AccessRuleEndpointsHandler handler) => handler.GetAll(orgId)) .WithName("Pam_AccessRules_GetAll"); @@ -20,10 +37,12 @@ public static RouteGroupBuilder MapAccessRuleEndpoints(this RouteGroupBuilder gr .WithName("Pam_AccessRules_Get"); group.MapPost("", (Guid orgId, AccessRuleRequestModel model, AccessRuleEndpointsHandler handler) => handler.Post(orgId, model)) - .WithName("Pam_AccessRules_Post"); + .WithName("Pam_AccessRules_Post") + .RequireAuthorization(new AuthorizeAttribute()); group.MapPut("{id:guid}", (Guid orgId, Guid id, AccessRuleRequestModel model, AccessRuleEndpointsHandler handler) => handler.Put(orgId, id, model)) - .WithName("Pam_AccessRules_Put"); + .WithName("Pam_AccessRules_Put") + .RequireAuthorization(new AuthorizeAttribute()); group.MapDelete("{id:guid}", async (Guid orgId, Guid id, AccessRuleEndpointsHandler handler) => @@ -31,7 +50,8 @@ public static RouteGroupBuilder MapAccessRuleEndpoints(this RouteGroupBuilder gr await handler.Delete(orgId, id); return TypedResults.NoContent(); }) - .WithName("Pam_AccessRules_Delete"); + .WithName("Pam_AccessRules_Delete") + .RequireAuthorization(new AuthorizeAttribute()); return group; } diff --git a/bitwarden_license/src/Services/Pam/Pam.csproj b/bitwarden_license/src/Services/Pam/Pam.csproj index 264ff0119b1a..e80fe52e7579 100644 --- a/bitwarden_license/src/Services/Pam/Pam.csproj +++ b/bitwarden_license/src/Services/Pam/Pam.csproj @@ -26,6 +26,7 @@ + diff --git a/bitwarden_license/src/Services/Pam/packages.lock.json b/bitwarden_license/src/Services/Pam/packages.lock.json index c77e4b8f25cd..8b97f591c54a 100644 --- a/bitwarden_license/src/Services/Pam/packages.lock.json +++ b/bitwarden_license/src/Services/Pam/packages.lock.json @@ -910,6 +910,12 @@ "httpextensions": { "type": "Project" }, + "organizationauthorization": { + "type": "Project", + "dependencies": { + "Core": "[2026.7.2, )" + } + }, "serilogfilelogging": { "type": "Project", "dependencies": { diff --git a/bitwarden_license/test/Services/Pam.Test/Api/Endpoints/AccessRuleEndpointsTests.cs b/bitwarden_license/test/Services/Pam.Test/Api/Endpoints/AccessRuleEndpointsTests.cs index 79d8ee82daef..a394786bf6f3 100644 --- a/bitwarden_license/test/Services/Pam.Test/Api/Endpoints/AccessRuleEndpointsTests.cs +++ b/bitwarden_license/test/Services/Pam.Test/Api/Endpoints/AccessRuleEndpointsTests.cs @@ -1,8 +1,11 @@ -using Bit.Core.Models.Api; +using Bit.Api.AdminConsole.Authorization.Requirements; +using Bit.Core.Auth.Identity; +using Bit.Core.Models.Api; using Bit.HttpExtensions; using Bit.Services.Pam.Api.Endpoints; using Bit.Services.Pam.Api.Endpoints.Handlers; using Bit.Services.Pam.Api.Models.Response; +using Microsoft.AspNetCore.Authorization; using Microsoft.AspNetCore.Builder; using Microsoft.AspNetCore.Http; using Microsoft.AspNetCore.Http.Metadata; @@ -84,6 +87,79 @@ public void AccessRuleGroup_DocumentsErrorResponseModel_For400And404() Assert.Contains(produces, p => p.StatusCode == StatusCodes.Status404NotFound && p.Type == typeof(ErrorResponseModel)); } + /// + /// Collects the authorization requirements an endpoint carries. They arrive as two shapes of metadata: + /// AuthorizeAttribute<T> contributes , while a policy + /// built inline contributes an . AuthorizationMiddleware combines both, so a + /// test asking "what must this endpoint satisfy" has to read both. + /// + private static List RequirementsFor(Endpoint endpoint) => + [ + .. endpoint.Metadata.GetOrderedMetadata().SelectMany(policy => policy.Requirements), + .. endpoint.Metadata.GetOrderedMetadata().SelectMany(data => data.GetRequirements()) + ]; + + [Theory] + [InlineData("Pam_AccessRules_GetAll", typeof(MemberRequirement))] + [InlineData("Pam_AccessRules_Get", typeof(MemberRequirement))] + [InlineData("Pam_AccessRules_Post", typeof(ManageAccessRulesRequirement))] + [InlineData("Pam_AccessRules_Put", typeof(ManageAccessRulesRequirement))] + [InlineData("Pam_AccessRules_Delete", typeof(ManageAccessRulesRequirement))] + public void MapPamEndpoints_AuthorizesRouteWithRequirement(string name, Type requirementType) + { + // Reads require membership; writes require authority over rule authorship. The requirements are carried as + // endpoint metadata, which AuthorizationMiddleware combines with the group's Policies.Application. + var endpoint = Assert.Single( + MaterializeEndpoints(), + e => e.Metadata.GetMetadata()?.EndpointName == name); + + Assert.Contains(RequirementsFor(endpoint), r => r.GetType() == requirementType); + + // The per-route requirement adds to the group's policy rather than replacing it. + Assert.Contains(endpoint.Metadata.GetOrderedMetadata(), + data => data.Policy == Policies.Application); + } + + [Fact] + public void MapPamEndpoints_AccessRuleWritesRequireMembershipBesidesThePermission() + { + // ManageAccessRulesRequirement derives from BasePermissionRequirement, whose final arm authorizes any + // provider for the organization. The group's MemberRequirement is the only thing keeping providers out of + // rule authorship, so each write must carry it *in addition to* the permission — the permission alone would + // admit them. + var writeRoutes = new[] { "Pam_AccessRules_Post", "Pam_AccessRules_Put", "Pam_AccessRules_Delete" }; + + var endpoints = MaterializeEndpoints() + .Where(e => writeRoutes.Contains(e.Metadata.GetMetadata()?.EndpointName)) + .ToList(); + + Assert.Equal(writeRoutes.Length, endpoints.Count); + Assert.All(endpoints, endpoint => + { + var requirements = RequirementsFor(endpoint); + Assert.Contains(requirements, r => r is MemberRequirement); + Assert.Contains(requirements, r => r is ManageAccessRulesRequirement); + }); + } + + [Fact] + public void MapPamEndpoints_AccessRulesNeverAuthorizeProvidersByMembership() + { + // Access rules gate who can lease credentials out of an organization, which is not a provider's to read or + // change. MemberOrProviderRequirement would let them in, so no access-rule route may carry it. + var endpoints = MaterializeEndpoints() + .Where(e => e.Metadata.GetMetadata()!.Tags.Contains("AccessRules")) + .ToList(); + + Assert.Equal(5, endpoints.Count); + Assert.All(endpoints, endpoint => + { + var requirements = RequirementsFor(endpoint); + Assert.Contains(requirements, r => r is MemberRequirement); + Assert.DoesNotContain(requirements, r => r is MemberOrProviderRequirement); + }); + } + [Theory] [InlineData(nameof(AccessRuleEndpointsHandler.GetAll), typeof(Task>))] [InlineData(nameof(AccessRuleEndpointsHandler.Get), typeof(Task))] diff --git a/bitwarden_license/test/Services/Pam.Test/packages.lock.json b/bitwarden_license/test/Services/Pam.Test/packages.lock.json index f9f6a0791440..0f5dd369d90b 100644 --- a/bitwarden_license/test/Services/Pam.Test/packages.lock.json +++ b/bitwarden_license/test/Services/Pam.Test/packages.lock.json @@ -1323,11 +1323,18 @@ "httpextensions": { "type": "Project" }, + "organizationauthorization": { + "type": "Project", + "dependencies": { + "Core": "[2026.7.2, )" + } + }, "pam": { "type": "Project", "dependencies": { "Core": "[2026.7.2, )", - "HttpExtensions": "[2026.7.2, )" + "HttpExtensions": "[2026.7.2, )", + "OrganizationAuthorization": "[0.0.1, )" } }, "serilogfilelogging": { diff --git a/src/Api/packages.lock.json b/src/Api/packages.lock.json index 5c8515b0a4fa..ec16c21fcdbc 100644 --- a/src/Api/packages.lock.json +++ b/src/Api/packages.lock.json @@ -1276,7 +1276,8 @@ "type": "Project", "dependencies": { "Core": "[2026.7.2, )", - "HttpExtensions": "[2026.7.2, )" + "HttpExtensions": "[2026.7.2, )", + "OrganizationAuthorization": "[0.0.1, )" } }, "serilogfilelogging": { diff --git a/src/Libraries/OrganizationAuthorization/Organizations/Requirements/PermissionRequirements.cs b/src/Libraries/OrganizationAuthorization/Organizations/Requirements/PermissionRequirements.cs index d50c74b971bb..81b2e1267dba 100644 --- a/src/Libraries/OrganizationAuthorization/Organizations/Requirements/PermissionRequirements.cs +++ b/src/Libraries/OrganizationAuthorization/Organizations/Requirements/PermissionRequirements.cs @@ -3,6 +3,7 @@ public class AccessEventLogsRequirement() : BasePermissionRequirement(p => p.AccessEventLogs); public class AccessImportExportRequirement() : BasePermissionRequirement(p => p.AccessImportExport); public class AccessReportsRequirement() : BasePermissionRequirement(p => p.AccessReports); +public class ManageAccessRulesRequirement() : BasePermissionRequirement(p => p.ManageAccessRules); public class ManageAccountRecoveryRequirement() : BasePermissionRequirement(p => p.ManageResetPassword); public class ManageGroupsRequirement() : BasePermissionRequirement(p => p.ManageGroups); public class ManagePoliciesRequirement() : BasePermissionRequirement(p => p.ManagePolicies); diff --git a/test/Api.IntegrationTest/packages.lock.json b/test/Api.IntegrationTest/packages.lock.json index fb98ea7ad115..f171c0e89520 100644 --- a/test/Api.IntegrationTest/packages.lock.json +++ b/test/Api.IntegrationTest/packages.lock.json @@ -1554,7 +1554,8 @@ "type": "Project", "dependencies": { "Core": "[2026.7.2, )", - "HttpExtensions": "[2026.7.2, )" + "HttpExtensions": "[2026.7.2, )", + "OrganizationAuthorization": "[0.0.1, )" } }, "rustsdk": { diff --git a/test/Api.Test/packages.lock.json b/test/Api.Test/packages.lock.json index 7c5ff5a4451c..d03c7f84177d 100644 --- a/test/Api.Test/packages.lock.json +++ b/test/Api.Test/packages.lock.json @@ -1892,7 +1892,8 @@ "type": "Project", "dependencies": { "Core": "[2026.7.2, )", - "HttpExtensions": "[2026.7.2, )" + "HttpExtensions": "[2026.7.2, )", + "OrganizationAuthorization": "[0.0.1, )" } }, "serilogfilelogging": { diff --git a/test/Billing.IntegrationTest/packages.lock.json b/test/Billing.IntegrationTest/packages.lock.json index 222098031e64..574c8151dd50 100644 --- a/test/Billing.IntegrationTest/packages.lock.json +++ b/test/Billing.IntegrationTest/packages.lock.json @@ -2099,7 +2099,8 @@ "type": "Project", "dependencies": { "Core": "[2026.7.2, )", - "HttpExtensions": "[2026.7.2, )" + "HttpExtensions": "[2026.7.2, )", + "OrganizationAuthorization": "[0.0.1, )" } }, "postgresmigrations": { diff --git a/test/Libraries/OrganizationAuthorization.Test/Organizations/Requirements/PermissionRequirementsTests.cs b/test/Libraries/OrganizationAuthorization.Test/Organizations/Requirements/PermissionRequirementsTests.cs index 1acfbd5be33b..0c8cd105249f 100644 --- a/test/Libraries/OrganizationAuthorization.Test/Organizations/Requirements/PermissionRequirementsTests.cs +++ b/test/Libraries/OrganizationAuthorization.Test/Organizations/Requirements/PermissionRequirementsTests.cs @@ -21,6 +21,7 @@ public class PermissionRequirementsTests new object[] { new AccessEventLogsRequirement(), nameof(Permissions.AccessEventLogs) }, new object[] { new AccessImportExportRequirement(), nameof(Permissions.AccessImportExport) }, new object[] { new AccessReportsRequirement(), nameof(Permissions.AccessReports) }, + new object[] { new ManageAccessRulesRequirement(), nameof(Permissions.ManageAccessRules) }, new object[] { new ManageAccountRecoveryRequirement(), nameof(Permissions.ManageResetPassword) }, new object[] { new ManageGroupsRequirement(), nameof(Permissions.ManageGroups) }, new object[] { new ManagePoliciesRequirement(), nameof(Permissions.ManagePolicies) },