fix(updater): Stop expiring secret prematurely

Signed-off-by: Josh Richards <[email protected]>
nextcloud · Dec 1, 2024 · 89de931 · 89de931
1 parent e17f011
commit 89de931
Show file tree

Hide file tree

Showing 4 changed files with 129 additions and 25 deletions.
diff --git 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,24 +27,31 @@ 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);
 	}
 
 	/**
 	 * @param $argument
 	 */
 	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
@@ -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
@@ -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
@@ -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('getSystemValue')
+			->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('getSystemValue')
+			->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();
+	}
 }