FIX: Introduce Guardian::BasicUser for oneboxing checks (#24681)

Through internal discussion, it has become clear that
we need a conceptual Guardian user that bridges the
gap between anon users and a logged in forum user with
an absolute baseline level of access to public topics,
which can be used in cases where:

1. Automated systems are running which shouldn't see any
   private data
1. A baseline level of user access is needed

In this case we are fixing the latter; when oneboxing a local
topic, and we are linking to a topic in another category from
the current one, we need to operate off a baseline level of
access, since not all users have access to the same categories,
and we don't want e.g. editing a post with an internal link to
expose sensitive internal information.
This commit is contained in:
Martin Brennan 2023-12-05 09:25:23 +10:00 committed by GitHub
parent 7756c210da
commit de983796e1
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
4 changed files with 166 additions and 5 deletions

View File

@ -873,6 +873,10 @@ module Discourse
@system_users[current_db] ||= User.find_by(id: SYSTEM_USER_ID)
end
def self.basic_user
Guardian.basic_user
end
def self.store
if SiteSetting.Upload.enable_s3_uploads
@s3_store_loaded ||= require "file_store/s3_store"

View File

@ -78,6 +78,64 @@ class Guardian
end
end
# In some cases, we want a user that is not totally anonymous, but has
# the absolute baseline of access to the forum, acting like a TL0 user
# that is logged in. This represents someone who cannot access any secure
# categories or PMs but can read public topics.
class BasicUser
def blank?
false
end
def admin?
false
end
def staff?
false
end
def moderator?
false
end
def anonymous?
false
end
def approved?
true
end
def staged?
false
end
def silenced?
false
end
def is_system_user?
false
end
def bot?
false
end
def secure_category_ids
[]
end
def groups
@groups ||= Group.where(id: Group::AUTO_GROUPS[:trust_level_0])
end
def has_trust_level?(level)
level == TrustLevel[0]
end
def has_trust_level_or_staff?(level)
has_trust_level?(level)
end
def email
nil
end
def whisperer?
false
end
def in_any_groups?(group_ids)
group_ids.include?(Group::AUTO_GROUPS[:everyone]) || (group_ids & groups.map(&:id)).any?
end
end
attr_reader :request
def initialize(user = nil, request = nil)
@ -85,6 +143,14 @@ class Guardian
@request = request
end
def self.anon_user(request: nil)
new(AnonymousUser.new, request)
end
def self.basic_user(request: nil)
new(BasicUser.new, request)
end
def user
@user.presence
end

View File

@ -385,19 +385,22 @@ module Oneboxer
def self.local_topic(url, route, opts)
if current_user = User.find_by(id: opts[:user_id])
if current_category = Category.find_by(id: opts[:category_id])
return unless Guardian.new(current_user).can_see_category?(current_category)
return if !current_user.guardian.can_see_category?(current_category)
end
if current_topic = Topic.find_by(id: opts[:topic_id])
return unless Guardian.new(current_user).can_see_topic?(current_topic)
return if !current_user.guardian.can_see_topic?(current_topic)
end
end
return unless topic = Topic.find_by(id: route[:id] || route[:topic_id])
return if topic.private_message?
topic = Topic.find_by(id: route[:id] || route[:topic_id])
return if topic.blank? || topic.private_message?
# If we are oneboxing from one category to another, we need to use a basic guardian
# because some users can see more than others and we need the absolute baseline of
# access control here.
if current_category.blank? || current_category.id != topic.category_id
return unless Guardian.new.can_see_topic?(topic)
return if !Guardian.basic_user.can_see_topic?(topic)
end
topic

View File

@ -932,4 +932,92 @@ RSpec.describe Oneboxer do
expect(Oneboxer.preview(url)).to eq("Custom Onebox for Wizard")
end
end
describe ".local_topic" do
fab!(:topic)
fab!(:user)
let(:url) { topic.url }
let(:route) { Discourse.route_for(url) }
context "when user_id is not provided" do
let(:opts) { {} }
it "returns nil if the topic is a private message" do
topic.update!(archetype: Archetype.private_message, category: nil)
expect(Oneboxer.local_topic(url, route, opts)).to eq(nil)
end
it "returns nil if basic user cannot see the topic" do
topic.update!(category: Fabricate(:private_category, group: Fabricate(:group)))
expect(Oneboxer.local_topic(url, route, opts)).to eq(nil)
end
it "returns topic if basic user can see the topic" do
expect(Oneboxer.local_topic(url, route, opts)).to eq(topic)
end
end
context "when user_id is provided" do
context "when category_id is provided" do
fab!(:category)
let(:opts) { { category_id: category.id, user_id: user.id } }
before { topic.update!(category: category) }
it "returns nil if the user cannot see the category" do
category.update!(read_restricted: true)
expect(Oneboxer.local_topic(url, route, opts)).to eq(nil)
end
it "returns the topic if the user can see the category" do
expect(Oneboxer.local_topic(url, route, opts)).to eq(topic)
end
it "returns nil if basic user users cannot see the topic" do
topic.update!(category: Fabricate(:private_category, group: Fabricate(:group)))
expect(Oneboxer.local_topic(url, route, opts)).to eq(nil)
end
it "returns nil if the topic is a private message" do
topic.update!(archetype: Archetype.private_message, category: nil)
expect(Oneboxer.local_topic(url, route, opts)).to eq(nil)
end
context "when category_id is mismatched" do
fab!(:other_category) { Fabricate(:private_category, group: Fabricate(:group)) }
before { topic.update!(category: other_category) }
it "returns nil if the basic user cannot see the topic" do
expect(Oneboxer.local_topic(url, route, opts)).to eq(nil)
end
it "returns topic if the basic user can see the topic" do
other_category.update!(read_restricted: false)
expect(Oneboxer.local_topic(url, route, opts)).to eq(topic)
end
end
end
context "when topic_id is provided" do
let(:opts) { { topic_id: topic.id, user_id: user.id } }
it "returns nil if the user cannot see the topic" do
topic.update!(category: Fabricate(:private_category, group: Fabricate(:group)))
expect(Oneboxer.local_topic(url, route, opts)).to eq(nil)
end
it "returns the topic if the user can see the topic" do
expect(Oneboxer.local_topic(url, route, opts)).to eq(topic)
end
it "returns nil if the topic is a private message" do
topic.update!(archetype: Archetype.private_message, category: nil)
expect(Oneboxer.local_topic(url, route, opts)).to eq(nil)
end
end
end
end
end