Fixed private group messages being exposed in suggested topics.

Revert "Revert "PERF: Avoid unnecessary expensive joins if possible.""

This reverts commit d9714c21c8.

lib/topic_query.rb

@@ -112,6 +112,7 @@ class TopicQuery
             ON topic_allowed_groups.group_id = gu.group_id
             AND user_id = #{@user.id.to_i}
           ")
+          .where("gu.group_id IS NOT NULL")
           .pluck(:group_id)
 
         {
@@ -729,10 +730,8 @@ class TopicQuery
     end
 
     def new_messages(params)
-
       TopicQuery.new_filter(messages_for_groups_or_user(params[:my_group_ids]), Time.at(SiteSetting.min_new_topics_time).to_datetime)
         .limit(params[:count])
-
     end
 
     def unread_messages(params)
@@ -744,53 +743,60 @@ class TopicQuery
     end
 
     def related_messages_user(params)
-      user_ids = ((params[:target_user_ids] || []) << -10)
+      messages = messages_for_user.limit(params[:count])
-      user_ids = ActiveRecord::Base.send(:sanitize_sql_array, user_ids.join(','))
+      messages = allowed_messages(messages, params)
-      group_ids = (((params[:target_group_ids] - params[:my_group_ids]) || []) << -10)
-      group_ids = ActiveRecord::Base.send(:sanitize_sql_array, group_ids.join(','))
-
-      result = messages_for_user
-        .limit(params[:count])
-        .joins("
-          LEFT JOIN topic_allowed_users ta2
-          ON topics.id = ta2.topic_id
-          AND ta2.user_id IN (#{user_ids})",
-        )
-        .joins("
-          LEFT JOIN topic_allowed_groups tg
-          ON topics.id = tg.topic_id
-          AND tg.group_id IN (#{group_ids})",
-        )
-        .where("ta2.topic_id IS NOT NULL OR tg.topic_id IS NOT NULL")
     end
 
     def related_messages_group(params)
-      messages_for_groups_or_user(params[:my_group_ids])
+      messages = messages_for_groups_or_user(params[:my_group_ids]).limit(params[:count])
-        .limit(params[:count])
+      messages = allowed_messages(messages, params)
-        .where('topics.id IN (
+    end
-                SELECT ta.topic_id
-                FROM topic_allowed_users ta
-                WHERE ta.user_id IN (:user_ids)
-              ) OR
-                topics.id IN (
-                  SELECT tg.topic_id
-                  FROM topic_allowed_groups tg
-                  WHERE tg.group_id IN (:group_ids)
-              )
-              ', user_ids: (params[:target_user_ids] || []) + [-10],
-                 group_ids: ((params[:target_group_ids] - params[:my_group_ids]) || []) + [-10])
 
+    def allowed_messages(messages, params)
+      user_ids = (params[:target_user_ids] || [])
+      group_ids = ((params[:target_group_ids] - params[:my_group_ids]) || [])
+
+      if user_ids.present?
+        messages =
+          messages.joins("
+            LEFT JOIN topic_allowed_users ta2
+            ON topics.id = ta2.topic_id
+            AND ta2.user_id IN (#{sanitize_sql_array(user_ids)})
+          ")
+      end
+
+      if group_ids.present?
+        messages =
+          messages.joins("
+            LEFT JOIN topic_allowed_groups tg2
+            ON topics.id = tg2.topic_id
+            AND tg2.group_id IN (#{sanitize_sql_array(group_ids)})
+          ")
+      end
+
+      messages =
+        if user_ids.present? && group_ids.present?
+          messages.where("ta2.topic_id IS NOT NULL OR tg2.topic_id IS NOT NULL")
+        elsif user_ids.present?
+          messages.where("ta2.topic_id IS NOT NULL")
+        elsif group_ids.present?
+          messages.where("tg2.topic_id IS NOT NULL")
+        end
     end
 
     def messages_for_groups_or_user(group_ids)
       if group_ids.present?
         base_messages
-          .where('topics.id IN (
+          .joins("
-                                  SELECT topic_id
+            LEFT JOIN (
-                                    FROM topic_allowed_groups tg
+              SELECT * FROM topic_allowed_groups _tg
-                                    JOIN group_users gu ON gu.user_id = :user_id AND gu.group_id = tg.group_id
+              LEFT JOIN group_users gu
-                                    WHERE gu.group_id IN (:group_ids)
+              ON gu.user_id = #{@user.id.to_i}
-                 )', user_id: @user.id, group_ids: group_ids)
+              AND gu.group_id = _tg.group_id
+              WHERE gu.group_id IN (#{sanitize_sql_array(group_ids)})
+            ) tg ON topics.id = tg.topic_id
+          ")
+          .where("tg.topic_id IS NOT NULL")
       else
         messages_for_user
       end
@@ -847,4 +853,10 @@ class TopicQuery
 
       result.order('topics.bumped_at DESC')
     end
+
+  private
+
+    def sanitize_sql_array(input)
+      ActiveRecord::Base.send(:sanitize_sql_array, input.join(','))
+    end
 end

spec/components/topic_query_spec.rb

@@ -759,6 +759,65 @@ describe TopicQuery do
 
       end
 
+      context 'with private messages' do
+        let(:group_user) { Fabricate(:user) }
+        let(:group) { Fabricate(:group) }
+        let(:another_group) { Fabricate(:group) }
+
+        let!(:topic) do
+          Fabricate(:private_message_topic,
+            topic_allowed_users: [
+              Fabricate.build(:topic_allowed_user, user: user)
+            ],
+            topic_allowed_groups: [
+              Fabricate.build(:topic_allowed_group, group: group)
+            ]
+          )
+        end
+
+        let!(:private_message) do
+          Fabricate(:private_message_topic,
+            topic_allowed_users: [
+              Fabricate.build(:topic_allowed_user, user: user)
+            ],
+            topic_allowed_groups: [
+              Fabricate.build(:topic_allowed_group, group: group),
+              Fabricate.build(:topic_allowed_group, group: another_group),
+            ]
+          )
+        end
+
+        let!(:private_group_topic) do
+          Fabricate(:private_message_topic,
+            user: Fabricate(:user),
+            topic_allowed_groups: [
+              Fabricate.build(:topic_allowed_group, group: group)
+            ]
+          )
+        end
+
+        before do
+          group.add(group_user)
+          another_group.add(user)
+        end
+
+        describe 'as user not part of group' do
+          let!(:user) { Fabricate(:user) }
+
+          it 'should not return topics by the group user' do
+            expect(suggested_topics).to eq([private_message.id])
+          end
+        end
+
+        describe 'as user part of group' do
+          let!(:user) { group_user }
+
+          it 'should return the group topics' do
+            expect(suggested_topics).to eq([private_group_topic.id, private_message.id])
+          end
+        end
+      end
+
       context 'with some existing topics' do
         let!(:partially_read) { Fabricate(:post, user: creator).topic }
         let!(:new_topic) { Fabricate(:post, user: creator).topic }