FIX: Require channel access to delete your own chat messages (#41003)

Before, a user who left a direct message or lost access to a private
category channel could still delete their own messages there, because
`can_delete_chat?` only checked ownership and channel status — not
current visibility.

This change requires `can_preview_chat_channel?` for self-deletes,
mirroring `can_restore_chat?`, so former members can no longer trash
messages in channels they can no longer access.

This ensures symmetry in guardian methods.

Relates to PATCH-1087

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
Gabriel Grubba
2026-06-18 10:18:25 -03:00
committed by GitHub
co-authored by Claude Opus 4.8
parent b6b103e7a0
commit 76cc8523f7
3 changed files with 89 additions and 0 deletions
@@ -221,6 +221,8 @@ module Chat
return false if !can_modify_channel_message?(message.chat_channel)
if message.user_id == current_user.id
return false if !can_preview_chat_channel?(message.chat_channel)
can_delete_own_chats?(chatable)
else
can_delete_other_chats?(chatable)
@@ -640,6 +640,62 @@ RSpec.describe Chat::GuardianExtensions do
end
end
describe "#can_delete_chat?" do
fab!(:message) { Fabricate(:chat_message, chat_channel: channel, user: user) }
fab!(:chatable, :category)
context "when user is owner of the message" do
it "allows the owner to delete while they can still see the channel" do
expect(guardian.can_delete_chat?(message, chatable)).to eq(true)
end
context "when the owner has lost access to a private category channel" do
fab!(:revoke_group, :group)
fab!(:revoked_category) { Fabricate(:private_category, group: revoke_group) }
fab!(:revoked_channel) { Fabricate(:chat_channel, chatable: revoked_category) }
fab!(:message) { Fabricate(:chat_message, chat_channel: revoked_channel, user: user) }
before do
revoke_group.add(user)
GroupUser.where(group: revoke_group, user: user).destroy_all
end
it "disallows the owner to delete" do
expect(guardian.can_delete_chat?(message, revoked_category)).to eq(false)
end
end
context "when the owner is no longer in a direct message channel" do
fab!(:other_user, :user)
fab!(:dm_channel) { Fabricate(:direct_message_channel, users: [user, other_user]) }
fab!(:message) { Fabricate(:chat_message, chat_channel: dm_channel, user: user) }
before { dm_channel.chatable.direct_message_users.find_by!(user: user).destroy! }
it "disallows the owner to delete" do
expect(guardian.can_delete_chat?(message, dm_channel.chatable)).to eq(false)
end
end
end
context "when user is not owner of the message" do
fab!(:other_user, :user)
fab!(:message) { Fabricate(:chat_message, chat_channel: channel, user: other_user) }
context "when chatable is a direct message the actor cannot preview" do
fab!(:chatable) { Chat::DirectMessage.create! }
it "still allows staff to delete (non-owner path is unchanged)" do
expect(staff_guardian.can_delete_chat?(message, chatable)).to eq(true)
end
it "disallows a regular user to delete" do
expect(guardian.can_delete_chat?(message, chatable)).to eq(false)
end
end
end
end
describe "#can_edit_chat" do
fab!(:message) { Fabricate(:chat_message, chat_channel: channel) }
@@ -105,6 +105,37 @@ RSpec.describe Chat::Api::ChannelMessagesController do
expect(response.status).to eq(200)
end
end
describe "when the owner has lost access to the channel" do
it "doesn't allow deleting their own message in a private category channel" do
group = Fabricate(:group)
group.add(current_user)
channel = Fabricate(:private_category_channel, group: group)
message = Fabricate(:chat_message, chat_channel: channel, user: current_user)
group.remove(current_user)
sign_in(current_user)
expect {
delete "/chat/api/channels/#{message.chat_channel_id}/messages/#{message.id}.json"
}.not_to change { Chat::Message.count }
expect(response.status).to eq(403)
end
it "doesn't allow deleting their own message after leaving a direct message" do
other_user = Fabricate(:user)
channel = Fabricate(:direct_message_channel, users: [current_user, other_user])
message = Fabricate(:chat_message, chat_channel: channel, user: current_user)
channel.chatable.direct_message_users.find_by!(user: current_user).destroy!
sign_in(current_user)
expect {
delete "/chat/api/channels/#{message.chat_channel_id}/messages/#{message.id}.json"
}.not_to change { Chat::Message.count }
expect(response.status).to eq(403)
end
end
end
describe "#restore" do