Skip to content

Commit

Permalink
Make #increment_failed_attempts concurrency safe (#4996)
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::CounterCache.increment_counter` method, which will do both steps at once, reading the value straight from the database.

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. 
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.
  • Loading branch information
tegon authored Dec 28, 2018
1 parent e3a00b2 commit 6270394
Show file tree
Hide file tree
Showing 2 changed files with 13 additions and 2 deletions.
4 changes: 2 additions & 2 deletions lib/devise/models/lockable.rb
Original file line number Diff line number Diff line change
Expand Up @@ -112,8 +112,8 @@ def valid_for_authentication?
end

def increment_failed_attempts
self.failed_attempts ||= 0
self.failed_attempts += 1
self.class.increment_counter(:failed_attempts, id)
reload
end

def unauthenticated_message
Expand Down
11 changes: 11 additions & 0 deletions test/models/lockable_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,17 @@ 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
same_user.increment_failed_attempts

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 6270394

Please sign in to comment.