Resolve merge conflicts

Author: DharshanBJ

msal/application.py

@@ -675,6 +675,7 @@ def _decide_broker(self, allow_broker, enable_pii_log):
                 "allow_broker is deprecated. "
                 "Please use PublicClientApplication(..., "
                 "enable_broker_on_windows=True, "
+                # No need to mention non-Windows platforms, because allow_broker is only for Windows
                 "...)",
                 DeprecationWarning)
         opted_in_for_broker = (
@@ -697,7 +698,7 @@ def _decide_broker(self, allow_broker, enable_pii_log):
                 _init_broker(enable_pii_log)
             except RuntimeError:
                 self._enable_broker = False
-                logger.exception(
+                logger.warning(  # It is common on Mac and Linux where broker is not built-in
                     "Broker is unavailable on this platform. "
                     "We will fallback to non-broker.")
         logger.debug("Broker enabled? %s", self._enable_broker)
@@ -1922,7 +1923,12 @@ class PublicClientApplication(ClientApplication):  # browser app or mobile app
     DEVICE_FLOW_CORRELATION_ID = "_correlation_id"
     CONSOLE_WINDOW_HANDLE = object()
 
-    def __init__(self, client_id, client_credential=None, **kwargs):
+    def __init__(
+        self, client_id, client_credential=None,
+        *,
+        enable_broker_on_windows=None,
+        enable_broker_on_mac=None,
+        **kwargs):
         """Same as :func:`ClientApplication.__init__`,
         except that ``client_credential`` parameter shall remain ``None``.
 
@@ -2007,10 +2013,6 @@ def __init__(self, client_id, client_credential=None, **kwargs):
         """
         if client_credential is not None:
             raise ValueError("Public Client should not possess credentials")
-        # Using kwargs notation for now. We will switch to keyword-only arguments.
-        enable_broker_on_windows = kwargs.pop("enable_broker_on_windows", False)
-        enable_broker_on_mac = kwargs.pop("enable_broker_on_mac", False)
-        enable_broker_on_linux = kwargs.pop("enable_broker_on_linux", False)
         self._enable_broker = bool(
             enable_broker_on_windows and sys.platform == "win32"
             or enable_broker_on_mac and sys.platform == "darwin"
@@ -2261,7 +2263,8 @@ def _acquire_token_interactive_via_broker(
                 # _signin_silently() only gets tokens for default account,
                 # but this seems to have been fixed in PyMsalRuntime 0.11.2
                 "access_token" in response and login_hint
-                and response.get("id_token_claims", {}) != login_hint)
+                and login_hint != response.get(
+                    "id_token_claims", {}).get("preferred_username"))
             wrong_account_error_message = (
                 'prompt="none" will not work for login_hint="non-default-user"')
             if is_wrong_account:

tests/broker_util.py

@@ -0,0 +1,21 @@
+import logging
+
+
+logger = logging.getLogger(__name__)
+
+
+def is_pymsalruntime_installed() -> bool:
+    try:
+        import pymsalruntime
+        logger.info("PyMsalRuntime installed and initialized")
+        return True
+    except ImportError:
+        logger.info("PyMsalRuntime not installed")
+        return False
+    except RuntimeError:
+        logger.warning(
+            "PyMsalRuntime installed but failed to initialize the real broker. "
+            "This may happen on Mac and Linux where broker is not built-in. "
+            "Test cases shall attempt broker and test its fallback behavior."
+        )
+        return True

tests/test_account_source.py

@@ -3,15 +3,11 @@
     from unittest.mock import patch
 except:
     from mock import patch
-try:
-    import pymsalruntime
-    broker_available = True
-except ImportError:
-    broker_available = False
 import msal
 from tests import unittest
 from tests.test_token_cache import build_response
 from tests.http_client import MinimalResponse
+from tests.broker_util import is_pymsalruntime_installed
 
 
 SCOPE = "scope_foo"
@@ -24,54 +20,62 @@
 def _mock_post(url, headers=None, *args, **kwargs):
     return MinimalResponse(status_code=200, text=json.dumps(TOKEN_RESPONSE))
 
-@unittest.skipUnless(broker_available, "These test cases need pip install msal[broker]")
+@unittest.skipUnless(is_pymsalruntime_installed(), "These test cases need pip install msal[broker]")
 @patch("msal.broker._acquire_token_silently", return_value=dict(
-    TOKEN_RESPONSE, _account_id="placeholder"))
+   TOKEN_RESPONSE, _account_id="placeholder"))
 @patch.object(msal.authority, "tenant_discovery", return_value={
     "authorization_endpoint": "https://contoso.com/placeholder",
     "token_endpoint": "https://contoso.com/placeholder",
 })  # Otherwise it would fail on OIDC discovery
 class TestAccountSourceBehavior(unittest.TestCase):
 
