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

Add mac broker support and a mac sample app #5051

Open
wants to merge 12 commits into
base: main
Choose a base branch
from
Open

Add mac broker support and a mac sample app #5051

wants to merge 12 commits into from
+978 −21

Conversation

fengga
Copy link
Contributor

@fengga fengga commented Jan 6, 2025

Changes proposed in this request

This PR added the macOS broker support via NativeInterop. Need to work with PR https://github.com/AzureAD/microsoft-authentication-library-for-cpp/pull/4438 to include NativeInterop.proj and get the needed binary files.
Also added a new MAUI sample app calling public APIs to invoke mac broker.

Testing

Performance impact

Documentation

  • All relevant documentation is updated.

bgavrilMS
bgavrilMS reviewed Jan 6, 2025
View reviewed changes
bgavrilMS
bgavrilMS reviewed Jan 6, 2025
View reviewed changes
@fengga fengga marked this pull request as ready for review January 10, 2025 05:24
@fengga fengga requested a review from a team as a code owner January 10, 2025 05:24
@bgavrilMS bgavrilMS requested a review from Copilot January 10, 2025 12:18
Copilot
Copilot AI reviewed Jan 10, 2025
View reviewed changes

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot reviewed 19 out of 34 changed files in this pull request and generated no comments.

Files not reviewed (15)
  • Mac.sln: Language not supported
  • src/client/Microsoft.Identity.Client/PublicApi/net462/PublicAPI.Shipped.txt: Language not supported
  • src/client/Microsoft.Identity.Client/PublicApi/net472/PublicAPI.Shipped.txt: Language not supported
  • src/client/Microsoft.Identity.Client/PublicApi/net8.0-android/PublicAPI.Shipped.txt: Language not supported
  • src/client/Microsoft.Identity.Client/PublicApi/net8.0-ios/PublicAPI.Shipped.txt: Language not supported
  • src/client/Microsoft.Identity.Client/PublicApi/net8.0/PublicAPI.Shipped.txt: Language not supported
  • src/client/Microsoft.Identity.Client/PublicApi/netstandard2.0/PublicAPI.Shipped.txt: Language not supported
  • tests/devapps/MauiMacAppWithBroker/App.xaml: Language not supported
  • tests/devapps/MauiMacAppWithBroker/AppShell.xaml: Language not supported
  • tests/devapps/MauiMacAppWithBroker/MainPage.xaml: Language not supported
  • tests/devapps/MauiMacAppWithBroker/MauiMacAppWithBroker.csproj: Language not supported
  • tests/Microsoft.Identity.Test.Common/Core/Helpers/OsHelper.cs: Evaluated as low risk
  • src/client/Microsoft.Identity.Client.Broker/RuntimeBroker.cs: Evaluated as low risk
  • src/client/Microsoft.Identity.Client.Broker/BrokerExtension.cs: Evaluated as low risk
  • src/client/Microsoft.Identity.Client/ApiConfig/BrokerOptions.cs: Evaluated as low risk
Comments suppressed due to low confidence (2)

tests/devapps/MauiMacAppWithBroker/MainPage.xaml.cs:30

  • [nitpick] The methods OnACIACSClicked and OnGetAllAccountsClicked have redundant code for building the PublicClientApplicationBuilder. Consider refactoring to avoid repetition.
PublicClientApplicationBuilder builder = PublicClientApplicationBuilder

tests/devapps/MauiMacAppWithBroker/MainPage.xaml.cs:102

  • [nitpick] The log file path in SampleLogging is hardcoded. Consider making it configurable.
string homeDirectory = Environment.GetEnvironmentVariable("HOME");
bgavrilMS
bgavrilMS requested changes Jan 10, 2025
View reviewed changes
Copy link
Member

@bgavrilMS bgavrilMS left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • 1 small issue with IsMac API
  • Broker packages need to be updated to reference the new runtime package

Otherwise LGTM

@gautamdsheth
Copy link

Very much interested in this :)

Could you also update the docs page whenever this gets merged ? Also, adding a sample for a console app would be awesome !

@fengga
Copy link
Contributor Author

fengga commented Jan 10, 2025

Very much interested in this :)

Could you also update the docs page whenever this gets merged ? Also, adding a sample for a console app would be awesome !

