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

diff --git a/includes/skins/SkinTemplate.php b/includes/skins/SkinTemplate.php
index 1a4554f..bf260aa 100644 (file)
--- 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 4db2198..98cdc09 100644 (file)
--- 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'] ) ) );
        }