[MAGNOLIA-6615] UserManager#getUsersWithGroup(groupName, transitive) is b0rked, never worked Created: 24/Mar/16  Updated: 21/Dec/16  Resolved: 07/Apr/16

Status: Closed
Project: Magnolia
Component/s: security
Affects Version/s: 5.3.7, 5.4.5
Fix Version/s: 5.3.15, 5.4.6

Type: Bug Priority: Major
Reporter: Mikaël Geljić Assignee: Oanh Thai Hoang
Resolution: Fixed Votes: 0
Labels: usermanager
Remaining Estimate: 0d
Time Spent: 5d 5h
Original Estimate: 4d

Attachments: PNG File wheres-david.png    
Issue Links:
causality
caused by MAGNOLIA-6041 Create a method on UserManager return... Closed
dependency
is depended upon by MGNLUI-3810 Pulse hides new tasks created by curr... Closed
relation
is related to MGNLLDAP-95 LDAP paging support Closed
supersession
is superseded by MAGNOLIA-6624 Introduce GroupManager #getAllSuperGr... Closed
Template:
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:
Sprint: Saigon 39
Story Points: 8

 Description   

UserManager#getUsersWithGroup(groupName, transitive) is just b0rked and most likely never worked properly...

Considering user/group setup from_templating-samples_, as follows:

  • (group) employees
    • (user) eve
    • (user) patrick
    • (group) developers
      • (user) david

UserManager#getUsersWithGroup("employees", true) doesn't return david (wrong)
whereas UserManager#getUser("david").getAllGroups() correctly returns developers + employees (correct)

Concept: Implementing only part https://wiki.magnolia-cms.com/display/DEV/Concept+-+Improvements+in+the+security+package#Concept-Improvementsinthesecuritypackage-1a.super-groupsvs.sub-groups
Solution: Provide new API to return all sub-groups MgnlGroupManager#getAllSubGroups and use that API to resolve all users transitively-assigned sub-groups correctly



 Comments   
Comment by Jan Haderka [ 24/Mar/16 ]

Was it actually supposed to find all the groups, including inherited ones or only directly assigned groups?
Originally Magnolia didn't support group inheritance and when it was added a lot of functionality (notifications, e-mail users that were members of groups, assigning tasks) was broken because it was not expected to be transitive.
Not to mention having to search groups recursively was causing performance issues.

While I have no idea what the intention behind the method was, it would be worth investigating its use and history of changes of the class, before assuming it is broken. It might be just incorrectly named.

Comment by Mikaël Geljić [ 24/Mar/16 ]

Linked the original ticket where this was implemented (fairly "recent" fwiw). This is indeed expected to return groups transitively, as opposed to UserManager#getUsersWithGroup(String), where only direct members of a group are returned.

Current implementation is transitive, but the wrong way around, i.e. #getUsersWithGroup("developers", true) would return all employees, even those who are non-developers. And again, comparing to impl of User#getAllGroups, as shown in screenshot, this is clearly at least inconsistent.

Comment by Jan Haderka [ 29/Mar/16 ]

Cool. Thx for checking it out.

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