diff --git a/apps/updatenotification/lib/BackgroundJob/ResetToken.php b/apps/updatenotification/lib/BackgroundJob/ResetToken.php index 35543ce5247db..b2bbbf6f9605d 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 `updatar.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) { + $secretCreatedDiff = $this->time->getTime() - $secretCreated; + if ($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 6e7f9935d938c..ab2ab0d4b0dc7 100644 --- a/apps/updatenotification/lib/Controller/AdminController.php +++ b/apps/updatenotification/lib/Controller/AdminController.php @@ -20,6 +20,7 @@ use OCP\IRequest; use OCP\Security\ISecureRandom; use OCP\Util; +use Psr\Log\LoggerInterface; class AdminController extends Controller { @@ -32,14 +33,11 @@ public function __construct( private IAppConfig $appConfig, private ITimeFactory $timeFactory, private IL10N $l10n, + private LoggerInterface $logger, ) { parent::__construct($appName, $request); } - private function isUpdaterEnabled() { - return !$this->config->getSystemValue('upgrade.disable-web', false); - } - /** * @param string $channel * @return DataResponse @@ -54,10 +52,14 @@ 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); } + 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 job and store the creation date $this->jobList->add(ResetToken::class); $this->appConfig->setValueInt('core', 'updater.secret.created', $this->timeFactory->getTime()); @@ -66,6 +68,8 @@ public function createCredentials(): DataResponse { $newToken = $this->secureRandom->generate(64); $this->config->setSystemValue('updater.secret', password_hash($newToken, PASSWORD_DEFAULT)); + $this->logger->warning('Created new `updater.secret`', ['app' => 'updatenotification']); + return new DataResponse($newToken); } } diff --git a/apps/updatenotification/tests/BackgroundJob/ResetTokenTest.php b/apps/updatenotification/tests/BackgroundJob/ResetTokenTest.php index 1b0fe0cbc5964..811c57b0d3c8f 100644 --- a/apps/updatenotification/tests/BackgroundJob/ResetTokenTest.php +++ b/apps/updatenotification/tests/BackgroundJob/ResetTokenTest.php @@ -13,35 +13,40 @@ use OCP\IAppConfig; use OCP\IConfig; use PHPUnit\Framework\MockObject\MockObject; +use Psr\Log\LoggerInterface; use Test\TestCase; class ResetTokenTest extends TestCase { + private ITimeFactory|MockObject $timeFactory; private IConfig|MockObject $config; private IAppConfig|MockObject $appConfig; - private ITimeFactory|MockObject $timeFactory; + private LoggerInterface|MockObject $logger; private BackgroundJobResetToken $resetTokenBackgroundJob; 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 { - $this->timeFactory + public function testRunWithNotExpiredToken(): void { // Affirm if updater.secret.created <48 hours ago then `updater.secret` is left alone + $this->timeFactory ->expects($this->atLeastOnce()) ->method('getTime') - ->willReturn(123); - $this->appConfig + ->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', 1733069649) + ->willReturn(1733069649 - 1 * 24 * 60 * 60); // 24h prior: "Sat, 30 Nov 2024 16:14:09 +0000" $this->config ->expects($this->once()) ->method('getSystemValueBool') @@ -50,29 +55,53 @@ 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]); + $this->invokePrivate($this->resetTokenBackgroundJob, 'run', [null]); } - public function testRunWithExpiredToken(): void { + public function testRunWithExpiredToken(): void { // Affirm if updater.secret.created >48 hours ago then `updater.secret` is removed $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 + ->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'); @@ -87,7 +116,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 877a027637300..fe039b2ff4686 100644 --- a/apps/updatenotification/tests/Controller/AdminControllerTest.php +++ b/apps/updatenotification/tests/Controller/AdminControllerTest.php @@ -19,6 +19,7 @@ use OCP\IRequest; use OCP\Security\ISecureRandom; use PHPUnit\Framework\MockObject\MockObject; +use Psr\Log\LoggerInterface; use Test\TestCase; class AdminControllerTest extends TestCase { @@ -29,6 +30,7 @@ class AdminControllerTest extends TestCase { private ITimeFactory|MockObject $timeFactory; private IL10N|MockObject $l10n; private IAppConfig|MockObject $appConfig; + private LoggerInterface|MockObject $logger; private AdminController $adminController; @@ -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,54 @@ 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(); + } + + public function testCreateCredentialsAndReadOnlyConfigFile(): void { + $this->config + ->expects($this->once()) + ->method('getSystemValueBool') + ->with('config_is_read_only') + ->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(); + } }