Skip to content

Commit 83513fa

Browse files
committed
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 <kano@sunet.se>
1 parent b8a68ae commit 83513fa

3 files changed

Lines changed: 24 additions & 34 deletions

File tree

apps/cloud_federation_api/lib/Controller/TokenController.php

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

180-
// Revoke the previous access token for this refresh token, if any.
181-
$existingMapping = $this->ocmTokenMapMapper->findByRefreshToken($refreshToken);
182-
if ($existingMapping !== null) {
183-
try {
184-
$this->tokenProvider->invalidateTokenById(
185-
$token->getUID(),
186-
$existingMapping->getAccessTokenId()
187-
);
188-
} catch (\Exception) {
189-
// Token may already be gone; ignore.
190-
}
191-
$this->ocmTokenMapMapper->delete($existingMapping);
192-
}
193-
180+
// A refresh token may back several concurrent access tokens (e.g. the
181+
// webdav mount and the webapp launcher exchange the same secret), so
182+
// each exchange issues a fresh one and leaves the others in place;
183+
// expiry and unshare cleanup revoke them.
194184
$share = $this->shareManager->getShareByToken($refreshToken);
195185
// access_token TTL from the refresh-token scope; default 3600, clamped 300..86400.
196186
$ttl = (int)($token->getScopeAsArray()['ocm_access_token_ttl'] ?? 3600);

apps/cloud_federation_api/lib/Db/OcmTokenMapMapper.php

Lines changed: 0 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -33,22 +33,6 @@ public function getByAccessTokenId(int $accessTokenId): OcmTokenMap {
3333
return $this->findEntity($qb);
3434
}
3535

36-
/**
37-
* Find the current mapping for a given refresh token, if any.
38-
*/
39-
public function findByRefreshToken(string $refreshToken): ?OcmTokenMap {
40-
$qb = $this->db->getQueryBuilder();
41-
$qb->select('*')
42-
->from($this->getTableName())
43-
->where($qb->expr()->eq('refresh_token', $qb->createNamedParameter($refreshToken)));
44-
45-
try {
46-
return $this->findEntity($qb);
47-
} catch (DoesNotExistException) {
48-
return null;
49-
}
50-
}
51-
5236
/**
5337
* All mappings for a refresh token. Unlike findByRefreshToken this
5438
* tolerates the duplicate rows a concurrent exchange can create.

apps/cloud_federation_api/tests/Controller/TokenControllerTest.php

Lines changed: 20 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -116,10 +116,6 @@ private function configureHappyPath(
116116
->with($refreshToken)
117117
->willReturn($refreshTokenMock);
118118

119-
$this->ocmTokenMapMapper->method('findByRefreshToken')
120-
->with($refreshToken)
121-
->willReturn(null);
122-
123119
$share = $this->createMock(IShare::class);
124120
$share->method('getShareOwner')->willReturn($shareOwner);
125121
$share->method('getSharedWith')->willReturn($sharedWith);
@@ -182,6 +178,26 @@ public function testAccessTokenSuccess(): void {
182178
$this->assertSame(1000000 + 3600, $decoded->exp);
183179
}
184180

181+
public function testAccessTokenDoesNotRevokeExistingTokens(): void {
182+
$signedRequest = $this->createMock(IIncomingSignedRequest::class);
183+
$signedRequest->method('getOrigin')->willReturn('remote.example.com');
184+
$this->signatureManager->method('getIncomingSignedRequest')
185+
->with($this->signatoryManager)
186+
->willReturn($signedRequest);
187+
188+
$this->configureHappyPath('valid-refresh-token', 123, 'testuser', 'owner', 'sharee@remote.example.com', 'fixedjtivalue00');
189+
190+
// A refresh token may back multiple concurrent access tokens, so an
191+
// exchange only adds one and never revokes or deletes an existing one.
192+
$this->tokenProvider->expects($this->never())->method('invalidateTokenById');
193+
$this->ocmTokenMapMapper->expects($this->never())->method('delete');
194+
$this->ocmTokenMapMapper->expects($this->once())->method('insert');
195+
196+
$result = $this->controller->accessToken('authorization_code', 'valid-refresh-token');
197+
198+
$this->assertEquals(Http::STATUS_OK, $result->getStatus());
199+
}
200+
185201
public function testAccessTokenLocksRefreshTokenToExchangeOnly(): void {
186202
$signedRequest = $this->createMock(IIncomingSignedRequest::class);
187203
$signedRequest->method('getOrigin')->willReturn('remote.example.com');

0 commit comments

Comments
 (0)