-
-
Notifications
You must be signed in to change notification settings - Fork 520
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
base: dev
Are you sure you want to change the base?
Fix conflict with ASP.NET authentication #2211
Conversation
Thanks for your PR!
I'm (sadly 😄) well aware of that issue (and posted more information on why it's really happening here, BTW).
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 ( 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 😅)
@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. |
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.
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.
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. |
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. |
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? |
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.