Merge pull request #9577 from metabase/revert-http-only-cookies

Revert http-only-cookies branch

bin/docker/build_image.sh

@@ -43,7 +43,7 @@ if [ "$BUILD_TYPE" == "release" ]; then
     echo "Building Docker image ${DOCKER_IMAGE} from official Metabase release ${MB_TAG}"
 
     # download the official version of Metabase which matches our tag
-    curl -f -o ${BASEDIR}/metabase.jar http://downloads.metabase.com/${MB_TAG}/metabase.jar
+    curl -L -f -o ${BASEDIR}/metabase.jar http://downloads.metabase.com/${MB_TAG}/metabase.jar
 
     if [[ $? -ne 0 ]]; then
         echo "Download failed!"

frontend/src/metabase/auth/auth.js

@@ -6,6 +6,7 @@ import {
 
 import { push } from "react-router-redux";
 
+import MetabaseCookies from "metabase/lib/cookies";
 import MetabaseUtils from "metabase/lib/utils";
 import MetabaseAnalytics from "metabase/lib/analytics";
 import MetabaseSettings from "metabase/lib/settings";
@@ -35,8 +36,10 @@ export const login = createThunkAction(LOGIN, function(
     }
 
     try {
-      // NOTE: this request will return a Set-Cookie header for the session
-      await SessionApi.create(credentials);
+      let newSession = await SessionApi.create(credentials);
+
+      // since we succeeded, lets set the session cookie
+      MetabaseCookies.setSessionCookie(newSession.id);
 
       MetabaseAnalytics.trackEvent("Auth", "Login");
       // TODO: redirect after login (carry user to intended destination)
@@ -56,11 +59,13 @@ export const loginGoogle = createThunkAction(LOGIN_GOOGLE, function(
 ) {
   return async function(dispatch, getState) {
     try {
-      // NOTE: this request will return a Set-Cookie header for the session
-      await SessionApi.createWithGoogleAuth({
+      let newSession = await SessionApi.createWithGoogleAuth({
         token: googleUser.getAuthResponse().id_token,
       });
 
+      // since we succeeded, lets set the session cookie
+      MetabaseCookies.setSessionCookie(newSession.id);
+
       MetabaseAnalytics.trackEvent("Auth", "Google Auth Login");
 
       // TODO: redirect after login (carry user to intended destination)
@@ -82,12 +87,13 @@ export const loginGoogle = createThunkAction(LOGIN_GOOGLE, function(
 export const LOGOUT = "metabase/auth/LOGOUT";
 export const logout = createThunkAction(LOGOUT, function() {
   return function(dispatch, getState) {
-    // actively delete the session and remove the cookie
-    SessionApi.delete();
-
-    // clear Google auth credentials if any are present
-    clearGoogleAuthCredentials();
+    // TODO: as part of a logout we want to clear out any saved state that we have about anything
 
+    let sessionId = MetabaseCookies.setSessionCookie();
+    if (sessionId) {
+      // actively delete the session
+      SessionApi.delete({ session_id: sessionId });
+    }
     MetabaseAnalytics.trackEvent("Auth", "Logout");
 
     dispatch(push("/auth/login"));
@@ -112,12 +118,16 @@ export const passwordReset = createThunkAction(PASSWORD_RESET, function(
     }
 
     try {
-      // NOTE: this request will return a Set-Cookie header for the session
-      await SessionApi.reset_password({
+      let result = await SessionApi.reset_password({
         token: token,
         password: credentials.password,
       });
 
+      if (result.session_id) {
+        // we should have a valid session that we can use immediately!
+        MetabaseCookies.setSessionCookie(result.session_id);
+      }
+
       MetabaseAnalytics.trackEvent("Auth", "Password Reset");
 
       return {

frontend/src/metabase/setup/actions.js

+//import _ from "underscore";
 import { createAction } from "redux-actions";
 import { createThunkAction } from "metabase/lib/redux";
 
 import MetabaseAnalytics from "metabase/lib/analytics";
+import MetabaseCookies from "metabase/lib/cookies";
 import MetabaseSettings from "metabase/lib/settings";
 
 import { SetupApi, UtilApi } from "metabase/services";
@@ -48,7 +50,6 @@ export const submitSetup = createThunkAction(SUBMIT_SETUP, function() {
     let { setup: { allowTracking, databaseDetails, userDetails } } = getState();
 
     try {
-      // NOTE: this request will return a Set-Cookie header for the session
       let response = await SetupApi.create({
         token: MetabaseSettings.get("setup_token"),
         prefs: {
@@ -74,6 +75,9 @@ export const submitSetup = createThunkAction(SUBMIT_SETUP, function() {
 export const completeSetup = createAction(COMPLETE_SETUP, function(
   apiResponse,
 ) {
+  // setup user session
+  MetabaseCookies.setSessionCookie(apiResponse.id);
+
   // clear setup token from settings
   MetabaseSettings.setAll({ setup_token: null });
 

src/metabase/api/session.clj

@@ -13,7 +13,6 @@
             [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]]
@@ -24,21 +23,19 @@
              [schema :as su]]
             [schema.core :as s]
             [throttle.core :as throttle]
-            [toucan.db :as db])
-  (:import com.unboundid.util.LDAPSDKException
-           java.util.UUID))
+            [toucan.db :as db]))
 
-(s/defn ^:private create-session! :- UUID
+(defn- create-session!
   "Generate a new `Session` for a given `User`. Returns the newly generated session ID."
-  [user :- {:id         su/IntGreaterThanZero
-            :last_login s/Any
-            s/Keyword   s/Any}]
-  (u/prog1 (UUID/randomUUID)
+  [user]
+  {:pre  [(map? user) (integer? (:id user)) (contains? user :last_login)]
+   :post [(string? %)]}
+  (u/prog1 (str (java.util.UUID/randomUUID))
     (db/insert! Session
-      :id      (str <>)
+      :id      <>
       :user_id (:id user))
     (events/publish-event! :user-login
-      {:user_id (:id user), :session_id (str <>), :first_login (nil? (:last_login user))})))
+      {:user_id (:id user), :session_id <>, :first_login (not (boolean (:last_login user)))})))
 
 ;;; ## API Endpoints
 
@@ -50,7 +47,7 @@
 (def ^:private password-fail-message (tru "Password did not match stored password."))
 (def ^:private password-fail-snippet (tru "did not match stored password"))
 
-(s/defn ^:private ldap-login :- (s/maybe UUID)
+(defn- ldap-login
   "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]
@@ -64,16 +61,18 @@
                    {:status-code 400
                     :errors      {:password password-fail-snippet}})))
         ;; password is ok, return new session
-        (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"))))))
+        {: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))))))))
 
-(s/defn ^:private email-login :- (s/maybe UUID)
+(defn- email-login
   "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))
-      (create-session! user))))
+      {:id (create-session! user)})))
 
 (def ^:private throttling-disabled? (config/config-bool :mb-disable-session-throttle))
 
@@ -83,20 +82,6 @@
   (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}]
@@ -104,17 +89,23 @@
    password su/NonBlankString}
   (throttle-check (login-throttlers :ip-address) remote-address)
   (throttle-check (login-throttlers :username)   username)
-  (let [session-id (login username password)
-        response   {:id session-id}]
-    (mw.session/set-session-cookie response session-id)))
+  ;; 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 DELETE "/"
   "Logout."
-  [: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))
+  [session_id]
+  {session_id su/NonBlankString}
+  (api/check-exists? Session session_id)
+  (db/delete! Session :id session_id)
+  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.
@@ -174,11 +165,8 @@
         (when-not (:last_login user)
           (email/send-user-joined-admin-notification-email! (User user-id)))
         ;; after a successful password update go ahead and offer the client a new session that they can use
-        (let [session-id (create-session! user)]
-          (mw.session/set-session-cookie
-           {:success    true
-            :session_id (str session-id)}
-           session-id)))
+        {:success    true
+         :session_id (create-session! user)})
       (api/throw-invalid-param-exception :password (tru "Invalid reset token"))))
 
 
@@ -243,13 +231,12 @@
   ;; things hairy and only enforce those for non-Google Auth users
   (user/create-new-google-auth-user! new-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)))
+(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)}))
 
 (api/defendpoint POST "/google_auth"
   "Login with Google Auth."
@@ -259,9 +246,7 @@
   ;; 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))
-    (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))))
+    (google-auth-fetch-or-create-user! given_name family_name email)))
 
 
 (api/define-routes)

src/metabase/api/setup.clj

@@ -10,7 +10,6 @@
              [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]]
@@ -19,8 +18,7 @@
              [i18n :refer [tru]]
              [schema :as su]]
             [schema.core :as s]
-            [toucan.db :as db])
-  (:import java.util.UUID))
+            [toucan.db :as db]))
 
 (def ^:private SetupToken
   "Schema for a string that matches the instance setup token."
@@ -44,12 +42,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 (UUID/randomUUID)
+  (let [session-id (str (java.util.UUID/randomUUID))
         new-user   (db/insert! User
                      :email        email
                      :first_name   first_name
                      :last_name    last_name
-                     :password     (str (UUID/randomUUID))
+                     :password     (str (java.util.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)
@@ -77,12 +75,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      (str session-id)
+      :id      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 (str session-id), :first_login true})
-    (mw.session/set-session-cookie {:id (str session-id)} session-id)))
+    (events/publish-event! :user-login {:user_id (:id new-user), :session_id session-id, :first_login true})
+    {:id session-id}))
 
 
 (api/defendpoint POST "/validate"

src/metabase/api/user.clj

@@ -237,8 +237,7 @@
     (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)))
