diff --git a/Rakefile b/Rakefile index 7723dfbb..9a575025 100644 --- a/Rakefile +++ b/Rakefile @@ -1,3 +1,9 @@ +begin + require 'bundler/setup' +rescue LoadError + puts 'You must `gem install bundler` and `bundle install` to run rake tasks' +end + require "bundler/gem_tasks" APP_RAKEFILE = File.expand_path("../spec/rails_app/Rakefile", __FILE__) diff --git a/app/views/devise/two_factor_authentication/show.html.erb b/app/views/devise/two_factor_authentication/show.html.erb index 9a3ae060..77f3707e 100644 --- a/app/views/devise/two_factor_authentication/show.html.erb +++ b/app/views/devise/two_factor_authentication/show.html.erb @@ -12,8 +12,8 @@ <% end %> <% if resource.direct_otp %> - <%= link_to "Resend Code", send("resend_code_#{resource_name}_two_factor_authentication_path"), action: :get %> + <%= link_to "Resend Code", send("resend_code_#{resource_name}_two_factor_authentication_path"), method: :post %> <% else %> -<%= link_to "Send me a code instead", send("resend_code_#{resource_name}_two_factor_authentication_path"), action: :get %> +<%= link_to "Send me a code instead", send("resend_code_#{resource_name}_two_factor_authentication_path"), method: :post %> <% end %> -<%= link_to "Sign out", send("destroy_#{resource_name}_session_path"), :method => :delete %> +<%= link_to "Sign out", send("destroy_#{resource_name}_session_path"), method: :delete %> diff --git a/lib/two_factor_authentication.rb b/lib/two_factor_authentication.rb index 7b1bbbc1..247b541f 100644 --- a/lib/two_factor_authentication.rb +++ b/lib/two_factor_authentication.rb @@ -27,6 +27,9 @@ module Devise mattr_accessor :otp_secret_encryption_key @@otp_secret_encryption_key = '' + mattr_accessor :skip_send_new_otp_in_after_set_user_for + @@skip_send_new_otp_in_after_set_user_for = [] + mattr_accessor :second_factor_resource_id @@second_factor_resource_id = 'id' diff --git a/lib/two_factor_authentication/controllers/helpers.rb b/lib/two_factor_authentication/controllers/helpers.rb index 64e8377c..501f4d9a 100644 --- a/lib/two_factor_authentication/controllers/helpers.rb +++ b/lib/two_factor_authentication/controllers/helpers.rb @@ -20,14 +20,15 @@ def handle_two_factor_authentication end def handle_failed_second_factor(scope) - if request.format.present? - if request.format.html? - session["#{scope}_return_to"] = request.original_fullpath if request.get? - redirect_to two_factor_authentication_path_for(scope) - elsif request.format.json? - session["#{scope}_return_to"] = root_path(format: :html) - render json: { redirect_to: two_factor_authentication_path_for(scope) }, status: :unauthorized - end + if request.format&.html? + session["#{scope}_return_to"] = request.original_fullpath if request.get? + redirect_to two_factor_authentication_path_for(scope) + elsif request.format&.json? + session["#{scope}_return_to"] = root_path(format: :html) + render json: { + redirect_to: two_factor_authentication_path_for(scope), + authentication_type: send("current_#{scope}")&.direct_otp ? :otp : :totp + }, status: :unauthorized else head :unauthorized 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..28a2b0f7 100644 --- a/lib/two_factor_authentication/hooks/two_factor_authenticatable.rb +++ b/lib/two_factor_authentication/hooks/two_factor_authenticatable.rb @@ -1,13 +1,16 @@ 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] - bypass_by_cookie = actual_cookie_value == expected_cookie_value - end + scopes_to_skip = [Rails.application.config.devise.skip_send_new_otp_in_after_set_user_for].compact.flatten + unless options[:scope].in?(scopes_to_skip) + 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 + 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 if user.send_new_otp_after_login? + 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 if user.send_new_otp_after_login? + end end 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 6d73a0fb..0e85c490 100644 --- a/lib/two_factor_authentication/models/two_factor_authenticatable.rb +++ b/lib/two_factor_authentication/models/two_factor_authenticatable.rb @@ -40,7 +40,7 @@ def authenticate_totp(code, options = {}) raise "authenticate_totp called with no otp_secret_key set" if totp_secret.nil? totp = ROTP::TOTP.new(totp_secret, digits: digits) new_timestamp = totp.verify( - without_spaces(code), + without_spaces(code), drift_ahead: drift, drift_behind: drift, after: totp_timestamp ) return false unless new_timestamp @@ -101,7 +101,7 @@ def generate_totp_secret def create_direct_otp(options = {}) # Create a new random OTP and store it in the database digits = options[:length] || self.class.direct_otp_length || 6 - update_attributes( + update( direct_otp: random_base10(digits), direct_otp_sent_at: Time.now.utc ) @@ -122,7 +122,7 @@ def direct_otp_expired? end def clear_direct_otp - update_attributes(direct_otp: nil, direct_otp_sent_at: nil) + update(direct_otp: nil, direct_otp_sent_at: nil) end end diff --git a/lib/two_factor_authentication/routes.rb b/lib/two_factor_authentication/routes.rb index 543059a2..1c1a8b26 100644 --- a/lib/two_factor_authentication/routes.rb +++ b/lib/two_factor_authentication/routes.rb @@ -2,10 +2,16 @@ module ActionDispatch::Routing class Mapper protected - def devise_two_factor_authentication(mapping, controllers) - resource :two_factor_authentication, :only => [:show, :update, :resend_code], :path => mapping.path_names[:two_factor_authentication], :controller => controllers[:two_factor_authentication] do - collection { get "resend_code" } - end - end + def devise_two_factor_authentication(mapping, controllers) + path = mapping.path_names[:two_factor_authentication] + controller = controllers[:two_factor_authentication] + + resource :two_factor_authentication, + only: [:show, :update], + path: path, + controller: controller + + post "#{path}/resend_code", to: "#{controller}#resend_code" + end end end diff --git a/spec/features/two_factor_authenticatable_spec.rb b/spec/features/two_factor_authenticatable_spec.rb index d433d636..e31a9f9b 100644 --- a/spec/features/two_factor_authenticatable_spec.rb +++ b/spec/features/two_factor_authenticatable_spec.rb @@ -76,6 +76,22 @@ expect(page).to have_content("Param B is param b") end + scenario "is redirected to TFA when path requires authentication and works with resent code" do + visit dashboard_path + "?A=param%20a&B=param%20b" + + expect(page).to_not have_content("Your Personal Dashboard") + SMSProvider.messages.clear() + + click_on "Resend Code" + fill_in "code", with: SMSProvider.last_message.body + click_button "Submit" + + expect(page).to have_content("Your Personal Dashboard") + expect(page).to have_content("You are signed in as Marissa") + expect(page).to have_content("Param A is param a") + expect(page).to have_content("Param B is param b") + end + scenario "is locked out after max failed attempts" do visit user_two_factor_authentication_path @@ -136,6 +152,21 @@ expect(page).to have_content("Enter the code that was sent to you") end + scenario 'Sends OTP code by SMS' do + login_as user + SMSProvider.messages.clear() + visit dashboard_path + expect(SMSProvider.messages).not_to be_empty + end + + scenario "Doesn't sends OTP code by SMS upon every request if so configured" do + login_as user + SMSProvider.messages.clear() + allow(Rails.application.config.devise).to receive(:skip_send_new_otp_in_after_set_user_for).and_return([:user]) + visit dashboard_path + expect(SMSProvider.messages).to be_empty + end + scenario 'TFA should be different for different users' do sms_sign_in @@ -160,6 +191,14 @@ def sms_sign_in click_button 'Submit' end + def sms_sign_in_with_resent_code + visit user_two_factor_authentication_path + SMSProvider.messages.clear() + click_on 'Resend Code' + fill_in 'code', with: SMSProvider.last_message.body + click_button 'Submit' + end + scenario 'TFA should be unique for specific user' do sms_sign_in @@ -175,6 +214,21 @@ def sms_sign_in expect(page).to have_content("Enter the code that was sent to you") end + scenario 'TFA should be unique for specific user with resent code' do + sms_sign_in_with_resent_code + + 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 the code that was sent to you") + end + scenario 'Delete cookie when user logs out if enabled' do user.class.delete_cookie_on_logout = true 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 6fb4f505..6e58fd1d 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 @@ -138,7 +138,7 @@ def instance.send_two_factor_authentication_code(code) it "returns uri with user's email" do expect(instance.provisioning_uri). - to match(%r{otpauth://totp/houdini@example.com\?secret=\w{32}}) + to match(%r{otpauth://totp/houdini%40example.com\?secret=\w{32}}) end it 'returns uri with issuer option' do diff --git a/spec/rails_app/app/assets/config/manifest.js b/spec/rails_app/app/assets/config/manifest.js new file mode 100644 index 00000000..a3d7d420 --- /dev/null +++ b/spec/rails_app/app/assets/config/manifest.js @@ -0,0 +1,2 @@ +//= link application.css +//= link application.js diff --git a/spec/rails_app/app/controllers/home_controller.rb b/spec/rails_app/app/controllers/home_controller.rb index f55da102..7007d886 100644 --- a/spec/rails_app/app/controllers/home_controller.rb +++ b/spec/rails_app/app/controllers/home_controller.rb @@ -5,6 +5,15 @@ def index end def dashboard + respond_to do |format| + format.html + format.json do + render json: {success: true} + end + format.xml do + render xml: "" + end + end end end diff --git a/spec/rails_app/app/models/guest_user.rb b/spec/rails_app/app/models/guest_user.rb index 8003624c..1222279a 100644 --- a/spec/rails_app/app/models/guest_user.rb +++ b/spec/rails_app/app/models/guest_user.rb @@ -7,7 +7,7 @@ class GuestUser attr_accessor :direct_otp, :direct_otp_sent_at, :otp_secret_key, :email, :second_factor_attempts_count, :totp_timestamp - def update_attributes(attrs) + def update(attrs) attrs.each do |key, value| send(key.to_s + '=', value) end diff --git a/spec/rails_app/config/initializers/devise.rb b/spec/rails_app/config/initializers/devise.rb index 9083d7f4..6a714f08 100644 --- a/spec/rails_app/config/initializers/devise.rb +++ b/spec/rails_app/config/initializers/devise.rb @@ -255,4 +255,5 @@ # config.omniauth_path_prefix = '/my_engine/users/auth' config.otp_secret_encryption_key = '0a8283fba984da1de24e4df1e93046cb53c5787944ef037b2dbf3e61d20fe11f25e25a855cec605fdf65b162329890d7230afdf64f681b4c32020281054e73ec' + config.skip_send_new_otp_in_after_set_user_for = [] end diff --git a/spec/requests/api_request_spec.rb b/spec/requests/api_request_spec.rb new file mode 100644 index 00000000..c7b48ad8 --- /dev/null +++ b/spec/requests/api_request_spec.rb @@ -0,0 +1,65 @@ +require 'spec_helper' + +describe "API request", type: :request do + context "when logged in" do + let(:user) { create_user("encrypted", email: "foo@example.com", otp_secret_key: "6iispf5cjufa4vsm") } + + before do + sign_in user + end + + context "with json" do + context "with totp authentication" do + it "returns 401 when path requires authentication" do + get "/dashboard.json" + expect(response.response_code).to eq(401) + body = JSON.parse(response.body) + expect(body["redirect_to"]).to eq(user_two_factor_authentication_path) + expect(body["authentication_type"]).to eq("totp") + end + end + + context "with direct otp authentication" do + it "returns 401 when path requires authentication" do + user.update!(direct_otp: true) + get "/dashboard.json" + expect(response.response_code).to eq(401) + body = JSON.parse(response.body) + expect(body["redirect_to"]).to eq(user_two_factor_authentication_path) + expect(body["authentication_type"]).to eq("otp") + end + end + + context "after TFA" do + it "returns successfully" do + get "/dashboard.json" + expect(response.response_code).to eq(401) + expect(JSON.parse(response.body)["redirect_to"]).to eq(user_two_factor_authentication_path) + totp_code = ROTP::TOTP.new(user.otp_secret_key, digits: 6).at(Time.now) + put "/users/two_factor_authentication", params: { code: totp_code } + get "/dashboard.json" + expect(response.response_code).to eq(200) + body = JSON.parse(response.body) + expect(body["success"]).to eq(true) + end + end + end + + context "with xml" do + it "returns 401 when path requires authentication" do + get "/dashboard.xml" + expect(response.response_code).to eq(401) + end + + context "after TFA" do + it "returns successfully" do + totp_code = ROTP::TOTP.new(user.otp_secret_key, digits: 6).at(Time.now) + put "/users/two_factor_authentication", params: { code: totp_code } + get "/dashboard.xml" + expect(response.response_code).to eq(200) + expect(response.body).to eq("") + end + end + end + end +end \ No newline at end of file diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index 63704333..02ac6723 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -21,6 +21,8 @@ config.order = 'random' config.after(:each) { Timecop.return } + + config.include Devise::Test::IntegrationHelpers, type: :request end Dir["#{Dir.pwd}/spec/support/**/*.rb"].each {|f| require f}