Merge pull request #62 from silverstripe-security/pulls/4.0/ss-2018-001

[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,27 +61,26 @@ use SilverStripe\ORM\ValidationResult;
  */
 class Member extends DataObject
 {
-
     private static $db = array(
-        'FirstName'          => 'Varchar',
+        'FirstName' => 'Varchar',
-        'Surname'            => 'Varchar',
+        'Surname' => 'Varchar',
-        'Email'              => 'Varchar(254)', // See RFC 5321, Section 4.5.3.1.3. (256 minus the < and > character)
+        'Email' => 'Varchar(254)', // See RFC 5321, Section 4.5.3.1.3. (256 minus the < and > character)
-        'TempIDHash'         => 'Varchar(160)', // Temporary id used for cms re-authentication
+        'TempIDHash' => 'Varchar(160)', // Temporary id used for cms re-authentication
-        'TempIDExpired'      => 'Datetime', // Expiry of temp login
+        'TempIDExpired' => 'Datetime', // Expiry of temp login
-        'Password'           => 'Varchar(160)',
+        'Password' => 'Varchar(160)',
-        'AutoLoginHash'      => 'Varchar(160)', // Used to auto-login the user on password reset
+        'AutoLoginHash' => 'Varchar(160)', // Used to auto-login the user on password reset
-        'AutoLoginExpired'   => 'Datetime',
+        'AutoLoginExpired' => 'Datetime',
         // This is an arbitrary code pointing to a PasswordEncryptor instance,
         // not an actual encryption algorithm.
         // Warning: Never change this field after its the first password hashing without
         // providing a new cleartext password as well.
         'PasswordEncryption' => "Varchar(50)",
-        'Salt'               => 'Varchar(50)',
+        'Salt' => 'Varchar(50)',
-        'PasswordExpiry'     => 'Date',
+        'PasswordExpiry' => 'Date',
-        'LockedOutUntil'     => 'Datetime',
+        'LockedOutUntil' => 'Datetime',
-        'Locale'             => 'Varchar(6)',
+        'Locale' => 'Varchar(6)',
         // handled in registerFailedLogin(), only used if $lock_out_after_incorrect_logins is set
-        'FailedLoginCount'   => 'Int',
+        'FailedLoginCount' => 'Int',
     );
 
     private static $belongs_many_many = array(
@@ -88,7 +88,7 @@ class Member extends DataObject
     );
 
     private static $has_many = array(
-        'LoggedPasswords'     => MemberPassword::class,
+        'LoggedPasswords' => MemberPassword::class,
         'RememberLoginHashes' => RememberLoginHash::class,
     );
 
@@ -312,7 +312,7 @@ class Member extends DataObject
                 break;
             }
         }
-            return $result;
+        return $result;
     }
 
     /**
@@ -521,10 +521,9 @@ 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()) {
+        if ($member && $member->exists()) {
-                return true;
+            return true;
-            }
         }
 
         return false;
@@ -655,7 +654,7 @@ class Member extends DataObject
     {
         /** @var Member $member */
         $member = static::get()->filter([
-            'AutoLoginHash'                => $hash,
+            'AutoLoginHash' => $hash,
             'AutoLoginExpired:GreaterThan' => DBDatetime::now()->getValue(),
         ])->first();
 
@@ -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;
     }
 
     /**
@@ -892,8 +892,8 @@ class Member extends DataObject
                     'Can\'t overwrite existing member #{id} with identical identifier ({name} = {value}))',
                     'Values in brackets show "fieldname = value", usually denoting an existing email address',
                     array(
-                        'id'    => $existingRecord->ID,
+                        'id' => $existingRecord->ID,
-                        'name'  => $identifierField,
+                        'name' => $identifierField,
                         'value' => $this->$identifierField
                     )
                 ));
@@ -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();
         }
 
@@ -973,17 +977,26 @@ class Member extends DataObject
      * @return bool True if the change can be accepted
      */
     public function onChangeGroups($ids)
+    {
+        // Ensure none of these match disallowed list
+        $disallowedGroupIDs = $this->disallowedGroups();
+        return count(array_intersect($ids, $disallowedGroupIDs)) == 0;
+    }
+
+    /**
+     * List of group IDs this user is disallowed from
+     *
+     * @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 true;
+            return [];
         }
 
-        // If there are no admin groups in this set then it's ok
+        // Non-admins may not belong to admin groups
-        $adminGroups = Permission::get_groups_by_permission('ADMIN');
+        return Permission::get_groups_by_permission('ADMIN')->column('ID');
-        $adminGroupIDs = ($adminGroups) ? $adminGroups->column('ID') : array();
-
-        return count(array_intersect($ids, $adminGroupIDs)) == 0;
     }
 
 
@@ -1159,7 +1172,7 @@ class Member extends DataObject
         if (!$format) {
             $format = [
                 'columns' => ['Surname', 'FirstName'],
-                'sep'     => ' ',
+                'sep' => ' ',
             ];
         }
 
@@ -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 &raquo;
                     $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) {
@@ -1673,8 +1696,8 @@ class Member extends DataObject
      *
      * This method will encrypt the password prior to writing.
      *
-     * @param string $password  Cleartext password
+     * @param string $password Cleartext password
-     * @param bool   $write     Whether to write the member afterwards
+     * @param bool $write Whether to write the member afterwards
      * @return ValidationResult
      */
     public function changePassword($password, $write = true)

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);