From 74cab8ac50ebf6b37e31afe09b5ba1509ba82f02 Mon Sep 17 00:00:00 2001 From: Andrew Duthie Date: Wed, 24 May 2023 15:28:12 -0400 Subject: [PATCH 01/20] LG-9709: Add autocomplete attribute to password fields (#8466) * Add autocomplete attribute to password fields changelog: User-Facing Improvements, Passwords, Improve password autocomplete support * Add preview for PasswordConfirmationComponent * Put input_html in the correct place (field_options) --- .../password_confirmation_component.html.erb | 4 +++- .../password_confirmation_component.scss | 3 +++ app/views/devise/passwords/edit.html.erb | 3 +++ app/views/devise/sessions/new.html.erb | 7 ++++++- app/views/event_disavowal/new.html.erb | 3 +++ app/views/idv/review/new.html.erb | 3 +++ app/views/mfa_confirmation/new.html.erb | 10 +++++++++- app/views/users/delete/show.html.erb | 3 +++ app/views/users/passwords/edit.html.erb | 5 ++++- app/views/users/verify_password/new.html.erb | 3 +++ .../password_confirmation_component_spec.rb | 15 +++++++++++++++ ...password_confirmation_component_preview.rb | 19 +++++++++++++++++++ 12 files changed, 74 insertions(+), 4 deletions(-) create mode 100644 app/components/password_confirmation_component.scss create mode 100644 spec/components/password_confirmation_component_spec.rb create mode 100644 spec/components/previews/password_confirmation_component_preview.rb diff --git a/app/components/password_confirmation_component.html.erb b/app/components/password_confirmation_component.html.erb index 0054091e06d..4c50243d3b3 100644 --- a/app/components/password_confirmation_component.html.erb +++ b/app/components/password_confirmation_component.html.erb @@ -8,6 +8,7 @@ **field_options, input_html: field_options[:input_html].to_h.merge( id: input_id, + autocomplete: 'new-password', class: ['password-confirmation__input', *field_options.dig(:input_html, :class)], ), ) %> @@ -20,6 +21,7 @@ **field_options, input_html: field_options[:input_html].to_h.merge( id: input_confirmation_id, + autocomplete: 'new-password', class: ['password-confirmation__input-confirmation', *field_options.dig(:input_html, :class)], ), wrapper_html: field_options[:wrapper_html].to_h.merge( @@ -28,7 +30,7 @@ error_messages: { valueMissing: t('components.password_confirmation.errors.empty'), }, - ) %> + ) %> <%= render 'devise/shared/password_strength', forbidden_passwords: @forbidden_passwords %> diff --git a/app/views/devise/sessions/new.html.erb b/app/views/devise/sessions/new.html.erb index bc19b2519c8..ef47fbf82f6 100644 --- a/app/views/devise/sessions/new.html.erb +++ b/app/views/devise/sessions/new.html.erb @@ -46,7 +46,12 @@ <%= render PasswordToggleComponent.new( form: f, class: 'margin-bottom-4', - field_options: { required: true }, + field_options: { + required: true, + input_html: { + autocomplete: 'current-password', + }, + }, ) %> <%= f.submit t('links.next'), full_width: true, wide: false %> <% if @sign_in_a_b_test_bucket == :default %> diff --git a/app/views/event_disavowal/new.html.erb b/app/views/event_disavowal/new.html.erb index 6a93cc4e865..3f35824c546 100644 --- a/app/views/event_disavowal/new.html.erb +++ b/app/views/event_disavowal/new.html.erb @@ -15,6 +15,9 @@ field_options: { label: t('forms.passwords.edit.labels.password'), required: true, + input_html: { + autocomplete: 'new-password', + }, }, ) %> <%= render 'devise/shared/password_strength', forbidden_passwords: @forbidden_passwords %> diff --git a/app/views/idv/review/new.html.erb b/app/views/idv/review/new.html.erb index fe72a284986..8734c6a298c 100644 --- a/app/views/idv/review/new.html.erb +++ b/app/views/idv/review/new.html.erb @@ -30,6 +30,9 @@ field_options: { label: t('idv.form.password'), required: true, + input_html: { + autocomplete: 'current-password', + }, }, ) %>
diff --git a/app/views/mfa_confirmation/new.html.erb b/app/views/mfa_confirmation/new.html.erb index c00b61584ff..1492e7d7323 100644 --- a/app/views/mfa_confirmation/new.html.erb +++ b/app/views/mfa_confirmation/new.html.erb @@ -12,7 +12,15 @@ url: reauthn_user_password_path, html: { autocomplete: 'off', method: 'post', class: 'margin-top-4' }, ) do |f| %> - <%= render PasswordToggleComponent.new(form: f, field_options: { required: true }) %> + <%= render PasswordToggleComponent.new( + form: f, + field_options: { + required: true, + input_html: { + autocomplete: 'current-password', + }, + }, + ) %> <%= f.submit t('forms.buttons.continue'), class: 'display-block margin-y-5' %> <% end %> <%= render 'shared/cancel', link: account_path %> diff --git a/app/views/users/delete/show.html.erb b/app/views/users/delete/show.html.erb index 3e15de0aeda..82483268f77 100644 --- a/app/views/users/delete/show.html.erb +++ b/app/views/users/delete/show.html.erb @@ -28,6 +28,9 @@ name: :password, label: t('idv.form.password'), required: true, + input_html: { + autocomplete: 'current-password', + }, }, ) %> diff --git a/app/views/users/passwords/edit.html.erb b/app/views/users/passwords/edit.html.erb index 10e4ee7c12a..926bd819196 100644 --- a/app/views/users/passwords/edit.html.erb +++ b/app/views/users/passwords/edit.html.erb @@ -17,7 +17,10 @@ name: :password, label: t('forms.passwords.edit.labels.password'), required: true, - input_html: { aria: { describedby: 'password-description' } }, + input_html: { + aria: { describedby: 'password-description' }, + autocomplete: 'new-password', + }, }, ) %> <%= render 'devise/shared/password_strength', forbidden_passwords: @forbidden_passwords %> diff --git a/app/views/users/verify_password/new.html.erb b/app/views/users/verify_password/new.html.erb index b3d8de22f83..a8fa3b8e44d 100644 --- a/app/views/users/verify_password/new.html.erb +++ b/app/views/users/verify_password/new.html.erb @@ -16,6 +16,9 @@ name: :password, label: t('idv.form.password'), required: true, + input_html: { + autocomplete: 'current-password', + }, }, ) %> <%= f.submit t('forms.buttons.continue'), class: 'margin-top-5' %> diff --git a/spec/components/password_confirmation_component_spec.rb b/spec/components/password_confirmation_component_spec.rb new file mode 100644 index 00000000000..54f52b3f631 --- /dev/null +++ b/spec/components/password_confirmation_component_spec.rb @@ -0,0 +1,15 @@ +require 'rails_helper' + +RSpec.describe PasswordConfirmationComponent, type: :component do + let(:lookup_context) { ActionView::LookupContext.new(ActionController::Base.view_paths) } + let(:view_context) { ActionView::Base.new(lookup_context, {}, controller) } + let(:form) { SimpleForm::FormBuilder.new('', {}, view_context, {}) } + + subject(:rendered) do + render_inline PasswordConfirmationComponent.new(form:) + end + + it 'renders password fields with expected attributes' do + expect(rendered).to have_css('[type=password][autocomplete=new-password]', count: 2) + end +end diff --git a/spec/components/previews/password_confirmation_component_preview.rb b/spec/components/previews/password_confirmation_component_preview.rb new file mode 100644 index 00000000000..d0421e2aa93 --- /dev/null +++ b/spec/components/previews/password_confirmation_component_preview.rb @@ -0,0 +1,19 @@ +class PasswordConfirmationComponentPreview < BaseComponentPreview + # @!group Preview + # @display form true + def default + render(PasswordConfirmationComponent.new(form: form_builder)) + end + # @!endgroup + + # @display form true + # @param toggle_label text + def workbench(toggle_label: nil) + render( + PasswordConfirmationComponent.new( + form: form_builder, + **{ toggle_label: }.compact, + ), + ) + end +end From e66247183686059bb83a15b57fe435826967ab16 Mon Sep 17 00:00:00 2001 From: Andrew Duthie Date: Wed, 24 May 2023 16:40:15 -0400 Subject: [PATCH 02/20] Fix spinner button animation for long action text (#8472) * Fix spinner button animation for long action text changelog: Bug Fixes, UI Components, Fix appearance of button spinner in specific scenarios * Sync SpinnerButton specs to markup revisions * Bump CodeClimate * Trim all whitespace * Add outline options to SpinnerButton previews Since we explicitly handle outline button as part of the spinner button implementation * Update SpinnerButton markup for React component Revert stylesheet to main application stylesheet, since currently there's not a way to auto-load component stylesheets for components implemented in React * Use String#lstrip instead of String#sub See: https://github.com/18F/identity-idp/pull/8472/files#r1204466631 Co-authored-by: Zach Margolis --------- Co-authored-by: Zach Margolis --- .../components/_spinner-button.scss | 37 +++---------- app/components/button_component.html.erb | 2 +- app/components/button_component.rb | 14 +++++ .../spinner_button_component.html.erb | 6 +-- .../spinner-button-element.spec.ts | 53 ++++++++----------- .../spinner-button/spinner-button-element.ts | 3 +- .../spinner-button/spinner-button.tsx | 7 +-- spec/components/button_component_spec.rb | 26 +++++++++ .../spinner_button_component_preview.rb | 26 +++++++-- 9 files changed, 100 insertions(+), 74 deletions(-) diff --git a/app/assets/stylesheets/components/_spinner-button.scss b/app/assets/stylesheets/components/_spinner-button.scss index 5d931537a15..887f7eb13c3 100644 --- a/app/assets/stylesheets/components/_spinner-button.scss +++ b/app/assets/stylesheets/components/_spinner-button.scss @@ -1,44 +1,21 @@ @use 'uswds-core' as *; lg-spinner-button { - display: block; - - @include at-media('mobile-lg') { - display: inline-block; + .usa-button { + position: relative; } &:not(.spinner-button--spinner-active) .spinner-dots { display: none; } -} - -.spinner-button--spinner-active { - .spinner-button__content { - position: relative; - a, - button:not([type]), - [type='submit'], - [type='button'], - .usa-button:disabled.usa-button--active:not(.usa-button--unstyled, .usa-button--secondary, .usa-button--accent-cool, .usa-button--accent-warm, .usa-button--base, .usa-button--outline, .usa-button--inverse), - .usa-button--disabled.usa-button--active:not(.usa-button--unstyled, .usa-button--secondary, .usa-button--accent-cool, .usa-button--accent-warm, .usa-button--base, .usa-button--outline, .usa-button--inverse) { - color: transparent; - opacity: 1; - } + &:not(.spinner-button--outline) .spinner-dots { + color: color('white'); } +} - &:not(.spinner-button--outline) .spinner-button__content { - a, - button:not([type]), - [type='submit'], - [type='button'] { - background-color: color('primary-darker'); - } - - .spinner-dots { - color: color('white'); - } - } +.spinner-button--spinner-active .spinner-button__content { + opacity: 0; } .spinner-button__action-message { diff --git a/app/components/button_component.html.erb b/app/components/button_component.html.erb index 4a8193d1c42..a3063a3e3c5 100644 --- a/app/components/button_component.html.erb +++ b/app/components/button_component.html.erb @@ -1,4 +1,4 @@ <%= action.call( **tag_options, class: css_class, - ) { safe_join([icon_content, content&.strip].compact) } %> + ) { safe_join([icon_content, content].compact) } %> diff --git a/app/components/button_component.rb b/app/components/button_component.rb index 188ad67148d..b2c60bd4a06 100644 --- a/app/components/button_component.rb +++ b/app/components/button_component.rb @@ -37,4 +37,18 @@ def css_class def icon_content render IconComponent.new(icon:) if icon end + + def content + original_content = super + if original_content.present? && icon.present? + # Content templates may include leading whitespace, which interferes with the layout when an + # icon is present. This can be solved in CSS using Flexbox, but doing so for all buttons may + # have unintended consequences. + trimmed_content = original_content.lstrip + trimmed_content = sanitize(trimmed_content) if original_content.html_safe? + trimmed_content + else + original_content + end + end end diff --git a/app/components/spinner_button_component.html.erb b/app/components/spinner_button_component.html.erb index d8be4e272b3..8c02b6c5890 100644 --- a/app/components/spinner_button_component.html.erb +++ b/app/components/spinner_button_component.html.erb @@ -1,12 +1,12 @@ <%= content_tag(:'lg-spinner-button', class: css_class, 'spin-on-click': spin_on_click) do %> - + <% end %> <% if action_message %> <%= tag.div '', role: 'status', diff --git a/app/javascript/packages/spinner-button/spinner-button-element.spec.ts b/app/javascript/packages/spinner-button/spinner-button-element.spec.ts index 12e132e0d1b..c6e78809351 100644 --- a/app/javascript/packages/spinner-button/spinner-button-element.spec.ts +++ b/app/javascript/packages/spinner-button/spinner-button-element.spec.ts @@ -13,28 +13,24 @@ describe('SpinnerButtonElement', () => { interface WrapperOptions { actionMessage?: string; - tagName?: string; spinOnClick?: boolean; inForm?: boolean; isButtonTo?: boolean; } - function createWrapper({ - actionMessage, - tagName = 'a', - spinOnClick, - inForm, - isButtonTo, - }: WrapperOptions = {}) { - let tag; - if (tagName === 'a') { - tag = 'Click Me'; - } else { - tag = ''; - } + function createWrapper({ actionMessage, spinOnClick, inForm, isButtonTo }: WrapperOptions = {}) { + let button = ` + `; if (isButtonTo) { - tag = `
${tag}
`; + button = `
${button}
`; } let html = ` @@ -42,14 +38,7 @@ describe('SpinnerButtonElement', () => { long-wait-duration-ms="${longWaitDurationMs}" ${spinOnClick === undefined ? '' : `spin-on-click="${spinOnClick}"`} > - + ${button} ${ actionMessage ? `
{ it('shows spinner on click', async () => { const wrapper = createWrapper(); - const button = screen.getByRole('link', { name: 'Click Me' }); + const button = screen.getByRole('button', { name: 'Click Me' }); await userEvent.click(button); @@ -80,7 +69,7 @@ describe('SpinnerButtonElement', () => { context('inside form', () => { it('disables button without preventing form handlers', async () => { - const wrapper = createWrapper({ tagName: 'button', inForm: true }); + const wrapper = createWrapper({ inForm: true }); let didSubmit = false; wrapper.form!.addEventListener('submit', (event) => { didSubmit = true; @@ -96,7 +85,7 @@ describe('SpinnerButtonElement', () => { }); it('unbinds events when disconnected', () => { - const wrapper = createWrapper({ tagName: 'button', inForm: true }); + const wrapper = createWrapper({ inForm: true }); const form = wrapper.form!; form.removeChild(wrapper); @@ -109,7 +98,7 @@ describe('SpinnerButtonElement', () => { context('with form inside (button_to)', () => { it('disables button without preventing form handlers', async () => { - const wrapper = createWrapper({ tagName: 'button', isButtonTo: true }); + const wrapper = createWrapper({ isButtonTo: true }); let didSubmit = false; wrapper.form!.addEventListener('submit', (event) => { didSubmit = true; @@ -126,7 +115,7 @@ describe('SpinnerButtonElement', () => { }); it('does not show spinner if form is invalid', async () => { - const wrapper = createWrapper({ tagName: 'button', inForm: true }); + const wrapper = createWrapper({ inForm: true }); const form = wrapper.closest('form')!; const input = document.createElement('input'); input.required = true; @@ -141,7 +130,7 @@ describe('SpinnerButtonElement', () => { it('announces action message', async () => { const wrapper = createWrapper({ actionMessage: 'Verifying...' }); const status = getByRole(wrapper, 'status'); - const button = screen.getByRole('link', { name: 'Click Me' }); + const button = screen.getByRole('button', { name: 'Click Me' }); expect(status.textContent).to.be.empty(); @@ -154,7 +143,7 @@ describe('SpinnerButtonElement', () => { it('shows action message visually after long delay', async () => { const wrapper = createWrapper({ actionMessage: 'Verifying...' }); const status = getByRole(wrapper, 'status'); - const button = screen.getByRole('link', { name: 'Click Me' }); + const button = screen.getByRole('button', { name: 'Click Me' }); expect(status.textContent).to.be.empty(); @@ -176,7 +165,7 @@ describe('SpinnerButtonElement', () => { it('supports disabling default spin on click behavior', async () => { const wrapper = createWrapper({ spinOnClick: false }); - const button = screen.getByRole('link', { name: 'Click Me' }); + const button = screen.getByRole('button', { name: 'Click Me' }); await userEvent.click(button); @@ -185,7 +174,7 @@ describe('SpinnerButtonElement', () => { it('removes action message timeout when disconnected from the page', async () => { const wrapper = createWrapper({ actionMessage: 'Verifying...' }); - const button = screen.getByRole('link', { name: 'Click Me' }); + const button = screen.getByRole('button', { name: 'Click Me' }); sandbox.spy(window, 'setTimeout'); sandbox.spy(window, 'clearTimeout'); diff --git a/app/javascript/packages/spinner-button/spinner-button-element.ts b/app/javascript/packages/spinner-button/spinner-button-element.ts index 902f1a5a3ae..f6f83cf16f6 100644 --- a/app/javascript/packages/spinner-button/spinner-button-element.ts +++ b/app/javascript/packages/spinner-button/spinner-button-element.ts @@ -42,7 +42,7 @@ export class SpinnerButtonElement extends HTMLElement { } get button(): HTMLElement { - return this.querySelector('a,button:not([type]),[type="submit"],[type="button"]')!; + return this.querySelector('.usa-button')!; } get actionMessage(): HTMLElement { @@ -64,6 +64,7 @@ export class SpinnerButtonElement extends HTMLElement { toggleSpinner(isVisible: boolean) { this.classList.toggle('spinner-button--spinner-active', isVisible); + this.button.classList.toggle('usa-button--active', isVisible); if (isVisible) { this.button.setAttribute('disabled', ''); diff --git a/app/javascript/packages/spinner-button/spinner-button.tsx b/app/javascript/packages/spinner-button/spinner-button.tsx index 3353b59bd40..aec9995a170 100644 --- a/app/javascript/packages/spinner-button/spinner-button.tsx +++ b/app/javascript/packages/spinner-button/spinner-button.tsx @@ -39,6 +39,7 @@ function SpinnerButton( actionMessage, longWaitDurationMs, isOutline, + children, ...buttonProps }: SpinnerButtonProps, ref: ForwardedRef, @@ -55,14 +56,14 @@ function SpinnerButton( ref={elementRef} class={classes} > -
-
+ {actionMessage && (
Button') + end + end + + context 'with content including whitespace and unsafe html' do + let(:content) { safe_join([" \n ", 'Button']) } + + it 'trims text of the content, maintaining html safety' do + rendered = render_inline ButtonComponent.new(icon: :print).with_content(content) + + expect(rendered.to_html).to include('<span class="example">Button</span>') + end + end + + context 'with no content' do + it 'renders without error' do + render_inline ButtonComponent.new(icon: :print) + end + end end context 'with custom button action' do diff --git a/spec/components/previews/spinner_button_component_preview.rb b/spec/components/previews/spinner_button_component_preview.rb index 679a39010a7..5ec440b84ff 100644 --- a/spec/components/previews/spinner_button_component_preview.rb +++ b/spec/components/previews/spinner_button_component_preview.rb @@ -4,7 +4,7 @@ def default render(SpinnerButtonComponent.new(big: true).with_content('Submit')) end - def action_message + def with_action_message render( SpinnerButtonComponent.new( big: true, @@ -12,15 +12,33 @@ def action_message ).with_content('Submit'), ) end + + def outline + render(SpinnerButtonComponent.new(big: true, outline: true).with_content('Submit')) + end + + def outline_with_action_message + render( + SpinnerButtonComponent.new( + big: true, + outline: true, + action_message: 'Verifying…', + ).with_content('Submit'), + ) + end # @!endgroup - # @display form true # @param action_message text - def workbench(action_message: nil) + # @param outline toggle + # @param wide toggle + # @param full_width toggle + def workbench(action_message: nil, outline: false, wide: false, full_width: false) render( SpinnerButtonComponent.new( - form: form_builder, big: true, + outline:, + wide:, + full_width:, **{ action_message: }.compact, ).with_content('Submit'), ) From 47a39265a4c4ba2678d2035eba1d4aa5582b6f8d Mon Sep 17 00:00:00 2001 From: Matt Wagner Date: Wed, 24 May 2023 16:40:52 -0400 Subject: [PATCH 03/20] Adds make tidy (#8439) [skip changelog] --- Makefile | 25 +++++++++++++++++++++++++ bin/setup | 15 +++------------ 2 files changed, 28 insertions(+), 12 deletions(-) diff --git a/Makefile b/Makefile index 151327b2445..08925bfd1fa 100644 --- a/Makefile +++ b/Makefile @@ -15,6 +15,9 @@ ARTIFACT_DESTINATION_FILE ?= ./tmp/idp.tar.gz brakeman \ build_artifact \ check \ + clobber_db \ + clobber_assets \ + clobber_logs \ docker_setup \ download_acuant_sdk \ fast_setup \ @@ -34,6 +37,7 @@ ARTIFACT_DESTINATION_FILE ?= ./tmp/idp.tar.gz optimize_assets \ optimize_svg \ run \ + tidy \ update \ urn \ README.md \ @@ -268,3 +272,24 @@ README.md: docs/ ## Generates README.md based on the contents of the docs direct download_acuant_sdk: ## Downloads the most recent Acuant SDK release from Github @scripts/download_acuant_sdk.sh + +clobber_db: ## Resets the database for make setup + bin/rake db:create + bin/rake db:environment:set + bin/rake db:reset + bin/rake db:environment:set + bin/rake dev:prime + +clobber_assets: ## Removes (clobbers) assets + bin/rake assets:clobber + RAILS_ENV=test bin/rake assets:clobber + +clobber_logs: ## Purges logs and tmp/ + rm -f log/* + rm -rf tmp/cache/* + rm -rf tmp/encrypted_doc_storage + rm -rf tmp/letter_opener + rm -rf tmp/mails + +tidy: clobber_assets clobber_logs ## Remove assets, logs, and unused gems, but leave DB alone + bundle clean diff --git a/bin/setup b/bin/setup index 323d382db4b..0b637303757 100755 --- a/bin/setup +++ b/bin/setup @@ -56,22 +56,13 @@ Dir.chdir APP_ROOT do run "yarn install" puts "\n== Preparing database ==" - run 'bin/rake db:create' - run 'bin/rake db:environment:set' - run 'bin/rake db:reset' - run 'bin/rake db:environment:set' - run 'bin/rake dev:prime' + run 'make clobber_db' puts "\n== Cleaning up old assets ==" - run "bin/rake assets:clobber" - run "RAILS_ENV=test bin/rake assets:clobber" + run "make clobber_assets" puts "\n== Removing old logs and tempfiles ==" - run "rm -f log/*" - run "rm -rf tmp/cache" - run "rm -rf tmp/encrypted_doc_storage" - run "rm -rf tmp/letter_opener" - run "rm -rf tmp/mails" + run "make clobber_logs" puts "\n== Restarting application server ==" run "mkdir -p tmp" From c4a4acd96b3d2eaefaa52485bd2e363f3d82820c Mon Sep 17 00:00:00 2001 From: Andrew Duthie Date: Thu, 25 May 2023 09:25:07 -0400 Subject: [PATCH 04/20] Reintroduce Propshaft as replacement for Sprockets (#8457) * Revert "Revert from Propshaft back to Sprockets (#8412)" This reverts commit d632066e532d23f03bb5519e66a21ff5d60fa1f5. * Avoid configuring asset_host --- .gitignore | 1 - Gemfile | 3 ++- Gemfile.lock | 15 ++++++------ app/assets/config/manifest.js | 10 -------- app/assets/stylesheets/_uswds-core.scss | 4 ++-- app/assets/stylesheets/components/_alert.scss | 2 +- app/assets/stylesheets/components/_icon.scss | 2 +- .../components/_language-picker.scss | 6 ++--- app/assets/stylesheets/components/_list.scss | 2 +- .../stylesheets/components/_personal-key.scss | 4 ++-- .../stylesheets/components/_phone-input.scss | 4 ++-- .../components/_step-indicator.scss | 2 +- app/components/icon_component.rb | 4 +--- app/components/phone_input_component.html.erb | 2 +- .../service_provider_session_decorator.rb | 2 ++ app/helpers/asset_helper.rb | 7 ------ app/helpers/script_helper.rb | 2 +- app/javascript/packages/components/icon.tsx | 2 +- .../accounts/_unphishable_badge.html.erb | 2 +- .../accounts/_verified_account_badge.html.erb | 2 +- app/views/layouts/base.html.erb | 8 +++---- app/views/shared/_banner-lock-icon.html.erb | 2 +- config/application.rb | 3 --- config/environments/development.rb | 5 +--- config/environments/production.rb | 8 ------- config/environments/test.rb | 5 ---- config/initializers/assets.rb | 11 +++------ docs/troubleshooting.md | 8 ------- scripts/artifact-upload | 2 +- spec/helpers/asset_helper_spec.rb | 23 ------------------- spec/helpers/script_helper_spec.rb | 8 +++---- 31 files changed, 43 insertions(+), 118 deletions(-) delete mode 100644 app/assets/config/manifest.js delete mode 100644 app/helpers/asset_helper.rb delete mode 100644 spec/helpers/asset_helper_spec.rb diff --git a/.gitignore b/.gitignore index 73ada7f4c7d..efcb4365403 100644 --- a/.gitignore +++ b/.gitignore @@ -7,7 +7,6 @@ .generators *.pyc *.rbc -*.sassc **.orig .bundle .byebug_history diff --git a/Gemfile b/Gemfile index 9cbb44f0a69..a4e3b0438af 100644 --- a/Gemfile +++ b/Gemfile @@ -44,6 +44,7 @@ gem 'pg' gem 'phonelib' gem 'premailer-rails', '>= 1.12.0' gem 'profanity_filter' +gem 'propshaft' gem 'rack', '>= 2.2.3.1' gem 'rack-attack', '>= 6.2.1' gem 'rack-cors', '>= 1.0.5', require: 'rack/cors' @@ -61,7 +62,6 @@ gem 'safe_target_blank', '>= 1.0.2' gem 'saml_idp', github: '18F/saml_idp', tag: '0.18.2-18f' gem 'scrypt' gem 'simple_form', '>= 5.0.2' -gem 'sprockets-rails' gem 'stringex', require: false gem 'strong_migrations', '>= 0.4.2' gem 'subprocess', require: false @@ -85,6 +85,7 @@ group :development do gem 'guard-rspec', require: false gem 'irb' gem 'letter_opener', '~> 1.8' + gem 'listen' gem 'octokit', '>= 4.25.0' gem 'rack-mini-profiler', '>= 1.1.3', require: false gem 'rails-erd', '>= 1.6.0' diff --git a/Gemfile.lock b/Gemfile.lock index ca2dafb71fd..36f61680e8d 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -451,6 +451,11 @@ GEM net-smtp premailer (~> 1.7, >= 1.7.9) profanity_filter (0.1.1) + propshaft (0.7.0) + actionpack (>= 7.0.0) + activesupport (>= 7.0.0) + rack + railties (>= 7.0.0) pry (0.14.1) coderay (~> 1.1) method_source (~> 1.0) @@ -635,13 +640,6 @@ GEM simpleidn (0.2.1) unf (~> 0.1.4) smart_properties (1.17.0) - sprockets (4.0.2) - concurrent-ruby (~> 1.0) - rack (> 1, < 3) - sprockets-rails (3.4.2) - actionpack (>= 5.2) - activesupport (>= 5.2) - sprockets (>= 3.0.0) stringex (2.8.5) strong_migrations (0.8.0) activerecord (>= 5.2) @@ -766,6 +764,7 @@ DEPENDENCIES jwt knapsack letter_opener (~> 1.8) + listen lograge (>= 0.11.2) lookbook (~> 1.5.3) lru_redux @@ -781,6 +780,7 @@ DEPENDENCIES phonelib premailer-rails (>= 1.12.0) profanity_filter + propshaft pry-byebug pry-doc pry-rails @@ -819,7 +819,6 @@ DEPENDENCIES simplecov (~> 0.21.0) simplecov-cobertura simplecov_json_formatter - sprockets-rails stringex strong_migrations (>= 0.4.2) subprocess diff --git a/app/assets/config/manifest.js b/app/assets/config/manifest.js deleted file mode 100644 index 6b147d7d533..00000000000 --- a/app/assets/config/manifest.js +++ /dev/null @@ -1,10 +0,0 @@ -//= link_tree ../images -//= link_tree ../fonts - -//= link intl-tel-input/build/css/intlTelInput.css -//= link intl-tel-input/build/img/flags.png -//= link intl-tel-input/build/img/flags@2x.png - -//= link_tree ../../../node_modules/@18f/identity-design-system/dist/assets/img -//= link_tree ../../../node_modules/@18f/identity-design-system/dist/assets/fonts -//= link_tree ../builds diff --git a/app/assets/stylesheets/_uswds-core.scss b/app/assets/stylesheets/_uswds-core.scss index 1a1be05c817..656649b41b0 100644 --- a/app/assets/stylesheets/_uswds-core.scss +++ b/app/assets/stylesheets/_uswds-core.scss @@ -4,8 +4,8 @@ $render-font-face: false !default; @forward '@18f/identity-design-system/packages/uswds-core' with ( $theme-body-font-size: 'sm', - $theme-font-path: '.', - $theme-image-path: '@18f/identity-design-system/dist/assets/img', + $theme-font-path: '', + $theme-image-path: '', $theme-global-border-box-sizing: true, $theme-global-link-styles: true, $theme-grid-container-max-width: 'tablet-lg', diff --git a/app/assets/stylesheets/components/_alert.scss b/app/assets/stylesheets/components/_alert.scss index 506cafceb76..fc35acaffb5 100644 --- a/app/assets/stylesheets/components/_alert.scss +++ b/app/assets/stylesheets/components/_alert.scss @@ -6,7 +6,7 @@ font-weight: bold; &::before { - background-image: url('alert/info-white.svg'); + background-image: url('/alert/info-white.svg'); } } diff --git a/app/assets/stylesheets/components/_icon.scss b/app/assets/stylesheets/components/_icon.scss index 05b22f6a71d..708d8447270 100644 --- a/app/assets/stylesheets/components/_icon.scss +++ b/app/assets/stylesheets/components/_icon.scss @@ -28,7 +28,7 @@ $icon-min-padding: 2px; &-success { &::before { - background-image: url('alert/success.svg'); + background-image: url('/alert/success.svg'); content: ''; display: block; height: $h4; diff --git a/app/assets/stylesheets/components/_language-picker.scss b/app/assets/stylesheets/components/_language-picker.scss index e6d14f6823b..5910f030e92 100644 --- a/app/assets/stylesheets/components/_language-picker.scss +++ b/app/assets/stylesheets/components/_language-picker.scss @@ -66,10 +66,10 @@ } &::after { - background-image: url(@18f/identity-design-system/dist/assets/img/angle-arrow-up.svg); + background-image: url('/angle-arrow-up.svg'); @include at-media('tablet') { - background-image: url(@18f/identity-design-system/dist/assets/img/angle-arrow-up-white.svg); + background-image: url('/angle-arrow-up-white.svg'); } } } @@ -79,7 +79,7 @@ color: color('white'); &::after { - background-image: url(@18f/identity-design-system/dist/assets/img/angle-arrow-down-white.svg); + background-image: url('/angle-arrow-down-white.svg'); } } } diff --git a/app/assets/stylesheets/components/_list.scss b/app/assets/stylesheets/components/_list.scss index 5be58d778d2..389920d1e7b 100644 --- a/app/assets/stylesheets/components/_list.scss +++ b/app/assets/stylesheets/components/_list.scss @@ -3,7 +3,7 @@ padding: 1rem 1rem 1rem 0; &::before { - background-image: url('alert/success.svg'); + background-image: url('/alert/success.svg'); background-repeat: no-repeat; content: ''; display: inline-block; diff --git a/app/assets/stylesheets/components/_personal-key.scss b/app/assets/stylesheets/components/_personal-key.scss index 27d55bc560e..c475e8c922f 100644 --- a/app/assets/stylesheets/components/_personal-key.scss +++ b/app/assets/stylesheets/components/_personal-key.scss @@ -2,7 +2,7 @@ .personal-key-block { @include u-padding-y(2); - background-image: url('personal-key/pkey-block.svg'); + background-image: url('/personal-key/pkey-block.svg'); background-position: center; background-repeat: no-repeat; @@ -28,7 +28,7 @@ .bg-personal-key { height: 145px; - background-image: url('personal-key/shield.svg'); + background-image: url('/personal-key/shield.svg'); background-position: center; background-repeat: no-repeat; background-size: 145px 145px; diff --git a/app/assets/stylesheets/components/_phone-input.scss b/app/assets/stylesheets/components/_phone-input.scss index 07f0e973b95..b9a1deec9fd 100644 --- a/app/assets/stylesheets/components/_phone-input.scss +++ b/app/assets/stylesheets/components/_phone-input.scss @@ -8,11 +8,11 @@ lg-phone-input { } .iti__flag { - background-image: url('intl-tel-input/build/img/flags.png'); + background-image: url('/flags.png'); /* stylelint-disable-next-line media-feature-name-no-vendor-prefix */ @media (-webkit-min-device-pixel-ratio: 2), (min-resolution: 192dpi) { - background-image: url('intl-tel-input/build/img/flags@2x.png'); + background-image: url('/flags@2x.png'); } } diff --git a/app/assets/stylesheets/components/_step-indicator.scss b/app/assets/stylesheets/components/_step-indicator.scss index 95a3835e984..b10b9cb5212 100644 --- a/app/assets/stylesheets/components/_step-indicator.scss +++ b/app/assets/stylesheets/components/_step-indicator.scss @@ -109,7 +109,7 @@ lg-step-indicator { .step-indicator__step--complete::before { background-color: color('white'); - background-image: url('alert/success.svg'); + background-image: url('/alert/success.svg'); } .step-indicator__step:not(:last-child)::after { diff --git a/app/components/icon_component.rb b/app/components/icon_component.rb index 7976d647003..f0c679a5955 100644 --- a/app/components/icon_component.rb +++ b/app/components/icon_component.rb @@ -1,6 +1,4 @@ class IconComponent < BaseComponent - include AssetHelper - # See: https://github.com/uswds/uswds/tree/develop/src/img/usa-icons ICONS = %i[ accessibility_new @@ -256,7 +254,7 @@ def initialize(icon:, **tag_options) end def icon_path - asset_path([design_system_asset_path('img/sprite.svg'), '#', icon].join, host: asset_host) + asset_path([asset_path('sprite.svg'), '#', icon].join, host: asset_host) end private diff --git a/app/components/phone_input_component.html.erb b/app/components/phone_input_component.html.erb index 24a495525de..a6d5ce7ecf9 100644 --- a/app/components/phone_input_component.html.erb +++ b/app/components/phone_input_component.html.erb @@ -53,4 +53,4 @@ }, ) %> <% end %> -<%= stylesheet_link_tag 'intl-tel-input/build/css/intlTelInput' %> +<%= stylesheet_link_tag 'intlTelInput' %> diff --git a/app/decorators/service_provider_session_decorator.rb b/app/decorators/service_provider_session_decorator.rb index 83ad7cce508..d6aeca42be5 100644 --- a/app/decorators/service_provider_session_decorator.rb +++ b/app/decorators/service_provider_session_decorator.rb @@ -42,6 +42,8 @@ def s3_logo_url(service_provider) def legacy_logo_url logo = sp_logo ActionController::Base.helpers.image_path("sp-logos/#{logo}") + rescue Propshaft::MissingAssetError + '' end def new_session_heading diff --git a/app/helpers/asset_helper.rb b/app/helpers/asset_helper.rb deleted file mode 100644 index 4a4d013ca9c..00000000000 --- a/app/helpers/asset_helper.rb +++ /dev/null @@ -1,7 +0,0 @@ -module AssetHelper - DESIGN_SYSTEM_ASSET_ROOT = '@18f/identity-design-system/dist/assets'.freeze - - def design_system_asset_path(path) - File.join(DESIGN_SYSTEM_ASSET_ROOT, path) - end -end diff --git a/app/helpers/script_helper.rb b/app/helpers/script_helper.rb index 1f7ba137b16..b2da5680250 100644 --- a/app/helpers/script_helper.rb +++ b/app/helpers/script_helper.rb @@ -39,7 +39,7 @@ def render_javascript_pack_once_tags(*names) private SAME_ORIGIN_ASSETS = %w[ - @18f/identity-design-system/dist/assets/img/sprite.svg + sprite.svg ].to_set.freeze def local_crossorigin_sources? diff --git a/app/javascript/packages/components/icon.tsx b/app/javascript/packages/components/icon.tsx index 5bd6d85083c..a0b57b7e74e 100644 --- a/app/javascript/packages/components/icon.tsx +++ b/app/javascript/packages/components/icon.tsx @@ -1,6 +1,6 @@ import { getAssetPath } from '@18f/identity-assets'; -const SPRITE_URL = getAssetPath('@18f/identity-design-system/dist/assets/img/sprite.svg'); +const SPRITE_URL = getAssetPath('sprite.svg'); export type DesignSystemIcon = | 'accessibility_new' diff --git a/app/views/accounts/_unphishable_badge.html.erb b/app/views/accounts/_unphishable_badge.html.erb index ff2873288ba..d93bdfe0130 100644 --- a/app/views/accounts/_unphishable_badge.html.erb +++ b/app/views/accounts/_unphishable_badge.html.erb @@ -1,6 +1,6 @@
<%= image_tag( - design_system_asset_path('img/alerts/unphishable.svg'), + asset_path('alerts/unphishable.svg'), size: 16, class: 'text-middle', alt: '', diff --git a/app/views/accounts/_verified_account_badge.html.erb b/app/views/accounts/_verified_account_badge.html.erb index f43f4f70768..7a4df1b1628 100644 --- a/app/views/accounts/_verified_account_badge.html.erb +++ b/app/views/accounts/_verified_account_badge.html.erb @@ -1,6 +1,6 @@
<%= image_tag( - design_system_asset_path('img/alerts/success.svg'), + asset_path('alerts/success.svg'), size: 16, class: 'text-middle', alt: '', diff --git a/app/views/layouts/base.html.erb b/app/views/layouts/base.html.erb index ee3319ca9c1..d71a5412a96 100644 --- a/app/views/layouts/base.html.erb +++ b/app/views/layouts/base.html.erb @@ -30,25 +30,25 @@ <%= csrf_meta_tags %> <%= favicon_link_tag( - design_system_asset_path('img/favicons/apple-touch-icon.png'), + asset_path('favicons/apple-touch-icon.png'), rel: 'apple-touch-icon', sizes: '180x180', type: 'image/png', ) %> <%= favicon_link_tag( - design_system_asset_path('img/favicons/favicon-40.png'), + asset_path('favicons/favicon-40.png'), rel: 'icon', sizes: '40x40', type: 'image/png', ) %> <%= favicon_link_tag( - design_system_asset_path('img/favicons/favicon-16.png'), + asset_path('favicons/favicon-16.png'), rel: 'icon', sizes: '16x16', type: 'image/png', ) %> <%= favicon_link_tag( - design_system_asset_path('img/favicons/safari-pinned-tab.svg'), + asset_path('favicons/safari-pinned-tab.svg'), rel: 'mask-icon', color: '#e21c3d', type: nil, diff --git a/app/views/shared/_banner-lock-icon.html.erb b/app/views/shared/_banner-lock-icon.html.erb index feef6e8ee5a..e4da7888a37 100644 --- a/app/views/shared/_banner-lock-icon.html.erb +++ b/app/views/shared/_banner-lock-icon.html.erb @@ -1,5 +1,5 @@ <%= image_tag( - design_system_asset_path('img/lock.svg'), + asset_path('lock.svg'), width: 9, height: 12, class: 'usa-banner__lock-image', diff --git a/config/application.rb b/config/application.rb index cb186ffee26..c788559fec9 100644 --- a/config/application.rb +++ b/config/application.rb @@ -5,7 +5,6 @@ require 'action_view/railtie' require 'action_mailer/railtie' require 'rails/test_unit/railtie' -require 'sprockets/railtie' require 'identity/logging/railtie' require_relative '../lib/asset_sources' @@ -56,8 +55,6 @@ class Application < Rails::Application config.load_defaults '7.0' config.active_record.belongs_to_required_by_default = false config.active_record.legacy_connection_handling = false - config.assets.unknown_asset_fallback = true - config.assets.resolve_assets_in_css_urls = true config.active_job.queue_adapter = :good_job FileUtils.mkdir_p(Rails.root.join('log')) diff --git a/config/environments/development.rb b/config/environments/development.rb index 1306d3da830..928cceb38fc 100644 --- a/config/environments/development.rb +++ b/config/environments/development.rb @@ -4,10 +4,7 @@ config.consider_all_requests_local = true config.active_support.deprecation = :log config.active_record.migration_error = :page_load - config.assets.debug = true - config.assets.digest = true - config.assets.gzip = false - config.assets.raise_runtime_errors = true + config.file_watcher = ActiveSupport::EventedFileUpdateChecker config.i18n.raise_on_missing_translations = true # Raise exceptions for disallowed deprecations. diff --git a/config/environments/production.rb b/config/environments/production.rb index 330ac83cb27..a586dd41076 100644 --- a/config/environments/production.rb +++ b/config/environments/production.rb @@ -6,14 +6,6 @@ config.action_controller.perform_caching = true config.force_ssl = true - config.asset_host = proc do |_source, request| - # we want precompiled assets to have domain-agnostic URLs - # and request is nil during asset precompilation - (IdentityConfig.store.asset_host.presence || IdentityConfig.store.domain_name) if request - end - config.assets.compile = false - config.assets.digest = true - config.assets.gzip = false config.i18n.fallbacks = true config.active_support.deprecation = :notify config.active_record.dump_schema_after_migration = false diff --git a/config/environments/test.rb b/config/environments/test.rb index 7a3bb2b6afa..cc6088fca3c 100644 --- a/config/environments/test.rb +++ b/config/environments/test.rb @@ -12,15 +12,12 @@ config.action_controller.allow_forgery_protection = false config.active_support.test_order = :random config.active_support.deprecation = :stderr - config.assets.gzip = false config.i18n.raise_on_missing_translations = true config.action_mailer.delivery_method = :test config.action_mailer.default_url_options = { host: IdentityConfig.store.domain_name } config.action_mailer.asset_host = IdentityConfig.store.mailer_domain_name - config.assets.debug = false - # Raise exceptions for disallowed deprecations. config.active_support.disallowed_deprecation = :raise @@ -32,8 +29,6 @@ config.action_controller.asset_host = ENV['RAILS_ASSET_HOST'] if ENV.key?('RAILS_ASSET_HOST') - config.assets.digest = ENV.key?('RAILS_DISABLE_ASSET_DIGEST') ? false : true - config.middleware.use RackSessionAccess::Middleware config.after_initialize do diff --git a/config/initializers/assets.rb b/config/initializers/assets.rb index cc0cc161230..9d43c2aae76 100644 --- a/config/initializers/assets.rb +++ b/config/initializers/assets.rb @@ -5,13 +5,8 @@ # Add additional assets to the asset load path Rails.application.config.assets.paths.push( - 'node_modules', + 'node_modules/intl-tel-input/build/img', + 'node_modules/intl-tel-input/build/css', + 'node_modules/@18f/identity-design-system/dist/assets/img', 'node_modules/@18f/identity-design-system/dist/assets/fonts', ) - -# Fix sassc sometimes segfaulting -Rails.application.config.assets.configure do |env| - env.export_concurrent = false -end - -Sprockets.export_concurrent = Rails.env.test? diff --git a/docs/troubleshooting.md b/docs/troubleshooting.md index b40fcaab89f..0867ca17ede 100644 --- a/docs/troubleshooting.md +++ b/docs/troubleshooting.md @@ -85,14 +85,6 @@ Use Control+X to save the file. Restart your Mac to cause the .plist to take effect. Check the limits again and you should see both `ulimit -n` and `launchctl limit maxfiles` return a limit of 524288. -### Errors related to _sassc_ - -If you are getting the error: -``` -LoadError: cannot load such file -- sassc -``` -Try `make run` for a short time, then use Ctrl+C to kill it - ### Errors relating to OpenSSL versions If you get this error during test runs: diff --git a/scripts/artifact-upload b/scripts/artifact-upload index 10b4c467914..99eae75e51a 100755 --- a/scripts/artifact-upload +++ b/scripts/artifact-upload @@ -33,7 +33,7 @@ branch_environments.map do |upload_environment| raise 'artifact copy failed' unless status.success? assets_sync_command = <<-CMD - aws s3 sync --size-only --cache-control max-age=31536000 --exclude .sprockets-manifest-*.json public/assets s3://#{static_bucket}/assets + aws s3 sync --size-only --cache-control max-age=31536000 --exclude ".manifest.json" public/assets s3://#{static_bucket}/assets CMD _output, status = Open3.capture2(assets_sync_command) raise 'assets sync failed' unless status.success? diff --git a/spec/helpers/asset_helper_spec.rb b/spec/helpers/asset_helper_spec.rb deleted file mode 100644 index 3952a2b34f2..00000000000 --- a/spec/helpers/asset_helper_spec.rb +++ /dev/null @@ -1,23 +0,0 @@ -require 'rails_helper' - -RSpec.describe AssetHelper do - include AssetHelper - - describe 'design_system_asset_path' do - let(:path) { 'img/example.png' } - - subject(:asset_path) { design_system_asset_path(path) } - - it 'produces an asset path' do - expect(asset_path).to eq('@18f/identity-design-system/dist/assets/img/example.png') - end - - context 'with leading slash' do - let(:path) { '/img/example.png' } - - it 'produces an asset path' do - expect(asset_path).to eq('@18f/identity-design-system/dist/assets/img/example.png') - end - end - end -end diff --git a/spec/helpers/script_helper_spec.rb b/spec/helpers/script_helper_spec.rb index f0abd0d6489..5e82355dd38 100644 --- a/spec/helpers/script_helper_spec.rb +++ b/spec/helpers/script_helper_spec.rb @@ -32,7 +32,7 @@ allow(AssetSources).to receive(:get_sources).with('application', 'document-capture'). and_return(['/application.js', '/document-capture.js']) allow(AssetSources).to receive(:get_assets).with('application', 'document-capture'). - and_return(['clock.svg', '@18f/identity-design-system/dist/assets/img/sprite.svg']) + and_return(['clock.svg', 'sprite.svg']) end it 'prints asset paths sources' do @@ -43,8 +43,7 @@ visible: :all, text: { 'clock.svg' => 'http://test.host/clock.svg', - '@18f/identity-design-system/dist/assets/img/sprite.svg' => - 'http://test.host/@18f/identity-design-system/dist/assets/img/sprite.svg', + 'sprite.svg' => 'http://test.host/sprite.svg', }.to_json, ) end @@ -66,8 +65,7 @@ visible: :all, text: { 'clock.svg' => 'http://assets.example.com/clock.svg', - '@18f/identity-design-system/dist/assets/img/sprite.svg' => - 'http://example.com/@18f/identity-design-system/dist/assets/img/sprite.svg', + 'sprite.svg' => 'http://example.com/sprite.svg', }.to_json, ) end From 87d71d5094fcf216dad5911dabb7d97faad54292 Mon Sep 17 00:00:00 2001 From: Amir Reavis-Bey <1261794+amirbey@users.noreply.github.com> Date: Thu, 25 May 2023 09:34:18 -0400 Subject: [PATCH 05/20] Remove combined params from mobile upload step url (#8481) * Remove combined params from mobile upload step url * [skip changelog] --------- Co-authored-by: AmirReavis-Bey --- app/views/idv/doc_auth/upload.html.erb | 2 +- spec/services/idv/steps/upload_step_spec.rb | 1 - 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/app/views/idv/doc_auth/upload.html.erb b/app/views/idv/doc_auth/upload.html.erb index 76cbfada0ea..f65d741af23 100644 --- a/app/views/idv/doc_auth/upload.html.erb +++ b/app/views/idv/doc_auth/upload.html.erb @@ -30,7 +30,7 @@ <%= simple_form_for( idv_phone_form, as: :doc_auth, - url: url_for(type: :mobile, combined: true), + url: url_for(type: :mobile), method: 'PUT', html: { autocomplete: 'off' }, ) do |f| %> diff --git a/spec/services/idv/steps/upload_step_spec.rb b/spec/services/idv/steps/upload_step_spec.rb index 7c7fe3046f9..34556385cf1 100644 --- a/spec/services/idv/steps/upload_step_spec.rb +++ b/spec/services/idv/steps/upload_step_spec.rb @@ -24,7 +24,6 @@ ActionController::Parameters.new( { doc_auth: { phone: '(201) 555-1212' }, - combined: true, }, ) end From b4b2fa7a2ea1c2a301bc6905b0ad650fdc870631 Mon Sep 17 00:00:00 2001 From: gina-yamada <125507397+gina-yamada@users.noreply.github.com> Date: Thu, 25 May 2023 08:43:46 -0600 Subject: [PATCH 06/20] LG-9816: Add accepted special characters to hint text on ID number page (#8435) * LG-9816 Edit hint text for ID number and increase input size * LG-9816 Add spaces in fr state_id_number_hint txt * changlog: User-Facing Improvements, In-person proofing, Edit state id number hint text * LG-9816 Restore en.yml section * changelog: User-Facing Improvements, In-person proofing, Edit state id number hint text * LG-9816 fix lint issue * LG-9816 Add grid-col-10 to hint text * LG-9816 Use existing class for breakpoint * LG-9816 remove extra line * LG-9816 Refactor to include html so we can tell screen reader value * LG-9816 Added fr and es translations --- app/views/idv/in_person/state_id.html.erb | 5 +++-- config/locales/in_person_proofing/en.yml | 8 +++++++- config/locales/in_person_proofing/es.yml | 8 +++++++- config/locales/in_person_proofing/fr.yml | 8 +++++++- 4 files changed, 24 insertions(+), 5 deletions(-) diff --git a/app/views/idv/in_person/state_id.html.erb b/app/views/idv/in_person/state_id.html.erb index 6cba583e773..23bc81a9f4e 100644 --- a/app/views/idv/in_person/state_id.html.erb +++ b/app/views/idv/in_person/state_id.html.erb @@ -111,11 +111,12 @@ ) %>
<% end %> -
+
<%= render ValidatedFieldComponent.new( name: :state_id_number, form: f, - hint: t('in_person_proofing.form.state_id.state_id_number_hint'), + hint: t('in_person_proofing.form.state_id.state_id_number_hint_html'), + hint_html: { class: 'tablet:grid-col-10' }, input_html: { value: pii[:state_id_number] }, label: t('in_person_proofing.form.state_id.state_id_number'), label_html: { class: 'usa-label' }, diff --git a/config/locales/in_person_proofing/en.yml b/config/locales/in_person_proofing/en.yml index 736af7b8a69..90fb482162b 100644 --- a/config/locales/in_person_proofing/en.yml +++ b/config/locales/in_person_proofing/en.yml @@ -130,7 +130,13 @@ en: state_id_jurisdiction_hint: This is the state that issued your ID state_id_jurisdiction_prompt: '- Select -' state_id_number: ID number - state_id_number_hint: May include letters and numbers + state_id_number_hint_html: "May include letters, numbers, and the following + symbols: spaces, forward + slashes, asterisks, + dashes" zipcode: ZIP Code headings: address: Enter your current residential address diff --git a/config/locales/in_person_proofing/es.yml b/config/locales/in_person_proofing/es.yml index 06b1e33e2e9..ddd6cb7802f 100644 --- a/config/locales/in_person_proofing/es.yml +++ b/config/locales/in_person_proofing/es.yml @@ -143,7 +143,13 @@ es: state_id_jurisdiction_hint: Este es el estado que emitió su identificación state_id_jurisdiction_prompt: '- Seleccione -' state_id_number: Número de cédula - state_id_number_hint: Puede incluir letras y números + state_id_number_hint_html: "Puede incluir letras, números y los siguientes + símbolos: espacios, barras + diagonales, asteriscos, guiones" zipcode: Código postal headings: address: Ingresa tu domicilio actual diff --git a/config/locales/in_person_proofing/fr.yml b/config/locales/in_person_proofing/fr.yml index 3761caa760d..f773ad8c562 100644 --- a/config/locales/in_person_proofing/fr.yml +++ b/config/locales/in_person_proofing/fr.yml @@ -145,7 +145,13 @@ fr: state_id_jurisdiction_hint: Il s’agit de l’État qui a émis votre pièce d’identité state_id_jurisdiction_prompt: '- Sélectionnez -' state_id_number: Numéro d’identification - state_id_number_hint: Peut comprendre des lettres et des chiffres + state_id_number_hint_html: "Il peut s’agir de lettres, de chiffres et des + symboles suivants: des espaces, des barres + obliques, des astérisques, des + tirets" zipcode: Code postal headings: address: Indiquez votre adresse résidentielle actuelle From f46937c31f9b0fc4ce8f31ec714f47c6e10e5b57 Mon Sep 17 00:00:00 2001 From: Matt Hinz Date: Thu, 25 May 2023 09:00:36 -0700 Subject: [PATCH 07/20] Allow using StepIndicatorConcern w/o IdvSession (#8480) * Allow using StepIndicatorConcern w/o IdvSession If the user is not logged in and does not have an `idv_session` available (as is the case in the hybrid document capture flow), this code would raise and the user would get a 500 error. This update makes an assumption that if the user is not properly logged in they are probably not in the gpo or in person flow. [skip changelog] * Update app/controllers/concerns/idv/step_indicator_concern.rb Co-authored-by: Sonia Connolly * Add IdvSession to controllers that need it These controllers previously included IdvSession via StepIndicatorConcern. * Refactor proofing_components being/end block into a method --------- Co-authored-by: Sonia Connolly --- .../concerns/idv/step_indicator_concern.rb | 24 ++++++++------ .../idv/come_back_later_controller.rb | 1 + .../in_person/ready_to_verify_controller.rb | 1 + .../idv/step_indicator_concern_spec.rb | 31 +++++++++++++++++++ 4 files changed, 47 insertions(+), 10 deletions(-) diff --git a/app/controllers/concerns/idv/step_indicator_concern.rb b/app/controllers/concerns/idv/step_indicator_concern.rb index 366f5f0be34..247a62b2278 100644 --- a/app/controllers/concerns/idv/step_indicator_concern.rb +++ b/app/controllers/concerns/idv/step_indicator_concern.rb @@ -2,8 +2,6 @@ module Idv module StepIndicatorConcern extend ActiveSupport::Concern - include IdvSession - included do helper_method :step_indicator_steps end @@ -31,20 +29,26 @@ def in_person_proofing? def gpo_address_verification? # Proofing component values are (currently) never reset between proofing attempts, hence why # this refers to the session address verification mechanism and not the proofing component. - !!current_user.pending_profile || idv_session.address_verification_mechanism == 'gpo' + return true if current_user&.gpo_verification_pending_profile? + + return idv_session&.address_verification_mechanism == 'gpo' if defined?(idv_session) + end + + def proofing_components + return {} if !current_user + + if current_user.pending_profile + current_user.pending_profile.proofing_components + else + ProofingComponent.find_by(user: current_user).as_json + end end def proofing_components_as_hash # A proofing component record exists as a zero-or-one-to-one relation with a user, and values # are set during identity verification. These values are recorded to the profile at creation, # including for a pending profile. - @proofing_components_as_hash ||= begin - if current_user.pending_profile - current_user.pending_profile.proofing_components - else - ProofingComponent.find_by(user: current_user).as_json - end - end.to_h + @proofing_components_as_hash ||= proofing_components.to_h end end end diff --git a/app/controllers/idv/come_back_later_controller.rb b/app/controllers/idv/come_back_later_controller.rb index 86c00def874..55b0415505e 100644 --- a/app/controllers/idv/come_back_later_controller.rb +++ b/app/controllers/idv/come_back_later_controller.rb @@ -1,5 +1,6 @@ module Idv class ComeBackLaterController < ApplicationController + include IdvSession include StepIndicatorConcern before_action :confirm_two_factor_authenticated diff --git a/app/controllers/idv/in_person/ready_to_verify_controller.rb b/app/controllers/idv/in_person/ready_to_verify_controller.rb index 95a1fc72d8a..566c40f2911 100644 --- a/app/controllers/idv/in_person/ready_to_verify_controller.rb +++ b/app/controllers/idv/in_person/ready_to_verify_controller.rb @@ -1,6 +1,7 @@ module Idv module InPerson class ReadyToVerifyController < ApplicationController + include IdvSession include RenderConditionConcern include StepIndicatorConcern diff --git a/spec/controllers/concerns/idv/step_indicator_concern_spec.rb b/spec/controllers/concerns/idv/step_indicator_concern_spec.rb index f26d95d1907..6bbb2190256 100644 --- a/spec/controllers/concerns/idv/step_indicator_concern_spec.rb +++ b/spec/controllers/concerns/idv/step_indicator_concern_spec.rb @@ -2,6 +2,7 @@ RSpec.describe Idv::StepIndicatorConcern, type: :controller do controller ApplicationController do + include IdvSession include Idv::StepIndicatorConcern end @@ -117,6 +118,36 @@ def force_gpo end end end + + context 'when user is not signed in' do + let(:user) { nil } + it 'returns doc auth flow steps and does not crash' do + expect(steps).to eq(Idv::Flows::DocAuthFlow::STEP_INDICATOR_STEPS) + end + + context 'when idv_session method is not present' do + controller ApplicationController do + include Idv::StepIndicatorConcern + end + it 'returns doc auth flow steps and does not crash' do + expect(steps).to eq(Idv::Flows::DocAuthFlow::STEP_INDICATOR_STEPS) + end + end + + context 'when idv_session is nil' do + controller ApplicationController do + include Idv::StepIndicatorConcern + end + + before do + allow(controller).to receive(:idv_session).and_return(nil) + end + + it 'returns doc auth flow steps and does not crash' do + expect(steps).to eq(Idv::Flows::DocAuthFlow::STEP_INDICATOR_STEPS) + end + end + end end end end From 45431f095025ee5efaa62d1e6279d2968a09e10a Mon Sep 17 00:00:00 2001 From: Andrew Duthie Date: Thu, 25 May 2023 12:00:50 -0400 Subject: [PATCH 08/20] Fix mobile unstyled appearance for clipboard button (#8484) changelog: Bug Fixes, Personal Key, Fix button appearance for "Copy" link --- app/components/clipboard_button_component.rb | 24 ++++++++++++++----- .../clipboard_button_component.scss | 4 ++++ .../clipboard_button_component_spec.rb | 14 ++++++++--- .../clipboard_button_component_preview.rb | 11 +++++++-- 4 files changed, 42 insertions(+), 11 deletions(-) diff --git a/app/components/clipboard_button_component.rb b/app/components/clipboard_button_component.rb index 0d8612cc54d..38719624eff 100644 --- a/app/components/clipboard_button_component.rb +++ b/app/components/clipboard_button_component.rb @@ -1,22 +1,34 @@ -class ClipboardButtonComponent < ButtonComponent - attr_reader :clipboard_text, :tag_options - - def initialize(clipboard_text:, **tag_options) - super(**tag_options, type: :button, icon: :content_copy) +class ClipboardButtonComponent < BaseComponent + attr_reader :clipboard_text, :button_options + def initialize(clipboard_text:, **button_options) @clipboard_text = clipboard_text + @button_options = button_options end def call content_tag( :'lg-clipboard-button', - super, + button_content, 'clipboard-text': clipboard_text, 'tooltip-text': t('components.clipboard_button.tooltip'), + class: css_class, ) end def content t('components.clipboard_button.label') end + + def css_class + 'clipboard-button--unstyled' if button_options[:unstyled] + end + + def button_content + render ButtonComponent.new( + **button_options, + type: :button, + icon: :content_copy, + ).with_content(content) + end end diff --git a/app/components/clipboard_button_component.scss b/app/components/clipboard_button_component.scss index 25e92517298..0073a14d6a4 100644 --- a/app/components/clipboard_button_component.scss +++ b/app/components/clipboard_button_component.scss @@ -12,5 +12,9 @@ .usa-tooltip { @include at-media-max('mobile-lg') { display: block; + + .clipboard-button--unstyled & { + display: inline; + } } } diff --git a/spec/components/clipboard_button_component_spec.rb b/spec/components/clipboard_button_component_spec.rb index 5360c0a67d7..bb2d8642147 100644 --- a/spec/components/clipboard_button_component_spec.rb +++ b/spec/components/clipboard_button_component_spec.rb @@ -2,10 +2,10 @@ RSpec.describe ClipboardButtonComponent, type: :component do let(:clipboard_text) { 'Copy Text' } - let(:tag_options) { {} } + let(:button_options) { {} } subject(:rendered) do - render_inline ClipboardButtonComponent.new(clipboard_text:, **tag_options) + render_inline ClipboardButtonComponent.new(clipboard_text:, **button_options) end it 'renders with clipboard text as attribute' do @@ -19,7 +19,7 @@ end context 'with tag options' do - let(:tag_options) { { outline: true, data: { foo: 'bar' } } } + let(:button_options) { { outline: true, data: { foo: 'bar' } } } it 'renders button given the tag options' do expect(rendered).to have_css('button.usa-button[type="button"][data-foo="bar"]') @@ -29,4 +29,12 @@ expect(rendered).to have_css('.usa-button--outline:not([outline])') end end + + context 'with unstyled button' do + let(:button_options) { { unstyled: true } } + + it 'renders with modifier class' do + expect(rendered).to have_css('lg-clipboard-button.clipboard-button--unstyled') + end + end end diff --git a/spec/components/previews/clipboard_button_component_preview.rb b/spec/components/previews/clipboard_button_component_preview.rb index d21354793cc..4e8d1897cdb 100644 --- a/spec/components/previews/clipboard_button_component_preview.rb +++ b/spec/components/previews/clipboard_button_component_preview.rb @@ -3,11 +3,18 @@ class ClipboardButtonComponentPreview < BaseComponentPreview def default render(ClipboardButtonComponent.new(clipboard_text: 'Copied Text', class: css_class)) end + + def unstyled + render( + ClipboardButtonComponent.new(clipboard_text: 'Copied Text', unstyled: true, class: css_class), + ) + end # @!endgroup # @param clipboard_text text - def workbench(clipboard_text: 'Copied Text') - render(ClipboardButtonComponent.new(clipboard_text:, class: css_class)) + # @param unstyled toggle + def workbench(clipboard_text: 'Copied Text', unstyled: false) + render(ClipboardButtonComponent.new(clipboard_text:, unstyled:, class: css_class)) end private From 3f1177aecffdef0159a520cf4aaf115dffeeed2e Mon Sep 17 00:00:00 2001 From: Tim Bradley <90272033+NavaTim@users.noreply.github.com> Date: Thu, 25 May 2023 11:58:28 -0700 Subject: [PATCH 09/20] LG-8440: Ingest in-person enrollment status updates from SQS (#8403) * LG-8440: Install aws-sdk-ruby; create migration for status check field * LG-8440: Start writing job to check enrollment status update messages (wip) * LG-8440: Install mail gem; update job to read email sent via SNS/SQS * LG-8440: Rename job; improve handling for errors and message deletion * LG-8440: Continue refining logic around error logging; break up logic into modules * LG-8440: Split out analytics methods; start writing specs for job * LG-8440: Test and refine error reporting module * LG-8440: Continue writing tests; improve docs and error logic * LG-8440: Continue writing tests * LG-8440: Write test for enrollment pipeline * LG-8440: Fix module imports and default config * LG-8440: Consolidate SQS client calls; update tests * LG-8440: Write spec for SQS batch processor * LG-8440: Format spec file; add method documentation to process_batch * LG-8440: Add test for job; add new config value * LG-8440: Add config keys; rename key; lint fixes * LG-8440: Update analytics params and documentation * changelog: Internal, In-Person Proofing, Ingest in-person enrollment status updates from SQS * LG-8440: Extract batch deletion into separate method * LG-8440: Refactor modules to support dependency injection (WIP) * LG-8440: Move factory methods to job; write tests for factory methods; update job * LG-8440: Fix date logging and test * LG-8440: Increase the AWS HTTP read timeout for SQS client * LG-8440: Update error reporter to use analytics directly * LG-8440: Update to remove analytics factory * Update spec/jobs/in_person/enrollments_ready_for_status_check/enrollment_pipeline_spec.rb Co-authored-by: Zach Margolis * Update app/jobs/in_person/enrollments_ready_for_status_check/sqs_batch_wrapper.rb Co-authored-by: Zach Margolis * LG-9905: Use blank? instead of unless for flag check * LG-9905: Fix conditional and update test to catch issue --------- Co-authored-by: Zach Margolis --- Gemfile | 2 + Gemfile.lock | 5 + .../batch_processor.rb | 75 ++++ .../enrollment_pipeline.rb | 145 +++++++ .../error_reporter.rb | 29 ++ .../sqs_batch_wrapper.rb | 37 ++ .../enrollments_ready_for_status_check_job.rb | 107 +++++ app/services/analytics_events.rb | 56 +++ config/application.yml.default | 7 + config/initializers/job_configurations.rb | 6 + ...r_status_check_to_in_person_enrollments.rb | 7 + db/schema.rb | 4 +- lib/identity_config.rb | 7 + .../batch_processor_spec.rb | 229 +++++++++++ .../enrollment_pipeline_spec.rb | 335 ++++++++++++++++ .../error_reporter_spec.rb | 102 +++++ .../sqs_batch_wrapper_spec.rb | 91 +++++ ...llments_ready_for_status_check_job_spec.rb | 370 ++++++++++++++++++ 18 files changed, 1613 insertions(+), 1 deletion(-) create mode 100644 app/jobs/in_person/enrollments_ready_for_status_check/batch_processor.rb create mode 100644 app/jobs/in_person/enrollments_ready_for_status_check/enrollment_pipeline.rb create mode 100644 app/jobs/in_person/enrollments_ready_for_status_check/error_reporter.rb create mode 100644 app/jobs/in_person/enrollments_ready_for_status_check/sqs_batch_wrapper.rb create mode 100644 app/jobs/in_person/enrollments_ready_for_status_check_job.rb create mode 100644 db/primary_migrate/20230503231037_add_ready_for_status_check_to_in_person_enrollments.rb create mode 100644 spec/jobs/in_person/enrollments_ready_for_status_check/batch_processor_spec.rb create mode 100644 spec/jobs/in_person/enrollments_ready_for_status_check/enrollment_pipeline_spec.rb create mode 100644 spec/jobs/in_person/enrollments_ready_for_status_check/error_reporter_spec.rb create mode 100644 spec/jobs/in_person/enrollments_ready_for_status_check/sqs_batch_wrapper_spec.rb create mode 100644 spec/jobs/in_person/enrollments_ready_for_status_check_job_spec.rb diff --git a/Gemfile b/Gemfile index a4e3b0438af..5a89e48b48d 100644 --- a/Gemfile +++ b/Gemfile @@ -11,6 +11,7 @@ gem 'aws-sdk-pinpoint' gem 'aws-sdk-pinpointsmsvoice' gem 'aws-sdk-ses', '~> 1.6' gem 'aws-sdk-sns' +gem 'aws-sdk-sqs' gem 'barby', '~> 0.6.8' gem 'base32-crockford' gem 'bootsnap', '~> 1.0', require: false @@ -35,6 +36,7 @@ gem 'jwt' gem 'lograge', '>= 0.11.2' gem 'lookbook', '~> 1.5.3', require: false gem 'lru_redux' +gem 'mail' gem 'msgpack', '~> 1.6' gem 'maxminddb' gem 'multiset' diff --git a/Gemfile.lock b/Gemfile.lock index 36f61680e8d..dd8c882db42 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -164,6 +164,9 @@ GEM aws-sdk-sns (1.49.0) aws-sdk-core (~> 3, >= 3.122.0) aws-sigv4 (~> 1.1) + aws-sdk-sqs (1.53.0) + aws-sdk-core (~> 3, >= 3.165.0) + aws-sigv4 (~> 1.1) aws-sigv4 (1.5.2) aws-eventstream (~> 1, >= 1.0.2) axe-core-api (4.3.2) @@ -727,6 +730,7 @@ DEPENDENCIES aws-sdk-pinpointsmsvoice aws-sdk-ses (~> 1.6) aws-sdk-sns + aws-sdk-sqs axe-core-rspec (~> 4.2) barby (~> 0.6.8) base32-crockford @@ -768,6 +772,7 @@ DEPENDENCIES lograge (>= 0.11.2) lookbook (~> 1.5.3) lru_redux + mail maxminddb msgpack (~> 1.6) multiset diff --git a/app/jobs/in_person/enrollments_ready_for_status_check/batch_processor.rb b/app/jobs/in_person/enrollments_ready_for_status_check/batch_processor.rb new file mode 100644 index 00000000000..2098bda3cae --- /dev/null +++ b/app/jobs/in_person/enrollments_ready_for_status_check/batch_processor.rb @@ -0,0 +1,75 @@ +module InPerson::EnrollmentsReadyForStatusCheck + class BatchProcessor + # @param [InPerson::EnrollmentsReadyForStatusCheck::ErrorReporter] error_reporter + # @param [InPerson::EnrollmentsReadyForStatusCheck::SqsBatchWrapper] sqs_batch_wrapper + # @param [InPerson::EnrollmentsReadyForStatusCheck::EnrollmentPipeline] enrollment_pipeline + def initialize(error_reporter:, sqs_batch_wrapper:, enrollment_pipeline:) + @error_reporter = error_reporter + @sqs_batch_wrapper = sqs_batch_wrapper + @enrollment_pipeline = enrollment_pipeline + end + + # Process a batch of incoming messages corresponding to in-person + # enrollments that are ready to have their status checked. + # + # Note: Stats are accepted as param to increment and facilitate logging even + # if this method raises an error. + # + # @param [Array] messages In-person enrollment SQS messages + # @param [Hash] analytics_stats Counters for aggregating info about how items were processed + # @option analytics_stats [Integer] :fetched_items Items received from SQS + # @option analytics_stats [Integer] :valid_items Items matching the expected format/data + # @option analytics_stats [Integer] :invalid_items Items not matching the expected format/data + # @option analytics_stats [Integer] :processed_items Items processed without errors + # @option analytics_stats [Integer] :deleted_items Items successfully deleted from queue + def process_batch(messages, analytics_stats) + analytics_stats[:fetched_items] += messages.size + # Keep messages to delete in an array for a batch call + deletion_batch = [] + messages.each do |sqs_message| + if process_message(sqs_message) + analytics_stats[:valid_items] += 1 + else + analytics_stats[:invalid_items] += 1 + end + + # Append messages to batch so we can dequeue any that we've processed. + # + # If we fail to process the message now but could process it later, then + # we should exclude that message from the deletion batch. + deletion_batch.append(sqs_message) + analytics_stats[:processed_items] += 1 + end + ensure + # The messages were processed, so remove them from the queue + analytics_stats[:deleted_items] += process_deletions(deletion_batch) + end + + private + + attr_reader :error_reporter, :sqs_batch_wrapper, :enrollment_pipeline + + delegate :report_error, to: :error_reporter + delegate :delete_message_batch, to: :sqs_batch_wrapper + delegate :process_message, to: :enrollment_pipeline + + # Delete messages from the queue and report deletion errors + # @param [Array] deletion_batch SQS messages to delete + # @return [Integer] Number of items deleted + def process_deletions(deletion_batch) + return 0 if deletion_batch.empty? + + delete_result = delete_message_batch(deletion_batch) + delete_result.failed.each do |error_entry| + report_error( + 'Failed to delete item from queue', + sqs_delete_error: error_entry.to_h, + ) + end + delete_result.successful.size + rescue StandardError => err + report_error(err) + 0 + end + end +end diff --git a/app/jobs/in_person/enrollments_ready_for_status_check/enrollment_pipeline.rb b/app/jobs/in_person/enrollments_ready_for_status_check/enrollment_pipeline.rb new file mode 100644 index 00000000000..a03d5b56e13 --- /dev/null +++ b/app/jobs/in_person/enrollments_ready_for_status_check/enrollment_pipeline.rb @@ -0,0 +1,145 @@ +module InPerson::EnrollmentsReadyForStatusCheck + class EnrollmentPipeline + # @param [InPerson::EnrollmentsReadyForStatusCheck::ErrorReporter] error_reporter + # @param [Regexp] email_body_pattern Pattern matching the expected email body + # Note: email_body_pattern must include a capture group named "enrollment_code" + def initialize(error_reporter:, email_body_pattern:) + @error_reporter = error_reporter + @email_body_pattern = email_body_pattern + end + + # Process a message from USPS indicating that an in-person + # enrollment is ready to have its status checked. + # + # When a message can't be processed, then this function will return + # false to indicate that a problem occurred with the message. Otherwise it + # will return true. If an error occurs that can't be clearly identified + # as a problem with the message, then an error will be raised instead. + # + # When a message is successfully processed, then the corresponding + # InPersonEnrollment record will be marked as ready for a status check. + # + # @param [Aws::SQS::Types::Message] sqs_message + # @return [Boolean] Returns false for messages that can't be processed + # @raise [StandardError] Raised when an unhandled error occurs + def process_message(sqs_message) + error_extra = { + sqs_message_id: sqs_message.message_id, + } + + # Unwrap SQS message to get SNS message + begin + sns_message = JSON.parse(sqs_message.body, { symbolize_names: true }) + rescue JSON::JSONError => err + report_error(err, **error_extra) + return false + end + + unless sns_message.is_a?(Hash) && sns_message.key?(:MessageId) && sns_message.key?(:Message) + report_error('SQS message body is not valid SNS payload', **error_extra) + return false + end + + error_extra[:sns_message_id] = sns_message[:MessageId] + + # Unwrap SNS message to get SES message + begin + ses_message = JSON.parse(sns_message[:Message], { symbolize_names: true }) + rescue JSON::JSONError => err + report_error(err, **error_extra) + return false + end + + unless ses_message.is_a?(Hash) && ses_message.key?(:content) && ses_message.key?(:mail) + report_error('SNS "Message" field is not a valid SES payload', **error_extra) + return false + end + + # Add information to help with debugging + error_extra[:ses_aws_message_id] = ses_message.dig(:mail, :messageId) + error_extra[:ses_mail_timestamp] = ses_message.dig(:mail, :timestamp) + error_extra[:ses_mail_source] = ses_message.dig(:mail, :source) + + # https://datatracker.ietf.org/doc/html/rfc5322#section-3.6.1 + error_extra[:ses_rfc_origination_date] = ses_message. + dig(:mail, :commonHeaders, :date)&.then do |date| + DateTime.parse(date).to_s + end + # https://datatracker.ietf.org/doc/html/rfc5322#section-3.6.4 + error_extra[:ses_rfc_message_id] = ses_message.dig(:mail, :commonHeaders, :messageId) + + # Parse email from content of SES message + begin + mail = Mail.read_from_string(ses_message[:content]) + # Depending on how the email is created, we may need to read different + # parts of the message + if mail.multipart? + text_body = mail.text_part&.decoded + else + text_body = mail.body.decoded + end + rescue StandardError + report_error(err, **error_extra) + return false + end + + unless text_body.respond_to?(:to_s) && text_body.to_s.present? + report_error('Failure occurred when attempting to get email body', **error_extra) + return false + end + + # Extract enrollment code from email body + enrollment_code = email_body_pattern.match(text_body.to_s)&.[](:enrollment_code) + + unless enrollment_code.is_a?(String) + report_error( + 'Failed to extract enrollment code using regex, check email body format and regex', + **error_extra, + ) + return false + end + + error_extra[:enrollment_code] = enrollment_code + + # Look up existing enrollment + id, user_id, ready_for_status_check = InPersonEnrollment. + where(enrollment_code:, status: :pending). + order(created_at: :desc). + limit(1). + pick( + :id, :user_id, :ready_for_status_check + ) + + if id.nil? + report_error( + 'Received code for enrollment that does not exist in the database', + **error_extra, + ) + return false + end + + error_extra[:enrollment_id] = id + error_extra[:user_id] = user_id + + # SQS can deliver the message multiple times, so it's expected that + # sometimes ready_for_status_check will already be set to true. + unless ready_for_status_check + # Mark enrollment as ready for the USPS status check. + # + # Note: There is still a chance of duplicate updates at this point, + # but the conditional above should catch most cases. + InPersonEnrollment.update(id, ready_for_status_check: true) + end + return true + rescue StandardError => err + # Report and re-throw unhandled errors + report_error(err, **error_extra) + raise err + end + + private + + attr_reader :error_reporter, :email_body_pattern + delegate :report_error, to: :error_reporter + end +end diff --git a/app/jobs/in_person/enrollments_ready_for_status_check/error_reporter.rb b/app/jobs/in_person/enrollments_ready_for_status_check/error_reporter.rb new file mode 100644 index 00000000000..31c2f2d2f1a --- /dev/null +++ b/app/jobs/in_person/enrollments_ready_for_status_check/error_reporter.rb @@ -0,0 +1,29 @@ +module InPerson + module EnrollmentsReadyForStatusCheck + class ErrorReporter + # @param [String] class_name Class for which to report errors + # @param [Analytics] analytics + def initialize(class_name, analytics) + @class_name = class_name + @analytics = analytics + end + + # Reports an error. A non-StandardError will be converted to + # a RuntimeError before being reported. + # @param [#to_s,StandardError] error + def report_error(error, **extra) + error = RuntimeError.new("#{@class_name}: #{error}") unless error.is_a?(StandardError) + analytics.idv_in_person_proofing_enrollments_ready_for_status_check_job_ingestion_error( + exception_class: error.class, + exception_message: error.message, + **extra, + ) + NewRelic::Agent.notice_error(error) + end + + private + + attr_reader :analytics + end + end +end diff --git a/app/jobs/in_person/enrollments_ready_for_status_check/sqs_batch_wrapper.rb b/app/jobs/in_person/enrollments_ready_for_status_check/sqs_batch_wrapper.rb new file mode 100644 index 00000000000..6c834b08b2f --- /dev/null +++ b/app/jobs/in_person/enrollments_ready_for_status_check/sqs_batch_wrapper.rb @@ -0,0 +1,37 @@ +module InPerson::EnrollmentsReadyForStatusCheck + class SqsBatchWrapper + # @param [Aws::SQS::Client] sqs_client AWS SQS Client + # @param [String] queue_url The URL identifying the SQS queue + # @param [Hash] receive_params Parameters passed to #receive_message + def initialize(sqs_client:, queue_url:, receive_params:) + @sqs_client = sqs_client + @queue_url = queue_url + @receive_params = receive_params + end + + # Fetch a batch of messages from the SQS queue + # @return [Array] + def poll + sqs_client.receive_message(receive_params).messages + end + + # Delete the provided messages from the SQS queue + # @param [Array] batch + # @return [Aws::SQS::Types::DeleteMessageBatchResult] + def delete_message_batch(batch) + sqs_client.delete_message_batch( + queue_url:, + entries: batch.map do |message| + { + id: message.message_id, + receipt_handle: message.receipt_handle, + } + end, + ) + end + + private + + attr_reader :sqs_client, :queue_url, :receive_params + end +end diff --git a/app/jobs/in_person/enrollments_ready_for_status_check_job.rb b/app/jobs/in_person/enrollments_ready_for_status_check_job.rb new file mode 100644 index 00000000000..3c7c48d1988 --- /dev/null +++ b/app/jobs/in_person/enrollments_ready_for_status_check_job.rb @@ -0,0 +1,107 @@ +module InPerson + # This job checks a queue regularly to determine whether USPS has notitied us + # about whether an in-person enrollment is ready to have its status checked. If + # the enrollment is ready, then this job updates a flag on the enrollment so that it + # will be checked earlier than other enrollments. + class EnrollmentsReadyForStatusCheckJob < ApplicationJob + queue_as :low + + def perform(_now) + return true if IdentityConfig.store.in_person_proofing_enabled.blank? || + IdentityConfig.store.in_person_enrollments_ready_job_enabled.blank? + + begin + analytics.idv_in_person_proofing_enrollments_ready_for_status_check_job_started + + analytics_stats = { + fetched_items: 0, + processed_items: 0, + deleted_items: 0, + valid_items: 0, + invalid_items: 0, + } + + # Continually request messages until no messages are received + while (messages = poll).any? + process_batch(messages, analytics_stats) + end + return true + ensure + analytics.idv_in_person_proofing_enrollments_ready_for_status_check_job_completed( + **analytics_stats, + incomplete_items: + analytics_stats[:fetched_items] - analytics_stats[:processed_items], + deletion_failed_items: + analytics_stats[:processed_items] - analytics_stats[:deleted_items], + ) + end + end + + private + + delegate :poll, to: :sqs_batch_wrapper + delegate :process_batch, to: :batch_processor + delegate :analytics, to: :analytics_factory + + def batch_processor + @batch_processor ||= begin + InPerson::EnrollmentsReadyForStatusCheck::BatchProcessor.new( + error_reporter: InPerson::EnrollmentsReadyForStatusCheck::ErrorReporter.new( + InPerson::EnrollmentsReadyForStatusCheck::BatchProcessor.name, + analytics, + ), + sqs_batch_wrapper: sqs_batch_wrapper, + enrollment_pipeline: InPerson::EnrollmentsReadyForStatusCheck::EnrollmentPipeline.new( + error_reporter: InPerson::EnrollmentsReadyForStatusCheck::ErrorReporter.new( + InPerson::EnrollmentsReadyForStatusCheck::EnrollmentPipeline.name, + analytics, + ), + email_body_pattern: Regexp.new( + # Regex pattern describing the expected email format. + # This should include an "enrollment_code" capture group. + IdentityConfig.store.in_person_enrollments_ready_job_email_body_pattern, + ), + ), + ) + end + end + + def sqs_batch_wrapper + @sqs_batch_wrapper ||= begin + config = IdentityConfig.store + queue_url = config.in_person_enrollments_ready_job_queue_url + max_number_of_messages = config.in_person_enrollments_ready_job_max_number_of_messages + visibility_timeout = config.in_person_enrollments_ready_job_visibility_timeout_seconds + wait_time_seconds = config.in_person_enrollments_ready_job_wait_time_seconds + + # The queue will need to remain connected for at least the duration set + # by wait_time_seconds, which may conflict with aws_http_timeout. + # + # Adding the two together here to create a buffer. + http_read_timeout = config.aws_http_timeout + wait_time_seconds + + InPerson::EnrollmentsReadyForStatusCheck::SqsBatchWrapper.new( + sqs_client: Aws::SQS::Client.new( + http_read_timeout:, + ), + queue_url:, + receive_params: { + queue_url:, + max_number_of_messages:, + visibility_timeout:, + wait_time_seconds:, + }, + ) + end + end + + def analytics + @analytics ||= Analytics.new( + user: AnonymousUser.new, + request: nil, + session: {}, + sp: nil, + ) + end + end +end diff --git a/app/services/analytics_events.rb b/app/services/analytics_events.rb index 5d8db363b16..457a6dd8406 100644 --- a/app/services/analytics_events.rb +++ b/app/services/analytics_events.rb @@ -947,6 +947,62 @@ def idv_in_person_ready_to_verify_what_to_bring_link_clicked(**extra) ) end + # A job to check USPS notifications about in-person enrollment status updates has started + def idv_in_person_proofing_enrollments_ready_for_status_check_job_started(**extra) + track_event( + 'InPersonEnrollmentsReadyForStatusCheckJob: Job started', + **extra, + ) + end + + # A job to check USPS notifications about in-person enrollment status updates has completed + # @param [Integer] fetched_items items fetched + # @param [Integer] processed_items items fetched and processed + # @param [Integer] deleted_items items fetched, processed, and then deleted from the queue + # @param [Integer] valid_items items that could be successfully used to update a record + # @param [Integer] invalid_items items that couldn't be used to update a record + # @param [Integer] incomplete_items fetched items not processed nor deleted from the queue + # @param [Integer] deletion_failed_items processed items that we failed to delete + def idv_in_person_proofing_enrollments_ready_for_status_check_job_completed( + fetched_items:, + processed_items:, + deleted_items:, + valid_items:, + invalid_items:, + incomplete_items:, + deletion_failed_items:, + **extra + ) + track_event( + 'InPersonEnrollmentsReadyForStatusCheckJob: Job completed', + fetched_items:, + processed_items:, + deleted_items:, + valid_items:, + invalid_items:, + incomplete_items:, + deletion_failed_items:, + **extra, + ) + end + + # A job to check USPS notifications about in-person enrollment status updates + # has encountered an error + # @param [String] exception_class + # @param [String] exception_message + def idv_in_person_proofing_enrollments_ready_for_status_check_job_ingestion_error( + exception_class:, + exception_message:, + **extra + ) + track_event( + 'InPersonEnrollmentsReadyForStatusCheckJob: Ingestion error', + exception_class:, + exception_message:, + **extra, + ) + end + # User has consented to share information with document upload and may # view the "hybrid handoff" step next unless "skip_upload" param is true def idv_doc_auth_agreement_submitted(**extra) diff --git a/config/application.yml.default b/config/application.yml.default index 409ca9adcbe..baf209db88f 100644 --- a/config/application.yml.default +++ b/config/application.yml.default @@ -139,6 +139,13 @@ in_person_email_reminder_final_benchmark_in_days: 1 in_person_email_reminder_late_benchmark_in_days: 4 in_person_proofing_enabled: false in_person_enrollment_validity_in_days: 30 +in_person_enrollments_ready_job_email_body_pattern: '\A\s*(?\d{16})\s*\Z' +in_person_enrollments_ready_job_cron: '0/10 * * * *' +in_person_enrollments_ready_job_enabled: false +in_person_enrollments_ready_job_queue_url: '' +in_person_enrollments_ready_job_max_number_of_messages: 10 +in_person_enrollments_ready_job_visibility_timeout_seconds: 30 +in_person_enrollments_ready_job_wait_time_seconds: 20 in_person_results_delay_in_hours: 1 in_person_completion_survey_url: 'https://login.gov' in_person_usps_outage_message_enabled: false diff --git a/config/initializers/job_configurations.rb b/config/initializers/job_configurations.rb index 833f94744d9..675bb6c44ec 100644 --- a/config/initializers/job_configurations.rb +++ b/config/initializers/job_configurations.rb @@ -104,6 +104,12 @@ class: 'HeartbeatJob', cron: cron_5m, }, + # Queue usps in-person visit notifications job to GoodJob + in_person_enrollments_ready_for_status_check_job: { + class: 'InPerson::EnrollmentsReadyForStatusCheckJob', + cron: IdentityConfig.store.in_person_enrollments_ready_job_cron, + args: -> { [Time.zone.now] }, + }, # Queue usps proofing job to GoodJob get_usps_proofing_results_job: { class: 'GetUspsProofingResultsJob', diff --git a/db/primary_migrate/20230503231037_add_ready_for_status_check_to_in_person_enrollments.rb b/db/primary_migrate/20230503231037_add_ready_for_status_check_to_in_person_enrollments.rb new file mode 100644 index 00000000000..db8b2e0918c --- /dev/null +++ b/db/primary_migrate/20230503231037_add_ready_for_status_check_to_in_person_enrollments.rb @@ -0,0 +1,7 @@ +class AddReadyForStatusCheckToInPersonEnrollments < ActiveRecord::Migration[7.0] + disable_ddl_transaction! + def change + add_column :in_person_enrollments, :ready_for_status_check, :boolean, default: false + add_index :in_person_enrollments, :ready_for_status_check, where: "(ready_for_status_check = true)", algorithm: :concurrently + end +end diff --git a/db/schema.rb b/db/schema.rb index 6cd4ba7f485..0135afe0ba2 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema[7.0].define(version: 2023_05_02_235856) do +ActiveRecord::Schema[7.0].define(version: 2023_05_03_231037) do # These are extensions that must be enabled in order to support this database enable_extension "pg_stat_statements" enable_extension "pgcrypto" @@ -308,7 +308,9 @@ t.datetime "proofed_at", precision: nil, comment: "timestamp when user attempted to proof at a Post Office" t.boolean "capture_secondary_id_enabled", default: false, comment: "record and proof state ID and residential addresses separately" t.datetime "status_check_completed_at", comment: "The last time a status check was successfully completed" + t.boolean "ready_for_status_check", default: false t.index ["profile_id"], name: "index_in_person_enrollments_on_profile_id" + t.index ["ready_for_status_check"], name: "index_in_person_enrollments_on_ready_for_status_check", where: "(ready_for_status_check = true)" t.index ["status_check_attempted_at"], name: "index_in_person_enrollments_on_status_check_attempted_at", where: "(status = 1)" t.index ["unique_id"], name: "index_in_person_enrollments_on_unique_id", unique: true t.index ["user_id", "status"], name: "index_in_person_enrollments_on_user_id_and_status", unique: true, where: "(status = 1)" diff --git a/lib/identity_config.rb b/lib/identity_config.rb index d8f59420682..e832cecdbf7 100644 --- a/lib/identity_config.rb +++ b/lib/identity_config.rb @@ -218,6 +218,13 @@ def self.build_store(config_map) config.add(:in_person_email_reminder_late_benchmark_in_days, type: :integer) config.add(:in_person_proofing_enabled, type: :boolean) config.add(:in_person_enrollment_validity_in_days, type: :integer) + config.add(:in_person_enrollments_ready_job_email_body_pattern, type: :string) + config.add(:in_person_enrollments_ready_job_cron, type: :string) + config.add(:in_person_enrollments_ready_job_enabled, type: :boolean) + config.add(:in_person_enrollments_ready_job_queue_url, type: :string) + config.add(:in_person_enrollments_ready_job_max_number_of_messages, type: :integer) + config.add(:in_person_enrollments_ready_job_visibility_timeout_seconds, type: :integer) + config.add(:in_person_enrollments_ready_job_wait_time_seconds, type: :integer) config.add(:in_person_results_delay_in_hours, type: :integer) config.add(:in_person_completion_survey_url, type: :string) config.add(:in_person_usps_outage_message_enabled, type: :boolean) diff --git a/spec/jobs/in_person/enrollments_ready_for_status_check/batch_processor_spec.rb b/spec/jobs/in_person/enrollments_ready_for_status_check/batch_processor_spec.rb new file mode 100644 index 00000000000..6eced3d0342 --- /dev/null +++ b/spec/jobs/in_person/enrollments_ready_for_status_check/batch_processor_spec.rb @@ -0,0 +1,229 @@ +require 'rails_helper' + +RSpec.describe InPerson::EnrollmentsReadyForStatusCheck::BatchProcessor do + let(:messages) { [] } + let(:analytics_stats) do + { + fetched_items: 0, + processed_items: 0, + deleted_items: 0, + valid_items: 0, + invalid_items: 0, + } + end + + let(:error_reporter) { instance_double(InPerson::EnrollmentsReadyForStatusCheck::ErrorReporter) } + let(:sqs_batch_wrapper) do + instance_double(InPerson::EnrollmentsReadyForStatusCheck::SqsBatchWrapper) + end + let(:enrollment_pipeline) do + instance_double(InPerson::EnrollmentsReadyForStatusCheck::EnrollmentPipeline) + end + + subject(:batch_processor) do + described_class.new( + error_reporter:, + sqs_batch_wrapper:, + enrollment_pipeline:, + ) + end + + describe '#process_batch' do + let(:delete_result) { instance_double(Aws::SQS::Types::DeleteMessageBatchResult) } + + def successful_delete + instance_double(Aws::SQS::Types::DeleteMessageBatchResultEntry) + end + + def failed_delete + instance_double(Aws::SQS::Types::BatchResultErrorEntry) + end + + it 'ignores an empty batch' do + expect(enrollment_pipeline).not_to receive(:process_message) + expect(sqs_batch_wrapper).not_to receive(:delete_message_batch) + expect(error_reporter).not_to receive(:report_error) + expected_analytics_stats = analytics_stats.dup + batch_processor.process_batch(messages, analytics_stats) + expect(analytics_stats).to eq(expected_analytics_stats) + end + + context 'one message' do + let(:message) { instance_double(Aws::SQS::Types::Message) } + let(:messages) { [message] } + + it 'unhandled exception does not delete item' do + error = RuntimeError.new 'test error' + expect(enrollment_pipeline).to receive(:process_message).with(message).and_raise(error).once + expect(sqs_batch_wrapper).not_to receive(:delete_message_batch) + expect(error_reporter).not_to receive(:report_error) + expected_analytics_stats = { + **analytics_stats, + fetched_items: 1, + } + expect do + batch_processor.process_batch(messages, analytics_stats) + end.to raise_error(error) + expect(analytics_stats).to eq(expected_analytics_stats) + end + + it 'invalid item is marked as processed and deleted' do + expect(enrollment_pipeline).to receive(:process_message). + with(message).and_return(false).once + expect(sqs_batch_wrapper).to receive(:delete_message_batch). + with(messages).and_return(delete_result).once + expect(delete_result).to receive(:failed).and_return([]) + expect(delete_result).to receive(:successful).and_return(messages) + expect(error_reporter).not_to receive(:report_error) + expected_analytics_stats = { + **analytics_stats, + fetched_items: 1, + invalid_items: 1, + deleted_items: 1, + processed_items: 1, + } + batch_processor.process_batch(messages, analytics_stats) + expect(analytics_stats).to eq(expected_analytics_stats) + end + + it 'valid item is marked as processed and deleted' do + expect(enrollment_pipeline).to receive(:process_message). + with(message).and_return(true).once + expect(sqs_batch_wrapper).to receive(:delete_message_batch). + with(messages).and_return(delete_result).once + expect(delete_result).to receive(:failed).and_return([]) + expect(delete_result).to receive(:successful).and_return( + [ + successful_delete, + ], + ) + expect(error_reporter).not_to receive(:report_error) + expected_analytics_stats = { + **analytics_stats, + fetched_items: 1, + valid_items: 1, + deleted_items: 1, + processed_items: 1, + } + batch_processor.process_batch(messages, analytics_stats) + expect(analytics_stats).to eq(expected_analytics_stats) + end + + it 'item is marked as processed but fails to be deleted' do + expect(enrollment_pipeline).to receive(:process_message). + with(message).and_return(true).once + expect(sqs_batch_wrapper).to receive(:delete_message_batch). + with(messages).and_return(delete_result).once + error_entry = failed_delete + expect(delete_result).to receive(:failed).and_return( + [ + error_entry, + ], + ) + error_entry_hash = { + id: 123, + } + expect(error_entry).to receive(:to_h).and_return(error_entry_hash) + expect(delete_result).to receive(:successful).and_return([]) + expect(error_reporter).to receive(:report_error).with( + 'Failed to delete item from queue', + sqs_delete_error: error_entry_hash, + ).once + expected_analytics_stats = { + **analytics_stats, + fetched_items: 1, + valid_items: 1, + deleted_items: 0, + processed_items: 1, + } + batch_processor.process_batch(messages, analytics_stats) + expect(analytics_stats).to eq(expected_analytics_stats) + end + + it 'item is marked as processed but the batch delete call throws an error' do + error = RuntimeError.new 'test batch error' + expect(enrollment_pipeline).to receive(:process_message). + with(message).and_return(true).once + expect(sqs_batch_wrapper).to receive(:delete_message_batch). + with(messages).and_raise(error).once + expect(error_reporter).to receive(:report_error).with(error).once + expected_analytics_stats = { + **analytics_stats, + fetched_items: 1, + valid_items: 1, + deleted_items: 0, + processed_items: 1, + } + batch_processor.process_batch(messages, analytics_stats) + expect(analytics_stats).to eq(expected_analytics_stats) + end + end + + context 'multiple messages' do + let(:message) { instance_double(Aws::SQS::Types::Message) } + let(:messages) do + [ + instance_double(Aws::SQS::Types::Message), + instance_double(Aws::SQS::Types::Message), + instance_double(Aws::SQS::Types::Message), + instance_double(Aws::SQS::Types::Message), + instance_double(Aws::SQS::Types::Message), + ] + end + + it 'handles combined valid, invalid, and non-deleted messages' do + expect(enrollment_pipeline).to receive(:process_message).and_return( + true, + false, + true, + true, + true, + ).exactly(5).times + expect(sqs_batch_wrapper).to receive(:delete_message_batch). + with(messages).and_return(delete_result).once + + error_entry = failed_delete + error_entry2 = failed_delete + expect(delete_result).to receive(:failed).and_return( + [ + error_entry, + error_entry2, + ], + ) + error_entry_hash = { + id: 123, + } + error_entry_hash2 = { + id: 456, + } + expect(error_entry).to receive(:to_h).and_return(error_entry_hash) + expect(error_entry2).to receive(:to_h).and_return(error_entry_hash2) + expect(delete_result).to receive(:successful).and_return( + [ + successful_delete, + successful_delete, + successful_delete, + ], + ) + expect(error_reporter).to receive(:report_error).with( + 'Failed to delete item from queue', + sqs_delete_error: error_entry_hash, + ).once + expect(error_reporter).to receive(:report_error).with( + 'Failed to delete item from queue', + sqs_delete_error: error_entry_hash2, + ).once + expected_analytics_stats = { + **analytics_stats, + fetched_items: 5, + valid_items: 4, + invalid_items: 1, + deleted_items: 3, + processed_items: 5, + } + batch_processor.process_batch(messages, analytics_stats) + expect(analytics_stats).to eq(expected_analytics_stats) + end + end + end +end diff --git a/spec/jobs/in_person/enrollments_ready_for_status_check/enrollment_pipeline_spec.rb b/spec/jobs/in_person/enrollments_ready_for_status_check/enrollment_pipeline_spec.rb new file mode 100644 index 00000000000..e8df2c9ebb4 --- /dev/null +++ b/spec/jobs/in_person/enrollments_ready_for_status_check/enrollment_pipeline_spec.rb @@ -0,0 +1,335 @@ +require 'rails_helper' + +RSpec.describe InPerson::EnrollmentsReadyForStatusCheck::EnrollmentPipeline do + let(:error_reporter) { instance_double(InPerson::EnrollmentsReadyForStatusCheck::ErrorReporter) } + let(:email_body_pattern) { /\A\s*(?\d{16})\s*\Z/ } + subject(:enrollment_pipeline) { described_class.new(error_reporter:, email_body_pattern:) } + + before(:each) do + allow(IdentityConfig.store).to receive(:in_person_enrollments_ready_job_email_body_pattern). + and_return('\A\s*(?\d{16})\s*\Z') + + allow(error_reporter).to receive(:report_error) + end + + describe '#process_message' do + let(:sqs_message) { instance_double(Aws::SQS::Types::Message) } + let(:sqs_message_id) { Random.uuid } + let(:sns_message_id) { Random.uuid } + let(:enrollment_code) { 16.times.map { rand(0..9) }.join } + let(:user) { create(:user) } + let(:user_id) { user.id } + let(:mail_date) { 16.hours.ago.to_datetime } + let(:ses_payload) do + { + content: Mail.new do |m| + m.text_part = enrollment_code + end.to_s, + mail: { + messageId: Random.uuid.delete('-'), + timestamp: DateTime.now.to_s, + source: 'testsource@example.com', + commonHeaders: { + date: Mail::DateField.new(mail_date).to_s, + messageId: Mail::Utilities.generate_message_id, + }, + }, + } + end + let(:logged_ses_values) do + { + ses_aws_message_id: ses_payload[:mail][:messageId], + ses_mail_source: ses_payload[:mail][:source], + ses_mail_timestamp: ses_payload[:mail][:timestamp], + ses_rfc_message_id: ses_payload[:mail][:commonHeaders][:messageId], + ses_rfc_origination_date: mail_date.to_s, + } + end + let(:sns_payload) do + { + MessageId: sns_message_id, + Message: ses_payload.to_json, + } + end + let(:expected_error) { nil } + let(:expected_error_extra) { nil } + + before(:each) do + allow(sqs_message).to receive(:message_id). + and_return(sqs_message_id) + end + + def expect_error(error, **extra) + expect(error_reporter).to receive(:report_error) do |err, err_extra| + expect(err).to eq(error) if error.is_a?(String) + expect(err.message).to eq(error.message) if error.is_a?(StandardError) + expect(err.class).to eq(error.class) + expect(err_extra).to eq(extra) + end + expect(enrollment_pipeline.process_message(sqs_message)).to be(false) + end + + context 'reports error and returns false' do + it 'SQS message is not JSON' do + allow(sqs_message).to receive(:body).and_return('not json') + expect_error(JSON::ParserError.new("unexpected token at 'not json'"), sqs_message_id:) + end + + it 'SQS message body is not a hash' do + allow(sqs_message).to receive(:body).and_return('abcd'.to_json) + expect_error('SQS message body is not valid SNS payload', sqs_message_id:) + end + + it 'SNS message is missing MessageId' do + allow(sqs_message).to receive(:body).and_return( + { + Message: 'abcd', + }.to_json, + ) + expect_error('SQS message body is not valid SNS payload', sqs_message_id:) + end + + it 'SNS message is missing Message' do + allow(sqs_message).to receive(:body).and_return( + { + MessageId: sns_message_id, + }.to_json, + ) + expect_error('SQS message body is not valid SNS payload', sqs_message_id:) + end + + it 'SNS "Message" field is not JSON' do + allow(sqs_message).to receive(:body).and_return( + { + MessageId: sns_message_id, + Message: 'not json', + }.to_json, + ) + expect_error( + JSON::ParserError.new("unexpected token at 'not json'"), sqs_message_id:, + sns_message_id: + ) + end + + it 'SES message is not a hash' do + allow(sqs_message).to receive(:body).and_return( + { + MessageId: sns_message_id, + Message: 'abcd'.to_json, + }.to_json, + ) + expect_error( + 'SNS "Message" field is not a valid SES payload', sqs_message_id:, + sns_message_id: + ) + end + + it 'SES message is missing "content" key' do + allow(sqs_message).to receive(:body).and_return( + { + MessageId: sns_message_id, + Message: { + mail: {}, + }.to_json, + }.to_json, + ) + expect_error( + 'SNS "Message" field is not a valid SES payload', sqs_message_id:, + sns_message_id: + ) + end + + it 'SES message is missing "mail" key' do + allow(sqs_message).to receive(:body).and_return( + { + MessageId: sns_message_id, + Message: { + content: 'abcd', + }.to_json, + }.to_json, + ) + expect_error( + 'SNS "Message" field is not a valid SES payload', + sqs_message_id:, + sns_message_id:, + ) + end + + it 'email content key is missing' do + payload = ses_payload + payload.delete(:content) + allow(sqs_message).to receive(:body).and_return( + { + MessageId: sns_message_id, + Message: { + **payload, + }.to_json, + }.to_json, + ) + expect_error( + 'SNS "Message" field is not a valid SES payload', sqs_message_id:, + sns_message_id: + ) + end + + it 'email body is missing (single part)' do + message = Mail.new + expect(message.multipart?).to be(false) + allow(sqs_message).to receive(:body).and_return( + { + MessageId: sns_message_id, + Message: { + **ses_payload, + content: message.to_s, + }.to_json, + }.to_json, + ) + expect_error( + 'Failure occurred when attempting to get email body', + sqs_message_id:, + sns_message_id:, + **logged_ses_values, + ) + end + + it 'email body is missing (multipart)' do + message = Mail.new do + html_part do + body nil + end + text_part do + body nil + end + end + expect(message.multipart?).to be(true) + allow(sqs_message).to receive(:body).and_return( + { + MessageId: sns_message_id, + Message: { + **ses_payload, + content: message.to_s, + }.to_json, + }.to_json, + ) + expect_error( + 'Failure occurred when attempting to get email body', + sqs_message_id:, + sns_message_id:, + **logged_ses_values, + ) + end + + it 'email body does not match pattern' do + allow(sqs_message).to receive(:body).and_return( + { + **sns_payload, + Message: { + **ses_payload, + content: Mail.new do |m| + m.text_part = 'abcd' + end.to_s, + }.to_json, + }.to_json, + ) + expect_error( + 'Failed to extract enrollment code using regex, check email body format and regex', + sqs_message_id:, + sns_message_id:, + **logged_ses_values, + ) + end + + it 'enrollment does not exist' do + allow(sqs_message).to receive(:body).and_return(sns_payload.to_json) + expect_error( + 'Received code for enrollment that does not exist in the database', + sqs_message_id:, + sns_message_id:, + enrollment_code:, + **logged_ses_values, + ) + end + end + + context 'reports and rethrows unhandled errors' do + it 'error thrown trying to fetch enrollment' do + allow(sqs_message).to receive(:body).and_return(sns_payload.to_json) + error = ActiveRecord::ConnectionNotEstablished.new + expect(InPersonEnrollment).to receive_message_chain( + :where, + :order, + :limit, + :pick, + ).and_raise(error) + + expect(error_reporter).to receive(:report_error). + with( + error, + sqs_message_id:, + sns_message_id:, + enrollment_code:, + **logged_ses_values, + ) + + expect do + enrollment_pipeline.process_message(sqs_message) + end.to raise_error(error) + end + + it 'error thrown trying to update enrollment' do + allow(sqs_message).to receive(:body).and_return(sns_payload.to_json) + + enrollment = create(:in_person_enrollment, enrollment_code:, status: :pending, user:) + + error = ActiveRecord::ConnectionNotEstablished.new + expect(InPersonEnrollment).to receive(:update). + with(enrollment.id, ready_for_status_check: true). + and_raise(error) + + expect(error_reporter).to receive(:report_error). + with( + error, + sqs_message_id:, + sns_message_id:, + enrollment_code:, + user_id:, + enrollment_id: enrollment.id, + **logged_ses_values, + ) + + expect do + enrollment_pipeline.process_message(sqs_message) + end.to raise_error(error) + end + end + + context 'returns true for records handled as expected' do + it 'marks non-ready record as ready' do + allow(sqs_message).to receive(:body).and_return(sns_payload.to_json) + + enrollment = create(:in_person_enrollment, enrollment_code:, status: :pending, user:) + + expect(InPersonEnrollment).to receive(:update). + with(enrollment.id, ready_for_status_check: true).once + + expect(error_reporter).not_to receive(:report_error) + + expect(enrollment_pipeline.process_message(sqs_message)).to be(true) + end + it 'leaves record already marked as ready' do + allow(sqs_message).to receive(:body).and_return(sns_payload.to_json) + + create( + :in_person_enrollment, enrollment_code:, status: :pending, user:, + ready_for_status_check: true + ) + + expect(InPersonEnrollment).not_to receive(:update) + + expect(error_reporter).not_to receive(:report_error) + + expect(enrollment_pipeline.process_message(sqs_message)).to be(true) + end + end + end +end diff --git a/spec/jobs/in_person/enrollments_ready_for_status_check/error_reporter_spec.rb b/spec/jobs/in_person/enrollments_ready_for_status_check/error_reporter_spec.rb new file mode 100644 index 00000000000..1c5b4a9707e --- /dev/null +++ b/spec/jobs/in_person/enrollments_ready_for_status_check/error_reporter_spec.rb @@ -0,0 +1,102 @@ +require 'rails_helper' + +RSpec.describe InPerson::EnrollmentsReadyForStatusCheck::ErrorReporter do + let(:class_name_suffix) { "TestClass#{[*('A'..'Z'), *('a'..'z')].sample(10).join}" } + + # We need the class name since it's part of what we're logging + let(:class_name) { "#{described_class.name.demodulize}#{class_name_suffix}" } + + let(:analytics) { FakeAnalytics.new } + subject(:error_reporter) { described_class.new(class_name, analytics) } + let(:analytics_extra) { nil } + let(:expected_error_class) { nil } + let(:expected_message) { nil } + + before(:each) do + allow(analytics).to receive( + :idv_in_person_proofing_enrollments_ready_for_status_check_job_ingestion_error, + ) + allow(NewRelic::Agent).to receive(:notice_error) + end + + def it_generates_and_records_the_error + expect(analytics).to have_received( + :idv_in_person_proofing_enrollments_ready_for_status_check_job_ingestion_error, + ).once do |exception_class:, exception_message:, **extra| + expect(exception_class).to eq(expected_error_class) + expect(exception_message).to eq(expected_message) + end + expect(NewRelic::Agent).to have_received(:notice_error).once do |error| + expect(error).to be_instance_of(expected_error_class) + expect(error.message).to eq(expected_message) + end + end + + def it_passes_expected_attributes_to_analytics + expect(analytics).to have_received( + :idv_in_person_proofing_enrollments_ready_for_status_check_job_ingestion_error, + ).once do |exception_class:, exception_message:, **extra| + expect(extra).to eq(analytics_extra) + end + end + + describe '#report_error' do + context 'given a string message' do + let(:string_message) { 'my string message here' } + let(:expected_error_class) { RuntimeError } + let(:expected_message) { "#{class_name}: #{string_message}" } + let(:analytics_extra) { {} } + + before(:each) do + error_reporter.report_error(string_message, **analytics_extra) + end + + it 'generates an error object and records the error' do + it_generates_and_records_the_error + end + it 'passes the expected data to analytics' do + it_passes_expected_attributes_to_analytics + end + + context 'with extra analytics data' do + let(:analytics_extra) { { test: 'abcd' } } + + it 'generates an error object and records the error' do + it_generates_and_records_the_error + end + it 'passes the expected data to analytics' do + it_passes_expected_attributes_to_analytics + end + end + end + + context 'given an error' do + let(:error) { ArgumentError.new(expected_message) } + let(:expected_error_class) { ArgumentError } + let(:expected_message) { 'my test message' } + let(:analytics_extra) { {} } + + before(:each) do + error_reporter.report_error(error, **analytics_extra) + end + + it 'generates an error object and records the error' do + it_generates_and_records_the_error + end + it 'passes the expected data to analytics' do + it_passes_expected_attributes_to_analytics + end + + context 'with extra analytics data' do + let(:analytics_extra) { { test: 'abcd' } } + + it 'generates an error object and records the error' do + it_generates_and_records_the_error + end + it 'passes the expected data to analytics' do + it_passes_expected_attributes_to_analytics + end + end + end + end +end diff --git a/spec/jobs/in_person/enrollments_ready_for_status_check/sqs_batch_wrapper_spec.rb b/spec/jobs/in_person/enrollments_ready_for_status_check/sqs_batch_wrapper_spec.rb new file mode 100644 index 00000000000..f9ae2baf538 --- /dev/null +++ b/spec/jobs/in_person/enrollments_ready_for_status_check/sqs_batch_wrapper_spec.rb @@ -0,0 +1,91 @@ +require 'rails_helper' + +RSpec.describe InPerson::EnrollmentsReadyForStatusCheck::SqsBatchWrapper do + let(:queue_url) { 'my/test/queue/url' } + let(:sqs_client) { instance_double(Aws::SQS::Client) } + let(:receive_params) do + { + queue_url:, + max_number_of_messages: 10, + visibility_timeout: 30, + wait_time_seconds: 20, + } + end + subject(:sqs_batch_wrapper) { described_class.new(sqs_client:, queue_url:, receive_params:) } + + def create_mock_message + instance_double(Aws::SQS::Types::Message) + end + + describe '#poll' do + it 'polls SQS and returns messages' do + mock_result = instance_double(Aws::SQS::Types::ReceiveMessageResult) + mock_messages = [ + create_mock_message, + create_mock_message, + create_mock_message, + ] + + expect(sqs_client).to receive(:receive_message). + with(receive_params). + and_return(mock_result) + + expect(mock_result).to receive(:messages).and_return(mock_messages) + expect(sqs_batch_wrapper.poll).to eq(mock_messages) + end + end + + describe '#delete_message_batch' do + it 'deletes a batch of 1 from SQS' do + message_id = Random.uuid + receipt_handle = Random.uuid + message = create_mock_message + allow(message).to receive(:message_id).and_return(message_id) + allow(message).to receive(:receipt_handle).and_return(receipt_handle) + deletion_result = instance_double(Aws::SQS::Types::DeleteMessageBatchResult) + expect(sqs_client).to receive(:delete_message_batch).with( + { + queue_url:, + entries: [ + { + id: message_id, + receipt_handle: receipt_handle, + }, + ], + }, + ).and_return(deletion_result) + expect(sqs_batch_wrapper.delete_message_batch([message])).to be(deletion_result) + end + it 'deletes a batch of 2 from SQS' do + message_id = Random.uuid + receipt_handle = Random.uuid + message = create_mock_message + allow(message).to receive(:message_id).and_return(message_id) + allow(message).to receive(:receipt_handle).and_return(receipt_handle) + + message_id2 = Random.uuid + receipt_handle2 = Random.uuid + message2 = create_mock_message + allow(message2).to receive(:message_id).and_return(message_id2) + allow(message2).to receive(:receipt_handle).and_return(receipt_handle2) + + deletion_result = instance_double(Aws::SQS::Types::DeleteMessageBatchResult) + expect(sqs_client).to receive(:delete_message_batch).with( + { + queue_url:, + entries: [ + { + id: message_id, + receipt_handle: receipt_handle, + }, + { + id: message_id2, + receipt_handle: receipt_handle2, + }, + ], + }, + ).and_return(deletion_result) + expect(sqs_batch_wrapper.delete_message_batch([message, message2])).to be(deletion_result) + end + end +end diff --git a/spec/jobs/in_person/enrollments_ready_for_status_check_job_spec.rb b/spec/jobs/in_person/enrollments_ready_for_status_check_job_spec.rb new file mode 100644 index 00000000000..429268793e7 --- /dev/null +++ b/spec/jobs/in_person/enrollments_ready_for_status_check_job_spec.rb @@ -0,0 +1,370 @@ +require 'rails_helper' + +RSpec.describe InPerson::EnrollmentsReadyForStatusCheckJob do + let(:in_person_proofing_enabled) { nil } + let(:in_person_enrollments_ready_job_enabled) { nil } + let(:analytics) { FakeAnalytics.new } + subject(:job) { described_class.new } + + describe '#perform' do + before(:each) do + allow(job).to receive(:analytics).and_return(analytics) + allow(IdentityConfig.store).to receive(:in_person_proofing_enabled). + and_return(in_person_proofing_enabled) + allow(IdentityConfig.store).to receive(:in_person_enrollments_ready_job_enabled). + and_return(in_person_enrollments_ready_job_enabled) + end + + def process_batch_result + { + fetched_items: 7, + processed_items: 7, + deleted_items: 4, + valid_items: 5, + invalid_items: 2, + } + end + + def new_message + instance_double(Aws::SQS::Types::Message) + end + + context 'in person proofing disabled' do + let(:in_person_proofing_enabled) { false } + let(:in_person_enrollments_ready_job_enabled) { true } + it 'returns true without doing anything' do + expect(analytics).not_to receive( + :idv_in_person_proofing_enrollments_ready_for_status_check_job_started, + ) + expect(analytics).not_to receive( + :idv_in_person_proofing_enrollments_ready_for_status_check_job_completed, + ) + expect(job).not_to receive(:poll) + expect(job).not_to receive(:process_batch) + expect(job.perform(Time.zone.now)).to be(true) + end + end + + context 'job disabled' do + let(:in_person_proofing_enabled) { true } + let(:in_person_enrollments_ready_job_enabled) { false } + it 'returns true without doing anything' do + expect(analytics).not_to receive( + :idv_in_person_proofing_enrollments_ready_for_status_check_job_started, + ) + expect(analytics).not_to receive( + :idv_in_person_proofing_enrollments_ready_for_status_check_job_completed, + ) + expect(job).not_to receive(:poll) + expect(job).not_to receive(:process_batch) + expect(job.perform(Time.zone.now)).to be(true) + end + end + + context 'in person proofing and job enabled' do + let(:in_person_proofing_enabled) { true } + let(:in_person_enrollments_ready_job_enabled) { true } + + it 'logs analytics for a run with zero batches' do + expect(job).to receive(:poll).and_return([]) + expect(analytics).to receive( + :idv_in_person_proofing_enrollments_ready_for_status_check_job_started, + ) + expect(analytics).to receive( + :idv_in_person_proofing_enrollments_ready_for_status_check_job_completed, + ).with( + fetched_items: 0, + processed_items: 0, + deleted_items: 0, + valid_items: 0, + invalid_items: 0, + incomplete_items: 0, + deletion_failed_items: 0, + ) + expect(job).not_to receive(:process_batch) + expect(job.perform(Time.zone.now)).to be(true) + end + + it 'logs analytics for a run with one batch' do + batch = [new_message] + expect(job).to receive(:poll).and_return(batch, []) + expect(analytics).to receive( + :idv_in_person_proofing_enrollments_ready_for_status_check_job_started, + ) + expect(job).to receive(:process_batch).with( + batch, + { + fetched_items: 0, + processed_items: 0, + deleted_items: 0, + valid_items: 0, + invalid_items: 0, + }, + ) do |_batch, analytics_stats| + analytics_stats.merge!(process_batch_result) + end.once + expect(analytics).to receive( + :idv_in_person_proofing_enrollments_ready_for_status_check_job_completed, + ).with( + fetched_items: 7, + processed_items: 7, + deleted_items: 4, + valid_items: 5, + invalid_items: 2, + incomplete_items: 0, + deletion_failed_items: 3, + ) + expect(job.perform(Time.zone.now)).to be(true) + end + + it 'logs analytics for a run with one batch that throws an error' do + batch = [new_message] + expect(job).to receive(:poll).and_return(batch) + expect(analytics).to receive( + :idv_in_person_proofing_enrollments_ready_for_status_check_job_started, + ) + error = RuntimeError.new('test error') + expect(job).to receive(:process_batch).with( + batch, + an_instance_of(Hash), + ) do |_batch, analytics_stats| + analytics_stats.merge!(process_batch_result) + raise error + end.once + expect(analytics).to receive( + :idv_in_person_proofing_enrollments_ready_for_status_check_job_completed, + ).with( + fetched_items: 7, + processed_items: 7, + deleted_items: 4, + valid_items: 5, + invalid_items: 2, + incomplete_items: 0, + deletion_failed_items: 3, + ) + expect { job.perform(Time.zone.now) }.to raise_error(error) + end + + it 'logs analytics for a run with three batches' do + batch = [new_message] + batch2 = [new_message] + batch3 = [new_message] + expect(job).to receive(:poll).and_return(batch, batch2, batch3, []) + expect(analytics).to receive( + :idv_in_person_proofing_enrollments_ready_for_status_check_job_started, + ) + expect(job).to receive(:process_batch) do |current_batch, analytics_stats| + if batch == current_batch + process_batch_result.each do |key, value| + analytics_stats[key] = value + analytics_stats[key] + end + elsif current_batch == batch2 || current_batch == batch3 + { + fetched_items: 3, + processed_items: 3, + deleted_items: 2, + valid_items: 3, + invalid_items: 0, + }.each do |key, value| + analytics_stats[key] = value + analytics_stats[key] + end + end + end.exactly(3).times + expect(analytics).to receive( + :idv_in_person_proofing_enrollments_ready_for_status_check_job_completed, + ).with( + fetched_items: 13, + processed_items: 13, + deleted_items: 8, + valid_items: 11, + invalid_items: 2, + incomplete_items: 0, + deletion_failed_items: 5, + ) + expect(job.perform(Time.zone.now)).to be(true) + end + + it 'logs analytics for a run with three batches with error' do + batch = [new_message] + batch2 = [new_message] + batch3 = [new_message] + expect(job).to receive(:poll).and_return(batch, batch2, batch3) + expect(analytics).to receive( + :idv_in_person_proofing_enrollments_ready_for_status_check_job_started, + ) + error = RuntimeError.new('test error') + expect(job).to receive(:process_batch) do |current_batch, analytics_stats| + if current_batch == batch + process_batch_result.each do |key, value| + analytics_stats[key] = value + analytics_stats[key] + end + elsif current_batch == batch3 + { + fetched_items: 3, + processed_items: 1, + deleted_items: 1, + valid_items: 1, + invalid_items: 0, + }.each do |key, value| + analytics_stats[key] = value + analytics_stats[key] + end + raise error + elsif current_batch == batch2 + { + fetched_items: 3, + processed_items: 3, + deleted_items: 2, + valid_items: 3, + invalid_items: 0, + }.each do |key, value| + analytics_stats[key] = value + analytics_stats[key] + end + end + end.exactly(3).times + expect(analytics).to receive( + :idv_in_person_proofing_enrollments_ready_for_status_check_job_completed, + ).with( + fetched_items: 13, + processed_items: 11, + deleted_items: 7, + valid_items: 9, + invalid_items: 2, + incomplete_items: 2, + deletion_failed_items: 4, + ) + expect { job.perform(Time.zone.now) }.to raise_error(error) + end + end + end + + # Normally we should stick to validating the contract and dependencies, + # but making an exception here because the construction of these classes + # is relatively complex; so expanding tests to additionally cover delegation + # and constructor calls. + # + # Also doing this b/c the codebase does not use an IoC framework like dry-system + # and there's not an established convention for creating factories. + + describe '#poll (private)' do + it 'delegates to sqs_batch_wrapper' do + sqs_batch_wrapper = instance_double(InPerson::EnrollmentsReadyForStatusCheck::SqsBatchWrapper) + poll_result = [] + expect(job).to receive(:sqs_batch_wrapper).and_return(sqs_batch_wrapper) + expect(sqs_batch_wrapper).to receive(:poll).and_return(poll_result).once + expect(job.send(:poll)).to be(poll_result) + end + end + + describe '#process_batch (private)' do + it 'delegates to batch_processor' do + batch_processor = instance_double(InPerson::EnrollmentsReadyForStatusCheck::BatchProcessor) + expect(job).to receive(:batch_processor).and_return(batch_processor) + messages = [] + analytics_stats = {} + expect(batch_processor).to receive(:process_batch).with(messages, analytics_stats).once + job.send(:process_batch, messages, analytics_stats) + end + end + + describe '#analytics (private)' do + it 'creates an analytics object' do + analytics = FakeAnalytics.new + expect(Analytics).to receive(:new).with( + user: instance_of(AnonymousUser), + request: nil, + session: {}, + sp: nil, + ).and_return(analytics) + expect(job.send(:analytics)).to be(analytics) + end + end + + describe '#sqs_batch_wrapper (private)' do + it 'creates SQS batch wrapper object with expected params' do + sqs_client = instance_double(Aws::SQS::Client) + + queue_url = 'test/queue/url' + max_number_of_messages = 10 + visibility_timeout_seconds = 30 + wait_time_seconds = 20 + aws_http_timeout = 5 + + expect(Aws::SQS::Client).to receive(:new). + with(http_read_timeout: wait_time_seconds + aws_http_timeout). + and_return(sqs_client) + + expect(IdentityConfig.store).to receive_messages( + aws_http_timeout:, + in_person_enrollments_ready_job_queue_url: queue_url, + in_person_enrollments_ready_job_max_number_of_messages: max_number_of_messages, + in_person_enrollments_ready_job_visibility_timeout_seconds: visibility_timeout_seconds, + in_person_enrollments_ready_job_wait_time_seconds: wait_time_seconds, + ) + + wrapper = instance_double(InPerson::EnrollmentsReadyForStatusCheck::SqsBatchWrapper) + expect(InPerson::EnrollmentsReadyForStatusCheck::SqsBatchWrapper).to receive(:new). + with( + sqs_client: sqs_client, + queue_url:, + receive_params: { + queue_url:, + max_number_of_messages:, + visibility_timeout: visibility_timeout_seconds, + wait_time_seconds:, + }, + ).and_return(wrapper) + expect(job.send(:sqs_batch_wrapper)).to be(wrapper) + end + end + + describe '#batch_processor (private)' do + it 'creates a batch processor with the expected arguments' do + analytics = FakeAnalytics.new + expect(job).to receive(:analytics).and_return(analytics).exactly(2).times + + batch_processor_error_reporter = instance_double( + InPerson::EnrollmentsReadyForStatusCheck::ErrorReporter, + ) + expect(InPerson::EnrollmentsReadyForStatusCheck::ErrorReporter).to receive(:new). + with( + InPerson::EnrollmentsReadyForStatusCheck::BatchProcessor.name, + analytics, + ).and_return(batch_processor_error_reporter) + + sqs_batch_wrapper = instance_double(InPerson::EnrollmentsReadyForStatusCheck::SqsBatchWrapper) + expect(job).to receive(:sqs_batch_wrapper).and_return(sqs_batch_wrapper) + + enrollment_pipeline_error_reporter = instance_double( + InPerson::EnrollmentsReadyForStatusCheck::ErrorReporter, + ) + expect(InPerson::EnrollmentsReadyForStatusCheck::ErrorReporter).to receive(:new). + with( + InPerson::EnrollmentsReadyForStatusCheck::EnrollmentPipeline.name, + analytics, + ).and_return(enrollment_pipeline_error_reporter) + + email_body_pattern = 'abcd' + expect(IdentityConfig.store).to receive(:in_person_enrollments_ready_job_email_body_pattern). + and_return(email_body_pattern) + + enrollment_pipeline = instance_double( + InPerson::EnrollmentsReadyForStatusCheck::EnrollmentPipeline, + ) + expect(InPerson::EnrollmentsReadyForStatusCheck::EnrollmentPipeline).to receive(:new). + with( + error_reporter: enrollment_pipeline_error_reporter, + email_body_pattern: /abcd/, + ).and_return(enrollment_pipeline) + + batch_processor = instance_double(InPerson::EnrollmentsReadyForStatusCheck::BatchProcessor) + expect(InPerson::EnrollmentsReadyForStatusCheck::BatchProcessor).to receive(:new). + with( + error_reporter: batch_processor_error_reporter, + sqs_batch_wrapper:, + enrollment_pipeline:, + ).and_return(batch_processor) + + expect(job.send(:batch_processor)).to be(batch_processor) + end + end +end From a6728ff9e4f718ae5877e77242bef76d7d46b2a3 Mon Sep 17 00:00:00 2001 From: Eric Gade <105373963+eric-gade@users.noreply.github.com> Date: Thu, 25 May 2023 16:07:49 -0400 Subject: [PATCH 10/20] LG-9784 New Hybrid Handoff Controller fka Upload Step (#8407) * Initial commit with feature flag * Adding controller, view, and route * Casing redirect for upload step * Adding view locals to hybrid handoff template * Adding controller spec * Adding step indicator to show view * Adding initial feature spec Co-authored-by: Sonia Connolly Co-authored-by: Jonathan Hooper Co-authored-by: Amir Reavis-Bey changelog: Internal, FSM Removal, Add new controller for hybrid handoff (feature-flagged) --- .../idv/hybrid_handoff_controller.rb | 228 ++++++++++++++++ app/controllers/idv/link_sent_controller.rb | 2 + .../idv/actions/cancel_link_sent_action.rb | 6 +- .../actions/redo_document_capture_action.rb | 2 + app/services/idv/session.rb | 1 + app/services/idv/steps/agreement_step.rb | 3 + app/views/idv/hybrid_handoff/show.html.erb | 83 ++++++ config/application.yml.default | 1 + config/routes.rb | 2 + lib/identity_config.rb | 1 + .../idv/hybrid_handoff_controller_spec.rb | 92 +++++++ .../idv/doc_auth/hybrid_handoff_spec.rb | 251 ++++++++++++++++++ 12 files changed, 671 insertions(+), 1 deletion(-) create mode 100644 app/controllers/idv/hybrid_handoff_controller.rb create mode 100644 app/views/idv/hybrid_handoff/show.html.erb create mode 100644 spec/controllers/idv/hybrid_handoff_controller_spec.rb create mode 100644 spec/features/idv/doc_auth/hybrid_handoff_spec.rb diff --git a/app/controllers/idv/hybrid_handoff_controller.rb b/app/controllers/idv/hybrid_handoff_controller.rb new file mode 100644 index 00000000000..09459ebd55e --- /dev/null +++ b/app/controllers/idv/hybrid_handoff_controller.rb @@ -0,0 +1,228 @@ +module Idv + class HybridHandoffController < ApplicationController + include ActionView::Helpers::DateHelper + include IdvSession + include IdvStepConcern + include StepIndicatorConcern + include StepUtilitiesConcern + + before_action :confirm_two_factor_authenticated + before_action :render_404_if_hybrid_handoff_controller_disabled + before_action :confirm_agreement_step_complete + + def show + flow_session[:flow_path] = 'standard' + analytics.idv_doc_auth_upload_visited(**analytics_arguments) + + Funnel::DocAuth::RegisterStep.new(current_user.id, sp_session[:issuer]).call( + 'upload', :view, + true + ) + + render :show, locals: extra_view_variables + end + + def update + irs_attempts_api_tracker.idv_document_upload_method_selected( + upload_method: params[:type], + ) + + # See the simple_form_for in + # app/views/idv/doc_auth/upload.html.erb + if hybrid_flow_chosen? + handle_phone_submission + else + bypass_send_link_steps + end + end + + def hybrid_flow_chosen? + params[:type] != 'desktop' && !mobile_device? + end + + def handle_phone_submission + throttle.increment! + return throttled_failure if throttle.throttled? + idv_session.phone_for_mobile_flow = params[:doc_auth][:phone] + flow_session[:phone_for_mobile_flow] = idv_session.phone_for_mobile_flow + telephony_result = send_link + telephony_form_response = build_telephony_form_response(telephony_result) + + failure_reason = nil + if !telephony_result.success? + failure_reason = { telephony: [telephony_result.error.class.name.demodulize] } + failure(telephony_form_response.errors[:message]) + end + irs_attempts_api_tracker.idv_phone_upload_link_sent( + success: telephony_result.success?, + phone_number: formatted_destination_phone, + failure_reason: failure_reason, + ) + + if !failure_reason + flow_session[:flow_path] = 'hybrid' + redirect_to idv_link_sent_url + else + redirect_to idv_hybrid_handoff_url + end + + analytics.idv_doc_auth_upload_submitted( + **analytics_arguments.merge(form_response(destination: :link_sent).to_h), + ) + end + + def send_link + session_uuid = flow_session[:document_capture_session_uuid] + update_document_capture_session_requested_at(session_uuid) + Telephony.send_doc_auth_link( + to: formatted_destination_phone, + link: link_for_send_link(session_uuid), + country_code: Phonelib.parse(formatted_destination_phone).country, + sp_or_app_name: sp_or_app_name, + ) + end + + def link_for_send_link(session_uuid) + idv_hybrid_mobile_entry_url( + 'document-capture-session': session_uuid, + request_id: sp_session[:request_id], + ) + end + + def sp_or_app_name + current_sp&.friendly_name.presence || APP_NAME + end + + def build_telephony_form_response(telephony_result) + FormResponse.new( + success: telephony_result.success?, + errors: { message: telephony_result.error&.friendly_message }, + extra: { + telephony_response: telephony_result.to_h, + destination: :link_sent, + }, + ) + end + + def update_document_capture_session_requested_at(session_uuid) + document_capture_session = DocumentCaptureSession.find_by(uuid: session_uuid) + return unless document_capture_session + document_capture_session.update!( + requested_at: Time.zone.now, + cancelled_at: nil, + issuer: sp_session[:issuer], + ) + end + + def bypass_send_link_steps + flow_session[:flow_path] = 'standard' + redirect_to idv_document_capture_url + + response = form_response(destination: :document_capture) + analytics.idv_doc_auth_upload_submitted( + **analytics_arguments.merge(response.to_h), + ) + response + end + + def extra_view_variables + { + flow_session: flow_session, + idv_phone_form: build_form, + } + end + + def mobile_device? + # See app/javascript/packs/document-capture-welcome.js + # And app/services/idv/steps/agreement_step.rb + !!flow_session[:skip_upload_step] + end + + def build_form + Idv::PhoneForm.new( + previous_params: {}, + user: current_user, + delivery_methods: [:sms], + ) + end + + def throttle + @throttle ||= Throttle.new( + user: current_user, + throttle_type: :idv_send_link, + ) + end + + def analytics_arguments + { + step: 'upload', + analytics_id: 'Doc Auth', + irs_reproofing: irs_reproofing?, + flow_path: flow_session[:flow_path], + }.merge(**acuant_sdk_ab_test_analytics_args) + end + + def mark_link_sent_step_complete + flow_session['Idv::Steps::LinkSentStep'] = true + end + + def mark_upload_step_complete + flow_session['Idv::Steps::UploadStep'] = true + end + + def form_response(destination:) + FormResponse.new( + success: true, + errors: {}, + extra: { + destination: destination, + skip_upload_step: mobile_device?, + }, + ) + end + + def throttled_failure + analytics.throttler_rate_limit_triggered( + throttle_type: :idv_send_link, + ) + message = I18n.t( + 'errors.doc_auth.send_link_throttle', + timeout: distance_of_time_in_words( + Time.zone.now, + [throttle.expires_at, Time.zone.now].compact.max, + except: :seconds, + ), + ) + + irs_attempts_api_tracker.idv_phone_send_link_rate_limited( + phone_number: formatted_destination_phone, + ) + + failure(message) + redirect_to idv_hybrid_handoff_url + end + + # copied from Flow::Failure module + def failure(message, extra = nil) + flow_session[:error_message] = message + form_response_params = { success: false, errors: { message: message } } + form_response_params[:extra] = extra unless extra.nil? + FormResponse.new(**form_response_params) + end + + def render_404_if_hybrid_handoff_controller_disabled + render_not_found unless IdentityConfig.store.doc_auth_hybrid_handoff_controller_enabled + end + + def confirm_agreement_step_complete + return if flow_session['Idv::Steps::AgreementStep'] + + redirect_to idv_doc_auth_url + end + + def formatted_destination_phone + raw_phone = params.require(:doc_auth).permit(:phone) + PhoneFormatter.format(raw_phone, country_code: 'US') + end + end +end diff --git a/app/controllers/idv/link_sent_controller.rb b/app/controllers/idv/link_sent_controller.rb index 76fee27d11f..10d1027be78 100644 --- a/app/controllers/idv/link_sent_controller.rb +++ b/app/controllers/idv/link_sent_controller.rb @@ -45,6 +45,8 @@ def confirm_upload_step_complete if flow_session[:flow_path] == 'standard' redirect_to idv_document_capture_url + elsif IdentityConfig.store.doc_auth_hybrid_handoff_controller_enabled + redirect_to idv_hybrid_handoff_url else redirect_to idv_doc_auth_url end diff --git a/app/services/idv/actions/cancel_link_sent_action.rb b/app/services/idv/actions/cancel_link_sent_action.rb index b38e6c0b4ee..66dc4ba49ae 100644 --- a/app/services/idv/actions/cancel_link_sent_action.rb +++ b/app/services/idv/actions/cancel_link_sent_action.rb @@ -6,7 +6,11 @@ def self.analytics_submitted_event end def call - mark_step_incomplete(:upload) + if IdentityConfig.store.doc_auth_hybrid_handoff_controller_enabled + redirect_to idv_hybrid_handoff_url + else + mark_step_incomplete(:upload) + end end end end diff --git a/app/services/idv/actions/redo_document_capture_action.rb b/app/services/idv/actions/redo_document_capture_action.rb index 811642b321b..2054bc349ce 100644 --- a/app/services/idv/actions/redo_document_capture_action.rb +++ b/app/services/idv/actions/redo_document_capture_action.rb @@ -9,6 +9,8 @@ def call flow_session['redo_document_capture'] = true if flow_session[:skip_upload_step] redirect_to idv_document_capture_url + elsif IdentityConfig.store.doc_auth_hybrid_handoff_controller_enabled + redirect_to idv_hybrid_handoff_url else mark_step_incomplete(:upload) end diff --git a/app/services/idv/session.rb b/app/services/idv/session.rb index b03c647902a..4f1d086f877 100644 --- a/app/services/idv/session.rb +++ b/app/services/idv/session.rb @@ -9,6 +9,7 @@ class Session vendor_phone_confirmation user_phone_confirmation gpo_code_verified + phone_for_mobile_flow pii previous_phone_step_params profile_confirmation diff --git a/app/services/idv/steps/agreement_step.rb b/app/services/idv/steps/agreement_step.rb index 44171846b07..e7acce811e1 100644 --- a/app/services/idv/steps/agreement_step.rb +++ b/app/services/idv/steps/agreement_step.rb @@ -12,6 +12,9 @@ def self.analytics_submitted_event end def call + if IdentityConfig.store.doc_auth_hybrid_handoff_controller_enabled + redirect_to idv_hybrid_handoff_url + end end def form_submit diff --git a/app/views/idv/hybrid_handoff/show.html.erb b/app/views/idv/hybrid_handoff/show.html.erb new file mode 100644 index 00000000000..70b28b3ed42 --- /dev/null +++ b/app/views/idv/hybrid_handoff/show.html.erb @@ -0,0 +1,83 @@ +<% title t('titles.doc_auth.upload') %> + +<% content_for(:pre_flash_content) do %> + <%= render StepIndicatorComponent.new( + steps: Idv::Flows::DocAuthFlow::STEP_INDICATOR_STEPS, + current_step: :verify_id, + locale_scope: 'idv', + class: 'margin-x-neg-2 margin-top-neg-4 tablet:margin-x-neg-6 tablet:margin-top-neg-4', + ) %> +<% end %> + +<%= render 'idv/doc_auth/error_messages', flow_session: flow_session %> + +<%= render PageHeadingComponent.new do %> + <%= t('doc_auth.headings.upload') %> +<% end %> + +

+ <%= t('doc_auth.info.upload') %> +

+ +
+
+ <%= image_tag( + asset_url('idv/phone-icon.svg'), + alt: t('image_description.camera_mobile_phone'), + width: 88, + height: 88, + ) %> +
+
+
+ <%= t('doc_auth.info.tag') %> +
+

+ <%= t('doc_auth.headings.upload_from_phone') %> +

+ <%= t('doc_auth.info.upload_from_phone') %> + <%= simple_form_for( + idv_phone_form, + as: :doc_auth, + url: url_for(type: :mobile, combined: true), + method: 'PUT', + html: { autocomplete: 'off' }, + ) do |f| %> + <%= render PhoneInputComponent.new( + form: f, + required: true, + delivery_methods: [:sms], + class: 'margin-bottom-4', + ) %> + <%= f.submit t('forms.buttons.send_link') %> + <% end %> +
+
+ +
+
+
+ <%= image_tag( + asset_url('idv/laptop-icon.svg'), + alt: t('image_description.laptop'), + width: 88, + height: 88, + ) %> +
+
+

+ <%= t('doc_auth.headings.upload_from_computer') %> +

+ <%= t('doc_auth.info.upload_from_computer') %>  + <%= simple_form_for( + :doc_auth, + url: url_for(type: :desktop), + method: 'PUT', + class: 'margin-bottom-4', + ) do |f| %> + <%= f.submit t('forms.buttons.upload_photos'), outline: true %> + <% end %> +
+
+ +<%= render 'idv/doc_auth/cancel', step: 'upload' %> diff --git a/config/application.yml.default b/config/application.yml.default index baf209db88f..a63c02d1c93 100644 --- a/config/application.yml.default +++ b/config/application.yml.default @@ -90,6 +90,7 @@ doc_auth_s3_request_timeout: 5 doc_auth_error_dpi_threshold: 290 doc_auth_error_glare_threshold: 40 doc_auth_error_sharpness_threshold: 40 +doc_auth_hybrid_handoff_controller_enabled: false doc_auth_max_attempts: 5 doc_auth_max_capture_attempts_before_tips: 3 doc_auth_max_capture_attempts_before_native_camera: 2 diff --git a/config/routes.rb b/config/routes.rb index 0841dbccf16..24e421d3a9c 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -333,6 +333,8 @@ get '/hybrid_mobile/document_capture' => 'hybrid_mobile/document_capture#show' put '/hybrid_mobile/document_capture' => 'hybrid_mobile/document_capture#update' get '/hybrid_mobile/capture_complete' => 'hybrid_mobile/capture_complete#show' + get '/hybrid_handoff' => 'hybrid_handoff#show' + put '/hybrid_handoff' => 'hybrid_handoff#update' get '/link_sent' => 'link_sent#show' put '/link_sent' => 'link_sent#update' get '/ssn' => 'ssn#show' diff --git a/lib/identity_config.rb b/lib/identity_config.rb index e832cecdbf7..583f5888d03 100644 --- a/lib/identity_config.rb +++ b/lib/identity_config.rb @@ -160,6 +160,7 @@ def self.build_store(config_map) config.add(:doc_auth_error_glare_threshold, type: :integer) config.add(:doc_auth_error_sharpness_threshold, type: :integer) config.add(:doc_auth_extend_timeout_by_minutes, type: :integer) + config.add(:doc_auth_hybrid_handoff_controller_enabled, type: :boolean) config.add(:doc_auth_max_attempts, type: :integer) config.add(:doc_auth_max_capture_attempts_before_native_camera, type: :integer) config.add(:doc_auth_max_submission_attempts_before_native_camera, type: :integer) diff --git a/spec/controllers/idv/hybrid_handoff_controller_spec.rb b/spec/controllers/idv/hybrid_handoff_controller_spec.rb new file mode 100644 index 00000000000..62fa39f37d2 --- /dev/null +++ b/spec/controllers/idv/hybrid_handoff_controller_spec.rb @@ -0,0 +1,92 @@ +require 'rails_helper' + +describe Idv::HybridHandoffController do + include IdvHelper + + let(:user) { create(:user) } + + before do + allow(IdentityConfig.store).to receive(:doc_auth_hybrid_handoff_controller_enabled). + and_return(true) + stub_sign_in(user) + stub_analytics + stub_attempts_tracker + subject.user_session['idv/doc_auth'] = { 'Idv::Steps::AgreementStep' => true } + end + + describe 'before_actions' do + it 'includes authentication before_action' do + expect(subject).to have_actions( + :before, + :confirm_two_factor_authenticated, + ) + end + + it 'checks that agreement step is complete' do + expect(subject).to have_actions( + :before, + :confirm_agreement_step_complete, + ) + end + end + + describe '#show' do + let(:analytics_name) { 'IdV: doc auth upload visited' } + let(:analytics_args) do + { flow_path: 'standard', + step: 'upload', + analytics_id: 'Doc Auth', + irs_reproofing: false } + end + + it 'renders the show template' do + get :show + + expect(response).to render_template :show + end + + it 'sends analytics_visited event' do + get :show + + expect(@analytics).to have_logged_event(analytics_name, analytics_args) + end + + it 'updates DocAuthLog document_capture_view_count' do + doc_auth_log = DocAuthLog.create(user_id: user.id) + + expect { get :show }.to( + change { doc_auth_log.reload.upload_view_count }.from(0).to(1), + ) + end + + context 'agreement step is not complete' do + it 'redirects to idv_doc_auth_url' do + subject.user_session['idv/doc_auth']['Idv::Steps::AgreementStep'] = nil + + get :show + + expect(response).to redirect_to(idv_doc_auth_url) + end + end + end + + describe '#update' do + let(:analytics_name) { 'IdV: doc auth upload submitted' } + let(:analytics_args) do + { success: true, + errors: {}, + destination: :link_sent, + flow_path: 'hybrid', + step: 'upload', + analytics_id: 'Doc Auth', + irs_reproofing: false, + skip_upload_step: false } + end + + it 'sends analytics_submitted event' do + put :update, params: { doc_auth: { phone: '202-555-5555' } } + + expect(@analytics).to have_logged_event(analytics_name, analytics_args) + end + end +end diff --git a/spec/features/idv/doc_auth/hybrid_handoff_spec.rb b/spec/features/idv/doc_auth/hybrid_handoff_spec.rb new file mode 100644 index 00000000000..a15d437ba52 --- /dev/null +++ b/spec/features/idv/doc_auth/hybrid_handoff_spec.rb @@ -0,0 +1,251 @@ +require 'rails_helper' + +feature 'doc auth upload step' do + include IdvStepHelper + include DocAuthHelper + include ActionView::Helpers::DateHelper + + let(:fake_analytics) { FakeAnalytics.new } + let(:fake_attempts_tracker) { IrsAttemptsApiTrackingHelper::FakeAttemptsTracker.new } + let(:new_controller_enabled) { true } + let(:document_capture_session) { DocumentCaptureSession.create! } + let(:idv_send_link_max_attempts) { 3 } + let(:idv_send_link_attempt_window_in_minutes) do + IdentityConfig.store.idv_send_link_attempt_window_in_minutes + end + + before do + sign_in_and_2fa_user + allow(IdentityConfig.store).to receive(:doc_auth_hybrid_handoff_controller_enabled). + and_return(new_controller_enabled) + allow_any_instance_of(Idv::HybridHandoffController).to receive(:mobile_device?).and_return(true) + complete_doc_auth_steps_before_upload_step + allow_any_instance_of(ApplicationController).to receive(:analytics).and_return(fake_analytics) + allow_any_instance_of(ApplicationController).to receive(:irs_attempts_api_tracker). + and_return(fake_attempts_tracker) + end + + context 'on a desktop device', js: true do + before do + allow_any_instance_of( + Idv::HybridHandoffController, + ).to receive( + :mobile_device?, + ).and_return(false) + end + + it 'displays with the expected content' do + expect(page).to have_content(t('doc_auth.headings.upload_from_computer')) + expect(page).to have_content(t('doc_auth.info.upload_from_computer')) + expect(page).to have_content(t('doc_auth.headings.upload_from_phone')) + end + + it 'proceeds to document capture when user chooses to upload from computer' do + expect(fake_attempts_tracker).to receive( + :idv_document_upload_method_selected, + ).with({ upload_method: 'desktop' }) + + expect_step_indicator_current_step(t('step_indicator.flows.idv.verify_id')) + + click_upload_from_computer + + expect(page).to have_current_path(idv_document_capture_url) + expect(fake_analytics).to have_logged_event( + 'IdV: doc auth upload submitted', + hash_including(step: 'upload', destination: :document_capture), + ) + end + + it "defaults phone to user's 2fa phone number" do + field = page.find_field(t('two_factor_authentication.phone_label')) + expect(field.value).to eq('(202) 555-1212') + end + + it 'proceeds to link sent page when user chooses to use phone' do + expect(fake_attempts_tracker).to receive( + :idv_document_upload_method_selected, + ).with({ upload_method: 'mobile' }) + + click_send_link + + expect(page).to have_current_path(idv_link_sent_path) + expect(fake_analytics).to have_logged_event( + 'IdV: doc auth upload submitted', + hash_including(step: 'upload', destination: :link_sent), + ) + end + + it 'proceeds to the next page with valid info' do + expect(fake_attempts_tracker).to receive( + :idv_phone_upload_link_sent, + ).with( + success: true, + phone_number: '+1 415-555-0199', + failure_reason: nil, + ) + + expect(Telephony).to receive(:send_doc_auth_link). + with(hash_including(to: '+1 415-555-0199')). + and_call_original + + expect_step_indicator_current_step(t('step_indicator.flows.idv.verify_id')) + + fill_in :doc_auth_phone, with: '415-555-0199' + click_send_link + + expect(page).to have_current_path(idv_link_sent_path) + end + + it 'does not proceed to the next page with invalid info' do + fill_in :doc_auth_phone, with: '' + click_send_link + + expect(page).to have_current_path(idv_hybrid_handoff_path, ignore_query: true) + end + + it 'sends a link that does not contain any underscores' do + # because URLs with underscores sometimes get messed up by carriers + expect(Telephony).to receive(:send_doc_auth_link).and_wrap_original do |impl, config| + expect(config[:link]).to_not include('_') + + impl.call(**config) + end + + fill_in :doc_auth_phone, with: '415-555-0199' + click_send_link + + expect(page).to have_current_path(idv_link_sent_path) + end + + it 'does not proceed if Telephony raises an error' do + expect(fake_attempts_tracker).to receive(:idv_phone_upload_link_sent).with( + success: false, + phone_number: '+1 225-555-1000', + failure_reason: { telephony: ['TelephonyError'] }, + ) + fill_in :doc_auth_phone, with: '225-555-1000' + + click_send_link + + expect(page).to have_current_path(idv_hybrid_handoff_path, ignore_query: true) + expect(page).to have_content I18n.t('telephony.error.friendly_message.generic') + end + + it 'displays error if user selects a country to which we cannot send SMS', js: true do + page.find('div[aria-label="Country code"]').click + within(page.find('.iti__flag-container', visible: :all)) do + find('span', text: 'Sri Lanka').click + end + focused_input = page.find('.phone-input__number:focus') + + error_message_id = focused_input[:'aria-describedby']&.split(' ')&.find do |id| + page.has_css?(".usa-error-message##{id}") + end + expect(error_message_id).to_not be_empty + + error_message = page.find_by_id(error_message_id) + expect(error_message).to have_content( + t( + 'two_factor_authentication.otp_delivery_preference.sms_unsupported', + location: 'Sri Lanka', + ), + ) + click_send_link + expect(page.find(':focus')).to match_css('.phone-input__number') + end + + it 'throttles sending the link' do + user = user_with_2fa + sign_in_and_2fa_user(user) + complete_doc_auth_steps_before_upload_step + timeout = distance_of_time_in_words( + Throttle.attempt_window_in_minutes(:idv_send_link).minutes, + ) + allow(IdentityConfig.store).to receive(:idv_send_link_max_attempts). + and_return(idv_send_link_max_attempts) + + expect(fake_attempts_tracker).to receive( + :idv_phone_send_link_rate_limited, + ).with({ phone_number: '+1 415-555-0199' }) + + freeze_time do + (idv_send_link_max_attempts - 1).times do + expect(page).to_not have_content( + I18n.t('errors.doc_auth.send_link_throttle', timeout: timeout), + ) + + fill_in :doc_auth_phone, with: '415-555-0199' + click_send_link + + expect(page).to have_current_path(idv_link_sent_path) + + click_doc_auth_back_link + end + + fill_in :doc_auth_phone, with: '415-555-0199' + + click_send_link + expect(page).to have_current_path(idv_hybrid_handoff_path, ignore_query: true) + expect(page).to have_content( + I18n.t( + 'errors.doc_auth.send_link_throttle', + timeout: timeout, + ), + ) + end + expect(fake_analytics).to have_logged_event( + 'Throttler Rate Limit Triggered', + throttle_type: :idv_send_link, + ) + + # Manual expiration is needed for now since the Throttle uses + # Redis ttl instead of expiretime + Throttle.new(throttle_type: :idv_send_link, user: user).reset! + travel_to(Time.zone.now + idv_send_link_attempt_window_in_minutes.minutes) do + fill_in :doc_auth_phone, with: '415-555-0199' + click_send_link + expect(page).to have_current_path(idv_link_sent_path) + end + end + + it 'includes expected URL parameters' do + allow_any_instance_of( + Idv::HybridHandoffController, + ).to receive( + :flow_session, + ).and_wrap_original do |method, args| + session = method.call(*args) + session['document_capture_session_uuid'] = document_capture_session.uuid + session + end + + expect(Telephony).to receive(:send_doc_auth_link).and_wrap_original do |impl, config| + params = Rack::Utils.parse_nested_query URI(config[:link]).query + expect(params).to eq('document-capture-session' => document_capture_session.uuid) + + impl.call(**config) + end + + fill_in :doc_auth_phone, with: '415-555-0199' + click_send_link + end + + it 'sets requested_at on the capture session' do + allow_any_instance_of( + Idv::HybridHandoffController, + ).to receive( + :flow_session, + ).and_wrap_original do |method, args| + session = method.call(*args) + session['document_capture_session_uuid'] = document_capture_session.uuid + session + end + + fill_in :doc_auth_phone, with: '415-555-0199' + click_send_link + + document_capture_session.reload + expect(document_capture_session).to have_attributes(requested_at: a_kind_of(Time)) + end + end +end From 34e22d29b448191d79ea352da8e582e0b9a95ad5 Mon Sep 17 00:00:00 2001 From: Mitchell Henke Date: Thu, 25 May 2023 16:43:50 -0500 Subject: [PATCH 11/20] Allow configuring shortcodes for international country codes (LG-9839) (#8486) * Allow configuring shortcodes for international country codes changelog: User-Facing Improvements, Telephony, Allow configuring shortcodes for international country codes * Update lib/telephony/configuration.rb Co-authored-by: Andrew Duthie --------- Co-authored-by: Andrew Duthie --- config/initializers/telephony.rb | 1 + lib/telephony/configuration.rb | 8 +++++ lib/telephony/pinpoint/sms_sender.rb | 2 ++ .../lib/telephony/pinpoint/sms_sender_spec.rb | 34 +++++++++++++++++++ .../telephony/pinpoint_configuration_spec.rb | 17 ++++++++++ spec/support/shared_contexts/telephony.rb | 1 + 6 files changed, 63 insertions(+) create mode 100644 spec/lib/telephony/pinpoint_configuration_spec.rb diff --git a/config/initializers/telephony.rb b/config/initializers/telephony.rb index eb47eb66779..a6a81259c06 100644 --- a/config/initializers/telephony.rb +++ b/config/initializers/telephony.rb @@ -22,6 +22,7 @@ c.pinpoint.add_sms_config do |sms| sms.application_id = sms_json_config['application_id'] sms.region = sms_json_config['region'] + sms.country_code_shortcodes = sms_json_config['country_code_shortcodes'] || {} sms.shortcode = sms_json_config['shortcode'] sms.country_code_longcode_pool = sms_json_config['country_code_longcode_pool'] || {} sms.credential_role_arn = sms_json_config['credential_role_arn'] diff --git a/lib/telephony/configuration.rb b/lib/telephony/configuration.rb index 020b38f7654..56feec80013 100644 --- a/lib/telephony/configuration.rb +++ b/lib/telephony/configuration.rb @@ -15,6 +15,13 @@ def add_sms_config raise 'missing sms configuration block' unless block_given? sms = PinpointSmsConfiguration.new(region: 'us-west-2') yield sms + + shortcode_country_codes = sms.country_code_shortcodes&.keys || [] + longcode_country_codes = sms.country_code_longcode_pool&.keys || [] + if shortcode_country_codes.intersect?(longcode_country_codes) + raise 'cannot configure a country code for both longcodes and a shortcode' + end + sms_configs << sms sms end @@ -42,6 +49,7 @@ def add_voice_config PinpointSmsConfiguration = Struct.new( :application_id, :shortcode, + :country_code_shortcodes, :country_code_longcode_pool, *PINPOINT_CONFIGURATION_NAMES, keyword_init: true, diff --git a/lib/telephony/pinpoint/sms_sender.rb b/lib/telephony/pinpoint/sms_sender.rb index ce63b395533..85a3b3420fa 100644 --- a/lib/telephony/pinpoint/sms_sender.rb +++ b/lib/telephony/pinpoint/sms_sender.rb @@ -139,6 +139,8 @@ def phone_info(phone_number) def origination_number(country_code, sms_config) if sms_config.country_code_longcode_pool&.dig(country_code).present? sms_config.country_code_longcode_pool[country_code].sample + elsif sms_config.country_code_shortcodes&.dig(country_code).present? + sms_config.country_code_shortcodes[country_code] else sms_config.shortcode end diff --git a/spec/lib/telephony/pinpoint/sms_sender_spec.rb b/spec/lib/telephony/pinpoint/sms_sender_spec.rb index 75966a5f728..b55f7c23820 100644 --- a/spec/lib/telephony/pinpoint/sms_sender_spec.rb +++ b/spec/lib/telephony/pinpoint/sms_sender_spec.rb @@ -285,6 +285,40 @@ def ==(other) end end + context 'in a non-sender_id country that has a configured shortcode' do + let(:country_code) { 'MX' } + + it 'sends a message with a longcode and no sender_id' do + mock_build_client + response = subject.send( + message: 'This is a test!', + to: '+525555555555', + country_code: country_code, + ) + + expected_result = { + application_id: Telephony.config.pinpoint.sms_configs.first.application_id, + message_request: { + addresses: { + '+525555555555' => { channel_type: 'SMS' }, + }, + message_configuration: { + sms_message: { + body: 'This is a test!', + message_type: 'TRANSACTIONAL', + origination_number: '987654', + }, + }, + }, + } + + expect(Pinpoint::MockClient.last_request).to eq(expected_result) + expect(response.success?).to eq(true) + expect(response.error).to eq(nil) + expect(response.extra[:request_id]).to eq('fake-message-request-id') + end + end + context 'with multiple sms configs' do before do Telephony.config.pinpoint.add_sms_config do |sms| diff --git a/spec/lib/telephony/pinpoint_configuration_spec.rb b/spec/lib/telephony/pinpoint_configuration_spec.rb new file mode 100644 index 00000000000..58e9b221009 --- /dev/null +++ b/spec/lib/telephony/pinpoint_configuration_spec.rb @@ -0,0 +1,17 @@ +require 'rails_helper' + +RSpec.describe Telephony::PinpointConfiguration do + context '#add_sms_config' do + it 'raises if the same country code is used in both longcode and shortcode configuration' do + config = Telephony::PinpointConfiguration.new + expect do + config.add_sms_config do |sms| + sms.country_code_shortcodes = { 'PR' => '123456' } + sms.country_code_longcode_pool = { 'PR' => ['+1 (939) 456-7890'] } + end.to raise_error( + 'cannot configure a country code for both longcodes and a shortcode', + ) + end + end + end +end diff --git a/spec/support/shared_contexts/telephony.rb b/spec/support/shared_contexts/telephony.rb index a5bb5afe751..67d04a26755 100644 --- a/spec/support/shared_contexts/telephony.rb +++ b/spec/support/shared_contexts/telephony.rb @@ -15,6 +15,7 @@ def telephony_use_default_config! sms.application_id = 'fake-pinpoint-application-id-sms' sms.shortcode = '123456' sms.country_code_longcode_pool = { 'PR' => ['+19393334444'] } + sms.country_code_shortcodes = { 'MX' => '987654' } end c.pinpoint.add_voice_config do |voice| From e370759c044c019037e7c35a7e269a96e8310ea6 Mon Sep 17 00:00:00 2001 From: Mitchell Henke Date: Thu, 25 May 2023 16:50:43 -0500 Subject: [PATCH 12/20] Remove handle_valid_otp method (#8392) * Consolidate two factor verification and confirmation behavior changelog: Internal, Two-Factor Authentication, Consolidate two factor verification and confirmation behavior --- .../concerns/remember_device_concern.rb | 1 - .../two_factor_authenticatable_methods.rb | 21 ------------------- .../backup_code_verification_controller.rb | 1 - .../otp_verification_controller.rb | 21 +++++++++++++++++-- .../personal_key_verification_controller.rb | 1 - .../piv_cac_verification_controller.rb | 1 - .../totp_verification_controller.rb | 6 +++++- .../webauthn_verification_controller.rb | 15 +++++++++---- .../users/piv_cac_login_controller.rb | 12 ++--------- ...ackup_code_verification_controller_spec.rb | 7 +++++++ .../otp_verification_controller_spec.rb | 11 +++++++++- ...rsonal_key_verification_controller_spec.rb | 8 +++++++ .../piv_cac_verification_controller_spec.rb | 9 ++++++++ .../totp_verification_controller_spec.rb | 9 +++++++- .../webauthn_verification_controller_spec.rb | 11 ++++++++++ .../users/piv_cac_login_controller_spec.rb | 8 ------- spec/support/controller_helper.rb | 4 ++-- 17 files changed, 92 insertions(+), 54 deletions(-) diff --git a/app/controllers/concerns/remember_device_concern.rb b/app/controllers/concerns/remember_device_concern.rb index 86cfaf999e2..81adc2aff16 100644 --- a/app/controllers/concerns/remember_device_concern.rb +++ b/app/controllers/concerns/remember_device_concern.rb @@ -74,7 +74,6 @@ def handle_valid_remember_device_cookie(remember_device_cookie:) mark_user_session_authenticated(:device_remembered) handle_valid_remember_device_analytics(cookie_created_at: remember_device_cookie.created_at) redirect_to after_otp_verification_confirmation_url unless reauthn? - reset_otp_session_data end def handle_valid_remember_device_analytics(cookie_created_at:) diff --git a/app/controllers/concerns/two_factor_authenticatable_methods.rb b/app/controllers/concerns/two_factor_authenticatable_methods.rb index d5316e965ac..59869d4e4d8 100644 --- a/app/controllers/concerns/two_factor_authenticatable_methods.rb +++ b/app/controllers/concerns/two_factor_authenticatable_methods.rb @@ -93,27 +93,11 @@ def reset_attempt_count_if_user_no_longer_locked_out ).call end - def handle_valid_otp(next_url:, auth_method: nil) - handle_valid_otp_for_context(auth_method: auth_method) - handle_remember_device - next_url ||= after_otp_verification_confirmation_url - reset_otp_session_data - redirect_to next_url - end - def handle_remember_device save_user_opted_remember_device_pref save_remember_device_preference end - def handle_valid_otp_for_context(auth_method:) - if UserSessionContext.authentication_or_reauthentication_context?(context) - handle_valid_verification_for_authentication_context(auth_method: auth_method) - elsif UserSessionContext.confirmation_context?(context) - handle_valid_verification_for_confirmation_context(auth_method: auth_method) - end - end - # Method will be renamed in the next refactor. # You can pass in any "type" with a corresponding I18n key in # two_factor_authentication.invalid_#{type} @@ -186,11 +170,6 @@ def reset_second_factor_attempts_count UpdateUser.new(user: current_user, attributes: { second_factor_attempts_count: 0 }).call end - def reset_otp_session_data - user_session.delete(:unconfirmed_phone) - user_session[:context] = 'authentication' - end - def after_otp_verification_confirmation_url return @next_mfa_setup_path if @next_mfa_setup_path return account_url if @updating_existing_number diff --git a/app/controllers/two_factor_authentication/backup_code_verification_controller.rb b/app/controllers/two_factor_authentication/backup_code_verification_controller.rb index 61cc1bf3147..66e0a01bebf 100644 --- a/app/controllers/two_factor_authentication/backup_code_verification_controller.rb +++ b/app/controllers/two_factor_authentication/backup_code_verification_controller.rb @@ -77,7 +77,6 @@ def backup_code_params def handle_valid_backup_code redirect_to after_otp_verification_confirmation_url - reset_otp_session_data end def check_sp_required_mfa diff --git a/app/controllers/two_factor_authentication/otp_verification_controller.rb b/app/controllers/two_factor_authentication/otp_verification_controller.rb index 2c19adb8f19..362f4ed4272 100644 --- a/app/controllers/two_factor_authentication/otp_verification_controller.rb +++ b/app/controllers/two_factor_authentication/otp_verification_controller.rb @@ -21,8 +21,17 @@ def create result = otp_verification_form.submit post_analytics(result) if result.success? - handle_valid_confirmation_otp if UserSessionContext.confirmation_context?(context) - handle_valid_otp(next_url: nil, auth_method: params[:otp_delivery_preference]) + if UserSessionContext.confirmation_context?(context) + handle_valid_confirmation_otp + else + handle_valid_verification_for_authentication_context( + auth_method: params[:otp_delivery_preference], + ) + end + + handle_remember_device + reset_otp_session_data + redirect_to after_otp_verification_confirmation_url else handle_invalid_otp(context: context, type: 'otp') end @@ -34,6 +43,9 @@ def handle_valid_confirmation_otp assign_phone track_mfa_added @next_mfa_setup_path = next_setup_path + handle_valid_verification_for_confirmation_context( + auth_method: params[:otp_delivery_preference], + ) flash[:success] = t('notices.phone_confirmed') end @@ -247,5 +259,10 @@ def selected_otp_make_default_number def direct_otp_code current_user.direct_otp if FeatureManagement.prefill_otp_codes? end + + def reset_otp_session_data + user_session.delete(:unconfirmed_phone) + user_session[:context] = 'authentication' + end end end diff --git a/app/controllers/two_factor_authentication/personal_key_verification_controller.rb b/app/controllers/two_factor_authentication/personal_key_verification_controller.rb index 98655c5a383..125effcc62a 100644 --- a/app/controllers/two_factor_authentication/personal_key_verification_controller.rb +++ b/app/controllers/two_factor_authentication/personal_key_verification_controller.rb @@ -99,7 +99,6 @@ def handle_valid_otp else redirect_to authentication_methods_setup_url end - reset_otp_session_data end end end diff --git a/app/controllers/two_factor_authentication/piv_cac_verification_controller.rb b/app/controllers/two_factor_authentication/piv_cac_verification_controller.rb index 0f33a572a04..84c22eed08f 100644 --- a/app/controllers/two_factor_authentication/piv_cac_verification_controller.rb +++ b/app/controllers/two_factor_authentication/piv_cac_verification_controller.rb @@ -54,7 +54,6 @@ def handle_valid_piv_cac auth_method: TwoFactorAuthenticatable::AuthMethod::PIV_CAC, ) redirect_to after_otp_verification_confirmation_url - reset_otp_session_data end def handle_invalid_piv_cac diff --git a/app/controllers/two_factor_authentication/totp_verification_controller.rb b/app/controllers/two_factor_authentication/totp_verification_controller.rb index 033ea9cfd48..ffff839eca9 100644 --- a/app/controllers/two_factor_authentication/totp_verification_controller.rb +++ b/app/controllers/two_factor_authentication/totp_verification_controller.rb @@ -23,7 +23,11 @@ def create irs_attempts_api_tracker.mfa_login_totp(success: result.success?) if result.success? - handle_valid_otp(next_url: nil, auth_method: 'authenticator') + handle_valid_verification_for_authentication_context( + auth_method: TwoFactorAuthenticatable::AuthMethod::TOTP, + ) + handle_remember_device + redirect_to after_otp_verification_confirmation_url else handle_invalid_otp(context: context, type: 'totp') end diff --git a/app/controllers/two_factor_authentication/webauthn_verification_controller.rb b/app/controllers/two_factor_authentication/webauthn_verification_controller.rb index fe8cff30809..5db5cb2730f 100644 --- a/app/controllers/two_factor_authentication/webauthn_verification_controller.rb +++ b/app/controllers/two_factor_authentication/webauthn_verification_controller.rb @@ -40,10 +40,17 @@ def handle_webauthn_result(result) end def handle_valid_webauthn - handle_valid_verification_for_authentication_context(auth_method: 'webauthn') + if form.webauthn_configuration.platform_authenticator + handle_valid_verification_for_authentication_context( + auth_method: TwoFactorAuthenticatable::AuthMethod::WEBAUTHN_PLATFORM, + ) + else + handle_valid_verification_for_authentication_context( + auth_method: TwoFactorAuthenticatable::AuthMethod::WEBAUTHN, + ) + end handle_remember_device redirect_to after_otp_verification_confirmation_url - reset_otp_session_data end def handle_invalid_webauthn @@ -96,9 +103,9 @@ def credential_ids def analytics_properties auth_method = if form&.webauthn_configuration&.platform_authenticator || params[:platform].to_s == 'true' - 'webauthn_platform' + TwoFactorAuthenticatable::AuthMethod::WEBAUTHN_PLATFORM else - 'webauthn' + TwoFactorAuthenticatable::AuthMethod::WEBAUTHN end { context: context, diff --git a/app/controllers/users/piv_cac_login_controller.rb b/app/controllers/users/piv_cac_login_controller.rb index 80cce49ca71..5092c755ac5 100644 --- a/app/controllers/users/piv_cac_login_controller.rb +++ b/app/controllers/users/piv_cac_login_controller.rb @@ -74,10 +74,10 @@ def process_valid_submission presented: true, ) - handle_valid_otp( - next_url: next_step, + handle_valid_verification_for_authentication_context( auth_method: TwoFactorAuthenticatable::AuthMethod::PIV_CAC, ) + redirect_to next_step end def next_step @@ -105,13 +105,5 @@ def process_invalid_submission def process_token_with_error redirect_to login_piv_cac_error_url(error: piv_cac_login_form.error_type) end - - def mark_user_session_authenticated(authentication_type) - user_session[TwoFactorAuthenticatable::NEED_AUTHENTICATION] = false - user_session[:authn_at] = Time.zone.now - analytics.user_marked_authed( - authentication_type: authentication_type, - ) - end end end diff --git a/spec/controllers/two_factor_authentication/backup_code_verification_controller_spec.rb b/spec/controllers/two_factor_authentication/backup_code_verification_controller_spec.rb index 34923ef1c08..64a3f6f597b 100644 --- a/spec/controllers/two_factor_authentication/backup_code_verification_controller_spec.rb +++ b/spec/controllers/two_factor_authentication/backup_code_verification_controller_spec.rb @@ -41,6 +41,11 @@ with(:mfa_login_backup_code, success: true) post :create, params: payload + + expect(subject.user_session[:auth_method]).to eq( + TwoFactorAuthenticatable::AuthMethod::BACKUP_CODE, + ) + expect(subject.user_session[TwoFactorAuthenticatable::NEED_AUTHENTICATION]).to eq false end it 'tracks the valid authentication event when there are exisitng codes' do @@ -120,6 +125,8 @@ expect(response).to render_template(:show) expect(flash[:error]).to eq t('two_factor_authentication.invalid_backup_code') + expect(subject.user_session[:auth_method]).to eq nil + expect(subject.user_session[TwoFactorAuthenticatable::NEED_AUTHENTICATION]).to eq true end it 'tracks the max attempts event' do diff --git a/spec/controllers/two_factor_authentication/otp_verification_controller_spec.rb b/spec/controllers/two_factor_authentication/otp_verification_controller_spec.rb index ff23ba93060..cc03c67b446 100644 --- a/spec/controllers/two_factor_authentication/otp_verification_controller_spec.rb +++ b/spec/controllers/two_factor_authentication/otp_verification_controller_spec.rb @@ -161,6 +161,11 @@ it 'displays flash error message' do expect(flash[:error]).to eq t('two_factor_authentication.invalid_otp') end + + it 'does not set auth_method and requires 2FA' do + expect(subject.user_session[:auth_method]).to eq nil + expect(subject.user_session[TwoFactorAuthenticatable::NEED_AUTHENTICATION]).to eq true + end end context 'when the user enters an invalid OTP during reauthentication context' do @@ -278,6 +283,9 @@ code: subject.current_user.reload.direct_otp, otp_delivery_preference: 'sms', } + + expect(subject.user_session[:auth_method]).to eq TwoFactorAuthenticatable::AuthMethod::SMS + expect(subject.user_session[TwoFactorAuthenticatable::NEED_AUTHENTICATION]).to eq false end it 'tracks the attempt event with reauthentication parameter true' do @@ -393,6 +401,7 @@ let(:user) { create(:user, :fully_registered) } before do sign_in_as_user(user) + subject.user_session[TwoFactorAuthenticatable::NEED_AUTHENTICATION] = false subject.user_session[:unconfirmed_phone] = '+1 (703) 555-5555' subject.user_session[:context] = 'confirmation' @@ -417,7 +426,7 @@ @previous_phone = MfaContext.new(subject.current_user).phone_configurations.first&.phone end - context 'user has an existing phone number' do + context 'user is fully authenticated and has an existing phone number' do context 'user enters a valid code' do before do subject.user_session[:mfa_selections] = ['sms'] diff --git a/spec/controllers/two_factor_authentication/personal_key_verification_controller_spec.rb b/spec/controllers/two_factor_authentication/personal_key_verification_controller_spec.rb index d96fa740c17..a3ec63a5570 100644 --- a/spec/controllers/two_factor_authentication/personal_key_verification_controller_spec.rb +++ b/spec/controllers/two_factor_authentication/personal_key_verification_controller_spec.rb @@ -67,6 +67,11 @@ with('User marked authenticated', authentication_type: :valid_2fa) post :create, params: payload + + expect(subject.user_session[:auth_method]).to eq( + TwoFactorAuthenticatable::AuthMethod::PERSONAL_KEY, + ) + expect(subject.user_session[TwoFactorAuthenticatable::NEED_AUTHENTICATION]).to eq false end end @@ -136,6 +141,9 @@ expect(subject).to receive(:handle_invalid_otp).and_call_original post :create, params: payload + + expect(subject.user_session[:auth_method]).to eq nil + expect(subject.user_session[TwoFactorAuthenticatable::NEED_AUTHENTICATION]).to eq true end it 're-renders the personal key entry screen' do diff --git a/spec/controllers/two_factor_authentication/piv_cac_verification_controller_spec.rb b/spec/controllers/two_factor_authentication/piv_cac_verification_controller_spec.rb index ed2fd99941e..c15a8f957fa 100644 --- a/spec/controllers/two_factor_authentication/piv_cac_verification_controller_spec.rb +++ b/spec/controllers/two_factor_authentication/piv_cac_verification_controller_spec.rb @@ -68,6 +68,10 @@ get :show, params: { token: 'good-token' } + expect(subject.user_session[:auth_method]).to eq( + TwoFactorAuthenticatable::AuthMethod::PIV_CAC, + ) + expect(subject.user_session[TwoFactorAuthenticatable::NEED_AUTHENTICATION]).to eq false expect(response).to redirect_to account_path expect(subject.user_session[:decrypted_x509]).to eq( { @@ -147,6 +151,11 @@ it 'resets the piv/cac session information' do expect(subject.user_session[:decrypted_x509]).to be_nil end + + it 'does not set auth_method and requires 2FA' do + expect(subject.user_session[:auth_method]).to eq nil + expect(subject.user_session[TwoFactorAuthenticatable::NEED_AUTHENTICATION]).to eq true + end end context 'when the user presents a different piv/cac' do diff --git a/spec/controllers/two_factor_authentication/totp_verification_controller_spec.rb b/spec/controllers/two_factor_authentication/totp_verification_controller_spec.rb index 9bb8a7dd935..3008b84a875 100644 --- a/spec/controllers/two_factor_authentication/totp_verification_controller_spec.rb +++ b/spec/controllers/two_factor_authentication/totp_verification_controller_spec.rb @@ -15,7 +15,7 @@ Db::AuthAppConfiguration.create(user, @secret, nil, 'foo') end - it 'redirects to the profile' do + it 'redirects to the profile and sets auth_method' do cfg = subject.current_user.auth_app_configurations.first expect(Db::AuthAppConfiguration).to receive(:authenticate).and_return(cfg) expect(subject.current_user.reload.second_factor_attempts_count).to eq 0 @@ -23,6 +23,8 @@ post :create, params: { code: generate_totp_code(@secret) } expect(response).to redirect_to account_path + expect(subject.user_session[:auth_method]).to eq TwoFactorAuthenticatable::AuthMethod::TOTP + expect(subject.user_session[TwoFactorAuthenticatable::NEED_AUTHENTICATION]).to eq false end it 'resets the second_factor_attempts_count' do @@ -74,6 +76,11 @@ it 'displays flash error message' do expect(flash[:error]).to eq t('two_factor_authentication.invalid_otp') end + + it 'does not set auth_method and still requires 2FA' do + expect(subject.user_session[:auth_method]).to eq nil + expect(subject.user_session[TwoFactorAuthenticatable::NEED_AUTHENTICATION]).to eq true + end end context 'when the user has reached the max number of TOTP attempts' do diff --git a/spec/controllers/two_factor_authentication/webauthn_verification_controller_spec.rb b/spec/controllers/two_factor_authentication/webauthn_verification_controller_spec.rb index 67eedb26e5d..edc18df1258 100644 --- a/spec/controllers/two_factor_authentication/webauthn_verification_controller_spec.rb +++ b/spec/controllers/two_factor_authentication/webauthn_verification_controller_spec.rb @@ -91,6 +91,11 @@ ) patch :confirm, params: params + + expect(subject.user_session[:auth_method]).to eq( + TwoFactorAuthenticatable::AuthMethod::WEBAUTHN, + ) + expect(subject.user_session[TwoFactorAuthenticatable::NEED_AUTHENTICATION]).to eq false end it 'tracks a valid platform authenticator submission' do @@ -117,6 +122,10 @@ ) patch :confirm, params: params + expect(subject.user_session[:auth_method]).to eq( + TwoFactorAuthenticatable::AuthMethod::WEBAUTHN_PLATFORM, + ) + expect(subject.user_session[TwoFactorAuthenticatable::NEED_AUTHENTICATION]).to eq false end it 'tracks an invalid submission' do @@ -173,6 +182,8 @@ it 'redirects to webauthn show page' do patch :confirm, params: params expect(response).to redirect_to login_two_factor_webauthn_url(platform: true) + expect(subject.user_session[:auth_method]).to eq nil + expect(subject.user_session[TwoFactorAuthenticatable::NEED_AUTHENTICATION]).to eq true end it 'displays flash error message' do diff --git a/spec/controllers/users/piv_cac_login_controller_spec.rb b/spec/controllers/users/piv_cac_login_controller_spec.rb index 6f47682d06b..465c3150542 100644 --- a/spec/controllers/users/piv_cac_login_controller_spec.rb +++ b/spec/controllers/users/piv_cac_login_controller_spec.rb @@ -146,14 +146,6 @@ end describe 'it handles the otp_context' do - it 'sets the otp user_session' do - expect(controller.user_session[:auth_method]). - to eq TwoFactorAuthenticatable::AuthMethod::PIV_CAC - - expect(controller.user_session[:unconfirmed_phone]).to be nil - expect(controller.user_session[:context]).to eq 'authentication' - end - it 'tracks the user_marked_authed event' do expect(@analytics).to have_received(:track_event).with( 'User marked authenticated', diff --git a/spec/support/controller_helper.rb b/spec/support/controller_helper.rb index 903850b0cf3..e5cb0c4ad8b 100644 --- a/spec/support/controller_helper.rb +++ b/spec/support/controller_helper.rb @@ -4,14 +4,13 @@ module ControllerHelper def sign_in_as_user(user = create(:user, :fully_registered, password: VALID_PASSWORD)) @request.env['devise.mapping'] = Devise.mappings[:user] sign_in user + controller.user_session[TwoFactorAuthenticatable::NEED_AUTHENTICATION] = true user end def sign_in_before_2fa(user = create(:user, :fully_registered)) sign_in_as_user(user) controller.current_user.create_direct_otp - allow(controller).to receive(:user_fully_authenticated?).and_return(false) - allow(controller).to receive(:signed_in_url).and_return(account_url) end def stub_sign_in(user = build(:user, password: VALID_PASSWORD)) @@ -32,6 +31,7 @@ def stub_sign_in_before_2fa(user = build(:user, password: VALID_PASSWORD)) allow(controller).to receive(:current_user).and_return(user) allow(controller).to receive(:user_fully_authenticated?).and_return(false) allow(controller).to receive(:signed_in_url).and_return(account_url) + controller.user_session[TwoFactorAuthenticatable::NEED_AUTHENTICATION] = true end def stub_idv_steps_before_verify_step(user) From 1ddc3e39b608ffb31341d865090dab60cc2a50d1 Mon Sep 17 00:00:00 2001 From: Andrew Duthie Date: Fri, 26 May 2023 08:50:51 -0400 Subject: [PATCH 13/20] Upgrade ViewComponent dependencies (#8469) * Upgrade ViewComponent dependencies changelog: Internal, Dependencies, Update UI component dependencies to latest versions * Password Confirmation: Fix translate in constructor error * Move listen to development+test group Avoid error in build See: https://gsa-tts.slack.com/archives/C0NGESUN5/p1685025108314149 --- Gemfile | 6 +++--- Gemfile.lock | 15 +++++++------- .../javascript_required_component.html.erb | 2 +- .../password_confirmation_component.html.erb | 6 +++--- .../password_confirmation_component.rb | 12 +---------- .../password_toggle_component.html.erb | 2 +- app/components/password_toggle_component.rb | 13 ++++++++---- .../recovery_options/show.html.erb | 8 ++++---- app/views/idv/cancellations/destroy.html.erb | 2 +- app/views/idv/cancellations/new.html.erb | 8 ++++---- app/views/idv/doc_auth/welcome.html.erb | 6 +++--- app/views/idv/forgot_password/new.html.erb | 6 +++--- app/views/idv/gpo_only_warning/show.html.erb | 14 ++++++------- .../in_person/ready_to_verify/show.html.erb | 8 ++++---- app/views/idv/review/new.html.erb | 2 +- app/views/idv/session_errors/warning.html.erb | 6 +++--- app/views/idv/unavailable/show.html.erb | 6 +++--- app/views/shared/_password_accordion.html.erb | 2 +- app/views/shared/_personal_key.html.erb | 2 +- .../shared/_troubleshooting_options.html.erb | 4 ++-- app/views/sign_up/cancellations/new.html.erb | 6 +++--- .../_required_pii_accordion.html.erb | 2 +- .../_locked.html.erb | 10 +++++----- .../phone_setup/spam_protection.html.erb | 6 +++--- .../piv_cac_authentication_setup/new.html.erb | 6 +++--- app/views/users/totp_setup/new.html.erb | 8 ++++---- app/views/users/verify_password/new.html.erb | 2 +- spec/components/accordion_component_spec.rb | 2 +- spec/components/base_component_spec.rb | 3 +-- .../captcha_submit_button_component_spec.rb | 3 +-- spec/components/icon_component_spec.rb | 4 +++- .../javascript_required_component_spec.rb | 2 +- .../memorable_date_component_spec.rb | 3 +-- .../one_time_code_input_component_spec.rb | 3 +-- .../password_confirmation_component_spec.rb | 12 +++++++++-- .../password_toggle_component_spec.rb | 14 +++++++++++-- spec/components/phone_input_component_spec.rb | 3 +-- .../previews/accordion_component_preview.rb | 6 +++--- ...password_confirmation_component_preview.rb | 10 ++-------- .../process_list_component_preview.rb | 20 +++++++++---------- ...oubleshooting_options_component_preview.rb | 16 +++++++-------- .../components/process_list_component_spec.rb | 6 +++--- spec/components/status_page_component_spec.rb | 16 +++++++-------- .../tab_navigation_component_spec.rb | 2 +- .../troubleshooting_options_component_spec.rb | 12 +++++------ .../validated_field_component_spec.rb | 3 +-- 46 files changed, 156 insertions(+), 154 deletions(-) diff --git a/Gemfile b/Gemfile index 5a89e48b48d..40a1feb67f2 100644 --- a/Gemfile +++ b/Gemfile @@ -34,7 +34,7 @@ gem 'jsbundling-rails', '~> 1.0.0' gem 'jwe' gem 'jwt' gem 'lograge', '>= 0.11.2' -gem 'lookbook', '~> 1.5.3', require: false +gem 'lookbook', '~> 2.0.0', require: false gem 'lru_redux' gem 'mail' gem 'msgpack', '~> 1.6' @@ -70,7 +70,7 @@ gem 'subprocess', require: false gem 'terminal-table', require: false gem 'uglifier', '~> 4.2' gem 'valid_email', '>= 0.1.3' -gem 'view_component', '~> 2.82.0' +gem 'view_component', '~> 3.0.0' gem 'webauthn', '~> 2.5.2' gem 'xmldsig', '~> 0.6' gem 'xmlenc', '~> 0.7', '>= 0.7.1' @@ -87,7 +87,6 @@ group :development do gem 'guard-rspec', require: false gem 'irb' gem 'letter_opener', '~> 1.8' - gem 'listen' gem 'octokit', '>= 4.25.0' gem 'rack-mini-profiler', '>= 1.1.3', require: false gem 'rails-erd', '>= 1.6.0' @@ -101,6 +100,7 @@ group :development, :test do gem 'erb_lint', '~> 0.3.0', require: false gem 'i18n-tasks', '~> 1.0' gem 'knapsack' + gem 'listen' gem 'nokogiri', '~> 1.14.0' gem 'pg_query', require: false gem 'pry-byebug' diff --git a/Gemfile.lock b/Gemfile.lock index dd8c882db42..1a366e8b663 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -378,17 +378,16 @@ GEM loofah (2.20.0) crass (~> 1.0.2) nokogiri (>= 1.5.9) - lookbook (1.5.3) - actioncable + lookbook (2.0.3) activemodel css_parser htmlbeautifier (~> 1.3) htmlentities (~> 4.3.4) - listen (~> 3.0) + marcel (~> 1.0) railties (>= 5.0) redcarpet (~> 3.5) rouge (>= 3.26, < 5.0) - view_component (> 2.0, < 4) + view_component (>= 2.0) yard (~> 0.9.25) zeitwerk (~> 2.5) lru_redux (1.1.0) @@ -552,7 +551,7 @@ GEM retries (0.0.5) rexml (3.2.5) rotp (6.2.0) - rouge (4.1.0) + rouge (4.1.1) rqrcode (2.1.0) chunky_png (~> 1.0) rqrcode_core (~> 1.0) @@ -670,7 +669,7 @@ GEM activemodel mail (>= 2.6.1) simpleidn - view_component (2.82.0) + view_component (3.0.0) activesupport (>= 5.2.0, < 8.0) concurrent-ruby (~> 1.0) method_source (~> 1.0) @@ -770,7 +769,7 @@ DEPENDENCIES letter_opener (~> 1.8) listen lograge (>= 0.11.2) - lookbook (~> 1.5.3) + lookbook (~> 2.0.0) lru_redux mail maxminddb @@ -831,7 +830,7 @@ DEPENDENCIES terminal-table uglifier (~> 4.2) valid_email (>= 0.1.3) - view_component (~> 2.82.0) + view_component (~> 3.0.0) webauthn (~> 2.5.2) webdrivers (~> 5.2.0) webmock diff --git a/app/components/javascript_required_component.html.erb b/app/components/javascript_required_component.html.erb index ece80238a07..514068d4efe 100644 --- a/app/components/javascript_required_component.html.erb +++ b/app/components/javascript_required_component.html.erb @@ -1,6 +1,6 @@
<%= render AccordionComponent.new do |c| %> - <% c.header { t('idv.messages.review.intro') } %> + <% c.with_header { t('idv.messages.review.intro') } %> <%= render 'shared/pii_review', pii: @applicant, phone: PhoneFormatter.format(@applicant[:phone]) %> <% end %> diff --git a/app/views/idv/session_errors/warning.html.erb b/app/views/idv/session_errors/warning.html.erb index efde3b5ab71..950cc1e4855 100644 --- a/app/views/idv/session_errors/warning.html.erb +++ b/app/views/idv/session_errors/warning.html.erb @@ -1,12 +1,12 @@ <% title t('titles.failure.information_not_verified') %> <%= render StatusPageComponent.new(status: :warning) do |c| %> - <% c.header { t('idv.warning.sessions.heading') } %> + <% c.with_header { t('idv.warning.sessions.heading') } %>

<%= t('idv.failure.sessions.warning') %>

<%= t('idv.warning.attempts', count: @remaining_attempts) %>

- <% c.action_button( + <% c.with_action_button( action: ->(**tag_options, &block) { link_to(@try_again_path, **tag_options, &block) }, class: 'margin-bottom-2', ) { t('idv.forgot_password.try_again') } %> @@ -14,4 +14,4 @@ <%= render PageFooterComponent.new do %> <%= link_to t('links.cancel'), idv_cancel_path %> -<% end %> \ No newline at end of file +<% end %> diff --git a/app/views/idv/unavailable/show.html.erb b/app/views/idv/unavailable/show.html.erb index 929e4228560..c385a886380 100644 --- a/app/views/idv/unavailable/show.html.erb +++ b/app/views/idv/unavailable/show.html.erb @@ -2,7 +2,7 @@ <%= render StatusPageComponent.new(status: :error) do |c| %> - <% c.header { t('idv.titles.unavailable') } %> + <% c.with_header { t('idv.titles.unavailable') } %>

<% if decorated_session.sp_name.present? %> @@ -27,7 +27,7 @@ ) %>

- <% c.action_button( + <% c.with_action_button( action: ->(**tag_options, &block) do link_to( return_to_sp_failure_to_proof_path(location: :unavailable), @@ -39,4 +39,4 @@ wide: true, ).with_content(t('idv.unavailable.exit_button', app_name: APP_NAME)) %> -<% end %> \ No newline at end of file +<% end %> diff --git a/app/views/shared/_password_accordion.html.erb b/app/views/shared/_password_accordion.html.erb index ec381c508bc..73bf9410cac 100644 --- a/app/views/shared/_password_accordion.html.erb +++ b/app/views/shared/_password_accordion.html.erb @@ -1,4 +1,4 @@ <%= render AccordionComponent.new(class: 'margin-bottom-4') do |c| %> - <% c.header { t('instructions.password.help_text_header') } %> + <% c.with_header { t('instructions.password.help_text_header') } %> <%= simple_format(t('instructions.password.help_text')) %> <% end %> diff --git a/app/views/shared/_personal_key.html.erb b/app/views/shared/_personal_key.html.erb index 4461cdece24..a78a49432ca 100644 --- a/app/views/shared/_personal_key.html.erb +++ b/app/views/shared/_personal_key.html.erb @@ -8,7 +8,7 @@
<%= render AccordionComponent.new do |c| %> - <% c.header { t('forms.personal_key_partial.explanation.header') } %> + <% c.with_header { t('forms.personal_key_partial.explanation.header') } %> <% t('forms.personal_key_partial.explanation.text').each do |paragraph| %>

<%= paragraph %>

<% end %> diff --git a/app/views/shared/_troubleshooting_options.html.erb b/app/views/shared/_troubleshooting_options.html.erb index 4d8d9700c17..f073ba90651 100644 --- a/app/views/shared/_troubleshooting_options.html.erb +++ b/app/views/shared/_troubleshooting_options.html.erb @@ -6,6 +6,6 @@ locals: * class: Additional class names to add to wrapper element. %> <%= render TroubleshootingOptionsComponent.new(class: local_assigns[:class]) do |c| %> - <% c.header(heading_level: local_assigns[:heading_tag] || :h2).with_content(heading) %> - <% options.each { |option| c.option(**option.except(:text)).with_content(option[:text]) } %> + <% c.with_header(heading_level: local_assigns[:heading_tag] || :h2).with_content(heading) %> + <% options.each { |option| c.with_option(**option.except(:text)).with_content(option[:text]) } %> <% end %> diff --git a/app/views/sign_up/cancellations/new.html.erb b/app/views/sign_up/cancellations/new.html.erb index c2681db6288..38cc751a3dc 100644 --- a/app/views/sign_up/cancellations/new.html.erb +++ b/app/views/sign_up/cancellations/new.html.erb @@ -1,7 +1,7 @@ <% title t('headings.cancellations.prompt') %> <%= render StatusPageComponent.new(status: :warning) do |c| %> - <% c.header { t('headings.cancellations.prompt') } %> + <% c.with_header { t('headings.cancellations.prompt') } %>

<%= t('sign_up.cancel.warning_header') %> @@ -14,13 +14,13 @@

  • <%= t('users.delete.bullet_4', app_name: APP_NAME) %>
    • - <% c.action_button( + <% c.with_action_button( action: ->(**tag_options, &block) do button_to(sign_up_destroy_path, method: :delete, **tag_options, &block) end, ) { t('forms.buttons.cancel') } %> - <% c.action_button( + <% c.with_action_button( action: ->(**tag_options, &block) do link_to(@presenter.go_back_path, **tag_options, &block) end, diff --git a/app/views/sign_up/registrations/_required_pii_accordion.html.erb b/app/views/sign_up/registrations/_required_pii_accordion.html.erb index 355a95cd0d5..15c157aa501 100644 --- a/app/views/sign_up/registrations/_required_pii_accordion.html.erb +++ b/app/views/sign_up/registrations/_required_pii_accordion.html.erb @@ -9,7 +9,7 @@

    <%= render AccordionComponent.new(class: 'margin-y-2') do |c| %> - <% c.header { t('devise.registrations.start.accordion') } %> + <% c.with_header { t('devise.registrations.start.accordion') } %>

    <%= image_tag( asset_url('get-started/email-password.svg'), diff --git a/app/views/two_factor_authentication/_locked.html.erb b/app/views/two_factor_authentication/_locked.html.erb index 0360cf507d2..aa792d032dd 100644 --- a/app/views/two_factor_authentication/_locked.html.erb +++ b/app/views/two_factor_authentication/_locked.html.erb @@ -1,7 +1,7 @@ <% title t('titles.account_locked') %> <%= render StatusPageComponent.new(status: :error, icon: :lock) do |c| %> - <% c.header { t('titles.account_locked') } %> + <% c.with_header { t('titles.account_locked') } %>

    <%= presenter.locked_reason %>

    @@ -14,13 +14,13 @@ ) %>

    - <% c.troubleshooting_options do |tc| %> - <% tc.header { t('components.troubleshooting_options.default_heading') } %> - <% tc.option( + <% c.with_troubleshooting_options do |tc| %> + <% tc.with_header { t('components.troubleshooting_options.default_heading') } %> + <% tc.with_option( url: MarketingSite.help_url, new_tab: true, ).with_content(t('two_factor_authentication.read_about_two_factor_authentication')) %> - <% tc.option( + <% tc.with_option( url: MarketingSite.contact_url, new_tab: true, ).with_content(t('idv.troubleshooting.options.contact_support', app_name: APP_NAME)) %> diff --git a/app/views/users/phone_setup/spam_protection.html.erb b/app/views/users/phone_setup/spam_protection.html.erb index f972185299d..61d823d06f7 100644 --- a/app/views/users/phone_setup/spam_protection.html.erb +++ b/app/views/users/phone_setup/spam_protection.html.erb @@ -42,13 +42,13 @@ <% end %> <%= render TroubleshootingOptionsComponent.new do |c| %> - <% c.header { t('components.troubleshooting_options.default_heading') } %> + <% c.with_header { t('components.troubleshooting_options.default_heading') } %> <% if local_assigns[:two_factor_options_path].present? %> - <% c.option( + <% c.with_option( url: two_factor_options_path, ).with_content(t('two_factor_authentication.login_options_link_text')) %> <% end %> - <% c.option( + <% c.with_option( url: help_center_redirect_path( category: 'get-started', article: 'authentication-options', diff --git a/app/views/users/piv_cac_authentication_setup/new.html.erb b/app/views/users/piv_cac_authentication_setup/new.html.erb index 5c8ae1e349e..cfa50095078 100644 --- a/app/views/users/piv_cac_authentication_setup/new.html.erb +++ b/app/views/users/piv_cac_authentication_setup/new.html.erb @@ -6,7 +6,7 @@ <%= simple_form_for('', url: submit_new_piv_cac_url) do |f| %> <%= render ProcessListComponent.new(connected: true, class: 'margin-y-4') do |c| %> - <%= c.item(heading: t('instructions.mfa.piv_cac.step_1')) do %> + <%= c.with_item(heading: t('instructions.mfa.piv_cac.step_1')) do %>

    <%= t('instructions.mfa.piv_cac.step_1_info') %>

    @@ -23,8 +23,8 @@ }, ) %> <% end %> - <%= c.item(heading: t('instructions.mfa.piv_cac.step_2')) %> - <%= c.item(heading: t('instructions.mfa.piv_cac.step_3')) do %> + <%= c.with_item(heading: t('instructions.mfa.piv_cac.step_2')) %> + <%= c.with_item(heading: t('instructions.mfa.piv_cac.step_3')) do %>

    <%= t('instructions.mfa.piv_cac.step_3_info_html') %>

    diff --git a/app/views/users/totp_setup/new.html.erb b/app/views/users/totp_setup/new.html.erb index 2e4afd1a318..9f147e7f2bf 100644 --- a/app/views/users/totp_setup/new.html.erb +++ b/app/views/users/totp_setup/new.html.erb @@ -11,7 +11,7 @@ <%= simple_form_for('', method: :patch, html: { class: 'margin-bottom-4' }) do |f| %> <%= render ProcessListComponent.new(connected: true, class: 'margin-y-4') do |c| %> - <%= c.item(heading: t('forms.totp_setup.totp_step_1')) do %> + <%= c.with_item(heading: t('forms.totp_setup.totp_step_1')) do %>

    <%= t('forms.totp_setup.totp_step_1a') %>

    <%= render ValidatedFieldComponent.new( form: f, @@ -25,8 +25,8 @@ }, ) %> <% end %> - <%= c.item(heading: t('forms.totp_setup.totp_step_2')) %> - <%= c.item(heading: t('forms.totp_setup.totp_step_3')) do %> + <%= c.with_item(heading: t('forms.totp_setup.totp_step_2')) %> + <%= c.with_item(heading: t('forms.totp_setup.totp_step_3')) do %>
    <%= image_tag @qrcode, size: 240, skip_pipeline: true, alt: t('image_description.totp_qrcode') %>
    @@ -36,7 +36,7 @@ <%= render ClipboardButtonComponent.new(clipboard_text: @code.upcase, outline: true) %> <% end %> - <%= c.item(heading: t('forms.totp_setup.totp_step_4')) do %> + <%= c.with_item(heading: t('forms.totp_setup.totp_step_4')) do %> <%= render OneTimeCodeInputComponent.new( form: f, transport: nil, diff --git a/app/views/users/verify_password/new.html.erb b/app/views/users/verify_password/new.html.erb index a8fa3b8e44d..4062087225e 100644 --- a/app/views/users/verify_password/new.html.erb +++ b/app/views/users/verify_password/new.html.erb @@ -26,7 +26,7 @@
    <%= render AccordionComponent.new do |c| %> - <% c.header { t('idv.messages.review.intro') } %> + <% c.with_header { t('idv.messages.review.intro') } %> <%= render 'shared/pii_review', pii: @decrypted_pii, phone: @decrypted_pii[:phone] %> <% end %>
    diff --git a/spec/components/accordion_component_spec.rb b/spec/components/accordion_component_spec.rb index 868f97b4a06..df1bec0e45e 100644 --- a/spec/components/accordion_component_spec.rb +++ b/spec/components/accordion_component_spec.rb @@ -7,7 +7,7 @@ subject(:rendered) do render_inline(described_class.new(**options)) do |c| - c.header { 'heading' } + c.with_header { 'heading' } 'content' end end diff --git a/spec/components/base_component_spec.rb b/spec/components/base_component_spec.rb index cb7fec481aa..6c9a23989e5 100644 --- a/spec/components/base_component_spec.rb +++ b/spec/components/base_component_spec.rb @@ -7,8 +7,7 @@ def call end end - let(:lookup_context) { ActionView::LookupContext.new(ActionController::Base.view_paths) } - let(:view_context) { ActionView::Base.new(lookup_context, {}, controller) } + let(:view_context) { vc_test_controller.view_context } before do allow_any_instance_of(ApplicationController).to receive(:view_context).and_return(view_context) diff --git a/spec/components/captcha_submit_button_component_spec.rb b/spec/components/captcha_submit_button_component_spec.rb index bfc8fda14a9..05cbea1a01d 100644 --- a/spec/components/captcha_submit_button_component_spec.rb +++ b/spec/components/captcha_submit_button_component_spec.rb @@ -1,8 +1,7 @@ require 'rails_helper' RSpec.describe CaptchaSubmitButtonComponent, type: :component do - let(:lookup_context) { ActionView::LookupContext.new(ActionController::Base.view_paths) } - let(:view_context) { ActionView::Base.new(lookup_context, {}, controller) } + let(:view_context) { vc_test_controller.view_context } let(:form_object) { NewPhoneForm.new(user: User.new) } let(:form) { SimpleForm::FormBuilder.new('', form_object, view_context, {}) } let(:content) { 'Button' } diff --git a/spec/components/icon_component_spec.rb b/spec/components/icon_component_spec.rb index 2b0935110cc..36a8daf51f3 100644 --- a/spec/components/icon_component_spec.rb +++ b/spec/components/icon_component_spec.rb @@ -12,7 +12,9 @@ it 'renders icon svg' do rendered = render_inline IconComponent.new(icon: :print) - expect(rendered).to have_css(".usa-icon use[href^='#{request.base_url}'][href$='.svg#print']") + expect(rendered).to have_css( + ".usa-icon use[href^='#{vc_test_request.base_url}'][href$='.svg#print']", + ) end context 'with invalid icon' do diff --git a/spec/components/javascript_required_component_spec.rb b/spec/components/javascript_required_component_spec.rb index 5e03ff6c2cc..b9e506f21c0 100644 --- a/spec/components/javascript_required_component_spec.rb +++ b/spec/components/javascript_required_component_spec.rb @@ -39,7 +39,7 @@ context 'with session which was previously no-js' do before do - controller.session[NoJsController::SESSION_KEY] = true + vc_test_controller.session[NoJsController::SESSION_KEY] = true end it 'renders alert confirming successful enabling of JS' do diff --git a/spec/components/memorable_date_component_spec.rb b/spec/components/memorable_date_component_spec.rb index f59684116dc..164ff54991d 100644 --- a/spec/components/memorable_date_component_spec.rb +++ b/spec/components/memorable_date_component_spec.rb @@ -3,8 +3,7 @@ RSpec.describe MemorableDateComponent, type: :component do include SimpleForm::ActionViewExtensions::FormHelper - let(:lookup_context) { ActionView::LookupContext.new(ActionController::Base.view_paths) } - let(:view_context) { ActionView::Base.new(lookup_context, {}, controller) } + let(:view_context) { vc_test_controller.view_context } let(:form_object) { Date.new } let(:form_builder) do SimpleForm::FormBuilder.new('MemorableDate', form_object, view_context, {}) diff --git a/spec/components/one_time_code_input_component_spec.rb b/spec/components/one_time_code_input_component_spec.rb index 9083fea27d8..c210e3b5d3d 100644 --- a/spec/components/one_time_code_input_component_spec.rb +++ b/spec/components/one_time_code_input_component_spec.rb @@ -3,8 +3,7 @@ RSpec.describe OneTimeCodeInputComponent, type: :component do include SimpleForm::ActionViewExtensions::FormHelper - let(:lookup_context) { ActionView::LookupContext.new(ActionController::Base.view_paths) } - let(:view_context) { ActionView::Base.new(lookup_context, {}, controller) } + let(:view_context) { vc_test_controller.view_context } let(:form) { SimpleForm::FormBuilder.new('', {}, view_context, {}) } let(:options) { {} } diff --git a/spec/components/password_confirmation_component_spec.rb b/spec/components/password_confirmation_component_spec.rb index 54f52b3f631..755d198e4d9 100644 --- a/spec/components/password_confirmation_component_spec.rb +++ b/spec/components/password_confirmation_component_spec.rb @@ -1,8 +1,7 @@ require 'rails_helper' RSpec.describe PasswordConfirmationComponent, type: :component do - let(:lookup_context) { ActionView::LookupContext.new(ActionController::Base.view_paths) } - let(:view_context) { ActionView::Base.new(lookup_context, {}, controller) } + let(:view_context) { vc_test_controller.view_context } let(:form) { SimpleForm::FormBuilder.new('', {}, view_context, {}) } subject(:rendered) do @@ -11,5 +10,14 @@ it 'renders password fields with expected attributes' do expect(rendered).to have_css('[type=password][autocomplete=new-password]', count: 2) + expect(rendered).to have_field(t('forms.password'), type: :password) + expect(rendered).to have_field( + t('components.password_confirmation.confirm_label'), + type: :password, + ) + expect(rendered).to have_field( + t('components.password_confirmation.toggle_label'), + type: :checkbox, + ) end end diff --git a/spec/components/password_toggle_component_spec.rb b/spec/components/password_toggle_component_spec.rb index bb4ea1fbc8e..726a9f8a27f 100644 --- a/spec/components/password_toggle_component_spec.rb +++ b/spec/components/password_toggle_component_spec.rb @@ -3,8 +3,7 @@ RSpec.describe PasswordToggleComponent, type: :component do include SimpleForm::ActionViewExtensions::FormHelper - let(:lookup_context) { ActionView::LookupContext.new(ActionController::Base.view_paths) } - let(:view_context) { ActionView::Base.new(lookup_context, {}, controller) } + let(:view_context) { vc_test_controller.view_context } let(:form) { SimpleForm::FormBuilder.new('', {}, view_context, {}) } let(:options) { {} } @@ -23,6 +22,17 @@ expect(rendered).to have_css("[aria-controls='#{input_id}']") end + describe '#label' do + context 'with custom label' do + let(:label) { 'Custom Label' } + let(:options) { { label: } } + + it 'renders custom field label' do + expect(rendered).to have_field(label, type: :password) + end + end + end + describe '#toggle_label' do context 'with custom label' do let(:toggle_label) { 'Custom Toggle Label' } diff --git a/spec/components/phone_input_component_spec.rb b/spec/components/phone_input_component_spec.rb index 22eaf363686..1d45245e9bc 100644 --- a/spec/components/phone_input_component_spec.rb +++ b/spec/components/phone_input_component_spec.rb @@ -3,8 +3,7 @@ RSpec.describe PhoneInputComponent, type: :component do include SimpleForm::ActionViewExtensions::FormHelper - let(:lookup_context) { ActionView::LookupContext.new(ActionController::Base.view_paths) } - let(:view_context) { ActionView::Base.new(lookup_context, {}, controller) } + let(:view_context) { vc_test_controller.view_context } let(:user) { build_stubbed(:user) } let(:form_object) { NewPhoneForm.new(user:) } let(:form_builder) do diff --git a/spec/components/previews/accordion_component_preview.rb b/spec/components/previews/accordion_component_preview.rb index 05acf787030..bf123be7edc 100644 --- a/spec/components/previews/accordion_component_preview.rb +++ b/spec/components/previews/accordion_component_preview.rb @@ -2,14 +2,14 @@ class AccordionComponentPreview < BaseComponentPreview # @!group Preview def default render(AccordionComponent.new) do |c| - c.header { 'Header' } + c.with_header { 'Header' } 'Content' end end def unbordered render(AccordionComponent.new(bordered: false)) do |c| - c.header { 'Header' } + c.with_header { 'Header' } 'Content' end end @@ -20,7 +20,7 @@ def unbordered # @param bordered toggle def workbench(header: 'Header', content: 'Content', bordered: true) render(AccordionComponent.new(bordered:)) do |c| - c.header { header } + c.with_header { header } content end end diff --git a/spec/components/previews/password_confirmation_component_preview.rb b/spec/components/previews/password_confirmation_component_preview.rb index d0421e2aa93..6ebf222f970 100644 --- a/spec/components/previews/password_confirmation_component_preview.rb +++ b/spec/components/previews/password_confirmation_component_preview.rb @@ -7,13 +7,7 @@ def default # @!endgroup # @display form true - # @param toggle_label text - def workbench(toggle_label: nil) - render( - PasswordConfirmationComponent.new( - form: form_builder, - **{ toggle_label: }.compact, - ), - ) + def workbench + render(PasswordConfirmationComponent.new(form: form_builder)) end end diff --git a/spec/components/previews/process_list_component_preview.rb b/spec/components/previews/process_list_component_preview.rb index bc503465460..5e2e6734bc2 100644 --- a/spec/components/previews/process_list_component_preview.rb +++ b/spec/components/previews/process_list_component_preview.rb @@ -2,29 +2,29 @@ class ProcessListComponentPreview < BaseComponentPreview # @!group Preview def default render(ProcessListComponent.new) do |c| - c.item(heading: 'Item 1') { 'Item 1 Content' } - c.item(heading: 'Item 2') { 'Item 2 Content' } + c.with_item(heading: 'Item 1') { 'Item 1 Content' } + c.with_item(heading: 'Item 2') { 'Item 2 Content' } end end def connected render(ProcessListComponent.new(connected: true)) do |c| - c.item(heading: 'Item 1') { 'Item 1 Content' } - c.item(heading: 'Item 2') { 'Item 2 Content' } + c.with_item(heading: 'Item 1') { 'Item 1 Content' } + c.with_item(heading: 'Item 2') { 'Item 2 Content' } end end def big render(ProcessListComponent.new(big: true)) do |c| - c.item(heading: 'Item 1') { 'Item 1 Content' } - c.item(heading: 'Item 2') { 'Item 2 Content' } + c.with_item(heading: 'Item 1') { 'Item 1 Content' } + c.with_item(heading: 'Item 2') { 'Item 2 Content' } end end def big_and_connected render(ProcessListComponent.new(big: true, connected: true)) do |c| - c.item(heading: 'Item 1') { 'Item 1 Content' } - c.item(heading: 'Item 2') { 'Item 2 Content' } + c.with_item(heading: 'Item 1') { 'Item 1 Content' } + c.with_item(heading: 'Item 2') { 'Item 2 Content' } end end # @!endgroup @@ -33,8 +33,8 @@ def big_and_connected # @param big toggle def workbench(big: false, connected: false) render(ProcessListComponent.new(big:, connected:)) do |c| - c.item(heading: 'Item 1') { 'Item 1 Content' } - c.item(heading: 'Item 2') { 'Item 2 Content' } + c.with_item(heading: 'Item 1') { 'Item 1 Content' } + c.with_item(heading: 'Item 2') { 'Item 2 Content' } end end end diff --git a/spec/components/previews/troubleshooting_options_component_preview.rb b/spec/components/previews/troubleshooting_options_component_preview.rb index b56a4ebf1fd..5a74cdf5461 100644 --- a/spec/components/previews/troubleshooting_options_component_preview.rb +++ b/spec/components/previews/troubleshooting_options_component_preview.rb @@ -2,10 +2,10 @@ class TroubleshootingOptionsComponentPreview < BaseComponentPreview # @!group Preview def default render(TroubleshootingOptionsComponent.new) do |c| - c.header { 'Header' } - c.option(url: '') { 'Option 1' } - c.option(url: '') { 'Option 2' } - c.option(url: '', new_tab: true) { 'Option 3 (New Tab)' } + c.with_header { 'Header' } + c.with_option(url: '') { 'Option 1' } + c.with_option(url: '') { 'Option 2' } + c.with_option(url: '', new_tab: true) { 'Option 3 (New Tab)' } end end # @!endgroup @@ -13,10 +13,10 @@ def default # @param header text def workbench(header: 'Header') render(TroubleshootingOptionsComponent.new) do |c| - c.header { header } - c.option(url: '') { 'Option 1' } - c.option(url: '') { 'Option 2' } - c.option(url: '', new_tab: true) { 'Option 3 (New Tab)' } + c.with_header { header } + c.with_option(url: '') { 'Option 1' } + c.with_option(url: '') { 'Option 2' } + c.with_option(url: '', new_tab: true) { 'Option 3 (New Tab)' } end end end diff --git a/spec/components/process_list_component_spec.rb b/spec/components/process_list_component_spec.rb index 138f727cf78..e835a40d62b 100644 --- a/spec/components/process_list_component_spec.rb +++ b/spec/components/process_list_component_spec.rb @@ -9,8 +9,8 @@ it 'renders items slot content at default heading level' do rendered = render_inline ProcessListComponent.new do |c| - c.item(heading: 'Item 1') { 'Item 1 Content' } - c.item(heading: 'Item 2') { 'Item 2 Content' } + c.with_item(heading: 'Item 1') { 'Item 1 Content' } + c.with_item(heading: 'Item 2') { 'Item 2 Content' } end expect(rendered).to have_css('h2.usa-process-list__heading', text: 'Item 1') @@ -22,7 +22,7 @@ context 'custom heading level' do it 'renders items slot content at custom heading level' do rendered = render_inline ProcessListComponent.new(heading_level: :h3) do |c| - c.item(heading: 'Item') { '' } + c.with_item(heading: 'Item') { '' } end expect(rendered).to have_css('h3.usa-process-list__heading', text: 'Item') diff --git a/spec/components/status_page_component_spec.rb b/spec/components/status_page_component_spec.rb index 16c16ec67a5..980d4bf2a02 100644 --- a/spec/components/status_page_component_spec.rb +++ b/spec/components/status_page_component_spec.rb @@ -4,7 +4,7 @@ include ActionView::Helpers::TagHelper it 'renders default icon associated with status' do - rendered = render_inline(StatusPageComponent.new(status: :warning)) { |c| c.header { '' } } + rendered = render_inline(StatusPageComponent.new(status: :warning)) { |c| c.with_header { '' } } expect(rendered).to have_css( "img[alt='#{t('image_description.warning')}'][src*='warning']", @@ -13,7 +13,7 @@ it 'renders icon associated with status' do rendered = render_inline(StatusPageComponent.new(status: :error, icon: :lock)) do |c| - c.header { '' } + c.with_header { '' } end expect(rendered).to have_css( @@ -22,14 +22,14 @@ end it 'renders page heading' do - rendered = render_inline(StatusPageComponent.new) { |c| c.header { 'Heading' } } + rendered = render_inline(StatusPageComponent.new) { |c| c.with_header { 'Heading' } } expect(rendered).to have_css('h1', text: 'Heading') end it 'renders block content' do rendered = render_inline(StatusPageComponent.new) do |c| - c.header { 'Heading' } + c.with_header { 'Heading' } content_tag(:p, 'Content') end @@ -38,7 +38,7 @@ it 'renders action buttons' do rendered = render_inline(StatusPageComponent.new) do |c| - c.action_button(outline: true) { 'Cancel' } + c.with_action_button(outline: true) { 'Cancel' } end expect(rendered).to have_css( @@ -49,9 +49,9 @@ it 'renders troubleshooting options' do rendered = render_inline(StatusPageComponent.new) do |c| - c.troubleshooting_options do |tc| - tc.header { 'Troubleshooting' } - tc.option(url: '/', new_tab: true) { 'Option' } + c.with_troubleshooting_options do |tc| + tc.with_header { 'Troubleshooting' } + tc.with_option(url: '/', new_tab: true) { 'Option' } end end diff --git a/spec/components/tab_navigation_component_spec.rb b/spec/components/tab_navigation_component_spec.rb index 46225b53aa5..cfae0201448 100644 --- a/spec/components/tab_navigation_component_spec.rb +++ b/spec/components/tab_navigation_component_spec.rb @@ -25,7 +25,7 @@ context 'with link for current request' do before do - allow(request).to receive(:path).and_return('/first') + allow(vc_test_request).to receive(:path).and_return('/first') end it 'renders current link as highlighted' do diff --git a/spec/components/troubleshooting_options_component_spec.rb b/spec/components/troubleshooting_options_component_spec.rb index 593c3dbcaa8..15dfb5021f4 100644 --- a/spec/components/troubleshooting_options_component_spec.rb +++ b/spec/components/troubleshooting_options_component_spec.rb @@ -10,7 +10,7 @@ context 'with options' do it 'renders troubleshooting options' do rendered = render_inline(TroubleshootingOptionsComponent.new) do |c| - c.option(url: '/').with_content('Link Text') + c.with_option(url: '/').with_content('Link Text') end expect(rendered).to have_css('.troubleshooting-options') @@ -24,7 +24,7 @@ class: 'my-custom-class', data: { foo: 'bar' }, ), - ) { |c| c.option(url: '/').with_content('Link Text') } + ) { |c| c.with_option(url: '/').with_content('Link Text') } expect(rendered).to have_css('.troubleshooting-options.my-custom-class[data-foo="bar"]') end @@ -33,8 +33,8 @@ context 'with header' do it 'renders header' do rendered = render_inline(TroubleshootingOptionsComponent.new) do |c| - c.header { 'Heading' } - c.option(url: '/') + c.with_header { 'Heading' } + c.with_option(url: '/') end expect(rendered).to have_css('h2.troubleshooting-options__heading', text: 'Heading') @@ -43,8 +43,8 @@ context 'with custom heading level' do it 'renders header' do rendered = render_inline(TroubleshootingOptionsComponent.new) do |c| - c.header(heading_level: :h3) { 'Heading' } - c.option(url: '/') + c.with_header(heading_level: :h3) { 'Heading' } + c.with_option(url: '/') end expect(rendered).to have_css('h3.troubleshooting-options__heading', text: 'Heading') diff --git a/spec/components/validated_field_component_spec.rb b/spec/components/validated_field_component_spec.rb index eb954ee5cf1..32f3e9dbc8e 100644 --- a/spec/components/validated_field_component_spec.rb +++ b/spec/components/validated_field_component_spec.rb @@ -3,8 +3,7 @@ RSpec.describe ValidatedFieldComponent, type: :component do include SimpleForm::ActionViewExtensions::FormHelper - let(:lookup_context) { ActionView::LookupContext.new(ActionController::Base.view_paths) } - let(:view_context) { ActionView::Base.new(lookup_context, {}, controller) } + let(:view_context) { vc_test_controller.view_context } let(:form_object) { User.new } let(:form_builder) do SimpleForm::FormBuilder.new(form_object.model_name.param_key, form_object, view_context, {}) From f1f9d87977a33d661f30a6a42558f4e8b666df62 Mon Sep 17 00:00:00 2001 From: Paul Hirsch <59626817+pauldoomgov@users.noreply.github.com> Date: Fri, 26 May 2023 09:05:49 -0500 Subject: [PATCH 14/20] CI speedup with support for more parallel jobs (#8311) * Increase parallel CI jobs This increases the parallel job count to speed CI. It also adds per-step timing to script lists to help in identifying slow steps. The end result is a mild speedup shaving around 4 minutes off the typical CI run. Further work on breaking down specs could lead better performance. changelog: Internal, CI, Minor speedup * Drop back to less fan out --- .gitlab-ci.yml | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index e171730e71a..6331a2e493b 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -5,6 +5,7 @@ variables: GITLAB_CI: 'true' + FF_SCRIPT_SECTIONS: 'true' JUNIT_OUTPUT: 'true' ECR_REGISTRY: '${AWS_ACCOUNT_ID}.dkr.ecr.${AWS_REGION}.amazonaws.com' IDP_CI_SHA: 'sha256:e846ed99aa3c8f9083731105b8ef25536c7ab3ed5e9f2b781b63431badcb091b' @@ -107,7 +108,7 @@ check_changelog: specs: stage: test - parallel: 11 + parallel: 22 cache: - <<: *ruby_cache - <<: *yarn_cache @@ -128,6 +129,7 @@ specs: services: - name: postgres:13.9 alias: db-postgres + command: ["--fsync=false", "--synchronous_commit=false", "--full_page_writes=false"] - name: redis:7.0 alias: db-redis artifacts: From 6447b1e65a4a297c85877309edcd06835f03d457 Mon Sep 17 00:00:00 2001 From: Jonathan Hooper Date: Fri, 26 May 2023 10:55:37 -0400 Subject: [PATCH 15/20] LG-9959 Fix a bug in which in-person and GPO pending users do not have GPO pending status cleared (#8492) We had a bug where users who are in the GPO pending state did not have that state cleared after entering a successful code and going through in-person proofing. This resulted in an error attempting to activate these users profiles when the enrollments background job runs. changelog: Improvements, In-person proofing, A bug in which users who were letter and in-person pending did not have their letter pending status cleared after entering a letter code resulting in the user not being proofed after visiting the post office was fixed. --- app/forms/gpo_verify_form.rb | 2 +- lib/tasks/fix_in_person_pending_user.rake | 11 +++++++++++ spec/forms/gpo_verify_form_spec.rb | 1 + 3 files changed, 13 insertions(+), 1 deletion(-) create mode 100644 lib/tasks/fix_in_person_pending_user.rake diff --git a/app/forms/gpo_verify_form.rb b/app/forms/gpo_verify_form.rb index dc9be77effd..707a609d651 100644 --- a/app/forms/gpo_verify_form.rb +++ b/app/forms/gpo_verify_form.rb @@ -19,11 +19,11 @@ def submit result = valid? threatmetrix_check_failed = fraud_review_checker.fraud_check_failed? if result + pending_profile&.remove_gpo_deactivation_reason if pending_in_person_enrollment? UspsInPersonProofing::EnrollmentHelper.schedule_in_person_enrollment(user, pii) pending_profile&.deactivate(:in_person_verification_pending) elsif fraud_review_checker.fraud_check_failed? && threatmetrix_enabled? - pending_profile&.remove_gpo_deactivation_reason deactivate_for_fraud_review else pending_profile&.update!( diff --git a/lib/tasks/fix_in_person_pending_user.rake b/lib/tasks/fix_in_person_pending_user.rake new file mode 100644 index 00000000000..31df9e7a143 --- /dev/null +++ b/lib/tasks/fix_in_person_pending_user.rake @@ -0,0 +1,11 @@ +namespace :profiles do + desc 'If a profile is pending in-person and pending GPO remove the GPO pending status' + task fix_in_person_and_gpo_pending_user: :environment do + enrollments = InPersonEnrollment.pending.joins(:profile).where( + 'profiles.gpo_verification_pending_at IS NOT NULL', + ) + enrollments.each do |enrollment| + enrollment.profile.update!(gpo_verification_pending_at: nil) + end + end +end diff --git a/spec/forms/gpo_verify_form_spec.rb b/spec/forms/gpo_verify_form_spec.rb index f04e5f49cd2..b49d9c2e93a 100644 --- a/spec/forms/gpo_verify_form_spec.rb +++ b/spec/forms/gpo_verify_form_spec.rb @@ -135,6 +135,7 @@ expect(pending_profile).not_to be_active expect(pending_profile.deactivation_reason).to eq('in_person_verification_pending') + expect(pending_profile.gpo_verification_pending?).to eq(false) end it 'updates establishing in-person enrollment to pending' do From 35af6cc1fe55b0b2289e431e083b155d995e78ba Mon Sep 17 00:00:00 2001 From: John Maxwell Date: Fri, 26 May 2023 11:55:21 -0400 Subject: [PATCH 16/20] Jmax/lg 9794 delete front end async document capture code (#8396) * Removed async uploads much more carefully changelog: Internal,Background proofing,Remove the JS/TS code for background proofing --- .../components/document-capture.tsx | 35 +-- .../components/documents-step.jsx | 3 +- .../components/review-issues-step.tsx | 9 +- .../document-capture/context/upload.tsx | 16 - .../with-background-encrypted-upload.jsx | 161 ---------- app/javascript/packs/document-capture.tsx | 186 +++++------ .../components/document-capture-spec.jsx | 189 +----------- .../components/review-issues-step-spec.jsx | 41 --- .../document-capture/context/upload-spec.jsx | 35 --- .../with-background-encrypted-upload-spec.jsx | 289 ------------------ 10 files changed, 86 insertions(+), 878 deletions(-) delete mode 100644 app/javascript/packages/document-capture/higher-order/with-background-encrypted-upload.jsx delete mode 100644 spec/javascript/packages/document-capture/higher-order/with-background-encrypted-upload-spec.jsx diff --git a/app/javascript/packages/document-capture/components/document-capture.tsx b/app/javascript/packages/document-capture/components/document-capture.tsx index 5ac4e3d6ba7..d7b5c94b15e 100644 --- a/app/javascript/packages/document-capture/components/document-capture.tsx +++ b/app/javascript/packages/document-capture/components/document-capture.tsx @@ -17,42 +17,19 @@ import AnalyticsContext from '../context/analytics'; import Submission from './submission'; import SubmissionStatus from './submission-status'; import { RetrySubmissionError } from './submission-complete'; -import { BackgroundEncryptedUploadError } from '../higher-order/with-background-encrypted-upload'; import SuspenseErrorBoundary from './suspense-error-boundary'; import SubmissionInterstitial from './submission-interstitial'; import withProps from '../higher-order/with-props'; import { InPersonContext } from '../context'; -/** - * Returns a new object with specified keys removed. - * - * @param object Original object. - * @param keys Keys to remove. - * - * @return Object with keys removed. - */ -export const except = >(object: T, ...keys: string[]): Partial => - Object.entries(object).reduce((result, [key, value]) => { - if (!keys.includes(key)) { - result[key] = value; - } - - return result; - }, {}); - interface DocumentCaptureProps { - /** - * Whether submission should poll for async response. - */ - isAsyncForm?: boolean; - /** * Callback triggered on step change. */ onStepChange?: () => void; } -function DocumentCapture({ isAsyncForm = false, onStepChange = () => {} }: DocumentCaptureProps) { +function DocumentCapture({ onStepChange = () => {} }: DocumentCaptureProps) { const [formValues, setFormValues] = useState | null>(null); const [submissionError, setSubmissionError] = useState(undefined); const [stepName, setStepName] = useState(undefined); @@ -82,10 +59,10 @@ function DocumentCapture({ isAsyncForm = false, onStepChange = () => {} }: Docum const submissionFormValues = useMemo( () => formValues && { - ...(isAsyncForm ? except(formValues, 'front', 'back') : formValues), + ...formValues, flow_path: flowPath, }, - [isAsyncForm, formValues, flowPath], + [formValues, flowPath], ); let initialActiveErrors; @@ -94,17 +71,11 @@ function DocumentCapture({ isAsyncForm = false, onStepChange = () => {} }: Docum field: error.field, error, })); - } else if (submissionError instanceof BackgroundEncryptedUploadError) { - initialActiveErrors = [{ field: submissionError.baseField, error: submissionError }]; } let initialValues; if (submissionError && formValues) { initialValues = formValues; - - if (submissionError instanceof BackgroundEncryptedUploadError) { - initialValues = except(initialValues, ...submissionError.fields); - } } const inPersonSteps: FormStep[] = diff --git a/app/javascript/packages/document-capture/components/documents-step.jsx b/app/javascript/packages/document-capture/components/documents-step.jsx index c89a0aed7e5..dd1c79d2938 100644 --- a/app/javascript/packages/document-capture/components/documents-step.jsx +++ b/app/javascript/packages/document-capture/components/documents-step.jsx @@ -7,7 +7,6 @@ import HybridDocCaptureWarning from './hybrid-doc-capture-warning'; import DocumentSideAcuantCapture from './document-side-acuant-capture'; import DeviceContext from '../context/device'; import UploadContext from '../context/upload'; -import withBackgroundEncryptedUpload from '../higher-order/with-background-encrypted-upload'; import CaptureTroubleshooting from './capture-troubleshooting'; import DocumentCaptureTroubleshootingOptions from './document-capture-troubleshooting-options'; @@ -76,4 +75,4 @@ function DocumentsStep({ ); } -export default withBackgroundEncryptedUpload(DocumentsStep); +export default DocumentsStep; diff --git a/app/javascript/packages/document-capture/components/review-issues-step.tsx b/app/javascript/packages/document-capture/components/review-issues-step.tsx index 733e0d0963d..212310a1850 100644 --- a/app/javascript/packages/document-capture/components/review-issues-step.tsx +++ b/app/javascript/packages/document-capture/components/review-issues-step.tsx @@ -6,7 +6,6 @@ import { PageHeading } from '@18f/identity-components'; import { Cancel } from '@18f/identity-verify-flow'; import type { FormStepComponentProps } from '@18f/identity-form-steps'; import DocumentSideAcuantCapture from './document-side-acuant-capture'; -import withBackgroundEncryptedUpload from '../higher-order/with-background-encrypted-upload'; import type { PII } from '../services/upload'; import DocumentCaptureTroubleshootingOptions from './document-capture-troubleshooting-options'; import Warning from './warning'; @@ -46,11 +45,11 @@ interface ReviewIssuesStepValue { } interface ReviewIssuesStepProps extends FormStepComponentProps { - remainingAttempts: number; + remainingAttempts?: number; - isFailedResult: boolean; + isFailedResult?: boolean; - captureHints: boolean; + captureHints?: boolean; pii?: PII; } @@ -291,4 +290,4 @@ function ReviewIssuesStep({ ); } -export default withBackgroundEncryptedUpload(ReviewIssuesStep); +export default ReviewIssuesStep; diff --git a/app/javascript/packages/document-capture/context/upload.tsx b/app/javascript/packages/document-capture/context/upload.tsx index 9f30da741d2..3a1ac1749b0 100644 --- a/app/javascript/packages/document-capture/context/upload.tsx +++ b/app/javascript/packages/document-capture/context/upload.tsx @@ -9,8 +9,6 @@ const UploadContext = createContext({ getStatus: () => Promise.resolve({} as UploadSuccessResponse), statusPollInterval: undefined as number | undefined, isMockClient: false, - backgroundUploadURLs: {} as Record, - backgroundUploadEncryptKey: undefined as CryptoKey | undefined, flowPath: 'standard' as FlowPath, formData: {} as Record, }); @@ -111,16 +109,6 @@ interface UploadContextProviderProps { */ isMockClient?: boolean; - /** - * URLs to which payload values corresponding to key should be uploaded as soon as possible. - */ - backgroundUploadURLs: Record; - - /** - * Background upload encryption key. - */ - backgroundUploadEncryptKey?: CryptoKey; - /** * Endpoint to which payload should be sent. */ @@ -161,8 +149,6 @@ const DEFAULT_FORM_DATA = {}; function UploadContextProvider({ upload = defaultUpload, isMockClient = false, - backgroundUploadURLs = {}, - backgroundUploadEncryptKey, endpoint, statusEndpoint, statusPollInterval, @@ -181,8 +167,6 @@ function UploadContextProvider({ upload: uploadWithFormData, getStatus, statusPollInterval, - backgroundUploadURLs, - backgroundUploadEncryptKey, isMockClient, flowPath, formData, diff --git a/app/javascript/packages/document-capture/higher-order/with-background-encrypted-upload.jsx b/app/javascript/packages/document-capture/higher-order/with-background-encrypted-upload.jsx deleted file mode 100644 index 1b6fd17d571..00000000000 --- a/app/javascript/packages/document-capture/higher-order/with-background-encrypted-upload.jsx +++ /dev/null @@ -1,161 +0,0 @@ -import { useContext } from 'react'; -import { t } from '@18f/identity-i18n'; -import { FormError } from '@18f/identity-form-steps'; -import { trackError } from '@18f/identity-analytics'; -import UploadContext from '../context/upload'; -import AnalyticsContext from '../context/analytics'; - -/** - * @typedef {import('@18f/identity-form-steps').FormStepComponentProps} FormStepComponentProps - * @template V - */ - -/** - * Non-breaking space (` `) represented as unicode escape sequence, which React will more - * happily tolerate than an HTML entity. - */ -const NBSP_UNICODE = '\u00A0'; - -/** - * Returns a new string from the given string, replacing spaces with non-breaking spaces. - * - * @param {string} string Original string. - * - * @return String with non-breaking spaces. - */ -const nonBreaking = (string) => string.split(' ').join(NBSP_UNICODE); - -/** - * An error representing a failure to complete encrypted upload of image. - */ -export class BackgroundEncryptedUploadError extends FormError { - baseField = ''; - - /** @type {string[]} */ - fields = []; - - message = `${t('doc_auth.errors.upload_error')} ${nonBreaking(t('errors.messages.try_again'))}`; -} - -/** - * Returns a promise resolving to an ArrayBuffer representation of the given Blob object. - * - * @param {Blob} blob Blob object. - * - * @return {Promise} - */ -export function blobToArrayBuffer(blob) { - return new Promise((resolve, reject) => { - const reader = new window.FileReader(); - reader.onload = ({ target }) => { - resolve(/** @type {ArrayBuffer} */ (target?.result)); - }; - reader.onerror = () => reject(reader.error); - reader.readAsArrayBuffer(blob); - }); -} - -/** - * Encrypt data. - * - * @param {CryptoKey} key Encryption key. - * @param {BufferSource} iv Initialization vector. - * @param {string|Blob} value Value to encrypt. - * - * @return {Promise} Encrypted data. - */ -export async function encrypt(key, iv, value) { - const data = - typeof value === 'string' ? new TextEncoder().encode(value) : await blobToArrayBuffer(value); - - return window.crypto.subtle.encrypt( - /** @type {AesGcmParams} */ ({ - name: 'AES-GCM', - iv, - }), - key, - data, - ); -} - -const withBackgroundEncryptedUpload = (Component) => { - /** - * @param {Pick>, 'onChange'|'onError'>} props - */ - function ComposedComponent({ onChange, onError, ...props }) { - const { backgroundUploadURLs, backgroundUploadEncryptKey } = useContext(UploadContext); - const { trackEvent } = useContext(AnalyticsContext); - - /** - * @param {Record} nextValues Next values. - */ - function onChangeWithBackgroundEncryptedUpload(nextValues) { - const nextValuesWithUpload = {}; - for (const [key, value] of Object.entries(nextValues)) { - nextValuesWithUpload[key] = value; - const url = backgroundUploadURLs[key]; - if (url && value) { - const iv = window.crypto.getRandomValues(new Uint8Array(12)); - nextValuesWithUpload[`${key}_image_iv`] = window.btoa(String.fromCharCode(...iv)); - nextValuesWithUpload[`${key}_image_url`] = encrypt( - /** @type {CryptoKey} */ (backgroundUploadEncryptKey), - iv, - value, - ) - .catch((error) => { - trackEvent('IdV: document capture async upload encryption', { success: false }); - trackError(error); - - // Rethrow error to skip upload and proceed from next `catch` block. - throw error; - }) - .then((encryptedValue) => { - trackEvent('IdV: document capture async upload encryption', { success: true }); - - return window.fetch(url, { - method: 'PUT', - body: encryptedValue, - headers: { 'Content-Type': 'application/octet-stream' }, - }); - }) - .then((response) => { - const traceId = response.headers.get('X-Amzn-Trace-Id'); - trackEvent('IdV: document capture async upload submitted', { - success: response.ok, - trace_id: traceId, - status_code: response.status, - }); - - if (!response.ok) { - throw new Error(); - } - - return url; - }) - .catch(() => { - onChange({ [key]: null }); - const error = new BackgroundEncryptedUploadError(); - error.baseField = key; - error.fields = [key, `${key}_image_iv`, `${key}_image_url`]; - onError(error, { field: key }); - throw error; - }); - } - } - - onChange(nextValuesWithUpload); - } - - return ( - - ); - } - - ComposedComponent.displayName = `WithBackgroundEncryptedUpload(${ - Component.displayName || Component.name - })`; - - return ComposedComponent; -}; - -export default withBackgroundEncryptedUpload; diff --git a/app/javascript/packs/document-capture.tsx b/app/javascript/packs/document-capture.tsx index d2c20b036b1..2e1dd7f7361 100644 --- a/app/javascript/packs/document-capture.tsx +++ b/app/javascript/packs/document-capture.tsx @@ -48,17 +48,6 @@ function getServiceProvider() { return { name, failureToProofURL }; } -function getBackgroundUploadURLs(): Record<'front' | 'back', string> { - return ['front', 'back'].reduce((result, key) => { - const url = appRoot.getAttribute(`data-${key}-image-upload-url`); - if (url) { - result[key] = url; - } - - return result; - }, {} as Record<'front' | 'back', string>); -} - function getMetaContent(name): string | null { const meta = document.querySelector(`meta[name="${name}"]`); return meta?.content ?? null; @@ -78,105 +67,82 @@ const trackEvent: typeof baseTrackEvent = (event, payload) => { }); }; -(async () => { - const backgroundUploadURLs = getBackgroundUploadURLs(); - const isAsyncForm = Object.keys(backgroundUploadURLs).length > 0; - - const formData: Record = { - document_capture_session_uuid: appRoot.getAttribute('data-document-capture-session-uuid'), - locale: document.documentElement.lang, - }; - - let backgroundUploadEncryptKey; - if (isAsyncForm) { - backgroundUploadEncryptKey = await window.crypto.subtle.generateKey( - { - name: 'AES-GCM', - length: 256, - }, - true, - ['encrypt', 'decrypt'], - ); - - const exportedKey = await window.crypto.subtle.exportKey('raw', backgroundUploadEncryptKey); - formData.encryption_key = btoa(String.fromCharCode(...new Uint8Array(exportedKey))); - formData.step = 'verify_document'; - } - - const { - helpCenterRedirectUrl: helpCenterRedirectURL, - maxCaptureAttemptsBeforeTips, - maxCaptureAttemptsBeforeNativeCamera, - maxSubmissionAttemptsBeforeNativeCamera, - acuantVersion, - flowPath, - cancelUrl: cancelURL, - idvInPersonUrl: inPersonURL, - securityAndPrivacyHowItWorksUrl: securityAndPrivacyHowItWorksURL, - inPersonCtaVariantTestingEnabled, - inPersonCtaVariantActive, - inPersonUspsOutageMessageEnabled, - } = appRoot.dataset as DOMStringMap & AppRootData; +const formData: Record = { + document_capture_session_uuid: appRoot.getAttribute('data-document-capture-session-uuid'), + locale: document.documentElement.lang, +}; - const App = composeComponents( - [MarketingSiteContextProvider, { helpCenterRedirectURL, securityAndPrivacyHowItWorksURL }], - [DeviceContext.Provider, { value: device }], - [ - InPersonContext.Provider, - { - value: { - inPersonCtaVariantTestingEnabled: inPersonCtaVariantTestingEnabled === true, - inPersonCtaVariantActive, - inPersonURL, - inPersonUspsOutageMessageEnabled: inPersonUspsOutageMessageEnabled === 'true', - }, - }, - ], - [AnalyticsContextProvider, { trackEvent }], - [ - AcuantContextProvider, - { - sdkSrc: acuantVersion && `/acuant/${acuantVersion}/AcuantJavascriptWebSdk.min.js`, - cameraSrc: acuantVersion && `/acuant/${acuantVersion}/AcuantCamera.min.js`, - credentials: getMetaContent('acuant-sdk-initialization-creds'), - endpoint: getMetaContent('acuant-sdk-initialization-endpoint'), - glareThreshold, - sharpnessThreshold, +const { + helpCenterRedirectUrl: helpCenterRedirectURL, + maxCaptureAttemptsBeforeTips, + maxCaptureAttemptsBeforeNativeCamera, + maxSubmissionAttemptsBeforeNativeCamera, + acuantVersion, + flowPath, + cancelUrl: cancelURL, + idvInPersonUrl: inPersonURL, + securityAndPrivacyHowItWorksUrl: securityAndPrivacyHowItWorksURL, + inPersonCtaVariantTestingEnabled, + inPersonCtaVariantActive, + inPersonUspsOutageMessageEnabled, +} = appRoot.dataset as DOMStringMap & AppRootData; + +const App = composeComponents( + [MarketingSiteContextProvider, { helpCenterRedirectURL, securityAndPrivacyHowItWorksURL }], + [DeviceContext.Provider, { value: device }], + [ + InPersonContext.Provider, + { + value: { + inPersonCtaVariantTestingEnabled: inPersonCtaVariantTestingEnabled === true, + inPersonCtaVariantActive, + inPersonURL, + inPersonUspsOutageMessageEnabled: inPersonUspsOutageMessageEnabled === 'true', }, - ], - [ - UploadContextProvider, - { - endpoint: String(appRoot.getAttribute('data-endpoint')), - statusEndpoint: String(appRoot.getAttribute('data-status-endpoint')), - statusPollInterval: Number(appRoot.getAttribute('data-status-poll-interval-ms')), - isMockClient, - backgroundUploadURLs, - backgroundUploadEncryptKey, - formData, - flowPath, + }, + ], + [AnalyticsContextProvider, { trackEvent }], + [ + AcuantContextProvider, + { + sdkSrc: acuantVersion && `/acuant/${acuantVersion}/AcuantJavascriptWebSdk.min.js`, + cameraSrc: acuantVersion && `/acuant/${acuantVersion}/AcuantCamera.min.js`, + credentials: getMetaContent('acuant-sdk-initialization-creds'), + endpoint: getMetaContent('acuant-sdk-initialization-endpoint'), + glareThreshold, + sharpnessThreshold, + }, + ], + [ + UploadContextProvider, + { + endpoint: String(appRoot.getAttribute('data-endpoint')), + statusEndpoint: String(appRoot.getAttribute('data-status-endpoint')), + statusPollInterval: Number(appRoot.getAttribute('data-status-poll-interval-ms')), + isMockClient, + formData, + flowPath, + }, + ], + [ + FlowContext.Provider, + { + value: { + cancelURL, + currentStep: 'document_capture', }, - ], - [ - FlowContext.Provider, - { - value: { - cancelURL, - currentStep: 'document_capture', - }, - }, - ], - [ServiceProviderContextProvider, { value: getServiceProvider() }], - [ - FailedCaptureAttemptsContextProvider, - { - maxFailedAttemptsBeforeTips: Number(maxCaptureAttemptsBeforeTips), - maxCaptureAttemptsBeforeNativeCamera: Number(maxCaptureAttemptsBeforeNativeCamera), - maxSubmissionAttemptsBeforeNativeCamera: Number(maxSubmissionAttemptsBeforeNativeCamera), - }, - ], - [DocumentCapture, { isAsyncForm, onStepChange: extendSession }], - ); - - render(, appRoot); -})(); + }, + ], + [ServiceProviderContextProvider, { value: getServiceProvider() }], + [ + FailedCaptureAttemptsContextProvider, + { + maxFailedAttemptsBeforeTips: Number(maxCaptureAttemptsBeforeTips), + maxCaptureAttemptsBeforeNativeCamera: Number(maxCaptureAttemptsBeforeNativeCamera), + maxSubmissionAttemptsBeforeNativeCamera: Number(maxSubmissionAttemptsBeforeNativeCamera), + }, + ], + [DocumentCapture, { onStepChange: extendSession }], +); + +render(, appRoot); diff --git a/spec/javascript/packages/document-capture/components/document-capture-spec.jsx b/spec/javascript/packages/document-capture/components/document-capture-spec.jsx index 9d4e419ab29..a79a3550bfc 100644 --- a/spec/javascript/packages/document-capture/components/document-capture-spec.jsx +++ b/spec/javascript/packages/document-capture/components/document-capture-spec.jsx @@ -1,6 +1,5 @@ -import sinon from 'sinon'; import userEvent from '@testing-library/user-event'; -import { render as baseRender, fireEvent } from '@testing-library/react'; +import { fireEvent } from '@testing-library/react'; import { waitFor } from '@testing-library/dom'; import httpUpload, { UploadFormEntriesError, @@ -13,9 +12,7 @@ import { DeviceContext, InPersonContext, } from '@18f/identity-document-capture'; -import DocumentCapture, { - except, -} from '@18f/identity-document-capture/components/document-capture'; +import DocumentCapture from '@18f/identity-document-capture/components/document-capture'; import { FlowContext } from '@18f/identity-verify-flow'; import { expect } from 'chai'; import { useSandbox } from '@18f/identity-test-helpers'; @@ -46,16 +43,6 @@ describe('document-capture/components/document-capture', () => { window.location.hash = originalHash; }); - describe('except', () => { - it('returns a new object without the specified keys', () => { - const original = { a: 1, b: 2, c: 3, d: 4 }; - const result = except(original, 'b', 'c'); - - expect(result).to.not.equal(original); - expect(result).to.deep.equal({ a: 1, d: 4 }); - }); - }); - it('renders the form steps', () => { const { getByText } = render(); @@ -299,178 +286,6 @@ describe('document-capture/components/document-capture', () => { expect(event.defaultPrevented).to.be.false(); }); - it('renders async upload pending progress', async () => { - const statusChecks = 3; - let remainingStatusChecks = statusChecks; - sandbox.stub(window, 'fetch').resolves(new Response()); - const upload = sinon.stub().callsFake((payload, { endpoint }) => { - switch (endpoint) { - case 'about:blank#upload': - expect(payload).to.have.keys([ - 'front_image_iv', - 'front_image_url', - 'front_image_metadata', - 'back_image_iv', - 'back_image_url', - 'back_image_metadata', - 'flow_path', - ]); - - return Promise.resolve({ success: true, isPending: true }); - case 'about:blank#status': - expect(payload).to.be.empty(); - - return Promise.resolve({ success: true, isPending: Boolean(remainingStatusChecks--) }); - default: - throw new Error(); - } - }); - const key = await window.crypto.subtle.generateKey( - { - name: 'AES-GCM', - length: 256, - }, - true, - ['encrypt', 'decrypt'], - ); - - const { getByLabelText, getByText, getAllByText, findAllByText } = baseRender( - - - - - , - ); - - const submitButton = getByText('forms.buttons.submit.default'); - await userEvent.click(submitButton); - await findAllByText('simple_form.required.text'); - await userEvent.upload(getByLabelText('doc_auth.headings.document_capture_front'), validUpload); - await userEvent.upload(getByLabelText('doc_auth.headings.document_capture_back'), validUpload); - await waitFor(() => expect(() => getAllByText('simple_form.required.text')).to.throw()); - - return new Promise((resolve) => { - onSubmit.callsFake(() => { - // Error logged at initial pending retry. - expect(console).to.have.loggedError(/^Error: Uncaught/); - expect(console).to.have.loggedError(/React will try to recreate this component/); - - // Error logged at every scheduled check thereafter. - for (let i = 0; i < statusChecks; i++) { - expect(console).to.have.loggedError(/^Error: Uncaught/); - expect(console).to.have.loggedError(/React will try to recreate this component/); - } - - resolve(); - }); - - // eslint-disable-next-line no-restricted-syntax - userEvent.click(submitButton); - }); - }); - - describe('pending promise values', () => { - let completeUploadAsSuccess; - let completeUploadAsFailure; - let renderResult; - let upload; - let submit; - - beforeEach(async () => { - sandbox.stub(window, 'fetch'); - window.fetch.withArgs('about:blank#front').returns( - new Promise((resolve, reject) => { - completeUploadAsSuccess = () => resolve(new Response()); - completeUploadAsFailure = () => reject(new Error()); - }), - ); - window.fetch.withArgs('about:blank#back').resolves(new Response()); - upload = sinon.stub().resolves({ success: true, isPending: false }); - const key = await window.crypto.subtle.generateKey( - { - name: 'AES-GCM', - length: 256, - }, - true, - ['encrypt', 'decrypt'], - ); - renderResult = render( - - - - - - - , - ); - const { getByLabelText, getByText } = renderResult; - - await userEvent.upload( - getByLabelText('doc_auth.headings.document_capture_front'), - validUpload, - ); - await userEvent.upload( - getByLabelText('doc_auth.headings.document_capture_back'), - validUpload, - ); - submit = () => userEvent.click(getByText('forms.buttons.submit.default')); - }); - - context('success', () => { - it('calls to upload once pending values resolve', async () => { - await submit(); - expect(upload).not.to.have.been.called(); - completeUploadAsSuccess(); - await new Promise((resolve) => onSubmit.callsFake(resolve)); - expect(upload).to.have.been.calledOnce(); - }); - }); - - context('failure', () => { - it('shows an error screen once pending values reject', async () => { - await submit(); - expect(upload).not.to.have.been.called(); - completeUploadAsFailure(); - const { findAllByRole, getByLabelText } = renderResult; - - const alerts = (await findAllByRole('alert')).filter((alert) => alert.textContent); - expect(alerts).to.have.lengthOf(2); - expect(alerts[0].textContent).to.equal('doc_auth.errors.general.network_error'); - expect(alerts[1].textContent).to.equal( - 'doc_auth.errors.upload_error errors.messages.try_again', - ); - - const input = await getByLabelText('doc_auth.headings.document_capture_front'); - expect(input.closest('.usa-file-input--has-value')).to.be.null(); - - expect(console).to.have.loggedError(/^Error: Uncaught/); - expect(console).to.have.loggedError( - /React will try to recreate this component tree from scratch using the error boundary you provided/, - ); - - expect(upload).not.to.have.been.called(); - }); - }); - }); - describe('step indicator', () => { it('renders the step indicator', () => { const { getByText } = render(); diff --git a/spec/javascript/packages/document-capture/components/review-issues-step-spec.jsx b/spec/javascript/packages/document-capture/components/review-issues-step-spec.jsx index a2f1318b360..acfbd12d3c5 100644 --- a/spec/javascript/packages/document-capture/components/review-issues-step-spec.jsx +++ b/spec/javascript/packages/document-capture/components/review-issues-step-spec.jsx @@ -2,7 +2,6 @@ import userEvent from '@testing-library/user-event'; import sinon from 'sinon'; import { ServiceProviderContextProvider, - UploadContextProvider, AnalyticsContext, InPersonContext, } from '@18f/identity-document-capture'; @@ -10,13 +9,11 @@ import { I18n } from '@18f/identity-i18n'; import { I18nContext } from '@18f/identity-react-i18n'; import ReviewIssuesStep from '@18f/identity-document-capture/components/review-issues-step'; import { toFormEntryError } from '@18f/identity-document-capture/services/upload'; -import { useSandbox } from '@18f/identity-test-helpers'; import { render } from '../../../support/document-capture'; import { getFixtureFile } from '../../../support/file'; describe('document-capture/components/review-issues-step', () => { const DEFAULT_PROPS = { remainingAttempts: 3 }; - const sandbox = useSandbox(); it('logs warning events', async () => { const trackEvent = sinon.spy(); @@ -136,44 +133,6 @@ describe('document-capture/components/review-issues-step', () => { }); }); - it('performs background encrypted uploads', async () => { - const onChange = sandbox.stub(); - sandbox.stub(window, 'fetch').callsFake(() => - Promise.resolve({ - ok: true, - status: 200, - headers: new window.Headers(), - }), - ); - const key = await window.crypto.subtle.generateKey( - { - name: 'AES-GCM', - length: 256, - }, - true, - ['encrypt', 'decrypt'], - ); - const { getByLabelText, getByRole } = render( - - ) - , - ); - - const file = await getFixtureFile('doc_auth_images/id-back.jpg'); - await userEvent.click(getByRole('button', { name: 'idv.failure.button.warning' })); - - await Promise.all([ - new Promise((resolve) => onChange.callsFake(resolve)), - userEvent.upload(getByLabelText('doc_auth.headings.document_capture_back'), file), - ]); - const patch = onChange.getCall(0).args[0]; - expect(await patch.back_image_url).to.equal('about:blank#back'); - expect(window.fetch.getCall(0).args[0]).to.equal('about:blank#back'); - }); - it('renders troubleshooting options', async () => { const { getByRole } = render( { 'isMockClient', 'statusPollInterval', 'getStatus', - 'backgroundUploadURLs', - 'backgroundUploadEncryptKey', 'flowPath', 'formData', ]); @@ -27,8 +25,6 @@ describe('document-capture/context/upload', () => { expect(result.current.getStatus).to.be.instanceOf(Function); expect(result.current.statusPollInterval).to.be.undefined(); expect(result.current.isMockClient).to.be.false(); - expect(result.current.backgroundUploadURLs).to.deep.equal({}); - expect(result.current.backgroundUploadEncryptKey).to.be.undefined(); expect(await result.current.getStatus()).to.deep.equal({}); }); @@ -76,37 +72,6 @@ describe('document-capture/context/upload', () => { expect(result.current.statusPollInterval).to.equal(1000); }); - it('can be overridden with background upload URLs', () => { - const backgroundUploadURLs = { foo: '/' }; - const { result } = renderHook(() => useContext(UploadContext), { - wrapper: ({ children }) => ( - - {children} - - ), - }); - - expect(result.current.backgroundUploadURLs).to.deep.equal(backgroundUploadURLs); - }); - - it('can be overridden with background upload encrypt key', async () => { - const key = await window.crypto.subtle.generateKey( - { - name: 'AES-GCM', - length: 256, - }, - true, - ['encrypt', 'decrypt'], - ); - const { result } = renderHook(() => useContext(UploadContext), { - wrapper: ({ children }) => ( - {children} - ), - }); - - expect(result.current.backgroundUploadEncryptKey).to.equal(key); - }); - it('provides endpoint to custom uploader', async () => { const { result } = renderHook(() => useContext(UploadContext), { wrapper: ({ children }) => ( diff --git a/spec/javascript/packages/document-capture/higher-order/with-background-encrypted-upload-spec.jsx b/spec/javascript/packages/document-capture/higher-order/with-background-encrypted-upload-spec.jsx deleted file mode 100644 index b78ad4f0e4c..00000000000 --- a/spec/javascript/packages/document-capture/higher-order/with-background-encrypted-upload-spec.jsx +++ /dev/null @@ -1,289 +0,0 @@ -import { useEffect } from 'react'; -import sinon from 'sinon'; -import { UploadContextProvider, AnalyticsContext } from '@18f/identity-document-capture'; -import withBackgroundEncryptedUpload, { - BackgroundEncryptedUploadError, - blobToArrayBuffer, - encrypt, -} from '@18f/identity-document-capture/higher-order/with-background-encrypted-upload'; -import { useSandbox } from '@18f/identity-test-helpers'; -import * as analytics from '@18f/identity-analytics'; -import { render } from '../../../support/document-capture'; - -/** - * @param {ArrayBuffer} a - * @param {ArrayBuffer} b - * - * @return {boolean} - */ -function isArrayBufferEqual(a, b) { - if (a.byteLength !== b.byteLength) { - return false; - } - - const aView = new DataView(a); - const bView = new DataView(b); - for (let i = 0; i < a.byteLength; i++) { - if (aView.getUint8(i) !== bView.getUint8(i)) { - return false; - } - } - - return true; -} - -describe('document-capture/higher-order/with-background-encrypted-upload', () => { - const sandbox = useSandbox(); - - describe('blobToArrayBuffer', () => { - it('converts blob to data view', async () => { - const data = new window.File(['Hello world'], 'demo.text', { type: 'text/plain' }); - const expected = new Uint8Array([72, 101, 108, 108, 111, 32, 119, 111, 114, 108, 100]).buffer; - - const actual = await blobToArrayBuffer(data); - expect(isArrayBufferEqual(actual, expected)).to.be.true(); - }); - - it('rejects on filereader error', async () => { - const error = new Error(); - sandbox.stub(window.FileReader.prototype, 'readAsArrayBuffer').callsFake(function () { - Object.defineProperty(this, 'error', { value: error }); - this.onerror(new window.Event('error')); - }); - - try { - await blobToArrayBuffer( - new window.File(['Hello world'], 'demo.text', { type: 'text/plain' }), - ); - } catch (actualError) { - expect(actualError).to.equal(error); - } - }); - }); - - describe('withBackgroundEncryptedUpload', () => { - function OriginalComponent({ onChange, onError, errorOnMount }) { - useEffect(() => { - onChange({ foo: 'bar', baz: 'quux' }); - }, []); - - useEffect(() => { - if (errorOnMount) { - onError(new Error()); - } - }, [errorOnMount]); - - return null; - } - const Component = withBackgroundEncryptedUpload(OriginalComponent); - - describe('encrypt', () => { - it('resolves to AES-GCM encrypted data from string value', async () => { - const key = await window.crypto.subtle.importKey( - 'raw', - new Uint8Array(32).buffer, - 'AES-GCM', - false, - ['encrypt', 'decrypt'], - ); - const iv = new Uint8Array(12); - const data = 'Hello world'; - const expected = new Uint8Array( - '134,194,44,81,34,64,28,1,117,34,161,11,192,7,169,19,140,29,89,104,50,208,250,152,208,214,65'.split( - ',', - ), - ).buffer; - - const encrypted = await encrypt(key, iv, data); - expect(isArrayBufferEqual(encrypted, expected)).to.be.true(); - }); - - it('resolves to AES-GCM encrypted data from blob', async () => { - const key = await window.crypto.subtle.importKey( - 'raw', - new Uint8Array(32).buffer, - 'AES-GCM', - false, - ['encrypt', 'decrypt'], - ); - const iv = new Uint8Array(12); - const data = new window.File(['Hello world'], 'demo.text', { type: 'text/plain' }); - const expected = new Uint8Array( - '134,194,44,81,34,64,28,1,117,34,161,11,192,7,169,19,140,29,89,104,50,208,250,152,208,214,65'.split( - ',', - ), - ).buffer; - - const encrypted = await encrypt(key, iv, data); - expect(isArrayBufferEqual(encrypted, expected)).to.be.true(); - }); - }); - - it('passes through original onError', () => { - const onError = sinon.spy(); - render( {}} onError={onError} errorOnMount />); - - expect(onError).to.have.been.calledOnce(); - }); - - it('maintains and decorates the original component display name', () => { - expect(Component.displayName).to.equal('WithBackgroundEncryptedUpload(OriginalComponent)'); - }); - - describe('upload', () => { - async function renderWithResponse(response) { - const trackEvent = sinon.spy(); - const onChange = sinon.spy(); - const onError = sinon.spy(); - const key = await window.crypto.subtle.generateKey( - { - name: 'AES-GCM', - length: 256, - }, - true, - ['encrypt', 'decrypt'], - ); - sandbox.stub(window, 'fetch').callsFake(() => Promise.resolve(response)); - render( - - - - - , - ); - - return { onChange, onError, trackEvent }; - } - - context('success', () => { - /** @type {Response} */ - const response = { - ok: true, - status: 200, - headers: new window.Headers(), - }; - - it('intercepts onChange to include background uploads', async () => { - const { onChange } = await renderWithResponse(response); - - expect(onChange.calledOnce).to.be.true(); - const patch = onChange.getCall(0).args[0]; - expect(patch).to.have.keys(['foo', 'baz', 'foo_image_iv', 'foo_image_url']); - expect(patch.foo).to.equal('bar'); - expect(patch.baz).to.equal('quux'); - expect(patch.foo_image_url).to.be.an.instanceOf(Promise); - expect(await patch.foo_image_url).to.equal('about:blank'); - const [url, params] = window.fetch.getCall(0).args; - expect(url).to.equal('about:blank'); - expect(params.method).to.equal('PUT'); - expect(params.body).to.be.instanceOf(ArrayBuffer); - const bodyAsString = String.fromCharCode.apply(null, new Uint8Array(params.body)); - expect(bodyAsString).to.not.equal('bar'); - }); - - it('logs result', async () => { - const { onChange, trackEvent } = await renderWithResponse(response); - - await onChange.getCall(0).args[0].foo_image_url; - expect(trackEvent).to.have.been.calledTwice(); - expect(trackEvent).to.have.been.calledWith( - 'IdV: document capture async upload encryption', - { success: true }, - ); - expect(trackEvent).to.have.been.calledWith( - 'IdV: document capture async upload submitted', - { success: true, trace_id: null, status_code: 200 }, - ); - }); - }); - - context('failure', () => { - /** @type {Response} */ - const response = { - ok: false, - status: 403, - headers: new window.Headers({ - 'X-Amzn-Trace-Id': '1-67891233-abcdef012345678912345678', - }), - text: Promise.resolve( - '\n' + - '' + - 'InvalidAccessKeyId' + - 'The AWS Access Key Id you provided does not exist in our records.' + - '...' + - '...' + - '...' + - '', - ), - }; - - it('throws on failed background upload', async () => { - const { onChange } = await renderWithResponse(response); - - expect(onChange.calledOnce).to.be.true(); - const patch = onChange.getCall(0).args[0]; - expect(patch).to.have.keys(['foo', 'baz', 'foo_image_iv', 'foo_image_url']); - expect(patch.foo).to.equal('bar'); - expect(patch.baz).to.equal('quux'); - expect(patch.foo_image_url).to.be.an.instanceOf(Promise); - await patch.foo_image_url.catch((error) => { - expect(error).to.be.instanceOf(BackgroundEncryptedUploadError); - }); - }); - - it('logs and throws on failed encryption', async () => { - const error = new Error(); - sandbox.stub(window.crypto.subtle, 'encrypt').throws(error); - sandbox.spy(analytics, 'trackError'); - const { onChange, onError, trackEvent } = await renderWithResponse(response); - - const patch = onChange.getCall(0).args[0]; - await patch.foo_image_url.catch(() => {}); - expect(onError).to.have.been.calledOnceWith( - sinon.match.instanceOf(BackgroundEncryptedUploadError), - { field: 'foo' }, - ); - expect(trackEvent).to.have.been.calledWith( - 'IdV: document capture async upload encryption', - { success: false }, - ); - expect(analytics.trackError).to.have.been.calledWith(error); - expect(window.fetch).not.to.have.been.called(); - }); - - it('calls onError', async () => { - const { onChange, onError } = await renderWithResponse(response); - - const patch = onChange.getCall(0).args[0]; - await patch.foo_image_url.catch(() => {}); - expect(onError).to.have.been.calledOnceWith( - sinon.match.instanceOf(BackgroundEncryptedUploadError), - { field: 'foo' }, - ); - }); - - it('logs result', async () => { - const { onChange, trackEvent } = await renderWithResponse(response); - - await onChange.getCall(0).args[0].foo_image_url.catch(() => {}); - expect(trackEvent).to.have.been.calledTwice(); - expect(trackEvent).to.have.been.calledWith( - 'IdV: document capture async upload encryption', - { success: true }, - ); - expect(trackEvent).to.have.been.calledWith( - 'IdV: document capture async upload submitted', - { - success: false, - trace_id: '1-67891233-abcdef012345678912345678', - status_code: 403, - }, - ); - }); - }); - }); - }); -}); From ff834ffc854c20a961807b7d4de2098811cca43c Mon Sep 17 00:00:00 2001 From: Andrew Duthie Date: Fri, 26 May 2023 13:20:00 -0400 Subject: [PATCH 17/20] LG-9860: Fix platform authenticator reauthentication redirect (#8490) * LG-9860: Fix platform authenticator reauthentication redirect changelog: Upcoming Features, Platform Authenticator, Fix redirect for reauthentication during platform authenticator setup * Omit platform parameter if false --- app/views/users/webauthn_setup/new.html.erb | 2 +- spec/features/webauthn/management_spec.rb | 11 +++++++++++ 2 files changed, 12 insertions(+), 1 deletion(-) diff --git a/app/views/users/webauthn_setup/new.html.erb b/app/views/users/webauthn_setup/new.html.erb index b1e78fd3c74..716ca4a870b 100644 --- a/app/views/users/webauthn_setup/new.html.erb +++ b/app/views/users/webauthn_setup/new.html.erb @@ -14,7 +14,7 @@ <%= simple_form_for( '', - url: webauthn_setup_path, + url: webauthn_setup_path(platform: @platform_authenticator.presence), method: :patch, html: { class: 'margin-top-4 margin-bottom-1', diff --git a/spec/features/webauthn/management_spec.rb b/spec/features/webauthn/management_spec.rb index 7436e257b1a..6b4fb55e552 100644 --- a/spec/features/webauthn/management_spec.rb +++ b/spec/features/webauthn/management_spec.rb @@ -169,6 +169,17 @@ def expect_webauthn_platform_setup_error expect(page).to have_current_path webauthn_setup_path(platform: true) + # Regression: LG-9860: Ensure that the platform URL parameter is maintained through reauthn + travel_to (IdentityConfig.store.reauthn_window + 1).seconds.from_now + fill_in_nickname_and_click_continue + mock_press_button_on_hardware_key_on_setup + + expect(page).to have_current_path login_two_factor_options_path(reauthn: true) + click_on t('forms.buttons.continue') + fill_in_code_with_last_phone_otp + click_submit_default + + expect(page).to have_current_path webauthn_setup_path(platform: true) fill_in_nickname_and_click_continue mock_press_button_on_hardware_key_on_setup From b121550fa7a3d9096ddbed34a8c13f77fe3649b0 Mon Sep 17 00:00:00 2001 From: Jessica Dembe Date: Fri, 26 May 2023 14:42:53 -0400 Subject: [PATCH 18/20] LG-9423: Radio buttons are not read correctly by assistive technology when reading with the arrow keys (#8487) * can I add lang as attribute on label? * add `lang` attribute to inputs * add lang attribute for span again * changelog: Improvements, accessibility, input not reading in correct language --- app/views/shared/_email_languages.html.erb | 1 + 1 file changed, 1 insertion(+) diff --git a/app/views/shared/_email_languages.html.erb b/app/views/shared/_email_languages.html.erb index 16ae3dca9ad..0f7cfb77774 100644 --- a/app/views/shared/_email_languages.html.erb +++ b/app/views/shared/_email_languages.html.erb @@ -29,6 +29,7 @@ locals: ), locale, checked: selection ? selection.to_s == locale.to_s : I18n.locale.to_s == locale.to_s, + lang: locale, ] end, ) %> From 36507eb63c8036d11265cc20a2cb992e898e3dbf Mon Sep 17 00:00:00 2001 From: Andrew Duthie Date: Tue, 30 May 2023 08:59:19 -0400 Subject: [PATCH 19/20] Cherry-pick design system form control styles (#8497) * Cherry-pick design system form control styles changelog: Internal, Performance, Reduce size of application stylesheets in critical path * Add custom LGDS form controls --- app/assets/stylesheets/_uswds-form-controls.scss | 13 +++++++++++++ 1 file changed, 13 insertions(+) create mode 100644 app/assets/stylesheets/_uswds-form-controls.scss diff --git a/app/assets/stylesheets/_uswds-form-controls.scss b/app/assets/stylesheets/_uswds-form-controls.scss new file mode 100644 index 00000000000..bcb8825cf56 --- /dev/null +++ b/app/assets/stylesheets/_uswds-form-controls.scss @@ -0,0 +1,13 @@ +@forward 'usa-checkbox'; +@forward 'usa-error-message'; +@forward 'usa-fieldset'; +@forward 'usa-file-input'; +@forward 'usa-form-group'; +@forward 'usa-hint'; +@forward 'usa-input'; +@forward 'usa-label'; +@forward 'usa-legend'; +@forward 'usa-memorable-date'; +@forward 'usa-radio'; +@forward 'usa-select'; +@forward 'usa-success-message'; From a43b3d7292d3ecf6c60a1bac243629e80a297bb5 Mon Sep 17 00:00:00 2001 From: Andrew Duthie Date: Tue, 30 May 2023 09:10:44 -0400 Subject: [PATCH 20/20] Improve one-time code pattern enforcement, prefix handling (#8495) * Improve one-time code pattern enforcement, prefix handling changelog: User-Facing Improvements, One-Time Code, Improve pattern recognition for one-time code fields * Update one_time_code_input_component_preview.rb * Remove code_length from OTP field render * Avoid escaping Regexp outside JS syntax characters Browsers may log an error otherwise * Re-add uppercase letter support for non-numeric one-time code * Update one_time_code_input_component_spec.rb * Update spec to satisfy code format in wrong code Previously, the form would allow this submission, but now that we're more strict about the format, it'd be caught on presubmission form validation --- .../one_time_code_input_component.html.erb | 2 +- .../one_time_code_input_component.rb | 37 ++++++++-- app/views/idv/otp_verification/show.html.erb | 3 +- .../otp_verification/show.html.erb | 7 +- .../totp_verification/show.html.erb | 2 +- app/views/users/totp_setup/new.html.erb | 2 +- .../one_time_code_input_component_spec.rb | 72 +++++++++++++++---- .../one_time_code_input_component_preview.rb | 9 ++- .../idv/phone_otp_rate_limiting_spec.rb | 2 +- .../two_factor_authentication/sign_in_spec.rb | 33 +++++++-- 10 files changed, 139 insertions(+), 30 deletions(-) diff --git a/app/components/one_time_code_input_component.html.erb b/app/components/one_time_code_input_component.html.erb index 53174f4bf7a..96fd23da093 100644 --- a/app/components/one_time_code_input_component.html.erb +++ b/app/components/one_time_code_input_component.html.erb @@ -10,7 +10,7 @@ input_html: { **field_options[:input_html].to_h, value:, - maxlength:, + maxlength: input_maxlength, autofocus: autofocus?, class: input_css_class, pattern: input_pattern, diff --git a/app/components/one_time_code_input_component.rb b/app/components/one_time_code_input_component.rb index 6f041f598c5..cc20e173609 100644 --- a/app/components/one_time_code_input_component.rb +++ b/app/components/one_time_code_input_component.rb @@ -2,7 +2,8 @@ class OneTimeCodeInputComponent < BaseComponent attr_reader :form, :name, :value, - :maxlength, + :code_length, + :optional_prefix, :autofocus, :transport, :numeric, @@ -12,11 +13,15 @@ class OneTimeCodeInputComponent < BaseComponent alias_method :numeric?, :numeric alias_method :autofocus?, :autofocus + # @see https://tc39.es/ecma262/#prod-SyntaxCharacter + JS_REGEXP_SYNTAX_CHARACTER = Regexp.union(%w[^ $ \ . * + ? ( ) [ ] { } |]) + # @param [FormBuilder] form Form builder instance. # @param [Symbol] name Field name. Defaults to `:code`. # @param [String] value Field value. Defaults to empty. - # @param [Integer] maxlength Sets maxlength for the field. Defaults to + # @param [Integer] code_length Expected code length. Defaults to # TwoFactorAuthenticatable::DIRECT_OTP_LENGTH + # @param [String] optional_prefix Optional prefix to allow before code # @param [Boolean] autofocus Whether the input should be focused on page load. Defaults to # `false`. # @param [String] transport WebOTP transport method. Defaults to 'sms'. @@ -27,7 +32,8 @@ def initialize( form:, name: :code, value: nil, - maxlength: TwoFactorAuthenticatable::DIRECT_OTP_LENGTH + 1, # to allow for leading '#' + code_length: TwoFactorAuthenticatable::DIRECT_OTP_LENGTH, + optional_prefix: '', autofocus: false, transport: 'sms', numeric: true, @@ -37,7 +43,8 @@ def initialize( @form = form @name = name @value = value - @maxlength = maxlength + @code_length = code_length + @optional_prefix = optional_prefix @autofocus = autofocus @transport = transport @numeric = numeric @@ -53,11 +60,23 @@ def hint end end + def input_maxlength + optional_prefix.size + code_length + end + def input_pattern + "#{input_pattern_prefix}#{input_pattern_character_set}{#{code_length}}" + end + + def input_pattern_prefix + "#{regexp_escape_for_js(optional_prefix)}?" if optional_prefix.present? + end + + def input_pattern_character_set if numeric? - '#?[0-9]*' + '[0-9]' else - '#?[a-zA-Z0-9]*' + '[a-zA-Z0-9]' end end @@ -72,4 +91,10 @@ def input_inputmode def input_css_class [*field_options.dig(:input_html, :class), 'one-time-code-input__input'] end + + def regexp_escape_for_js(string) + # `Regexp.escape` escapes more characters than what is considered "special" for JavaScript + # regular expressions. Browsers may log errors for unexpected escaping of characters. + string.gsub(JS_REGEXP_SYNTAX_CHARACTER) { |c| Regexp.escape(c) } + end end diff --git a/app/views/idv/otp_verification/show.html.erb b/app/views/idv/otp_verification/show.html.erb index c6b91628e17..e5fe067d7fe 100644 --- a/app/views/idv/otp_verification/show.html.erb +++ b/app/views/idv/otp_verification/show.html.erb @@ -21,7 +21,8 @@ value: @code, numeric: false, autofocus: true, - maxlength: TwoFactorAuthenticatable::PROOFING_DIRECT_OTP_LENGTH + 1, + code_length: TwoFactorAuthenticatable::PROOFING_DIRECT_OTP_LENGTH, + optional_prefix: '#', class: 'margin-bottom-5', ) %> <%= f.submit t('forms.buttons.submit.default'), class: 'margin-bottom-5' %> diff --git a/app/views/two_factor_authentication/otp_verification/show.html.erb b/app/views/two_factor_authentication/otp_verification/show.html.erb index c3c2afeca50..b3a5b09f403 100644 --- a/app/views/two_factor_authentication/otp_verification/show.html.erb +++ b/app/views/two_factor_authentication/otp_verification/show.html.erb @@ -25,7 +25,12 @@ <% if @presenter.reauthn %> <%= render 'two_factor_authentication/totp_verification/reauthn' %> <% end %> - <%= render OneTimeCodeInputComponent.new(form: f, autofocus: true, value: @presenter.code_value) %> + <%= render OneTimeCodeInputComponent.new( + form: f, + autofocus: true, + value: @presenter.code_value, + optional_prefix: '#', + ) %> <%= f.input( :remember_device, as: :boolean, diff --git a/app/views/two_factor_authentication/totp_verification/show.html.erb b/app/views/two_factor_authentication/totp_verification/show.html.erb index 0687aff3919..79ec2746cfe 100644 --- a/app/views/two_factor_authentication/totp_verification/show.html.erb +++ b/app/views/two_factor_authentication/totp_verification/show.html.erb @@ -11,7 +11,7 @@ value: @code, autofocus: true, transport: nil, - maxlength: TwoFactorAuthenticatable::OTP_LENGTH, + code_length: TwoFactorAuthenticatable::OTP_LENGTH, ) %> <%= f.input( :remember_device, diff --git a/app/views/users/totp_setup/new.html.erb b/app/views/users/totp_setup/new.html.erb index 9f147e7f2bf..ad656b932cb 100644 --- a/app/views/users/totp_setup/new.html.erb +++ b/app/views/users/totp_setup/new.html.erb @@ -40,7 +40,7 @@ <%= render OneTimeCodeInputComponent.new( form: f, transport: nil, - maxlength: TwoFactorAuthenticatable::OTP_LENGTH, + code_length: TwoFactorAuthenticatable::OTP_LENGTH, field_options: { label: false, input_html: { diff --git a/spec/components/one_time_code_input_component_spec.rb b/spec/components/one_time_code_input_component_spec.rb index c210e3b5d3d..d0fd59145b5 100644 --- a/spec/components/one_time_code_input_component_spec.rb +++ b/spec/components/one_time_code_input_component_spec.rb @@ -9,6 +9,10 @@ subject(:rendered) { render_inline OneTimeCodeInputComponent.new(form:, **options) } + before do + stub_const('TwoFactorAuthenticatable::DIRECT_OTP_LENGTH', 6) + end + describe 'name' do context 'no name given' do it 'renders default name "code"' do @@ -30,6 +34,10 @@ it 'renders input mode "numeric"' do expect(rendered).to have_selector('[inputmode="numeric"]') end + + it 'renders input pattern' do + expect(rendered).to have_css('[pattern="[0-9]{6}"]') + end end context 'numeric is false' do @@ -38,6 +46,10 @@ it 'renders input mode "text"' do expect(rendered).to have_selector('[inputmode="text"]') end + + it 'renders input pattern' do + expect(rendered).to have_css('[pattern="[a-zA-Z0-9]{6}"]') + end end end @@ -107,22 +119,58 @@ end end - describe 'maxlength' do - context 'no maxlength given' do - it 'renders input maxlength DIRECT_OTP_LENGTH + 1' do - expect(rendered).to have_selector( - "[maxlength=\"#{TwoFactorAuthenticatable::DIRECT_OTP_LENGTH + 1}\"]", - ) + describe 'code_length' do + context 'without code_length' do + it 'renders input default maxlength' do + expect(rendered).to have_css('[maxlength="6"]') + end + + it 'renders input pattern' do + expect(rendered).to have_css('[pattern="[0-9]{6}"]') end end - context 'maxlength given' do - let(:options) { { maxlength: 10 } } + context 'with code_length' do + let(:options) { { code_length: 10 } } + + it 'renders input maxlength based on given code_length' do + expect(rendered).to have_css('[maxlength="10"]') + end + + it 'renders input pattern' do + expect(rendered).to have_css('[pattern="[0-9]{10}"]') + end + end + end + + describe 'optional_prefix' do + context 'without optional_prefix' do + it 'renders input default maxlength' do + expect(rendered).to have_css('[maxlength="6"]') + end + + it 'renders input pattern' do + expect(rendered).to have_css('[pattern="[0-9]{6}"]') + end + end + + context 'with optional_prefix given' do + let(:options) { { optional_prefix: '$' } } + + it 'renders input maxlength based on given optional_prefix' do + expect(rendered).to have_css('[maxlength="7"]') + end + + it 'renders input pattern' do + expect(rendered).to have_css('[pattern="\\\$?[0-9]{6}"]') + end + + context 'with prefix which would be escaped in ruby but unescaped in javascript' do + let(:options) { { optional_prefix: '#' } } - it 'renders input given maxlength' do - expect(rendered).to have_selector( - '[maxlength="10"]', - ) + it 'renders input pattern without escaping' do + expect(rendered).to have_css('[pattern="#?[0-9]{6}"]') + end end end end diff --git a/spec/components/previews/one_time_code_input_component_preview.rb b/spec/components/previews/one_time_code_input_component_preview.rb index 123ff909e7f..f9ba52cc166 100644 --- a/spec/components/previews/one_time_code_input_component_preview.rb +++ b/spec/components/previews/one_time_code_input_component_preview.rb @@ -8,11 +8,16 @@ def numeric def alphanumeric render(OneTimeCodeInputComponent.new(form: form_builder, numeric: false)) end + + def with_optional_prefix + render(OneTimeCodeInputComponent.new(form: form_builder, numeric: false, optional_prefix: '#')) + end # @!endgroup # @display form true # @param numeric toggle - def workbench(numeric: true) - render(OneTimeCodeInputComponent.new(form: form_builder, numeric:)) + # @param optional_prefix text + def workbench(numeric: true, optional_prefix: '') + render(OneTimeCodeInputComponent.new(form: form_builder, numeric:, optional_prefix:)) end end diff --git a/spec/features/idv/phone_otp_rate_limiting_spec.rb b/spec/features/idv/phone_otp_rate_limiting_spec.rb index 30ccda4c15e..315e856f1fd 100644 --- a/spec/features/idv/phone_otp_rate_limiting_spec.rb +++ b/spec/features/idv/phone_otp_rate_limiting_spec.rb @@ -37,7 +37,7 @@ complete_idv_steps_before_phone_otp_verification_step(user) max_attempts.times do - fill_in('code', with: 'wrong') + fill_in('code', with: 'badbad') click_button t('forms.buttons.submit.default') end diff --git a/spec/features/two_factor_authentication/sign_in_spec.rb b/spec/features/two_factor_authentication/sign_in_spec.rb index e79bf300f4e..49b4aaa76f0 100644 --- a/spec/features/two_factor_authentication/sign_in_spec.rb +++ b/spec/features/two_factor_authentication/sign_in_spec.rb @@ -261,17 +261,42 @@ def attempt_to_bypass_2fa user = create(:user, :fully_registered) sign_in_before_2fa(user) - expect(page.evaluate_script('document.activeElement.id')).to start_with('code') + input = page.find_field(t('components.one_time_code_input.label')) + expect(page.evaluate_script('document.activeElement.id')).to eq(input[:id]) end - scenario 'user enters incorrect OTP', js: true do + it 'validates OTP format', js: true do user = create(:user, :fully_registered) sign_in_before_2fa(user) - expect(page.evaluate_script('document.activeElement.id')).to start_with('code') - fill_in 'code', with: 'BADBAD' + input = page.find_field(t('components.one_time_code_input.label')) + + # Invalid: Unsupported characters + input.fill_in with: 'BADBAD' + click_submit_default + expect(page).to have_content(t('errors.messages.otp_format')) + + # Invalid: Not enough characters, with prefix + fill_in t('components.one_time_code_input.label'), with: '#12345' click_submit_default expect(page).to have_content(t('errors.messages.otp_format')) + + # Invalid: Not enough characters, without prefix + fill_in t('components.one_time_code_input.label'), with: '12345' + click_submit_default + expect(page).to have_content(t('errors.messages.otp_format')) + + # Valid: Enough characters, with prefix + input.fill_in with: '#123456' + expect(input.value).to eq('#123456') + page.evaluate_script('document.activeElement.closest("form").reportValidity()') + expect(page).not_to have_content(t('errors.messages.otp_format')) + + # Valid: Enough characters, without prefix + input.fill_in with: '123456' + expect(input.value).to eq('123456') + page.evaluate_script('document.activeElement.closest("form").reportValidity()') + expect(page).not_to have_content(t('errors.messages.otp_format')) end scenario 'the user changes delivery method' do