From d8d88968e9f987c2d4916d55cc83ce0d5e19c260 Mon Sep 17 00:00:00 2001
From: Thorsten Eckel <te@znuny.com>
Date: Fri, 24 Feb 2017 18:27:27 +0100
Subject: [PATCH] Fixed issue #791 - User can't login after to many failed
 logins even if password gets resetted.

---
 app/controllers/users_controller.rb |  2 +-
 app/models/user.rb                  | 26 ++++++++--------
 spec/factories/token.rb             |  9 ++++++
 spec/factories/user.rb              |  5 +++
 spec/models/user_spec.rb            | 47 +++++++++++++++++++++++++++++
 5 files changed, 75 insertions(+), 14 deletions(-)
 create mode 100644 spec/factories/token.rb
 create mode 100644 spec/models/user_spec.rb

diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb
index 8ef94d4a3..a0d00a002 100644
--- a/app/controllers/users_controller.rb
+++ b/app/controllers/users_controller.rb
@@ -711,7 +711,7 @@ curl http://localhost/api/v1/users/password_reset_verify.json -v -u #{login}:#{p
       end
 
     else
-      user = User.password_reset_check(params[:token])
+      user = User.by_reset_token(params[:token])
     end
     if user
       render json: { message: 'ok', user_login: user.login }, status: :ok
diff --git a/app/models/user.rb b/app/models/user.rb
index 02b266ddb..1ce45b045 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -39,7 +39,7 @@ class User < ApplicationModel
 
   before_validation :check_name, :check_email, :check_login, :ensure_password
   before_create   :check_preferences_default, :validate_roles, :domain_based_assignment, :set_locale
-  before_update   :check_preferences_default, :validate_roles
+  before_update   :check_preferences_default, :validate_roles, :reset_login_failed
   after_create    :avatar_for_email_check
   after_update    :avatar_for_email_check
   after_destroy   :avatar_destroy
@@ -490,9 +490,9 @@ returns
 
 =begin
 
-check reset password token
+returns the User instance for a given password token if found
 
-  result = User.password_reset_check(token)
+  result = User.by_reset_token(token)
 
 returns
 
@@ -500,15 +500,8 @@ returns
 
 =end
 
-  def self.password_reset_check(token)
-    user = Token.check(action: 'PasswordReset', name: token)
-
-    # reset login failed if token is valid
-    if user
-      user.login_failed = 0
-      user.save
-    end
-    user
+  def self.by_reset_token(token)
+    Token.check(action: 'PasswordReset', name: token)
   end
 
 =begin
@@ -526,7 +519,7 @@ returns
   def self.password_reset_via_token(token, password)
 
     # check token
-    user = Token.check(action: 'PasswordReset', name: token)
+    user = by_reset_token(token)
     return if !user
 
     # reset password
@@ -938,4 +931,11 @@ returns
     self.password = password_was
     true
   end
+
+  # reset login_failed if password is changed
+  def reset_login_failed
+    return if !changes
+    return if !changes['password']
+    self.login_failed = 0
+  end
 end
diff --git a/spec/factories/token.rb b/spec/factories/token.rb
new file mode 100644
index 000000000..bdd77e6f3
--- /dev/null
+++ b/spec/factories/token.rb
@@ -0,0 +1,9 @@
+FactoryGirl.define do
+  factory :token do
+    user_id { FactoryGirl.create(:user).id }
+  end
+
+  factory :token_password_reset, parent: :token do
+    action 'PasswordReset'
+  end
+end
diff --git a/spec/factories/user.rb b/spec/factories/user.rb
index bae6b1953..c27cbc040 100644
--- a/spec/factories/user.rb
+++ b/spec/factories/user.rb
@@ -13,10 +13,15 @@ FactoryGirl.define do
     email         { generate(:email) }
     password      'zammad'
     active        true
+    login_failed  0
     updated_by_id 1
     created_by_id 1
   end
 
+  factory :user_login_failed, parent: :user do
+    login_failed { (Setting.get('password_max_login_failed').to_i || 10) + 1 }
+  end
+
   factory :user_legacy_password_sha2, parent: :user do
     after(:build) { |user| user.class.skip_callback(:validation, :before, :ensure_password) }
     password '{sha2}dd9c764fa7ea18cd992c8600006d3dc3ac983d1ba22e9ba2d71f6207456be0ba' # zammad
diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb
new file mode 100644
index 000000000..ca76a7fb0
--- /dev/null
+++ b/spec/models/user_spec.rb
@@ -0,0 +1,47 @@
+require 'rails_helper'
+
+RSpec.describe User do
+
+  let(:new_password) { 'N3W54V3PW!' }
+
+  context 'password' do
+
+    it 'resets login_failed on password change' do
+      user = create(:user_login_failed)
+      expect {
+        user.password = new_password
+        user.save
+      }.to change { user.login_failed }.to(0)
+    end
+  end
+
+  context '#by_reset_token' do
+
+    it 'returns a User instance for existing tokens' do
+      token = create(:token_password_reset)
+      expect(described_class.by_reset_token(token.name)).to be_instance_of(described_class)
+    end
+
+    it 'returns nil for not existing tokens' do
+      expect(described_class.by_reset_token('not-existing')).to be nil
+    end
+  end
+
+  context '#password_reset_via_token' do
+
+    it 'changes the password of the token user and destroys the token' do
+      token = create(:token_password_reset)
+      user  = User.find(token.user_id)
+
+      expect {
+        described_class.password_reset_via_token(token.name, new_password)
+        user.reload
+      }.to change {
+        user.password
+      }.and change {
+        Token.count
+      }.by(-1)
+    end
+  end
+
+end