From e02187e5e5ecaa668598c4312918e0452307810e Mon Sep 17 00:00:00 2001
From: Noah Moss <32746338+noahmoss@users.noreply.github.com>
Date: Mon, 3 May 2021 13:35:48 -0700
Subject: [PATCH] Return 401 on auth failure instead of 400 (#15883)
* Change 400 errors to 401s on failed authentication
* remove TODO comment
* return 401 for expired JWT in enterprise code
* keep existing exception message when JWT is expired
* fix test failures
* remove comment about timeout on CI that's no longer relevant
---
.../sso/integrations/jwt.clj | 9 +++++++--
.../sso/integrations/jwt_test.clj | 2 +-
src/metabase/api/session.clj | 18 +++++++++---------
test/metabase/api/session_test.clj | 18 +++++++++---------
test/metabase/http_client.clj | 8 ++------
5 files changed, 28 insertions(+), 27 deletions(-)
diff --git a/enterprise/backend/src/metabase_enterprise/sso/integrations/jwt.clj b/enterprise/backend/src/metabase_enterprise/sso/integrations/jwt.clj
index 889c1c6c46d..4db2f877176 100644
--- a/enterprise/backend/src/metabase_enterprise/sso/integrations/jwt.clj
+++ b/enterprise/backend/src/metabase_enterprise/sso/integrations/jwt.clj
@@ -54,8 +54,13 @@
(defn- login-jwt-user
[jwt {{redirect :return_to} :params, :as request}]
- (let [jwt-data (jwt/unsign jwt (sso-settings/jwt-shared-secret)
- {:max-age three-minutes-in-seconds})
+ (let [jwt-data (try
+ (jwt/unsign jwt (sso-settings/jwt-shared-secret)
+ {:max-age three-minutes-in-seconds})
+ (catch Throwable e
+ (throw (ex-info (ex-message e)
+ (assoc (ex-data e) :status-code 401)
+ e))))
login-attrs (jwt-data->login-attributes jwt-data)
email (get jwt-data (jwt-attribute-email))
first-name (get jwt-data (jwt-attribute-firstname) "Unknown")
diff --git a/enterprise/backend/test/metabase_enterprise/sso/integrations/jwt_test.clj b/enterprise/backend/test/metabase_enterprise/sso/integrations/jwt_test.clj
index cb4c5e8d922..cacd7f349cb 100644
--- a/enterprise/backend/test/metabase_enterprise/sso/integrations/jwt_test.clj
+++ b/enterprise/backend/test/metabase_enterprise/sso/integrations/jwt_test.clj
@@ -103,7 +103,7 @@
(testing "Check an expired JWT"
(with-jwt-default-setup
(is (= "Token is older than max-age (180)"
- (:message (saml-test/client :get 500 "/auth/sso" {:request-options {:redirect-strategy :none}}
+ (:message (saml-test/client :get 401 "/auth/sso" {:request-options {:redirect-strategy :none}}
:return_to default-redirect-uri
:jwt (jwt/sign {:email "test@metabase.com", :first_name "Test" :last_name "User"
:iat (- (buddy-util/now) (u/minutes->seconds 5))}
diff --git a/src/metabase/api/session.clj b/src/metabase/api/session.clj
index 2d7b77b6928..34f7a931c2a 100644
--- a/src/metabase/api/session.clj
+++ b/src/metabase/api/session.clj
@@ -93,7 +93,7 @@
;; Since LDAP knows about the user, fail here to prevent the local strategy to be tried with a possibly
;; outdated password
(throw (ex-info (str password-fail-message)
- {:status-code 400
+ {:status-code 401
:errors {:password password-fail-snippet}})))
;; password is ok, return new session
(create-session! :sso (ldap/fetch-or-create-user! user-info) device-info))
@@ -126,20 +126,20 @@
;; Don't leak whether the account doesn't exist or the password was incorrect
(throw
(ex-info (str password-fail-message)
- {:status-code 400
+ {:status-code 401
:errors {:password password-fail-snippet}}))))
-(defn- do-http-400-on-error [f]
+(defn- do-http-401-on-error [f]
(try
(f)
(catch clojure.lang.ExceptionInfo e
(throw (ex-info (ex-message e)
- (assoc (ex-data e) :status-code 400))))))
+ (assoc (ex-data e) :status-code 401))))))
-(defmacro http-400-on-error
- "Add `{:status-code 400}` to exception data thrown by `body`."
+(defmacro http-401-on-error
+ "Add `{:status-code 401}` to exception data thrown by `body`."
[& body]
- `(do-http-400-on-error (fn [] ~@body)))
+ `(do-http-401-on-error (fn [] ~@body)))
(api/defendpoint POST "/"
"Login."
@@ -153,7 +153,7 @@
(mw.session/set-session-cookie request response session)))]
(if throttling-disabled?
(do-login)
- (http-400-on-error
+ (http-401-on-error
(throttle/with-throttling [(login-throttlers :ip-address) ip-address
(login-throttlers :username) username]
(do-login))))))
@@ -345,7 +345,7 @@
;; Verify the token is valid with Google
(if throttling-disabled?
(do-google-auth token)
- (http-400-on-error
+ (http-401-on-error
(throttle/with-throttling [(login-throttlers :ip-address) (request.u/ip-address request)]
(do-google-auth request)))))
diff --git a/test/metabase/api/session_test.clj b/test/metabase/api/session_test.clj
index b20d1d7db44..dd6681ffb48 100644
--- a/test/metabase/api/session_test.clj
+++ b/test/metabase/api/session_test.clj
@@ -86,11 +86,11 @@
(testing "Test for inactive user (user shouldn't be able to login if :is_active = false)"
;; Return same error as incorrect password to avoid leaking existence of user
(is (= {:errors {:password "did not match stored password"}}
- (mt/client :post 400 "session" (mt/user->credentials :trashbird)))))
+ (mt/client :post 401 "session" (mt/user->credentials :trashbird)))))
(testing "Test for password checking"
(is (= {:errors {:password "did not match stored password"}}
- (mt/client :post 400 "session" (-> (mt/user->credentials :rasta)
+ (mt/client :post 401 "session" (-> (mt/user->credentials :rasta)
(assoc :password "something else"))))))))
(deftest login-throttling-test
@@ -98,7 +98,7 @@
" throttling works at the API level -- more tests in the throttle library itself:"
" https://github.com/metabase/throttle)")
(let [login (fn []
- (-> (mt/client :post 400 "session" {:username "fakeaccount3000@metabase.com", :password "toucans"})
+ (-> (mt/client :post 401 "session" {:username "fakeaccount3000@metabase.com", :password "toucans"})
:errors
:username))]
;; attempt to log in 10 times
@@ -143,7 +143,7 @@
(let [response (send-login-request (format "user-%d" n)
{"x-forwarded-for" "10.1.2.3"})
status-code (:status response)]
- (assert (= status-code 400) (str "Unexpected response status code:" status-code))))
+ (assert (= status-code 401) (str "Unexpected response status code:" status-code))))
(let [error (fn []
(-> (send-login-request "last-user" {"x-forwarded-for" "10.1.2.3"})
:body
@@ -163,11 +163,11 @@
(let [response (send-login-request (format "user-%d" n)
{"x-forwarded-for" "10.1.2.3"})
status-code (:status response)]
- (assert (= status-code 400) (str "Unexpected response status code:" status-code))))
+ (assert (= status-code 401) (str "Unexpected response status code:" status-code))))
(dotimes [n 50]
(let [response (send-login-request (format "round2-user-%d" n)) ; no x-forwarded-for
status-code (:status response)]
- (assert (= status-code 400) (str "Unexpected response status code:" status-code))))
+ (assert (= status-code 401) (str "Unexpected response status code:" status-code))))
(let [error (fn []
(-> (send-login-request "last-user" {"x-forwarded-for" "10.1.2.3"})
:body
@@ -276,7 +276,7 @@
:password (:new password)}))))
(testing "Old creds should no longer work"
(is (= {:errors {:password "did not match stored password"}}
- (mt/client :post 400 "session" (:old creds)))))
+ (mt/client :post 401 "session" (:old creds)))))
(testing "New creds *should* work"
(is (schema= SessionResponse
(mt/client :post 200 "session" (:new creds)))))
@@ -541,12 +541,12 @@
(testing "...but not if password login is disabled"
(mt/with-temporary-setting-values [enable-password-login false]
(is (= "Password login is disabled for this instance."
- (mt/client :post 400 "session" (mt/user->credentials :crowberto)))))))
+ (mt/client :post 401 "session" (mt/user->credentials :crowberto)))))))
(testing "Test that login will NOT fallback for users in LDAP but with an invalid password"
;; NOTE: there's a different password in LDAP for Lucky
(is (= {:errors {:password "did not match stored password"}}
- (mt/client :post 400 "session" (mt/user->credentials :lucky)))))
+ (mt/client :post 401 "session" (mt/user->credentials :lucky)))))
(testing "Test that login will fallback to local for broken LDAP settings"
(mt/with-temporary-setting-values [ldap-user-base "cn=wrong,cn=com"]
diff --git a/test/metabase/http_client.clj b/test/metabase/http_client.clj
index 50b860546ff..dd65fd7631e 100644
--- a/test/metabase/http_client.clj
+++ b/test/metabase/http_client.clj
@@ -228,11 +228,7 @@
:query-parameters query-parameters
:request-options request-options}))
-(def ^:private response-timeout-ms
- ;; CircleCI is crazy slow and likes to randomly pause, so use a much larger timeout when running on CI
- (u/seconds->ms (if (env/env :ci)
- 45
- 15)))
+(def ^:private response-timeout-ms (u/seconds->ms 45))
(defn client-full-response
"Identical to `client` except returns the full HTTP response map, not just the body of the response"
@@ -250,7 +246,7 @@
Examples:
- (client :get 200 \"card/1\") ; GET http://localhost:3000/api/card/1, throw exception is status code != 200
+ (client :get 200 \"card/1\") ; GET http://localhost:3000/api/card/1, throw exception if status code != 200
(client :get \"card\" :org 1) ; GET http://localhost:3000/api/card?org=1
(client :post \"card\" {:name \"My Card\"}) ; POST http://localhost:3000/api/card with JSON-encoded body {:name \"My Card\"}
--
GitLab