+    def setUp(self):
+        self.app = msal.PublicClientApplication(
+            "client_id",
+            enable_broker_on_windows=True,
+            )
+        if not self.app._enable_broker:
+            self.skipTest(
+                "These test cases require patching msal.broker which is only possible "
+                "when broker enabled successfully i.e. no RuntimeError")
+        return super().setUp()
+
     def test_device_flow_and_its_silent_call_should_bypass_broker(self, _, mocked_broker_ats):
-        app = msal.PublicClientApplication("client_id", enable_broker_on_windows=True)
-        result = app.acquire_token_by_device_flow({"device_code": "123"}, post=_mock_post)
+        result = self.app.acquire_token_by_device_flow({"device_code": "123"}, post=_mock_post)
         self.assertEqual(result["token_source"], "identity_provider")
 
-        account = app.get_accounts()[0]
+        account = self.app.get_accounts()[0]
         self.assertEqual(account["account_source"], "urn:ietf:params:oauth:grant-type:device_code")
 
-        result = app.acquire_token_silent_with_error(
+        result = self.app.acquire_token_silent_with_error(
             [SCOPE], account, force_refresh=True, post=_mock_post)
         mocked_broker_ats.assert_not_called()
         self.assertEqual(result["token_source"], "identity_provider")
 
     def test_ropc_flow_and_its_silent_call_should_invoke_broker(self, _, mocked_broker_ats):
-        app = msal.PublicClientApplication("client_id", enable_broker_on_windows=True)
         with patch("msal.broker._signin_silently", return_value=dict(TOKEN_RESPONSE, _account_id="placeholder")):
-            result = app.acquire_token_by_username_password(
+            result = self.app.acquire_token_by_username_password(
                 "username", "placeholder", [SCOPE], post=_mock_post)
         self.assertEqual(result["token_source"], "broker")
 
-        account = app.get_accounts()[0]
+        account = self.app.get_accounts()[0]
         self.assertEqual(account["account_source"], "broker")
 
-        result = app.acquire_token_silent_with_error(
+        result = self.app.acquire_token_silent_with_error(
             [SCOPE], account, force_refresh=True, post=_mock_post)
         self.assertEqual(result["token_source"], "broker")
 
     def test_interactive_flow_and_its_silent_call_should_invoke_broker(self, _, mocked_broker_ats):
-        app = msal.PublicClientApplication("client_id", enable_broker_on_windows=True)
-        with patch.object(app, "_acquire_token_interactive_via_broker", return_value=dict(
+        with patch.object(self.app, "_acquire_token_interactive_via_broker", return_value=dict(
                 TOKEN_RESPONSE, _account_id="placeholder")):
-            result = app.acquire_token_interactive(
-                [SCOPE], parent_window_handle=app.CONSOLE_WINDOW_HANDLE)
+            result = self.app.acquire_token_interactive(
+                [SCOPE], parent_window_handle=self.app.CONSOLE_WINDOW_HANDLE)
         self.assertEqual(result["token_source"], "broker")
 
-        account = app.get_accounts()[0]
+        account = self.app.get_accounts()[0]
         self.assertEqual(account["account_source"], "broker")
 
-        result = app.acquire_token_silent_with_error(
+        result = self.app.acquire_token_silent_with_error(
             [SCOPE], account, force_refresh=True, post=_mock_post)
         mocked_broker_ats.assert_called_once()
         self.assertEqual(result["token_source"], "broker")

tests/test_application.py

@@ -20,6 +20,12 @@
 logger = logging.getLogger(__name__)
 logging.basicConfig(level=logging.DEBUG)
 
+_OIDC_DISCOVERY = "msal.authority.tenant_discovery"
+_OIDC_DISCOVERY_MOCK = Mock(return_value={
+    "authorization_endpoint": "https://contoso.com/placeholder",
+    "token_endpoint": "https://contoso.com/placeholder",
+})
+
 
 class TestHelperExtractCerts(unittest.TestCase):  # It is used by SNI scenario
 
@@ -58,10 +64,9 @@ def test_bytes_to_bytes(self):
 
 class TestClientApplicationAcquireTokenSilentErrorBehaviors(unittest.TestCase):
 
+    @patch(_OIDC_DISCOVERY, new=_OIDC_DISCOVERY_MOCK)
     def setUp(self):
         self.authority_url = "https://login.microsoftonline.com/common"
-        self.authority = msal.authority.Authority(
-            self.authority_url, MinimalHttpClient())
         self.scopes = ["s1", "s2"]
         self.uid = "my_uid"
         self.utid = "my_utid"
@@ -116,12 +121,11 @@ def tester(url, **kwargs):
         self.assertEqual("", result.get("classification"))
 
 
+@patch(_OIDC_DISCOVERY, new=_OIDC_DISCOVERY_MOCK)
 class TestClientApplicationAcquireTokenSilentFociBehaviors(unittest.TestCase):
 
     def setUp(self):
         self.authority_url = "https://login.microsoftonline.com/common"
-        self.authority = msal.authority.Authority(
-            self.authority_url, MinimalHttpClient())
         self.scopes = ["s1", "s2"]
         self.uid = "my_uid"
         self.utid = "my_utid"
@@ -148,7 +152,7 @@ def tester(url, data=None, **kwargs):
             self.assertEqual(self.frt, data.get("refresh_token"), "Should attempt the FRT")
             return MinimalResponse(status_code=400, text=error_response)
         app._acquire_token_silent_by_finding_rt_belongs_to_me_or_my_family(
-            self.authority, self.scopes, self.account, post=tester)
+            app.authority, self.scopes, self.account, post=tester)
         self.assertNotEqual([], app.token_cache.find(
             msal.TokenCache.CredentialType.REFRESH_TOKEN, query={"secret": self.frt}),
             "The FRT should not be removed from the cache")
@@ -168,7 +172,7 @@ def tester(url, data=None, **kwargs):
             self.assertEqual(rt, data.get("refresh_token"), "Should attempt the RT")
             return MinimalResponse(status_code=200, text='{}')
         app._acquire_token_silent_by_finding_rt_belongs_to_me_or_my_family(
-            self.authority, self.scopes, self.account, post=tester)
+            app.authority, self.scopes, self.account, post=tester)
 
     def test_unknown_family_app_will_attempt_frt_and_join_family(self):
         def tester(url, data=None, **kwargs):
@@ -180,7 +184,7 @@ def tester(url, data=None, **kwargs):
         app = ClientApplication(
             "unknown_family_app", authority=self.authority_url, token_cache=self.cache)
         at = app._acquire_token_silent_by_finding_rt_belongs_to_me_or_my_family(
-            self.authority, self.scopes, self.account, post=tester)
+            app.authority, self.scopes, self.account, post=tester)
         logger.debug("%s.cache = %s", self.id(), self.cache.serialize())
         self.assertEqual("at", at.get("access_token"), "New app should get a new AT")
         app_metadata = app.token_cache.find(
@@ -202,7 +206,7 @@ def tester(url, data=None, **kwargs):
         app = ClientApplication(
             "preexisting_family_app", authority=self.authority_url, token_cache=self.cache)
         resp = app._acquire_token_silent_by_finding_rt_belongs_to_me_or_my_family(
-            self.authority, self.scopes, self.account, post=tester)
+            app.authority, self.scopes, self.account, post=tester)
         logger.debug("%s.cache = %s", self.id(), self.cache.serialize())
         self.assertEqual(json.loads(error_response), resp, "Error raised will be returned")
 
@@ -237,7 +241,7 @@ def test_family_app_remove_account(self):
 
 class TestClientApplicationForAuthorityMigration(unittest.TestCase):
 
-    @classmethod
+    # Chose to not mock oidc discovery, because AuthorityMigration might rely on real data
     def setUp(self):
         self.environment_in_cache = "sts.windows.net"
         self.authority_url_in_app = "https://login.microsoftonline.com/common"
@@ -444,6 +448,7 @@ def mock_post(url, headers=None, *args, **kwargs):
         self.assertRefreshOn(result, new_refresh_in)
 
 
+# TODO Patching oidc discovery ends up failing. But we plan to remove offline telemetry anyway.
 class TestTelemetryMaintainingOfflineState(unittest.TestCase):
     authority_url = "https://login.microsoftonline.com/common"
     scopes = ["s1", "s2"]
@@ -524,6 +529,7 @@ def mock_post(url, headers=None, *args, **kwargs):
 
 class TestTelemetryOnClientApplication(unittest.TestCase):
     @classmethod
+    @patch(_OIDC_DISCOVERY, new=_OIDC_DISCOVERY_MOCK)
     def setUpClass(cls):  # Initialization at runtime, not interpret-time
         cls.app = ClientApplication(
             "client_id", authority="https://login.microsoftonline.com/common")
@@ -552,6 +558,7 @@ def mock_post(url, headers=None, *args, **kwargs):
 
 class TestTelemetryOnPublicClientApplication(unittest.TestCase):
     @classmethod
+    @patch(_OIDC_DISCOVERY, new=_OIDC_DISCOVERY_MOCK)
     def setUpClass(cls):  # Initialization at runtime, not interpret-time
         cls.app = PublicClientApplication(
             "client_id", authority="https://login.microsoftonline.com/common")
@@ -581,6 +588,7 @@ def mock_post(url, headers=None, *args, **kwargs):
 
 class TestTelemetryOnConfidentialClientApplication(unittest.TestCase):
     @classmethod
+    @patch(_OIDC_DISCOVERY, new=_OIDC_DISCOVERY_MOCK)
     def setUpClass(cls):  # Initialization at runtime, not interpret-time
         cls.app = ConfidentialClientApplication(
             "client_id", client_credential="secret",
@@ -626,6 +634,7 @@ def mock_post(url, headers=None, *args, **kwargs):
         self.assertEqual(at, result.get("access_token"))
 
 
+@patch(_OIDC_DISCOVERY, new=_OIDC_DISCOVERY_MOCK)
 class TestClientApplicationWillGroupAccounts(unittest.TestCase):
     def test_get_accounts(self):
         client_id = "my_app"
@@ -678,15 +687,24 @@ def mock_post(url, headers=None, *args, **kwargs):
         with self.assertWarns(DeprecationWarning):
             app.acquire_token_for_client(["scope"], post=mock_post)
 
+    @patch(_OIDC_DISCOVERY, new=Mock(return_value={
+        "authorization_endpoint": "https://contoso.com/common",
+        "token_endpoint": "https://contoso.com/common",
+        }))
     def test_common_authority_should_emit_warning(self):
         self._test_certain_authority_should_emit_warning(
             authority="https://login.microsoftonline.com/common")
 
+    @patch(_OIDC_DISCOVERY, new=Mock(return_value={
+        "authorization_endpoint": "https://contoso.com/organizations",
+        "token_endpoint": "https://contoso.com/organizations",
+        }))
     def test_organizations_authority_should_emit_warning(self):
         self._test_certain_authority_should_emit_warning(
             authority="https://login.microsoftonline.com/organizations")
 
 
+@patch(_OIDC_DISCOVERY, new=_OIDC_DISCOVERY_MOCK)
 class TestRemoveTokensForClient(unittest.TestCase):
     def test_remove_tokens_for_client_should_remove_client_tokens_only(self):
         at_for_user = "AT for user"
@@ -716,6 +734,7 @@ def test_remove_tokens_for_client_should_remove_client_tokens_only(self):
         self.assertEqual(at_for_user, remaining_tokens[0].get("secret"))
 
 
+@patch(_OIDC_DISCOVERY, new=_OIDC_DISCOVERY_MOCK)
 class TestScopeDecoration(unittest.TestCase):
     def _test_client_id_should_be_a_valid_scope(self, client_id, other_scopes):
         # B2C needs this https://learn.microsoft.com/en-us/azure/active-directory-b2c/access-tokens#openid-connect-scopes
@@ -855,4 +874,3 @@ def test_app_did_not_register_redirect_uri_should_error_out(self):
                 parent_window_handle=app.CONSOLE_WINDOW_HANDLE,
                 )
             self.assertEqual(result.get("error"), "broker_error")
-