Prevent use of expiries to circumvent restrictions on removing user groups
authorThis, that and the other <at.light@live.com.au>
Wed, 1 Feb 2017 03:22:37 +0000 (14:22 +1100)
committerThis, that and the other <at.light@live.com.au>
Wed, 1 Feb 2017 03:22:37 +0000 (14:22 +1100)
I hadn't thought through what happens if a user has permission to add but
not remove a user group, or vice versa. This cleans up the UI logic,
showing controls that are available to users and vice versa, and the data
validation as well.

In particular, if user B can add users to the 'sysop' group but not remove
them from it, and user X is a sysop expiring in 1 year, user B should not
be allowed to modify the expiry to 1 second (which has the same effect as
removing the group). With this patch, user B can only extend user X's
sysop rights, perhaps to renew their temporary adminship for another year;
they can no longer bring forward the expiry date.

I'm omitting this check from the API on purpose. The API's validation
of the expiry dates seems to be there solely to reject bogus/invalid data.
Notably, the API doesn't throw an error when the user passes a group that
they can't add or remove.

Also added a # in the UI to show groups whose expiry cannot be brought
forward.

Bug: T156784
Change-Id: I0c0dadc2035c0cdf19accd5a97f08e33151a08ba

includes/specials/SpecialUserrights.php
languages/i18n/en.json
languages/i18n/qqq.json

