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

Use GET to determine node accessibility #14

Closed
MaxKot opened this issue Jan 12, 2012 · 6 comments
Closed

Use GET to determine node accessibility #14

MaxKot opened this issue Jan 12, 2012 · 6 comments

Comments

@MaxKot
Copy link

MaxKot commented Jan 12, 2012

I have two actions with the same name: one for GET requests and another one for POST. These action have require different user roles (the user must have write access for POST).

AuthorizeAttributeAclModule uses current HTTP method to determine the action so users without write access don't have the site map node after they POST on some other page. This is not right because following links in a site map generates GET requests.

I think HTTP method should be configurable or GET.

MaxKot pushed a commit to MaxKot/MvcSiteMapProvider that referenced this issue Aug 14, 2012
Adds 'routeMethod' attribute to sitemap provider initialization attributes. The value set in the attribute is used when determining current route. If the value is empty or not specified the method of the current request is used.
maartenba added a commit that referenced this issue Aug 25, 2012
 Fix for issue #14 (Use GET to determine node accessibility).
@MaxKot MaxKot closed this as completed Aug 27, 2012
@MaxKot MaxKot reopened this Aug 27, 2012
@MaxKot MaxKot closed this as completed Aug 27, 2012
@NightOwl888
Copy link
Collaborator

@MaxKot

Is it possible you could provide a small sample project showing how you use this functionality? We have v4 in beta now, but I just realized that this functionality is not yet implemented there. It would be a big help if we have a way to ensure we can get it to work the way it was intended to.

Alternatively, if you could submit another pull request based on the v4 branch that would be great.

@NightOwl888 NightOwl888 reopened this Jun 26, 2013
@MaxKot
Copy link
Author

MaxKot commented Jun 27, 2013

@NightOwl888

I created a demo project for the issue. The text in the views and comments in HomeController should explain the issue.

I hope I will fix the issue in the v4 branch tomorrow evening or on the weekend.

@NightOwl888
Copy link
Collaborator

Hey, thanks for doing this. If you don't have the time to implement it will be a big help.

MaxKot pushed a commit to MaxKot/MvcSiteMapProvider that referenced this issue Jun 30, 2013
…determine node accessibility) behavior.

Issue description
AuthorizeAttributeAclModule uses current HTTP context to determine whether the node is visible. If action overloads accepting different HTTP verbs have different authorization parameters then AuthorizeAttributeAclModule will use authorization parameters of an action accepting HTTP verb of current request. This behavior is invalid because clicking site map node links usually results in a GET request.
MaxKot pushed a commit to MaxKot/MvcSiteMapProvider that referenced this issue Jun 30, 2013
Note
I 'reused' ISiteMapNode.HttpMethod property because it seems to replicate DefaultSiteMapProvider.RouteMethod property (that was used to in the fix in the old project) and it is neither documented nor used after setting it in an ISiteMapBuilder.
@MaxKot MaxKot mentioned this issue Jun 30, 2013
Closed
NightOwl888 pushed a commit that referenced this issue Jul 1, 2013
Note
I 'reused' ISiteMapNode.HttpMethod property because it seems to replicate DefaultSiteMapProvider.RouteMethod property (that was used to in the fix in the old project) and it is neither documented nor used after setting it in an ISiteMapBuilder.
@NightOwl888
Copy link
Collaborator

@MaxKot

In your demo project, you provided XML as follows:

<?xml version="1.0" encoding="utf-8" ?>
<mvcSiteMap xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
            xmlns="http://mvcsitemap.codeplex.com/schemas/MvcSiteMap-File-4.0">

  <mvcSiteMapNode title="Demo site" controller="Home" action="Index" visibility="!*" httpMethod="GET">
    <mvcSiteMapNode title="Home" controller="Home" action="Index"/>
    <mvcSiteMapNode title="About" controller="Home" action="About"/>
  </mvcSiteMapNode>

</mvcSiteMap>

However, it does not function correctly this way because httpMethod is not inherited. It only functions correctly if the httpMethod is added specifically on the nodes that require it.

<?xml version="1.0" encoding="utf-8" ?>
<mvcSiteMap xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
            xmlns="http://mvcsitemap.codeplex.com/schemas/MvcSiteMap-File-4.0">

  <mvcSiteMapNode title="Demo site" controller="Home" action="Index" visibility="!*">
    <mvcSiteMapNode title="Home" controller="Home" action="Index"/>
    <mvcSiteMapNode title="About" controller="Home" action="About" httpMethod="GET"/>
  </mvcSiteMapNode>

</mvcSiteMap>

I am trying to determine if this is the expected behavior, as someone has opened another issue about this #230. I went back to v4.0.14 (before the changes to AuthorizeAttributeAclModule), but it functions exactly the same way.

It seems like it should use GET by default if httpMethod is not supplied (to make MvcSiteMapProvider do a POST, the HTML helpers would need to be modified to spit out form tags or JavaScript), but the logic you provided uses the current HTTP method by default.

So, it seems that the logical way to do this would be:

  • By default, httpMethod is empty string - use GET
  • httpMethod is * or "CURRENT", use HTTP method of current request
  • httpMethod is specified as something else, use that value

That would cover all of the possible scenarios, but using httpMethod parameter would only be required if you modify the HTML helpers.

Thoughts?

@MaxKot
Copy link
Author

MaxKot commented Sep 21, 2013

@NightOwl888

Using GET by default would be great for me. I used current HTTP method by default only as a backwards-compatibility precaution.

As for httpMethod I did expect it to be inherited by the child nodes but it is not important if GET would be used by default.

@NightOwl888
Copy link
Collaborator

Well, backwards compatibility wasn't that much of a concern in a major release - unfortunately, now we have to make a potentially breaking change in a minor release. But the fact of the matter is the default behavior is broken given the default behavior of how the nodes are rendered, and the reality is the change probably won't affect that many users.

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

No branches or pull requests

2 participants