Permalink
Browse files

Merge pull request #43 from f3ndot/issue-30-consume-valid-otps

Disallow subsequent OTPs once validated via timesteps
  • Loading branch information...
bsedat committed Sep 15, 2015
2 parents 234485d + 5f6c2f1 commit cb025fbd7fd257a057dd82de50aead7fbc987e8f
@@ -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
@@ -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
@@ -15,17 +15,19 @@ 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
def valid_otp?(code, options = {})
# If this hasn't been generated yet, pass a secret as an option
def validate_and_consume_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)
return consume_otp! if totp.verify_with_drift(code, self.class.otp_allowed_drift)
false
end
def otp(otp_secret = self.otp_secret)
@@ -36,6 +38,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)
@@ -47,6 +54,17 @@ def clean_up_passwords
protected
# An OTP cannot be used more than once in a given timestep
# Storing timestep of last valid OTP is sufficient to satisfy this requirement
def consume_otp!
if self.consumed_timestep != current_otp_timestep
self.consumed_timestep = current_otp_timestep
return save(validate: false)
end
false
end
module ClassMethods
Devise::Models.config(self, :otp_secret_length,
:otp_allowed_drift,
@@ -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
@@ -27,7 +27,7 @@
end
end
describe '#valid_otp?' do
describe '#validate_and_consume_otp!' do
let(:otp_secret) { '2z6hxkdwi3uvrnpn' }
before :each do
@@ -39,24 +39,50 @@
Timecop.return
end
context 'with a stored consumed_timestep' do
context 'given a precisely correct OTP' do
let(:consumed_otp) { ROTP::TOTP.new(otp_secret).at(Time.now) }
before do
subject.validate_and_consume_otp!(consumed_otp)
end
it 'fails to validate' do
expect(subject.validate_and_consume_otp!(consumed_otp)).to be false
end
end
context 'given a previously valid OTP within the allowed drift' do
let(:consumed_otp) { ROTP::TOTP.new(otp_secret).at(Time.now + subject.class.otp_allowed_drift, true) }
before do
subject.validate_and_consume_otp!(consumed_otp)
end
it 'fails to validate' do
expect(subject.validate_and_consume_otp!(consumed_otp)).to be false
end
end
end
it 'validates a precisely correct OTP' do
otp = ROTP::TOTP.new(otp_secret).at(Time.now)
expect(subject.valid_otp?(otp)).to be true
expect(subject.validate_and_consume_otp!(otp)).to be true
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
expect(subject.validate_and_consume_otp!(otp)).to be true
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
expect(subject.validate_and_consume_otp!(otp)).to be false
end
it 'does not validate an OTP below 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
expect(subject.validate_and_consume_otp!(otp)).to be false
end
end
@@ -22,7 +22,7 @@ def authenticate!
def validate_otp(resource)
return true unless resource.otp_required_for_login
return if params[scope]['otp_attempt'].nil?
resource.valid_otp?(params[scope]['otp_attempt'])
resource.validate_and_consume_otp!(params[scope]['otp_attempt'])
end
end
end
@@ -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"
]
@@ -6,6 +6,13 @@ class TwoFactorAuthenticatableDouble
extend ::Devise::Models
devise :two_factor_authenticatable, :otp_secret_encryption_key => 'test-key'
attr_accessor :consumed_timestep
def save(validate)
# noop for testing
true
end
end
describe ::Devise::Models::TwoFactorAuthenticatable do

0 comments on commit cb025fb

Please sign in to comment.