From 125144271ab819f51db33bb941557afdcb82370b Mon Sep 17 00:00:00 2001 From: Matheus Barbosa Silva <36537004+matheusbsilva137@users.noreply.github.com> Date: Thu, 11 Jul 2024 16:46:06 -0300 Subject: [PATCH] fix: SAML "Overwrite user fullname" setting is not being honored (#32628) --- .changeset/chilly-papayas-march.md | 5 ++ .../meteor-accounts-saml/server/lib/SAML.ts | 9 +--- apps/meteor/tests/e2e/saml.spec.ts | 52 +++++++++++++++++-- 3 files changed, 55 insertions(+), 11 deletions(-) create mode 100644 .changeset/chilly-papayas-march.md diff --git a/.changeset/chilly-papayas-march.md b/.changeset/chilly-papayas-march.md new file mode 100644 index 000000000000..a7724b126695 --- /dev/null +++ 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 index 76747b599104..6e68518ef31c 100644 --- 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 index 272411ae1b39..fe1295ca0b4b 100644 --- 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');