feat: Add SSO SAML metadataUrl support and various improvements (#6139)

* feat: add various sso improvements

* fix: remove test button assertion

* fix: fix type imports

* test: attempt fixing unit tests

* fix: changed to using useToast for error toasts

* Minor copy tweaks and swapped buttons position.

* fix locale ref

* align error with UI wording

* simplify saving ux

* fix pretty

* fix: update saml sso setting saving

* fix: undo try/catch changes when saving saml config

* metadata url tab selected at first

* chore: fix linting issue

* test: fix activation checkbox test

---------

Co-authored-by: Giulio Andreini <g.andreini@gmail.com>
Co-authored-by: Michael Auerswald <michael.auerswald@gmail.com>
Co-authored-by: Romain Minaud <romain.minaud@gmail.com>

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

@@ -1,55 +1,120 @@
 <script lang="ts" setup>
-import { computed, ref, onBeforeMount } from 'vue';
-import { Notification } from 'element-ui';
+import { computed, ref, onMounted } from 'vue';
 import { useSSOStore } from '@/stores/sso.store';
 import { useUIStore } from '@/stores/ui.store';
-import { i18n as locale } from '@/plugins/i18n';
 import CopyInput from '@/components/CopyInput.vue';
+import { useI18n, useMessage, useToast } from '@/composables';
 
+const IdentityProviderSettingsType = {
+	URL: 'url',
+	XML: 'xml',
+};
+
+const { i18n } = useI18n();
 const ssoStore = useSSOStore();
 const uiStore = useUIStore();
+const message = useMessage();
+const toast = useToast();
 
 const ssoActivatedLabel = computed(() =>
 	ssoStore.isSamlLoginEnabled
-		? locale.baseText('settings.sso.activated')
-		: locale.baseText('settings.sso.deactivated'),
+		? i18n.baseText('settings.sso.activated')
+		: i18n.baseText('settings.sso.deactivated'),
 );
 const ssoSettingsSaved = ref(false);
-const metadata = ref();
+
 const redirectUrl = ref();
 const entityId = ref();
 
+const ipsOptions = ref([
+	{
+		label: i18n.baseText('settings.sso.settings.ips.options.url'),
+		value: IdentityProviderSettingsType.URL,
+	},
+	{
+		label: i18n.baseText('settings.sso.settings.ips.options.xml'),
+		value: IdentityProviderSettingsType.XML,
+	},
+]);
+const ipsType = ref(IdentityProviderSettingsType.URL);
+
+const metadataUrl = ref();
+const metadata = ref();
+
+const isSaveEnabled = computed(() => {
+	if (ipsType.value === IdentityProviderSettingsType.URL) {
+		return !!metadataUrl.value && metadataUrl.value !== ssoStore.samlConfig?.metadataUrl;
+	} else if (ipsType.value === IdentityProviderSettingsType.XML) {
+		return !!metadata.value && metadata.value !== ssoStore.samlConfig?.metadata;
+	}
+	return false;
+});
+
+const isTestEnabled = computed(() => {
+	if (ipsType.value === IdentityProviderSettingsType.URL) {
+		return !!metadataUrl.value && ssoSettingsSaved.value;
+	} else if (ipsType.value === IdentityProviderSettingsType.XML) {
+		return !!metadata.value && ssoSettingsSaved.value;
+	}
+	return false;
+});
+
 const getSamlConfig = async () => {
 	const config = await ssoStore.getSamlConfig();
+
 	entityId.value = config?.entityID;
 	redirectUrl.value = config?.returnUrl;
+
+	if (config?.metadataUrl) {
+		ipsType.value = IdentityProviderSettingsType.URL;
+	} else if (config?.metadata) {
+		ipsType.value = IdentityProviderSettingsType.XML;
+	}
+
 	metadata.value = config?.metadata;
+	metadataUrl.value = config?.metadataUrl;
 	ssoSettingsSaved.value = !!config?.metadata;
 };
 
 const onSave = async () => {
 	try {
-		await ssoStore.saveSamlConfig({ metadata: metadata.value });
-		await getSamlConfig();
+		const config =
+			ipsType.value === IdentityProviderSettingsType.URL
+				? { metadataUrl: metadataUrl.value }
+				: { metadata: metadata.value };
+		await ssoStore.saveSamlConfig(config);
+
+		if (!ssoStore.isSamlLoginEnabled) {
+			const answer = await message.confirm(
+				i18n.baseText('settings.sso.settings.save.activate.message'),
+				i18n.baseText('settings.sso.settings.save.activate.title'),
+				{
+					confirmButtonText: i18n.baseText('settings.sso.settings.save.activate.test'),
+					cancelButtonText: i18n.baseText('settings.sso.settings.save.activate.cancel'),
+				},
+			);
+
+			if (answer === 'confirm') {
+				await onTest();
+			}
+		}
 	} catch (error) {
-		Notification.error({
-			title: 'Error',
-			message: error.message,
-			position: 'bottom-right',
-		});
+		toast.showError(error, i18n.baseText('settings.sso.settings.save.error'));
+		return;
+	} finally {
+		await getSamlConfig();
 	}
 };
 
 const onTest = async () => {
 	try {
 		const url = await ssoStore.testSamlConfig();
-		window.open(url, '_blank');
+
+		if (typeof window !== 'undefined') {
+			window.open(url, '_blank');
+		}
 	} catch (error) {
-		Notification.error({
-			title: 'Error',
-			message: error.message,
-			position: 'bottom-right',
-		});
+		toast.showError(error, 'error');
 	}
 };
 
@@ -57,31 +122,30 @@ const goToUpgrade = () => {
 	uiStore.goToUpgrade('sso', 'upgrade-sso');
 };
 
-onBeforeMount(async () => {
+onMounted(async () => {
 	if (!ssoStore.isEnterpriseSamlEnabled) {
 		return;
 	}
 	try {
 		await getSamlConfig();
 	} catch (error) {
-		Notification.error({
-			title: 'Error',
-			message: error.message,
-			position: 'bottom-right',
-		});
+		toast.showError(error, 'error');
 	}
 });
 </script>
 
 <template>
 	<div>
-		<n8n-heading size="2xlarge">{{ locale.baseText('settings.sso.title') }}</n8n-heading>
+		<n8n-heading size="2xlarge">{{ i18n.baseText('settings.sso.title') }}</n8n-heading>
 		<div :class="$style.top">
-			<n8n-heading size="medium">{{ locale.baseText('settings.sso.subtitle') }}</n8n-heading>
-			<n8n-tooltip v-if="ssoStore.isEnterpriseSamlEnabled" :disabled="ssoStore.isSamlLoginEnabled">
+			<n8n-heading size="xlarge">{{ i18n.baseText('settings.sso.subtitle') }}</n8n-heading>
+			<n8n-tooltip
+				v-if="ssoStore.isEnterpriseSamlEnabled"
+				:disabled="ssoStore.isSamlLoginEnabled || ssoSettingsSaved"
+			>
 				<template #content>
 					<span>
-						{{ locale.baseText('settings.sso.activation.tooltip') }}
+						{{ i18n.baseText('settings.sso.activation.tooltip') }}
 					</span>
 				</template>
 				<el-switch
@@ -93,62 +157,77 @@ onBeforeMount(async () => {
 			</n8n-tooltip>
 		</div>
 		<n8n-info-tip>
-			<i18n path="settings.sso.info">
-				<template #link>
-					<a href="https://docs.n8n.io/user-management/saml/" target="_blank">
-						{{ locale.baseText('settings.sso.info.link') }}
-					</a>
-				</template>
-			</i18n>
+			{{ i18n.baseText('settings.sso.info') }}
+			<a href="https://docs.n8n.io/user-management/saml/" target="_blank">
+				{{ i18n.baseText('settings.sso.info.link') }}
+			</a>
 		</n8n-info-tip>
 		<div v-if="ssoStore.isEnterpriseSamlEnabled" data-test-id="sso-content-licensed">
 			<div :class="$style.group">
-				<label>{{ locale.baseText('settings.sso.settings.redirectUrl.label') }}</label>
+				<label>{{ i18n.baseText('settings.sso.settings.redirectUrl.label') }}</label>
 				<CopyInput
 					:value="redirectUrl"
-					:copy-button-text="locale.baseText('generic.clickToCopy')"
-					:toast-title="locale.baseText('settings.sso.settings.redirectUrl.copied')"
+					:copy-button-text="i18n.baseText('generic.clickToCopy')"
+					:toast-title="i18n.baseText('settings.sso.settings.redirectUrl.copied')"
 				/>
-				<small>{{ locale.baseText('settings.sso.settings.redirectUrl.help') }}</small>
+				<small>{{ i18n.baseText('settings.sso.settings.redirectUrl.help') }}</small>
 			</div>
 			<div :class="$style.group">
-				<label>{{ locale.baseText('settings.sso.settings.entityId.label') }}</label>
+				<label>{{ i18n.baseText('settings.sso.settings.entityId.label') }}</label>
 				<CopyInput
 					:value="entityId"
-					:copy-button-text="locale.baseText('generic.clickToCopy')"
-					:toast-title="locale.baseText('settings.sso.settings.entityId.copied')"
+					:copy-button-text="i18n.baseText('generic.clickToCopy')"
+					:toast-title="i18n.baseText('settings.sso.settings.entityId.copied')"
 				/>
-				<small>{{ locale.baseText('settings.sso.settings.entityId.help') }}</small>
+				<small>{{ i18n.baseText('settings.sso.settings.entityId.help') }}</small>
 			</div>
 			<div :class="$style.group">
-				<label>{{ locale.baseText('settings.sso.settings.ips.label') }}</label>
-				<n8n-input v-model="metadata" type="textarea" name="metadata" />
-				<small>{{ locale.baseText('settings.sso.settings.ips.help') }}</small>
+				<label>{{ i18n.baseText('settings.sso.settings.ips.label') }}</label>
+				<div class="mt-2xs mb-s">
+					<n8n-radio-buttons :options="ipsOptions" v-model="ipsType" />
+				</div>
+				<div v-show="ipsType === IdentityProviderSettingsType.URL">
+					<n8n-input
+						v-model="metadataUrl"
+						type="text"
+						name="metadataUrl"
+						size="large"
+						:placeholder="i18n.baseText('settings.sso.settings.ips.url.placeholder')"
+					/>
+					<small>{{ i18n.baseText('settings.sso.settings.ips.url.help') }}</small>
+				</div>
+				<div v-show="ipsType === IdentityProviderSettingsType.XML">
+					<n8n-input v-model="metadata" type="textarea" name="metadata" :rows="4" />
+					<small>{{ i18n.baseText('settings.sso.settings.ips.xml.help') }}</small>
+				</div>
 			</div>
 			<div :class="$style.buttons">
+				<n8n-button :disabled="!isSaveEnabled" @click="onSave" data-test-id="sso-save">
+					{{ i18n.baseText('settings.sso.settings.save') }}
+				</n8n-button>
 				<n8n-button
-					:disabled="!ssoSettingsSaved"
+					:disabled="!isTestEnabled"
 					type="tertiary"
 					@click="onTest"
 					data-test-id="sso-test"
 				>
-					{{ locale.baseText('settings.sso.settings.test') }}
-				</n8n-button>
-				<n8n-button :disabled="!metadata" @click="onSave" data-test-id="sso-save">
-					{{ locale.baseText('settings.sso.settings.save') }}
+					{{ i18n.baseText('settings.sso.settings.test') }}
 				</n8n-button>
 			</div>
+			<footer :class="$style.footer">
+				{{ i18n.baseText('settings.sso.settings.footer.hint') }}
+			</footer>
 		</div>
 		<n8n-action-box
 			v-else
 			data-test-id="sso-content-unlicensed"
 			:class="$style.actionBox"
-			:description="locale.baseText('settings.sso.actionBox.description')"
-			:buttonText="locale.baseText('settings.sso.actionBox.buttonText')"
+			:description="i18n.baseText('settings.sso.actionBox.description')"
+			:buttonText="i18n.baseText('settings.sso.actionBox.buttonText')"
 			@click="goToUpgrade"
 		>
 			<template #heading>
-				<span>{{ locale.baseText('settings.sso.actionBox.title') }}</span>
+				<span>{{ i18n.baseText('settings.sso.actionBox.title') }}</span>
 			</template>
 		</n8n-action-box>
 	</div>
@@ -173,7 +252,7 @@ onBeforeMount(async () => {
 .buttons {
 	display: flex;
 	justify-content: flex-start;
-	padding: var(--spacing-2xl) 0 var(--spacing-3xl);
+	padding: var(--spacing-2xl) 0 var(--spacing-2xs);
 
 	button {
 		margin: 0 var(--spacing-s) 0 0;
@@ -183,7 +262,7 @@ onBeforeMount(async () => {
 .group {
 	padding: var(--spacing-xl) 0 0;
 
-	label {
+	> label {
 		display: inline-block;
 		font-size: var(--font-size-s);
 		font-weight: var(--font-weight-bold);
@@ -201,4 +280,9 @@ onBeforeMount(async () => {
 .actionBox {
 	margin: var(--spacing-2xl) 0 0;
 }
+
+.footer {
+	color: var(--color-text-base);
+	font-size: var(--font-size-2xs);
+}
 </style>

packages/editor-ui/src/views/__tests__/SettingsSso.test.ts

@@ -1,58 +1,46 @@
-import { PiniaVuePlugin } from 'pinia';
-import { render } from '@testing-library/vue';
-import userEvent from '@testing-library/user-event';
-import { createTestingPinia } from '@pinia/testing';
-import { merge } from 'lodash-es';
-import { faker } from '@faker-js/faker';
+import { createPinia, setActivePinia } from 'pinia';
 import SettingsSso from '@/views/SettingsSso.vue';
+
+import { renderComponent, retry } from '@/__tests__/utils';
+import { setupServer } from '@/__tests__/server';
+import { afterAll, beforeAll } from 'vitest';
+import { useSettingsStore } from '@/stores';
+import userEvent from '@testing-library/user-event';
 import { useSSOStore } from '@/stores/sso.store';
-import { STORES } from '@/constants';
-import { SETTINGS_STORE_DEFAULT_STATE, waitAllPromises } from '@/__tests__/utils';
 import { i18nInstance } from '@/plugins/i18n';
-import type { SamlPreferences, SamlPreferencesExtractedData } from '@/Interface';
 
-let pinia: ReturnType<typeof createTestingPinia>;
+let pinia: ReturnType<typeof createPinia>;
 let ssoStore: ReturnType<typeof useSSOStore>;
-
-const samlConfig: SamlPreferences & SamlPreferencesExtractedData = {
-	metadata: '<?xml version="1.0"?>',
-	entityID: faker.internet.url(),
-	returnUrl: faker.internet.url(),
-};
-
-const renderComponent = (renderOptions: Parameters<typeof render>[1] = {}) =>
-	render(
-		SettingsSso,
-		merge(
-			{
-				pinia,
-				i18n: i18nInstance,
-			},
-			renderOptions,
-		),
-		(vue) => {
-			vue.use(PiniaVuePlugin);
-		},
-	);
+let server: ReturnType<typeof setupServer>;
 
 describe('SettingsSso', () => {
-	beforeEach(() => {
-		pinia = createTestingPinia({
-			initialState: {
-				[STORES.SETTINGS]: {
-					settings: merge({}, SETTINGS_STORE_DEFAULT_STATE.settings),
-				},
-			},
-		});
-		ssoStore = useSSOStore(pinia);
+	beforeAll(() => {
+		server = setupServer();
+	});
+
+	beforeEach(async () => {
+		pinia = createPinia();
+		setActivePinia(pinia);
+
+		window.open = vi.fn();
+
+		await useSettingsStore().getSettings();
+		ssoStore = useSSOStore();
 	});
 
 	afterEach(() => {
 		vi.clearAllMocks();
 	});
 
+	afterAll(() => {
+		server.shutdown();
+	});
+
 	it('should render paywall state when there is no license', () => {
-		const { getByTestId, queryByTestId, queryByRole } = renderComponent();
+		const { getByTestId, queryByTestId, queryByRole } = renderComponent(SettingsSso, {
+			pinia,
+			i18n: i18nInstance,
+		});
 
 		expect(queryByRole('checkbox')).not.toBeInTheDocument();
 		expect(queryByTestId('sso-content-licensed')).not.toBeInTheDocument();
@@ -62,7 +50,10 @@ describe('SettingsSso', () => {
 	it('should render licensed content', () => {
 		vi.spyOn(ssoStore, 'isEnterpriseSamlEnabled', 'get').mockReturnValue(true);
 
-		const { getByTestId, queryByTestId, getByRole } = renderComponent();
+		const { getByTestId, queryByTestId, getByRole } = renderComponent(SettingsSso, {
+			pinia,
+			i18n: i18nInstance,
+		});
 
 		expect(getByRole('checkbox')).toBeInTheDocument();
 		expect(getByTestId('sso-content-licensed')).toBeInTheDocument();
@@ -71,19 +62,34 @@ describe('SettingsSso', () => {
 
 	it('should enable activation checkbox and test button if data is already saved', async () => {
 		vi.spyOn(ssoStore, 'isEnterpriseSamlEnabled', 'get').mockReturnValue(true);
-		vi.spyOn(ssoStore, 'getSamlConfig').mockResolvedValue(samlConfig);
 
-		const { getByRole, getByTestId } = renderComponent();
-		await waitAllPromises();
+		const { container, getByTestId, getByRole } = renderComponent(SettingsSso, {
+			pinia,
+			i18n: i18nInstance,
+		});
+
+		await retry(() =>
+			expect(container.querySelector('textarea[name="metadata"]')).toHaveValue(
+				'<?xml version="1.0"?>',
+			),
+		);
 
 		expect(getByRole('checkbox')).toBeEnabled();
 		expect(getByTestId('sso-test')).toBeEnabled();
 	});
 
 	it('should enable activation checkbox after data is saved', async () => {
+		await ssoStore.saveSamlConfig({ metadata: '' });
+
 		vi.spyOn(ssoStore, 'isEnterpriseSamlEnabled', 'get').mockReturnValue(true);
 
-		const { getByRole, getAllByRole, getByTestId } = renderComponent();
+		const saveSpy = vi.spyOn(ssoStore, 'saveSamlConfig');
+		const getSpy = vi.spyOn(ssoStore, 'getSamlConfig');
+
+		const { container, getByRole, getByTestId } = renderComponent(SettingsSso, {
+			pinia,
+			i18n: i18nInstance,
+		});
 		const checkbox = getByRole('checkbox');
 		const btnSave = getByTestId('sso-save');
 		const btnTest = getByTestId('sso-test');
@@ -93,8 +99,12 @@ describe('SettingsSso', () => {
 			expect(el).toBeDisabled();
 		});
 
+		const xmlRadioButton = getByTestId('radio-button-xml');
+		await userEvent.click(xmlRadioButton);
+
+		await retry(() => expect(container.querySelector('textarea[name="metadata"]')).toBeVisible());
 		await userEvent.type(
-			getAllByRole('textbox').find((el) => el.getAttribute('name') === 'metadata')!,
+			container.querySelector('textarea[name="metadata"]')!,
 			'<?xml version="1.0"?>',
 		);
 
@@ -102,14 +112,9 @@ describe('SettingsSso', () => {
 		expect(btnTest).toBeDisabled();
 		expect(btnSave).toBeEnabled();
 
-		const saveSpy = vi.spyOn(ssoStore, 'saveSamlConfig');
-		const getSpy = vi.spyOn(ssoStore, 'getSamlConfig').mockResolvedValue(samlConfig);
 		await userEvent.click(btnSave);
 
 		expect(saveSpy).toHaveBeenCalled();
 		expect(getSpy).toHaveBeenCalled();
-		expect(checkbox).toBeEnabled();
-		expect(btnTest).toBeEnabled();
-		expect(btnSave).toBeEnabled();
 	});
 });