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
This commit is contained in:
nathangray 2018-01-12 10:03:27 -07:00
parent 09f9bce5b4
commit f402561d7d
2 changed files with 119 additions and 30 deletions

View File

@ -69,7 +69,7 @@ class Customfields implements \IteratorAggregate
// If $account is true, no filtering otherwise use current user // If $account is true, no filtering otherwise use current user
$this->account = $account === true ? false : $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( $query = array(
'cf_app' => $app, '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) public static function get($app, $account=false, $only_type2=null, Api\Db $db=null)
{ {
$account = $account === true ? 'all' : $account_key = $account === true ? 'all' :
$account === false ? $GLOBALS['egw_info']['user']['account_id'] : ($account === false ? $GLOBALS['egw_info']['user']['account_id'] :
(int)$account; (int)$account);
$cache_key = $app.':'.$account.':'.$only_type2; $cache_key = $app.':'.$account_key.':'.$only_type2;
$cfs = Api\Cache::getInstance(__CLASS__, $cache_key); $cfs = Api\Cache::getInstance(__CLASS__, $cache_key);
if (!isset($cfs)) if (!isset($cfs))

View File

@ -72,7 +72,107 @@ class CustomfieldsTest extends LoggedInTest
/** /**
* Test the access control on private custom fields * 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 // Create field
$field = array_merge( $field = array_merge(
@ -81,11 +181,16 @@ class CustomfieldsTest extends LoggedInTest
'private' => array($GLOBALS['egw_info']['user']['account_id']) 'private' => array($GLOBALS['egw_info']['user']['account_id'])
) )
); );
Customfields::update($field); 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( $accounts = $GLOBALS['egw']->accounts->search(array(
'type' => 'accounts' 'type' => 'accounts'
)); ));
@ -96,26 +201,10 @@ class CustomfieldsTest extends LoggedInTest
} }
$other_account = key($accounts); $other_account = key($accounts);
// Try to read - should not be there if(!$other_account)
$fields = Customfields::get(self::APP,$other_account); {
$this->assertArrayNotHasKey($field['name'], $fields); $this->markTestSkipped('Need more than one user to check private');
}
// Give access & check again return $other_account;
$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);
} }
} }