FIX: allows bots to create/update/stream messages (#26900)

Prior to this commit, only system users had this pass.

Another significant change of the PR, is to make membership of a channel the angular stone of the permission check to create/update/stop streaming a message. The idea being, if you are a member of a channel already we don't need to check if you can join it AGAIN.

We also have `Chat::AutoRemove::HandleCategoryUpdated` which will deal with permissions change so it's simpler and less prone to error to consider the membership as the only source of truth.

plugins/chat/app/controllers/chat/api/channel_messages_controller.rb

@@ -64,7 +64,7 @@ class Chat::Api::ChannelMessagesController < Chat::ApiController
       on_failed_policy(:no_silenced_user) { raise Discourse::InvalidAccess }
       on_model_not_found(:channel) { raise Discourse::NotFound }
       on_failed_policy(:allowed_to_join_channel) { raise Discourse::InvalidAccess }
-      on_model_not_found(:channel_membership) { raise Discourse::InvalidAccess }
+      on_model_not_found(:membership) { raise Discourse::NotFound }
       on_failed_policy(:ensure_reply_consistency) { raise Discourse::NotFound }
       on_failed_policy(:allowed_to_create_message_in_channel) do |policy|
         render_json_error(policy.reason)

plugins/chat/app/controllers/chat/api/channels_messages_streaming_controller.rb

@@ -6,7 +6,7 @@ class Chat::Api::ChannelsMessagesStreamingController < Chat::Api::ChannelsContro
       on_success { render(json: success_json) }
       on_failure { render(json: failed_json, status: 422) }
       on_model_not_found(:message) { raise Discourse::NotFound }
-      on_failed_policy(:can_join_channel) { raise Discourse::InvalidAccess }
+      on_model_not_found(:membership) { raise Discourse::NotFound }
       on_failed_policy(:can_stop_streaming) { raise Discourse::InvalidAccess }
       on_failed_contract do |contract|
         render(json: failed_json.merge(errors: contract.errors.full_messages), status: 400)

plugins/chat/app/services/chat/create_message.rb

@@ -24,10 +24,9 @@ module Chat
     policy :no_silenced_user
     contract
     model :channel
-    step :enforce_system_membership
+    step :enforce_membership
-    policy :allowed_to_join_channel
+    model :membership
     policy :allowed_to_create_message_in_channel, class_name: Chat::Channel::MessageCreationPolicy
-    model :channel_membership
     model :reply, optional: true
     policy :ensure_reply_consistency
     model :thread, optional: true
@@ -81,8 +80,8 @@ module Chat
       Chat::Channel.find_by_id_or_slug(contract.chat_channel_id)
     end
 
-    def enforce_system_membership(guardian:, channel:, contract:)
+    def enforce_membership(guardian:, channel:, contract:)
-      if guardian.user&.is_system_user? || contract.enforce_membership
+      if guardian.user.bot? || contract.enforce_membership
         channel.add(guardian.user)
 
         if channel.direct_message_channel?
@@ -91,12 +90,8 @@ module Chat
       end
     end
 
-    def allowed_to_join_channel(guardian:, channel:)
+    def fetch_membership(guardian:, channel:)
-      channel.membership_for(guardian.user) || guardian.can_join_chat_channel?(channel)
+      channel.membership_for(guardian.user)
-    end
-
-    def fetch_channel_membership(guardian:, channel:)
-      Chat::ChannelMembershipManager.new(channel).find_for_user(guardian.user)
     end
 
     def fetch_reply(contract:)
@@ -187,15 +182,13 @@ module Chat
       channel.update!(last_message: message_instance)
     end
 
-    def update_membership_last_read(channel_membership:, message_instance:)
+    def update_membership_last_read(membership:, message_instance:)
       return if message_instance.in_thread?
-      channel_membership.update!(last_read_message: message_instance)
+      membership.update!(last_read_message: message_instance)
     end
 
-    def process_direct_message_channel(channel_membership:)
+    def process_direct_message_channel(membership:)
-      Chat::Action::PublishAndFollowDirectMessageChannel.call(
+      Chat::Action::PublishAndFollowDirectMessageChannel.call(channel_membership: membership)
-        channel_membership: channel_membership,
-      )
     end
 
     def publish_new_thread(reply:, contract:, channel:, thread:)
@@ -238,10 +231,10 @@ module Chat
       message_instance.excerpt = message_instance.build_excerpt
     end
 
-    def publish_user_tracking_state(message_instance:, channel:, channel_membership:, guardian:)
+    def publish_user_tracking_state(message_instance:, channel:, membership:, guardian:)
       message_to_publish = message_instance
       message_to_publish =
-        channel_membership.last_read_message || message_instance if message_instance.in_thread?
+        membership.last_read_message || message_instance if message_instance.in_thread?
       Chat::Publisher.publish_user_tracking_state!(guardian.user, channel, message_to_publish)
     end
   end

plugins/chat/app/services/chat/stop_message_streaming.rb

@@ -15,7 +15,8 @@ module Chat
     #   @return [Service::Base::Context]
     contract
     model :message
-    policy :can_join_channel
+    step :enforce_membership
+    model :membership
     policy :can_stop_streaming
     step :stop_message_streaming
     step :publish_message_streaming_state
@@ -33,12 +34,16 @@ module Chat
       ::Chat::Message.find_by(id: contract.message_id)
     end
 
-    def can_join_channel(guardian:, message:)
+    def enforce_membership(guardian:, message:)
-      guardian.can_join_chat_channel?(message.chat_channel)
+      message.chat_channel.add(guardian.user) if guardian.user.bot?
+    end
+
+    def fetch_membership(guardian:, message:)
+      message.chat_channel.membership_for(guardian.user)
     end
 
     def can_stop_streaming(guardian:, message:)
-      guardian.is_admin? || message.user.id == guardian.user.id ||
+      guardian.user.bot? || guardian.is_admin? || message.user.id == guardian.user.id ||
         message.in_reply_to && message.in_reply_to.user_id == guardian.user.id
     end
 

plugins/chat/app/services/chat/update_message.rb

@@ -18,7 +18,8 @@ module Chat
     contract
     model :message
     model :uploads, optional: true
-    step :enforce_system_membership
+    step :enforce_membership
+    model :membership
     policy :can_modify_channel_message
     policy :can_modify_message
     step :clean_message
@@ -49,8 +50,8 @@ module Chat
 
     private
 
-    def enforce_system_membership(guardian:, message:)
+    def enforce_membership(guardian:, message:)
-      message.chat_channel.add(guardian.user) if guardian.user.is_system_user?
+      message.chat_channel.add(guardian.user) if guardian.user.bot?
     end
 
     def fetch_message(contract:)
@@ -71,6 +72,10 @@ module Chat
       ).find_by(id: contract.message_id)
     end
 
+    def fetch_membership(guardian:, message:)
+      message.chat_channel.membership_for(guardian.user)
+    end
+
     def fetch_uploads(contract:, guardian:)
       return if !SiteSetting.chat_allow_uploads
       guardian.user.uploads.where(id: contract.upload_ids)

plugins/chat/lib/chat_sdk/message.rb

@@ -94,6 +94,9 @@ module ChatSDK
       with_service(Chat::StopMessageStreaming, message_id: message_id, guardian: guardian) do
         on_success { result.message }
         on_model_not_found(:message) { raise "Couldn't find message with id: `#{message_id}`" }
+        on_model_not_found(:membership) do
+          raise "Couldn't find membership for user with id: `#{guardian.user.id}`"
+        end
         on_failed_policy(:can_join_channel) do
           raise "User with id: `#{guardian.user.id}` can't join this channel"
         end
@@ -132,8 +135,8 @@ module ChatSDK
           strip_whitespaces: strip_whitespaces,
         ) do
           on_model_not_found(:channel) { raise "Couldn't find channel with id: `#{channel_id}`" }
-          on_model_not_found(:channel_membership) do
+          on_model_not_found(:membership) do
-            raise "User with id: `#{guardian.user.id}` has no membership to this channel"
+            raise "Couldn't find membership for user with id: `#{guardian.user.id}`"
           end
           on_failed_policy(:ensure_valid_thread_for_channel) do
             raise "Couldn't find thread with id: `#{thread_id}`"

plugins/chat/spec/lib/chat_sdk/message_spec.rb

