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

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
diff --git a/app/controllers/discourse_assign/assign_controller.rb b/app/controllers/discourse_assign/assign_controller.rb
index 2fa80ba..f9c7a2c 100644
--- a/app/controllers/discourse_assign/assign_controller.rb
+++ b/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) }
diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml
index 6cb18f7..d89ac26 100644
--- a/config/locales/server.en.yml
+++ b/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}'"
diff --git a/lib/topic_assigner.rb b/lib/topic_assigner.rb
index 6372e46..74de111 100644
--- a/lib/topic_assigner.rb
+++ b/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)
 
diff --git a/spec/components/topic_query_spec.rb b/spec/components/topic_query_spec.rb
index 8733612..a7ae070 100644
--- a/spec/components/topic_query_spec.rb
+++ b/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
diff --git a/spec/integration/assign_spec.rb b/spec/integration/assign_spec.rb
index 46cec29..d57fc11 100644
--- a/spec/integration/assign_spec.rb
+++ b/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)
diff --git a/spec/lib/pending_assigns_reminder_spec.rb b/spec/lib/pending_assigns_reminder_spec.rb
index a8373be..6956830 100644
--- a/spec/lib/pending_assigns_reminder_spec.rb
+++ b/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)
diff --git a/spec/lib/topic_assigner_spec.rb b/spec/lib/topic_assigner_spec.rb
index 69a5e6e..616323c 100644
--- a/spec/lib/topic_assigner_spec.rb
+++ b/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)

[... diff too long, it was truncated ...]

GitHub sha: d268d4f8