diff --git a/.changeset/thick-ravens-flow.md b/.changeset/thick-ravens-flow.md new file mode 100644 index 00000000000..e0c6ee94ff8 --- /dev/null +++ b/.changeset/thick-ravens-flow.md @@ -0,0 +1,5 @@ +--- +"@rocket.chat/meteor": patch +--- + +fixes an issue where using `/v1/users.updateOwnBasicInfo`, the user was not be able to set the password (not change), even when required diff --git a/apps/meteor/app/lib/server/functions/saveUser/validateUserEditing.ts b/apps/meteor/app/lib/server/functions/saveUser/validateUserEditing.ts index 2fd4c1ba9e7..c9f3ddbe296 100644 --- a/apps/meteor/app/lib/server/functions/saveUser/validateUserEditing.ts +++ b/apps/meteor/app/lib/server/functions/saveUser/validateUserEditing.ts @@ -45,7 +45,7 @@ export async function validateUserEditing(userId: IUser['_id'], userData: Update if ( isEditingField(user.username, userData.username) && !settings.get('Accounts_AllowUsernameChange') && - (!canEditOtherUserInfo || (editingMyself && user.username)) + (editingMyself ? user.username : !canEditOtherUserInfo) ) { throw new MeteorError('error-action-not-allowed', 'Edit username is not allowed', { method: 'insertOrUpdateUser', @@ -87,7 +87,11 @@ export async function validateUserEditing(userId: IUser['_id'], userData: Update }); } - if (userData.password && !settings.get('Accounts_AllowPasswordChange') && (!canEditOtherUserPassword || editingMyself)) { + if ( + userData.password && + !settings.get('Accounts_AllowPasswordChange') && + (editingMyself ? user.services?.password || !user.requirePasswordChange : !canEditOtherUserPassword) + ) { throw new MeteorError('error-action-not-allowed', 'Edit user password is not allowed', { method: 'insertOrUpdateUser', action: 'Update_user', diff --git a/apps/meteor/tests/e2e/containers/saml/config/simplesamlphp/authsources.php b/apps/meteor/tests/e2e/containers/saml/config/simplesamlphp/authsources.php index ee5a1928f3a..03889c7c3e6 100644 --- a/apps/meteor/tests/e2e/containers/saml/config/simplesamlphp/authsources.php +++ b/apps/meteor/tests/e2e/containers/saml/config/simplesamlphp/authsources.php @@ -37,5 +37,17 @@ $config = array( 'email' => 'samluser4@example.com', 'role' => 'saml-role', ), + 'samlusernoname:password' => array( + 'uid' => array('5'), + 'cn' => 'Saml User No Username', + 'eduPersonAffiliation' => array('group2'), + 'email' => 'samlusernoname@example.com', + ), + 'samlusernoname2:password' => array( + 'uid' => array('6'), + 'cn' => 'Saml User No Username 2', + 'eduPersonAffiliation' => array('group2'), + 'email' => 'samlusernoname2@example.com', + ), ), ); \ No newline at end of file diff --git a/apps/meteor/tests/e2e/fixtures/userStates.ts b/apps/meteor/tests/e2e/fixtures/userStates.ts index d2793dbf6a5..cafabd2d2a7 100644 --- a/apps/meteor/tests/e2e/fixtures/userStates.ts +++ b/apps/meteor/tests/e2e/fixtures/userStates.ts @@ -125,6 +125,8 @@ export const Users = { samluser1: generateContext('samluser1'), samluser2: generateContext('samluser2'), samluser4: generateContext('samluser4'), + samlusernoname: generateContext('custom_saml_username'), + samlusernoname2: generateContext('custom_saml_username2'), userForSamlMerge: generateContext('user_for_saml_merge'), userForSamlMerge2: generateContext('user_for_saml_merge2'), admin: generateContext('rocketchat.internal.admin.test'), diff --git a/apps/meteor/tests/e2e/saml.spec.ts b/apps/meteor/tests/e2e/saml.spec.ts index d7e0904fc2c..bce906f3b32 100644 --- a/apps/meteor/tests/e2e/saml.spec.ts +++ b/apps/meteor/tests/e2e/saml.spec.ts @@ -25,9 +25,19 @@ const resetTestData = async ({ api, cleanupOnly = false }: { api?: any; cleanupO // This is needed because those tests will modify this data and running them a second time would trigger different code paths const connection = await MongoClient.connect(constants.URL_MONGODB); - const usernamesToDelete = [Users.userForSamlMerge, Users.userForSamlMerge2, Users.samluser1, Users.samluser2, Users.samluser4].map( - ({ data: { username } }) => username, - ); + const usernamesToDelete = [ + ...[ + Users.userForSamlMerge, + Users.userForSamlMerge2, + Users.samluser1, + Users.samluser2, + Users.samluser4, + Users.samlusernoname, + Users.samlusernoname2, + ].map(({ data: { username } }) => username), + 'custom_saml_username', + 'custom_saml_username2', + ]; await connection .db() .collection('users') @@ -50,6 +60,7 @@ const resetTestData = async ({ api, cleanupOnly = false }: { api?: any; cleanupO const settings = [ { _id: 'Accounts_AllowAnonymousRead', value: false }, + { _id: 'Accounts_AllowUsernameChange', value: true }, { _id: 'SAML_Custom_Default_logout_behaviour', value: 'SAML' }, { _id: 'SAML_Custom_Default_immutable_property', value: 'EMail' }, { _id: 'SAML_Custom_Default_mail_overwrite', value: false }, @@ -229,6 +240,27 @@ test.describe('SAML', () => { }); }; + const doLoginStepWithUsernameSelection = async (page: Page, username: string) => { + await test.step('expect successful login without username to show username selection screen', async () => { + await poRegistration.btnLoginWithSaml.click(); + // Redirect to Idp + await expect(page).toHaveURL(/.*\/simplesaml\/module.php\/core\/loginuserpass.php.*/); + + // Fill username and password + await page.getByLabel('Username').fill(username); + await page.getByLabel('Password').fill('password'); + await page.locator('role=button[name="Login"]').click(); + + // Should redirect to username selection screen + await expect(poRegistration.username).toBeVisible(); + }); + }; + + const doUsernameSelection = async (customUsername: string) => { + await poRegistration.username.fill(customUsername); + await poRegistration.btnRegisterConfirmUsername.click(); + }; + const doLogoutStep = async (page: Page) => { await test.step('logout', async () => { await page.getByRole('button', { name: 'User menu' }).click(); @@ -465,6 +497,64 @@ test.describe('SAML', () => { await page2.close(); }); + test('Login - User without username can set initial username during first login', async ({ page, api }) => { + await doLoginStepWithUsernameSelection(page, 'samlusernoname2'); + + await test.step('expect to be redirected to the username selection page and set the username', async () => { + await expect(poRegistration.btnRegisterConfirmUsername).toBeVisible(); + await doUsernameSelection('custom_saml_username2'); + }); + + await test.step('expect to be redirected to the homepage after succesful login', async () => { + await expect(page).toHaveURL('/home'); + await expect(page.getByRole('button', { name: 'User menu' })).toBeVisible(); + }); + + await test.step('expect user data to have been created with custom username', async () => { + const user = await getUserInfo(api, 'custom_saml_username2'); + + expect(user).toBeDefined(); + expect(user?.username).toBe('custom_saml_username2'); + expect(user?.name).toBe('Saml User No Username 2'); + expect(user?.emails).toBeDefined(); + expect(user?.emails?.[0].address).toBe('samlusernoname2@example.com'); + }); + }); + + test.describe('Login - Username selection when allow username change is disabled', () => { + test.beforeAll(async ({ api }) => { + await expect((await setSettingValueById(api, 'Accounts_AllowUsernameChange', false)).status()).toBe(200); + }); + + test.afterAll(async ({ api }) => { + await expect((await setSettingValueById(api, 'Accounts_AllowUsernameChange', true)).status()).toBe(200); + }); + + test('User without username can set initial username when username changes are disabled', async ({ page, api }) => { + await doLoginStepWithUsernameSelection(page, 'samlusernoname'); + + await test.step('expect to be redirected to the username selection page and set the username', async () => { + await expect(poRegistration.btnRegisterConfirmUsername).toBeVisible(); + await doUsernameSelection('custom_saml_username'); + }); + + await test.step('expect to be redirected to the homepage after succesful login', async () => { + await expect(page).toHaveURL('/home'); + await expect(page.getByRole('button', { name: 'User menu' })).toBeVisible(); + }); + + await test.step('expect user data to have been created with the custom username', async () => { + const user = await getUserInfo(api, 'custom_saml_username'); + + expect(user).toBeDefined(); + expect(user?.username).toBe('custom_saml_username'); + expect(user?.name).toBe('Saml User No Username'); + expect(user?.emails).toBeDefined(); + expect(user?.emails?.[0].address).toBe('samlusernoname@example.com'); + }); + }); + }); + test.fixme('User Merge - By Custom Identifier', async () => { // Test user merge with a custom identifier configured in the fieldmap });