fix(permissions): All permissions functions handle user fetches consi…

…stently All permissions functions now find hidden/disabled users and, when a given GUID isn't found, they return false and issue a warning. Previously, only some functions issued warnings, and some functions would silently substitute the logged in user if a given GUID couldn't load. Fixes #8941 Fixes #8038 Fixes #8945
Elgg · Sep 20, 2015 · b875fd3 · b875fd3
1 parent 367d3c3
commit b875fd3
Show file tree

Hide file tree

Showing 8 changed files with 118 additions and 84 deletions.
diff --git a/engine/classes/Elgg/Database/AccessCollections.php b/engine/classes/Elgg/Database/AccessCollections.php
@@ -2,6 +2,8 @@
 
 namespace Elgg\Database;
 
+use Elgg\Database\EntityTable\UserFetchFailureException;
+
 /**
  * WARNING: API IN FLUX. DO NOT USE DIRECTLY.
  *
@@ -436,19 +438,19 @@ function getWriteAccessArray($user_guid = 0, $site_guid = 0, $flush = false, arr
 	 * @return bool
 	 */
 	function canEdit($collection_id, $user_guid = null) {
-		if ($user_guid) {
+		try {
-			$user = _elgg_services()->entityTable->get((int) $user_guid);
+			$user = _elgg_services()->entityTable->getUserForPermissionsCheck($user_guid);
-		} else {
+		} catch (UserFetchFailureException $e) {
-			$user = _elgg_services()->session->getLoggedInUser();
+			return false;
 		}
 
 		$collection = get_access_collection($collection_id);
 
-		if (!($user instanceof \ElggUser) || !$collection) {
+		if (!$user || !$collection) {
 			return false;
 		}
 
-		$write_access = get_write_access_array($user->getGUID(), 0, true);
+		$write_access = get_write_access_array($user->guid, 0, true);
 
 		// don't ignore access when checking users.
 		if ($user_guid) {

diff --git a/engine/classes/Elgg/Database/EntityTable.php b/engine/classes/Elgg/Database/EntityTable.php
@@ -1,6 +1,7 @@
 <?php
 namespace Elgg\Database;
 
+use Elgg\Database\EntityTable\UserFetchFailureException;
 use IncompleteEntityException;
 
 /**
@@ -1235,4 +1236,39 @@ function updateLastAction($guid, $posted = null) {
 			return false;
 		}
 	}
-}
+
+	/**
+	 * Get a user by GUID even if the entity is hidden or disabled
+	 *
+	 * @param int $guid User GUID. Default is logged in user
+	 *
+	 * @return \ElggUser|false
+	 * @throws UserFetchFailureException
+	 * @access private
+	 */
+	public function getUserForPermissionsCheck($guid = 0) {
+		if (!$guid) {
+			return _elgg_services()->session->getLoggedInUser();
+		}
+
+		// need to ignore access and show hidden entities for potential hidden/disabled users
+		$ia = _elgg_services()->session->setIgnoreAccess(true);
+		$show_hidden = access_show_hidden_entities(true);
+
+		$user = $this->get($guid, 'user');
+
+		_elgg_services()->session->setIgnoreAccess($ia);
+		access_show_hidden_entities($show_hidden);
+
+		if (!$user) {
+			// requested to check access for a specific user_guid, but there is no user entity, so the caller
+			// should cancel the check and return false
+			$message = _elgg_services()->translator->translate('UserFetchFailureException', array($guid));
+			_elgg_services()->logger->warn($message);
+
+			throw new UserFetchFailureException();
+		}
+
+		return $user;
+	}
+}
diff --git a/engine/classes/Elgg/Database/EntityTable/UserFetchResultException.php b/engine/classes/Elgg/Database/EntityTable/UserFetchResultException.php
@@ -0,0 +1,8 @@
+<?php
+
+namespace Elgg\Database\EntityTable;
+
+/**
+ * Exception indicating a user could not be looked up for a permissions check
+ */
+class UserFetchFailureException extends \RuntimeException {}
diff --git a/engine/classes/Elgg/WidgetsService.php b/engine/classes/Elgg/WidgetsService.php
@@ -1,6 +1,8 @@
 <?php
 namespace Elgg;
 
+use Elgg\Database\EntityTable\UserFetchFailureException;
+
 /**
  * WARNING: API IN FLUX. DO NOT USE DIRECTLY.
  *
@@ -108,17 +110,16 @@ public function createWidget($owner_guid, $handler, $context, $access_id = null)
 	 * @since 1.9.0
 	 */
 	public function canEditLayout($context, $user_guid = 0) {
-		$user = get_entity((int)$user_guid);
+		try {
-		if (!$user) {
+			$user = _elgg_services()->entityTable->getUserForPermissionsCheck($user_guid);
-			$user = _elgg_services()->session->getLoggedInUser();
+		} catch (UserFetchFailureException $e) {
+			return false;
 		}
 
-		$return = false;
+		if ($user) {
-		if (_elgg_services()->session->isAdminLoggedIn()) {
+			$return = ($user->isAdmin() || (elgg_get_page_owner_guid() == $user->guid));
-			$return = true;
+		} else {
-		}
+			$return = false;
-		if (elgg_get_page_owner_guid() == $user->guid) {
-			$return = true;
 		}
 
 		$params = array(

diff --git a/engine/classes/ElggAnnotation.php b/engine/classes/ElggAnnotation.php
@@ -1,4 +1,7 @@
 <?php
+
+use Elgg\Database\EntityTable\UserFetchFailureException;
+
 /**
  * Elgg Annotations
  *
@@ -120,27 +123,23 @@ public function enable() {
 	 * @see elgg_set_ignore_access()
 	 */
 	public function canEdit($user_guid = 0) {
-
+		try {
-		if ($user_guid) {
+			$user = _elgg_services()->entityTable->getUserForPermissionsCheck($user_guid);
-			$user = get_user($user_guid);
+		} catch (UserFetchFailureException $e) {
-			if (!$user) {
+			return false;
-				return false;
-			}
-		} else {
-			$user = _elgg_services()->session->getLoggedInUser();
-			$user_guid = _elgg_services()->session->getLoggedInUserGuid();
 		}
 
 		$result = false;
+		$entity = $this->getEntity();
+
 		if ($user) {
 			// If the owner of annotation is the specified user, they can edit.
-			if ($this->getOwnerGUID() == $user_guid) {
+			if ($this->owner_guid == $user->guid) {
 				$result = true;
 			}
 
 			// If the user can edit the entity this is attached to, they can edit.
-			$entity = $this->getEntity();
+			if ($result == false && $entity->canEdit($user->guid)) {
-			if ($result == false && $entity->canEdit($user->getGUID())) {
 				$result = true;
 			}
 		}

diff --git a/engine/classes/ElggEntity.php b/engine/classes/ElggEntity.php
@@ -1,4 +1,7 @@
 <?php
+
+use Elgg\Database\EntityTable\UserFetchFailureException;
+
 /**
  * The parent class for all Elgg Entities.
  *
@@ -991,30 +994,30 @@ public function countEntitiesFromRelationship($relationship, $inverse_relationsh
 	 * @see elgg_set_ignore_access()
 	 */
 	public function canEdit($user_guid = 0) {
-		$user_guid = (int)$user_guid;
+		try {
-		$user = get_entity($user_guid);
+			$user = _elgg_services()->entityTable->getUserForPermissionsCheck($user_guid);
-		if (!$user) {
+		} catch (UserFetchFailureException $e) {
-			$user = _elgg_services()->session->getLoggedInUser();
+			return false;
 		}
 
 		$return = false;
 
 		// Test user if possible - should default to false unless a plugin hook says otherwise
 		if ($user) {
-			if ($this->getOwnerGUID() == $user->getGUID()) {
+			if ($this->owner_guid == $user->guid) {
 				$return = true;
 			}
 
-			if ($this->getContainerGUID() == $user->getGUID()) {
+			if ($this->container_guid == $user->guid) {
 				$return = true;
 			}
 
-			if ($this->getGUID() == $user->getGUID()) {
+			if ($this->guid == $user->guid) {
 				$return = true;
 			}
 
 			$container = $this->getContainerEntity();
-			if ($container && $container->canEdit($user->getGUID())) {
+			if ($container && $container->canEdit($user->guid)) {
 				$return = true;
 			}
 		}
@@ -1035,29 +1038,12 @@ public function canEdit($user_guid = 0) {
 	 * @see elgg_set_ignore_access()
 	 */
 	public function canDelete($user_guid = 0) {
-		$user_guid = (int) $user_guid;
+		try {
-
+			$user = _elgg_services()->entityTable->getUserForPermissionsCheck($user_guid);
-		if (!$user_guid) {
+		} catch (UserFetchFailureException $e) {
-			$user_guid = _elgg_services()->session->getLoggedInUserGuid();
-		}
-
-		// need to ignore access and show hidden entities for potential hidden/disabled users
-		$ia = elgg_set_ignore_access(true);
-		$show_hidden = access_show_hidden_entities(true);
-
-		$user = _elgg_services()->entityTable->get($user_guid, 'user');
-
-		elgg_set_ignore_access($ia);
-		access_show_hidden_entities($show_hidden);
-
-		if ($user_guid & !$user) {
-			// requested to check access for a specific user_guid, but there is no user entity, so return false
-			$message = _elgg_services()->translator->translate('entity:can_delete:invaliduser', array($user_guid));
-			_elgg_services()->logger->warning($message);
-
 			return false;
 		}
-		
+
 		$return = $this->canEdit($user_guid);
 
 		$params = array('entity' => $this, 'user' => $user);
@@ -1085,13 +1071,13 @@ public function canEditMetadata($metadata = null, $user_guid = 0) {
 			return false;
 		}
 
-		if ($user_guid) {
+		try {
-			$user = get_user($user_guid);
+			$user = _elgg_services()->entityTable->getUserForPermissionsCheck($user_guid);
-			if (!$user) {
+		} catch (UserFetchFailureException $e) {
-				return false;
+			return false;
-			}
+		}
-		} else {
+
-			$user = _elgg_services()->session->getLoggedInUser();
+		if ($user) {
 			$user_guid = $user->guid;
 		}
 
@@ -1136,10 +1122,11 @@ public function canWriteToContainer($user_guid = 0, $type = 'all', $subtype = 'a
 	 * @return bool
 	 */
 	public function canComment($user_guid = 0) {
-		if ($user_guid == 0) {
+		try {
-			$user_guid = _elgg_services()->session->getLoggedInUserGuid();
+			$user = _elgg_services()->entityTable->getUserForPermissionsCheck($user_guid);
+		} catch (UserFetchFailureException $e) {
+			return false;
 		}
-		$user = get_entity($user_guid);
 
 		// By default, we don't take a position of whether commenting is allowed
 		// because it is handled by the subclasses of \ElggEntity
@@ -1162,15 +1149,13 @@ public function canComment($user_guid = 0) {
 	 * @return bool
 	 */
 	public function canAnnotate($user_guid = 0, $annotation_name = '') {
-		if ($user_guid == 0) {
+		try {
-			$user_guid = _elgg_services()->session->getLoggedInUserGuid();
+			$user = _elgg_services()->entityTable->getUserForPermissionsCheck($user_guid);
+		} catch (UserFetchFailureException $e) {
+			return false;
 		}
-		$user = get_entity($user_guid);
 
-		$return = true;
+		$return = (bool)$user;
-		if (!$user) {
-			$return = false;
-		}
 
 		$hooks = _elgg_services()->hooks;
 

diff --git a/engine/lib/entities.php b/engine/lib/entities.php
@@ -6,6 +6,8 @@
  * @subpackage DataModel.Entities
  */
 
+use Elgg\Database\EntityTable\UserFetchFailureException;
+
 /**
  * Cache entities in memory once loaded.
  *
@@ -302,15 +304,14 @@ function can_write_to_container($user_guid = 0, $container_guid = 0, $type = 'al
 
 	$container = get_entity($container_guid);
 
-	$user_guid = (int)$user_guid;
+	try {
-	if ($user_guid == 0) {
+		$user = _elgg_services()->entityTable->getUserForPermissionsCheck($user_guid);
-		$user = elgg_get_logged_in_user_entity();
+	} catch (UserFetchFailureException $e) {
-		$user_guid = elgg_get_logged_in_user_guid();
+		return false;
-	} else {
+	}
-		$user = get_user($user_guid);
+
-		if (!$user) {
+	if ($user) {
-			return false;
+		$user_guid = $user->guid;
-		}
 	}
 
 	$return = false;

diff --git a/languages/en.php b/languages/en.php
@@ -100,6 +100,8 @@
 	'LoginException:ChangePasswordFailure' => 'Failed current password check.',
 	'LoginException:Unknown' => 'We could not log you in due to an unknown error.',
 
+	'UserFetchFailureException' => 'Cannot check permission for user_guid [%s] as the user does not exist.',
+
 	'deprecatedfunction' => 'Warning: This code uses the deprecated function \'%s\' and is not compatible with this version of Elgg',
 
 	'pageownerunavailable' => 'Warning: The page owner %d is not accessible!',
@@ -1287,7 +1289,7 @@
 	'entity:delete:success' => 'Entity %s has been deleted',
 	'entity:delete:fail' => 'Entity %s could not be deleted',
 
-	'entity:can_delete:invaliduser' => 'Can not check canDelete for user_guid [%s] as the user does not exist.',
+	'entity:can_delete:invaliduser' => 'Cannot check canDelete() for user_guid [%s] as the user does not exist.',
 
 /**
  * Action gatekeeper