Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Prevent racecondition on consuming refresh token. #558

Merged

Conversation

phillbaker
Copy link
Contributor

@phillbaker phillbaker commented Feb 16, 2018

From checking to see whether the refresh token is valid in validate_refresh_token to when it's revoked in save_bearer_token in oauth2_provider/oauth2_validators.py, multiple requests can validate the same refresh token (where it's then stored in memory) and generate multiple access tokens from it. While this is allowed by the spec and explicitly configurable via the ROTATE_REFRESH_TOKEN, it seems like unexpected behavior.

To reproduce this, a fully concurrent example should be used, e.g. the demo with postgres and gunicorn with multiple workers and concurrent requests. This script will return multiple access tokens:

#!/bin/bash

curl 'http://localhost:8000/o/token/' -H 'Content-Type: application/x-www-form-urlencoded; charset=UTF-8' -H 'Authorization: Basic Y2xpZW50X2lkOmNsaWVudF9zZWNyZXQ=' -H 'Cookie: csrftoken=jmf3nQGZvhyMNhXwyEQ1dbAEHmQOgGGiFd54calZsDibfsXpBV6SlpxaFqF1nH9u; sessionid=6k57hsukvq11bhnewj2uaap832o970me'  --data 'refresh_token=JRfemZ8y1WM99qLNPcOsUJsXVkAUCE&grant_type=refresh_token' &
curl 'http://localhost:8000/o/token/' -H 'Content-Type: application/x-www-form-urlencoded; charset=UTF-8' -H 'Authorization: Basic Y2xpZW50X2lkOmNsaWVudF9zZWNyZXQ=' -H 'Cookie: csrftoken=jmf3nQGZvhyMNhXwyEQ1dbAEHmQOgGGiFd54calZsDibfsXpBV6SlpxaFqF1nH9u; sessionid=6k57hsukvq11bhnewj2uaap832o970me'  --data 'refresh_token=JRfemZ8y1WM99qLNPcOsUJsXVkAUCE&grant_type=refresh_token' &

Example output:

{"scope": "example", "token_type": "Bearer", "access_token": "272TOnLxwwaVHw0hzHPiF7gNXR0LgO", "refresh_token": "iAMVKoSNbnGE0wnX7iH4I6yyAYEy2e", "expires_in": 36000}{"scope": "example", "token_type": "Bearer", "access_token": "C29N6Q0ZcY4z9P2wOU64ZxGIzz7V4E", "refresh_token": "kcNlG3vPRNJatnnD5AoDiav2CGlY3X", "expires_in": 36000}{"scope": "example", "token_type": "Bearer", "access_token": "OSmIGe2fTBSvE5MrLIecNl3HU4rvnG", "refresh_token": "kl47VelyvVPrCV0a2WfadGjjBb1hhU", "expires_in": 36000}

It looks like this existed before #543, but might have been made a bit worse in moving from the .delete() to marking the refresh token as revoked (note that delete() can be called multiple times on an object without blowing up). I think this is related to #389, but that was solving a slightly different issue.

This PR takes out a lock on the refresh token before revoking it and allowing the request to proceed. I was not sure how to reproduce the scenario in tests - currently travis uses sqlite, which can't reproduce the issue because it's not concurrent.

@coveralls
Copy link

coveralls commented Feb 16, 2018

Coverage Status

Coverage decreased (-0.1%) to 93.487% when pulling e7e6a2e on phillbaker:bugfix-race-condition-refreesh into 2e4d15e on evonove:master.

Copy link
Member

@jleclanche jleclanche left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

one minor change, looks good otherwise

self.save()
with transaction.atomic():
access_token_model = get_access_token_model()
refresh_token_model = get_refresh_token_model()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Those first two lines don't need to be in transaction.atomic().

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, updated!

@jleclanche jleclanche merged commit 281b46c into jazzband:master Feb 18, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants