diff --git a/apps/updatenotification/lib/BackgroundJob/ResetToken.php b/apps/updatenotification/lib/BackgroundJob/ResetToken.php index 35543ce5247db..2ae43b53fb0b9 100644 --- a/apps/updatenotification/lib/BackgroundJob/ResetToken.php +++ b/apps/updatenotification/lib/BackgroundJob/ResetToken.php @@ -12,6 +12,7 @@ use OCP\BackgroundJob\TimedJob; use OCP\IAppConfig; use OCP\IConfig; +use Psr\Log\LoggerInterface; /** * Deletes the updater secret after if it is older than 48h @@ -26,10 +27,11 @@ public function __construct( ITimeFactory $time, private IConfig $config, private IAppConfig $appConfig, + private LoggerInterface $logger, ) { parent::__construct($time); - // Run all 10 minutes - parent::setInterval(60 * 10); + // Run once an hour + parent::setInterval(60 * 60); } /** @@ -37,13 +39,19 @@ public function __construct( */ protected function run($argument) { if ($this->config->getSystemValueBool('config_is_read_only')) { + $this->logger->debug('Skipping `updater.secret` reset since config_is_read_only is set', ['app' => 'updatenotification']); return; } - $secretCreated = $this->appConfig->getValueInt('core', 'updater.secret.created', $this->time->getTime()); - // Delete old tokens after 2 days - if ($secretCreated >= 172800) { + $secretCreated = $this->appConfig->getValueInt('core', 'updater.secret.created'); + // Delete old tokens after 2 days and also tokens without any created date + $secretCreatedDiff = $secretCreated - $this->time->getTime(); + if (abs($secretCreatedDiff) > 172800) { $this->config->deleteSystemValue('updater.secret'); + $this->appConfig->deleteKey('core', 'updater.secret.created'); + $this->logger->warning('Cleared old `updater.secret` that was created ' . $secretCreatedDiff . ' seconds ago', ['app' => 'updatenotification']); + } else { + $this->logger->debug('Keeping existing `updater.secret` that was created ' . $secretCreatedDiff . ' seconds ago', ['app' => 'updatenotification']); } } } diff --git a/apps/updatenotification/lib/Controller/AdminController.php b/apps/updatenotification/lib/Controller/AdminController.php index 267459488903a..5a4828a742ac2 100644 --- a/apps/updatenotification/lib/Controller/AdminController.php +++ b/apps/updatenotification/lib/Controller/AdminController.php @@ -21,6 +21,7 @@ use OCP\IRequest; use OCP\Security\ISecureRandom; use OCP\Util; +use Psr\Log\LoggerInterface; class AdminController extends Controller { @@ -33,14 +34,11 @@ public function __construct( private IAppConfig $appConfig, private ITimeFactory $timeFactory, private IL10N $l10n, + private LoggerInterface $logger, ) { parent::__construct($appName, $request); } - private function isUpdaterEnabled(): bool { - return !$this->config->getSystemValueBool('upgrade.disable-web'); - } - /** * @param string $channel * @return DataResponse @@ -55,18 +53,24 @@ public function setChannel(string $channel): DataResponse { * @return DataResponse */ public function createCredentials(): DataResponse { - if (!$this->isUpdaterEnabled()) { + if ($this->config->getSystemValueBool('upgrade.disable-web')) { return new DataResponse(['status' => 'error', 'message' => $this->l10n->t('Web updater is disabled')], Http::STATUS_FORBIDDEN); } - // Create a new job and store the creation date - $this->jobList->add(ResetToken::class); - $this->appConfig->setValueInt('core', 'updater.secret.created', $this->timeFactory->getTime()); + if ($this->config->getSystemValueBool('config_is_read_only')) { + return new DataResponse(['status' => 'error', 'message' => $this->l10n->t('Configuration is read-only')], Http::STATUS_FORBIDDEN); + } // Create a new token $newToken = $this->secureRandom->generate(64); $this->config->setSystemValue('updater.secret', password_hash($newToken, PASSWORD_DEFAULT)); + // Create a new job and store the creation date + $this->jobList->add(ResetToken::class); + $this->appConfig->setValueInt('core', 'updater.secret.created', $this->timeFactory->getTime()); + + $this->logger->warning('Created new `updater.secret` with token: ' . $newToken, ['app' => 'updatenotification']); + return new DataResponse($newToken); } } diff --git a/apps/updatenotification/tests/BackgroundJob/ResetTokenTest.php b/apps/updatenotification/tests/BackgroundJob/ResetTokenTest.php index c03d4c4882715..3845ee05be7bd 100644 --- a/apps/updatenotification/tests/BackgroundJob/ResetTokenTest.php +++ b/apps/updatenotification/tests/BackgroundJob/ResetTokenTest.php @@ -13,35 +13,44 @@ use OCP\IAppConfig; use OCP\IConfig; use PHPUnit\Framework\MockObject\MockObject; +use Psr\Log\LoggerInterface; use Test\TestCase; class ResetTokenTest extends TestCase { - private IConfig&MockObject $config; - private IAppConfig&MockObject $appConfig; - private ITimeFactory&MockObject $timeFactory; - private BackgroundJobResetToken $resetTokenBackgroundJob; + protected BackgroundJobResetToken $resetTokenBackgroundJob; + + protected IConfig&MockObject $config; + protected IAppConfig&MockObject $appConfig; + protected ITimeFactory&MockObject $timeFactory; + protected LoggerInterface&MockObject $logger; protected function setUp(): void { parent::setUp(); - $this->appConfig = $this->createMock(IAppConfig::class); - $this->config = $this->createMock(IConfig::class); $this->timeFactory = $this->createMock(ITimeFactory::class); + $this->config = $this->createMock(IConfig::class); + $this->appConfig = $this->createMock(IAppConfig::class); + $this->logger = $this->createMock(LoggerInterface::class); $this->resetTokenBackgroundJob = new BackgroundJobResetToken( $this->timeFactory, $this->config, $this->appConfig, + $this->logger, ); } - public function testRunWithNotExpiredToken(): void { + /** + * Affirm if updater.secret.created <48 hours ago then `updater.secret` is left alone. + */ + public function testKeepSecretWhenCreatedRecently(): void { $this->timeFactory ->expects($this->atLeastOnce()) ->method('getTime') - ->willReturn(123); + ->willReturn(1733069649); // "Sun, 01 Dec 2024 16:14:09 +0000" $this->appConfig ->expects($this->once()) ->method('getValueInt') - ->with('core', 'updater.secret.created', 123); + ->with('core', 'updater.secret.created') + ->willReturn(1733069649 - 1 * 24 * 60 * 60); // 24h prior: "Sat, 30 Nov 2024 16:14:09 +0000" $this->config ->expects($this->once()) ->method('getSystemValueBool') @@ -50,32 +59,59 @@ public function testRunWithNotExpiredToken(): void { $this->config ->expects($this->never()) ->method('deleteSystemValue'); + $this->appConfig + ->expects($this->never()) + ->method('deleteKey'); + $this->logger + ->expects($this->never()) + ->method('warning'); + $this->logger + ->expects($this->once()) + ->method('debug'); static::invokePrivate($this->resetTokenBackgroundJob, 'run', [null]); } - public function testRunWithExpiredToken(): void { + /** + * Affirm if updater.secret.created >48 hours ago then `updater.secret` is removed + */ + public function testSecretIsRemovedWhenOutdated(): void { $this->timeFactory - ->expects($this->once()) + ->expects($this->atLeastOnce()) ->method('getTime') - ->willReturn(1455045234); + ->willReturn(1455045234); // "Tue, 09 Feb 2016 19:13:54 +0000" $this->appConfig ->expects($this->once()) ->method('getValueInt') - ->with('core', 'updater.secret.created', 1455045234) - ->willReturn(2 * 24 * 60 * 60 + 1); // over 2 days + ->with('core', 'updater.secret.created') + ->willReturn(1455045234 - 3 * 24 * 60 * 60); // 72h prior: "Sat, 06 Feb 2016 19:13:54 +0000" + $this->config + ->expects($this->once()) + ->method('getSystemValueBool') + ->with('config_is_read_only') + ->willReturn(false); $this->config ->expects($this->once()) ->method('deleteSystemValue') ->with('updater.secret'); + $this->appConfig + ->expects($this->once()) + ->method('deleteKey') + ->with('core', 'updater.secret.created'); + $this->logger + ->expects($this->once()) + ->method('warning'); + $this->logger + ->expects($this->never()) + ->method('debug'); - static::invokePrivate($this->resetTokenBackgroundJob, 'run', [null]); + $this->invokePrivate($this->resetTokenBackgroundJob, 'run', [null]); } - public function testRunWithExpiredTokenAndReadOnlyConfigFile(): void { + public function testRunWithExpiredTokenAndReadOnlyConfigFile(): void { // Affirm if config_is_read_only is set that the secret is never reset $this->timeFactory - ->expects($this->never()) - ->method('getTime'); + ->expects($this->never()) + ->method('getTime'); $this->appConfig ->expects($this->never()) ->method('getValueInt'); @@ -87,7 +123,16 @@ public function testRunWithExpiredTokenAndReadOnlyConfigFile(): void { $this->config ->expects($this->never()) ->method('deleteSystemValue'); + $this->appConfig + ->expects($this->never()) + ->method('deleteKey'); + $this->logger + ->expects($this->never()) + ->method('warning'); + $this->logger + ->expects($this->once()) + ->method('debug'); - static::invokePrivate($this->resetTokenBackgroundJob, 'run', [null]); + $this->invokePrivate($this->resetTokenBackgroundJob, 'run', [null]); } } diff --git a/apps/updatenotification/tests/Controller/AdminControllerTest.php b/apps/updatenotification/tests/Controller/AdminControllerTest.php index 2263495fc148e..566d8cfa14637 100644 --- a/apps/updatenotification/tests/Controller/AdminControllerTest.php +++ b/apps/updatenotification/tests/Controller/AdminControllerTest.php @@ -19,18 +19,20 @@ use OCP\IRequest; use OCP\Security\ISecureRandom; use PHPUnit\Framework\MockObject\MockObject; +use Psr\Log\LoggerInterface; use Test\TestCase; class AdminControllerTest extends TestCase { - private IRequest&MockObject $request; - private IJobList&MockObject $jobList; - private ISecureRandom&MockObject $secureRandom; - private IConfig&MockObject $config; - private ITimeFactory&MockObject $timeFactory; - private IL10N&MockObject $l10n; - private IAppConfig&MockObject $appConfig; + protected IRequest&MockObject $request; + protected IJobList&MockObject $jobList; + protected ISecureRandom&MockObject $secureRandom; + protected IConfig&MockObject $config; + protected ITimeFactory&MockObject $timeFactory; + protected IL10N&MockObject $l10n; + protected IAppConfig&MockObject $appConfig; + protected LoggerInterface&MockObject $logger; - private AdminController $adminController; + protected AdminController $adminController; protected function setUp(): void { parent::setUp(); @@ -42,6 +44,7 @@ protected function setUp(): void { $this->appConfig = $this->createMock(IAppConfig::class); $this->timeFactory = $this->createMock(ITimeFactory::class); $this->l10n = $this->createMock(IL10N::class); + $this->logger = $this->createMock(LoggerInterface::class); $this->adminController = new AdminController( 'updatenotification', @@ -51,7 +54,8 @@ protected function setUp(): void { $this->config, $this->appConfig, $this->timeFactory, - $this->l10n + $this->l10n, + $this->logger, ); } @@ -81,4 +85,29 @@ public function testCreateCredentials(): void { $expected = new DataResponse('MyGeneratedToken'); $this->assertEquals($expected, $this->adminController->createCredentials()); } + + public function testCreateCredentialsAndWebUpdaterDisabled(): void { + $this->config + ->expects($this->once()) + ->method('getSystemValueBool') + ->with('upgrade.disable-web') + ->willReturn(true); + $this->jobList + ->expects($this->never()) + ->method('add'); + $this->secureRandom + ->expects($this->never()) + ->method('generate'); + $this->config + ->expects($this->never()) + ->method('setSystemValue'); + $this->timeFactory + ->expects($this->never()) + ->method('getTime'); + $this->appConfig + ->expects($this->never()) + ->method('setValueInt'); + + $this->adminController->createCredentials(); + } }