Authorization infrastructure does not handle "per action permissions" use case well

[kwaclaw]

In WebApi 2 we used an AuthorizationFlterAttribute to implement the following permissions based concept:

• Each controller action has a permission code associated.
• Roles are associated with a set of permissions. This set can change over time.
• A user has one or more roles.
• Authorization is determined by checking if any of the user's roles is associated with the action's permission code.

This was easy to use by the developer. He/she only had to decorate an action with the corresponding attribute.

It seems the new policy based approach in ASP.NET 5 makes it hard to implement his concept, because there is no way for the Handle method of the AuthorizationHandler to include the permission code in its arguments. The only way I could solve this problem was to create one new policy for each unique permission code. Since there is a large number of actions, I now have to create an equally large number of policies. It would not be a huge change to the infrastructure to be able to attach extra information to the attribute and pass it through to the handler in the AuthorizationContext, or even pass the attribute through itself. For example, the handler code could then look like this:

protected override void Handle(AuthorizationContext context, PermissionRequirement requirement) {
    var userRoles = context.User.FindAll(System.Security.Claims.ClaimTypes.Role);
    var permissionsAttribute = context.Attribute as PermissionAttribute;
    if (permissionsAttribute == null)
        return;
    if (myPermissionsChecker.RolesHavePermission(userRoles, permissionsAttribute.Permission)) {
        context.Succeed(requirement);
    }
}

This would only require a single policy, and the developer would not be burdened with having to create a new policy everytime a new controller action is added.

[brockallen]

This might help (in RC2): #630

[kwaclaw]

From where is the resource picked up? I don't see a change to the AuthorizeAttribute to include a Resource property.

[brockallen]

You would pass it explicitly. It's the imperative approach. You're right that it's harder to do declaratively.

[kwaclaw]

I see. I would prefer the declarative option though.

[davidfowl]

When you're using MVC, the resource by default is the filter's authorization context. You can use that to get information about the action being executed and that's how you can discover the attribute:

public class Startup
{
    public void ConfigureServices(IServiceCollection services)
    {
        services.AddMvc();

        services.AddAuthorization(options =>
        {
            options.AddPolicy("Permissions", builder =>
            {
                builder.Requirements.Add(new PermissionsRequirement());
            });
        });

        services.AddSingleton<IAuthorizationHandler, PermissionsHandler>();
        services.AddSingleton<IPermissionService, PermissionService>();
    }

    // This method gets called by the runtime. Use this method to configure the HTTP request pipeline.
    public void Configure(IApplicationBuilder app, ILoggerFactory loggerFactory)
    {
        loggerFactory.AddConsole(LogLevel.Debug);

        app.UseIISPlatformHandler();

        app.Use((context, next) =>
        {
            // This would be cookie or bearer auth, I'm just hacking to show how it works
            context.User = new ClaimsPrincipal(new ClaimsIdentity("Something"));

            return next();
        });

        app.UseMvc();
    }

    // Entry point for the application.
    public static void Main(string[] args) => WebApplication.Run<Startup>(args);
}

public class PermissionService : IPermissionService
{
    public bool RolesHavePermission(IEnumerable<Claim> userRoles, string permissions)
    {
        // Implement something real
        return string.Equals(permissions, "Teacher");
    }
}

public class PermissionsRequirement : IAuthorizationRequirement
{
}

public class PermissionsHandler : AuthorizationHandler<PermissionsRequirement, Microsoft.AspNet.Mvc.Filters.AuthorizationContext>
{
    private readonly IPermissionService _permissionService;
    public PermissionsHandler(IPermissionService permissionService)
    {
        _permissionService = permissionService;
    }

    protected override void Handle(Microsoft.AspNet.Authorization.AuthorizationContext context, PermissionsRequirement requirement, Microsoft.AspNet.Mvc.Filters.AuthorizationContext resource)
    {
        var userRoles = context.User.FindAll(ClaimTypes.Role);
        var permissionsAttribute = resource.ActionDescriptor.FilterDescriptors.Select(f => f.Filter).OfType<PermissionsAttribute>().SingleOrDefault();

        if (permissionsAttribute == null)
        {
            return;
        }

        if (_permissionService.RolesHavePermission(userRoles, permissionsAttribute.Permission))
        {
            context.Succeed(requirement);
        }
    }
}

public interface IPermissionService
{
    bool RolesHavePermission(IEnumerable<Claim> userRoles, string permissions);
}

