Make WordPress Core

Opened 3 years ago

Last modified 3 years ago

#54481 new defect (bug)

User list doesn't honor allow_password_reset filter

Reported by: desmith's profile desmith Owned by:
Milestone: Awaiting Review Priority: normal
Severity: minor Version: 5.8.2
Component: Users Keywords:
Focuses: administration Cc:

Description

In WP 5.7 or so, the user list (Users - All Users in the admin section) began to display a "Send password reset" link for users (other than yourself). On many of my sites, I have a filter like this:

add_filter('allow_password_reset', '__return_false');

(I use an outside authentication system, so the WP-local passwords aren't used for anything, and most users can't get to WordPress' own login screen anyway.)

I expected the above filter also to suppress the links in the user list, but this appears not to be the case.

The patch for this is simple enough (a few lines of new code around line 279 in wp-admin/includes/class-wp-users-list-table.php), but before I write and test it, is this the best approach to fix this bug? i.e. is there anyone expecting that the allow_password_reset filter only applies to users doing self-service password resets, as opposed to admins sending resets on behalf of other users? If that's the case, this probably should be a separate filter or define of some sort.

Attachments (1)

54481-v1.patch.txt (3.3 KB) - added by desmith 3 years ago.
First draft version of the discussed patch, creating is_password_reset_allowed() and adding a few calls to same

Download all attachments as: .zip

Change History (6)

#1 @SergeyBiryukov
3 years ago

Hi there, welcome back to WordPress Trac!

Indeed, I think it makes for these links to be hidden if the allow_password_reset filter returns false.

Just noting that there are several instances of these links:

  • Send password reset in the bulk actions dropdown.
  • Send password reset in action links.
  • Send Reset Link on Edit User screen.

To make the check more reusable, perhaps this code from get_password_reset_key() should be separated into its own function, e.g. is_password_reset_allowed():

$allow = true;
if ( is_multisite() && is_user_spammy( $user ) ) {
	$allow = false;
}

/**
 * Filters whether to allow a password to be reset.
 *
 * @since 2.7.0
 *
 * @param bool $allow   Whether to allow the password to be reset. Default true.
 * @param int  $user_id The ID of the user attempting to reset a password.
 */
$allow = apply_filters( 'allow_password_reset', $allow, $user->ID );

#2 follow-up: @desmith
3 years ago

As a newbie question, what is the correct way to handle the bulk dropdown? (Assuming allow_password_reset is a per-user flag, which is implied by the current filter, if it's in the bulk dropdown it would succeed for some users but fail for others.)

Since allow_password_reset can take a user ID, does that imply that there are cases (now or in the future) where a given user may be able to reset some other users' passwords, but not those of other users?

#3 @desmith
3 years ago

As another newbie question, where would a hypothetical is_password_reset_allowed() function go? My first guess would be wp-includes/user.php, probably near the existing get_password_reset_key(), but I've been burned before by not understanding which of the many many WP files are which.

#4 in reply to: ↑ 2 @SergeyBiryukov
3 years ago

Replying to desmith:

As a newbie question, what is the correct way to handle the bulk dropdown? (Assuming allow_password_reset is a per-user flag, which is implied by the current filter, if it's in the bulk dropdown it would succeed for some users but fail for others.)

Ah, good point. We might want to leave the bulk actions dropdown as is then.

Since allow_password_reset can take a user ID, does that imply that there are cases (now or in the future) where a given user may be able to reset some other users' passwords, but not those of other users?

Yes, I think that would be currently possible by returning true or false depending on the current user and the passed user ID value.

As another newbie question, where would a hypothetical is_password_reset_allowed() function go? My first guess would be wp-includes/user.php, probably near the existing get_password_reset_key(), but I've been burned before by not understanding which of the many many WP files are which.

My suggestion would be directly above get_password_reset_key().

@desmith
3 years ago

First draft version of the discussed patch, creating is_password_reset_allowed() and adding a few calls to same

#5 @desmith
3 years ago

Here's the suitably-newbie version of the above. Feel free to dissect. :)

This creates the new function is_password_reset_allowed(), and adds calls to it for individual users in the user list table; the user profile page; and the middle of get_password_reset_key().

Note: See TracTickets for help on using tickets.