Skip to content

Commit 232970e

Browse files
joshtrichardsjoshtrichards
committed
fix(updater): Stop expiring secret prematurely
Signed-off-by: Josh Richards <[email protected]>
1 parent e17f011 commit 232970e

4 files changed

Lines changed: 126 additions & 25 deletions

File tree

‎apps/updatenotification/lib/BackgroundJob/ResetToken.php‎

Lines changed: 11 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@
1212
use OCP\BackgroundJob\TimedJob;
1313
use OCP\IAppConfig;
1414
use OCP\IConfig;
15+
use Psr\Log\LoggerInterface;
1516

1617
/**
1718
* Deletes the updater secret after if it is older than 48h
@@ -26,24 +27,30 @@ public function __construct(
2627
ITimeFactory $time,
2728
private IConfig $config,
2829
private IAppConfig $appConfig,
30+
private LoggerInterface $logger,
2931
) {
3032
parent::__construct($time);
31-
// Run all 10 minutes
32-
parent::setInterval(60 * 10);
33+
// Run once an hour
34+
parent::setInterval(60 * 60);
3335
}
3436

3537
/**
3638
* @param $argument
3739
*/
3840
protected function run($argument) {
3941
if ($this->config->getSystemValueBool('config_is_read_only')) {
42+
$this->logger->debug('Skipping `updatar.secret` reset since config_is_read_only is set', ['app' => 'updatenotification']);
4043
return;
4144
}
4245

4346
$secretCreated = $this->appConfig->getValueInt('core', 'updater.secret.created', $this->time->getTime());
4447
// Delete old tokens after 2 days
45-
if ($secretCreated >= 172800) {
48+
$secretCreatedDiff = $this->time->getTime() - $secretCreated;
49+
if ($secretCreatedDiff >= 172800) {
4650
$this->config->deleteSystemValue('updater.secret');
47-
}
51+
$this->appConfig->deleteKey('core', 'updater.secret.created');
52+
$this->logger->warning('Cleared old `updater.secret` that was created ' . $secretCreatedDiff . ' seconds ago', ['app' => 'updatenotification']);
53+
} else {
54+
$this->logger->debug('Keeping existing `updater.secret` that was created ' . $secretCreatedDiff . ' seconds ago', ['app' => 'updatenotification']);
4855
}
4956
}

‎apps/updatenotification/lib/Controller/AdminController.php‎

Lines changed: 9 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@
2020
use OCP\IRequest;
2121
use OCP\Security\ISecureRandom;
2222
use OCP\Util;
23+
use Psr\Log\LoggerInterface;
2324

