Fix: Enforce new rules when assigning a topic. (#46)

* assign_to user must be allowed to assign.
* assign_to user must have access to the topic

app/controllers/discourse_assign/assign_controller.rb

@@ -85,8 +85,11 @@ module DiscourseAssign
     private
 
     def translate_failure(reason, user)
-      if reason == :already_assigned
+      case reason
+      when :already_assigned
         { error: I18n.t('discourse_assign.already_assigned', username: user.username) }
+      when :forbidden_assign_to
+        { error: I18n.t('discourse_assign.forbidden_assign_to', username: user.username) }
       else
         max = SiteSetting.max_assigned_topics
         { error: I18n.t('discourse_assign.too_many_assigns', username: user.username, max: max) }

config/locales/server.en.yml

@@ -22,6 +22,7 @@ en:
     already_claimed: "That topic has already been claimed."
     already_assigned: 'Topic is already assigned to @%{username}'
     too_many_assigns: "@%{username} has already reached the maximum number of assigned topics (%{max})."
+    forbidden_assign_to: "@%{username} can't be assigned since they don't have access to this topic."
     flag_assigned: "Sorry, that flag's topic is assigned to another user"
     flag_unclaimed: "You must claim that topic before acting on the flag"
     topic_assigned_excerpt: "assigned you the topic '%{title}'"

lib/topic_assigner.rb

@@ -108,7 +108,7 @@ class ::TopicAssigner
   end
 
   def allowed_user_ids
-    User.assign_allowed.pluck(:id)
+    @allowed_user_ids ||= User.assign_allowed.pluck(:id)
   end
 
   def can_assign_to?(user)
@@ -124,7 +124,25 @@ class ::TopicAssigner
     assigned_total < SiteSetting.max_assigned_topics
   end
 
+  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?
+  end
+
   def assign(assign_to, silent: false)
+    return { success: false, reason: :forbidden_assign_to } unless can_be_assigned?(assign_to)
     return { success: false, reason: :already_assigned } if @topic.custom_fields && @topic.custom_fields[ASSIGNED_TO_ID] == assign_to.id.to_s
     return { success: false, reason: :too_many_assigns } unless can_assign_to?(assign_to)
 

spec/components/topic_query_spec.rb

@@ -1,6 +1,7 @@
 # frozen_string_literal: true
 
 require 'rails_helper'
+require_relative '../support/assign_allowed_group'
 
 describe TopicQuery do
   before do
@@ -10,6 +11,13 @@ describe TopicQuery do
   let(:user) { Fabricate(:user) }
   let(:user2) { Fabricate(:user) }
 
+  include_context 'A group that is allowed to assign'
+
+  before do
+    add_to_assign_allowed_group(user)
+    add_to_assign_allowed_group(user2)
+  end
+
   describe '#list_messages_assigned' do
     before do
       @private_message = Fabricate(:private_message_topic, user: user)
@@ -63,14 +71,13 @@ describe TopicQuery do
       assign_to(topic, user)
     end
 
-    let(:group) { Fabricate(:group).add(user) }
     let(:group2) { Fabricate(:group) }
 
     let(:group_assigned_topic) do
       topic = Fabricate(:private_message_topic,
         topic_allowed_users: [],
         topic_allowed_groups: [
-          Fabricate.build(:topic_allowed_group, group: group),
+          Fabricate.build(:topic_allowed_group, group: assign_allowed_group),
           Fabricate.build(:topic_allowed_group, group: group2)
         ],
       )
@@ -101,7 +108,7 @@ describe TopicQuery do
         TopicQuery.new(user).list_private_messages_assigned(user).topics
       ).to contain_exactly(assigned_topic, group_assigned_topic)
 
-      GroupArchivedMessage.archive!(group.id, group_assigned_topic)
+      GroupArchivedMessage.archive!(assign_allowed_group.id, group_assigned_topic)
 
       expect(
         TopicQuery.new(user).list_private_messages_assigned(user).topics

spec/integration/assign_spec.rb

@@ -1,6 +1,7 @@
 # frozen_string_literal: true
 
 require 'rails_helper'
+require_relative '../support/assign_allowed_group'
 
 describe 'integration tests' do
   before do
@@ -35,6 +36,13 @@ describe 'integration tests' do
     let(:user2) { pm.allowed_users.last }
     let(:channel) { "/private-messages/assigned" }
 
+    include_context 'A group that is allowed to assign'
+
+    before do
+      add_to_assign_allowed_group(user)
+      add_to_assign_allowed_group(user2)
+    end
+
     def assert_publish_topic_state(topic, user)
       messages = MessageBus.track_publish do
         yield
@@ -93,6 +101,13 @@ describe 'integration tests' do
     let(:user1) { Fabricate(:user) }
     let(:user2) { Fabricate(:user) }
 
+    include_context 'A group that is allowed to assign'
+
+    before do
+      add_to_assign_allowed_group(user1)
+      add_to_assign_allowed_group(user2)
+    end
+
     it "assigns topic" do
       DiscourseEvent.trigger(:assign_topic, topic, user1, admin)
       expect(topic.reload.custom_fields[TopicAssigner::ASSIGNED_TO_ID].to_i).to eq(user1.id)

spec/lib/pending_assigns_reminder_spec.rb

@@ -1,6 +1,7 @@
 # frozen_string_literal: true
 
 require 'rails_helper'
+require_relative '../support/assign_allowed_group'
 
 RSpec.describe PendingAssignsReminder do
   before { SiteSetting.assign_enabled = true }
@@ -25,7 +26,11 @@ RSpec.describe PendingAssignsReminder do
   describe 'when the user has multiple tasks' do
     let(:system) { Discourse.system_user }
 
+    include_context 'A group that is allowed to assign'
+
     before do
+      add_to_assign_allowed_group(user)
+
       @post1 = Fabricate(:post)
       @post2 = Fabricate(:post)
       @post3 = Fabricate(:post)

spec/lib/topic_assigner_spec.rb

@@ -21,7 +21,7 @@ RSpec.describe TopicAssigner do
   describe 'assigning and unassigning private message' do
     it 'should publish the right message' do
       user = pm.allowed_users.first
-      user.groups << assign_allowed_group
+      assign_allowed_group.add(user)
       assigner = described_class.new(pm, user)
 
       assert_publish_topic_state(pm, user) { assigner.assign(user) }
@@ -118,6 +118,7 @@ RSpec.describe TopicAssigner do
 
     it "doesn't assign the same user more than once" do
       SiteSetting.assign_mailer_enabled = true
+      another_mod = Fabricate(:moderator, groups: [assign_allowed_group])
 
       Email::Sender.any_instance.expects(:send).once
       expect(assigned_to?(moderator)).to eq(true)
@@ -126,11 +127,11 @@ RSpec.describe TopicAssigner do
       expect(assigned_to?(moderator)).to eq(false)
 
       Email::Sender.any_instance.expects(:send).once
-      expect(assigned_to?(Fabricate(:moderator))).to eq(true)
+      expect(assigned_to?(another_mod)).to eq(true)
     end
 
-    def assigned_to?(moderator)
-      assigner.assign(moderator).fetch(:success)
+    def assigned_to?(asignee)
+      assigner.assign(asignee).fetch(:success)
     end
 
     it "doesn't assign if the user has too many assigned topics" do
@@ -170,6 +171,31 @@ RSpec.describe TopicAssigner do
       expect(second_assign[:success]).to eq(false)
       expect(third_assign[:success]).to eq(true)
     end
+
+    fab!(:admin) { Fabricate(:admin) }
+
+    it 'fails to assign when the assigned user cannot view the topic' do
+      assign = TopicAssigner.new(pm, 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
+
+      assign = TopicAssigner.new(pm, admin).assign(moderator)
+
+      expect(assign[:success]).to eq(true)
+    end
+
+    it "assigns the PM to the moderator when it's a member of an allowed group" do
+      pm.allowed_groups << assign_allowed_group
+
+      assign = TopicAssigner.new(pm, admin).assign(moderator)
+
+      expect(assign[:success]).to eq(true)
+    end
   end
 
   context "unassign_on_close" do

spec/requests/assign_controller_spec.rb

@@ -1,6 +1,7 @@
 # frozen_string_literal: true
 
 require 'rails_helper'
+require_relative '../support/assign_allowed_group'
 
 RSpec.describe DiscourseAssign::AssignController do
 
@@ -36,8 +37,8 @@ RSpec.describe DiscourseAssign::AssignController do
 
       it 'includes users in allowed groups' do
         allowed_group = Group.find_by(name: 'everyone')
-        user2.groups << allowed_group
-        user2.groups << default_allowed_group
+        allowed_group.add(user2)
+
         defaults = if above_min_version
           "#{default_allowed_group.id}|#{allowed_group.id}"
         else
@@ -55,7 +56,7 @@ RSpec.describe DiscourseAssign::AssignController do
 
       it 'does not include users from disallowed groups' do
         allowed_group = Group.find_by(name: 'everyone')
-        user2.groups << allowed_group
+        allowed_group.add(user2)
         SiteSetting.assign_allowed_on_groups = above_min_version ? default_allowed_group.id.to_s : default_allowed_group.name
         TopicAssigner.new(post.topic, user).assign(user2)
 
@@ -85,8 +86,12 @@ RSpec.describe DiscourseAssign::AssignController do
   end
 
   context '#assign' do
+
+    include_context 'A group that is allowed to assign'
+
     before do
       sign_in(user)
+      add_to_assign_allowed_group(user2)
     end
 
     it 'assigns topic to a user' do
@@ -115,19 +120,20 @@ RSpec.describe DiscourseAssign::AssignController do
     end
 
     it 'fails to assign topic to the user if they already reached the max assigns limit' do
-      another_admin = Fabricate(:admin)
+      another_user = Fabricate(:user)
+      add_to_assign_allowed_group(another_user)
       another_post = Fabricate(:post)
       max_assigns = 1
       SiteSetting.max_assigned_topics = max_assigns
-      TopicAssigner.new(post.topic, user).assign(another_admin)
+      TopicAssigner.new(post.topic, user).assign(another_user)
 
       put '/assign/assign.json', params: {
-        topic_id: another_post.topic_id, username: another_admin.username
+        topic_id: another_post.topic_id, username: another_user.username
       }
 
       expect(response.status).to eq(400)
       expect(JSON.parse(response.body)['error']).to eq(
-        I18n.t('discourse_assign.too_many_assigns', username: another_admin.username, max: max_assigns)
+        I18n.t('discourse_assign.too_many_assigns', username: another_user.username, max: max_assigns)
       )
     end
   end

spec/serializers/topic_list_serializer_spec.rb

@@ -3,7 +3,7 @@
 require 'rails_helper'
 
 RSpec.describe TopicListSerializer do
-  let(:user) { Fabricate(:user) }
+  fab!(:user) { Fabricate(:user) }
 
   let(:private_message_topic) do
     topic = Fabricate(:private_message_topic,
@@ -31,8 +31,11 @@ RSpec.describe TopicListSerializer do
   let(:guardian) { Guardian.new(user) }
   let(:serializer) { TopicListSerializer.new(topic_list, scope: guardian) }
 
+  include_context 'A group that is allowed to assign'
+
   before do
     SiteSetting.assign_enabled = true
+    add_to_assign_allowed_group(user)
   end
 
   describe '#assigned_messages_count' do

spec/support/assign_allowed_group.rb

@@ -0,0 +1,11 @@
+# frozen_string_literal: true
+
+shared_context 'A group that is allowed to assign' do
+  fab!(:assign_allowed_group) { Fabricate(:group) }
+
+  before { SiteSetting.assign_allowed_on_groups += "|#{assign_allowed_group.id}" }
+
+  def add_to_assign_allowed_group(user)
+    assign_allowed_group.add(user)
+  end
+end