FIX: Remove superfluous topic allowed users on group invite (#14656)

FIX: Remove superfluous topic allowed users on group invite (#14656)

When inviting a group to a topic, there may be members of the group already in the topic as topic allowed users. These can be safely removed from the topic, because they are implicitly allowed in the topic based on their group membership.

Also, this prevents issues with group SMTP emails, which rely on the topic_allowed_users of the topic to send to and cc’s for emails, and if there are members of the group as topic_allowed_users then that complicates things and causes odd behaviour.

We also ensure that the OP of the topic is not removed from the topic_allowed_users when a group they belong to is added, as it will make it harder to add them back later.

diff --git a/app/assets/javascripts/discourse/app/components/invite-panel.js b/app/assets/javascripts/discourse/app/components/invite-panel.js
index dd55ebe..17ea674 100644
--- a/app/assets/javascripts/discourse/app/components/invite-panel.js
+++ b/app/assets/javascripts/discourse/app/components/invite-panel.js
@@ -351,12 +351,11 @@ export default Component.extend({
     if (this.isInviteeGroup) {
       return this.inviteModel
         .createGroupInvite(this.invitee.trim())
-        .then((data) => {
+        .then(() => {
           model.setProperties({ saving: false, finished: true });
-          this.get("inviteModel.details.allowed_groups").pushObject(
-            EmberObject.create(data.group)
-          );
-          this.appEvents.trigger("post-stream:refresh");
+          this.inviteModel.reload().then(() => {
+            this.appEvents.trigger("post-stream:refresh");
+          });
         })
         .catch(onerror);
     } else {
diff --git a/app/models/topic.rb b/app/models/topic.rb
index 5f807a7..81d0d56 100644
--- a/app/models/topic.rb
+++ b/app/models/topic.rb
@@ -971,16 +971,38 @@ class Topic < ActiveRecord::Base
   end
 
   def invite_group(user, group)
-    TopicAllowedGroup.create!(topic_id: id, group_id: group.id)
-    allowed_groups.reload
+    TopicAllowedGroup.create!(topic_id: self.id, group_id: group.id)
+    self.allowed_groups.reload
 
-    last_post = posts.order('post_number desc').where('not hidden AND posts.deleted_at IS NULL').first
+    last_post = self.posts.order('post_number desc').where('not hidden AND posts.deleted_at IS NULL').first
     if last_post
       Jobs.enqueue(:post_alert, post_id: last_post.id)
       add_small_action(user, "invited_group", group.name)
       Jobs.enqueue(:group_pm_alert, user_id: user.id, group_id: group.id, post_id: last_post.id)
     end
 
+    # If the group invited includes the OP of the topic as one of is members,
+    # we cannot strip the topic_allowed_user record since it will be more
+    # complicated to recover the topic_allowed_user record for the OP if the
+    # group is removed.
+    allowed_user_where_clause = <<~SQL
+      users.id IN (
+        SELECT topic_allowed_users.user_id
+        FROM topic_allowed_users
+        INNER JOIN group_users ON group_users.user_id = topic_allowed_users.user_id
+        INNER JOIN topic_allowed_groups ON topic_allowed_groups.group_id = group_users.group_id
+        WHERE topic_allowed_groups.group_id = :group_id AND
+              topic_allowed_users.topic_id = :topic_id AND
+              topic_allowed_users.user_id != :op_user_id
+      )
+    SQL
+    User.where([
+      allowed_user_where_clause,
+      { group_id: group.id, topic_id: self.id, op_user_id: self.user_id }
+    ]).find_each do |allowed_user|
+      remove_allowed_user(Discourse.system_user, allowed_user)
+    end
+
     true
   end
 
diff --git a/spec/models/topic_spec.rb b/spec/models/topic_spec.rb
index 64d599f..40d9612 100644
--- a/spec/models/topic_spec.rb
+++ b/spec/models/topic_spec.rb
@@ -1021,6 +1021,51 @@ describe Topic do
               .to eq(Notification.types[:group_message_summary])
 
           end
+
+          it "removes users in topic_allowed_users who are part of the added group" do
+            admins = Group[:admins]
+            admins.update!(messageable_level: Group::ALIAS_LEVELS[:everyone])
+
+            # clear up the state so we can be more explicit with the test
+            TopicAllowedUser.where(topic: topic).delete_all
+            user0 = topic.user
+            user1 = Fabricate(:user)
+            user2 = Fabricate(:user)
+            user3 = Fabricate(:user)
+            Fabricate(:topic_allowed_user, topic: topic, user: user0)
+            Fabricate(:topic_allowed_user, topic: topic, user: user1)
+            Fabricate(:topic_allowed_user, topic: topic, user: user2)
+            Fabricate(:topic_allowed_user, topic: topic, user: user3)
+
+            admins.add(user1)
+            admins.add(user2)
+
+            other_topic = Fabricate(:topic)
+            Fabricate(:topic_allowed_user, user: user1, topic: other_topic)
+
+            expect(topic.invite_group(topic.user, admins)).to eq(true)
+            expect(topic.posts.last.action_code).to eq("removed_user")
+            expect(topic.allowed_users).to match_array([user0, user3, Discourse.system_user])
+            expect(other_topic.allowed_users).to match_array([user1])
+          end
+
+          it "does not remove the OP from topic_allowed_users if they are part of an added group" do
+            admins = Group[:admins]
+            admins.update!(messageable_level: Group::ALIAS_LEVELS[:everyone])
+
+            # clear up the state so we can be more explicit with the test
+            TopicAllowedUser.where(topic: topic).delete_all
+            user0 = topic.user
+            user1 = Fabricate(:user)
+            Fabricate(:topic_allowed_user, topic: topic, user: user0)
+            Fabricate(:topic_allowed_user, topic: topic, user: user1)
+
+            admins.add(topic.user)
+            admins.add(user1)
+
+            expect(topic.invite_group(topic.user, admins)).to eq(true)
+            expect(topic.allowed_users).to match_array([topic.user, Discourse.system_user])
+          end
         end
       end
     end

GitHub sha: 2b40049abb498095beec29787955e77c5cfc89b8

This commit appears in #14656 which was approved by tgxworld. It was merged by martin.