From a907bc891acdfbee05309510a7d43636a306ff1c Mon Sep 17 00:00:00 2001
From: Sam <sam.saffron@gmail.com>
Date: Wed, 11 Jun 2025 19:33:34 +1000
Subject: [PATCH] FIX: improve admin api for artifact key values (#1425)

Previously we had a logic error and were showing admins keys
that are not theirs when querying for all keys

This makes the API cleaner, to get all results you need to be explicit always
---
 .../ai_bot/artifact_key_values_controller.rb  |  8 ++-
 .../artifact_key_values_controller_spec.rb    | 51 ++++++++-----------
 2 files changed, 28 insertions(+), 31 deletions(-)

diff --git a/app/controllers/discourse_ai/ai_bot/artifact_key_values_controller.rb b/app/controllers/discourse_ai/ai_bot/artifact_key_values_controller.rb
index a05a7488..83b003f7 100644
--- a/app/controllers/discourse_ai/ai_bot/artifact_key_values_controller.rb
+++ b/app/controllers/discourse_ai/ai_bot/artifact_key_values_controller.rb
@@ -102,8 +102,12 @@ module DiscourseAi
 
         query = query.where("key = ?", index_params[:key]) if index_params[:key].present?
 
-        if !index_params[:all_users].to_s == "true" && current_user
-          query = query.where(user_id: current_user.id)
+        if index_params[:all_users].to_s != "true"
+          if current_user
+            query = query.where(user_id: current_user.id)
+          else
+            query = query.where("1 = 0")
+          end
         end
 
         query
diff --git a/spec/requests/ai_bot/artifact_key_values_controller_spec.rb b/spec/requests/ai_bot/artifact_key_values_controller_spec.rb
index b27bb47f..add30f0f 100644
--- a/spec/requests/ai_bot/artifact_key_values_controller_spec.rb
+++ b/spec/requests/ai_bot/artifact_key_values_controller_spec.rb
@@ -50,9 +50,23 @@ RSpec.describe DiscourseAi::AiBot::ArtifactKeyValuesController do
       )
     end
 
+    fab!(:admin_key_value) do
+      Fabricate(
+        :ai_artifact_key_value,
+        ai_artifact: artifact,
+        user: admin,
+        key: "admin_key",
+        value: "admin_value",
+        public: false,
+      )
+    end
+
     context "when not logged in" do
       it "returns only public key values" do
-        get "/discourse-ai/ai-bot/artifact-key-values/#{artifact.id}.json"
+        get "/discourse-ai/ai-bot/artifact-key-values/#{artifact.id}.json",
+            params: {
+              all_users: true,
+            }
 
         expect(response.status).to eq(200)
         json = response.parsed_body
@@ -86,7 +100,10 @@ RSpec.describe DiscourseAi::AiBot::ArtifactKeyValuesController do
       before { sign_in(user) }
 
       it "returns public key values and own private key values" do
-        get "/discourse-ai/ai-bot/artifact-key-values/#{artifact.id}.json"
+        get "/discourse-ai/ai-bot/artifact-key-values/#{artifact.id}.json",
+            params: {
+              all_users: true,
+            }
 
         expect(response.status).to eq(200)
         json = response.parsed_body
@@ -138,17 +155,13 @@ RSpec.describe DiscourseAi::AiBot::ArtifactKeyValuesController do
     context "when logged in as admin" do
       before { sign_in(admin) }
 
-      it "returns all key values including private ones from other users" do
+      it "returns only my own keys by default" do
         get "/discourse-ai/ai-bot/artifact-key-values/#{artifact.id}.json"
 
         expect(response.status).to eq(200)
         json = response.parsed_body
-        expect(json["key_values"].length).to eq(3)
-        expect(json["key_values"].map { |kv| kv["key"] }).to contain_exactly(
-          "test_key",
-          "private_key",
-          "other_key",
-        )
+        expect(json["key_values"].length).to eq(1)
+        expect(json["key_values"].map { |kv| kv["key"] }).to contain_exactly("admin_key")
       end
 
       it "can access private artifacts" do
@@ -420,24 +433,4 @@ RSpec.describe DiscourseAi::AiBot::ArtifactKeyValuesController do
       end
     end
   end
-
-  describe "private methods" do
-    let(:controller) { described_class.new }
-
-    before do
-      controller.instance_variable_set(:@artifact, artifact)
-      allow(controller).to receive(:params).and_return(
-        ActionController::Parameters.new(test_params),
-      )
-    end
-
-    describe "#key_value_params" do
-      let(:test_params) { { key: "test", value: "value", public: true, extra: "ignored" } }
-
-      it "permits only allowed parameters" do
-        # This would need to be tested by calling the actual method or through integration tests
-        # since private methods are typically tested through their public interfaces
-      end
-    end
-  end
 end