From 852a3193abf94f4f7f1393b8f17caf7bed1dc8d4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Arne=20Schl=C3=BCter?= Date: Mon, 16 Jul 2018 17:20:33 +0200 Subject: [PATCH] Make login-flow more robust and add more tests --- src/cljs/airsonic_ui/db.cljs | 3 +- src/cljs/airsonic_ui/events.cljs | 71 ++++++++++++----------- src/cljs/airsonic_ui/routes.cljs | 2 +- src/cljs/airsonic_ui/subs.cljs | 8 +++ src/cljs/airsonic_ui/views.cljs | 11 ++-- test/cljs/airsonic_ui/events_test.cljs | 40 +++++++------ test/cljs/airsonic_ui/fixtures.cljs | 10 +++- test/cljs/airsonic_ui/subs_test.cljs | 35 ++++++++++- test/cljs/airsonic_ui/utils/api_test.cljs | 8 +++ 9 files changed, 123 insertions(+), 65 deletions(-) diff --git a/src/cljs/airsonic_ui/db.cljs b/src/cljs/airsonic_ui/db.cljs index bc5ef7e..26d54e0 100644 --- a/src/cljs/airsonic_ui/db.cljs +++ b/src/cljs/airsonic_ui/db.cljs @@ -1,5 +1,4 @@ -(ns airsonic-ui.db - (:require [airsonic-ui.routes :as routes])) +(ns airsonic-ui.db) (def default-db {:notifications (sorted-map)}) diff --git a/src/cljs/airsonic_ui/events.cljs b/src/cljs/airsonic_ui/events.cljs index 34ed863..2861009 100644 --- a/src/cljs/airsonic_ui/events.cljs +++ b/src/cljs/airsonic_ui/events.cljs @@ -12,6 +12,11 @@ (fn [params] (apply println params))) +(defn noop + "An event handler that can be used for clarity; doesn't do anything, but might + give a name to an event" + [cofx _] cofx) + ;; --- ;; app boot flow ;; * restoring a previous session @@ -31,10 +36,9 @@ troubles with our router." [{:keys [db store]} _] (let [credentials (:credentials store)] - {:db (assoc db :credentials credentials) - :dispatch-n [(if credentials + {:dispatch-n [(if credentials [:init-flow/credentials-found credentials] - [:init-flow/credentials-missing])] + [:init-flow/credentials-not-found])] :routes/start-routing nil})) (re-frame/reg-event-fx @@ -45,19 +49,21 @@ (defn credentials-found [_ [_ {:keys [u p server]}]] {:dispatch [:credentials/verification-request u p server]}) -(re-frame/reg-event-fx - :init-flow/credentials-found credentials-found) +(re-frame/reg-event-fx :init-flow/credentials-found credentials-found) -(re-frame/reg-event-fx - :init-flow/credentials-missing - ;; we don't do anything special here, it's just for the sake of clarity - (fn [_ _] {})) +;; we don't do anything special here, it's just for the sake of clarity + +(defn credentials-not-found + [cofx _] + (assoc-in cofx [:db :credentials] :credentials/not-found)) + +(re-frame/reg-event-fx :init-flow/credentials-not-found credentials-not-found) ;; --- ;; auth logic ;; --- -(defn-traced credentials-verification-request +(defn credentials-verification-request "Tries to authenticate a user by pinging the server with credentials, saving them when the request was successful. Bypasses the request when a user saved their credentials." @@ -66,21 +72,25 @@ :uri (api/url server "ping" {:u user :p pass}) :response-format (ajax/json-response-format {:keywords? true}) :on-success [:credentials/verification-response user pass server] - :on-failure [:api/bad-response]}}) + :on-failure [:credentials/verification-failure]}}) -(re-frame/reg-event-fx - :credentials/verification-request credentials-verification-request) +(re-frame/reg-event-fx :credentials/verification-request credentials-verification-request) (defn credentials-verification-response "Since we don't get real status codes, we have to look into the server's response and see whether we actually sent the correct credentials" [fx [_ user pass server response]] {:dispatch (if (api/is-error? response) - [:notification/show :error (api/error-msg (api/->exception response))] + [:credentials/verification-failure response] [:credentials/verified user pass server])}) -(re-frame/reg-event-fx - :credentials/verification-response credentials-verification-response) +(re-frame/reg-event-fx :credentials/verification-response credentials-verification-response) + +(defn credentials-verification-failure [fx [_ response]] + (-> (assoc-in fx [:db :credentials] :credentials/verification-failure) + (assoc :dispatch [:notification/show :error (api/error-msg (api/->exception response))]))) + +(re-frame/reg-event-fx :credentials/verification-failure credentials-verification-failure) (defn credentials-verified "Gets called after the server indicates that the credentials entered by a user @@ -92,8 +102,7 @@ :db (assoc db :credentials credentials) :dispatch [::logged-in]})) -(re-frame/reg-event-fx - :credentials/verified credentials-verified) +(re-frame/reg-event-fx :credentials/verified credentials-verified) ;; TODO: We have to find another solution for this once we have routes that ;; don't require a login but have the bottom controls @@ -103,10 +112,10 @@ (fn [_] (.. js/document -documentElement -classList (add "has-navbar-fixed-bottom")))) + (defn logged-in [cofx _] - (let [redirect (or (get-in cofx [:routes/from-query-param :redirect]) - [::routes/main])] + (let [redirect (or (get-in cofx [:routes/from-query-param :redirect]) [::routes/main])] {:routes/navigate redirect :show-nav-bar nil})) @@ -117,17 +126,16 @@ (defn logout "Clears all credentials and redirects the user to the login page" - [_ [_ & args]] + [cofx [_ & args]] (let [args (apply hash-map args)] {:routes/navigate (if-let [redirect (:redirect-to args)] [::routes/login {} {:redirect (routes/encode-route redirect)}] [::routes/login]) :routes/unset-credentials nil :store nil - :db db/default-db})) + :db (merge (:db cofx) db/default-db {:credentials :credentials/logged-out})})) -(re-frame/reg-event-fx - ::logout logout) +(re-frame/reg-event-fx ::logout logout) ;; --- ;; api interaction @@ -144,8 +152,7 @@ :on-success [:api/good-response] :on-failure [:api/bad-response]}}) -(re-frame/reg-event-fx - :api/request api-request) +(re-frame/reg-event-fx :api/request api-request) (defn good-api-response [fx [_ response]] (try @@ -153,15 +160,13 @@ (catch ExceptionInfo e {:dispatch [:notification/show :error (api/error-msg e)]}))) -(re-frame/reg-event-fx - :api/good-response good-api-response) +(re-frame/reg-event-fx :api/good-response good-api-response) (defn bad-api-response [db event] {:log ["API call gone bad; are CORS headers missing? check for :status 0" event] :dispatch [:notification/show :error "Communication with server failed. Check browser logs for details."]}) -(re-frame/reg-event-fx - :api/bad-response bad-api-response) +(re-frame/reg-event-fx :api/bad-response bad-api-response) ;; --- ;; musique @@ -256,12 +261,10 @@ :message message}) (assoc :dispatch-later (hide-later level)))))) -(re-frame/reg-event-fx - :notification/show show-notification) +(re-frame/reg-event-fx :notification/show show-notification) (defn hide-notification [db [_ notification-id]] (update db :notifications dissoc notification-id)) -(re-frame/reg-event-db - :notification/hide hide-notification) +(re-frame/reg-event-db :notification/hide hide-notification) diff --git a/src/cljs/airsonic_ui/routes.cljs b/src/cljs/airsonic_ui/routes.cljs index 43e5914..f169bce 100644 --- a/src/cljs/airsonic_ui/routes.cljs +++ b/src/cljs/airsonic_ui/routes.cljs @@ -100,7 +100,7 @@ ;; this allows us to encode a complete route in a url fragment; useful for ;; doing redirects (let [[_ _ query] (current-route) - from-param (decode-route (get query param))] + from-param (some-> (get query param) (decode-route))] (assoc-in coeffects [:routes/from-query-param param] from-param)))) (defn start-routing! diff --git a/src/cljs/airsonic_ui/subs.cljs b/src/cljs/airsonic_ui/subs.cljs index 6513cb4..61ab0c6 100644 --- a/src/cljs/airsonic_ui/subs.cljs +++ b/src/cljs/airsonic_ui/subs.cljs @@ -2,6 +2,14 @@ (:require [re-frame.core :as re-frame :refer [subscribe]] [airsonic-ui.utils.api :as api])) +(defn is-booting? + "Predicate to tell whether our app is still in the process of initialization" + [{:keys [credentials]} _] + (and (not (map? credentials)) + (not (#{:credentials/not-found :credentials/verification-failure :credentials/logged-out} credentials)))) + +(re-frame/reg-sub ::is-booting? is-booting?) + ;; can be used to query the user's credentials (re-frame/reg-sub diff --git a/src/cljs/airsonic_ui/views.cljs b/src/cljs/airsonic_ui/views.cljs index 3ff38c6..184741d 100644 --- a/src/cljs/airsonic_ui/views.cljs +++ b/src/cljs/airsonic_ui/views.cljs @@ -68,10 +68,11 @@ [bottom-bar]]))) (defn main-panel [] - (let [[route params query] @(subscribe [::subs/current-route]) - notifications @(subscribe [::subs/notifications])] + (let [notifications @(subscribe [::subs/notifications]) + is-booting? @(subscribe [::subs/is-booting?]) + [route params query] @(subscribe [::subs/current-route])] [:div [notification-list notifications] - (if route - [app route params query] - [:div.app-loading>div.loader])])) + (if is-booting? + [:div.app-loading>div.loader] + [app route params query] )])) diff --git a/test/cljs/airsonic_ui/events_test.cljs b/test/cljs/airsonic_ui/events_test.cljs index c68b679..a9d654f 100644 --- a/test/cljs/airsonic_ui/events_test.cljs +++ b/test/cljs/airsonic_ui/events_test.cljs @@ -13,18 +13,17 @@ (letfn [(no-previous-session [] (events/restore-previous-session {} [:_])) (has-previous-session [] - (events/restore-previous-session {:store {:u "test" - :p "test" - :server "https://demo.airsonic.io/"}} [:_]))] + (events/restore-previous-session {:store {:credentials {:u "test" + :p "test" + :server "https://demo.airsonic.io/"}}} [:_]))] (testing "Should initialize routing after checking for previous credentials" (is (contains? (no-previous-session) :routes/start-routing)) (is (contains? (has-previous-session) :routes/start-routing))) (testing "Should indicate success or failure" - (is (dispatches? (no-previous-session)) :init-flow/credentials-missing) - (is (dispatches? (has-previous-session)) :init-flow/credentials-found)) + (is (true? (dispatches? (no-previous-session) :init-flow/credentials-not-found))) + (is (true? (dispatches? (has-previous-session) :init-flow/credentials-found)))) (testing "Should send an auth request on success" - (is (dispatches? (events/credentials-found {} [:_]) :credentials/verification-request))) - (testing "Should redirect to the login form when there's no previous session to be restored"))) + (is (true? (dispatches? (events/credentials-found {} [:_]) :credentials/verification-request)))))) (deftest authentication (testing "Server ping for verifications" @@ -32,20 +31,23 @@ fx (events/credentials-verification-request {} [:_ "user" "pass" server]) request (:http-xhrio fx)] (testing "uses correct server url" - (is (str/starts-with? (:uri request) server)) - (is (str/includes? (:uri request) "/ping")) - (is (str/includes? (:uri request) "p=pass")) - (is (str/includes? (:uri request) "u=user"))) + (let [uri (:uri request)] + (is (true? (str/starts-with? uri server))) + (is (true? (str/includes? uri "/ping"))) + (is (true? (str/includes? uri "p=pass"))) + (is (true? (str/includes? uri "u=user"))))) (testing "invokes correct success callback" (is (= :credentials/verification-response (first (:on-success request))))))) - (testing "Auth response verification" - (let [server "https://localhost" - fx (events/credentials-verification-response {} [:_ "user" "pass" server (:error responses)])] - (is (= (dispatches? fx :notification/show)) - "shows an error when we have a bad response")) + (testing "Auth response" + (testing "verification for bad responses" + (let [ev [:_ "user" "pass" "https://localhost"] + invalid-credentials (events/credentials-verification-response {} (conj ev (:auth-failure responses))) + verification-failure (events/credentials-verification-failure {} [:_ (:auth-failure responses)])] + (is (true? (dispatches? invalid-credentials :credentials/verification-failure)) "fails for bad responses") + (is (true? (dispatches? verification-failure :notification/show)) "shows the failure the the user"))) (let [server "https://localhost" fx (events/credentials-verification-response {} [:_ "username" "password" server (:auth-success responses)])] - (is (dispatches? fx [:credentials/verified "username" "password" server])))) + (is (true? (dispatches? fx [:credentials/verified "username" "password" server]))))) (testing "On succesful response" (let [credentials {:u "user" :p "pass" :server "https://localhost"} fx (events/credentials-verified {} [:_ (:u credentials) (:p credentials) (:server credentials)])] @@ -54,7 +56,7 @@ (testing "credentials are saved in the global state" (is (= credentials (get-in fx [:db :credentials])))) (testing "the login process is finalized" - (is (dispatches? fx ::events/logged-in)))))) + (is (true? (dispatches? fx ::events/logged-in))))))) (deftest logout (let [fx (events/logout {} [:_])] @@ -65,7 +67,7 @@ (testing "Should unset authentication in the router" (is (contains? fx :routes/unset-credentials))) (testing "Should reset the app-db" - (is (= db/default-db (:db fx))))) + (is (= (every? #(= (get db/default-db %) (get-in fx [:db %])) (keys db/default-db)))))) (testing "Should be able to keep a redirection parameter" (let [redirect [:route {:with-data #{1 2 3 4 5}}] fx (events/logout {} [:_ :redirect-to redirect])] diff --git a/test/cljs/airsonic_ui/fixtures.cljs b/test/cljs/airsonic_ui/fixtures.cljs index ffc4a42..56aadef 100644 --- a/test/cljs/airsonic_ui/fixtures.cljs +++ b/test/cljs/airsonic_ui/fixtures.cljs @@ -1,8 +1,8 @@ (ns airsonic-ui.fixtures) (def responses {:error {:subsonic-response - {:error {:code 40 - :message "Wrong username or password"} + {:error {:code 50 + :message "Incompatible Airsonic REST protocol version. Server must upgrade."} :status "failed" :version "1.15.0"}} :ok {:subsonic-response @@ -11,7 +11,11 @@ :status "ok" :version "1.15.0"}} :auth-success {:subsonic-response {:status "ok" - :version "1.15.0"}}}) + :version "1.15.0"}} + :auth-failure {:subsonic-response {:status "failed" + :version "1.15.0" + :error {:code 40 + :message "Wrong username or password."}}}}) (def song {:artistId 42, diff --git a/test/cljs/airsonic_ui/subs_test.cljs b/test/cljs/airsonic_ui/subs_test.cljs index fed93ff..fc145fc 100644 --- a/test/cljs/airsonic_ui/subs_test.cljs +++ b/test/cljs/airsonic_ui/subs_test.cljs @@ -1,9 +1,42 @@ (ns airsonic-ui.subs-test (:require [cljs.test :refer [deftest testing is]] - [airsonic-ui.fixtures :refer [song]] + [airsonic-ui.db :as db] + [airsonic-ui.fixtures :refer [song] :as fixtures] [airsonic-ui.utils.api :as api] + [airsonic-ui.events :as ev] [airsonic-ui.subs :as subs])) +(def creds {:credentials {:u "test" + :p "test" + :server "https://demo.airsonic.io/"}}) + +(deftest is-booting + (testing "Should be true when provided the initial state" + (is (true? (subs/is-booting? db/default-db [:_])))) + (testing "Should be true when we have credentials but no response yet" + (is (true? (-> (ev/restore-previous-session {:store creds} [:_]) + (ev/credentials-found [:_]) + :db + (subs/is-booting? [:_]))))) + (testing "Should be false when the login screen is shown" + (is (false? (-> (ev/restore-previous-session {} [:_]) + (ev/credentials-not-found [:_]) + :db + (subs/is-booting? [:_]))))) + (let [{:keys [u p server]} (:credentials creds)] + (testing "Should be false after we verified our credentials with the server" + (is (false? (-> (ev/credentials-verified {:db {}} [:_ u p server]) + :db + (subs/is-booting? [:_]))))) + (testing "Should be false after the server rejected our credentials" + (is (false? (-> (ev/credentials-verification-failure {} [:_ (:auth-failure fixtures/responses)]) + :db + (subs/is-booting? [:_])))))) + (testing "Should be false when a user logged out voluntarily" + (is (false? (-> (ev/logout {} [:_]) + :db + (subs/is-booting? [:_])))))) + (deftest cover-images (let [credentials {:server "https://foo.bar" :u "test-user" diff --git a/test/cljs/airsonic_ui/utils/api_test.cljs b/test/cljs/airsonic_ui/utils/api_test.cljs index 3513b17..c3b9c73 100644 --- a/test/cljs/airsonic_ui/utils/api_test.cljs +++ b/test/cljs/airsonic_ui/utils/api_test.cljs @@ -47,3 +47,11 @@ (api/unwrap-response error-response) (catch ExceptionInfo e (= (:error error-response) (ex-data e))))))) + +(deftest error-recognition + (testing "Should detect error responses" + (is (true? (api/is-error? (:error responses)))) + (is (true? (api/is-error? (:auth-failure responses))))) + (testing "Should pass on good responses" + (is (false? (api/is-error? (:ok responses)))) + (is (false? (api/is-error? (:auth-success responses))))))