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
1 change: 1 addition & 0 deletions app/controllers/concerns/saml_idp_auth_concern.rb
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ def add_sp_metadata_to_session
loa3: loa3_requested?,
request_id: @request_id,
request_url: request.original_url,
requested_attributes: requested_attributes,
}
end

Expand Down
7 changes: 6 additions & 1 deletion app/controllers/openid_connect/authorization_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,7 @@ def store_request
sp_request.issuer = @authorize_form.client_id
sp_request.loa = @authorize_form.acr_values.sort.max
sp_request.url = request.original_url
sp_request.requested_attributes = @authorize_decorator.requested_attributes
sp_request.requested_attributes = requested_attributes
end
end

Expand All @@ -134,7 +134,12 @@ def add_sp_metadata_to_session
loa3: @authorize_form.loa3_requested?,
request_id: @request_id,
request_url: request.original_url,
requested_attributes: requested_attributes,
}
end

def requested_attributes
@_attributes ||= @authorize_decorator.requested_attributes
end
end
end
3 changes: 2 additions & 1 deletion app/controllers/users/verify_account_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -9,9 +9,10 @@ def index

def create
@verify_account_form = build_verify_account_form

if @verify_account_form.submit
flash[:success] = t('account.index.verification.success')
redirect_to after_sign_in_path_for(current_user)
redirect_to sign_up_completed_url
else
render :index
end
Expand Down
2 changes: 1 addition & 1 deletion app/forms/verify_account_form.rb
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ def validate_pending_profile

def validate_otp
return if valid_otp?
errors.add :otp, :otp_incorrect
errors.add :otp, :confirmation_code_incorrect
end

def valid_otp?
Expand Down
17 changes: 17 additions & 0 deletions app/inputs/inline_input.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
class InlineInput < SimpleForm::Inputs::StringInput
def input(_wrapper_options)
input_html_classes.push('col-10 field monospace')
template.content_tag(
:div, builder.text_field(attribute_name, input_html_options),
class: 'col col-12 sm-col-4 mb4 sm-mb0'
)
end

def input_type
:text
end

def builder
@builder ||= :builder
end
end
7 changes: 5 additions & 2 deletions app/views/users/verify_account/index.html.slim
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,11 @@

h1.h3.my0 = t('forms.verify_profile.title')
p.mt-tiny.mb0 = t('forms.verify_profile.instructions')

= simple_form_for(@verify_account_form, url: verify_account_path,
html: { autocomplete: 'off', method: :post, role: 'form' }) do |f|
= f.error :base
= f.input :otp, required: true, label: 'Secret code'
= f.button :submit, t('forms.verify_profile.submit'), class: 'mb1'
= f.input :otp, required: true, label: t('forms.verify_profile.name'), wrapper: :inline_form do
= f.input_field :otp, as: :inline, autofocus: true, type: 'text', maxlength: '10'
= f.button :submit, t('forms.verify_profile.submit')
end
16 changes: 16 additions & 0 deletions config/initializers/simple_form.rb
Original file line number Diff line number Diff line change
@@ -1,9 +1,11 @@
# rubocop:disable Metrics/BlockLength
SimpleForm.setup do |config|
config.button_class = 'btn btn-primary'
config.boolean_label_class = nil
config.default_form_class = 'mt3 sm-mt4'
config.error_notification_tag = :div
config.error_notification_class = 'alert alert-error'
config.wrapper_mappings = { inline: :append }

config.wrappers :base do |b|
b.use :html5
Expand All @@ -23,5 +25,19 @@
b.use :error, wrap_with: { tag: 'div', class: 'mt-tiny h6 red error-message' }
end

config.wrappers :inline_form, tag: 'div' do |b|
b.use :label, class: 'bold block'
b.wrapper tag: 'div', class: 'col-12 clearfix' do |ba|
ba.use :input
end

b.wrapper tag: 'div' do |bb|
bb.use :error, wrap_with: { tag: 'span', class: 'mt-tiny h6 red error-message' }
end

