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.

[dcavins]

I revisited this issue and made the following notes to refresh my memory:
Update on how this works as of BP 12:

$bp->loggedin_user->id is populated via wp_get_current_user() in BP_Core::setup_globals().
$bp->loggedin_user->userdata is mostly populated via bp_core_get_core_userdata() which is actually a call to BP_Core_User::get_core_userdata( $user_id ) which relies on WP_User::get_data_by( 'id', $user_id ). Note that WP_User::get_data_by() doesn't return a user object; it returns the user data from the match in the users database table. The related WP wrapper function get_user_by() also uses WP_User::get_data_by() internally but initializes and returns a WP_User object.

Adding a logger to BP_REST_Members_Endpoint::get_item_permissions_check() yields the following when not passing a valid nonce:
bp_loggedin_user_id: 1, get_current_user_id: 0 (BP stores the user based on early cookie validation; WP's idea of the user is changed at rest_cookie_check_errors())
This is true for BP 12 and BP 12 + BP Classic, in WP 6.3+.

I've come up with a pretty simple outline of a solution that I'll attach a patch for that introduces a new class BP_LoggedIn_User which uses a magic __get() to fetch property values from WP as needed. I think it's what John was hinting at above. Anyway, it seems to work, and running the unit tests fails on two tests that look like they should fail:

1) BP_Tests_BP_Groups_Group_TestCases::test_is_member_property
Failed asserting that 354 is false.

/var/www/html/wp-content/plugins/buddypress/tests/phpunit/testcases/groups/class-bp-groups-group.php:1762

2) BP_Tests_BP_Groups_Group_TestCases::test_is_invited_property
Failed asserting that 40 is false.

/var/www/html/wp-content/plugins/buddypress/tests/phpunit/testcases/groups/class-bp-groups-group.php:1788.

[dcavins]

Introduce new class BP_LoggedIn_User.

[imath]

@dcavins Thanks for looking deeper into it, I wonder if we could have a BP_User class to serve as parent to BP_LoggedIn_User (and why not to BP_Displayed_User) and that could be used to bp_get_user( $id ) ? I had a similar idea once: see #8371

[dcavins]

Hi @imath Ha, my first thought about this was also to build a BP_User class that extends WP_User, then I decided to just stick to solving this specific problem, which is that the "loggedin_user" isn't persistent--it can change over the course of the WP load process.

I definitely think adding a true BP_User would help standardize interactions/storage of all of the info that makes up a BP User, but it isn't a necessary step in solving this one weird problem. :)

If we were to add BP_User, this class could become an extension of that class, and I'd be happy to work on BP_User for BP v15.

[dcavins]

Introduce new class BP_LoggedIn_User.

[dcavins]

I'm not sure we have decided that we are doing this, but I've uploaded the current best version of this class in case we come back to it.

In the new version, I'm handling "fullname" and "domain" in the class, also, which simplifies some logic throughout BP and in the tests. One change is that I'm supporting the situation where the admin has disabled BP-> WP profile sync--we used to do this but that logic appears to have been lost. (See src/bp-xprofile/bp-xprofile-functions.php xprofile_override_user_fullnames() where we're currently using bp_core_get_user_displayname() which in turn uses get_the_author_meta( 'display_name' ), so the xprofile fullname field isn't used.)

[imath]

@dcavins thanks for updating the patch, I think there's something wrong with 2 unit tests:

1. BP_Tests_BP_Groups_Group_TestCases::test_is_member_property
2. BP_Tests_BP_Groups_Group_TestCases::test_is_invited_property

Both are using wp_set_current_user( $users[1] );

I've tried to look at the first one, and if I dump $users[1], it's not the same user ID than bp_loggedin_user_id(). It looks like bp_loggedin_user_id() is not updated if wp_set_current_user(); is used.

So I doubt unlike what you wrote earlier, these tests should fail. Could you have a look at it?

[dcavins]

Hi @imath Thanks for your feedback! I must be misunderstanding the failing tests. I've added some notes to the test code below:

<?php
public function test_is_member_property() {
        $users = self::factory()->user->create_many( 2 );

        $g = self::factory()->group->create( array(
                'creator_id' => $users[0],
        ) );

        wp_set_current_user( $users[1] );

        $group_a = new BP_Groups_Group( $g );
        // Current user is not yet a member of the group, membership should be false.
        $this->assertFalse( $group_a->is_member );

        // The current user is added to the group.
        $this->add_user_to_group( $users[1], $g );
        $group_b = new BP_Groups_Group( $g );
        // Current user should be a member, why are we checking for false here?
        // This is the check that fails.
        $this->assertFalse( $group_b->is_member );
}

public function test_is_invited_property() {
        $users = self::factory()->user->create_many( 2 );

        $g = self::factory()->group->create( array(
                'creator_id' => $users[0],
        ) );

        wp_set_current_user( $users[1] );

        $group_a = new BP_Groups_Group( $g );
        // Current user has not yet been invited to the group, is_invited should be false.
        $this->assertFalse( $group_a->is_invited );

        groups_invite_user( array(
                'user_id'    => $users[1],
                'group_id'   => $g,
                'inviter_id' => $users[0],
                'send_invite' => 1
        ) );

        $group_b = new BP_Groups_Group( $g );
        // Current user has been invited to the group, is_invited should be true, but we are looking for false?
        $this->assertFalse( $group_b->is_invited );
}

When I run this code with the new class, it behaves as expected:

<?php
var_dump( "logged in?", bp_loggedin_user_id(), ); // Outputs 1
var_dump( "switch current user?", wp_set_current_user( 2 )->ID ); // Outputs 2
var_dump( "logged in after?", bp_loggedin_user_id() ); // Outputs 2

It does fail with the current master branch:

<?php
var_dump( "logged in?", bp_loggedin_user_id(), ); // Outputs 1
var_dump( "switch current user?", wp_set_current_user( 2 )->ID ); // Outputs 2
var_dump( "logged in after?", bp_loggedin_user_id() ); // Outputs 1

Thanks!

[espellcaste]

We will continue this ticket in the next version.

[espellcaste]

@imath or @dcavins What's the latest status of this ticket? I wonder if we could fix it for the V2 of the REST API.

[dcavins]

Hi @espellcaste I'm a fan of this solution (the new BP_LoggedIn_User class approach), and definitely would consider adding a new BP_User class that contains some of our extras (as an extension of WP_User), then making BP_LoggedIn_User an extension of BP_User.

I believe Mathieu has some existential doubts about it, though, so let's see what he thinks.

[imath]

@dcavins

and definitely would consider adding a new BP_User class that contains some of our extras (as an extension of WP_User), then making BP_LoggedIn_User an extension of BP_User.

Go for it! You have my full support!

[espellcaste]

@dcavins I guess you have the green light.