From a60ce8d434dd53e99746624af4035ef896c20915 Mon Sep 17 00:00:00 2001 From: Ugonna Akali Date: Wed, 28 Aug 2024 15:27:05 -0700 Subject: [PATCH 1/4] add support for force refresh in broker layer --- .gitignore | 1 + msal/application.py | 15 ++++++++ msal/broker.py | 4 ++- tests/test_force_refresh.py | 69 +++++++++++++++++++++++++++++++++++++ 4 files changed, 88 insertions(+), 1 deletion(-) create mode 100644 tests/test_force_refresh.py diff --git a/.gitignore b/.gitignore index 36b43713..eedae684 100644 --- a/.gitignore +++ b/.gitignore @@ -49,6 +49,7 @@ src/build docs/_build/ # Visual Studio Files /.vs/* +.vscode/* /tests/.vs/* # vim files diff --git a/msal/application.py b/msal/application.py index 75c36d59..5eca56a7 100644 --- a/msal/application.py +++ b/msal/application.py @@ -1556,6 +1556,21 @@ def _acquire_token_silent_from_cache_and_possibly_refresh_it( account_was_established_by_broker = account.get( "account_source") == _GRANT_TYPE_BROKER broker_attempt_succeeded_just_now = "error" not in response + + if (response.get("access_token") and force_refresh): + at_to_renew = response.get("access_token") + response = _acquire_token_silently( + "https://{}/{}".format(self.authority.instance, self.authority.tenant), + self.client_id, + account["local_account_id"], + scopes, + claims=_merge_claims_challenge_and_capabilities( + self._client_capabilities, claims_challenge), + correlation_id=correlation_id, + auth_scheme=auth_scheme, + at_to_renew= at_to_renew, + **data) + if account_was_established_by_broker or broker_attempt_succeeded_just_now: return self._process_broker_response(response, scopes, data) diff --git a/msal/broker.py b/msal/broker.py index e16e6102..4b567eb0 100644 --- a/msal/broker.py +++ b/msal/broker.py @@ -214,7 +214,7 @@ def _signin_interactively( def _acquire_token_silently( authority, client_id, account_id, scopes, claims=None, correlation_id=None, - auth_scheme=None, + auth_scheme=None, at_to_renew=None, **kwargs): # For MSA PT scenario where you use the /organizations, yes, # acquireTokenSilently is expected to fail. - Sam Wilson @@ -224,6 +224,8 @@ def _acquire_token_silently( return params = pymsalruntime.MSALRuntimeAuthParameters(client_id, authority) params.set_requested_scopes(scopes) + if at_to_renew: + params.set_access_token_to_renew(at_to_renew) if claims: params.set_decoded_claims(claims) if auth_scheme: diff --git a/tests/test_force_refresh.py b/tests/test_force_refresh.py new file mode 100644 index 00000000..76eddcc2 --- /dev/null +++ b/tests/test_force_refresh.py @@ -0,0 +1,69 @@ +from tests import unittest +import msal +import logging +import sys + +# from tests.test_e2e import LabBasedTestCase + +if not sys.platform.startswith("win"): + raise unittest.SkipTest("Currently, our broker supports Windows") + +SCOPE_ARM = "https://management.azure.com/.default" +_AZURE_CLI = "04b07795-8ddb-461a-bbee-02f9e1bf7b46" +pca = msal.PublicClientApplication( + _AZURE_CLI, + authority="https://login.microsoftonline.com/organizations", + enable_broker_on_mac=True, + enable_broker_on_windows=True) + + +# class ForceRefreshTestCase(LabBasedTestCase): +# def test_silent_with_force_refresh(self): +# # acquire token using username and password +# print("Testing silent flow with force_refresh=True") +# config = self.get_lab_user(usertype="cloud") +# config["password"] = self.get_lab_user_secret(config["lab_name"]) +# result = pca.acquire_token_by_username_password(username=config["lab_name"], password=config["password"], scopes=config["scope"]) +# # assert username and password, "You need to provide a test account and its password" + +# ropcToken = result.get("access_token") +# accounts = pca.get_accounts() +# account = accounts[0] +# assert account, "The logged in account should have been established by interactive flow" + +# result = pca.acquire_token_silent( +# config["scope"], +# account=account, +# force_refresh=False, +# auth_scheme=None, data=None) + +# assert result.get("access_token") == ropcToken, "Token should not be refreshed" + + +class ForceRefreshTestCase(unittest.TestCase): + def test_silent_with_force_refresh(self): + # acquire token using username and password + print("Testing silent flow with force_refresh=True") + result = pca.acquire_token_interactive(scopes=[SCOPE_ARM], prompt="select_account", parent_window_handle=pca.CONSOLE_WINDOW_HANDLE, enable_msa_passthrough=True) + accounts = pca.get_accounts() + account = accounts[0] + assert account, "The logged in account should have been established by interactive flow" + oldToken = result.get("access_token") + + + result = pca.acquire_token_silent( + scopes=[SCOPE_ARM], + account=account, + force_refresh=False) + + # This token should be recieved from cache + assert result.get("access_token") == oldToken, "Token should not be refreshed" + + + result = pca.acquire_token_silent( + scopes=[SCOPE_ARM], + account=account, + force_refresh=True) + + # Token will be different proving it is not from cache and was renewed + assert result.get("access_token") != oldToken, "Token should be refreshed" \ No newline at end of file From 6578ca26dcea35318e5a08e62b5fa38616414725 Mon Sep 17 00:00:00 2001 From: Ugonna Akali Date: Wed, 28 Aug 2024 15:31:27 -0700 Subject: [PATCH 2/4] remove commented code --- tests/test_force_refresh.py | 33 +++------------------------------ 1 file changed, 3 insertions(+), 30 deletions(-) diff --git a/tests/test_force_refresh.py b/tests/test_force_refresh.py index 76eddcc2..8eea3e00 100644 --- a/tests/test_force_refresh.py +++ b/tests/test_force_refresh.py @@ -3,8 +3,6 @@ import logging import sys -# from tests.test_e2e import LabBasedTestCase - if not sys.platform.startswith("win"): raise unittest.SkipTest("Currently, our broker supports Windows") @@ -14,35 +12,10 @@ _AZURE_CLI, authority="https://login.microsoftonline.com/organizations", enable_broker_on_mac=True, - enable_broker_on_windows=True) - - -# class ForceRefreshTestCase(LabBasedTestCase): -# def test_silent_with_force_refresh(self): -# # acquire token using username and password -# print("Testing silent flow with force_refresh=True") -# config = self.get_lab_user(usertype="cloud") -# config["password"] = self.get_lab_user_secret(config["lab_name"]) -# result = pca.acquire_token_by_username_password(username=config["lab_name"], password=config["password"], scopes=config["scope"]) -# # assert username and password, "You need to provide a test account and its password" - -# ropcToken = result.get("access_token") -# accounts = pca.get_accounts() -# account = accounts[0] -# assert account, "The logged in account should have been established by interactive flow" - -# result = pca.acquire_token_silent( -# config["scope"], -# account=account, -# force_refresh=False, -# auth_scheme=None, data=None) - -# assert result.get("access_token") == ropcToken, "Token should not be refreshed" - + enable_broker_on_windows=True) class ForceRefreshTestCase(unittest.TestCase): def test_silent_with_force_refresh(self): - # acquire token using username and password print("Testing silent flow with force_refresh=True") result = pca.acquire_token_interactive(scopes=[SCOPE_ARM], prompt="select_account", parent_window_handle=pca.CONSOLE_WINDOW_HANDLE, enable_msa_passthrough=True) accounts = pca.get_accounts() @@ -56,7 +29,7 @@ def test_silent_with_force_refresh(self): account=account, force_refresh=False) - # This token should be recieved from cache + # This token should have been recieved from cache assert result.get("access_token") == oldToken, "Token should not be refreshed" @@ -65,5 +38,5 @@ def test_silent_with_force_refresh(self): account=account, force_refresh=True) - # Token will be different proving it is not from cache and was renewed + # Token will be different proving it is not token from cache and was renewed assert result.get("access_token") != oldToken, "Token should be refreshed" \ No newline at end of file From c380781460e24c0387b6178f5a0ecfa94cd19c66 Mon Sep 17 00:00:00 2001 From: Ugonna Akali Date: Wed, 28 Aug 2024 15:51:16 -0700 Subject: [PATCH 3/4] adjust check order --- msal/application.py | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/msal/application.py b/msal/application.py index 5eca56a7..3bb01483 100644 --- a/msal/application.py +++ b/msal/application.py @@ -1552,12 +1552,8 @@ def _acquire_token_silent_from_cache_and_possibly_refresh_it( correlation_id=correlation_id, auth_scheme=auth_scheme, **data) - if response: # Broker provides a decisive outcome - account_was_established_by_broker = account.get( - "account_source") == _GRANT_TYPE_BROKER - broker_attempt_succeeded_just_now = "error" not in response - - if (response.get("access_token") and force_refresh): + + if (force_refresh and response.get("access_token")): at_to_renew = response.get("access_token") response = _acquire_token_silently( "https://{}/{}".format(self.authority.instance, self.authority.tenant), @@ -1570,6 +1566,11 @@ def _acquire_token_silent_from_cache_and_possibly_refresh_it( auth_scheme=auth_scheme, at_to_renew= at_to_renew, **data) + + if response: # Broker provides a decisive outcome + account_was_established_by_broker = account.get( + "account_source") == _GRANT_TYPE_BROKER + broker_attempt_succeeded_just_now = "error" not in response if account_was_established_by_broker or broker_attempt_succeeded_just_now: return self._process_broker_response(response, scopes, data) From c10ace1b4437e4ca349bf439e630a16cf590ba61 Mon Sep 17 00:00:00 2001 From: Ray Luo Date: Wed, 2 Oct 2024 01:43:43 -0700 Subject: [PATCH 4/4] Refactor --- msal/application.py | 35 +++++++++++----------- msal/broker.py | 3 +- tests/test_account_source.py | 2 +- tests/test_force_refresh.py | 57 ++++++++++++++++++------------------ 4 files changed, 49 insertions(+), 48 deletions(-) diff --git a/msal/application.py b/msal/application.py index 3bb01483..8f06c275 100644 --- a/msal/application.py +++ b/msal/application.py @@ -1542,31 +1542,30 @@ def _acquire_token_silent_from_cache_and_possibly_refresh_it( None, # Unknown data from older MSAL. Broker might still work. ): from .broker import _acquire_token_silently + _authority = "https://{}/{}".format( + self.authority.instance, self.authority.tenant) + claims = _merge_claims_challenge_and_capabilities( + self._client_capabilities, claims_challenge) response = _acquire_token_silently( - "https://{}/{}".format(self.authority.instance, self.authority.tenant), + _authority, self.client_id, account["local_account_id"], scopes, - claims=_merge_claims_challenge_and_capabilities( - self._client_capabilities, claims_challenge), + claims=claims, correlation_id=correlation_id, auth_scheme=auth_scheme, **data) - - if (force_refresh and response.get("access_token")): - at_to_renew = response.get("access_token") - response = _acquire_token_silently( - "https://{}/{}".format(self.authority.instance, self.authority.tenant), - self.client_id, - account["local_account_id"], - scopes, - claims=_merge_claims_challenge_and_capabilities( - self._client_capabilities, claims_challenge), - correlation_id=correlation_id, - auth_scheme=auth_scheme, - at_to_renew= at_to_renew, - **data) - + if force_refresh and response.get("access_token"): + response = _acquire_token_silently( + _authority, + self.client_id, + account["local_account_id"], + scopes, + claims=claims, + correlation_id=correlation_id, + auth_scheme=auth_scheme, + at_to_renew=response.get("access_token"), + **data) if response: # Broker provides a decisive outcome account_was_established_by_broker = account.get( "account_source") == _GRANT_TYPE_BROKER diff --git a/msal/broker.py b/msal/broker.py index 4b567eb0..02a316f1 100644 --- a/msal/broker.py +++ b/msal/broker.py @@ -214,7 +214,8 @@ def _signin_interactively( def _acquire_token_silently( authority, client_id, account_id, scopes, claims=None, correlation_id=None, - auth_scheme=None, at_to_renew=None, + auth_scheme=None, + at_to_renew=None, **kwargs): # For MSA PT scenario where you use the /organizations, yes, # acquireTokenSilently is expected to fail. - Sam Wilson diff --git a/tests/test_account_source.py b/tests/test_account_source.py index 662f0419..c9a2430d 100644 --- a/tests/test_account_source.py +++ b/tests/test_account_source.py @@ -73,6 +73,6 @@ def test_interactive_flow_and_its_silent_call_should_invoke_broker(self, _, mock result = app.acquire_token_silent_with_error( [SCOPE], account, force_refresh=True, post=_mock_post) - mocked_broker_ats.assert_called_once() + mocked_broker_ats.assert_called() self.assertEqual(result["token_source"], "broker") diff --git a/tests/test_force_refresh.py b/tests/test_force_refresh.py index 8eea3e00..93ca89d6 100644 --- a/tests/test_force_refresh.py +++ b/tests/test_force_refresh.py @@ -1,42 +1,43 @@ from tests import unittest import msal -import logging import sys -if not sys.platform.startswith("win"): - raise unittest.SkipTest("Currently, our broker supports Windows") -SCOPE_ARM = "https://management.azure.com/.default" +if sys.platform not in ("win32", "darwin"): + raise unittest.SkipTest(f"Our broker does not support {sys.platform}") + +SCOPES = ["https://management.azure.com/.default"] _AZURE_CLI = "04b07795-8ddb-461a-bbee-02f9e1bf7b46" pca = msal.PublicClientApplication( _AZURE_CLI, authority="https://login.microsoftonline.com/organizations", enable_broker_on_mac=True, - enable_broker_on_windows=True) + enable_broker_on_windows=True, + ) + class ForceRefreshTestCase(unittest.TestCase): - def test_silent_with_force_refresh(self): - print("Testing silent flow with force_refresh=True") - result = pca.acquire_token_interactive(scopes=[SCOPE_ARM], prompt="select_account", parent_window_handle=pca.CONSOLE_WINDOW_HANDLE, enable_msa_passthrough=True) + def test_silent_with_force_refresh_should_return_a_new_token(self): + result = pca.acquire_token_interactive( + scopes=SCOPES, + prompt="select_account", + parent_window_handle=pca.CONSOLE_WINDOW_HANDLE, + enable_msa_passthrough=True, + ) accounts = pca.get_accounts() + self.assertNotEqual( + [], accounts, + "Interactive flow should have established a logged-in account") account = accounts[0] - assert account, "The logged in account should have been established by interactive flow" - oldToken = result.get("access_token") - - - result = pca.acquire_token_silent( - scopes=[SCOPE_ARM], - account=account, - force_refresh=False) - - # This token should have been recieved from cache - assert result.get("access_token") == oldToken, "Token should not be refreshed" - - - result = pca.acquire_token_silent( - scopes=[SCOPE_ARM], - account=account, - force_refresh=True) - - # Token will be different proving it is not token from cache and was renewed - assert result.get("access_token") != oldToken, "Token should be refreshed" \ No newline at end of file + old_token = result.get("access_token") + + result = pca.acquire_token_silent(SCOPES, account) + assertion = "This token should have been received from cache" + self.assertEqual(result.get("access_token"), old_token, assertion) + self.assertEqual(result.get("token_source"), "cache", assertion) + + result = pca.acquire_token_silent(SCOPES, account, force_refresh=True) + assertion = "A new token should have been received from broker" + self.assertNotEqual(result.get("access_token"), old_token, assertion) + self.assertEqual(result.get("token_source"), "broker", assertion) +