From 7a553d529a1dc899b175bbb59eef4704530ebc96 Mon Sep 17 00:00:00 2001 From: Oleksander Piskun Date: Wed, 21 Oct 2026 10:36:38 +0000 Subject: [PATCH] perf: load the admin client key lazily Since Nextcloud 29 every request loads the non-lazy app config values of every app, and the lazy ones only once some code asks for one lazily. The app wrote its app config with IConfig::setAppValue(), which always stores a value non-lazy, so the admin's NUITEQ client key was loaded on every request, while only the app's own requests, its admin settings and its migrations read it. The client key is read and written through IAppConfig with lazy: true everywhere now: in NuiteqAPIService, the admin settings, the admin-config endpoint and the 1.0.8 migration that encrypted it. IConfig::getAppValue() reads only the non-lazy values, so no read of a lazy key may stay on the old API: it would answer the default, and the requests would go out with the shipped key instead of the admin's. base_url and override_link_click stay non-lazy. Application::boot() reads both on every request, for the script it adds to every page, and the CSP listener reads base_url for every page. Their other reads, and the admin-config endpoint's write of base_url, move to IAppConfig with the same flag. The admin-config endpoint stored whatever keys a request named. Through the typed setter that does harm: it sets the type of the row it writes, and Nextcloud's own rows of the app are untyped. Once installed_version is a string, the next upgrade fails on the type conflict and leaves the instance in maintenance mode, and once enabled is one, enabling the app fails the same way. The endpoint takes only what the admin settings send, base_url and client_key, and only as strings, and answers anything else with a 400 before it writes anything. override_link_click has no setting and is left to occ. A new migration marks client_key lazy on existing installs, where the old API stored it non-lazy; it does nothing on an install without a key, and until it runs the lazy reads still find the non-lazy row. The stored values do not change: the key stays encrypted with ICrypto, and every value stays a string. Signed-off-by: Oleksander Piskun --- lib/AppInfo/Application.php | 6 +- lib/Controller/ConfigController.php | 17 +- .../AddContentSecurityPolicyListener.php | 10 +- lib/Migration/Version1200Date202410151354.php | 8 +- lib/Migration/Version1301Date202610211026.php | 41 +++++ lib/Service/NuiteqAPIService.php | 6 +- lib/Settings/Admin.php | 8 +- lib/Settings/Personal.php | 4 +- tests/unit/AppInfo/ApplicationTest.php | 153 ++++++++++++++++++ .../unit/Controller/ConfigControllerTest.php | 148 ++++++++++++++++- .../AddContentSecurityPolicyListenerTest.php | 80 +++++++++ .../Version1200Date202410151354Test.php | 63 ++++++++ .../Version1301Date202610211026Test.php | 32 ++++ tests/unit/Service/NuiteqAPIServiceTest.php | 102 +++++++++++- tests/unit/Settings/AdminTest.php | 80 +++++++++ tests/unit/Settings/PersonalTest.php | 48 +++++- 16 files changed, 778 insertions(+), 28 deletions(-) create mode 100644 lib/Migration/Version1301Date202610211026.php create mode 100644 tests/unit/AppInfo/ApplicationTest.php create mode 100644 tests/unit/Listener/AddContentSecurityPolicyListenerTest.php create mode 100644 tests/unit/Migration/Version1200Date202410151354Test.php create mode 100644 tests/unit/Migration/Version1301Date202610211026Test.php create mode 100644 tests/unit/Settings/AdminTest.php 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');