MvcSiteMapProvider icon indicating copy to clipboard operation
MvcSiteMapProvider copied to clipboard

How to explore all nodes regardless the security trimming ?

Open eric-b opened this issue 11 years ago • 23 comments

Hello,

I'm not sur if it's the right place to ask questions... If it's not, I apologize in advance.

I'm looking for a way to explore all nodes (SiteMapNode.GetAllNodes()) of a sitemap with security trimming enabled.

I tried to duplicate the default sitemap into another wi security trimming disabled but it does not seem to be the right way: MvcSiteMapProvider manages a single XmlSiteMapProvider regardless the number of initial sitemaps configured.

Thanks in advance.

Cheers,

eric-b avatar Nov 21 '12 17:11 eric-b

There is support for multiple sitemaps in current build.

waynebrantley avatar Dec 13 '12 15:12 waynebrantley

I think this functionality is inherently buggy (probably because of StaticSiteMapProvider): two sitemaps cannot have the same node URL. So, it can't work for this scenario.

If I'm wrong, I thank any good soul to illustrate with an example to the contrary ...

eric-b avatar Dec 13 '12 18:12 eric-b

In v4 we have reworked the way sitemaps are implemented (no longer dependent on System.Web.SiteMapProvider), so you should now be able to pull up 1 sitemap with security trimming enabled and 1 with it disabled so you can see all nodes. Security trimming can be turned on and off at the XML file level now, so in order to do this you will need to inherit XmlSiteMapBuilder and override the SetEnableLocalization method (or alternatively create your own ISiteMapBuilder implementation).

There are a few hoops you need to jump through to make the current request map to more than one sitemap, so I am interested to see if this specific use case is covered by the new design.

https://github.com/maartenba/MvcSiteMapProvider/tree/v4

NightOwl888 avatar Apr 06 '13 12:04 NightOwl888

Hi,

Thanks very much for your explanation. I'm always interested by this use case, so i'l take a look at v4 as soon as possible (probably not this week though).

Actually, I do not want to map the current request on multiple sitemaps. I want to use this functionality in an admin area. I need some sort of logic to browse the sitemap tree (only one tree) regardless of current user rights. The goal is not to provide a kind of menu linked to the sitemap nodes. I want to allow an admin user to administer some aspects tied to the sitemap nodes.

Thanks again for your time

eric-b avatar Apr 06 '13 13:04 eric-b

Are you saying that you want to be able to administer the position of nodes within the SiteMap from the Administration screen? If so, that is the exact scenario I had in mind when I came up with this design.

There is a working demo of what I would like to achieve here: http://www.bigace.de/demo.html. Have a look at the v3 administration interface and go to Pages > Administration from the menu. The pane on the left allows for creating and moving of pages. All changes are reflected in the main menu of the site and the site map path.

Unfortunately, all of that is done in PHP (except for the drag and drop part, which I plan to utilize).

There is now a new interface, ISiteMapBuilder that allows you to build a site map from any source, and a couple of interfaces to determine which set of builders belong to a given request. The per request logic can be based on URL, cookie, user session, or whatever else you can imagine. Essentially, you can now implement whatever logic is necessary to enable this scenario and inject it.

NightOwl888 avatar Apr 06 '13 13:04 NightOwl888

Not exactly. I have a project with user rights management by application. An application is represented by a node in the sitemap. Actually, the project is an intranet. I want to allow administrators to manage some rights based on application scope. So I use the sitemap to discover all applications. But the admin not necessarily has access to all nodes. It's purely a functionality needed for the work of a controller. It's not used to present some links to the user.

Your scenario is interesting too. It is a core functionality needed by any CMS.

eric-b avatar Apr 06 '13 16:04 eric-b

I see. Well, the core hasn't changed that much from Microsoft's model. The IsAccessibleToUser() method controls which nodes are available in the tree. Currently, the check for security trimming is the only thing in there. The rest is injected as an IAclModule (which can be multiple with the composite pattern).

Anyway, I don't see any reason why some sort of switch can't be put in the IAclModule to enable all (or some) of the nodes under a specific condition so they can be used from the public interface.

Well, when you have time take a look and let us know what you think.

NightOwl888 avatar Apr 07 '13 00:04 NightOwl888

Yes, I will. Most probably before end of this month.

eric-b avatar Apr 07 '13 11:04 eric-b

FYI - If you haven't looked already, v4 is now available on the public Nuget feed as a prerelease.

NightOwl888 avatar Jun 23 '13 08:06 NightOwl888

Hi,

Thanks for this news. I haven't tried yet, the project on which I had this issue is closed now.

