From 35628832d6622e2d169221ea161a65badf8a109c Mon Sep 17 00:00:00 2001 From: Sean Harvey Date: Wed, 16 Dec 2009 05:39:39 +0000 Subject: [PATCH] BUGFIX #4686 Fixed $member non-object error, and decorated checks from not working in Member::canView(), Member::canEdit() and Member::canDelete() MINOR Added additional tests to MemberTest (from r94358) git-svn-id: svn://svn.silverstripe.com/silverstripe/open/modules/sapphire/trunk@95601 467b73ca-7a2a-4603-9d3b-597d59a354a9 --- security/Member.php | 17 ++++++-- tests/security/MemberTest.php | 81 ++++++++++++++++++++++++++++++++++- tests/security/MemberTest.yml | 8 ++++ 3 files changed, 101 insertions(+), 5 deletions(-) diff --git a/security/Member.php b/security/Member.php index da38622d2..fbd8cae90 100644 --- a/security/Member.php +++ b/security/Member.php @@ -972,10 +972,13 @@ class Member extends DataObject { // decorated access checks $results = $this->extend('canView', $member); - if($results && is_array($results)) if(!min($results)) return false; + if($results && is_array($results)) { + if(!min($results)) return false; + else return true; + } // members can usually edit their own record - if($this->ID == $member->ID) return true; + if($member && $this->ID == $member->ID) return true; if( Permission::checkMember($member, 'ADMIN') @@ -996,7 +999,10 @@ class Member extends DataObject { // decorated access checks $results = $this->extend('canEdit', $member); - if($results && is_array($results)) if(!min($results)) return false; + if($results && is_array($results)) { + if(!min($results)) return false; + else return true; + } // No member found if(!($member && $member->exists())) return false; @@ -1013,7 +1019,10 @@ class Member extends DataObject { // decorated access checks $results = $this->extend('canDelete', $member); - if($results && is_array($results)) if(!min($results)) return false; + if($results && is_array($results)) { + if(!min($results)) return false; + else return true; + } // No member found if(!($member && $member->exists())) return false; diff --git a/tests/security/MemberTest.php b/tests/security/MemberTest.php index f5e0de5b7..b06f0e3b5 100644 --- a/tests/security/MemberTest.php +++ b/tests/security/MemberTest.php @@ -3,7 +3,7 @@ * @package sapphire * @subpackage tests */ -class MemberTest extends SapphireTest { +class MemberTest extends FunctionalTest { static $fixture_file = 'sapphire/tests/security/MemberTest.yml'; function setUp() { @@ -300,4 +300,83 @@ class MemberTest extends SapphireTest { '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. + */ + public function testCanManipulateOwnRecord() { + $extensions = $this->removeExtensions(Object::get_extensions('Member')); + $member = $this->objFromFixture('Member', 'test'); + $member2 = $this->objFromFixture('Member', 'staffmember'); + + $this->session()->inst_set('loggedInAs', null); + + /* Not logged in, you can't view, delete or edit the record */ + $this->assertFalse($member->canView()); + $this->assertFalse($member->canDelete()); + $this->assertFalse($member->canEdit()); + + /* Logged in users can edit their own record */ + $this->session()->inst_set('loggedInAs', $member->ID); + $this->assertTrue($member->canView()); + $this->assertTrue($member->canDelete()); + $this->assertTrue($member->canEdit()); + + /* Other uses cannot view, delete or edit others records */ + $this->session()->inst_set('loggedInAs', $member2->ID); + $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'); + $member2 = $this->objFromFixture('Member', 'staffmember'); + + /* Group members with SecurityAdmin permissions can manipulate other records */ + $this->session()->inst_set('loggedInAs', $member->ID); + $this->assertTrue($member2->canView()); + $this->assertTrue($member2->canDelete()); + $this->assertTrue($member2->canEdit()); + + $this->addExtensions($extensions); + $this->session()->inst_set('loggedInAs', null); + } + + /** + * Add the given array of member extensions as class names. + * This is useful for re-adding extensions after being removed + * in a test case to produce an unbiased test. + * + * @param array $extensions + * @return array The added extensions + */ + protected function addExtensions($extensions) { + if($extensions) foreach($extensions as $extension) { + Object::add_extension('Member', $extension); + } + return $extensions; + } + + /** + * Remove given extensions from Member. This is useful for + * removing extensions that could produce a biased + * test result, as some extensions applied by project + * code or modules can do this. + * + * @param array $extensions + * @return array The removed extensions + */ + protected function removeExtensions($extensions) { + if($extensions) foreach($extensions as $extension) { + Object::remove_extension('Member', $extension); + } + return $extensions; + } + } \ No newline at end of file diff --git a/tests/security/MemberTest.yml b/tests/security/MemberTest.yml index 6c64b2154..9b9e87aec 100644 --- a/tests/security/MemberTest.yml +++ b/tests/security/MemberTest.yml @@ -1,4 +1,11 @@ +Permission: + security-admin: + Code: CMS_ACCESS_SecurityAdmin Group: + securityadminsgroup: + Title: securityadminsgroup + Code: securityadminsgroup + Permissions: =>Permission.security-admin staffgroup: Title: staffgroup Code: staffgroup @@ -21,6 +28,7 @@ Member: Email: sam@silverstripe.com Password: 1nitialPassword PasswordExpiry: 2030-01-01 + Groups: =>Group.securityadminsgroup expiredpassword: FirstName: Test Surname: User