From f402561d7d2a8ffd80f4878eb7b04eaed539a2d2 Mon Sep 17 00:00:00 2001 From: nathangray Date: Fri, 12 Jan 2018 10:03:27 -0700 Subject: [PATCH] Better tests for Customfields, fix a couple of permission bugs revealed Found a problems when requesting 'all' custom fields, it would still use current user permissions --- api/src/Storage/Customfields.php | 10 +- api/tests/Storage/CustomfieldsTest.php | 139 ++++++++++++++++++++----- 2 files changed, 119 insertions(+), 30 deletions(-) diff --git a/api/src/Storage/Customfields.php b/api/src/Storage/Customfields.php index ee96dcb700..8b61cd199b 100755 --- a/api/src/Storage/Customfields.php +++ b/api/src/Storage/Customfields.php @@ -69,7 +69,7 @@ class Customfields implements \IteratorAggregate // 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']; + (is_numeric($account) ? (int)$account : $GLOBALS['egw_info']['user']['account_id']); $query = array( 'cf_app' => $app, @@ -139,11 +139,11 @@ class Customfields implements \IteratorAggregate */ public static function get($app, $account=false, $only_type2=null, Api\Db $db=null) { - $account = $account === true ? 'all' : - $account === false ? $GLOBALS['egw_info']['user']['account_id'] : - (int)$account; + $account_key = $account === true ? 'all' : + ($account === false ? $GLOBALS['egw_info']['user']['account_id'] : + (int)$account); - $cache_key = $app.':'.$account.':'.$only_type2; + $cache_key = $app.':'.$account_key.':'.$only_type2; $cfs = Api\Cache::getInstance(__CLASS__, $cache_key); if (!isset($cfs)) diff --git a/api/tests/Storage/CustomfieldsTest.php b/api/tests/Storage/CustomfieldsTest.php index 11449b9217..51dc6fcb81 100644 --- a/api/tests/Storage/CustomfieldsTest.php +++ b/api/tests/Storage/CustomfieldsTest.php @@ -72,7 +72,107 @@ class CustomfieldsTest extends LoggedInTest /** * Test the access control on private custom fields */ - public function testPrivate() + public function testPrivateCannotBeReadWithoutPermission() + { + $field = $this->create_private_field(); + + // Get another user + $other_account = $this->get_another_user(); + + // Try to read - should not be there + $fields = Customfields::get(self::APP,$other_account); + $this->assertArrayNotHasKey($field['name'], $fields); + + // Switch the users + $field['private'] = array($other_account); + Customfields::update($field); + + // Try to read - should not be there + $fields = Customfields::get(self::APP,false); + $this->assertArrayNotHasKey($field['name'], $fields); + + // Clean up + unset($fields[$field['name']]); + Customfields::save(self::APP, $fields); + } + + /** + * Test that giving access allows access + */ + public function testGivingAccess() + { + $field = $this->create_private_field(); + + $fields = Customfields::get(self::APP); + + // Get another user + $other_account = $this->get_another_user(); + + // Give access & check + $field['private'][] = $other_account; + Customfields::update($field); + + $fields = Customfields::get(self::APP,$other_account); + $this->assertArrayHasKey($field['name'], $fields); + + // Clean up + unset($fields[$field['name']]); + Customfields::save(self::APP, $fields); + } + + /** + * Test that removing access disallows access + */ + public function testRemovingAccess() + { + $field = $this->create_private_field(); + + $fields = Customfields::get(self::APP); + + // Get another user + $other_account = $this->get_another_user(); + + // Give access + $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); + } + + /** + * Test getting all fields ignores any access restrictions + */ + public function testGetAllFields() + { + $field = $this->create_private_field(); + + // Get another user + $other_account = $this->get_another_user(); + + // Change access so current user can't read it + $field['private'] = array($other_account); + Customfields::update($field); + + $fields = Customfields::get(self::APP,true); + $this->assertEquals(1, count($fields)); + $this->assertArrayHasKey($field['name'], $fields); + + // Clean up + unset($fields[$field['name']]); + Customfields::save(self::APP, $fields); + } + + protected function create_private_field() { // Create field $field = array_merge( @@ -81,11 +181,16 @@ class CustomfieldsTest extends LoggedInTest 'private' => array($GLOBALS['egw_info']['user']['account_id']) ) ); - Customfields::update($field); - $fields = Customfields::get(self::APP); - // Get another user + return $field; + } + + /** + * Get another user that we can use to test + */ + protected function get_another_user() + { $accounts = $GLOBALS['egw']->accounts->search(array( 'type' => 'accounts' )); @@ -96,26 +201,10 @@ class CustomfieldsTest extends LoggedInTest } $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); + if(!$other_account) + { + $this->markTestSkipped('Need more than one user to check private'); + } + return $other_account; } }