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] Microsoft.Identity.Client references System.Windows.Forms #4468

Closed
pedrolupin opened this issue Dec 13, 2023 · 35 comments · Fixed by #4567
Closed

[Bug] Microsoft.Identity.Client references System.Windows.Forms #4468

pedrolupin opened this issue Dec 13, 2023 · 35 comments · Fixed by #4567

Comments

@pedrolupin
Copy link

Library version used

4.56.0

.NET version

.net 7.0.14

Scenario

ManagedIdentityClient - managed identity

Is this a new or an existing app?

The app is in production, and I have upgraded to a new version of MSAL

Issue description and reproduction steps

Azure.Identity 1.9.0 -> Microsoft.Identity.Client 4.49.1 did not have a dependency on System.Windows.Forms, but when updating to newer Azure.Identity 1.10.4 -> Microsoft.Identity.Client 4.56.0 the dependency to Windows.Forms appeared, this is a problem.

Azure.Identity should be able to run headless, on servers and many situations that don't have access to System.Windows.Forms because the .NET desktop runtime is not present.

Relevant code snippets

No response

Expected behavior

No response

Identity provider

Azure B2C Basic Policy

Regression

4.49.1

Solution and workarounds

No response

@pedrolupin pedrolupin added needs attention Delete label after triage untriaged Do not delete. Needed for Automation labels Dec 13, 2023
@bgavrilMS
Copy link
Member

bgavrilMS commented Dec 13, 2023

What's the actual problem @pedrolupin ? The dependency will not be used in headless scenarios.

Also, if you target pure .NET, the dependency is not present. MSAL.NET always had a dependency on Windows.Forms on .NET Fwk (not Core).

@bgavrilMS bgavrilMS added confidential-client question answered and removed untriaged Do not delete. Needed for Automation needs attention Delete label after triage labels Dec 13, 2023
@pedrolupin
Copy link
Author

pedrolupin commented Dec 13, 2023 via email

@pmaytak
Copy link
Contributor

pmaytak commented Dec 13, 2023

Microsoft.Identity.Client adds Windows Forms reference only on net6.0-windows and net462 platforms. So net7.0 app should pick up net6.0 MSAL binary without the Forms. What's the exception that you get?

@pedrolupin
Copy link
Author

pedrolupin commented Dec 14, 2023 via email

@pmaytak pmaytak changed the title [Bug] Microsoft.Identity.Client referecnes System.Windows.Forms [Bug] Microsoft.Identity.Client references System.Windows.Forms Dec 14, 2023
@pmaytak
Copy link
Contributor

pmaytak commented Dec 15, 2023

Just because the target is net7.0-windows that doesn't necessarily imply
that it's a desktop application.

Indeed. This is a known gap. We don't really have a good solution at the moment, but exploring some options. Some of the workarounds are to deploy the app as self-contained or have a post-build script which removes the Windows Forms reference from the runtime config JSON.

@SimonCropp
Copy link
Contributor

@pmaytak that this bit us this week. updated to a the security hotfix patch for sql client nuget. and our web apps would no longer run

@SimonCropp
Copy link
Contributor

given this prevents apps from starting. perhaps it should be a p1?

@bgavrilMS
Copy link
Member

bgavrilMS commented Jan 12, 2024

Thanks @SimonCropp , we will discuss this in the next few days and make a decisions. Bumping the major version of MSAL from version 4 (it's been this way for 4+ years) to version 5 is really big deal and we are not ready for it.

We are evaluating if we can introduce this change in a non-breaking way (well, similar to [Obsolete]-ing some API at least), maybe with some decent msbuild warning.

@SimonCropp
Copy link
Contributor

@bgavrilMS i dont see a way of doing this in a non-breaking way. even if we have the API as Obsolete with warning, it means it still needs to work if people ignore the warning. WindowsFormsWebAuthenticationDialog is exposed as a public type that inherits from System.Windows.Forms.Form. so that means System.Windows.Forms must continue to be referenced

IMO we should bite the bullet and make a breaking change to fix this. it would be relatively low impact in terms of scale for a breaking change. the winforms bit could be moved to a diff nuget and people add a reference to that nuget.

@nblumhardt
Copy link

We were able to work around it in our usage by abandoning net8.0-windows and reorganizing our project to publish for only net8.0 instead. I'm not sure if/where it will bite us further down the track :-) but just post this in case it helps someone else out in the meantime.

@robertmuehsig
Copy link

We encountered the same problem and were also forced to "migrate" from "net6.0-windows" to "net6.0".
Of course, this was only possible because we "muted" this rule in .editorconfig at the same time:
dotnet_diagnostic.CA1416.severity = none

We use a number of Windows-specific APIs (e.g. DirectorySearcher) - that was also the purpose of the net6.0-windows TargetFramework.
Of course, we don't know if any other NuGet package might have a different behavior because we are no longer following the Windows-specific case (I'm not sure what "switches" you have in NuGet packages to create a different behavior based on the target systems)

