From f826a28fc234331761d98db7b8958c83463181e8 Mon Sep 17 00:00:00 2001
From: adam-james <21064735+adam-james-v@users.noreply.github.com>
Date: Fri, 12 Apr 2024 14:49:50 -0700
Subject: [PATCH] Bugfix 37914 wrong filters UI in embedding (#41342)
* Keep Filter Values for enabled Parameters in Embedding
Fixes: #37914
Previously, we removed locked parameters entirely. This is to prevent leaking potentially sensitive values.
However, for the situation where the same field backs 1 locked or disabled paramter and 1 enabled parameter, we do
still want to send the paramater values, because the enabled parameter implies that the values are permissible to see
in the embed.
So, this change will still remove parameters based on their 'disabled/locked/enabled' status, but will NOT remove the
linked field ids if they're also being used by 'enabled' parameters.
This results in the backend correctly sending necessary parameter values to the embed, where the frontend can then
render the appropriate UI instead of falling back to just text filters.
* test the case where a locked and enabled param share same field
* Address feedback. Added comment to explain classify fn
---
src/metabase/api/embed.clj | 61 ++++++++++++++++++++------------
test/metabase/api/embed_test.clj | 46 ++++++++++++++++++++++++
2 files changed, 85 insertions(+), 22 deletions(-)
diff --git a/src/metabase/api/embed.clj b/src/metabase/api/embed.clj
index 08b7f42f0b7..920159eb9e1 100644
--- a/src/metabase/api/embed.clj
+++ b/src/metabase/api/embed.clj
@@ -117,23 +117,33 @@
:when (not (contains? params-to-remove (keyword (:slug param))))]
param))
+(defn- classify-params-as-keep-or-remove
+ "Classifies the params in the `dashboard-or-card-params` seq and the param slugs in `embedding-params` map according to:
+ Parameters in `dashboard-or-card-params` whose slugs are NOT in the `embedding-params` map must be removed.
+ Parameter slugs in `embedding-params` with the value 'enabled' are kept, 'disabled' or 'locked' are not kept.
+
+ The resulting classification is returned as a map with keys :keep and :remove whose values are sets of parameter slugs."
+ [dashboard-or-card-params embedding-params]
+ (let [param-slugs (map #(keyword (:slug %)) dashboard-or-card-params)
+ grouped-param-slugs {:remove (remove (fn [k] (contains? embedding-params k)) param-slugs)}
+ grouped-embedding-param-slugs (-> (group-by #(= (second %) "enabled") embedding-params)
+ (update-keys {true :keep false :remove})
+ (update-vals #(into #{} (map first) %)))]
+ (merge-with (comp set concat)
+ {:keep #{} :remove #{}}
+ grouped-param-slugs
+ grouped-embedding-param-slugs)))
+
(defn- get-params-to-remove
- "Gets the params in both the provided embedding-params and dashboard-or-card object that we should remove."
- [dashboard-or-card embedding-params]
- (set (concat (for [[param status] embedding-params
- :when (not= status "enabled")]
- param)
- (for [{slug :slug} (:parameters dashboard-or-card)
- :let [param (keyword slug)]
- :when (not (contains? embedding-params param))]
- param))))
+ [dashboard-or-card-params embedding-params]
+ (:remove (classify-params-as-keep-or-remove dashboard-or-card-params embedding-params)))
(mu/defn ^:private remove-locked-and-disabled-params
"Remove the `:parameters` for `dashboard-or-card` that listed as `disabled` or `locked` in the `embedding-params`
whitelist, or not present in the whitelist. This is done so the frontend doesn't display widgets for params the user
can't set."
[dashboard-or-card embedding-params :- ms/EmbeddingParams]
- (let [params-to-remove (get-params-to-remove dashboard-or-card embedding-params)]
+ (let [params-to-remove (get-params-to-remove (:parameters dashboard-or-card) embedding-params)]
(update dashboard-or-card :parameters remove-params-in-set params-to-remove)))
(defn- remove-token-parameters
@@ -275,20 +285,27 @@
(into {})))))
(defn- remove-locked-parameters [dashboard embedding-params]
- (let [params-to-remove (get-params-to-remove dashboard embedding-params)
- param-ids-to-remove (set (for [parameter (:parameters dashboard)
- :when (contains? params-to-remove (keyword (:slug parameter)))]
- (:id parameter)))
- linked-field-ids (set (mapcat (params/get-linked-field-ids (:dashcards dashboard)) param-ids-to-remove))
- remove-parameters (fn [dashcard]
- (update dashcard :parameter_mappings
- (fn [param-mappings]
- (remove (fn [{:keys [parameter_id]}]
- (contains? param-ids-to-remove parameter_id)) param-mappings))))]
+ (let [params (:parameters dashboard)
+ {params-to-remove :remove
+ params-to-keep :keep} (classify-params-as-keep-or-remove params embedding-params)
+ param-ids-to-remove (set (keep (fn [{:keys [slug id]}]
+ (when (contains? params-to-remove (keyword slug)) id))
+ params))
+ param-ids-to-keep (set (keep (fn [{:keys [slug id]}]
+ (when (contains? params-to-keep (keyword slug)) id))
+ params))
+ field-ids-to-maybe-remove (set (mapcat (params/get-linked-field-ids (:dashcards dashboard)) param-ids-to-remove))
+ field-ids-to-keep (set (mapcat (params/get-linked-field-ids (:dashcards dashboard)) param-ids-to-keep))
+ field-ids-to-remove (set/difference field-ids-to-maybe-remove field-ids-to-keep)
+ remove-parameters (fn [dashcard]
+ (update dashcard :parameter_mappings
+ (fn [param-mappings]
+ (remove (fn [{:keys [parameter_id]}]
+ (contains? param-ids-to-remove parameter_id)) param-mappings))))]
(-> dashboard
(update :dashcards #(map remove-parameters %))
- (update :param_fields #(apply dissoc % linked-field-ids))
- (update :param_values #(apply dissoc % linked-field-ids)))))
+ (update :param_fields #(apply dissoc % field-ids-to-remove))
+ (update :param_values #(apply dissoc % field-ids-to-remove)))))
(defn dashboard-for-unsigned-token
"Return the info needed for embedding about Dashboard specified in `token`. Additional `constraints` can be passed to
diff --git a/test/metabase/api/embed_test.clj b/test/metabase/api/embed_test.clj
index bf40e09c8cf..ac2c618e25d 100644
--- a/test/metabase/api/embed_test.clj
+++ b/test/metabase/api/embed_test.clj
@@ -651,6 +651,52 @@
:parameter_mappings
count))))))))
+(deftest locked-params-removes-values-fields-when-not-used-in-enabled-params
+ (testing "check that locked params are not removed in parameter mappings, param_values, and param_fields when an enabled param uses them (#37914)"
+ (with-embedding-enabled-and-new-secret-key
+ (t2.with-temp/with-temp [Dashboard dashboard {:enable_embedding true
+ :embedding_params {:venue_name "locked"
+ :venue_name_2 "enabled"}
+ :name "Test Dashboard"
+ :parameters [{:name "venue_name"
+ :slug "venue_name"
+ :id "foo"
+ :type :string/=
+ :sectionId "string"}
+ {:name "venue_name_2"
+ :slug "venue_name_2"
+ :id "bar"
+ :type :string/=
+ :sectionId "string"}]}
+ Card {card-id :id} {:name "Dashboard Test Card"}
+ DashboardCard {_ :id} {:dashboard_id (:id dashboard)
+ :card_id card-id
+ :parameter_mappings [{:card_id card-id
+ :slug "venue_name"
+ :parameter_id "foo"
+ :target [:dimension
+ [:field (mt/id :venues :name) nil]]}
+ {:card_id card-id
+ :slug "venue_name_2"
+ :parameter_id "bar"
+ :target [:dimension
+ [:field (mt/id :venues :name) nil]]}]}]
+ (let [embedding-dashboard (client/client :get 200 (dashboard-url dashboard {:params {:foo "BCD Tofu House"}}))]
+ (is (some?
+ (-> embedding-dashboard
+ :param_values
+ (get (mt/id :venues :name)))))
+ (is (some?
+ (-> embedding-dashboard
+ :param_fields
+ (get (mt/id :venues :name)))))
+ (is (= 1
+ (-> embedding-dashboard
+ :dashcards
+ first
+ :parameter_mappings
+ count))))))))
+
(deftest linked-param-to-locked-removes-param-values-test
(testing "Check that a linked parameter to a locked params we remove the param_values."
(with-embedding-enabled-and-new-secret-key
--
GitLab