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] DeviceCodeResultCallback doesn't execute on main or originating thread. #1555

Closed AMSoftwareNL closed 4 years ago

AMSoftwareNL commented 4 years ago

Which Version of MSAL are you using ? MSAL 4.7.1

Platform .Net Core 2.1 on PowerShell Core (6.2.2)

What authentication flow has the issue?

I'm writing a custom PowerShell Core library in .NET. So the solution run as a Cmdlet in a PowerShell console. From there I use DeviceCode flow to authenticate against Azure. Same as Login-AzAccount.

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

Repro

protected override void ProcessRecord()
{
  IPublicClientApplication pca = PublicClientApplicationBuilder.Create(PowerShellClientId)
    .WithAuthority(AzureCloudInstance.AzurePublic, TenantId)
    .WithDefaultRedirectUri()
    .Build();

    var accounts = pca.GetAccountsAsync().ConfigureAwait(continueOnCapturedContext: false).GetAwaiter().GetResult();

    AuthenticationResult authenticationResult;
    try
    {
      authenticationResult = pca.AcquireTokenSilent(new string[] { "https://globaldisco.crm.dynamics.com//.default" }, accounts.FirstOrDefault()).ExecuteAsync().ConfigureAwait(continueOnCapturedContext: false).GetAwaiter().GetResult();
    }
    catch (MsalUiRequiredException ex)
    {
      authenticationResult = pca.AcquireTokenWithDeviceCode(new string[] { "https://globaldisco.crm.dynamics.com//.default" },
      deviceCodeResult =>
      {
        WriteWarning(deviceCodeResult.Message);
        return Task.FromResult(0);
      }).ExecuteAsync().ConfigureAwait(continueOnCapturedContext: false).GetAwaiter().GetResult();
    }
    TokenCredentials credentials = new TokenCredentials(authenticationResult.AccessToken);
}

Expected behavior The message for the DeviceCodeResult is written to the PowerShell console, so I can manually open my browser and sign in. Just like Login-AzAccount does.

Actual behavior WriteWarning must be executed on the main processing thread, but this doesn't happen, so Powershell gives the following exception.

System.Management.Automation.PSInvalidOperationException: 'The WriteObject and WriteError methods cannot be called from outside the overrides of the BeginProcessing, ProcessRecord, and EndProcessing methods, and they can only be called from within the same thread. Validate that the cmdlet makes these calls correctly, or contact Microsoft Customer Support Services.'

Possible Solution Make sure the Func is always marshalled back to the correct (main or originating thread). I would assume this is also a problem in for example desktop applications where you also cannot change to UI from a background thread.

With ADAL I provide my own Action to a method to show the message, and this worked like a charm.

bgavrilMS commented 4 years ago

I would think it is the responsability of the calling application to marshal application logic to the "correct" thread. MSAL doesn't know what the "correct" thread is, it can make an assumption (i.e. the thread where you call AcquireTokenWithDeviceCode), but it is a pretty poor assumption.

bgavrilMS commented 4 years ago

Looks like dispatching to the "ui thread" (pipeline thread) is hard in powershell, as SyncronizationContext.Current is always null. Still investigating.

bgavrilMS commented 4 years ago

Easy solution

Use Console.WriteLine instead of WriteWarning since it will dispatch to the powershell console window from any thread. You can even use the colourisation options

 deviceCodeResult =>
                {
                    Console.BackgroundColor = ConsoleColor.Red;
                    Console.WriteLine(deviceCodeResult.Message);

                    return Task.FromResult(0);
                })

Better user experience

I'd recommend you use AcquireTokenInteractive instead, even on Mac and Linux. This pops up the browser and captures the result for the user. See https://github.com/AzureAD/microsoft-authentication-library-for-dotnet/wiki/System-Browser-on-.Net-Core#system-browser-experience for details

More correct (?) but harder solution

So it turns out this is a difficult problem (lots of unanswered questions on SO) because PowerShell does not implement a SyncronizationContext, so you need to do thread marshaling yourself. Have a look at https://github.com/StephenCleary/AsyncEx/wiki/AsyncContext for an idea.