Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make #increment_failed_attempts concurrency safe #4996

Merged
merged 3 commits into from
Dec 28, 2018

Conversation

tegon
Copy link
Member

@tegon tegon commented Dec 27, 2018

As reported in #4981, the method #increment_failed_attempts of Devise::Models::Lockable was
not concurrency safe. The increment operation was being done in two steps: first the value was read from the database, and then incremented by 1. This may result in wrong values if two requests try to update the value concurrently. For example:

Browser1 -------> Read `failed_attempts` from DB (1) -------> Increment `failed_attempts` to 2
    Browser2 -------> Read `failed_attempts` from DB (1) -------> Increment `failed_attempts` to 2

In the example above, failed_attempts should have been set to 3, but it will be set to 2.

This pull request handles this case by calling ActiveRecord::CounterCache.increment_counter method, which will do both steps at once, reading the value straight from the database.

This pull request also adds a ActiveRecord::AttributeMethods::Dirty#reload call to ensure that the application gets the updated value - i.e. that other request might have updated.
Although this does not ensure that the value is in fact the most recent one - other request could've updated it after the reload call - it seems good enough for this implementation.
Even if a request does not locks the account because it has a stale value, the next one - that updated that value - will do it. That's why we decided not to use a pessimistic lock here.

Closes #4981.

tegon and others added 3 commits December 26, 2018 16:57
As reported in #4981, the
method `#increment_failed_attempts` of `Devise::Models::Lockable` was
not concurrency safe. The increment operation was being done in two
steps: first the value was read from the database, and then incremented
by 1. This may result in wrong values if two requests try to update the
value concurrently. For example:

Browser1 -------> Read `failed_attempts` from DB (1) -------> Increment `failed_attempts` to 2
         Browser2 -------> Read `failed_attempts` from DB (1) -------> Increment `failed_attempts` to 2

In the example above, `failed_attempts` should have been set to 3, but it will be set to 2.
This commit handles this case by calling ActiveRecord's `#increment!`
method, which will do both steps at once, reading the value straight
from the database.

More info: https://api.rubyonrails.org/classes/ActiveRecord/Persistence.html#method-i-increment-21

Co-authored-by: Marcos Ferreira <[email protected]>
`ActiveRecord::Persistence#increment!` started using `ActiveRecord::CounterCache.update_counters` only in Rails 5, which means we got some broken specs in Rails 4.
It turns out that `ActiveRecord::CounterCache.increment_counter` exists
since Rails 4 and it also calls `.update_counters`, so we're using it
instead.
This commit also adds a `ActiveRecord::AttributeMethods::Dirty#reload`
call to ensure that the application gets the updated value - i.e. that
other request might have updated.
@tegon tegon merged commit 6270394 into master Dec 28, 2018
@tegon tegon deleted the let-mf-increment-failed-attempts-concurency branch December 28, 2018 19:00
ouranos added a commit to maestrano/devise that referenced this pull request Mar 14, 2019
@glebsts
Copy link

glebsts commented Aug 12, 2019

Hello, @tegon
Please excuse me for necroposting, but can this code in master be affected by same concurrency problem?

https://github.com/plataformatec/devise/blob/master/lib/devise/hooks/lockable.rb

# After each sign in, if resource responds to failed_attempts, sets it to 0
# This is only triggered when the user is explicitly set (with set_user)
Warden::Manager.after_set_user except: :fetch do |record, warden, options|
  if record.respond_to?(:failed_attempts) && warden.authenticated?(options[:scope])
    unless record.failed_attempts.to_i.zero?
      record.failed_attempts = 0
      record.save(validate: false)
    end
  end
end

We are investigating similar or related behavior reported for latest version, so just in case..
Thank you.

@tegon
Copy link
Member Author

tegon commented Aug 12, 2019

Hi @glebsts, the code you mentioned is for the other case: resetting the failed attempts account - i.e. unlocking a user.
I guess there's no problem in this case because if there are two concurrent requests they're both going to set the value to zero, and not increment like the original issue.
If you found a problem with this part of the code, please open a new issue so that we can discuss it better.

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Small security issue with :lockable
3 participants