I just tried on a sample web project, and I probably did something wrong because I didn't manage to enable security trimming.

What I've done :

  • Created an MVC 4 application (basic template)

  • Installed nuget package (MvcSiteMapProvider.MVC4 beta2)

  • Registered the sitemap provider in web.config :

    <add name="defaultSitemapProvider"
     type="MvcSiteMapProvider.DefaultSiteMapProvider, MvcSiteMapProvider"
     siteMapFile="Mvc.sitemap"
     securityTrimmingEnabled="true"
     cacheDuration="5"
     enableLocalization="false"
     scanAssembliesForSiteMapNodes="false"
     excludeAssembliesForScan=""
     includeAssembliesForScan=""
     attributesToIgnore=""
     nodeKeyGenerator="MvcSiteMapProvider.DefaultNodeKeyGenerator, MvcSiteMapProvider"
     controllerTypeResolver="MvcSiteMapProvider.DefaultControllerTypeResolver, MvcSiteMapProvider"
     actionMethodParameterResolver="MvcSiteMapProvider.DefaultActionMethodParameterResolver, MvcSiteMapProvider"
     aclModule="MvcSiteMapProvider.DefaultAclModule, MvcSiteMapProvider"
     siteMapNodeUrlResolver="MvcSiteMapProvider.DefaultSiteMapNodeUrlResolver, MvcSiteMapProvider"
     siteMapNodeVisibilityProvider="MvcSiteMapProvider.DefaultSiteMapNodeVisibilityProvider, MvcSiteMapProvider"
     siteMapProviderEventHandler="MvcSiteMapProvider.DefaultSiteMapProviderEventHandler, MvcSiteMapProvider"
     />
    
  • Added some pages in Mvc.sitemap file :

    <mvcSiteMapNode title="Home" controller="Home" action="Index">
        <mvcSiteMapNode title="About" controller="Home" action="About"/>
        <mvcSiteMapNode title="Page1" controller="Home" action="Page1"/>
        <mvcSiteMapNode title="Page2" controller="Home" action="Page2" roles="nonExistentRole"/>
        <mvcSiteMapNode title="Page3" controller="Home" action="Page3"/>
    </mvcSiteMapNode>
    
  • Added some action methods in Home Controller :

    public class HomeController : Controller
    {
        public ActionResult Index()
        {
            return View();
        }
    
        public ActionResult About()
        {
            return View();
        }
    
        public ActionResult Page1()
        {
            return View();
        }
    
        public ActionResult Page2()
        {
            return View();
        }
    
        [Authorize(Roles = "nonExistentRole")]
        public ActionResult Page3()
        {
            return View();
        }
    }
    
  • Then, updated the shared _layout.cshtml view like this :

    <body>
        @Html.MvcSiteMap().SiteMapPath()
        @RenderBody()
        @Html.MvcSiteMap().SiteMap()
    </body>
    

When I run the site, the sitemap shows all pages, regardless the "role" XML attribute and the "AuthorizeAttribute". I can navigate to "Page2" (protected with a role XML attribute), but I can't access to "Page3" (protected with AuthorizeAttribute).

What did I do wrong ?

This new release looks promising. Thanks for this very usefull project.

eric-b avatar Jun 23 '13 11:06 eric-b

Ahh, I see you are still in v3 mode :).

First of all, we have cut ties with the ASP.NET sitemap provider model completely - it was impossible to work around that for multi-tenant applications because of its dependency on web.config. So, you can completely remove the <sitemap> node from the web.config.

You are probably now wondering how to configure the provider if not in that element. If not using an external DI setup, many of those settings have been moved to the appSettings section and correspond exactly with the settings in v3.

<appSettings>
  <add key="MvcSiteMapProvider_UseExternalDIContainer" value="false"/>
  <add key="MvcSiteMapProvider_SiteMapFileName" value="~/Mvc.sitemap"/>
  <add key="MvcSiteMapProvider_ScanAssembliesForSiteMapNodes" value="false"/>
  <add key="MvcSiteMapProvider_ExcludeAssembliesForScan" value=""/>
  <add key="MvcSiteMapProvider_IncludeAssembliesForScan" value=""/>
  <add key="MvcSiteMapProvider_AttributesToIgnore" value=""/>
  <add key="MvcSiteMapProvider_CacheDuration" value="5"/>
  <add key="MvcSiteMapProvider_ExcludeNamespacesForResolver" value=""/>
  <add key="MvcSiteMapProvider_DefaultSiteMapNodeVisibiltyProvider" value=""/>
</appSettings>

All of these settings are completely optional. If not provided, the defaults are exactly what is configured above. Note that if no DefaultSiteMapNodeProvider is configured, visibility will always be true.

