search

AzureAD / azure-activedirectory-identitymodel-extensions-for-dotnet

IdentityModel extensions for .Net
MIT License
1.05k stars 396 forks source link

[Feature Request] Warn when different versions of the IdentityModel NuGet packages are used #2513

Open reduckted opened 6 months ago

reduckted commented 6 months ago

Is your feature request related to a problem? Please describe.

Related to #2506

The wiki says:

All the IdentityModel libraries must have the same version 7.0.0 in your project and including the recursive dependencies.

However, that message is hidden away in the wiki where not everyone will see it. Even if that message were to be moved into the readme file, I still do not think it's enough because not having the same version of the libraries can result in code that silently fails, as demonstrated in #2506.

Having a build-time warning would prevent this mistake from occurring.

Describe the solution you'd like

Each NuGet package for the Microsoft.IdentityModel.* and System.IdentityModel.* libraries would contain an MSBuild task that checks the version of all IdentityModel libraries that are referenced by the project. If there is more than one unique version in use, a warning will be logged.

For example, given these package references:

<PackageReference Include="Microsoft.AspNetCore.Authentication.JwtBearer" Version="8.0.2" />
<PackageReference Include="Microsoft.IdentityModel.Tokens" Version="7.4.0" />

A warning like this would be produced:

All of the IdentityModel libraries must have the same version in your project, including transitive dependencies. The versions in use are:
 - Microsoft.IdentityModel.Abstractions 7.4.0
 - Microsoft.IdentityModel.JsonWebTokens 7.4.0
 - Microsoft.IdentityModel.Logging 7.4.0
 - Microsoft.IdentityModel.Protocols 7.1.2
 - Microsoft.IdentityModel.Protocols.OpenIdConnect 7.1.2
 - Microsoft.IdentityModel.Tokens 7.4.0
 - System.IdentityModel.Tokens.Jwt 7.4.0

This warning explains what the problem is, and clearly identifies the versions of the transitive references, making it easy to understand what needs to be changed.

Describe alternatives you've considered

None.

Additional context

I have a working prototype for this and would be happy to contribute if this is a desired feature.

keegan-caruso commented 6 months ago

Hello, thanks for raising this issue.

I think I got a solution working where the nuget packages would have explicit version requirements. = 7.4.0 instead of >=

Would that meet your expectations here?

Edit.

This wouldn't help until asp.net core took a package with the new version restrictions.

jennyf19 commented 5 months ago

duplicate of #1794

etiennelepagel commented 4 months ago

Do you have any ETA on this? We are severly impacted by this problem.

halter73 commented 4 months ago

Errors caused by combining newer Microsoft.IdentityModel.Tokens package versions combined with older Microsoft.IdentityModel.Protocols.OpenIdConnect package versions have led to several bug reports in the aspnetcore repo.

While warning is better than nothing, we shouldn't be making breaking changes to packages like Microsoft.IdentityModel.Tokens. If behavior needs to change, we should be adding new APIs rather than breaking old ones.

I know every bug fix is technically a breaking change if you squint, but what's going on here is far beyond a simple bug fix. Microsoft.IdentityModel.Protocols.OpenIdConnect package versions that worked fine with older Microsoft.IdentityModel.Tokens versions now silently fail to read half the properties from .well-known/openid-configuration leading to inscrutable errors later on.

I think rather than producing warning or adding explicit max version requirements to packages (while better than nothing), we should revert the changes in https://github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/pull/2491 so older Microsoft.IdentityModel.Protocols.OpenIdConnect packages continue to work with newer Microsoft.IdentityModel.Tokens packages.

If we need a new, incompatible behavior for JsonSerializerPrimitives, we should add a JsonSerializerPrimitives2. It's hidden anyway since it's exposed by InternalsVisibleTo. We shouldn't break APIs exposed via InternalsVisiableTo just like we shouldn't break public APIs because of exactly what's going on here.

@brentschmaltz @jennyf19 @keegan-caruso

brentschmaltz commented 2 months ago

@halter73 @etiennelepagel the way we are planning on handling this is to ONLY change internals on major releases. . (dot) releases will not modify internals.

Our 8.x release will require all assemblies to be 8.x.

ItWorksOnMyMachine commented 2 months ago

@halter73 @etiennelepagel the way we are planning on handling this is to ONLY change internals on major releases. . (dot) releases will not modify internals.

Our 8.x release will require all assemblies to be 8.x.

I'm confused as to how to solve this. I have recently upgraded to .NET 8.0 and also upgraded the Microsoft.IdentityModel and System.IdentityModel packages. All of those packages are at 7.6.0 and I'm still experiencing this issue. Here are all of my packages.

<PackageReference Include="AWSSDK.AWSMarketplaceMetering" Version="3.7.300.103" />
<PackageReference Include="AWSSDK.MarketplaceEntitlementService" Version="3.7.302.20" />
<PackageReference Include="AWSSDK.RDS" Version="3.7.313.11" />
<PackageReference Include="AWSSDK.SSO" Version="3.7.300.103" />
<PackageReference Include="AWSSDK.SSOOIDC" Version="3.7.302.14" />
<PackageReference Include="Braintree" Version="5.25.0" />
<PackageReference Include="CustomerApi" Version="1.0.0-CI-20231122-210852" />
<PackageReference Include="Duende.IdentityServer.AspNetIdentity" Version="7.0.5" />
<PackageReference Include="EntityFramework6.Npgsql" Version="6.4.3" />
<PackageReference Include="EventApi" Version="1.0.0-CI-20231115-172843" />
<PackageReference Include="FluentValidation.AspNetCore" Version="11.3.0" />
<PackageReference Include="HtmlSanitizer" Version="8.0.865" />
<PackageReference Include="Humanizer.Core" Version="2.14.1" />
<PackageReference Include="IdentityModel" Version="7.0.0" />
<PackageReference Include="MediaTypeMap.Core" Version="2.3.3" />
<PackageReference Include="Microsoft.AspNetCore.Authentication.JwtBearer" Version="8.0.6" />
<PackageReference Include="Microsoft.AspNetCore.DataProtection.Extensions" Version="8.0.6" />
<PackageReference Include="Microsoft.AspNetCore.Mvc.NewtonsoftJson" Version="8.0.6" />
<PackageReference Include="Microsoft.AspNetCore.SignalR.Protocols.NewtonsoftJson" Version="8.0.6" />
<PackageReference Include="Microsoft.AspNetCore.SpaServices" Version="3.1.32" />
<PackageReference Include="Microsoft.CodeAnalysis.Common" Version="4.10.0" />
<PackageReference Include="Microsoft.CodeAnalysis.CSharp" Version="4.10.0" />
<PackageReference Include="Microsoft.CodeAnalysis.CSharp.Features" Version="4.10.0" />
<PackageReference Include="Microsoft.CodeAnalysis.CSharp.Scripting" Version="4.10.0" />
<PackageReference Include="Microsoft.CodeAnalysis.CSharp.Workspaces" Version="4.10.0" />
<PackageReference Include="Microsoft.CodeAnalysis.Features" Version="4.10.0" />
<PackageReference Include="Microsoft.CodeAnalysis.Scripting.Common" Version="4.10.0" />
<PackageReference Include="Microsoft.CodeAnalysis.VisualBasic" Version="4.10.0" />
<PackageReference Include="Microsoft.CodeAnalysis.VisualBasic.Features" Version="4.10.0" />
<PackageReference Include="Microsoft.CodeAnalysis.VisualBasic.Workspaces" Version="4.10.0" />
<PackageReference Include="Microsoft.CodeAnalysis.Workspaces.Common" Version="4.10.0" />
<PackageReference Include="Microsoft.CodeAnalysis.Workspaces.MSBuild" Version="4.10.0" />
<PackageReference Include="Microsoft.Extensions.Caching.Abstractions" Version="8.0.0" />
<PackageReference Include="Microsoft.Extensions.Configuration.CommandLine" Version="8.0.0" />
<PackageReference Include="Microsoft.Extensions.Http" Version="8.0.0" />
<PackageReference Include="Microsoft.Extensions.Logging.Debug" Version="8.0.0" />
<PackageReference Include="Microsoft.IdentityModel.Tokens" Version="7.6.0" />
<PackageReference Include="Microsoft.TypeScript.MSBuild" Version="5.4.4">
  <PrivateAssets>all</PrivateAssets>
  <IncludeAssets>runtime; build; native; contentfiles; analyzers; buildtransitive</IncludeAssets>
</PackageReference>
<PackageReference Include="Serilog.AspNetCore" Version="8.0.1" />
<PackageReference Include="Serilog.Enrichers.Environment" Version="3.0.0" />
<PackageReference Include="Serilog.Enrichers.Sensitive" Version="1.7.3" />
<PackageReference Include="Serilog.Sinks.Datadog.Logs" Version="0.5.2" />
<PackageReference Include="System.IO.FileSystem.Watcher" Version="4.3.0" />
<PackageReference Include="System.Net.Http" Version="4.3.4" />
<PackageReference Include="System.Reactive.Linq" Version="6.0.1" />
<PackageReference Include="System.Xml.XmlSerializer" Version="4.3.0" />
<PackageReference Include="Microsoft.VisualStudio.Web.CodeGeneration.Design" Version="8.0.2" />
<PackageReference Include="Mono.TextTemplating" Version="2.3.1" />
<PackageReference Include="Mono.TextTemplating.Roslyn" Version="2.3.1" />
<PackageReference Include="Newtonsoft.Json" Version="13.0.3" />
<PackageReference Include="SharpZipLib" Version="1.4.2" />
<PackageReference Include="Swashbuckle.AspNetCore" Version="6.6.2" />
<PackageReference Include="Swashbuckle.AspNetCore.Annotations" Version="6.6.2" />
<PackageReference Include="System.IdentityModel.Tokens.Jwt" Version="7.6.0" />
<PackageReference Include="Venminder.CQRS" Version="1.2.1" />
brentschmaltz commented 1 month ago

@ItWorksOnMyMachine the issue is we were not careful enough about moving around internals in minor releases. We have established the contract that we will not move internals in minor releases.

What other versions of IdentityModel are being pulled in? Are they all the same version?