Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Merged
merged 3 commits into from
Jan 16, 2019
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1876e37
Add test to demonstrate fatal error when accessing permitted users th…
eileenmcnaughton Jan 14, 2019
a99b82c
Fix mishandling / loss of check_permission flag
eileenmcnaughton Jan 14, 2019
dbb4e4f
Remove foreign key on civicrm_acl_contact_id.user_id.
eileenmcnaughton Jan 15, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 1 addition & 3 deletions CRM/Contact/DAO/ACLContactCache.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
*
* Generated from xml/schema/CRM/Contact/ACLContactCache.xml
* DO NOT EDIT. Generated by CRM_Core_CodeGen
* (GenCodeChecksum:4bb9eaae5704bfc98c258aa2f2130f5c)
* (GenCodeChecksum:ab40fa26e037ef4897359d3c288d42b8)
*/

/**
Expand Down Expand Up @@ -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']);
}
Expand Down Expand Up @@ -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',
Expand Down
4 changes: 2 additions & 2 deletions CRM/Dedupe/Finder.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand All @@ -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'];
Expand All @@ -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) {
Expand Down
6 changes: 2 additions & 4 deletions tests/phpunit/CRM/Dedupe/MergerTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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',
Expand Down Expand Up @@ -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';
Expand Down
28 changes: 28 additions & 0 deletions tests/phpunit/api/v3/ACLPermissionTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -678,6 +678,34 @@ 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,
]);
$this->assertEquals(2, $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']);

}

}
7 changes: 0 additions & 7 deletions xml/schema/Contact/ACLContactCache.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,13 +25,6 @@
<comment>FK to civicrm_contact (could be null for anon user)</comment>
<add>3.1</add>
</field>
<foreignKey>
<name>user_id</name>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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',

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

<table>civicrm_contact</table>
<key>id</key>
<add>3.1</add>
<onDelete>CASCADE</onDelete>
</foreignKey>
<field>
<name>contact_id</name>
<title>Contact ID</title>
Expand Down