Strip DB details from API calls for non-admins

src/metabase/api/routes.clj

@@ -15,18 +15,13 @@
                                [table :as table])
             [metabase.middleware.auth :as auth]))
 
-(defn- +apikey
+(def ^:private +apikey
   "Wrap API-ROUTES so they may only be accessed with proper apikey credentials."
-  [api-routes]
-  (-> api-routes
-      auth/enforce-apikey))
+  auth/enforce-api-key)
 
-(defn- +auth
+(def ^:private +auth
   "Wrap API-ROUTES so they may only be accessed with proper authentiaction credentials."
-  [api-routes]
-  (-> api-routes
-      auth/bind-current-user
-      auth/enforce-authentication))
+  auth/enforce-authentication)
 
 (defroutes routes
   (context "/card"         [] (+auth card/routes))

src/metabase/core.clj

+;; -*- comment-column: 35; -*-
 (ns metabase.core
   (:gen-class)
   (:require [clojure.tools.logging :as log]
             [clojure.java.browse :refer [browse-url]]
             [colorize.core :as color]
-            [medley.core :as medley]
-            [metabase.config :as config]
-            [metabase.db :as db]
-            (metabase.middleware [auth :as auth]
-                                 [log-api-call :refer :all]
-                                 [format :refer :all])
-            [metabase.models.setting :refer [defsetting]]
-            [metabase.models.user :refer [User]]
-            [metabase.routes :as routes]
-            [metabase.setup :as setup]
-            [metabase.task :as task]
             [ring.adapter.jetty :as ring-jetty]
             (ring.middleware [cookies :refer [wrap-cookies]]
                              [gzip :refer [wrap-gzip]]
@@ -21,7 +11,18 @@
                                            wrap-json-body]]
                              [keyword-params :refer [wrap-keyword-params]]
                              [params :refer [wrap-params]]
-                             [session :refer [wrap-session]])))
+                             [session :refer [wrap-session]])
+            [medley.core :as medley]
+            (metabase [config :as config]
+                      [db :as db]
+                      [routes :as routes]
+                      [setup :as setup]
+                      [task :as task])
+            (metabase.middleware [auth :as auth]
+                                 [log-api-call :refer :all]
+                                 [format :refer :all])
+            (metabase.models [setting :refer [defsetting]]
+                             [user :refer [User]])))
 
 ;; ## CONFIG
 
@@ -32,17 +33,19 @@
   "The primary entry point to the HTTP server"
   (-> routes/routes
       (log-api-call :request :response)
-      format-response         ; [METABASE] Do formatting before converting to JSON so serializer doesn't barf
-      (wrap-json-body         ; extracts json POST body and makes it avaliable on request
+      format-response              ; [METABASE] Do formatting before converting to JSON so serializer doesn't barf
+      (wrap-json-body              ; extracts json POST body and makes it avaliable on request
         {:keywords? true})
-      wrap-json-response      ; middleware to automatically serialize suitable objects as JSON in responses
-      wrap-keyword-params     ; converts string keys in :params to keyword keys
-      wrap-params             ; parses GET and POST params as :query-params/:form-params and both as :params
-      auth/wrap-apikey        ; looks for a Metabase API Key on the request and assocs as :metabase-apikey
-      auth/wrap-sessionid     ; looks for a Metabase sessionid and assocs as :metabase-sessionid
-      wrap-cookies            ; Parses cookies in the request map and assocs as :cookies
-      wrap-session            ; reads in current HTTP session and sets :session/key
-      wrap-gzip))             ; GZIP response if client can handle it
+      wrap-json-response           ; middleware to automatically serialize suitable objects as JSON in responses
+      wrap-keyword-params          ; converts string keys in :params to keyword keys
+      wrap-params                  ; parses GET and POST params as :query-params/:form-params and both as :params
+      auth/bind-current-user       ; Binds *current-user* and *current-user-id* if :metabase-user-id is non-nil
+      auth/wrap-current-user-id    ; looks for :metabase-session-id and sets :metabase-user-id if Session ID is valid
+      auth/wrap-api-key            ; looks for a Metabase API Key on the request and assocs as :metabase-api-key
+      auth/wrap-session-id         ; looks for a Metabase Session ID and assoc as :metabase-session-id
+      wrap-cookies                 ; Parses cookies in the request map and assocs as :cookies
+      wrap-session                 ; reads in current HTTP session and sets :session/key
+      wrap-gzip))                  ; GZIP response if client can handle it
 
 (defn- -init-create-setup-token
   "Create and set a new setup token, and open the setup URL on the user's system."
@@ -57,10 +60,7 @@
                          setup-token)]
     (log/info (color/green "Please use the following url to setup your Metabase installation:\n\n"
                            setup-url
-                           "\n\n"))
-    ;; Attempt to browse URL on user's system; this will just fail silently if we can't do it
-    ;(browse-url setup-url)
-    ))
+                           "\n\n"))))
 
 
 (defn init

src/metabase/middleware/auth.clj

 (ns metabase.middleware.auth
   "Middleware for dealing with authentication and session management."
-  (:require [korma.core :as korma]
+  (:require [korma.core :as k]
             [metabase.config :as config]
             [metabase.db :refer [sel]]
             [metabase.api.common :refer [*current-user* *current-user-id*]]
@@ -8,16 +8,16 @@
                              [user :refer [User current-user-fields]])))
 
 
-(def metabase-session-cookie "metabase.SESSION_ID")
-(def metabase-session-header "x-metabase-session")
-(def metabase-apikey-header "x-metabase-apikey")
+(def ^:const metabase-session-cookie "metabase.SESSION_ID")
+(def ^:const metabase-session-header "x-metabase-session")
+(def ^:const metabase-api-key-header "x-metabase-apikey")
 
-(def response-unauthentic {:status 401 :body "Unauthenticated"})
-(def response-forbidden {:status 403 :body "Forbidden"})
+(def ^:const response-unauthentic {:status 401 :body "Unauthenticated"})
+(def ^:const response-forbidden {:status 403 :body "Forbidden"})
 
 
-(defn wrap-sessionid
-  "Middleware that sets the :metabase-sessionid keyword on the request if a session id can be found.
+(defn wrap-session-id
+  "Middleware that sets the `:metabase-session-id` keyword on the request if a session id can be found.
 
    We first check the request :cookies for `metabase.SESSION_ID`, then if no cookie is found we look in the
    http headers for `X-METABASE-SESSION`.  If neither is found then then no keyword is bound to the request."
@@ -25,32 +25,34 @@
   (fn [{:keys [cookies headers] :as request}]
     (if-let [session-id (or (get-in cookies [metabase-session-cookie :value]) (headers metabase-session-header))]
       ;; alternatively we could always associate the keyword and just let it be nil if there is no value
-      (handler (assoc request :metabase-sessionid session-id))
+      (handler (assoc request :metabase-session-id session-id))
       (handler request))))
 
+(defn wrap-current-user-id
+  "Add `:metabase-user-id` to the request if a valid session token was passed."
+  [handler]
+  (fn [{:keys [metabase-session-id] :as request}]
+    ;; TODO - what kind of validations can we do on the sessionid to make sure it's safe to handle?  str?  alphanumeric?
+    (handler (or (when metabase-session-id
+                   (when-let [session (first (k/select Session
+                                                       ;; NOTE: we join with the User table and ensure user.is_active = true
+                                                       (k/with User (k/where {:is_active true}))
+                                                       (k/fields :created_at :user_id)
+                                                       (k/where {:id metabase-session-id})))]
+                     (let [session-age-ms (- (System/currentTimeMillis) (.getTime ^java.util.Date (get session :created_at (java.util.Date. 0))))]
+                       ;; If the session exists and is not expired (max-session-age > session-age) then validation is good
+                       (when (and session (> (config/config-int :max-session-age) (quot session-age-ms 60000)))
+                         (assoc request :metabase-user-id (:user_id session))))))
+                 request))))
 
-(defn enforce-authentication
-  "Middleware that enforces authentication of the client, cancelling the request processing if auth fails.
-
-   Authentication is determined by validating the :metabase-sessionid on the request against the db session list.
-   If the session is valid then we associate a :metabase-userid on the request and carry on, but if the validation
-   fails then we return an HTTP 401 response indicating that the client is not authentic.
 
-   NOTE: we are purposely not associating the full current user object here so that we can be modular."
+(defn enforce-authentication
+  "Middleware that returns a 401 response if REQUEST has no associated `:metabase-user-id`."
   [handler]
-  (fn [{:keys [metabase-sessionid] :as request}]
-    ;; TODO - what kind of validations can we do on the sessionid to make sure it's safe to handle?  str?  alphanumeric?
-    (let [session (first (korma/select Session
-                           ;; NOTE: we join with the User table and ensure user.is_active = true
-                           (korma/with User (korma/where {:is_active true}))
-                           (korma/fields :created_at :user_id)
-                           (korma/where {:id metabase-sessionid})))
-          session-age-ms (- (System/currentTimeMillis) (.getTime ^java.util.Date (get session :created_at (java.util.Date. 0))))]
-      ;; If the session exists and is not expired (max-session-age > session-age) then validation is good
-      (if (and session (> (config/config-int :max-session-age) (quot session-age-ms 60000)))
-        (handler (assoc request :metabase-userid (:user_id session)))
-        ;; default response is 401
-        response-unauthentic))))
+  (fn [{:keys [metabase-user-id] :as request}]
+    (if metabase-user-id
+      (handler request)
+      response-unauthentic)))
 
 
 (defmacro sel-current-user [current-user-id]
@@ -65,35 +67,35 @@
    *current-user*    delay that returns current user (or nil) from DB"
   [handler]
   (fn [request]
-    (let [current-user-id (:metabase-userid request)]
+    (if-let [current-user-id (:metabase-user-id request)]
       (binding [*current-user-id* current-user-id
-                *current-user* (if-not current-user-id (atom nil)
-                                                       (delay (sel-current-user current-user-id)))]
-        (handler request)))))
+                *current-user*    (delay (sel-current-user current-user-id))]
+        (handler request))
+      (handler request))))
 
 
-(defn wrap-apikey
-  "Middleware that sets the :metabase-apikey keyword on the request if a valid API Key can be found.
+(defn wrap-api-key
+  "Middleware that sets the :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 then no keyword is bound to the request."
+   We check the request headers for `X-METABASE-API-KEY` and if it's not found then then no keyword is bound to the request."
   [handler]
   (fn [{:keys [headers] :as request}]
-    (if-let [api-key (headers metabase-apikey-header)]
-      (handler (assoc request :metabase-apikey api-key))
+    (if-let [api-key (headers metabase-api-key-header)]
+      (handler (assoc request :metabase-api-key api-key))
       (handler request))))
 
 
-(defn enforce-apikey
+(defn enforce-api-key
   "Middleware that enforces validation of the client via API Key, cancelling the request processing if the check fails.
 
-   Validation is handled by first checking for the presence of the :metabase-apikey on the request.  If the api key
+   Validation is handled by first checking for the presence of the :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 :metabase-apikey matches the configured :mb-api-key value then the request continues, otherwise we
+   If the request :metabase-api-key matches the configured :mb-api-key value then the request continues, otherwise we
    reject the request and return a 403 Forbidden response."
   [handler]
-  (fn [{:keys [metabase-apikey] :as request}]
-    (if (= (config/config-str :mb-api-key) metabase-apikey)
+  (fn [{:keys [metabase-api-key] :as request}]
+    (if (= (config/config-str :mb-api-key) metabase-api-key)
       (handler request)
       ;; default response is 403
       response-forbidden)))

src/metabase/middleware/format.clj

@@ -3,6 +3,7 @@
             (cheshire factory
                       [generate :refer [add-encoder encode-str]])
             [medley.core :refer [filter-vals map-vals]]
+            [metabase.models.interface :refer [api-serialize]]
             [metabase.util :as util]))
 
 (declare -format-response)
@@ -45,11 +46,14 @@
   [m]
   (filter-vals #(not (or (delay? %)
                          (fn? %)))
-               m))
+               ;; Convert typed maps such as metabase.models.database/DatabaseInstance to plain maps because empty, which is used internally by filter-vals,
+               ;; will fail otherwise
+               (into {} m)))
 
 (defn- -format-response [obj]
   (cond
-    (map? obj)  (->> (remove-fns-and-delays obj)   ; recurse over all vals in the map
-                     (map-vals -format-response))
-    (coll? obj) (map -format-response obj)        ; recurse over all items in the collection
+    (map? obj)  (->> (api-serialize obj)
+                     remove-fns-and-delays
+                     (map-vals -format-response)) ; recurse over all vals in the map
+    (coll? obj) (map -format-response obj) ; recurse over all items in the collection
     :else       obj))

