From 918e583a146876fb42932a1d959796fd710b7b74 Mon Sep 17 00:00:00 2001
From: colemanw <coleman@civicrm.org>
Date: Sun, 17 Sep 2023 18:11:03 -0400
Subject: [PATCH] API/DAO - Improve permissioning sql clauses: support field
names and OR
Before: selectWhereClause was limited to returning arrays of clauses which would be
joined with AND. They could not reference any other fields on the entity.
After: selectWhereClause can return sub-arrays which are joined with OR,
and can reference any field on the entity using {curly_brace} syntax.
Stricter type checking emits noisy deprecations if arrays are expected and strings are given.
---
CRM/Contact/BAO/Query.php | 4 +-
CRM/Core/BAO/Note.php | 10 +++
CRM/Core/DAO.php | 81 ++++++++++++-------
CRM/Utils/SQL.php | 57 ++++++++++---
ext/financialacls/financialacls.php | 8 +-
.../CRM/OAuth/BAO/OAuthContactToken.php | 4 +-
tests/phpunit/CRM/Utils/SQLTest.php | 15 ++++
7 files changed, 130 insertions(+), 49 deletions(-)
diff --git a/CRM/Contact/BAO/Query.php b/CRM/Contact/BAO/Query.php
index 9f015588926..23499c9219b 100644
--- a/CRM/Contact/BAO/Query.php
+++ b/CRM/Contact/BAO/Query.php
@@ -5082,7 +5082,9 @@ civicrm_relationship.start_date > {$today}
$clauses = $subclauses = [];
foreach ($bao->addSelectWhereClause() as $field => $vals) {
if ($vals && $field !== 'id') {
- $clauses[] = $bao->tableName() . ".$field " . $vals;
+ foreach ($vals as $val) {
+ $clauses[] = $bao->tableName() . ".$field " . $val;
+ }
}
elseif ($vals) {
$subclauses[] = "$field " . implode(" AND $field ", (array) $vals);
diff --git a/CRM/Core/BAO/Note.php b/CRM/Core/BAO/Note.php
index 8df7525a989..d84e9b320f1 100644
--- a/CRM/Core/BAO/Note.php
+++ b/CRM/Core/BAO/Note.php
@@ -526,4 +526,14 @@ WHERE participant.contact_id = %1 AND note.entity_table = 'civicrm_participant'
}
}
+ public function addSelectWhereClause(): array {
+ $clauses = [];
+ $relatedClauses = self::getDynamicFkAclClauses('entity_table', 'entity_id');
+ if ($relatedClauses) {
+ // Nested array will be joined with OR
+ $clauses['entity_table'] = [$relatedClauses];
+ }
+ return $clauses;
+ }
+
}
diff --git a/CRM/Core/DAO.php b/CRM/Core/DAO.php
index fd0b562fd60..7568c03f6e6 100644
--- a/CRM/Core/DAO.php
+++ b/CRM/Core/DAO.php
@@ -3059,33 +3059,38 @@ SELECT contact_id
*
* Return format is in the form of fieldname => clauses starting with an operator. e.g.:
* ```
- * array(
- * 'location_type_id' => array('IS NOT NULL', 'IN (1,2,3)')
- * )
+ * [
+ * // Each string in the array will get joined with AND
+ * 'location_type_id' => ['IS NOT NULL', 'IN (1,2,3)'],
+ * // Each sub-array in the array will get joined with OR, field names must be enclosed in curly braces
+ * 'privacy' => [
+ * ['= 0', '= 1 AND {contact_id} = 456'],
+ * ],
+ * ]
* ```
*
* Note that all array keys must be actual field names in this entity. Use subqueries to filter on other tables e.g. custom values.
+ * The query strings MAY reference other fields in this entity; but they must be enclosed in {curly_braces}.
*
* @return array
*/
public function addSelectWhereClause(): array {
$clauses = [];
- $fields = $this->fields();
- // Notes should check permissions on the entity_id field, not the contact_id field
- $skipContactCheckFor = ['Note'];
+ $fields = $this::getSupportedFields();
foreach ($fields as $fieldName => $field) {
// Clause for contact-related entities like Email, Relationship, etc.
- if (strpos($field['name'], 'contact_id') === 0 && !in_array($field['entity'], $skipContactCheckFor) && ($field['FKClassName'] ?? NULL) == 'CRM_Contact_DAO_Contact') {
+ if (str_starts_with($fieldName, 'contact_id') && ($field['FKClassName'] ?? NULL) === 'CRM_Contact_DAO_Contact') {
$contactClause = CRM_Utils_SQL::mergeSubquery('Contact');
if (!empty($contactClause)) {
- $clauses[$field['name']] = $contactClause;
+ $clauses[$fieldName] = $contactClause;
}
}
// Clause for an entity_table/entity_id combo
- if ($field['name'] === 'entity_id' && isset($fields['entity_table'])) {
- $relatedClauses = self::getDynamicFkAclClauses();
+ if ($fieldName === 'entity_id' && isset($fields['entity_table'])) {
+ $relatedClauses = self::getDynamicFkAclClauses('entity_table', 'entity_id');
if ($relatedClauses) {
- $clauses['id'] = 'IN (SELECT id FROM `' . $this->tableName() . '` WHERE (' . implode(') OR (', $relatedClauses) . '))';
+ // Nested array will be joined with OR
+ $clauses['entity_table'] = [$relatedClauses];
}
}
}
@@ -3093,20 +3098,19 @@ SELECT contact_id
return $clauses;
}
- protected static function getDynamicFkAclClauses(): array {
+ protected static function getDynamicFkAclClauses($entityTableField, $entityIdField): array {
$relatedClauses = [];
- $relatedEntities = static::buildOptions('entity_table', 'get');
+ $relatedEntities = static::buildOptions($entityTableField, 'get');
foreach ((array) $relatedEntities as $table => $ent) {
- // Prevent recursion
- if (!empty($ent) && $table !== static::getTableName()) {
- $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')";
- }
+ // Ensure $ent is the machine name of the entity not a translated title
+ $ent = CRM_Core_DAO_AllCoreTables::getEntityNameForTable($table);
+ // Prevent infinite recursion
+ $subquery = $table === static::getTableName() ? NULL : CRM_Utils_SQL::mergeSubquery($ent);
+ if ($subquery) {
+ $relatedClauses[] = "= '$table' AND {{$entityIdField}} " . implode(" AND {{$entityIdField}} ", $subquery);
+ }
+ else {
+ $relatedClauses[] = "= '$table'";
}
}
return $relatedClauses;
@@ -3125,17 +3129,32 @@ SELECT contact_id
if ($tableAlias === NULL) {
$tableAlias = $bao->tableName();
}
- $clauses = [];
- foreach ((array) $bao->addSelectWhereClause() as $field => $vals) {
- $clauses[$field] = NULL;
- if ($vals) {
- $clauses[$field] = "(`$tableAlias`.`$field` " . implode(" AND `$tableAlias`.`$field` ", (array) $vals) . ')';
- if (empty(static::getSupportedFields()[$field]['required'])) {
- $clauses[$field] = "(`$tableAlias`.`$field` IS NULL OR {$clauses[$field]})";
+ $finalClauses = [];
+ $fields = static::getSupportedFields();
+ foreach ((array) $bao->addSelectWhereClause() as $fieldName => $fieldClauses) {
+ $finalClauses[$fieldName] = NULL;
+ if ($fieldClauses) {
+ if (!is_array($fieldClauses)) {
+ CRM_Core_Error::deprecatedWarning('Expected array of selectWhereClauses for ' . $bao->tableName() . '.' . $fieldName . ', instead got ' . json_encode($fieldClauses));
+ $fieldClauses = (array) $fieldClauses;
+ }
+ $formattedClauses = [];
+ foreach (CRM_Utils_SQL::prefixFieldNames($fieldClauses, array_keys($fields), $tableAlias) as $subClause) {
+ // Arrays of arrays get joined with OR (similar to CRM_Core_Permission::check)
+ if (is_array($subClause)) {
+ $formattedClauses[] = "(`$tableAlias`.`$fieldName` " . implode(" OR `$tableAlias`.`$fieldName` ", $subClause) . ')';
+ }
+ else {
+ $formattedClauses[] = "(`$tableAlias`.`$fieldName` " . $subClause . ')';
+ }
+ }
+ $finalClauses[$fieldName] = '(' . implode(' AND ', $formattedClauses) . ')';
+ if (empty($fields[$fieldName]['required'])) {
+ $finalClauses[$fieldName] = "(`$tableAlias`.`$fieldName` IS NULL OR {$finalClauses[$fieldName]})";
}
}
}
- return $clauses;
+ return $finalClauses;
}
/**
diff --git a/CRM/Utils/SQL.php b/CRM/Utils/SQL.php
index 981d7cb6e55..0c50dd795ad 100644
--- a/CRM/Utils/SQL.php
+++ b/CRM/Utils/SQL.php
@@ -57,22 +57,57 @@ class CRM_Utils_SQL {
* @return array
*/
public static function mergeSubquery($entity, $joinColumn = 'id') {
- require_once 'api/v3/utils.php';
- $baoName = _civicrm_api3_get_BAO($entity);
+ $baoName = CRM_Core_DAO_AllCoreTables::getBAOClassName(CRM_Core_DAO_AllCoreTables::getFullName($entity));
$bao = new $baoName();
- $clauses = $subclauses = [];
- foreach ((array) $bao->addSelectWhereClause() as $field => $vals) {
- if ($vals && $field == $joinColumn) {
- $clauses = array_merge($clauses, (array) $vals);
+ $fields = $bao::getSupportedFields();
+ $mergeClauses = $subClauses = [];
+ foreach ((array) $bao->addSelectWhereClause() as $fieldName => $fieldClauses) {
+ if ($fieldClauses) {
+ foreach ((array) $fieldClauses as $fieldClause) {
+ $formattedClause = CRM_Utils_SQL::prefixFieldNames($fieldClause, array_keys($fields), $bao->tableName());
+ // Same as join column with no additional fields - can be added directly
+ if ($fieldName === $joinColumn && $fieldClause === $formattedClause) {
+ $mergeClauses[] = $formattedClause;
+ }
+ // Arrays of arrays get joined with OR (similar to CRM_Core_Permission::check)
+ elseif (is_array($formattedClause)) {
+ $subClauses[] = "($fieldName " . implode(" OR $fieldName ", $formattedClause) . ')';
+ }
+ else {
+ $subClauses[] = "$fieldName $formattedClause";
+ }
+ }
}
- elseif ($vals) {
- $subclauses[] = "$field " . implode(" AND $field ", (array) $vals);
+ }
+ if ($subClauses) {
+ $mergeClauses[] = "IN (SELECT `$joinColumn` FROM `" . $bao->tableName() . "` WHERE " . implode(' AND ', $subClauses) . ")";
+ }
+ return $mergeClauses;
+ }
+
+ /**
+ * Walk a nested array and replace "{field_name}" with "`tableAlias`.`field_name`"
+ *
+ * @param string|array $clause
+ * @param array $fieldNames
+ * @param string $tableAlias
+ * @return string|array
+ */
+ public static function prefixFieldNames(&$clause, array $fieldNames, string $tableAlias) {
+ if (is_array($clause)) {
+ foreach ($clause as $index => $subclause) {
+ $clause[$index] = self::prefixFieldNames($subclause, $fieldNames, $tableAlias);
}
}
- if ($subclauses) {
- $clauses[] = "IN (SELECT `$joinColumn` FROM `" . $bao->tableName() . "` WHERE " . implode(' AND ', $subclauses) . ")";
+ if (is_string($clause) && str_contains($clause, '{')) {
+ $find = $replace = [];
+ foreach ($fieldNames as $fieldName) {
+ $find[] = '{' . $fieldName . '}';
+ $replace[] = '`' . $tableAlias . '`.`' . $fieldName . '`';
+ }
+ $clause = str_replace($find, $replace, $clause);
}
- return $clauses;
+ return $clause;
}
/**
diff --git a/ext/financialacls/financialacls.php b/ext/financialacls/financialacls.php
index 0e079fe4010..48b93922b32 100644
--- a/ext/financialacls/financialacls.php
+++ b/ext/financialacls/financialacls.php
@@ -93,19 +93,19 @@ function financialacls_civicrm_selectWhereClause($entity, &$clauses) {
case 'MembershipType':
case 'ContributionRecur':
case 'Contribution':
- $clauses['financial_type_id'] = _financialacls_civicrm_get_type_clause();
+ $clauses['financial_type_id'][] = _financialacls_civicrm_get_type_clause();
break;
case 'Membership':
- $clauses['membership_type_id'] = _financialacls_civicrm_get_membership_type_clause();
+ $clauses['membership_type_id'][] = _financialacls_civicrm_get_membership_type_clause();
break;
case 'FinancialType':
- $clauses['id'] = _financialacls_civicrm_get_type_clause();
+ $clauses['id'][] = _financialacls_civicrm_get_type_clause();
break;
case 'FinancialAccount':
- $clauses['id'] = _financialacls_civicrm_get_accounts_clause();
+ $clauses['id'][] = _financialacls_civicrm_get_accounts_clause();
break;
}
diff --git a/ext/oauth-client/CRM/OAuth/BAO/OAuthContactToken.php b/ext/oauth-client/CRM/OAuth/BAO/OAuthContactToken.php
index 6fbe616ac61..9786a68c1b3 100644
--- a/ext/oauth-client/CRM/OAuth/BAO/OAuthContactToken.php
+++ b/ext/oauth-client/CRM/OAuth/BAO/OAuthContactToken.php
@@ -109,11 +109,11 @@ class CRM_OAuth_BAO_OAuthContactToken extends CRM_OAuth_DAO_OAuthContactToken {
}
// With 'manage my' permission, limit to just the current user
elseif ($loggedInContactID && CRM_Core_Permission::check(['manage my OAuth contact tokens'])) {
- $clauses['contact_id'] = "= $loggedInContactID";
+ $clauses['contact_id'][] = "= $loggedInContactID";
}
// No permission, return nothing
else {
- $clauses['contact_id'] = "= -1";
+ $clauses['contact_id'][] = "= -1";
}
CRM_Utils_Hook::selectWhereClause($this, $clauses);
return $clauses;
diff --git a/tests/phpunit/CRM/Utils/SQLTest.php b/tests/phpunit/CRM/Utils/SQLTest.php
index 80d336fce7e..e37833e333f 100644
--- a/tests/phpunit/CRM/Utils/SQLTest.php
+++ b/tests/phpunit/CRM/Utils/SQLTest.php
@@ -37,6 +37,21 @@ class CRM_Utils_SQLTest extends CiviUnitTestCase {
}
}
+ public function testPrefixFieldNames(): void {
+ $exampleFieldNames = ['one', 'two', 'three'];
+ $tableAlias = 'foo';
+ $clause = [
+ '{one} = 1',
+ ['{two} = {three}', '`{threee}` IN ({one}, "{twothree}")'],
+ ];
+ $expected = [
+ '`foo`.`one` = 1',
+ ['`foo`.`two` = `foo`.`three`', '`{threee}` IN (`foo`.`one`, "{twothree}")'],
+ ];
+ CRM_Utils_SQL::prefixFieldNames($clause, $exampleFieldNames, $tableAlias);
+ $this->assertEquals($expected, $clause);
+ }
+
/**
* Test isSSLDSN
* @dataProvider dsnProvider
--
GitLab