Merge pull request #1035 from metabase/error-message-for-when-reset-token-expires

Error page for invalid or expired reset token

resources/frontend_client/app/auth/auth.controllers.js

@@ -89,32 +89,42 @@ AuthControllers.controller('ForgotPassword', ['$scope', '$cookies', '$location',
 
 
 AuthControllers.controller('PasswordReset', ['$scope', '$routeParams', '$location', 'AuthUtil', 'Session', function($scope, $routeParams, $location, AuthUtil, Session) {
-
-    $scope.resetSuccess = false;
-    $scope.newUserJoining = ($location.hash() === 'new');
-
-    $scope.resetPassword = function(password) {
-        $scope.$broadcast("form:reset");
-
-        if (password != $scope.password2) {
-            $scope.$broadcast("form:api-error", {'data': {'errors': {'password2': "Passwords do not match"}}});
+    // first, we need to ask the API if this token is expired. If so, so the expired token page. Otherwise, show the password reset page
+    Session.password_reset_token_valid({
+        token: $routeParams.token
+    }, function(result) {
+        if (!result.valid) {
+            $location.path('/auth/password_reset_token_expired');
             return;
         }
 
-        Session.reset_password({
-            'token': $routeParams.token,
-            'password': password
-        }, function (result) {
-            $scope.resetSuccess = true;
+        $scope.resetSuccess = false;
+        $scope.newUserJoining = ($location.hash() === 'new');
 
-            // we should have a valid session that we can use immediately now!
-            if (result.session_id) {
-                AuthUtil.setSession(result.session_id);
+        $scope.resetPassword = function(password) {
+            $scope.$broadcast("form:reset");
+
+            if (password != $scope.password2) {
+                $scope.$broadcast("form:api-error", {'data': {'errors': {'password2': "Passwords do not match"}}});
+                return;
             }
 
-        }, function (error) {
-            $scope.$broadcast("form:api-error", error);
-        });
-    };
+            Session.reset_password({
+                'token': $routeParams.token,
+                'password': password
+            }, function (result) {
+                $scope.resetSuccess = true;
+
+                // we should have a valid session that we can use immediately now!
+                if (result.session_id) {
+                    AuthUtil.setSession(result.session_id);
+                }
 
+            }, function (error) {
+                $scope.$broadcast("form:api-error", error);
+            });
+        };
+    }, function(error) {
+        $scope.$broadcast('form:api-error', error);
+    });
 }]);

resources/frontend_client/app/auth/auth.module.js

@@ -22,4 +22,8 @@ Auth.config(['$routeProvider', function($routeProvider) {
         templateUrl: '/app/auth/partials/password_reset.html',
         controller: 'PasswordReset'
     });
+
+    $routeProvider.when('/auth/password_reset_token_expired', {
+        templateUrl: '/app/auth/partials/password_reset_token_expired.html'
+    });
 }]);

resources/frontend_client/app/auth/partials/password_reset_token_expired.html

+<div class="bg-white flex flex-column flex-full layout-centered">
+    <div class="wrapper">
+        <div class="Login-wrapper Grid  Grid--full md-Grid--1of2">
+            <div class="Grid-cell flex layout-centered text-brand">
+                <mb-logo-icon class="Logo my4 sm-my0" width="66px" height="85px"></mb-logo-icon>
+            </div>
+            <div class="Grid-cell bordered rounded shadowed">
+                <h3 class="Login-header Form-offset mt4">Whoops, that's an expired link</h3>
+                <p class="Form-offset mb4 mr4">
+                    For security reasons, password reset links expire after a little while. If you still need
+                    to reset your password, you can <a href="/auth/forgot_password" class="link">request a new reset email</a>.
+                </p>
+            </div>
+        </div>
+    </div>
+</div>
+<div ng-include="'/app/auth/partials/auth_scene.html'"></div>

resources/frontend_client/app/services.js

@@ -302,6 +302,10 @@ CoreServices.factory('Session', ['$resource', '$cookies', function($resource, $c
         reset_password: {
             url: '/api/session/reset_password',
             method: 'POST'
+        },
+        password_reset_token_valid: {
+            url: '/api/session/password_reset_token_valid',
+            method: 'GET'
         }
     });
 }]);

src/metabase/api/session.clj

@@ -82,31 +82,46 @@
       (log/info password-reset-url))))
 
 
