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

Fix conflict with ASP.NET authentication #2211

Open
wants to merge 2 commits into
base: dev
Choose a base branch
from

Conversation

KirkMunroSagent
Copy link

@KirkMunroSagent KirkMunroSagent commented Oct 23, 2024

Since the release of OpenIddict 5.3.0, a conflict with ASP.NET authentication has been exposed.

The conflict arises when different versions of the Microsoft.IdentityModel.* packages are in use. In an ASP.NET project built on .NET 8 that uses OpenIddict 5.3.0 or later, authentication will fail due to OpenIddict using Microsoft.IdentityModel.* packages with versions in the 8.x range, while ASP.NET nupkgs such as Microsoft.AspNetCore.Authentication.OpenIdConnect have a dependency on Microsoft.IdentityModel.Protocols.OpenIdConnect with the version in the 7.x range. You can read more about this issue here and a feature request to warn when the versions are not in sync here.

Customers using ASP.NET that are facing this issue can resolve this by taking a direct dependency on Microsoft.AspNetCore.Authentication.OpenIdConnect, taking care to make sure that the version matches exactly what OpenIddict uses. It would be easier, however, if OpenIddict added the additional dependency since it appears from this issue and the discussion about it that this collection of nupkgs are designed to be used as set rather than individually due to incompatibilities in how they function between versions. Therefore, I thought it might be wise for OpenIddict to simply take on the additional package dependency even if it is not used directly in the OpenIddict code, if only to avoid this incompatibility downstream for OpenIddict customers. It's difficult to figure out what is going wrong when facing this incompatibility, so having this additional package will simply make the lives of OpenIddict customers easier.

@kevinchalet
Copy link
Member

Thanks for your PR!

You can read more about this issue here...

I'm (sadly 😄) well aware of that issue (and posted more information on why it's really happening here, BTW).

Therefore, I thought it might be wise for OpenIddict to simply take on the additional package dependency even if it is not used directly in the OpenIddict code, if only to avoid this incompatibility downstream for OpenIddict customers.

It's an option I considered too at some point to reduce the number of reports I was receiving but I eventually decided against it: that package brings a lot of artifacts with it, including the legacy JWT handler (System.IdentityModel.Tokens.Jwt) that OpenIddict no longer uses for a while now (it was actually one of the very first third-party projects to adopt the new JsonWebTokenHandler 😄)

I agree it would help make things easier for many people, but the tradeoff is hard to sell, unfortunately.

The positive note is that ASP.NET Core 9.0 will finally reference a much more recent version of IM so ASP.NET Core 9.0 users will no longer be impacted (until the Azure AD team decides to introduce a new breaking change, of course 😅)

... a feature request to warn when the versions are not in synd AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet#2513.

@jennyf19 hey Jenny, hope you're doing well. Last time we discussed, you mentioned you were still interested in making that happen. Did your team make any progress in this area?

Cheers.

@KirkMunroSagent
Copy link
Author

Thanks for your PR!

It's my pleasure. Sometimes it's just as easy to open a PR as it is to start a discussion, so I figured I'd just do that since this was a trivial one, knowing up front that the PR might not be something that you want to take on.

I'm (sadly 😄) well aware of that issue (and posted more information on why it's really happening AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet#2514 (comment), BTW).

Thanks for pointing out those details. I always like understanding what causes these problems, and hadn't dug quite deep enough to get to the specific cause like you had.

The positive note is that ASP.NET Core 9.0 will finally reference a much more recent version of IM so ASP.NET Core 9.0 users will no longer be impacted (until the Azure AD team decides to introduce a new breaking change, of course 😅)

That's good news...we have .NET 9 on our roadmap, so I'll plan on removing the workaround direct dependency I added to our project yesterday when we make that switch. If their regression test additions that they mention in response to the comment you linked are well designed, hopefully they'll pick up on such issues before they ship so that the chances of this happening again decrease.

@jennyf19
Copy link

jennyf19 commented Oct 25, 2024

We've been going back and forth with the .NET team on what the right fix would be, so far, our solution is to implement analyzers to detect package mismatches, no ETA on when that will be released, and that is a shift-left solution. We know this is a huge customer pain-point, we just want to find the right long-term solution, which @keegan-caruso opened: AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet#2943. We are tracking all of the related issues here, @pmaytak will be working on the analyzer and @keegan-caruso spent a lot of time investigation possible alternate solutions, including the one linked above.

@kevinchalet
Copy link
Member

We are tracking all of the related issues here, @pmaytak will be working on the analyzer and @keegan-caruso spent a lot of time investigation possible alternate solutions, including the one linked above.

Great to hear, thanks @jennyf19!

Out of curiosity, are the NuGet folks involved in that discussion? Maybe it's an issue that could be handled at the package manager level?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants