Skip to content
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

[Bug] AcquireTokenSilent / GetAccounts / RemoveAccounts + Broker is not consistent with MSAL.Android behaviour #1820

Closed
bgavrilMS opened this issue May 14, 2020 · 4 comments

Comments

@bgavrilMS
Copy link
Member

bgavrilMS commented May 14, 2020

Which Version of MSAL are you using ?
4.13

Platform
Android

Expected behavior

GetAccounts should fetch local accounts from MSAL cache and the broker accounts and merge them by home_account_id.
RemoveAccount should remove the account and associted tokens from both local cache and from the broker
AcquireTokenSilent should first try to get a token from local cache and, if none is available, from the broker.

Note: the local cache should not contain an RT because the broker does not give MSAL the RT. But if it does, MSAL can still use it to get back a token. If there are any failures, MSAL will fallback to the broker. I have a concern here that we will silently ignore a bunch of exceptions which can lead to perf degradation...

Actual behavior

GetAccounts does not merge local and broker accounts.
RemoveAccoutns does not remove local accoutns and tokens
ATS does not search the local cache

Impact
By not merging local state with broker state, developer does not have an overview of what accounts exist and thefore not be able to use AcquireTokenSilent, but have to rely on AcquireTokenInteractive.

This affects especially users who use an app before and after the broker is installed or who uninstall the broker.

@bgavrilMS
Copy link
Member Author

CC @rpdome for confirmation

@bgavrilMS bgavrilMS added this to the 4.15 milestone May 14, 2020
@rpdome
Copy link
Member

rpdome commented May 14, 2020

The way we implemented this is that we have two controllers, LocalMsalController and BrokerMsalController.

For silent operation, if broker is installed and is eligible***, then we'll add both controller to the list and pass it to the SilentTokenCommand. The command would try to get the token with LocalMsalController first (as it's added first in the list).

Other commands that could take both controllers are

  • getAccounts (merge results from both local MSAL and broker)
  • removeAccounts (remove account from both local MSAL and broker)

The rest of the commands (in your case, interactive acquire token) will always go straight to 'the default controller'.

*** in MSAL Android, an app can opt out from using Broker via configuration file.

@bgavrilMS bgavrilMS added Investigate and removed bug labels May 14, 2020
@bgavrilMS bgavrilMS added this to Todo/Committed in MSAL.NET (legacy) via automation May 14, 2020
@bgavrilMS
Copy link
Member Author

Thank you for confirming Dome and explaining the architecture.

A couple of follow up questions:

  • For GetAccounts, I assume the merge happens using home_account_id? Do you expect or handle conflicts in any way? (e.g. if a local account and a broker account have the same home_account_id?)

  • Some RTs are more powerful than others when it comes to CA. I assume the RT on the broker is more powerful than the local RT. However, AcquireTokenSilent would prioritize local RT over broker RT. What do you think?

  • When we get AT and idT from the broker, we currently also save them to MSAL cache. I believe you MSAL.Android does not do this. Are we breaking any scenarios?

@bgavrilMS bgavrilMS modified the milestones: 4.15, Ongoing Customer Help May 27, 2020
@bgavrilMS bgavrilMS moved this from Todo/Committed to Customer Help Queue in MSAL.NET (legacy) May 27, 2020
@bgavrilMS bgavrilMS changed the title [Bug] AcquireTokenSilent + Broker is not consistent with MSAL.Android behaviour [Bug] AcquireTokenSilent / GetAccounts / RemoveAccounts + Broker is not consistent with MSAL.Android behaviour May 27, 2020
@bgavrilMS bgavrilMS added bug and removed Investigate labels May 27, 2020
@bgavrilMS bgavrilMS moved this from Customer Help Queue to Todo/Committed in MSAL.NET (legacy) May 27, 2020
@bgavrilMS bgavrilMS modified the milestones: Ongoing Customer Help, 4.15 May 27, 2020
@bgavrilMS bgavrilMS moved this from Todo/Committed to In progress in MSAL.NET (legacy) Jun 1, 2020
@bgavrilMS bgavrilMS moved this from In progress to Fixed in MSAL.NET (legacy) Jun 16, 2020
@trwalke
Copy link
Member

trwalke commented Jun 18, 2020

Resolved in 4.15

@trwalke trwalke closed this as completed Jun 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Development

No branches or pull requests

3 participants