[#SM-68] PermissionAdapter not firing for lowest menu level - AppFuse JIRA

Struts Menu

PermissionAdapter not firing for lowest menu level

Created: 31/Jan/07 04:11 AM Updated: 02/Jun/07 09:19 AM Resolved: 31/May/07 04:23 PM

Component/s:

Displayers

Affects Version/s:

2.4.2

Fix Version/s:

2.4.3

File Attachments:	1. menu-config.xml (0.7 kB) 2. menuMacros.vm (0.5 kB)
Environment:	Webapplication using Struts and Acegi in combination with struts-menu. The application runs on a Tomcat server.

Description

« Hide

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

Sort Order:

[ Permalink | « Hide ]

Matt Raible added a comment - 31/May/07 02:20 PM

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?

[ Permalink | « Hide ]

lorenz fischer added a comment - 31/May/07 02:27 PM

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

[ Permalink | « Hide ]

Matt Raible added a comment - 31/May/07 02:30 PM

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.

[ Permalink | « Hide ]

lorenz fischer added a comment - 31/May/07 03:03 PM

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 - 31/May/07 03:03 PM 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 ;)

[ Permalink | « Hide ]

Matt Raible added a comment - 31/May/07 03:33 PM

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 - 31/May/07 03:33 PM 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.

[ Permalink | « Hide ]

Matt Raible added a comment - 31/May/07 04:20 PM

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!

[ Permalink | « Hide ]

lorenz fischer added a comment - 02/Jun/07 09:19 AM

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