Password reset form reveals whether an email or username is in use

Good and important suggestion. :-)

How about if it says "Further instructions have been sent to emailhere@emaildomain.com." in both cases (the email does exist and the email does not exist). If emailhere@emaildomain.com contains a typo, this gives the user the opportunity to realize that they have mis-typed their email address and need to try again. If this was an attempt to obtain membership information, no such information has been exposed.

+1 for this issue

i made a patch for it, containing the following changes:

• Validation if no value is entered in the name field
• Removed validation message if a wrong or none existing name or e-mail adress was entered
• The message form #3 by pdrake, regardless if the name or e-mail adress is known by the system
• A watchdog entry (warning) if the name or e-mail adress is not known in the system

I agree with the privacy issue.
I can imagine a lot of strategies how to handle the password reset in different ways.
The default handling of the core should be very save (like the strategy above) and a more complex or more open strategy can be realized with contrib modules.

fixed the test and fixed the patch

About patch of #7
Test with existing user OK.
Test with not existing gives the following error:

Notice: Undefined index: account in user_pass_submit() (line 102 of core/modules/user/user.pages.inc).

fixed the warning from #9
fixed whitespaces

Great stuff! but i do have 1 question:

Is it an idea to make this a configurable option? E.g. "B.S. mode" enabled? I had the same problem with a module i was working on to resend a user's name via e-email via a similiar way pass reset works. I ended up creating a config option that is enabled by default, so the admin has the choice. (because sometimes you just want to get the real info.)

Can (obviously) also start a new feature request / bug ones this issue is fixed. (I'll add it to my list of wanna-haves anyway, for when i have some time left. Thinking about making this a site wide option, used by multiple functions, registration / contact form can also make use use of it, if only for pranksters / bots.) And added it to the list of related on #1837054: [META] Refactor account workflow (and config)

updating issue summary

minor

This seems like something we could do at any time and not something that would be subject to feature freeze, so moving to a bug. However, since it's simply a security hardening thing, I'm making it a normal bug.

making it configurable could be in the scope of this issue. I dont think we need a new feature request right away. lets see what we can do here.

Updated issue summary to make it easer for new people

For the user story in the summary, a user also could try to register with the same email, and will get a validation error saying:
"The e-mail address bob@doe-family.example is already registered."

If we try to fix this in Core we should also take the registration into account, which is probably more difficult to handle (what should we do instead?).

Updated issue summary - needs updating with issue summary template (how to: http://drupal.org/node/1427826)

Patch of #10 has to be rerolled.

added rerolled patch user-reset-password-privacy-1521996-22.patch
now matching the new code of the user module

fixed a bug in the #22 patch

(novice) manual testing (how to: http://drupal.org/node/1489010)
Tested locally on my machine, tests have been adjusted too, waiting for testbot to run the tests.

(novice) check coding standards and documentation (how to: http://drupal.org/node/1487976)
Tested locally with php code sniffer (phpcs) within PHPStorm with the Drupal settings. Fine, except the following errors:

UserPasswordForm.php

• line 10: use Drupal\Core\Field\Plugin\Field\FieldType\EmailItem; - Alias is never used
• line 16: use Symfony\Component\HttpFoundation\Request; - Alias is never used
• line 117+120: $this->userStorage->loadByProperties() - Method loadByProperties() not found in class
• line 124: form_set_value() function is depcricated

UserPasswordResetTest.php

• line 34: function user_load() is depricated

Writing more tests? Needs a test only patch so the bot can show it fails. (how to: http://drupal.org/node/1468170)
Don't know if we need more tests for this. There is already the UserPasswordResetTest testing a wrong account and a valid account.

Needs an interdiff (how to: http://drupal.org/node/1488712)
The patch has to rerolled because there were a lot of changes in the structure of the user module. Code was sourced out into separate files. Therefore an interdiff is not really possible i think. But it is not much stuff ...

I think it makes a lot of sense to have the text customizeable, so that each site can determine the exact wording.

I also think that we might improve on the suggested wording here.

How about something a bit more clarifying, like:

"Due to privacy concerns, the policy of this web site is not to disclose the existence of registered email addresses. Hence, if you entered a valid email address, a password reset link are now being sent to it.
If the email address you entered does not exist, you will not get a reset link, and will neither get any confirmation or warning about that here. If you get an email, it should generally arrive within 30 minutes or so, sometimes instantly."

Would also be great to have this backported to D7.

good idea. Is there already a settings form, where it should be integrated?
How about the user settings?

And yes, if this is done for D8, i can create a patch for D7

i add the administration setting for the text

I added the setting for this text within the Account settings form - admin/config/people/accounts - AccountSettingsForm.php.
Just reinstall after aplying the patch and it is there. I added the text from Leeteq from #29.
The setting is just below the personalization form.

Maybe somebody has improvements for the texts ...

missed config settings schema for the new value
added it to the new patch

@Carsten Müller: You've been busy today on this issue. Are you sprinting in Amsterdam?

next steps

• somebody should also test the patch
• is the place of the settings within the user settings form admin/config/people/accounts ok?
• text improvement ideas?

Following tests ran on patch #35:
1) Used reset password form with a valid email address already registered
2) Used reset password form with a valid email address not registered
3) Changed the string in the form admin/config/people/accounts

All tests worked as expected (from the users point of view). However it was possible to enter an empty string in 3. It might be a good idea not to allow this.

Regarding the placing of the settings in admin/config/people/accounts I agree that it is a good place.

I would like the texts to be a bit shorter and more to the point. So here are my suggestions taking into consideration that: I agree with the comment #2 that it might be a good idea to add the possibility of printing the entered username/email so that the user can check whether it was entered correctly; and that it might not be necessary for the user to know why the information about previous existing accounts is not printed.

1) Change:

Due to privacy concerns the texts for user password reset or registration should not tell anything about already registered users. So the messages should not contain the username or the email of the registered users.

To:

Due to privacy concerns the texts for user password reset or registration should not contain any information regarding previously registered users.

2) Change:

Due to privacy concerns, the policy of this web site is not to disclose the existence of registered email addresses. Hence, if you entered a valid email address or username, a password reset link is now being sent to it. If the email address you entered does not exist, you will not get a reset link, and will neither get any confirmation or warning about that here. If you get an email, it should generally arrive within 30 minutes or so, sometimes instantly.

To:

If (username/email) is a registered username or email address, you will receive an email within 30 minutes with instructions on how to reset your password.

3) Change:

