WordPress.org

Make WordPress Core

Opened 5 weeks ago

Last modified 13 days ago

#52076 new defect (bug)

Checking anonymous user's exist capability returns inconsistent results across functions.

Reported by: peterwilsoncc Owned by:
Milestone: 5.7 Priority: normal
Severity: normal Version:
Component: Role/Capability Keywords: early has-patch needs-unit-tests needs-dev-note
Focuses: Cc:

Description (last modified by peterwilsoncc)

While looking at extending the capability checks to include the anonymous users, I've noticed the exist capability returns different results depending on how it is checked.

As noted in WP_User, all users are allowed to exist including the anonymous and invalid user IDs. (An invalid user ID in wp_set_current_user() sets the site to use the anonymous user).

Running the following in a WP CLI shell will demonstrate the problem:

wp> wp_set_current_user( 0 )
// Logs anon user object
wp> current_user_can( 'exist' );
bool(true)
wp> wp_get_current_user()->has_cap( 'exist' );
bool(true)
wp> user_can( 0, 'exist' );
bool(false)
wp> wp_get_current_user()->exists()
bool(false)
wp> user_can( wp_get_current_user(), 'exist' );
bool(false)

In an ideal world, each of these would return the correct result (true) consistently.

Such changes have backward compatibility concerns so it would be good to get other's thoughts on the ability to change this to be consistent.

Change History (9)

#1 @peterwilsoncc
5 weeks ago

  • Description modified (diff)

#2 @peterwilsoncc
5 weeks ago

Following up on the notes above with night brain thoughts:

$anon_user->exists();

The exists (plural) function is to check if the user exists in the database. The logged out user does not so this is returning false as expected.

// With current user set to logged out
user_can( 0, 'exist' );
current_user_can( 'exist' );

Both of these in there various forms ought to return true as they are checking the exist capability which both logged in and logged out users have.

#3 @johnbillion
3 weeks ago

  • Keywords early added

This is caused by the guard condition in user_can() which returns false early if the user doesn't exist.

For clarification:

  • Any check for the exist capability should return true because... well because. This is an anomaly which has existed since current_user_can() was introduced and it's been noted that some plugins such as BuddyPress make use of this behaviour.
  • The WP_User::exists() functionality is working as expected.

#4 @johnbillion
3 weeks ago

cc @johnjamesjacoby as we've chatted about this in the past but I don't recall where.

#6 @peterwilsoncc
3 weeks ago

@johnbillion @johnjamesjacoby Draft pull request with POC is up.

  • Changes user_can to allow anonymous users to exist
  • current_user_can() becomes a wrapper of user_can()
  • current_user_can_for_blog() uses current_user_can() to determine if the user can.
  • capabilities group of tests is passing on both single and multisite test suite
  • leaving the full suite to run on GH.

#7 @johnjamesjacoby
3 weeks ago

Hey gents!

@johnbillion I remember, but also not fully. Maybe related to init_roles() of init_caps() and site switching? Maybe related to a WP User Profiles thing?

@peterwilsoncc I’m following the code changes in the pull request, and they make perfect sense to me. If BuddyPress needs an internal change to accommodate it, I’m happy to see that through.

The exist() method name is an unfortunate collision against the cap key name. Given the intent of the capability, it could simply always return true, but given the intent of the method that’s obviously not correct.

I don’t think anything needs renaming (it probably would not be very easy to do) but I can imagine others arguing for improved clarity eventually, so it’s worth acknowledging that ::exists() and caps[‘exists’] definitely do different things, perhaps adding even more verbose documentation in the code.

I am comfortable committing this early in trunk if others are. It’s the kind of low-level change that may have unintended consequences, and it seems proper to plan for potential revisions or reverting.

#8 @peterwilsoncc
13 days ago

  • Keywords needs-unit-tests needs-dev-note added
  • Milestone changed from Awaiting Review to 5.7

Thanks both.

As there seems to be some agreement that it should be consistent, I've moved this to the 5.7 milestone.

I'll work up some unit tests on the pull request linked to this ticket.

#9 @prbot
13 days ago

peterwilsoncc commented on PR #841:

@JJJ @johnbillion

Still a bit of a work in progress as many tests are broken in various seemingly random locations.

I've had to make some changes to the WP_User object so get_userdata() works for anonymous users. I think this is causing some back-compatibility issues (thus the tests failing) and could be problematic for sites replacing the pluggable functions to use third party login integrations.

Note: See TracTickets for help on using tickets.