2
\$\begingroup\$

There is a need to authorize the Users based on their Roles, particularly a set of roles that should allow all GET operations and a second set that should allow all POST operations.

Eg:

  • All Get methods should be accessible to the reader
  • Post method access is granted to contributors

I don't want to mark the individual methods with the [Authorize()] as I have ~20 controllers with each supporting all 4 Http Verbs.

I have implemented something like this

[RoleBasedAuthorizeAttribute(new string[2] { "CJE.Reader", "CJE.Contributor" }, new string[1] { "CJE.Contributor" }, new string[1] { "CJE.Contributor" }, new string[1] { "CJE.Contributor" })]
public abstract class BaseController : ControllerBase
{

and

public sealed class RoleBasedAuthorizeAttribute : AuthorizeAttribute, IAuthorizationFilter
{
    public RoleBasedAuthorizeAttribute(string[] getRoleMap, string[] postRoleMap, string[] putRoleMap, string[] deleteRoleMap)
    {
        this.GetRoleMap = getRoleMap;
        this.PostRoleMap = postRoleMap;
        this.PutRoleMap = putRoleMap;
        this.DeleteRoleMap = deleteRoleMap;
    }

    public string[] GetRoleMap { get; }

    public string[] PostRoleMap { get; }

    public string[] PutRoleMap { get; }

    public string[] DeleteRoleMap { get; }

    public void OnAuthorization(AuthorizationFilterContext context)
    {
        if (!this.IsUserAllowed(context))
        {
            context.Result = new UnauthorizedResult();
        }
    }

    private bool IsUserAllowed(AuthorizationFilterContext context)
    {
        var roles = ((ClaimsIdentity)context.HttpContext.User.Identity).Claims
            .Where(c => c.Type == ClaimTypes.Role)
            .Select(c => c.Value);

        switch (context.HttpContext.Request.Method)
        {
            case "GET":
                return this.GetRoleMap.Any(r => roles.Contains(r));
            case "POST":
                return this.PostRoleMap.Any(r => roles.Contains(r));
            case "PUT":
                return this.PutRoleMap.Any(r => roles.Contains(r));
            case "DELETE":
                return this.DeleteRoleMap.Any(r => roles.Contains(r));
            default:
                return false;
        }
    }
}

this works fine. However, I feel that this needs to be optimized. Any suggestions?

\$\endgroup\$

1 Answer 1

1
\$\begingroup\$

BaseController

  • There is no need to use the Attribute suffix whenever you apply an attribute
[RoleBasedAuthorize(new string[2] { ... })]
public abstract class BaseController : ControllerBase
  • If possible please prefer enum over string since they are less error-prone
[RoleBasedAuthorize(new CJERoles[2] { CJERoles.Reader, CJERoles.Contributor }, ...)]
public enum CJERoles
{
    [Display(Name = "CJE.Reader")]
    Reader = 0,
    [Display(Name = "CJE.Contributor")]
    Contributor = 1,
}
  • Please also prefer named arguments and line breaks
  • I would also suggest to use xyzRoles parameter names over xyzRoleMap
[RoleBasedAuthorize(
    getRoles: new CJERoles[2] { CJERoles.Reader, CJERoles.Contributor },
    postRoles: new CJERoles[1] { CJERoles.Contributor },
    putRoles: new CJERoles[1] { CJERoles.Contributor },
    deleteRoles: new CJERoles[1] { CJERoles.Contributor })]

RoleBasedAuthorizeAttribute

XYZRoleMap

  • It is good that you have made the properties read-only
  • You could also remove the public access modifier since they are used only internally

Constructor

  • By accepting enums rather than strings you need to perform some mapping
  • I would suggest to map your enums to strings, because I assume not all user roles can be mapped to an enum value
    • In order to ease this mapping I would suggest to introduce the following extension method
public static class CJERolesExtensions
{
    public static string GetDisplayName(this CJERoles enumValue)
        => typeof(CJERoles)
            .GetMember(enumValue.ToString())
            .First()
            .GetCustomAttribute<DisplayAttribute>()
            .GetName();
}
  • With this in our hand we can perform the mappings like this
public RoleBasedAuthorizeAttribute(CJERoles[] getRoles, CJERoles[] postRoles, CJERoles[] putRoles, CJERoles[] deleteRoles)
{
    Func<CJERoles[], string[]> map = roles => roles.Select(role => role.GetDisplayName()).ToArray();

    (GetRoleMap, PostRoleMap, PutRoleMap, DeleteRoleMap) = (map(getRoles), map(postRoles), map(putRoles), map(deleteRoles));
}

IsUserAllowed

  • I would suggest to inline this method inside the OnAuthorization
  • I would also suggest to avoid code duplication use switch expression to select the proper string[] role map
public void OnAuthorization(AuthorizationFilterContext context)
{
    var roles = ((ClaimsIdentity)context.HttpContext.User.Identity).Claims
        .Where(c => c.Type == ClaimTypes.Role)
        .Select(c => c.Value);

    var roleMap = context.HttpContext.Request.Method switch
    {
        "GET" => GetRoleMap,
        "POST" => PostRoleMap,
        "PUT" => PutRoleMap,
        "DELETE" => DeleteRoleMap,
        _ => new string[] { }
    };

    if (!roleMap.Any(roles.Contains))
    {
        context.Result = new UnauthorizedResult();
    }
}
\$\endgroup\$
0

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.