b.optional :maxlength
end

config.default_wrapper = :vertical_form
end
# rubocop:enable Metrics/BlockLength
7 changes: 4 additions & 3 deletions config/locales/errors/en.yml
Original file line number Diff line number Diff line change
Expand Up @@ -9,12 +9,13 @@ en:
messages:
already_confirmed: was already confirmed, please try signing in
blank: Please fill in this field.
confirmation_code_incorrect: Incorrect code. Did you type it in correctly?
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we explicitly making a distinction between the USPS confirmation code and other security codes? For example, we have otp_incorrect: Security code is incorrect, but here we are changing the language. Is that intentional?

Copy link
Contributor Author

@el-mapache el-mapache May 11, 2017

Choose a reason for hiding this comment

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

I did change the language intentionally. The new designs, which I'll be linking in a moment, called for it.

confirmation_invalid_token: >
Invalid confirmation link. Either the link expired or you
already confirmed your account.
Invalid confirmation link. Either the link expired or you
already confirmed your account.
confirmation_period_expired: >
Expired confirmation link.
You can click "Resend confirmation instructions" to get another one.
You can click "Resend confirmation instructions" to get another one.
Copy link
Contributor

Choose a reason for hiding this comment

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

I recently learned about a neat trick where you can reference an existing translation inside another one. Since "Resend confirmation instructions" is defined in idv.messages.usps.send_again, you can refer to it by add a colon before it, like this:

You can click :idv.messages.usps.send_again to get another one.

Magic!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No way!! I was thinking it was odd that there wasn't anyway to reuse these strings.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I actually can't get this to work. From previous searches I've seen using & as an alias, but I can't find references to prefixing a translation key with a : to share them.

Copy link
Contributor

Choose a reason for hiding this comment

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

Strange. It worked for me. I tested by running a spec that loads this page and I added a screenshot_and_save_page, and I saw the translated text.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

and you put in exactly what you pasted here? I see the string as presented here, with the : prefix, uninterpolated

Copy link
Contributor

Choose a reason for hiding this comment

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

My bad. I was looking at the wrong page. I just realized this change is not related to this PR. It looks like it's just cleaning up whitespace. It turns out that to use this magic trick, the only thing you can put as the value is the key you are referencing, like this:

confirmation_period_expired: :idv.messages.usps.send_again

So, it doesn't look like we can use it here 😢

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh bummer 😞

expired: has expired, please request a new one
format_mismatch: Please match the requested format.
improbable_phone: Invalid phone number. Please make sure you enter a 10-digit phone number.
Expand Down
1 change: 1 addition & 0 deletions config/locales/errors/es.yml
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ es:
messages:
already_confirmed: Ya está confirmado, por favor intenta iniciar sesión.
blank: Por favor, rellenar este campo.
confirmation_code_incorrect: NOT TRANSLATED YET
confirmation_invalid_token:
El enlace de confirmación en el que ha hecho clic ya no es válido. Esto puede haber
sido causado al hacer clic en un enlace antiguo de su correo electrónico, o puede ser que ya haya
Expand Down
7 changes: 4 additions & 3 deletions config/locales/forms/en.yml
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ en:
personal_key: Personal key
try_again: Use another phone number
verify_profile:
title: Verify your identity
submit: Submit
instructions: Enter your secret code
name: Confirmation code
instructions: Enter the ten-character code in the letter we sent you.
submit: Confirm account
title: Confirm your account
1 change: 1 addition & 0 deletions config/locales/forms/es.yml
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ es:
personal_key: Código de recuperación
try_again: Inténtalo de nuevo
verify_profile:
name: NOT TRANSLATED YET
title: NOT TRANSLATED YET
submit: NOT TRANSLATED YET
instructions: NOT TRANSLATED YET
3 changes: 2 additions & 1 deletion config/locales/idv/en.yml
Original file line number Diff line number Diff line change
Expand Up @@ -154,7 +154,8 @@ en:
no_pii: Do not use real personal information (demo purposes only)
usps:
bad_address: I can't get mail at this address
byline: To activate by mail, we will mail a letter with a confirmation code to your street address.
byline: We will mail a letter with a confirmation code to your verified
address on file.
success: It should arrive in 5 to 10 business days.
personal_details_verified: Personal details verified!
modal:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,8 @@
loa3: false,
issuer: 'urn:gov:gsa:openidconnect:test',
request_id: sp_request_id,
request_url: request.original_url
request_url: request.original_url,
requested_attributes: %w[given_name family_name birthdate]
)
end
end
Expand Down
3 changes: 2 additions & 1 deletion spec/controllers/saml_idp_controller_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -295,7 +295,8 @@
loa3: false,
issuer: saml_settings.issuer,
request_id: sp_request_id,
request_url: @saml_request.request.original_url
request_url: @saml_request.request.original_url,
requested_attributes: [:email]
)
end

