From 55e063e56deb46bc03d954197fd67ac2b3804da6 Mon Sep 17 00:00:00 2001
From: dpsutton <dan@dpsutton.com>
Date: Thu, 5 Jan 2023 18:03:08 -0600
Subject: [PATCH] Notify api key not set hints (#27515)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
* nit alignment
* point to var so changes are picked up
need to refresh this ns after changing the middleware since these add a
bit more helpful doc strings. But capture previous function value so
more annoying at repl
* Ensure MB_API_KEY is set for notify endpoint
Previously when the MB_API_KEY was unset:
```
⯠http POST "localhost:3000/api/notify/db/1?scan=schema"
HTTP/1.1 403 Forbidden
<other headers removed>
Forbidden
```
And now:
```
⯠http POST "localhost:3000/api/notify/db/1?scan=schema"
HTTP/1.1 403 Forbidden
<other headers removed>
MB_API_KEY is not set. See https://www.metabase.com/docs/latest/configuring-metabase/environment-variables#mb_api_key for details
```
An annoying thing in the tests. We set `"-Dmb.api.key=test-api-key"` in
the `:test` alias for CI, but not in regular dev mode. So annoyingly we
either have tests that would fail in the repl in regular dev, or use
`mt/with-temporary-setting-values [api-key "test-api-key"]` when writing
tests that only matter for local stuff. c'est la vie
* Translate error message
Using site locale because this in an `+apikey` context, not an `+auth`
context. The importance of that is that there is no logged in user so no
way to get a user locale, and it just falls back to the server locale
anyways. So lets just use that.
* Fix temp settings when they are env-based
An annoying bit here. `api-key` setting in the notify test is set in the
env in the `:test` alias.
```clojure
notify-test=> (mt/with-temporary-setting-values [api-key nil]
(mw.auth/api-key))
"test-api-key" ;; where did the nil go?
```
But there are two bugs in `mt/with-temporary-setting-values`:
1. It gets the current env value and then temporarily sets it to the
current value. Note that `do-with-temp-env-var-value` is called with
`env-var-value` which is the current value in the environment. All of
this work for nothing
```
- (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-k value thunk)
```
So cannot possibly do what we want.
2. The second bug is that the pathway through
`mt/with-temporary-setting-values` did not convert `:api-key` to
`:mb-api-key`. So even if it put our new value in above, it would put
`:api-key nil` in the env, but when we look for it it would look for
`:mb-api-key` and we would not find it.
Thus the use of `setting-env-map-name`. Then clean up the few other
places that did it kinda right, but not necessarily. Seems the "correct"
way to do this is get the setting name, munge it, prepend with mb. The
other call sites were using variants of `(keyword (str "mb-" (name
setting-name)))` which is close but could miss the munging.
* Safely set env/env keys
There are two ways that we put things into the env/env so they appear as
environmentally set variables.
- explicitly with `mt/with-temp-env-var-value`. This simulates a user
setting these values and they are specified in the env way:
mb-email-smtp-port, "MB_DISABLE_SCHEDULER", etc. At the call site you
are aware that they are env related so get the mb prefix
- incidentally with `mt/with-temporary-setting-values`. Settings are
always referred to by their "nice" internal name like `api-key`,
etc. But if a setting is found to be an env variable, we override it in
env/env. But we have to make sure the key is set properly with an mb-
prefix.
Owing to this, we have to conditionally add the mb- prefix if not
already present.
* cleanup the constants a bit
* Fix some site-url tests
The tests themselves are testing useful stuff: site-url doesn't return a
broken value if it fails the verification when set as an env
variable. But the tests themselves were a bit nonsensical in their
setup:
```
@@ -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))))))
```
Why would it set an env-variable version [mb-site-url "localhost:3000/"]
then try to blank it out with `(mt/with-temporary-setting-values
[site-url nil])`.
Fixing the bug in how we set env var values made this nonsensical stuff
break.
Now we just have to do the obvious thing: set the env var to a bad
value, assert that (setting/get-value-of-type :string :site-url) is that
value, but assert that the _setting_ is a well-formatted value or nil if
we can't solve it (the "asd_12w31%$;" case).
Setting the env value then setting a temporary value to nil didn't make
much sense. And the fix made that set the env var to nil.
* fixup last tests
* Don't modify things from `with-temp-env-var-value`
this allows us to set non-mb-prefixed env vars. not sure if necessary
but let's roll with it.
* fix last of the tests
---
src/metabase/api/routes.clj | 2 +-
src/metabase/api/routes/common.clj | 8 ++++----
src/metabase/models/setting.clj | 7 ++++++-
src/metabase/server/middleware/auth.clj | 18 ++++++++++++++++--
test/metabase/api/notify_test.clj | 14 ++++++++++----
test/metabase/models/setting_test.clj | 4 ++--
test/metabase/public_settings_test.clj | 18 ++++++++----------
test/metabase/server/middleware/auth_test.clj | 14 ++++++--------
test/metabase/test/util.clj | 4 ++--
9 files changed, 55 insertions(+), 34 deletions(-)
diff --git a/src/metabase/api/routes.clj b/src/metabase/api/routes.clj
index fde4c21c1a9..d4b95a6bfe2 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 6bb5f7578b0..c9a03c785c2 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 94295e0e8c3..022c1774b8d 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 509f3f40238..d3928d701d9 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 6ec608da650..e51ef120df1 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 ba5cc613fb2..aae1c069e04 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 2db1ae84bff..de3e9091bc3 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 ff035279e92..51595e7be83 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 9e8bf777a4a..3a8d491c8c2 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))]
--
GitLab