Merge pull request #4223 from senky/ticket/10432

[ticket/10432] Don't require username when user forgets password

6 files changed, 121 insertions, 75 deletions

diff --git a/phpBB/includes/ucp/ucp_remind.php b/phpBB/includes/ucp/ucp_remind.php
index f46df99edb..e50428bfea 100644
--- a/phpBB/includes/ucp/ucp_remind.php
+++ b/phpBB/includes/ucp/ucp_remind.php
@@ -50,11 +50,16 @@ class ucp_remind
 				trigger_error('FORM_INVALID');
 			}
 
+			if (empty($email))
+			{
+				trigger_error('NO_EMAIL_USER');
+			}
+
 			$sql_array = array(
 				'SELECT'	=> 'user_id, username, user_permissions, user_email, user_jabber, user_notify_type, user_type, user_lang, user_inactive_reason',
 				'FROM'		=> array(USERS_TABLE => 'u'),
-				'WHERE'		=> "user_email_hash = '" . $db->sql_escape(phpbb_email_hash($email)) . "'
-									AND username_clean = '" . $db->sql_escape(utf8_clean_string($username)) . "'"
+				'WHERE'		=> "user_email_hash = '" . $db->sql_escape(phpbb_email_hash($email)) . "'" .
+					(!empty($username) ? " AND username_clean = '" . $db->sql_escape(utf8_clean_string($username)) . "'" : ''),
 			);
 
 			/**
@@ -74,82 +79,87 @@ class ucp_remind
 			extract($phpbb_dispatcher->trigger_event('core.ucp_remind_modify_select_sql', compact($vars)));
 
 			$sql = $db->sql_build_query('SELECT', $sql_array);
-			$result = $db->sql_query($sql);
-			$user_row = $db->sql_fetchrow($result);
-			$db->sql_freeresult($result);
+			$result = $db->sql_query_limit($sql, 2); // don't waste resources on more rows than we need
+			$rowset = $db->sql_fetchrowset($result);
 
-			if (!$user_row)
+			if (count($rowset) > 1)
 			{
-				trigger_error('NO_EMAIL_USER');
-			}
+				$db->sql_freeresult($result);
 
-			if ($user_row['user_type'] == USER_IGNORE)
-			{
-				trigger_error('NO_USER');
+				$template->assign_vars(array(
+					'USERNAME_REQUIRED'	=> true,
+					'EMAIL'				=> $email,
+				));
 			}
-
-			if ($user_row['user_type'] == USER_INACTIVE)
+			else
 			{
-				if ($user_row['user_inactive_reason'] == INACTIVE_MANUAL)
+				$message = $user->lang['PASSWORD_UPDATED_IF_EXISTED'] . '<br /><br />' . sprintf($user->lang['RETURN_INDEX'], '<a href="' . append_sid("{$phpbb_root_path}index.$phpEx") . '">', '</a>');
+
+				if (empty($rowset))
 				{
-					trigger_error('ACCOUNT_DEACTIVATED');
+					trigger_error($message);
 				}
-				else
+
+				$user_row = $rowset[0];
+				$db->sql_freeresult($result);
+
+				if (!$user_row)
 				{
-					trigger_error('ACCOUNT_NOT_ACTIVATED');
+					trigger_error($message);
 				}
-			}
 
-			// Check users permissions
-			$auth2 = new \phpbb\auth\auth();
-			$auth2->acl($user_row);
+				if ($user_row['user_type'] == USER_IGNORE || $user_row['user_type'] == USER_INACTIVE)
+				{
+					trigger_error($message);
+				}
 
-			if (!$auth2->acl_get('u_chgpasswd'))
-			{
-				send_status_line(403, 'Forbidden');
-				trigger_error('NO_AUTH_PASSWORD_REMINDER');
-			}
+				// Check users permissions
+				$auth2 = new \phpbb\auth\auth();
+				$auth2->acl($user_row);
 
-			$server_url = generate_board_url();
+				if (!$auth2->acl_get('u_chgpasswd'))
+				{
+					trigger_error($message);
+				}
 
-			// Make password at least 8 characters long, make it longer if admin wants to.
-			// gen_rand_string() however has a limit of 12 or 13.
-			$user_password = gen_rand_string_friendly(max(8, mt_rand((int) $config['min_pass_chars'], (int) $config['max_pass_chars'])));
+				$server_url = generate_board_url();
 
-			// For the activation key a random length between 6 and 10 will do.
-			$user_actkey = gen_rand_string(mt_rand(6, 10));
+				// Make password at least 8 characters long, make it longer if admin wants to.
+				// gen_rand_string() however has a limit of 12 or 13.
+				$user_password = gen_rand_string_friendly(max(8, mt_rand((int) $config['min_pass_chars'], (int) $config['max_pass_chars'])));
 
-			// Instantiate passwords manager
-			/* @var $manager \phpbb\passwords\manager */
-			$passwords_manager = $phpbb_container->get('passwords.manager');
+				// For the activation key a random length between 6 and 10 will do.
+				$user_actkey = gen_rand_string(mt_rand(6, 10));
 
