AzureAD / microsoft-authentication-cli

A command line utility for Azure authentication.
Other
42 stars 9 forks source link

Upgrade MSAL dependencies to 4.61.3 #400

Closed keyuxuan closed 1 month ago

keyuxuan commented 1 month ago

Changes:

  1. Upgrading MSAL version to 4.61.3
  2. Starting from MSAL version 4.61.0, usage of WithBroker method with no parameter is deprecated, so the method clientBuilder.WithBroker(); used in BrokerBenchmark class becomes obsolete.

    According to the release log, the alternative to that is to reference Microsoft.Identity.Client.Desktop when authenticating with browser and call WithWindowsEmbeddedBrowserSupport().

    I tried that, but looks like Microsoft.Identity.Client.Desktop only support Windows platform, it does not support macos and ubuntu platforms, so the Build and Test failed for those 2 platforms from my earlier commits .

    Also currently in Broker class, we are explicitly only using interactive authentication with Windows Broker, so the else block (which is used for ManagedBrokerBenchmark) in BrokerBenchmark class seems unnecessary. So i decided to just remove the else block.

Testing

Benchmark test

4.61.3


BenchmarkDotNet=v0.13.2, OS=Windows 11 (10.0.22631.4037)
Intel Core i9-10900K CPU 3.70GHz, 1 CPU, 20 logical and 10 physical cores
.NET SDK=8.0.304
  [Host]     : .NET 6.0.33 (6.0.3324.36610), X64 RyuJIT AVX2
  DefaultJob : .NET 6.0.33 (6.0.3324.36610), X64 RyuJIT AVX2
Method Mean Error StdDev
NativeBrokerBenchmark 42.02 ms 0.329 ms 0.275 ms

4.59.1

BenchmarkDotNet=v0.13.2, OS=Windows 11 (10.0.22631.4037)
Intel Core i9-10900K CPU 3.70GHz, 1 CPU, 20 logical and 10 physical cores
.NET SDK=8.0.304
  [Host]     : .NET 6.0.33 (6.0.3324.36610), X64 RyuJIT AVX2
  DefaultJob : .NET 6.0.33 (6.0.3324.36610), X64 RyuJIT AVX2
Method Mean Error StdDev
NativeBrokerBenchmark 44.16 ms 0.149 ms 0.132 ms

Other

Tested following commands on both mac and windows:

azureauth --help
azureauth aad --help
azureauth ado pat --help
azureauth ado token --help
azureauth ado pat scopes --help
azureauth info --help
azureauth ado token
azureauth ado pat
azureauth aad (with different output and auth modes)
shalinikhare27 commented 1 month ago

I am curious that since Microsoft.Identity.Client.Desktop is only supported in Windows, did we choose to simply not move to this alternative approach?

mijpeterson commented 1 month ago

FWIW, I think broker/WAM is Windows only. So I think we'd actively want to avoid doing any broker specific logic on Mac and Linux. Someone correct me if I'm wrong here.

keyuxuan commented 1 month ago

I am curious that since Microsoft.Identity.Client.Desktop is only supported in Windows, did we choose to simply not move to this alternative approach?

i dont think we can? because using Microsoft.Identity.Client.Desktop would cause Build and Test to fail for macos and ubuntu platforms

Haard30 commented 1 month ago

I am curious that since Microsoft.Identity.Client.Desktop is only supported in Windows, did we choose to simply not move to this alternative approach?

Shalini, were you asking about this PR where we moved away from Microsoft.Identity.Client.Desktop? If so yes, we moved away from that library earlier and use the alternative approach of newer Broker API. I think we still kept managed broker usage in benchmarking project to compare execution time of both broker apis. But MSAL has decided to deprecate that old API so we can no longer use that directly for benchmarking.

Haard30 commented 1 month ago

I am curious that since Microsoft.Identity.Client.Desktop is only supported in Windows, did we choose to simply not move to this alternative approach?

Shalini, were you asking about this PR where we moved away from Microsoft.Identity.Client.Desktop? If so yes, we moved away from that library earlier and use the alternative approach of newer Broker API. I think we still kept managed broker usage in benchmarking project to compare execution time of both broker apis. But MSAL has decided to deprecate that old API so we can no longer use that directly for benchmarking.

Keyu pointed out that we didn't use/remove Microsoft.Identity.Client.Desktop in that PR but we had PR open where we were planning to remove it but that didn't merge 😅 https://github.com/AzureAD/microsoft-authentication-cli/pull/82