[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: PNG File messageAppCase.png     PNG File publishingCase.png    
Issue Links:
Relates
relates to MGNLCI-12 Pulse icon does not show number of ne... Closed
causality
is causing MAGNOLIA-6906 Avoid calling #getAllUser several tim... Closed
relation
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   

Steps to reproduce

Assuming we have 2 LDAP accounts: "mary" belongs to group "travel-demo-editors", and "john" belongs to "travel-demo-publishers".

  • Steps to reproduce for Messages app case:
    • Login as "superuser" on Firefox.
    • Login as "mary" on Chrome.
    • On "superuser" side, go to Messages app, send a message to "mary".

      => Expected: "mary" receives that message in Pulse.
      => Actual: She doesn't.
  • Steps to reproduce for publishing case:
    • Login as "mary" on Firefox.
    • Login as "john" on Chrome.
    • On "mary" side, go to Pages app, publish a page.

      => Expected: "john" receives that task in Pulse.
      => Actual: He doesn't.

Description

The pulse badge doesn't get updated, when using an ExternalUserManager e.g. LDAPUserManager .
You have to explicitly click into the pulse-tab, to refresh the pulse. Even reload is not enough to refresh the badge.

Normally, in MessagesManagerImpl#sendGroupMessage the message is pushed into the pulse of the User. The relevant users are identified by DelegatingUserManager#getUsersWithGroup - but within the method delegateUntilSupported - after the first UserManager returns users (SystemUserManager), the other UserManagers don't get called.

Solution

  • Provide a default implementation for ExternalUserManager#getUsersWithGroup.
  • Add new implementation for DelegatingUserManager#getUsersWithGroup and DelegatingUserManager#getUsersWithRole to collect users from all user managers.


 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
1. Implement ExternalUserManager#getUsersWithGroup(String):

    @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:

A UserManager delegating to a set of user managers. The first user manager which does not through an UnsupportedOperationException will be used.

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.
There, we should indeed provide a default impl for #getUsersWithGroup(String, boolean), and document it the same way as other methods, just like you did in (1) oanh.thai :

/**
 * 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:

retrieve over all UserManagers until one throws UnsupportedOperationException

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!.

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