RPC endpoint to reset user password

OK let's get this going.

One thing I a wondering about ... should there be some kind of flood protection on this route as well?

@dawehner I'd say yes. We'll also need it in \Drupal\user\Form\UserPasswordForm.

1. +++ b/core/modules/user/src/Controller/UserAuthenticationController.php
   --- /dev/null
   +++ b/core/modules/user/tests/src/Functional/UserPasswordHttpTest.php
   

   Why should this not be added to UserLoginHttpTest?

2. +++ b/core/modules/user/tests/src/Functional/UserPasswordHttpTest.php
   @@ -0,0 +1,203 @@
   +  protected function getResultValue(ResponseInterface $response, $key, $format) {
   ...
   +  protected function assertHttpResponse(ResponseInterface $response, $expected_code, $expected_body) {
   ...
   +  protected function assertHttpResponseWithMessage(ResponseInterface $response, $expected_code, $expected_message, $format = 'json') {
   

   We're duplicating these helpers from UserLoginHttpTest.

@Wim Leers we could move the tests to UserLoginHttpTest.

I was not sure if password reset would be qualify under user login.

I'll make the change.

Updated patch as per #6.

00:00:26.585 Error response from daemon: Get https://registry-1.docker.io/v2/drupalci/php-5.6-apache/manifests/production: Get https://auth.docker.io/token?scope=repository%3Adrupalci%2Fphp-5.6-apache%3Apull&service=registry.docker.io: net/http: request canceled (Client.Timeout exceeded while awaiting headers)
00:00:26.591 Build step 'Execute shell' marked build as failure
00:00:26.629 [CHECKSTYLE] Skipping publisher since build result is FAILURE
00:00:26.629 Archiving artifacts
00:00:26.632 Checking console output
00:00:26.632 Recording test results
00:00:26.758 ERROR: Step ‘Publish JUnit test result report’ failed: No test report files were found. Configuration error?
00:00:26.769 Finished: FAILURE

Retesting.

@Wim Leers
Do you have an opinion regarding some flooding protection?

I was going to say "do the same as the form", assuming that the form had flood protection. But apparently it does not!

\Drupal\user\Form\UserPasswordForm::buildForm()
nor \Drupal\user\Controller\UserController::getResetPassForm() contain flood protection.

Therefore I don't think we need to do it here either. Nothing is preventing you from mass-triggering the password reset form today. So then there's no need to do so in the RPC endpoint either. If we ever add it, we should add it to both. But that should not block this issue.

Thanks, this is looking great already!

1. +++ b/core/modules/user/src/Controller/UserAuthenticationController.php
   @@ -208,6 +220,64 @@ public function login(Request $request) {
   +      throw new BadRequestHttpException('Missing credentials.name or credential.mail');
   

   s/credential/credentials/

2. +++ b/core/modules/user/src/Controller/UserAuthenticationController.php
   @@ -208,6 +220,64 @@ public function login(Request $request) {
   +    $name_or_mail = isset($credentials['name']) ? $credentials['name'] : $credentials['mail'];
   +
   +    $users = $this->userStorage->loadByProperties(['name' => trim($name_or_mail)]);
   +    if (!count($users) && isset($credentials['mail'])) {
   +      $users = $this->userStorage->loadByProperties(['mail' => trim($name_or_mail)]);
   +    }
   

   This looks strange…

3. +++ b/core/modules/user/src/Controller/UserAuthenticationController.php
   @@ -208,6 +220,64 @@ public function login(Request $request) {
   +      // Check if user is blocked.
   

   Pointless comment.

4. +++ b/core/modules/user/src/Controller/UserAuthenticationController.php
   @@ -208,6 +220,64 @@ public function login(Request $request) {
   +      if ($this->userIsBlocked($account->getAccountName())) {
   +        throw new BadRequestHttpException('The user is blocked or has not been activated yet.');
   

   Let's use exactly the same message as \Drupal\user\Controller\UserAuthenticationController::login().

5. +++ b/core/modules/user/src/Controller/UserAuthenticationController.php
   @@ -208,6 +220,64 @@ public function login(Request $request) {
   +      $mail = _user_mail_notify('password_reset', $account);
   ...
   +      if (empty($mail)) {
   ...
   +      $this->logger->notice('Password reset instructions mailed to %name at %email.', ['%name' => $account->getAccountName(), '%email' => $account->getEmail()]);
   

   This all matches what's in \Drupal\user\Form\UserPasswordForm::submitForm(), great!

6. +++ b/core/modules/user/src/Controller/UserAuthenticationController.php
   @@ -208,6 +220,64 @@ public function login(Request $request) {
   +      // Respond with message.
   +      $message = $this->t('Further instructions have been sent to the provided email address.');
   +      $response_data = [
   +        'message' => $message->render(),
   +      ];
   +      $encoded_response_data = $this->serializer->encode($response_data, $format);
   +      return new Response($encoded_response_data);
   

   The message seems rather pointless. Especially translating it seems pointless. I think a simple 200 response is sufficient.

7. +++ b/core/modules/user/tests/src/Functional/UserLoginHttpTest.php
   @@ -188,6 +188,96 @@ public function testLogin() {
   +        // 400 Bad Request for empty body.
   ...
   +        // 400 Bad Request for unrecognized username.
   ...
   +        // 400 Bad Request for unrecognized email.
   ...
   +        // Block the account.
   ...
   +        // 400 Bad Request for blocked user with username.
   ...
   +        // 400 Bad Request for blocked user with mail.
   ...
   +        // Activate account.
   ...
   +        // 200 for correct username.
   ...
   +        // 200 for correct email.
   

   These are all pointless comments TBH… they don't add any value. The test code is very clear already without them.

8. +++ b/core/modules/user/tests/src/Functional/UserLoginHttpTest.php
   @@ -188,6 +188,96 @@ public function testLogin() {
   +        $response = $this->passwordRequest(['name' => 'dramallama'], $format);
   ...
   +        $response = $this->passwordRequest(['mail' => 'llama@drupal.org'], $format);
   

   :D

Thanks @Wim Leers. I'll make the changes.

Do we need a separate issue to eventually add flood protection to password reset?

Do we need a separate issue to eventually add flood protection to password reset?

Yes, that'd be splendid :)

Updated patch as per #13.

• 13.1: Fixed.
• 13.2: This is mostly from what \Drupal\user\Form\UserPasswordForm::validateForm is doing. I reversed what we had to match it better. i.e use email if provided, fallback to username.
• 13.3: Comment removed.
• 13.4: Message updated.
• 13.5: -
• 13.6: Done. Response with 200.
• 13.7: Comments removed.
• 13.8: :D

Just fixing a few nits. Other than that, I think this is good to go! Thanks for rerolling so quickly!

1. +++ b/core/modules/user/src/Controller/UserAuthenticationController.php
   @@ -208,6 +220,55 @@ public function login(Request $request) {
   +    // Try to load by mail.
   +    $name = isset($credentials['mail']) ? $credentials['mail'] : $credentials['name'];
   +    $users = $this->userStorage->loadByProperties(['mail' => trim($name)]);
   +    if (!count($users) && isset($credentials['name'])) {
   +      // No success, try to load by name.
   +      $users = $this->userStorage->loadByProperties(['name' => trim($name)]);
   +    }
   

   This is an odd construction - I think we should load by mail or by name depending on what is provided. The form only has a single name field but here we have $credentials['name'] and $credentials['mail']. And the name will accept either a name of an email but mail will only accept a mail. And what happens when both are provided? Maybe we should entirely duplicate the form functionality and just call the input name_or_mail - @Wim Leers - maybe you have some thoughts on this?

2. +++ b/core/modules/user/src/Controller/UserAuthenticationController.php
   @@ -208,6 +220,55 @@ public function login(Request $request) {
   +    throw new BadRequestHttpException('Sorry, unrecognized username or email address.');
   

   I don't think we should be saying "Sorry" here - it's kind of like the please thing. I think the message should be We could improve it and say which of the provided way is wrong but that might be unnecessary.

3. The test coverage looks good
4. The followup suggested by #14 has been created #2856929: Add flood protections to password reset

+++ b/core/modules/user/src/Controller/UserAuthenticationController.php
@@ -208,6 +220,55 @@ public function login(Request $request) {
+    // Check if a name or mail is provided.
+    if (!isset($credentials['name']) && !isset($credentials['mail'])) {
+      throw new BadRequestHttpException('Missing credentials.name or credentials.mail');
+    }
+
+    // Try to load by mail.
+    $name = isset($credentials['mail']) ? $credentials['mail'] : $credentials['name'];
+    $users = $this->userStorage->loadByProperties(['mail' => trim($name)]);
+    if (!count($users) && isset($credentials['name'])) {

One thing I am wondering, could these information be sent via query parameters instead? Do they have to be part of the request body? Maybe i'm overthinking things a lot, but for me its a bit like you reset the account of a specific user

Here's an updated patch as per #19 that loads by name or by mail depending on what is provided.

I've also updated the error message.

@alexpott, the patch in #8 had $name_or_mail like \Drupal\user\Form\UserPasswordForm::validateForm.

#19.1: I shared your concerns, but this is literally what the existing code in \Drupal\user\Form\UserPasswordForm::validateForm() was doing, so I let it pass.

#20: I don't see what the advantage of that would be? This must be a POST request anyway. If we the need arises, we could add that capability in the future. For now, I think this makes sense.

Concerns in #19 are addressed.

#20: I don't see what the advantage of that would be? This must be a POST request anyway. If we the need arises, we could add that capability in the future. For now, I think this makes sense.

Well, maybe its more concrete as you would kinda act on the resource with name "X".

Thanks for your work on this @ruloweb. +1

Sigh, still random CI fails.

+++ b/core/modules/user/src/Controller/UserAuthenticationController.php
@@ -208,6 +220,57 @@ public function login(Request $request) {
+      $langcode = $this->languageManager()->getCurrentLanguage()->getId();
+
+      // Send the password reset email.
+      $mail = _user_mail_notify('password_reset', $account, $langcode);
+      if (empty($mail)) {
+        throw new BadRequestHttpException('Unable to send email. Contact the site administrator if the problem persists.');
+      }
+      else {
+        $this->logger->notice('Password reset instructions mailed to %name at %email.', ['%name' => $account->getAccountName(), '%email' => $account->getEmail()]);
+        return new Response();
+      }

Discussing with @alexpott, this same logic is used in UserPasswordForm but there the user initated the request I believe so using the page language is fine, so since the user could interact in that language, they could receive mail in that language. But here, there is no guarantee the endpoint was requested in a language the user can interact in, so the logic should ensure the user preferred language is used.

So you're saying that the REST request should specify the language?

I think pulling the user preferred language from the user profile would be the most solid solution here. Since the REST request may or may not be initiated by the user themselves, I think that would be the only stable way to always send email in a language the user should understand. AccountInterface::getPreferredLangcode() gets you the right langcode to use.

AccountInterface::getPreferredLangcode() gets you the right langcode to use.

Alright, that makes this issue actionable :)

• Ok adding get langcode for email from AccountInterface::getPreferredLangcode() as per #36

  So looking at \Drupal\Core\Session\UserSession::getPreferredLangcode it looks like if there is not a preferred language the langcode will be from
  \Drupal::languageManager()->getDefaultLanguage()->getId()
  where has before we were doing
  $this->languageManager()->getCurrentLanguage()->getId();
  That is probably fine but just want to point that out.

• I also noticed that in UserLoginHttpTest::testPasswordReset we were actually testing that email was sent.

  This is the same logic that is already in UserLoginTest so I made a new trait UserResetEmailTestTrait that both now use.

+++ b/core/modules/user/tests/src/Functional/UserLoginHttpTest.php
@@ -258,11 +261,17 @@ public function testPasswordReset() {
+        $account->
+

What happened here?

Fixing and removing unused use statement.

#40: hah, I wondered the same!

Thanks for bringing this one over the finish line!

Re-roll because #992540: Nothing clears the "5 failed login attempts" security message when a user resets their own password was reverted.

Will comment there about new trait.

Manually compared #40 and #43. The only change is the removal of the hunk to update core/modules/user/src/Tests/UserLoginTest.php, because the revert of #992540: Nothing clears the "5 failed login attempts" security message when a user resets their own password makes that change unnecessary. Other than that, it's identical.

Therefore, back to RTBC.

1. +++ b/core/modules/user/src/Controller/UserAuthenticationController.php
   @@ -208,6 +220,56 @@ public function login(Request $request) {
   +        throw new BadRequestHttpException('The user has not been activated or is blocked.');
   ...
   +    throw new BadRequestHttpException('Unrecognized username or email address.');
   

   Whilst this feels like username/email enumeration - it already exists in the existing form-based approach and is consistent with security team policy - see https://www.drupal.org/node/1004778 Disclosure of usernames and user ids is not considered a weakness

2. +++ b/core/modules/user/tests/src/Functional/UserLoginHttpTest.php
   @@ -188,6 +191,90 @@ public function testLogin() {
   +    foreach ([FALSE, TRUE] as $serialization_enabled_option) {
   ...
   +      foreach ($formats as $format) {
   

   Are we avoiding using a @dataProvider here because of setup cost? Feels like that is what we actually want...going to ask others their thoughts

1. Good :)
2. This is following the example set in \Drupal\Tests\user\Functional\UserLoginHttpTest::testLogin(). Refactoring this seems like an out-of-scope nice-to-have to me.

Refactoring this seems like an out-of-scope nice-to-have to me.

I respectfully disagree, see #1847540: [META] Clean up comment module tests and decouple from node for example of where technical debt in tests is hard to clean up later.

I'll meet you half-way, I'll refactor it here before it goes in (see attached patch) if you open a novice follow-up to refactor \Drupal\Tests\user\Functional\UserLoginHttpTest::testLogin, linking to this comment for an example? Should be plenty of takers for a simple task.

Thanks for that! Done: #2886198: Refactor \UserLoginHttpTest::testLogin() to use a protected method and remove the nested loops.

Committed/pushed to 8.4.x, thanks!

@ruloweb have you progressed with the other endpoint to complete the actual password reset through rest?

I just added #2904451: Allow to change password through RPC endpoints through the reset password workflow to follow up discussion on what was missing here.