From b53bcc5dbe34d29a8ab4c43ea38311f4c7fec40f Mon Sep 17 00:00:00 2001 From: Coleman Watts Date: Wed, 13 Jan 2016 16:14:30 -0500 Subject: [PATCH] CRM-17795 - Prevent (most) redundant acl sub clauses --- CRM/Contact/BAO/Contact.php | 6 +++-- CRM/Contact/BAO/Contact/Permission.php | 16 +++++------ CRM/Contact/BAO/Relationship.php | 14 ++++------ CRM/Core/BAO/UFMatch.php | 4 +-- CRM/Core/DAO.php | 22 ++++++++++----- Civi/API/SelectQuery.php | 37 +++++++++++++++++++++----- api/v3/Case.php | 2 +- 7 files changed, 66 insertions(+), 35 deletions(-) diff --git a/CRM/Contact/BAO/Contact.php b/CRM/Contact/BAO/Contact.php index 3bceb0df77..45d7c01fd4 100644 --- a/CRM/Contact/BAO/Contact.php +++ b/CRM/Contact/BAO/Contact.php @@ -3428,8 +3428,10 @@ LEFT JOIN civicrm_address add2 ON ( add1.master_id = add2.id ) /** * @inheritDoc */ - public function apiWhereClause($tableAlias) { - return CRM_Contact_BAO_Contact_Permission::cacheSubquery("`$tableAlias`.id"); + public function apiWhereClause() { + return array( + 'id' => CRM_Contact_BAO_Contact_Permission::cacheSubquery(), + ); } } diff --git a/CRM/Contact/BAO/Contact/Permission.php b/CRM/Contact/BAO/Contact/Permission.php index 5d63c51a56..72b53585ca 100644 --- a/CRM/Contact/BAO/Contact/Permission.php +++ b/CRM/Contact/BAO/Contact/Permission.php @@ -225,23 +225,23 @@ AND $operationClause LIMIT 1"; } /** - * Generate acl subquery that can be placed in the WHERE clause of a query or the ON clause of a JOIN + * Generate acl subqueries that can be placed in the WHERE clause of a query or the ON clause of a JOIN * - * @param string $contactIdField - * Full "table_name.field_name" for the field containing a contact id - * @return string|NULL + * Returns an array of clauses that can each be placed after the name of the contact_id field in the query. + * + * @return array */ - public static function cacheSubquery($contactIdField) { + public static function cacheSubquery() { $clauses = array(); if (!CRM_Core_Permission::check(array(array('view all contacts', 'edit all contacts')))) { $contactID = (int) CRM_Core_Session::getLoggedInContactID(); self::cache($contactID); - $clauses[] = "$contactIdField IN (SELECT contact_id FROM civicrm_acl_contact_cache WHERE user_id = $contactID)"; + $clauses[] = "IN (SELECT contact_id FROM civicrm_acl_contact_cache WHERE user_id = $contactID)"; } if (!CRM_Core_Permission::check('access deleted contacts')) { - $clauses[] = "$contactIdField NOT IN (SELECT id FROM civicrm_contact WHERE is_deleted = 1)"; + $clauses[] = "NOT IN (SELECT id FROM civicrm_contact WHERE is_deleted = 1)"; } - return $clauses ? implode(' AND ', $clauses) : NULL; + return $clauses; } /** diff --git a/CRM/Contact/BAO/Relationship.php b/CRM/Contact/BAO/Relationship.php index 9b2319031a..0d0dad70a9 100644 --- a/CRM/Contact/BAO/Relationship.php +++ b/CRM/Contact/BAO/Relationship.php @@ -2104,16 +2104,12 @@ AND cc.sort_name LIKE '%$name%'"; /** * @inheritDoc */ - public function apiWhereClause($tableAlias) { + public function apiWhereClause() { // Generate an acl clause for both contacts in the relationship - $clauses = array(); - foreach (array('a', 'b') as $a) { - $clause = CRM_Contact_BAO_Contact_Permission::cacheSubquery("`$tableAlias`.contact_id_$a"); - if ($clause !== NULL) { - $clauses[] = $clause; - } - } - return $clauses ? implode(' AND ', $clauses) : NULL; + return array( + 'contact_id_a' => CRM_Contact_BAO_Contact_Permission::cacheSubquery(), + 'contact_id_b' => CRM_Contact_BAO_Contact_Permission::cacheSubquery(), + ); } } diff --git a/CRM/Core/BAO/UFMatch.php b/CRM/Core/BAO/UFMatch.php index 03b018838d..bd837e81b7 100644 --- a/CRM/Core/BAO/UFMatch.php +++ b/CRM/Core/BAO/UFMatch.php @@ -636,9 +636,9 @@ AND domain_id = %4 /** * @inheritDoc */ - public function apiWhereClause($tableAlias) { + public function apiWhereClause() { // Prevent default behavior of joining ACLs onto the contact_id field - return NULL; + return array(); } } diff --git a/CRM/Core/DAO.php b/CRM/Core/DAO.php index 0759ba95f7..e001bcb025 100644 --- a/CRM/Core/DAO.php +++ b/CRM/Core/DAO.php @@ -2453,18 +2453,28 @@ SELECT contact_id } /** - * Generates a clause suitable for adding to WHERE or ON when doing an api.get for this entity + * Generates clauses suitable for adding to WHERE or ON when doing an api.get for this entity * - * @param string $tableAlias - * @return null|string + * Return format is in the form of fieldname => clauses starting with an operator. e.g.: + * @code + * array( + * 'location_type_id' => array('IS NOT NULL', 'IN (1,2,3)') + * ) + * @endcode + * + * Note that all array keys must be actual field names in this entity. Use subqueries to filter on other tables e.g. custom values. + * + * @return array */ - public function apiWhereClause($tableAlias) { + public function apiWhereClause() { $fields = $this->fields(); $cidField = CRM_Utils_Array::value('contact_id', $fields); if (CRM_Utils_Array::value('FKClassName', $cidField) == 'CRM_Contact_DAO_Contact') { - return CRM_Contact_BAO_Contact_Permission::cacheSubquery("`$tableAlias`.contact_id"); + return array( + 'contact_id' => CRM_Contact_BAO_Contact_Permission::cacheSubquery(), + ); } - return NULL; + return array(); } } diff --git a/Civi/API/SelectQuery.php b/Civi/API/SelectQuery.php index 26c6980525..a6bacb77e0 100644 --- a/Civi/API/SelectQuery.php +++ b/Civi/API/SelectQuery.php @@ -76,6 +76,10 @@ class SelectQuery { * @var array */ protected $entityFieldNames; + /** + * @var array + */ + protected $aclFields = array(); /** * @var string|bool */ @@ -104,6 +108,9 @@ class SelectQuery { $this->apiFieldSpec = $apiSpec['values']; $this->query = \CRM_Utils_SQL_Select::from($this->bao->tableName() . " a"); + + // Add ACLs + $this->query->where($this->getAclClause('a')); } /** @@ -278,8 +285,6 @@ class SelectQuery { $this->query->limit($this->options['limit'], $this->options['offset']); } - // ACLs - $this->query->where($this->getAclClause('a')); $this->bao->free(); $result_entities = array(); @@ -360,8 +365,9 @@ class SelectQuery { // Join doesn't exist - might be another param with a dot in it for some reason, we'll just ignore it. return NULL; } + $subStack = array_slice($stack, 0, $depth); // Ensure we have permission to access the other api - if (!$this->checkPermissionToJoin($fkField['FKApiName'], array_slice($stack, 0, $depth))) { + if (!$this->checkPermissionToJoin($fkField['FKApiName'], $subStack)) { throw new UnauthorizedException("Authorization failed to join onto {$fkField['FKApiName']} api in parameter $fkFieldName"); } if (!isset($fkField['FKApiSpec'])) { @@ -375,11 +381,11 @@ class SelectQuery { return NULL; } $fkTable = \CRM_Core_DAO_AllCoreTables::getTableForClass($fkField['FKClassName']); - $tableAlias = implode('_to_', array_slice($stack, 0, $depth)) . "_to_$fkTable"; + $tableAlias = implode('_to_', $subStack) . "_to_$fkTable"; $joinClause = "LEFT JOIN $fkTable $tableAlias ON $prev.$fk = $tableAlias.id"; // Add acl condition - $joinCondition = $this->getAclClause($tableAlias, $fkField['FKClassName']); + $joinCondition = $this->getAclClause($tableAlias, $fkField['FKClassName'], $subStack); if ($joinCondition !== NULL) { $joinClause .= " AND $joinCondition"; } @@ -504,9 +510,10 @@ class SelectQuery { * * @param string $tableAlias * @param string $daoName + * @param array $stack * @return null|string */ - private function getAclClause($tableAlias, $daoName = NULL) { + private function getAclClause($tableAlias, $daoName = NULL, $stack = array()) { if (!$this->checkPermissions) { return NULL; } @@ -517,7 +524,23 @@ class SelectQuery { $baoName = str_replace('_DAO_', '_BAO_', $daoName); $bao = class_exists($baoName) ? new $baoName() : new $daoName(); } - return $bao->apiWhereClause($tableAlias); + // Prevent (most) redundant acl sub clauses if they have already been applied to the main entity. + // FIXME: Currently this only works 1 level deep, but tracking through multiple joins would increase complexity + // and just doing it for the first join takes care of most acl clause deduping. + if (count($stack) === 1 && in_array($stack[0], $this->aclFields)) { + return NULL; + } + $clauses = array(); + foreach ((array) $bao->apiWhereClause() as $field => $vals) { + if (!$stack) { + // Track field clauses added to the main entity + $this->aclFields[] = $field; + } + if ($vals) { + $clauses[] = "`$tableAlias`.`$field` " . implode(" AND `$tableAlias`.`$field` ", (array) $vals); + } + } + return $clauses ? implode(' AND ', $clauses) : NULL; } /** diff --git a/api/v3/Case.php b/api/v3/Case.php index acc17ee042..722c97c37e 100644 --- a/api/v3/Case.php +++ b/api/v3/Case.php @@ -98,7 +98,7 @@ function civicrm_api3_case_create($params) { foreach ((array) $params['contact_id'] as $cid) { $contactParams = array('case_id' => $caseBAO->id, 'contact_id' => $cid); - CRM_Case_BAO_Case::addCaseToContact($contactParams); + CRM_Case_BAO_CaseContact::create($contactParams); } // Initialize XML processor with $params -- 2.25.1