If you are using an external DI container, the MvcSiteMapProvider_UseExternalDIContainer setting must be set to true, and all other configuration will be ignored. This is because we assume the DI setup will be providing the configuration. You are free to make your DI setup read from appSettings or a custom web.config section if that is what you prefer.

You probably also are wondering how to set the securityTrimmingEnabled setting. That is now a part of the Mvc.sitemap schema - you have to add it to the <mvcSiteMap> element right after the namespaces. This allows you to set the security trimming separately in each .sitemap file if there is more than one (multiple sitemap files - or other sources - are only supported if using an external DI container).

<mvcSiteMap xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
            xmlns="http://mvcsitemap.codeplex.com/schemas/MvcSiteMap-File-4.0"
            enableLocalization="true"
            securityTrimmingEnabled="true">

Actually, it looks like that is what you are missing here. However, we'd appreciate if you report anything else that looks like it doesn't work as there has been a lot of major refactoring (which is why we are still in beta).

NightOwl888 avatar Jun 23 '13 12:06 NightOwl888

wow ok :-)

I thought to load multiple instances of the same sitemap file, one with security trimming enabled, another one without security trimming.

Now, with the new architecture, I suppose I can load the sitemap regardless of the security trimming.

I have this :

<mvcSiteMap xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
        xmlns="http://mvcsitemap.codeplex.com/schemas/MvcSiteMap-File-4.0"
        xsi:schemaLocation="http://mvcsitemap.codeplex.com/schemas/MvcSiteMap-File-4.0 MvcSiteMapSchema.xsd"
        enableLocalization="true" securityTrimmingEnabled="true">

  <mvcSiteMapNode title="Home" controller="Home" action="Index">
    <mvcSiteMapNode title="About" controller="Home" action="About"/>
    <mvcSiteMapNode title="Page1" controller="Home" action="Page1"/>
    <mvcSiteMapNode title="Page2" controller="Home" action="Page2" roles="nonExistentRole"/>
    <mvcSiteMapNode title="Page3" controller="Home" action="Page3"/>
  </mvcSiteMapNode>

</mvcSiteMap>

The sitemap menu is correctly trimmed in regard of security. I would like to be capable of walking programmatically this sitemap without security trimming. How can do I this ?

Is there any documentation to learn about the new architecture ?

eric-b avatar Jun 23 '13 15:06 eric-b

You are correct, in order to walk the sitemap without security trimming, you can use a second sitemap tree with security trimming disabled. To accomplish this, you will need to do a couple of things.

First of all, note that this is an advanced scenario that will require an external DI setup in order to configure it. You can use any DI container you want, but we have provided the default wiring code for Autofac, Ninject, StructureMap, Unity, and Castle Windsor. Only the most basic setup - a single sitemap using a single file with a few options - can be accomplished without an external DI container.

You need to decide how requests will be sent to one sitemap vs the other. This can be accomplished by implementing either 1 or 2 interfaces. ISiteMapCacheKeyGenerator is what controls when to create a new cached sitemap - a new key will yield a new sitemap. Then ISiteMapCacheKeyToBuilderSetMapper (what a mouthful) is what, well, maps a sitemap cache key to an implementation of IBuilderSet. There can be 1 or many builder sets - all depends on how many different ways the application will need to build a tree. Each builder set is given a name in the configuration and ISiteMapCacheKeyToBuilderSetMapper is what decides which key calls which builder set/cache. The builder set is what controls the cache as well as way a site map tree is built.

The other thing you will need to do is set up a custom implementation of ISiteMapBuilder to actually build the second site map tree with security trimming disabled. Unfortunately, SecurityTrimmingEnabled is literally the only setting that is locked down - once it is enabled, it cannot be disabled again, otherwise there would be a quick and easy shortcut. However, what you will need to do is copy the entire implementation of XmlSiteMapBuilder and set it up so it gets the SecurityTrimmingEnabled value through a constructor parameter rather than the XML file so you can control it via dependency injection config.

