Merge pull request #375 from halkyon/onchangegroups_bug

Member::onChangeGroups() should allow ADMIN permission grant if logged in user is admin
silverstripe · Apr 27, 2012 · cbc5d3c · cbc5d3c
2 parents bd6ca59 + 8a6671d
commit cbc5d3c
Show file tree

Hide file tree

Showing 2 changed files with 11 additions and 3 deletions.
diff --git a/security/Member.php b/security/Member.php
@@ -702,9 +702,9 @@ function onAfterWrite() {
 	 * @return boolean
 	 */
 	function onChangeGroups($ids) {
-		// Filter out admin groups to avoid privilege escalation, 
-		// unless the current user is an admin already
-		if(!Permission::checkMember($this, 'ADMIN')) {
+		// Filter out admin groups to avoid privilege escalation,
+		// unless the current user is an admin already OR the logged in user is an admin
+		if(!(Permission::check('ADMIN') || Permission::checkMember($this, 'ADMIN'))) {
 			$adminGroups = Permission::get_groups_by_permission('ADMIN');
 			$adminGroupIDs = ($adminGroups) ? $adminGroups->column('ID') : array();
 			return count(array_intersect($ids, $adminGroupIDs)) == 0;

diff --git a/tests/security/MemberTest.php b/tests/security/MemberTest.php
@@ -551,6 +551,14 @@ function testOnChangeGroups() {
 			$staffMember->onChangeGroups(array($newAdminGroup->ID)),
 			'Adding new admin group relation is not allowed for non-admin members'
 		);
+
+		$this->session()->inst_set('loggedInAs', $adminMember->ID);
+		$this->assertTrue(
+			$staffMember->onChangeGroups(array($newAdminGroup->ID)),
+			'Adding new admin group relation is allowed for normal users, when granter is logged in as admin'
+		);
+		$this->session()->inst_set('loggedInAs', null);
+
 		$this->assertTrue(
 			$adminMember->onChangeGroups(array($newAdminGroup->ID)),
 			'Adding new admin group relation is allowed for admin members'