-
Notifications
You must be signed in to change notification settings - Fork 202
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
acquire_token_silent() shall not invoke broker if the account was not established by broker #569
Conversation
7281142
to
5e2a73b
Compare
I'm not sure that I agree with step 5, because it will violate the token protection promise. The scenario that can break is:
I would let the app handle this. If the app uses device code flow, they should not use broker when calling AcquireTokenSilent. |
Not necessarily.
App may already handles this - Azure CLI did - by suggesting end user to do an
Even if the app or the end user did not clear the native MSAL's token cache, the bearer AT would not meet CA requirements indeed, at that point the resource is expected to reject such an AT with a meaningful error. This shall be a defined and expected error-handling situation of CA. The end user shall be prompted (by the app such as Azure CLI) to do "az login" again which in turn will run the broker code path.
Not quite easy. In native MSAL's calling pattern, when AcquireTokenSilent is called, there is no intrinsic state to indicate whether the existing signed-in session was established by device code flow or other flow. App could handle this, but that is an extra burden that shall be handled by MSALs, somehow (we do have other options here, such as expanding token cache schema to indicate what flow an account was established by; I thought that would be more heavyweight fix).
You brought this up in an offline chat. Indeed, this use case is rare, and that is probably why this is not (yet?) reported by end users, by only found by Azure CLI team's internal coverage tests. On the other hand, besides of the "do not violate the token protection promise" design tenet, another implicit tenet is "do not break existing users and scenarios". The latter is the reason why native MSALs already have many fallback logic for scenarios that we know broker/WAM does not support: ADFS, B2C, etc., and device code flow is already one of them. We just did not do it completely. Currently, when broker is enabled, the initial device code flow is successful (which is what we want) but becomes broken after one hour (i.e. the AT expires). The latter is not our intention. |
In any case, there is a burden on the app - either they have to remember that DCF is used and follow up with ATS + no-broker or they have to provide guidance to the user (!) to clear the cache. To me, DCF is never a good solution because it doesn't satisfy device CA. So I wouldn't optimize for DCF. |
There shouldn't be a burden on the app. With this PR (or other different approaches with similar results), the app does not have to remember and differentiate between DCF and other flows. In particular, Azure CLI's guidance to the user (!) to clear the cache is debatable in itself, is not strictly necessary, and was NOT proposed for this DCF purpose (it was merely proposed as a hygiene measure: to not leave abandoned token cache files behind).
Although this issue was exposed in a DCF scenario, the purpose of this PR is not to optimize for DCF. It is to fix a logical ambiguity. When MSAL calls MsalRuntime's ReadAccountById() and the outcome is semantically an AccountNotFound, wouldn't it be a meaningful indicator suggesting that "this account was not handled by me (broker), not my business, you (msal) shall deal with it yourself"? |
ReadAccountById returned account was not found means that there is no account corresponding to that ID in MSAL C++ cache. Whether or not how native MSAL interpret and behave is purely based on the scenario. For example:
I agree with @bgavrilMS that if the MSALRuntime do not support the scenario, all the subsequent acquireToken calls should not go to the MSALRuntime. The application could give us the hint or we (in the native MSAL layer) to maintain the hint in the account cache to "remember" we are in this state. |
5e2a73b
to
bd24f23
Compare
bd24f23
to
775d110
Compare
FYI, @bgavrilMS , et al, you can ignore the lengthy and outdated conversation above, because the controversial bullet point No. 5 in the PR description has been replaced by a new bullet point No. 6. |
775d110
to
e828fcd
Compare
@@ -1628,7 +1638,7 @@ def acquire_token_by_username_password( | |||
""" | |||
claims = _merge_claims_challenge_and_capabilities( | |||
self._client_capabilities, claims_challenge) | |||
if self._enable_broker: | |||
if False: # Disabled, for now. It was if self._enable_broker: |
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.
FYI. In v1, we also expose broker for InteractiveBrowserCred, there is no support for username password for broker in Python identity too.
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.
FYI v2: We reverted course again, back to the original.
CC: @jiasli
Note: New comments, if any, shall be posted into that new feature request 702.
ROPC also bypass broker, for now Unit tests
08c7e63
to
6973a99
Compare
ROPC via WAM is enabled in #712. |
This is useful when the user somehow chose to use device code flow successfully and now want to do an acquire_token_silent(). The event sequence in this scenario looks like this:
read_account_by_id()
call, because the account was not established via MsalRuntime code path in step 2.Per our recent discussion (in May 2023) with Azure CLI team and MsalRuntime team, this PR changes to catch error in step 3, and falls back to native token cache, whose RT will work.acquire_token_silent()
calls. Also applies the same treatment to Username Password flow (a.k.a. ROPC). You may review this internal design doc here.Note: Most of the conversation below between June to October were based on the outdated bullet point No.5, so, you do not need to read them.
@MSamWils, you do not have to review the implementation details in this PR (which contains some other logistic changes), but please review the summary above.