Skip to content
This repository has been archived by the owner on Feb 6, 2024. It is now read-only.

Lock user after a configurable amount of failed login attempts #169

Open
wants to merge 14 commits into
base: master
Choose a base branch
from
Open
10 changes: 6 additions & 4 deletions app/controllers/casino/sessions_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -21,12 +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
log_failed_login params[:username]
show_login_error I18n.t('login_credential_acceptor.invalid_login_credentials')
else
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')
end
end

Expand Down
13 changes: 12 additions & 1 deletion app/helpers/casino/sessions_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -51,9 +51,14 @@ def sign_out
cookies.delete :tgt
end

def log_failed_login(username)
def user_locked?(username)
CASino::User.locked.where(username: username).any?
end

def handle_failed_login(username)
CASino::User.where(username: username).each do |user|
create_login_attempt(user, false)
prevent_brute_force(user)
end
end

Expand Down Expand Up @@ -91,4 +96,10 @@ def handle_signed_in_with_service(tgt, options)
redirect_to url, status: :see_other
end
end

def prevent_brute_force(user)
return unless user.max_failed_logins_reached?(CASino.config.max_failed_login_attempts)
lock_timeout_minutes = CASino.config.failed_login_lock_timeout.to_i.minutes
user.update locked_until: lock_timeout_minutes.from_now
end
end
4 changes: 4 additions & 0 deletions app/models/casino/login_attempt.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2,4 +2,8 @@ class CASino::LoginAttempt < ActiveRecord::Base
include CASino::ModelConcern::BrowserInfo

belongs_to :user

def failed?
!successful?
end
end
12 changes: 12 additions & 0 deletions app/models/casino/user.rb
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,19 @@ 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

def locked?
return false unless locked_until
locked_until.future?
end

def max_failed_logins_reached?(max)
return false if max.to_i <= 0
login_attempts.last(max).count(&:failed?) == max
end
end
2 changes: 2 additions & 0 deletions config/cas.yml
Original file line number Diff line number Diff line change
@@ -1,4 +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:
Expand Down
1 change: 1 addition & 0 deletions config/locales/de.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down
1 change: 1 addition & 0 deletions config/locales/en.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down
1 change: 1 addition & 0 deletions config/locales/fr.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down
5 changes: 5 additions & 0 deletions db/migrate/20160810113208_add_locked_until_to_users.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
class AddLockedUntilToUsers < ActiveRecord::Migration
def change
add_column :casino_users, :locked_until, :datetime
end
end
37 changes: 36 additions & 1 deletion spec/controllers/sessions_controller_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -208,6 +209,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
Expand Down Expand Up @@ -310,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
Expand Down Expand Up @@ -367,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
Expand Down
2 changes: 2 additions & 0 deletions spec/dummy/config/cas.yml
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
defaults: &defaults
max_failed_login_attempts: 5
failed_login_lock_timeout: 5
login_ticket:
lifetime: 600
service_ticket:
Expand Down
Original file line number Diff line number Diff line change
@@ -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
3 changes: 2 additions & 1 deletion spec/dummy/db/schema.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
12 changes: 12 additions & 0 deletions spec/model/login_attempt_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
73 changes: 73 additions & 0 deletions spec/model/user_spec.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,73 @@
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 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 }

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