From 3709d773d9e9d666a18fb00b5311fa683e61f9d9 Mon Sep 17 00:00:00 2001 From: nathangray Date: Wed, 10 Jan 2018 10:42:28 -0700 Subject: [PATCH] * Allow private custom fields in notifications, with filtering according to set permissions. Private custom fields are removed from notifications to non-users. --- api/src/Contacts/Tracking.php | 4 +- api/src/Storage/Customfields.php | 46 +++++--- api/src/Storage/Tracking.php | 68 ++++++++++-- api/tests/Storage/CustomfieldsTest.php | 121 +++++++++++++++++++++ api/tests/Storage/TestTracking.php | 32 ++++++ api/tests/Storage/TrackingTest.php | 78 +++++++++++++ infolog/inc/class.infolog_tracking.inc.php | 3 +- 7 files changed, 321 insertions(+), 31 deletions(-) create mode 100644 api/tests/Storage/CustomfieldsTest.php create mode 100644 api/tests/Storage/TestTracking.php create mode 100644 api/tests/Storage/TrackingTest.php diff --git a/api/src/Contacts/Tracking.php b/api/src/Contacts/Tracking.php index 347a0ae190..4b36b6ad02 100644 --- a/api/src/Contacts/Tracking.php +++ b/api/src/Contacts/Tracking.php @@ -227,8 +227,6 @@ class Tracking extends Api\Storage\Tracking */ function get_details($data,$receiver=null) { - unset($receiver); // not used, but required by function signature - foreach($this->contacts->contact_fields as $name => $label) { if (!$data[$name] && $name != 'owner') continue; @@ -288,7 +286,7 @@ class Tracking extends Api\Storage\Tracking } } // add custom fields for given type - $details += $this->get_customfields($data, $data['tid']); + $details += $this->get_customfields($data, $data['tid'], $receiver); return $details; } diff --git a/api/src/Storage/Customfields.php b/api/src/Storage/Customfields.php index 4690833b0e..ee96dcb700 100755 --- a/api/src/Storage/Customfields.php +++ b/api/src/Storage/Customfields.php @@ -38,11 +38,11 @@ class Customfields implements \IteratorAggregate protected $app; /** - * should all the private fields be returned too, default no + * User account to filter custom field private * - * @var boolean + * @var int */ - protected $all_private_too=false; + protected $account=false; /** * Iterator initialised for custom fields @@ -55,25 +55,29 @@ class Customfields implements \IteratorAggregate * Constructor * * @param string $app - * @param boolean $all_private_too =false should all the private fields be returned too, default no + * @param int|boolean $account =false Filter private for given account_id, + * false for current user or true for all the private fields be returned too, default current user * @param string $only_type2 =null if given only return fields of type2 == $only_type2 * @param int $start =0 * @param int $num_rows =null * @param Api\Db $db =null reference to database instance to use * @return array with customfields */ - function __construct($app, $all_private_too=false, $only_type2=null, $start=0, $num_rows=null, Api\Db $db=null) + function __construct($app, $account=false, $only_type2=null, $start=0, $num_rows=null, Api\Db $db=null) { $this->app = $app; - $this->all_private_too = $all_private_too; + + // If $account is true, no filtering otherwise use current user + $this->account = $account === true ? false : + is_numeric($account) ? (int)$account : $GLOBALS['egw_info']['user']['account_id']; $query = array( 'cf_app' => $app, ); - if (!$all_private_too) + if ($this->account) { - $memberships = $GLOBALS['egw']->accounts->memberships($GLOBALS['egw_info']['user']['account_id'], true); - $memberships[] = $GLOBALS['egw_info']['user']['account_id']; + $memberships = $GLOBALS['egw']->accounts->memberships($this->account, true); + $memberships[] = $this->account; $query[] = $this->commasep_match('cf_private', $memberships); } if ($only_type2) @@ -127,19 +131,24 @@ class Customfields implements \IteratorAggregate * Get customfield array of an application * * @param string $app - * @param boolean $all_private_too =false should all the private fields be returned too, default no + * @param int|boolean $account =false Filter private for given account_id, + * false for current user or true for all the private fields be returned too, default current user * @param string $only_type2 =null if given only return fields of type2 == $only_type2 * @param Api\Db $db =null reference to database to use * @return array with customfields */ - public static function get($app, $all_private_too=false, $only_type2=null, Api\Db $db=null) + public static function get($app, $account=false, $only_type2=null, Api\Db $db=null) { - $cache_key = $app.':'.($all_private_too?'all':$GLOBALS['egw_info']['user']['account_id']).':'.$only_type2; + $account = $account === true ? 'all' : + $account === false ? $GLOBALS['egw_info']['user']['account_id'] : + (int)$account; + + $cache_key = $app.':'.$account.':'.$only_type2; $cfs = Api\Cache::getInstance(__CLASS__, $cache_key); if (!isset($cfs)) { - $cfs = iterator_to_array(new Customfields($app, $all_private_too, $only_type2, 0, null, $db)); + $cfs = iterator_to_array(new Customfields($app, $account, $only_type2, 0, null, $db)); Api\Cache::setInstance(__CLASS__, $cache_key, $cfs); $cached = Api\Cache::getInstance(__CLASS__, $app); @@ -149,7 +158,7 @@ class Customfields implements \IteratorAggregate Api\Cache::setInstance(__CLASS__, $app, $cached); } } - //error_log(__METHOD__."('$app', $all_private_too, '$only_type2') returning fields: ".implode(', ', array_keys($cfs))); + //error_log(__METHOD__."('$app', $account, '$only_type2') returning fields: ".implode(', ', array_keys($cfs))); return $cfs; } @@ -157,13 +166,14 @@ class Customfields implements \IteratorAggregate * Check if any customfield uses html (type == 'htmlarea') * * @param string $app - * @param boolean $all_private_too =false should all the private fields be returned too, default no + * @param int|boolean $account =false Filter private for given account_id, + * false for current user or true for all the private fields be returned too, default current user * @param string $only_type2 =null if given only return fields of type2 == $only_type2 * @return boolen true: if there is a custom field useing html, false if not */ - public static function use_html($app, $all_private_too=false, $only_type2=null) + public static function use_html($app, $account=false, $only_type2=null) { - foreach(self::get($app, $all_private_too, $only_type2) as $data) + foreach(self::get($app, $account, $only_type2) as $data) { if ($data['type'] == 'htmlarea') return true; } @@ -544,7 +554,7 @@ class Customfields implements \IteratorAggregate self::$db = $GLOBALS['egw_setup']->db; } } - + /** * Handle any uploaded files that weren't dealt with immediately when uploaded. * This usually happens for new entries, where we don't have the entry's ID diff --git a/api/src/Storage/Tracking.php b/api/src/Storage/Tracking.php index 8c53d3c3ad..f09dfcdf59 100644 --- a/api/src/Storage/Tracking.php +++ b/api/src/Storage/Tracking.php @@ -242,13 +242,20 @@ abstract class Tracking * * @param array|object $data * @param string $only_type2 = null if given only return fields of type2 == $only_type2 + * @param int $user = false Use this user for custom field permissions, or false + * to strip all private custom fields + * * @return array of details as array with values for keys 'label','value','type' */ - function get_customfields($data, $only_type2=null) + function get_customfields($data, $only_type2=null, $user = false) { $details = array(); + if(!is_numeric($user)) + { + $user = false; + } - if (($cfs = Customfields::get($this->app, $all_private_too=false, $only_type2))) + if (($cfs = Customfields::get($this->app, $user, $only_type2))) { $header_done = false; foreach($cfs as $name => $field) @@ -256,8 +263,8 @@ abstract class Tracking if (in_array($field['type'], Customfields::$non_printable_fields)) continue; // Sometimes cached customfields let private fields the user can access - // leak through. Make sure we don't expose them. - if ($field['private']) continue; + // leak through. If no specific user provided, make sure we don't expose them. + if ($user === false && $field['private']) continue; if (!$header_done) { @@ -701,6 +708,7 @@ abstract class Tracking $save_user = $GLOBALS['egw_info']['user']; $do_notify = true; + $can_cache = false; if (is_numeric($user_or_lang)) // user --> read everything from his prefs { @@ -717,6 +725,7 @@ abstract class Tracking { $do_notify = false; // no notification requested / necessary } + $can_cache = (Customfields::get($this->app, true) == Customfields::get($this->app, $user_or_lang)); } else { @@ -741,8 +750,11 @@ abstract class Tracking $date_format = $GLOBALS['egw_info']['user']['preferences']['common']['dateformat'] . $GLOBALS['egw_info']['user']['preferences']['common']['timeformat']; - // Cache text body - $body_cache =& $this->body_cache[$data[$this->id_field]][$lang][$date_format]; + // Cache text body, if there's no private custom fields we might reveal + if($can_cache) + { + $body_cache =& $this->body_cache[$data[$this->id_field]][$lang][$date_format]; + } if(empty($data[$this->id_field]) || !isset($body_cache['text'])) { $body_cache['text'] = $this->get_body(false,$data,$old,false,$receiver); @@ -1015,7 +1027,7 @@ abstract class Tracking $body = ''; if($this->get_config(self::CUSTOM_NOTIFICATION, $data, $old)) { - $body = $this->get_custom_message($data,$old); + $body = $this->get_custom_message($data,$old,null,$receiver); if(($sig = $this->get_signature($data,$old,$receiver))) { $body .= ($html_email ? '
':'') . "\n$sig"; @@ -1201,7 +1213,7 @@ abstract class Tracking * Get a custom notification message to be used instead of the standard one. * It can use merge print placeholders to include data. */ - protected function get_custom_message($data, $old, $merge_class = null) + protected function get_custom_message($data, $old, $merge_class = null, $receiver = false) { $message = $this->get_config(self::CUSTOM_NOTIFICATION, $data, $old); if(!$message) @@ -1209,6 +1221,9 @@ abstract class Tracking return ''; } + // Check if there's any custom field privacy issues, and try to remove them + $message = $this->sanitize_custom_message($message, $receiver); + // Automatically set merge class from naming conventions if($merge_class == null) { @@ -1236,4 +1251,41 @@ abstract class Tracking throw new Api\Exception\WrongParameter("Invalid merge class '$merge_class' for {$this->app} custom notification"); } } + + /** + * Check to see if the message would expose any custom fields that are + * not visible to the receiver, and try to remove them from the message. + * + * @param string $message + * @param string|int $receiver Account ID or email address + */ + protected function sanitize_custom_message($message, $receiver) + { + if(!is_numeric($receiver)) + { + $receiver = false; + } + + $cfs = Customfields::get($this->app, $receiver); + $all_cfs = Customfields::get($this->app, true); + + // If we have a specific user and they're the same then there are + // no private fields so nothing needs to be done + if($receiver && $all_cfs == $cfs) + { + return $message; + } + + // Replace any placeholders that use the private field, or any sub-keys + // of the field + foreach($all_cfs as $name => $field) + { + if ($receiver === false && $field['private'] || !$cfs[$name]) + { + // {{field}} or {{field/subfield}} or $$field$$ or $$field/subfield$$ + $message = preg_replace('/(\{\{|\$\$)#'.$name.'(\/[^\}\$]+)?(\}\}|\$\$)/', '', $message); + } + } + return $message; + } } diff --git a/api/tests/Storage/CustomfieldsTest.php b/api/tests/Storage/CustomfieldsTest.php new file mode 100644 index 0000000000..11449b9217 --- /dev/null +++ b/api/tests/Storage/CustomfieldsTest.php @@ -0,0 +1,121 @@ + self::APP, + 'name' => 'test_field', + 'label' => 'Custom field', + 'type' => 'text', + 'type2' => array(), + 'help' => 'Custom field created for automated testing by CustomfieldsTest', + 'values' => null, + 'len' => null, + 'rows' => null, + 'order' => null, + 'needed' => null, + 'private' => array() + ); + + public function assertPreConditions() + { + parent::assertPreConditions(); + $tables = $GLOBALS['egw']->db->table_names(true); + $this->assertContains('egw_test', $tables, 'Could not find DB table "egw_test", make sure test app is installed'); + } + + /** + * Check to make sure we can create a custom field + */ + public function testCreateField() + { + // Create + $field = $this->simple_field; + + Customfields::update($field); + + // Check + $fields = Customfields::get(self::APP); + + $this->assertArrayHasKey($field['name'], $fields); + + $saved_field = $fields[$field['name']]; + + foreach(array('app','label','type','type2','help','values','len','rows','needed','private') as $key) + { + $this->assertEquals($field[$key], $saved_field[$key], "Load of $key did not match save"); + } + + // Clean + unset($fields[$field['name']]); + Customfields::save(self::APP, $fields); + } + + /** + * Test the access control on private custom fields + */ + public function testPrivate() + { + // Create field + $field = array_merge( + $this->simple_field, + array( + 'private' => array($GLOBALS['egw_info']['user']['account_id']) + ) + ); + + Customfields::update($field); + $fields = Customfields::get(self::APP); + + // Get another user + $accounts = $GLOBALS['egw']->accounts->search(array( + 'type' => 'accounts' + )); + unset($accounts[$GLOBALS['egw_info']['user']['account_id']]); + if(count($accounts) == 0) + { + $this->markTestSkipped('Need more than one user to check private'); + } + $other_account = key($accounts); + + // Try to read - should not be there + $fields = Customfields::get(self::APP,$other_account); + $this->assertArrayNotHasKey($field['name'], $fields); + + // Give access & check again + $field['private'][] = $other_account; + Customfields::update($field); + + $fields = Customfields::get(self::APP,$other_account); + $this->assertArrayHasKey($field['name'], $fields); + + // Remove access, check its gone + $field['private'] = array($GLOBALS['egw_info']['user']['account_id']); + Customfields::update($field); + + $fields = Customfields::get(self::APP,$other_account); + $this->assertArrayNotHasKey($field['name'], $fields); + + // Clean up + unset($fields[$field['name']]); + Customfields::save(self::APP, $fields); + } +} diff --git a/api/tests/Storage/TestTracking.php b/api/tests/Storage/TestTracking.php new file mode 100644 index 0000000000..ec40f4f36c --- /dev/null +++ b/api/tests/Storage/TestTracking.php @@ -0,0 +1,32 @@ + self::APP, + 'name' => 'test_field', + 'label' => 'Custom field', + 'type' => 'text', + 'type2' => array(), + 'help' => 'Custom field created for automated testing by CustomfieldsTest', + 'values' => null, + 'len' => null, + 'rows' => null, + 'order' => null, + 'needed' => null, + 'private' => array() + ); + + /** + * Test the access control on private custom fields + */ + public function testSanitizeCustomMessage() + { + // Create field + $field = array_merge( + $this->simple_field, + array( + 'private' => array($GLOBALS['egw_info']['user']['account_id']) + ) + ); + Customfields::update($field); + + $fields = Customfields::get(self::APP); + + $dirty_message = "This custom message contains {{#test_field}}, which only user {$GLOBALS['egw_info']['user']['account_id']} can access."; + $clean_message = "This custom message contains , which only user {$GLOBALS['egw_info']['user']['account_id']} can access."; + + // Get another user + $accounts = $GLOBALS['egw']->accounts->search(array( + 'type' => 'accounts' + )); + unset($accounts[$GLOBALS['egw_info']['user']['account_id']]); + if(count($accounts) == 0) + { + $this->markTestSkipped('Need more than one user to check private'); + } + $other_account = key($accounts); + + $tracking = new TestTracking(self::APP); + $cleaned = $tracking->sanitize_custom_message($dirty_message, $other_account); + + $this->assertEquals($clean_message, $cleaned); + + // Clean up + unset($fields[$field['name']]); + Customfields::save(self::APP, $fields); + } +} diff --git a/infolog/inc/class.infolog_tracking.inc.php b/infolog/inc/class.infolog_tracking.inc.php index fe5be3e5e0..9315ea1091 100644 --- a/infolog/inc/class.infolog_tracking.inc.php +++ b/infolog/inc/class.infolog_tracking.inc.php @@ -199,7 +199,6 @@ class infolog_tracking extends Api\Storage\Tracking */ function get_details($data,$receiver=null) { - unset($receiver); // not used, but required function signature //error_log(__METHOD__.__LINE__.' Data:'.array2string($data)); $responsible = array(); if ($data['info_responsible']) @@ -257,7 +256,7 @@ class infolog_tracking extends Api\Storage\Tracking 'type' => 'multiline', ); // add custom fields for given type - $details += $this->get_customfields($data, $data['info_type']); + $details += $this->get_customfields($data, $data['info_type'], $receiver); return $details; }