Authorization failures

[waynebrantley]

I was attempting to log all authorization failures of [Authorize] attributes on controllers and actions. I can log them of course. The problem is that MvcSiteMapProvider appears to go through the entire sitemap on every request and attempt to authorize every node - to do 'security trimming'.

Are there any options here? 1) Anyway I can know that MvcSiteMap was the one doing this 'check' so I can not log it? 2) That seems expensive to create all those controllers (and their DI injected dependencies) for every request to determine authorization. Seems like this would be cached for some timeframe.

Thoughts?

[NightOwl888]

1) You can skip logging with security trimming by checking whether the type of context starts with the namespace MvcSiteMapProvider.

if (!typeof(filterContext).Namespace.StartsWith("MvcSiteMapProvider"))
{
    // Do logging
}

Note that if you need to pass this information to AuthorizeCore (or another part of AuthorizeAttribute), you should pass the state through HttpContext.Items to ensure that it is thread safe. AuthorizeAttribute may be called by multiple threads at once.

2) Constructors should be simple so creating controllers is cheap. In fact, MVC creates a controller on every request anyway, so this sometimes reveals scalability issues applications that are doing too much in the controller constructor.

That said, I have considered doing caching but when I tried to implement it I realized just how complicated it would be. Not to mention, breaking API changes.

The controller can be created using the IControllerFactory or by Activator.CreateInstance. I am not entirely sure why the redundancy was put in place. However, it occurred to me since you asked that the original reason for this may have been to create the instance of the internal XmlSiteMapController, which is now being done by a decorator for the registered IControllerFactory. So this redundancy probably isn't required anymore.

If we keep this redundancy in place, the way each controller was created would need to be tracked along with the instance so we would know whether to call IControllerFactory.Release or check whether the controller implements IDisposable and then dispose it in the case it does.

The controller instances would need to be tracked in a thread-safe manner and released at the end of the request. To remain MVC-centric, that should probably be done using IActionFilter.OnActionExecuted and the wrapper for HttpContext.Items, RequestCache. Of course, this will mean some breaking DI configuration changes to register the action filter with MVC. Also, a service should be created responsible for tracking and destroying the controller instances to ensure neither the AuthorizeAttributeAclModule or IAuthorizationFilter have to deal with looking up the cache and so the caching details are not in multiple areas of the application.

In fact, this new service should probably be responsible for either creating the controller or retrieving it from the cache so the AuthorizeAttributeAclModule can drop the dependency on IControllerBuilder and replace it with the new service.

I would not be opposed to a pull request with a prototype, but be sure to incorporate the changes from this gist from #412. Also note that those changes don't actually fix the original issue in #412, which was to use the IValueProvider of the controller to get the query string values. I haven't been able to work out why that does not function.

[waynebrantley]

The filtercontext idea will definitely help! My constructors are cheap to create...but still to create 100 constructors on every page request (and their attributes and dependencies) to determine security is not going to be cheap.

For me, I am comfortable having the entire permissions structure of the sitemap read one time at startup...as long as there is a way to 'refresh' it...

[NightOwl888]

For me, I am comfortable having the entire permissions structure of the sitemap read one time at startup...as long as there is a way to 'refresh' it...

Since permissions are user-based, loading it at startup is not possible. The best we could do is load it into a user session. But then, that assumes we use session state, which we currently don't as session state is something I have been avoiding for scalability reasons.

But still, there would be a performance gain if each action didn't have to create a new instance of the controller just to check if it is authorized. So I will consider this a feature request.

[waynebrantley]

Well, it could store it in some ICacheProvider interface that defaulted to 'nothing' so it was not cached. Then simply make the key to the cache item a function of the username (or anonymous)...etc. (Just a quick thought...)

In my sitemap, I have filled out the Roles=* attribute. So, for me I really just need you to ask if the user is in that role. No need to create the controller.

So, in the above example - if I create custom IAclModule that simply checks the role - will all the controllers stop being created? (or is there another easy way)?

[NightOwl888]

Well, it could store it in some ICacheProvider interface that defaulted to 'nothing' so it was not cached. Then simply make the key to the cache item a function of the username (or anonymous)...etc. (Just a quick thought...)

I have considered doing something like this before. Then I remembered that some people are on server farms and it is much better for them to use session state (which can be shared between servers) rather than using a cache (unless of course it is a distributed cache).

In my sitemap, I have filled out the Roles=* attribute. So, for me I really just need you to ask if the user is in that role. No need to create the controller.

So, in the above example - if I create custom IAclModule that simply checks the role - will all the controllers stop being created? (or is there another easy way)?

Actually, you don't need to jump through so many hoops. The default configuration automatically wires up an XmlRolesAclModule that checks the ISiteMapNode.Roles property to see if any of the roles match. You just would need to remove the AuthorizeAttributeAclModule from your configuration (assuming you are using external DI).

But then, if you want your controller actions to have any measure of security, you need to duplicate your roles in the AuthorizeAttribute on each action method. So, for the extra performance gain, you have more maintenance. On the other hand, if you use the AuthorizeAttributeAclModule, you only need to configure the roles in one place - AuthorizeAttribute.

[waynebrantley]

We already use AuthorzeAttribute on the controllers...AND already have it in the sitemap...so was already doing both. I will try to disable that module and see what I get! (Could be some are out of sync too).

Cache - yes, that was why it would be an ICacheProvider. They could implement that with a session, local cache, distributed cache, etc. Thanks for responses.

[NightOwl888]

@waynebrantley

I will consider some sort of caching at the user level. But even if not doing that, most would benefit significantly by request caching the controllers so each one is only created once per request instead of once per action.

Either way, this is still an open issue that needs to be resolved. Perhaps this is an excuse to finally push forward version 4.7, which I have been holding off on because there are only bug fixes but no features to release and it will require breaking external DI configuration changes.

[yulObraz]

Is not it a user level caching? public class UserSiteMapCacheKeyGenerator : ISiteMapCacheKeyGenerator { public virtual string GenerateKey() { var context = HttpContext.Current; if(context.User.Identity.IsAuthenticated) { return context.User.Identity.Name; } else { return "default"; } } }