From a5f59fc63d47255049150de89dcc34d522204590 Mon Sep 17 00:00:00 2001 From: colemanw Date: Sun, 10 Sep 2023 13:57:12 -0400 Subject: [PATCH] APIv4 - Enforce ACLs for File entity, dedupe ACL clauses for bridge entities --- CRM/Core/BAO/File.php | 18 ++++ CRM/Core/DAO.php | 33 +++--- CRM/Core/Permission.php | 1 + Civi/Api4/Query/Api4SelectQuery.php | 26 +++-- .../phpunit/api/v4/Action/EntityFileTest.php | 101 ++++++++++++++++++ 5 files changed, 155 insertions(+), 24 deletions(-) create mode 100644 tests/phpunit/api/v4/Action/EntityFileTest.php diff --git a/CRM/Core/BAO/File.php b/CRM/Core/BAO/File.php index 3db9618489..0837317f1e 100644 --- a/CRM/Core/BAO/File.php +++ b/CRM/Core/BAO/File.php @@ -834,6 +834,24 @@ HEREDOC; return FALSE; } + /** + * @inheritDoc + */ + public function addSelectWhereClause() { + // We always return an array with these keys, even if they are empty, + // because this tells the query builder that we have considered these fields for acls + $clauses = [ + 'id' => [], + ]; + // File ACLs are driven by the EntityFile table + $entityFileClause = CRM_Core_DAO_EntityFile::getDynamicFkAclClauses(); + if ($entityFileClause) { + $clauses['id'] = 'IN (SELECT file_id FROM `civicrm_entity_file` WHERE (' . implode(') OR (', $entityFileClause) . '))'; + } + CRM_Utils_Hook::selectWhereClause($this, $clauses); + return $clauses; + } + /** * FIXME: Incomplete pseudoconstant for EntityFile.entity_table * diff --git a/CRM/Core/DAO.php b/CRM/Core/DAO.php index 61f9ced88e..3faae6f723 100644 --- a/CRM/Core/DAO.php +++ b/CRM/Core/DAO.php @@ -3083,20 +3083,7 @@ SELECT contact_id } // Clause for an entity_table/entity_id combo if ($field['name'] === 'entity_id' && isset($fields['entity_table'])) { - $relatedClauses = []; - $relatedEntities = $this->buildOptions('entity_table', 'get'); - foreach ((array) $relatedEntities as $table => $ent) { - if (!empty($ent)) { - $ent = CRM_Core_DAO_AllCoreTables::getEntityNameForTable($table); - $subquery = CRM_Utils_SQL::mergeSubquery($ent); - if ($subquery) { - $relatedClauses[] = "(entity_table = '$table' AND entity_id " . implode(' AND entity_id ', $subquery) . ")"; - } - else { - $relatedClauses[] = "(entity_table = '$table')"; - } - } - } + $relatedClauses = self::getDynamicFkAclClauses(); if ($relatedClauses) { $clauses['id'] = 'IN (SELECT id FROM `' . $this->tableName() . '` WHERE (' . implode(') OR (', $relatedClauses) . '))'; } @@ -3106,6 +3093,24 @@ SELECT contact_id return $clauses; } + protected static function getDynamicFkAclClauses(): array { + $relatedClauses = []; + $relatedEntities = static::buildOptions('entity_table', 'get'); + foreach ((array) $relatedEntities as $table => $ent) { + if (!empty($ent)) { + $ent = CRM_Core_DAO_AllCoreTables::getEntityNameForTable($table); + $subquery = CRM_Utils_SQL::mergeSubquery($ent); + if ($subquery) { + $relatedClauses[] = "(entity_table = '$table' AND entity_id " . implode(' AND entity_id ', $subquery) . ")"; + } + else { + $relatedClauses[] = "(entity_table = '$table')"; + } + } + } + return $relatedClauses; + } + /** * This returns the final permissioned query string for this entity * diff --git a/CRM/Core/Permission.php b/CRM/Core/Permission.php index 2fff4fc954..f8f2fb7ace 100644 --- a/CRM/Core/Permission.php +++ b/CRM/Core/Permission.php @@ -1259,6 +1259,7 @@ class CRM_Core_Permission { ], ]; $permissions['files_by_entity'] = $permissions['file']; + $permissions['entity_file'] = $permissions['file']; // Group permissions $permissions['group'] = [ diff --git a/Civi/Api4/Query/Api4SelectQuery.php b/Civi/Api4/Query/Api4SelectQuery.php index 614dda8a8b..2b5cf1c0a1 100644 --- a/Civi/Api4/Query/Api4SelectQuery.php +++ b/Civi/Api4/Query/Api4SelectQuery.php @@ -493,9 +493,9 @@ class Api4SelectQuery extends Api4Query { // getAclClause() expects a stack of 1-to-1 join fields to help it dedupe, but this is more flexible, // so unless this is a direct 1-to-1 join with the main entity, we'll just hack it // with a padded empty stack to bypass its deduping. - $stack = [NULL, NULL]; + $aclStack = [NULL, NULL]; // See if the ON clause already contains an FK reference to joinEntity - $explicitFK = array_filter($joinTree, function($clause) use ($alias, $joinEntityFields, &$stack) { + $explicitFK = array_filter($joinTree, function($clause) use ($alias, $joinEntityFields, &$aclStack) { [$sideA, $op, $sideB] = array_pad((array) $clause, 3, NULL); if ($op !== '=' || !$sideB) { return FALSE; @@ -514,7 +514,7 @@ class Api4SelectQuery extends Api4Query { $joinField === 'entity_id' || !empty($joinEntityFields[$joinField]['fk_entity'])) { // If the join links to a field on the main entity, ACL clauses can be deduped if (preg_match('/^[_a-z0-9]+$/i', $clause[$otherSide])) { - $stack = [$clause[$otherSide]]; + $aclStack = [$clause[$otherSide]]; } return TRUE; } @@ -533,17 +533,17 @@ class Api4SelectQuery extends Api4Query { } elseif (strpos($name, "$alias.") === 0 && substr_count($name, '.') === 1 && $field['fk_entity'] === $this->getEntity()) { $conditions[] = $this->treeWalkClauses([$name, '=', 'id'], 'ON'); - $stack = ['id']; + $aclStack = ['id']; } } // Hmm, if we came up with > 1 condition, then it's ambiguous how it should be joined so we won't return anything but the generic ACLs if (count($conditions) > 1) { - $stack = [NULL, NULL]; + $aclStack = [NULL, NULL]; $conditions = []; } } $baoName = CoreUtil::getBAOFromApiName($joinEntity); - $acls = array_values($this->getAclClause($alias, $baoName, $stack)); + $acls = array_values($this->getAclClause($alias, $baoName, $aclStack)); return array_merge($acls, $conditions); } @@ -568,11 +568,14 @@ class Api4SelectQuery extends Api4Query { $this->registerBridgeJoinFields($bridgeEntity, $joinRef, $baseRef, $alias, $bridgeAlias); + // Used to dedupe acl clauses + $aclStack = [NULL, NULL]; + $linkConditions = $this->getBridgeLinkConditions($bridgeAlias, $alias, $joinTable, $joinRef); - $bridgeConditions = $this->getBridgeJoinConditions($joinTree, $baseRef, $alias, $bridgeAlias, $bridgeEntity); + $bridgeConditions = $this->getBridgeJoinConditions($joinTree, $baseRef, $alias, $bridgeAlias, $bridgeEntity, $aclStack); - $acls = array_values($this->getAclClause($alias, CoreUtil::getBAOFromApiName($joinEntity), [NULL, NULL])); + $acls = array_values($this->getAclClause($alias, CoreUtil::getBAOFromApiName($joinEntity), $aclStack)); $outerConditions = []; foreach (array_filter($joinTree) as $clause) { @@ -676,13 +679,14 @@ class Api4SelectQuery extends Api4Query { * @param string $alias * @param string $bridgeAlias * @param string $bridgeEntity + * @param array $aclStack * @return string[] * @throws \CRM_Core_Exception */ - private function getBridgeJoinConditions(array &$joinTree, $baseRef, string $alias, string $bridgeAlias, string $bridgeEntity): array { + private function getBridgeJoinConditions(array &$joinTree, $baseRef, string $alias, string $bridgeAlias, string $bridgeEntity, array &$aclStack): array { $bridgeConditions = []; // Find explicit bridge join conditions and move them out of the joinTree - $joinTree = array_filter($joinTree, function ($clause) use ($baseRef, $alias, $bridgeAlias, &$bridgeConditions) { + $joinTree = array_filter($joinTree, function ($clause) use ($baseRef, $alias, $bridgeAlias, &$bridgeConditions, &$aclStack) { [$sideA, $op, $sideB] = array_pad((array) $clause, 3, NULL); // Skip AND/OR/NOT branches if (!$sideB) { @@ -692,6 +696,7 @@ class Api4SelectQuery extends Api4Query { if ($op === '=' && $sideB && ($sideA === "$alias.{$baseRef->getReferenceKey()}" || $sideB === "$alias.{$baseRef->getReferenceKey()}")) { $expr = $sideA === "$alias.{$baseRef->getReferenceKey()}" ? $sideB : $sideA; $bridgeConditions[] = "`$bridgeAlias`.`{$baseRef->getReferenceKey()}` = " . $this->getExpression($expr)->render($this); + $aclStack = [$expr]; return FALSE; } // Explicit link with dynamic "entity_table" column @@ -708,6 +713,7 @@ class Api4SelectQuery extends Api4Query { throw new \CRM_Core_Exception("Unable to join $bridgeEntity to " . $this->getEntity()); } $bridgeConditions[] = "`$bridgeAlias`.`{$baseRef->getReferenceKey()}` = a.`{$baseRef->getTargetKey()}`"; + $aclStack = [$baseRef->getTargetKey()]; if ($baseRef->getTypeColumn()) { $bridgeConditions[] = "`$bridgeAlias`.`{$baseRef->getTypeColumn()}` = '" . $this->getFrom() . "'"; } diff --git a/tests/phpunit/api/v4/Action/EntityFileTest.php b/tests/phpunit/api/v4/Action/EntityFileTest.php new file mode 100644 index 0000000000..54777e9802 --- /dev/null +++ b/tests/phpunit/api/v4/Action/EntityFileTest.php @@ -0,0 +1,101 @@ +saveTestRecords('Contact', ['records' => 4]) + ->column('id'); + $note = $this->saveTestRecords('Note', [ + 'records' => [ + ['entity_id' => $cid[0], 'note' => '0test'], + ['entity_id' => $cid[1], 'note' => '1test'], + ['entity_id' => $cid[2], 'note' => '2test'], + ['entity_id' => $cid[3], 'note' => '3test'], + ], + 'defaults' => ['entity_table' => 'civicrm_contact'], + ])->column('id'); + + $file = []; + + // FIXME: Use api4 when available + foreach ($note as $nid) { + $file[] = civicrm_api3('Attachment', 'create', [ + 'entity_table' => 'civicrm_note', + 'entity_id' => $nid, + 'name' => 'file_for_' . $nid . '.txt', + 'mime_type' => 'text/plain', + 'content' => 'hello', + ])['id']; + } + + // Grant access to contact 2 & 3, deny to 0 & 1 + $this->allowedContacts = array_slice($cid, 2); + \CRM_Core_Config::singleton()->userPermissionClass->permissions = ['access CiviCRM', 'access uploaded files', 'view debug output']; + \CRM_Utils_Hook::singleton()->setHook('civicrm_aclWhereClause', [$this, 'aclWhereMultipleContacts']); + + $allowedEntityFiles = EntityFile::get() + ->addWhere('entity_table', '=', 'civicrm_note') + ->addWhere('entity_id', 'IN', $note) + ->setDebug(TRUE) + ->execute(); + // ACL clause should have been inserted + $this->assertStringContainsString('civicrm_acl_contact_cache', $allowedEntityFiles->debug['sql'][0]); + // Results should have been filtered by allowed contacts + $this->assertCount(2, $allowedEntityFiles); + + $allowedFiles = File::get() + ->addWhere('id', 'IN', $file) + ->setDebug(TRUE) + ->execute(); + // ACL clause should have been inserted + $this->assertStringContainsString('civicrm_acl_contact_cache', $allowedFiles->debug['sql'][0]); + // Results should have been filtered by allowed contacts + $this->assertCount(2, $allowedFiles); + + $allowedNotes = Note::get() + ->addJoin('File AS file', 'LEFT', 'EntityFile', ['file.entity_id', '=', 'id'], ['file.entity_table', '=', '"civicrm_note"']) + ->addSelect('file.file_name', 'file.url', 'note', 'id') + ->setDebug(TRUE) + ->execute()->indexBy('id'); + // ACL clause should have been inserted + $this->assertStringContainsString('civicrm_acl_contact_cache', $allowedNotes->debug['sql'][0]); + // Results should have been filtered by allowed contacts + $this->assertCount(2, $allowedNotes); + $this->assertEquals('file_for_' . $note[2] . '.txt', $allowedNotes[$note[2]]['file.file_name']); + $this->assertEquals('file_for_' . $note[3] . '.txt', $allowedNotes[$note[3]]['file.file_name']); + $this->assertStringContainsString("id=$file[2]&eid=$note[2]&fcs=", $allowedNotes[$note[2]]['file.url']); + $this->assertStringContainsString("id=$file[3]&eid=$note[3]&fcs=", $allowedNotes[$note[3]]['file.url']); + } + +} -- 2.25.1