Skip to content

Commit

Permalink
Make #increment_failed_attempts concurrency safe
Browse files Browse the repository at this point in the history
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 this operation
[atomically](https://api.rubyonrails.org/classes/ActiveRecord/Persistence.html#method-i-increment-21).

Co-authored-by: Marcos Ferreira <marcos.ferreira@plataformatec.com.br>
  • Loading branch information
tegon and mracos committed Dec 26, 2018
1 parent 8266e85 commit 9616680
Show file tree
Hide file tree
Showing 2 changed files with 14 additions and 2 deletions.
3 changes: 1 addition & 2 deletions lib/devise/models/lockable.rb
Original file line number Diff line number Diff line change
Expand Up @@ -112,8 +112,7 @@ def valid_for_authentication?
end

def increment_failed_attempts
self.failed_attempts ||= 0
self.failed_attempts += 1
increment!(:failed_attempts)
end

def unauthenticated_message
Expand Down
13 changes: 13 additions & 0 deletions test/models/lockable_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,19 @@ def setup
end
end

test "should read failed_attempts from database when incrementing" do
user = create_user
initial_failed_attempts = user.failed_attempts
same_user = User.find(user.id)

user.increment_failed_attempts
user.save
same_user.increment_failed_attempts
same_user.save

assert_equal initial_failed_attempts + 2, user.reload.failed_attempts
end

test 'should be valid for authentication with a unlocked user' do
user = create_user
user.lock_access!
Expand Down

0 comments on commit 9616680

Please sign in to comment.