Skip to content
Merged
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: 3 additions & 3 deletions app/controllers/concerns/two_factor_authenticatable.rb
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ def handle_valid_otp
end

def authentication_context?
context == 'authentication'
context == 'authentication' || context == 'reauthentication'
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe now is still not the time...but I think it would be great if we could make these auth contexts into constants? Making them constants helps remind that they're finite and enumerable

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agreed. I was planning on making another pass at this module to see how to improve it.

end

def confirmation_context?
Expand All @@ -71,7 +71,7 @@ def delivery_method
# You can pass in any "type" with a corresponding I18n key in
# devise.two_factor_authentication.invalid_#{type}
def handle_invalid_otp(type: 'otp')
update_invalid_user if current_user.two_factor_enabled? && context == 'authentication'
update_invalid_user if current_user.two_factor_enabled? && authentication_context?

flash.now[:error] = t("devise.two_factor_authentication.invalid_#{type}")

Expand Down Expand Up @@ -183,7 +183,7 @@ def otp_phone_view_data
end

def display_phone_to_deliver_to
if context == 'authentication'
if authentication_context?
decorated_user.masked_two_factor_phone_number
else
user_session[:unconfirmed_phone]
Expand Down
2 changes: 1 addition & 1 deletion app/controllers/reauthn_required_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ def confirm_recently_authenticated
return unless user_signed_in?
return if recently_authenticated?
store_location_for(:user, request.url)
user_session[:context] = 'authentication'
user_session[:context] = 'reauthentication'
redirect_to user_password_confirm_url
end

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ def delivery_params
end

def phone_to_deliver_to
return current_user.phone if context == 'authentication'
return current_user.phone if authentication_context?

user_session[:unconfirmed_phone]
end
Expand Down
2 changes: 1 addition & 1 deletion spec/controllers/reauthn_required_controller_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ def show
it 'sets context to authentication' do
get :show

expect(controller.user_session[:context]).to eq 'authentication'
expect(controller.user_session[:context]).to eq 'reauthentication'
end
end
end
Expand Down
25 changes: 25 additions & 0 deletions spec/features/two_factor_authentication/change_factor_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,31 @@
end
end

context 'resending OTP code to old phone' do
it 'resends OTP and prompts user to enter their code' do
allow(SmsOtpSenderJob).to receive(:perform_later)

user = sign_in_and_2fa_user
old_phone = user.phone

Timecop.travel(Figaro.env.reauthn_window.to_i + 1) do
visit manage_phone_path
complete_2fa_confirmation_without_entering_otp
click_link t('links.two_factor_authentication.resend_code.sms')

expect(SmsOtpSenderJob).to have_received(:perform_later).
with(
code: user.reload.direct_otp,
phone: old_phone,
otp_created_at: user.reload.direct_otp_sent_at.to_s
)

expect(current_path).
to eq login_two_factor_path(delivery_method: 'sms')
end
end
end

scenario 'editing email' do
visit manage_email_path
complete_2fa_confirmation
Expand Down