Skip to content

Conversation

@FehintolaObafemi
Copy link
Contributor

@FehintolaObafemi FehintolaObafemi commented Jun 24, 2022

Fixes #1133

Changes proposed in this pull request

Other links

@peombwa
Copy link
Member

peombwa commented Jun 24, 2022

Since we are using MSAL 4.44.X in this implementation, you may want to wait for Azure.Identity.BrokeredAuthentication to provide us with an implementation that uses the new .WithBrokerPreview(). See https://github.com/AzureAD/microsoft-authentication-library-for-dotnet/wiki/wam#new-wam-preview-in-msal-444 and Azure/Identity's implementation.

Otherwise, we you'll also need to ensure the prerequisites in https://docs.microsoft.com/en-us/azure/active-directory/develop/scenario-desktop-acquire-token-wam#availability are met for it to work across multiple targets.
You can find PowerShell's .NET runtime version using [System.Runtime.InteropServices.RuntimeInformation]::FrameworkDescription. For example, PowerShell 5.1 runs on .NET Framework 4.8.X while PowerShell 7.2.5. runs on .NET 6.0.6.

cc\ @FehintolaObafemi

@FehintolaObafemi FehintolaObafemi requested a review from peombwa June 27, 2022 16:08
@adamedx
Copy link

adamedx commented Jun 28, 2022

Fixes #1133

Changes proposed in this pull request

Other links

One question @peombwa -- do we need to make this behaavior opt-in via Connect-MgGraph? I spoke to one of the implementers of WAM in Windows and I'm not clear on which Windows versions support WAM and also if there are any corner cases we'd like to provide a way to avoid.

@peombwa
Copy link
Member

peombwa commented Jun 29, 2022

One question @peombwa -- do we need to make this behaavior opt-in via Connect-MgGraph? I spoke to one of the implementers of WAM in Windows and I'm not clear on which Windows versions support WAM and also if there are any corner cases we'd like to provide a way to avoid.

@adamedx, good point! We should make this an opt-in feature since WAM has couple of limitations that customers may have a hard time with:

New accounts are automatically remembered by Windows. Work and School have the option of joining the organization's directory or opting out completely, in which case the account won't appear under "Email & Accounts". Microsoft accounts are automatically added to Windows.

See https://docs.microsoft.com/en-us/azure/active-directory/develop/scenario-desktop-acquire-token-wam#other-considerations.

MSAL's implementation is OS aware, and should fallback to a browser when WAM is not supported. See https://docs.microsoft.com/en-us/azure/active-directory/develop/scenario-desktop-acquire-token-wam#wam-limitations.

@peombwa
Copy link
Member

peombwa commented Jun 29, 2022

Since we are using MSAL 4.44.X in this implementation, you may want to wait for Azure.Identity.BrokeredAuthentication to provide us with an implementation that uses the new .WithBrokerPreview(). See https://github.com/AzureAD/microsoft-authentication-library-for-dotnet/wiki/wam#new-wam-preview-in-msal-444 and Azure/Identity's implementation.

Otherwise, we you'll also need to ensure the prerequisites in https://docs.microsoft.com/en-us/azure/active-directory/develop/scenario-desktop-acquire-token-wam#availability are met for it to work across multiple targets. You can find PowerShell's .NET runtime version using [System.Runtime.InteropServices.RuntimeInformation]::FrameworkDescription. For example, PowerShell 5.1 runs on .NET Framework 4.8.X while PowerShell 7.2.5. runs on .NET 6.0.6.

@FehintolaObafemi, I'm getting the following errors when I run Connect-MgGraph in PowerShell 7.2.5:

➜ .\tools\GenerateAuthenticationModule.ps1 -Build -Run
➜ Connect-MgGraph
Connect-MgGraph: InteractiveBrowserCredential authentication failed: If you have a Windows application which targets net5 or net5-windows, please change the target to net5-windows10.0.17763.0. 
Your app can still run on earlier versions of Windows such as Win7 if you add <SupportedOSPlatformVersion>7</SupportedOSPlatformVersion> in the csproj.
 The broker (WAM) is available only on Win10 and this library will fallback to a browser on older systems. 
If you have a NET5 cross-platform (Windows, Mac, Linux) application, please dual target net5 and net5-windows10.0.17763.0. Your installer should deploy the net5 version on Mac and Linux and the net5-window10.0.17763.0 on Windows.
If you have a .NET Core 3.1 application, please install the NuGet package named Microsoft.Identity.Client.Desktop and call the extension method .WithWindowsBroker() first. 
If you want to try the new broker preview, please install the NuGet package named Microsoft.Identity.Client.Broker and call the extension method .WithBrokerPreview(). 
For details see https://aka.ms/msal-net-wam and https://github.com/dotnet/designs/blob/main/accepted/2020/platform-checks/platform-checks.md 

@FehintolaObafemi
Copy link
Contributor Author

Since we are using MSAL 4.44.X in this implementation, you may want to wait for Azure.Identity.BrokeredAuthentication to provide us with an implementation that uses the new .WithBrokerPreview(). See https://github.com/AzureAD/microsoft-authentication-library-for-dotnet/wiki/wam#new-wam-preview-in-msal-444 and Azure/Identity's implementation.
Otherwise, we you'll also need to ensure the prerequisites in https://docs.microsoft.com/en-us/azure/active-directory/develop/scenario-desktop-acquire-token-wam#availability are met for it to work across multiple targets. You can find PowerShell's .NET runtime version using [System.Runtime.InteropServices.RuntimeInformation]::FrameworkDescription. For example, PowerShell 5.1 runs on .NET Framework 4.8.X while PowerShell 7.2.5. runs on .NET 6.0.6.

@FehintolaObafemi, I'm getting the following errors when I run Connect-MgGraph in PowerShell 7.2.5:

➜ .\tools\GenerateAuthenticationModule.ps1 -Build -Run
➜ Connect-MgGraph
Connect-MgGraph: InteractiveBrowserCredential authentication failed: If you have a Windows application which targets net5 or net5-windows, please change the target to net5-windows10.0.17763.0. 
Your app can still run on earlier versions of Windows such as Win7 if you add <SupportedOSPlatformVersion>7</SupportedOSPlatformVersion> in the csproj.
 The broker (WAM) is available only on Win10 and this library will fallback to a browser on older systems. 
If you have a NET5 cross-platform (Windows, Mac, Linux) application, please dual target net5 and net5-windows10.0.17763.0. Your installer should deploy the net5 version on Mac and Linux and the net5-window10.0.17763.0 on Windows.
If you have a .NET Core 3.1 application, please install the NuGet package named Microsoft.Identity.Client.Desktop and call the extension method .WithWindowsBroker() first. 
If you want to try the new broker preview, please install the NuGet package named Microsoft.Identity.Client.Broker and call the extension method .WithBrokerPreview(). 
For details see https://aka.ms/msal-net-wam and https://github.com/dotnet/designs/blob/main/accepted/2020/platform-checks/platform-checks.md 

@peombwa I can't seem to recreate this error on my end following the step you highlighted above.

@peombwa
Copy link
Member

peombwa commented Jun 30, 2022

@FehintolaObafemi, are you getting a WAM account picker pop up when you sign in? If not, then you are using an access token from the cache and not going through WAM. Run Disconnect-MgGraph after Connect-MgGraph then try again using the steps I provided above.

The issue is also present in PowerShell 5.1 as of commit #1fde021649fffc4fbbfcda886485c5a1a13bd23b:

Connect-MgGraph : InteractiveBrowserCredential authentication failed: The Windows broker is not directly available on MSAL for .NET
Framework To use it, please install the NuGet package named Microsoft.Identity.Client.Desktop and call the extension method
.WithWindowsBroker() first.If you want to try the new broker preview, please install the NuGet package named
Microsoft.Identity.Client.Broker and call the extension method .WithBrokerPreview().
At line:1 char:1
+ Connect-MgGraph
+ ~~~~~~~~~~~~~~~
    + CategoryInfo          : NotSpecified: (:) [Connect-MgGraph], AuthenticationFailedException
    + FullyQualifiedErrorId : Microsoft.Graph.PowerShell.Authentication.Cmdlets.ConnectMgGraph

We can sync offline for the repro steps.

@adamedx
Copy link

adamedx commented Jul 5, 2022

One question @peombwa -- do we need to make this behaavior opt-in via Connect-MgGraph? I spoke to one of the implementers of WAM in Windows and I'm not clear on which Windows versions support WAM and also if there are any corner cases we'd like to provide a way to avoid.

@adamedx, good point! We should make this an opt-in feature since WAM has couple of limitations that customers may have a hard time with:

New accounts are automatically remembered by Windows. Work and School have the option of joining the organization's directory or opting out completely, in which case the account won't appear under "Email & Accounts". Microsoft accounts are automatically added to Windows.

See https://docs.microsoft.com/en-us/azure/active-directory/develop/scenario-desktop-acquire-token-wam#other-considerations.

MSAL's implementation is OS aware, and should fallback to a browser when WAM is not supported. See https://docs.microsoft.com/en-us/azure/active-directory/develop/scenario-desktop-acquire-token-wam#wam-limitations.

@peombwa , @maisarissi, suggestion for how to expose it? Should it be a preference variable, or just a new option, e.g. SigninUi with the options of Auto, Browser, and Native, where Browser is the current mechanism (web browser), Native is WAM (not embedded web view, though we could have that option as well), and Auto is whatever we decide we want to default to, initially Browser to maintain current behavior.

@peombwa
Copy link
Member

peombwa commented Jul 5, 2022

