Revert "Revert "Title::checkUserBlock should call User::isBlockedFrom for every action""

This reverts commit 91fc7480301c8b2404cc280511c2fc83a6c3aa5d.

Hence it reapplies I6312a36911e5b73d773452fefef7ff25b9af08a4.

The changes made by this commit are (excluding the cases
checked for earlier):
* An action can only be blocked if prevents() is true/null and
isBlockedFrom() is truthy. Previously, any action other than
'edit' or 'create' would be blocked if prevents() was true/null,
regardless of isBlockedFrom().
* If an Action can be constructed, and requiresUnblock() is
false, the action won't be blocked. Previously, requiresUnblock()
wasn't checked.

This commit was previously reverted because it exposed that
EditEntityAction::requiresUnblock in Wikibase wrongly returned
false (fixed in I99061230023da2bbd0f98190a2907ca2e9717a4c).
Other potentially similar cases were audited in T211048.

Bug: T208862
Change-Id: If74a1d422290b8c62b7a7a8922621c73c9598269

diff --git a/RELEASE-NOTES-1.33 b/RELEASE-NOTES-1.33
index 759d912..b1da6dd 100644 (file)
--- a/RELEASE-NOTES-1.33
+++ b/RELEASE-NOTES-1.33
@@ -198,6 +198,10 @@ because of Phabricator reports.
 * The PasswordPolicy 'PasswordCannotBePopular' has been deprecated. To
   follow best practices, it is reccommended to use 'PasswordNotInLargeBlacklist'
   instead which blacklists 100,000 commonly used passwords.
+* (T208862) Action::requiresUnblock() is now called from
+  Title::getUserPermissionsErrors() and Title::userCan(). Previously, the method
+  was only called in Action::checkCanExecute(). Actions should ensure that their
+  requiresUnblock() returns the proper result (the default is `true`).
 * …
 
 === Other changes in 1.33 ===

diff --git a/includes/Title.php b/includes/Title.php
index 909f528..0c1d32a 100644 (file)
--- a/includes/Title.php
+++ b/includes/Title.php
@@ -2691,14 +2691,34 @@ class Title implements LinkTarget {
                }
 
                $useReplica = ( $rigor !== 'secure' );
