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] ASP.NET Classic projects fail to build because of MSAL's WinRT reference (Must use PackageReference) #2247

Closed
7 tasks
bengimblett opened this issue Nov 25, 2020 · 22 comments

Comments

@bengimblett
Copy link

bengimblett commented Nov 25, 2020

Logs and Network traces
Without logs or traces, it is unlikely that the team can investigate your issue. Capturing logs and network traces is described at https://github.com/AzureAD/microsoft-authentication-library-for-dotnet/wiki/logging

Which Version of MSAL are you using ?
Microsoft.Identity.Client 4.23

Platform
netfx 4.7.2

What authentication flow has the issue?

  • Desktop / Mobile
    • Interactive
    • Integrated Windows Auth
    • Username Password
    • Device code flow (browserless)
  • Web App
    • Authorization code
    • OBO
  • Daemon App
    • Service to Service calls

Other? - please describe;
Build Error, ASP NET WEB FORMS project, VS2019

Repro
ASPNET Web Forms new template, VS2019
Add latest nuget package Microsoft.Identity.Client 4.23 adds Microsoft.Windows.SDK.Contracts (a WinRT lib?)

Expected behavior
Build succeeds

Actual behavior
1>C:\Users\begim\Documents\source\repos\jotunoidc2\packages\Microsoft.Windows.SDK.Contracts.10.0.17763.1000\build\Microsoft.Windows.SDK.Contracts.targets(4,5): error : Must use PackageReference

@bgavrilMS bgavrilMS changed the title [Bug] [Bug] ASP.NET Classic projects fail to build because of MSAL's WinRT reference Nov 25, 2020
@bgavrilMS
Copy link
Member

bgavrilMS commented Nov 25, 2020

The new dependency is because MSAL is now able to call into WAM (Windows Authentication Manager). This is for public client scenarios only (i.e. rich client and console apps). Details and screenshots here: https://github.com/AzureAD/microsoft-authentication-library-for-dotnet/wiki/wam. For this dependency (Microsoft.Windows.SDK.Contracts) NuGet requires you to use <PackageReference> instead of the old packages.config

This migration is well documented on MSDN: https://docs.microsoft.com/en-us/nuget/consume-packages/migrate-packages-config-to-package-reference, but for a summary read below. Note that migration is safe, any problems would manifest themselves as build time issues, there is no risk of introducing runtime issues.

Projects except ASP.NET

Visual Studio provides migration tool. Right-click on packges.config, and a "Migrate" option is presented:
image

ASP.NET projects

Sadly, for some projects (mainly ASP.NET) this tool fails. This is described here, and several workaround exists.

Workaround 1 - make the tool work https://github.com/NuGet/docs.microsoft.com-nuget/issues/860#issuecomment-409207305
Workaround 2 - migrate manually:

  1. Revert any upgrade attempts (i.e. start clean)
  2. Unload your project (right click, unload)
  3. Edit Project File (right click, Edit Project File)
  4. In the csproj file, add
  <ItemGroup>
    <PackageReference Include="Microsoft.Identity.Client" version="4.23.0" />
  </ItemGroup>
  1. Delete the old entries around Microsoft.Identity.Client from the csproj and packages.config, including

image

and

image

As @dkrasnove points out below, it is recommended that you migrate all your references to PacakgeReference and get rid of packges.config, but this is not strictly necessary

@bgavrilMS bgavrilMS changed the title [Bug] ASP.NET Classic projects fail to build because of MSAL's WinRT reference [Bug] ASP.NET Classic projects fail to build because of MSAL's WinRT reference (Must use PackageReference) Nov 25, 2020
@dkrasnove
Copy link

dkrasnove commented Nov 25, 2020

@bgavrilMS I too am having this issue in an ASP.NET MVC 5 project. I had to upgrade to 4.23.0 to fix the B2C bug. Just to clarify, the only thing I need to do is add that ItemGroup to the project file? Do I need to erase existing references to 4.22.0 from the project file or packages.config?

EDIT: Indeed, this solution seemed to do the trick. I was having an unrelated issue with the newest version of Visual Studio. Thanks!

@bengimblett
Copy link
Author

Thanks @bgavrilMS - to play devils advocate , why the new dependency ? is this to support MSAL better with winRT ? Also i cant find mention in our docs that the you can manually edit the aspnet project to overcome the limitation. The docs suggest a hard limitation. I will look at a PR for that

@bgavrilMS
Copy link
Member

The new dependency is because MSAL is now able to call into WAM (Windows Authentication Manager). This is for public client scenarios only (i.e. rich client and console apps). I have added some details and screenshots here: https://github.com/AzureAD/microsoft-authentication-library-for-dotnet/wiki/wam

The actual dependency will not be loaded into memory for confidential client (website etc. scenarios).

Unfortunately .NET Fwk does not offer a way to separate the dependencies of a DLL based on scenario. If you look carefully, you'll notice that MSAL also depends on WinForms, although you clearly don't use that in your ASP.NET app. We have experimented with splitting Public Client from Confidential Client (i.e. having 2 packages to keep dependencies clean) but this would complicate things quite a lot (2-3 packages to maintain instead of 1, shared code problems etc.). Ultimately, for .NET Core this will not be a problem. We will support WAM on NET5-Windows, i.e. the platform geared at rich client apps, while NET5 will use the fewest number of dependencies.

