From 967fa331c4b65045682894f40051186b0be9e721 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Philippe=20H=C3=A4ssig?= <phil@nine.ch>
Date: Wed, 10 Aug 2016 15:31:44 +0200
Subject: [PATCH 01/14] Lock users after x failed attempts

After a configurable amount of consecutive failed login attempts the new attribute `locked_until` will be set.
The default amount of attempts is 5 and the default time the user is locked is 5 minutes.
After this commit locked users will still be able to log in. Locking out locked users is part of the next
commit.
---
 app/controllers/casino/sessions_controller.rb |  2 +-
 app/helpers/casino/sessions_helper.rb         |  9 ++-
 app/models/casino/login_attempt.rb            |  4 ++
 app/models/casino/user.rb                     | 10 +++
 ...0160810113208_add_locked_until_to_users.rb |  5 ++
 spec/controllers/sessions_controller_spec.rb  | 18 ++++++
 ...122605_add_locked_until_to_users.casino.rb |  6 ++
 spec/dummy/db/schema.rb                       |  3 +-
 spec/model/login_attempt_spec.rb              | 12 ++++
 spec/model/user_spec.rb                       | 63 +++++++++++++++++++
 10 files changed, 129 insertions(+), 3 deletions(-)
 create mode 100644 db/migrate/20160810113208_add_locked_until_to_users.rb
 create mode 100644 spec/dummy/db/migrate/20160810122605_add_locked_until_to_users.casino.rb
 create mode 100644 spec/model/user_spec.rb

diff --git a/app/controllers/casino/sessions_controller.rb b/app/controllers/casino/sessions_controller.rb
index 50c77ad4..5f9ea1d7 100644
--- a/app/controllers/casino/sessions_controller.rb
+++ b/app/controllers/casino/sessions_controller.rb
@@ -23,7 +23,7 @@ def new
   def create
     validation_result = validate_login_credentials(params[:username], params[:password])
     if !validation_result
-      log_failed_login params[:username]
+      handle_failed_login params[:username]
       show_login_error I18n.t('login_credential_acceptor.invalid_login_credentials')
     else
       sign_in(validation_result, long_term: params[:rememberMe], credentials_supplied: true)
diff --git a/app/helpers/casino/sessions_helper.rb b/app/helpers/casino/sessions_helper.rb
index 13d27d74..5d6cf574 100644
--- a/app/helpers/casino/sessions_helper.rb
+++ b/app/helpers/casino/sessions_helper.rb
@@ -4,6 +4,8 @@ module CASino::SessionsHelper
   include CASino::TicketGrantingTicketProcessor
   include CASino::ServiceTicketProcessor
 
+  LOCK_TIMEOUT = 5.minutes
+
   def current_ticket_granting_ticket?(ticket_granting_ticket)
     ticket_granting_ticket.ticket == cookies[:tgt]
   end
@@ -51,9 +53,10 @@ def sign_out
     cookies.delete :tgt
   end
 
-  def log_failed_login(username)
+  def handle_failed_login(username)
     CASino::User.where(username: username).each do |user|
       create_login_attempt(user, false)
+      lock_user(user) if user.max_failed_logins_reached?(CASino.config.max_failed_login_attempts)
     end
   end
 
@@ -91,4 +94,8 @@ def handle_signed_in_with_service(tgt, options)
       redirect_to url, status: :see_other
     end
   end
+
+  def lock_user(user)
+    user.update locked_until: LOCK_TIMEOUT.from_now
+  end
 end
diff --git a/app/models/casino/login_attempt.rb b/app/models/casino/login_attempt.rb
index 73ca13ca..b4477635 100644
--- a/app/models/casino/login_attempt.rb
+++ b/app/models/casino/login_attempt.rb
@@ -2,4 +2,8 @@ class CASino::LoginAttempt < ActiveRecord::Base
   include CASino::ModelConcern::BrowserInfo
 
   belongs_to :user
+
+  def failed?
+    !successful?
+  end
 end
diff --git a/app/models/casino/user.rb b/app/models/casino/user.rb
index 2bd2a72b..b110d02c 100644
--- a/app/models/casino/user.rb
+++ b/app/models/casino/user.rb
@@ -9,4 +9,14 @@ class CASino::User < ActiveRecord::Base
   def active_two_factor_authenticator
     self.two_factor_authenticators.where(active: true).first
   end
+
+  def locked?
+    return false unless locked_until
+    locked_until.future?
+  end
+
+  def max_failed_logins_reached?(max)
+    max ||= 5
+    login_attempts.last(max).count(&:failed?) == max
+  end
 end
diff --git a/db/migrate/20160810113208_add_locked_until_to_users.rb b/db/migrate/20160810113208_add_locked_until_to_users.rb
new file mode 100644
index 00000000..085b184e
--- /dev/null
+++ b/db/migrate/20160810113208_add_locked_until_to_users.rb
@@ -0,0 +1,5 @@
+class AddLockedUntilToUsers < ActiveRecord::Migration
+  def change
+    add_column :casino_users, :locked_until, :datetime
+  end
+end
diff --git a/spec/controllers/sessions_controller_spec.rb b/spec/controllers/sessions_controller_spec.rb
index 79ecb3f8..264898c0 100644
--- a/spec/controllers/sessions_controller_spec.rb
+++ b/spec/controllers/sessions_controller_spec.rb
@@ -208,6 +208,24 @@
           expect(CASino::LoginAttempt.last.user).to eq user
           expect(CASino::LoginAttempt.last.successful).to eq false
         end