src/metabase/models/common.clj

@@ -3,7 +3,7 @@
             [metabase.api.common :refer [*current-user* *current-user-id* check]]
             [metabase.util :as u]))
 
-(def timezones
+(def ^:const timezones
   ["GMT"
    "UTC"
    "US/Alaska"
@@ -17,11 +17,11 @@
 
 ;;; ALLEN'S PERMISSIONS IMPLEMENTATION
 
-(def perms-none 0)
-(def perms-read 1)
-(def perms-readwrite 2)
+(def ^:const perms-none 0)
+(def ^:const perms-read 1)
+(def ^:const perms-readwrite 2)
 
-(def permissions
+(def ^:const permissions
   [{:id perms-none :name "None"},
    {:id perms-read :name "Read Only"},
    {:id perms-readwrite :name "Read & Write"}])

src/metabase/models/database.clj

@@ -2,7 +2,8 @@
   (:require [korma.core :refer :all]
             [metabase.api.common :refer [*current-user*]]
             [metabase.db :refer :all]
-            [metabase.models.common :refer [assoc-permissions-sets]]))
+            (metabase.models [common :refer [assoc-permissions-sets]]
+                             [interface :refer :all])))
 
 
 (defentity Database
@@ -13,33 +14,23 @@
   (assoc :hydration-keys #{:database
                            :db}))
 
-(defmethod post-select Database [_ db]
-  (assoc db
-         :can_read     (delay true)
-         :can_write    (delay (:is_superuser @*current-user*))))
-
- {:created_at #inst "2015-06-30T19:51:45.294000000-00:00",
-   :engine :h2,
-   :id 3,
-   :details
-   {:db
-    "file:/Users/camsaul/metabase/target/Test_Database;AUTO_SERVER=TRUE;DB_CLOSE_DELAY=-1"},
-   :updated_at #inst "2015-06-30T19:51:45.294000000-00:00",
-   :name "Test Database",
-   :organization_id nil,
-   :description nil}
+(defrecord DatabaseInstance []
+  ;; preserve normal IFn behavior so things like ((sel :one Database) :id) work correctly
+  clojure.lang.IFn
+  (invoke [this k]
+    (get this k))
 
-{:description nil,
-   :organization_id nil,
-   :name "Test Database",
-   :updated_at #inst "2015-06-30T19:51:45.294000000-00:00",
-   :details
-   {:db
-    "file:/Users/camsaul/metabase/target/Test_Database;AUTO_SERVER=TRUE;DB_CLOSE_DELAY=-1"},
-   :id 3,
-   :engine "h2",
-   :created_at #inst "2015-06-30T19:51:45.294000000-00:00"}
+  IModelInstanceApiSerialze
+  (api-serialize [this]
+    ;; If current user isn't an admin strip out DB details which may include things like password
+    (cond-> this
+      (not (:is_superuser @*current-user*)) (dissoc :details))))
 
+(defmethod post-select Database [_ db]
+  (map->DatabaseInstance
+   (assoc db
+          :can_read  (delay true)
+          :can_write (delay (:is_superuser @*current-user*)))))
 
 (defmethod pre-cascade-delete Database [_ {:keys [id] :as database}]
   (cascade-delete 'metabase.models.table/Table :db_id id))

src/metabase/models/interface.clj

+(ns metabase.models.interface)
+
+(defprotocol IModelInstanceApiSerialze
+  (api-serialize [this]
+    "Called on all objects being written out by the API. Default implementations return THIS as-is, but models can provide
+     custom methods to strip sensitive data, from non-admins, etc."))
+
+(extend-protocol IModelInstanceApiSerialze
+  Object
+  (api-serialize [this]
+    this)
+  nil
+  (api-serialize [_]
+    nil))

test/metabase/api/meta/db_test.clj

@@ -40,18 +40,31 @@
 ;; # DB LIFECYCLE ENDPOINTS
 
 ;; ## GET /api/meta/db/:id
+;; regular users *should not* see DB details
 (expect
     (match-$ (db)
-      {:created_at $
-       :engine "h2"
-       :id $
-       :details $
-       :updated_at $
-       :name "Test Database"
+      {:created_at      $
+       :engine          "h2"
+       :id              $
+       :updated_at      $
+       :name            "Test Database"
        :organization_id nil
-       :description nil})
+       :description     nil})
   ((user->client :rasta) :get 200 (format "meta/db/%d" (db-id))))
 
+;; superusers *should* see DB details
+(expect
+    (match-$ (db)
+      {:created_at      $
+       :engine          "h2"
+       :id              $
+       :details         $
+       :updated_at      $
+       :name            "Test Database"
+       :organization_id nil
+       :description     nil})
+  ((user->client :crowberto) :get 200 (format "meta/db/%d" (db-id))))
+
 ;; ## POST /api/meta/db
 ;; Check that we can create a Database
 (let [db-name (random-name)]
@@ -106,6 +119,7 @@
 
 ;; ## GET /api/meta/db
 ;; Test that we can get all the DBs for an Org, ordered by name
+;; Database details *should not* come back for Rasta since she's not a superuser
 (let [db-name (str "A" (random-name))] ; make sure this name comes before "Test Database"
   (expect-eval-actual-first
       (set (filter identity
@@ -115,7 +129,6 @@
                                {:created_at      $
                                 :engine          (name $engine)
                                 :id              $
-                                :details         $
                                 :updated_at      $
                                 :name            "Test Database"
                                 :organization_id nil
@@ -125,7 +138,6 @@
                              {:created_at      $
                               :engine          "postgres"
                               :id              $
-                              :details         {:host "localhost", :port 5432, :dbname "fakedb", :user "cam"}
                               :updated_at      $
                               :name            $
                               :organization_id nil

test/metabase/api/meta/field_test.clj

@@ -22,7 +22,6 @@
                        {:created_at $
                         :engine "h2"
                         :id $
-                        :details $
                         :updated_at $
                         :name "Test Database"
                         :organization_id nil

test/metabase/api/meta/table_test.clj

@@ -64,7 +64,6 @@
              {:created_at $
               :engine "h2"
               :id $
-              :details $
               :updated_at $
               :name "Test Database"
               :organization_id nil
@@ -120,7 +119,6 @@
              {:created_at $
               :engine "h2"
               :id $
-              :details $
               :updated_at $
               :name "Test Database"
               :organization_id nil
@@ -193,7 +191,6 @@
              {:created_at $
               :engine "h2"
               :id $
-              :details $
               :updated_at $
               :name "Test Database"
               :organization_id nil
@@ -293,7 +290,6 @@
              {:created_at $
               :engine "h2"
               :id $
-              :details $
               :updated_at $
               :name "Test Database"
               :organization_id nil
@@ -444,8 +440,7 @@
                                   :updated_at $,
                                   :id $,
                                   :engine "h2",
-                                  :created_at $
-                                  :details $})})})
+                                  :created_at $})})})
       :destination (match-$ users-id-field
                      {:id $
                       :table_id $

test/metabase/middleware/auth_test.clj

 (ns metabase.middleware.auth-test
   (:require [expectations :refer :all]
-            [korma.core :as korma]
+            [korma.core :as k]
             [ring.mock.request :as mock]
             [metabase.api.common :refer [*current-user-id* *current-user*]]
             [metabase.middleware.auth :refer :all]
             [metabase.models.session :refer [Session]]
             [metabase.test.data :refer :all]
             [metabase.test.data.users :refer :all]
-            [metabase.util :as util]))
+            [metabase.util :as u]))
 
-;;  ===========================  TEST wrap-sessionid middleware  ===========================
+;;  ===========================  TEST wrap-session-id 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
-(def wrapped-handler
-  (wrap-sessionid (fn [req] req)))
+(def ^:private wrapped-handler
+  (wrap-session-id identity))
 
 
-;; no sessionid in the request
-(expect
-  {}
+;; no session-id in the request
+(expect nil
   (-> (wrapped-handler (mock/request :get "/anyurl") )
-      (select-keys [:metabase-sessionid])))
+      :metabase-session-id))
 
 
-;; extract sessionid from header
-(expect
-  {:metabase-sessionid "foobar"}
+;; extract session-id from header
+(expect "foobar"
   (-> (wrapped-handler (mock/header (mock/request :get "/anyurl") metabase-session-header "foobar"))
-      (select-keys [:metabase-sessionid])))
+      :metabase-session-id))
 
 
-;; extract sessionid from cookie
-(expect
-  {:metabase-sessionid "cookie-session"}
+;; extract session-id from cookie
+(expect "cookie-session"
   (-> (wrapped-handler (assoc (mock/request :get "/anyurl") :cookies {metabase-session-cookie {:value "cookie-session"}}))
-      (select-keys [:metabase-sessionid])))
+      :metabase-session-id))
 
 
-;; if both header and cookie sessionids exist, then we expect the cookie to take precedence
-(expect
-  {:metabase-sessionid "cookie-session"}
+;; if both header and cookie session-ids exist, then we expect the cookie to take precedence
+(expect "cookie-session"
   (-> (wrapped-handler (-> (mock/header (mock/request :get "/anyurl") metabase-session-header "foobar")
                            (assoc :cookies {metabase-session-cookie {:value "cookie-session"}})))
-      (select-keys [:metabase-sessionid])))
+      :metabase-session-id))
 
 
 ;;  ===========================  TEST enforce-authentication middleware  ===========================
 
 
 ;; create a simple example of our middleware wrapped around a handler that simply returns the request
