From a3314aeff7c0fbc4d3661d0c0f75e0c60e8c89ac Mon Sep 17 00:00:00 2001
From: Cam Saul <cam@geotip.com>
Date: Mon, 6 Jul 2015 20:09:47 -0700
Subject: [PATCH] Strip DB details from API calls for non-admins
---
src/metabase/api/routes.clj | 13 +-
src/metabase/core.clj | 52 +++----
src/metabase/middleware/auth.clj | 88 ++++++------
src/metabase/middleware/format.clj | 12 +-
src/metabase/models/common.clj | 10 +-
src/metabase/models/database.clj | 43 +++---
src/metabase/models/interface.clj | 14 ++
test/metabase/api/meta/db_test.clj | 30 ++--
test/metabase/api/meta/field_test.clj | 1 -
test/metabase/api/meta/table_test.clj | 7 +-
test/metabase/middleware/auth_test.clj | 184 +++++++++++--------------
test/metabase/task_test.clj | 8 +-
12 files changed, 229 insertions(+), 233 deletions(-)
create mode 100644 src/metabase/models/interface.clj
diff --git a/src/metabase/api/routes.clj b/src/metabase/api/routes.clj
index 470f277d078..51413b17f15 100644
--- a/src/metabase/api/routes.clj
+++ b/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))
diff --git a/src/metabase/core.clj b/src/metabase/core.clj
index 2e269bcd1f7..5584404dcff 100644
--- a/src/metabase/core.clj
+++ b/src/metabase/core.clj
@@ -1,19 +1,9 @@
+;; -*- 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
diff --git a/src/metabase/middleware/auth.clj b/src/metabase/middleware/auth.clj
index 080a10fc3ee..441ec9a16b5 100644
--- a/src/metabase/middleware/auth.clj
+++ b/src/metabase/middleware/auth.clj
@@ -1,6 +1,6 @@
(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)))
diff --git a/src/metabase/middleware/format.clj b/src/metabase/middleware/format.clj
index f6a372d76a1..673c11ad00a 100644
--- a/src/metabase/middleware/format.clj
+++ b/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))
diff --git a/src/metabase/models/common.clj b/src/metabase/models/common.clj
index 50a16eddf06..adb7a0a0980 100644
--- a/src/metabase/models/common.clj
+++ b/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"}])
diff --git a/src/metabase/models/database.clj b/src/metabase/models/database.clj
index 680d57ed30f..bd43d73ee43 100644
--- a/src/metabase/models/database.clj
+++ b/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))
diff --git a/src/metabase/models/interface.clj b/src/metabase/models/interface.clj
new file mode 100644
index 00000000000..1887ea2ffe2
--- /dev/null
+++ b/src/metabase/models/interface.clj
@@ -0,0 +1,14 @@
+(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))
diff --git a/test/metabase/api/meta/db_test.clj b/test/metabase/api/meta/db_test.clj
index 736efb3dc7e..4ef82a691b6 100644
--- a/test/metabase/api/meta/db_test.clj
+++ b/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
diff --git a/test/metabase/api/meta/field_test.clj b/test/metabase/api/meta/field_test.clj
index 09134c7bf6f..f60cc814da4 100644
--- a/test/metabase/api/meta/field_test.clj
+++ b/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
diff --git a/test/metabase/api/meta/table_test.clj b/test/metabase/api/meta/table_test.clj
index 4bb229203a3..0ed48e7a5bf 100644
--- a/test/metabase/api/meta/table_test.clj
+++ b/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 $
diff --git a/test/metabase/middleware/auth_test.clj b/test/metabase/middleware/auth_test.clj
index 871143d14a4..58ac400655c 100644
--- a/test/metabase/middleware/auth_test.clj
+++ b/test/metabase/middleware/auth_test.clj
@@ -1,188 +1,172 @@
(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")))
diff --git a/test/metabase/task_test.clj b/test/metabase/task_test.clj
index 5e1b817e390..a6e07ddfc41 100644
--- a/test/metabase/task_test.clj
+++ b/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)]))
--
GitLab