+
+        it 'does not lock the user' do
+          expect do
+            post :create, params
+          end.to_not change { user.reload.locked? }
+        end
+
+        context 'when the maximum of failed login attempts is reached' do
+          before do
+            allow(CASino.config).to receive(:max_failed_login_attempts).and_return(1)
+          end
+
+          it 'deactivates the user' do
+            expect do
+              post :create, params
+            end.to change { user.reload.locked? }.from(false).to(true)
+          end
+        end
       end
 
       context 'with valid credentials' do
diff --git a/spec/dummy/db/migrate/20160810122605_add_locked_until_to_users.casino.rb b/spec/dummy/db/migrate/20160810122605_add_locked_until_to_users.casino.rb
new file mode 100644
index 00000000..a09eea44
--- /dev/null
+++ b/spec/dummy/db/migrate/20160810122605_add_locked_until_to_users.casino.rb
@@ -0,0 +1,6 @@
+# This migration comes from casino (originally 20160810113208)
+class AddLockedUntilToUsers < ActiveRecord::Migration
+  def change
+    add_column :casino_users, :locked_until, :datetime
+  end
+end
diff --git a/spec/dummy/db/schema.rb b/spec/dummy/db/schema.rb
index 8cbc2295..57bb4f29 100644
--- a/spec/dummy/db/schema.rb
+++ b/spec/dummy/db/schema.rb
@@ -11,7 +11,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema.define(version: 20160502074450) do
+ActiveRecord::Schema.define(version: 20160810122605) do
 
   create_table "casino_auth_token_tickets", force: true do |t|
     t.string   "ticket",     null: false
@@ -119,6 +119,7 @@
     t.text     "extra_attributes"
     t.datetime "created_at"
     t.datetime "updated_at"
+    t.datetime "locked_until"
   end
 
   add_index "casino_users", ["authenticator", "username"], name: "index_casino_users_on_authenticator_and_username", unique: true
diff --git a/spec/model/login_attempt_spec.rb b/spec/model/login_attempt_spec.rb
index 2fe7af25..42d95535 100644
--- a/spec/model/login_attempt_spec.rb
+++ b/spec/model/login_attempt_spec.rb
@@ -4,4 +4,16 @@
   subject { described_class.new user_agent: 'TestBrowser' }
 
   it_behaves_like 'has browser info'
+
+  describe '#failed?' do
+    it 'is true when it is not successful' do
+      login_attempt = FactoryGirl.create :login_attempt, successful: false
+      expect(login_attempt).to be_failed
+    end
+
+    it 'is false when it is successful' do
+      login_attempt = FactoryGirl.create :login_attempt, successful: true
+      expect(login_attempt).to_not be_failed
+    end
+  end
 end
diff --git a/spec/model/user_spec.rb b/spec/model/user_spec.rb
new file mode 100644
index 00000000..3bfcd54e
--- /dev/null
+++ b/spec/model/user_spec.rb
@@ -0,0 +1,63 @@
+require 'spec_helper'
+
+RSpec.describe CASino::User do
+  let(:user) { FactoryGirl.create :user }
+
+  describe '#locked?' do
+    it 'is true when locked_until is in the future' do
+      user = FactoryGirl.create :user, locked_until: 1.hour.from_now
+      expect(user).to be_locked
+    end
+
+    it 'is false when locked_until is in the past' do
+      user = FactoryGirl.create :user, locked_until: 1.hour.ago
+      expect(user).to_not be_locked
+    end
+
+    it 'is false when locked_until is empty' do
+      user = FactoryGirl.create :user, locked_until: nil
+      expect(user).to_not be_locked
+    end
+  end
+
+  describe '#max_failed_logins_reached?' do
+    let(:max_failed_attempts) { 2 }
+
+    subject { user.max_failed_logins_reached?(max_failed_attempts) }
+
+    context 'when the user has no login attempts' do
+      it { is_expected.to eq false }
+    end
+
+    context 'when the user has only successful logins' do
+      it { is_expected.to eq false }
+    end
+
+    context 'when the maximum of attempts is reached' do
+      before { FactoryGirl.create_list :login_attempt, 2, successful: false, user: user }
+
+      context 'in a row' do
+        it { is_expected.to eq true }
+      end
+
+      context 'but the last attempt was successful' do
+        before { FactoryGirl.create :login_attempt, successful: true, user: user }
+        it { is_expected.to eq false }
+      end
+
+      context 'but a successful between' do
+        before do
+          FactoryGirl.create :login_attempt, successful: true, user: user
+          FactoryGirl.create :login_attempt, successful: false, user: user
+        end
+
+        it { is_expected.to eq false }
+      end
+    end
+
+    context 'when the user has less then the maximum failed attempts' do
+      before { FactoryGirl.create :login_attempt, successful: false, user: user }
+      it { is_expected.to eq false }
+    end
+  end
+end

From d382925be0cad73d966ae41610ad058e0072077c Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Philippe=20H=C3=A4ssig?= <phil@nine.ch>
Date: Fri, 12 Aug 2016 10:37:48 +0200
Subject: [PATCH 02/14] Reject login when a user is locked

