[ss-2018-001] Restrict non-admins from being assigned to admin groups

src/Security/Member.php

@@ -33,6 +33,7 @@ use SilverStripe\ORM\HasManyList;
 use SilverStripe\ORM\ManyManyList;
 use SilverStripe\ORM\Map;
 use SilverStripe\ORM\SS_List;
+use SilverStripe\ORM\UnsavedRelationList;
 use SilverStripe\ORM\ValidationException;
 use SilverStripe\ORM\ValidationResult;
 
@@ -60,7 +61,6 @@ use SilverStripe\ORM\ValidationResult;
  */
 class Member extends DataObject
 {
-
     private static $db = array(
         'FirstName' => 'Varchar',
         'Surname' => 'Varchar',
@@ -521,11 +521,10 @@ class Member extends DataObject
             'This method is deprecated and now does not add value. Please use Security::getCurrentUser()'
         );
 
-        if ($member = Security::getCurrentUser()) {
+        $member = Security::getCurrentUser();
         if ($member && $member->exists()) {
             return true;
         }
-        }
 
         return false;
     }
@@ -799,6 +798,7 @@ class Member extends DataObject
      * @param Member|null|int $member Member or member ID to log in as.
      * Set to null or 0 to act as a logged out user.
      * @param callable $callback
+     * @return mixed Result of $callback
      */
     public static function actAs($member, $callback)
     {
@@ -831,11 +831,11 @@ class Member extends DataObject
             'This method is deprecated. Please use Security::getCurrentUser() or an IdentityStore'
         );
 
