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
38 changes: 28 additions & 10 deletions app/controllers/users/phone_setup_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -5,16 +5,20 @@ class PhoneSetupController < ApplicationController
include PhoneConfirmation
include MfaSetupConcern
include RecaptchaConcern
include ReauthenticationRequiredConcern

before_action :authenticate_user
before_action :confirm_user_authenticated_for_2fa_setup
before_action :confirm_user_in_account_setup
before_action :set_setup_presenter
before_action :allow_csp_recaptcha_src, if: :recaptcha_enabled?
before_action :redirect_if_phone_vendor_outage
before_action :confirm_recently_authenticated_2fa
before_action :check_max_phone_numbers_per_account, only: %i[index create]

helper_method :in_multi_mfa_selection_flow?

def index
user_session[:phone_id] = nil
@new_phone_form = NewPhoneForm.new(
user: current_user,
analytics: analytics,
Expand Down Expand Up @@ -45,9 +49,13 @@ def recaptcha_enabled?

def track_phone_setup_visit
mfa_user = MfaContext.new(current_user)
analytics.user_registration_phone_setup_visit(
enabled_mfa_methods_count: mfa_user.enabled_mfa_methods_count,
)
if in_multi_mfa_selection_flow?
analytics.user_registration_phone_setup_visit(
enabled_mfa_methods_count: mfa_user.enabled_mfa_methods_count,
)
else
analytics.add_phone_setup_visit
end
end

def set_setup_presenter
Expand All @@ -70,13 +78,29 @@ def handle_create_success(phone)
phone: @new_phone_form.phone,
selected_delivery_method: @new_phone_form.otp_delivery_preference,
phone_type: @new_phone_form.phone_info&.type,
selected_default_number: @new_phone_form.otp_make_default_number,
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.

I noticed a slight difference from main, the selected_default_number parameter is now always present in the URL when confirming a number, and it's false when confirming the first phone number. I don't think this makes a meaningful impact, since I checked that in main, the absence of the parameter is effectively the same as it being false.

)
else
flash[:error] = t('errors.messages.phone_duplicate')
redirect_to phone_setup_url
end
end

def check_max_phone_numbers_per_account
max_phones_count = IdentityConfig.store.max_phone_numbers_per_account
return if current_user.phone_configurations.count < max_phones_count
flash[:phone_error] = t('users.phones.error_message')
redirect_path = request.referer.match(account_two_factor_authentication_url) ?
account_two_factor_authentication_url(anchor: 'phones') :
account_url(anchor: 'phones')
redirect_to redirect_path
end

def redirect_if_phone_vendor_outage
return unless OutageStatus.new.all_phone_vendor_outage?
redirect_to vendor_outage_path(from: :users_phones)
end

def new_phone_form_params
params.require(:new_phone_form).permit(
:phone,
Expand All @@ -88,11 +112,5 @@ def new_phone_form_params
:recaptcha_mock_score,
)
end

