Reverted r92364 (per-namespace permissions).
authorTim Starling <tstarling@users.mediawiki.org>
Mon, 12 Dec 2011 06:03:01 +0000 (06:03 +0000)
committerTim Starling <tstarling@users.mediawiki.org>
Mon, 12 Dec 2011 06:03:01 +0000 (06:03 +0000)
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
includes/DefaultSettings.php
includes/Title.php
includes/User.php
tests/phpunit/includes/ArticleTablesTest.php
tests/phpunit/includes/TitlePermissionTest.php
tests/phpunit/includes/UserTest.php

index 303382a..198953f 100644 (file)
@@ -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).
index 9be89cf..1361cab 100644 (file)
@@ -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.
index adaffc4..4899456 100644 (file)
@@ -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;
index 73bae4a..312a27b 100644 (file)
@@ -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;
                        }
                }
index 55919c2..28d6708 100644 (file)
@@ -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' );
 
index 9594f0c..4c6ad92 100644 (file)
@@ -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" ) {
index 19b72f8..909c0f9 100644 (file)
@@ -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'
                        ),
                );
        }