Use get_user_by in check_password_reset_key

[spacedmonkey]

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.

[davidbaumwald]

Patch with suggested fix

[iworks]

Replace raw SQL by get_user_by.

[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 :)

[davidbaumwald]

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?

[williampatton]

@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.

[SergeyBiryukov]

In 44780:

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

Props davidbaumwald, iworks, spacedmonkey.
Fixes #45845.

[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.

[SergeyBiryukov]

In 44784:

Users: Revert [44780] pending test failure investigation.

See #45845.

[davidbaumwald]

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?

[davidbaumwald]

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?

[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.

[davidbaumwald]

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 passses 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.

[davidbaumwald]

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.

[spacedmonkey]

Related: #47021

[spacedmonkey]

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.

[pento]

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

[SergeyBiryukov]

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.

[SergeyBiryukov]

In 45716:

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

Props davidbaumwald, iworks, spacedmonkey.
Fixes #45845.

[SergeyBiryukov]

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.

[SergeyBiryukov]

In 45717:

Users: Clean user cache in check_password_reset_key() tests.

Props davidbaumwald, spacedmonkey.
See #45845.