Once you have your classes implemented then it is simply a matter of altering the DI configuration to wire them up. Here is how that would be done in StructureMap:

            // Setup new cache key generator

            this.For<ISiteMapCacheKeyGenerator>()
                .Use<CustomSiteMapCacheKeyGenerator>();

            // Setup the new mapper

            this.For<ISiteMapCacheKeyToBuilderSetMappper>()
                .Use<CustomSiteMapCacheKeyToBuilderSetMapper>();

            // Setup cache

            this.For<System.Runtime.Caching.ObjectCache>()
                .Use(s => System.Runtime.Caching.MemoryCache.Default);

            this.For<ICacheProvider<ISiteMap>>().Use<RuntimeCacheProvider<ISiteMap>>();

            var cacheDependency =
                this.For<ICacheDependency>().Use<RuntimeFileCacheDependency>()
                    .Ctor<string>("fileName").Is(absoluteFileName);

            var cacheDetails =
                this.For<ICacheDetails>().Use<CacheDetails>()
                    .Ctor<TimeSpan>("absoluteCacheExpiration").Is(absoluteCacheExpiration)
                    .Ctor<TimeSpan>("slidingCacheExpiration").Is(TimeSpan.MinValue)
                    .Ctor<ICacheDependency>().Is(cacheDependency);

            // Configure the visitors
            this.For<ISiteMapNodeVisitor>()
                .Use<UrlResolvingSiteMapNodeVisitor>();


            // Register the sitemap builders
            var xmlSource = this.For<IXmlSource>().Use<FileXmlSource>()
                           .Ctor<string>("fileName").Is(absoluteFileName);

            var reservedAttributeNameProvider = this.For<ISiteMapXmlReservedAttributeNameProvider>()
                .Use<SiteMapXmlReservedAttributeNameProvider>()
                .Ctor<IEnumerable<string>>("attributesToIgnore").Is(new string[0]);

            var builder1 = this.For<ISiteMapBuilder>().Use<CompositeSiteMapBuilder>()
                .EnumerableOf<ISiteMapBuilder>().Contains(y =>
                {
                    y.Type<XmlSiteMapBuilder>()
                        .Ctor<ISiteMapXmlReservedAttributeNameProvider>().Is(reservedAttributeNameProvider)
                        .Ctor<IXmlSource>().Is(xmlSource);
                    y.Type<VisitingSiteMapBuilder>();
                });

            // This is where you would put your custom builder
            var builder2 = this.For<ISiteMapBuilder>().Use<CompositeSiteMapBuilder>()
                .EnumerableOf<ISiteMapBuilder>().Contains(y =>
                {
                    y.Type<CustomXmlSiteMapBuilder>()
                        .Ctor<ISiteMapXmlReservedAttributeNameProvider>().Is(reservedAttributeNameProvider)
                        .Ctor<IXmlSource>().Is(xmlSource)
                        .Ctor<bool>("securityTrimmingEnabled").Is(false);
                    y.Type<VisitingSiteMapBuilder>();
                });

            // Configure the builder sets
            this.For<ISiteMapBuilderSetStrategy>().Use<SiteMapBuilderSetStrategy>()
                .EnumerableOf<ISiteMapBuilderSet>().Contains(x =>
                {
                    x.Type<SiteMapBuilderSet>()
                        .Ctor<string>("instanceName").Is("default")
                        .Ctor<ISiteMapBuilder>().Is(builder1)
                        .Ctor<ICacheDetails>().Is(cacheDetails);
                    x.Type<SiteMapBuilderSet>()
                        .Ctor<string>("instanceName").Is("noSecurityTrimming")
                        .Ctor<ISiteMapBuilder>().Is(builder2)
                        .Ctor<ICacheDetails>().Is(cacheDetails);
                });

If you compare this against the MvcSiteMapProviderRegistry.cs file that we ship as the default, you can more clearly see what has changed from a default setup. Note you will need to drag along the other stuff from that file too and replace everything after the comment "setup cache" with the above, I abbreviated it here for clarity.

And no, there is currently no documentation on configuration. Please open a new issue if you see something specific that should have documentation that currently doesn't.

Also, I highly recommend the book Dependency Injection in .NET by Mark Seemann, as the approach I followed to DI is precisely what he is preaching. Not only is it a good DI book, but it also has some really good advice about how to refactor code, which makes it invaluable.

NightOwl888 avatar Jun 23 '13 18:06 NightOwl888

Actually, I gave this more thought and there are 2 options you have that are easier than re-implementing an entire class.

Option 1:

Following the above approach, it is not possible to turn off security trimming, but it is possible to turn it on. You can therefore set securityTrimmingEnabled = "false" in your .sitemap file and let your custom builder turn it on.

public class EnableSecurityTrimmingSiteMapBuilder
    : ISiteMapBuilder
{

        #region ISiteMapBuilder Members

        public virtual ISiteMapNode BuildSiteMap(ISiteMap siteMap, ISiteMapNode rootNode)
        {
            siteMap.SecurityTrimmingEnabled = true;

            return rootNode;
        }

        #endregion
}

