fix race condition for AccessToken creation in oauth2_validators

[n2ygk]

Addresses #609 in which two near-simultaneous requests both presenting the same previously unseen Bearer token (created by an external OAuth2 AS) try to create it.

I don't know how to write a test case to cause two calls to _get_token_from_authentication_server for which a mocked external introspection endpoint delays the its response. I would appreciate any help in coming up with a test case.

As coded, this change does not break existing functionality.

[n2ygk]

After testing this does not fix the problem. Back to the drawing board.

@jleclanche: Any suggestions? Maybe add @transaction.atomic or something to validate_bearer_token?

[jdelic]

This should probably be

access_token = AccessToken.objects.select_related().get_or_create(token=token, user=user, application=None, scope=scope, expires=expires)

instead of trying to catch DoesNotExist and then calling get_or_create. This should block any other thread trying to access the same token, I believe.

[n2ygk]

Thanks @jdelic. Will test and report back.

[coveralls]

Pull Request Test Coverage Report for Build 950

• 0 of 0 changed or added relevant lines in 0 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage increased (+0.6%) to 93.506%

---

Totals
Change from base Build 938:	0.6%
Covered Lines:	1080
Relevant Lines:	1155

---

💛 - Coveralls

[jdelic]

whoops... wow, I must have been asleep when writing this comment :D. Ok, so let's write this up correctly:

• The right thing to do is to lock for update when the token exists
• If the token does not exist, we want to create it

So, I wanted to add select_for_update()

# we also probably need to ensure this is in an atomic transaction
from django.db import transaction
with transaction.atomic():
    access_token = AccessToken.objects.select_related().select_for_update().get_or_create(token=token, user=user, application=None, scope=scope, expires=expires)

but I haven't tested any of this yet. This was meant as a "food for thought" comment :)

[n2ygk]

yeah, sorry for blindly committing. I'm having local troubles just getting various brews of python setup on my mac so that tox can find them all...

[n2ygk]

@jdelic:

[OK, I've fixed my local dev environment so tox works!]

But, more importantly, I think I've got this coded correctly now. I'm not sure if transaction.atomic is actually necessary since select_for_update should (might?) do the appropriate SQL transaction management. After all, our code is running in multiple processes (under Apache mod_wsgi) on horizontally-scaled servers, so the only place for locking to happen is in the database they share.

[n2ygk]

I'll try and add some additional test coverage once we confirm this fix.

[jdelic]

@n2ygk

But, more importantly, I think I've got this coded correctly now.

I agree. Looks good.

I'm not sure if transaction.atomic is actually necessary since select_for_update should (might?) do the appropriate SQL transaction management.

I believe it's necessary, because select_for_update doesn't do anything fancy. It just fails hard if called from outside of a transaction. Django's defaults are to disable ATOMIC_REQUESTS, so by default the token view will execute outside of a transaction.

Just as a pointer: when you get around to write a test, heed the warning on the select_for_update() docs to use TransactionTestCase.

Thank you for taking care of this, by the way.

[n2ygk]

This is not quite enough. The error is now sqlite3.OperationalError: database is locked

So at least it's not failing with sqlite3.IntegrityError: UNIQUE constraint failed: oauth2_provider_accesstoken.token which I can reproduce with 1.2.0.

I'll just have to figure out the right settings to make the client wait (or maybe this is a sqlite3 deficiency and I'll have to test with an external database).

[n2ygk]

Still having a database locked error with sqlite3, but, I found what I think is the correct approach by using update_or_create instead of get_or_create and then updating.

[n2ygk]

I am unable to reproduce the test case with sqlite3, which is no surprise, but means it will be difficult to write a self-contained test case.

However, I was able to reproduce the error with MS SQL Server running in MacOS in a Docker container:

Microsoft SQL Server 2017 (RTM-CU8) (KB4338363) - 14.0.3029.16 (X64) 
      Jun 13 2018 13:35:56 
      Copyright (C) 2017 Microsoft Corporation
      Developer Edition (64-bit) on Linux (Ubuntu 16.04.4 LTS)

I compared toolkit release 1.2.0 vs. e626757 with a test DRF-DJA server running under pycharm with verbose database debugging and it does appear to be doing the right thing w.r.t. setting up transactions in the database server: In the logs for e626757 there's a SAVE TRANSACTION whereas in the 1.2.0 logs there's multiple INSERTs of the same access token followed by:

django.db.utils.IntegrityError: ('23000', "[23000] [FreeTDS][SQL Server]Violation of UNIQUE KEY constraint 'oauth2_provider_accesstoken_token_8af090f8_uniq'. Cannot insert duplicate key in object 'dbo.oauth2_provider_accesstoken'. The duplicate key value is (1VTnm9UKnjIJ8xWCpGZACEWpvJkn). (2627) (SQLExecDirectW)")

However, I'm not comfortable that I've done extensive-enough testing just yet. We'll do that later this week I hope.

I'll scan the code for other cases where update_or_create is appropriate.

[jdelic]

update_or_create(...)... wow, this proves that there is always one more shortcut in Django to be learned.

Interestingly, it basically only wraps select_for_update().get_or_create(...) in a transaction.atomic(...), so it does precisely what you've been doing before. I take that as a sign that that's the right thing to do :).

I'll scan the code for other cases where update_or_create is appropriate.

awesome! I'll gladly review the PR when it's ready.

[n2ygk]

@jdelic I've scanned and don't see another obvious instance in oauth2_validators.py. There is some discussion in #609 from @giovannibelzoni about another possible case, which is in the OAuth2 server code I believe.... That should probably be a separate PR if it's still an issue.

[jdelic]

looks good to me (reviewed test output, code and context).

And I agree about the discussion in #609... without a repro, I'd say this PR covers the initial report. I'm going to keep my eyes open if I encounter the same pattern anywhere else in the code, too.

[lvm]

Hi there,
I'm still having django.db.utils.IntegrityError: duplicate key value violates unique constraint "oauth2_provider_accesstoken_source_refresh_token_id_key" DETAIL: Key (source_refresh_token_id)=(nn) already exists. error. Updated everything but still having this issue.

Current versions:

Django==2.1.1
django-oauth-toolkit==1.2.0

I've been forcing this error using this script.
Is there anything I could try to help solving this issue?

[n2ygk]

I've not touched the refresh token handling code but perhaps you could take a look at what I did in this PR and try the same approach for the refresh_token code.

[lvm]

@n2ygk great, sorry if my comment was a bit off-topic then :-)

[n2ygk]

Not at all. It's probably a similar error, just I don't have your environment to test against. It might be happening around here: https://github.com/jazzband/django-oauth-toolkit/blob/master/oauth2_provider/oauth2_validators.py#L521-L535

[lvm]

Right, I've been working on that piece, and that's why i ended up in this issue :-)
Will continue working on this and probably open a PR if I find a solution to this.

Thanks