When a user is locked (locked_until is in the future) he can't log in anymore.
---
 app/controllers/casino/sessions_controller.rb |  9 +++++++++
 config/locales/de.yml                         |  1 +
 config/locales/en.yml                         |  1 +
 config/locales/fr.yml                         |  1 +
 spec/controllers/sessions_controller_spec.rb  | 19 ++++++++++++++++++-
 5 files changed, 30 insertions(+), 1 deletion(-)

diff --git a/app/controllers/casino/sessions_controller.rb b/app/controllers/casino/sessions_controller.rb
index 5f9ea1d7..390e7cd1 100644
--- a/app/controllers/casino/sessions_controller.rb
+++ b/app/controllers/casino/sessions_controller.rb
@@ -25,6 +25,8 @@ def create
     if !validation_result
       handle_failed_login params[:username]
       show_login_error I18n.t('login_credential_acceptor.invalid_login_credentials')
+    elsif user_from_validation_result(validation_result).locked?
+      show_login_error I18n.t('login_credential_acceptor.user_is_locked')
     else
       sign_in(validation_result, long_term: params[:rememberMe], credentials_supplied: true)
     end
@@ -83,4 +85,11 @@ def load_ticket_granting_ticket_from_parameter
     @ticket_granting_ticket = find_valid_ticket_granting_ticket(params[:tgt], request.user_agent, ignore_two_factor: true)
     redirect_to login_path if @ticket_granting_ticket.nil?
   end
+
+  def user_from_validation_result(validation_result)
+      user_data = validation_result[:user_data]
+      load_or_initialize_user(validation_result[:authenticator],
+                              user_data[:username],
+                              user_data[:extra_attributes])
+  end
 end
diff --git a/config/locales/de.yml b/config/locales/de.yml
index 43aba4e4..64d59610 100644
--- a/config/locales/de.yml
+++ b/config/locales/de.yml
@@ -2,6 +2,7 @@ de:
   login_credential_acceptor:
     invalid_login_ticket: "Ihre Anfrage enthielt kein gültiges Login-Ticket."
     invalid_login_credentials: "Benutzername oder Passwort falsch."
+    user_is_locked: "Ihr Account ist wegen zu vieler falscher Loginversuche gesperrt."
   login:
     label_username: "Benutzername"
     label_password: "Passwort"
diff --git a/config/locales/en.yml b/config/locales/en.yml
index 0356f5d5..356aefde 100644
--- a/config/locales/en.yml
+++ b/config/locales/en.yml
@@ -2,6 +2,7 @@ en:
   login_credential_acceptor:
     invalid_login_ticket: "Your login request did not include a valid login ticket."
     invalid_login_credentials: "Incorrect username or password."
+    user_is_locked: "Your user is currently locked because of failed login attempts."
   login:
     label_username: "Username"
     label_password: "Password"
diff --git a/config/locales/fr.yml b/config/locales/fr.yml
index 9c74ea5d..4b552ad1 100644
--- a/config/locales/fr.yml
+++ b/config/locales/fr.yml
@@ -2,6 +2,7 @@ fr:
   login_credential_acceptor:
     invalid_login_ticket: "La demande de connexion n'inclue pas un ticket de connexion valide."
     invalid_login_credentials: "Nom d'utilisateur ou mot de passe incorrect."
+    user_is_locked: "Votre utilisateur est actuellement bloqué dû à des tentatives de connexions échouées."
   login:
     label_username: "Nom d'utilisateur"
     label_password: "Mot de passe"
diff --git a/spec/controllers/sessions_controller_spec.rb b/spec/controllers/sessions_controller_spec.rb
index 264898c0..0741ddec 100644
--- a/spec/controllers/sessions_controller_spec.rb
+++ b/spec/controllers/sessions_controller_spec.rb
@@ -188,7 +188,8 @@
       let(:login_ticket) { FactoryGirl.create :login_ticket }
       let(:username) { 'testuser' }
       let(:params) { { lt: login_ticket.ticket, username: username, password: 'wrrooonnng' }}
-      let!(:user) { FactoryGirl.create :user, username: username }
+      let(:locked_until) { nil }
+      let!(:user) { FactoryGirl.create :user, authenticator: 'static', username: username, locked_until: locked_until }
 
       context 'with invalid credentials' do
         it 'renders the new template' do
@@ -328,6 +329,8 @@
           end
 
           context 'when the user does not exist yet' do
+            before { CASino::User.destroy_all }
+
             it 'generates exactly one user' do
               lambda do
                 post :create, params
@@ -385,6 +388,20 @@
             end.should change(CASino::TicketGrantingTicket, :count).by(1)
           end
         end
+
+        context 'when the user is locked' do
+          let(:locked_until) { 5.minutes.from_now }
+
+          it 'renders the new template' do
+            post :create, params
+            expect(response).to render_template(:new)
+          end
+
+          it 'sets a flash to inform the user' do
+            post :create, params
+            expect(flash[:error]).to be_present
+          end
+        end
       end
     end
   end

From 1107716ae9a811d98637d500109d71e27244fd21 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Philippe=20H=C3=A4ssig?= <phil@nine.ch>
Date: Fri, 19 Aug 2016 13:50:53 +0200
Subject: [PATCH 03/14] Allow to disable th max failed attempts feature

