[MAGNOLIA-5730] Recursive groups causes an infinite loop with stackoverflow error in MgnlUser Created: 26/Mar/14  Updated: 06/May/14  Resolved: 27/Mar/14

Status: Closed
Project: Magnolia
Component/s: core
Affects Version/s: 4.5.16
Fix Version/s: 4.5.18, 5.2.4

Type: Bug Priority: Blocker
Reporter: Edgar Vonk Assignee: Milan Divilek
Resolution: Fixed Votes: 0
Labels: support
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified

Attachments: Text File MgnlUser.java    
Template:
Patch included:
Yes
Acceptance criteria:
Empty
Task DoD:
[ ]* Doc/release notes changes? Comment present?
[ ]* Downstream builds green?
[ ]* Solution information and context easily available?
[ ]* Tests
[ ]* FixVersion filled and not yet released
[ ]  Architecture Decision Record (ADR)
Bug DoR:
[ ]* Steps to reproduce, expected, and actual results filled
[ ]* Affected version filled
Release notes required:
Yes
Date of First Response:
Visible to:
Gordon Bentvelzen

 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.



 Comments   
Comment by Edgar Vonk [ 26/Mar/14 ]

Forgot to mention: the issue occurs on many situations but the most prominent is when the user in question logs in to Magnolia. In our case we have a custom group manager class WstGroupManager as you see in the stacktrace but this has nothing to do with the issue. The problem is in MgnlUser itself.

2014-03-26 13:30:21,767 ERROR info.magnolia.cms.security.SecuritySupportBase    : Can't login due to: 
javax.security.auth.login.LoginException: java.lang.StackOverflowError
	at org.apache.log4j.spi.LoggingEvent.<init>(LoggingEvent.java:165)
	at org.apache.log4j.Category.forcedLog(Category.java:391)
	at org.apache.log4j.Category.log(Category.java:856)
	at org.slf4j.impl.Log4jLoggerAdapter.error(Log4jLoggerAdapter.java:576)
	at info.magnolia.cms.security.SilentSessionOp.exec(SilentSessionOp.java:73)
	at info.magnolia.context.MgnlContext.doInSystemContext(MgnlContext.java:403)
	at info.magnolia.context.MgnlContext.doInSystemContext(MgnlContext.java:376)
	at com.worldsteel.magnolia.security.WstGroupManager.getGroup(WstGroupManager.java:110)
	at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:372)
	at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379)
	at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379)
	at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379)
	at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379)
	at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379)
	at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379)
	at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379)
	at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379)
	at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379)
	at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379)
	at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379)
	at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379)
	at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379)
	at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379)
	at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379)
	at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379)
	at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379)
	at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379)
	at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379)
	at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379)
	at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379)
	at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379)
	at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379)
	at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379)
	at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379)
	at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379)
	at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379)
	at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379)
	at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379)
	at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379)
	at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379)
	at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379)
	at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379)
	at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379)
	at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379)
	at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379)
	at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379)
	at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379)
	at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379)
	at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379)
	at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379)
	at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379)
	at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379)
	at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379)
	at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379)
	at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379)
	at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379)
	at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379)
	at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379)
	at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379)
	at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379)
	at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379)
	at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379)
	at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379)
	at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379)
	at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379)
	at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379)
	at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379)
	at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379)
	at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379)
	at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379)
	at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379)
	at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379)
	at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379)
	at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379)
	at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379)
	at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379)
	at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379)
	at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379)
	at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379)
	at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379)
	at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379)
	at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379)
	at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379)
	at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379)
	at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379)
	at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379)
	at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379)
	at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379)
	at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379)
	at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379)
	at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379)
	at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379)
	at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379)
	at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379)
	at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379)
	at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379)
	at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379)
	at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379)
	at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379)
	at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379)
	at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379)
	at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379)
	at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379)
	at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379)
	at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379)
	at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379)
	at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379)
	at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379)
	at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379)
	at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379)
	at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379)
	at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379)
	at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379)
	at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379)
	at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379)
	at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379)
	at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379)
	at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379)
	at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379)
	at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379)
	at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379)
	at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379)
	at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379)
	at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379)
	at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379)


[..]
Comment by Jan Haderka [ 27/Mar/14 ]

what is the reason for removing serial version UID? This class implements Serializable and indeed is serialised when user session is serialised since we keep it in session via subject. Deserializing such session after update would cause exception since uid would no longer match. Plus it looks completely irrelevant change to the current issue.

Comment by Milan Divilek [ 27/Mar/14 ]

serialVersionUID wasn't removed in this issue, but in MAGNOLIA-5594. see http://git.magnolia-cms.com/gitweb/?p=magnolia_main.pub.git;a=commit;h=10e531aa050e6b43f111323e96ab0f97739d8caf

Comment by Jan Haderka [ 27/Mar/14 ]

sry, i don't know why git it popped up in git code review tab as your change

Generated at Mon Feb 12 04:07:51 CET 2024 using Jira 9.4.2#940002-sha1:46d1a51de284217efdcb32434eab47a99af2938b.