From da106c7217c244e3d25a429f68fefbdec2f04161 Mon Sep 17 00:00:00 2001
From: Noah Moss <32746338+noahmoss@users.noreply.github.com>
Date: Fri, 6 Sep 2024 17:14:05 -0400
Subject: [PATCH] Fix admin override for public actions (#47717)
---
src/metabase/api/public.clj | 4 +-
test/metabase/api/dashboard_test.clj | 8 +-
test/metabase/api/public_test.clj | 126 +++++++++---------
.../metabase/driver/sql_jdbc/actions_test.clj | 3 +-
test/metabase/query_processor/card_test.clj | 3 +-
.../query_processor/dashboard_test.clj | 3 +-
6 files changed, 73 insertions(+), 74 deletions(-)
diff --git a/src/metabase/api/public.clj b/src/metabase/api/public.clj
index 90b03e67b0a..70fd0978f70 100644
--- a/src/metabase/api/public.clj
+++ b/src/metabase/api/public.clj
@@ -363,7 +363,7 @@
;; Run this query with full superuser perms. We don't want the various perms checks
;; failing because there are no current user perms; if this Dashcard is public
;; you're by definition allowed to run it without a perms check anyway
- (binding [api/*current-user-permissions-set* (delay #{"/"})]
+ (mw.session/as-admin
;; Undo middleware string->keyword coercion
(actions/execute-dashcard! dashboard-id dashcard-id (update-keys parameters name))))))))
@@ -679,7 +679,7 @@
;; Run this query with full superuser perms. We don't want the various perms checks
;; failing because there are no current user perms; if this Dashcard is public
;; you're by definition allowed to run it without a perms check anyway
- (binding [api/*current-user-permissions-set* (delay #{"/"})]
+ (mw.session/as-admin
(let [action (api/check-404 (action/select-action :public_uuid uuid :archived false))]
(snowplow/track-event! ::snowplow/action
{:event :action-executed
diff --git a/test/metabase/api/dashboard_test.clj b/test/metabase/api/dashboard_test.clj
index de4404f4541..0ce88898284 100644
--- a/test/metabase/api/dashboard_test.clj
+++ b/test/metabase/api/dashboard_test.clj
@@ -322,7 +322,7 @@
(deftest get-dashboard-test
(mt/dataset test-data
(mt/with-column-remappings [orders.user_id people.name]
- (binding [api/*current-user-permissions-set* (atom #{"/"})]
+ (mt/as-admin
(t2.with-temp/with-temp
[Dashboard {dashboard-id :id} {:name "Test Dashboard"
:creator_id (mt/user->id :crowberto)
@@ -393,7 +393,7 @@
(deftest last-used-parameter-value-test
(mt/dataset test-data
(mt/with-column-remappings [orders.user_id people.name]
- (binding [api/*current-user-permissions-set* (atom #{"/"})]
+ (mt/as-admin
(t2.with-temp/with-temp
[Dashboard {dashboard-a-id :id} {:name "Test Dashboard"
:creator_id (mt/user->id :crowberto)
@@ -422,7 +422,7 @@
:card_id card-id
:dashboard_id dashboard-b-id}]
(testing "User's set parameter is saved and sent back in the dashboard response, unique per dashboard."
- ;; api request mimicking a user setting a parameter value
+ ;; api request mimicking a user setting a parameter value
(is (some? (mt/user-http-request :rasta :post (format "dashboard/%d/dashcard/%s/card/%s/query" dashboard-a-id dashcard-a-id card-id)
{:parameters [{:id "a" :value ["initial value"]}]})))
(is (some? (mt/user-http-request :rasta :post (format "dashboard/%d/dashcard/%s/card/%s/query" dashboard-b-id dashcard-b-id card-id)
@@ -434,7 +434,7 @@
{:dashboard-a (:last_used_param_values (mt/user-http-request :rasta :get 200 (format "dashboard/%d" dashboard-a-id)))
:dashboard-b (:last_used_param_values (mt/user-http-request :rasta :get 200 (format "dashboard/%d" dashboard-b-id)))})))
(testing "If a User unsets a parameter's value, the default is NOT used."
- ;; api request mimicking a user clearing parameter value, and no default exists
+ ;; api request mimicking a user clearing parameter value, and no default exists
(is (some? (mt/user-http-request :rasta :post (format "dashboard/%d/dashcard/%s/card/%s/query" dashboard-a-id dashcard-a-id card-id)
{:parameters [{:id "a" :value nil}]})))
(is (= {}
diff --git a/test/metabase/api/public_test.clj b/test/metabase/api/public_test.clj
index ee0324f17f5..895db4a2714 100644
--- a/test/metabase/api/public_test.clj
+++ b/test/metabase/api/public_test.clj
@@ -1757,68 +1757,70 @@
;;; --------------------------------- POST /api/public/action/:uuid/execute ----------------------------------
(deftest execute-public-action-test
- (mt/with-actions-test-data-and-actions-enabled
- (mt/with-temporary-setting-values [enable-public-sharing true]
- (let [{:keys [public_uuid] :as action-opts} (shared-obj)]
- (mt/with-actions [{} action-opts]
- ;; Decrease the throttle threshold to 1 so we can test the throttle,
- ;; and set the throttle delay high enough the throttle will definitely trigger
- (with-redefs [api.public/action-execution-throttle (throttle/make-throttler :action-uuid :attempts-threshold 1 :initial-delay-ms 20000)]
- (testing "Happy path - we can execute a public action"
- (is (=? {:rows-affected 1}
- (client/client
- :post 200
- (format "public/action/%s/execute" public_uuid)
- {:parameters {:id 1 :name "European"}}))))
- (testing "Test throttle"
- (let [throttled-response (client/client-full-response
- :post 429
- (format "public/action/%s/execute" public_uuid)
- {:parameters {:id 1 :name "European"}})]
- (is (str/starts-with? (:body throttled-response) "Too many attempts!"))
- (is (contains? (:headers throttled-response) "Retry-After"))))))
- ;; Lift the throttle attempts threshold so we don't have to wait between requests
- (with-redefs [api.public/action-execution-throttle (throttle/make-throttler :action-uuid :attempts-threshold 1000)]
- (mt/with-actions [{} (assoc action-opts :archived true)]
- (testing "Check that we get a 400 if the action is archived"
- (is (= "Not found."
- (client/client
- :post 404
- (format "public/action/%s/execute" (str (random-uuid)))
- {:parameters {:id 1 :name "European"}})))))
- (mt/with-actions [{} action-opts]
- (testing "Check that we get a 400 if the action doesn't exist"
- (is (= "Not found."
- (client/client
- :post 404
- (format "public/action/%s/execute" (str (random-uuid)))
- {:parameters {:id 1 :name "European"}}))))
- (testing "Check that we get a 400 if sharing is disabled."
- (mt/with-temporary-setting-values [enable-public-sharing false]
- (is (= "An error occurred."
- (client/client
- :post 400
- (format "public/action/%s/execute" public_uuid)
- {:parameters {:id 1 :name "European"}})))))
- (testing "Check that we get a 400 if actions are disabled for the database."
- (mt/with-temp-vals-in-db Database (mt/id) {:settings {:database-enable-actions false}}
- (is (= "An error occurred."
- (client/client
- :post 400
- (format "public/action/%s/execute" public_uuid)
- {:parameters {:id 1 :name "European"}})))))
- (testing "Check that we send a snowplow event when execute an action"
- (snowplow-test/with-fake-snowplow-collector
- (client/client
- :post 200
- (format "public/action/%s/execute" public_uuid)
- {:parameters {:id 1 :name "European"}})
- (is (= {:data {"action_id" (t2/select-one-pk 'Action :public_uuid public_uuid)
- "event" "action_executed"
- "source" "public_form"
- "type" "query"}
- :user-id nil}
- (last (snowplow-test/pop-event-data-and-user-id!))))))))))))
+ (mt/with-premium-features #{:advanced-permissions}
+ (mt/with-actions-test-data-and-actions-enabled
+ (mt/with-no-data-perms-for-all-users!
+ (mt/with-temporary-setting-values [enable-public-sharing true]
+ (let [{:keys [public_uuid] :as action-opts} (shared-obj)]
+ (mt/with-actions [{} action-opts]
+ ;; Decrease the throttle threshold to 1 so we can test the throttle,
+ ;; and set the throttle delay high enough the throttle will definitely trigger
+ (with-redefs [api.public/action-execution-throttle (throttle/make-throttler :action-uuid :attempts-threshold 1 :initial-delay-ms 20000)]
+ (testing "Happy path - we can execute a public action"
+ (is (=? {:rows-affected 1}
+ (client/client
+ :post 200
+ (format "public/action/%s/execute" public_uuid)
+ {:parameters {:id 1 :name "European"}}))))
+ (testing "Test throttle"
+ (let [throttled-response (client/client-full-response
+ :post 429
+ (format "public/action/%s/execute" public_uuid)
+ {:parameters {:id 1 :name "European"}})]
+ (is (str/starts-with? (:body throttled-response) "Too many attempts!"))
+ (is (contains? (:headers throttled-response) "Retry-After"))))))
+ ;; Lift the throttle attempts threshold so we don't have to wait between requests
+ (with-redefs [api.public/action-execution-throttle (throttle/make-throttler :action-uuid :attempts-threshold 1000)]
+ (mt/with-actions [{} (assoc action-opts :archived true)]
+ (testing "Check that we get a 400 if the action is archived"
+ (is (= "Not found."
+ (client/client
+ :post 404
+ (format "public/action/%s/execute" (str (random-uuid)))
+ {:parameters {:id 1 :name "European"}})))))
+ (mt/with-actions [{} action-opts]
+ (testing "Check that we get a 400 if the action doesn't exist"
+ (is (= "Not found."
+ (client/client
+ :post 404
+ (format "public/action/%s/execute" (str (random-uuid)))
+ {:parameters {:id 1 :name "European"}}))))
+ (testing "Check that we get a 400 if sharing is disabled."
+ (mt/with-temporary-setting-values [enable-public-sharing false]
+ (is (= "An error occurred."
+ (client/client
+ :post 400
+ (format "public/action/%s/execute" public_uuid)
+ {:parameters {:id 1 :name "European"}})))))
+ (testing "Check that we get a 400 if actions are disabled for the database."
+ (mt/with-temp-vals-in-db Database (mt/id) {:settings {:database-enable-actions false}}
+ (is (= "An error occurred."
+ (client/client
+ :post 400
+ (format "public/action/%s/execute" public_uuid)
+ {:parameters {:id 1 :name "European"}})))))
+ (testing "Check that we send a snowplow event when execute an action"
+ (snowplow-test/with-fake-snowplow-collector
+ (client/client
+ :post 200
+ (format "public/action/%s/execute" public_uuid)
+ {:parameters {:id 1 :name "European"}})
+ (is (= {:data {"action_id" (t2/select-one-pk 'Action :public_uuid public_uuid)
+ "event" "action_executed"
+ "source" "public_form"
+ "type" "query"}
+ :user-id nil}
+ (last (snowplow-test/pop-event-data-and-user-id!))))))))))))))
(deftest format-export-middleware-test
(mt/with-temporary-setting-values [enable-public-sharing true]
diff --git a/test/metabase/driver/sql_jdbc/actions_test.clj b/test/metabase/driver/sql_jdbc/actions_test.clj
index 536785c72c4..631b1f45837 100644
--- a/test/metabase/driver/sql_jdbc/actions_test.clj
+++ b/test/metabase/driver/sql_jdbc/actions_test.clj
@@ -5,7 +5,6 @@
[clojure.test :refer :all]
[metabase.actions :as actions]
[metabase.actions.error :as actions.error]
- [metabase.api.common :refer [*current-user-permissions-set*]]
[metabase.driver :as driver]
[metabase.driver.sql-jdbc.actions :as sql-jdbc.actions]
[metabase.lib.schema.actions :as lib.schema.actions]
@@ -57,7 +56,7 @@
(reset! parse-sql-error-called? false)
;; attempting to delete the `Pizza` category should fail because there are several rows in `venues` that have
;; this `category_id` -- it's an FK constraint violation.
- (binding [*current-user-permissions-set* (delay #{"/"})]
+ (mt/as-admin
(is (thrown-with-msg? Exception #"Referential integrity constraint violation:.*"
(actions/perform-action! :row/delete (mt/mbql-query categories {:filter [:= $id 58]})))))
(testing "Make sure our impl was actually called."
diff --git a/test/metabase/query_processor/card_test.clj b/test/metabase/query_processor/card_test.clj
index eb1ea6676e1..a19fefce31e 100644
--- a/test/metabase/query_processor/card_test.clj
+++ b/test/metabase/query_processor/card_test.clj
@@ -3,7 +3,6 @@
(:require
[cheshire.core :as json]
[clojure.test :refer :all]
- [metabase.api.common :as api]
[metabase.models :refer [Card]]
[metabase.models.data-permissions :as data-perms]
[metabase.models.interface :as mi]
@@ -20,7 +19,7 @@
[card-id]
;; TODO -- we shouldn't do the perms checks if there is no current User context. It seems like API-level perms check
;; stuff doesn't belong in the Dashboard QP namespace
- (binding [api/*current-user-permissions-set* (atom #{"/"})]
+ (mt/as-admin
(qp.card/process-query-for-card
card-id :api
:make-run (constantly
diff --git a/test/metabase/query_processor/dashboard_test.clj b/test/metabase/query_processor/dashboard_test.clj
index 68e5d54b552..3562caa95bc 100644
--- a/test/metabase/query_processor/dashboard_test.clj
+++ b/test/metabase/query_processor/dashboard_test.clj
@@ -2,7 +2,6 @@
"There are more e2e tests in [[metabase.api.dashboard-test]]."
(:require
[clojure.test :refer :all]
- [metabase.api.common :as api]
[metabase.api.dashboard-test :as api.dashboard-test]
[metabase.models
:refer [Card Dashboard DashboardCard DashboardCardSeries]]
@@ -17,7 +16,7 @@
(defn- run-query-for-dashcard [dashboard-id card-id dashcard-id & options]
;; TODO -- we shouldn't do the perms checks if there is no current User context. It seems like API-level perms check
;; stuff doesn't belong in the Dashboard QP namespace
- (binding [api/*current-user-permissions-set* (atom #{"/"})]
+ (mt/as-admin
(apply qp.dashboard/process-query-for-dashcard
:dashboard-id dashboard-id
:card-id card-id
--
GitLab