From 21c6be2848f547aa6385246aca9717c81abe6d83 Mon Sep 17 00:00:00 2001 From: Seamus Lee Date: Mon, 29 Jun 2020 12:44:20 +1000 Subject: [PATCH 1/3] [REF] Use Standard function cacheClasue to re-use contact acl cache table --- CRM/Contact/BAO/Query.php | 36 +++++++++++++++--------------------- 1 file changed, 15 insertions(+), 21 deletions(-) diff --git a/CRM/Contact/BAO/Query.php b/CRM/Contact/BAO/Query.php index 4a108032a0c5..8eb59e32aa2c 100644 --- a/CRM/Contact/BAO/Query.php +++ b/CRM/Contact/BAO/Query.php @@ -1487,6 +1487,9 @@ public function query($count = FALSE, $sortByChar = FALSE, $groupContacts = FALS } if (!empty($this->_permissionWhereClause) && empty($this->_displayRelationshipType)) { + if (!empty($this->_permissionFromClause)) { + $from .= " $this->_permissionFromClause"; + } if (empty($where)) { $where = "WHERE $this->_permissionWhereClause"; } @@ -4603,6 +4606,9 @@ public static function apiQuery( $options = $query->_options; if (!empty($query->_permissionWhereClause)) { + if (!empty($query->_permissionFromClause) && !stripos($from, 'aclContactCache')) { + $from .= " $query->_permissionFromClause"; + } if (empty($where)) { $where = "WHERE $query->_permissionWhereClause"; } @@ -5051,14 +5057,9 @@ private function createSqlCase($idCol, $cids) { */ public function generatePermissionClause($onlyDeleted = FALSE, $count = FALSE) { if (!$this->_skipPermission) { - $this->_permissionWhereClause = CRM_ACL_API::whereClause( - CRM_Core_Permission::VIEW, - $this->_tables, - $this->_whereTables, - NULL, - $onlyDeleted, - $this->_skipDeleteClause - ); + $permissionClauses = CRM_Contact_BAO_Contact_Permission::cacheClause(); + $this->_permissionWhereClause = $permissionClauses[1]; + $this->_permissionFromClause = $permissionClauses[0]; if (!$onlyDeleted && CRM_Core_Permission::check('access deleted contacts')) { $this->_permissionWhereClause = str_replace(' ( 1 ) ', '(contact_a.is_deleted = 0)', $this->_permissionWhereClause); @@ -5085,19 +5086,6 @@ public function generatePermissionClause($onlyDeleted = FALSE, $count = FALSE) { $this->_permissionWhereClause .= '(' . implode(' AND ', $clauses) . ')'; } } - - // regenerate fromClause since permission might have added tables - if ($this->_permissionWhereClause) { - //fix for row count in qill (in contribute/membership find) - if (!$count) { - $this->_useDistinct = TRUE; - } - //CRM-15231 - $this->_fromClause = self::fromClause($this->_tables, NULL, NULL, $this->_primaryLocation, $this->_mode); - $this->_simpleFromClause = self::fromClause($this->_whereTables, NULL, NULL, $this->_primaryLocation, $this->_mode); - // note : this modifies _fromClause and _simpleFromClause - $this->includePseudoFieldsJoin($this->_sort); - } } else { // add delete clause if needed even if we are skipping permission @@ -5129,6 +5117,9 @@ public function setSkipPermission($val) { */ public function summaryContribution($context = NULL) { list($innerselect, $from, $where, $having) = $this->query(TRUE); + if (!empty($this->_permissionFromClause) && !stripos($from, 'aclContactCache')) { + $from .= " $this->_permissionFromClause"; + } if ($this->_permissionWhereClause) { $where .= " AND " . $this->_permissionWhereClause; } @@ -5832,6 +5823,9 @@ public function filterRelatedContacts(&$from, &$where, &$having) { $from = str_replace("INNER JOIN", "LEFT JOIN", $from); $from .= $qcache['from']; $where = $qcache['where']; + if (!empty($this->_permissionFromClause) && !stripos($from, 'aclContactCache')) { + $from .= " $this->_permissionFromClause"; + } if (!empty($this->_permissionWhereClause)) { $where .= "AND $this->_permissionWhereClause"; } From e27c3498e76b8a719e45465c5906810bf3acad57 Mon Sep 17 00:00:00 2001 From: Seamus Lee Date: Mon, 29 Jun 2020 13:09:42 +1000 Subject: [PATCH 2/3] Ensure that cacheClause respects the access deleted contacts permission Tidy up where clause generation as per Coleman --- CRM/Contact/BAO/Contact/Permission.php | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/CRM/Contact/BAO/Contact/Permission.php b/CRM/Contact/BAO/Contact/Permission.php index 964fc47295c2..1aae3cea9a7d 100644 --- a/CRM/Contact/BAO/Contact/Permission.php +++ b/CRM/Contact/BAO/Contact/Permission.php @@ -307,17 +307,16 @@ public static function cacheClause($contactAlias = 'contact_a') { if (CRM_Core_Permission::check('view all contacts') || CRM_Core_Permission::check('edit all contacts') ) { - if (is_array($contactAlias)) { + if (!CRM_Core_Permission::check('access deleted contacts')) { $wheres = []; - foreach ($contactAlias as $alias) { + foreach ((array) $contactAlias as $alias) { // CRM-6181 $wheres[] = "$alias.is_deleted = 0"; } return [NULL, '(' . implode(' AND ', $wheres) . ')']; } else { - // CRM-6181 - return [NULL, "$contactAlias.is_deleted = 0"]; + return [NULL, '( 1 )']; } } @@ -332,14 +331,17 @@ public static function cacheClause($contactAlias = 'contact_a') { } $fromClause = implode(" ", $clauses); - $whereClase = NULL; + $whereClause = NULL; } else { $fromClause = " INNER JOIN civicrm_acl_contact_cache aclContactCache ON {$contactAlias}.id = aclContactCache.contact_id "; - $whereClase = " aclContactCache.user_id = $contactID AND $contactAlias.is_deleted = 0"; + $whereClause = " aclContactCache.user_id = $contactID"; + if (!CRM_Core_Permission::check('access deleted contacts')) { + $whereClause .= " AND $contactAlias.is_deleted = 0"; + } } - return [$fromClause, $whereClase]; + return [$fromClause, $whereClause]; } /** From 21b8bcb53165965a178bfc992fe1c68b0af97f23 Mon Sep 17 00:00:00 2001 From: Seamus Lee Date: Thu, 16 Jul 2020 17:06:11 +1000 Subject: [PATCH 3/3] Fix trash searching now with the alternate join and add in new test to prove that when someone doesn't have view all contacts but does have access deleted contacts and we are searching in the trash an appropriate where clause is generated --- CRM/Contact/BAO/Query.php | 14 ++++++++-- tests/phpunit/CRM/Contact/SelectorTest.php | 31 ++++++++++++++++++++++ 2 files changed, 43 insertions(+), 2 deletions(-) diff --git a/CRM/Contact/BAO/Query.php b/CRM/Contact/BAO/Query.php index 8eb59e32aa2c..14ad083a4fed 100644 --- a/CRM/Contact/BAO/Query.php +++ b/CRM/Contact/BAO/Query.php @@ -5061,8 +5061,18 @@ public function generatePermissionClause($onlyDeleted = FALSE, $count = FALSE) { $this->_permissionWhereClause = $permissionClauses[1]; $this->_permissionFromClause = $permissionClauses[0]; - if (!$onlyDeleted && CRM_Core_Permission::check('access deleted contacts')) { - $this->_permissionWhereClause = str_replace(' ( 1 ) ', '(contact_a.is_deleted = 0)', $this->_permissionWhereClause); + if (CRM_Core_Permission::check('access deleted contacts')) { + if (!$onlyDeleted) { + $this->_permissionWhereClause = str_replace('( 1 )', '(contact_a.is_deleted = 0)', $this->_permissionWhereClause); + } + else { + if ($this->_permissionWhereClause === '( 1 )') { + $this->_permissionWhereClause = str_replace('( 1 )', '(contact_a.is_deleted)', $this->_permissionWhereClause); + } + else { + $this->_permissionWhereClause .= " AND (contact_a.is_deleted) "; + } + } } if (isset($this->_tables['civicrm_activity'])) { diff --git a/tests/phpunit/CRM/Contact/SelectorTest.php b/tests/phpunit/CRM/Contact/SelectorTest.php index fd10b18eeb14..bb103b9c123c 100644 --- a/tests/phpunit/CRM/Contact/SelectorTest.php +++ b/tests/phpunit/CRM/Contact/SelectorTest.php @@ -32,6 +32,12 @@ class CRM_Contact_SelectorTest extends CiviUnitTestCase { * @throws \Exception */ public function testSelectorQuery($dataSet) { + if (!empty($dataSet['limitedPermissions'])) { + CRM_Core_Config::singleton()->userPermissionClass->permissions = [ + 'access CiviCRM', + 'access deleted contacts', + ]; + } $params = CRM_Contact_BAO_Query::convertFormValues($dataSet['form_values'], 0, FALSE, NULL, []); $isDeleted = in_array(['deleted_contacts', '=', 1, 0, 0], $params); foreach ($dataSet['settings'] as $setting) { @@ -90,6 +96,9 @@ public function testSelectorQuery($dataSet) { $selector->getQueryObject()->getCachedContacts([$contactID], FALSE); } } + if (!empty($dataSet['limitedPermissions'])) { + $this->cleanUpAfterACLs(); + } } /** @@ -314,6 +323,28 @@ public function querySets() { 2 => "WHERE ( civicrm_email.email LIKE 'mickey@mouseville.com%' AND ( ( ( contact_a.sort_name LIKE 'Mouse%' ) OR ( civicrm_email.email LIKE 'Mouse%' ) ) ) ) AND (contact_a.is_deleted)", ], ], + ], + [ + [ + 'description' => 'Ensure that the Join to the acl contact cache is correct and that if we are searching in deleted contacts appropriate where clause is added', + 'class' => 'CRM_Contact_Selector', + 'settings' => [['name' => 'includeWildCardInName', 'value' => FALSE]], + 'form_values' => ['email' => 'mickey@mouseville.com', 'sort_name' => 'Mouse', 'deleted_contacts' => 1], + 'params' => [], + 'return_properties' => NULL, + 'context' => 'advanced', + 'action' => CRM_Core_Action::ADVANCED, + 'includeContactIds' => NULL, + 'searchDescendentGroups' => FALSE, + 'limitedPermissions' => TRUE, + 'expected_query' => [ + 0 => 'default', + 1 => 'FROM civicrm_contact contact_a LEFT JOIN civicrm_address ON ( contact_a.id = civicrm_address.contact_id AND civicrm_address.is_primary = 1 ) LEFT JOIN civicrm_country ON ( civicrm_address.country_id = civicrm_country.id ) LEFT JOIN civicrm_email ON (contact_a.id = civicrm_email.contact_id AND civicrm_email.is_primary = 1) LEFT JOIN civicrm_phone ON (contact_a.id = civicrm_phone.contact_id AND civicrm_phone.is_primary = 1) LEFT JOIN civicrm_im ON (contact_a.id = civicrm_im.contact_id AND civicrm_im.is_primary = 1) LEFT JOIN civicrm_worldregion ON civicrm_country.region_id = civicrm_worldregion.id INNER JOIN civicrm_acl_contact_cache aclContactCache ON contact_a.id = aclContactCache.contact_id', + 2 => "WHERE ( civicrm_email.email LIKE 'mickey@mouseville.com%' AND ( ( ( contact_a.sort_name LIKE 'Mouse%' ) OR ( civicrm_email.email LIKE 'Mouse%' ) ) ) ) AND aclContactCache.user_id = 0 AND (contact_a.is_deleted)", + ], + ], + ], + [ [ 'description' => 'Use of quotes for exact string', 'use_case_comments' => 'This is something that was in the code but seemingly not working. No UI info on it though!',