-			$sql = 'UPDATE ' . USERS_TABLE . "
-				SET user_newpasswd = '" . $db->sql_escape($passwords_manager->hash($user_password)) . "', user_actkey = '" . $db->sql_escape($user_actkey) . "'
-				WHERE user_id = " . $user_row['user_id'];
-			$db->sql_query($sql);
+				// Instantiate passwords manager
+				/* @var $manager \phpbb\passwords\manager */
+				$passwords_manager = $phpbb_container->get('passwords.manager');
 
-			include_once($phpbb_root_path . 'includes/functions_messenger.' . $phpEx);
+				$sql = 'UPDATE ' . USERS_TABLE . "
+					SET user_newpasswd = '" . $db->sql_escape($passwords_manager->hash($user_password)) . "', user_actkey = '" . $db->sql_escape($user_actkey) . "'
+					WHERE user_id = " . $user_row['user_id'];
+				$db->sql_query($sql);
 
-			$messenger = new messenger(false);
+				include_once($phpbb_root_path . 'includes/functions_messenger.' . $phpEx);
 
-			$messenger->template('user_activate_passwd', $user_row['user_lang']);
+				$messenger = new messenger(false);
 
-			$messenger->set_addresses($user_row);
+				$messenger->template('user_activate_passwd', $user_row['user_lang']);
 
-			$messenger->anti_abuse_headers($config, $user);
+				$messenger->set_addresses($user_row);
 
-			$messenger->assign_vars(array(
-				'USERNAME'		=> htmlspecialchars_decode($user_row['username']),
-				'PASSWORD'		=> htmlspecialchars_decode($user_password),
-				'U_ACTIVATE'	=> "$server_url/ucp.$phpEx?mode=activate&u={$user_row['user_id']}&k=$user_actkey")
-			);
+				$messenger->anti_abuse_headers($config, $user);
 
-			$messenger->send($user_row['user_notify_type']);
+				$messenger->assign_vars(array(
+					'USERNAME'		=> htmlspecialchars_decode($user_row['username']),
+					'PASSWORD'		=> htmlspecialchars_decode($user_password),
+					'U_ACTIVATE'	=> "$server_url/ucp.$phpEx?mode=activate&u={$user_row['user_id']}&k=$user_actkey")
+				);
 
-			meta_refresh(3, append_sid("{$phpbb_root_path}index.$phpEx"));
+				$messenger->send($user_row['user_notify_type']);
 
