Uploaded image for project: 'Magnolia'
  1. Magnolia
  2. MAGNOLIA-5730

Recursive groups causes an infinite loop with stackoverflow error in MgnlUser

    XMLWordPrintable

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Blocker
    • Resolution: Fixed
    • Affects Version/s: 4.5.16
    • Fix Version/s: 4.5.18, 5.2.4
    • Component/s: core
    • Labels:
    • Patch included:
      Yes
    • Release notes required:
      Yes
    • Magnolia Release:
      4.5.18, 5.2.4

      Description

      It seems we have discovered quite a serious bug related to the handling of user groups. When there are recursive groups in Magnolia the MgnlUser#addSubgroups method ends up in a recursive loop resulting in a stackoverflowerror.

      The method's JavaDoc claims to prevent against such infinite loops, however this is unfortunately not the case. The fix seems extremely simple. Just replace this code:

      // and recursively add more subgroups
      addSubgroups(allGroups, man, subgroups);
      allGroups.addAll(subgroups);
      

      with:

      allGroups.add(groupName);
      // and recursively add more subgroups
      addSubgroups(allGroups, man, subgroups);
      

      Because in the current code the 'allGroups' collection remains empty and no checking against recursive groups is done at all.

      In our temporary hotfix we have added a check to see if there actually are any subgroups:

      allGroups.add(groupName);
      if (!subgroups.isEmpty()) {
        // and recursively add more subgroups
        addSubgroups(allGroups, man, subgroups);
      }
      

      I have attached our MgnlUser.java with our fix (note: tabs instead of spaces so please reformat according to your conventions). It was build against Magnolia 4.5.16 but I think the issue is present in all recent Magnolia versions.

      Our situation: our client has a lot of groups in Magnolia and in some cases we have a recursive situation where a subgroup contains a parent group somewhere in the tree. I reproduced this bug locally with a very simple test case using 2 groups where group A contains group B as a subgroup and group B again contains group A. The current unit tests of the MgnlUser class do not contain such a recursive group setup. They should.

        Attachments

          Activity

            People

            Assignee:
            mdivilek Milan Divilek
            Reporter:
            edgar Edgar Vonk
            Visible to:
            Gordon Bentvelzen
            Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

              Dates

              Created:
              Updated:
              Resolved:
              Date of First Response: