From 79d9c543185f34974ef9510dadc87f7be0931066 Mon Sep 17 00:00:00 2001 From: "Misha M.-Kupriyanov" Date: Tue, 3 Dec 2024 15:31:41 +0100 Subject: [PATCH 1/3] test(files): add test for UserConfig service Signed-off-by: Misha M.-Kupriyanov --- apps/files/tests/Service/UserConfigTest.php | 177 ++++++++++++++++++++ 1 file changed, 177 insertions(+) create mode 100644 apps/files/tests/Service/UserConfigTest.php diff --git a/apps/files/tests/Service/UserConfigTest.php b/apps/files/tests/Service/UserConfigTest.php new file mode 100644 index 0000000000000..18438d9c05ec6 --- /dev/null +++ b/apps/files/tests/Service/UserConfigTest.php @@ -0,0 +1,177 @@ +configMock = $this->createMock(IConfig::class); + + $this->userUID = static::getUniqueID('user_id-'); + \OC::$server->getUserManager()->createUser($this->userUID, 'test'); + \OC_User::setUserId($this->userUID); + \OC_Util::setupFS($this->userUID); + $this->userMock = $this->getUserMock($this->userUID); + + $this->userSessionMock = $this->createMock(IUserSession::class); + $this->userSessionMock->expects($this->any()) + ->method('getUser') + ->withAnyParameters() + ->willReturn($this->userMock); + + $this->userConfigService = $this->getUserConfigService(['addActivity']); + } + + /** + * @param array $methods + * @return UserConfig|\PHPUnit\Framework\MockObject\MockObject + */ + protected function getUserConfigService(array $methods = []) { + return $this->getMockBuilder(UserConfig::class) + ->setConstructorArgs([ + $this->configMock, + $this->userSessionMock, + ]) + ->setMethods($methods) + ->getMock(); + } + + /** + * @param string $uid + * @return IUser|MockObject + */ + protected function getUserMock($uid) { + $user = $this->createMock(IUser::class); + $user->expects($this->any()) + ->method('getUID') + ->willReturn($uid); + return $user; + } + protected function tearDown(): void { + \OC_User::setUserId(''); + $user = \OC::$server->getUserManager()->get($this->userUID); + if ($user !== null) { + $user->delete(); + } + } + public function testThrowsExceptionWhenNoUserLoggedInForSetConfig(): void { + $this->userSessionMock = $this->createMock(IUserSession::class); + $this->userSessionMock->expects($this->any()) + ->method('getUser') + ->withAnyParameters() + ->willReturn(null); + + $this->expectException(\Exception::class); + $this->expectExceptionMessage('No user logged in'); + + $userConfig = new UserConfig($this->configMock, $this->userSessionMock); + $userConfig->setConfig('crop_image_previews', true); + } + + public function testThrowsInvalidArgumentExceptionForUnknownConfigKey(): void { + $this->expectException(\InvalidArgumentException::class); + $this->expectExceptionMessage('Unknown config key'); + + $userConfig = new UserConfig($this->configMock, $this->userSessionMock); + $userConfig->setConfig('unknown_key', true); + } + + public function testSetsConfigSuccessfully(): void { + $this->configMock->expects($this->once()) + ->method('setUserValue') + ->with($this->userUID, Application::APP_ID, 'crop_image_previews', '1'); + $userConfig = new UserConfig($this->configMock, $this->userSessionMock); + $userConfig->setConfig('crop_image_previews', true); + } + + public function testGetsConfigsWithDefaultValuesSuccessfully(): void { + $this->userSessionMock->method('getUser')->willReturn($this->userMock); + $this->configMock->method('getUserValue') + ->willReturnCallback(function ($userId, $appId, $key, $default) { + return $default; + }); + + $userConfig = new UserConfig($this->configMock, $this->userSessionMock); + $configs = $userConfig->getConfigs(); + $this->assertEquals([ + 'crop_image_previews' => true, + 'show_hidden' => false, + 'sort_favorites_first' => true, + 'sort_folders_first' => true, + 'grid_view' => false, + 'folder_tree' => true, + ], $configs); + } + + private function getDefaultConfigValue(string $key) { + foreach (UserConfig::ALLOWED_CONFIGS as $config) { + if ($config['key'] === $key) { + return $config['default']; + } + } + return ''; + } + + public function testGetsConfigsOverrideWithUserValuesSuccessfully(): void { + $this->userSessionMock->method('getUser')->willReturn($this->userMock); + $this->configMock->method('getUserValue') + ->willReturnCallback(function ($userId, $appId, $key, $default) { + + // Override the default values + if ($key === 'crop_image_previews') { + return !$this->getDefaultConfigValue($key); + } elseif ($key === 'show_hidden') { + return !$this->getDefaultConfigValue($key); + } + + return $default; + }); + + $userConfig = new UserConfig($this->configMock, $this->userSessionMock); + $configs = $userConfig->getConfigs(); + $this->assertEquals([ + 'crop_image_previews' => false, + 'show_hidden' => true, + 'sort_favorites_first' => true, + 'sort_folders_first' => true, + 'grid_view' => false, + 'folder_tree' => true, + ], $configs); + } +} From da6c374ef3a2c3c7448462757629a7c8c6659f31 Mon Sep 17 00:00:00 2001 From: "Misha M.-Kupriyanov" Date: Tue, 3 Dec 2024 15:58:59 +0100 Subject: [PATCH 2/3] fix(files): strict check of default values Not ok: in_array("foo", [true, false]); // returns true ok: in_array("foo", [true, false], true); // returns false Signed-off-by: Misha M.-Kupriyanov --- apps/files/lib/Service/UserConfig.php | 7 ++++- apps/files/tests/Service/UserConfigTest.php | 31 +++++++++++++++++++-- 2 files changed, 34 insertions(+), 4 deletions(-) diff --git a/apps/files/lib/Service/UserConfig.php b/apps/files/lib/Service/UserConfig.php index c233996579379..850f584bc4a7e 100644 --- a/apps/files/lib/Service/UserConfig.php +++ b/apps/files/lib/Service/UserConfig.php @@ -115,7 +115,12 @@ public function setConfig(string $key, $value): void { throw new \InvalidArgumentException('Unknown config key'); } - if (!in_array($value, $this->getAllowedConfigValues($key))) { + $isBoolValue = filter_var($value, FILTER_VALIDATE_BOOLEAN, FILTER_NULL_ON_FAILURE); + if ($isBoolValue !== null) { + $value = $isBoolValue; + } + + if (!in_array($value, $this->getAllowedConfigValues($key), true)) { throw new \InvalidArgumentException('Invalid config value'); } diff --git a/apps/files/tests/Service/UserConfigTest.php b/apps/files/tests/Service/UserConfigTest.php index 18438d9c05ec6..feaa1cd622003 100644 --- a/apps/files/tests/Service/UserConfigTest.php +++ b/apps/files/tests/Service/UserConfigTest.php @@ -112,12 +112,37 @@ public function testThrowsInvalidArgumentExceptionForUnknownConfigKey(): void { $userConfig->setConfig('unknown_key', true); } - public function testSetsConfigSuccessfully(): void { + public function testThrowsInvalidArgumentExceptionForInvalidConfigValue(): void { + $this->expectException(\InvalidArgumentException::class); + $this->expectExceptionMessage('Invalid config value'); + + $userConfig = new UserConfig($this->configMock, $this->userSessionMock); + $userConfig->setConfig('crop_image_previews', 'foo'); + } + + public static function validBoolConfigValues(): array { + return [ + ['true', '1'], + ['false', '0'], + ['1', '1'], + ['0', '0'], + ['yes', '1'], + ['no', '0'], + [true, '1'], + [false, '0'], + ]; + } + + /** + * @dataProvider validBoolConfigValues + */ + public function testSetsConfigWithBooleanValuesSuccessfully($boolValue, $expectedValue): void { $this->configMock->expects($this->once()) ->method('setUserValue') - ->with($this->userUID, Application::APP_ID, 'crop_image_previews', '1'); + ->with($this->userUID, Application::APP_ID, 'crop_image_previews', $expectedValue); + $userConfig = new UserConfig($this->configMock, $this->userSessionMock); - $userConfig->setConfig('crop_image_previews', true); + $userConfig->setConfig('crop_image_previews', $boolValue); } public function testGetsConfigsWithDefaultValuesSuccessfully(): void { From 07e1a8b7b9c36fab230fe436d37db7747f229604 Mon Sep 17 00:00:00 2001 From: Kai Henseler Date: Mon, 2 Dec 2024 14:41:34 +0100 Subject: [PATCH 3/3] feat(files/user-config): make configs admin-configurable Signed-off-by: Kai Henseler --- apps/files/lib/Service/UserConfig.php | 11 ++- apps/files/tests/Service/UserConfigTest.php | 85 +++++++++++++++++++-- 2 files changed, 87 insertions(+), 9 deletions(-) diff --git a/apps/files/lib/Service/UserConfig.php b/apps/files/lib/Service/UserConfig.php index 850f584bc4a7e..a7c0e62894262 100644 --- a/apps/files/lib/Service/UserConfig.php +++ b/apps/files/lib/Service/UserConfig.php @@ -6,6 +6,7 @@ namespace OCA\Files\Service; use OCA\Files\AppInfo\Application; +use OCP\AppFramework\Services\IAppConfig; use OCP\IConfig; use OCP\IUser; use OCP\IUserSession; @@ -53,7 +54,7 @@ class UserConfig { protected IConfig $config; protected ?IUser $user = null; - public function __construct(IConfig $config, IUserSession $userSession) { + public function __construct(IConfig $config, IUserSession $userSession, protected IAppConfig $appConfig) { $this->config = $config; $this->user = $userSession->getUser(); } @@ -143,8 +144,12 @@ public function getConfigs(): array { $userId = $this->user->getUID(); $userConfigs = array_map(function (string $key) use ($userId) { - $value = $this->config->getUserValue($userId, Application::APP_ID, $key, $this->getDefaultConfigValue($key)); - // If the default is expected to be a boolean, we need to cast the value + $value = $this->config->getUserValue($userId, Application::APP_ID, $key, null); + // If the default value is expected to be a boolean, we need to cast the value + if ($value === null) { + $value = $this->appConfig->getAppValueBool($key, $this->getDefaultConfigValue($key)); + } + if (is_bool($this->getDefaultConfigValue($key)) && is_string($value)) { return $value === '1'; } diff --git a/apps/files/tests/Service/UserConfigTest.php b/apps/files/tests/Service/UserConfigTest.php index feaa1cd622003..97d91a02c5fd1 100644 --- a/apps/files/tests/Service/UserConfigTest.php +++ b/apps/files/tests/Service/UserConfigTest.php @@ -9,6 +9,7 @@ use OCA\Files\AppInfo\Application; use OCA\Files\Service\UserConfig; +use OCP\AppFramework\Services\IAppConfig; use OCP\IConfig; use OCP\IUser; use OCP\IUserSession; @@ -34,6 +35,9 @@ class UserConfigTest extends \Test\TestCase { /** @var IUserSession|\PHPUnit\Framework\MockObject\MockObject */ private $userSessionMock; + /** @var IAppConfig|\PHPUnit\Framework\MockObject\MockObject */ + private $appConfigMock; + /** * @var UserConfig|\PHPUnit\Framework\MockObject\MockObject */ @@ -42,6 +46,7 @@ class UserConfigTest extends \Test\TestCase { protected function setUp(): void { parent::setUp(); $this->configMock = $this->createMock(IConfig::class); + $this->appConfigMock = $this->createMock(IAppConfig::class); $this->userUID = static::getUniqueID('user_id-'); \OC::$server->getUserManager()->createUser($this->userUID, 'test'); @@ -67,6 +72,7 @@ protected function getUserConfigService(array $methods = []) { ->setConstructorArgs([ $this->configMock, $this->userSessionMock, + $this->appConfigMock, ]) ->setMethods($methods) ->getMock(); @@ -100,7 +106,7 @@ public function testThrowsExceptionWhenNoUserLoggedInForSetConfig(): void { $this->expectException(\Exception::class); $this->expectExceptionMessage('No user logged in'); - $userConfig = new UserConfig($this->configMock, $this->userSessionMock); + $userConfig = new UserConfig($this->configMock, $this->userSessionMock, $this->appConfigMock); $userConfig->setConfig('crop_image_previews', true); } @@ -108,7 +114,7 @@ public function testThrowsInvalidArgumentExceptionForUnknownConfigKey(): void { $this->expectException(\InvalidArgumentException::class); $this->expectExceptionMessage('Unknown config key'); - $userConfig = new UserConfig($this->configMock, $this->userSessionMock); + $userConfig = new UserConfig($this->configMock, $this->userSessionMock, $this->appConfigMock); $userConfig->setConfig('unknown_key', true); } @@ -116,7 +122,7 @@ public function testThrowsInvalidArgumentExceptionForInvalidConfigValue(): void $this->expectException(\InvalidArgumentException::class); $this->expectExceptionMessage('Invalid config value'); - $userConfig = new UserConfig($this->configMock, $this->userSessionMock); + $userConfig = new UserConfig($this->configMock, $this->userSessionMock, $this->appConfigMock); $userConfig->setConfig('crop_image_previews', 'foo'); } @@ -141,7 +147,7 @@ public function testSetsConfigWithBooleanValuesSuccessfully($boolValue, $expecte ->method('setUserValue') ->with($this->userUID, Application::APP_ID, 'crop_image_previews', $expectedValue); - $userConfig = new UserConfig($this->configMock, $this->userSessionMock); + $userConfig = new UserConfig($this->configMock, $this->userSessionMock, $this->appConfigMock); $userConfig->setConfig('crop_image_previews', $boolValue); } @@ -152,7 +158,13 @@ public function testGetsConfigsWithDefaultValuesSuccessfully(): void { return $default; }); - $userConfig = new UserConfig($this->configMock, $this->userSessionMock); + // pass the default app settings unchanged + $this->appConfigMock->method('getAppValueBool') + ->willReturnCallback(function ($key, $default) { + return $default; + }); + + $userConfig = new UserConfig($this->configMock, $this->userSessionMock, $this->appConfigMock); $configs = $userConfig->getConfigs(); $this->assertEquals([ 'crop_image_previews' => true, @@ -188,7 +200,13 @@ public function testGetsConfigsOverrideWithUserValuesSuccessfully(): void { return $default; }); - $userConfig = new UserConfig($this->configMock, $this->userSessionMock); + // pass the default app settings unchanged + $this->appConfigMock->method('getAppValueBool') + ->willReturnCallback(function ($key, $default) { + return $default; + }); + + $userConfig = new UserConfig($this->configMock, $this->userSessionMock, $this->appConfigMock); $configs = $userConfig->getConfigs(); $this->assertEquals([ 'crop_image_previews' => false, @@ -199,4 +217,59 @@ public function testGetsConfigsOverrideWithUserValuesSuccessfully(): void { 'folder_tree' => true, ], $configs); } + + public static function overrideConfigValues(): array { + // app config key, default value, user value, admin value, expected value, test description + return [ + // default value is set due to no user and no admin values + ['show_hidden', false, null, null, false, "user has no setting, admin has no setting - user gets default setting"], + + // admin value is set due to no user value + ['show_hidden', false, null, true, true, "user has no setting, admin value is set - user gets admin setting"], + ['show_hidden', false, null, false, false, "user has no setting, admin value is set - user gets admin setting"], + + // admin value is ignored in favor of user value + ['show_hidden', false, true, null, true, "user overrides default setting, admin has no setting - user gets user setting"], + ['show_hidden', false, true, true, true, "user overrides default setting, admin value is set - user gets user setting"], + ['show_hidden', false, true, false, true, "user overrides default setting, admin value is set - user gets user setting"], + + ['show_hidden', false, false, null, false, "user sets default setting, admin has no setting - user gets user setting"], + ['show_hidden', false, false, true, false, "user sets default setting, admin value is set - user gets user setting"], + ['show_hidden', false, false, false, false, "user sets default setting, admin value is set - user gets user setting"], + ]; + } + + /** + * @dataProvider overrideConfigValues + */ + public function testGetsConfigsOverrideWithAppsValuesSuccessfully(string $appConfigKey, bool $defaultValue, $userValue, $adminValue, $expectedValue, $testDescription): void { + $this->userSessionMock->method('getUser')->willReturn($this->userMock); + + // emulate override by the user config values + $this->configMock->method('getUserValue') + ->willReturnCallback(function ($userId, $appId, $key, $default) use ($appConfigKey, $userValue) { + if ($key === $appConfigKey) { + return $userValue === null ? $default : $userValue; + } + return $default; + }); + + // emulate override by the app config values + $this->appConfigMock->method('getAppValueBool') + ->willReturnCallback(function ($key, $default) use ($adminValue, $appConfigKey) { + if ($key === $appConfigKey) { + return $adminValue === null ? $default : $adminValue; + } + return $default; + }); + + $userConfig = new UserConfig($this->configMock, $this->userSessionMock, $this->appConfigMock); + $configs = $userConfig->getConfigs(); + + // ony the expected value should be different from the default + $expectedConfigs = $configs; + $expectedConfigs[$appConfigKey] = $expectedValue; + + $this->assertEquals($expectedConfigs, $configs, $testDescription); + } }