diff --git a/security/Member.php b/security/Member.php index 964b329c8..9a5bf8d96 100644 --- a/security/Member.php +++ b/security/Member.php @@ -1294,16 +1294,20 @@ class Member extends DataObject implements TemplateGlobalProvider { */ public function canDelete($member = null) { if(!$member || !(is_a($member, 'Member')) || is_numeric($member)) $member = Member::currentUser(); - + // extended access checks $results = $this->extend('canDelete', $member); if($results && is_array($results)) { if(!min($results)) return false; else return true; } - + // No member found if(!($member && $member->exists())) return false; + + // Members are not allowed to remove themselves, + // since it would create inconsistencies in the admin UIs. + if($this->ID && $member->ID == $this->ID) return false; return $this->canEdit($member); } diff --git a/tests/security/MemberTest.php b/tests/security/MemberTest.php index 099c8dcfb..c61f77cc7 100644 --- a/tests/security/MemberTest.php +++ b/tests/security/MemberTest.php @@ -432,7 +432,7 @@ class MemberTest extends FunctionalTest { /* Logged in users can edit their own record */ $this->session()->inst_set('loggedInAs', $member->ID); $this->assertTrue($member->canView()); - $this->assertTrue($member->canDelete()); + $this->assertFalse($member->canDelete()); $this->assertTrue($member->canEdit()); /* Other uses cannot view, delete or edit others records */ @@ -653,6 +653,34 @@ class MemberTest extends FunctionalTest { $this->assertFalse($m2->validateAutoLoginToken($m1Token), 'Fails token validity test against other member.'); } + public function testCanDelete() { + $admin1 = $this->objFromFixture('Member', 'admin'); + $admin2 = $this->objFromFixture('Member', 'other-admin'); + $member1 = $this->objFromFixture('Member', 'grouplessmember'); + $member2 = $this->objFromFixture('Member', 'noformatmember'); + + $this->assertTrue( + $admin1->canDelete($admin2), + 'Admins can delete other admins' + ); + $this->assertTrue( + $member1->canDelete($admin2), + 'Admins can delete non-admins' + ); + $this->assertFalse( + $admin1->canDelete($admin1), + 'Admins can not delete themselves' + ); + $this->assertFalse( + $member1->canDelete($member2), + 'Non-admins can not delete other non-admins' + ); + $this->assertFalse( + $member1->canDelete($member1), + 'Non-admins can not delete themselves' + ); + } + } class MemberTest_ViewingAllowedExtension extends DataExtension implements TestOnly {