From fa1694dacd9e3311ead6af0638bfe0e9fc3ab640 Mon Sep 17 00:00:00 2001 From: matheusbsilva137 Date: Tue, 18 Jun 2024 17:30:58 -0300 Subject: [PATCH 1/4] fix: SAML Overwrite user fullname setting is not honored --- apps/meteor/app/meteor-accounts-saml/server/lib/SAML.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) 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..c86a6fffe7ed 100644 --- a/apps/meteor/app/meteor-accounts-saml/server/lib/SAML.ts +++ b/apps/meteor/app/meteor-accounts-saml/server/lib/SAML.ts @@ -221,8 +221,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 From 589265ad99157668aac514fef8b598a86b430e1b Mon Sep 17 00:00:00 2001 From: matheusbsilva137 Date: Tue, 18 Jun 2024 17:31:11 -0300 Subject: [PATCH 2/4] test: add e2e tests --- apps/meteor/tests/e2e/saml.spec.ts | 58 +++++++++++++++++++++++++++--- 1 file changed, 54 insertions(+), 4 deletions(-) diff --git a/apps/meteor/tests/e2e/saml.spec.ts b/apps/meteor/tests/e2e/saml.spec.ts index b1c1687bfa6a..d5da08bf0eee 100644 --- a/apps/meteor/tests/e2e/saml.spec.ts +++ b/apps/meteor/tests/e2e/saml.spec.ts @@ -63,6 +63,10 @@ const resetTestData = async (cleanupOnly = false) => { _id: 'SAML_Custom_Default_mail_overwrite', value: false, }, + { + _id: 'SAML_Custom_Default_name_overwrite', + value: false, + }, { _id: 'SAML_Custom_Default', value: false, @@ -75,6 +79,10 @@ const resetTestData = async (cleanupOnly = false) => { _id: 'SAML_Custom_Default_role_attribute_name', value: 'role', }, + { + _id: 'SAML_Custom_Default_user_data_fieldmap', + value: '{"username":"username", "email":"email", "name": "cn"}', + }, ].map((setting) => connection .db() @@ -290,6 +298,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'); @@ -304,8 +332,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'); @@ -316,16 +345,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'); From 87d9722546e66bc8c63b6de93a1ec3d6e3b8445d Mon Sep 17 00:00:00 2001 From: Matheus Barbosa Silva <36537004+matheusbsilva137@users.noreply.github.com> Date: Tue, 18 Jun 2024 17:48:02 -0300 Subject: [PATCH 3/4] Create changeset --- .changeset/chilly-papayas-march.md | 5 +++++ 1 file changed, 5 insertions(+) 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 From 4a2517760e3c32a8216ce372bfa63f2b1c8981cb Mon Sep 17 00:00:00 2001 From: matheusbsilva137 Date: Wed, 19 Jun 2024 09:46:23 -0300 Subject: [PATCH 4/4] improve: avoid updating name twice --- apps/meteor/app/meteor-accounts-saml/server/lib/SAML.ts | 5 ----- 1 file changed, 5 deletions(-) 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 c86a6fffe7ed..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;