From: eileen <emcnaughton@wikimedia.org>
Date: Tue, 19 Apr 2016 02:46:33 +0000 (+1200)
Subject: CRM-18409 fix inability to view activity if source contact id is deleted.
X-Git-Url: https://vcs.fsf.org/?a=commitdiff_plain;h=bbd2743ba1f1097fbd47d94a377b61befd1ac43f;p=civicrm-core.git

CRM-18409 fix inability to view activity if source contact id is deleted.

This fix adds the ability to access activities through the api with the same permission checks as in the BAO if 'id' is passed in (and contact_id is not for 'supporting legacy stuff reasons').

Note that the api functionality is a good thing, but it is actually being added primarily for the purposes of being able to add a test fix for the change
---

diff --git a/CRM/Activity/BAO/Activity.php b/CRM/Activity/BAO/Activity.php
index 154729bbee..6c49ccbb23 100644
--- a/CRM/Activity/BAO/Activity.php
+++ b/CRM/Activity/BAO/Activity.php
@@ -2410,9 +2410,7 @@ INNER JOIN  civicrm_option_group grp ON ( grp.id = val.option_group_id AND grp.n
     if (!$componentId || $allow) {
       $sourceContactId = self::getActivityContact($activity->id, $sourceID);
       // Account for possibility of activity not having a source contact (as it may have been deleted).
-      if ($sourceContactId) {
-        $allow = CRM_Contact_BAO_Contact_Permission::allow($sourceContactId, $permission);
-      }
+      $allow = $sourceContactId ? CRM_Contact_BAO_Contact_Permission::allow($sourceContactId, $permission) : TRUE;
     }
 
     // Check for target and assignee contacts.
diff --git a/CRM/Core/DAO/permissions.php b/CRM/Core/DAO/permissions.php
index 03e6c2f25f..04ad1cfbbb 100644
--- a/CRM/Core/DAO/permissions.php
+++ b/CRM/Core/DAO/permissions.php
@@ -173,6 +173,12 @@ function _civicrm_api3_permissions($entity, $action, &$params) {
       'access CiviCRM',
       'delete activities',
     ),
+    'get' => array(
+      'access CiviCRM',
+      // Note that view all activities is also required within the api
+      // if the id is not passed in. Where the id is passed in the activity
+      // specific check functions are used and tested.
+    ),
     'default' => array(
       'access CiviCRM',
       'view all activities',
diff --git a/api/v3/Activity.php b/api/v3/Activity.php
index 494772a98b..f2e0304e05 100644
--- a/api/v3/Activity.php
+++ b/api/v3/Activity.php
@@ -229,10 +229,33 @@ function _civicrm_api3_activity_create_spec(&$params) {
  * @param array $params
  *   Array per getfields documentation.
  *
- * @return array
+ * @return array API result array
  *   API result array
+ *
+ * @throws \API_Exception
+ * @throws \CiviCRM_API3_Exception
+ * @throws \Civi\API\Exception\UnauthorizedException
  */
 function civicrm_api3_activity_get($params) {
+  if (!empty($params['check_permissions']) && !CRM_Core_Permission::check('view all activities')) {
+    // In absence of view all activities permission it's possible to see a specific activity by ACL.
+    // Note still allowing view all activities to override ACLs is based on the 'don't change too much
+    // if you are not sure principle' and it could be argued that the ACLs should always be applied.
+    if (empty($params['id']) || !empty($params['contact_id'])) {
+      // We fall back to the original blunt permissions if we don't have an id to check or we are about
+      // to go to the weird place that the legacy 'contact_id' parameter takes us to.
+      throw new \Civi\API\Exception\UnauthorizedException(
+        "Cannot access activities. Required permission: 'view all activities''"
+      );
+    }
+
+    if (!CRM_Activity_BAO_Activity::checkPermission($params['id'], CRM_Core_Action::VIEW)) {
+      throw new \Civi\API\Exception\UnauthorizedException(
+        'You do not have permission to view this activity'
+      );
+    }
+  }
+
   if (!empty($params['contact_id'])) {
     $activities = CRM_Activity_BAO_Activity::getContactActivity($params['contact_id']);
     // BAO function doesn't actually return a contact ID - hack api for now & add to test so when api re-write
diff --git a/tests/phpunit/api/v3/ACLPermissionTest.php b/tests/phpunit/api/v3/ACLPermissionTest.php
index 5f79cf65ce..d38085ca80 100644
--- a/tests/phpunit/api/v3/ACLPermissionTest.php
+++ b/tests/phpunit/api/v3/ACLPermissionTest.php
@@ -64,6 +64,8 @@ class api_v3_ACLPermissionTest extends CiviUnitTestCase {
       'civicrm_contribution',
       'civicrm_participant',
       'civicrm_uf_match',
+      'civicrm_activity',
+      'civicrm_activity_contact',
     );
     $this->quickCleanup($tablesToTruncate);
     $config = CRM_Core_Config::singleton();
@@ -348,7 +350,6 @@ class api_v3_ACLPermissionTest extends CiviUnitTestCase {
    * @throws \PHPUnit_Framework_IncompleteTestError
    */
   public function testEntitiesGetCoreACLLimitingCheck($entity) {
-    $this->markTestIncomplete('this does not work in 4.4 but can be enabled in 4.5 or a security release of 4.4 including the important security fix CRM-14877');
     $this->setupCoreACL();
     $this->setUpEntities($entity);
     $result = $this->callAPISuccess($entity, 'get', array(
@@ -452,4 +453,132 @@ class api_v3_ACLPermissionTest extends CiviUnitTestCase {
     $where = " contact_a.id = " . $this->allowedContactId;
   }
 
+  /**
+   * Basic check that an unpermissioned call keeps working and permissioned call fails.
+   */
+  public function testGetActivityNoPermissions() {
+    $this->setPermissions(array());
+    $this->callAPISuccess('Activity', 'get', array());
+    $this->callAPIFailure('Activity', 'get', array('check_permissions' => 1));
+  }
+
+  /**
+   * View all activities is enough regardless of contact ACLs.
+   */
+  public function testGetActivityViewAllActivitiesEnoughWithOrWithoutID() {
+    $activity = $this->activityCreate();
+    $this->setPermissions(array('view all activities', 'access CiviCRM'));
+    $this->callAPISuccess('Activity', 'getsingle', array('check_permissions' => 1, 'id' => $activity['id']));
+    $this->callAPISuccess('Activity', 'getsingle', array('check_permissions' => 1));
+  }
+
+  /**
+   * View all activities is required unless id is passed in.
+   */
+  public function testGetActivityViewAllContactsNotEnoughWIthoutID() {
+    $this->setPermissions(array('view all contacts', 'access CiviCRM'));
+    $this->callAPIFailure('Activity', 'get', array('check_permissions' => 1));
+  }
+
+  /**
+   * View all activities is required unless id is passed in, in which case ACLs are used.
+   */
+  public function testGetActivityViewAllContactsEnoughWIthID() {
+    $activity = $this->activityCreate();
+    $this->setPermissions(array('view all contacts', 'access CiviCRM'));
+    $this->callAPISuccess('Activity', 'getsingle', array('check_permissions' => 1, 'id' => $activity['id']));
+  }
+
+  /**
+   * View all activities is required unless id is passed in, in which case ACLs are used.
+   */
+  public function testGetActivityAccessCiviCRMNotEnough() {
+    $activity = $this->activityCreate();
+    $this->setPermissions(array('access CiviCRM'));
+    $this->callAPIFailure('Activity', 'getsingle', array('check_permissions' => 1, 'id' => $activity['id']));
+  }
+
+  /**
+   * Check that activities can be retrieved by ACL.
+   *
+   * The activities api applies ACLs in a very limited circumstance, if id is passed in.
+   * Otherwise it sticks with the blunt original permissions.
+   */
+  public function testGetActivityByACL() {
+    $this->setPermissions(array('access CiviCRM'));
+    $activity = $this->activityCreate();
+
+    $this->hookClass->setHook('civicrm_aclWhereClause', array($this, 'aclWhereHookAllResults'));
+    $this->callAPISuccess('Activity', 'getsingle', array('check_permissions' => 1, 'id' => $activity['id']));
+  }
+
+  /**
+   * To leverage ACL permission to view an activity you must be able to see all of the contacts.
+   */
+  public function testGetActivityByAclCannotViewAllContacts() {
+    $activity = $this->activityCreate();
+    $contacts = $this->getActivityContacts($activity);
+    $this->setPermissions(array('access CiviCRM'));
+
+    foreach ($contacts as $contact_id) {
+      $this->allowedContactId = $contact_id;
+      $this->hookClass->setHook('civicrm_aclWhereClause', array($this, 'aclWhereOnlyOne'));
+      $this->callAPIFailure('Activity', 'getsingle', array('check_permissions' => 1, 'id' => $activity['id']));
+    }
+  }
+
+  /**
+   * Check that if the source contact is deleted but we can view the others we can see the activity.
+   *
+   * CRM-18409.
+   *
+   * @throws \CRM_Core_Exception
+   */
+  public function testGetActivityACLSourceContactDeleted() {
+    $this->setPermissions(array('access CiviCRM', 'delete contacts'));
+    $activity = $this->activityCreate();
+    $contacts = $this->getActivityContacts($activity);
+
+    $this->hookClass->setHook('civicrm_aclWhereClause', array($this, 'aclWhereHookAllResults'));
+    $this->contactDelete($contacts['source_contact_id']);
+    $this->callAPISuccess('Activity', 'getsingle', array('check_permissions' => 1, 'id' => $activity['id']));
+  }
+
+  /**
+   * Get the contacts for the activity.
+   *
+   * @param $activity
+   *
+   * @return array
+   * @throws \CRM_Core_Exception
+   */
+  protected function getActivityContacts($activity) {
+    $contacts = array();
+
+    $activityContacts = $this->callAPISuccess('ActivityContact', 'get', array(
+        'activity_id' => $activity['id'],
+      )
+    );
+
+    $activityRecordTypes = $this->callAPISuccess('ActivityContact', 'getoptions', array('field' => 'record_type_id'));
+    foreach ($activityContacts['values'] as $activityContact) {
+      $type = $activityRecordTypes['values'][$activityContact['record_type_id']];
+      switch ($type) {
+        case 'Activity Source':
+          $contacts['source_contact_id'] = $activityContact['contact_id'];
+          break;
+
+        case 'Activity Targets':
+          $contacts['target_contact_id'] = $activityContact['contact_id'];
+          break;
+
+        case 'Activity Assignees':
+          $contacts['assignee_contact_id'] = $activityContact['contact_id'];
+          break;
+
+      }
+    }
+    return $contacts;
+  }
+
 }