User is de-authenticated when making REST API request

[dcavins]

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.

[johnjamesjacoby]

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 and displayed_user objects that's different from core WordPress WP_User objects, but I think the answer is yes.

• If not, we should switch everything to use the current WP_User object-based functions, and add __get() magic support when something tries to look at $bp->current_user
• If BuddyPress does modify the object, we need to decide if we keep supporting it in perpetuity, and/or if we are more comfortable modifying the current WP_User object directly instead

---

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.

[dcavins]

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.

[DJPaul]

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.

[DJPaul]

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 .

[dcavins]

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.

[DJPaul]

If so, it seems like more duplication of what WP is already doing with the current user.

I know those functions are easy to change; we need to maintain backwards compatibility by not removing the property on the global.