Users affected by ACLs intermittently lose access to contacts
Overview
When a user's permission to view Contacts is affected by multiple ACLs, they will intermittently lose access to some of them, especially directly after editing a record.
Reproduction steps
- Create a test User who is not an Administrator and does not have "view all contacts" or "edit all contacts" permission
- Create two Groups (of type Access Control List), and add the test User to both groups
- Create two Groups (not of ACL type) with distinct names like "Contact Group1" and "Contact Group2"
- Create two Contacts and add one to each of the Groups created in step 3
- Create two ACLs, each giving users in one of the ACL groups access to one of the Contact groups
- Log in as the test User and repeatedly edit the Nickname (or any other data) on both of the test Contacts until you encounter a failure ("Error You do not have the necessary permission to view this contact").
It will rarely take more than ten edits before you encounter a failure, and the failures will never occur for one of the ACL groups.
Current behaviour
Sometimes it works, sometimes the user loses permission during the save or reload operation, resulting in the error message below. Our users reported that usually the data would be saved, but sometimes not (e.g. when saving some Activities on a Case). I have not been able to replicate the data loss scenario.
Error
You do not have the necessary permission to view this contact.
Expected behaviour
The data should be saved without error messages appearing (and this is what happens some of the time).
Environment information
- Browser: Mostly tested on Chromium, most recently version 90.0.4430.93
- CiviCRM: Tested on 5.33.5 and prior ESR versions
- PHP: 7.3
- CMS: Drupal 7.80
- Database: 10.3.27-MariaDB
- Web Server: Apache
Comments
We have mitigated the problem with this patch (minus the watchdog line with the FIXME comment): https://github.com/AsylumSeekersCentre/civicrm-core/commit/030ecb6e31ed17b9fd01716cb772f13bb60bf455
With this in place, I've been unable to reproduce the error in manual testing. If it still happens, it's very rare. However, I'm not sure that this is a very "proper" solution so I'm opening the issue without a pull request for discussion.
The watchdog line from that patch usually produces output like this:
( ( `contact_a`.id IN ( SELECT contact_id FROM civicrm_group_contact WHERE group_id IN (5, 6) AND status = 'Added' ) ) ) AND (contact_a.is_deleted = 0)
On the occasions when the permission failure occurs, it instead looks like this:
( ( `contact_a`.id IN ( SELECT contact_id FROM civicrm_group_contact WHERE group_id IN (5) AND status = 'Added' ) ) ) AND (contact_a.is_deleted = 0)
It has somehow lost one of the groups it should be checking for. It always seems to retain the same one, which I think is why it works if the user is only affected by one ACL, and the bug only shows up if users are affected by multiple ACLs.