Wrong message for blocked users who request password reset

I've added some additional code to the user_pass_validate function to account for blocked users, and to display a different error message. The updated function is:

function user_pass_validate($form, &$form_state) {
  $name = trim($form_state['values']['name']);
  
  // Blocked accounts cannot request a new password,
  // check provided username and email against access rules.
  if (drupal_is_denied('user', $name) || drupal_is_denied('mail', $name)) {
    form_set_error('name', t('%name is not allowed to request a new password.', array('%name' => $name)));
  }

  // Try to load by email.
  $account = user_load(array('mail' => $name, 'status' => 1));
  if (!$account) {
    // No success, try to load by name.
    $account = user_load(array('name' => $name, 'status' => 1));
  }

  $name = $form_state['values']['name'];
  // Check if the account name exists, and if the account status is '0'
  if (db_result(db_query("SELECT COUNT(*) FROM {users} WHERE name = '%s';", $name)) && $account->status == 0) {
    form_set_error('name', t('The username %name has not been activated or is blocked.', array('%name' => $name)));
  }
  
  if (isset($account->uid)) {
    form_set_value(array('#parents' => array('account')), $account, $form_state);
  }
    
  else {
    form_set_error('name', t('Sorry, %name is not recognized as a user name or an e-mail address.', array('%name' => $name)));
  }
}

Thank you! I was just wondering how to make a better message for blocked accounts! Perfect.

No problem! :)

Is this something that can be added to D7 (or D8)?

This was fixed in D6. See https://drupal.org/node/332703

And in Drupal 7 the account is loaded with the new function user_load_multiple with the condition status = 1 (not load the user if it's blocked). But with this approach, there is no good way to know the reason of why is not being loaded.

In the patch attached...

I removed the condition 'status' => '1' so we can set a suitable message, checking the status.

Surprisingly, even in the database the status is '1' (blocked), after being loaded with user_load_multiple, it says '0' (non blocked). I will confirm later again and open a issue in that case.

now with correct extension .patch

Attached is a patch that includes my code above. :)

#7: different_messages_for_blocked_users.patch queued for re-testing.

Updated patch.

Hi Opdavies.

I'm a newbie on submitting patches, but if am not wrong, you are trying to submit patches against Drupal 6.

I moved the thread to Drupal-7 version, where is the problem now. Because in Drupal 6 this problem has been *fixed* (I think since 6.16) (see this for more http://drupal.org/node/332703).

So let's try to fix this for Drupal 7. What do you think folks about this:

Sorry, the username %name has not been activated yet or is blocked
Or you prefer to stick with Drupal 6 message
'%name is not allowed to request a new password.'

Here is a correct patch. Forget about what I said about the "status" bug, I mixed the status 0 and 1. 0 is blocked, 1 active :)

Hmmm... Why is Ignored? extension was right..
Trying to rename the patch file name and indicating the path for the file
cvs diff -up modules/user/user.pages.inc > blocked.patch

Well thank you for taking the time to start writing patches! Set status to 'needs review' to summon the test bot :-)

We had an epic patch weeks ago that removed all instances of 'Please' in the interface text. Let's not add to the 'Sorry's' here :) Drupal tone of voice wants to focus on short and simple but not casual or anthropomorphized. Nevertheless, the new message is more concrete and gives more hints to what might be wrong in a particular case. Propose:

%name is blocked or has not been activated yet.

hehehe.. yes I like more your propose, thanks yoroy !

Hi corbacho,

I didn't realise that this issue had been moved into the Drupal 7 queue. I was starting to wonder why my patch kept failing. :)

Ollie.

np

#12 and #13 are identical patches. First failed, the other passed. Did I miss something?

I think it's important to differ the message for non existing and blocked user.

I tested (Drupal 7.34) and reviewed the patch #15 and think its ready to commit. Patch on comment #18 is the same as #15, I just changed the patch name.

Reroll the patch.

Retest the patch.

Thanks weri for bringing back this issue after 4 years.

I don't know if bug still persists in D8.

In that case, first needs to be fixed in Drupal 8.1.x (Drupal 8.0.x translatable strings are already frozen), and maybe then, backported to D7.

That link says strings are unfrozen, not frozen. They are frozen after the first release candidate.

It does seem likely that this exists in Drupal 8 also because there is similar code in UserPasswordForm::validateForm().

I wonder if this behavior is somewhat intentional though? Maybe we don't want to inform the whole world that a particular user has been blocked from the site... On the other hand it's quite possible there are already other ways to find that out.

In the current state it is already possible to see an account has been blocked by the exact same mechanic: the email address is already registered, but unable to request a new password. Might as well update the status message accordingly then :)

The issue still occurs in 8.0.x. I ported the patch and added a test. Strangely enough, there was no test yet to check if a blocked user can request a password.

I'm at DrupalCon LA sprints and we are working on this issue.

We've just successfully applied the last patch on latest 8.0.x-dev code, tested and it worked for us.
Moving it to RTBC.

Updated patch to meet the new changes to 8.0.x-dev.

The patch seems to apply locally. Re-queuing the test.

Tests are failing at Drupal\user\Tests\UserPasswordResetTest->testUserPasswordReset() are failing, on lines 161 and 162 in UserPasswordResetTest.php.

Updated the test so it post the form accordingly.

