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.

Sign up for GitHub

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

Jump to bottom

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

Open
reduckted opened this issue Mar 2, 2024 · 8 comments
Open

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

reduckted opened this issue Mar 2, 2024 · 8 comments
Assignees
@keegan-caruso
Labels
Dependency Mismatch Transitive dependency might be at play and create issues resulting in incorrect versions of a class Enhancement The issue is a new feature IdentityModel8x Future breaking issues/features for IdentityModel 8x P2 High, but not urgent. Needs to be addressed within the next couple of sprints
Milestone
7.7.2

Comments

@reduckted
Copy link

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.
@jennyf19 jennyf19 added the Enhancement The issue is a new feature label Mar 5, 2024
@keegan-caruso
Copy link
Contributor

keegan-caruso commented Mar 6, 2024 •
edited
Loading

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.

@keegan-caruso keegan-caruso added the IdentityModel8x Future breaking issues/features for IdentityModel 8x label Mar 26, 2024
@jennyf19
Copy link
Collaborator

duplicate of #1794

@FuPingFranco FuPingFranco mentioned this issue Apr 15, 2024
Closed
14 tasks
@etiennelepagel
Copy link

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

This was referenced May 7, 2024
Closed
Closed
@halter73
Copy link
Contributor

halter73 commented May 7, 2024

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 #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

@halter73 halter73 mentioned this issue May 21, 2024
Closed
1 task
This was referenced Jun 3, 2024
Open
Closed
@brentschmaltz
Copy link
Member

@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
Copy link

ItWorksOnMyMachine commented Jun 13, 2024 •
edited
Loading

@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" />

@keegan-caruso keegan-caruso mentioned this issue Jun 28, 2024
Open
@brentschmaltz
Copy link
Member

@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?

This was referenced Jul 23, 2024
Closed
Open
@jennyf19 jennyf19 added the Dependency Mismatch Transitive dependency might be at play and create issues resulting in incorrect versions of a class label Aug 29, 2024
@keegan-caruso keegan-caruso added the P1 More important, prioritize highly label Sep 5, 2024
@pmaytak pmaytak modified the milestones: 8.0.3, 7.7.2 Sep 5, 2024
@pmaytak pmaytak mentioned this issue Sep 5, 2024
Open
14 tasks
@keegan-caruso
Copy link
Contributor

@ItWorksOnMyMachine if you list the transitive dependencies I expect you will see mismatched versions

dotnet list package --include-transitive

@jennyf19 jennyf19 added P2 High, but not urgent. Needs to be addressed within the next couple of sprints and removed P1 More important, prioritize highly labels Oct 4, 2024
This was referenced Oct 23, 2024
Open
Open
@KirkMunroSagent KirkMunroSagent mentioned this issue Oct 23, 2024
Closed
@keegan-caruso keegan-caruso mentioned this issue Oct 25, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@keegan-caruso keegan-caruso

Labels
Dependency Mismatch Transitive dependency might be at play and create issues resulting in incorrect versions of a class Enhancement The issue is a new feature IdentityModel8x Future breaking issues/features for IdentityModel 8x P2 High, but not urgent. Needs to be addressed within the next couple of sprints
Projects
None yet
Milestone
7.7.2
Development

No branches or pull requests
8 participants
@halter73 @ItWorksOnMyMachine @brentschmaltz @reduckted @jennyf19 @pmaytak @etiennelepagel @keegan-caruso