Simplify writing custom authorization attributes

[danroth27]

As originally reported by @nathan-alden in #5532:

The forced authentication abstraction (requirements, policies) eliminated a very useful shared custom authorization attribute I had written that allowed this: [CustomAuthorize(Operator.And, Permission.Create, Permission.Read)]. My attribute allowed for a single authorization algorithm to be implemented (within the attribute's code) that would check various JWT claims, with each action method choosing what parameters fed into that algorithm.

See also http://stackoverflow.com/questions/31464359/custom-authorizeattribute-in-asp-net-5-mvc-6/31465227

[frankabbruzzese]

I read the whole discussion on StackOverflow, and frankly, I dont undestand the polemic. Why having all conditions hardwired in the code within an AuthorizeAttribute should be more flexible than defining dynamically requirements that might take their data from a DB for instance?

With the new approach you have just to group all action methods having the same permissions in equivalence classes and for each such class you need to create a policy. This is the only stuff that will remain hardired in the code. In the worst case (impossible in my opinion) you will have a policy for each action method, not for all permutations of requirements.

Everything else may be changed dynamically.

Obviously one doesn't need to create a different Requirement class for each policy, since each class may have several parameters. Since each policy is an implict "and" of several requirements, one might object there is no way to have an "Or" of existing requirements types. However this is false since you might create an OrRequirement that accepts a list of requirements and is satidfied if at least one requirement is satisfied.

Try to switch the way you think to policies. Everything appears logic if one thinks to policies as equivalence classes of action methods not as sets of requirements. Where several action methods belong to the same equivalence class iff they have same requirements, or better if there is a reason implicit in the business rules why they MUST have the same requirements..

In other terms policies are an abstraction over "actions" not over "requirements".

[davidfowl]

One of the big reasons is the fact that the attribute doesn't scale past the mvc controller action. We're trying to move the actual meat of the authorization logic into an imperative service that can be called from anywhere in application code across frameworks and non-action callsites. When SignalR is done for example, it should be possible to both use the same attribute and logic. Baking mvc specific logic goes against that.

The biggest pain point is the fact that it's hard to flow state from the callsite (in this case the attribute) to your authorization logic. In mvc the resource policy is an mvc specific context, not much more you can flow from that beside the policy name itself.

[derekgreer]

With the new approach you have just to group all action methods having the same permissions in equivalence classes and for each such class you need to create a policy.

That really is the issue ... policies are great for defining discrete types of authorization needs that are computed dynamically (e.g. options.AddPolicy("CanBuyAlcohol", policy => policy.Requirements.Add(new Authorization.MinimumAgeRequirement(21))); ), but the current implementation seems limited for variations of the same class of requirement.

The bottom line is, people want the ability to define a claim type (e.g. "http://schemas.mycompany.com/ws/2016/12/identity/claims/permission") with different values for separate classes of actions ("CanManageOrders", "CanManageUsers", etc.) and declare an attribute which indicates a requirement for the specific claim (e.g. [Authorize(Policy ="HasClaim", ClaimType = MyClaimTypes.Permission, "CanManageOrders")] or better [Permission("CanManageOrders")]
) without having to register a new policy for each value of MyClaimTypes.Permission.

Sure, if your application has 450 different types of permissions that can be assigned to a given user then you can have a Startup.cs which has something like the following:

options.AddPolicy("CanDoOperation001", policy => policy.Requirements.Add(new Authorization.ClaimRequirement(MyClaimTypes.Permission, "CanDoOperation001")));
options.AddPolicy("CanDoOperation002", policy => policy.Requirements.Add(new Authorization.ClaimRequirement(MyClaimTypes.Permission, "CanDoOperation002")));
options.AddPolicy("CanDoOperation003", policy => policy.Requirements.Add(new Authorization.ClaimRequirement(MyClaimTypes.Permission, "CanDoOperation003")));
options.AddPolicy("CanDoOperation004", policy => policy.Requirements.Add(new Authorization.ClaimRequirement(MyClaimTypes.Permission, "CanDoOperation004")));
...
options.AddPolicy("CanDoOperation450", policy => policy.Requirements.Add(new Authorization.ClaimRequirement(MyClaimTypes.Permission, "CanDoOperation450")));

... but I hope you can see how developers are seeing an issue with this approach.

[frankabbruzzese]

With the new policy approach one should adopt a different perspective. You look at all policies as a "bag" of predefined authorizations each action method designer must choose one.

The right perpspective is that there is no predefined "bag" of policies, but policies are designed together with action methods. Namely, each time you define a new "action type" you define a policy name for it where, "action types" are an abstraction over action methods. For instance, suppose you have an ERP where "user actions" are classified depending on the "area" they affect (marketing, strategic planning, etc). For instance you might put in the same class all actions that compute values based on the company balance sheet, since for sure they'll have the same permissions. Now you may deine a policy for each "area" and for each action type (read, update, create, delete). So for instance you'll have a policy called: "BalanceSheetRead" that will be applied to all action methods that show values computed from the "official" company balance sheet.

Thus, policies names are decided in a way that is completely independent from the claims defining them: they are created starting from action methods classification. So in the worst case (pratically impossible) you will have a different policy for each action method which is completely equivalent to putting all claims in the autorize attribute itself, with the difference that now you may modify policy definitions without recompiling the code, since permission definition is not harwired in action method definition.

Policies may be defined in an ad hoc class that is called in the startup class and this class may read all definitions from an xml, or json file, or from a database.

[derekgreer]

I believe I understand the intent of the policy design and it seems to provide a suitable solution for complex authorization needs. What it doesn't do is provide a convenient solution for simple authorization needs where there is a one-to-one mapping between a claim and what you are wanting people to define as a "policy". I would submit that 80% of customers simply need a way to easily express that a given action requires a given claim. In this case, the claim IS the abstraction. Sure, creating a policy named "BalanceSheetRead" provides a physical abstraction from a claim type of "http://.../claims/permission" with a value of "BalanceSheetRead", but when you're never going to need that level of abstraction, it's unnecessary complexity.

Perhaps it's the message, rather than the solution that is in need of adjustment. The message being sent out from the ASP.Net Core team and those advocating acceptance of this design is: "You shouldn't be implementing your own Authorize attribute" (i.e. "You're doing it wrong if you are"). It seems one of the goals for the new design was to provide an out-of-the-box solution for those writing custom solutions, but it still seems more sensible to use a solution like the following when the overhead of creating policy abstractions isn't needed:

    public class ClaimRequirementAttribute : TypeFilterAttribute
    {
        public ClaimRequirementAttribute(string claimType, string claimValue) : base(typeof(ClaimRequirementFilter))
        {
            Arguments = new object[] {new Claim(claimType, claimValue) };
        }
    }

    public class ClaimRequirementFilter : IAsyncActionFilter
    {
        readonly Claim _claim;

        public ClaimRequirementFilter(Claim claim)
        {
            _claim = claim;
        }

        public async Task OnActionExecutionAsync(ActionExecutingContext context, ActionExecutionDelegate next)
        {            
            var hasClaim = context.HttpContext.User.Claims.Any(c => c.Type == _claim.Type && c.Value == _claim.Value);
            if (!hasClaim)
            {
                context.Result = new UnauthorizedResult();

            }
            else
            {
                await next();
            }
        }
    }

    [Route("api/resource")]
    public class MyController : Controller
    {
        [ClaimRequirement(MyClaimTypes.Permission, "CanReadResource")]
        [HttpGet]
        public IActionResult GetResource()
        {
            return Ok();
        }
    }

[rynowak]

/cc @HaoK @blowdart

[frankabbruzzese]

@derekgreer ,
Now I understand your point. We should need at least an out of the box ClaimRequirement attribute, for the simplest cases. I agree.

[HaoK]

So we do have an extensibility point for this today in the PolicyProvider, granted its not as polished as I'd like for something like this scenario:

        public class DynamicPolicyProvider : IAuthorizationPolicyProvider
        {
            public Task<AuthorizationPolicy> GetDefaultPolicyAsync()
            {
                return Task.FromResult(new AuthorizationPolicyBuilder().RequireAuthenticatedUser().Build());
            }

            // policyName = encoded claims type/values, i.e. "Permission:Read+Write"
            public Task<AuthorizationPolicy> GetPolicyAsync(string policyName)
            {
                return Task.FromResult(new AuthorizationPolicyBuilder().RequireClaim(decodePolicy(policyName)).Build());
            }
        }

                services.AddSingleton<IAuthorizationPolicyProvider, DynamicPolicyProvider>();
                services.AddAuthorization(options => { });

The general problem that exists today is the policyName is the only thing that really flows from the Authorize attribute to the AuthZ service...

[derekgreer]

Yes, parameters could be encoded within the policy name, but surely this wouldn't be the team's recommended way of addressing this issue. I think the best answer at this point really is: The current infrastructure isn't suited for simple claim-based authorization, so we recommend creating custom Action Filters for such needs.

[frankabbruzzese]

@derekgreer ,
After some thinking about the problem I realized that "simple cases" you are referring to might use roles. Written this way it appears kinda dirty hack but actually isn't. In fact, in simple cases where the Authorization attribute simply needs just one claim, this claim usually doesnt map to any "immediate user property" (such ase age....or similar) but is already the result of some processing performed on the user data. In a few word such claim has been computed "ad hoc" to perform authorization. Thus, it is kinda user grouping for authorization purpose, ie a role!

As a matter of fact the main advantage of claims is that one may define authorization criteria depending on several "native" user properties. If one doesn't use several "native" user properties it is simple using roles! On the other side if one needs a complex user property processing the simplest way is "Policy".

In both cases we avoid to hardcode specific property values next to the action methods, but, instead, we put "identifier" that are computed somehow from such values, thus decoupling the overall authorization logic, from specific property values. In the case of roles, they are computed either at user registration time, or when the auth cookie is created by using user data. In the case of policies they are computed in each policy record.

[derekgreer]

I understand the appeal of wanting the new policy design to be applicable for all authorization cases, but in practice it simply doesn't satisfy a large use case eloquently. It might be helpful to step back from the implementation and consider the concepts we are talking about at a conceptual level.

A Role is a designation for a person or thing which represents a grouping boundary around one or more expected set of behaviors or operations (e.g. CEO, CFO, Director, Manager, Driver, Accountant, etc.).

A Policy is a governance rule which describes an established set of behavior or boundaries for operation (e.g. "Employees must wash hands", "You must be 18 to vote in the United States", etc.). Policies may be based upon "native" properties for those to whom they apply (e.g. "You must be 4 feet tall for this ride"), or they can be based upon non-native properties (e.g. "You must have a wrist-band to enter the theme park".)

A Permission is a license to perform a particular action (e.g. "Can Approve Vacation Requests", "Can Order Supplies", etc.). Permissions imply the presence of one or more Policies.

A Claim is a piece of information about a user which may be a native or non-native property.

Each of these concepts are related to Authorization, but in different ways. Roles can be used for authorization because they are associated with an implied set of permissions and satisfy an implied set of policies, but the course-grained nature of Roles only lends themselves to simple authorization needs (e.g. "User" vs. "Administrator").

Policies are great introduction to the .Net framework, but due to the one-to-one relationship between a Permission ("You may delete orders") and a Policy ("Those with permission to delete orders may delete orders"), their use is superfluous.

So, all Policies based upon non-native properties are not Roles.

Here's really what it comes down to: Microsoft has many customers who have requirements for modeling authorization around the concept of permissions. The new authorization infrastructure doesn't support the use of permissions as the primary means of controlling authorization without ceremony.

[nathan-alden-sr]

There's nothing left to be said by me that others haven't already said. My original complaint was that ASP.NET Core removed/replaced useful abstractions that allowed for flexible designs in Web API 2. The new policy system is an example of catering to the 20%, not the 80%. We are supposed to accept magic strings (i.e., the policy name appearing in an attribute) in place of custom authorization attributes that were refactoring-friendly (see my example at the top). The design of the authorization system seems to force one of three paths: 1) a complete re-implementation of authorization using filters or middleware, 2) needless class bloat (basically, almost a 1:1 between action method and policy), or 3) the selection of a competing framework (in my case, NancyFx) that has friendlier pipeline abstractions.

