Explicitly convert user-specified query params keys to keywords

project.clj

@@ -152,7 +152,8 @@
                    :aot [metabase.logger]}                            ; Log appender class needs to be compiled for log4j to use it
              :ci {:jvm-opts ["-Xmx3g"]}
              :reflection-warnings {:global-vars {*warn-on-reflection* true}} ; run `lein check-reflection-warnings` to check for reflection warnings
-             :expectations {:injections [(require 'metabase.test-setup)]
+             :expectations {:injections [(require 'metabase.test-setup  ; for test setup stuff
+                                                  'metabase.test.util)] ; for the toucan.util.test default values for temp models
                             :resource-paths ["test_resources"]
                             :env {:mb-test-setting-1 "ABCDEFG"
                                   :mb-run-mode "test"}

src/metabase/api/embed.clj

@@ -83,12 +83,12 @@
        (or (not (string? v))
            (not (str/blank? v)))))
 
-(s/defn ^:private validate-params
+(s/defn ^:private validate-and-merge-params :- {s/Keyword s/Any}
   "Validate that the TOKEN-PARAMS passed in the JWT and the USER-PARAMS (passed as part of the URL) are allowed, and
   that ones that are required are specified by checking them against a Card or Dashboard's OBJECT-EMBEDDING-PARAMS
   (the object's value of `:embedding_params`). Throws a 400 if any of the checks fail. If all checks are successful,
-  returns a merged parameters map."
-  [object-embedding-params :- su/EmbeddingParams, token-params user-params]
+  returns a *merged* parameters map."
+  [object-embedding-params :- su/EmbeddingParams, token-params :- {s/Keyword s/Any}, user-params :- {s/Keyword s/Any}]
   (validate-param-sets object-embedding-params
                        (set (keys (m/filter-vals valid-param? token-params)))
                        (set (keys (m/filter-vals valid-param? user-params))))
@@ -177,6 +177,14 @@
           :when         param]
       (assoc param :target (:target param-mapping)))))
 
+(s/defn ^:private normalize-query-params :- {s/Keyword s/Any}
+  "Take a map of `query-params` and make sure they're in the right format for the rest of our code. Our
+  `wrap-keyword-params` middleware normally converts all query params keys to keywords, but only if they seem like
+  ones that make sense as keywords. Some params, such as ones that start with a number, do not pass this test, and are
+  not automatically converted. Thus we must do it ourselves here to make sure things are done as we'd expect."
+  [query-params]
+  (m/map-keys keyword query-params))
+
 
 ;;; ---------------------------- Card Fns used by both /api/embed and /api/preview_embed -----------------------------
 
@@ -199,7 +207,7 @@
   {:style/indent 0}
   [& {:keys [card-id embedding-params token-params query-params options]}]
   {:pre [(integer? card-id) (u/maybe? map? embedding-params) (map? token-params) (map? query-params)]}
-  (let [parameter-values (validate-params embedding-params token-params query-params)
+  (let [parameter-values (validate-and-merge-params embedding-params token-params (normalize-query-params query-params))
         parameters       (apply-parameter-values (resolve-card-parameters card-id) parameter-values)]
     (apply public-api/run-query-for-card-with-id card-id parameters, :context :embedded-question, options)))
 
@@ -224,8 +232,9 @@
   [& {:keys [dashboard-id dashcard-id card-id embedding-params token-params query-params]}]
   {:pre [(integer? dashboard-id) (integer? dashcard-id) (integer? card-id) (u/maybe? map? embedding-params)
          (map? token-params) (map? query-params)]}
-  (let [parameter-values (validate-params embedding-params token-params query-params)
-        parameters       (apply-parameter-values (resolve-dashboard-parameters dashboard-id dashcard-id card-id) parameter-values)]
+  (let [parameter-values (validate-and-merge-params embedding-params token-params (normalize-query-params query-params))
+        parameters       (apply-parameter-values (resolve-dashboard-parameters dashboard-id dashcard-id card-id)
+                                                 parameter-values)]
     (public-api/public-dashcard-results dashboard-id card-id parameters, :context :embedded-dashboard)))
 
 

src/metabase/core.clj

@@ -42,6 +42,8 @@
 
 ;;; CONFIG
 
+;; TODO - why not just put this in `metabase.middleware` with *all* of our other custom middleware. Also, what's the
+;; difference between this and `streaming-json-response`?
 (defn- streamed-json-response
   "Write `RESPONSE-SEQ` to a PipedOutputStream as JSON, returning the connected PipedInputStream"
   [response-seq options]

test/metabase/api/preview_embed_test.clj

@@ -2,14 +2,16 @@
   (:require [expectations :refer :all]
             [metabase.api.embed-test :as embed-test]
             [metabase.models.dashboard :refer [Dashboard]]
+            [metabase.test.data :as data]
             [metabase.test.data.users :as test-users]
             [metabase.test.util :as tu]
             [metabase.util :as u]
             [toucan.util.test :as tt]))
 
-;;; ------------------------------------------------------------ GET /api/preview_embed/card/:token ------------------------------------------------------------
+;;; --------------------------------------- GET /api/preview_embed/card/:token ---------------------------------------
 
-(defn- card-url [card & [additional-token-params]] (str "preview_embed/card/" (embed-test/card-token card (merge {:_embedding_params {}} additional-token-params))))
+(defn- card-url [card & [additional-token-params]]
+  (str "preview_embed/card/" (embed-test/card-token card (merge {:_embedding_params {}} additional-token-params))))
 
 ;; it should be possible to use this endpoint successfully if all the conditions are met
 (expect
@@ -45,15 +47,20 @@
 (expect
   [{:id nil, :type "date/single", :target ["variable" ["template-tag" "d"]], :name "d", :slug "d", :default nil}]
   (embed-test/with-embedding-enabled-and-new-secret-key
-    (embed-test/with-temp-card [card {:dataset_query {:native {:template_tags {:a {:type "date", :name "a", :display_name "a"}
-                                                                               :b {:type "date", :name "b", :display_name "b"}
-                                                                               :c {:type "date", :name "c", :display_name "c"}
-                                                                               :d {:type "date", :name "d", :display_name "d"}}}}}]
-      (:parameters ((test-users/user->client :crowberto) :get 200 (card-url card {:_embedding_params {:a "locked", :b "disabled", :c "enabled", :d "enabled"}
-                                                                                  :params            {:c 100}}))))))
+    (embed-test/with-temp-card [card {:dataset_query
+                                      {:native {:template_tags {:a {:type "date", :name "a", :display_name "a"}
+                                                                :b {:type "date", :name "b", :display_name "b"}
+                                                                :c {:type "date", :name "c", :display_name "c"}
+                                                                :d {:type "date", :name "d", :display_name "d"}}}}}]
+      (-> ((test-users/user->client :crowberto) :get 200 (card-url card {:_embedding_params {:a "locked"
+                                                                                             :b "disabled"
+                                                                                             :c "enabled"
+                                                                                             :d "enabled"}
+                                                                         :params            {:c 100}}))
+          :parameters ))))
 
 
-;;; ------------------------------------------------------------ GET /api/preview_embed/card/:token/query ------------------------------------------------------------
+;;; ------------------------------------ GET /api/preview_embed/card/:token/query ------------------------------------
 
 (defn- card-query-url [card & [additional-token-params]]
   (str "preview_embed/card/"
@@ -104,14 +111,17 @@
   (embed-test/successful-query-results)
   (embed-test/with-embedding-enabled-and-new-secret-key
     (embed-test/with-temp-card [card]
-      ((test-users/user->client :crowberto) :get 200 (card-query-url card {:_embedding_params {:abc "locked"}, :params {:abc 100}})))))
+      ((test-users/user->client :crowberto) :get 200 (card-query-url card {:_embedding_params {:abc "locked"}
+                                                                           :params            {:abc 100}})))))
 
 ;; if `:locked` parameter is present in URL params, request should fail
 (expect
   "You can only specify a value for :abc in the JWT."
   (embed-test/with-embedding-enabled-and-new-secret-key
     (embed-test/with-temp-card [card]
-      ((test-users/user->client :crowberto) :get 400 (str (card-query-url card {:_embedding_params {:abc "locked"}, :params {:abc 100}}) "?abc=200")))))
+      ((test-users/user->client :crowberto) :get 400 (str (card-query-url card {:_embedding_params {:abc "locked"}
+                                                                                :params            {:abc 100}})
+                                                          "?abc=200")))))
 
 ;;; DISABLED params
 