index b0808f6..454d1e3 100644 (file)
@@ -257,6 +257,7 @@ class UserrightsPage extends SpecialPage {
                $addgroup = [];
                $groupExpiries = []; // associative array of (group name => expiry)
                $removegroup = [];
+               $existingUGMs = $user->getGroupMemberships();
 
                // This could possibly create a highly unlikely race condition if permissions are changed between
                //  when the form is loaded and when the form is saved. Ignoring it for the moment.
@@ -269,12 +270,14 @@ class UserrightsPage extends SpecialPage {
                                if ( $this->canProcessExpiries() ) {
                                        // read the expiry information from the request
                                        $expiryDropdown = $this->getRequest()->getVal( "wpExpiry-$group" );
+                                       if ( $expiryDropdown === 'existing' ) {
+                                               continue;
+                                       }
+
                                        if ( $expiryDropdown === 'other' ) {
                                                $expiryValue = $this->getRequest()->getVal( "wpExpiry-$group-other" );
-                                       } elseif ( $expiryDropdown !== 'existing' ) {
-                                               $expiryValue = $expiryDropdown;
                                        } else {
-                                               continue;
+                                               $expiryValue = $expiryDropdown;
                                        }
 
                                        // validate the expiry
@@ -288,6 +291,16 @@ class UserrightsPage extends SpecialPage {
                                        if ( $groupExpiries[$group] && $groupExpiries[$group] < wfTimestampNow() ) {
                                                return Status::newFatal( 'userrights-expiry-in-past', $group );
                                        }
+
+                                       // if the user can only add this group (not remove it), the expiry time
+                                       // cannot be brought forward (T156784)
+                                       if ( !$this->canRemove( $group ) &&
+                                               isset( $existingUGMs[$group] ) &&
+                                               ( $existingUGMs[$group]->getExpiry() ?: 'infinity' ) >
+                                                       ( $groupExpiries[$group] ?: 'infinity' )
+                                       ) {
+                                               return Status::newFatal( 'userrights-cannot-shorten-expiry', $group );
+                                       }
                                }
                        } else {
                                $removegroup[] = $group;
@@ -300,7 +313,8 @@ class UserrightsPage extends SpecialPage {
        }
 
        /**
-        * Save user groups changes in the database.
+        * Save user groups changes in the database. This function does not throw errors;
+        * instead, it ignores groups that the performer does not have permission to set.
         *
         * @param User|UserRightsProxy $user
         * @param array $add Array of groups to add
@@ -317,6 +331,7 @@ class UserrightsPage extends SpecialPage {
                // Validate input set...
                $isself = $user->getName() == $this->getUser()->getName();
                $groups = $user->getGroups();
+               $ugms = $user->getGroupMemberships();
                $changeable = $this->changeableGroups();
                $addable = array_merge( $changeable['add'], $isself ? $changeable['add-self'] : [] );
                $removable = array_merge( $changeable['remove'], $isself ? $changeable['remove-self'] : [] );
@@ -325,9 +340,19 @@ class UserrightsPage extends SpecialPage {
                        array_intersect( (array)$remove, $removable, $groups ) );
                $add = array_intersect( (array)$add, $addable );
 
-               // add only groups that are not already present or that need their expiry updated
+               // add only groups that are not already present or that need their expiry updated,
+               // UNLESS the user can only add this group (not remove it) and the expiry time
+               // is being brought forward (T156784)
                $add = array_filter( $add,
-                       function( $group ) use ( $groups, $groupExpiries ) {
+                       function( $group ) use ( $groups, $groupExpiries, $removable, $ugms ) {
+                               if ( isset( $groupExpiries[$group] ) &&
+                                       !in_array( $group, $removable ) &&
+                                       isset( $ugms[$group] ) &&
+                                       ( $ugms[$group]->getExpiry() ?: 'infinity' ) >
+                                               ( $groupExpiries[$group] ?: 'infinity' )
+                               ) {
+                                       return false;
+                               }
                                return !in_array( $group, $groups ) || array_key_exists( $group, $groupExpiries );
                        } );
 
@@ -757,22 +782,30 @@ class UserrightsPage extends SpecialPage {
 
                foreach ( $allgroups as $group ) {
                        $set = isset( $usergroups[$group] );
+                       // Users who can add the group, but not remove it, can only lengthen
+                       // expiries, not shorten them. So they should only see the expiry
+                       // dropdown if the group currently has a finite expiry
+                       $canOnlyLengthenExpiry = ( $set && $this->canAdd( $group ) &&
+                                !$this->canRemove( $group ) && $usergroups[$group]->getExpiry() );
                        // Should the checkbox be disabled?
-                       $disabled = !(
+                       $disabledCheckbox = !(
                                ( $set && $this->canRemove( $group ) ) ||
                                ( !$set && $this->canAdd( $group ) ) );
+                       // Should the expiry elements be disabled?
+                       $disabledExpiry = $disabledCheckbox && !$canOnlyLengthenExpiry;
                        // Do we need to point out that this action is irreversible?
-                       $irreversible = !$disabled && (
+                       $irreversible = !$disabledCheckbox && (
                                ( $set && !$this->canAdd( $group ) ) ||
                                ( !$set && !$this->canRemove( $group ) ) );
 
                        $checkbox = [
                                'set' => $set,
-                               'disabled' => $disabled,
+                               'disabled' => $disabledCheckbox,
+                               'disabled-expiry' => $disabledExpiry,
                                'irreversible' => $irreversible
                        ];
 
-                       if ( $disabled ) {
+                       if ( $disabledCheckbox && $disabledExpiry ) {
                                $columns['unchangeable'][$group] = $checkbox;
                        } else {
                                $columns['changeable'][$group] = $checkbox;
@@ -806,12 +839,14 @@ class UserrightsPage extends SpecialPage {
                                $member = UserGroupMembership::getGroupMemberName( $group, $user->getName() );
                                if ( $checkbox['irreversible'] ) {
                                        $text = $this->msg( 'userrights-irreversible-marker', $member )->text();
+                               } elseif ( $checkbox['disabled'] && !$checkbox['disabled-expiry'] ) {
+                                       $text = $this->msg( 'userrights-no-shorten-expiry-marker', $member )->text();
                                } else {
                                        $text = $member;
                                }
                                $checkboxHtml = Xml::checkLabel( $text, "wpGroup-" . $group,
                                        "wpGroup-" . $group, $checkbox['set'], $attr );
-                               $ret .= "\t\t" . ( $checkbox['disabled']
+                               $ret .= "\t\t" . ( ( $checkbox['disabled'] && $checkbox['disabled-expiry'] )
                                        ? Xml::tags( 'div', [ 'class' => 'mw-userrights-disabled' ], $checkboxHtml )
                                        : Xml::tags( 'div', [], $checkboxHtml )
                                ) . "\n";
@@ -824,9 +859,11 @@ class UserrightsPage extends SpecialPage {
                                                $usergroups[$group]->getExpiry() :
                                                null;
 
-                                       // If the user can't uncheck this checkbox, print the current expiry below
+                                       // If the user can't modify the expiry, print the current expiry below
                                        // it in plain text. Otherwise provide UI to set/change the expiry
-                                       if ( $checkbox['set'] && ( $checkbox['irreversible'] || $checkbox['disabled'] ) ) {
+                                       if ( $checkbox['set'] &&
+                                               ( $checkbox['irreversible'] || $checkbox['disabled-expiry'] )
+                                       ) {
                                                if ( $currentExpiry ) {
                                                        $expiryFormatted = $uiLanguage->userTimeAndDate( $currentExpiry, $uiUser );
                                                        $expiryFormattedD = $uiLanguage->userDate( $currentExpiry, $uiUser );
@@ -848,7 +885,7 @@ class UserrightsPage extends SpecialPage {
                                                        "mw-input-wpExpiry-$group", // forward compatibility with HTMLForm
                                                        $currentExpiry ? 'existing' : 'infinite'
                                                );
-                                               if ( $checkbox['disabled'] ) {
+                                               if ( $checkbox['disabled-expiry'] ) {
                                                        $expiryFormOptions->setAttribute( 'disabled', 'disabled' );
                                                }
 
@@ -883,11 +920,17 @@ class UserrightsPage extends SpecialPage {
 
                                                // Add custom expiry field
                                                $attribs = [ 'id' => "mw-input-wpExpiry-$group-other" ];
-                                               if ( $checkbox['disabled'] ) {
+                                               if ( $checkbox['disabled-expiry'] ) {
                                                        $attribs['disabled'] = 'disabled';
                                                }
                                                $expiryHtml .= Xml::input( "wpExpiry-$group-other", 30, '', $attribs );
 
+                                               // If the user group is set but the checkbox is disabled, mimic a
+                                               // checked checkbox in the form submission
+                                               if ( $checkbox['set'] && $checkbox['disabled'] ) {
+                                                       $expiryHtml .= Html::hidden( "wpGroup-$group", 1 );
+                                               }
+
                                                $expiryHtml .= Xml::closeElement( 'span' );
                                        }
 
index 81d9af1..01f8029 100644 (file)
        "userrights-groupsmember": "Member of:",
        "userrights-groupsmember-auto": "Implicit member of:",
        "userrights-groupsmember-type": "$1",
-       "userrights-groups-help": "You may alter the groups this user is in:\n* A checked box means the user is in that group.\n* An unchecked box means the user is not in that group.\n* A * indicates that you cannot remove the group once you have added it, or vice versa.",
+       "userrights-groups-help": "You may alter the groups this user is in:\n* A checked box means the user is in that group.\n* An unchecked box means the user is not in that group.\n* A * indicates that you cannot remove the group once you have added it, or vice versa.\n* A # indicates that you can only put back the expiration time of this group; you cannot bring it forward.",
        "userrights-reason": "Reason:",
        "userrights-no-interwiki": "You do not have permission to edit user rights on other wikis.",
        "userrights-nodatabase": "Database $1 does not exist or is not local.",
        "userrights-changeable-col": "Groups you can change",
        "userrights-unchangeable-col": "Groups you cannot change",
        "userrights-irreversible-marker": "$1*",
+       "userrights-no-shorten-expiry-marker": "$1#",
        "userrights-expiry-current": "Expires $1",
        "userrights-expiry-none": "Does not expire",
        "userrights-expiry": "Expires:",
        "userrights-expiry-options": "1 day:1 day,1 week:1 week,1 month:1 month,3 months:3 months,6 months:6 months,1 year:1 year",
        "userrights-invalid-expiry": "The expiry time for group \"$1\" is invalid.",
        "userrights-expiry-in-past": "The expiry time for group \"$1\" is in the past.",
+       "userrights-cannot-shorten-expiry": "You cannot bring forward the expiry of group \"$1\". Only users with permission to add and remove this group can bring forward expiry times.",
        "userrights-conflict": "Conflict of user rights changes! Please review and confirm your changes.",
        "group": "Group:",
        "group-user": "Users",
index b141ffc..3c7a87c 100644 (file)
        "userrights-changeable-col": "Used when editing user groups in [[Special:Userrights]].\n\nThe message is the head of a column of group assignments.\n\nParameters:\n* $1 - (Optional) for PLURAL use, the number of items in the column following the message. Avoid PLURAL, if your language can do without.",
        "userrights-unchangeable-col": "Used when editing user groups in [[Special:Userrights]]. The message is the head of a column of group assignments.\n\nParameters:\n* $1 - (Optional) for PLURAL use, the number of items in the column following the message. Avoid PLURAL, if your language allows that.",
        "userrights-irreversible-marker": "{{optional}}\nParameters:\n* $1 - group member",
+       "userrights-no-shorten-expiry-marker": "{{optional}}\nParameters:\n* $1 - group member",
        "userrights-expiry-current": "Indicates when a user's membership of a user group expires.\n\nParameters:\n* $1 - time and date of expiry\n* $2 - date of expiry\n* $3 - time of expiry\n{{Identical|Expire}}",
        "userrights-expiry-none": "Indicates that a user's membership of a user group lasts indefinitely, and does not expire.",
        "userrights-expiry": "Used as a label for a form element which can be used to select an expiry date/time.\n{{Identical|Expire}}",
        "userrights-expiry-options": "{{doc-important|Be careful: '''1 translation:1 english''', so the first part is the translation and the second part should stay in English.}}\nOptions for the duration of the user group membership. Example: See e.g. [[MediaWiki:Userrights-expiry-options/nl]] if you still don't know how to do it.\n\nSee also {{msg-mw|protect-expiry-options}}.",
        "userrights-invalid-expiry": "Error message on [[Special:UserRights]].\n\nParameters:\n* $1 - group name",
        "userrights-expiry-in-past": "Error message on [[Special:UserRights]] when the user types an expiry date that has already passed.\n\nParameters:\n* $1 - group name",
+       "userrights-cannot-shorten-expiry": "Error message on [[Special:UserRights]] when the user tries to move the expiry date to be closer to the present and they do not have permission to do so. \"Bring forward\" is a phrasal verb meaning \"move to an earlier time\".\n\nParameters:\n* $1 - group name",
        "userrights-conflict": "Shown on [[Special:UserRights]] if the target's rights have been changed since the form was loaded.",
        "group": "{{Identical|Group}}",
        "group-user": "{{doc-group|user}}\n{{Identical|User}}",