-			$message = $user->lang['PASSWORD_UPDATED'] . '<br /><br />' . sprintf($user->lang['RETURN_INDEX'], '<a href="' . append_sid("{$phpbb_root_path}index.$phpEx") . '">', '</a>');
-			trigger_error($message);
+				trigger_error($message);
+			}
 		}
 
 		$template->assign_vars(array(
diff --git a/phpBB/language/en/common.php b/phpBB/language/en/common.php
index 213563aea0..a037c5bfe8 100644
--- a/phpBB/language/en/common.php
+++ b/phpBB/language/en/common.php
@@ -62,7 +62,6 @@ $lang = array_merge($lang, array(
 
 	'ACCOUNT_ALREADY_ACTIVATED'		=> 'Your account has already been activated.',
 	'ACCOUNT_DEACTIVATED'			=> 'Your account has been manually deactivated and is only able to be reactivated by an administrator.',
-	'ACCOUNT_NOT_ACTIVATED'			=> 'Your account has not been activated yet.',
 	'ACP'							=> 'Administration Control Panel',
 	'ACP_SHORT'						=> 'ACP',
 	'ACTIVE'						=> 'active',
diff --git a/phpBB/language/en/ucp.php b/phpBB/language/en/ucp.php
index 64b624ff3f..5875099fb8 100644
--- a/phpBB/language/en/ucp.php
+++ b/phpBB/language/en/ucp.php
@@ -373,7 +373,6 @@ $lang = array_merge($lang, array(
 	'NO_AUTH_EDIT_MESSAGE'			=> 'You are not authorised to edit private messages.',
 	'NO_AUTH_FORWARD_MESSAGE'		=> 'You are not authorised to forward private messages.',
 	'NO_AUTH_GROUP_MESSAGE'			=> 'You are not authorised to send private messages to groups.',
-	'NO_AUTH_PASSWORD_REMINDER'		=> 'You are not authorised to request a new password.',
 	'NO_AUTH_PROFILEINFO'			=> 'You are not authorised to change your profile information.',
 	'NO_AUTH_READ_HOLD_MESSAGE'		=> 'You are not authorised to read private messages that are on hold.',
 	'NO_AUTH_READ_MESSAGE'			=> 'You are not authorised to read private messages.',
@@ -387,6 +386,7 @@ $lang = array_merge($lang, array(
 	'NO_BOOKMARKS_SELECTED'		=> 'You have selected no bookmarks.',
 	'NO_EDIT_READ_MESSAGE'		=> 'Private message cannot be edited because it has already been read.',
 	'NO_EMAIL_USER'				=> 'The email/username information submitted could not be found.',
+	'EMAIL_NOT_UNIQUE'			=> 'Email you specified is used by multiple users. You must specify username as well.',
 	'NO_FOES'					=> 'No foes currently defined',
 	'NO_FRIENDS'				=> 'No friends currently defined',
 	'NO_FRIENDS_OFFLINE'		=> 'No friends offline',
@@ -412,7 +412,7 @@ $lang = array_merge($lang, array(
 	'PASS_TYPE_SYMBOL_EXPLAIN'	=> 'Password must be between %1$s and %2$s long, must contain letters in mixed case, must contain numbers and must contain symbols.',
 	'PASSWORD'					=> 'Password',
 	'PASSWORD_ACTIVATED'		=> 'Your new password has been activated.',
-	'PASSWORD_UPDATED'			=> 'A new password was sent to your registered email address.',
+	'PASSWORD_UPDATED_IF_EXISTED'	=> 'If your account exists, a new password was sent to your registered email address. If you do not receive an email, it may be because you are banned, your account is not activated, or you are not allowed to change your password. Contact admin if any of those reasons apply. Also, check your spam filter.',
 	'PERMISSIONS_RESTORED'		=> 'Successfully restored original permissions.',
 	'PERMISSIONS_TRANSFERRED'	=> 'Successfully transferred permissions from <strong>%s</strong>, you are now able to browse the board with this user’s permissions.<br />Please note that admin permissions were not transferred. You are able to revert to your permission set at any time.',
 	'PM_DISABLED'				=> 'Private messaging has been disabled on this board.',
diff --git a/phpBB/styles/prosilver/template/ucp_remind.html b/phpBB/styles/prosilver/template/ucp_remind.html
index 0ab1251d9e..8b700de430 100644
--- a/phpBB/styles/prosilver/template/ucp_remind.html
+++ b/phpBB/styles/prosilver/template/ucp_remind.html
@@ -9,14 +9,19 @@
 		<h2>{L_SEND_PASSWORD}</h2>
 
 		<fieldset>
+			{% if USERNAME_REQUIRED %}
+				<p class="error">{{ lang('EMAIL_NOT_UNIQUE') }}</p>
+			{% endif %}
 		<dl>
-			<dt><label for="username">{L_USERNAME}{L_COLON}</label></dt>
-			<dd><input class="inputbox narrow" type="text" name="username" id="username" size="25" /></dd>
+			<dt><label for="email">{L_EMAIL_ADDRESS}{L_COLON}</label><br /><span>{L_EMAIL_REMIND}</span></dt>
+			<dd><input class="inputbox narrow" type="email" name="email" id="email" size="25" maxlength="100" value="{{ EMAIL }}" autofocus /></dd>
 		</dl>
+		{% if USERNAME_REQUIRED %}
 		<dl>
-			<dt><label for="email">{L_EMAIL_ADDRESS}{L_COLON}</label><br /><span>{L_EMAIL_REMIND}</span></dt>
-			<dd><input class="inputbox narrow" type="email" name="email" id="email" size="25" maxlength="100" /></dd>
+			<dt><label for="username">{L_USERNAME}{L_COLON}</label></dt>
+			<dd><input class="inputbox narrow" type="text" name="username" id="username" size="25" /></dd>
 		</dl>
+		{% endif %}
 		<dl>
 			<dt>&nbsp;</dt>
 			<dd>{S_HIDDEN_FIELDS}<input type="submit" name="submit" id="submit" class="button1" value="{L_SUBMIT}" tabindex="2" />&nbsp; <input type="reset" value="{L_RESET}" name="reset" class="button2" /></dd>
diff --git a/tests/functional/user_password_reset_test.php b/tests/functional/user_password_reset_test.php
index 3da78407cf..2361eed066 100644
--- a/tests/functional/user_password_reset_test.php
+++ b/tests/functional/user_password_reset_test.php
@@ -21,25 +21,56 @@ class phpbb_functional_user_password_reset_test extends phpbb_functional_test_ca
 	public function test_password_reset()
 	{
 		$this->add_lang('ucp');
-		$user_id = $this->create_user('reset-password-test-user');
+		$user_id = $this->create_user('reset-password-test-user', 'reset-password-test-user@test.com');
 
+		// test without email
+		$crawler = self::request('GET', "ucp.php?mode=sendpassword&sid={$this->sid}");
+		$form = $crawler->selectButton('submit')->form();
+		$crawler = self::submit($form);
+		$this->assertContainsLang('NO_EMAIL_USER', $crawler->text());
+
+		// test with non-existent email
 		$crawler = self::request('GET', "ucp.php?mode=sendpassword&sid={$this->sid}");
 		$form = $crawler->selectButton('submit')->form(array(
-			'username'	=> 'reset-password-test-user',
+			'email'	=> 'non-existent@email.com',
 		));
 		$crawler = self::submit($form);
-		$this->assertContainsLang('NO_EMAIL_USER', $crawler->text());
+		$this->assertContainsLang('PASSWORD_UPDATED_IF_EXISTED', $crawler->text());
 
+		// test with correct email
 		$crawler = self::request('GET', "ucp.php?mode=sendpassword&sid={$this->sid}");
 		$form = $crawler->selectButton('submit')->form(array(
-			'username'	=> 'reset-password-test-user',
-			'email'		=> 'nobody@example.com',
+			'email'		=> 'reset-password-test-user@test.com',
+		));
+		$crawler = self::submit($form);
+		$this->assertContainsLang('PASSWORD_UPDATED_IF_EXISTED', $crawler->text());
+
+		// Check if columns in database were updated for password reset
+		$this->get_user_data('reset-password-test-user');
+		$this->assertNotNull($this->user_data['user_actkey']);
+		$this->assertNotNull($this->user_data['user_newpasswd']);
+
+		// Create another user with the same email
+		$this->create_user('reset-password-test-user1', 'reset-password-test-user@test.com');
+
+		// Test that username is now also required
+		$crawler = self::request('GET', "ucp.php?mode=sendpassword&sid={$this->sid}");
+		$form = $crawler->selectButton('submit')->form(array(
+			'email'		=> 'reset-password-test-user@test.com',
+		));
+		$crawler = self::submit($form);
+		$this->assertContainsLang('EMAIL_NOT_UNIQUE', $crawler->text());
+
+		// Provide both username and email
+		$form = $crawler->selectButton('submit')->form(array(
+			'email'		=> 'reset-password-test-user@test.com',
+			'username'	=> 'reset-password-test-user1',
 		));
 		$crawler = self::submit($form);
-		$this->assertContainsLang('PASSWORD_UPDATED', $crawler->text());
+		$this->assertContainsLang('PASSWORD_UPDATED_IF_EXISTED', $crawler->text());
 
 		// Check if columns in database were updated for password reset
-		$this->get_user_data();
+		$this->get_user_data('reset-password-test-user1');
 		$this->assertNotNull($this->user_data['user_actkey']);
 		$this->assertNotNull($this->user_data['user_newpasswd']);
 
@@ -73,7 +104,7 @@ class phpbb_functional_user_password_reset_test extends phpbb_functional_test_ca
 	public function test_activate_new_password($expected, $user_id, $act_key)
 	{
 		$this->add_lang('ucp');
-		$this->get_user_data();
+		$this->get_user_data('reset-password-test-user');
 		$user_id = (!$user_id) ? $this->user_data['user_id'] : $user_id;
 		$act_key = (!$act_key) ? $this->user_data['user_actkey'] : $act_key;
 
@@ -119,7 +150,7 @@ class phpbb_functional_user_password_reset_test extends phpbb_functional_test_ca
 	public function test_acivateAfterDeactivate()
 	{
 		// User is active, actkey should not exist
-		$this->get_user_data();
+		$this->get_user_data('reset-password-test-user');
 		$this->assertEmpty($this->user_data['user_actkey']);
 
 		$this->login();
@@ -143,7 +174,7 @@ class phpbb_functional_user_password_reset_test extends phpbb_functional_test_ca
 		$crawler = self::request('GET', preg_replace('#(.+)(adm/index.php.+)#', '$2', $link->getUri()));
 
 		// Ensure again that actkey is empty after deactivation
-		$this->get_user_data();
+		$this->get_user_data('reset-password-test-user');
 		$this->assertEmpty($this->user_data['user_actkey']);
 
 		// Force reactivation of account and check that act key is not empty anymore
@@ -152,16 +183,16 @@ class phpbb_functional_user_password_reset_test extends phpbb_functional_test_ca
 		$crawler = self::submit($form, array('action' => 'reactivate'));
 		$this->assertContainsLang('FORCE_REACTIVATION_SUCCESS', $crawler->filter('html')->text());
 
-		$this->get_user_data();
+		$this->get_user_data('reset-password-test-user');
 		$this->assertNotEmpty($this->user_data['user_actkey']);
 	}
 
-	protected function get_user_data()
+	protected function get_user_data($username)
 	{
 		$db = $this->get_db();
 		$sql = 'SELECT user_id, username, user_type, user_email, user_newpasswd, user_lang, user_notify_type, user_actkey, user_inactive_reason
 			FROM ' . USERS_TABLE . "
-			WHERE username = 'reset-password-test-user'";
+			WHERE username = '" . $db->sql_escape($username) . "'";
 		$result = $db->sql_query($sql);
 		$this->user_data = $db->sql_fetchrow($result);
 		$db->sql_freeresult($result);
diff --git a/tests/test_framework/phpbb_functional_test_case.php b/tests/test_framework/phpbb_functional_test_case.php
index 12a296a4bf..d4856f954a 100644
--- a/tests/test_framework/phpbb_functional_test_case.php
+++ b/tests/test_framework/phpbb_functional_test_case.php
@@ -550,9 +550,10 @@ class phpbb_functional_test_case extends phpbb_test_case
 	* Creates a new user with limited permissions
 	*
 	* @param string $username Also doubles up as the user's password
+	* @param string $email User email (defaults to nobody@example.com)
 	* @return int ID of created user
 	*/
-	protected function create_user($username)
+	protected function create_user($username, $email = 'nobody@example.com')
 	{
 		// Required by unique_id
 		global $config;
@@ -604,7 +605,7 @@ class phpbb_functional_test_case extends phpbb_test_case
 		$user_row = array(
 			'username' => $username,
 			'group_id' => 2,
-			'user_email' => 'nobody@example.com',
+			'user_email' => $email,
 			'user_type' => 0,
 			'user_lang' => 'en',
 			'user_timezone' => 'UTC',