Security trimming does not pass querystring parameters to custom authorizeattributes

[yulObraz]

AuthorizeAttributeAclModule creates httpcontext from RouteData. This does not include querystring route parameters. So my custom authorizeattribute does not gets the parameters.
Values in filterContext.Controller.ValueProvider differs on normal security check and on security trimming check.

[NightOwl888]

First of all, query string parameters are not part of routing - MVC does not add them into the RouteValueCollection.

But that is not the only issue with this expectation. MvcSiteMapProvider checks the AuthorizeAttribute for every node in every request. For all of the nodes but one (the current node) the query string information would be completely meaningless. In fact, the AuthorizeAttributeAclModule uses a fake HttpContext based on the URL of the home page so the checks based on the nodes don't get polluted by the context of the current request.

Perhaps there is a solution for what you are trying to achieve, but it would be helpful to know what that goal is.

[yulObraz]

You create HttpContext by route data (RouteValueDictionary) and it contains querystring. With default routing all action parameters exept ids become querystring parameters. Then you through away it. And you call custom authorize attribute without this part of the path.
And the check is different from security check on original httprequest. What annoy me is that you get this information than throw away it.
I think that your MVC HTTPContext can transfer querystring a little further.

Many my methods contain Method parameter and user have roles for this methods. I wrote custom authorize attribure for this check. To be precisely I check Request.Unvalidated(). It worked correctly with security check but was unexpectedly broken with security trimming. (I have feeling that it worked for sometime.)
For me I created ugly workaround by replacement of MvcContextFactory and creating some special checks in custom authorize attribute.

You correctly check all authorize attributes that the node has. And it means that somebody thought which attibutes have sense for current node. So there is no much problem with every node and so on.

[NightOwl888]

It is unclear what you mean by "throwing it away" at this point. If you could provide a code sample of what you did as a workaround it would help with my understanding of the issue.

You could also try this alternative AuthorizeAttributeAclModule to see if that works for you.

I would appreciate if you could make this clear - I am working on a patch release right now and it would be nice to include a fix for this, if possible.

[yulObraz]

The original mvc routedata consept was as in http://www.tutorialsteacher.com/articles/what_is_routedata that routedata includes action parameters. But as understand it was removed (or processed after authorizeattribute). And there are suggestions to use filterContext.Controller.ValueProvider instead (http://stackoverflow.com/questions/5549371/using-action-parameters-in-custom-authorization-attribute-in-asp-net-mvc3).

I use MVC 4 and nugets 4.6.18 and I transfer routedata from node to authorization attribute explicitly because I don't get action parameters. They go to querystring and should be accessed through value provider or request.querystring.

FindRoutesForNode - create httpcontext by node routedata. And it contains routevalues and querystring. But querystring part is thrown away. MVC pass all the data to custom authorize attribute and attribute works.

[yulObraz]

Alternative gaves empty result on FindSiteMapNode and menu is empty with it.

[NightOwl888]

MVC always runs AuthorizeAttribute in the context of the current page, so there it makes sense to use the current context and pass in the route values and query string information from the current request.

MvcSiteMapProvider runs the security check for other pages as well (not just the current request). It would only make sense to use query string information if that information applies to every single page on the site. My question to you is what if the query string information has nothing to do with the node in question?

Correct FindRoutesForNode creates a fake HTTP context based on the home page and discards any route value or query string information. This is so we don't pollute the authorization check with irrelevant values from the current request.

Aside from this chicken-and-egg problem with checking security for other pages than the current one which MVC doesn't do at all and we do, Microsoft has made it pretty clear that you are not supposed to make a security scheme based on the URL. The purpose of AuthorizeAttribute is to ensure that _any_ URL that can reach an action method is checked to ensure that it is authorized. As soon as you depend on values from the URL you are allowing for vulnerabilities in security. In your case, someone could bypass the security scheme simply by changing the query string.

If I misunderstand and you are not using those values for security but for redirecting your request as in the example you linked to, do note that there is a HandleUnauthorizedRequest method that you can override for this purpose. MvcSiteMapProvider only checks to see if there is a null/not null value in the filterContext.Result, but does not execute it. Therefore, you could work around this by using the default behavior if the query string values don't exist.

protected override void HandleUnauthorizedRequest(AuthorizationContext filterContext)
{
    var id = (httpContext.Request.RequestContext.RouteData.Values["id"] as string) 
             ?? (httpContext.Request["id"] as string);

    if (string.IsNullOrEmpty(id))
    {
        // Query string doesn't exist, so just use the default logic
        base.HandleUnauthorizedRequest(filterContext);
    }
    else
    {
        // Set your custom result handler here
        filterContext.Result =
    }
}

[yulObraz]

