From f6dd91580ba9983f84aee903428202b56d70c784 Mon Sep 17 00:00:00 2001 From: Oleksander Piskun Date: Wed, 21 Oct 2026 10:35:35 +0000 Subject: [PATCH] perf: load the app config values lazily Since Nextcloud 29 every request loads the non-lazy app config rows of every app. All of this app's values were read and written through the deprecated IConfig::getAppValue() and setAppValue(), and setAppValue() always stores a key as non-lazy, although only dashboard_enabled and issue_notifications_enabled are read on every request, in Application::register(). Every read and write of an app config key now goes through the typed methods of IAppConfig, and every delete through its deleteAppValue(): the app-scoped IAppConfig in the app's code, and OCP\IAppConfig in the two migrations. The OAuth client id and secret, the default link token, use_popup, link_preview_enabled, the two allow_default_link_token_* switches and the default token's user_name and user_displayname pass lazy: true on every read and write, because a read without it can return the default once the key is lazy, and a write without it makes the key non-lazy again. dashboard_enabled and issue_notifications_enabled stay non-lazy. Stored values keep their format, and the secrets stay encrypted with ICrypto. Besides the three secrets, the two admin endpoints wrote any key the request carried. Written lazily, the server's own enabled key would make the app look disabled, so setAdminConfig() now accepts only the six switches of the admin settings and setSensitiveAdminConfig() only the three secrets, both only as strings, and they answer 400 without writing anything otherwise. A new migration marks the lazy keys lazy on existing installs with IAppConfig::updateLazy(). The unit tests pin the flag and the default of each call, which of the app's keys, the server's own keys and an unknown key each endpoint accepts, and the keys the migration marks. Signed-off-by: Oleksander Piskun --- CHANGELOG.md | 4 + lib/AppInfo/Application.php | 7 +- lib/Controller/ConfigController.php | 45 +- lib/Dashboard/GithubWidget.php | 10 +- .../Version030010Date20241002015812.php | 8 +- .../Version030301Date20261021102507.php | 58 ++ lib/Reference/GithubCodeReferenceProvider.php | 4 +- .../GithubIssuePrReferenceProvider.php | 4 +- lib/Service/GithubAPIService.php | 12 +- lib/Service/SecretService.php | 12 +- lib/Settings/Admin.php | 20 +- lib/Settings/Personal.php | 6 +- tests/integration/MockedGithubApiTrait.php | 22 +- .../Controller/GithubAPIControllerTest.php | 2 + tests/unit/LazyAppConfigTest.php | 631 ++++++++++++++++++ .../Version030010Date20241002015812Test.php | 65 ++ .../Version030301Date20261021102507Test.php | 45 ++ tests/unit/Service/SecretServiceTest.php | 20 +- 18 files changed, 917 insertions(+), 58 deletions(-) create mode 100644 lib/Migration/Version030301Date20261021102507.php create mode 100644 tests/unit/LazyAppConfigTest.php create mode 100644 tests/unit/Migration/Version030010Date20241002015812Test.php create mode 100644 tests/unit/Migration/Version030301Date20261021102507Test.php diff --git a/CHANGELOG.md b/CHANGELOG.md index 6c8e8192..57abda88 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,10 @@ and this project adheres to [Semantic Versioning](http://semver.org/). ## [Unreleased] +### Changed + +- Stop loading most of the app's settings on every request + ## 3.3.0 - 2026-10-02 ### Added diff --git a/lib/AppInfo/Application.php b/lib/AppInfo/Application.php index 5fe81788..9a6214a3 100644 --- a/lib/AppInfo/Application.php +++ b/lib/AppInfo/Application.php @@ -19,6 +19,7 @@ use OCP\AppFramework\Bootstrap\IBootContext; use OCP\AppFramework\Bootstrap\IBootstrap; use OCP\AppFramework\Bootstrap\IRegistrationContext; +use OCP\AppFramework\Services\IAppConfig; use OCP\Collaboration\Reference\RenderReferenceEvent; @@ -37,20 +38,22 @@ class Application extends App implements IBootstrap { private ContainerInterface $container; private IConfig $config; + private IAppConfig $appConfig; public function __construct(array $urlParams = []) { parent::__construct(self::APP_ID, $urlParams); $this->container = $this->getContainer(); $this->config = $this->container->get(IConfig::class); + $this->appConfig = $this->container->get(IAppConfig::class); } public function register(IRegistrationContext $context): void { - if ($this->config->getAppValue(self::APP_ID, 'dashboard_enabled', '1') === '1') { + if ($this->appConfig->getAppValueString('dashboard_enabled', '1') === '1') { $context->registerDashboardWidget(GithubWidget::class); } - if ($this->config->getAppValue(self::APP_ID, 'issue_notifications_enabled', '1') === '1') { + if ($this->appConfig->getAppValueString('issue_notifications_enabled', '1') === '1') { $context->registerNotifierService(GitHubNotifier::class); } diff --git a/lib/Controller/ConfigController.php b/lib/Controller/ConfigController.php index 48e49c55..d312e472 100644 --- a/lib/Controller/ConfigController.php +++ b/lib/Controller/ConfigController.php @@ -21,6 +21,7 @@ use OCP\AppFramework\Http\DataResponse; use OCP\AppFramework\Http\RedirectResponse; use OCP\AppFramework\Http\TemplateResponse; +use OCP\AppFramework\Services\IAppConfig; use OCP\AppFramework\Services\IInitialState; use OCP\IConfig; use OCP\IL10N; @@ -32,10 +33,27 @@ class ConfigController extends Controller { + /** + * The switches of the admin settings, and whether each one is stored lazily. + * dashboard_enabled and issue_notifications_enabled are read in Application::register(), + * which runs on every request, so they stay non-lazy. + */ + private const ADMIN_SWITCHES = [ + 'use_popup' => true, + 'link_preview_enabled' => true, + 'allow_default_link_token_to_anonymous' => true, + 'allow_default_link_token_to_guests' => true, + 'dashboard_enabled' => false, + 'issue_notifications_enabled' => false, + ]; + + private const SECRET_KEYS = ['client_id', 'client_secret', 'default_link_token']; + public function __construct( string $appName, IRequest $request, private IConfig $config, + private IAppConfig $appConfig, private IURLGenerator $urlGenerator, private IL10N $l, private IInitialState $initialStateService, @@ -108,13 +126,15 @@ public function setConfig(array $values): DataResponse { * @return DataResponse */ public function setAdminConfig(array $values): DataResponse { + // only the switches, so that a request cannot write a key the server owns, such as "enabled" foreach ($values as $key => $value) { - if (in_array($key, ['client_id', 'client_secret', 'default_link_token'], true)) { + if (!array_key_exists($key, self::ADMIN_SWITCHES) || !is_string($value)) { return new DataResponse([], Http::STATUS_BAD_REQUEST); - } else { - $this->config->setAppValue(Application::APP_ID, $key, $value); } } + foreach ($values as $key => $value) { + $this->appConfig->setAppValueString($key, $value, lazy: self::ADMIN_SWITCHES[$key]); + } return new DataResponse([]); } @@ -127,14 +147,15 @@ public function setAdminConfig(array $values): DataResponse { #[PasswordConfirmationRequired] public function setSensitiveAdminConfig(array $values): DataResponse { foreach ($values as $key => $value) { - if (in_array($key, ['client_id', 'client_secret', 'default_link_token'], true)) { - $this->secretService->setEncryptedAppValue($key, $value); - if ($key === 'default_link_token') { - $info = $this->githubAPIService->updateLinkTokenUserInfo($value); - return new DataResponse($info); - } - } else { - $this->config->setAppValue(Application::APP_ID, $key, $value); + if (!in_array($key, self::SECRET_KEYS, true) || !is_string($value)) { + return new DataResponse([], Http::STATUS_BAD_REQUEST); + } + } + foreach ($values as $key => $value) { + $this->secretService->setEncryptedAppValue($key, $value); + if ($key === 'default_link_token') { + $info = $this->githubAPIService->updateLinkTokenUserInfo($value); + return new DataResponse($info); } } return new DataResponse([]); @@ -184,7 +205,7 @@ public function oauthRedirect(string $code, string $state): RedirectResponse { $this->config->setUserValue($this->userId, Application::APP_ID, 'token_type', 'oauth'); $userInfo = $this->storeUserInfo(); - $usePopup = $this->config->getAppValue(Application::APP_ID, 'use_popup', '0') === '1'; + $usePopup = $this->appConfig->getAppValueString('use_popup', '0', lazy: true) === '1'; if ($usePopup) { return new RedirectResponse( $this->urlGenerator->linkToRoute('integration_github.config.popupSuccessPage', [ diff --git a/lib/Dashboard/GithubWidget.php b/lib/Dashboard/GithubWidget.php index c4b26d83..508bf044 100644 --- a/lib/Dashboard/GithubWidget.php +++ b/lib/Dashboard/GithubWidget.php @@ -9,6 +9,7 @@ use OCA\Github\AppInfo\Application; use OCA\Github\Service\GithubAPIService; +use OCP\AppFramework\Services\IAppConfig; use OCP\AppFramework\Services\IInitialState; use OCP\Dashboard\IAPIWidget; use OCP\Dashboard\IButtonWidget; @@ -16,7 +17,6 @@ use OCP\Dashboard\IOptionWidget; use OCP\Dashboard\Model\WidgetButton; use OCP\Dashboard\Model\WidgetOptions; -use OCP\IConfig; use OCP\IL10N; use OCP\IURLGenerator; @@ -26,7 +26,7 @@ class GithubWidget implements IAPIWidget, IButtonWidget, IIconWidget, IOptionWid public function __construct( private IL10N $l10n, - private IConfig $config, + private IAppConfig $appConfig, private IURLGenerator $urlGenerator, private IInitialState $initialStateService, private GithubAPIService $githubAPIService, @@ -82,10 +82,10 @@ public function getUrl(): ?string { * @inheritDoc */ public function load(): void { - $clientID = $this->config->getAppValue(Application::APP_ID, 'client_id'); - $clientSecret = $this->config->getAppValue(Application::APP_ID, 'client_secret'); + $clientID = $this->appConfig->getAppValueString('client_id', lazy: true); + $clientSecret = $this->appConfig->getAppValueString('client_secret', lazy: true); $oauthPossible = $clientID !== '' && $clientSecret !== ''; - $usePopup = $this->config->getAppValue(Application::APP_ID, 'use_popup', '0'); + $usePopup = $this->appConfig->getAppValueString('use_popup', '0', lazy: true); $userConfig = [ 'oauth_is_possible' => $oauthPossible, diff --git a/lib/Migration/Version030010Date20241002015812.php b/lib/Migration/Version030010Date20241002015812.php index 0c98f529..25160ab8 100644 --- a/lib/Migration/Version030010Date20241002015812.php +++ b/lib/Migration/Version030010Date20241002015812.php @@ -11,7 +11,7 @@ use Closure; use OCA\Github\AppInfo\Application; use OCP\DB\QueryBuilder\IQueryBuilder; -use OCP\IConfig; +use OCP\IAppConfig; use OCP\IDBConnection; use OCP\Migration\IOutput; use OCP\Migration\SimpleMigrationStep; @@ -22,7 +22,7 @@ class Version030010Date20241002015812 extends SimpleMigrationStep { public function __construct( private IDBConnection $connection, private ICrypto $crypto, - private IConfig $config, + private IAppConfig $appConfig, ) { } @@ -34,10 +34,10 @@ public function __construct( public function postSchemaChange(IOutput $output, Closure $schemaClosure, array $options) { // client_id, client_secret, default_link_token foreach (['client_id', 'client_secret', 'default_link_token'] as $key) { - $value = $this->config->getAppValue(Application::APP_ID, $key); + $value = $this->appConfig->getValueString(Application::APP_ID, $key, lazy: true); if ($value !== '') { $encryptedValue = $this->crypto->encrypt($value); - $this->config->setAppValue(Application::APP_ID, $key, $encryptedValue); + $this->appConfig->setValueString(Application::APP_ID, $key, $encryptedValue, lazy: true); } } diff --git a/lib/Migration/Version030301Date20261021102507.php b/lib/Migration/Version030301Date20261021102507.php new file mode 100644 index 00000000..ad071189 --- /dev/null +++ b/lib/Migration/Version030301Date20261021102507.php @@ -0,0 +1,58 @@ +appConfig->updateLazy(Application::APP_ID, $key, true); + } + + return null; + } +} diff --git a/lib/Reference/GithubCodeReferenceProvider.php b/lib/Reference/GithubCodeReferenceProvider.php index 82f2bd26..62f63adb 100644 --- a/lib/Reference/GithubCodeReferenceProvider.php +++ b/lib/Reference/GithubCodeReferenceProvider.php @@ -10,6 +10,7 @@ use OC\Collaboration\Reference\ReferenceManager; use OCA\Github\AppInfo\Application; use OCA\Github\Service\GithubAPIService; +use OCP\AppFramework\Services\IAppConfig; use OCP\Collaboration\Reference\ADiscoverableReferenceProvider; use OCP\Collaboration\Reference\IReference; use OCP\Collaboration\Reference\Reference; @@ -24,6 +25,7 @@ class GithubCodeReferenceProvider extends ADiscoverableReferenceProvider { public function __construct( private GithubAPIService $githubAPIService, private IConfig $config, + private IAppConfig $appConfig, private IL10N $l10n, private IURLGenerator $urlGenerator, private ReferenceManager $referenceManager, @@ -71,7 +73,7 @@ public function matchReference(string $referenceText): bool { return false; } } - $adminLinkPreviewEnabled = $this->config->getAppValue(Application::APP_ID, 'link_preview_enabled', '1') === '1'; + $adminLinkPreviewEnabled = $this->appConfig->getAppValueString('link_preview_enabled', '1', lazy: true) === '1'; if (!$adminLinkPreviewEnabled) { return false; } diff --git a/lib/Reference/GithubIssuePrReferenceProvider.php b/lib/Reference/GithubIssuePrReferenceProvider.php index 74d2e65b..a52db1e0 100644 --- a/lib/Reference/GithubIssuePrReferenceProvider.php +++ b/lib/Reference/GithubIssuePrReferenceProvider.php @@ -12,6 +12,7 @@ use OC\Collaboration\Reference\ReferenceManager; use OCA\Github\AppInfo\Application; use OCA\Github\Service\GithubAPIService; +use OCP\AppFramework\Services\IAppConfig; use OCP\Collaboration\Reference\ADiscoverableReferenceProvider; use OCP\Collaboration\Reference\IReference; use OCP\Collaboration\Reference\ISearchableReferenceProvider; @@ -31,6 +32,7 @@ class GithubIssuePrReferenceProvider extends ADiscoverableReferenceProvider impl public function __construct( private GithubAPIService $githubAPIService, private IConfig $config, + private IAppConfig $appConfig, private IL10N $l10n, private IURLGenerator $urlGenerator, private ReferenceManager $referenceManager, @@ -85,7 +87,7 @@ public function matchReference(string $referenceText): bool { return false; } } - $adminLinkPreviewEnabled = $this->config->getAppValue(Application::APP_ID, 'link_preview_enabled', '1') === '1'; + $adminLinkPreviewEnabled = $this->appConfig->getAppValueString('link_preview_enabled', '1', lazy: true) === '1'; if (!$adminLinkPreviewEnabled) { return false; } diff --git a/lib/Service/GithubAPIService.php b/lib/Service/GithubAPIService.php index 064c235b..a7fcd8f5 100644 --- a/lib/Service/GithubAPIService.php +++ b/lib/Service/GithubAPIService.php @@ -13,6 +13,7 @@ use GuzzleHttp\Exception\ClientException; use GuzzleHttp\Exception\ServerException; use OCA\Github\AppInfo\Application; +use OCP\AppFramework\Services\IAppConfig; use OCP\Dashboard\Model\WidgetItem; use OCP\Http\Client\IClient; use OCP\Http\Client\IClientService; @@ -37,6 +38,7 @@ public function __construct( private LoggerInterface $logger, private IL10N $l10n, private IConfig $config, + private IAppConfig $appConfig, private IURLGenerator $urlGenerator, private IUserManager $userManager, private IManager $notificationManager, @@ -377,15 +379,15 @@ public function getCommitInfo(?string $userId, string $owner, string $repo, stri public function updateLinkTokenUserInfo(string $defaultLinkToken): array { $info = $this->request(null, 'user', [], 'GET', true, 30, $defaultLinkToken); if (isset($info['login'], $info['name'])) { - $this->config->setAppValue(Application::APP_ID, 'user_name', $info['login']); - $this->config->setAppValue(Application::APP_ID, 'user_displayname', $info['name']); + $this->appConfig->setAppValueString('user_name', $info['login'], lazy: true); + $this->appConfig->setAppValueString('user_displayname', $info['name'], lazy: true); return [ 'user_name' => $info['login'], 'user_displayname' => $info['name'], ]; } else { - $this->config->deleteAppValue(Application::APP_ID, 'user_name'); - $this->config->deleteAppValue(Application::APP_ID, 'user_displayname'); + $this->appConfig->deleteAppValue('user_name'); + $this->appConfig->deleteAppValue('user_displayname'); return [ 'user_name' => '', 'user_displayname' => '', @@ -394,7 +396,7 @@ public function updateLinkTokenUserInfo(string $defaultLinkToken): array { } public function checkNewNotifications() { - $notificationEnabled = $this->config->getAppValue(Application::APP_ID, 'issue_notifications_enabled', '1') === '1'; + $notificationEnabled = $this->appConfig->getAppValueString('issue_notifications_enabled', '1') === '1'; if (!$notificationEnabled) { return; } diff --git a/lib/Service/SecretService.php b/lib/Service/SecretService.php index c96a1c6d..7d972fb9 100644 --- a/lib/Service/SecretService.php +++ b/lib/Service/SecretService.php @@ -9,6 +9,7 @@ use Exception; use OCA\Github\AppInfo\Application; +use OCP\AppFramework\Services\IAppConfig; use OCP\IConfig; use OCP\IUserManager; use OCP\PreConditionNotMetException; @@ -22,6 +23,7 @@ class SecretService { public function __construct( private IConfig $config, + private IAppConfig $appConfig, private IUserManager $userManager, private ICrypto $crypto, private LoggerInterface $logger, @@ -61,11 +63,11 @@ public function getEncryptedUserValue(string $userId, string $key): string { */ public function setEncryptedAppValue(string $key, string $value): void { if ($value === '') { - $this->config->setAppValue(Application::APP_ID, $key, ''); + $this->appConfig->setAppValueString($key, '', lazy: true); return; } $encryptedValue = $this->crypto->encrypt($value); - $this->config->setAppValue(Application::APP_ID, $key, $encryptedValue); + $this->appConfig->setAppValueString($key, $encryptedValue, lazy: true); } /** @@ -73,7 +75,7 @@ public function setEncryptedAppValue(string $key, string $value): void { * @return string the decrypted value, or an empty string if it cannot be decrypted */ public function getEncryptedAppValue(string $key): string { - $storedValue = $this->config->getAppValue(Application::APP_ID, $key); + $storedValue = $this->appConfig->getAppValueString($key, lazy: true); return $this->decryptOrDiscard($storedValue, $key, null); } @@ -124,7 +126,7 @@ public function getAccessToken(?string $userId, bool $endpointUsesDefaultToken = if ($accessToken === '' && $endpointUsesDefaultToken) { $user = $this->userManager->get($userId); $isGuestUser = $user->getBackendClassName() === 'Guests'; - $allowDefaultTokenToGuests = $this->config->getAppValue(Application::APP_ID, 'allow_default_link_token_to_guests', '0') === '1'; + $allowDefaultTokenToGuests = $this->appConfig->getAppValueString('allow_default_link_token_to_guests', '0', lazy: true) === '1'; if ((!$isGuestUser) || $allowDefaultTokenToGuests) { $accessToken = $this->getEncryptedAppValue('default_link_token'); @@ -132,7 +134,7 @@ public function getAccessToken(?string $userId, bool $endpointUsesDefaultToken = } } elseif ($endpointUsesDefaultToken) { // anonymous users - $allowDefaultTokenToAnonymous = $this->config->getAppValue(Application::APP_ID, 'allow_default_link_token_to_anonymous', '0') === '1'; + $allowDefaultTokenToAnonymous = $this->appConfig->getAppValueString('allow_default_link_token_to_anonymous', '0', lazy: true) === '1'; if ($allowDefaultTokenToAnonymous) { $accessToken = $this->getEncryptedAppValue('default_link_token'); } diff --git a/lib/Settings/Admin.php b/lib/Settings/Admin.php index a7e85b83..06cf6c83 100644 --- a/lib/Settings/Admin.php +++ b/lib/Settings/Admin.php @@ -10,15 +10,15 @@ use OCA\Github\Service\GithubAPIService; use OCA\Github\Service\SecretService; 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 SecretService $secretService, - private IConfig $config, + private IAppConfig $appConfig, private IInitialState $initialStateService, private GitHubAPIService $githubAPIService, ) { @@ -30,15 +30,15 @@ public function __construct( public function getForm(): TemplateResponse { $clientID = $this->secretService->getEncryptedAppValue('client_id'); $clientSecret = $this->secretService->getEncryptedAppValue('client_secret'); - $usePopup = $this->config->getAppValue(Application::APP_ID, 'use_popup', '0'); - $adminDashboardEnabled = $this->config->getAppValue(Application::APP_ID, 'dashboard_enabled', '1') === '1'; - $adminLinkPreviewEnabled = $this->config->getAppValue(Application::APP_ID, 'link_preview_enabled', '1') === '1'; + $usePopup = $this->appConfig->getAppValueString('use_popup', '0', lazy: true); + $adminDashboardEnabled = $this->appConfig->getAppValueString('dashboard_enabled', '1') === '1'; + $adminLinkPreviewEnabled = $this->appConfig->getAppValueString('link_preview_enabled', '1', lazy: true) === '1'; $defaultLinkToken = $this->secretService->getEncryptedAppValue('default_link_token'); - $userName = $this->config->getAppValue(Application::APP_ID, 'user_name'); - $userDisplayName = $this->config->getAppValue(Application::APP_ID, 'user_displayname'); - $allowDefaultTokenToAnonymous = $this->config->getAppValue(Application::APP_ID, 'allow_default_link_token_to_anonymous', '0') === '1'; - $allowDefaultTokenToGuests = $this->config->getAppValue(Application::APP_ID, 'allow_default_link_token_to_guests', '0') === '1'; - $issueNotificationsEnabled = $this->config->getAppValue(Application::APP_ID, 'issue_notifications_enabled', '1') === '1'; + $userName = $this->appConfig->getAppValueString('user_name', lazy: true); + $userDisplayName = $this->appConfig->getAppValueString('user_displayname', lazy: true); + $allowDefaultTokenToAnonymous = $this->appConfig->getAppValueString('allow_default_link_token_to_anonymous', '0', lazy: true) === '1'; + $allowDefaultTokenToGuests = $this->appConfig->getAppValueString('allow_default_link_token_to_guests', '0', lazy: true) === '1'; + $issueNotificationsEnabled = $this->appConfig->getAppValueString('issue_notifications_enabled', '1') === '1'; if ($defaultLinkToken !== '' && ($userName === '' || $userDisplayName === '')) { $userInfo = $this->githubAPIService->updateLinkTokenUserInfo($defaultLinkToken); diff --git a/lib/Settings/Personal.php b/lib/Settings/Personal.php index 88b373d2..6c37ffa6 100644 --- a/lib/Settings/Personal.php +++ b/lib/Settings/Personal.php @@ -9,6 +9,7 @@ use OCA\Github\AppInfo\Application; use OCA\Github\Service\SecretService; use OCP\AppFramework\Http\TemplateResponse; +use OCP\AppFramework\Services\IAppConfig; use OCP\AppFramework\Services\IInitialState; use OCP\IConfig; use OCP\Settings\ISettings; @@ -18,6 +19,7 @@ class Personal implements ISettings { public function __construct( private SecretService $secretService, private IConfig $config, + private IAppConfig $appConfig, private IInitialState $initialStateService, private ?string $userId, ) { @@ -35,12 +37,12 @@ public function getForm(): TemplateResponse { $userName = $this->config->getUserValue($this->userId, Application::APP_ID, 'user_name'); $userDisplayName = $this->config->getUserValue($this->userId, Application::APP_ID, 'user_displayname'); $notificationsEnabled = $this->config->getUserValue($this->userId, Application::APP_ID, 'issue_notifications_enabled', '0'); - $adminNotificationsEnabled = $this->config->getAppValue(Application::APP_ID, 'issue_notifications_enabled', '1'); + $adminNotificationsEnabled = $this->appConfig->getAppValueString('issue_notifications_enabled', '1'); // for OAuth $clientID = $this->secretService->getEncryptedAppValue('client_id'); $clientSecret = $this->secretService->getEncryptedAppValue('client_secret'); - $usePopup = $this->config->getAppValue(Application::APP_ID, 'use_popup', '0'); + $usePopup = $this->appConfig->getAppValueString('use_popup', '0', lazy: true); $userConfig = [ 'token' => $token === '' ? '' : 'dummyToken', diff --git a/tests/integration/MockedGithubApiTrait.php b/tests/integration/MockedGithubApiTrait.php index c86f5604..4ba2d75d 100644 --- a/tests/integration/MockedGithubApiTrait.php +++ b/tests/integration/MockedGithubApiTrait.php @@ -14,6 +14,7 @@ use OCA\Github\Reference\GithubIssuePrReferenceProvider; use OCA\Github\Service\GithubAPIService; use OCA\Github\Service\SecretService; +use OCP\AppFramework\Services\IAppConfig; use OCP\AppFramework\Services\IInitialState; use OCP\Http\Client\IClient; use OCP\Http\Client\IClientService; @@ -79,6 +80,7 @@ private function createGithubAPIService(): GithubAPIService { Server::get(LoggerInterface::class), $this->l10n, Server::get(IConfig::class), + $this->getAppConfig(), Server::get(IURLGenerator::class), Server::get(IUserManager::class), Server::get(INotificationManager::class), @@ -93,6 +95,7 @@ private function createConfigController(): ConfigController { Application::APP_ID, $this->createMock(IRequest::class), Server::get(IConfig::class), + $this->getAppConfig(), Server::get(IURLGenerator::class), $this->l10n, $this->createMock(IInitialState::class), @@ -104,6 +107,13 @@ private function createConfigController(): ConfigController { ); } + /** + * The app-scoped app config, which only the app's own container provides + */ + private function getAppConfig(): IAppConfig { + return Server::get(Application::class)->getContainer()->get(IAppConfig::class); + } + private function mockResponse(int $statusCode, string $body): IResponse&MockObject { $response = $this->createMock(IResponse::class); $response->method('getStatusCode')->willReturn($statusCode); @@ -117,26 +127,26 @@ private function mockResponse(int $statusCode, string $body): IResponse&MockObje */ private function useOAuthApp(string $clientId, string $clientSecret): void { $secretService = Server::get(SecretService::class); - $config = Server::get(IConfig::class); + $appConfig = $this->getAppConfig(); $this->previousAppValues = [ 'client_id' => $secretService->getEncryptedAppValue('client_id'), 'client_secret' => $secretService->getEncryptedAppValue('client_secret'), - 'use_popup' => $config->getAppValue(Application::APP_ID, 'use_popup'), + 'use_popup' => $appConfig->getAppValueString('use_popup', lazy: true), ]; $secretService->setEncryptedAppValue('client_id', $clientId); $secretService->setEncryptedAppValue('client_secret', $clientSecret); - $config->setAppValue(Application::APP_ID, 'use_popup', '0'); + $appConfig->setAppValueString('use_popup', '0', lazy: true); } private function restoreOAuthApp(): void { $secretService = Server::get(SecretService::class); - $config = Server::get(IConfig::class); + $appConfig = $this->getAppConfig(); foreach ($this->previousAppValues as $key => $value) { if ($value === '') { - $config->deleteAppValue(Application::APP_ID, $key); + $appConfig->deleteAppValue($key); } elseif ($key === 'use_popup') { - $config->setAppValue(Application::APP_ID, $key, $value); + $appConfig->setAppValueString($key, $value, lazy: true); } else { $secretService->setEncryptedAppValue($key, $value); } diff --git a/tests/unit/Controller/GithubAPIControllerTest.php b/tests/unit/Controller/GithubAPIControllerTest.php index 71923210..978190e3 100644 --- a/tests/unit/Controller/GithubAPIControllerTest.php +++ b/tests/unit/Controller/GithubAPIControllerTest.php @@ -4,6 +4,7 @@ use OCA\Github\Service\GithubAPIService; use OCA\Github\Service\SecretService; +use OCP\AppFramework\Services\IAppConfig; use OCP\Http\Client\IClient; use OCP\Http\Client\IClientService; use OCP\Http\Client\IResponse; @@ -60,6 +61,7 @@ protected function setUp(): void { \OC::$server->get(\Psr\Log\LoggerInterface::class), $this->createMock(IL10N::class), \OC::$server->get(IConfig::class), + $this->createMock(IAppConfig::class), \OC::$server->get(IURLGenerator::class), \OC::$server->get(IUserManager::class), \OC::$server->get(IManager::class), diff --git a/tests/unit/LazyAppConfigTest.php b/tests/unit/LazyAppConfigTest.php new file mode 100644 index 00000000..7f596687 --- /dev/null +++ b/tests/unit/LazyAppConfigTest.php @@ -0,0 +1,631 @@ + '', + 'client_secret' => '', + 'default_link_token' => '', + 'allow_default_link_token_to_anonymous' => '0', + 'allow_default_link_token_to_guests' => '0', + 'user_name' => '', + 'user_displayname' => '', + 'use_popup' => '0', + 'link_preview_enabled' => '1', + 'dashboard_enabled' => '1', + 'issue_notifications_enabled' => '1', + ]; + + /** @var array */ + private array $stored = []; + /** @var array> the lazy flag of each read, by key */ + private array $reads = []; + /** @var array> the lazy flag of each write, by key */ + private array $writes = []; + /** @var list */ + private array $deletes = []; + /** @var list the key and the default of each read */ + private array $readDefaults = []; + /** @var list the key and the sensitive flag of each write */ + private array $writeSensitive = []; + /** @var array */ + private array $userValues = []; + + private IAppConfig&MockObject $appConfig; + private IConfig&MockObject $config; + private IUserManager&MockObject $userManager; + private SecretService $secretService; + + protected function setUp(): void { + parent::setUp(); + + $this->appConfig = $this->createMock(IAppConfig::class); + $this->appConfig->method('getAppValueString') + ->willReturnCallback(function (string $key, string $default = '', bool $lazy = false): string { + // asserted in assertPostConditions(), where no catch in the code under test can swallow it + $this->readDefaults[] = [$key, $default]; + $this->reads[$key][] = $lazy; + return $this->stored[$key] ?? $default; + }); + $this->appConfig->method('setAppValueString') + ->willReturnCallback(function (string $key, string $value, bool $lazy = false, bool $sensitive = false): bool { + $this->writeSensitive[] = [$key, $sensitive]; + $this->writes[$key][] = $lazy; + $this->stored[$key] = $value; + return true; + }); + $this->appConfig->method('deleteAppValue') + ->willReturnCallback(function (string $key): void { + $this->deletes[] = $key; + unset($this->stored[$key]); + }); + // the untyped methods always read and write non-lazy + $this->appConfig->expects($this->never())->method('getAppValue'); + $this->appConfig->expects($this->never())->method('setAppValue'); + + $this->config = $this->createMock(IConfig::class); + $this->config->method('getUserValue')->willReturnCallback( + fn (string $userId, string $appName, string $key, mixed $default = ''): mixed => $this->userValues[$key] ?? $default + ); + // what the server returns for a lazy key read through the legacy API + $this->config->method('getAppValue')->willReturnCallback( + static fn (string $appName, string $key, mixed $default = ''): mixed => $default + ); + // a legacy write makes the key non-lazy again + $this->config->expects($this->never())->method('setAppValue'); + $this->config->expects($this->never())->method('deleteAppValue'); + $this->userManager = $this->createMock(IUserManager::class); + + $crypto = $this->createMock(ICrypto::class); + $crypto->method('encrypt')->willReturnCallback(static fn (string $value): string => 'encrypted:' . $value); + $crypto->method('decrypt')->willReturnCallback(static fn (string $value): string => substr($value, strlen('encrypted:'))); + + $this->secretService = new SecretService( + $this->config, + $this->appConfig, + $this->userManager, + $crypto, + $this->createMock(LoggerInterface::class), + ); + } + + /** + * Every read passes the default main passed, and no write marks a value sensitive. + */ + protected function assertPostConditions(): void { + foreach ($this->readDefaults as [$key, $default]) { + $this->assertArrayHasKey($key, self::DEFAULTS); + $this->assertSame(self::DEFAULTS[$key], $default, 'default of ' . $key); + } + foreach ($this->writeSensitive as [$key, $sensitive]) { + $this->assertFalse($sensitive, 'sensitive flag of ' . $key); + } + } + + public function testRegisterReadsTheDashboardAndNotificationTogglesNonLazy(): void { + $application = (new ReflectionClass(Application::class))->newInstanceWithoutConstructor(); + (new ReflectionProperty(Application::class, 'config'))->setValue($application, $this->config); + (new ReflectionProperty(Application::class, 'appConfig'))->setValue($application, $this->appConfig); + $context = $this->createMock(IRegistrationContext::class); + $context->expects($this->once())->method('registerDashboardWidget')->with(GithubWidget::class); + $context->expects($this->once())->method('registerNotifierService')->with(GitHubNotifier::class); + + $application->register($context); + + $this->assertEquals([ + 'dashboard_enabled' => [false], + 'issue_notifications_enabled' => [false], + ], $this->reads); + } + + public function testAdminTogglesAreStoredLazilyExceptTheOnesReadOnEveryRequest(): void { + $controller = $this->createConfigController($this->createMock(GithubAPIService::class)); + + $response = $controller->setAdminConfig([ + 'use_popup' => '1', + 'link_preview_enabled' => '0', + 'allow_default_link_token_to_anonymous' => '1', + 'allow_default_link_token_to_guests' => '1', + 'dashboard_enabled' => '0', + 'issue_notifications_enabled' => '0', + ]); + + $this->assertEquals([ + 'use_popup' => [true], + 'link_preview_enabled' => [true], + 'allow_default_link_token_to_anonymous' => [true], + 'allow_default_link_token_to_guests' => [true], + 'dashboard_enabled' => [false], + 'issue_notifications_enabled' => [false], + ], $this->writes); + $this->assertSame('0', $this->stored['dashboard_enabled']); + $this->assertSame(Http::STATUS_OK, $response->getStatus()); + $this->assertSame([], $response->getData()); + } + + public function testSensitiveAdminValuesAreStoredLazilyAndStillEncrypted(): void { + $githubAPIService = $this->createMock(GithubAPIService::class); + $githubAPIService->expects($this->once())->method('updateLinkTokenUserInfo')->with('token') + ->willReturn(['user_name' => 'octocat', 'user_displayname' => 'The Octocat']); + $controller = $this->createConfigController($githubAPIService); + + $response = $controller->setSensitiveAdminConfig([ + 'client_id' => 'id', + 'client_secret' => 'secret', + 'default_link_token' => 'token', + ]); + + $this->assertEquals([ + 'client_id' => [true], + 'client_secret' => [true], + 'default_link_token' => [true], + ], $this->writes); + $this->assertSame('encrypted:secret', $this->stored['client_secret']); + $this->assertSame('encrypted:token', $this->stored['default_link_token']); + $this->assertSame(Http::STATUS_OK, $response->getStatus()); + $this->assertSame(['user_name' => 'octocat', 'user_displayname' => 'The Octocat'], $response->getData()); + } + + /** + * A key the app does not own, such as the server's "enabled", must not reach the app config: + * stored lazily it would make the app look disabled. + */ + public function testAdminEndpointsRejectKeysTheyDoNotWriteAndWriteNothing(): void { + $githubAPIService = $this->createMock(GithubAPIService::class); + $githubAPIService->expects($this->never())->method('updateLinkTokenUserInfo'); + $controller = $this->createConfigController($githubAPIService); + + $responses = [ + $controller->setAdminConfig(['use_popup' => '1', 'enabled' => 'yes']), + $controller->setAdminConfig(['installed_version' => '99.0.0']), + $controller->setAdminConfig(['client_secret' => 'secret']), + $controller->setSensitiveAdminConfig(['client_id' => 'id', 'enabled' => 'yes']), + $controller->setSensitiveAdminConfig(['client_id' => 'id', 'use_popup' => '1']), + ]; + + foreach ($responses as $response) { + $this->assertSame(Http::STATUS_BAD_REQUEST, $response->getStatus()); + } + $this->assertSame([], $this->writes); + $this->assertSame([], $this->deletes); + } + + public function testEachAdminEndpointAcceptsExactlyItsOwnKeys(): void { + $controller = $this->createConfigController($this->createMock(GithubAPIService::class)); + $accepted = ['admin-config' => [], 'sensitive-admin-config' => []]; + + foreach ([...array_keys(self::DEFAULTS), 'enabled', 'installed_version', 'types', 'not_an_app_key'] as $key) { + if ($controller->setAdminConfig([$key => '1'])->getStatus() === Http::STATUS_OK) { + $accepted['admin-config'][] = $key; + } + if ($controller->setSensitiveAdminConfig([$key => '1'])->getStatus() === Http::STATUS_OK) { + $accepted['sensitive-admin-config'][] = $key; + } + } + + $this->assertEqualsCanonicalizing([ + 'use_popup', + 'link_preview_enabled', + 'allow_default_link_token_to_anonymous', + 'allow_default_link_token_to_guests', + 'dashboard_enabled', + 'issue_notifications_enabled', + ], $accepted['admin-config']); + $this->assertEqualsCanonicalizing(['client_id', 'client_secret', 'default_link_token'], $accepted['sensitive-admin-config']); + } + + public function testAdminEndpointsRejectValuesThatAreNotStringsAndWriteNothing(): void { + $controller = $this->createConfigController($this->createMock(GithubAPIService::class)); + + $responses = [ + $controller->setAdminConfig(['use_popup' => '1', 'link_preview_enabled' => true]), + $controller->setAdminConfig(['dashboard_enabled' => 0]), + $controller->setSensitiveAdminConfig(['client_id' => 'id', 'client_secret' => ['secret']]), + $controller->setSensitiveAdminConfig(['default_link_token' => null]), + ]; + + foreach ($responses as $response) { + $this->assertSame(Http::STATUS_BAD_REQUEST, $response->getStatus()); + } + $this->assertSame([], $this->writes); + $this->assertSame([], $this->deletes); + } + + public function testOauthRedirectReadsTheClientAndThePopupSettingLazily(): void { + $this->stored = [ + 'client_id' => 'encrypted:id', + 'client_secret' => 'encrypted:secret', + 'use_popup' => '1', + ]; + $this->userValues['oauth_state'] = 'state'; + $githubAPIService = $this->createMock(GithubAPIService::class); + $githubAPIService->expects($this->once())->method('requestOAuthAccessToken') + ->with(['client_id' => 'id', 'client_secret' => 'secret', 'code' => 'code', 'state' => 'state'], 'POST') + ->willReturn(['access_token' => 'access-token']); + $githubAPIService->method('request')->willReturn(['login' => 'octocat', 'id' => 1, 'name' => 'The Octocat']); + $urlGenerator = $this->createMock(IURLGenerator::class); + $urlGenerator->method('linkToRoute')->willReturnArgument(0); + $controller = $this->createConfigController($githubAPIService, $urlGenerator); + + $response = $controller->oauthRedirect('code', 'state'); + + $this->assertSame('integration_github.config.popupSuccessPage', $response->getRedirectURL()); + $this->assertEquals([ + 'client_id' => [true], + 'client_secret' => [true], + 'use_popup' => [true], + ], $this->reads); + } + + /** + * @return array, array}> + */ + public static function adminFormProvider(): array { + return [ + 'every value stored, each differing from its default' => [ + [ + 'client_id' => 'encrypted:id', + 'client_secret' => 'encrypted:secret', + 'use_popup' => '1', + 'dashboard_enabled' => '0', + 'link_preview_enabled' => '0', + 'default_link_token' => 'encrypted:token', + 'user_name' => 'octocat', + 'user_displayname' => 'The Octocat', + 'allow_default_link_token_to_anonymous' => '1', + 'allow_default_link_token_to_guests' => '1', + 'issue_notifications_enabled' => '0', + ], + [ + 'client_id' => 'id', + 'client_secret' => 'dummyClientSecret', + 'use_popup' => true, + 'dashboard_enabled' => false, + 'link_preview_enabled' => false, + 'default_link_token' => 'dummyToken', + 'allow_default_link_token_to_anonymous' => true, + 'allow_default_link_token_to_guests' => true, + 'user_name' => 'octocat', + 'user_displayname' => 'The Octocat', + 'issue_notifications_enabled' => false, + ], + ], + 'mixed values, some stored and some left to their default' => [ + [ + 'client_id' => 'encrypted:other-id', + 'use_popup' => '1', + 'link_preview_enabled' => '1', + 'user_name' => 'hubot', + 'allow_default_link_token_to_anonymous' => '1', + 'allow_default_link_token_to_guests' => '0', + 'issue_notifications_enabled' => '0', + ], + [ + 'client_id' => 'other-id', + 'client_secret' => '', + 'use_popup' => true, + 'dashboard_enabled' => true, + 'link_preview_enabled' => true, + 'default_link_token' => '', + 'allow_default_link_token_to_anonymous' => true, + 'allow_default_link_token_to_guests' => false, + 'user_name' => 'hubot', + 'user_displayname' => '', + 'issue_notifications_enabled' => false, + ], + ], + 'other mixed values' => [ + [ + 'client_secret' => 'encrypted:secret', + 'use_popup' => '1', + 'dashboard_enabled' => '1', + 'link_preview_enabled' => '0', + 'user_displayname' => 'Hubot', + 'allow_default_link_token_to_guests' => '1', + ], + [ + 'client_id' => '', + 'client_secret' => 'dummyClientSecret', + 'use_popup' => true, + 'dashboard_enabled' => true, + 'link_preview_enabled' => false, + 'default_link_token' => '', + 'allow_default_link_token_to_anonymous' => false, + 'allow_default_link_token_to_guests' => true, + 'user_name' => '', + 'user_displayname' => 'Hubot', + 'issue_notifications_enabled' => true, + ], + ], + 'nothing stored, so the defaults decide' => [ + [], + [ + 'client_id' => '', + 'client_secret' => '', + 'use_popup' => false, + 'dashboard_enabled' => true, + 'link_preview_enabled' => true, + 'default_link_token' => '', + 'allow_default_link_token_to_anonymous' => false, + 'allow_default_link_token_to_guests' => false, + 'user_name' => '', + 'user_displayname' => '', + 'issue_notifications_enabled' => true, + ], + ], + ]; + } + + /** + * @param array $stored + * @param array $expectedForm + */ + #[DataProvider('adminFormProvider')] + public function testAdminSettingsReadEachKeyWithItsFlag(array $stored, array $expectedForm): void { + $this->stored = $stored; + $githubAPIService = $this->createMock(GithubAPIService::class); + $githubAPIService->expects($this->never())->method('updateLinkTokenUserInfo'); + $initialState = $this->createMock(IInitialState::class); + $initialState->expects($this->once())->method('provideInitialState') + ->with('admin-config', $this->identicalTo($expectedForm)); + $admin = new Admin($this->secretService, $this->appConfig, $initialState, $githubAPIService); + + $admin->getForm(); + + $this->assertEquals([ + 'client_id' => [true], + 'client_secret' => [true], + 'use_popup' => [true], + 'dashboard_enabled' => [false], + 'link_preview_enabled' => [true], + 'default_link_token' => [true], + 'user_name' => [true], + 'user_displayname' => [true], + 'allow_default_link_token_to_anonymous' => [true], + 'allow_default_link_token_to_guests' => [true], + 'issue_notifications_enabled' => [false], + ], $this->reads); + } + + /** + * @return array}> + */ + public static function missingAccountProvider(): array { + // the default token switches are on here, a value pattern no other form data set has + $switches = ['allow_default_link_token_to_anonymous' => '1', 'allow_default_link_token_to_guests' => '1']; + return [ + 'user name missing' => [['default_link_token' => 'encrypted:token', 'user_displayname' => 'Stale', ...$switches]], + 'display name missing' => [['default_link_token' => 'encrypted:token', 'user_name' => 'stale', ...$switches]], + ]; + } + + /** + * @param array $stored + */ + #[DataProvider('missingAccountProvider')] + public function testAdminSettingsFetchTheDefaultTokenAccountWhenItIsMissing(array $stored): void { + $this->stored = $stored; + $githubAPIService = $this->createMock(GithubAPIService::class); + $githubAPIService->expects($this->once())->method('updateLinkTokenUserInfo')->with('token') + ->willReturn(['user_name' => 'octocat', 'user_displayname' => 'The Octocat']); + $initialState = $this->createMock(IInitialState::class); + $initialState->expects($this->once())->method('provideInitialState') + ->with('admin-config', $this->identicalTo([ + 'client_id' => '', + 'client_secret' => '', + 'use_popup' => false, + 'dashboard_enabled' => true, + 'link_preview_enabled' => true, + 'default_link_token' => 'dummyToken', + 'allow_default_link_token_to_anonymous' => true, + 'allow_default_link_token_to_guests' => true, + 'user_name' => 'octocat', + 'user_displayname' => 'The Octocat', + 'issue_notifications_enabled' => true, + ])); + + (new Admin($this->secretService, $this->appConfig, $initialState, $githubAPIService))->getForm(); + + $this->assertSame([true], $this->reads['user_name']); + $this->assertSame([true], $this->reads['user_displayname']); + } + + public function testPersonalSettingsReadEachKeyWithItsFlag(): void { + $personal = new Personal($this->secretService, $this->config, $this->appConfig, $this->createMock(IInitialState::class), 'alice'); + + $personal->getForm(); + + $this->assertEquals([ + 'issue_notifications_enabled' => [false], + 'client_id' => [true], + 'client_secret' => [true], + 'use_popup' => [true], + ], $this->reads); + } + + public function testDashboardWidgetReadsTheClientAndThePopupSettingLazily(): void { + $widget = new GithubWidget( + $this->createMock(IL10N::class), + $this->appConfig, + $this->createMock(IURLGenerator::class), + $this->createMock(IInitialState::class), + $this->createMock(GithubAPIService::class), + 'alice', + ); + + $widget->load(); + + $this->assertEquals([ + 'client_id' => [true], + 'client_secret' => [true], + 'use_popup' => [true], + ], $this->reads); + } + + public function testReferenceProvidersReadTheLinkPreviewSettingLazily(): void { + $this->stored['link_preview_enabled'] = '0'; + $arguments = [ + $this->createMock(GithubAPIService::class), + $this->config, + $this->appConfig, + $this->createMock(IL10N::class), + $this->createMock(IURLGenerator::class), + $this->createMock(ReferenceManager::class), + null, + ]; + $issueProvider = new GithubIssuePrReferenceProvider(...$arguments); + $codeProvider = new GithubCodeReferenceProvider(...$arguments); + + $this->assertFalse($issueProvider->matchReference('https://github.com/nextcloud/server/issues/1')); + $this->assertFalse($codeProvider->matchReference('https://github.com/nextcloud/server/blob/0123abc/index.php#L1')); + + $this->assertEquals(['link_preview_enabled' => [true, true]], $this->reads); + } + + public function testAccessTokenFallbackReadsItsSettingsLazily(): void { + $this->stored = [ + 'allow_default_link_token_to_anonymous' => '1', + 'allow_default_link_token_to_guests' => '1', + 'default_link_token' => 'encrypted:token', + ]; + $guest = $this->createMock(IUser::class); + $guest->method('getBackendClassName')->willReturn('Guests'); + $this->userManager->method('get')->with('guest')->willReturn($guest); + + $this->assertSame('token', $this->secretService->getAccessToken(null, true)); + $this->assertSame('token', $this->secretService->getAccessToken('guest', true)); + + $this->assertEquals([ + 'allow_default_link_token_to_anonymous' => [true], + 'allow_default_link_token_to_guests' => [true], + 'default_link_token' => [true, true], + ], $this->reads); + } + + public function testClearingASecretKeepsItLazy(): void { + $this->secretService->setEncryptedAppValue('client_secret', ''); + + $this->assertEquals(['client_secret' => [true]], $this->writes); + $this->assertSame('', $this->stored['client_secret']); + } + + public function testDefaultTokenUserInfoIsStoredLazilyAndDeletedThroughTheAppConfig(): void { + $response = $this->createMock(IResponse::class); + $response->method('getStatusCode')->willReturn(200); + $response->method('getBody')->willReturnOnConsecutiveCalls('{"login":"octocat","name":"The Octocat"}', '{}'); + $client = $this->createMock(IClient::class); + $client->method('get')->willReturn($response); + $githubAPIService = $this->createGithubAPIService($client); + + $this->assertSame( + ['user_name' => 'octocat', 'user_displayname' => 'The Octocat'], + $githubAPIService->updateLinkTokenUserInfo('token'), + ); + $this->assertEquals([ + 'user_name' => [true], + 'user_displayname' => [true], + ], $this->writes); + $this->assertSame('octocat', $this->stored['user_name']); + $this->assertSame('The Octocat', $this->stored['user_displayname']); + + $this->assertSame( + ['user_name' => '', 'user_displayname' => ''], + $githubAPIService->updateLinkTokenUserInfo('token'), + ); + $this->assertSame(['user_name', 'user_displayname'], $this->deletes); + } + + public function testNotificationJobReadsTheAdminToggleNonLazy(): void { + $this->stored['issue_notifications_enabled'] = '0'; + $this->userManager->expects($this->never())->method('callForAllUsers'); + + $this->createGithubAPIService($this->createMock(IClient::class))->checkNewNotifications(); + + $this->assertEquals(['issue_notifications_enabled' => [false]], $this->reads); + } + + private function createConfigController(GithubAPIService $githubAPIService, ?IURLGenerator $urlGenerator = null): ConfigController { + return new ConfigController( + Application::APP_ID, + $this->createMock(IRequest::class), + $this->config, + $this->appConfig, + $urlGenerator ?? $this->createMock(IURLGenerator::class), + $this->createMock(IL10N::class), + $this->createMock(IInitialState::class), + $this->createMock(LoggerInterface::class), + $githubAPIService, + $this->secretService, + $this->createMock(GithubIssuePrReferenceProvider::class), + 'alice', + ); + } + + private function createGithubAPIService(IClient $client): GithubAPIService { + $clientService = $this->createMock(IClientService::class); + $clientService->method('newClient')->willReturn($client); + + return new GithubAPIService( + $this->secretService, + $this->createMock(LoggerInterface::class), + $this->createMock(IL10N::class), + $this->config, + $this->appConfig, + $this->createMock(IURLGenerator::class), + $this->userManager, + $this->createMock(IManager::class), + $clientService, + ); + } +} diff --git a/tests/unit/Migration/Version030010Date20241002015812Test.php b/tests/unit/Migration/Version030010Date20241002015812Test.php new file mode 100644 index 00000000..c0f24ddd --- /dev/null +++ b/tests/unit/Migration/Version030010Date20241002015812Test.php @@ -0,0 +1,65 @@ + 'id', 'client_secret' => 'secret']; + $appConfig = $this->createMock(IAppConfig::class); + $appConfig->method('getValueString') + ->willReturnCallback(static function (string $app, string $key, string $default = '', bool $lazy = false) use (&$reads, $stored): string { + $reads[$key][] = [$app, $default, $lazy]; + return $stored[$key] ?? $default; + }); + $appConfig->method('setValueString') + ->willReturnCallback(static function (string $app, string $key, string $value, bool $lazy = false, bool $sensitive = false) use (&$writes): bool { + $writes[] = [$app, $key, $value, $lazy, $sensitive]; + return true; + }); + $crypto = $this->createMock(ICrypto::class); + $crypto->method('encrypt')->willReturnCallback(static fn (string $value): string => 'encrypted:' . $value); + + // no user token to encrypt + $queryBuilder = $this->createMock(IQueryBuilder::class); + foreach (['update', 'set', 'where', 'andWhere', 'select', 'from'] as $method) { + $queryBuilder->method($method)->willReturnSelf(); + } + $queryBuilder->method('expr')->willReturn($this->createMock(IExpressionBuilder::class)); + $connection = $this->createMock(IDBConnection::class); + $connection->method('getQueryBuilder')->willReturn($queryBuilder); + + $migration = new Version030010Date20241002015812($connection, $crypto, $appConfig); + $migration->postSchemaChange($this->createMock(IOutput::class), static fn () => null, []); + + $this->assertSame([ + 'client_id' => [[Application::APP_ID, '', true]], + 'client_secret' => [[Application::APP_ID, '', true]], + 'default_link_token' => [[Application::APP_ID, '', true]], + ], $reads); + $this->assertSame([ + [Application::APP_ID, 'client_id', 'encrypted:id', true, false], + [Application::APP_ID, 'client_secret', 'encrypted:secret', true, false], + ], $writes); + } +} diff --git a/tests/unit/Migration/Version030301Date20261021102507Test.php b/tests/unit/Migration/Version030301Date20261021102507Test.php new file mode 100644 index 00000000..752397fb --- /dev/null +++ b/tests/unit/Migration/Version030301Date20261021102507Test.php @@ -0,0 +1,45 @@ +createMock(IAppConfig::class); + $appConfig->method('updateLazy') + ->willReturnCallback(static function (string $app, string $key, bool $lazy) use (&$marked): bool { + $marked[] = [$app, $key, $lazy]; + return true; + }); + + $migration = new Version030301Date20261021102507($appConfig); + $migration->postSchemaChange($this->createMock(IOutput::class), static fn () => null, []); + + // dashboard_enabled and issue_notifications_enabled are read on every request and stay non-lazy + $this->assertEqualsCanonicalizing([ + [Application::APP_ID, 'client_id', true], + [Application::APP_ID, 'client_secret', true], + [Application::APP_ID, 'default_link_token', true], + [Application::APP_ID, 'use_popup', true], + [Application::APP_ID, 'link_preview_enabled', true], + [Application::APP_ID, 'allow_default_link_token_to_anonymous', true], + [Application::APP_ID, 'allow_default_link_token_to_guests', true], + [Application::APP_ID, 'user_name', true], + [Application::APP_ID, 'user_displayname', true], + ], $marked); + } +} diff --git a/tests/unit/Service/SecretServiceTest.php b/tests/unit/Service/SecretServiceTest.php index a84f045d..b46566f1 100644 --- a/tests/unit/Service/SecretServiceTest.php +++ b/tests/unit/Service/SecretServiceTest.php @@ -10,6 +10,7 @@ use Exception; use OCA\Github\AppInfo\Application; use OCA\Github\Service\SecretService; +use OCP\AppFramework\Services\IAppConfig; use OCP\IConfig; use OCP\IUserManager; use OCP\Security\ICrypto; @@ -20,6 +21,7 @@ class SecretServiceTest extends TestCase { private IConfig|MockObject $config; + private IAppConfig|MockObject $appConfig; private IUserManager|MockObject $userManager; private ICrypto|MockObject $crypto; private LoggerInterface|MockObject $logger; @@ -28,11 +30,19 @@ class SecretServiceTest extends TestCase { protected function setUp(): void { parent::setUp(); $this->config = $this->createMock(IConfig::class); + // what the server returns for a lazy key read through the legacy API + $this->config->method('getAppValue')->willReturnCallback( + static fn (string $appName, string $key, mixed $default = ''): mixed => $default + ); + // a legacy write makes the key non-lazy again + $this->config->expects($this->never())->method('setAppValue'); + $this->config->expects($this->never())->method('deleteAppValue'); + $this->appConfig = $this->createMock(IAppConfig::class); $this->userManager = $this->createMock(IUserManager::class); $this->crypto = $this->createMock(ICrypto::class); $this->logger = $this->createMock(LoggerInterface::class); $this->secretService = new SecretService( - $this->config, $this->userManager, $this->crypto, $this->logger + $this->config, $this->appConfig, $this->userManager, $this->crypto, $this->logger ); } @@ -44,7 +54,7 @@ public function testUserValueIsDecrypted(): void { } public function testAppValueIsDecrypted(): void { - $this->config->method('getAppValue')->willReturn('ciphertext'); + $this->appConfig->method('getAppValueString')->willReturn('ciphertext'); $this->crypto->expects($this->once())->method('decrypt')->with('ciphertext')->willReturn('plain'); $this->assertSame('plain', $this->secretService->getEncryptedAppValue('client_id')); @@ -52,7 +62,7 @@ public function testAppValueIsDecrypted(): void { public function testUnsetValuesAreNotDecrypted(): void { $this->config->method('getUserValue')->willReturn(''); - $this->config->method('getAppValue')->willReturn(''); + $this->appConfig->method('getAppValueString')->willReturn(''); $this->crypto->expects($this->never())->method('decrypt'); $this->assertSame('', $this->secretService->getEncryptedUserValue('alice', 'token')); @@ -75,7 +85,7 @@ public function testUndecryptableUserValueIsTreatedAsUnset(): void { } public function testUndecryptableAppValueIsTreatedAsUnset(): void { - $this->config->method('getAppValue')->willReturn('not-actually-ciphertext'); + $this->appConfig->method('getAppValueString')->willReturn('not-actually-ciphertext'); $this->crypto->method('decrypt') ->willThrowException(new Exception('Authenticated ciphertext could not be decoded.')); $this->logger->expects($this->once())->method('warning'); @@ -89,7 +99,7 @@ public function testUndecryptableAppValueIsTreatedAsUnset(): void { */ public function testAccessTokenIsEmptyWhenTheStoredTokenCannotBeDecrypted(): void { $this->config->method('getUserValue')->willReturn('not-actually-ciphertext'); - $this->config->method('getAppValue')->willReturn(''); + $this->appConfig->method('getAppValueString')->willReturn(''); $this->crypto->method('decrypt') ->willThrowException(new Exception('Authenticated ciphertext could not be decoded.'));