Merge pull request #433 from tevans/OF-122

OF-122: Fix inconsistent MUC subject handling

src/java/org/jivesoftware/openfire/muc/HistoryStrategy.java

@@ -181,20 +181,17 @@ public class HistoryStrategy {
         }
 
         // Room subject change messages are special
-        boolean subjectChange = false;
-        if (packet.getSubject() != null && packet.getSubject().length() > 0){
-            subjectChange = true;
+        boolean subjectChange = isSubjectChangeRequest(packet);
+        if (subjectChange) {
             roomSubject = packet;
         }
 
         // store message according to active strategy
-        if (strategyType == Type.none){
-            if (subjectChange) {
-                history.clear();
-                history.add(packet);
-            }
+        if (strategyType == Type.none && subjectChange) {
+            history.clear();
+            history.add(packet);
         }
-        else if (strategyType == Type.all) {
+        else if (strategyType == Type.all || subjectChange) {
             history.add(packet);
         }
         else if (strategyType == Type.number) {
@@ -322,7 +319,30 @@ public class HistoryStrategy {
         return roomSubject;
     }
 
-    private static class MessageComparator implements Comparator<Message> {
+    /**
+     * Returns true if the given message qualifies as a subject change request for
+     * the target MUC room, per XEP-0045. Note that this does not validate whether 
+     * the sender has permission to make the change, because subject change requests
+     * may be loaded from history or processed "live" during a user's session.
+     * 
+     * Refer to http://xmpp.org/extensions/xep-0045.html#subject-mod for details.
+     * 
+     * @return true if the given packet is a subject change request
+     */
+	public boolean isSubjectChangeRequest(Message message) {
+		
+		// The subject is changed by sending a message of type "groupchat" to the <room@service>, 
+		// where the <message/> MUST contain a <subject/> element that specifies the new subject 
+		// but MUST NOT contain a <body/> element (or a <thread/> element).
+		// An empty <subject/> value means that the room subject should be removed.
+
+		return Message.Type.groupchat == message.getType() && 
+				message.getSubject() != null && 
+				message.getBody() == null && 
+				message.getThread() == null;
+	}
+
+	private static class MessageComparator implements Comparator<Message> {
         @Override
         public int compare(Message o1, Message o2) {
             String stamp1 = o1.getChildElement("delay", "urn:xmpp:delay").attributeValue("stamp");

src/java/org/jivesoftware/openfire/muc/MUCRoomHistory.java

@@ -51,23 +51,23 @@ public final class MUCRoomHistory {
     }
 
     public void addMessage(Message packet) {
+    	boolean isSubjectChangeRequest = isSubjectChangeRequest(packet);
+    	JID fromJID = packet.getFrom();
         // Don't keep messages whose sender is the room itself (thus address without resource)
         // unless the message is changing the room's subject
-        if ((packet.getFrom() == null || packet.getFrom().toString().length() == 0 ||
-                packet.getFrom().equals(room.getRole().getRoleAddress())) &&
-                packet.getSubject() == null) {
+        if (!isSubjectChangeRequest &&
+        	(fromJID == null || fromJID.toString().length() == 0 ||
+        	 fromJID.equals(room.getRole().getRoleAddress()))) {
             return;
         }
-        // Do not store messages is strategy is none and message is not changing the room subject
-        if (!historyStrategy.isHistoryEnabled()) {
-            if (packet.getSubject() == null || packet.getSubject().trim().length() == 0) {
-                return;
-            }
+        // Do not store regular messages if there is no message strategy (keep subject change requests)
+        if (!isSubjectChangeRequest && !historyStrategy.isHistoryEnabled()) {
+            return;
         }
 
-        // Ignore messages with no subject AND no body
-        if ((packet.getSubject() == null || "".equals(packet.getSubject().trim())) &&
-                (packet.getBody() == null || "".equals(packet.getBody().trim()))) {
+        // Ignore empty messages (no subject AND no body)
+        if (!isSubjectChangeRequest &&
+            (packet.getBody() == null || packet.getBody().trim().length() == 0)) {
             return;
         }
 
@@ -198,4 +198,13 @@ public final class MUCRoomHistory {
     public Message getChangedSubject() {
         return historyStrategy.getChangedSubject();
     }
+
+    /**
+     * Returns true if the given message qualifies as a subject change request, per XEP-0045.
+     * 
+     * @return true if the given packet is a subject change request
+     */
+    public boolean isSubjectChangeRequest(Message message) {
+        return historyStrategy.isSubjectChangeRequest(message);
+    }
 }
\ No newline at end of file

src/java/org/jivesoftware/openfire/muc/spi/LocalMUCRoom.java

@@ -2024,10 +2024,6 @@ public class LocalMUCRoom implements MUCRoom, GroupEventListener {
     public void changeSubject(Message packet, MUCRole role) throws ForbiddenException {
         if ((canOccupantsChangeSubject() && role.getRole().compareTo(MUCRole.Role.visitor) < 0) ||
                 MUCRole.Role.moderator == role.getRole()) {
-            // Do nothing if the new subject is the same as the existing one
-            if (packet.getSubject().equals(subject)) {
-                return;
-            }
             // Set the new subject to the room
             subject = packet.getSubject();
             MUCPersistenceManager.updateRoomSubject(this);
@@ -2038,10 +2034,8 @@ public class LocalMUCRoom implements MUCRoom, GroupEventListener {
             // Fire event signifying that the room's subject has changed.
             MUCEventDispatcher.roomSubjectChanged(getJID(), role.getUserAddress(), subject);
 
-            if (!"local-only".equals(packet.getID())) {
-	            // Let other cluster nodes that the room has been updated
-	            CacheFactory.doClusterTask(new RoomUpdatedEvent(this));
-            }
+            // Let other cluster nodes that the room has been updated
+	        CacheFactory.doClusterTask(new RoomUpdatedEvent(this));
         }
         else {
             throw new ForbiddenException();

src/java/org/jivesoftware/openfire/muc/spi/LocalMUCUser.java

@@ -271,9 +271,7 @@ public class LocalMUCUser implements MUCUser {
                 }
                 else {
                     try {
-                        if (packet.getSubject() != null && packet.getSubject().trim().length() > 0 &&
-                                Message.Type.groupchat == packet.getType() &&
-                                (packet.getBody() == null || packet.getBody().trim().length() == 0)) {
+                        if (role.getChatRoom().getRoomHistory().isSubjectChangeRequest(packet)) {
                             // An occupant is trying to change the room's subject
                             role.getChatRoom().changeSubject(packet, role);
 

src/web/muc-room-edit-form.jsp

@@ -269,7 +269,6 @@
                 message.setSubject(roomSubject);
                 message.setFrom(room.getRole().getRoleAddress());
                 message.setTo(room.getRole().getRoleAddress());
-                message.setID("local-only");
                 room.changeSubject(message, room.getRole());
             }