As for your specific problem - did you try to migrate the MSAL dependency manually to a PackageReference? The docs you pointed at suggest the automated migration tool is broken. I tried to manually migrate an ASP.NET sample and it worked fine. @dkrasnove also confirmed that manual migration it is working for him. Are you getting any error?

@dkrasnove
Copy link

Side note: One mildly annoying quirk of manually adding the PackageReference node to the project file is that it overrides the package manifest in the VS NuGet GUI. If I want to manage packages listed in packages.config via the GUI, I have to first remove any PackageReference nodes from the project file. Otherwise, the GUI will only display PackageReference items and ignore packages.config.

The solution of course is to migrate completely to PackageReference, but unfortunately the automated tool does not yet support ASP.NET projects and it would be quite an effort to do it manually at this point.

@bgavrilMS
Copy link
Member

@bgavrilMS
Copy link
Member

For the current status of migration tool support see NuGet/Home#5877 - many of the actual issues have been resolved.

@bgavrilMS
Copy link
Member

bgavrilMS commented Nov 30, 2020

By the way, here's a PR of how we upgraded one of our samples (ASP.NET) from packages.config to PackageReference using the technique described at https://github.com/NuGet/docs.microsoft.com-nuget/issues/860#issuecomment-409207305

@dkrasnove
Copy link

dkrasnove commented Dec 1, 2020

@bgavrilMS It turns out the hybrid PackageReference/packages.config method breaks our Azure CI pipeline. My guess is that it works locally because the packages are cached, but Azure isn't caching packages and tries to do a fresh nuget restore on every build. Like before, it looks like if there is a PackageReference node in the csproj, NuGet will ignore the packages.config.

Luckily, after a lot of finagling, I got the trick from here to work. It seemed to add the right references, but it caused all cshtml files to suddenly complain about using C# 6 features in C# 5 (which it never did before), and thus wouldn't compile.

Turns out the migration tool deleted some important lines from our Web.config. It also deleted most of our ApplicationInsights.config. After restoring the missing code, it at least seems to work locally now. I'll have to try deploying to Azure tomorrow.

UPDATE: Azure deployment worked!

@bgavrilMS
Copy link
Member

Thanks for updating the thread @dkrasnove , this is not a great experience :(

@ronaldbarendse
Copy link

I'd like to add that migrating to PackageReference isn't always possible, e.g. if you're depending on other packages that use NuGet Install.ps1 scripts. The only thing to do for now is keep the version at 4.22.0 by limiting the allowedVersions in the package.config:

<package id="Microsoft.Identity.Client" version="4.22.0" targetFramework="net472" allowedVersions="(,4.23)" />

@jmprieur
Copy link
Contributor

jmprieur commented Dec 8, 2020

Thanks for your feedback, @ronaldbarendse

@bgavrilMS
Copy link
Member

bgavrilMS commented Dec 8, 2020

Ok, will explore a mechanism for removing this dependency. I think we should ship WAM in a different nuget package, and let users inject it via smth like .WithBroker(true, WamBroker.Instance) or similar.

@skerr4311
Copy link

Thanks a lot for your solution @bgavrilMS this issue has been plaguing me since yesterday.
Workaround 2 - migrate manually: worked for .NET Framework 4.7.2

@b3hdad
Copy link

b3hdad commented Dec 13, 2020

I am having the same issue as well. When I upgrade my Microsoft.Identity.Client from 4.22 to 4.23 or 4.24, I get the error Must Use PackageReference. I have tried all workarounds but they cause other issues. For example the closest I came to fix this was following @bgavrilMS's instruction above. However, even though, that fixes this issue , it seems to break package updates. Basically, after applying this workaround, when I go to Manage nuget packages for solution-->Updates, if there are any new updates for any of the packages, they do not appear under this tab anymore. (However, thanks for sharing solutions that has worked for you).

@bgavrilMS
Copy link
Member

Thank you for all the feedback. Although moving to PackageReference is generally better, I agree that the tooling and the old packages will make this very difficult.

We're going to create a plugin for the new functionality, thus eliminating this dependency form the main Microsoft.Identity.Client

Tracking issue: #2300

@cscuser
Copy link

cscuser commented Dec 24, 2020

@bgavrilMS Thanks Its Working

@henrik-me
Copy link
Contributor

@bgavrilMS bgavrilMS added this to the 4.25.0 milestone Jan 18, 2021
@bgavrilMS
Copy link
Member

The dependency will go away with MSAL 4.25

@pmaytak
Copy link
Contributor

pmaytak commented Jan 20, 2021

This is included in MSAL 4.25.0 release.

cc: @bengimblett @b3hdad @cscuser @skerr4311 @dkrasnove @ronaldbarendse

@pmaytak pmaytak closed this as completed Jan 20, 2021
@hannespreishuber
Copy link

doesnt work for me
mustuse

.25 and .27

@Debro012
Copy link

@hannespreishuber I too faced this issue, but with v4.29.0. I had to remove Microsoft.Windows.SDK.Contracts from the project to get a successful build. However, I still need to deploy and test.

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