From 1876e376e3b63322c73d5dffce39324f3533b94b Mon Sep 17 00:00:00 2001 From: eileen Date: Tue, 15 Jan 2019 12:44:50 +1300 Subject: [PATCH 1/3] Add test to demonstrate fatal error when accessing permitted users that are cached using the acl_cache. This has arisen during investigation of a possible regression - it turns out that if you give the 'everyone' group access to a contact using ACLs (or hooks I believe) they get a fatal error on any attempt at event or other registration. The issue is that when attempting to check for duplicates the call is made using check_permission. This in itself is a possible regression as the CRM_Dedupe_Finder::dupesByParams function now drops the check_permission key when it is equal to 0 from https://github.com/civicrm/civicrm-core/commit/4f33e78b901fb7cdb38a3026f88b59a2f9fd2c68 So we have an issue that 1) we are now applying check_permission when doing a dupe_check from front end forms - this probably is resulting in 5.9 sites getting too many duplicates are they would always be null for anon users 2) if we DO do a permissions check when an acl or hook has been used to give anon users permission to access contacts then they will get a fatal error. This is because it sets contact_id to 0 and attempts to insert it into the acl_contact_cache. I think we need to either remove the array_filter line that we think we may not need per code comments or add specific handling for the check_permission flag AND drop the foreign key constraint on the civicm_acl_contact_cache table. This means they will no longer be removed when a contact is deleted but this is a clean up issue rather than one with functionaly implications & we *should* have some form of cleanup in play on that table. In addition, removing the constraint will reduce write contention --- tests/phpunit/api/v3/ACLPermissionTest.php | 30 ++++++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/tests/phpunit/api/v3/ACLPermissionTest.php b/tests/phpunit/api/v3/ACLPermissionTest.php index da3c1ce092f4..73ffd354efec 100644 --- a/tests/phpunit/api/v3/ACLPermissionTest.php +++ b/tests/phpunit/api/v3/ACLPermissionTest.php @@ -678,6 +678,36 @@ public function testGetACLEveryonePermittedEntity() { 'id' => $this->scenarioIDs['Contact']['non_permitted_contact'], 'check_permissions' => 1, ], 0); + + // Also check that we can access ACLs through a path that uses the acl_contact_cache table. + // historically this has caused errors due to the key_constraint on that table. + // This is a bit of an artificial check as we have to amp up permissions to access this api. + // However, the lower level function is more directly accessed through the Contribution & Event & Profile + $dupes = $this->callAPISuccess('Contact', 'duplicatecheck', [ + 'match' => [ + 'first_name' => 'Anthony', + 'last_name' => 'Anderson', + 'contact_type' => 'Individual', + 'email' => 'anthony_anderson@civicrm.org', + ], + 'check_permissions' => 0, + ]); + // Actually this should be 2 but there is a line of array_filter in dupesByParams that causes + // check_permissions to be dropped at that point. I am working aginst rc now - that should possibly be removed against master. + $this->assertEquals(1, $dupes['count']); + CRM_Core_Config::singleton()->userPermissionClass->permissions = ['administer CiviCRM']; + + $dupes = $this->callAPISuccess('Contact', 'duplicatecheck', [ + 'match' => [ + 'first_name' => 'Anthony', + 'last_name' => 'Anderson', + 'contact_type' => 'Individual', + 'email' => 'anthony_anderson@civicrm.org', + ], + 'check_permissions' => 1, + ]); + $this->assertEquals(1, $dupes['count']); + } } From a99b82c59f31a113e0f9fdfcfab0c6701a95d944 Mon Sep 17 00:00:00 2001 From: eileen Date: Tue, 15 Jan 2019 12:48:36 +1300 Subject: [PATCH 2/3] Fix mishandling / loss of check_permission flag --- CRM/Dedupe/Finder.php | 4 ++-- tests/phpunit/api/v3/ACLPermissionTest.php | 4 +--- 2 files changed, 3 insertions(+), 5 deletions(-) diff --git a/CRM/Dedupe/Finder.php b/CRM/Dedupe/Finder.php index 26d62a3f53a3..ef50af8ed03b 100644 --- a/CRM/Dedupe/Finder.php +++ b/CRM/Dedupe/Finder.php @@ -125,6 +125,7 @@ public static function dupesByParams( if (!$params) { return array(); } + $checkPermission = CRM_Utils_Array::value('check_permission', $params, TRUE); // This may no longer be required - see https://github.com/civicrm/civicrm-core/pull/13176 $params = array_filter($params); @@ -146,7 +147,6 @@ public static function dupesByParams( CRM_Core_Error::fatal("$used rule for $ctype does not exist"); } } - $params['check_permission'] = CRM_Utils_Array::value('check_permission', $params, TRUE); if (isset($params['civicrm_phone']['phone_numeric'])) { $orig = $params['civicrm_phone']['phone_numeric']; @@ -155,7 +155,7 @@ public static function dupesByParams( $rgBao->params = $params; $rgBao->fillTable(); $dao = new CRM_Core_DAO(); - $dao->query($rgBao->thresholdQuery($params['check_permission'])); + $dao->query($rgBao->thresholdQuery($checkPermission)); $dupes = array(); while ($dao->fetch()) { if (isset($dao->id) && $dao->id) { diff --git a/tests/phpunit/api/v3/ACLPermissionTest.php b/tests/phpunit/api/v3/ACLPermissionTest.php index 73ffd354efec..b45a11d0fb3e 100644 --- a/tests/phpunit/api/v3/ACLPermissionTest.php +++ b/tests/phpunit/api/v3/ACLPermissionTest.php @@ -692,9 +692,7 @@ public function testGetACLEveryonePermittedEntity() { ], 'check_permissions' => 0, ]); - // Actually this should be 2 but there is a line of array_filter in dupesByParams that causes - // check_permissions to be dropped at that point. I am working aginst rc now - that should possibly be removed against master. - $this->assertEquals(1, $dupes['count']); + $this->assertEquals(2, $dupes['count']); CRM_Core_Config::singleton()->userPermissionClass->permissions = ['administer CiviCRM']; $dupes = $this->callAPISuccess('Contact', 'duplicatecheck', [ From dbb4e4f9c9e03ac2958dbad101e6bf1e0b9f19bc Mon Sep 17 00:00:00 2001 From: eileen Date: Tue, 15 Jan 2019 13:48:18 +1300 Subject: [PATCH 3/3] Remove foreign key on civicrm_acl_contact_id.user_id. This won't actually remove it from installs - we need to address that separately via ensuring people can and do run the System.updateIndices api call but it removes it from new installs and from tests, hence the test should pass --- CRM/Contact/DAO/ACLContactCache.php | 4 +--- tests/phpunit/CRM/Dedupe/MergerTest.php | 6 ++---- xml/schema/Contact/ACLContactCache.xml | 7 ------- 3 files changed, 3 insertions(+), 14 deletions(-) diff --git a/CRM/Contact/DAO/ACLContactCache.php b/CRM/Contact/DAO/ACLContactCache.php index 1ea72d573b9e..1deee6674ac7 100644 --- a/CRM/Contact/DAO/ACLContactCache.php +++ b/CRM/Contact/DAO/ACLContactCache.php @@ -6,7 +6,7 @@ * * Generated from xml/schema/CRM/Contact/ACLContactCache.xml * DO NOT EDIT. Generated by CRM_Core_CodeGen - * (GenCodeChecksum:4bb9eaae5704bfc98c258aa2f2130f5c) + * (GenCodeChecksum:ab40fa26e037ef4897359d3c288d42b8) */ /** @@ -73,7 +73,6 @@ public function __construct() { public static function getReferenceColumns() { if (!isset(Civi::$statics[__CLASS__]['links'])) { Civi::$statics[__CLASS__]['links'] = static ::createReferenceColumns(__CLASS__); - Civi::$statics[__CLASS__]['links'][] = new CRM_Core_Reference_Basic(self::getTableName(), 'user_id', 'civicrm_contact', 'id'); Civi::$statics[__CLASS__]['links'][] = new CRM_Core_Reference_Basic(self::getTableName(), 'contact_id', 'civicrm_contact', 'id'); CRM_Core_DAO_AllCoreTables::invoke(__CLASS__, 'links_callback', Civi::$statics[__CLASS__]['links']); } @@ -108,7 +107,6 @@ public static function &fields() { 'entity' => 'ACLContactCache', 'bao' => 'CRM_Contact_DAO_ACLContactCache', 'localizable' => 0, - 'FKClassName' => 'CRM_Contact_DAO_Contact', ], 'contact_id' => [ 'name' => 'contact_id', diff --git a/tests/phpunit/CRM/Dedupe/MergerTest.php b/tests/phpunit/CRM/Dedupe/MergerTest.php index 4ebf200baf77..7f3f03435bdb 100644 --- a/tests/phpunit/CRM/Dedupe/MergerTest.php +++ b/tests/phpunit/CRM/Dedupe/MergerTest.php @@ -795,8 +795,7 @@ public function getStaticCIDRefs() { 0 => 'contact_id', ), 'civicrm_acl_contact_cache' => array( - 0 => 'user_id', - 1 => 'contact_id', + 0 => 'contact_id', ), 'civicrm_action_log' => array( 0 => 'contact_id', @@ -1002,8 +1001,7 @@ public function getCalculatedCIDRefs() { // There might be cleverer ways to do this but it shouldn't change much. $cidRefs['civicrm_contact'][0] = 'primary_contact_id'; $cidRefs['civicrm_contact'][1] = 'employer_id'; - $cidRefs['civicrm_acl_contact_cache'][0] = 'user_id'; - $cidRefs['civicrm_acl_contact_cache'][1] = 'contact_id'; + $cidRefs['civicrm_acl_contact_cache'][0] = 'contact_id'; $cidRefs['civicrm_mailing'][0] = 'created_id'; $cidRefs['civicrm_mailing'][1] = 'scheduled_id'; $cidRefs['civicrm_mailing'][2] = 'approver_id'; diff --git a/xml/schema/Contact/ACLContactCache.xml b/xml/schema/Contact/ACLContactCache.xml index d544de405067..450b56b693eb 100644 --- a/xml/schema/Contact/ACLContactCache.xml +++ b/xml/schema/Contact/ACLContactCache.xml @@ -25,13 +25,6 @@ FK to civicrm_contact (could be null for anon user) 3.1 - - user_id - civicrm_contact
- id - 3.1 - CASCADE - contact_id Contact ID