From 0827eb8eda9ee2436763d5add1154c574a7be6ec Mon Sep 17 00:00:00 2001 From: Savelii Date: Wed, 23 Sep 2026 10:16:51 +0000 Subject: [PATCH] feat(editor): Make SSO the primary option on the sign-in page when SSO is enabled (#39268) Co-authored-by: Claude Fable 5.1 --- packages/@n8n/api-types/src/constants/auth.ts | 7 + packages/@n8n/api-types/src/index.ts | 1 + .../__tests__/auth.controller.test.ts | 8 +- .../cli/src/controllers/auth.controller.ts | 8 +- .../__tests__/oidc.controller.ee.test.ts | 70 ++++ .../__tests__/oidc.service.ee.test.ts | 31 ++ .../modules/sso-oidc/oidc.controller.ee.ts | 9 +- .../src/modules/sso-oidc/oidc.service.ee.ts | 25 +- .../__tests__/validate-redirect-url.test.ts | 8 + .../cli/src/utils/validate-redirect-url.ts | 17 + .../N8nFormInputs/FormInputs.test.ts | 32 +- .../components/N8nFormInputs/FormInputs.vue | 8 +- .../frontend/@n8n/i18n/src/locales/en.json | 4 + .../src/api/__tests__/sso.test.ts | 33 ++ .../@n8n/rest-api-client/src/api/sso.ts | 3 +- .../editor-ui/src/app/constants/auth.ts | 6 + .../auth/components/SsoSigninCard.test.ts | 161 ++++++++ .../core/auth/components/SsoSigninCard.vue | 345 ++++++++++++++++++ .../features/core/auth/views/AuthView.test.ts | 34 +- .../src/features/core/auth/views/AuthView.vue | 29 +- .../core/auth/views/SigninView.test.ts | 117 ++++++ .../features/core/auth/views/SigninView.vue | 73 +++- .../settings/sso/components/SSOLogin.test.ts | 49 --- .../settings/sso/components/SSOLogin.vue | 73 ---- .../src/features/settings/sso/sso.store.ts | 21 ++ .../src/features/settings/sso/sso.test.ts | 48 +++ .../testing/playwright/pages/SignInPage.ts | 15 +- 27 files changed, 1058 insertions(+), 177 deletions(-) create mode 100644 packages/@n8n/api-types/src/constants/auth.ts create mode 100644 packages/frontend/@n8n/rest-api-client/src/api/__tests__/sso.test.ts create mode 100644 packages/frontend/editor-ui/src/features/core/auth/components/SsoSigninCard.test.ts create mode 100644 packages/frontend/editor-ui/src/features/core/auth/components/SsoSigninCard.vue delete mode 100644 packages/frontend/editor-ui/src/features/settings/sso/components/SSOLogin.test.ts delete mode 100644 packages/frontend/editor-ui/src/features/settings/sso/components/SSOLogin.vue diff --git a/packages/@n8n/api-types/src/constants/auth.ts b/packages/@n8n/api-types/src/constants/auth.ts new file mode 100644 index 00000000000..c1cc47c4ea1 --- /dev/null +++ b/packages/@n8n/api-types/src/constants/auth.ts @@ -0,0 +1,7 @@ +/** + * Error code of the `POST /login` response when password sign-in is refused + * because single sign-on (SAML or OIDC) is the active authentication method. + * The sign-in page uses it to point the user to the SSO flow instead of + * showing a generic login failure. + */ +export const SSO_LOGIN_REQUIRED_ERROR_CODE = 996; diff --git a/packages/@n8n/api-types/src/index.ts b/packages/@n8n/api-types/src/index.ts index 1d30f25c77c..c0bc40d5bf0 100644 --- a/packages/@n8n/api-types/src/index.ts +++ b/packages/@n8n/api-types/src/index.ts @@ -853,6 +853,7 @@ export { SSO_ERROR_QUERY_PARAM, SSO_ERROR_LOGIN_FAILED, } from './constants/role-mapping'; +export { SSO_LOGIN_REQUIRED_ERROR_CODE } from './constants/auth'; export { instanceAiApprovalDetailsSchema, diff --git a/packages/cli/src/controllers/__tests__/auth.controller.test.ts b/packages/cli/src/controllers/__tests__/auth.controller.test.ts index bcaf85fd80e..9548d0ba967 100644 --- a/packages/cli/src/controllers/__tests__/auth.controller.test.ts +++ b/packages/cli/src/controllers/__tests__/auth.controller.test.ts @@ -1,5 +1,5 @@ import type { LoginRequestDto } from '@n8n/api-types'; -import { ResolveSignupTokenQueryDto } from '@n8n/api-types'; +import { ResolveSignupTokenQueryDto, SSO_LOGIN_REQUIRED_ERROR_CODE } from '@n8n/api-types'; import { Logger } from '@n8n/backend-common'; import { mockInstance } from '@n8n/backend-test-utils'; import type { AuthenticatedRequest, User } from '@n8n/db'; @@ -145,6 +145,9 @@ describe('AuthController', () => { const execution = controller.login(req, res, body); await expect(execution).rejects.toThrow(AuthError); await expect(execution).rejects.toThrow('SSO is enabled, please log in with SSO'); + await expect(execution).rejects.toMatchObject({ + errorCode: SSO_LOGIN_REQUIRED_ERROR_CODE, + }); // Assert @@ -177,6 +180,9 @@ describe('AuthController', () => { const execution = controller.login(req, res, body); await expect(execution).rejects.toThrow(AuthError); await expect(execution).rejects.toThrow('SSO is enabled, please log in with SSO'); + await expect(execution).rejects.toMatchObject({ + errorCode: SSO_LOGIN_REQUIRED_ERROR_CODE, + }); expect(eventsService.emit).toHaveBeenCalledWith('user-login-failed', { authenticationMethod: 'email', diff --git a/packages/cli/src/controllers/auth.controller.ts b/packages/cli/src/controllers/auth.controller.ts index 5ae4a7c767b..0d7208b0372 100644 --- a/packages/cli/src/controllers/auth.controller.ts +++ b/packages/cli/src/controllers/auth.controller.ts @@ -1,4 +1,8 @@ -import { LoginRequestDto, ResolveSignupTokenQueryDto } from '@n8n/api-types'; +import { + LoginRequestDto, + ResolveSignupTokenQueryDto, + SSO_LOGIN_REQUIRED_ERROR_CODE, +} from '@n8n/api-types'; import { Logger } from '@n8n/backend-common'; import { Time } from '@n8n/constants'; import type { User, PublicUser, AuthProviderType } from '@n8n/db'; @@ -124,7 +128,7 @@ export class AuthController { userEmail, reason: 'SSO is enabled, please log in with SSO', }); - throw new AuthError('SSO is enabled, please log in with SSO'); + throw new AuthError('SSO is enabled, please log in with SSO', SSO_LOGIN_REQUIRED_ERROR_CODE); } } diff --git a/packages/cli/src/modules/sso-oidc/__tests__/oidc.controller.ee.test.ts b/packages/cli/src/modules/sso-oidc/__tests__/oidc.controller.ee.test.ts index 16b212d3da5..7ba85ec32fe 100644 --- a/packages/cli/src/modules/sso-oidc/__tests__/oidc.controller.ee.test.ts +++ b/packages/cli/src/modules/sso-oidc/__tests__/oidc.controller.ee.test.ts @@ -106,6 +106,48 @@ describe('OidcController', () => { expect(res.redirect).toHaveBeenCalledWith('/'); }); + test('Should redirect to the destination stored in the state after login', async () => { + const req = mock({ + originalUrl: '/sso/oidc/callback?code=auth_code&state=state_value', + browserId: 'browser-id-123', + cookies: { + [OIDC_STATE_COOKIE_NAME]: 'state_value', + [OIDC_NONCE_COOKIE_NAME]: 'nonce_value', + }, + }); + const res = mock(); + oidcService.verifyState.mockReturnValue({ + state: 'n8n_state:uuid', + redirectUrl: '/workflow/abc?tab=1', + }); + oidcService.loginUser.mockResolvedValueOnce({ user }); + + await controller.callbackHandler(req, res); + + expect(res.redirect).toHaveBeenCalledWith('/workflow/abc?tab=1'); + }); + + test('Should fall back to the home page when the stored destination is not a local path', async () => { + const req = mock({ + originalUrl: '/sso/oidc/callback?code=auth_code&state=state_value', + browserId: 'browser-id-123', + cookies: { + [OIDC_STATE_COOKIE_NAME]: 'state_value', + [OIDC_NONCE_COOKIE_NAME]: 'nonce_value', + }, + }); + const res = mock(); + oidcService.verifyState.mockReturnValue({ + state: 'n8n_state:uuid', + redirectUrl: '//evil.example/phish', + }); + oidcService.loginUser.mockResolvedValueOnce({ user }); + + await controller.callbackHandler(req, res); + + expect(res.redirect).toHaveBeenCalledWith('/'); + }); + test('Should handle callback URL with different query parameters', async () => { const req = mock({ originalUrl: @@ -549,6 +591,34 @@ describe('OidcController', () => { }); }); + test('Should carry a safe in-app destination into the login state', async () => { + const req = mock({ query: { redirect: '/workflow/abc?tab=1' } }); + const res = mock(); + oidcService.generateLoginUrl.mockResolvedValueOnce({ + url: new URL('https://provider.com/auth'), + state: 'state_value', + nonce: 'nonce_value', + }); + + await controller.redirectToAuthProvider(req, res); + + expect(oidcService.generateLoginUrl).toHaveBeenCalledWith('/workflow/abc?tab=1'); + }); + + test('Should replace an external destination with the home page', async () => { + const req = mock({ query: { redirect: 'https://evil.example/phish' } }); + const res = mock(); + oidcService.generateLoginUrl.mockResolvedValueOnce({ + url: new URL('https://provider.com/auth'), + state: 'state_value', + nonce: 'nonce_value', + }); + + await controller.redirectToAuthProvider(req, res); + + expect(oidcService.generateLoginUrl).toHaveBeenCalledWith('/'); + }); + test('Should propagate errors from OIDC service during URL generation', async () => { const req = mock(); const res = mock(); diff --git a/packages/cli/src/modules/sso-oidc/__tests__/oidc.service.ee.test.ts b/packages/cli/src/modules/sso-oidc/__tests__/oidc.service.ee.test.ts index 5455d413d4a..9a3f36b4b76 100644 --- a/packages/cli/src/modules/sso-oidc/__tests__/oidc.service.ee.test.ts +++ b/packages/cli/src/modules/sso-oidc/__tests__/oidc.service.ee.test.ts @@ -394,6 +394,37 @@ describe('OidcService', () => { }); }); + describe('state redirect target', () => { + beforeEach(() => { + vi.mocked(jwtService.sign).mockImplementation((payload) => JSON.stringify(payload)); + vi.mocked(jwtService.verify).mockImplementation((token) => JSON.parse(token)); + }); + + it('round-trips the in-app destination through the signed state', () => { + const { signed } = oidcService.generateState(false, '/workflow/abc?tab=1'); + + expect(oidcService.verifyState(signed)).toEqual({ + state: expect.stringMatching(/^n8n_state:/), + redirectUrl: '/workflow/abc?tab=1', + }); + }); + + it('omits the destination for the home page', () => { + const { signed } = oidcService.generateState(false, '/'); + + expect(oidcService.verifyState(signed)).not.toHaveProperty('redirectUrl'); + }); + + it('ignores a destination that is not a string', () => { + const signed = JSON.stringify({ + state: 'n8n_state:5d3a3f6e-1d2b-4c8a-9e1f-0a1b2c3d4e5f', + redirectUrl: 42, + }); + + expect(oidcService.verifyState(signed)).not.toHaveProperty('redirectUrl'); + }); + }); + describe('loginUser', () => { it('throws an error if authorizationCodeGrant throws an error', async () => { oidcService.verifyState = vi.fn().mockReturnValue('valid-state'); diff --git a/packages/cli/src/modules/sso-oidc/oidc.controller.ee.ts b/packages/cli/src/modules/sso-oidc/oidc.controller.ee.ts index b5507914f5d..5fd11b83819 100644 --- a/packages/cli/src/modules/sso-oidc/oidc.controller.ee.ts +++ b/packages/cli/src/modules/sso-oidc/oidc.controller.ee.ts @@ -19,6 +19,7 @@ import { SsoAccessDeniedError } from '@/modules/provisioning.ee/errors/sso-acces import { AuthlessRequest } from '@/requests'; import { UrlService } from '@/services/url.service'; import { isOidcCurrentAuthenticationMethod } from '@/sso.ee/sso-helpers'; +import { validateRedirectUrl } from '@/utils/validate-redirect-url'; import { OIDC_CLIENT_SECRET_REDACTED_VALUE, @@ -94,8 +95,10 @@ export class OidcController { @Get('/login', { skipAuth: true }) @Licensed('feat:oidc') - async redirectToAuthProvider(_req: Request, res: Response) { - const authorization = await this.oidcService.generateLoginUrl(); + async redirectToAuthProvider(req: Request<{}, {}, {}, { redirect?: string }>, res: Response) { + // Same in-app destination the sign-in page received; the callback redirects there. + const redirectUrl = validateRedirectUrl(req.query.redirect ?? ''); + const authorization = await this.oidcService.generateLoginUrl(redirectUrl); const { samesite, secure } = this.globalConfig.auth.cookie; res.cookie(OIDC_STATE_COOKIE_NAME, authorization.state, { @@ -188,7 +191,7 @@ export class OidcController { authenticationMethod: 'oidc', }); - return res.redirect('/'); + return res.redirect(validateRedirectUrl(stateInfo.redirectUrl ?? '')); } catch (error) { if (error instanceof SsoAccessDeniedError) { this.eventService.emit('user-login-failed', { diff --git a/packages/cli/src/modules/sso-oidc/oidc.service.ee.ts b/packages/cli/src/modules/sso-oidc/oidc.service.ee.ts index abc40d3b6d7..22debf04884 100644 --- a/packages/cli/src/modules/sso-oidc/oidc.service.ee.ts +++ b/packages/cli/src/modules/sso-oidc/oidc.service.ee.ts @@ -131,25 +131,34 @@ export class OidcService { }; } - generateState(testMode = false) { + /** + * The signed state also carries the in-app destination the user asked for + * before the login, so the callback can send them there instead of `/`. + */ + generateState(testMode = false, redirectUrl?: string) { const state = `n8n_state:${randomUUID()}`; const payload: Record = { state }; if (testMode) { payload.testMode = true; } + if (redirectUrl && redirectUrl !== '/') { + payload.redirectUrl = redirectUrl; + } return { signed: this.jwtService.sign(payload, { expiresIn: '15m' }), plaintext: state, }; } - verifyState(signedState: string): { state: string; testMode?: boolean } { + verifyState(signedState: string): { state: string; testMode?: boolean; redirectUrl?: string } { let state: string; let testMode: boolean | undefined; + let redirectUrl: unknown; try { const decodedState = this.jwtService.verify(signedState); state = decodedState?.state; testMode = decodedState?.testMode; + redirectUrl = decodedState?.redirectUrl; } catch (error) { this.logger.error('Failed to verify state', { error }); throw new BadRequestError('Invalid state'); @@ -175,7 +184,11 @@ export class OidcService { this.logger.error('Provided state is not formatted correctly'); throw new BadRequestError('Invalid state'); } - return { state, testMode }; + return { + state, + testMode, + ...(typeof redirectUrl === 'string' && { redirectUrl }), + }; } generateNonce() { @@ -225,12 +238,14 @@ export class OidcService { } } - async generateLoginUrl(): Promise<{ url: URL; state: string; nonce: string }> { + async generateLoginUrl( + redirectUrl?: string, + ): Promise<{ url: URL; state: string; nonce: string }> { this.assertOidcLoginEnabled(); await this.loadOpenIdClient(); const configuration = await this.getOidcConfiguration(); - const state = this.generateState(); + const state = this.generateState(false, redirectUrl); const nonce = this.generateNonce(); const prompt = this.oidcConfig.prompt; diff --git a/packages/cli/src/utils/__tests__/validate-redirect-url.test.ts b/packages/cli/src/utils/__tests__/validate-redirect-url.test.ts index 48db4eeccef..1a54fd9b2a1 100644 --- a/packages/cli/src/utils/__tests__/validate-redirect-url.test.ts +++ b/packages/cli/src/utils/__tests__/validate-redirect-url.test.ts @@ -8,6 +8,8 @@ describe('validateRedirectUrl', () => { ['/path?query=1', '/path?query=1'], ['/path#fragment', '/path#fragment'], [' /trimmed ', '/trimmed'], + ['/workflow/abc?tab=1&x=2', '/workflow/abc?tab=1&x=2'], + ['/%5Cexample.com', '/%5Cexample.com'], ])('allows valid relative path %s → %s', (input, expected) => { expect(validateRedirectUrl(input)).toBe(expected); }); @@ -21,6 +23,12 @@ describe('validateRedirectUrl', () => { ['%2F%2Fevil.com/phishing', '/'], ['workflows/123', '/'], ['data:text/html,

hi

', '/'], + ['/\\example.com/path', '/'], + ['/\\\\example.com', '/'], + ['/\t/example.com', '/'], + ['/\r\n/example.com', '/'], + ['/\n\\example.com', '/'], + ['/\\/example.com', '/'], ])('rejects unsafe redirect %s → %s', (input, expected) => { expect(validateRedirectUrl(input)).toBe(expected); }); diff --git a/packages/cli/src/utils/validate-redirect-url.ts b/packages/cli/src/utils/validate-redirect-url.ts index 45b2491db42..95c85599d8f 100644 --- a/packages/cli/src/utils/validate-redirect-url.ts +++ b/packages/cli/src/utils/validate-redirect-url.ts @@ -1,3 +1,6 @@ +/** Any origin works here; the check is only that the path resolves back onto it. */ +const PLACEHOLDER_ORIGIN = 'https://n8n.invalid'; + /** * Validates that a redirect URL is safe (relative path only, no external redirects). * Returns '/' if the URL is invalid or unsafe. @@ -17,6 +20,20 @@ export function validateRedirectUrl(redirectUrl: string): string { if (trimmed.startsWith('//')) { return '/'; } + // Browsers read a backslash as a slash and drop tabs and newlines while they + // parse a URL, so `/\example.com` and `//example.com` also leave the instance. + if (/[\\\t\n\r]/.test(trimmed)) { + return '/'; + } + // Let the URL parser have the final word: a relative path resolved against a + // placeholder origin has to stay on that origin. + try { + if (new URL(trimmed, PLACEHOLDER_ORIGIN).origin !== PLACEHOLDER_ORIGIN) { + return '/'; + } + } catch { + return '/'; + } return trimmed; } diff --git a/packages/frontend/@n8n/design-system/src/components/N8nFormInputs/FormInputs.test.ts b/packages/frontend/@n8n/design-system/src/components/N8nFormInputs/FormInputs.test.ts index 523023ccd4a..1037938c70a 100644 --- a/packages/frontend/@n8n/design-system/src/components/N8nFormInputs/FormInputs.test.ts +++ b/packages/frontend/@n8n/design-system/src/components/N8nFormInputs/FormInputs.test.ts @@ -1,11 +1,41 @@ import { mount } from '@vue/test-utils'; -import { describe, it, expect } from 'vitest'; +import { describe, it, expect, vi } from 'vitest'; import FormInputs from './FormInputs.vue'; import type { IFormInput } from '../../types'; +import { createFormEventBus } from '../../utils'; import N8nFormInput from '../N8nFormInput'; describe('FormInputs', () => { + describe('event bus', () => { + const inputs: IFormInput[] = [ + { name: 'email', properties: { type: 'text', label: 'Email' }, initialValue: 'a@b.c' }, + ]; + + it('should submit the current values when the bus emits submit', async () => { + const eventBus = createFormEventBus(); + const wrapper = mount(FormInputs, { props: { inputs, eventBus } }); + await wrapper.vm.$nextTick(); + + eventBus.emit('submit'); + + expect(wrapper.emitted('submit')).toEqual([[{ email: 'a@b.c' }]]); + }); + + it('should stop listening to the bus when unmounted', async () => { + const eventBus = createFormEventBus(); + const onSpy = vi.spyOn(eventBus, 'on'); + const offSpy = vi.spyOn(eventBus, 'off'); + const wrapper = mount(FormInputs, { props: { inputs, eventBus } }); + await wrapper.vm.$nextTick(); + + wrapper.unmount(); + + const [, registeredHandler] = onSpy.mock.calls[0]; + expect(offSpy).toHaveBeenCalledWith('submit', registeredHandler); + }); + }); + describe('dangling field removal', () => { it('should remove values for fields that are no longer in the inputs list', async () => { const initialInputs: IFormInput[] = [ diff --git a/packages/frontend/@n8n/design-system/src/components/N8nFormInputs/FormInputs.vue b/packages/frontend/@n8n/design-system/src/components/N8nFormInputs/FormInputs.vue index f741fe5f264..7fb16485357 100644 --- a/packages/frontend/@n8n/design-system/src/components/N8nFormInputs/FormInputs.vue +++ b/packages/frontend/@n8n/design-system/src/components/N8nFormInputs/FormInputs.vue @@ -1,5 +1,5 @@