From 22a9bc6b526bac48ecb9e05c7188003400894936 Mon Sep 17 00:00:00 2001 From: jonathan vanasco Date: Fri, 14 Sep 2018 19:06:38 -0400 Subject: [PATCH 1/3] first attempt --- HISTORY.rst | 12 ++ requests_oauthlib/oauth2_session.py | 104 +++++++++++++---- tests/test_compliance_fixes.py | 28 ++--- tests/test_oauth2_session.py | 175 ++++++++++++++++++++++------ 4 files changed, 246 insertions(+), 73 deletions(-) diff --git a/HISTORY.rst b/HISTORY.rst index 88267555..c958bcf8 100644 --- a/HISTORY.rst +++ b/HISTORY.rst @@ -6,6 +6,18 @@ UNRELEASED nothing yet +- Updated oauth2 tests to use 'sess' for an OAuth2Session instance instead of `auth` + because OAuth2Session objects and methods acceept an `auth` paramether which is + typically an instance of `requests.auth.HTTPBasicAuth` + +- `OAuth2Session.fetch_token` previously tried to guess how and where to provide + "client" and "user" credentials incorrectly. This was incompatible with some + OAuth servers and incompatible with breaking changes in oauthlib that seek to + correctly provide the `client_id`. The older implementation also did not raise + the correct exceptions when username and password are not present on Legacy + clients. + + v1.0.0 (4 June 2018) ++++++++++++++++++++ diff --git a/requests_oauthlib/oauth2_session.py b/requests_oauthlib/oauth2_session.py index 7ad7b46c..ba01deff 100644 --- a/requests_oauthlib/oauth2_session.py +++ b/requests_oauthlib/oauth2_session.py @@ -4,6 +4,7 @@ from oauthlib.common import generate_token, urldecode from oauthlib.oauth2 import WebApplicationClient, InsecureTransportError +from oauthlib.oauth2 import LegacyApplicationClient from oauthlib.oauth2 import TokenExpiredError, is_secure_transport import requests @@ -154,11 +155,14 @@ def authorization_url(self, url, state=None, **kwargs): def fetch_token(self, token_url, code=None, authorization_response=None, body='', auth=None, username=None, password=None, method='POST', - timeout=None, headers=None, verify=True, proxies=None, **kwargs): + timeout=None, headers=None, verify=True, proxies=None, + include_client_id=None, client_secret=None, **kwargs): """Generic method for fetching an access token from the token endpoint. If you are using the MobileApplicationClient you will want to use - token_from_fragment instead of fetch_token. + `token_from_fragment` instead of `fetch_token`. + + The current implementation enforces the RFC guidelines. :param token_url: Token endpoint URL, must use HTTPS. :param code: Authorization code (used by WebApplicationClients). @@ -167,15 +171,26 @@ def fetch_token(self, token_url, code=None, authorization_response=None, WebApplicationClients instead of code. :param body: Optional application/x-www-form-urlencoded body to add the include in the token request. Prefer kwargs over body. - :param auth: An auth tuple or method as accepted by requests. - :param username: Username used by LegacyApplicationClients. - :param password: Password used by LegacyApplicationClients. + :param auth: An auth tuple or method as accepted by `requests`. + :param username: Username required by LegacyApplicationClients to appear + in the request body. + :param password: Password required by LegacyApplicationClients to appear + in the request body. :param method: The HTTP method used to make the request. Defaults to POST, but may also be GET. Other methods should be added as needed. - :param headers: Dict to default request headers with. :param timeout: Timeout of the request in seconds. + :param headers: Dict to default request headers with. :param verify: Verify SSL certificate. + :param proxies: The `proxies` argument is passed onto `requests`. + :param include_client_id: Should the request body include the + `client_id` parameter. Default is `None`, + which will attempt to autodetect. This can be + forced to always include (True) or never + include (False). + :param client_secret: The `client_secret` paired to the `client_id`. + This is generally required unless provided in the + `auth` tuple. :param kwargs: Extra parameters to include in the token request. :return: A token dict """ @@ -192,23 +207,66 @@ def fetch_token(self, token_url, code=None, authorization_response=None, raise ValueError('Please supply either code or ' 'authorization_response parameters.') + # Earlier versions of this library build an HTTPBasicAuth header out of + # `username` and `password`. The RFC states, however these attributes + # must be in the request body and not the header. + # If an upstream server is not spec compliant and requires them to + # appear as an Authorization header, supply an explicit `auth` header + # to this function. + # This check will allow for empty strings, but not `None`. + # + # Refernences + # 4.3.2 - Resource Owner Password Credentials Grant + # https://tools.ietf.org/html/rfc6749#section-4.3.2 + + if isinstance(self._client, LegacyApplicationClient): + if username is None: + raise ValueError('`LegacyApplicationClient` requires both the ' + '`username` and `password` parameters.') + if password is None: + raise ValueError('The required paramter `username` was supplied, ' + 'but `password` was not.') + + # TODO: + # username or password could be allowed in other requests? + if username is not None: + kwargs['username'] = username + if password is not None: + kwargs['password'] = password + + # is an auth explicitly supplied? + if auth is not None: + # if we're dealing with the default of `include_client_id` (None): + # we will assume the `auth` argument is for an RFC compliant server + # and we should not send the `client_id` in the body. + # This approach allows us to still force the client_id by submitting + # `include_client_id=True` along with an `auth` object. + if include_client_id is None: + include_client_id = False + + # otherwise we may need to create an auth header + else: + # since we don't have an auth header, we MAY need to create one + # it is possible that we want to send the `client_id` in the body + # if so, `include_client_id` should be set to True + # otherwise, we will generate an auth header + if include_client_id is not True: + client_id = self.client_id + if client_id: + log.debug('Encoding `client_id` "%s" with `client_secret` ' + 'as Basic auth credentials.', client_id) + client_secret = client_secret if client_secret is not None else '' + auth = requests.auth.HTTPBasicAuth(client_id, client_secret) + + if include_client_id: + # this was pulled out of the params + # it needs to be passed into prepare_request_body + if client_secret: + kwargs['client_secret'] = client_secret body = self._client.prepare_request_body(code=code, body=body, - redirect_uri=self.redirect_uri, username=username, - password=password, **kwargs) - - client_id = kwargs.get('client_id', '') - if auth is None: - if client_id: - log.debug('Encoding client_id "%s" with client_secret as Basic auth credentials.', client_id) - client_secret = kwargs.get('client_secret', '') - client_secret = client_secret if client_secret is not None else '' - auth = requests.auth.HTTPBasicAuth(client_id, client_secret) - elif username: - if password is None: - raise ValueError('Username was supplied, but not password.') - log.debug('Encoding username, password as Basic auth credentials.') - auth = requests.auth.HTTPBasicAuth(username, password) + redirect_uri=self.redirect_uri, + include_client_id=include_client_id, **kwargs) headers = headers or { 'Accept': 'application/json', @@ -265,9 +323,11 @@ def refresh_token(self, token_url, refresh_token=None, body='', auth=None, :param refresh_token: The refresh_token to use. :param body: Optional application/x-www-form-urlencoded body to add the include in the token request. Prefer kwargs over body. - :param auth: An auth tuple or method as accepted by requests. + :param auth: An auth tuple or method as accepted by `requests`. :param timeout: Timeout of the request in seconds. + :param headers: A dict of headers to be used by `requests`. :param verify: Verify SSL certificate. + :param proxies: The `proxies` argument will be passed to `requests`. :param kwargs: Extra parameters to include in the token request. :return: A token dict """ diff --git a/tests/test_compliance_fixes.py b/tests/test_compliance_fixes.py index 05722c36..7510a6da 100644 --- a/tests/test_compliance_fixes.py +++ b/tests/test_compliance_fixes.py @@ -32,13 +32,13 @@ def setUp(self): mocker.start() self.addCleanup(mocker.stop) - facebook = OAuth2Session('foo', redirect_uri='https://i.b') + facebook = OAuth2Session('someclientid', redirect_uri='https://i.b') self.session = facebook_compliance_fix(facebook) def test_fetch_access_token(self): token = self.session.fetch_token( 'https://graph.facebook.com/oauth/access_token', - client_secret='bar', + client_secret='someclientsecret', authorization_response='https://i.b/?code=hello', ) self.assertEqual(token, {'access_token': 'urlencoded', 'token_type': 'Bearer'}) @@ -55,7 +55,7 @@ def setUp(self): self.mocker.start() self.addCleanup(self.mocker.stop) - fitbit = OAuth2Session('foo', redirect_uri='https://i.b') + fitbit = OAuth2Session('someclientid', redirect_uri='https://i.b') self.session = fitbit_compliance_fix(fitbit) def test_fetch_access_token(self): @@ -63,7 +63,7 @@ def test_fetch_access_token(self): InvalidGrantError, self.session.fetch_token, 'https://api.fitbit.com/oauth2/token', - client_secret='bar', + client_secret='someclientsecret', authorization_response='https://i.b/?code=hello', ) @@ -84,7 +84,7 @@ def test_refresh_token(self): InvalidGrantError, self.session.refresh_token, 'https://api.fitbit.com/oauth2/token', - auth=requests.auth.HTTPBasicAuth('foo', 'bar') + auth=requests.auth.HTTPBasicAuth('someclientid', 'someclientsecret') ) self.mocker.post( @@ -94,7 +94,7 @@ def test_refresh_token(self): token = self.session.refresh_token( 'https://api.fitbit.com/oauth2/token', - auth=requests.auth.HTTPBasicAuth('foo', 'bar') + auth=requests.auth.HTTPBasicAuth('someclientid', 'someclientsecret') ) self.assertEqual(token['access_token'], 'access') @@ -120,13 +120,13 @@ def setUp(self): mocker.start() self.addCleanup(mocker.stop) - linkedin = OAuth2Session('foo', redirect_uri='https://i.b') + linkedin = OAuth2Session('someclientid', redirect_uri='https://i.b') self.session = linkedin_compliance_fix(linkedin) def test_fetch_access_token(self): token = self.session.fetch_token( 'https://www.linkedin.com/uas/oauth2/accessToken', - client_secret='bar', + client_secret='someclientsecret', authorization_response='https://i.b/?code=hello', ) self.assertEqual(token, {'access_token': 'linkedin', 'token_type': 'Bearer'}) @@ -152,13 +152,13 @@ def setUp(self): mocker.start() self.addCleanup(mocker.stop) - mailchimp = OAuth2Session('foo', redirect_uri='https://i.b') + mailchimp = OAuth2Session('someclientid', redirect_uri='https://i.b') self.session = mailchimp_compliance_fix(mailchimp) def test_fetch_access_token(self): token = self.session.fetch_token( "https://login.mailchimp.com/oauth2/token", - client_secret='bar', + client_secret='someclientsecret', authorization_response='https://i.b/?code=hello', ) # Times should be close @@ -184,13 +184,13 @@ def setUp(self): mocker.start() self.addCleanup(mocker.stop) - weibo = OAuth2Session('foo', redirect_uri='https://i.b') + weibo = OAuth2Session('someclientid', redirect_uri='https://i.b') self.session = weibo_compliance_fix(weibo) def test_fetch_access_token(self): token = self.session.fetch_token( 'https://api.weibo.com/oauth2/access_token', - client_secret='bar', + client_secret='someclientsecret', authorization_response='https://i.b/?code=hello', ) self.assertEqual(token, {'access_token': 'weibo', 'token_type': 'Bearer'}) @@ -223,7 +223,7 @@ def setUp(self): mocker.start() self.addCleanup(mocker.stop) - slack = OAuth2Session('foo', redirect_uri='https://i.b') + slack = OAuth2Session('someclientid', redirect_uri='https://i.b') self.session = slack_compliance_fix(slack) def test_protected_request(self): @@ -293,7 +293,7 @@ def setUp(self): mocker.start() self.addCleanup(mocker.stop) - plentymarkets = OAuth2Session('foo', redirect_uri='https://i.b') + plentymarkets = OAuth2Session('someclientid', redirect_uri='https://i.b') self.session = plentymarkets_compliance_fix(plentymarkets) def test_fetch_access_token(self): diff --git a/tests/test_oauth2_session.py b/tests/test_oauth2_session.py index e5892cab..5a980e25 100644 --- a/tests/test_oauth2_session.py +++ b/tests/test_oauth2_session.py @@ -12,10 +12,12 @@ from oauthlib.oauth2 import WebApplicationClient, MobileApplicationClient from oauthlib.oauth2 import LegacyApplicationClient, BackendApplicationClient from requests_oauthlib import OAuth2Session, TokenUpdated +import requests +from requests.auth import _basic_auth_str -fake_time = time.time() +fake_time = time.time() def fake_token(token): @@ -37,16 +39,25 @@ def setUp(self): 'token_type': 'Bearer', 'access_token': 'asdfoiw37850234lkjsdfsdf', 'refresh_token': 'sldvafkjw34509s8dfsdf', - 'expires_in': '3600', + 'expires_in': 3600, 'expires_at': fake_time + 3600, } - self.client_id = 'foo' + # use someclientid:someclientsecret to easily differentiate between client and user credentials + # these are the values used in oauthlib tests + self.client_id = 'someclientid' + self.client_secret = 'someclientsecret' + self.user_username = 'user_username' + self.user_password = 'user_password' + self.client_WebApplication = WebApplicationClient(self.client_id, code='asdf345xdf') + self.client_LegacyApplication = LegacyApplicationClient(self.client_id) + self.client_BackendApplication = BackendApplicationClient(self.client_id) + self.client_MobileApplication = MobileApplicationClient(self.client_id) self.clients = [ - WebApplicationClient(self.client_id, code='asdf345xdf'), - LegacyApplicationClient(self.client_id), - BackendApplicationClient(self.client_id), + self.client_WebApplication, + self.client_LegacyApplication, + self.client_BackendApplication, ] - self.all_clients = self.clients + [MobileApplicationClient(self.client_id)] + self.all_clients = self.clients + [self.client_MobileApplication, ] def test_add_token(self): token = 'Bearer ' + self.token['access_token'] @@ -59,9 +70,9 @@ def verifier(r, **kwargs): return resp for client in self.all_clients: - auth = OAuth2Session(client=client, token=self.token) - auth.send = verifier - auth.get('https://i.b') + sess = OAuth2Session(client=client, token=self.token) + sess.send = verifier + sess.get('https://i.b') def test_authorization_url(self): url = 'https://example.com/authorize?foo=bar' @@ -95,31 +106,31 @@ def fake_refresh(r, **kwargs): # No auto refresh setup for client in self.clients: - auth = OAuth2Session(client=client, token=self.expired_token) - self.assertRaises(TokenExpiredError, auth.get, 'https://i.b') + sess = OAuth2Session(client=client, token=self.expired_token) + self.assertRaises(TokenExpiredError, sess.get, 'https://i.b') # Auto refresh but no auto update for client in self.clients: - auth = OAuth2Session(client=client, token=self.expired_token, + sess = OAuth2Session(client=client, token=self.expired_token, auto_refresh_url='https://i.b/refresh') - auth.send = fake_refresh - self.assertRaises(TokenUpdated, auth.get, 'https://i.b') + sess.send = fake_refresh + self.assertRaises(TokenUpdated, sess.get, 'https://i.b') # Auto refresh and auto update def token_updater(token): self.assertEqual(token, self.token) for client in self.clients: - auth = OAuth2Session(client=client, token=self.expired_token, + sess = OAuth2Session(client=client, token=self.expired_token, auto_refresh_url='https://i.b/refresh', token_updater=token_updater) - auth.send = fake_refresh - auth.get('https://i.b') + sess.send = fake_refresh + sess.get('https://i.b') def fake_refresh_with_auth(r, **kwargs): if "/refresh" in r.url: self.assertIn("Authorization", r.headers) - encoded = b64encode(b"foo:bar") + encoded = b64encode(b"%s:%s" % (self.client_id, self.client_secret)) content = (b"Basic " + encoded).decode('latin1') self.assertEqual(r.headers["Authorization"], content) resp = mock.MagicMock() @@ -127,33 +138,106 @@ def fake_refresh_with_auth(r, **kwargs): return resp for client in self.clients: - auth = OAuth2Session(client=client, token=self.expired_token, + sess = 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') + sess.send = fake_refresh_with_auth + sess.get('https://i.b', client_id=self.client_id, client_secret=self.client_secret) @mock.patch("time.time", new=lambda: fake_time) def test_token_from_fragment(self): mobile = MobileApplicationClient(self.client_id) response_url = 'https://i.b/callback#' + urlencode(self.token.items()) - auth = OAuth2Session(client=mobile) - self.assertEqual(auth.token_from_fragment(response_url), self.token) + sess = OAuth2Session(client=mobile) + self.assertEqual(sess.token_from_fragment(response_url), self.token) @mock.patch("time.time", new=lambda: fake_time) def test_fetch_token(self): url = 'https://example.com/token' for client in self.clients: - auth = OAuth2Session(client=client, token=self.token) - auth.send = fake_token(self.token) - self.assertEqual(auth.fetch_token(url), self.token) + sess = OAuth2Session(client=client, token=self.token) + sess.send = fake_token(self.token) + if isinstance(client, LegacyApplicationClient): + # this client requires a username+password + # if unset, an error will be raised + self.assertRaises(ValueError, sess.fetch_token, url) + self.assertRaises(ValueError, sess.fetch_token, url, username='username1',) + self.assertRaises(ValueError, sess.fetch_token, url, password='password1',) + # otherwise it will pass + self.assertEqual(sess.fetch_token(url, username='username1', password='password1'), self.token) + else: + self.assertEqual(sess.fetch_token(url), self.token) error = {'error': 'invalid_request'} for client in self.clients: - auth = OAuth2Session(client=client, token=self.token) - auth.send = fake_token(error) - self.assertRaises(OAuth2Error, auth.fetch_token, url) + sess = OAuth2Session(client=client, token=self.token) + sess.send = fake_token(error) + if isinstance(client, LegacyApplicationClient): + # this client requires a username+password + # if unset, an error will be raised + self.assertRaises(ValueError, sess.fetch_token, url) + self.assertRaises(ValueError, sess.fetch_token, url, username='username1',) + self.assertRaises(ValueError, sess.fetch_token, url, password='password1',) + # otherwise it will pass + self.assertRaises(OAuth2Error, sess.fetch_token, url, username='username1', password='password1') + else: + self.assertRaises(OAuth2Error, sess.fetch_token, url) + + # there are different scenarios in which the client_id can be specified + # reference `oauthlib.tests.oauth2.rfc6749.clients.test_web_application.WebApplicationClientTest.test_prepare_request_body` + # this only needs to test WebApplicationClient + client = self.client_WebApplication + client.tester = True + + # this should be a tuple of (r.url, r.body, r.headers.get('Authorization')) + _fetch_history = [] + + def fake_token_history(token): + def fake_send(r, **kwargs): + resp = mock.MagicMock() + resp.text = json.dumps(token) + _fetch_history.append((r.url, r.body, r.headers.get('Authorization', None))) + return resp + return fake_send + + sess = OAuth2Session(client=client, token=self.token) + sess.send = fake_token_history(self.token) + expected_auth_header = _basic_auth_str(self.client_id, self.client_secret) + + # scenario 1 - default request + # this should send the client_id in the headers, as that is recommended by the RFC + self.assertEqual(sess.fetch_token(url, client_secret='someclientsecret'), self.token) + self.assertEqual(len(_fetch_history), 1) + self.assertNotIn('client_id', _fetch_history[0][1]) # no client_id in the body + self.assertNotIn('client_secret', _fetch_history[0][1]) # no client_secret in the body + self.assertEqual(_fetch_history[0][2], expected_auth_header) # ensure a Basic Authorization header + + # scenario 2 - force the client_id into the body + # this should send the client_id in the headers, as that is recommended by the RFC + self.assertEqual(sess.fetch_token(url, client_secret='someclientsecret', include_client_id=True), self.token) + self.assertEqual(len(_fetch_history), 2) + self.assertIn('client_id=%s' % self.client_id, _fetch_history[1][1]) + self.assertIn('client_secret=%s' % self.client_secret, _fetch_history[1][1]) + self.assertEqual(_fetch_history[1][2], None) # ensure NO Basic Authorization header + + # scenario 3 - send in an auth object + auth = requests.auth.HTTPBasicAuth(self.client_id, self.client_secret) + self.assertEqual(sess.fetch_token(url, auth=auth), self.token) + self.assertEqual(len(_fetch_history), 3) + self.assertNotIn('client_id', _fetch_history[2][1]) # no client_id in the body + self.assertNotIn('client_secret', _fetch_history[2][1]) # no client_secret in the body + self.assertEqual(_fetch_history[2][2], expected_auth_header) # ensure a Basic Authorization header + + # scneario 4 - send in a username/password combo + # this should send the client_id in the headers, like scenario 1 + self.assertEqual(sess.fetch_token(url, username=self.user_username, password=self.user_password), self.token) + self.assertEqual(len(_fetch_history), 4) + self.assertNotIn('client_id', _fetch_history[3][1]) # no client_id in the body + self.assertNotIn('client_secret', _fetch_history[3][1]) # no client_secret in the body + self.assertEqual(_fetch_history[0][2], expected_auth_header) # ensure a Basic Authorization header + self.assertIn('username=%s' % self.user_username, _fetch_history[3][1]) + self.assertIn('password=%s' % self.user_password, _fetch_history[3][1]) def test_cleans_previous_token_before_fetching_new_one(self): """Makes sure the previous token is cleaned before fetching a new one. @@ -172,14 +256,22 @@ def test_cleans_previous_token_before_fetching_new_one(self): with mock.patch('time.time', lambda: now): for client in self.clients: - auth = OAuth2Session(client=client, token=self.token) - auth.send = fake_token(new_token) - self.assertEqual(auth.fetch_token(url), new_token) - + sess = OAuth2Session(client=client, token=self.token) + sess.send = fake_token(new_token) + if isinstance(client, LegacyApplicationClient): + # this client requires a username+password + # if unset, an error will be raised + self.assertRaises(ValueError, sess.fetch_token, url) + self.assertRaises(ValueError, sess.fetch_token, url, username='username1',) + self.assertRaises(ValueError, sess.fetch_token, url, password='password1',) + # otherwise it will pass + self.assertEqual(sess.fetch_token(url, username='username1', password='password1'), new_token) + else: + self.assertEqual(sess.fetch_token(url), new_token) def test_web_app_fetch_token(self): # Ensure the state parameter is used, see issue #105. - client = OAuth2Session('foo', state='somestate') + client = OAuth2Session('someclientid', state='somestate') self.assertRaises(MismatchingStateError, client.fetch_token, 'https://i.b/token', authorization_response='https://i.b/no-state?code=abc') @@ -224,7 +316,7 @@ def test_token_proxy(self): del sess.token def test_authorized_false(self): - sess = OAuth2Session('foo') + sess = OAuth2Session('someclientid') self.assertFalse(sess.authorized) @mock.patch("time.time", new=lambda: fake_time) @@ -241,5 +333,14 @@ def fake_send(r, **kwargs): sess = OAuth2Session(client=client) sess.send = fake_token(self.token) self.assertFalse(sess.authorized) - sess.fetch_token(url) + if isinstance(client, LegacyApplicationClient): + # this client requires a username+password + # if unset, an error will be raised + self.assertRaises(ValueError, sess.fetch_token, url) + self.assertRaises(ValueError, sess.fetch_token, url, username='username1',) + self.assertRaises(ValueError, sess.fetch_token, url, password='password1',) + # otherwise it will pass + sess.fetch_token(url, username='username1', password='password1') + else: + sess.fetch_token(url) self.assertTrue(sess.authorized) From d6ffb1fca38fc41bee3e66627768552b810e8b28 Mon Sep 17 00:00:00 2001 From: jonathan vanasco Date: Mon, 17 Sep 2018 13:30:34 -0400 Subject: [PATCH 2/3] ensuring an empty string can be sent for `client_secret` --- requests_oauthlib/oauth2_session.py | 6 +++-- tests/test_oauth2_session.py | 37 ++++++++++++++++++++--------- 2 files changed, 30 insertions(+), 13 deletions(-) diff --git a/requests_oauthlib/oauth2_session.py b/requests_oauthlib/oauth2_session.py index ba01deff..8d6ae50c 100644 --- a/requests_oauthlib/oauth2_session.py +++ b/requests_oauthlib/oauth2_session.py @@ -190,7 +190,9 @@ def fetch_token(self, token_url, code=None, authorization_response=None, include (False). :param client_secret: The `client_secret` paired to the `client_id`. This is generally required unless provided in the - `auth` tuple. + `auth` tuple. If the value is `None`, it will be + omitted from the request, however if the value is + an empty string, an empty string will be sent. :param kwargs: Extra parameters to include in the token request. :return: A token dict """ @@ -261,7 +263,7 @@ def fetch_token(self, token_url, code=None, authorization_response=None, if include_client_id: # this was pulled out of the params # it needs to be passed into prepare_request_body - if client_secret: + if client_secret is not None: kwargs['client_secret'] = client_secret body = self._client.prepare_request_body(code=code, body=body, diff --git a/tests/test_oauth2_session.py b/tests/test_oauth2_session.py index 5a980e25..223d953e 100644 --- a/tests/test_oauth2_session.py +++ b/tests/test_oauth2_session.py @@ -184,7 +184,7 @@ def test_fetch_token(self): else: self.assertRaises(OAuth2Error, sess.fetch_token, url) - # there are different scenarios in which the client_id can be specified + # there are different scenarios in which the `client_id` can be specified # reference `oauthlib.tests.oauth2.rfc6749.clients.test_web_application.WebApplicationClientTest.test_prepare_request_body` # this only needs to test WebApplicationClient client = self.client_WebApplication @@ -206,15 +206,14 @@ def fake_send(r, **kwargs): expected_auth_header = _basic_auth_str(self.client_id, self.client_secret) # scenario 1 - default request - # this should send the client_id in the headers, as that is recommended by the RFC + # this should send the `client_id` in the headers, as that is recommended by the RFC self.assertEqual(sess.fetch_token(url, client_secret='someclientsecret'), self.token) self.assertEqual(len(_fetch_history), 1) - self.assertNotIn('client_id', _fetch_history[0][1]) # no client_id in the body - self.assertNotIn('client_secret', _fetch_history[0][1]) # no client_secret in the body + self.assertNotIn('client_id', _fetch_history[0][1]) # no `client_id` in the body + self.assertNotIn('client_secret', _fetch_history[0][1]) # no `client_secret` in the body self.assertEqual(_fetch_history[0][2], expected_auth_header) # ensure a Basic Authorization header - # scenario 2 - force the client_id into the body - # this should send the client_id in the headers, as that is recommended by the RFC + # scenario 2 - force the `client_id` into the body self.assertEqual(sess.fetch_token(url, client_secret='someclientsecret', include_client_id=True), self.token) self.assertEqual(len(_fetch_history), 2) self.assertIn('client_id=%s' % self.client_id, _fetch_history[1][1]) @@ -225,20 +224,36 @@ def fake_send(r, **kwargs): auth = requests.auth.HTTPBasicAuth(self.client_id, self.client_secret) self.assertEqual(sess.fetch_token(url, auth=auth), self.token) self.assertEqual(len(_fetch_history), 3) - self.assertNotIn('client_id', _fetch_history[2][1]) # no client_id in the body - self.assertNotIn('client_secret', _fetch_history[2][1]) # no client_secret in the body + self.assertNotIn('client_id', _fetch_history[2][1]) # no `client_id` in the body + self.assertNotIn('client_secret', _fetch_history[2][1]) # no `client_secret` in the body self.assertEqual(_fetch_history[2][2], expected_auth_header) # ensure a Basic Authorization header # scneario 4 - send in a username/password combo - # this should send the client_id in the headers, like scenario 1 + # this should send the `client_id` in the headers, like scenario 1 self.assertEqual(sess.fetch_token(url, username=self.user_username, password=self.user_password), self.token) self.assertEqual(len(_fetch_history), 4) - self.assertNotIn('client_id', _fetch_history[3][1]) # no client_id in the body - self.assertNotIn('client_secret', _fetch_history[3][1]) # no client_secret in the body + self.assertNotIn('client_id', _fetch_history[3][1]) # no `client_id` in the body + self.assertNotIn('client_secret', _fetch_history[3][1]) # no `client_secret` in the body self.assertEqual(_fetch_history[0][2], expected_auth_header) # ensure a Basic Authorization header self.assertIn('username=%s' % self.user_username, _fetch_history[3][1]) self.assertIn('password=%s' % self.user_password, _fetch_history[3][1]) + # some quick tests for valid ways of supporting `client_secret` + + # scenario 2b - force the `client_id` into the body; but the `client_secret` is `None` + self.assertEqual(sess.fetch_token(url, client_secret=None, include_client_id=True), self.token) + self.assertEqual(len(_fetch_history), 5) + self.assertIn('client_id=%s' % self.client_id, _fetch_history[4][1]) + self.assertNotIn('client_secret', _fetch_history[4][1]) # no `client_secret` in the body + self.assertEqual(_fetch_history[4][2], None) # ensure NO Basic Authorization header + + # scenario 2c - force the `client_id` into the body; but the `client_secret` is an empty string + self.assertEqual(sess.fetch_token(url, client_secret='', include_client_id=True), self.token) + self.assertEqual(len(_fetch_history), 6) + self.assertIn('client_id=%s' % self.client_id, _fetch_history[5][1]) + self.assertIn('client_secret=', _fetch_history[5][1]) + self.assertEqual(_fetch_history[5][2], None) # ensure NO Basic Authorization header + def test_cleans_previous_token_before_fetching_new_one(self): """Makes sure the previous token is cleaned before fetching a new one. From ad8277b87523551f263971b737c4e5e470c93e94 Mon Sep 17 00:00:00 2001 From: jonathan vanasco Date: Mon, 1 Oct 2018 13:08:27 -0400 Subject: [PATCH 3/3] removed todo, clarified comment --- requests_oauthlib/oauth2_session.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/requests_oauthlib/oauth2_session.py b/requests_oauthlib/oauth2_session.py index 8d6ae50c..5b8f7857 100644 --- a/requests_oauthlib/oauth2_session.py +++ b/requests_oauthlib/oauth2_session.py @@ -229,8 +229,7 @@ def fetch_token(self, token_url, code=None, authorization_response=None, raise ValueError('The required paramter `username` was supplied, ' 'but `password` was not.') - # TODO: - # username or password could be allowed in other requests? + # merge username and password into kwargs for `prepare_request_body` if username is not None: kwargs['username'] = username if password is not None: