-
Notifications
You must be signed in to change notification settings - Fork 5.3k
Description
Continuing discussion from #46464.
Add the ability to support list of claim values in the System.Security.Claims.HasClaim method.
The issue is that when I pass an array of claims of the same type to this method System.IdentityModel.Tokens.Jwt.JwtSecurityTokenHandler.WriteToken it groups them into an array.
@bartonjs I'm not sure if there is an issue in this repo.
There are those two, aspnet/Identity#1394 and DuendeArchive/IdentityServer4#1452 though they aren't from this repository.
Additionally this is a specification OpenID Connect Core 1.0, rfc7519.Also, I agree that the default implementation shouldn't change. Maybe an extension method would be more appropriate?
Or something like System.Security.Claims.Extensions.Json? I don't know. But, I'm open to suggestions, guidelines and/or PRs
Originally posted by @TsakiDev in #46464 (comment)
Hm, apparently somewhere I missed the email version of the reply.
I agree that the default implementation shouldn't change. Maybe an extension method would be more appropriate?
Yeah, I can see an extension method working, though naming it will be interesting 😄. It would work better as a derived type (so it can override the behavior) but then all relevant callers need to switch to it. Of course, nothing says you can't do both.
The bigger problem is where to put it/them. It looks like System.Security.Claims.dll is part of the shared runtime. Adding the JSON array support there would mean adding a dependency to System.Text.Json (which we /could/ undo later if we felt it necessary) and that the functionality would be limited to .NET 6+ caller-contexts.
If the functionality were added to a new library (System.Security.Claims.Json?) it could be a .NET Standard 2.0 package (assuming that the common callers are themselves .NET Standard 2.0 libraries), but at basically one method that's pretty high overhead. Maybe there are other JSON-claims things that would make sense once there's a place for them, though. (@terrajobst do you have thoughts here?)
Since you agree that changing the default behavior isn't right, we should probably close this PR and open an issue to talk out what the plan is. I'd prefer that you opened the issue rather than I just sort of text-dump things from here into it, so that you have the opportunity to give a good description of what functional behaviors you'd hope for in an ideal world (maybe there's already enough to justify a package) and who you think might want this/what platforms they're targeting (aka is .NET Standard 2.0 actually important, or is everyone building on latest only).
Originally posted by @bartonjs in #46464 (comment)