-
Notifications
You must be signed in to change notification settings - Fork 23
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Update AutofacFilterProvider.GetFilters() to skip evaluation if the controller instance is null #24
Comments
Can you show us how to reproduce the issue? It's not a big deal to guard against, but sometimes those checks are just there for testing, not a condition that would normally be hit in runtime. Hence the question - how would you get a null controller in a normal runtime environment? |
Yes, well...I have no idea. That's the problem I was hoping you would have some insight into, such that I wouldn't have to detail our dependency structure and configuration. I was hoping that a null check would be the easy (and correct) solution, just based on what I saw in the base implementation. But here goes... We use MVCSiteMapProvider for navigation on our site. We recently enabled "Security Trimming", which can show or hide nodes based on AuthorizeAttributes that decorate the routes that the menu nodes point to. This all worked fine when the site was using Windows Authentication. We recently changed to Forms Auth (we use both in different production environments), and that's when it started breaking. The Stack trace ends with the null reference in AutofacFilterProvider. Now, there could just as likely be a problem in our site map configuration/usage (or even a bug in MVCSiteMapProvider code). I thought as much when I started debugging this, because this is happening upon first visiting the site. The user is redirected to the log-in page, as he is not yet authenticated. I thought it would have something to do with the user being anonymous. However, when I started debugging the code, I hit a breakpoint in GetFilters() for 3 or 4 controller actions before we hit the action that contained the null Controller property. Hopefully that's of some help. I'll keep digging in the mean time.
|
Okay, it turns out that it was the MvcSiteMapProvider.AuthorizeAttributeAclModule.IsAccessibleToUser() call that was setting the Controller to null. Ultimately, the problem was in our code, as the controller construction was throwing an exception with an anonymous user. The method above swallows the exception and proceeds as though nothing is amiss. I don't know, frankly, if a null Controller property should be an exceptional condition or not. But you can make that determination on your own time, as this is no longer a pressing need for us. |
I'll leave this open and we can add the check. It makes sense to behave like the base provider though I don't think it would have guaranteed correct behavior for you - if the controller is null it skips filter evaluation entirely, which means the authorization wouldn't necessarily have been calculated correctly. As soon as you mentioned MvcSiteMapProvider I pretty much knew the issue was going to be there. I've spent a lot of time with MvcSiteMapProvider in the past. The idea is good, but given proper authorization filter eval requires a controller instance (as you're seeing in that default filter attribute provider) I found that we were eating up a lot of memory on every request when we had to actually instantiate every controller in the map just to evaluate whether or not it should show up in the map. I'm not sure if that issue's been resolved; for the projects I was on we ended up writing a different, custom version that didn't require that. Anyway, glad you figured it out. |
When the controllerContext.Controller property is null, the call to GetType() to populate the filterContext throws an exception.
We've never seen this exception before, and are only now seeing it with a configuration change. Given that fact, I would normally assume that this is a problem with our configuration (I can't figure out how/why the Controller property would be null).
HOWEVER, AutofacFilterProvider inherits from System.Web.Mvc.FilterAttributeFilterProvider, and that base implementation of GetFilters() does a null check on the Controller property. Therefore, I assume there must be a normal condition under which that property is expected to be null. If so, then I expect that the null check should also be done in this method.
The text was updated successfully, but these errors were encountered: