Adds the ability to support list of claim values, of the same claim type

[DefinitelyADev]

Adds the ability to support list of claim values in the System.Security.Claims.HasClaim method.

[ghost]

Tagging subscribers to this area: @bartonjs, @vcsjones, @krwq
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Adds the ability to support list of claim values in the System.Security.Claims.HasClaim method.

Author:	TsakiDev
Assignees:	-
Labels:	area-System.Security
Milestone:	-

[dnfadmin]

All CLA requirements met.

[KalleOlaviNiemitalo]

It seems this would cause errors if Claim.Value starts with "[" but is not in JSON format. Perhaps it could be made safer by also verifying that Claim.ValueType means JSON, but I don't see any such URI in ClaimValueTypes.

Can you instead make the ClaimsIdentity have multiple Claim instances with the same Claim.Type? That's what System.IdentityModel.Tokens.Saml2SecurityTokenHandler.ProcessAttributeStatement in .NET Framework does if an attribute has multiple values: https://github.com/microsoft/referencesource/blob/5697c29004a34d80acdaf5742d7e699022c64ecd/System.IdentityModel/System/IdentityModel/Tokens/Saml2SecurityTokenHandler.cs#L1841-L1865

[DefinitelyADev]

@KalleOlaviNiemitalo Since RFC7519 supports this I don't see a reason for not supporting it. Also, I believe I fixed it in the latest commit.

https://tools.ietf.org/html/rfc7519

[DefinitelyADev]

I do not understand what the problem is and the tests fail.
Is it the line endings?

[KalleOlaviNiemitalo]

You added U+200B ZERO WIDTH SPACE characters at the end of several lines.

[KalleOlaviNiemitalo]

What stores a JSON array of strings as Claim.Value? Is it Microsoft.AspNetCore.Authentication.OAuth.Claims.JsonKeyClaimAction or something specific to your application? If ASP.NET Core does that, I think it would be better to make ASP.NET Core provide an extension method that parses the JSON from the claim, than to make the Claim class itself expect JSON syntax. That would keep the Claim class more neutral on whether it is used for JWT or SAML or something entirely different.

[DefinitelyADev]

You added U+200B ZERO WIDTH SPACE characters at the end of several lines.

Thanks!
Brain-freeze...

[DefinitelyADev]

What stores a JSON array of strings as Claim.Value? Is it Microsoft.AspNetCore.Authentication.OAuth.Claims.JsonKeyClaimAction or something specific to your application? If ASP.NET Core does that, I think it would be better to make ASP.NET Core provide an extension method that parses the JSON from the claim, than to make the Claim class itself expect JSON syntax. That would keep the Claim class more neutral on whether it is used for JWT or SAML or something entirely different.

This does it System.IdentityModel.Tokens.Jwt.JwtSecurityTokenHandler.WriteToken
When I pass an array of claims of the same type it groups them into an array.

[KalleOlaviNiemitalo]

System.IdentityModel.Tokens.Jwt (JwtPayload.cs:458) seems to set Claim.ValueType = "JSON_ARRAY".

[DefinitelyADev]

System.IdentityModel.Tokens.Jwt (JwtPayload.cs:458) seems to set Claim.ValueType = "JSON_ARRAY".

Ok yes, what is your point?

[KalleOlaviNiemitalo]

It continued my earlier comment: "Perhaps it could be made safer by also verifying that Claim.ValueType means JSON". If ClaimsIdentity.HasClaim deserialized JSON only from those claims where Claim.ValueType == "JSON_ARRAY", I think it would minimize the risk of losing performance or causing unexpected results with claims whose values are not actually JSON.

However, I wonder how a JSON dependency at this level will affect the ability to trim an application.

The HasClaim methods are virtual. Perhaps the JSON-specific logic could instead be implemented in System.IdentityModel.Tokens.Jwt, by deriving a class from ClaimsIdentity and then using that in Microsoft.IdentityModel.Tokens.TokenValidationParameters.CreateClaimsIdentity… that does not look simple, either.

[DefinitelyADev]

It continued my earlier comment: "Perhaps it could be made safer by also verifying that Claim.ValueType means JSON". If ClaimsIdentity.HasClaim deserialized JSON only from those claims where Claim.ValueType == "JSON_ARRAY", I think it would minimize the risk of losing performance or causing unexpected results with claims whose values are not actually JSON.

