From 306f4e4c6782e7a8013fc0799b6b824f224852de Mon Sep 17 00:00:00 2001 From: CactusBlue Date: Tue, 1 Apr 2025 16:12:13 -0700 Subject: [PATCH] Permission Robustness (#591) > [!IMPORTANT] > Enhance permission management by adding unique constraints, handling duplicate ID errors, and updating frontend and backend logic with comprehensive tests. > > - **Database**: > - Add unique constraint on `Permission` table for `[tenancyId, queryableId]` in `migration.sql`. > - Update `schema.prisma` to reflect new unique constraints. > - **Backend**: > - Update `crud.tsx` files to handle `PERMISSION_ID_ALREADY_EXISTS` error using `isErrorForNonUniquePermission()`. > - Add `isPrismaUniqueConstraintViolation()` in `prisma-client.tsx` to identify unique constraint violations. > - Add `PermissionIdAlreadyExists` error in `known-errors.tsx`. > - **Frontend**: > - Update `page-client.tsx` and `permission-table.tsx` to check for duplicate permission IDs before creation. > - **Tests**: > - Add tests in `project-permission-definitions.test.ts` and `team-permission-definitions.test.ts` to verify duplicate ID handling. > - Ensure permissions cannot be created with duplicate IDs across project and team contexts. > > This description was created by [Ellipsis](https://www.ellipsis.dev?ref=stack-auth%2Fstack-auth&utm_source=github&utm_medium=referral) for b3ccd15bca9a6dc9cef1a9da09b38ca750d7c2a1. It will automatically update as commits are pushed. --------- Co-authored-by: Konsti Wohlwend Co-authored-by: Zai Shi --- .../migration.sql | 8 ++ apps/backend/prisma/schema.prisma | 5 +- .../project-permission-definitions/crud.tsx | 42 +++++++--- .../team-permission-definitions/crud.tsx | 39 ++++++--- apps/backend/src/lib/permissions.tsx | 9 +- apps/backend/src/prisma-client.tsx | 20 ++++- .../project-permissions/page-client.tsx | 7 +- .../team-permissions/page-client.tsx | 7 +- .../data-table/permission-table.tsx | 10 ++- .../v1/project-permission-definitions.test.ts | 79 ++++++++++++++++++ .../api/v1/project-permissions.test.ts | 83 +++++++++++++++++++ .../v1/team-permission-definitions.test.ts | 79 ++++++++++++++++++ .../endpoints/api/v1/team-permissions.test.ts | 83 +++++++++++++++++++ .../pages-template/concepts/permissions.mdx | 6 +- packages/stack-shared/src/known-errors.tsx | 14 ++++ 15 files changed, 450 insertions(+), 41 deletions(-) create mode 100644 apps/backend/prisma/migrations/20250401220515_permission_unique_constraint/migration.sql diff --git a/apps/backend/prisma/migrations/20250401220515_permission_unique_constraint/migration.sql b/apps/backend/prisma/migrations/20250401220515_permission_unique_constraint/migration.sql new file mode 100644 index 000000000..c50a09be2 --- /dev/null +++ b/apps/backend/prisma/migrations/20250401220515_permission_unique_constraint/migration.sql @@ -0,0 +1,8 @@ +/* + Warnings: + + - A unique constraint covering the columns `[tenancyId,queryableId]` on the table `Permission` will be added. If there are existing duplicate values, this will fail. + +*/ +-- CreateIndex +CREATE UNIQUE INDEX "Permission_tenancyId_queryableId_key" ON "Permission"("tenancyId", "queryableId"); diff --git a/apps/backend/prisma/schema.prisma b/apps/backend/prisma/schema.prisma index 7b39afb02..7a98f56cf 100644 --- a/apps/backend/prisma/schema.prisma +++ b/apps/backend/prisma/schema.prisma @@ -211,7 +211,7 @@ model Permission { description String? - // The scope of the permission. If projectConfigId is set, may be USER or TEAM; if teamId is set, must be TEAM. + // The scope of the permission. If projectConfigId is set, may be PROJECT or TEAM; if teamId is set, must be PROJECT. scope PermissionScope projectConfig ProjectConfig? @relation(fields: [projectConfigId], references: [id], onDelete: Cascade) @@ -224,8 +224,9 @@ model Permission { isDefaultTeamCreatorPermission Boolean @default(false) isDefaultTeamMemberPermission Boolean @default(false) - isDefaultProjectPermission Boolean @default(false) + isDefaultProjectPermission Boolean @default(false) + @@unique([tenancyId, queryableId]) @@unique([projectConfigId, queryableId]) @@unique([tenancyId, teamId, queryableId]) } diff --git a/apps/backend/src/app/api/latest/project-permission-definitions/crud.tsx b/apps/backend/src/app/api/latest/project-permission-definitions/crud.tsx index dce118326..fa640954d 100644 --- a/apps/backend/src/app/api/latest/project-permission-definitions/crud.tsx +++ b/apps/backend/src/app/api/latest/project-permission-definitions/crud.tsx @@ -1,31 +1,47 @@ -import { createPermissionDefinition, deletePermissionDefinition, listPermissionDefinitions, updatePermissionDefinitions } from "@/lib/permissions"; -import { retryTransaction } from "@/prisma-client"; +import { createPermissionDefinition, deletePermissionDefinition, isErrorForNonUniquePermission, listPermissionDefinitions, updatePermissionDefinitions } from "@/lib/permissions"; +import { isPrismaUniqueConstraintViolation, retryTransaction } from "@/prisma-client"; import { createCrudHandlers } from "@/route-handlers/crud-handler"; +import { KnownErrors } from "@stackframe/stack-shared"; import { projectPermissionDefinitionsCrud } from '@stackframe/stack-shared/dist/interface/crud/project-permissions'; import { permissionDefinitionIdSchema, yupObject } from "@stackframe/stack-shared/dist/schema-fields"; import { createLazyProxy } from "@stackframe/stack-shared/dist/utils/proxies"; + export const projectPermissionDefinitionsCrudHandlers = createLazyProxy(() => createCrudHandlers(projectPermissionDefinitionsCrud, { paramsSchema: yupObject({ permission_id: permissionDefinitionIdSchema.defined(), }), async onCreate({ auth, data }) { return await retryTransaction(async (tx) => { - return await createPermissionDefinition(tx, { - scope: "PROJECT", - tenancy: auth.tenancy, - data, - }); + try { + return await createPermissionDefinition(tx, { + scope: "PROJECT", + tenancy: auth.tenancy, + data, + }); + } catch (error) { + if (isErrorForNonUniquePermission(error)) { + throw new KnownErrors.PermissionIdAlreadyExists(data.id); + } + throw error; + } }); }, async onUpdate({ auth, data, params }) { return await retryTransaction(async (tx) => { - return await updatePermissionDefinitions(tx, { - scope: "PROJECT", - tenancy: auth.tenancy, - permissionId: params.permission_id, - data, - }); + try { + return await updatePermissionDefinitions(tx, { + scope: "PROJECT", + tenancy: auth.tenancy, + permissionId: params.permission_id, + data, + }); + } catch (error) { + if (isErrorForNonUniquePermission(error)) { + throw new KnownErrors.PermissionIdAlreadyExists(data.id ?? ''); + } + throw error; + } }); }, async onDelete({ auth, params }) { diff --git a/apps/backend/src/app/api/latest/team-permission-definitions/crud.tsx b/apps/backend/src/app/api/latest/team-permission-definitions/crud.tsx index c32f8f9db..b4fe00cd0 100644 --- a/apps/backend/src/app/api/latest/team-permission-definitions/crud.tsx +++ b/apps/backend/src/app/api/latest/team-permission-definitions/crud.tsx @@ -1,6 +1,7 @@ -import { createPermissionDefinition, deletePermissionDefinition, listPermissionDefinitions, updatePermissionDefinitions } from "@/lib/permissions"; +import { createPermissionDefinition, deletePermissionDefinition, isErrorForNonUniquePermission, listPermissionDefinitions, updatePermissionDefinitions } from "@/lib/permissions"; import { retryTransaction } from "@/prisma-client"; import { createCrudHandlers } from "@/route-handlers/crud-handler"; +import { KnownErrors } from "@stackframe/stack-shared"; import { teamPermissionDefinitionsCrud } from '@stackframe/stack-shared/dist/interface/crud/team-permissions'; import { permissionDefinitionIdSchema, yupObject } from "@stackframe/stack-shared/dist/schema-fields"; import { createLazyProxy } from "@stackframe/stack-shared/dist/utils/proxies"; @@ -11,21 +12,35 @@ export const teamPermissionDefinitionsCrudHandlers = createLazyProxy(() => creat }), async onCreate({ auth, data }) { return await retryTransaction(async (tx) => { - return await createPermissionDefinition(tx, { - scope: "TEAM", - tenancy: auth.tenancy, - data, - }); + try { + return await createPermissionDefinition(tx, { + scope: "TEAM", + tenancy: auth.tenancy, + data, + }); + } catch (error) { + if (isErrorForNonUniquePermission(error)) { + throw new KnownErrors.PermissionIdAlreadyExists(data.id); + } + throw error; + } }); }, async onUpdate({ auth, data, params }) { return await retryTransaction(async (tx) => { - return await updatePermissionDefinitions(tx, { - scope: "TEAM", - tenancy: auth.tenancy, - permissionId: params.permission_id, - data, - }); + try { + return await updatePermissionDefinitions(tx, { + scope: "TEAM", + tenancy: auth.tenancy, + permissionId: params.permission_id, + data, + }); + } catch (error) { + if (isErrorForNonUniquePermission(error)) { + throw new KnownErrors.PermissionIdAlreadyExists(data.id ?? ''); + } + throw error; + } }); }, async onDelete({ auth, params }) { diff --git a/apps/backend/src/lib/permissions.tsx b/apps/backend/src/lib/permissions.tsx index 969b0648a..3837d208f 100644 --- a/apps/backend/src/lib/permissions.tsx +++ b/apps/backend/src/lib/permissions.tsx @@ -1,7 +1,8 @@ +import { isPrismaUniqueConstraintViolation } from "@/prisma-client"; import { TeamSystemPermission as DBTeamSystemPermission, Prisma } from "@prisma/client"; import { KnownErrors } from "@stackframe/stack-shared"; -import { TeamPermissionDefinitionsCrud, TeamPermissionsCrud } from "@stackframe/stack-shared/dist/interface/crud/team-permissions"; import { ProjectPermissionsCrud } from "@stackframe/stack-shared/dist/interface/crud/project-permissions"; +import { TeamPermissionDefinitionsCrud, TeamPermissionsCrud } from "@stackframe/stack-shared/dist/interface/crud/team-permissions"; import { groupBy } from "@stackframe/stack-shared/dist/utils/arrays"; import { StackAssertionError, throwErr } from "@stackframe/stack-shared/dist/utils/errors"; import { stringCompare, typedToLowercase, typedToUppercase } from "@stackframe/stack-shared/dist/utils/strings"; @@ -649,3 +650,9 @@ export async function grantDefaultProjectPermissions( return defaultPermissions.length > 0; } + +export function isErrorForNonUniquePermission(error: unknown): boolean { + return isPrismaUniqueConstraintViolation(error, "Permission", ["tenancyId", "queryableId"]) || + isPrismaUniqueConstraintViolation(error, "Permission", ["projectConfigId", "queryableId"]) || + isPrismaUniqueConstraintViolation(error, "Permission", ["tenancyId", "teamId", "queryableId"]); +} diff --git a/apps/backend/src/prisma-client.tsx b/apps/backend/src/prisma-client.tsx index a78693d3a..999a30975 100644 --- a/apps/backend/src/prisma-client.tsx +++ b/apps/backend/src/prisma-client.tsx @@ -1,7 +1,7 @@ import { Prisma, PrismaClient } from "@prisma/client"; import { withAccelerate } from "@prisma/extension-accelerate"; import { getEnvVariable, getNodeEnvironment } from '@stackframe/stack-shared/dist/utils/env'; -import { filterUndefined, typedFromEntries, typedKeys } from "@stackframe/stack-shared/dist/utils/objects"; +import { deepPlainEquals, filterUndefined, typedFromEntries, typedKeys } from "@stackframe/stack-shared/dist/utils/objects"; import { Result } from "@stackframe/stack-shared/dist/utils/results"; import { traceSpan } from "./utils/telemetry"; @@ -135,3 +135,21 @@ async function rawQueryArray[]>(queries: Q): Promise<[] }); } +// not exhaustive +export const PRISMA_ERROR_CODES = { + VALUE_TOO_LONG: "P2000", + RECORD_NOT_FOUND: "P2001", + UNIQUE_CONSTRAINT_VIOLATION: "P2002", + FOREIGN_CONSTRAINT_VIOLATION: "P2003", + GENERIC_CONSTRAINT_VIOLATION: "P2004", +} as const; + +export function isPrismaError(error: unknown, code: keyof typeof PRISMA_ERROR_CODES): error is Prisma.PrismaClientKnownRequestError { + return error instanceof Prisma.PrismaClientKnownRequestError && error.code === PRISMA_ERROR_CODES[code]; +} + +export function isPrismaUniqueConstraintViolation(error: unknown, modelName: string, target: string | string[]): error is Prisma.PrismaClientKnownRequestError { + if (!isPrismaError(error, "UNIQUE_CONSTRAINT_VIOLATION")) return false; + if (!error.meta?.target) return false; + return error.meta.modelName === modelName && deepPlainEquals(error.meta.target, target); +} diff --git a/apps/dashboard/src/app/(main)/(protected)/projects/[projectId]/project-permissions/page-client.tsx b/apps/dashboard/src/app/(main)/(protected)/projects/[projectId]/project-permissions/page-client.tsx index ecabf029e..fa3a3bb49 100644 --- a/apps/dashboard/src/app/(main)/(protected)/projects/[projectId]/project-permissions/page-client.tsx +++ b/apps/dashboard/src/app/(main)/(protected)/projects/[projectId]/project-permissions/page-client.tsx @@ -40,17 +40,18 @@ function CreateDialog(props: { onOpenChange: (open: boolean) => void, }) { const stackAdminApp = useAdminApp(); - const permissions = stackAdminApp.useProjectPermissionDefinitions(); + const projectPermissions = stackAdminApp.useProjectPermissionDefinitions(); + const combinedPermissions = [...stackAdminApp.useTeamPermissionDefinitions(), ...projectPermissions]; const formSchema = yup.object({ id: yup.string().defined() - .notOneOf(permissions.map((p) => p.id), "ID already exists") + .notOneOf(combinedPermissions.map((p) => p.id), "ID already exists") .matches(/^[a-z0-9_:]+$/, 'Only lowercase letters, numbers, ":" and "_" are allowed') .label("ID"), description: yup.string().label("Description"), containedPermissionIds: yup.array().of(yup.string().defined()).defined().default([]).meta({ stackFormFieldRender: (props) => ( - + ), }), }); diff --git a/apps/dashboard/src/app/(main)/(protected)/projects/[projectId]/team-permissions/page-client.tsx b/apps/dashboard/src/app/(main)/(protected)/projects/[projectId]/team-permissions/page-client.tsx index 3af1f3789..b251bc5e2 100644 --- a/apps/dashboard/src/app/(main)/(protected)/projects/[projectId]/team-permissions/page-client.tsx +++ b/apps/dashboard/src/app/(main)/(protected)/projects/[projectId]/team-permissions/page-client.tsx @@ -41,17 +41,18 @@ function CreateDialog(props: { onOpenChange: (open: boolean) => void, }) { const stackAdminApp = useAdminApp(); - const permissions = stackAdminApp.useTeamPermissionDefinitions(); + const teamPermissions = stackAdminApp.useTeamPermissionDefinitions(); + const combinedPermissions = [...teamPermissions, ...stackAdminApp.useProjectPermissionDefinitions()]; const formSchema = yup.object({ id: yup.string().defined() - .notOneOf(permissions.map((p) => p.id), "ID already exists") + .notOneOf(combinedPermissions.map((p) => p.id), "ID already exists") .matches(/^[a-z0-9_:]+$/, 'Only lowercase letters, numbers, ":" and "_" are allowed') .label("ID"), description: yup.string().label("Description"), containedPermissionIds: yup.array().of(yup.string().defined()).defined().default([]).meta({ stackFormFieldRender: (props) => ( - + ), }), }); diff --git a/apps/dashboard/src/components/data-table/permission-table.tsx b/apps/dashboard/src/components/data-table/permission-table.tsx index 459cbc250..1cc1cb6fb 100644 --- a/apps/dashboard/src/components/data-table/permission-table.tsx +++ b/apps/dashboard/src/components/data-table/permission-table.tsx @@ -31,9 +31,11 @@ function EditDialog(props: { permissionType: PermissionType, }) { const stackAdminApp = useAdminApp(); - const permissions = props.permissionType === 'project' - ? stackAdminApp.useProjectPermissionDefinitions() - : stackAdminApp.useTeamPermissionDefinitions(); + const teamPermissions = stackAdminApp.useTeamPermissionDefinitions(); + const projectPermissions = stackAdminApp.useProjectPermissionDefinitions(); + const permissions = props.permissionType === 'project' ? projectPermissions : teamPermissions; + const combinedPermissions = [...teamPermissions, ...projectPermissions]; + const currentPermission = permissions.find((p) => p.id === props.selectedPermissionId); if (!currentPermission) { return null; @@ -42,7 +44,7 @@ function EditDialog(props: { const formSchema = yup.object({ id: yup.string() .defined() - .notOneOf(permissions.map((p) => p.id).filter(p => p !== props.selectedPermissionId), "ID already exists") + .notOneOf(combinedPermissions.map((p) => p.id).filter(p => p !== props.selectedPermissionId), "ID already exists") .matches(/^[a-z0-9_:]+$/, 'Only lowercase letters, numbers, ":" and "_" are allowed') .label("ID"), description: yup.string().label("Description"), diff --git a/apps/e2e/tests/backend/endpoints/api/v1/project-permission-definitions.test.ts b/apps/e2e/tests/backend/endpoints/api/v1/project-permission-definitions.test.ts index c5fe0f7f8..a971e5817 100644 --- a/apps/e2e/tests/backend/endpoints/api/v1/project-permission-definitions.test.ts +++ b/apps/e2e/tests/backend/endpoints/api/v1/project-permission-definitions.test.ts @@ -173,3 +173,82 @@ it("creates, updates, and deletes a new user permission", async ({ expect }) => } `); }); + +it("handles duplicate permission IDs correctly", async ({ expect }) => { + backendContext.set({ projectKeys: InternalProjectKeys }); + const { adminAccessToken } = await Project.createAndGetAdminToken(); + + // Create first permission + const response1 = await niceBackendFetch(`/api/v1/project-permission-definitions`, { + accessType: "admin", + method: "POST", + body: { + id: 'duplicate_test', + description: "Test permission" + }, + headers: { + 'x-stack-admin-access-token': adminAccessToken + }, + }); + expect(response1.status).toBe(201); + + // Try to create another permission with the same ID + const response2 = await niceBackendFetch(`/api/v1/project-permission-definitions`, { + accessType: "admin", + method: "POST", + body: { + id: 'duplicate_test', + description: "Another test permission" + }, + headers: { + 'x-stack-admin-access-token': adminAccessToken + }, + }); + expect(response2.status).toBe(400); + expect(response2.body).toHaveProperty("code", "PERMISSION_ID_ALREADY_EXISTS"); + + // Create another permission + const response3 = await niceBackendFetch(`/api/v1/project-permission-definitions`, { + accessType: "admin", + method: "POST", + body: { + id: 'update_test', + description: "Test permission for update" + }, + headers: { + 'x-stack-admin-access-token': adminAccessToken + }, + }); + expect(response3.status).toBe(201); + + // Update the first permission to have the ID of the second (which should fail) + const response4 = await niceBackendFetch(`/api/v1/project-permission-definitions/duplicate_test`, { + accessType: "admin", + method: "PATCH", + body: { + id: 'update_test', + description: "Updated description" + }, + headers: { + 'x-stack-admin-access-token': adminAccessToken + }, + }); + expect(response4.status).toBe(400); + expect(response4.body).toHaveProperty("code", "PERMISSION_ID_ALREADY_EXISTS"); + + // Clean up + await niceBackendFetch(`/api/v1/project-permission-definitions/duplicate_test`, { + accessType: "admin", + method: "DELETE", + headers: { + 'x-stack-admin-access-token': adminAccessToken + }, + }); + await niceBackendFetch(`/api/v1/project-permission-definitions/update_test`, { + accessType: "admin", + method: "DELETE", + headers: { + 'x-stack-admin-access-token': adminAccessToken + }, + }); +}); diff --git a/apps/e2e/tests/backend/endpoints/api/v1/project-permissions.test.ts b/apps/e2e/tests/backend/endpoints/api/v1/project-permissions.test.ts index e1df627b8..43cc872a4 100644 --- a/apps/e2e/tests/backend/endpoints/api/v1/project-permissions.test.ts +++ b/apps/e2e/tests/backend/endpoints/api/v1/project-permissions.test.ts @@ -315,3 +315,86 @@ it("should trigger project permission webhook when a permission is revoked from } `); }); + +it("should not be able to create a permission with the same name as an existing project permission", async ({ expect }) => { + await Auth.Otp.signIn(); + const { adminAccessToken } = await Project.createAndGetAdminToken(); + + // First, create a project permission definition + const createTeamPermissionResponse = await niceBackendFetch(`/api/v1/project-permission-definitions`, { + accessType: "admin", + method: "POST", + body: { + id: 'custom_project_permission', + description: 'A custom project permission', + }, + headers: { + 'x-stack-admin-access-token': adminAccessToken + }, + }); + + expect(createTeamPermissionResponse).toMatchInlineSnapshot(` + NiceResponse { + "status": 201, + "body": { + "contained_permission_ids": [], + "description": "A custom project permission", + "id": "custom_project_permission", + }, + "headers": Headers {