From 1fb5d73612b8137641bf3229c9a873323193cf97 Mon Sep 17 00:00:00 2001 From: Bryan Tong Minh Date: Sat, 16 Jul 2011 16:09:00 +0000 Subject: [PATCH] First steps for bug 14801: add backend support for per-namespace permissions to core. This extends $wgGroupPermissions syntax from $wgGroupPermissions[$group][$right] = bool to $wgGroupPermissions[$group][$right] = array( NS_X => bool ). This is safely backwards compatible; the booleans are still fully supported, and any unset namespace will default to false. * User::getRights(), User::isAllowed() and User::getGroupPermissions now optionally accept a namespace parameter. If not set, it will check whether the user has the right for all namespaces. * Anything that uses Title::getUserPermissionsErrorsInternal() automatically supports per-namespace permissions. This includes Title::getUserPermissionsErrors and Title::(quick)UserCan. * Fix tests that set User::mRights The next step would be to change all User::isAllowed() to Title::quickUserCan or pass the namespace to User::isAllowed(). --- RELEASE-NOTES-1.19 | 1 + includes/DefaultSettings.php | 4 + includes/Title.php | 49 ++++---- includes/User.php | 77 ++++++++++--- tests/phpunit/includes/ArticleTablesTest.php | 2 +- .../phpunit/includes/TitlePermissionTest.php | 14 ++- tests/phpunit/includes/UserTest.php | 108 +++++++++++++++++- 7 files changed, 206 insertions(+), 49 deletions(-) diff --git a/RELEASE-NOTES-1.19 b/RELEASE-NOTES-1.19 index 1f6b3988d8..71d7da14d6 100644 --- a/RELEASE-NOTES-1.19 +++ b/RELEASE-NOTES-1.19 @@ -29,6 +29,7 @@ production. hooks have been removed. * New hook "Collation::factory" to allow extensions to create custom category collations. +* $wgGroupPermissions now supports per namespace permissions. === New features in 1.19 === * BREAKING CHANGE: action=watch / action=unwatch now requires a token. diff --git a/includes/DefaultSettings.php b/includes/DefaultSettings.php index 2b1e148e68..0d0f28eb3b 100644 --- a/includes/DefaultSettings.php +++ b/includes/DefaultSettings.php @@ -3304,6 +3304,10 @@ $wgEmailConfirmToEdit = false; * unable to perform certain essential tasks or access new functionality * when new permissions are introduced and default grants established. * + * If set to an array instead of a boolean, it is assumed that the array is in + * NS => bool form in order to support per-namespace permissions. Note that + * this feature does not fully work for all permission types. + * * Functionality to make pages inaccessible has not been extensively tested * for security. Use at your own risk! * diff --git a/includes/Title.php b/includes/Title.php index 6646a75498..d3d6b66f0a 100644 --- a/includes/Title.php +++ b/includes/Title.php @@ -1239,34 +1239,33 @@ class Title { * @return Array list of errors */ private function checkQuickPermissions( $action, $user, $errors, $doExpensiveQueries, $short ) { + $ns = $this->getNamespace(); + if ( $action == 'create' ) { - if ( ( $this->isTalkPage() && !$user->isAllowed( 'createtalk' ) ) || - ( !$this->isTalkPage() && !$user->isAllowed( 'createpage' ) ) ) { + if ( ( $this->isTalkPage() && !$user->isAllowed( 'createtalk', $ns ) ) || + ( !$this->isTalkPage() && !$user->isAllowed( 'createpage', $ns ) ) ) { $errors[] = $user->isAnon() ? array( 'nocreatetext' ) : array( 'nocreate-loggedin' ); } } elseif ( $action == 'move' ) { - if ( !$user->isAllowed( 'move-rootuserpages' ) - && $this->mNamespace == NS_USER && !$this->isSubpage() ) { + if ( !$user->isAllowed( 'move-rootuserpages', $ns ) + && $ns == NS_USER && !$this->isSubpage() ) { // Show user page-specific message only if the user can move other pages $errors[] = array( 'cant-move-user-page' ); } // Check if user is allowed to move files if it's a file - if ( $this->mNamespace == NS_FILE && !$user->isAllowed( 'movefile' ) ) { + if ( $ns == NS_FILE && !$user->isAllowed( 'movefile', $ns ) ) { $errors[] = array( 'movenotallowedfile' ); } - if ( !$user->isAllowed( 'move' ) ) { + if ( !$user->isAllowed( 'move', $ns) ) { // User can't move anything - global $wgGroupPermissions; - $userCanMove = false; - if ( isset( $wgGroupPermissions['user']['move'] ) ) { - $userCanMove = $wgGroupPermissions['user']['move']; - } - $autoconfirmedCanMove = false; - if ( isset( $wgGroupPermissions['autoconfirmed']['move'] ) ) { - $autoconfirmedCanMove = $wgGroupPermissions['autoconfirmed']['move']; - } + + $userCanMove = in_array( 'move', User::getGroupPermissions( + array( 'user' ), $ns ), true ); + $autoconfirmedCanMove = in_array( 'move', User::getGroupPermissions( + array( 'autoconfirmed' ), $ns ), true ); + if ( $user->isAnon() && ( $userCanMove || $autoconfirmedCanMove ) ) { // custom message if logged-in users without any special rights can move $errors[] = array( 'movenologintext' ); @@ -1275,20 +1274,20 @@ class Title { } } } elseif ( $action == 'move-target' ) { - if ( !$user->isAllowed( 'move' ) ) { + if ( !$user->isAllowed( 'move', $ns ) ) { // User can't move anything $errors[] = array( 'movenotallowed' ); - } elseif ( !$user->isAllowed( 'move-rootuserpages' ) - && $this->mNamespace == NS_USER && !$this->isSubpage() ) { + } elseif ( !$user->isAllowed( 'move-rootuserpages', $ns ) + && $ns == NS_USER && !$this->isSubpage() ) { // Show user page-specific message only if the user can move other pages $errors[] = array( 'cant-move-to-user-page' ); } - } elseif ( !$user->isAllowed( $action ) ) { + } elseif ( !$user->isAllowed( $action, $ns ) ) { // We avoid expensive display logic for quickUserCan's and such $groups = false; if ( !$short ) { $groups = array_map( array( 'User', 'makeGroupLinkWiki' ), - User::getGroupsWithPermission( $action ) ); + User::getGroupsWithPermission( $action, $ns ) ); } if ( $groups ) { @@ -1440,9 +1439,9 @@ class Title { if ( $right == 'sysop' ) { $right = 'protect'; } - if ( $right != '' && !$user->isAllowed( $right ) ) { + if ( $right != '' && !$user->isAllowed( $right, $this->mNamespace ) ) { // Users with 'editprotected' permission can edit protected pages - if ( $action == 'edit' && $user->isAllowed( 'editprotected' ) ) { + if ( $action == 'edit' && $user->isAllowed( 'editprotected', $this->mNamespace ) ) { // Users with 'editprotected' permission cannot edit protected pages // with cascading option turned on. if ( $this->mCascadeRestriction ) { @@ -1483,7 +1482,7 @@ class Title { if ( isset( $restrictions[$action] ) ) { foreach ( $restrictions[$action] as $right ) { $right = ( $right == 'sysop' ) ? 'protect' : $right; - if ( $right != '' && !$user->isAllowed( $right ) ) { + if ( $right != '' && !$user->isAllowed( $right, $this->mNamespace ) ) { $pages = ''; foreach ( $cascadingSources as $page ) $pages .= '* [[:' . $page->getPrefixedText() . "]]\n"; @@ -1519,7 +1518,9 @@ class Title { if( $title_protection['pt_create_perm'] == 'sysop' ) { $title_protection['pt_create_perm'] = 'protect'; // B/C } - if( $title_protection['pt_create_perm'] == '' || !$user->isAllowed( $title_protection['pt_create_perm'] ) ) { + if( $title_protection['pt_create_perm'] == '' || + !$user->isAllowed( $title_protection['pt_create_perm'], + $this->mNamespace ) ) { $errors[] = array( 'titleprotected', User::whoIs( $title_protection['pt_user'] ), $title_protection['pt_reason'] ); } } diff --git a/includes/User.php b/includes/User.php index d93efa87ce..ccbd792889 100644 --- a/includes/User.php +++ b/includes/User.php @@ -2240,16 +2240,29 @@ class User { /** * Get the permissions this user has. + * @param $ns int If numeric, get permissions for this namespace * @return Array of String permission names */ - function getRights() { + function getRights( $ns = null ) { + $key = is_null( $ns ) ? '*' : intval( $ns ); + if ( is_null( $this->mRights ) ) { - $this->mRights = self::getGroupPermissions( $this->getEffectiveGroups() ); - wfRunHooks( 'UserGetRights', array( $this, &$this->mRights ) ); + $this->mRights = array(); + } + + if ( !isset( $this->mRights[$key] ) ) { + $this->mRights[$key] = self::getGroupPermissions( $this->getEffectiveGroups(), $ns ); + wfRunHooks( 'UserGetRights', array( $this, &$this->mRights[$key], $ns ) ); // Force reindexation of rights when a hook has unset one of them - $this->mRights = array_values( $this->mRights ); + $this->mRights[$key] = array_values( $this->mRights[$key] ); + } + if ( is_null( $ns ) ) { + return $this->mRights[$key]; + } else { + // Merge non namespace specific rights + return array_merge( $this->mRights[$key], $this->getRights() ); } - return $this->mRights; + } /** @@ -2351,7 +2364,7 @@ class User { } $this->loadGroups(); $this->mGroups[] = $group; - $this->mRights = User::getGroupPermissions( $this->getEffectiveGroups( true ) ); + $this->mRights = null; $this->invalidateCache(); } @@ -2381,7 +2394,7 @@ class User { } $this->loadGroups(); $this->mGroups = array_diff( $this->mGroups, array( $group ) ); - $this->mRights = User::getGroupPermissions( $this->getEffectiveGroups( true ) ); + $this->mRights = null; $this->invalidateCache(); } @@ -2434,9 +2447,10 @@ class User { /** * Internal mechanics of testing a permission * @param $action String + * @paramn $ns int Namespace optional * @return bool */ - public function isAllowed( $action = '' ) { + public function isAllowed( $action = '', $ns = null ) { if ( $action === '' ) { return true; // In the spirit of DWIM } @@ -2448,7 +2462,7 @@ class User { } # Use strict parameter to avoid matching numeric 0 accidentally inserted # by misconfiguration: 0 == 'foo' - return in_array( $action, $this->getRights(), true ); + return in_array( $action, $this->getRights( $ns ), true ); } /** @@ -3397,26 +3411,51 @@ class User { * @param $groups Array of Strings List of internal group names * @return Array of Strings List of permission key names for given groups combined */ - static function getGroupPermissions( $groups ) { + static function getGroupPermissions( $groups, $ns = null ) { global $wgGroupPermissions, $wgRevokePermissions; $rights = array(); - // grant every granted permission first + + // Grant every granted permission first foreach( $groups as $group ) { if( isset( $wgGroupPermissions[$group] ) ) { - $rights = array_merge( $rights, - // array_filter removes empty items - array_keys( array_filter( $wgGroupPermissions[$group] ) ) ); + $rights = array_merge( $rights, self::extractRights( + $wgGroupPermissions[$group], $ns ) ); } } - // now revoke the revoked permissions + + // Revoke the revoked permissions foreach( $groups as $group ) { if( isset( $wgRevokePermissions[$group] ) ) { - $rights = array_diff( $rights, - array_keys( array_filter( $wgRevokePermissions[$group] ) ) ); + $rights = array_diff( $rights, self::extractRights( + $wgRevokePermissions[$group], $ns ) ); } } return array_unique( $rights ); } + + /** + * Helper for User::getGroupPermissions + * @param array $list + * @param int $ns + * @return array + */ + private static function extractRights( $list, $ns ) { + $rights = array(); + foreach( $list as $right => $value ) { + if ( is_array( $value ) ) { + # This is a list of namespaces where the permission applies + if ( !is_null( $ns ) && !empty( $value[$ns] ) ) { + $rights[] = $right; + } + } else { + # This is a boolean indicating that the permission applies + if ( $value ) { + $rights[] = $right; + } + } + } + return $rights; + } /** * Get all the groups who have a given permission @@ -3424,11 +3463,11 @@ class User { * @param $role String Role to check * @return Array of Strings List of internal group names with the given permission */ - static function getGroupsWithPermission( $role ) { + static function getGroupsWithPermission( $role, $ns = null ) { global $wgGroupPermissions; $allowedGroups = array(); foreach ( $wgGroupPermissions as $group => $rights ) { - if ( isset( $rights[$role] ) && $rights[$role] ) { + if ( in_array( $role, self::getGroupPermissions( array( $group ), $ns ), true ) ) { $allowedGroups[] = $group; } } diff --git a/tests/phpunit/includes/ArticleTablesTest.php b/tests/phpunit/includes/ArticleTablesTest.php index 01776c9590..ef8e255cae 100644 --- a/tests/phpunit/includes/ArticleTablesTest.php +++ b/tests/phpunit/includes/ArticleTablesTest.php @@ -11,7 +11,7 @@ class ArticleTablesTest extends MediaWikiLangTestCase { $title = Title::newFromText("Bug 14404"); $article = new Article( $title ); $wgUser = new User(); - $wgUser->mRights = array( 'createpage', 'edit', 'purge' ); + $wgUser->mRights['*'] = array( 'createpage', 'edit', 'purge' ); $wgLanguageCode = 'es'; $wgContLang = Language::factory( 'es' ); diff --git a/tests/phpunit/includes/TitlePermissionTest.php b/tests/phpunit/includes/TitlePermissionTest.php index 1b179686cf..e9e83dd533 100644 --- a/tests/phpunit/includes/TitlePermissionTest.php +++ b/tests/phpunit/includes/TitlePermissionTest.php @@ -56,11 +56,17 @@ class TitlePermissionTest extends MediaWikiLangTestCase { } function setUserPerm( $perm ) { - if ( is_array( $perm ) ) { - $this->user->mRights = $perm; - } else { - $this->user->mRights = array( $perm ); + // Setting member variables is evil!!! + + if ( !is_array( $perm ) ) { + $perm = array( $perm ); + } + for ($i = 0; $i < 100; $i++) { + $this->user->mRights[$i] = $perm; } + + // Hack, hack hack ... + $this->user->mRights['*'] = $perm; } function setTitle( $ns, $title = "Main_Page" ) { diff --git a/tests/phpunit/includes/UserTest.php b/tests/phpunit/includes/UserTest.php index df91aca897..6fa730ebea 100644 --- a/tests/phpunit/includes/UserTest.php +++ b/tests/phpunit/includes/UserTest.php @@ -1,7 +1,11 @@ savedRevokedPermissions = $GLOBALS['wgRevokePermissions']; $this->setUpPermissionGlobals(); + $this->setUpUser(); } private function setUpPermissionGlobals() { global $wgGroupPermissions, $wgRevokePermissions; + # Data for regular $wgGroupPermissions test $wgGroupPermissions['unittesters'] = array( + 'test' => true, 'runtest' => true, 'writetest' => false, 'nukeworld' => false, ); $wgGroupPermissions['testwriters'] = array( + 'test' => true, 'writetest' => true, 'modifytest' => true, ); - + # Data for regular $wgRevokePermissions test $wgRevokePermissions['formertesters'] = array( 'runtest' => true, ); + + # Data for namespace based $wgGroupPermissions test + $wgGroupPermissions['unittesters']['writedocumentation'] = array( + NS_MAIN => false, NS_UNITTEST => true, + ); + $wgGroupPermissions['testwriters']['writedocumentation'] = true; + + } + private function setUpUser() { + $this->user = new User; + $this->user->addGroup( 'unittesters' ); } + public function tearDown() { parent::tearDown(); @@ -55,4 +75,90 @@ class UserTest extends MediaWikiTestCase { $this->assertNotContains( 'modifytest', $rights ); $this->assertNotContains( 'nukeworld', $rights ); } + + public function testNamespaceGroupPermissions() { + $rights = User::getGroupPermissions( array( 'unittesters' ) ); + $this->assertNotContains( 'writedocumentation', $rights ); + + $rights = User::getGroupPermissions( array( 'unittesters' ) , NS_MAIN ); + $this->assertNotContains( 'writedocumentation', $rights ); + $this->assertNotContains( 'modifytest', $rights ); + + $rights = User::getGroupPermissions( array( 'unittesters' ), NS_HELP ); + $this->assertNotContains( 'writedocumentation', $rights ); + $this->assertNotContains( 'modifytest', $rights ); + + $rights = User::getGroupPermissions( array( 'unittesters' ), NS_UNITTEST ); + $this->assertContains( 'writedocumentation', $rights ); + + $rights = User::getGroupPermissions( + array( 'unittesters', 'testwriters' ), NS_MAIN ); + $this->assertContains( 'writedocumentation', $rights ); + } + + public function testUserPermissions() { + $rights = $this->user->getRights(); + $this->assertContains( 'runtest', $rights ); + $this->assertNotContains( 'writetest', $rights ); + $this->assertNotContains( 'modifytest', $rights ); + $this->assertNotContains( 'nukeworld', $rights ); + $this->assertNotContains( 'writedocumentation', $rights ); + + $rights = $this->user->getRights( NS_MAIN ); + $this->assertNotContains( 'writedocumentation', $rights ); + $this->assertNotContains( 'modifytest', $rights ); + + $rights = $this->user->getRights( NS_HELP ); + $this->assertNotContains( 'writedocumentation', $rights ); + $this->assertNotContains( 'modifytest', $rights ); + + $rights = $this->user->getRights( NS_UNITTEST ); + $this->assertContains( 'writedocumentation', $rights ); + } + + /** + * @dataProvider provideGetGroupsWithPermission + */ + public function testGetGroupsWithPermission( $expected, $right, $ns ) { + $result = User::getGroupsWithPermission( $right, $ns ); + sort( $result ); + sort( $expected ); + + $this->assertEquals( $expected, $result, "Groups with permission $right" . + ( is_null( $ns ) ? '' : "in namespace $ns" ) ); + } + public function provideGetGroupsWithPermission() { + return array( + array( + array( 'unittesters', 'testwriters' ), + 'test', + null + ), + array( + array( 'unittesters' ), + 'runtest', + null + ), + array( + array( 'testwriters' ), + 'writetest', + null + ), + array( + array( 'testwriters' ), + 'modifytest', + null + ), + array( + array( 'testwriters' ), + 'writedocumentation', + NS_MAIN + ), + array( + array( 'unittesters', 'testwriters' ), + 'writedocumentation', + NS_UNITTEST + ), + ); + } } \ No newline at end of file -- 2.20.1