DEV: Revert guardian changes (#24742)

I took the wrong approach here, need to rethink.

* Revert "FIX: Use Guardian.basic_user instead of new (anon) (#24705)"

This reverts commit 9057272ee2.

* Revert "DEV: Remove unnecessary method_missing from GuardianUser (#24735)"

This reverts commit a5d4bf6dd2.

* Revert "DEV: Improve Guardian devex (#24706)"

This reverts commit 77b6a038ba.

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

This reverts commit de983796e1.
This commit is contained in:
Martin Brennan 2023-12-06 16:37:32 +10:00 committed by GitHub
parent ac60b9fe72
commit 30d5e752d7
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
14 changed files with 25 additions and 221 deletions

View File

@ -22,7 +22,7 @@ class AboutController < ApplicationController
end end
category_topic_ids = Category.select(:topic_id).where.not(topic_id: nil) category_topic_ids = Category.select(:topic_id).where.not(topic_id: nil)
public_topics = public_topics =
Topic.listable_topics.visible.secured(Guardian.anon_user).where.not(id: category_topic_ids) Topic.listable_topics.visible.secured(Guardian.new(nil)).where.not(id: category_topic_ids)
stats = { public_topic_count: public_topics.count } stats = { public_topic_count: public_topics.count }
stats[:public_post_count] = public_topics.sum(:posts_count) - stats[:public_topic_count] stats[:public_post_count] = public_topics.sum(:posts_count) - stats[:public_topic_count]
render json: stats render json: stats

View File

@ -47,6 +47,6 @@ class EmailController < ApplicationController
user = User.find_by_email(@email) user = User.find_by_email(@email)
raise Discourse::NotFound unless user raise Discourse::NotFound unless user
topic = Topic.find_by(id: params[:topic_id].to_i) if @topic_id topic = Topic.find_by(id: params[:topic_id].to_i) if @topic_id
@topic = topic if topic && Guardian.anon_user.can_see?(topic) @topic = topic if topic && Guardian.new(nil).can_see?(topic)
end end
end end

View File

@ -59,7 +59,7 @@ class CookedPostProcessor
end end
def grant_badges def grant_badges
return if @post.user.blank? || !Guardian.basic_user.can_see?(@post) return if @post.user.blank? || !Guardian.new.can_see?(@post)
BadgeGranter.grant(Badge.find(Badge::FirstEmoji), @post.user, post_id: @post.id) if has_emoji? BadgeGranter.grant(Badge.find(Badge::FirstEmoji), @post.user, post_id: @post.id) if has_emoji?
if @has_oneboxes if @has_oneboxes

View File

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

View File

@ -11,32 +11,6 @@ require "guardian/tag_guardian"
require "guardian/topic_guardian" require "guardian/topic_guardian"
require "guardian/user_guardian" require "guardian/user_guardian"
class GuardianUser
def initialize(user_alike)
@user_alike = user_alike
end
def actual
@user_alike
end
def fake?
if @user_alike.respond_to?(:fake?)
@user_alike.fake?
else
false
end
end
def authenticated?
if @user_alike.respond_to?(:authenticated?)
@user_alike.authenticated?
else
true
end
end
end
# The guardian is responsible for confirming access to various site resources and operations # The guardian is responsible for confirming access to various site resources and operations
class Guardian class Guardian
include BookmarkGuardian include BookmarkGuardian
@ -54,12 +28,6 @@ class Guardian
def blank? def blank?
true true
end end
def fake?
true
end
def authenticated?
false
end
def admin? def admin?
false false
end end
@ -110,88 +78,15 @@ class Guardian
end end
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?
true
end
def fake?
true
end
def authenticated?
true
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 attr_reader :request
def initialize(user = nil, request = nil) def initialize(user = nil, request = nil)
@guardian_user = GuardianUser.new(user.presence || AnonymousUser.new) @user = user.presence || AnonymousUser.new
@user = @guardian_user.actual
@request = request @request = request
end 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 def user
@guardian_user.fake? ? nil : @user @user.presence
end end
alias current_user user alias current_user user
@ -200,7 +95,7 @@ class Guardian
end end
def authenticated? def authenticated?
@guardian_user.authenticated? @user.present?
end end
def is_admin? def is_admin?

View File

@ -169,7 +169,7 @@ module Middleware
def theme_ids def theme_ids
ids, _ = @request.cookies["theme_ids"]&.split("|") ids, _ = @request.cookies["theme_ids"]&.split("|")
id = ids&.split(",")&.map(&:to_i)&.first id = ids&.split(",")&.map(&:to_i)&.first
if id && Guardian.anon_user.allow_themes?([id]) if id && Guardian.new.allow_themes?([id])
Theme.transform_ids(id) Theme.transform_ids(id)
else else
[] []

View File

@ -385,22 +385,19 @@ module Oneboxer
def self.local_topic(url, route, opts) def self.local_topic(url, route, opts)
if current_user = User.find_by(id: opts[:user_id]) if current_user = User.find_by(id: opts[:user_id])
if current_category = Category.find_by(id: opts[:category_id]) if current_category = Category.find_by(id: opts[:category_id])
return if !current_user.guardian.can_see_category?(current_category) return unless Guardian.new(current_user).can_see_category?(current_category)
end end
if current_topic = Topic.find_by(id: opts[:topic_id]) if current_topic = Topic.find_by(id: opts[:topic_id])
return if !current_user.guardian.can_see_topic?(current_topic) return unless Guardian.new(current_user).can_see_topic?(current_topic)
end end
end end
topic = Topic.find_by(id: route[:id] || route[:topic_id]) return unless topic = Topic.find_by(id: route[:id] || route[:topic_id])
return if topic.blank? || topic.private_message? return if 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 if current_category.blank? || current_category.id != topic.category_id
return if !Guardian.basic_user.can_see_topic?(topic) return unless Guardian.new.can_see_topic?(topic)
end end
topic topic
@ -483,7 +480,7 @@ module Oneboxer
return unless route[:category_slug_path_with_id] return unless route[:category_slug_path_with_id]
category = Category.find_by_slug_path_with_id(route[:category_slug_path_with_id]) category = Category.find_by_slug_path_with_id(route[:category_slug_path_with_id])
if Guardian.basic_user.can_see_category?(category) if Guardian.new.can_see_category?(category)
args = { args = {
url: category.url, url: category.url,
name: category.name, name: category.name,

View File

@ -89,7 +89,7 @@ module PrettyText
return unless topic_id.is_a?(Integer) return unless topic_id.is_a?(Integer)
# TODO this only handles public topics, secured one do not get this # TODO this only handles public topics, secured one do not get this
topic = Topic.find_by(id: topic_id) topic = Topic.find_by(id: topic_id)
if topic && Guardian.basic_user.can_see?(topic) if topic && Guardian.new.can_see?(topic)
{ title: Rack::Utils.escape_html(topic.title), href: topic.url } { title: Rack::Utils.escape_html(topic.title), href: topic.url }
elsif topic elsif topic
{ title: I18n.t("on_another_topic"), href: Discourse.base_url + topic.slugless_url } { title: I18n.t("on_another_topic"), href: Discourse.base_url + topic.slugless_url }

View File

@ -52,7 +52,7 @@ module Chat
message: message:
Chat::MessageSerializer.new( Chat::MessageSerializer.new(
chat_message, chat_message,
{ scope: Guardian.anon_user, root: false }, { scope: anonymous_guardian, root: false },
).as_json, ).as_json,
}, },
permissions(chat_channel), permissions(chat_channel),
@ -69,7 +69,7 @@ module Chat
message: message:
Chat::MessageSerializer.new( Chat::MessageSerializer.new(
chat_message, chat_message,
{ scope: Guardian.anon_user, root: false }, { scope: anonymous_guardian, root: false },
).as_json, ).as_json,
}, },
permissions(chat_channel), permissions(chat_channel),
@ -258,7 +258,7 @@ module Chat
def self.serialize_message_with_type(chat_message, type, options = {}) def self.serialize_message_with_type(chat_message, type, options = {})
Chat::MessageSerializer Chat::MessageSerializer
.new(chat_message, { scope: Guardian.anon_user, root: :chat_message }) .new(chat_message, { scope: anonymous_guardian, root: :chat_message })
.as_json .as_json
.merge(type: type) .merge(type: type)
.merge(options) .merge(options)
@ -470,5 +470,9 @@ module Chat
group_ids: channel.allowed_group_ids.presence, group_ids: channel.allowed_group_ids.presence,
}.compact }.compact
end end
def self.anonymous_guardian
Guardian.new(nil)
end
end end
end end

