From 14f86c01d589bdc4a4275ab0715cf38badc71d07 Mon Sep 17 00:00:00 2001 From: Darcy Ye Date: Tue, 13 Dec 2022 10:19:42 +0800 Subject: [PATCH] fix: target can not be updated (#2609) --- packages/core/src/routes/connector.test.ts | 11 ++--- packages/core/src/routes/connector.ts | 47 ++++++++++++------- .../core/src/routes/connector.update.test.ts | 14 +++--- .../integration-tests/src/api/connector.ts | 12 +++-- .../tests/api/connector.test.ts | 10 ++-- packages/phrases/src/locales/de/errors.ts | 2 + packages/phrases/src/locales/en/errors.ts | 2 + packages/phrases/src/locales/fr/errors.ts | 2 + packages/phrases/src/locales/ko/errors.ts | 2 + packages/phrases/src/locales/pt-br/errors.ts | 2 + packages/phrases/src/locales/pt-pt/errors.ts | 2 + packages/phrases/src/locales/tr-tr/errors.ts | 2 + packages/phrases/src/locales/zh-cn/errors.ts | 1 + 13 files changed, 70 insertions(+), 39 deletions(-) diff --git a/packages/core/src/routes/connector.test.ts b/packages/core/src/routes/connector.test.ts index f0fd3caa6..e4398ac9b 100644 --- a/packages/core/src/routes/connector.test.ts +++ b/packages/core/src/routes/connector.test.ts @@ -188,7 +188,6 @@ describe('connector route', () => { metadata: { ...mockConnectorFactory.metadata, id: 'connectorId' }, }, ]); - countConnectorByConnectorId.mockResolvedValueOnce({ count: 0 }); const response = await connectorRequest.post('/connectors').send({ connectorId: 'id0', config: { cliend_id: 'client_id', client_secret: 'client_secret' }, @@ -255,7 +254,7 @@ describe('connector route', () => { { ...mockConnectorFactory, type: ConnectorType.Sms, - metadata: { ...mockConnectorFactory.metadata, id: 'id0', isStandard: true }, + metadata: { ...mockConnectorFactory.metadata, id: 'id1' }, }, ]); getLogtoConnectors.mockResolvedValueOnce([ @@ -266,20 +265,19 @@ describe('connector route', () => { ...mockLogtoConnector, }, ]); + countConnectorByConnectorId.mockResolvedValueOnce({ count: 0 }); const response = await connectorRequest.post('/connectors').send({ - connectorId: 'id0', + connectorId: 'id1', config: { cliend_id: 'client_id', client_secret: 'client_secret' }, - metadata: { target: 'target', name: { en: '' }, logo: '', logoDark: null }, }); expect(response).toHaveProperty('statusCode', 200); expect(response.body).toMatchObject( expect.objectContaining({ - connectorId: 'id0', + connectorId: 'id1', config: { cliend_id: 'client_id', client_secret: 'client_secret', }, - metadata: { target: 'target' }, }) ); expect(deleteConnectorByIds).toHaveBeenCalledWith(['id']); @@ -347,6 +345,7 @@ describe('connector route', () => { ]); const response = await connectorRequest.post('/connectors').send({ connectorId: 'id0', + metadata: { target: 'target' }, }); expect(response).toHaveProperty('statusCode', 422); }); diff --git a/packages/core/src/routes/connector.ts b/packages/core/src/routes/connector.ts index b30fb5e68..8673c615d 100644 --- a/packages/core/src/routes/connector.ts +++ b/packages/core/src/routes/connector.ts @@ -3,7 +3,6 @@ import { emailRegEx, phoneRegEx } from '@logto/core-kit'; import type { ConnectorFactoryResponse, ConnectorResponse } from '@logto/schemas'; import { arbitraryObjectGuard, Connectors, ConnectorType } from '@logto/schemas'; import { buildIdGenerator } from '@logto/shared'; -import { conditional } from '@silverhand/essentials'; import cleanDeep from 'clean-deep'; import { object, string } from 'zod'; @@ -114,9 +113,9 @@ export default function connectorRoutes(router: T) { // eslint-disable-next-line complexity async (ctx, next) => { const { - body: { connectorId }, - body, + body: { connectorId, metadata, config, syncProfile }, } = ctx.guard; + const connectorFactories = await loadConnectorFactories(); const connectorFactory = connectorFactories.find( ({ metadata: { id } }) => id === connectorId @@ -129,6 +128,15 @@ export default function connectorRoutes(router: T) { }); } + assertThat( + connectorFactory.metadata.isStandard !== true || metadata?.target, + 'connector.can_not_modify_target' + ); + assertThat( + connectorFactory.metadata.isStandard === true || metadata === undefined, + 'connector.cannot_change_metadata_for_non_standard_connector' + ); + const { count } = await countConnectorByConnectorId(connectorId); assertThat( count === 0 || connectorFactory.metadata.isStandard === true, @@ -140,25 +148,23 @@ export default function connectorRoutes(router: T) { if (connectorFactory.type === ConnectorType.Social) { const connectors = await getLogtoConnectors(); - const connectorTarget = body.metadata?.target ?? connectorFactory.metadata.target; assertThat( !connectors .filter(({ type }) => type === ConnectorType.Social) .some( ({ metadata: { target, platform } }) => - target === connectorTarget && platform === connectorFactory.metadata.platform + target === cleanDeep(metadata)?.target && + platform === connectorFactory.metadata.platform ), new RequestError({ code: 'connector.multiple_target_with_same_platform', status: 422 }) ); } const insertConnectorId = generateConnectorId(); - const { metadata, ...rest } = body; - ctx.body = await insertConnector({ id: insertConnectorId, - ...conditional(metadata && { metadata: cleanDeep(metadata) }), - ...rest, + connectorId, + ...cleanDeep({ syncProfile, config, metadata }), }); /** @@ -194,17 +200,25 @@ export default function connectorRoutes(router: T) { .pick({ config: true, metadata: true, syncProfile: true }) .partial(), }), - async (ctx, next) => { const { params: { id }, - body: { config }, - body, + body: { config, metadata, syncProfile }, } = ctx.guard; - const { type, validateConfig } = await getLogtoConnectorById(id); + const { type, validateConfig, metadata: originalMetadata } = await getLogtoConnectorById(id); - if (body.syncProfile) { + assertThat( + originalMetadata.isStandard !== true || metadata?.target === originalMetadata.target, + 'connector.can_not_modify_target' + ); + + assertThat( + originalMetadata.isStandard === true || metadata === undefined, + 'connector.cannot_change_metadata_for_non_standard_connector' + ); + + if (syncProfile) { assertThat( type === ConnectorType.Social, new RequestError({ code: 'connector.invalid_type_for_syncing_profile', status: 422 }) @@ -214,12 +228,9 @@ export default function connectorRoutes(router: T) { if (config) { validateConfig(config); } - // Once created, target can not be modified. - assertThat(body.metadata?.target === undefined, 'connector.can_not_modify_target'); - const { metadata: databaseMetadata, ...rest } = body; await updateConnector({ - set: databaseMetadata ? { metadata: cleanDeep(databaseMetadata), ...rest } : rest, + set: cleanDeep({ config, metadata, syncProfile }), where: { id }, jsonbMode: 'replace', }); diff --git a/packages/core/src/routes/connector.update.test.ts b/packages/core/src/routes/connector.update.test.ts index b295476e5..f71e1bae0 100644 --- a/packages/core/src/routes/connector.update.test.ts +++ b/packages/core/src/routes/connector.update.test.ts @@ -120,7 +120,7 @@ describe('connector PATCH routes', () => { updateConnector.mockResolvedValueOnce({ ...mockConnector, metadata: { - target: 'target', + target: 'connector', name: { en: 'connector_name', fr: 'connector_name' }, logo: 'new_logo.png', }, @@ -131,6 +131,7 @@ describe('connector PATCH routes', () => { name: { en: 'connector_name', fr: 'connector_name' }, logo: 'new_logo.png', logoDark: null, + target: 'connector', }, }); expect(updateConnector).toHaveBeenCalledWith( @@ -141,6 +142,7 @@ describe('connector PATCH routes', () => { metadata: { name: { en: 'connector_name', fr: 'connector_name' }, logo: 'new_logo.png', + target: 'connector', }, }, jsonbMode: 'replace', @@ -161,24 +163,20 @@ describe('connector PATCH routes', () => { updateConnector.mockResolvedValueOnce({ ...mockConnector, metadata: { - target: '', + target: 'connector', name: { en: '' }, logo: '', logoDark: '', }, }); const response = await connectorRequest.patch('/connectors/id').send({ - metadata: { - name: { en: '' }, - logo: '', - logoDark: '', - }, + metadata: { target: 'connector', name: { en: '' }, logo: '', logoDark: '' }, }); expect(updateConnector).toHaveBeenCalledWith( expect.objectContaining({ where: { id: 'id' }, set: { - metadata: {}, + metadata: { target: 'connector' }, }, jsonbMode: 'replace', }) diff --git a/packages/integration-tests/src/api/connector.ts b/packages/integration-tests/src/api/connector.ts index 388659776..15bdad19f 100644 --- a/packages/integration-tests/src/api/connector.ts +++ b/packages/integration-tests/src/api/connector.ts @@ -9,22 +9,26 @@ export const getConnector = async (connectorId: string) => authedAdminApi.get(`connectors/${connectorId}`).json(); // FIXME @Darcy: correct use of `id` and `connectorId`. -export const postConnector = async (connectorId: string) => +export const postConnector = async (connectorId: string, metadata?: Record) => authedAdminApi .post({ url: `connectors`, - json: { connectorId }, + json: { connectorId, metadata }, }) .json(); export const deleteConnectorById = async (id: string) => authedAdminApi.delete({ url: `connectors/${id}` }).json(); -export const updateConnectorConfig = async (connectorId: string, config: Record) => +export const updateConnectorConfig = async ( + connectorId: string, + config: Record, + metadata?: Record +) => authedAdminApi .patch({ url: `connectors/${connectorId}`, - json: { config }, + json: { config, metadata }, }) .json(); diff --git a/packages/integration-tests/tests/api/connector.test.ts b/packages/integration-tests/tests/api/connector.test.ts index ec5d65aca..9738da3b6 100644 --- a/packages/integration-tests/tests/api/connector.test.ts +++ b/packages/integration-tests/tests/api/connector.test.ts @@ -71,13 +71,18 @@ test('connector set-up flow', async () => { /* * Change to another SMS/Email connector */ - const { id } = await postConnector(mockStandardEmailConnectorId); - await updateConnectorConfig(id, mockStandardEmailConnectorConfig); + const { id } = await postConnector(mockStandardEmailConnectorId, { + target: 'mock-standard-mail', + }); // TODO [LOG-4862]: update mock connector + await updateConnectorConfig(id, mockStandardEmailConnectorConfig, { + target: 'mock-standard-mail', + }); // TODO [LOG-4862]: update mock connector connectorIdMap.set(mockStandardEmailConnectorId, id); const currentConnectors = await listConnectors(); expect( currentConnectors.some((connector) => connector.connectorId === mockEmailConnectorId) ).toBeFalsy(); + connectorIdMap.delete(mockEmailConnectorId); expect( currentConnectors.some((connector) => connector.connectorId === mockStandardEmailConnectorId) ).toBeTruthy(); @@ -85,7 +90,6 @@ test('connector set-up flow', async () => { currentConnectors.find((connector) => connector.connectorId === mockStandardEmailConnectorId) ?.config ).toEqual(mockStandardEmailConnectorConfig); - connectorIdMap.delete(mockEmailConnectorId); /* * Delete (i.e. disable) a connector diff --git a/packages/phrases/src/locales/de/errors.ts b/packages/phrases/src/locales/de/errors.ts index 00bcdeefa..a5097f1dd 100644 --- a/packages/phrases/src/locales/de/errors.ts +++ b/packages/phrases/src/locales/de/errors.ts @@ -112,6 +112,8 @@ const errors = { can_not_modify_target: 'The connector target can not be modified.', multiple_target_with_same_platform: 'You can not have multiple social connectors that have same target and platform.', + cannot_change_metadata_for_non_standard_connector: + "This connector's `metadata` cannot be changed.", }, passcode: { phone_email_empty: 'Telefonnummer oder E-Mail darf nicht leer sein.', diff --git a/packages/phrases/src/locales/en/errors.ts b/packages/phrases/src/locales/en/errors.ts index ffe1a535e..eed909db1 100644 --- a/packages/phrases/src/locales/en/errors.ts +++ b/packages/phrases/src/locales/en/errors.ts @@ -111,6 +111,8 @@ const errors = { can_not_modify_target: 'The connector target can not be modified.', multiple_target_with_same_platform: 'You can not have multiple social connectors that have same target and platform.', + cannot_change_metadata_for_non_standard_connector: + "This connector's `metadata` cannot be changed.", }, passcode: { phone_email_empty: 'Both phone and email are empty.', diff --git a/packages/phrases/src/locales/fr/errors.ts b/packages/phrases/src/locales/fr/errors.ts index dd75d6499..97ff0bec0 100644 --- a/packages/phrases/src/locales/fr/errors.ts +++ b/packages/phrases/src/locales/fr/errors.ts @@ -118,6 +118,8 @@ const errors = { can_not_modify_target: 'The connector target can not be modified.', // UNTRANSLATED multiple_target_with_same_platform: 'You can not have multiple social connectors that have same target and platform.', // UNTRANSLATED + cannot_change_metadata_for_non_standard_connector: + "This connector's `metadata` cannot be changed.", // UNTRANSLATED }, passcode: { phone_email_empty: "Le téléphone et l'email sont vides.", diff --git a/packages/phrases/src/locales/ko/errors.ts b/packages/phrases/src/locales/ko/errors.ts index 2cf32646b..f1ee88d42 100644 --- a/packages/phrases/src/locales/ko/errors.ts +++ b/packages/phrases/src/locales/ko/errors.ts @@ -110,6 +110,8 @@ const errors = { can_not_modify_target: 'The connector target can not be modified.', // UNTRANSLATED multiple_target_with_same_platform: 'You can not have multiple social connectors that have same target and platform.', // UNTRANSLATED + cannot_change_metadata_for_non_standard_connector: + "This connector's `metadata` cannot be changed.", // UNTRANSLATED }, passcode: { phone_email_empty: '휴대전화번호 그리고 이메일이 비어있어요.', diff --git a/packages/phrases/src/locales/pt-br/errors.ts b/packages/phrases/src/locales/pt-br/errors.ts index fc22265e5..71439ef8a 100644 --- a/packages/phrases/src/locales/pt-br/errors.ts +++ b/packages/phrases/src/locales/pt-br/errors.ts @@ -115,6 +115,8 @@ const errors = { can_not_modify_target: 'O destino do conector não pode ser modificado.', multiple_target_with_same_platform: 'Você não pode ter vários conectores sociais com o mesmo destino e plataforma.', + cannot_change_metadata_for_non_standard_connector: + "This connector's `metadata` cannot be changed.", // UNTRANSLATED }, passcode: { phone_email_empty: 'Telefone e e-mail estão vazios.', diff --git a/packages/phrases/src/locales/pt-pt/errors.ts b/packages/phrases/src/locales/pt-pt/errors.ts index df664d633..919084e60 100644 --- a/packages/phrases/src/locales/pt-pt/errors.ts +++ b/packages/phrases/src/locales/pt-pt/errors.ts @@ -113,6 +113,8 @@ const errors = { can_not_modify_target: 'The connector target can not be modified.', // UNTRANSLATED multiple_target_with_same_platform: 'You can not have multiple social connectors that have same target and platform.', // UNTRANSLATED + cannot_change_metadata_for_non_standard_connector: + "This connector's `metadata` cannot be changed.", // UNTRANSLATED }, passcode: { phone_email_empty: 'O campos telefone e email estão vazios.', diff --git a/packages/phrases/src/locales/tr-tr/errors.ts b/packages/phrases/src/locales/tr-tr/errors.ts index 5c8343819..a213c2254 100644 --- a/packages/phrases/src/locales/tr-tr/errors.ts +++ b/packages/phrases/src/locales/tr-tr/errors.ts @@ -112,6 +112,8 @@ const errors = { can_not_modify_target: 'The connector target can not be modified.', // UNTRANSLATED multiple_target_with_same_platform: 'You can not have multiple social connectors that have same target and platform.', // UNTRANSLATED + cannot_change_metadata_for_non_standard_connector: + "This connector's `metadata` cannot be changed.", // UNTRANSLATED }, passcode: { phone_email_empty: 'Hem telefon hem de e-posta adresi yok.', diff --git a/packages/phrases/src/locales/zh-cn/errors.ts b/packages/phrases/src/locales/zh-cn/errors.ts index c432762e6..a4b3f66f4 100644 --- a/packages/phrases/src/locales/zh-cn/errors.ts +++ b/packages/phrases/src/locales/zh-cn/errors.ts @@ -103,6 +103,7 @@ const errors = { invalid_type_for_syncing_profile: '只有社交连接器可以开启用户档案同步。', can_not_modify_target: '不可修改连接器 target。', multiple_target_with_same_platform: '不能同时存在多个有相同 target 和平台类型的社交连接器。', + cannot_change_metadata_for_non_standard_connector: '不可配置该连接器的 metadata 参数。', }, passcode: { phone_email_empty: '手机号与邮箱地址均为空',