Add HttpOnly, SameSite, and Secure directives to session cookies

src/metabase/api/session.clj

@@ -13,6 +13,7 @@
             [metabase.api.common :as api]
             [metabase.email.messages :as email]
             [metabase.integrations.ldap :as ldap]
+            [metabase.middleware.session :as mw.session]
             [metabase.models
              [session :refer [Session]]
              [setting :refer [defsetting]]
@@ -23,19 +24,21 @@
              [schema :as su]]
             [schema.core :as s]
             [throttle.core :as throttle]
-            [toucan.db :as db]))
+            [toucan.db :as db])
+  (:import com.unboundid.util.LDAPSDKException
+           java.util.UUID))
 
-(defn- create-session!
+(s/defn ^:private create-session! :- UUID
   "Generate a new `Session` for a given `User`. Returns the newly generated session ID."
-  [user]
-  {:pre  [(map? user) (integer? (:id user)) (contains? user :last_login)]
-   :post [(string? %)]}
-  (u/prog1 (str (java.util.UUID/randomUUID))
+  [user :- {:id         su/IntGreaterThanZero
+            :last_login s/Any
+            s/Keyword   s/Any}]
+  (u/prog1 (UUID/randomUUID)
     (db/insert! Session
-      :id      <>
+      :id      (str <>)
       :user_id (:id user))
     (events/publish-event! :user-login
-      {:user_id (:id user), :session_id <>, :first_login (not (boolean (:last_login user)))})))
+      {:user_id (:id user), :session_id (str <>), :first_login (nil? (:last_login user))})))
 
 ;;; ## API Endpoints
 
@@ -47,7 +50,7 @@
 (def ^:private password-fail-message (tru "Password did not match stored password."))
 (def ^:private password-fail-snippet (tru "did not match stored password"))
 
-(defn- ldap-login
+(s/defn ^:private ldap-login :- (s/maybe UUID)
   "If LDAP is enabled and a matching user exists return a new Session for them, or `nil` if they couldn't be
   authenticated."
   [username password]
@@ -61,18 +64,16 @@
                    {:status-code 400
                     :errors      {:password password-fail-snippet}})))
         ;; password is ok, return new session
-        {:id (create-session! (ldap/fetch-or-create-user! user-info password))})
-      (catch com.unboundid.util.LDAPSDKException e
-        (log/error
-         (u/format-color 'red
-             (trs "Problem connecting to LDAP server, will fall back to local authentication: {0}" (.getMessage e))))))))
+        (create-session! (ldap/fetch-or-create-user! user-info password)))
+      (catch LDAPSDKException e
+        (log/error e (trs "Problem connecting to LDAP server, will fall back to local authentication"))))))
 
-(defn- email-login
+(s/defn ^:private email-login :- (s/maybe UUID)
   "Find a matching `User` if one exists and return a new Session for them, or `nil` if they couldn't be authenticated."
   [username password]
   (when-let [user (db/select-one [User :id :password_salt :password :last_login], :email username, :is_active true)]
     (when (pass/verify-password password (:password_salt user) (:password user))
-      {:id (create-session! user)})))
+      (create-session! user))))
 
 (def ^:private throttling-disabled? (config/config-bool :mb-disable-session-throttle))
 
@@ -82,6 +83,20 @@
   (when-not throttling-disabled?
     (throttle/check throttler throttle-key)))
 
+(s/defn ^:private login :- UUID
+  "Attempt to login with different avaialable methods with `username` and `password`, returning new Session ID or
+  throwing an Exception if login could not be completed."
+  [username :- su/NonBlankString, password :- su/NonBlankString]
+  ;; Primitive "strategy implementation", should be reworked for modular providers in #3210
+  (or (ldap-login username password)    ; First try LDAP if it's enabled
+      (email-login username password)   ; Then try local authentication
+      ;; If nothing succeeded complain about it
+      ;; Don't leak whether the account doesn't exist or the password was incorrect
+      (throw
+       (ui18n/ex-info password-fail-message
+         {:status-code 400
+          :errors      {:password password-fail-snippet}}))))
+
 (api/defendpoint POST "/"
   "Login."
   [:as {{:keys [username password]} :body, remote-address :remote-addr}]
@@ -89,23 +104,17 @@
    password su/NonBlankString}
   (throttle-check (login-throttlers :ip-address) remote-address)
   (throttle-check (login-throttlers :username)   username)
