Opened 6 years ago
Last modified 6 years ago
#7658 new defect (bug)
User is de-authenticated when making REST API request
Reported by: |
|
Owned by: | |
---|---|---|---|
Milestone: | Awaiting Contributions | Priority: | normal |
Severity: | normal | Version: | |
Component: | Core | Keywords: | |
Cc: | dcavins |
Description
In BuddyPress, we store the current user ID in the $bp
global. The function bp_loggedin_user_id()
then refers to this value. I wonder if there's any value in storing this value, and if instead we should let WP handle the current user ID, and simplify bp_loggedin_user_id()
to call get_current_user_id()
and pass it through the bp_loggedin_user_id
filter.
One case in particular caused me to look more into this. When making a WP REST API request, if the request doesn't include a valid nonce, the current user is downgraded to "not logged in" status. However, because BP has already stashed the current user ID before the downgrade happens in rest_cookie_check_errors()
, bp_loggedin_user_id()
doesn't match the value of get_current_user_id()
. The return values of these functions looks like this, for instance:
bp_loggedin_user_id: 1 get_current_user_id: 0
On the other hand, it's pretty handy for cookie authentication to continue to work.
Thanks for your thoughts on how best to resolve this inconsistency.
Change History (8)
#3
@
6 years ago
At the end of the run, we don't really end up with a WP_User object. We grab the user data in raw form and store it as userdata
:
$bp->displayed_user->userdata = WP_User::get_data_by( 'id', $user_id ); $bp->loggedin_user->userdata = WP_User::get_data_by( 'id', $user_id );
Then add some other some data, like:
$bp->loggedin_user->fullname = $bp->loggedin_user->userdata->display_name; $bp->loggedin_user->is_super_admin = $bp->loggedin_user->is_site_admin = is_super_admin( bp_loggedin_user_id() ); $bp->loggedin_user->domain = bp_core_get_user_domain( bp_loggedin_user_id() );
It's kind of interesting.
#4
@
6 years ago
This came from a Slack discussion, see nested conversation here: https://wordpress.slack.com/archives/C02RQBYUG/p1513187013000352
The REST downgrade is interesting in that I think the point was always that those two user objects be clones of one-another, so however that variable is being nerfed, it should be bubbling back up to $bp->current_user, I would have assumed.
These aren't objects, like @dcavins shows above.
As per the report, this downgrade is done in rest_cookie_check_errors()
. This happens late - after init
- I think this occurs during parse_request
, so it's after BuddyPress initialises its initial settings.
#5
@
6 years ago
We don't have to fix the entire thing right now (particularly, displayed_user
) - the bug report is about the consequence of the downgrade for loggedin_user
, and I'd rather we get a fix for that, and not tie ourselves to trying to re-architecture the rest of this at the same time (that'd be a separate enhancement).
I think we replace the contents of all bp_loggedin_*()
functions with calls to WordPress equivalents.
And for backwards compatibility, we replace $bp->loggedin_user
with an object that has magic getter to handle id
, is_super_admin
, domain
etc (the properties @dcavins mentioned), and we no longer touch that global property directly .
#6
@
6 years ago
Unless I'm missing something, adding a magic getter is going to require some new architecture, because $bp->loggedin_user
is a stdClass
. As I understand it, we'd have to make loggedin_user
a first-class class to add methods. Is that correct?
If so, it seems like more duplication of what WP is already doing with the current user. Most of the current user stuff could be switched to this style:
function bp_loggedin_user_id() { /** * Filters the ID of the currently logged-in user. * * @since 1.0.0 * * @param int $id ID of the currently logged-in user. */ return (int) apply_filters( 'bp_loggedin_user_id', get_current_user_id() ); }
Similarly, to fetch the current user's display name (fullname), we could use get_the_author_meta( 'display_name', get_current_user_id() )
. (I'm assuming that these values are cached by WP. Edit: They are--everything boils down to WP_User::get_data_by
which uses the user cache.)
The exception I believe is the domain, which is calculated by BP.
I've thought about this a lot, and these types of multisite work-arounds do come back as problems frequently.
I forget if BuddyPress does anything to the
current_user
anddisplayed_user
objects that's different from core WordPressWP_User
objects, but I think the answer is yes.WP_User
object-based functions, and add__get()
magic support when something tries to look at$bp->current_user
WP_User
object directly insteadThe REST downgrade is interesting in that I think the point was always that those two user objects be clones of one-another, so however that variable is being nerfed, it should be bubbling back up to
$bp->current_user
, I would have assumed.