fix: fix some SAML app bugs (#6852)

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<SamlApplicationSecret> => {
+  /**
+   * @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<SamlApplicationSecret> => {
     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<SamlApplicationResponse> => {
@@ -71,8 +84,9 @@ export const createSamlApplicationsLibrary = (queries: Queries) => {
     id: string,
     patchApplicationObject: PatchSamlApplication
   ): Promise<SamlApplicationResponse> => {
-    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: [
         {

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}$/);
 

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

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<OmitAutoSetFields<CreateSamlApplicationSecret>, 'active'>
+  ) => {
+    return buildInsertIntoWithPool(pool)(SamlApplicationSecrets, {
+      returning: true,
+    })({ ...data, active: false });
+  };
+
+  const insertActiveSamlApplicationSecret = async (
+    data: Omit<OmitAutoSetFields<CreateSamlApplicationSecret>, '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<SamlApplicationSecret>(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<SamlApplicationSecret>(sql`
@@ -83,7 +123,8 @@ export const createSamlApplicationSecretsQueries = (pool: CommonQueryMethods) =>
   };
 
   return {
-    insertSamlApplicationSecret,
+    insertInactiveSamlApplicationSecret,
+    insertActiveSamlApplicationSecret,
     findSamlApplicationSecretsByApplicationId,
     findActiveSamlApplicationSecretByApplicationId,
     findSamlApplicationSecretByApplicationIdAndId,

packages/core/src/saml-applications/routes/anonymous.ts

@@ -14,7 +14,7 @@ export default function samlApplicationAnonymousRoutes<T extends AnonymousRouter
     '/saml-applications/:id/metadata',
     koaGuard({
       params: z.object({ id: z.string() }),
-      status: [200, 400, 404],
+      status: [200, 404],
       response: z.string(),
     }),
     async (ctx, next) => {

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<T extends ManagementApiRouter>(
       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<T extends ManagementApiRouter>(
             applicationId: application.id,
             ...config,
           }),
-          createSamlApplicationSecret(application.id),
+          createSamlApplicationSecret({ applicationId: application.id, isActive: true }),
         ]);
 
         ctx.status = 201;
@@ -176,8 +175,12 @@ export default function samlApplicationRoutes<T extends ManagementApiRouter>(
         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<T extends ManagementApiRouter>(
       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<T extends ManagementApiRouter>(
         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<T extends ManagementApiRouter>(
     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<T extends ManagementApiRouter>(
     '/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) => {

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',

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

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<typeof samlApplicationCreateGuard>;
 
@@ -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<typeof samlApplicationPatchGuard>;
 
-// 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<typeof samlApplicationSecretResponseGuard>;
-
-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<typeof samlApplicationResponseGuard>;
 
@@ -73,3 +67,16 @@ export type CertificateFingerprints = {
 export const certificateFingerprintsGuard = z.object({
   sha256: fingerprintFormatGuard,
 }) satisfies ToZodObject<CertificateFingerprints>;
+
+// 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<typeof samlApplicationSecretResponseGuard>;