From 30d5e752d780de31e7c078248e11127423cce702 Mon Sep 17 00:00:00 2001 From: Martin Brennan Date: Wed, 6 Dec 2023 16:37:32 +1000 Subject: [PATCH] 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 9057272ee242b3bc977e50977b4142066c36c05d. * Revert "DEV: Remove unnecessary method_missing from GuardianUser (#24735)" This reverts commit a5d4bf6dd28a8304ccd519536f33bd4a13232ed4. * Revert "DEV: Improve Guardian devex (#24706)" This reverts commit 77b6a038bae010c9e9e630a137f9ccd570f60784. * Revert "FIX: Introduce Guardian::BasicUser for oneboxing checks (#24681)" This reverts commit de983796e1b66aa2ab039a4fb6e32cec8a65a098. --- app/controllers/about_controller.rb | 2 +- app/controllers/email_controller.rb | 2 +- lib/cooked_post_processor.rb | 2 +- lib/discourse.rb | 4 - lib/guardian.rb | 111 +------------------- lib/middleware/anonymous_cache.rb | 2 +- lib/oneboxer.rb | 15 ++- lib/pretty_text/helpers.rb | 2 +- plugins/chat/app/services/chat/publisher.rb | 10 +- plugins/chat/lib/chat/onebox_handler.rb | 2 +- plugins/chat/plugin.rb | 2 +- plugins/poll/lib/polls_updater.rb | 2 +- spec/lib/oneboxer_spec.rb | 88 ---------------- spec/lib/search_spec.rb | 2 +- 14 files changed, 25 insertions(+), 221 deletions(-) diff --git a/app/controllers/about_controller.rb b/app/controllers/about_controller.rb index c007370ee83..002f2e52b06 100644 --- a/app/controllers/about_controller.rb +++ b/app/controllers/about_controller.rb @@ -22,7 +22,7 @@ class AboutController < ApplicationController end category_topic_ids = Category.select(:topic_id).where.not(topic_id: nil) 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_post_count] = public_topics.sum(:posts_count) - stats[:public_topic_count] render json: stats diff --git a/app/controllers/email_controller.rb b/app/controllers/email_controller.rb index a7d272eeb52..ed317022724 100644 --- a/app/controllers/email_controller.rb +++ b/app/controllers/email_controller.rb @@ -47,6 +47,6 @@ class EmailController < ApplicationController user = User.find_by_email(@email) raise Discourse::NotFound unless user 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 diff --git a/lib/cooked_post_processor.rb b/lib/cooked_post_processor.rb index 26708091539..bc7fbcf2890 100644 --- a/lib/cooked_post_processor.rb +++ b/lib/cooked_post_processor.rb @@ -59,7 +59,7 @@ class CookedPostProcessor end 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? if @has_oneboxes diff --git a/lib/discourse.rb b/lib/discourse.rb index e62af531912..c6280786eb2 100644 --- a/lib/discourse.rb +++ b/lib/discourse.rb @@ -873,10 +873,6 @@ 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" diff --git a/lib/guardian.rb b/lib/guardian.rb index 59b76746cba..6af80ee6233 100644 --- a/lib/guardian.rb +++ b/lib/guardian.rb @@ -11,32 +11,6 @@ require "guardian/tag_guardian" require "guardian/topic_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 class Guardian include BookmarkGuardian @@ -54,12 +28,6 @@ class Guardian def blank? true end - def fake? - true - end - def authenticated? - false - end def admin? false end @@ -110,88 +78,15 @@ 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? - 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 def initialize(user = nil, request = nil) - @guardian_user = GuardianUser.new(user.presence || AnonymousUser.new) - @user = @guardian_user.actual + @user = user.presence || AnonymousUser.new @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 - @guardian_user.fake? ? nil : @user + @user.presence end alias current_user user @@ -200,7 +95,7 @@ class Guardian end def authenticated? - @guardian_user.authenticated? + @user.present? end def is_admin? diff --git a/lib/middleware/anonymous_cache.rb b/lib/middleware/anonymous_cache.rb index 489afd75db7..d41069c92e0 100644 --- a/lib/middleware/anonymous_cache.rb +++ b/lib/middleware/anonymous_cache.rb @@ -169,7 +169,7 @@ module Middleware def theme_ids ids, _ = @request.cookies["theme_ids"]&.split("|") 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) else [] diff --git a/lib/oneboxer.rb b/lib/oneboxer.rb index 0f3d9ade066..5349b8a1d05 100644 --- a/lib/oneboxer.rb +++ b/lib/oneboxer.rb @@ -385,22 +385,19 @@ 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 if !current_user.guardian.can_see_category?(current_category) + return unless Guardian.new(current_user).can_see_category?(current_category) end 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 - topic = Topic.find_by(id: route[:id] || route[:topic_id]) - return if topic.blank? || topic.private_message? + return unless topic = Topic.find_by(id: route[:id] || route[:topic_id]) + 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 - return if !Guardian.basic_user.can_see_topic?(topic) + return unless Guardian.new.can_see_topic?(topic) end topic @@ -483,7 +480,7 @@ module Oneboxer return unless 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 = { url: category.url, name: category.name, diff --git a/lib/pretty_text/helpers.rb b/lib/pretty_text/helpers.rb index 53c631f94aa..b07951ed79a 100644 --- a/lib/pretty_text/helpers.rb +++ b/lib/pretty_text/helpers.rb @@ -89,7 +89,7 @@ module PrettyText return unless topic_id.is_a?(Integer) # TODO this only handles public topics, secured one do not get this 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 } elsif topic { title: I18n.t("on_another_topic"), href: Discourse.base_url + topic.slugless_url } diff --git a/plugins/chat/app/services/chat/publisher.rb b/plugins/chat/app/services/chat/publisher.rb index 0bd307dd0aa..f960f679abe 100644 --- a/plugins/chat/app/services/chat/publisher.rb +++ b/plugins/chat/app/services/chat/publisher.rb @@ -52,7 +52,7 @@ module Chat message: Chat::MessageSerializer.new( chat_message, - { scope: Guardian.anon_user, root: false }, + { scope: anonymous_guardian, root: false }, ).as_json, }, permissions(chat_channel), @@ -69,7 +69,7 @@ module Chat message: Chat::MessageSerializer.new( chat_message, - { scope: Guardian.anon_user, root: false }, + { scope: anonymous_guardian, root: false }, ).as_json, }, permissions(chat_channel), @@ -258,7 +258,7 @@ module Chat def self.serialize_message_with_type(chat_message, type, options = {}) Chat::MessageSerializer - .new(chat_message, { scope: Guardian.anon_user, root: :chat_message }) + .new(chat_message, { scope: anonymous_guardian, root: :chat_message }) .as_json .merge(type: type) .merge(options) @@ -470,5 +470,9 @@ module Chat group_ids: channel.allowed_group_ids.presence, }.compact end + + def self.anonymous_guardian + Guardian.new(nil) + end end end diff --git a/plugins/chat/lib/chat/onebox_handler.rb b/plugins/chat/lib/chat/onebox_handler.rb index 1a6b6f9e8d2..3715dc9964d 100644 --- a/plugins/chat/lib/chat/onebox_handler.rb +++ b/plugins/chat/lib/chat/onebox_handler.rb @@ -19,7 +19,7 @@ module Chat thread = Chat::Thread.find_by(id: route[:thread_id]) if route[:thread_id] 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) diff --git a/plugins/chat/plugin.rb b/plugins/chat/plugin.rb index 4645ffe64d5..af5d8b3292b 100644 --- a/plugins/chat/plugin.rb +++ b/plugins/chat/plugin.rb @@ -105,7 +105,7 @@ after_initialize do 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 } end diff --git a/plugins/poll/lib/polls_updater.rb b/plugins/poll/lib/polls_updater.rb index a125329f635..22c74fd8104 100644 --- a/plugins/poll/lib/polls_updater.rb +++ b/plugins/poll/lib/polls_updater.rb @@ -114,7 +114,7 @@ module DiscoursePoll polls, each_serializer: PollSerializer, root: false, - scope: Guardian.basic_user, + scope: Guardian.new(nil), ).as_json post.publish_message!("/polls/#{post.topic_id}", post_id: post.id, polls: polls) end diff --git a/spec/lib/oneboxer_spec.rb b/spec/lib/oneboxer_spec.rb index e595e311ed4..8912b91adf2 100644 --- a/spec/lib/oneboxer_spec.rb +++ b/spec/lib/oneboxer_spec.rb @@ -932,92 +932,4 @@ 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 diff --git a/spec/lib/search_spec.rb b/spec/lib/search_spec.rb index c8922c16c82..4ebc44caebc 100644 --- a/spec/lib/search_spec.rb +++ b/spec/lib/search_spec.rb @@ -1988,7 +1988,7 @@ RSpec.describe Search do expect( Search - .execute("test created:@#{another_user.username}", guardian: Guardian.basic_user) + .execute("test created:@#{another_user.username}", guardian: Guardian.new()) .posts .length, ).to eq(1)