with-temporary-setting-values should use Setting's getter to fetch orig value (#18861)

src/metabase/api/geojson.clj

@@ -18,7 +18,7 @@
               :region_name              (s/maybe s/Str)
               (s/optional-key :builtin) s/Bool}})
 
-(def ^:private ^:const builtin-geojson
+(def ^:private builtin-geojson
   {:us_states       {:name        "United States"
                      :url         "app/assets/geojson/us-states.json"
                      :region_key  "STATE"
@@ -83,9 +83,11 @@
   :default {}
   :getter  (fn [] (merge (setting/get-json :custom-geojson) builtin-geojson))
   :setter  (fn [new-value]
-             (when new-value
-               (validate-geojson new-value))
-             (setting/set-json! :custom-geojson new-value))
+             ;; remove the built-in keys you can't override them and we don't want those to be subject to validation.
+             (let [new-value (not-empty (reduce dissoc new-value (keys builtin-geojson)))]
+               (when new-value
+                 (validate-geojson new-value))
+               (setting/set-json! :custom-geojson new-value)))
   :visibility :public)
 
 (api/defendpoint-async GET "/:key"

test/metabase/api/geojson_test.clj

@@ -67,6 +67,23 @@
             (is (valid? geojson) (str url))
             (is (thrown? clojure.lang.ExceptionInfo (valid? geojson)) (str url))))))))
 
+(deftest custom-geojson-disallow-overriding-builtins-test
+  (testing "We shouldn't let people override the builtin GeoJSON and put weird stuff in there; ignore changes to them"
+    (mt/with-temporary-setting-values [custom-geojson nil]
+      (let [built-in @#'geojson-api/builtin-geojson]
+        (testing "Make sure the built-in entries still look like what we expect so our test still makes sense."
+          (is (schema= {:us_states {:name     (s/eq "United States")
+                                    s/Keyword s/Any}
+                        s/Keyword  s/Any}
+                       built-in))
+          (is (= built-in
+                 (geojson-api/custom-geojson))))
+        (testing "Try to change one of the built-in entries..."
+          (geojson-api/custom-geojson (assoc-in built-in [:us_states :name] "USA"))
+          (testing "Value should not have actually changed"
+            (is (= built-in
+                   (geojson-api/custom-geojson)))))))))
+
 (deftest update-endpoint-test
   (testing "PUT /api/setting/custom-geojson"
     (testing "test that we can set the value of geojson-api/custom-geojson via the normal routes"
@@ -76,14 +93,14 @@
                ;; bind a temporary value so it will get set back to its old value here after the API calls are done
                ;; stomping all over it
                (mt/with-temporary-setting-values [custom-geojson nil]
-                 ((mt/user->client :crowberto) :put 204 "setting/custom-geojson" {:value test-custom-geojson})
-                 ((mt/user->client :crowberto) :get 200 "setting/custom-geojson"))))))
+                 (mt/user-http-request :crowberto :put 204 "setting/custom-geojson" {:value test-custom-geojson})
+                 (mt/user-http-request :crowberto :get 200 "setting/custom-geojson"))))))
     (testing "passing in an invalid URL" ; see above validation test
       (is (= (str "Invalid GeoJSON file location: must either start with http:// or https:// or be a relative path to a file on the classpath. "
                   "URLs referring to hosts that supply internal hosting metadata are prohibited.")
-             ((mt/user->client :crowberto) :put 400 "setting/custom-geojson"
-              {:value {:mordor (assoc (first (vals test-custom-geojson))
-                                      :url "ftp://example.com")}}))))
+             (mt/user-http-request :crowberto :put 400 "setting/custom-geojson"
+                                   {:value {:mordor (assoc (first (vals test-custom-geojson))
+                                                           :url "ftp://example.com")}}))))
     (testing "it accepts resources"
       (let [resource-geojson {(first (keys test-custom-geojson))
                               (assoc (first (vals test-custom-geojson))
@@ -91,27 +108,27 @@
         (is (= (merge @#'geojson-api/builtin-geojson resource-geojson)
                (u/auto-retry 3
                  (mt/with-temporary-setting-values [custom-geojson nil]
-                   ((mt/user->client :crowberto) :put 204 "setting/custom-geojson"
-                    {:value resource-geojson})
-                   ((mt/user->client :crowberto) :get 200 "setting/custom-geojson")))))))))
+                   (mt/user-http-request :crowberto :put 204 "setting/custom-geojson"
+                                         {:value resource-geojson})
+                   (mt/user-http-request :crowberto :get 200 "setting/custom-geojson")))))))))
 
 (deftest url-proxy-endpoint-test
   (testing "GET /api/geojson"
     (testing "test the endpoint that fetches JSON files given a URL"
       (is (= {:type        "Point"
               :coordinates [37.77986 -122.429]}
-             ((mt/user->client :crowberto) :get 200 "geojson" :url test-geojson-url))))
+             (mt/user-http-request :crowberto :get 200 "geojson" :url test-geojson-url))))
     (testing "error is returned if URL connection fails"
       (is (= "GeoJSON URL failed to load"
-             ((mt/user->client :crowberto) :get 400 "geojson" :url test-broken-geojson-url))))
+             (mt/user-http-request :crowberto :get 400 "geojson" :url test-broken-geojson-url))))
     (testing "error is returned if URL is invalid"
       (is (= (str "Invalid GeoJSON file location: must either start with http:// or https:// or be a relative path to "
                   "a file on the classpath. URLs referring to hosts that supply internal hosting metadata are "
                   "prohibited.")
-             ((mt/user->client :crowberto) :get 400 "geojson" :url "file://tmp"))))
+             (mt/user-http-request :crowberto :get 400 "geojson" :url "file://tmp"))))
     (testing "cannot be called by non-admins"
       (is (= "You don't have permissions to do that."
-             ((mt/user->client :rasta) :get 403 "geojson" :url test-geojson-url))))))
+             (mt/user-http-request :rasta :get 403 "geojson" :url test-geojson-url))))))
 
 (deftest key-proxy-endpoint-test
   (testing "GET /api/geojson/:key"
@@ -119,7 +136,7 @@
       (testing "test the endpoint that fetches JSON files given a GeoJSON key"
         (is (= {:type        "Point"
                 :coordinates [37.77986 -122.429]}
-               ((mt/user->client :rasta) :get 200 "geojson/middle-earth"))))
+               (mt/user-http-request :rasta :get 200 "geojson/middle-earth"))))
       (testing "response should not include the usual cache-busting headers"
         (is (= (#'mw.security/cache-far-future-headers)
                (select-keys (:headers (client/client-full-response :get 200 "geojson/middle-earth"))
@@ -130,8 +147,8 @@
                (client/client :get 200 "geojson/middle-earth"))))
       (testing "try fetching an invalid key; should fail"
         (is (= "Invalid custom GeoJSON key: invalid-key"
-               ((mt/user->client :rasta) :get 400 "geojson/invalid-key")))))
+               (mt/user-http-request :rasta :get 400 "geojson/invalid-key")))))
     (mt/with-temporary-setting-values [custom-geojson test-broken-custom-geojson]
       (testing "fetching a broken URL should fail"
         (is (= "GeoJSON URL failed to load"
-               ((mt/user->client :rasta) :get 400 "geojson/middle-earth")))))))
+               (mt/user-http-request :rasta :get 400 "geojson/middle-earth")))))))

