Skip to content

Commit 242ea1f

Browse files
committed
bug #252 Unlink cleaned up access tokens from refresh tokens (frankdekker)
This PR was squashed before being merged into the 1.x-dev branch. Discussion ---------- Unlink cleaned up access tokens from refresh tokens Fixes issue: #247 Breaking changes: no Changes `AccessTokenManager::clearExpiredTokens` to: - Find all access token identifiers that have expired. - Execute query to unlink these identifiers from possible refresh tokens. - Thirdly delete the access tokens with those ids. This order ensures that at no point in time there's a refresh token with an invalid access token reference. ## Testing There was already an existing unit test testing the above scenario but didn't trigger the issue because the entity manager wasn't cleared (and the test ran against the EntityManager memory instead of the database). As soon as I added the `$em->clear()` to the test the scenario above occurred. As the original object compare doesn't work anymore as the refresh tokens are actually retrieved from the db instead of the entity manager memory, the DateTime values and object references differ. The test now should find all unlinked refresh tokens. ## Technical choices Looking at the relation between RefreshToken and AccessToken, the relation is defined as `SET TO NULL` when the access token is deleted. However as the access tokens aren't deleted through the EntityManager this `SET TO NULL` is never triggered. An alternative implementation would be to retrieve all the expired AccessTokens and delete them through the EntityManager. However I think performance wise this will not be quick. Commits ------- f1b5c20 Unlink cleaned up access tokens from refresh tokens
2 parents ed23d56 + f1b5c20 commit 242ea1f

File tree

2 files changed

+34
-7
lines changed

2 files changed

+34
-7
lines changed

src/Manager/Doctrine/AccessTokenManager.php

Lines changed: 31 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@
88
use League\Bundle\OAuth2ServerBundle\Manager\AccessTokenManagerInterface;
99
use League\Bundle\OAuth2ServerBundle\Model\AccessToken;
1010
use League\Bundle\OAuth2ServerBundle\Model\AccessTokenInterface;
11+
use League\Bundle\OAuth2ServerBundle\Model\RefreshToken;
1112

1213
final class AccessTokenManager implements AccessTokenManagerInterface
1314
{
@@ -50,12 +51,39 @@ public function clearExpired(): int
5051
return 0;
5152
}
5253

53-
/** @var int */
54-
return $this->entityManager->createQueryBuilder()
55-
->delete(AccessToken::class, 'at')
54+
/** @var array{identifier: string}[] */
55+
$results = $this->entityManager->createQueryBuilder()
56+
->select('at.identifier')
57+
->from(AccessToken::class, 'at')
5658
->where('at.expiry < :expiry')
5759
->setParameter('expiry', new \DateTimeImmutable(), 'datetime_immutable')
5860
->getQuery()
61+
->getScalarResult();
62+
if (0 === \count($results)) {
63+
return 0;
64+
}
65+
66+
/** @var string[] */
67+
$ids = array_column($results, 'identifier');
68+
69+
// unlink access tokens from refresh tokens
70+
$this->entityManager->createQueryBuilder()
71+
->update(RefreshToken::class, 'rt')
72+
->set('rt.accessToken', ':null')
73+
->where('rt.accessToken IN (:ids)')
74+
->setParameter('null', null)
75+
->setParameter('ids', $ids)
76+
->getQuery()
5977
->execute();
78+
79+
// delete expired access tokens
80+
$this->entityManager->createQueryBuilder()
81+
->delete(AccessToken::class, 'at')
82+
->where('at.identifier IN (:ids)')
83+
->setParameter('ids', $ids)
84+
->getQuery()
85+
->execute();
86+
87+
return \count($ids);
6088
}
6189
}

tests/Acceptance/DoctrineAccessTokenManagerTest.php

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -123,10 +123,9 @@ public function testClearExpiredWithRefreshToken(): void
123123

124124
$this->assertSame(3, $doctrineAccessTokenManager->clearExpired());
125125

126-
$this->assertSame(
127-
$testData['output'],
128-
$em->getRepository(RefreshToken::class)->findBy(['accessToken' => null], ['identifier' => 'ASC'])
129-
);
126+
$em->clear();
127+
128+
self::assertCount(3, $em->getRepository(RefreshToken::class)->findBy(['accessToken' => null], ['identifier' => 'ASC']));
130129
}
131130

132131
public function testClearExpiredWithRefreshTokenWithoutSavingAccessToken(): void

0 commit comments

Comments
 (0)