[Authorize("Permissions")]
public class SomethingController
{
    [Permissions("Student")]
    [HttpGet("/")]
    public string Get()
    {
        return "Hello World";
    }
}

// Marking this attribute with IFilterMetadata makes it accessible on MVC primitives
public sealed class PermissionsAttribute : Attribute, IFilterMetadata
{
    public string Permission { get; }

    public PermissionsAttribute(string permission)
    {
        Permission = permission;
    }
}

[davidfowl]

Tiny improvement to get rid of the auth policy and Authorize attribute altogether:

public class PermissionsAuthorizationFilter : AuthorizeFilter
{
    public PermissionsAuthorizationFilter() : base(GetPolicy())
    {
    }

    private static AuthorizationPolicy GetPolicy()
    {
        var policy = new AuthorizationPolicyBuilder()
                .AddRequirements(new PermissionsRequirement())
                .Build();

        return policy;
    }

    public override Task OnAuthorizationAsync(Microsoft.AspNet.Mvc.Filters.AuthorizationContext context)
    {
        // Only check if there's a permissions attribute on the action
        if (context.ActionDescriptor.FilterDescriptors.Any(fd => fd.Filter is PermissionsAttribute))
        {
            return base.OnAuthorizationAsync(context);
        }

        return Task.FromResult(0);
    }
}

And Startup.Configure can be changed to:

services.AddMvc(options =>
{
    options.Filters.Add(new PermissionsAuthorizationFilter());
});

services.AddSingleton<IAuthorizationHandler, PermissionsHandler>();
services.AddSingleton<IPermissionService, PermissionService>();

[blowdart]

To be fair, creating a policy per permission is just as much work as adding a parameter onto each attribute.

[davidfowl]

To be fair, creating a policy per permission is just as much work as adding a parameter onto each attribute.

Possibly, it depends on what data needs to be round tripped to the Handler. You can't really pass any data besides the policy name itself. If permissions was a flags enum for example you couldn't do the same thing.

[blowdart]

That's not true at all. Requirements can have parameters. That's how you pass them down. So you'd have a general permissions parameter which takes data in the constructor, and then that is passed as the requirement parameter into Handle.

[davidfowl]

You'd have to enumerate all permissions up front and add a policy for each with the same name:

Startup.ConfigureServices

services.AddAuthorization(options =>
{
    foreach (var permission in GetPermissions())
    {
        options.AddPolicy(permission, builder =>
        {
            builder.AddRequirements(new PermissionRequirement(permission));
        });
    }    
});

public class PermissionsRequirement : IAuthorizationRequirement
{
    public string Permissions { get; private set; }

    public PermissionsRequirement(string permissions)
    {
        Permissions = permissions;
    }
}

public class PermissionsHandler : AuthorizationHandler<PermissionsRequirement>
{
    private readonly IPermissionService _permissionService;
    public PermissionsHandler(IPermissionService permissionService)
    {
        _permissionService = permissionService;
    }

    protected override void Handle(Microsoft.AspNet.Authorization.AuthorizationContext context, PermissionsRequirement requirement)
    {
        var userRoles = context.User.FindAll(ClaimTypes.Role);

        if (_permissionService.RolesHavePermission(userRoles, requirement.Permissions))
        {
            context.Succeed(requirement);
        }
    }
}

[davidfowl]

That matches our pattern but does feel a bit cumbersome having to turn your permissions into a concrete class in order to round trip state. If you were trying to represent ACLs for example would you model it that way?

[blowdart]

ACLs are a strange thing, and don't map to web permissions easily. Instead you'd probably see a claim like "DocumentPermissions" with a CRUD value which spans the library, and then you'd use resource based auth to further drill in, just like a file ACL acts on a file, not a path.

[blowdart]

Just like you enumerated all the permissions up front and put an attribute for each. It's much the same amount of work!

[davidfowl]

What if we turned this:

https://github.com/aspnet/Security/blob/dev/src/Microsoft.AspNetCore.Authorization/AuthorizationOptions.cs#L9 into a Policy provider (like Cors has) and the default implementation would be based on the policy map, but in this case it could be implemented by making permissions based on whatever data source you were using.

public interface IAuthorizationPolicyProvider
{
  Policy GetPolicy(string name);
}

For reference https://github.com/aspnet/CORS/blob/dev/src/Microsoft.AspNetCore.Cors/ICorsPolicyProvider.cs.