WordPress.org

Make WordPress Core

Opened 5 years ago

Last modified 3 months ago

#28020 new defect (bug)

get_userdata and wp_get_current_user do not share WP_User instance

Reported by: rmccue Owned by:
Milestone: Priority: normal
Severity: normal Version:
Component: Users Keywords: has-patch has-unit-tests
Focuses: Cc:
PR Number:

Description

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.

Attachments (3)

28020.diff (3.5 KB) - added by donmhico 3 months ago.
Patch + unit test.
28020.1.diff (4.4 KB) - added by donmhico 3 months ago.
Fix infinite loop issue in first patch.
28020.2.diff (4.4 KB) - added by donmhico 2 months ago.
Correct @since to adhere to WPCS.

Download all attachments as: .zip

Change History (8)

#1 @rmccue
5 years ago

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

#2 @chriscct7
4 years ago

  • Keywords needs-patch added

@donmhico
3 months ago

Patch + unit test.

#3 @donmhico
3 months ago

  • Keywords has-patch has-unit-tests added; needs-patch removed

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.

  1. 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 );
}
  1. 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/

Last edited 3 months ago by donmhico (previous) (diff)

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


3 months ago

@donmhico
3 months ago

Fix infinite loop issue in first patch.

#5 @donmhico
3 months ago

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
2 months ago

Correct @since to adhere to WPCS.

Note: See TracTickets for help on using tickets.