From 755a18357bf04fb264b6a1bfad316d8d4f8328bf Mon Sep 17 00:00:00 2001 From: eileen Date: Thu, 6 Feb 2020 17:07:11 +1300 Subject: [PATCH] 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 --- CRM/Core/Permission.php | 58 +++++++++++++++++-- CRM/Core/xml/Menu/Admin.xml | 15 ++--- CRM/Event/Form/ManageEvent.php | 2 +- CRM/Event/Form/ManageEvent/TabHeader.php | 2 +- CRM/Event/Form/Registration/Register.php | 2 +- CRM/Event/Page/ManageEvent.php | 2 +- CRM/Grant/Page/DashBoard.php | 2 +- CRM/UF/Form/Inline/Preview.php | 2 +- CRM/Utils/Check.php | 2 +- .../phpunit/CRM/Core/Permission/BaseTest.php | 30 +++++++--- 10 files changed, 91 insertions(+), 26 deletions(-) diff --git a/CRM/Core/Permission.php b/CRM/Core/Permission.php index 2c71a406de..cbed76e3c8 100644 --- 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 20b52451cc..df4d108a1d 100644 --- a/CRM/Core/xml/Menu/Admin.xml +++ b/CRM/Core/xml/Menu/Admin.xml @@ -259,7 +259,7 @@ Schedule Reminders. CRM_Admin_Page_ScheduleReminders 1 - administer CiviCRM;edit all events + administer CiviCRM data;edit all events Communications 40 @@ -397,7 +397,7 @@ Manage Extensions CRM_Admin_Page_Extensions - administer CiviCRM + administer CiviCRM system System Settings 120 @@ -405,7 +405,7 @@ civicrm/admin/extensions/upgrade Database Upgrades CRM_Admin_Page_ExtensionsUpgrade - administer CiviCRM + administer CiviCRM system civicrm/admin/setting/smtp @@ -413,6 +413,7 @@ CRM_Admin_Form_Setting_Smtp System Settings 20 + administer CiviCRM system civicrm/admin/paymentProcessor @@ -535,14 +536,14 @@ civicrm/admin/runjobs URL used for running scheduled jobs. CRM_Utils_System::executeScheduledJobs - access CiviCRM,administer CiviCRM + access CiviCRM,administer CiviCRM system civicrm/admin/job Scheduled Jobs Managing periodially running tasks. CRM_Admin_Page_Job - access CiviCRM,administer CiviCRM + access CiviCRM,administer CiviCRM system System Settings 1370 @@ -551,7 +552,7 @@ Scheduled Jobs Log Browsing the log of periodially running tasks. CRM_Admin_Page_JobLog - access CiviCRM,administer CiviCRM + access CiviCRM,administer CiviCRM system Manage 1380 @@ -573,7 +574,7 @@ civicrm/admin Administer CiviCRM - administer CiviCRM,access CiviCRM + administer CiviCRM system,administer CiviCRM data,access CiviCRM 1 CRM_Admin_Page_Admin true diff --git a/CRM/Event/Form/ManageEvent.php b/CRM/Event/Form/ManageEvent.php index 74c595c499..292e8b2b4c 100644 --- 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 ebfba8f0dc..260605871f 100644 --- 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 bd133450b0..88cea65ebd 100644 --- 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 08dc44dfbb..f0d05726c5 100644 --- 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 3389c4abb9..df53739b3c 100644 --- 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 49f1adbd97..b604fe8a64 100644 --- 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 1e4d9f7df7..290de63123 100644 --- 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 dca187540c..2095aba686 100644 --- 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')); + } + } -- 2.25.1