+(def ^:private ^:const reset-token-ttl-ms
+  "Number of milliseconds a password reset is considered valid."
+  (* 48 60 60 1000)) ; token considered valid for 48 hours
+
+(defn- valid-reset-token->user-id
+  "Check if a password reset token is valid. If so, return the `User` ID it corresponds to."
+  [^String token]
+  (when-let [[_ user-id] (re-matches #"(^\d+)_.+$" token)]
+    (let [user-id (Integer/parseInt user-id)]
+      (when-let [{:keys [reset_token reset_triggered]} (sel :one :fields [User :reset_triggered :reset_token] :id user-id)]
+        ;; Make sure the plaintext token matches up with the hashed one for this user
+        (when (try (creds/bcrypt-verify token reset_token)
+                   (catch Throwable _))
+          ;; check that the reset was triggered within the last 48 HOURS, after that the token is considered expired
+          (let [token-age (- (System/currentTimeMillis) reset_triggered)]
+            (when (< token-age reset-token-ttl-ms)
+              user-id)))))))
+
 (defendpoint POST "/reset_password"
   "Reset password with a reset token."
-  [:as {{:keys [token password] :as body} :body}]
+  [:as {{:keys [token password]} :body}]
   {token    Required
    password [Required ComplexPassword]}
-  (or (when-let [[_ user-id] (re-matches #"(^\d+)_.+$" token)]
-        (let [user-id (Integer/parseInt user-id)]
-          (when-let [{:keys [reset_token reset_triggered]} (sel :one :fields [User :reset_triggered :reset_token] :id user-id)]
-            ;; Make sure the plaintext token matches up with the hashed one for this user
-            (when (try (creds/bcrypt-verify token reset_token)
-                       (catch Throwable _))
-
-              ;; check that the reset was triggered within the last 48 HOURS, after that the token is considered expired
-              (checkp (> (* 48 60 60 1000) (- (System/currentTimeMillis) (or reset_triggered 0)))
-                'password "Reset token has expired")
-
-              (set-user-password user-id password)
-              ;; after a successful password update go ahead and offer the client a new session that they can use
-              (let [session-id (create-session user-id)]
-                (events/publish-event :user-login {:user_id user-id :session_id session-id})
-                {:success    true
-                 :session_id session-id})))))
+  (or (when-let [user-id (valid-reset-token->user-id token)]
+        (set-user-password user-id password)
+        ;; after a successful password update go ahead and offer the client a new session that they can use
+        (let [session-id (create-session user-id)]
+          (events/publish-event :user-login {:user_id user-id :session_id session-id})
+          {:success    true
+           :session_id session-id}))
       (throw (invalid-param-exception :password "Invalid reset token"))))
 
 
+(defendpoint GET "/password_reset_token_valid"
+  "Check is a password reset token is valid and isn't expired."
+  [token]
+  {token Required}
+  {:valid (boolean (valid-reset-token->user-id token))})
+
+
 (defendpoint GET "/properties"
   "Get all global properties and their values. These are the specific `Settings` which are meant to be public."
   []

test/metabase/api/session_test.clj

@@ -97,15 +97,16 @@
                             :new {:password (:new password)
                                   :email    email}}]
     ;; Check that creds work
-    (metabase.http-client/client :post 200 "session" (:old creds))
-    ;; Change the PW
-    (metabase.http-client/client :post 200 "session/reset_password" {:token    token
-                                                                     :password (:new password)})
+    (client :post 200 "session" (:old creds))
+
+    ;; Call reset password endpoint to change the PW
+    (client :post 200 "session/reset_password" {:token    token
+                                                :password (:new password)})
     ;; Old creds should no longer work
-    (assert (= (metabase.http-client/client :post 400 "session" (:old creds))
+    (assert (= (client :post 400 "session" (:old creds))
               {:errors {:password "did not match stored password"}}))
     ;; New creds *should* work
-    (metabase.http-client/client :post 200 "session" (:new creds))
+    (client :post 200 "session" (:new creds))
     ;; Double check that reset token was cleared
     (sel :one :fields [User :reset_token :reset_triggered] :id id)))
 
@@ -120,8 +121,8 @@
           token              (str id "_" (java.util.UUID/randomUUID))
           _                  (upd User id :reset_token token)]
       ;; run the password reset
-      (metabase.http-client/client :post 200 "session/reset_password" {:token    token
-                                                                       :password "whateverUP12!!"}))))
+      (client :post 200 "session/reset_password" {:token    token
+                                                  :password "whateverUP12!!"}))))
 
 ;; Test that token and password are required
 (expect {:errors {:token "field is a required param."}}
@@ -140,14 +141,33 @@
   (client :post 400 "session/reset_password" {:token "1_not-found"
                                               :password "whateverUP12!!"}))
 
-;; Test that old token can expire
-(expect {:errors {:password "Reset token has expired"}}
+;; Test that an expired token doesn't work
+(expect {:errors {:password "Invalid reset token"}}
   (let [token (str (user->id :rasta) "_" (java.util.UUID/randomUUID))]
     (upd User (user->id :rasta) :reset_token token, :reset_triggered 0)
     (client :post 400 "session/reset_password" {:token token
                                                 :password "whateverUP12!!"})))
 
 
+;;; GET /session/password_reset_token_valid
+
+;; Check that a valid, unexpired token returns true
+(expect {:valid true}
+  (let [token (str (user->id :rasta) "_" (java.util.UUID/randomUUID))]
+    (upd User (user->id :rasta) :reset_token token, :reset_triggered (dec (System/currentTimeMillis)))
+    (client :get 200 "session/password_reset_token_valid", :token token)))
+
+;; Check than an made-up token returns false
+(expect {:valid false}
+  (client :get 200 "session/password_reset_token_valid", :token "ABCDEFG"))
+
+;; Check that an expired but valid token returns false
+(expect {:valid false}
+  (let [token (str (user->id :rasta) "_" (java.util.UUID/randomUUID))]
+    (upd User (user->id :rasta) :reset_token token, :reset_triggered 0)
+    (client :get 200 "session/password_reset_token_valid", :token token)))
+
+
 ;; GET /session/properties
 ;; Check that a non-superuser can't read settings
 (expect