Unlink cleaned up access tokens from refresh tokens #252
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes issue: #247
Breaking changes: no
Changes
AccessTokenManager::clearExpiredTokens
to: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 thisSET 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.