-  {:success true})
+      (email/send-new-user-email! user @api/*current-user* join-url))))
 
 
 (api/define-routes)

src/metabase/middleware/session.clj

@@ -2,53 +2,22 @@
   "Ring middleware related to session (binding current user and permissions)."
   (:require [metabase
              [config :as config]
-             [db :as mdb]
-             [public-settings :as public-settings]]
+             [db :as mdb]]
             [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]]]
-            [ring.util.response :as resp]
-            [schema.core :as s]
-            [toucan.db :as db])
-  (:import java.net.URL
-           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")
-
-(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]
-  (resp/set-cookie response metabase-session-cookie nil {:expires (DateTime. 0)}))
+            [toucan.db :as db]))
+
+(def ^:private ^:const ^String metabase-session-cookie "metabase.SESSION_ID")
+(def ^:private ^:const ^String metabase-session-header "x-metabase-session")
 
 (defn- wrap-session-id* [{:keys [cookies headers] :as request}]
-  (let [session-id (or (get-in cookies [metabase-session-cookie :value])
-                       (headers metabase-session-header))]
-    (if (seq session-id)
-      (assoc request :metabase-session-id session-id)
-      request)))
+  (if-let [session-id (or (get-in cookies [metabase-session-cookie :value])
+                          (headers metabase-session-header))]
+    (assoc request :metabase-session-id session-id)
+    request))
 
 (defn wrap-session-id
   "Middleware that sets the `:metabase-session-id` keyword on the request if a session id can be found.

test/expectation_options.clj

 (ns expectation-options
-  "Namespace expectations will automatically load before running a tests"
-  (:require [clojure
-             [data :as data]
-             [set :as set]]
-            [expectations :as expectations]
-            [metabase.util :as u]))
-
-;;; ---------------------------------------- Expectations Framework Settings -----------------------------------------
-
-;; ## EXPECTATIONS FORMATTING OVERRIDES
-
-;; These overrides the methods Expectations usually uses for printing failed tests.
-;; These are basically the same as the original implementations, but they colorize and pretty-print the
-;; output, which makes it an order of magnitude easier to read, especially for tests that compare a
-;; lot of data, like Query Processor or API tests.
-(defn- format-failure [e a str-e str-a]
-  {:type             :fail
-   :expected-message (when-let [in-e (first (data/diff e a))]
-                       (format "\nin expected, not actual:\n%s" (u/pprint-to-str 'green in-e)))
-   :actual-message   (when-let [in-a (first (data/diff a e))]
-                       (format "\nin actual, not expected:\n%s" (u/pprint-to-str 'red in-a)))
-   :raw              [str-e str-a]
-   :result           ["\nexpected:\n"
-                      (u/pprint-to-str 'green e)
-                      "\nwas:\n"
-                      (u/pprint-to-str 'red a)]})
-
-(defmethod expectations/compare-expr :expectations/maps [e a str-e str-a]
-  (let [[in-e in-a] (data/diff e a)]
-    (if (and (nil? in-e) (nil? in-a))
-      {:type :pass}
-      (format-failure e a str-e str-a))))
-
-(defmethod expectations/compare-expr :expectations/sets [e a str-e str-a]
-  (format-failure e a str-e str-a))
-
-(defmethod expectations/compare-expr :expectations/sequentials [e a str-e str-a]
-  (let [diff-fn (fn [e a] (seq (set/difference (set e) (set a))))]
-    (assoc (format-failure e a str-e str-a)
-           :message (cond
-                      (and (= (set e) (set a))
-                           (= (count e) (count a))
-                           (= (count e) (count (set a)))) "lists appear to contain the same items with different ordering"
-                      (and (= (set e) (set a))
-                           (< (count e) (count a)))       "some duplicate items in actual are not expected"
-                      (and (= (set e) (set a))
-                           (> (count e) (count a)))       "some duplicate items in expected are not actual"
-                      (< (count e) (count a))             "actual is larger than expected"
-                      (> (count e) (count a))             "expected is larger than actual"))))
+  "Namespace expectations will automatically load before running a tests")
 
 ;;; ---------------------------------------------- check-for-slow-tests ----------------------------------------------
 

test/metabase/api/session_test.clj

@@ -13,12 +13,11 @@
             [metabase.test
              [data :refer :all]
              [util :as tu]]
-            [metabase.test.data.users :as test-users :refer :all]
+            [metabase.test.data.users :refer :all]
             [metabase.test.integrations.ldap :refer [expect-with-ldap-server]]
             [metabase.test.util.log :as tu.log]
             [toucan.db :as db]
-            [toucan.util.test :as tt])
-  (:import java.util.UUID))
+            [toucan.util.test :as tt]))
 
 ;; ## POST /api/session
 ;; Test that we can login
@@ -62,16 +61,10 @@
 ;; Test that we can logout
 (expect
   nil
-  (do
-    ;; clear out cached session tokens so next time we make an API request it log in & we'll know we have a valid
-    ;; Session
-    (test-users/clear-cached-session-tokens!)
-    (let [session-id (test-users/username->token :rasta)]
-      ;; Ok, calling the logout endpoint should delete the Session in the DB. Don't worry, `test-users` will log back
-      ;; in on the next API call
-      ((user->client :rasta) :delete 204 "session")
-      ;; check whether it's still there -- should be GONE
-      (Session session-id))))
+  (let [{session_id :id} ((user->client :rasta) :post 200 "session" (user->credentials :rasta))]
+    (assert session_id)
+    ((user->client :rasta) :delete 204 "session" :session_id session_id)
+    (Session session_id)))
 
 
 ;; ## POST /api/session/forgot_password
@@ -241,15 +234,18 @@
                    [:first_name :last_name :email]))))
 
 
-;;; --------------------------------------- google-auth-fetch-or-create-user! ----------------------------------------
+;;; tests for google-auth-fetch-or-create-user!
+
+(defn- is-session? [session]
+  (u/ignore-exceptions
+    (tu/is-uuid-string? (:id session))))
 
 ;; test that an existing user can log in with Google auth even if the auto-create accounts domain is different from
 ;; their account should return a Session
 (expect
-  UUID
   (tt/with-temp User [user {:email "cam@sf-toucannery.com"}]
     (tu/with-temporary-setting-values [google-auth-auto-create-accounts-domain "metabase.com"]
-      (#'session-api/google-auth-fetch-or-create-user! "Cam" "Saul" "cam@sf-toucannery.com"))))
+      (is-session? (#'session-api/google-auth-fetch-or-create-user! "Cam" "Saül" "cam@sf-toucannery.com")))))
 
 ;; test that a user that doesn't exist with a *different* domain than the auto-create accounts domain gets an
 ;; exception
@@ -262,14 +258,11 @@
 ;; test that a user that doesn't exist with the *same* domain as the auto-create accounts domain means a new user gets
 ;; created
 (expect
-  UUID
   (et/with-fake-inbox
     (tu/with-temporary-setting-values [google-auth-auto-create-accounts-domain "sf-toucannery.com"
                                        admin-email                             "rasta@toucans.com"]
-      (try
-        (#'session-api/google-auth-fetch-or-create-user! "Rasta" "Toucan" "rasta@sf-toucannery.com")
-        (finally
-          (db/delete! User :email "rasta@sf-toucannery.com")))))) ; clean up after ourselves
+      (u/prog1 (is-session? (#'session-api/google-auth-fetch-or-create-user! "Rasta" "Toucan" "rasta@sf-toucannery.com"))
+        (db/delete! User :email "rasta@sf-toucannery.com"))))) ; clean up after ourselves
 
 
 ;;; ------------------------------------------- TESTS FOR LDAP AUTH STUFF --------------------------------------------

test/metabase/http_client.clj

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

test/metabase/test/data/users.clj

@@ -8,10 +8,8 @@
             [metabase.core.initialization-status :as init-status]
             [metabase.middleware.session :as mw.session]
             [metabase.models.user :as user :refer [User]]
-            [schema.core :as s]
             [toucan.db :as db])
-  (:import clojure.lang.ExceptionInfo
-           metabase.models.user.UserInstance))
+  (:import clojure.lang.ExceptionInfo))
 
 ;;; ------------------------------------------------ User Definitions ------------------------------------------------
 
@@ -44,12 +42,9 @@
                :password "birdseed"
                :active   false}})
 
-(def ^:private usernames
+(def ^:private ^:const usernames
   (set (keys user->info)))
 
-(def ^:private TestUserName
-  (apply s/enum usernames))
-
 ;;; ------------------------------------------------- Test User Fns --------------------------------------------------
 
 (defn- wait-for-initiailization
@@ -72,8 +67,8 @@
 (defn- fetch-or-create-user!
   "Create User if they don't already exist and return User."
   [& {:keys [email first last password superuser active]
-      :or   {superuser false
-             active    true}}]
+      :or {superuser false
+           active    true}}]
   {:pre [(string? email) (string? first) (string? last) (string? password) (m/boolean? superuser) (m/boolean? active)]}
   (wait-for-initiailization)
   (or (User :email email)
@@ -87,18 +82,19 @@
         :is_active    active)))
 
 
-(s/defn fetch-user :- UserInstance
+(defn fetch-user
   "Fetch the User object associated with USERNAME. Creates user if needed.
 
     (fetch-user :rasta) -> {:id 100 :first_name \"Rasta\" ...}"
-  [username :- TestUserName]
+  [username]
+  {:pre [(contains? usernames username)]}
   (m/mapply fetch-or-create-user! (user->info username)))
 
-(s/defn create-users-if-needed!
+(defn create-users-if-needed!
   "Force creation of the test users if they don't already exist."
   ([]
    (apply create-users-if-needed! usernames))
-  ([& usernames :- [TestUserName]]
+  ([& usernames]
    (doseq [username usernames]
      ;; fetch-user will force creation of users
      (fetch-user username))))
@@ -108,15 +104,15 @@
 
     (user->id :rasta) -> 4"
   (memoize
-   (s/fn :- s/Int [username :- TestUserName]
+   (fn [username]
      {:pre [(contains? usernames username)]}
-     (u/get-id (fetch-user username)))))
+     (:id (fetch-user username)))))
 
-(s/defn user->credentials :- {:username (s/pred u/email?), :password s/Str}
+(defn user->credentials
   "Return a map with `:username` and `:password` for User with USERNAME.
 
     (user->credentials :rasta) -> {:username \"rasta@metabase.com\", :password \"blueberries\"}"
-  [username :- TestUserName]
+  [username]
   {:pre [(contains? usernames username)]}
   (let [{:keys [email password]} (user->info username)]
     {:username email
@@ -132,9 +128,9 @@
 
 (defonce ^:private tokens (atom {}))
 
-(s/defn username->token :- u/uuid-regex
+(defn username->token
   "Return cached session token for a test User, logging in first if needed."
-  [username :- TestUserName]
+  [username]
   (or (@tokens username)
       (u/prog1 (http/authenticate (user->credentials username))
         (swap! tokens assoc username <>))
@@ -158,18 +154,18 @@
         (clear-cached-session-tokens!)
         (apply client-fn username args)))))
 
-(s/defn user->client :- (s/pred fn?)
+(defn user->client
   "Returns a `metabase.http-client/client` partially bound with the credentials for User with USERNAME.
    In addition, it forces lazy creation of the User if needed.
 
      ((user->client) :get 200 \"meta/table\")"
-  [username :- TestUserName]
+  [username]
   (create-users-if-needed! username)
   (partial client-fn username))
 
-(s/defn do-with-test-user
+(defn do-with-test-user
   "Call `f` with various `metabase.api.common` dynamic vars bound to the test User named by `user-kwd`."
-  [user-kwd :- TestUserName, f :- (s/pred fn?)]
+  [user-kwd f]
   ((mw.session/bind-current-user (fn [_ respond _] (respond (f))))
    (let [user-id (user->id user-kwd)]
      {:metabase-user-id user-id

test/metabase/test/util.clj

@@ -3,11 +3,11 @@
   (:require [cheshire.core :as json]
             [clj-time.core :as time]
             [clojure
-             [string :as str]
+             [string :as s]
              [walk :as walk]]
             [clojure.tools.logging :as log]
             [clojurewerkz.quartzite.scheduler :as qs]
-            [expectations :as expectations :refer [expect]]
+            [expectations :refer :all]
             [metabase
              [driver :as driver]
              [task :as task]
@@ -35,35 +35,11 @@
             [metabase.test.data :as data]
             [metabase.test.data.dataset-definitions :as defs]
             [metabase.util.date :as du]
-            [schema.core :as s]
             [toucan.db :as db]
             [toucan.util.test :as test])
   (:import org.apache.log4j.Logger
            [org.quartz CronTrigger JobDetail JobKey Scheduler Trigger]))
 
-;; record type for testing that results match a Schema
-(defrecord SchemaExpectation [schema]
-  expectations/CustomPred
-  (expect-fn [_ actual]
-    (nil? (s/check schema actual)))
-  (expected-message [_ _ _ _]
-    (str "Result did not match schema:\n"
-         (u/pprint-to-str (s/explain schema))))
-  (actual-message [_ actual _ _]
-    (str "Was:\n"
-         (u/pprint-to-str actual)))
-  (message [_ actual _ _]
-    (u/pprint-to-str (s/check schema actual))))
-
-(defmacro expect-schema
-  "Like `expect`, but checks that results match a schema."
-  {:style/indent 0}
-  [expected actual]
-  `(expect
-     (SchemaExpectation. ~expected)
-     ~actual))
-
-
 ;;; ---------------------------------------------------- match-$ -----------------------------------------------------
 
 (defn- $->prop
@@ -132,8 +108,8 @@
     (every-pred (some-fn keyword? string?)
                 (some-fn #{:id :created_at :updated_at :last_analyzed :created-at :updated-at :field-value-id :field-id
                            :fields_hash :date_joined :date-joined :last_login :dimension-id :human-readable-field-id}
-                         #(str/ends-with? % "_id")
-                         #(str/ends-with? % "_at")))
+                         #(s/ends-with? % "_id")
+                         #(s/ends-with? % "_at")))
     data))
   ([pred data]
    (walk/prewalk (fn [maybe-map]

test/metabase/test_setup.clj

 (ns metabase.test-setup
   "Functions that run before + after unit tests (setup DB, start web server, load test data)."
-  (:require [clojure.java.io :as io]
-            [clojure.string :as str]
+  (:require [clojure
+             [data :as data]
+             [set :as set]
+             [string :as str]]
+            [clojure.java.io :as io]
             [clojure.tools.logging :as log]
+            [expectations :refer :all]
             [metabase
              [db :as mdb]
              [handler :as handler]
@@ -16,6 +20,50 @@
             [metabase.test.data.env :as tx.env]
             [yaml.core :as yaml]))
 
+;;; ---------------------------------------- Expectations Framework Settings -----------------------------------------
+
+;; ## EXPECTATIONS FORMATTING OVERRIDES
+
+;; These overrides the methods Expectations usually uses for printing failed tests.
+;; These are basically the same as the original implementations, but they colorize and pretty-print the
+;; output, which makes it an order of magnitude easier to read, especially for tests that compare a
+;; lot of data, like Query Processor or API tests.
+(defn- format-failure [e a str-e str-a]
+  {:type             :fail
+   :expected-message (when-let [in-e (first (data/diff e a))]
+                       (format "\nin expected, not actual:\n%s" (u/pprint-to-str 'green in-e)))
+   :actual-message   (when-let [in-a (first (data/diff a e))]
+                       (format "\nin actual, not expected:\n%s" (u/pprint-to-str 'red in-a)))
+   :raw              [str-e str-a]
+   :result           ["\nexpected:\n"
+                      (u/pprint-to-str 'green e)
+                      "\nwas:\n"
+                      (u/pprint-to-str 'red a)]})
+
+(defmethod compare-expr :expectations/maps [e a str-e str-a]
+  (let [[in-e in-a] (data/diff e a)]
+    (if (and (nil? in-e) (nil? in-a))
+      {:type :pass}
+      (format-failure e a str-e str-a))))
+
+(defmethod compare-expr :expectations/sets [e a str-e str-a]
+  (format-failure e a str-e str-a))
+
+(defmethod compare-expr :expectations/sequentials [e a str-e str-a]
+  (let [diff-fn (fn [e a] (seq (set/difference (set e) (set a))))]
+    (assoc (format-failure e a str-e str-a)
+           :message (cond
+                      (and (= (set e) (set a))
+                           (= (count e) (count a))
+                           (= (count e) (count (set a)))) "lists appear to contain the same items with different ordering"
+                      (and (= (set e) (set a))
+                           (< (count e) (count a)))       "some duplicate items in actual are not expected"
+                      (and (= (set e) (set a))
+                           (> (count e) (count a)))       "some duplicate items in expected are not actual"
+                      (< (count e) (count a))             "actual is larger than expected"
+                      (> (count e) (count a))             "expected is larger than actual"))))
+
+
 ;;; ------------------------------- Functions That Get Ran On Test Suite Start / Stop --------------------------------
 
 (defn- driver-plugin-manifest [driver]

test/metabase/util_test.clj

@@ -65,7 +65,7 @@
 (expect "cam_s_awesome_toucan_emporium" (slugify "Cam's awesome toucan emporium"))
 (expect "frequently_used_cards"         (slugify "Frequently-Used Cards"))
 ;; check that diactrics get removed
-(expect "cam_saul_s_toucannery"         (slugify "Cam Saul's Toucannery"))
+(expect "cam_saul_s_toucannery"         (slugify "Cam Saül's Toucannery"))
 (expect "toucans_dislike_pinatas___"    (slugify "toucans dislike piñatas :("))
 ;; check that non-ASCII characters get URL-encoded (so we can support non-Latin alphabet languages; see #3818)
 (expect "%E5%8B%87%E5%A3%AB"            (slugify "勇士")) ; go dubs