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 🙂

[oglekler]

@letraceursnork you possibly missed the previous answer, please follow up with the PR update. Thank you 🙏

[oglekler]

This ticket was discussed during bug scrub.

This ticket is marked as good-first-bug and patch is quite simple, but change in another place was requested and this still needs testing. It was decided to leave it not in the current milestone a little bit longer.

If you are working on an updated patch, please keep in mind that RC1 is in 7 days and after it, only vital changes/regression fixes will go into the trunk.

Props @mukesh27

[letraceursnork]

Replying to oglekler:

This ticket was discussed during bug scrub.

This ticket is marked as good-first-bug and patch is quite simple, but change in another place was requested and this still needs testing. It was decided to leave it not in the current milestone a little bit longer.

If you are working on an updated patch, please keep in mind that RC1 is in 7 days and after it, only vital changes/regression fixes will go into the trunk.

Props @mukesh27

Done

[oglekler]

This is simple enough patch, and it still lands into 6.3, but it needs testing.

I think true === is likely the safer option. With the PR's current logic, if retrieve_password() ever returned a non-WP_Error, the email would be sent.

While retrieve_password() is documented for true|WP_Error, bugs are bugs, so true === would at least flag that there's an issue since the email wouldn't be sent.

[Hareesh Pillai]

I agree with Colin. @costdev

Patch 58407.diff seems like the best approach for reasons shared above.

[Hareesh Pillai]

After disabling password reset, I do not see the fields to reset the password at all

[Hareesh Pillai]

May be I'm testing it wrong, but after disabling password reset, I do not see the fields to reset the password at all.

Refer the attached image above.

How I disabled reset password?
By adding the following code to functions.php

function disable_password_reset() {
    return false;
}
add_filter( 'allow_password_reset', 'disable_password_reset' );

I think true === is likely the safer option. With the PR's current logic, if retrieve_password() ever returned a non-true, non-WP_Error, the email would be sent.

While retrieve_password() is documented for true|WP_Error, bugs are bugs, so true === would at least flag that there's an issue since the email wouldn't be sent.

Have to disagree with you. Like my teamlead said once - "What, are we don't trust ourselves now?". We should trust our own code and if it says WP_Error or true - we considering not-WP_Error response as "true or identical". Ofc, if we want to extend function return to, say, WP_Error|bool (which is, btw, kinda weird - why is answers is so inconsistent?) - then we should also change its usages and return handlers - in the same PR (which is, in my opinion, should ot be this one, since that was not the original issue)

"What, are we don't trust ourselves now?". We should trust our own code and if it says WP_Error or true - we considering not-WP_Error response as "true or identical"

This isn't so much about trusting our code as it is about being specific.

We want to check whether we can send a successful response:

• if success then send a successful response is specific.
• if not a specific indicator of failure then send a successful response is inverted logic that opens it up to other types of failure being missed.

If a successful response was ever sent when it shouldn't have been, we don't need to check the value of $results with true === $results. We know it's true. With ! is_wp_error( $results ), we now need to investigate what's being returned. If anything other than WP_Error sends a success, we may not know there's a bug causing a non-true|WP_Error value to be returned for quite some time. Changing this to true === means _as soon as_ there's a regression, something has behaved differently.

On the other topic of trusting our own code, your team lead and I would disagree on this. A discussion about those differing philosophies doesn't really belong on this PR, but if you want to discuss it further with me, feel free to DM me on Slack (same username) 🙂

[huzaifaalmesbah]

I agree with @costdev. true === better option.

I'm testing it. This is satisfactory to me.

[audrasjb]

As per today's bug scrub and since 6.3 RC2 is planned tomorrow, let's move it to 6.4.
Feel free to move it back to 6.3 if you feel it can be committed in the next few hours.

[letraceursnork]

IDK whether I'm on time or not, but check the PR, it's consistent now, though I changed condition on ajax-actions a bit

[marybaum]

Bonjour @letraceursnork! Per the PR, it would be good if you could separate out the testing fails and address them on separate tickets. Then, if the tests are still passing on the main issue in users.php, I could see referring it to commit.

[letraceursnork]

Replying to marybaum:

Bonjour @letraceursnork! Per the PR, it would be good if you could separate out the testing fails and address them on separate tickets. Then, if the tests are still passing on the main issue in users.php, I could see referring it to commit.

I've fixed code-style, but I'm afraid I'm not qualified enough (or interested in this PR so hard) to fix Performance and E2E pipelines (especially given that my code doesn't breaks the functionality they cover)

[oglekler]

In the retrieve_password() function there is a shortcut that is returning true well before wp_email(), so when this function is returning true, it isn't a fact that email has been sent.

In the current patch the part in the ajax-actions.php from my point of view has no sense, because retrieve_password() is only returns true or an error object and an error cannot be a success.

[nicolefurlan]

This ticket was discussed in today's bug scrub.

@hellofromTonya will comment on the ticket and PR, but as a summary, we agreed with @costdev that ​the patch/PR has too much in it, and that the small change aligns with @SergeyBiryukov's suggestion, and by itself, it is ready for commit.

Props to @hellofromTonya :)

[hellofromTonya]

Consistency - which to use?

Example from wp-admin/includes/ajax-actions.php​:

