From baf78d3d91d190b6e03beac3ac024f70b8d163f3 Mon Sep 17 00:00:00 2001 From: Martin Brennan Date: Mon, 19 Dec 2022 11:05:37 +1000 Subject: [PATCH] FIX: Add missing user_id args for ChatMessage.cook (#19508) In both ChatMessage#rebake! and in ChatMessageProcessor when we were calling ChatMessage.cook we were missing the user_id to cook with, which causes missed hashtag cooks because of missing permissions. --- lib/pretty_text/helpers.rb | 7 ++- plugins/chat/app/models/chat_message.rb | 21 ++++--- plugins/chat/lib/chat_message_processor.rb | 2 +- .../spec/lib/chat_message_processor_spec.rb | 10 ++++ plugins/chat/spec/models/chat_message_spec.rb | 57 +++++++++++++++++++ 5 files changed, 88 insertions(+), 9 deletions(-) create mode 100644 plugins/chat/spec/lib/chat_message_processor_spec.rb diff --git a/lib/pretty_text/helpers.rb b/lib/pretty_text/helpers.rb index cb6a5faa16c..819dc060eff 100644 --- a/lib/pretty_text/helpers.rb +++ b/lib/pretty_text/helpers.rb @@ -111,8 +111,13 @@ module PrettyText end def hashtag_lookup(slug, cooking_user_id, types_in_priority_order) - # This is _somewhat_ expected since we need to be able to cook posts + # NOTE: This is _somewhat_ expected since we need to be able to cook posts # etc. without a user sometimes, but it is still an edge case. + # + # The Discourse.system_user is usually an admin with access to _all_ + # categories, however if the suppress_secured_categories_from_admin + # site setting is activated then this user will not be able to access + # secure categories, so hashtags that are secure will not render. if cooking_user_id.blank? cooking_user = Discourse.system_user else diff --git a/plugins/chat/app/models/chat_message.rb b/plugins/chat/app/models/chat_message.rb index dfa43c848ad..6cbe32377c4 100644 --- a/plugins/chat/app/models/chat_message.rb +++ b/plugins/chat/app/models/chat_message.rb @@ -110,19 +110,20 @@ class ChatMessage < ActiveRecord::Base def cook ensure_last_editor_id - # A rule in our Markdown pipeline may have Guardian checks that require a - # user to be present. The last editing user of the message will be more - # generally up to date than the creating user. For example, we use - # this when cooking #hashtags to determine whether we should render - # the found hashtag based on whether the user can access the channel it - # is referencing. self.cooked = self.class.cook(self.message, user_id: self.last_editor_id) self.cooked_version = BAKED_VERSION end def rebake!(invalidate_oneboxes: false, priority: nil) + ensure_last_editor_id + previous_cooked = self.cooked - new_cooked = self.class.cook(message, invalidate_oneboxes: invalidate_oneboxes) + new_cooked = + self.class.cook( + message, + invalidate_oneboxes: invalidate_oneboxes, + user_id: self.last_editor_id, + ) update_columns(cooked: new_cooked, cooked_version: BAKED_VERSION) args = { chat_message_id: self.id } args[:queue] = priority.to_s if priority && priority != :normal @@ -177,6 +178,12 @@ class ChatMessage < ActiveRecord::Base ] def self.cook(message, opts = {}) + # A rule in our Markdown pipeline may have Guardian checks that require a + # user to be present. The last editing user of the message will be more + # generally up to date than the creating user. For example, we use + # this when cooking #hashtags to determine whether we should render + # the found hashtag based on whether the user can access the channel it + # is referencing. cooked = PrettyText.cook( message, diff --git a/plugins/chat/lib/chat_message_processor.rb b/plugins/chat/lib/chat_message_processor.rb index 078e73cf0a0..bf2a621d920 100644 --- a/plugins/chat/lib/chat_message_processor.rb +++ b/plugins/chat/lib/chat_message_processor.rb @@ -10,7 +10,7 @@ class Chat::ChatMessageProcessor @size_cache = {} @opts = {} - cooked = ChatMessage.cook(chat_message.message) + cooked = ChatMessage.cook(chat_message.message, user_id: chat_message.last_editor_id) @doc = Loofah.fragment(cooked) end diff --git a/plugins/chat/spec/lib/chat_message_processor_spec.rb b/plugins/chat/spec/lib/chat_message_processor_spec.rb new file mode 100644 index 00000000000..3fdbd19baa7 --- /dev/null +++ b/plugins/chat/spec/lib/chat_message_processor_spec.rb @@ -0,0 +1,10 @@ +# frozen_string_literal: true + +RSpec.describe Chat::ChatMessageProcessor do + fab!(:message) { Fabricate(:chat_message) } + + it "cooks using the last_editor_id of the message" do + ChatMessage.expects(:cook).with(message.message, user_id: message.last_editor_id) + described_class.new(message) + end +end diff --git a/plugins/chat/spec/models/chat_message_spec.rb b/plugins/chat/spec/models/chat_message_spec.rb index d1a62d7e85a..3982f83800c 100644 --- a/plugins/chat/spec/models/chat_message_spec.rb +++ b/plugins/chat/spec/models/chat_message_spec.rb @@ -5,6 +5,18 @@ require "rails_helper" describe ChatMessage do fab!(:message) { Fabricate(:chat_message, message: "hey friend, what's up?!") } + # TODO (martin) Remove this after https://github.com/discourse/discourse/pull/19491 is merged + def register_hashtag_contexts + # This is annoying, but we need to reset the hashtag data sources inbetween + # tests, and since this is normally done in plugin.rb with the plugin API + # there is not an easier way to do this. + HashtagAutocompleteService.register_data_source("channel", Chat::ChatChannelHashtagDataSource) + HashtagAutocompleteService.register_type_in_context("channel", "chat-composer", 200) + HashtagAutocompleteService.register_type_in_context("category", "chat-composer", 100) + HashtagAutocompleteService.register_type_in_context("tag", "chat-composer", 50) + HashtagAutocompleteService.register_type_in_context("channel", "topic-composer", 10) + end + describe ".cook" do it "does not support HTML tags" do cooked = ChatMessage.cook("

test

") @@ -234,6 +246,7 @@ describe ChatMessage do expect(cooked).to eq("

@mention

") end + # TODO (martin) Remove this when enable_experimental_hashtag_autocomplete is default it "supports category-hashtag plugin" do category = Fabricate(:category) @@ -244,6 +257,20 @@ describe ChatMessage do ) end + it "supports hashtag-autocomplete plugin" do + register_hashtag_contexts + SiteSetting.enable_experimental_hashtag_autocomplete = true + + category = Fabricate(:category) + user = Fabricate(:user) + + cooked = ChatMessage.cook("##{category.slug}", user_id: user.id) + + expect(cooked).to eq( + "

#{category.name}

", + ) + end + it "supports censored plugin" do watched_word = Fabricate(:watched_word, action: WatchedWord.actions[:censor]) @@ -484,4 +511,34 @@ describe ChatMessage do end end end + + describe "#rebake!" do + fab!(:chat_message) { Fabricate(:chat_message) } + + describe "hashtags" do + fab!(:category) { Fabricate(:category) } + fab!(:group) { Fabricate(:group) } + fab!(:secure_category) { Fabricate(:private_category, group: group) } + + before do + register_hashtag_contexts + SiteSetting.enable_experimental_hashtag_autocomplete = true + SiteSetting.suppress_secured_categories_from_admin = true + end + + it "keeps the same hashtags the user has permission to after rebake" do + group.add(chat_message.user) + chat_message.update!( + message: "this is the message ##{category.slug} ##{secure_category.slug} ##{chat_message.chat_channel.slug}", + ) + chat_message.cook + chat_message.save! + + expect(chat_message.reload.cooked).to include(secure_category.name) + + chat_message.rebake! + expect(chat_message.reload.cooked).to include(secure_category.name) + end + end + end end