WordPress.org

Make WordPress Core

Opened 6 years ago

Closed 6 years ago

#27205 closed defect (bug) (fixed)

Unit tests: super admin privledges leak to other tests

Reported by: jdgrimes Owned by:
Milestone: 3.9 Priority: normal
Severity: normal Version: 3.9
Component: Build/Test Tools Keywords: has-patch
Focuses: Cc:
PR Number:

Description

If you create a user during a test and grant them super admin privileges, users created in the following tests will also be super admins.

Example:

class Example_Super_Admin_Test extends WP_UnitTestCase {

	/**
	 * This will work as expected.
	 */
	public function test_one() {
		$user_id = $this->factory->user->create();
		grant_super_admin( $user_id );
		$this->assertTrue( is_super_admin( $user_id ) );
	}
	
	/**
	 * This will fail.
	 */
	public function test_two() {
		$user_id = $this->factory->user->create();
		$this->assertFalse( is_super_admin( $user_id ) );
	}
}

The reason is because the factory assigns these two users the same user_login, 'user 1'. But the $super_admins global is not cleared at the end of each test, and so once 'user 1' is added as a super admin in the first test, all users created with that login in the following tests will also have super admin privileges.

The solution is to reset the $super_admins global in our tearDown(), or change the behavior of get_super_admins() to call the database/cache each time instead of relying on the global.

Attachments (5)

27205.diff (506 bytes) - added by jdgrimes 6 years ago.
Query the db/cache each time in get_super_admins()
27205.2.diff (378 bytes) - added by jdgrimes 6 years ago.
Unset $GLOBALS['super_admins'] on setUp()
27205.3.diff (817 bytes) - added by nacin 6 years ago.
27205.4.diff (1.7 KB) - added by jdgrimes 6 years ago.
Don't overwrite the $super_admins global in *_super_admin()
27205.5.diff (1.0 KB) - added by jdgrimes 6 years ago.
Use $GLOBALS['super_admins'] insteaad of bringing it into the current scope

Download all attachments as: .zip

Change History (14)

#1 @SergeyBiryukov
6 years ago

  • Milestone changed from Awaiting Review to 3.9

@jdgrimes
6 years ago

Query the db/cache each time in get_super_admins()

#2 follow-up: @jdgrimes
6 years ago

  • Keywords has-patch dev-feedback added

Do you think 27205.diff is the way to go?

#3 in reply to: ↑ 2 ; follow-up: @SergeyBiryukov
6 years ago

Replying to jdgrimes:

Do you think 27205.diff is the way to go?

Per [14206] and #12815, $super_admins is meant to be set as an override in wp-config.php.

Looks like 27205.diff would break grant_super_admin() and revoke_super_admin().

#4 in reply to: ↑ 3 @jdgrimes
6 years ago

  • Keywords has-patch dev-feedback removed

Replying to SergeyBiryukov:

Replying to jdgrimes:

Do you think 27205.diff is the way to go?

Per [14206] and #12815, $super_admins is meant to be set as an override in wp-config.php.

Looks like 27205.diff would break grant_super_admin() and revoke_super_admin().

Ah, OK. I'll see if I can work up a patch for just the test framework then.

@jdgrimes
6 years ago

Unset $GLOBALS['super_admins'] on setUp()

#5 @jdgrimes
6 years ago

  • Keywords has-patch added

New patch approaches this from the other angle.

@nacin
6 years ago

#6 follow-up: @nacin
6 years ago

Well this is embarrassing. grant_super_admin() only works once per pageload. It checks the $super_admins global and bails if it is set, then proceeds to set it. 27205.3.diff is a unit test that fails.

get_super_admins() consults $super_admins directly, but as this function is called fairly often, I wonder if this was accidentally done for speed purposes.

@jdgrimes
6 years ago

Don't overwrite the $super_admins global in *_super_admin()

@jdgrimes
6 years ago

Use $GLOBALS['super_admins'] insteaad of bringing it into the current scope

#7 in reply to: ↑ 6 @jdgrimes
6 years ago

Replying to nacin:

Well this is embarrassing. grant_super_admin() only works once per pageload. It checks the $super_admins global and bails if it is set, then proceeds to set it. 27205.3.diff is a unit test that fails.

get_super_admins() consults $super_admins directly, but as this function is called fairly often, I wonder if this was accidentally done for speed purposes.

revoke_super_admin() is also contributing to this problem. 27205.5.diff addresses this by checking $GLOBALS['super_admins'] istead of bringing the $super_admins into the functions' scopes. (Ignore 27205.4.diff)

#8 @nacin
6 years ago

In 27706:

Multisite: Don't set the $super_admins global in grant_super_admin(), revoke_super_admin().

Adds tests and docs.

props jdgrimes.
see #27205.

#9 @nacin
6 years ago

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

[27706] solves the leakage issue as well.

Note: See TracTickets for help on using tickets.