From eec464d8d1c058e60b25348612542282f2da4180 Mon Sep 17 00:00:00 2001 From: David Taylor Date: Thu, 24 Oct 2019 13:23:19 +0100 Subject: [PATCH] SECURITY: Check permissions when autocompleting mentions --- app/models/user_search.rb | 28 +++++++---- lib/guardian.rb | 9 ++++ spec/models/user_search_spec.rb | 88 ++++++++++++++++++++++++--------- 3 files changed, 92 insertions(+), 33 deletions(-) diff --git a/app/models/user_search.rb b/app/models/user_search.rb index ffb23e46f96..447f5d44925 100644 --- a/app/models/user_search.rb +++ b/app/models/user_search.rb @@ -16,7 +16,9 @@ class UserSearch @limit = opts[:limit] || 20 @groups = opts[:groups] @guardian = Guardian.new(@searching_user) - @guardian.ensure_can_see_groups!(@groups) if @groups + @guardian.ensure_can_see_groups_members! @groups if @groups + @guardian.ensure_can_see_category! Category.find(@category_id) if @category_id + @guardian.ensure_can_see_topic! Topic.find(@topic_id) if @topic_id end def scoped_users @@ -117,18 +119,26 @@ class UserSearch # 3. category matches if secure_category_id + @searching_user.present? + + category_groups = Group.where(<<~SQL, secure_category_id, MAX_SIZE_PRIORITY_MENTION) + groups.id IN ( + SELECT group_id FROM category_groups + JOIN groups g ON group_id = g.id + WHERE + category_id = ? AND + user_count < ? + ) + SQL + + category_groups = category_groups.members_visible_groups(@searching_user) + in_category = filtered_by_term_users - .where(<<~SQL, secure_category_id, MAX_SIZE_PRIORITY_MENTION) + .where(<<~SQL, category_groups.pluck(:id)) users.id IN ( SELECT gu.user_id FROM group_users gu - WHERE group_id IN ( - SELECT group_id FROM category_groups - JOIN groups g ON group_id = g.id - WHERE - category_id = ? AND - user_count < ? - ) + WHERE group_id IN (?) LIMIT 200 ) SQL diff --git a/lib/guardian.rb b/lib/guardian.rb index 41a93f5fee2..b89f2c6a825 100644 --- a/lib/guardian.rb +++ b/lib/guardian.rb @@ -249,6 +249,15 @@ class Guardian true end + def can_see_groups_members?(groups) + return false if groups.blank? + + requested_group_ids = groups.map(&:id) # Can't use pluck, groups could be a regular array + matching_groups = Group.where(id: requested_group_ids).members_visible_groups(user) + + matching_groups.pluck(:id).sort == requested_group_ids.sort + end + # Can we impersonate this user? def can_impersonate?(target) target && diff --git a/spec/models/user_search_spec.rb b/spec/models/user_search_spec.rb index 4953a4e29aa..5dad83c32be 100644 --- a/spec/models/user_search_spec.rb +++ b/spec/models/user_search_spec.rb @@ -26,36 +26,57 @@ describe UserSearch do UserSearch.new(*args).search end - it 'finds users in secure category' do - group = Fabricate(:group) - user = Fabricate(:user) - group.add(user) - group.save + context 'with a secure category' do + fab!(:group) { Fabricate :group } + fab!(:user) { Fabricate :user } + fab!(:searching_user) { Fabricate :user } + before_all do + group.add(user) + group.add(searching_user) + group.save + end + fab!(:category) { Fabricate(:category, + read_restricted: true, + user: user) + } + before_all { Fabricate(:category_group, category: category, group: group) } - category = - Fabricate( - :category, - read_restricted: true, - user: user + it 'autocompletes with people in the category' do + results = search_for("", searching_user: searching_user, category_id: category.id) + + expect(user.username).to eq(results[0].username) + expect(results.length).to eq(1) + end + + it 'will lookup the category from the topic id' do + topic = Fabricate(:topic, category: category) + _post = Fabricate(:post, user: topic.user, topic: topic) + + results = search_for("", searching_user: searching_user, topic_id: topic.id) + + expect(results.length).to eq(2) + + expect(results.map(&:username)).to contain_exactly( + user.username, topic.user.username ) + end - Fabricate(:category_group, category: category, group: group) + it 'will raise an error if the user cannot see the category' do + expect do + search_for("", searching_user: Fabricate(:user), category_id: category.id) + end.to raise_error(Discourse::InvalidAccess) + end - results = search_for("", category_id: category.id) + it 'will respect the group member visibility setting' do + group.update(members_visibility_level: Group.visibility_levels[:owners]) + results = search_for("", searching_user: searching_user, category_id: category.id) + expect(results.length).to eq(0) - expect(user.username).to eq(results[0].username) - expect(results.length).to eq(1) + group.add_owner(searching_user) + results = search_for("", searching_user: searching_user, category_id: category.id) + expect(results.length).to eq(1) + end - topic = Fabricate(:topic, category: category) - _post = Fabricate(:post, user: topic.user, topic: topic) - - results = search_for("", topic_id: topic.id) - - expect(results.length).to eq(2) - - expect(results.map(&:username)).to contain_exactly( - user.username, topic.user.username - ) end it 'allows for correct underscore searching' do @@ -156,6 +177,25 @@ describe UserSearch do expect(results[1]).to eq(user5) end + it "only reveals topic participants to people with permission" do + pm_topic = Fabricate(:private_message_post).topic + + # Anonymous, does not have access + expect do + search_for("", topic_id: pm_topic.id) + end.to raise_error(Discourse::InvalidAccess) + + # Random user, does not have access + expect do + search_for("", topic_id: pm_topic.id, searching_user: user1) + end.to raise_error(Discourse::InvalidAccess) + + pm_topic.invite(pm_topic.user, user1.username) + results = search_for("", topic_id: pm_topic.id, searching_user: user1) + expect(results.length).to eq(1) + expect(results[0]).to eq(pm_topic.user) + end + it "only searches by name when enabled" do # When searching by name is enabled, it returns the record SiteSetting.enable_names = true