Add test to demonstrate fatal error when accessing permitted users that are cached...
authoreileen <emcnaughton@wikimedia.org>
Mon, 14 Jan 2019 23:44:50 +0000 (12:44 +1300)
committereileen <emcnaughton@wikimedia.org>
Tue, 15 Jan 2019 08:30:48 +0000 (21:30 +1300)
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

index da3c1ce092f451ae05908043577a1ba6655a0311..73ffd354efecae8f365a39f3e372158c4125789f 100644 (file)
@@ -678,6 +678,36 @@ class api_v3_ACLPermissionTest extends CiviUnitTestCase {
       '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']);
+
   }
 
 }