Expand Down
6 changes: 4 additions & 2 deletions spec/controllers/users/verify_account_controller_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -54,10 +54,12 @@
end

context 'with a valid form' do
it 'redirects to the account verification page' do
let(:success) { true }

it 'redirects to the sign_up/completions page' do
action

expect(response).to redirect_to(verify_account_path)
expect(response).to redirect_to(sign_up_completed_url)
end
end

Expand Down
23 changes: 0 additions & 23 deletions spec/features/idv/flow_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -346,29 +346,6 @@
end
end

scenario 'pick USPS address verification' do
sign_in_and_2fa_user
visit verify_session_path

fill_out_idv_form_ok
click_idv_continue
fill_out_financial_form_ok
click_idv_continue
click_idv_address_choose_usps
click_on t('idv.buttons.send_letter')

expect(current_path).to eq verify_review_path

fill_in :user_password, with: user_password
click_submit_default

expect(current_url).to eq verify_confirmations_url
click_acknowledge_personal_key

expect(current_url).to eq(account_url)
expect(page).to have_content(t('account.index.verification.reactivate_button'))
end

context 'cancel from USPS/Phone verification screen' do
context 'without js' do
it 'returns user to profile path' do
Expand Down
7 changes: 5 additions & 2 deletions spec/features/openid_connect/openid_connect_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -285,11 +285,14 @@

sign_in_live_with_2fa(user)

fill_in 'Secret code', with: otp
fill_in t('forms.verify_profile.name'), with: usps_otp_code_for(user)
click_button t('forms.verify_profile.submit')
click_button t('openid_connect.authorization.index.allow')

expect(current_path).to eq(sign_up_completed_path)
find('input').click
click_button t('openid_connect.authorization.index.allow')
redirect_uri = URI(current_url)

expect(redirect_uri.to_s).to start_with('http://localhost:7654/auth/result')
end
end
Expand Down
62 changes: 46 additions & 16 deletions spec/features/saml/loa3_sso_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -5,22 +5,23 @@
include IdvHelper

context 'First time registration' do
it 'redirects to original SAML Authn Request after IdV is complete', email: true do
let(:email) { 'test@test.com' }
before do
allow(FeatureManagement).to receive(:prefill_otp_codes?).and_return(true)
saml_authn_request = auth_request.create(loa3_with_bundle_saml_settings)
@saml_authn_request = auth_request.create(loa3_with_bundle_saml_settings)
end

it 'redirects to original SAML Authn Request after IdV is complete', email: true do
xmldoc = SamlResponseDoc.new('feature', 'response_assertion')
email = 'test@test.com'

visit saml_authn_request
click_link t('sign_up.registrations.create_account')
submit_form_with_valid_email
click_confirmation_link_in_email(email)
submit_form_with_valid_password
set_up_2fa_with_valid_phone
enter_2fa_code
visit @saml_authn_request

saml_register_loa3_user(email)

expect(current_path).to eq verify_path
click_on 'Yes'

