Permalink
Browse files

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,
66eb9e6,
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
  • Loading branch information...
mrclay committed Aug 20, 2016
1 parent 139bb14 commit 01f4f1df12d95c9828c5e7581b5352c7953e8109
@@ -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;
}
@@ -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() {
@@ -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();
@@ -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;
+ }
+
}
/**
@@ -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();
+ }
}
@@ -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));
@@ -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() {

0 comments on commit 01f4f1d

Please sign in to comment.