From 18be3201ed7896db0d78a04a57c7f85913cfb695 Mon Sep 17 00:00:00 2001 From: Coleman Watts Date: Thu, 26 Jul 2018 14:36:07 -0400 Subject: [PATCH 1/3] Support $contactId param in permission checks --- CRM/Core/Permission.php | 17 ++++++++++++----- CRM/Core/Permission/Backdrop.php | 10 +++++++--- CRM/Core/Permission/Base.php | 3 ++- CRM/Core/Permission/Drupal.php | 10 +++++++--- CRM/Core/Permission/Drupal6.php | 10 +++++++--- CRM/Core/Permission/Drupal8.php | 7 ++++--- CRM/Core/Permission/Joomla.php | 9 +++++++-- CRM/Core/Permission/Soap.php | 3 ++- CRM/Core/Permission/UnitTests.php | 3 ++- CRM/Core/Permission/WordPress.php | 11 +++++++---- 10 files changed, 57 insertions(+), 26 deletions(-) diff --git a/CRM/Core/Permission.php b/CRM/Core/Permission.php index a532a4a2c4d2..bd73266fca7a 100644 --- a/CRM/Core/Permission.php +++ b/CRM/Core/Permission.php @@ -83,9 +83,11 @@ public static function getPermission() { /** * Given a permission string or array, check for access requirements - * @param mixed $permissions + * @param string|array $permissions * The permission to check as an array or string -see examples. - * arrays + * + * @param int $contactId + * Contact id to check permissions for. Defaults to current logged-in user. * * Ex 1 * @@ -117,15 +119,20 @@ public static function getPermission() { * @return bool * true if yes, else false */ - public static function check($permissions) { + public static function check($permissions, $contactId = NULL) { $permissions = (array) $permissions; + $userId = NULL; + if ($contactId) { + $userId = CRM_Core_BAO_UFMatch::getUFId($contactId); + } + /** @var CRM_Core_Permission_Temp $tempPerm */ $tempPerm = CRM_Core_Config::singleton()->userPermissionTemp; foreach ($permissions as $permission) { if (is_array($permission)) { foreach ($permission as $orPerm) { - if (self::check($orPerm)) { + if (self::check($orPerm, $contactId)) { //one of our 'or' permissions has succeeded - stop checking this permission return TRUE; } @@ -135,7 +142,7 @@ public static function check($permissions) { } else { // This is an individual permission - $granted = CRM_Core_Config::singleton()->userPermissionClass->check($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); if ( diff --git a/CRM/Core/Permission/Backdrop.php b/CRM/Core/Permission/Backdrop.php index f6a0d6f330ab..3d40db218ec2 100644 --- a/CRM/Core/Permission/Backdrop.php +++ b/CRM/Core/Permission/Backdrop.php @@ -68,12 +68,12 @@ class CRM_Core_Permission_Backdrop extends CRM_Core_Permission_DrupalBase { * @param string $str * The permission to check. * - * @param int $contactID + * @param int $userId * * @return bool * true if yes, else false */ - public function check($str, $contactID = NULL) { + public function check($str, $userId = NULL) { $str = $this->translatePermission($str, 'Drupal', array( 'view user account' => 'access user profiles', 'administer users' => 'administer users', @@ -85,7 +85,11 @@ public function check($str, $contactID = NULL) { return TRUE; } if (function_exists('user_access')) { - return user_access($str) ? TRUE : FALSE; + $account = NULL; + if ($userId) { + $account = user_load($userId); + } + return user_access($str, $account); } return TRUE; } diff --git a/CRM/Core/Permission/Base.php b/CRM/Core/Permission/Base.php index c1a48cc49ce4..be286cafbdb9 100644 --- a/CRM/Core/Permission/Base.php +++ b/CRM/Core/Permission/Base.php @@ -157,9 +157,10 @@ public function groupClause($type, &$tables, &$whereTables) { * * @param string $str * The permission to check. + * @param int $userId * */ - public function check($str) { + public function check($str, $userId = NULL) { //no default behaviour } diff --git a/CRM/Core/Permission/Drupal.php b/CRM/Core/Permission/Drupal.php index 45ebe56fe149..03b8ed6eda22 100644 --- a/CRM/Core/Permission/Drupal.php +++ b/CRM/Core/Permission/Drupal.php @@ -68,12 +68,12 @@ class CRM_Core_Permission_Drupal extends CRM_Core_Permission_DrupalBase { * @param string $str * The permission to check. * - * @param int $contactID + * @param int $userId * * @return bool * true if yes, else false */ - public function check($str, $contactID = NULL) { + public function check($str, $userId = NULL) { $str = $this->translatePermission($str, 'Drupal', array( 'view user account' => 'access user profiles', 'administer users' => 'administer users', @@ -85,7 +85,11 @@ public function check($str, $contactID = NULL) { return TRUE; } if (function_exists('user_access')) { - return user_access($str) ? TRUE : FALSE; + $account = NULL; + if ($userId) { + $account = user_load($userId); + } + return user_access($str, $account); } return TRUE; } diff --git a/CRM/Core/Permission/Drupal6.php b/CRM/Core/Permission/Drupal6.php index 93ddf08a4c53..9aaa4ffc0217 100644 --- a/CRM/Core/Permission/Drupal6.php +++ b/CRM/Core/Permission/Drupal6.php @@ -67,12 +67,12 @@ class CRM_Core_Permission_Drupal6 extends CRM_Core_Permission_DrupalBase { * @param string $str * The permission to check. * - * @param int $contactID + * @param int $userId * * @return bool * true if yes, else false */ - public function check($str, $contactID = NULL) { + public function check($str, $userId = NULL) { $str = $this->translatePermission($str, 'Drupal6', array( 'view user account' => 'access user profiles', 'administer users' => 'administer users', @@ -84,7 +84,11 @@ public function check($str, $contactID = NULL) { return TRUE; } if (function_exists('user_access')) { - return user_access($str) ? TRUE : FALSE; + $account = NULL; + if ($userId) { + $account = user_load($userId); + } + return user_access($str, $account); } return TRUE; } diff --git a/CRM/Core/Permission/Drupal8.php b/CRM/Core/Permission/Drupal8.php index 7c65629ef26d..3969874fae18 100644 --- a/CRM/Core/Permission/Drupal8.php +++ b/CRM/Core/Permission/Drupal8.php @@ -43,11 +43,11 @@ class CRM_Core_Permission_Drupal8 extends CRM_Core_Permission_DrupalBase { * @param string $str * The permission to check. * - * @param null $contactID + * @param int $userId * * @return bool */ - public function check($str, $contactID = NULL) { + public function check($str, $userId = NULL) { $str = $this->translatePermission($str, 'Drupal', array( 'view user account' => 'access user profiles', )); @@ -58,7 +58,8 @@ public function check($str, $contactID = NULL) { if ($str == CRM_Core_Permission::ALWAYS_ALLOW_PERMISSION) { return TRUE; } - return \Drupal::currentUser()->hasPermission($str); + $acct = $userId ? \Drupal\user\Entity\User::load($userId) : \Drupal::currentUser(); + return $acct->hasPermission($str); } /** diff --git a/CRM/Core/Permission/Joomla.php b/CRM/Core/Permission/Joomla.php index 50f583fb61c3..a0e256b718fb 100644 --- a/CRM/Core/Permission/Joomla.php +++ b/CRM/Core/Permission/Joomla.php @@ -42,12 +42,17 @@ class CRM_Core_Permission_Joomla extends CRM_Core_Permission_Base { * * @param string $str * The permission to check. + * @param int $userId * * @return bool * true if yes, else false */ - public function check($str) { + public function check($str, $userId = NULL) { $config = CRM_Core_Config::singleton(); + // JFactory::getUser does strict type checking, so convert falesy values to NULL + if (!$userId) { + $userId = NULL; + } $translated = $this->translateJoomlaPermission($str); if ($translated === CRM_Core_Permission::ALWAYS_DENY_PERMISSION) { @@ -61,7 +66,7 @@ public function check($str) { // we've not yet figured out how to bootstrap joomla, so we should // not execute hooks if joomla is not loaded if (defined('_JEXEC')) { - $user = JFactory::getUser(); + $user = JFactory::getUser($userId); $api_key = CRM_Utils_Request::retrieve('api_key', 'String', $store, FALSE, NULL, 'REQUEST'); // If we are coming from REST we don't have a user but we do have the api_key for a user. diff --git a/CRM/Core/Permission/Soap.php b/CRM/Core/Permission/Soap.php index 57938812b6bf..9ea9ef60e8d4 100644 --- a/CRM/Core/Permission/Soap.php +++ b/CRM/Core/Permission/Soap.php @@ -43,11 +43,12 @@ class CRM_Core_Permission_Soap extends CRM_Core_Permission_Base { * * @param string $str * The permission to check. + * @param int $userId * * @return bool * true if yes, else false */ - public function check($str) { + public function check($str, $userId = NULL) { return TRUE; } diff --git a/CRM/Core/Permission/UnitTests.php b/CRM/Core/Permission/UnitTests.php index 8dc1a057934c..22b18f76f0fe 100644 --- a/CRM/Core/Permission/UnitTests.php +++ b/CRM/Core/Permission/UnitTests.php @@ -46,11 +46,12 @@ class CRM_Core_Permission_UnitTests extends CRM_Core_Permission_Base { * * @param string $str * The permission to check. + * @param int $userId * * @return bool * true if yes, else false */ - public function check($str) { + public function check($str, $userId = NULL) { if ($str == CRM_Core_Permission::ALWAYS_DENY_PERMISSION) { return FALSE; } diff --git a/CRM/Core/Permission/WordPress.php b/CRM/Core/Permission/WordPress.php index bc032d05a276..12ec6438615a 100644 --- a/CRM/Core/Permission/WordPress.php +++ b/CRM/Core/Permission/WordPress.php @@ -42,11 +42,12 @@ class CRM_Core_Permission_WordPress extends CRM_Core_Permission_Base { * * @param string $str * The permission to check. + * @param int $userId * * @return bool * true if yes, else false */ - public function check($str) { + public function check($str, $userId = NULL) { // Generic cms 'administer users' role tranlates to users with the 'edit_users' capability' in WordPress $str = $this->translatePermission($str, 'WordPress', array( 'administer users' => 'edit_users', @@ -74,16 +75,18 @@ public function check($str) { return TRUE; } - if (current_user_can('super admin') || current_user_can('administrator')) { + $user = $userId ? get_userdata($userId) : wp_get_current_user(); + + if ($user->has_cap('super admin') || $user->has_cap('administrator')) { return TRUE; } // Make string lowercase and convert spaces into underscore $str = CRM_Utils_String::munge(strtolower($str)); - if (is_user_logged_in()) { + if ($user->exists()) { // Check whether the logged in user has the capabilitity - if (current_user_can($str)) { + if ($user->has_cap($str)) { return TRUE; } } From a7d9f31a12f8783ed037e55c2cd975e2861caaa0 Mon Sep 17 00:00:00 2001 From: Coleman Watts Date: Thu, 26 Jul 2018 15:20:48 -0400 Subject: [PATCH 2/3] Fix CRM_ACL_API::whereClause to respect $contactId param --- CRM/ACL/API.php | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/CRM/ACL/API.php b/CRM/ACL/API.php index b3eacfc3f0f0..b317fde85f27 100644 --- a/CRM/ACL/API.php +++ b/CRM/ACL/API.php @@ -113,18 +113,18 @@ public static function whereClause( } } - // first see if the contact has edit / view all contacts - if (CRM_Core_Permission::check('edit all contacts') || - ($type == self::VIEW && CRM_Core_Permission::check('view all contacts')) - ) { - return $deleteClause; - } - if (!$contactID) { $contactID = CRM_Core_Session::getLoggedInContactID(); } $contactID = (int) $contactID; + // first see if the contact has edit / view all permission + if (CRM_Core_Permission::check('edit all contacts', $contactID) || + ($type == self::VIEW && CRM_Core_Permission::check('view all contacts', $contactID)) + ) { + return $deleteClause; + } + $where = implode(' AND ', array( CRM_ACL_BAO_ACL::whereClause($type, From fa4dac9c1d834f7e49e835bfe96d9391b3315c42 Mon Sep 17 00:00:00 2001 From: Coleman Watts Date: Thu, 26 Jul 2018 17:01:34 -0400 Subject: [PATCH 3/3] Pass contactId to hook_civicrm_permission_check --- CRM/Core/Permission.php | 2 +- CRM/Utils/Hook.php | 8 +++++--- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/CRM/Core/Permission.php b/CRM/Core/Permission.php index bd73266fca7a..49d003dd62c4 100644 --- a/CRM/Core/Permission.php +++ b/CRM/Core/Permission.php @@ -144,7 +144,7 @@ public static function check($permissions, $contactId = NULL) { // 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); + CRM_Utils_Hook::permission_check($permission, $granted, $contactId); if ( !$granted && !($tempPerm && $tempPerm->check($permission)) diff --git a/CRM/Utils/Hook.php b/CRM/Utils/Hook.php index 81b322cb5e9a..92989a902534 100644 --- a/CRM/Utils/Hook.php +++ b/CRM/Utils/Hook.php @@ -1820,13 +1820,15 @@ public static function permission(&$permissions) { * The name of an atomic permission, ie. 'access deleted contacts' * @param bool $granted * Whether this permission is currently granted. The hook can change this value. + * @param int $contactId + * Contact whose permissions we are checking (if null, assume current user). * * @return null * The return value is ignored */ - public static function permission_check($permission, &$granted) { - return self::singleton()->invoke(array('permission', 'granted'), $permission, $granted, - self::$_nullObject, self::$_nullObject, self::$_nullObject, self::$_nullObject, + public static function permission_check($permission, &$granted, $contactId) { + return self::singleton()->invoke(array('permission', 'granted', 'contactId'), $permission, $granted, $contactId, + self::$_nullObject, self::$_nullObject, self::$_nullObject, 'civicrm_permission_check' ); }