Refactor role claim retrieval to use identity-specific role claim type

[zyofeng]

Why this PR?

Currently RoleTypes are hardcoded to ClaimTypes.Role, which is not flexible if claims are mapped differently

[zhiyuanliang-ms]

Hey, @zyofeng

This PR looks good to me. Thanks for your contribution.

[zhiyuanliang-ms]

Sample code to illustrate the problem:

using System.Security.Claims;


internal static class Program
{
    private static void Main()
    {
        Scenario1();

        Console.WriteLine();
        Console.WriteLine(new string('=', 80));
        Console.WriteLine();

        Scenario2();
    }

    private static void Scenario1()
    {
        // This identity says that its role claims use the claim type "roles", not ClaimTypes.Role.
        var jwtIdentity = new ClaimsIdentity(
            claims: new[]
            {
                new Claim(ClaimTypes.Name, "alice"),
                new Claim("roles", "Admin"),
                new Claim("roles", "Auditor")
            },
            authenticationType: "Bearer",
            nameType: ClaimTypes.Name,
            roleType: "roles");

        var principal = new ClaimsPrincipal(jwtIdentity);

        PrintPrincipal(principal);

        var firstVersion = GetRolesUsingFixedClaimTypesRole(principal).ToList();
        var secondVersion = GetRolesUsingEachIdentityRoleClaimType(principal).ToList();

        Console.WriteLine($"First version: {(firstVersion.Count == 0 ? "<empty>" : string.Join(", ", firstVersion))}");
        Console.WriteLine($"Second version: {(secondVersion.Count == 0 ? "<empty>" : string.Join(", ", secondVersion))}");

        // IsInRole uses identity.RoleClaimType internally, so this succeeds.
        Console.WriteLine($"principal.IsInRole(\"Admin\") = {principal.IsInRole("Admin")}");
    }

    private static void Scenario2()
    {
        var cookieIdentity = new ClaimsIdentity(
            claims: new[]
            {
                new Claim(ClaimTypes.Name, "bob"),
                new Claim(ClaimTypes.Role, "Manager"),
                new Claim(ClaimTypes.Role, "ReportReader")
            },
            authenticationType: "Cookies",
            nameType: ClaimTypes.Name,
            roleType: ClaimTypes.Role);

        var principal = new ClaimsPrincipal(cookieIdentity);

        PrintPrincipal(principal);

        var firstVersion = GetRolesUsingFixedClaimTypesRole(principal).ToList();
        var secondVersion = GetRolesUsingEachIdentityRoleClaimType(principal).ToList();

        Console.WriteLine($"First version: {(firstVersion.Count == 0 ? "<empty>" : string.Join(", ", firstVersion))}");
        Console.WriteLine($"Second version: {(secondVersion.Count == 0 ? "<empty>" : string.Join(", ", secondVersion))}");

        Console.WriteLine($"principal.IsInRole(\"Manager\") = {principal.IsInRole("Manager")}");
    }

    private static IEnumerable<string> GetRolesUsingFixedClaimTypesRole(ClaimsPrincipal principal)
    {
        return principal.Claims
            .Where(c => c.Type == ClaimTypes.Role)
            .Select(c => c.Value);
    }

    private static IEnumerable<string> GetRolesUsingEachIdentityRoleClaimType(ClaimsPrincipal principal)
    {
        return principal.Identities
            .SelectMany(identity => identity.Claims.Where(c => c.Type == identity.RoleClaimType))
            .Select(c => c.Value);
    }

    private static void PrintPrincipal(ClaimsPrincipal principal)
    {
        int index = 0;

        foreach (var identity in principal.Identities)
        {
            Console.WriteLine($"Identity #{index}");
            Console.WriteLine($"  AuthenticationType : {identity.AuthenticationType}");
            Console.WriteLine($"  Name               : {identity.Name}");
            Console.WriteLine($"  RoleClaimType      : {identity.RoleClaimType}");
            Console.WriteLine("  Claims:");

            foreach (var claim in identity.Claims)
            {
                Console.WriteLine($"    Type = {claim.Type}");
                Console.WriteLine($"    Value = {claim.Value}");
            }

            Console.WriteLine();
            index++;
        }
    }
}

ClaimsIdentity.RoleClaimType

cc @jimmyca15 @rossgrambo

[rossgrambo]

Thank you for this PR. This makes sense.

I had a small comment and want to note that this PR is a breaking change since group assignments may be added or omitted after this change so we'll likely need to wait for a major version bump.

[zyofeng]

Thank you for this PR. This makes sense.

I had a small comment and want to note that this PR is a breaking change since group assignments may be added or omitted after this change so we'll likely need to wait for a major version bump.

Honestly, this feels more like a bug fix. The DefaultRoleClaimType for ClaimsIdentity is ClaimTypes.Role, so this would only impact cases where someone has customized the claims mapping (and do that's expected based on config)

https://github.com/dotnet/runtime/blob/9af7d041cf445c184ed84756b54b07104b50d943/src/libraries/System.Security.Claims/src/System/Security/Claims/ClaimsIdentity.cs#L45

[jimmyca15]

I think it's fine for group assignments to be added. But I do think it's problematic that there's a chance of omission of previously included groups.

Perhaps we could check both ClaimTypes.Role and identity.RoleClaimType.

[zyofeng]

I think it's fine for group assignments to be added. But I do think it's problematic that there's a chance of omission of previously included groups.

Perhaps we could check both ClaimTypes.Role and identity.RoleClaimType.

Are we good to merge this?