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.
This commit is contained in:
Alan Guo Xiang Tan 2022-10-27 06:13:21 +08:00 committed by GitHub
parent d4583357cb
commit a473e352de
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
7 changed files with 222 additions and 18 deletions

View File

@ -1,6 +1,9 @@
# frozen_string_literal: true # frozen_string_literal: true
class ReviewableFlaggedPost < Reviewable class ReviewableFlaggedPost < Reviewable
scope :pending_and_default_visible, -> {
pending.default_visible
}
# Penalties are handled by the modal after the action is performed # Penalties are handled by the modal after the action is performed
def self.action_aliases def self.action_aliases

View File

@ -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 posts.where(post_type: Post.types[:regular], user_deleted: false).order('score desc nulls last').limit(1).first
end end
def self.has_flag_scope
ReviewableFlaggedPost.pending_and_default_visible
end
def has_flags? def has_flags?
ReviewableFlaggedPost.pending.default_visible.where(topic_id: id).exists? self.class.has_flag_scope.exists?(topic_id: self.id)
end end
def is_official_warning? def is_official_warning?

View File

@ -116,21 +116,18 @@ class Guardian
end end
def is_category_group_moderator?(category) def is_category_group_moderator?(category)
return false if !SiteSetting.enable_category_group_moderation?
return false if !category return false if !category
return false if !authenticated? return false if !category_group_moderation_allowed?
reviewable_by_group_id = category.reviewable_by_group_id reviewable_by_group_id = category.reviewable_by_group_id
return false if reviewable_by_group_id.blank? return false if reviewable_by_group_id.blank?
@is_group_member ||= {} @category_group_moderator_groups ||= {}
if @is_group_member.key?(reviewable_by_group_id) if @category_group_moderator_groups.key?(reviewable_by_group_id)
@is_group_member[reviewable_by_group_id] @category_group_moderator_groups[reviewable_by_group_id]
else else
@is_group_member[reviewable_by_group_id] = begin @category_group_moderator_groups[reviewable_by_group_id] = category_group_moderator_scope.exists?("categories.id": category.id)
GroupUser.where(group_id: reviewable_by_group_id, user_id: @user.id).exists?
end
end end
end end
@ -622,4 +619,16 @@ class Guardian
end end
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 end

View File

@ -2,7 +2,6 @@
#mixin for all guardian methods dealing with topic permissions #mixin for all guardian methods dealing with topic permissions
module TopicGuardian module TopicGuardian
def can_remove_allowed_users?(topic, target_user = nil) def can_remove_allowed_users?(topic, target_user = nil)
is_staff? || is_staff? ||
(topic.user == @user && @user.has_trust_level?(TrustLevel[2])) || (topic.user == @user && @user.has_trust_level?(TrustLevel[2])) ||
@ -199,6 +198,49 @@ module TopicGuardian
is_staff? || is_category_group_moderator?(category) is_staff? || is_category_group_moderator?(category)
end 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) def can_see_topic?(topic, hide_deleted = true)
return false unless topic return false unless topic
return true if is_admin? return true if is_admin?
@ -277,4 +319,45 @@ module TopicGuardian
def affected_by_slow_mode?(topic) def affected_by_slow_mode?(topic)
topic&.slow_mode_seconds.to_i > 0 && @user.human? && !is_staff? topic&.slow_mode_seconds.to_i > 0 && @user.human? && !is_staff?
end 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 end

View File

@ -1,10 +1,22 @@
# frozen_string_literal: true # frozen_string_literal: true
RSpec.describe TopicGuardian do RSpec.describe TopicGuardian do
fab!(:user) { Fabricate(:user) }
fab!(:admin) { Fabricate(:admin) } fab!(:admin) { Fabricate(:admin) }
fab!(:tl3_user) { Fabricate(:leader) } fab!(:tl3_user) { Fabricate(:leader) }
fab!(:moderator) { Fabricate(:moderator) } fab!(:moderator) { Fabricate(:moderator) }
fab!(:category) { Fabricate(:category) } 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 describe '#can_create_shared_draft?' do
it 'when shared_drafts are disabled' do it 'when shared_drafts are disabled' do
@ -88,7 +100,6 @@ RSpec.describe TopicGuardian do
end end
it 'returns true if a shared draft exists' do it 'returns true if a shared draft exists' do
topic = Fabricate(:topic, category: category)
Fabricate(:shared_draft, topic: topic) Fabricate(:shared_draft, topic: topic)
expect(Guardian.new(tl2_user).can_edit_topic?(topic)).to eq(true) 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 it 'returns false if the user has a lower trust level' do
tl1_user = Fabricate(:user, trust_level: TrustLevel[1]) tl1_user = Fabricate(:user, trust_level: TrustLevel[1])
topic = Fabricate(:topic, category: category)
Fabricate(:shared_draft, topic: topic) Fabricate(:shared_draft, topic: topic)
expect(Guardian.new(tl1_user).can_edit_topic?(topic)).to eq(false) 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) expect(Guardian.new(tl4_user).can_review_topic?(topic)).to eq(false)
end end
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 end

View File

@ -27,6 +27,11 @@ RSpec.describe Guardian do
before do before do
Group.refresh_automatic_groups! Group.refresh_automatic_groups!
Guardian.enable_topic_can_see_consistency_check
end
after do
Guardian.disable_topic_can_see_consistency_check
end end
it 'can be created without a user (not logged in)' do 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 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) 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 expect(Guardian.new(user_gm).can_see?(topic)).to be_truthy
end end
@ -1118,7 +1124,7 @@ RSpec.describe Guardian do
context "with trashed topic" do context "with trashed topic" do
before do before do
topic.deleted_at = Time.now topic.trash!(admin)
end end
it "doesn't allow new posts from regular users" do it "doesn't allow new posts from regular users" do
@ -1729,14 +1735,14 @@ RSpec.describe Guardian do
end end
context 'with private message' do context 'with private message' do
fab!(:private_message) { Fabricate(:private_message_topic) }
it 'returns false at trust level 3' do it 'returns false at trust level 3' do
topic.archetype = 'private_message' expect(Guardian.new(trust_level_3).can_edit?(private_message)).to eq(false)
expect(Guardian.new(trust_level_3).can_edit?(topic)).to eq(false)
end end
it 'returns false at trust level 4' do it 'returns false at trust level 4' do
topic.archetype = 'private_message' expect(Guardian.new(trust_level_4).can_edit?(private_message)).to eq(false)
expect(Guardian.new(trust_level_4).can_edit?(topic)).to eq(false)
end end
end end
@ -3965,7 +3971,7 @@ RSpec.describe Guardian do
it "should correctly detect category moderation" do it "should correctly detect category moderation" do
group.add(user) group.add(user)
category.reviewable_by_group_id = group.id category.update!(reviewable_by_group_id: group.id)
guardian = Guardian.new(user) guardian = Guardian.new(user)
# implementation detail, ensure memoization is good (hence testing twice) # implementation detail, ensure memoization is good (hence testing twice)

View File

@ -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