diff --git a/app/controllers/list_controller.rb b/app/controllers/list_controller.rb index e635a8815d9..7a259b40af0 100644 --- a/app/controllers/list_controller.rb +++ b/app/controllers/list_controller.rb @@ -143,22 +143,37 @@ class ListController < ApplicationController end def self.generate_message_route(action) - define_method("#{action}") do - if action == :private_messages_tag && !guardian.can_tag_pms? - raise Discourse::NotFound + case action + when :private_messages_tag + define_method("#{action}") do + raise Discourse::NotFound if !guardian.can_tag_pms? + message_route(action) end + when :private_messages_group, :private_messages_group_archive + define_method("#{action}") do + group = Group.find_by(name: params[:group_name]) + raise Discourse::NotFound unless guardian.can_see_group_messages?(group) - list_opts = build_topic_list_options - target_user = fetch_user_from_params({ include_inactive: current_user.try(:staff?) }, [:user_stat, :user_option]) - guardian.ensure_can_see_private_messages!(target_user.id) - list = generate_list_for(action.to_s, target_user, list_opts) - url_prefix = "topics" - list.more_topics_url = construct_url_with(:next, list_opts, url_prefix) - list.prev_topics_url = construct_url_with(:prev, list_opts, url_prefix) - respond_with_list(list) + message_route(action) + end + else + define_method("#{action}") do + message_route(action) + end end end + def message_route(action) + target_user = fetch_user_from_params({ include_inactive: current_user.try(:staff?) }, [:user_stat, :user_option]) + guardian.ensure_can_see_private_messages!(target_user.id) + list_opts = build_topic_list_options + list = generate_list_for(action.to_s, target_user, list_opts) + url_prefix = "topics" + list.more_topics_url = construct_url_with(:next, list_opts, url_prefix) + list.prev_topics_url = construct_url_with(:prev, list_opts, url_prefix) + respond_with_list(list) + end + %i{ private_messages private_messages_sent diff --git a/lib/topic_query.rb b/lib/topic_query.rb index 5f8658661b4..8b183843c60 100644 --- a/lib/topic_query.rb +++ b/lib/topic_query.rb @@ -532,22 +532,15 @@ class TopicQuery result = Topic.includes(:tags) if type == :group - result = result.includes(:allowed_users) - result = result.where(" - topics.id IN ( - SELECT topic_id FROM topic_allowed_groups - WHERE ( - group_id IN ( - SELECT group_id - FROM group_users - WHERE user_id = #{user.id.to_i} - OR #{user.staff?} - ) - ) - AND group_id IN (SELECT id FROM groups WHERE name ilike ?) - )", - @options[:group_name] - ) + result = result + .includes(:allowed_users) + .joins("INNER JOIN topic_allowed_groups tag ON tag.topic_id = topics.id AND tag.group_id IN (SELECT id FROM groups WHERE name ilike '#{sanitize_sql_array([@options[:group_name]])}')") + + unless user.admin? + result = result.joins("INNER JOIN group_users gu ON gu.group_id = tag.group_id AND gu.user_id = #{user.id.to_i}") + end + + result elsif type == :user result = result.includes(:allowed_users) result = result.where("topics.id IN (SELECT topic_id FROM topic_allowed_users WHERE user_id = #{user.id.to_i})") diff --git a/spec/components/topic_query_spec.rb b/spec/components/topic_query_spec.rb index d6e56a069de..9ad251b93aa 100644 --- a/spec/components/topic_query_spec.rb +++ b/spec/components/topic_query_spec.rb @@ -1097,7 +1097,15 @@ describe TopicQuery do expect(topics).to contain_exactly(group_message) end - it 'should return the right list for a user not part of the group' do + it "should not allow a moderator not part of the group to view the group's messages" do + topics = TopicQuery.new(nil, group_name: group.name) + .list_private_messages_group(Fabricate(:moderator)) + .topics + + expect(topics).to eq([]) + end + + it "should not allow a user not part of the group to view the group's messages" do topics = TopicQuery.new(nil, group_name: group.name) .list_private_messages_group(Fabricate(:user)) .topics diff --git a/spec/requests/list_controller_spec.rb b/spec/requests/list_controller_spec.rb index 2fea625c058..452b5b15435 100644 --- a/spec/requests/list_controller_spec.rb +++ b/spec/requests/list_controller_spec.rb @@ -183,8 +183,17 @@ RSpec.describe ListController do describe 'with unicode_usernames' do before { SiteSetting.unicode_usernames = false } + it 'should return the right response when user does not belong to group' do + Fabricate(:private_message_topic, allowed_groups: [group]) + + group.remove(user) + + get "/topics/private-messages-group/#{user.username}/#{group.name}.json" + + expect(response.status).to eq(404) + end + it 'should return the right response' do - group.add(user) topic = Fabricate(:private_message_topic, allowed_groups: [group]) get "/topics/private-messages-group/#{user.username}/#{group.name}.json"