From: eileen <emcnaughton@wikimedia.org>
Date: Tue, 1 Jan 2019 23:14:32 +0000 (+1300)
Subject: Fix activity.getcount function to filter out unpermitted activities.
X-Git-Url: https://vcs.fsf.org/?a=commitdiff_plain;h=ff2a3553040a24da409e73cd02786c8bd11d6fd0;p=civicrm-core.git

Fix activity.getcount function to filter out unpermitted activities.

As part of an ongoing performance refactor of the activity.get api handling this extends the application of activity
type filtering to the getcount function.

The current methodology of the activity.get api is to load all activities that meet the passed in criteria and
then go through them one by one running a series of queries on each one to determine if the contact has
permission to see them and if not then filtering them from the array. At some point this did work with
getcount but it had a crippling performance impact. More recently getcount was simply giving a fatal error
when acls were in play and right before this patch the status was that no permissions
were applied when the action is getcount.

This alters that to partially apply permissions - notably a filter is added to the query that reflects
which activity types the contact may access - this is basically those with no component or a component
the contact has high level permissions to. It is a check that is applied in the 'allow' function
and it still will be after this but it's a fairly cheap check and it will always return TRUE
from here on as no activities will be fetched if their activity type is not available.

The mechanism is that the CRM_Utils_SQL_Select class calls the relevant addSelectWhereClause but ONLY when
check_permissions is TRUE. In this case the activity type filter is added if they don't have
permission to check all activities.

Tangental changes
1) test enabled for getcount with activity type checks
2) I added a type casting for activity_type_id in the function that retrieves
and caches them - it should be pretty impossible for them not to be all integers
but as I later concat them this seems a good security insurance
3) I changed a very recently added function used in this from public to protected. It was added in Dec 2018
and is only called from the Activity BAO and I want to discourage other classes from calling it
now that I believe this is the correct approach.
---

diff --git a/CRM/Activity/BAO/Activity.php b/CRM/Activity/BAO/Activity.php
index 423b86a7dd..b5aacf45fb 100644
--- a/CRM/Activity/BAO/Activity.php
+++ b/CRM/Activity/BAO/Activity.php
@@ -1117,6 +1117,22 @@ ORDER BY    fixed_sort_order
     return $values;
   }
 
+  /**
+   * @inheritDoc
+   */
+  public function addSelectWhereClause() {
+    $clauses = parent::addSelectWhereClause();
+    if (!CRM_Core_Permission::check('view all activities')) {
+      $permittedActivityTypeIDs = self::getPermittedActivityTypes();
+      if (empty($permittedActivityTypeIDs)) {
+        // This just prevents a mysql fail if they have no access - should be extremely edge case.
+        $permittedActivityTypeIDs = [0];
+      }
+      $clauses['activity_type_id'] = ('IN (' . implode(', ', $permittedActivityTypeIDs) . ')');
+    }
+    return $clauses;
+  }
+
   /**
    * Get an array of components that are accessible by the currenct user.
    *
@@ -2694,6 +2710,8 @@ AND cl.modified_id  = c.id
     }
 
     if (!self::hasPermissionForActivityType($activity->activity_type_id)) {
+      // this check is redundant for api access / anything that calls the selectWhereClause
+      // to determine ACLs.
       return FALSE;
     }
     // Return early when it is case activity.
@@ -2799,7 +2817,7 @@ AND cl.modified_id  = c.id
    *
    * @return array
    */
-  public static function getPermittedActivityTypes() {
+  protected static function getPermittedActivityTypes() {
     $userID = (int) CRM_Core_Session::getLoggedInContactID();
     if (!isset(Civi::$statics[__CLASS__]['permitted_activity_types'][$userID])) {
       $permittedActivityTypes = [];
@@ -2813,7 +2831,7 @@ AND cl.modified_id  = c.id
 INNER JOIN  civicrm_option_group grp ON (grp.id = option_group_id AND grp.name = 'activity_type')
      WHERE  component_id IS NULL $componentClause")->fetchAll();
       foreach ($types as $type) {
-        $permittedActivityTypes[$type['activity_type_id']] = $type['activity_type_id'];
+        $permittedActivityTypes[$type['activity_type_id']] = (int) $type['activity_type_id'];
       }
       Civi::$statics[__CLASS__]['permitted_activity_types'][$userID] = $permittedActivityTypes;
     }
diff --git a/tests/phpunit/api/v3/ACLPermissionTest.php b/tests/phpunit/api/v3/ACLPermissionTest.php
index 63a36a5411..1500f81a38 100644
--- a/tests/phpunit/api/v3/ACLPermissionTest.php
+++ b/tests/phpunit/api/v3/ACLPermissionTest.php
@@ -503,6 +503,8 @@ class api_v3_ACLPermissionTest extends CiviUnitTestCase {
     $this->hookClass->setHook('civicrm_aclWhereClause', array($this, 'aclWhereHookAllResults'));
     $this->setPermissions(['access CiviCRM', 'access CiviContribute']);
     $this->callAPISuccessGetSingle('Activity', ['check_permissions' => 1, 'id' => ['IN' => [$activity['id'], $activity2['id']]]]);
+    $this->callAPISuccessGetCount('Activity', ['check_permissions' => 1, 'id' => ['IN' => [$activity['id'], $activity2['id']]]], 1);
+
   }
 
   /**
@@ -515,6 +517,7 @@ class api_v3_ACLPermissionTest extends CiviUnitTestCase {
     $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']]]]);
+    $this->callAPISuccessGetCount('Activity', ['check_permissions' => 1, 'id' => ['IN' => [$activity['id'], $activity2['id']]]], 1);
   }
 
   /**