Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Sep 2, 2025
+203 −18
Merged

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

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
b373dff
Fix OIDC configuration update
afitzek Sep 1, 2025
f7493f4
Fix test case
afitzek Sep 1, 2025
67b043d
Fix tests
afitzek Sep 2, 2025
251645f
Add test case for error reporting
afitzek Sep 2, 2025
f191a03
Fix testing
afitzek Sep 2, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 27 additions & 5 deletions packages/cli/src/sso.ee/oidc/oidc.service.ee.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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,
Expand All @@ -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);
}
Expand All @@ -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,
Expand Down
92 changes: 91 additions & 1 deletion packages/cli/test/integration/oidc/oidc.service.ee.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down Expand Up @@ -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 () => {
Expand All @@ -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();
Expand All @@ -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');
Expand Down
5 changes: 5 additions & 0 deletions packages/frontend/editor-ui/src/views/SettingsSso.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
31 changes: 19 additions & 12 deletions packages/frontend/editor-ui/src/views/SettingsSso.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having finally and a return above in the catch block looks a bit weird to me.

If you want to execute getOidcConfig always just put it after the try-catch block (without finally) or if you want to run it only after success then put it into the try block.

Also If getOidcConfig rejects, you lose the original save error and instead see only the error from getOidcConfig.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I basically followed the SAML example the same way, so that UI behaves the same. ;)
The return is not really necessary. I think the idea is that the UI always shows the state that is currently in the db. I don't think that if getOidcConfig rejects one would lose the original save error, since it is at that point already displayed to the user via toast right?

n8n/packages/frontend/editor-ui/src/views/SettingsSso.vue

Line 179 in 238fe84

} catch (error) {

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's true, it'd only be a problem if we had onOidcSettingsSave executed in another async function.
And actually finally runs despite of the return, it just looks weird, never seen this before

await getOidcConfig();
}
}
</script>

Expand Down
Loading