Add "priority" column to ACLs and support ACL rule precedence #26592

seamuslee001 · 2023-06-21T23:00:19Z

Overview

This extends the work done by #26041 by adding in the concept of a priority to ACL rules. This allows for allow or deny rules to take precedence by the priority they are given.

Before

Deny rules always override allow rules without any real flexibility

After

Flexibility

ping @colemanw

civibot · 2023-06-21T23:00:22Z

(Standard links)

If you are reviewing this pull-request, you may wish to consult the test sites and the Review Standards (long template, short template).

colemanw · 2023-06-22T02:52:56Z

CRM/ACL/BAO/ACL.php

  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
+GROUP BY a.operation,a.object_id, a.weight


Why are you grouping by weight? I would think you would want to order by weight instead.

colemanw · 2023-06-22T02:53:02Z

CRM/ACL/BAO/ACL.php

@@ -530,22 +530,37 @@ private static function loadDenyIDs(int $contactID, string $tableName, int $type
    $aclKeys = array_keys($acls);
    $aclKeys = implode(',', $aclKeys);
    $query = "
-SELECT   a.operation, a.object_id
+SELECT   a.operation, a.object_id, a.weight


I'm a little concerned about this query causing a crash in an upgrade situation when the column hasn't been added yet. DAO::getSupportedFields() is your friend.

colemanw · 2023-06-22T16:42:12Z

CRM/ACL/Form/ACL.php

@@ -164,6 +165,7 @@ public function buildQuickForm() {
      0 => ts('Allow'),
      1 => ts('Deny'),
    ]);
+    $this->add('text', 'priority', ts('Priority'));


Suggested change

$this->add('text', 'priority', ts('Priority'));

$this->add('number', 'priority', ts('Priority'));

colemanw · 2023-06-22T16:42:47Z

templates/CRM/ACL/Page/ACL.tpl

@@ -31,6 +31,7 @@
              <th>{ts}Description{/ts}</th>
              <th>{ts}Enabled?{/ts}</th>
              <th>{ts}Mode{/ts}</th>
+              <th>{ts}Weight{/ts}</th>


Suggested change

<th>{ts}Weight{/ts}</th>

<th>{ts}Priority{/ts}</th>

colemanw · 2023-06-22T16:44:28Z

CRM/Upgrade/Incremental/php/FiveSixtyFour.php

