From d268d4f817a1203168ea56f2af985feb3022d7c2 Mon Sep 17 00:00:00 2001 From: Roman Rizzi Date: Thu, 5 Sep 2019 10:31:52 -0300 Subject: [PATCH] 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 --- .../discourse_assign/assign_controller.rb | 5 ++- config/locales/server.en.yml | 1 + lib/topic_assigner.rb | 20 ++++++++++- spec/components/topic_query_spec.rb | 13 +++++-- spec/integration/assign_spec.rb | 15 ++++++++ spec/lib/pending_assigns_reminder_spec.rb | 5 +++ spec/lib/topic_assigner_spec.rb | 34 ++++++++++++++++--- spec/requests/assign_controller_spec.rb | 20 +++++++---- .../serializers/topic_list_serializer_spec.rb | 5 ++- spec/support/assign_allowed_group.rb | 11 ++++++ 10 files changed, 112 insertions(+), 17 deletions(-) create mode 100644 spec/support/assign_allowed_group.rb 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) + 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 diff --git a/spec/requests/assign_controller_spec.rb b/spec/requests/assign_controller_spec.rb index f743263..886eabf 100644 --- a/spec/requests/assign_controller_spec.rb +++ b/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 diff --git a/spec/serializers/topic_list_serializer_spec.rb b/spec/serializers/topic_list_serializer_spec.rb index b614acb..98a865d 100644 --- a/spec/serializers/topic_list_serializer_spec.rb +++ b/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 diff --git a/spec/support/assign_allowed_group.rb b/spec/support/assign_allowed_group.rb new file mode 100644 index 0000000..a3c97fa --- /dev/null +++ b/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