From d1e6b3a89558113b7cf6b3a0852fd446d1611a24 Mon Sep 17 00:00:00 2001
From: Tom Robinson <tlrobinson@gmail.com>
Date: Mon, 18 Mar 2019 11:55:54 -0700
Subject: [PATCH] Revert "Test fixes :wrench:; set session cookie when
resetting password"
This reverts commit 7b718205b48b0e5fa5fef08007d193f854525d0a.
---
src/metabase/api/session.clj | 7 ++--
src/metabase/api/setup.clj | 2 +-
src/metabase/middleware/session.clj | 15 ++++-----
test/expectation_options.clj | 50 +--------------------------
test/metabase/api/session_test.clj | 35 ++++++++-----------
test/metabase/test/data/users.clj | 42 +++++++++++------------
test/metabase/test/util.clj | 32 +++---------------
test/metabase/test_setup.clj | 52 +++++++++++++++++++++++++++--
test/metabase/util_test.clj | 2 +-
9 files changed, 99 insertions(+), 138 deletions(-)
diff --git a/src/metabase/api/session.clj b/src/metabase/api/session.clj
index 3b27ad0ee0c..618cadd9155 100644
--- a/src/metabase/api/session.clj
+++ b/src/metabase/api/session.clj
@@ -174,11 +174,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"))))
diff --git a/src/metabase/api/setup.clj b/src/metabase/api/setup.clj
index 34db8770dfb..a76b677f6c8 100644
--- a/src/metabase/api/setup.clj
+++ b/src/metabase/api/setup.clj
@@ -77,7 +77,7 @@
(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 (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)})
diff --git a/src/metabase/middleware/session.clj b/src/metabase/middleware/session.clj
index ecb64c7b7e7..c47a2c5418d 100644
--- a/src/metabase/middleware/session.clj
+++ b/src/metabase/middleware/session.clj
@@ -13,8 +13,7 @@
[schema.core :as s]
[toucan.db :as db])
(:import java.net.URL
- java.util.UUID
- org.joda.time.DateTime))
+ java.util.UUID))
(def ^:private ^String metabase-session-cookie "metabase.SESSION_ID")
(def ^:private ^String metabase-session-header "x-metabase-session")
@@ -41,14 +40,14 @@
(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)}))
+ (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}]
- (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.
diff --git a/test/expectation_options.clj b/test/expectation_options.clj
index 635fa87d7c3..f9dbc19aa97 100644
--- a/test/expectation_options.clj
+++ b/test/expectation_options.clj
@@ -1,53 +1,5 @@
(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 ----------------------------------------------
diff --git a/test/metabase/api/session_test.clj b/test/metabase/api/session_test.clj
index acf23547c4d..e46dbce332d 100644
--- a/test/metabase/api/session_test.clj
+++ b/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 --------------------------------------------
diff --git a/test/metabase/test/data/users.clj b/test/metabase/test/data/users.clj
index a714ccf73ba..edfa9d35e6e 100644
--- a/test/metabase/test/data/users.clj
+++ b/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
diff --git a/test/metabase/test/util.clj b/test/metabase/test/util.clj
index c25c43af326..c9b7dca0f02 100644
--- a/test/metabase/test/util.clj
+++ b/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]
diff --git a/test/metabase/test_setup.clj b/test/metabase/test_setup.clj
index ed47f29be45..8d23f9cbad0 100644
--- a/test/metabase/test_setup.clj
+++ b/test/metabase/test_setup.clj
@@ -1,8 +1,12 @@
(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]
diff --git a/test/metabase/util_test.clj b/test/metabase/util_test.clj
index d84afec8080..5a278d0aea3 100644
--- a/test/metabase/util_test.clj
+++ b/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
--
GitLab