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 fix] Use hub app's FRT to get nested app's AT #2379

Closed
wants to merge 4 commits into from

Conversation

somalaya
Copy link
Contributor

@somalaya somalaya commented Apr 12, 2024

Problem : In cross cloud requests, we fallback to BrokerLocallController. This is applicable for NAA requests as well.
Let's say the user performs below steps

  1. Cross cloud request from Teams app --- an FRT entry is made for teams app
  2. Cross cloud request from Outlook app --- the above FRT will be overwritten
  3. Now, if a cross cloud request from a nested app inside Teams app is made, i.e ATS request from nested app in Teams is made..
  4. Expected Behavior : ATS should succeed. Observed : ATS fails with "AADSTS900054: Specified Broker Client ID does not match ID in provided grant." error.
  5. The reason why it is failing is because, there is a logic in eSTS where they validate the clientId passed in RT is same as clientId of hub app (clientIdOfRTPassed == registeredHubClientId). Since FRT in this case is of Outlook, the request fails.

The above problem is observed in OneAuth and iOS as well. We have checked with eSTS team is they can fix this on their side to let any FRT mint an RT for a nested app and they replied that this is not acceptable by design.

Fix : When a silent request is made from a nested app in cross cloud scenario, it reaches renewAT step (OneAuth only forwards silent calls to broker when AT is expired). I have modified renewAT for nested app to follow below steps

  1. Check if hub app is part of FOCI
  2. If not, continue with normal logic of renewing AT.
  3. If yes, the RT is an FRT. We do not have any way to confirm if an FRT is current app's RT or not! There is no clientId in the key. We only maintain a familyId in the key.
  4. So, we first get an RT of current/hub app using FRT and then use the hub app's RT to renew nested app's AT.

NOTE : There is another bug on eSTS side where if an RT of a hub app is retrieved in a nested app's context and it is an FRT, we are unable to use that FRT for other apps in FOCI family. This PR does not address that issue. We are waiting for eSTS to send a fix for this.

Related broker PR : https://github.com/AzureAD/ad-accounts-for-android/pull/2772
UI tests added in : AzureAD/microsoft-authentication-library-for-android#2075

/**
* Renewing AT of nested app.
*/
protected synchronized void renewAccessTokenForNestedApp(@NonNull final SilentTokenCommandParameters parameters,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

renewAccessTokenForNestedApp

Why in BaseController? would non broker controller need this?
This method by itself has no check around NAA or FOCI, so can be called erroneously.

I think Foci RT request logic can be directly inside a BrokerLocalController private method. Then you can call renewAccessToken()

Copy link
Contributor Author

@somalaya somalaya Apr 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. This is not required in any other cases. Moved it to BrokerLocalController.

@somalaya
Copy link
Contributor Author

Closing this PR as this won't be required once Mohit implements the long term fix. He will be working on this in next sprint.

@somalaya somalaya closed this Apr 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants