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

Issue:
MSAL shouldn't remove Refresh Token upon receiving invalid_grant
**Describe the bug**
MSAL shouldn't remove Refresh Token upon receiving `invalid_grant`.

**To Reproduce**
When 

1. ARM (`https://management.azure.com/.default`) doesn't require MFA
2. VM SSH (`https://pas.windows.net/CheckMyAccess/Linux/.default`) requires MFA

This script will cause the RT of ARM to be removed, making subsequence ARM requests fail:

```py
import msal

app = msal.PublicClientApplication("04b07795-8ddb-461a-bbee-02f9e1bf7b46",
                                   authority="https://login.microsoftonline.com/organizations")
result = app.acquire_token_interactive(["https://management.azure.com/.default"])
print(app.token_cache._cache['RefreshToken'])

account = app.get_accounts(result['id_token_claims']['preferred_username'])[0]
result = app.acquire_token_silent_with_error(['https://pas.windows.net/CheckMyAccess/Linux/.default'], account)
print(result)
print(app.token_cache._cache['RefreshToken'])
```

**Expected behavior**
The refresh token should be persisted so that ARM request can still work.

**What you see instead**
Output (prettified):

```json
{
    "92bebeb6-c875-4f40-b2d2-f6324929f04a.54826b22-38d6-4fb2-bad9-b7b93a3e9c5a-login.microsoftonline.com-refreshtoken-04b07795-8ddb-461a-bbee-02f9e1bf7b46--https://management.azure.com/user_impersonation https://management.azure.com/.default": {
        "credential_type": "RefreshToken",
        "secret": "...",
        "home_account_id": "92bebeb6-c875-4f40-b2d2-f6324929f04a.54826b22-38d6-4fb2-bad9-b7b93a3e9c5a",
        "environment": "login.microsoftonline.com",
        "client_id": "04b07795-8ddb-461a-bbee-02f9e1bf7b46",
        "target": "https://management.azure.com/user_impersonation https://management.azure.com/.default",
        "family_id": "1"
    }
}

{
    "error": "invalid_grant",
    "error_description": "AADSTS50076: Due to a configuration change made by your administrator, or because you moved to a new location, you must use multi-factor authentication to access 'ce6ff14a-7fdc-4685-bbe0-f6afdfcfa8e0'.\r\nTrace ID: 3ddb15bc-d1d1-4d67-9b9f-77c0beb99000\r\nCorrelation ID: d8e4d35b-419e-4b25-8152-c833f497c38e\r\nTimestamp: 2021-03-01 11:21:13Z",
    "error_codes": [
        50076
    ],
    "timestamp": "2021-03-01 11:21:13Z",
    "trace_id": "3ddb15bc-d1d1-4d67-9b9f-77c0beb99000",
    "correlation_id": "d8e4d35b-419e-4b25-8152-c833f497c38e",
    "error_uri": "https://login.microsoftonline.com/error?code=50076",
    "suberror": "basic_action",
    "classification": "basic_action"
}

{}
```

**The MSAL Python version you are using**
1.9.0

**Additional context**
`invalid_grant` in this case only means the RT is invalid for acquiring an AT for VM SSH, but doesn't necessarily mean it can't be used to acquire an AT for ARM.

Use Conditional Access to configure MFA requirement for VM SSH:

