-
Notifications
You must be signed in to change notification settings - Fork 204
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Bubble up refresh exception when we cannot recover #434
Conversation
@@ -1207,7 +1207,9 @@ def _acquire_token_silent_from_cache_and_possibly_refresh_it( | |||
if (result and "error" not in result) or (not access_token_from_cache): | |||
return result | |||
except: # The exact HTTP exception is transportation-layer dependent | |||
logger.exception("Refresh token failed") # Potential AAD outage? | |||
# Typically network error. Potential AAD outage? | |||
if not access_token_from_cache: # It means there is no fall back option |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is possible for access_token_from_cache
to be not-None
here? It seems L1198 will prevent a not-None
access_token_from_cache
to reach here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Line 1198 would be skipped by line 1196. So, this check here is still beneficial.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I see. Even if AT_AGING
refresh fails, access_token_from_cache
can serve as a fallback. Thanks for the explanation.
Tested by manually changing It fails at Full call stack
Without the change it fails at Full call stack
If the tenant discovery succeeds, it will fail in CLI code as shown in the issue description. |
This will fix #431.
We won't have an easy way to test this. So, @jiasli please help the code review.
And, if the ConnectionError in Xing Zhuo's report is still observable, you can then pull in this feature branch and test it in rare real environment (by
pip install git+https://github.com/AzureAD/microsoft-authentication-library-for-python.git@bubble-up-refresh-exception
).