From 5b03030de23898885daa77086c8b263e9e13c925 Mon Sep 17 00:00:00 2001 From: wangsijie Date: Thu, 11 Apr 2024 17:20:53 +0800 Subject: [PATCH] fix(core): not allow to modify management api resource (#5626) --- .changeset/four-goats-rush.md | 9 ++++ .../core/src/routes/resource.scope.test.ts | 44 ++++++++++++++++++- packages/core/src/routes/resource.scope.ts | 34 +++++++------- packages/core/src/routes/resource.test.ts | 9 ++++ packages/core/src/routes/resource.ts | 8 +++- .../src/tests/api/resource.scope.test.ts | 37 ++++++++++++++++ .../src/tests/api/resource.test.ts | 12 +++++ .../phrases/src/locales/de/errors/resource.ts | 2 + .../phrases/src/locales/en/errors/resource.ts | 1 + .../phrases/src/locales/es/errors/resource.ts | 2 + .../phrases/src/locales/fr/errors/resource.ts | 2 + .../phrases/src/locales/it/errors/resource.ts | 2 + .../phrases/src/locales/ja/errors/resource.ts | 2 + .../phrases/src/locales/ko/errors/resource.ts | 2 + .../src/locales/pl-pl/errors/resource.ts | 2 + .../src/locales/pt-br/errors/resource.ts | 2 + .../src/locales/pt-pt/errors/resource.ts | 2 + .../phrases/src/locales/ru/errors/resource.ts | 2 + .../src/locales/tr-tr/errors/resource.ts | 2 + .../src/locales/zh-cn/errors/resource.ts | 2 + .../src/locales/zh-hk/errors/resource.ts | 2 + .../src/locales/zh-tw/errors/resource.ts | 2 + 22 files changed, 161 insertions(+), 21 deletions(-) create mode 100644 .changeset/four-goats-rush.md diff --git a/.changeset/four-goats-rush.md b/.changeset/four-goats-rush.md new file mode 100644 index 000000000..b26fe0190 --- /dev/null +++ b/.changeset/four-goats-rush.md @@ -0,0 +1,9 @@ +--- +"@logto/integration-tests": patch +"@logto/phrases": patch +"@logto/core": patch +--- + +Not allow to modify management API resource through API. + +Previously, management API resource and its scopes are readonly in Console. But it was possible to modify through the API. This is not allowed anymore. diff --git a/packages/core/src/routes/resource.scope.test.ts b/packages/core/src/routes/resource.scope.test.ts index e34e04142..6d6575d1a 100644 --- a/packages/core/src/routes/resource.scope.test.ts +++ b/packages/core/src/routes/resource.scope.test.ts @@ -1,4 +1,8 @@ -import { type Resource, type CreateResource } from '@logto/schemas'; +import { + type Resource, + type CreateResource, + getManagementApiResourceIndicator, +} from '@logto/schemas'; import { pickDefault } from '@logto/shared/esm'; import { type Nullable } from '@silverhand/essentials'; @@ -91,6 +95,19 @@ describe('resource scope routes', () => { expect(response.status).toEqual(400); }); + it('POST /resources/:id/scopes should throw when the resource is management API', async () => { + const { findResourceById } = resources; + findResourceById.mockResolvedValueOnce({ + ...mockResource, + indicator: getManagementApiResourceIndicator('mock'), + }); + await expect( + resourceScopeRequest + .post('/resources/foo/scopes') + .send({ name: 'name', description: 'description' }) + ).resolves.toHaveProperty('status', 400); + }); + it('PATCH /resources/:id/scopes/:scopeId', async () => { const name = 'write:users'; const description = 'description'; @@ -107,10 +124,35 @@ describe('resource scope routes', () => { }); }); + it('PATCH /resources/:id/scopes/:scopeId should throw when the resource is management API', async () => { + const { findResourceById } = resources; + findResourceById.mockResolvedValueOnce({ + ...mockResource, + indicator: getManagementApiResourceIndicator('mock'), + }); + await expect( + resourceScopeRequest + .patch('/resources/foo/scopes/foz') + .send({ name: 'name', description: 'description' }) + ).resolves.toHaveProperty('status', 400); + }); + it('DELETE /resources/:id/scopes/:scopeId', async () => { await expect(resourceScopeRequest.delete('/resources/foo/scopes/foz')).resolves.toHaveProperty( 'status', 204 ); }); + + it('DELETE /resources/:id/scopes/:scopeId should throw when the resource is management API', async () => { + const { findResourceById } = resources; + findResourceById.mockResolvedValueOnce({ + ...mockResource, + indicator: getManagementApiResourceIndicator('mock'), + }); + await expect(resourceScopeRequest.delete('/resources/foo/scopes/foz')).resolves.toHaveProperty( + 'status', + 400 + ); + }); }); diff --git a/packages/core/src/routes/resource.scope.ts b/packages/core/src/routes/resource.scope.ts index e5e001324..3628c6a7e 100644 --- a/packages/core/src/routes/resource.scope.ts +++ b/packages/core/src/routes/resource.scope.ts @@ -1,4 +1,4 @@ -import { Scopes } from '@logto/schemas'; +import { Scopes, isManagementApi } from '@logto/schemas'; import { generateStandardId } from '@logto/shared'; import { tryThat } from '@silverhand/essentials'; import { object, string } from 'zod'; @@ -93,15 +93,10 @@ export default function resourceScopeRoutes( assertThat(!/\s/.test(body.name), 'scope.name_with_space'); + const { indicator } = await findResourceById(resourceId); assertThat( - await findResourceById(resourceId), - new RequestError({ - code: 'entity.not_exists_with_id', - name: 'resource', - id: resourceId, - resourceId, - status: 404, - }) + !isManagementApi(indicator), + new RequestError({ code: 'resource.cannot_modify_management_api' }) ); assertThat( @@ -138,15 +133,10 @@ export default function resourceScopeRoutes( body, } = ctx.guard; + const { indicator } = await findResourceById(resourceId); assertThat( - await findResourceById(resourceId), - new RequestError({ - code: 'entity.not_exists_with_id', - name: 'resource', - id: resourceId, - resourceId, - status: 404, - }) + !isManagementApi(indicator), + new RequestError({ code: 'resource.cannot_modify_management_api' }) ); if (body.name) { @@ -171,13 +161,19 @@ export default function resourceScopeRoutes( '/resources/:resourceId/scopes/:scopeId', koaGuard({ params: object({ resourceId: string().min(1), scopeId: string().min(1) }), - status: [204, 404], + status: [204, 400, 404], }), async (ctx, next) => { const { - params: { scopeId }, + params: { scopeId, resourceId }, } = ctx.guard; + const { indicator } = await findResourceById(resourceId); + assertThat( + !isManagementApi(indicator), + new RequestError({ code: 'resource.cannot_modify_management_api' }) + ); + await deleteScopeById(scopeId); ctx.status = 204; diff --git a/packages/core/src/routes/resource.test.ts b/packages/core/src/routes/resource.test.ts index 88f766174..967ecb6b8 100644 --- a/packages/core/src/routes/resource.test.ts +++ b/packages/core/src/routes/resource.test.ts @@ -152,6 +152,15 @@ describe('resource routes', () => { expect(response.status).toEqual(400); }); + it('PATCH /resources/:id should throw when trying to modify management API', async () => { + const { findResourceById } = resources; + findResourceById.mockResolvedValueOnce({ + ...mockResource, + indicator: getManagementApiResourceIndicator('mock'), + }); + await expect(resourceRequest.patch('/resources/foo')).resolves.toHaveProperty('status', 400); + }); + it('DELETE /resources/:id', async () => { await expect(resourceRequest.delete('/resources/foo')).resolves.toHaveProperty('status', 204); }); diff --git a/packages/core/src/routes/resource.ts b/packages/core/src/routes/resource.ts index b528321d2..7e4947cbf 100644 --- a/packages/core/src/routes/resource.ts +++ b/packages/core/src/routes/resource.ts @@ -136,7 +136,7 @@ export default function resourceRoutes( // Use the dedicated API `PATCH /resources/:id/is-default` to update. body: Resources.createGuard.omit({ id: true, indicator: true, isDefault: true }).partial(), response: Resources.guard, - status: [200, 404], + status: [200, 400, 404], }), async (ctx, next) => { const { @@ -144,6 +144,12 @@ export default function resourceRoutes( body, } = ctx.guard; + const { indicator } = await findResourceById(id); + assertThat( + !isManagementApi(indicator), + new RequestError({ code: 'resource.cannot_modify_management_api' }) + ); + const resource = await updateResourceById(id, body); ctx.body = resource; diff --git a/packages/integration-tests/src/tests/api/resource.scope.test.ts b/packages/integration-tests/src/tests/api/resource.scope.test.ts index 96eaeb747..1ae7e4db0 100644 --- a/packages/integration-tests/src/tests/api/resource.scope.test.ts +++ b/packages/integration-tests/src/tests/api/resource.scope.test.ts @@ -5,6 +5,7 @@ import { HTTPError } from 'ky'; import { createResource } from '#src/api/index.js'; import { createScope, deleteScope, getScopes, updateScope } from '#src/api/scope.js'; +import { expectRejects } from '#src/helpers/index.js'; import { generateName, generateScopeName } from '#src/utils.js'; describe('scopes', () => { @@ -56,6 +57,13 @@ describe('scopes', () => { expect(response instanceof HTTPError && response.response.status === 400).toBe(true); }); + it('should throw when creating scope under management API resource', async () => { + await expectRejects(createScope(defaultManagementApi.resource.id, 'scope'), { + code: 'resource.cannot_modify_management_api', + status: 400, + }); + }); + it('should update scope successfully', async () => { const resource = await createResource(); const scope = await createScope(resource.id); @@ -114,6 +122,23 @@ describe('scopes', () => { expect(response instanceof HTTPError && response.response.status === 404).toBe(true); }); + it('should throw when updating scope under management API resource', async () => { + const scopes = await getScopes(defaultManagementApi.resource.id); + const scopeId = scopes[0]?.id; + if (!scopeId) { + throw new Error('No scope for management API resource found'); + } + await expectRejects( + updateScope(defaultManagementApi.resource.id, scopeId, { + name: 'scope', + }), + { + code: 'resource.cannot_modify_management_api', + status: 400, + } + ); + }); + it('should delete scope successfully', async () => { const resource = await createResource(); const scope = await createScope(resource.id); @@ -126,4 +151,16 @@ describe('scopes', () => { expect(scopes.some(({ name }) => name === scope.name)).toBeFalsy(); }); + + it('should throw when deleting scope under management API resource', async () => { + const scopes = await getScopes(defaultManagementApi.resource.id); + const scopeId = scopes[0]?.id; + if (!scopeId) { + throw new Error('No scope for management API resource found'); + } + await expectRejects(deleteScope(defaultManagementApi.resource.id, scopeId), { + code: 'resource.cannot_modify_management_api', + status: 400, + }); + }); }); diff --git a/packages/integration-tests/src/tests/api/resource.test.ts b/packages/integration-tests/src/tests/api/resource.test.ts index f4f121965..87503a8a6 100644 --- a/packages/integration-tests/src/tests/api/resource.test.ts +++ b/packages/integration-tests/src/tests/api/resource.test.ts @@ -96,6 +96,18 @@ describe('admin console api resources', () => { expect(response instanceof HTTPError && response.response.status === 400).toBe(true); }); + it('should throw when updating management api resource', async () => { + await expectRejects( + updateResource(defaultManagementApi.resource.id, { + name: 'new-name', + }), + { + code: 'resource.cannot_modify_management_api', + status: 400, + } + ); + }); + it('should not update api resource indicator', async () => { const resource = await createResource(); const newResourceName = `new_${resource.name}`; diff --git a/packages/phrases/src/locales/de/errors/resource.ts b/packages/phrases/src/locales/de/errors/resource.ts index 4172a193b..969ec8cac 100644 --- a/packages/phrases/src/locales/de/errors/resource.ts +++ b/packages/phrases/src/locales/de/errors/resource.ts @@ -2,6 +2,8 @@ const resource = { resource_identifier_in_use: 'Die API-Kennung {{indicator}} wird bereits verwendet', /** UNTRANSLATED */ cannot_delete_management_api: 'Cannot delete Logto management API.', + /** UNTRANSLATED */ + cannot_modify_management_api: 'Cannot modify Logto management API.', }; export default Object.freeze(resource); diff --git a/packages/phrases/src/locales/en/errors/resource.ts b/packages/phrases/src/locales/en/errors/resource.ts index 4a47fbf34..8c6cabaa2 100644 --- a/packages/phrases/src/locales/en/errors/resource.ts +++ b/packages/phrases/src/locales/en/errors/resource.ts @@ -1,6 +1,7 @@ const resource = { resource_identifier_in_use: 'The API identifier {{indicator}} is already in use', cannot_delete_management_api: 'Cannot delete Logto management API.', + cannot_modify_management_api: 'Cannot modify Logto management API.', }; export default Object.freeze(resource); diff --git a/packages/phrases/src/locales/es/errors/resource.ts b/packages/phrases/src/locales/es/errors/resource.ts index 780df333b..0f3dc89d6 100644 --- a/packages/phrases/src/locales/es/errors/resource.ts +++ b/packages/phrases/src/locales/es/errors/resource.ts @@ -2,6 +2,8 @@ const resource = { resource_identifier_in_use: '"El identificador de API {{indicator}} ya está en uso', /** UNTRANSLATED */ cannot_delete_management_api: 'Cannot delete Logto management API.', + /** UNTRANSLATED */ + cannot_modify_management_api: 'Cannot modify Logto management API.', }; export default Object.freeze(resource); diff --git a/packages/phrases/src/locales/fr/errors/resource.ts b/packages/phrases/src/locales/fr/errors/resource.ts index fe4c8aeda..54e3955cd 100644 --- a/packages/phrases/src/locales/fr/errors/resource.ts +++ b/packages/phrases/src/locales/fr/errors/resource.ts @@ -2,6 +2,8 @@ const resource = { resource_identifier_in_use: "L'identifiant d'API {{indicator}} est déjà utilisé", /** UNTRANSLATED */ cannot_delete_management_api: 'Cannot delete Logto management API.', + /** UNTRANSLATED */ + cannot_modify_management_api: 'Cannot modify Logto management API.', }; export default Object.freeze(resource); diff --git a/packages/phrases/src/locales/it/errors/resource.ts b/packages/phrases/src/locales/it/errors/resource.ts index 997fdba34..b6332a463 100644 --- a/packages/phrases/src/locales/it/errors/resource.ts +++ b/packages/phrases/src/locales/it/errors/resource.ts @@ -2,6 +2,8 @@ const resource = { resource_identifier_in_use: "L'identificatore API {{indicator}} è già in uso", /** UNTRANSLATED */ cannot_delete_management_api: 'Cannot delete Logto management API.', + /** UNTRANSLATED */ + cannot_modify_management_api: 'Cannot modify Logto management API.', }; export default Object.freeze(resource); diff --git a/packages/phrases/src/locales/ja/errors/resource.ts b/packages/phrases/src/locales/ja/errors/resource.ts index 17aad104a..2b4246976 100644 --- a/packages/phrases/src/locales/ja/errors/resource.ts +++ b/packages/phrases/src/locales/ja/errors/resource.ts @@ -2,6 +2,8 @@ const resource = { resource_identifier_in_use: 'API識別子 {{indicator}} はすでに使用されています', /** UNTRANSLATED */ cannot_delete_management_api: 'Cannot delete Logto management API.', + /** UNTRANSLATED */ + cannot_modify_management_api: 'Cannot modify Logto management API.', }; export default Object.freeze(resource); diff --git a/packages/phrases/src/locales/ko/errors/resource.ts b/packages/phrases/src/locales/ko/errors/resource.ts index 92ad2a501..405b298c3 100644 --- a/packages/phrases/src/locales/ko/errors/resource.ts +++ b/packages/phrases/src/locales/ko/errors/resource.ts @@ -2,6 +2,8 @@ const resource = { resource_identifier_in_use: 'API 식별자 {{indicator}}가 이미 사용 중입니다', /** UNTRANSLATED */ cannot_delete_management_api: 'Cannot delete Logto management API.', + /** UNTRANSLATED */ + cannot_modify_management_api: 'Cannot modify Logto management API.', }; export default Object.freeze(resource); diff --git a/packages/phrases/src/locales/pl-pl/errors/resource.ts b/packages/phrases/src/locales/pl-pl/errors/resource.ts index 1a26d6f79..1982afca7 100644 --- a/packages/phrases/src/locales/pl-pl/errors/resource.ts +++ b/packages/phrases/src/locales/pl-pl/errors/resource.ts @@ -2,6 +2,8 @@ const resource = { resource_identifier_in_use: 'Identyfikator API {{indicator}} jest już używany', /** UNTRANSLATED */ cannot_delete_management_api: 'Cannot delete Logto management API.', + /** UNTRANSLATED */ + cannot_modify_management_api: 'Cannot modify Logto management API.', }; export default Object.freeze(resource); diff --git a/packages/phrases/src/locales/pt-br/errors/resource.ts b/packages/phrases/src/locales/pt-br/errors/resource.ts index 930c2f326..959ec7de2 100644 --- a/packages/phrases/src/locales/pt-br/errors/resource.ts +++ b/packages/phrases/src/locales/pt-br/errors/resource.ts @@ -2,6 +2,8 @@ const resource = { resource_identifier_in_use: 'O identificador de API {{indicator}} já está em uso', /** UNTRANSLATED */ cannot_delete_management_api: 'Cannot delete Logto management API.', + /** UNTRANSLATED */ + cannot_modify_management_api: 'Cannot modify Logto management API.', }; export default Object.freeze(resource); diff --git a/packages/phrases/src/locales/pt-pt/errors/resource.ts b/packages/phrases/src/locales/pt-pt/errors/resource.ts index 69884fc53..1f01ab540 100644 --- a/packages/phrases/src/locales/pt-pt/errors/resource.ts +++ b/packages/phrases/src/locales/pt-pt/errors/resource.ts @@ -2,6 +2,8 @@ const resource = { resource_identifier_in_use: 'O identificador da API {{indicator}} já está em uso', /** UNTRANSLATED */ cannot_delete_management_api: 'Cannot delete Logto management API.', + /** UNTRANSLATED */ + cannot_modify_management_api: 'Cannot modify Logto management API.', }; export default Object.freeze(resource); diff --git a/packages/phrases/src/locales/ru/errors/resource.ts b/packages/phrases/src/locales/ru/errors/resource.ts index 8dd8c80c1..6cc4c6031 100644 --- a/packages/phrases/src/locales/ru/errors/resource.ts +++ b/packages/phrases/src/locales/ru/errors/resource.ts @@ -2,6 +2,8 @@ const resource = { resource_identifier_in_use: 'Идентификатор API {{indicator}} уже используется', /** UNTRANSLATED */ cannot_delete_management_api: 'Cannot delete Logto management API.', + /** UNTRANSLATED */ + cannot_modify_management_api: 'Cannot modify Logto management API.', }; export default Object.freeze(resource); diff --git a/packages/phrases/src/locales/tr-tr/errors/resource.ts b/packages/phrases/src/locales/tr-tr/errors/resource.ts index f0bb68939..fbfa33bbd 100644 --- a/packages/phrases/src/locales/tr-tr/errors/resource.ts +++ b/packages/phrases/src/locales/tr-tr/errors/resource.ts @@ -2,6 +2,8 @@ const resource = { resource_identifier_in_use: 'API tanımlayıcısı {{indicator}} zaten kullanımda', /** UNTRANSLATED */ cannot_delete_management_api: 'Cannot delete Logto management API.', + /** UNTRANSLATED */ + cannot_modify_management_api: 'Cannot modify Logto management API.', }; export default Object.freeze(resource); diff --git a/packages/phrases/src/locales/zh-cn/errors/resource.ts b/packages/phrases/src/locales/zh-cn/errors/resource.ts index 6ab300c0f..d563c5b1f 100644 --- a/packages/phrases/src/locales/zh-cn/errors/resource.ts +++ b/packages/phrases/src/locales/zh-cn/errors/resource.ts @@ -2,6 +2,8 @@ const resource = { resource_identifier_in_use: 'API 标识符 {{indicator}} 已被使用', /** UNTRANSLATED */ cannot_delete_management_api: 'Cannot delete Logto management API.', + /** UNTRANSLATED */ + cannot_modify_management_api: 'Cannot modify Logto management API.', }; export default Object.freeze(resource); diff --git a/packages/phrases/src/locales/zh-hk/errors/resource.ts b/packages/phrases/src/locales/zh-hk/errors/resource.ts index c74d93bfc..c03c3fcac 100644 --- a/packages/phrases/src/locales/zh-hk/errors/resource.ts +++ b/packages/phrases/src/locales/zh-hk/errors/resource.ts @@ -2,6 +2,8 @@ const resource = { resource_identifier_in_use: 'API 識別碼 {{indicator}} 已經被使用', /** UNTRANSLATED */ cannot_delete_management_api: 'Cannot delete Logto management API.', + /** UNTRANSLATED */ + cannot_modify_management_api: 'Cannot modify Logto management API.', }; export default Object.freeze(resource); diff --git a/packages/phrases/src/locales/zh-tw/errors/resource.ts b/packages/phrases/src/locales/zh-tw/errors/resource.ts index c74d93bfc..c03c3fcac 100644 --- a/packages/phrases/src/locales/zh-tw/errors/resource.ts +++ b/packages/phrases/src/locales/zh-tw/errors/resource.ts @@ -2,6 +2,8 @@ const resource = { resource_identifier_in_use: 'API 識別碼 {{indicator}} 已經被使用', /** UNTRANSLATED */ cannot_delete_management_api: 'Cannot delete Logto management API.', + /** UNTRANSLATED */ + cannot_modify_management_api: 'Cannot modify Logto management API.', }; export default Object.freeze(resource);