diff --git a/src/metabase/models/params/chain_filter.clj b/src/metabase/models/params/chain_filter.clj
index 42bcd9e991015742fbab6082cf6f176ac7f8cd18..13c09d0c3efa3b4eabdf7784766a27f973fa3231 100644
--- a/src/metabase/models/params/chain_filter.clj
+++ b/src/metabase/models/params/chain_filter.clj
@@ -79,6 +79,7 @@
[metabase.models.params.field-values :as params.field-values]
[metabase.models.table :as table]
[metabase.query-processor :as qp]
+ [metabase.query-processor.middleware.permissions :as qp.perms]
[metabase.types :as types]
[metabase.util :as u]
[metabase.util.i18n :refer [tru]]
@@ -517,6 +518,13 @@
field-id
(field-values/field-should-have-field-values? field-id)))
+(defn- check-field-value-query-permissions
+ "Check query permissions against the chain-filter-mbql-query (private #196)"
+ [field-id constraints options]
+ (->> (chain-filter-mbql-query field-id constraints options)
+ qp/preprocess
+ qp.perms/check-query-permissions*))
+
(defn- cached-field-values [field-id constraints {:keys [limit]}]
;; TODO: why don't we remap the human readable values here?
(let [{:keys [values has_more_values]}
@@ -561,7 +569,9 @@
(update :values add-human-readable-values v->human-readable))
(and (use-cached-field-values? field-id) (nil? @the-remapped-field-id))
- (cached-field-values field-id constraints options)
+ (do
+ (check-field-value-query-permissions field-id constraints options)
+ (cached-field-values field-id constraints options))
;; This is Field->Field remapping e.g. `venue.category_id `-> `category.name `;
;; search by `category.name` but return tuples of `[venue.category_id category.name]`.
diff --git a/src/metabase/query_processor/middleware/permissions.clj b/src/metabase/query_processor/middleware/permissions.clj
index fa5c49da2f2cf247cc9d6a44ce140acb26ec46da..20d712e5270b35febc71a89b57d1be1554601859 100644
--- a/src/metabase/query_processor/middleware/permissions.clj
+++ b/src/metabase/query_processor/middleware/permissions.clj
@@ -106,7 +106,7 @@
(throw (ex-info (tru "Querying this database requires the audit-app feature flag")
query)))
-(mu/defn ^:private check-query-permissions*
+(mu/defn check-query-permissions*
"Check that User with `user-id` has permissions to run `query`, or throw an exception."
[{database-id :database, :as outer-query} :- [:map [:database ::lib.schema.id/database]]]
(when *current-user-id*
diff --git a/test/metabase/api/dashboard_test.clj b/test/metabase/api/dashboard_test.clj
index 3093d86a33977296f776252061dd0dd149ff8a43..61eef165e3819fba3f28208bdec93b5f8dafb455 100644
--- a/test/metabase/api/dashboard_test.clj
+++ b/test/metabase/api/dashboard_test.clj
@@ -2814,7 +2814,7 @@
;; 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)]
+ (with-redefs [chain-filter/use-cached-field-values? (fn [_] false)]
(is (= {:values [["African"] ["American"] ["Artisan"]]
:has_more_values false}
(->> (chain-filter-values-url (:id dashboard) (:category-name param-keys))
@@ -2831,6 +2831,19 @@
(mt/user-http-request :rasta :get 200)
(chain-filter-test/take-n-values 3)))))))))
+(deftest block-data-should-not-expose-field-values
+ (testing "block data perms should not allow access to field values (private#196)"
+ (when config/ee-available?
+ (mt/with-premium-features #{:advanced-permissions}
+ (mt/with-temp-copy-of-db
+ (with-chain-filter-fixtures [{:keys [dashboard param-keys]}]
+ (perms/revoke-data-perms! (perms-group/all-users) (mt/id))
+ (perms/update-data-perms-graph! [(:id (perms-group/all-users)) (mt/id) :data]
+ {:schemas :block})
+ (is (= "You don't have permissions to do that."
+ (->> (chain-filter-values-url (:id dashboard) (:category-name param-keys))
+ (mt/user-http-request :rasta :get 403))))))))))
+
(deftest dashboard-with-static-list-parameters-test
(testing "A dashboard that has parameters that has static values"
(with-chain-filter-fixtures [{:keys [dashboard param-keys]}]
@@ -4048,3 +4061,31 @@
(testing "We have values and they all match the expression concatenation"
(is (pos? (count values)))
(is (every? (partial re-matches #"[^🦜]+🦜🦜🦜[^🦜]+") (map first values))))))))))
+
+(deftest param-values-permissions-test
+ (testing "Users without permissions should not see all options in a dashboard filter (private#196)"
+ (when config/ee-available?
+ (mt/with-premium-features #{:advanced-permissions}
+ (mt/with-temp-copy-of-db
+ (with-chain-filter-fixtures [{:keys [dashboard param-keys]}]
+ (testing "Return values with access"
+ (is (=? {:values (comp #(contains? % ["African"]) set)}
+ (mt/user-http-request :rasta :get 200
+ (str "dashboard/" (:id dashboard) "/params/" (:category-name param-keys) "/values")))))
+ (testing "Return values with no self-service (#26874)"
+ (perms/revoke-data-perms! (perms-group/all-users) (mt/id))
+ (is (=? {:values (comp #(contains? % ["African"]) set)}
+ (mt/user-http-request :rasta :get 200
+ (str "dashboard/" (:id dashboard) "/params/" (:category-name param-keys) "/values")))))
+ (testing "Return values for admin"
+ (perms/update-data-perms-graph! [(:id (perms-group/all-users)) (mt/id) :data]
+ {:schemas :block})
+ (is (=? {:values (comp #(contains? % ["African"]) set)}
+ (mt/user-http-request :crowberto :get 200
+ (str "dashboard/" (:id dashboard) "/params/" (:category-name param-keys) "/values")))))
+ (testing "Don't return with block perms."
+ (perms/update-data-perms-graph! [(:id (perms-group/all-users)) (mt/id) :data]
+ {:schemas :block})
+ (is (= "You don't have permissions to do that."
+ (mt/user-http-request :rasta :get 403
+ (str "dashboard/" (:id dashboard) "/params/" (:category-name param-keys) "/values")))))))))))