Disallow subsequent OTPs once validated via timesteps

In order to be fully RFC 6238 compliant for TOTPs, no valid OTP may be used more than once for a given timestep. By storing the timestep of the last successfully validated OTP, we achieve this desired behaviour.

TOTP#timecode is a private method, so we had to duplicate it in order to obtain the current server timestep. Thankfully it's simply time divided by interval.

I was having issues with using `resource.save!` in the strategy class as a means of object persistence to the database. Ultimately, I opted to use the ActiveRecord `save()` method inside the devise model which is what Devise does in its own model definitions.

Burning the timestep bumps the `updated_at` value for the model. Not sure if this is a feature or bug.

Refs devise-two-factor#30

demo/db/migrate/20140516191259_add_devise_two_factor_to_users.rb

@@ -3,6 +3,7 @@ def change
     add_column :users, :encrypted_otp_secret, :string
     add_column :users, :encrypted_otp_secret_iv, :string
     add_column :users, :encrypted_otp_secret_salt, :string
+    add_column :users, :consumed_timestep, :integer
     add_column :users, :otp_required_for_login, :boolean
   end
 end

demo/db/schema.rb

@@ -32,6 +32,7 @@
     t.string   "encrypted_otp_secret"
     t.string   "encrypted_otp_secret_iv"
     t.string   "encrypted_otp_secret_salt"
+    t.integer  "consumed_timestep"
     t.boolean  "otp_required_for_login"
   end
 

lib/devise_two_factor/models/two_factor_authenticatable.rb

@@ -15,17 +15,27 @@ module TwoFactorAuthenticatable
       end
 
       def self.required_fields(klass)
-        [:encrypted_otp_secret, :encrypted_otp_secret_iv, :encrypted_otp_secret_salt]
+        [:encrypted_otp_secret, :encrypted_otp_secret_iv, :encrypted_otp_secret_salt, :consumed_timestep]
       end
 
       # This defaults to the model's otp_secret
-      # If this hasn't been generated yet, pass a secret as an  option
+      # If this hasn't been generated yet, pass a secret as an option
       def valid_otp?(code, options = {})
         otp_secret = options[:otp_secret] || self.otp_secret
         return false unless otp_secret.present?
 
         totp = self.otp(otp_secret)
-        totp.verify_with_drift(code, self.class.otp_allowed_drift)
+        if totp.verify_with_drift(code, self.class.otp_allowed_drift)
+          # An OTP cannot be used more than once in a given timestep
+          # Storing timestep of last valid OTP is sufficient to satisfy this requirement
+          if self.consumed_timestep != current_otp_timestep
+            self.consumed_timestep = current_otp_timestep
+            save(validate: false)
+            return true
+          end
+        end
+
+        false
       end
 
       def otp(otp_secret = self.otp_secret)
@@ -36,6 +46,11 @@ def current_otp
         otp.at(Time.now)
       end
 
+      # ROTP's TOTP#timecode is private, so we duplicate it here
+      def current_otp_timestep
+         Time.now.utc.to_i / otp.interval
+      end
+
       def otp_provisioning_uri(account, options = {})
         otp_secret = options[:otp_secret] || self.otp_secret
         ROTP::TOTP.new(otp_secret, options).provisioning_uri(account)

lib/devise_two_factor/spec_helpers/two_factor_authenticatable_shared_examples.rb

@@ -5,7 +5,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)
+      expect(Devise::Models::TwoFactorAuthenticatable.required_fields(subject.class)).to contain_exactly(:encrypted_otp_secret, :encrypted_otp_secret_iv, :encrypted_otp_secret_salt, :consumed_timestep)
     end
   end
 
@@ -44,11 +44,25 @@
       expect(subject.valid_otp?(otp)).to be true
     end
 
+    it 'does not validate a previously precisely correct OTP' do
+      otp = ROTP::TOTP.new(otp_secret).at(Time.now)
+      expect(subject.valid_otp?(otp)).to be true
+      expect(subject.valid_otp?(otp)).to be false
+      expect(subject.valid_otp?(otp)).to be false
+    end
+
     it 'validates an OTP within the allowed drift' do
       otp = ROTP::TOTP.new(otp_secret).at(Time.now + subject.class.otp_allowed_drift, true)
       expect(subject.valid_otp?(otp)).to be true
     end
 
+    it 'does not validate a previously valid OTP within the allowed drift' do
+      otp = ROTP::TOTP.new(otp_secret).at(Time.now + subject.class.otp_allowed_drift, true)
+      expect(subject.valid_otp?(otp)).to be true
+      expect(subject.valid_otp?(otp)).to be false
+      expect(subject.valid_otp?(otp)).to be false
+    end
+
     it 'does not validate an OTP above the allowed drift' do
       otp = ROTP::TOTP.new(otp_secret).at(Time.now + subject.class.otp_allowed_drift * 2, true)
       expect(subject.valid_otp?(otp)).to be false

lib/generators/devise_two_factor/devise_two_factor_generator.rb

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

spec/devise/models/two_factor_authenticatable_spec.rb

@@ -6,6 +6,12 @@ class TwoFactorAuthenticatableDouble
   extend  ::Devise::Models
 
   devise :two_factor_authenticatable, :otp_secret_encryption_key => 'test-key'
+
+  attr_accessor :consumed_timestep
+
+  def save(validate)
+    # noop
+  end
 end
 
 describe ::Devise::Models::TwoFactorAuthenticatable do