fix(core): fix create org role API

Author: darcyYe

.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.

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."
           }
         }
       }

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<T extends ManagementApiRouter>(
         resourceScopeIds: z.array(z.string()).default([]),
       });
 
+  /** Helper function to handle scope insertion with error handling */
+  const insertScopesWithErrorHandling = async (
+    roleId: string,
+    insertFunction: () => Promise<unknown>,
+    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<T extends ManagementApiRouter>(
       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;

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 */

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);