-
Notifications
You must be signed in to change notification settings - Fork 201
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
Using allow_broker
with device code flow causes token refreshing to fail: Account with id '(pii)' not found.
#563
Comments
Yes, I can reproduce this issue. And @jiasli guessed the reason right, "Apparently, this is because device code's refresh token is saved to MSAL token cache, not WAM, but during token refreshing, MSAL tries to get access token from WAM." The reason for that is, back then when we implemented the Broker/MsalRuntime/WAM integration, the guidance was "surface all MsalRuntime errors, instead of fallback to non-MsalRuntime code path". Now that I think about it, an MsalRuntime error of "hey I cannot find this account inside my (MsalRuntime's) cache so it is not my business", is not really an error, but an invite to tell MSAL Python to fallback gracefully. I incline to change MSAL Python to also fallback when the error happens in |
@rayluo , my understanding is that MSALRuntime do not support device code flow. |
MSAL Python already does not invoke MsalRuntime in device code flow. The current issue is on the second leg i.e. the acquire_token_silent(). At that point, MSAL Python will attempt MsalRuntime based on |
I personally don't like "fallback" as "fallback" introduces unpredictability. |
Fair enough, and perhaps "fallback" is not the right word here. In my book (and probably in your "fallback introduces unpredictability" statement, too), fallback hints "some unknown error occurred and we do not know how to deal with it, so let's fallback to a default option". But in this case, "account not found" is actually expected, and going back to the non-broker code path is the right move. If there are different proposal, please let us know. |
An update on this topic. We tried #569 but that approach was controversial. How about MSAL simply emits a warning during the initial Device Code Flow saying something like "Broker was enabled. Subsequent acquire_token_silent() will only work after a successful acquire_token_interactive()"? Azure CLI can then use What do you think, @jiasli ? |
This is a regression in my humble opinion. As #569 as been merged, after bumping MSAL in Azure/azure-cli#27726, I can verify the device code flow now works as before even when broker is enabled (by running I can see in
In addition, for auth code flow, |
Describe the bug
Using
allow_broker
with device code flow causes token refreshing to fail:Account with id '(pii)' not found.
To Reproduce
Expected behavior
Token refreshing should succeed.
What you see instead
Apparently, this is because device code's refresh token is saved to MSAL token cache, not WAM, but during token refreshing, MSAL tries to get access token from WAM.
The MSAL Python version you are using
Latest
dev
branch: 56256e4Additional context
I understand there is no point to use device code flow when broker is available, but it's better to make the whole workflow more user-friendly.
If MSAL exposes
enable_broker
rather thanallow_broker
(like what it is doing internally):microsoft-authentication-library-for-python/msal/application.py
Line 564 in 2168027
life will be much easier - simply raise an error if
acquire_token_by_device_flow
is called on aPublicClientApplication
withenable_broker=True
.Such
allow_...
design makes the logic far more complex - bothacquire_token_by_device_flow
andacquire_token_silent_with_error
need to implement such fallback logic and be consistent with each other. In this issue,acquire_token_by_device_flow
falls back to MSAL token cache, butacquire_token_silent_with_error
doesn't and still reads from WAM.The text was updated successfully, but these errors were encountered: