[IMPLEMENTED] Support multiple roles in oidc role mapping

When an identity is created it may belong to one or more roles.

Currently the OidcHandler has

public override Task TokenValidated(TokenValidatedContext context)
    var identity = (ClaimsIdentity)context.Principal!.Identity!;
    if (!string.IsNullOrWhiteSpace(options.OidcRoleClaimType) && options.OidcRoleMapping?.Count >= 0)
        var role = identity.FindFirst(x => x.Type == options.OidcRoleClaimType)?.Value;
        if (!string.IsNullOrWhiteSpace(role) && options.OidcRoleMapping.TryGetValue(role, out var permissions) && permissions != null)
            foreach (var permission in permissions)
                identity.AddClaim(new Claim(SquidexClaimTypes.Permissions, permission));
    return base.TokenValidated(context);

which is grabbing the first role it sees and then trying to use that to match on the role mapping collection. I’m curious if you would be open to expanding this to either

  1. Matching a single role within the collection.
  2. Matching all roles in the collection.

Take the below example:

var options = new Options {
    OidcRoleClaimType = "role",
    OidcRoleMapping = new Dictionary<string, List<string>> {
        { "role1", new List<string> { "perm1", "perm2" } },
        { "role2", new List<string> { "perm3", "perm1" } }
var identity = new Identity(
    new List<Claim> {
        new Claim("role", "user"),
        new Claim("role", "role3"),
        new Claim("role", "role2"),
        new Claim("role", "role1")

In the current handler, the first role user does not exist in the role mapping and thus no permissions are returned.

In option 1, we would match on the first role that exists in the mapping. In this case, role1 and the resulting permissions added would be ["perm1", "perm2"]

In option2, we would match on all roles that exist in the mapping. So, in this case both role1 and role2 exist in the mapping and the result would be ["perm1", "perm2", "perm3"]

While I think it would make sense to go with option #2, it does potentially result in a breaking change for people using the existing role mapping; this may result in more permissions being applied than anticipated. While it does fall more inline with rbac in general, it may be worthwhile to consider.

An example showing the current implementation and the options: https://gist.github.com/jrlost/adaf6e32502ee55ad7c401cfbde9fec5

Sure, PRs are welcome :wink:

I am absolutely willing to submit a PR; just wanted to get your input on which option you think would make the most sense.

Traditionally, in rbac, role bindings are aggregated; hense I’d lean towards option #2. However, there is the concern that anyone using this currently would potentially have more exposure than they are/were expecting.

I think they should be aggregated.

Sounds good, I’ll get a PR up today.

1 Like

https://github.com/Squidex/squidex/pull/870 has been submitted. Thanks again.

This topic was automatically closed after 2 days. New replies are no longer allowed.