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.

Sign up for GitHub

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

Jump to bottom

Fix acceptance of invalid otp attempts #173

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Fix acceptance of invalid otp attempts #173

wants to merge 2 commits into from

Commits on Aug 31, 2020

  1. Extract TwoFactorAuthenticatable doubles to support

    @kmwhite
    kmwhite committed Aug 31, 2020
    Configuration menu
    Copy the full SHA
    f9d0f72 View commit details
    Browse the repository at this point in the history

  2. Ensure Failure with Invalid OTP Attempts 

    In the event that an OTP attempt is invalid, the super method is never
called. If the authentication_hash did describe a record, it was not
rejecting the authentication. The find_for_database_authentication
method is inherited from Devise::Strategies::DatabaseAuthenticatable
which pays no attention to the otp_attempt, considering this pry
session:

    From: /usr/local/bundle/bundler/gems/devise-two-factor-07acb0ae5068/lib/devise_two_factor/strategies/two_factor_authenticatable.rb @ line 12 Devise::Strategies::TwoFactorAuthenticatable#authenticate!:

         1: module Devise
         2:   module Strategies
         3:     class TwoFactorAuthenticatable < Devise::Strategies::DatabaseAuthenticatable
         4:
         5:       def authenticate!
         6:         resource = mapping.to.find_for_database_authentication(authentication_hash)
         7:         # We authenticate in two cases:
         8:         # 1. The password and the OTP are correct
         9:         # 2. The password is correct, and OTP is not required for login
        10:         # We check the OTP, then defer to DatabaseAuthenticatable
        11:         require "pry"; binding.pry
     => 12:         if validate(resource) { validate_otp(resource) }
        13:           super
        14:         end
        15:
        16:         fail(Devise.paranoid ? :invalid : :not_found_in_database) unless resource
        17:
        18:         # We want to cascade to the next strategy if this one fails,
        19:         # but database authenticatable automatically halts on a bad password
        20:         @Halted = false if @Result == :failure
        21:       end
        22:
        23:       def validate_otp(resource)
        24:         return true unless resource.otp_required_for_login
        25:         return if params[scope]['otp_attempt'].nil?
        26:         resource.validate_and_consume_otp!(params[scope]['otp_attempt'])
        27:       end
        28:     end
        29:   end
        30: end
        31:
        32: Warden::Strategies.add(:two_factor_authenticatable, Devise::Strategies::TwoFactorAuthenticatable)

    [6] pry(#<Devise::Strategies::TwoFactorAuthenticatable>)> params[scope]
    => {"login"=>"miranda", "password"=>"password", "otp_attempt"=>"123456"}
    [7] pry(#<Devise::Strategies::TwoFactorAuthenticatable>)> validate(resource)
    => true
    [8] pry(#<Devise::Strategies::TwoFactorAuthenticatable>)> validate_otp(resource)
    => false
    [9] pry(#<Devise::Strategies::TwoFactorAuthenticatable>)> next

    From: /usr/local/bundle/bundler/gems/devise-two-factor-07acb0ae5068/lib/devise_two_factor/strategies/two_factor_authenticatable.rb @ line 16 Devise::Strategies::TwoFactorAuthenticatable#authenticate!:

         5: def authenticate!
         6:   resource = mapping.to.find_for_database_authentication(authentication_hash)
         7:   # We authenticate in two cases:
         8:   # 1. The password and the OTP are correct
         9:   # 2. The password is correct, and OTP is not required for login
        10:   # We check the OTP, then defer to DatabaseAuthenticatable
        11:   require "pry"; binding.pry
        12:   if validate(resource) { validate_otp(resource) }
        13:     super
        14:   end
        15:
     => 16:   fail(Devise.paranoid ? :invalid : :not_found_in_database) unless resource
        17:
        18:   # We want to cascade to the next strategy if this one fails,
        19:   # but database authenticatable automatically halts on a bad password
        20:   @Halted = false if @Result == :failure
        21: end

    [9] pry(#<Devise::Strategies::TwoFactorAuthenticatable>):1> resource.present?
    => true
    [10] pry(#<Devise::Strategies::TwoFactorAuthenticatable>):1> resource.login
    => "miranda"
    [11] pry(#<Devise::Strategies::TwoFactorAuthenticatable>):1>

While the `super` is not called to set up the session, but it does not
necessarily *fail* the login. This change ensures that fail() is called
when this case is hit.
    @kmwhite
    kmwhite committed Aug 31, 2020
    Configuration menu
    Copy the full SHA
    64c3d8a View commit details
    Browse the repository at this point in the history