diff --git a/lib/AppInfo/Application.php b/lib/AppInfo/Application.php index 9303ee9..aae943d 100644 --- a/lib/AppInfo/Application.php +++ b/lib/AppInfo/Application.php @@ -12,6 +12,7 @@ use OCP\AppFramework\Bootstrap\IBootContext; use OCP\AppFramework\Bootstrap\IBootstrap; use OCP\AppFramework\Bootstrap\IRegistrationContext; +use OCP\AppFramework\Services\IAppConfig; use OCP\AppFramework\Services\IInitialState; use OCP\IConfig; use OCP\Security\CSP\AddContentSecurityPolicyEvent; @@ -43,16 +44,17 @@ public function register(IRegistrationContext $context): void { public function boot(IBootContext $context): void { $context->injectFn(function ( IInitialState $initialState, + IAppConfig $appConfig, IConfig $config, ?string $userId, ) { - $adminBaseUrl = $config->getAppValue(Application::APP_ID, 'base_url', Application::DEFAULT_BASE_URL) ?: Application::DEFAULT_BASE_URL; + $adminBaseUrl = $appConfig->getAppValueString('base_url', Application::DEFAULT_BASE_URL) ?: Application::DEFAULT_BASE_URL; if ($userId === null) { $baseUrl = $adminBaseUrl; } else { $baseUrl = $config->getUserValue($userId, Application::APP_ID, 'base_url', $adminBaseUrl) ?: $adminBaseUrl; } - $overrideClick = $config->getAppValue(Application::APP_ID, 'override_link_click', '0') === '1'; + $overrideClick = $appConfig->getAppValueString('override_link_click', '0') === '1'; $initialState->provideInitialState('base_url', $baseUrl); // TODO add setting to set this value diff --git a/lib/Controller/ConfigController.php b/lib/Controller/ConfigController.php index 5235dfe..5f60ecc 100755 --- a/lib/Controller/ConfigController.php +++ b/lib/Controller/ConfigController.php @@ -13,6 +13,7 @@ use OCP\AppFramework\Http; use OCP\AppFramework\Http\Attribute\PasswordConfirmationRequired; use OCP\AppFramework\Http\DataResponse; +use OCP\AppFramework\Services\IAppConfig; use OCP\IConfig; use OCP\IRequest; use OCP\Security\ICrypto; @@ -22,6 +23,7 @@ class ConfigController extends Controller { public function __construct( string $appName, IRequest $request, + private IAppConfig $appConfig, private IConfig $config, private NuiteqAPIService $nuiteqAPIService, private ICrypto $crypto, @@ -63,6 +65,17 @@ public function setConfig(array $values): DataResponse { */ #[PasswordConfirmationRequired] public function setAdminConfig(array $values): DataResponse { + // only what the admin settings send: a typed write of any other key + // would change the type of rows Nextcloud owns, installed_version or + // enabled, and the next upgrade or enable fails on the conflict + foreach ($values as $key => $value) { + if (!in_array($key, ['base_url', 'client_key'], true)) { + return new DataResponse(['error' => 'Unknown admin setting ' . $key], Http::STATUS_BAD_REQUEST); + } + if (!is_string($value)) { + return new DataResponse(['error' => 'The admin setting ' . $key . ' must be a string'], Http::STATUS_BAD_REQUEST); + } + } foreach ($values as $key => $value) { if (in_array($key, ['client_key']) && $value !== '') { if ($value === 'dummySecret') { @@ -71,7 +84,9 @@ public function setAdminConfig(array $values): DataResponse { } $value = $this->crypto->encrypt($value); } - $this->config->setAppValue(Application::APP_ID, $key, $value); + // the client key is read only by this app's own settings and requests, + // base_url on every page + $this->appConfig->setAppValueString($key, $value, lazy: $key === 'client_key'); } return new DataResponse(''); } diff --git a/lib/Listener/AddContentSecurityPolicyListener.php b/lib/Listener/AddContentSecurityPolicyListener.php index 28e9a26..4a91811 100644 --- a/lib/Listener/AddContentSecurityPolicyListener.php +++ b/lib/Listener/AddContentSecurityPolicyListener.php @@ -9,6 +9,7 @@ use OCA\Nuiteq\AppInfo\Application; use OCP\AppFramework\Http\ContentSecurityPolicy; +use OCP\AppFramework\Services\IAppConfig; use OCP\EventDispatcher\Event; use OCP\EventDispatcher\IEventListener; use OCP\IConfig; @@ -28,8 +29,11 @@ class AddContentSecurityPolicyListener implements IEventListener { */ private $userId; - public function __construct(IConfig $config, - ?string $userId) { + public function __construct( + IConfig $config, + private IAppConfig $appConfig, + ?string $userId, + ) { $this->config = $config; $this->userId = $userId; } @@ -39,7 +43,7 @@ public function handle(Event $event): void { return; } - $adminBaseUrl = $this->config->getAppValue(Application::APP_ID, 'base_url', Application::DEFAULT_BASE_URL) ?: Application::DEFAULT_BASE_URL; + $adminBaseUrl = $this->appConfig->getAppValueString('base_url', Application::DEFAULT_BASE_URL) ?: Application::DEFAULT_BASE_URL; if ($this->userId === null) { $baseUrl = $adminBaseUrl; } else { diff --git a/lib/Migration/Version1200Date202410151354.php b/lib/Migration/Version1200Date202410151354.php index 56a80fd..e53301f 100644 --- a/lib/Migration/Version1200Date202410151354.php +++ b/lib/Migration/Version1200Date202410151354.php @@ -13,7 +13,7 @@ use OCA\Nuiteq\AppInfo\Application; use OCP\DB\ISchemaWrapper; use OCP\DB\QueryBuilder\IQueryBuilder; -use OCP\IConfig; +use OCP\IAppConfig; use OCP\IDBConnection; use OCP\Migration\IOutput; use OCP\Migration\SimpleMigrationStep; @@ -24,7 +24,7 @@ class Version1200Date202410151354 extends SimpleMigrationStep { public function __construct( private IDBConnection $connection, private ICrypto $crypto, - private IConfig $config, + private IAppConfig $appConfig, ) { } @@ -37,10 +37,10 @@ public function __construct( */ public function postSchemaChange(IOutput $output, Closure $schemaClosure, array $options) { // encrypt "client_key" in the appconfig - $clientSecret = $this->config->getAppValue(Application::APP_ID, 'client_key'); + $clientSecret = $this->appConfig->getValueString(Application::APP_ID, 'client_key', lazy: true); if ($clientSecret !== '') { $clientSecret = $this->crypto->encrypt($clientSecret); - $this->config->setAppValue(Application::APP_ID, 'client_key', $clientSecret); + $this->appConfig->setValueString(Application::APP_ID, 'client_key', $clientSecret, lazy: true); } // encrypt user's "client_key" diff --git a/lib/Migration/Version1301Date202610211026.php b/lib/Migration/Version1301Date202610211026.php new file mode 100644 index 0000000..6064221 --- /dev/null +++ b/lib/Migration/Version1301Date202610211026.php @@ -0,0 +1,41 @@ +appConfig->updateLazy(Application::APP_ID, 'client_key', true); + + return null; + } +} diff --git a/lib/Service/NuiteqAPIService.php b/lib/Service/NuiteqAPIService.php index 39b9192..f912e43 100755 --- a/lib/Service/NuiteqAPIService.php +++ b/lib/Service/NuiteqAPIService.php @@ -11,6 +11,7 @@ use GuzzleHttp\Exception\ClientException; use GuzzleHttp\Exception\ServerException; use OCA\Nuiteq\AppInfo\Application; +use OCP\AppFramework\Services\IAppConfig; use OCP\Http\Client\IClientService; use OCP\IConfig; use OCP\IL10N; @@ -29,6 +30,7 @@ public function __construct( private string $appName, private LoggerInterface $logger, private IL10N $l10n, + private IAppConfig $appConfig, private IConfig $config, private ICrypto $crypto, IClientService $clientService, @@ -41,7 +43,7 @@ public function __construct( * @return string */ private function getClientKey(string $userId): string { - $adminClientKey = $this->config->getAppValue(Application::APP_ID, 'client_key', Application::DEFAULT_CLIENT_KEY) ?: Application::DEFAULT_CLIENT_KEY; + $adminClientKey = $this->appConfig->getAppValueString('client_key', Application::DEFAULT_CLIENT_KEY, lazy: true) ?: Application::DEFAULT_CLIENT_KEY; $clientKey = $this->config->getUserValue($userId, Application::APP_ID, 'client_key', $adminClientKey) ?: $adminClientKey; // Decrypt the clientKey if it's encrypted @@ -73,7 +75,7 @@ private function getClientKey(string $userId): string { * @return string */ public function getBaseUrl(string $userId): string { - $adminBaseUrl = $this->config->getAppValue(Application::APP_ID, 'base_url', Application::DEFAULT_BASE_URL) ?: Application::DEFAULT_BASE_URL; + $adminBaseUrl = $this->appConfig->getAppValueString('base_url', Application::DEFAULT_BASE_URL) ?: Application::DEFAULT_BASE_URL; return $this->config->getUserValue($userId, Application::APP_ID, 'base_url', $adminBaseUrl) ?: $adminBaseUrl; } diff --git a/lib/Settings/Admin.php b/lib/Settings/Admin.php index 3a5f8b9..3f6f95d 100755 --- a/lib/Settings/Admin.php +++ b/lib/Settings/Admin.php @@ -9,14 +9,14 @@ use OCA\Nuiteq\AppInfo\Application; use OCP\AppFramework\Http\TemplateResponse; +use OCP\AppFramework\Services\IAppConfig; use OCP\AppFramework\Services\IInitialState; -use OCP\IConfig; use OCP\Settings\ISettings; class Admin implements ISettings { public function __construct( - private IConfig $config, + private IAppConfig $appConfig, private IInitialState $initialStateService, ) { } @@ -25,8 +25,8 @@ public function __construct( * @return TemplateResponse */ public function getForm(): TemplateResponse { - $baseUrl = $this->config->getAppValue(Application::APP_ID, 'base_url', 'https://nuiteqstage.se'); - $clientKey = $this->config->getAppValue(Application::APP_ID, 'client_key'); + $baseUrl = $this->appConfig->getAppValueString('base_url', 'https://nuiteqstage.se'); + $clientKey = $this->appConfig->getAppValueString('client_key', lazy: true); $adminConfig = [ 'base_url' => $baseUrl, diff --git a/lib/Settings/Personal.php b/lib/Settings/Personal.php index 6ed9957..cdaec15 100755 --- a/lib/Settings/Personal.php +++ b/lib/Settings/Personal.php @@ -10,6 +10,7 @@ use Exception; use OCA\Nuiteq\AppInfo\Application; use OCP\AppFramework\Http\TemplateResponse; +use OCP\AppFramework\Services\IAppConfig; use OCP\AppFramework\Services\IInitialState; use OCP\IConfig; use OCP\Security\ICrypto; @@ -20,6 +21,7 @@ class Personal implements ISettings { public function __construct( + private IAppConfig $appConfig, private IConfig $config, private IInitialState $initialStateService, private ICrypto $crypto, @@ -32,7 +34,7 @@ public function __construct( * @return TemplateResponse */ public function getForm(): TemplateResponse { - $adminBaseUrl = $this->config->getAppValue(Application::APP_ID, 'base_url', 'https://nuiteqstage.se') ?: 'https://nuiteqstage.se'; + $adminBaseUrl = $this->appConfig->getAppValueString('base_url', 'https://nuiteqstage.se') ?: 'https://nuiteqstage.se'; $baseUrl = $this->config->getUserValue($this->userId, Application::APP_ID, 'base_url', $adminBaseUrl) ?: $adminBaseUrl; $apiKey = $this->config->getUserValue($this->userId, Application::APP_ID, 'api_key'); diff --git a/tests/unit/AppInfo/ApplicationTest.php b/tests/unit/AppInfo/ApplicationTest.php new file mode 100644 index 0000000..f0c6d06 --- /dev/null +++ b/tests/unit/AppInfo/ApplicationTest.php @@ -0,0 +1,153 @@ + the admin values asked for, and whether lazily */ + private array $reads = []; + + /** + * Boots the app the way Nextcloud does, and returns what it hands every + * page. + * + * @param array $adminValues the stored admin values; a + * missing one reads as the default + * @param array $userValues the stored values of the user + */ + private function boot(array $adminValues, ?string $userId, array $userValues = []): array { + $appConfig = $this->createMock(IAppConfig::class); + $appConfig->method('getAppValueString')->willReturnCallback( + function (string $key, string $default = '', bool $lazy = false) use ($adminValues): string { + $this->reads[] = [$key, $lazy]; + return $adminValues[$key] ?? $default; + } + ); + $config = $this->createMock(IConfig::class); + $config->method('getUserValue')->willReturnCallback( + fn (string $user, string $app, string $key, string $default = '') => $userValues[$key] ?? $default + ); + // the old API answers a lazy value with the default, so a read through + // it must not pass for the right one + foreach (['getAppValue', 'setAppValue', 'deleteAppValue'] as $method) { + $config->expects($this->never())->method($method); + } + $state = []; + $initialState = $this->createMock(IInitialState::class); + $initialState->method('provideInitialState')->willReturnCallback( + function (string $key, $value) use (&$state): void { + $state[$key] = $value; + } + ); + $services = [ + IAppConfig::class => $appConfig, + IConfig::class => $config, + IInitialState::class => $initialState, + ]; + $context = $this->createMock(IBootContext::class); + $context->expects($this->once()) + ->method('injectFn') + ->willReturnCallback(fn (callable $fn) => $this->inject($fn, $services, $userId)); + + // the constructor asks a running server for its debug setting + $application = (new ReflectionClass(Application::class))->newInstanceWithoutConstructor(); + $application->boot($context); + + return $state; + } + + /** + * Calls the function as Nextcloud's injector does, with a service for + * each class it asks for and the user id for the rest. + */ + private function inject(callable $fn, array $services, ?string $userId): void { + $arguments = []; + foreach ((new ReflectionFunction(Closure::fromCallable($fn)))->getParameters() as $parameter) { + $type = $parameter->getType(); + $arguments[] = $type instanceof ReflectionNamedType && !$type->isBuiltin() + ? $services[$type->getName()] + : $userId; + } + try { + $fn(...$arguments); + } catch (Error $e) { + // the script it adds last looks up the user's language with a + // running server, which a clone on its own does not have; the + // values it hands the page come before that + if (!str_contains($e->getMessage(), 'Class "OC" not found')) { + throw $e; + } + } + } + + /** + * boot() runs on every request, so what it reads has to be among the + * values Nextcloud loads up front: a lazy read there would load every lazy + * value of every app with each page. + */ + public function testBootReadsTheAdminValuesNonLazily(): void { + $this->boot([], 'jdoe'); + + $this->assertSame([['base_url', false], ['override_link_click', false]], $this->reads); + } + + /** + * override_link_click has no setting, so on nearly every install it is + * not set, and its default decides how a click on a board link behaves. + */ + #[DataProvider('provideUsers')] + public function testAnInstallWithNothingSetGetsTheShippedStageUrlAndNoOverride(?string $userId): void { + $state = $this->boot([], $userId); + + $this->assertSame(Application::DEFAULT_BASE_URL, $state['base_url'] ?? null); + $this->assertFalse($state['override_link_click'] ?? null); + } + + /** + * Clearing the Stage URL on the admin page stores it empty, which stands + * for the one the app ships with. + */ + #[DataProvider('provideUsers')] + public function testAClearedStageUrlFallsBackToTheShippedOne(?string $userId): void { + $state = $this->boot(['base_url' => ''], $userId); + + $this->assertSame(Application::DEFAULT_BASE_URL, $state['base_url'] ?? null); + } + + public static function provideUsers(): array { + return [ + 'a visitor who is not logged in' => [null], + 'a user without a Stage URL of their own' => ['jdoe'], + ]; + } + + public function testTheAdminValuesReachThePage(): void { + $state = $this->boot(['base_url' => 'https://stage.example.com', 'override_link_click' => '1'], 'jdoe'); + + $this->assertSame('https://stage.example.com', $state['base_url'] ?? null); + $this->assertTrue($state['override_link_click'] ?? null); + } + + public function testAUsersOwnStageUrlComesFirst(): void { + $state = $this->boot(['base_url' => 'https://stage.example.com'], 'jdoe', ['base_url' => 'https://own.example.com']); + + $this->assertSame('https://own.example.com', $state['base_url'] ?? null); + } +} diff --git a/tests/unit/Controller/ConfigControllerTest.php b/tests/unit/Controller/ConfigControllerTest.php index 3a1f284..84bde37 100644 --- a/tests/unit/Controller/ConfigControllerTest.php +++ b/tests/unit/Controller/ConfigControllerTest.php @@ -9,13 +9,17 @@ use OCA\Nuiteq\Controller\ConfigController; use OCA\Nuiteq\Service\NuiteqAPIService; +use OCP\AppFramework\Http; +use OCP\AppFramework\Services\IAppConfig; use OCP\IConfig; use OCP\IRequest; use OCP\Security\ICrypto; +use PHPUnit\Framework\Attributes\DataProvider; use PHPUnit\Framework\TestCase; class ConfigControllerTest extends TestCase { + private IAppConfig $appConfig; private IConfig $config; private ICrypto $crypto; private ConfigController $controller; @@ -23,13 +27,20 @@ class ConfigControllerTest extends TestCase { public function setUp(): void { parent::setUp(); + $this->appConfig = $this->createMock(IAppConfig::class); $this->config = $this->createMock(IConfig::class); + // the old API answers a lazy value with the default, so a read through + // it must not pass for the right one + foreach (['getAppValue', 'setAppValue', 'deleteAppValue'] as $method) { + $this->config->expects($this->never())->method($method); + } $this->crypto = $this->createMock(ICrypto::class); $this->crypto->method('encrypt')->willReturnCallback(fn ($value) => 'encrypted(' . $value . ')'); $this->controller = new ConfigController( 'integration_nuiteq', $this->createMock(IRequest::class), + $this->appConfig, $this->config, $this->createMock(NuiteqAPIService::class), $this->crypto, @@ -42,9 +53,9 @@ public function setUp(): void { * other admin setting used to replace the real key with the placeholder. */ public function testThePlaceholderSecretIsNotStoredAsTheAdminClientKey(): void { - $this->config->expects($this->once()) - ->method('setAppValue') - ->with('integration_nuiteq', 'base_url', 'https://stage.example.com'); + $this->appConfig->expects($this->once()) + ->method('setAppValueString') + ->with('base_url', 'https://stage.example.com'); $this->controller->setAdminConfig([ 'client_key' => 'dummySecret', @@ -79,9 +90,10 @@ public function testAUserClientKeyIsTheOnlyValueEncrypted(): void { public function testAnAdminClientKeyIsStoredEncrypted(): void { $stored = []; - $this->config->method('setAppValue') - ->willReturnCallback(function ($app, $key, $value) use (&$stored): void { + $this->appConfig->method('setAppValueString') + ->willReturnCallback(function ($key, $value) use (&$stored): bool { $stored[$key] = $value; + return true; }); $this->controller->setAdminConfig(['client_key' => 'a-real-key']); @@ -91,9 +103,10 @@ public function testAnAdminClientKeyIsStoredEncrypted(): void { public function testAClearedAdminClientKeyIsStoredAsItIs(): void { $stored = []; - $this->config->method('setAppValue') - ->willReturnCallback(function ($app, $key, $value) use (&$stored): void { + $this->appConfig->method('setAppValueString') + ->willReturnCallback(function ($key, $value) use (&$stored): bool { $stored[$key] = $value; + return true; }); $this->controller->setAdminConfig(['client_key' => '']); @@ -101,6 +114,127 @@ public function testAClearedAdminClientKeyIsStoredAsItIs(): void { $this->assertSame(['client_key' => ''], $stored); } + /** + * A write sets the flag of the row it writes: written non-lazy, the admin + * key would be loaded on every request again. + */ + public function testTheAdminClientKeyIsWrittenLazily(): void { + $this->appConfig->expects($this->once()) + ->method('setAppValueString') + ->with('client_key', 'encrypted(a-real-key)', true); + + $this->controller->setAdminConfig(['client_key' => 'a-real-key']); + } + + public function testAClearedAdminClientKeyIsWrittenLazilyToo(): void { + $this->appConfig->expects($this->once()) + ->method('setAppValueString') + ->with('client_key', '', true); + + $this->controller->setAdminConfig(['client_key' => '']); + } + + /** + * The Stage URL is read on every page, so it stays a non-lazy value. + */ + public function testTheAdminBaseUrlIsWrittenNonLazily(): void { + $this->appConfig->expects($this->once()) + ->method('setAppValueString') + ->with('base_url', 'https://stage.example.com', false); + + $this->controller->setAdminConfig(['base_url' => 'https://stage.example.com']); + } + + /** + * What the admin settings send: the Stage URL always, and the key unless + * the field still shows the placeholder for a stored one. + */ + #[DataProvider('provideWhatTheAdminPageSends')] + public function testWhatTheAdminPageSendsIsSaved(array $values, array $expected): void { + $stored = []; + $this->appConfig->method('setAppValueString') + ->willReturnCallback(function (string $key, string $value, bool $lazy = false) use (&$stored): bool { + $stored[$key] = [$value, $lazy]; + return true; + }); + + $response = $this->controller->setAdminConfig($values); + + $this->assertSame(Http::STATUS_OK, $response->getStatus()); + $this->assertSame($expected, $stored); + } + + public static function provideWhatTheAdminPageSends(): array { + return [ + 'a Stage URL, with the key field empty' => [ + ['base_url' => 'https://new.example.com', 'client_key' => ''], + ['base_url' => ['https://new.example.com', false], 'client_key' => ['', true]], + ], + 'a Stage URL, with a key stored before' => [ + ['base_url' => 'https://new.example.com'], + ['base_url' => ['https://new.example.com', false]], + ], + 'a new key' => [ + ['base_url' => 'https://stage.example.com', 'client_key' => 'a-real-key'], + ['base_url' => ['https://stage.example.com', false], 'client_key' => ['encrypted(a-real-key)', true]], + ], + ]; + } + + /** + * A typed write sets the type of the row it writes, and Nextcloud's own + * rows of this app are untyped: once installed_version is a string, the + * next upgrade fails on the conflict and leaves the instance in + * maintenance mode, and once enabled is one, enabling the app fails. + */ + #[DataProvider('provideWhatIsNotAnAdminSetting')] + public function testNothingButTheAdminSettingsIsStored(array $values): void { + $this->appConfig->expects($this->never())->method('setAppValueString'); + + $response = $this->controller->setAdminConfig($values); + + $this->assertSame(Http::STATUS_BAD_REQUEST, $response->getStatus()); + $this->assertArrayHasKey('error', $response->getData()); + } + + public static function provideWhatIsNotAnAdminSetting(): array { + return [ + 'the installed version' => [['installed_version' => '1.3.0']], + 'whether the app is enabled' => [['enabled' => 'no']], + 'the link override, which has no setting' => [['override_link_click' => '1']], + 'a value without a name' => [['https://stage.example.com']], + // every key is checked before any is written + 'a Stage URL beside the installed version' => [['base_url' => 'https://stage.example.com', 'installed_version' => '1.3.0']], + ]; + } + + /** + * The admin settings send only strings. Anything else used to be stored + * as some string, null as an empty one, and the typed setter answers null + * or a list with a server error. + */ + #[DataProvider('provideValuesThatAreNotStrings')] + public function testAValueThatIsNotAStringIsRefused(array $values): void { + $this->appConfig->expects($this->never())->method('setAppValueString'); + + $response = $this->controller->setAdminConfig($values); + + $this->assertSame(Http::STATUS_BAD_REQUEST, $response->getStatus()); + $this->assertArrayHasKey('error', $response->getData()); + } + + public static function provideValuesThatAreNotStrings(): array { + return [ + 'a Stage URL that is null' => [['base_url' => null]], + 'a key that is null' => [['client_key' => null]], + 'a Stage URL in a list' => [['base_url' => ['https://stage.example.com']]], + 'a Stage URL that is a boolean' => [['base_url' => true]], + 'a key that is a number' => [['client_key' => 42]], + // nothing is written before the value that is refused + 'a key beside a Stage URL that is null' => [['client_key' => 'a-real-key', 'base_url' => null]], + ]; + } + public function testThePlaceholderSecretIsNotStoredAsTheUserClientKeyEither(): void { $stored = []; $this->config->method('setUserValue') diff --git a/tests/unit/Listener/AddContentSecurityPolicyListenerTest.php b/tests/unit/Listener/AddContentSecurityPolicyListenerTest.php new file mode 100644 index 0000000..a653299 --- /dev/null +++ b/tests/unit/Listener/AddContentSecurityPolicyListenerTest.php @@ -0,0 +1,80 @@ +createMock(IConfig::class); + $config->method('getUserValue')->willReturnArgument(3); + // the old API answers a lazy value with the default, so a read through + // it must not pass for the right one + foreach (['getAppValue', 'setAppValue', 'deleteAppValue'] as $method) { + $config->expects($this->never())->method($method); + } + $policy = ''; + $event = $this->createMock(AddContentSecurityPolicyEvent::class); + $event->expects($this->once()) + ->method('addPolicy') + ->willReturnCallback(function (ContentSecurityPolicy $csp) use (&$policy): void { + $policy = $csp->buildPolicy(); + }); + + (new AddContentSecurityPolicyListener($config, $appConfig, 'jdoe'))->handle($event); + + return $policy; + } + + /** + * The policy is built for every page, so the Stage URL it allows is read + * on every request, from the values Nextcloud loads up front: a lazy read + * would load every lazy value of every app with each page. + */ + public function testTheAdminBaseUrlIsReadNonLazily(): void { + $appConfig = $this->createMock(IAppConfig::class); + $appConfig->expects($this->once()) + ->method('getAppValueString') + ->with('base_url', Application::DEFAULT_BASE_URL, false) + ->willReturn('https://stage.example.com'); + + $this->assertStringContainsString('https://stage.example.com', $this->policy($appConfig)); + } + + /** + * @param array $adminValues the stored admin values; a + * missing one reads as the default + */ + #[DataProvider('provideStageUrlsThatAreNotSet')] + public function testWithoutAStageUrlTheShippedOneIsAllowed(array $adminValues): void { + $appConfig = $this->createMock(IAppConfig::class); + $appConfig->method('getAppValueString')->willReturnCallback( + fn (string $key, string $default = '', bool $lazy = false): string => $adminValues[$key] ?? $default + ); + + $this->assertStringContainsString(Application::DEFAULT_BASE_URL, $this->policy($appConfig)); + } + + public static function provideStageUrlsThatAreNotSet(): array { + return [ + 'never set' => [[]], + 'cleared on the admin page' => [['base_url' => '']], + ]; + } +} diff --git a/tests/unit/Migration/Version1200Date202410151354Test.php b/tests/unit/Migration/Version1200Date202410151354Test.php new file mode 100644 index 0000000..e32f2ab --- /dev/null +++ b/tests/unit/Migration/Version1200Date202410151354Test.php @@ -0,0 +1,63 @@ +markTestSkipped('a database connection cannot be mocked without Doctrine, which a server checkout ships'); + } + } + + /** + * A write sets the flag of the row it writes, so encrypting the admin key + * of an install older than 1.0.8 has to keep it lazy. + */ + public function testTheAdminClientKeyIsReadAndWrittenLazily(): void { + $appConfig = $this->createMock(IAppConfig::class); + $appConfig->expects($this->once()) + ->method('getValueString') + ->with('integration_nuiteq', 'client_key', '', true) + ->willReturn('a-key-in-clear'); + $appConfig->expects($this->once()) + ->method('setValueString') + ->with('integration_nuiteq', 'client_key', 'encrypted(a-key-in-clear)', true) + ->willReturn(true); + $crypto = $this->createMock(ICrypto::class); + $crypto->method('encrypt')->willReturnCallback(fn (string $value): string => 'encrypted(' . $value . ')'); + + // no user has a client key of their own + $result = $this->createMock(IResult::class); + $result->method('fetch')->willReturn(false); + $qb = $this->createMock(IQueryBuilder::class); + foreach (['update', 'set', 'where', 'andWhere', 'select', 'from'] as $method) { + $qb->method($method)->willReturnSelf(); + } + $qb->method('expr')->willReturn($this->createMock(IExpressionBuilder::class)); + $qb->method('executeQuery')->willReturn($result); + $connection = $this->createMock(IDBConnection::class); + $connection->method('getQueryBuilder')->willReturn($qb); + + (new Version1200Date202410151354($connection, $crypto, $appConfig)) + ->postSchemaChange($this->createMock(IOutput::class), fn () => null, []); + } +} diff --git a/tests/unit/Migration/Version1301Date202610211026Test.php b/tests/unit/Migration/Version1301Date202610211026Test.php new file mode 100644 index 0000000..213a28c --- /dev/null +++ b/tests/unit/Migration/Version1301Date202610211026Test.php @@ -0,0 +1,32 @@ +createMock(IAppConfig::class); + $appConfig->expects($this->once()) + ->method('updateLazy') + ->with('integration_nuiteq', 'client_key', true) + ->willReturn(true); + + (new Version1301Date202610211026($appConfig)) + ->postSchemaChange($this->createMock(IOutput::class), fn () => null, []); + } +} diff --git a/tests/unit/Service/NuiteqAPIServiceTest.php b/tests/unit/Service/NuiteqAPIServiceTest.php index c1a5b88..47aadf8 100644 --- a/tests/unit/Service/NuiteqAPIServiceTest.php +++ b/tests/unit/Service/NuiteqAPIServiceTest.php @@ -13,6 +13,7 @@ use GuzzleHttp\Psr7\Response; use OCA\Nuiteq\AppInfo\Application; use OCA\Nuiteq\Service\NuiteqAPIService; +use OCP\AppFramework\Services\IAppConfig; use OCP\Http\Client\IClient; use OCP\Http\Client\IClientService; use OCP\Http\Client\IResponse; @@ -29,6 +30,14 @@ class NuiteqAPIServiceTest extends TestCase { private LoggerInterface $logger; private ICrypto $crypto; private string $storedClientKey = 'encrypted'; + /** the user's own Stage URL, null for none */ + private ?string $storedBaseUrl = 'https://stage.example.com'; + /** @var array the admin values stored as non-lazy ones */ + private array $adminValues = []; + /** @var array the admin values stored as lazy ones */ + private array $lazyAdminValues = []; + /** @var list the admin values asked for, and whether lazily */ + private array $adminReads = []; private NuiteqAPIService $apiService; public function setUp(): void { @@ -49,13 +58,28 @@ public function setUp(): void { $config = $this->createMock(IConfig::class); $config->method('getUserValue')->willReturnCallback( fn ($userId, $appName, $key, $default = '') => match ($key) { - 'base_url' => 'https://stage.example.com', + 'base_url' => $this->storedBaseUrl ?? $default, 'api_key' => 'an-api-key', 'client_key' => $this->storedClientKey, default => $default, } ); - $config->method('getAppValue')->willReturn(''); + // the old API answers a lazy value with the default, so a read through + // it must not pass for the right one + foreach (['getAppValue', 'setAppValue', 'deleteAppValue'] as $method) { + $config->expects($this->never())->method($method); + } + + // as Nextcloud answers before anything has loaded the lazy values: a + // non-lazy read of a lazy value gets the default, as does a read of a + // value never set + $appConfig = $this->createMock(IAppConfig::class); + $appConfig->method('getAppValueString')->willReturnCallback( + function (string $key, string $default = '', bool $lazy = false): string { + $this->adminReads[] = [$key, $lazy]; + return $this->adminValues[$key] ?? ($lazy ? ($this->lazyAdminValues[$key] ?? $default) : $default); + } + ); $this->logger = $this->createMock(LoggerInterface::class); @@ -63,6 +87,7 @@ public function setUp(): void { 'integration_nuiteq', $this->logger, $l10n, + $appConfig, $config, $this->crypto, $clientService, @@ -183,6 +208,79 @@ public function testAKeyThatCannotBeReadFallsBackToTheConfiguredOne(): void { $this->assertSame(Application::DEFAULT_CLIENT_KEY, $sent['clientKey'] ?? null); } + /** + * A non-lazy read of the admin key, now a lazy value, answers the default, + * and every request would go out with the shipped key instead. + */ + public function testTheAdminClientKeyIsReadLazily(): void { + $this->storedClientKey = ''; + $this->lazyAdminValues['client_key'] = 'the-admin-key-encrypted'; + $sent = []; + $response = $this->createMock(IResponse::class); + $response->method('getStatusCode')->willReturn(200); + $response->method('getBody')->willReturn('{"apiKey":"a-fresh-api-key"}'); + $this->client->method('post')->willReturnCallback(function ($url, $options) use (&$sent, $response) { + $sent = $options['json'] ?? []; + return $response; + }); + + $this->apiService->login('admin', 'jdoe', 'secret'); + + // what the crypto double decrypts every readable value to + $this->assertSame('a-client-key', $sent['clientKey'] ?? null); + $this->assertContains(['client_key', true], $this->adminReads); + $this->assertNotContains(['client_key', false], $this->adminReads); + } + + /** + * The Stage URL is read on every page, so it stays a non-lazy value, and a + * lazy read of it would load every lazy value for nothing. + */ + public function testTheAdminBaseUrlIsReadNonLazily(): void { + $this->answers(['boardId' => 'b-42']); + + $this->apiService->newBoard('admin', 'New board', ''); + + $this->assertContains(['base_url', false], $this->adminReads); + $this->assertNotContains(['base_url', true], $this->adminReads); + } + + /** + * Clearing the key on the admin page stores it empty, which stands for + * the key the app ships with. + */ + public function testAClearedAdminClientKeyFallsBackToTheShippedOne(): void { + $this->storedClientKey = ''; + $this->lazyAdminValues['client_key'] = ''; + $sent = []; + $response = $this->createMock(IResponse::class); + $response->method('getStatusCode')->willReturn(200); + $response->method('getBody')->willReturn('{"apiKey":"a-fresh-api-key"}'); + $this->client->method('post')->willReturnCallback(function ($url, $options) use (&$sent, $response) { + $sent = $options['json'] ?? []; + return $response; + }); + + $this->apiService->login('admin', 'jdoe', 'secret'); + + $this->assertSame(Application::DEFAULT_CLIENT_KEY, $sent['clientKey'] ?? null); + } + + #[DataProvider('provideStageUrlsThatAreNotSet')] + public function testWithoutAStageUrlTheShippedOneIsUsed(array $adminValues): void { + $this->storedBaseUrl = null; + $this->adminValues = $adminValues; + + $this->assertSame(Application::DEFAULT_BASE_URL, $this->apiService->getBaseUrl('admin')); + } + + public static function provideStageUrlsThatAreNotSet(): array { + return [ + 'never set' => [[]], + 'cleared on the admin page' => [['base_url' => '']], + ]; + } + public function testAnAnswerThatIsNotJsonIsReportedAsSuch(): void { $response = $this->createMock(IResponse::class); $response->method('getStatusCode')->willReturn(200); diff --git a/tests/unit/Settings/AdminTest.php b/tests/unit/Settings/AdminTest.php new file mode 100644 index 0000000..d8acdfa --- /dev/null +++ b/tests/unit/Settings/AdminTest.php @@ -0,0 +1,80 @@ + the admin values asked for, and whether lazily */ + private array $reads = []; + + /** + * The admin settings as they are handed to the page. + * + * @param array $values the admin values stored as non-lazy ones + * @param array $lazyValues the admin values stored as lazy ones + */ + private function state(array $values, array $lazyValues): array { + // as Nextcloud answers before anything has loaded the lazy values: a + // non-lazy read of a lazy value gets the default, as does a read of a + // value never set + $appConfig = $this->createMock(IAppConfig::class); + $appConfig->method('getAppValueString')->willReturnCallback( + function (string $key, string $default = '', bool $lazy = false) use ($values, $lazyValues): string { + $this->reads[] = [$key, $lazy]; + return $values[$key] ?? ($lazy ? ($lazyValues[$key] ?? $default) : $default); + } + ); + $state = null; + $initialState = $this->createMock(IInitialState::class); + $initialState->expects($this->once()) + ->method('provideInitialState') + ->willReturnCallback(function (string $key, $value) use (&$state): void { + $state = $value; + }); + + (new Admin($appConfig, $initialState))->getForm(); + + return $state; + } + + /** + * Read non-lazily, the admin key would read as unset, and the page would + * ask for a key the admin has already set. + */ + public function testAStoredAdminClientKeyIsShownAsSet(): void { + $state = $this->state(['base_url' => 'https://stage.example.com'], ['client_key' => 'encrypted']); + + // the stored key is never shipped to the browser + $this->assertSame('dummySecret', $state['client_key']); + $this->assertContains(['client_key', true], $this->reads); + $this->assertNotContains(['client_key', false], $this->reads); + } + + public function testAnInstallWithNothingSetShowsTheShippedStageUrlAndNoKey(): void { + $state = $this->state([], []); + + $this->assertSame('https://nuiteqstage.se', $state['base_url']); + $this->assertSame('', $state['client_key']); + } + + /** + * The Stage URL is read on every page, so it stays a non-lazy value, and a + * lazy read of it would load every lazy value for nothing. + */ + public function testTheAdminBaseUrlIsReadNonLazily(): void { + $state = $this->state(['base_url' => 'https://stage.example.com'], []); + + $this->assertSame('https://stage.example.com', $state['base_url']); + $this->assertContains(['base_url', false], $this->reads); + $this->assertNotContains(['base_url', true], $this->reads); + } +} diff --git a/tests/unit/Settings/PersonalTest.php b/tests/unit/Settings/PersonalTest.php index efbff39..700fa27 100644 --- a/tests/unit/Settings/PersonalTest.php +++ b/tests/unit/Settings/PersonalTest.php @@ -9,9 +9,11 @@ use Exception; use OCA\Nuiteq\Settings\Personal; +use OCP\AppFramework\Services\IAppConfig; use OCP\AppFramework\Services\IInitialState; use OCP\IConfig; use OCP\Security\ICrypto; +use PHPUnit\Framework\Attributes\DataProvider; use PHPUnit\Framework\TestCase; use Psr\Log\LoggerInterface; @@ -21,18 +23,28 @@ * connected. */ class PersonalTest extends TestCase { + private IAppConfig $appConfig; private IConfig $config; private ICrypto $crypto; private IInitialState $initialState; private Personal $settings; + /** @var list the admin values asked for, and whether lazily */ + private array $adminReads = []; protected function setUp(): void { parent::setUp(); + $this->appConfig = $this->createMock(IAppConfig::class); $this->config = $this->createMock(IConfig::class); + // the old API answers a lazy value with the default, so a read through + // it must not pass for the right one + foreach (['getAppValue', 'setAppValue', 'deleteAppValue'] as $method) { + $this->config->expects($this->never())->method($method); + } $this->crypto = $this->createMock(ICrypto::class); $this->initialState = $this->createMock(IInitialState::class); $this->settings = new Personal( + $this->appConfig, $this->config, $this->initialState, $this->crypto, @@ -43,12 +55,18 @@ protected function setUp(): void { /** * @param array $values the stored user values + * @param array $adminValues the stored admin values */ - private function stored(array $values): void { + private function stored(array $values, array $adminValues = []): void { $this->config->method('getUserValue')->willReturnCallback( fn (string $userId, string $app, string $key, string $default = '') => $values[$key] ?? $default ); - $this->config->method('getAppValue')->willReturn(''); + $this->appConfig->method('getAppValueString')->willReturnCallback( + function (string $key, string $default = '', bool $lazy = false) use ($adminValues): string { + $this->adminReads[] = [$key, $lazy]; + return $adminValues[$key] ?? $default; + } + ); } private function state(): array { @@ -87,6 +105,32 @@ public function testAKeyThatCannotBeReadIsShownAsNotConnected(): void { $this->assertSame('', $state['client_key']); } + /** + * The Stage URL is read on every page, so it stays a non-lazy value, and a + * lazy read of it would load every lazy value for nothing. + */ + public function testTheAdminBaseUrlIsReadNonLazily(): void { + $this->stored(['client_key' => '', 'api_key' => '', 'user_name' => '']); + + $this->state(); + + $this->assertSame([['base_url', false]], $this->adminReads); + } + + #[DataProvider('provideStageUrlsThatAreNotSet')] + public function testWithoutAStageUrlTheShippedOneIsShown(array $adminValues): void { + $this->stored(['client_key' => '', 'api_key' => '', 'user_name' => ''], $adminValues); + + $this->assertSame('https://nuiteqstage.se', $this->state()['base_url']); + } + + public static function provideStageUrlsThatAreNotSet(): array { + return [ + 'never set' => [[]], + 'cleared on the admin page' => [['base_url' => '']], + ]; + } + public function testAnAccountWithNoKeyIsLeftAlone(): void { $this->stored(['client_key' => '', 'api_key' => 'an-api-key', 'user_name' => 'jdoe']); $this->crypto->expects($this->never())->method('decrypt');