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: |
|
Owned by: | |
---|---|---|---|
Milestone: | 5.8 | Priority: | normal |
Severity: | normal | Version: | |
Component: | Users | Keywords: | has-patch has-unit-tests early |
Focuses: | rest-api | Cc: |
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)
Change History (11)
#3
@
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.
- 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 usingwp_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.
- 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 aswp_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 );
}
- Lastly, I added a new param -
$force
with default value offalse
- in the functionwp_set_current_user()
. It is becausewp_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/
This ticket was mentioned in Slack in #core by donmhico. View the logs.
19 months ago
#5
@
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.
#6
@
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
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
Just hit this again. Extremely annoying, and insane to debug.