From e409d6f673c49846086b23677aecdc3fde5fc4d5 Mon Sep 17 00:00:00 2001 From: Damian Mooyman Date: Wed, 11 Apr 2018 13:04:22 +1200 Subject: [PATCH] [ss-2018-001] Restrict non-admins from being assigned to admin groups --- src/Security/Member.php | 101 ++++++++++++++++++------------ tests/php/Security/MemberTest.php | 38 ++++++++++- 2 files changed, 97 insertions(+), 42 deletions(-) diff --git a/src/Security/Member.php b/src/Security/Member.php index 40bdd9389..f1cf2b568 100644 --- a/src/Security/Member.php +++ b/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', - 'Surname' => 'Varchar', - '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 - 'TempIDExpired' => 'Datetime', // Expiry of temp login - 'Password' => 'Varchar(160)', - 'AutoLoginHash' => 'Varchar(160)', // Used to auto-login the user on password reset - 'AutoLoginExpired' => 'Datetime', + 'FirstName' => 'Varchar', + 'Surname' => 'Varchar', + '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 + 'TempIDExpired' => 'Datetime', // Expiry of temp login + 'Password' => 'Varchar(160)', + 'AutoLoginHash' => 'Varchar(160)', // Used to auto-login the user on password reset + '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)', - 'PasswordExpiry' => 'Date', - 'LockedOutUntil' => 'Datetime', - 'Locale' => 'Varchar(6)', + 'Salt' => 'Varchar(50)', + 'PasswordExpiry' => 'Date', + 'LockedOutUntil' => 'Datetime', + '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()) { - if ($member && $member->exists()) { - return true; - } + $member = Security::getCurrentUser(); + if ($member && $member->exists()) { + 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, - 'name' => $identifierField, + 'id' => $existingRecord->ID, + '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 - $adminGroups = Permission::get_groups_by_permission('ADMIN'); - $adminGroupIDs = ($adminGroups) ? $adminGroups->column('ID') : array(); - - return count(array_intersect($ids, $adminGroupIDs)) == 0; + // Non-admins may not belong to admin groups + return Permission::get_groups_by_permission('ADMIN')->column('ID'); } @@ -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 » $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 bool $write Whether to write the member afterwards + * @param string $password Cleartext password + * @param bool $write Whether to write the member afterwards * @return ValidationResult */ public function changePassword($password, $write = true) diff --git a/tests/php/Security/MemberTest.php b/tests/php/Security/MemberTest.php index 8476380ca..ccb069ad9 100644 --- a/tests/php/Security/MemberTest.php +++ b/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);