From 8d55049f51b6e5d150e1611273615cc077dd02de Mon Sep 17 00:00:00 2001 From: Mobeen Abdullah Date: Fri, 11 Sep 2026 13:31:26 +0300 Subject: [PATCH 1/4] fix(nextly): a super-admin's key copies the catalogue, and a plugin caller has roles A read-only or full-access key copies its owner's permission rows, and a super-admin's power is a bypass rather than rows: the role is granted the permissions that exist at setup and nothing grants it the ones a later collection adds, while the session never needs them. So a super-admin's key held a stale subset at best and, on an install whose first user came before the grant, nothing, which refused every request with the first key an operator mints. Such a key copies the catalogue now, every permission the install declares with orphans left out, bounded by the key's kind like anyone else's: read-only stays read-only. The super-admin check the service already ran for role assignment is the one it runs here. On the plugin path, resolveServiceOpts built the caller with role: "", a documented limitation, so a code-defined rule reading req.user.role refused every caller while the same caller's own request passed, and a negative rule granted what it was written to refuse. The roles are resolved and the caller is built by buildUserContext, the constructor every other authenticated path uses; the lookup is injectable, as resolve-run-as already has it, so the translation is tested without a database. Proven end to end through the real catch-all: a super-admin's own read-only key reads through a plugin route and cannot write through one. --- ...a-super-admins-key-copies-the-catalogue.md | 44 ++++++ docs/guides/authentication.mdx | 2 + .../__tests__/api-key-token-types.test.ts | 126 ++++++++++++++++++ .../domains/auth/services/api-key-service.ts | 77 ++++++++--- ...plugin-route-key-scope.integration.test.ts | 101 +++++++++++++- .../src/plugins/service-opts-wrapper.test.ts | 36 ++++- .../nextly/src/plugins/service-opts.test.ts | 108 +++++++++++---- packages/nextly/src/plugins/service-opts.ts | 61 +++++++-- .../nextly/src/schemas/api-keys/postgres.ts | 3 +- 9 files changed, 489 insertions(+), 69 deletions(-) create mode 100644 .changeset/a-super-admins-key-copies-the-catalogue.md diff --git a/.changeset/a-super-admins-key-copies-the-catalogue.md b/.changeset/a-super-admins-key-copies-the-catalogue.md new file mode 100644 index 0000000000..0a5871974e --- /dev/null +++ b/.changeset/a-super-admins-key-copies-the-catalogue.md @@ -0,0 +1,44 @@ +--- + +"@nextlyhq/adapter-drizzle": patch +"@nextlyhq/adapter-mysql": patch +"@nextlyhq/adapter-postgres": patch +"@nextlyhq/adapter-sqlite": patch +"@nextlyhq/admin": patch +"@nextlyhq/admin-css": patch +"@nextlyhq/blocks-engine": patch +"@nextlyhq/blocks-react": patch +"@nextlyhq/builder": patch +"create-nextly-app": patch +"@nextlyhq/eslint-config": patch +"@nextlyhq/eslint-plugin": patch +"@nextlyhq/module-specifiers": patch +"nextly": patch +"@nextlyhq/plugin-form-builder": patch +"@nextlyhq/plugin-page-builder": patch +"@nextlyhq/plugin-sdk": patch +"@nextlyhq/plugin-seo": patch +"@nextlyhq/prettier-config": patch +"@nextlyhq/storage-s3": patch +"@nextlyhq/storage-uploadthing": patch +"@nextlyhq/storage-vercel-blob": patch +"@nextlyhq/telemetry": patch +"@nextlyhq/tsconfig": patch +"@nextlyhq/ui": patch + +An API key created by a Super Admin copies the permission catalogue rather +than the role's rows. A Super Admin's power is a bypass, and the role only +holds the permissions that existed when the install was set up, so a key of +theirs held a stale subset at best and, where the first user came before the +grant, nothing: every request refused, starting with the first key an operator +minted to try an integration with. A `read-only` key of theirs now reads every +collection the install declares, including one added later, and still cannot +write; a `full-access` key holds every permission. A permission a package +stopped declaring is not inherited. + +A plugin calling `ctx.services` as a user now sees that user's roles. The +caller was built with an empty role, so a code-defined rule such as +`req.user?.role === "editor"` refused every caller on the plugin path while +the same caller's own request passed it, and a negative rule granted what it +was written to refuse. The roles are resolved and the caller is built by the +one constructor every other authenticated path uses. diff --git a/docs/guides/authentication.mdx b/docs/guides/authentication.mdx index 43e12156d4..83531c3f37 100644 --- a/docs/guides/authentication.mdx +++ b/docs/guides/authentication.mdx @@ -166,6 +166,8 @@ Nextly supports API keys via the `Authorization: Bearer nx_live_...` header (no | `full-access` | All permissions of the creator's roles. | | `role-based` | Permissions of an explicitly chosen role (independent of the creator). | +A key created by a Super Admin copies the whole permission catalogue rather than the role's rows, because the Super Admin's power is a bypass and the role only holds the permissions that existed when the install was set up: a `read-only` key of theirs can read every collection, including one added later, and still cannot write. + Keys can be time-bound (set an expiry date) and revoked at any time. The full key value is shown to the operator **once** at creation; the database only stores the SHA-256 hash. ### How verification works diff --git a/packages/nextly/src/domains/auth/__tests__/api-key-token-types.test.ts b/packages/nextly/src/domains/auth/__tests__/api-key-token-types.test.ts index 002da162a3..d24cdf6339 100644 --- a/packages/nextly/src/domains/auth/__tests__/api-key-token-types.test.ts +++ b/packages/nextly/src/domains/auth/__tests__/api-key-token-types.test.ts @@ -48,6 +48,7 @@ function createTestAdapter(db: unknown) { const KEY_A = "test-key-a"; const KEY_B = "test-key-b"; const KEY_CACHE = "test-key-cache"; +const KEY_SUPER = "test-key-super"; // ───────────────────────────────────────────────────────────────────────────── // Test suite @@ -208,6 +209,7 @@ describe("ApiKeyService – Token Type Permission Resolution", () => { invalidateApiKeyPermissionsCache(KEY_A); invalidateApiKeyPermissionsCache(KEY_B); invalidateApiKeyPermissionsCache(KEY_CACHE); + invalidateApiKeyPermissionsCache(KEY_SUPER); await testDb.reset(); testDb.close(); vi.resetAllMocks(); @@ -373,6 +375,130 @@ describe("ApiKeyService – Token Type Permission Resolution", () => { }); }); + // ── a super-admin's key ──────────────────────────────────────────────── + describe("a key created by a super-admin", () => { + /** + * A super-admin whose role holds NO permission rows, which is what an + * install looks like whose first user came before the setup grant, and + * what every install drifts toward: the role is granted the rows that + * exist at setup and never the ones a later collection adds. Their power + * is the bypass, so the session never notices; a key copies the rows. + */ + let superAdminId: string; + + beforeEach(async () => { + superAdminId = randomUUID(); + const superAdminRoleId = randomUUID(); + await testDb.db.insert(testDb.schema.users).values({ + id: superAdminId, + email: `super-${superAdminId}@example.com`, + isActive: true, + }); + await testDb.db.insert(testDb.schema.roles).values({ + id: superAdminRoleId, + name: "Super Admin", + slug: "super-admin", + level: 100, + isSystem: true, + }); + await testDb.db.insert(testDb.schema.userRoles).values({ + id: randomUUID(), + userId: superAdminId, + roleId: superAdminRoleId, + }); + // A permission a package stopped declaring: kept in the table so a + // grant survives, and never copied to a key that inherits nothing else new. + await testDb.db.insert(testDb.schema.permissions).values({ + id: randomUUID(), + name: "Read Legacy", + slug: "read-legacy", + action: "read", + resource: "legacy", + orphanedAt: new Date(), + }); + }); + + it("read-only: holds every read-* permission the install declares, from the catalogue", async () => { + const slugs = await service.resolveApiKeyPermissions( + "read-only", + null, + superAdminId, + KEY_SUPER + ); + expect([...slugs].sort()).toEqual([ + "read-media", + "read-posts", + "read-users", + ]); + }); + + it("read-only: still cannot write, whoever created it", async () => { + const slugs = await service.resolveApiKeyPermissions( + "read-only", + null, + superAdminId, + KEY_SUPER + ); + expect(slugs.some(s => !s.startsWith("read-"))).toBe(false); + }); + + it("full-access: holds every permission the install declares", async () => { + const slugs = await service.resolveApiKeyPermissions( + "full-access", + null, + superAdminId, + KEY_SUPER + ); + expect([...slugs].sort()).toEqual([ + "create-posts", + "delete-posts", + "read-media", + "read-posts", + "read-users", + "update-posts", + ]); + }); + + it("holds a permission added after the role was granted, which no role copy would", async () => { + // The drift the catalogue exists for: a collection added after setup. + await testDb.db.insert(testDb.schema.permissions).values({ + id: randomUUID(), + name: "Read Events", + slug: "read-events", + action: "read", + resource: "events", + }); + const slugs = await service.resolveApiKeyPermissions( + "read-only", + null, + superAdminId, + KEY_SUPER + ); + expect(slugs).toContain("read-events"); + }); + + it("never inherits a permission the install stopped declaring", async () => { + const slugs = await service.resolveApiKeyPermissions( + "full-access", + null, + superAdminId, + KEY_SUPER + ); + expect(slugs).not.toContain("read-legacy"); + }); + + it("is the control that an ordinary creator still copies their own rows only", async () => { + // The editor's read-only key is judged the way it was: their rows, + // not the catalogue. `read-media` is in the catalogue and not theirs. + const slugs = await service.resolveApiKeyPermissions( + "read-only", + null, + userId, + KEY_A + ); + expect(slugs).not.toContain("read-media"); + }); + }); // ── role-based ───────────────────────────────────────────────────────── describe("role-based token type", () => { diff --git a/packages/nextly/src/domains/auth/services/api-key-service.ts b/packages/nextly/src/domains/auth/services/api-key-service.ts index da6ed93295..1901dbe2cf 100644 --- a/packages/nextly/src/domains/auth/services/api-key-service.ts +++ b/packages/nextly/src/domains/auth/services/api-key-service.ts @@ -35,7 +35,7 @@ import { createHash, randomBytes, randomUUID } from "crypto"; import type { DrizzleAdapter } from "@nextlyhq/adapter-drizzle"; -import { and, desc, eq, inArray } from "drizzle-orm"; +import { and, desc, eq, inArray, isNull } from "drizzle-orm"; import type { GrantedPermission } from "../../../auth/authenticated-scope"; import { toDbError } from "../../../database/errors"; @@ -670,6 +670,11 @@ export class ApiKeyService extends BaseService { * - **role-based** — the assigned role's permission set. * If the role has been deleted (`roleId === null`), returns `[]` and logs a warning. * + * A super-admin creator's "full permission set" is the catalogue, every + * permission the install declares, because a super-admin's power is a bypass + * and the role's own rows are the ones that existed at setup. The key's kind + * still bounds it: read-only stays read-only. + * * @param tokenType - The key's token type * @param roleId - The assigned role ID (only relevant for "role-based" keys) * @param userId - The key creator's user ID @@ -742,7 +747,17 @@ export class ApiKeyService extends BaseService { } rows = await this.resolveRolePermissionRows(roleId); } else { - const all = await this.resolveUserPermissionRows(userId); + // A super-admin's power is a bypass, not a list. The role is granted + // every permission that exists when the install is set up and nothing + // grants it the ones a later collection adds, while the session never + // needs the rows. A key copies the rows, so a key of theirs held a stale + // subset at best and, on an install whose first user came before the + // grant, nothing: every request refused, and the first key an operator + // mints to try an integration with. The catalogue is what their key + // copies, bounded by the key's kind like anyone else's. + const all = (await this.ownerIsSuperAdmin(userId)) + ? await this.resolveCataloguePermissionRows() + : await this.resolveUserPermissionRows(userId); // Still filtered on the STORED slug rather than on `action === "read"`. // A deliberately custom slug is supported, so the two disagree — a row // named `view-dashboard` on action `read` is excluded by the slug test @@ -837,6 +852,46 @@ export class ApiKeyService extends BaseService { return rows as GrantedPermission[]; } + /** + * Every permission the install declares today, orphans left out. + * + * Read from the catalogue rather than from any role, because no role is + * kept complete: the super-admin role is granted the rows that exist at + * setup and never the ones a later collection adds. A permission a package + * stopped declaring stays in the table, marked, so a grant survives; a + * super-admin's key does not inherit it, since nothing else new can. + */ + private async resolveCataloguePermissionRows(): Promise { + const rows = await this.db + .select({ + slug: this.permissionsTable.slug, + action: this.permissionsTable.action, + resource: this.permissionsTable.resource, + }) + .from(this.permissionsTable) + .where(isNull(this.permissionsTable.orphanedAt)); + return rows as GrantedPermission[]; + } + + /** Whether a user holds the super-admin role, by the slug every other check reads. */ + private async ownerIsSuperAdmin(userId: string): Promise { + const rows = await this.db + .select({ id: this.rolesTable.id }) + .from(this.userRolesTable) + .innerJoin( + this.rolesTable, + eq(this.userRolesTable.roleId, this.rolesTable.id) + ) + .where( + and( + eq(this.userRolesTable.userId, userId), + eq(this.rolesTable.slug, "super-admin") + ) + ) + .limit(1); + return (rows as unknown[]).length > 0; + } + private async resolveUserPermissionRows( userId: string ): Promise { @@ -1007,23 +1062,7 @@ export class ApiKeyService extends BaseService { if (!roleId) return; // Super-admin bypass: a super-admin can assign any role - - const superAdminCheck = await this.db - .select({ id: this.rolesTable.id }) - .from(this.userRolesTable) - .innerJoin( - this.rolesTable, - eq(this.userRolesTable.roleId, this.rolesTable.id) - ) - .where( - and( - eq(this.userRolesTable.userId, creatorId), - eq(this.rolesTable.slug, "super-admin") - ) - ) - .limit(1); - - if ((superAdminCheck as unknown[]).length > 0) return; + if (await this.ownerIsSuperAdmin(creatorId)) return; const creatorRoleRows = await this.db .select({ roleId: this.userRolesTable.roleId }) diff --git a/packages/nextly/src/plugins/routes/__tests__/plugin-route-key-scope.integration.test.ts b/packages/nextly/src/plugins/routes/__tests__/plugin-route-key-scope.integration.test.ts index e453d2bc55..3c964277ff 100644 --- a/packages/nextly/src/plugins/routes/__tests__/plugin-route-key-scope.integration.test.ts +++ b/packages/nextly/src/plugins/routes/__tests__/plugin-route-key-scope.integration.test.ts @@ -127,13 +127,12 @@ afterEach(async () => { * A `viewer` role holding ONLY `read-posts`, and a role-based key scoped to it, * minted by the super-admin first user. * - * Role-based rather than read-only because `resolveApiKeyPermissions` derives a - * read-only key's grants from its OWNER's enumerated slugs — and a super-admin - * has none, its power being an implicit bypass rather than a list. Such a key - * resolves to an empty scope and is refused everything, which cannot separate - * "held to its grant" from "denied outright". A role-based key resolves from - * the ROLE, so it holds exactly `read-posts` and both directions are testable. - * It is also the vector `auth/authenticated-scope.ts` names in its own header. + * Role-based so the key holds exactly `read-posts`, which makes both directions + * testable against one grant. It is also the vector `auth/authenticated-scope.ts` + * names in its own header. The super-admin's own read-only key is the other + * case below: it used to resolve to an empty scope, because a super-admin's + * role holds the rows that existed at setup and their power is the bypass, so + * the first key an operator minted was refused everything. */ async function viewerKeyOwnedBySuperAdmin(): Promise { const nextly = handle!.nextly as unknown as { @@ -225,6 +224,94 @@ async function viewerKeyOwnedBySuperAdmin(): Promise { let ownerId = ""; +/** + * The super-admin first user's OWN read-only key, the first key an operator + * mints to try an integration with. Its grants are the catalogue's read + * permissions rather than the role's rows, so it reads whatever the install + * declares, and still cannot write. + */ +async function readOnlyKeyOwnedBySuperAdmin(): Promise { + const nextly = handle!.nextly as unknown as { + users: { + create: (a: { data: Record }) => Promise<{ + item: { id: string }; + }>; + }; + }; + const owner = await nextly.users.create({ + data: { + email: "owner@example.com", + password: "Password123!", + name: "Owner", + isActive: true, + }, + }); + const apiKeys = handle!.getService("apiKeyService") as unknown as { + createApiKey: ( + userId: string, + input: { name: string; tokenType: string; expiresIn: string } + ) => Promise<{ key: string; meta: { id: string } }>; + resolveApiKeyPermissions: ( + tokenType: string, + roleId: string | null, + userId: string, + keyId: string + ) => Promise; + }; + const { key, meta } = await apiKeys.createApiKey(owner.item.id, { + name: "the operator's own read-only key", + tokenType: "read-only", + expiresIn: "never", + }); + const scope = await apiKeys.resolveApiKeyPermissions( + "read-only", + null, + owner.item.id, + meta.id + ); + // Named here so an emptied scope fails by its cause rather than downstream + // as a refusal that reads like the guard working. + expect(scope, "the key must hold read-posts and nothing writable").toContain( + "read-posts" + ); + expect(scope.every(slug => slug.startsWith("read-"))).toBe(true); + ownerId = owner.item.id; + return key; +} + +describe("a super-admin's own read-only key", () => { + it("reads through a plugin route, the way the operator's first key is used", async () => { + const key = await readOnlyKeyOwnedBySuperAdmin(); + expect( + await isSuperAdmin(ownerId), + "the key's owner must be a super-admin, or this test proves nothing" + ).toBe(true); + + const res = await post("read", { authorization: `Bearer ${key}` }); + expect( + res.status, + "a 401 would mean the key never reached the handler" + ).not.toBe(401); + const body = (await res.json()) as { read: boolean; reason?: string }; + expect( + body.read, + "a super-admin's read-only key holds the catalogue's read permissions; " + + `a refusal here is the empty scope back: ${body.reason ?? ""}` + ).toBe(true); + }); + + it("still cannot write: the catalogue is bounded by the key's kind", async () => { + const key = await readOnlyKeyOwnedBySuperAdmin(); + const res = await post("write", { authorization: `Bearer ${key}` }); + expect(res.status).not.toBe(401); + const body = (await res.json()) as { wrote: boolean }; + expect( + body.wrote, + "a read-only key wrote through a plugin route because its owner is a super-admin" + ).toBe(false); + }); +}); + describe("a plugin route judges an API key on its own grants", () => { it("refuses a write the key's own grant does not cover", async () => { const key = await viewerKeyOwnedBySuperAdmin(); diff --git a/packages/nextly/src/plugins/service-opts-wrapper.test.ts b/packages/nextly/src/plugins/service-opts-wrapper.test.ts index 43eb92255a..da747d0e97 100644 --- a/packages/nextly/src/plugins/service-opts-wrapper.test.ts +++ b/packages/nextly/src/plugins/service-opts-wrapper.test.ts @@ -1,6 +1,16 @@ import { describe, expect, it, vi } from "vitest"; -import { resolveServiceOpts, wrapCollectionsForPlugin } from "./service-opts"; +import { + resolveServiceOpts as resolve, + type ServiceOpts, + wrapCollectionsForPlugin as wrap, +} from "./service-opts"; + +/** No roles for anyone: what a fresh account looks like, and what the tests below assume. */ +const deps = { listRoleSlugs: async () => [] as string[] }; +const wrapCollectionsForPlugin = (collections: never) => + wrap(collections, deps); +const resolveServiceOpts = (opts: ServiceOpts) => resolve(opts, deps); function mockCollections() { return { @@ -53,7 +63,13 @@ describe("wrapCollectionsForPlugin (D35, Unit C)", () => { "vault", { title: "a" }, { - user: { id: "u1", email: "u@e.com", role: "", permissions: [] }, + user: { + id: "u1", + email: "u@e.com", + role: "", + roles: [], + permissions: [], + }, overrideAccess: false, } ); @@ -81,7 +97,13 @@ describe("wrapCollectionsForPlugin (D35, Unit C)", () => { "vault", { title: "a" }, { - user: { id: "u1", email: "u@e.com", role: "", permissions: [] }, + user: { + id: "u1", + email: "u@e.com", + role: "", + roles: [], + permissions: [], + }, overrideAccess: false, authenticatedScope: { actorType: "apiKey", @@ -207,12 +229,14 @@ describe("the hook context a plugin passes", () => { ); }); - it("is absent when the caller passes none", () => { + it("is absent when the caller passes none", async () => { // The control: a facade that invented a context would satisfy the two // above without carrying anything the caller said. - expect(resolveServiceOpts({ as: "system" }).context).toBeUndefined(); expect( - resolveServiceOpts({ as: "system", context: { a: 1 } }).context + (await resolveServiceOpts({ as: "system" })).context + ).toBeUndefined(); + expect( + (await resolveServiceOpts({ as: "system", context: { a: 1 } })).context ).toEqual({ a: 1 }); }); }); diff --git a/packages/nextly/src/plugins/service-opts.test.ts b/packages/nextly/src/plugins/service-opts.test.ts index 09dc72e092..daabdeaf4a 100644 --- a/packages/nextly/src/plugins/service-opts.test.ts +++ b/packages/nextly/src/plugins/service-opts.test.ts @@ -1,33 +1,72 @@ import { describe, expect, it } from "vitest"; -import { resolveServiceOpts } from "./service-opts"; +import { + resolveServiceOpts as resolve, + type ServiceOpts, +} from "./service-opts"; + +/** + * The roles the resolver answers with, by user id. A user the table does not + * name holds none, which is what a fresh account looks like. + */ +const ROLES: Record = { editor1: ["editor", "viewer"] }; +const deps = { listRoleSlugs: async (userId: string) => ROLES[userId] ?? [] }; +const resolveServiceOpts = (opts: ServiceOpts) => resolve(opts, deps); describe("resolveServiceOpts", () => { - it("defaults to system when no as and no user", () => { - expect(resolveServiceOpts({})).toEqual({ overrideAccess: true }); + it("defaults to system when no as and no user", async () => { + expect(await resolveServiceOpts({})).toEqual({ overrideAccess: true }); }); - it("as:'system' → overrideAccess, no user", () => { - expect(resolveServiceOpts({ as: "system" })).toEqual({ + it("as:'system' → overrideAccess, no user", async () => { + expect(await resolveServiceOpts({ as: "system" })).toEqual({ overrideAccess: true, }); }); - it("as:'user' with a user → enforce, RequestContext.user shape", () => { + it("as:'user' with a user → enforce, RequestContext.user shape", async () => { expect( - resolveServiceOpts({ + await resolveServiceOpts({ as: "user", user: { id: "u1", email: "u@e.com", name: "U" }, }) ).toEqual({ overrideAccess: false, - user: { id: "u1", email: "u@e.com", role: "", permissions: [] }, + user: { + id: "u1", + email: "u@e.com", + name: "U", + role: "", + roles: [], + permissions: [], + }, + }); + }); + + it("resolves the caller's roles, so a rule reading user.role sees them", async () => { + // Built with `role: ""` before, so `req.user?.role === "editor"` refused + // every caller on this path while the same caller's own request passed. + expect( + await resolveServiceOpts({ + as: "user", + user: { id: "editor1", email: "e@e.com", name: "E" }, + }) + ).toEqual({ + overrideAccess: false, + user: { + id: "editor1", + email: "e@e.com", + name: "E", + role: "editor", + roles: ["editor", "viewer"], + permissions: [], + }, }); }); - it("a user without explicit as is treated as as:'user'", () => { + it("a user without explicit as is treated as as:'user'", async () => { expect( - resolveServiceOpts({ user: { id: "u1", email: "u@e.com" } }) + await resolveServiceOpts({ user: { id: "u1", email: "u@e.com" } }) ).toMatchObject({ overrideAccess: false, user: { id: "u1" }, @@ -43,37 +82,45 @@ describe("resolveServiceOpts", () => { * user, and every other spelling set `overrideAccess`. So a public route * could only read by bypassing whatever the host had configured. */ - it("as:'public' enforces access with no user at all", () => { - expect(resolveServiceOpts({ as: "public" })).toEqual({ + it("as:'public' enforces access with no user at all", async () => { + expect(await resolveServiceOpts({ as: "public" })).toEqual({ overrideAccess: false, }); }); - it("as:'public' does not become a user when one is in scope", () => { + it("as:'public' does not become a user when one is in scope", async () => { // A route may hold a `user` for other reasons while deliberately reading as // the public. Letting the presence of one silently upgrade the mode would // make the elevation depend on an unrelated field. expect( - resolveServiceOpts({ + await resolveServiceOpts({ as: "public", user: { id: "u1", email: "u@e.com", name: "U" }, }) ).toEqual({ overrideAccess: false }); }); - it("as:'public' is the ONLY mode that enforces without a user", () => { + it("as:'public' is the ONLY mode that enforces without a user", async () => { // The control that makes the two above mean something. If any other // spelling also enforced, a route could reach the right behaviour by // accident and this mode would not need to exist. - const enforcingWithoutUser = ( - [{}, { as: "system" as const }, { as: "public" as const }] as const - ).filter(opts => resolveServiceOpts(opts).overrideAccess === false); + const spellings = [ + {}, + { as: "system" as const }, + { as: "public" as const }, + ] as const; + const resolved = await Promise.all( + spellings.map(opts => resolveServiceOpts(opts)) + ); + const enforcingWithoutUser = spellings.filter( + (_, index) => resolved[index].overrideAccess === false + ); expect(enforcingWithoutUser).toEqual([{ as: "public" }]); }); - it("as:'user' without a user throws", () => { - expect(() => resolveServiceOpts({ as: "user" })).toThrow(); + it("as:'user' without a user throws", async () => { + await expect(resolveServiceOpts({ as: "user" })).rejects.toThrow(); }); }); @@ -89,26 +136,33 @@ describe("resolveServiceOpts — the caller's own scope", () => { permissions: ["read-posts"], }; - it("forwards an API key's grants alongside the account", () => { + it("forwards an API key's grants alongside the account", async () => { expect( - resolveServiceOpts({ + await resolveServiceOpts({ as: "user", user: { id: "u1", email: "u@e.com", name: "U" }, authenticatedScope: KEY_SCOPE, }) ).toEqual({ overrideAccess: false, - user: { id: "u1", email: "u@e.com", role: "", permissions: [] }, + user: { + id: "u1", + email: "u@e.com", + name: "U", + role: "", + roles: [], + permissions: [], + }, authenticatedScope: KEY_SCOPE, }); }); - it("omits the key entirely for a session caller, who has no key scope", () => { + it("omits the key entirely for a session caller, who has no key scope", async () => { // The control. `toEqual` ignores an explicitly-undefined property, so // asserting the scope is absent has to be done on the KEYS — otherwise a // hop that always wrote `authenticatedScope: undefined` would satisfy it, // and so would one that wrote nothing. - const resolved = resolveServiceOpts({ + const resolved = await resolveServiceOpts({ as: "user", user: { id: "u1", email: "u@e.com", name: "U" }, }); @@ -124,11 +178,11 @@ describe("resolveServiceOpts — the caller's own scope", () => { expect(resolved.overrideAccess).toBe(false); }); - it("drops a scope under system elevation, which bypasses the check it feeds", () => { + it("drops a scope under system elevation, which bypasses the check it feeds", async () => { // A scope only means anything to an access check, and `as:'system'` skips // it. Carrying one here would imply a narrowing that is not applied. expect( - resolveServiceOpts({ as: "system", authenticatedScope: KEY_SCOPE }) + await resolveServiceOpts({ as: "system", authenticatedScope: KEY_SCOPE }) ).toEqual({ overrideAccess: true }); }); }); diff --git a/packages/nextly/src/plugins/service-opts.ts b/packages/nextly/src/plugins/service-opts.ts index b09e7c4686..ee7340d4bc 100644 --- a/packages/nextly/src/plugins/service-opts.ts +++ b/packages/nextly/src/plugins/service-opts.ts @@ -1,5 +1,6 @@ import type { AuthenticatedScope } from "../auth/authenticated-scope"; import { effectiveCallerScope, runWithCallerScope } from "../auth/caller-scope"; +import { buildUserContext } from "../auth/user-context"; import { buildMutationMessage } from "../direct-api/namespaces/helpers"; import type { MutationResult } from "../direct-api/types/shared"; import { NextlyError } from "../errors/nextly-error"; @@ -8,6 +9,7 @@ import type { CollectionEntry, CollectionService, } from "../services/collections/collection-service"; +import { listRoleSlugsForUser } from "../services/lib/permissions"; import type { RequestContext } from "../services/shared"; import type { AuthUser } from "../types/auth"; @@ -16,9 +18,9 @@ import type { AuthUser } from "../types/auth"; * Default: `system` when no `user` is supplied (no-user → system). Validation/ * hooks/events ALWAYS run, even under `system` — only the access check is bypassed. * - * Under `as:'user'`, RBAC is enforced by `user.id` (DB lookup). Code-defined - * `access` rules that read `ctx.user.role` see it empty — pass `system`, or rely on - * DB RBAC, for now (documented v1 limitation). + * Under `as:'user'`, RBAC is enforced by `user.id` (DB lookup), and the caller's + * roles are resolved so a code-defined `access` rule reading `ctx.user.role` or + * `ctx.user.roles` sees the same caller a session request would. */ export interface ServiceOpts { /** @@ -80,14 +82,39 @@ export interface ServiceOpts { authenticatedScope?: AuthenticatedScope; } -/** Translate {@link ServiceOpts} into the facade's `{ user, overrideAccess }`. */ -export function resolveServiceOpts(opts: ServiceOpts): { +/** + * What resolving a caller needs from the outside: the roles a user holds, by + * slug. Injectable so the translation is tested without a database; the facade + * wrapper supplies the real lookup. + */ +export interface ServiceOptsDeps { + listRoleSlugs: (userId: string) => Promise; +} + +const REAL_DEPS: ServiceOptsDeps = { listRoleSlugs: listRoleSlugsForUser }; + +/** + * Translate {@link ServiceOpts} into the facade's `{ user, overrideAccess }`. + * + * A caller is built by `buildUserContext`, the one constructor every + * authenticated path uses, with the roles it holds resolved here. Built by hand + * with `role: ""`, as it was, a code-defined rule such as + * `req.user?.role === "editor"` refused every caller on the plugin path, while + * the same caller's own request passed it; and a negative rule granted what it + * was written to refuse. The permission list stays empty: the access services + * resolve permissions from the id, and from the key's own scope when there is + * one. + */ +export async function resolveServiceOpts( + opts: ServiceOpts, + deps: ServiceOptsDeps = REAL_DEPS +): Promise<{ user?: RequestContext["user"]; authenticatedScope?: AuthenticatedScope; overrideAccess: boolean; context?: Record; request?: Request; -} { +}> { const { as, user, context, request } = opts; // The caller's own scope wins when named; otherwise the one the dispatcher // pinned for this request. A route that omits it is the common case, not the @@ -113,9 +140,21 @@ export function resolveServiceOpts(opts: ServiceOpts): { logContext: { reason: "service-opts-user-missing" }, }); } + const identity = buildUserContext({ + id: user.id, + name: user.name ?? undefined, + email: user.email, + roles: await deps.listRoleSlugs(user.id), + }); return { overrideAccess: false, - user: { id: user.id, email: user.email, role: "", permissions: [] }, + user: { + ...identity, + id: user.id, + email: user.email, + role: identity.role ?? "", + permissions: [], + }, context, request, ...(authenticatedScope ? { authenticatedScope } : {}), @@ -220,7 +259,8 @@ export type PluginCollectionService = Omit< * `overrideAccess` directly. */ export function wrapCollectionsForPlugin( - collections: CollectionService + collections: CollectionService, + deps: ServiceOptsDeps = REAL_DEPS ): PluginCollectionService { return new Proxy(collections, { get(target, prop, receiver) { @@ -235,7 +275,10 @@ export function wrapCollectionsForPlugin( prop as string ]; return async (...args: unknown[]) => { - const resolved = resolveServiceOpts((args[idx] as ServiceOpts) ?? {}); + const resolved = await resolveServiceOpts( + (args[idx] as ServiceOpts) ?? {}, + deps + ); const next = [...args]; // Spread rather than named one by one. Rebuilding this literal is // what kept a plugin from reaching the hook context, and the same diff --git a/packages/nextly/src/schemas/api-keys/postgres.ts b/packages/nextly/src/schemas/api-keys/postgres.ts index f183209483..f003370d02 100644 --- a/packages/nextly/src/schemas/api-keys/postgres.ts +++ b/packages/nextly/src/schemas/api-keys/postgres.ts @@ -40,7 +40,8 @@ import { users } from "../users/postgres"; * * Token types: * - "read-only" — resolves to creator's read-* permissions only - * - "full-access" — resolves to creator's full permission set (at request time) + * - "full-access" — resolves to creator's full permission set (at request time; + * the whole catalogue for a super-admin, whose power is a bypass rather than rows) * - "role-based" — resolves to the referenced role's permissions (at request time) * * Revocation: From 48e457d7fd154f35f170850dbb927e3c9b507019 Mon Sep 17 00:00:00 2001 From: Mobeen Abdullah Date: Fri, 11 Sep 2026 13:41:17 +0300 Subject: [PATCH 2/4] test(nextly): two permission mocks derive from the real module Both were closed literals of services/lib/permissions, and the plugin facade now imports one more export from it, listRoleSlugsForUser, which vitest refused as undefined on the mock. Each spreads the real module and overrides only what it stubs, so the next export the subject gains is there too. --- .../src/__tests__/routeHandler-direct-branches.test.ts | 6 ++++-- packages/nextly/src/api/dashboard.test.ts | 5 ++++- 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/packages/nextly/src/__tests__/routeHandler-direct-branches.test.ts b/packages/nextly/src/__tests__/routeHandler-direct-branches.test.ts index d301f2fb92..caef8bec4d 100644 --- a/packages/nextly/src/__tests__/routeHandler-direct-branches.test.ts +++ b/packages/nextly/src/__tests__/routeHandler-direct-branches.test.ts @@ -59,8 +59,10 @@ vi.mock("../di/container", () => ({ // `services/lib/permissions` is pulled into routeHandler for super-admin // guards. None of the tested branches hit those guards but the import must -// resolve. -vi.mock("../services/lib/permissions", () => ({ +// resolve. Derived from the real module rather than written out: a closed +// literal breaks the moment the subject imports one more export, and did. +vi.mock("../services/lib/permissions", async importOriginal => ({ + ...(await importOriginal()), isSuperAdmin: vi.fn().mockResolvedValue(false), containsSuperAdminRole: vi.fn().mockResolvedValue(false), hasSuperAdminExcluding: vi.fn().mockResolvedValue(false), diff --git a/packages/nextly/src/api/dashboard.test.ts b/packages/nextly/src/api/dashboard.test.ts index 5c5ccab5d3..59c736699f 100644 --- a/packages/nextly/src/api/dashboard.test.ts +++ b/packages/nextly/src/api/dashboard.test.ts @@ -41,7 +41,10 @@ vi.mock("../di/container", () => ({ container: { get: containerGet, has: containerHas }, })); -vi.mock("../services/lib/permissions", () => ({ +vi.mock("../services/lib/permissions", async importOriginal => ({ + // Derived from the real module, so an export the subject gains later is + // still there; a closed literal broke on exactly that. + ...(await importOriginal()), // `readCaller` (via `authenticated-read.ts`) resolves this to build the // caller it hands the dashboard service. Unmocked, it falls through to a // real database lookup that has nothing to connect to in this suite. From 5722a7097a99de49963694f91c292ca1e90557f5 Mon Sep 17 00:00:00 2001 From: Mobeen Abdullah Date: Fri, 11 Sep 2026 14:26:03 +0300 Subject: [PATCH 3/4] fix(nextly): ask the canonical resolver, and judge a key on its own roles A super-admin by INHERITANCE was not one to the key service. The catalogue branch read `user_roles` directly, so an operator who builds a role on top of Super Admin is a super-admin to the session bypass, the admin's checks and the key ceiling, and an ordinary user to the key they mint. `isSuperAdmin` resolves the inherited set and is what every other gate asks; the key service asks it too, and the ceiling check that carried the same direct query is derived from it rather than repeating it. A plugin caller that arrived on an API key is judged on the KEY's roles. A stored role rule reads `user.roles` with no scope in front of it, so resolving the owner's roles let a viewer-scoped key minted by an administrator satisfy an administrators-only rule, and refused a key holding the very role a rule names because its owner did not hold it. The REST path answers this with `resolveRoleSlugs`; the facade now answers it the same way, falling back to the account only for a scope that carries no roles, as `apiKeyWriteAllowed` does. Controls: an integration case whose owner holds super-admin ONLY by inheritance, asserting their read-only key holds a catalogue permission no role of theirs grants (the one thing the role-rows branch cannot produce); a unit case asserting the resolver is called, for the owner and nobody else; and four cases pinning the key's roles in both directions, including an emptied role list that must not fall back to the owner. --- ...a-super-admins-key-copies-the-catalogue.md | 10 +- docs/guides/authentication.mdx | 2 +- .../__tests__/api-key-token-types.test.ts | 67 +++++--- .../domains/auth/services/api-key-service.ts | 38 ++--- ...plugin-route-key-scope.integration.test.ts | 155 ++++++++++++++++++ .../nextly/src/plugins/service-opts.test.ts | 69 ++++++++ packages/nextly/src/plugins/service-opts.ts | 15 +- 7 files changed, 305 insertions(+), 51 deletions(-) diff --git a/.changeset/a-super-admins-key-copies-the-catalogue.md b/.changeset/a-super-admins-key-copies-the-catalogue.md index 0a5871974e..9be9b16fd8 100644 --- a/.changeset/a-super-admins-key-copies-the-catalogue.md +++ b/.changeset/a-super-admins-key-copies-the-catalogue.md @@ -34,7 +34,9 @@ grant, nothing: every request refused, starting with the first key an operator minted to try an integration with. A `read-only` key of theirs now reads every collection the install declares, including one added later, and still cannot write; a `full-access` key holds every permission. A permission a package -stopped declaring is not inherited. +stopped declaring is not inherited. Whether the creator is a Super Admin is +asked of the same resolver as the session bypass, so a role built on top of +Super Admin counts here exactly as it does everywhere else. A plugin calling `ctx.services` as a user now sees that user's roles. The caller was built with an empty role, so a code-defined rule such as @@ -42,3 +44,9 @@ caller was built with an empty role, so a code-defined rule such as the same caller's own request passed it, and a negative rule granted what it was written to refuse. The roles are resolved and the caller is built by the one constructor every other authenticated path uses. + +A caller that arrived on an API key is judged on the KEY's roles, not its +owner's, the way the REST path already judges one. A stored role rule reads +the caller's roles directly, so the owner's roles let a viewer-scoped key +minted by an administrator satisfy an administrators-only rule, and refused a +key holding the very role a rule names because its owner did not hold it. diff --git a/docs/guides/authentication.mdx b/docs/guides/authentication.mdx index 83531c3f37..44211d0e15 100644 --- a/docs/guides/authentication.mdx +++ b/docs/guides/authentication.mdx @@ -166,7 +166,7 @@ Nextly supports API keys via the `Authorization: Bearer nx_live_...` header (no | `full-access` | All permissions of the creator's roles. | | `role-based` | Permissions of an explicitly chosen role (independent of the creator). | -A key created by a Super Admin copies the whole permission catalogue rather than the role's rows, because the Super Admin's power is a bypass and the role only holds the permissions that existed when the install was set up: a `read-only` key of theirs can read every collection, including one added later, and still cannot write. +A key created by a Super Admin copies the whole permission catalogue rather than the role's rows, because the Super Admin's power is a bypass and the role only holds the permissions that existed when the install was set up: a `read-only` key of theirs can read every collection, including one added later, and still cannot write. A role that inherits Super Admin counts, the same way it counts everywhere else. Keys can be time-bound (set an expiry date) and revoked at any time. The full key value is shown to the operator **once** at creation; the database only stores the SHA-256 hash. diff --git a/packages/nextly/src/domains/auth/__tests__/api-key-token-types.test.ts b/packages/nextly/src/domains/auth/__tests__/api-key-token-types.test.ts index d24cdf6339..7c0f7f5440 100644 --- a/packages/nextly/src/domains/auth/__tests__/api-key-token-types.test.ts +++ b/packages/nextly/src/domains/auth/__tests__/api-key-token-types.test.ts @@ -3,19 +3,27 @@ import { randomUUID } from "crypto"; import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; import { createTestDb, type TestDb } from "../../../__tests__/fixtures/db"; -import { listRoleSlugsForUser } from "../../../services/lib/permissions"; +import { + isSuperAdmin, + listRoleSlugsForUser, +} from "../../../services/lib/permissions"; import type { Logger } from "../../../services/shared"; import { ApiKeyService, invalidateApiKeyPermissionsCache, } from "../services/api-key-service"; -// Mock listRoleSlugsForUser — it uses a global db singleton (not the test DB). -// We must mock it so tests do not depend on the runtime database connection. -// The path the subject imports: `api-key-service` reads -// `listRoleSlugsForUser` from `services/lib/permissions`, and a factory -// registered against any other specifier leaves the real one in place. +// Mock the RBAC resolvers — they read a global db singleton (not the test DB). +// We must mock them so tests do not depend on the runtime database connection. +// The path the subject imports: `api-key-service` reads `listRoleSlugsForUser` +// and `isSuperAdmin` from `services/lib/permissions`, and a factory registered +// against any other specifier leaves the real ones in place. `isSuperAdmin` is +// the canonical resolver (inheritance and its cache are proven in its own +// suite, and end to end in `plugin-route-key-scope.integration.test.ts`); here +// it answers by id, so what this suite proves is what the service does with +// the answer. vi.mock("../../../services/lib/permissions", () => ({ + isSuperAdmin: vi.fn(), listRoleSlugsForUser: vi.fn(), })); @@ -87,6 +95,8 @@ describe("ApiKeyService – Token Type Permission Resolution", () => { beforeEach(async () => { testDb = await createTestDb(); service = new ApiKeyService(createTestAdapter(testDb.db), noopLogger); + // Nobody is a super-admin unless a case says so. + vi.mocked(isSuperAdmin).mockResolvedValue(false); // ── Users ──────────────────────────────────────────────────────────────── userId = randomUUID(); @@ -378,34 +388,29 @@ describe("ApiKeyService – Token Type Permission Resolution", () => { // ── a super-admin's key ──────────────────────────────────────────────── describe("a key created by a super-admin", () => { /** - * A super-admin whose role holds NO permission rows, which is what an - * install looks like whose first user came before the setup grant, and - * what every install drifts toward: the role is granted the rows that - * exist at setup and never the ones a later collection adds. Their power - * is the bypass, so the session never notices; a key copies the rows. + * A super-admin who holds NO permission rows, which is what an install + * looks like whose first user came before the setup grant, and what + * every install drifts toward: the role is granted the rows that exist + * at setup and never the ones a later collection adds. Their power is + * the bypass, so the session never notices; a key copies the rows. + * + * Whether they are one is the canonical resolver's answer, mocked by id + * above; no `user_roles` row is seeded because the service must not + * read one. It did, directly, and a role that INHERITS super-admin was + * a super-admin to every other gate and an ordinary user to its key. */ let superAdminId: string; beforeEach(async () => { superAdminId = randomUUID(); - const superAdminRoleId = randomUUID(); + vi.mocked(isSuperAdmin).mockImplementation( + async id => id === superAdminId + ); await testDb.db.insert(testDb.schema.users).values({ id: superAdminId, email: `super-${superAdminId}@example.com`, isActive: true, }); - await testDb.db.insert(testDb.schema.roles).values({ - id: superAdminRoleId, - name: "Super Admin", - slug: "super-admin", - level: 100, - isSystem: true, - }); - await testDb.db.insert(testDb.schema.userRoles).values({ - id: randomUUID(), - userId: superAdminId, - roleId: superAdminRoleId, - }); // A permission a package stopped declaring: kept in the table so a // grant survives, and never copied to a key that inherits nothing else new. await testDb.db.insert(testDb.schema.permissions).values({ @@ -498,6 +503,20 @@ describe("ApiKeyService – Token Type Permission Resolution", () => { ); expect(slugs).not.toContain("read-media"); }); + + it("asks the resolver every other gate asks, for the owner and nobody else", async () => { + // The control on the question itself. A direct read of `user_roles` + // would answer these cases identically for a directly-assigned role + // and differently for an inherited one; only the call proves the + // service asks the canonical resolver rather than its own rows. + await service.resolveApiKeyPermissions( + "full-access", + null, + superAdminId, + KEY_SUPER + ); + expect(vi.mocked(isSuperAdmin).mock.calls).toEqual([[superAdminId]]); + }); }); // ── role-based ───────────────────────────────────────────────────────── diff --git a/packages/nextly/src/domains/auth/services/api-key-service.ts b/packages/nextly/src/domains/auth/services/api-key-service.ts index 1901dbe2cf..3822ea7b87 100644 --- a/packages/nextly/src/domains/auth/services/api-key-service.ts +++ b/packages/nextly/src/domains/auth/services/api-key-service.ts @@ -66,7 +66,10 @@ import { // info (key id, role id, exceeded permission) moves from public message to logContext // per spec §13.8 (no identifiers/values in publicMessage). import { BaseService } from "../../../services/base-service"; -import { listRoleSlugsForUser } from "../../../services/lib/permissions"; +import { + isSuperAdmin, + listRoleSlugsForUser, +} from "../../../services/lib/permissions"; import type { Logger } from "../../../services/shared"; /** The three token types that determine how permissions are resolved at request time. */ @@ -754,8 +757,11 @@ export class ApiKeyService extends BaseService { // subset at best and, on an install whose first user came before the // grant, nothing: every request refused, and the first key an operator // mints to try an integration with. The catalogue is what their key - // copies, bounded by the key's kind like anyone else's. - const all = (await this.ownerIsSuperAdmin(userId)) + // copies, bounded by the key's kind like anyone else's. Whether they are + // one is the session bypass's own question, asked of its own resolver: + // a role that inherits super-admin answers yes there, so it answers yes + // here, where a direct read of `user_roles` said no. + const all = (await isSuperAdmin(userId)) ? await this.resolveCataloguePermissionRows() : await this.resolveUserPermissionRows(userId); // Still filtered on the STORED slug rather than on `action === "read"`. @@ -873,25 +879,6 @@ export class ApiKeyService extends BaseService { return rows as GrantedPermission[]; } - /** Whether a user holds the super-admin role, by the slug every other check reads. */ - private async ownerIsSuperAdmin(userId: string): Promise { - const rows = await this.db - .select({ id: this.rolesTable.id }) - .from(this.userRolesTable) - .innerJoin( - this.rolesTable, - eq(this.userRolesTable.roleId, this.rolesTable.id) - ) - .where( - and( - eq(this.userRolesTable.userId, userId), - eq(this.rolesTable.slug, "super-admin") - ) - ) - .limit(1); - return (rows as unknown[]).length > 0; - } - private async resolveUserPermissionRows( userId: string ): Promise { @@ -1061,8 +1048,11 @@ export class ApiKeyService extends BaseService { if (tokenType !== "role-based") return; if (!roleId) return; - // Super-admin bypass: a super-admin can assign any role - if (await this.ownerIsSuperAdmin(creatorId)) return; + // Super-admin bypass: a super-admin can assign any role. Asked of the + // same resolver as the session bypass, so a role that INHERITS super-admin + // counts here exactly as it does there; a direct-row read of `user_roles` + // said no to such a creator while every other gate said yes. + if (await isSuperAdmin(creatorId)) return; const creatorRoleRows = await this.db .select({ roleId: this.userRolesTable.roleId }) diff --git a/packages/nextly/src/plugins/routes/__tests__/plugin-route-key-scope.integration.test.ts b/packages/nextly/src/plugins/routes/__tests__/plugin-route-key-scope.integration.test.ts index 3c964277ff..0295202688 100644 --- a/packages/nextly/src/plugins/routes/__tests__/plugin-route-key-scope.integration.test.ts +++ b/packages/nextly/src/plugins/routes/__tests__/plugin-route-key-scope.integration.test.ts @@ -113,6 +113,16 @@ beforeEach(async () => { access: { read: () => true, create: () => false }, fields: [text({ name: "title" })], }), + // A second collection exists only so the catalogue holds a read + // permission no hand-built role below is granted. That one slug is what + // separates "copied the catalogue" from "copied the owner's rows"; with + // a single collection both branches answer `read-posts` and the + // assertion would be satisfied by the behaviour it was written to refuse. + defineCollection({ + slug: "notes", + access: { read: () => true }, + fields: [text({ name: "title" })], + }), ], plugins: [writePlugin], }); @@ -279,6 +289,151 @@ async function readOnlyKeyOwnedBySuperAdmin(): Promise { return key; } +/** + * A super-admin by INHERITANCE, which is what the canonical resolver answers + * and a direct read of `user_roles` does not. + * + * An operator who gives a deputy a role built on top of Super Admin has made a + * super-admin: `isSuperAdmin` resolves the inherited set, so the session + * bypass, the admin's own checks and the key ceiling all agree they are one. + * The key service asked its own narrower question for a while, and their key + * took the ordinary branch — so the same person was a super-admin everywhere + * except in the key they minted. + * + * Returns the catalogue permission their own role does NOT hold, which is the + * only thing that separates the two branches: the role-rows branch cannot + * produce it. + */ +async function readOnlyKeyOwnedByAnInheritedSuperAdmin(): Promise<{ + slugs: string[]; + ownerId: string; + onlyInTheCatalogue: string; +}> { + const nextly = handle!.nextly as unknown as { + users: { + create: (a: { data: Record }) => Promise<{ + item: { id: string }; + }>; + }; + permissions: { + find: (a: { limit: number }) => Promise<{ + items: { id: string; slug: string }[]; + }>; + }; + roles: { + find: (a: { limit: number }) => Promise<{ + items: { id: string; slug: string }[]; + }>; + create: (a: { data: Record }) => Promise<{ + item: { id: string }; + }>; + }; + }; + + // The first user holds the seeded super-admin role directly, which is also + // what puts that role in the table for the deputy's role to inherit. + await nextly.users.create({ + data: { + email: "first@example.com", + password: "Password123!", + name: "First", + isActive: true, + }, + }); + + const roles = await nextly.roles.find({ limit: 200 }); + const superAdmin = roles.items.find(r => r.slug === "super-admin"); + expect( + superAdmin, + "the seeded super-admin role must exist, or nothing below inherits it" + ).toBeDefined(); + + const permissions = await nextly.permissions.find({ limit: 300 }); + const readPosts = permissions.items.find(p => p.slug === "read-posts"); + const readNotes = permissions.items.find(p => p.slug === "read-notes"); + expect( + readPosts && readNotes, + "both collections must have seeded their read permission" + ).toBeTruthy(); + + // Built ON TOP of Super Admin: one child role, so the service requires at + // least one permission of its own, and `read-posts` is the one it gets. + // `read-notes` is therefore in the catalogue and NOT in this role's rows. + const deputy = await nextly.roles.create({ + data: { + name: "Deputy", + slug: "deputy", + permissionIds: [readPosts!.id], + childRoleIds: [superAdmin!.id], + }, + }); + + const owner = await nextly.users.create({ + data: { + email: "deputy@example.com", + password: "Password123!", + name: "Deputy", + isActive: true, + roles: [deputy.item.id], + }, + }); + + const apiKeys = handle!.getService("apiKeyService") as unknown as { + createApiKey: ( + userId: string, + input: { name: string; tokenType: string; expiresIn: string } + ) => Promise<{ key: string; meta: { id: string } }>; + resolveApiKeyPermissions: ( + tokenType: string, + roleId: string | null, + userId: string, + keyId: string + ) => Promise; + }; + const { meta } = await apiKeys.createApiKey(owner.item.id, { + name: "the deputy's read-only key", + tokenType: "read-only", + expiresIn: "never", + }); + const slugs = await apiKeys.resolveApiKeyPermissions( + "read-only", + null, + owner.item.id, + meta.id + ); + return { + slugs, + ownerId: owner.item.id, + onlyInTheCatalogue: readNotes!.slug, + }; +} + +describe("a super-admin by inheritance", () => { + it("is one to the resolver every other gate asks", async () => { + // The precondition, and the whole point: the deputy holds no `user_roles` + // row naming super-admin, so a direct read of that table answers no here + // while every gate in the codebase answers yes. + const { ownerId } = await readOnlyKeyOwnedByAnInheritedSuperAdmin(); + expect(await isSuperAdmin(ownerId)).toBe(true); + }); + + it("mints a key that copies the catalogue, not their own role's rows", async () => { + const { slugs, onlyInTheCatalogue } = + await readOnlyKeyOwnedByAnInheritedSuperAdmin(); + expect( + slugs, + `a permission no role of theirs holds is the only thing the ordinary ` + + `branch cannot produce; without it the key copied ${onlyInTheCatalogue}'s ` + + `absence, which is the direct-user_roles read back` + ).toContain(onlyInTheCatalogue); + }); + + it("still cannot write, so the inheritance widened nothing", async () => { + const { slugs } = await readOnlyKeyOwnedByAnInheritedSuperAdmin(); + expect(slugs.every(slug => slug.startsWith("read-"))).toBe(true); + }); +}); + describe("a super-admin's own read-only key", () => { it("reads through a plugin route, the way the operator's first key is used", async () => { const key = await readOnlyKeyOwnedBySuperAdmin(); diff --git a/packages/nextly/src/plugins/service-opts.test.ts b/packages/nextly/src/plugins/service-opts.test.ts index daabdeaf4a..be8dd9c09b 100644 --- a/packages/nextly/src/plugins/service-opts.test.ts +++ b/packages/nextly/src/plugins/service-opts.test.ts @@ -157,6 +157,75 @@ describe("resolveServiceOpts — the caller's own scope", () => { }); }); + it("judges a key on ITS roles, never its owner's, when the scope carries them", async () => { + // The owner holds editor and viewer; the key was minted on viewer alone. + // A stored role rule reads `user.roles` with no scope in front of it, so + // the owner's roles here would let this key satisfy an editors-only rule + // over the plugin path that the REST path refuses it. + expect( + await resolveServiceOpts({ + as: "user", + user: { id: "editor1", email: "e@e.com", name: "E" }, + authenticatedScope: { ...KEY_SCOPE, roles: ["viewer"] }, + }) + ).toMatchObject({ + overrideAccess: false, + user: { id: "editor1", role: "viewer", roles: ["viewer"] }, + }); + }); + + it("grants a key the role it was minted on even when its owner lacks it", async () => { + // The other direction, and the one that separates this from a blanket + // refusal: u1 holds no roles at all, the key holds editor. + expect( + await resolveServiceOpts({ + as: "user", + user: { id: "u1", email: "u@e.com", name: "U" }, + authenticatedScope: { ...KEY_SCOPE, roles: ["editor"] }, + }) + ).toMatchObject({ user: { role: "editor", roles: ["editor"] } }); + }); + + it("a key whose role is gone holds no roles here either", async () => { + // `[]` is what authentication resolves for a role-based key whose role + // was deleted. It is a list, not an absence: falling back to the owner's + // roles on an empty one would revive the deleted role as the owner. + expect( + await resolveServiceOpts({ + as: "user", + user: { id: "editor1", email: "e@e.com", name: "E" }, + authenticatedScope: { ...KEY_SCOPE, roles: [] }, + }) + ).toMatchObject({ user: { role: "", roles: [] } }); + }); + + it("falls back to the account's roles for a scope that carries none", async () => { + // `roles` is omitted, not emptied, when authentication resolved none; + // `apiKeyWriteAllowed` reads `scope.roles ?? user.roles` for the same + // reason, and this is the `user.roles` it falls back to. + expect( + await resolveServiceOpts({ + as: "user", + user: { id: "editor1", email: "e@e.com", name: "E" }, + authenticatedScope: KEY_SCOPE, + }) + ).toMatchObject({ user: { role: "editor", roles: ["editor", "viewer"] } }); + }); + + it("hands the rule a copy, not the scope's frozen list", async () => { + const scope = Object.freeze({ + ...KEY_SCOPE, + roles: Object.freeze(["viewer"]) as readonly string[], + }); + const resolved = await resolveServiceOpts({ + as: "user", + user: { id: "editor1", email: "e@e.com", name: "E" }, + authenticatedScope: scope, + }); + expect(resolved.user?.roles).toEqual(["viewer"]); + expect(Object.isFrozen(resolved.user?.roles)).toBe(false); + }); + it("omits the key entirely for a session caller, who has no key scope", async () => { // The control. `toEqual` ignores an explicitly-undefined property, so // asserting the scope is absent has to be done on the KEYS — otherwise a diff --git a/packages/nextly/src/plugins/service-opts.ts b/packages/nextly/src/plugins/service-opts.ts index ee7340d4bc..4c16c7158c 100644 --- a/packages/nextly/src/plugins/service-opts.ts +++ b/packages/nextly/src/plugins/service-opts.ts @@ -104,6 +104,15 @@ const REAL_DEPS: ServiceOptsDeps = { listRoleSlugs: listRoleSlugsForUser }; * was written to refuse. The permission list stays empty: the access services * resolve permissions from the id, and from the key's own scope when there is * one. + * + * The roles are the KEY's when the caller arrived on one. `user` names the + * key's owner, and a stored role rule reads `user.roles` with no scope in + * front of it, so the owner's roles here let a viewer key minted by an + * administrator satisfy an administrators-only rule, and refused a key + * holding the very role the rule names because its owner did not. The REST + * path answers the same question with `resolveRoleSlugs`: a key's roles as + * authentication resolved them, an account's from the database. A scope that + * carries none falls back to the account, as `apiKeyWriteAllowed` does. */ export async function resolveServiceOpts( opts: ServiceOpts, @@ -144,7 +153,11 @@ export async function resolveServiceOpts( id: user.id, name: user.name ?? undefined, email: user.email, - roles: await deps.listRoleSlugs(user.id), + // Copied: the scope's arrays are frozen, and the caller object is the + // mutable shape every consumer of it is typed against. + roles: authenticatedScope?.roles + ? [...authenticatedScope.roles] + : await deps.listRoleSlugs(user.id), }); return { overrideAccess: false, From 9f9e46c747fe2525714d0cb08537a8bf460c5a32 Mon Sep 17 00:00:00 2001 From: Mobeen Abdullah Date: Fri, 11 Sep 2026 15:01:40 +0300 Subject: [PATCH 4/4] fix(nextly): clear the super-admin answer when the roles behind it change `isSuperAdmin` answers from a process-wide map with a 60-second life, and `invalidatePermissionCache` cleared the two permission tiers beside it and not that map. A user removed from the role kept the session bypass until the entry aged out. Reading a key's grants through that answer extends it: the grants are cached for five minutes of their own, so a stale `true` could be copied into a key's grants after the demotion and outlive it by both windows together. The cause is the missing eviction, not the new reader, and it is fixed there, which closes the same window for the session bypass that has always had it. A `userId` hint clears that user. A `roleId` hint clears the map: it names no user, the map does not record which users a role reaches, and the in-memory permission keys only name users that happen to hold a cached entry, so the subset cannot be derived. The map is bounded at 1,000 entries with a 60-second life and a role change is rare, so re-asking costs one indexed query. Controls, on a real database with the role removed by a raw row delete so only the call under test can clear anything: the answer is still cached after the demotion, which is what makes the other three mean something; a userId invalidation clears that user; a roleId invalidation clears them too; and an unrelated user's answer survives a userId invalidation, which separates this from clearing the map on every hint. --- ...a-super-admins-key-copies-the-catalogue.md | 6 + .../nextly/src/services/lib/permissions.ts | 38 ++++- ...min-cache-invalidation.integration.test.ts | 152 ++++++++++++++++++ 3 files changed, 189 insertions(+), 7 deletions(-) create mode 100644 packages/nextly/src/services/lib/super-admin-cache-invalidation.integration.test.ts diff --git a/.changeset/a-super-admins-key-copies-the-catalogue.md b/.changeset/a-super-admins-key-copies-the-catalogue.md index 9be9b16fd8..b32d4d6c33 100644 --- a/.changeset/a-super-admins-key-copies-the-catalogue.md +++ b/.changeset/a-super-admins-key-copies-the-catalogue.md @@ -45,6 +45,12 @@ the same caller's own request passed it, and a negative rule granted what it was written to refuse. The roles are resolved and the caller is built by the one constructor every other authenticated path uses. +Losing the Super Admin role now takes effect at once. The cached answer to +"is this user a super admin" was not cleared when roles changed, so a demoted +user kept the session bypass until the entry aged out, and an API key's grants +resolved through that answer could be cached for five minutes of their own on +top of it. Role and permission invalidation clears it. + A caller that arrived on an API key is judged on the KEY's roles, not its owner's, the way the REST path already judges one. A stored role rule reads the caller's roles directly, so the owner's roles let a viewer-scoped key diff --git a/packages/nextly/src/services/lib/permissions.ts b/packages/nextly/src/services/lib/permissions.ts index 9fc5ee568d..51f11e58a0 100644 --- a/packages/nextly/src/services/lib/permissions.ts +++ b/packages/nextly/src/services/lib/permissions.ts @@ -575,6 +575,20 @@ export async function listEffectivePermissions( } } +/** + * The super-admin answer, cached per user. + * + * Declared here rather than beside `isSuperAdmin` because + * `invalidatePermissionCache` below has to clear it: it is the same question as + * the permission caches above, asked of the same rows, and a cache that no + * invalidation reaches is one that outlives the change it should have seen. + */ +const superAdminCache = new Map< + string, + { value: boolean; expiresAt: number } +>(); +const SUPER_ADMIN_CACHE_TTL_MS = 60_000; // 60 seconds + /** * Invalidate permission cache (both in-memory and database tiers). * @@ -598,6 +612,23 @@ export async function invalidatePermissionCache( ): Promise { const { userId, roleId } = _hint || {}; + // The super-admin answer is a cache of its own and has to go with them. + // + // It is the same question, asked of the same rows, and it was surviving a + // role change for its full TTL: a user demoted out of super-admin kept the + // session bypass for up to a minute. It reaches further than that now, + // because an API key's grants are resolved through this answer and cached + // for five minutes of their own, so a stale `true` could be copied into the + // key's grants after the demotion and outlive it by both windows together. + // + // A `roleId` hint clears the whole map rather than a subset: the map does not + // record which users a role reaches, the in-memory permission keys only name + // users who happen to have a cached entry, and the map is bounded at 1,000 + // entries with a 60-second life. Re-asking is a single indexed query, and a + // role change is rare; guessing at the subset is how a demotion survives. + if (userId) superAdminCache.delete(userId); + if (roleId) superAdminCache.clear(); + // Invalidate in-memory caches (Tier 1) if (userId) { const keys = userIdToKeys.get(userId); @@ -658,13 +689,6 @@ export async function invalidatePermissionCache( } } -// ---- Super-admin check with caching ---- -const superAdminCache = new Map< - string, - { value: boolean; expiresAt: number } ->(); -const SUPER_ADMIN_CACHE_TTL_MS = 60_000; // 60 seconds - /** * Check if a user has the super-admin role. * diff --git a/packages/nextly/src/services/lib/super-admin-cache-invalidation.integration.test.ts b/packages/nextly/src/services/lib/super-admin-cache-invalidation.integration.test.ts new file mode 100644 index 0000000000..5c8dc71150 --- /dev/null +++ b/packages/nextly/src/services/lib/super-admin-cache-invalidation.integration.test.ts @@ -0,0 +1,152 @@ +/** + * A demotion out of super-admin must not survive in a cache. + * + * `isSuperAdmin` answers from a process-wide map with a 60-second life, and + * `invalidatePermissionCache` cleared the two permission tiers beside it and + * not that map. So a user removed from the role kept the session bypass until + * the entry aged out. + * + * It reaches further than one minute. An API key's grants are resolved through + * this answer and cached for five minutes of their own, so a stale `true` could + * be copied into a key's grants after the demotion and outlive it by both + * windows together: the catalogue, in the hands of someone who no longer holds + * the role that grants it. + * + * Exercised on a real database, against the cache itself: the role is removed + * by a raw row delete rather than through the role service, so the only thing + * that can clear the entry is the call under test. + */ +import { eq } from "drizzle-orm"; +import { createTestNextly, type TestNextly } from "nextly/testing"; +import { afterEach, beforeEach, describe, expect, it } from "vitest"; + +import { getDialectTables } from "../../database/index"; + +import { invalidatePermissionCache, isSuperAdmin } from "./permissions"; + +let harness: TestNextly | undefined; + +function rawDb() { + return harness!.adapter.getDrizzle() as unknown as { + insert: (table: unknown) => { values: (row: unknown) => Promise }; + delete: (table: unknown) => { where: (cond: unknown) => Promise }; + }; +} + +/** + * The `super-admin` role, by the slug every check reads, inserted once per + * harness. Raw rows throughout: the role service invalidates caches as it + * writes, and a cache left standing is the whole subject here. + */ +const SUPER_ADMIN_ROLE = "super-admin-role"; + +async function seedSuperAdminRole(): Promise { + const tables = getDialectTables(); + await rawDb().insert(tables.roles).values({ + id: SUPER_ADMIN_ROLE, + name: "Super Admin", + slug: "super-admin", + level: 1000, + isSystem: true, + createdAt: new Date(), + updatedAt: new Date(), + }); +} + +/** A user holding that role, by a raw row. */ +async function promote(userId: string, email: string): Promise { + const db = rawDb(); + const tables = getDialectTables(); + await db.insert(tables.users).values({ + id: userId, + email, + name: userId, + isActive: true, + createdAt: new Date(), + updatedAt: new Date(), + }); + await db.insert(tables.userRoles).values({ + id: `${userId}-role`, + userId, + roleId: SUPER_ADMIN_ROLE, + createdAt: new Date(), + }); +} + +/** Remove every role row for a user WITHOUT going through the role service. */ +async function demote(userId: string): Promise { + const tables = getDialectTables(); + await rawDb() + .delete(tables.userRoles) + .where(eq(tables.userRoles.userId, userId)); +} + +beforeEach(async () => { + harness = await createTestNextly(); + await seedSuperAdminRole(); +}); + +afterEach(async () => { + await harness?.destroy(); + harness = undefined; +}); + +describe("the super-admin answer is invalidated with the permissions it is asked beside", () => { + it("is cached, which is what makes the rest of this file mean anything", async () => { + // The control. Every case below asserts that an invalidation CHANGED the + // answer; without a live cache the answer would be recomputed anyway and + // each of them would pass against a function that caches nothing. + const userId = "super-cache-control"; + await promote(userId, "super-cache-control@example.com"); + expect(await isSuperAdmin(userId)).toBe(true); + + await demote(userId); + expect( + await isSuperAdmin(userId), + "a raw demotion must still read true, or the cache is not live here" + ).toBe(true); + }); + + it("clears the demoted user on a userId invalidation", async () => { + const userId = "super-cache-by-user"; + await promote(userId, "super-cache-by-user@example.com"); + expect(await isSuperAdmin(userId)).toBe(true); + + await demote(userId); + await invalidatePermissionCache({ userId }); + + expect(await isSuperAdmin(userId)).toBe(false); + }); + + it("clears the demoted user on a roleId invalidation, which names no user", async () => { + // A role's own change arrives with a roleId and nothing else, and the map + // does not record which users a role reaches. The subset cannot be derived, + // so the whole map goes. + const userId = "super-cache-by-role"; + await promote(userId, "super-cache-by-role@example.com"); + expect(await isSuperAdmin(userId)).toBe(true); + + await demote(userId); + await invalidatePermissionCache({ roleId: "any-role-at-all" }); + + expect(await isSuperAdmin(userId)).toBe(false); + }); + + it("leaves an unrelated user's answer alone on a userId invalidation", async () => { + // The discriminating control: an implementation that clears the whole map + // for every hint passes the two cases above and is a different behaviour. + const kept = "super-cache-kept"; + const other = "super-cache-other"; + await promote(kept, "super-cache-kept@example.com"); + await promote(other, "super-cache-other@example.com"); + expect(await isSuperAdmin(kept)).toBe(true); + expect(await isSuperAdmin(other)).toBe(true); + + await demote(kept); + await demote(other); + await invalidatePermissionCache({ userId: other }); + + expect(await isSuperAdmin(kept), "still cached").toBe(true); + expect(await isSuperAdmin(other), "evicted").toBe(false); + }); +});