Struts Menu
  1. Struts Menu
  2. SM-29

Velocity Menu + role based security bug

    Details

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

      Description

      Copied from SF http://sourceforge.net/tracker/index.php?func=detail&aid=1060402&group_id=48726&atid=453974:

      Using VelocityMenuDisplayer to display menus with role
      restrictions, after loggin in as a user with more
      restrictions (less menu items visible), then logging in
      as a user with less restrictions (more menu items
      visible) : result --> the menu item that is role
      protected does not show anymore for the less restricted
      user. Cause: caching the menu probalby and the code in
      VelocityMenuDisplayer around line 175 that removes the
      item from the menu. Possible solution:
      The code below to be run against a clone instead of the
      menu object and that clone put in the context afterwards.

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


      Comments

      Date: 2005-05-20 15:48
      Sender: wrschneider99
      Logged In: YES
      user_id=768885

      One of the developers I work with arrived at a simple solution:
      eliminate this logic entirely from the Velocity displayer
      and make the template do the work. the template can just do

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

      This maintains the spirit of the Velocity displayer as
      mainly a pass-through to shove stuff into a VelocityContext.
      In most cases our menu.vm template is already recursive,
      and the VelocityMenuDisplayer was actually not handling
      permissions properly for the sub-items (items below another
      item rather than the top-level menu).

      Date: 2005-05-18 15:49
      Sender: razvan11
      Logged In: YES
      user_id=919177

      Your comment is correct. In the application for which I
      described my solution, one has to log in before he sees any
      menus rendered by StrutsMenu. With the code at hand, this
      solution was the least code intrusive. The filter builds
      and stores the menu after the login.

      Dynamically producing a role based filtered menu for each
      rendering would for sure by more appropiate.

      Date: 2005-05-18 14:16
      Sender: wrschneider99
      Logged In: YES
      user_id=768885

      Another use case to consider: your roles may change in the
      scope of a single HttpSession because you might display the
      menu before typing the username and password.

      I believe this is a legitimate bug, because it violates the
      "principle of least astonishment": a new user would
      not
      expect this kind of interaction between the provided Struts
      plug-in (or analogous Bean to use with Spring), the
      rolesAdapter and the VelocityMenuDispl.

      Also, one would think that it's odd semantics for a
      MenuDisplayer to mutate the passed-in MenuComponent.

      Date: 2005-05-18 14:06
      Sender: razvan11
      Logged In: YES
      user_id=919177

      On second thought, this is not a bug. The limitation occurs
      from the fact that the menu is loaded at server startup and
      shared among all users/sessions within the server. The
      solution is to change the way the loading of the menu
      resource is done so that each new session gets it's own
      copy
      and can properly filter out items based on the specific user
      role. As such, I have eliminated the menu preloading via the
      struts-config plugin and created a Filter implementation
      which checks/loads the menu resource if it does not exist.
      Also, in the JSP pages using the menu tag, I set the
      "repository" property to match the name of the
      attribute the
      filter is saving the menu resource under.

      Date: 2005-05-18 13:44
      Sender: wrschneider99
      Logged In: YES
      user_id=768885

      Found the same bug. Any chance of a 2.3.1 patch release?

      Date: 2004-11-18 03:33
      Sender: uded
      Logged In: YES
      user_id=126964

      One must change some lines to do so, but it's quite
      simple.

      >>>> File: VelocityMenuDisplayer.java

      Change lines 165-176:
      — From:
      // update menucomponents with only allowed
      components
      List componentsAllowed = new ArrayList();

      MenuComponent[] components =
      menu.getMenuComponents();

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

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

      }

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

      +++ To:
      // update menucomponents with only allowed components
      List componentsAllowed = new ArrayList();

      MenuComponent[] components = menu.getMenuComponents();
      for (int i = 0; i < components.length; i++) {
      // Case 1 - there is no submenu
      if (components[i].getMenuComponents().length
      == 0) {
      if (isAllowed(components[i]))

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

      // Case 2 - some submenus
      } else {
      MenuComponent mc = components[i];
      // I assumed that the user must have
      permission to parent level
      // of the menu to consider the submenu
      at all
      if (isAllowed(mc)) {
      MenuComponent[] subComponents =
      mc.getMenuComponents();
      for (int k = 0; k <
      subComponents.length; k++) {
      if (isAllowed(subComponents[k])
      == false)

      { mc.removeMenuComponent(subComponents[k]); }

      }
      // In case if there are some
      submenus with proper permissions
      // for that user we're checkin if
      there won't be some nulls
      // in main params and then adding
      everything to componentsAllowed.
      if (mc.getMenuComponents().length >
      0 || mc.getAction() != null || mc.getAction().length() > 0

      mc.getForward() != null ||
      mc.getForward().length() > 0 || mc.getLocation() != null ||
      mc.getLocation().length() > 0 ||
      mc.getPage() != null || mc.getPage().length() > 0)

      { componentsAllowed.add(mc); }

      }
      }
      }

      MenuComponent localMenu = new MenuComponent();
      localMenu.getComponents().addAll(componentsAllowed);

      In file MenuComponent add a method at line ~57:

      public void removeMenuComponent(MenuComponent
      menuComponent) {
      if (menuComponents.contains(menuComponent))

      { menuComponents.remove(menuComponent); }

      }

      This should do it.

        Activity

        Show
        Matt Raible added a comment - Related: http://sourceforge.net/tracker/index.php?func=detail&aid=1358227&group_id=48726&atid=453976
        Hide
        Matt Raible added a comment -

        Should be fixed in 2.4.

        Show
        Matt Raible added a comment - Should be fixed in 2.4.
        Hide
        Craxy Z added a comment -

        Sorry, but the 2.4 didn't fix it.

        Show
        Craxy Z added a comment - Sorry, but the 2.4 didn't fix it.
        Hide
        Matt Raible added a comment -

        Can you please provide a way of reproducing this in the sample application that ships with Struts Menu?

        Show
        Matt Raible added a comment - Can you please provide a way of reproducing this in the sample application that ships with Struts Menu?
        Hide
        Craxy Z added a comment -

        i use appfuse in cvs as my application base.
        in my application, there are roles like this:
        admin
        user
        customer
        marginsInAuditor
        marginsOutAuditor
        marginsInChecker
        marginsOutChecker

        and my menu-config.xml like this:
        <Menu name="MarginsManageMenu" title="menu.margins" roles="marginsInChecker,marginsOutChecker,marginsInAuditor,marginsOutAuditor">
        <Item name="AuditMarginsIn" title="menu.margins.auditIn" page="/auditMarginsIns.html" roles="marginsInAuditor"/>
        <Item name="AuditMarginsOut" title="menu.margins.auditOut" page="/auditMarginsOuts.html" roles="marginsOutAuditor"/>
        <Item name="CheckMarginsIn" title="menu.margins.checkIn" page="/checkMarginsIns.html" roles="marginsInChecker"/>
        <Item name="CheckMarginsOut" title="menu.margins.checkOut" page="/checkMarginsOuts.html" roles="marginsOutChecker"/>
        </Menu>

        then when admin logined without any of the other roles, the MarginsManageMenu don't shown, it's right.
        when admin aquire one of the roles, the MarginsManageMenu show with the right submenu, it's right.
        but when admin aquire one or more the other roles, there still only the previous one submenu, the others
        can't show, only when i restart tomcat, the others show.

        so i think the problem remains there, the menu repositry was updated by the VelocityMenuDisplayer,
        anytime the updateAllowedComponents called, it reduce the set of menu repositry.
        so maybe we need a reload, or just store a global repositry, when display, just use a local subset.

        Show
        Craxy Z added a comment - i use appfuse in cvs as my application base. in my application, there are roles like this: admin user customer marginsInAuditor marginsOutAuditor marginsInChecker marginsOutChecker and my menu-config.xml like this: <Menu name="MarginsManageMenu" title="menu.margins" roles="marginsInChecker,marginsOutChecker,marginsInAuditor,marginsOutAuditor"> <Item name="AuditMarginsIn" title="menu.margins.auditIn" page="/auditMarginsIns.html" roles="marginsInAuditor"/> <Item name="AuditMarginsOut" title="menu.margins.auditOut" page="/auditMarginsOuts.html" roles="marginsOutAuditor"/> <Item name="CheckMarginsIn" title="menu.margins.checkIn" page="/checkMarginsIns.html" roles="marginsInChecker"/> <Item name="CheckMarginsOut" title="menu.margins.checkOut" page="/checkMarginsOuts.html" roles="marginsOutChecker"/> </Menu> then when admin logined without any of the other roles, the MarginsManageMenu don't shown, it's right. when admin aquire one of the roles, the MarginsManageMenu show with the right submenu, it's right. but when admin aquire one or more the other roles, there still only the previous one submenu, the others can't show, only when i restart tomcat, the others show. so i think the problem remains there, the menu repositry was updated by the VelocityMenuDisplayer, anytime the updateAllowedComponents called, it reduce the set of menu repositry. so maybe we need a reload, or just store a global repositry, when display, just use a local subset.
        Hide
        Craxy Z added a comment -

        ps.
        1. i use springMVC
        2. i can't reproduce it in the struts-menu example, in the example, when basic role modified, i need restart my ie to take it effect. and it works, but in my app, it only worked when i restart tomcat.
        so i wonder is it a appfuse or my own problem.
        3. now i use the template solution, disable the updateAllowedComponents call, and add the role check in the template.
        here is my cssHorizontalMenu.vm
        #macro( displayCssMenu $menu )

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

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

          1. create a single menu item
            #if ($menu.components.size() == 0)
            #if $($currentCount == $allowedCount)
            <li class="last">
            #else
            <li>
            #end
            #if ($menu.name == $currentMenu)
            <strong><a href="$url" title="$title" #if($menu.target)target="$menu.target"#end#if($menu.width)style="width: $ {menu.width}px"#end>${title}</a></strong>
            #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">
            #if ($menu.name == $currentMenu)<strong>#end
            <a href="$url" title="$title" #if($menu.target)target="$menu.target"#end#if($menu.width)style="width: ${menu.width}px"#end>${title}

            </a>
            #if ($menu.name == $currentMenu)</strong>#end
            #end

        #if ($menu.components.size() > 0)
        <ul>
        #set ($allowedCount = 0)
        #foreach ($menuIt in $menu.components)
        #if ($displayer.isAllowed($menuIt))
        #set ($allowedCount = $allowedCount + 1)
        #end
        #end
        #set ($currentCount = 0)
        #foreach ($menuIt in $menu.components)
        #if ($displayer.isAllowed($menuIt))
        #set ($currentCount = $currentCount + 1)
        #displayCssMenu($menuIt)
        #end
        #end
        </li>
        </ul>
        #else
        </li>
        #end

        #end

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

        and my cssVerticalMenu.vm

        #macro( displayCssMenu $menu )

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

        #if (!$menu.parent) <h2>$title</h2> #end

        <ul id="local">
        #foreach ($menuItem in $menu.components)
        #if ($displayer.isAllowed($menuItem))
        #set ($title = $displayer.getMessage($menuItem.title))
        <li><a href="$menuItem.url" title="$title" #if($menuItem.target)target="$menuItem.target"#end>$

        {title}

        </a>
        #end
        #end
        </ul>
        #end

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

        Show
        Craxy Z added a comment - ps. 1. i use springMVC 2. i can't reproduce it in the struts-menu example, in the example, when basic role modified, i need restart my ie to take it effect. and it works, but in my app, it only worked when i restart tomcat. so i wonder is it a appfuse or my own problem. 3. now i use the template solution, disable the updateAllowedComponents call, and add the role check in the template. here is my cssHorizontalMenu.vm #macro( displayCssMenu $menu ) set menu title #set ($title = $displayer.getMessage($menu.title)) #if (!$menu.url) #set ($url="#") #else #set ($url=$menu.url) #end create a single menu item #if ($menu.components.size() == 0) #if $($currentCount == $allowedCount) <li class="last"> #else <li> #end #if ($menu.name == $currentMenu) <strong><a href="$url" title="$title" #if($menu.target)target="$menu.target"#end#if($menu.width)style="width: $ {menu.width}px"#end>${title}</a></strong> #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"> #if ($menu.name == $currentMenu)<strong>#end <a href="$url" title="$title" #if($menu.target)target="$menu.target"#end#if($menu.width)style="width: ${menu.width}px"#end>${title} </a> #if ($menu.name == $currentMenu)</strong>#end #end #if ($menu.components.size() > 0) <ul> #set ($allowedCount = 0) #foreach ($menuIt in $menu.components) #if ($displayer.isAllowed($menuIt)) #set ($allowedCount = $allowedCount + 1) #end #end #set ($currentCount = 0) #foreach ($menuIt in $menu.components) #if ($displayer.isAllowed($menuIt)) #set ($currentCount = $currentCount + 1) #displayCssMenu($menuIt) #end #end </li> </ul> #else </li> #end #end #if ($displayer.isAllowed($menu)) #displayCssMenu($menu) #end and my cssVerticalMenu.vm #macro( displayCssMenu $menu ) set menu title #set ($title = $displayer.getMessage($menu.title)) #if (!$menu.parent) <h2>$title</h2> #end <ul id="local"> #foreach ($menuItem in $menu.components) #if ($displayer.isAllowed($menuItem)) #set ($title = $displayer.getMessage($menuItem.title)) <li><a href="$menuItem.url" title="$title" #if($menuItem.target)target="$menuItem.target"#end>$ {title} </a> #end #end </ul> #end #if ($displayer.isAllowed($menu)) #displayCssMenu($menu) #end
        Hide
        Matt Raible added a comment -

        Here's a patch that clones the menu so the original isn't modified. Please try the JAR from http://static.appfuse.org/struts-menu-2.4.1.jar. Does this fix your issue?

        Show
        Matt Raible added a comment - Here's a patch that clones the menu so the original isn't modified. Please try the JAR from http://static.appfuse.org/struts-menu-2.4.1.jar . Does this fix your issue?
        Hide
        Craxy Z added a comment -

        Hi, Matt, it do fixed the issue, thanks for the great job.
        but there still another one, it's about the cssHorizontalMenu.vm in appfuse cvs.
        maybe i should post it to appfuse jira. but it is also about the velocity, so let's finish it here.

        the problem is there is the Logout Menu after my MarginsManageMenu like this:

        <Menu name="MarginsManageMenu" title="menu.margins" roles="...">
        ...
        </Menu>

        <Menu name="Logout" title="user.logout" page="/logout.jsp" roles="admin,user"/>

        as i metioned before, there is four roles and four items in MarginsManageMenu,
        so when admin got less than four of those roles, the Logout Menu become a subitem
        to the MarginsManageMenu, and when admin got none or all of them the Logout Menu
        remain it's menu situation.

        i thought it's coused by these lines from the cssHorizontalMenu.vm in appfuse cvs

        #if $($velocityCount == $menu.parent.components.size())
        <li class="last">

        and

        #if $($velocityCount == $menu.parent.components.size())
        </ul>
        #end

        maybe you should check the .parent.components.size(), it remains old value after the owned check.

        Show
        Craxy Z added a comment - Hi, Matt, it do fixed the issue, thanks for the great job. but there still another one, it's about the cssHorizontalMenu.vm in appfuse cvs. maybe i should post it to appfuse jira. but it is also about the velocity, so let's finish it here. the problem is there is the Logout Menu after my MarginsManageMenu like this: <Menu name="MarginsManageMenu" title="menu.margins" roles="..."> ... </Menu> <Menu name="Logout" title="user.logout" page="/logout.jsp" roles="admin,user"/> as i metioned before, there is four roles and four items in MarginsManageMenu, so when admin got less than four of those roles, the Logout Menu become a subitem to the MarginsManageMenu, and when admin got none or all of them the Logout Menu remain it's menu situation. i thought it's coused by these lines from the cssHorizontalMenu.vm in appfuse cvs #if $($velocityCount == $menu.parent.components.size()) <li class="last"> and #if $($velocityCount == $menu.parent.components.size()) </ul> #end maybe you should check the .parent.components.size(), it remains old value after the owned check.
        Hide
        Matt Raible added a comment -

        So you're saying that $menu.parent.components.size() doesn't take into account the items removed by the permission check? If that's the issue, it shouldn't be too hard to fix.

        Show
        Matt Raible added a comment - So you're saying that $menu.parent.components.size() doesn't take into account the items removed by the permission check? If that's the issue, it shouldn't be too hard to fix.
        Hide
        Matt Raible added a comment -

        Can you post a menu-config.xml that I can use to reproduce the problem? If I can't reproduce the problem, it's difficult for me to fix.

        Show
        Matt Raible added a comment - Can you post a menu-config.xml that I can use to reproduce the problem? If I can't reproduce the problem, it's difficult for me to fix.
        Hide
        Craxy Z added a comment -

        <?xml version="1.0" encoding="UTF-8"?>
        <MenuConfig>
        <Displayers>
        <Displayer name="Velocity" type="net.sf.navigator.displayer.VelocityMenuDisplayer"/>
        </Displayers>
        <Menus>
        <Menu name="MainMenu" title="mainMenu.title" page="/mainMenu.html" roles="admin,user">
        <Item name="UserMenu" title="menu.user" description="User Menu" page="/editProfile.html"/>
        </Menu>

        <Menu name="AccountMenu" title="menu.account" description="Account Menu" roles="customer" width="100">
        <Item name="MarginsInputMenu" title="menu.account.marginsinput" page="/marginsInputs.html" roles="customer"/>
        </Menu>

        <!-- ==================== Administrator Menu =========================== -->
        <Menu name="AdminMenu" title="menu.admin" description="Admin Menu" roles="admin" width="100">
        <Item name="ViewUsers" title="menu.admin.users" page="/users.html" roles="admin"/>
        <Item name="ActiveUsers" title="mainMenu.activeUsers" page="/activeUsers.html" roles="admin"/>
        <Item name="ReloadContext" title="menu.admin.reload" page="/reload.html" roles="admin"/>
        <Item name="FlushCache" title="menu.flushCache" page="/flushCache.html" roles="admin"/>
        </Menu>
        <Menu name="MarketMenu" title="menu.market" description="Market Management Menu" roles="admin">
        <Item name="ViewMarketControls" title="menu.market.controls" page="/marketControlConfigs.html" roles="admin"/>
        <Item name="ViewMarkets" title="menu.market.markets" page="/markets.html" roles="admin"/>
        </Menu>
        <Menu name="CustomerManageMenu" title="menu.customer" roles="admin" width="100">
        <Item name="ViewCustomers" title="menu.customer.list" page="/customers.html" roles="admin"/>
        <Item name="SettlementAccounts" title="menu.customer.settlementaccount.list" page="/settlementAccounts.html" roles="admin"/>
        </Menu>
        <Menu name="MarginsManageMenu" title="menu.margins" roles="marginsInChecker,marginsOutChecker,marginsInAuditor,marginsOutAuditor">
        <Item name="AuditMarginsIn" title="menu.margins.auditIn" page="/auditMarginsIns.html" roles="marginsInAuditor"/>
        <Item name="AuditMarginsOut" title="menu.margins.auditOut" page="/auditMarginsOuts.html" roles="marginsOutAuditor"/>
        <Item name="CheckMarginsIn" title="menu.margins.checkIn" page="/checkMarginsIns.html" roles="marginsInChecker"/>
        <Item name="CheckMarginsOut" title="menu.margins.checkOut" page="/checkMarginsOuts.html" roles="marginsOutChecker"/>
        </Menu>

        <Menu name="Logout" title="user.logout" page="/logout.jsp" roles="admin,user"/>
        </Menus>
        </MenuConfig>

        Show
        Craxy Z added a comment - <?xml version="1.0" encoding="UTF-8"?> <MenuConfig> <Displayers> <Displayer name="Velocity" type="net.sf.navigator.displayer.VelocityMenuDisplayer"/> </Displayers> <Menus> <Menu name="MainMenu" title="mainMenu.title" page="/mainMenu.html" roles="admin,user"> <Item name="UserMenu" title="menu.user" description="User Menu" page="/editProfile.html"/> </Menu> <Menu name="AccountMenu" title="menu.account" description="Account Menu" roles="customer" width="100"> <Item name="MarginsInputMenu" title="menu.account.marginsinput" page="/marginsInputs.html" roles="customer"/> </Menu> <!-- ==================== Administrator Menu =========================== --> <Menu name="AdminMenu" title="menu.admin" description="Admin Menu" roles="admin" width="100"> <Item name="ViewUsers" title="menu.admin.users" page="/users.html" roles="admin"/> <Item name="ActiveUsers" title="mainMenu.activeUsers" page="/activeUsers.html" roles="admin"/> <Item name="ReloadContext" title="menu.admin.reload" page="/reload.html" roles="admin"/> <Item name="FlushCache" title="menu.flushCache" page="/flushCache.html" roles="admin"/> </Menu> <Menu name="MarketMenu" title="menu.market" description="Market Management Menu" roles="admin"> <Item name="ViewMarketControls" title="menu.market.controls" page="/marketControlConfigs.html" roles="admin"/> <Item name="ViewMarkets" title="menu.market.markets" page="/markets.html" roles="admin"/> </Menu> <Menu name="CustomerManageMenu" title="menu.customer" roles="admin" width="100"> <Item name="ViewCustomers" title="menu.customer.list" page="/customers.html" roles="admin"/> <Item name="SettlementAccounts" title="menu.customer.settlementaccount.list" page="/settlementAccounts.html" roles="admin"/> </Menu> <Menu name="MarginsManageMenu" title="menu.margins" roles="marginsInChecker,marginsOutChecker,marginsInAuditor,marginsOutAuditor"> <Item name="AuditMarginsIn" title="menu.margins.auditIn" page="/auditMarginsIns.html" roles="marginsInAuditor"/> <Item name="AuditMarginsOut" title="menu.margins.auditOut" page="/auditMarginsOuts.html" roles="marginsOutAuditor"/> <Item name="CheckMarginsIn" title="menu.margins.checkIn" page="/checkMarginsIns.html" roles="marginsInChecker"/> <Item name="CheckMarginsOut" title="menu.margins.checkOut" page="/checkMarginsOuts.html" roles="marginsOutChecker"/> </Menu> <Menu name="Logout" title="user.logout" page="/logout.jsp" roles="admin,user"/> </Menus> </MenuConfig>
        Hide
        Matt Raible added a comment -

        Fixed in SM 2.4.1 and AppFuse 1.9.3.

        Show
        Matt Raible added a comment - Fixed in SM 2.4.1 and AppFuse 1.9.3.

          People

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

            Dates

            • Created:
              Updated:
              Resolved:

              Development