From ed3f3ae7567a2a343cffcc6fd59aeee95660cfee Mon Sep 17 00:00:00 2001
From: "metabase-bot[bot]"
<109303359+metabase-bot[bot]@users.noreply.github.com>
Date: Wed, 1 Mar 2023 08:05:09 +0100
Subject: [PATCH] Make chain filters viewable when collection permissions allow
it (#28581) (#28766)
---
.../sandbox/api/dashboard_test.clj | 24 +++++-----
...ta-should-use-dashboard-filters.cy.spec.js | 32 +++++++++----
.../dashboard_data_permissions.cy.spec.js | 18 +++++++-
src/metabase/api/dashboard.clj | 33 +++++++-------
src/metabase/api/field.clj | 3 +-
src/metabase/models/params/custom_values.clj | 8 ++--
.../middleware/permissions.clj | 20 +++++++--
test/metabase/api/dashboard_test.clj | 45 ++++++++++++++++---
8 files changed, 133 insertions(+), 50 deletions(-)
diff --git a/enterprise/backend/test/metabase_enterprise/sandbox/api/dashboard_test.clj b/enterprise/backend/test/metabase_enterprise/sandbox/api/dashboard_test.clj
index 6852cb2de51..d6bf784eb12 100644
--- a/enterprise/backend/test/metabase_enterprise/sandbox/api/dashboard_test.clj
+++ b/enterprise/backend/test/metabase_enterprise/sandbox/api/dashboard_test.clj
@@ -5,6 +5,7 @@
[metabase-enterprise.test :as met]
[metabase.api.dashboard-test :as api.dashboard-test]
[metabase.models :refer [Card Dashboard DashboardCard FieldValues]]
+ [metabase.models.params.chain-filter]
[metabase.models.params.chain-filter-test :as chain-filter-test]
[metabase.models.permissions :as perms]
[metabase.models.permissions-group :as perms-group]
@@ -18,16 +19,19 @@
(met/with-gtaps {:gtaps {:categories {:query (mt/mbql-query categories {:filter [:< $id 3]})}}}
(mt/with-temp-vals-in-db FieldValues (u/the-id (db/select-one-id FieldValues :field_id (mt/id :categories :name))) {:values ["Good" "Bad"]}
(api.dashboard-test/with-chain-filter-fixtures [{:keys [dashboard]}]
- (testing "GET /api/dashboard/:id/params/:param-key/values"
- (api.dashboard-test/let-url [url (api.dashboard-test/chain-filter-values-url dashboard "_CATEGORY_NAME_")]
- (is (= {:values ["African" "American"]
- :has_more_values false}
- (chain-filter-test/take-n-values 2 (mt/user-http-request :rasta :get 200 url))))))
- (testing "GET /api/dashboard/:id/params/:param-key/search/:query"
- (api.dashboard-test/let-url [url (api.dashboard-test/chain-filter-search-url dashboard "_CATEGORY_NAME_" "a")]
- (is (= {:values ["African" "American"]
- :has_more_values false}
- (mt/user-http-request :rasta :get 200 url))))))))))
+ (with-redefs [metabase.models.params.chain-filter/use-cached-field-values? (constantly false)]
+ (testing "GET /api/dashboard/:id/params/:param-key/values"
+ (api.dashboard-test/let-url [url (api.dashboard-test/chain-filter-values-url dashboard "_CATEGORY_NAME_")]
+ (is (= {:values ["African" "American"]
+ :has_more_values false}
+ (->> url
+ (mt/user-http-request :rasta :get 200)
+ (chain-filter-test/take-n-values 2))))))
+ (testing "GET /api/dashboard/:id/params/:param-key/search/:query"
+ (api.dashboard-test/let-url [url (api.dashboard-test/chain-filter-search-url dashboard "_CATEGORY_NAME_" "a")]
+ (is (= {:values ["African" "American"]
+ :has_more_values false}
+ (mt/user-http-request :rasta :get 200 url)))))))))))
(deftest add-card-parameter-mapping-permissions-test
(testing "POST /api/dashboard/:id/cards"
diff --git a/frontend/test/metabase/scenarios/dashboard-filters/reproductions/16112-nodata-should-use-dashboard-filters.cy.spec.js b/frontend/test/metabase/scenarios/dashboard-filters/reproductions/16112-nodata-should-use-dashboard-filters.cy.spec.js
index 0890d5659b2..333f0e0f966 100644
--- a/frontend/test/metabase/scenarios/dashboard-filters/reproductions/16112-nodata-should-use-dashboard-filters.cy.spec.js
+++ b/frontend/test/metabase/scenarios/dashboard-filters/reproductions/16112-nodata-should-use-dashboard-filters.cy.spec.js
@@ -72,23 +72,39 @@ describe("issues 15119 and 16112", () => {
],
});
+ // Actually need to setup the linked filter:
+ visitDashboard(dashboard_id);
+ cy.get('[data-metabase-event="Dashboard;Edit"]').click();
+ cy.findByText("Rating Filter").click();
+ cy.findByText("Linked filters").click();
+ // cy.findByText("Reviewer Filter").click();
+ cy.findByText("Limit this filter's choices")
+ .parent()
+ .within(() => {
+ // turn on the toggle
+ cy.get("input").click();
+ });
+ cy.findByText("Save").click();
+
cy.signIn("nodata");
visitDashboard(dashboard_id);
},
);
- cy.findByText(ratingFilter.name).click();
- popover().contains("3").click();
+ cy.findByText(reviewerFilter.name).click();
+ popover().contains("adam").click();
cy.button("Add filter").click();
- cy.get(".DashCard").should("contain", "maia").and("contain", "daryl");
- cy.location("search").should("eq", "?rating=3");
+ cy.get(".DashCard").should("contain", "adam");
+ cy.location("search").should("eq", "?reviewer=adam");
- cy.findByText(reviewerFilter.name).click();
- cy.findByPlaceholderText("Enter some text").type("maia{enter}").blur();
+ cy.findByText(ratingFilter.name).click();
+
+ popover().contains("5").click();
cy.button("Add filter").click();
- cy.get(".DashCard").should("contain", "maia").and("not.contain", "daryl");
- cy.location("search").should("eq", "?reviewer=maia&rating=3");
+ cy.get(".DashCard").should("contain", "adam");
+ cy.get(".DashCard").should("contain", "5");
+ cy.location("search").should("eq", "?reviewer=adam&rating=5");
});
});
diff --git a/frontend/test/metabase/scenarios/dashboard/dashboard_data_permissions.cy.spec.js b/frontend/test/metabase/scenarios/dashboard/dashboard_data_permissions.cy.spec.js
index 11b9e4c2db2..07de30a9108 100644
--- a/frontend/test/metabase/scenarios/dashboard/dashboard_data_permissions.cy.spec.js
+++ b/frontend/test/metabase/scenarios/dashboard/dashboard_data_permissions.cy.spec.js
@@ -58,6 +58,22 @@ describe("support > permissions (metabase#8472)", () => {
it("should allow a nodata user to select the filter", () => {
cy.signIn("nodata");
- filterDashboard(false);
+ filterDashboard();
+ });
+
+ it("should not allow a nocollection user to visit the page, hence cannot see the filter", () => {
+ cy.server();
+ cy.route("GET", "/api/dashboard/1/params/search/100 Main Street").as(
+ "search",
+ );
+
+ cy.signIn("nocollection");
+ cy.request({
+ method: "GET",
+ url: "/api/dashboard/1",
+ failOnStatusCode: false,
+ }).should(xhr => {
+ expect(xhr.status).to.equal(403);
+ });
});
});
diff --git a/src/metabase/api/dashboard.clj b/src/metabase/api/dashboard.clj
index bf751f2278c..4a36918fba4 100644
--- a/src/metabase/api/dashboard.clj
+++ b/src/metabase/api/dashboard.clj
@@ -31,12 +31,15 @@
[metabase.query-processor.dashboard :as qp.dashboard]
[metabase.query-processor.error-type :as qp.error-type]
[metabase.query-processor.middleware.constraints :as qp.constraints]
+ [metabase.query-processor.middleware.permissions :as qp.perms]
[metabase.query-processor.pivot :as qp.pivot]
[metabase.query-processor.util :as qp.util]
[metabase.related :as related]
[metabase.util :as u]
[metabase.util.i18n :refer [tru]]
[metabase.util.log :as log]
+ [metabase.util.malli :as mu]
+ [metabase.util.malli.schema :as ms]
[metabase.util.schema :as su]
[schema.core :as s]
[toucan.db :as db]
@@ -716,25 +719,17 @@
field-id (param-key->field-ids dashboard param-key)]
[field-id value])))
-(s/defn chain-filter
+(mu/defn chain-filter
"C H A I N filters!
- ;; show me categories
- (chain-filter 62 \"ee876336\" {})
- ;; -> {:values (\"African\" \"American\" \"Artisan\" ...)
- :has_more_values false}
-
- ;; show me categories that have expensive restaurants
- (chain-filter 62 \"ee876336\" {\"6f10a41f\" 4})
- ;; -> {:values (\"Japanese\" \"Steakhouse\")
- :has_more_values false}"
+ Used to query for values that populate chained filter dropdowns and text search boxes."
([dashboard param-key constraint-param-key->value]
(chain-filter dashboard param-key constraint-param-key->value nil))
- ([dashboard :- su/Map
- param-key :- su/NonBlankString
- constraint-param-key->value :- su/Map
- query :- (s/maybe su/NonBlankString)]
+ ([dashboard :- ms/Map
+ param-key :- ms/NonBlankString
+ constraint-param-key->value :- ms/Map
+ query :- [:maybe ms/NonBlankString]]
(let [constraints (chain-filter-constraints dashboard constraint-param-key->value)
field-ids (param-key->field-ids dashboard param-key)]
(when (empty? field-ids)
@@ -750,7 +745,7 @@
field-ids)
values (distinct (mapcat :values results))
has_more_values (boolean (some true? (map :has_more_values results)))]
- ;; results can come back as [v ...] *or* as [[orig remapped] ...]. Sort by remapped value if that's the case
+ ;; results can come back as [v ...] *or* as [[orig remapped] ...]. Sort by remapped value if it's there
{:values (if (sequential? (first values))
(sort-by second values)
(sort values))
@@ -791,7 +786,9 @@
GET /api/dashboard/1/params/abc/values?def=100"
[id param-key :as {:keys [query-params]}]
(let [dashboard (api/read-check Dashboard id)]
- (param-values dashboard param-key query-params)))
+ ;; If a user can read the dashboard, then they can lookup filters. This also works with sandboxing.
+ (binding [qp.perms/*param-values-query* true]
+ (param-values dashboard param-key query-params))))
#_{:clj-kondo/ignore [:deprecated-var]}
(api/defendpoint-schema GET "/:id/params/:param-key/search/:query"
@@ -805,7 +802,9 @@
Currently limited to first 1000 results."
[id param-key query :as {:keys [query-params]}]
(let [dashboard (api/read-check Dashboard id)]
- (param-values dashboard param-key query-params query)))
+ ;; If a user can read the dashboard, then they can lookup filters. This also works with sandboxing.
+ (binding [qp.perms/*param-values-query* true]
+ (param-values dashboard param-key query-params query))))
#_{:clj-kondo/ignore [:deprecated-var]}
(api/defendpoint-schema GET "/params/valid-filter-fields"
diff --git a/src/metabase/api/field.clj b/src/metabase/api/field.clj
index 967c162abe7..45178969c52 100644
--- a/src/metabase/api/field.clj
+++ b/src/metabase/api/field.clj
@@ -247,8 +247,7 @@
:field_id field-id})))
;; TODO -- not sure `has_field_values` actually has to be `:list` -- see code above.
-#_{:clj-kondo/ignore [:deprecated-var]}
-(api/defendpoint-schema GET "/:id/values"
+(api/defendpoint GET "/:id/values"
"If a Field's value of `has_field_values` is `:list`, return a list of all the distinct values of the Field, and (if
defined by a User) a map of human-readable remapped values."
[id]
diff --git a/src/metabase/models/params/custom_values.clj b/src/metabase/models/params/custom_values.clj
index 90f9c9a0126..99348baa9fc 100644
--- a/src/metabase/models/params/custom_values.clj
+++ b/src/metabase/models/params/custom_values.clj
@@ -116,10 +116,10 @@
(defn parameter->values
"Given a parameter with a custom-values source, return the values.
- `default-case-fn` is a 0-arity function that returns values list when:
+ `default-case-thunk` is a 0-arity function that returns values list when:
- :values_source_type = card but the card is archived or the card no longer contains the value-field.
- :values_source_type = nil."
- [parameter query default-case-fn]
+ [parameter query default-case-thunk]
(case (:values_source_type parameter)
"static-list" (static-list-values parameter query)
"card" (let [card (db/select-one Card :id (get-in parameter [:values_source_config :card_id]))]
@@ -127,8 +127,8 @@
(throw (ex-info "You don't have permissions to do that." {:status-code 403})))
(if (can-get-card-values? card (get-in parameter [:values_source_config :value_field]))
(card-values parameter query)
- (default-case-fn)))
- nil (default-case-fn)
+ (default-case-thunk)))
+ nil (default-case-thunk)
(throw (ex-info (tru "Invalid parameter source {0}" (:values_source_type parameter))
{:status-code 400
:parameter parameter}))))
diff --git a/src/metabase/query_processor/middleware/permissions.clj b/src/metabase/query_processor/middleware/permissions.clj
index 18da7350991..08ce2aaaf25 100644
--- a/src/metabase/query_processor/middleware/permissions.clj
+++ b/src/metabase/query_processor/middleware/permissions.clj
@@ -15,6 +15,7 @@
[metabase.util :as u]
[metabase.util.i18n :refer [tru]]
[metabase.util.log :as log]
+ [metabase.util.malli :as mu]
[metabase.util.schema :as su]
[schema.core :as s]
[toucan.db :as db]))
@@ -89,16 +90,29 @@
(doseq [{query :dataset_query} (qp.u.tag-referenced-cards/tags-referenced-cards outer-query)]
(check-query-permissions* query)))
-(s/defn ^:private check-query-permissions*
+(def ^:dynamic *param-values-query*
+ "Used to allow users looking at a dashboard to view (possibly chained) filters."
+ false)
+
+(mu/defn ^:private check-query-permissions*
"Check that User with `user-id` has permissions to run `query`, or throw an exception."
- [outer-query :- su/Map]
+ [outer-query :- :map]
(when *current-user-id*
(log/tracef "Checking query permissions. Current user perms set = %s" (pr-str @*current-user-permissions-set*))
- (if *card-id*
+ (cond
+ *card-id*
(do
(check-card-read-perms *card-id*)
(when-not (has-data-perms? (required-perms outer-query))
(check-block-permissions outer-query)))
+
+ ;; set when querying for field values of dashboard filters, which only require
+ ;; collection perms for the dashboard and not ad-hoc query perms
+ *param-values-query*
+ (when-not (has-data-perms? (required-perms outer-query))
+ (check-block-permissions outer-query))
+
+ :else
(check-ad-hoc-query-perms outer-query))))
(defn check-query-permissions
diff --git a/test/metabase/api/dashboard_test.clj b/test/metabase/api/dashboard_test.clj
index 7930a3b2755..cd396c39e38 100644
--- a/test/metabase/api/dashboard_test.clj
+++ b/test/metabase/api/dashboard_test.clj
@@ -31,6 +31,7 @@
[metabase.models.dashboard-test :as dashboard-test]
[metabase.models.field-values :as field-values]
[metabase.models.interface :as mi]
+ [metabase.models.params.chain-filter :as chain-filter]
[metabase.models.params.chain-filter-test :as chain-filter-test]
[metabase.models.permissions :as perms]
[metabase.models.permissions-group :as perms-group]
@@ -39,6 +40,7 @@
[metabase.public-settings.premium-features-test
:as premium-features-test]
[metabase.query-processor :as qp]
+ [metabase.query-processor.middleware.permissions :as qp.perms]
[metabase.query-processor.streaming.test-util :as streaming.test-util]
[metabase.server.middleware.util :as mw.util]
[metabase.test :as mt]
@@ -1098,6 +1100,7 @@
(deftest update-cards-for-copy-test
(testing "When copy style is shallow returns original ordered-cards"
+
(let [ordered-cards [{:card_id 1 :card {:id 1} :series [{:id 2}]}
{:card_id 3 :card {:id 3}}]]
(is (= ordered-cards
@@ -1941,6 +1944,24 @@
query)
query-params))
+(deftest dashboard-chain-filter-permissions-test
+ (with-chain-filter-fixtures [{:keys [dashboard param-keys]}]
+ (let [url (chain-filter-values-url dashboard (:category-name param-keys))]
+ (testing (str "\nGET /api/" url "\n")
+ (testing "\nShow me names of categories that have expensive venues (price = 4), while I lack permisisons."
+ (with-redefs [metabase.models.params.chain-filter/use-cached-field-values? (constantly false)]
+ (binding [qp.perms/*card-id* nil] ;; this situation was observed when running constrained chain filters.
+ (is (= {:values ["African" "American" "Artisan" "Asian"] :has_more_values false}
+ (chain-filter-test/take-n-values 4 (mt/user-http-request :rasta :get 200 url)))))))))
+
+ (let [url (chain-filter-values-url dashboard (:category-name param-keys) (:price param-keys) 4)]
+ (testing (str "\nGET /api/" url "\n")
+ (testing "\nShow me names of categories that have expensive venues (price = 4), while I lack permisisons."
+ (with-redefs [chain-filter/use-cached-field-values? (constantly false)]
+ (binding [qp.perms/*card-id* nil]
+ (is (= {:values ["Japanese" "Steakhouse"], :has_more_values false}
+ (chain-filter-test/take-n-values 3 (mt/user-http-request :rasta :get 200 url)))))))))))
+
(deftest dashboard-chain-filter-test
(testing "GET /api/dashboard/:id/params/:param-key/values"
(with-chain-filter-fixtures [{:keys [dashboard param-keys]}]
@@ -1983,19 +2004,33 @@
(assoc :card_id (:id card-2)))]]
(is (= {:values ["African" "American" "Artisan"]
:has_more_values false}
- (chain-filter-test/take-n-values 3 (mt/user-http-request :rasta :get 200 (chain-filter-values-url
- (:id dashboard)
- (:category-name param-keys)))))))))
+ (->> (chain-filter-values-url (:id dashboard) (:category-name param-keys))
+ (mt/user-http-request :rasta :get 200)
+ (chain-filter-test/take-n-values 3)))))))
+
(testing "should check perms for the Fields in question"
(mt/with-temp-copy-of-db
(with-chain-filter-fixtures [{:keys [dashboard param-keys]}]
(perms/revoke-data-perms! (perms-group/all-users) (mt/id))
+
;; HACK: we currently 403 on chain-filter calls that require running a MBQL
;; but 200 on calls that we could just use the cache.
;; It's not ideal and we definitely need to have a consistent behavior
(with-redefs [field-values/field-should-have-field-values? (fn [_] false)]
- (is (= "You don't have permissions to do that."
- (mt/user-http-request :rasta :get 403 (chain-filter-values-url (:id dashboard) (:category-name param-keys)))))))))))
+ (is (= {:values ["African" "American" "Artisan"]
+ :has_more_values false}
+ (->> (chain-filter-values-url (:id dashboard) (:category-name param-keys))
+ (mt/user-http-request :rasta :get 200)
+ (chain-filter-test/take-n-values 3))))))))
+
+ (testing "missing data perms should not affect perms for the Fields in question when users have collection access"
+ (mt/with-temp-copy-of-db
+ (with-chain-filter-fixtures [{:keys [dashboard param-keys]}]
+ (perms/revoke-data-perms! (perms-group/all-users) (mt/id))
+ (is (= {:values ["African" "American" "Artisan"] :has_more_values false}
+ (->> (chain-filter-values-url (:id dashboard) (:category-name param-keys))
+ (mt/user-http-request :rasta :get 200)
+ (chain-filter-test/take-n-values 3)))))))))
(deftest dashboard-with-static-list-parameters-test
(testing "A dashboard that has parameters that has static values"
--
GitLab