chore(access): type/subtype is required for container permissions

BREAKING CHANGE: when validating the container write permissions the
type and subtype of the content need to be provided.

fixes #12684

docs/appendix/upgrade-notes/3.x-to-4.0.rst

@@ -113,6 +113,12 @@ The following files are no longer included during bootstrapping of a plugin:
 * ``views.php`` use ``elgg-plugin.php``
 * ``start.php`` use ``elgg-plugin.php`` and/or ``PluginBootstrap``
 
+Container permissions
+---------------------
+
+The function parameters for ``ElggEntity::canWriteToContainer()`` now require a ``$type`` and ``$subtype`` to be passed. This is to give more
+information to the resulting hook in order to be able to determine if a user is allowed write access to the container.
+
 Datamodel / Schema changes
 --------------------------
 
@@ -174,6 +180,7 @@ Class functions
 ~~~~~~~~~~~~~~~
 
 * ``Elgg\Http\ResponseBuilder::setStatusCode()`` no longer has a default value
+* ``ElggEntity::canWriteToContainer()`` no longer has a default value for ``$type`` and ``$subtype`` but these are required
 
 Lib functions
 ~~~~~~~~~~~~~

docs/design/database.rst

@@ -201,7 +201,7 @@ Adding content
 By passing along the group as ``container_guid`` via a hidden input field,
 you can use a single form and action to add both user and group content.
 
-Use ``ElggEntity->canWriteToContainer()`` to determine whether or not the current user has the right to
+Use ``ElggEntity->canWriteToContainer(0, $type, $subtype)`` to determine whether or not the current user has the right to
 add content to a group.
 
 Be aware that you will then need to pass the container GUID
@@ -219,7 +219,7 @@ your new element (defaulting to the current user's container):
     if ($container_guid) {
     	$container = get_entity($container_guid);
     	
-        if (!$container instanceof \ElggEntity || !$container->canWriteToContainer($user->guid)) {
+        if (!$container instanceof \ElggEntity || !$container->canWriteToContainer($user->guid, 'object', 'my_content_subtype')) {
             return elgg_error_response(elgg_echo('actionunauthorized'));
         }
     } else {

engine/classes/Elgg/UserCapabilities.php

@@ -244,13 +244,13 @@ public function canEditAnnotation(ElggEntity $entity, $user_guid = 0, ElggAnnota
 	 * Can a user add an entity to this container
 	 *
 	 * @param ElggEntity $entity    Container entity
-	 * @param int        $user_guid The GUID of the user creating the entity (0 for logged in user).
 	 * @param string     $type      The type of entity we're looking to write
 	 * @param string     $subtype   The subtype of the entity we're looking to write
+	 * @param int        $user_guid The GUID of the user creating the entity (0 for logged in user).
 	 *
 	 * @return bool
 	 */
-	public function canWriteToContainer(ElggEntity $entity, $user_guid = 0, $type = 'all', $subtype = 'all') {
+	public function canWriteToContainer(ElggEntity $entity, string $type, string $subtype, int $user_guid = 0) {
 		try {
 			$user = $this->entities->getUserForPermissionsCheck($user_guid);
 		} catch (UserFetchFailureException $e) {
@@ -284,13 +284,8 @@ public function canWriteToContainer(ElggEntity $entity, $user_guid = 0, $type =
 			return true;
 		}
 
-		$return = false;
-		if ($entity) {
-			// If the user can edit the container, they can also write to it
-			if ($entity->canEdit($user_guid)) {
-				$return = true;
-			}
-		}
+		// If the user can edit the container, they can also write to it
+		$return = $entity->canEdit($user_guid);
 
 		// Container permissions can prevent users from writing to an entity.
 		// For instance, these permissions can prevent non-group members from writing

engine/classes/ElggEntity.php

@@ -6,6 +6,7 @@
 use Elgg\Exceptions\InvalidParameterException;
 use Elgg\Exceptions\DatabaseException;
 use Elgg\Exceptions\Filesystem\IOException;
+use Elgg\Exceptions\InvalidArgumentException;
 
 /**
  * The parent class for all Elgg Entities.
@@ -1044,9 +1045,14 @@ public function canDelete($user_guid = 0) {
 	 * @param string $subtype   The subtype of the entity we're looking to write
 	 *
 	 * @return bool
+	 * @throws InvalidArgumentException
 	 */
-	public function canWriteToContainer($user_guid = 0, $type = 'all', $subtype = 'all') {
-		return _elgg_services()->userCapabilities->canWriteToContainer($this, $user_guid, $type, $subtype);
+	public function canWriteToContainer($user_guid = 0, $type = '', $subtype = '') {
+		if (empty($type) || empty($subtype)) {
+			throw new InvalidArgumentException(__METHOD__ . ' requires $type and $subtype to be set');
+		}
+
+		return _elgg_services()->userCapabilities->canWriteToContainer($this, $type, $subtype, $user_guid);
 	}
 
 	/**

engine/lib/navigation.php

@@ -162,7 +162,7 @@ function elgg_unregister_menu_item($menu_name, $item_name) {
  * @return void
  * @since 1.8.0
  */
-function elgg_register_title_button($handler = null, $name = 'add', $entity_type = 'all', $entity_subtype = 'all') {
+function elgg_register_title_button($handler = null, $name = 'add', $entity_type = '', $entity_subtype = '') {
 
 	$owner = elgg_get_page_owner_entity();
 	if (!$owner) {
@@ -180,7 +180,7 @@ function elgg_register_title_button($handler = null, $name = 'add', $entity_type
 	}
 
 	// do we have an owner and is the current user allowed to create content here
-	if (!$owner || !$owner->canWriteToContainer(0, $entity_type, $entity_subtype)) {
+	if (!$owner instanceof \ElggEntity || empty($entity_type) || empty($entity_subtype) || !$owner->canWriteToContainer(0, $entity_type, $entity_subtype)) {
 		return;
 	}
 

engine/tests/phpunit/integration/Elgg/Integration/ElggCoreRegressionBugsTest.php

@@ -53,7 +53,7 @@ function testCanWriteToContainer() {
 
 		// disable access overrides because we're admin.
 		elgg_call(ELGG_ENFORCE_ACCESS, function() use ($user, $object, $group) {
-			$this->assertFalse($object->canWriteToContainer($user->guid));
+			$this->assertFalse($object->canWriteToContainer($user->guid, 'object', 'foo'));
 
 			// register hook to allow access
 			$handler = function (\Elgg\Hook $hook) use ($user) {
@@ -63,13 +63,13 @@ function testCanWriteToContainer() {
 				}
 			};
 
-			elgg_register_plugin_hook_handler('container_permissions_check', 'all', $handler, 600);
-			$this->assertTrue($object->canWriteToContainer($user->guid));
-			elgg_unregister_plugin_hook_handler('container_permissions_check', 'all', $handler);
+			elgg_register_plugin_hook_handler('container_permissions_check', 'object', $handler, 600);
+			$this->assertTrue($object->canWriteToContainer($user->guid, 'object', 'foo'));
+			elgg_unregister_plugin_hook_handler('container_permissions_check', 'object', $handler);
 
-			$this->assertFalse($group->canWriteToContainer($user->guid));
+			$this->assertFalse($group->canWriteToContainer($user->guid, $object->getType(), $object->getSubtype()));
 			$group->join($user);
-			$this->assertTrue($group->canWriteToContainer($user->guid));
+			$this->assertTrue($group->canWriteToContainer($user->guid, $object->getType(), $object->getSubtype()));
 		});
 
 		$user->delete();

engine/tests/phpunit/unit/Elgg/UserCapabilitiesUnitTest.php

@@ -167,12 +167,12 @@ public function testCanWriteToContainerWhenCanEdit() {
 			'container_guid' => $container->guid,
 		]);
 
-		$this->assertTrue($entity->canWriteToContainer($owner->guid));
-		$this->assertTrue($entity->canWriteToContainer($container->guid));
+		$this->assertTrue($entity->canWriteToContainer($owner->guid, 'object', 'foo'));
+		$this->assertTrue($entity->canWriteToContainer($container->guid, 'object', 'foo'));
 		$this->assertEquals($entity->canEdit($owner->guid), $entity->canDelete($owner->guid));
 		$this->assertEquals($entity->canEdit($container->guid), $entity->canDelete($container->guid));
 
-		$this->assertFalse($entity->canWriteToContainer($viewer->guid));
+		$this->assertFalse($entity->canWriteToContainer($viewer->guid, 'object', 'foo'));
 		$this->assertEquals($entity->canEdit($viewer->guid), $entity->canDelete($viewer->guid));
 	}
 
@@ -186,26 +186,33 @@ public function testCanOverrideContainerPermissionsWithAHook() {
 
 		$this->assertTrue($entity->canWriteToContainer($owner->guid, 'object', 'bar'));
 
+		// only prevent the write access for 'object', 'bar'
 		$this->hooks->registerHandler('container_permissions_check', 'object', function(\Elgg\Hook $hook) use ($entity, $owner) {
+			$this->assertNotEmpty($hook->getParam('subtype'));
+			if ($hook->getParam('subtype') !== 'bar') {
+				return;
+			}
+
 			$this->assertInstanceOf(ElggEntity::class, $hook->getParam('container'));
 			$this->assertInstanceOf(ElggUser::class, $hook->getUserParam());
 			$this->assertEquals($entity, $hook->getParam('container'));
 			$this->assertEquals($owner, $hook->getUserParam());
-			$this->assertEquals('bar', $hook->getParam('subtype'));
 			$this->assertTrue($hook->getValue());
 			return false;
 		});
 
 		$this->assertFalse($entity->canWriteToContainer($owner->guid, 'object', 'bar'));
 
-		// Should still be able to write to container without particular entity type specified
-		$this->assertTrue($entity->canWriteToContainer($owner->guid));
+		// Should still be able to write to container with a different type/subtype
+		$this->assertTrue($entity->canWriteToContainer($owner->guid, 'object', 'foo'));
 
+		// Admins should always be allowed
 		$admin_user = $this->createUser([], [
 			'admin' => 'yes',
 		]);
 		$this->assertTrue($entity->canWriteToContainer($admin_user->guid, 'object', 'bar'));
 
+		// when access is ignored it should also be allowed
 		elgg_call(ELGG_IGNORE_ACCESS, function() use ($entity, $owner) {
 			$this->assertTrue($entity->canWriteToContainer($owner->guid, 'object', 'bar'));
 		});

mod/groups/actions/groups/edit.php

@@ -60,7 +60,7 @@
 	$container_guid = get_input('container_guid', $user->guid);
 	$container = get_entity($container_guid);
 
-	if (!$container || !$container->canWriteToContainer($user->guid, 'group')) {
+	if (!$container || !$container->canWriteToContainer($user->guid, 'group', 'group')) {
 		$error = elgg_echo('groups:cantcreate');
 		return elgg_error_response($error);
 	}

mod/groups/classes/Elgg/Groups/Access.php

@@ -98,7 +98,7 @@ public static function getWriteAccess(\Elgg\Hook $hook) {
 			return;
 		}
 
-		if (!$page_owner->canWriteToContainer($user_guid)) {
+		if (!$page_owner->isMember($user) && !$page_owner->canEdit($user_guid)) {
 			return;
 		}
 
@@ -142,7 +142,7 @@ public static function getWriteAccess(\Elgg\Hook $hook) {
 	/**
 	 * Return the write access for the current group if the user has write access to it
 	 *
-	 * @param \Elgg\Hook $hook 'access_collection:display_name' 'access_collection'
+	 * @param \Elgg\Hook $hook 'access_collection:name' 'access_collection'
 	 * @return void|string
 	 */
 	public static function getAccessCollectionName(\Elgg\Hook $hook) {
@@ -159,11 +159,16 @@ public static function getAccessCollectionName(\Elgg\Hook $hook) {
 
 		$page_owner = elgg_get_page_owner_entity();
 
-		if ($page_owner && $page_owner->guid == $owner->guid) {
+		if ($page_owner && $page_owner->guid === $owner->guid) {
 			return elgg_echo('groups:acl:in_context');
 		}
 
-		if ($owner->canWriteToContainer()) {
+		$user = elgg_get_logged_in_user_entity();
+		if (empty($user)) {
+			return;
+		}
+
+		if ($owner->isMember($user) || $owner->canEdit($user->guid)) {
 			return elgg_echo('groups:acl', [$owner->getDisplayName()]);
 		}
 	}

mod/groups/views/default/groups/profile/module.php

@@ -86,11 +86,8 @@
 	}
 }
 
-if (!empty($add_link)) {
-	$check_type = $entity_type ?: 'all';
-	$check_subtype = $entity_subtype ?: 'all';
-
-	if ($group->canWriteToContainer(0, $check_type, $check_subtype)) {
+if (!empty($add_link) && !empty($entity_type) && !empty($entity_subtype)) {
+	if ($group->canWriteToContainer(0, $entity_type, $entity_subtype)) {
 		$footer = elgg_format_element('span', [
 			'class' => 'elgg-widget-more',
 		], $add_link);