Multiple concurrency issues

[gbataille]

Hi all,

Issue

So for 2 years that I have been using django-oauth-toolkit, I have been plagued with concurrency issues of the form

DoesNotExist: AccessToken matching query does not exist

originally from

oauth2_provider/oauth2_validators.py in get_original_scopes at line 427

but after moving to version 1.1.2, it's

oauth2_provider/oauth2_validators.py in get_original_scopes at line 608

The thing is that (for me), 1.1.2 has also brought a new kind of issues

IntegrityError: duplicate key value violates unique constraint "oauth2_provider_accesstoken_source_refresh_token_id_key"
DETAIL:  Key (source_refresh_token_id)=(80187) already exists.

from

oauth2_provider/oauth2_validators.py in save_bearer_token at line 534
--> oauth2_provider/oauth2_validators.py in _create_access_token at line 566

I finally got to really look into them.

Problem

For me, those are concurrency issues that are due to the fact that we get the refresh_token from the db early in the process and that we try to use it later (to create a new access token for example).

I have drawn the process in a small diagram (source here)

With the problem being in between the 2 red steps.
We must not have 2 processes going to this rightmost branch with the same refresh token.

To Reproduce

You can try and force some concurrency like so.
https://gist.github.com/gbataille/099a9eb4989c4277e9ce3312c8014391

Using it locally, I can make it fail in one of the 2 cases above quite often.

Fix

I think I have a fix. At least the logic seems sound, and I can't reproduce the issue anymore after applying it.

Submitting the PR

The key is to add the green step

[gbataille]

Ok, so I think the (second) problem is different now. Might not even be a concurrency issue.

I have RefreshToken in my db that have no access_token linked. That tends to mean (to my understading) that those RefreshToken have been used. Yet

• they are not marked as expired (no revoked date)
• there is no AccessToken with source_refresh_token pointing to the RefreshToken

I'm not excluding the fact that some of the clients that connect to me are doing some weird things, but still, we should not fall in this situation.
I'm continuing to try and find more details

[gbataille]

Wow, so found it (I think). Nothing to do with a concurrency issue.
In some cases, we are deleting (django delete) access tokens directly...

So I think the PR submitted fixes the concurrency part.
The other part is purely me

(actually maybe it's rather unsafe that you can do AccessToken.revoke() and that it deletes the AccessToken but leave a valid but broken refreshToken. WDYT?)

--> Opening a separate issue to discuss the data inconsistency we can end up in

[cleder]

could that be related to https://code.djangoproject.com/ticket/29499 ?

[gbataille]

well it's not linked to an update_or_create no. Django is not at fault here (that I can see), it's just that we need to handle the transactionality cleanly, like in the PR I submitted.

[waqasraz]

I don't think it's is fixed. I am still getting it in version 1.2.0

raise dj_exc_value.with_traceback(traceback) from exc_value File "/home/waqas/.virtualenv/automation_manager/lib/python3.6/site-packages/django/db/backends/utils.py", line 85, in _execute return self.cursor.execute(sql, params) django.db.utils.IntegrityError: duplicate key value violates unique constraint "oauth2_provider_accesstoken_source_refresh_token_id_key" DETAIL: Key (source_refresh_token_id)=(198) already exists.

[waqasraz]

Any chance this will be fixed. Or at least any version that is kinda stable I can use that.

[n2ygk]

See #663

[jamesshaw49]

@gbataille did you manage to find the issue?

I am also having the same error Integrity error. Out of 288,000 requests, only 80 raised the error, so it is very hard to replicate.

I am also having another error, which I believe may be related:
oauth2_provider.models:DoesNotExist: AccessToken matching query does not exist.

This is even less frequent, about 5 requests raised this error out of 288,000 making it even harder to replicate.

I have a feeling the issue arises due to our access/refresh application logic, but not entirely sure. I would have thought that if it were an issue with django-oauth, that the issue would be more widespread and django-oauth would have been updated recently.

[gbataille]

@jamesshaw49 since I raised these 2 PR and put them in my production, I have never had the issue again.
Note that there was 2 things (that I can recall): 1 or 2 concurrency issues that I have provided PRs for AND a migration that was not orchestrated properly between the 0.x version and the 1.x leaving existing token in the database in a state that was inconsistent for the new application and I had to run a data migration

Lookup my comments in #645

[Sticky-Bits]

We are also suffering from these issues using 1.2.
@gbataille Looks like your PR only made it to the stable/1.1 branch. The latest pypy version 1.2 doesn't have your changes:

django-oauth-toolkit/oauth2_provider/oauth2_validators.py

Line 510 in 86e8f9b

if isinstance(refresh_token_instance, RefreshToken):

Should we merge your PR into master as well? We can use 1.1 in the mean time, but I'm worried this will get forgotten about and lost for future releases.

Edit: My apologies, looks like it did get merged back into master, but the 1.2 tag is before the cherry pick. Can we cut a new release to fix the issue for those not on 1.1?

[gbataille]

I'm indeed still on 1.1. Following your comment, I'll wait before moving to 1.2!

scratch that, I'm actually still on my own fork. Not on Django 2 yet :( so I have not tried to update to a version of django-oauth-toolkit that has my changes.

[oliamb]

Is it possibly solved by #810?
It is part of 1.3.1 release

[daveisfera]

This appears to still be an issue in the current version. Anything I can do to help get this resolved?

[n2ygk]

This appears to still be an issue in the current version. Anything I can do to help get this resolved?

@daveisfera Please document in detail how you are able to reproduce this error with the latest code release and master branch, preferably with code that someone else can run. And submit a PR if you are able to resolve it. Given the history of this ticket, it looks like a hard one to clearly understand and seems to be limited to a small number of people experiencing it.

Thanks.

[daveisfera]

I've been working on making a reproducer for this, but I haven't had any luck so far. I thought it was from multiple requests made at the same time, but I haven't been able to get my tests to fail with all of the variations that I've tried.

Any ideas on what I can try to make it happen?