From: Aryeh Gregor <ayg@aryeh.name>
Date: Tue, 16 Apr 2019 12:37:41 +0000 (+0300)
Subject: MovePage methods need to run safety checks
X-Git-Tag: 1.34.0-rc.0~1803^2
X-Git-Url: https://git.heureux-cyclage.org/?p=lhc%2Fweb%2Fwiklou.git;a=commitdiff_plain;h=24949e738d837a3c97619791febf36e62ee113f3

MovePage methods need to run safety checks

A move method that doesn't check for things like moving a page on top of
itself or moving to a namespace with a different content model does not
seem like it's what callers would expect, and not what Title::moveTo
ever did. If there's a caller that really wants this behavior, we can
make moveUnsafe public.

I also made the $reason and $createRedirect parameters to move()
optional to match Title::moveTo() behavior. However, I made $reason
default to null instead of '', to distinguish between an empty edit
reason provided by the user and no edit reason provided at all (e.g., a
move done internally without specific user request).

Depends-On: I971e619eb76c4474fe037fad258f9c496717bf41
Change-Id: I6ddcc9f34a48f997ae39b79cd2df40dd2cc10197
---

diff --git a/includes/MovePage.php b/includes/MovePage.php
index 24178acdad..e49398a0f7 100644
--- a/includes/MovePage.php
+++ b/includes/MovePage.php
@@ -233,14 +233,69 @@ class MovePage {
 	}
 
 	/**
+	 * Move a page without taking user permissions into account. Only checks if the move is itself
+	 * invalid, e.g., trying to move a special page or trying to move a page onto one that already
+	 * exists.
+	 *
+	 * @param User $user
+	 * @param string|null $reason
+	 * @param bool|null $createRedirect
+	 * @param string[] $changeTags Change tags to apply to the entry in the move log
+	 * @return Status
+	 */
+	public function move(
+		User $user, $reason = null, $createRedirect = true, array $changeTags = []
+	) {
+		$status = $this->isValidMove();
+		if ( !$status->isOK() ) {
+			return $status;
+		}
+
+		return $this->moveUnsafe( $user, $reason, $createRedirect, $changeTags );
+	}
+
+	/**
+	 * Same as move(), but with permissions checks.
+	 *
+	 * @param User $user
+	 * @param string|null $reason
+	 * @param bool|null $createRedirect Ignored if user doesn't have suppressredirect permission
+	 * @param string[] $changeTags Change tags to apply to the entry in the move log
+	 * @return Status
+	 */
+	public function moveIfAllowed(
+		User $user, $reason = null, $createRedirect = true, array $changeTags = []
+	) {
+		$status = $this->isValidMove();
+		$status->merge( $this->checkPermissions( $user, $reason ) );
+		if ( $changeTags ) {
+			$status->merge( ChangeTags::canAddTagsAccompanyingChange( $changeTags, $user ) );
+		}
+
+		if ( !$status->isOK() ) {
+			// Auto-block user's IP if the account was "hard" blocked
+			$user->spreadAnyEditBlock();
+			return $status;
+		}
+
+		// Check suppressredirect permission
+		if ( !$user->isAllowed( 'suppressredirect' ) ) {
+			$createRedirect = true;
+		}
+
+		return $this->moveUnsafe( $user, $reason, $createRedirect, $changeTags );
+	}
+
+	/**
+	 * Moves *without* any sort of safety or sanity checks. Hooks can still fail the move, however.
+	 *
 	 * @param User $user
 	 * @param string $reason
 	 * @param bool $createRedirect
-	 * @param string[] $changeTags Change tags to apply to the entry in the move log. Caller
-	 *  should perform permission checks with ChangeTags::canAddTagsAccompanyingChange
+	 * @param string[] $changeTags Change tags to apply to the entry in the move log
 	 * @return Status
 	 */
-	public function move( User $user, $reason, $createRedirect, array $changeTags = [] ) {
+	private function moveUnsafe( User $user, $reason, $createRedirect, array $changeTags ) {
 		global $wgCategoryCollation;
 
 		$status = Status::newGood();
diff --git a/includes/Title.php b/includes/Title.php
index 2203ccbc0b..4cac844294 100644
--- a/includes/Title.php
+++ b/includes/Title.php
@@ -3445,19 +3445,10 @@ class Title implements LinkTarget, IDBAccessObject {
 		array $changeTags = []
 	) {
 		global $wgUser;
-		$err = $this->isValidMoveOperation( $nt, $auth, $reason );
-		if ( is_array( $err ) ) {
-			// Auto-block user's IP if the account was "hard" blocked
-			$wgUser->spreadAnyEditBlock();
-			return $err;
-		}
-		// Check suppressredirect permission
-		if ( $auth && !$wgUser->isAllowed( 'suppressredirect' ) ) {
-			$createRedirect = true;
-		}
 
 		$mp = new MovePage( $this, $nt );
-		$status = $mp->move( $wgUser, $reason, $createRedirect, $changeTags );
+		$method = $auth ? 'moveIfAllowed' : 'move';
+		$status = $mp->$method( $wgUser, $reason, $createRedirect, $changeTags );
 		if ( $status->isOK() ) {
 			return true;
 		} else {
diff --git a/includes/specials/SpecialMovepage.php b/includes/specials/SpecialMovepage.php
index 8b5562f9c7..39976c0d20 100644
--- a/includes/specials/SpecialMovepage.php
+++ b/includes/specials/SpecialMovepage.php
@@ -591,21 +591,12 @@ class MovePageForm extends UnlistedSpecialPage {
 
 		# Do the actual move.
 		$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(), true );
-			return;
-		}
+		$userPermitted = $mp->checkPermissions( $user, $this->reason )->isOK();
 
-		$status = $mp->move( $user, $this->reason, $createRedirect );
+		$status = $mp->moveIfAllowed( $user, $this->reason, $createRedirect );
 		if ( !$status->isOK() ) {
-			$this->showForm( $status->getErrorsArray() );
+			$this->showForm( $status->getErrorsArray(), !$userPermitted );
 			return;
 		}