From f535e3d23922a55ec3d6d1d15511e290b74f00da Mon Sep 17 00:00:00 2001 From: Darcy Ye Date: Fri, 24 Jan 2025 13:51:33 +0800 Subject: [PATCH 1/6] chore: hide oidcClientMetadata of SAML apps when using GET app APIs --- .../core/src/routes/applications/application.ts | 15 +++++++++++++-- .../api/application/saml-application.test.ts | 17 +++++++++++++++++ 2 files changed, 30 insertions(+), 2 deletions(-) diff --git a/packages/core/src/routes/applications/application.ts b/packages/core/src/routes/applications/application.ts index 828e54b66d1..dc927137276 100644 --- a/packages/core/src/routes/applications/application.ts +++ b/packages/core/src/routes/applications/application.ts @@ -10,7 +10,7 @@ import { InternalRole, } from '@logto/schemas'; import { generateStandardId, generateStandardSecret } from '@logto/shared'; -import { conditional } from '@silverhand/essentials'; +import { cond, conditional } from '@silverhand/essentials'; import { boolean, object, string, z } from 'zod'; import RequestError from '#src/errors/RequestError/index.js'; @@ -134,7 +134,12 @@ export default function applicationRoutes( // Return totalCount to pagination middleware ctx.pagination.totalCount = count; - ctx.body = applications; + ctx.body = applications.map((application) => + application.type === ApplicationType.SAML + ? // Hide `oidcClientMetadata` for SAML application + { ...application, oidcClientMetadata: buildOidcClientMetadata() } + : application + ); return next(); } @@ -239,6 +244,12 @@ export default function applicationRoutes( ctx.body = { ...application, + ...cond( + // Hide `oidcClientMetadata` for SAML application + application.type === ApplicationType.SAML && { + oidcClientMetadata: buildOidcClientMetadata(), + } + ), isAdmin: includesInternalAdminRole(applicationsRoles), }; diff --git a/packages/integration-tests/src/tests/api/application/saml-application.test.ts b/packages/integration-tests/src/tests/api/application/saml-application.test.ts index 1a846444ac9..35d559a0c30 100644 --- a/packages/integration-tests/src/tests/api/application/saml-application.test.ts +++ b/packages/integration-tests/src/tests/api/application/saml-application.test.ts @@ -5,6 +5,7 @@ import { createApplication, deleteApplication, getApplications, + getApplication, updateApplication, } from '#src/api/application.js'; import { @@ -30,6 +31,15 @@ describe('SAML application', () => { description: 'test', }); + await expect(getApplication(createdSamlApplication.id)).resolves.toMatchObject( + expect.objectContaining({ + oidcClientMetadata: { + redirectUris: [], + postLogoutRedirectUris: [], + }, + }) + ); + expect(createdSamlApplication.nameIdFormat).toBe(NameIdFormat.Persistent); // Check if the SAML application's OIDC metadata redirect URI is properly set. @@ -46,6 +56,13 @@ describe('SAML application', () => { ) ).toBe(true); + expect( + samlApplications.every( + ({ oidcClientMetadata: { redirectUris, postLogoutRedirectUris } }) => + redirectUris.length === 0 && postLogoutRedirectUris.length === 0 + ) + ).toBe(true); + await deleteSamlApplication(createdSamlApplication.id); }); From d770e4d481e046725ac11f01d91787a5c611fedf Mon Sep 17 00:00:00 2001 From: Darcy Ye Date: Sat, 25 Jan 2025 12:37:39 +0800 Subject: [PATCH 2/6] chore: add integration log for debugging --- .../src/tests/api/application/saml-application.test.ts | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/packages/integration-tests/src/tests/api/application/saml-application.test.ts b/packages/integration-tests/src/tests/api/application/saml-application.test.ts index 35d559a0c30..ce19a180f21 100644 --- a/packages/integration-tests/src/tests/api/application/saml-application.test.ts +++ b/packages/integration-tests/src/tests/api/application/saml-application.test.ts @@ -45,6 +45,7 @@ describe('SAML application', () => { // Check if the SAML application's OIDC metadata redirect URI is properly set. // We need to do this since we do not return OIDC related info when using SAML app APIs. const samlApplications = await getApplications([ApplicationType.SAML]); + expect(samlApplications.every(({ type }) => type === ApplicationType.SAML)); const pickedSamlApplication = samlApplications.find( ({ id }) => id === createdSamlApplication.id ); @@ -56,6 +57,9 @@ describe('SAML application', () => { ) ).toBe(true); + console.log('pickedSamlApplication', pickedSamlApplication); + console.log('samlApplications', samlApplications); + expect( samlApplications.every( ({ oidcClientMetadata: { redirectUris, postLogoutRedirectUris } }) => From ee72dcdd37f95b83ca87ca0ab182c8e8f3fe4e59 Mon Sep 17 00:00:00 2001 From: Darcy Ye Date: Sat, 25 Jan 2025 14:46:28 +0800 Subject: [PATCH 3/6] chore: add integration log for debugging --- .../src/routes/applications/application.ts | 22 +++++++++++-------- 1 file changed, 13 insertions(+), 9 deletions(-) diff --git a/packages/core/src/routes/applications/application.ts b/packages/core/src/routes/applications/application.ts index dc927137276..1ae2f05085d 100644 --- a/packages/core/src/routes/applications/application.ts +++ b/packages/core/src/routes/applications/application.ts @@ -13,6 +13,7 @@ import { generateStandardId, generateStandardSecret } from '@logto/shared'; import { cond, conditional } from '@silverhand/essentials'; import { boolean, object, string, z } from 'zod'; +import { EnvSet } from '#src/env-set/index.js'; import RequestError from '#src/errors/RequestError/index.js'; import koaGuard from '#src/middleware/koa-guard.js'; import koaPagination from '#src/middleware/koa-pagination.js'; @@ -134,12 +135,14 @@ export default function applicationRoutes( // Return totalCount to pagination middleware ctx.pagination.totalCount = count; - ctx.body = applications.map((application) => - application.type === ApplicationType.SAML - ? // Hide `oidcClientMetadata` for SAML application - { ...application, oidcClientMetadata: buildOidcClientMetadata() } - : application - ); + ctx.body = applications.map((application) => ({ + ...application, + // Hide `oidcClientMetadata` for SAML application + ...cond( + application.type === ApplicationType.SAML && + EnvSet.values.isDevFeaturesEnabled && { oidcClientMetadata: buildOidcClientMetadata() } + ), + })); return next(); } @@ -246,9 +249,10 @@ export default function applicationRoutes( ...application, ...cond( // Hide `oidcClientMetadata` for SAML application - application.type === ApplicationType.SAML && { - oidcClientMetadata: buildOidcClientMetadata(), - } + application.type === ApplicationType.SAML && + EnvSet.values.isDevFeaturesEnabled && { + oidcClientMetadata: buildOidcClientMetadata(), + } ), isAdmin: includesInternalAdminRole(applicationsRoles), }; From e961cae9c6c103752a141389ec293493ea8b18ac Mon Sep 17 00:00:00 2001 From: Darcy Ye Date: Sat, 25 Jan 2025 15:47:14 +0800 Subject: [PATCH 4/6] chore: add integration log for debugging --- .../src/routes/applications/application.ts | 40 ++++++++++--------- 1 file changed, 22 insertions(+), 18 deletions(-) diff --git a/packages/core/src/routes/applications/application.ts b/packages/core/src/routes/applications/application.ts index 1ae2f05085d..bc411a075cd 100644 --- a/packages/core/src/routes/applications/application.ts +++ b/packages/core/src/routes/applications/application.ts @@ -2,6 +2,7 @@ /* eslint-disable max-lines */ import type { Role } from '@logto/schemas'; import { + type Application, Applications, ApplicationType, buildDemoAppDataForTenant, @@ -10,7 +11,7 @@ import { InternalRole, } from '@logto/schemas'; import { generateStandardId, generateStandardSecret } from '@logto/shared'; -import { cond, conditional } from '@silverhand/essentials'; +import { conditional } from '@silverhand/essentials'; import { boolean, object, string, z } from 'zod'; import { EnvSet } from '#src/env-set/index.js'; @@ -38,6 +39,22 @@ const parseIsThirdPartQueryParam = (isThirdPartyQuery: 'true' | 'false' | undefi return isThirdPartyQuery === 'true'; }; +const hideOidcClientMetadataForSamlApp = (application: Application) => { + return { + ...application, + ...conditional( + application.type === ApplicationType.SAML && + EnvSet.values.isDevFeaturesEnabled && { + oidcClientMetadata: buildOidcClientMetadata(), + } + ), + }; +}; + +const hideOidcClientMetadataForSamlApps = (applications: readonly Application[]) => { + return applications.map((application) => hideOidcClientMetadataForSamlApp(application)); +}; + const applicationTypeGuard = z.nativeEnum(ApplicationType); export default function applicationRoutes( @@ -102,13 +119,14 @@ export default function applicationRoutes( ); if (paginationDisabled) { - ctx.body = await queries.applications.findApplications({ + const rawApplications = await queries.applications.findApplications({ search, excludeApplicationIds, excludeOrganizationId, types, isThirdParty, }); + ctx.body = hideOidcClientMetadataForSamlApps(rawApplications); return next(); } @@ -135,14 +153,7 @@ export default function applicationRoutes( // Return totalCount to pagination middleware ctx.pagination.totalCount = count; - ctx.body = applications.map((application) => ({ - ...application, - // Hide `oidcClientMetadata` for SAML application - ...cond( - application.type === ApplicationType.SAML && - EnvSet.values.isDevFeaturesEnabled && { oidcClientMetadata: buildOidcClientMetadata() } - ), - })); + ctx.body = hideOidcClientMetadataForSamlApps(applications); return next(); } @@ -246,14 +257,7 @@ export default function applicationRoutes( await queries.applicationsRoles.findApplicationsRolesByApplicationId(id); ctx.body = { - ...application, - ...cond( - // Hide `oidcClientMetadata` for SAML application - application.type === ApplicationType.SAML && - EnvSet.values.isDevFeaturesEnabled && { - oidcClientMetadata: buildOidcClientMetadata(), - } - ), + ...hideOidcClientMetadataForSamlApp(application), isAdmin: includesInternalAdminRole(applicationsRoles), }; From 4576e23a33f86c9ec39e76647bdbf4f9f387f5ba Mon Sep 17 00:00:00 2001 From: Darcy Ye Date: Sat, 25 Jan 2025 16:28:03 +0800 Subject: [PATCH 5/6] chore: add integration log for debugging --- packages/core/src/routes/applications/application.ts | 3 +-- .../src/tests/api/application/saml-application.test.ts | 10 ---------- 2 files changed, 1 insertion(+), 12 deletions(-) diff --git a/packages/core/src/routes/applications/application.ts b/packages/core/src/routes/applications/application.ts index bc411a075cd..3cc40fb7fc8 100644 --- a/packages/core/src/routes/applications/application.ts +++ b/packages/core/src/routes/applications/application.ts @@ -1,8 +1,7 @@ // TODO: @darcyYe refactor this file later to remove disable max line comment /* eslint-disable max-lines */ -import type { Role } from '@logto/schemas'; +import type { Role, Application } from '@logto/schemas'; import { - type Application, Applications, ApplicationType, buildDemoAppDataForTenant, diff --git a/packages/integration-tests/src/tests/api/application/saml-application.test.ts b/packages/integration-tests/src/tests/api/application/saml-application.test.ts index ce19a180f21..54f01e34056 100644 --- a/packages/integration-tests/src/tests/api/application/saml-application.test.ts +++ b/packages/integration-tests/src/tests/api/application/saml-application.test.ts @@ -50,16 +50,6 @@ describe('SAML application', () => { ({ id }) => id === createdSamlApplication.id ); expect(pickedSamlApplication).toBeDefined(); - expect(pickedSamlApplication!.oidcClientMetadata.redirectUris.length).toBe(1); - expect( - pickedSamlApplication!.oidcClientMetadata.redirectUris[0]!.endsWith( - `api/saml-applications/${createdSamlApplication.id}/callback` - ) - ).toBe(true); - - console.log('pickedSamlApplication', pickedSamlApplication); - console.log('samlApplications', samlApplications); - expect( samlApplications.every( ({ oidcClientMetadata: { redirectUris, postLogoutRedirectUris } }) => From be7e43027ed13a3b155d808ce20d556ba25da1b6 Mon Sep 17 00:00:00 2001 From: Darcy Ye Date: Sun, 26 Jan 2025 10:26:23 +0800 Subject: [PATCH 6/6] fix: remove unnecessary paywall for creating SAML app secrets --- packages/core/src/routes/saml-application/index.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/packages/core/src/routes/saml-application/index.ts b/packages/core/src/routes/saml-application/index.ts index 2ecb692fce9..6ac11c8f35d 100644 --- a/packages/core/src/routes/saml-application/index.ts +++ b/packages/core/src/routes/saml-application/index.ts @@ -179,7 +179,6 @@ export default function samlApplicationRoutes( router.post( '/saml-applications/:id/secrets', - koaQuotaGuard({ key: 'samlApplicationsLimit', quota }), koaGuard({ params: z.object({ id: z.string() }), // The life span of the SAML app secret is in years (at least 1 year), and for security concern, secrets which never expire are not recommended.