Fix double failed attempts and add error code for invalid otp

lib/devise_two_factor/strategies/two_factor_authenticatable.rb

@@ -3,25 +3,33 @@ module Strategies
     class TwoFactorAuthenticatable < 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 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
+        resource  = password.present? && mapping.to.find_for_database_authentication(authentication_hash)
+        hashed = false
+
+        unless resource && validate(resource){ hashed = true; resource.valid_password?(password) }
+          mapping.to.new.password = password if !hashed && Devise.paranoid
+          return fail!(:not_found_in_database)
         end
 
-        fail(:not_found_in_database) unless resource
+        if resource.otp_required_for_login
+          return if params[scope].key?('otp_backup')
+
+          unless validate(resource){ valid_otp?(resource) }
+            return fail!(:invalid_otp)
+          end
+        end
 
-        # 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
+        remember_me(resource)
+        resource.after_database_authentication
+        success!(resource)
+
+        mapping.to.new.password = password if !hashed && Devise.paranoid
       end
 
-      def validate_otp(resource)
-        return true unless resource.otp_required_for_login
-        return if params[scope]['otp_attempt'].nil?
+      private
+
+      def valid_otp?(resource)
+        return false if params[scope]['otp_attempt'].nil?
         resource.validate_and_consume_otp!(params[scope]['otp_attempt'])
       end
     end

lib/devise_two_factor/strategies/two_factor_backupable.rb

@@ -3,20 +3,35 @@ module Strategies
     class TwoFactorBackupable < Devise::Strategies::DatabaseAuthenticatable
 
       def authenticate!
-        resource = mapping.to.find_for_database_authentication(authentication_hash)
+        resource  = password.present? && mapping.to.find_for_database_authentication(authentication_hash)
+        hashed = false
 
-        if validate(resource) { resource.invalidate_otp_backup_code!(params[scope]['otp_attempt']) }
-          # Devise fails to authenticate invalidated resources, but if we've
-          # gotten here, the object changed (Since we deleted a recovery code)
-          resource.save!
-          super
+        unless resource && validate(resource){ hashed = true; resource.valid_password?(password) }
+          mapping.to.new.password = password if !hashed && Devise.paranoid
+          return fail!(:not_found_in_database)
         end
 
-        fail(:not_found_in_database) unless resource
+        if resource.otp_required_for_login
+          return if params[scope].key?('otp_attempt')
 
-        # 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
+          unless validate(resource){ valid_otp_backup?(resource) }
+            return fail!(:invalid_otp)
+          end
+        end
+
+        resource.save!
+        remember_me(resource)
+        resource.after_database_authentication
+        success!(resource)
+
+        mapping.to.new.password = password if !hashed && Devise.paranoid
+      end
+
+      private
+
+      def valid_otp_backup?(resource)
+        return false if params[scope]['otp_backup'].nil?
+        resource.invalidate_otp_backup_code!(params[scope]['otp_backup'])
       end
     end
   end

spec/strateries/two_factor_authenticatable_spec.rb

@@ -0,0 +1,46 @@
+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
+
+describe Devise::Strategies::TwoFactorAuthenticatable do
+  let(:resource) { TwoFactorAuthenticatableDouble.new }
+  let(:strategy){ Devise::Strategies::TwoFactorAuthenticatable.new(resource) }
+
+  before { strategy.authenticate! }
+
+  describe '#authenticate!' do
+    context 'when resource does not exist' do
+      it '@result be failure' do
+        expect(strategy.result).to eq :failure
+      end
+
+      it 'return not_found_in_database message' do
+        expect(strategy.message).to eq :not_found_in_database
+      end
+
+      it '@halted be true' do
+        expect(strategy.halted?).to eq true
+      end
+    end
+
+    context 'when resource exists' do
+
+    end
+  end
+end