From ff9aa2cf774d039ea7a77b53fa53661954d224b2 Mon Sep 17 00:00:00 2001 From: Flo <53355483+Flo4604@users.noreply.github.com> Date: Fri, 24 Jan 2025 20:29:15 +0100 Subject: [PATCH 01/13] try to load role/perm before creating to check for duplicates --- .../routes/v1_permissions_createPermission.ts | 60 +++++++++++++++---- .../src/routes/v1_permissions_createRole.ts | 60 +++++++++++++++---- 2 files changed, 99 insertions(+), 21 deletions(-) diff --git a/apps/api/src/routes/v1_permissions_createPermission.ts b/apps/api/src/routes/v1_permissions_createPermission.ts index 70a8823e6a..0c80711ff9 100644 --- a/apps/api/src/routes/v1_permissions_createPermission.ts +++ b/apps/api/src/routes/v1_permissions_createPermission.ts @@ -6,7 +6,7 @@ import { validation } from "@unkey/validation"; import { insertUnkeyAuditLog } from "@/pkg/audit"; import { rootKeyAuth } from "@/pkg/auth/root_key"; import { openApiErrorResponses } from "@/pkg/errors"; -import { schema } from "@unkey/db"; +import { eq, schema } from "@unkey/db"; import { newId } from "@unkey/id"; import { buildUnkeyQuery } from "@unkey/rbac"; @@ -29,7 +29,8 @@ const route = createRoute({ description: validation.description.optional().openapi({ description: "Explain what this permission does. This is just for your team, your users will not see this.", - example: "record.write can create new dns records for our domains.", + example: + "record.write can create new dns records for our domains.", }), }), }, @@ -67,7 +68,7 @@ export const registerV1PermissionsCreatePermission = (app: App) => const req = c.req.valid("json"); const auth = await rootKeyAuth( c, - buildUnkeyQuery(({ or }) => or("*", "rbac.*.create_permission")), + buildUnkeyQuery(({ or }) => or("*", "rbac.*.create_permission")) ); const { db } = c.get("services"); @@ -78,17 +79,56 @@ export const registerV1PermissionsCreatePermission = (app: App) => name: req.name, description: req.description, }; + await db.primary.transaction(async (tx) => { - await tx - .insert(schema.permissions) - .values(permission) - .onDuplicateKeyUpdate({ - set: { - name: req.name, - description: req.description, + const currentPermission = await tx.query.permissions.findFirst({ + where: (table, { and, eq }) => + and( + eq(table.workspaceId, auth.authorizedWorkspaceId), + eq(table.name, req.name) + ), + columns: { + id: true, + }, + }); + + if (currentPermission) { + permission.id = currentPermission.id as `perm_${string}`; + + await tx + .update(schema.permissions) + .set({ description: req.description }) + .where(eq(schema.permissions.id, permission.id)); + + await insertUnkeyAuditLog(c, tx, { + workspaceId: auth.authorizedWorkspaceId, + event: "permission.update", + actor: { + type: "key", + id: auth.key.id, + }, + description: `Created ${permission.id}`, + resources: [ + { + type: "permission", + id: permission.id, + meta: { + name: permission.name, + description: permission.description, + }, + }, + ], + context: { + location: c.get("location"), + userAgent: c.get("userAgent"), }, }); + return; + } + + await tx.insert(schema.permissions).values(permission); + await insertUnkeyAuditLog(c, tx, { workspaceId: auth.authorizedWorkspaceId, event: "permission.create", diff --git a/apps/api/src/routes/v1_permissions_createRole.ts b/apps/api/src/routes/v1_permissions_createRole.ts index dc106dda7d..1ad384be4e 100644 --- a/apps/api/src/routes/v1_permissions_createRole.ts +++ b/apps/api/src/routes/v1_permissions_createRole.ts @@ -4,7 +4,7 @@ import { createRoute, z } from "@hono/zod-openapi"; import { insertUnkeyAuditLog } from "@/pkg/audit"; import { rootKeyAuth } from "@/pkg/auth/root_key"; import { openApiErrorResponses } from "@/pkg/errors"; -import { schema } from "@unkey/db"; +import { eq, schema } from "@unkey/db"; import { newId } from "@unkey/id"; import { buildUnkeyQuery } from "@unkey/rbac"; import { validation } from "@unkey/validation"; @@ -28,7 +28,8 @@ const route = createRoute({ description: validation.description.optional().openapi({ description: "Explain what this role does. This is just for your team, your users will not see this.", - example: "dns.records.manager can read and write dns records for our domains.", + example: + "dns.records.manager can read and write dns records for our domains.", }), }), }, @@ -66,7 +67,7 @@ export const registerV1PermissionsCreateRole = (app: App) => const req = c.req.valid("json"); const auth = await rootKeyAuth( c, - buildUnkeyQuery(({ or }) => or("*", "rbac.*.create_role")), + buildUnkeyQuery(({ or }) => or("*", "rbac.*.create_role")) ); const { db } = c.get("services"); @@ -79,15 +80,53 @@ export const registerV1PermissionsCreateRole = (app: App) => }; await db.primary.transaction(async (tx) => { - await tx - .insert(schema.roles) - .values(role) - .onDuplicateKeyUpdate({ - set: { - name: req.name, - description: req.description, + const currentRole = await tx.query.roles.findFirst({ + where: (table, { and, eq }) => + and( + eq(table.workspaceId, auth.authorizedWorkspaceId), + eq(table.name, req.name) + ), + columns: { + id: true, + }, + }); + + if (currentRole) { + role.id = currentRole.id as `role_${string}`; + + await tx + .update(schema.roles) + .set({ description: req.description }) + .where(eq(schema.roles.id, role.id)); + + await insertUnkeyAuditLog(c, tx, { + workspaceId: auth.authorizedWorkspaceId, + event: "role.update", + actor: { + type: "key", + id: auth.key.id, + }, + description: `Updated ${role.id}`, + resources: [ + { + type: "role", + id: role.id, + meta: { + name: role.name, + description: role.description, + }, + }, + ], + context: { + location: c.get("location"), + userAgent: c.get("userAgent"), }, }); + + return; + } + + await tx.insert(schema.roles).values(role); await insertUnkeyAuditLog(c, tx, { workspaceId: auth.authorizedWorkspaceId, event: "role.create", @@ -106,7 +145,6 @@ export const registerV1PermissionsCreateRole = (app: App) => }, }, ], - context: { location: c.get("location"), userAgent: c.get("userAgent") }, }); }); From cc081c664ee99f88241337a3e2e1c69f40dcf59c Mon Sep 17 00:00:00 2001 From: Flo <53355483+Flo4604@users.noreply.github.com> Date: Wed, 29 Jan 2025 18:02:47 +0100 Subject: [PATCH 02/13] fix formatting --- apps/api/src/routes/v1_permissions_createPermission.ts | 10 +++------- apps/api/src/routes/v1_permissions_createRole.ts | 10 +++------- 2 files changed, 6 insertions(+), 14 deletions(-) diff --git a/apps/api/src/routes/v1_permissions_createPermission.ts b/apps/api/src/routes/v1_permissions_createPermission.ts index 0c80711ff9..d3df192d0f 100644 --- a/apps/api/src/routes/v1_permissions_createPermission.ts +++ b/apps/api/src/routes/v1_permissions_createPermission.ts @@ -29,8 +29,7 @@ const route = createRoute({ description: validation.description.optional().openapi({ description: "Explain what this permission does. This is just for your team, your users will not see this.", - example: - "record.write can create new dns records for our domains.", + example: "record.write can create new dns records for our domains.", }), }), }, @@ -68,7 +67,7 @@ export const registerV1PermissionsCreatePermission = (app: App) => const req = c.req.valid("json"); const auth = await rootKeyAuth( c, - buildUnkeyQuery(({ or }) => or("*", "rbac.*.create_permission")) + buildUnkeyQuery(({ or }) => or("*", "rbac.*.create_permission")), ); const { db } = c.get("services"); @@ -83,10 +82,7 @@ export const registerV1PermissionsCreatePermission = (app: App) => await db.primary.transaction(async (tx) => { const currentPermission = await tx.query.permissions.findFirst({ where: (table, { and, eq }) => - and( - eq(table.workspaceId, auth.authorizedWorkspaceId), - eq(table.name, req.name) - ), + and(eq(table.workspaceId, auth.authorizedWorkspaceId), eq(table.name, req.name)), columns: { id: true, }, diff --git a/apps/api/src/routes/v1_permissions_createRole.ts b/apps/api/src/routes/v1_permissions_createRole.ts index 1ad384be4e..1eaac82870 100644 --- a/apps/api/src/routes/v1_permissions_createRole.ts +++ b/apps/api/src/routes/v1_permissions_createRole.ts @@ -28,8 +28,7 @@ const route = createRoute({ description: validation.description.optional().openapi({ description: "Explain what this role does. This is just for your team, your users will not see this.", - example: - "dns.records.manager can read and write dns records for our domains.", + example: "dns.records.manager can read and write dns records for our domains.", }), }), }, @@ -67,7 +66,7 @@ export const registerV1PermissionsCreateRole = (app: App) => const req = c.req.valid("json"); const auth = await rootKeyAuth( c, - buildUnkeyQuery(({ or }) => or("*", "rbac.*.create_role")) + buildUnkeyQuery(({ or }) => or("*", "rbac.*.create_role")), ); const { db } = c.get("services"); @@ -82,10 +81,7 @@ export const registerV1PermissionsCreateRole = (app: App) => await db.primary.transaction(async (tx) => { const currentRole = await tx.query.roles.findFirst({ where: (table, { and, eq }) => - and( - eq(table.workspaceId, auth.authorizedWorkspaceId), - eq(table.name, req.name) - ), + and(eq(table.workspaceId, auth.authorizedWorkspaceId), eq(table.name, req.name)), columns: { id: true, }, From 7abeb1d99b77f774340a75c51a19686a3bdbe316 Mon Sep 17 00:00:00 2001 From: Flo <53355483+Flo4604@users.noreply.github.com> Date: Wed, 29 Jan 2025 18:09:05 +0100 Subject: [PATCH 03/13] remove duplicate file and fix log description --- apps/api/src/routes/v1_keys_ami.ts | 174 ------------------ .../routes/v1_permissions_createPermission.ts | 4 +- 2 files changed, 2 insertions(+), 176 deletions(-) delete mode 100644 apps/api/src/routes/v1_keys_ami.ts diff --git a/apps/api/src/routes/v1_keys_ami.ts b/apps/api/src/routes/v1_keys_ami.ts deleted file mode 100644 index a32b4172d4..0000000000 --- a/apps/api/src/routes/v1_keys_ami.ts +++ /dev/null @@ -1,174 +0,0 @@ -import type { App } from "@/pkg/hono/app"; -import { createRoute, z } from "@hono/zod-openapi"; - -import { rootKeyAuth } from "@/pkg/auth/root_key"; -import { UnkeyApiError, openApiErrorResponses } from "@/pkg/errors"; -import { sha256 } from "@unkey/hash"; -import { buildUnkeyQuery } from "@unkey/rbac"; - -const route = createRoute({ - tags: ["keys"], - operationId: "whoami", - method: "post", - path: "/v1/keys.whoami", - security: [{ bearerAuth: [] }], - request: { - body: { - required: true, - content: { - "application/json": { - schema: z.object({ - key: z.string().min(1).openapi({ - description: "The actual key to fetch", - example: "sk_123", - }), - }), - }, - }, - }, - }, - - responses: { - 200: { - description: "The configuration for a single key", - content: { - "application/json": { - schema: z.object({ - id: z.string().openapi({ - description: "The ID of the key", - example: "key_123", - }), - name: z.string().optional().openapi({ - description: "The name of the key", - example: "API Key 1", - }), - remaining: z.number().int().optional().openapi({ - description: "The remaining number of requests for the key", - example: 1000, - }), - identity: z - .object({ - id: z.string().openapi({ - description: "The identity ID associated with the key", - example: "id_123", - }), - externalId: z.string().openapi({ - description: "The external identity ID associated with the key", - example: "ext123", - }), - }) - .optional() - .openapi({ - description: "The identity object associated with the key", - }), - meta: z - .record(z.unknown()) - .optional() - .openapi({ - description: "Metadata associated with the key", - example: { role: "admin", plan: "premium" }, - }), - createdAt: z.number().int().openapi({ - description: "The timestamp in milliseconds when the key was created", - example: 1620000000000, - }), - enabled: z.boolean().openapi({ - description: "Whether the key is enabled", - example: true, - }), - environment: z.string().optional().openapi({ - description: "The environment the key is associated with", - example: "production", - }), - }), - }, - }, - }, - ...openApiErrorResponses, - }, -}); - -export type Route = typeof route; -export type V1KeysWhoAmIRequest = z.infer< - (typeof route.request.body.content)["application/json"]["schema"] ->; -export type V1KeysWhoAmIResponse = z.infer< - (typeof route.responses)[200]["content"]["application/json"]["schema"] ->; - -export const registerV1KeysWhoAmI = (app: App) => - app.openapi(route, async (c) => { - const { key: secret } = c.req.valid("json"); - const { cache, db } = c.get("services"); - const hash = await sha256(secret); - const { val: data, err } = await cache.keyByHash.swr(hash, async () => { - const dbRes = await db.readonly.query.keys.findFirst({ - where: (table, { eq, and, isNull }) => and(eq(table.hash, hash), isNull(table.deletedAt)), - with: { - keyAuth: { - with: { - api: true, - }, - }, - identity: true, - }, - }); - - if (!dbRes) { - return null; - } - - return { - key: { - ...dbRes, - }, - api: dbRes.keyAuth.api, - identity: dbRes.identity, - } as any; // this was necessary so that we don't need to return the workspace and other types defined in keyByHash - }); - - if (err) { - throw new UnkeyApiError({ - code: "INTERNAL_SERVER_ERROR", - message: `unable to load key: ${err.message}`, - }); - } - if (!data) { - throw new UnkeyApiError({ - code: "NOT_FOUND", - message: "Key not found", - }); - } - const { api, key } = data; - const auth = await rootKeyAuth( - c, - buildUnkeyQuery(({ or }) => or("*", "api.*.read_key", `api.${api.id}.read_key`)), - ); - - if (key.workspaceId !== auth.authorizedWorkspaceId) { - throw new UnkeyApiError({ - code: "NOT_FOUND", - message: "Key not found", - }); - } - let meta = key.meta ? JSON.parse(key.meta) : undefined; - if (!meta || Object.keys(meta).length === 0) { - meta = undefined; - } - - return c.json({ - id: key.id, - name: key.name ?? undefined, - remaining: key.remaining ?? undefined, - identity: data.identity - ? { - id: data.identity.id, - externalId: data.identity.externalId, - } - : undefined, - meta: meta, - createdAt: key.createdAt.getTime(), - enabled: key.enabled, - environment: key.environment ?? undefined, - }); - }); diff --git a/apps/api/src/routes/v1_permissions_createPermission.ts b/apps/api/src/routes/v1_permissions_createPermission.ts index d3df192d0f..241a60ff7d 100644 --- a/apps/api/src/routes/v1_permissions_createPermission.ts +++ b/apps/api/src/routes/v1_permissions_createPermission.ts @@ -103,7 +103,7 @@ export const registerV1PermissionsCreatePermission = (app: App) => type: "key", id: auth.key.id, }, - description: `Created ${permission.id}`, + description: `Updated ${permission.id}`, resources: [ { type: "permission", @@ -124,7 +124,6 @@ export const registerV1PermissionsCreatePermission = (app: App) => } await tx.insert(schema.permissions).values(permission); - await insertUnkeyAuditLog(c, tx, { workspaceId: auth.authorizedWorkspaceId, event: "permission.create", @@ -147,6 +146,7 @@ export const registerV1PermissionsCreatePermission = (app: App) => context: { location: c.get("location"), userAgent: c.get("userAgent") }, }); }); + return c.json({ permissionId: permission.id, }); From bfdc3d7cd01cd64247d411a1aadb80dba4f64dcf Mon Sep 17 00:00:00 2001 From: Flo <53355483+Flo4604@users.noreply.github.com> Date: Mon, 3 Feb 2025 17:53:39 +0100 Subject: [PATCH 04/13] fix: catch duplicate key errors --- .../routes/v1_permissions_createPermission.ts | 57 +++++-------------- .../src/routes/v1_permissions_createRole.ts | 57 +++++-------------- 2 files changed, 26 insertions(+), 88 deletions(-) diff --git a/apps/api/src/routes/v1_permissions_createPermission.ts b/apps/api/src/routes/v1_permissions_createPermission.ts index 241a60ff7d..fd3738e35f 100644 --- a/apps/api/src/routes/v1_permissions_createPermission.ts +++ b/apps/api/src/routes/v1_permissions_createPermission.ts @@ -5,8 +5,9 @@ import { validation } from "@unkey/validation"; import { insertUnkeyAuditLog } from "@/pkg/audit"; import { rootKeyAuth } from "@/pkg/auth/root_key"; -import { openApiErrorResponses } from "@/pkg/errors"; -import { eq, schema } from "@unkey/db"; +import { UnkeyApiError, openApiErrorResponses } from "@/pkg/errors"; +import { DatabaseError } from "@planetscale/database"; +import { schema } from "@unkey/db"; import { newId } from "@unkey/id"; import { buildUnkeyQuery } from "@unkey/rbac"; @@ -80,50 +81,18 @@ export const registerV1PermissionsCreatePermission = (app: App) => }; await db.primary.transaction(async (tx) => { - const currentPermission = await tx.query.permissions.findFirst({ - where: (table, { and, eq }) => - and(eq(table.workspaceId, auth.authorizedWorkspaceId), eq(table.name, req.name)), - columns: { - id: true, - }, - }); - - if (currentPermission) { - permission.id = currentPermission.id as `perm_${string}`; - - await tx - .update(schema.permissions) - .set({ description: req.description }) - .where(eq(schema.permissions.id, permission.id)); - - await insertUnkeyAuditLog(c, tx, { - workspaceId: auth.authorizedWorkspaceId, - event: "permission.update", - actor: { - type: "key", - id: auth.key.id, - }, - description: `Updated ${permission.id}`, - resources: [ - { - type: "permission", - id: permission.id, - meta: { - name: permission.name, - description: permission.description, - }, - }, - ], - context: { - location: c.get("location"), - userAgent: c.get("userAgent"), - }, + await tx + .insert(schema.permissions) + .values(permission) + .catch((e) => { + if (e instanceof DatabaseError && e.body.message.includes("Duplicate entry")) { + throw new UnkeyApiError({ + code: "PRECONDITION_FAILED", + message: "Duplicate Permission", + }); + } }); - return; - } - - await tx.insert(schema.permissions).values(permission); await insertUnkeyAuditLog(c, tx, { workspaceId: auth.authorizedWorkspaceId, event: "permission.create", diff --git a/apps/api/src/routes/v1_permissions_createRole.ts b/apps/api/src/routes/v1_permissions_createRole.ts index 1eaac82870..f0a73d0cd2 100644 --- a/apps/api/src/routes/v1_permissions_createRole.ts +++ b/apps/api/src/routes/v1_permissions_createRole.ts @@ -3,8 +3,9 @@ import { createRoute, z } from "@hono/zod-openapi"; import { insertUnkeyAuditLog } from "@/pkg/audit"; import { rootKeyAuth } from "@/pkg/auth/root_key"; -import { openApiErrorResponses } from "@/pkg/errors"; -import { eq, schema } from "@unkey/db"; +import { UnkeyApiError, openApiErrorResponses } from "@/pkg/errors"; +import { DatabaseError } from "@planetscale/database"; +import { schema } from "@unkey/db"; import { newId } from "@unkey/id"; import { buildUnkeyQuery } from "@unkey/rbac"; import { validation } from "@unkey/validation"; @@ -79,50 +80,18 @@ export const registerV1PermissionsCreateRole = (app: App) => }; await db.primary.transaction(async (tx) => { - const currentRole = await tx.query.roles.findFirst({ - where: (table, { and, eq }) => - and(eq(table.workspaceId, auth.authorizedWorkspaceId), eq(table.name, req.name)), - columns: { - id: true, - }, - }); - - if (currentRole) { - role.id = currentRole.id as `role_${string}`; - - await tx - .update(schema.roles) - .set({ description: req.description }) - .where(eq(schema.roles.id, role.id)); - - await insertUnkeyAuditLog(c, tx, { - workspaceId: auth.authorizedWorkspaceId, - event: "role.update", - actor: { - type: "key", - id: auth.key.id, - }, - description: `Updated ${role.id}`, - resources: [ - { - type: "role", - id: role.id, - meta: { - name: role.name, - description: role.description, - }, - }, - ], - context: { - location: c.get("location"), - userAgent: c.get("userAgent"), - }, + await tx + .insert(schema.roles) + .values(role) + .catch((e) => { + if (e instanceof DatabaseError && e.body.message.includes("Duplicate entry")) { + throw new UnkeyApiError({ + code: "PRECONDITION_FAILED", + message: "Duplicate Role", + }); + } }); - return; - } - - await tx.insert(schema.roles).values(role); await insertUnkeyAuditLog(c, tx, { workspaceId: auth.authorizedWorkspaceId, event: "role.create", From 938c5569a4c644aa8f6d9b09f1dc3a9dc99c608e Mon Sep 17 00:00:00 2001 From: Flo <53355483+Flo4604@users.noreply.github.com> Date: Mon, 3 Feb 2025 20:10:52 +0100 Subject: [PATCH 05/13] fix: rename NOT_UNIQUE to CONFLICT and change tests to reflect --- apps/api/src/pkg/errors/http.ts | 4 +-- apps/api/src/pkg/errors/openapi_responses.ts | 9 +++++ apps/api/src/routes/v1_keys_deleteKey.ts | 2 +- .../v1_migrations_createKey.happy.test.ts | 2 +- .../api/src/routes/v1_migrations_createKey.ts | 2 +- ...permissions_createPermission.error.test.ts | 36 +++++++++++++++++++ ...permissions_createPermission.happy.test.ts | 25 ------------- .../routes/v1_permissions_createPermission.ts | 2 +- .../v1_permissions_createRole.error.test.ts | 34 ++++++++++++++++++ .../v1_permissions_createRole.happy.test.ts | 22 ------------ .../src/routes/v1_permissions_createRole.ts | 2 +- .../error_code/unkey_database_not_unique.go | 4 +-- 12 files changed, 88 insertions(+), 56 deletions(-) diff --git a/apps/api/src/pkg/errors/http.ts b/apps/api/src/pkg/errors/http.ts index 965a3fdd33..6c63aa9408 100644 --- a/apps/api/src/pkg/errors/http.ts +++ b/apps/api/src/pkg/errors/http.ts @@ -13,7 +13,7 @@ const ErrorCode = z.enum([ "USAGE_EXCEEDED", "DISABLED", "NOT_FOUND", - "NOT_UNIQUE", + "CONFLICT", "RATE_LIMITED", "UNAUTHORIZED", "PRECONDITION_FAILED", @@ -80,7 +80,7 @@ function codeToStatus(code: z.infer): StatusCode { return 404; case "METHOD_NOT_ALLOWED": return 405; - case "NOT_UNIQUE": + case "CONFLICT": return 409; case "DELETE_PROTECTED": case "PRECONDITION_FAILED": diff --git a/apps/api/src/pkg/errors/openapi_responses.ts b/apps/api/src/pkg/errors/openapi_responses.ts index 0afa28520b..9ac2867e29 100644 --- a/apps/api/src/pkg/errors/openapi_responses.ts +++ b/apps/api/src/pkg/errors/openapi_responses.ts @@ -46,6 +46,15 @@ export const openApiErrorResponses = { }, }, }, + 412: { + description: + "This response is sent when the request does not meet one or more preconditions required for fulfillment.", + content: { + "application/json": { + schema: errorSchemaFactory(z.enum(["CONFLICT"])).openapi("ErrConflict"), + }, + }, + }, 429: { description: `The user has sent too many requests in a given amount of time ("rate limiting")`, content: { diff --git a/apps/api/src/routes/v1_keys_deleteKey.ts b/apps/api/src/routes/v1_keys_deleteKey.ts index 0718b208bf..8c054f519c 100644 --- a/apps/api/src/routes/v1_keys_deleteKey.ts +++ b/apps/api/src/routes/v1_keys_deleteKey.ts @@ -26,7 +26,7 @@ const route = createRoute({ }), permanent: z.boolean().default(false).optional().openapi({ description: - "By default Unkey soft deletes keys, so they may be recovered later. If you want to permanently delete it, set permanent=true. This might be necessary if you run into NOT_UNIQUE errors during key migration.", + "By default Unkey soft deletes keys, so they may be recovered later. If you want to permanently delete it, set permanent=true. This might be necessary if you run into CONFLICT errors during key migration.", }), }), }, diff --git a/apps/api/src/routes/v1_migrations_createKey.happy.test.ts b/apps/api/src/routes/v1_migrations_createKey.happy.test.ts index d15bba2e9d..c67d17636a 100644 --- a/apps/api/src/routes/v1_migrations_createKey.happy.test.ts +++ b/apps/api/src/routes/v1_migrations_createKey.happy.test.ts @@ -488,7 +488,7 @@ test("an error rolls back and does not create any keys", async (t) => { }); expect(res.status).toEqual(409); - expect(res.body.error.code).toEqual("NOT_UNIQUE"); + expect(res.body.error.code).toEqual("CONFLICT"); for (let i = 0; i < req.length; i++) { const key = await h.db.primary.query.keys.findFirst({ diff --git a/apps/api/src/routes/v1_migrations_createKey.ts b/apps/api/src/routes/v1_migrations_createKey.ts index b17de8a501..078858a337 100644 --- a/apps/api/src/routes/v1_migrations_createKey.ts +++ b/apps/api/src/routes/v1_migrations_createKey.ts @@ -559,7 +559,7 @@ export const registerV1MigrationsCreateKeys = (app: App) => workspaceId: authorizedWorkspaceId, }); throw new UnkeyApiError({ - code: "NOT_UNIQUE", + code: "CONFLICT", message: e.body.message, }); } diff --git a/apps/api/src/routes/v1_permissions_createPermission.error.test.ts b/apps/api/src/routes/v1_permissions_createPermission.error.test.ts index 0c6dc3adfb..1b02b94f05 100644 --- a/apps/api/src/routes/v1_permissions_createPermission.error.test.ts +++ b/apps/api/src/routes/v1_permissions_createPermission.error.test.ts @@ -1,5 +1,6 @@ import { describe, expect, test } from "vitest"; +import { randomUUID } from "node:crypto"; import { IntegrationHarness } from "src/pkg/testutil/integration-harness"; import type { @@ -39,3 +40,38 @@ describe.each([ }); }); }); + +test("creating the same permission twice does not error", async (t) => { + const h = await IntegrationHarness.init(t); + const root = await h.createRootKey(["rbac.*.create_permission"]); + + const name = randomUUID(); + // The First request should succeed + // The Second request should fail + const expectedStatuses: Record = { + 0: 200, + 1: 409, + }; + + for (let i = 0; i < 2; i++) { + const res = await h.post< + V1PermissionsCreatePermissionRequest, + V1PermissionsCreatePermissionResponse + >({ + url: "/v1/permissions.createPermission", + headers: { + "Content-Type": "application/json", + Authorization: `Bearer ${root.key}`, + }, + body: { + name, + }, + }); + + const expectedStatus = expectedStatuses[i]; + expect( + res.status, + `expected ${expectedStatus}, received: ${JSON.stringify(res, null, 2)}`, + ).toBe(expectedStatus); + } +}); diff --git a/apps/api/src/routes/v1_permissions_createPermission.happy.test.ts b/apps/api/src/routes/v1_permissions_createPermission.happy.test.ts index 3db7878711..d9bfc0ad43 100644 --- a/apps/api/src/routes/v1_permissions_createPermission.happy.test.ts +++ b/apps/api/src/routes/v1_permissions_createPermission.happy.test.ts @@ -32,28 +32,3 @@ test("creates new permission", async (t) => { }); expect(found).toBeDefined(); }); - -test("creating the same permission twice does not error", async (t) => { - const h = await IntegrationHarness.init(t); - const root = await h.createRootKey(["rbac.*.create_permission"]); - - const name = randomUUID(); - - for (let i = 0; i < 2; i++) { - const res = await h.post< - V1PermissionsCreatePermissionRequest, - V1PermissionsCreatePermissionResponse - >({ - url: "/v1/permissions.createPermission", - headers: { - "Content-Type": "application/json", - Authorization: `Bearer ${root.key}`, - }, - body: { - name, - }, - }); - - expect(res.status, `expected 200, received: ${JSON.stringify(res, null, 2)}`).toBe(200); - } -}); diff --git a/apps/api/src/routes/v1_permissions_createPermission.ts b/apps/api/src/routes/v1_permissions_createPermission.ts index fd3738e35f..380d53ff35 100644 --- a/apps/api/src/routes/v1_permissions_createPermission.ts +++ b/apps/api/src/routes/v1_permissions_createPermission.ts @@ -87,7 +87,7 @@ export const registerV1PermissionsCreatePermission = (app: App) => .catch((e) => { if (e instanceof DatabaseError && e.body.message.includes("Duplicate entry")) { throw new UnkeyApiError({ - code: "PRECONDITION_FAILED", + code: "CONFLICT", message: "Duplicate Permission", }); } diff --git a/apps/api/src/routes/v1_permissions_createRole.error.test.ts b/apps/api/src/routes/v1_permissions_createRole.error.test.ts index 4e9448896f..441da6c91a 100644 --- a/apps/api/src/routes/v1_permissions_createRole.error.test.ts +++ b/apps/api/src/routes/v1_permissions_createRole.error.test.ts @@ -1,5 +1,6 @@ import { describe, expect, test } from "vitest"; +import { randomUUID } from "node:crypto"; import { IntegrationHarness } from "src/pkg/testutil/integration-harness"; import type { @@ -36,3 +37,36 @@ describe.each([ }); }); }); + +test("creating the same role twice errors", async (t) => { + const h = await IntegrationHarness.init(t); + const root = await h.createRootKey(["rbac.*.create_role"]); + + const name = randomUUID(); + + // The First request should succeed + // The Second request should fail + const expectedStatuses: Record = { + 0: 200, + 1: 409, + }; + + for (let i = 0; i < 2; i++) { + const res = await h.post({ + url: "/v1/permissions.createRole", + headers: { + "Content-Type": "application/json", + Authorization: `Bearer ${root.key}`, + }, + body: { + name, + }, + }); + + const expectedStatus = expectedStatuses[i]; + expect( + res.status, + `expected ${expectedStatus}, received: ${JSON.stringify(res, null, 2)}`, + ).toBe(expectedStatus); + } +}); diff --git a/apps/api/src/routes/v1_permissions_createRole.happy.test.ts b/apps/api/src/routes/v1_permissions_createRole.happy.test.ts index 199b406083..59c5d89ec1 100644 --- a/apps/api/src/routes/v1_permissions_createRole.happy.test.ts +++ b/apps/api/src/routes/v1_permissions_createRole.happy.test.ts @@ -29,25 +29,3 @@ test("creates new role", async (t) => { }); expect(found).toBeDefined(); }); - -test("creating the same role twice does not error", async (t) => { - const h = await IntegrationHarness.init(t); - const root = await h.createRootKey(["rbac.*.create_role"]); - - const name = randomUUID(); - - for (let i = 0; i < 2; i++) { - const res = await h.post({ - url: "/v1/permissions.createRole", - headers: { - "Content-Type": "application/json", - Authorization: `Bearer ${root.key}`, - }, - body: { - name, - }, - }); - - expect(res.status, `expected 200, received: ${JSON.stringify(res, null, 2)}`).toBe(200); - } -}); diff --git a/apps/api/src/routes/v1_permissions_createRole.ts b/apps/api/src/routes/v1_permissions_createRole.ts index f0a73d0cd2..084dc4d08f 100644 --- a/apps/api/src/routes/v1_permissions_createRole.ts +++ b/apps/api/src/routes/v1_permissions_createRole.ts @@ -86,7 +86,7 @@ export const registerV1PermissionsCreateRole = (app: App) => .catch((e) => { if (e instanceof DatabaseError && e.body.message.includes("Duplicate entry")) { throw new UnkeyApiError({ - code: "PRECONDITION_FAILED", + code: "CONFLICT", message: "Duplicate Role", }); } diff --git a/go/pkg/error_code/unkey_database_not_unique.go b/go/pkg/error_code/unkey_database_not_unique.go index 2d04e1e106..2879b562e0 100644 --- a/go/pkg/error_code/unkey_database_not_unique.go +++ b/go/pkg/error_code/unkey_database_not_unique.go @@ -10,8 +10,8 @@ func NewUnkeyDatabaseNotUniqueError(err error) UnkeyDatabaseNotUniqueError { err, SystemUnkey, NamespaceKey, - "NOT_UNIQUE", - "The resource identifier msut be unique.", + "CONFLICT", + "The resource identifier must be unique.", ), } From f9aa23a4484b4daa903aaf7032ae3a2d59e39065 Mon Sep 17 00:00:00 2001 From: Flo <53355483+Flo4604@users.noreply.github.com> Date: Mon, 3 Feb 2025 20:29:27 +0100 Subject: [PATCH 06/13] fix: change identity PRECONDITION_FAILED TO CONFLICT --- apps/api/src/pkg/errors/openapi_responses.ts | 4 +++- .../v1_identities_createIdentity.error.test.ts | 4 ++-- .../src/routes/v1_identities_createIdentity.ts | 4 ++-- .../v1_identities_updateIdentity.error.test.ts | 4 ++-- .../src/routes/v1_identities_updateIdentity.ts | 2 +- .../lib/trpc/routers/ratelimit/createNamespace.ts | 2 +- .../errors/code/PRECONDITION_FAILED.mdx | 15 +++++++++++++++ 7 files changed, 26 insertions(+), 9 deletions(-) create mode 100644 apps/docs/api-reference/errors/code/PRECONDITION_FAILED.mdx diff --git a/apps/api/src/pkg/errors/openapi_responses.ts b/apps/api/src/pkg/errors/openapi_responses.ts index 9ac2867e29..c361d171e8 100644 --- a/apps/api/src/pkg/errors/openapi_responses.ts +++ b/apps/api/src/pkg/errors/openapi_responses.ts @@ -51,7 +51,9 @@ export const openApiErrorResponses = { "This response is sent when the request does not meet one or more preconditions required for fulfillment.", content: { "application/json": { - schema: errorSchemaFactory(z.enum(["CONFLICT"])).openapi("ErrConflict"), + schema: errorSchemaFactory(z.enum(["PRECONDITION_FAILED"])).openapi( + "ErrPreconditionFailed", + ), }, }, }, diff --git a/apps/api/src/routes/v1_identities_createIdentity.error.test.ts b/apps/api/src/routes/v1_identities_createIdentity.error.test.ts index f5cd8defee..f363bf3578 100644 --- a/apps/api/src/routes/v1_identities_createIdentity.error.test.ts +++ b/apps/api/src/routes/v1_identities_createIdentity.error.test.ts @@ -65,8 +65,8 @@ describe("when identity exists already", () => { expect(res.status).toEqual(412); expect(res.body).toMatchObject({ error: { - code: "PRECONDITION_FAILED", - docs: "https://unkey.dev/docs/api-reference/errors/code/PRECONDITION_FAILED", + code: "CONFLICT", + docs: "https://unkey.dev/docs/api-reference/errors/code/CONFLICT", message: "Duplicate identity", }, }); diff --git a/apps/api/src/routes/v1_identities_createIdentity.ts b/apps/api/src/routes/v1_identities_createIdentity.ts index 0c405fc96a..430b9e2c44 100644 --- a/apps/api/src/routes/v1_identities_createIdentity.ts +++ b/apps/api/src/routes/v1_identities_createIdentity.ts @@ -30,7 +30,7 @@ const route = createRoute({ This usually comes from your authentication provider and could be a userId, organisationId or even an email. It does not matter what you use, as long as it uniquely identifies something in your application. -\`externalId\`s are unique across your workspace and therefore a \`PRECONDITION_FAILED\` error is returned when you try to create duplicates. +\`externalId\`s are unique across your workspace and therefore a \`CONFLICT\` error is returned when you try to create duplicates. `, example: "user_123", }), @@ -138,7 +138,7 @@ export const registerV1IdentitiesCreateIdentity = (app: App) => .catch((e) => { if (e instanceof DatabaseError && e.body.message.includes("Duplicate entry")) { throw new UnkeyApiError({ - code: "PRECONDITION_FAILED", + code: "CONFLICT", message: "Duplicate identity", }); } diff --git a/apps/api/src/routes/v1_identities_updateIdentity.error.test.ts b/apps/api/src/routes/v1_identities_updateIdentity.error.test.ts index d60fbd6fd7..bbbd36178e 100644 --- a/apps/api/src/routes/v1_identities_updateIdentity.error.test.ts +++ b/apps/api/src/routes/v1_identities_updateIdentity.error.test.ts @@ -100,8 +100,8 @@ describe("updating ratelimits", () => { expect(res.status).toEqual(412); expect(res.body).toMatchObject({ error: { - code: "PRECONDITION_FAILED", - docs: "https://unkey.dev/docs/api-reference/errors/code/PRECONDITION_FAILED", + code: "CONFLICT", + docs: "https://unkey.dev/docs/api-reference/errors/code/CONFLICT", message: "ratelimit names must be unique", }, }); diff --git a/apps/api/src/routes/v1_identities_updateIdentity.ts b/apps/api/src/routes/v1_identities_updateIdentity.ts index 7101f1b896..ab70319fd7 100644 --- a/apps/api/src/routes/v1_identities_updateIdentity.ts +++ b/apps/api/src/routes/v1_identities_updateIdentity.ts @@ -152,7 +152,7 @@ export const registerV1IdentitiesUpdateIdentity = (app: App) => for (const { name } of req.ratelimits) { if (uniqueNames.has(name)) { throw new UnkeyApiError({ - code: "PRECONDITION_FAILED", + code: "CONFLICT", message: "ratelimit names must be unique", }); } diff --git a/apps/dashboard/lib/trpc/routers/ratelimit/createNamespace.ts b/apps/dashboard/lib/trpc/routers/ratelimit/createNamespace.ts index 181adbb14e..83e77bbe28 100644 --- a/apps/dashboard/lib/trpc/routers/ratelimit/createNamespace.ts +++ b/apps/dashboard/lib/trpc/routers/ratelimit/createNamespace.ts @@ -67,7 +67,7 @@ export const createNamespace = t.procedure .catch((e) => { if (e instanceof DatabaseError && e.body.message.includes("desc = Duplicate entry")) { throw new TRPCError({ - code: "PRECONDITION_FAILED", + code: "CONFLICT", message: "duplicate namespace name. Please use a unique name for each namespace.", }); } diff --git a/apps/docs/api-reference/errors/code/PRECONDITION_FAILED.mdx b/apps/docs/api-reference/errors/code/PRECONDITION_FAILED.mdx new file mode 100644 index 0000000000..6007aad273 --- /dev/null +++ b/apps/docs/api-reference/errors/code/PRECONDITION_FAILED.mdx @@ -0,0 +1,15 @@ +--- +title: PRECONDITION_FAILED +openapi-schema: ErrPreconditionFailed + +--- + +## Problem + +The request does not meet one or more preconditions required for fulfillment. For example, that you are not flagged into the a beta feature. + +## Solution + +Please check if you meet all conditions outlined in the error message. + +If that doesn't help, ask for help on [Discord](https://unkey.com/discord) From 83072d1ec212d78b1f20eb84460878865c44b14c Mon Sep 17 00:00:00 2001 From: Flo <53355483+Flo4604@users.noreply.github.com> Date: Mon, 3 Feb 2025 20:34:23 +0100 Subject: [PATCH 07/13] fix: identity tests --- apps/api/src/routes/v1_identities_createIdentity.error.test.ts | 2 +- apps/api/src/routes/v1_identities_updateIdentity.error.test.ts | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/apps/api/src/routes/v1_identities_createIdentity.error.test.ts b/apps/api/src/routes/v1_identities_createIdentity.error.test.ts index f363bf3578..cc6b602ec3 100644 --- a/apps/api/src/routes/v1_identities_createIdentity.error.test.ts +++ b/apps/api/src/routes/v1_identities_createIdentity.error.test.ts @@ -62,7 +62,7 @@ describe("when identity exists already", () => { }, }); - expect(res.status).toEqual(412); + expect(res.status).toEqual(409); expect(res.body).toMatchObject({ error: { code: "CONFLICT", diff --git a/apps/api/src/routes/v1_identities_updateIdentity.error.test.ts b/apps/api/src/routes/v1_identities_updateIdentity.error.test.ts index bbbd36178e..2d961206c4 100644 --- a/apps/api/src/routes/v1_identities_updateIdentity.error.test.ts +++ b/apps/api/src/routes/v1_identities_updateIdentity.error.test.ts @@ -97,7 +97,7 @@ describe("updating ratelimits", () => { }, ); - expect(res.status).toEqual(412); + expect(res.status).toEqual(409); expect(res.body).toMatchObject({ error: { code: "CONFLICT", From b0ef1f0d558afc97e048a859b308f6e43ae4fd45 Mon Sep 17 00:00:00 2001 From: Flo <53355483+Flo4604@users.noreply.github.com> Date: Mon, 3 Feb 2025 20:46:46 +0100 Subject: [PATCH 08/13] fix: coderabbit comments --- apps/api/src/routes/v1_identities_createIdentity.ts | 2 ++ .../src/routes/v1_permissions_createPermission.error.test.ts | 2 +- apps/api/src/routes/v1_permissions_createPermission.ts | 2 ++ apps/api/src/routes/v1_permissions_createRole.ts | 2 ++ apps/dashboard/lib/trpc/routers/ratelimit/createNamespace.ts | 2 +- apps/docs/api-reference/errors/code/PRECONDITION_FAILED.mdx | 2 +- 6 files changed, 9 insertions(+), 3 deletions(-) diff --git a/apps/api/src/routes/v1_identities_createIdentity.ts b/apps/api/src/routes/v1_identities_createIdentity.ts index 430b9e2c44..b6a214fb23 100644 --- a/apps/api/src/routes/v1_identities_createIdentity.ts +++ b/apps/api/src/routes/v1_identities_createIdentity.ts @@ -142,6 +142,8 @@ export const registerV1IdentitiesCreateIdentity = (app: App) => message: "Duplicate identity", }); } + + throw e; }); const ratelimits = req.ratelimits diff --git a/apps/api/src/routes/v1_permissions_createPermission.error.test.ts b/apps/api/src/routes/v1_permissions_createPermission.error.test.ts index 1b02b94f05..5e51ab4886 100644 --- a/apps/api/src/routes/v1_permissions_createPermission.error.test.ts +++ b/apps/api/src/routes/v1_permissions_createPermission.error.test.ts @@ -41,7 +41,7 @@ describe.each([ }); }); -test("creating the same permission twice does not error", async (t) => { +test("creating the same permission twice returns conflict", async (t) => { const h = await IntegrationHarness.init(t); const root = await h.createRootKey(["rbac.*.create_permission"]); diff --git a/apps/api/src/routes/v1_permissions_createPermission.ts b/apps/api/src/routes/v1_permissions_createPermission.ts index 380d53ff35..cbce32eea5 100644 --- a/apps/api/src/routes/v1_permissions_createPermission.ts +++ b/apps/api/src/routes/v1_permissions_createPermission.ts @@ -91,6 +91,8 @@ export const registerV1PermissionsCreatePermission = (app: App) => message: "Duplicate Permission", }); } + + throw e; }); await insertUnkeyAuditLog(c, tx, { diff --git a/apps/api/src/routes/v1_permissions_createRole.ts b/apps/api/src/routes/v1_permissions_createRole.ts index 084dc4d08f..0228f44230 100644 --- a/apps/api/src/routes/v1_permissions_createRole.ts +++ b/apps/api/src/routes/v1_permissions_createRole.ts @@ -90,6 +90,8 @@ export const registerV1PermissionsCreateRole = (app: App) => message: "Duplicate Role", }); } + + throw e; }); await insertUnkeyAuditLog(c, tx, { diff --git a/apps/dashboard/lib/trpc/routers/ratelimit/createNamespace.ts b/apps/dashboard/lib/trpc/routers/ratelimit/createNamespace.ts index 83e77bbe28..afdfbc1f77 100644 --- a/apps/dashboard/lib/trpc/routers/ratelimit/createNamespace.ts +++ b/apps/dashboard/lib/trpc/routers/ratelimit/createNamespace.ts @@ -74,7 +74,7 @@ export const createNamespace = t.procedure throw new TRPCError({ code: "INTERNAL_SERVER_ERROR", message: - "We are unable to create namspace. Please try again or contact support@unkey.dev", + "We are unable to create namespace. Please try again or contact support@unkey.dev", }); }); diff --git a/apps/docs/api-reference/errors/code/PRECONDITION_FAILED.mdx b/apps/docs/api-reference/errors/code/PRECONDITION_FAILED.mdx index 6007aad273..70ab454f43 100644 --- a/apps/docs/api-reference/errors/code/PRECONDITION_FAILED.mdx +++ b/apps/docs/api-reference/errors/code/PRECONDITION_FAILED.mdx @@ -6,7 +6,7 @@ openapi-schema: ErrPreconditionFailed ## Problem -The request does not meet one or more preconditions required for fulfillment. For example, that you are not flagged into the a beta feature. +The request does not meet one or more preconditions required for fulfillment. For example, that you are not flagged into a beta feature. ## Solution From 4499f4d900f573d5d8c40ddd8abd18b31e70db56 Mon Sep 17 00:00:00 2001 From: Flo <53355483+Flo4604@users.noreply.github.com> Date: Mon, 3 Feb 2025 21:36:19 +0100 Subject: [PATCH 09/13] fix: adjust error messages to be more in-depth --- apps/api/src/pkg/errors/openapi_responses.ts | 2 +- apps/api/src/routes/v1_identities_createIdentity.ts | 7 +++++-- apps/api/src/routes/v1_identities_updateIdentity.ts | 2 +- apps/api/src/routes/v1_permissions_createPermission.ts | 2 +- apps/api/src/routes/v1_permissions_createRole.ts | 2 +- .../lib/trpc/routers/ratelimit/createNamespace.ts | 2 +- 6 files changed, 10 insertions(+), 7 deletions(-) diff --git a/apps/api/src/pkg/errors/openapi_responses.ts b/apps/api/src/pkg/errors/openapi_responses.ts index c361d171e8..943469e63e 100644 --- a/apps/api/src/pkg/errors/openapi_responses.ts +++ b/apps/api/src/pkg/errors/openapi_responses.ts @@ -48,7 +48,7 @@ export const openApiErrorResponses = { }, 412: { description: - "This response is sent when the request does not meet one or more preconditions required for fulfillment.", + "The requested operation cannot be completed because certain conditions were not met. This typically occurs when a required resource state or version check fails.", content: { "application/json": { schema: errorSchemaFactory(z.enum(["PRECONDITION_FAILED"])).openapi( diff --git a/apps/api/src/routes/v1_identities_createIdentity.ts b/apps/api/src/routes/v1_identities_createIdentity.ts index b6a214fb23..8aac417b7a 100644 --- a/apps/api/src/routes/v1_identities_createIdentity.ts +++ b/apps/api/src/routes/v1_identities_createIdentity.ts @@ -139,7 +139,7 @@ export const registerV1IdentitiesCreateIdentity = (app: App) => if (e instanceof DatabaseError && e.body.message.includes("Duplicate entry")) { throw new UnkeyApiError({ code: "CONFLICT", - message: "Duplicate identity", + message: `Identity with externalId "${identity.externalId}" already exists in this workspace`, }); } @@ -201,7 +201,10 @@ export const registerV1IdentitiesCreateIdentity = (app: App) => }, ], - context: { location: c.get("location"), userAgent: c.get("userAgent") }, + context: { + location: c.get("location"), + userAgent: c.get("userAgent"), + }, })), ]); }) diff --git a/apps/api/src/routes/v1_identities_updateIdentity.ts b/apps/api/src/routes/v1_identities_updateIdentity.ts index ab70319fd7..8a11a40f34 100644 --- a/apps/api/src/routes/v1_identities_updateIdentity.ts +++ b/apps/api/src/routes/v1_identities_updateIdentity.ts @@ -153,7 +153,7 @@ export const registerV1IdentitiesUpdateIdentity = (app: App) => if (uniqueNames.has(name)) { throw new UnkeyApiError({ code: "CONFLICT", - message: "ratelimit names must be unique", + message: `Ratelimit with name "${name}" is already defined in the request`, }); } uniqueNames.add(name); diff --git a/apps/api/src/routes/v1_permissions_createPermission.ts b/apps/api/src/routes/v1_permissions_createPermission.ts index cbce32eea5..e3541168f8 100644 --- a/apps/api/src/routes/v1_permissions_createPermission.ts +++ b/apps/api/src/routes/v1_permissions_createPermission.ts @@ -88,7 +88,7 @@ export const registerV1PermissionsCreatePermission = (app: App) => if (e instanceof DatabaseError && e.body.message.includes("Duplicate entry")) { throw new UnkeyApiError({ code: "CONFLICT", - message: "Duplicate Permission", + message: `Permission with name "${permission.name}" already exists in this workspace`, }); } diff --git a/apps/api/src/routes/v1_permissions_createRole.ts b/apps/api/src/routes/v1_permissions_createRole.ts index 0228f44230..48a055cabd 100644 --- a/apps/api/src/routes/v1_permissions_createRole.ts +++ b/apps/api/src/routes/v1_permissions_createRole.ts @@ -87,7 +87,7 @@ export const registerV1PermissionsCreateRole = (app: App) => if (e instanceof DatabaseError && e.body.message.includes("Duplicate entry")) { throw new UnkeyApiError({ code: "CONFLICT", - message: "Duplicate Role", + message: `Role with name "${role.name}" already exists in this workspace`, }); } diff --git a/apps/dashboard/lib/trpc/routers/ratelimit/createNamespace.ts b/apps/dashboard/lib/trpc/routers/ratelimit/createNamespace.ts index afdfbc1f77..dfb1d8fe3c 100644 --- a/apps/dashboard/lib/trpc/routers/ratelimit/createNamespace.ts +++ b/apps/dashboard/lib/trpc/routers/ratelimit/createNamespace.ts @@ -68,7 +68,7 @@ export const createNamespace = t.procedure if (e instanceof DatabaseError && e.body.message.includes("desc = Duplicate entry")) { throw new TRPCError({ code: "CONFLICT", - message: "duplicate namespace name. Please use a unique name for each namespace.", + message: `A namespace with name "${input.name}" already exists in this workspace. Please choose a different name.`, }); } throw new TRPCError({ From 98fc1d7211ec9d88e5aa9420ac8a9040105bcc1c Mon Sep 17 00:00:00 2001 From: Flo <53355483+Flo4604@users.noreply.github.com> Date: Mon, 3 Feb 2025 22:25:15 +0100 Subject: [PATCH 10/13] fix: adjust tests to not match response body anymore --- .../src/routes/v1_identities_createIdentity.error.test.ts | 7 ------- .../src/routes/v1_identities_updateIdentity.error.test.ts | 7 ------- 2 files changed, 14 deletions(-) diff --git a/apps/api/src/routes/v1_identities_createIdentity.error.test.ts b/apps/api/src/routes/v1_identities_createIdentity.error.test.ts index cc6b602ec3..b410f313f1 100644 --- a/apps/api/src/routes/v1_identities_createIdentity.error.test.ts +++ b/apps/api/src/routes/v1_identities_createIdentity.error.test.ts @@ -63,12 +63,5 @@ describe("when identity exists already", () => { }); expect(res.status).toEqual(409); - expect(res.body).toMatchObject({ - error: { - code: "CONFLICT", - docs: "https://unkey.dev/docs/api-reference/errors/code/CONFLICT", - message: "Duplicate identity", - }, - }); }); }); diff --git a/apps/api/src/routes/v1_identities_updateIdentity.error.test.ts b/apps/api/src/routes/v1_identities_updateIdentity.error.test.ts index 2d961206c4..5cabb3e9f5 100644 --- a/apps/api/src/routes/v1_identities_updateIdentity.error.test.ts +++ b/apps/api/src/routes/v1_identities_updateIdentity.error.test.ts @@ -98,12 +98,5 @@ describe("updating ratelimits", () => { ); expect(res.status).toEqual(409); - expect(res.body).toMatchObject({ - error: { - code: "CONFLICT", - docs: "https://unkey.dev/docs/api-reference/errors/code/CONFLICT", - message: "ratelimit names must be unique", - }, - }); }); }); From c758e7c83ba4fa2c61824f5c02487b21d4b15903 Mon Sep 17 00:00:00 2001 From: Flo <53355483+Flo4604@users.noreply.github.com> Date: Mon, 3 Feb 2025 23:03:45 +0100 Subject: [PATCH 11/13] fix: adjust tests to log incase of mismatches --- apps/api/src/routes/v1_identities_createIdentity.error.test.ts | 2 +- apps/api/src/routes/v1_identities_updateIdentity.error.test.ts | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/apps/api/src/routes/v1_identities_createIdentity.error.test.ts b/apps/api/src/routes/v1_identities_createIdentity.error.test.ts index b410f313f1..14b756daff 100644 --- a/apps/api/src/routes/v1_identities_createIdentity.error.test.ts +++ b/apps/api/src/routes/v1_identities_createIdentity.error.test.ts @@ -62,6 +62,6 @@ describe("when identity exists already", () => { }, }); - expect(res.status).toEqual(409); + expect(res.status, `expected 409, received: ${JSON.stringify(res, null, 2)}`).toEqual(409); }); }); diff --git a/apps/api/src/routes/v1_identities_updateIdentity.error.test.ts b/apps/api/src/routes/v1_identities_updateIdentity.error.test.ts index 5cabb3e9f5..b6fa0b24cb 100644 --- a/apps/api/src/routes/v1_identities_updateIdentity.error.test.ts +++ b/apps/api/src/routes/v1_identities_updateIdentity.error.test.ts @@ -97,6 +97,6 @@ describe("updating ratelimits", () => { }, ); - expect(res.status).toEqual(409); + expect(res.status, `expected 409, received: ${JSON.stringify(res, null, 2)}`).toEqual(409); }); }); From 114f4014634118faf7da9d6c74c12da8d2fb95c1 Mon Sep 17 00:00:00 2001 From: Flo <53355483+Flo4604@users.noreply.github.com> Date: Tue, 4 Feb 2025 17:55:53 +0100 Subject: [PATCH 12/13] fix: adjust tests to comments --- ...v1_identities_createIdentity.error.test.ts | 7 +++ ...v1_identities_updateIdentity.error.test.ts | 13 ++++- ...permissions_createPermission.error.test.ts | 53 ++++++++++--------- .../v1_permissions_createRole.error.test.ts | 51 +++++++++--------- 4 files changed, 72 insertions(+), 52 deletions(-) diff --git a/apps/api/src/routes/v1_identities_createIdentity.error.test.ts b/apps/api/src/routes/v1_identities_createIdentity.error.test.ts index 14b756daff..083aecff83 100644 --- a/apps/api/src/routes/v1_identities_createIdentity.error.test.ts +++ b/apps/api/src/routes/v1_identities_createIdentity.error.test.ts @@ -63,5 +63,12 @@ describe("when identity exists already", () => { }); expect(res.status, `expected 409, received: ${JSON.stringify(res, null, 2)}`).toEqual(409); + expect(res.body).toMatchObject({ + error: { + code: "CONFLICT", + docs: "https://unkey.dev/docs/api-reference/errors/code/CONFLICT", + message: `Identity with externalId "${externalId}" already exists in this workspace`, + }, + }); }); }); diff --git a/apps/api/src/routes/v1_identities_updateIdentity.error.test.ts b/apps/api/src/routes/v1_identities_updateIdentity.error.test.ts index b6fa0b24cb..8a367ea638 100644 --- a/apps/api/src/routes/v1_identities_updateIdentity.error.test.ts +++ b/apps/api/src/routes/v1_identities_updateIdentity.error.test.ts @@ -72,6 +72,8 @@ describe("updating ratelimits", () => { ]; await h.db.primary.insert(schema.ratelimits).values(ratelimits); + const name = "requests"; + const res = await h.post( { url: "/v1/identities.updateIdentity", @@ -83,12 +85,12 @@ describe("updating ratelimits", () => { identityId: identity.id, ratelimits: [ { - name: "a", + name, limit: 10, duration: 20000, }, { - name: "a", + name, limit: 10, duration: 124124, }, @@ -98,5 +100,12 @@ describe("updating ratelimits", () => { ); expect(res.status, `expected 409, received: ${JSON.stringify(res, null, 2)}`).toEqual(409); + expect(res.body).toMatchObject({ + error: { + code: "CONFLICT", + docs: "https://unkey.dev/docs/api-reference/errors/code/CONFLICT", + message: `Ratelimit with name "${name}" is already defined in the request`, + }, + }); }); }); diff --git a/apps/api/src/routes/v1_permissions_createPermission.error.test.ts b/apps/api/src/routes/v1_permissions_createPermission.error.test.ts index 5e51ab4886..fe10f41eeb 100644 --- a/apps/api/src/routes/v1_permissions_createPermission.error.test.ts +++ b/apps/api/src/routes/v1_permissions_createPermission.error.test.ts @@ -45,33 +45,34 @@ test("creating the same permission twice returns conflict", async (t) => { const h = await IntegrationHarness.init(t); const root = await h.createRootKey(["rbac.*.create_permission"]); - const name = randomUUID(); - // The First request should succeed - // The Second request should fail - const expectedStatuses: Record = { - 0: 200, - 1: 409, + const createPermissionRequest = { + url: "/v1/permissions.createPermission", + headers: { + "Content-Type": "application/json", + Authorization: `Bearer ${root.key}`, + }, + body: { + name: randomUUID(), + }, }; - for (let i = 0; i < 2; i++) { - const res = await h.post< - V1PermissionsCreatePermissionRequest, - V1PermissionsCreatePermissionResponse - >({ - url: "/v1/permissions.createPermission", - headers: { - "Content-Type": "application/json", - Authorization: `Bearer ${root.key}`, - }, - body: { - name, - }, - }); + const successResponse = await h.post< + V1PermissionsCreatePermissionRequest, + V1PermissionsCreatePermissionResponse + >(createPermissionRequest); + + expect( + successResponse.status, + `expected 200, received: ${JSON.stringify(successResponse, null, 2)}`, + ).toBe(200); + + const errorResponse = await h.post< + V1PermissionsCreatePermissionRequest, + V1PermissionsCreatePermissionResponse + >(createPermissionRequest); - const expectedStatus = expectedStatuses[i]; - expect( - res.status, - `expected ${expectedStatus}, received: ${JSON.stringify(res, null, 2)}`, - ).toBe(expectedStatus); - } + expect( + errorResponse.status, + `expected 409, received: ${JSON.stringify(errorResponse, null, 2)}`, + ).toBe(409); }); diff --git a/apps/api/src/routes/v1_permissions_createRole.error.test.ts b/apps/api/src/routes/v1_permissions_createRole.error.test.ts index 441da6c91a..6dde656013 100644 --- a/apps/api/src/routes/v1_permissions_createRole.error.test.ts +++ b/apps/api/src/routes/v1_permissions_createRole.error.test.ts @@ -42,31 +42,34 @@ test("creating the same role twice errors", async (t) => { const h = await IntegrationHarness.init(t); const root = await h.createRootKey(["rbac.*.create_role"]); - const name = randomUUID(); - - // The First request should succeed - // The Second request should fail - const expectedStatuses: Record = { - 0: 200, - 1: 409, + const createRoleRequest = { + url: "/v1/permissions.createRole", + headers: { + "Content-Type": "application/json", + Authorization: `Bearer ${root.key}`, + }, + body: { + name: randomUUID(), + }, }; - for (let i = 0; i < 2; i++) { - const res = await h.post({ - url: "/v1/permissions.createRole", - headers: { - "Content-Type": "application/json", - Authorization: `Bearer ${root.key}`, - }, - body: { - name, - }, - }); + const successResponse = await h.post< + V1PermissionsCreateRoleRequest, + V1PermissionsCreateRoleResponse + >(createRoleRequest); + + expect( + successResponse.status, + `expected 200, received: ${JSON.stringify(successResponse, null, 2)}`, + ).toBe(200); + + const errorResponse = await h.post< + V1PermissionsCreateRoleRequest, + V1PermissionsCreateRoleResponse + >(createRoleRequest); - const expectedStatus = expectedStatuses[i]; - expect( - res.status, - `expected ${expectedStatus}, received: ${JSON.stringify(res, null, 2)}`, - ).toBe(expectedStatus); - } + expect( + errorResponse.status, + `expected 409, received: ${JSON.stringify(errorResponse, null, 2)}`, + ).toBe(409); }); From 34472ac7543ccf655179c96e5e8c1a299fe4d136 Mon Sep 17 00:00:00 2001 From: Flo <53355483+Flo4604@users.noreply.github.com> Date: Tue, 4 Feb 2025 18:01:14 +0100 Subject: [PATCH 13/13] fix: add body check to createPermission and createRole --- .../routes/v1_permissions_createPermission.error.test.ts | 7 +++++++ .../api/src/routes/v1_permissions_createRole.error.test.ts | 7 +++++++ 2 files changed, 14 insertions(+) diff --git a/apps/api/src/routes/v1_permissions_createPermission.error.test.ts b/apps/api/src/routes/v1_permissions_createPermission.error.test.ts index fe10f41eeb..ed5ba534f5 100644 --- a/apps/api/src/routes/v1_permissions_createPermission.error.test.ts +++ b/apps/api/src/routes/v1_permissions_createPermission.error.test.ts @@ -75,4 +75,11 @@ test("creating the same permission twice returns conflict", async (t) => { errorResponse.status, `expected 409, received: ${JSON.stringify(errorResponse, null, 2)}`, ).toBe(409); + expect(errorResponse.body).toMatchObject({ + error: { + code: "CONFLICT", + docs: "https://unkey.dev/docs/api-reference/errors/code/CONFLICT", + message: `Permission with name "${createPermissionRequest.body.name}" already exists in this workspace`, + }, + }); }); diff --git a/apps/api/src/routes/v1_permissions_createRole.error.test.ts b/apps/api/src/routes/v1_permissions_createRole.error.test.ts index 6dde656013..52eb2b3d37 100644 --- a/apps/api/src/routes/v1_permissions_createRole.error.test.ts +++ b/apps/api/src/routes/v1_permissions_createRole.error.test.ts @@ -72,4 +72,11 @@ test("creating the same role twice errors", async (t) => { errorResponse.status, `expected 409, received: ${JSON.stringify(errorResponse, null, 2)}`, ).toBe(409); + expect(errorResponse.body).toMatchObject({ + error: { + code: "CONFLICT", + docs: "https://unkey.dev/docs/api-reference/errors/code/CONFLICT", + message: `Role with name "${createRoleRequest.body.name}" already exists in this workspace`, + }, + }); });