From 577138882163e4b8782ea043487944d30d88e753 Mon Sep 17 00:00:00 2001
From: Damian Mooyman <damian@silverstripe.com>
Date: Wed, 11 Apr 2018 13:23:09 +1200
Subject: [PATCH] [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 &raquo;
-					$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 &raquo;
+                    $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
 	 */