From eeaed68eb3b485a43d5fde8a0116c5fd3238a98b Mon Sep 17 00:00:00 2001 From: iammukeshm Date: Sat, 6 Jun 2026 00:26:56 +0530 Subject: [PATCH 1/2] fix(identity): enforce permissions on .RequireAuthorization() endpoints (broken access control) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Permission checks live in the "RequiredPermission" authorization policy, which reads each endpoint's RequiredPermissionAttribute (added by .RequirePermission(...)). It was wired ONLY as the FallbackPolicy. In ASP.NET Core the fallback policy runs only on endpoints with no authorization metadata — but Catalog, Billing, Chat, Files, Notifications, Tickets and Webhooks all call .RequireAuthorization() at the route-group level. That attaches the built-in default policy (authenticated-only), which suppressed the fallback, so .RequirePermission(...) was attached as metadata but NEVER evaluated. Result: any authenticated tenant member could perform gated writes in those modules — e.g. a Basic user creating/deleting products despite lacking Products.Create/Delete. (Identity/Auditing/Multitenancy have no group-level .RequireAuthorization(), so their fallback ran and they were unaffected.) Fix: set DefaultPolicy to the same RequiredPermission policy as the fallback, so .RequireAuthorization() (default policy) also evaluates permissions. Endpoints without permission metadata still just require authentication (the handler succeeds when no RequiredPermissionAttribute is present). One-line wiring change fixes all 7 modules. Adds a regression test (CreateProduct_Should_Return403_When_AuthenticatedUserLacksPermission) covering the suite's blind spot: it previously only tested admin->200 and unauthenticated->401, never authenticated-but-forbidden->403. Verified red (2xx) before the fix, green (403) after; the full ProductsEndpointTests class stays green (admin create/update/delete/restore/search unaffected). Co-Authored-By: Claude Opus 4.8 (1M context) --- .../Jwt/JwtAuthenticationExtensions.cs | 11 +++ .../Tests/Catalog/ProductsEndpointTests.cs | 67 +++++++++++++++++++ 2 files changed, 78 insertions(+) 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/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] From bbfb966ce462417b192798a6d48a7f7fa6536854 Mon Sep 17 00:00:00 2001 From: iammukeshm Date: Sat, 6 Jun 2026 00:41:28 +0530 Subject: [PATCH 2/2] fix(deploy+dashboard): unblock CI (S125) and hide identity-admin nav from Basic users MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CI: the Release `-warnaserror` build was already red on main — SonarAnalyzer S125 false-positives on 6 descriptive test comments (semicolon/code-like prose, not dead code) in Integration.Tests and Architecture.Tests. Suppress S125 in those two test projects (deleting the comments would lose useful documentation). Dashboard: the Users/Roles/Groups nav items had no permission gate, so they showed to everyone. Gate each on its *.Update permission. Basic users lack Update, so the identity-admin pages hide for them — without touching View Users/Roles/Groups (those stay IsBasic because the chat/user picker depends on Users.View). Co-Authored-By: Claude Opus 4.8 (1M context) --- clients/dashboard/src/components/layout/nav-data.ts | 9 ++++++--- src/Tests/Architecture.Tests/Architecture.Tests.csproj | 3 ++- src/Tests/Integration.Tests/Integration.Tests.csproj | 5 +++-- 3 files changed, 11 insertions(+), 6 deletions(-) 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/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