@@ -48,7 +48,7 @@ describe ChatSDK::Message do
         params[:guardian] = Fabricate(:user).guardian
 
         expect { described_class.create(**params) }.to raise_error(
-          "User with id: `#{params[:guardian].user.id}` can't join this channel",
+          "Couldn't find membership for user with id: `#{params[:guardian].user.id}`",
         )
       end
     end
@@ -111,6 +111,7 @@ describe ChatSDK::Message do
 
     before do
       SiteSetting.chat_allowed_groups = [Group::AUTO_GROUPS[:everyone]]
+      message_1.chat_channel.add(message_1.user)
       message_1.update!(streaming: true)
     end
 
@@ -131,7 +132,7 @@ describe ChatSDK::Message do
       end
     end
 
-    context "when user can't join channel" do
+    context "when user is not part of the channel" do
       fab!(:message_1) do
         Fabricate(:chat_message, chat_channel: Fabricate(:private_category_channel))
       end
@@ -141,7 +142,7 @@ describe ChatSDK::Message do
 
         expect {
           described_class.stop_stream(message_id: message_1.id, guardian: user.guardian)
-        }.to raise_error("User with id: `#{user.id}` can't join this channel")
+        }.to raise_error("Couldn't find membership for user with id: `#{user.id}`")
       end
     end
   end
@@ -167,7 +168,11 @@ describe ChatSDK::Message do
 
   describe ".stream" do
     fab!(:message_1) { Fabricate(:chat_message, message: "first\n") }
-    before { message_1.update!(streaming: true) }
+
+    before do
+      message_1.chat_channel.add(message_1.user)
+      message_1.update!(streaming: true)
+    end
 
     it "streams" do
       edit =

plugins/chat/spec/models/chat/message_spec.rb

@@ -513,6 +513,8 @@ describe Chat::Message do
 
       it "keeps the same hashtags the user has permission to after rebake" do
         group.add(chat_message.user)
