fix: throw conflict error for duplicate roles/permissions (#2845)

* try to load role/perm before creating to check for duplicates * fix formatting * remove duplicate file and fix log description * fix: catch duplicate key errors * fix: rename NOT_UNIQUE to CONFLICT and change tests to reflect * fix: change identity PRECONDITION_FAILED TO CONFLICT * fix: identity tests * fix: coderabbit comments * fix: adjust error messages to be more in-depth * fix: adjust tests to not match response body anymore * fix: adjust tests to log incase of mismatches * fix: adjust tests to comments * fix: add body check to createPermission and createRole --------- Co-authored-by: Oğuzhan Olguncu <[email protected]>
unkeyed · Feb 10, 2025 · 0096509 · 0096509
1 parent 9ab5c73
commit 0096509
Show file tree

Hide file tree

Showing 19 changed files with 172 additions and 260 deletions.
diff --git 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<typeof ErrorCode>): 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
@@ -46,6 +46,17 @@ export const openApiErrorResponses = {
       },
     },
   },
+  412: {
+    description:
+      "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(
+          "ErrPreconditionFailed",
+        ),
+      },
+    },
+  },
   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_identities_createIdentity.error.test.ts b/apps/api/src/routes/v1_identities_createIdentity.error.test.ts
@@ -62,12 +62,12 @@ describe("when identity exists already", () => {
       },
     });
 
-    expect(res.status).toEqual(412);
+    expect(res.status, `expected 409, received: ${JSON.stringify(res, null, 2)}`).toEqual(409);
     expect(res.body).toMatchObject({
       error: {
-        code: "PRECONDITION_FAILED",
-        docs: "https://unkey.dev/docs/api-reference/errors/code/PRECONDITION_FAILED",
-        message: "Duplicate identity",
+        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_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,10 +138,12 @@ export const registerV1IdentitiesCreateIdentity = (app: App) =>
           .catch((e) => {
             if (e instanceof DatabaseError && e.body.message.includes("Duplicate entry")) {
               throw new UnkeyApiError({
-                code: "PRECONDITION_FAILED",
-                message: "Duplicate identity",
+                code: "CONFLICT",
+                message: `Identity with externalId "${identity.externalId}" already exists in this workspace`,
               });
             }
+
+            throw e;
           });
 
         const ratelimits = req.ratelimits
@@ -199,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.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<V1IdentitiesUpdateIdentityRequest, V1IdentitiesUpdateIdentityResponse>(
       {
         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,
             },
@@ -97,12 +99,12 @@ describe("updating ratelimits", () => {
       },
     );
 
-    expect(res.status).toEqual(412);
+    expect(res.status, `expected 409, received: ${JSON.stringify(res, null, 2)}`).toEqual(409);
     expect(res.body).toMatchObject({
       error: {
-        code: "PRECONDITION_FAILED",
-        docs: "https://unkey.dev/docs/api-reference/errors/code/PRECONDITION_FAILED",
-        message: "ratelimit names must be unique",
+        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_identities_updateIdentity.ts b/apps/api/src/routes/v1_identities_updateIdentity.ts
@@ -152,8 +152,8 @@ export const registerV1IdentitiesUpdateIdentity = (app: App) =>
       for (const { name } of req.ratelimits) {
         if (uniqueNames.has(name)) {
           throw new UnkeyApiError({
-            code: "PRECONDITION_FAILED",
-            message: "ratelimit names must be unique",
+            code: "CONFLICT",
+            message: `Ratelimit with name "${name}" is already defined in the request`,
           });
         }
         uniqueNames.add(name);

diff --git a/apps/api/src/routes/v1_keys_ami.ts b/apps/api/src/routes/v1_keys_ami.ts
diff --git 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
@@ -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
@@ -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
@@ -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,46 @@ describe.each([
     });
   });
 });
+
+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 createPermissionRequest = {
+    url: "/v1/permissions.createPermission",
+    headers: {
+      "Content-Type": "application/json",
+      Authorization: `Bearer ${root.key}`,
+    },
+    body: {
+      name: randomUUID(),
+    },
+  };
+
+  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);
+
+  expect(
+    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`,
+    },
+  });
+});