Repository: AzureAD/microsoft-authentication-library-for-python

Issue:
Refresh token errors are discarded
`acquire_token_silent_with_error` appears to be intended to return the last error response received from AAD, and the documentation implies it does so, but there are cases in which it does not.

For example, if AAD rejects a refresh token, its error response is discarded (`at` is the response):
https://github.com/AzureAD/microsoft-authentication-library-for-python/blob/5c1f0f4ad18a61587393c290b1094b5f8fd014d4/msal/application.py#L601-L609

When AAD rejected the token, that token was evicted from the cache:
https://github.com/AzureAD/microsoft-authentication-library-for-python/blob/5c1f0f4ad18a61587393c290b1094b5f8fd014d4/msal/oauth2cli/oauth2.py#L449-L450

Subsequent calls to `_acquire_token_silent_by_finding_specific_refresh_token` may now return `None` because no refresh token remains in the cache. In such a case, `acquire_token_silent_with_error` returns `None`.

This behavior arguably makes sense because the difference between "no refresh token in the cache" and "only invalid refresh tokens in the cache" may not be interesting. But the documentation on `acquire_token_silent_with_error` implies it will distinguish these cases, so I describe the inconsistency here for comment and clarification.

Discussion:

Patch:
diff --git a/msal/application.py b/msal/application.py
index a17d359..66d9b43 100644
--- a/msal/application.py
+++ b/msal/application.py
@@ -633,8 +633,9 @@ class ClientApplication(object):
                 **kwargs)
             if at and "error" not in at:
                 return at
+        last_resp = None
         if app_metadata.get("family_id"):  # Meaning this app belongs to this family
-            at = self._acquire_token_silent_by_finding_specific_refresh_token(
+            last_resp = at = self._acquire_token_silent_by_finding_specific_refresh_token(
                 authority, scopes, dict(query, family_id=app_metadata["family_id"]),
                 **kwargs)
             if at and "error" not in at:
@@ -642,7 +643,8 @@ class ClientApplication(object):
         # Either this app is an orphan, so we will naturally use its own RT;
         # or all attempts above have failed, so we fall back to non-foci behavior.
         return self._acquire_token_silent_by_finding_specific_refresh_token(
-            authority, scopes, dict(query, client_id=self.client_id), **kwargs)
+            authority, scopes, dict(query, client_id=self.client_id),
+            **kwargs) or last_resp
 
     def _get_app_metadata(self, environment):
         apps = self.token_cache.find(  # Use find(), rather than token_cache.get(...)


Test patch:
diff --git a/tests/test_application.py b/tests/test_application.py
index 39becd5..65b36b3 100644
--- a/tests/test_application.py
+++ b/tests/test_application.py
@@ -176,6 +176,19 @@ class TestClientApplicationAcquireTokenSilentFociBehaviors(unittest.TestCase):
 
     # Will not test scenario of app leaving family. Per specs, it won't happen.
 
+    def test_preexisting_family_app_will_attempt_frt_and_return_error(self):
+        error_response = '{"error": "invalid_grant", "error_description": "xyz"}'
+        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 = 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)
+        logger.debug("%s.cache = %s", self.id(), self.cache.serialize())
+        self.assertEqual(json.loads(error_response), resp, "Error raised will be returned")
+
     def test_family_app_remove_account(self):
         logger.debug("%s.cache = %s", self.id(), self.cache.serialize())
         app = ClientApplication(