From 6186282fb1ab0af3dc6ad5b7fc0e939ca0177e0a Mon Sep 17 00:00:00 2001 From: Steve Clay Date: Sat, 20 Aug 2016 15:56:10 -0400 Subject: [PATCH] fix(access): updates no longer mistakenly blocked in some scenarios In order to fix `canEdit()` in 1.9, `update()` needed to load a fresh copy of entity from the DB to check the persisted attributes. In that fix, https://github.com/Elgg/Elgg/commit/66eb9e6d14252314769867cd45aed7557a29532a, a corner case was checked out of my paranoia that a dev would load an invisible entity with access control on, but try to delete it with access off. I now believe this check was unnecessary (we never did similar checks for `delete()`). When the original `canEdit()` attributes issue was resolved in #9434, I should've made this change. --- engine/classes/ElggEntity.php | 6 ------ engine/classes/ElggSession.php | 2 +- engine/tests/ElggCoreMetadataAPITest.php | 7 +++---- engine/tests/ElggCoreUnitTest.php | 17 +++++++++++++++++ engine/tests/ElggEntityTest.php | 21 +++++++++++++++++++++ engine/tests/ElggObjectTest.php | 5 ++--- engine/tests/ElggUserTest.php | 2 +- 7 files changed, 45 insertions(+), 15 deletions(-) diff --git a/engine/classes/ElggEntity.php b/engine/classes/ElggEntity.php index c81ae7b6aa1..a8ce7c16cbd 100644 --- a/engine/classes/ElggEntity.php +++ b/engine/classes/ElggEntity.php @@ -1592,12 +1592,6 @@ protected function update() { _elgg_services()->boot->invalidateCache($this->guid); - if (!has_access_to_entity($this)) { - // Why worry about this case? If access control was off when the user fetched $this, but - // was turned back on again. Better to just bail than to turn access control off again. - return false; - } - if (!$this->canEdit()) { return false; } diff --git a/engine/classes/ElggSession.php b/engine/classes/ElggSession.php index d432cec696c..1e57005c110 100644 --- a/engine/classes/ElggSession.php +++ b/engine/classes/ElggSession.php @@ -209,7 +209,7 @@ public function setLoggedInUser(\ElggUser $user) { /** * Gets the logged in user * - * @return \ElggUser + * @return \ElggUser|null * @since 1.9 */ public function getLoggedInUser() { diff --git a/engine/tests/ElggCoreMetadataAPITest.php b/engine/tests/ElggCoreMetadataAPITest.php index 24c9ca0145e..c07cee70529 100644 --- a/engine/tests/ElggCoreMetadataAPITest.php +++ b/engine/tests/ElggCoreMetadataAPITest.php @@ -161,8 +161,7 @@ public function test_elgg_metadata_multiple_values() { // need to fake different logins. // good times without mocking. - $original_user = elgg_get_logged_in_user_entity(); - $_SESSION['user'] = $u1; + $original_user = $this->replaceSession($u1); elgg_set_ignore_access(false); @@ -182,7 +181,7 @@ public function test_elgg_metadata_multiple_values() { } // add md w/ same name as a different user - $_SESSION['user'] = $u2; + $this->replaceSession($u2); $md_values2 = array( 'four', 'five', @@ -202,7 +201,7 @@ public function test_elgg_metadata_multiple_values() { $this->assertEqual('test', $md->name); } - $_SESSION['user'] = $original_user; + $this->replaceSession($original_user); $obj->delete(); $u1->delete(); diff --git a/engine/tests/ElggCoreUnitTest.php b/engine/tests/ElggCoreUnitTest.php index e87979a8f52..779106adda4 100644 --- a/engine/tests/ElggCoreUnitTest.php +++ b/engine/tests/ElggCoreUnitTest.php @@ -48,6 +48,23 @@ public function assertIdenticalEntities(\ElggEntity $first, \ElggEntity $second, return $this->assert(new IdenticalEntityExpectation($first), $second, $message); } + /** + * Replace the current user session + * + * @param ElggUser $user New user to login as (null to log out) + * @return ElggUser|null Removed session user (or null) + */ + public function replaceSession(ElggUser $user = null) { + $session = elgg_get_session(); + $old = $session->getLoggedInUser(); + if ($user) { + $session->setLoggedInUser($user); + } else { + $session->removeLoggedInUser(); + } + return $old; + } + } /** diff --git a/engine/tests/ElggEntityTest.php b/engine/tests/ElggEntityTest.php index 5c60089e94d..d01b17859e7 100644 --- a/engine/tests/ElggEntityTest.php +++ b/engine/tests/ElggEntityTest.php @@ -379,4 +379,25 @@ public function testCreateWithContainerGuidEqualsZero() { $user->delete(); } + + public function testUpdateAbilityDependsOnCanEdit() { + $this->entity->access_id = ACCESS_PRIVATE; + + $user = new ElggUser(); + $user->save(); + $old_user = $this->replaceSession($user); + + $this->assertFalse($this->entity->save()); + + elgg_register_plugin_hook_handler('permissions_check', 'object', [Elgg\Values::class, 'getTrue']); + + // even though this user can't look up the entity via the DB, permission allows update. + $this->assertFalse(has_access_to_entity($this->entity, $user)); + $this->assertTrue($this->entity->save()); + + elgg_unregister_plugin_hook_handler('permissions_check', 'object', [Elgg\Values::class, 'getTrue']); + + $this->replaceSession($old_user); + $user->delete(); + } } diff --git a/engine/tests/ElggObjectTest.php b/engine/tests/ElggObjectTest.php index cec0704de8f..9a0f57baa44 100644 --- a/engine/tests/ElggObjectTest.php +++ b/engine/tests/ElggObjectTest.php @@ -201,14 +201,13 @@ public function testElggEntityRecursiveDisableWhenLoggedOut() { $guid2 = $e2->getGUID(); // fake being logged out - $user = $_SESSION['user']; - unset($_SESSION['user']); + $old_user = $this->replaceSession(); $ia = elgg_set_ignore_access(true); $this->assertTrue($e1->disable(null, true)); // "log in" original user - $_SESSION['user'] = $user; + $this->replaceSession($old_user); elgg_set_ignore_access($ia); $this->assertFalse(get_entity($guid1)); diff --git a/engine/tests/ElggUserTest.php b/engine/tests/ElggUserTest.php index d2a5a2022c2..7b07c04e18a 100644 --- a/engine/tests/ElggUserTest.php +++ b/engine/tests/ElggUserTest.php @@ -98,7 +98,7 @@ public function testElggUserConstructorWithGarbage() { public function testElggUserConstructorByDbRow() { $row = $this->fetchUser(elgg_get_logged_in_user_guid()); $user = new \ElggUser($row); - $this->assertIdenticalEntities($user, $_SESSION['user']); + $this->assertIdenticalEntities($user, elgg_get_logged_in_user_entity()); } public function testElggUserSave() {