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
4 changes: 4 additions & 0 deletions .reek
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,7 @@ NestedIterators:
NilCheck:
enabled: false
LongParameterList:
max_params: 4
exclude:
- IdentityLinker#optional_attributes
- Idv::ProoferJob#perform
Expand Down Expand Up @@ -127,6 +128,9 @@ UncommunicativeModuleName:
- X509::Attribute
- X509::Attributes
- X509::SessionStore
UnusedParameters:
exclude:
- SmsOtpSenderJob#perform
UnusedPrivateMethod:
exclude:
- ApplicationController
Expand Down
25 changes: 17 additions & 8 deletions app/controllers/users/two_factor_authentication_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ def send_code
else
handle_invalid_otp_delivery_preference(result)
end
rescue Twilio::REST::RestError => exception
rescue Twilio::REST::RestError, PhoneVerification::VerifyError => exception
invalid_phone_number(exception)
end

Expand All @@ -51,17 +51,20 @@ def handle_invalid_otp_delivery_preference(result)
end

def invalid_phone_number(exception)
code = exception.code
analytics.track_event(
Analytics::TWILIO_PHONE_VALIDATION_FAILED, error: exception.message, code: exception.code
Analytics::TWILIO_PHONE_VALIDATION_FAILED, error: exception.message, code: code
)
flash_error_for_exception(exception)
flash[:error] = error_message(code)
redirect_back(fallback_location: account_url)
end

def flash_error_for_exception(exception)
flash[:error] = TwilioErrors::REST_ERRORS.fetch(
exception.code, t('errors.messages.otp_failed')
)
def error_message(code)
twilio_errors.fetch(code, t('errors.messages.otp_failed'))
end

def twilio_errors
TwilioErrors::REST_ERRORS.merge(TwilioErrors::VERIFY_ERRORS)
end

def otp_delivery_selection_form
Expand Down Expand Up @@ -94,7 +97,8 @@ def send_user_otp(method)
job = "#{method.capitalize}OtpSenderJob".constantize
job_priority = confirmation_context? ? :perform_now : :perform_later
job.send(job_priority, code: current_user.direct_otp, phone: phone_to_deliver_to,
otp_created_at: current_user.direct_otp_sent_at.to_s)
otp_created_at: current_user.direct_otp_sent_at.to_s,
locale: user_locale)
end

def user_selected_otp_delivery_preference
Expand All @@ -111,6 +115,11 @@ def phone_to_deliver_to
user_session[:unconfirmed_phone]
end

def user_locale
available_locales = PhoneVerification::AVAILABLE_LOCALES
http_accept_language.language_region_compatible_from(available_locales)
end

def otp_rate_limiter
@_otp_rate_limited ||= OtpRateLimiter.new(phone: phone_to_deliver_to, user: current_user)
end
Expand Down
10 changes: 10 additions & 0 deletions app/errors/twilio_errors.rb
Original file line number Diff line number Diff line change
Expand Up @@ -5,4 +5,14 @@ module TwilioErrors
21_215 => I18n.t('errors.messages.invalid_calling_area'),
21_614 => I18n.t('errors.messages.invalid_sms_number'),
}.freeze

VERIFY_ERRORS = {
60_033 => I18n.t('errors.messages.invalid_phone_number'),
# invalid country code
60_078 => I18n.t('errors.messages.invalid_phone_number'),
# cannot send sms to landline
60_082 => I18n.t('errors.messages.invalid_sms_number'),
# phone number not provisioned with any carrier
60_083 => I18n.t('errors.messages.invalid_phone_number'),
}.freeze
end
38 changes: 31 additions & 7 deletions app/jobs/sms_otp_sender_job.rb
Original file line number Diff line number Diff line change
@@ -1,19 +1,30 @@
class SmsOtpSenderJob < ApplicationJob
queue_as :sms

def perform(code:, phone:, otp_created_at:)
send_otp(TwilioService.new, code, phone) if otp_valid?(otp_created_at)
# rubocop:disable Lint/UnusedMethodArgument
def perform(code:, phone:, otp_created_at:, locale: nil)
return unless otp_valid?(otp_created_at)

if us_number?
send_sms_via_twilio_rest_api
else
send_sms_via_twilio_verify_api(locale)
end
end
# rubocop:enable Lint/UnusedMethodArgument

private

def otp_valid?(otp_created_at)
time_zone = Time.zone
time_zone.now < time_zone.parse(otp_created_at) + Devise.direct_otp_valid_for
def code
arguments[0][:code]
end

