Enable broker support on Linux platform #5086
Conversation
src/client/Microsoft.Identity.Client.Broker/Microsoft.Identity.Client.Broker.csproj
Outdated
Show resolved
Hide resolved
| @@ -0,0 +1,19 @@ | |||
| <Project Sdk="Microsoft.NET.Sdk"> | |||
There was a problem hiding this comment.
can you also add this project to the MSAL Runtime dll checks that happens in the build pipepline
| @@ -586,7 +587,7 @@ public void HandleInstallUrl(string appLink) | |||
|
|
|||
| public bool IsBrokerInstalledAndInvokable(AuthorityType authorityType) | |||
| { | |||
| if (!DesktopOsHelper.IsWin10OrServerEquivalent()) | |||
| if (!DesktopOsHelper.IsWin10OrServerEquivalent() && !DesktopOsHelper.IsLinux()) | |||
There was a problem hiding this comment.
are you able to add some unit tests?
There was a problem hiding this comment.
Well, we don't really have unit tests for these helpers and I'm not sure how useful they'd be.
There was a problem hiding this comment.
Hi, for this PR, I start from enabling the Microsoft.Identity.Test.Integration.NetCore tests in the CI for linux platform. Please take a look. the Unit tests rely on Microsoft.Identity.Client.Desktop which is Windows only
There was a problem hiding this comment.
Copilot reviewed 5 out of 10 changed files in this pull request and generated no comments.
Files not reviewed (5)
- Directory.Packages.props: Language not supported
- NuGet.Config: Language not supported
- src/client/Microsoft.Identity.Client.Broker/Microsoft.Identity.Client.Broker.csproj: Language not supported
- tests/devapps/WAM/NetWSLWam/Properties/launchSettings.json: Language not supported
- tests/devapps/WAM/NetWSLWam/test.csproj: Language not supported
Comments suppressed due to low confidence (2)
src/client/Microsoft.Identity.Client/PlatformsCommon/Shared/DesktopOsHelper.cs:59
- [nitpick] The method name 'IsRunningOnWsl' could be more descriptive. Consider renaming it to 'IsRunningOnWindowsSubsystemForLinux'.
public static bool IsRunningOnWsl()
src/client/Microsoft.Identity.Client.Broker/RuntimeBroker.cs:129
- Replace the debug log statement with a proper logging mechanism: _logger.Info("Runtime Broker AcquireTokenInteractiveAsync");
Console.WriteLine("Runtime Broker AcquireTokenInteractiveAsync");
| @@ -79,6 +79,20 @@ private static void AddRuntimeSupport(PublicClientApplicationBuilder builder) | |||
| logger.Info("[Runtime] WAM supported OS."); | |||
| return new RuntimeBroker(uiParent, appConfig, logger); | |||
| }; | |||
| } else if (DesktopOsHelper.IsRunningOnWsl()) { | |||
There was a problem hiding this comment.
can you create a helper method so the dupe can be avoided?
src/client/Microsoft.Identity.Client/PlatformsCommon/Shared/DesktopOsHelper.cs
Show resolved
Hide resolved
| /// <summary> | ||
| /// Use broker on WSL | ||
| /// </summary> | ||
| WSL = 0b_0000_0011, // 3 |
There was a problem hiding this comment.
The reason we made the OS an explicit API is to allow app developers to opt-in to different brokers, because different brokers have different:
- redirect_uri
- parent window details
Is the E2E setup on Linux different than the broker setup on WSL? If not, I would not complicate the dev experience with this option and I'd just add "Linux".
There was a problem hiding this comment.
If it is, pls explain with a comment in the code, ideally pointing to an aka.ms doc
There was a problem hiding this comment.
I agree with what @bgavrilMS said. To that end, I am curious to know what the scope of this PR is. Does it bring broker support for BOTH wsl and non-wsl, @xinyuxu1026 ?
For what it's worth, there is currently a similar PR for MSAL Python, but it targets wsl only; and I am told that the "wsl broker" is WAM so it has the redirect_uri requirement identical to WAM, and the "non-wsl broker" does not use that redirect_uri.
There was a problem hiding this comment.
Hi, this PR would support for both WSL and Standalone linux environments. For the redirect_uri, the Linux platform doesn't require a new one to be registered, it should be able to use the same redirect_uri as Windows platform. For the parent window, on Linux we import libX11 to get the window handle, and this applies to both Linux and WSL since they are both Linux environments. Therefore, I removed the WSL broker options.
Fixes #
Changes proposed in this request
Testing
Performance impact
Documentation