[frankabbruzzese]

@nathan-alden , I dont see any difference between the magic strings of policy names and the magic strings of Claims name. Defining policies with full .Net code power for sure is more flexible that putting them into an AuthorizeAttribute that may accepts just strings.

My only complaint is that one can't add several Policies separated by commas like for Roles. This from one side forces a better abstraction but on the other side somehow prevent incremental implementation of complex systems since incremental changes might break the Action Method equivalence classes classification (ie classes with the same policy).

[nathan-alden-sr]

Enums are not magic strings. My custom attribute hid the underlying serialization logic from developers working on action methods. The enums are discoverable by IntelliSense and ReSharper. Changes to the shared package that contains the attribute and the enums will bubble up as compiler errors should developers consume a non-backward-compatible new version of the shared package.

[frankabbruzzese]

@nathan-alden , in the general case you cant avoid magic strings usage in the Authorize attribute. True, if you have just one Claim per attribute with a primitive type value you can avoid string, but what if you need and/or combination of claims (which is quite common)? Attributes do not allow complex structures so either you put the whole and/or expression in a string or you use something like "Policies" ie place the complex expression elsewhere an just put its "name" in the Attribute.

Moreover, imagine you somehow place Claims expressions in attributes. What if you need to change authorization Criteria? You need to review ALL attribute instances manually! On the contrary, if you use policies, you just review policies (which should be sensibly less than attribute instances), or maybe just one policy. Then all Action methods using it will work properly.

