From 3ee4b59c64104ee27bba64fd84b372ebf4184970 Mon Sep 17 00:00:00 2001 From: Martin Brennan Date: Tue, 13 Dec 2022 09:14:17 +1000 Subject: [PATCH] DEV: Use guardian user for can_chat? (#19418) Instead of passing `user` to `guardian.can_chat?`, we can just use the inner `@user` that is part of the guardian instance already to determine whether that user can chat, since this is how it works for all other usages of guardian even within chat. --- plugins/chat/app/controllers/api_controller.rb | 2 +- plugins/chat/app/controllers/chat_base_controller.rb | 2 +- plugins/chat/app/controllers/chat_controller.rb | 4 ++-- .../app/controllers/direct_messages_controller.rb | 4 ++-- .../chat/app/jobs/regular/chat_notify_watching.rb | 2 +- plugins/chat/lib/chat_channel_hashtag_data_source.rb | 8 ++++---- plugins/chat/lib/chat_notifier.rb | 2 +- .../lib/extensions/user_notifications_extension.rb | 2 +- plugins/chat/lib/guardian_extensions.rb | 7 +++---- plugins/chat/plugin.rb | 4 ++-- plugins/chat/spec/lib/guardian_extensions_spec.rb | 12 ++++++------ 11 files changed, 24 insertions(+), 25 deletions(-) diff --git a/plugins/chat/app/controllers/api_controller.rb b/plugins/chat/app/controllers/api_controller.rb index eaf3db9be5c..fa27b825d83 100644 --- a/plugins/chat/app/controllers/api_controller.rb +++ b/plugins/chat/app/controllers/api_controller.rb @@ -8,6 +8,6 @@ class Chat::Api < Chat::ChatBaseController def ensure_can_chat raise Discourse::NotFound unless SiteSetting.chat_enabled - guardian.ensure_can_chat!(current_user) + guardian.ensure_can_chat! end end diff --git a/plugins/chat/app/controllers/chat_base_controller.rb b/plugins/chat/app/controllers/chat_base_controller.rb index 14cc69f2710..6e014502ddb 100644 --- a/plugins/chat/app/controllers/chat_base_controller.rb +++ b/plugins/chat/app/controllers/chat_base_controller.rb @@ -8,7 +8,7 @@ class Chat::ChatBaseController < ::ApplicationController def ensure_can_chat raise Discourse::NotFound unless SiteSetting.chat_enabled - guardian.ensure_can_chat!(current_user) + guardian.ensure_can_chat! end def set_channel_and_chatable_with_access_check(chat_channel_id: nil) diff --git a/plugins/chat/app/controllers/chat_controller.rb b/plugins/chat/app/controllers/chat_controller.rb index 57fc55e6da6..9dd460ca73b 100644 --- a/plugins/chat/app/controllers/chat_controller.rb +++ b/plugins/chat/app/controllers/chat_controller.rb @@ -346,7 +346,7 @@ class Chat::ChatController < Chat::ChatBaseController .where(id: params[:user_ids]) users.each do |user| guardian = Guardian.new(user) - if guardian.can_chat?(user) && guardian.can_see_chat_channel?(@chat_channel) + if guardian.can_chat? && guardian.can_see_chat_channel?(@chat_channel) data = { message: "chat.invitation_notification", chat_channel_id: @chat_channel.id, @@ -368,7 +368,7 @@ class Chat::ChatController < Chat::ChatBaseController def dismiss_retention_reminder params.require(:chatable_type) - guardian.ensure_can_chat!(current_user) + guardian.ensure_can_chat! unless ChatChannel.chatable_types.include?(params[:chatable_type]) raise Discourse::InvalidParameters end diff --git a/plugins/chat/app/controllers/direct_messages_controller.rb b/plugins/chat/app/controllers/direct_messages_controller.rb index 8464b705e04..fb07f66874b 100644 --- a/plugins/chat/app/controllers/direct_messages_controller.rb +++ b/plugins/chat/app/controllers/direct_messages_controller.rb @@ -4,7 +4,7 @@ class Chat::DirectMessagesController < Chat::ChatBaseController # NOTE: For V1 of chat channel archiving and deleting we are not doing # anything for DM channels, their behaviour will stay as is. def create - guardian.ensure_can_chat!(current_user) + guardian.ensure_can_chat! users = users_from_usernames(current_user, params) begin @@ -22,7 +22,7 @@ class Chat::DirectMessagesController < Chat::ChatBaseController end def index - guardian.ensure_can_chat!(current_user) + guardian.ensure_can_chat! users = users_from_usernames(current_user, params) direct_message = DirectMessage.for_user_ids(users.map(&:id).uniq) diff --git a/plugins/chat/app/jobs/regular/chat_notify_watching.rb b/plugins/chat/app/jobs/regular/chat_notify_watching.rb index e9b8805e88d..9debb13fc99 100644 --- a/plugins/chat/app/jobs/regular/chat_notify_watching.rb +++ b/plugins/chat/app/jobs/regular/chat_notify_watching.rb @@ -43,7 +43,7 @@ module Jobs def send_notifications(membership) user = membership.user guardian = Guardian.new(user) - return unless guardian.can_chat?(user) && guardian.can_see_chat_channel?(@chat_channel) + return unless guardian.can_chat? && guardian.can_see_chat_channel?(@chat_channel) return if Chat::ChatNotifier.user_has_seen_message?(membership, @chat_message.id) return if online_user_ids.include?(user.id) diff --git a/plugins/chat/lib/chat_channel_hashtag_data_source.rb b/plugins/chat/lib/chat_channel_hashtag_data_source.rb index 9540770c366..7fed9fc8b68 100644 --- a/plugins/chat/lib/chat_channel_hashtag_data_source.rb +++ b/plugins/chat/lib/chat_channel_hashtag_data_source.rb @@ -7,7 +7,7 @@ class Chat::ChatChannelHashtagDataSource def self.channel_to_hashtag_item(guardian, channel) HashtagAutocompleteService::HashtagItem.new.tap do |item| - item.text = channel.title(guardian.user) + item.text = channel.title item.description = channel.description item.slug = channel.slug item.icon = icon @@ -18,7 +18,7 @@ class Chat::ChatChannelHashtagDataSource def self.lookup(guardian, slugs) if SiteSetting.enable_experimental_hashtag_autocomplete - return [] if !guardian.can_chat?(guardian.user) + return [] if !guardian.can_chat? Chat::ChatChannelFetcher .secured_public_channel_slug_lookup(guardian, slugs) .map { |channel| channel_to_hashtag_item(guardian, channel) } @@ -29,7 +29,7 @@ class Chat::ChatChannelHashtagDataSource def self.search(guardian, term, limit) if SiteSetting.enable_experimental_hashtag_autocomplete - return [] if !guardian.can_chat?(guardian.user) + return [] if !guardian.can_chat? Chat::ChatChannelFetcher .secured_public_channel_search( guardian, @@ -49,7 +49,7 @@ class Chat::ChatChannelHashtagDataSource def self.search_without_term(guardian, limit) if SiteSetting.enable_experimental_hashtag_autocomplete - return [] if !guardian.can_chat?(guardian.user) + return [] if !guardian.can_chat? allowed_channel_ids_sql = Chat::ChatChannelFetcher.generate_allowed_channel_ids_sql( guardian, diff --git a/plugins/chat/lib/chat_notifier.rb b/plugins/chat/lib/chat_notifier.rb index 4aada3a946d..47501688336 100644 --- a/plugins/chat/lib/chat_notifier.rb +++ b/plugins/chat/lib/chat_notifier.rb @@ -195,7 +195,7 @@ class Chat::ChatNotifier potential_participants, unreachable = users.partition do |user| guardian = Guardian.new(user) - guardian.can_chat?(user) && guardian.can_see_chat_channel?(@chat_channel) + guardian.can_chat? && guardian.can_see_chat_channel?(@chat_channel) end participants, welcome_to_join = diff --git a/plugins/chat/lib/extensions/user_notifications_extension.rb b/plugins/chat/lib/extensions/user_notifications_extension.rb index 55fd73470f0..23c77295a46 100644 --- a/plugins/chat/lib/extensions/user_notifications_extension.rb +++ b/plugins/chat/lib/extensions/user_notifications_extension.rb @@ -3,7 +3,7 @@ module Chat::UserNotificationsExtension def chat_summary(user, opts) guardian = Guardian.new(user) - return unless guardian.can_chat?(user) + return unless guardian.can_chat? @messages = ChatMessage diff --git a/plugins/chat/lib/guardian_extensions.rb b/plugins/chat/lib/guardian_extensions.rb index c69b1d66328..04bee8088c6 100644 --- a/plugins/chat/lib/guardian_extensions.rb +++ b/plugins/chat/lib/guardian_extensions.rb @@ -10,10 +10,9 @@ module Chat::GuardianExtensions end end - def can_chat?(user) - return false unless user - - user.staff? || user.in_any_groups?(Chat.allowed_group_ids) + def can_chat? + return false if anonymous? + @user.staff? || @user.in_any_groups?(Chat.allowed_group_ids) end def can_create_chat_message? diff --git a/plugins/chat/plugin.rb b/plugins/chat/plugin.rb index fc5d33fdc51..4f6388b69db 100644 --- a/plugins/chat/plugin.rb +++ b/plugins/chat/plugin.rb @@ -386,7 +386,7 @@ after_initialize do return false if !SiteSetting.chat_enabled return false if scope.user.blank? - scope.user.id != object.id && scope.can_chat?(scope.user) && scope.can_chat?(object) + scope.user.id != object.id && scope.can_chat? && Guardian.new(object).can_chat? end add_to_serializer(:current_user, :can_chat) { true } @@ -394,7 +394,7 @@ after_initialize do add_to_serializer(:current_user, :include_can_chat?) do return @can_chat if defined?(@can_chat) - @can_chat = SiteSetting.chat_enabled && scope.can_chat?(object) + @can_chat = SiteSetting.chat_enabled && scope.can_chat? end add_to_serializer(:current_user, :has_chat_enabled) { true } diff --git a/plugins/chat/spec/lib/guardian_extensions_spec.rb b/plugins/chat/spec/lib/guardian_extensions_spec.rb index 73767fc2151..92bdeaa946f 100644 --- a/plugins/chat/spec/lib/guardian_extensions_spec.rb +++ b/plugins/chat/spec/lib/guardian_extensions_spec.rb @@ -13,28 +13,28 @@ RSpec.describe Chat::GuardianExtensions do it "cannot chat if the user is not in the Chat.allowed_group_ids" do SiteSetting.chat_allowed_groups = "" - expect(guardian.can_chat?(user)).to eq(false) + expect(guardian.can_chat?).to eq(false) end it "staff can always chat regardless of chat_allowed_grups" do SiteSetting.chat_allowed_groups = "" - expect(guardian.can_chat?(staff)).to eq(true) + expect(staff_guardian.can_chat?).to eq(true) end it "allows TL1 to chat by default and by extension higher trust levels" do Group.refresh_automatic_groups! - expect(guardian.can_chat?(user)).to eq(true) + expect(guardian.can_chat?).to eq(true) user.update!(trust_level: TrustLevel[3]) Group.refresh_automatic_groups! - expect(guardian.can_chat?(user)).to eq(true) + expect(guardian.can_chat?).to eq(true) end it "allows user in specific group to chat" do SiteSetting.chat_allowed_groups = chat_group.id - expect(guardian.can_chat?(user)).to eq(false) + expect(guardian.can_chat?).to eq(false) chat_group.add(user) user.reload - expect(guardian.can_chat?(user)).to eq(true) + expect(guardian.can_chat?).to eq(true) end describe "chat channel" do