Skip to content

Modify token auto refresh handling. #265

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.

Sign up for GitHub

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

Closed
wants to merge 7 commits into from
Closed

Modify token auto refresh handling. #265

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
cf3ff0a
Implement changes to allow control over token refresh authentication.
btimby Mar 3, 2017
f22426b
Place refresh_token() arguments under caller's control.
btimby Mar 3, 2017
e5e888a
Revert previous prototypes. Preserve behavior, with warnings.
btimby Mar 3, 2017
5e91016
Print and log warnings.
btimby Mar 3, 2017
fd7eaa5
Final edits.
btimby Mar 3, 2017
eee296a
Remove old duplicate warning.
btimby Mar 3, 2017
6960d8d
Address review.
btimby Mar 3, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
60 changes: 52 additions & 8 deletions requests_oauthlib/oauth2_session.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,15 +1,33 @@
from __future__ import unicode_literals

import logging
import warnings

from oauthlib.common import generate_token, urldecode
from oauthlib.oauth2 import WebApplicationClient, InsecureTransportError
from oauthlib.oauth2 import TokenExpiredError, is_secure_transport

import requests

log = logging.getLogger(__name__)


# Preserve so we can call it.
old_showwarning = warnings.showwarning

def logwarning(message, category, filename, lineno, file=None):
"""
Log warnings as well as print them.
"""
log.warning(
'%s:%s: %s:%s' %
(filename, lineno, category.__name__, message))
old_showwarning(message, category, filename, lineno, file=file)

# Install our new handler.
warnings.showwarning = logwarning
Copy link
Member

@Lukasa Lukasa Mar 3, 2017

Choose a reason for hiding this comment

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

I am very nervous about monkeypatching the warnings module. I'd rather not do it.

Copy link
Author

@btimby btimby Mar 3, 2017

Choose a reason for hiding this comment

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

https://docs.python.org/2/library/warnings.html#warnings.showwarning

Copy link
Member

@Lukasa Lukasa Mar 3, 2017

Choose a reason for hiding this comment

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

Sure, but it is still unnecessary for us to do this. We can just use a regular function, rather than change global state.



