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.'));