AzureAD / microsoft-authentication-library-for-dotnet

Microsoft Authentication Library (MSAL) for .NET
https://aka.ms/msal-net
MIT License
1.39k stars 340 forks source link

[Bug] Microsoft.Identity.Client 4.37.0 IConfidentialClientApplication.AcquireTokenOnBehalfOf cache issue #2983

Closed thiagodpaz closed 2 years ago

thiagodpaz commented 3 years ago

Which version of MSAL.NET are you using? Microsoft.Identity.Client 4.37.0

Platform .NET Core

What authentication flow has the issue?

Is this a new or existing app? a. The app is in production, and I have upgraded to a new version of MSAL. -->

Repro

var userAssertion = new UserAssertion(someExistingAccessToken);      
var result = await app.AcquireTokenOnBehalfOf(scopes, userAssertion).ExecuteAsync();
var newToken = result?.AccessToken;
var silentToken = await app.AcquireTokenSilent(scopes, username).ExecuteAsync();

Assert.AreEqual(newToken , silentToken);

Expected behavior

After getting a token by calling IConfidentialClientApplication.AcquireTokenOnBehalfOf(), calling IConfidentialClientApplication.AcquireTokenSilent() should return the same token from the cache.

Actual behavior IConfidentialClientApplication.AcquireTokenSilent() returns a different token from a previous IConfidentialClientApplication.AcquireTokenOnBehalfOf() call. Subsequent AcquireTokenOnBehalfOf() calls re-use the last token, which suggests that AcquireTokenOnBehalfOf() reads the cache properly. Most likely AcquireTokenOnBehalfOf() doesn't write to it correctly.

Possible solution Use Microsoft.Identity.Client version 4.36.2 or below. Tested with 4.35.1 and 4.36.2.

Additional context / logs / screenshots

The IConfidentialClientApplication is configured to use IMemory Token Cache It was working fine with Microsoft.Identity.Client version 4.35.1. It broke after upgrading to 4.37.0. Reverting to the previous available stable version 4.36.2 fixed the issue.

jmprieur commented 3 years ago

@thiagodpaz when you request a token in a web API using the on behalf of flow, you don't need (and shouldn't) call AcquireTokenSilent before. AcquireTokenOnBehalfOf() checks and maintain the cache.

AcquireTokenSilent() should not return anything unless you used another flow before?

thiagodpaz commented 3 years ago

The AquireTokenSilent() is actually called after AquireTokenOnBehalf(). This is part of a unit test code of a shared library used by services that often need to call each other and benefit from the cache.

jmprieur commented 3 years ago

AcquireTokenSilent shouldn't return any token in that case? @pmaytak : can you please check if there is a regression

pmaytak commented 3 years ago

@thiagodpaz How do you create the confidential client application instance and how is in-memory token cache configured?

jmprieur commented 3 years ago

@thiagodpaz, your repro code seams to compare a token and an AuthenticationResult

Did you mean to write the following?

var userAssertion = new UserAssertion(someExistingAccessToken);      
var result = await app.AcquireTokenOnBehalfOf(scopes, userAssertion).ExecuteAsync();
var newToken = result?.AccessToken;
var silentToken = await app.AcquireTokenSilent(scopes, username).ExecuteAsync();

Assert.AreEqual(newToken , silentToken.AccessToken);

Also from which claim did you get the username?

Finally is it a web API that would also be a web app?

thiagodpaz commented 3 years ago

Hi @jmprieur, yes. Sorry, I quickly extracted the code from a wrapper code that returns the actual AccessToken property.

The unit test is using the InMemory cache. The actual code is a custom shared library consumed by several WebAPI's that can choose between InMemory, Redis or SQL.

In cases where the InMemory option is used it ends up injected like this:

services.AddInMemoryTokenCaches();

services.AddSingleton(serviceProvider => {

                var authority = string.Format(CultureInfo.InvariantCulture, settings.AadInstanceFormatString, settings.AadTenantId);

                var app = ConfidentialClientApplicationBuilder.Create(settings.AppId)
                    .WithClientSecret(settings.AppSecret)
                    .WithAuthority(new Uri(authority))
                    .WithTenantId(settings.AadTenantId)
                    .Build();

                IMsalTokenCacheProvider msalTokenCacheProvider = serviceProvider.GetRequiredService<IMsalTokenCacheProvider>();

                msalTokenCacheProvider.Initialize(app.UserTokenCache);
                msalTokenCacheProvider.Initialize(app.AppTokenCache);

                return app;
            });
pmaytak commented 3 years ago

@thiagodpaz With MSAL 4.37.0, I wasn't able to reproduce when AcquireTokenSilent retrieves an exact same previous OnBehalfOf token. OBO tokens are partitioned/saved under assertion hash, while AcquireTokenSilent searches by user account, so AcquireTokenSilent should throw MsalUiRequiredException.

Can you try when you create a ConfidentialClientApplication, add .WithLegacyCacheCompatibility(false)?

Also,

pmaytak commented 2 years ago

@thiagodpaz Closing this as we can't repro but can reopen if there's more information provided.

alejandl-msft commented 2 years ago

I'm hitting this issue as well, and also after upgrading from 4.36.0 to 4.37.0. 4.38.0 doesn't fix the issue.

Our flow looks something like this:

BareMetalConfidentialClient bareMetalClient = new BareMetalConfidentialClient(
                this.ClientApplication, this.SignedJwtClientAssertion);
            CachedToken token = await bareMetalClient.AcquireTokenByUsernamePassword(
                user, password, this.ClientApplication.ClientId);

            // Put the token in the cache by OBO'ing to ourselves
            AuthenticationResult cacheAuthResult = await this.App.AcquireTokenOnBehalfOf(
                new[] { this.ClientApplication.ClientId + "/.default" }, new UserAssertion(token.AccessToken))
                .WithSendX5C(true)
                .ExecuteAsync();

            // Use the cached account to acquire a token for the resource
            AuthenticationResult result = await this.App.AcquireTokenSilent(
                new[] { resource + "/.default" }, cacheAuthResult.Account)
                .WithSendX5C(true)
                .ExecuteAsync();

bareMetalConfidentialClient uses some HTTP calls to get an access token. We then make an on-bhealf-of call to get the token into the MSAL cache. We get an exception thrown when we call AcquireTokenSilent.

App is created this way

this.App = ConfidentialClientApplicationBuilder.Create(this.ClientApplication.ClientId)
                .WithAuthority(new Uri(this.ClientApplication.Authority))
                .WithCertificate(clientCertificate)
                .Build();
pmaytak commented 2 years ago

@alejandl-msft The behavior you describe above is correct. AcquireTokenSilent should not return a cached OBO token and should not be used along with AcquireTokenOBO. AcquireTokenOnBehalfOf does check the cache, and so if called a second time in your code sample, it will return a cached OBO token.

@jmprieur @bgavrilMS I think I repro'ed this in 4.36.0. AcquireTokenSilent filters through all cached access tokens (since the cache is not partitioned) and it filters by account ID since it's a silent search and not OBO search; then returns OBO token.

In 4.37+, AcquireTokenSilent only searches through that specific account ID partition, which of course doesn't have OBO tokens, since those are under assertion hash partitions; so it returns nothing.

So the behavior in 4.37.0+ is correct.