[Bug] can't use WithEmbeddedWebViewOptions on net8 android

softlion commented 2 months ago

Library version used

4.60.0

.NET version

net8-android

Scenario

PublicClient - mobile app

Is this a new or an existing app?

The app is in production, and I have upgraded to a new version of MSAL

Issue description and reproduction steps

When adding the line WithEmbeddedWebViewOptions that triggers the throw line 60 of https://github.com/AzureAD/microsoft-authentication-library-for-dotnet/blob/main/src/client/Microsoft.Identity.Client/ApiConfig/EmbeddedWebViewOptions.cs

If I remove the line WithEmbeddedWebViewOptions(), it is working as expected.

        var query = adClient.AcquireTokenInteractive(scopes)
            .WithUseEmbeddedWebView(true)
            .WithEmbeddedWebViewOptions(new () { Title = "Title" }) //crash
            .WithPrompt(Prompt.ForceLogin);

I updated a xamarin app to maui. Now when the embedded login view is displayed, it has a title. On xamarin it did not have a title. So I tried to change the default title, and hit the crash.

It looks like the #if condition in https://github.com/AzureAD/microsoft-authentication-library-for-dotnet/blob/main/src/client/Microsoft.Identity.Client/ApiConfig/EmbeddedWebViewOptions.cs is incorrect.

Instead of checking for Net6, it checks if the app is running on Windows.

Relevant code snippets

No response

Expected behavior

No response

Identity provider

Microsoft Entra ID (Work and School accounts and Personal Microsoft accounts)

Regression

No response

Solution and workarounds

None

bgavrilMS commented 2 months ago

This option doesn't work on mobile. But I think MSAL should not throw exception, just log a warning.

In any case, can you please set this option conditionally only on windows app (if needed).

softlion commented 2 months ago

Well, so why .WithUseEmbeddedWebView(true) does work.

You mean on mobile you can't set the title ?

Is there a way to hide that title on the embedded view ?

bgavrilMS commented 2 months ago

On mobile nobody asked to set the title before so we haven't looked into it.

softlion commented 2 months ago

On mobile nobody asked to set the title before so we haven't looked into it.

That's because there used to have no title.
When upgrading from xamarin to maui, the title appeared in the embedded web view. Why ? dunno.

localden commented 2 weeks ago

Assigning this to myself to test and validate. @softlion - can you please post a screenshot here of the behavior that you are seeing?

bgavrilMS commented 1 week ago

@localden - I am not sure it's worth testing this. We have never implemented this on Android or iOS.

The need for title customization is relevant on desktop OS (Windows, Mac ?, Linux) because of the way apps run on this OS.

You can have 10 apps running and one of them (not the "foreground" one) will require an auth prompt. For example you have Visual Studio in the background, but you are working in Excel. All of a sudden VS pops up an auth prompt, because it does some git operation which needs re-auth. This surprises the user, because Excel shows it is logged in correctly.

Afaik this cannot happen on mobile OS. A background app cannot start a web browser or broker unless it is brought back to the foreground. The "current" app is the only app that can pop up the browser / broker. There is no user confusion.

So this customization is not really needed? Or maybe we are missing some scenario @softlion ?

AzureAD / microsoft-authentication-library-for-dotnet