Bugfix 37914 wrong filters UI in embedding (#41342) (#41386)

* 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>

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
@@ -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

test/metabase/api/embed_test.clj

@@ -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