Added support for multiple values (arrays) in default claim action - Remove Original Claim

[phantasm33]

2.1.0-preview1 added support for multiple values (array) in default claim action as requested in:

• Issue Uninstall RC1 requires re-download of original installer #1449 - OIDC, I cannot add extra claims from userinfo endpoint aspnet/Security#1449 (comment)

• Pull Request Can ASP.NET Membership interoperate with ASP.NET Core? #1501 - Added support for multiple values (arrays) in default claim action aspnet/Security#1501

While the added class - JsonKeyClaimAction - does do the requested action, it still leaves the original claim in place. My request is to modify the code so that it also remove the claim.

Here is an illustration of the issue. Right now, I have the following claim:
`role - ["foo", "bar] ``

Which e JsonKeyClaimAction transoforinto:

role - ["foo","bar']
role - "foo"
role - "bar"

The transformation is good, but what I really want to end up with is this:

role - "foo"
role - "bar"

Here is my suggested change (sorry, I don't know how to do pull requests. I am an old TFS guy):

/// <summary>
/// A ClaimAction that selects a top level value from the json user data with the given key name and adds it as a Claim.
/// This no-ops if the key is not found or the value is empty.
/// </summary>
public class JsonKeyClaimAction : ClaimAction
{
    /// <summary>
    /// Creates a new JsonKeyClaimAction.
    /// </summary>
    /// <param name="claimType">The value to use for Claim.Type when creating a Claim.</param>
    /// <param name="valueType">The value to use for Claim.ValueType when creating a Claim.</param>
    /// <param name="jsonKey">The top level key to look for in the json user data.</param>
    public JsonKeyClaimAction(string claimType, string valueType, string jsonKey)
        : base(claimType, valueType)
    {
        JsonKey = jsonKey;
    }

    /// <summary>
    /// The top level key to look for in the json user data.
    /// </summary>
    public string JsonKey { get; }

    /// <inheritdoc />
    public override void Run(JObject userData, ClaimsIdentity identity, string issuer)
    {
        var value = userData?[JsonKey];
        if (value is JValue)
        {
            AddClaim(value?.ToString(), identity, issuer);
        }
        else if (value is JArray)
        {
	var claim = identity.FindFirst(c => c.Type == this.JsonKey);

            foreach (var v in value)
            {
                AddClaim(v?.ToString(), identity, issuer);
            }

	RemoveClaim(claim, identity);
        }
    }

    private void AddClaim(string value, ClaimsIdentity identity, string issuer)
    {
        if (!string.IsNullOrEmpty(value))
        {
            identity.AddClaim(new Claim(ClaimType, value, ValueType, issuer));
        }
    }

    private void RemoveClaim(Claim claim, ClaimsIdentity identity)
    {
        if (claim != null)
            identity.TryRemoveClaim(claim);
    }

I don't know if it makes sense to modify the original JsonKeyClaimAction (as I have done above) or to create a new ClaimActon that does the the removal. This way folks have the option of removing the bad claim or not. Personally the original makes no sense so I would like to remove it. Others may not agree.

[Tratcher]

The code today is:
https://github.com/aspnet/AspNetCore/blob/f3072339de24621b279694b75cadb4526e14b87d/src/Security/Authentication/OAuth/src/JsonKeyClaimAction.cs#L40-L46
How does role - ["foo","bar'] get added?

[phantasm33]

@Tratcher

Thanks for asking the question. It got me rethinking about the issue. I believe I see my mistake:

            JwtSecurityTokenHandler.DefaultInboundClaimTypeMap.Clear();

            var identityConfig = Configuration.GetSection("IdentityConfig").Get<IdentityConfiguration>();
            services.AddAuthentication(options =>
                {
                    options.DefaultScheme = "Cookies";
                    options.DefaultChallengeScheme = "oidc";
                })
                .AddCookie("Cookies", options =>
                {
                    options.Cookie.Name = "psaid"; 
                })
                .AddOpenIdConnect("oidc", options =>
                {
                    options.SignInScheme = "Cookies";
                    options.Authority = identityConfig.Authority;
                    options.RequireHttpsMetadata = true;
                    options.ClientId = identityConfig.ClientId;
                    options.ClientSecret = identityConfig.ClientSecret;
                    options.ResponseType = "code id_token";
                    options.SaveTokens = true;
                    options.GetClaimsFromUserInfoEndpoint = true;
                    options.TokenValidationParameters = new TokenValidationParameters()
                    {
                        RoleClaimType = "role",
                        NameClaimType = "name"
                    };
                    
                    options.ClaimActions.MapAllExcept("iss", "nbf", "exp", "aud", "nonce", "iat", "c_hash", "auth_time", "idp", "amr");
                    options.ClaimActions.Add(new JsonKeyClaimAction(JwtClaimTypes.Role, null, JwtClaimTypes.Role));
                 });

In the code above, by the time the JsonKeyClaimAction is run, the role claim is already mapped into the Identity.Claims collection. This is why I need to remove it. I suspect that the correct solution is to add the "role" claim to the list of exempted claims:

 options.ClaimActions.MapAllExcept("role", "iss", "nbf", "exp", "aud", "nonce", "iat", "c_hash", "auth_time", "idp", "amr");
 options.ClaimActions.Add(new JsonKeyClaimAction(JwtClaimTypes.Role, null, JwtClaimTypes.Role));

I'm thinking making this change would eliminate the need to remove to role - ["foo", "bar"] as it is never added to the Claims collection.

I will test it on Monday and let you know, but I'm guessing you already know that's where I went wrong😄.