Skip to content
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

Open
demonsweatlive opened this issue Nov 29, 2017 · 4 comments
Labels

Comments

@demonsweatlive
Copy link

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.

@tillig
Copy link
Member

tillig commented Nov 30, 2017

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?

@demonsweatlive
Copy link
Author

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.

[NullReferenceException: Object reference not set to an instance of an object.]
System.Object.GetType() +0
Autofac.Integration.Mvc.AutofacFilterProvider.GetFilters(ControllerContext controllerContext, ActionDescriptor actionDescriptor) +336
MvcSiteMapProvider.AuthorizeAttributeAclModule.IsAccessibleToUser(IControllerTypeResolver controllerTypeResolver, DefaultSiteMapProvider provider, HttpContext context, SiteMapNode node) +1497
MvcSiteMapProvider.DefaultAclModule.IsAccessibleToUser(IControllerTypeResolver controllerTypeResolver, DefaultSiteMapProvider provider, HttpContext context, SiteMapNode node) +170
MvcSiteMapProvider.DefaultSiteMapProvider.IsAccessibleToUser(HttpContext context, SiteMapNode node) +448
System.Web.StaticSiteMapProvider.GetChildNodes(SiteMapNode node) +334
MvcSiteMapProvider.DefaultSiteMapProvider.FindControllerActionNode(SiteMapNode rootNode, IDictionary`2 values, RouteBase route) +65
MvcSiteMapProvider.DefaultSiteMapProvider.FindSiteMapNode(HttpContext context, RouteData routeData) +705
MvcSiteMapProvider.DefaultSiteMapProvider.FindSiteMapNode(HttpContext context) +122
System.Web.SiteMapProvider.get_CurrentNode() +70
MvcSiteMapProvider.DefaultSiteMapProvider.get_CurrentNode() +141
MvcSiteMapProvider.Web.Html.SiteMapPathHelper.SiteMapPath(MvcSiteMapHtmlHelper helper, String templateName) +48
WOTI.Xift.XiftWebApp.LayoutHelperExtensions.XiftSiteMapPath(HtmlHelper Html) +19

@demonsweatlive
Copy link
Author

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.

@tillig
Copy link
Member

tillig commented Nov 30, 2017

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.

@tillig tillig added the bug label Nov 30, 2017
@tillig tillig changed the title NullReferenceException in AutofacFilterProvider.GetFilters() Update AutofacFilterProvider.GetFilters() to skip evaluation if the controller instance is null Nov 30, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants