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

Give BrokerMsalController higher priority than LocalMsalController if available #2040

Merged
merged 4 commits into from
Mar 15, 2024

Conversation

iamgusain
Copy link
Contributor

@iamgusain iamgusain commented Feb 14, 2024

What

Giving BrokerMsalController higher priority than LocalMsalController if available. This means that if Broker is available, msal will first try to acquire token via broker before trying its localController

Why

In order to push brokered auth as default when available. And reduce usage of regular RT, when PRT can be used which is bound to the device.

How

Changing the order of controllers to put BrokerMsalController (if eligible) before LocalMsalController

Testing

Verified AcquireTokenSilent calls reach Broker first.

@github-actions github-actions bot added the msal label Feb 14, 2024
@iamgusain iamgusain marked this pull request as ready for review February 14, 2024 22:03
@iamgusain iamgusain requested a review from a team as a code owner February 14, 2024 22:03
@rpdome
Copy link
Member

rpdome commented Feb 15, 2024

What would happen in the following scenario

  1. Sign in with MSAL only
  2. Install Broker
  3. try to get a token again

would the user be prompted?

@mohitc1
Copy link
Contributor

mohitc1 commented Feb 15, 2024

 * 5) The broker redirect URI for the client is registered

This comment should be revised based on final logic in the method.


Refers to: msal/src/main/java/com/microsoft/identity/client/internal/controllers/MSALControllerFactory.kt:101 in 986bd62. [](commit_id = 986bd62, deletion_comment = False)

@iamgusain
Copy link
Contributor Author

What would happen in the following scenario

  1. Sign in with MSAL only
  2. Install Broker
  3. try to get a token again

would the user be prompted?

Silent requests will fallback to local controller so no prompt.
Interactive requests will anyway go to broker so users will be prompted.

@iamgusain iamgusain merged commit 08f1c47 into dev Mar 15, 2024
9 checks passed
somalaya added a commit to AzureAD/microsoft-authentication-library-common-for-android that referenced this pull request Mar 20, 2024
Issue : AT PoP generate SHR tests are failing in the daily pipeline

Cause : In this PR, we changed the order of the controller -
AzureAD/microsoft-authentication-library-for-android#2040.
We are first adding BrokerMsalController if broker is installed. After
this change, GenerateSHRCommand was first trying to generateSHR with
BrokerMsalController and it would succeed. But the result is not
returned. It would then try with LocalMsalController and fails because
the account is added to broker. This is an existing bug which got
surfaced with recent changes.

Fix : Return from GenerateSHRCommand with the result if a controller
returns a successful response.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants