Revert "Remove old `MB_API_KEY` setting (#48592)" (#49507)

* Revert "Remove old `MB_API_KEY` setting (#48592)"

This reverts commit 187e9508.

* preserve metadata of the wrapped handler

src/metabase/api/routes.clj

@@ -37,7 +37,7 @@
    [metabase.api.pulse.unsubscribe :as api.pulse.unsubscribe]
    [metabase.api.revision :as api.revision]
    [metabase.api.routes.common
-    :refer [+auth +message-only-exceptions +public-exceptions]]
+    :refer [+auth +message-only-exceptions +public-exceptions +static-apikey]]
    [metabase.api.search :as api.search]
    [metabase.api.segment :as api.segment]
    [metabase.api.session :as api.session]
@@ -124,7 +124,7 @@
   (context "/metabot"              [] (+auth api.metabot/routes))
   (context "/model-index"          [] (+auth api.model-index/routes))
   (context "/native-query-snippet" [] (+auth api.native-query-snippet/routes))
-  (context "/notify"               [] (+auth api.notify/routes))
+  (context "/notify"               [] (+static-apikey api.notify/routes))
   (context "/permissions"          [] (+auth api.permissions/routes))
   (context "/persist"              [] (+auth api.persist/routes))
   (context "/premium-features"     [] (+auth api.premium-features/routes))

src/metabase/api/routes/common.clj

@@ -15,6 +15,10 @@
   Exception (i.e., remove the original stacktrace), to prevent details from leaking in public endpoints."
   #'mw.exceptions/message-only-exceptions)
 
+(def +static-apikey
+  "Wrap `routes` so they may only be accessed with a correct API key header."
+  #'mw.auth/enforce-static-api-key)
+
 (def +auth
   "Wrap `routes` so they may only be accessed with proper authentication credentials."
   #'mw.auth/enforce-authentication)

src/metabase/server/handler.clj

@@ -3,6 +3,7 @@
   (:require
    [metabase.analytics.sdk :as sdk]
    [metabase.config :as config]
+   [metabase.server.middleware.auth :as mw.auth]
    [metabase.server.middleware.browser-cookie :as mw.browser-cookie]
    [metabase.server.middleware.exceptions :as mw.exceptions]
    [metabase.server.middleware.json :as mw.json]
@@ -59,6 +60,7 @@
    #'mw.session/wrap-current-user-info          ; looks for :metabase-session-id and sets :metabase-user-id and other info if Session ID is valid
    #'sdk/embedding-mw                           ; reads sdk client headers, binds them to *client* and *version*, and tracks sdk-response metrics
    #'mw.session/wrap-session-id                 ; looks for a Metabase Session ID and assoc as :metabase-session-id
+   #'mw.auth/wrap-static-api-key                ; looks for a static Metabase API Key on the request and assocs as :metabase-api-key
    #'wrap-cookies                               ; Parses cookies in the request map and assocs as :cookies
    #'mw.misc/add-content-type                   ; Adds a Content-Type header for any response that doesn't already have one
    #'mw.misc/disable-streaming-buffering        ; Add header to streaming (async) responses so ngnix doesn't buffer keepalive bytes

src/metabase/server/middleware/auth.clj

@@ -2,8 +2,12 @@
   "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.request.util :as req.util]))
+   [metabase.server.request.util :as req.util]
+   [metabase.util.i18n :refer [deferred-trs]]))
+
+(def ^:private ^:const ^String static-metabase-api-key-header "x-metabase-apikey")
 
 (defn enforce-authentication
   "Middleware that returns a 401 response if `request` has no associated `:metabase-user-id`."
@@ -15,12 +19,65 @@
        (respond req.util/response-unauthentic)))
    (meta handler)))
 
+(defn- wrap-static-api-key* [{:keys [headers], :as request}]
+  (if-let [api-key (headers static-metabase-api-key-header)]
+    (assoc request :static-metabase-api-key api-key)
+    request))
+
+(defn wrap-static-api-key
+  "Middleware that sets the `:static-metabase-api-key` keyword on the request if a valid API Key can be found. We check
+  the request headers for `X-METABASE-APIKEY` and if it's not found then no keyword is bound to the request."
+  [handler]
+  (with-meta
+   (fn [request respond raise]
+     (handler (wrap-static-api-key* request) respond raise))
+   (meta handler)))
+
 (defsetting api-key
   "When set, this API key is required for all API requests."
-  :deprecated "0.50.0"
   :encryption :when-encryption-key-set
   :visibility :internal
-  :doc "Decommissioned setting that previously allowed specifying a static API key required for all API requests to
-        the `/notify` endpoints. No longer has any effect, as these endpoints are now protected by normal auth (with
-        normal API keys set via the admin panel); purely keeping this around to emit a WARN log message on startup. We
-        can remove this in a version or two.")
+  :doc "Middleware that enforces validation of the client via the request header X-Metabase-Apikey.
+        If the header is available, then it’s validated against MB_API_KEY.
+        When it matches, the request continues; otherwise it’s blocked with a 403 Forbidden response.")
+
+(defn static-api-key
+  "We don't want to change the name of the setting from `MB_API_KEY`, but we want to differentiate this static key from
+  the API keys that can be generated by admins."
+  [] (api-key))
+
+(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-static-api-key
+  "Middleware that enforces validation of the client via API Key, canceling the request processing if the check fails.
+
+  Validation is handled by first checking for the presence of the `:static-metabase-api-key` on the request. If the
+  api key is available then we validate it by checking it against the configured `:mb-api-key` value set in our global
+  config.
+
+  If the request `:static-metabase-api-key` matches the configured `api-key` value then the request continues,
+  otherwise we reject the request and return a 403 Forbidden response.
+
+  This variable only works for /api/notify/db/:id endpoint"
+  [handler]
+  (with-meta
+   (fn [{:keys [static-metabase-api-key], :as request} respond raise]
+     (cond (str/blank? (static-api-key))
+           (respond key-not-set-response)
+
+           (not static-metabase-api-key)
+           (respond req.util/response-forbidden)
+
+           (= (static-api-key) static-metabase-api-key)
+           (handler request respond raise)
+
+           :else
+           (respond req.util/response-forbidden)))
+   (meta handler)))

test/metabase/api/notify_test.clj

 (ns ^:mb/driver-tests metabase.api.notify-test
   (:require
+   [clj-http.client :as http]
    [clojure.java.jdbc :as jdbc]
    [clojure.test :refer :all]
    [metabase.driver.postgres-test :as postgres-test]
    [metabase.driver.sql-jdbc.connection :as sql-jdbc.conn]
    [metabase.http-client :as client]
    [metabase.models.database :as database :refer [Database]]
+   [metabase.server.middleware.auth :as mw.auth]
+   [metabase.server.request.util :as req.util]
    [metabase.sync :as sync]
    [metabase.sync.sync-metadata]
    [metabase.test :as mt]
@@ -17,24 +20,48 @@
 
 (deftest authentication-test
   (testing "POST /api/notify/db/:id"
+    (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"
-      (is (= "Unauthenticated"
-             (client/client :post 401 "notify/db/100"))))))
+      (mt/with-temporary-setting-values [api-key "test-api-key"] ;; set in :test but not in :dev
+        (is (= (get req.util/response-forbidden :body)
+               (client/client :post 403 "notify/db/100")))))))
+
+(def ^:private api-headers {:headers {"x-metabase-apikey" "test-api-key"
+                                      "content-type"      "application/json"}})
 
 (deftest not-found-test
   (mt/with-temporary-setting-values [api-key "test-api-key"]
     (testing "POST /api/notify/db/:id"
       (testing "database must exist or we get a 404"
-        (is (= "Not found."
-               (mt/user-http-request :rasta :post 404 (format "notify/db/%d" Integer/MAX_VALUE)))))
+        (is (= {:status 404
+                :body   "Not found."}
+               (try (http/post (client/build-url (format "notify/db/%d" Integer/MAX_VALUE) {})
+                               (merge {:accept :json} api-headers))
+                    (catch clojure.lang.ExceptionInfo e
+                      (select-keys (ex-data e) [:status :body]))))))
       (testing "table ID must exist or we get a 404"
-        (is (= "Not found."
-               (mt/user-http-request :rasta :post (format "notify/db/%d" (:id (mt/db)))
-                                     {:table_id Integer/MAX_VALUE}))))
+        (is (= {:status 404
+                :body   "Not found."}
+               (try (http/post (client/build-url (format "notify/db/%d" (:id (mt/db))) {})
+                               (merge {:accept       :json
+                                       :content-type :json
+                                       :form-params  {:table_id Integer/MAX_VALUE}}
+                                      api-headers))
+                    (catch clojure.lang.ExceptionInfo e
+                      (select-keys (ex-data e) [:status :body]))))))
       (testing "table name must exist or we get a 404"
-        (is (= "Not found."
-               (mt/user-http-request :rasta :post (format "notify/db/%d" (:id (mt/db)))
-                                     {:table_name "IncorrectToucanFact"})))))))
+        (is (= {:status 404
+                :body   "Not found."}
+               (try (http/post (client/build-url (format "notify/db/%d" (:id (mt/db))) {})
+                               (merge {:accept       :json
+                                       :content-type :json
+                                       :form-params  {:table_name "IncorrectToucanFact"}}
+                                      api-headers))
+                    (catch clojure.lang.ExceptionInfo e
+                      (select-keys (ex-data e) [:status :body])))))))))
 
 (deftest post-db-id-test
   (mt/test-drivers (mt/normal-drivers)
@@ -42,8 +69,11 @@
           post       (fn post-api
                        ([payload] (post-api payload 200))
                        ([payload expected-code]
-                        (mt/user-http-request :rasta :post expected-code (format "notify/db/%d" (u/the-id (mt/db)))
-                                              (merge {:synchronous? true} payload))))]
+                        (mt/with-temporary-setting-values [api-key "test-api-key"]
+                          (mt/client :post expected-code (format "notify/db/%d" (u/the-id (mt/db)))
+                                     {:request-options api-headers}
+                                     (merge {:synchronous? true}
+                                            payload)))))]
       (testing "sync just table when table is provided"
         (let [long-sync-called? (promise), short-sync-called? (promise)]
           (with-redefs [sync/sync-table!                                 (fn [_table] (deliver long-sync-called? true))
@@ -88,8 +118,12 @@
                 post     (fn post-api
                            ([payload] (post-api payload 200))
                            ([payload expected-code]
-                            (mt/user-http-request :rasta :post expected-code (format "notify/db/%d/new-table" (:id database))
-                                                  (merge {:synchronous? true} payload))))
+                            (mt/with-temporary-setting-values [api-key "test-api-key"]
+                              (mt/client-full-response
+                               :post expected-code (format "notify/db/%d/new-table" (:id database))
+                               {:request-options api-headers}
+                               (merge {:synchronous? true}
+                                      payload)))))
                 sync!    #(sync/sync-database! database)]
             ;; Create the initial table and sync it.
             (exec! spec ["CREATE TABLE public.FOO (val bigint NOT NULL);"])
@@ -97,14 +131,14 @@
             (let [tables (tableset database)]
               (is (= #{"public.foo"} tables)))
             ;; We can't add an existing table
-            (is (post {:schema_name "public" :table_name "foo"} 400))
+            (is (= 400 (:status (post {:schema_name "public" :table_name "foo"} 400))))
             ;; We can't add a nonexistent table
-            (is (post {:schema_name "public" :table_name "bar"} 404))
+            (is (= 404 (:status (post {:schema_name "public" :table_name "bar"} 404))))
             ;; Create two more tables that are not yet synced
             (exec! spec ["CREATE TABLE public.BAR (val bigint NOT NULL);"
                          "CREATE TABLE public.FERN (val bigint NOT NULL);"])
             ;; This will add bar to metabase (but not fern).
-            (is (post {:schema_name "public" :table_name "bar"} 200))
+            (is (= 200 (:status (post {:schema_name "public" :table_name "bar"}))))
             ;; Assert that only the synced tables are present.
             (let [tables (tableset database)]
               (is (= #{"public.foo" "public.bar"} tables))

test/metabase/server/middleware/auth_test.clj

@@ -6,6 +6,7 @@
    [metabase.server.middleware.auth :as mw.auth]
    [metabase.server.middleware.session :as mw.session]
    [metabase.server.request.util :as req.util]
+   [metabase.test :as mt]
    [metabase.test.data.users :as test.users]
    [metabase.test.fixtures :as fixtures]
    [ring.mock.request :as ring.mock]
@@ -76,3 +77,67 @@
                  (auth-enforced-handler
                   (request-with-session-id session-id))))
           (finally (t2/delete! Session :id session-id)))))))
+
+;;; ------------------------------------------ TEST wrap-static-api-key middleware ------------------------------------------
+
+;; create a simple example of our middleware wrapped around a handler that simply returns the request
+;; this works in this case because the only impact our middleware has is on the request
+(defn- wrapped-api-key-handler [request]
+  ((mw.auth/wrap-static-api-key
+    (fn [request respond _] (respond request)))
+   request
+   identity
+   (fn [e] (throw e))))
+
+(deftest wrap-static-api-key-test
+  (testing "No API key in the request"
+    (is (nil?
+         (:metabase-session-id
+          (wrapped-api-key-handler
+           (ring.mock/request :get "/anyurl"))))))
+
+  (testing "API Key in header"
+    (is (= "foobar"
+           (:static-metabase-api-key
+            (wrapped-api-key-handler
+             (ring.mock/header (ring.mock/request :get "/anyurl") @#'mw.auth/static-metabase-api-key-header "foobar")))))))
+
+;;; ---------------------------------------- TEST enforce-static-api-key middleware -----------------------------------------
+
+;; create a simple example of our middleware wrapped around a handler that simply returns the request
+(defn- api-key-enforced-handler [request]
+  ((mw.auth/enforce-static-api-key (fn [_ respond _] (respond {:success true})))
+   request
+   identity
+   (fn [e] (throw e))))
+
+(defn- request-with-api-key
+  "Creates a mock Ring request with the given apikey applied"
+  [api-key]
+  (-> (ring.mock/request :get "/anyurl")
+      (assoc :static-metabase-api-key api-key)))
+
+(deftest enforce-static-api-key-request
+  (mt/with-temporary-setting-values [api-key "test-api-key"]
+    (testing "no apikey in the request, expect 403"
+      (is (= req.util/response-forbidden
+             (api-key-enforced-handler
+              (ring.mock/request :get "/anyurl")))))
+
+    (testing "valid apikey, expect 200"
+      (is (= {:success true}
+             (api-key-enforced-handler
+              (request-with-api-key "test-api-key")))))
+
+    (testing "invalid apikey, expect 403"
+      (is (= req.util/response-forbidden
+             (api-key-enforced-handler
+              (request-with-api-key "foobar"))))))
+
+  (testing "no apikey is set, expect 403"
+    (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")))))))))