From 22d0a17389f0f455bd8af6c19abfd43c86aa5e84 Mon Sep 17 00:00:00 2001 From: Darcy Ye Date: Wed, 4 Dec 2024 22:54:23 -0800 Subject: [PATCH] fix: fix some SAML app bugs (#6852) --- .../libraries/saml-applications.ts | 44 +++++++----- .../saml-applications/libraries/utils.test.ts | 2 +- .../src/saml-applications/libraries/utils.ts | 3 + .../src/saml-applications/queries/secrets.ts | 53 ++++++++++++-- .../src/saml-applications/routes/anonymous.ts | 2 +- .../src/saml-applications/routes/index.ts | 32 ++++++--- .../api/application/saml-application.test.ts | 70 ++++++------------- .../jsonb-types/saml-application-configs.ts | 2 +- .../schemas/src/types/saml-application.ts | 51 ++++++++------ 9 files changed, 153 insertions(+), 106 deletions(-) diff --git a/packages/core/src/saml-applications/libraries/saml-applications.ts b/packages/core/src/saml-applications/libraries/saml-applications.ts index d369c9505..3b178c2a1 100644 --- a/packages/core/src/saml-applications/libraries/saml-applications.ts +++ b/packages/core/src/saml-applications/libraries/saml-applications.ts @@ -18,13 +18,15 @@ import { ensembleSamlApplication, generateKeyPairAndCertificate, buildSingleSignOnUrl, + buildSamlIdentityProviderEntityId, } from './utils.js'; export const createSamlApplicationsLibrary = (queries: Queries) => { const { applications: { findApplicationById, updateApplicationById }, samlApplicationSecrets: { - insertSamlApplicationSecret, + insertInactiveSamlApplicationSecret, + insertActiveSamlApplicationSecret, findActiveSamlApplicationSecretByApplicationId, }, samlApplicationConfigs: { @@ -33,23 +35,34 @@ export const createSamlApplicationsLibrary = (queries: Queries) => { }, } = queries; - const createSamlApplicationSecret = async ( - applicationId: string, - // Set certificate life span to 3 years by default. - lifeSpanInDays = 365 * 3 - ): Promise => { + /** + * @remarks + * When creating a SAML app secret, it is set to inactive by default. Since a SAML app can only have one active secret at a time, creating a new active secret will deactivate all current secrets. + */ + const createSamlApplicationSecret = async ({ + applicationId, + lifeSpanInDays = 365 * 3, + isActive = false, + }: { + applicationId: string; + lifeSpanInDays?: number; + isActive?: boolean; + }): Promise => { const { privateKey, certificate, notAfter } = await generateKeyPairAndCertificate( lifeSpanInDays ); - return insertSamlApplicationSecret({ + const createObject = { id: generateStandardId(), applicationId, privateKey, certificate, expiresAt: notAfter.getTime(), - active: false, - }); + }; + + return isActive + ? insertActiveSamlApplicationSecret(createObject) + : insertInactiveSamlApplicationSecret(createObject); }; const findSamlApplicationById = async (id: string): Promise => { @@ -71,8 +84,9 @@ export const createSamlApplicationsLibrary = (queries: Queries) => { id: string, patchApplicationObject: PatchSamlApplication ): Promise => { - const { config, ...applicationData } = patchApplicationObject; + const { name, description, customData, ...config } = patchApplicationObject; const originalApplication = await findApplicationById(id); + const applicationData = { name, description, customData }; assertThat( originalApplication.type === ApplicationType.SAML, @@ -86,11 +100,11 @@ export const createSamlApplicationsLibrary = (queries: Queries) => { Object.keys(applicationData).length > 0 ? updateApplicationById(id, removeUndefinedKeys(applicationData)) : originalApplication, - config + Object.keys(config).length > 0 ? updateSamlApplicationConfig({ set: config, where: { applicationId: id }, - jsonbMode: 'replace', + jsonbMode: 'merge', }) : findSamlApplicationConfigByApplicationId(id), ]); @@ -102,18 +116,16 @@ export const createSamlApplicationsLibrary = (queries: Queries) => { }; const getSamlIdPMetadataByApplicationId = async (id: string): Promise<{ metadata: string }> => { - const [{ tenantId, entityId }, { certificate }] = await Promise.all([ + const [{ tenantId }, { certificate }] = await Promise.all([ findSamlApplicationConfigByApplicationId(id), findActiveSamlApplicationSecretByApplicationId(id), ]); - assertThat(entityId, 'application.saml.entity_id_required'); - const tenantEndpoint = getTenantEndpoint(tenantId, EnvSet.values); // eslint-disable-next-line new-cap const idp = saml.IdentityProvider({ - entityID: entityId, + entityID: buildSamlIdentityProviderEntityId(tenantEndpoint, id), signingCert: certificate, singleSignOnService: [ { diff --git a/packages/core/src/saml-applications/libraries/utils.test.ts b/packages/core/src/saml-applications/libraries/utils.test.ts index 2cfbabb39..5419109ce 100644 --- a/packages/core/src/saml-applications/libraries/utils.test.ts +++ b/packages/core/src/saml-applications/libraries/utils.test.ts @@ -74,7 +74,7 @@ describe('calculateCertificateFingerprints', () => { it('should calculate correct fingerprints for valid certificate', () => { const fingerprints = calculateCertificateFingerprints(validCertificate); - // Verify fingerprint format + // Verify fingerprints format expect(fingerprints.sha256.formatted).toMatch(/^([\dA-F]{2}:){31}[\dA-F]{2}$/); expect(fingerprints.sha256.unformatted).toMatch(/^[\dA-F]{64}$/); diff --git a/packages/core/src/saml-applications/libraries/utils.ts b/packages/core/src/saml-applications/libraries/utils.ts index 7442e745a..eb3210912 100644 --- a/packages/core/src/saml-applications/libraries/utils.ts +++ b/packages/core/src/saml-applications/libraries/utils.ts @@ -152,3 +152,6 @@ export const validateAcsUrl = (acsUrl: SamlAcsUrl) => { export const buildSingleSignOnUrl = (baseUrl: URL, samlApplicationId: string) => appendPath(baseUrl, `api/saml/${samlApplicationId}/authn`).toString(); + +export const buildSamlIdentityProviderEntityId = (baseUrl: URL, samlApplicationId: string) => + appendPath(baseUrl, `saml/${samlApplicationId}`).toString(); diff --git a/packages/core/src/saml-applications/queries/secrets.ts b/packages/core/src/saml-applications/queries/secrets.ts index 871ad40d1..237a85118 100644 --- a/packages/core/src/saml-applications/queries/secrets.ts +++ b/packages/core/src/saml-applications/queries/secrets.ts @@ -1,18 +1,58 @@ -import { SamlApplicationSecrets, type SamlApplicationSecret } from '@logto/schemas'; +import { + type CreateSamlApplicationSecret, + SamlApplicationSecrets, + type SamlApplicationSecret, +} from '@logto/schemas'; import type { CommonQueryMethods } from '@silverhand/slonik'; import { sql } from '@silverhand/slonik'; import { buildInsertIntoWithPool } from '#src/database/insert-into.js'; import RequestError from '#src/errors/RequestError/index.js'; import { DeletionError } from '#src/errors/SlonikError/index.js'; -import { convertToIdentifiers } from '#src/utils/sql.js'; +import { convertToIdentifiers, type OmitAutoSetFields } from '#src/utils/sql.js'; const { table, fields } = convertToIdentifiers(SamlApplicationSecrets); export const createSamlApplicationSecretsQueries = (pool: CommonQueryMethods) => { - const insertSamlApplicationSecret = buildInsertIntoWithPool(pool)(SamlApplicationSecrets, { - returning: true, - }); + /** + * @remarks + * When creating a SAML app secret, it is set to inactive by default. Since only one secret can be active for a SAML app at a time, creating a new active secret will deactivate all current secrets. + * If you need to create an active secret, it is best to use a transaction to ensure that all steps are successful. + */ + const insertInactiveSamlApplicationSecret = async ( + data: Omit, 'active'> + ) => { + return buildInsertIntoWithPool(pool)(SamlApplicationSecrets, { + returning: true, + })({ ...data, active: false }); + }; + + const insertActiveSamlApplicationSecret = async ( + data: Omit, 'active'> + ) => { + return pool.transaction(async (transaction) => { + const newSecret = await buildInsertIntoWithPool(transaction)(SamlApplicationSecrets, { + returning: true, + })({ ...data, active: false }); + + // If activating this secret, first deactivate all other secrets of the same application + await transaction.query(sql` + update ${table} + set ${fields.active} = false + where ${fields.applicationId} = ${newSecret.applicationId} + `); + + // Update the status of the specified secret + const updatedSecret = await transaction.one(sql` + update ${table} + set ${fields.active} = true + where ${fields.id} = ${newSecret.id} + returning ${sql.join(Object.values(fields), sql`, `)} + `); + + return updatedSecret; + }); + }; const findSamlApplicationSecretsByApplicationId = async (applicationId: string) => pool.any(sql` @@ -83,7 +123,8 @@ export const createSamlApplicationSecretsQueries = (pool: CommonQueryMethods) => }; return { - insertSamlApplicationSecret, + insertInactiveSamlApplicationSecret, + insertActiveSamlApplicationSecret, findSamlApplicationSecretsByApplicationId, findActiveSamlApplicationSecretByApplicationId, findSamlApplicationSecretByApplicationIdAndId, diff --git a/packages/core/src/saml-applications/routes/anonymous.ts b/packages/core/src/saml-applications/routes/anonymous.ts index 5753c3337..661eb48bb 100644 --- a/packages/core/src/saml-applications/routes/anonymous.ts +++ b/packages/core/src/saml-applications/routes/anonymous.ts @@ -14,7 +14,7 @@ export default function samlApplicationAnonymousRoutes { diff --git a/packages/core/src/saml-applications/routes/index.ts b/packages/core/src/saml-applications/routes/index.ts index 140e7c1a5..e78d4fe91 100644 --- a/packages/core/src/saml-applications/routes/index.ts +++ b/packages/core/src/saml-applications/routes/index.ts @@ -1,6 +1,5 @@ import { ApplicationType, - certificateFingerprintsGuard, samlApplicationCreateGuard, samlApplicationPatchGuard, samlApplicationResponseGuard, @@ -56,9 +55,9 @@ export default function samlApplicationRoutes( status: [201, 400, 422], }), async (ctx, next) => { - const { name, description, customData, config } = ctx.guard.body; + const { name, description, customData, ...config } = ctx.guard.body; - if (config?.acsUrl) { + if (config.acsUrl) { validateAcsUrl(config.acsUrl); } @@ -81,7 +80,7 @@ export default function samlApplicationRoutes( applicationId: application.id, ...config, }), - createSamlApplicationSecret(application.id), + createSamlApplicationSecret({ applicationId: application.id, isActive: true }), ]); ctx.status = 201; @@ -176,8 +175,12 @@ export default function samlApplicationRoutes( params: { id }, } = ctx.guard; + const secret = await createSamlApplicationSecret({ applicationId: id, lifeSpanInDays }); ctx.status = 201; - ctx.body = await createSamlApplicationSecret(id, lifeSpanInDays); + ctx.body = { + ...secret, + fingerprints: calculateCertificateFingerprints(secret.certificate), + }; return next(); } @@ -194,7 +197,11 @@ export default function samlApplicationRoutes( const { id } = ctx.guard.params; ctx.status = 200; - ctx.body = await findSamlApplicationSecretsByApplicationId(id); + const secrets = await findSamlApplicationSecretsByApplicationId(id); + ctx.body = secrets.map((secret) => ({ + ...secret, + fingerprints: calculateCertificateFingerprints(secret.certificate), + })); return next(); } @@ -255,7 +262,10 @@ export default function samlApplicationRoutes( await updateSamlApplicationSecretStatusByApplicationIdAndSecretId(id, secretId, active); ctx.status = 200; - ctx.body = updatedSamlApplicationSecret; + ctx.body = { + ...updatedSamlApplicationSecret, + fingerprints: calculateCertificateFingerprints(updatedSamlApplicationSecret.certificate), + }; return next(); } @@ -266,9 +276,9 @@ export default function samlApplicationRoutes( koaGuard({ params: z.object({ id: z.string() }), status: [200, 400, 404], - response: z.object({ - certificate: z.string(), - fingerprints: certificateFingerprintsGuard, + response: samlApplicationSecretResponseGuard.pick({ + certificate: true, + fingerprints: true, }), }), async (ctx, next) => { @@ -310,7 +320,7 @@ export default function samlApplicationRoutes( '/saml-applications/:id/metadata.xml', koaGuard({ params: z.object({ id: z.string() }), - status: [200, 400, 404], + status: [200, 404], response: z.string(), }), async (ctx, next) => { 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 f73aff319..6adcae11b 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 @@ -33,11 +33,9 @@ describe('SAML application', () => { createSamlApplication({ name: 'test', description: 'test', - config: { - acsUrl: { - binding: BindingType.Redirect, - url: 'https://example.com', - }, + acsUrl: { + binding: BindingType.Redirect, + url: 'https://example.com', }, }), { @@ -58,7 +56,7 @@ describe('SAML application', () => { const createdSamlApplication = await createSamlApplication({ name: 'test', description: 'test', - config, + ...config, }); expect(createdSamlApplication.entityId).toEqual(config.entityId); expect(createdSamlApplication.acsUrl).toEqual(config.acsUrl); @@ -69,18 +67,27 @@ describe('SAML application', () => { const createdSamlApplication = await createSamlApplication({ name: 'test', description: 'test', + entityId: 'http://example.logto.io/foo', }); + expect(createdSamlApplication.entityId).toEqual('http://example.logto.io/foo'); + expect(createdSamlApplication.acsUrl).toEqual(null); + expect(createdSamlApplication.attributeMapping).toEqual({}); const newConfig = { acsUrl: { binding: BindingType.Post, url: 'https://example.logto.io/sso/saml', }, + entityId: null, }; const updatedSamlApplication = await updateSamlApplication(createdSamlApplication.id, { name: 'updated', - config: newConfig, + ...newConfig, }); + expect(updatedSamlApplication.acsUrl).toEqual(newConfig.acsUrl); + expect(updatedSamlApplication.entityId).toEqual(newConfig.entityId); + expect(updatedSamlApplication.attributeMapping).toEqual({}); + const upToDateSamlApplication = await getSamlApplication(createdSamlApplication.id); expect(updatedSamlApplication).toEqual(upToDateSamlApplication); @@ -134,21 +141,14 @@ describe('SAML application secrets/certificate/metadata', () => { await deleteSamlApplication(id); }); - it('should return 404 when getting certificate/metadata without active secret', async () => { + it('should be able to get certificate/metadata after creating a SAML app', async () => { const { id } = await createSamlApplication({ name: 'test', description: 'test', }); - await expectRejects(getSamlApplicationCertificate(id), { - status: 404, - code: 'application.saml.no_active_secret', - }); - - await expectRejects(getSamlApplicationMetadata(id), { - status: 404, - code: 'application.saml.no_active_secret', - }); + await expect(getSamlApplicationCertificate(id)).resolves.not.toThrow(); + await expect(getSamlApplicationMetadata(id)).resolves.not.toThrow(); await deleteSamlApplication(id); }); @@ -161,46 +161,20 @@ describe('SAML application secrets/certificate/metadata', () => { const createdSecret = await createSamlApplicationSecret(id, 30); - const secrets = await getSamlApplicationSecrets(id); - expect(secrets.length).toBe(2); - expect(secrets.every(({ createdAt, expiresAt }) => createdAt < expiresAt)).toBe(true); - const updatedSecret = await updateSamlApplicationSecret(id, createdSecret.id, true); expect(updatedSecret.active).toBe(true); + const secrets = await getSamlApplicationSecrets(id); + expect(secrets.length).toBe(2); + expect(secrets.every(({ createdAt, expiresAt }) => createdAt < expiresAt)).toBe(true); + expect(secrets.filter(({ active }) => active).length).toBe(1); + const { certificate } = await getSamlApplicationCertificate(id); expect(typeof certificate).toBe('string'); await deleteSamlApplication(id); }); - it('should handle metadata requirements correctly', async () => { - const { id } = await createSamlApplication({ - name: 'test', - description: 'test', - }); - - const secret = await createSamlApplicationSecret(id, 30); - await updateSamlApplicationSecret(id, secret.id, true); - - await expect(getSamlApplicationCertificate(id)).resolves.not.toThrow(); - - await expectRejects(getSamlApplicationMetadata(id), { - status: 400, - code: 'application.saml.entity_id_required', - }); - - await updateSamlApplication(id, { - config: { - entityId: 'https://example.logto.io', - }, - }); - - await expect(getSamlApplicationMetadata(id)).resolves.not.toThrow(); - - await deleteSamlApplication(id); - }); - it('should prevent deletion of active secrets', async () => { const { id } = await createSamlApplication({ name: 'test', diff --git a/packages/schemas/src/foundations/jsonb-types/saml-application-configs.ts b/packages/schemas/src/foundations/jsonb-types/saml-application-configs.ts index 97c4f3222..a2df45102 100644 --- a/packages/schemas/src/foundations/jsonb-types/saml-application-configs.ts +++ b/packages/schemas/src/foundations/jsonb-types/saml-application-configs.ts @@ -13,7 +13,7 @@ export enum BindingType { } export type SamlAcsUrl = { - binding?: BindingType; + binding: BindingType; url: string; }; diff --git a/packages/schemas/src/types/saml-application.ts b/packages/schemas/src/types/saml-application.ts index acbf83425..dce5cf3df 100644 --- a/packages/schemas/src/types/saml-application.ts +++ b/packages/schemas/src/types/saml-application.ts @@ -19,10 +19,8 @@ export const samlApplicationCreateGuard = applicationCreateGuard description: true, customData: true, }) - .extend({ - // The reason for encapsulating attributeMapping and spMetadata into an object within the config field is that you cannot provide only one of `attributeMapping` or `spMetadata`. Due to the structure of the `saml_application_configs` table, both must be not null. - config: samlAppConfigGuard.partial().optional(), - }); + // The reason for encapsulating attributeMapping and spMetadata into an object within the config field is that you cannot provide only one of `attributeMapping` or `spMetadata`. Due to the structure of the `saml_application_configs` table, both must be not null. + .merge(samlAppConfigGuard.partial()); export type CreateSamlApplication = z.infer; @@ -32,27 +30,23 @@ export const samlApplicationPatchGuard = applicationPatchGuard description: true, customData: true, }) - .extend({ - // The reason for encapsulating attributeMapping and spMetadata into an object within the config field is that you cannot provide only one of `attributeMapping` or `spMetadata`. Due to the structure of the `saml_application_configs` table, both must be not null. - config: samlAppConfigGuard.partial().optional(), - }); + // The reason for encapsulating attributeMapping and spMetadata into an object within the config field is that you cannot provide only one of `attributeMapping` or `spMetadata`. Due to the structure of the `saml_application_configs` table, both must be not null. + .merge(samlAppConfigGuard.partial()); export type PatchSamlApplication = z.infer; -// Make sure the `privateKey` is not exposed in the response. -export const samlApplicationSecretResponseGuard = SamlApplicationSecrets.guard.omit({ - tenantId: true, - applicationId: true, - privateKey: true, -}); - -export type SamlApplicationSecretResponse = z.infer; - -export const samlApplicationResponseGuard = Applications.guard.merge( - // Partial to allow the optional fields to be omitted in the response. - // When starting to create a SAML application, SAML configuration is optional, which can lead to the absence of SAML configuration. - samlAppConfigGuard -); +export const samlApplicationResponseGuard = Applications.guard + .omit({ + secret: true, + oidcClientMetadata: true, + customClientMetadata: true, + protectedAppMetadata: true, + }) + .merge( + // Partial to allow the optional fields to be omitted in the response. + // When starting to create a SAML application, SAML configuration is optional, which can lead to the absence of SAML configuration. + samlAppConfigGuard + ); export type SamlApplicationResponse = z.infer; @@ -73,3 +67,16 @@ export type CertificateFingerprints = { export const certificateFingerprintsGuard = z.object({ sha256: fingerprintFormatGuard, }) satisfies ToZodObject; + +// Make sure the `privateKey` is not exposed in the response. +export const samlApplicationSecretResponseGuard = SamlApplicationSecrets.guard + .omit({ + tenantId: true, + applicationId: true, + privateKey: true, + }) + .extend({ + fingerprints: certificateFingerprintsGuard, + }); + +export type SamlApplicationSecretResponse = z.infer;