You can then inject this class after the standard implementation and it will turn on security trimming for the "normal" case. Naturally, you would also want to have an "administrator" builder set where this class is not injected:

    var builder1 = this.For<ISiteMapBuilder>().Use<CompositeSiteMapBuilder>()
        .EnumerableOf<ISiteMapBuilder>().Contains(y =>
        {
            y.Type<XmlSiteMapBuilder>()
                .Ctor<ISiteMapXmlReservedAttributeNameProvider>().Is(reservedAttributeNameProvider)
                .Ctor<IXmlSource>().Is(xmlSource);
            y.Type<VisitingSiteMapBuilder>();
        });

    // This is where you would put your custom builder
    var builder2 = this.For<ISiteMapBuilder>().Use<CompositeSiteMapBuilder>()
        .EnumerableOf<ISiteMapBuilder>().Contains(y =>
        {
            y.Type<XmlSiteMapBuilder>()
                .Ctor<ISiteMapXmlReservedAttributeNameProvider>().Is(reservedAttributeNameProvider)
                .Ctor<IXmlSource>().Is(xmlSource);
            y.Type<EnableSecurityTrimmingSiteMapBuilder>();
            y.Type<VisitingSiteMapBuilder>();
        });

I failed to mention before that you need to use the SiteMaps static class to access a sitemap with a specific site map cache key:

    ISiteMap siteMap = SiteMaps.GetSiteMap("mySiteMapKey");

Then of course, you can use the siteMap object to iterate the nodes recursively.

Note you can also invalidate the site map cache using a similar call (in case you made a change to some data that you want to appear in the site map immediately).

    SiteMaps.ReleaseSiteMap("mySiteMapKey");

Option 2:

This is even simpler. Rather than implementing 3 interfaces, you would only need to implement 1 - IAclModule. This assumes that you can determine whether you are in your "special administrative mode" from ambient context (httpContext, file system, database, etc).

You can use the decorator pattern to make your rule apply to everything really easily, like this:

public class SpecialAdministrativeAclModule
    : IAclModule
{
        public SpecialAdministrativeAclModule(
            IAclModule innerAclModule
            )
        {
            if (innerAclModule == null)
                throw new ArgumentNullException("innerAclModule");
            this.innerAclModule = innerAclModule;
        }
        private readonly IAclModule innerAclModule;

        #region IAclModule Members

        public virtual bool IsAccessibleToUser(ISiteMap siteMap, ISiteMapNode node)
        {
            bool isSpecialAdministrativeMode = false;

            // Put your logic here - set isSpecialAdministrativeMode to true if we are in the mode.

            if (isSpecialAdministrativeMode)
                return true;
            else
                return innerAclModule.IsAccessibleToUser(siteMap, node);
        }

        #endregion
}

And then inject this to wrap the "normal" ACL logic:

    // Configure Security
    var normalAclModule = 
        this.For<IAclModule>().Use<CompositeAclModule>()
            .EnumerableOf<IAclModule>().Contains(x =>
            {
                x.Type<AuthorizeAttributeAclModule>();
                x.Type<XmlRolesAclModule>();
            });

    this.For<IAclModule>().Use<SpecialAdministrativeAclModule>()
        .Ctor<IAclModule>().Is(normalAclModule);

NightOwl888 avatar Jun 24 '13 07:06 NightOwl888

It works :-). Note that, due to a lack of time, I did it on a very basic sample project.

I followed your initial recipe which includes :

  • Setup of an external DI (I like Simple Injector)
  • Override of the default SiteMapCacheKeyToBuilderSetMapper to allow selection of a builder set based on cache key.
  • Override of XmlSiteMapBuilder to replace default implementation of the method SetSecurityTrimmingEnabled (no action). It's not required to copy/update the entire implementation. I could add securityTrimming in a constructor parameter but in my case, it simplifies DI setup.

In my DI container, I registered two CompositeSiteMapBuilder : the first is the default implementation, the second uses my "FullAccessXmlSiteMapBuilder" in place of default XmlSiteMapBuilder.

public class CustomSiteMapCacheKeyToBuilderSetMapper : SiteMapCacheKeyToBuilderSetMapper
{
    public const string AdminBuilderSet = "adminBuilderSet";

    public override string GetBuilderSetName(string cacheKey)
    {
        return cacheKey == AdminBuilderSet ? cacheKey : base.GetBuilderSetName(cacheKey);
    }
}

public class FullAccessXmlSiteMapBuilder : XmlSiteMapBuilder
{
    public FullAccessXmlSiteMapBuilder(IXmlSource xmlSource, ISiteMapXmlReservedAttributeNameProvider reservedAttributeNameProvider, INodeKeyGenerator nodeKeyGenerator, IDynamicNodeBuilder dynamicNodeBuilder, ISiteMapNodeFactory siteMapNodeFactory, ISiteMapXmlNameProvider xmlNameProvider)
        : base(xmlSource, reservedAttributeNameProvider, nodeKeyGenerator, dynamicNodeBuilder, siteMapNodeFactory, xmlNameProvider)
    {

    }

