powered by Jive Software

A wrong occupant having multiple sessions was removed from a clustered MUC room

I would like to report a bug in MUC OccupantLeftEvent.java that a remote node may remove a wrong entry when one client session of an occupant (who had multiple client sessions) left a room. I had a 2-node clustered Openfire and 2 clients with same username but different resources (focus@mydomain/f1 and focus@mydomain/f2.) The two clients were connecting to two XMPP nodes and they both joined the MUC room “jibribrewery” with same nickname “focus”. When the focus@mydomain/f1 disconnected, this occupant was removed correctly from the room in both XMPP nodes. When this user (focus@mydomain/f1) reconnected, rejoined the room and disconnected again, the remote XMPP removed “focus@mydomain/f2” from the room. In one XMPP node the remaining occupant was focus@mydomain/f2 (which is correct), but in the other XMPP node the remaining occupant was focus@mydomain/f1 (which is incorrect.) Although my use case may be unusual, it may happen to normal clients with Stream-Management enabled (a mobile client lost connection temporarily, but reconnected with different resource.)

The bug is in OccupantLeftEvent.getRole() that it used a deprecated getRoom().getOccupant(nickname) which always returns the first MUCRole from a list. Since this occupant had two client sessions and used the same nickname, the list had multiple MUCRole entries. Subsequently, the occupants in this room among the nodes would be out of sync and caused a lot of unexpected behavior. The fix is to pass along the full JID and use it to get the correct MUCRole from the list. Also, the field member “role” probably can be declared as transient.

Here is a suggested fix in xmppserver/src/main/java/org/jivesoftware/openfire/muc/cluster/OccupantLeftEvent.java

git diff OccupantLeftEvent.java
diff --git a/xmppserver/src/main/java/org/jivesoftware/openfire/muc/cluster/OccupantLeftEvent.java b/xmppserver/src/main/java/org/jivesoftware/openfire/muc/cluster/OccupantLeftEvent.java
index 219805c..8ac348f 100644
--- a/xmppserver/src/main/java/org/jivesoftware/openfire/muc/cluster/OccupantLeftEvent.java
+++ b/xmppserver/src/main/java/org/jivesoftware/openfire/muc/cluster/OccupantLeftEvent.java
@@ -20,20 +20,23 @@ import org.jivesoftware.openfire.muc.MUCRole;
 import org.jivesoftware.openfire.muc.spi.LocalMUCRoom;
 import org.jivesoftware.openfire.user.UserNotFoundException;
 import org.jivesoftware.util.cache.ExternalizableUtil;
+import org.xmpp.packet.JID;
 
 import java.io.IOException;
 import java.io.ObjectInput;
 import java.io.ObjectOutput;
+import java.util.List;
 
 /**
  * Task that removes a room occupant from the list of occupants in the room. The
- * occupant to remove is actualy a {@link org.jivesoftware.openfire.muc.spi.RemoteMUCRole}.
+ * occupant to remove is actually a {@link org.jivesoftware.openfire.muc.spi.RemoteMUCRole}.
  *
  * @author Gaston Dombiak
  */
 public class OccupantLeftEvent extends MUCRoomTask<Void> {
     private MUCRole role;
     private String nickname;
+    private JID userAddress;
 
     public OccupantLeftEvent() {
     }
@@ -42,12 +45,21 @@ public class OccupantLeftEvent extends MUCRoomTask<Void> {
         super(room);
         this.role = role;
         this.nickname = role.getNickname();
+        this.userAddress = role.getUserAddress();
     }
 
     public MUCRole getRole() {
         if (role == null) {
             try {
-                role = getRoom().getOccupant(nickname);
+                // If there are multiple entries, get one with same full JID
+                List<MUCRole> roles = getRoom().getOccupantsByNickname(nickname);
+                for (MUCRole r : roles) {
+                    if (userAddress.equals(r.getUserAddress())) {
+                        role = r;
+                        break;
+                    }
+                }
+                // TODO: if no matching full JID, what to do?
             } catch (UserNotFoundException e) {
                 // Ignore
             }
@@ -75,11 +87,13 @@ public class OccupantLeftEvent extends MUCRoomTask<Void> {
     public void writeExternal(ObjectOutput out) throws IOException {
         super.writeExternal(out);
         ExternalizableUtil.getInstance().writeSafeUTF(out, nickname);
+        ExternalizableUtil.getInstance().writeSerializable(out, userAddress);
     }
 
     @Override
     public void readExternal(ObjectInput in) throws IOException, ClassNotFoundException {
         super.readExternal(in);
         nickname = ExternalizableUtil.getInstance().readSafeUTF(in);
+        userAddress = (JID) ExternalizableUtil.getInstance().readSerializable(in);
     }
 }

Thanks, could you kindly submit this as a pull request against the github repository?

I never did a pull request against open source repo. I just created a pull request #1179 from my forked repo (tags/v4.2.3). If I did it incorrectly, please let me know.

Thanks for the PR - but it looks like it needs a bit of work; your patch suggests that one file was modified, the PR at https://github.com/igniterealtime/Openfire/pull/1179 indicates 227 (!) files have changed. You probably need to rebase, I’d offer more help but I still struggle with git in similar ways.

Google suggests https://github.com/edx/edx-platform/wiki/How-to-Rebase-a-Pull-Request is a good starting point.

Hope this helps,

Greg

Thanks for the article; it is very useful. Previously I created my branch from “tags/v4.2.3” (not from “master” branch) and it caused the mess in PR. Now I re-applied my fix to a new branch based from “master”. Should I submit a new PR? Please advise.