Skip to content

Fix failed login attempts counter

Imre Farkas requested to merge if-369134-fix_devise_failed_attempts_counter into master

What does this MR do and why?

The problem

Failed login attempts counter is incremented inconsistently. When incorrect password is entered, the counter is incremented by 2. When incorrect OTP is entered, the counter is incremented by 1.

This difference is due to the reason how password and OTP authentication flows are handled.

Password authentication

For password authentication, we rely on devise strategies. SessionsController#create action leads to Warden::Proxy#authenticate running the configured strategies.

Strategy responsible for password authentication is database_authenticatable, but we don't use it explicitly. The reason for that is that 2FA strategies (two_factor_authenticatable, and two_factor_backupable) are incompatible with it.

devise-two-factor's README states:

Loading both :database_authenticatable and :two_factor_authenticatable in a model will allow users to bypass two-factor authenticatable due to the way Warden handles cascading strategies.

and

If present, it will remove :database_authenticatable from the model, as the two strategies are incompatible

But both two_factor_authenticatable, and two_factor_backupable inherited from database_authenticatable. After checking for valid OTP code, they hand over the request params to the parent class which tries password authentication against the database.

In case of an incorrect password, both strategies fail, and each of them tries to increment the failed login attempts counter. This is the reason why the counter is double incremented.

There are existing bug reports describing this:

It also worth noting, that devise-two-factor's README treats two_factor_backupable as an "example extension":

This gem includes TwoFactorBackupable as an example extension meant to solve this problem

2FA OTP

For 2FA OTP, we don't run any of the devise strategies, but we manually call some of the methods (1, 2) of the related devise strategies (1, 2) (a related issue: https://gitlab.com/gitlab-org/gitlab/-/issues/21278). When incorrect OTP is provided, we increment the failed login attempts counter manually.

Calling Warden::Proxy#authenticate would handle 2FA OTP via devise strategies consistent with password authentication.

Despite these 2FA strategies not being used for the actual 2FA OTP authentication, we cannot remove them, because password authentication is done via these 2FA strategies. We also cannot revert back to using database_authenticatable, because we need to configure the parameters of 2FA OTP, so we need to enable these extensions in the User model.

Solution

Combine devise 2FA strategies to avoid double incrementing the failed login attempts counter when incorrect password is provided.

Screenshots or screen recordings

Screenshots are required for UI changes, and strongly recommended for all other merge requests.

Before After

How to set up and validate locally

Numbered steps to set up and validate the change are strongly suggested.

MR acceptance checklist

This checklist encourages us to confirm any changes have been analyzed to reduce risks in quality, performance, reliability, security, and maintainability.

Edited by Imre Farkas

Merge request reports