View File

@ -19,7 +19,7 @@ module Chat
thread = Chat::Thread.find_by(id: route[:thread_id]) if route[:thread_id] thread = Chat::Thread.find_by(id: route[:thread_id]) if route[:thread_id]
end end
return if !Guardian.basic_user.can_preview_chat_channel?(chat_channel) return if !Guardian.new.can_preview_chat_channel?(chat_channel)
args = build_args(url, chat_channel) args = build_args(url, chat_channel)

View File

@ -105,7 +105,7 @@ after_initialize do
end end
end end
next if !Guardian.basic_user.can_preview_chat_channel?(chat_channel) next if !Guardian.new.can_preview_chat_channel?(chat_channel)
{ url: url, title: title } { url: url, title: title }
end end

View File

@ -114,7 +114,7 @@ module DiscoursePoll
polls, polls,
each_serializer: PollSerializer, each_serializer: PollSerializer,
root: false, root: false,
scope: Guardian.basic_user, scope: Guardian.new(nil),
).as_json ).as_json
post.publish_message!("/polls/#{post.topic_id}", post_id: post.id, polls: polls) post.publish_message!("/polls/#{post.topic_id}", post_id: post.id, polls: polls)
end end

View File

@ -932,92 +932,4 @@ RSpec.describe Oneboxer do
expect(Oneboxer.preview(url)).to eq("Custom Onebox for Wizard") expect(Oneboxer.preview(url)).to eq("Custom Onebox for Wizard")
end end
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 end

View File

@ -1988,7 +1988,7 @@ RSpec.describe Search do
expect( expect(
Search Search
.execute("test created:@#{another_user.username}", guardian: Guardian.basic_user) .execute("test created:@#{another_user.username}", guardian: Guardian.new())
.posts .posts
.length, .length,
).to eq(1) ).to eq(1)