AzureAD / microsoft-identity-web

Helps creating protected web apps and web APIs with Microsoft identity platform and Azure AD B2C
MIT License
674 stars 208 forks source link

GetAccessTokenForUserAsync failing silently when called asynchronously #1801

Open Bidthedog opened 2 years ago

Bidthedog commented 2 years ago

Microsoft.Identity.Web Library

Microsoft.Identity.Web

Microsoft.Identity.Web version

1.25.0.0

Web app

Sign-in users and call web APIs

Web API

Protected web APIs call downstream web APIs

Token cache serialization

In-memory caches

Description

This looks to be a similar (though not the same) issue as the following:

https://github.com/AzureAD/microsoft-identity-web/issues/360

The issue is that no exception is raised, no errors are logged in the debug or trace output, and execution just stops once await _tokenAcquisition.GetAccessTokenForUserAsync(...) is called. I've tried adding additional handling for Exception in the try-catch, but that code is never hit either.

Note that this only happens once per browser session, after the application starts up. A force refresh (ctrl+F5) for each browser session results in the method being executed successfully (either I get a token, or an exception is raised and the consent redirect executes as expected), so the Blazor circuit doesn't seem to be responsible for this - and it's not actually being broken anywhere - the app just doesn't render properly because code execution just stops without notifying anything. It doesn't seem to matter if the user has just performed a fresh login, or if they logged in via a previous session.

Reproduction steps

I'm injecting ITokenAcquisition into a low level service in a Blazor Server application. The application is hosted in IISExpress during development. I have also tried hosting with dotnet.exe and the results are the same.

Note that I am calling GetToken() rather than GetTokenAsync() directly, as this is a brownfield app that has a rather convoluted synchronous callstack that I have not convinced my client to change yet. It would be very difficult for me to modify it even for testing purposes. I have no meaningful way to call the async method directly for this particular call.

The call stack is as follows:

I'm not sure if the callstack bears any responsibility for the issue, but I thought it was worth mentioning.

In memory caching is being used at present, but I am planning to change this to distributed caching soon, though I'm not sure if this will solve the problem.

Note that no consent is required by the users - all scopes are already pre-consented.

Error message

No error message is produced.

Id Web logs

Logs sanitised for privacy purposes. The problem call executes on line 147 of the log:

log.txt

Relevant code snippets

Relevant startup registration looks like this:

            services.AddTransient<IAPITokenService, AzureAADTokenService>();

            var initialScopes = config
                .GetValue<string>(ClientAuthConstants.DownstreamAPIScopesConfigKeyName)?
                .Split(' ');

            services
                .AddAuthentication(OpenIdConnectDefaults.AuthenticationScheme)
                .AddMicrosoftIdentityWebApp(config.GetSection(ClientAuthConstants.AzureADConfigSectionName))
                .EnableTokenAcquisitionToCallDownstreamApi(initialScopes)
                .AddDownstreamWebApi("MyAPI", config.GetSection("MyAPISection"))
                .AddInMemoryTokenCaches();

            services
                .AddControllersWithViews()
                .AddMicrosoftIdentityUI();

            services.AddScoped<IAuthorizationHandler, AzureADUserRequirementAuthorizationHandler>();

            services
                .AddAuthorization(options => {
                    options.AddPolicy("MyPolicyName",
                        policy => {
                            policy.RequireAuthenticatedUser();

                            policy.RequireClaim(ClaimConstants.PreferredUserName);
                            policy.RequireClaim(ClaimConstants.ObjectId);
                            policy.RequireClaim(ClaimConstants.TenantId);
                            policy.RequireClaim(ClaimConstants.Role, "my.role");

                            policy.AddRequirements(new MyUserRequirement());
                        });

                    var userPolicy = options.GetPolicy("MyPolicyName");
                    options.DefaultPolicy = userPolicy ?? options.DefaultPolicy;
                });

            services.AddMicrosoftIdentityConsentHandler();

The lower level service looks like this:

    /// <summary>
    ///     Retrieves an access token from Azure AAD.
    /// </summary>
    public class AzureAADTokenService: IAPITokenService
    {
        private readonly ITokenProvider _tokenProvider;
        private readonly ITokenAcquisition _tokenAcquisition;
        private readonly MicrosoftIdentityConsentAndConditionalAccessHandler _consentHandler;

        public AzureAADTokenService(
            ITokenProvider tokenProvider,
            ITokenAcquisition tokenAcquisition,
            MicrosoftIdentityConsentAndConditionalAccessHandler consentHandler
        )
        {
            _tokenProvider = tokenProvider;
            _tokenAcquisition = tokenAcquisition;
            _consentHandler = consentHandler;
        }

        /// <inheritdoc />
        [Obsolete("Use GetTokenAsync")]
        public string GetToken()
        {
            return GetTokenAsync().GetAwaiter().GetResult();
        }

        /// <inheritdoc />
        public async Task<string> GetTokenAsync()
        {
            // The below two links may be relevant to any issues experienced when retrieving an access token.
            // https://github.com/AzureAD/microsoft-identity-web/issues/360
            // https://github.com/AzureAD/microsoft-identity-web/wiki/Managing-incremental-consent-and-conditional-access

            try
            {
                // Always call GetAccessTokenForUserAsync so a new token is provided if the old token has expired.

                // Note that IF the in memory cache is used instead of the distributed cache, and this is the first
                // time this code has been executed for a user, this call will fail silently without raising an exception
                // and code will stop executing. There is no way to handle this at present :(
                var token = await _tokenAcquisition
                    .GetAccessTokenForUserAsync(new[]
                    {
                        "api://mydomain.com/.default"
                    });
                _tokenProvider.AccessToken = token;
            } catch(MicrosoftIdentityWebChallengeUserException ex)
            {
                // The below line will redirect the user to AAD (for consent and / or to get a new / existing access token)
                // if they do not have an active access token for the API with the specified scope.
                _consentHandler.HandleException(ex);
                return null;
            }

            return _tokenProvider.AccessToken;
        }
    }