For the first version, console app scenarios will not be supported. Some crucial Runloop operations has not been added to .NET mac and the mac broker UI will not be able to pop up.
Maybe we could support console app scenarios in the future.

@fengga fengga requested a review from bgavrilMS January 13, 2025 21:41
bgavrilMS
bgavrilMS reviewed Jan 18, 2025
View reviewed changes
src/client/Microsoft.Identity.Client/ApiConfig/BrokerOptions.cs Outdated
/// <summary>
/// Use broker on OSX
/// </summary>
OSX = 0b_0000_0010, // 10
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The interop package needs to be updated. The current one only has Windows DLLs

https://nuget.info/packages/Microsoft.Identity.Client.NativeInterop/0.17.2

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like interop 0.18.0 was released and it has Linux x64 support. But no mac support https://nuget.info/packages/Microsoft.Identity.Client.NativeInterop/0.18.0

Copy link
Contributor Author

@fengga fengga Feb 27, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I published a NativeInterop-0.19.0-alpha to IDDP, which contains the mac binaries. (We can discuss the version number separately)
https://dev.azure.com/IdentityDivision/IDDP/_artifacts/feed/IDDP/NuGet/Microsoft.Identity.Client.NativeInterop/overview/0.19.0-alpha

.nupgk was generaged and can be found at https://dev.azure.com/IdentityDivision/IDDP/_build/results?buildId=1436705&view=results under the folder drop_MSALRuntime_Download_Pack_Sign_DotNet_Interop_Sign_interop_package/Nuget/

Our original plan was first publishing all the packages to IDDP, and partners verify everything works fine, then publish to nuget.org. Does this make sense?

bgavrilMS
bgavrilMS requested changes Jan 18, 2025
View reviewed changes
Copy link
Member

@bgavrilMS bgavrilMS left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interop package needs a bump. See the other PR about Linux broker too.

gladjohn
gladjohn reviewed Jan 18, 2025
View reviewed changes
Copy link
Contributor

@gladjohn gladjohn left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good.

  • CI pipeline integration of the test app is a must.
  • As Bogdan mentioned, need an updated Interop with MAC runtimes
  • Highly recommend building a broker app with MAC libs/runtimes and testing from within Visual Studio for MAC. Ashok/Chris maybe able to help here with some guidance if needed.

src/client/Microsoft.Identity.Client.Broker/RuntimeBroker.cs
@@ -100,10 +100,22 @@ public RuntimeBroker(
s_lazyCore.Value.EnablePii(_logger.PiiLoggingEnabled);
}

_parentHandle = GetParentWindow(uiParent);
if (DesktopOsHelper.IsWindows())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please update the summary of this method

tests/devapps/MauiMacAppWithBroker/MainPage.xaml.cs
using System;
using System.Runtime.InteropServices;

public partial class MainPage : ContentPage
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you please add this dev app to the pipeline, build it, and make sure the interop files are placed in the appropriate folders

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see here : https://github.com/AzureAD/microsoft-authentication-library-for-dotnet/blob/main/build/template-build-and-prep-automation.yaml#L63

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we track it with another work item? I created one https://identitydivision.visualstudio.com/Engineering/_workitems/edit/3168507
It's in my epic and will not be forgotten

tests/devapps/MauiMacAppWithBroker/MainPage.xaml.cs Outdated
inUse = true;

PublicClientApplicationBuilder builder = PublicClientApplicationBuilder
.Create("04b07795-8ddb-461a-bbee-02f9e1bf7b46")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what app is this? can we not use one from the lab?

tests/devapps/MauiMacAppWithBroker/MainPage.xaml.cs Outdated
PublicClientApplicationBuilder builder = PublicClientApplicationBuilder
.Create("04b07795-8ddb-461a-bbee-02f9e1bf7b46")
.WithRedirectUri("msauth.com.msauth.unsignedapp://auth")
.WithClientCapabilities(new[] { "cp1" })
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do you need WithClientCapabilities the request is not even passing any claims or testing the CAE flow

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

@bgavrilMS bgavrilMS bgavrilMS requested changes

@gladjohn gladjohn gladjohn left review comments

Copilot code review Copilot Copilot left review comments

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@fengga @gautamdsheth @bgavrilMS @gladjohn