Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace instances of update_attributes with update to avoid deprecation warnings in Rails 6. #178

Open
wants to merge 19 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions Rakefile
Original file line number Diff line number Diff line change
@@ -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__)
Expand Down
6 changes: 3 additions & 3 deletions app/views/devise/two_factor_authentication/show.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -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 %>
3 changes: 3 additions & 0 deletions lib/two_factor_authentication.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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'

Expand Down
17 changes: 9 additions & 8 deletions lib/two_factor_authentication/controllers/helpers.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
19 changes: 11 additions & 8 deletions lib/two_factor_authentication/hooks/two_factor_authenticatable.rb
Original file line number Diff line number Diff line change
@@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
)
Expand All @@ -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

Expand Down
16 changes: 11 additions & 5 deletions lib/two_factor_authentication/routes.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
54 changes: 54 additions & 0 deletions spec/features/two_factor_authenticatable_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down Expand Up @@ -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

Expand All @@ -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

Expand All @@ -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

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 2 additions & 0 deletions spec/rails_app/app/assets/config/manifest.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
//= link application.css
//= link application.js
9 changes: 9 additions & 0 deletions spec/rails_app/app/controllers/home_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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: "<success></success>"
end
end
end

end
2 changes: 1 addition & 1 deletion spec/rails_app/app/models/guest_user.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
1 change: 1 addition & 0 deletions spec/rails_app/config/initializers/devise.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
65 changes: 65 additions & 0 deletions spec/requests/api_request_spec.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
require 'spec_helper'

describe "API request", type: :request do
context "when logged in" do
let(:user) { create_user("encrypted", email: "[email protected]", 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("<success></success>")
end
end
end
end
end
2 changes: 2 additions & 0 deletions spec/spec_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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}