Repository: box/box-python-sdk
Pull request #11 (2015-02-18 23:40:31)

Use urllib for better OAuth2 URL construction
Use ``six.moves.urllib.parse.urlencode()`` to correctly encode the query string parameters that make up the OAuth2 authorize URL.

Previously, the URL was constructed via direct string formatting. This meant that if reserved or non-ASCII characters were present in the data, they would be used unencoded in the URL. This was particularly
problematic with the ``redirect_uri`` query string parameter:

- It is itself a URI and therefore likely to contain reserved characters.

- It is user-supplied and therefore has the possibility of containing non-ASCII unicode characters.

The correct encoding of characters in URLs is specified in various Internet RFCs, which the ``urllib`` package has been designed to match. So using ``urllib`` fixes these problems.

This commit also corrects some related issues:

- When the ``redirect_url`` parameter is not supplied to ``OAuth2.get_authorization_url()``, the ``redirect_uri`` query string parameter is omitted from the constructed URL.

- Text is utf-8 encoded to byte strings before being URL encoded.

- Fixes a misuse of the output of ``six.moves.urllib.parse.parse_qs()``. The values of the dictionary were being assumed to be strings, when they are actually lists of strings. And in Python 2, these strings need to be manually decoded from ``bytes`` to ``unicode``.

- Adds unit test coverage for non-ASCII and ``None`` values of the ``redirect_url`` parameter to ``OAuth2.get_authorization_url()``.


Patch:
diff --git a/boxsdk/auth/oauth2.py b/boxsdk/auth/oauth2.py
index d66eaa1..232b60c 100644
--- a/boxsdk/auth/oauth2.py
+++ b/boxsdk/auth/oauth2.py
@@ -6,6 +6,8 @@
 import random
 import string  # pylint:disable=deprecated-module
 
+from six.moves.urllib.parse import urlencode, urlunsplit  # pylint:disable=import-error,no-name-in-module
+
 from boxsdk.network.default_network import DefaultNetwork
 from boxsdk.config import API
 from boxsdk.exception import BoxOAuthException
@@ -91,7 +93,7 @@ def get_authorization_url(self, redirect_url):
             An HTTPS URI or custom URL scheme where the response will be redirected. Optional if the redirect URI is
             registered with Box already.
         :type redirect_url:
-            `unicode`
+            `unicode` or None
         :return:
             A tuple of the URL of Box’s authorization page and the CSRF token.
             This is the URL that your application should forward the user to in first leg of OAuth 2.
@@ -99,12 +101,21 @@ def get_authorization_url(self, redirect_url):
             (`unicode`, `unicode`)
         """
         csrf_token = self._get_state_csrf_token()
-        return '{0}?state={1}&response_type=code&client_id={2}&redirect_uri={3}'.format(
-            API.OAUTH2_AUTHORIZE_URL,
-            csrf_token,
-            self._client_id,
-            redirect_url,
-        ), csrf_token
+        # For the query string parameters, use a sequence of two-element
+        # tuples, rather than a dictionary, in order to get a consistent and
+        # predictable order of parameters in the output of `urlencode()`.
+        params = [
+            ('state', csrf_token),
+            ('response_type', 'code'),
+            ('client_id', self._client_id),
+        ]
+        if redirect_url:
+            params.append(('redirect_uri', redirect_url))
+        # `urlencode()` doesn't work with non-ASCII unicode characters, so
+        # encode the parameters as ASCII bytes.
+        params = [(key.encode('utf-8'), value.encode('utf-8')) for (key, value) in params]
+        query_string = urlencode(params)
+        return urlunsplit(('', '', API.OAUTH2_AUTHORIZE_URL, query_string, '')), csrf_token
 
     def authenticate(self, auth_code):
         """


Test patch:
diff --git a/test/functional/conftest.py b/test/functional/conftest.py
index daa2e02..15a7a88 100644
--- a/test/functional/conftest.py
+++ b/test/functional/conftest.py
@@ -5,6 +5,7 @@
 import pytest
 import re
 import requests
+import six
 from six.moves.urllib import parse  # pylint:disable=import-error, no-name-in-module
 from boxsdk.auth.oauth2 import OAuth2
 from boxsdk.config import API
@@ -33,7 +34,16 @@ def box_oauth(client_id, client_secret, user_login):
     redirect_url = auth_response.headers['Location']
     query_string = parse.urlparse(redirect_url).query
     parsed_query_string_dict = parse.parse_qs(query_string)
-    oauth2.authenticate(parsed_query_string_dict['code'])
+
+    # Get the OAuth2 authorization code from `parsed_query_string_dict`
+    # (NOTE: the values in the dictionary are lists of strings),
+    # and if necessary decode it from a utf-8 encoded byte string to
+    # a unicode string.
+    auth_code = parsed_query_string_dict['code'][0]
+    if isinstance(auth_code, six.binary_type):
+        auth_code = auth_code.decode('utf-8')
+
+    oauth2.authenticate(auth_code)
     return oauth2
 
 
diff --git a/test/unit/auth/test_oauth2.py b/test/unit/auth/test_oauth2.py
index 5c0dcde..7016e66 100644
--- a/test/unit/auth/test_oauth2.py
+++ b/test/unit/auth/test_oauth2.py
@@ -7,26 +7,37 @@
 import re
 from threading import Thread
 
+from six.moves.urllib import parse as urlparse  # pylint:disable=import-error,no-name-in-module
+
 from boxsdk.exception import BoxOAuthException
 from boxsdk.network.default_network import DefaultNetworkResponse
 from boxsdk.auth.oauth2 import OAuth2
 from boxsdk.config import API
 
 
-def test_get_correct_authorization_url():
+@pytest.fixture(params=('https://url.com/foo?bar=baz', 'https://ȕŕľ.com/ƒőő?Ƅȁŕ=Ƅȁż', None))
+def redirect_url(request):
+    """A value for the `redirect_uri` query string parameter for OAuth2."""
+    return request.param
+
+
+def test_get_correct_authorization_url(redirect_url):
+    # pylint:disable=redefined-outer-name
     fake_client_id = 'fake_client_id'
     fake_client_secret = 'fake_client_secret'
     oauth2 = OAuth2(
         client_id=fake_client_id,
         client_secret=fake_client_secret,
     )
-    redirect_url = 'http://some.redirect.url.com'
-    auth_url, csrf_token = oauth2.get_authorization_url(redirect_url)
-    assert auth_url == '{0}?state={1}&response_type=code&client_id={2}&redirect_uri={3}'.format(
+    auth_url, csrf_token = oauth2.get_authorization_url(redirect_url=redirect_url)
+    expected_auth_url_format = '{0}?state={1}&response_type=code&client_id={2}'
+    if redirect_url:
+        expected_auth_url_format += '&redirect_uri={3}'
+    assert auth_url == expected_auth_url_format.format(
         API.OAUTH2_AUTHORIZE_URL,
         csrf_token,
         fake_client_id,
-        redirect_url,
+        urlparse.quote_plus((redirect_url or '').encode('utf-8')),
     )
     assert re.match('^box_csrf_token_[A-Za-z0-9]{16}$', csrf_token)