alexbeletsky / elmah-mvc

Painless integration of ELMAH into ASP.NET MVC application
http://nuget.org/packages/Elmah.MVC
Apache License 2.0
266 stars 61 forks source link

Interface for injecting custom authorization #24

Closed Mellbourn closed 2 months ago

Mellbourn commented 11 years ago

I added an interface so that you can inject your own authorization implementation. We needed this in our project since we were using custom authorization code.

How to use it: 1) Create class implementing the IAuthorizerForElmah interface, which is just the single method IsAuthorizedForElmah(username). In this method you can write your own code for determining whether this user is authorized to use elmah. 2) Inject the class using the Dependency Injection framework you are using in your project. (Elmah.mvc will find the class using the DependencyResolver)

alexbeletsky commented 11 years ago

This is brilliant idea, Klas!

I'll merge that up (probably change some naming),document and push new version soon. Thanks a lot for support.

Mellbourn commented 11 years ago

Great! Please feel free to change the naming.

alexbeletsky commented 11 years ago

Merry Christmas, Klas

I was working on the changes from you and basically came up with idea.

By implementing that idea, we can get same behavior as you expecting but without any code changes to Elmah.MVC. I'm talking about global filters.

Here is the code,

namespace ElmahMvcTest
{
    public class ElmahAuthorizationFilter : IAuthorizationFilter
    {
        public void OnAuthorization(AuthorizationContext filterContext)
        {
            if (filterContext.Controller is ElmahController)
            {
                // do whatever check you want here..
                filterContext.Result = new HttpUnauthorizedResult();
            }
        }
    }

    public class FilterConfig
    {
        public static void RegisterGlobalFilters(GlobalFilterCollection filters)
        {
            filters.Add(new ElmahAuthorizationFilter());
            filters.Add(new HandleErrorAttribute());
        }
    }
}

So, it's possible to add global authrozation filter that could check, is authorization is done for ElmahController and apply any checks you want there. In this scenario, I'm blocking all users to access /elmah endpoint.

Does it make sense to you?

Mellbourn commented 11 years ago

Merry Christmas Alexander :)

Sure, it makes sense. Both versions work. I guess what you prefer is a matter of taste.

Klas

On Wed, Dec 26, 2012 at 2:46 PM, Alexander Beletsky < notifications@github.com> wrote:

Merry Christmas, Klas

I was working on the changes from you and basically came up with idea.

By implementing that idea, we can get same behavior as you expecting but without any code changes to Elmah.MVC. I'm talking about global filters.

Here is the code,

namespace ElmahMvcTest{ public class ElmahAuthorizationFilter : IAuthorizationFilter { public void OnAuthorization(AuthorizationContext filterContext) { if (filterContext.Controller is ElmahController) { // do whatever check you want here.. filterContext.Result = new HttpUnauthorizedResult(); } } }

public class FilterConfig
{
    public static void RegisterGlobalFilters(GlobalFilterCollection filters)
    {
        filters.Add(new ElmahAuthorizationFilter());
        filters.Add(new HandleErrorAttribute());
    }
}}

So, it's possible to add global authrozation filter that could check, is authorization is done for ElmahController and apply any checks you want there. In this scenario, I'm blocking all users to access /elmah endpoint.

Does it make sense to you?

— Reply to this email directly or view it on GitHubhttps://github.com/alexanderbeletsky/elmah.mvc/pull/24#issuecomment-11686606.

alexbeletsky commented 11 years ago

Yeah, performance could be a good reason to include it. Will give it another shot :)

Mellbourn commented 11 years ago

Come to think of it, the authorization code itself will only be called when the ElmahController is used, so the speed of the authorization code is not really an issue, since it will be called equally often in the DI scenario.

Perhaps a more important issue is the API of elmah.mvc. The DI version makes it less clear. With my code, if you inject something, then allowedRoles will be ignored. That makes the API confusing: both DI and app settings alter authorization behavior. Maybe the API should be altered in the DI case, but I'm not sure how.

Klas

On Wed, Dec 26, 2012 at 3:09 PM, Alexander Beletsky < notifications@github.com> wrote:

Yeah, performance could be a good reason to include it. Will give it another shot :)

— Reply to this email directly or view it on GitHubhttps://github.com/alexanderbeletsky/elmah.mvc/pull/24#issuecomment-11686900.

MaximRouiller commented 11 years ago

Technically, when unit testing a controller with that global filter attribute, it won't affect it at all. So technically, filters are decoupled by default if you think about it.

var controllerToTest = new MyController();

This piece will never invoke the filters. So it's all good in my opinion.

Mellbourn commented 11 years ago

Maxim, I'm not sure I understand what you are saying. Do you mean that putting the authorization in global filters is more decoupled than extending the authorization code in elmah.mvc?

alexbeletsky commented 11 years ago

The progress a bit slowed down here. I would like to clearly understand, is that feature "really" required? In one hand @Mellbourn has already been created it and it's not a problem to put it in.. in another hand - introducing the new interface if nobody will use it, seems not to good as for me :)..

but you never know, before you try..

Mellbourn commented 11 years ago

We felt that we needed it in our project, but naturally I cannot answer if others will need it. The global filter authorization is an alternative solution which absolutely works. I'll leave it to you to decide if you want to put in the DI code or not.

stijnherreman commented 11 years ago

This should be documented somewhere, I only found this discussion when I wanted to open an issue myself :)

The global filter works fine for me, I did do it slightly different though. Instead, I inherit from my custom authorisation attribute and work with that. The constructor calls the base constructor with the Elmah permission.

Be careful when overriding OnAuthorization, the default implementation takes care of possible security issues with caching.

public class ElmahAuthorisation : RequirePermission
{
    public ElmahAuthorisation()
        : base(Permission.Elmah)
    {
    }

    public override void OnAuthorization(System.Web.Mvc.AuthorizationContext filterContext)
    {
        if (filterContext.Controller is Elmah.Mvc.ElmahController)
        {
            base.OnAuthorization(filterContext);
        }
    }
}
alexbeletsky commented 11 years ago

Thanks @stijn-h I think your implementation makes sense.

Since ELMAH.MVC does not have dedicated documentation page, will keep this issue. I'll try to blog about that someday as well :)

alexbeletsky commented 11 years ago

I think this one should be re-opened. Just got few request on same topic.

So, even the workarounds exist, it would be nice to have some built-in to Elmah.MVC.. Probably something that @Mellbourn has already built.

arangari commented 9 years ago

Is there a working example of "filter" approach or the "DI" approach?

Mellbourn commented 2 months ago

I'm closing this as too old