lockable with two factor backupable causing double failed attempts

[NickPapacostas]

Heyo,

In our User model we have

class User < ApplicationRecord
  devise :two_factor_authenticatable, :two_factor_backupable, otp_number_of_backup_codes: 10,
         otp_backup_code_length: 6, :otp_secret_encryption_key => ENV['OPT_KEY']

  devise :registerable, :recoverable, :rememberable, :trackable,
         :validatable, :confirmable, :lockable

When I enter an incorrect 2fa code the users failed_attempts is incremented by two instead of 1.

I found where failed_attempts incremented in the lockable code and added a log of caller to see the call stack and I can see that both two_factor_authenticatable and two_factor_backupable are incrementing the failed_attempts.

I've tried various changes to the configuration and re-followed all the setup instructions with no luck. Any insight would be welcome! I'm happy to make a pull request for a fix I just don't know what's wrong.

Thanks!

Rails version: 5.1.4
Ruby: 2.4
gem versions:

    devise (4.4.3)
      bcrypt (~> 3.0)
      orm_adapter (~> 0.1)
      railties (>= 4.1.0, < 6.0)
      responders
      warden (~> 1.2.3)
    devise-two-factor (3.0.3)
      activesupport (< 5.3)
      attr_encrypted (>= 1.3, < 4, != 2)

[SimonVillage]

any update or workaround for this?

[pdkproitf]

Is there any update for this one?

[BiggerNoise]

Definitely an issue. I don't have a P/R for this, but the issue is pretty straightforward. Each of the two strategies calls the base class validate method. If that method fails (including a two factor fail), it records a failure attempt.

My work around was fairly simple:

• Remove the :two_factor_backupable and :two_factor_authenticatable strategies from the default list

initializers/devise.rb

  config.warden do |manager|
    strategies = manager.default_strategies(:scope => :user)
    strategies.delete(:two_factor_backupable)
    strategies.delete(:two_factor_authenticatable)
  end

Register a new strategy that does one check for authentication and two factor. (Mine implements a remember_me, that's the third call):

      def authenticate!
        resource = mapping.to.find_for_database_authentication(authentication_hash)

        if validate(resource) {validate_otp(resource) || recover_otp(resource) || validate_remembered_computer(resource)}
          super
        end

        fail(:not_found_in_database) unless resource

        # We want to cascade to the next strategy if this one fails,
        # but database authenticatable automatically halts on a bad password
        @halted = false if @result == :failure
      end

[verenion]

There is an open PR for this from over 8 months ago. #136. Is this likely to ever be fixed? It's been a problem for 4 years #28.

I would happily offer a PR, but my knowledge of the inner workings of this are lacking.

Definitely an issue. I don't have a P/R for this, but the issue is pretty straightforward. Each of the two strategies calls the base class validate method. If that method fails (including a two factor fail), it records a failure attempt.

My work around was fairly simple:

• Remove the :two_factor_backupable and :two_factor_authenticatable strategies from the default list

initializers/devise.rb

  config.warden do |manager|
    strategies = manager.default_strategies(:scope => :user)
    strategies.delete(:two_factor_backupable)
    strategies.delete(:two_factor_authenticatable)
  end

Register a new strategy that does one check for authentication and two factor. (Mine implements a remember_me, that's the third call):

      def authenticate!
        resource = mapping.to.find_for_database_authentication(authentication_hash)

        if validate(resource) {validate_otp(resource) || recover_otp(resource) || validate_remembered_computer(resource)}
          super
        end

        fail(:not_found_in_database) unless resource

        # We want to cascade to the next strategy if this one fails,
        # but database authenticatable automatically halts on a bad password
        @halted = false if @result == :failure
      end

This sadly just stops the failed_attempts increase completely for me.

[bwencke]

After researching for a couple days and trying many solutions, I found it simplest to just work around this bug in the library for the time being.

You can override the lock_access! method in your User model to return without sending an email if the number of failed attempts does not exactly equal Devise.maximum_attempts. This way, when the lock_access! method is called several times, it will just return without doing anything. An email will only be sent the first time.

Add the following into your User model:

def lock_access!(opts = { })
    # every devise strategy tries to lock the account again, which led to many emails being sent and the unlock token not being correct in all the emails
    # only locking at exactly the max attempt prevents this (although is just working around the problem, instead of fixing the 2FA library)
    return if failed_attempts != Devise.maximum_attempts
    self.locked_at = Time.now.utc

    if unlock_strategy_enabled?(:email) && opts.fetch(:send_instructions, true)
      send_unlock_instructions
    else
      save(validate: false)
    end
end

I should note that if you are having an issue with the failed_attempts incrementing by 2 instead of 1 (I did not have this issue), this probably won't resolve that issue. It will only prevent multiple emails from being sent.

[veekram]

Is this fixed? PR is still open

[bluesnotred]

This sadly just stops the failed_attempts increase completely for me.

@verenion I had some trouble with it myself. As it is now 2022 and there is still no fix for this problem a response anyway:

@BiggerNoise's solution did work for me. Make sure you create a custom strategy like this:

lib/custom_authenticatable.rb

module Devise
  module Strategies
    class CustomAuthenticatable < TwoFactorAuthenticatable

      def authenticate!
        resource = mapping.to.find_for_database_authentication(authentication_hash)

        if validate(resource) {validate_otp(resource) || recover_otp(resource) || validate_remembered_computer(resource)}
          super
        end

        fail(:not_found_in_database) unless resource

        # We want to cascade to the next strategy if this one fails,
        # but database authenticatable automatically halts on a bad password
        @halted = false if @result == :failure
      end
      
    end
  end
end

Warden::Strategies.add(:custom_authenticatable, Devise::Strategies::CustomAuthenticatable)

initializers/devise.rb

require 'custom_authenticatable'

Devise.setup do |config|
  config.warden do |manager|
     strategies = manager.default_strategies(:scope => :user)
     strategies.delete :two_factor_backupable
     strategies.delete :two_factor_authenticatable
     strategies.unshift :custom_authenticatable
  end
end

[igray]

For anyone reading this today, this is what I had to do to make this work:

module Devise
  module Strategies
    # This combines database_authenticatable with two_factor_authenticatable
    # and two_factor_backupable in such a way that does not generate more than
    # one lockable failed-attempt per login attempt.
    class CustomAuthenticatable < Devise::Strategies::DatabaseAuthenticatable
      # Warden authentication Strategy method
      # @see Warden::Strategies::Base#authenticate!
      # @see Devise::Strategies::TwoFactorAuthenticatable
      # @see Devise::Strategies::TwoFactorBackupable
      def authenticate!
        resource = mapping.to.find_for_database_authentication(authentication_hash)

        validation_result = validate(resource) do
          validate_otp(resource) ||
            resource.invalidate_otp_backup_code!(params[scope]['otp_attempt'])
        end
        if validation_result
          # Devise fails to authenticate invalidated resources, but if we've
          # gotten here, the object changed (Since we potentially deleted a
          # recovery code)
          resource.save!
          super
        end

        fail(Devise.paranoid ? :invalid : :not_found_in_database) unless resource
      end

      # Validate provided OTP Attempt if this user has OTP enabled
      # @params resource [User]
      # @see Devise::Strategies::TwoFactorAuthenticatable#validate_otp
      def validate_otp(resource)
        return true unless resource.otp_required_for_login
        return if params[scope].nil? || params[scope]['otp_attempt'].nil?

        resource.validate_and_consume_otp!(params[scope]['otp_attempt'])
      end
    end
  end
end

Warden::Strategies.add(:custom_authenticatable, Devise::Strategies::CustomAuthenticatable)

initializers/devise.rb

require 'custom_authenticatable'

Devise.setup do |config|
  config.warden do |manager|
     strategies = manager.default_strategies(:scope => :user)
     strategies.delete :two_factor_backupable
     strategies.delete :two_factor_authenticatable
     strategies.unshift :custom_authenticatable
  end
end

The main difference is that this custom authenticator performs everything that TwoFactorAuthenticatable and TwoFactorBackupable does without inheriting TwoFactorAuthenticatable. The reason that inheriting TwoFactorAuthenticatable does not work is because calling super inside the validate(resource) block calls validate_otp a second time. validate_otp calls validate_and_consume_otp! and validate_and_consume_otp! "consumes" the given otp_attempt and therefore will return true the first time and false the second time.

Since this inherits from DatabaseAuthenticatable, the super in this case is validating the password only.

[williantenfen]

module Devise
  OTP_AUTH_PARAM_NAME = "otp_attempt".freeze

  module Strategies
    class CustomDatabaseAuthenticatable < Devise::Strategies::DatabaseAuthenticatable
      def authenticate!
        resource = mapping.to.find_for_database_authentication(authentication_hash)
        # We authenticate in two cases:
        # 1. The password and the OTP or backup-code are correct
        # 2. The password is correct, and OTP is not required for login
        # We check the OTP, then defer to DatabaseAuthenticatable (super)
        super if validate(resource) { validate_otp(resource) || validate_backup(resource) }
        
        # user not found (doesnt exits on db)
        fail(Devise.paranoid ? :invalid : :not_found_in_database) unless resource 

       # super has been called and set the result
        fail(error_code) if @result == :failure 
      end

      def validate_otp(resource)
        return true unless resource.otp_required_for_login
        return if params[scope].nil? || params[scope][Devise::OTP_AUTH_PARAM_NAME].nil?

        otp_result = resource.validate_and_consume_otp!(params[scope][Devise::OTP_AUTH_PARAM_NAME])
        @otp_failed = !otp_result
        otp_result
      end

      def validate_backup(resource)
        return if params[scope].nil? || params[scope][Devise::OTP_AUTH_PARAM_NAME].nil?

        result = resource.invalidate_otp_backup_code!(params[scope][Devise::OTP_AUTH_PARAM_NAME])
        resource.save! if result
        @otp_failed = !result
        result
      end

      def error_code
        if @otp_failed
          :invalid_otp
        else
          Devise.paranoid ? :invalid : :not_found_in_database
        end
      end
    end
  end
end

Warden::Strategies.add(:custom_database_authenticatable, Devise::Strategies::CustomDatabaseAuthenticatable)