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

Don't nullify token before refresh #219

Merged
merged 3 commits into from
Jan 12, 2016
Merged

Conversation

dsanader
Copy link
Contributor

Hello,

I'm using an OAuth2Session object, with the token autorefresh feature, inside a library. This involves some level of concurrent requests.

I've noticed that some requests were issued without the Authorization header. This was happening around the refresh requests. I've tracked it to the fact that the token was nullified inside the refresh_token method, causing my OAuth2Session instance to lack a token for a short period of time (until a new one is fetched).

The attached changes fixed the issue for me (no token reset, and issue a regular requests.post instead of using the class equivalent). That way the token is simply replaced.

Regards.

Avoids issuing requests lacking the Authorization header until a new
token is fetched
@Lukasa
Copy link
Member

Lukasa commented Jan 12, 2016

Unfortunately, this fix isn't going to work.

Using a normal post instead of the Session one means that a whole lot of settings the user may have applied via Transport Adapters, or on the Session itself. While the fix may work specifically in your case, it is unlikely to work generally and a little surprising.

It may be more fruitful to add a flag that can be passed into post (and that ends up in OAuth2Session.request) that skips the if self.token logic in the OAuth2Session.request method. That should be fairly safe, and will ensure that the fix behaves properly in the general case.

Are you interested in making that fix?

Keep on using self.post instead of requests.post
@dsanader
Copy link
Contributor Author

I must confess I haven't given much thought to the bigger picture.

I think I've updated my branch accordingly. Is this what you meant ?

@Lukasa
Copy link
Member

Lukasa commented Jan 12, 2016

Yup, that's a lot closer! The name isn't great though: let's call it withhold_token.

@dsanader
Copy link
Contributor Author

Flag renamed.

@@ -289,7 +287,7 @@ def refresh_token(self, token_url, refresh_token=None, body='', auth=None,
}

r = self.post(token_url, data=dict(urldecode(body)), auth=auth,
timeout=timeout, headers=headers, verify=verify)
timeout=timeout, headers=headers, verify=verify, withhold_token=True)
Copy link
Member

Choose a reason for hiding this comment

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

Can we fix up the indenting here? You'll probably need to move withold_token to the next line to make everything fit an appropriate line length.

@Lukasa
Copy link
Member

Lukasa commented Jan 12, 2016

Fantastic, thanks! One small style nit and we're good to go!

@dsanader
Copy link
Contributor Author

I've matched indenting to the way it looked in the fetch_token method (lines 212 and 218).

Should I fix these lines too ?

@Lukasa
Copy link
Member

Lukasa commented Jan 12, 2016

@dsanader Ah, good spot. Let's leave it then.

Thanks! ✨ 🍰 ✨

Lukasa added a commit that referenced this pull request Jan 12, 2016
Don't nullify token before refresh
@Lukasa Lukasa merged commit 25908e5 into requests:master Jan 12, 2016
@dsanader
Copy link
Contributor Author

Cool! Thanks for you quick and kind review.

Cheers.

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

2 participants