diff --git a/src/metabase/api/routes.clj b/src/metabase/api/routes.clj
index fde4c21c1a9de13bae64f20527f8486f6ddb64e3..d4b95a6bfe28d922e70ced55ac95cda9c55b5365 100644
--- a/src/metabase/api/routes.clj
+++ b/src/metabase/api/routes.clj
@@ -65,7 +65,7 @@
(defroutes ^{:doc "Ring routes for API endpoints."} routes
ee-routes
- (context "/action" [] (+auth api.action/routes))
+ (context "/action" [] (+auth api.action/routes))
(context "/activity" [] (+auth api.activity/routes))
(context "/alert" [] (+auth api.alert/routes))
(context "/automagic-dashboards" [] (+auth api.magic/routes))
diff --git a/src/metabase/api/routes/common.clj b/src/metabase/api/routes/common.clj
index 6bb5f7578b09a0835d10cfc019162c66964c4771..c9a03c785c289ab71b6cc93bf2917e92db09a774 100644
--- a/src/metabase/api/routes/common.clj
+++ b/src/metabase/api/routes/common.clj
@@ -8,17 +8,17 @@
(def +generic-exceptions
"Wrap `routes` so any Exception thrown is just returned as a generic 400, to prevent details from leaking in public
endpoints."
- mw.exceptions/genericize-exceptions)
+ #'mw.exceptions/genericize-exceptions)
(def +message-only-exceptions
"Wrap `routes` so any Exception thrown is just returned as a 400 with only the message from the original
Exception (i.e., remove the original stacktrace), to prevent details from leaking in public endpoints."
- mw.exceptions/message-only-exceptions)
+ #'mw.exceptions/message-only-exceptions)
(def +apikey
"Wrap `routes` so they may only be accessed with a correct API key header."
- mw.auth/enforce-api-key)
+ #'mw.auth/enforce-api-key)
(def +auth
"Wrap `routes` so they may only be accessed with proper authentication credentials."
- mw.auth/enforce-authentication)
+ #'mw.auth/enforce-authentication)
diff --git a/src/metabase/models/setting.clj b/src/metabase/models/setting.clj
index 94295e0e8c39a7e9e43b6fc6b3f0555b641b6960..022c1774b8d17d8f57589b8a619d618114396a08 100644
--- a/src/metabase/models/setting.clj
+++ b/src/metabase/models/setting.clj
@@ -398,6 +398,11 @@
(str/replace "-" "_")
str/upper-case)))
+(defn setting-env-map-name
+ "Correctly translate a setting to the keyword it will be found at in [[env/env]]."
+ [setting-definition-or-name]
+ (keyword (str "mb-" (munge-setting-name (setting-name setting-definition-or-name)))))
+
(defn env-var-value
"Get the value of `setting-definition-or-name` from the corresponding env var, if any.
The name of the Setting is converted to uppercase and dashes to underscores; for example, a setting named
@@ -407,7 +412,7 @@
^String [setting-definition-or-name]
(let [setting (resolve-setting setting-definition-or-name)]
(when (allows-site-wide-values? setting)
- (let [v (env/env (keyword (str "mb-" (munge-setting-name (setting-name setting)))))]
+ (let [v (env/env (setting-env-map-name setting))]
(when (seq v)
v)))))
diff --git a/src/metabase/server/middleware/auth.clj b/src/metabase/server/middleware/auth.clj
index 509f3f402388afb772299370952da3499887fad3..d3928d701d9e802d8e0903b1714a514850a89848 100644
--- a/src/metabase/server/middleware/auth.clj
+++ b/src/metabase/server/middleware/auth.clj
@@ -2,8 +2,10 @@
"Middleware related to enforcing authentication/API keys (when applicable). Unlike most other middleware most of this
is not used as part of the normal `app`; it is instead added selectively to appropriate routes."
(:require
+ [clojure.string :as str]
[metabase.models.setting :refer [defsetting]]
- [metabase.server.middleware.util :as mw.util]))
+ [metabase.server.middleware.util :as mw.util]
+ [metabase.util.i18n :refer [deferred-trs]]))
(def ^:private ^:const ^String metabase-api-key-header "x-metabase-apikey")
@@ -31,6 +33,15 @@
"When set, this API key is required for all API requests."
:visibility :internal)
+(def mb-api-key-doc-url
+ "Url for documentation on how to set MB_API_KEY."
+ "https://www.metabase.com/docs/latest/configuring-metabase/environment-variables#mb_api_key")
+
+(def key-not-set-response
+ "Response when the MB_API_KEY is not set."
+ {:status 403
+ :body (deferred-trs "MB_API_KEY is not set. See {0} for details" mb-api-key-doc-url)})
+
(defn enforce-api-key
"Middleware that enforces validation of the client via API Key, canceling the request processing if the check fails.
@@ -43,7 +54,10 @@
This variable only works for /api/notify/db/:id endpoint"
[handler]
(fn [{:keys [metabase-api-key], :as request} respond raise]
- (cond (not metabase-api-key)
+ (cond (str/blank? (api-key))
+ (respond key-not-set-response)
+
+ (not metabase-api-key)
(respond mw.util/response-forbidden)
(= (api-key) metabase-api-key)
diff --git a/test/metabase/api/notify_test.clj b/test/metabase/api/notify_test.clj
index 6ec608da6507d48b1ad46d3195e6cf8b883a0875..e51ef120df11375bcf88afad1aca3d74ac36678f 100644
--- a/test/metabase/api/notify_test.clj
+++ b/test/metabase/api/notify_test.clj
@@ -4,6 +4,7 @@
[clojure.test :refer :all]
[metabase.http-client :as client]
[metabase.models.database :as database]
+ [metabase.server.middleware.auth :as mw.auth]
[metabase.server.middleware.util :as mw.util]
[metabase.sync]
[metabase.sync.sync-metadata]
@@ -13,11 +14,16 @@
(use-fixtures :once (fixtures/initialize :db :web-server))
-(deftest unauthenticated-test
+(deftest authentication-test
(testing "POST /api/notify/db/:id"
- (testing "endpoint should require authentication"
- (is (= (get mw.util/response-forbidden :body)
- (client/client :post 403 "notify/db/100"))))))
+ (testing "endpoint requires MB_API_KEY set"
+ (mt/with-temporary-setting-values [api-key nil]
+ (is (= (-> mw.auth/key-not-set-response :body str)
+ (client/client :post 403 "notify/db/100")))))
+ (testing "endpoint requires authentication"
+ (mt/with-temporary-setting-values [api-key "test-api-key"] ;; set in :test but not in :dev
+ (is (= (get mw.util/response-forbidden :body)
+ (client/client :post 403 "notify/db/100")))))))
(def api-headers {:headers {"X-METABASE-APIKEY" "test-api-key"
"Content-Type" "application/json"}})
diff --git a/test/metabase/models/setting_test.clj b/test/metabase/models/setting_test.clj
index ba5cc613fb28a32a01f6d3fa6045556d4bf93d69..aae1c069e04a5535e7a6515b8723fb87a23da395 100644
--- a/test/metabase/models/setting_test.clj
+++ b/test/metabase/models/setting_test.clj
@@ -226,7 +226,7 @@
(tu/do-with-temporary-setting-value setting db-value
(fn []
(tu/do-with-temp-env-var-value
- (keyword (str "mb-" (name setting)))
+ (setting/setting-env-map-name (keyword setting))
env-var-value
(fn []
(dissoc (#'setting/user-facing-info (#'setting/resolve-setting setting))
@@ -632,7 +632,7 @@
(setting.cache/restore-cache!)))))
(fn [thunk]
(tu/do-with-temp-env-var-value
- (keyword (str "mb-" (name setting-name)))
+ (setting/setting-env-map-name setting-name)
site-wide-value
thunk))]]
;; clear out Setting if it was already set for some reason (except for `:only` where this is explicitly
diff --git a/test/metabase/public_settings_test.clj b/test/metabase/public_settings_test.clj
index 2db1ae84bffcc360895657fd4fbc1ead348ff494..de3e9091bc385be72ee1bb2c66fe068b05fd0903 100644
--- a/test/metabase/public_settings_test.clj
+++ b/test/metabase/public_settings_test.clj
@@ -62,21 +62,19 @@
(deftest site-url-settings-normalize
(testing "We should normalize `site-url` when set via env var we should still normalize it (#9764)"
(mt/with-temp-env-var-value [mb-site-url "localhost:3000/"]
- (mt/with-temporary-setting-values [site-url nil]
- (is (= "localhost:3000/"
- (setting/get-value-of-type :string :site-url)))
- (is (= "http://localhost:3000"
- (public-settings/site-url)))))))
+ (is (= "localhost:3000/"
+ (setting/get-value-of-type :string :site-url)))
+ (is (= "http://localhost:3000"
+ (public-settings/site-url))))))
(deftest invalid-site-url-env-var-test
(testing (str "If `site-url` is set via an env var, and it's invalid, we should return `nil` rather than having the"
" whole instance break")
(mt/with-temp-env-var-value [mb-site-url "asd_12w31%$;"]
- (mt/with-temporary-setting-values [site-url nil]
- (is (= "asd_12w31%$;"
- (setting/get-value-of-type :string :site-url)))
- (is (= nil
- (public-settings/site-url)))))))
+ (is (= "asd_12w31%$;"
+ (setting/get-value-of-type :string :site-url)))
+ (is (= nil
+ (public-settings/site-url))))))
(deftest site-url-should-update-https-redirect-test
(testing "Changing `site-url` to non-HTTPS should disable forced HTTPS redirection"
diff --git a/test/metabase/server/middleware/auth_test.clj b/test/metabase/server/middleware/auth_test.clj
index ff035279e92db14fa517f8ed18e79e91762b6aa1..51595e7be8345e1f43bbc98a08b7abb1a77a9aab 100644
--- a/test/metabase/server/middleware/auth_test.clj
+++ b/test/metabase/server/middleware/auth_test.clj
@@ -138,11 +138,9 @@
(request-with-api-key "foobar"))))))
(testing "no apikey is set, expect 403"
- (mt/with-temporary-setting-values [api-key nil]
- (is (= mw.util/response-forbidden
- (api-key-enforced-handler
- (ring.mock/request :get "/anyurl")))))
- (mt/with-temporary-setting-values [api-key ""]
- (is (= mw.util/response-forbidden
- (api-key-enforced-handler
- (ring.mock/request :get "/anyurl")))))))
+ (doseq [api-key-value [nil ""]]
+ (testing (str "when key is " ({nil "nil" "" "empty"} api-key-value))
+ (mt/with-temporary-setting-values [api-key api-key-value]
+ (is (= mw.auth/key-not-set-response
+ (api-key-enforced-handler
+ (ring.mock/request :get "/anyurl")))))))))
diff --git a/test/metabase/test/util.clj b/test/metabase/test/util.clj
index 9e8bf777a4a61bef8f802f9d981456419c7fe697..3a8d491c8c29d1420883a13f4f2f9ac10e28bfce 100644
--- a/test/metabase/test/util.clj
+++ b/test/metabase/test/util.clj
@@ -424,8 +424,8 @@
(catch Exception e
(when-not raw-setting?
(throw e))))]
- (if-let [env-var-value (and (not raw-setting?) (#'setting/env-var-value setting-k))]
- (do-with-temp-env-var-value setting env-var-value thunk)
+ (if (and (not raw-setting?) (#'setting/env-var-value setting-k))
+ (do-with-temp-env-var-value (setting/setting-env-map-name setting-k) value thunk)
(let [original-value (if raw-setting?
(db/select-one-field :value Setting :key setting-k)
(#'setting/get setting-k))]