Merge pull request #26592 from seamuslee001/acl_rule_weight

Add "priority" column to ACLs and support ACL rule precedence
civicrm · Jun 22, 2023 · 7eb8afe · 7eb8afe
2 parents d2916c8 + b2f5462
commit 7eb8afe
Show file tree

Hide file tree

Showing 9 changed files with 154 additions and 47 deletions.
diff --git a/CRM/ACL/BAO/ACL.php b/CRM/ACL/BAO/ACL.php
@@ -365,8 +365,6 @@ 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);
       }
     }
@@ -481,31 +479,43 @@ protected static function loadPermittedIDs(int $contactID, string $tableName, in
     $acls = CRM_ACL_BAO_Cache::build($contactID);
     $aclKeys = array_keys($acls);
     $aclKeys = implode(',', $aclKeys);
+    $orderBy = 'a.object_id';
+    if (array_key_exists('priority', CRM_ACL_BAO_ACL::getSupportedFields())) {
+      $orderBy .= ',a.priority';
+    }
     $query = "
-SELECT   a.operation, a.object_id
+SELECT   a.operation,a.object_id,a.deny
   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         = 0
-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;
+          if (!$dao->deny) {
+            $ids[] = $dao->object_id;
+          }
+          else {
+            $ids = array_diff($ids, [$dao->object_id]);
+          }
         }
       }
       else {
         // this user has got the permission for all objects of this type
         // check if the type matches
         if (self::matchType($type, $dao->operation)) {
-          foreach ($allGroups as $id => $dontCare) {
-            $ids[] = $id;
+          if (!$dao->deny) {
+            foreach ($allGroups as $id => $dontCare) {
+              $ids[] = $id;
+            }
+          }
+          else {
+            $ids = array_diff($ids, array_keys($allGroups));
           }
         }
         break;
@@ -514,42 +524,4 @@ 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/DAO/ACL.php b/CRM/ACL/DAO/ACL.php
@@ -6,7 +6,7 @@
  *
  * Generated from xml/schema/CRM/ACL/ACL.xml
  * DO NOT EDIT.  Generated by CRM_Core_CodeGen
- * (GenCodeChecksum:f21ef3073d6247d130341cd182793ea6)
+ * (GenCodeChecksum:9d50ed80344474830f87df285dc6cbf2)
  */
 
 /**
@@ -129,6 +129,13 @@ class CRM_ACL_DAO_ACL extends CRM_Core_DAO {
    */
   public $is_active;
 
+  /**
+   * @var int|string
+   *   (SQL type: int)
+   *   Note that values will be retrieved from the database as a string.
+   */
+  public $priority;
+
   /**
    * Class constructor.
    */
@@ -403,6 +410,28 @@ public static function &fields() {
           ],
           'add' => '1.6',
         ],
+        'priority' => [
+          'name' => 'priority',
+          'type' => CRM_Utils_Type::T_INT,
+          'title' => ts('Priority'),
+          'required' => TRUE,
+          'usage' => [
+            'import' => FALSE,
+            'export' => FALSE,
+            'duplicate_matching' => FALSE,
+            'token' => FALSE,
+          ],
+          'where' => 'civicrm_acl.priority',
+          'default' => '0',
+          'table_name' => 'civicrm_acl',
+          'entity' => 'ACL',
+          'bao' => 'CRM_ACL_BAO_ACL',
+          'localizable' => 0,
+          'html' => [
+            'type' => 'Number',
+          ],
+          'add' => '5.64',
+        ],
       ];
       CRM_Core_DAO_AllCoreTables::invoke(__CLASS__, 'fields_callback', Civi::$statics[__CLASS__]['fields']);
     }
@@ -481,6 +510,14 @@ public static function indices($localize = TRUE) {
         'localizable' => FALSE,
         'sig' => 'civicrm_acl::0::acl_id',
       ],
+      'index_priority' => [
+        'name' => 'index_priority',
+        'field' => [
+          0 => 'priority',
+        ],
+        'localizable' => FALSE,
+        'sig' => 'civicrm_acl::0::priority',
+      ],
     ];
     return ($localize && !empty($indices)) ? CRM_Core_DAO_AllCoreTables::multilingualize(__CLASS__, $indices) : $indices;
   }

diff --git a/CRM/ACL/Form/ACL.php b/CRM/ACL/Form/ACL.php
@@ -29,6 +29,7 @@ public function setDefaultValues() {
 
     if ($this->_action & CRM_Core_Action::ADD) {
       $defaults['object_type'] = 1;
+      $defaults['priority'] = 0;
     }
 
     $showHide = new CRM_Core_ShowHideBlocks();
@@ -164,6 +165,7 @@ public function buildQuickForm() {
       0 => ts('Allow'),
       1 => ts('Deny'),
     ]);
+    $this->add('number', 'priority', ts('Priority'));
 
     $this->addFormRule(['CRM_ACL_Form_ACL', 'formRule']);
   }

