Review changes 1

msal/application.py

@@ -1,3 +1,4 @@
+import functools
 import json
 import time
 try:  # Python 2
@@ -146,14 +147,17 @@ def __init__(
         :param verify: (optional)
             It will be passed to the
             `verify parameter in the underlying requests library
+            This does not apply if you have chosen to pass your own Http client
             <http://docs.python-requests.org/en/v2.9.1/user/advanced/#ssl-cert-verification>`_
         :param proxies: (optional)
             It will be passed to the
             `proxies parameter in the underlying requests library
+            This does not apply if you have chosen to pass your own Http client
             <http://docs.python-requests.org/en/v2.9.1/user/advanced/#proxies>`_
         :param timeout: (optional)
             It will be passed to the
             `timeout parameter in the underlying requests library
+            This does not apply if you have chosen to pass your own Http client
             <http://docs.python-requests.org/en/v2.9.1/user/advanced/#timeouts>`_
         :param app_name: (optional)
             You can provide your application name for Microsoft telemetry purposes.
@@ -171,12 +175,15 @@ def __init__(
             self.http_client = requests.Session()
             self.http_client.verify = verify
             self.http_client.proxies = proxies
-        self.timeout = timeout
+            # Requests, does not support session - wide timeout
+            # But you can patch that (https://github.com/psf/requests/issues/3341):
+            self.http_client.request = functools.partial(
+                self.http_client.request, timeout=timeout)
         self.app_name = app_name
         self.app_version = app_version
         self.authority = Authority(
                 authority or "https://login.microsoftonline.com/common/",
-                http_client=self.http_client, validate_authority=validate_authority, timeout=timeout)
+                http_client=self.http_client, validate_authority=validate_authority)
             # Here the self.authority is not the same type as authority in input
         self.token_cache = token_cache or TokenCache()
         self.client = self._build_client(client_credential, self.authority)
@@ -226,8 +233,7 @@ def _build_client(self, client_credential, authority):
             client_assertion_type=client_assertion_type,
             on_obtaining_tokens=self.token_cache.add,
             on_removing_rt=self.token_cache.remove_rt,
-            on_updating_rt=self.token_cache.update_rt,
-            timeout=self.timeout)
+            on_updating_rt=self.token_cache.update_rt)
 
     def get_authorization_request_url(
             self,
@@ -277,8 +283,7 @@ def get_authorization_request_url(
         # The previous implementation is, it will use self.authority by default.
         # Multi-tenant app can use new authority on demand
         the_authority = Authority(
-            authority, http_client=self.http_client, timeout=self.timeout
-            ) if authority else self.authority
+            authority, http_client=self.http_client) if authority else self.authority
 
         client = Client(
             {"authorization_endpoint": the_authority.authorization_endpoint},
@@ -389,8 +394,7 @@ def _get_authority_aliases(self, instance):
         if not self.authority_groups:
             resp = self.http_client.get(
                 "https://login.microsoftonline.com/common/discovery/instance?api-version=1.1&authorization_endpoint=https://login.microsoftonline.com/common/oauth2/authorize",
-                headers={'Accept': 'application/json'},
-                timeout=self.timeout)
+                headers={'Accept': 'application/json'})
             resp.raise_for_status()
             resp = json.loads(resp.text)
             self.authority_groups = [
@@ -513,7 +517,6 @@ def acquire_token_silent_with_error(
             warnings.warn("We haven't decided how/if this method will accept authority parameter")
         # the_authority = Authority(
         #     authority, http_client=self.http_client,
-        #     timeout=self.timeout
         #     ) if authority else self.authority
         result = self._acquire_token_silent_from_cache_and_possibly_refresh_it(
             scopes, account, self.authority, force_refresh=force_refresh,
@@ -525,8 +528,7 @@ def acquire_token_silent_with_error(
         for alias in self._get_authority_aliases(self.authority.instance):
             the_authority = Authority(
                 "https://" + alias + "/" + self.authority.tenant,
-                 http_client=self.http_client, validate_authority=False,
-                 timeout=self.timeout)
+                 http_client=self.http_client, validate_authority=False)
             result = self._acquire_token_silent_from_cache_and_possibly_refresh_it(
                 scopes, account, the_authority, force_refresh=force_refresh,
                 correlation_id=correlation_id,

msal/authority.py

@@ -33,9 +33,7 @@ class Authority(object):
     """
     _domains_without_user_realm_discovery = set([])
 
-    def __init__(self, authority_url, http_client, validate_authority=True,
-            timeout=None
-            ):
+    def __init__(self, authority_url, http_client, validate_authority=True):
         """Creates an authority instance, and also validates it.
 
         :param validate_authority:
@@ -45,17 +43,16 @@ def __init__(self, authority_url, http_client, validate_authority=True,
             performed.
         """
         self.http_client = http_client
-        self.timeout = timeout
         authority, self.instance, tenant = canonicalize(authority_url)
         parts = authority.path.split('/')
         self.is_b2c = any(self.instance.endswith("." + d) for d in WELL_KNOWN_B2C_HOSTS) or (
             len(parts) == 3 and parts[2].lower().startswith("b2c_"))
         if (tenant != "adfs" and (not self.is_b2c) and validate_authority
                 and self.instance not in WELL_KNOWN_AUTHORITY_HOSTS):
-            payload = self.instance_discovery(
+            payload = instance_discovery(
                 "https://{}{}/oauth2/v2.0/authorize".format(
                     self.instance, authority.path),
-                timeout=timeout)
+                self.http_client)
             if payload.get("error") == "invalid_instance":
                 raise ValueError(
                     "invalid_instance: "
@@ -72,9 +69,9 @@ def __init__(self, authority_url, http_client, validate_authority=True,
                     authority.path,  # In B2C scenario, it is "/tenant/policy"
                     "" if tenant == "adfs" else "/v2.0" # the AAD v2 endpoint
                     ))
-        openid_config = self.tenant_discovery(
+        openid_config = tenant_discovery(
             tenant_discovery_endpoint,
-            timeout=timeout)
+            self.http_client)
         logger.debug("openid_config = %s", openid_config)
         self.authorization_endpoint = openid_config['authorization_endpoint']
         self.token_endpoint = openid_config['token_endpoint']
@@ -85,28 +82,17 @@ def user_realm_discovery(self, username, correlation_id=None, response=None):
         # It will typically return a dict containing "ver", "account_type",
         # "federation_protocol", "cloud_audience_urn",
         # "federation_metadata_url", "federation_active_auth_url", etc.
-        resp = response or self.http_client.get("https://{netloc}/common/userrealm/{username}?api-version=1.0".format(
-                     netloc=self.instance, username=username),
-                     headers={'Accept':'application/json', 'client-request-id': correlation_id},
-                     timeout=self.timeout)
-        return json.loads(resp.text)
-
-    def instance_discovery(self, url, **kwargs):
-        resp = self.http_client.get('https://{}/common/discovery/instance'.format(
-                 WORLD_WIDE  # Historically using WORLD_WIDE. Could use self.instance too
-                     # See https://github.com/AzureAD/microsoft-authentication-library-for-dotnet/blob/4.0.0/src/Microsoft.Identity.Client/Instance/AadInstanceDiscovery.cs#L101-L103
-                     # and https://github.com/AzureAD/microsoft-authentication-library-for-dotnet/blob/4.0.0/src/Microsoft.Identity.Client/Instance/AadAuthority.cs#L19-L33
-                 ), params={'authorization_endpoint': url, 'api-version': '1.0'},
-             **kwargs)
-        return json.loads(resp.text)
-
-    def tenant_discovery(self, tenant_discovery_endpoint, **kwargs):
-        # Returns Openid Configuration
-        resp = self.http_client.get(tenant_discovery_endpoint, **kwargs)
-        payload = json.loads(resp.text)
-        if 'authorization_endpoint' in payload and 'token_endpoint' in payload:
-            return payload
-        raise MsalServiceError(status_code=resp.status_code, **payload)
+        if self.instance not in self.__class__._domains_without_user_realm_discovery:
+            resp = response or self.http_client.get(
+                "https://{netloc}/common/userrealm/{username}?api-version=1.0".format(
+                    netloc=self.instance, username=username),
+                headers={'Accept': 'application/json',
+                         'client-request-id': correlation_id},)
+            if resp.status_code != 404:
+                resp.raise_for_status()
+                return json.loads(resp.text)
+            self.__class__._domains_without_user_realm_discovery.add(self.instance)
+        return {}  # This can guide the caller to fall back normal ROPC flow
 
 
 def canonicalize(authority_url):
@@ -121,3 +107,20 @@ def canonicalize(authority_url):
             "or https://<tenant_name>.b2clogin.com/<tenant_name>.onmicrosoft.com/policy"
             % authority_url)
     return authority, authority.hostname, parts[1]
+
+def instance_discovery(url, http_client, **kwargs):
+    resp = http_client.get('https://{}/common/discovery/instance'.format(
+             WORLD_WIDE  # Historically using WORLD_WIDE. Could use self.instance too
+                 # See https://github.com/AzureAD/microsoft-authentication-library-for-dotnet/blob/4.0.0/src/Microsoft.Identity.Client/Instance/AadInstanceDiscovery.cs#L101-L103
+                 # and https://github.com/AzureAD/microsoft-authentication-library-for-dotnet/blob/4.0.0/src/Microsoft.Identity.Client/Instance/AadAuthority.cs#L19-L33
+             ), params={'authorization_endpoint': url, 'api-version': '1.0'},
+         **kwargs)
+    return json.loads(resp.text)
+
+def tenant_discovery(tenant_discovery_endpoint, http_client, **kwargs):
+    # Returns Openid Configuration
+    resp = http_client.get(tenant_discovery_endpoint, **kwargs)
+    payload = json.loads(resp.text)
+    if 'authorization_endpoint' in payload and 'token_endpoint' in payload:
+        return payload
+    raise MsalServiceError(status_code=resp.status_code, **payload)

msal/oauth2cli/oauth2.py

@@ -39,7 +39,6 @@ def __init__(
             client_assertion_type=None,  # type: Optional[str]
             default_headers=None,  # type: Optional[dict]
             default_body=None,  # type: Optional[dict]
-            timeout=None,  # type: Union[tuple, float, None]
             ):
         """Initialize a client object to talk all the OAuth2 grants to the server.
 
@@ -84,7 +83,6 @@ def __init__(
             self.default_body["client_assertion_type"] = client_assertion_type
         self.logger = logging.getLogger(__name__)
         self.http_client = http_client
-        self.timeout = timeout
 
     def _build_auth_request_params(self, response_type, **kwargs):
         # response_type is a string defined in
@@ -104,7 +102,6 @@ def _obtain_token(  # The verb "obtain" is influenced by OAUTH2 RFC 6749
             params=None,  # a dict to be sent as query string to the endpoint
             data=None,  # All relevant data, which will go into the http body
             headers=None,  # a dict to be sent as request headers
-            timeout=None,
             post=None,  # A callable to replace requests.post(), for testing.
                         # Such as: lambda url, **kwargs:
                         #   Mock(status_code=200, json=Mock(return_value={}))
@@ -148,7 +145,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)(self.configuration["token_endpoint"],
-            headers=_headers, params=params, data=_data, timeout=timeout or self.timeout,
+            headers=_headers, params=params, data=_data,
             **kwargs)
         if resp.status_code >= 500:
             resp.raise_for_status()  # TODO: Will probably retry here
@@ -197,7 +194,7 @@ class Client(BaseClient):  # We choose to implement all 4 grants in 1 class
     grant_assertion_encoders = {GRANT_TYPE_SAML2: BaseClient.encode_saml_assertion}
 
 
-    def initiate_device_flow(self, scope=None, timeout=None, **kwargs):
+    def initiate_device_flow(self, scope=None, **kwargs):
         # type: (list, **dict) -> dict
         # The naming of this method is following the wording of this specs
         # https://tools.ietf.org/html/draft-ietf-oauth-device-flow-12#section-3.1
@@ -217,7 +214,7 @@ def initiate_device_flow(self, scope=None, timeout=None, **kwargs):
             raise ValueError("You need to provide device authorization endpoint")
         resp = self.http_client.post(self.configuration[DAE],
             data={"client_id": self.client_id, "scope": self._stringify(scope or [])},
-            timeout=timeout or self.timeout, **kwargs)
+            **kwargs)
         flow = json.loads(resp.text)
         flow["interval"] = int(flow.get("interval", 5))  # Some IdP returns string
         flow["expires_in"] = int(flow.get("expires_in", 1800))
@@ -378,12 +375,13 @@ class initialization.
         return self._obtain_token("client_credentials", data=data, **kwargs)
 
     def __init__(self,
-            server_configuration, client_id,
+            server_configuration, client_id, http_client,
             on_obtaining_tokens=lambda event: None,  # event is defined in _obtain_token(...)
             on_removing_rt=lambda token_item: None,
             on_updating_rt=lambda token_item, new_rt: None,
             **kwargs):
-        super(Client, self).__init__(server_configuration, client_id, **kwargs)
+        super(Client, self).__init__(
+            server_configuration, client_id, http_client, **kwargs)
         self.on_obtaining_tokens = on_obtaining_tokens
         self.on_removing_rt = on_removing_rt
         self.on_updating_rt = on_updating_rt

tests/http_client.py

@@ -0,0 +1,19 @@
+import requests
+
+
+class MinimalHttpClient:
+
+    def __init__(self, verify=True, proxies=None, timeout=None):
+        self.session = requests.Session()
+        self.session.verify = verify
+        self.session.proxies = proxies
+        self.timeout = timeout
+
+    def post(self, url, params=None, data=None, headers=None, **kwargs):
+        return self.session.post(
+            url, params=params, data=data, headers=headers,
+            timeout=self.timeout)
+
+    def get(self, url, params=None, headers=None, **kwargs):
+        return self.session.get(
+            url, params=params, headers=headers, timeout=self.timeout)

tests/test_authority.py

@@ -4,14 +4,16 @@
 from tests import unittest
 import requests
 
+from tests.http_client import MinimalHttpClient
+
 
 @unittest.skipIf(os.getenv("TRAVIS_TAG"), "Skip network io during tagged release")
 class TestAuthority(unittest.TestCase):
 
     def test_wellknown_host_and_tenant(self):
         # Assert all well known authority hosts are using their own "common" tenant
         for host in WELL_KNOWN_AUTHORITY_HOSTS:
-            a = Authority('https://{}/common'.format(host), requests.Session())
+            a = Authority('https://{}/common'.format(host), MinimalHttpClient())
             self.assertEqual(
                 a.authorization_endpoint,
                 'https://%s/common/oauth2/v2.0/authorize' % host)
@@ -24,14 +26,14 @@ def test_lessknown_host_will_return_a_set_of_v1_endpoints(self):
         # It is probably not a strict API contract. I simply mention it here.
         less_known = 'login.windows.net'  # less.known.host/
         v1_token_endpoint = 'https://{}/common/oauth2/token'.format(less_known)
-        a = Authority('https://{}/common'.format(less_known), requests.Session())
+        a = Authority('https://{}/common'.format(less_known), MinimalHttpClient())
         self.assertEqual(a.token_endpoint, v1_token_endpoint)
         self.assertNotIn('v2.0', a.token_endpoint)
 
     def test_unknown_host_wont_pass_instance_discovery(self):
         _assert = getattr(self, "assertRaisesRegex", self.assertRaisesRegexp)  # Hack
         with _assert(ValueError, "invalid_instance"):
-            Authority('https://example.com/tenant_doesnt_matter_in_this_case', requests.Session())
+            Authority('https://example.com/tenant_doesnt_matter_in_this_case', MinimalHttpClient())
 
     def test_invalid_host_skipping_validation_can_be_turned_off(self):
         try:
@@ -73,3 +75,21 @@ def test_canonicalize_rejects_tenantless_host_with_trailing_slash(self):
             canonicalize("https://no.tenant.example.com/")
 
 
+@unittest.skipIf(os.getenv("TRAVIS_TAG"), "Skip network io during tagged release")
+class TestAuthorityInternalHelperUserRealmDiscovery(unittest.TestCase):
+    def test_memorize(self):
+        # We use a real authority so the constructor can finish tenant discovery
+        authority = "https://login.microsoftonline.com/common"
+        self.assertNotIn(authority, Authority._domains_without_user_realm_discovery)
+        a = Authority(authority, validate_authority=False)
+
+        # We now pretend this authority supports no User Realm Discovery
+        class MockResponse(object):
+            status_code = 404
+        a.user_realm_discovery("[email protected]", response=MockResponse())
+        self.assertIn(
+            "login.microsoftonline.com",
+            Authority._domains_without_user_realm_discovery,
+            "user_realm_discovery() should memorize domains not supporting URD")
+        a.user_realm_discovery("[email protected]",
+            response="This would cause exception if memorization did not work")