From d16666a4713f33b95b559b03b496c76bce94cda5 Mon Sep 17 00:00:00 2001 From: Darcy Ye Date: Fri, 10 Jan 2025 16:09:40 +0800 Subject: [PATCH] feat(core): add default redirect URI for SAML apps on creation (#6932) --- .../SamlApplication/index.test.ts | 51 ++++++++++++------- .../SamlApplication/index.ts | 24 +++++---- .../SamlApplication/utils.ts | 4 ++ .../src/saml-applications/routes/index.ts | 19 +++++-- .../api/application/saml-application.test.ts | 21 +++++++- 5 files changed, 86 insertions(+), 33 deletions(-) diff --git a/packages/core/src/saml-applications/SamlApplication/index.test.ts b/packages/core/src/saml-applications/SamlApplication/index.test.ts index d3fb0af6c..54eac471a 100644 --- a/packages/core/src/saml-applications/SamlApplication/index.test.ts +++ b/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).toEqual([UserScope.Email]); + expect(scopes).toHaveLength(2); + 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(UserScope.Email); // For 'email' - expect(scopes).toContain(UserScope.CustomData); // For 'custom_data' + expect(scopes).toContain(ReservedScope.OpenId); + expect(scopes).toContain(UserScope.Profile); + 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(UserScope.Profile); // Only for 'name' + expect(scopes).toContain(ReservedScope.OpenId); + 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', - family_name: 'familyName', + nickname: 'nickname', + 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(UserScope.Profile); // All claims map to 'profile' scope + expect(scopes).toContain(ReservedScope.OpenId); + 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); }); }); }); diff --git a/packages/core/src/saml-applications/SamlApplication/index.ts b/packages/core/src/saml-applications/SamlApplication/index.ts index f623ee61f..8f2f7150d 100644 --- a/packages/core/src/saml-applications/SamlApplication/index.ts +++ b/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 { Prompt, QueryKey, ReservedScope } from '@logto/js'; +import { userClaims, type UserClaim, UserScope, ReservedScope } from '@logto/core-kit'; +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( - this.tenantEndpoint, - `api/saml-applications/${this.samlApplicationId}/callback` - ).toString(); + return getSamlAppCallbackUrl(this.tenantEndpoint, this.samlApplicationId).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[] => { - const requiredScopes = new Set(); + protected getScopesFromAttributeMapping = (): Array => { + const requiredScopes = new Set(); + + // Add default scopes. + requiredScopes.add(ReservedScope.OpenId); + requiredScopes.add(UserScope.Profile); + if (this.details.nameIdFormat === NameIdFormat.EmailAddress) { requiredScopes.add(UserScope.Email); } diff --git a/packages/core/src/saml-applications/SamlApplication/utils.ts b/packages/core/src/saml-applications/SamlApplication/utils.ts index 02a2aafdc..e2976f35f 100644 --- a/packages/core/src/saml-applications/SamlApplication/utils.ts +++ b/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): `; }; + +export const getSamlAppCallbackUrl = (baseUrl: URL, samlAppId: string) => + appendPath(baseUrl, `api/saml-applications/${samlAppId}/callback`); diff --git a/packages/core/src/saml-applications/routes/index.ts b/packages/core/src/saml-applications/routes/index.ts index 446be17d9..f523583a3 100644 --- a/packages/core/src/saml-applications/routes/index.ts +++ b/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( - ...[router, { queries, libraries }]: RouterInitArgs + ...[router, { id: tenantId, queries, libraries }]: RouterInitArgs ) { const { applications: { insertApplication, findApplicationById, deleteApplicationById }, @@ -58,14 +61,24 @@ export default function samlApplicationRoutes( 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, }) ); 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 28a3848a6..14fde07f9 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 @@ -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); });