From bdebf2ab3e72410cbf4c86b25b9ed7fc0ab1ca98 Mon Sep 17 00:00:00 2001 From: Darcy Ye Date: Thu, 19 Dec 2024 14:55:17 +0800 Subject: [PATCH] fix(core): fix create org role API --- .changeset/thick-baboons-sniff.md | 7 ++ .../organization-role/index.openapi.json | 2 +- .../src/routes/organization-role/index.ts | 63 ++++++++++---- .../organization/organization-role.test.ts | 85 +++++++++++++++++++ .../src/locales/en/errors/organization.ts | 4 + 5 files changed, 143 insertions(+), 18 deletions(-) create mode 100644 .changeset/thick-baboons-sniff.md diff --git a/.changeset/thick-baboons-sniff.md b/.changeset/thick-baboons-sniff.md new file mode 100644 index 00000000000..8e908c791f5 --- /dev/null +++ b/.changeset/thick-baboons-sniff.md @@ -0,0 +1,7 @@ +--- +"@logto/core": patch +--- + +fix POST /api/organization-roles API + +When invalid organization scope IDs or resource scope IDs are provided, the API should return a 422 error without creating the organization role. diff --git a/packages/core/src/routes/organization-role/index.openapi.json b/packages/core/src/routes/organization-role/index.openapi.json index fbdcbcf4dff..afb06ab8d22 100644 --- a/packages/core/src/routes/organization-role/index.openapi.json +++ b/packages/core/src/routes/organization-role/index.openapi.json @@ -46,7 +46,7 @@ "description": "The organization role was created successfully." }, "422": { - "description": "The organization role name is already in use." + "description": "The organization role name is already in use, or at least one of the IDs (organizationScopeIds or resourceScopeIds) provided is invalid. For example, the organization scope ID or resource scope ID does not exist." } } } diff --git a/packages/core/src/routes/organization-role/index.ts b/packages/core/src/routes/organization-role/index.ts index ff700c676dc..ed5fa45b41a 100644 --- a/packages/core/src/routes/organization-role/index.ts +++ b/packages/core/src/routes/organization-role/index.ts @@ -6,9 +6,10 @@ import { type OrganizationRoleKeys, } from '@logto/schemas'; import { generateStandardId } from '@logto/shared'; -import { condArray } from '@silverhand/essentials'; +import { condArray, tryThat } from '@silverhand/essentials'; import { z } from 'zod'; +import RequestError from '#src/errors/RequestError/index.js'; import { buildManagementApiContext } from '#src/libraries/hook/utils.js'; import koaGuard from '#src/middleware/koa-guard.js'; import koaPagination from '#src/middleware/koa-pagination.js'; @@ -93,6 +94,28 @@ export default function organizationRoleRoutes( resourceScopeIds: z.array(z.string()).default([]), }); + /** Helper function to handle scope insertion with error handling */ + const insertScopesWithErrorHandling = async ( + roleId: string, + insertFunction: () => Promise, + errorCode: + | 'organization.roles.invalid_scope_ids' + | 'organization.roles.invalid_resource_scope_ids' + ) => { + await tryThat( + async () => insertFunction(), + (error) => { + void roles.deleteById(roleId); + + throw new RequestError({ + code: errorCode, + status: 422, + details: error instanceof Error ? error.message : String(error), + }); + } + ); + }; + router.post( '/', koaGuard({ @@ -104,23 +127,29 @@ export default function organizationRoleRoutes( const { organizationScopeIds, resourceScopeIds, ...data } = ctx.guard.body; const role = await roles.insert({ id: generateStandardId(), ...data }); - if (organizationScopeIds.length > 0) { - await rolesScopes.insert( - ...organizationScopeIds.map((id) => ({ - organizationRoleId: role.id, - organizationScopeId: id, - })) - ); - } + await insertScopesWithErrorHandling( + role.id, + async () => + rolesScopes.insert( + ...organizationScopeIds.map((id) => ({ + organizationRoleId: role.id, + organizationScopeId: id, + })) + ), + 'organization.roles.invalid_scope_ids' + ); - if (resourceScopeIds.length > 0) { - await rolesResourceScopes.insert( - ...resourceScopeIds.map((id) => ({ - organizationRoleId: role.id, - scopeId: id, - })) - ); - } + await insertScopesWithErrorHandling( + role.id, + async () => + rolesResourceScopes.insert( + ...resourceScopeIds.map((id) => ({ + organizationRoleId: role.id, + scopeId: id, + })) + ), + 'organization.roles.invalid_resource_scope_ids' + ); ctx.body = role; ctx.status = 201; diff --git a/packages/integration-tests/src/tests/api/organization/organization-role.test.ts b/packages/integration-tests/src/tests/api/organization/organization-role.test.ts index 285af393c4a..5f0be86f804 100644 --- a/packages/integration-tests/src/tests/api/organization/organization-role.test.ts +++ b/packages/integration-tests/src/tests/api/organization/organization-role.test.ts @@ -1,11 +1,16 @@ +/* eslint-disable max-lines */ +// TODO: @darcy reorg to break this file into smaller files import assert from 'node:assert'; import { generateStandardId } from '@logto/shared'; import { isKeyInObject, pick } from '@silverhand/essentials'; import { HTTPError } from 'ky'; +import { createResource } from '#src/api/index.js'; +import { createScope } from '#src/api/scope.js'; import { OrganizationRoleApiTest, OrganizationScopeApiTest } from '#src/helpers/organization.js'; import { ScopeApiTest } from '#src/helpers/resource.js'; +import { generateScopeName } from '#src/utils.js'; const randomId = () => generateStandardId(4); @@ -179,6 +184,85 @@ describe('organization role APIs', () => { const response = await roleApi.delete('0').catch((error: unknown) => error); expect(response instanceof HTTPError && response.response.status).toBe(404); }); + + it('should fail when creating a role with invalid organization scope IDs', async () => { + const invalidScopeId = generateStandardId(); + const response = await roleApi + .create({ + name: 'test' + randomId(), + organizationScopeIds: [invalidScopeId], + }) + .catch((error: unknown) => error); + + assert(response instanceof HTTPError); + const body: unknown = await response.response.json(); + expect(response.response.status).toBe(422); + expect(body).toMatchObject( + expect.objectContaining({ + code: 'organization.roles.invalid_scope_ids', + }) + ); + + const roles = await roleApi.getList(); + expect(roles).toHaveLength(0); + }); + + it('should fail when creating a role with invalid resource scope IDs', async () => { + const invalidScopeId = generateStandardId(); + const response = await roleApi + .create({ + name: 'test' + randomId(), + resourceScopeIds: [invalidScopeId], + }) + .catch((error: unknown) => error); + + assert(response instanceof HTTPError); + const body: unknown = await response.response.json(); + expect(response.response.status).toBe(422); + expect(body).toMatchObject( + expect.objectContaining({ + code: 'organization.roles.invalid_resource_scope_ids', + }) + ); + + const roles = await roleApi.getList(); + expect(roles).toHaveLength(0); + }); + + it('should successfully create a role with scope IDs are provided', async () => { + const resource = await createResource(); + const scopeName = generateScopeName(); + const createdScope = await createScope(resource.id, scopeName); + + const [scope1, scope2] = await Promise.all([ + scopeApi.create({ name: 'test' + randomId() }), + scopeApi.create({ name: 'test' + randomId() }), + ]); + const createdRole = await roleApi.create({ + name: 'test' + randomId(), + organizationScopeIds: [scope1.id, scope2.id], + resourceScopeIds: [createdScope.id], + }); + + expect(createdRole).toHaveProperty('id'); + expect(createdRole).toHaveProperty('name'); + + const scopes = await roleApi.getScopes(createdRole.id); + expect(scopes).toContainEqual( + expect.objectContaining({ + name: scope1.name, + }) + ); + expect(scopes).toContainEqual( + expect.objectContaining({ + name: scope2.name, + }) + ); + + const { resourceScopes } = await roleApi.get(createdRole.id); + expect(resourceScopes.length).toBe(1); + expect(resourceScopes[0]).toHaveProperty('name', scopeName); + }); }); describe('organization role - scope relations', () => { @@ -397,3 +481,4 @@ describe('organization role APIs', () => { }); }); }); +/* eslint-enable max-lines */ diff --git a/packages/phrases/src/locales/en/errors/organization.ts b/packages/phrases/src/locales/en/errors/organization.ts index 5551cc9b43c..310da6b2211 100644 --- a/packages/phrases/src/locales/en/errors/organization.ts +++ b/packages/phrases/src/locales/en/errors/organization.ts @@ -1,5 +1,9 @@ const organizations = { require_membership: 'The user must be a member of the organization to proceed.', + roles: { + invalid_scope_ids: 'The organization scope IDs are not valid.', + invalid_resource_scope_ids: 'The resource scope IDs are not valid.', + }, }; export default Object.freeze(organizations);