SECURITY: Move 'UserGetRights' call before application of Session::getAllowedUserRights()

This prevents hook functions from accidentally adding rights that should
be denied based on the session grants.

If some extension really needs to be able to override session grants,
add a new hook where the old call was, with documentation explicitly
warning about the security implications.

Bug: T139670
Change-Id: I6392cf4d7cc9d3ea96554b25bb5f8abb66e9031b

diff --git a/includes/user/User.php b/includes/user/User.php
index 8859609..045fc64 100644 (file)
--- a/includes/user/User.php
+++ b/includes/user/User.php
@@ -3134,6 +3134,7 @@ class User implements IDBAccessObject {
        public function getRights() {
                if ( is_null( $this->mRights ) ) {
                        $this->mRights = self::getGroupPermissions( $this->getEffectiveGroups() );
+                       Hooks::run( 'UserGetRights', [ $this, &$this->mRights ] );
 
                        // Deny any rights denied by the user's session, unless this
                        // endpoint has no sessions.
@@ -3144,7 +3145,6 @@ class User implements IDBAccessObject {
                                }
                        }
 
-                       Hooks::run( 'UserGetRights', [ $this, &$this->mRights ] );
                        // Force reindexation of rights when a hook has unset one of them
                        $this->mRights = array_values( array_unique( $this->mRights ) );
 

diff --git a/tests/phpunit/includes/user/UserTest.php b/tests/phpunit/includes/user/UserTest.php
index bd076ba..985554b 100644 (file)
--- a/tests/phpunit/includes/user/UserTest.php
+++ b/tests/phpunit/includes/user/UserTest.php
@@ -92,6 +92,57 @@ class UserTest extends MediaWikiTestCase {
                $this->assertNotContains( 'nukeworld', $rights );
        }
 
+       /**
+        * @covers User::getRights
+        */
+       public function testUserGetRightsHooks() {
+               $user = new User;
+               $user->addGroup( 'unittesters' );
+               $user->addGroup( 'testwriters' );
+               $userWrapper = TestingAccessWrapper::newFromObject( $user );
+
+               $rights = $user->getRights();
+               $this->assertContains( 'test', $rights, 'sanity check' );
+               $this->assertContains( 'runtest', $rights, 'sanity check' );
+               $this->assertContains( 'writetest', $rights, 'sanity check' );
+               $this->assertNotContains( 'nukeworld', $rights, 'sanity check' );
+
+               // Add a hook manipluating the rights
+               $this->mergeMwGlobalArrayValue( 'wgHooks', [ 'UserGetRights' => [ function ( $user, &$rights ) {
+                       $rights[] = 'nukeworld';
+                       $rights = array_diff( $rights, [ 'writetest' ] );
+               } ] ] );
+
+               $userWrapper->mRights = null;
+               $rights = $user->getRights();
+               $this->assertContains( 'test', $rights );
+               $this->assertContains( 'runtest', $rights );
+               $this->assertNotContains( 'writetest', $rights );
+               $this->assertContains( 'nukeworld', $rights );
+
+               // Add a Session that limits rights
+               $mock = $this->getMockBuilder( stdclass::class )
+                       ->setMethods( [ 'getAllowedUserRights', 'deregisterSession', 'getSessionId' ] )
+                       ->getMock();
+               $mock->method( 'getAllowedUserRights' )->willReturn( [ 'test', 'writetest' ] );
+               $mock->method( 'getSessionId' )->willReturn(
+                       new MediaWiki\Session\SessionId( str_repeat( 'X', 32 ) )
+               );
+               $session = MediaWiki\Session\TestUtils::getDummySession( $mock );
+               $mockRequest = $this->getMockBuilder( FauxRequest::class )
+                       ->setMethods( [ 'getSession' ] )
+                       ->getMock();
+               $mockRequest->method( 'getSession' )->willReturn( $session );
+               $userWrapper->mRequest = $mockRequest;
+
+               $userWrapper->mRights = null;
+               $rights = $user->getRights();
+               $this->assertContains( 'test', $rights );
+               $this->assertNotContains( 'runtest', $rights );
+               $this->assertNotContains( 'writetest', $rights );
+               $this->assertNotContains( 'nukeworld', $rights );
+       }
+
        /**
         * @dataProvider provideGetGroupsWithPermission
         * @covers User::getGroupsWithPermission