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

Fix ACLs to use the correct name of the civicrm_group table #26615

Merged
merged 1 commit into from
Jun 23, 2023
Merged

Fix ACLs to use the correct name of the civicrm_group table #26615

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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: 2 additions & 2 deletions CRM/ACL/API.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,7 +148,7 @@ public static function whereClause(
public static function group(
$type,
$contactID = NULL,
$tableName = 'civicrm_saved_search',
$tableName = 'civicrm_group',
$allGroups = NULL,
$includedGroups = []
) {
Expand Down Expand Up @@ -181,7 +181,7 @@ public static function groupPermission(
$type,
$groupID,
$contactID = NULL,
$tableName = 'civicrm_saved_search',
$tableName = 'civicrm_group',
$allGroups = NULL,
$includedGroups = NULL
) {
Expand Down
6 changes: 3 additions & 3 deletions CRM/ACL/BAO/ACL.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -234,7 +234,7 @@ public static function whereClause($type, &$tables, &$whereTables, $contactID =
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
Expand All @@ -259,7 +259,7 @@ public static function whereClause($type, &$tables, &$whereTables, $contactID =
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)
Expand Down Expand Up @@ -333,7 +333,7 @@ public static function whereClause($type, &$tables, &$whereTables, $contactID =
public static function group(
$type,
$contactID = NULL,
$tableName = 'civicrm_saved_search',
$tableName = 'civicrm_group',
$allGroups = NULL,
$includedGroups = []
) {
Expand Down
4 changes: 2 additions & 2 deletions CRM/ACL/Form/ACL.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ public function setDefaultValues() {

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");
Expand Down Expand Up @@ -265,7 +265,7 @@ public function postProcess() {
// 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;

Expand Down
2 changes: 1 addition & 1 deletion CRM/ACL/Page/ACL.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,7 +142,7 @@ public function browse() {
}

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;
Expand Down
2 changes: 1 addition & 1 deletion CRM/Bridge/OG/Drupal.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -175,7 +175,7 @@ public static function updateCiviACLEntityRole(&$params, $op) {
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') {
Expand Down
7 changes: 3 additions & 4 deletions CRM/Contact/BAO/Group.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -285,15 +285,15 @@ public static function checkPermission($id, $excludeHidden = FALSE) {
$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;
}

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;
Expand All @@ -316,8 +316,7 @@ public function addSelectWhereClause() {
$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)";
}
Expand Down
4 changes: 2 additions & 2 deletions CRM/Core/Permission/Base.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -169,7 +169,7 @@ public function group($groupType = NULL, $excludeHidden = TRUE) {
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');
Expand All @@ -178,7 +178,7 @@ public function group($groupType = NULL, $excludeHidden = TRUE) {
}
}

$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');
Expand Down
5 changes: 4 additions & 1 deletion CRM/Upgrade/Incremental/sql/5.64.alpha1.mysql.tpl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
15 changes: 12 additions & 3 deletions CRM/Utils/Hook.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -598,9 +598,7 @@ public static function aclWhereClause($type, &$tables, &$whereTables, &$contactI
* @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
Expand All @@ -611,6 +609,17 @@ public static function aclWhereClause($type, &$tables, &$whereTables, &$contactI
*/
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');
}
Expand Down
2 changes: 1 addition & 1 deletion Civi/Test/ACLPermissionTrait.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +125,7 @@ public function aclWhereGreaterThan($type, &$tables, &$whereTables, &$contactID,
* @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;
Expand Down
5 changes: 4 additions & 1 deletion tests/phpunit/CRM/Group/Page/AjaxTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -666,6 +666,9 @@ public function testGroupDontRegenerateSmartGroups() {
* @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 = [];
Expand Down Expand Up @@ -734,7 +737,7 @@ public function setupEditAllGroupsACL() {
`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
);
");

Expand Down
3 changes: 3 additions & 0 deletions tests/phpunit/CRM/Mailing/BAO/MailingTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -225,6 +225,9 @@ public function aclWhereAllowedOnlyOne($type, &$tables, &$whereTables, &$contact
* @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 = [];
Expand Down
4 changes: 2 additions & 2 deletions tests/phpunit/CiviTest/CiviUnitTestCase.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2182,7 +2182,7 @@ public function setupACL($isProfile = FALSE) {
`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
);
");

Expand All @@ -2191,7 +2191,7 @@ public function setupACL($isProfile = FALSE) {
`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
);
");
}
Expand Down
6 changes: 4 additions & 2 deletions tests/phpunit/api/v3/GroupTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -362,8 +362,10 @@ public function testGroupGetACLs($version) {
* @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']);
}
}

}
3 changes: 1 addition & 2 deletions tests/phpunit/api/v3/RelationshipTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1477,8 +1477,7 @@ public function testCreateWithLesserPermissions() {
* 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)");
Copy link
Contributor

Choose a reason for hiding this comment

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

@demeritcowboy why did you include this? :) object_id surely should be an id not a string right? :)

Copy link
Contributor

Choose a reason for hiding this comment

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

It's null, and I took it straight out of civicrm_generated, but the removed "Core ACL" line is necessary for the test above to work if I remember right. The test may now ALWAYS pass which wasn't the intent.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm open to doing the test a different way - it might be useful to have the test installs mirror more closely a real install for ALL tests.

Copy link
Member Author

@colemanw colemanw Jun 23, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@demeritcowboy the line I removed in the test is also being removed from real databases by this PR

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok I see. I'll do a followup after since the test isn't completely useless but it's not testing what it was testing and the ACL part of it now is a bit misleading.

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)");
}

}
4 changes: 3 additions & 1 deletion tests/phpunit/api/v3/ReportTemplateTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1811,7 +1811,9 @@ private function doQuestionableStuffInASeparateFunctionSoNobodyNotices(): void {
* @param array $ids
*/
public function aclGroupOnly($type, $contactID, $tableName, $allGroups, &$ids) {
$ids = [$this->aclGroupID];
if ($tableName === 'civicrm_group') {
$ids = [$this->aclGroupID];
}
}

/**
Expand Down
2 changes: 1 addition & 1 deletion xml/templates/civicrm_acl.tpl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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`)
Expand Down