diff --git a/app/controllers/discourse_ai/ai_bot/shared_ai_conversations_controller.rb b/app/controllers/discourse_ai/ai_bot/shared_ai_conversations_controller.rb index c197f9fc..aaed408e 100644 --- a/app/controllers/discourse_ai/ai_bot/shared_ai_conversations_controller.rb +++ b/app/controllers/discourse_ai/ai_bot/shared_ai_conversations_controller.rb @@ -26,7 +26,9 @@ module DiscourseAi def destroy ensure_allowed_destroy! - @shared_conversation.destroy + + SharedAiConversation.destroy_conversation(@shared_conversation) + render json: success_json.merge(message: I18n.t("discourse_ai.share_ai.conversation_deleted")) end diff --git a/app/jobs/regular/shared_conversation_adjust_upload_security.rb b/app/jobs/regular/shared_conversation_adjust_upload_security.rb new file mode 100644 index 00000000..350ca0e7 --- /dev/null +++ b/app/jobs/regular/shared_conversation_adjust_upload_security.rb @@ -0,0 +1,51 @@ +# frozen_string_literal: true + +module ::Jobs + class SharedConversationAdjustUploadSecurity < ::Jobs::Base + def execute(args) + if args[:conversation_id].present? + # The conversation context includes post cooked content so this + # must be updated when target uploads security changes. + update_conversation(args[:conversation_id]) + elsif args[:target_id].present? && args[:target_type].present? + # If we deleted the conversation then we just need to update the target's + # uploads security, no need to update the conversation. + update_target(args[:target_id], args[:target_type]) + end + end + + private + + def update_conversation(conversation_id) + conversation = SharedAiConversation.find_by(id: conversation_id) + return if conversation.blank? + + # NOTE: Only Topics are supported for now, in future we will need a more flexible + # way of doing this. + if conversation.target_type == "Topic" + rebaked_posts = TopicUploadSecurityManager.new(conversation.target).run + + if rebaked_posts.any? + new_context = + conversation.context.map do |context_post| + rebaked_post = rebaked_posts.find { |p| p.id == context_post["id"] } + context_post["cooked"] = rebaked_post.cooked if rebaked_post + context_post + end + + conversation.update(context: new_context) + end + end + end + + def update_target(target_id, target_type) + # NOTE: Only Topics are supported for now, in future we will need a more flexible + # way of doing this. + if target_type == "Topic" + topic = target_type.constantize.find_by(id: target_id) + return if topic.blank? + TopicUploadSecurityManager.new(topic).run + end + end + end +end diff --git a/app/models/shared_ai_conversation.rb b/app/models/shared_ai_conversation.rb index 970deaa9..a3ba6781 100644 --- a/app/models/shared_ai_conversation.rb +++ b/app/models/shared_ai_conversation.rb @@ -19,12 +19,26 @@ class SharedAiConversation < ActiveRecord::Base conversation = find_by(user: user, target: target) conversation_data = build_conversation_data(target, max_posts: max_posts) - if conversation - conversation.update(**conversation_data) - conversation - else - create(user_id: user.id, target: target, **conversation_data) - end + conversation = + if conversation + conversation.update(**conversation_data) + conversation + else + create(user_id: user.id, target: target, **conversation_data) + end + + ::Jobs.enqueue(:shared_conversation_adjust_upload_security, conversation_id: conversation.id) + + conversation + end + + def self.destroy_conversation(conversation) + conversation.destroy + ::Jobs.enqueue( + :shared_conversation_adjust_upload_security, + target_id: conversation.target_id, + target_type: conversation.target_type, + ) end # Technically this may end up being a chat message diff --git a/plugin.rb b/plugin.rb index 846e6b93..b90801d7 100644 --- a/plugin.rb +++ b/plugin.rb @@ -64,4 +64,8 @@ after_initialize do end reloadable_patch { |plugin| Guardian.prepend DiscourseAi::GuardianExtensions } + + register_modifier(:post_should_secure_uploads?) do |_, _, topic| + false if topic.private_message? && SharedAiConversation.exists?(target: topic) + end end diff --git a/spec/jobs/shared_conversation_adjust_upload_security_spec.rb b/spec/jobs/shared_conversation_adjust_upload_security_spec.rb new file mode 100644 index 00000000..3a240495 --- /dev/null +++ b/spec/jobs/shared_conversation_adjust_upload_security_spec.rb @@ -0,0 +1,67 @@ +# frozen_string_literal: true + +RSpec.describe Jobs::SharedConversationAdjustUploadSecurity do + let(:params) { {} } + + fab!(:bot_user) do + SiteSetting.discourse_ai_enabled = true + SiteSetting.ai_bot_enabled_chat_bots = "claude-2" + SiteSetting.ai_bot_enabled = true + SiteSetting.ai_bot_allowed_groups = "10" + SiteSetting.ai_bot_public_sharing_allowed_groups = "10" + User.find(DiscourseAi::AiBot::EntryPoint::CLAUDE_V2_ID) + end + fab!(:user) + fab!(:topic) { Fabricate(:private_message_topic, user: user, recipient: bot_user) } + fab!(:post_1) { Fabricate(:post, topic: topic, user: bot_user) } + fab!(:post_2) { Fabricate(:post, topic: topic, user: user) } + fab!(:conversation) { SharedAiConversation.share_conversation(user, topic) } + + def run_job + described_class.new.execute(params) + end + + context "when conversation is created" do + let(:params) { { conversation_id: conversation.id } } + + it "does nothing for a conversation that has been deleted before the job ran" do + conversation.destroy + SharedAiConversation.any_instance.expects(:update).never + run_job + end + + it "does nothing if there weren't any posts with secure uploads in the topic" do + original_context = conversation.context + run_job + expect(conversation.reload.context).to eq(original_context) + end + + context "when topic posts were rebaked because they had secure uploads" do + it "updates the conversation cooked post content after rebaking" do + post_2.update!(raw: "some new rebaked content") + TopicUploadSecurityManager.any_instance.expects(:run).returns([post_2]) + original_context = conversation.context + run_job + expect(conversation.reload.context).not_to eq(original_context) + end + end + end + + context "when conversation has been deleted" do + let(:params) { { target_id: topic.id, target_type: "Topic" } } + + before { conversation.destroy! } + + it "runs the topic upload security manager but doesn't attempt to update a conversation" do + SharedAiConversation.any_instance.expects(:update).never + TopicUploadSecurityManager.any_instance.expects(:run).once + run_job + end + + it "doesn't attempt to run the topic upload security manager if the topic has been deleted" do + TopicUploadSecurityManager.any_instance.expects(:run).never + topic.trash! + run_job + end + end +end diff --git a/spec/requests/ai_bot/shared_ai_conversations_spec.rb b/spec/requests/ai_bot/shared_ai_conversations_spec.rb index a46c317b..c45d153a 100644 --- a/spec/requests/ai_bot/shared_ai_conversations_spec.rb +++ b/spec/requests/ai_bot/shared_ai_conversations_spec.rb @@ -83,6 +83,59 @@ RSpec.describe DiscourseAi::AiBot::SharedAiConversationsController do post "#{path}.json", params: { topic_id: user_pm_share.id } expect(response).to have_http_status(:success) end + + context "when secure uploads are enabled" do + let(:upload_1) { Fabricate(:s3_image_upload, user: bot_user, secure: true) } + let(:upload_2) { Fabricate(:s3_image_upload, user: bot_user, secure: true) } + let(:post_with_upload_1) { Fabricate(:post, topic: user_pm_share, user: bot_user) } + let(:post_with_upload_2) { Fabricate(:post, topic: user_pm_share, user: bot_user) } + + before do + enable_secure_uploads + stub_s3_store + SiteSetting.secure_uploads_pm_only = true + FileStore::S3Store.any_instance.stubs(:update_upload_ACL).returns(true) + Jobs.run_immediately! + + upload_1.update!( + access_control_post: post_with_upload_1, + sha1: SecureRandom.hex(20), + original_sha1: upload_1.sha1, + ) + upload_2.update!( + access_control_post: post_with_upload_2, + sha1: SecureRandom.hex(20), + original_sha1: upload_2.sha1, + ) + post_with_upload_1.update!( + raw: "This is a post with a cool AI generated picture ![wow](#{upload_1.short_url})", + ) + post_with_upload_2.update!( + raw: + "Another post that has been birthed by AI with a picture ![meow](#{upload_2.short_url})", + ) + end + + it "marks all of those uploads as not secure when sharing the topic" do + post "#{path}.json", params: { topic_id: user_pm_share.id } + expect(response).to have_http_status(:success) + expect(upload_1.reload.secure).to eq(false) + expect(upload_2.reload.secure).to eq(false) + end + + it "rebakes any posts in the topic with uploads attached when sharing the topic so image urls become non-secure" do + post_1_cooked = post_with_upload_1.cooked + post_2_cooked = post_with_upload_2.cooked + + post "#{path}.json", params: { topic_id: user_pm_share.id } + expect(response).to have_http_status(:success) + + expect(post_with_upload_1.reload.cooked).not_to eq(post_1_cooked) + expect(post_with_upload_1.reload.cooked).not_to include("secure-uploads") + expect(post_with_upload_2.reload.cooked).not_to eq(post_2_cooked) + expect(post_with_upload_2.reload.cooked).not_to include("secure-uploads") + end + end end context "when not logged in" do @@ -107,6 +160,57 @@ RSpec.describe DiscourseAi::AiBot::SharedAiConversationsController do delete "#{path}/123.json" expect(response).not_to have_http_status(:success) end + + context "when secure uploads are enabled" do + let(:upload_1) { Fabricate(:s3_image_upload, user: bot_user, secure: false) } + let(:upload_2) { Fabricate(:s3_image_upload, user: bot_user, secure: false) } + + before do + enable_secure_uploads + stub_s3_store + SiteSetting.secure_uploads_pm_only = true + FileStore::S3Store.any_instance.stubs(:update_upload_ACL).returns(true) + Jobs.run_immediately! + + upload_1.update!( + access_control_post: shared_conversation.target.posts.first, + sha1: SecureRandom.hex(20), + original_sha1: upload_1.sha1, + ) + upload_2.update!( + access_control_post: shared_conversation.target.posts.second, + sha1: SecureRandom.hex(20), + original_sha1: upload_2.sha1, + ) + shared_conversation.target.posts.first.update!( + raw: "This is a post with a cool AI generated picture ![wow](#{upload_1.short_url})", + ) + shared_conversation.target.posts.second.update!( + raw: + "Another post that has been birthed by AI with a picture ![meow](#{upload_2.short_url})", + ) + end + + it "marks all uploads in the PM back as secure when unsharing the conversation" do + delete "#{path}/#{shared_conversation.share_key}.json" + expect(response).to have_http_status(:success) + expect(upload_1.reload.secure).to eq(true) + expect(upload_2.reload.secure).to eq(true) + end + + it "rebakes any posts in the topic with uploads attached when sharing the topic so image urls become secure" do + post_1_cooked = shared_conversation.target.posts.first.cooked + post_2_cooked = shared_conversation.target.posts.second.cooked + + delete "#{path}/#{shared_conversation.share_key}.json" + expect(response).to have_http_status(:success) + + expect(shared_conversation.target.posts.first.reload.cooked).not_to eq(post_1_cooked) + expect(shared_conversation.target.posts.first.reload.cooked).to include("secure-uploads") + expect(shared_conversation.target.posts.second.reload.cooked).not_to eq(post_2_cooked) + expect(shared_conversation.target.posts.second.reload.cooked).to include("secure-uploads") + end + end end context "when not logged in" do