def confirm_user_in_account_setup
return if user_fully_authenticated? && in_multi_mfa_selection_flow?
return unless MfaPolicy.new(current_user).two_factor_enabled?
redirect_to account_path
end
end
end
2 changes: 1 addition & 1 deletion app/presenters/navigation_presenter.rb
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ def navigation_items
NavItem.new(
I18n.t('account.navigation.two_factor_authentication'),
account_two_factor_authentication_path, [
NavItem.new(I18n.t('account.navigation.add_phone_number'), add_phone_path),
NavItem.new(I18n.t('account.navigation.add_phone_number'), phone_setup_path),
NavItem.new(
I18n.t('account.navigation.add_authentication_apps'),
authenticator_setup_url,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ def redirect_location_step

def troubleshoot_change_phone_or_method_option
if unconfirmed_phone
BlockLinkComponent.new(url: add_phone_path).with_content(
BlockLinkComponent.new(url: phone_setup_path).with_content(
t('two_factor_authentication.phone_verification.troubleshooting.change_number'),
)
else
Expand Down
2 changes: 1 addition & 1 deletion app/views/accounts/_phone.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@
</div>
<% if current_user.phone_configurations.count < IdentityConfig.store.max_phone_numbers_per_account %>
<%= render ButtonComponent.new(
action: ->(**tag_options, &block) { link_to(add_phone_path, **tag_options, &block) },
action: ->(**tag_options, &block) { link_to(phone_setup_path, **tag_options, &block) },
outline: true,
icon: :add,
class: 'margin-top-2',
Expand Down
4 changes: 2 additions & 2 deletions app/views/users/phone_setup/index.html.erb
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
<% title t('titles.phone_setup') %>
<%= title t('titles.add_info.phone') %>

<%= render(VendorOutageAlertComponent.new(vendors: [:sms, :voice])) %>

<%= render PageHeadingComponent.new.with_content(t('titles.phone_setup')) %>
<%= render PageHeadingComponent.new.with_content(t('headings.add_info.phone')) %>

<p>
<%= t('two_factor_authentication.phone_info') %>
Expand Down
2 changes: 1 addition & 1 deletion app/views/users/phones/add.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@
<%= render CaptchaSubmitButtonComponent.new(
form: f,
action: PhoneRecaptchaValidator::RECAPTCHA_ACTION,
).with_content(t('forms.buttons.continue')) %>
).with_content(t('forms.buttons.send_one_time_code')) %>
<% end %>

<p class="margin-top-5">
Expand Down
1 change: 0 additions & 1 deletion config/locales/titles/en.yml
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,6 @@ en:
change: Change the password for your account
forgot: Reset password
personal_key: Just in case
phone_setup: Get your one-time code
piv_cac_login:
add: Add your PIV or CAC
new: Use your PIV/CAC to sign in to your account
Expand Down
1 change: 0 additions & 1 deletion config/locales/titles/es.yml
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,6 @@ es:
change: Cambie la contraseña de su cuenta
forgot: Restablecer la contraseña
personal_key: Por si acaso
phone_setup: Obtenga su código único
piv_cac_login:
add: Agregue su PIV o CAC
new: Use su PIV / CAC para iniciar sesión en su cuenta
Expand Down
1 change: 0 additions & 1 deletion config/locales/titles/fr.yml
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,6 @@ fr:
change: Changez le mot de passe de votre compte
forgot: Réinitialisez le mot de passe
personal_key: Juste au cas
phone_setup: Obtenez votre code à usage unique
piv_cac_login:
add: Ajoutez votre PIV ou CAC
new: Utilisez votre PIV / CAC pour vous connecter à votre compte
Expand Down
25 changes: 8 additions & 17 deletions spec/controllers/users/phone_setup_controller_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -17,11 +17,14 @@
end

context 'when signed in' do
it 'renders the index view' do
let(:user) { build(:user, otp_delivery_preference: 'voice') }
before do
stub_analytics
user = build(:user, otp_delivery_preference: 'voice')
stub_sign_in_before_2fa(user)
subject.user_session[:mfa_selections] = ['voice']
end

it 'renders the index view' do
expect(@analytics).to receive(:track_event).
with('User Registration: phone setup visited',
{ enabled_mfa_methods_count: 0 })
Expand All @@ -37,18 +40,6 @@
end
end

context 'when fully registered and signed in' do
it 'redirects to account page' do
stub_analytics
user = build(:user, :with_phone)
stub_sign_in(user)

get :index

expect(response).to redirect_to(account_path)
end
end

context 'when fully registered and partially signed in' do
it 'redirects to 2FA page' do
stub_analytics
Expand Down Expand Up @@ -152,7 +143,7 @@
expect(response).to redirect_to(
otp_send_path(
otp_delivery_selection_form: { otp_delivery_preference: 'voice',
otp_make_default_number: nil },
otp_make_default_number: false },
),
)

Expand Down Expand Up @@ -192,7 +183,7 @@
expect(response).to redirect_to(
otp_send_path(
otp_delivery_selection_form: { otp_delivery_preference: 'sms',
otp_make_default_number: nil },
otp_make_default_number: false },
),
)

Expand Down Expand Up @@ -231,7 +222,7 @@
expect(response).to redirect_to(
otp_send_path(
otp_delivery_selection_form: { otp_delivery_preference: 'sms',
otp_make_default_number: nil },
otp_make_default_number: false },
),
)

Expand Down
2 changes: 1 addition & 1 deletion spec/features/accessibility/user_pages_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -138,7 +138,7 @@
scenario 'add phone page' do
sign_in_and_2fa_user

visit add_phone_path
visit phone_setup_path

expect_page_to_have_no_accessibility_violations(page)
end
Expand Down
4 changes: 2 additions & 2 deletions spec/features/event_disavowal_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -43,13 +43,13 @@

scenario 'disavowing a phone being added' do
sign_in_and_2fa_user(user)
visit add_phone_path
visit phone_setup_path

fill_in 'new_phone_form[phone]', with: '202-555-3434'

choose 'new_phone_form_otp_delivery_preference_sms'
check 'new_phone_form_otp_make_default_number'
click_button t('forms.buttons.continue')
click_button t('forms.buttons.send_one_time_code')

submit_prefilled_otp_code(user, 'sms')

Expand Down
32 changes: 16 additions & 16 deletions spec/features/phone/add_phone_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@
click_on t('account.navigation.add_phone_number')
end
fill_in :new_phone_form_phone, with: phone
click_continue
click_send_one_time_code
fill_in_code_with_last_phone_otp
click_submit_default

Expand All @@ -30,7 +30,7 @@
click_on t('account.navigation.add_phone_number')
end
fill_in :new_phone_form_phone, with: phone
click_continue
click_send_one_time_code
fill_in_code_with_last_phone_otp
click_submit_default

Expand All @@ -51,7 +51,7 @@
hidden_select = page.find('[name="new_phone_form[international_code]"]', visible: :hidden)

# Required field should prompt as required on submit
click_continue
click_send_one_time_code
focused_input = page.find(':focus')
expect(focused_input).to match_css('.phone-input__number.usa-input--error')
expect(hidden_select.value).to eq('US')
Expand All @@ -66,7 +66,7 @@

# Invalid number should prompt as invalid on submit
fill_in :new_phone_form_phone, with: 'abcd1234'
click_continue
click_send_one_time_code
focused_input = page.find(':focus')
expect(focused_input).to match_css('.phone-input__number.usa-input--error')
expect(hidden_select.value).to eq('US')
Expand Down Expand Up @@ -102,7 +102,7 @@
expect(page).to_not have_content(t('two_factor_authentication.otp_delivery_preference.title'))
expect(hidden_select.value).to eq('LK')
fill_in :new_phone_form_phone, with: '+94 071 234 5678'
click_continue
click_send_one_time_code
expect(page.find(':focus')).to match_css('.phone-input__number')

# Switching to supported country should re-show delivery options, but prompt as invalid number
Expand All @@ -113,15 +113,15 @@
expect(page).to have_content(t('two_factor_authentication.otp_delivery_preference.title'))
expect(page).to have_css('.usa-error-message', text: '', visible: false)
expect(hidden_select.value).to eq('US')
click_continue
click_send_one_time_code
expect(page.find(':focus')).to match_css('.phone-input__number')
expect(page).to have_content(t('errors.messages.invalid_phone_number.us'))

# Entering valid number should allow submission
input = fill_in :new_phone_form_phone, with: '+81543543643'
expect(input.value).to eq('+81 543543643')
expect(hidden_select.value).to eq('JP')
click_continue
click_send_one_time_code
expect(page).to have_content(t('components.one_time_code_input.label'))
end

Expand Down Expand Up @@ -157,7 +157,7 @@
phone = phone_configuration.phone.sub(/^\+1\s*/, '').gsub(/\D/, '')

fill_in :new_phone_form_phone, with: phone
click_continue
click_send_one_time_code

expect(page).to have_content(I18n.t('errors.messages.phone_duplicate'))

Expand All @@ -179,7 +179,7 @@
click_on t('account.navigation.add_phone_number')
end
fill_in :new_phone_form_phone, with: telephony_gem_voip_number
click_continue
click_send_one_time_code
expect(page).to have_content(t('errors.messages.voip_check_error'))
end

Expand Down Expand Up @@ -218,7 +218,7 @@
# Failing international should display spam protection screen
fill_in t('two_factor_authentication.phone_label'), with: '3065550100'
fill_in t('components.captcha_submit_button.mock_score_label'), with: '0.5'
click_continue
click_send_one_time_code
expect(page).to have_content(t('titles.spam_protection'), wait: 5)
click_continue
expect(page).to have_content(t('two_factor_authentication.header_text'))
Expand All @@ -243,23 +243,23 @@
# Passing international should display OTP confirmation
fill_in t('two_factor_authentication.phone_label'), with: '3065550100'
fill_in t('components.captcha_submit_button.mock_score_label'), with: '0.7'
click_continue
click_send_one_time_code
expect(page).to have_content(t('two_factor_authentication.header_text'), wait: 25)
visit account_path
within('.sidenav') { click_on t('account.navigation.add_phone_number') }

# Failing domestic should display OTP confirmation
fill_in t('two_factor_authentication.phone_label'), with: '5135550100'
fill_in t('components.captcha_submit_button.mock_score_label'), with: '0.5'
click_continue
click_send_one_time_code
expect(page).to have_content(t('two_factor_authentication.header_text'), wait: 5)
visit account_path
within('.sidenav') { click_on t('account.navigation.add_phone_number') }

# Passing domestic should display OTP confirmation
fill_in t('two_factor_authentication.phone_label'), with: '5135550100'
fill_in t('components.captcha_submit_button.mock_score_label'), with: '0.7'
click_continue
click_send_one_time_code
expect(page).to have_content(t('two_factor_authentication.header_text'), wait: 5)
end

Expand All @@ -272,7 +272,7 @@
click_on t('account.navigation.add_phone_number')
end
fill_in :new_phone_form_phone, with: phone
click_continue
click_send_one_time_code
click_link t('links.cancel')

expect(page).to have_current_path(account_path)
Expand All @@ -290,9 +290,9 @@
end

fill_in :new_phone_form_phone, with: phone
click_continue
click_send_one_time_code
click_link t('two_factor_authentication.phone_verification.troubleshooting.change_number')

expect(page).to have_current_path(add_phone_path)
expect(page).to have_current_path(phone_setup_path)
end
end
2 changes: 1 addition & 1 deletion spec/features/phone/confirmation_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ def visit_otp_confirmation(delivery_method)
end
fill_in :new_phone_form_phone, with: phone
select_phone_delivery_option(delivery_method)
click_continue
click_send_one_time_code
end

def expect_successful_otp_confirmation(delivery_method)
Expand Down
Loading