Make WordPress Core

Opened 11 months ago

Closed 6 months ago

Last modified 6 months ago

#58407 closed defect (bug) (fixed)

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

Reported by: letraceursnork's profile letraceursnork Owned by: hellofromtonya's profile hellofromTonya
Milestone: 6.4 Priority: normal
Severity: trivial Version: 6.2.2
Component: Users Keywords: good-first-bug has-patch has-testing-info has-screenshots commit
Focuses: ui, administration Cc:

Description

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'

Attachments (4)

58407.diff (432 bytes) - added by prashantbhivsane 11 months ago.
58407-with-error-handling.diff (1.4 KB) - added by prashantbhivsane 11 months ago.
58407-with-error-handling-2.diff (1.4 KB) - added by prashantbhivsane 11 months ago.
Disable reset password.png (88.8 KB) - added by Hareesh Pillai 9 months ago.
After disabling password reset, I do not see the fields to reset the password at all

Download all attachments as: .zip

Change History (46)

#1 follow-up: @petitphp
11 months ago

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.

#2 in reply to: ↑ 1 @letraceursnork
11 months ago

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

#3 follow-up: @SergeyBiryukov
11 months ago

  • Keywords needs-patch good-first-bug added
  • Milestone changed from Awaiting Review to 6.3

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 :)

#4 @prashantbhivsane
11 months ago

  • Keywords has-patch added; needs-patch removed

#5 @dilipbheda
11 months ago

  • Keywords needs-refresh added

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

#6 @prashantbhivsane
11 months ago

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

#7 @letraceursnork
11 months ago

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

#8 @petitphp
11 months ago

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).

This ticket was mentioned in PR #4531 on WordPress/wordpress-develop by @letraceursnork.


11 months ago
#9

  • Keywords needs-refresh removed

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:

@petitphp commented on PR #4531:


11 months ago
#11

Change looks good to me 👍.

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

@letraceursnork commented on PR #4531:


11 months ago
#12

@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?

@petitphp commented on PR #4531:


11 months ago
#13

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

#14 @oglekler
10 months ago

  • Keywords changes-requested added

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

This ticket was mentioned in Slack in #core by mukeshpanchal27. View the logs.


9 months ago

#16 follow-up: @oglekler
9 months ago

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

#17 in reply to: ↑ 16 @letraceursnork
9 months ago

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

#18 @oglekler
9 months ago

  • Keywords needs-testing added; changes-requested removed

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

This ticket was mentioned in Slack in #core by chaion07. View the logs.


9 months ago

@costdev commented on PR #4531:


9 months ago
#20

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.

#21 @Hareesh Pillai
9 months ago

I agree with Colin. @costdev

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

@Hareesh Pillai
9 months ago

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

#22 @Hareesh Pillai
9 months ago

  • Keywords needs-testing-info added

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' );

@letraceursnork commented on PR #4531:


9 months ago
#23

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)

@costdev commented on PR #4531:


9 months ago
#24

"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) 🙂

#25 @huzaifaalmesbah
9 months ago

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

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

This ticket was mentioned in Slack in #core by audrasjb. View the logs.


9 months ago

#27 @audrasjb
9 months ago

  • Milestone changed from 6.3 to 6.4

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.

#28 @letraceursnork
9 months ago

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

This ticket was mentioned in Slack in #core by marybaum. View the logs.


7 months ago

#30 follow-up: @marybaum
7 months ago

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.

#31 in reply to: ↑ 30 @letraceursnork
7 months ago

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)

#32 @oglekler
7 months ago

  • Keywords changes-requested added; needs-testing needs-testing-info removed

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.

This ticket was mentioned in Slack in #core by nicolefurlan. View the logs.


6 months ago

#34 @nicolefurlan
6 months ago

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 :)

#35 in reply to: ↑ 3 @hellofromTonya
6 months ago

  • Keywords needs-testing added; changes-requested removed

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.

#36 @ironprogrammer
6 months ago

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

  1. 👀 Click the "Send password reset" link for a user and observe the displayed status message. Click the browser's Back button*.
  2. 👀 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

  1. 🩹 Apply patch.
  2. 👀 Click the "Send password reset" link for a user and observe the displayed status message. Click the browser's Back button*.
  3. 👀 Try the same by checking the boxes for one or more users and using the bulk option "Send password reset" and clicking Apply.
  4. 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.

https://cldup.com/c8Ukys-loI.thumb.png

Figure 2: After patch, issue resolved.

https://cldup.com/OiwSVcLdWf.thumb.jpg

Last edited 6 months ago by ironprogrammer (previous) (diff)

#37 @ironprogrammer
6 months ago

  • Keywords has-testing-info has-screenshots added

#38 @nicolefurlan
6 months ago

  • Keywords commit added

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.

#39 @hellofromTonya
6 months ago

  • Keywords needs-testing removed
  • Owner set to hellofromTonya
  • Status changed from new to reviewing

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

#40 @hellofromTonya
6 months ago

  • Resolution set to fixed
  • Status changed from reviewing to closed

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.

@hellofromTonya commented on PR #4531:


6 months ago
#41

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

#42 @hellofromTonya
6 months ago

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 :)

Note: See TracTickets for help on using tickets.