This is the text displayed when a user uses the password reset form. Due to privacy concerns other users should not be able to check if an account is already registered.

To:

Text displayed after using the password reset form. (Due to privacy concerns it should not include information on whether the account is registered)

Due to the following commit the patch was no longer working, so I updated it
https://github.com/drupal/drupal/commit/66762a5361c1b1f2ab47d462681d804a...

Rerolled

Rerolled.

After a little bit working on it @javiprada1 and me found this minor mistakes in #44:

• +      ->set('password_reset_text', $form_state->getValue('user_password_reset_text'))
  

  is missed in #44

• We have to remove a white line in 154.

We have worked on it at sprintweekend2015

We still need to update the strings suggested in #39

Patch applies.

The patch doesn't apply.

I have rerolled the patch.

I think the privacy concern here is somewhat overstated, given that Drupal does not verify email address. Once I have an account on a Drupal site, I can edit the account and change the email address to president@whitehouse.gov. That doesn't prove that Barack Obama or his staff are members of the site.

With #85494: Use email verification when changing user email addresses it would be more of an issue though.

A fairly rudimentary privacy issue. Any plans on backporting this fix for Drupal 7?

[Double posted]

A fairly rudimentary privacy issue. Any plans on backporting this fix for Drupal 7?

It is tagged "Needs backport to D7" so, yes.

Thanks @cilefen for pointing that out. If you are in the know, any idea of the priority/timeframe?

According to the backport policy this must be fixed in Drupal 8 before making it to Drupal 7. According to Allowed changes during the Drupal 8 release cycle, since this issue makes "string, markup, user interface, or render array changes", it must be postponed to 8.1, which will release in about 6 months.

The best thing anybody can do to move this along right now is to provide a detailed review. Here is a partial review:

