Closed jiasli closed 8 months ago
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 read_account_by_id()
, unless @msamwils has different opinion. Feel free to contact me if you need more info or discussion.
@rayluo , my understanding is that MSALRuntime do not support device code flow. Should MSAL Python not even invoking MSALRuntime in this scenario?
@rayluo , my understanding is that MSALRuntime do not support device code flow. Should MSAL Python not even invoking MSALRuntime in this scenario?
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 allow_broker=True
. My proposal above is to gracefully absorb the "account not found" error from MsalRuntime, and then fallback to the RT in native MSAL's token cache. I built a proof-of-concept and it worked well. Shall I proceed?
My proposal above is to gracefully absorb the "account not found" error from MsalRuntime, and then fallback to the RT in native MSAL's token cache.
I personally don't like "fallback" as "fallback" introduces unpredictability.
My proposal above is to gracefully absorb the "account not found" error from MsalRuntime, and then fallback to the RT in native MSAL's token cache.
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.
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 with warnings.catch_warnings()
to catch that warning, and then also emit a more user friendly warning from Azure CLI to end users.
What do you think, @jiasli ?
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()"?
This is a regression in my humble opinion.
As https://github.com/AzureAD/microsoft-authentication-library-for-python/pull/569 as been merged, after bumping MSAL in https://github.com/Azure/azure-cli/pull/27726, I can verify the device code flow now works as before even when broker is enabled (by running az config set core.enable_broker_on_windows=true
).
I can see in ~/.azure/msal_token_cache.json
, the account looks like
"Account": {
"...": {
...
"username": "xxx@xxx.onmicrosoft.com",
"authority_type": "MSSTS",
"account_source": "urn:ietf:params:oauth:grant-type:device_code"
}
},
In addition, for auth code flow, account_source
is authorization_code
; for username password flow, account_source
is password
.
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: 56256e4c516e0d8469b554223f4bf3954f77437eAdditional 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):https://github.com/AzureAD/microsoft-authentication-library-for-python/blob/2168027c4694b243ddeff08592396557d9848de0/msal/application.py#L564
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.