WordPress.org

Make WordPress Core

Opened 7 months ago

Last modified 3 months ago

#45845 reopened defect (bug)

Use get_user_by in check_password_reset_key

Reported by: spacedmonkey Owned by: SergeyBiryukov
Milestone: 5.3 Priority: normal
Severity: normal Version: 3.1
Component: Users Keywords: good-first-bug has-patch
Focuses: performance Cc:

Description

In check_password_reset_key, there is an uncached call to users table. This could easily be replaced with get_user_by that is cached.

Attachments (3)

45845.diff (2.0 KB) - added by davidbaumwald 6 months ago.
Patch with suggested fix
45845.2.diff (641 bytes) - added by iworks 6 months ago.
Replace raw SQL by get_user_by.
45845.3.diff (3.1 KB) - added by spacedmonkey 3 months ago.

Download all attachments as: .zip

Change History (23)

@davidbaumwald
6 months ago

Patch with suggested fix

@iworks
6 months ago

Replace raw SQL by get_user_by.

#1 @karpstrucking
6 months ago

  • Keywords has-patch added; needs-patch removed

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


5 months ago

#3 @SergeyBiryukov
5 months ago

  • Milestone changed from Awaiting Review to 5.2
  • Owner set to SergeyBiryukov
  • Status changed from new to reviewing

#4 follow-up: @williampatton
5 months ago

This looks good to me, I would like to see the redundant checking changed though to be just a single check and error return.

If nobody is available to update the patch I will try and get it done in the next few days :)

#5 in reply to: ↑ 4 @davidbaumwald
5 months ago

Replying to williampatton:

This looks good to me, I would like to see the redundant checking changed though to be just a single check and error return.

If nobody is available to update the patch I will try and get it done in the next few days :)

Which checks do you think should be condensed?

#6 @williampatton
5 months ago

@davidbaumwald

The if ( false === $user ) is followed up by casting data to a different $var and then testing if ( ! $row ). I think we could have both tests performed in a single block.

if ( false === $user || ! $user->data ) could work here instead. Since the same error is thrown on both the tests and no additional context is added to the error message I would prefer to test both at the same time is all.

#7 @SergeyBiryukov
5 months ago

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

In 44780:

Users: Replace raw SQL query in check_password_reset_key() with get_user_by().

Props davidbaumwald, iworks, spacedmonkey.
Fixes #45845.

#8 follow-up: @desrosj
5 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening this because there are some failed unit tests resulting.

I have to step away but wanted to add what I have found so far. I'm not sure what exactly is happening yet, but deleting the user's cache with wp_delete_cache() results in this test passing.

#9 @SergeyBiryukov
5 months ago

In 44784:

Users: Revert [44780] pending test failure investigation.

See #45845.

#10 in reply to: ↑ 8 @davidbaumwald
5 months ago

Replying to desrosj:

Reopening this because there are some failed unit tests resulting.

I have to step away but wanted to add what I have found so far. I'm not sure what exactly is happening yet, but deleting the user's cache with wp_delete_cache() results in this test passing.

So, should the cache be cleared before the test case so that fresh results are fetched when get_user_by is called?

Last edited 5 months ago by davidbaumwald (previous) (diff)

#11 @davidbaumwald
5 months ago

After digging into this a bit more, caching is definitely the cause of the failure. Series of events as follows:

  1. User row is updated in the database with a generated activation key.
  2. Assertion that check_password_reset_key is not WP_Error fails.

Failure is due to the updated code using cached user data. At the time of the above assertion, the user_activation_key is empty.

Expanding upon what @desrosj added, if I add clean_user_cache right after the activation key is created, the tests pass. Maybe that's the solution?

#12 follow-up: @desrosj
5 months ago

@davidbaumwald I haven’t gotten to return to this yet, but my thinking was that digging a bit deeper to understand why the cache does not need to be cleared for other similar tests to ensure there is not a bigger issue buried in these changes somewhere was a good idea.

Last edited 5 months ago by desrosj (previous) (diff)

#13 in reply to: ↑ 12 @davidbaumwald
5 months ago

Replying to desrosj:

@davidbaumwald I haven’t gotten to return to this yet, but my thinking was that digging a bit deeper to understand why the cache does not need to be cleared for other similar tests to ensure there is not a bigger issue buried in these changes somewhere was a good idea.

@desrosj That's a good idea. In scanning the tests in auth.php as an example, I see reasons why this test sticks out as having an issue with caching.

All subsequent tests in the file that call check_password_reset_key are looking for expected errors. However, they are just assertInstanceOf using WP_Error. They aren't checking the specific error. Since the user is cached and the user_activation_key is empty, they will pass these assertions, albeit for the wrong reasons.

There is a preceding test case for test_password_length_limit in which get_user_by is also referenced. That test passes, but that's because the user cache is reset by the wp_set_password before assertions are made.

Let me know if you'd like me to dig into anything else. Happy to help.

Last edited 4 months ago by davidbaumwald (previous) (diff)

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


4 months ago

#15 @davidbaumwald
4 months ago

Adding to my notes, calling clean_user_cache in the case after the DB update would clear only the user cache without affecting all other caches.

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


3 months ago

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


3 months ago

#19 @spacedmonkey
3 months ago

Thanks for the feedback @desrosj . Really helped.

In 45845.3.diff fixes the tests. Just calling clean_user_cache makes the tests pass. I personally believe the old tests were invalid and this fix is fine. Not sure if this breaking change.

I am going to leave this up to @SergeyBiryukov to commit in 5.2 or punt.

#20 @pento
3 months ago

  • Milestone changed from 5.2 to 5.3

The patch needs reviewing, with 5.2 RC1 due out today, I'm bumping it to 5.3.

Note: See TracTickets for help on using tickets.