-  ;; Primitive "strategy implementation", should be reworked for modular providers in #3210
-  (or (ldap-login username password)  ; First try LDAP if it's enabled
-      (email-login username password) ; Then try local authentication
-      ;; If nothing succeeded complain about it
-      ;; Don't leak whether the account doesn't exist or the password was incorrect
-      (throw (ui18n/ex-info password-fail-message
-               {:status-code 400
-                :errors      {:password password-fail-snippet}}))))
+  (let [session-id (login username password)
+        response   {:id session-id}]
+    (mw.session/set-session-cookie response session-id)))
 
 
 (api/defendpoint DELETE "/"
   "Logout."
-  [session_id]
-  {session_id su/NonBlankString}
-  (api/check-exists? Session session_id)
-  (db/delete! Session :id session_id)
-  api/generic-204-no-content)
+  [:as {:keys [metabase-session-id]}]
+  (api/check-exists? Session metabase-session-id)
+  (db/delete! Session :id metabase-session-id)
+  (mw.session/clear-session-cookie api/generic-204-no-content))
 
 ;; Reset tokens: We need some way to match a plaintext token with the a user since the token stored in the DB is
 ;; hashed. So we'll make the plaintext token in the format USER-ID_RANDOM-UUID, e.g.
@@ -231,12 +240,13 @@
   ;; things hairy and only enforce those for non-Google Auth users
   (user/create-new-google-auth-user! new-user))
 
-(defn- google-auth-fetch-or-create-user! [first-name last-name email]
-  (if-let [user (or (db/select-one [User :id :last_login] :email email)
-                    (google-auth-create-new-user! {:first_name first-name
-                                                   :last_name  last-name
-                                                   :email      email}))]
-    {:id (create-session! user)}))
+(s/defn ^:private google-auth-fetch-or-create-user! :- (s/maybe UUID)
+  [first-name last-name email]
+  (when-let [user (or (db/select-one [User :id :last_login] :email email)
+                      (google-auth-create-new-user! {:first_name first-name
+                                                     :last_name  last-name
+                                                     :email      email}))]
+    (create-session! user)))
 
 (api/defendpoint POST "/google_auth"
   "Login with Google Auth."
@@ -246,7 +256,9 @@
   ;; Verify the token is valid with Google
   (let [{:keys [given_name family_name email]} (google-auth-token-info token)]
     (log/info (trs "Successfully authenticated Google Auth token for: {0} {1}" given_name family_name))
-    (google-auth-fetch-or-create-user! given_name family_name email)))
+    (let [session-id (api/check-500 (google-auth-fetch-or-create-user! given_name family_name email))
+          response   {:id session-id}]
+      (mw.session/set-session-cookie response session-id))))
 
 
 (api/define-routes)

src/metabase/api/setup.clj

@@ -10,6 +10,7 @@
              [common :as api]
              [database :as database-api :refer [DBEngineString]]]
             [metabase.integrations.slack :as slack]
+            [metabase.middleware.session :as mw.session]
             [metabase.models
              [database :refer [Database]]
              [session :refer [Session]]
@@ -18,7 +19,8 @@
              [i18n :refer [tru]]
              [schema :as su]]
             [schema.core :as s]
-            [toucan.db :as db]))
+            [toucan.db :as db])
+  (:import java.util.UUID))
 
 (def ^:private SetupToken
   "Schema for a string that matches the instance setup token."
@@ -42,12 +44,12 @@
    allow_tracking (s/maybe (s/cond-pre s/Bool su/BooleanString))
    schedules      (s/maybe database-api/ExpandedSchedulesMap)}
   ;; Now create the user
-  (let [session-id (str (java.util.UUID/randomUUID))
+  (let [session-id (UUID/randomUUID)
         new-user   (db/insert! User
                      :email        email
                      :first_name   first_name
                      :last_name    last_name
-                     :password     (str (java.util.UUID/randomUUID))
+                     :password     (str (UUID/randomUUID))
                      :is_superuser true)]
     ;; this results in a second db call, but it avoids redundant password code so figure it's worth it
     (user/set-password! (:id new-user) password)
@@ -75,12 +77,12 @@
     (setup/clear-token!)
     ;; then we create a session right away because we want our new user logged in to continue the setup process
     (db/insert! Session
-      :id      session-id
+                :id      (str session-id)
       :user_id (:id new-user))
     ;; notify that we've got a new user in the system AND that this user logged in
     (events/publish-event! :user-create {:user_id (:id new-user)})
-    (events/publish-event! :user-login {:user_id (:id new-user), :session_id session-id, :first_login true})
-    {:id session-id}))
+    (events/publish-event! :user-login {:user_id (:id new-user), :session_id (str session-id), :first_login true})
+    (mw.session/set-session-cookie {:id (str session-id)} session-id)))
 
 
 (api/defendpoint POST "/validate"

src/metabase/api/user.clj

@@ -237,7 +237,8 @@
     (let [reset-token (user/set-password-reset-token! id)
           ;; NOTE: the new user join url is just a password reset with an indicator that this is a first time user
           join-url    (str (user/form-password-reset-url reset-token) "#new")]
-      (email/send-new-user-email! user @api/*current-user* join-url))))
+      (email/send-new-user-email! user @api/*current-user* join-url)))
+  {:success true})
 
 
 (api/define-routes)

src/metabase/middleware/session.clj

@@ -2,16 +2,46 @@
   "Ring middleware related to session (binding current user and permissions)."
   (:require [metabase
              [config :as config]
-             [db :as mdb]]
+             [db :as mdb]
+             [public-settings :as public-settings]]
             [metabase.api.common :refer [*current-user* *current-user-id* *current-user-permissions-set* *is-superuser?*]]
             [metabase.core.initialization-status :as init-status]
             [metabase.models
              [session :refer [Session]]
              [user :as user :refer [User]]]
-            [toucan.db :as db]))
-
-(def ^:private ^:const ^String metabase-session-cookie "metabase.SESSION_ID")
-(def ^:private ^:const ^String metabase-session-header "x-metabase-session")
+            [ring.util.response :as resp]
+            [schema.core :as s]
+            [toucan.db :as db])
+  (:import java.net.URL
+           java.util.UUID))
+
+(def ^:private ^String metabase-session-cookie "metabase.SESSION_ID")
+(def ^:private ^String metabase-session-header "x-metabase-session")
+
+(s/defn set-session-cookie
+  "Add a `Set-Cookie` header to `response` to persist the Metabase session."
+  [response, session-id :- UUID]
+  (if-not (and (map? response) (:body response))
+    (recur {:body response, :status 200} session-id)
+    (resp/set-cookie
+     response
+     metabase-session-cookie
+     (str session-id)
+     (merge
+      {:same-site :lax
+       :http-only true
+       :path      "/api"
+       :max-age   (config/config-int :max-session-age)}
+      ;; If Metabase is running over HTTPS (hopefully always except for local dev instances) then make sure to make this
+      ;; cookie HTTPS-only
+      (when (some-> (public-settings/site-url) URL. .getProtocol (= "https"))
+        {:secure true})))))
+
+(defn clear-session-cookie
+  "Add a header to `response` to clear the current Metabase session cookie."
+  [response]
+  (println "response:" response "->" (resp/set-cookie response nil {:expires "01 Jan 1970 00:00:00 GMT"})) ; NOCOMMIT
+  #_(resp/set-cookie response nil {:expires "01 Jan 1970 00:00:00 GMT"}))
 
 (defn- wrap-session-id* [{:keys [cookies headers] :as request}]
   (if-let [session-id (or (get-in cookies [metabase-session-cookie :value])

test/metabase/http_client.clj

@@ -2,12 +2,14 @@
   "HTTP client for making API calls against the Metabase API. For test/REPL purposes."
   (:require [cheshire.core :as json]
             [clj-http.client :as client]
-            [clojure.string :as s]
+            [clojure.string :as str]
             [clojure.tools.logging :as log]
             [metabase
              [config :as config]
              [util :as u]]
-            [metabase.util.date :as du]))
+            [metabase.middleware.session :as mw.session]
+            [metabase.util.date :as du]
+            [schema.core :as s]))
 
 ;;; build-url
 
@@ -22,7 +24,7 @@
   [url url-param-kwargs]
   {:pre [(string? url) (u/maybe? map? url-param-kwargs)]}
   (str *url-prefix* url (when (seq url-param-kwargs)
-                          (str "?" (s/join \& (for [[k v] url-param-kwargs]
+                          (str "?" (str/join \& (for [[k v] url-param-kwargs]
                                                 (str (if (keyword? k) (name k) k)
                                                      \=
                                                      (if (keyword? v) (name v) v))))))))
@@ -56,7 +58,7 @@
     (try
       (auto-deserialize-dates (json/parse-string body keyword))
       (catch Throwable _
-        (when-not (s/blank? body)
+        (when-not (str/blank? body)
           body)))))
 
 
@@ -64,15 +66,14 @@
 
 (declare client)
 
-(defn authenticate
+(s/defn authenticate
   "Authenticate a test user with USERNAME and PASSWORD, returning their Metabase Session token;
    or throw an Exception if that fails."
-  [{:keys [username password], :as credentials}]
-  {:pre [(string? username) (string? password)]}
+  [credentials :- {:username s/Str, :password s/Str}]
   (try
     (:id (client :post 200 "session" credentials))
     (catch Throwable e
-      (println "Failed to authenticate with username:" username "and password:" password ":" (.getMessage e)))))
+      (println "Failed to authenticate with credentials" credentials e))))
 
 
 ;;; client
@@ -80,10 +81,11 @@
 (defn- build-request-map [credentials http-body]
   (merge
    {:accept       :json
-    :headers      {"X-METABASE-SESSION" (when credentials
-                                          (if (map? credentials)
-                                            (authenticate credentials)
-                                            credentials))}
+    :headers      {@#'mw.session/metabase-session-header
+                   (when credentials
+                     (if (map? credentials)
+                       (authenticate credentials)
+                       credentials))}
     :content-type :json}
    (when (seq http-body)
      {:body (json/generate-string http-body)})))
@@ -119,7 +121,7 @@
   (let [request-map (merge (build-request-map credentials http-body) request-options)
         request-fn  (method->request-fn method)
         url         (build-url url url-param-kwargs)
-        method-name (s/upper-case (name method))
+        method-name (str/upper-case (name method))
         ;; Now perform the HTTP request
         {:keys [status body] :as resp} (try (request-fn url request-map)
                                             (catch clojure.lang.ExceptionInfo e