Make WordPress Core

Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#45845 closed defect (bug) (fixed)

Use get_user_by in check_password_reset_key

Reported by: spacedmonkey's profile spacedmonkey Owned by: sergeybiryukov's profile 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 5 years ago.
Patch with suggested fix
45845.2.diff (641 bytes) - added by iworks 5 years ago.
Replace raw SQL by get_user_by.
45845.3.diff (3.1 KB) - added by spacedmonkey 5 years ago.

Download all attachments as: .zip

Change History (27)

@davidbaumwald
5 years ago

Patch with suggested fix

@iworks
5 years ago

Replace raw SQL by get_user_by.

#1 @karpstrucking
5 years ago

  • Keywords has-patch added; needs-patch removed

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


5 years ago

#3 @SergeyBiryukov
5 years ago

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

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

In 44784:

Users: Revert [44780] pending test failure investigation.

See #45845.

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

#11 @davidbaumwald
5 years 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 years ago

@davidbaumwald I haven’t gotten to return to this yet, but I wanted to dig 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.

Version 0, edited 5 years ago by desrosj (next)

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

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


5 years ago

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


5 years ago

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


5 years ago

@spacedmonkey
5 years ago

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