From 6099940734a922dc4fcea57806274aa89ae4ac45 Mon Sep 17 00:00:00 2001
From: Brad Jorsch <bjorsch@wikimedia.org>
Date: Thu, 7 Jul 2016 17:24:50 -0400
Subject: [PATCH] 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
---
 includes/user/User.php                   |  2 +-
 tests/phpunit/includes/user/UserTest.php | 51 ++++++++++++++++++++++++++++++++
 2 files changed, 52 insertions(+), 1 deletion(-)

diff --git a/includes/user/User.php b/includes/user/User.php
index 4a92f65..19e273b 100644
--- a/includes/user/User.php
+++ b/includes/user/User.php
@@ -3253,6 +3253,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.
@@ -3263,7 +3264,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 801ab91..110a5a6 100644
--- a/tests/phpunit/includes/user/UserTest.php
+++ b/tests/phpunit/includes/user/UserTest.php
@@ -93,6 +93,57 @@ class UserTest extends MediaWikiTestCase {
 	}
 
 	/**
+	 * @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, array( '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
 	 */
-- 
2.8.1