WordPress.org

Make WordPress Core

Opened 2 years ago

Closed 2 years ago

#20372 closed enhancement (fixed)

WP_User::exists()

Reported by: markjaquith Owned by:
Milestone: 3.4 Priority: normal
Severity: normal Version:
Component: Users Keywords: has-patch commit
Focuses: Cc:

Description

Would be good to have a canonical way of testing that a WP_User object refers to a real user.

Attachments (6)

20372.diff (586 bytes) - added by ryan 2 years ago.
20372.2.diff (5.3 KB) - added by ryan 2 years ago.
Use it in a few places
20372.3.diff (2.0 KB) - added by ryan 2 years ago.
Sanity check current_user global in get_currentuserinfo()
20372-unit.diff (1.7 KB) - added by ryan 2 years ago.
Unit tests for get and set current
20372.4.diff (1.9 KB) - added by ryan 2 years ago.
Tweaked comment
20372.xmlrpc-caps.diff (866 bytes) - added by duck_ 2 years ago.

Download all attachments as: .zip

Change History (21)

ryan2 years ago

comment:1 nacin2 years ago

  • Component changed from General to Users
  • Keywords has-patch commit added
  • Milestone changed from Awaiting Review to 3.4

WP_Theme has the same method.

ryan2 years ago

Use it in a few places

comment:2 ryan2 years ago

In [20378]:

Introduce WP_User::exists(). see #20372

comment:3 follow-up: nacin2 years ago

Not sure what's going on, but seeing this on WP.org:

Call to a member function exists() on a non-object
File: wp-includes/capabilities.php
Line: 1371
Source: http://cn.wordpress.org/xmlrpc.php

comment:4 in reply to: ↑ 3 duck_2 years ago

Replying to nacin:

Not sure what's going on, but seeing this on WP.org:

Call to a member function exists() on a non-object
File: wp-includes/capabilities.php
Line: 1371
Source: http://cn.wordpress.org/xmlrpc.php

This happens on Multisite installations. ms_site_check() is called in wp-settings.php and it tries to use is_super_admin() which gets NULL from wp_get_current_user() since this is an XML-RPC request.

comment:5 soulseekah2 years ago

Fatal error: Call to a member function exists() on a non-object in /home/www/wordpress.lo/trunk/wp-includes/capabilities.php on line 1371

Call Stack:

0.0004 140172 1. {main}() /home/www/wordpress.lo/trunk/xmlrpc.php:0
0.0008 148492 2. include('/home/www/wordpress.lo/trunk/wp-load.php') /home/www/wordpress.lo/trunk/xmlrpc.php:29
0.0012 155948 3. require_once('/home/www/wordpress.lo/trunk/wp-config.php') /home/www/wordpress.lo/trunk/wp-load.php:29
0.0024 199076 4. require_once('/home/www/wordpress.lo/trunk/wp-settings.php') /home/www/wordpress.lo/trunk/wp-config.php:101
0.3641 8247928 5. ms_site_check() /home/www/wordpress.lo/trunk/wp-settings.php:308
0.3641 8248084 6. is_super_admin() /home/www/wordpress.lo/trunk/wp-includes/ms-load.php:80

Version 0, edited 2 years ago by soulseekah (next)

comment:6 follow-up: nacin2 years ago

I looked at [3430] and wp_xmlrpc_server::login(). I feel like get_currentuserinfo() should call wp_set_current_user(0) in the XMLRPC_REQUEST branch. That would mean that wp_get_current_user() would always return a WP_User object, which it does in literally every other situation.

If wp_get_current_user() returns a non-existent user before login() runs in XML-RPC, I think that's just fine. $current_user should never be null after calling get_currentuserinfo().

I am going to commit this to avoid seeing the fatal error in trunk, but open for discussion for what the best fix would be.

comment:7 follow-up: nacin2 years ago

In [20402]:

wp_set_current_user(0) for XMLRPC_REQUEST in get_currentuserinfo(). Ensures that wp_get_current_user() always returns a WP_User object. see #20372.

comment:8 in reply to: ↑ 6 duck_2 years ago

Replying to nacin:

I looked at [3430] and wp_xmlrpc_server::login(). I feel like get_currentuserinfo() should call wp_set_current_user(0) in the XMLRPC_REQUEST branch. That would mean that wp_get_current_user() would always return a WP_User object, which it does in literally every other situation.

If wp_get_current_user() returns a non-existent user before login() runs in XML-RPC, I think that's just fine. $current_user should never be null after calling get_currentuserinfo().

I was thinking along similar lines. Maybe even explicit wp_set_current_user(0) in WP::init() so we can be sure that the $current_user global is always a WP_User object.

Looking at the code it would require that get_currentuserinto() switches to using the exists() method too.

ryan2 years ago

Sanity check current_user global in get_currentuserinfo()

ryan2 years ago

Unit tests for get and set current

ryan2 years ago

Tweaked comment

comment:10 nacin2 years ago

20372.4.diff looks good. Should prevent a lot of issues. I like it, a lot.

comment:11 ryan2 years ago

In [20410]:

When fetching the user in get_currentuserinfo(), make sure it is a valid WP_User object. If it is stdClass, upgrade it to WP_User. If it is WP_Error, an int, or anything else, set the current user to ID 0.

In wp_set_current_user(), return the current user global only if it is a WP_User object. If it is not, fall through and go about setting it up properly.

Formatting cleanups for both functions.

see #20372

comment:12 scribu2 years ago

  • Cc scribu added

comment:13 in reply to: ↑ 7 duck_2 years ago

Replying to nacin:

In [20402]:

wp_set_current_user(0) for XMLRPC_REQUEST in get_currentuserinfo(). Ensures that wp_get_current_user() always returns a WP_User object. see #20372.

As reported by koke in IRC this breaks caps checks during XML-RPC requests. This is because the call to wp_get_current_user() in current_user_can() makes get_currentuserinfo() override the user set by wp_xmlrpc_server::login().

A solution would be to move the XMLRPC_REQUEST block to come after the check for a non-empty $current_user global, see attached patch.

duck_2 years ago

comment:14 duck_2 years ago

In [20424]:

Don't override the $current_user global in get_currentuserinfo() on an XML-RPC request
if it's non-empty. Fixes capabilities checks for XML-RPC requests. See #20372.

comment:15 nacin2 years ago

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

Think we're good here?

Note: See TracTickets for help on using tickets.