The whole problem is quite complex. We had to update Azure.Identity because the older version contained a security vulnerability and Visual Studio "forced" the update on us (which is ok). Since all runtimes are installed on our dev environments, we were a bit surprised that the application did not run on Azure - especially with such a "minor update", we would not have assumed a breaking change.

We use Azure.Identity for DataProtection, which is even recommended as best practice by Microsoft.

Basically, this change made the "-windows" TargetFrameworks unusable and I'm surprised that this didn't make bigger waves.
We are a small development team and can, for example, easily change rules in .editorconfig or customize the TargetFrameworks.

The idea that -windows means it runs on the desktop is also simply a complete misinterpretation.

The fact that Visual Studio now encourages everyone to update Azure.Identity and thus also includes this dependency here will lead to a flood of these bugs.

Sorry for the long comment, but this bugged me.

@bjornbouetsmith
Copy link

We were able to work around it in our usage by abandoning net8.0-windows and reorganizing our project to publish for only net8.0 instead. I'm not sure if/where it will bite us further down the track :-) but just post this in case it helps someone else out in the meantime.

Thats a poor solution in my opinion - also the work around we made made ourselves - having a post-build script that removes the Desktop reference from the runtime config.

Whoever added Windows.Forms as a dependency to this library must have red ears - and in my opinion that change should be either rolled back, or a new version released immediately with this dependency removed.

@SimonCropp
Copy link
Contributor

SimonCropp commented Jan 19, 2024

i also want winforms (and related dlls/pdbs) gone so they dont bloat my app deployments

@bgavrilMS bgavrilMS self-assigned this Feb 5, 2024
@nicolascotton
Copy link

I am using Microsoft.Identity.Client in my Windows services, but they are now failing to start with the .NET runtime. Installing the desktop runtime is not an acceptable solution for me. Would it be possible to have a solution that is not reliant on these unnecessary components for my services?

@bgavrilMS
Copy link
Member

Yes, we will drop support for net6-windows from MSAL. The extra logic lives in MSAL.Desktop. I do not have an ETA for this, but hope we'll get it done in a few weeks.

@rarowston
Copy link

@bgavrilMS Any updates/ETAs on this one?
We're using workarounds for it for the time being, but it would be good to get things working fully again.

@alekdavis
Copy link

We just ran into this issue with an ASP.NET Core REST API app built for net8.0-windows. We need the -windows OS only because the app uses System.DirectoryServices (which would not build just for net8.0). So moving to a non-OS-specific build is not an option. What are the other workarounds now?

@rarowston
Copy link

We just ran into this issue with an ASP.NET Core REST API app built for net8.0-windows. We need the -windows OS only because the app uses System.DirectoryServices (which would not build just for net8.0). So moving to a non-OS-specific build is not an option. What are the other workarounds now?

Others might be able to chime in with other options, or clarification on how to do number 4, but there are 4 options that I have seen mentioned to work around this issue.

  1. Remove the -windows option in the target framework (This is the option I am using as it is the simplest). I'm not sure what the issue you are having with this as I am also using DirectoryServices and it builds 'fine' after I suppress all the warnings that this causes.
  2. Deploy the runtime to the target server. It is a bit odd to deploy the desktop runtime to a web server - but that's why we call it a workaround.
  3. Downgrade the version of this package before the issue was introduced, though this re-introduces a potential security vulnerability - so I don't like this option much.
  4. There has been mention above of using a post-build script to manually remove the dependency from the runtime config file though I am not sure how well this will work in practice (i.e. if it will ever attempt to access functions from this runtime in your usage) [Bug] Microsoft.Identity.Client references System.Windows.Forms #4468 (comment)

@SimonCropp
Copy link
Contributor

@alekdavis @rarowston

Remove the -windows option in the target framework (This is the option I am using as it is the simplest). I'm not sure what the issue you are having with this as I am also using DirectoryServices and it builds 'fine' after I suppress all the warnings that this causes.

