diff --git a/apps/cloud_federation_api/composer/composer/autoload_classmap.php b/apps/cloud_federation_api/composer/composer/autoload_classmap.php index 085ea9dc176..f5a74754b28 100644 --- a/apps/cloud_federation_api/composer/composer/autoload_classmap.php +++ b/apps/cloud_federation_api/composer/composer/autoload_classmap.php @@ -15,8 +15,10 @@ return array( 'OCA\\CloudFederationAPI\\Controller\\TokenController' => $baseDir . '/../lib/Controller/TokenController.php', 'OCA\\CloudFederationAPI\\Db\\OcmTokenMap' => $baseDir . '/../lib/Db/OcmTokenMap.php', 'OCA\\CloudFederationAPI\\Db\\OcmTokenMapMapper' => $baseDir . '/../lib/Db/OcmTokenMapMapper.php', + 'OCA\\CloudFederationAPI\\Listener\\ShareDeletedListener' => $baseDir . '/../lib/Listener/ShareDeletedListener.php', 'OCA\\CloudFederationAPI\\Migration\\DropFederatedInvitesTable' => $baseDir . '/../lib/Migration/DropFederatedInvitesTable.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 70a4a8fa6a7..87d1dac996b 100644 --- a/apps/cloud_federation_api/composer/composer/autoload_static.php +++ b/apps/cloud_federation_api/composer/composer/autoload_static.php @@ -30,10 +30,12 @@ class ComposerStaticInitCloudFederationAPI 'OCA\\CloudFederationAPI\\Controller\\TokenController' => __DIR__ . '/..' . '/../lib/Controller/TokenController.php', 'OCA\\CloudFederationAPI\\Db\\OcmTokenMap' => __DIR__ . '/..' . '/../lib/Db/OcmTokenMap.php', 'OCA\\CloudFederationAPI\\Db\\OcmTokenMapMapper' => __DIR__ . '/..' . '/../lib/Db/OcmTokenMapMapper.php', + 'OCA\\CloudFederationAPI\\Listener\\ShareDeletedListener' => __DIR__ . '/..' . '/../lib/Listener/ShareDeletedListener.php', 'OCA\\CloudFederationAPI\\Migration\\DropFederatedInvitesTable' => __DIR__ . '/..' . '/../lib/Migration/DropFederatedInvitesTable.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 9170e9d0792..7d563fd666a 100644 --- a/apps/cloud_federation_api/lib/AppInfo/Application.php +++ b/apps/cloud_federation_api/lib/AppInfo/Application.php @@ -9,10 +9,12 @@ declare(strict_types=1); namespace OCA\CloudFederationAPI\AppInfo; +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'; @@ -23,6 +25,7 @@ class Application extends App implements IBootstrap { #[\Override] public function register(IRegistrationContext $context): void { + $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 f946e0dc707..342cecb0dd2 100644 --- a/apps/cloud_federation_api/lib/BackgroundJob/CleanupExpiredOcmTokensJob.php +++ b/apps/cloud_federation_api/lib/BackgroundJob/CleanupExpiredOcmTokensJob.php @@ -9,27 +9,22 @@ declare(strict_types=1); 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 @@ class CleanupExpiredOcmTokensJob extends TimedJob { #[\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 4417cbb8d5e..c6f6875fae4 100644 --- a/apps/cloud_federation_api/lib/Db/OcmTokenMapMapper.php +++ b/apps/cloud_federation_api/lib/Db/OcmTokenMapMapper.php @@ -49,6 +49,21 @@ class OcmTokenMapMapper extends QBMapper { } } + /** + * 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 00000000000..eafd0a71517 --- /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 00000000000..f07e26b78d0 --- /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 834d925157d..1ecabdecc4e 100644 --- a/apps/cloud_federation_api/tests/BackgroundJob/CleanupExpiredOcmTokensJobTest.php +++ b/apps/cloud_federation_api/tests/BackgroundJob/CleanupExpiredOcmTokensJobTest.php @@ -9,21 +9,15 @@ declare(strict_types=1); 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; #[\Override] @@ -31,73 +25,18 @@ class CleanupExpiredOcmTokensJobTest extends TestCase { 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 00000000000..a1161b4e6f8 --- /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 00000000000..3b1e054807d --- /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'); + } +}