From 5c54a213a2d51ebf177c5ff4214e3a5d84c1e714 Mon Sep 17 00:00:00 2001 From: Dachi Natsvlishvili Date: Thu, 15 Nov 2018 23:53:11 +0400 Subject: [PATCH 1/8] Save scope in cookie name. Allow scoped subdomains --- .../devise/two_factor_authentication_controller.rb | 4 ++-- lib/two_factor_authentication.rb | 4 ++++ lib/two_factor_authentication/controllers/helpers.rb | 11 ++++++++--- .../hooks/two_factor_authenticatable.rb | 11 ++++++++--- .../two_factor_authentication_controller_spec.rb | 6 +++--- 5 files changed, 25 insertions(+), 11 deletions(-) diff --git a/app/controllers/devise/two_factor_authentication_controller.rb b/app/controllers/devise/two_factor_authentication_controller.rb index 0e7aed84..3b756072 100644 --- a/app/controllers/devise/two_factor_authentication_controller.rb +++ b/app/controllers/devise/two_factor_authentication_controller.rb @@ -27,7 +27,7 @@ def resend_code def after_two_factor_success_for(resource) set_remember_two_factor_cookie(resource) - warden.session(resource_name)[TwoFactorAuthentication::NEED_AUTHENTICATION] = false + warden.session(resource_name)[TwoFactorAuthentication::name_for(:need_authentication, resource_name)] = false # For compatability with devise versions below v4.2.0 # https://github.com/plataformatec/devise/commit/2044fffa25d781fcbaf090e7728b48b65c854ccb if respond_to?(:bypass_sign_in) @@ -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 - cookies.signed[TwoFactorAuthentication::REMEMBER_TFA_COOKIE_NAME] = { + cookies.signed[TwoFactorAuthentication::name_for(:remember_tfa_cookie_name, Devise::Mapping.find_scope!(resource))] = { value: "#{resource.class}-#{resource.public_send(Devise.second_factor_resource_id)}", expires: expires_seconds.seconds.from_now } diff --git a/lib/two_factor_authentication.rb b/lib/two_factor_authentication.rb index 7b1bbbc1..c462823d 100644 --- a/lib/two_factor_authentication.rb +++ b/lib/two_factor_authentication.rb @@ -38,6 +38,10 @@ module TwoFactorAuthentication NEED_AUTHENTICATION = 'need_two_factor_authentication' REMEMBER_TFA_COOKIE_NAME = "remember_tfa" + def self.name_for(sym, scope) + "#{self.const_get(sym.upcase.to_s)}_#{scope.to_s}" + end + autoload :Schema, 'two_factor_authentication/schema' module Controllers autoload :Helpers, 'two_factor_authentication/controllers/helpers' diff --git a/lib/two_factor_authentication/controllers/helpers.rb b/lib/two_factor_authentication/controllers/helpers.rb index 9a92fa75..000f1829 100644 --- a/lib/two_factor_authentication/controllers/helpers.rb +++ b/lib/two_factor_authentication/controllers/helpers.rb @@ -12,7 +12,8 @@ module Helpers def handle_two_factor_authentication unless devise_controller? Devise.mappings.keys.flatten.any? do |scope| - if signed_in?(scope) and warden.session(scope)[TwoFactorAuthentication::NEED_AUTHENTICATION] + if signed_in?(scope) and warden.session(scope)[TwoFactorAuthentication::name_for(:need_authentication, + scope)] handle_failed_second_factor(scope) end end @@ -22,6 +23,9 @@ def handle_two_factor_authentication def handle_failed_second_factor(scope) if request.format.present? and request.format.html? session["#{scope}_return_to"] = request.original_fullpath if request.get? + + scoped_to_subdomain = public_send("current_#{scope}")&.scoped_to_subdomain + return if scoped_to_subdomain && request.subdomain != scoped_to_subdomain redirect_to two_factor_authentication_path_for(scope) else head :unauthorized @@ -41,8 +45,9 @@ def two_factor_authentication_path_for(resource_or_scope = nil) module Devise module Controllers module Helpers - def is_fully_authenticated? - !session["warden.user.user.session"].try(:[], TwoFactorAuthentication::NEED_AUTHENTICATION) + def is_fully_authenticated?(resource_name) + !session["warden.user.user.session"].try(:[], TwoFactorAuthentication::name_for(:need_authentication, + resource_name)) end end end diff --git a/lib/two_factor_authentication/hooks/two_factor_authenticatable.rb b/lib/two_factor_authentication/hooks/two_factor_authenticatable.rb index 3ff03415..a3e5300e 100644 --- a/lib/two_factor_authentication/hooks/two_factor_authenticatable.rb +++ b/lib/two_factor_authentication/hooks/two_factor_authenticatable.rb @@ -1,17 +1,22 @@ Warden::Manager.after_authentication do |user, auth, options| 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] + actual_cookie_value = auth.env["action_dispatch.cookies"] + .signed[TwoFactorAuthentication::name_for(:remember_tfa_cookie_name, + options[:scope])] bypass_by_cookie = actual_cookie_value == expected_cookie_value 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) + if auth.session(options[:scope])[TwoFactorAuthentication::name_for( + :need_authentication, options[:scope] + )] = user.need_two_factor_authentication?(auth.request) user.send_new_otp if user.send_new_otp_after_login? end end end Warden::Manager.before_logout do |user, auth, _options| - auth.cookies.delete TwoFactorAuthentication::REMEMBER_TFA_COOKIE_NAME if Devise.delete_cookie_on_logout + auth.cookies.delete TwoFactorAuthentication::name_for(:remember_tfa_cookie_name, + _options[:scope]) if Devise.delete_cookie_on_logout end diff --git a/spec/controllers/two_factor_authentication_controller_spec.rb b/spec/controllers/two_factor_authentication_controller_spec.rb index d578d0bb..e978211f 100644 --- a/spec/controllers/two_factor_authentication_controller_spec.rb +++ b/spec/controllers/two_factor_authentication_controller_spec.rb @@ -18,7 +18,7 @@ def post_code(code) it 'returns true' do controller.current_user.send_new_otp post_code controller.current_user.direct_otp - expect(subject.is_fully_authenticated?).to eq true + expect(subject.is_fully_authenticated?("user")).to eq true end end @@ -26,7 +26,7 @@ def post_code(code) it 'returns false' do get :show - expect(subject.is_fully_authenticated?).to eq false + expect(subject.is_fully_authenticated?("user")).to eq false end end @@ -34,7 +34,7 @@ def post_code(code) it 'returns false' do post_code '12345' - expect(subject.is_fully_authenticated?).to eq false + expect(subject.is_fully_authenticated?("user")).to eq false end end end From 3041a9b65df55012117057cb2ca977988b8828a3 Mon Sep 17 00:00:00 2001 From: Dachi Natsvlishvili Date: Fri, 16 Nov 2018 00:14:32 +0400 Subject: [PATCH 2/8] Fix invokation in helper --- lib/two_factor_authentication/controllers/helpers.rb | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/lib/two_factor_authentication/controllers/helpers.rb b/lib/two_factor_authentication/controllers/helpers.rb index 000f1829..73be5742 100644 --- a/lib/two_factor_authentication/controllers/helpers.rb +++ b/lib/two_factor_authentication/controllers/helpers.rb @@ -23,9 +23,11 @@ def handle_two_factor_authentication def handle_failed_second_factor(scope) if request.format.present? and request.format.html? session["#{scope}_return_to"] = request.original_fullpath if request.get? - - scoped_to_subdomain = public_send("current_#{scope}")&.scoped_to_subdomain - return if scoped_to_subdomain && request.subdomain != scoped_to_subdomain + begin + scoped_to_subdomain = public_send("current_#{scope}")&.scoped_to_subdomain + return if scoped_to_subdomain && request.subdomain != scoped_to_subdomain + rescue => e + end redirect_to two_factor_authentication_path_for(scope) else head :unauthorized From d3e002817c429512a1e5be3c08e007dde7dab1e4 Mon Sep 17 00:00:00 2001 From: Dachi Natsvlishvili Date: Fri, 16 Nov 2018 00:31:01 +0400 Subject: [PATCH 3/8] Add another check to helper --- lib/two_factor_authentication/controllers/helpers.rb | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/lib/two_factor_authentication/controllers/helpers.rb b/lib/two_factor_authentication/controllers/helpers.rb index 73be5742..8dbf982a 100644 --- a/lib/two_factor_authentication/controllers/helpers.rb +++ b/lib/two_factor_authentication/controllers/helpers.rb @@ -24,8 +24,9 @@ def handle_failed_second_factor(scope) if request.format.present? and request.format.html? session["#{scope}_return_to"] = request.original_fullpath if request.get? begin - scoped_to_subdomain = public_send("current_#{scope}")&.scoped_to_subdomain - return if scoped_to_subdomain && request.subdomain != scoped_to_subdomain + model = public_send("current_#{scope}").class + return if (scoped_to_subdomain && request.subdomain != model.scoped_to_subdomain) || + (neglect_subdomain && request.subdomain == model.neglect_subdomain) rescue => e end redirect_to two_factor_authentication_path_for(scope) From 10d0033a87541b70152e6aa79eb7de2f2422e5b5 Mon Sep 17 00:00:00 2001 From: Dachi Natsvlishvili Date: Fri, 16 Nov 2018 00:50:18 +0400 Subject: [PATCH 4/8] Refactor check --- lib/two_factor_authentication/controllers/helpers.rb | 9 ++------- .../hooks/two_factor_authenticatable.rb | 2 +- .../models/two_factor_authenticatable.rb | 9 +++++++++ 3 files changed, 12 insertions(+), 8 deletions(-) diff --git a/lib/two_factor_authentication/controllers/helpers.rb b/lib/two_factor_authentication/controllers/helpers.rb index 8dbf982a..3307bf90 100644 --- a/lib/two_factor_authentication/controllers/helpers.rb +++ b/lib/two_factor_authentication/controllers/helpers.rb @@ -13,7 +13,8 @@ def handle_two_factor_authentication unless devise_controller? Devise.mappings.keys.flatten.any? do |scope| if signed_in?(scope) and warden.session(scope)[TwoFactorAuthentication::name_for(:need_authentication, - scope)] + scope)] and + public_send("current_#{scope}").class.subdomain_in_scope? handle_failed_second_factor(scope) end end @@ -23,12 +24,6 @@ def handle_two_factor_authentication def handle_failed_second_factor(scope) if request.format.present? and request.format.html? session["#{scope}_return_to"] = request.original_fullpath if request.get? - begin - model = public_send("current_#{scope}").class - return if (scoped_to_subdomain && request.subdomain != model.scoped_to_subdomain) || - (neglect_subdomain && request.subdomain == model.neglect_subdomain) - rescue => e - end redirect_to two_factor_authentication_path_for(scope) else head :unauthorized diff --git a/lib/two_factor_authentication/hooks/two_factor_authenticatable.rb b/lib/two_factor_authentication/hooks/two_factor_authenticatable.rb index a3e5300e..1fd00d8b 100644 --- a/lib/two_factor_authentication/hooks/two_factor_authenticatable.rb +++ b/lib/two_factor_authentication/hooks/two_factor_authenticatable.rb @@ -10,7 +10,7 @@ if user.respond_to?(:need_two_factor_authentication?) && !bypass_by_cookie if auth.session(options[:scope])[TwoFactorAuthentication::name_for( :need_authentication, options[:scope] - )] = user.need_two_factor_authentication?(auth.request) + )] = user.need_two_factor_authentication?(auth.request) and user.class.subdomain_in_scope? user.send_new_otp if user.send_new_otp_after_login? end end diff --git a/lib/two_factor_authentication/models/two_factor_authenticatable.rb b/lib/two_factor_authentication/models/two_factor_authenticatable.rb index bd1df46e..87bee2f0 100644 --- a/lib/two_factor_authentication/models/two_factor_authenticatable.rb +++ b/lib/two_factor_authentication/models/two_factor_authenticatable.rb @@ -101,6 +101,15 @@ def create_direct_otp(options = {}) ) end + def subdomain_in_scope? + begin + false if (scoped_to_subdomain && request.subdomain != scoped_to_subdomain) || + (neglect_subdomain && request.subdomain == neglect_subdomain) + rescue + true + end + end + private def without_spaces(code) From 0a3a710e9ddb1f0195972f75b1e68131f337a32d Mon Sep 17 00:00:00 2001 From: Dachi Natsvlishvili Date: Fri, 16 Nov 2018 01:10:27 +0400 Subject: [PATCH 5/8] Fix class method --- .../models/two_factor_authenticatable.rb | 21 +++++++++++-------- 1 file changed, 12 insertions(+), 9 deletions(-) diff --git a/lib/two_factor_authentication/models/two_factor_authenticatable.rb b/lib/two_factor_authentication/models/two_factor_authenticatable.rb index 87bee2f0..349f022e 100644 --- a/lib/two_factor_authentication/models/two_factor_authenticatable.rb +++ b/lib/two_factor_authentication/models/two_factor_authenticatable.rb @@ -11,6 +11,7 @@ module ClassMethods def has_one_time_password(options = {}) include InstanceMethodsOnActivation include EncryptionInstanceMethods if options[:encrypted] == true + extend ClassMethodsOnActivation end ::Devise::Models.config( @@ -20,6 +21,17 @@ def has_one_time_password(options = {}) ) end + module ClassMethodsOnActivation + def subdomain_in_scope? + begin + false if (scoped_to_subdomain && request.subdomain != scoped_to_subdomain) || + (neglect_subdomain && request.subdomain == neglect_subdomain) + rescue + true + end + end + end + module InstanceMethodsOnActivation def authenticate_otp(code, options = {}) return true if direct_otp && authenticate_direct_otp(code) @@ -101,15 +113,6 @@ def create_direct_otp(options = {}) ) end - def subdomain_in_scope? - begin - false if (scoped_to_subdomain && request.subdomain != scoped_to_subdomain) || - (neglect_subdomain && request.subdomain == neglect_subdomain) - rescue - true - end - end - private def without_spaces(code) From 944c15d47f180e81f04ead0bfb58fa96db9e07fd Mon Sep 17 00:00:00 2001 From: Dachi Natsvlishvili Date: Fri, 16 Nov 2018 01:20:27 +0400 Subject: [PATCH 6/8] Refactor scope method --- lib/two_factor_authentication/controllers/helpers.rb | 2 +- .../hooks/two_factor_authenticatable.rb | 2 +- .../models/two_factor_authenticatable.rb | 6 +++--- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/lib/two_factor_authentication/controllers/helpers.rb b/lib/two_factor_authentication/controllers/helpers.rb index 3307bf90..5c66b0ce 100644 --- a/lib/two_factor_authentication/controllers/helpers.rb +++ b/lib/two_factor_authentication/controllers/helpers.rb @@ -14,7 +14,7 @@ def handle_two_factor_authentication Devise.mappings.keys.flatten.any? do |scope| if signed_in?(scope) and warden.session(scope)[TwoFactorAuthentication::name_for(:need_authentication, scope)] and - public_send("current_#{scope}").class.subdomain_in_scope? + public_send("current_#{scope}").class.subdomain_in_scope?(request.subdomain) handle_failed_second_factor(scope) end end diff --git a/lib/two_factor_authentication/hooks/two_factor_authenticatable.rb b/lib/two_factor_authentication/hooks/two_factor_authenticatable.rb index 1fd00d8b..5ed6ff2d 100644 --- a/lib/two_factor_authentication/hooks/two_factor_authenticatable.rb +++ b/lib/two_factor_authentication/hooks/two_factor_authenticatable.rb @@ -10,7 +10,7 @@ if user.respond_to?(:need_two_factor_authentication?) && !bypass_by_cookie if auth.session(options[:scope])[TwoFactorAuthentication::name_for( :need_authentication, options[:scope] - )] = user.need_two_factor_authentication?(auth.request) and user.class.subdomain_in_scope? + )] = user.need_two_factor_authentication?(auth.request) and user.class.subdomain_in_scope?(auth.request.subdomain) user.send_new_otp if user.send_new_otp_after_login? end end diff --git a/lib/two_factor_authentication/models/two_factor_authenticatable.rb b/lib/two_factor_authentication/models/two_factor_authenticatable.rb index 349f022e..57dff02a 100644 --- a/lib/two_factor_authentication/models/two_factor_authenticatable.rb +++ b/lib/two_factor_authentication/models/two_factor_authenticatable.rb @@ -22,10 +22,10 @@ def has_one_time_password(options = {}) end module ClassMethodsOnActivation - def subdomain_in_scope? + def subdomain_in_scope?(subdomain) begin - false if (scoped_to_subdomain && request.subdomain != scoped_to_subdomain) || - (neglect_subdomain && request.subdomain == neglect_subdomain) + false if (scoped_to_subdomain && subdomain != scoped_to_subdomain) || + (neglect_subdomain && subdomain == neglect_subdomain) rescue true end From 0082af9e204a435e9908bbaaf8d20b13e4283ca8 Mon Sep 17 00:00:00 2001 From: Dachi Natsvlishvili Date: Fri, 16 Nov 2018 01:45:51 +0400 Subject: [PATCH 7/8] Fix subdomain_in_scope? --- .../models/two_factor_authenticatable.rb | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/lib/two_factor_authentication/models/two_factor_authenticatable.rb b/lib/two_factor_authentication/models/two_factor_authenticatable.rb index 57dff02a..60ca3bd8 100644 --- a/lib/two_factor_authentication/models/two_factor_authenticatable.rb +++ b/lib/two_factor_authentication/models/two_factor_authenticatable.rb @@ -23,12 +23,8 @@ def has_one_time_password(options = {}) module ClassMethodsOnActivation def subdomain_in_scope?(subdomain) - begin - false if (scoped_to_subdomain && subdomain != scoped_to_subdomain) || - (neglect_subdomain && subdomain == neglect_subdomain) - rescue - true - end + (respond_to?(:scoped_to_subdomain) && subdomain == scoped_to_subdomain) || + (respond_to?(:neglect_subdomain) && subdomain != neglect_subdomain) end end From b98cf9d6a867b09b6bed964d342f653af67b4918 Mon Sep 17 00:00:00 2001 From: Dachi Natsvlishvili Date: Fri, 16 Nov 2018 05:56:48 +0400 Subject: [PATCH 8/8] Use match against single domain --- .../models/two_factor_authenticatable.rb | 4 +-- .../two_factor_authenticatable_spec.rb | 2 +- .../models/two_factor_authenticatable_spec.rb | 27 +++++++++++++++++++ spec/support/controller_helper.rb | 2 +- spec/support/features_spec_helper.rb | 4 +-- 5 files changed, 33 insertions(+), 6 deletions(-) diff --git a/lib/two_factor_authentication/models/two_factor_authenticatable.rb b/lib/two_factor_authentication/models/two_factor_authenticatable.rb index 60ca3bd8..ff841613 100644 --- a/lib/two_factor_authentication/models/two_factor_authenticatable.rb +++ b/lib/two_factor_authentication/models/two_factor_authenticatable.rb @@ -23,8 +23,8 @@ def has_one_time_password(options = {}) module ClassMethodsOnActivation def subdomain_in_scope?(subdomain) - (respond_to?(:scoped_to_subdomain) && subdomain == scoped_to_subdomain) || - (respond_to?(:neglect_subdomain) && subdomain != neglect_subdomain) + return true unless respond_to? :two_factor_subdomains + !!(subdomain =~ two_factor_subdomains) end end diff --git a/spec/features/two_factor_authenticatable_spec.rb b/spec/features/two_factor_authenticatable_spec.rb index d433d636..9588c9c8 100644 --- a/spec/features/two_factor_authenticatable_spec.rb +++ b/spec/features/two_factor_authenticatable_spec.rb @@ -189,7 +189,7 @@ def sms_sign_in end it 'sets the warden session need_two_factor_authentication key to true' do - session_hash = { 'need_two_factor_authentication' => true } + session_hash = { 'need_two_factor_authentication_user' => true } expect(page.get_rack_session_key('warden.user.user.session')).to eq session_hash end diff --git a/spec/lib/two_factor_authentication/models/two_factor_authenticatable_spec.rb b/spec/lib/two_factor_authentication/models/two_factor_authenticatable_spec.rb index 3a932d60..5436cb11 100644 --- a/spec/lib/two_factor_authentication/models/two_factor_authenticatable_spec.rb +++ b/spec/lib/two_factor_authentication/models/two_factor_authenticatable_spec.rb @@ -323,4 +323,31 @@ def instance.send_two_factor_authentication_code(code) end end end + + describe 'self.subdomain_in_scope?' do + let(:user) { EncryptedUser.new } + + it 'allows scope if no optional methods' do + expect(user.class.subdomain_in_scope?('test1')) + end + + it 'correctly parses a regex (neglect example)' do + allow(user.class).to receive(:two_factor_subdomains).and_return /^(?!(bad)$).*$/ + + expect(user.class.subdomain_in_scope?('good')).to be(true) + expect(user.class.subdomain_in_scope?('another-one')).to be(true) + expect(user.class.subdomain_in_scope?('bad')).to be(false) + expect(user.class.subdomain_in_scope?('goodbadugly')).to be(true) + expect(user.class.subdomain_in_scope?('')).to be(true) + end + + it 'correctly parses a regex (whitelist example)' do + allow(user.class).to receive(:two_factor_subdomains).and_return /^(good|ugly)$/ + + expect(user.class.subdomain_in_scope?('good')).to be(true) + expect(user.class.subdomain_in_scope?('ugly')).to be(true) + expect(user.class.subdomain_in_scope?('bad')).to be(false) + expect(user.class.subdomain_in_scope?('')).to be(false) + end + end end diff --git a/spec/support/controller_helper.rb b/spec/support/controller_helper.rb index 2ca3a31f..5bc58f03 100644 --- a/spec/support/controller_helper.rb +++ b/spec/support/controller_helper.rb @@ -2,7 +2,7 @@ module ControllerHelper def sign_in(user = create_user('not_encrypted')) allow(warden).to receive(:authenticated?).with(:user).and_return(true) allow(controller).to receive(:current_user).and_return(user) - warden.session(:user)[TwoFactorAuthentication::NEED_AUTHENTICATION] = true + warden.session(:user)[TwoFactorAuthentication::name_for(:need_authentication, "user")] = true end end diff --git a/spec/support/features_spec_helper.rb b/spec/support/features_spec_helper.rb index 9662e19e..138d0a88 100644 --- a/spec/support/features_spec_helper.rb +++ b/spec/support/features_spec_helper.rb @@ -20,11 +20,11 @@ def get_cookie key end def set_tfa_cookie value - set_cookie TwoFactorAuthentication::REMEMBER_TFA_COOKIE_NAME, value + set_cookie TwoFactorAuthentication::name_for(:remember_tfa_cookie_name, "user"), value end def get_tfa_cookie - get_cookie TwoFactorAuthentication::REMEMBER_TFA_COOKIE_NAME + get_cookie TwoFactorAuthentication::name_for(:remember_tfa_cookie_name, "user") end end