+        chat_message.chat_channel.add(chat_message.user)
+
         update_message!(
           chat_message,
           user: chat_message.user,

plugins/chat/spec/requests/chat/api/channel_messages_controller_spec.rb

@@ -11,7 +11,7 @@ RSpec.describe Chat::Api::ChannelMessagesController do
     sign_in(current_user)
   end
 
-  describe "index" do
+  describe "#index" do
     describe "success" do
       fab!(:message_1) { Fabricate(:chat_message, chat_channel: channel) }
       fab!(:message_2) { Fabricate(:chat_message) }
@@ -92,4 +92,50 @@ RSpec.describe Chat::Api::ChannelMessagesController do
       end
     end
   end
+
+  describe "#update" do
+    context "when message is updated" do
+      fab!(:message_1) { Fabricate(:chat_message, chat_channel: channel, user: current_user) }
+      it "works" do
+        put "/chat/api/channels/#{channel.id}/messages/#{message_1.id}",
+            params: {
+              message: "abcdefg",
+            }
+
+        expect(response.status).to eq(200)
+        expect(message_1.reload.message).to eq("abcdefg")
+      end
+
+      context "when params are invalid" do
+        it "returns a 400" do
+          put "/chat/api/channels/#{channel.id}/messages/#{message_1.id}"
+
+          expect(response.status).to eq(400)
+        end
+      end
+
+      context "when user is not part of the channel" do
+        before { channel.membership_for(current_user).destroy! }
+
+        it "returns a 404" do
+          put "/chat/api/channels/#{channel.id}/messages/#{message_1.id}"
+
+          expect(response.status).to eq(400)
+        end
+      end
+
+      context "when user is not the author" do
+        fab!(:message_1) { Fabricate(:chat_message, chat_channel: channel) }
+
+        it "returns a 422" do
+          put "/chat/api/channels/#{channel.id}/messages/#{message_1.id}",
+              params: {
+                message: "abcdefg",
+              }
+
+          expect(response.status).to eq(422)
+        end
+      end
+    end
+  end
 end

plugins/chat/spec/requests/chat/api/channel_threads_controller_spec.rb

@@ -105,6 +105,7 @@ RSpec.describe Chat::Api::ChannelThreadsController do
     end
 
     before do
+      public_channel.add(current_user)
       thread_1.add(current_user)
       thread_3.add(current_user)
     end
@@ -118,6 +119,7 @@ RSpec.describe Chat::Api::ChannelThreadsController do
     end
 
     it "has preloaded chat mentions and users for the thread original message" do
+      public_channel.add(thread_1.original_message.user)
       update_message!(
         thread_1.original_message,
         user: thread_1.original_message.user,

plugins/chat/spec/requests/chat/api/channels_messages_streaming_controller_spec.rb

@@ -43,6 +43,8 @@ RSpec.describe Chat::Api::ChannelsMessagesStreamingController do
     context "when the user can’t stop" do
       fab!(:message_1) { Fabricate(:chat_message, chat_channel: channel_1) }
 
+      before { channel_1.add(current_user) }
+
       it "returns a 403 error" do
         delete "/chat/api/channels/#{channel_1.id}/messages/#{message_1.id}/streaming"
 
@@ -50,9 +52,21 @@ RSpec.describe Chat::Api::ChannelsMessagesStreamingController do
       end
     end
 
+    context "when the user is not a member" do
+      fab!(:message_1) { Fabricate(:chat_message, chat_channel: channel_1, user: current_user) }
+
+      it "returns a 404 error" do
+        delete "/chat/api/channels/#{channel_1.id}/messages/#{message_1.id}/streaming"
+
+        expect(response.status).to eq(404)
+      end
+    end
+
     context "when the user can stop" do
       fab!(:current_user) { Fabricate(:admin) }
-      fab!(:message_1) { Fabricate(:chat_message, chat_channel: channel_1) }
+      fab!(:message_1) { Fabricate(:chat_message, chat_channel: channel_1, user: current_user) }
+
+      before { channel_1.add(current_user) }
 
       it "returns a 200" do
         delete "/chat/api/channels/#{channel_1.id}/messages/#{message_1.id}/streaming"

plugins/chat/spec/requests/chat/api/messages_controller_spec.rb

@@ -207,12 +207,13 @@ RSpec.describe Chat::Api::ChannelMessagesController do
     describe "for category" do
       fab!(:chat_channel) { Fabricate(:category_channel, chatable: category) }
 
+      before do
+        chat_channel.add(user)
+        sign_in(user)
+      end
+
       context "when current user is silenced" do
-        before do
+        before { UserSilencer.new(user).silence }
-          chat_channel.add(user)
-          sign_in(user)
-          UserSilencer.new(user).silence
-        end
 
         it "raises invalid acces" do
           post "/chat/#{chat_channel.id}.json", params: { message: message }
@@ -221,24 +222,20 @@ RSpec.describe Chat::Api::ChannelMessagesController do
       end
 
       it "errors for regular user when chat is staff-only" do
-        sign_in(user)
         SiteSetting.chat_allowed_groups = Group::AUTO_GROUPS[:staff]
 
         post "/chat/#{chat_channel.id}.json", params: { message: message }
         expect(response.status).to eq(403)
       end
 
-      it "errors when the user isn't following the channel" do
+      it "errors when the user isn't member of the channel" do
-        sign_in(user)
+        chat_channel.membership_for(user).destroy!
 
         post "/chat/#{chat_channel.id}.json", params: { message: message }
-        expect(response.status).to eq(403)
+        expect(response.status).to eq(404)
       end
 
       it "errors when the user is not staff and the channel is not open" do
-        Fabricate(:user_chat_channel_membership, chat_channel: chat_channel, user: user)
-        sign_in(user)
-
         chat_channel.update(status: :closed)
         post "/chat/#{chat_channel.id}.json", params: { message: message }
         expect(response.status).to eq(422)
@@ -247,20 +244,21 @@ RSpec.describe Chat::Api::ChannelMessagesController do
         )
       end
 
-      it "errors when the user is staff and the channel is not open or closed" do
+      context "when the user is staff" do
-        Fabricate(:user_chat_channel_membership, chat_channel: chat_channel, user: admin)
+        fab!(:user) { Fabricate(:admin) }
-        sign_in(admin)
 
-        chat_channel.update(status: :closed)
+        it "errors when the channel is not open or closed" do
-        post "/chat/#{chat_channel.id}.json", params: { message: message }
+          chat_channel.update(status: :closed)
-        expect(response.status).to eq(200)
+          post "/chat/#{chat_channel.id}.json", params: { message: message }
+          expect(response.status).to eq(200)
 
-        chat_channel.update(status: :read_only)
+          chat_channel.update(status: :read_only)
-        post "/chat/#{chat_channel.id}.json", params: { message: message }
+          post "/chat/#{chat_channel.id}.json", params: { message: message }
-        expect(response.status).to eq(422)
+          expect(response.status).to eq(422)
-        expect(response.parsed_body["errors"]).to include(
+          expect(response.parsed_body["errors"]).to include(
-          I18n.t("chat.errors.channel_new_message_disallowed.read_only"),
+            I18n.t("chat.errors.channel_new_message_disallowed.read_only"),
-        )
+          )
+        end
       end
 
       context "when the regular user is following the channel" do
@@ -275,8 +273,6 @@ RSpec.describe Chat::Api::ChannelMessagesController do
         end
 
         it "sends a message for regular user when staff-only is disabled and they are following channel" do
-          sign_in(user)
-
           expect { post "/chat/#{chat_channel.id}.json", params: { message: message } }.to change {
             Chat::Message.count
           }.by(1)
@@ -292,7 +288,6 @@ RSpec.describe Chat::Api::ChannelMessagesController do
         end
 
         it "publishes user tracking state using the new chat message as the last_read_message_id" do
-          sign_in(user)
           messages =
             MessageBus.track_publish(
               Chat::Publisher.user_tracking_state_message_bus_channel(user.id),
@@ -306,8 +301,6 @@ RSpec.describe Chat::Api::ChannelMessagesController do
             Fabricate(:chat_thread, channel: chat_channel, original_message: message_1)
           end
 
-          before { sign_in(user) }
-
           it "does not update the last_read_message_id for the user who sent the message" do
             post "/chat/#{chat_channel.id}.json", params: { message: message, thread_id: thread.id }
             expect(response.status).to eq(200)
@@ -332,9 +325,7 @@ RSpec.describe Chat::Api::ChannelMessagesController do
           context "when thread is not part of the provided channel" do
             let!(:another_channel) { Fabricate(:category_channel) }
 
-            before do
+            before { another_channel.add(user) }
-              Fabricate(:user_chat_channel_membership, chat_channel: another_channel, user: user)
-            end
 
             it "returns an error" do
               post "/chat/#{another_channel.id}.json",
@@ -399,7 +390,7 @@ RSpec.describe Chat::Api::ChannelMessagesController do
 
         sign_in(user1)
         post "/chat/#{direct_message_channel.id}.json", params: { message: message }
-        expect(response.status).to eq(403)
+        expect(response.status).to eq(404)
 
         Chat::UserChatChannelMembership.find_by(user_id: user2.id).update!(following: true)
         sign_in(user2)

plugins/chat/spec/services/chat/create_message_spec.rb

@@ -47,6 +47,8 @@ RSpec.describe Chat::CreateMessage do
     end
     let(:message) { result[:message_instance].reload }
 
+    before { channel.add(guardian.user) }
+
     shared_examples "creating a new message" do
       it "saves the message" do
         expect { result }.to change { Chat::Message.count }.by(1)
@@ -226,19 +228,13 @@ RSpec.describe Chat::CreateMessage do
         end
 
         context "when channel model is found" do
-          context "when user can't join channel" do
+          context "when user is not part of the channel" do
-            let(:guardian) { other_user.guardian }
+            before { channel.membership_for(user).destroy! }
 
-            it { is_expected.to fail_a_policy(:allowed_to_join_channel) }
+            it { is_expected.to fail_to_find_a_model(:membership) }
-
-            context "when the user is already member of the channel" do
-              before { channel.add(guardian.user) }
-
-              it { is_expected.to be_a_success }
-            end
           end
 
-          context "when user is system" do
+          context "when user is a bot" do
             fab!(:user) { Discourse.system_user }
 
             it { is_expected.to be_a_success }
@@ -265,17 +261,13 @@ RSpec.describe Chat::CreateMessage do
             end
 
             context "when user can create a message in the channel" do
-              context "when user is not a member of the channel" do
-                it { is_expected.to fail_to_find_a_model(:channel_membership) }
-              end
-
               context "when user is a member of the channel" do
                 fab!(:existing_message) { Fabricate(:chat_message, chat_channel: channel) }
 
-                let(:membership) { Chat::UserChatChannelMembership.last }
+                let(:membership) { channel.membership_for(user) }
 
                 before do
-                  channel.add(user).update!(last_read_message: existing_message)
+                  membership.update!(last_read_message: existing_message)
                   DiscourseEvent.stubs(:trigger)
                 end
 

plugins/chat/spec/services/chat/stop_message_streaming_spec.rb

@@ -4,19 +4,20 @@ RSpec.describe Chat::StopMessageStreaming do
   describe ".call" do
     subject(:result) { described_class.call(params) }
 
-    let(:params) { { guardian: guardian } }
     let(:guardian) { Guardian.new(current_user) }
+    let(:params) { { guardian: guardian, message_id: message_1.id } }
 
     fab!(:current_user) { Fabricate(:user) }
     fab!(:channel_1) { Fabricate(:chat_channel) }
+    fab!(:message_1) { Fabricate(:chat_message, chat_channel: channel_1, streaming: true) }
 
-    before { SiteSetting.chat_allowed_groups = [Group::AUTO_GROUPS[:everyone]] }
+    before do
+      channel_1.add(current_user)
+      SiteSetting.chat_allowed_groups = [Group::AUTO_GROUPS[:everyone]]
+    end
 
     context "with valid params" do
       fab!(:current_user) { Fabricate(:admin) }
-      fab!(:message_1) { Fabricate(:chat_message, chat_channel: channel_1, streaming: true) }
-
-      let(:params) { { guardian: guardian, channel_id: channel_1.id, message_id: message_1.id } }
 
       it { is_expected.to be_a_success }
 
@@ -33,25 +34,31 @@ RSpec.describe Chat::StopMessageStreaming do
       end
     end
 
-    context "when the channel_id is not provided" do
+    context "when the user is not part of the channel" do
-      it { is_expected.to fail_a_contract }
+      before { channel_1.membership_for(current_user).destroy! }
+
+      it { is_expected.to fail_to_find_a_model(:membership) }
+
+      context "when the user is a bot" do
+        fab!(:current_user) { Discourse.system_user }
+
+        it { is_expected.to be_a_success }
+      end
     end
 
     context "when the message_id is not provided" do
-      let(:params) { { guardian: guardian, channel_id: channel_1.id } }
+      before { params[:message_id] = nil }
 
       it { is_expected.to fail_a_contract }
     end
 
     context "when the message doesnt exist" do
-      let(:params) { { guardian: guardian, channel_id: channel_1.id, message_id: -999 } }
+      before { params[:message_id] = -999 }
 
       it { is_expected.to fail_to_find_a_model(:message) }
     end
 
     context "when the message is a reply" do
-      let(:params) { { guardian: guardian, channel_id: channel_1.id, message_id: reply.id } }
-
       context "when the OM is from current user" do
         fab!(:original_message) do
           Fabricate(:chat_message, chat_channel: channel_1, user: current_user)
@@ -60,6 +67,8 @@ RSpec.describe Chat::StopMessageStreaming do
           Fabricate(:chat_message, chat_channel: channel_1, in_reply_to: original_message)
         end
 
+        before { params[:message_id] = reply.id }
+
         it { is_expected.to be_a_success }
       end
 
@@ -71,10 +80,18 @@ RSpec.describe Chat::StopMessageStreaming do
           Fabricate(:chat_message, chat_channel: channel_1, in_reply_to: original_message)
         end
 
+        before { params[:message_id] = reply.id }
+
         context "when current user is a regular user" do
           it { is_expected.to fail_a_policy(:can_stop_streaming) }
         end
 
+        context "when current user is a bot" do
+          fab!(:current_user) { Discourse.system_user }
+
+          it { is_expected.to be_a_success }
+        end
+
         context "when current user is an admin" do
           fab!(:current_user) { Fabricate(:admin) }
 
@@ -84,14 +101,20 @@ RSpec.describe Chat::StopMessageStreaming do
     end
 
     context "when the message is not a reply" do
-      let(:params) { { guardian: guardian, channel_id: channel_1.id, message_id: message.id } }
-
       fab!(:message) { Fabricate(:chat_message, chat_channel: channel_1) }
 
+      before { params[:message_id] = message.id }
+
       context "when current user is a regular user" do
         it { is_expected.to fail_a_policy(:can_stop_streaming) }
       end
 
+      context "when current user is a bot" do
+        fab!(:current_user) { Discourse.system_user }
+
+        it { is_expected.to be_a_success }
+      end
+
       context "when current user is an admin" do
         fab!(:current_user) { Fabricate(:admin) }
 

plugins/chat/spec/services/chat/update_message_spec.rb

@@ -43,9 +43,7 @@ RSpec.describe Chat::UpdateMessage do
       SiteSetting.chat_duplicate_message_sensitivity = 0
       Jobs.run_immediately!
 
-      [admin1, admin2, user1, user2, user3, user4].each do |user|
+      [admin1, admin2, user1, user2, user3, user4].each { |user| public_chat_channel.add(user) }
-        Fabricate(:user_chat_channel_membership, chat_channel: public_chat_channel, user: user)
-      end
     end
 
     def create_chat_message(user, message, channel, upload_ids: nil)
@@ -452,11 +450,7 @@ RSpec.describe Chat::UpdateMessage do
 
         it "doesn't notify the second time users that has already been notified when creating the message" do
           group_user = Fabricate(:user)
-          Fabricate(
+          public_chat_channel.add(group_user)
-            :user_chat_channel_membership,
-            chat_channel: public_chat_channel,
-            user: group_user,
-          )
           group =
             Fabricate(
               :public_group,
@@ -483,7 +477,7 @@ RSpec.describe Chat::UpdateMessage do
       describe "with @here mentions" do
         it "doesn't notify the second time users that has already been notified when creating the message" do
           user = Fabricate(:user)
-          Fabricate(:user_chat_channel_membership, chat_channel: public_chat_channel, user: user)
+          public_chat_channel.add(user)
           user.update!(last_seen_at: 4.minutes.ago)
 
           chat_message = create_chat_message(user1, "Mentioning @here", public_chat_channel)
@@ -504,7 +498,7 @@ RSpec.describe Chat::UpdateMessage do
       describe "with @all mentions" do
         it "doesn't notify the second time users that has already been notified when creating the message" do
           user = Fabricate(:user)
-          Fabricate(:user_chat_channel_membership, chat_channel: public_chat_channel, user: user)
+          public_chat_channel.add(user)
 
           chat_message = create_chat_message(user1, "Mentioning @all", public_chat_channel)
           notification_id = user.notifications.first.id
@@ -880,6 +874,8 @@ RSpec.describe Chat::UpdateMessage do
       SiteSetting.chat_editing_grace_period = 10
       SiteSetting.chat_editing_grace_period_max_diff_low_trust = 10
       SiteSetting.chat_editing_grace_period_max_diff_high_trust = 40
+
+      channel_1.add(current_user)
     end
 
     context "when all steps pass" do
@@ -920,6 +916,15 @@ RSpec.describe Chat::UpdateMessage do
         Jobs::Chat::ProcessMessage.any_instance.expects(:execute).once
         expect_not_enqueued_with(job: Jobs::Chat::ProcessMessage) { result }
       end
+
+      context "when user is a bot" do
+        fab!(:bot) { Discourse.system_user }
+        let(:guardian) { Guardian.new(bot) }
+
+        it "creates the membership" do
+          expect { result }.to change { channel_1.membership_for(bot) }.from(nil).to(be_present)
+        end
+      end
     end
 
     context "when params are not valid" do
@@ -934,10 +939,10 @@ RSpec.describe Chat::UpdateMessage do
       it { is_expected.to fail_a_policy(:can_modify_channel_message) }
     end
 
-    context "when user can't modify this message" do
+    context "when user is not member of the channel" do
       let(:message_id) { Fabricate(:chat_message).id }
 
-      it { is_expected.to fail_a_policy(:can_modify_message) }
+      it { is_expected.to fail_to_find_a_model(:membership) }
     end
 
     context "when edit grace period" do