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] Cache misses in client_credentials when authority is specified at request level, cloud is not global and region is used #2686

Closed
bgavrilMS opened this issue Jun 8, 2021 · 7 comments · Fixed by #2689
Assignees
Labels
Milestone

Comments

@bgavrilMS
Copy link
Member

bgavrilMS commented Jun 8, 2021

 var cca = ConfidentialClientApplicationBuilder.Create("client_id")              
              .WithClientSecret("secret")
              .WithAzureRegion("eastus")            
              .Build();

            var result = await cca.AcquireTokenForClient(new[] { "https://graph.ppe.windows.net/.default" })
                           .WithAuthority("https://login.windows-ppe.net/17b189bc-2b81-4ec5-aa51-3e628cbc931b")
                           .ExecuteAsync();

            Assert.AreEqual(TokenSource.IdentityProvider, result.AuthenticationResultMetadata.TokenSource);

            var result2 = await cca.AcquireTokenForClient(new[] { "https://graph.ppe.windows.net/.default" })
                       .WithAuthority("https://login.windows-ppe.net/17b189bc-2b81-4ec5-aa51-3e628cbc931b")
                       .ExecuteAsync();

            // fails
            Assert.AreEqual(TokenSource.Cache, result2.AuthenticationResultMetadata.TokenSource);

Workaround: Add .WithAuthority("https://login.windows-ppe.net/common") or any authority of the same cloud to the application builder.

 var cca = ConfidentialClientApplicationBuilder.Create("client_id")              
              .WithClientSecret("secret")
              .WithAzureRegion("eastus")            
              .WithAuthority("https://login.windows-ppe.net/common")              
              .Build();
@bgavrilMS bgavrilMS self-assigned this Jun 8, 2021
@bgavrilMS
Copy link
Member Author

@jmprieur - since this is our recommended pattern for client_creds flow, I think we should fix it asap.
@jennyf19 - how about we release MSAL 4.32.1 and M.I.W. at the same time tomorrow?

@jmprieur
Copy link
Contributor

jmprieur commented Jun 8, 2021

@bgavrilMS : when you write Add .WithAuthority: you mean when building the CCA?
What would be the fix? throw an exception ? I thought the authority host had to be the same? in the app builder and in the AcquireTokenXX? I'm confused?

@jennyf19
Copy link
Collaborator

jennyf19 commented Jun 8, 2021

@bgavrilMS i need more context as well, but we can release after you release.

@bgavrilMS
Copy link
Member Author

@jmprieur - I think this should just work, i.e. the authority at the request level overrides the authority at the app level.

The only conflicts between authority at app level and authority at request level are:

  • if the 2 authority types are different (e.g. B2C at app, AAD at request)
  • if the 2 AAD authorities have different tenants, (e.g. GUID1 is the tenant at app level, GUID2 at request level). Does not apply to common , organizations and consumers

@jmprieur
Copy link
Contributor

jmprieur commented Jun 8, 2021

I'm still confused.
Don't we need customers to use .WithAuthority("https://login.windows-ppe.net/something") in the CCA builder if they use .WithAuthority("https://login.windows-ppe.net/something_else") in the token acquisition builders?
given the default authority is .WithAuthority("https://login.microsoftonline.com/common")

@bgavrilMS
Copy link
Member Author

We do need it for flows where users / accounts are involved, because otherwise you cannot do app.GetAccounts(). But for client_credentials flow, I don't see why we would require it? We can just use the authority override.

Do you want to make the programming model more consistent between AcquireTokenForClient and other AcquireToken* calls and require that the 2 authorities have the same host? Throw an exception if they do not?

bgavrilMS added a commit that referenced this issue Jun 9, 2021
#2689)

* Fix for #2686 - cache misses / bad authority validation on PPE + regional

* Address PR
@jennyf19 jennyf19 added this to the 4.32.1 milestone Jun 9, 2021
@mancyc
Copy link

mancyc commented Aug 3, 2021

I hit this issue in national clouds where caching was not working because app.GetAccounts() call returned no accounts, thereby resulting in throttling from ESTS.
Upgrading from 4.31.0 to latest version(4.35) fixed the issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
4 participants