Implement 'super permissions' as described by @totten

This picks up on an idea Tim has pushed several times - ie that instead of giving out 'Administer CiviCRM' willy nilly we could deprioritise it in favour of 2 more granular permission bundles - ie Administer CiviCRM data & administe CiviCRM system. This allows us to make some permissions more 'locked away' without endlessly adding new 'administer Payment Processors' because we've realised not everyone who can create profiles needs to be able to see payment processor credentials. It also allows us to make system checks less broadly visible where they are not appropriate. Note that to proceed with this we would need to go through all places that check Administer CiviCRM & put in one or both of the 2 new permissions. Having Administer CiviCRM implicitly includes anything granted to the existing permissions so the implementation is smooth-ish there. However, I can imagine we would need a hook allowing people to categorise themselves or we would find ourselves litigating all sorts
civicrm · Sep 8, 2020 · 755a183 · 755a183
1 parent d2ef210
commit 755a183
Show file tree

Hide file tree

Showing 10 changed files with 91 additions and 26 deletions.
diff --git a/CRM/Core/Permission.php b/CRM/Core/Permission.php
@@ -119,9 +119,17 @@ public static function check($permissions, $contactId = NULL) {
       }
       else {
         // This is an individual permission
-        $granted = CRM_Core_Config::singleton()->userPermissionClass->check($permission, $userId);
-        // Call the permission_check hook to permit dynamic escalation (CRM-19256)
-        CRM_Utils_Hook::permission_check($permission, $granted, $contactId);
+        $impliedPermissions = self::getImpliedPermissionsFor($permission);
+        $impliedPermissions[] = $permission;
+        foreach ($impliedPermissions as $permissionOption) {
+          $granted = CRM_Core_Config::singleton()->userPermissionClass->check($permissionOption, $userId);
+          // Call the permission_check hook to permit dynamic escalation (CRM-19256)
+          CRM_Utils_Hook::permission_check($permissionOption, $granted, $contactId);
+          if ($granted) {
+            break;
+          }
+        }
+
         if (
           !$granted
           && !($tempPerm && $tempPerm->check($permission))
@@ -893,11 +901,53 @@ public static function getCorePermissions() {
         $prefix . ts('send SMS'),
         ts('Send an SMS'),
       ],
+      'administer CiviCRM system' => [
+        'label' => $prefix . ts('administer CiviCRM System'),
+        'description' => ts('Perform all system administration tasks in CiviCRM:'),
+      ],
+      'administer CiviCRM data' => [
+        'label' => $prefix . ts('administer CiviCRM Data'),
+        'description' => ts('Perform all system administration tasks in CiviCRM:'),
+      ],
     ];
-
+    foreach (self::getImpliedPermissions() as $name => $includes) {
+      foreach ($includes as $permission) {
+        $permissions[$name][] = $permissions[$permission];
+      }
+    }
     return $permissions;
   }
 
