Struts Menu
  1. Struts Menu
  2. SM-68

PermissionAdapter not firing for lowest menu level

    Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.4.2
    • Fix Version/s: 2.4.3
    • Component/s: Displayers
    • Labels:
      None
    • Environment:
      Webapplication using Struts and Acegi in combination with struts-menu. The application runs on a Tomcat server.

      Description

      The globalMacros.vm file in the directory "struts-menu-2.4.2/src/java/net/sf/navigator/displayer" of the jar file contains an error. The recursion part of the macro doesn't check for permissions in the PermissionAdapter class on the lowest level of the menu tree.

      My workaround is to put a menuMacros.vm file into my root source folder which is attached. You should be able to reproduce the bug quite easily. Just create a menu and use the "roles" property for an entry on the lowest menu level. The problem should not only rise when using acegi but just as soon as you're going to use a PermissionAdapter.

      I hope this helps. If you need more information about how to reproduce my situation just send me a message (I hope this is possible on this system - I'm not only new to struts-menu but also to JIRA

      Cheers
      Lorenz

      1. menu-config.xml
        0.7 kB
        lorenz fischer
      2. menuMacros.vm
        0.5 kB
        lorenz fischer

        Activity

        Hide
        Matt Raible added a comment -

        I don't understand what you mean by "the lowest menu level". In your example:

        <Menu name="Settings" title="menu.settings">
        <Item name="NewUser" roles="ROLE_SUPERADMINISTRATOR" title="New User" location="welcome.do"/>
        <Item name="NewProfile" roles="ROLE_SUPERADMINISTRATOR" title="New Profile" location="welcome.do"/>
        <Item name="NewRole" roles="ROLE_SUPERADMINISTRATOR" title="New Role" location="welcome.do"/>
        </Menu>

        Do you mean that the last <Item> gets displayed w/o this patch? Even if you're logged in as a user w/o the ROLE_SUPERADMINISTRATOR role?

        Show
        Matt Raible added a comment - I don't understand what you mean by "the lowest menu level". In your example: <Menu name="Settings" title="menu.settings"> <Item name="NewUser" roles="ROLE_SUPERADMINISTRATOR" title="New User" location="welcome.do"/> <Item name="NewProfile" roles="ROLE_SUPERADMINISTRATOR" title="New Profile" location="welcome.do"/> <Item name="NewRole" roles="ROLE_SUPERADMINISTRATOR" title="New Role" location="welcome.do"/> </Menu> Do you mean that the last <Item> gets displayed w/o this patch? Even if you're logged in as a user w/o the ROLE_SUPERADMINISTRATOR role?
        Hide
        lorenz fischer added a comment -

        Hi Matt

        Yes that's what I meant. Sorry for causing confusion. As far as I'm concerned there's a bug in the macro file of version 2.4.2.

        Thanks for your great work!

        Regards
        Lorenz

        Show
        lorenz fischer added a comment - Hi Matt Yes that's what I meant. Sorry for causing confusion. As far as I'm concerned there's a bug in the macro file of version 2.4.2. Thanks for your great work! Regards Lorenz
        Hide
        Matt Raible added a comment -

        In your menuMacros.vm, you have:

        #local ($menu $level)
        #set ($level = $level+1)
        #eval("#displayMenu($menu $level)")
        #end

        However, in globalMacros.vm, the following code exists:

        #local ($menu $level)
        #set ($level = $level+1)
        #if ($menu.components.size() > 0)
        #eval("#displayMenu($menu $level)")
        #else
        #menuItem($menu $level)
        #end
        #end

        So you're saying removing the #if statement causes the problem? The question is - will removing it cause other issues? Unfortunately, this library isn't well tested so it won't be easy to see if this change will cause other issues.

        Show
        Matt Raible added a comment - In your menuMacros.vm, you have: #local ($menu $level) #set ($level = $level+1) #eval("#displayMenu($menu $level)") #end However, in globalMacros.vm, the following code exists: #local ($menu $level) #set ($level = $level+1) #if ($menu.components.size() > 0) #eval("#displayMenu($menu $level)") #else #menuItem($menu $level) #end #end So you're saying removing the #if statement causes the problem? The question is - will removing it cause other issues? Unfortunately, this library isn't well tested so it won't be easy to see if this change will cause other issues.
        Hide
        lorenz fischer added a comment -

        Yes. As I see it by removing the #if statement the recursion gets one step deeper and executes the displayMenu() method even for the last items in the tree. By having that #if statement the last items (the ones having $menu.components.size() == 0) will not be checked over the

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

        statement in the next recursion step, but just be added right away. For those "last" items the #if statement on line 3 will return false (since they don't have any children), so the #menuItem($menu $level) statement on line 13 will add them to the menu (but only if the #if statement on line 2 already returned true, which means the user is allowed to see the menu item).

        01: #macro( displayMenu $menu $level )
        02: #if ($displayer.isAllowed($menu))
        03: #if ($menu.components.size() > 0)
        04: ## display top menu
        05: #menuItem($menu $level)
        06: #foreach ($menu in $menu.components)
        07: #local ($menu $level)
        08: #set ($level = $level+1)
        09: #eval("#displayMenu($menu $level)")
        10: #end
        11: #end
        12: #else
        13: #menuItem($menu $level)
        14: #end
        15: #end
        16: #end

        (sorry but I didn't find out how to indent the lines properly)

        I can imagine that you probably don't want to put code into your project if you're not a 100% sure of if it is error prone. Unfortunately I only had one project in which I had to use struts-menu and I don't have that code handy right now Actually, you should be able to reproduce the behavior I experienced quite easily as soon as you do a test with a PermissionAdapter and require some permissions for the last items in the tree.

        However, if somebody else has problems with permissions on the last items they can just put that menuMacros.vm file into the root source folder which should overwrite the code of globalMacros.vm in the package (if I got that right). And in that case you don't have to change your code at all

        Regards
        Lorenz

        p.s. I'm sorry if my english sounds billowed (I'm not native speaker

        Show
        lorenz fischer added a comment - Yes. As I see it by removing the #if statement the recursion gets one step deeper and executes the displayMenu() method even for the last items in the tree. By having that #if statement the last items (the ones having $menu.components.size() == 0) will not be checked over the #if ($displayer.isAllowed($menu)) statement in the next recursion step, but just be added right away. For those "last" items the #if statement on line 3 will return false (since they don't have any children), so the #menuItem($menu $level) statement on line 13 will add them to the menu (but only if the #if statement on line 2 already returned true, which means the user is allowed to see the menu item). 01: #macro( displayMenu $menu $level ) 02: #if ($displayer.isAllowed($menu)) 03: #if ($menu.components.size() > 0) 04: ## display top menu 05: #menuItem($menu $level) 06: #foreach ($menu in $menu.components) 07: #local ($menu $level) 08: #set ($level = $level+1) 09: #eval("#displayMenu($menu $level)") 10: #end 11: #end 12: #else 13: #menuItem($menu $level) 14: #end 15: #end 16: #end (sorry but I didn't find out how to indent the lines properly) I can imagine that you probably don't want to put code into your project if you're not a 100% sure of if it is error prone. Unfortunately I only had one project in which I had to use struts-menu and I don't have that code handy right now Actually, you should be able to reproduce the behavior I experienced quite easily as soon as you do a test with a PermissionAdapter and require some permissions for the last items in the tree. However, if somebody else has problems with permissions on the last items they can just put that menuMacros.vm file into the root source folder which should overwrite the code of globalMacros.vm in the package (if I got that right). And in that case you don't have to change your code at all Regards Lorenz p.s. I'm sorry if my english sounds billowed (I'm not native speaker
        Hide
        Matt Raible added a comment -

        Which displayer are you using? I'm using a Velocity displayer and testing it in AppFuse. I've changed my menu-config.xml from having:

        <Menu name="AdminMenu" title="menu.admin" description="Admin Menu" roles="ROLE_ADMIN" width="120" page="/users.html">
        <Item name="ViewUsers" title="menu.admin.users" page="/users.html"/>
        <Item name="ActiveUsers" title="mainMenu.activeUsers" page="/activeUsers.html"/>
        <Item name="ReloadContext" title="menu.admin.reload" page="/reload.html"/>
        <Item name="FileUpload" title="menu.selectFile" page="/fileupload.html"/>
        <Item name="FlushCache" title="menu.flushCache" page="/flushCache.html"/>
        <Item name="Clickstream" title="menu.clickstream" page="/clickstreams.jsp"/>
        </Menu>

        to:

        <Menu name="AdminMenu" title="menu.admin" description="Admin Menu" width="120" page="/users.html">
        <Item name="ViewUsers" title="menu.admin.users" page="/users.html" roles="ROLE_ADMIN"/>
        <Item name="ActiveUsers" title="mainMenu.activeUsers" page="/activeUsers.html" roles="ROLE_ADMIN"/>
        <Item name="ReloadContext" title="menu.admin.reload" page="/reload.html" roles="ROLE_ADMIN"/>
        <Item name="FileUpload" title="menu.selectFile" page="/fileupload.html" roles="ROLE_ADMIN"/>
        <Item name="FlushCache" title="menu.flushCache" page="/flushCache.html" roles="ROLE_ADMIN"/>
        <Item name="Clickstream" title="menu.clickstream" page="/clickstreams.jsp" roles="ROLE_ADMIN"/>
        </Menu>

        My menu.jsp is as follows:

        <menu:useMenuDisplayer name="Velocity" config="cssHorizontalMenu.vm" permissions="rolesAdapter">
        <ul id="primary-nav" class="menuList">
        <li class="pad"> </li>
        <c:if test="$

        {empty pageContext.request.remoteUser}

        "><li><a href="<c:url value="/login.jsp"/>" class="current"><fmt:message key="login.title"/></a></li></c:if>
        <menu:displayMenu name="MainMenu"/>
        <menu:displayMenu name="UserMenu"/>
        <menu:displayMenu name="AdminMenu"/>
        <menu:displayMenu name="Logout"/>
        </ul>
        </menu:useMenuDisplayer>

        It doesn't seem to matter if I use the current globalMacros.vm in 2.4.2 vs. the one attached to this issue. Both result in the same "Administration" menu being displayed, but no child menus.

        Show
        Matt Raible added a comment - Which displayer are you using? I'm using a Velocity displayer and testing it in AppFuse. I've changed my menu-config.xml from having: <Menu name="AdminMenu" title="menu.admin" description="Admin Menu" roles="ROLE_ADMIN" width="120" page="/users.html"> <Item name="ViewUsers" title="menu.admin.users" page="/users.html"/> <Item name="ActiveUsers" title="mainMenu.activeUsers" page="/activeUsers.html"/> <Item name="ReloadContext" title="menu.admin.reload" page="/reload.html"/> <Item name="FileUpload" title="menu.selectFile" page="/fileupload.html"/> <Item name="FlushCache" title="menu.flushCache" page="/flushCache.html"/> <Item name="Clickstream" title="menu.clickstream" page="/clickstreams.jsp"/> </Menu> to: <Menu name="AdminMenu" title="menu.admin" description="Admin Menu" width="120" page="/users.html"> <Item name="ViewUsers" title="menu.admin.users" page="/users.html" roles="ROLE_ADMIN"/> <Item name="ActiveUsers" title="mainMenu.activeUsers" page="/activeUsers.html" roles="ROLE_ADMIN"/> <Item name="ReloadContext" title="menu.admin.reload" page="/reload.html" roles="ROLE_ADMIN"/> <Item name="FileUpload" title="menu.selectFile" page="/fileupload.html" roles="ROLE_ADMIN"/> <Item name="FlushCache" title="menu.flushCache" page="/flushCache.html" roles="ROLE_ADMIN"/> <Item name="Clickstream" title="menu.clickstream" page="/clickstreams.jsp" roles="ROLE_ADMIN"/> </Menu> My menu.jsp is as follows: <menu:useMenuDisplayer name="Velocity" config="cssHorizontalMenu.vm" permissions="rolesAdapter"> <ul id="primary-nav" class="menuList"> <li class="pad"> </li> <c:if test="$ {empty pageContext.request.remoteUser} "><li><a href="<c:url value="/login.jsp"/>" class="current"><fmt:message key="login.title"/></a></li></c:if> <menu:displayMenu name="MainMenu"/> <menu:displayMenu name="UserMenu"/> <menu:displayMenu name="AdminMenu"/> <menu:displayMenu name="Logout"/> </ul> </menu:useMenuDisplayer> It doesn't seem to matter if I use the current globalMacros.vm in 2.4.2 vs. the one attached to this issue. Both result in the same "Administration" menu being displayed, but no child menus.
        Hide
        Matt Raible added a comment -

        I've been able to reproduce a similar issue, but not exactly the one that you point out. If I use the menu definition above (where ROLE_ADMIN is on each <Item>)...

        index.jsp:

        <menu:useMenuDisplayer name="Velocity" config="/index.vm" permissions="rolesAdapter">
        <ul class="glassList">
        <menu:displayMenu name="AdminMenu"/>
        </ul>
        </menu:useMenuDisplayer>

        and index.vm:

          1. The displayMenu macro (at the bottom of this template) is defined in
          2. struts-menu.jar!/net/sf/displayer/globalMacros.vm. It has a callback to the #menuItem macro
          3. you see below. You can override this macro by creating a menuMacros.vm template
          4. at the root of your classpath

        #macro( menuItem $menu $level )
        #set ($title = $displayer.getMessage($menu.title))
        #if ($stringUtils.indexOf($menu.url, "velocity") > -1 && !$writeNewSeparator)
        </ul>
        <h3>New and Cool Examples</h3>
        <ul class="glassList">
        #set($writeNewSeparator = "false")
        #end
        #if ($menu.url)
        <li><a href="$!menu.url" title="$title">$title</a>
        #if ($menu.toolTip)- $!menu.toolTip #end</li>
        #end
        #end

        #displayMenu($menu 0)

        With 2.4.2, it displays all the menu items. With your change, it displays only the Administration menu item. So the problem I was able to reproduce is much worse than the one you describe. Your change fixes the problem, so I'll go ahead and apply the patch. Thanks!

        Show
        Matt Raible added a comment - I've been able to reproduce a similar issue, but not exactly the one that you point out. If I use the menu definition above (where ROLE_ADMIN is on each <Item>)... index.jsp: <menu:useMenuDisplayer name="Velocity" config="/index.vm" permissions="rolesAdapter"> <ul class="glassList"> <menu:displayMenu name="AdminMenu"/> </ul> </menu:useMenuDisplayer> and index.vm: The displayMenu macro (at the bottom of this template) is defined in struts-menu.jar!/net/sf/displayer/globalMacros.vm. It has a callback to the #menuItem macro you see below. You can override this macro by creating a menuMacros.vm template at the root of your classpath #macro( menuItem $menu $level ) #set ($title = $displayer.getMessage($menu.title)) #if ($stringUtils.indexOf($menu.url, "velocity") > -1 && !$writeNewSeparator) </ul> <h3>New and Cool Examples</h3> <ul class="glassList"> #set($writeNewSeparator = "false") #end #if ($menu.url) <li><a href="$!menu.url" title="$title">$title</a> #if ($menu.toolTip)- $!menu.toolTip #end</li> #end #end #displayMenu($menu 0) With 2.4.2, it displays all the menu items. With your change, it displays only the Administration menu item. So the problem I was able to reproduce is much worse than the one you describe. Your change fixes the problem, so I'll go ahead and apply the patch. Thanks!
        Hide
        lorenz fischer added a comment -

        Hi Matt

        I can't look at the code I was using when I wrote this issue at the moment, but if I remember correctly I was also using the velocity displayer.

        Anyways I'm glad that I could contribute something to your project

        Regrads
        Lorenz

        Show
        lorenz fischer added a comment - Hi Matt I can't look at the code I was using when I wrote this issue at the moment, but if I remember correctly I was also using the velocity displayer. Anyways I'm glad that I could contribute something to your project Regrads Lorenz

          People

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

            Dates

            • Created:
              Updated:
              Resolved: