(dev/core#660) Address regression whereby Anonymous users can no longer register for an event if they have ACLs to see a contact

[eileenmcnaughton]

Overview

A brief description of the pull request. Try to keep it non-technical.

Before

After

Check is done with permissions set to FALSE, this prevents the fatals.

In addition the foreign_key on civicrm_acl_contact_cache.user_id is removed from the schema. At this stage this means it's not added to new installs or test installs but will not be removed from existing installs as yet (that is a follow on task and we will do some testing on the System.update_indexes job for that). Removing it fixes a bug that preceeds the regression but which was exposed by the regression.

Technical Details

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

4f33e78

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

Comments

This needs to go in 5.9

[civibot]

(Standard links)

• If this is your first pull-request for CiviCRM, please browse CONTRIBUTING.md for information about the development and testing processes.
• If you are reviewing this pull-request, you may wish to consult the test sites and the Review Standards (long template, short template).

[eileenmcnaughton]

Note the huge amount of failures here https://test.civicrm.org/job/CiviCRM-Core-PR/24030/#showFailuresLink - come from data cleanup failing on the new test - which is fixed by removing the index

[davejenx]

Thanks for this PR, @eileenmcnaughton . I tested it against this issue:
dev/core/issues/660 Fatal DB Error: already exists on event registration/contribution pages when profile has user creation
on local dmaster, just applying the code changes but not altering table civicm_acl_contact_cache. This was sufficient to stop the db error on civicrm_uf_match. A single contact was created and registered for the event, user and uf_match also correctly created. 😁

[eileenmcnaughton]

test this please

[eileenmcnaughton]

unrelated fail - CRM_Contact_Page_View_UserDashBoardTest.testDashboardContentContributionsWithInvoicingEnabled

(there is some clean up issue affecting that test)

[totten]

@eileenmcnaughton, I think I follow why check_permission is relevant to this bug/problem. I'm having trouble seeing why the schema change is relevant.

Relatedly... when grepping for user_id in civicrm_acl_contact_cache, it seems like CRM/Contact/BAO/Contact/Permission.php (at a minimum) has several references to it:

[nix-shell:~/bknix/build/dmaster/sites/all/modules/civicrm]$ svngrep user_id  CRM/ Civi api/ bin/
CRM/Contact/BAO/Contact/Permission.php:  AND user_id = {$contactID}
CRM/Contact/BAO/Contact/Permission.php:WHERE  user_id = %1
CRM/Contact/BAO/Contact/Permission.php:INSERT INTO civicrm_acl_contact_cache ( user_id, contact_id, operation )
CRM/Contact/BAO/Contact/Permission.php:SELECT DISTINCT $userID as user_id, contact_a.id as contact_id, '{$operation}' as operation
CRM/Contact/BAO/Contact/Permission.php:         LEFT JOIN civicrm_acl_contact_cache ac ON ac.user_id = $userID AND ac.contact_id = contact_a.id AND ac.operation = '{$operation}'
CRM/Contact/BAO/Contact/Permission.php:AND ac.user_id IS NULL
CRM/Contact/BAO/Contact/Permission.php:        SELECT count(*) FROM civicrm_acl_contact_cache WHERE user_id = %1 AND contact_id = %1 AND operation = '{$operation}' LIMIT 1", $queryParams)) {
CRM/Contact/BAO/Contact/Permission.php:        CRM_Core_DAO::executeQuery("INSERT INTO civicrm_acl_contact_cache ( user_id, contact_id, operation ) VALUES(%1, %1, '{$operation}')", $queryParams);
CRM/Contact/BAO/Contact/Permission.php:        $clauses[] = " INNER JOIN civicrm_acl_contact_cache aclContactCache_{$k} ON {$alias}.id = aclContactCache_{$k}.contact_id AND aclContactCache_{$k}.user_id = $contactID ";
CRM/Contact/BAO/Contact/Permission.php:      $whereClase = " aclContactCache.user_id = $contactID AND $contactAlias.is_deleted = 0";
CRM/Contact/BAO/Contact/Permission.php:      return "IN (SELECT contact_id FROM civicrm_acl_contact_cache WHERE user_id = $contactID)";
CRM/Contact/BAO/Contact/Permission.php:      $user_id_column    = "contact_id_{$direction['from']}";
CRM/Contact/BAO/Contact/Permission.php: WHERE civicrm_relationship.{$user_id_column} = {$contactID}
CRM/Contact/BAO/Contact/Permission.php:    CRM_Core_DAO::executeQuery('SET @civicrm_user_id = %1',

[eileenmcnaughton]

@totten yeah the field is used& even without the fk it is still indexed - but it stops fatal errors under some configs (at least on new installs or ones that run the system.update_indexes api

[totten]

Doh, I misread in haste. This is only dropping the <foreignKey>; I misread it as dropping the column. Haven't fully got my head around this, but just want to note that the grep may be less important than I originally thought.

[eileenmcnaughton]

right & it really only affects new installs at this point & crucially the test suite runs on new installs so we can start testing acls with no logged in user.

If people want to benefit from this on their own pre-existing sites there is still an extra step for them at this point

[totten]

In favor of merging:

• There are multiple reports of the new problem/regression which is addressed here. For folks affected, it'd be a significant problem.
• This is at an intersection of topics (ACL cache and dedupe) where Eileen has special expertise.
• @davejenx reports the patch fixing the regression in his usage.
• At a macro level, I can see the connection between the symptom and the use of check_permission within the deduper, so that aspect of the fix is plausible.

The one drawback -- part of the patch+explanation (re:dropping the FK-constraint for user_id) touches on edge-cases involving anonymous-users and ACLs which I don't fully grok. However... what's the foreseeable consequence of dropping the FK-constraint? I guess the cache retains unnecessary cache-rows (i.e. for the benefit of an ill-defined user). However, any realistic reads on the cache should be filtered in a way which precludes focuses on active users. (To wit: typical query on this table should involve WHERE ac.user_id = {CRM_Core_Session::getLoggedInContactID()}). Restating -- we potentially allow it to retain data that won't be be read again. That data should eventually get cleaned-up when there's a more general cache-clear. I struggle to find a problem with this.

[eileenmcnaughton]

@totten also - we actually clear the cache on each contact edit I believe - without the opt out permitted on group contact cache

[vingle]

This patch solves the same issue when creating new contributions and memberships (described here), thanks.

[eileenmcnaughton]

@vingle @davejenx this is now released in 5.9.1

[davejenx]

Related SE post: DB Error: constraint violation on anonymous user event registration after 5.8.2 -> 5.9.0 update.