feat(core): add default redirect URI for SAML apps on creation (#6932)

packages/core/src/saml-applications/SamlApplication/index.test.ts

@@ -1,4 +1,4 @@
-import { UserScope } from '@logto/core-kit';
+import { UserScope, ReservedScope } from '@logto/core-kit';
 import { NameIdFormat } from '@logto/schemas';
 import nock from 'nock';
 
@@ -173,7 +173,7 @@ describe('SamlApplication', () => {
   });
 
   describe('getScopesFromAttributeMapping', () => {
-    it('should include email scope when nameIdFormat is EmailAddress', () => {
+    it('should include default scopes and email scope when nameIdFormat is EmailAddress', () => {
       const app = new TestSamlApplication(
         // @ts-expect-error
         {
@@ -188,14 +188,16 @@ describe('SamlApplication', () => {
 
       const scopes = app.exposedGetScopesFromAttributeMapping();
       expect(scopes).toContain(UserScope.Email);
+      expect(scopes).toContain(ReservedScope.OpenId);
+      expect(scopes).toContain(UserScope.Profile);
+      expect(scopes).toHaveLength(3);
     });
 
-    it('should return only nameIdFormat related scope when attributeMapping is empty', () => {
+    it('should include default scopes when attributeMapping is empty', () => {
       const app = new TestSamlApplication(
         // @ts-expect-error
         {
           ...mockDetails,
-          nameIdFormat: NameIdFormat.EmailAddress,
           attributeMapping: {},
         },
         mockSamlApplicationId,
@@ -204,8 +206,9 @@ describe('SamlApplication', () => {
       );
 
       const scopes = app.exposedGetScopesFromAttributeMapping();
-      expect(scopes).toHaveLength(1);
+      expect(scopes).toHaveLength(2);
-      expect(scopes).toEqual([UserScope.Email]);
+      expect(scopes).toContain(ReservedScope.OpenId);
+      expect(scopes).toContain(UserScope.Profile);
     });
 
     it('should return correct scopes based on attributeMapping', () => {
@@ -214,9 +217,8 @@ describe('SamlApplication', () => {
         {
           ...mockDetails,
           attributeMapping: {
-            name: 'name',
             email: 'email',
-            custom_data: 'customData',
+            name: 'name',
           },
         },
         mockSamlApplicationId,
@@ -225,9 +227,10 @@ describe('SamlApplication', () => {
       );
 
       const scopes = app.exposedGetScopesFromAttributeMapping();
-      expect(scopes).toContain(UserScope.Profile); // For 'name'
+      expect(scopes).toContain(ReservedScope.OpenId);
-      expect(scopes).toContain(UserScope.Email); // For 'email'
+      expect(scopes).toContain(UserScope.Profile);
-      expect(scopes).toContain(UserScope.CustomData); // For 'custom_data'
+      expect(scopes).toContain(UserScope.Email);
+      expect(scopes).toHaveLength(3);
     });
 
     it('should ignore id claim in attributeMapping', () => {
@@ -236,7 +239,7 @@ describe('SamlApplication', () => {
         {
           ...mockDetails,
           attributeMapping: {
-            id: 'userId',
+            id: 'id',
             name: 'name',
           },
         },
@@ -246,8 +249,9 @@ describe('SamlApplication', () => {
       );
 
       const scopes = app.exposedGetScopesFromAttributeMapping();
-      expect(scopes).toHaveLength(1);
+      expect(scopes).toContain(ReservedScope.OpenId);
-      expect(scopes).toContain(UserScope.Profile); // Only for 'name'
+      expect(scopes).toContain(UserScope.Profile);
+      expect(scopes).toHaveLength(2);
     });
 
     it('should deduplicate scopes when multiple claims map to the same scope', () => {
@@ -257,8 +261,13 @@ describe('SamlApplication', () => {
           ...mockDetails,
           attributeMapping: {
             name: 'name',
-            given_name: 'givenName',
+            nickname: 'nickname',
-            family_name: 'familyName',
+            preferred_username: 'preferred_username',
+            custom_data: 'custom_data',
+            organization_data: 'organization_data',
+            sso_identities: 'sso_identities',
+            phone_number: 'phone_number',
+            roles: 'roles',
           },
         },
         mockSamlApplicationId,
@@ -267,8 +276,14 @@ describe('SamlApplication', () => {
       );
 
       const scopes = app.exposedGetScopesFromAttributeMapping();
-      expect(scopes).toHaveLength(1);
+      expect(scopes).toContain(ReservedScope.OpenId);
-      expect(scopes).toContain(UserScope.Profile); // All claims map to 'profile' scope
+      expect(scopes).toContain(UserScope.Profile);
+      expect(scopes).toContain(UserScope.Organizations);
+      expect(scopes).toContain(UserScope.Identities);
+      expect(scopes).toContain(UserScope.CustomData);
+      expect(scopes).toContain(UserScope.Phone);
+      expect(scopes).toContain(UserScope.Roles);
+      expect(scopes).toHaveLength(7);
     });
   });
 });

packages/core/src/saml-applications/SamlApplication/index.ts

@@ -1,8 +1,8 @@
 /* eslint-disable max-lines */
 // TODO: refactor this file to reduce LOC
 import { parseJson } from '@logto/connector-kit';
-import { userClaims, type UserClaim, UserScope } from '@logto/core-kit';
+import { userClaims, type UserClaim, UserScope, ReservedScope } from '@logto/core-kit';
-import { Prompt, QueryKey, ReservedScope } from '@logto/js';
+import { Prompt, QueryKey } from '@logto/js';
 import {
   type SamlAcsUrl,
   BindingType,
@@ -11,7 +11,7 @@ import {
   type SamlAttributeMapping,
 } from '@logto/schemas';
 import { generateStandardId } from '@logto/shared';
-import { tryThat, appendPath, deduplicate, type Nullable, cond } from '@silverhand/essentials';
+import { tryThat, type Nullable, cond } from '@silverhand/essentials';
 import camelcaseKeys, { type CamelCaseKeys } from 'camelcase-keys';
 import { XMLValidator } from 'fast-xml-parser';
 import saml from 'samlify';
@@ -39,7 +39,7 @@ import {
 import { buildSingleSignOnUrl, buildSamlIdentityProviderEntityId } from '../libraries/utils.js';
 import { type SamlApplicationDetails } from '../queries/index.js';
 
-import { buildSamlAssertionNameId } from './utils.js';
+import { buildSamlAssertionNameId, getSamlAppCallbackUrl } from './utils.js';
 
 type ValidSamlApplicationDetails = {
   secret: string;
@@ -237,10 +237,7 @@ export class SamlApplication {
   }
 
   public get samlAppCallbackUrl() {
-    return appendPath(
+    return getSamlAppCallbackUrl(this.tenantEndpoint, this.samlApplicationId).toString();
-      this.tenantEndpoint,
-      `api/saml-applications/${this.samlApplicationId}/callback`
-    ).toString();
   }
 
   public async parseLoginRequest(
@@ -295,7 +292,7 @@ export class SamlApplication {
     queryParameters.append(
       QueryKey.Scope,
       // For security reasons, DO NOT include the offline_access scope by default.
-      deduplicate([ReservedScope.OpenId, ...this.getScopesFromAttributeMapping()]).join(' ')
+      this.getScopesFromAttributeMapping().join(' ')
     );
 
     if (state) {
@@ -372,8 +369,13 @@ export class SamlApplication {
   };
 
   // Get required scopes based on attribute mapping configuration
-  protected getScopesFromAttributeMapping = (): UserScope[] => {
+  protected getScopesFromAttributeMapping = (): Array<UserScope | ReservedScope> => {
-    const requiredScopes = new Set<UserScope>();
+    const requiredScopes = new Set<UserScope | ReservedScope>();
+
+    // Add default scopes.
+    requiredScopes.add(ReservedScope.OpenId);
+    requiredScopes.add(UserScope.Profile);
+
     if (this.details.nameIdFormat === NameIdFormat.EmailAddress) {
       requiredScopes.add(UserScope.Email);
     }

packages/core/src/saml-applications/SamlApplication/utils.ts

@@ -1,5 +1,6 @@
 import { NameIdFormat } from '@logto/schemas';
 import { generateStandardId } from '@logto/shared';
+import { appendPath } from '@silverhand/essentials';
 
 import RequestError from '#src/errors/RequestError/index.js';
 import { type IdTokenProfileStandardClaims } from '#src/sso/types/oidc.js';
@@ -68,3 +69,6 @@ export const generateAutoSubmitForm = (actionUrl: string, samlResponse: string):
     </html>
   `;
 };
+
+export const getSamlAppCallbackUrl = (baseUrl: URL, samlAppId: string) =>
+  appendPath(baseUrl, `api/saml-applications/${samlAppId}/callback`);

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

@@ -10,6 +10,7 @@ import { generateStandardId } from '@logto/shared';
 import { removeUndefinedKeys } from '@silverhand/essentials';
 import { 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 { buildOidcClientMetadata } from '#src/oidc/utils.js';
@@ -17,6 +18,8 @@ import { generateInternalSecret } from '#src/routes/applications/application-sec
 import type { ManagementApiRouter, RouterInitArgs } from '#src/routes/types.js';
 import assertThat from '#src/utils/assert-that.js';
 
+import { getTenantEndpoint } from '../../env-set/utils.js';
+import { getSamlAppCallbackUrl } from '../SamlApplication/utils.js';
 import {
   calculateCertificateFingerprints,
   ensembleSamlApplication,
@@ -24,7 +27,7 @@ import {
 } from '../libraries/utils.js';
 
 export default function samlApplicationRoutes<T extends ManagementApiRouter>(
-  ...[router, { queries, libraries }]: RouterInitArgs<T>
+  ...[router, { id: tenantId, queries, libraries }]: RouterInitArgs<T>
 ) {
   const {
     applications: { insertApplication, findApplicationById, deleteApplicationById },
@@ -58,14 +61,24 @@ export default function samlApplicationRoutes<T extends ManagementApiRouter>(
         validateAcsUrl(config.acsUrl);
       }
 
+      const id = generateStandardId();
+      // Set the default redirect URI for SAML apps when creating a new SAML app.
+      const redirectUri = getSamlAppCallbackUrl(
+        getTenantEndpoint(tenantId, EnvSet.values),
+        id
+      ).toString();
+
       const application = await insertApplication(
         removeUndefinedKeys({
-          id: generateStandardId(),
+          id,
           secret: generateInternalSecret(),
           name,
           description,
           customData,
-          oidcClientMetadata: buildOidcClientMetadata(),
+          oidcClientMetadata: {
+            ...buildOidcClientMetadata(),
+            redirectUris: [redirectUri],
+          },
           type: ApplicationType.SAML,
         })
       );

packages/integration-tests/src/tests/api/application/saml-application.test.ts

@@ -1,7 +1,12 @@
 import { ApplicationType, BindingType, NameIdFormat } from '@logto/schemas';
 import { conditional } from '@silverhand/essentials';
 
-import { createApplication, deleteApplication, updateApplication } from '#src/api/application.js';
+import {
+  createApplication,
+  deleteApplication,
+  getApplications,
+  updateApplication,
+} from '#src/api/application.js';
 import {
   createSamlApplication,
   deleteSamlApplication,
@@ -27,6 +32,20 @@ describe('SAML application', () => {
 
     expect(createdSamlApplication.nameIdFormat).toBe(NameIdFormat.Persistent);
 
+    // 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]);
+    const pickedSamlApplication = samlApplications.find(
+      ({ 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);
+
     await deleteSamlApplication(createdSamlApplication.id);
   });