From dddf4bf63e7e5e5dbb2d8f6bb2bb07f6c09aff4e Mon Sep 17 00:00:00 2001 From: systopia Date: Wed, 12 Oct 2016 00:51:28 +0100 Subject: [PATCH 01/22] added new list permission functions --- CRM/Contact/BAO/Contact/Permission.php | 171 +++++++++++++++++++++++++ 1 file changed, 171 insertions(+) diff --git a/CRM/Contact/BAO/Contact/Permission.php b/CRM/Contact/BAO/Contact/Permission.php index 5a3e4710c2d6..dc2f1fcb6325 100644 --- a/CRM/Contact/BAO/Contact/Permission.php +++ b/CRM/Contact/BAO/Contact/Permission.php @@ -32,6 +32,82 @@ */ 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') + * + * @param array $contact_ids + * Contact IDs. + * @param int|string $type the type of operation (view|edit) + * + * @see CRM_Contact_BAO_Contact_Permission::allow + * + * @return array + * list of contact IDs the logged in user has the given permission for + */ + public static function allowList($contact_ids, $type = CRM_Core_Permission::VIEW) { + $result_set = array(); + if (empty($contact_ids)) { + // empty contact lists would cause trouble in the SQL. And be pointless. + return $result_set; + } + + // make sure the the general permissions are given + if ( $type == CRM_Core_Permission::VIEW && CRM_Core_Permission::check('view all contacts') + || $type == CRM_Core_Permission::EDIT && CRM_Core_Permission::check('edit all contacts') + ) { + // if the general permission is there, all good + // TODO: deleted + return $contact_ids; + } + + // get logged in user + $session = CRM_Core_Session::singleton(); + $contactID = (int) $session->get('userID'); + if (empty($contactID)) { + return $result_set; + } + + // make sure the cache is filled + self::cache($contactID, $type); + + // compile query + $contact_id_list = implode(',', $contact_ids); + $operation = ($type == CRM_Core_Permission::VIEW) ? 'View' : 'Edit'; + + // add clause for deleted contacts, if the user doesn't have the permission to access them + $LEFT_JOIN_DELETED = $CAN_ACCESS_DELETED = ''; + if (!CRM_Core_Permission::check('access deleted contacts')) { + $LEFT_JOIN_DELETED = 'LEFT JOIN civicrm_contact ON civicrm_contact.id = contact_id'; + $AND_CAN_ACCESS_DELETED = 'AND civicrm_contact.is_deleted = 0'; + } + + // RUN the query + $query = " +SELECT contact_id + FROM civicrm_acl_contact_cache + {$LEFT_JOIN_DELETED} +WHERE contact_id IN ({$contact_id_list}) + AND user_id = {$contactID} + AND operation = '{$operation}' + {$AND_CAN_ACCESS_DELETED}"; + $result = CRM_Core_DAO::executeQuery($query); + while ($result->fetch()) { + $result_set[] = (int) $result->contact_id; + } + + // if some have been rejected, double check for permissions inherited by relationship + if (count($result_set) < count($contact_ids)) { + $rejected_contacts = array_diff($contact_ids, $result_set); + $allowed_by_relationship = self::relationshipList($rejected_contacts); + $result_set = array_merge($result_set, $allowed_by_relationship); + } + + return $result_set; + } + /** * Check if the logged in user has permissions for the operation type. * @@ -334,6 +410,101 @@ public static function relationship($selectedContactID, $contactID = NULL) { } + + /** + * Filter a list of contact_ids by the ones that the + * currently active user as a permissioned relationship with + * + * @param array $contact_ids + * List of contact IDs to be filtered + * + * @return array + * List of contact IDs that the user has permissions for + */ + public static function relationshipList($contact_ids) { + $result_set = array(); + + // no processing empty lists (avoid SQL errors as well) + if (empty($contact_ids)) { + return $result_set; + } + + // get the currently logged in user + $session = CRM_Core_Session::singleton(); + $contactID = (int) $session->get('userID'); + if (empty($contactID)) { + return $result_set; + } + + // compile a list of queries (later to UNION) + $queries = array(); + $contact_id_list = implode(',', $contact_ids); + + + // add a select for each direection + $directions = array(array('from' => 'a', 'to' => 'b'), array('from' => 'b', 'to' => 'a')); + foreach ($directions as $direction) { + $user_id_column = "contact_id_{$direction['from']}"; + $contact_id_column = "contact_id_{$direction['to']}"; + + // add clause for deleted contacts, if the user doesn't have the permission to access them + $LEFT_JOIN_DELETED = $CAN_ACCESS_DELETED = ''; + if (!CRM_Core_Permission::check('access deleted contacts')) { + $LEFT_JOIN_DELETED = 'LEFT JOIN civicrm_contact ON civicrm_contact.id = {$contact_id_column}'; + $AND_CAN_ACCESS_DELETED = 'AND civicrm_contact.is_deleted = 0'; + } + + $queries[] = " +SELECT DISTINCT(civicrm_relationship.{$contact_id_column}) AS contact_id + FROM civicrm_relationship + {$LEFT_JOIN_DELETED} + WHERE civicrm_relationship.{$user_id_column} = {$contactID} + AND civicrm_relationship.{$contact_id_column} IN ({$contact_id_list}) + AND civicrm_relationship.is_active = 1 + AND civicrm_relationship.is_permission_{$direction['from']}_{$direction['to']} = 1 + $AND_CAN_ACCESS_DELETED"; + } + + // add second degree relationship support + 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 = $CAN_ACCESS_DELETED = ''; + if (!CRM_Core_Permission::check('access deleted contacts')) { + $LEFT_JOIN_DELETED = 'LEFT JOIN civicrm_contact ON civicrm_contact.id = {$contact_id_column}'; + $AND_CAN_ACCESS_DELETED = 'AND civicrm_contact.is_deleted = 0'; + } + + $queries[] = " +SELECT DISTINCT(civicrm_relationship.{$contact_id_column}) 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) . ")"; + $result = CRM_Core_DAO::executeQuery($query); + while ($result->fetch()) { + $result_set[] = (int) $result->contact_id; + } + + return $result_set; + } + + + + /** * @param int $contactID * @param CRM_Core_Form $form From 2b8d25f0e5e29c6b8931e24eafe855fedc4ad3bc Mon Sep 17 00:00:00 2001 From: systopia Date: Wed, 12 Oct 2016 00:53:17 +0100 Subject: [PATCH 02/22] added check for 'view/edit my contact' --- CRM/Contact/BAO/Contact/Permission.php | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/CRM/Contact/BAO/Contact/Permission.php b/CRM/Contact/BAO/Contact/Permission.php index dc2f1fcb6325..a165a77048d1 100644 --- a/CRM/Contact/BAO/Contact/Permission.php +++ b/CRM/Contact/BAO/Contact/Permission.php @@ -121,6 +121,12 @@ public static function allowList($contact_ids, $type = CRM_Core_Permission::VIEW public static function allow($id, $type = CRM_Core_Permission::VIEW) { $tables = array(); $whereTables = array(); + // first: check if contact is trying to view own contact + if ( $type == CRM_Core_Permission::VIEW && CRM_Core_Permission::check('view my contact') + || $type == CRM_Core_Permission::EDIT && CRM_Core_Permission::check('edit my contact') + ) { + return TRUE; + } # FIXME: push this somewhere below, to not give this permission so many rights $isDeleted = (bool) CRM_Core_DAO::getFieldValue('CRM_Contact_DAO_Contact', $id, 'is_deleted'); From 9a41e1685f8a550dfbd740591be916c4b680a51b Mon Sep 17 00:00:00 2001 From: systopia Date: Wed, 12 Oct 2016 00:54:54 +0100 Subject: [PATCH 03/22] cleanup and documentation --- CRM/Contact/BAO/Contact/Permission.php | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/CRM/Contact/BAO/Contact/Permission.php b/CRM/Contact/BAO/Contact/Permission.php index a165a77048d1..36c61c506458 100644 --- a/CRM/Contact/BAO/Contact/Permission.php +++ b/CRM/Contact/BAO/Contact/Permission.php @@ -119,8 +119,10 @@ public static function allowList($contact_ids, $type = CRM_Core_Permission::VIEW * true if the user has permission, false otherwise */ public static function allow($id, $type = CRM_Core_Permission::VIEW) { - $tables = array(); - $whereTables = array(); + // get logged in user + $session = CRM_Core_Session::singleton(); + $contactID = (int) $session->get('userID'); + // first: check if contact is trying to view own contact if ( $type == CRM_Core_Permission::VIEW && CRM_Core_Permission::check('view my contact') || $type == CRM_Core_Permission::EDIT && CRM_Core_Permission::check('edit my contact') @@ -142,13 +144,16 @@ public static function allow($id, $type = CRM_Core_Permission::VIEW) { return TRUE; } - //check permission based on relationship, CRM-2963 + // check permission based on relationship, CRM-2963 if (self::relationship($id)) { return TRUE; } - $permission = CRM_ACL_API::whereClause($type, $tables, $whereTables); + // check permission based on ACL + $tables = array(); + $whereTables = array(); + $permission = CRM_ACL_API::whereClause($type, $tables, $whereTables); $from = CRM_Contact_BAO_Query::fromClause($whereTables); $query = " @@ -207,6 +212,7 @@ public static function cache($userID, $type = CRM_Core_Permission::VIEW, $force $from = CRM_Contact_BAO_Query::fromClause($whereTables); + // FIXME: don't use 'ON DUPLICATE KEY UPDATE' CRM_Core_DAO::executeQuery(" INSERT INTO civicrm_acl_contact_cache ( user_id, contact_id, operation ) SELECT $userID as user_id, contact_a.id as contact_id, '$operation' as operation From ea8011f678a76b372cb9e6c87e710f6810a73c8b Mon Sep 17 00:00:00 2001 From: systopia Date: Wed, 12 Oct 2016 00:55:16 +0100 Subject: [PATCH 04/22] started unit tests for new list permission functions --- tests/phpunit/CRM/ACL/ListTest.php | 127 +++++++++++++++++++++++++++++ 1 file changed, 127 insertions(+) create mode 100644 tests/phpunit/CRM/ACL/ListTest.php diff --git a/tests/phpunit/CRM/ACL/ListTest.php b/tests/phpunit/CRM/ACL/ListTest.php new file mode 100644 index 000000000000..1a3c2cdbbbec --- /dev/null +++ b/tests/phpunit/CRM/ACL/ListTest.php @@ -0,0 +1,127 @@ +useTransaction(TRUE); + } + + /** + * general test for the 'view all contacts' permission + */ + public function testViewAllPermission() { + // create test contacts + $contacts = $this->createScenarioA(); + // CRM_Core_Error::debug_log_message(json_encode($contacts)); + + // test WITH permission + CRM_Core_Config::singleton()->userPermissionClass->permissions = array('view all contacts'); + $result = CRM_Contact_BAO_Contact_Permission::allowList($contacts); + // CRM_Core_Error::debug_log_message(json_encode($result)); + $this->assertEqual($result, $contacts, "Contacts should be viewable when 'view all contacts'"); + + + // test WITH explicit permission + CRM_Core_Config::singleton()->userPermissionClass->permissions = array('view all contacts'); + $result = CRM_Contact_BAO_Contact_Permission::allowList($contacts, CRM_Core_Permission::VIEW); + // CRM_Core_Error::debug_log_message(json_encode($result)); + $this->assertEqual($result, $contacts, "Contacts should be viewable when 'view all contacts'"); + + + // test WITHOUT permission + CRM_Core_Config::singleton()->userPermissionClass->permissions = array(); + $result = CRM_Contact_BAO_Contact_Permission::allowList($contacts); + $this->assertEmpty($result, "Contacts should NOT be viewable when 'view all contacts' is not set"); + } + + + /** + * general test for the 'view all contacts' permission + */ + public function testEditAllPermission() { + // create test contacts + + $contacts = $this->createScenarioA(); + + // test WITH explicit permission + CRM_Core_Config::singleton()->userPermissionClass->permissions = array('edit all contacts'); + $result = CRM_Contact_BAO_Contact_Permission::allowList($contacts, CRM_Core_Permission::EDIT); + $this->assertEqual($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); + $this->assertEmpty($result, "Contacts should NOT be viewable when 'edit all contacts' is not set"); + } + + + /** + * general test for the 'view all contacts' permission + */ + public function testViewEditDeleted() { + CRM_Core_Config::singleton()->userPermissionClass->permissions = array('edit all contacts', 'view all contacts'); + $contacts = $this->createScenarioA(); + + + } + + + + + + + + /** + * create test scenario A + */ + protected function createScenarioA() { + // get logged in user + $user_id = $this->createLoggedInUser(); + $this->assertNotEmpty($user_id); + + // create test contacts + $bush_sr_id = $this->individualCreate(array('first_name' => 'George', 'middle_name' => 'W.', 'last_name' => 'Bush')); + $bush_jr_id = $this->individualCreate(array('first_name' => 'George', 'middle_name' => 'H. W.', 'last_name' => 'Bush')); + $bush_laura_id = $this->individualCreate(array('first_name' => 'Laura Lane', 'last_name' => 'Bush')); + $bush_brbra_id = $this->individualCreate(array('first_name' => 'Barbara', 'last_name' => 'Bush')); + + // create some relationships + $this->callAPISuccess('Relationship', 'create', array( + 'relationship_type_id' => 1, // CHILD OF + 'contact_id_a' => $bush_sr_id, + 'contact_id_b' => $user_id, + 'is_permission_a_b' => 1, + )); + + $this->callAPISuccess('Relationship', 'create', array( + 'relationship_type_id' => 1, // CHILD OF + 'contact_id_a' => $bush_jr_id, + 'contact_id_b' => $bush_sr_id, + 'is_permission_a_b' => 1, + )); + + // create some relationships + $this->callAPISuccess('Relationship', 'create', array( + 'relationship_type_id' => 1, // CHILD OF + 'contact_id_a' => $bush_brbra_id, + 'contact_id_b' => $bush_jr_id, + 'is_permission_a_b' => 1, + )); + + return array($user_id, $bush_sr_id, $bush_jr_id, $bush_laura_id, $bush_brbra_id); + } + +} From 67df1408361347ee10c2d6c1d18e48ed72f96226 Mon Sep 17 00:00:00 2001 From: systopia Date: Wed, 12 Oct 2016 12:27:42 +0100 Subject: [PATCH 05/22] adding new list permission functions (wip) --- CRM/Contact/BAO/Contact/Permission.php | 105 +++++++++++++++---------- 1 file changed, 63 insertions(+), 42 deletions(-) diff --git a/CRM/Contact/BAO/Contact/Permission.php b/CRM/Contact/BAO/Contact/Permission.php index 36c61c506458..ebbd26562578 100644 --- a/CRM/Contact/BAO/Contact/Permission.php +++ b/CRM/Contact/BAO/Contact/Permission.php @@ -53,35 +53,47 @@ public static function allowList($contact_ids, $type = CRM_Core_Permission::VIEW // 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 ( $type == CRM_Core_Permission::VIEW && CRM_Core_Permission::check('view all contacts') || $type == CRM_Core_Permission::EDIT && CRM_Core_Permission::check('edit all contacts') ) { + // if the general permission is there, all good - // TODO: deleted - return $contact_ids; + if (CRM_Core_Permission::check('access deleted contacts')) { + // if user can access delted contacts -> fine + return $contact_ids; + + } else { + // if the user CANNOT access deleted contacts, these need to be filtered + $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()) { + $result_set[(int) $query->id] = TRUE; + } + return array_keys($result_set); + } } // get logged in user $session = CRM_Core_Session::singleton(); $contactID = (int) $session->get('userID'); if (empty($contactID)) { - return $result_set; + return array(); } // make sure the cache is filled self::cache($contactID, $type); // compile query - $contact_id_list = implode(',', $contact_ids); $operation = ($type == CRM_Core_Permission::VIEW) ? 'View' : 'Edit'; // add clause for deleted contacts, if the user doesn't have the permission to access them $LEFT_JOIN_DELETED = $CAN_ACCESS_DELETED = ''; if (!CRM_Core_Permission::check('access deleted contacts')) { - $LEFT_JOIN_DELETED = 'LEFT JOIN civicrm_contact ON civicrm_contact.id = contact_id'; - $AND_CAN_ACCESS_DELETED = 'AND civicrm_contact.is_deleted = 0'; + $LEFT_JOIN_DELETED = "LEFT JOIN civicrm_contact ON civicrm_contact.id = contact_id"; + $AND_CAN_ACCESS_DELETED = "AND civicrm_contact.is_deleted = 0"; } // RUN the query @@ -95,17 +107,19 @@ public static function allowList($contact_ids, $type = CRM_Core_Permission::VIEW {$AND_CAN_ACCESS_DELETED}"; $result = CRM_Core_DAO::executeQuery($query); while ($result->fetch()) { - $result_set[] = (int) $result->contact_id; + $result_set[(int) $result->contact_id] = TRUE; } // if some have been rejected, double check for permissions inherited by relationship if (count($result_set) < count($contact_ids)) { - $rejected_contacts = array_diff($contact_ids, $result_set); + $rejected_contacts = array_diff_key($contact_ids, $result_set); $allowed_by_relationship = self::relationshipList($rejected_contacts); - $result_set = array_merge($result_set, $allowed_by_relationship); + foreach ($allowed_by_relationship as $contact_id) { + $result_set[(int) $contact_id] = TRUE; + } } - return $result_set; + return array_keys($result_set); } /** @@ -438,32 +452,34 @@ public static function relationshipList($contact_ids) { // no processing empty lists (avoid SQL errors as well) if (empty($contact_ids)) { - return $result_set; + return array(); } // get the currently logged in user + $config = CRM_Core_Config::singleton(); $session = CRM_Core_Session::singleton(); $contactID = (int) $session->get('userID'); if (empty($contactID)) { - return $result_set; + return array(); } // compile a list of queries (later to UNION) $queries = array(); $contact_id_list = implode(',', $contact_ids); - - // add a select for each direection + // add a select statement for each direection $directions = array(array('from' => 'a', 'to' => 'b'), array('from' => 'b', 'to' => 'a')); + + // NORMAL/SINGLE DEGREE RELATIONSHIPS foreach ($directions as $direction) { $user_id_column = "contact_id_{$direction['from']}"; $contact_id_column = "contact_id_{$direction['to']}"; // add clause for deleted contacts, if the user doesn't have the permission to access them - $LEFT_JOIN_DELETED = $CAN_ACCESS_DELETED = ''; + $LEFT_JOIN_DELETED = $AND_CAN_ACCESS_DELETED = ''; if (!CRM_Core_Permission::check('access deleted contacts')) { - $LEFT_JOIN_DELETED = 'LEFT JOIN civicrm_contact ON civicrm_contact.id = {$contact_id_column}'; - $AND_CAN_ACCESS_DELETED = 'AND civicrm_contact.is_deleted = 0'; + $LEFT_JOIN_DELETED = "LEFT JOIN civicrm_contact ON civicrm_contact.id = {$contact_id_column} "; + $AND_CAN_ACCESS_DELETED = "AND civicrm_contact.is_deleted = 0"; } $queries[] = " @@ -475,31 +491,36 @@ public static function relationshipList($contact_ids) { AND civicrm_relationship.is_active = 1 AND civicrm_relationship.is_permission_{$direction['from']}_{$direction['to']} = 1 $AND_CAN_ACCESS_DELETED"; - } + } + - // add second degree relationship support 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 = $CAN_ACCESS_DELETED = ''; - if (!CRM_Core_Permission::check('access deleted contacts')) { - $LEFT_JOIN_DELETED = 'LEFT JOIN civicrm_contact ON civicrm_contact.id = {$contact_id_column}'; - $AND_CAN_ACCESS_DELETED = 'AND civicrm_contact.is_deleted = 0'; + // 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"; } - - $queries[] = " -SELECT DISTINCT(civicrm_relationship.{$contact_id_column}) 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"; } } } @@ -508,10 +529,10 @@ public static function relationshipList($contact_ids) { $query = "(" . implode(")\nUNION (", $queries) . ")"; $result = CRM_Core_DAO::executeQuery($query); while ($result->fetch()) { - $result_set[] = (int) $result->contact_id; + $result_set[(int) $result->contact_id] = TRUE; } - - return $result_set; + $keys = array_keys($result_set); + return array_keys($result_set); } From 134b2b64005345d23e97d3c7ecd202072fbbf581 Mon Sep 17 00:00:00 2001 From: systopia Date: Wed, 12 Oct 2016 12:28:01 +0100 Subject: [PATCH 06/22] unit tests for new list permission functions (wip) --- tests/phpunit/CRM/ACL/ListTest.php | 148 ++++++++++++++++++++++++----- 1 file changed, 122 insertions(+), 26 deletions(-) diff --git a/tests/phpunit/CRM/ACL/ListTest.php b/tests/phpunit/CRM/ACL/ListTest.php index 1a3c2cdbbbec..18261bf0377a 100644 --- a/tests/phpunit/CRM/ACL/ListTest.php +++ b/tests/phpunit/CRM/ACL/ListTest.php @@ -23,26 +23,26 @@ public function setUp() { */ public function testViewAllPermission() { // create test contacts - $contacts = $this->createScenarioA(); - // CRM_Core_Error::debug_log_message(json_encode($contacts)); + $contacts = $this->createScenarioPlain(); - // test WITH permission - CRM_Core_Config::singleton()->userPermissionClass->permissions = array('view all contacts'); + // test WITH all permissions + CRM_Core_Config::singleton()->userPermissionClass->permissions = NULL; $result = CRM_Contact_BAO_Contact_Permission::allowList($contacts); - // CRM_Core_Error::debug_log_message(json_encode($result)); - $this->assertEqual($result, $contacts, "Contacts should be viewable when 'view all contacts'"); + sort($result); + $this->assertEquals($result, $contacts, "Contacts should be viewable when 'view all contacts'"); // test WITH explicit permission CRM_Core_Config::singleton()->userPermissionClass->permissions = array('view all contacts'); $result = CRM_Contact_BAO_Contact_Permission::allowList($contacts, CRM_Core_Permission::VIEW); - // CRM_Core_Error::debug_log_message(json_encode($result)); - $this->assertEqual($result, $contacts, "Contacts should be viewable when 'view all contacts'"); + sort($result); + $this->assertEquals($result, $contacts, "Contacts should be viewable when 'view all contacts'"); // test WITHOUT permission CRM_Core_Config::singleton()->userPermissionClass->permissions = array(); $result = CRM_Contact_BAO_Contact_Permission::allowList($contacts); + sort($result); $this->assertEmpty($result, "Contacts should NOT be viewable when 'view all contacts' is not set"); } @@ -52,42 +52,125 @@ public function testViewAllPermission() { */ public function testEditAllPermission() { // create test contacts - - $contacts = $this->createScenarioA(); + $contacts = $this->createScenarioPlain(); // test WITH explicit permission CRM_Core_Config::singleton()->userPermissionClass->permissions = array('edit all contacts'); $result = CRM_Contact_BAO_Contact_Permission::allowList($contacts, CRM_Core_Permission::EDIT); - $this->assertEqual($result, $contacts, "Contacts should be viewable when 'edit all contacts'"); + 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); + sort($result); $this->assertEmpty($result, "Contacts should NOT be viewable when 'edit all contacts' is not set"); } /** - * general test for the 'view all contacts' permission + * Test access related to the 'access deleted contact' permission */ public function testViewEditDeleted() { + // create test contacts + $contacts = $this->createScenarioPlain(); + + // delete one contact + $deleted_contact_id = $contacts[2]; + $this->callAPISuccess('Contact', 'create', array('id' => $deleted_contact_id, 'contact_is_deleted' => 1)); + $deleted_contact = $this->callAPISuccess('Contact', 'getsingle', array('id' => $deleted_contact_id)); + $this->assertEquals($deleted_contact['contact_is_deleted'], 1, "Contact should've been deleted"); + + // test WITH explicit permission CRM_Core_Config::singleton()->userPermissionClass->permissions = array('edit all contacts', 'view all contacts'); - $contacts = $this->createScenarioA(); + $result = CRM_Contact_BAO_Contact_Permission::allowList($contacts, CRM_Core_Permission::EDIT); + sort($result); + $this->assertNotContains($deleted_contact_id, $result, "Deleted contacts should be excluded"); + $this->assertEquals(count($result), count($contacts)-1, "Only deleted contacts should be excluded"); + + } + + /** + * Test access related to the 'access deleted contact' permission + * + * There should be the following permission-relationship + * contact[0] -> contact[1] -> contact[2] + */ + public function testPermissionByRelation() { + // create test scenario + $contacts = $this->createScenarioRelation(); + + // remove all permissions + $config = CRM_Core_Config::singleton(); + $config->userPermissionClass->permissions = array(); + $permissions_to_check = array(CRM_Core_Permission::VIEW => 'View', CRM_Core_Permission::EDIT => 'Edit'); + + // run this for SIMPLE relations + $config->secondDegRelPermissions = FALSE; + $this->assertFalse($config->secondDegRelPermissions); + foreach ($permissions_to_check as $permission => $permission_label) { + $result = CRM_Contact_BAO_Contact_Permission::allowList($contacts, $permission); + sort($result); + + + $this->assertNotContains($contacts[0], $result, "Contact[0] should NOT have $permission_label permission on contact[0]."); + $this->assertContains( $contacts[1], $result, "Contact[0] should have $permission_label permission on contact[1]."); + $this->assertNotContains($contacts[2], $result, "Contact[0] should NOT have $permission_label permission on contact[2]."); + $this->assertNotContains($contacts[3], $result, "Contact[0] should NOT have $permission_label permission on contact[3]."); + $this->assertNotContains($contacts[4], $result, "Contact[0] should NOT have $permission_label permission on contact[4]."); + } + // run this for SECOND DEGREE relations + $config->secondDegRelPermissions = TRUE; + $this->assertTrue($config->secondDegRelPermissions); + foreach ($permissions_to_check as $permission => $permission_label) { + $result = CRM_Contact_BAO_Contact_Permission::allowList($contacts, $permission); + sort($result); + + $this->assertNotContains($contacts[0], $result, "Contact[0] should NOT have $permission_label permission on contact[0]."); + $this->assertContains( $contacts[1], $result, "Contact[0] should have $permission_label permission on contact[1]."); + $this->assertContains( $contacts[2], $result, "Contact[0] should have second degree $permission_label permission on contact[2]."); + $this->assertNotContains($contacts[3], $result, "Contact[0] should NOT have $permission_label permission on contact[3]."); + $this->assertNotContains($contacts[4], $result, "Contact[0] should NOT have $permission_label permission on contact[4]."); + } } + /** + * Test access related to the 'access deleted contact' permission + */ + public function _testPermissionByACL() { + // CRM_Core_Config::singleton()->userPermissionClass->permissions = array('edit all contacts', 'view all contacts'); + // $contacts = $this->createScenarioPlain(); + } + /** + * Test access related to the 'access deleted contact' permission + */ + public function _testPermissionACLvsRelationship() { + // CRM_Core_Config::singleton()->userPermissionClass->permissions = array('edit all contacts', 'view all contacts'); + // $contacts = $this->createScenarioPlain(); + } + /** + * Test access related to the 'access deleted contact' permission + */ + public function _testPermissionCompare() { + // CRM_Core_Config::singleton()->userPermissionClass->permissions = array('edit all contacts', 'view all contacts'); + // $contacts = $this->createScenarioPlain(); + } + /**************************************************** + * Scenario Builders * + ***************************************************/ /** - * create test scenario A + * create plain test scenario, no relationships/ACLs */ - protected function createScenarioA() { + protected function createScenarioPlain() { // get logged in user $user_id = $this->createLoggedInUser(); $this->assertNotEmpty($user_id); @@ -98,30 +181,43 @@ protected function createScenarioA() { $bush_laura_id = $this->individualCreate(array('first_name' => 'Laura Lane', 'last_name' => 'Bush')); $bush_brbra_id = $this->individualCreate(array('first_name' => 'Barbara', 'last_name' => 'Bush')); + $contacts = array($user_id, $bush_sr_id, $bush_jr_id, $bush_laura_id, $bush_brbra_id); + sort($contacts); + return $contacts; + } + + /** + * create plain test scenario, no relationships/ACLs + */ + protected function createScenarioRelation() { + $contacts = $this->createScenarioPlain(); + // create some relationships $this->callAPISuccess('Relationship', 'create', array( 'relationship_type_id' => 1, // CHILD OF - 'contact_id_a' => $bush_sr_id, - 'contact_id_b' => $user_id, - 'is_permission_a_b' => 1, + 'contact_id_a' => $contacts[1], + 'contact_id_b' => $contacts[0], + 'is_permission_b_a' => 1, + 'is_active' => 1, )); $this->callAPISuccess('Relationship', 'create', array( 'relationship_type_id' => 1, // CHILD OF - 'contact_id_a' => $bush_jr_id, - 'contact_id_b' => $bush_sr_id, - 'is_permission_a_b' => 1, + 'contact_id_a' => $contacts[2], + 'contact_id_b' => $contacts[1], + 'is_permission_b_a' => 1, + 'is_active' => 1, )); // create some relationships $this->callAPISuccess('Relationship', 'create', array( 'relationship_type_id' => 1, // CHILD OF - 'contact_id_a' => $bush_brbra_id, - 'contact_id_b' => $bush_jr_id, - 'is_permission_a_b' => 1, + 'contact_id_a' => $contacts[4], + 'contact_id_b' => $contacts[2], + 'is_permission_b_a' => 1, + 'is_active' => 1, )); - return array($user_id, $bush_sr_id, $bush_jr_id, $bush_laura_id, $bush_brbra_id); + return $contacts; } - } From c1ebd31fa0f83ef01f6fa4792a0678153449548b Mon Sep 17 00:00:00 2001 From: systopia Date: Wed, 12 Oct 2016 14:02:06 +0100 Subject: [PATCH 07/22] unit tests for new list permission functions (wip) --- tests/phpunit/CRM/ACL/ListTest.php | 87 ++++++++++++++++++++++-------- 1 file changed, 66 insertions(+), 21 deletions(-) diff --git a/tests/phpunit/CRM/ACL/ListTest.php b/tests/phpunit/CRM/ACL/ListTest.php index 18261bf0377a..cc7c7b2b739f 100644 --- a/tests/phpunit/CRM/ACL/ListTest.php +++ b/tests/phpunit/CRM/ACL/ListTest.php @@ -16,6 +16,7 @@ class CRM_ACL_ListTest extends CiviUnitTestCase { public function setUp() { parent::setUp(); $this->useTransaction(TRUE); + $this->allowedContactsACL = array(); } /** @@ -93,14 +94,14 @@ public function testViewEditDeleted() { /** - * Test access related to the 'access deleted contact' permission + * Test access based on relations * * There should be the following permission-relationship * contact[0] -> contact[1] -> contact[2] */ public function testPermissionByRelation() { // create test scenario - $contacts = $this->createScenarioRelation(); + $contacts = $this->createScenarioRelations(); // remove all permissions $config = CRM_Core_Config::singleton(); @@ -115,11 +116,11 @@ public function testPermissionByRelation() { sort($result); - $this->assertNotContains($contacts[0], $result, "Contact[0] should NOT have $permission_label permission on contact[0]."); - $this->assertContains( $contacts[1], $result, "Contact[0] should have $permission_label permission on contact[1]."); - $this->assertNotContains($contacts[2], $result, "Contact[0] should NOT have $permission_label permission on contact[2]."); - $this->assertNotContains($contacts[3], $result, "Contact[0] should NOT have $permission_label permission on contact[3]."); - $this->assertNotContains($contacts[4], $result, "Contact[0] should NOT have $permission_label permission on contact[4]."); + $this->assertNotContains($contacts[0], $result, "User[0] should NOT have $permission_label permission on contact[0]."); + $this->assertContains( $contacts[1], $result, "User[0] should have $permission_label permission on contact[1]."); + $this->assertNotContains($contacts[2], $result, "User[0] should NOT have $permission_label permission on contact[2]."); + $this->assertNotContains($contacts[3], $result, "User[0] should NOT have $permission_label permission on contact[3]."); + $this->assertNotContains($contacts[4], $result, "User[0] should NOT have $permission_label permission on contact[4]."); } // run this for SECOND DEGREE relations @@ -129,29 +130,63 @@ public function testPermissionByRelation() { $result = CRM_Contact_BAO_Contact_Permission::allowList($contacts, $permission); sort($result); - $this->assertNotContains($contacts[0], $result, "Contact[0] should NOT have $permission_label permission on contact[0]."); - $this->assertContains( $contacts[1], $result, "Contact[0] should have $permission_label permission on contact[1]."); - $this->assertContains( $contacts[2], $result, "Contact[0] should have second degree $permission_label permission on contact[2]."); - $this->assertNotContains($contacts[3], $result, "Contact[0] should NOT have $permission_label permission on contact[3]."); - $this->assertNotContains($contacts[4], $result, "Contact[0] should NOT have $permission_label permission on contact[4]."); + $this->assertNotContains($contacts[0], $result, "User[0] should NOT have $permission_label permission on contact[0]."); + $this->assertContains( $contacts[1], $result, "User[0] should have $permission_label permission on contact[1]."); + $this->assertContains( $contacts[2], $result, "User[0] should have second degree $permission_label permission on contact[2]."); + $this->assertNotContains($contacts[3], $result, "User[0] should NOT have $permission_label permission on contact[3]."); + $this->assertNotContains($contacts[4], $result, "User[0] should NOT have $permission_label permission on contact[4]."); } } /** - * Test access related to the 'access deleted contact' permission + * Test access based on ACL */ - public function _testPermissionByACL() { - // CRM_Core_Config::singleton()->userPermissionClass->permissions = array('edit all contacts', 'view all contacts'); - // $contacts = $this->createScenarioPlain(); + public function testPermissionByACL() { + $contacts = $this->createScenarioPlain(); + + // set custom hook + $this->hookClass->setHook('civicrm_aclWhereClause', array($this, 'hook_civicrm_aclWhereClause')); + + // run simple test + $permissions_to_check = array(CRM_Core_Permission::VIEW => 'View', CRM_Core_Permission::EDIT => 'Edit'); + + $this->allowedContactsACL = array($contacts[0], $contacts[1], $contacts[4]); + CRM_Core_Config::singleton()->userPermissionClass->permissions = array(); + $result = CRM_Contact_BAO_Contact_Permission::allowList($contacts); + sort($result); + + $this->assertContains( $contacts[0], $result, "User[0] should NOT have an ACL permission on contact[0]."); + $this->assertContains( $contacts[1], $result, "User[0] should have an ACL permission on contact[1]."); + $this->assertNotContains($contacts[2], $result, "User[0] should NOT have an ACL permission on contact[2]."); + $this->assertNotContains($contacts[3], $result, "User[0] should NOT have an RELATION permission on contact[3]."); + $this->assertContains( $contacts[4], $result, "User[0] should NOT have an ACL permission on contact[4]."); } + /** - * Test access related to the 'access deleted contact' permission + * Test access with a mix of ACL and relationship */ - public function _testPermissionACLvsRelationship() { - // CRM_Core_Config::singleton()->userPermissionClass->permissions = array('edit all contacts', 'view all contacts'); - // $contacts = $this->createScenarioPlain(); + public function testPermissionACLvsRelationship() { + $contacts = $this->createScenarioRelations(); + + // set custom hook + $this->hookClass->setHook('civicrm_aclWhereClause', array($this, 'hook_civicrm_aclWhereClause')); + + $config = CRM_Core_Config::singleton(); + $config->userPermissionClass->permissions = array(); + $config->secondDegRelPermissions = TRUE; + + $this->allowedContactsACL = array($contacts[0], $contacts[1], $contacts[4]); + CRM_Core_Config::singleton()->userPermissionClass->permissions = array(); + $result = CRM_Contact_BAO_Contact_Permission::allowList($contacts); + sort($result); + + $this->assertContains( $contacts[0], $result, "User[0] should have an ACL permission on contact[0]."); + $this->assertContains( $contacts[1], $result, "User[0] should have an ACL permission on contact[1]."); + $this->assertContains( $contacts[2], $result, "User[0] should have second degree an relation permission on contact[2]."); + $this->assertNotContains($contacts[3], $result, "User[0] should NOT have an ACL permission on contact[3]."); + $this->assertContains( $contacts[4], $result, "User[0] should have an ACL permission on contact[4]."); } /** @@ -189,7 +224,7 @@ protected function createScenarioPlain() { /** * create plain test scenario, no relationships/ACLs */ - protected function createScenarioRelation() { + protected function createScenarioRelations() { $contacts = $this->createScenarioPlain(); // create some relationships @@ -220,4 +255,14 @@ protected function createScenarioRelation() { return $contacts; } + + /** + * ACL HOOK implementation for various tests + */ + public function hook_civicrm_aclWhereClause($type, &$tables, &$whereTables, &$contactID, &$where ) { + if (!empty($this->allowedContactsACL)) { + $contact_id_list = implode(',', $this->allowedContactsACL); + $where = " contact_a.id IN ($contact_id_list)"; + } + } } From 19f13a7cc8f6442bde01735c837c8ed27e9033f7 Mon Sep 17 00:00:00 2001 From: systopia Date: Wed, 12 Oct 2016 21:19:12 +0100 Subject: [PATCH 08/22] fixed: EDIT implies VIEW --- CRM/Contact/BAO/Contact/Permission.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/CRM/Contact/BAO/Contact/Permission.php b/CRM/Contact/BAO/Contact/Permission.php index ebbd26562578..e4ffc92d37ac 100644 --- a/CRM/Contact/BAO/Contact/Permission.php +++ b/CRM/Contact/BAO/Contact/Permission.php @@ -56,8 +56,8 @@ public static function allowList($contact_ids, $type = CRM_Core_Permission::VIEW $contact_id_list = implode(',', $contact_ids); // make sure the the general permissions are given - if ( $type == CRM_Core_Permission::VIEW && CRM_Core_Permission::check('view all contacts') - || $type == CRM_Core_Permission::EDIT && CRM_Core_Permission::check('edit all contacts') + if ( CRM_Core_Permission::check('edit all contacts') + || $type == CRM_Core_Permission::VIEW && CRM_Core_Permission::check('view all contacts') ) { // if the general permission is there, all good From c0e87307e210b824c99f90cb154ec46a0876e061 Mon Sep 17 00:00:00 2001 From: systopia Date: Wed, 12 Oct 2016 21:19:50 +0100 Subject: [PATCH 09/22] fixed bug in the original function --- CRM/Contact/BAO/Contact/Permission.php | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/CRM/Contact/BAO/Contact/Permission.php b/CRM/Contact/BAO/Contact/Permission.php index e4ffc92d37ac..44ba8e7d37eb 100644 --- a/CRM/Contact/BAO/Contact/Permission.php +++ b/CRM/Contact/BAO/Contact/Permission.php @@ -188,9 +188,10 @@ public static function allow($id, $type = CRM_Core_Permission::VIEW) { * Should we force a recompute. */ public static function cache($userID, $type = CRM_Core_Permission::VIEW, $force = FALSE) { - static $_processed = array(); + static $_processed = array( CRM_Core_Permission::VIEW => array(), + CRM_Core_Permission::EDIT => array()); - if ($type = CRM_Core_Permission::VIEW) { + if ($type == CRM_Core_Permission::VIEW) { $operationClause = " operation IN ( 'Edit', 'View' ) "; $operation = 'View'; } @@ -200,7 +201,8 @@ public static function cache($userID, $type = CRM_Core_Permission::VIEW, $force } if (!$force) { - if (!empty($_processed[$userID])) { + // skip if already calculated + if (!empty($_processed[$type][$userID])) { return; } @@ -214,7 +216,7 @@ public static function cache($userID, $type = CRM_Core_Permission::VIEW, $force $params = array(1 => array($userID, 'Integer')); $count = CRM_Core_DAO::singleValueQuery($sql, $params); if ($count > 0) { - $_processed[$userID] = 1; + $_processed[$type][$userID] = 1; return; } } @@ -238,8 +240,7 @@ public static function cache($userID, $type = CRM_Core_Permission::VIEW, $force contact_id=VALUES(contact_id), operation=VALUES(operation)" ); - - $_processed[$userID] = 1; + $_processed[$type][$userID] = 1; } /** From 3c645834c0ed85faf9be0c9dc5baba2f1b290026 Mon Sep 17 00:00:00 2001 From: systopia Date: Wed, 12 Oct 2016 21:21:08 +0100 Subject: [PATCH 10/22] finished unit tests --- tests/phpunit/CRM/ACL/ListTest.php | 55 ++++++++++++++++++++++++++++-- 1 file changed, 52 insertions(+), 3 deletions(-) diff --git a/tests/phpunit/CRM/ACL/ListTest.php b/tests/phpunit/CRM/ACL/ListTest.php index cc7c7b2b739f..85365751c4bc 100644 --- a/tests/phpunit/CRM/ACL/ListTest.php +++ b/tests/phpunit/CRM/ACL/ListTest.php @@ -15,6 +15,7 @@ class CRM_ACL_ListTest extends CiviUnitTestCase { */ public function setUp() { parent::setUp(); + // $this->quickCleanup(array('civicrm_acl_contact_cache'), TRUE); $this->useTransaction(TRUE); $this->allowedContactsACL = array(); } @@ -192,9 +193,57 @@ public function testPermissionACLvsRelationship() { /** * Test access related to the 'access deleted contact' permission */ - public function _testPermissionCompare() { - // CRM_Core_Config::singleton()->userPermissionClass->permissions = array('edit all contacts', 'view all contacts'); - // $contacts = $this->createScenarioPlain(); + public function testPermissionCompare() { + $contacts = $this->createScenarioRelations(); + $contact_index = array_flip($contacts); + + // set custom hook + $this->hookClass->setHook('civicrm_aclWhereClause', array($this, 'hook_civicrm_aclWhereClause')); + + $config = CRM_Core_Config::singleton(); + $this->allowedContactsACL = array($contacts[0], $contacts[1], $contacts[4]); + $config->secondDegRelPermissions = TRUE; + + // test configurations + $permissions_to_check = array(CRM_Core_Permission::VIEW => 'View', CRM_Core_Permission::EDIT => 'Edit'); + $user_permission_options = array(/*ALL*/ NULL, /*NONE*/ array(), array('view all contacts'), array('edit all contacts'), array('view all contacts', 'edit all contacts')); + + // run all combinations of those + foreach ($permissions_to_check as $permission_to_check => $permission_label) { + foreach ($user_permission_options as $user_permissions) { + // select the contact range + $contact_range = $contacts; + if (is_array($user_permissions) && count($user_permissions)==0) { + // slight (explainable) deviation on the own contact + unset($contact_range[0]); + } + + $config->userPermissionClass->permissions = $user_permissions; + $user_permissions_label = json_encode($user_permissions); + + // get the list result + $list_result = CRM_Contact_BAO_Contact_Permission::allowList($contact_range, $permission_to_check); + $this->assertTrue(count($list_result) <= count($contact_range), "Permission::allowList should return a subset of the contats."); + foreach ($list_result as $contact_id) { + $this->assertContains($contact_id, $contact_range, "Permission::allowList should return a subset of the contats."); + } + + // now compare the results + foreach ($contact_range as $contact_id) { + $individual_result = CRM_Contact_BAO_Contact_Permission::allow($contact_id, $permission_to_check); + + if (in_array($contact_id, $list_result)) { + // listPermission reports PERMISSION GRANTED + $this->assertTrue($individual_result, "Permission::allow denies {$permission_label} access for contact[{$contact_index[$contact_id]}], while Permission::allowList grants it. User permission: '{$user_permissions_label}'"); + + } else { + // listPermission reports PERMISSION DENIED + $this->assertFalse($individual_result, "Permission::allow grantes {$permission_label} access for contact[{$contact_index[$contact_id]}], while Permission::allowList denies it. User permission: '{$user_permissions_label}'"); + + } + } + } + } } From 730afb4688f483dfeb64cb8bfc0de327f22b8986 Mon Sep 17 00:00:00 2001 From: systopia Date: Wed, 12 Oct 2016 21:47:32 +0100 Subject: [PATCH 11/22] using new Permission::allowList to fix CRM-12645 --- CRM/Contact/Selector.php | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/CRM/Contact/Selector.php b/CRM/Contact/Selector.php index c436d83a1bdc..d77437d904a7 100644 --- a/CRM/Contact/Selector.php +++ b/CRM/Contact/Selector.php @@ -928,9 +928,20 @@ public function addActions(&$rows) { // mask value to hide map link if there are not lat/long $mapMask = $mask & 4095; - $links = self::links($this->_context, $this->_contextMenu, $this->_key); + // get permissions on an individual level (CRM-12645) + $can_edit_list = CRM_Contact_BAO_Contact_Permission::allowList(array_keys($rows), CRM_Core_Permission::EDIT); + + $links_template = self::links($this->_context, $this->_contextMenu, $this->_key); + foreach ($rows as $id => & $row) { + $links = $links_template; + + // remove edit/view links (CRM-12645) + if (isset($links[CRM_Core_Action::UPDATE]) && !in_array($id, $can_edit_list)) { + unset($links[CRM_Core_Action::UPDATE]); + } + if (!empty($this->_formValues['deleted_contacts']) && CRM_Core_Permission::check('access deleted contacts') ) { $links = array( From e4541c561c2dcbc6e5ba955c5798c5f6207da6d8 Mon Sep 17 00:00:00 2001 From: systopia Date: Wed, 12 Oct 2016 22:51:26 +0100 Subject: [PATCH 12/22] obeying master jenkins obeying master jenkins --- CRM/Contact/BAO/Contact/Permission.php | 33 ++++++++--------- CRM/Contact/Selector.php | 1 - tests/phpunit/CRM/ACL/ListTest.php | 51 ++++++++++++-------------- 3 files changed, 39 insertions(+), 46 deletions(-) diff --git a/CRM/Contact/BAO/Contact/Permission.php b/CRM/Contact/BAO/Contact/Permission.php index 44ba8e7d37eb..9cdcee8c0b42 100644 --- a/CRM/Contact/BAO/Contact/Permission.php +++ b/CRM/Contact/BAO/Contact/Permission.php @@ -33,7 +33,7 @@ class CRM_Contact_BAO_Contact_Permission { /** - * Check which of the given contact IDs the logged in user + * 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') @@ -56,7 +56,7 @@ public static function allowList($contact_ids, $type = CRM_Core_Permission::VIEW $contact_id_list = implode(',', $contact_ids); // make sure the the general permissions are given - if ( CRM_Core_Permission::check('edit all contacts') + if (CRM_Core_Permission::check('edit all contacts') || $type == CRM_Core_Permission::VIEW && CRM_Core_Permission::check('view all contacts') ) { @@ -64,8 +64,8 @@ public static function allowList($contact_ids, $type = CRM_Core_Permission::VIEW if (CRM_Core_Permission::check('access deleted contacts')) { // if user can access delted contacts -> fine return $contact_ids; - - } else { + } + else { // if the user CANNOT access deleted contacts, these need to be filtered $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); @@ -93,7 +93,7 @@ public static function allowList($contact_ids, $type = CRM_Core_Permission::VIEW $LEFT_JOIN_DELETED = $CAN_ACCESS_DELETED = ''; if (!CRM_Core_Permission::check('access deleted contacts')) { $LEFT_JOIN_DELETED = "LEFT JOIN civicrm_contact ON civicrm_contact.id = contact_id"; - $AND_CAN_ACCESS_DELETED = "AND civicrm_contact.is_deleted = 0"; + $AND_CAN_ACCESS_DELETED = "AND civicrm_contact.is_deleted = 0"; } // RUN the query @@ -110,7 +110,7 @@ public static function allowList($contact_ids, $type = CRM_Core_Permission::VIEW $result_set[(int) $result->contact_id] = TRUE; } - // if some have been rejected, double check for permissions inherited by relationship + // if some have been rejected, double check for permissions inherited by relationship if (count($result_set) < count($contact_ids)) { $rejected_contacts = array_diff_key($contact_ids, $result_set); $allowed_by_relationship = self::relationshipList($rejected_contacts); @@ -138,8 +138,8 @@ public static function allow($id, $type = CRM_Core_Permission::VIEW) { $contactID = (int) $session->get('userID'); // first: check if contact is trying to view own contact - if ( $type == CRM_Core_Permission::VIEW && CRM_Core_Permission::check('view my contact') - || $type == CRM_Core_Permission::EDIT && CRM_Core_Permission::check('edit my contact') + if ($type == CRM_Core_Permission::VIEW && CRM_Core_Permission::check('view my contact') + || $type == CRM_Core_Permission::EDIT && CRM_Core_Permission::check('edit my contact') ) { return TRUE; } @@ -188,8 +188,9 @@ public static function allow($id, $type = CRM_Core_Permission::VIEW) { * Should we force a recompute. */ public static function cache($userID, $type = CRM_Core_Permission::VIEW, $force = FALSE) { - static $_processed = array( CRM_Core_Permission::VIEW => array(), - CRM_Core_Permission::EDIT => array()); + static $_processed = array( + CRM_Core_Permission::VIEW => array(), + CRM_Core_Permission::EDIT => array()); if ($type == CRM_Core_Permission::VIEW) { $operationClause = " operation IN ( 'Edit', 'View' ) "; @@ -437,7 +438,6 @@ public static function relationship($selectedContactID, $contactID = NULL) { } - /** * Filter a list of contact_ids by the ones that the * currently active user as a permissioned relationship with @@ -450,7 +450,7 @@ public static function relationship($selectedContactID, $contactID = NULL) { */ public static function relationshipList($contact_ids) { $result_set = array(); - + // no processing empty lists (avoid SQL errors as well) if (empty($contact_ids)) { return array(); @@ -480,7 +480,7 @@ public static function relationshipList($contact_ids) { $LEFT_JOIN_DELETED = $AND_CAN_ACCESS_DELETED = ''; if (!CRM_Core_Permission::check('access deleted contacts')) { $LEFT_JOIN_DELETED = "LEFT JOIN civicrm_contact ON civicrm_contact.id = {$contact_id_column} "; - $AND_CAN_ACCESS_DELETED = "AND civicrm_contact.is_deleted = 0"; + $AND_CAN_ACCESS_DELETED = "AND civicrm_contact.is_deleted = 0"; } $queries[] = " @@ -492,8 +492,7 @@ public static function relationshipList($contact_ids) { AND civicrm_relationship.is_active = 1 AND civicrm_relationship.is_permission_{$direction['from']}_{$direction['to']} = 1 $AND_CAN_ACCESS_DELETED"; - } - + } if ($config->secondDegRelPermissions) { // ADD SECOND DEGREE RELATIONSHIPS @@ -501,7 +500,7 @@ public static function relationshipList($contact_ids) { 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 = ''; + $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']} "; @@ -537,8 +536,6 @@ public static function relationshipList($contact_ids) { } - - /** * @param int $contactID * @param CRM_Core_Form $form diff --git a/CRM/Contact/Selector.php b/CRM/Contact/Selector.php index d77437d904a7..a8e1aa30fc01 100644 --- a/CRM/Contact/Selector.php +++ b/CRM/Contact/Selector.php @@ -933,7 +933,6 @@ public function addActions(&$rows) { $links_template = self::links($this->_context, $this->_contextMenu, $this->_key); - foreach ($rows as $id => & $row) { $links = $links_template; diff --git a/tests/phpunit/CRM/ACL/ListTest.php b/tests/phpunit/CRM/ACL/ListTest.php index 85365751c4bc..0b5b650806cc 100644 --- a/tests/phpunit/CRM/ACL/ListTest.php +++ b/tests/phpunit/CRM/ACL/ListTest.php @@ -24,7 +24,7 @@ public function setUp() { * general test for the 'view all contacts' permission */ public function testViewAllPermission() { - // create test contacts + // create test contacts $contacts = $this->createScenarioPlain(); // test WITH all permissions @@ -33,14 +33,12 @@ public function testViewAllPermission() { sort($result); $this->assertEquals($result, $contacts, "Contacts should be viewable when 'view all contacts'"); - // test WITH explicit permission CRM_Core_Config::singleton()->userPermissionClass->permissions = array('view 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 'view all contacts'"); - // test WITHOUT permission CRM_Core_Config::singleton()->userPermissionClass->permissions = array(); $result = CRM_Contact_BAO_Contact_Permission::allowList($contacts); @@ -62,7 +60,6 @@ public function testEditAllPermission() { 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); @@ -89,14 +86,13 @@ public function testViewEditDeleted() { $result = CRM_Contact_BAO_Contact_Permission::allowList($contacts, CRM_Core_Permission::EDIT); sort($result); $this->assertNotContains($deleted_contact_id, $result, "Deleted contacts should be excluded"); - $this->assertEquals(count($result), count($contacts)-1, "Only deleted contacts should be excluded"); - + $this->assertEquals(count($result), count($contacts) - 1, "Only deleted contacts should be excluded"); } /** * Test access based on relations - * + * * There should be the following permission-relationship * contact[0] -> contact[1] -> contact[2] */ @@ -116,14 +112,13 @@ public function testPermissionByRelation() { $result = CRM_Contact_BAO_Contact_Permission::allowList($contacts, $permission); sort($result); - $this->assertNotContains($contacts[0], $result, "User[0] should NOT have $permission_label permission on contact[0]."); - $this->assertContains( $contacts[1], $result, "User[0] should have $permission_label permission on contact[1]."); + $this->assertContains($contacts[1], $result, "User[0] should have $permission_label permission on contact[1]."); $this->assertNotContains($contacts[2], $result, "User[0] should NOT have $permission_label permission on contact[2]."); $this->assertNotContains($contacts[3], $result, "User[0] should NOT have $permission_label permission on contact[3]."); $this->assertNotContains($contacts[4], $result, "User[0] should NOT have $permission_label permission on contact[4]."); } - + // run this for SECOND DEGREE relations $config->secondDegRelPermissions = TRUE; $this->assertTrue($config->secondDegRelPermissions); @@ -132,8 +127,8 @@ public function testPermissionByRelation() { sort($result); $this->assertNotContains($contacts[0], $result, "User[0] should NOT have $permission_label permission on contact[0]."); - $this->assertContains( $contacts[1], $result, "User[0] should have $permission_label permission on contact[1]."); - $this->assertContains( $contacts[2], $result, "User[0] should have second degree $permission_label permission on contact[2]."); + $this->assertContains($contacts[1], $result, "User[0] should have $permission_label permission on contact[1]."); + $this->assertContains($contacts[2], $result, "User[0] should have second degree $permission_label permission on contact[2]."); $this->assertNotContains($contacts[3], $result, "User[0] should NOT have $permission_label permission on contact[3]."); $this->assertNotContains($contacts[4], $result, "User[0] should NOT have $permission_label permission on contact[4]."); } @@ -157,11 +152,11 @@ public function testPermissionByACL() { $result = CRM_Contact_BAO_Contact_Permission::allowList($contacts); sort($result); - $this->assertContains( $contacts[0], $result, "User[0] should NOT have an ACL permission on contact[0]."); - $this->assertContains( $contacts[1], $result, "User[0] should have an ACL permission on contact[1]."); + $this->assertContains($contacts[0], $result, "User[0] should NOT have an ACL permission on contact[0]."); + $this->assertContains($contacts[1], $result, "User[0] should have an ACL permission on contact[1]."); $this->assertNotContains($contacts[2], $result, "User[0] should NOT have an ACL permission on contact[2]."); $this->assertNotContains($contacts[3], $result, "User[0] should NOT have an RELATION permission on contact[3]."); - $this->assertContains( $contacts[4], $result, "User[0] should NOT have an ACL permission on contact[4]."); + $this->assertContains($contacts[4], $result, "User[0] should NOT have an ACL permission on contact[4]."); } @@ -183,11 +178,11 @@ public function testPermissionACLvsRelationship() { $result = CRM_Contact_BAO_Contact_Permission::allowList($contacts); sort($result); - $this->assertContains( $contacts[0], $result, "User[0] should have an ACL permission on contact[0]."); - $this->assertContains( $contacts[1], $result, "User[0] should have an ACL permission on contact[1]."); - $this->assertContains( $contacts[2], $result, "User[0] should have second degree an relation permission on contact[2]."); + $this->assertContains($contacts[0], $result, "User[0] should have an ACL permission on contact[0]."); + $this->assertContains($contacts[1], $result, "User[0] should have an ACL permission on contact[1]."); + $this->assertContains($contacts[2], $result, "User[0] should have second degree an relation permission on contact[2]."); $this->assertNotContains($contacts[3], $result, "User[0] should NOT have an ACL permission on contact[3]."); - $this->assertContains( $contacts[4], $result, "User[0] should have an ACL permission on contact[4]."); + $this->assertContains($contacts[4], $result, "User[0] should have an ACL permission on contact[4]."); } /** @@ -213,7 +208,7 @@ public function testPermissionCompare() { foreach ($user_permission_options as $user_permissions) { // select the contact range $contact_range = $contacts; - if (is_array($user_permissions) && count($user_permissions)==0) { + if (is_array($user_permissions) && count($user_permissions) == 0) { // slight (explainable) deviation on the own contact unset($contact_range[0]); } @@ -236,7 +231,8 @@ public function testPermissionCompare() { // listPermission reports PERMISSION GRANTED $this->assertTrue($individual_result, "Permission::allow denies {$permission_label} access for contact[{$contact_index[$contact_id]}], while Permission::allowList grants it. User permission: '{$user_permissions_label}'"); - } else { + } + else { // listPermission reports PERMISSION DENIED $this->assertFalse($individual_result, "Permission::allow grantes {$permission_label} access for contact[{$contact_index[$contact_id]}], while Permission::allowList denies it. User permission: '{$user_permissions_label}'"); @@ -278,7 +274,7 @@ protected function createScenarioRelations() { // create some relationships $this->callAPISuccess('Relationship', 'create', array( - 'relationship_type_id' => 1, // CHILD OF + 'relationship_type_id' => 1, // CHILD OF 'contact_id_a' => $contacts[1], 'contact_id_b' => $contacts[0], 'is_permission_b_a' => 1, @@ -286,7 +282,7 @@ protected function createScenarioRelations() { )); $this->callAPISuccess('Relationship', 'create', array( - 'relationship_type_id' => 1, // CHILD OF + 'relationship_type_id' => 1, // CHILD OF 'contact_id_a' => $contacts[2], 'contact_id_b' => $contacts[1], 'is_permission_b_a' => 1, @@ -295,7 +291,7 @@ protected function createScenarioRelations() { // create some relationships $this->callAPISuccess('Relationship', 'create', array( - 'relationship_type_id' => 1, // CHILD OF + 'relationship_type_id' => 1, // CHILD OF 'contact_id_a' => $contacts[4], 'contact_id_b' => $contacts[2], 'is_permission_b_a' => 1, @@ -305,13 +301,14 @@ protected function createScenarioRelations() { return $contacts; } - /** - * ACL HOOK implementation for various tests + /** + * ACL HOOK implementation for various tests */ - public function hook_civicrm_aclWhereClause($type, &$tables, &$whereTables, &$contactID, &$where ) { + public function hook_civicrm_aclWhereClause($type, &$tables, &$whereTables, &$contactID, &$where) { if (!empty($this->allowedContactsACL)) { $contact_id_list = implode(',', $this->allowedContactsACL); $where = " contact_a.id IN ($contact_id_list)"; } } + } From 340be2e7d78acae0d00dc0055d4072c0a284034d Mon Sep 17 00:00:00 2001 From: systopia Date: Thu, 13 Oct 2016 11:48:20 +0100 Subject: [PATCH 13/22] 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 9cdcee8c0b42..43c5e0b440ff 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 @@ public static function allowList($contact_ids, $type = CRM_Core_Permission::VIEW // 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 @@ public static function allowList($contact_ids, $type = CRM_Core_Permission::VIEW } 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 @@ public static function allowList($contact_ids, $type = CRM_Core_Permission::VIEW } // 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 @@ public static function allowList($contact_ids, $type = CRM_Core_Permission::VIEW } // RUN the query + $contact_id_list = implode(',', $contact_ids); $query = " SELECT contact_id FROM civicrm_acl_contact_cache @@ -134,8 +136,7 @@ public static function allowList($contact_ids, $type = CRM_Core_Permission::VIEW */ 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 @@ public static function allow($id, $type = CRM_Core_Permission::VIEW) { * 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 @@ public static function hasContactsInCache( $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 @@ public static function cacheSubquery() { * @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 @@ public static function relationship($selectedContactID, $contactID = NULL) { 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 @@ public static function relationshipList($contact_ids) { } // 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 @@ public static function relationshipList($contact_ids) { } $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 @@ public static function relationshipList($contact_ids) { $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 0b5b650806cc..c47594f9b6a7 100644 --- a/tests/phpunit/CRM/ACL/ListTest.php +++ b/tests/phpunit/CRM/ACL/ListTest.php @@ -28,7 +28,7 @@ public function testViewAllPermission() { $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 @@ public function testViewAllPermission() { 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); From 163bfad3421e1f6f8c5a8bb0ccaceaa0f136dd70 Mon Sep 17 00:00:00 2001 From: eileen Date: Thu, 13 Oct 2016 15:10:51 +0100 Subject: [PATCH 14/22] Fix enotice --- CRM/Contact/BAO/Contact/Permission.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CRM/Contact/BAO/Contact/Permission.php b/CRM/Contact/BAO/Contact/Permission.php index 43c5e0b440ff..7098ee913871 100644 --- a/CRM/Contact/BAO/Contact/Permission.php +++ b/CRM/Contact/BAO/Contact/Permission.php @@ -91,7 +91,7 @@ public static function allowList($contact_ids, $type = CRM_Core_Permission::VIEW $operation = ($type == CRM_Core_Permission::VIEW) ? 'View' : 'Edit'; // add clause for deleted contacts, if the user doesn't have the permission to access them - $LEFT_JOIN_DELETED = $CAN_ACCESS_DELETED = ''; + $LEFT_JOIN_DELETED = $AND_CAN_ACCESS_DELETED = ''; if (!CRM_Core_Permission::check('access deleted contacts')) { $LEFT_JOIN_DELETED = "LEFT JOIN civicrm_contact ON civicrm_contact.id = contact_id"; $AND_CAN_ACCESS_DELETED = "AND civicrm_contact.is_deleted = 0"; From e8a0f9e01f917a75527b326f3fe3c65093f25ce4 Mon Sep 17 00:00:00 2001 From: eileen Date: Thu, 13 Oct 2016 17:04:56 +0100 Subject: [PATCH 15/22] Minor in-passing tidy-ups --- CRM/Contact/BAO/Contact/Permission.php | 3 +-- CRM/Contact/Form/Search.php | 6 ++---- CRM/Contact/Selector.php | 5 ----- 3 files changed, 3 insertions(+), 11 deletions(-) diff --git a/CRM/Contact/BAO/Contact/Permission.php b/CRM/Contact/BAO/Contact/Permission.php index 7098ee913871..befee1347ea6 100644 --- a/CRM/Contact/BAO/Contact/Permission.php +++ b/CRM/Contact/BAO/Contact/Permission.php @@ -63,7 +63,7 @@ public static function allowList($contact_ids, $type = CRM_Core_Permission::VIEW // if the general permission is there, all good if (CRM_Core_Permission::check('access deleted contacts')) { - // if user can access delted contacts -> fine + // if user can access deleted contacts -> fine return $contact_ids; } else { @@ -534,7 +534,6 @@ public static function relationshipList($contact_ids) { while ($result->fetch()) { $result_set[(int) $result->contact_id] = TRUE; } - $keys = array_keys($result_set); return array_keys($result_set); } diff --git a/CRM/Contact/Form/Search.php b/CRM/Contact/Form/Search.php index db90c66280eb..94fa086ec783 100644 --- a/CRM/Contact/Form/Search.php +++ b/CRM/Contact/Form/Search.php @@ -507,11 +507,9 @@ public function preProcess() { * driven by the wizard framework */ - $this->_reset = CRM_Utils_Request::retrieve('reset', 'Boolean', - CRM_Core_DAO::$_nullObject - ); + $this->_reset = CRM_Utils_Request::retrieve('reset', 'Boolean'); - $this->_force = CRM_Utils_Request::retrieve('force', 'Boolean', CRM_Core_DAO::$_nullObject); + $this->_force = CRM_Utils_Request::retrieve('force', 'Boolean'); $this->_groupID = CRM_Utils_Request::retrieve('gid', 'Positive', $this); $this->_amtgID = CRM_Utils_Request::retrieve('amtgID', 'Positive', $this); $this->_ssID = CRM_Utils_Request::retrieve('ssID', 'Positive', $this); diff --git a/CRM/Contact/Selector.php b/CRM/Contact/Selector.php index a8e1aa30fc01..563f2a9b11f4 100644 --- a/CRM/Contact/Selector.php +++ b/CRM/Contact/Selector.php @@ -542,7 +542,6 @@ public function getTotalCount($action) { * the total number of rows for this action */ public function &getRows($action, $offset, $rowCount, $sort, $output = NULL) { - $config = CRM_Core_Config::singleton(); if (($output == CRM_Core_Selector_Controller::EXPORT || $output == CRM_Core_Selector_Controller::SCREEN @@ -915,7 +914,6 @@ public function buildPrevNextCache($sort) { * @param $rows */ public function addActions(&$rows) { - $config = CRM_Core_Config::singleton(); $permissions = array(CRM_Core_Permission::getPermission()); if (CRM_Core_Permission::check('delete contacts')) { @@ -925,9 +923,6 @@ public function addActions(&$rows) { // mask value to hide map link if there are not lat/long $mapMask = $mask & 4095; - // mask value to hide map link if there are not lat/long - $mapMask = $mask & 4095; - // get permissions on an individual level (CRM-12645) $can_edit_list = CRM_Contact_BAO_Contact_Permission::allowList(array_keys($rows), CRM_Core_Permission::EDIT); From 98445ac505bda04132a23bb4a4732c773947e5ef Mon Sep 17 00:00:00 2001 From: eileen Date: Thu, 13 Oct 2016 17:17:55 +0100 Subject: [PATCH 16/22] CRM-12645 fix the code that calls the links function to not whomp it. Also, index rows by id so array_keys is useful --- CRM/Contact/BAO/Contact/Permission.php | 1 + CRM/Contact/Selector.php | 50 +++++++++----------------- 2 files changed, 18 insertions(+), 33 deletions(-) diff --git a/CRM/Contact/BAO/Contact/Permission.php b/CRM/Contact/BAO/Contact/Permission.php index befee1347ea6..432339166b7a 100644 --- a/CRM/Contact/BAO/Contact/Permission.php +++ b/CRM/Contact/BAO/Contact/Permission.php @@ -115,6 +115,7 @@ public static function allowList($contact_ids, $type = CRM_Core_Permission::VIEW // if some have been rejected, double check for permissions inherited by relationship if (count($result_set) < count($contact_ids)) { $rejected_contacts = array_diff_key($contact_ids, $result_set); + // @todo consider storing these to the acl cache for next time, since we have fetched. $allowed_by_relationship = self::relationshipList($rejected_contacts); foreach ($allowed_by_relationship as $contact_id) { $result_set[(int) $contact_id] = TRUE; diff --git a/CRM/Contact/Selector.php b/CRM/Contact/Selector.php index 563f2a9b11f4..3e36f01d38d5 100644 --- a/CRM/Contact/Selector.php +++ b/CRM/Contact/Selector.php @@ -653,7 +653,6 @@ public function &getRows($action, $offset, $rowCount, $sort, $output = NULL) { ); } - $seenIDs = array(); while ($result->fetch()) { $row = array(); $this->_query->convertToPseudoNames($result); @@ -838,16 +837,13 @@ public function &getRows($action, $offset, $rowCount, $sort, $output = NULL) { $row['contact_sub_type'] = $result->contact_sub_type ? CRM_Contact_BAO_ContactType::contactTypePairs(FALSE, $result->contact_sub_type, ', ') : $result->contact_sub_type; $row['contact_id'] = $result->contact_id; $row['sort_name'] = $result->sort_name; + // Surely this if should be if NOT - otherwise it's just wierd. if (array_key_exists('id', $row)) { $row['id'] = $result->contact_id; } } - // Dedupe contacts - if (in_array($row['contact_id'], $seenIDs) === FALSE) { - $seenIDs[] = $row['contact_id']; - $rows[] = $row; - } + $rows[$row['contact_id']] = $row; } return $rows; @@ -915,25 +911,29 @@ public function buildPrevNextCache($sort) { */ public function addActions(&$rows) { - $permissions = array(CRM_Core_Permission::getPermission()); - if (CRM_Core_Permission::check('delete contacts')) { - $permissions[] = CRM_Core_Permission::DELETE; - } - $mask = CRM_Core_Action::mask($permissions); - // mask value to hide map link if there are not lat/long - $mapMask = $mask & 4095; + $basicPermissions = CRM_Core_Permission::check('delete contacts') ? array(CRM_Core_Permission::DELETE) : array(); // get permissions on an individual level (CRM-12645) + // @todo look at storing this to the session as this is called twice during search results render. $can_edit_list = CRM_Contact_BAO_Contact_Permission::allowList(array_keys($rows), CRM_Core_Permission::EDIT); $links_template = self::links($this->_context, $this->_contextMenu, $this->_key); foreach ($rows as $id => & $row) { $links = $links_template; + if (in_array($id, $can_edit_list)) { + $mask = CRM_Core_Action::mask(array_merge(array(CRM_Core_Permission::EDIT), $basicPermissions)); + } + else { + $mask = CRM_Core_Action::mask(array_merge(array(CRM_Core_Permission::VIEW), $basicPermissions)); + } - // remove edit/view links (CRM-12645) - if (isset($links[CRM_Core_Action::UPDATE]) && !in_array($id, $can_edit_list)) { - unset($links[CRM_Core_Action::UPDATE]); + if ((!is_numeric(CRM_Utils_Array::value('geo_code_1', $row))) && + (empty($row['city']) || + !CRM_Utils_Array::value('state_province', $row) + ) + ) { + $mask = $mask & 4095; } if (!empty($this->_formValues['deleted_contacts']) && CRM_Core_Permission::check('access deleted contacts') @@ -972,26 +972,10 @@ public function addActions(&$rows) { $row['contact_id'] ); } - elseif ((is_numeric(CRM_Utils_Array::value('geo_code_1', $row))) || - (!empty($row['city']) && - CRM_Utils_Array::value('state_province', $row) - ) - ) { - $row['action'] = CRM_Core_Action::formLink( - $links, - $mask, - array('id' => $row['contact_id']), - ts('more'), - FALSE, - 'contact.selector.actions', - 'Contact', - $row['contact_id'] - ); - } else { $row['action'] = CRM_Core_Action::formLink( $links, - $mapMask, + $mask, array('id' => $row['contact_id']), ts('more'), FALSE, From 0f765440522a78052e9dc73cdac568d2e24fb6b9 Mon Sep 17 00:00:00 2001 From: eileenmcnaugton Date: Mon, 29 Feb 2016 18:48:46 +1300 Subject: [PATCH 17/22] CRM-18120 make acl query less debilitating --- CRM/Contact/BAO/Contact/Permission.php | 13 ++++--------- 1 file changed, 4 insertions(+), 9 deletions(-) diff --git a/CRM/Contact/BAO/Contact/Permission.php b/CRM/Contact/BAO/Contact/Permission.php index 432339166b7a..bbdc30ea2c6c 100644 --- a/CRM/Contact/BAO/Contact/Permission.php +++ b/CRM/Contact/BAO/Contact/Permission.php @@ -234,19 +234,14 @@ public static function cache($userID, $type = CRM_Core_Permission::VIEW, $force $permission = CRM_ACL_API::whereClause($type, $tables, $whereTables, $userID); $from = CRM_Contact_BAO_Query::fromClause($whereTables); - - // FIXME: don't use 'ON DUPLICATE KEY UPDATE' CRM_Core_DAO::executeQuery(" INSERT INTO civicrm_acl_contact_cache ( user_id, contact_id, operation ) -SELECT $userID as user_id, contact_a.id as contact_id, '$operation' as operation +SELECT DISTINCT $userID as user_id, contact_a.id as contact_id, '{$operation}' as operation $from + LEFT JOIN civicrm_acl_contact_cache ac ON ac.user_id = $userID AND ac.contact_id = contact_a.id AND ac.operation = '{$operation}' WHERE $permission -GROUP BY contact_a.id -ON DUPLICATE KEY UPDATE - user_id=VALUES(user_id), - contact_id=VALUES(contact_id), - operation=VALUES(operation)" - ); +AND ac.user_id IS NULL +"); $_processed[$type][$userID] = 1; } From 135367a61b21afe17c174851fa0a68fd0d1ddf4a Mon Sep 17 00:00:00 2001 From: eileen Date: Fri, 14 Oct 2016 15:24:13 +0100 Subject: [PATCH 18/22] CRM-12645 fix regression in previous refactor --- CRM/Contact/BAO/Contact/Permission.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/CRM/Contact/BAO/Contact/Permission.php b/CRM/Contact/BAO/Contact/Permission.php index bbdc30ea2c6c..0112b38a06f2 100644 --- a/CRM/Contact/BAO/Contact/Permission.php +++ b/CRM/Contact/BAO/Contact/Permission.php @@ -140,8 +140,8 @@ public static function allow($id, $type = CRM_Core_Permission::VIEW) { $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') - || $type == CRM_Core_Permission::EDIT && CRM_Core_Permission::check('edit my contact') + if ($contactID == $id && ($type == CRM_Core_Permission::VIEW && CRM_Core_Permission::check('view my contact') + || $type == CRM_Core_Permission::EDIT && CRM_Core_Permission::check('edit my contact')) ) { return TRUE; } From 680a52de96590a946e3771376edae395108295c2 Mon Sep 17 00:00:00 2001 From: eileen Date: Fri, 14 Oct 2016 16:06:29 +0100 Subject: [PATCH 19/22] CRM-12645 remove replaced function --- CRM/Contact/BAO/Contact/Permission.php | 111 ++----------------------- 1 file changed, 8 insertions(+), 103 deletions(-) diff --git a/CRM/Contact/BAO/Contact/Permission.php b/CRM/Contact/BAO/Contact/Permission.php index 0112b38a06f2..2dfb14ce015b 100644 --- a/CRM/Contact/BAO/Contact/Permission.php +++ b/CRM/Contact/BAO/Contact/Permission.php @@ -161,7 +161,7 @@ public static function allow($id, $type = CRM_Core_Permission::VIEW) { } // check permission based on relationship, CRM-2963 - if (self::relationship($id)) { + if (self::relationshipList(array($id))) { return TRUE; } @@ -173,12 +173,15 @@ public static function allow($id, $type = CRM_Core_Permission::VIEW) { $from = CRM_Contact_BAO_Query::fromClause($whereTables); $query = " -SELECT count(DISTINCT contact_a.id) +SELECT contact_a.id $from -WHERE contact_a.id = %1 AND $permission"; - $params = array(1 => array($id, 'Integer')); +WHERE contact_a.id = %1 AND $permission + LIMIT 1 +"; - return (CRM_Core_DAO::singleValueQuery($query, $params) > 0) ? TRUE : FALSE; + if (CRM_Core_DAO::singleValueQuery($query, array(1 => array($id, 'Integer')))) { + return TRUE; + } } /** @@ -342,104 +345,6 @@ public static function cacheSubquery() { return NULL; } - /** - * Get the permission base on its relationship. - * - * @param int $selectedContactID - * Contact id of selected contact. - * @param int $contactID - * Contact id of the current contact. - * - * @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) { - if (!$contactID) { - $contactID = CRM_Core_Session::getLoggedInContactID(); - if (!$contactID) { - return FALSE; - } - } - if ($contactID == $selectedContactID && - (CRM_Core_Permission::check('edit my contact')) - ) { - return TRUE; - } - else { - // FIXME: secondDegRelPermissions should be a setting - $config = CRM_Core_Config::singleton(); - if ($config->secondDegRelPermissions) { - $query = " -SELECT firstdeg.id -FROM civicrm_relationship firstdeg -LEFT JOIN civicrm_relationship seconddegaa - on firstdeg.contact_id_a = seconddegaa.contact_id_b - and seconddegaa.is_permission_b_a = 1 - and firstdeg.is_permission_b_a = 1 - and seconddegaa.is_active = 1 -LEFT JOIN civicrm_relationship seconddegab - on firstdeg.contact_id_a = seconddegab.contact_id_a - and seconddegab.is_permission_a_b = 1 - and firstdeg.is_permission_b_a = 1 - and seconddegab.is_active = 1 -LEFT JOIN civicrm_relationship seconddegba - on firstdeg.contact_id_b = seconddegba.contact_id_b - and seconddegba.is_permission_b_a = 1 - and firstdeg.is_permission_a_b = 1 - and seconddegba.is_active = 1 -LEFT JOIN civicrm_relationship seconddegbb - on firstdeg.contact_id_b = seconddegbb.contact_id_a - and seconddegbb.is_permission_a_b = 1 - and firstdeg.is_permission_a_b = 1 - and seconddegbb.is_active = 1 -WHERE - ( - ( firstdeg.contact_id_a = %1 AND firstdeg.contact_id_b = %2 AND firstdeg.is_permission_a_b = 1 ) - OR ( firstdeg.contact_id_a = %2 AND firstdeg.contact_id_b = %1 AND firstdeg.is_permission_b_a = 1 ) - OR ( - firstdeg.contact_id_a = %1 AND seconddegba.contact_id_a = %2 - AND (seconddegba.contact_id_a NOT IN (SELECT id FROM civicrm_contact WHERE is_deleted = 1)) - ) - OR ( - firstdeg.contact_id_a = %1 AND seconddegbb.contact_id_b = %2 - AND (seconddegbb.contact_id_b NOT IN (SELECT id FROM civicrm_contact WHERE is_deleted = 1)) - ) - OR ( - firstdeg.contact_id_b = %1 AND seconddegab.contact_id_b = %2 - AND (seconddegab.contact_id_b NOT IN (SELECT id FROM civicrm_contact WHERE is_deleted = 1)) - ) - OR ( - firstdeg.contact_id_b = %1 AND seconddegaa.contact_id_a = %2 AND (seconddegaa.contact_id_a NOT IN (SELECT id FROM civicrm_contact WHERE is_deleted = 1)) - ) - ) - AND (firstdeg.contact_id_a NOT IN (SELECT id FROM civicrm_contact WHERE is_deleted = 1)) - AND (firstdeg.contact_id_b NOT IN (SELECT id FROM civicrm_contact WHERE is_deleted = 1)) - AND ( firstdeg.is_active = 1) - "; - } - else { - $query = " -SELECT id -FROM civicrm_relationship -WHERE (( contact_id_a = %1 AND contact_id_b = %2 AND is_permission_a_b = 1 ) OR - ( contact_id_a = %2 AND contact_id_b = %1 AND is_permission_b_a = 1 )) AND - (contact_id_a NOT IN (SELECT id FROM civicrm_contact WHERE is_deleted = 1)) AND - (contact_id_b NOT IN (SELECT id FROM civicrm_contact WHERE is_deleted = 1)) - AND ( civicrm_relationship.is_active = 1 ) -"; - } - $params = array( - 1 => array($contactID, 'Integer'), - 2 => array($selectedContactID, 'Integer'), - ); - return CRM_Core_DAO::singleValueQuery($query, $params); - } - } - - /** * Filter a list of contact_ids by the ones that the * currently active user as a permissioned relationship with From 8210399ab45f3bc992536305d9cf8f9c7ada246b Mon Sep 17 00:00:00 2001 From: eileen Date: Fri, 14 Oct 2016 16:13:23 +0100 Subject: [PATCH 20/22] CRM-12645 remove unused function --- CRM/Contact/BAO/Contact/Permission.php | 40 +------------------------- 1 file changed, 1 insertion(+), 39 deletions(-) diff --git a/CRM/Contact/BAO/Contact/Permission.php b/CRM/Contact/BAO/Contact/Permission.php index 2dfb14ce015b..93d0917042ea 100644 --- a/CRM/Contact/BAO/Contact/Permission.php +++ b/CRM/Contact/BAO/Contact/Permission.php @@ -165,6 +165,7 @@ public static function allow($id, $type = CRM_Core_Permission::VIEW) { return TRUE; } + // We should probably do a cheap check whether it's in the cache first. // check permission based on ACL $tables = array(); $whereTables = array(); @@ -248,45 +249,6 @@ public static function cache($userID, $type = CRM_Core_Permission::VIEW, $force $_processed[$type][$userID] = 1; } - /** - * Check if there are any contacts in cache table. - * - * @param int|string $type the type of operation (view|edit) - * @param int $contactID - * Contact id. - * - * @return bool - */ - public static function hasContactsInCache( - $type = CRM_Core_Permission::VIEW, - $contactID = NULL - ) { - if (!$contactID) { - $contactID = CRM_Core_Session::getLoggedInContactID(); - } - - if ($type = CRM_Core_Permission::VIEW) { - $operationClause = " operation IN ( 'Edit', 'View' ) "; - $operation = 'View'; - } - else { - $operationClause = " operation = 'Edit' "; - $operation = 'Edit'; - } - - // fill cache - self::cache($contactID); - - $sql = " -SELECT id -FROM civicrm_acl_contact_cache -WHERE user_id = %1 -AND $operationClause LIMIT 1"; - - $params = array(1 => array($contactID, 'Integer')); - return (bool) CRM_Core_DAO::singleValueQuery($sql, $params); - } - /** * @param string $contactAlias * From 5f652ac7f5328d3242e6868eed75ed874ba7beb6 Mon Sep 17 00:00:00 2001 From: eileen Date: Fri, 14 Oct 2016 18:08:20 +0100 Subject: [PATCH 21/22] Return explicit FALSE for test expectation Change-Id: Ic975b736eae4edf145636b01cb6e618bbfc0ab70 --- CRM/Contact/BAO/Contact/Permission.php | 1 + 1 file changed, 1 insertion(+) diff --git a/CRM/Contact/BAO/Contact/Permission.php b/CRM/Contact/BAO/Contact/Permission.php index 93d0917042ea..88767deb9a8d 100644 --- a/CRM/Contact/BAO/Contact/Permission.php +++ b/CRM/Contact/BAO/Contact/Permission.php @@ -183,6 +183,7 @@ public static function allow($id, $type = CRM_Core_Permission::VIEW) { if (CRM_Core_DAO::singleValueQuery($query, array(1 => array($id, 'Integer')))) { return TRUE; } + return FALSE; } /** From 9aea8e14859f3502e257ffea9315ee8d62d04abc Mon Sep 17 00:00:00 2001 From: eileenmcnaugton Date: Mon, 24 Oct 2016 21:50:26 +1300 Subject: [PATCH 22/22] CRM-19557 Fix ACL caching function to not use inefficient query for view my contact Conflicts: CRM/Contact/BAO/Contact/Permission.php --- CRM/ACL/API.php | 13 +++++++++---- CRM/Contact/BAO/Contact/Permission.php | 20 +++++++++++++++----- 2 files changed, 24 insertions(+), 9 deletions(-) diff --git a/CRM/ACL/API.php b/CRM/ACL/API.php index 113bcce0e925..27ce2b1561a7 100644 --- a/CRM/ACL/API.php +++ b/CRM/ACL/API.php @@ -84,6 +84,10 @@ public static function check($str, $contactID = NULL) { * @param bool $skipDeleteClause * Don't add delete clause if this is true,. * this means it is handled by generating query + * @param bool $skipOwnContactClause + * Do not add 'OR contact_id = $userID' to the where clause. + * This is a hideously inefficient query and should be avoided + * wherever possible. * * @return string * the group where clause for this user @@ -94,7 +98,8 @@ public static function whereClause( &$whereTables, $contactID = NULL, $onlyDeleted = FALSE, - $skipDeleteClause = FALSE + $skipDeleteClause = FALSE, + $skipOwnContactClause = FALSE ) { // the default value which is valid for the final AND $deleteClause = ' ( 1 ) '; @@ -131,9 +136,9 @@ public static function whereClause( ) ); - // Add permission on self - if ($contactID && (CRM_Core_Permission::check('edit my contact') || - $type == self::VIEW && CRM_Core_Permission::check('view my contact')) + // Add permission on self if we really hate our server or have hardly any contacts. + if (!$skipOwnContactClause && $contactID && (CRM_Core_Permission::check('edit my contact') || + $type == self::VIEW && CRM_Core_Permission::check('view my contact')) ) { $where = "(contact_a.id = $contactID OR ($where))"; } diff --git a/CRM/Contact/BAO/Contact/Permission.php b/CRM/Contact/BAO/Contact/Permission.php index 88767deb9a8d..ceb765cdad9d 100644 --- a/CRM/Contact/BAO/Contact/Permission.php +++ b/CRM/Contact/BAO/Contact/Permission.php @@ -103,7 +103,7 @@ public static function allowList($contact_ids, $type = CRM_Core_Permission::VIEW SELECT contact_id FROM civicrm_acl_contact_cache {$LEFT_JOIN_DELETED} -WHERE contact_id IN ({$contact_id_list}) +WHERE contact_id IN ({$contact_id_list}) AND user_id = {$contactID} AND operation = '{$operation}' {$AND_CAN_ACCESS_DELETED}"; @@ -211,6 +211,7 @@ public static function cache($userID, $type = CRM_Core_Permission::VIEW, $force $operationClause = " operation = 'Edit' "; $operation = 'Edit'; } + $queryParams = array(1 => array($userID, 'Integer')); if (!$force) { // skip if already calculated @@ -225,8 +226,7 @@ public static function cache($userID, $type = CRM_Core_Permission::VIEW, $force WHERE user_id = %1 AND $operationClause "; - $params = array(1 => array($userID, 'Integer')); - $count = CRM_Core_DAO::singleValueQuery($sql, $params); + $count = CRM_Core_DAO::singleValueQuery($sql, $queryParams); if ($count > 0) { $_processed[$type][$userID] = 1; return; @@ -236,7 +236,7 @@ public static function cache($userID, $type = CRM_Core_Permission::VIEW, $force $tables = array(); $whereTables = array(); - $permission = CRM_ACL_API::whereClause($type, $tables, $whereTables, $userID); + $permission = CRM_ACL_API::whereClause($type, $tables, $whereTables, $userID, FALSE, FALSE, TRUE); $from = CRM_Contact_BAO_Query::fromClause($whereTables); CRM_Core_DAO::executeQuery(" @@ -247,6 +247,16 @@ public static function cache($userID, $type = CRM_Core_Permission::VIEW, $force WHERE $permission AND ac.user_id IS NULL "); + + // Add in a row for the logged in contact. Do not try to combine with the above query or an ugly OR will appear in + // the permission clause. + if (CRM_Core_Permission::check('edit my contact') || + ($type == CRM_Core_Permission::VIEW && CRM_Core_Permission::check('view my contact'))) { + if (!CRM_Core_DAO::executeQuery(" + SELECT count(*) FROM civicrm_acl_contact_cache WHERE user_id = %1 AND contact_id = %1 AND operation = '{$operation}'", $queryParams)) { + CRM_Core_DAO::executeQuery("INSERT INTO civicrm_acl_contact_cache ( user_id, contact_id, operation ) VALUES(%1, %1, '{$operation}')"); + } + } $_processed[$type][$userID] = 1; } @@ -353,7 +363,7 @@ public static function relationshipList($contact_ids) { $queries[] = " SELECT civicrm_relationship.{$contact_id_column} AS contact_id - FROM civicrm_relationship + FROM civicrm_relationship {$LEFT_JOIN_DELETED} WHERE civicrm_relationship.{$user_id_column} = {$contactID} AND civicrm_relationship.{$contact_id_column} IN ({$contact_id_list})