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

AuthorizeAttributeAclModule fails to take into account request method #230

Closed
siimv opened this issue Sep 19, 2013 · 8 comments
Closed

AuthorizeAttributeAclModule fails to take into account request method #230

siimv opened this issue Sep 19, 2013 · 8 comments

Comments

@siimv
Copy link

siimv commented Sep 19, 2013

When I have multiple actions with the same name but one of them decorated with [HttpPost] attribute, then it finds wrong ActionDescriptor.
I assume links in sitemap are always meant for GET?

Fortunately it was quite an easy fix, I just needed to overwrite CreateControllerContext method in acl module.

@NightOwl888
Copy link
Collaborator

The httpMethod can be overridden in RequestContext, which is used to cover this scenario. There is an httpMethod property that can be set on the SiteMap node where you can specify the HTTP method to use. It defaults to "*", or any. For your scenario, you just need to set it to "POST". Keep in mind you will probably need to make another node to cover the "GET" scenario.

If you find that it is not working the way you expect, feel free to open this issue again. Thanks.

@NightOwl888
Copy link
Collaborator

I took a look at the code, and the default value is being set to empty string instead of "*". Furthermore it not possible to set this value from Dynamic Nodes.

@NightOwl888 NightOwl888 reopened this Sep 20, 2013
NightOwl888 added a commit to NightOwl888/MvcSiteMapProvider that referenced this issue Sep 20, 2013
…odes. Also added HttpMethod property to DynamicNode so HttpMethod can be set.
@NightOwl888
Copy link
Collaborator

By the way, what method are you using to register your nodes?

@siimv
Copy link
Author

siimv commented Sep 20, 2013

I have XML sitemap file for that, so no dynamic nodes. I use it for simple menu.

I noticed some other weird behavior also - like sometimes acl shows an item, sometimes not and some area related stuff also, but I have not been able to pinpoint the problem and maybe it's just my code.

@NightOwl888
Copy link
Collaborator

For areas, you need to follow the MVC conventions outlined here: http://msdn.microsoft.com/en-us/library/ee671793(v=vs.100).aspx. In addition, you must specify namespaces on each of your routes.

BTW - I have confirmed GET/POST is no longer functioning as it was intended using the demo project in #14. For some reason I have not yet determined, it is not creating a controller context for the About node.

@NightOwl888
Copy link
Collaborator

My bad - the GET/POST behavior is working as it was originally intended. If you add httpMethod="GET" to your node, it will use the AuthorizeAttribute from the GET action method.

However, I don't think the default behavior makes sense given that the default HTML helper templates never do anything but GET.

So, it seems the behavior of this feature should be changed:

  • 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 in some unusual way that makes them do a POST or other HTTP method.

Thoughts?

@siimv
Copy link
Author

siimv commented Sep 21, 2013

Changing default httpMethod to GET seems a good idea.

@NightOwl888
Copy link
Collaborator

Okay, the fix is now in v4.4.0, which is also available on NuGet.

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