From 3da36a9103bafe8963686764c7a837805c9b2339 Mon Sep 17 00:00:00 2001 From: Tim Starling Date: Mon, 12 Dec 2011 06:03:01 +0000 Subject: [PATCH] Reverted r92364 (per-namespace permissions). This is the wrong configuration format for such a feature, and the wrong interface. We already have certain per-namespace permissions in the Title class, and we didn't need to add extra formal parameters to a whole lot of User methods in order to get them. The feature should be implemented wholly in Title, and the concept of user rights should remain relatively simple and easy to understand, and independent of its many applications, i.e. a user either has a right or doesn't. Rights are just a tool for developing access policies; the complexity should be in the caller. The revert was mostly done by hand, since there were a lot of conflicts. I tried to preserve the gist of conflicting changes in r102187 and r102873. The test changes are not simple reverts, rather I just edited out the per-namespace tests. I reverted the followups r92589 and r104310. --- docs/hooks.txt | 1 - includes/DefaultSettings.php | 5 +- includes/Title.php | 51 ++++++------ includes/User.php | 82 +++++-------------- tests/phpunit/includes/ArticleTablesTest.php | 2 +- .../phpunit/includes/TitlePermissionTest.php | 12 +-- tests/phpunit/includes/UserTest.php | 68 ++------------- 7 files changed, 59 insertions(+), 162 deletions(-) diff --git a/docs/hooks.txt b/docs/hooks.txt index 303382ad2c..198953f140 100644 --- a/docs/hooks.txt +++ b/docs/hooks.txt @@ -2091,7 +2091,6 @@ $user: User object 'UserGetRights': Called in User::getRights() $user: User to get rights for &$rights: Current rights -$namespace: Namespace to get permissions for; if null all namespaces 'UserIsBlockedFrom': Check if a user is blocked from a specific page (for specific block exemptions). diff --git a/includes/DefaultSettings.php b/includes/DefaultSettings.php index 9be89cf8d9..1361cab225 100644 --- a/includes/DefaultSettings.php +++ b/includes/DefaultSettings.php @@ -3379,9 +3379,8 @@ $wgEmailConfirmToEdit = false; /** * Permission keys given to users in each group. - * This is an array where the keys are all groups and each value is either: - * a) An array of the format (right => boolean) - * b) An array of the format (right => namespace => boolean) + * This is an array where the keys are all groups and each value is an + * array of the format (right => boolean). * * The second format is used to support per-namespace permissions. * Note that this feature does not fully work for all permission types. diff --git a/includes/Title.php b/includes/Title.php index adaffc4861..4899456ad5 100644 --- a/includes/Title.php +++ b/includes/Title.php @@ -1559,33 +1559,34 @@ 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', $ns ) ) || - ( !$this->isTalkPage() && !$user->isAllowed( 'createpage', $ns ) ) ) { + if ( ( $this->isTalkPage() && !$user->isAllowed( 'createtalk' ) ) || + ( !$this->isTalkPage() && !$user->isAllowed( 'createpage' ) ) ) { $errors[] = $user->isAnon() ? array( 'nocreatetext' ) : array( 'nocreate-loggedin' ); } } elseif ( $action == 'move' ) { - if ( !$user->isAllowed( 'move-rootuserpages', $ns ) - && $ns == NS_USER && !$this->isSubpage() ) { + if ( !$user->isAllowed( 'move-rootuserpages' ) + && $this->mNamespace == 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 ( $ns == NS_FILE && !$user->isAllowed( 'movefile', $ns ) ) { + if ( $this->mNamespace == NS_FILE && !$user->isAllowed( 'movefile' ) ) { $errors[] = array( 'movenotallowedfile' ); } - if ( !$user->isAllowed( 'move', $ns) ) { + if ( !$user->isAllowed( 'move' ) ) { // User can't move anything - - $userCanMove = in_array( 'move', User::getGroupPermissions( - array( 'user' ), $ns ), true ); - $autoconfirmedCanMove = in_array( 'move', User::getGroupPermissions( - array( 'autoconfirmed' ), $ns ), true ); - + global $wgGroupPermissions; + $userCanMove = false; + if ( isset( $wgGroupPermissions['user']['move'] ) ) { + $userCanMove = $wgGroupPermissions['user']['move']; + } + $autoconfirmedCanMove = false; + if ( isset( $wgGroupPermissions['autoconfirmed']['move'] ) ) { + $autoconfirmedCanMove = $wgGroupPermissions['autoconfirmed']['move']; + } if ( $user->isAnon() && ( $userCanMove || $autoconfirmedCanMove ) ) { // custom message if logged-in users without any special rights can move $errors[] = array( 'movenologintext' ); @@ -1594,15 +1595,15 @@ class Title { } } } elseif ( $action == 'move-target' ) { - if ( !$user->isAllowed( 'move', $ns ) ) { + if ( !$user->isAllowed( 'move' ) ) { // User can't move anything $errors[] = array( 'movenotallowed' ); - } elseif ( !$user->isAllowed( 'move-rootuserpages', $ns ) - && $ns == NS_USER && !$this->isSubpage() ) { + } elseif ( !$user->isAllowed( 'move-rootuserpages' ) + && $this->mNamespace == 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, $ns ) ) { + } elseif ( !$user->isAllowed( $action ) ) { $errors[] = $this->missingPermissionError( $action, $short ); } @@ -1740,10 +1741,10 @@ class Title { if ( $right == 'sysop' ) { $right = 'protect'; } - if ( $right != '' && !$user->isAllowed( $right, $this->mNamespace ) ) { + if ( $right != '' && !$user->isAllowed( $right ) ) { // Users with 'editprotected' permission can edit protected pages // without cascading option turned on. - if ( $action != 'edit' || !$user->isAllowed( 'editprotected', $this->mNamespace ) + if ( $action != 'edit' || !$user->isAllowed( 'editprotected' ) || $this->mCascadeRestriction ) { $errors[] = array( 'protectedpagetext', $right ); @@ -1780,7 +1781,7 @@ class Title { if ( isset( $restrictions[$action] ) ) { foreach ( $restrictions[$action] as $right ) { $right = ( $right == 'sysop' ) ? 'protect' : $right; - if ( $right != '' && !$user->isAllowed( $right, $this->mNamespace ) ) { + if ( $right != '' && !$user->isAllowed( $right ) ) { $pages = ''; foreach ( $cascadingSources as $page ) $pages .= '* [[:' . $page->getPrefixedText() . "]]\n"; @@ -1817,8 +1818,8 @@ class Title { $title_protection['pt_create_perm'] = 'protect'; // B/C } if( $title_protection['pt_create_perm'] == '' || - !$user->isAllowed( $title_protection['pt_create_perm'], - $this->mNamespace ) ) { + !$user->isAllowed( $title_protection['pt_create_perm'] ) ) + { $errors[] = array( 'titleprotected', User::whoIs( $title_protection['pt_user'] ), $title_protection['pt_reason'] ); } } @@ -1950,7 +1951,7 @@ class Title { } # If the user is allowed to read pages, he is allowed to read all pages - if ( $user->isAllowed( 'read', $this->mNamespace ) ) { + if ( $user->isAllowed( 'read' ) ) { return $errors; } @@ -2017,7 +2018,7 @@ class Title { } $groups = array_map( array( 'User', 'makeGroupLinkWiki' ), - User::getGroupsWithPermission( $action, $this->mNamespace ) ); + User::getGroupsWithPermission( $action ) ); if ( count( $groups ) ) { global $wgLang; diff --git a/includes/User.php b/includes/User.php index 73bae4a1db..312a27bbf0 100644 --- a/includes/User.php +++ b/includes/User.php @@ -2316,29 +2316,16 @@ class User { /** * Get the permissions this user has. - * @param $ns int If numeric, get permissions for this namespace * @return Array of String permission names */ - public function getRights( $ns = null ) { - $key = is_null( $ns ) ? '*' : intval( $ns ); - + public function getRights() { if ( is_null( $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 ) ); + $this->mRights = self::getGroupPermissions( $this->getEffectiveGroups() ); + wfRunHooks( 'UserGetRights', array( $this, &$this->mRights ) ); // Force reindexation of rights when a hook has unset one of them - $this->mRights[$key] = array_values( $this->mRights[$key] ); + $this->mRights = array_values( $this->mRights ); } - 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; } /** @@ -2463,7 +2450,7 @@ class User { } $this->loadGroups(); $this->mGroups[] = $group; - $this->mRights = null; + $this->mRights = User::getGroupPermissions( $this->getEffectiveGroups( true ) ); $this->invalidateCache(); } @@ -2493,7 +2480,7 @@ class User { } $this->loadGroups(); $this->mGroups = array_diff( $this->mGroups, array( $group ) ); - $this->mRights = null; + $this->mRights = User::getGroupPermissions( $this->getEffectiveGroups( true ) ); $this->invalidateCache(); } @@ -2550,10 +2537,9 @@ class User { /** * Internal mechanics of testing a permission * @param $action String - * @param $ns int|null Namespace optional * @return bool */ - public function isAllowed( $action = '', $ns = null ) { + public function isAllowed( $action = '' ) { if ( $action === '' ) { return true; // In the spirit of DWIM } @@ -2565,7 +2551,7 @@ class User { } # Use strict parameter to avoid matching numeric 0 accidentally inserted # by misconfiguration: 0 == 'foo' - return in_array( $action, $this->getRights( $ns ), true ); + return in_array( $action, $this->getRights(), true ); } /** @@ -3544,70 +3530,40 @@ class User { * Get the permissions associated with a given list of groups * * @param $groups Array of Strings List of internal group names - * @param $ns int - * * @return Array of Strings List of permission key names for given groups combined */ - public static function getGroupPermissions( array $groups, $ns = null ) { + public static function getGroupPermissions( $groups ) { 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, self::extractRights( - $wgGroupPermissions[$group], $ns ) ); + $rights = array_merge( $rights, + // array_filter removes empty items + array_keys( array_filter( $wgGroupPermissions[$group] ) ) ); } } - - // Revoke the revoked permissions + // now revoke the revoked permissions foreach( $groups as $group ) { if( isset( $wgRevokePermissions[$group] ) ) { - $rights = array_diff( $rights, self::extractRights( - $wgRevokePermissions[$group], $ns ) ); + $rights = array_diff( $rights, + array_keys( array_filter( $wgRevokePermissions[$group] ) ) ); } } return array_unique( $rights ); } - /** - * Helper for User::getGroupPermissions - * @param $list array - * @param $ns int - * @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 * * @param $role String Role to check - * @param $ns int - * - * * @return Array of Strings List of internal group names with the given permission */ - public static function getGroupsWithPermission( $role, $ns = null ) { + public static function getGroupsWithPermission( $role ) { global $wgGroupPermissions; $allowedGroups = array(); foreach ( $wgGroupPermissions as $group => $rights ) { - if ( in_array( $role, self::getGroupPermissions( array( $group ), $ns ), true ) ) { + if ( isset( $rights[$role] ) && $rights[$role] ) { $allowedGroups[] = $group; } } diff --git a/tests/phpunit/includes/ArticleTablesTest.php b/tests/phpunit/includes/ArticleTablesTest.php index 55919c2c7d..28d67087b4 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 9594f0c631..4c6ad929f8 100644 --- a/tests/phpunit/includes/TitlePermissionTest.php +++ b/tests/phpunit/includes/TitlePermissionTest.php @@ -62,15 +62,11 @@ class TitlePermissionTest extends MediaWikiLangTestCase { function setUserPerm( $perm ) { // Setting member variables is evil!!! - if ( !is_array( $perm ) ) { - $perm = array( $perm ); - } - for ($i = 0; $i < 100; $i++) { - $this->user->mRights[$i] = $perm; + if ( is_array( $perm ) ) { + $this->user->mRights = $perm; + } else { + $this->user->mRights = array( $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 19b72f82d2..909c0f958e 100644 --- a/tests/phpunit/includes/UserTest.php +++ b/tests/phpunit/includes/UserTest.php @@ -38,13 +38,6 @@ class UserTest extends MediaWikiTestCase { $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; @@ -79,88 +72,41 @@ class UserTest extends MediaWikiTestCase { $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 ); + public function testGetGroupsWithPermission( $expected, $right ) { + $result = User::getGroupsWithPermission( $right ); sort( $result ); sort( $expected ); - $this->assertEquals( $expected, $result, "Groups with permission $right" . - ( is_null( $ns ) ? '' : "in namespace $ns" ) ); + $this->assertEquals( $expected, $result, "Groups with permission $right" ); } public function provideGetGroupsWithPermission() { return array( array( array( 'unittesters', 'testwriters' ), - 'test', - null + 'test' ), array( array( 'unittesters' ), - 'runtest', - null + 'runtest' ), array( array( 'testwriters' ), - 'writetest', - null + 'writetest' ), array( array( 'testwriters' ), - 'modifytest', - null - ), - array( - array( 'testwriters' ), - 'writedocumentation', - NS_MAIN - ), - array( - array( 'unittesters', 'testwriters' ), - 'writedocumentation', - NS_UNITTEST + 'modifytest' ), ); } -- 2.20.1