From ac53e45035e7b7ea22e123b404608e5e699024be Mon Sep 17 00:00:00 2001 From: Kunal Mehta Date: Fri, 10 Oct 2014 22:43:02 -0700 Subject: [PATCH] Fully replace Title::moveTo() with MovePage * AbortMove hook is removed in favor of two more specificly focused hooks: MovePageCheckPermissions and MovePageIsValidMove. ** MovePageIsValidMove is for extensions to specify whether a page cannot be moved for technical reasons, and should not be overridden. ** MovePageCheckPermissions is for checking whether the given user is allowed to make the move. * Title::moveNoAuth() deprecated * Title::moveTo() deprecated * Title::isValidMoveOperation() broken down into MovePage::isValidMove() and MovePage::checkPermissions(). * Title::getTitleProtection() is now public, and returns unprefixed fields Change-Id: Ic5026384b92a0d68d628397ffe1de6e5b6183f02 --- RELEASE-NOTES-1.25 | 10 ++ docs/hooks.txt | 19 ++-- includes/MovePage.php | 99 ++++++++++++++++- includes/Title.php | 103 ++++++------------ includes/api/ApiMove.php | 38 +++++-- includes/specials/SpecialMovepage.php | 18 ++- maintenance/cleanupCaps.php | 11 +- maintenance/moveBatch.php | 8 +- .../phpunit/includes/TitlePermissionTest.php | 10 +- 9 files changed, 216 insertions(+), 100 deletions(-) diff --git a/RELEASE-NOTES-1.25 b/RELEASE-NOTES-1.25 index 28a3958543..161ba40612 100644 --- a/RELEASE-NOTES-1.25 +++ b/RELEASE-NOTES-1.25 @@ -48,6 +48,16 @@ production. SVG images in Internet Explorer 6 and 7, as they don't support them anyway. * (bug 67021) On Special:BookSources, corrected validation of ISBNs (both 10- and 13-digit forms) containing "X". +* Page moving was refactored into a MovePage class. As part of that: +** The AbortMove hook was removed. +** MovePageIsValidMove is for extensions to specify whether a page + cannot be moved for technical reasons, and should not be overriden. +** MovePageCheckPermissions is for checking whether the given user is + allowed to make the move. +** Title::moveNoAuth() was deprecated. Use the MovePage class instead. +** Title::moveTo() was deprecated. Use the MovePage class instead. +** Title::isValidMoveOperation() broken down into MovePage::isValidMove() + and MovePage::checkPermissions(). === Action API changes in 1.25 === * (bug 65403) XML tag highlighting is now only performed for formats diff --git a/docs/hooks.txt b/docs/hooks.txt index b71c347279..52eeab8356 100644 --- a/docs/hooks.txt +++ b/docs/hooks.txt @@ -260,13 +260,6 @@ $password: the password being submitted, not yet checked for validity a machine API rather than the HTML user interface. &$msg: the message identifier for abort reason (new in 1.18, not available before 1.18) -'AbortMove': Allows to abort moving an article (title). -$old: old title -$nt: new title -$user: user who is doing the move -$err: error message -$reason: the reason for the move (added in 1.13) - 'AbortNewAccount': Return false to cancel explicit account creation. $user: the User object about to be created (read-only, incomplete) &$msg: out parameter: HTML to display on abort @@ -1835,6 +1828,18 @@ $db: The database object to be queried. &$opts: Options for the query. &$join_conds: Join conditions for the query. +'MovePageCheckPermissions': Specify whether the user is allowed to move the page. +$oldTitle: Title object of the current (old) location +$newTitle: Title object of the new location +$user: User making the move +$reason: string of the reason provided by the user +$status: Status object to pass error messages to + +'MovePageIsValidMove': Specify whether a page can be moved for technical reasons. +$oldTitle: Title object of the current (old) location +$newTitle: Title object of the new location +$status: Status object to pass error messages to + 'BaseTemplateToolbox': Called by BaseTemplate when building the $toolbox array and returning it for the skin to output. You can add items to the toolbox while still letting the skin make final decisions on skin-specific markup conventions diff --git a/includes/MovePage.php b/includes/MovePage.php index 79095e9ae7..7bad3f9492 100644 --- a/includes/MovePage.php +++ b/includes/MovePage.php @@ -42,6 +42,52 @@ class MovePage { $this->newTitle = $newTitle; } + public function checkPermissions( User $user, $reason ) { + $status = new Status(); + + $errors = wfMergeErrorArrays( + $this->oldTitle->getUserPermissionsErrors( 'move', $user ), + $this->oldTitle->getUserPermissionsErrors( 'edit', $user ), + $this->newTitle->getUserPermissionsErrors( 'move-target', $user ), + $this->newTitle->getUserPermissionsErrors( 'edit', $user ) + ); + + // Convert into a Status object + if ( $errors ) { + foreach ( $errors as $error ) { + call_user_func_array( array( $status, 'fatal' ), $error ); + } + } + + if ( EditPage::matchSummarySpamRegex( $reason ) !== false ) { + // This is kind of lame, won't display nice + $status->fatal( 'spamprotectiontext' ); + } + + # The move is allowed only if (1) the target doesn't exist, or + # (2) the target is a redirect to the source, and has no history + # (so we can undo bad moves right after they're done). + + if ( $this->newTitle->getArticleID() ) { # Target exists; check for validity + if ( !$this->isValidMoveTarget() ) { + $status->fatal( 'articleexists' ); + } + } else { + $tp = $this->newTitle->getTitleProtection(); + if ( $tp !== false ) { + if ( !$user->isAllowed( $tp['permission'] ) ) { + $status->fatal( 'cantmove-titleprotected' ); + } + } + } + + wfRunHooks( 'MovePageCheckPermissions', + array( $this->oldTitle, $this->newTitle, $user, $reason, $status ) + ); + + return $status; + } + /** * Does various sanity checks that the move is * valid. Only things based on the two titles @@ -99,6 +145,9 @@ class MovePage { $status->fatal( 'nonfile-cannot-move-to-file' ); } + // Hook for extensions to say a title can't be moved for technical reasons + wfRunHooks( 'MovePageIsValidMove', array( $this->oldTitle, $this->newTitle, $status ) ); + return $status; } @@ -126,6 +175,53 @@ class MovePage { return $status; } + /** + * Checks if $this can be moved to a given Title + * - Selects for update, so don't call it unless you mean business + * + * @since 1.25 + * @return bool + */ + protected function isValidMoveTarget() { + # Is it an existing file? + if ( $this->newTitle->inNamespace( NS_FILE ) ) { + $file = wfLocalFile( $this->newTitle ); + if ( $file->exists() ) { + wfDebug( __METHOD__ . ": file exists\n" ); + return false; + } + } + # Is it a redirect with no history? + if ( !$this->newTitle->isSingleRevRedirect() ) { + wfDebug( __METHOD__ . ": not a one-rev redirect\n" ); + return false; + } + # Get the article text + $rev = Revision::newFromTitle( $this->newTitle, false, Revision::READ_LATEST ); + if ( !is_object( $rev ) ) { + return false; + } + $content = $rev->getContent(); + # Does the redirect point to the source? + # Or is it a broken self-redirect, usually caused by namespace collisions? + $redirTitle = $content ? $content->getRedirectTarget() : null; + + if ( $redirTitle ) { + if ( $redirTitle->getPrefixedDBkey() !== $this->oldTitle->getPrefixedDBkey() && + $redirTitle->getPrefixedDBkey() !== $this->newTitle->getPrefixedDBkey() ) { + wfDebug( __METHOD__ . ": redirect points to other page\n" ); + return false; + } else { + return true; + } + } else { + # Fail safe (not a redirect after all. strange.) + wfDebug( __METHOD__ . ": failsafe: database says " . $this->newTitle->getPrefixedDBkey() . + " is a redirect, but it doesn't contain a valid redirect.\n" ); + return false; + } + } + /** * @param User $user * @param string $reason @@ -135,6 +231,8 @@ class MovePage { public function move( User $user, $reason, $createRedirect ) { global $wgCategoryCollation; + wfRunHooks( 'TitleMove', array( $this->oldTitle, $this->newTitle, $user ) ); + // If it is a file, move it first. // It is done before all other moving stuff is done because it's hard to revert. $dbw = wfGetDB( DB_MASTER ); @@ -274,7 +372,6 @@ class MovePage { wfRunHooks( 'TitleMoveComplete', array( &$this->oldTitle, &$this->newTitle, &$user, $pageid, $redirid, $reason ) ); return Status::newGood(); - } /** diff --git a/includes/Title.php b/includes/Title.php index 0efc94e832..b97d36a416 100644 --- a/includes/Title.php +++ b/includes/Title.php @@ -2232,19 +2232,13 @@ class Title { } elseif ( $action == 'create' ) { $title_protection = $this->getTitleProtection(); if ( $title_protection ) { - if ( $title_protection['pt_create_perm'] == 'sysop' ) { - $title_protection['pt_create_perm'] = 'editprotected'; // B/C - } - if ( $title_protection['pt_create_perm'] == 'autoconfirmed' ) { - $title_protection['pt_create_perm'] = 'editsemiprotected'; // B/C - } - if ( $title_protection['pt_create_perm'] == '' - || !$user->isAllowed( $title_protection['pt_create_perm'] ) + if ( $title_protection['permission'] == '' + || !$user->isAllowed( $title_protection['permission'] ) ) { $errors[] = array( 'titleprotected', - User::whoIs( $title_protection['pt_user'] ), - $title_protection['pt_reason'] + User::whoIs( $title_protection['user'] ), + $title_protection['reason'] ); } } @@ -2536,7 +2530,7 @@ class Title { * @return array|bool An associative array representing any existent title * protection, or false if there's none. */ - private function getTitleProtection() { + public function getTitleProtection() { // Can't protect pages in special namespaces if ( $this->getNamespace() < 0 ) { return false; @@ -2551,13 +2545,27 @@ class Title { $dbr = wfGetDB( DB_SLAVE ); $res = $dbr->select( 'protected_titles', - array( 'pt_user', 'pt_reason', 'pt_expiry', 'pt_create_perm' ), + array( + 'user' => 'pt_user', + 'reason' => 'pt_reason', + 'expiry' => 'pt_expiry', + 'permission' => 'pt_create_perm' + ), array( 'pt_namespace' => $this->getNamespace(), 'pt_title' => $this->getDBkey() ), __METHOD__ ); // fetchRow returns false if there are no rows. - $this->mTitleProtection = $dbr->fetchRow( $res ); + $row = $dbr->fetchRow( $res ); + if ( $row ) { + if ( $row['permission'] == 'sysop' ) { + $row['permission'] = 'editprotected'; // B/C + } + if ( $row['permission'] == 'autoconfirmed' ) { + $row['permission'] = 'editsemiprotected'; // B/C + } + } + $this->mTitleProtection = $row; } return $this->mTitleProtection; } @@ -2978,12 +2986,12 @@ class Title { if ( $title_protection ) { $now = wfTimestampNow(); - $expiry = $wgContLang->formatExpiry( $title_protection['pt_expiry'], TS_MW ); + $expiry = $wgContLang->formatExpiry( $title_protection['expiry'], TS_MW ); if ( !$expiry || $expiry > $now ) { // Apply the restrictions $this->mRestrictionsExpiry['create'] = $expiry; - $this->mRestrictions['create'] = explode( ',', trim( $title_protection['pt_create_perm'] ) ); + $this->mRestrictions['create'] = explode( ',', trim( $title_protection['permission'] ) ); } else { // Get rid of the old restrictions Title::purgeExpiredRestrictions(); $this->mTitleProtection = false; @@ -3579,10 +3587,12 @@ class Title { /** * Move this page without authentication * + * @deprecated since 1.25 use MovePage class instead * @param Title $nt The new page Title * @return array|bool True on success, getUserPermissionsErrors()-like array on failure */ public function moveNoAuth( &$nt ) { + wfDeprecated( __METHOD__, '1.25' ); return $this->moveTo( $nt, false ); } @@ -3590,10 +3600,9 @@ class Title { * Check whether a given move operation would be valid. * Returns true if ok, or a getUserPermissionsErrors()-like array otherwise * - * @todo finish moving this into MovePage + * @deprecated since 1.25, use MovePage's methods instead * @param Title $nt The new title - * @param bool $auth Indicates whether $wgUser's permissions - * should be checked + * @param bool $auth Ignored * @param string $reason Is the log summary of the move, used for spam checking * @return array|bool True on success, getUserPermissionsErrors()-like array on failure */ @@ -3607,54 +3616,12 @@ class Title { } $mp = new MovePage( $this, $nt ); - $errors = $mp->isValidMove()->getErrorsArray(); - - $newid = $nt->getArticleID(); - - if ( $auth ) { - $errors = wfMergeErrorArrays( $errors, - $this->getUserPermissionsErrors( 'move', $wgUser ), - $this->getUserPermissionsErrors( 'edit', $wgUser ), - $nt->getUserPermissionsErrors( 'move-target', $wgUser ), - $nt->getUserPermissionsErrors( 'edit', $wgUser ) ); - } - - $match = EditPage::matchSummarySpamRegex( $reason ); - if ( $match !== false ) { - // This is kind of lame, won't display nice - $errors[] = array( 'spamprotectiontext' ); - } - - $err = null; - if ( !wfRunHooks( 'AbortMove', array( $this, $nt, $wgUser, &$err, $reason ) ) ) { - $errors[] = array( 'hookaborted', $err ); - } - - # The move is allowed only if (1) the target doesn't exist, or - # (2) the target is a redirect to the source, and has no history - # (so we can undo bad moves right after they're done). + $errors = wfMergeErrorArrays( + $mp->isValidMove()->getErrorsArray(), + $mp->checkPermissions( $wgUser, $reason )->getErrorsArray() + ); - if ( 0 != $newid ) { # Target exists; check for validity - if ( !$this->isValidMoveTarget( $nt ) ) { - $errors[] = array( 'articleexists' ); - } - } else { - $tp = $nt->getTitleProtection(); - $right = $tp['pt_create_perm']; - if ( $right == 'sysop' ) { - $right = 'editprotected'; // B/C - } - if ( $right == 'autoconfirmed' ) { - $right = 'editsemiprotected'; // B/C - } - if ( $tp && !$wgUser->isAllowed( $right ) ) { - $errors[] = array( 'cantmove-titleprotected' ); - } - } - if ( empty( $errors ) ) { - return true; - } - return $errors; + return $errors ? : true; } /** @@ -3679,7 +3646,7 @@ class Title { /** * Move a title to a new location * - * @todo Deprecate this in favor of MovePage + * @deprecated since 1.25, use the MovePage class instead * @param Title $nt The new title * @param bool $auth Indicates whether $wgUser's permissions * should be checked @@ -3701,8 +3668,6 @@ class Title { $createRedirect = true; } - wfRunHooks( 'TitleMove', array( $this, $nt, $wgUser ) ); - $mp = new MovePage( $this, $nt ); $status = $mp->move( $wgUser, $reason, $createRedirect ); if ( $status->isOK() ) { @@ -3838,7 +3803,7 @@ class Title { * Checks if $this can be moved to a given Title * - Selects for update, so don't call it unless you mean business * - * @todo move to MovePage + * @deprecated since 1.25, use MovePage's methods instead * @param Title $nt The new title to check * @return bool */ diff --git a/includes/api/ApiMove.php b/includes/api/ApiMove.php index db0fde3486..1aa0d1155a 100644 --- a/includes/api/ApiMove.php +++ b/includes/api/ApiMove.php @@ -72,9 +72,9 @@ class ApiMove extends ApiBase { // Move the page $toTitleExists = $toTitle->exists(); - $retval = $fromTitle->moveTo( $toTitle, true, $params['reason'], !$params['noredirect'] ); - if ( $retval !== true ) { - $this->dieUsageMsg( reset( $retval ) ); + $status = $this->movePage( $fromTitle, $toTitle, $params['reason'], !$params['noredirect'] ); + if ( !$status->isOK() ) { + $this->dieStatus( $status ); } $r = array( @@ -99,8 +99,8 @@ class ApiMove extends ApiBase { // Move the talk page if ( $params['movetalk'] && $fromTalk->exists() && !$fromTitle->isTalkPage() ) { $toTalkExists = $toTalk->exists(); - $retval = $fromTalk->moveTo( $toTalk, true, $params['reason'], !$params['noredirect'] ); - if ( $retval === true ) { + $status = $this->movePage( $fromTalk, $toTalk, $params['reason'], !$params['noredirect'] ); + if ( $status->isOK() ) { $r['talkfrom'] = $fromTalk->getPrefixedText(); $r['talkto'] = $toTalk->getPrefixedText(); if ( $toTalkExists ) { @@ -108,9 +108,9 @@ class ApiMove extends ApiBase { } } else { // We're not gonna dieUsage() on failure, since we already changed something - $parsed = $this->parseMsg( reset( $retval ) ); - $r['talkmove-error-code'] = $parsed['code']; - $r['talkmove-error-info'] = $parsed['info']; + $error = $this->getErrorFromStatus( $status ); + $r['talkmove-error-code'] = $error[0]; + $r['talkmove-error-info'] = $error[1]; } } @@ -147,6 +147,28 @@ class ApiMove extends ApiBase { $result->addValue( null, $this->getModuleName(), $r ); } + /** + * @param Title $from + * @param Title $to + * @param string $reason + * @param bool $createRedirect + * @return Status + */ + protected function movePage( Title $from, Title $to, $reason, $createRedirect ) { + $mp = new MovePage( $from, $to ); + $valid = $mp->isValidMove(); + if ( !$valid->isOK() ) { + return $valid; + } + + $permStatus = $mp->checkPermissions( $this->getUser(), $reason ); + if ( !$permStatus->isOK() ) { + return $permStatus; + } + + return $mp->move( $this->getUser(), $reason, $createRedirect ); + } + /** * @param Title $fromTitle * @param Title $toTitle diff --git a/includes/specials/SpecialMovepage.php b/includes/specials/SpecialMovepage.php index ec9593f77f..4cbf584a35 100644 --- a/includes/specials/SpecialMovepage.php +++ b/includes/specials/SpecialMovepage.php @@ -549,10 +549,22 @@ class MovePageForm extends UnlistedSpecialPage { } # Do the actual move. - $error = $ot->moveTo( $nt, true, $this->reason, $createRedirect ); - if ( $error !== true ) { - $this->showForm( $error ); + $mp = new MovePage( $ot, $nt ); + $valid = $mp->isValidMove(); + if ( !$valid->isOK() ) { + $this->showForm( $valid->getErrorsArray() ); + return; + } + + $permStatus = $mp->checkPermissions( $user, $this->reason ); + if ( !$permStatus->isOK() ) { + $this->showForm( $permStatus->getErrorsArray() ); + return; + } + $status = $mp->move( $user, $this->reason, $createRedirect ); + if ( !$status->isOK() ) { + $this->showForm( $status->getErrorsArray() ); return; } diff --git a/maintenance/cleanupCaps.php b/maintenance/cleanupCaps.php index 9e88c13593..6234db48ca 100644 --- a/maintenance/cleanupCaps.php +++ b/maintenance/cleanupCaps.php @@ -37,6 +37,9 @@ require_once __DIR__ . '/cleanupTable.inc'; * @ingroup Maintenance */ class CapsCleanup extends TableCleanup { + + private $user; + public function __construct() { parent::__construct(); $this->mDescription = "Script to cleanup capitalization"; @@ -44,13 +47,13 @@ class CapsCleanup extends TableCleanup { } public function execute() { - global $wgCapitalLinks, $wgUser; + global $wgCapitalLinks; if ( $wgCapitalLinks ) { $this->error( "\$wgCapitalLinks is on -- no need for caps links cleanup.", true ); } - $wgUser = User::newFromName( 'Conversion script' ); + $this->user = User::newFromName( 'Conversion script' ); $this->namespace = intval( $this->getOption( 'namespace', 0 ) ); $this->dryrun = $this->hasOption( 'dry-run' ); @@ -87,7 +90,9 @@ class CapsCleanup extends TableCleanup { $this->output( "\"$display\" -> \"$targetDisplay\": DRY RUN, NOT MOVED\n" ); $ok = true; } else { - $ok = $current->moveTo( $target, false, 'Converting page titles to lowercase' ); + $mp = new MovePage( $current, $target ); + $status = $mp->move( $this->user, 'Converting page titles to lowercase', true ); + $ok = $status->isOK() ? 'OK' : $status->getWikiText(); $this->output( "\"$display\" -> \"$targetDisplay\": $ok\n" ); } if ( $ok === true ) { diff --git a/maintenance/moveBatch.php b/maintenance/moveBatch.php index 713753f261..a27a77262c 100644 --- a/maintenance/moveBatch.php +++ b/maintenance/moveBatch.php @@ -103,10 +103,10 @@ class MoveBatch extends Maintenance { $this->output( $source->getPrefixedText() . ' --> ' . $dest->getPrefixedText() ); $dbw->begin( __METHOD__ ); - $err = $source->moveTo( $dest, false, $reason, !$noredirects ); - if ( $err !== true ) { - $msg = array_shift( $err[0] ); - $this->output( "\nFAILED: " . wfMessage( $msg, $err[0] )->text() ); + $mp = new MovePage( $source, $dest ); + $status = $mp->move( $wgUser, $reason, !$noredirects ); + if ( !$status->isOK() ) { + $this->output( "\nFAILED: " . $status->getWikiText() ); } $dbw->commit( __METHOD__ ); $this->output( "\n" ); diff --git a/tests/phpunit/includes/TitlePermissionTest.php b/tests/phpunit/includes/TitlePermissionTest.php index 49c0108b50..6af186206c 100644 --- a/tests/phpunit/includes/TitlePermissionTest.php +++ b/tests/phpunit/includes/TitlePermissionTest.php @@ -650,10 +650,10 @@ class TitlePermissionTest extends MediaWikiLangTestCase { public function testActionPermissions() { $this->setUserPerm( array( "createpage" ) ); $this->setTitle( NS_MAIN, "test page" ); - $this->title->mTitleProtection['pt_create_perm'] = ''; - $this->title->mTitleProtection['pt_user'] = $this->user->getID(); - $this->title->mTitleProtection['pt_expiry'] = wfGetDB( DB_SLAVE )->getInfinity(); - $this->title->mTitleProtection['pt_reason'] = 'test'; + $this->title->mTitleProtection['permission'] = ''; + $this->title->mTitleProtection['user'] = $this->user->getID(); + $this->title->mTitleProtection['expiry'] = wfGetDB( DB_SLAVE )->getInfinity(); + $this->title->mTitleProtection['reason'] = 'test'; $this->title->mCascadeRestriction = false; $this->assertEquals( array( array( 'titleprotected', 'Useruser', 'test' ) ), @@ -661,7 +661,7 @@ class TitlePermissionTest extends MediaWikiLangTestCase { $this->assertEquals( false, $this->title->userCan( 'create', $this->user ) ); - $this->title->mTitleProtection['pt_create_perm'] = 'sysop'; + $this->title->mTitleProtection['permission'] = 'editprotected'; $this->setUserPerm( array( 'createpage', 'protect' ) ); $this->assertEquals( array( array( 'titleprotected', 'Useruser', 'test' ) ), $this->title->getUserPermissionsErrors( 'create', $this->user ) ); -- 2.20.1