DavidParks8 / Owin-Authorization

Backport of Asp.Net core's policy based authorization to Asp.Net 4
Other
60 stars 16 forks source link

How to intercept authorization failure? #56

Closed seangwright closed 5 years ago

seangwright commented 6 years ago

I would like to have a hook point into the end of the authorization process.

One option that isn't possible at the moment is to inherit from DefaultAuthorizationService and have something like

public class LoggingAuthorizationService : DefaultAuthorizationService
{
    // Inject any other dependencies for contextual logging here
    public LoggingAuthorizationService(
        IMyLoggingService loggingService,
        IMyNotificationService notificationService,
        IAuthorizationPolicyProvider policyProvider, 
        IEnumerable<IAuthorizationHandler> handlers) : base(policyProvider, handlers)
    {
    }

    public override async Task<bool> AuthorizeAsync(ClaimsPrincipal user, object resource, IEnumerable<IAuthorizationRequirement> requirements)
    {
        bool isAuthorized = await base.AuthorizeAsync(user, resource, requirements);

        // log here

        return isAuthorized;
    }
}

I could then register my LoggingAuthorizationService implementation with my DI container (Simple Injector) and have all the information I need to log when a request is not authorized.

But since AuthorizeAsync is not virtual I cannot use the above approach.

Do you have a recommended approach for this?

The reason I would like to have a hook point is that currently I'm using my Global Exception Handler as a central logging point for collecting 400, 401, 403, 404 and 500 responses and ensuring these failed requests along with their request context is logged out to Application Insights and to my logging infrastructure via Serilog.

This project's approach is a really nice solution for authorization but it bypasses all of that centralization.

Also, I'm not sure how to customize the response message, which is "Authorization has been denied for this request" and the response status code is 401, which is correct for being un-authenticated, but not un-authorized (which should be a 403). I know that ASP.NET has these mixed up and returns 401 for failed authZ checks but there should be a way to correct this.

Details mentioned by Dominick at https://leastprivilege.com/2014/10/02/401-vs-403/ and more details here https://github.com/aspnet/Security/issues/720#issuecomment-190528294 concerning the wrong status codes in ASP.NET 4 and the need to overwrite them.

DavidParks8 commented 6 years ago

Ultimately, the mechanism which writes the response when using Owin-Authorization belongs to the underlying framework (web api or MVC). That is why you see the default behavior of returning a 401 when a 403 may be more appropriate.

The recommended approach to log something custom would be to inherit from and augment the ResourceAuthorizeAttribute.

public class MyCustomLoggingResourceAuthorizeAttribute : ResourceAuthorizeAttribute
{
    // setup property injection with your DI framework to inject the logger
    public ILog Log { get; set; }

    protected override bool IsAuthorized(HttpActionContext actionContext)
    {
        bool isAuthorized = base.IsAuthorized(actionContext);
        // log here
        return isAuthorized;
    }
}

The same method is recommended for returning a custom error message/different status code. Inherit from and augment the ResourceAuthorizeAttribute and override the HandleUnauthorizedRequest method that is provided by the framework.

While 403 is often the correct status code, this library is sometimes used for authentication and authorization in which case altering the default asp.net behavior would amount to a breaking change. By not touching the default behavior, and instead leaving overrides up to the library consumer, we give more familiarity and control over what response is produced. I want to make it easier for people to do complex authorization in older asp.net frameworks, but if everything is fixed, what reason would people have to move to asp.net core? :)

seangwright commented 6 years ago

Hmm... inheriting from ResourceAuthorizeAttribute forces me to use non-DI ways of getting access to services (Service Locator?) because attributes are created once and cached for the lifetime of the application. Any services I would inject through a constructor would become captured dependencies/singletons.

I suppose I could do something like this to check if the user is authenticated and breakout of the framework pipeline to my GlobalExceptionHandler.

public class AuthorizeAccessAttribute : ResourceAuthorizeAttribute
{
    protected override bool IsAuthorized(HttpActionContext actionContext)
    {
        bool isAuthorized = base.IsAuthorized(actionContext);

        bool isAuthenticated = (actionContext.RequestContext.Principal?.Identity?.IsAuthenticated ?? false);

        if (isAuthenticated && !isAuthorized)
        {
            throw new NotAuthorizedException("Resource ...");
        }

        return isAuthorized;
    }
}

Is there any collection available of the specific Requirements that failed?

Assuming there are multiple requirements to be authorized for a request, then isAuthorized above being false would indicate that all requirements failed to pass. It would be great for logging or (if it doesn't leak security information) messaging back to the client what those were.

I also would prefer to move to ASP.NET core :) :) ... but I'm working within my constraints atm.

Also I really appreciate all the work you've done backporting this policy authorization infrastructure.

Thanks for the suggestions!

DavidParks8 commented 6 years ago

ActionFilters/authorizationfilters have a different lifetime in asp.net than normal attributes. Though you and I see attributes, asp.net sees an IAuthorizationFilter to instantiate in a custom way so that it can be used in the request/response pipeline.

What I suggest is to use property injection (an alternative to constructor injection) for the authorization attributes and not to use the service locator pattern. Take a look at the simple injector property injection docs.

There is no collection available with the specific failed requirements because there are some internal classes which could be exposed. What I have done in the past to return specific error messages about what authorization step failed is make small/granular authorization policies. Then, you can apply multiple authorization attributes to an action and change the error response body based upon which policy is the one for which IsAuthorized returns false.

DavidParks8 commented 6 years ago

More specifically, what I mean by granular policy messaging is something like the following:

    public class ResourceAuthorizeWithMessageAttribute : ResourceAuthorizeAttribute
    {
        private readonly string _responseMessage;

        public ResourceAuthorizeWithMessageAttribute(string policyName, string responseMessage)
        {
            Policy = policName;
            _responseMessage = responseMessage;
        }

        protected override void HandleUnauthorizedRequest(HttpActionContext actionContext)
        {
            actionContext.Response = actionContext.ControllerContext.Request.CreateErrorResponse(
                HttpStatusCode.Forbidden,
                _responseMessage);
        }
    }

Then you could make a bunch of simple attributes for various policies like this one:

    public class RequireNumberAttribute : ResourceAuthorizeWithMessageAttribute
    {
        public RequireNumberAttribute()
           : base("The name of one of your policies", "Number is required")
        {
        }
    }

If you need localized errors, some alterations to this would have to be made, but I think this small example gets the point across on how each policy could have its own message.