Handle case where require_client_password_reset_token is true and tok…

…en is invalid, small refactors for clarity

app/controllers/devise_token_auth/passwords_controller.rb

@@ -17,7 +17,7 @@ def create
         @resource.send_reset_password_instructions(
           email: @email,
           provider: 'email',
-          redirect_url: redirect_url,
+          redirect_url: @redirect_url,
           client_config: params[:config_name]
         )
 
@@ -39,13 +39,8 @@ def edit(&block)
       if @resource && @resource.reset_password_period_valid?
         if require_client_password_reset_token?
           update_allow_password_change(&block)
-          callback = Addressable::URI.parse(redirect_url)
-
-          # add password_reset_token while preserving existing query params
-          callback.query_values = (callback.query_values || {}).merge(
-            reset_password_token: resource_params[:reset_password_token]
-          )
-          redirect_to callback.to_s
+          callback_url = build_callback_url(resource_params[:reset_password_token])
+          redirect_to callback_url
         else
           client_id, token = @resource.create_token
           update_allow_password_change(&block)
@@ -66,9 +61,11 @@ def update
       # make sure user is authorized
       if require_client_password_reset_token? && resource_params[:reset_password_token]
         @resource = resource_class.with_reset_password_token(resource_params[:reset_password_token])
+        return render_update_error_unauthorized unless @resource
+
         @client_id, @token = @resource.create_token
       else
-        set_user_by_token
+        @resource = set_user_by_token
       end
 
       return render_update_error_unauthorized unless @resource
@@ -192,12 +189,14 @@ def validate_redirect_url_param
       return render_error_not_allowed_redirect_url if blacklisted_redirect_url?
     end
 
-    def redirect_url
-      # give redirect value from params priority, otherwise fall back to default value if provided
-      @redirect_url ||= params.fetch(
-        :redirect_url,
-        DeviseTokenAuth.default_password_reset_url
-      )
+    def build_callback_url(reset_password_token)
+      url          = URI.parse(@redirect_url)
+      query_params = Rack::Utils.parse_nested_query(URI.parse(url).query)
+      query_params = query_params.merge(reset_password_token: reset_password_token)
+      query_string = query_params.collect { |k, v| "#{k}=#{v}" }.join('&')
+      url.query    = query_string
+
+      url.to_s
     end
 
     def update_allow_password_change

test/controllers/devise_token_auth/passwords_controller_test.rb

@@ -239,10 +239,10 @@ class DeviseTokenAuth::PasswordsControllerTest < ActionController::TestCase
           end
         end
 
-        describe 'Cheking reset_password_token' do
+        describe 'Checking reset_password_token' do
           before do
             post :create, params: {
-              email:        @resource.email,
+              email: @resource.email,
               redirect_url: @redirect_url
             }
 
@@ -581,23 +581,25 @@ class DeviseTokenAuth::PasswordsControllerTest < ActionController::TestCase
           end
         end
 
-        describe 'with reset password token' do
+        describe 'with valid reset password token' do
           before do
             reset_password_token = @resource.send_reset_password_instructions
             @new_password = Faker::Internet.password
             @params = { password: @new_password,
                         password_confirmation: @new_password,
-                        reset_password_token: reset_password_token  }
+                        reset_password_token: reset_password_token }
           end
 
           describe 'with require_client_password_reset_token disabled' do
             before do
+              DeviseTokenAuth.require_client_password_reset_token = false
               put :update, params: @params
+
               @data = JSON.parse(response.body)
               @resource.reload
             end
 
-            test 'request should be successful' do
+            test 'request should be not be successful' do
               assert_equal 401, response.status
             end
           end
@@ -606,6 +608,7 @@ class DeviseTokenAuth::PasswordsControllerTest < ActionController::TestCase
             before do
               DeviseTokenAuth.require_client_password_reset_token = true
               put :update, params: @params
+
               @data = JSON.parse(response.body)
               @resource.reload
             end
@@ -629,6 +632,24 @@ class DeviseTokenAuth::PasswordsControllerTest < ActionController::TestCase
             end
           end
         end
+
+        describe 'with invalid reset password token' do
+          before do
+            DeviseTokenAuth.require_client_password_reset_token = true
+            @resource.update reset_password_token: 'koskoskoskos'
+            put :update, params: @params
+            @data = JSON.parse(response.body)
+            @resource.reload
+          end
+
+          test 'request should fail' do
+            assert_equal 401, response.status
+          end
+
+          test 'new password should not authenticate user' do
+            assert !@resource.valid_password?(@new_password)
+          end
+        end
       end
     end