@@ -120,14 +130,16 @@
   "You're not allowed to specify a value for :abc."
   (embed-test/with-embedding-enabled-and-new-secret-key
     (embed-test/with-temp-card [card]
-      ((test-users/user->client :crowberto) :get 400 (card-query-url card {:_embedding_params {:abc "disabled"}, :params {:abc 100}})))))
+      ((test-users/user->client :crowberto) :get 400 (card-query-url card {:_embedding_params {:abc "disabled"}
+                                                                           :params            {:abc 100}})))))
 
 ;; If a `:disabled` param is passed in the URL the request should fail
 (expect
   "You're not allowed to specify a value for :abc."
   (embed-test/with-embedding-enabled-and-new-secret-key
     (embed-test/with-temp-card [card]
-      ((test-users/user->client :crowberto) :get 400 (str (card-query-url card {:_embedding_params {:abc "disabled"}}) "?abc=200")))))
+      ((test-users/user->client :crowberto) :get 400 (str (card-query-url card {:_embedding_params {:abc "disabled"}})
+                                                          "?abc=200")))))
 
 ;;; ENABLED params
 
@@ -136,26 +148,32 @@
   "You can't specify a value for :abc if it's already set in the JWT."
   (embed-test/with-embedding-enabled-and-new-secret-key
     (embed-test/with-temp-card [card]
-      ((test-users/user->client :crowberto) :get 400 (str (card-query-url card {:_embedding_params {:abc "enabled"}, :params {:abc 100}}) "?abc=200")))))
+      ((test-users/user->client :crowberto) :get 400 (str (card-query-url card {:_embedding_params {:abc "enabled"}
+                                                                                :params            {:abc 100}})
+                                                          "?abc=200")))))
 
 ;; If an `:enabled` param is present in the JWT, that's ok
 (expect
   (embed-test/successful-query-results)
   (embed-test/with-embedding-enabled-and-new-secret-key
     (embed-test/with-temp-card [card]
-      ((test-users/user->client :crowberto) :get 200 (card-query-url card {:_embedding_params {:abc "enabled"}, :params {:abc "enabled"}})))))
+      ((test-users/user->client :crowberto) :get 200 (card-query-url card {:_embedding_params {:abc "enabled"}
+                                                                           :params            {:abc "enabled"}})))))
 
 ;; If an `:enabled` param is present in URL params but *not* the JWT, that's ok
 (expect
   (embed-test/successful-query-results)
   (embed-test/with-embedding-enabled-and-new-secret-key
     (embed-test/with-temp-card [card]
-      ((test-users/user->client :crowberto) :get 200 (str (card-query-url card {:_embedding_params {:abc "enabled"}}) "?abc=200")))))
+      ((test-users/user->client :crowberto) :get 200 (str (card-query-url card {:_embedding_params {:abc "enabled"}})
+                                                          "?abc=200")))))
 
 
-;;; ------------------------------------------------------------ GET /api/preview_embed/dashboard/:token ------------------------------------------------------------
+;;; ------------------------------------ GET /api/preview_embed/dashboard/:token -------------------------------------
 
-(defn- dashboard-url [dashboard & [additional-token-params]] (str "preview_embed/dashboard/" (embed-test/dash-token dashboard (merge {:_embedding_params {}} additional-token-params))))
+(defn- dashboard-url {:style/indent 1} [dashboard & [additional-token-params]]
+  (str "preview_embed/dashboard/" (embed-test/dash-token dashboard (merge {:_embedding_params {}}
+                                                                          additional-token-params))))
 
 ;; it should be possible to call this endpoint successfully...
 (expect
@@ -195,14 +213,19 @@
                                                 {:slug "b", :name "b", :type "date"}
                                                 {:slug "c", :name "c", :type "date"}
                                                 {:slug "d", :name "d", :type "date"}]}]
-      (:parameters ((test-users/user->client :crowberto) :get 200 (dashboard-url dash {:params            {:c 100},
-                                                                                       :_embedding_params {:a "locked", :b "disabled", :c "enabled", :d "enabled"}}))))))
+      (:parameters ((test-users/user->client :crowberto) :get 200 (dashboard-url dash
+                                                                    {:params            {:c 100},
+                                                                     :_embedding_params {:a "locked"
+                                                                                         :b "disabled"
+                                                                                         :c "enabled"
+                                                                                         :d "enabled"}}))))))
 
 
-;;; ------------------------------------------------------------ GET /api/preview_embed/dashboard/:token/dashcard/:dashcard-id/card/:card-id ------------------------------------------------------------
+;;; ------------------ GET /api/preview_embed/dashboard/:token/dashcard/:dashcard-id/card/:card-id -------------------
 
 (defn- dashcard-url {:style/indent 1} [dashcard & [additional-token-params]]
-  (str "preview_embed/dashboard/" (embed-test/dash-token (:dashboard_id dashcard) (merge {:_embedding_params {}} additional-token-params))
+  (str "preview_embed/dashboard/" (embed-test/dash-token (:dashboard_id dashcard) (merge {:_embedding_params {}}
+                                                                                         additional-token-params))
        "/dashcard/" (u/get-id dashcard)
        "/card/" (:card_id dashcard)))
 
@@ -277,7 +300,8 @@
   "You're not allowed to specify a value for :abc."
   (embed-test/with-embedding-enabled-and-new-secret-key
     (embed-test/with-temp-dashcard [dashcard]
-      ((test-users/user->client :crowberto) :get 400 (str (dashcard-url dashcard {:_embedding_params {:abc "disabled"}}) "?abc=200")))))
+      ((test-users/user->client :crowberto) :get 400 (str (dashcard-url dashcard {:_embedding_params {:abc "disabled"}})
+                                                          "?abc=200")))))
 
 ;;; ENABLED params
 
@@ -286,18 +310,47 @@
   "You can't specify a value for :abc if it's already set in the JWT."
   (embed-test/with-embedding-enabled-and-new-secret-key
     (embed-test/with-temp-dashcard [dashcard]
-      ((test-users/user->client :crowberto) :get 400 (str (dashcard-url dashcard {:_embedding_params {:abc "enabled"}, :params {:abc 100}}) "?abc=200")))))
+      ((test-users/user->client :crowberto) :get 400 (str (dashcard-url dashcard {:_embedding_params {:abc "enabled"}
+                                                                                  :params            {:abc 100}})
+                                                          "?abc=200")))))
 
 ;; If an `:enabled` param is present in the JWT, that's ok
 (expect
   (embed-test/successful-query-results)
   (embed-test/with-embedding-enabled-and-new-secret-key
     (embed-test/with-temp-dashcard [dashcard]
-      ((test-users/user->client :crowberto) :get 200 (dashcard-url dashcard {:_embedding_params {:abc "enabled"}, :params {:abc 100}})))))
+      ((test-users/user->client :crowberto) :get 200 (dashcard-url dashcard {:_embedding_params {:abc "enabled"}
+                                                                             :params            {:abc 100}})))))
 
 ;; If an `:enabled` param is present in URL params but *not* the JWT, that's ok
 (expect
   (embed-test/successful-query-results)
   (embed-test/with-embedding-enabled-and-new-secret-key
     (embed-test/with-temp-dashcard [dashcard]
-      ((test-users/user->client :crowberto) :get 200 (str (dashcard-url dashcard {:_embedding_params {:abc "enabled"}}) "?abc=200")))))
+      ((test-users/user->client :crowberto) :get 200 (str (dashcard-url dashcard {:_embedding_params {:abc "enabled"}})
+                                                          "?abc=200")))))
+
+;; Check that editable query params work correctly and keys get coverted from strings to keywords, even if they're
+;; something that our middleware doesn't normally assume is implicitly convertable to a keyword. See
+;; `ring.middleware.keyword-params/keyword-syntax?` (#6783)
+(expect
+  "completed"
+  (embed-test/with-embedding-enabled-and-new-secret-key
+    (embed-test/with-temp-card [card {:dataset_query
+                                      {:database (data/id)
+                                       :type     "native"
+                                       :native   {:query         (str "SELECT {{num_birds}} AS num_birds,"
+                                                                      "       {{2nd_date_seen}} AS 2nd_date_seen")
+                                                  :template_tags {:equipment        {:name         "num_birds"
+                                                                                     :display_name "Num Birds"
+                                                                                     :type         "number"}
+                                                                  :7_days_ending_on {:name         "2nd_date_seen",
+                                                                                     :display_name "Date Seen",
+                                                                                     :type         "date"}}}}}]
+      (-> (embed-test/with-temp-dashcard [dashcard {:dash {:enable_embedding true}}]
+            ((test-users/user->client :crowberto) :get 200 (str (dashcard-url dashcard
+                                                                  {:_embedding_params {:num_birds     :locked
+                                                                                       :2nd_date_seen :enabled}
+                                                                   :params            {:num_birds 2}})
+                                                                "?2nd_date_seen=2018-02-14")))
+          :status))))

test/metabase/test/data/users.clj

@@ -10,7 +10,7 @@
             [toucan.db :as db])
   (:import clojure.lang.ExceptionInfo))
 
