From 80ca119017bedd46586c1f752bc0a310ddab2e13 Mon Sep 17 00:00:00 2001
From: Cam Saul <cammsaul@gmail.com>
Date: Thu, 21 Mar 2019 12:34:45 -0700
Subject: [PATCH] Change HttpOnly cookie name -> metabase.SESSION
---
frontend/src/metabase/lib/cookies.js | 31 +--------
.../test/legacy-selenium/auth/login.spec.js | 6 +-
src/metabase/middleware/session.clj | 67 +++++++++++++------
3 files changed, 54 insertions(+), 50 deletions(-)
diff --git a/frontend/src/metabase/lib/cookies.js b/frontend/src/metabase/lib/cookies.js
index 860ccb863ce..1b934a75485 100644
--- a/frontend/src/metabase/lib/cookies.js
+++ b/frontend/src/metabase/lib/cookies.js
@@ -1,38 +1,11 @@
-import { clearGoogleAuthCredentials } from "metabase/lib/auth";
-
import Cookies from "js-cookie";
-export const METABASE_SESSION_COOKIE = "metabase.SESSION_ID";
+// METABASE_SESSION_COOKIE is only used for e2e tests. In normal usage cookie is set automatically by login endpoints
+export const METABASE_SESSION_COOKIE = "metabase.SESSION";
export const METABASE_SEEN_ALERT_SPLASH_COOKIE = "metabase.SEEN_ALERT_SPLASH";
// Handles management of Metabase cookie work
let MetabaseCookies = {
- // set the session cookie. if sessionId is null, clears the cookie
- setSessionCookie: function(sessionId) {
- const options = {
- path: window.MetabaseRoot || "/",
- expires: 14,
- secure: window.location.protocol === "https:",
- };
-
- try {
- if (sessionId) {
- // set a session cookie
- Cookies.set(METABASE_SESSION_COOKIE, sessionId, options);
- } else {
- sessionId = Cookies.get(METABASE_SESSION_COOKIE);
-
- // delete the current session cookie and Google Auth creds
- Cookies.remove(METABASE_SESSION_COOKIE);
- clearGoogleAuthCredentials();
-
- return sessionId;
- }
- } catch (e) {
- console.error("setSessionCookie:", e);
- }
- },
-
setHasSeenAlertSplash: hasSeen => {
const options = {
path: window.MetabaseRoot || "/",
diff --git a/frontend/test/legacy-selenium/auth/login.spec.js b/frontend/test/legacy-selenium/auth/login.spec.js
index c2d77233c96..624be75d3ca 100644
--- a/frontend/test/legacy-selenium/auth/login.spec.js
+++ b/frontend/test/legacy-selenium/auth/login.spec.js
@@ -10,6 +10,8 @@ import {
describeE2E,
} from "../support/utils";
+import { METABASE_SESSION_COOKIE } from "metabase/lib/cookies";
+
jasmine.DEFAULT_TIMEOUT_INTERVAL = 60000;
describeE2E("auth/login", () => {
@@ -36,7 +38,7 @@ describeE2E("auth/login", () => {
await waitForUrl(driver, `${server.host}/`);
const sessionCookie = await driver
.manage()
- .getCookie("metabase.SESSION_ID");
+ .getCookie(METABASE_SESSION_COOKIE);
sessionId = sessionCookie.value;
});
@@ -55,7 +57,7 @@ describeE2E("auth/login", () => {
beforeEach(async () => {
await driver.get(`${server.host}/`);
await driver.manage().deleteAllCookies();
- await driver.manage().addCookie("metabase.SESSION_ID", sessionId);
+ await driver.manage().addCookie(METABASE_SESSION_COOKIE, sessionId);
});
it("is logged in", async () => {
diff --git a/src/metabase/middleware/session.clj b/src/metabase/middleware/session.clj
index ecb64c7b7e7..4757e14593c 100644
--- a/src/metabase/middleware/session.clj
+++ b/src/metabase/middleware/session.clj
@@ -4,7 +4,8 @@
[config :as config]
[db :as mdb]
[public-settings :as public-settings]]
- [metabase.api.common :refer [*current-user* *current-user-id* *current-user-permissions-set* *is-superuser?*]]
+ [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]]
@@ -16,35 +17,63 @@
java.util.UUID
org.joda.time.DateTime))
-(def ^:private ^String metabase-session-cookie "metabase.SESSION_ID")
-(def ^:private ^String metabase-session-header "x-metabase-session")
+;; How do authenticated API requests work? Metabase first looks for a cookie called `metabase.SESSION`. This is the
+;; normal way of doing things; this cookie gets set automatically upon login. `metabase.SESSION` is an HttpOnly
+;; cookie and thus can't be viewed by FE code.
+;;
+;; If that cookie is isn't present, we look for the `metabase.SESSION_ID`, which is the old session cookie set in
+;; 0.31.x and older. Unlike `metabase.SESSION`, this cookie was set directly by the frontend and thus was not
+;; HttpOnly; for 0.32.x we'll continue to accept it rather than logging every one else out on upgrade. (We've
+;; switched to a new Cookie name for 0.32.x because the new cookie includes a `path` attribute, thus browsers consider
+;; it to be a different Cookie; Ring cookie middleware does not handle multiple cookies with the same name.)
+;;
+;; Finally we'll check for the presence of a `X-Metabase-Session` header. If that isn't present, you don't have a
+;; Session ID and thus are definitely not authenticated
+(def ^:private ^String metabase-session-cookie "metabase.SESSION")
+(def ^:private ^String metabase-legacy-session-cookie "metabase.SESSION_ID") ; this can be removed in 0.33.x
+(def ^:private ^String metabase-session-header "x-metabase-session")
+
+(defn- clear-cookie [response cookie-name]
+ (resp/set-cookie response cookie-name nil {:expires (DateTime. 0)}))
+
+(defn- wrap-body-if-needed
+ "You can't add a cookie (by setting the `:cookies` key of a response) if the response is an unwrapped JSON response;
+ wrap `response` if needed."
+ [response]
+ (if (and (map? response) (contains? response :body))
+ response
+ {:body response, :status 200}))
(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})))))
+ (-> response
+ wrap-body-if-needed
+ (clear-cookie metabase-legacy-session-cookie)
+ (resp/set-cookie
+ 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]
- (resp/set-cookie response metabase-session-cookie nil {:expires (DateTime. 0)}))
+ (-> response
+ wrap-body-if-needed
+ (clear-cookie metabase-session-cookie)
+ (clear-cookie metabase-legacy-session-cookie)))
(defn- wrap-session-id* [{:keys [cookies headers] :as request}]
(let [session-id (or (get-in cookies [metabase-session-cookie :value])
+ (get-in cookies [metabase-legacy-session-cookie :value])
(headers metabase-session-header))]
(if (seq session-id)
(assoc request :metabase-session-id session-id)
--
GitLab