@@ -28,6 +28,7 @@ class CRM_Upgrade_Incremental_php_FiveSixtyFour extends CRM_Upgrade_Incremental_
   *   The version number matching this function name
   */
  public function upgrade_5_64_alpha1($rev): void {
+    $this->addTask('Add weight column onto ACL table', 'addColumn', 'civicrm_acl', 'priority', 'int NOT NULL DEFAULT 0');


Suggested change

$this->addTask('Add weight column onto ACL table', 'addColumn', 'civicrm_acl', 'priority', 'int NOT NULL DEFAULT 0');

$this->addTask('Add priority column onto ACL table', 'addColumn', 'civicrm_acl', 'priority', 'int NOT NULL DEFAULT 0');

Add in upgrade step Rename field to be priority and add in upgrade guards as per Coleman Updates as per discussion with coleman

colemanw · 2023-06-22T17:31:47Z

CRM/ACL/BAO/ACL.php

+    $orderBy = 'a.object_id';
+    $hasPriorty = FALSE;
+    if (array_key_exists('priority', CRM_ACL_BAO_ACL::getSupportedFields())) {
+      $select .= ',a.priority';


I don't think this needs to be selected.

colemanw · 2023-06-22T17:37:10Z

CRM/ACL/BAO/ACL.php

+    if (array_key_exists('priority', CRM_ACL_BAO_ACL::getSupportedFields())) {
+      $select .= ',a.priority';
+      $orderBy .= ',a.priority';
+      $hasPriority = TRUE;


Unused variable?

…function as per Coleman

colemanw · 2023-06-22T19:33:21Z

Ok great, this has passed review and tests are passing.

TobiasVoigt · 2023-09-08T16:56:16Z

Hi there,

today I tested the newest implementation of the weighting functionality in CiviCRM 5.65.0 and found that I wasn't able to set up my initial usecase due to what seems to be a bug.

My usecase included three ACLs:

ACL rule 1: Allow access to all custom field groups for all authenticated users → Priority 1
ACL rule 2: Disallow access to the privileged set of custom fields for all authenticated users → Priority 2
ACL rule 3: Allow access to the privileged set of custom fields for a privileged role → Priority 3

I used the new weighting functionality but didn't get the results I expected. I expected that priority 3 would have precedence over 2 (and 2 over 1), in other words: The higher the number, the higher the priority. But with this setup all users where able to access all custom fields.

I then made some tests and realized:

When I create a test setup with several rules that all refer to ONE SPECIFIC custom field group the weighting logic works like I expected: The higher number has the priority.
When I create a test setup with several rules that all refer to ALL custom field groups, the weighting logic seems to be flipped: The smaller number has the higher priority.

So my guess is that somewhere in the ACL code the weighting logic is somehow flipped for "All custom field groups". This would explain why mixing rules for "All custom field groups" and rules for specific custom field groups in my test setup didn't work.

To my regret, this means I won't be able to show the new functionality to a live audience at CiviCamp in Leipzig next monday (2023-09-11). If there's any chance someone would be able to deliver a quick fix that would be absolutely awesome.

In any case - I hope my analysis makes sense and shed some light onto a potential bug. Let me know! :)

Best regards,

Tobias

civibot bot added the master label Jun 21, 2023

seamuslee001 force-pushed the acl_rule_weight branch from a3b4a1a to 7b92ef6 Compare June 21, 2023 23:25

colemanw reviewed Jun 22, 2023

View reviewed changes

colemanw changed the title ~~Add weight column to ACLs and support weighted ACL rules~~ Add "priority" column to ACLs and support ACL rule precedence Jun 22, 2023

colemanw reviewed Jun 22, 2023

View reviewed changes

Add weight column to ACLs and support weighted ACL rules

bc6e647

Add in upgrade step Rename field to be priority and add in upgrade guards as per Coleman Updates as per discussion with coleman

seamuslee001 force-pushed the acl_rule_weight branch from 66ccb70 to bc6e647 Compare June 22, 2023 16:46

colemanw reviewed Jun 22, 2023

View reviewed changes

Refactor to add deny and weighting aspects into the loadPermittedIDs …

b2f5462

…function as per Coleman

seamuslee001 force-pushed the acl_rule_weight branch from 2061111 to b2f5462 Compare June 22, 2023 18:28

colemanw merged commit 7eb8afe into civicrm:master Jun 22, 2023

colemanw mentioned this pull request Jun 22, 2023

Permit ACL rules that negate (deny) access #26041

Merged

seamuslee001 deleted the acl_rule_weight branch July 8, 2023 00:44

This was referenced Sep 9, 2023

dev/core#4542 Fix priority handling in ACLs when dealing with objects… #27380

Closed

dev/core#4542 Fix priority handling in ACLs when dealing with objects… #27381

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add "priority" column to ACLs and support ACL rule precedence #26592

Add "priority" column to ACLs and support ACL rule precedence #26592

seamuslee001 commented Jun 21, 2023 •

edited by colemanw

Loading

civibot bot commented Jun 21, 2023

colemanw Jun 22, 2023

colemanw Jun 22, 2023

colemanw Jun 22, 2023

colemanw Jun 22, 2023

colemanw Jun 22, 2023

colemanw Jun 22, 2023

colemanw Jun 22, 2023

colemanw commented Jun 22, 2023

TobiasVoigt commented Sep 8, 2023

	$this->add('text', 'priority', ts('Priority'));
	$this->add('number', 'priority', ts('Priority'));

	$this->addTask('Add weight column onto ACL table', 'addColumn', 'civicrm_acl', 'priority', 'int NOT NULL DEFAULT 0');
	$this->addTask('Add priority column onto ACL table', 'addColumn', 'civicrm_acl', 'priority', 'int NOT NULL DEFAULT 0');

Add "priority" column to ACLs and support ACL rule precedence #26592

Add "priority" column to ACLs and support ACL rule precedence #26592

Conversation

seamuslee001 commented Jun 21, 2023 • edited by colemanw Loading

Overview

Before

After

civibot bot commented Jun 21, 2023

colemanw Jun 22, 2023

Choose a reason for hiding this comment

colemanw Jun 22, 2023

Choose a reason for hiding this comment

colemanw Jun 22, 2023

Choose a reason for hiding this comment

colemanw Jun 22, 2023

Choose a reason for hiding this comment

colemanw Jun 22, 2023

Choose a reason for hiding this comment

colemanw Jun 22, 2023

Choose a reason for hiding this comment

colemanw Jun 22, 2023

Choose a reason for hiding this comment

colemanw commented Jun 22, 2023

TobiasVoigt commented Sep 8, 2023

seamuslee001 commented Jun 21, 2023 •

edited by colemanw

Loading