Skip to content

Commit

Permalink
Bugfix 37914 wrong filters UI in embedding (#41342) (#41386)
Browse files Browse the repository at this point in the history 
* 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

Co-authored-by: adam-james <21064735+adam-james-v@users.noreply.github.com>
  • Loading branch information
@metabase-bot @adam-james-v
metabase-bot[bot] and adam-james-v committed Apr 12, 2024
1 parent 098f588 commit 91e625f
Show file tree
Hide file tree
Showing 2 changed files with 85 additions and 22 deletions.
61 changes: 39 additions & 22 deletions src/metabase/api/embed.clj
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -276,20 +286,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
Expand Down
46 changes: 46 additions & 0 deletions test/metabase/api/embed_test.clj
View file
Original file line number Diff line number Diff line change
Expand Up @@ -631,6 +631,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
Expand Down

0 comments on commit 91e625f

Please sign in to comment.