@peombwa , @maisarissi, suggestion for how to expose it? Should it be a preference variable, or just a new option, e.g. SigninUi with the options of Auto, Browser, and Native, where Browser is the current mechanism (web browser), Native is WAM (not embedded web view, though we could have that option as well), and Auto is whatever we decide we want to default to, initially Browser to maintain current behavior.

@adamedx, I like the idea of having a -SigninUi parameter. We can work with just Browser and Native as the parameter values by making -SigninUi parameter optional with a default value of Browser. @maisarissi, thoughts?

@FehintolaObafemi, let's make -SigninUi an optional parameter on Connect-MgGraph with:

  • Browser and Native as the enum values.
  • UserParameterSet as the parameterSetName
  • GraphSession object can be used to hold the value of -SigninUi for the lifetime of the current process.

See -ContextScope example as a reference.

@maisarissi
Copy link

@adamedx, I like the idea of having a -SigninUi parameter. We can work with just Browser and Native as the parameter values by making -SigninUi parameter optional with a default value of Browser. @maisarissi, thoughts?

Agreed! I like this idea.

@adamedx
Copy link

adamedx commented Aug 17, 2022

@FehintolaObafemi , once everything looks good, would be great to rebase this into a smaller set of commits to simplify the change log.

@FehintolaObafemi FehintolaObafemi force-pushed the fehintolaobafemi/WAM-integration branch 2 times, most recently from 1e78b82 to def70ed Compare August 18, 2022 17:09
@FehintolaObafemi FehintolaObafemi force-pushed the fehintolaobafemi/WAM-integration branch 2 times, most recently from 7e78289 to b5c0598 Compare August 31, 2022 16:42
@FehintolaObafemi FehintolaObafemi force-pushed the fehintolaobafemi/WAM-integration branch from 24cffe1 to 56464e8 Compare October 12, 2022 12:22
@peombwa
Copy link
Member

peombwa commented Oct 12, 2022

@FehintolaObafemi, I've resolved the msalRuntime.dll not found exception in 56464e8. Please update the PR with the suggestions I've made in the comments above for us to approve the PR.

@FehintolaObafemi
Copy link
Contributor Author

Azure/azure-sdk-for-net#31844

Issue has been filed with the Azure Identity team

@FehintolaObafemi FehintolaObafemi force-pushed the fehintolaobafemi/WAM-integration branch 3 times, most recently from c59a506 to 50f6484 Compare October 31, 2022 15:15
@FehintolaObafemi FehintolaObafemi force-pushed the fehintolaobafemi/WAM-integration branch from 2e0b06b to 4a91429 Compare February 6, 2023 15:09
@FehintolaObafemi FehintolaObafemi force-pushed the fehintolaobafemi/WAM-integration branch from 4a91429 to e6197fd Compare February 22, 2023 16:06
@FehintolaObafemi FehintolaObafemi force-pushed the fehintolaobafemi/WAM-integration branch 2 times, most recently from 75cc8b9 to 53066fd Compare March 13, 2023 15:34
@FehintolaObafemi FehintolaObafemi force-pushed the fehintolaobafemi/WAM-integration branch from 53066fd to 616ef71 Compare March 15, 2023 14:40
@FehintolaObafemi FehintolaObafemi force-pushed the fehintolaobafemi/WAM-integration branch 2 times, most recently from 7ebe788 to 66c90df Compare March 27, 2023 15:07
@FehintolaObafemi FehintolaObafemi force-pushed the fehintolaobafemi/WAM-integration branch 2 times, most recently from 8cbf0c0 to e5719c9 Compare April 6, 2023 14:19
@FehintolaObafemi FehintolaObafemi force-pushed the fehintolaobafemi/WAM-integration branch 5 times, most recently from 4557f1c to 06348fd Compare April 21, 2023 16:25
@FehintolaObafemi FehintolaObafemi force-pushed the fehintolaobafemi/WAM-integration branch 2 times, most recently from 4e66090 to cb9f2a6 Compare May 3, 2023 14:35
@FehintolaObafemi FehintolaObafemi force-pushed the fehintolaobafemi/WAM-integration branch 4 times, most recently from da6576c to 7e0372e Compare May 8, 2023 15:47
@FehintolaObafemi FehintolaObafemi force-pushed the fehintolaobafemi/WAM-integration branch from 7e0372e to 10d1ac1 Compare May 15, 2023 15:09
@FehintolaObafemi FehintolaObafemi force-pushed the fehintolaobafemi/WAM-integration branch 4 times, most recently from 2d7c97b to 7a18adc Compare May 24, 2023 15:12
@FehintolaObafemi FehintolaObafemi force-pushed the fehintolaobafemi/WAM-integration branch from 7a18adc to 7329697 Compare May 24, 2023 15:14
@FehintolaObafemi
Copy link
Contributor Author

Continued in #2034 due to this PR being closed on accident and not re-opening.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants