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

diff --git a/CRM/Core/Permission.php b/CRM/Core/Permission.php
index 2c71a406defaeeb5851d23eaea4e1062330d2b20..cbed76e3c8f2aa2c85df28aaff48840d59587a44 100644 (file)
--- a/CRM/Core/Permission.php
+++ b/CRM/Core/Permission.php
@@ -119,9 +119,17 @@ class CRM_Core_Permission {
       }
       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 @@ class CRM_Core_Permission {
         $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
index 20b52451cc27c20c438e6643e304fe25000f42c8..df4d108a1d478e4ad225fa51bef9590785f16544 100644 (file)
--- 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,7 +397,7 @@
      <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>
@@ -405,7 +405,7 @@
      <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>
@@ -413,6 +413,7 @@
      <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
index 74c595c4999b06129f199cb8a0fac22b7b24830b..292e8b2b4c85eae76f0a0f12d5e23d09c4deec50 100644 (file)
--- a/CRM/Event/Form/ManageEvent.php
+++ b/CRM/Event/Form/ManageEvent.php
@@ -180,7 +180,7 @@ class CRM_Event_Form_ManageEvent extends CRM_Core_Form {
     $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
index ebfba8f0dc39c1fdaca1e36f4bb13e64cf30dcd7..260605871f1a9b396e985abfd4118ae7600067a5 100644 (file)
--- a/CRM/Event/Form/ManageEvent/TabHeader.php
+++ b/CRM/Event/Form/ManageEvent/TabHeader.php
@@ -70,7 +70,7 @@ class CRM_Event_Form_ManageEvent_TabHeader {
     $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
index bd133450b0e12884b7b89e8d10103f80dfa489df..88cea65ebddf81ccf667bf693ad33fa65f5f8766 100644 (file)
--- a/CRM/Event/Form/Registration/Register.php
+++ b/CRM/Event/Form/Registration/Register.php
@@ -563,7 +563,7 @@ class CRM_Event_Form_Registration_Register extends CRM_Event_Form_Registration {
 
       // 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
index 08dc44dfbba45c68d38820beb6b7a10b629fa524..f0d05726c551b996d2ee9e9361c5ab81cfee5d19 100644 (file)
--- a/CRM/Event/Page/ManageEvent.php
+++ b/CRM/Event/Page/ManageEvent.php
@@ -165,7 +165,7 @@ class CRM_Event_Page_ManageEvent extends CRM_Core_Page {
 
       // @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
index 3389c4abb90365de814a1c22c2b57b8478f0c3b2..df53739b3c0c65e51ab6ff9b39ebdd41fa86d26c 100644 (file)
--- 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
index 49f1adbd97c3086be84fd06794478e0b9147d2e4..b604fe8a64f2fe8fbbbbc20d483d21ec216839d4 100644 (file)
--- a/CRM/UF/Form/Inline/Preview.php
+++ b/CRM/UF/Form/Inline/Preview.php
@@ -29,7 +29,7 @@ class CRM_UF_Form_Inline_Preview extends CRM_UF_Form_AbstractPreview {
     // 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
index 1e4d9f7df707bee0cd99aa5e5944f0ea89299cd7..290de63123d491eb0782ff272e8cf99719b05276 100644 (file)
--- a/CRM/Utils/Check.php
+++ b/CRM/Utils/Check.php
@@ -64,7 +64,7 @@ class CRM_Utils_Check {
    * 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
index dca187540c7a06c588217662ae223e5b4b57609e..2095aba6861ac09b9fd3edd2ef4b8476f1dda1a1 100644 (file)
--- a/tests/phpunit/CRM/Core/Permission/BaseTest.php
+++ b/tests/phpunit/CRM/Core/Permission/BaseTest.php
@@ -6,6 +6,8 @@
  */
 class CRM_Core_Permission_BaseTest extends CiviUnitTestCase {
 
+  use CRMTraits_ACL_PermissionTrait;
+
   /**
    * @return array
    *   (0 => input to translatePermission, 1 => expected output from translatePermission)
@@ -13,13 +15,13 @@ class CRM_Core_Permission_BaseTest extends CiviUnitTestCase {
   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;
@@ -27,6 +29,7 @@ class CRM_Core_Permission_BaseTest extends CiviUnitTestCase {
 
   /**
    * @dataProvider translateData
+   *
    * @param string $input
    *   The name of a permission which should be translated.
    * @param string $expected
@@ -34,7 +37,7 @@ class CRM_Core_Permission_BaseTest extends CiviUnitTestCase {
    */
   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',
@@ -42,4 +45,15 @@ class CRM_Core_Permission_BaseTest extends CiviUnitTestCase {
     $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'));
+  }
+
 }