Node has many additional attributes. Most used to create url as routevalues. Only Id and parameters with route configuration can be used in authorization attribute. Why? Why behavior depends on route definition?

MyAuthorizeAttribute in the sample checks that you are authorized this attribute value (id =8). It goes to routedata by default. And it works with SiteMap the same as with MVC. If you rename it to something like "objectId" it would work only with MVC, not SiteMap.

This security check works on access to the page, not on creating of querystring. So it is impossible use unautorized querystring value.

[NightOwl888]

Node has many additional attributes. Most used to create url as routevalues. Only Id and parameters with route configuration can be used in authorization attribute. Why? Why behavior depends on route definition?

Because MVC depends on routing to work. However, there is another option - set the URL property of the node and then you will have a URL-based node. You could potentially put a query string in there if you really need one.

MyAuthorizeAttribute in the sample checks that you are authorized this attribute value (id =8). It goes to routedata by default. And it works with SiteMap the same as with MVC. If you rename it to something like "objectId" it would work only with MVC, not SiteMap.

Not true. You need to account for every route value in the node. The value "objectId" would need to be added either to preservedRouteParamters or as a route value in order to be considered part of the match.

See: http://www.shiningtreasures.com/post/2013/09/02/how-to-make-mvcsitemapprovider-remember-a-user-position

This security check works on access to the page, not on creating of querystring. So it is impossible use unautorized querystring value.

I don't understand what you mean by this. Again, unless you provide some code to make this discussion clear, I probably won't be able to help you.

[yulObraz]

I thought that preservedRouteParamters means use value from current path. You removed attributesToIgnore?
Sample is here: https://github.com/yulObraz/Test

[NightOwl888]

I was able to track down one of the issues with this, but I am unable to work out how to make the ValueProvider function. I went through the source code of MVC, but it didn't seem to be much help.

If you grab the 2 files from this gist, and update your DI configuration as follows, you will be able to use the filterRequest.HttpContext.Request.QueryString["method"] to get your value.

var excludeTypes = new Type[] {
    // Use this array to add types you wish to explicitly exclude from convention-based  
    // auto-registration. By default all types that either match I[TypeName] = [TypeName] or 
    // I[TypeName] = [TypeName]Adapter will be automatically wired up as long as they don't 
    // have the [ExcludeFromAutoRegistrationAttribute].
    //
    // If you want to override a type that follows the convention, you should add the name 
    // of either the implementation name or the interface that it inherits to this list and 
    // add your manual registration code below. This will prevent duplicate registrations 
    // of the types from occurring. 

    // Example:
    // typeof(SiteMap),
    // typeof(SiteMapNodeVisibilityProviderStrategy)

    // Add these 2 lines...
    typeof(MvcContextFactory),
    typeof(AuthorizeAttributeAclModule)
};

/// Other registration code here...

// Multiple implementations of strategy based extension points (and not decorated with [ExcludeFromAutoRegistrationAttribute]).
CommonConventions.RegisterAllImplementationsOfInterface(
    (interfaceType, implementationType) => this.For(interfaceType).Singleton().Use(implementationType),
    multipleImplementationTypes,
    allAssemblies,
    excludeTypes,
    string.Empty);

// Add this line ...
this.for<IMvcContextFactory>().Use<MvcContextFactory2>();

/// Other registration code here...

// Configure Security
this.For<IAclModule>().Use<CompositeAclModule>()
    .EnumerableOf<IAclModule>().Contains(x =>
    {
        // Change this line...
        x.Type<AuthorizeAttributeAclModule2>();
        //x.Type<XmlRolesAclModule>();
    });

It turns out there is a bug in the MvcContextFactory that is improperly setting the query string value - it needs to chop off the ?, but wasn't doing so.

Your other option is just to configure another route, and then your code will work like you have it.

public class RouteConfig
{
    public static void RegisterRoutes(RouteCollection routes)
    {
        routes.IgnoreRoute("{resource}.axd/{*pathInfo}");

        // Add this route
        routes.MapRoute(
            name: "Test",
            url: "Test/{action}/{method}",
            defaults: new { controller = "Test" }
        );

        routes.MapRoute(
            name: "Default",
            url: "{controller}/{action}/{id}",
            defaults: new { controller = "Home", action = "Index", id = UrlParameter.Optional }
        );
    }
}

[yulObraz]

Yes, it works so (and titles are not mixed). QueryString is ok for me. I prefer override AuthorizeCore and ValueProvider is not available here. I am thinking about routes in attributes. Route config is too far from actions.

[NightOwl888]

Well, attribute routes just get registered as routes in the end, so they work exactly the same way. You will need either upgrade to MVC5 or use the open source attribute routing to use them, though. We officially support the MVC5 attribute routing. I have not tested the open source project, but haven't had a single person complain about issues with it.