That said I agree that current implementation need to be improved. Namely:

1. Possibility to provide several policies within the same attribute (whose semantic is a lofical or). This way like in case of Roles we may specify alternative authorization "reasons". In fact the same action may be allowed because it belongs to different partially overlapping set of user actions. True that overlapping sets may be transformed into not overlapping sets by computing all possible intersections, but this would lead to a combinatorial explosion.

2. Simplified syntax for the single claim simple case, ie avoiding policies definition and placing it directly in the attribute.

[nathan-alden-sr]

My original post addressed that. My attribute's constructor was declared like this:

public AuthorizationAttribute(Operator @operator, params Permission[] permissions)
...
public enum Operator
{
    And,
    Or
}

This handled authorization rules for most of my endpoints. For anything more custom than that, I had another constructor that takes a Type implementing a specific interface. The interface would be passed the set of claims and allowed to do whatever checks it needed.

If I need to change authorization criteria, then I need to change it; the only thing to argue is where the changes will occur. It is very unlikely that the meaning of a value in my Permission enum will change; it is far more likely that a particular action method will simply require a different set of Permission values for its authorization. The change is much closer to the action method driving that change when I am able to use attributes, rather than hidden in some completely disconnected policy class.

[frankabbruzzese]

@nathan-alden ,
public AuthorizationAttribute(Operator @operator, params Permission[] permissions) is smart, but you cant specify claims values either, just claims names. For sure, it is better than standard string based roles but doesnt add expressive power to it, just a better implementation. It doesnt go in the direction of declarative rules based on native user properies (represented by claims).