    protected override void SetSecurityTrimmingEnabled(ISiteMap siteMap, System.Xml.Linq.XDocument xml)
    {
        // no action!
    }
}

Finally, an excerpt of my DI setup :

private static IEnumerable<ISiteMapBuilderSet> ResolveISiteMapBuilderSets(Container container)
{

    yield return new SiteMapBuilderSet(
    "default",
    new CompositeSiteMapBuilder(
        container.GetInstance<XmlSiteMapBuilder>(),
        container.GetInstance<ReflectionSiteMapBuilder>(),
        container.GetInstance<VisitingSiteMapBuilder>()),
    container.GetInstance<ICacheDetails>());

    yield return new SiteMapBuilderSet(
    CustomSiteMapCacheKeyToBuilderSetMapper.AdminBuilderSet,
    new CompositeSiteMapBuilder(
        container.GetInstance<FullAccessXmlSiteMapBuilder>(),
        container.GetInstance<ReflectionSiteMapBuilder>(),
        container.GetInstance<VisitingSiteMapBuilder>()),
    container.GetInstance<ICacheDetails>());
}

I tested it with the Html helper :

@Html.MvcSiteMap().SiteMap() // default @Html.MvcSiteMap(CustomSiteMapCacheKeyToBuilderSetMapper.AdminBuilderSet).SiteMap() // ignore security trimming

The whole process seems to be a bit complicated from scratch. But I have to admit that from an existing project that already defines an external DI setup, it's incredibly simple.

Thank you also for the note about the book of Mark Seemann that I did'nt read. I'm already an advocate of this approach. It's always nice to have reading recommandations from other developers.

I have new questions if you have some more time :

  1. ReflectionSiteMapBuilder and VisitingSiteMapBuilder: these classes do not have a security trimming parameter ?

  2. I'm not familiar with the integrated DI containers (Autofac, Ninject, StructureMap, Unity, and Castle). Are there any singleton or per web request instance ? If so, could you give me a list of them ? I have essentially set transient instances in my DI setup.

Finally, I wish to share a thought : in your place, I would have replaced the constructors that take as a parameter an array of instances with an IEnumerable<T> (instead of T[]). I do not know if it is a major change, but this would have facilitated Simple Injector integration and it would have permitted, I think, better performances where all instances are eventually not used. It is the case in my advanced scenario : only some administration pages need to walk all the sitemap regardless security trimming. However, this is surely not significant in term of performances (in my use case).

Thank you again for your time.

eric-b avatar Jun 24 '13 21:06 eric-b

  1. ReflectionSiteMapBuilder and VisitingSiteMapBuilder: these classes do not have a security trimming parameter ?

That is a good point, perhaps this is something that should be added since now it is technically possible to run it without the XmlSiteMapBuilder completely, in which case one of the other builders must set it.

But actually there may be another bug here, it would make more sense if turning this setting on were moved to a later step after building is complete. This is because depending on the permission of the user that ends up building the cache, the builders might not be able to completely work because of access restrictions.

However, the VisitingSiteMapBuilder is just reusing the ISiteMapBuilder interface to do something different. Maarten wanted to have a "final phase" where a visitor could do last minute clean up on the nodes. One such task is to pre-resolve the URLs, which is why that is in there. In any case, there is no point in setting the security trimming in this stage, because there is actually no building going on.

  1. I'm not familiar with the integrated DI containers (Autofac, Ninject, StructureMap, Unity, and Castle). Are there any singleton or per web request instance ? If so, could you give me a list of them ? I have essentially set transient instances in my DI setup.

Actually, it would probably be best to set everything up as singleton EXCEPT XmlSiteMapController, since all controllers must be transient or HTTP request scoped. Although, the MvcSiteMapProvider still performs well if all objects are transient, it is most important that the providers (ISiteMapNodeUrlResolver, ISiteMapNodeVisibilityProvider, and IDynamicNodeProvider) be singleton so they are shared between all of the nodes - that could save a good piece of memory depending on how many nodes there are (or how many sitemaps there are).

Finally, I wish to share a thought : in your place, I would have replaced the constructors that take as a parameter an array of instances with an IEnumerable (instead of T[]). I do not know if it is a major change, but this would have facilitated Simple Injector integration and it would have permitted, I think, better performances where all instances are eventually not used. It is the case in my advanced scenario : only some administration pages need to walk all the sitemap regardless security trimming. However, this is surely not significant in term of performances (in my use case).