-(def auth-enforced-handler
-  (enforce-authentication (fn [req] req)))
+(def ^:private auth-enforced-handler
+  (wrap-current-user-id (enforce-authentication identity)))
 
 
-(defn request-with-sessionid
-  "Creates a mock Ring request with the given sessionid applied"
-  [sessionid]
+(defn- request-with-session-id
+  "Creates a mock Ring request with the given session-id applied"
+  [session-id]
   (-> (mock/request :get "/anyurl")
-      (assoc :metabase-sessionid sessionid)))
+      (assoc :metabase-session-id session-id)))
 
 
-;; no sessionid in the request
-(expect
-  {:status (:status response-unauthentic)
-   :body (:body response-unauthentic)}
+;; no session-id in the request
+(expect response-unauthentic
   (auth-enforced-handler (mock/request :get "/anyurl")))
 
+(defn- random-session-id []
+  {:post [(string? %)]}
+  (.toString (java.util.UUID/randomUUID)))
 
-;; valid sessionid
-(let [sessionid (.toString (java.util.UUID/randomUUID))]
-  (assert sessionid)
-  ;; validate that we are authenticated
-  (expect-let [res (korma/insert Session (korma/values {:id sessionid :user_id (user->id :rasta) :created_at (util/new-sql-timestamp)}))]
-    {:metabase-userid (user->id :rasta)}
-    (-> (auth-enforced-handler (request-with-sessionid sessionid))
-        (select-keys [:metabase-userid]))))
+;; valid session ID
+(expect (user->id :rasta)
+  (let [session-id (random-session-id)]
+    (k/insert Session (k/values {:id session-id, :user_id (user->id :rasta), :created_at (u/new-sql-timestamp)}))
+    (-> (auth-enforced-handler (request-with-session-id session-id))
+        :metabase-user-id)))
 
 
-;; expired sessionid
-(let [sessionid (.toString (java.util.UUID/randomUUID))]
-  (assert sessionid)
-  ;; create a new session (specifically created some time in the past so it's EXPIRED)
-  ;; should fail due to session expiration
-  (expect-let [res (korma/insert Session (korma/values {:id sessionid :user_id (user->id :rasta) :created_at (java.sql.Timestamp. 0)}))]
-    {:status (:status response-unauthentic)
-     :body (:body response-unauthentic)}
-    (auth-enforced-handler (request-with-sessionid sessionid))))
+;; expired session-id
+;; create a new session (specifically created some time in the past so it's EXPIRED)
+;; should fail due to session expiration
+(expect response-unauthentic
+  (let [session-id (random-session-id)]
+    (k/insert Session (k/values {:id session-id, :user_id (user->id :rasta), :created_at (java.sql.Timestamp. 0)}))
+    (auth-enforced-handler (request-with-session-id session-id))))
 
 
-;; inactive user sessionid
-(let [sessionid (.toString (java.util.UUID/randomUUID))]
-  (assert sessionid)
-  ;; create a new session (specifically created some time in the past so it's EXPIRED)
-  ;; should fail due to inactive user
-  ;; NOTE that :trashbird is our INACTIVE test user
-  (expect-let [res (korma/insert Session (korma/values {:id sessionid :user_id (user->id :trashbird) :created_at (util/new-sql-timestamp)}))]
-    {:status (:status response-unauthentic)
-     :body (:body response-unauthentic)}
-    (auth-enforced-handler (request-with-sessionid sessionid))))
+;; inactive user session-id
+;; create a new session (specifically created some time in the past so it's EXPIRED)
+;; should fail due to inactive user
+;; NOTE that :trashbird is our INACTIVE test user
+(expect response-unauthentic
+  (let [session-id (random-session-id)]
+    (k/insert Session (k/values {:id session-id, :user_id (user->id :trashbird), :created_at (u/new-sql-timestamp)}))
+    (auth-enforced-handler (request-with-session-id session-id))))
 
 
 ;;  ===========================  TEST bind-current-user middleware  ===========================
 
 
 ;; create a simple example of our middleware wrapped around a handler that simply returns our bound variables for users
-(def user-bound-handler
-  (bind-current-user (fn [req] {:userid *current-user-id*
-                                :user (select-keys @*current-user* [:id :email])})))
+(def ^:private user-bound-handler
+  (bind-current-user (fn [_] {:user-id *current-user-id*
+                              :user    (select-keys @*current-user* [:id :email])})))
 
-
-(defn request-with-userid
-  "Creates a mock Ring request with the given userid applied"
-  [userid]
+(defn- request-with-user-id
+  "Creates a mock Ring request with the given user-id applied"
+  [user-id]
   (-> (mock/request :get "/anyurl")
-      (assoc :metabase-userid userid)))
+      (assoc :metabase-user-id user-id)))
 
 
 ;; with valid user-id
 (expect
-  {:userid (user->id :rasta)
-   :user {:id (user->id :rasta)
-          :email (:email (fetch-user :rasta))}}
-  (user-bound-handler (request-with-userid (user->id :rasta))))
+    {:user-id (user->id :rasta)
+     :user    {:id    (user->id :rasta)
+               :email (:email (fetch-user :rasta))}}
+  (user-bound-handler (request-with-user-id (user->id :rasta))))
 
 ;; with invalid user-id (not sure how this could ever happen, but lets test it anyways)
 (expect
-  {:userid 0
-   :user {}}
-  (user-bound-handler (request-with-userid 0)))
+    {:user-id 0
+     :user    {}}
+  (user-bound-handler (request-with-user-id 0)))
 
 
-;;  ===========================  TEST wrap-apikey middleware  ===========================
+;;  ===========================  TEST wrap-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
-(def wrapped-apikey-handler
-  (wrap-apikey (fn [req] req)))
+(def ^:private wrapped-api-key-handler
+  (wrap-api-key identity))
 
 
 ;; no apikey in the request
-(expect
-  {}
-  (-> (wrapped-apikey-handler (mock/request :get "/anyurl") )
-      (select-keys [:metabase-sessionid])))
+(expect nil
+  (-> (wrapped-api-key-handler (mock/request :get "/anyurl") )
+      :metabase-session-id))
 
 
 ;; extract apikey from header
-(expect
-  {:metabase-apikey "foobar"}
-  (-> (wrapped-apikey-handler (mock/header (mock/request :get "/anyurl") metabase-apikey-header "foobar"))
-      (select-keys [:metabase-apikey])))
+(expect "foobar"
+  (-> (wrapped-api-key-handler (mock/header (mock/request :get "/anyurl") metabase-api-key-header "foobar"))
+      :metabase-api-key))
 
 
-;;  ===========================  TEST enforce-apikey middleware  ===========================
+;;  ===========================  TEST enforce-api-key middleware  ===========================
 
 
 ;; create a simple example of our middleware wrapped around a handler that simply returns the request
-(def apikey-enforced-handler
-  (enforce-apikey (fn [req] {:success true})))
+(def ^:private api-key-enforced-handler
+  (enforce-api-key (constantly {:success true})))
 
 
-(defn request-with-apikey
+(defn- request-with-api-key
   "Creates a mock Ring request with the given apikey applied"
-  [apikey]
+  [api-key]
   (-> (mock/request :get "/anyurl")
-      (assoc :metabase-apikey apikey)))
+      (assoc :metabase-api-key api-key)))
 
 
 ;; no apikey in the request, expect 403
-(expect
-  {:status (:status response-forbidden)
-   :body (:body response-forbidden)}
-  (apikey-enforced-handler (mock/request :get "/anyurl")))
+(expect response-forbidden
+  (api-key-enforced-handler (mock/request :get "/anyurl")))
 
 
 ;; valid apikey, expect 200
 (expect
-  {:success true}
-  (apikey-enforced-handler (request-with-apikey "test-api-key")))
+    {:success true}
+  (api-key-enforced-handler (request-with-api-key "test-api-key")))
 
 
 ;; invalid apikey, expect 403