However, I wonder how a JSON dependency at this level will affect the ability to trim an application.

The HasClaim methods are virtual. Perhaps the JSON-specific logic could instead be implemented in System.IdentityModel.Tokens.Jwt, by deriving a class from ClaimsIdentity and then using that in Microsoft.IdentityModel.Tokens.TokenValidationParameters.CreateClaimsIdentity… that does not look simple, either.

I'm thinking that it doesn't add much footprint, since the original comparison is done first and then tries the deserialization, that's when it does the additional steps.

[KalleOlaviNiemitalo]

I'm thinking that it doesn't add much footprint, since the original comparison is done first and then tries the deserialization, that's when it does the additional steps.

I meant trimming as described in Trim self-contained deployments and executables. If ClaimsIdentity.HasClaim can call JsonSerializer.Deserialize, then that could prevent the IL linker from removing the JSON classes from the published application, even if the call is never executed at run time. I don't know how much they cost in size; and perhaps trimming is most important for Blazor applications that would be likely to require the JSON classes for other reasons anyway.

[DefinitelyADev]

I'm thinking that it doesn't add much footprint, since the original comparison is done first and then tries the deserialization, that's when it does the additional steps.

I meant trimming as described in Trim self-contained deployments and executables. If ClaimsIdentity.HasClaim can call JsonSerializer.Deserialize, then that could prevent the IL linker from removing the JSON classes from the published application, even if the call is never executed at run time. I don't know how much they cost in size; and perhaps trimming is most important for Blazor applications that would be likely to require the JSON classes for other reasons anyway.

Yeah I'm currently working on a couple Blazor projects and there aren't any of them that don't use JSON. One of those projects was also the reason that I implemented this PR and it will probably be useful for the others.

[bartonjs]

@TsakiDev Was there an issue discussing this feature that I'm not seeing?

Personally, I'm not sure that I like the idea of changing the claim processor at this point to have knowledge of some particular JSON-ims, and there are degenerate cases where things can go wrong. Consider, for the sake of discussion, an identity provider that has two distinct claim values, ["a"] and a.

// Does anyone know why Fabrikam Identity uses such bad role names?
bool isAuditor = id.HasClaim("[\"a\"]");
bool isAdministrator = id.HasClaim("a");

In this degenerate case, an identity with the claim of [\"a\"] is now automatically both an auditor (String.Equals) and an administrator (String.Equals on the processed first array element). This would break the security model, resulting in an elevation of privilege.

Adding special "some identities use JSON arrays" support now invites that other formats would be added later (and that no format can ever be taken away). Combined with the fact that this type lives in the shared runtime, it means that any library using claims and using .NET Standard 2.0 couldn't really reason about whether their HasClaim is going to work or not. (They'd have to target specific versions of .NET to indicate the minimum runtime behaviors they want for the same API).

---

All in all, I think that a better answer would be to make a new type (which extends ClaimsIdentity to override HasClaim) so that there's explicit opt-in (at some level) to this behavior. It probably wants to be in a different library than System.Security.Claims, so that it doesn't leave a perpetual dependency on JSON. And I'm not really sure that it even belongs in dotnet/runtime (it seems to mesh more with the JWT stuff, which is in a different repository... but maybe there's room for a new System.Security.Claims.Json package built out of dotnet/runtime).

[DefinitelyADev]

@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

[bartonjs]

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).

[DefinitelyADev]

Yeah, I can see an extension method working, though naming it will be interesting 😄.

Indeed I tried it for my project, imagine 2 people looking at a screen for a minute or two with a flat encephalogram... 🤪
Ended up naming it HasClaimNew....
Renaming will occur....

I wanted to ask if an API proposal or a Blank issue would be more appropriate?

[bartonjs]

I wanted to ask if an API proposal or a Blank issue would be more appropriate?

I think "API Proposal" is more for when you have an idea for "this is what I want, unless discussion takes it elsewhere". If you're in the "I know the concept I want, but not quite what it should look like, let's hope stakeholders weigh in and we figure out what we all collectively want" then a blank issue is probably better. (Once it's ready the top post gets edited with the proposal and a label says API review should look at it... the templates just guide how the issue starts)

[DefinitelyADev]

Closing as of #48467