thats the workaround we went with. had to suppress some warnings about "please use -windows". but at runtime it works

@alekdavis
Copy link

@rarowston @SimonCropp Ah, thank you soooo much. I did not realize that this was just a warning that can be safely ignored, so I disabled it, switched OS platform from Windows to (none) and it worked. In case anyone wonders, the easies way to disable the warning was via this line in the .editorconfig file:

dotnet_diagnostic.CA1416.severity = none

@SimonCropp
Copy link
Contributor

@alekdavis yeah the warning can be interpreted as "if you deploy this to non windows, it wont work"

@rclabo
Copy link

rclabo commented Apr 17, 2024

I just got bit by this. Sucks. Any update on the status of the fix?

@pedrolupin
Copy link
Author

pedrolupin commented Apr 17, 2024 via email

@rclabo
Copy link

rclabo commented Apr 17, 2024

There has been mention above of using a post-build script to manually remove the dependency from the runtime config file though I am not sure how well this will work in practice (i.e. if it will ever attempt to access functions from this runtime in your usage) #4468 (comment)

It's been mentioned that one workaround is to remove the "Microsoft.WindowsDesktop.App" dependency from the runtimeconfig.json file. However this approach did not work for us.

Changing the file from

{
  "runtimeOptions": {
    "tfm": "net8.0",
    "frameworks": [
      {
        "name": "Microsoft.NETCore.App",
        "version": "8.0.0"
      },
      {
        "name": "Microsoft.WindowsDesktop.App",
        "version": "8.0.0"
      },
      {
        "name": "Microsoft.AspNetCore.App",
        "version": "8.0.0"
      }
    ],
    "configProperties": {
      "System.GC.Server": false,
      "System.Runtime.Serialization.EnableUnsafeBinaryFormatterSerialization": false
    }
  }
}

to

{
  "runtimeOptions": {
    "tfm": "net8.0",
    "frameworks": [
      {
        "name": "Microsoft.NETCore.App",
        "version": "8.0.0"
      },
      {
        "name": "Microsoft.AspNetCore.App",
        "version": "8.0.0"
      }
    ],
    "configProperties": {
      "System.GC.Server": false,
      "System.Runtime.Serialization.EnableUnsafeBinaryFormatterSerialization": false
    }
  }
}

for us did help to allow the website to start up but as soon as the code attempted to use Microsoft.Data.SqlClient the SqlClient throws with the following exception:

 ---> System.TypeInitializationException: The type initializer for 'Microsoft.Data.SqlClient.SqlAuthenticationProviderManager' threw an exception.
 ---> System.IO.FileNotFoundException: Could not load file or assembly 'System.Configuration.ConfigurationManager, Version=8.0.0.0, Culture=neutral, PublicKeyToken=cc7b13ffcd2ddd51'. The system cannot find the file specified.