-;;; ------------------------------------------------------------ User Definitions ------------------------------------------------------------
+;;; ------------------------------------------------ User Definitions ------------------------------------------------
 
 ;; ## Test Users
 ;;
@@ -44,7 +44,7 @@
 (def ^:private ^:const usernames
   (set (keys user->info)))
 
-;;; ------------------------------------------------------------ Test User Fns ------------------------------------------------------------
+;;; ------------------------------------------------- Test User Fns --------------------------------------------------
 
 (defn- wait-for-initiailization
   "Wait up to MAX-WAIT-SECONDS (default: 30) for Metabase to finish initializing.
@@ -58,7 +58,8 @@
      (when-not (init-status/complete?)
        (when (<= max-wait-seconds 0)
          (throw (Exception. "Metabase still hasn't finished initializing.")))
-       (printf "Metabase is not yet initialized, waiting 1 second (max wait remaining: %d seconds)...\n" max-wait-seconds)
+       (println (format "Metabase is not yet initialized, waiting 1 second (max wait remaining: %d seconds)...\n"
+                        max-wait-seconds))
        (Thread/sleep 1000)
        (recur (dec max-wait-seconds))))))
 
@@ -130,7 +131,8 @@
   (or (@tokens username)
       (u/prog1 (http/authenticate (user->credentials username))
         (swap! tokens assoc username <>))
-      (throw (Exception. (format "Authentication failed for %s with credentials %s" username (user->credentials username))))))
+      (throw (Exception. (format "Authentication failed for %s with credentials %s"
+                                 username (user->credentials username))))))
 
 (defn- client-fn [username & args]
   (try
@@ -155,6 +157,7 @@
 
 (defn ^:deprecated delete-temp-users!
   "Delete all users besides the 4 persistent test users.
-   This is a HACK to work around tests that don't properly clean up after themselves; one day we should be able to remove this. (TODO)"
+  This is a HACK to work around tests that don't properly clean up after themselves; one day we should be able to
+  remove this. (TODO)"
   []
   (db/delete! User :id [:not-in (map user->id [:crowberto :lucky :rasta :trashbird])]))