SECURITY: Check permissions when autocompleting mentions

app/models/user_search.rb

@@ -16,7 +16,9 @@ class UserSearch
     @limit = opts[:limit] || 20
     @groups = opts[:groups]
     @guardian = Guardian.new(@searching_user)
-    @guardian.ensure_can_see_groups!(@groups) if @groups
+    @guardian.ensure_can_see_groups_members! @groups if @groups
+    @guardian.ensure_can_see_category! Category.find(@category_id) if @category_id
+    @guardian.ensure_can_see_topic! Topic.find(@topic_id) if @topic_id
   end
 
   def scoped_users
@@ -117,18 +119,26 @@ class UserSearch
 
     # 3. category matches
     if secure_category_id
+      @searching_user.present?
+
+      category_groups = Group.where(<<~SQL, secure_category_id, MAX_SIZE_PRIORITY_MENTION)
+        groups.id IN (
+          SELECT group_id FROM category_groups
+          JOIN groups g ON group_id = g.id
+          WHERE
+            category_id = ? AND
+            user_count < ?
+        )
+      SQL
+
+      category_groups = category_groups.members_visible_groups(@searching_user)
+
       in_category = filtered_by_term_users
-        .where(<<~SQL, secure_category_id, MAX_SIZE_PRIORITY_MENTION)
+        .where(<<~SQL, category_groups.pluck(:id))
           users.id IN (
             SELECT gu.user_id
             FROM group_users gu
-            WHERE group_id IN (
-              SELECT group_id FROM category_groups
-              JOIN groups g ON group_id = g.id
-              WHERE
-                category_id = ? AND
-                user_count < ?
-            )
+            WHERE group_id IN (?)
             LIMIT 200
           )
           SQL

lib/guardian.rb

@@ -249,6 +249,15 @@ class Guardian
     true
   end
 
+  def can_see_groups_members?(groups)
+    return false if groups.blank?
+
+    requested_group_ids = groups.map(&:id) # Can't use pluck, groups could be a regular array
+    matching_groups = Group.where(id: requested_group_ids).members_visible_groups(user)
+
+    matching_groups.pluck(:id).sort == requested_group_ids.sort
+  end
+
   # Can we impersonate this user?
   def can_impersonate?(target)
     target &&

spec/models/user_search_spec.rb

@@ -26,36 +26,57 @@ describe UserSearch do
     UserSearch.new(*args).search
   end
 
-  it 'finds users in secure category' do
-    group = Fabricate(:group)
-    user = Fabricate(:user)
-    group.add(user)
-    group.save
+  context 'with a secure category' do
+    fab!(:group) { Fabricate :group }
+    fab!(:user) { Fabricate :user }
+    fab!(:searching_user) { Fabricate :user }
+    before_all do
+      group.add(user)
+      group.add(searching_user)
+      group.save
+    end
+    fab!(:category) { Fabricate(:category,
+                        read_restricted: true,
+                        user: user)
+    }
+    before_all { Fabricate(:category_group, category: category, group: group) }
 
-    category =
-      Fabricate(
-        :category,
-        read_restricted: true,
-        user: user
+    it 'autocompletes with people in the category' do
+      results = search_for("", searching_user: searching_user, category_id: category.id)
+
+      expect(user.username).to eq(results[0].username)
+      expect(results.length).to eq(1)
+    end
+
+    it 'will lookup the category from the topic id' do
+      topic = Fabricate(:topic, category: category)
+      _post = Fabricate(:post, user: topic.user, topic: topic)
+
+      results = search_for("", searching_user: searching_user, topic_id: topic.id)
+
+      expect(results.length).to eq(2)
+
+      expect(results.map(&:username)).to contain_exactly(
+        user.username, topic.user.username
       )
+    end
 
-    Fabricate(:category_group, category: category, group: group)
+    it 'will raise an error if the user cannot see the category' do
+      expect do
+        search_for("", searching_user: Fabricate(:user), category_id: category.id)
+      end.to raise_error(Discourse::InvalidAccess)
+    end
 
-    results = search_for("", category_id: category.id)
+    it 'will respect the group member visibility setting' do
+      group.update(members_visibility_level: Group.visibility_levels[:owners])
+      results = search_for("", searching_user: searching_user, category_id: category.id)
+      expect(results.length).to eq(0)
 
-    expect(user.username).to eq(results[0].username)
-    expect(results.length).to eq(1)
+      group.add_owner(searching_user)
+      results = search_for("", searching_user: searching_user, category_id: category.id)
+      expect(results.length).to eq(1)
+    end
 
-    topic = Fabricate(:topic, category: category)
-    _post = Fabricate(:post, user: topic.user, topic: topic)
-
-    results = search_for("", topic_id: topic.id)
-
-    expect(results.length).to eq(2)
-
-    expect(results.map(&:username)).to contain_exactly(
-      user.username, topic.user.username
-    )
   end
 
   it 'allows for correct underscore searching' do
@@ -156,6 +177,25 @@ describe UserSearch do
       expect(results[1]).to eq(user5)
     end
 
+    it "only reveals topic participants to people with permission" do
+      pm_topic = Fabricate(:private_message_post).topic
+
+      # Anonymous, does not have access
+      expect do
+        search_for("", topic_id: pm_topic.id)
+      end.to raise_error(Discourse::InvalidAccess)
+
+      # Random user, does not have access
+      expect do
+        search_for("", topic_id: pm_topic.id, searching_user: user1)
+      end.to raise_error(Discourse::InvalidAccess)
+
+      pm_topic.invite(pm_topic.user, user1.username)
+      results = search_for("", topic_id: pm_topic.id, searching_user: user1)
+      expect(results.length).to eq(1)
+      expect(results[0]).to eq(pm_topic.user)
+    end
+
     it "only searches by name when enabled" do
       # When searching by name is enabled, it returns the record
       SiteSetting.enable_names = true