Merge branch 'backward-compatibility' into dev

Author: rayluo

msal/authority.py

@@ -5,6 +5,11 @@
     from urlparse import urlparse
 import logging
 
+# Historically some customers patched this module-wide requests instance.
+# We keep it here for now. They will be removed in next major release.
+import requests
+import requests as _requests
+
 from .exceptions import MsalServiceError
 
 
@@ -33,6 +38,11 @@ class Authority(object):
     """
     _domains_without_user_realm_discovery = set([])
 
+    @property
+    def http_client(self):  # Obsolete. We will remove this in next major release.
+        # A workaround: if module-wide requests is patched, we honor it.
+        return self._http_client if requests is _requests else requests
+
     def __init__(self, authority_url, http_client, validate_authority=True):
         """Creates an authority instance, and also validates it.
 
@@ -42,7 +52,7 @@ def __init__(self, authority_url, http_client, validate_authority=True):
             This parameter only controls whether an instance discovery will be
             performed.
         """
-        self.http_client = http_client
+        self._http_client = http_client
         authority, self.instance, tenant = canonicalize(authority_url)
         parts = authority.path.split('/')
         is_b2c = any(self.instance.endswith("." + d) for d in WELL_KNOWN_B2C_HOSTS) or (

msal/oauth2cli/http.py

@@ -11,7 +11,14 @@ class HttpClient(object):
     def post(self, url, params=None, data=None, headers=None, **kwargs):
         """HTTP post.
 
-        params, data and headers MUST accept a dictionary.
+        :param dict params: A dict to be url-encoded and sent as query-string.
+        :param dict headers: A dict representing headers to be sent via request.
+        :param data:
+            Implementation needs to support 2 types.
+
+            * A dict, which will need to be urlencode() before being sent.
+            * (Recommended) A string, which will be sent in request as-is.
+
         It returns an :class:`~Response`-like object.
 
         Note: In its async counterpart, this method would be defined as async.
@@ -21,7 +28,9 @@ def post(self, url, params=None, data=None, headers=None, **kwargs):
     def get(self, url, params=None, headers=None, **kwargs):
         """HTTP get.
 
-        params, data and headers MUST accept a dictionary.
+        :param dict params: A dict to be url-encoded and sent as query-string.
+        :param dict headers: A dict representing headers to be sent via request.
+
         It returns an :class:`~Response`-like object.
 
         Note: In its async counterpart, this method would be defined as async.

msal/oauth2cli/oauth2.py

@@ -33,6 +33,17 @@ def encode_saml_assertion(assertion):
     CLIENT_ASSERTION_TYPE_SAML2 = "urn:ietf:params:oauth:client-assertion-type:saml2-bearer"
     client_assertion_encoders = {CLIENT_ASSERTION_TYPE_SAML2: encode_saml_assertion}
 
+    @property
+    def session(self):
+        warnings.warn("Will be gone in next major release", DeprecationWarning)
+        return self._http_client
+
+    @session.setter
+    def session(self, value):
+        warnings.warn("Will be gone in next major release", DeprecationWarning)
+        self._http_client = value
+
+
     def __init__(
             self,
             server_configuration,  # type: dict
@@ -43,7 +54,7 @@ def __init__(
             client_assertion_type=None,  # type: Optional[str]
             default_headers=None,  # type: Optional[dict]
             default_body=None,  # type: Optional[dict]
-            verify=True,  # type: Union[str, True, False, None]
+            verify=None,  # type: Union[str, True, False, None]
             proxies=None,  # type: Optional[dict]
             timeout=None,  # type: Union[tuple, float, None]
             ):
@@ -60,9 +71,21 @@ def __init__(
                 or
                 https://example.com/.../.well-known/openid-configuration
             client_id (str): The client's id, issued by the authorization server
+
             http_client (http.HttpClient):
                 Your implementation of abstract class :class:`http.HttpClient`.
                 Defaults to a requests session instance.
+
+                There is no session-wide `timeout` parameter defined here.
+                Timeout behavior is determined by the actual http client you use.
+                If you happen to use Requests, it disallows session-wide timeout
+                (https://github.com/psf/requests/issues/3341). The workaround is:
+
+                    s = requests.Session()
+                    s.request = functools.partial(s.request, timeout=3)
+
+                and then feed that patched session instance to this class.
+
             client_secret (str):  Triggers HTTP AUTH for Confidential Client
             client_assertion (bytes, callable):
                 The client assertion to authenticate this client, per RFC 7521.
@@ -86,28 +109,25 @@ def __init__(
             verify (boolean):
                 It will be passed to the
                 `verify parameter in the underlying requests library
-                <http://docs.python-requests.org/en/v2.9.1/user/advanced/#ssl-cert-verification>`_
+                <http://docs.python-requests.org/en/v2.9.1/user/advanced/#ssl-cert-verification>`_.
+                When leaving it with default value (None), we will use True instead.
+
                 This does not apply if you have chosen to pass your own Http client.
+
             proxies (dict):
                 It will be passed to the
                 `proxies parameter in the underlying requests library
-                <http://docs.python-requests.org/en/v2.9.1/user/advanced/#proxies>`_
+                <http://docs.python-requests.org/en/v2.9.1/user/advanced/#proxies>`_.
+
                 This does not apply if you have chosen to pass your own Http client.
+
             timeout (object):
                 It will be passed to the
                 `timeout parameter in the underlying requests library
-                <http://docs.python-requests.org/en/v2.9.1/user/advanced/#timeouts>`_
-                This does not apply if you have chosen to pass your own Http client.
-
-        There is no session-wide `timeout` parameter defined here.
-        The timeout behavior is determined by the actual http client you use.
-        If you happen to use Requests, it chose to not support session-wide timeout
-        (https://github.com/psf/requests/issues/3341), but you can patch that by:
+                <http://docs.python-requests.org/en/v2.9.1/user/advanced/#timeouts>`_.
 
-            s = requests.Session()
-            s.request = functools.partial(s.request, timeout=3)
+                This does not apply if you have chosen to pass your own Http client.
 
-        and then feed that patched session instance to this class.
         """
         self.configuration = server_configuration
         self.client_id = client_id
