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

Skips extra validate when otp attempt looks like its coming from Auth… #130

Closed
wants to merge 1 commit into from

Conversation

jeremy04
Copy link

@jeremy04 jeremy04 commented May 8, 2018

#127

Assuming you have a Devise setup like this:

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

With a configuration like this:

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

failed_attempts gets called every time validate runs in the cascading strategies.

Assuming you enforce two_factor_backupable strategy first, you can reduce the failed_attempts to not increment twice in the following cases:

  • Normal login with invalid password
  • OTP enabled, invalid OTP entered less than 7 characters

Unfortunately, it will still increment twice when:

  • OTP enabled, invalid OTP entered 7 digits or more

#1)

* Skips extra validate when otp attempt looks like its coming from Authenticator vs. backup code

* added unit test for TwoFactorBackupableStrategy

* Made tests more descriptive
@jeremy04
Copy link
Author

I just realized my specific otp_backup_code_length is hard-coded in resembles_backup_code?, so this probably needs to be more generic, or approached a different way.

@bsedat Thoughts?

@SimonVillage
Copy link

can we have this fixed please?

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

Successfully merging this pull request may close these issues.

None yet

3 participants