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 154729bbee70f7ab721da8acbe0999200780796b..6c49ccbb23930ba39a9f304f59e8b676b4b95669 100644 (file)
--- 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 03e6c2f25f60e945ad5b379918c1363ce01ec436..04ad1cfbbb1fb9de2e7b4ec022d065f9901c33f5 100644 (file)
--- 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 494772a98b7b992be050599b88a79f2e736863cc..f2e0304e05bc2f925c5ae039be98970d16844017 100644 (file)
--- 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 5f79cf65ce34f25366c300f400eb85a48af3943d..d38085ca80166b8a37c29f9751eee9562692349f 100644 (file)
--- 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;
+  }
+
 }