Skip to content
This repository was archived by the owner on Feb 6, 2024. It is now read-only.
Open
Show file tree
Hide file tree
Changes from 8 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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
15 changes: 14 additions & 1 deletion app/helpers/casino/sessions_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -51,9 +51,16 @@ def sign_out
cookies.delete :tgt
end

def log_failed_login(username)
def user_locked?(username)
CASino::User.where(username: username).to_a.any? do |user|
user.locked?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is .to_a.any? necessary? Also, I would return after the first locked one:

CASino::User.where(username: username).each do |user|
  return true if user.locked?
end

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to_a is necessary to use Enumerable.any?:

any? [{ |obj| block }] → true or false
Passes each element of the collection to the given block. The method returns true if the block ever returns a value other than false or nil.

vs. the ActiveRecord::Relation.any?:

any?()
Returns true if there are any records.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be totally possible to do it through each if that's somehow more efficient.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is where used anyway? Isn't this sufficient?

!!CASino::User.find_by(username: username).try(:locked?)

@cimnine cimnine Nov 3, 2016

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The same username can exist within different adapters. That's why.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm... if there are the same usernames on several backends, all of them should be locked? So just fetching the first one would be okay for this check...?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:seemsgood:

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could also just add a scope for locked users:

CASino::User.locked.where(username: username).any?

end
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 +98,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
10 changes: 10 additions & 0 deletions app/models/casino/user.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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)
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