The decision to go this route actually stemmed from the fact that some DI containers require extra setup to inject into an IEnumerable<> than an array. Autofac, Ninject, and StructureMap natively inject all registered instances into a constructor parameter of type T[] and there is not any configuration code required other than registering the objects.

Before you mentioned it, I had never heard of Simple Injector, but if it suits your taste that is fine. We chose the DI containers to include by their popularity (Ninject is the 9th most popular download on Nuget gallery), but if you would like to donate some of your time to contribute another DI setup for Simple Injector we have no objections.

Anyway, thanks for the valuable feedback on your experience.

NightOwl888 avatar Jun 24 '13 23:06 NightOwl888

  1. That is a good point, perhaps this is something that should be added since now it is technically possible to run it without the XmlSiteMapBuilder completely, in which case one of the other builders must set it.

But actually there may be another bug here, it would make more sense if turning this setting on were moved to a later step after building is complete. This is because depending on the permission of the user that ends up building the cache, the builders might not be able to completely work because of access restrictions.

Ok, so this is something to keep in mind. My advanced scenario should work in most simple cases (xml sitemap) :-).

  1. Actually, it would probably be best to set everything up as singleton EXCEPT XmlSiteMapController, since all controllers must be transient or HTTP request scoped. Although, the MvcSiteMapProvider still performs well if all objects are transient, it is most important that the providers (ISiteMapNodeUrlResolver, ISiteMapNodeVisibilityProvider, and IDynamicNodeProvider) be singleton so they are shared between all of the nodes - that could save a good piece of memory depending on how many nodes there are (or how many sitemaps there are).

Thanks, it's very clear.

The decision to go this route actually stemmed from the fact that some DI containers require extra setup to inject into an IEnumerable<> than an array. Autofac, Ninject, and StructureMap natively inject all registered instances into a constructor parameter of type T[] and there is not any configuration code required other than registering the objects.

I understand. Unfortunately, it's exactly the contrary for Simple Injector, so I have to register constructors which contain array parameters with a delegate.

I will don't hesitate to contribute if I have the time to make a clean Simple Injector registration.

FYI, a good IoC container benchmark : http://www.palmmedia.de/Blog/2011/8/30/ioc-container-benchmark-performance-comparison It contains a lots of containers and I think it is a very good reference since its author makes updates regularly. Some containers are less popular indeed (because younger), but worth a try.

eric-b avatar Jun 25 '13 10:06 eric-b

I understand. Unfortunately, it's exactly the contrary for Simple Injector, so I have to register constructors which contain array parameters with a delegate.

If you have any bright ideas, I am open to suggestions. I can't think of a reasonable way to support both approaches. It is much easier to design an API targeted to a specific DI container than one that is open to any of them.

I will don't hesitate to contribute if I have the time to make a clean Simple Injector registration.

Just follow the conventions used for the other DI containers and it should be pretty straightforward.

  • Please use our custom conventions to scan and auto-wire the "regular" case where we have 1 implementation with the same name as the interface and the case where we want to register all instances of providers (as it is in the other DI modules). I realize there is probably some sweet line of code that could replace all that, but then, learning to be an expert on every DI container's auto registration syntax (and quirks) is virtually impossible - keep the support aspect in mind.
  • The general idea is to supply a standard working implementation out of the box with "the usual" stuff that was included in v3, with the option to customize the code as needed.
  • The conditional symbols are required to ensure we can support every version of MVC >= 2 and .NET >= 3.5. These are processed by the Nuget packaging script to create a different version of code for each MVC/.NET combination.
  • You can add or remove modules to the project as needed, but always start the namespace with the name of the root folder, not the project. For example, you will need to change namespace SimpleInjector.DI.SimpleInjector.Modulesto namespace DI.SimpleInjector.Modules.
  • The build process will not automatically include any files located outside of the App_Start and DI folders.
  • The DI container wrapper class must be named SimpleInjectorDependencyInjectionContainer and implement IDependencyInjectionContainer.
  • We use the MvcMusicStore demo to test the DI config to see if it is working. You will need to modify the MvcMusicStore.csproj file manually to add logic to include dependent files depending on the compilation symbol for "SimpleInjector". The code can be run in debug mode by adding "SimpleInjector" (without the quotes) as a conditional compilation symbol in the MvcMusicStore project properties.
  • We support both IControllerFactory and IDependencyResolver MVC integration points for DI. Both are wired up already and IControllerFactory is the default. To test IDependencyResolver, add "DependencyResolver" (without the quotes) as a compilation symbol to the MvcMusicStore project properties. Note that IControllerFactory and IDependencyResolver cannot be used at the same time in MVC.
  • Changing the build to support a new container can be done by adding a new ".newtrans" file following the convention used in the \nuget\mvcsitemapprovider.di.modules\ directory with the proper dependencies and adding the new package name "SimpleInjector" to the runbuild.ps1 file at this line: Create-DIContainer-Packages ("Autofac", "Ninject", "StructureMap", "Unity", "Windsor"). Do note they are ordered alphabetically.

