Opened 11 years ago
Closed 11 years ago
#27205 closed defect (bug) (fixed)
Unit tests: super admin privledges leak to other tests
Reported by: |
|
Owned by: | |
---|---|---|---|
Milestone: | 3.9 | Priority: | normal |
Severity: | normal | Version: | 3.9 |
Component: | Build/Test Tools | Keywords: | has-patch |
Focuses: | Cc: |
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)
Change History (14)
#2
follow-up:
↓ 3
@
11 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:
↓ 4
@
11 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
@
11 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 inwp-config.php
.
Looks like 27205.diff would break
grant_super_admin()
andrevoke_super_admin()
.
Ah, OK. I'll see if I can work up a patch for just the test framework then.
#6
follow-up:
↓ 7
@
11 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.
#7
in reply to:
↑ 6
@
11 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)
Query the db/cache each time in
get_super_admins()