0
0
Fork 0
mirror of https://github.com/discourse/discourse.git synced 2026-08-07 13:19:19 +08:00
discourse/plugins/chat/app/services/chat/update_message.rb
Régis Hanol 2708818126
FIX: Surface actionable errors when chat policy checks fail (#41971)
Previously, several chat endpoints answered policy failures with a
generic 422 `{"failed":"FAILED"}`, rendered literally as "FAILED" in the
UI: an admin creating a channel while `enable_public_channels` was
disabled had no way to know why creation failed, and a user saving a
message edit after staff closed the channel hit the same dead end. The
root cause is structural — a service policy failing without a matching
`on_failed_policy` handler silently falls through to the catch-all
`on_failure`, and near-identical actor-shaped policy names made the
handler lists look exhaustive when they weren't.

This change makes policy failures on the channel-creation and
message-edit endpoints answer with either a 403 (authorization) or a 422
carrying an actionable reason (feature or channel state), and makes that
split visible in the code:

- Creating a channel while public channels are disabled now explains the
`enable public channels` site setting instead of failing blankly.
- Editing a message in a closed/read-only channel now explains the
channel status via a new `Chat::Channel::Policy::MessageModification`
reason, mirroring the existing `MessageCreation` pattern.
- Authorization policies run before feature/state policies on both
endpoints (spec-pinned), so unauthorized users keep getting a plain 403
and are never shown state guidance they cannot act on. This flips a few
edit-endpoint failures (non-author, silenced, lost channel access) from
the opaque 422 to a proper 403.
- State policies are renamed with the channel as the grammatical subject
— `channel_allows_message_creation`,
`channel_allows_message_modification` — to distinguish them from actor
checks like `can_edit_message`; the old actor-shaped names are how the
gaps went unnoticed.
- An audit of every handler block in the chat plugin found nine handlers
naming policies or models that no longer exist. Eight were dead code
(removed or repaired to the current names); one was a live bug:
`bulk_destroy` listened for `:invalid_access` while
`Chat::TrashMessages` declares `:can_delete_all_chat_messages`, so
unauthorized bulk deletions returned the generic 422 instead of 403.
That endpoint also gains its first request specs.

Ref - t/188375
2026-07-24 12:27:14 +02:00

198 lines
5.9 KiB
Ruby
Vendored

# frozen_string_literal: true
module Chat
# Service responsible for updating a message.
#
# @example
# Chat::UpdateMessage.call(guardian: guardian, params: { message: "A new message", message_id: 2 })
#
class UpdateMessage
include Service::Base
# @!method self.call(guardian:, params:, options:)
# @param guardian [Guardian]
# @param [Hash] params
# @option params [Integer] :message_id
# @option params [String] :message
# @option params [Array<Integer>] :upload_ids IDs of uploaded documents
# @param [Hash] options
# @option options [Boolean] (true) :strip_whitespaces
# @option options [Boolean] :process_inline
# @return [Service::Base::Context]
options do
attribute :strip_whitespaces, :boolean, default: true
attribute :process_inline, :boolean, default: -> { Rails.env.test? }
end
params do
attribute :message_id, :string
attribute :channel_id, :integer
attribute :message, :string
attribute :upload_ids, :array
validates :message_id, presence: true
validates :channel_id, presence: true
validates :message, presence: true, if: -> { upload_ids.blank? }
validates :message, length: { maximum: -> { SiteSetting.chat_maximum_message_length } }
after_validation do
next if message.blank?
self.message =
TextCleaner.clean(
message,
strip_whitespaces: options.strip_whitespaces,
strip_zero_width_spaces: true,
)
end
end
model :message
model :uploads, optional: true
step :enforce_membership
model :membership
policy :can_edit_message
policy :channel_allows_message_modification,
class_name: Chat::Channel::Policy::MessageModification
transaction do
step :modify_message
step :update_excerpt
step :save_message
step :save_revision
step :publish
end
step :index_message
private
def enforce_membership(guardian:, message:)
message.chat_channel.add(guardian.user) if guardian.user.bot?
end
def fetch_message(params:)
::Chat::Message
.includes(
:chat_mentions,
:bookmarks,
:chat_webhook_event,
:revisions,
reactions: [:user],
thread: [:channel, last_message: [:user]],
chat_channel: [
:last_message,
:chat_channel_archive,
chatable: [:topic_only_relative_url, direct_message_users: [user: :user_option]],
],
user: :user_status,
)
.includes(uploads: { optimized_videos: :optimized_upload })
.find_by(id: params.message_id, chat_channel_id: params.channel_id)
end
def fetch_membership(guardian:, message:)
message.chat_channel.membership_for(guardian.user)
end
def fetch_uploads(params:, guardian:)
return if !SiteSetting.chat_allow_uploads
Upload
.where(id: params.upload_ids)
.joins(:user_uploads)
.where(user_uploads: { user: guardian.user })
end
def can_edit_message(guardian:, message:)
guardian.can_edit_chat?(message)
end
def modify_message(params:, message:, guardian:, uploads:)
message.message = params.message
message.last_editor_id = guardian.user.id
message.cook
return if uploads&.size != params.upload_ids.to_a.size
new_upload_ids = uploads.map(&:id)
existing_upload_ids = message.upload_ids
difference = (existing_upload_ids + new_upload_ids) - (existing_upload_ids & new_upload_ids)
return if !difference.any?
message.upload_ids = new_upload_ids
end
def update_excerpt(message:)
message.excerpt = message.build_excerpt
end
def save_message(message:)
message.save!
end
def save_revision(message:, guardian:)
return false if message.streaming_before_last_save
prev_message = message.message_before_last_save || message.message_was
return if !should_create_revision(message, prev_message, guardian)
context[:revision] = message.revisions.create!(
old_message: prev_message,
new_message: message.message,
user_id: guardian.user.id,
)
end
def should_create_revision(new_message, prev_message, guardian)
max_seconds = SiteSetting.chat_editing_grace_period
seconds_since_created = Time.now.to_i - new_message&.created_at&.iso8601&.to_time.to_i
return true if seconds_since_created > max_seconds
max_edited_chars =
(
if guardian.user.has_trust_level?(TrustLevel[2])
SiteSetting.chat_editing_grace_period_max_diff_high_trust
else
SiteSetting.chat_editing_grace_period_max_diff_low_trust
end
)
chars_edited =
ONPDiff
.new(prev_message, new_message.message)
.short_diff
.sum { |str, type| type == :common ? 0 : str.size }
chars_edited > max_edited_chars
end
def publish(message:, guardian:, options:)
edit_timestamp = context[:revision]&.created_at&.iso8601(6) || Time.zone.now.iso8601(6)
::Chat::Publisher.publish_edit!(message.chat_channel, message)
DiscourseEvent.trigger(:chat_message_edited, message, message.chat_channel, message.user)
if options.process_inline
Jobs::Chat::ProcessMessage.new.execute(
{ chat_message_id: message.id, edit_timestamp: edit_timestamp },
)
else
Jobs.enqueue(
Jobs::Chat::ProcessMessage,
{ chat_message_id: message.id, edit_timestamp: edit_timestamp },
)
end
if message.thread.present?
::Chat::Publisher.publish_thread_original_message_metadata!(message.thread)
end
end
def index_message(message:)
Scheduler::Defer.later "Index chat message for search" do
SearchIndexer.index(message)
end
end
end
end