Skip to content

Commit b77688a

Browse files
joshtrichardsjoshtrichards
committed
fix(updater): Stop expiring secret prematurely
Signed-off-by: Josh Richards <josh.t.richards@gmail.com>
1 parent e17f011 commit b77688a

4 files changed

Lines changed: 102 additions & 23 deletions

File tree

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

Lines changed: 11 additions & 3 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,31 @@ 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');
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']);
4755
}
4856
}
4957
}

‎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: 52 additions & 14 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 {
40+
public function testRunWithNotExpiredToken(): void { // Affirm if updater.secret.created <48 hours ago then `updater.secret` is left alone
3741
$this->timeFactory
3842
->expects($this->atLeastOnce())
3943
->method('getTime')
40-
->willReturn(123);
44+
->willReturn(1733069649); // "Sun, 01 Dec 2024 16:14:09 +0000"
4145
$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: 30 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@
1919
use OCP\IRequest;
2020
use OCP\Security\ISecureRandom;
2121
use PHPUnit\Framework\MockObject\MockObject;
22+
use Psr\Log\LoggerInterface;
2223
use Test\TestCase;
2324

2425
class AdminControllerTest extends TestCase {
@@ -29,6 +30,7 @@ class AdminControllerTest extends TestCase {
2930
private ITimeFactory|MockObject $timeFactory;
3031
private IL10N|MockObject $l10n;
3132
private IAppConfig|MockObject $appConfig;
33+
private LoggerInterface|MockObject $logger;
3234

3335
private AdminController $adminController;
3436

@@ -42,6 +44,7 @@ protected function setUp(): void {
4244
$this->appConfig = $this->createMock(IAppConfig::class);
4345
$this->timeFactory = $this->createMock(ITimeFactory::class);
4446
$this->l10n = $this->createMock(IL10N::class);
47+
$this->logger = $this->createMock(LoggerInterface::class);
4548

4649
$this->adminController = new AdminController(
4750
'updatenotification',
@@ -51,7 +54,8 @@ protected function setUp(): void {
5154
$this->config,
5255
$this->appConfig,
5356
$this->timeFactory,
54-
$this->l10n
57+
$this->l10n,
58+
$this->logger,
5559
);
5660
}
5761

@@ -81,4 +85,29 @@ public function testCreateCredentials(): void {
8185
$expected = new DataResponse('MyGeneratedToken');
8286
$this->assertEquals($expected, $this->adminController->createCredentials());
8387
}
88+
89+
public function testCreateCredentialsAndWebUpdaterDisabled(): void {
90+
$this->config
91+
->expects($this->once())
92+
->method('getSystemValueBool')
93+
->with('upgrade.disable-web')
94+
->willReturn(true);
95+
$this->jobList
96+
->expects($this->never())
97+
->method('add');
98+
$this->secureRandom
99+
->expects($this->never())
100+
->method('generate');
101+
$this->config
102+
->expects($this->never())
103+
->method('setSystemValue');
104+
$this->timeFactory
105+
->expects($this->never())
106+
->method('getTime');
107+
$this->appConfig
108+
->expects($this->never())
109+
->method('setValueInt');
110+
111+
$this->adminController->createCredentials();
112+
}
84113
}

0 commit comments

Comments
 (0)