WordPress.org

Make WordPress Core

Opened 15 months ago

Closed 8 months ago

Last modified 8 months ago

#45845 closed defect (bug) (fixed)

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 15 months ago.
Patch with suggested fix
45845.2.diff (641 bytes) - added by iworks 15 months ago.
Replace raw SQL by get_user_by.
45845.3.diff (3.1 KB) - added by spacedmonkey 12 months ago.

Download all attachments as: .zip

Change History (27)

@davidbaumwald
15 months ago

Patch with suggested fix

@iworks
15 months ago

Replace raw SQL by get_user_by.

#1 @karpstrucking
14 months ago

  • Keywords has-patch added; needs-patch removed

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


14 months ago

#3 @SergeyBiryukov
14 months ago

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

#4 follow-up: @williampatton
14 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
14 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
14 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
14 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
14 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
14 months ago

In 44784:

Users: Revert [44780] pending test failure investigation.

See #45845.

#10 in reply to: ↑ 8 @davidbaumwald
14 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 14 months ago by davidbaumwald (previous) (diff)

#11 @davidbaumwald
13 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
13 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 13 months ago by desrosj (previous) (diff)

#13 in reply to: ↑ 12 @davidbaumwald
13 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 13 months ago by davidbaumwald (previous) (diff)

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


13 months ago

#15 @davidbaumwald
13 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.


12 months ago

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


12 months ago

#19 @spacedmonkey
12 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
12 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.

#21 follow-up: @SergeyBiryukov
8 months ago

Since [45714] and [45715], get_password_reset_key() uses wp_update_user(), which clears the cache.

So reintroducing [44780] as is seems fine now, without the extra clean_user_cache() calls. The tests pass for me.

#22 @SergeyBiryukov
8 months ago

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

In 45716:

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

Props davidbaumwald, iworks, spacedmonkey.
Fixes #45845.

#23 in reply to: ↑ 21 @SergeyBiryukov
8 months ago

Replying to SergeyBiryukov:

So reintroducing [44780] as is seems fine now, without the extra clean_user_cache() calls.

Ah, since the tests use a direct update query instead of get_password_reset_key(), the extra clean_user_cache() calls are still needed.

#24 @SergeyBiryukov
8 months ago

In 45717:

Users: Clean user cache in check_password_reset_key() tests.

Props davidbaumwald, spacedmonkey.
See #45845.

Note: See TracTickets for help on using tickets.