Skip to content

Commit 7dd9f40

Browse files
authored
Merge pull request #61889 from nextcloud/kano-fix-ocm-duplicate-sharedSecret
CloudFederationApi: access-token lifecycle fixes
2 parents 4935c60 + 58cf6c7 commit 7dd9f40

12 files changed

Lines changed: 351 additions & 46 deletions

File tree

apps/cloud_federation_api/composer/composer/autoload_classmap.php

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,8 +15,10 @@
1515
'OCA\\CloudFederationAPI\\Controller\\TokenController' => $baseDir . '/../lib/Controller/TokenController.php',
1616
'OCA\\CloudFederationAPI\\Db\\OcmTokenMap' => $baseDir . '/../lib/Db/OcmTokenMap.php',
1717
'OCA\\CloudFederationAPI\\Db\\OcmTokenMapMapper' => $baseDir . '/../lib/Db/OcmTokenMapMapper.php',
18+
'OCA\\CloudFederationAPI\\Listener\\ShareDeletedListener' => $baseDir . '/../lib/Listener/ShareDeletedListener.php',
1819
'OCA\\CloudFederationAPI\\Migration\\DropFederatedInvitesTable' => $baseDir . '/../lib/Migration/DropFederatedInvitesTable.php',
1920
'OCA\\CloudFederationAPI\\Migration\\Version1016Date202502262004' => $baseDir . '/../lib/Migration/Version1016Date202502262004.php',
2021
'OCA\\CloudFederationAPI\\Migration\\Version1017Date20260306120000' => $baseDir . '/../lib/Migration/Version1017Date20260306120000.php',
2122
'OCA\\CloudFederationAPI\\ResponseDefinitions' => $baseDir . '/../lib/ResponseDefinitions.php',
23+
'OCA\\CloudFederationAPI\\Service\\OcmTokenService' => $baseDir . '/../lib/Service/OcmTokenService.php',
2224
);

apps/cloud_federation_api/composer/composer/autoload_static.php

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -30,10 +30,12 @@ class ComposerStaticInitCloudFederationAPI
3030
'OCA\\CloudFederationAPI\\Controller\\TokenController' => __DIR__ . '/..' . '/../lib/Controller/TokenController.php',
3131
'OCA\\CloudFederationAPI\\Db\\OcmTokenMap' => __DIR__ . '/..' . '/../lib/Db/OcmTokenMap.php',
3232
'OCA\\CloudFederationAPI\\Db\\OcmTokenMapMapper' => __DIR__ . '/..' . '/../lib/Db/OcmTokenMapMapper.php',
33+
'OCA\\CloudFederationAPI\\Listener\\ShareDeletedListener' => __DIR__ . '/..' . '/../lib/Listener/ShareDeletedListener.php',
3334
'OCA\\CloudFederationAPI\\Migration\\DropFederatedInvitesTable' => __DIR__ . '/..' . '/../lib/Migration/DropFederatedInvitesTable.php',
3435
'OCA\\CloudFederationAPI\\Migration\\Version1016Date202502262004' => __DIR__ . '/..' . '/../lib/Migration/Version1016Date202502262004.php',
3536
'OCA\\CloudFederationAPI\\Migration\\Version1017Date20260306120000' => __DIR__ . '/..' . '/../lib/Migration/Version1017Date20260306120000.php',
3637
'OCA\\CloudFederationAPI\\ResponseDefinitions' => __DIR__ . '/..' . '/../lib/ResponseDefinitions.php',
38+
'OCA\\CloudFederationAPI\\Service\\OcmTokenService' => __DIR__ . '/..' . '/../lib/Service/OcmTokenService.php',
3739
);
3840

3941
public static function getInitializer(ClassLoader $loader)

apps/cloud_federation_api/lib/AppInfo/Application.php

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,10 +9,12 @@
99

1010
namespace OCA\CloudFederationAPI\AppInfo;
1111

12+
use OCA\CloudFederationAPI\Listener\ShareDeletedListener;
1213
use OCP\AppFramework\App;
1314
use OCP\AppFramework\Bootstrap\IBootContext;
1415
use OCP\AppFramework\Bootstrap\IBootstrap;
1516
use OCP\AppFramework\Bootstrap\IRegistrationContext;
17+
use OCP\Share\Events\ShareDeletedEvent;
1618