diff --git a/CRM/ACL/Page/ACL.php b/CRM/ACL/Page/ACL.php
@@ -137,6 +137,7 @@ public function browse() {
       $acl[$dao->id]['object_id'] = $dao->object_id;
       $acl[$dao->id]['is_active'] = $dao->is_active;
       $acl[$dao->id]['deny'] = $dao->deny;
+      $acl[$dao->id]['priority'] = $dao->priority;
 
       if ($acl[$dao->id]['entity_id']) {
         $acl[$dao->id]['entity'] = $roles[$acl[$dao->id]['entity_id']] ?? NULL;

diff --git a/CRM/Upgrade/Incremental/php/FiveSixtyFour.php b/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 priority column onto ACL table', 'addColumn', 'civicrm_acl', 'priority', 'int NOT NULL DEFAULT 0');
     $this->addTask(ts('Upgrade DB to %1: SQL', [1 => $rev]), 'runSql', $rev);
     $this->addTask('Update post_URL/cancel_URL in logging tables', 'updateLogging');
   }

diff --git a/templates/CRM/ACL/Form/ACL.tpl b/templates/CRM/ACL/Form/ACL.tpl
@@ -49,6 +49,12 @@
        <td class="label">{$form.deny.label}</td>
        <td>{$form.deny.html}</td>
      </tr>
+     <tr class="crm-acl-form-block-priority">
+       <td class="label">{$form.priority.label}</td>
+       <td>{$form.priority.label}<br />
+         <span class="description">{ts}Higher priority ACL rules will override lower priority rules{/ts}</span>
+       </td>
+     </tr>
   </table>
   <div id="id-group-acl">
    <table  class="form-layout-compressed">

diff --git a/templates/CRM/ACL/Page/ACL.tpl b/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}Priority{/ts}</th>
               <th></th>
             </tr>
             </thead>
@@ -44,6 +45,7 @@
                 <td class="crm-acl-name crm-editable" data-field="name">{$row.name}</td>
                 <td class="crm-acl-is_active" id="row_{$aclID}_status">{if $row.is_active eq 1} {ts}Yes{/ts} {else} {ts}No{/ts} {/if}</td>
                 <td class="crm-acl-deny" id="row_{$aclID}_deny">{if $row.deny}{ts}Deny{/ts}{else}{ts}Allow{/ts}{/if}</td>
+                <td class="crm-acl-priority" id="row_{$aclID}_priority">{$row.priority}</td>
                 <td>{$row.action|replace:'xx':$aclID}</td>
               </tr>
             {/foreach}

diff --git a/tests/phpunit/api/v3/ACLPermissionTest.php b/tests/phpunit/api/v3/ACLPermissionTest.php
@@ -1316,6 +1316,78 @@ public function testNegativeCustomGroupACL(): void {
     Civi::$statics['CRM_ACL_BAO_ACL'] = [];
   }
 
+  /**
+   * @throws \CRM_Core_Exception
+   */
+  public function testPriorityCustomGroupACL(): void {
+    // Create 2 multi-record custom entities and 2 regular custom fields
+    $customGroups = [];
+    foreach ([1, 2] as $i) {
+      $customGroups[$i] = CustomGroup::create(FALSE)
+        ->addValue('title', "priority_extra_group_$i")
+        ->addValue('extends', 'Contact')
+        ->addValue('is_multiple', FALSE)
+        ->addChain('field', CustomField::create()
+          ->addValue('label', "priority_extra_field_$i")
+          ->addValue('custom_group_id', '$id')
+          ->addValue('html_type', 'Text')
+          ->addValue('data_type', 'String')
+        )
+        ->execute()->single()['id'];
+      $this->callAPISuccess('Acl', 'create', [
+        'name' => 'Deny everyone to access custom group ' . $customGroups[$i],
+        'entity_table' => 'civicrm_acl_role',
+        'entity_id' => 0,
+        'operation' => 'Edit',
+        'object_table' => 'civicrm_custom_group',
+        'object_id' => $customGroups[$i],
+        'deny' => 1,
+      ]);
+    }
+
+    $this->callAPISuccess('OptionValue', 'create', [
+      'option_group_id' => 'acl_role',
+      'label' => 'Test Priority ACL Role',
+      'value' => 5,
+      'is_active' => 1,
+    ]);
+    $aclGroup = $this->groupCreate();
+    ACLEntityRole::create(FALSE)->setValues([
+      'acl_role_id' => 5,
+      'entity_table' => 'civicrm_group',
+      'entity_id' => $aclGroup,
+      'is_active' => 1,
+    ])->execute();
+    $this->callAPISuccess('Acl', 'create', [
+      'name' => 'Test Postive Priority ACL',
+      'priority' => 1,
+      'entity_table' => 'civicrm_acl_role',
+      'entity_id' => 5,
+      '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', 'priority_extra_group_%.priority_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);

diff --git a/xml/schema/ACL/ACL.xml b/xml/schema/ACL/ACL.xml
@@ -126,4 +126,18 @@
       <label>Enabled</label>
     </html>
   </field>
+  <field>
+    <name>priority</name>
+    <type>int</type>
+    <default>0</default>
+    <required>true</required>
+    <add>5.64</add>
+    <html>
+      <type>Number</type>
+    </html>
+  </field>
+  <index>
+    <name>index_priority</name>
+    <fieldName>priority</fieldName>
+  </index>
 </table>