[MAGNOLIA-6647] Pulse badge not updated when using ExternalUserManager Created: 02/Mar/16 Updated: 09/Feb/17 Resolved: 17/May/16 |
|
| Status: | Closed |
| Project: | Magnolia |
| Component/s: | None |
| Affects Version/s: | None |
| Fix Version/s: | 5.4.7, 5.5 |
| Type: | Bug | Priority: | Major |
| Reporter: | Florian Fuchs | Assignee: | Oanh Thai Hoang |
| Resolution: | Fixed | Votes: | 0 |
| Labels: | support | ||
| Remaining Estimate: | 0d | ||
| Time Spent: | 5d 5.5h | ||
| Original Estimate: | 5d | ||
| Attachments: |
|
||||||||||||||||||||
| Issue Links: |
|
||||||||||||||||||||
| 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 44 | ||||||||||||||||||||
| Story Points: | 8 | ||||||||||||||||||||
| Description |
| Comments |
| Comment by Oanh Thai Hoang [ 06/Apr/16 ] |
|
Root cause: In DelegatingUserManager#getUsersWithGroup(String) - the UserManagers are called until one doesn't throw UnsupportedOperationException - this will be the SystemUserManager - according to the order of /server/security/userManagers configuration. So absolutely LdapUserManager can't be retrieved to get user. Solution: There is only one solution that should provide ability of getting user by group for DelegatingUserManager
@Override
public Collection<String> getUsersWithGroup(String groupName, boolean transitive) {
if (!transitive) {
return getUsersWithGroup(groupName);
}
Set<String> users = new HashSet<>();
// FYI: can't inject securitySupport or get static instance of SecuritySupport during the initialization phase.
GroupManager man = SecuritySupport.Factory.getInstance().getGroupManager();
if (man instanceof MgnlGroupManager) {
Collection<String> groupNames = ((MgnlGroupManager) man).getAllSubGroups(groupName);
groupNames.add(groupName);
for (String transitiveGroup : groupNames) {
Collection<String> userNames = getUsersWithGroup(transitiveGroup);
users.addAll(userNames);
}
}
return users;
}
2. Alter DelegatingUserManager#getUsersWithGroup(String) to retrieve users from all managers no matter what!
@Override
public Collection<String> getUsersWithGroup(final String groupName) {
Collection<String> result = new HashSet<>();
for (String realmName : delegates.keySet()) {
final UserManager um = delegates.get(realmName);
try {
result.addAll(um.getUsersWithGroup(groupName));
} catch (UnsupportedOperationException e) {
// should log something
}
}
return result;
}
3. Alter DelegatingUserManager#getUsersWithGroup(String, boolean) to retrieve users from all managers no matter what! public Collection<String> getUsersWithGroup(final String groupName, final boolean transitive) throws UnsupportedOperationException { Collection<String> result = new HashSet<>(); for (String realmName : delegates.keySet()) { final UserManager um = delegates.get(realmName); try { result.addAll(um.getUsersWithGroup(groupName, transitive)); } catch (UnsupportedOperationException e) { // should log something } } return result; } |
| Comment by Mikaël Geljić [ 11/Apr/16 ] |
|
Well, the DelegatingUserManager says what it does, and does what it says:
I would keep this as it is; while if we want another, "collecting" implementation, we could provide an AggregatingUserManager or something. Let's see if I can get some consensus with other architects there. That said, even if we delegated to all userManagers, ExternalUserManager still doesn't support UserManager#getUsersWithGroup(groupName, transitive)—which is quite critical for the pulse badge update afaik. Even if you don't use group inheritance. /** * Default implementation for external userManagers. Please overwrite in case it can be implemented in a more efficient way. */ |
| Comment by Mikaël Geljić [ 27/Apr/16 ] |
|
Upon further consideration, I realized the DelegatingUserManager is already aggregating results for some operations, e.g. #getAllUsers. So this invalidates my proposal for a separate AggregatingUserManager, my bad. :# The flaw is indeed that #getUsersWithGroup|Role doesn't use a similar strategy (it currently uses #delegateUntilSupported, which is not a good fit). Even when a UserManager doesn't support the operation, it should keep collecting. Yes, that's merely what you proposed in solutions 2. and 3. above. I somehow got a wrong impression because you said:
but the code was correct. Note again: that code doesn't retrieve users until an exception is thrown; it retrieves users from all managers no matter what!. |