WordPress.org

Make WordPress Core

Opened 4 years ago

Closed 11 months ago

Last modified 11 months ago

#20845 closed defect (bug) (fixed)

wp_set_current_user( null, "name") does not work when $current_user is already 0

Reported by: bobbingwide Owned by: boonebgorges
Milestone: 4.4 Priority: normal
Severity: normal Version: 3.3.2
Component: Users Keywords: has-patch needs-testing
Focuses: Cc:

Description (last modified by scribu)

According to the documentation for wp_set_current_user() you can

Set $id to null and specify a name if you do not know a user's ID.

This does not work when the current user has already been set to anonymous ( ID = 0 ) since the following test returns true.

if ( isset( $current_user ) && ( $current_user instanceof WP_User ) && ( $id == $current_user->ID ) ) {

The last part of the test should be corrected to
&& ($id === $current_user->ID) )

which will ensure that the test fails when the $id parameter is null, which will allow the rest of the pluggable function to continue and set the current user to the specified $name.

This is similar to #19769.
Note: the change in 3.4 has not fixed this particular problem.

Attachments (4)

20845.patch (579 bytes) - added by barrykooij 3 years ago.
20845-1.patch (584 bytes) - added by bobbingwide 11 months ago.
Refreshed version of the patch
20845-1.2.patch (1.5 KB) - added by bobbingwide 11 months ago.
Refreshed version with a unit test
20845-1.3.patch (1.8 KB) - added by bobbingwide 11 months ago.
Added the ID trumps name test

Download all attachments as: .zip

Change History (25)

#1 @scribu
4 years ago

  • Description modified (diff)

#2 @bobbingwide
4 years ago

Note: The change in 3.4 was Revision [20410] for #20372

I tried a simple workaround; set to "admin" first.

wp_set_current_user( 1 );
wp_set_current_user( null, "workaround-for-20845" );

Last edited 4 years ago by SergeyBiryukov (previous) (diff)

#3 @scribu
4 years ago

  • Milestone changed from Awaiting Review to 3.4

#4 @nacin
4 years ago

  • Milestone changed from 3.4 to Awaiting Review

I believe bobbingwide was simply pointing out that the change in 3.4, [20410], did not fix this problem, and that it is not a regression (it was reported against 3.3.2).

#5 @bobbingwide
4 years ago

TRUE. I found the problem in 3.3.2 and confirmed that the latest change for 3.4 doesn't address this problem. I suspect that the problem has potentially existed for a long time but was exacerbated by more recent code that sets the current user to 0.

@barrykooij
3 years ago

#6 @barrykooij
3 years ago

I agree on the importance of the difference between 0 and null, I've added a patch with the strict check.

#7 @barrykooij
3 years ago

  • Keywords has-patch added

#8 @markoheijnen
3 years ago

Is this something that can be added to 3.7? or should the milestone be set to 3.8?

#9 @barrykooij
3 years ago

Just talked this over with Nacin, I will write some unit test for this patch ASAP.

#10 @chriscct7
11 months ago

  • Keywords needs-unit-tests needs-refresh added

@bobbingwide
11 months ago

Refreshed version of the patch

@bobbingwide
11 months ago

Refreshed version with a unit test

#11 @bobbingwide
11 months ago

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

I've added a new file which contains a test setting the user by name with ID=null.
and a test demonstrating that name is trumped by ID. e.g. wp_set_current_user( "admin", "bobbingwide" ); will return the "admin" user.

There seems little point in adding tests for setting it by a numeric ID, since this is already implicit in many other tests.

Last edited 11 months ago by bobbingwide (previous) (diff)

@bobbingwide
11 months ago

Added the ID trumps name test

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


11 months ago

#13 @boonebgorges
11 months ago

  • Milestone changed from Awaiting Review to 4.4
  • Owner set to boonebgorges
  • Status changed from new to reviewing

#14 @boonebgorges
11 months ago

In 34945:

Add tests for current behavior of wp_set_current_user().

See #20845.

#15 follow-up: @boonebgorges
11 months ago

Thanks for the patch, bobbingwide. The strict check you've suggested will force a user switch in the following case:

wp_set_current_user( 3 );
wp_set_current_user( '3' );

I don't think we want that. So we'll be a bit more precise and check for $id === null, which is how the $id bypass is supposed to work anyway.

#16 @boonebgorges
11 months ago

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

In 34947:

Allow a null id to do a name lookup in wp_set_current_user().

Previously, the name fallback was failing in the case where the current user
was 0, due to a loose comparison between 0 (the current user) and null (the
value that is used to trigger the name fallback).

Props bobbingwide.
Fixes #20845.

#17 in reply to: ↑ 15 ; follow-up: @bobbingwide
11 months ago

Replying to boonebgorges:

Thanks for the patch, bobbingwide. The strict check you've suggested will force a user switch in the following case:

wp_set_current_user( 3 );
wp_set_current_user( '3' );

I don't think we want that. So we'll be a bit more precise and check for $id === null, which is how the $id bypass is supposed to work anyway.

Good spot. Will there need to be a test where?

ID=3 user_login="three"
ID=4 user_login="3"

#18 in reply to: ↑ 17 ; follow-up: @boonebgorges
11 months ago

Replying to bobbingwide:

Good spot. Will there need to be a test where?

ID=3 user_login="three"
ID=4 user_login="3"

I don't think we need a separate case for that, since $id and $name are separate parameters. The only way they can be mixed up is by developer error, not by internal type mismatches.

#19 @barrykooij
11 months ago

@boonebgorges is it correct I didn't get props here?

#20 @boonebgorges
11 months ago

barrykooij - Dang it, you are correct. Many apologies. You'll be in the 4.4 credits for #22212.

#21 in reply to: ↑ 18 @bobbingwide
11 months ago

Replying to boonebgorges:

Replying to bobbingwide:

Good spot. Will there need to be a test where?

ID=3 user_login="three"
ID=4 user_login="3"

I don't think we need a separate case for that, since $id and $name are separate parameters. The only way they can be mixed up is by developer error, not by internal type mismatches.

For this function it's OK. It's WP_User::__construct() that could do with the test.

Note: See TracTickets for help on using tickets.