File name: 'System.Configuration.ConfigurationManager, Version=8.0.0.0, Culture=neutral, PublicKeyToken=cc7b13ffcd2ddd51'
   at Microsoft.Data.SqlClient.SqlAuthenticationProviderManager..cctor()
   --- End of inner exception stack trace ---
   at Microsoft.Data.ProviderBase.DbConnectionPool.TryGetConnection(DbConnection owningObject, UInt32 waitForMultipleObjectsTimeout, Boolean allowCreate, Boolean onlyOneCheckConnection, DbConnectionOptions userOptions, DbConnectionInternal& connection)
   at Microsoft.Data.ProviderBase.DbConnectionPool.TryGetConnection(DbConnection owningObject, TaskCompletionSource`1 retry, DbConnectionOptions userOptions, DbConnectionInternal& connection)
   at Microsoft.Data.ProviderBase.DbConnectionFactory.TryGetConnection(DbConnection owningConnection, TaskCompletionSource`1 retry, DbConnectionOptions userOptions, DbConnectionInternal oldConnection, DbConnectionInternal& connection)
   at Microsoft.Data.ProviderBase.DbConnectionInternal.TryOpenConnectionInternal(DbConnection outerConnection, DbConnectionFactory connectionFactory, TaskCompletionSource`1 retry, DbConnectionOptions userOptions)
   at Microsoft.Data.ProviderBase.DbConnectionClosed.TryOpenConnection(DbConnection outerConnection, DbConnectionFactory connectionFactory, TaskCompletionSource`1 retry, DbConnectionOptions userOptions)
   at Microsoft.Data.SqlClient.SqlConnection.TryOpen(TaskCompletionSource`1 retry, SqlConnectionOverrides overrides)
   at Microsoft.Data.SqlClient.SqlConnection.Open(SqlConnectionOverrides overrides)
   at Microsoft.Data.SqlClient.SqlConnection.Open()
   at App.Sql.SqlTube.OpenConnection(SqlCommand command, Boolean makeMultipleTries) in C:\Users\RonC\source\repos\wwwGiftOasis3\MP_Prims\Sql\SqlTube.cs:line 332

Has anyone managed to make that workaround actually work?

@localden
Copy link
Collaborator

Hey folks - we're currently working on mitigating this. @bgavrilMS has been working on this in #4567

@rclabo
Copy link

rclabo commented Apr 17, 2024

@localden - Your work on this is very important to the community. Thanks you for the update.

I did manage to work around the issue by doing a self contained deployment of our website so at least we are up and running again. We look forward to getting back to non-self-contained deployments as usual once the bug is fixed.

Again, much appreciate the update and the fact that this is actively being worked on.

@pedrolupin
Copy link
Author

Thanks Bogdan looking forward to the release :)

@bgavrilMS
Copy link
Member

The team aims to release around the end of April.

@gautamdsheth
Copy link

@bgavrilMS - does this mean that the with 4.61.0 release, it will also work with net6.0-windows ? Or do we need to change anything in the application targeting that ? Sorry, didn't get it from the above thread.

@pmaytak
Copy link
Contributor

pmaytak commented May 10, 2024

Hello. We've released MSAL 4.61.0 which removes net6.0-windows binary (Windows Forms dependency).

For confidential client apps targeting net6.0-windows, there are no migration steps. You should be able to upgrade MSAL and remove the workarounds. The app will pick up net6.0 MSAL binary.

For public client apps targeting net6.0-windows, reference Microsoft.Identity.Client.Broker when using interactive authentication with Windows Broker and call WithBroker(BrokerOptions); or reference Microsoft.Identity.Client.Desktop when authenticating with browser and call WithWindowsEmbeddedBrowserSupport(). There are no changes to the usage of the system browser.

@pedrolupin @SimonCropp @nblumhardt @robertmuehsig @bjornbouetsmith @nicolascotton @alekdavis @rarowston @rclabo @gautamdsheth @Kavya-Gogula @ayrton97

albertospelta added a commit to sql-bi/Bravo that referenced this issue Sep 19, 2024
This update addresses a breaking change in the `Microsoft.Identity.Client` library, which has removed support for the `net6.0-windows7.0` binary. To maintain compatibility for desktop applications targeting `net6.0-windows`, the `Microsoft.Identity.Client.Desktop` dependency has been added. This change ensures that authentication using a browser, with the method `WithWindowsEmbeddedBrowserSupport()`, continues to work seamlessly. See AzureAD/microsoft-authentication-library-for-dotnet#4468

Additionally, this update allows us to remove the dependency on the `Microsoft.Windows.SDK` framework, which has resulted in a reduction of installed binary sizes by approximately 20%.
albertospelta added a commit to sql-bi/Bravo that referenced this issue Sep 19, 2024
This update addresses a breaking change in the `Microsoft.Identity.Client` library, which has removed support for the `net6.0-windows7.0` binary. To maintain compatibility for desktop applications targeting `net6.0-windows`, the `Microsoft.Identity.Client.Desktop` dependency has been added. This change ensures that authentication using a browser, with the method `WithWindowsEmbeddedBrowserSupport()`, continues to work seamlessly. See AzureAD/microsoft-authentication-library-for-dotnet#4468

Additionally, this update allows us to remove the dependency on the `Microsoft.Windows.SDK` framework, which has resulted in a reduction of installed binary sizes by approximately 20%.
albertospelta added a commit to sql-bi/Bravo that referenced this issue Sep 19, 2024
…819)

This update addresses a breaking change in the `Microsoft.Identity.Client` library, which has removed support for the `net6.0-windows7.0` binary. To maintain compatibility for desktop applications targeting `net6.0-windows`, the `Microsoft.Identity.Client.Desktop` dependency has been added. This change ensures that authentication using a browser, with the method `WithWindowsEmbeddedBrowserSupport()`, continues to work seamlessly. See AzureAD/microsoft-authentication-library-for-dotnet#4468

Additionally, this update allows us to remove the dependency on the `Microsoft.Windows.SDK` framework, which has resulted in a reduction of installed binary sizes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.