Make WordPress Core

Opened 7 years ago

Last modified 5 weeks ago

#28020 new defect (bug)

get_userdata and wp_get_current_user do not share WP_User instance

Reported by: rmccue Owned by:
Milestone: 5.8 Priority: normal
Severity: normal Version:
Component: Users Keywords: has-patch has-unit-tests early
Focuses: rest-api Cc:


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' );

(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 19 months ago.
Patch + unit test.
28020.1.diff (4.4 KB) - added by donmhico 19 months ago.
Fix infinite loop issue in first patch.
28020.2.diff (4.4 KB) - added by donmhico 19 months ago.
Correct @since to adhere to WPCS.

Download all attachments as: .zip

Change History (11)

#1 @rmccue
7 years ago

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

#2 @chriscct7
5 years ago

  • Keywords needs-patch added

19 months ago

Patch + unit test.

#3 @donmhico
19 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(
    '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.


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

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

19 months ago

19 months ago

Fix infinite loop issue in first patch.

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

19 months ago

Correct @since to adhere to WPCS.

#6 @TimothyBlynJacobs
5 months ago

  • Focuses rest-api added
  • Milestone set to 5.7

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.

This ticket was mentioned in PR #880 on WordPress/wordpress-develop by peterwilsoncc.

7 weeks ago


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

#8 @TimothyBlynJacobs
5 weeks ago

  • Keywords early added
  • Milestone changed from 5.7 to 5.8

Bumping. This will need to land early.

Note: See TracTickets for help on using tickets.