![image](https://user-images.githubusercontent.com/4003950/109492421-f1def980-7ac5-11eb-942f-0a5eaaf05f42.png)

![image](https://user-images.githubusercontent.com/4003950/109492054-56e61f80-7ac5-11eb-886c-82aed88c65a0.png)


Discussion:

Patch:
diff --git a/msal/application.py b/msal/application.py
index 9fcad59..8a3fcae 100644
--- a/msal/application.py
+++ b/msal/application.py
@@ -918,11 +918,17 @@ class ClientApplication(object):
         client = self._build_client(self.client_credential, authority)
 
         response = None  # A distinguishable value to mean cache is empty
-        for entry in matches:
+        for entry in sorted(  # Since unfit RTs would not be aggressively removed,
+                              # we start from newer RTs which are more likely fit.
+                matches,
+                key=lambda e: int(e.get("last_modification_time", "0")),
+                reverse=True):
             logger.debug("Cache attempts an RT")
             response = client.obtain_token_by_refresh_token(
                 entry, rt_getter=lambda token_item: token_item["secret"],
-                on_removing_rt=rt_remover or self.token_cache.remove_rt,
+                on_removing_rt=lambda rt_item: None,  # Disable RT removal,
+                    # because an invalid_grant could be caused by new MFA policy,
+                    # the RT could still be useful for other MFA-less scope or tenant
                 on_obtaining_tokens=lambda event: self.token_cache.add(dict(
                     event,
                     environment=authority.instance,
diff --git a/msal/token_cache.py b/msal/token_cache.py
index 028635b..edc7dcb 100644
--- a/msal/token_cache.py
+++ b/msal/token_cache.py
@@ -148,9 +148,9 @@ class TokenCache(object):
         target = ' '.join(event.get("scope", []))  # Per schema, we don't sort it
 
         with self._lock:
+            now = int(time.time() if now is None else now)
 
             if access_token:
-                now = int(time.time() if now is None else now)
                 expires_in = int(  # AADv1-like endpoint returns a string
 			response.get("expires_in", 3599))
                 ext_expires_in = int(  # AADv1-like endpoint returns a string
@@ -212,6 +212,7 @@ class TokenCache(object):
                     "environment": environment,
                     "client_id": event.get("client_id"),
                     "target": target,  # Optional per schema though
+                    "last_modification_time": str(now),  # Optional. Schema defines it as a string.
                     }
                 if "foci" in response:
                     rt["family_id"] = response["foci"]
@@ -249,8 +250,10 @@ class TokenCache(object):
 
     def update_rt(self, rt_item, new_rt):
         assert rt_item.get("credential_type") == self.CredentialType.REFRESH_TOKEN
-        return self.modify(
-            self.CredentialType.REFRESH_TOKEN, rt_item, {"secret": new_rt})
+        return self.modify(self.CredentialType.REFRESH_TOKEN, rt_item, {
+            "secret": new_rt,
+            "last_modification_time": str(int(time.time())),  # Optional. Schema defines it as a string.
+            })
 
     def remove_at(self, at_item):
         assert at_item.get("credential_type") == self.CredentialType.ACCESS_TOKEN


Test patch:
diff --git a/tests/test_application.py b/tests/test_application.py
index 3c3b464..28e598b 100644
--- a/tests/test_application.py
+++ b/tests/test_application.py
@@ -73,8 +73,7 @@ class TestClientApplicationAcquireTokenSilentErrorBehaviors(unittest.TestCase):
             self.client_id, authority=self.authority_url, token_cache=self.cache)
 
     def test_cache_empty_will_be_returned_as_None(self):
-        self.assertEqual(
-            None, self.app.acquire_token_silent(['cache_miss'], self.account))
+        self.app.token_cache = msal.SerializableTokenCache()  # Reset it to empty
         self.assertEqual(
             None, self.app.acquire_token_silent_with_error(['cache_miss'], self.account))
 
diff --git a/tests/test_token_cache.py b/tests/test_token_cache.py
index 92ab7c3..3cce0c8 100644
--- a/tests/test_token_cache.py
+++ b/tests/test_token_cache.py
@@ -84,6 +84,7 @@ class TokenCacheTestCase(unittest.TestCase):
                 'credential_type': 'RefreshToken',
                 'environment': 'login.example.com',
                 'home_account_id': "uid.utid",
+                'last_modification_time': '1000',
                 'secret': 'a refresh token',
                 'target': 's2 s1 s3',
             },
@@ -157,6 +158,7 @@ class TokenCacheTestCase(unittest.TestCase):
                 'credential_type': 'RefreshToken',
                 'environment': 'fs.msidlab8.com',
                 'home_account_id': "subject",
+                'last_modification_time': "1000",
                 'secret': 'a refresh token',
                 'target': 's2 s1 s3',
             },