-(expect
-  {:status (:status response-forbidden)
-   :body (:body response-forbidden)}
-  (apikey-enforced-handler (request-with-apikey "foobar")))
+(expect response-forbidden
+  (api-key-enforced-handler (request-with-api-key "foobar")))

test/metabase/task_test.clj

@@ -67,16 +67,16 @@
          :restarted]
   [(do
      (stop-task-runner!)
-     (with-redefs [metabase.task/hourly-task-delay (constantly 100)
+     (with-redefs [metabase.task/hourly-task-delay (constantly 200)
                    metabase.task/hourly-tasks-hook mock-hourly-tasks-hook]
        (add-hook! #'hourly-tasks-hook inc-task-test-atom-counter-by-system-hour)
        (reset! task-test-atom-counter 0)
        (start-task-runner!)
 
        [@task-test-atom-counter      ; should be 0, since not enough time has elaspsed for the hook to be executed
-        (do (Thread/sleep 150)
-            @task-test-atom-counter) ; should have been called once (~50ms ago)
-        (do (Thread/sleep 200)
+        (do (Thread/sleep 300)
+            @task-test-atom-counter) ; should have been called once (~100ms ago)
+        (do (Thread/sleep 400)
             @task-test-atom-counter) ; should have been called two more times
         (do (stop-task-runner!)
             :stopped)]))