-        if ($member = Security::getCurrentUser()) {
+        $member = Security::getCurrentUser();
+        if ($member) {
             return $member->ID;
-        } else {
-            return 0;
         }
+        return 0;
     }
 
     /**
@@ -912,7 +912,11 @@ class Member extends DataObject
                 ->setHTMLTemplate('SilverStripe\\Control\\Email\\ChangePasswordEmail')
                 ->setData($this)
                 ->setTo($this->Email)
-                ->setSubject(_t(__CLASS__ . '.SUBJECTPASSWORDCHANGED', "Your password has been changed", 'Email subject'))
+                ->setSubject(_t(
+                    __CLASS__ . '.SUBJECTPASSWORDCHANGED',
+                    "Your password has been changed",
+                    'Email subject'
+                ))
                 ->send();
         }
 
@@ -974,16 +978,25 @@ class Member extends DataObject
      */
     public function onChangeGroups($ids)
     {
-        // unless the current user is an admin already OR the logged in user is an admin
+        // Ensure none of these match disallowed list
-        if (Permission::check('ADMIN') || Permission::checkMember($this, 'ADMIN')) {
+        $disallowedGroupIDs = $this->disallowedGroups();
-            return true;
+        return count(array_intersect($ids, $disallowedGroupIDs)) == 0;
     }
 
-        // If there are no admin groups in this set then it's ok
+    /**
-        $adminGroups = Permission::get_groups_by_permission('ADMIN');
+     * List of group IDs this user is disallowed from
-        $adminGroupIDs = ($adminGroups) ? $adminGroups->column('ID') : array();
+     *
+     * @return int[] List of group IDs
+     */
+    protected function disallowedGroups()
+    {
+        // unless the current user is an admin already OR the logged in user is an admin
+        if (Permission::check('ADMIN') || Permission::checkMember($this, 'ADMIN')) {
+            return [];
+        }
 
-        return count(array_intersect($ids, $adminGroupIDs)) == 0;
+        // Non-admins may not belong to admin groups
+        return Permission::get_groups_by_permission('ADMIN')->column('ID');
     }
 
 
@@ -1287,7 +1300,7 @@ class Member extends DataObject
     }
 
     /**
-     * @return ManyManyList
+     * @return ManyManyList|UnsavedRelationList
      */
     public function DirectGroups()
     {
@@ -1468,8 +1481,14 @@ class Member extends DataObject
             $fields->removeByName('RememberLoginHashes');
 
             if (Permission::check('EDIT_PERMISSIONS')) {
+                // Filter allowed groups
+                $groups = Group::get();
+                $disallowedGroupIDs = $this->disallowedGroups();
+                if ($disallowedGroupIDs) {
+                    $groups = $groups->exclude('ID', $disallowedGroupIDs);
+                }
                 $groupsMap = array();
-                foreach (Group::get() as $group) {
+                foreach ($groups as $group) {
                     // Listboxfield values are escaped, use ASCII char instead of »
                     $groupsMap[$group->ID] = $group->getBreadcrumbs(' > ');
                 }
@@ -1524,7 +1543,11 @@ class Member extends DataObject
         /** @skipUpgrade */
         $labels['Email'] = _t(__CLASS__ . '.EMAIL', 'Email');
         $labels['Password'] = _t(__CLASS__ . '.db_Password', 'Password');
-        $labels['PasswordExpiry'] = _t(__CLASS__ . '.db_PasswordExpiry', 'Password Expiry Date', 'Password expiry date');
+        $labels['PasswordExpiry'] = _t(
+            __CLASS__ . '.db_PasswordExpiry',
+            'Password Expiry Date',
+            'Password expiry date'
+        );
         $labels['LockedOutUntil'] = _t(__CLASS__ . '.db_LockedOutUntil', 'Locked out until', 'Security related date');
         $labels['Locale'] = _t(__CLASS__ . '.db_Locale', 'Interface Locale');
         if ($includerelations) {

tests/php/Security/MemberTest.php

@@ -7,6 +7,7 @@ use SilverStripe\Core\Config\Config;
 use SilverStripe\Core\Convert;
 use SilverStripe\Core\Injector\Injector;
 use SilverStripe\Dev\FunctionalTest;
+use SilverStripe\Forms\ListboxField;
 use SilverStripe\i18n\i18n;
 use SilverStripe\ORM\DataObject;
 use SilverStripe\ORM\DB;
@@ -20,7 +21,6 @@ use SilverStripe\Security\Member_Validator;
 use SilverStripe\Security\MemberAuthenticator\MemberAuthenticator;
 use SilverStripe\Security\MemberAuthenticator\SessionAuthenticationHandler;
 use SilverStripe\Security\MemberPassword;
-use SilverStripe\Security\PasswordEncryptor_Blowfish;
 use SilverStripe\Security\Permission;
 use SilverStripe\Security\RememberLoginHash;
 use SilverStripe\Security\Security;
@@ -656,6 +656,8 @@ class MemberTest extends FunctionalTest
         $staffMember = $this->objFromFixture(Member::class, 'staffmember');
         /** @var Member $adminMember */
         $adminMember = $this->objFromFixture(Member::class, 'admin');
+
+        // Construct admin and non-admin gruops
         $newAdminGroup = new Group(array('Title' => 'newadmin'));
         $newAdminGroup->write();
         Permission::grant($newAdminGroup->ID, 'ADMIN');
@@ -688,6 +690,37 @@ class MemberTest extends FunctionalTest
         );
     }
 
+    /**
+     * Ensure DirectGroups listbox disallows admin-promotion
+     */
+    public function testAllowedGroupsListbox()
+    {
+        /** @var Group $adminGroup */
+        $adminGroup = $this->objFromFixture(Group::class, 'admingroup');
+        /** @var Member $staffMember */
+        $staffMember = $this->objFromFixture(Member::class, 'staffmember');
+        /** @var Member $adminMember */
+        $adminMember = $this->objFromFixture(Member::class, 'admin');
+
+        // Ensure you can see the DirectGroups box
+        $this->logInWithPermission('EDIT_PERMISSIONS');
+
+        // Non-admin member field contains non-admin groups
+        /** @var ListboxField $staffListbox */
+        $staffListbox = $staffMember->getCMSFields()->dataFieldByName('DirectGroups');
+        $this->assertArrayNotHasKey($adminGroup->ID, $staffListbox->getSource());
+
+        // admin member field contains admin group
+        /** @var ListboxField $adminListbox */
+        $adminListbox = $adminMember->getCMSFields()->dataFieldByName('DirectGroups');
+        $this->assertArrayHasKey($adminGroup->ID, $adminListbox->getSource());
+
+        // If logged in as admin, staff listbox has admin group
+        $this->logInWithPermission('ADMIN');
+        $staffListbox = $staffMember->getCMSFields()->dataFieldByName('DirectGroups');
+        $this->assertArrayHasKey($adminGroup->ID, $staffListbox->getSource());
+    }
+
     /**
      * Test Member_GroupSet::add
      */
@@ -866,8 +899,6 @@ class MemberTest extends FunctionalTest
 
     public function testValidateAutoLoginToken()
     {
-        $enc = new PasswordEncryptor_Blowfish();
-
         $m1 = new Member();
         $m1->PasswordEncryption = 'blowfish';
         $m1->Salt = $enc->salt('123');
@@ -1463,6 +1494,7 @@ class MemberTest extends FunctionalTest
     public function testChangePasswordWithExtensionsThatModifyValidationResult()
     {
         // Default behaviour
+        /** @var Member $member */
         $member = $this->objFromFixture(Member::class, 'admin');
         $result = $member->changePassword('my-secret-new-password');
         $this->assertInstanceOf(ValidationResult::class, $result);