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

[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.

[peterwilsoncc]

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.

[johnbillion]

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.

[johnbillion]

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

https://core.trac.wordpress.org/ticket/52076

[peterwilsoncc]

@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.

[johnjamesjacoby]

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.

[peterwilsoncc]

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.

@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.

[TimothyBlynJacobs]

So I suppose the obvious risk here is someone writing a map_meta_cap filter that doesn't handle user_id = 0 properly. Some bad code like if ( ! $user_id ) { return []; }. But I imagine there are more plausible scenarios you could end up writing code that would wind up having the same effect.

To alleviate this, I wonder if it makes sense to require exists to be listed in the response from map_meta_cap if the user ID is 0. Ie something like this at the end of ::has_cap().

if ( ! $this->exists() && ! in_array( 'exist', $caps, true ) ) {
    return false;
}

That way we can do things like use read_post for both logged-in and logged out users. But there'd be a much smaller risk of a developer accidentally giving a logged out user permission to do something.

[TimothyBlynJacobs]

Though, thinking on that more. If someone is intentionally doing wp_get_current_user()->has_cap() so that logged out users were handled. If we do care about that risk, I guess we could pass a flag to has_cap when called from (current_)user_can that would enable that stricter checking. But that sounds a bit ugly.

[peterwilsoncc]

Replying to TimothyBlynJacobs:

Though, thinking on that more. If someone is intentionally doing wp_get_current_user()->has_cap() so that logged out users were handled. If we do care about that risk, I guess we could pass a flag to has_cap when called from (current_)user_can that would enable that stricter checking. But that sounds a bit ugly.

Here's a search of ​plugins using the `user_can()` function rather than `current_user_can()`, the regex is ( |(->)|(::))user_can\(. I've also ​searched for the string `exist` with the regex [\'\"]exist[\'\"].

Upon a sample review, I couldn't find any using the exist capability. To discount the logged out user, the documented method is wp_get_current_user()->exists() but it's certainly worth considering that user_can( $u_id, 'exist' ) might be used as a shortcut.

So I suppose the obvious risk here is someone writing a map_meta_cap filter that doesn't handle user_id = 0 properly. Some bad code like if ( ! $user_id ) { return []; }. But I imagine there are more plausible scenarios you could end up writing code that would wind up having the same effect.

I'm not too concerned about this as current_user_can() already frequently passes the user ID 0 for logged out users.

[TimothyBlynJacobs]

I'm not too concerned about this as current_user_can() already frequently passes the user ID 0 for logged out users.

👍 Sounds good to me!

Thanks @johnbillion, I'll follow up these comments during the week.

• for the new functions, I will move these and add a pre_ filter in case an alternative login (eg SSO) plugin needs to override the objects for any reason
• Ideally there wouldn't be any new functions but I needed them to avoid back-compat issues. If you can think of a way to maintain back-compat without the new functions I am all ears.

Thanks again.

I've made the following changes:

• new functions are no longer pluggable
• added the filter pre_get_user_object_by
• The tests attached to WP#38433 have the same outcome on both this branch and master

[peterwilsoncc]

Bumping from milestone. 5.7 is now in beta, it's no longer early

[johnbillion]

This is close to being ready, let's do it 5.8 early.

[SergeyBiryukov]

At a glance, having similar functions with minor differences (get_user_object() vs. get_userdata(), get_user_object_by() vs. get_user_by()) doesn't seem ideal.

I don't mind new functions when they have a clear purpose, but just to fix the 'exist' inconsistency they seem a bit like an overkill :) They might make user_can() clearer in this case, but my concern is that there might be confusion later on when to use the newer ones vs. the older ones.

Perhaps we could instantiate an anonymous user object within user_can() instead?

[peterwilsoncc]

Thanks for taking a look, that makes sense.

This is currently on my list of things to ignore until at least the 5.7 RC1 is out but I'll refresh after that.

I've just made the following changes:

• removed the new functions for getting user objects per @SergeyBiryukov's feedback
• Anonymous user object is created in user_can() if needs be

At the moment, both an invalid and zero user ID are treated as anonymous users ( get_userdata( /* invalid ID */ ) === get_userdata( 0 ) ). That's been the case for a while so I don't really want to change it.

[peterwilsoncc]

PR refreshed following @SergeyBiryukov's comment above. I've added the logged out user to the capability checks.

@jjj has approved the PR so I reckon this is good to go in while it's still early.

[peterwilsoncc]

In 50490:

Roles/Caps: Return same result from current_user_can and user_can().

Ensure current_user_can() and user_can() return the same results for logged out users. For core capabilities this changes user_can( 0, 'exist' ) to return true rather than false in line with current_user_can( 'exist' ) for logged out users.

Convert current_user_can() and current_user_can_for_blog() to wrapper functions ultimately calling user_can().

Add anonymous user to primitive capability checks as appropriate. Convert Tests_User_Capabilities::test_other_caps_for_all_roles() to use a data provider and add tests to check whether user exists in the database (WP_User::exists()) as that intentionally differs from the exist capability.

Props jjj, johnbillion, peterwilsoncc, SergeyBiryukov, TimothyBlynJacobs.
Fixes #52076.

https://core.trac.wordpress.org/changeset/50490

[juliobox]

Hey there, 2 things:
First and most important, there is a regression with this new code.

<?php
// Before this ticket
user_can( 0, 'exist' ); // false
// With this ticket
user_can( 0, 'exist' ); // true

The comment says "User is logged out, create anonymous user object." but what about a non existing user? Before the function could return "false", but now it's impossible to get a false with "exist".
I've read the comment "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."

But I guess that there is also other cases with plugins doing "user_can( $id, 'exist' ) false" and expecting this to be false! And this is a game changer now.

We should keep the possibility of a return false like before:

<?php
        if ( ! $user || ! $user->exists() ) {
                return false;
        }

Because a "false user" is not the same as a "logged-out user". So a fake one does not exists.
Still I agree that a current_user_can( 'exists' ) should always return true, because I mean… I'm the current, so I exist.

Then I've checked the wp repo and I have to admit that I didn't find even 1 plugin where this could cause a bug because they all check if the $user is a WP_User first (​https://wpdirectory.net/search/01F2Q4CJTZFV747PPKFMQR5X53). This leads us to the second part of my comment…

…since we are here now, I would like to suggest to change this:

<?php
function user_can( $user, $capability, ...$args ) {
        if ( ! is_object( $user ) ) {
                $user = get_userdata( $user );
        }

by this:

<?php
function user_can( $user, $capability, ...$args ) {
        if ( ! is_a( $user, 'WP_User' ) ) {
                $user = get_userdata( $user );
        }

to prevent this:

<?php
$new_user = wp_insert_user( [ 'ID' => 1337 ] ); // this ID does not exist
user_can( $new_user, 'manage_options' ); // Fatal error has_cap() method does not exist (or exists() method does not exist with the previous code)

I know, I should test $user myself before inserting/updating the user.
But like #33800 (another of mine), same kind of issue, and this ticket has been merged.
I hope this one will do ;)

[peterwilsoncc]

@juliobox Just a quick acknowledgement of your comment, I currently focused on the 5.7.1 release but will get back to you once that is out the door.

The first change was the purpose of the ticket as, even though it's a change in behavior, it's important that checking permissions returns the same result, no matter which function used. The rest of you comments I'll review after the release.

[peterwilsoncc]

The comment says "User is logged out, create anonymous user object." but what about a non existing user? Before the function could return "false", but now it's impossible to get a false with "exist".

This has been the case with current_user_can() for a while. An invalid ID is treated as an anonymous user in all instances. Both before and after the change above the following code would return true, so user_can() is changing to match the more common permission check.

<?php
wp_set_current_user( 99999 ); // invalid user id
var_dump( current_user_can( 'exist' ) );
// dumps `true`.

As @jjj mentions above, BuddyPress will be changing this to account for the now consistent results between the two functions. I'm unable to find the ticket on the BuddyPress trac, I'm afraid. I'll reach out to John and ask.

A ​search of the plugin repo for the string exist didn't reveal any matches either.

I would like to suggest [... checking the object is a WP_User object to avoid a fatal error]

Testing with prior to the commit above, the following code has caused a fatal error for some time.

<?php
$user = new stdClass();
user_can( $user, 'any_capability' );
// Fatal error.

I agree it's worth discussing defensive coding to avoid this but think it's best to go on a follow up ticket as it's not a new issue following the changes on this ticket.

Are you able to create a the follow up ticket (so you can be properly credited with the idea). Please mention me on the ticket and I will put it on the 5.8 milestone.

[peterwilsoncc]

Per discussion in today's 5.8 Beta 1 release bug scrub in #core, reclosing this to add defensive code to ensure the object is an instance of WP_User in a follow up ticket.