2425
class AdminController extends Controller {
2526

@@ -32,14 +33,11 @@ public function __construct(
3233
private IAppConfig $appConfig,
3334
private ITimeFactory $timeFactory,
3435
private IL10N $l10n,
36+
private LoggerInterface $logger,
3537
) {
3638
parent::__construct($appName, $request);
3739
}
3840

39-
private function isUpdaterEnabled() {
40-
return !$this->config->getSystemValue('upgrade.disable-web', false);
41-
}
42-
4341
/**
4442
* @param string $channel
4543
* @return DataResponse
@@ -54,10 +52,14 @@ public function setChannel(string $channel): DataResponse {
5452
* @return DataResponse
5553
*/
5654
public function createCredentials(): DataResponse {
57-
if (!$this->isUpdaterEnabled()) {
55+
if ($this->config->getSystemValueBool('upgrade.disable-web')) {
5856
return new DataResponse(['status' => 'error', 'message' => $this->l10n->t('Web updater is disabled')], Http::STATUS_FORBIDDEN);
5957
}
6058

59+
if ($this->config->getSystemValueBool('config_is_read_only')) {
60+
return new DataResponse(['status' => 'error', 'message' => $this->l10n->t('Configuration is read-only')], Http::STATUS_FORBIDDEN);
61+
}
62+
6163
// Create a new job and store the creation date
6264
$this->jobList->add(ResetToken::class);
6365
$this->appConfig->setValueInt('core', 'updater.secret.created', $this->timeFactory->getTime());
@@ -66,6 +68,8 @@ public function createCredentials(): DataResponse {
6668
$newToken = $this->secureRandom->generate(64);
6769
$this->config->setSystemValue('updater.secret', password_hash($newToken, PASSWORD_DEFAULT));
6870

71+
$this->logger->warning('Created new `updater.secret`', ['app' => 'updatenotification']);
72+
6973
return new DataResponse($newToken);
7074
}
7175
}

‎apps/updatenotification/tests/BackgroundJob/ResetTokenTest.php‎

Lines changed: 54 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -13,35 +13,40 @@
1313
use OCP\IAppConfig;
1414
use OCP\IConfig;
1515
use PHPUnit\Framework\MockObject\MockObject;
16+
use Psr\Log\LoggerInterface;
1617
use Test\TestCase;
1718

1819
class ResetTokenTest extends TestCase {
20+
private ITimeFactory|MockObject $timeFactory;
1921
private IConfig|MockObject $config;
2022
private IAppConfig|MockObject $appConfig;
21-
private ITimeFactory|MockObject $timeFactory;
23+
private LoggerInterface|MockObject $logger;
2224
private BackgroundJobResetToken $resetTokenBackgroundJob;
2325

2426
protected function setUp(): void {
2527
parent::setUp();
26-
$this->appConfig = $this->createMock(IAppConfig::class);
27-
$this->config = $this->createMock(IConfig::class);
2828
$this->timeFactory = $this->createMock(ITimeFactory::class);
29+
$this->config = $this->createMock(IConfig::class);
30+
$this->appConfig = $this->createMock(IAppConfig::class);
31+
$this->logger = $this->createMock(LoggerInterface::class);
2932
$this->resetTokenBackgroundJob = new BackgroundJobResetToken(
3033
$this->timeFactory,
3134
$this->config,
3235
$this->appConfig,
36+
$this->logger,
3337
);
3438
}
3539

36-
public function testRunWithNotExpiredToken(): void {
37-
$this->timeFactory
40+
public function testRunWithNotExpiredToken(): void { // Affirm if updater.secret.created <48 hours ago then `updater.secret` is left alone
41+
$this->timeFactory
3842
->expects($this->atLeastOnce())
3943
->method('getTime')
40-
->willReturn(123);
41-
$this->appConfig
44+
->willReturn(1733069649); // "Sun, 01 Dec 2024 16:14:09 +0000"
45+
$this->appConfig
4246
->expects($this->once())
4347
->method('getValueInt')
44-
->with('core', 'updater.secret.created', 123);
48+
->with('core', 'updater.secret.created', 1733069649)
49+
->willReturn(1733069649 - 1 * 24 * 60 * 60); // 24h prior: "Sat, 30 Nov 2024 16:14:09 +0000"
4550
$this->config
4651
->expects($this->once())
4752
->method('getSystemValueBool')
@@ -50,29 +55,53 @@ public function testRunWithNotExpiredToken(): void {
5055
$this->config
5156
->expects($this->never())
5257
->method('deleteSystemValue');
58+
$this->appConfig
59+
->expects($this->never())
60+
->method('deleteKey');
61+
$this->logger
62+
->expects($this->never())
63+
->method('warning');
64+
$this->logger
65+
->expects($this->once())
66+
->method('debug');
5367

54-
static::invokePrivate($this->resetTokenBackgroundJob, 'run', [null]);
68+
$this->invokePrivate($this->resetTokenBackgroundJob, 'run', [null]);
5569
}
5670

57-
public function testRunWithExpiredToken(): void {
71+
public function testRunWithExpiredToken(): void { // Affirm if updater.secret.created >48 hours ago then `updater.secret` is removed
5872
$this->timeFactory
59-
->expects($this->once())
73+
->expects($this->atLeastOnce())
6074
->method('getTime')
61-
->willReturn(1455045234);
75+
->willReturn(1455045234); // "Tue, 09 Feb 2016 19:13:54 +0000"
6276
$this->appConfig
6377
->expects($this->once())
6478
->method('getValueInt')
6579
->with('core', 'updater.secret.created', 1455045234)
66-
->willReturn(2 * 24 * 60 * 60 + 1); // over 2 days
80+
->willReturn(1455045234 - 3 * 24 * 60 * 60); // 72h prior: "Sat, 06 Feb 2016 19:13:54 +0000"
81+
$this->config
82+
->expects($this->once())
83+
->method('getSystemValueBool')
84+
->with('config_is_read_only')
85+
->willReturn(false);
6786
$this->config
6887
->expects($this->once())
6988
->method('deleteSystemValue')
7089
->with('updater.secret');
90+
$this->appConfig
91+
->expects($this->once())
92+
->method('deleteKey')
93+
->with('core', 'updater.secret.created');
94+
$this->logger
95+
->expects($this->once())
96+
->method('warning');
97+
$this->logger
98+
->expects($this->never())
99+
->method('debug');
71100

72-
static::invokePrivate($this->resetTokenBackgroundJob, 'run', [null]);
101+
$this->invokePrivate($this->resetTokenBackgroundJob, 'run', [null]);
73102
}
74103

75-
public function testRunWithExpiredTokenAndReadOnlyConfigFile(): void {
104+
public function testRunWithExpiredTokenAndReadOnlyConfigFile(): void { // Affirm if config_is_read_only is set that the secret is never reset
76105
$this->timeFactory
77106
->expects($this->never())
78107
->method('getTime');
@@ -87,7 +116,16 @@ public function testRunWithExpiredTokenAndReadOnlyConfigFile(): void {
87116
$this->config
88117
->expects($this->never())
89118
->method('deleteSystemValue');
119+
$this->appConfig
120+
->expects($this->never())
121+
->method('deleteKey');
122+
$this->logger
123+
->expects($this->never())
124+
->method('warning');
125+
$this->logger
126+
->expects($this->once())
127+
->method('debug');
90128

91-
static::invokePrivate($this->resetTokenBackgroundJob, 'run', [null]);
129+
$this->invokePrivate($this->resetTokenBackgroundJob, 'run', [null]);
92130
}
93131
}

‎apps/updatenotification/tests/Controller/AdminControllerTest.php‎

Lines changed: 52 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -81,4 +81,56 @@ public function testCreateCredentials(): void {
8181
$expected = new DataResponse('MyGeneratedToken');
8282
$this->assertEquals($expected, $this->adminController->createCredentials());
8383
}
84+
85+
public function testCreateCredentialsAndWebUpdaterDisabled(): void {
86+
$this->config
87+
->expects($this->once())
88+
->method('getSystemValue')
89+
->with('upgrade.disable-web')
90+
->willReturn(true);
91+
$this->jobList
92+
->expects($this->never())
93+
->method('add')
94+
->with(ResetToken::class);
95+
$this->secureRandom
96+
->expects($this->never())
97+
->method('generate');
98+
$this->config
99+
->expects($this->never())
100+
->method('setSystemValue');
101+
$this->timeFactory
102+
->expects($this->never())
103+
->method('getTime');
104+
$this->appConfig
105+
->expects($this->never())
106+
->method('setValueInt');
107+
108+
$this->adminController->createCredentials();
109+
}
110+
111+
public function testCreateCredentialsAndReadOnlyConfigFile(): void {
112+
$this->config
113+
->expects($this->once())
114+
->method('getSystemValue')
115+
->with('config_is_read_only')
116+
->willReturn(true);
117+
$this->jobList
118+
->expects($this->never())
119+
->method('add')
120+
->with(ResetToken::class);
121+
$this->secureRandom
122+
->expects($this->never())
123+
->method('generate');
124+
$this->config
125+
->expects($this->never())
126+
->method('setSystemValue');
127+
$this->timeFactory
128+
->expects($this->never())
129+
->method('getTime');
130+
$this->appConfig
131+
->expects($this->never())
132+
->method('setValueInt');
133+
134+
$this->adminController->createCredentials();
135+
}
84136
}

0 commit comments

Comments
 (0)