Skip to content

Commit

Permalink
Fix hasPermissionForActivityType to use components function.
Browse files Browse the repository at this point in the history
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
  • Loading branch information
eileenmcnaughton committed Oct 24, 2018
1 parent 12e4974 commit c4937fe
Show file tree
Hide file tree
Showing 2 changed files with 36 additions and 34 deletions.
58 changes: 24 additions & 34 deletions CRM/Activity/BAO/Activity.php
Original file line number Diff line number Diff line change
Expand Up @@ -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}";
}
Expand Down Expand Up @@ -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;
Expand All @@ -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 = "
Expand All @@ -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 {
Expand Down
12 changes: 12 additions & 0 deletions tests/phpunit/api/v3/ACLPermissionTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -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.
*
Expand Down

0 comments on commit c4937fe

Please sign in to comment.