+  /**
+   * Get permissions implied by 'superset' permissions.
+   *
+   * @return array
+   */
+  public static function getImpliedPermissions() {
+    return [
+      'administer CiviCRM' => ['administer CiviCRM system', 'administer CiviCRM data'],
+      'administer CiviCRM data' => ['edit message templates', 'administer dedupe rules'],
+      'administer CiviCRM system' => ['edit api keys', 'edit system workflow message templates', 'administer payment processors'],
+    ];
+  }
+
+  /**
+   * Get any super-permissions that imply the given permission.
+   *
+   * @param string $permission
+   *
+   * @return array
+   */
+  public static function getImpliedPermissionsFor(string $permission) {
+    $return = [];
+    foreach (self::getImpliedPermissions() as $superPermission => $components) {
+      if (in_array($permission, $components, TRUE)) {
+        $return[$superPermission] = $superPermission;
+      }
+    }
+    return $return;
+  }
+
   /**
    * For each entity provides an array of permissions required for each action
    *

diff --git a/CRM/Core/xml/Menu/Admin.xml b/CRM/Core/xml/Menu/Admin.xml
@@ -259,7 +259,7 @@
      <desc>Schedule Reminders.</desc>
      <page_callback>CRM_Admin_Page_ScheduleReminders</page_callback>
      <access_callback>1</access_callback>
-     <access_arguments>administer CiviCRM;edit all events</access_arguments>
+     <access_arguments>administer CiviCRM data;edit all events</access_arguments>
      <adminGroup>Communications</adminGroup>
      <weight>40</weight>
   </item>
@@ -397,22 +397,23 @@
      <title>Manage Extensions</title>
      <page_callback>CRM_Admin_Page_Extensions</page_callback>
      <desc></desc>
-     <access_arguments>administer CiviCRM</access_arguments>
+     <access_arguments>administer CiviCRM system</access_arguments>
      <adminGroup>System Settings</adminGroup>
      <weight>120</weight>
   </item>
   <item>
      <path>civicrm/admin/extensions/upgrade</path>
      <title>Database Upgrades</title>
      <page_callback>CRM_Admin_Page_ExtensionsUpgrade</page_callback>
-     <access_arguments>administer CiviCRM</access_arguments>
+     <access_arguments>administer CiviCRM system</access_arguments>
   </item>
   <item>
      <path>civicrm/admin/setting/smtp</path>
      <title>Outbound Email Settings</title>
      <page_callback>CRM_Admin_Form_Setting_Smtp</page_callback>
      <adminGroup>System Settings</adminGroup>
      <weight>20</weight>
+    <access_arguments>administer CiviCRM system</access_arguments>
   </item>
   <item>
      <path>civicrm/admin/paymentProcessor</path>
@@ -535,14 +536,14 @@
      <path>civicrm/admin/runjobs</path>
      <desc>URL used for running scheduled jobs.</desc>
      <page_callback>CRM_Utils_System::executeScheduledJobs</page_callback>
-     <access_arguments>access CiviCRM,administer CiviCRM</access_arguments>
+     <access_arguments>access CiviCRM,administer CiviCRM system</access_arguments>
   </item>
   <item>
      <path>civicrm/admin/job</path>
      <title>Scheduled Jobs</title>
      <desc>Managing periodially running tasks.</desc>
      <page_callback>CRM_Admin_Page_Job</page_callback>
-     <access_arguments>access CiviCRM,administer CiviCRM</access_arguments>
+     <access_arguments>access CiviCRM,administer CiviCRM system</access_arguments>
      <adminGroup>System Settings</adminGroup>
      <weight>1370</weight>
   </item>
@@ -551,7 +552,7 @@
      <title>Scheduled Jobs Log</title>
      <desc>Browsing the log of periodially running tasks.</desc>
      <page_callback>CRM_Admin_Page_JobLog</page_callback>
-     <access_arguments>access CiviCRM,administer CiviCRM</access_arguments>
+     <access_arguments>access CiviCRM,administer CiviCRM system</access_arguments>
      <adminGroup>Manage</adminGroup>
      <weight>1380</weight>
   </item>
@@ -573,7 +574,7 @@
   <item>
      <path>civicrm/admin</path>
      <title>Administer CiviCRM</title>
-     <access_arguments>administer CiviCRM,access CiviCRM</access_arguments>
+     <access_arguments>administer CiviCRM system,administer CiviCRM data,access CiviCRM</access_arguments>
      <page_type>1</page_type>
      <page_callback>CRM_Admin_Page_Admin</page_callback>
      <is_ssl>true</is_ssl>

diff --git a/CRM/Event/Form/ManageEvent.php b/CRM/Event/Form/ManageEvent.php
@@ -180,7 +180,7 @@ public function preProcess() {
     $ufEdit = CRM_ACL_API::group(CRM_Core_Permission::EDIT, NULL, 'civicrm_uf_group', $ufGroups);
     $checkPermission = [
       [
-        'administer CiviCRM',
+        'administer CiviCRM data',
         'manage event profiles',
       ],
     ];

diff --git a/CRM/Event/Form/ManageEvent/TabHeader.php b/CRM/Event/Form/ManageEvent/TabHeader.php
@@ -70,7 +70,7 @@ public static function process(&$form) {
     $tabs['registration'] = ['title' => ts('Online Registration')] + $default;
     // @fixme I don't understand the event permissions check here - can we just get rid of it?
     $permissions = CRM_Event_BAO_Event::getAllPermissions();
-    if (CRM_Core_Permission::check('administer CiviCRM') || !empty($permissions[CRM_Core_Permission::EDIT])) {
+    if (CRM_Core_Permission::check('administer CiviCRM data') || !empty($permissions[CRM_Core_Permission::EDIT])) {
       $tabs['reminder'] = ['title' => ts('Schedule Reminders'), 'class' => 'livePage'] + $default;
     }
     $tabs['conference'] = ['title' => ts('Conference Slots')] + $default;

diff --git a/CRM/Event/Form/Registration/Register.php b/CRM/Event/Form/Registration/Register.php
@@ -563,7 +563,7 @@ public static function buildAmount(&$form, $required = TRUE, $discountId = NULL)
 
       // CRM-14492 Admin price fields should show up on event registration if user has 'administer CiviCRM' permissions
       $adminFieldVisible = FALSE;
-      if (CRM_Core_Permission::check('administer CiviCRM')) {
+      if (CRM_Core_Permission::check('administer CiviCRM data')) {
         $adminFieldVisible = TRUE;
       }
 

diff --git a/CRM/Event/Page/ManageEvent.php b/CRM/Event/Page/ManageEvent.php
@@ -165,7 +165,7 @@ public static function &tabs() {
 
       // @fixme I don't understand the event permissions check here - can we just get rid of it?
       $permissions = CRM_Event_BAO_Event::getAllPermissions();
-      if (CRM_Core_Permission::check('administer CiviCRM') || !empty($permissions[CRM_Core_Permission::EDIT])) {
+      if (CRM_Core_Permission::check('administer CiviCRM data') || !empty($permissions[CRM_Core_Permission::EDIT])) {
         self::$_tabLinks[$cacheKey]['reminder']
           = [
             'title' => ts('Schedule Reminders'),

diff --git a/CRM/Grant/Page/DashBoard.php b/CRM/Grant/Page/DashBoard.php
@@ -27,7 +27,7 @@ class CRM_Grant_Page_DashBoard extends CRM_Core_Page {
    * @return void
    */
   public function preProcess() {
-    $admin = CRM_Core_Permission::check('administer CiviCRM');
+    $admin = CRM_Core_Permission::check('administer CiviCRM data');
 
     $grantSummary = CRM_Grant_BAO_Grant::getGrantSummary($admin);
 

diff --git a/CRM/UF/Form/Inline/Preview.php b/CRM/UF/Form/Inline/Preview.php
@@ -29,7 +29,7 @@ public function preProcess() {
     // Inline forms don't get menu-level permission checks
     $checkPermission = [
       [
-        'administer CiviCRM',
+        'administer CiviCRM data',
         'manage event profiles',
       ],
     ];

diff --git a/CRM/Utils/Check.php b/CRM/Utils/Check.php
@@ -64,7 +64,7 @@ public static function getSeverityList() {
    * Display daily system status alerts (admin only).
    */
   public function showPeriodicAlerts() {
-    if (CRM_Core_Permission::check('administer CiviCRM')) {
+    if (CRM_Core_Permission::check('administer CiviCRM system')) {
       $session = CRM_Core_Session::singleton();
       if ($session->timer('check_' . __CLASS__, self::CHECK_TIMER)) {
 

diff --git a/tests/phpunit/CRM/Core/Permission/BaseTest.php b/tests/phpunit/CRM/Core/Permission/BaseTest.php
@@ -6,40 +6,54 @@
  */
 class CRM_Core_Permission_BaseTest extends CiviUnitTestCase {
 
+  use CRMTraits_ACL_PermissionTrait;
+
   /**
    * @return array
    *   (0 => input to translatePermission, 1 => expected output from translatePermission)
    */
   public function translateData() {
     $cases = [];
 
-    $cases[] = ["administer CiviCRM", "administer CiviCRM"];
-    $cases[] = ["cms:universal name", "local name"];
-    $cases[] = ["cms:universal name2", "local name2"];
-    $cases[] = ["cms:unknown universal name", CRM_Core_Permission::ALWAYS_DENY_PERMISSION];
-    $cases[] = ["myruntime:foo", "foo"];
-    $cases[] = ["otherruntime:foo", CRM_Core_Permission::ALWAYS_DENY_PERMISSION];
-    $cases[] = ["otherruntime:foo:bar", CRM_Core_Permission::ALWAYS_DENY_PERMISSION];
+    $cases[] = ['administer CiviCRM', 'administer CiviCRM'];
+    $cases[] = ['cms:universal name', 'local name'];
+    $cases[] = ['cms:universal name2', 'local name2'];
+    $cases[] = ['cms:unknown universal name', CRM_Core_Permission::ALWAYS_DENY_PERMISSION];
+    $cases[] = ['myruntime:foo', 'foo'];
+    $cases[] = ['otherruntime:foo', CRM_Core_Permission::ALWAYS_DENY_PERMISSION];
+    $cases[] = ['otherruntime:foo:bar', CRM_Core_Permission::ALWAYS_DENY_PERMISSION];
     $cases[] = [CRM_Core_Permission::ALWAYS_ALLOW_PERMISSION, CRM_Core_Permission::ALWAYS_ALLOW_PERMISSION];
 
     return $cases;
   }
 
   /**
    * @dataProvider translateData
+   *
    * @param string $input
    *   The name of a permission which should be translated.
    * @param string $expected
    *   The name of an actual permission (based on translation matrix for "runtime").
    */
   public function testTranslate($input, $expected) {
     $perm = new CRM_Core_Permission_Base();
-    $actual = $perm->translatePermission($input, "myruntime", [
+    $actual = $perm->translatePermission($input, 'myruntime', [
       'universal name' => 'local name',
       'universal name2' => 'local name2',
       'gunk' => 'gunky',
     ]);
     $this->assertEquals($expected, $actual);
   }
 
+  /**
+   * Test that the user has the implied permission of administer CiviCRM data by virtue of having administer CiviCRM.
+   */
+  public function testImpliedPermission() {
+    $this->createLoggedInUser();
+    CRM_Core_Config::singleton()->userPermissionClass->permissions = [
+      'administer CiviCRM',
+    ];
+    $this->assertTrue(CRM_Core_Permission::check('administer CiviCRM data'));
+  }
+
 }