From da2fe1cbbcfac767c3033670ea6cce3497241c76 Mon Sep 17 00:00:00 2001 From: Matthew Ingle Date: Wed, 5 Apr 2017 00:13:32 -0400 Subject: [PATCH 1/3] Added database level check for expiration of cookie --- .../devise/two_factor_authentication_controller.rb | 1 + lib/generators/active_record/templates/migration.rb | 1 + .../hooks/two_factor_authenticatable.rb | 2 +- lib/two_factor_authentication/schema.rb | 4 ++++ spec/rails_app/app/models/encrypted_user.rb | 3 ++- 5 files changed, 9 insertions(+), 2 deletions(-) diff --git a/app/controllers/devise/two_factor_authentication_controller.rb b/app/controllers/devise/two_factor_authentication_controller.rb index 2cd8d6fa..0109c4d4 100644 --- a/app/controllers/devise/two_factor_authentication_controller.rb +++ b/app/controllers/devise/two_factor_authentication_controller.rb @@ -45,6 +45,7 @@ def set_remember_two_factor_cookie(resource) expires_seconds = resource.class.remember_otp_session_for_seconds if expires_seconds && expires_seconds > 0 + resource.update_attribute(:expire_token, expires_seconds.from_now) cookies.signed[TwoFactorAuthentication::REMEMBER_TFA_COOKIE_NAME] = { value: "#{resource.class}-#{resource.public_send(Devise.second_factor_resource_id)}", expires: expires_seconds.from_now diff --git a/lib/generators/active_record/templates/migration.rb b/lib/generators/active_record/templates/migration.rb index 251ef402..ab2f50c2 100644 --- a/lib/generators/active_record/templates/migration.rb +++ b/lib/generators/active_record/templates/migration.rb @@ -7,6 +7,7 @@ def change add_column :<%= table_name %>, :direct_otp, :string add_column :<%= table_name %>, :direct_otp_sent_at, :datetime add_column :<%= table_name %>, :totp_timestamp, :timestamp + add_column :<%= table_name %>, :cookie_expire, :timestamp add_index :<%= table_name %>, :encrypted_otp_secret_key, unique: true end diff --git a/lib/two_factor_authentication/hooks/two_factor_authenticatable.rb b/lib/two_factor_authentication/hooks/two_factor_authenticatable.rb index 159ae144..f3f1f9ac 100644 --- a/lib/two_factor_authentication/hooks/two_factor_authenticatable.rb +++ b/lib/two_factor_authentication/hooks/two_factor_authenticatable.rb @@ -2,7 +2,7 @@ if auth.env["action_dispatch.cookies"] expected_cookie_value = "#{user.class}-#{user.public_send(Devise.second_factor_resource_id)}" actual_cookie_value = auth.env["action_dispatch.cookies"].signed[TwoFactorAuthentication::REMEMBER_TFA_COOKIE_NAME] - bypass_by_cookie = actual_cookie_value == expected_cookie_value + bypass_by_cookie = (actual_cookie_value == expected_cookie_value) && user.expire_token > Time.now end if user.respond_to?(:need_two_factor_authentication?) && !bypass_by_cookie diff --git a/lib/two_factor_authentication/schema.rb b/lib/two_factor_authentication/schema.rb index 9f938b7c..f4b30467 100644 --- a/lib/two_factor_authentication/schema.rb +++ b/lib/two_factor_authentication/schema.rb @@ -27,5 +27,9 @@ def direct_otp_sent_at def totp_timestamp apply_devise_schema :totp_timestamp, Timestamp end + + def cookie_expire + apply_devise_schema :cookie_expire, Timestamp + end end end diff --git a/spec/rails_app/app/models/encrypted_user.rb b/spec/rails_app/app/models/encrypted_user.rb index 292004a3..7a155799 100644 --- a/spec/rails_app/app/models/encrypted_user.rb +++ b/spec/rails_app/app/models/encrypted_user.rb @@ -9,7 +9,8 @@ class EncryptedUser :encrypted_otp_secret_key_salt, :email, :second_factor_attempts_count, - :totp_timestamp + :totp_timestamp, + :cookie_expire has_one_time_password(encrypted: true) end From 6707579efce504d8a5f05ac11489f555e4a00166 Mon Sep 17 00:00:00 2001 From: Matthew Ingle Date: Wed, 5 Apr 2017 00:37:49 -0400 Subject: [PATCH 2/3] Added test --- app/controllers/devise/two_factor_authentication_controller.rb | 2 +- .../hooks/two_factor_authenticatable.rb | 3 ++- .../models/two_factor_authenticatable.rb | 2 +- spec/rails_app/app/models/guest_user.rb | 2 +- spec/support/authenticated_model_helper.rb | 1 + 5 files changed, 6 insertions(+), 4 deletions(-) diff --git a/app/controllers/devise/two_factor_authentication_controller.rb b/app/controllers/devise/two_factor_authentication_controller.rb index 0109c4d4..decb93fc 100644 --- a/app/controllers/devise/two_factor_authentication_controller.rb +++ b/app/controllers/devise/two_factor_authentication_controller.rb @@ -45,7 +45,7 @@ def set_remember_two_factor_cookie(resource) expires_seconds = resource.class.remember_otp_session_for_seconds if expires_seconds && expires_seconds > 0 - resource.update_attribute(:expire_token, expires_seconds.from_now) + resource.update_attribute(:cookie_expire, expires_seconds.from_now) cookies.signed[TwoFactorAuthentication::REMEMBER_TFA_COOKIE_NAME] = { value: "#{resource.class}-#{resource.public_send(Devise.second_factor_resource_id)}", expires: expires_seconds.from_now diff --git a/lib/two_factor_authentication/hooks/two_factor_authenticatable.rb b/lib/two_factor_authentication/hooks/two_factor_authenticatable.rb index f3f1f9ac..d2055114 100644 --- a/lib/two_factor_authentication/hooks/two_factor_authenticatable.rb +++ b/lib/two_factor_authentication/hooks/two_factor_authenticatable.rb @@ -2,9 +2,10 @@ if auth.env["action_dispatch.cookies"] expected_cookie_value = "#{user.class}-#{user.public_send(Devise.second_factor_resource_id)}" actual_cookie_value = auth.env["action_dispatch.cookies"].signed[TwoFactorAuthentication::REMEMBER_TFA_COOKIE_NAME] - bypass_by_cookie = (actual_cookie_value == expected_cookie_value) && user.expire_token > Time.now + bypass_by_cookie = (actual_cookie_value == expected_cookie_value) && (user.cookie_expire.blank? || (user.cookie_expire > Time.now)) end + if user.respond_to?(:need_two_factor_authentication?) && !bypass_by_cookie if auth.session(options[:scope])[TwoFactorAuthentication::NEED_AUTHENTICATION] = user.need_two_factor_authentication?(auth.request) user.send_new_otp unless user.totp_enabled? diff --git a/lib/two_factor_authentication/models/two_factor_authenticatable.rb b/lib/two_factor_authentication/models/two_factor_authenticatable.rb index 376038e2..5b918d91 100644 --- a/lib/two_factor_authentication/models/two_factor_authenticatable.rb +++ b/lib/two_factor_authentication/models/two_factor_authenticatable.rb @@ -16,7 +16,7 @@ def has_one_time_password(options = {}) ::Devise::Models.config( self, :max_login_attempts, :allowed_otp_drift_seconds, :otp_length, :remember_otp_session_for_seconds, :otp_secret_encryption_key, - :direct_otp_length, :direct_otp_valid_for, :totp_timestamp) + :direct_otp_length, :direct_otp_valid_for, :totp_timestamp, :cookie_expire) end module InstanceMethodsOnActivation diff --git a/spec/rails_app/app/models/guest_user.rb b/spec/rails_app/app/models/guest_user.rb index 8003624c..646494bb 100644 --- a/spec/rails_app/app/models/guest_user.rb +++ b/spec/rails_app/app/models/guest_user.rb @@ -5,7 +5,7 @@ class GuestUser define_model_callbacks :create attr_accessor :direct_otp, :direct_otp_sent_at, :otp_secret_key, :email, - :second_factor_attempts_count, :totp_timestamp + :second_factor_attempts_count, :totp_timestamp,:cookie_expire def update_attributes(attrs) attrs.each do |key, value| diff --git a/spec/support/authenticated_model_helper.rb b/spec/support/authenticated_model_helper.rb index 42696e68..5b9956bb 100644 --- a/spec/support/authenticated_model_helper.rb +++ b/spec/support/authenticated_model_helper.rb @@ -50,6 +50,7 @@ def create_table_for_nonencrypted_user t.string 'direct_otp' t.datetime 'direct_otp_sent_at' t.timestamp 'totp_timestamp' + t.timestamp 'cookie_expire' end end end From 4add3c014d0fc6bd5456568660ddbd73e9048489 Mon Sep 17 00:00:00 2001 From: Matthew Ingle Date: Thu, 13 Apr 2017 12:42:51 -0400 Subject: [PATCH 3/3] fix --- .../hooks/two_factor_authenticatable.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/two_factor_authentication/hooks/two_factor_authenticatable.rb b/lib/two_factor_authentication/hooks/two_factor_authenticatable.rb index d2055114..4a132c1d 100644 --- a/lib/two_factor_authentication/hooks/two_factor_authenticatable.rb +++ b/lib/two_factor_authentication/hooks/two_factor_authenticatable.rb @@ -2,7 +2,7 @@ if auth.env["action_dispatch.cookies"] expected_cookie_value = "#{user.class}-#{user.public_send(Devise.second_factor_resource_id)}" actual_cookie_value = auth.env["action_dispatch.cookies"].signed[TwoFactorAuthentication::REMEMBER_TFA_COOKIE_NAME] - bypass_by_cookie = (actual_cookie_value == expected_cookie_value) && (user.cookie_expire.blank? || (user.cookie_expire > Time.now)) + bypass_by_cookie = (actual_cookie_value == expected_cookie_value) && !user.cookie_expire.blank? && (Time.now < user.cookie_expire) end