test/metabase/test/util.clj

@@ -332,9 +332,9 @@
 
 (defn do-with-temporary-setting-value
   "Temporarily set the value of the Setting named by keyword `setting-k` to `value` and execute `f`, then re-establish
-  the original value. This works much the same way as `binding`.
+  the original value. This works much the same way as [[binding]].
 
-  If an env var value is set for the setting, this acts as a wrapper around `do-with-temp-env-var-value`.
+  If an env var value is set for the setting, this acts as a wrapper around [[do-with-temp-env-var-value]].
 
   Prefer the macro `with-temporary-setting-values` over using this function directly."
   {:style/indent 2}
@@ -342,22 +342,30 @@
   (test-runner.parallel/assert-test-is-not-parallel "with-temporary-setting-values")
   ;; plugins have to be initialized because changing `report-timezone` will call driver methods
   (initialize/initialize-if-needed! :db :plugins)
-  (let [setting                    (#'setting/resolve-setting setting-k)
-        env-var-value              (#'setting/env-var-value setting)
-        original-db-or-cache-value (#'setting/db-or-cache-value setting)]
+  (let [setting       (#'setting/resolve-setting setting-k)
+        env-var-value (#'setting/env-var-value setting)]
     (if env-var-value
       (do-with-temp-env-var-value setting env-var-value thunk)
-      (try
-        (setting/set! setting-k value)
-        (testing (colorize/blue (format "\nSetting %s = %s\n" (keyword setting-k) (pr-str value)))
-          (thunk))
-        (catch Throwable e
-          (throw (ex-info (str "Error in with-temporary-setting-values: " (ex-message e))
-                          {:setting setting-k
-                           :value   value}
-                          e)))
-        (finally
-          (setting/set! setting-k original-db-or-cache-value))))))
+      (let [original-value (setting/get setting-k)]
+        (try
+          (setting/set! setting-k value)
+          (testing (colorize/blue (format "\nSetting %s = %s\n" (keyword setting-k) (pr-str value)))
+            (thunk))
+          (catch Throwable e
+            (throw (ex-info (str "Error in with-temporary-setting-values: " (ex-message e))
+                            {:setting  setting-k
+                             :location (symbol (name (:namespace setting)) (name setting-k))
+                             :value    value}
+                            e)))
+          (finally
+            (try
+              (setting/set! setting-k original-value)
+              (catch Throwable e
+                (throw (ex-info (str "Error restoring original Setting value: " (ex-message e))
+                                {:setting        setting-k
+                                 :location       (symbol (name (:namespace setting)) (name setting-k))
+                                 :original-value original-value}
+                                e))))))))))
 
 (defmacro with-temporary-setting-values
   "Temporarily bind the values of one or more `Settings`, execute body, and re-establish the original values. This
@@ -367,7 +375,7 @@
        (google-auth-auto-create-accounts-domain)) -> \"metabase.com\"
 
   If an env var value is set for the setting, this will change the env var rather than the setting stored in the DB.
-  To temporarily override the value of *read-only* env vars, use `with-temp-env-var-value`."
+  To temporarily override the value of *read-only* env vars, use [[with-temp-env-var-value]]."
   [[setting-k value & more :as bindings] & body]
   (assert (even? (count bindings)) "mismatched setting/value pairs: is each setting name followed by a value?")
   (if (empty? bindings)