Make WordPress Core

Opened 12 months ago

Closed 10 months ago

Last modified 10 months ago

#58194 closed defect (bug) (fixed)

Users: Don't show 'Reset Password' link if password reset is not allowed

Reported by: ocean90's profile ocean90 Owned by: audrasjb's profile audrasjb
Milestone: 6.3 Priority: normal
Severity: normal Version: 5.7
Component: Users Keywords: good-first-bug has-patch needs-testing
Focuses: administration Cc:

Description

With the allow_password_reset filter it's possible to disallow a password reset per user. The value of this filter is not checked when adding the 'Reset Password' link to the list table actions added in #34281. In such cases you still get a "Password reset link sent." notice but no email is actually sent.

Relevant part in core: https://core.trac.wordpress.org/browser/trunk/src/wp-admin/includes/class-wp-users-list-table.php?rev=55276&marks=502-507#L502

Change History (22)

#1 @johnbillion
12 months ago

  • Keywords needs-patch added

This ticket was mentioned in PR #4389 on WordPress/wordpress-develop by Cshark1.


12 months ago
#2

  • Keywords has-patch added; needs-patch removed

If the allow_password_reset filter is set to false, the admin panel still shows the Send Password reset, even though this does nothing. This PR adds a check to see if the filter is set to false and if it is set to false it does not show the button.

See: https://core.trac.wordpress.org/ticket/58194

#3 @cshark
12 months ago

Without this patch, the Send password reset button appears for all the users, even though the user uty has this option disabled though the allow_password_reset filter.
https://i.imgur.com/ZpA4mTu.png

With the patch, the option is not displayed for that user.
https://i.imgur.com/vDW6vqN.png

#5 @ocean90
10 months ago

@cshark Thank you for the patch. Based on this I created a new PR where I have introduced wp_is_password_reset_allowed_for_user() which can be used to DRY up the code a bit because there's also a link on the profile page which shouldn't be visible either.

#6 @ocean90
10 months ago

  • Milestone changed from Future Release to 6.3

#7 @oglekler
10 months ago

  • Keywords needs-testing added

#8 @robinwpdeveloper
10 months ago

We need to test in 3 different places.

  • "Send Reset Link" button on user profile screen.
  • "Send password reset" option in the user list bulk action dropdown.
  • "Send password reset" quick action when hovering over a username in the user list.

Ref: https://core.trac.wordpress.org/ticket/34281#comment:87

Last edited 10 months ago by robinwpdeveloper (previous) (diff)

#9 @tahmina1du
10 months ago

Test Report

This report validates that the indicated patch doesn't address the issue.

Patch tested: https://github.com/WordPress/wordpress-develop/pull/4573

Environment

  • OS: macOS 13.2.1
  • Web Server: Nginx
  • PHP: 7.4.30
  • WordPress: 6.3-beta2-56100-src
  • Browser: Chrome 114.0.5735.198
  • Theme: Twenty Twenty-Three
  • Active Plugins:
    • Demo Plugin (to use the filter)

Actual Results

  • ❌ Issue not resolved with the patch.

Additional Notes

Checked in 3 places.

  • ✅ "Send Reset Link" button on user profile screen.
  • ❌ "Send password reset" option in the user list bulk action dropdown.
  • ✅ "Send password reset" quick action when hovering over a username in the user list.

After disabling the reset password using the filter, I see 2 out of 3 places are fixed. However, bulk action is still having Send password reset option.

Supplemental Artifacts

Screenshots after applying patch:

#10 @kraftbj
10 months ago

Regarding the test report, I think this may be acceptable for the patch landing.

The logic is that there is a filter for allowing password resets generally and/or specific users. In order to know we should not display the bulk item, we need to confirm that the filter is set to disallow generally and set to disallow for all users (at least those listed on the page).

There's also not going to be a good way, IMO, to indicate which users could or could not have a reset sent in that scenario.

This ticket was mentioned in Slack in #core-test by robinwpdeveloper. View the logs.


10 months ago

#12 @robinwpdeveloper
10 months ago

I agree with @kraftbj
We can skip => "Send password reset" option in the user list bulk action dropdown.

Remaining 2 occurrences are working fine.

PR is good to land.

#13 @audrasjb
10 months ago

  • Owner set to audrasjb
  • Status changed from new to reviewing

#14 @audrasjb
10 months ago

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

In 56150:

Users: Remove password reset links when the feature is not allowed for a specific user.

This also introduces wp_is_password_reset_allowed_for_user() which returns false when password reset is not allowed for a specific user. This can be
filtered by developers using the existing allow_password_reset hook.

Props ocean90, cshark, robinwpdeveloper, tahmina1du, kraftbj.
Fixes #58194.

#15 @swissspidy
10 months ago

When exactly would wp_is_password_reset_allowed_for_user() return WP_Error? Just when the return value is filtered? If so, the possible type should be added to the filter's docblock.

#16 @audrasjb
10 months ago

In 56151:

Coding Standards: Add missing newline at the end of wp-admin/user.php.

Follow-up to [56150].

See #58194.

#19 @audrasjb
10 months ago

Good point @swissspidy.
I'm not sure there is any value in assuming that wp_is_password_reset_allowed_for_user() may return WP_Error, even using the filter. In that case, it may also return a string, an array, etc.

#20 @audrasjb
10 months ago

  • Keywords needs-testing removed

Probably better to simply remove WP_Error from the @return declaration.

#21 @swissspidy
10 months ago

Then the is_wp_error() check is superfluous as well.

#22 @ocean90
10 months ago

  • Keywords needs-testing added

The WP_Error is just for back-compat as it was already possible before, see https://github.com/WordPress/wordpress-develop/blob/bea4307daa21a3f97d159898b0ac676332e0e7de/src/wp-includes/user.php#L2903-L2905.

Since it wasn't documented in the filter before I didn't want to promote this usage further but since it's possible it's important to document at least the return value otherwise a non-true check will cause issues. I think [56150] is good as is.

Note: See TracTickets for help on using tickets.