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] can't use WithEmbeddedWebViewOptions on net8 android #4691

Open
softlion opened this issue Mar 29, 2024 · 6 comments
Open

[Bug] can't use WithEmbeddedWebViewOptions on net8 android #4691

softlion opened this issue Mar 29, 2024 · 6 comments

Comments

@softlion
Copy link

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

@softlion softlion added needs attention Delete label after triage untriaged Do not delete. Needed for Automation labels Mar 29, 2024
@bgavrilMS
Copy link
Member

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).

@bgavrilMS bgavrilMS added bug workaround exists P3 public-client and removed untriaged Do not delete. Needed for Automation needs attention Delete label after triage labels Mar 29, 2024
@softlion
Copy link
Author

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
Copy link
Member

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

@softlion
Copy link
Author

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 localden self-assigned this May 19, 2024
@localden
Copy link
Collaborator

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

@bgavrilMS
Copy link
Member

@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 ?

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

No branches or pull requests

3 participants