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.

Sign up for GitHub

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

Jump to bottom

[Authentication] After Uno.Sdk update, MSAL login on Windows using msal redirect url gives Microsoft.Identity.Client.MsalClientException #2443

Open
2 tasks done
VincentH-Net opened this issue Jul 17, 2024 · 10 comments · May be fixed by #2631
Open
2 tasks done

[Authentication] After Uno.Sdk update, MSAL login on Windows using msal redirect url gives Microsoft.Identity.Client.MsalClientException #2443

VincentH-Net opened this issue Jul 17, 2024 · 10 comments · May be fixed by #2631
Assignees
@eriklimakc
Labels
kind/bug Something isn't working triage/untriaged Indicates an issue requires triaging or verification.

Comments

@VincentH-Net
Copy link

VincentH-Net commented Jul 17, 2024 •
edited
Loading

Current behavior

After updating Uno.Sdk from 5.2.139 to 5.2.175, IAuthenticationService.LoginAsync gives this exception on Windows:

Microsoft.Identity.Client.MsalClientException: 'Only loopback redirect uri is supported, but msal<Application (client) ID guid>://auth/ was found. Configure http://localhost or http://localhost:port both during app registration and when you create the PublicClientApplication object. See https://aka.ms/msal-net-os-browser for details'

When downgrading Uno.Sdk back to 5.2.139, there is no exception and the native Windows account picker appears.
However, NuGet reports a security vulnerability that is fixed by updating Uno.Sdk, so downgrading is not an acceptable workaround.

MSAL configuration and code:
appsettings.json

  "MsalAuthentication": {
    "ClientId": "<Application (client) ID guid>",
    "Scopes": [ "User.Read" ],
    "RedirectUri": "msal<Application (client) ID guid>://auth"
  }

App.xaml.cs

.UseAuthentication(auth => auth.AddMsal(name: "MsalAuthentication"))

LoginViewModel.cs

public partial class LoginViewModel(
    IDispatcher dispatcher,
    IAuthenticationService authentication,
   // ...
) : BaseViewModel
{
    [RelayCommand]
    async Task Login()
    {
        bool success = await authentication.LoginAsync(dispatcher);
        // ...
    }
}

Workaround

Do what the exception message says:

  1. Register an additional desktop application redirect url in Entra Id
  2. In code for Windows only, set that URL: 
    .UseAuthentication(auth =>
  auth.AddMsal(
#if WINDOWS
    c => c.Builder(b => b.WithRedirectUri("http://localhost:5001")),
#endif
    name: "MsalAuthentication"
  )
)

However, this workaround will open the account picker in the web browser instead of use the native Windows account picker.

Expected behavior

I can update to the latest stable Uno Sdk and continue to use the native account picker on Windows with the Uno Auth Extensions.

How to reproduce it (as minimally and precisely as possible)

  1. Register an Entra Id application with the msal redirect url
  2. Create a new Uno Platform solution with authentication and Windows platform selected
  3. Update the solution as shown in the code snippets under "Current Behavior" above
  4. Run the app and click on Login

Environment

Nuget Package (s):
Uno.Sdk packages for:

    <UnoFeatures>
      CSharpMarkup;
      Material;
      Dsp;
      Hosting;
      Toolkit;
      Logging;
      LoggingSerilog;
      Mvvm;
      Configuration;
      Http;
      Serialization;
      Localization;
      AuthenticationMsal;
      Navigation;
      ThemeService;
    </UnoFeatures>

Package Version(s):

{
  "sdk": {
    "version": "8.0.300"
  },
  "msbuild-sdks": {
    "Uno.Sdk": "5.2.175"
  }
}

Affected platform(s):

  • Windows

Visual Studio:

  • 2022 (version: 17.10.0)
@VincentH-Net VincentH-Net added kind/bug Something isn't working triage/untriaged Indicates an issue requires triaging or verification. labels Jul 17, 2024
@VincentH-Net
Copy link
Author

Update: the issue still exists in "Uno.Sdk": "5.3.96"

@agneszitte
Copy link
Contributor

Hi @VincentH-Net are you still able to reproduce this issue with latest stable Uno.Sdk 5.4.8 please?

