Fixed all known (and some not reported) issues with shared groups. JM-148

git-svn-id: http://svn.igniterealtime.org/svn/repos/messenger/trunk@997 b35dd754-fafc-0310-a699-88a17e54d16e

src/java/org/jivesoftware/messenger/roster/Roster.java

@@ -19,7 +19,6 @@ import org.jivesoftware.messenger.user.UserManager;
 import org.jivesoftware.messenger.*;
 import org.jivesoftware.messenger.group.GroupManager;
 import org.jivesoftware.messenger.group.Group;
-import org.jivesoftware.messenger.group.GroupNotFoundException;
 import org.jivesoftware.util.Cacheable;
 import org.jivesoftware.util.CacheSizes;
 import org.jivesoftware.util.Log;
@@ -97,7 +96,7 @@ public class Roster implements Cacheable {
             for (Group group : sharedGroups) {
                 if (group.isUser(item.getJid().getNode())) {
                     // TODO Group name conflicts are not being considered (do we need this?)
-                    item.addSharedGroup(group.getProperties().get("sharedRoster.displayName"));
+                    item.addSharedGroup(group);
                 }
             }
             rosterItems.put(item.getJid().toBareJID(), item);
@@ -114,11 +113,11 @@ public class Roster implements Cacheable {
                 // Add the shared groups to the new roster item
                 for (Group group : sharedUsers.get(jid)) {
                     if (group.isUser(jid.getNode())) {
-                        item.addSharedGroup(group.getProperties().get("sharedRoster.displayName"));
+                        item.addSharedGroup(group);
                         itemGroups.add(group);
                     }
                     else {
-                        item.addInvisibleSharedGroup(group.getProperties().get("sharedRoster.displayName"));
+                        item.addInvisibleSharedGroup(group);
                     }
                 }
                 // Set subscription type to BOTH if the roster user belongs to a shared group
@@ -372,7 +371,9 @@ public class Roster implements Cacheable {
                     .getName());
             // Set the groups to broadcast (include personal and shared groups)
             List<String> groups = new ArrayList<String>(item.getGroups());
-            groups.addAll(item.getSharedGroups());
+            for (Group sharedGroup : item.getSharedGroups()) {
+                groups.add(sharedGroup.getProperties().get("sharedRoster.displayName"));
+            }
             if (item.getSubStatus() != RosterItem.SUB_NONE ||
                     item.getAskStatus() != RosterItem.ASK_NONE) {
                 roster.addItem(item.getJid(), item.getNickname(), ask, sub, groups);
@@ -474,7 +475,9 @@ public class Roster implements Cacheable {
     void broadcast(RosterItem item) {
         // Set the groups to broadcast (include personal and shared groups)
         List<String> groups = new ArrayList<String>(item.getGroups());
-        groups.addAll(item.getSharedGroups());
+        for (Group sharedGroup : item.getSharedGroups()) {
+            groups.add(sharedGroup.getProperties().get("sharedRoster.displayName"));
+        }
 
         org.xmpp.packet.Roster roster = new org.xmpp.packet.Roster();
         roster.setType(IQ.Type.set);
@@ -510,13 +513,11 @@ public class Roster implements Cacheable {
         boolean newItem = false;
         RosterItem item = null;
         JID jid = XMPPServer.getInstance().createJID(addedUser, "");
-        // Get the display name of the group
-        String groupName = group.getProperties().get("sharedRoster.displayName");
         try {
             // Get the RosterItem for the *local* user to add
             item = getRosterItem(jid);
             // Do nothing if the item already includes the shared group
-            if (item.getSharedGroups().contains(groupName)) {
+            if (item.getSharedGroups().contains(group)) {
                 return;
             }
             newItem = false;
@@ -534,7 +535,7 @@ public class Roster implements Cacheable {
                 newItem = true;
             }
             catch (UserNotFoundException ex) {
-                Log.error("Group (" + groupName + ") includes non-existent username (" +
+                Log.error("Group (" + group.getName() + ") includes non-existent username (" +
                         addedUser +
                         ")");
             }
@@ -547,15 +548,9 @@ public class Roster implements Cacheable {
                 User rosterUser = UserManager.getInstance().getUser(getUsername());
                 GroupManager groupManager = GroupManager.getInstance();
                 userGroups = groupManager.getGroups(rosterUser);
-                for (String name : item.getSharedGroups()) {
-                    try {
-                        sharedGroups.add(groupManager.getGroup(name));
-                    }
-                    catch (GroupNotFoundException e) {
-                    }
-                }
+                sharedGroups.addAll(item.getSharedGroups());
                 // Add the new group to the list of groups to check
-                sharedGroups.add(groupManager.getGroup(groupName));
+                sharedGroups.add(group);
                 // Set subscription type to BOTH if the roster user belongs to a shared group
                 // that is mutually visible with a shared group of the new roster item
                 if (rosterManager.hasMutualVisibility(getUsername(), userGroups, jid.getNode(),
@@ -573,16 +568,14 @@ public class Roster implements Cacheable {
             }
             catch (UserNotFoundException e) {
             }
-            catch (GroupNotFoundException e) {
-            }
         }
 
         // Add the shared group to the list of shared groups
         if (item.getSubStatus() != RosterItem.SUB_FROM) {
-            item.addSharedGroup(groupName);
+            item.addSharedGroup(group);
         }
         else {
-            item.addInvisibleSharedGroup(groupName);
+            item.addInvisibleSharedGroup(group);
         }
         // Brodcast to all the user resources of the updated roster item
         broadcast(item);
@@ -639,10 +632,19 @@ public class Roster implements Cacheable {
                     item.setSubStatus(RosterItem.SUB_BOTH);
                     for (Group group : groups) {
                         if (rosterManager.isGroupVisible(group, getUsername())) {
-                            // Get the display name of the group
-                            String groupName = group.getProperties().get("sharedRoster.displayName");
                             // Add the shared group to the list of shared groups
-                            item.addSharedGroup(groupName);
+                            item.addSharedGroup(group);
+                        }
+                    }
+                    // Add to the item the groups of this user that generated a FROM subscription
+                    // Note: This FROM subscription is overridden by the BOTH subscription but in
+                    // fact there is a TO-FROM relation between these two users that ends up in a
+                    // BOTH subscription
+                    for (Group group : userGroups) {
+                        if (!group.isUser(addedUser) &&
+                                rosterManager.isGroupVisible(group, addedUser)) {
+                            // Add the shared group to the list of invisible shared groups
+                            item.addInvisibleSharedGroup(group);
                         }
                     }
                 }
@@ -653,15 +655,13 @@ public class Roster implements Cacheable {
                     // Check if the user may see the new contact in a shared group
                     for (Group group : groups) {
                         if (rosterManager.isGroupVisible(group, getUsername())) {
-                            // Get the display name of the group
-                            String groupName = group.getProperties().get("sharedRoster.displayName");
                             // Add the shared group to the list of shared groups
-                            item.addSharedGroup(groupName);
+                            item.addSharedGroup(group);
                             item.setSubStatus(RosterItem.SUB_TO);
                         }
                     }
                     if (item.getSubStatus() == RosterItem.SUB_FROM) {
-                        item.addInvisibleSharedGroup(addedGroup.getProperties().get("sharedRoster.displayName"));
+                        item.addInvisibleSharedGroup(addedGroup);
                     }
                 }
             }
@@ -686,7 +686,7 @@ public class Roster implements Cacheable {
      * @param sharedGroup the shared group from where the user was deleted.
      * @param deletedUser the contact to update in the roster.
      */
-    void deleteSharedUser(String sharedGroup, String deletedUser) {
+    void deleteSharedUser(Group sharedGroup, String deletedUser) {
         JID jid = XMPPServer.getInstance().createJID(deletedUser, "");
         try {
             // Get the RosterItem for the *local* user to remove
@@ -712,13 +712,7 @@ public class Roster implements Cacheable {
                         User rosterUser = UserManager.getInstance().getUser(getUsername());
                         GroupManager groupManager = GroupManager.getInstance();
                         userGroups = groupManager.getGroups(rosterUser);
-                        for (String groupName : item.getSharedGroups()) {
-                            try {
-                                sharedGroups.add(groupManager.getGroup(groupName));
-                            }
-                            catch (GroupNotFoundException e) {
-                            }
-                        }
+                        sharedGroups.addAll(item.getSharedGroups());
                         // Set subscription type to BOTH if the roster user belongs to a shared group
                         // that is mutually visible with a shared group of the new roster item
                         if (rosterManager.hasMutualVisibility(getUsername(), userGroups,
@@ -761,14 +755,12 @@ public class Roster implements Cacheable {
                 deleteRosterItem(jid, false);
             }
             else {
-                item.removeSharedGroup(deletedGroup.getProperties().get("sharedRoster.displayName"));
+                item.removeSharedGroup(deletedGroup);
                 // Remove all invalid shared groups from the roster item
                 for (Group group : groups) {
                     if (!rosterManager.isGroupVisible(group, getUsername())) {
-                        // Get the display name of the group
-                        String groupName = group.getProperties().get("sharedRoster.displayName");
                         // Remove the shared group from the list of shared groups
-                        item.removeSharedGroup(groupName);
+                        item.removeSharedGroup(group);
                     }
                 }
 
@@ -816,11 +808,9 @@ public class Roster implements Cacheable {
      * A shared group of the user has been renamed. Update the existing roster items with the new
      * name of the shared group and make a roster push for all the available resources.
      *
-     * @param oldName old name of the shared group.
-     * @param newName new name of the shared group.
      * @param users group users of the renamed group.
      */
-    void shareGroupRenamed(String oldName, String newName, Collection<String> users) {
+    void shareGroupRenamed(Collection<String> users) {
         for (String user : users) {
             if (username.equals(user)) {
                 continue;
@@ -830,9 +820,6 @@ public class Roster implements Cacheable {
             try {
                 // Get the RosterItem for the *local* user to add
                 item = getRosterItem(jid);
-                // Update the shared group name in the list of shared groups
-                item.removeSharedGroup(oldName);
-                item.addSharedGroup(newName);
                 // Brodcast to all the user resources of the updated roster item
                 broadcast(item);
             }

src/java/org/jivesoftware/messenger/roster/RosterItem.java

@@ -16,6 +16,7 @@ import org.jivesoftware.util.Cacheable;
 import org.jivesoftware.util.CacheSizes;
 import org.jivesoftware.messenger.group.GroupManager;
 import org.jivesoftware.messenger.group.GroupNotFoundException;
+import org.jivesoftware.messenger.group.Group;
 import org.jivesoftware.messenger.SharedGroupException;
 import org.xmpp.packet.JID;
 
@@ -124,8 +125,8 @@ public class RosterItem implements Cacheable {
     protected JID jid;
     protected String nickname;
     protected List<String> groups;
-    protected Set<String> sharedGroups = new HashSet<String>();
-    protected Set<String> invisibleSharedGroups = new HashSet<String>();
+    protected Set<Group> sharedGroups = new HashSet<Group>();
+    protected Set<Group> invisibleSharedGroups = new HashSet<Group>();
     protected SubType subStatus;
     protected AskType askStatus;
     private long rosterID;
@@ -311,7 +312,9 @@ public class RosterItem implements Cacheable {
         }
         else {
             // Raise an error if the user is trying to remove the item from a shared group
-            for (String groupName : getSharedGroups()) {
+            for (Group group: getSharedGroups()) {
+                // Get the display name of the group
+                String groupName = group.getProperties().get("sharedRoster.displayName");
                 // Check if the group has been removed from the new groups list
                 if (!groups.contains(groupName)) {
                     throw new SharedGroupException("Cannot remove item from shared group");
@@ -340,7 +343,7 @@ public class RosterItem implements Cacheable {
      *
      * @return The shared groups this item belongs to.
      */
-    public Collection<String> getSharedGroups() {
+    public Collection<Group> getSharedGroups() {
         return sharedGroups;
     }
 
@@ -351,7 +354,7 @@ public class RosterItem implements Cacheable {
      *
      * @return The shared groups this item belongs to.
      */
-    public Collection<String> getInvisibleSharedGroups() {
+    public Collection<Group> getInvisibleSharedGroups() {
         return invisibleSharedGroups;
     }
 
@@ -360,7 +363,7 @@ public class RosterItem implements Cacheable {
      *
      * @param sharedGroup The shared group to add to the list of shared groups.
      */
-    public void addSharedGroup(String sharedGroup) {
+    public void addSharedGroup(Group sharedGroup) {
         sharedGroups.add(sharedGroup);
         invisibleSharedGroups.remove(sharedGroup);
     }
@@ -372,7 +375,7 @@ public class RosterItem implements Cacheable {
      *
      * @param sharedGroup The shared group to add to the list of shared groups.
      */
-    public void addInvisibleSharedGroup(String sharedGroup) {
+    public void addInvisibleSharedGroup(Group sharedGroup) {
         invisibleSharedGroups.add(sharedGroup);
     }
 
@@ -381,7 +384,7 @@ public class RosterItem implements Cacheable {
      *
      * @param sharedGroup The shared group to remove from the list of shared groups.
      */
-    public void removeSharedGroup(String sharedGroup) {
+    public void removeSharedGroup(Group sharedGroup) {
         sharedGroups.remove(sharedGroup);
         invisibleSharedGroups.remove(sharedGroup);
     }

src/java/org/jivesoftware/messenger/roster/RosterManager.java

@@ -283,7 +283,7 @@ public class RosterManager extends BasicModule implements GroupEventListener {
                 Roster roster = (Roster) CacheManager.getCache("username2roster").get(updatedUser);
                 if (roster != null) {
                     // Update the roster with the new group display name
-                    roster.shareGroupRenamed(originalValue, currentValue, users);
+                    roster.shareGroupRenamed(users);
                 }
             }
         }
@@ -407,8 +407,6 @@ public class RosterManager extends BasicModule implements GroupEventListener {
     private void groupUserDeleted(Group group, Collection<String> users, String deletedUser) {
         // Get the roster of the deleted user.
         Roster deletedUserRoster = (Roster) CacheManager.getCache("username2roster").get(deletedUser);
-        // Get the display name of the group
-        String groupName = group.getProperties().get("sharedRoster.displayName");
 
         // Iterate on all the affected users and update their rosters
         for (String userToUpdate : users) {
@@ -416,7 +414,7 @@ public class RosterManager extends BasicModule implements GroupEventListener {
             Roster roster = (Roster)CacheManager.getCache("username2roster").get(userToUpdate);
             // Only update rosters in memory
             if (roster != null) {
-                roster.deleteSharedUser(groupName, deletedUser);
+                roster.deleteSharedUser(group, deletedUser);
             }
             // Update the roster of the newly deleted group user.
             if (deletedUserRoster != null) {
@@ -523,8 +521,6 @@ public class RosterManager extends BasicModule implements GroupEventListener {
 
         // Get the roster of the user from which we need to remove the shared group users
         Roster userRoster = (Roster) CacheManager.getCache("username2roster").get(username);
-        // Get the display name of the group
-        String groupName = group.getProperties().get("sharedRoster.displayName");
 
         // Iterate on all the group users and update their rosters
         for (String userToRemove : users) {
@@ -532,7 +528,7 @@ public class RosterManager extends BasicModule implements GroupEventListener {
             Roster roster = (Roster)CacheManager.getCache("username2roster").get(userToRemove);
             // Only update rosters in memory
             if (roster != null) {
-                roster.deleteSharedUser(groupName, username);
+                roster.deleteSharedUser(group, username);
             }
             // Update the roster of the user
             if (userRoster != null) {
@@ -636,7 +632,8 @@ public class RosterManager extends BasicModule implements GroupEventListener {
      * Returns true if a group in the first collection may mutually see a group of the
      * second collection. More precisely, return true if both collections contain a public
      * group (i.e. anybody can see the group) or if both collection have a group that may see
-     * each other and the users are members of those groups.
+     * each other and the users are members of those groups or if one group is public and the
+     * other group allowed the public group to see it.
      *
      * @param user the name of the user associated to the first collection of groups.
      * @param groups a collection of groups to check against the other collection of groups.
@@ -673,6 +670,23 @@ public class RosterManager extends BasicModule implements GroupEventListener {
                         }
                     }
                 }
+                else if ("everybody".equals(showInRoster) && "onlyGroup".equals(otherShowInRoster)) {
+                    // Return true if one group is public and the other group allowed the public
+                    // group to see him
+                    String otherGroupNames = otherGroup.getProperties().get("sharedRoster.groupList");
+                    if (otherGroupNames != null && otherGroupNames.contains(group.getName())) {
+                            return true;
+                    }
+                }
+                else if ("onlyGroup".equals(showInRoster) && "everybody".equals(otherShowInRoster)) {
+                    // Return true if one group is public and the other group allowed the public
+                    // group to see him
+                    String groupNames = group.getProperties().get("sharedRoster.groupList");
+                    // Return true if each group may see the other group
+                    if (groupNames != null && groupNames.contains(otherGroup.getName())) {
+                            return true;
+                    }
+                }
             }
         }
         return false;