From c30d636417840e484df758aa598db764e48f355b Mon Sep 17 00:00:00 2001 From: Seamus Lee Date: Tue, 11 Apr 2023 11:18:41 +1000 Subject: [PATCH 1/3] [WIP] Permit ACL rules that negate (deny) Further fixes Expose the deny field on the ACL form and fix current test failures Update lanague as per Coleman --- CRM/ACL/BAO/ACL.php | 57 ++++++++++++++++++++++++++++++++++ CRM/ACL/Form/ACL.php | 5 ++- CRM/ACL/Page/ACL.php | 1 + templates/CRM/ACL/Form/ACL.tpl | 6 +++- templates/CRM/ACL/Page/ACL.tpl | 2 ++ 5 files changed, 69 insertions(+), 2 deletions(-) diff --git a/CRM/ACL/BAO/ACL.php b/CRM/ACL/BAO/ACL.php index 6668ff4e3368..154b17835724 100644 --- a/CRM/ACL/BAO/ACL.php +++ b/CRM/ACL/BAO/ACL.php @@ -236,6 +236,7 @@ public static function whereClause($type, &$tables, &$whereTables, $contactID = AND a.is_active = 1 AND a.object_table = 'civicrm_saved_search' AND a.id IN ( $aclKeys ) + AND a.deny = 0 ORDER BY a.object_id "; @@ -254,6 +255,21 @@ public static function whereClause($type, &$tables, &$whereTables, $contactID = $ids[] = $dao->object_id; } } + $denyQuery = "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.id IN ( $aclKeys ) + AND a.deny = 1 + AND a.object_id IN (%1) +ORDER BY a.object_id +"; + $denyDao = CRM_Core_DAO::executeQuery($denyQuery, [1 => [implode(',', $ids), 'CommaSeparatedIntegers']]); + while ($denyDao->fetch()) { + $key = array_search($denyDao->object_id, $ids); + unset($ids[$key]); + } if (!empty($ids)) { $ids = implode(',', $ids); @@ -347,6 +363,8 @@ public static function group( $ids = $cache->get($cacheKey); if (!is_array($ids)) { $ids = self::loadPermittedIDs((int) $contactID, $tableName, $type, $allGroups); + $denyIds = self::loadDenyIDs((int) $contactID, $tableName, $type, $allGroups); + $ids = array_diff($ids, $denyIds); $cache->set($cacheKey, $ids); } } @@ -468,6 +486,7 @@ protected static function loadPermittedIDs(int $contactID, string $tableName, in AND a.is_active = 1 AND a.object_table = %1 AND a.id IN ( $aclKeys ) + AND a.deny = 0 GROUP BY a.operation,a.object_id ORDER BY a.object_id "; @@ -493,4 +512,42 @@ protected static function loadPermittedIDs(int $contactID, string $tableName, in return $ids; } + /** + * Load deny acl IDs + * + * @param int $contactID + * @param string $tableName + * @param int $type + * @param array $allGroups + * + * @return array + */ + private static function loadDenyIDs(int $contactID, string $tableName, int $type, $allGroups): array { + $ids = []; + $acls = CRM_ACL_BAO_Cache::build($contactID); + $aclKeys = array_keys($acls); + $aclKeys = implode(',', $aclKeys); + $query = " +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 = %1 + AND a.id IN ( $aclKeys ) + AND a.deny = 1 +GROUP BY a.operation,a.object_id +ORDER BY a.object_id +"; + $params = [1 => [$tableName, 'String']]; + $dao = CRM_Core_DAO::executeQuery($query, $params); + while ($dao->fetch()) { + if ($dao->object_id) { + if (self::matchType($type, $dao->operation)) { + $ids[] = $dao->object_id; + } + } + } + return $ids; + } + } diff --git a/CRM/ACL/Form/ACL.php b/CRM/ACL/Form/ACL.php index 809793014a01..10e6369f5050 100644 --- a/CRM/ACL/Form/ACL.php +++ b/CRM/ACL/Form/ACL.php @@ -160,6 +160,10 @@ public function buildQuickForm() { $this->add('select', 'event_id', ts('Event'), $event); $this->add('checkbox', 'is_active', ts('Enabled?')); + $this->addRadio('deny', ts('Mode'), [ + 0 => ts('Allow'), + 1 => ts('Deny'), + ]); $this->addFormRule(['CRM_ACL_Form_ACL', 'formRule']); } @@ -253,7 +257,6 @@ public function postProcess() { else { $params = $this->controller->exportValues($this->_name); $params['is_active'] = CRM_Utils_Array::value('is_active', $params, FALSE); - $params['deny'] = 0; $params['entity_table'] = 'civicrm_acl_role'; // Figure out which type of object we're permissioning on and set object_table and object_id. diff --git a/CRM/ACL/Page/ACL.php b/CRM/ACL/Page/ACL.php index 5e4e6e971a78..8c3b3b79a1e5 100644 --- a/CRM/ACL/Page/ACL.php +++ b/CRM/ACL/Page/ACL.php @@ -136,6 +136,7 @@ public function browse() { $acl[$dao->id]['object_table'] = $dao->object_table; $acl[$dao->id]['object_id'] = $dao->object_id; $acl[$dao->id]['is_active'] = $dao->is_active; + $acl[$dao->id]['deny'] = $dao->deny; if ($acl[$dao->id]['entity_id']) { $acl[$dao->id]['entity'] = $roles[$acl[$dao->id]['entity_id']] ?? NULL; diff --git a/templates/CRM/ACL/Form/ACL.tpl b/templates/CRM/ACL/Form/ACL.tpl index f27f8974bde9..583be52ac03a 100644 --- a/templates/CRM/ACL/Form/ACL.tpl +++ b/templates/CRM/ACL/Form/ACL.tpl @@ -32,7 +32,7 @@ {$form.operation.label} {$form.operation.html}
- {ts}What type of operation (action) is being permitted?{/ts} + {ts}What type of operation (action) is being referenced?{/ts} @@ -45,6 +45,10 @@
{ts}IMPORTANT: The Drupal permissions for 'access all custom data' and 'profile listings and forms' override and disable specific ACL settings for custom field groups and profiles respectively. Do not enable those Drupal permissions for a Drupal role if you want to use CiviCRM ACL's to control access.{/ts}
{/if} + + {$form.deny.label} + {$form.deny.html} +
diff --git a/templates/CRM/ACL/Page/ACL.tpl b/templates/CRM/ACL/Page/ACL.tpl index d1e07327ce0a..5d8e168b3f5c 100644 --- a/templates/CRM/ACL/Page/ACL.tpl +++ b/templates/CRM/ACL/Page/ACL.tpl @@ -30,6 +30,7 @@ + @@ -42,6 +43,7 @@ + {/foreach} From 0f7879d53126d0ba3ef8ab2177df7177c1e4b191 Mon Sep 17 00:00:00 2001 From: Seamus Lee Date: Mon, 19 Jun 2023 10:14:33 +1000 Subject: [PATCH 2/3] Add unit test of negative ACL rules --- tests/phpunit/api/v3/ACLPermissionTest.php | 74 +++++++++++++++++++++- 1 file changed, 73 insertions(+), 1 deletion(-) diff --git a/tests/phpunit/api/v3/ACLPermissionTest.php b/tests/phpunit/api/v3/ACLPermissionTest.php index e5dd1ec6e910..5be31665708f 100644 --- a/tests/phpunit/api/v3/ACLPermissionTest.php +++ b/tests/phpunit/api/v3/ACLPermissionTest.php @@ -9,6 +9,7 @@ +--------------------------------------------------------------------+ */ +use Civi\Api4\ACLEntityRole; use Civi\Api4\Contact; use Civi\Api4\CustomField; use Civi\Api4\CustomGroup; @@ -1187,7 +1188,6 @@ public function testApi4CustomGroupACL(): void { ) ->execute()->single()['id']; } - $this->createLoggedInUser(); $this->aclGroupHookType = 'civicrm_custom_group'; CRM_Core_Config::singleton()->userPermissionClass->permissions = [ @@ -1244,6 +1244,78 @@ public function testApi4CustomGroupACL(): void { $this->assertEquals('Custom_extra_group_4', $getEntities[0]['name']); } + /** + * @throws \CRM_Core_Exception + */ + public function testNegativeCustomGroupACL(): void { + // Create 2 multi-record custom entities and 2 regular custom fields + $customGroups = []; + foreach ([1, 2, 3, 4] as $i) { + $customGroups[$i] = CustomGroup::create(FALSE) + ->addValue('title', "negative_extra_group_$i") + ->addValue('extends', 'Contact') + ->addValue('is_multiple', $i >= 3) + ->addChain('field', CustomField::create() + ->addValue('label', "negative_extra_field_$i") + ->addValue('custom_group_id', '$id') + ->addValue('html_type', 'Text') + ->addValue('data_type', 'String') + ) + ->execute()->single()['id']; + $this->callAPISuccess('Acl', 'create', [ + 'name' => 'Permit everyone to access custom group ' . $customGroups[$i], + 'deny' => 0, + 'entity_table' => 'civicrm_acl_role', + 'entity_id' => 0, + 'operation' => 'Edit', + 'object_table' => 'civicrm_custom_group', + 'object_id' => $customGroups[$i], + ]); + } + + $this->callAPISuccess('OptionValue', 'create', [ + 'option_group_id' => 'acl_role', + 'label' => 'Test Negative ACL Role', + 'value' => 4, + 'is_active' => 1, + ]); + $aclGroup = $this->groupCreate(); + ACLEntityRole::create(FALSE)->setValues([ + 'acl_role_id' => 4, + 'entity_table' => 'civicrm_group', + 'entity_id' => $aclGroup, + 'is_active' => 1, + ])->execute(); + $this->callAPISuccess('Acl', 'create', [ + 'name' => 'Test Negative ACL', + 'deny' => 1, + 'entity_table' => 'civicrm_acl_role', + 'entity_id' => 4, + 'operation' => 'Edit', + 'object_table' => 'civicrm_custom_group', + 'object_id' => $customGroups[2], + ]); + $userID = $this->createLoggedInUser(); + CRM_Core_Config::singleton()->userPermissionClass->permissions = [ + 'access CiviCRM', + 'view my contact', + ]; + $this->callAPISuccess('GroupContact', 'create', [ + 'contact_id' => $userID, + 'group_id' => $aclGroup, + 'status' => 'Added', + ]); + Civi::cache('metadata')->clear(); + Civi::$statics['CRM_ACL_BAO_ACL'] = []; + $getFields = Contact::getFields() + ->addWhere('name', 'LIKE', 'negative_extra_group_%.negative_extra_field_%') + ->execute(); + $this->assertCount(1, $getFields); + + Civi::cache('metadata')->clear(); + Civi::$statics['CRM_ACL_BAO_ACL'] = []; + } + public function aclGroupHookAllResults($action, $contactID, $tableName, &$allGroups, &$currentGroups) { if ($tableName === $this->aclGroupHookType) { $currentGroups = array_keys($allGroups); From f6b845d1cc507ee291a233adc64c806cd622f6de Mon Sep 17 00:00:00 2001 From: colemanw Date: Sun, 18 Jun 2023 20:28:44 -0400 Subject: [PATCH 3/3] Fix capitalization in ACL form --- templates/CRM/ACL/Page/ACL.tpl | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/templates/CRM/ACL/Page/ACL.tpl b/templates/CRM/ACL/Page/ACL.tpl index 5d8e168b3f5c..f5b54b9c260e 100644 --- a/templates/CRM/ACL/Page/ACL.tpl +++ b/templates/CRM/ACL/Page/ACL.tpl @@ -43,7 +43,7 @@ - + {/foreach}
{ts}Which Data{/ts} {ts}Description{/ts} {ts}Enabled?{/ts}{ts}Mode{/ts}
{$row.object} {$row.name} {if $row.is_active eq 1} {ts}Yes{/ts} {else} {ts}No{/ts} {/if}{if $row.deny eq 1} {ts}Deny{/ts} {else} {ts}allow{/ts} {/if} {$row.action|replace:'xx':$aclID}
{$row.object} {$row.name} {if $row.is_active eq 1} {ts}Yes{/ts} {else} {ts}No{/ts} {/if}{if $row.deny eq 1} {ts}Deny{/ts} {else} {ts}allow{/ts} {/if}{if $row.deny}{ts}Deny{/ts}{else}{ts}Allow{/ts}{/if} {$row.action|replace:'xx':$aclID}