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

Modifying SiteMapNode.RouteValues in action method does not change URL when security trimming is enabled #272

Closed
rahulbpatel opened this issue Feb 5, 2014 · 2 comments

Comments

@rahulbpatel
Copy link

As per the article How to Make MvcSiteMapProvider Remember a User’s Position
(http://www.shiningtreasures.com/post/2013/09/02/how-to-make-mvcsitemapprovider-remember-a-user-position), this code example does not seem to work when security trimming is enabled:

public ActionResult Edit(int id)
{
    var node = SiteMaps.Current.FindSiteMapNodeFromKey("Product_Edit");
    if (node != null)
    {
        node.RouteValues["id"] = id;
        var parent = node.ParentNode;
        if (parent != null)
        {
            parent.RouteValues["id"] = id;
        }
    }

    // Implementation omitted
}

Here's what looks like is happening:

  1. SiteMaps.Current.FindSiteMapNodeFromKey causes AuthorizeAttributeAclModule.FindRoutesForNode method to be executed
  2. AuthorizeAttributeAclModule.FindRoutesForNode is calling RequestCacheableSiteMapNode.Url and causing the URL to be cached for the request
  3. The action method will execute and modify the RouteValues but it has no effect because
  4. RequestCacheableSiteMapNode.get_URL calls GetCachedOrMemberValue with the storeInCache argument set to true

I'm not sure exactly would be the most sensible way to fix this issue. Perhaps if the RouteValues property is modified that the request cache is cleared?

@NightOwl888
Copy link
Collaborator

I see your point.

One option would be to simply remove the request caching from the RequestCacheableSiteMapNode.Url property (just remove the property definition from the class). I am not certain if there is any real benefit to request caching this field because MVC normally resolves URLs on demand with no caching. The primary reason why it is here is because we don't know how inefficient a custom URL resolver might be, but I have since learned that the need for a custom URL resolver is quite limited because it is almost always better to use custom routes instead.

@NightOwl888
Copy link
Collaborator

After thinking it over some more, that isn't a fix either. Changing the RouteValues has the potential to invalidate the security context. Imagine if you did this:

public ActionResult Edit(int id)
{
    var node = SiteMaps.Current.FindSiteMapNodeFromKey("Home");
    if (node != null)
    {
        node.RouteValues["controller"] = "Account";
        node.RouteValues["action"] = "Index";
    }
}

So if you did this after the security check, we could effectively ignore the security by changing the URL of the home page to a more privileged URL. MVC would still respect the security context, though.

The only reasonable way around this situation is to move the code that updates it earlier in the request lifecycle. One way to do this currently is to put the code that updates the node in the Global.asax file's Application_BeginRequest event.

protected void Application_BeginRequest(object sender, EventArgs e)
{
    // Get the current node
    var currentNode = MvcSiteMapProvider.SiteMaps.Current.CurrentNode;
    if (currentNode != null)
    {
        switch (currentNode.Key)
        {
            case "Product_Edit":
                var routeData = RouteTable.Routes.GetRouteData(new HttpContextWrapper(HttpContext.Current));

                // If using attribute routing (MVC 5) do this
                if (routeData.Values.ContainsKey("MS_DirectRouteMatches"))
                {
                    routeData = ((IEnumerable<RouteData>)routeData.Values["MS_DirectRouteMatches"]).First();
                }

                int id = int.Parse(routeData.Values["id"] != null ? routeData.Values["id"].ToString() : "0");
                currentNode.RouteValues["id"] = id;
                var parent = currentNode.ParentNode;
                if (parent != null)
                {
                    parent.RouteValues["id"] = id;
                }
                break;

            // Put additional cases here for other nodes if they are the current one.
        }
    }
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants