diff --git a/db/migrations/20260725120000_add-rbac-indexes.sql b/db/migrations/20260725120000_add-rbac-indexes.sql new file mode 100644 index 00000000..6cdbb2d5 --- /dev/null +++ b/db/migrations/20260725120000_add-rbac-indexes.sql @@ -0,0 +1,7 @@ +-- 2026-07-25 12:00:00 : add-rbac-indexes + +CREATE INDEX IF NOT EXISTS "IX_RbacGroupMember_UserId" ON "RbacGroupMember" ("UserId"); +CREATE INDEX IF NOT EXISTS "IX_RbacPermissionGrants_AssignedEntityId_Lower" ON "RbacPermissionGrants" (lower("AssignedEntityId")); +CREATE INDEX IF NOT EXISTS "IX_RbacRoleGrants_AssignedEntityId_Lower" ON "RbacRoleGrants" (lower("AssignedEntityId")); +CREATE INDEX IF NOT EXISTS "IX_RbacRoleGrants_AssignedEntityId" ON "RbacRoleGrants" ("AssignedEntityId"); +CREATE INDEX IF NOT EXISTS "IX_RbacRole_Name_Lower" ON "RbacRole" (lower("Name")); diff --git a/db/seed/RbacPermissionGrants.csv b/db/seed/RbacPermissionGrants.csv index f91f6f76..336416a2 100644 --- a/db/seed/RbacPermissionGrants.csv +++ b/db/seed/RbacPermissionGrants.csv @@ -138,5 +138,6 @@ BE05A459-AA3A-4A74-B300-62BA19B50618;2026-07-23T09:15:35.802676;Role;5E32EE6A-1A B908B85B-8489-4E5F-8130-BFA4FAA7A631;2026-07-23T09:15:35.802680;Role;5E32EE6A-1A73-4ACF-9C61-90E4D0D59261;system-admin;get-user-emails;Global; FAABC901-C540-4FDD-9408-16812B3683E3;2026-07-23T09:15:35.802684;Role;5E32EE6A-1A73-4ACF-9C61-90E4D0D59261;system-admin;delete-membership-application-as-admin;Global; 708B2C75-2CDD-4545-A717-D5BBDE8C9884;2026-07-23T09:15:35.802688;Role;5E32EE6A-1A73-4ACF-9C61-90E4D0D59261;system-admin;retry-creating-message-contract;Global; +77763D1C-2D3A-426F-BB4B-CDBFC48BDC3D;2026-07-23T09:15:35.802690;Role;5E32EE6A-1A73-4ACF-9C61-90E4D0D59261;system-admin;manage-json-schemas;Global; 7E6A90BB-9E8C-4F11-AFE8-F53D97B4B803;2026-07-23T09:15:35.802693;Role;6A2EE52C-6A9B-4A2A-B9C8-5851DD2D9A6F;capability-management;batch-create-capabilities;Global; 213F793E-3048-427E-863C-D359BBA7D9CA;2026-07-23T09:15:35.802697;Role;A983CF2E-772E-437D-B9D8-5DDF769339D3;service-catalogue;read;Global; diff --git a/src/SelfService.Tests/Application/TestMembershipApplicationService.cs b/src/SelfService.Tests/Application/TestMembershipApplicationService.cs index b25ed63b..e58cce7b 100644 --- a/src/SelfService.Tests/Application/TestMembershipApplicationService.cs +++ b/src/SelfService.Tests/Application/TestMembershipApplicationService.cs @@ -127,6 +127,9 @@ public async Task submit_membership_application_auto_finalizes_when_capability_h Assert.NotNull(added); Assert.True(added!.IsFinalized); - rbacService.Verify(x => x.GrantRoleGrant(userId.ToString(), It.IsAny()), Times.Once); + rbacService.Verify( + x => x.GrantRoleGrant(userId.ToString(), It.IsAny(), It.IsAny()), + Times.Once + ); } } diff --git a/src/SelfService.Tests/Application/TestRbacApplicationService.cs b/src/SelfService.Tests/Application/TestRbacApplicationService.cs index e38afb49..143e0de7 100644 --- a/src/SelfService.Tests/Application/TestRbacApplicationService.cs +++ b/src/SelfService.Tests/Application/TestRbacApplicationService.cs @@ -758,4 +758,139 @@ await rbacSvc.IsUserPermitted( ); */ } + + private static RbacPermissionGrant UserGrant(RbacAccessType type, string resource) => + new( + id: RbacPermissionGrantId.New(), + createdAt: DateTime.Now, + assignedEntityType: AssignedEntityType.User, + assignedEntityId: "test01@dfds.cloud", + @namespace: RbacNamespace.Rbac, + permission: "create", + type: type, + resource: resource + ); + + private static Permission RbacCreate(RbacAccessType accessType) => + new() + { + Namespace = RbacNamespace.Rbac, + Name = "create", + AccessType = accessType, + }; + + [Fact] + public async Task CapabilityScopedGrantDoesNotSatisfyGlobalCheck() + { + var fixture = await RbacTestData.NewInMemoryFixture( + true, + new List { UserGrant(RbacAccessType.Capability, "test01") }, + new List() + ); + var rbacSvc = fixture.ApiApplication.Services.GetService()!; + + // The object id matches the grant's resource exactly — before the fix that alone was enough. + Assert.False( + ( + await rbacSvc.IsUserPermitted("test01@dfds.cloud", [RbacCreate(RbacAccessType.Global)], "test01") + ).Permitted() + ); + + // The same grant still answers the capability-scoped question it was issued for. + Assert.True( + ( + await rbacSvc.IsUserPermitted("test01@dfds.cloud", [RbacCreate(RbacAccessType.Capability)], "test01") + ).Permitted() + ); + } + + [Fact] + public async Task GlobalGrantSatisfiesBothCapabilityAndGlobalChecks() + { + var fixture = await RbacTestData.NewInMemoryFixture( + true, + new List { UserGrant(RbacAccessType.Global, "") }, + new List() + ); + var rbacSvc = fixture.ApiApplication.Services.GetService()!; + + Assert.True( + ( + await rbacSvc.IsUserPermitted("test01@dfds.cloud", [RbacCreate(RbacAccessType.Global)], "test01") + ).Permitted() + ); + + // This is how the CloudEngineers group keeps working: its role grant is Global, and the + // routes it reaches are largely capability-scoped. + Assert.True( + ( + await rbacSvc.IsUserPermitted("test01@dfds.cloud", [RbacCreate(RbacAccessType.Capability)], "test01") + ).Permitted() + ); + } + + [Fact] + public async Task RoleDerivedGrantsFollowTheSameScopeHierarchy() + { + // GetPermissionGrantsForRoleGrants stamps the role grant's type/resource onto every + // permission of the role, so the second FindAll pass needs its own coverage. + var capabilityRoleId = RbacRoleId.New(); + var globalRoleId = RbacRoleId.New(); + + RbacPermissionGrant RolePermission(RbacRoleId roleId) => + new( + id: RbacPermissionGrantId.New(), + createdAt: DateTime.Now, + assignedEntityType: AssignedEntityType.Role, + assignedEntityId: roleId.ToString(), + @namespace: RbacNamespace.Rbac, + permission: "create", + type: RbacAccessType.Global, + resource: "" + ); + + RbacRoleGrant RoleGrant(RbacRoleId roleId, string user, RbacAccessType type, string resource) => + new( + id: RbacRoleGrantId.New(), + roleId: roleId, + createdAt: DateTime.Now, + assignedEntityType: AssignedEntityType.User, + assignedEntityId: user, + type: type, + resource: resource + ); + + var fixture = await RbacTestData.NewInMemoryFixture( + true, + new List { RolePermission(capabilityRoleId), RolePermission(globalRoleId) }, + new List + { + RoleGrant(capabilityRoleId, "owner@dfds.cloud", RbacAccessType.Capability, "test01"), + RoleGrant(globalRoleId, "admin@dfds.cloud", RbacAccessType.Global, ""), + } + ); + var rbacSvc = fixture.ApiApplication.Services.GetService()!; + + Assert.False( + ( + await rbacSvc.IsUserPermitted("owner@dfds.cloud", [RbacCreate(RbacAccessType.Global)], "test01") + ).Permitted() + ); + Assert.True( + ( + await rbacSvc.IsUserPermitted("owner@dfds.cloud", [RbacCreate(RbacAccessType.Capability)], "test01") + ).Permitted() + ); + + Assert.True( + ( + await rbacSvc.IsUserPermitted("admin@dfds.cloud", [RbacCreate(RbacAccessType.Global)], "test01") + ).Permitted() + ); + Assert.True( + ( + await rbacSvc.IsUserPermitted("admin@dfds.cloud", [RbacCreate(RbacAccessType.Capability)], "test01") + ).Permitted() + ); + } } diff --git a/src/SelfService.Tests/Application/TestRbacGrantAuthorization.cs b/src/SelfService.Tests/Application/TestRbacGrantAuthorization.cs new file mode 100644 index 00000000..170514b7 --- /dev/null +++ b/src/SelfService.Tests/Application/TestRbacGrantAuthorization.cs @@ -0,0 +1,283 @@ +using Microsoft.AspNetCore.Http; +using Microsoft.EntityFrameworkCore; +using Microsoft.Extensions.DependencyInjection; +using SelfService.Application; +using SelfService.Domain.Models; + +namespace SelfService.Tests.Application; + +/// +/// Authorization tests for when the grant comes +/// from a caller-supplied request body (userInitiated: true). +/// +public class TestRbacGrantAuthorization +{ + private const string Owner = "owner@bar.com"; + private const string CloudEngineer = "ce@dfds.com"; + private const string Nobody = "nobody@dfds.com"; + private const string Bystander = "someone@dfds.com"; + private const string Bar = "bar"; + private const string OtherCapability = "other-capability"; + + private class Scenario + { + public required RbacInMemoryTestFixture Fixture { get; init; } + public required IRbacApplicationService Service { get; init; } + + /// Mirrors the seeded Owner role: capability manage-permissions plus rbac/create. + public required RbacRoleId OwnerRoleId { get; init; } + public required RbacRoleId CloudEngineerRoleId { get; init; } + public required RbacRoleId ReaderRoleId { get; init; } + public required RbacRoleId GuestRoleId { get; init; } + } + + private static async Task NewScenario() + { + // Seed explicitly rather than through PopulateRbac's arguments: that helper is `async void` and + // is not awaited by NewInMemoryFixture, so anything assertions depend on must be added here. + var fixture = await RbacTestData.NewInMemoryFixture( + populateDatabase: true, + rbacPermissionGrantsSeed: new List(), + rbacRoleGrantsSeed: new List(), + rbacGroupSeed: new List() + ); + + var db = fixture.DbContext; + + var ownerRole = RbacRole.New("system", "Owner", "", RbacAccessType.Global); + var cloudEngineerRole = RbacRole.New("system", "CloudEngineer", "", RbacAccessType.Global); + var readerRole = RbacRole.New("system", "Reader", "", RbacAccessType.Global); + var guestRole = RbacRole.New("system", "Guest", "", RbacAccessType.Global); + + db.RbacRoles.AddRange(ownerRole, cloudEngineerRole, readerRole, guestRole); + + db.RbacPermissionGrants.AddRange( + RbacPermissionGrant.New( + AssignedEntityType.Role, + ownerRole.Id.ToString(), + RbacNamespace.CapabilityManagement, + "manage-permissions", + RbacAccessType.Capability, + "" + ), + // The Owner role really does carry rbac/create — that is what made finding #1 exploitable. + RbacPermissionGrant.New( + AssignedEntityType.Role, + ownerRole.Id.ToString(), + RbacNamespace.Rbac, + "create", + RbacAccessType.Global, + "" + ), + RbacPermissionGrant.New( + AssignedEntityType.Role, + cloudEngineerRole.Id.ToString(), + RbacNamespace.Rbac, + "create", + RbacAccessType.Global, + "" + ) + ); + + db.RbacRoleGrants.AddRange( + RbacRoleGrant.New(ownerRole.Id, AssignedEntityType.User, Owner, RbacAccessType.Capability, Bar), + RbacRoleGrant.New(cloudEngineerRole.Id, AssignedEntityType.User, CloudEngineer, RbacAccessType.Global, "") + ); + + await db.SaveChangesAsync(); + + return new Scenario + { + Fixture = fixture, + Service = fixture.ApiApplication.Services.GetService()!, + OwnerRoleId = ownerRole.Id, + CloudEngineerRoleId = cloudEngineerRole.Id, + ReaderRoleId = readerRole.Id, + GuestRoleId = guestRole.Id, + }; + } + + private static RbacRoleGrant Grant(RbacRoleId roleId, string assignedTo, RbacAccessType type, string? resource) => + new(RbacRoleGrantId.New(), roleId, DateTime.Now, AssignedEntityType.User, assignedTo, type, resource!); + + // The proxied TransactionalAspect doesn't apply when ConfigureRbac re-registers the service without + // rewiring, so changes live in the DbContext tracker until we flush them. + private static async Task> PersistedGrants(Scenario scenario) + { + await scenario.Fixture.DbContext.SaveChangesAsync(); + return await scenario.Fixture.DbContext.RbacRoleGrants.ToListAsync(); + } + + [Fact] + public async Task capability_owner_cannot_create_a_global_role_grant() + { + // Regression test for security-review-01 finding #1: a capability Owner holds rbac/create, but + // only through a Capability-scoped role grant, so it must not authorize a global grant. + var scenario = await NewScenario(); + + await Assert.ThrowsAsync( + async () => + await scenario.Service.GrantRoleGrant( + Owner, + Grant(scenario.CloudEngineerRoleId, Owner, RbacAccessType.Global, ""), + userInitiated: true + ) + ); + } + + [Fact] + public async Task global_admin_can_create_a_global_role_grant() + { + var scenario = await NewScenario(); + + await scenario.Service.GrantRoleGrant( + CloudEngineer, + Grant(scenario.CloudEngineerRoleId, Bystander, RbacAccessType.Global, ""), + userInitiated: true + ); + + var grants = await PersistedGrants(scenario); + Assert.Contains(grants, g => g.AssignedEntityId == Bystander && g.Type == RbacAccessType.Global); + } + + [Fact] + public async Task capability_manager_cannot_grant_on_another_capability() + { + var scenario = await NewScenario(); + + await Assert.ThrowsAsync( + async () => + await scenario.Service.GrantRoleGrant( + Owner, + Grant(scenario.ReaderRoleId, Bystander, RbacAccessType.Capability, OtherCapability), + userInitiated: true + ) + ); + } + + [Fact] + public async Task capability_manager_can_grant_to_another_user_on_own_capability() + { + var scenario = await NewScenario(); + + await scenario.Service.GrantRoleGrant( + Owner, + Grant(scenario.ReaderRoleId, Bystander, RbacAccessType.Capability, Bar), + userInitiated: true + ); + + var grants = await PersistedGrants(scenario); + Assert.Contains(grants, g => g.AssignedEntityId == Bystander && g.Resource == Bar); + } + + [Fact] + public async Task capability_manager_cannot_grant_to_themselves() + { + var scenario = await NewScenario(); + + await Assert.ThrowsAsync( + async () => + await scenario.Service.GrantRoleGrant( + Owner, + Grant(scenario.OwnerRoleId, Owner, RbacAccessType.Capability, Bar), + userInitiated: true + ) + ); + } + + [Fact] + public async Task global_admin_can_grant_to_themselves() + { + var scenario = await NewScenario(); + + await scenario.Service.GrantRoleGrant( + CloudEngineer, + Grant(scenario.OwnerRoleId, CloudEngineer, RbacAccessType.Capability, Bar), + userInitiated: true + ); + + var grants = await PersistedGrants(scenario); + Assert.Contains(grants, g => g.AssignedEntityId == CloudEngineer && g.Resource == Bar); + } + + [Fact] + public async Task user_without_permissions_cannot_grant_anything() + { + var scenario = await NewScenario(); + + await Assert.ThrowsAsync( + async () => + await scenario.Service.GrantRoleGrant( + Nobody, + Grant(scenario.ReaderRoleId, Bystander, RbacAccessType.Capability, Bar), + userInitiated: true + ) + ); + } + + [Fact] + public async Task system_initiated_grants_are_not_authorized() + { + // Mirror of the finding #1 test with the flag defaulted. This pins the contract the five + // bootstrap call sites rely on — flipping the default must fail loudly here. + var scenario = await NewScenario(); + + await scenario.Service.GrantRoleGrant( + Owner, + Grant(scenario.CloudEngineerRoleId, Owner, RbacAccessType.Global, "") + ); + + var grants = await PersistedGrants(scenario); + Assert.Contains(grants, g => g.AssignedEntityId == Owner && g.Type == RbacAccessType.Global); + } + + [Fact] + public async Task bulk_grants_stay_on_the_trusted_path() + { + var scenario = await NewScenario(); + + await scenario.Service.GrantRoleGrants( + Nobody, + new List + { + Grant(scenario.ReaderRoleId, Bystander, RbacAccessType.Capability, Bar), + Grant(scenario.ReaderRoleId, Bystander, RbacAccessType.Capability, OtherCapability), + } + ); + + var grants = await PersistedGrants(scenario); + Assert.Contains(grants, g => g.AssignedEntityId == Bystander && g.Resource == Bar); + Assert.Contains(grants, g => g.AssignedEntityId == Bystander && g.Resource == OtherCapability); + } + + [Fact] + public async Task guest_role_cannot_be_granted_at_capability_scope() + { + var scenario = await NewScenario(); + + await Assert.ThrowsAsync( + async () => + await scenario.Service.GrantRoleGrant( + Owner, + Grant(scenario.GuestRoleId, Bystander, RbacAccessType.Capability, Bar), + userInitiated: true + ) + ); + } + + [Fact] + public async Task capability_grant_without_resource_is_rejected() + { + var scenario = await NewScenario(); + + // Granted by a global admin so the request reaches the switch's own validation. + await Assert.ThrowsAsync( + async () => + await scenario.Service.GrantRoleGrant( + CloudEngineer, + Grant(scenario.ReaderRoleId, Bystander, RbacAccessType.Capability, null), + userInitiated: true + ) + ); + } +} diff --git a/src/SelfService.Tests/Infrastructure/Api/TestCapabilityRoleGrantRoutes.cs b/src/SelfService.Tests/Infrastructure/Api/TestCapabilityRoleGrantRoutes.cs new file mode 100644 index 00000000..ca7d5679 --- /dev/null +++ b/src/SelfService.Tests/Infrastructure/Api/TestCapabilityRoleGrantRoutes.cs @@ -0,0 +1,135 @@ +using System.Net; +using System.Text; +using SelfService.Application; +using SelfService.Domain.Models; +using SelfService.Domain.Queries; +using SelfService.Tests.TestDoubles; + +namespace SelfService.Tests.Infrastructure.Api; + +/// +/// The RBAC engine is stubbed to permit everything, so these tests prove that the controller itself +/// binds the request body to the route capability instead of trusting whatever the caller posted. +/// +public class TestCapabilityRoleGrantRoutes +{ + private static readonly string RoleId = Guid.NewGuid().ToString(); + + private static (ApiApplication Application, StubRbacApplicationService Rbac) NewApplication() + { + var rbac = new StubRbacApplicationService(isPermitted: true); + var application = new ApiApplication(); + application.ReplaceService(new StubCapabilityRepository(A.Capability.WithId("foo"))); + application.ReplaceService(new StubRbacPermissionGrantRepository()); + application.ReplaceService(new StubRbacRoleGrantRepository()); + application.ReplaceService(rbac); + application.ReplaceService(new StubPermissionQuery()); + return (application, rbac); + } + + private static StringContent Body(string json) => new(json, Encoding.UTF8, "application/json"); + + [Fact] + public async Task global_grant_is_rejected_before_reaching_the_service() + { + var (application, rbac) = NewApplication(); + await using var _ = application; + using var client = application.CreateClient(); + + var response = await client.PostAsync( + "/capabilities/foo/roles/grant", + Body( + $$""" + {"roleId":"{{RoleId}}","assignedEntityType":"User","assignedEntityId":"attacker@dfds.com","type":"global","resource":""} + """ + ) + ); + + Assert.Equal(HttpStatusCode.BadRequest, response.StatusCode); + Assert.Empty(rbac.GrantedRoleGrants); + } + + [Fact] + public async Task grant_naming_another_capability_is_rejected() + { + var (application, rbac) = NewApplication(); + await using var _ = application; + using var client = application.CreateClient(); + + var response = await client.PostAsync( + "/capabilities/foo/roles/grant", + Body( + $$""" + {"roleId":"{{RoleId}}","assignedEntityType":"User","assignedEntityId":"someone@dfds.com","type":"capability","resource":"other-capability"} + """ + ) + ); + + Assert.Equal(HttpStatusCode.BadRequest, response.StatusCode); + Assert.Empty(rbac.GrantedRoleGrants); + } + + [Fact] + public async Task grant_matching_the_route_capability_is_accepted() + { + var (application, rbac) = NewApplication(); + await using var _ = application; + using var client = application.CreateClient(); + + var response = await client.PostAsync( + "/capabilities/foo/roles/grant", + Body( + $$""" + {"roleId":"{{RoleId}}","assignedEntityType":"User","assignedEntityId":"someone@dfds.com","type":"capability","resource":"foo"} + """ + ) + ); + + // Created() carries no body, which HttpNoContentOutputFormatter normalizes to 204. + Assert.Equal(HttpStatusCode.NoContent, response.StatusCode); + var grant = Assert.Single(rbac.GrantedRoleGrants); + Assert.Equal("foo", grant.Resource); + Assert.Equal(RbacAccessType.Capability, grant.Type); + } + + [Fact] + public async Task omitted_resource_defaults_to_the_route_capability() + { + var (application, rbac) = NewApplication(); + await using var _ = application; + using var client = application.CreateClient(); + + var response = await client.PostAsync( + "/capabilities/foo/roles/grant", + Body( + $$""" + {"roleId":"{{RoleId}}","assignedEntityType":"User","assignedEntityId":"someone@dfds.com","type":"capability"} + """ + ) + ); + + Assert.Equal(HttpStatusCode.NoContent, response.StatusCode); + var grant = Assert.Single(rbac.GrantedRoleGrants); + Assert.Equal("foo", grant.Resource); + } + + [Fact] + public async Task unparseable_access_type_is_a_bad_request_not_a_server_error() + { + var (application, rbac) = NewApplication(); + await using var _ = application; + using var client = application.CreateClient(); + + var response = await client.PostAsync( + "/capabilities/foo/roles/grant", + Body( + $$""" + {"roleId":"{{RoleId}}","assignedEntityType":"User","assignedEntityId":"someone@dfds.com","type":"nonsense","resource":"foo"} + """ + ) + ); + + Assert.Equal(HttpStatusCode.BadRequest, response.StatusCode); + Assert.Empty(rbac.GrantedRoleGrants); + } +} diff --git a/src/SelfService.Tests/Infrastructure/Api/TestRbacCanIRoutes.cs b/src/SelfService.Tests/Infrastructure/Api/TestRbacCanIRoutes.cs new file mode 100644 index 00000000..50363dbc --- /dev/null +++ b/src/SelfService.Tests/Infrastructure/Api/TestRbacCanIRoutes.cs @@ -0,0 +1,50 @@ +using System.Net; +using System.Text; +using SelfService.Tests.Application; + +namespace SelfService.Tests.Infrastructure.Api; + +public class TestRbacCanIRoutes +{ + private static StringContent Body(string json) => new(json, Encoding.UTF8, "application/json"); + + [Fact] + public async Task duplicate_permissions_do_not_crash_the_endpoint() + { + var fixture = await RbacTestData.NewInMemoryFixture(); + var application = fixture.ApiApplication; + await using var _ = application; + using var client = application.CreateClient(); + + var response = await client.PostAsync( + "/rbac/can-i", + Body( + """ + {"permissions":[{"namespace":"topics","name":"create"},{"namespace":"topics","name":"create"}],"objectid":"sandbox-emcla-pmyxn"} + """ + ) + ); + + Assert.Equal(HttpStatusCode.OK, response.StatusCode); + } + + [Fact] + public async Task distinct_permissions_still_work() + { + var fixture = await RbacTestData.NewInMemoryFixture(); + var application = fixture.ApiApplication; + await using var _ = application; + using var client = application.CreateClient(); + + var response = await client.PostAsync( + "/rbac/can-i", + Body( + """ + {"permissions":[{"namespace":"topics","name":"create"},{"namespace":"topics","name":"read-private"}],"objectid":"sandbox-emcla-pmyxn"} + """ + ) + ); + + Assert.Equal(HttpStatusCode.OK, response.StatusCode); + } +} diff --git a/src/SelfService.Tests/TestDoubles/StupRbacApplicationService.cs b/src/SelfService.Tests/TestDoubles/StupRbacApplicationService.cs index 7d43783e..0be1d561 100644 --- a/src/SelfService.Tests/TestDoubles/StupRbacApplicationService.cs +++ b/src/SelfService.Tests/TestDoubles/StupRbacApplicationService.cs @@ -125,9 +125,16 @@ public Task> GrantPermissions(string user, throw new NotImplementedException(); } - public Task GrantRoleGrant(string user, RbacRoleGrant roleGrant) + /// + /// Grants recorded by . Route tests assert on this to prove a request + /// was rejected by the controller before it ever reached the service. + /// + public List GrantedRoleGrants { get; } = new(); + + public Task GrantRoleGrant(string user, RbacRoleGrant roleGrant, bool userInitiated = false) { - throw new NotImplementedException(); + GrantedRoleGrants.Add(roleGrant); + return Task.CompletedTask; } public Task> GrantRoleGrants(string user, List grants) diff --git a/src/SelfService/Application/IRbacApplicationService.cs b/src/SelfService/Application/IRbacApplicationService.cs index 365d085e..d3adbfa6 100644 --- a/src/SelfService/Application/IRbacApplicationService.cs +++ b/src/SelfService/Application/IRbacApplicationService.cs @@ -28,7 +28,15 @@ public interface IRbacApplicationService Task GrantPermission(string user, RbacPermissionGrant permissionGrant); Task> GrantPermissions(string user, List grants); Task RevokePermission(string user, string id); - Task GrantRoleGrant(string user, RbacRoleGrant roleGrant); + + /// + /// True when came from a caller-supplied request body rather than a + /// platform workflow. Enables authorization: global grants are refused, the caller must hold + /// capability-management/manage-permissions on roleGrant.Resource (or global rbac/create), and + /// self-grants are refused. Defaults to false, which preserves the trusted behaviour used by + /// capability bootstrap, membership approval, bulk import and the global /rbac endpoints. + /// + Task GrantRoleGrant(string user, RbacRoleGrant roleGrant, bool userInitiated = false); Task> GrantRoleGrants(string user, List grants); Task RevokeRoleGrant(string user, string id); Task RevokeCapabilityRoleGrant(UserId userId, CapabilityId capabilityId); diff --git a/src/SelfService/Application/RbacApplicationService.cs b/src/SelfService/Application/RbacApplicationService.cs index d17ac416..73e65855 100644 --- a/src/SelfService/Application/RbacApplicationService.cs +++ b/src/SelfService/Application/RbacApplicationService.cs @@ -38,7 +38,7 @@ IRbacRoleRepository roleRepository public async Task IsUserPermitted(string user, List permissions, string objectId) { var resp = new PermittedResponse(); - permissions.ForEach(p => resp.PermissionMatrix.Add($"{p.Namespace}-{p.Name}", new PermissionMatrix(p))); + permissions.ForEach(p => resp.PermissionMatrix.TryAdd($"{p.Namespace}-{p.Name}", new PermissionMatrix(p))); // user level var userPermissions = await GetPermissionGrantsForUser(user); @@ -87,7 +87,9 @@ public async Task IsUserPermitted(string user, List + // Shared by both grant sources below (direct/group grants and role-derived grants) so the + // matching rule cannot drift between them. + bool Evaluate(RbacPermissionGrant p) { var policyGrantsAccess = false; if (p.Type != RbacAccessType.Global && (p.Resource is null || !p.Resource.Equals(objectId))) @@ -97,7 +99,13 @@ public async Task IsUserPermitted(string user, List { - if (pm.Namespace == p.Namespace && pm.Name == p.Permission) + // Scope is a hierarchy: a Global grant satisfies a capability-scoped check, but a + // capability-scoped grant must never satisfy a Global one + if ( + pm.Namespace == p.Namespace + && pm.Name == p.Permission + && (pm.AccessType != RbacAccessType.Global || p.Type == RbacAccessType.Global) + ) { resp.PermissionMatrix[$"{p.Namespace}-{p.Permission}"].Permitted = true; policyGrantsAccess = true; @@ -105,32 +113,14 @@ public async Task IsUserPermitted(string user, List - { - var policyGrantsAccess = false; - if (p.Type != RbacAccessType.Global && (p.Resource is null || !p.Resource.Equals(objectId))) - { - return false; - } - - permissions.ForEach(pm => - { - if (pm.Namespace == p.Namespace && pm.Name == p.Permission) - { - resp.PermissionMatrix[$"{p.Namespace}-{p.Permission}"].Permitted = true; - policyGrantsAccess = true; - } - }); - - return policyGrantsAccess; - }); - resp.PermissionGrants.AddRange(accessGrantingPermissionGrants); + resp.PermissionGrants.AddRange(permissionsFromRoles.FindAll(Evaluate)); return resp; } @@ -439,24 +429,77 @@ public async Task RevokePermission(string user, string id) _cache.Reset(); } + // Resolves the same grant sources as IsUserPermitted (direct user grants, group grants and + // role-derived grants) but only accepts matches whose scope is Global. + private async Task HasGlobalPermission(string user, RbacNamespace ns, string name) + { + var userPermissions = await GetPermissionGrantsForUser(user); + var userRoles = await GetRoleGrantsForUser(user); + + var groupPermissions = await _cache.GetOrAddAsync( + CacheConst.UserGroupPermissions, + user, + () => _permissionQuery.FindUserGroupPermissionsByUserId(user) + ); + var groupRoles = await _cache.GetOrAddAsync( + CacheConst.UserGroupRoles, + user, + () => _permissionQuery.FindUserGroupRolesByUserId(user) + ); + + var combinedPermissions = userPermissions.Concat(groupPermissions).ToList(); + var combinedRoles = userRoles.Concat(groupRoles).ToList(); + combinedPermissions.AddRange(await GetPermissionGrantsForRoleGrants(combinedRoles)); + + return combinedPermissions.Any(p => + p.Type == RbacAccessType.Global && p.Namespace == ns && p.Permission == name + ); + } + [TransactionalBoundary] - public async Task GrantRoleGrant(string user, RbacRoleGrant roleGrant) + public async Task GrantRoleGrant(string user, RbacRoleGrant roleGrant, bool userInitiated = false) { - //PermittedResponse? canUserCreateGlobalRbac; - switch (roleGrant.Type) + if (userInitiated) { - case var a when a == RbacAccessType.Global: - /* - canUserCreateGlobalRbac = await IsUserPermitted( - user, - new List { new(RbacNamespace.Rbac, "create", "", RbacAccessType.Global) }, - roleGrant.Resource ?? "" - ); - if (!canUserCreateGlobalRbac.Permitted()) + var globalAdmin = await HasGlobalPermission(user, RbacNamespace.Rbac, "create"); + if (!globalAdmin) + { + // A caller without global rbac/create may only grant within a capability they manage. + if (roleGrant.Type != RbacAccessType.Capability) { throw new UnauthorizedAccessException(); } - */ + + var canManage = ( + await IsUserPermitted( + user, + new List + { + new( + RbacNamespace.CapabilityManagement, + "manage-permissions", + "", + RbacAccessType.Capability + ), + }, + roleGrant.Resource ?? "" + ) + ).Permitted(); + + var userGrantsToSelf = + roleGrant.AssignedEntityType == AssignedEntityType.User + && string.Equals(roleGrant.AssignedEntityId, user, StringComparison.OrdinalIgnoreCase); + + if (!canManage || userGrantsToSelf) + { + throw new UnauthorizedAccessException(); + } + } + } + + switch (roleGrant.Type) + { + case var a when a == RbacAccessType.Global: await _roleGrantRepository.Add( RbacRoleGrant.New( roleGrant.RoleId, @@ -469,32 +512,6 @@ await _roleGrantRepository.Add( break; case var a when a == RbacAccessType.Capability: - /* - canUserCreateGlobalRbac = await IsUserPermitted( - user, - new List { new(RbacNamespace.Rbac, "create", "", RbacAccessType.Global) }, - roleGrant.Resource ?? "" - ); - var canUserCreateCapabilityRbac = await IsUserPermitted( - user, - new List - { - new(RbacNamespace.CapabilityManagement, "manage-permissions", "", RbacAccessType.Capability), - }, - roleGrant.Resource ?? "" - ); - var userGrantsToSelf = canUserCreateGlobalRbac.Permitted() - ? false - : user == roleGrant.AssignedEntityId && roleGrant.AssignedEntityType == AssignedEntityType.User; - - if ( - (!canUserCreateGlobalRbac.Permitted() && !canUserCreateCapabilityRbac.Permitted()) - || userGrantsToSelf - ) - { - throw new UnauthorizedAccessException(); - } - */ if (roleGrant.Resource == null) { throw new BadHttpRequestException("Capability ID is required for capability role grants"); @@ -1020,6 +1037,13 @@ public static List BootstrapPermissions() "Retry failed message contract creation as administrator", RbacAccessType.Global ), + new(RbacNamespace.SystemAdmin, "manage-teams", "Create, delete and link teams", RbacAccessType.Global), + new( + RbacNamespace.SystemAdmin, + "manage-json-schemas", + "Publish json schema versions", + RbacAccessType.Global + ), }; return permissions; diff --git a/src/SelfService/Infrastructure/Api/Capabilities/CapabilityController.cs b/src/SelfService/Infrastructure/Api/Capabilities/CapabilityController.cs index a0e6e1f0..94e77157 100644 --- a/src/SelfService/Infrastructure/Api/Capabilities/CapabilityController.cs +++ b/src/SelfService/Infrastructure/Api/Capabilities/CapabilityController.cs @@ -1703,14 +1703,76 @@ public async Task DeactivateSelfAssessmentOption([FromRoute] stri [HttpPost("{id:required}/roles/grant")] [ProducesResponseType(StatusCodes.Status201Created)] + [ProducesResponseType(StatusCodes.Status400BadRequest)] [ProducesResponseType(StatusCodes.Status401Unauthorized)] + [ProducesResponseType(StatusCodes.Status403Forbidden)] [RequiresPermission("rbac", "create")] - public async Task GrantRole([FromBody] RbacRoleGrant roleGrant) + public async Task GrantRole(string id, [FromBody] RbacRoleGrant roleGrant) { if (!User.TryGetUserId(out var userId)) return Unauthorized(); - await _rbacApplicationService.GrantRoleGrant(userId.ToString(), roleGrant.IntoDomainModel()); + // The request was authorized as capability-scoped rbac/create on {id}, so the body must not be + // allowed to name a different scope or a different capability. + if ( + !RbacAccessType.TryParse(roleGrant.Type ?? "", out var accessType) + || accessType != RbacAccessType.Capability + ) + { + return BadRequest( + new ProblemDetails + { + Title = "Invalid role grant type", + Detail = "Only capability-scoped role grants can be created through this endpoint.", + Status = StatusCodes.Status400BadRequest, + } + ); + } + + if ( + !string.IsNullOrEmpty(roleGrant.Resource) + && !string.Equals(roleGrant.Resource, id, StringComparison.OrdinalIgnoreCase) + ) + { + return BadRequest( + new ProblemDetails + { + Title = "Resource does not match capability", + Detail = $"The role grant resource must be \"{id}\" or omitted.", + Status = StatusCodes.Status400BadRequest, + } + ); + } + + if (!RbacRoleId.TryParse(roleGrant.RoleId, out var roleId)) + { + return BadRequest( + new ProblemDetails + { + Title = "Invalid role id", + Detail = $"Value \"{roleGrant.RoleId}\" is not a valid role id.", + Status = StatusCodes.Status400BadRequest, + } + ); + } + + var grant = Domain.Models.RbacRoleGrant.New( + roleId, + roleGrant.AssignedEntityType, + roleGrant.AssignedEntityId, + RbacAccessType.Capability, + id + ); + + try + { + await _rbacApplicationService.GrantRoleGrant(userId.ToString(), grant, userInitiated: true); + } + catch (UnauthorizedAccessException) + { + return StatusCode(StatusCodes.Status403Forbidden); + } + return Created(); } diff --git a/src/SelfService/Infrastructure/Api/JsonSchema/SelfServiceJsonSchemaController.cs b/src/SelfService/Infrastructure/Api/JsonSchema/SelfServiceJsonSchemaController.cs index 9e00934a..618a365d 100644 --- a/src/SelfService/Infrastructure/Api/JsonSchema/SelfServiceJsonSchemaController.cs +++ b/src/SelfService/Infrastructure/Api/JsonSchema/SelfServiceJsonSchemaController.cs @@ -2,12 +2,14 @@ using SelfService.Domain.Exceptions; using SelfService.Domain.Models; using SelfService.Domain.Services; +using SelfService.Infrastructure.Api.RBAC; namespace SelfService.Infrastructure.Api.JsonSchema; [Route("json-schema")] [Produces("application/json")] [ApiController] +[RbacConfig(nameof(RbacObjectType.Global), "id")] public class SelfServiceJsonSchemaController : ControllerBase { private readonly ISelfServiceJsonSchemaService _selfServiceJsonSchemaService; @@ -58,9 +60,11 @@ public async Task GetSchema(string id, [FromQuery] int schemaVers } [HttpPost("{id:required}")] + [RequiresPermission("system-admin", "manage-json-schemas")] [ProducesResponseType(StatusCodes.Status200OK)] [ProducesResponseType(typeof(ProblemDetails), StatusCodes.Status400BadRequest, "application/problem+json")] [ProducesResponseType(typeof(ProblemDetails), StatusCodes.Status401Unauthorized, "application/problem+json")] + [ProducesResponseType(StatusCodes.Status403Forbidden)] [ProducesResponseType(typeof(ProblemDetails), StatusCodes.Status500InternalServerError, "application/problem+json")] public async Task AddSchema(string id, [FromBody] AddSelfServiceJsonSchemaRequest? request) { diff --git a/src/SelfService/Infrastructure/Api/MembershipApplications/MembershipApplicationController.cs b/src/SelfService/Infrastructure/Api/MembershipApplications/MembershipApplicationController.cs index 3e51a761..39035d29 100644 --- a/src/SelfService/Infrastructure/Api/MembershipApplications/MembershipApplicationController.cs +++ b/src/SelfService/Infrastructure/Api/MembershipApplications/MembershipApplicationController.cs @@ -217,8 +217,66 @@ public async Task SubmitApproval(string id, [FromBody] RBAC.Dto.R ); } + // A caller-supplied grant may only target the capability of the application being approved, + // and only at capability scope — this endpoint has no RBAC gate of its own. + if (request != null) + { + if ( + !string.IsNullOrEmpty(request.Type) + && ( + !RbacAccessType.TryParse(request.Type, out var requestedType) + || requestedType != RbacAccessType.Capability + ) + ) + { + return BadRequest( + new ProblemDetails + { + Title = "Invalid role grant type", + Detail = "Only capability-scoped role grants can be created through this endpoint.", + Status = StatusCodes.Status400BadRequest, + } + ); + } + + if ( + !string.IsNullOrEmpty(request.Resource) + && !string.Equals( + request.Resource, + application.CapabilityId.ToString(), + StringComparison.OrdinalIgnoreCase + ) + ) + { + return BadRequest( + new ProblemDetails + { + Title = "Resource does not match capability", + Detail = $"The role grant resource must be \"{application.CapabilityId}\" or omitted.", + Status = StatusCodes.Status400BadRequest, + } + ); + } + + if (!RbacRoleId.TryParse(request.RoleId, out _)) + { + return BadRequest( + new ProblemDetails + { + Title = "Invalid role id", + Detail = $"Value \"{request.RoleId}\" is not a valid role id.", + Status = StatusCodes.Status400BadRequest, + } + ); + } + } + try { + // Approve first: it runs its own CanApproveMembershipApplications check and does not depend + // on the role grant, so an unauthorized caller cannot commit a grant on the way through. + await _membershipApplicationService.ApproveMembershipApplication(membershipApplicationId, userId); + // If request is null, create a legal RbacRoleGrant for Reader role if (request == null) { @@ -253,9 +311,15 @@ public async Task SubmitApproval(string id, [FromBody] RBAC.Dto.R } else { - await _rbacApplicationService.GrantRoleGrant(userId.ToString(), request.IntoDomainModel()); + var suppliedGrant = Domain.Models.RbacRoleGrant.New( + RbacRoleId.Parse(request.RoleId), + request.AssignedEntityType, + request.AssignedEntityId, + RbacAccessType.Capability, + application.CapabilityId.ToString() + ); + await _rbacApplicationService.GrantRoleGrant(userId.ToString(), suppliedGrant, userInitiated: true); } - await _membershipApplicationService.ApproveMembershipApplication(membershipApplicationId, userId); } catch (EntityNotFoundException) { @@ -278,6 +342,10 @@ public async Task SubmitApproval(string id, [FromBody] RBAC.Dto.R } ); } + catch (UnauthorizedAccessException) + { + return StatusCode(StatusCodes.Status403Forbidden); + } return NoContent(); } diff --git a/src/SelfService/Infrastructure/Api/Teams/TeamController.cs b/src/SelfService/Infrastructure/Api/Teams/TeamController.cs index afd57b80..cd36594d 100644 --- a/src/SelfService/Infrastructure/Api/Teams/TeamController.cs +++ b/src/SelfService/Infrastructure/Api/Teams/TeamController.cs @@ -2,12 +2,14 @@ using SelfService.Application; using SelfService.Domain.Models; using SelfService.Infrastructure.Api.Capabilities; +using SelfService.Infrastructure.Api.RBAC; namespace SelfService.Infrastructure.Api.Teams; [Route("teams")] [Produces("application/json")] [ApiController] +[RbacConfig(nameof(RbacObjectType.Global), "id")] public class TeamController : ControllerBase { private readonly ITeamApplicationService _teamApplicationService; @@ -52,8 +54,10 @@ public async Task GetTeam([FromRoute] string id) } [HttpPost("")] + [RequiresPermission("system-admin", "manage-teams")] [ProducesResponseType(typeof(ProblemDetails), StatusCodes.Status400BadRequest, "application/problem+json")] [ProducesResponseType(typeof(ProblemDetails), StatusCodes.Status401Unauthorized, "application/problem+json")] + [ProducesResponseType(StatusCodes.Status403Forbidden)] public async Task AddTeam([FromBody] AddTeamRequest request) { if (!User.TryGetUserId(out var userId)) @@ -92,8 +96,10 @@ public async Task AddTeam([FromBody] AddTeamRequest request) } [HttpDelete("{id}")] + [RequiresPermission("system-admin", "manage-teams")] [ProducesResponseType(typeof(ProblemDetails), StatusCodes.Status400BadRequest, "application/problem+json")] [ProducesResponseType(typeof(ProblemDetails), StatusCodes.Status401Unauthorized, "application/problem+json")] + [ProducesResponseType(StatusCodes.Status403Forbidden)] public async Task RemoveTeam([FromRoute] string id) { if (!TeamId.TryParse(id, out var teamId)) @@ -109,8 +115,10 @@ public async Task RemoveTeam([FromRoute] string id) } [HttpPost("{id}/capability-links/{capabilityId}")] + [RequiresPermission("system-admin", "manage-teams")] [ProducesResponseType(typeof(ProblemDetails), StatusCodes.Status400BadRequest, "application/problem+json")] [ProducesResponseType(typeof(ProblemDetails), StatusCodes.Status401Unauthorized, "application/problem+json")] + [ProducesResponseType(StatusCodes.Status403Forbidden)] public async Task AddLinkToCapability([FromRoute] string id, [FromRoute] string capabilityId) { if (!TeamId.TryParse(id, out var teamId)) @@ -144,8 +152,10 @@ public async Task AddLinkToCapability([FromRoute] string id, [Fro } [HttpDelete("{id}/capability-links/{capabilityId}")] + [RequiresPermission("system-admin", "manage-teams")] [ProducesResponseType(typeof(ProblemDetails), StatusCodes.Status400BadRequest, "application/problem+json")] [ProducesResponseType(typeof(ProblemDetails), StatusCodes.Status401Unauthorized, "application/problem+json")] + [ProducesResponseType(StatusCodes.Status403Forbidden)] public async Task RemoveLinkToCapability([FromRoute] string id, [FromRoute] string capabilityId) { if (!TeamId.TryParse(id, out var teamId)) diff --git a/tools/config.json b/tools/config.json index 6a220149..2f4fe670 100644 --- a/tools/config.json +++ b/tools/config.json @@ -277,7 +277,8 @@ "delete-news-item", "get-user-emails", "delete-membership-application-as-admin", - "retry-creating-message-contract" + "retry-creating-message-contract", + "manage-json-schemas" ] } },