+++ b/core/modules/user/src/Form/UserPasswordForm.php
@@ -116,14 +116,20 @@ public function buildForm(array $form, FormStateInterface $form_state) {
+      if ($account->isBlocked()) {

I think this should be !$account->isActive() that way we're maintaining the same logic as previously which would have ignored users where status equalled anything other than 1.

as for #45

Verified! seems fine to me.
When a user is blocked then message "The username manauwar has not been activated or is blocked." will appear.
I have checked for different cases.
a. When blocked user is trying to login.
b. When blocked user enter wrong inputs.
c. When blocked user is trying to "request password/reset password".
Screenshot attached.

I think this makes sense. Thanks all for the work on this issue!

I think this should be !$account->isActive() that way we're maintaining the same logic as previously which would have ignored users where status equalled anything other than 1.

Nice catch!

As a usability improvement, this issue is a prioritized change as per https://www.drupal.org/core/beta-changes and its benefits outweigh any disruption. Committed and pushed to 8.0.x. Thanks!

Moving to 7.x for backport.

Patch #22 re-rolled. Reused the status message used for login for validation. Also added a test case but it is failing in my local.

Tested the above patch patches/753898-54.patch. Its working fine. Manually verified if the user is blocked & try to reset the password the error message appears as "Sorry, %name is not recognized as a username or an email address".
- If user try to create a new account with same username or email-id error message appears as "The username %name is already taken.
The email address %email is already taken".

After applying the patch:
- If user try to reset the password for blocked account:
""The username %email has not been activated or is blocked"

Its working as expected. Screenshots attached.

1. +  if ($account){
   +    // Blocked accounts cannot request a new password,
   +    // check provided username and email against access rules.
   +    if ($account->status == 0) {
   +      form_set_error('name', t('The username %name has not been activated or is blocked.', array('%name' => $name)));
   +    }
   +  }
      if (isset($account->uid)) {
        form_set_value(array('#parents' => array('account')), $account, $form_state);
      }
   

   I think we need to be a little more careful here. Especially since this is a form that is frequently altered, we should preserve the behavior where form_set_value() only gets called if the account is valid and the password reset email is actually going to be sent. Otherwise $form_state['values']['account'] could get set when it's not valid, and that could mess with other validation handlers.

   So basically the new code should not be its own check that runs in addition to the form_set_value(), but rather inside the isset($account->uid) so it runs as an alternative to it.

2. +  if ($account){
   

   (minor) There should be a space after the closing parenthesis.

3. +    // Blocked accounts cannot request a new password,
   +    // check provided username and email against access rules.
   

   I'm not sure what the second sentence means here - should it just be removed?

4. +      form_set_error('name', t('The username %name has not been activated or is blocked.', array('%name' => $name)));
   

   I like that this is reusing an existing translatable string (so it doesn't break translations) but unfortunately "username" isn't really correct here since it can also be an email address.

   Maybe if we go ahead with this issue, we'll just have to live with that, though.

5. +    // Confirm the password reset.
   +    $this->assertNoText(t('Further instructions have been sent to your e-mail address.'));
   

   Shouldn't we also be asserting that the expected message is shown? Perhaps we can borrow more from the committed Drupal 8 patch here?

Backported to D7. Applied all changes suggested at #58 to the patch at #54

Forgot adding space after a parenthesis in the previous patch

Verified and tested on the latest 8.4.x-dev version. Its working fine as per the description added on top.
Here are the test observations:
Without applying any patch on 8.4.x-dev :
1. If user try to reset the password for blocked account the error message appears as:
""The username %email has not been activated or is blocked"
2. If user try to create a new account with same username or email-id error message appears as
"The username %name is already taken.
The email address %email is already taken".
Screenshots attached

- Moving it to RTBC as the issue is already fixed on latest version 8.4.x-dev.

Hey NikitaJain,

Thanks for your work on this!

I'm moving this back to Needs Review because this issue was already fixed on 8.0.x and it becomes part to the new releases/branches automatically.

What we need now is to get the D7 backport reviewed and tested so it can get pushed to the latest 7.x.

Cheers!

Patch in #61 applies cleanly to the latest 7.x branch.

Going to do some manual testing and check the testbot failure.

+      form_set_error('name', t('The account %name has not been activated or is blocked.', array('%name' => $name)));

Like I said in point 4 of #58 it is probably better to use the text that the previous patch had, even though it's slightly inaccurate (since it's text that is already translated elsewhere in Drupal). This is an important error message and it would be bad to display it in English to users who don't speak that language.

+    //Blocked accounts cannot request a new password.
+    if($account->status == 0) {
+      form_set_error('name', t('The account %name has not been activated or is blocked.', array('%name' => $name)));
+    }
+    else {
+    form_set_value(array('#parents' => array('account')), $account,
+      $form_state);
+    }

There are still minor coding standards issues here - need a space after the //, a space after the if, the form_set_value line should be indented by two spaces (since it's inside the else {}), and I don't think there's any reason to split the form_set_value onto multiple lines.

Reuploading patch from #61 with code issues fixed, form_set_error message changed and test moved to better location. Hopefully all points of @David_Rothstein should be now addressed.

Applied #72 patch successfully in D7

This would need a change record for D7.

I think we probably want to change the message text based on #3200198: [D7] password reset form prevent revealing email or username in use.

Ah, I see that, thanks! It seems like that not all changes were backported from the original D9 issue (#1521996) in the issue you are referencing.

In the D9 issue this was removed too (so that no specific message is output in this case):

-      // Blocked accounts cannot request a new password.
-      if (!$account->isActive()) {
-        $form_state->setErrorByName('name', $this->t('%name is blocked or has not been activated yet.', ['%name' => $name]));
-      }

But it was not backported, because D7 is already loading only active accounts. See:

  // Try to load by email.
  $users = user_load_multiple(array(), array('mail' => $name, 'status' => '1'));
  $account = reset($users);
  if (!$account) {
    // No success, try to load by name.
    $users = user_load_multiple(array(), array('name' => $name, 'status' => '1'));
    $account = reset($users);
  }
  if (isset($account->uid)) {
  ...
  }

Looking at that, this seems that we do not need to do any changes in D7 anymore, because the process is now working correctly (it was fixed by #3200198: [D7] password reset form prevent revealing email or username in use).

1 - If you request a password for non-existing account, you will get "If %identifier is a valid account, an email will be sent with instructions to reset your password."

2 - If you request a password for a blocked account, you will get the same message, because no account will be loaded either.

If I have not missed anything, this could be closed as Fixed in D8 and thats all.

Yeah that makes sense - I agree we can close this for D7 as the correct non-specific message is displayed.

Thank you everyone for all the work that went into it.

As this was a D8/D7 mixed issue and it was committed to D8, it is probably better to close this as Fixed (for D8), so that people get credits for that D8 patch finally.