get_userdata and wp_get_current_user do not share WP_User instance

[rmccue]

Just discovered this fun one.

Steps to reproduce:

$uid = 3;
// UID for this demo should be a subscriber

wp_set_current_user( $uid );

// $GLOBALS['current_user'] now contains a WP_User instance

$from_userdata = get_userdata( $uid );

// $from_userdata now also contains a WP_User instance, but not the same one

// Fun times with desynchronisation:
$from_userdata->set_role( 'administrator' );
assert(current_user_can('administrator'));

(I suspect you can also reproduce by using wp_update_user instead of get_userdata)

This is probably due to the fact that we don't use object caching for the user object instances, which would let us share the instances.

[rmccue]

Just hit this again. Extremely annoying, and insane to debug.

[donmhico]

Patch + unit test.

[donmhico]

So here's how I approached the problem - see my attached patch 28020.diff.

1. Check if what is being fetched inside the function get_user_by() is the same as the current user. If it is, then return the current user object using wp_get_current_user().

if ( get_current_user_id() == $userdata->ID ) {
    return wp_get_current_user();
}

However, after I added the code above and ran the full test suite, bunch of failed tests emerged.

2. After debugging, I found out that the failed tests' origin is from the function clean_user_cache(). The code from no. 1 above returns the current user object instead of a new user object with updated data. So functions that transform the current state of a user object such as wp_set_password() fails if the user in context is the current user.

To solve this, the current user object should be re-setup / re-created if it is passed to clean_user_cache(). Hence the code.

if ( get_current_user_id() == $user->ID ) {
    wp_set_current_user( $user->ID, '', true );
}

3. Lastly, I added a new param - $force with default value of false - in the function wp_set_current_user(). It is because wp_set_current_user() returns the same current user object if the passed $id is the same as the current user. At first I thought of using

wp_set_current_user( 0 )
wp_set_current_user( $current_user_id )

The code above will first "remove" the current user effectively bypassing the check. However, I believe it's not good to go around like this. Another possible solution was to create another function which basically do the same as wp_set_current_user() just without the check so it will always re-setup the current user object. But then again, creating almost duplicate functions are just ticking bombs.

So ultimately, I settled on creating a new param $force and added it to the check.

Also, just to mention, I had to edit the test function getPluggableFunctionSignatures() and add the newly added $force param to pass the test.

'wp_set_current_user' => array(
    'id',
    'name' => '',
    'force' => false // ticket 28020
                        ),

I run the full test suite and it looks good.

OK, but incomplete, skipped, or risky tests!
Tests: 9640, Assertions: 73874, Skipped: 39

Any thoughts and suggestions are greatly appreciated.

References.
​https://developer.wordpress.org/reference/functions/get_user_by/
​https://developer.wordpress.org/reference/functions/wp_get_current_user/
​https://developer.wordpress.org/reference/functions/clean_user_cache/
​https://developer.wordpress.org/reference/functions/wp_set_current_user/

[donmhico]

Fix infinite loop issue in first patch.

[donmhico]

My first patch, 28020.diff, doesn't work. I found out that the code:

if ( get_current_user_id() == $userdata->ID ) {
    return wp_get_current_user();
}

If placed inside get_user_by(), makes the WP run in an infinite loop. It's because wp_get_current_user() has the code

$user_id = apply_filters( 'determine_current_user', false );

Which, if we dive deeper, invokes the function wp_validate_auth_cookie() that also uses get_user_by() creating the infinite loop error.

So instead, I found out that directly using the global $current_user solves the problem. I've attached a new patch, 28020.1.diff, that includes the fix.

@rmccue - You're not kidding. It's extremely annoying and insane to debug.

[donmhico]

Correct @since to adhere to WPCS.

[TimothyBlynJacobs]

Going to try and take a look at this for 5.7 because I think we'll run into this when trying to work on App Password scoping.

https://core.trac.wordpress.org/ticket/28020

Just a copy-paste of the existing patch (28020.2.diff), I can see it needs some work already but just getting it in the nice interface.

cc @TimothyBJacobs

[TimothyBlynJacobs]

Bumping. This will need to land early.

[lukecarbis]

@peterwilsoncc What are the next steps with the Pull Request? You mentioned it needs more work?

[chaion07]

We brought this ticket in a early 5.8 bug scrubs. We are expecting a reply from Peter at his earliest possible convenience. Thanks in advance!

[peterwilsoncc]

I've pushed a couple of further changes to the pull request but super minor things.

