SECURITY: Improve topic permission checks

app/controllers/discourse_assign/assign_controller.rb

@@ -83,6 +83,8 @@ module DiscourseAssign
     end
 
     def assigned
+      raise Discourse::InvalidAccess unless current_user&.admin?
+
       offset = (params[:offset] || 0).to_i
       limit = (params[:limit] || 100).to_i
 

lib/pending_assigns_reminder.rb

@@ -35,8 +35,11 @@ class PendingAssignsReminder
   end
 
   def assigned_topics(user, order:)
+    secure = Topic.listable_topics.secured(Guardian.new(user)).or(Topic.private_messages_for_user(user))
+
     Topic.joins(:_custom_fields).select(:slug, :id, :title, :fancy_title, 'topic_custom_fields.created_at AS assigned_at')
       .where('topic_custom_fields.name = ? AND topic_custom_fields.value = ?', TopicAssigner::ASSIGNED_TO_ID, user.id.to_s)
+      .merge(secure)
       .order("topic_custom_fields.created_at #{order}")
       .limit(3)
   end

lib/topic_assigner.rb

@@ -136,19 +136,7 @@ class ::TopicAssigner
 
   def can_be_assigned?(assign_to)
     return false unless allowed_user_ids.include?(assign_to.id)
-    return true if (!@topic.private_message? || assign_to.admin?)
-
-    results = DB.query_single(<<~SQL
-      SELECT 1
-      FROM topics
-      LEFT OUTER JOIN topic_allowed_users tau ON tau.topic_id = topics.id
-      LEFT OUTER JOIN topic_allowed_groups tag ON tag.topic_id = topics.id
-      LEFT OUTER JOIN group_users gu ON gu.group_id = tag.group_id
-      WHERE topics.id = #{@topic.id} AND (gu.user_id = #{assign_to.id} OR tau.user_id = #{assign_to.id})
-    SQL
-    )
-
-    results.present?
+    Guardian.new(assign_to).can_see_topic?(@topic)
   end
 
   def assign(assign_to, silent: false)

spec/lib/pending_assigns_reminder_spec.rb

@@ -31,14 +31,19 @@ RSpec.describe PendingAssignsReminder do
     before do
       add_to_assign_allowed_group(user)
 
+      secure_category = Fabricate(:private_category, group: Fabricate(:group))
+
       @post1 = Fabricate(:post)
       @post2 = Fabricate(:post)
       @post2.topic.update_column(:fancy_title, nil)
       @post3 = Fabricate(:post)
+      @post4 = Fabricate(:post)
       TopicAssigner.new(@post1.topic, user).assign(user)
       TopicAssigner.new(@post2.topic, user).assign(user)
       TopicAssigner.new(@post3.topic, user).assign(user)
+      TopicAssigner.new(@post4.topic, user).assign(user)
       @post3.topic.trash!
+      @post4.topic.update(category: secure_category)
     end
 
     it 'creates a reminder for a particular user and sets the timestamp of the last reminder' do
@@ -57,11 +62,13 @@ RSpec.describe PendingAssignsReminder do
 
       expect(topic.title).to eq(I18n.t(
         'pending_assigns_reminder.title',
-        pending_assignments: 2
+        pending_assignments: 3
       ))
 
       expect(post.raw).to include(@post1.topic.fancy_title)
       expect(post.raw).to include(@post2.topic.fancy_title)
+      expect(post.raw).to_not include(@post3.topic.fancy_title)
+      expect(post.raw).to_not include(@post4.topic.fancy_title)
 
       expect(
         user.reload.custom_fields[described_class::REMINDED_AT].to_datetime

spec/lib/topic_assigner_spec.rb

@@ -32,6 +32,8 @@ RSpec.describe TopicAssigner do
   context "assigning and unassigning" do
     let(:post) { Fabricate(:post) }
     let(:topic) { post.topic }
+    let(:secure_category) { Fabricate(:private_category, group: Fabricate(:group)) }
+    let(:secure_topic) { Fabricate(:post).topic.tap { |t| t.update(category: secure_category) } }
     let(:moderator) { Fabricate(:moderator, groups: [assign_allowed_group]) }
     let(:moderator2) { Fabricate(:moderator, groups: [assign_allowed_group]) }
     let(:assigner) { TopicAssigner.new(topic, moderator2) }
@@ -189,13 +191,20 @@ RSpec.describe TopicAssigner do
 
     fab!(:admin) { Fabricate(:admin) }
 
-    it 'fails to assign when the assigned user cannot view the topic' do
+    it 'fails to assign when the assigned user cannot view the pm' do
       assign = TopicAssigner.new(pm, admin).assign(moderator)
 
       expect(assign[:success]).to eq(false)
       expect(assign[:reason]).to eq(:forbidden_assign_to)
     end
 
+    it 'fails to assign when the assigned user cannot view the topic' do
+      assign = TopicAssigner.new(secure_topic, admin).assign(moderator)
+
+      expect(assign[:success]).to eq(false)
+      expect(assign[:reason]).to eq(:forbidden_assign_to)
+    end
+
     it "assigns the PM to the moderator when it's included in the list of allowed users" do
       pm.allowed_users << moderator
 

spec/requests/assign_controller_spec.rb

@@ -175,6 +175,25 @@ RSpec.describe DiscourseAssign::AssignController do
       get '/assign/assigned.json', params: { offset: 2 }
       expect(JSON.parse(response.body)['topics'].map { |t| t['id'] }).to match_array([post1.topic_id])
     end
+
+    context "with custom allowed groups" do
+      let(:custom_allowed_group) { Fabricate(:group, name: 'mygroup') }
+      let(:other_user) { Fabricate(:user, groups: [custom_allowed_group]) }
+      before do
+        SiteSetting.assign_allowed_on_groups += "|#{custom_allowed_group.id}"
+      end
+
+      it 'works for admins' do
+        get '/assign/assigned.json'
+        expect(response.status).to eq(200)
+      end
+
+      it 'does not work for other groups' do
+        sign_in(other_user)
+        get '/assign/assigned.json'
+        expect(response.status).to eq(403)
+      end
+    end
   end
 
 end