Skip to content

Commit

Permalink
Parameter Values that are used in 'enabled' params are not removed
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@adam-james-v
adam-james-v committed Apr 11, 2024
1 parent d0fc8ad commit a7fcb1a
Show file tree
Hide file tree
Showing 2 changed files with 34 additions and 24 deletions.
55 changes: 33 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,28 @@
:when (not (contains? params-to-remove (keyword (:slug param))))]
param))

(defn- classify-params-as-keep-or-remove
[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 @@ -275,20 +280,26 @@
(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-to-remove :remove
params-to-keep :keep} (classify-params-as-keep-or-remove (:parameters dashboard) embedding-params)
param-ids-to-remove (set (for [parameter (:parameters dashboard)
:when (contains? params-to-remove (keyword (:slug parameter)))]
(:id parameter)))
param-ids-to-keep (set (for [parameter (:parameters dashboard)
:when (contains? params-to-keep (keyword (:slug parameter)))]
(:id parameter)))
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
3 changes: 1 addition & 2 deletions test/metabase/api/embed_test.clj
View file
Original file line number Diff line number Diff line change
Expand Up @@ -616,8 +616,7 @@
(testing "check that locked params are removed in parameter mappings, param_values, and param_fields"
(with-embedding-enabled-and-new-secret-key
(t2.with-temp/with-temp [Dashboard dashboard {:enable_embedding true
:embedding_params {:venue_name "locked"}
:name "Test Dashboard"
:embedding_params {:venue_name "locked"} :name "Test Dashboard"
:parameters [{:name "venue_name"
:slug "venue_name"
:id "foo"
Expand Down

0 comments on commit a7fcb1a

Please sign in to comment.