From b373dff2de833b5b657811a9c7fdd37ea2041b07 Mon Sep 17 00:00:00 2001 From: Andreas Fitzek Date: Mon, 1 Sep 2025 17:25:35 +0200 Subject: [PATCH 1/5] Fix OIDC configuration update --- .../cli/src/sso.ee/oidc/oidc.service.ee.ts | 32 +++++-- .../integration/oidc/oidc.service.ee.test.ts | 90 ++++++++++++++++++- .../editor-ui/src/views/SettingsSso.vue | 31 ++++--- 3 files changed, 135 insertions(+), 18 deletions(-) diff --git a/packages/cli/src/sso.ee/oidc/oidc.service.ee.ts b/packages/cli/src/sso.ee/oidc/oidc.service.ee.ts index e716c520565e3..8f43015bec5fc 100644 --- a/packages/cli/src/sso.ee/oidc/oidc.service.ee.ts +++ b/packages/cli/src/sso.ee/oidc/oidc.service.ee.ts @@ -12,7 +12,7 @@ import { } from '@n8n/db'; import { Container, Service } from '@n8n/di'; import { Cipher } from 'n8n-core'; -import { jsonParse } from 'n8n-workflow'; +import { jsonParse, UserError } from 'n8n-workflow'; import * as client from 'openid-client'; import { BadRequestError } from '@/errors/response-errors/bad-request.error'; @@ -212,11 +212,24 @@ export class OidcService { // Validating that discoveryEndpoint is a valid URL discoveryEndpoint = new URL(newConfig.discoveryEndpoint); } catch (error) { - throw new BadRequestError('Provided discovery endpoint is not a valid URL'); + this.logger.error(`The provided endpoint is not a valid URL: ${newConfig.discoveryEndpoint}`); + throw new UserError('Provided discovery endpoint is not a valid URL'); } if (newConfig.clientSecret === OIDC_CLIENT_SECRET_REDACTED_VALUE) { newConfig.clientSecret = this.oidcConfig.clientSecret; } + try { + const discoveredMetadata = await client.discovery( + discoveryEndpoint, + newConfig.clientId, + newConfig.clientSecret, + ); + // TODO: validate Metadata against features + this.logger.debug(`Discovered OIDC metadata: ${JSON.stringify(discoveredMetadata)}`); + } catch (error) { + this.logger.error('Failed to discover OIDC metadata', { error }); + throw new UserError('Failed to discover OIDC metadata, based on the provided configuration'); + } await this.settingsRepository.update( { key: OIDC_PREFERENCES_DB_KEY, @@ -238,6 +251,10 @@ export class OidcService { ...newConfig, discoveryEndpoint, }; + this.cachedOidcConfiguration = undefined; // reset cached configuration + this.logger.debug( + `OIDC login is now ${this.oidcConfig.loginEnabled ? 'enabled' : 'disabled'}.`, + ); await this.setOidcLoginEnabled(this.oidcConfig.loginEnabled); } @@ -259,19 +276,24 @@ export class OidcService { } private cachedOidcConfiguration: - | { + | ({ configuration: Promise; validTill: Date; - } + } & OidcRuntimeConfig) | undefined; private async getOidcConfiguration(): Promise { const now = Date.now(); if ( this.cachedOidcConfiguration === undefined || - now >= this.cachedOidcConfiguration.validTill.getTime() + now >= this.cachedOidcConfiguration.validTill.getTime() || + this.oidcConfig.discoveryEndpoint.toString() !== + this.cachedOidcConfiguration.discoveryEndpoint.toString() || + this.oidcConfig.clientId !== this.cachedOidcConfiguration.clientId || + this.oidcConfig.clientSecret !== this.cachedOidcConfiguration.clientSecret ) { this.cachedOidcConfiguration = { + ...this.oidcConfig, configuration: client.discovery( this.oidcConfig.discoveryEndpoint, this.oidcConfig.clientId, diff --git a/packages/cli/test/integration/oidc/oidc.service.ee.test.ts b/packages/cli/test/integration/oidc/oidc.service.ee.test.ts index ad8440e04d6cc..82ec3782a0c07 100644 --- a/packages/cli/test/integration/oidc/oidc.service.ee.test.ts +++ b/packages/cli/test/integration/oidc/oidc.service.ee.test.ts @@ -21,6 +21,7 @@ import { ForbiddenError } from '@/errors/response-errors/forbidden.error'; import { OIDC_CLIENT_SECRET_REDACTED_VALUE } from '@/sso.ee/oidc/constants'; import { OidcService } from '@/sso.ee/oidc/oidc.service.ee'; import { createUser } from '@test-integration/db/users'; +import { UserError } from 'n8n-workflow'; beforeAll(async () => { await testDb.init(); @@ -113,7 +114,7 @@ describe('OIDC service', () => { loginEnabled: true, }; - await expect(oidcService.updateConfig(newConfig)).rejects.toThrowError(BadRequestError); + await expect(oidcService.updateConfig(newConfig)).rejects.toThrowError(UserError); }); it('should keep current secret if redact value is given in update', async () => { @@ -136,6 +137,84 @@ describe('OIDC service', () => { ); expect(loadedConfig.loginEnabled).toBe(true); }); + + it('should throw UserError when OIDC discovery fails during updateConfig', async () => { + const newConfig: OidcConfigDto = { + clientId: 'test-client-id', + clientSecret: 'test-client-secret', + discoveryEndpoint: 'https://example.com/.well-known/openid-configuration', + loginEnabled: true, + }; + + discoveryMock.mockRejectedValueOnce(new Error('Discovery failed')); + + await expect(oidcService.updateConfig(newConfig)).rejects.toThrowError(UserError); + expect(discoveryMock).toHaveBeenCalledWith( + expect.any(URL), + 'test-client-id', + 'test-client-secret', + ); + }); + + it('should invalidate cached configuration when updateConfig is called', async () => { + // First, set up a working configuration + const initialConfig: OidcConfigDto = { + clientId: 'initial-client-id', + clientSecret: 'initial-client-secret', + discoveryEndpoint: 'https://example.com/.well-known/openid-configuration', + loginEnabled: true, + }; + + const mockConfiguration = new real_odic_client.Configuration( + { + issuer: 'https://example.com/auth/realms/n8n', + client_id: 'initial-client-id', + redirect_uris: ['http://n8n.io/sso/oidc/callback'], + response_types: ['code'], + scopes: ['openid', 'profile', 'email'], + authorization_endpoint: 'https://example.com/auth', + }, + 'initial-client-id', + ); + + discoveryMock.mockResolvedValue(mockConfiguration); + await oidcService.updateConfig(initialConfig); + + // Generate a login URL to populate the cache + await oidcService.generateLoginUrl(); + expect(discoveryMock).toHaveBeenCalledTimes(2); // Once in updateConfig, once in generateLoginUrl + + // Update config with new values + const newConfig: OidcConfigDto = { + clientId: 'new-client-id', + clientSecret: 'new-client-secret', + discoveryEndpoint: 'https://newprovider.example.com/.well-known/openid-configuration', + loginEnabled: true, + }; + + const newMockConfiguration = new real_odic_client.Configuration( + { + issuer: 'https://newprovider.example.com/auth/realms/n8n', + client_id: 'new-client-id', + redirect_uris: ['http://n8n.io/sso/oidc/callback'], + response_types: ['code'], + scopes: ['openid', 'profile', 'email'], + authorization_endpoint: 'https://newprovider.example.com/auth', + }, + 'new-client-id', + ); + + discoveryMock.mockResolvedValue(newMockConfiguration); + await oidcService.updateConfig(newConfig); + + // Generate login URL again - should use new configuration + const authUrl = await oidcService.generateLoginUrl(); + expect(authUrl.pathname).toEqual('/auth'); + expect(authUrl.searchParams.get('client_id')).toEqual('new-client-id'); + + // Verify discovery was called again due to cache invalidation + expect(discoveryMock).toHaveBeenCalledTimes(4); // Initial config, initial login, new config, new login + }); }); it('should generate a valid callback URL', () => { const callbackUrl = oidcService.getCallbackUrl(); @@ -156,6 +235,15 @@ describe('OIDC service', () => { ); discoveryMock.mockResolvedValue(mockConfiguration); + const initialConfig: OidcConfigDto = { + clientId: 'test-client-id', + clientSecret: 'test-client-secret', + discoveryEndpoint: 'https://example.com/.well-known/openid-configuration', + loginEnabled: true, + }; + + await oidcService.updateConfig(initialConfig); + const authUrl = await oidcService.generateLoginUrl(); expect(authUrl.pathname).toEqual('/auth'); diff --git a/packages/frontend/editor-ui/src/views/SettingsSso.vue b/packages/frontend/editor-ui/src/views/SettingsSso.vue index 3dbc494564e8d..a23ade15d31b8 100644 --- a/packages/frontend/editor-ui/src/views/SettingsSso.vue +++ b/packages/frontend/editor-ui/src/views/SettingsSso.vue @@ -284,18 +284,25 @@ async function onOidcSettingsSave() { if (confirmAction !== MODAL_CONFIRM) return; } - const newConfig = await ssoStore.saveOidcConfig({ - clientId: clientId.value, - clientSecret: clientSecret.value, - discoveryEndpoint: discoveryEndpoint.value, - loginEnabled: ssoStore.isOidcLoginEnabled, - }); - - // Update store with saved protocol selection - ssoStore.selectedAuthProtocol = authProtocol.value; - - clientSecret.value = newConfig.clientSecret; - trackUpdateSettings(); + try { + const newConfig = await ssoStore.saveOidcConfig({ + clientId: clientId.value, + clientSecret: clientSecret.value, + discoveryEndpoint: discoveryEndpoint.value, + loginEnabled: ssoStore.isOidcLoginEnabled, + }); + + // Update store with saved protocol selection + ssoStore.selectedAuthProtocol = authProtocol.value; + + clientSecret.value = newConfig.clientSecret; + trackUpdateSettings(); + } catch (error) { + toast.showError(error, i18n.baseText('settings.sso.settings.save.error')); + return; + } finally { + await getOidcConfig(); + } } From f7493f4707fa87fa1b73af0cef59cd5e2368ded7 Mon Sep 17 00:00:00 2001 From: Andreas Fitzek Date: Mon, 1 Sep 2025 17:49:35 +0200 Subject: [PATCH 2/5] Fix test case --- packages/cli/test/integration/oidc/oidc.service.ee.test.ts | 2 ++ 1 file changed, 2 insertions(+) diff --git a/packages/cli/test/integration/oidc/oidc.service.ee.test.ts b/packages/cli/test/integration/oidc/oidc.service.ee.test.ts index 82ec3782a0c07..af5731ed04f15 100644 --- a/packages/cli/test/integration/oidc/oidc.service.ee.test.ts +++ b/packages/cli/test/integration/oidc/oidc.service.ee.test.ts @@ -177,6 +177,8 @@ describe('OIDC service', () => { 'initial-client-id', ); + discoveryMock.mockReset(); + discoveryMock.mockClear(); discoveryMock.mockResolvedValue(mockConfiguration); await oidcService.updateConfig(initialConfig); From 67b043d5766fcf839bdfe8d59aabd6852feb347f Mon Sep 17 00:00:00 2001 From: Andreas Fitzek Date: Tue, 2 Sep 2025 10:19:10 +0200 Subject: [PATCH 3/5] Fix tests --- packages/frontend/editor-ui/src/views/SettingsSso.test.ts | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/packages/frontend/editor-ui/src/views/SettingsSso.test.ts b/packages/frontend/editor-ui/src/views/SettingsSso.test.ts index 6c6d85cdd3337..b17dc84b75aeb 100644 --- a/packages/frontend/editor-ui/src/views/SettingsSso.test.ts +++ b/packages/frontend/editor-ui/src/views/SettingsSso.test.ts @@ -331,6 +331,11 @@ describe('SettingsSso View', () => { ssoStore.isOidcLoginEnabled = true; ssoStore.isSamlLoginEnabled = false; + ssoStore.getOidcConfig.mockResolvedValue({ + ...oidcConfig, + discoveryEndpoint: '', + }); + const { getByTestId, getByRole } = renderView(); // Set authProtocol component ref to OIDC From 251645f9e129e0758f505768a134a3b687543302 Mon Sep 17 00:00:00 2001 From: Andreas Fitzek Date: Tue, 2 Sep 2025 11:11:51 +0200 Subject: [PATCH 4/5] Add test case for error reporting --- .../editor-ui/src/views/SettingsSso.test.ts | 60 +++++++++++++++++++ .../editor-ui/src/views/SettingsSso.vue | 2 +- 2 files changed, 61 insertions(+), 1 deletion(-) diff --git a/packages/frontend/editor-ui/src/views/SettingsSso.test.ts b/packages/frontend/editor-ui/src/views/SettingsSso.test.ts index b17dc84b75aeb..5b06504fc23b0 100644 --- a/packages/frontend/editor-ui/src/views/SettingsSso.test.ts +++ b/packages/frontend/editor-ui/src/views/SettingsSso.test.ts @@ -387,6 +387,66 @@ describe('SettingsSso View', () => { }), ); }); + + it('shows error message to user when OIDC config save fails', async () => { + const error = new Error('Save failed'); + ssoStore.saveOidcConfig.mockRejectedValue(error); + ssoStore.isEnterpriseOidcEnabled = true; + ssoStore.isEnterpriseSamlEnabled = false; + ssoStore.isOidcLoginEnabled = true; + ssoStore.isSamlLoginEnabled = false; + + ssoStore.getOidcConfig.mockResolvedValue({ + ...oidcConfig, + discoveryEndpoint: '', + }); + + const { getByTestId, getByRole } = renderView(); + showError.mockClear(); + + // Set authProtocol component ref to OIDC + const protocolSelect = getByRole('combobox'); + expect(protocolSelect).toBeInTheDocument(); + await userEvent.click(protocolSelect); + + const dropdown = await waitFor(() => getByRole('listbox')); + expect(dropdown).toBeInTheDocument(); + const items = dropdown.querySelectorAll('.el-select-dropdown__item'); + const oidcItem = Array.from(items).find((item) => item.textContent?.includes('OIDC')); + expect(oidcItem).toBeDefined(); + + await userEvent.click(oidcItem!); + + const saveButton = await waitFor(() => getByTestId('sso-oidc-save')); + expect(saveButton).toBeVisible(); + + const oidcDiscoveryUrlInput = getByTestId('oidc-discovery-endpoint'); + + expect(oidcDiscoveryUrlInput).toBeVisible(); + await userEvent.type(oidcDiscoveryUrlInput, oidcConfig.discoveryEndpoint); + + const clientIdInput = getByTestId('oidc-client-id'); + expect(clientIdInput).toBeVisible(); + await userEvent.type(clientIdInput, 'test-client-id'); + const clientSecretInput = getByTestId('oidc-client-secret'); + expect(clientSecretInput).toBeVisible(); + await userEvent.type(clientSecretInput, 'test-client-secret'); + + expect(saveButton).not.toBeDisabled(); + await userEvent.click(saveButton); + + expect(ssoStore.saveOidcConfig).toHaveBeenCalledWith( + expect.objectContaining({ + discoveryEndpoint: oidcConfig.discoveryEndpoint, + clientId: 'test-client-id', + clientSecret: 'test-client-secret', + loginEnabled: true, + }), + ); + + expect(telemetryTrack).not.toBeCalled(); + expect(showError).toHaveBeenCalledWith(error, 'settings.sso.settings.save.error_oidc'); + }); }); describe('Protocol Selection Persistence', () => { diff --git a/packages/frontend/editor-ui/src/views/SettingsSso.vue b/packages/frontend/editor-ui/src/views/SettingsSso.vue index a23ade15d31b8..3e73e6ee0c190 100644 --- a/packages/frontend/editor-ui/src/views/SettingsSso.vue +++ b/packages/frontend/editor-ui/src/views/SettingsSso.vue @@ -298,7 +298,7 @@ async function onOidcSettingsSave() { clientSecret.value = newConfig.clientSecret; trackUpdateSettings(); } catch (error) { - toast.showError(error, i18n.baseText('settings.sso.settings.save.error')); + toast.showError(error, i18n.baseText('settings.sso.settings.save.error_oidc')); return; } finally { await getOidcConfig(); From f191a037f3ef90e61df2734338bd7ded0f5e0ca4 Mon Sep 17 00:00:00 2001 From: Andreas Fitzek Date: Tue, 2 Sep 2025 11:29:06 +0200 Subject: [PATCH 5/5] Fix testing --- packages/frontend/@n8n/i18n/src/locales/en.json | 1 + packages/frontend/editor-ui/src/views/SettingsSso.test.ts | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/packages/frontend/@n8n/i18n/src/locales/en.json b/packages/frontend/@n8n/i18n/src/locales/en.json index bd1b30ecd6d24..5e8594a35ba3e 100644 --- a/packages/frontend/@n8n/i18n/src/locales/en.json +++ b/packages/frontend/@n8n/i18n/src/locales/en.json @@ -2996,6 +2996,7 @@ "settings.sso.settings.save.activate.cancel": "Cancel", "settings.sso.settings.save.activate.test": "Test settings", "settings.sso.settings.save.error": "Error saving SAML SSO configuration", + "settings.sso.settings.save.error_oidc": "Error saving OIDC SSO configuration", "settings.sso.settings.footer.hint": "Don't forget to activate SAML SSO once you've saved the settings.", "settings.sso.actionBox.title": "Available on the Enterprise plan", "settings.sso.actionBox.description": "Use Single Sign On to consolidate authentication into a single platform to improve security and agility.", diff --git a/packages/frontend/editor-ui/src/views/SettingsSso.test.ts b/packages/frontend/editor-ui/src/views/SettingsSso.test.ts index 5b06504fc23b0..2afd8cc62745c 100644 --- a/packages/frontend/editor-ui/src/views/SettingsSso.test.ts +++ b/packages/frontend/editor-ui/src/views/SettingsSso.test.ts @@ -445,7 +445,7 @@ describe('SettingsSso View', () => { ); expect(telemetryTrack).not.toBeCalled(); - expect(showError).toHaveBeenCalledWith(error, 'settings.sso.settings.save.error_oidc'); + expect(showError).toHaveBeenCalledWith(error, 'Error saving OIDC SSO configuration'); }); });