From 901dc6490ed2f2e893868afe46c617962903a52f Mon Sep 17 00:00:00 2001
From: =?utf8?q?Bartosz=20Dziewo=C5=84ski?= <matma.rex@gmail.com>
Date: Mon, 23 Jan 2017 17:32:33 +0100
Subject: [PATCH] Fix functionality and usages of
 SpecialUserrights::userCanChangeRights

SpecialUserrights::userCanChangeRights is a funky method that is
called by things outside of SpecialUserrights to see if they should
display a link to it. It had some bugs and missing documentation.

* SpecialUserrights::userCanChangeRights relied on $this->isself to
  see if the acting user and target user are the same, but it gets set
  in execute() which never runs when used like this.
* It wasn't clear whether the $user parameter represented the acting
  user or the target user, resulting in incorrect usage in
  SkinTemplate.

The net effect of these bugs is that skin sidebar would display
"Change user groups" instead of "View user groups" for all users when
the current user was only allowed to change some of their own groups.

Change-Id: Ie47b9c7463b373fe17006567239aa09e824b015d
---
 includes/skins/SkinTemplate.php         |  2 +-
 includes/specials/SpecialUserrights.php | 16 +++++++++++-----
 2 files changed, 12 insertions(+), 6 deletions(-)

diff --git a/includes/skins/SkinTemplate.php b/includes/skins/SkinTemplate.php
index 1a4554f4b0..bf260aa464 100644
--- a/includes/skins/SkinTemplate.php
+++ b/includes/skins/SkinTemplate.php
@@ -1322,7 +1322,7 @@ class SkinTemplate extends Skin {
 			if ( !$user->isAnon() ) {
 				$sur = new UserrightsPage;
 				$sur->setContext( $this->getContext() );
-				$canChange = $sur->userCanChangeRights( $this->getUser(), false );
+				$canChange = $sur->userCanChangeRights( $user );
 				$nav_urls['userrights'] = [
 					'text' => $this->msg(
 						$canChange ? 'tool-link-userrights' : 'tool-link-userrights-readonly',
diff --git a/includes/specials/SpecialUserrights.php b/includes/specials/SpecialUserrights.php
index 4db2198203..98cdc09f1a 100644
--- a/includes/specials/SpecialUserrights.php
+++ b/includes/specials/SpecialUserrights.php
@@ -49,19 +49,25 @@ class UserrightsPage extends SpecialPage {
 	}
 
 	/**
-	 * @param User $user
-	 * @param bool $checkIfSelf
+	 * Check whether the current user (from context) can change the target user's rights.
+	 *
+	 * @param User $targetUser User whose rights are being changed
+	 * @param bool $checkIfSelf If false, assume that the current user can add/remove groups defined
+	 *   in $wgGroupsAddToSelf / $wgGroupsRemoveFromSelf, without checking if it's the same as target
+	 *   user
 	 * @return bool
 	 */
-	public function userCanChangeRights( $user, $checkIfSelf = true ) {
+	public function userCanChangeRights( $targetUser, $checkIfSelf = true ) {
+		$isself = $this->getUser()->equals( $targetUser );
+
 		$available = $this->changeableGroups();
-		if ( $user->getId() == 0 ) {
+		if ( $targetUser->getId() == 0 ) {
 			return false;
 		}
 
 		return !empty( $available['add'] )
 			|| !empty( $available['remove'] )
-			|| ( ( $this->isself || !$checkIfSelf ) &&
+			|| ( ( $isself || !$checkIfSelf ) &&
 				( !empty( $available['add-self'] )
 					|| !empty( $available['remove-self'] ) ) );
 	}
-- 
2.20.1