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.
This commit is contained in:
Martin Brennan
2022-12-13 09:14:17 +10:00
committed by GitHub
parent de53cf7657
commit 3ee4b59c64
11 changed files with 24 additions and 25 deletions

View File

@ -8,6 +8,6 @@ class Chat::Api < Chat::ChatBaseController
def ensure_can_chat def ensure_can_chat
raise Discourse::NotFound unless SiteSetting.chat_enabled raise Discourse::NotFound unless SiteSetting.chat_enabled
guardian.ensure_can_chat!(current_user) guardian.ensure_can_chat!
end end
end end

View File

@ -8,7 +8,7 @@ class Chat::ChatBaseController < ::ApplicationController
def ensure_can_chat def ensure_can_chat
raise Discourse::NotFound unless SiteSetting.chat_enabled raise Discourse::NotFound unless SiteSetting.chat_enabled
guardian.ensure_can_chat!(current_user) guardian.ensure_can_chat!
end end
def set_channel_and_chatable_with_access_check(chat_channel_id: nil) def set_channel_and_chatable_with_access_check(chat_channel_id: nil)

View File

@ -346,7 +346,7 @@ class Chat::ChatController < Chat::ChatBaseController
.where(id: params[:user_ids]) .where(id: params[:user_ids])
users.each do |user| users.each do |user|
guardian = Guardian.new(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 = { data = {
message: "chat.invitation_notification", message: "chat.invitation_notification",
chat_channel_id: @chat_channel.id, chat_channel_id: @chat_channel.id,
@ -368,7 +368,7 @@ class Chat::ChatController < Chat::ChatBaseController
def dismiss_retention_reminder def dismiss_retention_reminder
params.require(:chatable_type) params.require(:chatable_type)
guardian.ensure_can_chat!(current_user) guardian.ensure_can_chat!
unless ChatChannel.chatable_types.include?(params[:chatable_type]) unless ChatChannel.chatable_types.include?(params[:chatable_type])
raise Discourse::InvalidParameters raise Discourse::InvalidParameters
end end

View File

@ -4,7 +4,7 @@ class Chat::DirectMessagesController < Chat::ChatBaseController
# NOTE: For V1 of chat channel archiving and deleting we are not doing # NOTE: For V1 of chat channel archiving and deleting we are not doing
# anything for DM channels, their behaviour will stay as is. # anything for DM channels, their behaviour will stay as is.
def create def create
guardian.ensure_can_chat!(current_user) guardian.ensure_can_chat!
users = users_from_usernames(current_user, params) users = users_from_usernames(current_user, params)
begin begin
@ -22,7 +22,7 @@ class Chat::DirectMessagesController < Chat::ChatBaseController
end end
def index def index
guardian.ensure_can_chat!(current_user) guardian.ensure_can_chat!
users = users_from_usernames(current_user, params) users = users_from_usernames(current_user, params)
direct_message = DirectMessage.for_user_ids(users.map(&:id).uniq) direct_message = DirectMessage.for_user_ids(users.map(&:id).uniq)

View File

@ -43,7 +43,7 @@ module Jobs
def send_notifications(membership) def send_notifications(membership)
user = membership.user user = membership.user
guardian = Guardian.new(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 Chat::ChatNotifier.user_has_seen_message?(membership, @chat_message.id)
return if online_user_ids.include?(user.id) return if online_user_ids.include?(user.id)

View File

@ -7,7 +7,7 @@ class Chat::ChatChannelHashtagDataSource
def self.channel_to_hashtag_item(guardian, channel) def self.channel_to_hashtag_item(guardian, channel)
HashtagAutocompleteService::HashtagItem.new.tap do |item| HashtagAutocompleteService::HashtagItem.new.tap do |item|
item.text = channel.title(guardian.user) item.text = channel.title
item.description = channel.description item.description = channel.description
item.slug = channel.slug item.slug = channel.slug
item.icon = icon item.icon = icon
@ -18,7 +18,7 @@ class Chat::ChatChannelHashtagDataSource
def self.lookup(guardian, slugs) def self.lookup(guardian, slugs)
if SiteSetting.enable_experimental_hashtag_autocomplete if SiteSetting.enable_experimental_hashtag_autocomplete
return [] if !guardian.can_chat?(guardian.user) return [] if !guardian.can_chat?
Chat::ChatChannelFetcher Chat::ChatChannelFetcher
.secured_public_channel_slug_lookup(guardian, slugs) .secured_public_channel_slug_lookup(guardian, slugs)
.map { |channel| channel_to_hashtag_item(guardian, channel) } .map { |channel| channel_to_hashtag_item(guardian, channel) }
@ -29,7 +29,7 @@ class Chat::ChatChannelHashtagDataSource
def self.search(guardian, term, limit) def self.search(guardian, term, limit)
if SiteSetting.enable_experimental_hashtag_autocomplete if SiteSetting.enable_experimental_hashtag_autocomplete
return [] if !guardian.can_chat?(guardian.user) return [] if !guardian.can_chat?
Chat::ChatChannelFetcher Chat::ChatChannelFetcher
.secured_public_channel_search( .secured_public_channel_search(
guardian, guardian,
@ -49,7 +49,7 @@ class Chat::ChatChannelHashtagDataSource
def self.search_without_term(guardian, limit) def self.search_without_term(guardian, limit)
if SiteSetting.enable_experimental_hashtag_autocomplete if SiteSetting.enable_experimental_hashtag_autocomplete
return [] if !guardian.can_chat?(guardian.user) return [] if !guardian.can_chat?
allowed_channel_ids_sql = allowed_channel_ids_sql =
Chat::ChatChannelFetcher.generate_allowed_channel_ids_sql( Chat::ChatChannelFetcher.generate_allowed_channel_ids_sql(
guardian, guardian,

View File

@ -195,7 +195,7 @@ class Chat::ChatNotifier
potential_participants, unreachable = potential_participants, unreachable =
users.partition do |user| users.partition do |user|
guardian = Guardian.new(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 end
participants, welcome_to_join = participants, welcome_to_join =

View File

@ -3,7 +3,7 @@
module Chat::UserNotificationsExtension module Chat::UserNotificationsExtension
def chat_summary(user, opts) def chat_summary(user, opts)
guardian = Guardian.new(user) guardian = Guardian.new(user)
return unless guardian.can_chat?(user) return unless guardian.can_chat?
@messages = @messages =
ChatMessage ChatMessage

View File

@ -10,10 +10,9 @@ module Chat::GuardianExtensions
end end
end end
def can_chat?(user) def can_chat?
return false unless user return false if anonymous?
@user.staff? || @user.in_any_groups?(Chat.allowed_group_ids)
user.staff? || user.in_any_groups?(Chat.allowed_group_ids)
end end
def can_create_chat_message? def can_create_chat_message?

View File

@ -386,7 +386,7 @@ after_initialize do
return false if !SiteSetting.chat_enabled return false if !SiteSetting.chat_enabled
return false if scope.user.blank? 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 end
add_to_serializer(:current_user, :can_chat) { true } add_to_serializer(:current_user, :can_chat) { true }
@ -394,7 +394,7 @@ after_initialize do
add_to_serializer(:current_user, :include_can_chat?) do add_to_serializer(:current_user, :include_can_chat?) do
return @can_chat if defined?(@can_chat) 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 end
add_to_serializer(:current_user, :has_chat_enabled) { true } add_to_serializer(:current_user, :has_chat_enabled) { true }

View File

@ -13,28 +13,28 @@ RSpec.describe Chat::GuardianExtensions do
it "cannot chat if the user is not in the Chat.allowed_group_ids" do it "cannot chat if the user is not in the Chat.allowed_group_ids" do
SiteSetting.chat_allowed_groups = "" SiteSetting.chat_allowed_groups = ""
expect(guardian.can_chat?(user)).to eq(false) expect(guardian.can_chat?).to eq(false)
end end
it "staff can always chat regardless of chat_allowed_grups" do it "staff can always chat regardless of chat_allowed_grups" do
SiteSetting.chat_allowed_groups = "" SiteSetting.chat_allowed_groups = ""
expect(guardian.can_chat?(staff)).to eq(true) expect(staff_guardian.can_chat?).to eq(true)
end end
it "allows TL1 to chat by default and by extension higher trust levels" do it "allows TL1 to chat by default and by extension higher trust levels" do
Group.refresh_automatic_groups! 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]) user.update!(trust_level: TrustLevel[3])
Group.refresh_automatic_groups! Group.refresh_automatic_groups!
expect(guardian.can_chat?(user)).to eq(true) expect(guardian.can_chat?).to eq(true)
end end
it "allows user in specific group to chat" do it "allows user in specific group to chat" do
SiteSetting.chat_allowed_groups = chat_group.id 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) chat_group.add(user)
user.reload user.reload
expect(guardian.can_chat?(user)).to eq(true) expect(guardian.can_chat?).to eq(true)
end end
describe "chat channel" do describe "chat channel" do