diff --git a/core/modules/system/src/Tests/System/SiteMaintenanceTest.php b/core/modules/system/src/Tests/System/SiteMaintenanceTest.php index b89dc81..b864553 100644 --- a/core/modules/system/src/Tests/System/SiteMaintenanceTest.php +++ b/core/modules/system/src/Tests/System/SiteMaintenanceTest.php @@ -123,7 +123,8 @@ protected function testSiteMaintenance() { $path = substr($mails[0]['body'], $start, 66 + strlen($this->user->id())); // Log in with temporary login link. - $this->drupalPostForm($path, array(), t('Log in')); + $this->drupalGet($path); + $this->drupalPostForm(NULL, [], t('Log in')); $this->assertText($user_message); // Regression test to check if title displays in Bartik on maintenance page. diff --git a/core/modules/user/src/AccountForm.php b/core/modules/user/src/AccountForm.php index 8b0149e..14eb3d2 100644 --- a/core/modules/user/src/AccountForm.php +++ b/core/modules/user/src/AccountForm.php @@ -127,8 +127,9 @@ public function form(array $form, FormStateInterface $form_state) { // To skip the current password field, the user must have logged in via a // one-time link and have the token in the URL. Store this in $form_state // so it persists even on subsequent Ajax requests. - if (!$form_state->get('user_pass_reset')) { - $user_pass_reset = isset($_SESSION['pass_reset_' . $account->id()]) && Crypt::hashEquals($_SESSION['pass_reset_' . $account->id()], \Drupal::request()->query->get('pass-reset-token')); + $session = $this->getRequest()->getSession(); + if ($session && !$form_state->get('user_pass_reset') && $session->has('pass_reset_' . $account->id())) { + $user_pass_reset = Crypt::hashEquals($session->get('pass_reset_' . $account->id()), \Drupal::request()->query->get('pass-reset-token')); $form_state->set('user_pass_reset', $user_pass_reset); } @@ -391,8 +392,6 @@ public function submitForm(array &$form, FormStateInterface $form_state) { $user = $this->getEntity($form_state); // If there's a session set to the users id, remove the password reset tag // since a new password was saved. - if (isset($_SESSION['pass_reset_'. $user->id()])) { - unset($_SESSION['pass_reset_'. $user->id()]); - } + $this->getRequest()->getSession()->remove('pass_reset_' . $user->id()); } } diff --git a/core/modules/user/src/Controller/UserController.php b/core/modules/user/src/Controller/UserController.php index 772884b..f309b2f 100644 --- a/core/modules/user/src/Controller/UserController.php +++ b/core/modules/user/src/Controller/UserController.php @@ -10,11 +10,11 @@ use Drupal\Component\Utility\Crypt; use Drupal\Component\Utility\Xss; use Drupal\Core\Controller\ControllerBase; -use Drupal\Core\Datetime\DateFormatterInterface; use Drupal\user\UserDataInterface; use Drupal\user\UserInterface; use Drupal\user\UserStorageInterface; use Symfony\Component\DependencyInjection\ContainerInterface; +use Symfony\Component\HttpFoundation\Request; use Symfony\Component\HttpKernel\Exception\AccessDeniedHttpException; /** @@ -23,13 +23,6 @@ class UserController extends ControllerBase { /** - * The date formatter service. - * - * @var \Drupal\Core\Datetime\DateFormatterInterface - */ - protected $dateFormatter; - - /** * The user storage. * * @var \Drupal\user\UserStorageInterface @@ -46,15 +39,12 @@ class UserController extends ControllerBase { /** * Constructs a UserController object. * - * @param \Drupal\Core\Datetime\DateFormatterInterface $date_formatter - * The date formatter service. * @param \Drupal\user\UserStorageInterface $user_storage * The user storage. * @param \Drupal\user\UserDataInterface $user_data * The user data service. */ - public function __construct(DateFormatterInterface $date_formatter, UserStorageInterface $user_storage, UserDataInterface $user_data) { - $this->dateFormatter = $date_formatter; + public function __construct(UserStorageInterface $user_storage, UserDataInterface $user_data) { $this->userStorage = $user_storage; $this->userData = $user_data; } @@ -64,7 +54,6 @@ public function __construct(DateFormatterInterface $date_formatter, UserStorageI */ public static function create(ContainerInterface $container) { return new static( - $container->get('date.formatter'), $container->get('entity.manager')->getStorage('user'), $container->get('user.data') ); @@ -73,69 +62,58 @@ public static function create(ContainerInterface $container) { /** * Returns the user password reset page. * + * In order to never disclose a redirect link via a referrer header this + * controller must always return a redirect response. + * * @param int $uid * UID of user requesting reset. * @param int $timestamp * The current timestamp. * @param string $hash * Login link hash. + * @param \Symfony\Component\HttpFoundation\Request $request + * The request object. * - * @return array|\Symfony\Component\HttpFoundation\RedirectResponse - * The form structure or a redirect response. - * - * @throws \Symfony\Component\HttpKernel\Exception\AccessDeniedHttpException - * If the login link is for a blocked user or invalid user ID. + * @return \Symfony\Component\HttpFoundation\RedirectResponse + * The redirect response. */ - public function resetPass($uid, $timestamp, $hash) { + public function resetPass($uid, $timestamp, $hash, Request $request) { $account = $this->currentUser(); - $config = $this->config('user.settings'); + /* @var \Drupal\user\UserInterface $user */ + $user = $this->userStorage->load($uid); + if (!$user || !$user->isActive()) { + // Blocked or invalid user ID, redirect to front page. + drupal_set_message($this->t('You have tried to use a one-time login link that has either been used or is not valid'), 'error'); + return $this->redirect(''); + } + // When processing the one-time login link, we have to make sure that a user // isn't already logged in. if ($account->isAuthenticated()) { // The current user is already logged in. - if ($account->id() == $uid) { + if ($account->id() == $user->id()) { user_logout(); + // Redirect back to the same page now the user is logged out so that + // using session works. + return $this->redirect( + 'user.reset', + ['uid' => $uid, 'timestamp' => $timestamp, 'hash' => $hash] + ); } // A different user is already logged in on the computer. else { - if ($reset_link_user = $this->userStorage->load($uid)) { - drupal_set_message($this->t('Another user (%other_user) is already logged into the site on this computer, but you tried to use a one-time link for user %resetting_user. Please logout and try using the link again.', - array('%other_user' => $account->getUsername(), '%resetting_user' => $reset_link_user->getUsername(), ':logout' => $this->url('user.logout'))), 'warning'); - } - else { - // Invalid one-time link specifies an unknown user. - drupal_set_message($this->t('The one-time login link you clicked is invalid.'), 'error'); - } + drupal_set_message($this->t('Another user (%other_user) is already logged into the site on this computer, but you tried to use a one-time link for user %resetting_user. Please logout and try using the link again.', + array('%other_user' => $account->getUsername(), '%resetting_user' => $user->getUsername(), ':logout' => $this->url('user.logout'))), 'warning'); return $this->redirect(''); } } - // The current user is not logged in, so check the parameters. - // Time out, in seconds, until login URL expires. - $timeout = $config->get('password_reset_timeout'); - $current = REQUEST_TIME; - - /* @var \Drupal\user\UserInterface $user */ - $user = $this->userStorage->load($uid); - - // Verify that the user exists and is active. - if ($user && $user->isActive()) { - // No time out for first time login. - if ($user->getLastLoginTime() && $current - $timestamp > $timeout) { - drupal_set_message($this->t('You have tried to use a one-time login link that has expired. Please request a new one using the form below.'), 'error'); - return $this->redirect('user.pass'); - } - elseif ($user->isAuthenticated() && ($timestamp >= $user->getLastLoginTime()) && ($timestamp <= $current) && Crypt::hashEquals($hash, user_pass_rehash($user, $timestamp))) { - $expiration_date = $user->getLastLoginTime() ? $this->dateFormatter->format($timestamp + $timeout) : NULL; - return $this->formBuilder()->getForm('Drupal\user\Form\UserPasswordResetForm', $user, $expiration_date, $timestamp, $hash); - } - else { - drupal_set_message($this->t('You have tried to use a one-time login link that has either been used or is no longer valid. Please request a new one using the form below.'), 'error'); - return $this->redirect('user.pass'); - } - } - // Blocked or invalid user ID, so deny access. The parameters will be in the - // watchdog's URL for the administrator to check. - throw new AccessDeniedHttpException(); + $session = $request->getSession(); + $session->set('pass_reset_hash', $hash); + $session->set('pass_reset_timeout', $timestamp); + return $this->redirect( + 'user.reset.form', + ['user' => $user->id()] + ); } /** diff --git a/core/modules/user/src/Form/UserPasswordResetForm.php b/core/modules/user/src/Form/UserPasswordResetForm.php index 58f177f..8a60219 100644 --- a/core/modules/user/src/Form/UserPasswordResetForm.php +++ b/core/modules/user/src/Form/UserPasswordResetForm.php @@ -7,10 +7,11 @@ namespace Drupal\user\Form; +use Drupal\Core\Datetime\DateFormatterInterface; use Drupal\Core\Form\FormStateInterface; -use Drupal\Core\Session\AccountInterface; use Drupal\Component\Utility\Crypt; use Drupal\Core\Form\FormBase; +use Drupal\user\UserInterface; use Psr\Log\LoggerInterface; use Symfony\Component\DependencyInjection\ContainerInterface; @@ -27,13 +28,21 @@ class UserPasswordResetForm extends FormBase { protected $logger; /** + * The date formatter service. + * + * @var \Drupal\Core\Datetime\DateFormatterInterface + */ + protected $dateFormatter; + + /** * Constructs a new UserPasswordResetForm. * * @param \Psr\Log\LoggerInterface $logger * A logger instance. */ - public function __construct(LoggerInterface $logger) { + public function __construct(LoggerInterface $logger, DateFormatterInterface $date_formatter) { $this->logger = $logger; + $this->dateFormatter = $date_formatter; } /** @@ -41,7 +50,8 @@ public function __construct(LoggerInterface $logger) { */ public static function create(ContainerInterface $container) { return new static( - $container->get('logger.factory')->get('user') + $container->get('logger.factory')->get('user'), + $container->get('date.formatter') ); } @@ -59,7 +69,7 @@ public function getFormId() { * An associative array containing the structure of the form. * @param \Drupal\Core\Form\FormStateInterface $form_state * The current state of the form. - * @param \Drupal\Core\Session\AccountInterface $user + * @param \Drupal\user\UserInterface $user * User requesting reset. * @param string $expiration_date * Formatted expiration date for the login link, or NULL if the link does @@ -69,7 +79,28 @@ public function getFormId() { * @param string $hash * Login link hash. */ - public function buildForm(array $form, FormStateInterface $form_state, AccountInterface $user = NULL, $expiration_date = NULL, $timestamp = NULL, $hash = NULL) { + public function buildForm(array $form, FormStateInterface $form_state, UserInterface $user = NULL) { + if (!$user->isActive()) { + drupal_set_message($this->t('You have tried to use a one-time login link that has either been used or is not valid'), 'error'); + return $this->redirect(''); + } + + $session = $this->getRequest()->getSession(); + $timestamp = $session->get('pass_reset_timeout'); + $timeout = $this->config('user.settings')->get('password_reset_timeout'); + // If the user has never logged in before do not check timeout. + if ($user->getLastLoginTime() && REQUEST_TIME - $timestamp > $timeout) { + drupal_set_message($this->t('You have tried to use a one-time login link that has expired. Please request a new one using the form below.'), 'error'); + return $this->redirect('user.pass'); + } + + $hash = $session->get('pass_reset_hash'); + if (!$hash || !$timestamp || !$user->isAuthenticated() || !Crypt::hashEquals($hash, user_pass_rehash($user, $timestamp))) { + drupal_set_message($this->t('You have tried to use a one-time login link that has either been used or is no longer valid. Please request a new one using the form below.'), 'error'); + return $this->redirect('user.pass'); + } + + $expiration_date = $user->getLastLoginTime() ? $this->dateFormatter->format($timestamp + $timeout) : NULL; if ($expiration_date) { $form['message'] = array('#markup' => $this->t('

This is a one-time login for %user_name and will expire on %expiration_date.

Click on this button to log in to the site and change your password.

', array('%user_name' => $user->getUsername(), '%expiration_date' => $expiration_date))); $form['#title'] = $this->t('Reset password'); @@ -108,7 +139,10 @@ public function submitForm(array &$form, FormStateInterface $form_state) { drupal_set_message($this->t('You have just used your one-time login link. It is no longer necessary to use this link to log in. Please change your password.')); // Let the user's password be changed without the current password check. $token = Crypt::randomBytesBase64(55); - $_SESSION['pass_reset_' . $user->id()] = $token; + $session = $this->getRequest()->getSession(); + $session->set('pass_reset_' . $user->id(), $token); + $session->remove('pass_reset_hash'); + $session->remove('pass_reset_timeout'); $form_state->setRedirect( 'entity.user.edit_form', array('user' => $user->id()), diff --git a/core/modules/user/src/Tests/UserPasswordResetTest.php b/core/modules/user/src/Tests/UserPasswordResetTest.php index 420b97a..b767068 100644 --- a/core/modules/user/src/Tests/UserPasswordResetTest.php +++ b/core/modules/user/src/Tests/UserPasswordResetTest.php @@ -7,6 +7,7 @@ namespace Drupal\user\Tests; +use Drupal\Core\Url; use Drupal\system\Tests\Cache\PageCacheTagsTestBase; use Drupal\user\Entity\User; @@ -106,6 +107,7 @@ function testUserPasswordReset() { // Check successful login. $this->drupalPostForm(NULL, NULL, t('Log in')); + $this->assertText('You have just used your one-time login link. It is no longer necessary to use this link to log in. Please change your password.'); $this->assertLink(t('Log out')); $this->assertTitle(t('@name | @site', array('@name' => $this->account->getUsername(), '@site' => $this->config('system.site')->get('name'))), 'Logged in using password reset link.'); @@ -152,7 +154,9 @@ function testUserPasswordReset() { $blocked_account = $this->drupalCreateUser()->block(); $blocked_account->save(); $this->drupalGet("user/reset/" . $blocked_account->id() . "/$timestamp/" . user_pass_rehash($blocked_account, $timestamp)); - $this->assertResponse(403); + $this->assertResponse(200); + $this->assertUrl(''); + $this->assertText('You have tried to use a one-time login link that has either been used or is not valid'); // Verify a blocked user can not request a new password. $this->drupalGet('user/password'); @@ -163,6 +167,13 @@ function testUserPasswordReset() { $this->assertRaw(t('%name is blocked or has not been activated yet.', array('%name' => $blocked_account->getUsername())), 'Notified user blocked accounts can not request a new password'); $this->assertTrue(count($this->drupalGetMails(array('id' => 'user_password_reset'))) === $before, 'No email was sent when requesting password reset for a blocked account'); + // Verify that a user that no longer exists is handled correctly. + $blocked_account->delete(); + $this->drupalGet("user/reset/100000/$timestamp/" . user_pass_rehash($blocked_account, $timestamp)); + $this->assertResponse(200); + $this->assertUrl(''); + $this->assertText('You have tried to use a one-time login link that has either been used or is not valid'); + // Verify a password reset link is invalidated when the user's email address changes. $this->drupalGet('user/password'); $edit = array('name' => $this->account->getUsername()); @@ -172,6 +183,47 @@ function testUserPasswordReset() { $this->account->save(); $this->drupalGet($old_email_reset_link); $this->assertText(t('You have tried to use a one-time login link that has either been used or is no longer valid. Please request a new one using the form below.'), 'One-time link is no longer valid.'); + + // Try to access the user password reset form directly. + $this->drupalGet(Url::fromRoute('user.reset.form', ['user' => $this->account->id()])); + $this->assertText(t('You have tried to use a one-time login link that has either been used or is no longer valid. Please request a new one using the form below.')); + $this->assertUrl(Url::fromRoute('user.pass')); + + // Get a new password reset link to test directly accessing the + // user.reset.form route once the session properties are set. + $edit['name'] = $this->account->getUsername(); + $this->drupalPostForm('user/password', $edit, t('Submit')); + $resetURL = $this->getResetURL(); + // This will set up the session. + $this->drupalGet($resetURL); + // Requesting the form directly will work because the session has the + // correct properties. + $this->drupalGet(Url::fromRoute('user.reset.form', ['user' => $this->account->id()])); + $this->assertText(t('This login can be used only once.'), 'Found warning about one-time login.'); + // Change the email address to invalidate the session properties. + $this->account->setEmail("2" . $this->account->getEmail())->save(); + $this->drupalGet(Url::fromRoute('user.reset.form', ['user' => $this->account->id()])); + $this->assertText(t('You have tried to use a one-time login link that has either been used or is no longer valid. Please request a new one using the form below.')); + $this->assertUrl(Url::fromRoute('user.pass')); + + // Ensure that a user that is blocked after getting a one time link. + $blocked_account = $this->drupalCreateUser(); + $edit['name'] = $blocked_account->getUsername(); + $this->drupalPostForm('user/password', $edit, t('Submit')); + $resetURL = $this->getResetURL(); + // This will set up the session. + $this->drupalGet($resetURL); + $blocked_account->block()->save(); + $this->drupalGet(Url::fromRoute('user.reset.form', ['user' => $blocked_account->id()])); + $this->assertResponse(200); + $this->assertUrl(''); + $this->assertText('You have tried to use a one-time login link that has either been used or is not valid'); + + // Delete the account and ensure that accessing the form directly does not + // work. + $blocked_account->delete(); + $this->drupalGet(Url::fromRoute('user.reset.form', ['user' => $blocked_account->id()])); + $this->assertResponse(404, 'Requesting the password reset form directly for a non-existing user results in a 404.'); } /** diff --git a/core/modules/user/user.routing.yml b/core/modules/user/user.routing.yml index 6eea7ec..ee0337d 100644 --- a/core/modules/user/user.routing.yml +++ b/core/modules/user/user.routing.yml @@ -150,3 +150,14 @@ user.reset: options: _maintenance_access: TRUE no_cache: TRUE + +user.reset.form: + path: '/user/reset/{user}' + defaults: + _form: '\Drupal\user\Form\UserPasswordResetForm' + _title: 'Reset password' + requirements: + _access: 'TRUE' + options: + _maintenance_access: TRUE + no_cache: TRUE