From ea54c6e527ebb2c44b2dea2817df1e6a79dd10d9 Mon Sep 17 00:00:00 2001 From: eileen Date: Sat, 13 Mar 2021 15:31:48 +1300 Subject: [PATCH] Add new Super-duper-no-permissions-apply permission We discussed in the context of search kit that there are 2 competing concepts of 'administer CiviCRM' 1) like drupal user 1, can do anything 2) role that has various administrative access but acls etc still apply In search kit we have an interest in allowing users who won't do dumb things the ability to expose data to people who otherwise would not have access to that data - e.g to create a listing of event participants to expose to anonymous users. This effectively means we are giving people the power to create displays that set check_permissions to FALSE. This would potentially enable people to not just bypass ACLs applied to others but also acls applied to them. In order words it could be a privellege escallation. To prevent any unexpected escallation we decided that this ability should only be given to contacts who explicitly have access to everything anyway. There is no existing permission that does this (although there is a perception that there is) --- CRM/Core/Permission.php | 37 +++++++++++-------- CRM/Core/Permission/Base.php | 17 ++++++--- CRM/Core/Permission/Drupal6.php | 2 +- CRM/Utils/Hook.php | 13 ++++--- .../phpunit/CRM/Core/Permission/BaseTest.php | 17 +++++++-- 5 files changed, 55 insertions(+), 31 deletions(-) diff --git a/CRM/Core/Permission.php b/CRM/Core/Permission.php index 7191a8e60c..a403c9b81f 100644 --- a/CRM/Core/Permission.php +++ b/CRM/Core/Permission.php @@ -591,7 +591,7 @@ class CRM_Core_Permission { $permissions = self::getCoreAndComponentPermissions($all); // Add any permissions defined in hook_civicrm_permission implementations. - $module_permissions = CRM_Core_Config::singleton()->userPermissionClass->getAllModulePermissions(TRUE); + $module_permissions = CRM_Core_Config::singleton()->userPermissionClass->getAllModulePermissions(TRUE, $permissions); $permissions = array_merge($permissions, $module_permissions); if (!$descriptions) { foreach ($permissions as $name => $attr) { @@ -874,6 +874,10 @@ class CRM_Core_Permission { 'label' => $prefix . ts('administer CiviCRM Data'), 'description' => ts('Permit altering all restricted data options'), ], + 'all CiviCRM permissions and ACLs' => [ + 'label' => $prefix . ts('all CiviCRM permissions and ACLs'), + 'description' => ts('Administer and use CiviCRM bypassing any other permission or ACL checks and enabling the creation of displays and forms that allow others to bypass checks. This permission should be given out with care'), + ], ]; if (self::isMultisiteEnabled()) { // This could arguably be moved to the multisite extension but @@ -883,11 +887,6 @@ class CRM_Core_Permission { 'description' => ts('Administer multiple organizations. In practice this allows editing the group organization link'), ]; } - foreach (self::getImpliedPermissions() as $name => $includes) { - foreach ($includes as $permission) { - $permissions[$name][] = $permissions[$permission]; - } - } return $permissions; } @@ -896,11 +895,11 @@ class CRM_Core_Permission { * * @return array */ - public static function getImpliedPermissions() { + public static function getImpliedAdminPermissions(): array { return [ - 'administer CiviCRM' => ['administer CiviCRM system', 'administer CiviCRM data'], - 'administer CiviCRM data' => ['edit message templates', 'administer dedupe rules'], - 'administer CiviCRM system' => ['edit system workflow message templates'], + 'administer CiviCRM' => ['implied_permissions' => ['administer CiviCRM system', 'administer CiviCRM data']], + 'administer CiviCRM data' => ['implied_permissions' => ['edit message templates', 'administer dedupe rules']], + 'administer CiviCRM system' => ['implied_permissions' => ['edit system workflow message templates']], ]; } @@ -911,14 +910,19 @@ class CRM_Core_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; + public static function getImpliedPermissionsFor(string $permission): array { + $implied = Civi::cache('metadata')->get('implied_permissions', []); + if (isset($implied[$permission])) { + return $implied[$permission]; + } + $implied[$permission] = []; + foreach (self::basicPermissions(FALSE, TRUE) as $key => $details) { + if (in_array($permission, $details['implied_permissions'] ?? [], TRUE)) { + $implied[$permission][] = $key; } } - return $return; + Civi::cache('metadata')->set('implied_permissions', $implied); + return $implied[$permission]; } /** @@ -1728,6 +1732,7 @@ class CRM_Core_Permission { protected static function getCoreAndComponentPermissions(bool $all): array { $permissions = self::getCorePermissions(); $permissions = array_merge($permissions, self::getComponentPermissions($all)); + $permissions['all CiviCRM permissions and ACLs']['implied_permissions'] = array_keys($permissions); return $permissions; } diff --git a/CRM/Core/Permission/Base.php b/CRM/Core/Permission/Base.php index 0c7bf678a3..60ffbe3c9e 100644 --- a/CRM/Core/Permission/Base.php +++ b/CRM/Core/Permission/Base.php @@ -57,7 +57,7 @@ class CRM_Core_Permission_Base { * a permission name */ public function translatePermission($perm, $nativePrefix, $map) { - list ($civiPrefix, $name) = CRM_Utils_String::parsePrefix(':', $perm, NULL); + [$civiPrefix, $name] = CRM_Utils_String::parsePrefix(':', $perm, NULL); switch ($civiPrefix) { case $nativePrefix: return $name; @@ -272,9 +272,12 @@ class CRM_Core_Permission_Base { * The permission to check. * @param int $userId * + * @return bool; + * */ public function check($str, $userId = NULL) { //no default behaviour + return FALSE; } /** @@ -374,7 +377,7 @@ class CRM_Core_Permission_Base { * Array of permissions, in the same format as CRM_Core_Permission::getCorePermissions(). * @see CRM_Core_Permission::getCorePermissions */ - public static function getModulePermissions($module) { + public function getModulePermissions($module): array { $return_permissions = []; $fn_name = "{$module}_civicrm_permission"; if (function_exists($fn_name)) { @@ -390,14 +393,15 @@ class CRM_Core_Permission_Base { * in all enabled CiviCRM module extensions. * * @param bool $descriptions + * @param array $permissions * * @return array * Array of permissions, in the same format as CRM_Core_Permission::getCorePermissions(). */ - public function getAllModulePermissions($descriptions = FALSE) { - // Passing in false here is to be deprecated. - $permissions = []; - CRM_Utils_Hook::permission($permissions); + public function getAllModulePermissions($descriptions = FALSE, &$permissions): array { + $newPermissions = []; + CRM_Utils_Hook::permission($newPermissions, $permissions); + $permissions = array_merge($permissions, $newPermissions); if ($descriptions) { foreach ($permissions as $permission => $label) { @@ -405,6 +409,7 @@ class CRM_Core_Permission_Base { } } else { + // Passing in false here is to be deprecated. foreach ($permissions as $permission => $label) { $permissions[$permission] = (is_array($label)) ? array_shift($label) : $label; } diff --git a/CRM/Core/Permission/Drupal6.php b/CRM/Core/Permission/Drupal6.php index 2ff671fb09..d478f09b5d 100644 --- a/CRM/Core/Permission/Drupal6.php +++ b/CRM/Core/Permission/Drupal6.php @@ -192,7 +192,7 @@ class CRM_Core_Permission_Drupal6 extends CRM_Core_Permission_DrupalBase { * @return array * Array of permissions, in the same format as CRM_Core_Permission::getCorePermissions(). */ - public static function getModulePermissions($module) { + public function getModulePermissions($module):array { $return_permissions = []; $fn_name = "{$module}_civicrm_permission"; if (function_exists($fn_name)) { diff --git a/CRM/Utils/Hook.php b/CRM/Utils/Hook.php index 2c0d195862..dc54996f14 100644 --- a/CRM/Utils/Hook.php +++ b/CRM/Utils/Hook.php @@ -2023,16 +2023,19 @@ abstract class CRM_Utils_Hook { * This hook is called when exporting Civi's permission to the CMS. Use this hook to modify * the array of system permissions for CiviCRM. * + * @param array $newPermissions + * Array to be filled with permissions. * @param array $permissions - * Array of permissions. See CRM_Core_Permission::getCorePermissions() for - * the format of this array. + * Already calculated permissions. These can be altered. Notably an + * extension might want to add it's permissions to 'implied' or to + * remove some permissions. * * @return null * The return value is ignored */ - public static function permission(&$permissions) { - return self::singleton()->invoke(['permissions'], $permissions, - self::$_nullObject, self::$_nullObject, self::$_nullObject, self::$_nullObject, self::$_nullObject, + public static function permission(&$newPermissions, &$permissions) { + return self::singleton()->invoke(['permissions', 'all_permissions'], $newPermissions, $permissions, + self::$_nullObject, self::$_nullObject, self::$_nullObject, self::$_nullObject, 'civicrm_permission' ); } diff --git a/tests/phpunit/CRM/Core/Permission/BaseTest.php b/tests/phpunit/CRM/Core/Permission/BaseTest.php index 2095aba686..08d930defa 100644 --- a/tests/phpunit/CRM/Core/Permission/BaseTest.php +++ b/tests/phpunit/CRM/Core/Permission/BaseTest.php @@ -12,7 +12,7 @@ class CRM_Core_Permission_BaseTest extends CiviUnitTestCase { * @return array * (0 => input to translatePermission, 1 => expected output from translatePermission) */ - public function translateData() { + public function translateData(): array { $cases = []; $cases[] = ['administer CiviCRM', 'administer CiviCRM']; @@ -35,7 +35,7 @@ class CRM_Core_Permission_BaseTest extends CiviUnitTestCase { * @param string $expected * The name of an actual permission (based on translation matrix for "runtime"). */ - public function testTranslate($input, $expected) { + public function testTranslate(string $input, string $expected): void { $perm = new CRM_Core_Permission_Base(); $actual = $perm->translatePermission($input, 'myruntime', [ 'universal name' => 'local name', @@ -48,7 +48,7 @@ class CRM_Core_Permission_BaseTest extends CiviUnitTestCase { /** * Test that the user has the implied permission of administer CiviCRM data by virtue of having administer CiviCRM. */ - public function testImpliedPermission() { + public function testImpliedPermission(): void { $this->createLoggedInUser(); CRM_Core_Config::singleton()->userPermissionClass->permissions = [ 'administer CiviCRM', @@ -56,4 +56,15 @@ class CRM_Core_Permission_BaseTest extends CiviUnitTestCase { $this->assertTrue(CRM_Core_Permission::check('administer CiviCRM data')); } + /** + * Test that the super permission gives the implied permission of the whole shebang. + */ + public function testImpliedPermissionSuperDuper(): void { + $this->createLoggedInUser(); + CRM_Core_Config::singleton()->userPermissionClass->permissions = [ + 'all CiviCRM permissions and ACLs', + ]; + $this->assertTrue(CRM_Core_Permission::check('view all contacts')); + } + } -- 2.25.1