It is no possible to disable the feature by setting the configuration for max failed login attempts to -1.
---
 app/helpers/casino/sessions_helper.rb |  5 +++--
 app/models/casino/user.rb             |  2 +-
 config/cas.yml                        |  1 +
 spec/model/user_spec.rb               | 10 ++++++++++
 4 files changed, 15 insertions(+), 3 deletions(-)

diff --git a/app/helpers/casino/sessions_helper.rb b/app/helpers/casino/sessions_helper.rb
index 5d6cf574..dcf3b658 100644
--- a/app/helpers/casino/sessions_helper.rb
+++ b/app/helpers/casino/sessions_helper.rb
@@ -56,7 +56,7 @@ def sign_out
   def handle_failed_login(username)
     CASino::User.where(username: username).each do |user|
       create_login_attempt(user, false)
-      lock_user(user) if user.max_failed_logins_reached?(CASino.config.max_failed_login_attempts)
+      prevent_brute_force(user)
     end
   end
 
@@ -95,7 +95,8 @@ def handle_signed_in_with_service(tgt, options)
     end
   end
 
-  def lock_user(user)
+  def prevent_brute_force(user)
+    return unless user.max_failed_logins_reached?(CASino.config.max_failed_login_attempts)
     user.update locked_until: LOCK_TIMEOUT.from_now
   end
 end
diff --git a/app/models/casino/user.rb b/app/models/casino/user.rb
index b110d02c..015fa723 100644
--- a/app/models/casino/user.rb
+++ b/app/models/casino/user.rb
@@ -16,7 +16,7 @@ def locked?
   end
 
   def max_failed_logins_reached?(max)
-    max ||= 5
+    return false if max.to_i <= 0
     login_attempts.last(max).count(&:failed?) == max
   end
 end
diff --git a/config/cas.yml b/config/cas.yml
index 26818b50..713cda8c 100644
--- a/config/cas.yml
+++ b/config/cas.yml
@@ -1,4 +1,5 @@
 defaults: &defaults
+  max_failed_login_attempts: -1 # disabled
   service_ticket:
     lifetime_unconsumed: 299
   authenticators:
diff --git a/spec/model/user_spec.rb b/spec/model/user_spec.rb
index 3bfcd54e..a7b31ea4 100644
--- a/spec/model/user_spec.rb
+++ b/spec/model/user_spec.rb
@@ -33,6 +33,16 @@
       it { is_expected.to eq false }
     end
 
+    context 'when the feature is disabled' do
+      let(:max_failed_attempts) { -1 }
+      it { is_expected.to eq false }
+    end
+
+    context 'when the maxium value is invalid' do
+      let(:max_failed_attempts) { nil }
+      it { is_expected.to eq false }
+    end
+
     context 'when the maximum of attempts is reached' do
       before { FactoryGirl.create_list :login_attempt, 2, successful: false, user: user }
 

From 77f73f1e027bcf1fdd76a81df9d5b92ad587282c Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Philippe=20H=C3=A4ssig?= <phil@nine.ch>
Date: Fri, 19 Aug 2016 13:57:45 +0200
Subject: [PATCH 04/14] Make the lock timeout configurable

---
 app/helpers/casino/sessions_helper.rb | 3 ++-
 config/cas.yml                        | 1 +
 spec/dummy/config/cas.yml             | 2 ++
 3 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/app/helpers/casino/sessions_helper.rb b/app/helpers/casino/sessions_helper.rb
index dcf3b658..98b7de16 100644
--- a/app/helpers/casino/sessions_helper.rb
+++ b/app/helpers/casino/sessions_helper.rb
@@ -97,6 +97,7 @@ def handle_signed_in_with_service(tgt, options)
 
   def prevent_brute_force(user)
     return unless user.max_failed_logins_reached?(CASino.config.max_failed_login_attempts)
-    user.update locked_until: LOCK_TIMEOUT.from_now
+    lock_timeout_minutes = CASino.config.failed_login_lock_timeout.to_i.minutes
+    user.update locked_until: lock_timeout_minutes.from_now
   end
 end
diff --git a/config/cas.yml b/config/cas.yml
index 713cda8c..6f1e27b2 100644
--- a/config/cas.yml
+++ b/config/cas.yml
@@ -1,5 +1,6 @@
 defaults: &defaults
   max_failed_login_attempts: -1 # disabled
+  failed_login_lock_timeout: 5 # minutes a user gets locked for when using max_failed_login_attempts
   service_ticket:
     lifetime_unconsumed: 299
   authenticators:
diff --git a/spec/dummy/config/cas.yml b/spec/dummy/config/cas.yml
index 2915edc4..e8f1626b 100644
--- a/spec/dummy/config/cas.yml
+++ b/spec/dummy/config/cas.yml
@@ -1,4 +1,6 @@
 defaults: &defaults
+  max_failed_login_attempts: 5
+  failed_login_lock_timeout: 5
   login_ticket:
     lifetime: 600
   service_ticket:

From a32fa91e2a7eaccff4abd164839dabb26be14f92 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Christian=20Ma=CC=88der?= <cma@nine.ch>
Date: Thu, 3 Nov 2016 09:39:35 +0100
Subject: [PATCH 05/14] Defers credential check

To prevent possible timing attacks, a concern raised in [1], this defers
the credentials check until it's validated that the user is not
currently locked.

