SECURITY: Make $wgBlockDisablesLogin also restrict logged in permissions

Does both Title and user related methods, so it catches things that only
call $wgUser->isAllowed( 'read' ), as well as giving a nicer error message
for things that use $title->userCan().

Otherwise, the user can still do stuff and read pages if they have an
ongoing session.

Issue reported by Multichill

Bug: T129738
Change-Id: Ic929a385fa81c27cbc6ac3a0862f51190d3ae993

diff --git a/includes/Block.php b/includes/Block.php
index bcbf494..79b31bb 100644 (file)
--- a/includes/Block.php
+++ b/includes/Block.php
@@ -969,28 +969,40 @@ class Block {
 
        /**
         * Get/set whether the Block prevents a given action
-        * @param string $action
-        * @param bool|null $x
-        * @return bool
+        *
+        * @param string $action Action to check
+        * @param bool|null $x Value for set, or null to just get value
+        * @return bool|null Null for unrecognized rights.
         */
        public function prevents( $action, $x = null ) {
+               global $wgBlockDisablesLogin;
+               $res = null;
                switch ( $action ) {
                        case 'edit':
                                # For now... <evil laugh>
-                               return true;
-
+                               $res = true;
+                               break;
                        case 'createaccount':
-                               return wfSetVar( $this->mCreateAccount, $x );
-
+                               $res = wfSetVar( $this->mCreateAccount, $x );
+                               break;
                        case 'sendemail':
-                               return wfSetVar( $this->mBlockEmail, $x );
-
+                               $res = wfSetVar( $this->mBlockEmail, $x );
+                               break;
                        case 'editownusertalk':
-                               return wfSetVar( $this->mDisableUsertalk, $x );
-
-                       default:
-                               return null;
+                               $res = wfSetVar( $this->mDisableUsertalk, $x );
+                               break;
+                       case 'read':
+                               $res = false;
+                               break;
                }
+               if ( !$res && $wgBlockDisablesLogin ) {
+                       // If a block would disable login, then it should
+                       // prevent any action that all users cannot do
+                       $anon = new User;
+                       $res = $anon->isAllowed( $action ) ? $res : true;
+               }
+
+               return $res;
        }
 
        /**

diff --git a/includes/Title.php b/includes/Title.php
index ed445cc..2021e0a 100644 (file)
--- a/includes/Title.php
+++ b/includes/Title.php
@@ -2271,13 +2271,17 @@ class Title implements LinkTarget {
         * @return array List of errors
         */
        private function checkUserBlock( $action, $user, $errors, $rigor, $short ) {
+               global $wgEmailConfirmToEdit, $wgBlockDisablesLogin;
                // Account creation blocks handled at userlogin.
                // Unblocking handled in SpecialUnblock
                if ( $rigor === 'quick' || in_array( $action, [ 'createaccount', 'unblock' ] ) ) {
                        return $errors;
                }
 
-               global $wgEmailConfirmToEdit;
+               // Optimize for a very common case
+               if ( $action === 'read' && !$wgBlockDisablesLogin ) {
+                       return $errors;
+               }
 
                if ( $wgEmailConfirmToEdit && !$user->isEmailConfirmed() ) {
                        $errors[] = [ 'confirmedittext' ];
@@ -2434,6 +2438,7 @@ class Title implements LinkTarget {
                        $checks = [
                                'checkPermissionHooks',
                                'checkReadPermissions',
+                               'checkUserBlock', // for wgBlockDisablesLogin
                        ];
                # Don't call checkSpecialsAndNSPermissions or checkCSSandJSPermissions
                # here as it will lead to duplicate error messages. This is okay to do

diff --git a/includes/user/User.php b/includes/user/User.php
index 83cfa40..8859609 100644 (file)
--- a/includes/user/User.php
+++ b/includes/user/User.php
@@ -3147,6 +3147,22 @@ 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 ) );
+
+                       // If block disables login, we should also remove any
+                       // extra rights blocked users might have, in case the
+                       // blocked user has a pre-existing session (T129738).
+                       // This is checked here for cases where people only call
+                       // $user->isAllowed(). It is also checked in Title::checkUserBlock()
+                       // to give a better error message in the common case.
+                       $config = RequestContext::getMain()->getConfig();
+                       if (
+                               $this->isLoggedIn() &&
+                               $config->get( 'BlockDisablesLogin' ) &&
+                               $this->isBlocked()
+                       ) {
+                               $anon = new User;
+                               $this->mRights = array_intersect( $this->mRights, $anon->getRights() );
+                       }
                }
                return $this->mRights;
        }