Opened 12 days ago
Last modified 5 days ago
#58407 new defect (bug)
resetpassword action on users.php (users list page) handles retrieve_password() return incorrectly
Reported by: |
|
Owned by: | |
---|---|---|---|
Milestone: | 6.3 | Priority: | normal |
Severity: | trivial | Version: | 6.2.2 |
Component: | Users | Keywords: | good-first-bug has-patch |
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:
- Forbid to reset users passwords via
allow_password_reset
hook (for example, by hooking__return_false
to it) - Tried to reset it as an admin on '/uesers.php' page
- 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 (3)
Change History (16)
#2
in reply to:
↑ 1
@
12 days 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 totrue
.
Yeah, pretty much that, with one small addition: it also should handle the WP_Error somehow if one appears
#3
@
12 days 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 :)
#5
@
11 days 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
@
11 days 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
@
11 days ago
How about that behavior:
- Count
reset_count
ANDreset_count_success
params both - If
reset_count_success === 0
- show some message like 'Could not send any passwords because of error' with red-colored border (.error class) - 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) - 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
@
11 days 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.
5 days 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:
5 days ago
#11
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
@letraceursnork commented on PR #4531:
5 days 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:
5 days ago
#13
@LeTraceurSnork you can do that in the current PR, no need for a new PR or trac ticket 🙂
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 totrue
.