@VincentH-Net
Copy link
Author

Thanks for following up @agneszitte , I will recheck this Monday and update here.

In case the issue still exists, this may help to track it down:

As far as I can see the issue was introduced when a vulnerability in the underlying MS lib forced Uno to update. So to get rid of that vulnerability build warning it is necessary to update the Uno lib and then the issue is that you lose the native UX on Windows.

@VincentH-Net
Copy link
Author

@agneszitte I rechecked with latest stable "Uno.Sdk": "5.4.8" - the issue still exists

@VincentH-Net
Copy link
Author

VincentH-Net commented Oct 7, 2024 •
edited
Loading

@agneszitte It seems to be this MSAL issue

This comment points to this PR as the example to follow how to use the native Windows broker.

That PR uses both .WithWindowsEmbeddedBrowserSupport() and .WithBroker(new BrokerOptions(BrokerOptions.OperatingSystems.Windows)) for Windows.

If that does restore the native Windows UX you may need to implement it in the Uno wrapper for MSAL

@agneszitte
Copy link
Contributor

@agneszitte It seems to be this MSAL issue

This comment points to this PR as the example to follow how to use the native Windows broker.

That PR uses both .WithWindowsEmbeddedBrowserSupport() and .WithBroker(new BrokerOptions(BrokerOptions.OperatingSystems.Windows)) for Windows.

If that does restore the native Windows UX you may need to implement it in the Uno wrapper for MSAL

@VincentH-Net thanks a lot for the test and all the details, really appreciated !
We will look at the details (cc @jeromelaban, @nickrandolph, @kazo0, @eriklimakc for info)

@MartinZikmund
Copy link
Member

Afaik opening the account picker in the default browser is the recommended approach as per the latest security standards (instead of the embedded browser) - the docs say so quite overwhelmingly as well - https://learn.microsoft.com/en-us/entra/msal/dotnet/acquiring-tokens/using-web-browsers#embedded-web-view-vs-system-browser

@kazo0 kazo0 linked a pull request Dec 5, 2024 that will close this issue
fix: Add msal fix for MSALClientException #2631
Open
7 tasks
@VincentH-Net
Copy link
Author

VincentH-Net commented Dec 13, 2024 •
edited
Loading

@MartinZikmund

Afaik opening the account picker in the default browser is the recommended approach as per the latest security standards (instead of the embedded browser) - the docs say so quite overwhelmingly as well - https://learn.microsoft.com/en-us/entra/msal/dotnet/acquiring-tokens/using-web-browsers#embedded-web-view-vs-system-browser

Correct for a browser based approach - however the desired and recommended behavior on Windows is to use the native WAM, which is built into the OS:
https://learn.microsoft.com/en-us/entra/msal/dotnet/acquiring-tokens/desktop-mobile/wam

Using the native Windows WAM was the behavior up to Uno.Sdk 5.2.139, which was broken in 5.2.175; that is what I referred to in this issue:

Expected behavior
I can update to the latest stable Uno Sdk and continue to use the native account picker on Windows with the Uno Auth Extensions.

@kazo0 I hope that #2631 enables using the native Windows WAM, not an embedded browser?

@VincentH-Net VincentH-Net linked a pull request Dec 13, 2024 that will close this issue
Open
7 tasks
@kazo0
Copy link
Contributor

kazo0 commented Dec 13, 2024

@kazo0 I hope that #2631 enables using the native Windows WAM, not an embedded browser?

@eriklimakc If I'm not mistaken, #2631 should fix this and continue to use the native Windows WAM

@VincentH-Net, I'll be working to get this merged today and we can validate

@VincentH-Net
Copy link
Author

Thanks @kazo0, please look at below comment that I added to #2631

It describes what seems to be missing in the fix to get native Windows WAM and why, and how to solve it.

#2631 (comment)

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

@eriklimakc eriklimakc

Labels
kind/bug Something isn't working triage/untriaged Indicates an issue requires triaging or verification.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

fix: Add msal fix for MSALClientException unoplatform/uno.extensions
5 participants
@MartinZikmund @VincentH-Net @kazo0 @agneszitte @eriklimakc