From a473e352de0b75d9c53f30b09eed05855934512d Mon Sep 17 00:00:00 2001 From: Alan Guo Xiang Tan Date: Thu, 27 Oct 2022 06:13:21 +0800 Subject: [PATCH] DEV: Introduce TopicGuardian#can_see_topic_ids method (#18692) Before this commit, there was no way for us to efficiently check an array of topics for which a user can see. Therefore, this commit introduces the `TopicGuardian#can_see_topic_ids` method which accepts an array of `Topic#id`s and filters out the ids which the user is not allowed to see. The `TopicGuardian#can_see_topic_ids` method is meant to maintain feature parity with `TopicGuardian#can_see_topic?` at all times so a consistency check has been added in our tests to ensure that `TopicGuardian#can_see_topic_ids` returns the same result as `TopicGuardian#can_see_topic?`. In the near future, the plan is for us to switch to `TopicGuardian#can_see_topic_ids` completely but I'm not doing that in this commit as we have to be careful with the performance impact of such a change. This method is currently not being used in the current commit but will be relied on in a subsequent commit. --- app/models/reviewable_flagged_post.rb | 3 + app/models/topic.rb | 6 +- lib/guardian.rb | 25 ++++-- lib/guardian/topic_guardian.rb | 85 ++++++++++++++++++- spec/lib/guardian/topic_guardian_spec.rb | 59 ++++++++++++- spec/lib/guardian_spec.rb | 18 ++-- ...opic_guardian_can_see_consistency_check.rb | 44 ++++++++++ 7 files changed, 222 insertions(+), 18 deletions(-) create mode 100644 spec/support/topic_guardian_can_see_consistency_check.rb diff --git a/app/models/reviewable_flagged_post.rb b/app/models/reviewable_flagged_post.rb index d79f1daf1ce..032f6bed900 100644 --- a/app/models/reviewable_flagged_post.rb +++ b/app/models/reviewable_flagged_post.rb @@ -1,6 +1,9 @@ # frozen_string_literal: true class ReviewableFlaggedPost < Reviewable + scope :pending_and_default_visible, -> { + pending.default_visible + } # Penalties are handled by the modal after the action is performed def self.action_aliases diff --git a/app/models/topic.rb b/app/models/topic.rb index 88578cdce4d..2db3023458b 100644 --- a/app/models/topic.rb +++ b/app/models/topic.rb @@ -423,8 +423,12 @@ class Topic < ActiveRecord::Base posts.where(post_type: Post.types[:regular], user_deleted: false).order('score desc nulls last').limit(1).first end + def self.has_flag_scope + ReviewableFlaggedPost.pending_and_default_visible + end + def has_flags? - ReviewableFlaggedPost.pending.default_visible.where(topic_id: id).exists? + self.class.has_flag_scope.exists?(topic_id: self.id) end def is_official_warning? diff --git a/lib/guardian.rb b/lib/guardian.rb index 69415b54709..0dbdd1dec3c 100644 --- a/lib/guardian.rb +++ b/lib/guardian.rb @@ -116,21 +116,18 @@ class Guardian end def is_category_group_moderator?(category) - return false if !SiteSetting.enable_category_group_moderation? return false if !category - return false if !authenticated? + return false if !category_group_moderation_allowed? reviewable_by_group_id = category.reviewable_by_group_id return false if reviewable_by_group_id.blank? - @is_group_member ||= {} + @category_group_moderator_groups ||= {} - if @is_group_member.key?(reviewable_by_group_id) - @is_group_member[reviewable_by_group_id] + if @category_group_moderator_groups.key?(reviewable_by_group_id) + @category_group_moderator_groups[reviewable_by_group_id] else - @is_group_member[reviewable_by_group_id] = begin - GroupUser.where(group_id: reviewable_by_group_id, user_id: @user.id).exists? - end + @category_group_moderator_groups[reviewable_by_group_id] = category_group_moderator_scope.exists?("categories.id": category.id) end end @@ -622,4 +619,16 @@ class Guardian end end + protected + + def category_group_moderation_allowed? + authenticated? && SiteSetting.enable_category_group_moderation + end + + def category_group_moderator_scope + Category + .joins("INNER JOIN group_users ON group_users.group_id = categories.reviewable_by_group_id") + .where("group_users.user_id = ?", user.id) + end + end diff --git a/lib/guardian/topic_guardian.rb b/lib/guardian/topic_guardian.rb index 874ed0fb1d0..04feb20b242 100644 --- a/lib/guardian/topic_guardian.rb +++ b/lib/guardian/topic_guardian.rb @@ -2,7 +2,6 @@ #mixin for all guardian methods dealing with topic permissions module TopicGuardian - def can_remove_allowed_users?(topic, target_user = nil) is_staff? || (topic.user == @user && @user.has_trust_level?(TrustLevel[2])) || @@ -199,6 +198,49 @@ module TopicGuardian is_staff? || is_category_group_moderator?(category) end + # Accepts an array of `Topic#id` and returns an array of `Topic#id` which the user can see. + def can_see_topic_ids(topic_ids: [], hide_deleted: true) + topic_ids = topic_ids.compact + + return topic_ids if is_admin? + return [] if topic_ids.blank? + + default_scope = Topic.unscoped.where(id: topic_ids) + + # When `hide_deleted` is `true`, hide deleted topics if user is not staff or category moderator + if hide_deleted && !is_staff? + if category_group_moderation_allowed? + default_scope = default_scope.where(<<~SQL) + ( + deleted_at IS NULL OR + ( + deleted_at IS NOT NULL + AND topics.category_id IN (#{category_group_moderator_scope.select(:id).to_sql}) + ) + ) + SQL + else + default_scope = default_scope.where("deleted_at IS NULL") + end + end + + # Filter out topics with shared drafts if user cannot see shared drafts + if !can_see_shared_draft? + default_scope = default_scope.left_outer_joins(:shared_draft).where("shared_drafts.id IS NULL") + end + + all_topics_scope = + if authenticated? + Topic.unscoped.merge( + secured_regular_topic_scope(default_scope, topic_ids: topic_ids).or(private_message_topic_scope(default_scope)) + ) + else + Topic.unscoped.merge(secured_regular_topic_scope(default_scope, topic_ids: topic_ids)) + end + + all_topics_scope.pluck(:id) + end + def can_see_topic?(topic, hide_deleted = true) return false unless topic return true if is_admin? @@ -277,4 +319,45 @@ module TopicGuardian def affected_by_slow_mode?(topic) topic&.slow_mode_seconds.to_i > 0 && @user.human? && !is_staff? end + + private + + def private_message_topic_scope(scope) + pm_scope = scope.private_messages_for_user(user) + + if is_moderator? + pm_scope = pm_scope.or(scope.where(<<~SQL)) + topics.subtype = '#{TopicSubtype.moderator_warning}' + OR topics.id IN (#{Topic.has_flag_scope.select(:topic_id).to_sql}) + SQL + end + + pm_scope + end + + def secured_regular_topic_scope(scope, topic_ids:) + secured_scope = Topic.unscoped.secured(self) + + # Staged users are allowed to see their own topics in read restricted categories when Category#email_in and + # Category#email_in_allow_strangers has been configured. + if is_staged? + sql = <<~SQL + topics.id IN ( + SELECT + topics.id + FROM topics + INNER JOIN categories ON categories.id = topics.category_id + WHERE categories.read_restricted + AND categories.email_in IS NOT NULL + AND categories.email_in_allow_strangers + AND topics.user_id = :user_id + AND topics.id IN (:topic_ids) + ) + SQL + + secured_scope = secured_scope.or(Topic.unscoped.where(sql, user_id: user.id, topic_ids: topic_ids)) + end + + scope.listable_topics.merge(secured_scope) + end end diff --git a/spec/lib/guardian/topic_guardian_spec.rb b/spec/lib/guardian/topic_guardian_spec.rb index 24ef2e03322..c6da52b9ee4 100644 --- a/spec/lib/guardian/topic_guardian_spec.rb +++ b/spec/lib/guardian/topic_guardian_spec.rb @@ -1,10 +1,22 @@ # frozen_string_literal: true RSpec.describe TopicGuardian do + fab!(:user) { Fabricate(:user) } fab!(:admin) { Fabricate(:admin) } fab!(:tl3_user) { Fabricate(:leader) } fab!(:moderator) { Fabricate(:moderator) } fab!(:category) { Fabricate(:category) } + fab!(:topic) { Fabricate(:topic, category: category) } + fab!(:private_message_topic) { Fabricate(:private_message_topic) } + fab!(:group) { Fabricate(:group) } + + before do + Guardian.enable_topic_can_see_consistency_check + end + + after do + Guardian.disable_topic_can_see_consistency_check + end describe '#can_create_shared_draft?' do it 'when shared_drafts are disabled' do @@ -88,7 +100,6 @@ RSpec.describe TopicGuardian do end it 'returns true if a shared draft exists' do - topic = Fabricate(:topic, category: category) Fabricate(:shared_draft, topic: topic) expect(Guardian.new(tl2_user).can_edit_topic?(topic)).to eq(true) @@ -96,7 +107,6 @@ RSpec.describe TopicGuardian do it 'returns false if the user has a lower trust level' do tl1_user = Fabricate(:user, trust_level: TrustLevel[1]) - topic = Fabricate(:topic, category: category) Fabricate(:shared_draft, topic: topic) expect(Guardian.new(tl1_user).can_edit_topic?(topic)).to eq(false) @@ -119,4 +129,49 @@ RSpec.describe TopicGuardian do expect(Guardian.new(tl4_user).can_review_topic?(topic)).to eq(false) end end + + # The test cases here are intentionally kept brief because majority of the cases are already handled by + # `TopicGuardianCanSeeConsistencyCheck` which we run to ensure that the implementation between `TopicGuardian#can_see_topic_ids` + # and `TopicGuardian#can_see_topic?` is consistent. + describe '#can_see_topic_ids' do + it 'returns the topic ids for the topics which a user is allowed to see' do + expect(Guardian.new.can_see_topic_ids(topic_ids: [topic.id, private_message_topic.id])).to contain_exactly( + topic.id + ) + + expect(Guardian.new(user).can_see_topic_ids(topic_ids: [topic.id, private_message_topic.id])).to contain_exactly( + topic.id + ) + + expect(Guardian.new(moderator).can_see_topic_ids(topic_ids: [topic.id, private_message_topic.id])).to contain_exactly( + topic.id, + ) + + expect(Guardian.new(admin).can_see_topic_ids(topic_ids: [topic.id, private_message_topic.id])).to contain_exactly( + topic.id, + private_message_topic.id + ) + end + + it 'returns the topic ids for topics which are deleted but user is a category moderator of' do + SiteSetting.enable_category_group_moderation = true + + category.update!(reviewable_by_group_id: group.id) + group.add(user) + topic.update!(category: category) + topic.trash!(admin) + + topic2 = Fabricate(:topic) + user2 = Fabricate(:user) + + expect(Guardian.new(user).can_see_topic_ids(topic_ids: [topic.id, topic2.id])).to contain_exactly( + topic.id, + topic2.id + ) + + expect(Guardian.new(user2).can_see_topic_ids(topic_ids: [topic.id, topic2.id])).to contain_exactly( + topic2.id, + ) + end + end end diff --git a/spec/lib/guardian_spec.rb b/spec/lib/guardian_spec.rb index f9f3bfe2348..feed9f9dac2 100644 --- a/spec/lib/guardian_spec.rb +++ b/spec/lib/guardian_spec.rb @@ -27,6 +27,11 @@ RSpec.describe Guardian do before do Group.refresh_automatic_groups! + Guardian.enable_topic_can_see_consistency_check + end + + after do + Guardian.disable_topic_can_see_consistency_check end it 'can be created without a user (not logged in)' do @@ -853,6 +858,7 @@ RSpec.describe Guardian do expect(Guardian.new(user_gm).can_see?(topic)).to be_falsey topic.category.update!(reviewable_by_group_id: group.id, topic_id: post.topic.id) + expect(Guardian.new(user_gm).can_see?(topic)).to be_truthy end @@ -1118,7 +1124,7 @@ RSpec.describe Guardian do context "with trashed topic" do before do - topic.deleted_at = Time.now + topic.trash!(admin) end it "doesn't allow new posts from regular users" do @@ -1729,14 +1735,14 @@ RSpec.describe Guardian do end context 'with private message' do + fab!(:private_message) { Fabricate(:private_message_topic) } + it 'returns false at trust level 3' do - topic.archetype = 'private_message' - expect(Guardian.new(trust_level_3).can_edit?(topic)).to eq(false) + expect(Guardian.new(trust_level_3).can_edit?(private_message)).to eq(false) end it 'returns false at trust level 4' do - topic.archetype = 'private_message' - expect(Guardian.new(trust_level_4).can_edit?(topic)).to eq(false) + expect(Guardian.new(trust_level_4).can_edit?(private_message)).to eq(false) end end @@ -3965,7 +3971,7 @@ RSpec.describe Guardian do it "should correctly detect category moderation" do group.add(user) - category.reviewable_by_group_id = group.id + category.update!(reviewable_by_group_id: group.id) guardian = Guardian.new(user) # implementation detail, ensure memoization is good (hence testing twice) diff --git a/spec/support/topic_guardian_can_see_consistency_check.rb b/spec/support/topic_guardian_can_see_consistency_check.rb new file mode 100644 index 00000000000..f50b3dbb721 --- /dev/null +++ b/spec/support/topic_guardian_can_see_consistency_check.rb @@ -0,0 +1,44 @@ +# frozen_string_literal: true + +if !Guardian.new.respond_to?(:can_see_topic?) + raise "Guardian no longer implements a `can_see_topic?` method making this consistency check invalid" +end + +# Monkey patches `TopicGuardian#can_see_topic?` to ensure that `TopicGuardian#can_see_topic_ids` returns the same +# result for the same inputs. We're using this check to bridge the transition to `TopicGuardian#can_see_topic_ids` as the +# backing implementation for `TopicGuardian#can_see_topic?` in the near future. +module TopicGuardianCanSeeConsistencyCheck + extend ActiveSupport::Concern + + module ClassMethods + def enable_topic_can_see_consistency_check + @enable_can_see_consistency_check = true + end + + def disable_topic_can_see_consistency_check + @enable_can_see_consistency_check = false + end + + def run_topic_can_see_consistency_check? + @enable_can_see_consistency_check + end + end + + def can_see_topic?(topic, hide_deleted = true) + result = super + + if self.class.run_topic_can_see_consistency_check? + new_result = self.can_see_topic_ids(topic_ids: [topic&.id], hide_deleted: hide_deleted).present? + + if result != new_result + raise "result between TopicGuardian#can_see_topic? (#{result}) and TopicGuardian#can_see_topic_ids (#{new_result}) has drifted and returned different results for the same input" + end + end + + result + end +end + +class Guardian + prepend TopicGuardianCanSeeConsistencyCheck +end