From c4937fe9dd5f2f44978b7116bd27b6cea871a9ff Mon Sep 17 00:00:00 2001 From: eileen Date: Wed, 24 Oct 2018 13:56:14 +1300 Subject: [PATCH] Fix hasPermissionForActivityType to use components function. This is a refactor step - not the final destination. I want to move from loading all activities & then checking if each one can be accessed to determining a list of activity types and then adding that filter to the sql query. 2 reasons - performance - because getcount actually hard fails when permissions are applied currently :-( In this step I note the function activityComponents looks like it expected the concept of showActivitiesInCore by components to take off but in practice it is hard coded to 0 for CiviCase & CiviCase only. Oddly there is acually handling for CiviCase in this function in a place unreachable without the changes in this patch. In addition the few places that call this function also do weird CiviCase handling. I've opted for relatively low intervention since I think most places that call this function are likely to change themselves in the short-medium term. note this implements https://lab.civicrm.org/dev/core/issues/454 which slightly downgrades the relevant permission --- CRM/Activity/BAO/Activity.php | 58 +++++++++------------- tests/phpunit/api/v3/ACLPermissionTest.php | 12 +++++ 2 files changed, 36 insertions(+), 34 deletions(-) diff --git a/CRM/Activity/BAO/Activity.php b/CRM/Activity/BAO/Activity.php index af76376b7d6a..342e2fe6b530 100644 --- a/CRM/Activity/BAO/Activity.php +++ b/CRM/Activity/BAO/Activity.php @@ -1147,21 +1147,33 @@ public static function deprecatedGetActivities($input) { } /** - * Get the component id and name if those are enabled and allowed. + * Get an array of components that are accessible by the currenct user. * - * Checks whether logged in user has permission. - * To decide whether we are going to include - * component related activities with core activity retrieve process. - * (what did that just mean?) + * This means checking if they are enabled and if the user has appropriate permission. * - * @return array + * For most components the permission is access component (e.g 'access CiviContribute'). + * Exceptions as CiviCampaign (administer CiviCampaign) and CiviCase + * (accesses a case function which enforces edit all cases or edit my cases. Case + * permissions are also handled on a per activity basis). + * + * Checks whether logged in user has permission to the component. + * + * @param bool $excludeComponentHandledActivities + * Should we exclude components whose display is handled in the components. + * In practice this means should we include CiviCase in the results. Presumbaly + * at the time it was decided case activities should be shown in the case framework and + * that this concept might be extended later. In practice most places that + * call this then re-add CiviCase in some way so it's all a bit... odd. + * + * @return array Array of component id and name. * Array of component id and name. */ - public static function activityComponents() { + public static function activityComponents($excludeComponentHandledActivities = TRUE) { $components = array(); $compInfo = CRM_Core_Component::getEnabledComponents(); foreach ($compInfo as $compObj) { - if (!empty($compObj->info['showActivitiesInCore'])) { + $includeComponent = !$excludeComponentHandledActivities || !empty($compObj->info['showActivitiesInCore']); + if ($includeComponent) { if ($compObj->info['name'] == 'CiviCampaign') { $componentPermission = "administer {$compObj->name}"; } @@ -2784,7 +2796,7 @@ public static function checkPermission($activityId, $action) { */ protected static function isContactPermittedAccessToCaseActivity($activityId, $action, $activityTypeID) { $allow = FALSE; - foreach (['administer CiviCase', 'access my cases and activities', 'access all cases and activities'] as $per) { + foreach (['access my cases and activities', 'access all cases and activities'] as $per) { if (CRM_Core_Permission::check($per)) { $allow = TRUE; break; @@ -2811,21 +2823,7 @@ protected static function isContactPermittedAccessToCaseActivity($activityId, $a * @return bool */ protected static function hasPermissionForActivityType($activityTypeID) { - $compPermissions = [ - 'CiviCase' => [ - 'administer CiviCase', - 'access my cases and activities', - 'access all cases and activities', - ], - 'CiviMail' => ['access CiviMail'], - 'CiviEvent' => ['access CiviEvent'], - 'CiviGrant' => ['access CiviGrant'], - 'CiviPledge' => ['access CiviPledge'], - 'CiviMember' => ['access CiviMember'], - 'CiviReport' => ['access CiviReport'], - 'CiviContribute' => ['access CiviContribute'], - 'CiviCampaign' => ['administer CiviCampaign'], - ]; + $components = self::activityComponents(FALSE); // First check the component permission. $sql = " @@ -2840,16 +2838,8 @@ protected static function hasPermissionForActivityType($activityTypeID) { $componentId = CRM_Core_DAO::singleValueQuery($sql, $params); if ($componentId) { - $componentName = CRM_Core_Component::getComponentName($componentId); - $compPermission = CRM_Utils_Array::value($componentName, $compPermissions); - - // Here we are interesting in any single permission. - if (is_array($compPermission)) { - foreach ($compPermission as $per) { - if (CRM_Core_Permission::check($per)) { - return TRUE; - } - } + if (!empty($components[$componentId])) { + return TRUE; } } else { diff --git a/tests/phpunit/api/v3/ACLPermissionTest.php b/tests/phpunit/api/v3/ACLPermissionTest.php index edadb0fbb403..45bd7bceed54 100644 --- a/tests/phpunit/api/v3/ACLPermissionTest.php +++ b/tests/phpunit/api/v3/ACLPermissionTest.php @@ -545,6 +545,18 @@ public function testGetActivityCheckPermissionsByComponent() { $this->callAPISuccessGetSingle('Activity', ['check_permissions' => 1, 'id' => ['IN' => [$activity['id'], $activity2['id']]]]); } + /** + * Check that component related activity filtering works for CiviCase. + */ + public function testGetActivityCheckPermissionsByCaseComponent() { + CRM_Core_BAO_ConfigSetting::enableComponent('CiviCase'); + $activity = $this->activityCreate(['activity_type_id' => 'Open Case']); + $activity2 = $this->activityCreate(['activity_type_id' => 'Pledge Reminder']); + $this->hookClass->setHook('civicrm_aclWhereClause', array($this, 'aclWhereHookAllResults')); + $this->setPermissions(['access CiviCRM', 'access CiviContribute', 'access all cases and activities']); + $this->callAPISuccessGetSingle('Activity', ['check_permissions' => 1, 'id' => ['IN' => [$activity['id'], $activity2['id']]]]); + } + /** * Check that activities can be retrieved by ACL. *