diff --git a/security/Member.php b/security/Member.php index a09329f6b..de28fe2ac 100644 --- a/security/Member.php +++ b/security/Member.php @@ -542,6 +542,24 @@ class Member extends DataObject { MemberPassword::log($this); } } + + /** + * If any admin groups are requested, deny the whole save operation. + * + * @param Array $ids Database IDs of Group records + * @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')) { + $adminGroups = Permission::get_groups_by_permission('ADMIN'); + $adminGroupIDs = ($adminGroups) ? $adminGroups->column('ID') : array(); + return count(array_intersect($ids, $adminGroupIDs)) == 0; + } else { + return true; + } + } /** diff --git a/tests/security/MemberTest.php b/tests/security/MemberTest.php index 2e76d123a..ceac25a17 100644 --- a/tests/security/MemberTest.php +++ b/tests/security/MemberTest.php @@ -260,4 +260,33 @@ class MemberTest extends SapphireTest { 'Non-existant group returns false' ); } + + function testOnChangeGroups() { + $staffGroup = $this->objFromFixture('Group', 'staffgroup'); + $adminGroup = $this->objFromFixture('Group', 'admingroup'); + $staffMember = $this->objFromFixture('Member', 'staffmember'); + $adminMember = $this->objFromFixture('Member', 'admin'); + $newAdminGroup = new Group(array('Title' => 'newadmin')); + $newAdminGroup->write(); + Permission::grant($newAdminGroup->ID, 'ADMIN'); + $newOtherGroup = new Group(array('Title' => 'othergroup')); + $newOtherGroup->write(); + + $this->assertTrue( + $staffMember->onChangeGroups(array($staffGroup->ID)), + 'Adding existing non-admin group relation is allowed for non-admin members' + ); + $this->assertTrue( + $staffMember->onChangeGroups(array($newOtherGroup->ID)), + 'Adding new non-admin group relation is allowed for non-admin members' + ); + $this->assertFalse( + $staffMember->onChangeGroups(array($newAdminGroup->ID)), + 'Adding new admin group relation is not allowed for non-admin members' + ); + $this->assertTrue( + $adminMember->onChangeGroups(array($newAdminGroup->ID)), + 'Adding new admin group relation is allowed for admin members' + ); + } } \ No newline at end of file diff --git a/tests/security/MemberTest.yml b/tests/security/MemberTest.yml index 6c64b2154..3ea9a90f1 100644 --- a/tests/security/MemberTest.yml +++ b/tests/security/MemberTest.yml @@ -1,4 +1,17 @@ +Permission: + admin: + Code: ADMIN + security-admin: + Code: CMS_ACCESS_SecurityAdmin Group: + admingroup: + Title: Admin + Code: admin + Permissions: =>Permission.admin + securityadminsgroup: + Title: securityadminsgroup + Code: securityadminsgroup + Permissions: =>Permission.security-admin staffgroup: Title: staffgroup Code: staffgroup @@ -13,14 +26,26 @@ Group: ceogroup: Title: ceogroup Code: ceogroup - Parent: =>Group.managementgroup + Parent: =>Group.managementgroup + memberlessgroup: + Title: Memberless Group + code: memberless Member: + admin: + FirstName: Admin + Email: admin@silverstripe.com + Groups: =>Group.admingroup + other-admin: + FirstName: OtherAdmin + Email: other-admin@silverstripe.com + Groups: =>Group.admingroup test: FirstName: Test Surname: User Email: sam@silverstripe.com Password: 1nitialPassword PasswordExpiry: 2030-01-01 + Groups: =>Group.securityadminsgroup expiredpassword: FirstName: Test Surname: User @@ -43,4 +68,11 @@ Member: Groups: =>Group.accountinggroup ceomember: Email: ceomember@test.com - Groups: =>Group.ceogroup \ No newline at end of file + Groups: =>Group.ceogroup + grouplessmember: + FirstName: Groupless Member + noformatmember: + Email: noformat@test.com + delocalemember: + Email: delocalemember@test.com + Locale: de_DE \ No newline at end of file