1719
class Application extends App implements IBootstrap {
1820
public const APP_ID = 'cloud_federation_api';
@@ -23,6 +25,7 @@ public function __construct() {
2325

2426
#[\Override]
2527
public function register(IRegistrationContext $context): void {
28+
$context->registerEventListener(ShareDeletedEvent::class, ShareDeletedListener::class);
2629
}
2730

2831
#[\Override]

apps/cloud_federation_api/lib/BackgroundJob/CleanupExpiredOcmTokensJob.php

Lines changed: 8 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -9,20 +9,22 @@
99

1010
namespace OCA\CloudFederationAPI\BackgroundJob;
1111

12-
use OCA\CloudFederationAPI\Db\OcmTokenMapMapper;
12+
use OCA\CloudFederationAPI\Service\OcmTokenService;
1313
use OCP\AppFramework\Utility\ITimeFactory;
1414
use OCP\BackgroundJob\TimedJob;
1515

1616
/**
17-
* Periodically purge expired OCM access token mappings from ocm_token_map.
17+
* Periodically purge expired OCM access tokens.
1818
*
19-
* The corresponding oc_authtoken entries (TEMPORARY_TOKEN with an expires
20-
* timestamp) are cleaned up by Nextcloud's own token expiry jobs.
19+
* Each expired mapping has its access token deleted from oc_authtoken before
20+
* its ocm_token_map row is removed. Dropping the mapping first would orphan
21+
* the access token, since nothing else records which oc_authtoken id belongs
22+
* to a given refresh token.
2123
*/
2224
class CleanupExpiredOcmTokensJob extends TimedJob {
2325
public function __construct(
2426
ITimeFactory $timeFactory,
25-
private readonly OcmTokenMapMapper $mapper,
27+
private readonly OcmTokenService $tokenService,
2628
) {
2729
parent::__construct($timeFactory);
2830

@@ -32,6 +34,6 @@ public function __construct(
3234

3335
#[\Override]
3436
protected function run($argument): void {
35-
$this->mapper->deleteExpired($this->time->getTime());
37+
$this->tokenService->revokeExpired($this->time->getTime());
3638
}
3739
}

apps/cloud_federation_api/lib/Controller/TokenController.php

Lines changed: 4 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -226,20 +226,10 @@ public function accessToken(string $grant_type = '', string $code = ''): DataRes
226226
$this->tokenProvider->updateToken($token);
227227
}
228228

229-
// Revoke the previous access token for this refresh token, if any.
230-
$existingMapping = $this->ocmTokenMapMapper->findByRefreshToken($refreshToken);
231-
if ($existingMapping !== null) {
232-
try {
233-
$this->tokenProvider->invalidateTokenById(
234-
$token->getUID(),
235-
$existingMapping->getAccessTokenId()
236-
);
237-
} catch (\Exception) {
238-
// Token may already be gone; ignore.
239-
}
240-
$this->ocmTokenMapMapper->delete($existingMapping);
241-
}
242-
229+
// A refresh token may back several concurrent access tokens (e.g. the
230+
// webdav mount and the webapp launcher exchange the same secret), so
231+
// each exchange issues a fresh one and leaves the others in place;
232+
// expiry and unshare cleanup revoke them.
243233
$share = $this->shareManager->getShareByToken($refreshToken);
244234
// access_token TTL from the refresh-token scope; default 3600, clamped 300..86400.
245235
$ttl = (int)($token->getScopeAsArray()['ocm_access_token_ttl'] ?? 3600);

apps/cloud_federation_api/lib/Db/OcmTokenMapMapper.php

Lines changed: 17 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -34,25 +34,32 @@ public function getByAccessTokenId(int $accessTokenId): OcmTokenMap {
3434
}
3535

3636
/**
37-
* Find the current mapping for a given refresh token, if any.
37+
* All mappings for a refresh token. Returns every match, since a refresh
38+
* token can back several access tokens and concurrent exchanges may add
39+
* duplicate rows.
40+
*
41+
* @return OcmTokenMap[]
3842
*/
39-
public function findByRefreshToken(string $refreshToken): ?OcmTokenMap {
43+
public function findAllByRefreshToken(string $refreshToken): array {
4044
$qb = $this->db->getQueryBuilder();
4145
$qb->select('*')
4246
->from($this->getTableName())
4347
->where($qb->expr()->eq('refresh_token', $qb->createNamedParameter($refreshToken)));
4448

45-
try {
46-
return $this->findEntity($qb);
47-
} catch (DoesNotExistException) {
48-
return null;
49-
}
49+
return $this->findEntities($qb);
5050
}
5151

52-
public function deleteExpired(int $time): void {
52+
/**
53+
* All mappings whose access token has expired before $time.
54+
*
55+
* @return OcmTokenMap[]
56+
*/
57+
public function findExpired(int $time): array {
5358
$qb = $this->db->getQueryBuilder();
54-
$qb->delete($this->getTableName())
59+
$qb->select('*')
60+
->from($this->getTableName())
5561
->where($qb->expr()->lt('expires', $qb->createNamedParameter($time)));
56-
$qb->executeStatement();
62+
63+
return $this->findEntities($qb);
5764
}
5865
}
Lines changed: 56 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,56 @@
1+
<?php
2+
3+
declare(strict_types=1);
4+
5+
/**
6+
* SPDX-FileCopyrightText: 2026 Nextcloud GmbH and Nextcloud contributors
7+
* SPDX-License-Identifier: AGPL-3.0-or-later
8+
*/
9+
10+
namespace OCA\CloudFederationAPI\Listener;
11+
12+
use OC\Authentication\Token\IProvider;
13+
use OCA\CloudFederationAPI\Service\OcmTokenService;
14+
use OCP\EventDispatcher\Event;
15+
use OCP\EventDispatcher\IEventListener;
16+
use OCP\Share\Events\ShareDeletedEvent;
17+
use OCP\Share\IShare;
18+
19+
/**
20+
* When a federated share is removed, revoke the OCM access tokens it minted
21+
* and invalidate its refresh token immediately, instead of waiting up to six
22+
* hours for the expiry job — which cannot even find them once the share, and
23+
* with it the mapping's context, is gone.
24+
*
25+
* @template-implements IEventListener<ShareDeletedEvent>
26+
*/
27+
class ShareDeletedListener implements IEventListener {
28+
public function __construct(
29+
private readonly OcmTokenService $tokenService,
30+
private readonly IProvider $tokenProvider,
31+
) {
32+
}
33+
34+
#[\Override]
35+
public function handle(Event $event): void {
36+
if (!$event instanceof ShareDeletedEvent) {
37+
return;
38+
}
39+
40+
$share = $event->getShare();
41+
if (!in_array($share->getShareType(), [IShare::TYPE_REMOTE, IShare::TYPE_REMOTE_GROUP], true)) {
42+
return;
43+
}
44+
45+
$refreshToken = $share->getToken();
46+
if ($refreshToken === null || $refreshToken === '') {
47+
return;
48+
}
49+
50+
// Revoke the access tokens exchanged from this share's secret...
51+
$this->tokenService->revokeByRefreshToken($refreshToken);
52+
// ...and the refresh (permanent) token itself. invalidateToken is a
53+
// no-op when the token is already gone.
54+
$this->tokenProvider->invalidateToken($refreshToken);
55+
}
56+
}
Lines changed: 70 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,70 @@
1+
<?php
2+
3+
declare(strict_types=1);
4+
5+
/**
6+
* SPDX-FileCopyrightText: 2026 Nextcloud GmbH and Nextcloud contributors
7+
* SPDX-License-Identifier: AGPL-3.0-or-later
8+
*/
9+
10+
namespace OCA\CloudFederationAPI\Service;
11+
12+
use OC\Authentication\Exceptions\ExpiredTokenException;
13+
use OC\Authentication\Exceptions\InvalidTokenException;
14+
use OC\Authentication\Exceptions\WipeTokenException;
15+
use OC\Authentication\Token\IProvider;
16+
use OCA\CloudFederationAPI\Db\OcmTokenMap;
17+
use OCA\CloudFederationAPI\Db\OcmTokenMapMapper;
18+
19+
/**
20+
* Revokes OCM access tokens together with their ocm_token_map rows, so a
21+
* removed or expired mapping never leaves an orphaned oc_authtoken entry.
22+
*/
23+
class OcmTokenService {
24+
public function __construct(
25+
private readonly OcmTokenMapMapper $mapper,
26+
private readonly IProvider $tokenProvider,
27+
) {
28+
}
29+
30+
/**
31+
* Revoke every access token whose mapping expired before $time.
32+
*/
33+
public function revokeExpired(int $time): void {
34+
foreach ($this->mapper->findExpired($time) as $mapping) {
35+
$this->revokeMapping($mapping);
36+
}
37+
}
38+
39+
/**
40+
* Revoke every access token issued for the given refresh token. Tolerates
41+
* the duplicate rows a concurrent exchange can leave behind.
42+
*/
43+
public function revokeByRefreshToken(string $refreshToken): void {
44+
foreach ($this->mapper->findAllByRefreshToken($refreshToken) as $mapping) {
45+
$this->revokeMapping($mapping);
46+
}
47+
}
48+
49+
private function revokeMapping(OcmTokenMap $mapping): void {
50+
$this->revokeAccessToken($mapping->getAccessTokenId());
51+
$this->mapper->delete($mapping);
52+
}
53+
54+
/**
55+
* Delete the access token from oc_authtoken. getTokenById throws for an
56+
* expired token but still carries it, so the owner uid required by
57+
* invalidateTokenById is recoverable.
58+
*/
59+
private function revokeAccessToken(int $accessTokenId): void {
60+
try {
61+
$token = $this->tokenProvider->getTokenById($accessTokenId);
62+
} catch (ExpiredTokenException|WipeTokenException $e) {
63+
$token = $e->getToken();
64+
} catch (InvalidTokenException) {
65+
// Access token already gone; nothing left to revoke.
66+
return;
67+
}
68+
$this->tokenProvider->invalidateTokenById($token->getUID(), $accessTokenId);
69+
}
70+
}

apps/cloud_federation_api/tests/BackgroundJob/CleanupExpiredOcmTokensJobTest.php

Lines changed: 8 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -10,35 +10,31 @@
1010
namespace OCA\CloudFederationAPI\Tests\BackgroundJob;
1111

1212
use OCA\CloudFederationAPI\BackgroundJob\CleanupExpiredOcmTokensJob;
13-
use OCA\CloudFederationAPI\Db\OcmTokenMapMapper;
13+
use OCA\CloudFederationAPI\Service\OcmTokenService;
1414
use OCP\AppFramework\Utility\ITimeFactory;
1515
use PHPUnit\Framework\MockObject\MockObject;
1616
use Test\TestCase;
1717

1818
class CleanupExpiredOcmTokensJobTest extends TestCase {
1919
private ITimeFactory&MockObject $timeFactory;
20-
private OcmTokenMapMapper&MockObject $mapper;
20+
private OcmTokenService&MockObject $tokenService;
2121
private CleanupExpiredOcmTokensJob $job;
2222

2323
#[\Override]
2424
protected function setUp(): void {
2525
parent::setUp();
2626

2727
$this->timeFactory = $this->createMock(ITimeFactory::class);
28-
$this->mapper = $this->createMock(OcmTokenMapMapper::class);
28+
$this->tokenService = $this->createMock(OcmTokenService::class);
2929

30-
$this->job = new CleanupExpiredOcmTokensJob($this->timeFactory, $this->mapper);
30+
$this->job = new CleanupExpiredOcmTokensJob($this->timeFactory, $this->tokenService);
3131
}
3232

33-
public function testRunDeletesExpiredTokens(): void {
33+
public function testRunRevokesExpiredAtCurrentTime(): void {
3434
$now = 1700000000;
35-
$this->timeFactory->expects($this->once())
36-
->method('getTime')
37-
->willReturn($now);
38-
39-
$this->mapper->expects($this->once())
40-
->method('deleteExpired')
41-
->with($now);
35+
$this->timeFactory->method('getTime')->willReturn($now);
36+
$this->tokenService->expects($this->once())
37+
->method('revokeExpired')->with($now);
4238

4339
$method = new \ReflectionMethod(CleanupExpiredOcmTokensJob::class, 'run');
4440
$method->invoke($this->job, []);

apps/cloud_federation_api/tests/Controller/TokenControllerTest.php

Lines changed: 20 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -125,10 +125,6 @@ private function configureHappyPath(
125125
->with($refreshToken)
126126
->willReturn($refreshTokenMock);
127127

128-
$this->ocmTokenMapMapper->method('findByRefreshToken')
129-
->with($refreshToken)
130-
->willReturn(null);
131-
132128
$share = $this->createMock(IShare::class);
133129
$share->method('getShareOwner')->willReturn($shareOwner);
134130
$share->method('getSharedWith')->willReturn($sharedWith);
@@ -196,6 +192,26 @@ public function testAccessTokenSuccess(): void {
196192
$this->assertSame(1000000 + 3600, $decoded->exp);
197193
}
198194

195+
public function testAccessTokenDoesNotRevokeExistingTokens(): void {
196+
$signedRequest = $this->createMock(IIncomingSignedRequest::class);
197+
$signedRequest->method('getOrigin')->willReturn('remote.example.com');
198+
$this->signatureManager->method('getIncomingSignedRequest')
199+
->with($this->signatoryManager)
200+
->willReturn($signedRequest);
201+
202+
$this->configureHappyPath('valid-refresh-token', 123, 'testuser', 'owner', 'sharee@remote.example.com', 'fixedjtivalue00');
203+
204+
// A refresh token may back multiple concurrent access tokens, so an
205+
// exchange only adds one and never revokes or deletes an existing one.
206+
$this->tokenProvider->expects($this->never())->method('invalidateTokenById');
207+
$this->ocmTokenMapMapper->expects($this->never())->method('delete');
208+
$this->ocmTokenMapMapper->expects($this->once())->method('insert');
209+
210+
$result = $this->controller->accessToken('authorization_code', 'valid-refresh-token');
211+
212+
$this->assertEquals(Http::STATUS_OK, $result->getStatus());
213+
}
214+
199215
public function testAccessTokenLocksRefreshTokenToExchangeOnly(): void {
200216
$signedRequest = $this->createMock(IIncomingSignedRequest::class);
201217
$signedRequest->method('getOrigin')->willReturn('remote.example.com');

0 commit comments

Comments
 (0)