Permalink
Browse files

fix(entities): Entity creation no longer needlessly checks owner cont…

…ainer

BREAKING CHANGE:
When creating within a group, ElggEntity::create used to always separately
check if the current user can use the owner's account as a container. This
made sure that one group member could not post to the group using another
member as owner. This separate check led to confusion, as handlers of the container_permissions_check hook were told that the owner was to be the
container, when it was actually the group.

Here we bypass the separate owner container check if the desired owner_guid
is the logged in user GUID. This eliminates the check under all normal
circumstances but leaves it in place in case a poorly coded plugin allows
the impersonation described above.

This also denies creation if the owner/container GUIDs are set but can't
be loaded. Before, create() would simply bypass the permissions check if
it couldn't load the owner/container.

Fixes #4231
  • Loading branch information...
mrclay committed Jul 31, 2015
1 parent fb871be commit 5adf98fd83e6c15a6f417b63e02f1fb4f0c3fcb4
Showing with 42 additions and 7 deletions.
  1. +4 −0 docs/guides/hooks-list.rst
  2. +7 −0 docs/guides/upgrading.rst
  3. +31 −7 engine/classes/ElggEntity.php
@@ -195,6 +195,10 @@ Permission hooks
Return boolean for if the user ``$params['user']`` can use the entity ``$params['container']``
as a container for an entity of ``<entity_type>`` and subtype ``$params['subtype']``.
+ In the rare case where an entity is created with neither the ``container_guid`` nor the ``owner_guid``
+ matching the logged in user, this hook is called *twice*, and in the first call ``$params['container']``
+ will be the *owner*, not the entity's real container.
+
**permissions_check, <entity_type>**
Return boolean for if the user ``$params['user']`` can edit the entity ``$params['entity']``.
@@ -302,6 +302,13 @@ Comments plugin hook
Plugins can now return an empty string from ``'comments',$entity_type`` hook in order to override the default comments component view. To force the default comments component, your plugin must return ``false``. If you were using empty strings to force the default comments view, you need to update your hook handlers to return ``false``.
+Container permissions hook
+--------------------------
+
+The behavior of the ``container_permissions_check`` hook has changed when an entity is being created: Before 2.0, the hook would be called twice if the entity's container was not the owner. On the first call, the entity's owner would be passed in as ``$params['container']``, which could confuse handlers.
+
+In 2.0, when an entity is created in a container like a group, if the owner is the same as the logged in user (almost always the case), this first check is bypassed. So the ``container_permissions_check`` hook will almost always be called once with ``$params['container']`` being the correct container of the entity.
+
Creating a relationship triggers only one event
-----------------------------------------------
@@ -1524,18 +1524,42 @@ protected function create() {
throw new \InvalidParameterException('ACCESS_DEFAULT is not a valid access level. See its documentation in elgglib.h');
}
- $owner = $this->getOwnerEntity();
- if ($owner && !$owner->canWriteToContainer(0, $type, $subtype)) {
- return false;
+ $user_guid = elgg_get_logged_in_user_guid();
+
+ // If given an owner, verify it can be loaded
+ if ($owner_guid) {
+ $owner = $this->getOwnerEntity();
+ if (!$owner) {
+ _elgg_services()->logger->error("User $user_guid tried to create a ($type, $subtype), but the given"
+ . " owner $owner_guid could not be loaded.");
+ return false;
+ }
+
+ // If different owner than logged in, verify can write to container.
+
+ if ($user_guid != $owner_guid && !$owner->canWriteToContainer(0, $type, $subtype)) {
+ _elgg_services()->logger->error("User $user_guid tried to create a ($type, $subtype) with owner"
+ . " $owner_guid, but the user wasn't permitted to write to the owner's container.");
+ return false;
+ }
}
-
- if ($owner_guid != $container_guid) {
+
+ // If given a container, verify it can be loaded and that the current user can write to it
+ if ($container_guid) {
$container = $this->getContainerEntity();
- if ($container && !$container->canWriteToContainer(0, $type, $subtype)) {
+ if (!$container) {
+ _elgg_services()->logger->error("User $user_guid tried to create a ($type, $subtype), but the given"
+ . " container $container_guid could not be loaded.");
+ return false;
+ }
+
+ if (!$container->canWriteToContainer(0, $type, $subtype)) {
+ _elgg_services()->logger->error("User $user_guid tried to create a ($type, $subtype), but was not"
+ . " permitted to write to container $container_guid.");
return false;
}
}
-
+
$result = $this->getDatabase()->insertData("INSERT into {$CONFIG->dbprefix}entities
(type, subtype, owner_guid, site_guid, container_guid,
access_id, time_created, time_updated, last_action)

0 comments on commit 5adf98f

Please sign in to comment.