Regression

No response

Expected behavior

The method should either produce an exception, or should produce a token, then allow the application to continue rendering. None of this happens.

Bidthedog commented 2 years ago

To follow up on this, I have now implemented distributed caching - and the problem is worse! The above issue occurs on every single refresh - so basically we can't use the app now! New code in startup (no other significant changes):

services
                .AddAuthentication(OpenIdConnectDefaults.AuthenticationScheme)
                .AddMicrosoftIdentityWebApp(config.GetSection(ClientAuthConstants.AzureADConfigSectionName))
                .EnableTokenAcquisitionToCallDownstreamApi(initialScopes)
                .AddDownstreamWebApi("MyAPIName", config.GetSection("ConfigSectionName"))
                // .AddInMemoryTokenCaches();
                .AddDistributedTokenCaches();

            // Add distributed cache options
            services.AddDistributedSqlServerCache(options =>
            {
                var cacheConnectionString = config.GetValue<string>(ClientAuthConstants.AzureADTokenCacheConnectionStringKey);
                options.ConnectionString = cacheConnectionString;
                options.SchemaName = "dbo";
                options.TableName = "MyTableName";
                options.DefaultSlidingExpiration = TimeSpan.FromMinutes(90);
            });

Any advice would be appreciated, as this is now putting a rather large downer on roughly 4 week's worth of work!

jmprieur commented 2 years ago

@Bidthedog

Did you try with that service being Scoped instead of Transient? I think this is the issue

Otherwise, do you have a repo with code we could test with and debug through? I've a hard time understanding how your AzureAADTokenService service would be called.

Bidthedog commented 2 years ago

@Bidthedog

Did you try with that service being Scoped instead of Transient? I think this is the issue

Otherwise, do you have a repo with code we could test with and debug through? I've a hard time understanding how your AzureAADTokenService service would be called.

Thanks for the reply!

Which service? The AzureAADTokenService service? Not sure why that would need to be scoped, as it doesn't store anything, it just calls ITokenAcquisition? The ITokenProvider is scoped, as that stores user tokens for other purposes.

Either way, I just tried registering it at scoped, and I get exactly the same behaviour.

I'll try and replicate it in another project today - wish me luck! :)

Bidthedog commented 2 years ago

@jmprieur I've managed to replicate the issue with a minimal call stack (you can ignore all the call stack detail I originally posted). I've narrowed it down to this being some kind of async issue when using distributed caching.

The repository with the example code is here:

https://github.com/shadow-moses-developments/AzureSSO.Spike.MS

I appreciate that the return GetTokenAsync().GetAwaiter().GetResult(); is not ideal, but the call stack in the original app is extremely convoluted and not easy to change. I have found a workaround for the time being (i.e. using Task.Run()), but using this method this is not ideal for various reasons. GetAccessTokenForUserAsync is still displaying unexpected behaviour when called in this manner - possibly a deadlock?

jmprieur commented 2 years ago

Thanks for the update, @Bidthedog. We appreciate you digging into this issue.

jennyf19 commented 1 month ago

@Bidthedog Sorry for not getting back to you. Is this still an issue with 3.0.1, and are the repro steps you shared still valid? Thanks.

valenhas commented 1 month ago

Hi @jennyf19, the issue still exists, we use:

<PackageReference Include="Microsoft.AspNetCore.Authentication.JwtBearer" Version="8.0.8" NoWarn="NU1605" />
<PackageReference Include="Microsoft.AspNetCore.Authentication.OpenIdConnect" Version="8.0.8" NoWarn="NU1605" />
<PackageReference Include="Microsoft.Identity.Web" Version="3.1.0" />
<PackageReference Include="Microsoft.Identity.Web.MicrosoftGraph" Version="3.1.0" />
<PackageReference Include="Microsoft.Identity.Web.UI" Version="3.1.0" />
<PackageReference Include="System.IdentityModel.Tokens.Jwt" Version="8.0.2" />

It's an empty string first try OnInitializedAsync, reloading the page delivers a token that is expired if login was some time ago.

We do .AddInMemoryTokenCaches(); at startup.

If you have any workarounds to get a token in this context, it would be appreciated.