if ( true === retrieve_password( $user->user_login ) ) {
	wp_send_json_success(
} else {
	wp_send_json_error(
}

The true comparison check is used for the success branch, with the else handling the error. That order makes for what it's doing: if success, else error.

Example from wp-login.php​:

$errors = retrieve_password();

if ( ! is_wp_error( $errors ) ) {
	// redirect and exit;
}
// else process the errors.

In this example, the code is designed to process the errors, i.e. $errors is expected to be an instance of WP_Error. But if not an error, then it redirects and exits.

Yes, both of these examples are inconsistent in how they check for the return value from retrieve_password(), but both are appropriate for how the code is designed. I think inconsistency here is by design and okay.

Which should this new bugfix?

Only 2 values will be returned: true or an instance of WP_Error.

true does not necessarily mean the email was sent, as it can be short-circuited by hooking into one or these filters 'send_retrieve_password_email' or 'retrieve_password_message' as @oglekler notes in comment:32. Looking at how it's documented in the @return annotation, returning true means "finished":

* @return true|WP_Error True when finished, WP_Error object on error.

where "finished" may or may not actually mean an email was sent. Huh, interesting. I suspect that's by-design. But if no and error or different value should be returned when short-circuiting, that would be out-of-scope for fixing this ticket. It is an interesting question though that would take a contextual dive back in time to review the changesets and tickets.

Okay, so true means it "finished".

For this bugfix, I think true is the information needed to determine whether or not to increment the reset count:

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

If an instance of WP_Error is returned, then the count is not incremented. If later retrieve_password() is changed to where true no longer includes short-circuit, this bugfix change should still be valid.

58407.diff​ is then ready for final testing and commit consideration.

[ironprogrammer]

Thank you, everyone, for helping move this ticket forward 🙌🏻

Test Report

Patch tested (the initial approach discussed during the scrub in comment:34 and comment:35): https://core.trac.wordpress.org/attachment/ticket/58407/58407.diff 👍🏻

Steps to Reproduce and Test Patch

1. Prepare a password reset disablement plugin by creating a PHP file in the /wp-content/mu-plugins/ directory with these contents:

   <?php
   // This filter is intentionally commented out until testing is required.
   //add_filter( 'allow_password_reset', '__return_false' );
   

2. Navigate to Users > All Users. If only one user exists (your login), create another user to use for testing.
3. For users listed (other than yourself), note that "Send password reset" should be an option. This should also be available in the "Bulk options" dropdown above the list.
4. In the plugin file above, remove the comment from the add_filter line to enable the filter, and save the file.

REPRODUCE

5. 👀 Click the "Send password reset" link for a user and observe the displayed status message. Click the browser's Back button*.
6. 👀 Try the same by checking the boxes for one or more users and using the bulk option "Send password reset" and clicking Apply.

TEST PATCH

7. 🩹 Apply patch.
8. 👀 Click the "Send password reset" link for a user and observe the displayed status message. Click the browser's Back button*.
9. 👀 Try the same by checking the boxes for one or more users and using the bulk option "Send password reset" and clicking Apply.
10. Remove PHP file from Step 1 when testing is complete.

*If during testing the links to "Send password reset" disappear and your browser's Back button doesn't restore them, then re-comment out the add_filter line in the plugin, save it, and refresh the page. Then continue from Step 4.

Environment

• Hardware: MacBook Pro Apple M1 Pro
• OS: macOS 13.6
• Browser: Safari 16.6
• Server: nginx/1.25.2
• PHP: 8.2.11
• WordPress: 6.4-beta4-56923-src
• Theme: twentytwentythree v1.2
• Active Plugins:
  • test-trac-58407 (the test mu-plugin noted in Step 1)

Actual Results

• ✅ Issue reproduced: with password resets disabled, the status message displayed is "Password reset link sent." (Figure 1).
• ✅ Issue resolved: after patch, with password resets disabled, the status message is "Password reset links sent to 0 users." (Figure 2).

Supplemental Artifacts

Figure 1: Issue reproduced.

Figure 2: After patch, issue resolved.

[nicolefurlan]

Thank you @ironprogrammer! :)

With that passing test report, I will add commit. Of course, if we disagree, please don't hesitate to remove it and we can discuss this ticket more.

[hellofromTonya]

Thanks @ironprogrammer for thoroughly testing 58407.diff​. Yes, @nicolefurlan it is ready for commit. Prepping a commit now.

[hellofromTonya]

In 56937:

Users: Show "Password reset link sent" message only when finished.

When an admin sends a password reset link to a user (from the Users UI screen), the "finished" UI message:

Password reset link sent.

is displayed on "finished" and no longer displayed on error.

What is the change?

Previously, the conditional (for incrementing the reset counter) checked for a truthy return from retrieve_password(). But retrieve_password() always returns a truthy state: true (meaning "finished") or an instance of WP_Error. Thus, checking for a truthy meant the "finished" message was sent on both "finished" and error/failed.

This fix checks for retrieve_password() returning true to indicate "finished".

What is displayed on error?

If retrieve_password() returns an instance of WP_Error, the following UI message is shown:

Password reset links sent to 0 users.

The UI messages were not modified by this changeset.

Follow-up to [50129].

Props letraceursnork, prashantbhivsane, audrasjb, costdev, dilipbheda, hareesh-pillai, hellofromTonya, ironprogrammer, huzaifaalmesbah, marybaum, nicolefurlan, oglekler, petitphp, SergeyBiryukov.
Fixes #58407.

The true === retrieve_password() approach to fix the reported bug was committed https://core.trac.wordpress.org/changeset/56937.

[hellofromTonya]

Thank you everyone for your contributions :)

The changeset committed fixes the reported issue of not displaying "Password reset link sent." when an error happens. As 6.4 is in the beta cycle and nearly at RC1, it's too late for enhancements. Thus, only the bugfix for the reported issue was committed.

But this ticket contains discussions and code changes for other enhancements such as:

• Modifying the UI messages.
• Adding error messages.
• Making the all instances consistent.
• etc.

If there's interest in these, please open a new enhancement ticket. Thank you :)