I'm not sure of the purpose of the new $force parameter so have asked for some clarification there.

@peterwilsoncc thanks for pinging me. It's been a while since I worked on this one so i'll take a look again why I added the $force param in there. I'll get back to you as soon as I can.

@peterwilsoncc - First, thank you for looking into this and doing improvements. Really appreciate it!

Regarding the addition of $force param in wp_set_current_user(). First we if omit the changes related to $force parameter and just have this changes
{{{php
function get_user_by( $field, $value ) {

global $current_user;

$userdata = WP_User::get_data_by( $field, $value );

if ( ! $userdata ) {

return false;

}

if ( $current_user instanceof WP_User && $current_user->ID === (int) $userdata->ID ) {

return $current_user;

}

}}}
Running the full test suite will result to 8 failures

There were 8 failures:

1) Tests_Auth::test_password_trimming
Failed asserting that WP_Error Object (...) is an instance of class "WP_User".

/var/www/tests/phpunit/tests/auth.php:98

2) Tests_Auth::test_password_length_limit
Failed asserting that WP_Error Object (...) is an instance of class "WP_User".

/var/www/tests/phpunit/tests/auth.php:217

3) Tests_Auth::test_user_activation_key_is_saved
Invalid key.
Failed asserting that WP_Error Object (...) is not an instance of class "WP_Error".

/var/www/tests/phpunit/includes/abstract-testcase.php:609
/var/www/tests/phpunit/tests/auth.php:261

4) Tests_Auth::test_user_activation_key_is_checked
Invalid key.
Failed asserting that WP_Error Object (...) is not an instance of class "WP_Error".

/var/www/tests/phpunit/includes/abstract-testcase.php:609
/var/www/tests/phpunit/tests/auth.php:286

5) WP_Test_REST_Users_Controller::test_update_item_existing_email_case
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-'Editor@example.com'
+'editor@example.com'

/var/www/tests/phpunit/tests/rest-api/rest-users-controller.php:1600

6) WP_Test_REST_Users_Controller::test_user_roundtrip_as_editor
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-'\o/ ¯\_(ツ)_/¯'
+'User 0007480'

/var/www/tests/phpunit/tests/rest-api/rest-users-controller.php:2122
/var/www/tests/phpunit/tests/rest-api/rest-users-controller.php:2159

7) WP_Test_REST_Users_Controller::test_user_roundtrip_as_editor_html
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-'div strong'
+'User 0007480'

/var/www/tests/phpunit/tests/rest-api/rest-users-controller.php:2122
/var/www/tests/phpunit/tests/rest-api/rest-users-controller.php:2210

8) Tests_XMLRPC_wp_editProfile::test_subscriber_profile
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-'http://www.example.org/subscriber'
+''

/var/www/tests/phpunit/tests/xmlrpc/wp/editProfile.php:35

Tracing what's common to these errors is clean_user_cache(). What we want when clean_user_cache( $user ) is invoked, is if the passed $user is the global $current_user, we reset / refresh the $current_user so it will reflect the latest changes.

Now doing just something like this won't work (without $force)

{{{php
function clean_user_cache( $user ) {

if ( is_numeric( $user ) ) {

$user = new WP_User( $user );

}

if ( ! $user->exists() ) {

return;

}

wp_cache_delete( $user->ID, 'users' );
wp_cache_delete( $user->user_login, 'userlogins' );
wp_cache_delete( $user->user_email, 'useremail' );
wp_cache_delete( $user->user_nicename, 'userslugs' );

/

• Fires immediately after the given user's cache is cleaned. *
• @since 4.4.0 *
• @param int $user_id User ID.
• @param WP_User $user User object. */

do_action( 'clean_user_cache', $user->ID, $user );

if ( get_current_user_id() === (int) $user->ID ) {

wp_set_current_user( $user->ID );

}

}
}}}

The code block above won't work because wp_set_current_user( $id ) is returning early the global $current_user without doing anything else if the passed $id is the same as the $current_user