[1] https://github.com/rbCAS/CASino/pull/169#issuecomment-242814703
---
 app/controllers/casino/sessions_controller.rb | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/app/controllers/casino/sessions_controller.rb b/app/controllers/casino/sessions_controller.rb
index 390e7cd1..d0bf3ce8 100644
--- a/app/controllers/casino/sessions_controller.rb
+++ b/app/controllers/casino/sessions_controller.rb
@@ -22,11 +22,11 @@ def new
 
   def create
     validation_result = validate_login_credentials(params[:username], params[:password])
-    if !validation_result
+    if user_from_validation_result(validation_result).locked?
+      show_login_error I18n.t('login_credential_acceptor.user_is_locked')
+    elsif !validation_result
       handle_failed_login params[:username]
       show_login_error I18n.t('login_credential_acceptor.invalid_login_credentials')
-    elsif user_from_validation_result(validation_result).locked?
-      show_login_error I18n.t('login_credential_acceptor.user_is_locked')
     else
       sign_in(validation_result, long_term: params[:rememberMe], credentials_supplied: true)
     end

From 8ec656283e53a2ad3083df868aa7ebd4c18043e4 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Christian=20Ma=CC=88der?= <cma@nine.ch>
Date: Thu, 3 Nov 2016 09:45:51 +0100
Subject: [PATCH 06/14] Removes superfluous instruction.

---
 app/helpers/casino/sessions_helper.rb | 2 --
 1 file changed, 2 deletions(-)

diff --git a/app/helpers/casino/sessions_helper.rb b/app/helpers/casino/sessions_helper.rb
index 98b7de16..880bb610 100644
--- a/app/helpers/casino/sessions_helper.rb
+++ b/app/helpers/casino/sessions_helper.rb
@@ -4,8 +4,6 @@ module CASino::SessionsHelper
   include CASino::TicketGrantingTicketProcessor
   include CASino::ServiceTicketProcessor
 
-  LOCK_TIMEOUT = 5.minutes
-
   def current_ticket_granting_ticket?(ticket_granting_ticket)
     ticket_granting_ticket.ticket == cookies[:tgt]
   end

From fc49a9abfbf1c4afe609ea3c2a7fe46b062d897f Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Christian=20Ma=CC=88der?= <cma@nine.ch>
Date: Thu, 3 Nov 2016 10:16:20 +0100
Subject: [PATCH 07/14] Revert "Defers credential check"

This reverts commit a32fa91e2a7eaccff4abd164839dabb26be14f92.
---
 app/controllers/casino/sessions_controller.rb | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/app/controllers/casino/sessions_controller.rb b/app/controllers/casino/sessions_controller.rb
index d0bf3ce8..390e7cd1 100644
--- a/app/controllers/casino/sessions_controller.rb
+++ b/app/controllers/casino/sessions_controller.rb
@@ -22,11 +22,11 @@ def new
 
   def create
     validation_result = validate_login_credentials(params[:username], params[:password])
-    if user_from_validation_result(validation_result).locked?
-      show_login_error I18n.t('login_credential_acceptor.user_is_locked')
-    elsif !validation_result
+    if !validation_result
       handle_failed_login params[:username]
       show_login_error I18n.t('login_credential_acceptor.invalid_login_credentials')
+    elsif user_from_validation_result(validation_result).locked?
+      show_login_error I18n.t('login_credential_acceptor.user_is_locked')
     else
       sign_in(validation_result, long_term: params[:rememberMe], credentials_supplied: true)
     end

From 5167b81a909bf2ede250d16d31c5c5fb056e5073 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Christian=20Ma=CC=88der?= <cma@nine.ch>
Date: Thu, 3 Nov 2016 11:37:15 +0100
Subject: [PATCH 08/14] Defers credential check, take two.

Prevents timing attack concern by checking the user lock status first. The concern seemed very valid and was raised in https://github.com/rbCAS/CASino/pull/169#issuecomment-242814703 .
---
 app/controllers/casino/sessions_controller.rb | 17 +++++------------
 app/helpers/casino/sessions_helper.rb         |  6 ++++++
 2 files changed, 11 insertions(+), 12 deletions(-)

diff --git a/app/controllers/casino/sessions_controller.rb b/app/controllers/casino/sessions_controller.rb
index 390e7cd1..64681d62 100644
--- a/app/controllers/casino/sessions_controller.rb
+++ b/app/controllers/casino/sessions_controller.rb
@@ -21,14 +21,14 @@ def new
   end
 
   def create
+    return show_login_error I18n.t('login_credential_acceptor.user_is_locked') if user_locked?(params[:username])
+
     validation_result = validate_login_credentials(params[:username], params[:password])
-    if !validation_result
+    if validation_result
+      sign_in(validation_result, long_term: params[:rememberMe], credentials_supplied: true)
+    else
       handle_failed_login params[:username]
       show_login_error I18n.t('login_credential_acceptor.invalid_login_credentials')
-    elsif user_from_validation_result(validation_result).locked?
-      show_login_error I18n.t('login_credential_acceptor.user_is_locked')
-    else
-      sign_in(validation_result, long_term: params[:rememberMe], credentials_supplied: true)
     end
   end
 
@@ -85,11 +85,4 @@ def load_ticket_granting_ticket_from_parameter
     @ticket_granting_ticket = find_valid_ticket_granting_ticket(params[:tgt], request.user_agent, ignore_two_factor: true)
     redirect_to login_path if @ticket_granting_ticket.nil?
   end
