dotnet / aspnetcore

ASP.NET Core is a cross-platform .NET framework for building modern cloud-based web applications on Windows, Mac, or Linux.
https://asp.net
MIT License
35.23k stars 9.95k forks source link

How can i manually execute model binding for a AuthorizationFilterContext #13701

Closed taori closed 5 years ago

taori commented 5 years ago

Is your feature request related to a problem? Please describe.

I am trying to utilize IAsyncAuthorizationFilter to execute custom validation logic before granting authorization to objects.

However within the scope of this filter, ModelBinding did not happen yet, i assume because of authorization not being done yet.

I there any way to manually execute ModelBinding on that context so i can read the object from ModelState?

blowdart commented 5 years ago

We perform the authorization pieces before model binding on purpose, in case someone does something weird like bind to an EF backed model. If we bound first, or within, then you could have database access for an unauthorized or unauthenticated user.

If you're trying to make decisions based on the resource you would end up loading, you're doing resource authorization, and we can support that, but it's imperative, not via attributes.

taori commented 5 years ago

Hmmmm. What you're suggesting sounds like it does indeed cover my use case without excessive effort.

There is one issue with it for me though. It requires me to insert authorization logic into every action instead of keeping it at a minimum.

I'll elaborate a little bit:

This is my current code:

    public abstract class PermissionAttributeBase : Attribute, IAsyncAuthorizationFilter
    {
        /// <inheritdoc />
        public async Task OnAuthorizationAsync(AuthorizationFilterContext filterContext)
        {
            if (filterContext.ActionDescriptor is ControllerActionDescriptor controllerActionDescriptor)
            {
                if (controllerActionDescriptor.MethodInfo.IsDefined(typeof(AllowAnonymousAttribute)))
                    return;

                if (!await IsAuthorizedAsync(new PermissionContext(filterContext, controllerActionDescriptor)))
                    filterContext.Result = new ForbidResult();
            }
        }

        protected abstract Task<bool> IsAuthorizedAsync(PermissionContext permissionContext);
    }

    public class PermissionContext
    {
        public AuthorizationFilterContext FilterContext { get; }
        public ControllerActionDescriptor ControllerActionDescriptor { get; }

        public PermissionContext(AuthorizationFilterContext filterContext, ControllerActionDescriptor controllerActionDescriptor)
        {
            FilterContext = filterContext;
            ControllerActionDescriptor = controllerActionDescriptor;
        }
    }

    public enum TypePermissionKind
    {
        Overview,
        Read,
        Create,
        Modify
    }

    public interface ITypePermissionGuard
    {
        Task<bool> IsAuthorizedAsync(Type type, TypePermissionKind kind);
    }

    public class TypePermission : PermissionAttributeBase
    {
        public Type Type { get; }

        public TypePermissionKind Kind { get; }

        public TypePermission(Type type, TypePermissionKind kind)
        {
            Type = type;
            Kind = kind;
        }

        protected override async Task<bool> IsAuthorizedAsync(PermissionContext permissionContext)
        {
            var authorizers = permissionContext.FilterContext.HttpContext.RequestServices.GetRequiredService<IEnumerable<ITypePermissionGuard>>();
            foreach (var guard in authorizers)
            {
                if (await guard.IsAuthorizedAsync(Type, Kind))
                    return true;
            }

            return false;
        }
    }

Regarding the TypePermission i could achieve the same result using default requirements - for what i am now trying i would indeed be forced to use resource authorization in order to get a somewhat clean approach of solving this scenario.

There are 2 key factors i want to achieve within my application

If model binding were to be available at this point i could solve it the way i started it, similar to resources but using the models i pull from the request - your implications about all the funky stuff that could happen if model binding were to happen by default make sense - but is there a way to make model binding happen on demand?

The issue i am having is the following: The current model requires me to write lots of boilerplate code at the call site to get the desired authorization. Taking your link as example i would want to write code like this:

[ResourceLocator(typeof(Document), "documentId")]
[AuthorizeCrudResource(typeof(Document), Operations.Read)]
public async Task<IActionResult> OnGetAsync(Guid documentId)
{
        return Page();
}

[ResourceLocator(typeof(Document), "documentId")]
[ResourceLocator(typeof(Document2), "documentId2")]
[AuthorizeCrudResource(typeof(Document), Operations.Read)]
[AuthorizeCrudResource(typeof(Document2), Operations.Read)]
public async Task<IActionResult> OnGetAsync(Guid documentId, Guid documentId2)
{
        return Page();
}

not this

public async Task<IActionResult> OnGetAsync(Guid documentId)
{
    Document = _documentRepository.Find(documentId);

    if (Document == null)
    {
        return new NotFoundResult();
    }

    var authorizationResult = await _authorizationService
            .AuthorizeAsync(User, Document, Operations.Read);

    if (authorizationResult.Succeeded)
    {
        return Page();
    }
    else if (User.Identity.IsAuthenticated)
    {
        return new ForbidResult();
    }
    else
    {
        return new ChallengeResult();
    }
}

Don't get me wrong. I think authorization in netcore really is great now, better than any previous mvc stage, but in non baseline cases where Roles are enough already, the code required at call site quickly becomes very verbose once your authorization demands go beyond ordinary use cases. If that boilerplate were to disappear from resource based authorization at call site i think that's just as much as anyone could ever ask from it.

After all any authorization i can imagine right now is in someway described as "user x wants to perform operation y on subject z".

blowdart commented 5 years ago

Well, in any action where there's a resource to check, yes, but it's safer, because, as I said, people bind to EF models and you don't want that to happen unauthenticated, which is what would happen in filters and attributes.

@rynowak @HaoK may have better ideas, but nothing we have now would safely support your scenarios.

rynowak commented 5 years ago

is there a way to make model binding happen on demand?

Unfortunately no. There's no imperative way to call model binding.

taori commented 5 years ago

@blowdart I suppose your concern is about if model A contains a propery for a collection of entities B? Assuming you were thinking about that would still pose an issue if i were to verify the models with resource authorization, which would then pose a general issue for both kind of authorization approaches. Furthermore it would produce more boilerplate code to restrict resources then.

Then again is EF really an issue there? Because the models you get from model binding don't have the required db context to set up their property load proxies? Getting models which can actually execute queries would require the model to be a datacontext itself, because a model built from query/body/whatever cannot possibly exist without there being a serious security flaw in the architecture itself, because only objects built from a db context are capable of that? correct me if i am wrong here.

Discard this if any of my assumptions are wrong, but if i wasn't there would be a benefit for this?

I think if it were to exist it would be possible to build the resources from given parameters and even apply those rules to the properties you originally attempted to authorize.

I think it's probably best if i drop it here though. There is also the issue where one would want to authorize a resource and the resource it contains and deny authorization if not all the resources are authorizes, or pass authorization if any are granted and discard the unauthorized data. That sort of points out how if authorization can be so deeply specific it is probably right for a framework not to bother you and just let you write the details in your own business code.

Thanks for the enlightenment.

@rynowak Thanks for checking.

Feel free to close this one. I suppose making model binding invokable at authorization time would not be worth the work and considerations required to offer it.

blowdart commented 5 years ago

I'm being a little unfair to EF here, but it's our main example if why binding has to happen after authentication. Given there is resource auth, albeit manual, you have a supported way to do it.

So yes, closing. It's a lovely idea, but not one we can do safely.