fix: SAML "Overwrite user fullname" setting is not being honored (#32628

)
RocketChat · Jul 11, 2024 · 1251442 · 1251442
1 parent d7de0a9
commit 1251442
Show file tree

Hide file tree

Showing 3 changed files with 55 additions and 11 deletions.
diff --git a/.changeset/chilly-papayas-march.md b/.changeset/chilly-papayas-march.md
@@ -0,0 +1,5 @@
+---
+"@rocket.chat/meteor": patch
+---
+
+Fixed SAML users' full names being updated on login regardless of the "Overwrite user fullname (use idp attribute)" setting
diff --git a/apps/meteor/app/meteor-accounts-saml/server/lib/SAML.ts b/apps/meteor/app/meteor-accounts-saml/server/lib/SAML.ts
@@ -198,11 +198,6 @@ export class SAML {
 			updateData.emails = emails;
 		}
 
-		// Overwrite fullname if needed
-		if (nameOverwrite === true) {
-			updateData.name = fullName;
-		}
-
 		// When updating an user, we only update the roles if we received them from the mapping
 		if (userObject.roles?.length) {
 			updateData.roles = userObject.roles;
@@ -221,8 +216,8 @@ export class SAML {
 			},
 		);
 
-		if ((username && username !== user.username) || (fullName && fullName !== user.name)) {
-			await saveUserIdentity({ _id: user._id, name: fullName || undefined, username });
+		if ((username && username !== user.username) || (nameOverwrite && fullName && fullName !== user.name)) {
+			await saveUserIdentity({ _id: user._id, name: nameOverwrite ? fullName || undefined : user.name, username });
 		}
 
 		// sending token along with the userId

diff --git a/apps/meteor/tests/e2e/saml.spec.ts b/apps/meteor/tests/e2e/saml.spec.ts
@@ -51,9 +51,11 @@ const resetTestData = async ({ api, cleanupOnly = false }: { api?: any; cleanupO
 		{ _id: 'SAML_Custom_Default_logout_behaviour', value: 'SAML' },
 		{ _id: 'SAML_Custom_Default_immutable_property', value: 'EMail' },
 		{ _id: 'SAML_Custom_Default_mail_overwrite', value: false },
+		{ _id: 'SAML_Custom_Default_name_overwrite', value: false },
 		{ _id: 'SAML_Custom_Default', value: false },
 		{ _id: 'SAML_Custom_Default_role_attribute_sync', value: true },
 		{ _id: 'SAML_Custom_Default_role_attribute_name', value: 'role' },
+		{ _id: 'SAML_Custom_Default_user_data_fieldmap', value: '{"username":"username", "email":"email", "name": "cn"}' },
 		{ _id: 'SAML_Custom_Default_provider', value: 'test-sp' },
 		{ _id: 'SAML_Custom_Default_issuer', value: 'http://localhost:3000/_saml/metadata/test-sp' },
 		{ _id: 'SAML_Custom_Default_entry_point', value: 'http://localhost:8080/simplesaml/saml2/idp/SSOService.php' },
@@ -275,6 +277,26 @@ test.describe('SAML', () => {
 
 		await doLoginStep(page, 'samluser2');
 
+		await test.step('expect user data to have been mapped to the correct fields', async () => {
+			const user = await getUserInfo(api, 'samluser2');
+
+			expect(user).toBeDefined();
+			expect(user?._id).toBe('user_for_saml_merge');
+			expect(user?.username).toBe('samluser2');
+			expect(user?.name).toBe('user_for_saml_merge');
+			expect(user?.emails).toBeDefined();
+			expect(user?.emails?.[0].address).toBe('user_for_saml_merge@email.com');
+		});
+	});
+
+	test('User Merge - By Email with Name Override', async ({ page, api }) => {
+		await test.step('Configure SAML to identify users by email', async () => {
+			expect((await setSettingValueById(api, 'SAML_Custom_Default_immutable_property', 'EMail')).status()).toBe(200);
+			expect((await setSettingValueById(api, 'SAML_Custom_Default_name_overwrite', true)).status()).toBe(200);
+		});
+
+		await doLoginStep(page, 'samluser2');
+
 		await test.step('expect user data to have been mapped to the correct fields', async () => {
 			const user = await getUserInfo(api, 'samluser2');
 
@@ -289,8 +311,9 @@ test.describe('SAML', () => {
 
 	test('User Merge - By Username', async ({ page, api }) => {
 		await test.step('Configure SAML to identify users by username', async () => {
-			await expect((await setSettingValueById(api, 'SAML_Custom_Default_immutable_property', 'Username')).status()).toBe(200);
-			await expect((await setSettingValueById(api, 'SAML_Custom_Default_mail_overwrite', false)).status()).toBe(200);
+			expect((await setSettingValueById(api, 'SAML_Custom_Default_immutable_property', 'Username')).status()).toBe(200);
+			expect((await setSettingValueById(api, 'SAML_Custom_Default_name_overwrite', false)).status()).toBe(200);
+			expect((await setSettingValueById(api, 'SAML_Custom_Default_mail_overwrite', false)).status()).toBe(200);
 		});
 
 		await doLoginStep(page, 'samluser3');
@@ -301,16 +324,37 @@ test.describe('SAML', () => {
 			expect(user).toBeDefined();
 			expect(user?._id).toBe('user_for_saml_merge2');
 			expect(user?.username).toBe('user_for_saml_merge2');
-			expect(user?.name).toBe('Saml User 3');
+			expect(user?.name).toBe('user_for_saml_merge2');
 			expect(user?.emails).toBeDefined();
 			expect(user?.emails?.[0].address).toBe('user_for_saml_merge2@email.com');
 		});
 	});
 
 	test('User Merge - By Username with Email Override', async ({ page, api }) => {
+		await test.step('Configure SAML to identify users by username', async () => {
+			expect((await setSettingValueById(api, 'SAML_Custom_Default_immutable_property', 'Username')).status()).toBe(200);
+			expect((await setSettingValueById(api, 'SAML_Custom_Default_name_overwrite', false)).status()).toBe(200);
+			expect((await setSettingValueById(api, 'SAML_Custom_Default_mail_overwrite', true)).status()).toBe(200);
+		});
+
+		await doLoginStep(page, 'samluser3');
+
+		await test.step('expect user data to have been mapped to the correct fields', async () => {
+			const user = await getUserInfo(api, 'user_for_saml_merge2');
+
+			expect(user).toBeDefined();
+			expect(user?._id).toBe('user_for_saml_merge2');
+			expect(user?.username).toBe('user_for_saml_merge2');
+			expect(user?.name).toBe('user_for_saml_merge2');
+			expect(user?.emails).toBeDefined();
+			expect(user?.emails?.[0].address).toBe('samluser3@example.com');
+		});
+	});
+
+	test('User Merge - By Username with Name Override', async ({ page, api }) => {
 		await test.step('Configure SAML to identify users by username', async () => {
 			await expect((await setSettingValueById(api, 'SAML_Custom_Default_immutable_property', 'Username')).status()).toBe(200);
-			await expect((await setSettingValueById(api, 'SAML_Custom_Default_mail_overwrite', true)).status()).toBe(200);
+			await expect((await setSettingValueById(api, 'SAML_Custom_Default_name_overwrite', true)).status()).toBe(200);
 		});
 
 		await doLoginStep(page, 'samluser3');