From 577138882163e4b8782ea043487944d30d88e753 Mon Sep 17 00:00:00 2001 From: Damian Mooyman Date: Wed, 11 Apr 2018 13:23:09 +1200 Subject: [PATCH 1/3] [ss-2018-001] Restrict non-admins from being assigned to admin groups --- security/Member.php | 39 ++++++++++++++++++++++++----------- tests/security/MemberTest.php | 30 +++++++++++++++++++++++++++ 2 files changed, 57 insertions(+), 12 deletions(-) diff --git a/security/Member.php b/security/Member.php index 927ae14c0..cd1d22518 100644 --- a/security/Member.php +++ b/security/Member.php @@ -1042,15 +1042,24 @@ class Member extends DataObject implements TemplateGlobalProvider { * @return boolean 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; + if (Permission::check('ADMIN') || Permission::checkMember($this, 'ADMIN')) { + return array(); } - // 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'); } @@ -1465,12 +1474,18 @@ class Member extends DataObject implements TemplateGlobalProvider { $fields->removeByName('LoggedPasswords'); if(Permission::check('EDIT_PERMISSIONS')) { - $groupsMap = array(); - foreach(Group::get() as $group) { - // Listboxfield values are escaped, use ASCII char instead of » - $groupsMap[$group->ID] = $group->getBreadcrumbs(' > '); - } - asort($groupsMap); + // Filter allowed groups + $groups = Group::get(); + $disallowedGroupIDs = $this->disallowedGroups(); + if ($disallowedGroupIDs) { + $groups = $groups->exclude('ID', $disallowedGroupIDs); + } + $groupsMap = array(); + foreach ($groups as $group) { + // Listboxfield values are escaped, use ASCII char instead of » + $groupsMap[$group->ID] = $group->getBreadcrumbs(' > '); + } + asort($groupsMap); $fields->addFieldToTab('Root.Main', ListboxField::create('DirectGroups', singleton('Group')->i18n_plural_name()) ->setMultiple(true) diff --git a/tests/security/MemberTest.php b/tests/security/MemberTest.php index 22753c4e1..46ac2c17e 100644 --- a/tests/security/MemberTest.php +++ b/tests/security/MemberTest.php @@ -665,6 +665,36 @@ class MemberTest extends FunctionalTest { ); } + /** + * Ensure DirectGroups listbox disallows admin-promotion + */ + public function testAllowedGroupsListbox() { + /** @var Group $adminGroup */ + $adminGroup = $this->objFromFixture('Group', 'admingroup'); + /** @var Member $staffMember */ + $staffMember = $this->objFromFixture('Member', 'staffmember'); + /** @var Member $adminMember */ + $adminMember = $this->objFromFixture('Member', '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 */ From 9d055dd94658012d3f3d94d4c3a4a2f28d37d744 Mon Sep 17 00:00:00 2001 From: Robbie Averill Date: Thu, 10 May 2018 09:38:08 +1200 Subject: [PATCH 2/3] Added 3.5.8-rc1 changelog --- docs/en/04_Changelogs/rc/3.5.8-rc1.md | 9 +++++++++ 1 file changed, 9 insertions(+) create mode 100644 docs/en/04_Changelogs/rc/3.5.8-rc1.md diff --git a/docs/en/04_Changelogs/rc/3.5.8-rc1.md b/docs/en/04_Changelogs/rc/3.5.8-rc1.md new file mode 100644 index 000000000..c19407a63 --- /dev/null +++ b/docs/en/04_Changelogs/rc/3.5.8-rc1.md @@ -0,0 +1,9 @@ +# 3.5.8-rc1 + + + +## Change Log + +### Security + + * 2018-04-11 [577138882]() Restrict non-admins from being assigned to admin groups (Damian Mooyman) - See [ss-2018-001](http://www.silverstripe.org/download/security-releases/ss-2018-001) From 912dc60cf369c559e090ee33433f84fd5f7a7b6c Mon Sep 17 00:00:00 2001 From: Robbie Averill Date: Thu, 10 May 2018 11:59:58 +1200 Subject: [PATCH 3/3] Added 3.5.8 changelog --- docs/en/04_Changelogs/3.5.8.md | 9 +++++++++ 1 file changed, 9 insertions(+) create mode 100644 docs/en/04_Changelogs/3.5.8.md diff --git a/docs/en/04_Changelogs/3.5.8.md b/docs/en/04_Changelogs/3.5.8.md new file mode 100644 index 000000000..39c54a5d5 --- /dev/null +++ b/docs/en/04_Changelogs/3.5.8.md @@ -0,0 +1,9 @@ +# 3.5.8 + + + +## Change Log + +### Security + + * 2018-04-11 [577138882](https://github.com/silverstripe/silverstripe-framework/commit/577138882) Restrict non-admins from being assigned to admin groups (Damian Mooyman) - See [ss-2018-001](http://www.silverstripe.org/download/security-releases/ss-2018-001)