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 7419439555204..342cecb0dd2ba 100644 --- a/apps/cloud_federation_api/lib/BackgroundJob/CleanupExpiredOcmTokensJob.php +++ b/apps/cloud_federation_api/lib/BackgroundJob/CleanupExpiredOcmTokensJob.php @@ -9,20 +9,22 @@ namespace OCA\CloudFederationAPI\BackgroundJob; -use OCA\CloudFederationAPI\Db\OcmTokenMapMapper; +use OCA\CloudFederationAPI\Service\OcmTokenService; 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 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 OcmTokenService $tokenService, ) { parent::__construct($timeFactory); @@ -32,6 +34,6 @@ public function __construct( #[\Override] protected function run($argument): void { - $this->mapper->deleteExpired($this->time->getTime()); + $this->tokenService->revokeExpired($this->time->getTime()); } } 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 c54e4e106be03..a5d411a5b8963 100644 --- a/apps/cloud_federation_api/lib/Db/OcmTokenMapMapper.php +++ b/apps/cloud_federation_api/lib/Db/OcmTokenMapMapper.php @@ -34,25 +34,31 @@ public function getByAccessTokenId(int $accessTokenId): OcmTokenMap { } /** - * Find the current mapping for a given refresh token, if any. + * All mappings for a refresh token. Unlike findByRefreshToken this + * tolerates the duplicate rows a concurrent exchange can create. + * + * @return OcmTokenMap[] */ - public function findByRefreshToken(string $refreshToken): ?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))); - try { - return $this->findEntity($qb); - } catch (DoesNotExistException) { - return null; - } + return $this->findEntities($qb); } - 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/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 c81f734579f31..140ad17c19631 100644 --- a/apps/cloud_federation_api/tests/BackgroundJob/CleanupExpiredOcmTokensJobTest.php +++ b/apps/cloud_federation_api/tests/BackgroundJob/CleanupExpiredOcmTokensJobTest.php @@ -10,34 +10,30 @@ namespace OCA\CloudFederationAPI\Tests\BackgroundJob; use OCA\CloudFederationAPI\BackgroundJob\CleanupExpiredOcmTokensJob; -use OCA\CloudFederationAPI\Db\OcmTokenMapMapper; +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 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->tokenService = $this->createMock(OcmTokenService::class); - $this->job = new CleanupExpiredOcmTokensJob($this->timeFactory, $this->mapper); + $this->job = new CleanupExpiredOcmTokensJob($this->timeFactory, $this->tokenService); } - public function testRunDeletesExpiredTokens(): void { + public function testRunRevokesExpiredAtCurrentTime(): void { $now = 1700000000; - $this->timeFactory->expects($this->once()) - ->method('getTime') - ->willReturn($now); - - $this->mapper->expects($this->once()) - ->method('deleteExpired') - ->with($now); + $this->timeFactory->method('getTime')->willReturn($now); + $this->tokenService->expects($this->once()) + ->method('revokeExpired')->with($now); $method = new \ReflectionMethod(CleanupExpiredOcmTokensJob::class, 'run'); $method->invoke($this->job, []); 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'); 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'); + } +}