Merge pull request #3 from civisanalytics/prevent_reuse_of_otp

Prevent reuse of otp

demo/db/migrate/20170516191259_add_lastotpat_to_users.rb

@@ -0,0 +1,5 @@
+class AddLastotpatToUsers < ActiveRecord::Migration
+  def change
+    add_column :users, :last_otp_at, :integer
+  end
+end

demo/db/schema.rb

@@ -34,6 +34,7 @@
     t.string   "encrypted_otp_secret_salt"
     t.integer  "consumed_timestep"
     t.boolean  "otp_required_for_login"
+    t.integer  "last_otp_at"
   end
 
   add_index "users", ["email"], name: "index_users_on_email", unique: true, using: :btree

devise-two-factor.gemspec

@@ -28,12 +28,13 @@ Gem::Specification.new do |s|
   s.add_runtime_dependency 'activesupport'
   s.add_runtime_dependency 'attr_encrypted', '>= 1.3', '< 4', '!= 2'
   s.add_runtime_dependency 'devise',         '~> 4.0'
-  s.add_runtime_dependency 'rotp',           '~> 2.0'
+  s.add_runtime_dependency 'rotp',           '~> 3.0'
 
   s.add_development_dependency 'activemodel'
   s.add_development_dependency 'bundler',    '> 1.0'
   s.add_development_dependency 'rspec',      '> 3'
   s.add_development_dependency 'simplecov'
   s.add_development_dependency 'faker'
   s.add_development_dependency 'timecop'
+  s.add_development_dependency 'byebug'
 end

lib/devise_two_factor/models/two_factor_authenticatable.rb

@@ -22,7 +22,7 @@ module TwoFactorAuthenticatable
       end
 
       def self.required_fields(klass)
-        [:encrypted_otp_secret, :encrypted_otp_secret_iv, :encrypted_otp_secret_salt, :consumed_timestep]
+        [:encrypted_otp_secret, :encrypted_otp_secret_iv, :encrypted_otp_secret_salt, :consumed_timestep, :last_otp_at]
       end
 
       # This defaults to the model's otp_secret
@@ -32,7 +32,11 @@ def validate_and_consume_otp!(code, options = {})
         return false unless code.present? && otp_secret.present?
 
         totp = self.otp(otp_secret)
-        return consume_otp! if totp.verify_with_drift(code, self.class.otp_allowed_drift)
+        verified_at_timestamp = totp.verify_with_drift_and_prior(code, self.class.otp_allowed_drift, self.last_otp_at)
+        if verified_at_timestamp
+          self.last_otp_at = verified_at_timestamp
+          return consume_otp!
+        end
 
         false
       end

lib/devise_two_factor/spec_helpers/two_factor_authenticatable_shared_examples.rb

@@ -6,7 +6,7 @@
 
   describe 'required_fields' do
     it 'should have the attr_encrypted fields for otp_secret' do
-      expect(Devise::Models::TwoFactorAuthenticatable.required_fields(subject.class)).to contain_exactly(:encrypted_otp_secret, :encrypted_otp_secret_iv, :encrypted_otp_secret_salt, :consumed_timestep)
+      expect(Devise::Models::TwoFactorAuthenticatable.required_fields(subject.class)).to contain_exactly(:encrypted_otp_secret, :encrypted_otp_secret_iv, :encrypted_otp_secret_salt, :consumed_timestep, :last_otp_at)
     end
   end
 
@@ -102,8 +102,27 @@
     end
 
     it 'should return uri with issuer option' do
-      expect(subject.otp_provisioning_uri(account, issuer: issuer)).to match(%r{otpauth://totp/#{account}\?.*secret=\w{#{otp_secret_length}}(&|$)})
-      expect(subject.otp_provisioning_uri(account, issuer: issuer)).to match(%r{otpauth://totp/#{account}\?.*issuer=#{issuer}(&|$)})
+      expect(subject.otp_provisioning_uri(account, issuer: issuer)).to match(%r{otpauth://totp/#{issuer}:#{account}\?.*secret=\w{#{otp_secret_length}}(&|$)})
+      expect(subject.otp_provisioning_uri(account, issuer: issuer)).to match(%r{otpauth://totp/#{issuer}:#{account}\?.*issuer=#{issuer}(&|$)})
+    end
+  end
+
+  context 'given a valid OTP used multiple times within the allowed drift' do
+    let(:consumed_otp) { ROTP::TOTP.new(subject.otp_secret).at(Time.now) }
+
+    before do
+      subject.validate_and_consume_otp!(consumed_otp)
+    end
+
+    context 'after the otp interval' do
+      before do
+        Timecop.travel(subject.otp.interval.seconds.from_now)
+      end
+
+      # This spec tests that reuse of the OTP is not allowed
+      it 'fails to validate' do
+        expect(subject.validate_and_consume_otp!(consumed_otp)).to be false
+      end
     end
   end
 end

lib/generators/devise_two_factor/devise_two_factor_generator.rb

@@ -23,6 +23,7 @@ def create_devise_two_factor_migration
                                 "encrypted_otp_secret_iv:string",
                                 "encrypted_otp_secret_salt:string",
                                 "consumed_timestep:integer",
+                                "last_otp_at:integer",
                                 "otp_required_for_login:boolean"
                               ]
 

spec/devise/models/two_factor_authenticatable_spec.rb

@@ -11,6 +11,7 @@ class TwoFactorAuthenticatableDouble
   devise :two_factor_authenticatable, :otp_secret_encryption_key => 'test-key'*4
 
   attr_accessor :consumed_timestep
+  attr_accessor :last_otp_at
 
   def save(validate)
     # noop for testing
@@ -36,6 +37,7 @@ class TwoFactorAuthenticatableWithCustomizeAttrEncryptedDouble
   devise :two_factor_authenticatable, :otp_secret_encryption_key => 'test-key'*4
 
   attr_accessor :consumed_timestep
+  attr_accessor :last_otp_at
 
   def save(validate)
     # noop for testing