Do not attempt to refresh when RT is unavailable #87
Conversation
| @@ -164,11 +164,13 @@ def _refresh_entry_if_necessary(self, entry, is_resource_specific): | |||
| now_plus_buffer = now + timedelta(minutes=Misc.CLOCK_BUFFER) | |||
|
|
|||
| if is_resource_specific and now_plus_buffer > expiry_date: | |||
| self._log.info('Cached token is expired. Refreshing: %s', expiry_date) | |||
| return self._refresh_expired_entry(entry) | |||
| if TokenResponseFields.REFRESH_TOKEN in entry: | |||
There was a problem hiding this comment.
No matched else here means we return "None"? If yes, then it means the old expired entry will stay in the cache. My concern is, the subsequent token request will further push in a new valid token, but the old one might still get matched in future, rather the new token; so more and more new tokens will be added and the older ones still stay? This will end up corrupting the token cache. If I get it right, I would suggest we delete this token before return None
There was a problem hiding this comment.
Good point. I will remove have removed the unused cache entry in my next commit. A side note: the current cache logic is somewhat complicated. We may refactor them someday in future, probably starting from scratch in MSAL implementation.
There was a problem hiding this comment.
the current cache logic is somewhat complicated
Agreed, please do improve it in MSAL
| @@ -164,11 +164,13 @@ def _refresh_entry_if_necessary(self, entry, is_resource_specific): | |||
| now_plus_buffer = now + timedelta(minutes=Misc.CLOCK_BUFFER) | |||
|
|
|||
| if is_resource_specific and now_plus_buffer > expiry_date: | |||
| self._log.info('Cached token is expired. Refreshing: %s', expiry_date) | |||
| return self._refresh_expired_entry(entry) | |||
| if TokenResponseFields.REFRESH_TOKEN in entry: | |||
There was a problem hiding this comment.
the current cache logic is somewhat complicated
Agreed, please do improve it in MSAL
This fixes #82, which was caused by the cache implementation was trying to refresh an entry, but ended up with an exception because the cached entry came from a previous
acquire_token_with_client_credentials(...)call contained no RT. This fix will skip the refresh attempt when there is no RT. A new unit test case is also included.From the library user's perspective, the intuitive way to manually reproduce #82 before this fix, and to verify the new behavior after this fix, is to duplicate the
acquire_token_with_client_credentials(...)call at the end of this sample, and see the presence and absence of the exception, before and after this fix.