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] ApiContractViolation exception with WithBrokerPreview() during logout #3685

Closed
1 of 7 tasks
Tracked by #3375
mcpat-it opened this issue Sep 19, 2022 · 4 comments · Fixed by #3888
Closed
1 of 7 tasks
Tracked by #3375

[Bug] ApiContractViolation exception with WithBrokerPreview() during logout #3685

mcpat-it opened this issue Sep 19, 2022 · 4 comments · Fixed by #3888

Comments

@mcpat-it
Copy link

Logs and network traces

GlobalExceptionHandler: Status: ApiContractViolation
Context: (pii)
Tag: 0x2039c1cb 
GlobalExceptionHandler: Status: ApiContractViolation
Context: (pii)
Tag: 0x2039c1cb

Which version of MSAL.NET are you using?
Microsoft.Identity.Client 4.47.0
Microsoft.Identity.Client.Broker 4.47.0-preview

Platform
.NET 6 WPF

What authentication flow has the issue?

  • Desktop / Mobile
    • Interactive
    • Integrated Windows Authentication
    • Username Password
    • Device code flow (browserless)
  • Web app
    • Authorization code
    • On-Behalf-Of
  • Daemon app
    • Service to Service calls

Is this a new or existing app?
This is a new app or experiment.

Repro

using Microsoft.Graph;
using Microsoft.Identity.Client;
using Microsoft.Identity.Client.Broker;
using System;
using System.Collections.Generic;
using System.Diagnostics;
using System.Linq;
using System.Net.Http.Headers;
using System.Runtime.InteropServices;
using System.Threading.Tasks;
using System.Windows;
using System.Windows.Interop;
using WinCopies.Linq;

namespace mcpat
{
    public class GraphApi
    {
        private GraphServiceClient? graphClient;
        readonly string tenantId = "common";
        readonly string clientId = "xxxxxxxxxxxxxxxxxx";
        readonly string[] scopes = new[] { "User.Read" };
        private readonly IPublicClientApplication? _clientApp;
        private IPublicClientApplication? PublicClientApp { get { return _clientApp; } }
        public GraphApi()
        {
            try
            {
                _clientApp = PublicClientApplicationBuilder.Create(clientId)
                    .WithAuthority(AzureCloudInstance.AzurePublic, tenantId)
                    .WithDefaultRedirectUri()
                    .WithBrokerPreview()
                    .Build();

                TokenCacheHelper.EnableSerialization(_clientApp.UserTokenCache);
            }
            catch (Exception ex)
            {
                Trace.WriteLine("Error GraphApi: " + ex.Message);
            }
        }
        public async Task SignOut()
        {
            try
            {
                if (PublicClientApp == null)
                    return;
                IEnumerable<IAccount> accounts = await PublicClientApp.GetAccountsAsync();

                if (accounts.Any())
                {
                    try
                    {
                        await PublicClientApp.RemoveAsync(accounts.FirstOrDefault());
                        Trace.WriteLine("User has signed-out");
                    }
                    catch (MsalException ex)
                    {
                        Trace.WriteLine($"MsalException Error signing-out user: {ex.Message}");
                    }
                    catch (Exception ex)
                    {
                        Trace.WriteLine($"Exception Error signing-out user: {ex.Message}");
                    }
                }
            } catch (Exception ex)
            {
                Trace.WriteLine($"Error signing-out user: {ex.Message}");
            }  
        }
        public async Task<AuthenticationResult?> GetTokenForUserAsync(Window window)
        {
            try
            {
                IAccount? firstAccount;
                var app = PublicClientApp;
                if (app == null)
                    return null;
                var accounts = await app.GetAccountsAsync();
                firstAccount = accounts.FirstOrDefault();
                AuthenticationResult? authResult = null;

                if (firstAccount != null)
                    authResult = await app.AcquireTokenSilent(scopes, firstAccount).ExecuteAsync();
                else
                    authResult = await app.AcquireTokenInteractive(scopes)
                        .WithParentActivityOrWindow(new WindowInteropHelper(window).Handle)
                        .WithUseEmbeddedWebView(true)
                        .ExecuteAsync();

                return authResult;
            }
            catch (MsalClientException ex)
            {
                int errorCode = Marshal.GetHRForException(ex) & ((1 << 16) - 1);
                Trace.WriteLine("MsalClientException (ErrCode " + errorCode + "): " + ex.ErrorCode);
            }
            catch (Exception ex)
            {
                int errorCode = Marshal.GetHRForException(ex) & ((1 << 16) - 1);
                Trace.WriteLine("Error Acquiring Token (ErrCode " + errorCode + "): " + ex);
            }

            return null;
        }
        public async Task LoginAsync(Window window)
        {
            try
            {
                if (_clientApp == null)
                    return;
                AuthenticationResult? token = await GetTokenForUserAsync(window);
                if (token == null)
                    return;
                
                DelegateAuthenticationProvider authenticationProvider = new((request) =>
                {
                    request.Headers.Authorization = new AuthenticationHeaderValue("Bearer", token.AccessToken);
                    return Task.FromResult(0);
                });

                graphClient = new GraphServiceClient(authenticationProvider);

            } catch(Exception ex)
            {
                Trace.WriteLine("Error LoginAsync: " + ex.Message);
            }
        }
    }
}

Expected behavior
During logout no exception

Actual behavior
During logout I receive the exception as written above in the log output

Possible solution
?

Additional context / logs / screenshots / links to code
I receive the exception with this piece of code:

//catch exceptions
            AppDomain.CurrentDomain.FirstChanceException += (sender, eventArgs) =>
            {
                Trace.WriteLine("GlobalExceptionHandler: " + eventArgs.Exception.Message + " " + eventArgs.Exception.InnerException);
            };
@bgavrilMS
Copy link
Member

@pwallner - I think sign-out from WAM is best effort. Obviously we should not throw an error and we'll need to fix this. But as a workaround, can you catch this exception and ignore it? I think the experience is ok, i.e.:

  • all account and token metadata are removed from MSAL's cache
  • GetAccounts() no longer returns this account.

@mcpat-it
Copy link
Author

@bgavrilMS
Yes I can ignore it, the experience is ok:

  • all account and token metadata are removed from MSAL's cache
  • GetAccounts() no longer returns this account

@gladjohn
Copy link
Contributor

gladjohn commented Jan 23, 2023

AB#2284444

During signout using MSALRuntime broker all account and token metadata are removed from MSAL's cache and
GetAccounts() no longer returns the account that was signed out. But on a test app with global exception handler we see the following error :

GlobalExceptionHandler: Status: ApiContractViolation
Context: (pii)
Tag: 0x2039c1cb 

Found the issue in Signout API where it was calling into ReleaseAuthResult to release the SignOut Result handle. Also fixed issues with Account Discovery and Read Account APIs.

Testing :

  • Added global exception handler to the debug app to catch such scenarios in the future.
  • modified debug test app to include using in the ReadAccount test so MSALRuntime issues are exposed
  • added account discovery test
  • made logging optional as it fills the console and the actual intent of using the test app is being lost

PR in Msal C++ repo for Runtime Interop

@bgavrilMS
Copy link
Member

Great catch, thank you @gladjohn

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment