fix(core): Fix OIDC configuration update path (#19065)

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<client.Configuration>;
 				validTill: Date;
-		  }
+		  } & OidcRuntimeConfig)
 		| undefined;
 
 	private async getOidcConfiguration(): Promise<client.Configuration> {
 		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,

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,86 @@ 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.mockReset();
+			discoveryMock.mockClear();
+			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 +237,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');

packages/frontend/@n8n/i18n/src/locales/en.json

@@ -3000,6 +3000,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.",

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
@@ -382,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, 'Error saving OIDC SSO configuration');
+		});
 	});
 
 	describe('Protocol Selection Persistence', () => {

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({
+	try {
-		clientId: clientId.value,
+		const newConfig = await ssoStore.saveOidcConfig({
-		clientSecret: clientSecret.value,
+			clientId: clientId.value,
-		discoveryEndpoint: discoveryEndpoint.value,
+			clientSecret: clientSecret.value,
-		loginEnabled: ssoStore.isOidcLoginEnabled,
+			discoveryEndpoint: discoveryEndpoint.value,
-	});
+			loginEnabled: ssoStore.isOidcLoginEnabled,
+		});
 
-	// Update store with saved protocol selection
+		// Update store with saved protocol selection
-	ssoStore.selectedAuthProtocol = authProtocol.value;
+		ssoStore.selectedAuthProtocol = authProtocol.value;
 
-	clientSecret.value = newConfig.clientSecret;
+		clientSecret.value = newConfig.clientSecret;
-	trackUpdateSettings();
+		trackUpdateSettings();
+	} catch (error) {
+		toast.showError(error, i18n.baseText('settings.sso.settings.save.error_oidc'));
+		return;
+	} finally {
+		await getOidcConfig();
+	}
 }
 </script>