-
-  def user_from_validation_result(validation_result)
-      user_data = validation_result[:user_data]
-      load_or_initialize_user(validation_result[:authenticator],
-                              user_data[:username],
-                              user_data[:extra_attributes])
-  end
 end
diff --git a/app/helpers/casino/sessions_helper.rb b/app/helpers/casino/sessions_helper.rb
index 880bb610..a245cbc6 100644
--- a/app/helpers/casino/sessions_helper.rb
+++ b/app/helpers/casino/sessions_helper.rb
@@ -51,6 +51,12 @@ def sign_out
     cookies.delete :tgt
   end
 
+  def user_locked?(username)
+    CASino::User.where(username: username).to_a.any? do |user|
+      user.locked?
+    end
+  end
+
   def handle_failed_login(username)
     CASino::User.where(username: username).each do |user|
       create_login_attempt(user, false)

From b81827b8daecfb84be6b5bb8beff3cab7e2917f0 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Christian=20Ma=CC=88der?= <cma@nine.ch>
Date: Fri, 4 Nov 2016 10:47:11 +0100
Subject: [PATCH 09/14] Improves check whether any given user is locked.

---
 app/helpers/casino/sessions_helper.rb | 4 +---
 app/models/casino/user.rb             | 2 ++
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/app/helpers/casino/sessions_helper.rb b/app/helpers/casino/sessions_helper.rb
index a245cbc6..8a8f5d08 100644
--- a/app/helpers/casino/sessions_helper.rb
+++ b/app/helpers/casino/sessions_helper.rb
@@ -52,9 +52,7 @@ def sign_out
   end
 
   def user_locked?(username)
-    CASino::User.where(username: username).to_a.any? do |user|
-      user.locked?
-    end
+    CASino::User.locked.where(username: username).any?
   end
 
   def handle_failed_login(username)
diff --git a/app/models/casino/user.rb b/app/models/casino/user.rb
index 015fa723..79bdd075 100644
--- a/app/models/casino/user.rb
+++ b/app/models/casino/user.rb
@@ -6,6 +6,8 @@ class CASino::User < ActiveRecord::Base
   has_many :two_factor_authenticators
   has_many :login_attempts
 
+  scope :locked, -> { where('locked_until > ?', Time.now) }
+
   def active_two_factor_authenticator
     self.two_factor_authenticators.where(active: true).first
   end

From e57aa7361d2b1ed26a494c076d8d9bcd6b7b2d82 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Christian=20Ma=CC=88der?= <cma@nine.ch>
Date: Thu, 24 Nov 2016 14:38:04 +0100
Subject: [PATCH 10/14] No separate message when account is locked by default.

There should not be a separate message when the account is locked
due to too many login attempts, as this would be an information leak.
---
 config/locales/ar.yml    | 1 +
 config/locales/de.yml    | 4 ++--
 config/locales/en.yml    | 4 ++--
 config/locales/fr.yml    | 4 ++--
 config/locales/pt-BR.yml | 1 +
 config/locales/ru.yml    | 1 +
 config/locales/zh-CN.yml | 1 +
 config/locales/zh-TW.yml | 1 +
 8 files changed, 11 insertions(+), 6 deletions(-)

diff --git a/config/locales/ar.yml b/config/locales/ar.yml
index 0b5075e8..65da5038 100644
--- a/config/locales/ar.yml
+++ b/config/locales/ar.yml
@@ -2,6 +2,7 @@ ar:
   login_credential_acceptor:
     invalid_login_ticket: "لم يتضمّن طلب تسجيل دخولك شهادة تسجيل صالحة."
     invalid_login_credentials: "اسم المستخدم أو كلمة المرور غير صحيحة."
+    user_is_locked: "اسم المستخدم أو كلمة المرور غير صحيحة."
   login:
     label_username: "اسم المستخدم"
     label_password: "كلمة المرور"
diff --git a/config/locales/de.yml b/config/locales/de.yml
index 64d59610..9fbe0cef 100644
--- a/config/locales/de.yml
+++ b/config/locales/de.yml
@@ -1,8 +1,8 @@
 de:
   login_credential_acceptor:
     invalid_login_ticket: "Ihre Anfrage enthielt kein gültiges Login-Ticket."
-    invalid_login_credentials: "Benutzername oder Passwort falsch."
-    user_is_locked: "Ihr Account ist wegen zu vieler falscher Loginversuche gesperrt."
+    invalid_login_credentials: "Benutzername oder Passwort falsch oder Konto gesperrt."
+    user_is_locked: "Benutzername oder Passwort falsch oder Konto gesperrt."
   login:
     label_username: "Benutzername"
     label_password: "Passwort"
diff --git a/config/locales/en.yml b/config/locales/en.yml
index 356aefde..f30ba553 100644
--- a/config/locales/en.yml
+++ b/config/locales/en.yml
@@ -1,8 +1,8 @@
 en:
   login_credential_acceptor:
     invalid_login_ticket: "Your login request did not include a valid login ticket."
-    invalid_login_credentials: "Incorrect username or password."
-    user_is_locked: "Your user is currently locked because of failed login attempts."
+    invalid_login_credentials: "Incorrect username or password or locked account."
+    user_is_locked: "Incorrect username or password or locked account."
   login:
     label_username: "Username"
     label_password: "Password"
