Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 18 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions apps/api/src/pkg/errors/http.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ const ErrorCode = z.enum([
"USAGE_EXCEEDED",
"DISABLED",
"NOT_FOUND",
"NOT_UNIQUE",
"CONFLICT",
Flo4604 marked this conversation as resolved.
Show resolved Hide resolved
"RATE_LIMITED",
"UNAUTHORIZED",
"PRECONDITION_FAILED",
Expand Down Expand Up @@ -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":
Expand Down
11 changes: 11 additions & 0 deletions apps/api/src/pkg/errors/openapi_responses.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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: {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Flo4604 marked this conversation as resolved.
Show resolved Hide resolved
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`,
},
});
});
Expand Down
13 changes: 9 additions & 4 deletions apps/api/src/routes/v1_identities_createIdentity.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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",
}),
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -199,7 +201,10 @@ export const registerV1IdentitiesCreateIdentity = (app: App) =>
},
],

context: { location: c.get("location"), userAgent: c.get("userAgent") },
context: {
ogzhanolguncu marked this conversation as resolved.
Show resolved Hide resolved
location: c.get("location"),
userAgent: c.get("userAgent"),
},
})),
]);
})
Expand Down
14 changes: 8 additions & 6 deletions apps/api/src/routes/v1_identities_updateIdentity.error.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand All @@ -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,
},
Expand All @@ -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({
Flo4604 marked this conversation as resolved.
Show resolved Hide resolved
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`,
},
});
});
Expand Down
4 changes: 2 additions & 2 deletions apps/api/src/routes/v1_identities_updateIdentity.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
174 changes: 0 additions & 174 deletions apps/api/src/routes/v1_keys_ami.ts

This file was deleted.

2 changes: 1 addition & 1 deletion apps/api/src/routes/v1_keys_deleteKey.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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.",
}),
}),
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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({
Expand Down
2 changes: 1 addition & 1 deletion apps/api/src/routes/v1_migrations_createKey.ts
Original file line number Diff line number Diff line change
Expand Up @@ -559,7 +559,7 @@ export const registerV1MigrationsCreateKeys = (app: App) =>
workspaceId: authorizedWorkspaceId,
});
throw new UnkeyApiError({
code: "NOT_UNIQUE",
code: "CONFLICT",
message: e.body.message,
});
}
Expand Down
Original file line number Diff line number Diff line change
@@ -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 {
Expand Down Expand Up @@ -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`,
},
});
});
Loading