1

I am trying to limit failed login attempts per ip.

I have the following:

  def validate(email, context)
    attempt = insert_into_attempts(email, context)
    return nil unless allow_login_by_ip(context.ip_address)
    flag_successful_attempt(attempt, context.ip_address)
    load_data
  end

  def allow_login_by_ip(ip_address)
    limit = LoginLimits.new(ip_address).limit
    last_5_attempts = AuthenticationAttempt.select("id","successful").where(ip: ip_address).last(5)
    last_5_attempts.include?("true")
  end 

  def insert_into_attempts(email, context)
    attempt = AuthenticationAttempt.new(
      :email => email,
      :ip => context.ip_address)
    attempt.save
  end 

  def flag_successful_attempt(attempt, ip_address)
    AuthenticationAttempt.where(ip: ip_address).last.update(successful: '1')
  end

The issue I am having is that it always returns fasle. I must be searching the array incorrectly but I am not sure why. last_5_attempts is:

#<AuthenticationAttempt id: 1, successful: false>, 
#<AuthenticationAttempt id: 2, successful: false>, 
#<AuthenticationAttempt id: 3, successful: true>,
#<AuthenticationAttempt id: 4, successful: false>, 
#<AuthenticationAttempt id: 5, successful: false>]
Improve this question
1
  • You might want to consider the rack-attack gem instead of implementing this feature by yourself. Commented Feb 26, 2019 at 7:20

3 Answers 3

Reset to default
5

If you mean true, then you mean:

last_5_attempts.include?(true)

Because:

true == "true"
# => false

Yet that's not quite enough, as you're asking if an array of [id, successful] values has any entry that is literally just true, ([1,true] != true) so you want:

last_5_attempts.any? |id, successful|
  successful
end

You could also omit id from your column fetch since you don't use it and instead:

AuthenticationAttempt.where(ip: ip_address).pluck(:successful).last(5).any?

Where pluck with a single argument returns a "flat" array instead of an array of arrays.

To check for at least one successful login in the last 5 or no login history:

attempts = AuthenticationAttempt.where(ip: ip_address)
!attempts.any? or attempts.pluck(:successful).last(5).any?
Improve this answer
Sign up to request clarification or add additional context in comments.

5 Comments

AuthenticationAttempt.where(ip: ip_address).pluck(:successful).last(5).any? makes everything cleaner and easier. Thank you!
Is there a way to add a condition where it would return true in case ip does not exist or just doesn't have 5 tries yet? I am asking because I am trying to figure out how to deal with new customers but failing to come up with an addition to the method.
That's a little more complicated but I've added an example there.
I am also allowing users to log in if they have under 5 attempts so I added size to it !attempts.any? or attempts.size < 5 or attempts.pluck(:successful).last(5).any?. I guess the any? is not relevant anymore because I am checking size?
Sure. You could go with either none or less than 5, as zero is less than five.
1

try

last_5_attempts.map(&:to_s).include?("true")

instead of

last_5_attempts.include?("true")
Improve this answer

Comments

1 
AuthenticationAttempt.where(ip: ip_address).last(5).exists?(successful: true)
AuthenticationAttempt.where(ip: ip_address).order(id: :desc).limit(5).exists?(successful: true)

You can use ActiveRecord::FinderMethods#exists? to check for a successful attempt without retrieving any data or instantiating any records.

Update: We need to use .order(id: :desc).limit(5) in place of .last(5) to ensure we have an ActiveRecord::Relation instance to call exists? on.

Update 2: exists? replaces any limit given with limit(1)

AuthenticationAttempt.limit(5).exists?
=> SELECT 1 AS one FROM "authentication_attempts" LIMIT $1  [["LIMIT", 1]]

Therefore we need to wrap the subquery in an outer existence query:

AuthenticationAttempt.exists?(AuthenticationAttempt.limit(5))
=> SELECT  1 AS one FROM "authentication_attmepts" WHERE "authentication_attmepts"."id" IN (SELECT  "authentication_attmepts"."id" FROM "authentication_attmepts" LIMIT $1) LIMIT $2  [["LIMIT", 5], ["LIMIT", 1]]

This is a slightly more complex query, but still has the performance benefits of not loading anything from the database. The inner subquery gives us our last 5 attempts, and the outer query checks for the existence of a successful attempt:

 AuthenticationAttempt
   .where(successful: true)
   .exists?(AuthenticationAttempt.where(ip: ip_address).order(id: :desc).limit(5))
Improve this answer

3 Comments

I am not sure I can run exists? on an Array? It gives an error undefined method 'exists?' for []:Array.
@A.J Try order(id: :desc).limit(5) instead of last(5). I thought #last returned an Association, but it looks like I am wrong and it is returning an Array. Let me know if that works and I will update my answer.
@A.J I updated the query. exists? will overwrite any limits, so you need to structure the query to create a subquery of the correct records and then do the existence check on them.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.