Struts Menu
  1. Struts Menu
  2. SM-58

VelocityDisplayer: Removing menu items for a particular role effects all users

    Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.4.1
    • Fix Version/s: 2.4.2
    • Component/s: Displayers
    • Labels:
      None

      Description

      Description from AppFuse mailing list:

      Add a new menu called "CicosopMenu"
      and in one of the item tags "EditSopDocuments" added
      roles="admin".

      Now it works fine if I login as admin and shows
      all the links; logout and log back in as 'user' (hides
      the link with tag EditSopDocuments). But when I login
      as admin again, the link with tag EditSopDocuments is
      not shown. If I restart my server, then the same
      process follows.

      http://www.nabble.com/Re%3A-Problem-with-access-control-in-top-menu-p5715180s2369.html

        Activity

        Hide
        Robert Lang added a comment -

        Hello. I too am having this issue.

        I believe that the problem lies in the fact that MenuComponent's original list is being modified. When the list is retrieved again, the list no longer contains all of the items.

        Here's the offending code from VelocityMenuDisplayer:

        menu.getComponents().clear();
        menu.getComponents().addAll(componentsAllowed);

        So, if the original list had 5 items and one person only has access to one, the original list now only has one item. Cloning will not help.

        I repaired this by keeping a "proto" list separate from the component list. Instead of asking for menuComponents() in the Displayer, it asks for the proto list, then sets the menuComponents list as usual.

        It's a bit of a hack, but it allows me to move on. Here's the revelant source:

        Component.java

        protected List menuComponents = Collections.synchronizedList(new ArrayList());
        protected List protoMenuComponents = Collections.synchronizedList(new ArrayList());
        protected MenuComponent parentMenu;
        private boolean last;
        private String breadCrumb;

        //~ Methods ================================================================

        public void addMenuComponent(MenuComponent menuComponent) {
        if ((menuComponent.getName() == null) || (menuComponent.getName().equals("")))

        { menuComponent.setName(this.name + menuComponents.size()); }

        if (!menuComponents.contains(menuComponent))

        { menuComponents.add(menuComponent); menuComponent.setParent(this); }

        if (!protoMenuComponents.contains(menuComponent))

        { protoMenuComponents.add(menuComponent); menuComponent.setParent(this); }

        }

        public MenuComponent[] getMenuComponents()

        { return (MenuComponent[]) menuComponents.toArray(_menuComponent); }

        public MenuComponent[] getProtoMenuComponents()

        { return (MenuComponent[]) protoMenuComponents.toArray(_menuComponent); }

        public void setParent(MenuComponent parentMenu) {
        if (parentMenu != null) {
        // look up the parent and make sure that it has this menu as a child
        if (!parentMenu.getComponents().contains(this))

        { parentMenu.addMenuComponent(this); }

        }
        this.parentMenu = parentMenu;
        }

        public MenuComponent getParent()

        { return parentMenu; }

        /**

        • Convenience method for Velocity templates
          */
          public List getComponents() { return menuComponents; }

          /**
          * Convenience method for Velocity templates
          */
          public List getProtoComponents() { return menuComponents; }

        From VelocityMenuDislayer.java:

        private void updateAllowedComponents(MenuComponent menu) {
        MenuComponent[] components = menu.getProtoMenuComponents();

        if (components.length > 0) {
        List componentsAllowed = new ArrayList();

        for (int i = 0; i < components.length; i++) {
        if (isAllowed(components[i]))

        { componentsAllowed.add(components[i]); updateAllowedComponents(components[i]); }

        }

        menu.getComponents().clear();
        menu.getComponents().addAll(componentsAllowed);
        }
        }

        That's for the great menu.

        Robert

        Show
        Robert Lang added a comment - Hello. I too am having this issue. I believe that the problem lies in the fact that MenuComponent's original list is being modified. When the list is retrieved again, the list no longer contains all of the items. Here's the offending code from VelocityMenuDisplayer: menu.getComponents().clear(); menu.getComponents().addAll(componentsAllowed); So, if the original list had 5 items and one person only has access to one, the original list now only has one item. Cloning will not help. I repaired this by keeping a "proto" list separate from the component list. Instead of asking for menuComponents() in the Displayer, it asks for the proto list, then sets the menuComponents list as usual. It's a bit of a hack, but it allows me to move on. Here's the revelant source: Component.java protected List menuComponents = Collections.synchronizedList(new ArrayList()); protected List protoMenuComponents = Collections.synchronizedList(new ArrayList()); protected MenuComponent parentMenu; private boolean last; private String breadCrumb; //~ Methods ================================================================ public void addMenuComponent(MenuComponent menuComponent) { if ((menuComponent.getName() == null) || (menuComponent.getName().equals(""))) { menuComponent.setName(this.name + menuComponents.size()); } if (!menuComponents.contains(menuComponent)) { menuComponents.add(menuComponent); menuComponent.setParent(this); } if (!protoMenuComponents.contains(menuComponent)) { protoMenuComponents.add(menuComponent); menuComponent.setParent(this); } } public MenuComponent[] getMenuComponents() { return (MenuComponent[]) menuComponents.toArray(_menuComponent); } public MenuComponent[] getProtoMenuComponents() { return (MenuComponent[]) protoMenuComponents.toArray(_menuComponent); } public void setParent(MenuComponent parentMenu) { if (parentMenu != null) { // look up the parent and make sure that it has this menu as a child if (!parentMenu.getComponents().contains(this)) { parentMenu.addMenuComponent(this); } } this.parentMenu = parentMenu; } public MenuComponent getParent() { return parentMenu; } /** Convenience method for Velocity templates */ public List getComponents() { return menuComponents; } /** * Convenience method for Velocity templates */ public List getProtoComponents() { return menuComponents; } From VelocityMenuDislayer.java: private void updateAllowedComponents(MenuComponent menu) { MenuComponent[] components = menu.getProtoMenuComponents(); if (components.length > 0) { List componentsAllowed = new ArrayList(); for (int i = 0; i < components.length; i++) { if (isAllowed(components [i] )) { componentsAllowed.add(components[i]); updateAllowedComponents(components[i]); } } menu.getComponents().clear(); menu.getComponents().addAll(componentsAllowed); } } That's for the great menu. Robert
        Hide
        Robert Lang added a comment -

        ooops. I didn't think this through. You fellas are correct, the MenuComponent object needs to be cloned. All that I've managed to do is introduce a race condition. Sorry. I'll implement the clone and give that a go.

        Robert

        Show
        Robert Lang added a comment - ooops. I didn't think this through. You fellas are correct, the MenuComponent object needs to be cloned. All that I've managed to do is introduce a race condition. Sorry. I'll implement the clone and give that a go. Robert
        Hide
        Matt Raible added a comment -

        roc.bhakta - your fix worked, thanks.

        Show
        Matt Raible added a comment - roc.bhakta - your fix worked, thanks.
        Hide
        Matt Raible added a comment -

        Latest fix in CVS works for Velocity-based menus, but ends up modifying menus in repository as well - resulting in duplicates if you're using anything other than Velocity-based menus. I'm going to try putting the permission check into globalMacros.vm and the templates themselves for optimization.

        Show
        Matt Raible added a comment - Latest fix in CVS works for Velocity-based menus, but ends up modifying menus in repository as well - resulting in duplicates if you're using anything other than Velocity-based menus. I'm going to try putting the permission check into globalMacros.vm and the templates themselves for optimization.
        Hide
        Matt Raible added a comment -

        After struggling with cloning for several hours, and even trying to get deep-copy cloning (http://javatechniques.com/public/java/docs/basics/faster-deep-copy.html) to work - I decided it's easier to call $displayer.isAllowed($menu)) in the Velocity macros themselves. I added this by default to globalMacros.vm and moved the file to be in net.sf.navigator.displayer. You can override the displayMenu macro in this file by putting a menuMacros.vm file at the root of your classpath.

        For templates (in the sample app) that don't use this macro - i.e. cssMenu.html - it's important to put the permission check w/in the macro. If you put the permission check surrounding the macro call, it doesn't work. For example, the following doesn't work:

        #macro( displayCssMenu $menu )

          1. set menu title
            #set ($title = $displayer.getMessage($menu.title))

        #if (!$menu.url) #set ($url="javascript.void(0)") #else #set ($url=$menu.url) #end

          1. create a single menu item
            #if ($menu.components.size() == 0)
            #if ($velocityCount == $menu.parent.components.size())
            <li class="last">
            #else
            <li>
            #end
            #if ($menu.name == $currentMenu)
            <a href="$url" title="$title" class="current" #if($menu.target)target="$menu.target"#end#if($menu.width)style="width: $ {menu.width}px"#end>${title}</a>
            #else
            <a href="$url" title="$title" #if($menu.target)target="$menu.target"#end#if($menu.width)style="width: ${menu.width}

            px"#end>$

            {title}</a>
            #end
            #else ## create multiple menu items in a menu
            <li class="menubar">
            <a href="$url" title="$title" #if ($menu.name == $currentMenu) class="current" #end#if($menu.target)target="$menu.target"#end#if($menu.width)style="width: ${menu.width}px"#end>${title}

            </a>
            #end

        #if ($menu.components.size() > 0)
        <ul>
        #foreach ($menuIt in $menu.components)
        #displayCssMenu($menuIt)
        #end
        </li>
        #else
        </li>
        #if ($velocityCount == $menu.parent.components.size())
        </ul>
        #end
        #end
        #end

        #if ($displayer.isAllowed($menu))
        #displayCssMenu($menu)
        #end

        But this one does:

        #macro( displayCssMenu $menu )
        #if ($displayer.isAllowed($menu))

          1. set menu title
            #set ($title = $displayer.getMessage($menu.title))

        #if (!$menu.url) #set ($url="javascript.void(0)") #else #set ($url=$menu.url) #end

          1. create a single menu item
            #if ($menu.components.size() == 0)
            #if ($velocityCount == $menu.parent.components.size())
            <li class="last">
            #else
            <li>
            #end
            #if ($menu.name == $currentMenu)
            <a href="$url" title="$title" class="current" #if($menu.target)target="$menu.target"#end#if($menu.width)style="width: $ {menu.width}px"#end>${title}</a>
            #else
            <a href="$url" title="$title" #if($menu.target)target="$menu.target"#end#if($menu.width)style="width: ${menu.width}

            px"#end>$

            {title}</a>
            #end
            #else ## create multiple menu items in a menu
            <li class="menubar">
            <a href="$url" title="$title" #if ($menu.name == $currentMenu) class="current" #end#if($menu.target)target="$menu.target"#end#if($menu.width)style="width: ${menu.width}px"#end>${title}

            </a>
            #end

        #if ($menu.components.size() > 0)
        <ul>
        #foreach ($menuIt in $menu.components)
        #displayCssMenu($menuIt)
        #end
        </li>
        #else
        </li>
        #if ($velocityCount == $menu.parent.components.size())
        </ul>
        #end
        #end
        #end
        #end

        #displayCssMenu($menu)

        Show
        Matt Raible added a comment - After struggling with cloning for several hours, and even trying to get deep-copy cloning ( http://javatechniques.com/public/java/docs/basics/faster-deep-copy.html ) to work - I decided it's easier to call $displayer.isAllowed($menu)) in the Velocity macros themselves. I added this by default to globalMacros.vm and moved the file to be in net.sf.navigator.displayer. You can override the displayMenu macro in this file by putting a menuMacros.vm file at the root of your classpath. For templates (in the sample app) that don't use this macro - i.e. cssMenu.html - it's important to put the permission check w/in the macro. If you put the permission check surrounding the macro call, it doesn't work. For example, the following doesn't work: #macro( displayCssMenu $menu ) set menu title #set ($title = $displayer.getMessage($menu.title)) #if (!$menu.url) #set ($url="javascript.void(0)") #else #set ($url=$menu.url) #end create a single menu item #if ($menu.components.size() == 0) #if ($velocityCount == $menu.parent.components.size()) <li class="last"> #else <li> #end #if ($menu.name == $currentMenu) <a href="$url" title="$title" class="current" #if($menu.target)target="$menu.target"#end#if($menu.width)style="width: $ {menu.width}px"#end>${title}</a> #else <a href="$url" title="$title" #if($menu.target)target="$menu.target"#end#if($menu.width)style="width: ${menu.width} px"#end>$ {title}</a> #end #else ## create multiple menu items in a menu <li class="menubar"> <a href="$url" title="$title" #if ($menu.name == $currentMenu) class="current" #end#if($menu.target)target="$menu.target"#end#if($menu.width)style="width: ${menu.width}px"#end>${title} </a> #end #if ($menu.components.size() > 0) <ul> #foreach ($menuIt in $menu.components) #displayCssMenu($menuIt) #end </li> #else </li> #if ($velocityCount == $menu.parent.components.size()) </ul> #end #end #end #if ($displayer.isAllowed($menu)) #displayCssMenu($menu) #end But this one does: #macro( displayCssMenu $menu ) #if ($displayer.isAllowed($menu)) set menu title #set ($title = $displayer.getMessage($menu.title)) #if (!$menu.url) #set ($url="javascript.void(0)") #else #set ($url=$menu.url) #end create a single menu item #if ($menu.components.size() == 0) #if ($velocityCount == $menu.parent.components.size()) <li class="last"> #else <li> #end #if ($menu.name == $currentMenu) <a href="$url" title="$title" class="current" #if($menu.target)target="$menu.target"#end#if($menu.width)style="width: $ {menu.width}px"#end>${title}</a> #else <a href="$url" title="$title" #if($menu.target)target="$menu.target"#end#if($menu.width)style="width: ${menu.width} px"#end>$ {title}</a> #end #else ## create multiple menu items in a menu <li class="menubar"> <a href="$url" title="$title" #if ($menu.name == $currentMenu) class="current" #end#if($menu.target)target="$menu.target"#end#if($menu.width)style="width: ${menu.width}px"#end>${title} </a> #end #if ($menu.components.size() > 0) <ul> #foreach ($menuIt in $menu.components) #displayCssMenu($menuIt) #end </li> #else </li> #if ($velocityCount == $menu.parent.components.size()) </ul> #end #end #end #end #displayCssMenu($menu)

          People

          • Assignee:
            Matt Raible
            Reporter:
            Matt Raible
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development