Your more general solution of implementing an interface to define complex and/or combinations improve expressibe power but appears heavier than the Policy solution. Sincerely, I prefer the Policy solution where each and/or combination may be defined with a lambda, ie with the minimum code required to define the and/or combination, also if it has the disadvantage of relying on "magic strings".

In any case an extension point for customizing Authorization in a not trivial way should be provided.

[SocVi100]

@derekgreer I don't have enough words to express how right you're about this issue (I could upvote your comments one million times).
Please, to the people developing the asp.net core authorization system: Don't force us to code one requeriment per permission, IT'S A COMPLETE OVERKILL. Please, provide a way to pass parameters to the Authorize attribute...

@derekgreer The example of ClaimRequirementAttribute you shown above, is functional code or just pseudocode to expose the functionality?

[derekgreer]

Yes, the ClaimRequirementAttribute code should be functional.

[rynowak]

@danroth27 @HaoK - I'm not sure how to sum up the issues here, but this really seems like it's not an MVC concern.

The currency that MVC operates on is IAuthorizeData - https://github.com/aspnet/Security/blob/dev/src/Microsoft.AspNetCore.Authorization/IAuthorizeData.cs It sounds like in order to satisfy some of the requests that are being discussed here, you'd have to have a different metadata interface. Then we could teach MVC how to deal with this new API.

What do y'all intend to do about this?

[HaoK]

@blowdart do you want to migrate this / xform this issue into something in security? Its kinda related to aspnet/Security#917

[blowdart]

Not really, no. It's Dan's issue :)

[davidfowl]

It's not. The problem is that you can't flow context through. I don't think we should build in anything first class for permissions, we should try to address the issue generally so that other frameworks (like SignalR) get the benefit.

[HaoK]

AuthZ improvements have been consolidated into this issue: https://github.com/aspnet/Security/issues/1359

[bugproof]

Found this issue here: https://stackoverflow.com/questions/31464359/how-do-you-create-a-custom-authorizeattribute-in-asp-net-core

@derekgreer is perfectly right, it would be great if there was ClaimRequirementAttribute available or something similar.

Current usage of policies is pathological I'd say. Having to create policies for each claim (one-to-one mapping) is really an overkill

options.AddPolicy("CanUpdateOrder", policy => policy.RequireClaim(MyClaimTypes.Permission, "CanUpdateOrder));)

I'm not surprised many people aren't happy with this.

Quoting @blowdart from here https://stackoverflow.com/a/31465227/2828480

We don't want you writing custom authorize attributes. If you need to do that we've done something wrong. Instead you should be writing authorization requirements.

yes you did.

Also using strings everywhere without constants is bad "CanUpdateOrder", so probably the best thing you can do is something like:

public static class PermissionNames
{
    public const string CanUpdateOrder = nameof(CanUpdateOrder);
}

and then [Authorize(PermissionNames.CanUpdateOrder)]

and options.AddPolicy(PermissionNames.CanUpdateOrder, policy => policy.RequireClaim(MyClaimTypes.Permission, PermissionNames.CanUpdateOrder));)

[JoshClose]

This is a much more simple solution. Thanks for sharing.

For me to do an activity based auth, just like the answer you linked to, I created an AuthorizationHandler<IAuthorizationRequirement>, an IAuthorizationPolicyProvider, an IAuthorizationRequirement, and an AuthorizeAttribute. I also created a manager class so I could do things like IsAuthorizedForActivity(string activity).

It's a lot of code just to do a simple query.