diff --git a/app/models/authenticator/base.rb b/app/models/authenticator/base.rb index 716dcfe90d0..25858aa0835 100644 --- a/app/models/authenticator/base.rb +++ b/app/models/authenticator/base.rb @@ -44,7 +44,6 @@ def authenticate(username, password, request = nil, options = {}) options = options.dup options[:require_user] ||= false options[:authorize_only] ||= false - fail_message = _("Authentication failed") user_or_taskid = nil @@ -52,7 +51,10 @@ def authenticate(username, password, request = nil, options = {}) username = normalize_username(username) audit = {:event => audit_event, :userid => username} - authenticated = options[:authorize_only] || _authenticate(username, password, request) + # The fail_message might or might not come from the _authenticate method + authenticated, fail_message = options[:authorize_only] || _authenticate(username, password, request) + fail_message ||= _("Authentication failed") # Fall back to the default fail_message + if authenticated audit_success(audit.merge(:message => "User #{username} successfully validated by #{self.class.proper_name}")) diff --git a/app/models/authenticator/database.rb b/app/models/authenticator/database.rb index 438841d8c69..12d513d6519 100644 --- a/app/models/authenticator/database.rb +++ b/app/models/authenticator/database.rb @@ -13,7 +13,15 @@ def uses_stored_password? def _authenticate(username, password, _request) user = case_insensitive_find_by_userid(username) - user && user.authenticate_bcrypt(password) + return [false, _('Your account has been locked due to too many failed login attempts, please contact the administrator.')] if user&.locked? + + if user&.authenticate_bcrypt(password) # Authenticate if the username matches + user.unlock! # Reset the number of failed logins + return true + end + + user&.fail_login! # Increase the number of failed login attempts + [false, _("The username or password you entered is incorrect.")] end def authorize? diff --git a/app/models/user.rb b/app/models/user.rb index 438879792a1..db2f0b5b2c5 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -53,6 +53,8 @@ class User < ApplicationRecord serialize :settings, Hash # Implement settings column as a hash default_value_for(:settings) { Hash.new } + default_value_for :failed_login_attempts, 0 + scope :with_same_userid, ->(id) { where(:userid => User.where(:id => id).pluck(:userid)) } def self.with_roles_excluding(identifier) @@ -168,6 +170,20 @@ def change_password(oldpwd, newpwd) end end + def locked? + ::Settings.authentication.max_failed_login_attempts.positive? && failed_login_attempts >= ::Settings.authentication.max_failed_login_attempts + end + + def unlock! + update!(:failed_login_attempts => 0) + end + + def fail_login! + update!(:failed_login_attempts => failed_login_attempts + 1) + + unlock_queue if locked? + end + def ldap_group current_group.try(:description) end @@ -366,4 +382,17 @@ def self.seed_data File.exist?(seed_file_name) ? YAML.load_file(seed_file_name) : [] end private_class_method :seed_data + + private + + def unlock_queue + MiqQueue.put_or_update( + :class_name => self.class.name, + :instance_id => id, + :method_name => 'unlock!', + :priority => MiqQueue::MAX_PRIORITY + ) do |_msg, queue_options| + queue_options.merge(:deliver_on => Time.now.utc + ::Settings.authentication.locked_account_timeout.to_i) + end + end end diff --git a/config/settings.yml b/config/settings.yml index c408f605d24..9e224377838 100644 --- a/config/settings.yml +++ b/config/settings.yml @@ -11,6 +11,8 @@ :ldaphost: :ldapport: '389' :mode: database + :max_failed_login_attempts: 3 + :locked_account_timeout: 2.minutes :search_timeout: 30 :user_suffix: :user_type: userprincipalname diff --git a/spec/models/authenticator/database_spec.rb b/spec/models/authenticator/database_spec.rb index 0e5bf265ac2..22d102bbdc0 100644 --- a/spec/models/authenticator/database_spec.rb +++ b/spec/models/authenticator/database_spec.rb @@ -56,6 +56,24 @@ def authenticate it "updates lastlogon" do expect { authenticate }.to(change { alice.reload.lastlogon }) end + + it "resets failed login attempts" do + alice.update(:failed_login_attempts => 1) + authenticate + expect(alice.reload.failed_login_attempts).to eq(0) + end + + context "with too many failed login attempts" do + before do + EvmSpecHelper.create_guid_miq_server_zone + alice.update(:failed_login_attempts => 4) + allow(alice).to receive(:unlock_queue) + end + + it 'fails' do + expect { authenticate }.to raise_error(MiqException::MiqEVMLoginError, "Your account has been locked due to too many failed login attempts, please contact the administrator.") + end + end end context "with bad password" do @@ -66,7 +84,13 @@ def authenticate end it "fails" do - expect { authenticate }.to raise_error(MiqException::MiqEVMLoginError, "Authentication failed") + expect { authenticate }.to raise_error(MiqException::MiqEVMLoginError, "The username or password you entered is incorrect.") + end + + it "increases the number of failed logins" do + count = alice.failed_login_attempts + authenticate rescue nil + expect(alice.reload.failed_login_attempts).to eq(count + 1) end it "records one failing audit entry" do @@ -81,7 +105,7 @@ def authenticate it "logs the failure" do allow($log).to receive(:warn).with(/Audit/) - expect($log).to receive(:warn).with(/Authentication failed$/) + expect($log).to receive(:warn).with(/The username or password you entered is incorrect.$/) authenticate rescue nil end @@ -113,7 +137,7 @@ def authenticate it "logs the failure" do allow($log).to receive(:warn).with(/Audit/) - expect($log).to receive(:warn).with(/Authentication failed$/) + expect($log).to receive(:warn).with(/The username or password you entered is incorrect.$/) authenticate rescue nil end end diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index e4995a01f49..40ae6c65f67 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -145,6 +145,21 @@ end end + describe '#fail_login!' do + let(:user) { FactoryBot.create(:user, :password => "smartvm", :failed_login_attempts => 0) } + + it 'increases the number of failed login attempts' do + user.fail_login! + expect(user.reload.failed_login_attempts).to eq(1) + end + + it 'queues an unlock task if the account is locked' do + allow(user).to receive(:locked?).and_return(true) + expect(user).to receive(:unlock_queue) + user.fail_login! + end + end + context "#authorize_ldap" do before do @fq_user = "thin1@manageiq.com"