diff --git a/config/locales/fr.yml b/config/locales/fr.yml
index 4b552ad1..35809175 100644
--- a/config/locales/fr.yml
+++ b/config/locales/fr.yml
@@ -1,8 +1,8 @@
 fr:
   login_credential_acceptor:
     invalid_login_ticket: "La demande de connexion n'inclue pas un ticket de connexion valide."
-    invalid_login_credentials: "Nom d'utilisateur ou mot de passe incorrect."
-    user_is_locked: "Votre utilisateur est actuellement bloqué dû à des tentatives de connexions échouées."
+    invalid_login_credentials: "Nom d'utilisateur ou mot de passe incorrect ou l'utilisateur est bloqué."
+    user_is_locked: "Nom d'utilisateur ou mot de passe incorrect ou l'utilisateur est bloqué."
   login:
     label_username: "Nom d'utilisateur"
     label_password: "Mot de passe"
diff --git a/config/locales/pt-BR.yml b/config/locales/pt-BR.yml
index 8376a8a3..ede23507 100644
--- a/config/locales/pt-BR.yml
+++ b/config/locales/pt-BR.yml
@@ -2,6 +2,7 @@ pt-BR:
   login_credential_acceptor:
     invalid_login_ticket: "Por favor, insira seus dados de acesso."
     invalid_login_credentials: "Usuário ou senha incorretos."
+    user_is_locked: "Usuário ou senha incorretos."
   login:
     label_username: "Usuário"
     label_password: "Senha"
diff --git a/config/locales/ru.yml b/config/locales/ru.yml
index 31d05d1a..ea3f396b 100644
--- a/config/locales/ru.yml
+++ b/config/locales/ru.yml
@@ -2,6 +2,7 @@ ru:
   login_credential_acceptor:
     invalid_login_ticket: "Ваш запрос на авторизацию не содержит корректную информацию об авторизации."
     invalid_login_credentials: "Неверное имя пользователя или пароль."
+    user_is_locked: "Неверное имя пользователя или пароль."
   login:
     label_username: "Имя пользователя"
     label_password: "Пароль"
diff --git a/config/locales/zh-CN.yml b/config/locales/zh-CN.yml
index 653f0a65..bf6e90b0 100644
--- a/config/locales/zh-CN.yml
+++ b/config/locales/zh-CN.yml
@@ -2,6 +2,7 @@ zh-CN:
   login_credential_acceptor:
     invalid_login_ticket: "您的登录请求没有包含有效的登录授权。"
     invalid_login_credentials: "用户名或密码错误"
+    user_is_locked: "用户名或密码错误"
   login:
     label_username: "用户名"
     label_password: "密码"
diff --git a/config/locales/zh-TW.yml b/config/locales/zh-TW.yml
index d577709d..57cb5ca9 100644
--- a/config/locales/zh-TW.yml
+++ b/config/locales/zh-TW.yml
@@ -2,6 +2,7 @@ zh-TW:
   login_credential_acceptor:
     invalid_login_ticket: "您的登錄請求沒有包含有效的登錄授權。"
     invalid_login_credentials: "用戶名或密碼錯誤"
+    user_is_locked: "用戶名或密碼錯誤"
   login:
     label_username: "用戶名"
     label_password: "密碼"

From 4177592835968c524788f1a447d3f33ab4c2998d Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Christian=20Ma=CC=88der?= <cma@nine.ch>
Date: Fri, 25 Nov 2016 12:56:34 +0100
Subject: [PATCH 11/14] Fixes the check whether a user is actually locked.

Before, the user was claimed to be locked when any CASino::User sharing
the same username was locked. But a user is actually only locked, if he
does not have any CASino::User left, which is unlocked. The reason
herefore is, that he could have any secondary/legacy authentication
providers, on which he can never login. Therefore, one of his 'alter
egos' may be getting locked on any regular (and otherwise successful)
login attempt and he would be locked out regardless. That is now
changed, so that all CASino::Users sharing the same username must be
first be in locked state before one is considered locked.
---
 app/helpers/casino/sessions_helper.rb | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/app/helpers/casino/sessions_helper.rb b/app/helpers/casino/sessions_helper.rb
index 8a8f5d08..4fbe8476 100644
--- a/app/helpers/casino/sessions_helper.rb
+++ b/app/helpers/casino/sessions_helper.rb
@@ -52,7 +52,16 @@ def sign_out
   end
 
   def user_locked?(username)
-    CASino::User.locked.where(username: username).any?
+    result = CASino::User.where(username: username)
+
+
+    # If we've never seen this user before, it can't be locked already.
+    return false if result.empty?
+
+    # A user is only locked, if all its CASino::Users, from all providers, are locked.
+    # Because it might be, that it is locked for one (e.g. legacy) provider, but not for another.
+    # So it should still have the chance to login to said other provider.
+    return result.where('locked_until IS NULL or locked_until <= :now', username: username, now: Time.now).empty?
   end
 
   def handle_failed_login(username)

From b4c741d1866c35328004859c0a25f61817417f55 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Christian=20Ma=CC=88der?= <cma@nine.ch>
Date: Fri, 25 Nov 2016 15:42:53 +0100
Subject: [PATCH 12/14] Adds information about lock status to users rake task

