Fix acceptance of invalid otp attempts

lib/devise_two_factor/strategies/two_factor_authenticatable.rb

@@ -3,17 +3,17 @@ module Strategies
     class TwoFactorAuthenticatable < Devise::Strategies::DatabaseAuthenticatable
 
       def authenticate!
-        resource = mapping.to.find_for_database_authentication(authentication_hash)
+        resource = find_resource_from_mapping_and_auth_hash
         # We authenticate in two cases:
         # 1. The password and the OTP are correct
         # 2. The password is correct, and OTP is not required for login
         # We check the OTP, then defer to DatabaseAuthenticatable
         if validate(resource) { validate_otp(resource) }
           super
+        else
+          fail(Devise.paranoid ? :invalid : :not_found_in_database)
         end
 
-        fail(Devise.paranoid ? :invalid : :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
@@ -24,6 +24,10 @@ def validate_otp(resource)
         return if params[scope]['otp_attempt'].nil?
         resource.validate_and_consume_otp!(params[scope]['otp_attempt'])
       end
+
+      def find_resource_from_mapping_and_auth_hash
+        mapping.to.find_for_database_authentication(authentication_hash)
+      end
     end
   end
 end

spec/devise/models/two_factor_authenticatable_spec.rb

@@ -1,47 +1,5 @@
 require 'spec_helper'
-require 'active_model'
-
-class TwoFactorAuthenticatableDouble
-  extend ::ActiveModel::Callbacks
-  include ::ActiveModel::Validations::Callbacks
-  extend  ::Devise::Models
-
-  define_model_callbacks :update
-
-  devise :two_factor_authenticatable, :otp_secret_encryption_key => 'test-key'*4
-
-  attr_accessor :consumed_timestep
-
-  def save(validate)
-    # noop for testing
-    true
-  end
-end
-
-class TwoFactorAuthenticatableWithCustomizeAttrEncryptedDouble
-  extend ::ActiveModel::Callbacks
-  include ::ActiveModel::Validations::Callbacks
-
-  # like https://github.com/tinfoil/devise-two-factor/blob/cf73e52043fbe45b74d68d02bc859522ad22fe73/UPGRADING.md#guide-to-upgrading-from-2x-to-3x
-  extend ::AttrEncrypted
-  attr_encrypted :otp_secret,
-                  :key       => 'test-key'*8,
-                  :mode      => :per_attribute_iv_and_salt,
-                  :algorithm => 'aes-256-cbc'
-
-  extend  ::Devise::Models
-
-  define_model_callbacks :update
-
-  devise :two_factor_authenticatable, :otp_secret_encryption_key => 'test-key'*4
-
-  attr_accessor :consumed_timestep
-
-  def save(validate)
-    # noop for testing
-    true
-  end
-end
+require 'support/two_factor_authenticatable_doubles'
 
 describe ::Devise::Models::TwoFactorAuthenticatable do
   context 'When included in a class' do

spec/devise/strategies/two_factor_authenticatable_spec.rb

@@ -0,0 +1,90 @@
+require 'spec_helper'
+require 'support/two_factor_authenticatable_doubles'
+
+describe ::Devise::Strategies::TwoFactorAuthenticatable do
+  let(:scope) { "foo" }
+  let(:strategy) { described_class.new({}, scope) }
+  let(:params) do
+    { scope => { "otp_attempt" => otp_attempt } }
+  end
+
+  let(:resource) do
+    res = TwoFactorAuthenticatableWithCustomizeAttrEncryptedDouble.new
+    res.otp_secret = res.class.generate_otp_secret
+    res.consumed_timestep = nil
+
+    res
+  end
+
+  describe "#validate_otp" do
+    let(:params) do
+      { scope => { "otp_attempt" => otp_attempt } }
+    end
+
+    before(:each) do
+      allow(strategy).to receive(:params) { params }
+      allow(resource).to receive(:otp_required_for_login) { otp_required_for_login }
+    end
+
+    subject { strategy.validate_otp(resource) }
+
+    context "when otp is required" do
+      let(:otp_required_for_login) { true }
+
+      context "and params do include an otp_attempt" do
+        context "that is valid" do
+          let(:otp_attempt) { resource.current_otp }
+          it { is_expected.to be(true) }
+        end
+
+        context "that is invalid" do
+          let(:otp_attempt) { Faker::Number.number(digits: 6).to_s }
+          it { is_expected.to be(false) }
+        end
+      end
+
+      context "and params do not include an otp_attempt" do
+        let(:otp_attempt) { nil }
+        it { is_expected.to be_nil }
+      end
+    end
+
+    context "when otp is not required" do
+      let(:otp_required_for_login) { false }
+
+      context "and params do include an otp_attempt" do
+        let(:otp_attempt) { Faker::Number.number(digits: 6).to_s }
+        it { is_expected.to be(true) }
+      end
+
+      context "and params do not include an otp_attempt" do
+        let(:otp_attempt) { nil }
+        it { is_expected.to be(true) }
+      end
+    end
+  end
+
+  describe "#authenticate!" do
+    subject { strategy.authenticate! }
+
+    before(:each) do
+      allow(strategy).to receive(:find_resource_from_mapping_and_auth_hash).and_return(resource)
+      allow(strategy).to receive(:params) { params }
+      allow(resource).to receive(:otp_required_for_login) { otp_required_for_login }
+    end
+
+    context "when resource.otp_required_for_login is true" do
+      let(:otp_required_for_login) { true }
+
+      context "and params do not include an otp_attempt" do
+        let(:otp_attempt) { nil }
+        it { is_expected.not_to eq(:success) }
+      end
+
+      context "and params include an invalid otp_attempt" do
+        let(:otp_attempt) { Faker::Number.number(digits: 10).to_s }
+        it { is_expected.not_to eq(:success) }
+      end
+    end
+  end
+end

spec/support/two_factor_authenticatable_doubles.rb

@@ -0,0 +1,43 @@
+require 'active_model'
+
+class TwoFactorAuthenticatableDouble
+  extend ::ActiveModel::Callbacks
+  include ::ActiveModel::Validations::Callbacks
+  extend  ::Devise::Models
+
+  define_model_callbacks :update
+
+  devise :two_factor_authenticatable, :otp_secret_encryption_key => 'test-key'*4
+
+  attr_accessor :consumed_timestep
+
+  def save(validate)
+    # noop for testing
+    true
+  end
+end
+
+class TwoFactorAuthenticatableWithCustomizeAttrEncryptedDouble
+  extend ::ActiveModel::Callbacks
+  include ::ActiveModel::Validations::Callbacks
+
+  # like https://github.com/tinfoil/devise-two-factor/blob/cf73e52043fbe45b74d68d02bc859522ad22fe73/UPGRADING.md#guide-to-upgrading-from-2x-to-3x
+  extend ::AttrEncrypted
+  attr_encrypted :otp_secret,
+                  :key       => 'test-key'*8,
+                  :mode      => :per_attribute_iv_and_salt,
+                  :algorithm => 'aes-256-cbc'
+
+  extend  ::Devise::Models
+
+  define_model_callbacks :update
+
+  devise :two_factor_authenticatable, :otp_secret_encryption_key => 'test-key'*4
+
+  attr_accessor :consumed_timestep
+
+  def save(validate)
+    # noop for testing
+    true
+  end
+end