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

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")

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))}

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)))))
 

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"]

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\"}