Merge pull request #10103 from mrclay/10098_update

fix(access): updates no longer mistakenly blocked in some scenarios
Elgg · Aug 23, 2016 · b0f8d16 · b0f8d16
2 parents 139bb14 + 01f4f1d
commit b0f8d16
Show file tree

Hide file tree

Showing 7 changed files with 57 additions and 15 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -379,4 +379,37 @@ public function testCreateWithContainerGuidEqualsZero() {
 		$user->delete();
 
 	}
+
+	public function testUpdateAbilityDependsOnCanEdit() {
+		$this->entity->access_id = ACCESS_PRIVATE;
+
+		$this->assertTrue($this->entity->save());
+
+		// even owner can't bypass permissions
+		elgg_register_plugin_hook_handler('permissions_check', 'object', [Elgg\Values::class, 'getFalse'], 999);
+		$this->assertFalse($this->entity->save());
+		elgg_unregister_plugin_hook_handler('permissions_check', 'object', [Elgg\Values::class, 'getFalse']);
+
+		$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']);
+
+		// can save with access ignore
+		$ia = elgg_set_ignore_access();
+		$this->assertTrue($this->entity->save());
+		elgg_set_ignore_access($ia);
+
+		$this->replaceSession($old_user);
+		$user->delete();
+	}
 }
diff --git 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
@@ -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() {