[Mono-dev] SiteMapProvider patch

[Marek Habersack]

On Wed, 30 May 2007 10:03:58 +0300, "Dumitru Ban" <dban at dako.ro> scribbled:

> Hi,
Hey Dumitru,

> Yesterday, when I post my questions, the #1 looked like this:
> 
> if (node.Roles != null)
>     foreach (string rolename in node.Roles)
>         if (rolename == "*" || context.User.IsInRole (rolename))
>             return true;
> 
> It was not returning false if there were roles defined for the node and there
> was no match for the user.
> 
> Marek updated the code and now it looks like this:
Yes, I meant to write about it in response to you, but forgot... :) - sorry
about that.

> IList roles = node.Roles;
> if (roles != null && roles.Count > 0) {
>     foreach (string rolename in roles)
>         if (rolename == "*" || context.User.IsInRole (rolename))
>             return true;
>         return false;
> }
> 
> But, in the MSDN, it says that the method should return true if:
> The Roles exists on node and the current user is in at least one of the
> specified roles.
Which is what the above code does. 

> - or -
> The current thread has an associated WindowsIdentity that has file access to
> the requested URL and the URL is located within the directory structure for
> the application.
This part is not implemented yet.

> - or -
> The current user is authorized specifically for the requested URL in the
> authorization element for the current application and the URL is located
> within the directory structure for the application.
> 
> In my opinion #1 should not return false at all. It should go and check for
> #2 and/or #3. The update Marek made is working for the case where no url is
> defined for the node. But what happens if the node has an url, a role is
> defined for that node, the user is not in that role, but the user is
> specifically authorized for the requested url in the authorization element?
> It will return false, but it should return true...
I think you're right. We should proceed with all of the checks (in the order
described in the MSDN docs) and return true if any of them succeeds.

> 
> I think the correct code should look something like this:
> 
> /* 1. */
> IList roles = node.Roles;
> if (roles != null && roles.Count > 0) {
>     foreach (string rolename in roles)
>         if (rolename == "*" || context.User.IsInRole (rolename))
>             return true;
> }
> 
> /* 2. */
> /* XXX */
> 
> /* 3. */
> string url = node.Url;
> if(!String.IsNullOrEmpty(url)) {
>     // TODO check url is located within the current application
> 
>     if (VirtualPathUtility.IsAppRelative (url)
> || !VirtualPathUtility.IsAbsolute (url)) url = VirtualPathUtility.Combine
> (VirtualPathUtility.AppendTrailingSlash
> (HttpRuntime.AppDomainAppVirtualPath), url);
> 
>     AuthorizationSection config = (AuthorizationSection)
> WebConfigurationManager.GetSection ( "system.web/authorization",
>         url);
>     if (config != null)
>         return config.IsValidUser (context.User, context.Request.HttpMethod);
> }
> 
> return false;
yes, that's definitely more like it. Feel free to send in a diff to the list -
I'll apply it.

thanks a lot, best regards

marek
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 189 bytes
Desc: not available
Url : http://lists.ximian.com/pipermail/mono-devel-list/attachments/20070530/d8049995/attachment.bin