Merge pull request #49578 from nextcloud/fix-updater-secret

fix(updater): Stop expiring secret prematurely

apps/updatenotification/lib/BackgroundJob/ResetToken.php

@@ -12,6 +12,7 @@ use OCP\AppFramework\Utility\ITimeFactory;
 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 @@ class ResetToken extends TimedJob {
 		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 @@ class ResetToken extends TimedJob {
 	 */
 	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 = $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']);
 		}
 	}
 }

apps/updatenotification/lib/Controller/AdminController.php

@@ -21,6 +21,7 @@ use OCP\IL10N;
 use OCP\IRequest;
 use OCP\Security\ISecureRandom;
 use OCP\Util;
+use Psr\Log\LoggerInterface;
 
 class AdminController extends Controller {
 
@@ -33,14 +34,11 @@ class AdminController extends Controller {
 		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,10 +53,14 @@ class AdminController extends Controller {
 	 * @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());
@@ -67,6 +69,8 @@ class AdminController extends Controller {
 		$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);
 	}
 }

apps/updatenotification/tests/BackgroundJob/ResetTokenTest.php

@@ -13,35 +13,44 @@ use OCP\AppFramework\Utility\ITimeFactory;
 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 @@ class ResetTokenTest extends TestCase {
 		$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 @@ class ResetTokenTest extends TestCase {
 		$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]);
 	}
 }

apps/updatenotification/tests/Controller/AdminControllerTest.php

@@ -19,18 +19,20 @@ use OCP\IL10N;
 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 @@ class AdminControllerTest extends TestCase {
 		$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 @@ class AdminControllerTest extends TestCase {
 			$this->config,
 			$this->appConfig,
 			$this->timeFactory,
-			$this->l10n
+			$this->l10n,
+			$this->logger,
 		);
 	}
 
@@ -81,4 +85,29 @@ class AdminControllerTest extends TestCase {
 		$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();
+	}
 }