{{{php
function wp_set_current_user( $id, $name = ) {

global $current_user;

If $id matches the current user, there is nothing to do.
if ( isset( $current_user )
&& ( $current_user instanceof WP_User )
&& ( $id == $current_user->ID )
&& ( null !== $id )
) {

return $current_user;

}

}}}

Hence I added $force param to force the re-setup_userdata().

{{{php
function wp_set_current_user( $id, $name = , $force = false ) {

global $current_user;

If $id matches the current user, there is nothing to do.
if ( isset( $current_user )
&& ( $current_user instanceof WP_User )
&& ( $id == $current_user->ID )
&& ( null !== $id )
&& ( ! $force )
) {

return $current_user;

}

$current_user = new WP_User( $id, $name );

setup_userdata( $current_user->ID );

/

• Fires after the current user is set. *
• @since 2.0.1 */

do_action( 'set_current_user' );

return $current_user;

}
}}}

I'm not sure if I explained my mind clearly and I hope this clarifies my reasoning why I added $force. Let me know if you have any other inquiries.

Thanks, that's a great help.

I need to test the code but I am wondering if something like this could work when clearing the cache instead.

{{{php
if ( get_current_user_id() === (int) $user->ID ) {

global $current_user;
unset( $current_user );
wp_set_current_user( $user->ID );

}
}}}

As the function is pluggable, it would be good to avoid the new parameter to avoid the need for third party developers to update it if they have replaced the function.

I've made a change along the lines of my suggestion above in 20e1d684e510652b35da8f2d60e9cc42197a84f6

Using unset() broke a bunch of things (such as authentication for one) so I set the current user to null to force a refresh instead. isset( null ) === false so the effect is the same. ​https://3v4l.org/BZau4

[peterwilsoncc]

The ​linked pull request makes the following changes from 28020.2.diff​ :

• reverted the new $force parameter in the original patch on wp_set_current_user() to avoid changing the signature of a pluggable function
• force refresh the current user's object as part of clean_user_cache() per original patch but using a different technique
• clean_user_cache() immediately rewarms the current user's cache as a result of above. Strictly speaking the function is meant to clean rather than clear but it is a backward compatibility change
• switched to strict comparisons in a few places

I think this is good to go in but given I've made a few changes it would be good to get another set of eyes on it.

[peterwilsoncc]

In 50790:

Users: Share current user instance across functions.

Share the WP_User instance for the current user between the functions get_userdata() and wp_get_current_user(). Both functions return the $current_user global for the current user.

Force refresh the $current_user global within clean_user_cache() by immediately re-calling wp_set_current_user() with the current user's ID. This ensures any changes to the current user's permissions or other settings are reflected in the global. As a side-effect this immediately rewarms the current user's cache.

Props chaion07, chriscct7, donmhico, hellofromtonya, lukecarbis, peterwilsoncc, rmccue, TimothyBlynJacobs.
Fixes #28020.

Committed as https://core.trac.wordpress.org/changeset/50790

[hellofromTonya]

Manual Testing

Steps to Reproduce and manually test:

1. Added ​this testing script file to the wp-content/mu-plugins folder
2. Logged into the test site as an admin
3. Created a test user and set access to Subscriber
4. Viewed the site on the frontend to capture the results from the testing script

Reproduce original problem

Was able to reproduce the problem.

Results: 3 different objects for 1 user ❌

Current user => you
Current user ID is: 1

Has admin rights? 👍

Object source	        Object ID
get_user_by()	           832
$GLOBALS['current_user']   540

Changing to a different user
Current user ID is: 2

Has admin rights? 👎

Object source	         Object ID
get_user_by()	            307
$GLOBALS['current_user'].   833
get_userdata()	            831

Changing different user to be an admin
Current user ID is: 2

Has admin rights? 👎

Object source	           Object ID
get_user_by()	            307
$GLOBALS['current_user']    833
get_userdata()	            831

Notice the object IDs are different, i.e. via ​spl_object_id for the same user when getting from get_user_user_by(), get_userdata(), and the global $GLOBALS['current_user']. One user but 3 different objects.

Testing with the fix applied

Applied the fix and refreshed the test site.

Results: 1 object per user. ✅

Current user => you
Current user ID is: 1

Has admin rights? 👍

Object source	        Object ID
get_user_by()	           540
$GLOBALS['current_user']   540

Changing to a different user
Current user ID is: 2

Has admin rights? 👎

Object source	         Object ID
get_user_by()	            829
$GLOBALS['current_user']    829
get_userdata()	            829

Changing different user to be an admin
Current user ID is: 2

Has admin rights? 👍

Object source	           Object ID
get_user_by()	            829
$GLOBALS['current_user']    829
get_userdata()	            829

Observations

• Can reproduce the problem
• Changeset [50790] fixes it

[SergeyBiryukov]

[50790] was reverted in [54397] / #54984.

Leaving closed as maybelater for now, feel free to reopen if there is any interest in working on this further.