API CHANGE Member->canEdit() returns false if the editing member has lower permissions than the edited member, for example if a member with CMS_ACCESS_SecurityAdmin permissions tries to edit an ADMIN (fixes #5651) (from r110856)

git-svn-id: svn://svn.silverstripe.com/silverstripe/open/modules/sapphire/trunk@112861 467b73ca-7a2a-4603-9d3b-597d59a354a9

dev/SapphireTest.php

@@ -67,7 +67,12 @@ class SapphireTest extends PHPUnit_Framework_TestCase {
 	 * not applied, they will be temporarily added and a database migration called.
 	 * 
 	 * The keys of the are the classes to apply the extensions to, and the values are an array
-	 * of illegal required extensions on that class.
+	 * of required extensions on that class.
+	 * 
+	 * Example:
+	 * <code>
+	 * array("MyTreeDataObject" => array("Versioned", "Hierarchy"))
+	 * </code>
 	 */
 	protected $requiredExtensions = array(
 	);

security/Member.php

@@ -1280,6 +1280,13 @@ class Member extends DataObject {
 		// No member found
 		if(!($member && $member->exists())) return false;
 		
+		// If the requesting member is not an admin, but has access to manage members,
+		// he still can't edit other members with ADMIN permission.
+		// This is a bit weak, strictly speaking he shouldn't be allowed to
+		// perform any action that could change the password on a member
+		// with "higher" permissions than himself, but thats hard to determine.		
+		if(!Permission::checkMember($member, 'ADMIN') && Permission::checkMember($this, 'ADMIN')) return false;
+
 		return $this->canView($member);
 	}
 	

tests/security/MemberTest.php

@@ -288,19 +288,19 @@ class MemberTest extends FunctionalTest {
 		$this->assertFalse($member->isPasswordExpired());
 		
 	}
-
+	
 	function testMemberWithNoDateFormatFallsbackToGlobalLocaleDefaultFormat() {
 		$member = $this->objFromFixture('Member', 'noformatmember');
 		$this->assertEquals('MMM d, yyyy', $member->DateFormat);
 		$this->assertEquals('h:mm:ss a', $member->TimeFormat);
 	}
-
+	
 	function testMemberWithNoDateFormatFallsbackToTheirLocaleDefaultFormat() {
 		$member = $this->objFromFixture('Member', 'delocalemember');
 		$this->assertEquals('dd.MM.yyyy', $member->DateFormat);
 		$this->assertEquals('HH:mm:ss', $member->TimeFormat);
 	}
-
+	
 	function testInGroups() {
 		$staffmember = $this->objFromFixture('Member', 'staffmember');
 		$managementmember = $this->objFromFixture('Member', 'managementmember');
@@ -332,9 +332,9 @@ class MemberTest extends FunctionalTest {
 		
 		$this->assertFalse($grouplessMember->Groups()->exists());
 		$this->assertFalse($memberlessGroup->Members()->exists());
-
+	
 		$grouplessMember->addToGroupByCode('memberless');
-
+	
 		$this->assertEquals($memberlessGroup->Members()->Count(), 1);
 		$this->assertEquals($grouplessMember->Groups()->Count(), 1);
 		
@@ -400,7 +400,7 @@ class MemberTest extends FunctionalTest {
 			'Non-existant group returns false'
 		);
 	}
-
+	
 	/**
 	 * Tests that the user is able to view their own record, and in turn, they can
 	 * edit and delete their own record too.
@@ -428,11 +428,11 @@ class MemberTest extends FunctionalTest {
 		$this->assertFalse($member->canView());
 		$this->assertFalse($member->canDelete());
 		$this->assertFalse($member->canEdit());
-
+	
 		$this->addExtensions($extensions);
 		$this->session()->inst_set('loggedInAs', null);
 	}
-
+	
 	public function testAuthorisedMembersCanManipulateOthersRecords() {
 		$extensions = $this->removeExtensions(Object::get_extensions('Member'));
 		$member = $this->objFromFixture('Member', 'test');
@@ -447,7 +447,7 @@ class MemberTest extends FunctionalTest {
 		$this->addExtensions($extensions);
 		$this->session()->inst_set('loggedInAs', null);
 	}
-
+	
 	public function testDecoratedCan() {
 		$extensions = $this->removeExtensions(Object::get_extensions('Member'));
 		$member = $this->objFromFixture('Member', 'test');
@@ -464,7 +464,7 @@ class MemberTest extends FunctionalTest {
 		$this->assertTrue($member2->canView());
 		$this->assertFalse($member2->canDelete());
 		$this->assertFalse($member2->canEdit());
-
+	
 		/* Apply a decorator that denies viewing of the Member */
 		Object::remove_extension('Member', 'MemberTest_ViewingAllowedExtension');
 		Object::add_extension('Member', 'MemberTest_ViewingDeniedExtension');
@@ -473,7 +473,7 @@ class MemberTest extends FunctionalTest {
 		$this->assertFalse($member3->canView());
 		$this->assertFalse($member3->canDelete());
 		$this->assertFalse($member3->canEdit());
-
+	
 		/* Apply a decorator that allows viewing and editing but denies deletion */
 		Object::remove_extension('Member', 'MemberTest_ViewingDeniedExtension');
 		Object::add_extension('Member', 'MemberTest_EditingAllowedDeletingDeniedExtension');
@@ -486,7 +486,7 @@ class MemberTest extends FunctionalTest {
 		Object::remove_extension('Member', 'MemberTest_EditingAllowedDeletingDeniedExtension');
 		$this->addExtensions($extensions);
 	}
-
+	
 	/**
 	 * Tests for {@link Member::getName()} and {@link Member::setName()}
 	 */
@@ -500,6 +500,24 @@ class MemberTest extends FunctionalTest {
 		$member->Surname = '';
 		$this->assertEquals('Test', $member->getName());
 	}
+	
+	function testMembersWithSecurityAdminAccessCantEditAdminsUnlessTheyreAdminsThemselves() {
+		$adminMember = $this->objFromFixture('Member', 'admin');
+		$otherAdminMember = $this->objFromFixture('Member', 'other-admin');
+		$securityAdminMember = $this->objFromFixture('Member', 'test');
+		$ceoMember = $this->objFromFixture('Member', 'ceomember');
+		
+		// Careful: Don't read as english language.
+		// More precisely this should read canBeEditedBy()
+		
+		$this->assertTrue($adminMember->canEdit($adminMember), 'Admins can edit themselves');
+		$this->assertTrue($otherAdminMember->canEdit($adminMember), 'Admins can edit other admins');
+		$this->assertTrue($securityAdminMember->canEdit($adminMember), 'Admins can edit other members');
+		
+		$this->assertTrue($securityAdminMember->canEdit($securityAdminMember), 'Security-Admins can edit themselves');
+		$this->assertFalse($adminMember->canEdit($securityAdminMember), 'Security-Admins can not edit other admins');
+		$this->assertTrue($ceoMember->canEdit($securityAdminMember), 'Security-Admins can edit other members');
+	}
 
 	/**
 	 * Add the given array of member extensions as class names.

tests/security/MemberTest.yml

@@ -1,7 +1,13 @@
 Permission:
+   admin:
+      Code: ADMIN
    security-admin:
       Code: CMS_ACCESS_SecurityAdmin
 Group:
+   admingroup:
+      Title: Admin
+      Code: admin
+      Permissions: =>Permission.admin
    securityadminsgroup:
       Title: securityadminsgroup
       Code: securityadminsgroup
@@ -25,6 +31,14 @@ Group:
       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