From 4f7ab0d995cca65653a0bbce487b300e6df2c78d Mon Sep 17 00:00:00 2001 From: Micke Nordin Date: Tue, 7 Jul 2026 16:19:59 +0200 Subject: [PATCH 1/3] fix(cloud_federation_api): revoke OCM access tokens on expiry cleanup The cleanup job only deleted ocm_token_map rows, orphaning the temporary access tokens in oc_authtoken so they accumulated indefinitely. Revoke each expired access token before dropping its mapping. Assisted-by: ClaudeCode:claude-fable-5 Signed-off-by: Micke Nordin --- .../CleanupExpiredOcmTokensJob.php | 36 ++++++++- .../lib/Db/OcmTokenMapMapper.php | 13 +++- .../CleanupExpiredOcmTokensJobTest.php | 77 ++++++++++++++++--- 3 files changed, 109 insertions(+), 17 deletions(-) diff --git a/apps/cloud_federation_api/lib/BackgroundJob/CleanupExpiredOcmTokensJob.php b/apps/cloud_federation_api/lib/BackgroundJob/CleanupExpiredOcmTokensJob.php index 7419439555204..f946e0dc707f6 100644 --- a/apps/cloud_federation_api/lib/BackgroundJob/CleanupExpiredOcmTokensJob.php +++ b/apps/cloud_federation_api/lib/BackgroundJob/CleanupExpiredOcmTokensJob.php @@ -9,20 +9,27 @@ namespace OCA\CloudFederationAPI\BackgroundJob; +use OC\Authentication\Exceptions\ExpiredTokenException; +use OC\Authentication\Exceptions\InvalidTokenException; +use OC\Authentication\Exceptions\WipeTokenException; +use OC\Authentication\Token\IProvider; use OCA\CloudFederationAPI\Db\OcmTokenMapMapper; use OCP\AppFramework\Utility\ITimeFactory; use OCP\BackgroundJob\TimedJob; /** - * Periodically purge expired OCM access token mappings from ocm_token_map. + * Periodically purge expired OCM access tokens. * - * The corresponding oc_authtoken entries (TEMPORARY_TOKEN with an expires - * timestamp) are cleaned up by Nextcloud's own token expiry jobs. + * Each expired mapping is revoked in two steps: the access token is deleted + * from oc_authtoken and only then is the ocm_token_map row removed. Dropping + * the mapping first would orphan the access token, since nothing else records + * which oc_authtoken id belongs to a given refresh token. */ class CleanupExpiredOcmTokensJob extends TimedJob { public function __construct( ITimeFactory $timeFactory, private readonly OcmTokenMapMapper $mapper, + private readonly IProvider $tokenProvider, ) { parent::__construct($timeFactory); @@ -32,6 +39,27 @@ public function __construct( #[\Override] protected function run($argument): void { - $this->mapper->deleteExpired($this->time->getTime()); + $now = $this->time->getTime(); + foreach ($this->mapper->findExpired($now) as $mapping) { + $this->revokeAccessToken($mapping->getAccessTokenId()); + $this->mapper->delete($mapping); + } + } + + /** + * Delete the access token itself from oc_authtoken. getTokenById throws + * for an already-expired token but still carries it, so we can recover the + * owner uid required by invalidateTokenById. + */ + private function revokeAccessToken(int $accessTokenId): void { + try { + $token = $this->tokenProvider->getTokenById($accessTokenId); + } catch (ExpiredTokenException|WipeTokenException $e) { + $token = $e->getToken(); + } catch (InvalidTokenException) { + // Access token already gone; nothing left to revoke. + return; + } + $this->tokenProvider->invalidateTokenById($token->getUID(), $accessTokenId); } } diff --git a/apps/cloud_federation_api/lib/Db/OcmTokenMapMapper.php b/apps/cloud_federation_api/lib/Db/OcmTokenMapMapper.php index c54e4e106be03..4417cbb8d5e6e 100644 --- a/apps/cloud_federation_api/lib/Db/OcmTokenMapMapper.php +++ b/apps/cloud_federation_api/lib/Db/OcmTokenMapMapper.php @@ -49,10 +49,17 @@ public function findByRefreshToken(string $refreshToken): ?OcmTokenMap { } } - public function deleteExpired(int $time): void { + /** + * All mappings whose access token has expired before $time. + * + * @return OcmTokenMap[] + */ + public function findExpired(int $time): array { $qb = $this->db->getQueryBuilder(); - $qb->delete($this->getTableName()) + $qb->select('*') + ->from($this->getTableName()) ->where($qb->expr()->lt('expires', $qb->createNamedParameter($time))); - $qb->executeStatement(); + + return $this->findEntities($qb); } } diff --git a/apps/cloud_federation_api/tests/BackgroundJob/CleanupExpiredOcmTokensJobTest.php b/apps/cloud_federation_api/tests/BackgroundJob/CleanupExpiredOcmTokensJobTest.php index c81f734579f31..505f9ac6b4391 100644 --- a/apps/cloud_federation_api/tests/BackgroundJob/CleanupExpiredOcmTokensJobTest.php +++ b/apps/cloud_federation_api/tests/BackgroundJob/CleanupExpiredOcmTokensJobTest.php @@ -9,8 +9,13 @@ namespace OCA\CloudFederationAPI\Tests\BackgroundJob; +use OC\Authentication\Exceptions\ExpiredTokenException; +use OC\Authentication\Exceptions\InvalidTokenException; +use OC\Authentication\Token\IProvider; use OCA\CloudFederationAPI\BackgroundJob\CleanupExpiredOcmTokensJob; +use OCA\CloudFederationAPI\Db\OcmTokenMap; use OCA\CloudFederationAPI\Db\OcmTokenMapMapper; +use OC\Authentication\Token\IToken; use OCP\AppFramework\Utility\ITimeFactory; use PHPUnit\Framework\MockObject\MockObject; use Test\TestCase; @@ -18,6 +23,7 @@ class CleanupExpiredOcmTokensJobTest extends TestCase { private ITimeFactory&MockObject $timeFactory; private OcmTokenMapMapper&MockObject $mapper; + private IProvider&MockObject $tokenProvider; private CleanupExpiredOcmTokensJob $job; protected function setUp(): void { @@ -25,21 +31,72 @@ protected function setUp(): void { $this->timeFactory = $this->createMock(ITimeFactory::class); $this->mapper = $this->createMock(OcmTokenMapMapper::class); + $this->tokenProvider = $this->createMock(IProvider::class); - $this->job = new CleanupExpiredOcmTokensJob($this->timeFactory, $this->mapper); + $this->job = new CleanupExpiredOcmTokensJob( + $this->timeFactory, + $this->mapper, + $this->tokenProvider, + ); } - public function testRunDeletesExpiredTokens(): void { - $now = 1700000000; - $this->timeFactory->expects($this->once()) - ->method('getTime') - ->willReturn($now); - - $this->mapper->expects($this->once()) - ->method('deleteExpired') - ->with($now); + private function mapping(int $accessTokenId): OcmTokenMap { + $mapping = new OcmTokenMap(); + $mapping->setAccessTokenId($accessTokenId); + return $mapping; + } + private function runJob(): void { $method = new \ReflectionMethod(CleanupExpiredOcmTokensJob::class, 'run'); $method->invoke($this->job, []); } + + public function testRunRevokesTokenThenDeletesMapping(): void { + $now = 1700000000; + $this->timeFactory->method('getTime')->willReturn($now); + $mapping = $this->mapping(42); + $this->mapper->expects($this->once()) + ->method('findExpired')->with($now) + ->willReturn([$mapping]); + + $token = $this->createMock(IToken::class); + $token->method('getUID')->willReturn('alice'); + $this->tokenProvider->expects($this->once()) + ->method('getTokenById')->with(42)->willReturn($token); + $this->tokenProvider->expects($this->once()) + ->method('invalidateTokenById')->with('alice', 42); + $this->mapper->expects($this->once())->method('delete')->with($mapping); + + $this->runJob(); + } + + public function testRunRevokesEvenWhenAccessTokenExpired(): void { + $this->timeFactory->method('getTime')->willReturn(1700000000); + $mapping = $this->mapping(7); + $this->mapper->method('findExpired')->willReturn([$mapping]); + + $token = $this->createMock(IToken::class); + $token->method('getUID')->willReturn('bob'); + // getTokenById throws for the expired token but still carries it. + $this->tokenProvider->method('getTokenById')->with(7) + ->willThrowException(new ExpiredTokenException($token)); + $this->tokenProvider->expects($this->once()) + ->method('invalidateTokenById')->with('bob', 7); + $this->mapper->expects($this->once())->method('delete')->with($mapping); + + $this->runJob(); + } + + public function testRunSkipsRevokeWhenAccessTokenAlreadyGone(): void { + $this->timeFactory->method('getTime')->willReturn(1700000000); + $mapping = $this->mapping(9); + $this->mapper->method('findExpired')->willReturn([$mapping]); + + $this->tokenProvider->method('getTokenById')->with(9) + ->willThrowException(new InvalidTokenException()); + $this->tokenProvider->expects($this->never())->method('invalidateTokenById'); + $this->mapper->expects($this->once())->method('delete')->with($mapping); + + $this->runJob(); + } } From 62fcb220522671d390d539232147d0a81f3abdeb Mon Sep 17 00:00:00 2001 From: Micke Nordin Date: Tue, 7 Jul 2026 18:07:58 +0200 Subject: [PATCH 2/3] fix(cloud_federation_api): revoke OCM tokens when a federated share is removed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Extract the revoke-then-delete logic into OcmTokenService and add a ShareDeletedEvent listener that revokes the access tokens exchanged from a removed share's secret, drops their ocm_token_map rows, and invalidates the refresh token — instead of leaving them until the expiry job, which can no longer find them once the share is gone. Assisted-by: ClaudeCode:claude-fable-5 Signed-off-by: Micke Nordin --- .../composer/composer/autoload_classmap.php | 2 + .../composer/composer/autoload_static.php | 2 + .../lib/AppInfo/Application.php | 3 + .../CleanupExpiredOcmTokensJob.php | 40 ++------ .../lib/Db/OcmTokenMapMapper.php | 15 +++ .../lib/Listener/ShareDeletedListener.php | 56 +++++++++++ .../lib/Service/OcmTokenService.php | 70 +++++++++++++ .../CleanupExpiredOcmTokensJobTest.php | 79 ++------------- .../Listener/ShareDeletedListenerTest.php | 62 ++++++++++++ .../tests/Service/OcmTokenServiceTest.php | 97 +++++++++++++++++++ 10 files changed, 323 insertions(+), 103 deletions(-) create mode 100644 apps/cloud_federation_api/lib/Listener/ShareDeletedListener.php create mode 100644 apps/cloud_federation_api/lib/Service/OcmTokenService.php create mode 100644 apps/cloud_federation_api/tests/Listener/ShareDeletedListenerTest.php create mode 100644 apps/cloud_federation_api/tests/Service/OcmTokenServiceTest.php diff --git a/apps/cloud_federation_api/composer/composer/autoload_classmap.php b/apps/cloud_federation_api/composer/composer/autoload_classmap.php index 038e4d6873d24..936e0041dc10b 100644 --- a/apps/cloud_federation_api/composer/composer/autoload_classmap.php +++ b/apps/cloud_federation_api/composer/composer/autoload_classmap.php @@ -19,7 +19,9 @@ 'OCA\\CloudFederationAPI\\Db\\OcmTokenMap' => $baseDir . '/../lib/Db/OcmTokenMap.php', 'OCA\\CloudFederationAPI\\Db\\OcmTokenMapMapper' => $baseDir . '/../lib/Db/OcmTokenMapMapper.php', 'OCA\\CloudFederationAPI\\Events\\FederatedInviteAcceptedEvent' => $baseDir . '/../lib/Events/FederatedInviteAcceptedEvent.php', + 'OCA\\CloudFederationAPI\\Listener\\ShareDeletedListener' => $baseDir . '/../lib/Listener/ShareDeletedListener.php', 'OCA\\CloudFederationAPI\\Migration\\Version1016Date202502262004' => $baseDir . '/../lib/Migration/Version1016Date202502262004.php', 'OCA\\CloudFederationAPI\\Migration\\Version1017Date20260306120000' => $baseDir . '/../lib/Migration/Version1017Date20260306120000.php', 'OCA\\CloudFederationAPI\\ResponseDefinitions' => $baseDir . '/../lib/ResponseDefinitions.php', + 'OCA\\CloudFederationAPI\\Service\\OcmTokenService' => $baseDir . '/../lib/Service/OcmTokenService.php', ); diff --git a/apps/cloud_federation_api/composer/composer/autoload_static.php b/apps/cloud_federation_api/composer/composer/autoload_static.php index f68f6714afa27..486f7f893dfbb 100644 --- a/apps/cloud_federation_api/composer/composer/autoload_static.php +++ b/apps/cloud_federation_api/composer/composer/autoload_static.php @@ -34,9 +34,11 @@ class ComposerStaticInitCloudFederationAPI 'OCA\\CloudFederationAPI\\Db\\OcmTokenMap' => __DIR__ . '/..' . '/../lib/Db/OcmTokenMap.php', 'OCA\\CloudFederationAPI\\Db\\OcmTokenMapMapper' => __DIR__ . '/..' . '/../lib/Db/OcmTokenMapMapper.php', 'OCA\\CloudFederationAPI\\Events\\FederatedInviteAcceptedEvent' => __DIR__ . '/..' . '/../lib/Events/FederatedInviteAcceptedEvent.php', + 'OCA\\CloudFederationAPI\\Listener\\ShareDeletedListener' => __DIR__ . '/..' . '/../lib/Listener/ShareDeletedListener.php', 'OCA\\CloudFederationAPI\\Migration\\Version1016Date202502262004' => __DIR__ . '/..' . '/../lib/Migration/Version1016Date202502262004.php', 'OCA\\CloudFederationAPI\\Migration\\Version1017Date20260306120000' => __DIR__ . '/..' . '/../lib/Migration/Version1017Date20260306120000.php', 'OCA\\CloudFederationAPI\\ResponseDefinitions' => __DIR__ . '/..' . '/../lib/ResponseDefinitions.php', + 'OCA\\CloudFederationAPI\\Service\\OcmTokenService' => __DIR__ . '/..' . '/../lib/Service/OcmTokenService.php', ); public static function getInitializer(ClassLoader $loader) diff --git a/apps/cloud_federation_api/lib/AppInfo/Application.php b/apps/cloud_federation_api/lib/AppInfo/Application.php index 67f55d6663706..dadfe5a643c36 100644 --- a/apps/cloud_federation_api/lib/AppInfo/Application.php +++ b/apps/cloud_federation_api/lib/AppInfo/Application.php @@ -10,10 +10,12 @@ namespace OCA\CloudFederationAPI\AppInfo; use OCA\CloudFederationAPI\Capabilities; +use OCA\CloudFederationAPI\Listener\ShareDeletedListener; use OCP\AppFramework\App; use OCP\AppFramework\Bootstrap\IBootContext; use OCP\AppFramework\Bootstrap\IBootstrap; use OCP\AppFramework\Bootstrap\IRegistrationContext; +use OCP\Share\Events\ShareDeletedEvent; class Application extends App implements IBootstrap { public const APP_ID = 'cloud_federation_api'; @@ -25,6 +27,7 @@ public function __construct() { #[\Override] public function register(IRegistrationContext $context): void { $context->registerCapability(Capabilities::class); + $context->registerEventListener(ShareDeletedEvent::class, ShareDeletedListener::class); } #[\Override] diff --git a/apps/cloud_federation_api/lib/BackgroundJob/CleanupExpiredOcmTokensJob.php b/apps/cloud_federation_api/lib/BackgroundJob/CleanupExpiredOcmTokensJob.php index f946e0dc707f6..342cecb0dd2ba 100644 --- a/apps/cloud_federation_api/lib/BackgroundJob/CleanupExpiredOcmTokensJob.php +++ b/apps/cloud_federation_api/lib/BackgroundJob/CleanupExpiredOcmTokensJob.php @@ -9,27 +9,22 @@ namespace OCA\CloudFederationAPI\BackgroundJob; -use OC\Authentication\Exceptions\ExpiredTokenException; -use OC\Authentication\Exceptions\InvalidTokenException; -use OC\Authentication\Exceptions\WipeTokenException; -use OC\Authentication\Token\IProvider; -use OCA\CloudFederationAPI\Db\OcmTokenMapMapper; +use OCA\CloudFederationAPI\Service\OcmTokenService; use OCP\AppFramework\Utility\ITimeFactory; use OCP\BackgroundJob\TimedJob; /** * Periodically purge expired OCM access tokens. * - * Each expired mapping is revoked in two steps: the access token is deleted - * from oc_authtoken and only then is the ocm_token_map row removed. Dropping - * the mapping first would orphan the access token, since nothing else records - * which oc_authtoken id belongs to a given refresh token. + * Each expired mapping has its access token deleted from oc_authtoken before + * its ocm_token_map row is removed. Dropping the mapping first would orphan + * the access token, since nothing else records which oc_authtoken id belongs + * to a given refresh token. */ class CleanupExpiredOcmTokensJob extends TimedJob { public function __construct( ITimeFactory $timeFactory, - private readonly OcmTokenMapMapper $mapper, - private readonly IProvider $tokenProvider, + private readonly OcmTokenService $tokenService, ) { parent::__construct($timeFactory); @@ -39,27 +34,6 @@ public function __construct( #[\Override] protected function run($argument): void { - $now = $this->time->getTime(); - foreach ($this->mapper->findExpired($now) as $mapping) { - $this->revokeAccessToken($mapping->getAccessTokenId()); - $this->mapper->delete($mapping); - } - } - - /** - * Delete the access token itself from oc_authtoken. getTokenById throws - * for an already-expired token but still carries it, so we can recover the - * owner uid required by invalidateTokenById. - */ - private function revokeAccessToken(int $accessTokenId): void { - try { - $token = $this->tokenProvider->getTokenById($accessTokenId); - } catch (ExpiredTokenException|WipeTokenException $e) { - $token = $e->getToken(); - } catch (InvalidTokenException) { - // Access token already gone; nothing left to revoke. - return; - } - $this->tokenProvider->invalidateTokenById($token->getUID(), $accessTokenId); + $this->tokenService->revokeExpired($this->time->getTime()); } } diff --git a/apps/cloud_federation_api/lib/Db/OcmTokenMapMapper.php b/apps/cloud_federation_api/lib/Db/OcmTokenMapMapper.php index 4417cbb8d5e6e..c6f6875fae4d8 100644 --- a/apps/cloud_federation_api/lib/Db/OcmTokenMapMapper.php +++ b/apps/cloud_federation_api/lib/Db/OcmTokenMapMapper.php @@ -49,6 +49,21 @@ public function findByRefreshToken(string $refreshToken): ?OcmTokenMap { } } + /** + * All mappings for a refresh token. Unlike findByRefreshToken this + * tolerates the duplicate rows a concurrent exchange can create. + * + * @return OcmTokenMap[] + */ + public function findAllByRefreshToken(string $refreshToken): array { + $qb = $this->db->getQueryBuilder(); + $qb->select('*') + ->from($this->getTableName()) + ->where($qb->expr()->eq('refresh_token', $qb->createNamedParameter($refreshToken))); + + return $this->findEntities($qb); + } + /** * All mappings whose access token has expired before $time. * diff --git a/apps/cloud_federation_api/lib/Listener/ShareDeletedListener.php b/apps/cloud_federation_api/lib/Listener/ShareDeletedListener.php new file mode 100644 index 0000000000000..eafd0a715179a --- /dev/null +++ b/apps/cloud_federation_api/lib/Listener/ShareDeletedListener.php @@ -0,0 +1,56 @@ + + */ +class ShareDeletedListener implements IEventListener { + public function __construct( + private readonly OcmTokenService $tokenService, + private readonly IProvider $tokenProvider, + ) { + } + + #[\Override] + public function handle(Event $event): void { + if (!$event instanceof ShareDeletedEvent) { + return; + } + + $share = $event->getShare(); + if (!in_array($share->getShareType(), [IShare::TYPE_REMOTE, IShare::TYPE_REMOTE_GROUP], true)) { + return; + } + + $refreshToken = $share->getToken(); + if ($refreshToken === null || $refreshToken === '') { + return; + } + + // Revoke the access tokens exchanged from this share's secret... + $this->tokenService->revokeByRefreshToken($refreshToken); + // ...and the refresh (permanent) token itself. invalidateToken is a + // no-op when the token is already gone. + $this->tokenProvider->invalidateToken($refreshToken); + } +} diff --git a/apps/cloud_federation_api/lib/Service/OcmTokenService.php b/apps/cloud_federation_api/lib/Service/OcmTokenService.php new file mode 100644 index 0000000000000..f07e26b78d062 --- /dev/null +++ b/apps/cloud_federation_api/lib/Service/OcmTokenService.php @@ -0,0 +1,70 @@ +mapper->findExpired($time) as $mapping) { + $this->revokeMapping($mapping); + } + } + + /** + * Revoke every access token issued for the given refresh token. Tolerates + * the duplicate rows a concurrent exchange can leave behind. + */ + public function revokeByRefreshToken(string $refreshToken): void { + foreach ($this->mapper->findAllByRefreshToken($refreshToken) as $mapping) { + $this->revokeMapping($mapping); + } + } + + private function revokeMapping(OcmTokenMap $mapping): void { + $this->revokeAccessToken($mapping->getAccessTokenId()); + $this->mapper->delete($mapping); + } + + /** + * Delete the access token from oc_authtoken. getTokenById throws for an + * expired token but still carries it, so the owner uid required by + * invalidateTokenById is recoverable. + */ + private function revokeAccessToken(int $accessTokenId): void { + try { + $token = $this->tokenProvider->getTokenById($accessTokenId); + } catch (ExpiredTokenException|WipeTokenException $e) { + $token = $e->getToken(); + } catch (InvalidTokenException) { + // Access token already gone; nothing left to revoke. + return; + } + $this->tokenProvider->invalidateTokenById($token->getUID(), $accessTokenId); + } +} diff --git a/apps/cloud_federation_api/tests/BackgroundJob/CleanupExpiredOcmTokensJobTest.php b/apps/cloud_federation_api/tests/BackgroundJob/CleanupExpiredOcmTokensJobTest.php index 505f9ac6b4391..140ad17c19631 100644 --- a/apps/cloud_federation_api/tests/BackgroundJob/CleanupExpiredOcmTokensJobTest.php +++ b/apps/cloud_federation_api/tests/BackgroundJob/CleanupExpiredOcmTokensJobTest.php @@ -9,94 +9,33 @@ namespace OCA\CloudFederationAPI\Tests\BackgroundJob; -use OC\Authentication\Exceptions\ExpiredTokenException; -use OC\Authentication\Exceptions\InvalidTokenException; -use OC\Authentication\Token\IProvider; use OCA\CloudFederationAPI\BackgroundJob\CleanupExpiredOcmTokensJob; -use OCA\CloudFederationAPI\Db\OcmTokenMap; -use OCA\CloudFederationAPI\Db\OcmTokenMapMapper; -use OC\Authentication\Token\IToken; +use OCA\CloudFederationAPI\Service\OcmTokenService; use OCP\AppFramework\Utility\ITimeFactory; use PHPUnit\Framework\MockObject\MockObject; use Test\TestCase; class CleanupExpiredOcmTokensJobTest extends TestCase { private ITimeFactory&MockObject $timeFactory; - private OcmTokenMapMapper&MockObject $mapper; - private IProvider&MockObject $tokenProvider; + private OcmTokenService&MockObject $tokenService; private CleanupExpiredOcmTokensJob $job; protected function setUp(): void { parent::setUp(); $this->timeFactory = $this->createMock(ITimeFactory::class); - $this->mapper = $this->createMock(OcmTokenMapMapper::class); - $this->tokenProvider = $this->createMock(IProvider::class); + $this->tokenService = $this->createMock(OcmTokenService::class); - $this->job = new CleanupExpiredOcmTokensJob( - $this->timeFactory, - $this->mapper, - $this->tokenProvider, - ); + $this->job = new CleanupExpiredOcmTokensJob($this->timeFactory, $this->tokenService); } - private function mapping(int $accessTokenId): OcmTokenMap { - $mapping = new OcmTokenMap(); - $mapping->setAccessTokenId($accessTokenId); - return $mapping; - } - - private function runJob(): void { - $method = new \ReflectionMethod(CleanupExpiredOcmTokensJob::class, 'run'); - $method->invoke($this->job, []); - } - - public function testRunRevokesTokenThenDeletesMapping(): void { + public function testRunRevokesExpiredAtCurrentTime(): void { $now = 1700000000; $this->timeFactory->method('getTime')->willReturn($now); - $mapping = $this->mapping(42); - $this->mapper->expects($this->once()) - ->method('findExpired')->with($now) - ->willReturn([$mapping]); - - $token = $this->createMock(IToken::class); - $token->method('getUID')->willReturn('alice'); - $this->tokenProvider->expects($this->once()) - ->method('getTokenById')->with(42)->willReturn($token); - $this->tokenProvider->expects($this->once()) - ->method('invalidateTokenById')->with('alice', 42); - $this->mapper->expects($this->once())->method('delete')->with($mapping); - - $this->runJob(); - } + $this->tokenService->expects($this->once()) + ->method('revokeExpired')->with($now); - public function testRunRevokesEvenWhenAccessTokenExpired(): void { - $this->timeFactory->method('getTime')->willReturn(1700000000); - $mapping = $this->mapping(7); - $this->mapper->method('findExpired')->willReturn([$mapping]); - - $token = $this->createMock(IToken::class); - $token->method('getUID')->willReturn('bob'); - // getTokenById throws for the expired token but still carries it. - $this->tokenProvider->method('getTokenById')->with(7) - ->willThrowException(new ExpiredTokenException($token)); - $this->tokenProvider->expects($this->once()) - ->method('invalidateTokenById')->with('bob', 7); - $this->mapper->expects($this->once())->method('delete')->with($mapping); - - $this->runJob(); - } - - public function testRunSkipsRevokeWhenAccessTokenAlreadyGone(): void { - $this->timeFactory->method('getTime')->willReturn(1700000000); - $mapping = $this->mapping(9); - $this->mapper->method('findExpired')->willReturn([$mapping]); - - $this->tokenProvider->method('getTokenById')->with(9) - ->willThrowException(new InvalidTokenException()); - $this->tokenProvider->expects($this->never())->method('invalidateTokenById'); - $this->mapper->expects($this->once())->method('delete')->with($mapping); - - $this->runJob(); + $method = new \ReflectionMethod(CleanupExpiredOcmTokensJob::class, 'run'); + $method->invoke($this->job, []); } } diff --git a/apps/cloud_federation_api/tests/Listener/ShareDeletedListenerTest.php b/apps/cloud_federation_api/tests/Listener/ShareDeletedListenerTest.php new file mode 100644 index 0000000000000..a1161b4e6f87e --- /dev/null +++ b/apps/cloud_federation_api/tests/Listener/ShareDeletedListenerTest.php @@ -0,0 +1,62 @@ +tokenService = $this->createMock(OcmTokenService::class); + $this->tokenProvider = $this->createMock(IProvider::class); + $this->listener = new ShareDeletedListener($this->tokenService, $this->tokenProvider); + } + + private function event(int $shareType, ?string $token): ShareDeletedEvent { + $share = $this->createMock(IShare::class); + $share->method('getShareType')->willReturn($shareType); + $share->method('getToken')->willReturn($token); + return new ShareDeletedEvent($share); + } + + public function testHandleFederatedShareRevokesTokens(): void { + $this->tokenService->expects($this->once()) + ->method('revokeByRefreshToken')->with('secret'); + $this->tokenProvider->expects($this->once()) + ->method('invalidateToken')->with('secret'); + + $this->listener->handle($this->event(IShare::TYPE_REMOTE, 'secret')); + } + + public function testHandleIgnoresNonFederatedShare(): void { + $this->tokenService->expects($this->never())->method('revokeByRefreshToken'); + $this->tokenProvider->expects($this->never())->method('invalidateToken'); + + $this->listener->handle($this->event(IShare::TYPE_USER, 'secret')); + } + + public function testHandleIgnoresEmptyToken(): void { + $this->tokenService->expects($this->never())->method('revokeByRefreshToken'); + $this->tokenProvider->expects($this->never())->method('invalidateToken'); + + $this->listener->handle($this->event(IShare::TYPE_REMOTE, '')); + } +} diff --git a/apps/cloud_federation_api/tests/Service/OcmTokenServiceTest.php b/apps/cloud_federation_api/tests/Service/OcmTokenServiceTest.php new file mode 100644 index 0000000000000..3b1e054807d20 --- /dev/null +++ b/apps/cloud_federation_api/tests/Service/OcmTokenServiceTest.php @@ -0,0 +1,97 @@ +mapper = $this->createMock(OcmTokenMapMapper::class); + $this->tokenProvider = $this->createMock(IProvider::class); + $this->service = new OcmTokenService($this->mapper, $this->tokenProvider); + } + + private function mapping(int $accessTokenId): OcmTokenMap { + $mapping = new OcmTokenMap(); + $mapping->setAccessTokenId($accessTokenId); + return $mapping; + } + + private function token(string $uid): IToken&MockObject { + $token = $this->createMock(IToken::class); + $token->method('getUID')->willReturn($uid); + return $token; + } + + public function testRevokeExpiredRevokesTokenThenDeletesMapping(): void { + $now = 1700000000; + $mapping = $this->mapping(42); + $this->mapper->expects($this->once()) + ->method('findExpired')->with($now)->willReturn([$mapping]); + $this->tokenProvider->method('getTokenById')->with(42) + ->willReturn($this->token('alice')); + $this->tokenProvider->expects($this->once()) + ->method('invalidateTokenById')->with('alice', 42); + $this->mapper->expects($this->once())->method('delete')->with($mapping); + + $this->service->revokeExpired($now); + } + + public function testRevokeHandlesExpiredAccessToken(): void { + $mapping = $this->mapping(7); + $this->mapper->method('findExpired')->willReturn([$mapping]); + // getTokenById throws for the expired token but still carries it. + $this->tokenProvider->method('getTokenById')->with(7) + ->willThrowException(new ExpiredTokenException($this->token('bob'))); + $this->tokenProvider->expects($this->once()) + ->method('invalidateTokenById')->with('bob', 7); + $this->mapper->expects($this->once())->method('delete')->with($mapping); + + $this->service->revokeExpired(1700000000); + } + + public function testRevokeSkipsWhenAccessTokenAlreadyGone(): void { + $mapping = $this->mapping(9); + $this->mapper->method('findExpired')->willReturn([$mapping]); + $this->tokenProvider->method('getTokenById')->with(9) + ->willThrowException(new InvalidTokenException()); + $this->tokenProvider->expects($this->never())->method('invalidateTokenById'); + $this->mapper->expects($this->once())->method('delete')->with($mapping); + + $this->service->revokeExpired(1700000000); + } + + public function testRevokeByRefreshTokenRevokesMapping(): void { + $mapping = $this->mapping(5); + $this->mapper->expects($this->once()) + ->method('findAllByRefreshToken')->with('secret')->willReturn([$mapping]); + $this->tokenProvider->method('getTokenById')->with(5) + ->willReturn($this->token('alice')); + $this->tokenProvider->expects($this->once()) + ->method('invalidateTokenById')->with('alice', 5); + $this->mapper->expects($this->once())->method('delete')->with($mapping); + + $this->service->revokeByRefreshToken('secret'); + } +} From 60f962dc0ee5ff4e8c835a052519ebbfefc71e2d Mon Sep 17 00:00:00 2001 From: Micke Nordin Date: Tue, 7 Jul 2026 19:11:19 +0200 Subject: [PATCH 3/3] fix(cloud_federation_api): allow multiple access tokens per refresh token A refresh token may back several concurrent access tokens. E.g. in a multi protocol share for a webapp the webdav mount and the webapp launcher exchange the same secret, so the exchange must not assume a single mapping. Drop the revoke-previous step and the findByRefreshToken (findEntity) lookup, which threw MultipleObjectsReturnedException once more than one access token existed. Expiry and unshare cleanup revoke the tokens instead. Assisted-by: ClaudeCode:claude-fable-5 Signed-off-by: Micke Nordin --- .../lib/Controller/TokenController.php | 18 ++++---------- .../lib/Db/OcmTokenMapMapper.php | 16 ------------- .../tests/Controller/TokenControllerTest.php | 24 +++++++++++++++---- 3 files changed, 24 insertions(+), 34 deletions(-) diff --git a/apps/cloud_federation_api/lib/Controller/TokenController.php b/apps/cloud_federation_api/lib/Controller/TokenController.php index d62281f898a1b..1dd7c6d5f97ce 100644 --- a/apps/cloud_federation_api/lib/Controller/TokenController.php +++ b/apps/cloud_federation_api/lib/Controller/TokenController.php @@ -177,20 +177,10 @@ public function accessToken(string $grant_type = '', string $code = ''): DataRes $this->tokenProvider->updateToken($token); } - // Revoke the previous access token for this refresh token, if any. - $existingMapping = $this->ocmTokenMapMapper->findByRefreshToken($refreshToken); - if ($existingMapping !== null) { - try { - $this->tokenProvider->invalidateTokenById( - $token->getUID(), - $existingMapping->getAccessTokenId() - ); - } catch (\Exception) { - // Token may already be gone; ignore. - } - $this->ocmTokenMapMapper->delete($existingMapping); - } - + // A refresh token may back several concurrent access tokens (e.g. the + // webdav mount and the webapp launcher exchange the same secret), so + // each exchange issues a fresh one and leaves the others in place; + // expiry and unshare cleanup revoke them. $share = $this->shareManager->getShareByToken($refreshToken); // access_token TTL from the refresh-token scope; default 3600, clamped 300..86400. $ttl = (int)($token->getScopeAsArray()['ocm_access_token_ttl'] ?? 3600); diff --git a/apps/cloud_federation_api/lib/Db/OcmTokenMapMapper.php b/apps/cloud_federation_api/lib/Db/OcmTokenMapMapper.php index c6f6875fae4d8..a5d411a5b8963 100644 --- a/apps/cloud_federation_api/lib/Db/OcmTokenMapMapper.php +++ b/apps/cloud_federation_api/lib/Db/OcmTokenMapMapper.php @@ -33,22 +33,6 @@ public function getByAccessTokenId(int $accessTokenId): OcmTokenMap { return $this->findEntity($qb); } - /** - * Find the current mapping for a given refresh token, if any. - */ - public function findByRefreshToken(string $refreshToken): ?OcmTokenMap { - $qb = $this->db->getQueryBuilder(); - $qb->select('*') - ->from($this->getTableName()) - ->where($qb->expr()->eq('refresh_token', $qb->createNamedParameter($refreshToken))); - - try { - return $this->findEntity($qb); - } catch (DoesNotExistException) { - return null; - } - } - /** * All mappings for a refresh token. Unlike findByRefreshToken this * tolerates the duplicate rows a concurrent exchange can create. diff --git a/apps/cloud_federation_api/tests/Controller/TokenControllerTest.php b/apps/cloud_federation_api/tests/Controller/TokenControllerTest.php index 7ccdca44b7b1e..44f72f96f2c05 100644 --- a/apps/cloud_federation_api/tests/Controller/TokenControllerTest.php +++ b/apps/cloud_federation_api/tests/Controller/TokenControllerTest.php @@ -116,10 +116,6 @@ private function configureHappyPath( ->with($refreshToken) ->willReturn($refreshTokenMock); - $this->ocmTokenMapMapper->method('findByRefreshToken') - ->with($refreshToken) - ->willReturn(null); - $share = $this->createMock(IShare::class); $share->method('getShareOwner')->willReturn($shareOwner); $share->method('getSharedWith')->willReturn($sharedWith); @@ -182,6 +178,26 @@ public function testAccessTokenSuccess(): void { $this->assertSame(1000000 + 3600, $decoded->exp); } + public function testAccessTokenDoesNotRevokeExistingTokens(): void { + $signedRequest = $this->createMock(IIncomingSignedRequest::class); + $signedRequest->method('getOrigin')->willReturn('remote.example.com'); + $this->signatureManager->method('getIncomingSignedRequest') + ->with($this->signatoryManager) + ->willReturn($signedRequest); + + $this->configureHappyPath('valid-refresh-token', 123, 'testuser', 'owner', 'sharee@remote.example.com', 'fixedjtivalue00'); + + // A refresh token may back multiple concurrent access tokens, so an + // exchange only adds one and never revokes or deletes an existing one. + $this->tokenProvider->expects($this->never())->method('invalidateTokenById'); + $this->ocmTokenMapMapper->expects($this->never())->method('delete'); + $this->ocmTokenMapMapper->expects($this->once())->method('insert'); + + $result = $this->controller->accessToken('authorization_code', 'valid-refresh-token'); + + $this->assertEquals(Http::STATUS_OK, $result->getStatus()); + } + public function testAccessTokenLocksRefreshTokenToExchangeOnly(): void { $signedRequest = $this->createMock(IIncomingSignedRequest::class); $signedRequest->method('getOrigin')->willReturn('remote.example.com');