-               if ( ( $action == 'edit' || $action == 'create' )
-                       && !$user->isBlockedFrom( $this, $useReplica )
-               ) {
-                       // Don't block the user from editing their own talk page unless they've been
-                       // explicitly blocked from that too.
-               } elseif ( $user->isBlocked() && $user->getBlock()->prevents( $action ) !== false ) {
+               $block = $user->getBlock( $useReplica );
+
+               // The block may explicitly allow an action (like "read" or "upload").
+               if ( $block && $block->prevents( $action ) === false ) {
+                       return $errors;
+               }
+
+               // Determine if the user is blocked from this action on this page.
+               // What gets passed into this method is a user right, not an action nmae.
+               // There is no way to instantiate an action by restriction. However, this
+               // will get the action where the restriction is the same. This may result
+               // in actions being blocked that shouldn't be.
+               if ( Action::exists( $action ) ) {
                        // @todo FIXME: Pass the relevant context into this function.
-                       $errors[] = $user->getBlock()->getPermissionsError( RequestContext::getMain() );
+                       $action = Action::factory( $action, WikiPage::factory( $this ), RequestContext::getMain() );
+               } else {
+                       $action = null;
+               }
+
+               // If no action object is returned, assume that the action requires unblock
+               // which is the default.
+               if ( !$action || $action->requiresUnblock() ) {
+                       if ( $user->isBlockedFrom( $this, $useReplica ) ) {
+                               // @todo FIXME: Pass the relevant context into this function.
+                               $errors[] = $block
+                                       ? $block->getPermissionsError( RequestContext::getMain() )
+                                       : [ 'actionblockedtext' ];
+                       }
                }
 
                return $errors;

diff --git a/languages/i18n/en.json b/languages/i18n/en.json
index 3425056..bbbb9ec 100644 (file)
--- a/languages/i18n/en.json
+++ b/languages/i18n/en.json
@@ -672,6 +672,7 @@
        "blockedtext": "<strong>Your username or IP address has been blocked.</strong>\n\nThe block was made by $1.\nThe reason given is <em>$2</em>.\n\n* Start of block: $8\n* Expiration of block: $6\n* Intended blockee: $7\n\nYou can contact $1 or another [[{{MediaWiki:Grouppage-sysop}}|administrator]] to discuss the block.\nYou cannot use the \"{{int:emailuser}}\" feature unless a valid email address is specified in your [[Special:Preferences|account preferences]] and you have not been blocked from using it.\nYour current IP address is $3, and the block ID is #$5.\nPlease include all above details in any queries you make.",
        "autoblockedtext": "Your IP address has been automatically blocked because it was used by another user, who was blocked by $1.\nThe reason given is:\n\n:<em>$2</em>\n\n* Start of block: $8\n* Expiration of block: $6\n* Intended blockee: $7\n\nYou may contact $1 or one of the other [[{{MediaWiki:Grouppage-sysop}}|administrators]] to discuss the block.\n\nNote that you may not use the \"{{int:emailuser}}\" feature unless you have a valid email address registered in your [[Special:Preferences|user preferences]] and you have not been blocked from using it.\n\nYour current IP address is $3, and the block ID is #$5.\nPlease include all above details in any queries you make.",
        "systemblockedtext": "Your username or IP address has been automatically blocked by MediaWiki.\nThe reason given is:\n\n:<em>$2</em>\n\n* Start of block: $8\n* Expiration of block: $6\n* Intended blockee: $7\n\nYour current IP address is $3.\nPlease include all above details in any queries you make.",
+       "actionblockedtext": "You have been blocked from performing this action.",
        "blockednoreason": "no reason given",
        "whitelistedittext": "Please $1 to edit pages.",
        "confirmedittext": "You must confirm your email address before editing pages.\nPlease set and validate your email address through your [[Special:Preferences|user preferences]].",

diff --git a/languages/i18n/qqq.json b/languages/i18n/qqq.json
index 2349b20..693c1c6 100644 (file)
--- a/languages/i18n/qqq.json
+++ b/languages/i18n/qqq.json
@@ -875,6 +875,7 @@
        "blockedtext": "Text displayed to blocked users.\n\n\"email this user\" should be consistent with {{msg-mw|Emailuser}}.\n\nParameters:\n* $1 - the blocking sysop (with a link to his/her userpage)\n* $2 - the reason for the block\n* $3 - the current IP address of the blocked user\n* $4 - (Unused) the blocking sysop's username (plain text, without the link)\n* $5 - the unique numeric identifier of the applied autoblock\n* $6 - the expiry of the block\n* $7 - the intended target of the block (what the blocking user specified in the blocking form)\n* $8 - the timestamp when the block started\nSee also:\n* {{msg-mw|Grouppage-sysop}}\n* {{msg-mw|Autoblockedtext|notext=1}}\n* {{msg-mw|Systemblockedtext|notext=1}}",
        "autoblockedtext": "Text displayed to automatically blocked users.\n\n\"email this user\" should be consistent with {{msg-mw|Emailuser}}.\n\nParameters:\n* $1 - the blocking sysop (with a link to his/her userpage)\n* $2 - the reason for the block (in case of autoblocks: {{msg-mw|autoblocker}})\n* $3 - the current IP address of the blocked user\n* $4 - (Unused) the blocking sysop's username (plain text, without the link). Use it for GENDER.\n* $5 - the unique numeric identifier of the applied autoblock\n* $6 - the expiry of the block\n* $7 - the intended target of the block (what the blocking user specified in the blocking form)\n* $8 - the timestamp when the block started\nSee also:\n* {{msg-mw|Grouppage-sysop}}\n* {{msg-mw|Blockedtext|notext=1}}\n* {{msg-mw|Systemblockedtext|notext=1}}",
        "systemblockedtext": "Text displayed to requests blocked by MediaWiki configuration.\n\n\"email this user\" should be consistent with {{msg-mw|Emailuser}}.\n\nParameters:\n* $1 - (Unused) A dummy user attributed as the blocker, possibly as a link to a user page.\n* $2 - the reason for the block\n* $3 - the current IP address of the blocked user\n* $4 - (Unused) the dummy blocking user's username (plain text, without the link).\n* $5 - A short string indicating the type of system block.\n* $6 - the expiry of the block\n* $7 - the intended target of the block\n* $8 - the timestamp when the block started\nSee also:\n* {{msg-mw|Grouppage-sysop}}\n* {{msg-mw|Blockedtext|notext=1}}\n* {{msg-mw|Autoblockedtext|notext=1}}",
+       "actionblockedtext": "Text displayed when a user is blocked from perofmring an action, but no matching block for the user exists. This can happen if an extension forces a user to be blocked.",
        "blockednoreason": "Substituted with <code>$2</code> in the following message if the reason is not given:\n* {{msg-mw|cantcreateaccount-text}}.\n{{Identical|No reason given}}",
        "whitelistedittext": "Used as error message. Parameters:\n* $1 - a link to [[Special:UserLogin]] with {{msg-mw|loginreqlink}} as link description\n* $2 - an URL to the same\n\nSee also:\n* {{msg-mw|Nocreatetext}}\n* {{msg-mw|Uploadnologintext}}\n* {{msg-mw|Loginreqpagetext}}",
        "confirmedittext": "Used as error message.",

diff --git a/tests/phpunit/includes/TitlePermissionTest.php b/tests/phpunit/includes/TitlePermissionTest.php
index 11b9c01..cb5e1f8 100644 (file)
--- a/tests/phpunit/includes/TitlePermissionTest.php
+++ b/tests/phpunit/includes/TitlePermissionTest.php
@@ -1,5 +1,6 @@
 <?php
 
+use MediaWiki\Block\Restriction\PageRestriction;
 use MediaWiki\MediaWikiServices;
 
 /**
@@ -892,7 +893,7 @@ class TitlePermissionTest extends MediaWikiLangTestCase {
                        'wgEmailAuthentication' => true,
                ] );
 
-               $this->setUserPerm( [ "createpage", "move" ] );
+               $this->setUserPerm( [ 'createpage', 'edit', 'move', 'rollback', 'patrol', 'upload', 'purge' ] );
                $this->setTitle( NS_HELP, "test page" );
 
                # $wgEmailConfirmToEdit only applies to 'edit' action
@@ -964,11 +965,24 @@ class TitlePermissionTest extends MediaWikiLangTestCase {
                        'expiry' => 10,
                        'systemBlock' => 'test',
                ] );
-               $this->assertEquals( [ [ 'systemblockedtext',
+
+               $errors = [ [ 'systemblockedtext',
                                '[[User:Useruser|Useruser]]', 'no reason given', '127.0.0.1',
                                'Useruser', 'test', '23:00, 31 December 1969', '127.0.8.1',
-                               $wgLang->timeanddate( wfTimestamp( TS_MW, $now ), true ) ] ],
+                               $wgLang->timeanddate( wfTimestamp( TS_MW, $now ), true ) ] ];
+
+               $this->assertEquals( $errors,
+                       $this->title->getUserPermissionsErrors( 'edit', $this->user ) );
+               $this->assertEquals( $errors,
                        $this->title->getUserPermissionsErrors( 'move-target', $this->user ) );
+               $this->assertEquals( $errors,
+                       $this->title->getUserPermissionsErrors( 'rollback', $this->user ) );
+               $this->assertEquals( $errors,
+                       $this->title->getUserPermissionsErrors( 'patrol', $this->user ) );
+               $this->assertEquals( $errors,
+                       $this->title->getUserPermissionsErrors( 'upload', $this->user ) );
+               $this->assertEquals( [],
+                       $this->title->getUserPermissionsErrors( 'purge', $this->user ) );
 
                // partial block message test
                $this->user->mBlockedby = $this->user->getName();
@@ -981,10 +995,39 @@ class TitlePermissionTest extends MediaWikiLangTestCase {
                        'expiry' => 10,
                ] );
 
-               $this->assertEquals( [ [ 'blockedtext-partial',
+               $this->assertEquals( [],
+                       $this->title->getUserPermissionsErrors( 'edit', $this->user ) );
+               $this->assertEquals( [],
+                       $this->title->getUserPermissionsErrors( 'move-target', $this->user ) );
+               $this->assertEquals( [],
+                       $this->title->getUserPermissionsErrors( 'rollback', $this->user ) );
+               $this->assertEquals( [],
+                       $this->title->getUserPermissionsErrors( 'patrol', $this->user ) );
+               $this->assertEquals( [],
+                       $this->title->getUserPermissionsErrors( 'upload', $this->user ) );
+               $this->assertEquals( [],
+                       $this->title->getUserPermissionsErrors( 'purge', $this->user ) );
+
+               $this->user->mBlock->setRestrictions( [
+                               ( new PageRestriction( 0, $this->title->getArticleID() ) )->setTitle( $this->title ),
+               ] );
+
+               $errors = [ [ 'blockedtext-partial',
                                '[[User:Useruser|Useruser]]', 'no reason given', '127.0.0.1',
                                'Useruser', null, '23:00, 31 December 1969', '127.0.8.1',
-                               $wgLang->timeanddate( wfTimestamp( TS_MW, $now ), true ) ] ],
+                               $wgLang->timeanddate( wfTimestamp( TS_MW, $now ), true ) ] ];
+
+               $this->assertEquals( $errors,
+                       $this->title->getUserPermissionsErrors( 'edit', $this->user ) );
+               $this->assertEquals( $errors,
                        $this->title->getUserPermissionsErrors( 'move-target', $this->user ) );
+               $this->assertEquals( $errors,
+                       $this->title->getUserPermissionsErrors( 'rollback', $this->user ) );
+               $this->assertEquals( $errors,
+                       $this->title->getUserPermissionsErrors( 'patrol', $this->user ) );
+               $this->assertEquals( [],
+                       $this->title->getUserPermissionsErrors( 'upload', $this->user ) );
+               $this->assertEquals( [],
+                       $this->title->getUserPermissionsErrors( 'purge', $this->user ) );
        }
 }