---
 lib/casino/tasks/user.rake | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/lib/casino/tasks/user.rake b/lib/casino/tasks/user.rake
index e9965af1..fb510fd6 100644
--- a/lib/casino/tasks/user.rake
+++ b/lib/casino/tasks/user.rake
@@ -6,11 +6,11 @@ namespace :casino do
     task :search, [:query] => :environment do |task, args|
       users = CASino::User.where('username LIKE ?', "%#{args[:query]}%")
       if users.any?
-        headers = ['User ID', 'Username', 'Authenticator', 'Two-factor authentication enabled?']
+        headers = ['User ID', 'Username', 'Authenticator', 'Two-factor authentication enabled?', 'Locked?']
         table = Terminal::Table.new :headings => headers do |t|
           users.each do |user|
             two_factor_enabled = user.active_two_factor_authenticator ? 'yes' : 'no'
-            t.add_row [user.id, user.username, user.authenticator, two_factor_enabled]
+            t.add_row [user.id, user.username, user.authenticator, two_factor_enabled, user.locked_until.future?]
           end
         end
         puts table

From b8b305e536986f1cb2cb014e60d4ce0f327ee355 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Christian=20Ma=CC=88der?= <cma@nine.ch>
Date: Fri, 25 Nov 2016 15:59:36 +0100
Subject: [PATCH 13/14] Adds a rake task to unlock locked users and list lock
 status

---
 lib/casino/tasks/user.rake | 40 +++++++++++++++++++++++++++++++-------
 1 file changed, 33 insertions(+), 7 deletions(-)

diff --git a/lib/casino/tasks/user.rake b/lib/casino/tasks/user.rake
index fb510fd6..84de7e2a 100644
--- a/lib/casino/tasks/user.rake
+++ b/lib/casino/tasks/user.rake
@@ -3,14 +3,28 @@ require 'terminal-table'
 namespace :casino do
   namespace :user do
     desc 'Search users by name.'
-    task :search, [:query] => :environment do |task, args|
+    task :search, [:query] => :environment do |_task, args|
       users = CASino::User.where('username LIKE ?', "%#{args[:query]}%")
       if users.any?
-        headers = ['User ID', 'Username', 'Authenticator', 'Two-factor authentication enabled?', 'Locked?']
-        table = Terminal::Table.new :headings => headers do |t|
+        headers = [
+          'User ID',
+          'Username',
+          'Authenticator',
+          'Two-factor authentication enabled?',
+          'Lock active?',
+          'Locked until',
+        ]
+        table = Terminal::Table.new(headings: headers) do |t|
           users.each do |user|
             two_factor_enabled = user.active_two_factor_authenticator ? 'yes' : 'no'
-            t.add_row [user.id, user.username, user.authenticator, two_factor_enabled, user.locked_until.future?]
+            t.add_row [
+              user.id,
+              user.username,
+              user.authenticator,
+              two_factor_enabled,
+              user.locked_until.future?,
+              user.locked_until,
+            ]
           end
         end
         puts table
@@ -20,13 +34,25 @@ namespace :casino do
     end
 
     desc 'Deactivate two-factor authentication for a user.'
-    task :deactivate_two_factor_authentication, [:user_id] => :environment do |task, args|
-      if CASino::User.find(args[:user_id]).active_two_factor_authenticator
-        CASino::User.find(args[:user_id]).active_two_factor_authenticator.destroy
+    task :deactivate_two_factor_authentication, [:user_id] => :environment do |_task, args|
+      user = CASino::User.find args[:user_id]
+      if user.active_two_factor_authenticator
+        user.active_two_factor_authenticator.destroy
         puts "Successfully deactivated two-factor authentication for user ##{args[:user_id]}."
       else
         puts "No two-factor authenticator found for user ##{args[:user_id]}."
       end
     end
+
+    desc 'Re-enable locked user.'
+    task :reenable_locked_user, [:user_id] => :environment do |_task, args|
+      user = CASino::User.find args[:user_id]
+      if user.locked_until.nil? || user.locked_until.past?
+        puts "The given user ##{args[:user_id]} is not locked."
+      else
+        user.update(locked_until: nil)
+        puts "The given user ##{args[:user_id]} was successfully unlocked."
+      end
+    end
   end
 end

From 672d1761becc9d47ca779fd9b78936ec8ea33519 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Christian=20Ma=CC=88der?= <cma@nine.ch>
Date: Fri, 25 Nov 2016 16:08:01 +0100
Subject: [PATCH 14/14] Enhanced with nil check

---
 lib/casino/tasks/user.rake | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/lib/casino/tasks/user.rake b/lib/casino/tasks/user.rake
index 84de7e2a..95726202 100644
--- a/lib/casino/tasks/user.rake
+++ b/lib/casino/tasks/user.rake
@@ -17,12 +17,17 @@ namespace :casino do
         table = Terminal::Table.new(headings: headers) do |t|
           users.each do |user|
             two_factor_enabled = user.active_two_factor_authenticator ? 'yes' : 'no'
+            user_locked = if user.locked_until.nil?
+                            'no'
+                          else
+                            user.locked_until.future? ? 'yes' : 'no'
+                          end
             t.add_row [
               user.id,
               user.username,
               user.authenticator,
               two_factor_enabled,
-              user.locked_until.future?,
+              user_locked,
               user.locked_until,
             ]
           end