1. +++ b/core/modules/user/src/AccountSettingsForm.php
   @@ -186,6 +186,39 @@ public function buildForm(array $form, FormStateInterface $form_state) {
   +      '#title' => $this->t('Privacy settings'),
   +      '#description' => $this->t('Due to privacy concerns the texts for user password reset or
   +      registration should not tell anything about already registered users. So the messages should not contain the
   

   I would not use the word "texts" in a sentence like this. "messages" would be fine. The whole sentence is wordy for me.

2. +++ b/core/modules/user/src/Form/UserPasswordForm.php
   @@ -141,16 +144,22 @@ public function validateForm(array &$form, FormStateInterface $form_state) {
   +    $message = \Drupal::config('user.settings')->get('password_reset_text');
   

   How is this text going to exist without a hook_update_N() setting it?

+++ b/core/modules/user/config/install/user.settings.yml
@@ -14,3 +14,4 @@ cancel_method: user_cancel_block
+password_reset_text: 'Due to privacy concerns, the policy of this web site is not to disclose the existence of registered email addresses. Hence, if you entered a valid email address or username, a password reset link is now being sent to it. If the email address you entered does not exist, you will not get a reset link, and will neither get any confirmation or warning about that here. If you get an email, it should generally arrive within 30 minutes or so, sometimes instantly.'

The default message is unnecessarily long. What about something like "If the email address or username you provided is associated with a site account, you will receive an email with instructions for resetting your password."?

Picking this up, we have a client need.

Patch re-rolled against latest 8.3 dev. Haven't included an interdiff as it's reasonably different but I can provide one if requested.

Some other changes:
* Removed 'include signatures' from the patch
* Changed text values so they are more concise
* Added an update function to set the value of password_reset_text for existing installations
* Removed extra form validation because that field is required
* Set the value in the admin form
Will work on a D7 backport. Gotta install D7 locally on shaky Dublin wifi first :-)

New patch attached. Did I hear that there should be a tests-only patch for testbot? This is the first time I've done core patches with tests so any advice on that would be appreciated.

Changing status,

Patch for Drupal 7.x dev attached. Default variable values!

Nope, I need to change directory first. Reuploading the correct D7 patch. Now to figure out why the D8 patch failed.

I did wonder if there were tests in d7 ... new patches for both d7 and d8. I don't know why the d8 one is failing - I only changed two lines in that file and it's failing on something elsewhere.

7.x patch is ready for review, but I'm really not sure why the 8.x version is failing testing. Any thoughts appreciated.

@Sophie.SK the D8 test failure is because submission of the reset password form would previously have failed with a validation error, and so would re-present the same form.

Now it always appears to succeed with the generic message (and redirects to the login page.)

You'll need to amend testUserPasswordReset() and ensure that the form is loaded again with $this->drupalGet('user/password'); before the second *real* submission.

Small typo in '#description' in #66 . Resubmitting the patch with correction.

An attempt to fix failing tests from #66.

I wasn't sure how to proceed in a situation where a blocked user tries to reset his password ? Should the displayed message be %name is blocked or has not been activated yet. or should it be If an account exists with these credentials, password reset instructions will be sent to the email associated with the account. Any thoughts ?

@aerozeppelin - The point of this issue is to *not* say "%name is blocked or has not been activated yet".

Attaching patch with typo amended for D7, and an updated version of the D8 patch, re-rolled against the latest 8.3.

Tested. #75 patch for D7 works.

Re-queued patches for the failing environments. Not sure why they're failing; they still apply cleanly to latest dev versions of 7 and 8 (with no changes that I can see), and they're failing on tests that haven't been touched in any of this work.

Finally figured out testbot for my D8 patch.

Still have no idea what the D7 testbot is doing! Resubmitting the same patch, passed all tests locally.

Updating issue summary to provide further tasks (for novices).

Although I am for this issue, Disclosure of usernames and user ids is not considered a weakness should be considered

@dpi this issue also extends to email addresses.

Had this on Drupalcamp London website - using https://www.drupal.org/project/username_enumeration_prevention solved it.

@markpavlitski of course, I was only speaking for usernames :)

whoops, not that one

Related issue #2828724: Username enumeration via one time login route

Try that again...

The tests are all green, so looks like it was a testbot issue. Marking for review.

Rerolled the patch against 8.4.x. The same patch applies and the interdiff is empty, so not uploading a new one.

Is there a version of this patch that works for 8.3.0-rc2?

Swilmes, you could try the latest version (which applied against 8.4 dev), or else the one in #80 above (working 8.3 version).

Latest patch doesn´t apply to 8.4.x

Updated #80 patch to apply in 8.4.x

Before applying patch

After applying patch

I just tested this and it works exactly as expected. Marking RTBC

oops

1. Nice to see this being fixed - we shouldn't be disclosing email addresses via the password reset form.

2. +++ b/core/modules/user/config/schema/user.schema.yml
   @@ -50,6 +50,9 @@ user.settings:
   +      type: string
   

   This needs to be

   type: label<code> so it is translatable.
   </li>
   
   <li>
   <code>
   +++ b/core/modules/user/src/Tests/UserPasswordResetTest.php
   @@ -81,10 +81,13 @@ public function testUserPasswordReset() {
   -    $this->assertText(t('@name is not recognized as a username or an email address.', ['@name' => $edit['name']]), 'Validation error message shown when trying to request password for invalid account.');
   -    $this->assertEqual(count($this->drupalGetMails(['id' => 'user_password_reset'])), 0, 'No email was sent when requesting a password for an invalid account.');
   +    $message = $this->config('user.settings')->get('password_reset_text');
   +    $count = count($this->drupalGetMails(['id' => 'user_password_reset']));
   +    $this->assertText($message, 'Validation error message shown when trying to request password for invalid account.');
   +    $this->assertEqual(count($this->drupalGetMails(['id' => 'user_password_reset'])), $count, 'No email was sent when requesting a password for an invalid account.');
   

   I don't get why we're creating a $count variable and changing $this->assertEqual(count($this->drupalGetMails(['id' => 'user_password_reset'])), 0, 'No email was sent when requesting a password for an invalid account.');.

   As far as I can see only the first assertion needs changing.

3. +++ b/core/modules/user/src/Tests/UserPasswordResetTest.php
   @@ -144,7 +147,7 @@ public function testUserPasswordReset() {
   -    $this->assertTrue( count($this->drupalGetMails(['id' => 'user_password_reset'])) === $before + 1, 'Email sent when requesting password reset using email address.');
   +    $this->assertTrue(count($this->drupalGetMails(['id' => 'user_password_reset'])) === $before + 1, 'Email sent when requesting password reset using email address.');
   

   Unrelated change. Out-of-scope.

4. +++ b/core/modules/user/user.install
   @@ -98,3 +98,13 @@ function user_update_8100() {
   +/**
   + * Add password reset notification text.
   + */
   +function user_update_8400() {
   +  $config_factory = \Drupal::configFactory();
   +  $config = $config_factory->getEditable('user.settings');
   +  $default_value = t('If an account exists with these credentials, password reset instructions will be sent to the email associated with the account.');
   +  $config->set('password_reset_text', $default_value)->save(TRUE);
   +}
   

   This needs testing. Shouldn't be using t() - in fact it would probably be best if read the default config file and got the value from there. Less text to maintain. No need to have a $config_factory variable.

5. +++ b/core/modules/user/src/Form/UserPasswordForm.php
   @@ -136,14 +130,22 @@ public function validateForm(array &$form, FormStateInterface $form_state) {
   +    $name = $form_state->getValue('name');
   

   Only need this when logging the unknown or inactive.

6. +++ b/core/modules/user/src/Form/UserPasswordForm.php
   @@ -136,14 +130,22 @@ public function validateForm(array &$form, FormStateInterface $form_state) {
   +    else {
   +      $this->logger('user')->notice('Password reset form was submitted with an unknown or inactive account: %name.', ['%name' => $name]);
   

   I ummed and ahhed about this - given that we log 404s this makes sense. I think we should consider a logging different message for inactive accounts. To make it easy for a site administrator to why they are getting lots of invalid password reset form usage. The log messages need to be tested.

7. +++ b/core/modules/user/src/Tests/UserPasswordResetTest.php
   @@ -174,9 +177,10 @@ public function testUserPasswordReset() {
   +    $message = $this->config('user.settings')->get('password_reset_text');
   

   Only need to get this message once for the entire test.

"If an account exists with these credentials, password reset instructions will be sent to the email associated with the account."

Message is relevant to update for this issue as:

"Email sent when requesting password reset using email address." message can still indicates that account exists on drupal site.

I think the changes listed in #101 are clear enough that this issue still deserves the Novice tag.

This really seems like bad usability to me, and all of it is in service of a level of privacy that I don't think most sites actually want or care about...

Is there a way we can keep the current behavior (helpful error message) in place by default, but make it easily configurable so that sites which do have extra privacy requirements can get rid of it and replace it with generic text?

Also:

+      '#description' => $this->t('The text that appears when a user successfully submits the password reset form. Due to privacy concerns, it should not contain any information about previously registered users.'),

I don't understand this help text. It doesn't say anything about supporting tokens a.k.a. variable substitution (the form doesn't even have that, does it?), so if it's just raw text with no variable substitution I don't see how it would even be possible for the administrator to include user-specific information in it even if they wanted to...

+password_reset_text: 'If an account exists with these credentials, password reset instructions will be sent to the email associated with the account.'

I'm pretty sure it should say "...to the email address associated with the account" instead.

@alexpott I went ahead and did from point 2 to point 7 from comment #101. I also applied the recomendations by @David_Rothstein on #103. Still a few things are missing due to my lack of knowledge on how it is done in the Drupal Core.

Points that still need to be done:

1. From #101 point #1 is a bit unclear, needs a bit of clarification.
2. From #101 point #4 still needs testing, personally I couldn't include the ones I wrote because, after writing the test, I tried to follow Running PHPUnit tests to run them, and I wasn't successful on it. I would recommend updating that instruction page to be more up to date and beginner friendly. It took me a while to get the hang of it, yet I wasn't successful on running the test locally, still trying to make them run. Any help is greatly appreciated it.
3. From #101 point #6 still needs testing. This is due to the reason mentioned above.
4. Personally, I still find that we should be more informative on the description mentioned on #103.

Re-roll Patch for 7.56

Triggering Test bots for the patch #108.

And we're back on this one...!

Patch re-rolled for 8.4 but we'll need a new one for 8.5 no doubt.

Any updates ?

I am working on this as part of SprintWeekend2018.

This hasn't succeeded since #96 which was on 8.4. I am going to reroll just those changes for 8.6 and then try to reapply #101 and #104.

Awesome will add #105 and so what we get.

Here's the patch that incorporates #105. @cjgratacos, I updated the phpUnit document - with a huge help by @phenaproxima- let me know if those work for you.

I'm going to test this so assigning to myself for now.

Thanks for the update. I tested the patch in #120 locally and unfortunately it didn't work for me. Prior to the patch, I saw the Sorry, john.doe@example.com is not recognized as a user name or an e-mail address. message text. After the patch (and running drush updb), I get redirected to the login page and there is no message.

Steps I took:

1. Tested without patch
2. Went to reset password page
3. Filled in a bogus email address
4. Got the old error message as expected
5. Applied the patch
6. Ran: drush updb, drush cim, drush cr
7. Tested again
8. Get redirected to the login page
9. No message shown

I'll test on simplytest.me before switching the status just to be sure.

This did work on simplytest.me so something must be up with my local.

I reviewed the code and noticed a couple nitpicks but otherwise it seems fine.

1. +++ b/core/modules/user/src/Tests/UserPasswordResetTest.php
   @@ -70,7 +70,10 @@ protected function setUp() {
   +    // Arrange constants to be used in the tests.
   

   Nitpick: "Arrange constants" is odd wording.

   Perhaps something like:

   Define constants to be used in the tests.

2. +++ b/core/modules/user/src/Tests/UserPasswordResetTest.php
   @@ -70,7 +70,10 @@ protected function setUp() {
   +    // Verify that accessing the password reset form without having the session.
        // variables set results in an access denied message.
   

   Awkward wording. And, is there supposed to be a period after session? If it's session.variables then it should stay on one line.

Perhaps something like:

Verify access denied message is shown if password reset form is accessed without a session.

Did changes as per #125 1st point , 2nd point still to be done.

+++ b/core/modules/user/user.install
@@ -98,3 +98,12 @@ function user_update_8100() {
+/**
+ * Add password reset notification text.
+ */
+function user_update_8601() {

This can/should be a post update. See core/modules/system/system.post_update.php

Also this won't work. In order to do this you need to read the value from the user module's supplied configuration.

So we need a test too.

A thought from a security tester's view -

https://hackernoon.com/username-or-password-is-incorrect-is-bullshit-899...

In the last paragraph, he's mentioned what would be the best practice according to him.

I was wondering why it didn't work on my local (see #123) but worked on simplytest.me (see #124) so I tried again on my local and I understand what is happening now. Per #128, the update hook doesn't do the trick.

I was able to get it to work on my local only by doing a full installation with the patch in place. It did not work when I had a fresh install and then applied the patch and then ran drush updb / drush cim / drush cr.

An additional way of determining existing accounts is to track the timing of the page response, as there will typically be a difference in the response time (might only be milliseconds, but detectable). A defense against this is to include a random delay function such as

usleep(rand(100, 750)); // add random delay of .1-.75 seconds to response to prevent timing attacks

before returning the page.

This will add some wobble to the response time and prevent identifying existing accounts through this vector.

This will also slow down a brute force iteration of accounts, but at the expense of keeping server resources held a little longer.

The settings can be stored in the yml file so they can be overridden in settings.php if desired.

The same timing should be added in #2346389: Prevent registered email address enumeration via user registration form to prevent that from being iterated.

@alexpott re: #128, can you elaborate more on why the current update won't work and what testing needs to be added? From @kristen_pol testing, I see it works for a full install but doesn't for an already existing site.

In order to do this you need to read the value from the user module's supplied configuration.

+++ b/core/modules/user/user.install
@@ -98,3 +98,12 @@ function user_update_8100() {
+function user_update_8601() {
+  $config = \Drupal::configFactory()->getEditable('user.settings');
+  $default_value = \Drupal::config('user.settings')->get('password_reset_text');
+  $config->set('password_reset_text', $default_value)->save(TRUE);

I read the above code as, get the password_reset_text config from the user module.

I just tested #127 in local here at Drupalcon Nashville. I would call it a partial success. The

Sorry, john.doe@example.com is not recognized as a user name or an e-mail address.

message is now gone, but it no longer says

Further instructions have been sent to your e-mail address.

even when I enter a good address.

Attempting to fix #127 update issue. Hard coding the message into the hook_update_n() function.

Reroll #136 and move update hook to post update.

Fix for failing test.

I tested the patch #139. I got no message after applying the patch. I think #135 still needs to be fixed.

@lomasr did you run the post update hooks? Applying the patch and running the updates works for me.

Functionally this looks good but there's still a couple of items outstanding from #101:

1. +++ b/core/modules/user/src/Form/UserPasswordForm.php
   @@ -136,13 +130,22 @@ public function submitForm(array &$form, FormStateInterface $form_state) {
   +        $this->logger('user')->notice('Password reset instructions mailed to %name at %email.', ['%name' => $account->getUsername(), '%email' => $account->getEmail()]);
   ...
   +      $this->logger('user')->notice('Password reset form was submitted with an unknown or inactive account: %name.', ['%name' => $name]);
   

   Need tests for this.

2. +++ b/core/modules/user/user.post_update.php
   @@ -20,3 +20,12 @@ function user_post_update_enforce_order_of_permissions() {
   +  $config = \Drupal::configFactory()->getEditable('user.settings');
   +  $default_value = 'If an account exists with these credentials, password reset instructions will be sent to the email address associated with the account.';
   +  $config->set('password_reset_text', $default_value)->save(TRUE);
   

   Need tests for this. We could also make it a one-liner.

   @alexpott also suggested reading the text directly from the config install file.

#142

1. Not sure if this is the right approach for testing log messages in a web test, but gave it a go.

+++ b/core/modules/user/src/Tests/UserPasswordResetTest.php
@@ -81,10 +84,12 @@ public function testUserPasswordReset() {
+    $this->assertErrorLogged(t("Password reset form was submitted with an unknown or inactive account: %name.", ['%name' => $this->account->getAccountName()]));

@@ -92,6 +97,10 @@ public function testUserPasswordReset() {
+    $this->assertErrorLogged(t("Password reset instructions mailed to %name at %email.", [

We shouldn't be using t() in tests.

Replaced t function with sprintf as mentioned in #145

How about message like this "If this account is valid, you will receive password reset instructions in your inbox."

@lomasr - a similar message is already a part of the proposal, i.e. "If an account exists with these credentials, password reset instructions will be sent to the email address associated with the account."

@zaporylie - Looks good, I just suggested to have a shorter message with same meaning.

+        $this->messenger()->addStatus($this->t('Further instructions have been sent to your email address.'));

Surely that shouldn't be there?

Hi,
tried to apply #146 but it needs a reroll.
here is the reroll of #146 with no addition nor modification

Hi,

here is a patch and the interdiff adressing #152

Thanks

Hi,
there was a patch change in a core commit yesterday so the patch didn't apply
the only difference between #154 and #156 is changing the path to UserPasswordResetTest.php file
Thanks

I'll try to fix the last test error as part of DrupalEurope sprint.

Patch from #139 no longer working on 8.6 due to a different test location, so rerolled.

I'll take a look and do the manual testing

I've been able to reproduce the issue without the patch, with the Umami and Out-of-the-box drupal installation :

I've applied the patch 1521996-156.patch (the 1521996-159.patch is not working, the path to the test class is not correct, I suppose it's the patch for D8.6).

When I try to retrieve the password with the known e-mail and with the unknown e-mail I get the following results :

The patch seems to work fine but the message is not displayed correctly (translation issue ?). I tried with French and English.

@mangy.fox the patch in #159 does not address #152

@mjpa We just noticed that too! Here's another reroll with it removed.

I don't know what to do with the test error in #156 (on 8.7.x). There are calls to ->assertErrorLogged() but this method is not in the class nor in the parent classes.
I found this method in :
- UncaughtExceptionTest
- Drupal\simpletest\TestBase

But we cannot extend these classes because we already extend another one. Should this method be moved to a trait?
Because of this, I guess this issue is not a novice one anymore?

Here we go...

1. The patch from #164 doesn't apply.
2. It's a bug, so it should be in 8.6.x.
3. Updated the issue summary.
4. Cleaned up the issue tags.
5. Needs usability review.
6. Needs security review.
7. I'm not sure we have to let this message configurable. At least, I would not expose this configuration in UI, making it hard to be changed. Also, I'm not sure the default message is correct, I would keep the actual text: "Further instructions have been sent to your email address." Period.

8. +++ b/core/modules/user/src/Form/UserPasswordForm.php
   index 11d0691..9027162 100644
   --- a/core/modules/user/src/Tests/Functional/UserPasswordResetTest.php
   
   --- a/core/modules/user/src/Tests/Functional/UserPasswordResetTest.php
   +++ b/core/modules/user/src/Tests/Functional/UserPasswordResetTest.php
   

   This test was converted to functional and javascript functional tests.

9. +++ b/core/modules/user/user.post_update.php
   @@ -20,3 +20,12 @@ function user_post_update_enforce_order_of_permissions() {
   +function user_post_update_add_password_reset_text() {
   

   Needs an update path test.

More on this...

1. +++ b/core/modules/user/src/Form/UserPasswordForm.php
   @@ -116,17 +116,11 @@ public function validateForm(array &$form, FormStateInterface $form_state) {
   +      // The account is valid and active.
   +      if ($account->isActive()) {
   

   What is the reason for testing if the account is active?

2. +++ b/core/modules/user/src/Form/UserPasswordForm.php
   @@ -136,13 +130,21 @@ public function submitForm(array &$form, FormStateInterface $form_state) {
   +    if ($account && $account->id()) {
   

   Instead of $account->id(), use !$account->isAnonymous().

   Also, what if the user is blocked? Should we send him also the E-mail?

EDIT: In fact we should move the whole logic in UserPasswordForm::submitForm() method. Now, that we don't show a validation error anymore, we should drop ::validateForm() entirely.

Reworked #164 for 8.6.x, and checked that it also applies to 8.6.3.

I have also made some changes, as suggested by @claudiu.cristea

* Removed the password reset text field from the UI, so now it has to be updated manually in config or overridden
* Simplified the default message
* Moved the logic from validation to submit as there is no validation to fail
* Updated the new functional tests

As far as I am aware, it is the "isActive" check that prevents blocked users from receiving the password reset email.

I'm not really sure what an update path test looks like so that is still to do.

Triggering testbot

Reroll #96.
8.4.x : PHP 7 : MySQL 5.6.35
password-reset-text-1521996-173.patch (8.25 KB)

Re-rolled the previous patch from #168 so it applies to 8.7.x

EDIT: Work in progress. That doesn't apply properly. I'll rework it.

Re-rolled patch from #168 to apply to 8.7.x

Fixed a failing test in #173.

Hi guys.

For now, instead of applying a core patch, I'm trying (unsuccessfully) to override that validateForm function for UserPasswordForm.

  protected function alterRoutes(RouteCollection $collection) {
    // Pass Reset form.
    if ($route = $collection->get('user.pass')) {
      $route->setDefault('_form', '\Drupal\my_module\Form\NewUserPasswordForm');
    }
  }

namespace Drupal\my_module\Form;

use Drupal\user\Form\UserPasswordForm;
use Drupal\Core\Form\FormBase;
use Drupal\Core\Form\FormStateInterface;
use Drupal\Core\Language\LanguageManagerInterface;
use Drupal\Core\Render\Element\Email;
use Drupal\user\UserStorageInterface;
use Symfony\Component\DependencyInjection\ContainerInterface;

/**
 * Provides a user password reset form.
 *
 * @internal
 */
class NewUserPasswordForm extends UserPasswordForm {

    /**
     * {@inheritdoc}
     */
    public function buildForm(array $form, FormStateInterface $form_state) {
      // I'M ABLE TO OVERRIDE THE BUILD FORM FUNCTION
    }

    /**
     * {@inheritdoc}
     */
    public function validateForm(array &$form, FormStateInterface $form_state) {
      // I'M NOT ABLE TO OVERRIDE THE VALIDATE FORM.
      // Always goes to the original validateForm
    }

}

As described above, I'm able to override the class and all public functions other than the validateForm.

Also, if I try to override the validate function in a hook_form_alter, like this:

 $form['#validate'] = ['myValidateFormCallback];

It stills goes to the original validateForm callback.

Can anyone help me with this particular issue? Why am I not able to override the validateForm callback?

Thanks in advance.

UPDATE: Nevermind. I discovered I'm not able to override the validateForm because of a contrib module hardcoding and forcing the callback (user_registrationpassword).
Thanks.

Wrapping $message inside t() function in submit function. It is a change in patch https://www.drupal.org/files/issues/2018-11-14/password-reset-text-15219...

Hi guys,

The following patch does not use the configuration variable to set the message. This approach helps us in the following ways:
1. The message can be made translatable by passing it through t() function. Earlier we may need to pass the $message variable via t() which is not according to Drupal standards.
2. The message still cannot be changed using UI so security is maintained.
3. If anyone wants to change the message, he or she can do it using the translation interface easily. This right generally remains with the administrator, so security is also maintained.

cc @claudiucristea

This patch makes the configured message translatable.

Rerolling the patch for the latest version of core 8.8.0

Removed my last contributed patch as it failed

Bumping this up to a Major, the current functionality allows malicious visitors to find out if people have accounts on the site, if the site is of a sensitive nature it can have serious consequences.

I think this should not be major because an attacker can also find out on the registration form if a certain email address has an account.

So this patch only helps if you have no registration form on your drupal site. Otherwise attackers can just use the registration form instead.

Please also note that other sites such https://github.com/ also have this vulnerability. I can check any email address if it has an account on the registration form.

This also decreases usability for users since they might be resetting their password with the wrong email address and then wonder why they never get an email. Right now you immediately know that you typed in a wrong email address.

So although the decreased usability vs. better privacy is probably a tradeoff we accept I still think this should not be major.

Good points, and as far as I know, https://www.drupal.org/project/username_enumeration_prevention is the thing to use until this lands. On my *ehem* non-Drupal projects we tend not to allow enumeration this way if there is no registration feature.