AttributeError: 'NoneType' object has no attribute 'token' (race condition)

[Timvde]

Describe the bug
In _save_bearer_token (source) in the OAuth2Validator, there is a race condition when REFRESH_TOKEN_GRACE_PERIOD_SECONDS is set.

To Reproduce
Have a misbehaving client that sends many refresh requests in (very) short succession and doesn't have proper refresh token management :)

But more precisely, assume that we start with an access token AT1 and its refresh token RT1. The following sequence of events triggers the issue:

• Request Req1 invalidates RT1 and AT1. This means that RT1 has its revoked field set to now() and access_token set to None, while AT1 is simply deleted. This request generates access token AT2 (with source_refresh_token = RT1) and refresh token RT2 (with access_token = AT2).
• Request Req2 reuses RT1. This should return RT2 and AT2 again. The way that django-oauth-toolkit ensures this, is by using RefreshToken.objects.select_for_update().get(pk=refresh_token_instance.pk), which locks the request until Req1 commits its transaction. At that point, the database state should be consistent, and there is no issue. However, imagine that the request gets delayed somewhere, and the following happens at the same time...
• Request Req3 already uses RT2. This removes AT2 and invalidates RT2. Crucially, during processing, it sets a database lock on RT2, while Req2 has a lock on RT1. Both Req2 and Req3 can continue simultaneously.
  • First, Req2 retrieves what the code calls previous_access_token, but what (as I understand it) should better be named next_access_token (from the POV of the current refresh token), as it looks for an access token for which the source_refresh_token is the current refresh token (RT1), which is AT2.
  • Then, Req3 commits and revokes AT2 / RT2, which sets RT2.access_token to None
  • Next, Req2 tries to retrieve the refresh token for AT2 by doing RefreshToken.objects.filter(access_token=previous_access_token).first().token. However, Req3 has invalidated that token, and there is no refresh token anymore that matches this, resulting in the mentioned AttributeError.

Expected behavior
No attribute error.

Version
3.2.0

• I have tested with the latest published release and it's still a problem.
• I have tested with the master branch and it's still a problem.
  • I have not tested it with the master branch, but the code is unchanged, so I am quite certain that this is still a problem.

Additional context

I don't have a proposed patch at the moment, this seems like it's a difficult problem to fix. My first reaction would be to set a similar lock when retrieving previous_access_token, but that would instead lead to a deadlock: Req2 would have a lock on AT2 and get stuck retrieving RT2, while Req3 has a lock on RT2 and would get stuck deleting AT2. So this doesn't work. Given my limited knowledge of django-oauth-toolkit internals, I don't have another proposal, I'm afraid.

[Timvde]

A simple solution could be to just check whether RefreshToken.objects.filter(access_token=previous_access_token).first() is None, but that would end up in a situation where a single refresh token created two access tokens, which feels semantically very ugly.

... However, I just checked this behaviour in a purely sequential version:

• Use RT1, which creates RT2/AT2
• Use RT2, which deletes AT2
• Use RT1 again, which creates a new RT3/AT3
• I now have two independent pairs of access/refresh tokens that both are valid

So... Is this actually valid? Are we supposed to be able to generate two different access tokens from the same refresh token when within the grace period? If so, then this simple solution is valid, I guess. If not, that seems like another bug to fix 😅

[Timvde]

Tomorrow morning, I will deploy a release with the following addition to our production application, I will keep you posted about the results:

                if next_access_token:
                    next_refresh_token = RefreshToken.objects.filter(access_token=next_access_token).first()
                    if not next_refresh_token:
                        # Race condition: the access token was revoked (deleted) in between retrieving it and now
                        next_access_token = None
                        next_refresh_token = None

(As you can see, I also renamed previous_access_token to next_access_token, although in retrospect, also that name isn't great. Maybe existing_descendant_access_token or something?)

I can submit a PR for this, but this race condition is completely untestable, as far as I can tell. The contribution guidelines say:

Whenever you add code, you have to add tests as well. We cannot accept untested code, so unless it is a peculiar situation you previously discussed with the core committers, if your pull request reduces the test coverage it will be immediately rejected.

So before doing so, I'd like to discuss whether you see any way to test this. Also let me know what you prefer with regards to the naming of previous_access_token. I don't like the name, but given that it's hard to find an alternative, I can just leave it as it is when submitting a PR.