WordPress.org

Make WordPress Core

Opened 4 months ago

Last modified 4 days ago

#52076 reopened defect (bug)

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

Reported by: peterwilsoncc Owned by: peterwilsoncc
Milestone: 5.8 Priority: normal
Severity: normal Version:
Component: Role/Capability Keywords: early has-patch needs-dev-note has-unit-tests
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 (26)

#1 @peterwilsoncc
4 months ago

  • Description modified (diff)

#2 @peterwilsoncc
4 months 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
4 months 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
4 months ago

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

#6 @peterwilsoncc
4 months 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
4 months 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
3 months 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
3 months 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.

#10 @TimothyBlynJacobs
3 months ago

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.

#11 follow-up: @TimothyBlynJacobs
3 months ago

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.

#12 in reply to: ↑ 11 @peterwilsoncc
3 months ago

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.

#13 @TimothyBlynJacobs
3 months ago

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!

#14 @prbot
3 months ago

peterwilsoncc commented on PR #841:

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.

#15 @prbot
2 months ago

peterwilsoncc commented on PR #841:

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

#16 @peterwilsoncc
2 months ago

  • Milestone changed from 5.7 to Future Release

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

#17 @johnbillion
2 months ago

  • Milestone changed from Future Release to 5.8

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

#18 @SergeyBiryukov
2 months ago

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?

#19 @peterwilsoncc
2 months ago

  • Keywords needs-refresh added
  • Owner set to peterwilsoncc
  • Status changed from new to assigned

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.

#20 @prbot
7 weeks ago

peterwilsoncc commented on PR #841:

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.

#21 @peterwilsoncc
6 weeks ago

  • Keywords has-unit-tests added; needs-unit-tests needs-refresh removed

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.

#22 @peterwilsoncc
6 weeks ago

  • Resolution set to fixed
  • Status changed from assigned to closed

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.

#24 @juliobox
9 days ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

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 ;)

Last edited 9 days ago by juliobox (previous) (diff)

#25 @peterwilsoncc
5 days ago

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

This ticket was mentioned in Slack in #core by chaion07. View the logs.


4 days ago

Note: See TracTickets for help on using tickets.