[PATCH conductor] BZ#864393 Fixed number of logins and number of failed logins for LDAP authentication

Martin Povolny mpovolny at redhat.com
Thu Oct 11 09:06:58 UTC 2012


Hi

2 things:

* the fix seems right, but simillar bug is below in the kerberos login, so please fix that too

  def self.authenticate_using_krb(username, ipaddress)
    u = User.find_by_username(username) || create_krb_user!(username)
    u.login_count += 1
    update_login_attributes(u, ipaddress)
    u.save!
    u
  end

the increment is duplicated with the one in update_login_attributes

2nd thing -- your test:

* in your both tests you check just one counter, which would not detect an error, where on either case both got incremented

I would add an assertion to for the 2nd counter to each of the tests.

Please resubmit a fixed patch.

--martin


----- Original Message -----
From: "Tomáš Hrčka" <thrcka at redhat.com>
To: aeolus-devel at lists.fedorahosted.org
Cc: "Tomáš Hrčka" <thrcka at redhat.com>
Sent: Wednesday, October 10, 2012 4:07:57 PM
Subject: [PATCH conductor] BZ#864393 Fixed number of logins and number of	failed logins for LDAP authentication

http://bugzilla.redhat.com/show_bug.cgi?id=864393
---
 src/app/models/user.rb       |  6 +++++-
 src/spec/models/user_spec.rb | 15 +++++++++++++++
 2 files changed, 20 insertions(+), 1 deletion(-)

diff --git a/src/app/models/user.rb b/src/app/models/user.rb
index 5db8e95..41ac20b 100644
--- a/src/app/models/user.rb
+++ b/src/app/models/user.rb
@@ -122,9 +122,13 @@ class User < ActiveRecord::Base
   def self.authenticate_using_ldap(username, password, ipaddress)
     if Ldap.valid_ldap_authentication?(username, password)
       u = User.find_by_username(username) || create_ldap_user!(username)
-      u.login_count += 1
       update_login_attributes(u, ipaddress)
     else
+      u = User.find_by_username(username)
+      if u.present?
+        u.failed_login_count += 1
+        u.save!
+      end
       u = nil
     end
     u.save! unless u.nil?
diff --git a/src/spec/models/user_spec.rb b/src/spec/models/user_spec.rb
index 9e38278..fbb8990 100644
--- a/src/spec/models/user_spec.rb
+++ b/src/spec/models/user_spec.rb
@@ -111,6 +111,21 @@ describe User do
     u.id.should == user.id
   end
 
+  it "should authenticate a existing user against LDAP and increment login count" do
+    user = FactoryGirl.create(:tuser, :ignore_password => true, :password => nil)
+    Ldap.should_receive(:valid_ldap_authentication?).and_return(true)
+    u = User.authenticate_using_ldap(user.username, 'random', user.last_login_ip)
+    u.login_count.should be(1)
+  end
+
+  it "should not authenticate a existing user against LDAP and increment failed login count" do
+    user = FactoryGirl.create(:tuser, :ignore_password => true, :password => nil)
+    Ldap.should_receive(:valid_ldap_authentication?).and_return(false)
+    User.authenticate_using_ldap(user.username, 'random', user.last_login_ip)
+    user.reload
+    user.failed_login_count.should be(1)
+  end
+
   it "should authenticate a user with valid kerberos ticket" do
     User.authenticate_using_krb('krbuser', '192.168.1.1').should_not be_nil
     u = User.find_by_username('krbuser')
-- 
1.7.11.4




More information about the aeolus-devel mailing list