From a990316a9a5506fe9b4407274c2a357adc954e1a Mon Sep 17 00:00:00 2001
From: Guus der Kinderen <guus.der.kinderen@gmail.com>
Date: Tue, 1 Nov 2022 17:05:01 +0100
Subject: [PATCH] fixes #159: Ensure that a JID can be affiliated only once

Issues pop up when an attempt is ade to affiliate a JID more than once, especially when that JID is also an owner. Affiliating such a JID as something else will remove the ownership affiliation. This can trigger an issue where the room has no owners.

This commit introduces a check to verify that the inbound request does not define multiple affiliations for the same JID. It also reworks the way in which affiliations are applied.
---
 changelog.html                                |   5 +
 plugin.xml                                    |   2 +-
 .../rest/controller/MUCRoomController.java    | 161 ++++++++++--------
 3 files changed, 99 insertions(+), 69 deletions(-)

diff --git a/changelog.html b/changelog.html
index 09ecda0ac..18ebcb8d0 100644
--- a/changelog.html
+++ b/changelog.html
@@ -44,6 +44,11 @@ <h1>
 REST API Plugin Changelog
 </h1>
 
+<p><b>1.10.1</b> (tbd)</p>
+<ul>
+    <li>[<a href='https://github.com/igniterealtime/openfire-restAPI-plugin/issues/159'>#159</a>] - Fix issues with duplicated MUC room affiliations</li>
+</ul>
+
 <p><b>1.10.0</b> September 29, 2022</p>
 <ul>
     <li>[<a href='https://github.com/igniterealtime/openfire-restAPI-plugin/issues/155'>#155</a>] - Add statistics for endpoint responses</li>
diff --git a/plugin.xml b/plugin.xml
index 27d9f0317..394487097 100644
--- a/plugin.xml
+++ b/plugin.xml
@@ -6,7 +6,7 @@
     <description>Allows administration over a RESTful API.</description>
     <author>Roman Soldatow</author>
     <version>${project.version}</version>
-    <date>2022-09-29</date>
+    <date>2022-11-01</date>
     <minServerVersion>4.7.0</minServerVersion>
     <adminconsole>
         <tab id="tab-server">
diff --git a/src/java/org/jivesoftware/openfire/plugin/rest/controller/MUCRoomController.java b/src/java/org/jivesoftware/openfire/plugin/rest/controller/MUCRoomController.java
index 34b893e95..57b07d850 100644
--- a/src/java/org/jivesoftware/openfire/plugin/rest/controller/MUCRoomController.java
+++ b/src/java/org/jivesoftware/openfire/plugin/rest/controller/MUCRoomController.java
@@ -339,17 +339,30 @@ public void updateChatRoom(String roomName, String serviceName, MUCRoomEntity mu
     private void createRoom(MUCRoomEntity mucRoomEntity, String serviceName, boolean sendInvitations) throws NotAllowedException,
         ForbiddenException, ConflictException, AlreadyExistsException, ServiceException
     {
-        log("Create a chat room: " + mucRoomEntity.getRoomName());
+        log("Create or updating a chat room: " + mucRoomEntity.getRoomName());
         // Set owner
         JID owner = XMPPServer.getInstance().createJID("admin", null);
         if (mucRoomEntity.getOwners() != null && mucRoomEntity.getOwners().size() > 0) {
             owner = new JID(mucRoomEntity.getOwners().get(0));
         } else {
+            log("Room '" + mucRoomEntity.getRoomName() + "' is being created/updated without an owner. Adding default owner (as having an owner is non-optional).");
             List<String> owners = new ArrayList<>();
             owners.add(owner.toBareJID());
             mucRoomEntity.setOwners(owners);
         }
 
+        // Issue #159: Do not allow duplicate affiliations.
+        final Collection<JID> allRequestedAffiliations = new LinkedList<>();
+        if (mucRoomEntity.getOwners() != null) allRequestedAffiliations.addAll(MUCRoomUtils.convertStringsToJIDs(mucRoomEntity.getOwners()));
+        if (mucRoomEntity.getAdmins() != null) allRequestedAffiliations.addAll(MUCRoomUtils.convertStringsToJIDs(mucRoomEntity.getAdmins()));
+        if (mucRoomEntity.getMembers() != null) allRequestedAffiliations.addAll(MUCRoomUtils.convertStringsToJIDs(mucRoomEntity.getMembers()));
+        if (mucRoomEntity.getOutcasts() != null) allRequestedAffiliations.addAll(MUCRoomUtils.convertStringsToJIDs(mucRoomEntity.getOutcasts()));
+        final Set<JID> uniques = new HashSet<>();
+        final Set<String> duplicates = allRequestedAffiliations.stream().filter(n -> !uniques.add(n)).map(JID::toString).collect(Collectors.toSet());
+        if (!duplicates.isEmpty()) {
+            throw new ConflictException("The requested room defines duplicate affiliations for the following entities: " + String.join(", ", duplicates));
+        }
+
         //	Check if chat service is available, if not create a new one
         boolean serviceRegistered = XMPPServer.getInstance().getMultiUserChatManager().isServiceRegistered(serviceName);
         if(!serviceRegistered) {
@@ -357,8 +370,9 @@ private void createRoom(MUCRoomEntity mucRoomEntity, String serviceName, boolean
             XMPPServer.getInstance().getMultiUserChatManager().createMultiUserChatService(serviceName, serviceName, false);
         }
 
-        log("Setting initial values for room that is being created: " + mucRoomEntity.getRoomName());
+        log("Setting initial values for room that is being created/updated: " + mucRoomEntity.getRoomName());
         MUCRoom room = MUCServiceController.getService(serviceName).getChatRoom(mucRoomEntity.getRoomName(), owner);
+        log("Room " + mucRoomEntity.getRoomName() + " is being " + (room.isLocked() ? "created" : "updated"));
 
         // Set values
         room.setNaturalLanguageName(mucRoomEntity.getNaturalName());
@@ -388,7 +402,7 @@ private void createRoom(MUCRoomEntity mucRoomEntity, String serviceName, boolean
         }
 
         // Set all roles
-        log("Setting roles for room that is being created: " + mucRoomEntity.getRoomName());
+        log("Setting roles for room that is being " + (room.isLocked() ? "created" : "updated") + ": " + mucRoomEntity.getRoomName());
         Collection<JID> allUsersWithNewAffiliations = null;
         if (!equalToAffiliations(room, mucRoomEntity)) {
             allUsersWithNewAffiliations = setRoles(room, mucRoomEntity);
@@ -409,23 +423,23 @@ private void createRoom(MUCRoomEntity mucRoomEntity, String serviceName, boolean
         }
 
         // Unlock the room, because the default configuration lock the room.  		
-        log("Unlocking room that is being created: " + mucRoomEntity.getRoomName());
+        log("Unlocking room that is being " + (room.isLocked() ? "created" : "updated") + ": " + mucRoomEntity.getRoomName());
         room.unlock(room.getRole());
 
         // Save the room to the DB if the room should be persistent
         if (room.isPersistent()) {
-            log("Persisting room that is being created: " + mucRoomEntity.getRoomName());
+            log("Persisting room that is being created/updated: " + mucRoomEntity.getRoomName());
             room.saveToDB();
         }
 
-        log("Syncing room that is being created: " + mucRoomEntity.getRoomName());
+        log("Syncing room that is being created/updated: " + mucRoomEntity.getRoomName());
         MUCServiceController.getService(serviceName).syncChatRoom(room);
 
         if (sendInvitations && allUsersWithNewAffiliations != null) {
-            log("Sending invitations for room that is being created: " + mucRoomEntity.getRoomName());
+            log("Sending invitations for room that is being created/updated: " + mucRoomEntity.getRoomName());
             sendInvitationsFromRoom(room, null, allUsersWithNewAffiliations, null, true);
         }
-        log("Done creating room: " + mucRoomEntity.getRoomName());
+        log("Done creating/updating room: " + mucRoomEntity.getRoomName());
     }
 
     private boolean equalToAffiliations(MUCRoom room, MUCRoomEntity mucRoomEntity) {
@@ -859,92 +873,103 @@ public MUCRoomEntity convertToMUCRoomEntity(MUCRoom room, boolean expand) {
      * @throws ConflictException
      *             the conflict exception
      */
-    private Collection<JID> setRoles(MUCRoom room, MUCRoomEntity mucRoomEntity) throws ForbiddenException, NotAllowedException,
-            ConflictException {
-        Collection<JID> allNewAffiliations = new ArrayList<>();
-
-        List<JID> roles = new ArrayList<>();
-        Collection<JID> existingOwners = new ArrayList<>();
-
-        List<JID> mucRoomEntityOwners = MUCRoomUtils.convertStringsToJIDs(mucRoomEntity.getOwners());
-        Collection<JID> owners = new ArrayList<>(room.getOwners());
-
-        // Find same owners
-        for (JID jid : owners) {
-            if (mucRoomEntityOwners.contains(jid)) {
-                existingOwners.add(jid);
-            }
-        }
-
-        // Don't delete the same owners
-        owners.removeAll(existingOwners);
-
-        // Collect all roles to reset
-        roles.addAll(owners);
-        roles.addAll(room.getAdmins());
-        roles.addAll(room.getMembers());
-        roles.addAll(room.getOutcasts());
-
-        for (JID jid : roles) {
-            room.addNone(jid, room.getRole());
+    private static Collection<JID> setRoles(MUCRoom room, MUCRoomEntity mucRoomEntity) throws ForbiddenException, NotAllowedException, ConflictException
+    {
+        final Collection<JID> allNewAffiliations = new ArrayList<>();
+
+        // Calculate which affiliations are to be removed from the room. These are all the old affiliations, that are now no longer affiliations.
+        final List<JID> affiliationsToReset = new ArrayList<>();
+        affiliationsToReset.addAll(room.getOwners());
+        affiliationsToReset.addAll(room.getAdmins());
+        affiliationsToReset.addAll(room.getMembers());
+        affiliationsToReset.addAll(room.getOutcasts());
+
+        // Calculate which are the new owners.
+        final Collection<JID> newOwners = new ArrayList<>();
+        if (mucRoomEntity.getOwners() != null) {
+            newOwners.addAll(MUCRoomUtils.convertStringsToJIDs(mucRoomEntity.getOwners()));
         }
-
-        // Owners
-        List<JID> ownersToAdd = MUCRoomUtils.convertStringsToJIDs(mucRoomEntity.getOwners());
-        allNewAffiliations.addAll(ownersToAdd);
-        room.addOwners(ownersToAdd, room.getRole());
         if (mucRoomEntity.getOwnerGroups() != null) {
             for (final String groupName : mucRoomEntity.getOwnerGroups()) {
-                final JID groupJID = UserUtils.checkAndGetJID(groupName);
-                room.addOwner(groupJID, room.getRole());
-                allNewAffiliations.add(groupJID);
+                newOwners.add(UserUtils.checkAndGetJID(groupName));
             }
         }
+        affiliationsToReset.removeAll(newOwners); // Do not remove these from the room after we're done re-affiliating everyone!
+        newOwners.removeAll(room.getOwners()); // Removing the ones that are already associated. We don't need to add these again.
+
+        // Update the room by adding new owners.
+        for (final JID newOwner : newOwners) {
+            log("Adding new 'owner' affiliation for '" + newOwner + "' to room: " + room.getName());
+            room.addOwner(newOwner, room.getRole());
+            allNewAffiliations.add(newOwner);
+        }
 
-        // Admins
+        // Calculate which are the new admins.
+        final Collection<JID> newAdmins = new ArrayList<>();
         if (mucRoomEntity.getAdmins() != null) {
-            List<JID> newAdmins = MUCRoomUtils.convertStringsToJIDs(mucRoomEntity.getAdmins());
-            newAdmins.removeAll(room.getAdmins());
-            allNewAffiliations.addAll(newAdmins);
-            room.addAdmins(newAdmins, room.getRole());
+            newAdmins.addAll(MUCRoomUtils.convertStringsToJIDs(mucRoomEntity.getAdmins()));
         }
         if (mucRoomEntity.getAdminGroups() != null) {
             for (final String groupName : mucRoomEntity.getAdminGroups()) {
-                final JID groupJID = UserUtils.checkAndGetJID(groupName);
-                room.addAdmin(groupJID, room.getRole());
-                allNewAffiliations.add(groupJID);
+                newAdmins.add(UserUtils.checkAndGetJID(groupName));
             }
         }
+        affiliationsToReset.removeAll(newAdmins); // Do not remove these from the room after we're done re-affiliating everyone!
+        newAdmins.removeAll(room.getAdmins()); // Removing the ones that are already associated. We don't need to add these again.
+
+        // Update the room by adding new admins.
+        for (final JID newAdmin : newAdmins) {
+            log("Adding new 'admin' affiliation for '" + newAdmin + "' to room: " + room.getName());
+            room.addAdmin(newAdmin, room.getRole());
+            allNewAffiliations.add(newAdmin);
+        }
 
-        // Members
+        // Calculate which are the new members.
+        final Collection<JID> newMembers = new ArrayList<>();
         if (mucRoomEntity.getMembers() != null) {
-            List<JID> newMembers = MUCRoomUtils.convertStringsToJIDs(mucRoomEntity.getMembers());
-            newMembers.removeAll(room.getMembers());
-            allNewAffiliations.addAll(newMembers);
-            for (JID memberJid : newMembers) {
-                room.addMember(memberJid, null, room.getRole());
-            }
+            newMembers.addAll(MUCRoomUtils.convertStringsToJIDs(mucRoomEntity.getMembers()));
         }
         if (mucRoomEntity.getMemberGroups() != null) {
             for (final String groupName : mucRoomEntity.getMemberGroups()) {
-                final JID groupJID = UserUtils.checkAndGetJID(groupName);
-                room.addMember(groupJID, null, room.getRole());
-                allNewAffiliations.add(groupJID);
+                newMembers.add(UserUtils.checkAndGetJID(groupName));
             }
         }
+        affiliationsToReset.removeAll(newMembers); // Do not remove these from the room after we're done re-affiliating everyone!
+        newMembers.removeAll(room.getMembers()); // Removing the ones that are already associated. We don't need to add these again.
+
+        // Update the room by adding new members.
+        for (final JID newMember : newMembers) {
+            log("Adding new 'member' affiliation for '" + newMember + "' to room: " + room.getName());
+            room.addMember(newMember, null, room.getRole());
+            allNewAffiliations.add(newMember);
+        }
 
-        // Outcasts
+        // Calculate which are the new outcasts.
+        final Collection<JID> newOutcasts = new ArrayList<>();
         if (mucRoomEntity.getOutcasts() != null) {
-            for (String outcastJid : mucRoomEntity.getOutcasts()) {
-                room.addOutcast(new JID(outcastJid), null, room.getRole());
-            }
+            newOutcasts.addAll(MUCRoomUtils.convertStringsToJIDs(mucRoomEntity.getOutcasts()));
         }
         if (mucRoomEntity.getOutcastGroups() != null) {
             for (final String groupName : mucRoomEntity.getOutcastGroups()) {
-                final JID groupJID = UserUtils.checkAndGetJID(groupName);
-                room.addOutcast(groupJID, null, room.getRole());
+                newOutcasts.add(UserUtils.checkAndGetJID(groupName));
             }
         }
+        affiliationsToReset.removeAll(newOutcasts); // Do not remove these from the room after we're done re-affiliating everyone!
+        newOutcasts.removeAll(room.getOutcasts()); // Removing the ones that are already associated. We don't need to add these again.
+
+        // Update the room by adding new outcasts.
+        for (final JID newOutcast : newOutcasts) {
+            log("Adding new 'outcast' affiliation for '" + newOutcast + "' to room: " + room.getName());
+            room.addOutcast(newOutcast, null, room.getRole());
+            allNewAffiliations.add(newOutcast);
+        }
+
+        // Finally, clean up every old affiliation that is not carrying over.
+        for (JID affiliationToReset : affiliationsToReset) {
+            log("Removing old affiliation for '" + affiliationToReset + "' from room: " + room.getName());
+            room.addNone(affiliationToReset, room.getRole());
+        }
+
         return allNewAffiliations;
     }