AzureAD / microsoft-authentication-library-for-dotnet

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

[Unexpected behaviour] WAM Broker with CAE token cache #4430

Open peter1155 opened 10 months ago

peter1155 commented 10 months ago

Library version used

4.56.0

.NET version

net6.0-windows10.0.17763.0

Scenario

PublicClient - desktop app

Is this a new or an existing app?

The app is in production, I haven't upgraded MSAL, but started seeing this issue

Issue description and reproduction steps

We recently switched from usage of SystemWebBrowser to WAM and started to see some strange behavior regarding continuous access evaluation. After user session is revoked and token requested the MsalUiRequiredException is raised by MSAL then after successful interactive login the token is acquired. Then later token is acquired by AcquireTokenSilent and send to MsGraph API. The API retruns HTTP 401 with claim challange. (I am not sure whether this is expected after interactive login.) After passing the claim challange to MSAL using .WithClaims() access token is successfully acquired. But the next call to acquire token without the claim challange returns invalid token and again MsGraph API returns 401.

So I am not sure is CAE supported in conjunction with WAM ? We are using Microsoft.Identiti.Client.Broker 4.56.0.

Relevant code snippets

...
    public async Task<AuthenticationResult> GetAccessTokenByEmailWithPossibleUiPromptAsync(IEnumerable<AudiencePermissions> audiencePermissionsList,
    string email,
    IEnumerable<AudiencePermissions> consentAudiencePermissionsList = null,
    CancellationToken cancellationToken = default,
    bool retriedOnFailure = false,
    bool accountSwitchAllowed = false,
    string claimChallenge = null)
{
    var account = await FindLocalAccount(email);
    var scopes = GetScopes(audiencePermissionsList);
    var consentScopes = consentAudiencePermissionsList != null
        ? GetScopes(consentAudiencePermissionsList)
        : Enumerable.Empty<string>();

    // Try get token from cache
    try
    {
        if (account != null)
        {
            var silentAuthResult = await AcquireTokenSilentlyAsync(scopes, account, claimChallenge, cancellationToken);
            if (silentAuthResult != null)
            {
                log.LogInformation("Token acquired silently");
                telemetryLogger.LogTrace(LogEvent.BusinessLogic.Authentication.AccessToken.AcquireSilently, "Token acquired silently.");
                return Parse(silentAuthResult, false);
            }
        }

        return await AcquireTokenInteractivelyAsync(scopes, consentScopes, email, cancellationToken, true, accountSwitchAllowed, claimChallenge);
    }
    catch (MsalUiRequiredException ex)
    {        
        return await AcquireTokenInteractivelyAsync(scopes, consentScopes, email, cancellationToken, retriedOnFailure, accountSwitchAllowed, claimChallenge ?? ex.Claims);

    }
}
....

private async Task<AuthenticationResult> AcquireTokenInteractivelyAsync(IEnumerable<string> scopes,
    IEnumerable<string> consentScopes,
    string email,
    CancellationToken cancellationToken = default,
    bool retriedOnFailure = false,
    bool accountSwitchAllowed = false,
    string claimChallenge = null)
{

 ....
        async () =>
        {
            try
            {
                var builder = PublicClient.AcquireTokenInteractive(scopes);
                builder = builder
                            .WithLoginHint(email)
                            .WithClaims(claimChallenge)
                            .WithParentActivityOrWindow(appWindowHandleProvider.GetAppWindowHandle())
                            .WithPrompt(Prompt.ForceLogin)
                            .WithCorrelationId(CorrelationContext.Current.CorrelationId);

                if (!accountServiceOptions.UseMsalWindowsBroker)
                {
                    builder.WithUseEmbeddedWebView(false);
                }

                if (consentScopes.Any())
                {
                    builder = builder.WithExtraScopesToConsent(consentScopes);
                }

                var authResponse = await builder.ExecuteAsync(CancellationToken.None);
                var wasAccountSwitched = authResponse?.Account?.Username != email;

                if (wasAccountSwitched && !accountSwitchAllowed)
                {
                    throw new AccountSwitchDeniedException(email);
                }

                log.LogInformation("User {User} was successfully authenticated", authResponse.Account.Username);

                return Parse(authResponse, wasAccountSwitched);
            }
            catch (MsalClientException ex) when (ex.ErrorCode?.Equals("authentication_canceled") ?? false)
            {
                throw new MsalAuthenticationCanceledException(ex);
            }
            catch (MsalServiceException ex) when (ex.ErrorCode?.Equals("access_denied") ?? false)
            {
                throw new OperationCanceledException(ex.Message, ex);
            }            
        },
....
....
        private async Task<MSIdentityClient.AuthenticationResult> AcquireTokenSilentlyAsync(IEnumerable<string> scopes, IAccount account, string claimChallenge = null, CancellationToken cancellationToken = default)
        {
            return await client.AcquireTokenSilent(scopes, account)
                               .WithClaims(claimChallenge)
                               .ExecuteAsync(cancellationToken);
        }
...

Expected behavior

I would expect that after acquiring token with non empty claim challenge the previous cached token should be invalidated. And each subsequent call to AcquireTokenSilent/AcquireTokenInteractive should return updated token.

Identity provider

Other

Regression

No response

Solution and workarounds

No response

bgavrilMS commented 10 months ago

Thanks for reporting. @ashok672, @iulico-1 - this is potentially breaking the CAE scenario, so marking as P1.

bgavrilMS commented 9 months ago

@iulico-1 @ashok672 - I think this one needs to be investigated with priority, as it seems to break critical CAE scenario.

I'm wondering if there's a mismatch in how MSAL C++ and other MSALs handle claims?

With MSAL.NET alone, once a claims challenge comes in, we ask the app developer to use WithClaims(challenge) - this bypasses the token cache. The new token overrides the old token. Claims are not part of the cache key.

With MSAL C++, afaik claims are part of the key. Are apps supposed to send the claim over and over ?

peter1155 commented 1 day ago

Hi @bgavrilMS, any update here ?

bgavrilMS commented 1 day ago

CC @localden , this might have been missed in triage