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.