Permalink
Browse files

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
  • Loading branch information...
1 parent 7170f38 commit d8a863537488755f34cd57154bcb7758b4f923da @sminnee sminnee committed Oct 19, 2010
Showing with 57 additions and 13 deletions.
  1. +6 −1 dev/SapphireTest.php
  2. +7 −0 security/Member.php
  3. +30 −12 tests/security/MemberTest.php
  4. +14 −0 tests/security/MemberTest.yml
View
@@ -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(
);
View
@@ -1280,6 +1280,13 @@ function canEdit($member = null) {
// 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);
}
@@ -288,19 +288,19 @@ function testIsPasswordExpired() {
$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 @@ function testAddToGroupByCode() {
$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 @@ function testInGroup() {
'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 @@ public function testCanManipulateOwnRecord() {
$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 @@ public function testAuthorisedMembersCanManipulateOthersRecords() {
$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 @@ public function testDecoratedCan() {
$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 @@ public function testDecoratedCan() {
$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 @@ public function testDecoratedCan() {
Object::remove_extension('Member', 'MemberTest_EditingAllowedDeletingDeniedExtension');
$this->addExtensions($extensions);
}
-
+
/**
* Tests for {@link Member::getName()} and {@link Member::setName()}
*/
@@ -500,6 +500,24 @@ function testName() {
$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.
@@ -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

0 comments on commit d8a8635

Please sign in to comment.