From 340be2e7d78acae0d00dc0055d4072c0a284034d Mon Sep 17 00:00:00 2001 From: systopia Date: Thu, 13 Oct 2016 11:48:20 +0100 Subject: [PATCH] implementing @eileen's suggestions: - style - extra test - documentation - FIXME remarks --- CRM/Contact/BAO/Contact/Permission.php | 91 +++++++++++++------------- tests/phpunit/CRM/ACL/ListTest.php | 8 ++- 2 files changed, 54 insertions(+), 45 deletions(-) diff --git a/CRM/Contact/BAO/Contact/Permission.php b/CRM/Contact/BAO/Contact/Permission.php index 9cdcee8c0b..43c5e0b440 100644 --- a/CRM/Contact/BAO/Contact/Permission.php +++ b/CRM/Contact/BAO/Contact/Permission.php @@ -34,13 +34,15 @@ class CRM_Contact_BAO_Contact_Permission { /** * Check which of the given contact IDs the logged in user - * has permissions for the operation type according - * - * Caution: general permissions (like 'edit all contacts') + * has permissions for the operation type according to: + * - general permissions (e.g. 'edit all contacts') + * - deletion status (unless you have 'access deleted contacts') + * - ACL + * - permissions inherited through relationships (also second degree if enabled) * * @param array $contact_ids * Contact IDs. - * @param int|string $type the type of operation (view|edit) + * @param int $type the type of operation (view|edit) * * @see CRM_Contact_BAO_Contact_Permission::allow * @@ -53,7 +55,6 @@ class CRM_Contact_BAO_Contact_Permission { // empty contact lists would cause trouble in the SQL. And be pointless. return $result_set; } - $contact_id_list = implode(',', $contact_ids); // make sure the the general permissions are given if (CRM_Core_Permission::check('edit all contacts') @@ -67,6 +68,7 @@ class CRM_Contact_BAO_Contact_Permission { } else { // if the user CANNOT access deleted contacts, these need to be filtered + $contact_id_list = implode(',', $contact_ids); $filter_query = "SELECT DISTINCT(id) FROM civicrm_contact WHERE id IN ($contact_id_list) AND is_deleted = 0"; $query = CRM_Core_DAO::executeQuery($filter_query); while ($query->fetch()) { @@ -77,8 +79,7 @@ class CRM_Contact_BAO_Contact_Permission { } // get logged in user - $session = CRM_Core_Session::singleton(); - $contactID = (int) $session->get('userID'); + $contactID = CRM_Core_Session::getLoggedInContactID(); if (empty($contactID)) { return array(); } @@ -97,6 +98,7 @@ class CRM_Contact_BAO_Contact_Permission { } // RUN the query + $contact_id_list = implode(',', $contact_ids); $query = " SELECT contact_id FROM civicrm_acl_contact_cache @@ -134,8 +136,7 @@ WHERE contact_id IN ({$contact_id_list}) */ public static function allow($id, $type = CRM_Core_Permission::VIEW) { // get logged in user - $session = CRM_Core_Session::singleton(); - $contactID = (int) $session->get('userID'); + $contactID = CRM_Core_Session::getLoggedInContactID(); // first: check if contact is trying to view own contact if ($type == CRM_Core_Permission::VIEW && CRM_Core_Permission::check('view my contact') @@ -188,6 +189,10 @@ WHERE contact_a.id = %1 AND $permission"; * Should we force a recompute. */ public static function cache($userID, $type = CRM_Core_Permission::VIEW, $force = FALSE) { + // FIXME: maybe find a better way of keeping track of this. @eileen pointed out + // that somebody might flush the cache away from under our feet, + // but the altenative would be a SQL call every time this is called, + // and a complete rebuild if the result was an empty set... static $_processed = array( CRM_Core_Permission::VIEW => array(), CRM_Core_Permission::EDIT => array()); @@ -258,8 +263,7 @@ ON DUPLICATE KEY UPDATE $contactID = NULL ) { if (!$contactID) { - $session = CRM_Core_Session::singleton(); - $contactID = $session->get('userID'); + $contactID = CRM_Core_Session::getLoggedInContactID(); } if ($type = CRM_Core_Permission::VIEW) { @@ -353,12 +357,12 @@ AND $operationClause LIMIT 1"; * @return bool * true if logged in user has permission to view * selected contact record else false + * + * @deprecated should be replaced by a ::relationshipList(array($selectedContactID)) call */ public static function relationship($selectedContactID, $contactID = NULL) { - $session = CRM_Core_Session::singleton(); - $config = CRM_Core_Config::singleton(); if (!$contactID) { - $contactID = $session->get('userID'); + $contactID = CRM_Core_Session::getLoggedInContactID(); if (!$contactID) { return FALSE; } @@ -369,6 +373,8 @@ AND $operationClause LIMIT 1"; return TRUE; } else { + // FIXME: secondDegRelPermissions should be a setting + $config = CRM_Core_Config::singleton(); if ($config->secondDegRelPermissions) { $query = " SELECT firstdeg.id @@ -457,9 +463,7 @@ WHERE (( contact_id_a = %1 AND contact_id_b = %2 AND is_permission_a_b = 1 ) OR } // get the currently logged in user - $config = CRM_Core_Config::singleton(); - $session = CRM_Core_Session::singleton(); - $contactID = (int) $session->get('userID'); + $contactID = CRM_Core_Session::getLoggedInContactID(); if (empty($contactID)) { return array(); } @@ -484,7 +488,7 @@ WHERE (( contact_id_a = %1 AND contact_id_b = %2 AND is_permission_a_b = 1 ) OR } $queries[] = " -SELECT DISTINCT(civicrm_relationship.{$contact_id_column}) AS contact_id +SELECT civicrm_relationship.{$contact_id_column} AS contact_id FROM civicrm_relationship {$LEFT_JOIN_DELETED} WHERE civicrm_relationship.{$user_id_column} = {$contactID} @@ -494,39 +498,38 @@ SELECT DISTINCT(civicrm_relationship.{$contact_id_column}) AS contact_id $AND_CAN_ACCESS_DELETED"; } + // FIXME: secondDegRelPermissions should be a setting + $config = CRM_Core_Config::singleton(); if ($config->secondDegRelPermissions) { - // ADD SECOND DEGREE RELATIONSHIPS - if ($config->secondDegRelPermissions) { - foreach ($directions as $first_direction) { - foreach ($directions as $second_direction) { - // add clause for deleted contacts, if the user doesn't have the permission to access them - $LEFT_JOIN_DELETED = $AND_CAN_ACCESS_DELETED = ''; - if (!CRM_Core_Permission::check('access deleted contacts')) { - $LEFT_JOIN_DELETED = "LEFT JOIN civicrm_contact first_degree_contact ON first_degree_contact.id = second_degree_relationship.contact_id_{$second_direction['from']}\n"; - $LEFT_JOIN_DELETED .= "LEFT JOIN civicrm_contact second_degree_contact ON second_degree_contact.id = second_degree_relationship.contact_id_{$second_direction['to']} "; - $AND_CAN_ACCESS_DELETED = "AND first_degree_contact.is_deleted = 0\n"; - $AND_CAN_ACCESS_DELETED .= "AND second_degree_contact.is_deleted = 0 "; - } - - $queries[] = " - SELECT DISTINCT(second_degree_relationship.contact_id_{$second_direction['to']}) AS contact_id - FROM civicrm_relationship first_degree_relationship - LEFT JOIN civicrm_relationship second_degree_relationship ON first_degree_relationship.contact_id_{$first_direction['to']} = second_degree_relationship.contact_id_{$first_direction['from']} - {$LEFT_JOIN_DELETED} - WHERE first_degree_relationship.contact_id_{$first_direction['from']} = {$contactID} - AND second_degree_relationship.contact_id_{$second_direction['to']} IN ({$contact_id_list}) - AND first_degree_relationship.is_active = 1 - AND first_degree_relationship.is_permission_{$first_direction['from']}_{$first_direction['to']} = 1 - AND second_degree_relationship.is_active = 1 - AND second_degree_relationship.is_permission_{$second_direction['from']}_{$second_direction['to']} = 1 - $AND_CAN_ACCESS_DELETED"; + foreach ($directions as $first_direction) { + foreach ($directions as $second_direction) { + // add clause for deleted contacts, if the user doesn't have the permission to access them + $LEFT_JOIN_DELETED = $AND_CAN_ACCESS_DELETED = ''; + if (!CRM_Core_Permission::check('access deleted contacts')) { + $LEFT_JOIN_DELETED = "LEFT JOIN civicrm_contact first_degree_contact ON first_degree_contact.id = second_degree_relationship.contact_id_{$second_direction['from']}\n"; + $LEFT_JOIN_DELETED .= "LEFT JOIN civicrm_contact second_degree_contact ON second_degree_contact.id = second_degree_relationship.contact_id_{$second_direction['to']} "; + $AND_CAN_ACCESS_DELETED = "AND first_degree_contact.is_deleted = 0\n"; + $AND_CAN_ACCESS_DELETED .= "AND second_degree_contact.is_deleted = 0 "; } + + $queries[] = " +SELECT second_degree_relationship.contact_id_{$second_direction['to']} AS contact_id + FROM civicrm_relationship first_degree_relationship + LEFT JOIN civicrm_relationship second_degree_relationship ON first_degree_relationship.contact_id_{$first_direction['to']} = second_degree_relationship.contact_id_{$first_direction['from']} + {$LEFT_JOIN_DELETED} + WHERE first_degree_relationship.contact_id_{$first_direction['from']} = {$contactID} + AND second_degree_relationship.contact_id_{$second_direction['to']} IN ({$contact_id_list}) + AND first_degree_relationship.is_active = 1 + AND first_degree_relationship.is_permission_{$first_direction['from']}_{$first_direction['to']} = 1 + AND second_degree_relationship.is_active = 1 + AND second_degree_relationship.is_permission_{$second_direction['from']}_{$second_direction['to']} = 1 + $AND_CAN_ACCESS_DELETED"; } } } // finally UNION the queries and call - $query = "(" . implode(")\nUNION (", $queries) . ")"; + $query = "(" . implode(")\nUNION DISTINCT (", $queries) . ")"; $result = CRM_Core_DAO::executeQuery($query); while ($result->fetch()) { $result_set[(int) $result->contact_id] = TRUE; diff --git a/tests/phpunit/CRM/ACL/ListTest.php b/tests/phpunit/CRM/ACL/ListTest.php index 0b5b650806..c47594f9b6 100644 --- a/tests/phpunit/CRM/ACL/ListTest.php +++ b/tests/phpunit/CRM/ACL/ListTest.php @@ -28,7 +28,7 @@ class CRM_ACL_ListTest extends CiviUnitTestCase { $contacts = $this->createScenarioPlain(); // test WITH all permissions - CRM_Core_Config::singleton()->userPermissionClass->permissions = NULL; + CRM_Core_Config::singleton()->userPermissionClass->permissions = NULL; // NULL means 'all permissions' in UnitTests environment $result = CRM_Contact_BAO_Contact_Permission::allowList($contacts); sort($result); $this->assertEquals($result, $contacts, "Contacts should be viewable when 'view all contacts'"); @@ -39,6 +39,12 @@ class CRM_ACL_ListTest extends CiviUnitTestCase { sort($result); $this->assertEquals($result, $contacts, "Contacts should be viewable when 'view all contacts'"); + // test WITH EDIT permissions (should imply VIEW) + CRM_Core_Config::singleton()->userPermissionClass->permissions = array('edit all contacts'); + $result = CRM_Contact_BAO_Contact_Permission::allowList($contacts, CRM_Core_Permission::VIEW); + sort($result); + $this->assertEquals($result, $contacts, "Contacts should be viewable when 'edit all contacts'"); + // test WITHOUT permission CRM_Core_Config::singleton()->userPermissionClass->permissions = array(); $result = CRM_Contact_BAO_Contact_Permission::allowList($contacts); -- 2.25.1