Merge pull request #61889 from nextcloud/kano-fix-ocm-duplicate-sharedSecret
CloudFederationApi: access-token lifecycle fixespull/63239/merge
Micke Nordin
2 days ago
committed by
GitHub
commit
7dd9f40799
@ -0,0 +1,56 @@ |
||||
<?php |
||||
|
||||
declare(strict_types=1); |
||||
|
||||
/** |
||||
* SPDX-FileCopyrightText: 2026 Nextcloud GmbH and Nextcloud contributors |
||||
* SPDX-License-Identifier: AGPL-3.0-or-later |
||||
*/ |
||||
|
||||
namespace OCA\CloudFederationAPI\Listener; |
||||
|
||||
use OC\Authentication\Token\IProvider; |
||||
use OCA\CloudFederationAPI\Service\OcmTokenService; |
||||
use OCP\EventDispatcher\Event; |
||||
use OCP\EventDispatcher\IEventListener; |
||||
use OCP\Share\Events\ShareDeletedEvent; |
||||
use OCP\Share\IShare; |
||||
|
||||
/** |
||||
* When a federated share is removed, revoke the OCM access tokens it minted |
||||
* and invalidate its refresh token immediately, instead of waiting up to six |
||||
* hours for the expiry job — which cannot even find them once the share, and |
||||
* with it the mapping's context, is gone. |
||||
* |
||||
* @template-implements IEventListener<ShareDeletedEvent> |
||||
*/ |
||||
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); |
||||
} |
||||
} |
||||
@ -0,0 +1,70 @@ |
||||
<?php |
||||
|
||||
declare(strict_types=1); |
||||
|
||||
/** |
||||
* SPDX-FileCopyrightText: 2026 Nextcloud GmbH and Nextcloud contributors |
||||
* SPDX-License-Identifier: AGPL-3.0-or-later |
||||
*/ |
||||
|
||||
namespace OCA\CloudFederationAPI\Service; |
||||
|
||||
use OC\Authentication\Exceptions\ExpiredTokenException; |
||||
use OC\Authentication\Exceptions\InvalidTokenException; |
||||
use OC\Authentication\Exceptions\WipeTokenException; |
||||
use OC\Authentication\Token\IProvider; |
||||
use OCA\CloudFederationAPI\Db\OcmTokenMap; |
||||
use OCA\CloudFederationAPI\Db\OcmTokenMapMapper; |
||||
|
||||
/** |
||||
* Revokes OCM access tokens together with their ocm_token_map rows, so a |
||||
* removed or expired mapping never leaves an orphaned oc_authtoken entry. |
||||
*/ |
||||
class OcmTokenService { |
||||
public function __construct( |
||||
private readonly OcmTokenMapMapper $mapper, |
||||
private readonly IProvider $tokenProvider, |
||||
) { |
||||
} |
||||
|
||||
/** |
||||
* Revoke every access token whose mapping expired before $time. |
||||
*/ |
||||
public function revokeExpired(int $time): void { |
||||
foreach ($this->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); |
||||
} |
||||
} |
||||
@ -0,0 +1,63 @@ |
||||
<?php |
||||
|
||||
declare(strict_types=1); |
||||
|
||||
/** |
||||
* SPDX-FileCopyrightText: 2026 Nextcloud GmbH and Nextcloud contributors |
||||
* SPDX-License-Identifier: AGPL-3.0-or-later |
||||
*/ |
||||
|
||||
namespace OCA\CloudFederationAPI\Tests\Listener; |
||||
|
||||
use OC\Authentication\Token\IProvider; |
||||
use OCA\CloudFederationAPI\Listener\ShareDeletedListener; |
||||
use OCA\CloudFederationAPI\Service\OcmTokenService; |
||||
use OCP\Share\Events\ShareDeletedEvent; |
||||
use OCP\Share\IShare; |
||||
use PHPUnit\Framework\MockObject\MockObject; |
||||
use Test\TestCase; |
||||
|
||||
class ShareDeletedListenerTest extends TestCase { |
||||
private OcmTokenService&MockObject $tokenService; |
||||
private IProvider&MockObject $tokenProvider; |
||||
private ShareDeletedListener $listener; |
||||
|
||||
#[\Override] |
||||
protected function setUp(): void { |
||||
parent::setUp(); |
||||
|
||||
$this->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, '')); |
||||
} |
||||
} |
||||
@ -0,0 +1,98 @@ |
||||
<?php |
||||
|
||||
declare(strict_types=1); |
||||
|
||||
/** |
||||
* SPDX-FileCopyrightText: 2026 Nextcloud GmbH and Nextcloud contributors |
||||
* SPDX-License-Identifier: AGPL-3.0-or-later |
||||
*/ |
||||
|
||||
namespace OCA\CloudFederationAPI\Tests\Service; |
||||
|
||||
use OC\Authentication\Exceptions\ExpiredTokenException; |
||||
use OC\Authentication\Exceptions\InvalidTokenException; |
||||
use OC\Authentication\Token\IProvider; |
||||
use OC\Authentication\Token\IToken; |
||||
use OCA\CloudFederationAPI\Db\OcmTokenMap; |
||||
use OCA\CloudFederationAPI\Db\OcmTokenMapMapper; |
||||
use OCA\CloudFederationAPI\Service\OcmTokenService; |
||||
use PHPUnit\Framework\MockObject\MockObject; |
||||
use Test\TestCase; |
||||
|
||||
class OcmTokenServiceTest extends TestCase { |
||||
private OcmTokenMapMapper&MockObject $mapper; |
||||
private IProvider&MockObject $tokenProvider; |
||||
private OcmTokenService $service; |
||||
|
||||
#[\Override] |
||||
protected function setUp(): void { |
||||
parent::setUp(); |
||||
|
||||
$this->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'); |
||||
} |
||||
} |
||||
Loading…
Reference in new issue