FYI, a good IoC container benchmark : http://www.palmmedia.de/Blog/2011/8/30/ioc-container-benchmark-performance-comparison It contains a lots of containers and I think it is a very good reference since its author makes updates regularly. Some containers are less popular indeed (because younger), but worth a try.

Thanks for the link. I sure hope we don't have to support all of those containers :). I had no idea that Ninject was the slowest container.

But then, the charts don't really consider that much. I chose StructureMap because of its ability to use conventions on setter injection, and nice clean separation of concerns when it comes to customizing conventions and making modules. Not to mention, its complete feature set including custom lifetimes and interception.

One really great feature is the ability to return a "smart instance" from registering one component and adding it as a variable to the constructor of another component. In most other containers, you have to use a "named instance" to achieve the same, but you have to wait until runtime to see if that is going to fail.

However, admittedly the documentation for StructureMap is not very good and finding the correct syntax to use can sometimes be challenging. That has been my experience with MVC as well - it seems like most of the documentation out there is on prerelease versions where the proposed solution no longer works.

NightOwl888 avatar Jun 25 '13 13:06 NightOwl888

Ok, I will follow these rules. I think I will be able to submit something in a few days.

eric-b avatar Jun 25 '13 18:06 eric-b

  1. ReflectionSiteMapBuilder and VisitingSiteMapBuilder: these classes do not have a security trimming parameter ?

That is a good point, perhaps this is something that should be added since now it is technically possible to run it without the XmlSiteMapBuilder completely, in which case one of the other builders must set it.

What do you think about the idea of moving the SecurityTrimmingEnabled and EnableLocalization settings to the SiteMap constructor, in which case you would need to provide these settings in the DI setup to the SiteMapBuilderSet (which would pass them on down during the loader phase). Both of the settings could then just be read-only on the SiteMap object. This would fix any security concerns about the security trimming setting being changed during runtime, and also means there is a way to set the settings 1 time per site map. The builders wouldn't have anything to do with these settings in this case.

NightOwl888 avatar Jul 01 '13 13:07 NightOwl888

Can't we have something simple like SiteMaps.Current.GetAllNodes(true/false); when true could return with security trimmed and when false return all nodes?

Why? Because the current approach forces us to use external DI

rvramesh avatar May 20 '14 12:05 rvramesh

Yes it can be done, but no, it is not simple.

For one, the SiteMap is a hierarchy, not a list. If you return all of the nodes using a query like this it would not be possible to tell how the nodes are related to one another. There was another request similar to this (to view the nodes in a list instead of a hierarchy), but is that really what you are after?

Secondly, the security trimming is enforced at every level of the hierarchy (in several methods of the SiteMap class), so passing a single boolean parameter like this would effectively mean changing the interface of a large number of methods in the SiteMap class in order to pass this value through. Microsoft's model (which this is based on) didn't allow for ways around the security. In fact, even if you return the nodes in a list (as mentioned above) the ParentNode and ChildNodes properties will still enforce security trimming, so that really won't be a complete solution anyway.

On top of this, the HTML helpers also repeat the check for security trimming. I am not sure why that was done, as that "feature" was carried over from v3. So even if the problem is solved with the SiteMap object, it would require custom HTML helper models to be complete (assuming you are looking for a way to display them on the UI).

Despite these problems, I think it is a good idea to add the ability to view the whole hierarchy (and will need to be done anyway to support #188), so I am reopening this issue. That said, it is not likely to be implemented anytime soon because of the number of breaking changes that will be required.

For now, implementing a custom IAclModule is the simplest way. A possible alternative is to use a special role for the case that you need to view all of the nodes and assign that role to every AuthorizeAttribute.

NightOwl888 avatar May 22 '14 18:05 NightOwl888

I am also in need of the hierarchy. GetNodes can return the entire hierarchy by traversing the sitemap once without applying security trimming or taking the current user in context.

And whether the node is accessible to current user has to be done by passing the node to the SiteMaps.Current.IsAccessibleToUser.

I understand this is a nice to have feature request and not expecting it to be available immediately. Also, would like to Thank you for such a library and taking time to support it.

rvramesh avatar May 24 '14 02:05 rvramesh