click_idv_begin

user = User.find_with_email(email)
complete_idv_profile_ok(user.reload)
click_acknowledge_personal_key
Expand All @@ -41,14 +42,42 @@
end

click_on I18n.t('forms.buttons.continue')
expect(current_url).to eq saml_authn_request
expect(current_url).to eq @saml_authn_request

user_access_key = user.unlock_user_access_key(Features::SessionHelper::VALID_PASSWORD)
profile_phone = user.active_profile.decrypt_pii(user_access_key).phone

expect(xmldoc.phone_number.children.children.to_s).to eq(profile_phone)
end

it 'allows the user to select verification via USPS letter', email: true do
visit @saml_authn_request

saml_register_loa3_user(email)

click_idv_begin

fill_out_idv_form_ok
click_idv_continue
fill_out_financial_form_ok
click_idv_continue

click_idv_address_choose_usps

click_on t('idv.buttons.send_letter')

expect(current_path).to eq verify_review_path

fill_in :user_password, with: user_password
click_submit_default

expect(current_url).to eq verify_confirmations_url
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you mean to stop the test here? Don't we want to test all the way through to seeing the completions page and then continuing to the SP?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I stopped it because the user will never be able to go all the way through the flow when they first register. There is a second test beginning at line 141 that checks that the user is able to return and finish the verification process.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure, I get that, but they are still able to do more on the site, right? Once on the verify/confirmations page, they will acknowledge their personal key, and after that, they should be sent to the account page, where they should see a message about needing to confirm their USPS code.

I suggest we merge #1424 (I approved it last night. It just needs to be squashed), then rebase this PR against master so we can properly test the whole flow as the user would experience it.

click_acknowledge_personal_key

expect(current_url).to eq(account_url)
expect(page).to have_content(t('account.index.verification.reactivate_button'))
end

it 'shows user the start page with accordion' do
saml_authn_request = auth_request.create(loa3_with_bundle_saml_settings)
sp_content = [
Expand Down Expand Up @@ -130,18 +159,19 @@
context 'having previously selected USPS verification' do
let(:phone_confirmed) { false }

it 'prompts for OTP at sign in' do
it 'prompts for confirmation code at sign in' do
saml_authn_request = auth_request.create(loa3_with_bundle_saml_settings)

visit saml_authn_request

sign_in_live_with_2fa(user)

expect(current_path).to eq verify_account_path

fill_in 'Secret code', with: otp
fill_in t('forms.verify_profile.name'), with: usps_otp_code_for(user)
click_button t('forms.verify_profile.submit')

expect(current_path).to eq(sign_up_completed_path)
find('input').click

expect(current_url).to eq saml_authn_request
end
end
Expand Down
6 changes: 3 additions & 3 deletions spec/features/users/verify_profile_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@

expect(current_path).to eq verify_account_path

fill_in 'Secret code', with: otp
fill_in t('forms.verify_profile.name'), with: otp
click_button t('forms.verify_profile.submit')

expect(current_path).to eq account_path
Expand All @@ -35,11 +35,11 @@

scenario 'wrong OTP used' do
sign_in_live_with_2fa(user)
fill_in 'Secret code', with: 'the wrong code'
fill_in t('forms.verify_profile.name'), with: 'the wrong code'
click_button t('forms.verify_profile.submit')

expect(current_path).to eq verify_account_path
expect(page).to have_content(t('errors.messages.otp_incorrect'))
expect(page).to have_content(t('errors.messages.confirmation_code_incorrect'))
expect(page.body).to_not match('the wrong code')
end
end
Expand Down
2 changes: 1 addition & 1 deletion spec/forms/verify_account_form_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@

it 'is invalid' do
expect(subject).to_not be_valid
expect(subject.errors[:otp]).to eq [t('errors.messages.otp_incorrect')]
expect(subject.errors[:otp]).to eq [t('errors.messages.confirmation_code_incorrect')]
end
end
end
Expand Down
Loading