From 4b2be263790a53bf46b99f3301ddec6a771b2daf Mon Sep 17 00:00:00 2001 From: Csaba Tuncsik Date: Wed, 23 Jul 2025 13:22:54 +0200 Subject: [PATCH] fix(editor): Persist SSO protocol setting properly in the UI (#17572) --- .../editor-ui/src/stores/sso.store.ts | 18 ++ .../frontend/editor-ui/src/stores/sso.test.ts | 116 ++++++++- .../editor-ui/src/views/SettingsSso.test.ts | 229 ++++++++++++++---- .../editor-ui/src/views/SettingsSso.vue | 23 +- 4 files changed, 326 insertions(+), 60 deletions(-) diff --git a/packages/frontend/editor-ui/src/stores/sso.store.ts b/packages/frontend/editor-ui/src/stores/sso.store.ts index ef66e63f06..59bbcc9cd7 100644 --- a/packages/frontend/editor-ui/src/stores/sso.store.ts +++ b/packages/frontend/editor-ui/src/stores/sso.store.ts @@ -9,10 +9,18 @@ import type { LdapConfig } from '@n8n/rest-api-client/api/ldap'; import type { IDataObject } from 'n8n-workflow'; import { UserManagementAuthenticationMethod } from '@/Interface'; +export const SupportedProtocols = { + SAML: 'saml', + OIDC: 'oidc', +} as const; + +export type SupportedProtocolType = (typeof SupportedProtocols)[keyof typeof SupportedProtocols]; + export const useSSOStore = defineStore('sso', () => { const rootStore = useRootStore(); const authenticationMethod = ref(undefined); + const selectedAuthProtocol = ref(undefined); const showSsoLoginButton = computed( () => @@ -188,10 +196,20 @@ export const useSSOStore = defineStore('sso', () => { return await ldapApi.runLdapSync(rootStore.restApiContext, data); }; + const initializeSelectedProtocol = () => { + if (selectedAuthProtocol.value) return; + + selectedAuthProtocol.value = isDefaultAuthenticationOidc.value + ? SupportedProtocols.OIDC + : SupportedProtocols.SAML; + }; + return { showSsoLoginButton, getSSORedirectUrl, initialize, + selectedAuthProtocol, + initializeSelectedProtocol, saml, samlConfig, diff --git a/packages/frontend/editor-ui/src/stores/sso.test.ts b/packages/frontend/editor-ui/src/stores/sso.test.ts index 389b486cd3..ffb5513e81 100644 --- a/packages/frontend/editor-ui/src/stores/sso.test.ts +++ b/packages/frontend/editor-ui/src/stores/sso.test.ts @@ -1,5 +1,5 @@ import { createPinia, setActivePinia } from 'pinia'; -import { useSSOStore } from '@/stores/sso.store'; +import { useSSOStore, SupportedProtocols } from '@/stores/sso.store'; import type { UserManagementAuthenticationMethod } from '@/Interface'; let ssoStore: ReturnType; @@ -36,4 +36,118 @@ describe('SSO store', () => { expect(ssoStore.showSsoLoginButton).toBe(expectation); }, ); + + describe('Protocol Selection Initialization', () => { + beforeEach(() => { + setActivePinia(createPinia()); + ssoStore = useSSOStore(); + }); + + it('should initialize selectedAuthProtocol to OIDC when default authentication is OIDC', () => { + // Initialize with OIDC as default authentication method + ssoStore.initialize({ + authenticationMethod: 'oidc' as UserManagementAuthenticationMethod, + config: { + oidc: { loginEnabled: true }, + }, + features: { + saml: false, + ldap: false, + oidc: true, + }, + }); + + // selectedAuthProtocol should be undefined initially + expect(ssoStore.selectedAuthProtocol).toBeUndefined(); + + // Call initializeSelectedProtocol + ssoStore.initializeSelectedProtocol(); + + // Should now be set to OIDC + expect(ssoStore.selectedAuthProtocol).toBe(SupportedProtocols.OIDC); + }); + + it('should initialize selectedAuthProtocol to SAML when default authentication is SAML', () => { + // Initialize with SAML as default authentication method + ssoStore.initialize({ + authenticationMethod: 'saml' as UserManagementAuthenticationMethod, + config: { + saml: { loginEnabled: true }, + }, + features: { + saml: true, + ldap: false, + oidc: false, + }, + }); + + // selectedAuthProtocol should be undefined initially + expect(ssoStore.selectedAuthProtocol).toBeUndefined(); + + // Call initializeSelectedProtocol + ssoStore.initializeSelectedProtocol(); + + // Should now be set to SAML + expect(ssoStore.selectedAuthProtocol).toBe(SupportedProtocols.SAML); + }); + + it('should initialize selectedAuthProtocol to SAML when default authentication is email', () => { + // Initialize with email as default authentication method + ssoStore.initialize({ + authenticationMethod: 'email' as UserManagementAuthenticationMethod, + config: {}, + features: { + saml: true, + ldap: false, + oidc: true, + }, + }); + + // selectedAuthProtocol should be undefined initially + expect(ssoStore.selectedAuthProtocol).toBeUndefined(); + + // Call initializeSelectedProtocol + ssoStore.initializeSelectedProtocol(); + + // Should default to SAML when not OIDC + expect(ssoStore.selectedAuthProtocol).toBe(SupportedProtocols.SAML); + }); + + it('should not reinitialize selectedAuthProtocol if already set', () => { + // Initialize with SAML as default authentication method + ssoStore.initialize({ + authenticationMethod: 'saml' as UserManagementAuthenticationMethod, + config: { + saml: { loginEnabled: true }, + }, + features: { + saml: true, + ldap: false, + oidc: true, + }, + }); + + // Manually set selectedAuthProtocol to OIDC + ssoStore.selectedAuthProtocol = SupportedProtocols.OIDC; + + // Call initializeSelectedProtocol + ssoStore.initializeSelectedProtocol(); + + // Should remain OIDC (not overwritten) + expect(ssoStore.selectedAuthProtocol).toBe(SupportedProtocols.OIDC); + }); + + it('should handle undefined authentication method gracefully', () => { + // Don't initialize the store (authenticationMethod remains undefined) + + // selectedAuthProtocol should be undefined initially + expect(ssoStore.selectedAuthProtocol).toBeUndefined(); + + // Call initializeSelectedProtocol + ssoStore.initializeSelectedProtocol(); + + // Should default to SAML when authentication method is undefined + expect(ssoStore.selectedAuthProtocol).toBe(SupportedProtocols.SAML); + }); + }); }); diff --git a/packages/frontend/editor-ui/src/views/SettingsSso.test.ts b/packages/frontend/editor-ui/src/views/SettingsSso.test.ts index ad3cbbba22..81d15d1a77 100644 --- a/packages/frontend/editor-ui/src/views/SettingsSso.test.ts +++ b/packages/frontend/editor-ui/src/views/SettingsSso.test.ts @@ -1,10 +1,10 @@ import type { OidcConfigDto, SamlPreferences } from '@n8n/api-types'; import { createTestingPinia } from '@pinia/testing'; import { within, waitFor } from '@testing-library/vue'; -import { mockedStore } from '@/__tests__/utils'; +import { type MockedStore, mockedStore } from '@/__tests__/utils'; import SettingsSso from '@/views/SettingsSso.vue'; import userEvent from '@testing-library/user-event'; -import { useSSOStore } from '@/stores/sso.store'; +import { useSSOStore, SupportedProtocols } from '@/stores/sso.store'; import { createComponentRenderer } from '@/__tests__/render'; import { usePageRedirectionHelper } from '@/composables/usePageRedirectionHelper'; import type { SamlPreferencesExtractedData } from '@n8n/rest-api-client/api/sso'; @@ -53,8 +53,21 @@ vi.mock('@/composables/usePageRedirectionHelper', () => { }; }); +// Mock window.open to avoid JSDOM "Not implemented" error +Object.defineProperty(window, 'open', { + writable: true, + value: vi.fn(), +}); + describe('SettingsSso View', () => { + let ssoStore: MockedStore; + beforeEach(() => { + createTestingPinia(); + ssoStore = mockedStore(useSSOStore); + }); + + afterEach(() => { telemetryTrack.mockReset(); confirmMessage.mockReset(); showError.mockReset(); @@ -62,14 +75,12 @@ describe('SettingsSso View', () => { describe('SAML', () => { it('should show upgrade banner when enterprise SAML is disabled', async () => { - const pinia = createTestingPinia(); - const ssoStore = mockedStore(useSSOStore); ssoStore.isEnterpriseSamlEnabled = false; ssoStore.isEnterpriseOidcEnabled = false; const pageRedirectionHelper = usePageRedirectionHelper(); - const { getByTestId } = renderView({ pinia }); + const { getByTestId } = renderView(); const actionBox = getByTestId('sso-content-unlicensed'); expect(actionBox).toBeInTheDocument(); @@ -79,15 +90,12 @@ describe('SettingsSso View', () => { }); it('should show user SSO config', async () => { - const pinia = createTestingPinia(); - - const ssoStore = mockedStore(useSSOStore); ssoStore.isEnterpriseSamlEnabled = true; ssoStore.isEnterpriseOidcEnabled = true; ssoStore.getSamlConfig.mockResolvedValue(samlConfig); - const { getAllByTestId } = renderView({ pinia }); + const { getAllByTestId } = renderView(); expect(ssoStore.getSamlConfig).toHaveBeenCalledTimes(1); @@ -99,16 +107,13 @@ describe('SettingsSso View', () => { }); it('allows user to toggle SSO', async () => { - const pinia = createTestingPinia(); - - const ssoStore = mockedStore(useSSOStore); ssoStore.isEnterpriseSamlEnabled = true; ssoStore.isSamlLoginEnabled = false; ssoStore.isEnterpriseOidcEnabled = true; ssoStore.getSamlConfig.mockResolvedValue(samlConfig); - const { getByTestId } = renderView({ pinia }); + const { getByTestId } = renderView(); const toggle = getByTestId('sso-toggle'); @@ -124,14 +129,12 @@ describe('SettingsSso View', () => { it("allows user to fill Identity Provider's URL", async () => { confirmMessage.mockResolvedValueOnce('confirm'); - const pinia = createTestingPinia(); const windowOpenSpy = vi.spyOn(window, 'open'); - const ssoStore = mockedStore(useSSOStore); ssoStore.isEnterpriseSamlEnabled = true; ssoStore.isEnterpriseOidcEnabled = true; - const { getByTestId } = renderView({ pinia }); + const { getByTestId } = renderView(); const saveButton = getByTestId('sso-save'); expect(saveButton).toBeDisabled(); @@ -162,14 +165,12 @@ describe('SettingsSso View', () => { it("allows user to fill Identity Provider's XML", async () => { confirmMessage.mockResolvedValueOnce('confirm'); - const pinia = createTestingPinia(); const windowOpenSpy = vi.spyOn(window, 'open'); - const ssoStore = mockedStore(useSSOStore); ssoStore.isEnterpriseSamlEnabled = true; ssoStore.isEnterpriseOidcEnabled = true; - const { getByTestId } = renderView({ pinia }); + const { getByTestId } = renderView(); const saveButton = getByTestId('sso-save'); expect(saveButton).toBeDisabled(); @@ -200,13 +201,10 @@ describe('SettingsSso View', () => { }); it('should validate the url before setting the saml config', async () => { - const pinia = createTestingPinia(); - - const ssoStore = mockedStore(useSSOStore); ssoStore.isEnterpriseSamlEnabled = true; ssoStore.isEnterpriseOidcEnabled = true; - const { getByTestId } = renderView({ pinia }); + const { getByTestId } = renderView(); const saveButton = getByTestId('sso-save'); expect(saveButton).toBeDisabled(); @@ -230,13 +228,10 @@ describe('SettingsSso View', () => { }); it('should ensure the url does not support invalid protocols like mailto', async () => { - const pinia = createTestingPinia(); - - const ssoStore = mockedStore(useSSOStore); ssoStore.isEnterpriseSamlEnabled = true; ssoStore.isEnterpriseOidcEnabled = true; - const { getByTestId } = renderView({ pinia }); + const { getByTestId } = renderView(); const saveButton = getByTestId('sso-save'); expect(saveButton).toBeDisabled(); @@ -260,9 +255,6 @@ describe('SettingsSso View', () => { }); it('allows user to disable SSO even if config request failed', async () => { - const pinia = createTestingPinia(); - - const ssoStore = mockedStore(useSSOStore); ssoStore.isEnterpriseSamlEnabled = true; ssoStore.isSamlLoginEnabled = true; ssoStore.isEnterpriseOidcEnabled = true; @@ -271,7 +263,7 @@ describe('SettingsSso View', () => { const error = new Error('Request failed with status code 404'); ssoStore.getSamlConfig.mockRejectedValue(error); - const { getByTestId } = renderView({ pinia }); + const { getByTestId } = renderView(); expect(ssoStore.getSamlConfig).toHaveBeenCalledTimes(1); @@ -283,10 +275,8 @@ describe('SettingsSso View', () => { expect(toggle.textContent).toContain('Deactivated'); }); }); - it('should enable activation checkbox and test button if data is already saved', async () => { - const pinia = createTestingPinia(); - const ssoStore = mockedStore(useSSOStore); + it('should enable activation checkbox and test button if data is already saved', async () => { ssoStore.isEnterpriseSamlEnabled = true; ssoStore.isSamlLoginEnabled = true; @@ -295,9 +285,7 @@ describe('SettingsSso View', () => { ssoStore.getSamlConfig.mockResolvedValue(samlConfig); - const { container, getByTestId, getByRole } = renderView({ - pinia, - }); + const { container, getByTestId, getByRole } = renderView(); await userEvent.click(getByTestId('radio-button-xml')); @@ -310,9 +298,6 @@ describe('SettingsSso View', () => { describe('OIDC', () => { it('should show upgrade banner when enterprise OIDC is disabled', async () => { - const pinia = createTestingPinia(); - const ssoStore = mockedStore(useSSOStore); - ssoStore.isDefaultAuthenticationSaml = false; ssoStore.isEnterpriseSamlEnabled = false; @@ -321,7 +306,7 @@ describe('SettingsSso View', () => { const pageRedirectionHelper = usePageRedirectionHelper(); - const { getByTestId } = renderView({ pinia }); + const { getByTestId } = renderView(); await waitFor(() => { const actionBox = getByTestId('sso-content-unlicensed'); @@ -335,16 +320,13 @@ describe('SettingsSso View', () => { }); it('allows user to save OIDC config', async () => { - const pinia = createTestingPinia(); - - const ssoStore = mockedStore(useSSOStore); ssoStore.saveOidcConfig.mockResolvedValue(oidcConfig); ssoStore.isEnterpriseOidcEnabled = true; ssoStore.isEnterpriseSamlEnabled = false; ssoStore.isOidcLoginEnabled = true; ssoStore.isSamlLoginEnabled = false; - const { getByTestId, getByRole } = renderView({ pinia }); + const { getByTestId, getByRole } = renderView(); // Set authProtocol component ref to OIDC const protocolSelect = getByRole('combobox'); @@ -396,4 +378,161 @@ describe('SettingsSso View', () => { ); }); }); + + describe('Protocol Selection Persistence', () => { + it('should not persist protocol selection to store until save is clicked', async () => { + ssoStore.isEnterpriseSamlEnabled = true; + ssoStore.isEnterpriseOidcEnabled = true; + ssoStore.selectedAuthProtocol = SupportedProtocols.SAML; + ssoStore.getSamlConfig.mockResolvedValue(samlConfig); + ssoStore.getOidcConfig.mockResolvedValue(oidcConfig); + + const { getByRole } = renderView(); + + // Change protocol selection in dropdown + const protocolSelect = getByRole('combobox'); + await userEvent.click(protocolSelect); + + const dropdown = await waitFor(() => getByRole('listbox')); + const items = dropdown.querySelectorAll('.el-select-dropdown__item'); + const oidcItem = Array.from(items).find((item) => item.textContent?.includes('OIDC')); + + await userEvent.click(oidcItem!); + + // Verify store selectedAuthProtocol is still SAML (not updated) + expect(ssoStore.selectedAuthProtocol).toBe(SupportedProtocols.SAML); + }); + + it('should persist SAML protocol selection to store only after successful save', async () => { + ssoStore.isEnterpriseSamlEnabled = true; + ssoStore.isEnterpriseOidcEnabled = true; + ssoStore.selectedAuthProtocol = SupportedProtocols.OIDC; // Initially OIDC + ssoStore.getSamlConfig.mockResolvedValue(samlConfig); + ssoStore.getOidcConfig.mockResolvedValue(oidcConfig); + + const { getByRole, getByTestId } = renderView(); + + // Change to SAML protocol in dropdown + const protocolSelect = getByRole('combobox'); + await userEvent.click(protocolSelect); + + const dropdown = await waitFor(() => getByRole('listbox')); + const items = dropdown.querySelectorAll('.el-select-dropdown__item'); + const samlItem = Array.from(items).find((item) => item.textContent?.includes('SAML')); + + await userEvent.click(samlItem!); + + // Verify store selectedAuthProtocol is still OIDC (not updated yet) + expect(ssoStore.selectedAuthProtocol).toBe(SupportedProtocols.OIDC); + + // Fill and save SAML config + const urlInput = getByTestId('sso-provider-url'); + await userEvent.type(urlInput, samlConfig.metadataUrl as string); + + const saveButton = getByTestId('sso-save'); + await userEvent.click(saveButton); + + // Now verify store selectedAuthProtocol is updated to SAML after save + await waitFor(() => { + expect(ssoStore.selectedAuthProtocol).toBe(SupportedProtocols.SAML); + }); + }); + + it('should persist OIDC protocol selection to store only after successful save', async () => { + ssoStore.isEnterpriseSamlEnabled = true; + ssoStore.isEnterpriseOidcEnabled = true; + ssoStore.selectedAuthProtocol = SupportedProtocols.SAML; // Initially SAML + ssoStore.getSamlConfig.mockResolvedValue(samlConfig); + ssoStore.getOidcConfig.mockResolvedValue(oidcConfig); + ssoStore.saveOidcConfig.mockResolvedValue(oidcConfig); + + const { getByRole, getByTestId } = renderView(); + + // Change to OIDC protocol in dropdown + const protocolSelect = getByRole('combobox'); + await userEvent.click(protocolSelect); + + const dropdown = await waitFor(() => getByRole('listbox')); + const items = dropdown.querySelectorAll('.el-select-dropdown__item'); + const oidcItem = Array.from(items).find((item) => item.textContent?.includes('OIDC')); + + await userEvent.click(oidcItem!); + + // Verify store selectedAuthProtocol is still SAML (not updated yet) + expect(ssoStore.selectedAuthProtocol).toBe(SupportedProtocols.SAML); + + // Fill and save OIDC config + const discoveryInput = getByTestId('oidc-discovery-endpoint'); + await userEvent.type(discoveryInput, oidcConfig.discoveryEndpoint); + + const clientIdInput = getByTestId('oidc-client-id'); + await userEvent.type(clientIdInput, 'test-client-id'); + + const clientSecretInput = getByTestId('oidc-client-secret'); + await userEvent.type(clientSecretInput, 'test-client-secret'); + + const saveButton = getByTestId('sso-oidc-save'); + await userEvent.click(saveButton); + + // Now verify store selectedAuthProtocol is updated to OIDC after save + await waitFor(() => { + expect(ssoStore.selectedAuthProtocol).toBe(SupportedProtocols.OIDC); + }); + }); + + it('should initialize local protocol selection from store on mount', async () => { + ssoStore.isEnterpriseSamlEnabled = true; + ssoStore.isEnterpriseOidcEnabled = true; + ssoStore.selectedAuthProtocol = SupportedProtocols.OIDC; // Store has OIDC saved + ssoStore.getSamlConfig.mockResolvedValue(samlConfig); + ssoStore.getOidcConfig.mockResolvedValue(oidcConfig); + + const { getByRole } = renderView(); + + // Wait for component to mount and initialize + await waitFor(() => { + expect(ssoStore.initializeSelectedProtocol).toHaveBeenCalled(); + }); + + // Wait for component to mount and initialize local state from store + await waitFor(() => { + const protocolSelect = getByRole('combobox'); + // Check that the dropdown shows OIDC (reflecting store state) + expect(protocolSelect).toHaveDisplayValue('OIDC'); + }); + }); + + it('should show correct conditional content based on local protocol selection, not store', async () => { + ssoStore.isEnterpriseSamlEnabled = true; + ssoStore.isEnterpriseOidcEnabled = true; + ssoStore.selectedAuthProtocol = SupportedProtocols.SAML; // Store has SAML + ssoStore.getSamlConfig.mockResolvedValue(samlConfig); + ssoStore.getOidcConfig.mockResolvedValue(oidcConfig); + + const { getByRole, getByTestId, queryByTestId } = renderView(); + + // Initially should show SAML content (matching store) + expect(getByTestId('sso-provider-url')).toBeVisible(); + expect(queryByTestId('oidc-discovery-endpoint')).not.toBeInTheDocument(); + + // Change to OIDC in dropdown (local state only) + const protocolSelect = getByRole('combobox'); + await userEvent.click(protocolSelect); + + const dropdown = await waitFor(() => getByRole('listbox')); + const items = dropdown.querySelectorAll('.el-select-dropdown__item'); + const oidcItem = Array.from(items).find((item) => item.textContent?.includes('OIDC')); + + await userEvent.click(oidcItem!); + + // Now should show OIDC content (based on local state, not store) + await waitFor(() => { + expect(getByTestId('oidc-discovery-endpoint')).toBeVisible(); + expect(queryByTestId('sso-provider-url')).not.toBeInTheDocument(); + }); + + // Verify store still has SAML (unchanged) + expect(ssoStore.selectedAuthProtocol).toBe(SupportedProtocols.SAML); + }); + }); }); diff --git a/packages/frontend/editor-ui/src/views/SettingsSso.vue b/packages/frontend/editor-ui/src/views/SettingsSso.vue index 9fd1acadf0..c515616dfc 100644 --- a/packages/frontend/editor-ui/src/views/SettingsSso.vue +++ b/packages/frontend/editor-ui/src/views/SettingsSso.vue @@ -6,23 +6,16 @@ import { usePageRedirectionHelper } from '@/composables/usePageRedirectionHelper import { useTelemetry } from '@/composables/useTelemetry'; import { useToast } from '@/composables/useToast'; import { MODAL_CONFIRM } from '@/constants'; -import { useSSOStore } from '@/stores/sso.store'; +import { useSSOStore, SupportedProtocols, type SupportedProtocolType } from '@/stores/sso.store'; import { useI18n } from '@n8n/i18n'; import { useRootStore } from '@n8n/stores/useRootStore'; import { computed, onMounted, ref } from 'vue'; -type SupportedProtocolType = (typeof SupportedProtocols)[keyof typeof SupportedProtocols]; - const IdentityProviderSettingsType = { URL: 'url', XML: 'xml', }; -const SupportedProtocols = { - SAML: 'saml', - OIDC: 'oidc', -} as const; - const i18n = useI18n(); const telemetry = useTelemetry(); const rootStore = useRootStore(); @@ -164,6 +157,9 @@ const onSave = async () => { : { metadata: metadata.value }; await ssoStore.saveSamlConfig(config); + // Update store with saved protocol selection + ssoStore.selectedAuthProtocol = authProtocol.value; + if (!ssoStore.isSamlLoginEnabled) { const answer = await message.confirm( i18n.baseText('settings.sso.settings.save.activate.message'), @@ -235,12 +231,8 @@ const isToggleSsoDisabled = computed(() => { onMounted(async () => { documentTitle.set(i18n.baseText('settings.sso.title')); await Promise.all([loadSamlConfig(), loadOidcConfig()]); - - if (ssoStore.isDefaultAuthenticationSaml) { - authProtocol.value = SupportedProtocols.SAML; - } else if (ssoStore.isDefaultAuthenticationOidc) { - authProtocol.value = SupportedProtocols.OIDC; - } + ssoStore.initializeSelectedProtocol(); + authProtocol.value = ssoStore.selectedAuthProtocol || SupportedProtocols.SAML; }); const getOidcConfig = async () => { @@ -299,6 +291,9 @@ async function onOidcSettingsSave() { loginEnabled: ssoStore.isOidcLoginEnabled, }); + // Update store with saved protocol selection + ssoStore.selectedAuthProtocol = authProtocol.value; + clientSecret.value = newConfig.clientSecret; trackUpdateSettings(); }