def send_otp(twilio_service, code, phone)
twilio_service.send_sms(
def phone
arguments[0][:phone]
end

def send_sms_via_twilio_rest_api
TwilioService.new.send_sms(
to: phone,
body: I18n.t(
'jobs.sms_otp_sender_job.message',
Expand All @@ -22,6 +33,19 @@ def send_otp(twilio_service, code, phone)
)
end

def send_sms_via_twilio_verify_api(locale)
PhoneVerification.new(phone: phone, locale: locale, code: code).send_sms
end

def us_number?
Phonelib.parse(phone).country == 'US'
end

def otp_valid?(otp_created_at)
time_zone = Time.zone
time_zone.now < time_zone.parse(otp_created_at) + Devise.direct_otp_valid_for
end

def otp_valid_for_minutes
Devise.direct_otp_valid_for.to_i / 60
end
Expand Down
83 changes: 83 additions & 0 deletions app/services/phone_verification.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,83 @@
class PhoneVerification
AUTHY_START_ENDPOINT = 'https://api.authy.com/protected/json/phones/verification/start'.freeze

HEADERS = { 'X-Authy-API-Key' => Figaro.env.twilio_verify_api_key }.freeze
OPEN_TIMEOUT = 5
READ_TIMEOUT = 5

AVAILABLE_LOCALES = %w[af ar ca zh zh-CN zh-HK hr cs da nl en fi fr de el he hi hu id it ja ko ms
nb pl pt-BR pt ro ru es sv tl th tr vi].freeze

cattr_accessor :adapter do
Typhoeus
end

def initialize(phone:, code:, locale: nil)
@phone = phone
@code = code
@locale = locale
end

def send_sms
raise VerifyError.new(code: error_code, message: error_message) unless start_request.success?
end

private

attr_reader :phone, :code, :locale

def error_code
response_body.fetch('error_code', nil).to_i
end

def error_message
response_body.fetch('message', '')
end

def response_body
@response_body ||= JSON.parse(start_request.response_body)
end

def start_request
@start_request ||= adapter.post(AUTHY_START_ENDPOINT, start_params)
end

# rubocop:disable Metrics/MethodLength
def start_params
{
headers: HEADERS,
body: {
code_length: 6,
country_code: country_code,
custom_code: code,
locale: locale,
phone_number: number_without_country_code,
via: 'sms',
},
connecttimeout: OPEN_TIMEOUT,
timeout: READ_TIMEOUT,
}
end
# rubocop:enable Metrics/MethodLength

def number_without_country_code
parsed_phone.raw_national
end

def parsed_phone
@parsed_phone ||= Phonelib.parse(phone)
end

def country_code
parsed_phone.country_code
end

class VerifyError < StandardError
attr_reader :code, :message

def initialize(code:, message:)
@code = code
@message = message
end
end
end
2 changes: 2 additions & 0 deletions config/application.yml.example
Original file line number Diff line number Diff line change
Expand Up @@ -296,6 +296,7 @@ production:
twilio_auth_token: # Twilio auth token
twilio_messaging_service_sid: # Twilio CoPilot SID
twilio_record_voice: 'false'
twilio_verify_api_key: 'change-me'
use_kms: 'true'
usps_confirmation_max_days: '30'
enable_i18n_mode: 'false'
Expand Down Expand Up @@ -411,6 +412,7 @@ test:
twilio_auth_token: 'token1'
twilio_messaging_service_sid: '123abc'
twilio_record_voice: 'true'
twilio_verify_api_key: 'secret'
use_kms: 'false'
usps_confirmation_max_days: '10'
enable_i18n_mode: 'false'
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -135,7 +135,8 @@ def index
expect(SmsOtpSenderJob).to have_received(:perform_later).with(
code: subject.current_user.direct_otp,
phone: subject.current_user.phone,
otp_created_at: subject.current_user.direct_otp_sent_at.to_s
otp_created_at: subject.current_user.direct_otp_sent_at.to_s,
locale: nil
)
expect(subject.current_user.direct_otp).not_to eq(@old_otp)
expect(subject.current_user.direct_otp).not_to be_nil
Expand Down Expand Up @@ -203,7 +204,8 @@ def index
expect(VoiceOtpSenderJob).to have_received(:perform_later).with(
code: subject.current_user.direct_otp,
phone: subject.current_user.phone,
otp_created_at: subject.current_user.direct_otp_sent_at.to_s
otp_created_at: subject.current_user.direct_otp_sent_at.to_s,
locale: nil
)
expect(subject.current_user.direct_otp).not_to eq(@old_otp)
expect(subject.current_user.direct_otp).not_to be_nil
Expand Down Expand Up @@ -251,7 +253,8 @@ def index
expect(SmsOtpSenderJob).to have_received(:perform_now).with(
code: subject.current_user.direct_otp,
phone: @unconfirmed_phone,
otp_created_at: subject.current_user.direct_otp_sent_at.to_s
otp_created_at: subject.current_user.direct_otp_sent_at.to_s,
locale: nil
)
end

Expand Down Expand Up @@ -312,7 +315,7 @@ def index
expect(flash[:error]).to eq(failed_to_send_otp)
end

it 'records an analytics event when Twilio responds with an error' do
it 'records an analytics event when Twilio responds with a RestError' do
stub_analytics
twilio_error = Twilio::REST::RestError.new(
'error message', FakeTwilioErrorResponse.new
Expand All @@ -337,6 +340,32 @@ def index

get :send_code, params: { otp_delivery_selection_form: { otp_delivery_preference: 'sms' } }
end

it 'records an analytics event when Twilio responds with a VerifyError' do
stub_analytics
code = 60_033
error_message = 'error'
verify_error = PhoneVerification::VerifyError.new(code: code, message: error_message)

allow(SmsOtpSenderJob).to receive(:perform_now).and_raise(verify_error)
analytics_hash = {
success: true,
errors: {},
otp_delivery_preference: 'sms',
resend: nil,
context: 'confirmation',
country_code: '1',
area_code: '202',
}

expect(@analytics).to receive(:track_event).
with(Analytics::OTP_DELIVERY_SELECTION, analytics_hash)

expect(@analytics).to receive(:track_event).
with(Analytics::TWILIO_PHONE_VALIDATION_FAILED, error: error_message, code: code)

get :send_code, params: { otp_delivery_selection_form: { otp_delivery_preference: 'sms' } }
end
end

context 'when selecting an invalid delivery method' do
Expand Down
6 changes: 4 additions & 2 deletions spec/features/two_factor_authentication/change_factor_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,8 @@
with(
code: user.reload.direct_otp,
phone: old_phone,
otp_created_at: user.reload.direct_otp_sent_at.to_s
otp_created_at: user.reload.direct_otp_sent_at.to_s,
locale: nil
)

expect(page).to have_content UserDecorator.new(user).masked_two_factor_phone_number
Expand All @@ -114,7 +115,8 @@
with(
code: user.reload.direct_otp,
phone: old_phone,
otp_created_at: user.reload.direct_otp_sent_at.to_s
otp_created_at: user.reload.direct_otp_sent_at.to_s,
locale: nil
)

expect(current_path).
Expand Down
41 changes: 41 additions & 0 deletions spec/features/two_factor_authentication/sign_in_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,28 @@
expect(phone_field.value).to eq('12345678901234567890')
end
end

context 'with SMS option, international number, and locale header' do
it 'passes locale to SmsOtpSenderJob' do
page.driver.header 'Accept-Language', 'ar'
PhoneVerification.adapter = FakeAdapter
allow(SmsOtpSenderJob).to receive(:perform_now)

user = sign_in_before_2fa
select_2fa_option('sms')
select 'Morocco', from: 'user_phone_form_international_code'
fill_in 'user_phone_form_phone', with: '6 61 28 93 24'
click_send_security_code

expect(SmsOtpSenderJob).to have_received(:perform_now).with(
code: user.reload.direct_otp,
phone: '+212 661-289324',
otp_created_at: user.direct_otp_sent_at.to_s,
locale: 'ar'
)
expect(current_path).to eq login_two_factor_path(otp_delivery_preference: 'sms')
end
end
end

def phone_field
Expand Down Expand Up @@ -515,6 +537,25 @@ def submit_prefilled_otp_code
expect(current_path).to eq login_two_factor_piv_cac_path
expect(page).to have_content(t('devise.two_factor_authentication.invalid_piv_cac'))
end

context 'with SMS, international number, and locale header' do
it 'passes locale to SmsOtpSenderJob' do
page.driver.header 'Accept-Language', 'ar'
PhoneVerification.adapter = FakeAdapter
allow(SmsOtpSenderJob).to receive(:perform_later)

user = create(:user, :signed_up, phone: '+212 661-289324')
sign_in_user(user)

expect(SmsOtpSenderJob).to have_received(:perform_later).with(
code: user.reload.direct_otp,
phone: '+212 661-289324',
otp_created_at: user.direct_otp_sent_at.to_s,
locale: 'ar'
)
expect(current_path).to eq login_two_factor_path(otp_delivery_preference: 'sms')
end
end
end

describe 'when the user is not piv/cac enabled' do
Expand Down
Loading