* 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

@@ -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;
 	}

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']->accounts->memberships($this->account, true);
-			$memberships[] = $GLOBALS['egw_info']['user']['account_id'];
+			$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;
 		}

api/src/Storage/Tracking.php

@@ -241,13 +241,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)
@@ -255,8 +262,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.
+				// leak through.  If no specific user provided, make sure we don't expose them.
-				if ($field['private']) continue;
+				if ($user === false && $field['private']) continue;
 
 				if (!$header_done)
 				{
@@ -700,6 +707,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
 		{
@@ -716,6 +724,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
 		{
@@ -740,8 +749,11 @@ abstract class Tracking
 			$date_format = $GLOBALS['egw_info']['user']['preferences']['common']['dateformat'] .
 				$GLOBALS['egw_info']['user']['preferences']['common']['timeformat'];
 
-			// Cache text body
+			// 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);
@@ -1014,7 +1026,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 ? '<br />':'') . "\n$sig";
@@ -1200,7 +1212,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)
@@ -1208,6 +1220,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)
 		{
@@ -1235,4 +1250,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;
+	}
 }

api/tests/Storage/CustomfieldsTest.php

@@ -0,0 +1,121 @@
+<?php
+/**
+ * Tests for customfields
+ *
+ * @package api
+ * @subpackage tests
+ * @link http://www.egroupware.org
+ * @author Nathan Gray
+ * @copyright 2018 Nathan Gray
+ * @license http://opensource.org/licenses/gpl-license.php GPL - GNU General Public License
+ */
+
+namespace EGroupware\Api\Storage;
+
+use EGroupware\Api;
+use EGroupware\Api\LoggedInTest as LoggedInTest;
+
+class CustomfieldsTest extends LoggedInTest
+{
+	const APP = 'test';
+	protected $customfields = null;
+
+	protected $simple_field = array(
+			'app'         => 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);
+	}
+}

api/tests/Storage/TestTracking.php

@@ -0,0 +1,32 @@
+<?php
+
+/**
+ * Concrete implementation of tracking class for testing
+ *
+ * @link http://www.egroupware.org
+ * @author Nathan Gray
+ * @package api
+ * @subpackage tests
+ * @copyright (c) 2018  Nathan Gray
+ * @license http://opensource.org/licenses/gpl-license.php GPL - GNU General Public License
+ */
+
+namespace EGroupware\Api\Storage;
+
+
+
+class TestTracking extends Tracking {
+
+	var $app = 'test';
+
+	/**
+	 * Expose protected parent method so it can be tested
+	 * @param string $message
+	 * @param string|int $receiver
+	 * @return string
+	 */
+	public function sanitize_custom_message($message, $receiver)
+	{
+		return parent::sanitize_custom_message($message, $receiver);
+	}
+}

api/tests/Storage/TrackingTest.php

@@ -0,0 +1,78 @@
+<?php
+/**
+ * Tests for Tracking
+ *
+ * @package api
+ * @subpackage tests
+ * @link http://www.egroupware.org
+ * @author Nathan Gray
+ * @copyright 2018 Nathan Gray
+ * @license http://opensource.org/licenses/gpl-license.php GPL - GNU General Public License
+ */
+
+namespace EGroupware\Api\Storage;
+
+require_once __DIR__ . '/TestTracking.php';
+
+use EGroupware\Api;
+use EGroupware\Api\LoggedInTest as LoggedInTest;
+
+class TrackingTest extends LoggedInTest
+{
+	const APP = 'test';
+
+	protected $simple_field = array(
+		'app'         => 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);
+	}
+}

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;
 	}