fix: allow users to set password first time (#36627)

Co-authored-by: Guilherme Gazzo <guilhermegazzo@gmail.com>

.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

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',

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',
+		),
 	),
 );

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'),

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
 	});