From bdb89e50f8c3ad5ef2210dd966c7321218a676a8 Mon Sep 17 00:00:00 2001 From: Dmitrii Golub Date: Mon, 1 Feb 2016 15:07:39 +0800 Subject: [PATCH] fix: different cookies for different users --- Gemfile | 1 + .../two_factor_authentication_controller.rb | 2 +- .../hooks/two_factor_authenticatable.rb | 11 +++- .../two_factor_authenticatable_spec.rb | 62 +++++++++++++++++-- spec/support/features_spec_helper.rb | 17 ++++- 5 files changed, 85 insertions(+), 8 deletions(-) diff --git a/Gemfile b/Gemfile index 59ad495..70ab496 100644 --- a/Gemfile +++ b/Gemfile @@ -27,4 +27,5 @@ end group :test do gem 'rack_session_access' gem 'ammeter' + gem 'pry' end diff --git a/app/controllers/devise/two_factor_authentication_controller.rb b/app/controllers/devise/two_factor_authentication_controller.rb index 856cc12..f40c10c 100644 --- a/app/controllers/devise/two_factor_authentication_controller.rb +++ b/app/controllers/devise/two_factor_authentication_controller.rb @@ -22,7 +22,7 @@ class Devise::TwoFactorAuthenticationController < DeviseController if expires_seconds && expires_seconds > 0 cookies.signed[TwoFactorAuthentication::REMEMBER_TFA_COOKIE_NAME] = { - value: true, + value: "#{resource.class}-#{resource.id}", expires: expires_seconds.from_now } end diff --git a/lib/two_factor_authentication/hooks/two_factor_authenticatable.rb b/lib/two_factor_authentication/hooks/two_factor_authenticatable.rb index 3f89bdb..5d19ac2 100644 --- a/lib/two_factor_authentication/hooks/two_factor_authenticatable.rb +++ b/lib/two_factor_authentication/hooks/two_factor_authenticatable.rb @@ -1,8 +1,15 @@ Warden::Manager.after_authentication do |user, auth, options| reset_otp_state_for(user) - if user.respond_to?(:need_two_factor_authentication?) && - !auth.env["action_dispatch.cookies"].signed[TwoFactorAuthentication::REMEMBER_TFA_COOKIE_NAME] + expected_cookie_value = "#{user.class}-#{user.id}" + actual_cookie_value = auth.env["action_dispatch.cookies"].signed[TwoFactorAuthentication::REMEMBER_TFA_COOKIE_NAME] + if actual_cookie_value.nil? + bypass_by_cookie = false + else + 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) user.send_two_factor_authentication_code end diff --git a/spec/features/two_factor_authenticatable_spec.rb b/spec/features/two_factor_authenticatable_spec.rb index 29aa44f..753d69f 100644 --- a/spec/features/two_factor_authenticatable_spec.rb +++ b/spec/features/two_factor_authenticatable_spec.rb @@ -138,6 +138,44 @@ feature "User of two factor authentication" do expect(page).to have_content("You are signed in as Marissa") expect(page).to have_content("Enter your personal code") end + + scenario 'TFA should be different for different users' do + visit user_two_factor_authentication_path + fill_in 'code', with: user.otp_code + click_button 'Submit' + + tfa_cookie1 = get_tfa_cookie() + + logout + reset_session! + + user2 = create_user() + login_as(user2) + visit user_two_factor_authentication_path + fill_in 'code', with: user2.otp_code + click_button 'Submit' + + tfa_cookie2 = get_tfa_cookie() + + expect(tfa_cookie1).not_to eq tfa_cookie2 + end + + scenario 'TFA should be unique for specific user' do + visit user_two_factor_authentication_path + fill_in 'code', with: user.otp_code + click_button 'Submit' + + tfa_cookie1 = get_tfa_cookie() + + logout + reset_session! + + user2 = create_user() + set_tfa_cookie(tfa_cookie1) + login_as(user2) + visit dashboard_path + expect(page).to have_content('Enter your personal code') + end end it 'sets the warden session need_two_factor_authentication key to true' do @@ -151,7 +189,11 @@ feature "User of two factor authentication" do let(:user) { create_user } scenario 'when UserOtpSender#reset_otp_state is defined' do - stub_const 'UserOtpSender', Class.new + klass = stub_const 'UserOtpSender', Class.new + + klass.class_eval do + def reset_otp_state; end + end otp_sender = instance_double(UserOtpSender) expect(UserOtpSender).to receive(:new).with(user).and_return(otp_sender) @@ -162,7 +204,11 @@ feature "User of two factor authentication" do end scenario 'when UserOtpSender#reset_otp_state is not defined' do - stub_const 'UserOtpSender', Class.new + klass = stub_const 'UserOtpSender', Class.new + + klass.class_eval do + def reset_otp_state; end + end otp_sender = instance_double(UserOtpSender) allow(otp_sender).to receive(:respond_to?).with(:reset_otp_state).and_return(false) @@ -182,7 +228,11 @@ feature "User of two factor authentication" do visit new_user_session_path complete_sign_in_form_for(user) - stub_const 'UserOtpSender', Class.new + klass = stub_const 'UserOtpSender', Class.new + klass.class_eval do + def reset_otp_state; end + end + otp_sender = instance_double(UserOtpSender) expect(UserOtpSender).to receive(:new).with(user).and_return(otp_sender) @@ -195,7 +245,11 @@ feature "User of two factor authentication" do visit new_user_session_path complete_sign_in_form_for(user) - stub_const 'UserOtpSender', Class.new + klass = stub_const 'UserOtpSender', Class.new + klass.class_eval do + def reset_otp_state; end + end + otp_sender = instance_double(UserOtpSender) allow(otp_sender).to receive(:respond_to?).with(:reset_otp_state).and_return(false) diff --git a/spec/support/features_spec_helper.rb b/spec/support/features_spec_helper.rb index e5aaa3b..9662e19 100644 --- a/spec/support/features_spec_helper.rb +++ b/spec/support/features_spec_helper.rb @@ -10,6 +10,22 @@ module FeaturesSpecHelper fill_in "Password", with: 'password' find('.actions input').click # 'Sign in' or 'Log in' end + + def set_cookie key, value + page.driver.browser.set_cookie [key, value].join('=') + end + + def get_cookie key + Capybara.current_session.driver.request.cookies[key] + end + + def set_tfa_cookie value + set_cookie TwoFactorAuthentication::REMEMBER_TFA_COOKIE_NAME, value + end + + def get_tfa_cookie + get_cookie TwoFactorAuthentication::REMEMBER_TFA_COOKIE_NAME + end end RSpec.configure do |config| @@ -24,4 +40,3 @@ RSpec.configure do |config| Warden.test_reset! end end -