resetpassword action on users.php (users list page) handles retrieve_password() return incorrectly

[letraceursnork]

I've noticed there is 3 usages of retrieve_password() function across the core - it's in ajax-actions.php, wp-login.php and users.php. The last one handles its return incorrectly: if (retrieve_password($user->user_login)) despite the fact that function returns true|WP_Error, and if the answer is WP_Error - if condition still works, while semantically it should not (and two other usages implements that kind of logic - there are additional checks via is_wp_error())

What did I do to produce the problem:

1. Forbid to reset users passwords via allow_password_reset hook (for example, by hooking __return_false to it)
2. Tried to reset it as an admin on '/uesers.php' page
3. Got a success message 'Password reset link sent.'

Step three is the problem - message should be smth like 'Password reset is not allowed for this user'

[petitphp]

Hello @letraceursnork and welcome back to trac.

If I understand correctly, you are talking about this condition in ​wp-admin/users.php when bulk resetting users passwords.

The condition should be changed to check if the return of retrieve_password is equal to true.

[letraceursnork]

Replying to petitphp:

Hello @letraceursnork and welcome back to trac.

If I understand correctly, you are talking about this condition in ​wp-admin/users.php when bulk resetting users passwords.

The condition should be changed to check if the return of retrieve_password is equal to true.

Yeah, pretty much that, with one small addition: it also should handle the WP_Error somehow if one appears

[SergeyBiryukov]

Hi there, welcome back to WordPress Trac! Thanks for the ticket.

Good catch, we should update that condition to either explicitly check for true:

if ( true === retrieve_password( $user->user_login ) ) {
	++$reset_count;
}

or check for is_wp_error():

if ( ! is_wp_error( retrieve_password( $user->user_login ) ) ) {
	++$reset_count;
}

We use the former in wp-admin/includes/ajax-actions.php​ and the latter in wp-login.php​. I don't have a strong preference at the moment, but it would be great to have some consistency with these checks :)

[dilipbheda]

Hey @prashantbhivsane

I believe it should be good if we display an error notice instead of a success notice.

Need the below changes in the wp-admin/users.php file.

case 'resetpassword':
	$reset_count = isset( $_GET['reset_count'] ) ? (int) $_GET['reset_count'] : 0;
	$notice_type = 'updated';
	if ( 1 === $reset_count ) {
		$message = __( 'Password reset link sent.' );
	} else {
		/* translators: %s: Number of users. */
		$message     = _n( 'Password reset links sent to %s user.', 'Password reset links sent to %s users.', $reset_count );
                $notice_type = 'error';
	}
	$messages[] = '<div id="message" class="notice is-dismissible ' . esc_attr( $notice_type ) . '"><p>' . sprintf( $message, number_format_i18n( $reset_count ) ) . '</p></div>';
	break;

Before: ​https://tinyurl.com/2fjpsasl
After apply changes: ​https://tinyurl.com/2ps5c4y7

CC @SergeyBiryukov

Thanks

[prashantbhivsane]

I think that the error should only be displayed when $reset_count < 1 (or 0), according to this, It will be displayed as an error if the $reset_count is anything other than 1 ( both >1 and <1 ), need to handle it for both.

Before ​https://tinyurl.com/2q2k49f6

After ​https://tinyurl.com/2jq89pyq

For error case ​https://tinyurl.com/2jo7hqlx

have added the updated patch

[letraceursnork]

How about that behavior:

1. Count reset_count AND reset_count_success params both
2. If reset_count_success === 0 - show some message like 'Could not send any passwords because of error' with red-colored border (.error class)
3. If reset_count_success !==0 && reset_count_success < reset_count - show smth like 'Passwords sent to %s users, hasn't sent to %s users because of error` with yellow-colored border (.notice-warning class)
4. If reset_count_success === reset_count && reset_count > 0 - show usual message 'Passwords sent to %s users` with green-colored border, just like now

[petitphp]

I believe it should be good if we display an error notice instead of a success notice.

This ticket was about fixing a condition check. I agree the notice could be rework to be more precise about what really happened but I think this should be handle in a follow-up ticket and not in this one.

The original patch from @prashantbhivsane in comment 4 looks good to me. As Sergey mentioned it would be good to update all the conditions to use the same pattern (either true === or is_wp_error).

Handling the retrieve_password() in wo-admin/users.php answer correctly (as potential WP_Error) to prevent incorrect Password reset link sent for users that are not allowed to reset their passwords

Trac ticket:

[letraceursnork]

Opened a PR (​https://github.com/WordPress/wordpress-develop/pull/4531)

Change looks good to me 👍.

As proposed in the ticket, we could also update the other place where this check is done :

• in wp-admin/includes/ajax-actions.php : change to use a check with is_wp_error

@petitphp do you think I should do that in this PR or another PR related to same trac-ticket or should I open the whole new trac-ticket and open a PR for that one?

@LeTraceurSnork you can do that in the current PR, no need for a new PR or trac ticket 🙂