class TokenUpdated(Warning):
def __init__(self, token):
super(TokenUpdated, self).__init__()
Expand Down Expand Up @@ -279,9 +297,6 @@ def refresh_token(self, token_url, refresh_token=None, body='', auth=None,

refresh_token = refresh_token or self.token.get('refresh_token')

log.debug('Adding auto refresh key word arguments %s.',
self.auto_refresh_kwargs)
kwargs.update(self.auto_refresh_kwargs)
body = self._client.prepare_refresh_body(body=body,
refresh_token=refresh_token, scope=self.scope, **kwargs)
log.debug('Prepared refresh token request body %s', body)
Expand Down Expand Up @@ -334,13 +349,42 @@ def request(self, method, url, data=None, headers=None, withhold_token=False,
log.debug('Auto refresh is set, attempting to refresh at %s.',
self.auto_refresh_url)

# We mustn't pass auth twice.
auth = kwargs.pop('auth', None)
if client_id and client_secret and (auth is None):
# Someday this code can be eliminated, we should be able to
# simply pass **self.auto_refresh_kwargs to
# `refresh_token()` what follows is to maintain
# compatibility

# Start with kwargs explicitly requested for refresh.
refresh_kwargs = self.auto_refresh_kwargs.copy()
Copy link

@east825 east825 May 2, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

I wonder, isn't it going to change the semantic of auto_refresh_kwargs parameter? Previously it was intended to contain only key-value pairs to include into the body of the request, but now it also holds arguments of refresh_token() method such as auth, timeout, proxies, etc. For instance, if somebody wants to send auth parameter in the request body together with refresh_token and grant_type, this change will break their code.


if kwargs.get('auth', None):
# If user supplied `auth` to `request()` warn them to
# instead use `auto_refresh_kwargs`. But honor their
# intent for now.
warnings.warn('auth argument supplied. Please specify '
'token refresh authentication in '
'auto_refresh_kwargs.')
refresh_kwargs['auth'] = kwargs.pop('auth')
elif client_id:
# If no auth was provided, but `client_id` and
# `client_secret` were, warn the user and create an
# HTTP Basic auth header. It is preferred if the user
# instead place an auth param into `auto_refresh_kwargs`
warnings.warn('client_id argument supplied, Please '
'specify token refresh authentication in'
'auto_refresh_kwargs')
log.debug('Encoding client_id "%s" with client_secret as Basic auth credentials.', client_id)
auth = requests.auth.HTTPBasicAuth(client_id, client_secret)
if not client_secret:
warnings.warn('client_id provided, but '
'client_secret missing')
Copy link
Member

@Lukasa Lukasa Mar 3, 2017

Choose a reason for hiding this comment

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

Do we need this warning? I'm not certain how likely this is to be an error.

Copy link
Author

@btimby btimby Mar 3, 2017

Choose a reason for hiding this comment

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

I added it because the code replaces the missing argument with an empty string and continues with authentication. I thought that seemed like an omission on the callers part, something they might need warned about. This could be an incorrect assumption.

Copy link
Member

@Lukasa Lukasa Mar 3, 2017

Choose a reason for hiding this comment

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

Rather than do that, let's just require both the client ID and secret.

client_secret = ''
refresh_kwargs['auth'] = \
Copy link
Member

@Lukasa Lukasa Mar 3, 2017

Choose a reason for hiding this comment

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

A style note: we'd rather use implicit newlines inside parentheses than explicit ones.

Copy link
Author

@btimby btimby Mar 3, 2017

Choose a reason for hiding this comment

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

The newline does not show up in the resulting string, it is strictly for formatting, the two strings end up concatenated into one (single line) string.

Copy link
Author

@btimby btimby Mar 3, 2017

Choose a reason for hiding this comment

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

Example:

>>> print('this is a multi-'
... 'line string in the code but'
... ' a single line when printed.')
this is a multi-line string in the code but a single line when printed.

Copy link
Member

@Lukasa Lukasa Mar 3, 2017

Choose a reason for hiding this comment

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

I know. I'm saying don't use \ (the explicit line continuation character), use parentheses instead.

requests.auth.HTTPBasicAuth(client_id, client_secret)

# End of compat. code.

token = self.refresh_token(
self.auto_refresh_url, auth=auth, **kwargs
self.auto_refresh_url, **refresh_kwargs
)
if self.token_updater:
log.debug('Updating token to %s using %s.',
Expand Down
32 changes: 31 additions & 1 deletion tests/test_oauth2_session.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@
from oauthlib.oauth2 import WebApplicationClient, MobileApplicationClient
from oauthlib.oauth2 import LegacyApplicationClient, BackendApplicationClient
from requests_oauthlib import OAuth2Session, TokenUpdated

from requests.auth import HTTPBasicAuth

fake_time = time.time()

Expand Down Expand Up @@ -131,13 +131,43 @@ def fake_refresh_with_auth(r, **kwargs):
resp.text = json.dumps(self.token)
return resp

# Make sure client_id and client_secret are converted to Basic Auth.
# This is a deprecated implicit auth method and should be removed in
# favor of auto_refresh_kwargs.
for client in self.clients:
auth = OAuth2Session(client=client, token=self.expired_token,
auto_refresh_url='https://i.b/refresh',
token_updater=token_updater)
auth.send = fake_refresh_with_auth
auth.get('https://i.b', client_id='foo', client_secret='bar')

# Make sure auth param is passed through, this is a deprecated implicit
# auth method and should be removed in favor of auto_refresh_kwargs.
for client in self.clients:
auth = OAuth2Session(client=client, token=self.expired_token,
auto_refresh_url='https://i.b/refresh',
token_updater=token_updater)
auth.send = fake_refresh_with_auth
auth.get('https://i.b', auth=HTTPBasicAuth('foo', 'bar'))

def explicit_refresh_post(r, **kwargs):
if "/refresh" in r.url:
self.assertIn('foo-client-id', r.body)
self.assertIn('foo-client-secret', r.body)
self.assertNotIn("Authorization", r.headers)
resp = mock.MagicMock()
resp.text = json.dumps(self.token)
return resp

for client in self.clients:
auth = OAuth2Session(client=client, token=self.expired_token,
auto_refresh_url='https://i.b/refresh',
auto_refresh_kwargs={'client_id': 'foo-client-id',
'client_secret': 'foo-client-secret'},
token_updater=token_updater)
auth.send = explicit_refresh_post
auth.post('https://i.b')

@mock.patch("time.time", new=lambda: fake_time)
def test_token_from_fragment(self):
mobile = MobileApplicationClient(self.client_id)
Expand Down