From f704d628e1484cc9ca425970d951b7f482a31ce9 Mon Sep 17 00:00:00 2001 From: colemanw Date: Thu, 22 Jun 2023 17:42:59 -0700 Subject: [PATCH] Fix ACLs to use the correct name of the civicrm_group table For unknown historical reasons, group-based ACLs incorrectly used 'civicrm_saved_search' as the value for 'object_table' when referencing a group id. This updates all ACL records, tests, and adds legacy handling for hook subscribers with a noisy deprecation notice. --- CRM/ACL/API.php | 4 ++-- CRM/ACL/BAO/ACL.php | 6 +++--- CRM/ACL/Form/ACL.php | 4 ++-- CRM/ACL/Page/ACL.php | 2 +- CRM/Bridge/OG/Drupal.php | 2 +- CRM/Contact/BAO/Group.php | 7 +++---- CRM/Core/Permission/Base.php | 4 ++-- CRM/Upgrade/Incremental/sql/5.64.alpha1.mysql.tpl | 5 ++++- CRM/Utils/Hook.php | 15 ++++++++++++--- Civi/Test/ACLPermissionTrait.php | 2 +- tests/phpunit/CRM/Group/Page/AjaxTest.php | 5 ++++- tests/phpunit/CRM/Mailing/BAO/MailingTest.php | 3 +++ tests/phpunit/CiviTest/CiviUnitTestCase.php | 4 ++-- tests/phpunit/api/v3/GroupTest.php | 6 ++++-- tests/phpunit/api/v3/RelationshipTest.php | 3 +-- tests/phpunit/api/v3/ReportTemplateTest.php | 4 +++- xml/templates/civicrm_acl.tpl | 2 +- 17 files changed, 49 insertions(+), 29 deletions(-) diff --git a/CRM/ACL/API.php b/CRM/ACL/API.php index d9739d48d7..ce2fc6ae7f 100644 --- a/CRM/ACL/API.php +++ b/CRM/ACL/API.php @@ -148,7 +148,7 @@ class CRM_ACL_API { public static function group( $type, $contactID = NULL, - $tableName = 'civicrm_saved_search', + $tableName = 'civicrm_group', $allGroups = NULL, $includedGroups = [] ) { @@ -181,7 +181,7 @@ class CRM_ACL_API { $type, $groupID, $contactID = NULL, - $tableName = 'civicrm_saved_search', + $tableName = 'civicrm_group', $allGroups = NULL, $includedGroups = NULL ) { diff --git a/CRM/ACL/BAO/ACL.php b/CRM/ACL/BAO/ACL.php index 10ff5e51bd..77311c8cf7 100644 --- a/CRM/ACL/BAO/ACL.php +++ b/CRM/ACL/BAO/ACL.php @@ -234,7 +234,7 @@ SELECT a.operation, a.object_id FROM civicrm_acl_cache c, civicrm_acl a WHERE c.acl_id = a.id AND a.is_active = 1 - AND a.object_table = 'civicrm_saved_search' + AND a.object_table = 'civicrm_group' AND a.id IN ( $aclKeys ) AND a.deny = 0 ORDER BY a.object_id @@ -259,7 +259,7 @@ ORDER BY a.object_id FROM civicrm_acl_cache c, civicrm_acl a WHERE c.acl_id = a.id AND a.is_active = 1 - AND a.object_table = 'civicrm_saved_search' + AND a.object_table = 'civicrm_group' AND a.id IN ( $aclKeys ) AND a.deny = 1 AND a.object_id IN (%1) @@ -333,7 +333,7 @@ SELECT g.* public static function group( $type, $contactID = NULL, - $tableName = 'civicrm_saved_search', + $tableName = 'civicrm_group', $allGroups = NULL, $includedGroups = [] ) { diff --git a/CRM/ACL/Form/ACL.php b/CRM/ACL/Form/ACL.php index a86ebb1fcb..7b80b81543 100644 --- a/CRM/ACL/Form/ACL.php +++ b/CRM/ACL/Form/ACL.php @@ -37,7 +37,7 @@ class CRM_ACL_Form_ACL extends CRM_Admin_Form { if (isset($defaults['object_table'])) { switch ($defaults['object_table']) { - case 'civicrm_saved_search': + case 'civicrm_group': $defaults['group_id'] = $defaults['object_id']; $defaults['object_type'] = 1; $showHide->addShow("id-group-acl"); @@ -265,7 +265,7 @@ class CRM_ACL_Form_ACL extends CRM_Admin_Form { // Figure out which type of object we're permissioning on and set object_table and object_id. switch ($params['object_type']) { case 1: - $params['object_table'] = 'civicrm_saved_search'; + $params['object_table'] = 'civicrm_group'; $params['object_id'] = $params['group_id']; break; diff --git a/CRM/ACL/Page/ACL.php b/CRM/ACL/Page/ACL.php index 9aea585919..78c7fe26e7 100644 --- a/CRM/ACL/Page/ACL.php +++ b/CRM/ACL/Page/ACL.php @@ -142,7 +142,7 @@ class CRM_ACL_Page_ACL extends CRM_Core_Page_Basic { } switch ($acl[$dao->id]['object_table']) { - case 'civicrm_saved_search': + case 'civicrm_group': $acl[$dao->id]['object'] = $group[$acl[$dao->id]['object_id']] ?? NULL; $acl[$dao->id]['object_name'] = ts('Group'); break; diff --git a/CRM/Bridge/OG/Drupal.php b/CRM/Bridge/OG/Drupal.php index 7a8b0be8b5..6933ac0fcc 100644 --- a/CRM/Bridge/OG/Drupal.php +++ b/CRM/Bridge/OG/Drupal.php @@ -175,7 +175,7 @@ SELECT v.id public static function updateCiviACL(&$params, $op) { $dao = new CRM_ACL_DAO_ACL(); - $dao->object_table = 'civicrm_saved_search'; + $dao->object_table = 'civicrm_group'; $dao->object_id = $params['civicrm_group_id']; if ($op == 'delete') { diff --git a/CRM/Contact/BAO/Group.php b/CRM/Contact/BAO/Group.php index 6563b61d57..f201164844 100644 --- a/CRM/Contact/BAO/Group.php +++ b/CRM/Contact/BAO/Group.php @@ -285,7 +285,7 @@ class CRM_Contact_BAO_Group extends CRM_Contact_DAO_Group { $permissions = NULL; if (CRM_Core_Permission::check('edit all contacts') || CRM_ACL_API::groupPermission(CRM_ACL_API::EDIT, $id, NULL, - 'civicrm_saved_search', $allGroups + 'civicrm_group', $allGroups ) ) { $permissions[] = CRM_Core_Permission::EDIT; @@ -293,7 +293,7 @@ class CRM_Contact_BAO_Group extends CRM_Contact_DAO_Group { if (CRM_Core_Permission::check('view all contacts') || CRM_ACL_API::groupPermission(CRM_ACL_API::VIEW, $id, NULL, - 'civicrm_saved_search', $allGroups + 'civicrm_group', $allGroups ) ) { $permissions[] = CRM_Core_Permission::VIEW; @@ -316,8 +316,7 @@ class CRM_Contact_BAO_Group extends CRM_Contact_DAO_Group { $clauses = []; if (!CRM_Core_Permission::check([['edit all contacts', 'view all contacts']])) { $allGroups = CRM_Core_PseudoConstant::allGroup(NULL, FALSE); - // FIXME: TableName 'civicrm_saved_search' seems wrong but is consistent with self::checkPermission - $allowedGroups = \CRM_ACL_API::group(CRM_ACL_API::VIEW, NULL, 'civicrm_saved_search', $allGroups); + $allowedGroups = \CRM_ACL_API::group(CRM_ACL_API::VIEW, NULL, 'civicrm_group', $allGroups); $groupsIn = $allowedGroups ? implode(',', $allowedGroups) : '0'; $clauses['id'][] = "IN ($groupsIn)"; } diff --git a/CRM/Core/Permission/Base.php b/CRM/Core/Permission/Base.php index 9b1ae5709b..ac9fbea549 100644 --- a/CRM/Core/Permission/Base.php +++ b/CRM/Core/Permission/Base.php @@ -169,7 +169,7 @@ class CRM_Core_Permission_Base { Civi::$statics[__CLASS__]['viewPermissionedGroups_' . $domainId . '_' . $userId][$groupKey] = $groups; } - $ids = CRM_ACL_API::group(CRM_Core_Permission::VIEW, NULL, 'civicrm_saved_search', $groups); + $ids = CRM_ACL_API::group(CRM_Core_Permission::VIEW, NULL, 'civicrm_group', $groups); if (!empty($ids)) { foreach (array_values($ids) as $id) { $title = CRM_Core_DAO::getFieldValue('CRM_Contact_DAO_Group', $id, 'title'); @@ -178,7 +178,7 @@ class CRM_Core_Permission_Base { } } - $ids = CRM_ACL_API::group(CRM_Core_Permission::EDIT, NULL, 'civicrm_saved_search', $groups); + $ids = CRM_ACL_API::group(CRM_Core_Permission::EDIT, NULL, 'civicrm_group', $groups); if (!empty($ids)) { foreach (array_values($ids) as $id) { $title = CRM_Core_DAO::getFieldValue('CRM_Contact_DAO_Group', $id, 'title'); diff --git a/CRM/Upgrade/Incremental/sql/5.64.alpha1.mysql.tpl b/CRM/Upgrade/Incremental/sql/5.64.alpha1.mysql.tpl index f1f12629b2..d44f0b8c4f 100644 --- a/CRM/Upgrade/Incremental/sql/5.64.alpha1.mysql.tpl +++ b/CRM/Upgrade/Incremental/sql/5.64.alpha1.mysql.tpl @@ -4,7 +4,10 @@ UPDATE `civicrm_acl` SET `priority` = `id`; -- Remove obsolete "Basic ACLs" DELETE FROM civicrm_acl -WHERE object_table NOT IN ('civicrm_saved_search', 'civicrm_uf_group', 'civicrm_custom_group', 'civicrm_event'); +WHERE object_table NOT IN ('civicrm_group', 'civicrm_saved_search', 'civicrm_uf_group', 'civicrm_custom_group', 'civicrm_event'); + +-- Fix wrong table name +UPDATE `civicrm_acl` SET `object_table` = 'civicrm_group' WHERE `object_table` = 'civicrm_saved_search'; -- fix mis-casing of field name. Note the php function doesn't permit the name change hence it is here -- but field is not localised. diff --git a/CRM/Utils/Hook.php b/CRM/Utils/Hook.php index 9403947013..5e6005b77b 100644 --- a/CRM/Utils/Hook.php +++ b/CRM/Utils/Hook.php @@ -598,9 +598,7 @@ abstract class CRM_Utils_Hook { * @param int $contactID * User contactID for whom the check is made. * @param string $tableName - * Table name of group, e.g. `civicrm_uf_group` or `civicrm_custom_group`. - * Note: for some weird reason when this hook is called for contact groups, this - * value will be `civicrm_saved_search` instead of `civicrm_group` as you'd expect. + * Table name of group, e.g. 'civicrm_group' or 'civicrm_uf_group' or 'civicrm_custom_group'. * @param array $allGroups * All groups from the above table, keyed by id. * @param int[] $currentGroups @@ -611,6 +609,17 @@ abstract class CRM_Utils_Hook { */ public static function aclGroup($type, $contactID, $tableName, &$allGroups, &$currentGroups) { $null = NULL; + // Legacy support for hooks that still expect 'civicrm_group' to be 'civicrm_saved_search' + // This was changed in 5.64 + if ($tableName === 'civicrm_group') { + $initialValue = $currentGroups; + $legacyTableName = 'civicrm_saved_search'; + self::singleton() + ->invoke(['type', 'contactID', 'tableName', 'allGroups', 'currentGroups'], $type, $contactID, $legacyTableName, $allGroups, $currentGroups, $null, 'civicrm_aclGroup'); + if ($initialValue != $currentGroups) { + CRM_Core_Error::deprecatedWarning('Since 5.64 hook_civicrm_aclGroup passes "civicrm_group" instead of "civicrm_saved_search" for the $tableName when referring to Groups. Hook listeners should be updated.'); + } + } return self::singleton() ->invoke(['type', 'contactID', 'tableName', 'allGroups', 'currentGroups'], $type, $contactID, $tableName, $allGroups, $currentGroups, $null, 'civicrm_aclGroup'); } diff --git a/Civi/Test/ACLPermissionTrait.php b/Civi/Test/ACLPermissionTrait.php index 91b6681cf6..7ff1705e31 100644 --- a/Civi/Test/ACLPermissionTrait.php +++ b/Civi/Test/ACLPermissionTrait.php @@ -125,7 +125,7 @@ trait ACLPermissionTrait { * @throws CRM_Core_Exception */ public function setupCoreACLPermittedAcl($permissionedEntities = [], $groupAllowedAccess = 'Everyone', $operation = 'View', $entity = 'Group') { - $tableMap = ['Group' => 'civicrm_saved_search', 'CustomGroup' => 'civicrm_custom_group', 'Profile' => 'civicrm_uf_match', 'Event' => 'civicrm_event']; + $tableMap = ['Group' => 'civicrm_group', 'CustomGroup' => 'civicrm_custom_group', 'Profile' => 'civicrm_uf_group', 'Event' => 'civicrm_event']; $entityTable = $tableMap[$entity]; $permittedRoleID = ($groupAllowedAccess === 'Everyone') ? 0 : $groupAllowedAccess; diff --git a/tests/phpunit/CRM/Group/Page/AjaxTest.php b/tests/phpunit/CRM/Group/Page/AjaxTest.php index acb7ad4bc7..6f52b0805c 100644 --- a/tests/phpunit/CRM/Group/Page/AjaxTest.php +++ b/tests/phpunit/CRM/Group/Page/AjaxTest.php @@ -666,6 +666,9 @@ class CRM_Group_Page_AjaxTest extends CiviUnitTestCase { * @param array $currentGroups */ public function hook_civicrm_aclGroup($type, $contactID, $tableName, &$allGroups, &$currentGroups) { + if ($tableName !== 'civicrm_group') { + return; + } //don't use api - you will get a loop $sql = " SELECT * FROM civicrm_group WHERE name LIKE '%pick%'"; $groups = []; @@ -734,7 +737,7 @@ class CRM_Group_Page_AjaxTest extends CiviUnitTestCase { `name`, `entity_table`, `entity_id`, `operation`, `object_table`, `object_id`, `is_active` ) VALUES ( - 'core-580', 'civicrm_acl_role', 55, 'Edit', 'civicrm_saved_search', 0, 1 + 'core-580', 'civicrm_acl_role', 55, 'Edit', 'civicrm_group', 0, 1 ); "); diff --git a/tests/phpunit/CRM/Mailing/BAO/MailingTest.php b/tests/phpunit/CRM/Mailing/BAO/MailingTest.php index 93d2289539..9af4831d16 100644 --- a/tests/phpunit/CRM/Mailing/BAO/MailingTest.php +++ b/tests/phpunit/CRM/Mailing/BAO/MailingTest.php @@ -225,6 +225,9 @@ class CRM_Mailing_BAO_MailingTest extends CiviUnitTestCase { * @param array $currentGroups */ public function hook_civicrm_aclGroup($type, $contactID, $tableName, &$allGroups, &$currentGroups) { + if ($tableName !== 'civicrm_group') { + return; + } //don't use api - you will get a loop $sql = " SELECT * FROM civicrm_group"; $groups = []; diff --git a/tests/phpunit/CiviTest/CiviUnitTestCase.php b/tests/phpunit/CiviTest/CiviUnitTestCase.php index df2a5f2774..48eb3612f9 100644 --- a/tests/phpunit/CiviTest/CiviUnitTestCase.php +++ b/tests/phpunit/CiviTest/CiviUnitTestCase.php @@ -2182,7 +2182,7 @@ class CiviUnitTestCase extends PHPUnit\Framework\TestCase { `name`, `entity_table`, `entity_id`, `operation`, `object_table`, `object_id`, `is_active` ) VALUES ( - 'view picked', 'civicrm_group', $this->_permissionedGroup , 'Edit', 'civicrm_saved_search', {$this->_permissionedGroup}, 1 + 'view picked', 'civicrm_group', $this->_permissionedGroup , 'Edit', 'civicrm_group', {$this->_permissionedGroup}, 1 ); "); @@ -2191,7 +2191,7 @@ class CiviUnitTestCase extends PHPUnit\Framework\TestCase { `name`, `entity_table`, `entity_id`, `operation`, `object_table`, `object_id`, `is_active` ) VALUES ( - 'view picked', 'civicrm_group', $this->_permissionedGroup, 'Edit', 'civicrm_saved_search', {$this->_permissionedDisabledGroup}, 1 + 'view picked', 'civicrm_group', $this->_permissionedGroup, 'Edit', 'civicrm_group', {$this->_permissionedDisabledGroup}, 1 ); "); } diff --git a/tests/phpunit/api/v3/GroupTest.php b/tests/phpunit/api/v3/GroupTest.php index 70b542dfd9..26f721a6c1 100644 --- a/tests/phpunit/api/v3/GroupTest.php +++ b/tests/phpunit/api/v3/GroupTest.php @@ -362,8 +362,10 @@ class api_v3_GroupTest extends CiviUnitTestCase { * @param array $ids */ public function aclGroupAllGroups($type, $contactID, $tableName, $allGroups, &$ids) { - $group = $this->callAPISuccess('Group', 'get', ['name' => 'Test Group 1']); - $ids = array_keys($group['values']); + if ($tableName === 'civicrm_group') { + $group = $this->callAPISuccess('Group', 'get', ['name' => 'Test Group 1']); + $ids = array_keys($group['values']); + } } } diff --git a/tests/phpunit/api/v3/RelationshipTest.php b/tests/phpunit/api/v3/RelationshipTest.php index 91ace3a2e3..09b6d694a8 100644 --- a/tests/phpunit/api/v3/RelationshipTest.php +++ b/tests/phpunit/api/v3/RelationshipTest.php @@ -1477,8 +1477,7 @@ class api_v3_RelationshipTest extends CiviUnitTestCase { * this from civicrm_generated.mysql */ private function setUpACLByCheating() { - CRM_Core_DAO::executeQuery("INSERT INTO civicrm_acl (name, deny, entity_table, entity_id, operation, object_table, object_id, acl_table, acl_id, is_active) VALUES ('Edit All Contacts', 0, 'civicrm_acl_role', 1, 'Edit', 'civicrm_saved_search', 0, NULL, NULL, 1)"); - CRM_Core_DAO::executeQuery("INSERT INTO civicrm_acl (name, deny, entity_table, entity_id, operation, object_table, object_id, acl_table, acl_id, is_active) VALUES ('Core ACL',0,'civicrm_acl_role',0,'All','access CiviMail subscribe/unsubscribe pages',NULL,NULL,NULL,1)"); + CRM_Core_DAO::executeQuery("INSERT INTO civicrm_acl (name, deny, entity_table, entity_id, operation, object_table, object_id, acl_table, acl_id, is_active) VALUES ('Edit All Contacts', 0, 'civicrm_acl_role', 1, 'Edit', 'civicrm_group', 0, NULL, NULL, 1)"); } } diff --git a/tests/phpunit/api/v3/ReportTemplateTest.php b/tests/phpunit/api/v3/ReportTemplateTest.php index 723dd7f7c3..f824fd346d 100644 --- a/tests/phpunit/api/v3/ReportTemplateTest.php +++ b/tests/phpunit/api/v3/ReportTemplateTest.php @@ -1811,7 +1811,9 @@ class api_v3_ReportTemplateTest extends CiviUnitTestCase { * @param array $ids */ public function aclGroupOnly($type, $contactID, $tableName, $allGroups, &$ids) { - $ids = [$this->aclGroupID]; + if ($tableName === 'civicrm_group') { + $ids = [$this->aclGroupID]; + } } /** diff --git a/xml/templates/civicrm_acl.tpl b/xml/templates/civicrm_acl.tpl index 0877ac3d46..afa070c7a5 100644 --- a/xml/templates/civicrm_acl.tpl +++ b/xml/templates/civicrm_acl.tpl @@ -14,7 +14,7 @@ -- Create ACL to edit and view contacts in all groups INSERT INTO civicrm_acl (name, deny, entity_table, entity_id, operation, object_table, object_id, acl_table, acl_id, is_active, priority) VALUES -('Edit All Contacts', 0, 'civicrm_acl_role', 1, 'Edit', 'civicrm_saved_search', 0, NULL, NULL, 1, 1); +('Edit All Contacts', 0, 'civicrm_acl_role', 1, 'Edit', 'civicrm_group', 0, NULL, NULL, 1, 1); -- Create default Groups for User Permissioning INSERT INTO civicrm_group (`id`, `name`, `title`, `description`, `source`, `saved_search_id`, `is_active`, `visibility`, `group_type`) -- 2.25.1