diff --git a/clients/dashboard/src/components/layout/nav-data.ts b/clients/dashboard/src/components/layout/nav-data.ts index 6b5f7f7290..653ee15e12 100644 --- a/clients/dashboard/src/components/layout/nav-data.ts +++ b/clients/dashboard/src/components/layout/nav-data.ts @@ -88,9 +88,12 @@ export const sections: NavSection[] = [ caption: "Identity", icon: Users, items: [ - { to: "/identity/users", label: "Users", icon: Users }, - { to: "/identity/roles", label: "Roles", icon: ShieldCheck }, - { to: "/identity/groups", label: "Groups", icon: UsersRound }, + // Gate the identity-management pages on a manage permission (not View): View Users/Roles/Groups + // are IsBasic so every member holds them (the chat/user picker relies on Users.View), but only + // managers should see these admin pages. Basic lacks the *.Update perms, so the items hide for them. + { to: "/identity/users", label: "Users", icon: Users, perm: "Permissions.Users.Update" }, + { to: "/identity/roles", label: "Roles", icon: ShieldCheck, perm: "Permissions.Roles.Update" }, + { to: "/identity/groups", label: "Groups", icon: UsersRound, perm: "Permissions.Groups.Update" }, ], }, { diff --git a/src/Modules/Identity/Modules.Identity/Authorization/Jwt/JwtAuthenticationExtensions.cs b/src/Modules/Identity/Modules.Identity/Authorization/Jwt/JwtAuthenticationExtensions.cs index 2404e06f72..d6cae8e680 100644 --- a/src/Modules/Identity/Modules.Identity/Authorization/Jwt/JwtAuthenticationExtensions.cs +++ b/src/Modules/Identity/Modules.Identity/Authorization/Jwt/JwtAuthenticationExtensions.cs @@ -25,6 +25,17 @@ internal static IServiceCollection ConfigureJwtAuth(this IServiceCollection serv services.AddAuthorizationBuilder().AddRequiredPermissionPolicy(); services.AddAuthorization(options => { + // Permission evaluation lives in the RequiredPermission policy (it reads each + // endpoint's RequiredPermissionAttribute metadata). Wire it as BOTH the default + // AND the fallback policy: + // - FallbackPolicy covers endpoints with no auth metadata at all. + // - DefaultPolicy covers endpoints that opt in via .RequireAuthorization() — + // including the module route-groups (Catalog/Billing/Chat/Files/…). Without + // this, a group-level .RequireAuthorization() applied the built-in + // authenticated-only default, which SUPPRESSED the fallback, so + // .RequirePermission(...) was never evaluated and any authenticated tenant + // member could perform gated writes. Both must point at the permission policy. + options.DefaultPolicy = options.GetPolicy(RequiredPermissionDefaults.PolicyName)!; options.FallbackPolicy = options.GetPolicy(RequiredPermissionDefaults.PolicyName); }); return services; diff --git a/src/Tests/Architecture.Tests/Architecture.Tests.csproj b/src/Tests/Architecture.Tests/Architecture.Tests.csproj index ff69562c3d..14572d8b25 100644 --- a/src/Tests/Architecture.Tests/Architecture.Tests.csproj +++ b/src/Tests/Architecture.Tests/Architecture.Tests.csproj @@ -5,7 +5,8 @@ false enable enable - $(NoWarn);CA1515;CA1861;CA1707;CA1307 + + $(NoWarn);CA1515;CA1861;CA1707;CA1307;S125 diff --git a/src/Tests/Integration.Tests/Integration.Tests.csproj b/src/Tests/Integration.Tests/Integration.Tests.csproj index 08470f2940..1aea12e3ab 100644 --- a/src/Tests/Integration.Tests/Integration.Tests.csproj +++ b/src/Tests/Integration.Tests/Integration.Tests.csproj @@ -10,8 +10,9 @@ CA1515 - internal types; CA1861 - const arrays; CA1707 - underscores in test names; CA1307 - StringComparison; CA2234 - Uri vs string; CA2000 - dispose (test scope); CA1062 - null checks in test helpers; CA1031 - broad catch in test setup; - CA1812 - DI-instantiated test doubles; S1481 - deliberately unused locals in tests --> - $(NoWarn);CA1515;CA1861;CA1707;CA1307;CA2234;CA2000;CA1062;CA1031;CA1812;CA1056;S1481 + CA1812 - DI-instantiated test doubles; S1481 - deliberately unused locals in tests; + S125 - false-positives on descriptive test comments containing code-like tokens --> + $(NoWarn);CA1515;CA1861;CA1707;CA1307;CA2234;CA2000;CA1062;CA1031;CA1812;CA1056;S1481;S125 diff --git a/src/Tests/Integration.Tests/Tests/Catalog/ProductsEndpointTests.cs b/src/Tests/Integration.Tests/Tests/Catalog/ProductsEndpointTests.cs index 2365dd7ae9..462d7cfc33 100644 --- a/src/Tests/Integration.Tests/Tests/Catalog/ProductsEndpointTests.cs +++ b/src/Tests/Integration.Tests/Tests/Catalog/ProductsEndpointTests.cs @@ -1,6 +1,11 @@ +using Finbuckle.MultiTenant; +using Finbuckle.MultiTenant.Abstractions; +using FSH.Framework.Shared.Multitenancy; using FSH.Modules.Catalog.Contracts.Dtos; +using FSH.Modules.Identity.Domain; using Integration.Tests.Infrastructure; using Integration.Tests.Infrastructure.Extensions; +using Microsoft.AspNetCore.Identity; namespace Integration.Tests.Tests.Catalog; @@ -469,6 +474,68 @@ public async Task ListTrashedProducts_Should_Return401_When_Unauthenticated() response.StatusCode.ShouldBe(HttpStatusCode.Unauthorized); } + [Fact] + public async Task CreateProduct_Should_Return403_When_AuthenticatedUserLacksPermission() + { + // Regression (broken access control): catalog endpoints sit behind a group-level + // .RequireAuthorization(), whose default policy only required authentication. That + // shadowed the permission FallbackPolicy, so .RequirePermission(Products.Create) was + // never evaluated — ANY authenticated tenant member could create products. A seeded + // user holding no catalog-write permission MUST be Forbidden, not fail open. + var (email, password) = await CreateActiveNoPermissionUserAsync($"noperm{Guid.NewGuid():N}"[..14]); + using var userClient = await _auth.CreateAuthenticatedClientAsync(email, password); + + using var response = await userClient.PostAsJsonAsync( + $"{TestConstants.CatalogBasePath}/products", + new + { + sku = UniqueSku("Forbidden"), + name = "no-permission user must not create products", + description = (string?)null, + brandId = Guid.NewGuid(), + categoryId = Guid.NewGuid(), + priceAmount = 1m, + priceCurrency = "USD", + stock = 1, + }); + + response.StatusCode.ShouldBe(HttpStatusCode.Forbidden, + "Catalog permission gates must not fail open: a user without Permissions.Products.Create must be rejected."); + } + + /// + /// Seeds a confirmed + active root-tenant user with NO roles (hence no catalog + /// permissions) via UserManager. Tenant context is set INLINE because it is AsyncLocal — + /// an awaited-helper set would be lost and the tenant query filter would throw. + /// + private async Task<(string Email, string Password)> CreateActiveNoPermissionUserAsync(string handle) + { + const string password = TestConstants.DefaultPassword; + var email = $"{handle}@example.com"; + + using var scope = _factory.Services.CreateScope(); + var tenant = await scope.ServiceProvider + .GetRequiredService>() + .GetAsync(TestConstants.RootTenantId); + scope.ServiceProvider.GetRequiredService() + .MultiTenantContext = new MultiTenantContext(tenant); + + var userManager = scope.ServiceProvider.GetRequiredService>(); + var user = new FshUser + { + FirstName = "No", + LastName = "Perm", + Email = email, + UserName = handle, + EmailConfirmed = true, + IsActive = true, + }; + var result = await userManager.CreateAsync(user, password); + result.Succeeded.ShouldBeTrue( + $"Seeding active user failed: {string.Join(", ", result.Errors.Select(e => e.Description))}"); + return (email, password); + } + // ─── idempotency ───────────────────────────────────────────────── [Fact]