@@ -119,14 +139,18 @@ def __init__(
             self.default_body["client_assertion_type"] = client_assertion_type
         self.logger = logging.getLogger(__name__)
         if http_client:
-            self.http_client = http_client
+            if verify is not None or proxies is not None or timeout is not None:
+                raise ValueError(
+                    "verify, proxies, or timeout is not allowed "
+                    "when http_client is in use")
+            self._http_client = http_client
         else:
-            self.http_client = requests.Session()
-            self.http_client.verify = verify
-            self.http_client.proxies = proxies
-            self.http_client.request = functools.partial(
+            self._http_client = requests.Session()
+            self._http_client.verify = True if verify is None else verify
+            self._http_client.proxies = proxies
+            self._http_client.request = functools.partial(
                 # A workaround for requests not supporting session-wide timeout
-                self.http_client.request, timeout=timeout)
+                self._http_client.request, timeout=timeout)
 
     def _build_auth_request_params(self, response_type, **kwargs):
         # response_type is a string defined in
@@ -187,7 +211,7 @@ def _obtain_token(  # The verb "obtain" is influenced by OAUTH2 RFC 6749
 
         if "token_endpoint" not in self.configuration:
             raise ValueError("token_endpoint not found in configuration")
-        resp = (post or self.http_client.post)(
+        resp = (post or self._http_client.post)(
             self.configuration["token_endpoint"],
             headers=_headers, params=params, data=_data,
             **kwargs)
@@ -256,7 +280,7 @@ def initiate_device_flow(self, scope=None, **kwargs):
         DAE = "device_authorization_endpoint"
         if not self.configuration.get(DAE):
             raise ValueError("You need to provide device authorization endpoint")
-        resp = self.http_client.post(self.configuration[DAE],
+        resp = self._http_client.post(self.configuration[DAE],
             data={"client_id": self.client_id, "scope": self._stringify(scope or [])},
             headers=dict(self.default_headers, **kwargs.pop("headers", {})),
             **kwargs)

tests/test_authority_patch.py

@@ -0,0 +1,32 @@
+import unittest
+
+import msal
+from tests.http_client import MinimalHttpClient
+
+
+class DummyHttpClient(object):
+    def get(self, url, **kwargs):
+        raise RuntimeError("just for testing purpose")
+
+
+class TestAuthorityHonorsPatchedRequests(unittest.TestCase):
+    """This is only a workaround for an undocumented behavior."""
+    def test_authority_honors_a_patched_requests(self):
+        # First, we test that the original, unmodified authority is working
+        a = msal.authority.Authority(
+            "https://login.microsoftonline.com/common", MinimalHttpClient())
+        self.assertEqual(
+            a.authorization_endpoint,
+            'https://login.microsoftonline.com/common/oauth2/v2.0/authorize')
+
+        original = msal.authority.requests
+        try:
+            # Now we mimic a (discouraged) practice of patching authority.requests
+            msal.authority.requests = DummyHttpClient()
+            # msal.authority is expected to honor that patch.
+            with self.assertRaises(RuntimeError):
+                a = msal.authority.Authority(
+                    "https://login.microsoftonline.com/common", MinimalHttpClient())
+        finally:  # Tricky:
+            # Unpatch is necessary otherwise other test cases would be affected
+            msal.authority.requests = original

tests/test_client.py

@@ -174,3 +174,11 @@ def test_device_flow(self):
                 assertion=lambda: self.assertIn('access_token', result),
                 skippable_errors=self.client.DEVICE_FLOW_RETRIABLE_ERRORS)
 
+
+class TestSessionAccessibility(unittest.TestCase):
+    def test_accessing_session_property_for_backward_compatibility(self):
+        client = Client({}, "client_id")
+        client.session
+        client.session.close()
+        client.session = "something"
+