Merge pull request #1305 from fullstackhero/chore/pre-publish-cleanup

iammukeshm · web-flow · commit 44412b23675b · 2026-06-20T03:47:22.000+05:30
chore: pre-publish cleanup — group-permission fix, fetched-permissions docs, .NET 10 nits
diff --git a/.agents/rules/frontend/dashboard.md b/.agents/rules/frontend/dashboard.md
@@ -10,9 +10,9 @@ Tenant-facing application. Read `frontend/shared.md` first; this file is only th
 
 The dashboard does **not** depend on react-hook-form or zod. Use controlled inputs + local state. Don't add those deps to match admin.
 
-## Permissions — straight from the JWT
+## Permissions — fetched, not in the JWT
 
-`auth-context.tsx` reads `claims.permissions` off the decoded JWT — no separate fetch, no `permissionsHydrated`, no permissions cache key. `ProtectedRoute` is **auth-only** (no permission gating). Don't add `RouteGuard`-style gating here.
+The JWT carries **only role names**. `auth-context.tsx` fetches the effective permission list from `GET /api/v1/identity/permissions` (`getMyPermissions()` in `src/api/identity.ts`), caches it in `tokenStore` under `fsh.dashboard.permissions`, and exposes a `permissionsHydrated` flag so gated UI doesn't flash while the fetch is in flight (re-fetched on login/impersonation swaps; `refreshPermissions()` for role changes). Nav items are gated via `perm`/`anyPerm` in `src/components/layout/nav-data.ts`. `ProtectedRoute` is still **auth-only** (no per-route permission gating) — don't add `RouteGuard`-style gating here.
 
 ## Routing & realtime/SSE
 
diff --git a/.agents/skills/add-permission/SKILL.md b/.agents/skills/add-permission/SKILL.md
@@ -61,7 +61,7 @@ So `RouteGuard` passes on first paint, add the new permission to the test seed s
 
 ## Dashboard
 
-No mirror, no `RouteGuard`. The permission rides in the JWT (`claims.permissions`) and the server enforces it; a missing permission yields a 403 the UI surfaces. Nothing to add client-side beyond consuming the gated endpoint.
+No mirror, no `RouteGuard`. The JWT carries only role names — the app fetches the permission list from `GET /api/v1/identity/permissions` at hydration and the server enforces access; a missing permission yields a 403 the UI surfaces. Routes aren't permission-gated; to hide a nav entry, set `perm`/`anyPerm` on the item in `src/components/layout/nav-data.ts`. Permission-gated specs mock `GET /identity/permissions` with the grants they need (shell mocks stub it to `[]`).
 
 ## Checklist
 
diff --git a/.agents/skills/add-react-page/SKILL.md b/.agents/skills/add-react-page/SKILL.md
@@ -17,7 +17,7 @@ The frontend slice. Read `.agents/rules/frontend/shared.md` plus the app file (`
 | Forms | **react-hook-form + zod** | **hand-rolled** controlled inputs (no RHF/zod) |
 | List + create | separate routed pages (`list.tsx`, `create.tsx`) | one file with `<Dialog>` editors |
 | Route wrapper | `<RouteGuard perms={[…]}>` | `withSuspense(<X/>)` (no permission gate) |
-| Permissions | mirror in `src/lib/permissions.ts` | none — JWT claims + server 403 |
+| Permissions | mirror in `src/lib/permissions.ts` | fetched from `GET /identity/permissions` (not JWT); nav gating via `perm`/`anyPerm` in `nav-data.ts`; no route guard — server 403 backstops |
 
 Shared everywhere: types are **hand-written** (no codegen); `apiFetch<T>` from `@/lib/api-client`; `cn()` from `@/lib/cn`; `env.apiBase` from runtime `/config.json`; CVA `components/ui` + `components/list` primitives; Tailwind v4 CSS-first (tokens in `src/styles/globals.css`); `toast` from `sonner`; pages are **named exports**; `placeholderData: keepPreviousData` (v5).
 
diff --git a/.devcontainer/devcontainer.json b/.devcontainer/devcontainer.json
@@ -4,7 +4,7 @@
     "features": {
         "ghcr.io/devcontainers/features/docker-in-docker:2": {}
     },
-    "postCreateCommand": "dotnet workload install aspire && dotnet restore src/FSH.Starter.slnx",
+    "postCreateCommand": "dotnet restore src/FSH.Starter.slnx",
     "forwardPorts": [5030, 7030, 15888],
     "customizations": {
         "vscode": {
diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md
@@ -10,7 +10,7 @@ Thanks for helping out. The conventions below keep PRs reviewable.
 
 ## Dev setup
 
-Prerequisites: .NET 10 SDK, Docker, Node.js 22+.
+Prerequisites: .NET 10 SDK, Docker, Node.js 20+.
 
 ```bash
 dotnet build src/FSH.Starter.slnx
diff --git a/README-template.md b/README-template.md
@@ -12,7 +12,6 @@ the shared code lives in `src/BuildingBlocks` and is yours to change.
 - [.NET 10 SDK](https://dotnet.microsoft.com/download/dotnet/10.0)
 - [Node.js 20+](https://nodejs.org) — for the React apps
 - [Docker](https://www.docker.com/) — Postgres, Redis, MinIO (orchestrated by Aspire)
-- .NET Aspire workload: `dotnet workload install aspire`
 
 ## Quick start
 
diff --git a/clients/dashboard/tests/helpers/auth-seed.ts b/clients/dashboard/tests/helpers/auth-seed.ts
@@ -7,15 +7,23 @@ import type { Page } from "@playwright/test";
  * We populate the same localStorage keys the runtime tokenStore writes
  * (see clients/dashboard/src/auth/token-store.ts). The token value is
  * a JWT-shaped string that decodes to the supplied user — useAuth's
- * decoder reads sub/email/given_name/family_name/tenant/permissions
- * out of the payload.
+ * decoder reads sub/email/given_name/family_name/tenant out of the
+ * payload. Permissions are NOT read from the JWT: at runtime the app
+ * fetches them from GET /api/v1/identity/permissions (installShellMocks
+ * stubs that route to []; permission-gated specs re-mock it with the
+ * grants they need — see tests/system/trash.spec.ts).
  */
 export type SeededUser = {
   sub: string;
   email: string;
   firstName: string;
   lastName: string;
   tenant: string;
+  /**
+   * Inert — written into the fake JWT payload but ignored by the app.
+   * To grant permissions in a spec, mock GET /identity/permissions
+   * instead.
+   */
   permissions?: string[];
 };
 
@@ -36,7 +44,7 @@ function fakeJwt(payload: Record<string, unknown>): string {
 
 export async function seedAuthedSession(page: Page, user: SeededUser) {
   // Build the JWT-shaped payload. Claim names match what useAuth's decoder
-  // looks for in the runtime path.
+  // looks for in the runtime path (permissions excepted — inert, see SeededUser).
   const payload = {
     sub: user.sub,
     email: user.email,
diff --git a/src/Host/FSH.Starter.DbMigrator/MigratorCommand.cs b/src/Host/FSH.Starter.DbMigrator/MigratorCommand.cs
@@ -66,7 +66,7 @@ apply           Apply pending migrations (default). Use --seed to also run SeedA
           seed            Run only the SeedAsync step per tenant.
           seed-demo       Provision the demo tenants (acme, globex) with users, catalog,
                           tickets, and chat. Dev-only — refuses to run unless
-                          ASPNETCORE_ENVIRONMENT=Development.
+                          DOTNET_ENVIRONMENT=Development.
           list-pending    Print pending migrations without applying anything.
 
         Options:
diff --git a/src/Host/FSH.Starter.DbMigrator/Program.cs b/src/Host/FSH.Starter.DbMigrator/Program.cs
@@ -291,7 +291,7 @@ await Console.Out.WriteLineAsync(
         if (!env.IsDevelopment())
         {
             await Console.Error.WriteLineAsync(
-                $"[demo-seed] REFUSING to run — ASPNETCORE_ENVIRONMENT is '{env.EnvironmentName}'. "
+                $"[demo-seed] REFUSING to run — DOTNET_ENVIRONMENT is '{env.EnvironmentName}'. "
                 + "seed-demo is dev-only by design.")
                 .ConfigureAwait(false);
             return 1;
diff --git a/src/Host/FSH.Starter.DbMigrator/README.md b/src/Host/FSH.Starter.DbMigrator/README.md
@@ -43,7 +43,7 @@ dotnet run --project src/Host/FSH.Starter.DbMigrator -- seed
 # Dev only — provision the demo tenants (acme, globex) with users,
 # custom roles, sample catalog, tickets, and chat. Hard-refuses outside
 # Development. Idempotent: safe to re-run.
-ASPNETCORE_ENVIRONMENT=Development \
+DOTNET_ENVIRONMENT=Development \
   dotnet run --project src/Host/FSH.Starter.DbMigrator -- seed-demo
 ```
 
diff --git a/src/Modules/Identity/Modules.Identity/Services/UserPermissionService.cs b/src/Modules/Identity/Modules.Identity/Services/UserPermissionService.cs
@@ -70,11 +70,25 @@ private static async ValueTask<PermissionSet> LoadPermissionsAsync(FactoryState
 
         var userRoles = await s.UserManager.GetRolesAsync(user).ConfigureAwait(false);
 
-        var roleIds = await s.RoleManager.Roles
+        var directRoleIds = await s.RoleManager.Roles
             .Where(r => userRoles.Contains(r.Name!))
             .Select(r => r.Id)
             .ToListAsync(ct).ConfigureAwait(false);
 
+        // Group-derived roles confer permissions too — the JWT already unions them
+        // (IdentityService.AddRoleClaimsAsync) and every group mutation invalidates this
+        // cache entry, so the effective set must include roles reachable via UserGroups.
+        var groupRoleIds = await s.Db.GroupRoles
+            .Where(gr => s.Db.UserGroups
+                .Where(ug => ug.UserId == s.UserId)
+                .Select(ug => ug.GroupId)
+                .Contains(gr.GroupId))
+            .Select(gr => gr.RoleId)
+            .Distinct()
+            .ToListAsync(ct).ConfigureAwait(false);
+
+        var roleIds = directRoleIds.Union(groupRoleIds, StringComparer.Ordinal).ToList();
+
         if (roleIds.Count == 0)
         {
             return PermissionSet.Empty;
diff --git a/src/Tests/Architecture.Tests/AuthorizationMetadataTests.cs b/src/Tests/Architecture.Tests/AuthorizationMetadataTests.cs
@@ -0,0 +1,96 @@
+using FSH.Framework.Shared.Identity.Authorization;
+using Shouldly;
+using System.Reflection;
+using Xunit;
+
+namespace Architecture.Tests;
+
+/// <summary>
+/// Guards the permission-metadata contract between <c>RequiredPermissionAttribute</c> and
+/// <c>RequiredPermissionAuthorizationHandler</c>. The handler resolves endpoint permissions via
+/// <see cref="IRequiredPermissionMetadata"/>; if a duplicate <c>RequiredPermissionAttribute</c>
+/// appears in another assembly without implementing that interface, endpoints decorated with it
+/// carry no recognizable metadata and every <c>.RequirePermission()</c> gate silently fails open.
+/// </summary>
+public class AuthorizationMetadataTests
+{
+    private const string AttributeName = "RequiredPermissionAttribute";
+    private const string ExpectedNamespace = "FSH.Framework.Shared.Identity.Authorization";
+
+    [Fact]
+    public void RequiredPermissionAttribute_Should_Exist_Exactly_Once_Across_All_FSH_Assemblies()
+    {
+        var matches = GetAllFshAssemblies()
+            .SelectMany(GetLoadableTypes)
+            .Where(t => string.Equals(t.Name, AttributeName, StringComparison.Ordinal))
+            .ToArray();
+
+        matches.ShouldNotBeEmpty(
+            $"{AttributeName} was not found in any FSH assembly. " +
+            "The permission authorization pipeline depends on it.");
+
+        matches.Length.ShouldBe(1,
+            $"Exactly one {AttributeName} must exist across all FSH assemblies. " +
+            "A duplicate that does not implement IRequiredPermissionMetadata silently disables " +
+            $"every .RequirePermission() gate. Found: {string.Join(", ", matches.Select(t => $"{t.FullName} ({t.Assembly.GetName().Name})"))}");
+
+        matches[0].Namespace.ShouldBe(ExpectedNamespace,
+            $"{AttributeName} must live in {ExpectedNamespace}, where " +
+            "RequiredPermissionAuthorizationHandler resolves its metadata from.");
+    }
+
+    [Fact]
+    public void RequiredPermissionAttribute_Should_Implement_IRequiredPermissionMetadata()
+    {
+        var attributeType = typeof(RequiredPermissionAttribute);
+
+        typeof(IRequiredPermissionMetadata).IsAssignableFrom(attributeType).ShouldBeTrue(
+            $"{attributeType.FullName} must implement IRequiredPermissionMetadata. " +
+            "RequiredPermissionAuthorizationHandler discovers endpoint permissions through that " +
+            "interface; without it, every .RequirePermission() gate silently fails open.");
+    }
+
+    /// <summary>
+    /// Loads every FSH.* assembly deployed alongside the tests so the duplicate sweep covers
+    /// BuildingBlocks, all modules (including Contracts), and host assemblies — not just the
+    /// runtime module assemblies that ModuleAssemblyDiscovery returns.
+    /// </summary>
+    private static Assembly[] GetAllFshAssemblies()
+    {
+        string baseDir = AppContext.BaseDirectory;
+
+        var assemblies = new List<Assembly>();
+
+        foreach (var file in Directory.GetFiles(baseDir, "FSH.*.dll"))
+        {
+            try
+            {
+                var assemblyName = AssemblyName.GetAssemblyName(file);
+                assemblies.Add(Assembly.Load(assemblyName));
+            }
+#pragma warning disable CA1031 // Do not catch general exception types
+            catch (Exception)
+            {
+                // Skip if not a valid .NET assembly or other load error
+            }
+#pragma warning restore CA1031
+        }
+
+        assemblies.ShouldNotBeEmpty(
+            "No FSH.* assemblies were found in the test output directory; the duplicate sweep would be a no-op.");
+
+        return [.. assemblies];
+    }
+
+    private static IEnumerable<Type> GetLoadableTypes(Assembly assembly)
+    {
+        try
+        {
+            return assembly.GetTypes();
+        }
+        catch (ReflectionTypeLoadException ex)
+        {
+            return ex.Types.Where(t => t is not null)!;
+        }
+    }
+}
diff --git a/src/Tests/Integration.Tests/Tests/Groups/GroupRolePermissionTests.cs b/src/Tests/Integration.Tests/Tests/Groups/GroupRolePermissionTests.cs
diff --git a/src/Tools/CLI/Program.cs b/src/Tools/CLI/Program.cs

Original file line number	Diff line number	Diff line change
`@@ -291,7 +291,7 @@ await Console.Out.WriteLineAsync(`
`291`	`291`	`if (!env.IsDevelopment())`
`292`	`292`	`{`
`293`	`293`	`await Console.Error.WriteLineAsync(`
`294`		`- $"[demo-seed] REFUSING to run — ASPNETCORE_ENVIRONMENT is '{env.EnvironmentName}'. "`
	`294`	`+ $"[demo-seed] REFUSING to run — DOTNET_ENVIRONMENT is '{env.EnvironmentName}'. "`
`295`	`295`	`+ "seed-demo is dev-only by design.")`
`296`	`296`	`.ConfigureAwait(false);`
`297`	`297`	`return 1;`