From 7f2f7508c8b853c5f9792f4a24e0f7b9493c298f Mon Sep 17 00:00:00 2001 From: Jonathan Hooper Date: Tue, 19 Dec 2023 15:25:43 -0500 Subject: [PATCH 01/19] Revert "Revert "LG-11697 Store whether a biometric comparison is required in the SP session (#9759)" (#9804)" (#9806) This reverts commit 558ebd6b85d971d053f1a56450b9c1720c8158ed. The reverted commit here was reverting the changes in #9759. That change had issues with compatibility which were addressed in the changes in the reverted commit and deployed. The deployment of those changes makes this commit safe to merge. [skip changelog] --- app/forms/openid_connect_authorize_form.rb | 15 ++++++- app/models/federated_protocols/oidc.rb | 4 ++ app/models/federated_protocols/saml.rb | 4 ++ app/models/service_provider_request.rb | 6 ++- .../service_provider_request_handler.rb | 1 + .../service_provider_request_proxy.rb | 8 +++- app/services/store_sp_metadata_in_session.rb | 1 + .../authorization_controller_spec.rb | 9 +++++ spec/controllers/saml_idp_controller_spec.rb | 2 + .../openid_connect_authorize_form_spec.rb | 2 + .../store_sp_metadata_in_session_spec.rb | 39 +++++++++++++++++++ 11 files changed, 86 insertions(+), 5 deletions(-) diff --git a/app/forms/openid_connect_authorize_form.rb b/app/forms/openid_connect_authorize_form.rb index 6906ef1e049..9000ec7c518 100644 --- a/app/forms/openid_connect_authorize_form.rb +++ b/app/forms/openid_connect_authorize_form.rb @@ -17,7 +17,15 @@ class OpenidConnectAuthorizeForm state ].freeze - ATTRS = [:unauthorized_scope, :acr_values, :scope, :verified_within, *SIMPLE_ATTRS].freeze + ATTRS = [ + :unauthorized_scope, + :acr_values, + :scope, + :verified_within, + :biometric_comparison_required, + *SIMPLE_ATTRS, + ].freeze + AALS_BY_PRIORITY = [Saml::Idp::Constants::AAL2_HSPD12_AUTHN_CONTEXT_CLASSREF, Saml::Idp::Constants::AAL3_HSPD12_AUTHN_CONTEXT_CLASSREF, Saml::Idp::Constants::AAL2_PHISHING_RESISTANT_AUTHN_CONTEXT_CLASSREF, @@ -55,6 +63,7 @@ def initialize(params) @prompt ||= 'select_account' @scope = parse_to_values(params[:scope], scopes) @unauthorized_scope = check_for_unauthorized_scope(params) + @biometric_comparison_required = params[:biometric_comparison_required].to_s == 'true' if verified_within_allowed? @duration_parser = DurationParser.new(params[:verified_within]) @@ -130,6 +139,10 @@ def requested_aal_value :ial2_or_greater?, :ial2_requested? + def biometric_comparison_required? + @biometric_comparison_required + end + private attr_reader :identity, :success diff --git a/app/models/federated_protocols/oidc.rb b/app/models/federated_protocols/oidc.rb index 74b82e1697b..33b92251cf3 100644 --- a/app/models/federated_protocols/oidc.rb +++ b/app/models/federated_protocols/oidc.rb @@ -20,6 +20,10 @@ def requested_attributes OpenidConnectAttributeScoper.new(request.scope).requested_attributes end + def biometric_comparison_required? + request.biometric_comparison_required? + end + def service_provider request.service_provider end diff --git a/app/models/federated_protocols/saml.rb b/app/models/federated_protocols/saml.rb index 0840edfb97c..ecc0dea6569 100644 --- a/app/models/federated_protocols/saml.rb +++ b/app/models/federated_protocols/saml.rb @@ -26,6 +26,10 @@ def service_provider current_service_provider end + def biometric_comparison_required? + false + end + private attr_reader :request diff --git a/app/models/service_provider_request.rb b/app/models/service_provider_request.rb index ba4fe894ff1..06f39863ddf 100644 --- a/app/models/service_provider_request.rb +++ b/app/models/service_provider_request.rb @@ -2,7 +2,8 @@ class ServiceProviderRequest # WARNING - Modification of these params requires particular care # since these objects are serialized to/from Redis and may be present # upon deployment - attr_accessor :uuid, :issuer, :url, :ial, :aal, :requested_attributes + attr_accessor :uuid, :issuer, :url, :ial, :aal, :requested_attributes, + :biometric_comparison_required def initialize( uuid: nil, @@ -11,7 +12,7 @@ def initialize( ial: nil, aal: nil, requested_attributes: [], - biometric_comparison_required: false # rubocop:disable Lint/UnusedMethodArgument + biometric_comparison_required: false ) @uuid = uuid @issuer = issuer @@ -19,6 +20,7 @@ def initialize( @ial = ial @aal = aal @requested_attributes = requested_attributes&.map(&:to_s) + @biometric_comparison_required = biometric_comparison_required end def ==(other) diff --git a/app/services/service_provider_request_handler.rb b/app/services/service_provider_request_handler.rb index d23aecbf837..089293b8f77 100644 --- a/app/services/service_provider_request_handler.rb +++ b/app/services/service_provider_request_handler.rb @@ -64,6 +64,7 @@ def attributes ial: protocol.ial, aal: protocol.aal, requested_attributes: protocol.requested_attributes, + biometric_comparison_required: protocol.biometric_comparison_required?, uuid: request_id, url: url, } diff --git a/app/services/service_provider_request_proxy.rb b/app/services/service_provider_request_proxy.rb index 0840f2ae695..d39e615c085 100644 --- a/app/services/service_provider_request_proxy.rb +++ b/app/services/service_provider_request_proxy.rb @@ -33,7 +33,8 @@ def self.find_or_create_by(uuid:) return obj if obj spr = ServiceProviderRequest.new( uuid: uuid, issuer: nil, url: nil, ial: nil, - aal: nil, requested_attributes: nil + aal: nil, requested_attributes: nil, + biometric_comparison_required: false ) yield(spr) create( @@ -43,12 +44,15 @@ def self.find_or_create_by(uuid:) ial: spr.ial, aal: spr.aal, requested_attributes: spr.requested_attributes, + biometric_comparison_required: spr.biometric_comparison_required, ) end def self.create(hash) uuid = hash[:uuid] - obj = hash.slice(:issuer, :url, :ial, :aal, :requested_attributes) + obj = hash.slice( + :issuer, :url, :ial, :aal, :requested_attributes, :biometric_comparison_required + ) write(obj, uuid) hash_to_spr(obj, uuid) end diff --git a/app/services/store_sp_metadata_in_session.rb b/app/services/store_sp_metadata_in_session.rb index fc44045b9df..13c052b2646 100644 --- a/app/services/store_sp_metadata_in_session.rb +++ b/app/services/store_sp_metadata_in_session.rb @@ -36,6 +36,7 @@ def update_session request_url: sp_request.url, request_id: sp_request.uuid, requested_attributes: sp_request.requested_attributes, + biometric_comparison_required: sp_request.biometric_comparison_required, } end diff --git a/spec/controllers/openid_connect/authorization_controller_spec.rb b/spec/controllers/openid_connect/authorization_controller_spec.rb index aab1adbc70d..6b362094969 100644 --- a/spec/controllers/openid_connect/authorization_controller_spec.rb +++ b/spec/controllers/openid_connect/authorization_controller_spec.rb @@ -995,8 +995,17 @@ request_id: sp_request_id, request_url: request.original_url, requested_attributes: %w[], + biometric_comparison_required: false, ) end + + it 'sets biometric_comparison_required to true if biometric comparison is required' do + params[:biometric_comparison_required] = true + + action + + expect(session[:sp][:biometric_comparison_required]).to eq(true) + end end end end diff --git a/spec/controllers/saml_idp_controller_spec.rb b/spec/controllers/saml_idp_controller_spec.rb index 58abedb78b7..2a29333ab46 100644 --- a/spec/controllers/saml_idp_controller_spec.rb +++ b/spec/controllers/saml_idp_controller_spec.rb @@ -1127,6 +1127,7 @@ def name_id_version(format_urn) request_url: @stored_request_url.gsub('authpost', 'auth'), request_id: sp_request_id, requested_attributes: ['email'], + biometric_comparison_required: false, ) end @@ -1158,6 +1159,7 @@ def name_id_version(format_urn) request_url: @saml_request.request.original_url.gsub('authpost', 'auth'), request_id: sp_request_id, requested_attributes: ['email'], + biometric_comparison_required: false, ) end diff --git a/spec/forms/openid_connect_authorize_form_spec.rb b/spec/forms/openid_connect_authorize_form_spec.rb index d986634faa4..fafe7188723 100644 --- a/spec/forms/openid_connect_authorize_form_spec.rb +++ b/spec/forms/openid_connect_authorize_form_spec.rb @@ -14,6 +14,7 @@ code_challenge: code_challenge, code_challenge_method: code_challenge_method, verified_within: verified_within, + biometric_comparison_required: biometric_comparison_required, ) end @@ -33,6 +34,7 @@ let(:code_challenge) { nil } let(:code_challenge_method) { nil } let(:verified_within) { nil } + let(:biometric_comparison_required) { nil } describe '#submit' do subject(:result) { form.submit } diff --git a/spec/services/store_sp_metadata_in_session_spec.rb b/spec/services/store_sp_metadata_in_session_spec.rb index 773b7e2a881..6504e43d929 100644 --- a/spec/services/store_sp_metadata_in_session_spec.rb +++ b/spec/services/store_sp_metadata_in_session_spec.rb @@ -20,6 +20,7 @@ sp_request.ial = Saml::Idp::Constants::IAL1_AUTHN_CONTEXT_CLASSREF sp_request.url = 'http://issuer.gov' sp_request.requested_attributes = %w[email] + sp_request.biometric_comparison_required = false end instance = StoreSpMetadataInSession.new(session: app_session, request_id: request_id) @@ -34,6 +35,7 @@ request_url: 'http://issuer.gov', request_id: request_id, requested_attributes: %w[email], + biometric_comparison_required: false, } instance.call @@ -51,6 +53,7 @@ sp_request.aal = Saml::Idp::Constants::AAL3_AUTHN_CONTEXT_CLASSREF sp_request.url = 'http://issuer.gov' sp_request.requested_attributes = %w[email] + sp_request.biometric_comparison_required = false end instance = StoreSpMetadataInSession.new(session: app_session, request_id: request_id) @@ -65,6 +68,7 @@ request_url: 'http://issuer.gov', request_id: request_id, requested_attributes: %w[email], + biometric_comparison_required: false, } instance.call @@ -82,6 +86,7 @@ sp_request.aal = Saml::Idp::Constants::AAL2_PHISHING_RESISTANT_AUTHN_CONTEXT_CLASSREF sp_request.url = 'http://issuer.gov' sp_request.requested_attributes = %w[email] + sp_request.biometric_comparison_required = false end instance = StoreSpMetadataInSession.new(session: app_session, request_id: request_id) @@ -96,6 +101,40 @@ request_url: 'http://issuer.gov', request_id: request_id, requested_attributes: %w[email], + biometric_comparison_required: false, + } + + instance.call + expect(app_session[:sp]).to eq app_session_hash + end + end + + context 'when biometric comparison is requested' do + it 'sets the session[:sp] hash' do + app_session = {} + request_id = SecureRandom.uuid + ServiceProviderRequestProxy.find_or_create_by(uuid: request_id) do |sp_request| + sp_request.issuer = 'issuer' + sp_request.ial = Saml::Idp::Constants::IAL2_AUTHN_CONTEXT_CLASSREF + sp_request.aal = Saml::Idp::Constants::AAL3_AUTHN_CONTEXT_CLASSREF + sp_request.url = 'http://issuer.gov' + sp_request.requested_attributes = %w[email] + sp_request.biometric_comparison_required = true + end + instance = StoreSpMetadataInSession.new(session: app_session, request_id: request_id) + + app_session_hash = { + issuer: 'issuer', + aal_level_requested: 3, + piv_cac_requested: false, + phishing_resistant_requested: true, + ial: 2, + ial2: true, + ialmax: false, + request_url: 'http://issuer.gov', + request_id: request_id, + requested_attributes: %w[email], + biometric_comparison_required: true, } instance.call From 414ec3d23575654b2d90b92a0d8954a088362f0a Mon Sep 17 00:00:00 2001 From: Andrew Duthie <1779930+aduth@users.noreply.github.com> Date: Tue, 19 Dec 2023 15:35:25 -0500 Subject: [PATCH 02/19] Fix and test for mismatched locale content (#9801) changelog: Bug Fixes, Identity Verification, Fix incorrect information collection disclosure content for French and Spanish --- config/locales/idv/es.yml | 16 ++++++++-------- config/locales/idv/fr.yml | 24 ++++++++++++------------ spec/i18n_spec.rb | 31 ++++++++++++++++++++++++++----- 3 files changed, 46 insertions(+), 25 deletions(-) diff --git a/config/locales/idv/es.yml b/config/locales/idv/es.yml index 1e59b23e67c..9ef6880606f 100644 --- a/config/locales/idv/es.yml +++ b/config/locales/idv/es.yml @@ -222,14 +222,14 @@ es: come_back_later: Carta con una marca de verificación legal_statement: information_collection: >- - Cette collecte d’informations répond aux exigences de l’article 3507 du - 44 U.S.C., tel que modifié par l’article 2 de la loi de 1995 sur la - réduction des tâches administratives. Vous n’avez pas besoin de répondre - à ces questions, sauf si nous affichons un numéro de contrôle valide de - l’Office of Management and Budget (OMB). Le numéro de contrôle de l’OMB - pour cette collecte est 3090-0325. Calculamos que tomará un minuto leer - las instrucciones, recopilar los datos y responder las preguntas. Envíe - solo comentarios relacionados con nuestro tiempo estimado, incluidas + Esta recopilación de información cumple con los requisitos del título 44 + del U.S.C., § 3507, modificado por la sección 2 de la Ley de Reducción + de Trámites de 1995. No es necesario que responda a estas preguntas, a + menos que le mostremos un número de control válido de la Oficina de + Administración y Presupuesto (OMB). El número de control de la OMB para + esta recopilación es 3090-0325. Calculamos que tomará un minuto leer las + instrucciones, recopilar los datos y responder las preguntas. Envíe solo + comentarios relacionados con nuestro tiempo estimado, incluidas sugerencias para reducir esta molestia, o cualquier otro aspecto relacionado con esta recopilación de información a: Administración de Servicios Generales, División de la Secretaría Reguladora (MVCB), a la diff --git a/config/locales/idv/fr.yml b/config/locales/idv/fr.yml index b090d64f9b7..f7bc8a88251 100644 --- a/config/locales/idv/fr.yml +++ b/config/locales/idv/fr.yml @@ -230,18 +230,18 @@ fr: come_back_later: Lettre avec un crochet legal_statement: information_collection: >- - Esta recopilación de información cumple con los requisitos del título 44 - del U.S.C., § 3507, modificado por la sección 2 de la Ley de Reducción - de Trámites de 1995. No es necesario que responda a estas preguntas, a - menos que le mostremos un número de control válido de la Oficina de - Administración y Presupuesto (OMB). El número de control de la OMB para - esta recopilación es 3090-0325. Nous estimons qu’il faut une minute pour - lire les instructions, rassembler les preuves et répondre aux questions. - N’envoyez que des commentaires relatifs à notre estimation du temps, y - compris des suggestions pour réduire cette charge, ou tout autre aspect - de cette collecte d’informations à : General Services Administration, - Regulatory Secretariat Division (MVCB), ATTN : Lois Mandell/IC - 3090-0325, 1800 F Street, NW, Washington, DC 20405. + Cette collecte d’informations répond aux exigences de l’article 3507 du + 44 U.S.C., tel que modifié par l’article 2 de la loi de 1995 sur la + réduction des tâches administratives. Vous n’avez pas besoin de répondre + à ces questions, sauf si nous affichons un numéro de contrôle valide de + l’Office of Management and Budget (OMB). Le numéro de contrôle de l’OMB + pour cette collecte est 3090-0325. Nous estimons qu’il faut une minute + pour lire les instructions, rassembler les preuves et répondre aux + questions. N’envoyez que des commentaires relatifs à notre estimation du + temps, y compris des suggestions pour réduire cette charge, ou tout + autre aspect de cette collecte d’informations à : General Services + Administration, Regulatory Secretariat Division (MVCB), ATTN : Lois + Mandell/IC 3090-0325, 1800 F Street, NW, Washington, DC 20405. messages: activated_html: Votre identité a été vérifiée. Si vous souhaitez modifier votre information vérifiée, veuillez %{link_html}. diff --git a/spec/i18n_spec.rb b/spec/i18n_spec.rb index d11c55a67f1..aea5b92134e 100644 --- a/spec/i18n_spec.rb +++ b/spec/i18n_spec.rb @@ -8,6 +8,15 @@ 'time.formats.event_timestamp_js', ] +# A set of patterns which are expected to only occur within specific locales. This is an imperfect +# solution based on current content, intended to help prevent accidents when adding new translated +# content. If you are having issues with new content, it would be reasonable to remove or modify +# the parts of the pattern which are valid for the content you're adding. +LOCALE_SPECIFIC_CONTENT = { + fr: / [nd]’|à/i, + es: /¿|ó/, +}.freeze + module I18n module Tasks class BaseTask @@ -155,17 +164,19 @@ def allowed_untranslated_key?(locale, key) i18n_file = full_path.sub("#{root_dir}/", '') describe i18n_file do + let(:flattened_yaml_data) { flatten_hash(YAML.load_file(full_path)) } + # Transliteration includes special characters by definition, so it could fail checks below if !full_path.match?(%(/config/locales/transliterate/)) it 'has only lower_snake_case keys' do - keys = flatten_hash(YAML.load_file(full_path)).keys + keys = flattened_yaml_data.keys bad_keys = keys.reject { |key| key =~ /^[a-z0-9_.]+$/ } expect(bad_keys).to be_empty end it 'has only has XML-safe identifiers (keys start with a letter)' do - keys = flatten_hash(YAML.load_file(full_path)).keys + keys = flattened_yaml_data.keys bad_keys = keys.select { |key| key.split('.').any? { |part| part =~ /^[0-9]/ } } @@ -174,7 +185,7 @@ def allowed_untranslated_key?(locale, key) end it 'has correctly-formatted interpolation values' do - bad_keys = flatten_hash(YAML.load_file(full_path)).select do |_key, value| + bad_keys = flattened_yaml_data.select do |_key, value| next unless value.is_a?(String) interpolation_names = value.scan(/%\{([^}]+)\}/).flatten @@ -186,7 +197,7 @@ def allowed_untranslated_key?(locale, key) end it 'does not contain any translations expecting legacy fallback behavior' do - bad_keys = flatten_hash(YAML.load_file(full_path)).select do |_key, value| + bad_keys = flattened_yaml_data.select do |_key, value| value.include?('NOT TRANSLATED YET') end @@ -194,12 +205,22 @@ def allowed_untranslated_key?(locale, key) end it 'does not contain any translations that hardcode APP_NAME' do - bad_keys = flatten_hash(YAML.load_file(full_path)).select do |_key, value| + bad_keys = flattened_yaml_data.select do |_key, value| value.include?(APP_NAME) end expect(bad_keys).to be_empty end + + it 'does not contain content from another language' do + flattened_yaml_data.each do |key, value| + locale = key.split('.', 2).first.to_sym + other_locales = LOCALE_SPECIFIC_CONTENT.keys - [locale] + expect(value).not_to match( + Regexp.union(*LOCALE_SPECIFIC_CONTENT.slice(*other_locales).values), + ) + end + end end end end From 3456305bf77485a0ab18a3e5853bd336c551a465 Mon Sep 17 00:00:00 2001 From: Sonia Connolly Date: Tue, 19 Dec 2023 13:11:24 -0800 Subject: [PATCH 03/19] Move opt in analytics to ab_test_analytics_buckets (#9793) * Add OptInHelper and opt_in_analytics_properties to ab_test_analytics_concern * Remove OptInHelper and opt_in_analytics_properties from controllers using ab_test_analytics_buckets * OtpVerificationController: Replace opt_in_analytics_properties with ab_test_analytics_buckets [skip changelog] --- .../concerns/idv/ab_test_analytics_concern.rb | 2 ++ .../idv/by_mail/request_letter_controller.rb | 2 -- .../idv/in_person/address_controller.rb | 4 +--- .../idv/in_person/ssn_controller.rb | 4 +--- .../idv/in_person/verify_info_controller.rb | 4 +--- .../idv/otp_verification_controller.rb | 3 +-- app/controllers/idv/phone_controller.rb | 2 -- .../idv/ab_test_analytics_concern_spec.rb | 20 ++++++++++++++++++- .../idv/otp_verification_controller_spec.rb | 5 +++++ spec/features/idv/analytics_spec.rb | 10 +++++----- 10 files changed, 35 insertions(+), 21 deletions(-) diff --git a/app/controllers/concerns/idv/ab_test_analytics_concern.rb b/app/controllers/concerns/idv/ab_test_analytics_concern.rb index afbedba2364..c7da1aba1dc 100644 --- a/app/controllers/concerns/idv/ab_test_analytics_concern.rb +++ b/app/controllers/concerns/idv/ab_test_analytics_concern.rb @@ -1,11 +1,13 @@ module Idv module AbTestAnalyticsConcern include AcuantConcern + include OptInHelper def ab_test_analytics_buckets buckets = {} if defined?(idv_session) buckets[:skip_hybrid_handoff] = idv_session&.skip_hybrid_handoff + buckets = buckets.merge(opt_in_analytics_properties) end if defined?(document_capture_session_uuid) diff --git a/app/controllers/idv/by_mail/request_letter_controller.rb b/app/controllers/idv/by_mail/request_letter_controller.rb index e85a38d24ec..f4e59a71ce2 100644 --- a/app/controllers/idv/by_mail/request_letter_controller.rb +++ b/app/controllers/idv/by_mail/request_letter_controller.rb @@ -5,7 +5,6 @@ class RequestLetterController < ApplicationController include IdvStepConcern skip_before_action :confirm_no_pending_gpo_profile include Idv::StepIndicatorConcern - include OptInHelper before_action :confirm_mail_not_rate_limited before_action :confirm_step_allowed @@ -81,7 +80,6 @@ def update_tracking gpo_mail_service.hours_since_first_letter(first_letter_requested_at), phone_step_attempts: gpo_mail_service.phone_step_attempts, **ab_test_analytics_buckets, - **opt_in_analytics_properties, ) irs_attempts_api_tracker.idv_gpo_letter_requested(resend: resend_requested?) create_user_event(:gpo_mail_sent, current_user) diff --git a/app/controllers/idv/in_person/address_controller.rb b/app/controllers/idv/in_person/address_controller.rb index f0d8aaf2442..506b2614246 100644 --- a/app/controllers/idv/in_person/address_controller.rb +++ b/app/controllers/idv/in_person/address_controller.rb @@ -3,7 +3,6 @@ module InPerson class AddressController < ApplicationController include Idv::AvailabilityConcern include IdvStepConcern - include OptInHelper before_action :render_404_if_in_person_residential_address_controller_enabled_not_set before_action :confirm_in_person_state_id_step_complete @@ -76,8 +75,7 @@ def analytics_arguments analytics_id: 'In Person Proofing', irs_reproofing: irs_reproofing?, }.merge(ab_test_analytics_buckets). - merge(extra_analytics_properties). - merge(opt_in_analytics_properties) + merge(extra_analytics_properties) end def redirect_to_next_page diff --git a/app/controllers/idv/in_person/ssn_controller.rb b/app/controllers/idv/in_person/ssn_controller.rb index 3e7804912a9..a4a1e079f55 100644 --- a/app/controllers/idv/in_person/ssn_controller.rb +++ b/app/controllers/idv/in_person/ssn_controller.rb @@ -6,7 +6,6 @@ class SsnController < ApplicationController include StepIndicatorConcern include Steps::ThreatMetrixStepHelper include ThreatMetrixConcern - include OptInHelper before_action :confirm_not_rate_limited_after_doc_auth before_action :confirm_in_person_address_step_complete @@ -97,8 +96,7 @@ def analytics_arguments analytics_id: 'In Person Proofing', irs_reproofing: irs_reproofing?, }.merge(ab_test_analytics_buckets). - merge(**extra_analytics_properties). - merge(**opt_in_analytics_properties) + merge(**extra_analytics_properties) end def confirm_in_person_address_step_complete diff --git a/app/controllers/idv/in_person/verify_info_controller.rb b/app/controllers/idv/in_person/verify_info_controller.rb index b3b8eeed1f3..7e8d05fc4f1 100644 --- a/app/controllers/idv/in_person/verify_info_controller.rb +++ b/app/controllers/idv/in_person/verify_info_controller.rb @@ -6,7 +6,6 @@ class VerifyInfoController < ApplicationController include StepIndicatorConcern include Steps::ThreatMetrixStepHelper include VerifyInfoConcern - include OptInHelper before_action :confirm_not_rate_limited_after_doc_auth, except: [:show] before_action :confirm_ssn_step_complete @@ -88,8 +87,7 @@ def analytics_arguments analytics_id: 'In Person Proofing', irs_reproofing: irs_reproofing?, }.merge(ab_test_analytics_buckets). - merge(**extra_analytics_properties). - merge(**opt_in_analytics_properties) + merge(**extra_analytics_properties) end def confirm_ssn_step_complete diff --git a/app/controllers/idv/otp_verification_controller.rb b/app/controllers/idv/otp_verification_controller.rb index 36dda6a2d66..9274f42206b 100644 --- a/app/controllers/idv/otp_verification_controller.rb +++ b/app/controllers/idv/otp_verification_controller.rb @@ -4,7 +4,6 @@ class OtpVerificationController < ApplicationController include IdvStepConcern include StepIndicatorConcern include PhoneOtpRateLimitable - include OptInHelper before_action :confirm_two_factor_authenticated before_action :confirm_step_allowed @@ -20,7 +19,7 @@ def show def update clear_future_steps! result = phone_confirmation_otp_verification_form.submit(code: params[:code]) - analytics.idv_phone_confirmation_otp_submitted(**result.to_h, **opt_in_analytics_properties) + analytics.idv_phone_confirmation_otp_submitted(**result.to_h, **ab_test_analytics_buckets) irs_attempts_api_tracker.idv_phone_otp_submitted( success: result.success?, diff --git a/app/controllers/idv/phone_controller.rb b/app/controllers/idv/phone_controller.rb index 2aa67b5bafc..61bc17b4157 100644 --- a/app/controllers/idv/phone_controller.rb +++ b/app/controllers/idv/phone_controller.rb @@ -5,7 +5,6 @@ class PhoneController < ApplicationController include StepIndicatorConcern include PhoneOtpRateLimitable include PhoneOtpSendable - include OptInHelper attr_reader :idv_form @@ -33,7 +32,6 @@ def new analytics.idv_phone_of_record_visited( **ab_test_analytics_buckets, - **opt_in_analytics_properties, ) render :new, locals: { gpo_letter_available: gpo_letter_available } elsif async_state.missing? diff --git a/spec/controllers/concerns/idv/ab_test_analytics_concern_spec.rb b/spec/controllers/concerns/idv/ab_test_analytics_concern_spec.rb index 0144f341918..2efdccc5fde 100644 --- a/spec/controllers/concerns/idv/ab_test_analytics_concern_spec.rb +++ b/spec/controllers/concerns/idv/ab_test_analytics_concern_spec.rb @@ -32,7 +32,7 @@ def document_capture_session_uuid context 'idv_session is available' do before do sign_in(user) - expect(subject).to receive(:idv_session).once.and_return(idv_session) + allow(subject).to receive(:idv_session).and_return(idv_session) end it 'includes acuant_sdk_ab_test_analytics_args' do @@ -47,6 +47,24 @@ def document_capture_session_uuid idv_session.skip_hybrid_handoff = :shh_value expect(controller.ab_test_analytics_buckets).to include({ skip_hybrid_handoff: :shh_value }) end + + context 'opted_in_to_in_person_proofing value' do + before do + idv_session.opted_in_to_in_person_proofing = :opt_in_value + end + + it 'includes opted_in_to_in_person_proofing when enabled' do + allow(IdentityConfig.store).to receive(:in_person_proofing_opt_in_enabled). + and_return(true) + expect(controller.ab_test_analytics_buckets). + to include({ opted_in_to_in_person_proofing: :opt_in_value }) + end + + it 'does not include opted_in_to_in_person_proofing when disabled' do + expect(controller.ab_test_analytics_buckets). + not_to include({ opted_in_to_in_person_proofing: :opt_in_value }) + end + end end context 'idv_session is not available' do diff --git a/spec/controllers/idv/otp_verification_controller_spec.rb b/spec/controllers/idv/otp_verification_controller_spec.rb index 0bd703989d6..cb968aa1d43 100644 --- a/spec/controllers/idv/otp_verification_controller_spec.rb +++ b/spec/controllers/idv/otp_verification_controller_spec.rb @@ -21,11 +21,15 @@ sent_at: phone_confirmation_otp_sent_at, ) end + let(:ab_test_args) do + { sample_bucket1: :sample_value1, sample_bucket2: :sample_value2 } + end before do stub_analytics stub_attempts_tracker allow(@analytics).to receive(:track_event) + allow(subject).to receive(:ab_test_analytics_buckets).and_return(ab_test_args) sign_in(user) stub_verify_steps_one_and_two(user) @@ -177,6 +181,7 @@ second_factor_attempts_count: 0, second_factor_locked_at: nil, proofing_components: nil, + **ab_test_args, } expect(@analytics).to have_received(:track_event).with( diff --git a/spec/features/idv/analytics_spec.rb b/spec/features/idv/analytics_spec.rb index 81d9e6b4e81..fbd2d165fa5 100644 --- a/spec/features/idv/analytics_spec.rb +++ b/spec/features/idv/analytics_spec.rb @@ -113,7 +113,7 @@ proofing_components: { document_check: 'mock', document_type: 'state_id', source_check: 'aamva', resolution_check: 'lexis_nexis', threatmetrix: threatmetrix, threatmetrix_review_status: 'pass', address_check: 'lexis_nexis_address' }, }, 'IdV: phone confirmation otp submitted' => { - success: true, code_expired: false, code_matches: true, second_factor_attempts_count: 0, second_factor_locked_at: nil, errors: {}, + success: true, code_expired: false, code_matches: true, second_factor_attempts_count: 0, second_factor_locked_at: nil, errors: {}, acuant_sdk_upgrade_ab_test_bucket: :default, lexisnexis_instant_verify_workflow_ab_test_bucket: :default, skip_hybrid_handoff: nil, proofing_components: { document_check: 'mock', document_type: 'state_id', source_check: 'aamva', resolution_check: 'lexis_nexis', threatmetrix: threatmetrix, threatmetrix_review_status: 'pass', address_check: 'lexis_nexis_address' } }, :idv_enter_password_visited => { @@ -221,7 +221,7 @@ proofing_components: { document_check: 'mock', document_type: 'state_id', source_check: 'aamva', resolution_check: 'lexis_nexis', threatmetrix: threatmetrix, threatmetrix_review_status: 'pass', address_check: 'lexis_nexis_address' }, }, 'IdV: phone confirmation otp submitted' => { - success: true, code_expired: false, code_matches: true, second_factor_attempts_count: 0, second_factor_locked_at: nil, errors: {}, + success: true, code_expired: false, code_matches: true, second_factor_attempts_count: 0, second_factor_locked_at: nil, errors: {}, acuant_sdk_upgrade_ab_test_bucket: :default, lexisnexis_instant_verify_workflow_ab_test_bucket: :default, skip_hybrid_handoff: nil, proofing_components: { document_check: 'mock', document_type: 'state_id', source_check: 'aamva', resolution_check: 'lexis_nexis', threatmetrix: threatmetrix, threatmetrix_review_status: 'pass', address_check: 'lexis_nexis_address' } }, :idv_enter_password_visited => { @@ -431,7 +431,7 @@ proofing_components: { address_check: 'lexis_nexis_address', document_check: 'usps', resolution_check: 'lexis_nexis', threatmetrix: threatmetrix, threatmetrix_review_status: 'pass', source_check: 'aamva' }, }, 'IdV: phone confirmation otp submitted' => { - success: true, code_expired: false, code_matches: true, second_factor_attempts_count: 0, second_factor_locked_at: nil, errors: {}, + success: true, code_expired: false, code_matches: true, second_factor_attempts_count: 0, second_factor_locked_at: nil, errors: {}, acuant_sdk_upgrade_ab_test_bucket: :default, lexisnexis_instant_verify_workflow_ab_test_bucket: :default, skip_hybrid_handoff: nil, proofing_components: { document_check: 'usps', source_check: 'aamva', resolution_check: 'lexis_nexis', threatmetrix: threatmetrix, threatmetrix_review_status: 'pass', address_check: 'lexis_nexis_address' } }, :idv_enter_password_visited => { @@ -542,7 +542,7 @@ end end - context 'Happy hybrid path' do + context 'Happy hybrid path', allow_browser_log: true do before do allow(Telephony).to receive(:send_doc_auth_link).and_wrap_original do |impl, config| @sms_link = config[:link] @@ -633,7 +633,7 @@ complete_enter_password_step(user) end - it 'records all of the events' do + it 'records all of the events', allow_browser_log: true do gpo_path_events.each do |event, attributes| expect(fake_analytics).to have_logged_event(event, attributes) end From 25c6a08732fe1dccfbdd32ff1c406901184df518 Mon Sep 17 00:00:00 2001 From: Sonia Connolly Date: Tue, 19 Dec 2023 15:56:25 -0800 Subject: [PATCH 04/19] Add `ServiceProviderSession#selfie_required?` which returns true or false (#9808) It is set from sp_session[:biometric_comparison_required] and is always false if IdentityConfig.store.doc_auth_selfie_capture_enabled is false. [skip changelog] --- app/decorators/service_provider_session.rb | 5 +++ .../service_provider_session_spec.rb | 43 ++++++++++++++++++- 2 files changed, 47 insertions(+), 1 deletion(-) diff --git a/app/decorators/service_provider_session.rb b/app/decorators/service_provider_session.rb index e06869d9eae..4081137ea6c 100644 --- a/app/decorators/service_provider_session.rb +++ b/app/decorators/service_provider_session.rb @@ -70,6 +70,11 @@ def sp_issuer sp.issuer end + def selfie_required? + !!(IdentityConfig.store.doc_auth_selfie_capture_enabled && + sp_session[:biometric_comparison_required]) + end + def cancel_link_url view_context.new_user_session_url(request_id: sp_session[:request_id]) end diff --git a/spec/decorators/service_provider_session_spec.rb b/spec/decorators/service_provider_session_spec.rb index 921577d7dc3..3e02af038a1 100644 --- a/spec/decorators/service_provider_session_spec.rb +++ b/spec/decorators/service_provider_session_spec.rb @@ -6,11 +6,12 @@ ServiceProviderSession.new( sp: sp, view_context: view_context, - sp_session: {}, + sp_session: sp_session, service_provider_request: service_provider_request, ) end let(:sp) { build_stubbed(:service_provider) } + let(:sp_session) { {} } let(:service_provider_request) { ServiceProviderRequest.new } let(:sp_name) { subject.sp_name } let(:sp_create_link) { '/sign_up/enter_email' } @@ -178,6 +179,46 @@ end end + describe '#selfie_required' do + before do + allow(IdentityConfig.store).to receive(:doc_auth_selfie_capture_enabled). + and_return(selfie_capture_enabled) + end + + context 'doc_auth_selfie_capture_enabled is true' do + let(:selfie_capture_enabled) { true } + + it 'returns true when sp biometric_comparison_required is true' do + sp_session[:biometric_comparison_required] = true + expect(subject.selfie_required?).to eq(true) + end + + it 'returns true when sp biometric_comparison_required is truthy' do + sp_session[:biometric_comparison_required] = 1 + expect(subject.selfie_required?).to eq(true) + end + + it 'returns false when sp biometric_comparison_required is false' do + sp_session[:biometric_comparison_required] = false + expect(subject.selfie_required?).to eq(false) + end + + it 'returns false when sp biometric_comparison_required is nil' do + sp_session[:biometric_comparison_required] = nil + expect(subject.selfie_required?).to eq(false) + end + end + + context 'doc_auth_selfie_capture_enabled is false' do + let(:selfie_capture_enabled) { false } + + it 'returns false' do + sp_session[:biometric_comparison_required] = true + expect(subject.selfie_required?).to eq(false) + end + end + end + describe '#cancel_link_url' do subject(:decorator) do ServiceProviderSession.new( From 45a64683ba9cfa57f098520e4810e6cad9eb9293 Mon Sep 17 00:00:00 2001 From: Sonia Connolly Date: Tue, 19 Dec 2023 17:12:57 -0800 Subject: [PATCH 05/19] Use FlowPolicyHelper in controller specs (#9779) * Use stub_up_to helper in agreement_controller_spec * Use stub_up_to helper in hybrid_handoff_controller_spec * Use stub_up_to helper in document_capture_controller_spec * Use stub_up_to helper in ssn_controller * Use stub_up_to helper in verify_info_controller * Use stub_up_to helper in phone_controller * Remove repeated stub_analytics in phone_controller_spec * Remove stub_verify_steps_one_and_two in individual phone_controller specs [skip changelog] --------- Co-authored-by: Doug Price --- .../idv/agreement_controller_spec.rb | 11 +- .../idv/document_capture_controller_spec.rb | 13 +-- .../idv/hybrid_handoff_controller_spec.rb | 7 +- spec/controllers/idv/phone_controller_spec.rb | 100 ++++-------------- spec/controllers/idv/ssn_controller_spec.rb | 8 +- .../idv/verify_info_controller_spec.rb | 13 +-- 6 files changed, 42 insertions(+), 110 deletions(-) diff --git a/spec/controllers/idv/agreement_controller_spec.rb b/spec/controllers/idv/agreement_controller_spec.rb index dd13721b7e8..5c2e3bb9bdf 100644 --- a/spec/controllers/idv/agreement_controller_spec.rb +++ b/spec/controllers/idv/agreement_controller_spec.rb @@ -1,6 +1,8 @@ require 'rails_helper' RSpec.describe Idv::AgreementController do + include FlowPolicyHelper + let(:user) { create(:user) } let(:ab_test_args) do @@ -9,8 +11,8 @@ before do stub_sign_in(user) + stub_up_to(:welcome, idv_session: subject.idv_session) stub_analytics - subject.idv_session.welcome_visited = true allow(subject).to receive(:ab_test_analytics_buckets).and_return(ab_test_args) end @@ -79,7 +81,7 @@ context 'agreement already visited' do it 'does not redirect to hybrid_handoff' do - subject.idv_session.idv_consent_given = true + stub_up_to(:agreement, idv_session: subject.idv_session) get :show @@ -88,10 +90,7 @@ context 'and verify info already completed' do before do - subject.idv_session.flow_path = 'standard' - subject.idv_session.pii_from_doc = { first_name: 'Susan' } - subject.idv_session.ssn = '123-45-6789' - subject.idv_session.resolution_successful = true + stub_up_to(:verify_info, idv_session: subject.idv_session) end it 'renders the show template' do diff --git a/spec/controllers/idv/document_capture_controller_spec.rb b/spec/controllers/idv/document_capture_controller_spec.rb index c25066bca35..8dd84a6324b 100644 --- a/spec/controllers/idv/document_capture_controller_spec.rb +++ b/spec/controllers/idv/document_capture_controller_spec.rb @@ -1,6 +1,8 @@ require 'rails_helper' RSpec.describe Idv::DocumentCaptureController do + include FlowPolicyHelper + let(:document_capture_session_requested_at) { Time.zone.now } let!(:document_capture_session) do @@ -20,8 +22,8 @@ before do stub_sign_in(user) + stub_up_to(:hybrid_handoff, idv_session: subject.idv_session) stub_analytics - subject.idv_session.flow_path = 'standard' subject.idv_session.document_capture_session_uuid = document_capture_session_uuid allow(subject).to receive(:ab_test_analytics_buckets).and_return(ab_test_args) @@ -102,8 +104,6 @@ context 'hybrid handoff step is not complete' do it 'redirects to hybrid handoff' do - subject.idv_session.welcome_visited = true - subject.idv_session.idv_consent_given = true subject.idv_session.flow_path = nil get :show @@ -114,12 +114,7 @@ context 'verify info step is complete' do it 'renders show' do - subject.idv_session.welcome_visited = true - subject.idv_session.idv_consent_given = true - subject.idv_session.flow_path = 'standard' - subject.idv_session.pii_from_doc = Idp::Constants::MOCK_IDV_APPLICANT - subject.idv_session.ssn = Idp::Constants::MOCK_IDV_APPLICANT_WITH_SSN[:ssn] - subject.idv_session.resolution_successful = true + stub_up_to(:verify_info, idv_session: subject.idv_session) get :show diff --git a/spec/controllers/idv/hybrid_handoff_controller_spec.rb b/spec/controllers/idv/hybrid_handoff_controller_spec.rb index bd02f735e77..90bb5f04ccf 100644 --- a/spec/controllers/idv/hybrid_handoff_controller_spec.rb +++ b/spec/controllers/idv/hybrid_handoff_controller_spec.rb @@ -1,6 +1,8 @@ require 'rails_helper' RSpec.describe Idv::HybridHandoffController do + include FlowPolicyHelper + let(:user) { create(:user) } let(:ab_test_args) do @@ -9,9 +11,9 @@ before do stub_sign_in(user) + stub_up_to(:agreement, idv_session: subject.idv_session) stub_analytics stub_attempts_tracker - subject.idv_session.idv_consent_given = true allow(subject).to receive(:ab_test_analytics_buckets).and_return(ab_test_args) end @@ -71,7 +73,6 @@ context 'agreement step is not complete' do before do - subject.idv_session.welcome_visited = true subject.idv_session.idv_consent_given = nil end @@ -138,7 +139,7 @@ context 'user has already completed verify info' do before do - subject.idv_session.mark_verify_info_step_complete! + stub_up_to(:verify_info, idv_session: subject.idv_session) end it 'does set redo_document_capture to true in idv_session' do diff --git a/spec/controllers/idv/phone_controller_spec.rb b/spec/controllers/idv/phone_controller_spec.rb index 1be321178aa..4d3952f75be 100644 --- a/spec/controllers/idv/phone_controller_spec.rb +++ b/spec/controllers/idv/phone_controller_spec.rb @@ -1,6 +1,8 @@ require 'rails_helper' RSpec.describe Idv::PhoneController do + include FlowPolicyHelper + let(:max_attempts) { RateLimiter.max_attempts(:proof_address) } let(:good_phone) { '+1 (703) 555-0000' } let(:bad_phone) do @@ -39,18 +41,22 @@ end end - describe '#new' do - let(:user) do - create( - :user, :with_phone, - with: { phone: good_phone, confirmed_at: Time.zone.now } - ) - end + let(:user) do + create( + :user, :with_phone, + with: { phone: good_phone, confirmed_at: Time.zone.now } + ) + end - before do - stub_verify_steps_one_and_two(user) - end + before do + stub_sign_in(user) + stub_up_to(:verify_info, idv_session: subject.idv_session) + stub_analytics + stub_attempts_tracker + allow(@analytics).to receive(:track_event) + end + describe '#new' do it 'updates the doc auth log for the user for the usps_letter_sent event' do unstub_analytics doc_auth_log = DocAuthLog.create(user_id: user.id) @@ -88,11 +94,6 @@ context 'when the user has not finished the verify step' do before do - subject.idv_session.welcome_visited = true - subject.idv_session.idv_consent_given = true - subject.idv_session.flow_path = 'standard' - subject.idv_session.pii_from_doc = Idp::Constants::MOCK_IDV_APPLICANT - subject.idv_session.ssn = '123-45-6789' subject.idv_session.applicant = nil subject.idv_session.resolution_successful = nil end @@ -239,6 +240,12 @@ end describe '#create' do + let(:user) do + create( + :user, :with_phone, + with: { phone: '+1 (415) 555-0130' } + ) + end let(:ab_test_args) do { sample_bucket1: :sample_value1, sample_bucket2: :sample_value2 } end @@ -260,11 +267,6 @@ } end before do - user = build(:user, :with_phone, with: { phone: '+1 (415) 555-0130' }) - stub_verify_steps_one_and_two(user) - stub_analytics - stub_attempts_tracker - allow(@analytics).to receive(:track_event) end it 'renders #new' do @@ -337,15 +339,7 @@ } } end - before do - stub_analytics - stub_attempts_tracker - allow(@analytics).to receive(:track_event) - end - it 'invalidates future steps and invalidates phone step' do - user = build(:user, :with_phone, with: { phone: good_phone, confirmed_at: Time.zone.now }) - stub_verify_steps_one_and_two(user) subject.idv_session.vendor_phone_confirmation = true subject.idv_session.user_phone_confirmation = true @@ -358,9 +352,6 @@ end it 'tracks events with valid phone' do - user = build(:user, :with_phone, with: { phone: good_phone, confirmed_at: Time.zone.now }) - stub_verify_steps_one_and_two(user) - expect(@irs_attempts_api_tracker).to receive(:idv_phone_submitted).with( success: true, phone_number: good_phone, @@ -388,10 +379,6 @@ end it 'updates the doc auth log for the user with verify_phone_submit step' do - user = create(:user, :with_phone, with: { phone: good_phone, confirmed_at: Time.zone.now }) - unstub_analytics - stub_verify_steps_one_and_two(user) - doc_auth_log = DocAuthLog.create(user_id: user.id) expect { put :create, params: { idv_phone_form: { phone: good_phone } } }.to( @@ -400,15 +387,6 @@ end context 'when same as user phone' do - before do - user = build( - :user, :with_phone, with: { - phone: good_phone, confirmed_at: Time.zone.now - } - ) - stub_verify_steps_one_and_two(user) - end - it 'redirects to otp delivery page' do original_applicant = subject.idv_session.applicant.dup @@ -446,15 +424,6 @@ end context 'when different phone from user phone' do - before do - user = build( - :user, :with_phone, with: { - phone: '+1 (415) 555-0130', confirmed_at: Time.zone.now - } - ) - stub_verify_steps_one_and_two(user) - end - it 'redirects to otp page and does not set phone_confirmed_at' do put :create, params: phone_params @@ -484,11 +453,6 @@ it 'tracks event with valid phone' do proofing_phone = Phonelib.parse(good_phone) - user = build(:user, with: { phone: '+1 (415) 555-0130', phone_confirmed_at: Time.zone.now }) - stub_verify_steps_one_and_two(user) - - stub_analytics - allow(@analytics).to receive(:track_event) result = { success: true, @@ -523,12 +487,6 @@ end it 'tracks that the hybrid handoff phone was used' do - user = build(:user) - stub_verify_steps_one_and_two(user) - - stub_analytics - allow(@analytics).to receive(:track_event) - expect(@analytics).to receive(:track_event).ordered.with( 'IdV: phone confirmation form', hash_including(:success) ) @@ -545,9 +503,6 @@ context 'when verification fails' do it 'renders failure page and does not set phone confirmation' do - user = build(:user, with: { phone: '+1 (415) 555-0130', phone_confirmed_at: Time.zone.now }) - stub_verify_steps_one_and_two(user) - put :create, params: { idv_phone_form: { phone: bad_phone } } expect(response).to redirect_to idv_phone_path @@ -560,9 +515,6 @@ end it 'renders timeout page and does not set phone confirmation' do - user = build(:user, with: { phone: '+1 (415) 555-0130', phone_confirmed_at: Time.zone.now }) - stub_verify_steps_one_and_two(user) - put :create, params: { idv_phone_form: { phone: timeout_phone } } expect(response).to redirect_to idv_phone_path @@ -576,11 +528,6 @@ it 'tracks event with invalid phone' do proofing_phone = Phonelib.parse(bad_phone) - user = build(:user, with: { phone: '+1 (415) 555-0130', phone_confirmed_at: Time.zone.now }) - stub_verify_steps_one_and_two(user) - - stub_analytics - allow(@analytics).to receive(:track_event) result = { success: false, @@ -621,9 +568,6 @@ before do stub_analytics - user = create(:user, with: { phone: '+1 (415) 555-0130' }) - stub_verify_steps_one_and_two(user) - rate_limiter = RateLimiter.new(rate_limit_type: :proof_address, user: user) rate_limiter.increment_to_limited! diff --git a/spec/controllers/idv/ssn_controller_spec.rb b/spec/controllers/idv/ssn_controller_spec.rb index 3adea38c86e..81d276d2735 100644 --- a/spec/controllers/idv/ssn_controller_spec.rb +++ b/spec/controllers/idv/ssn_controller_spec.rb @@ -1,6 +1,8 @@ require 'rails_helper' RSpec.describe Idv::SsnController do + include FlowPolicyHelper + let(:ssn) { Idp::Constants::MOCK_IDV_APPLICANT_WITH_SSN[:ssn] } let(:user) { create(:user) } @@ -11,8 +13,7 @@ before do stub_sign_in(user) - subject.idv_session.flow_path = 'standard' - subject.idv_session.pii_from_doc = Idp::Constants::MOCK_IDV_APPLICANT.dup + stub_up_to(:document_capture, idv_session: subject.idv_session) stub_analytics stub_attempts_tracker allow(@analytics).to receive(:track_event) @@ -218,9 +219,6 @@ context 'when pii_from_doc is not present' do before do - subject.idv_session.welcome_visited = true - subject.idv_session.idv_consent_given = true - subject.idv_session.flow_path = 'standard' subject.idv_session.pii_from_doc = nil end diff --git a/spec/controllers/idv/verify_info_controller_spec.rb b/spec/controllers/idv/verify_info_controller_spec.rb index 20aec02dce3..88b138fbc1b 100644 --- a/spec/controllers/idv/verify_info_controller_spec.rb +++ b/spec/controllers/idv/verify_info_controller_spec.rb @@ -1,6 +1,8 @@ require 'rails_helper' RSpec.describe Idv::VerifyInfoController do + include FlowPolicyHelper + let(:user) { create(:user) } let(:analytics_hash) do { @@ -17,13 +19,9 @@ before do stub_sign_in(user) + stub_up_to(:ssn, idv_session: subject.idv_session) stub_analytics stub_attempts_tracker - subject.idv_session.welcome_visited = true - subject.idv_session.idv_consent_given = true - subject.idv_session.flow_path = 'standard' - subject.idv_session.pii_from_doc = Idp::Constants::MOCK_IDV_APPLICANT.dup - subject.idv_session.ssn = Idp::Constants::MOCK_IDV_APPLICANT_WITH_SSN[:ssn] allow(subject).to receive(:ab_test_analytics_buckets).and_return(ab_test_args) end @@ -101,10 +99,7 @@ context 'when the user has already verified their info' do it 'renders show' do - subject.idv_session.resolution_successful = true - subject.idv_session.pii_from_doc = Idp::Constants::MOCK_IDV_APPLICANT - subject.idv_session.ssn = Idp::Constants::MOCK_IDV_APPLICANT_WITH_SSN[:ssn] - subject.idv_session.applicant = Idp::Constants::MOCK_IDV_APPLICANT_WITH_SSN + stub_up_to(:verify_info, idv_session: subject.idv_session) get :show From 169102d56e0873f57f8b5e0ba0263d181c70f662 Mon Sep 17 00:00:00 2001 From: Jonathan Hooper Date: Wed, 20 Dec 2023 09:33:51 -0500 Subject: [PATCH 06/19] Remove the passive encryption of encrypted PII in the session (#9772) In #9754 the code that depended on the passive encryption of `decrypted_pii` in the session was removed. Some of the code removed here was left behind to ensure that PII remained encrypted. Once the code in #9754 is deployed to production this should be safe to merge. [skip changelog] --- app/services/out_of_band_session_accessor.rb | 1 - app/services/pii/cacher.rb | 2 -- lib/session_encryptor.rb | 17 ----------------- .../controllers/idv/sessions_controller_spec.rb | 2 +- spec/lib/session_encryptor_spec.rb | 13 ------------- 5 files changed, 1 insertion(+), 34 deletions(-) diff --git a/app/services/out_of_band_session_accessor.rb b/app/services/out_of_band_session_accessor.rb index 556125e8cc6..319053c4659 100644 --- a/app/services/out_of_band_session_accessor.rb +++ b/app/services/out_of_band_session_accessor.rb @@ -64,7 +64,6 @@ def put_empty_user_session(expiration = 5.minutes) # @param [#to_s] profile_id def put_pii(profile_id:, pii:, expiration: 5.minutes) data = { - decrypted_pii: pii.to_h.to_json, encrypted_profiles: { profile_id.to_s => SessionEncryptor.new.kms_encrypt(pii.to_h.to_json) }, } diff --git a/app/services/pii/cacher.rb b/app/services/pii/cacher.rb index 4f39dbebeef..4d22331d1e8 100644 --- a/app/services/pii/cacher.rb +++ b/app/services/pii/cacher.rb @@ -36,8 +36,6 @@ def exists_in_session? end def delete - user_session.delete(:decrypted_pii) - user_session.delete(:encrypted_pii) user_session.delete(:encrypted_profiles) end diff --git a/lib/session_encryptor.rb b/lib/session_encryptor.rb index 77b94875441..919c06475d3 100644 --- a/lib/session_encryptor.rb +++ b/lib/session_encryptor.rb @@ -61,7 +61,6 @@ def load(value) def dump(value) value.deep_stringify_keys! - kms_encrypt_pii!(value) kms_encrypt_sensitive_paths!(value, SENSITIVE_PATHS) alert_or_raise_if_contains_sensitive_keys!(value) plain = JSON.generate(value) @@ -104,22 +103,6 @@ def outer_decrypt(ciphertext) private - # The PII bundle is stored in the user session in the 'decrypted_pii' key. - # The PII is decrypted with the user's password when they successfully submit it and then - # stored in the session. Before saving the session, this method encrypts the PII with KMS and - # stores it in the 'encrypted_pii' key. - # - # The PII is not frequently needed in its KMS-decrypted state. To reduce the - # risks around holding plaintext PII in memory during requests, this PII is KMS-decrypted - # on-demand by the Pii::Cacher. - def kms_encrypt_pii!(session) - return unless session.dig('warden.user.user.session', 'decrypted_pii') - decrypted_pii = session['warden.user.user.session'].delete('decrypted_pii') - session['warden.user.user.session']['encrypted_pii'] = - kms_encrypt(decrypted_pii) - nil - end - # This method extracts all of the sensitive paths that exist into a # separate hash. This separate hash is then encrypted and placed in the session. # We use #reduce to build the nested empty hash if needed. If Hash#bury diff --git a/spec/controllers/idv/sessions_controller_spec.rb b/spec/controllers/idv/sessions_controller_spec.rb index 909694f0ca1..ea5331427dd 100644 --- a/spec/controllers/idv/sessions_controller_spec.rb +++ b/spec/controllers/idv/sessions_controller_spec.rb @@ -35,7 +35,7 @@ expect(controller.user_session['idv/in_person']).to be_blank end - it 'clears the decrypted_pii session' do + it 'clears the encrypted_profiles session' do expect(controller.user_session[:encrypted_profiles]).to be_blank end end diff --git a/spec/lib/session_encryptor_spec.rb b/spec/lib/session_encryptor_spec.rb index 5c2c5e38a18..048351aa390 100644 --- a/spec/lib/session_encryptor_spec.rb +++ b/spec/lib/session_encryptor_spec.rb @@ -34,19 +34,6 @@ ) end - it 'encrypts decrypted_pii bundle without automatically decrypting' do - session = { 'warden.user.user.session' => { - 'decrypted_pii' => { 'ssn' => '666-66-6666' }.to_json, - } } - - ciphertext = subject.dump(session) - - result = subject.load(ciphertext) - - expect(result.fetch('warden.user.user.session')['decrypted_pii']).to eq nil - expect(result.fetch('warden.user.user.session')['encrypted_pii']).to_not eq nil - end - it 'KMS encrypts/decrypts doc auth elements of the session' do session = { 'warden.user.user.session' => { 'idv' => { 'ssn' => '666-66-6666' }, From 923fc916e33f0ed4cc6c43ba1e53219476349db6 Mon Sep 17 00:00:00 2001 From: dawei-nava <130466753+dawei-nava@users.noreply.github.com> Date: Wed, 20 Dec 2023 10:32:52 -0500 Subject: [PATCH 07/19] LG-11118: zip code format validation, zip+4 code. (#9802) changelog: Internal, Doc Auth, Validate zip code having zip+4 format. --- app/forms/idv/doc_pii_form.rb | 14 +++++++------- spec/forms/idv/doc_pii_form_spec.rb | 6 +++--- 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/app/forms/idv/doc_pii_form.rb b/app/forms/idv/doc_pii_form.rb index e9eb97c01fc..35e1a81d2a7 100644 --- a/app/forms/idv/doc_pii_form.rb +++ b/app/forms/idv/doc_pii_form.rb @@ -11,7 +11,13 @@ class DocPiiForm message: proc { I18n.t('doc_auth.errors.general.no_liveness') } } - validate :zipcode_valid? + validates :zipcode, format: { + with: /\A[0-9]{5}(?:-[0-9]{4})?\z/, + message: proc { + I18n.t('doc_auth.errors.general.no_liveness') + }, + } + validates :jurisdiction, inclusion: { in: Idp::Constants::STATE_AND_TERRITORY_CODES, message: proc { I18n.t('doc_auth.errors.general.no_liveness') @@ -82,12 +88,6 @@ def dob_valid? end end - def zipcode_valid? - return if zipcode.is_a?(String) && zipcode.present? - - errors.add(:zipcode, generic_error, type: :zipcode) - end - def generic_error I18n.t('doc_auth.errors.general.no_liveness') end diff --git a/spec/forms/idv/doc_pii_form_spec.rb b/spec/forms/idv/doc_pii_form_spec.rb index eb8487fc3b2..f339e7aea35 100644 --- a/spec/forms/idv/doc_pii_form_spec.rb +++ b/spec/forms/idv/doc_pii_form_spec.rb @@ -43,14 +43,14 @@ state: Faker::Address.state_abbr, } end - let(:non_string_zipcode_pii) do + let(:invalid_zipcode_pii) do { first_name: Faker::Name.first_name, last_name: Faker::Name.last_name, dob: valid_dob, address1: Faker::Address.street_address, state: Faker::Address.state_abbr, - zipcode: 12345, + zipcode: 123456, state_id_jurisdiction: 'AL', } end @@ -174,7 +174,7 @@ end context 'when there is a non-string zipcode' do - let(:pii) { non_string_zipcode_pii } + let(:pii) { invalid_zipcode_pii } it 'returns a single generic pii error' do result = subject.submit From a3026f5bd23d683a2e1119326c08065bcc3e5c2f Mon Sep 17 00:00:00 2001 From: Mitchell Henke Date: Wed, 20 Dec 2023 11:17:37 -0600 Subject: [PATCH 08/19] Update Brakeman and view_component (#9813) * update view_component * update brakeman changelog: Internal, Dependencies, Update brakeman and view_component * Use new method option for with_request_url --------- Co-authored-by: Andrew Duthie --- Gemfile | 2 +- Gemfile.lock | 6 +++--- spec/components/tab_navigation_component_spec.rb | 10 +--------- 3 files changed, 5 insertions(+), 13 deletions(-) diff --git a/Gemfile b/Gemfile index 5d324100567..9eb5b52f8d6 100644 --- a/Gemfile +++ b/Gemfile @@ -74,7 +74,7 @@ gem 'strong_migrations', '>= 0.4.2' gem 'subprocess', require: false gem 'terminal-table', require: false gem 'valid_email', '>= 0.1.3' -gem 'view_component', '~> 3.0.0' +gem 'view_component', '~> 3.0' gem 'webauthn', '~> 2.5.2' gem 'xmldsig', '~> 0.6' gem 'xmlenc', '~> 0.7', '>= 0.7.1' diff --git a/Gemfile.lock b/Gemfile.lock index d7f261966ea..f3799b684e9 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -212,7 +212,7 @@ GEM bindata (2.4.15) bootsnap (1.17.0) msgpack (~> 1.2) - brakeman (6.0.1) + brakeman (6.1.0) browser (5.3.1) builder (3.2.4) bullet (7.1.4) @@ -684,7 +684,7 @@ GEM activemodel mail (>= 2.6.1) simpleidn - view_component (3.0.0) + view_component (3.8.0) activesupport (>= 5.2.0, < 8.0) concurrent-ruby (~> 1.0) method_source (~> 1.0) @@ -842,7 +842,7 @@ DEPENDENCIES tableparser terminal-table valid_email (>= 0.1.3) - view_component (~> 3.0.0) + view_component (~> 3.0) webauthn (~> 2.5.2) webmock xmldsig (~> 0.6) diff --git a/spec/components/tab_navigation_component_spec.rb b/spec/components/tab_navigation_component_spec.rb index df1672f19a0..8bf8686a95c 100644 --- a/spec/components/tab_navigation_component_spec.rb +++ b/spec/components/tab_navigation_component_spec.rb @@ -35,15 +35,7 @@ post '(:example_param)/second' => 'application#second_create' end - with_request_url(request_path) do - vc_test_request.request_method = request_method - vc_test_request.path_parameters = Rails.application.routes.recognize_path_with_request( - vc_test_request, - request_path, - {}, - ) - example.run - end + with_request_url(request_path, method: request_method) { example.run } Rails.application.reload_routes! end From 4b9bccc97472ab5c17bf241edf4ad83d584c988c Mon Sep 17 00:00:00 2001 From: Shannon A <20867088+svalexander@users.noreply.github.com> Date: Wed, 20 Dec 2023 12:40:47 -0500 Subject: [PATCH 09/19] LG-11202 add untracked costs (#9753) * update add costs with residential_address stage * in person cost spec added to verify info controller spec * skip adding res_address lex nex cost if same_address is true * add tests for cost for ipp flow * refactor spec * changelog: Internal, Verify info concern, update cost tracking for ipp * update spec --- .../concerns/idv/verify_info_concern.rb | 4 + .../in_person/verify_info_controller_spec.rb | 112 ++++++++++++++++++ 2 files changed, 116 insertions(+) diff --git a/app/controllers/concerns/idv/verify_info_concern.rb b/app/controllers/concerns/idv/verify_info_concern.rb index fa84a187410..ace6e33f77a 100644 --- a/app/controllers/concerns/idv/verify_info_concern.rb +++ b/app/controllers/concerns/idv/verify_info_concern.rb @@ -315,6 +315,10 @@ def add_proofing_costs(results) if stage == :resolution # transaction_id comes from ConversationId add_cost(:lexis_nexis_resolution, transaction_id: hash[:transaction_id]) + elsif stage == :residential_address + next if pii[:same_address_as_id] == 'true' + next if hash[:vendor_name] == 'ResidentialAddressNotRequired' + add_cost(:lexis_nexis_resolution, transaction_id: hash[:transaction_id]) elsif stage == :state_id next if hash[:exception].present? next if hash[:vendor_name] == 'UnsupportedJurisdiction' diff --git a/spec/controllers/idv/in_person/verify_info_controller_spec.rb b/spec/controllers/idv/in_person/verify_info_controller_spec.rb index dd4331b0535..f09de384860 100644 --- a/spec/controllers/idv/in_person/verify_info_controller_spec.rb +++ b/spec/controllers/idv/in_person/verify_info_controller_spec.rb @@ -103,6 +103,7 @@ threatmetrix_review_status: review_status, } end + it 'logs proofing results with analytics_id' do allow(controller).to receive(:load_async_state).and_return(async_state) allow(async_state).to receive(:done?).and_return(true) @@ -116,6 +117,117 @@ ) end end + + context 'tracks costs' do + let(:review_status) { 'pass' } + let(:async_state) { instance_double(ProofingSessionAsyncResult) } + let(:adjudicated_result) do + { + context: { + stages: { + threatmetrix: { + transaction_id: 1, + }, + resolution: { + transaction_id: 'resolution-mock-transaction-id-123', + vendor_name: 'ResolutionMock', + }, + residential_address: { + transaction_id: 'resolution-mock-transaction-id-123', + vendor_name: 'ResolutionMock', + }, + state_id: { + transaction_id: 'state-id-mock-transaction-id-456', + vendor_name: 'StateIdMock', + }, + }, + }, + } + end + + before do + allow(controller).to receive(:load_async_state).and_return(async_state) + allow(async_state).to receive(:done?).and_return(true) + end + + context 'when same address as id is true and in aamva jurisdiction' do + it 'adds costs to database' do + allow(async_state).to receive(:result).and_return(adjudicated_result) + + get :show + + lexis_nexis_costs = SpCost.where(cost_type: 'lexis_nexis_resolution') + expect(lexis_nexis_costs.count).to eq(1) + + aamva_costs = SpCost.where(cost_type: 'aamva') + expect(aamva_costs.count).to eq(1) + + threatmetrix_costs = SpCost.where(cost_type: 'threatmetrix') + expect(threatmetrix_costs.count).to eq(1) + end + end + + context 'when same address as id is true and not in aamva jurisdiction' do + it 'adds costs to database' do + adjudicated_result[:context][:stages][:state_id][:vendor_name] = 'UnsupportedJurisdiction' + allow(async_state).to receive(:result).and_return(adjudicated_result) + + get :show + + lexis_nexis_costs = SpCost.where(cost_type: 'lexis_nexis_resolution') + expect(lexis_nexis_costs.count).to eq(1) + + aamva_costs = SpCost.where(cost_type: 'aamva') + expect(aamva_costs.count).to eq(0) + + threatmetrix_costs = SpCost.where(cost_type: 'threatmetrix') + expect(threatmetrix_costs.count).to eq(1) + end + end + + context 'when same address as id is false and in aamva jurisdiction' do + let(:pii_from_user) do + { same_address_as_id: 'false' } + end + + it 'adds costs to database' do + allow(async_state).to receive(:result).and_return(adjudicated_result) + + get :show + + lexis_nexis_costs = SpCost.where(cost_type: 'lexis_nexis_resolution') + expect(lexis_nexis_costs.count).to eq(2) + + aamva_costs = SpCost.where(cost_type: 'aamva') + expect(aamva_costs.count).to eq(1) + + threatmetrix_costs = SpCost.where(cost_type: 'threatmetrix') + expect(threatmetrix_costs.count).to eq(1) + end + end + + context 'when same address as id is false and not in aamva jurisdiction' do + let(:pii_from_user) do + { same_address_as_id: 'false' } + end + + it 'adds costs to database' do + adjudicated_result[:context][:stages][:state_id][:vendor_name] = 'UnsupportedJurisdiction' + allow(async_state).to receive(:result).and_return(adjudicated_result) + + get :show + + lexis_nexis_costs = SpCost.where(cost_type: 'lexis_nexis_resolution') + expect(lexis_nexis_costs.count).to eq(2) + + aamva_costs = SpCost.where(cost_type: 'aamva') + expect(aamva_costs.count).to eq(0) + + threatmetrix_costs = SpCost.where(cost_type: 'threatmetrix') + expect(threatmetrix_costs.count).to eq(1) + end + end + end end describe '#update' do From ce49617e9682dae8333f439f11ddaeaae68e96a3 Mon Sep 17 00:00:00 2001 From: Andrew Duthie <1779930+aduth@users.noreply.github.com> Date: Wed, 20 Dec 2023 12:47:35 -0500 Subject: [PATCH 10/19] Remove unnecessary clearfix from account page widgets (#9812) changelog: Internal, Code Quality, Simplify to remove unnecessary page markup --- app/views/accounts/_pii.html.erb | 2 +- app/views/accounts/_webauthn_roaming.html.erb | 1 - 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/app/views/accounts/_pii.html.erb b/app/views/accounts/_pii.html.erb index a3e78960153..c4597b8ebaa 100644 --- a/app/views/accounts/_pii.html.erb +++ b/app/views/accounts/_pii.html.erb @@ -68,7 +68,7 @@ <% unless locked_for_session %> -
+
<%= image_tag asset_url('lock.svg'), width: 8, height: 10, class: 'margin-right-1' %> <%= t('account.security.text') %> diff --git a/app/views/accounts/_webauthn_roaming.html.erb b/app/views/accounts/_webauthn_roaming.html.erb index 0627c1d1f29..e50316bbce8 100644 --- a/app/views/accounts/_webauthn_roaming.html.erb +++ b/app/views/accounts/_webauthn_roaming.html.erb @@ -17,7 +17,6 @@
<% end %>
-
<% end %>
From d2e0b2a255206cb7d59598df2b15288c0a1eba1e Mon Sep 17 00:00:00 2001 From: Matt Hinz Date: Wed, 20 Dec 2023 10:51:34 -0800 Subject: [PATCH 11/19] LG-11725 FlowPolicy for personal key (#9776) * Move #confirm_profile_has_been_created tests into #show * First crack at wiring personal key into FlowPolicy * Skip handle_fraud entirely in PersonalKeyController We know you are a fraud, but we need you to acknowledge your personal key first * Add tests for StepInfo * Remove action: :show from stepinfo * Remove redundant concerns / before actions * Tidy up preconditions a smidge * Fix method name in spec * Update preconditions spec * Test confirm_step_allowed before_action in use * Test we're skipping the right before_actions * changelog: Internal, Identity verification, Integrate PersonalKeyController with FlowPolicy * Refactor (most) of before block to use FlowPolicyHelper * Invalidate personal key when undoing * Use stub_up_to instead of stub_verify_steps_one_and_two * Don't pass applicant to stub_up_to * Set applicant to have a phone when stubbing :phone step * Add personal key specs to flow_policy_spec --------- Co-authored-by: Sonia Connolly --- .../idv/enter_password_controller.rb | 2 +- .../idv/personal_key_controller.rb | 46 ++-- app/policies/idv/flow_policy.rb | 1 + .../idv/personal_key_controller_spec.rb | 208 ++++++++++++++---- spec/policies/idv/flow_policy_spec.rb | 28 ++- spec/support/flow_policy_helper.rb | 1 + 6 files changed, 220 insertions(+), 66 deletions(-) diff --git a/app/controllers/idv/enter_password_controller.rb b/app/controllers/idv/enter_password_controller.rb index 48fd51fc886..f8a8088c5ec 100644 --- a/app/controllers/idv/enter_password_controller.rb +++ b/app/controllers/idv/enter_password_controller.rb @@ -77,7 +77,7 @@ def self.step_info key: :enter_password, controller: self, action: :new, - next_steps: [FlowPolicy::FINAL], + next_steps: [:personal_key], preconditions: ->(idv_session:, user:) do idv_session.phone_or_address_step_complete? end, diff --git a/app/controllers/idv/personal_key_controller.rb b/app/controllers/idv/personal_key_controller.rb index 6e6bbef4ea0..4df87e7b96a 100644 --- a/app/controllers/idv/personal_key_controller.rb +++ b/app/controllers/idv/personal_key_controller.rb @@ -1,17 +1,21 @@ module Idv class PersonalKeyController < ApplicationController include Idv::AvailabilityConcern - include IdvSession + include IdvStepConcern include StepIndicatorConcern include SecureHeadersConcern - include FraudReviewConcern include OptInHelper before_action :apply_secure_headers_override - before_action :confirm_two_factor_authenticated - before_action :confirm_phone_or_address_confirmed - before_action :confirm_profile_has_been_created - before_action :confirm_personal_key_not_acknowledged + before_action :confirm_step_allowed + + # Personal key is kind of a special case, since you're always meant to + # look at it after your profile has been minted. We opt out of a few + # standard before_actions and handle them in our own special way below. + skip_before_action :confirm_idv_needed + skip_before_action :confirm_personal_key_acknowledged_if_needed + skip_before_action :confirm_no_pending_in_person_enrollment + skip_before_action :handle_fraud def show analytics.idv_personal_key_visited( @@ -38,6 +42,22 @@ def update redirect_to next_step end + def self.step_info + Idv::StepInfo.new( + key: :personal_key, + controller: self, + next_steps: [FlowPolicy::FINAL], + preconditions: ->(idv_session:, user:) do + idv_session.phone_or_address_step_complete? && + user.active_or_pending_profile && + !idv_session.personal_key_acknowledged + end, + undo_step: ->(idv_session:, user:) { + idv_session.invalidate_personal_key! + }, + ) + end + private def next_step @@ -52,20 +72,6 @@ def next_step end end - def confirm_phone_or_address_confirmed - return if idv_session.address_confirmed? || idv_session.phone_confirmed? - - redirect_to idv_enter_password_url - end - - def confirm_personal_key_not_acknowledged - redirect_to next_step if idv_session.personal_key_acknowledged - end - - def confirm_profile_has_been_created - redirect_to account_url if profile.blank? - end - def add_proofing_component ProofingComponent.find_or_create_by(user: current_user).update(verified_at: Time.zone.now) end diff --git a/app/policies/idv/flow_policy.rb b/app/policies/idv/flow_policy.rb index 1a6f82787c2..36483d17e79 100644 --- a/app/policies/idv/flow_policy.rb +++ b/app/policies/idv/flow_policy.rb @@ -66,6 +66,7 @@ def steps otp_verification: Idv::OtpVerificationController.step_info, request_letter: Idv::ByMail::RequestLetterController.step_info, enter_password: Idv::EnterPasswordController.step_info, + personal_key: Idv::PersonalKeyController.step_info, } end diff --git a/spec/controllers/idv/personal_key_controller_spec.rb b/spec/controllers/idv/personal_key_controller_spec.rb index 7abf3a8b158..483e9405306 100644 --- a/spec/controllers/idv/personal_key_controller_spec.rb +++ b/spec/controllers/idv/personal_key_controller_spec.rb @@ -1,6 +1,7 @@ require 'rails_helper' RSpec.describe Idv::PersonalKeyController do + include FlowPolicyHelper include SamlAuthHelper include PersonalKeyValidator @@ -42,91 +43,198 @@ def assert_personal_key_generated_for_profiles(*profile_pii_pairs) let(:idv_session) { subject.idv_session } + let(:threatmetrix_review_status) { nil } + before do stub_analytics stub_attempts_tracker - stub_verify_steps_one_and_two(user, applicant: applicant) + + stub_sign_in(user) case address_verification_mechanism when 'phone' - idv_session.address_verification_mechanism = 'phone' - idv_session.user_phone_confirmation = true - idv_session.vendor_phone_confirmation = true + stub_up_to(:otp_verification, idv_session: idv_session) when 'gpo' - idv_session.address_verification_mechanism = 'gpo' - idv_session.user_phone_confirmation = false - idv_session.vendor_phone_confirmation = false + stub_up_to(:request_letter, idv_session: idv_session) + idv_session.gpo_code_verified = true + when nil + stub_up_to(:verify_info, idv_session: idv_session) else raise 'invalid address_verification_mechanism' end + idv_session.applicant = applicant + if mint_profile_from_idv_session idv_session.create_profile_from_applicant_with_password(password) end end + describe '#step_info' do + let(:step_info) do + controller.class.step_info + end + + describe '#undo_step' do + it 'clears personal_key_acknowledged' do + idv_session.acknowledge_personal_key! + step_info.undo_step.call(idv_session: idv_session, user: user) + expect(idv_session.personal_key_acknowledged).to eql(nil) + end + + it 'clears personal_key' do + idv_session.personal_key = 'ABCD-1234' + step_info.undo_step.call(idv_session: idv_session, user: user) + expect(idv_session.personal_key).to be_nil + end + end + + describe '#preconditions' do + let(:preconditions) do + step_info.preconditions.call(idv_session: idv_session, user: user) + end + + context 'when all conditions met' do + it 'returns a truthy result' do + expect(preconditions).to be_truthy + end + end + + context 'when user does not have a pending or active profile' do + before do + user.active_profile.deactivate(:password_reset) + expect(user.active_profile).to eql(nil) + user.reload + end + + it 'returns something falsey' do + expect(preconditions).to be_falsey + end + end + + context 'when address confirmed via GPO' do + let(:address_verification_mechanism) { 'gpo' } + it 'returns a truthy result' do + expect(preconditions).to be_truthy + end + end + + context 'when address confirmed via phone' do + let(:address_verification_mechanism) { 'phone' } + it 'returns a truthy result' do + expect(preconditions).to be_truthy + end + end + + context 'when address unconfirmed' do + let(:address_verification_mechanism) { nil } + it 'returns a falsey result' do + expect(preconditions).to be_falsey + end + end + + context 'when personal_key_acknowledged is false' do + before do + idv_session.personal_key_acknowledged = false + end + it 'returns a truthy result' do + expect(preconditions).to be_truthy + end + end + + context 'when personal_key_acknowledged is true' do + before do + idv_session.personal_key_acknowledged = true + end + it 'returns a falsey result' do + expect(preconditions).to be_falsey + end + end + + context 'when personal_key_acknowledged is nil' do + before do + idv_session.personal_key_acknowledged = nil + end + it 'returns a truthy result' do + expect(preconditions).to be_truthy + end + end + end + end + describe 'before_actions' do it 'includes before_actions' do expect(subject).to have_actions( :before, :confirm_two_factor_authenticated, - :confirm_phone_or_address_confirmed, + :confirm_step_allowed, + ) + end + + it 'skips redundant or irrelevant before_actions' do + expect(subject).not_to have_actions( + :before, + :confirm_idv_needed, + :confirm_personal_key_acknowledged_if_needed, + :confirm_no_pending_in_person_enrollment, + :handle_fraud, ) end it 'includes before_actions from IdvSession' do - expect(subject).to have_actions(:before, :redirect_unless_sp_requested_verification) + expect(subject).to have_actions( + :before, + :redirect_unless_sp_requested_verification, + ) end + end - describe '#confirm_profile_has_been_created' do - controller do - before_action :confirm_profile_has_been_created + describe '#show' do + context 'profile has been created from idv_session' do + it 'does not redirect' do + get :show - def index - render plain: 'Hello' - end + expect(response).to_not be_redirect end - context 'profile has been created' do + context 'profile is pending fraud review' do + let(:threatmetrix_review_status) { 'reject' } it 'does not redirect' do - get :index - + get :show expect(response).to_not be_redirect end end + end - context 'profile has not been created from idv_session' do - let(:mint_profile_from_idv_session) { false } + context 'profile has not been created from idv_session' do + let(:mint_profile_from_idv_session) { false } - it 'redirects to the account path' do - get :index - expect(response).to redirect_to account_path - end + it 'redirects to the enter password screen' do + get :show + expect(response).to redirect_to idv_enter_password_url + end - context 'profile is pending from a different session' do - context 'profile is pending due to fraud review' do - let!(:pending_profile) { create(:profile, :fraud_review_pending, user: user) } + context 'but a profile is pending from a different session' do + context 'due to fraud review' do + let!(:pending_profile) { create(:profile, :fraud_review_pending, user: user) } - it 'does not redirect' do - get :index - expect(response).to_not be_redirect - end + it 'does not redirect' do + get :show + expect(response).not_to be_redirect end + end - context 'profile is pending due to in person proofing' do - let!(:pending_profile) { create(:profile, :in_person_verification_pending, user: user) } + context 'due to in person proofing' do + let!(:pending_profile) { create(:profile, :in_person_verification_pending, user: user) } - it 'does not redirect' do - get :index - expect(response).to_not be_redirect - end + it 'does not redirect' do + get :show + expect(response).to_not be_redirect end end end end - end - describe '#show' do it 'sets code instance variable' do code = idv_session.personal_key expect(code).to be_present @@ -165,10 +273,10 @@ def index context 'user selected gpo verification' do let(:address_verification_mechanism) { 'gpo' } - it 'redirects to enter password url' do + it 'redirects to letter enqueued url' do get :show - expect(response).to redirect_to idv_enter_password_url + expect(response).to redirect_to idv_letter_enqueued_url end end @@ -266,6 +374,16 @@ def index end end end + + context 'personal key already acknowledged' do + before do + idv_session.acknowledge_personal_key! + end + it 'redirects away' do + get :show + expect(response).to be_redirect + end + end end describe '#update' do @@ -307,10 +425,14 @@ def index context 'user selected gpo verification' do let(:address_verification_mechanism) { 'gpo' } - it 'redirects to review url' do + it 'redirects to correct url' do patch :update + expect(response).to redirect_to idv_letter_enqueued_url + end - expect(response).to redirect_to idv_enter_password_url + it 'does not log any events' do + expect(@analytics).not_to have_logged_event + patch :update end end diff --git a/spec/policies/idv/flow_policy_spec.rb b/spec/policies/idv/flow_policy_spec.rb index 4c51192fc7d..cf0463d7cfd 100644 --- a/spec/policies/idv/flow_policy_spec.rb +++ b/spec/policies/idv/flow_policy_spec.rb @@ -294,7 +294,7 @@ expect(subject.info_for_latest_step.key).to eq(:enter_password) expect(subject.controller_allowed?(controller: Idv::EnterPasswordController)).to be - # expect(subject.controller_allowed?(controller: Idv::PersonalKeyController)).not_to be + expect(subject.controller_allowed?(controller: Idv::PersonalKeyController)).not_to be end end @@ -304,7 +304,31 @@ expect(subject.info_for_latest_step.key).to eq(:enter_password) expect(subject.controller_allowed?(controller: Idv::EnterPasswordController)).to be - # expect(subject.controller_allowed?(controller: Idv::PersonalKeyController)).not_to be + expect(subject.controller_allowed?(controller: Idv::PersonalKeyController)).not_to be + end + end + end + + context 'preconditions for personal_key are present' do + let(:password) { 'sekrit phrase' } + context 'user has a verify by mail pending profile' do + it 'returns personal_key' do + stub_up_to(:request_letter, idv_session: idv_session) + idv_session.gpo_code_verified = true + idv_session.create_profile_from_applicant_with_password('password') + + expect(subject.info_for_latest_step.key).to eq(:personal_key) + expect(subject.controller_allowed?(controller: Idv::PersonalKeyController)).to be + end + end + + context 'user has a newly activated profile' do + it 'returns personal_key' do + stub_up_to(:otp_verification, idv_session: idv_session) + idv_session.create_profile_from_applicant_with_password('password') + + expect(subject.info_for_latest_step.key).to eq(:personal_key) + expect(subject.controller_allowed?(controller: Idv::PersonalKeyController)).to be end end end diff --git a/spec/support/flow_policy_helper.rb b/spec/support/flow_policy_helper.rb index 6c413d9b20c..2f270caee17 100644 --- a/spec/support/flow_policy_helper.rb +++ b/spec/support/flow_policy_helper.rb @@ -35,6 +35,7 @@ def stub_step(key:, idv_session:) idv_session.applicant = Idp::Constants::MOCK_IDV_APPLICANT_WITH_SSN.dup when :phone idv_session.mark_phone_step_started! + idv_session.applicant = Idp::Constants::MOCK_IDV_APPLICANT_WITH_PHONE.dup when :otp_verification idv_session.mark_phone_step_complete! when :request_letter From 5a70cd1642d235ffa67b9ca489d803b15d7f3071 Mon Sep 17 00:00:00 2001 From: Brittany Greaner <35475380+night-jellyfish@users.noreply.github.com> Date: Wed, 20 Dec 2023 10:57:50 -0800 Subject: [PATCH 12/19] LG-11631: Add front end logging for selfie capture (#9795) * changelog: Internal, Doc Auth, Add front end logging for selfie capture This commit adds 5 logging events: 1. `idv_sdk_selfie_image_capture_opened` (**mobile only** - logs when a user opens the SDK for selfie capture) 2. `idv_sdk_selfie_image_capture_closed_without_photo` (**mobile only** - logs when a user starts to take a selfie with the SDK but exits out without adding one) 3. `idv_sdk_selfie_image_capture_failed` (**mobile only** - logs when an error happens in the process of adding the selfie via the SDK) 4. `idv_sdk_selfie_image_added` (**mobile only** - logs when a user uses the SDK to click the green checkmark and accept their selfie) 5. `idv_selfie_image_file_uploaded` (**desktop only** - logs when a user uses the file picker instead of the SDK to add a selfie) We may eventually want to send more metadata of the selfie image as well, but it is not currently available. So sending the metadata was determined out of scope, and looked at in future work. idv_selfie_image_file_uploaded is the only "selfie" event we can currently test on the backend, so that is why it's the only one included in `analytics_spec` for now. We may add a ticket for finding ways to test the other events on the backend, especially since we are unsure if file upload will stay as an option for selfie. Co-authored-by: Charles Ferguson --- app/controllers/frontend_log_controller.rb | 5 + .../components/acuant-capture.tsx | 31 +++- .../components/acuant-selfie-camera.tsx | 4 +- app/services/analytics_events.rb | 68 +++++++ spec/features/idv/analytics_spec.rb | 168 ++++++++++++++++++ .../components/acuant-capture-spec.jsx | 115 ++++++++++-- spec/javascript/support/document-capture.jsx | 4 +- 7 files changed, 378 insertions(+), 17 deletions(-) diff --git a/app/controllers/frontend_log_controller.rb b/app/controllers/frontend_log_controller.rb index 3e34430f7e2..849e113b869 100644 --- a/app/controllers/frontend_log_controller.rb +++ b/app/controllers/frontend_log_controller.rb @@ -47,6 +47,11 @@ class FrontendLogController < ApplicationController # rubocop:enable Layout/LineLength ALLOWED_EVENTS = %i[ + idv_sdk_selfie_image_added + idv_sdk_selfie_image_capture_closed_without_photo + idv_sdk_selfie_image_capture_failed + idv_sdk_selfie_image_capture_opened + idv_selfie_image_file_uploaded phone_input_country_changed ].freeze diff --git a/app/javascript/packages/document-capture/components/acuant-capture.tsx b/app/javascript/packages/document-capture/components/acuant-capture.tsx index 6bc249fb945..a26b4d6baea 100644 --- a/app/javascript/packages/document-capture/components/acuant-capture.tsx +++ b/app/javascript/packages/document-capture/components/acuant-capture.tsx @@ -409,7 +409,11 @@ function AcuantCapture( size: nextValue.size, failedImageResubmission: hasFailed, }); - trackEvent(`IdV: ${name} image added`, analyticsPayload); + + trackEvent( + name === 'selfie' ? 'idv_selfie_image_file_uploaded' : `IdV: ${name} image added`, + analyticsPayload, + ); } onChangeAndResetError(nextValue, analyticsPayload); @@ -498,13 +502,32 @@ function AcuantCapture( } } + function onSelfieCaptureOpen() { + trackEvent('idv_sdk_selfie_image_capture_opened'); + + setIsCapturingEnvironment(true); + } + + function onSelfieCaptureClosed() { + trackEvent('idv_sdk_selfie_image_capture_closed_without_photo'); + + setIsCapturingEnvironment(false); + } + function onSelfieCaptureSuccess({ image }: { image: string }) { + trackEvent('idv_sdk_selfie_image_added', { attempt }); + onChangeAndResetError(image); onResetFailedCaptureAttempts(); setIsCapturingEnvironment(false); } - function onSelfieCaptureFailure() { + function onSelfieCaptureFailure(error) { + trackEvent('idv_sdk_selfie_image_capture_failed', { + sdk_error_code: error.code, + sdk_error_message: error.message, + }); + // Internally, Acuant sets a cookie to bail on guided capture if initialization had // previously failed for any reason, including declined permission. Since the cookie // never expires, and since we want to re-prompt even if the user had previously @@ -653,8 +676,8 @@ function AcuantCapture( setIsCapturingEnvironment(true)} - onImageCaptureClose={() => setIsCapturingEnvironment(false)} + onImageCaptureOpen={onSelfieCaptureOpen} + onImageCaptureClose={onSelfieCaptureClosed} > void; /** * Capture open callback, tells the rest of the page * when the fullscreen selfie capture page is open @@ -100,7 +100,7 @@ function AcuantSelfieCamera({ onError: (error) => { // Error occurred. Camera permission not granted will // manifest here with 1 as error code. Unexpected errors will have 2 as error code. - onImageCaptureFailure({ error }); + onImageCaptureFailure(error); }, onPhotoTaken: () => { // The photo has been taken and it's showing a preview with a button to accept or retake the image. diff --git a/app/services/analytics_events.rb b/app/services/analytics_events.rb index 3559dd34b24..45b85cd8cf6 100644 --- a/app/services/analytics_events.rb +++ b/app/services/analytics_events.rb @@ -2729,6 +2729,74 @@ def idv_request_letter_visited( ) end + # @param [Integer] attempt number of attempts + # User captured and approved of their selfie + def idv_sdk_selfie_image_added(attempt:, **extra) + track_event(:idv_sdk_selfie_image_added, attempt: attempt, **extra) + end + + # User closed the SDK for taking a selfie without submitting a photo + def idv_sdk_selfie_image_capture_closed_without_photo(**extra) + track_event(:idv_sdk_selfie_image_capture_closed_without_photo, **extra) + end + + # @param [Integer] sdk_error_code SDK code for the error encountered + # @param [String] sdk_error_message SDK message for the error encountered + # User encountered an error with the SDK selfie process + # Error code 1: camera permission not granted + # Error code 2: unexpected errors + def idv_sdk_selfie_image_capture_failed(sdk_error_code:, sdk_error_message:, **extra) + track_event( + :idv_sdk_selfie_image_capture_failed, + sdk_error_code: sdk_error_code, + sdk_error_message: sdk_error_message, + **extra, + ) + end + + # User opened the SDK to take a selfie + def idv_sdk_selfie_image_capture_opened(**extra) + track_event(:idv_sdk_selfie_image_capture_opened, **extra) + end + + # @param [Integer] attempt number of attempts + # @param [Integer] failedImageResubmission + # @param [String] fingerprint fingerprint of the image added + # @param [String] flow_path whether the user is in the hybrid or standard flow + # @param [Integer] height height of image added in pixels + # @param [String] mimeType MIME type of image added + # @param [Integer] size size of image added in bytes + # @param [String] source + # @param [Integer] width width of image added in pixels + # User uploaded a selfie using the file picker + # rubocop:disable Naming/VariableName,Naming/MethodParameterName + def idv_selfie_image_file_uploaded( + attempt:, + failedImageResubmission:, + fingerprint:, + flow_path:, + height:, + mimeType:, + size:, + source:, + width:, + **_extra + ) + track_event( + :idv_selfie_image_file_uploaded, + attempt: attempt, + failedImageResubmission: failedImageResubmission, + fingerprint: fingerprint, + flow_path: flow_path, + height: height, + mimeType: mimeType, + size: size, + source: source, + width: width, + ) + end + # rubocop:enable Naming/VariableName,Naming/MethodParameterName + # Tracks when the user visits one of the the session error pages. # @param [String] type # @param [Integer,nil] attempts_remaining diff --git a/spec/features/idv/analytics_spec.rb b/spec/features/idv/analytics_spec.rb index fbd2d165fa5..78e797a19cd 100644 --- a/spec/features/idv/analytics_spec.rb +++ b/spec/features/idv/analytics_spec.rb @@ -465,6 +465,117 @@ 'IdV: user clicked sp link on ready to verify page' => {}, } end + + let(:happy_selfie_path_events) do + { + 'IdV: intro visited' => {}, + 'IdV: doc auth welcome visited' => { + step: 'welcome', analytics_id: 'Doc Auth', irs_reproofing: false, skip_hybrid_handoff: nil, lexisnexis_instant_verify_workflow_ab_test_bucket: :default + }, + 'IdV: doc auth welcome submitted' => { + step: 'welcome', analytics_id: 'Doc Auth', irs_reproofing: false, skip_hybrid_handoff: nil, lexisnexis_instant_verify_workflow_ab_test_bucket: :default + }, + 'IdV: doc auth agreement visited' => { + step: 'agreement', analytics_id: 'Doc Auth', skip_hybrid_handoff: nil, irs_reproofing: false, acuant_sdk_upgrade_ab_test_bucket: :default, lexisnexis_instant_verify_workflow_ab_test_bucket: :default + }, + 'IdV: consent checkbox toggled' => { + checked: true, + }, + 'IdV: doc auth agreement submitted' => { + success: true, errors: {}, step: 'agreement', analytics_id: 'Doc Auth', skip_hybrid_handoff: nil, irs_reproofing: false, acuant_sdk_upgrade_ab_test_bucket: :default, lexisnexis_instant_verify_workflow_ab_test_bucket: :default + }, + 'IdV: doc auth hybrid handoff visited' => { + step: 'hybrid_handoff', redo_document_capture: nil, acuant_sdk_upgrade_ab_test_bucket: :default, lexisnexis_instant_verify_workflow_ab_test_bucket: :default, analytics_id: 'Doc Auth', skip_hybrid_handoff: nil, irs_reproofing: false + }, + 'IdV: doc auth hybrid handoff submitted' => { + success: true, errors: {}, destination: :document_capture, flow_path: 'standard', step: 'hybrid_handoff', redo_document_capture: nil, acuant_sdk_upgrade_ab_test_bucket: :default, lexisnexis_instant_verify_workflow_ab_test_bucket: :default, analytics_id: 'Doc Auth', skip_hybrid_handoff: nil, irs_reproofing: false + }, + 'IdV: doc auth document_capture visited' => { + flow_path: 'standard', step: 'document_capture', redo_document_capture: nil, skip_hybrid_handoff: nil, acuant_sdk_upgrade_ab_test_bucket: :default, lexisnexis_instant_verify_workflow_ab_test_bucket: :default, analytics_id: 'Doc Auth', irs_reproofing: false + }, + 'Frontend: IdV: front image added' => { + width: 284, height: 38, mimeType: 'image/png', source: 'upload', size: 3694, attempt: 1, flow_path: 'standard', acuant_sdk_upgrade_a_b_testing_enabled: 'false', use_alternate_sdk: anything, acuant_version: anything, acuantCaptureMode: nil, fingerprint: anything, failedImageResubmission: boolean, documentType: nil, dpi: nil, glare: nil, glareScoreThreshold: nil, isAssessedAsBlurry: nil, isAssessedAsGlare: nil, isAssessedAsUnsupported: nil, moire: nil, sharpness: nil, sharpnessScoreThreshold: nil, assessment: nil + }, + 'Frontend: IdV: back image added' => { + width: 284, height: 38, mimeType: 'image/png', source: 'upload', size: 3694, attempt: 1, flow_path: 'standard', acuant_sdk_upgrade_a_b_testing_enabled: 'false', use_alternate_sdk: anything, acuant_version: anything, acuantCaptureMode: nil, fingerprint: anything, failedImageResubmission: boolean, documentType: nil, dpi: nil, glare: nil, glareScoreThreshold: nil, isAssessedAsBlurry: nil, isAssessedAsGlare: nil, isAssessedAsUnsupported: nil, moire: nil, sharpness: nil, sharpnessScoreThreshold: nil, assessment: nil + }, + 'IdV: doc auth image upload form submitted' => { + success: true, errors: {}, attempts: 1, remaining_attempts: 3, user_id: user.uuid, flow_path: 'standard', front_image_fingerprint: an_instance_of(String), back_image_fingerprint: an_instance_of(String) + }, + 'IdV: doc auth image upload vendor pii validation' => { + success: true, errors: {}, user_id: user.uuid, attempts: 1, remaining_attempts: 3, flow_path: 'standard', attention_with_barcode: false, front_image_fingerprint: an_instance_of(String), back_image_fingerprint: an_instance_of(String), classification_info: {} + }, + 'IdV: doc auth document_capture submitted' => { + success: true, errors: {}, flow_path: 'standard', step: 'document_capture', redo_document_capture: nil, skip_hybrid_handoff: nil, acuant_sdk_upgrade_ab_test_bucket: :default, lexisnexis_instant_verify_workflow_ab_test_bucket: :default, analytics_id: 'Doc Auth', irs_reproofing: false + }, + :idv_selfie_image_file_uploaded => { + attempt: 1, failedImageResubmission: nil, fingerprint: 'aIzxkX_iMtoxFOURZr55qkshs53emQKUOr7VfTf6G1Q', flow_path: 'standard', height: 38, mimeType: 'image/png', size: 3694, source: 'upload', width: 284 + }, + 'IdV: doc auth ssn visited' => { + flow_path: 'standard', step: 'ssn', acuant_sdk_upgrade_ab_test_bucket: :default, lexisnexis_instant_verify_workflow_ab_test_bucket: :default, skip_hybrid_handoff: nil, analytics_id: 'Doc Auth', irs_reproofing: false + }, + 'IdV: doc auth ssn submitted' => { + success: true, errors: {}, flow_path: 'standard', step: 'ssn', acuant_sdk_upgrade_ab_test_bucket: :default, lexisnexis_instant_verify_workflow_ab_test_bucket: :default, skip_hybrid_handoff: nil, analytics_id: 'Doc Auth', irs_reproofing: false + }, + 'IdV: doc auth verify visited' => { + flow_path: 'standard', step: 'verify', acuant_sdk_upgrade_ab_test_bucket: :default, lexisnexis_instant_verify_workflow_ab_test_bucket: :default, skip_hybrid_handoff: nil, analytics_id: 'Doc Auth', irs_reproofing: false + }, + 'IdV: doc auth verify submitted' => { + flow_path: 'standard', step: 'verify', acuant_sdk_upgrade_ab_test_bucket: :default, lexisnexis_instant_verify_workflow_ab_test_bucket: :default, skip_hybrid_handoff: nil, analytics_id: 'Doc Auth', irs_reproofing: false + }, + 'IdV: doc auth verify proofing results' => { + success: true, errors: {}, flow_path: 'standard', address_edited: false, address_line2_present: false, analytics_id: 'Doc Auth', ssn_is_unique: true, step: 'verify', acuant_sdk_upgrade_ab_test_bucket: :default, lexisnexis_instant_verify_workflow_ab_test_bucket: :default, irs_reproofing: false, skip_hybrid_handoff: nil, + proofing_results: { exception: nil, timed_out: false, threatmetrix_review_status: 'pass', context: { device_profiling_adjudication_reason: 'device_profiling_result_pass', resolution_adjudication_reason: 'pass_resolution_and_state_id', should_proof_state_id: true, stages: { resolution: { success: true, errors: {}, exception: nil, timed_out: false, transaction_id: 'resolution-mock-transaction-id-123', reference: 'aaa-bbb-ccc', can_pass_with_additional_verification: false, attributes_requiring_additional_verification: [], vendor_name: 'ResolutionMock', vendor_workflow: nil }, residential_address: { attributes_requiring_additional_verification: [], can_pass_with_additional_verification: false, errors: {}, exception: nil, reference: '', success: true, timed_out: false, transaction_id: '', vendor_name: 'ResidentialAddressNotRequired', vendor_workflow: nil }, state_id: { success: true, errors: {}, exception: nil, mva_exception: nil, timed_out: false, transaction_id: 'state-id-mock-transaction-id-456', vendor_name: 'StateIdMock', verified_attributes: [], state: 'MT', state_id_jurisdiction: 'ND', state_id_number: '#############' }, threatmetrix: threatmetrix_response } } } + }, + 'IdV: phone of record visited' => { + acuant_sdk_upgrade_ab_test_bucket: :default, lexisnexis_instant_verify_workflow_ab_test_bucket: :default, skip_hybrid_handoff: nil, + proofing_components: { document_check: 'mock', document_type: 'state_id', source_check: 'aamva', resolution_check: 'lexis_nexis', threatmetrix: threatmetrix, threatmetrix_review_status: 'pass' } + }, + 'IdV: phone confirmation form' => { + success: true, errors: {}, phone_type: :mobile, types: [:fixed_or_mobile], carrier: 'Test Mobile Carrier', country_code: 'US', area_code: '202', acuant_sdk_upgrade_ab_test_bucket: :default, lexisnexis_instant_verify_workflow_ab_test_bucket: :default, skip_hybrid_handoff: nil, otp_delivery_preference: 'sms', + proofing_components: { document_check: 'mock', document_type: 'state_id', source_check: 'aamva', resolution_check: 'lexis_nexis', threatmetrix: threatmetrix, threatmetrix_review_status: 'pass' } + }, + 'IdV: phone confirmation vendor' => { + success: true, errors: {}, vendor: { exception: nil, vendor_name: 'AddressMock', transaction_id: 'address-mock-transaction-id-123', timed_out: false, reference: '' }, new_phone_added: false, hybrid_handoff_phone_used: false, area_code: '202', country_code: 'US', phone_fingerprint: anything, + proofing_components: { document_check: 'mock', document_type: 'state_id', source_check: 'aamva', resolution_check: 'lexis_nexis', threatmetrix: threatmetrix, threatmetrix_review_status: 'pass', address_check: 'lexis_nexis_address' } + }, + 'IdV: phone confirmation otp sent' => { + success: true, otp_delivery_preference: :sms, country_code: 'US', area_code: '202', adapter: :test, errors: {}, phone_fingerprint: anything, rate_limit_exceeded: false, telephony_response: anything, + proofing_components: { document_check: 'mock', document_type: 'state_id', source_check: 'aamva', resolution_check: 'lexis_nexis', threatmetrix: threatmetrix, threatmetrix_review_status: 'pass', address_check: 'lexis_nexis_address' } + }, + 'IdV: phone confirmation otp visited' => { + proofing_components: { document_check: 'mock', document_type: 'state_id', source_check: 'aamva', resolution_check: 'lexis_nexis', threatmetrix: threatmetrix, threatmetrix_review_status: 'pass', address_check: 'lexis_nexis_address' }, + }, + 'IdV: phone confirmation otp submitted' => { + success: true, code_expired: false, code_matches: true, second_factor_attempts_count: 0, second_factor_locked_at: nil, errors: {}, + proofing_components: { document_check: 'mock', document_type: 'state_id', source_check: 'aamva', resolution_check: 'lexis_nexis', threatmetrix: threatmetrix, threatmetrix_review_status: 'pass', address_check: 'lexis_nexis_address' } + }, + :idv_enter_password_visited => { + address_verification_method: 'phone', acuant_sdk_upgrade_ab_test_bucket: :default, lexisnexis_instant_verify_workflow_ab_test_bucket: :default, skip_hybrid_handoff: nil, + proofing_components: { document_check: 'mock', document_type: 'state_id', source_check: 'aamva', resolution_check: 'lexis_nexis', threatmetrix: threatmetrix, threatmetrix_review_status: 'pass', address_check: 'lexis_nexis_address' } + }, + :idv_enter_password_submitted => { + success: true, acuant_sdk_upgrade_ab_test_bucket: :default, lexisnexis_instant_verify_workflow_ab_test_bucket: :default, skip_hybrid_handoff: nil, fraud_review_pending: false, fraud_rejection: false, gpo_verification_pending: false, in_person_verification_pending: false, deactivation_reason: nil, + proofing_components: { document_check: 'mock', document_type: 'state_id', source_check: 'aamva', resolution_check: 'lexis_nexis', threatmetrix: threatmetrix, threatmetrix_review_status: 'pass', address_check: 'lexis_nexis_address' } + }, + 'IdV: final resolution' => { + success: true, acuant_sdk_upgrade_ab_test_bucket: :default, lexisnexis_instant_verify_workflow_ab_test_bucket: :default, skip_hybrid_handoff: nil, fraud_review_pending: false, fraud_rejection: false, gpo_verification_pending: false, in_person_verification_pending: false, deactivation_reason: nil, + proofing_components: { document_check: 'mock', document_type: 'state_id', source_check: 'aamva', resolution_check: 'lexis_nexis', threatmetrix: threatmetrix, threatmetrix_review_status: 'pass', address_check: 'lexis_nexis_address' } + }, + 'IdV: personal key visited' => { + address_verification_method: 'phone', in_person_verification_pending: false, + proofing_components: { document_check: 'mock', document_type: 'state_id', source_check: 'aamva', resolution_check: 'lexis_nexis', threatmetrix: threatmetrix, threatmetrix_review_status: 'pass', address_check: 'lexis_nexis_address' } + }, + 'IdV: personal key acknowledgment toggled' => { + checked: true, + proofing_components: { document_check: 'mock', document_type: 'state_id', source_check: 'aamva', resolution_check: 'lexis_nexis', threatmetrix: threatmetrix, threatmetrix_review_status: 'pass', address_check: 'lexis_nexis_address' }, + }, + 'IdV: personal key submitted' => { + address_verification_method: 'phone', fraud_review_pending: false, fraud_rejection: false, in_person_verification_pending: false, deactivation_reason: nil, + proofing_components: { document_check: 'mock', document_type: 'state_id', source_check: 'aamva', resolution_check: 'lexis_nexis', threatmetrix: threatmetrix, threatmetrix_review_status: 'pass', address_check: 'lexis_nexis_address' } + }, + } + end # rubocop:enable Layout/LineLength # rubocop:enable Layout/MultilineHashKeyLineBreaks @@ -730,4 +841,61 @@ def wait_for_event(event, wait) end end end + + context 'Happy selfie path' do + before do + allow(IdentityConfig.store).to receive(:doc_auth_selfie_capture_enabled).and_return(true) + + mobile_device = Browser.new(mobile_user_agent) + allow(BrowserCache).to receive(:parse).and_return(mobile_device) + + perform_in_browser(:mobile) do + sign_in_and_2fa_user(user) + visit_idp_from_sp_with_ial2(:oidc) + complete_doc_auth_steps_before_document_capture_step + + attach_images + attach_selfie + submit_images + + click_idv_continue + visit idv_ssn_url + complete_ssn_step + complete_verify_step + fill_out_phone_form_ok('202-555-1212') + verify_phone_otp + complete_enter_password_step(user) + acknowledge_and_confirm_personal_key + end + end + + it 'records all of the events' do + happy_selfie_path_events.each do |event, attributes| + expect(fake_analytics).to have_logged_event(event, attributes) + end + end + + context 'proofing_device_profiling disabled' do + let(:proofing_device_profiling) { :disabled } + let(:threatmetrix) { false } + let(:threatmetrix_response) do + { client: 'tmx_disabled', + success: true, + errors: {}, + exception: nil, + timed_out: false, + transaction_id: nil, + review_status: 'pass', + response_body: { error: 'TMx response body was empty' } } + end + + it 'records all of the events' do + aggregate_failures 'analytics events' do + happy_selfie_path_events.each do |event, attributes| + expect(fake_analytics).to have_logged_event(event, attributes) + end + end + end + end + end end diff --git a/spec/javascript/packages/document-capture/components/acuant-capture-spec.jsx b/spec/javascript/packages/document-capture/components/acuant-capture-spec.jsx index a542fd4c00f..a9b18500dae 100644 --- a/spec/javascript/packages/document-capture/components/acuant-capture-spec.jsx +++ b/spec/javascript/packages/document-capture/components/acuant-capture-spec.jsx @@ -9,7 +9,7 @@ import { AnalyticsContext, FailedCaptureAttemptsContextProvider, } from '@18f/identity-document-capture'; -import { createEvent, waitFor } from '@testing-library/dom'; +import { createEvent, waitFor, screen } from '@testing-library/dom'; import DeviceContext from '@18f/identity-document-capture/context/device'; import { I18n } from '@18f/identity-i18n'; @@ -1114,21 +1114,116 @@ describe('document-capture/components/acuant-capture', () => { }); context('mobile selfie', () => { - it('renders the selfie capture loading div in acuant-capture', async () => { + const trackEvent = sinon.stub(); + + beforeEach(async () => { + // Set up the components so that everything is as it would actually be -except- the AcuantSDK + // The AcuantSDK isn't possible to run in test, so the initialize({...}) call below mocks it. + render( + + + + + + + , + ); + + // Simulate the user clicking on the box that usually opens full screen selfie capture. + // This isn't strictly necessary for the logging tests, but doing this makes the calls to + // trackEvent appear in the actual order we'd expect when using the Acuant SDK. + await userEvent.click(screen.getByLabelText('Image')); + }); + + it('renders the selfie capture loading div in acuant-capture', () => { // What we want to test is that the selfie version of the FileInput appears // when the name="selfie". The only difference between the selfie and document // versions is what happens when you click the FileInput, so this test clicks // the file input, then checks that the full screen div opened - const { getByRole, getByLabelText } = render( - - - - - , + expect(screen.getByRole('dialog')).to.be.ok(); + }); + + it('calls trackEvent from onSelfieCaptureOpen', () => { + // In real use the `start` method opens the Acuant SDK full screen selfie capture window. + // Because we can't do that in test (AcuantSDK does not allow), this doesn't attempt to load + // the SDK. Instead, it simply calls the callback that happens when a photo is captured. + // This allows us to test everything about that callback -except- the Acuant SDK parts. + initialize({ + selfieStart: sinon.stub().callsFake((callbacks) => { + callbacks.onOpened(); + }), + }); + + expect(trackEvent).to.be.calledWith('IdV: selfie image clicked'); + expect(trackEvent).to.be.calledWith('IdV: Acuant SDK loaded'); + + expect(trackEvent).to.have.been.calledWith('idv_sdk_selfie_image_capture_opened'); + }); + + it('calls trackEvent from onSelfieCaptureClosed', () => { + // In real use the `start` method opens the Acuant SDK full screen selfie capture window. + // Because we can't do that in test (AcuantSDK does not allow), this doesn't attempt to load + // the SDK. Instead, it simply calls the callback that happens when a photo is captured. + // This allows us to test everything about that callback -except- the Acuant SDK parts. + initialize({ + selfieStart: sinon.stub().callsFake((callbacks) => { + callbacks.onClosed(); + }), + }); + + expect(trackEvent).to.be.calledWith('IdV: selfie image clicked'); + expect(trackEvent).to.be.calledWith('IdV: Acuant SDK loaded'); + + expect(trackEvent).to.have.been.calledWith( + 'idv_sdk_selfie_image_capture_closed_without_photo', ); + }); - await userEvent.click(getByLabelText('Image')); - expect(getByRole('dialog')).to.be.ok(); + it('calls trackEvent from onSelfieCaptureSuccess', () => { + // In real use the `start` method opens the Acuant SDK full screen selfie capture window. + // Because we can't do that in test (AcuantSDK does not allow), this doesn't attempt to load + // the SDK. Instead, it simply calls the callback that happens when a photo is captured. + // This allows us to test everything about that callback -except- the Acuant SDK parts. + initialize({ + selfieStart: sinon.stub().callsFake((callbacks) => { + callbacks.onCaptured(); + }), + }); + + expect(trackEvent).to.be.calledWith('IdV: selfie image clicked'); + expect(trackEvent).to.be.calledWith('IdV: Acuant SDK loaded'); + + expect(trackEvent).to.have.been.calledWith( + 'idv_sdk_selfie_image_added', + sinon.match({ + attempt: sinon.match.number, + }), + ); + }); + + it('calls trackEvent from onSelfieCaptureFailure', () => { + const errorHash = { code: 1, message: 'Camera permission not granted' }; + + // In real use the `start` method opens the Acuant SDK full screen selfie capture window. + // Because we can't do that in test (AcuantSDK does not allow), this doesn't attempt to load + // the SDK. Instead, it simply calls the callback that happens when a photo is captured. + // This allows us to test everything about that callback -except- the Acuant SDK parts. + initialize({ + selfieStart: sinon.stub().callsFake((callbacks) => { + callbacks.onError(errorHash); + }), + }); + + expect(trackEvent).to.be.calledWith('IdV: selfie image clicked'); + expect(trackEvent).to.be.calledWith('IdV: Acuant SDK loaded'); + + expect(trackEvent).to.have.been.calledWith( + 'idv_sdk_selfie_image_capture_failed', + sinon.match({ + sdk_error_code: sinon.match.number, + sdk_error_message: sinon.match.string, + }), + ); }); }); diff --git a/spec/javascript/support/document-capture.jsx b/spec/javascript/support/document-capture.jsx index 24dea01116a..5259b26eded 100644 --- a/spec/javascript/support/document-capture.jsx +++ b/spec/javascript/support/document-capture.jsx @@ -70,6 +70,8 @@ export function useAcuant() { isCameraSupported = true, start = sinon.stub(), end = sinon.stub(), + selfieStart = sinon.stub(), + selfieEnd = sinon.stub(), triggerCapture = sinon.stub(), } = {}) { window.AcuantJavascriptWebSdk = { @@ -92,7 +94,7 @@ export function useAcuant() { }), end, }; - window.AcuantPassiveLiveness = { start: sinon.stub(), end: sinon.stub() }; + window.AcuantPassiveLiveness = { start: selfieStart, end: selfieEnd }; window.loadAcuantSdk = () => {}; const sdkScript = document.querySelector('[data-acuant-sdk]'); sdkScript.onload(); From 6df56e5d92cd5ef90a14d755c3bb52f5a1945c67 Mon Sep 17 00:00:00 2001 From: Matt Hinz Date: Wed, 20 Dec 2023 12:12:45 -0800 Subject: [PATCH 13/19] Remove unused Profile::includes_phone_check? method (#9815) Not referenced anywhere but its own specs. [skip changelog] --- app/models/profile.rb | 5 ----- spec/models/profile_spec.rb | 20 -------------------- 2 files changed, 25 deletions(-) diff --git a/app/models/profile.rb b/app/models/profile.rb index fc1fcd6e30f..7c439e11054 100644 --- a/app/models/profile.rb +++ b/app/models/profile.rb @@ -279,11 +279,6 @@ def self.build_compound_pii(pii) values.join(':') end - def includes_phone_check? - return false if proofing_components.blank? - proofing_components['address_check'] == 'lexis_nexis_address' - end - def irs_attempts_api_tracker @irs_attempts_api_tracker ||= IrsAttemptsApi::Tracker.new end diff --git a/spec/models/profile_spec.rb b/spec/models/profile_spec.rb index 95d4e833a09..8e7f9f5c946 100644 --- a/spec/models/profile_spec.rb +++ b/spec/models/profile_spec.rb @@ -46,26 +46,6 @@ end end - describe '#includes_phone_check?' do - it 'returns true if the address_check component is lexis_nexis_address' do - profile = create(:profile, proofing_components: { address_check: 'lexis_nexis_address' }) - - expect(profile.includes_phone_check?).to eq(true) - end - - it 'returns false if the address_check componet is gpo_letter' do - profile = create(:profile, proofing_components: { address_check: 'gpo_letter' }) - - expect(profile.includes_phone_check?).to eq(false) - end - - it 'returns false if proofing_components is blank' do - profile = create(:profile, proofing_components: '') - - expect(profile.includes_phone_check?).to eq(false) - end - end - describe '#in_person_verification_pending?' do it 'returns true if the in_person_verification_pending_at is present' do profile = create( From c6038836e9ec973cba5cc64cfffcdc1913032fb8 Mon Sep 17 00:00:00 2001 From: John Maxwell Date: Wed, 20 Dec 2023 15:13:51 -0500 Subject: [PATCH 14/19] LG-11743 - Ensure personal key works for GPO users (#9791) * LG-11743 - Personal key doesn't work for GPO users Spec to check profile reactivation for GPO-verified users. This is a bug reported by and believed fixed by Matt Hinz (see LG-11549), but we wanted a feature spec to test this exact scenario. changelog: Internal,Feature Specs,Added a feature spec for an issue in LG-11549 Co-authored by: Matt Hinz --- .../profile_recovery_for_gpo_verified_spec.rb | 56 +++++++++++++++++++ 1 file changed, 56 insertions(+) create mode 100644 spec/features/users/profile_recovery_for_gpo_verified_spec.rb diff --git a/spec/features/users/profile_recovery_for_gpo_verified_spec.rb b/spec/features/users/profile_recovery_for_gpo_verified_spec.rb new file mode 100644 index 00000000000..d260e4e0d50 --- /dev/null +++ b/spec/features/users/profile_recovery_for_gpo_verified_spec.rb @@ -0,0 +1,56 @@ +require 'rails_helper' + +RSpec.feature 'Password recovery via personal key for a GPO-verified user' do + include IdvStepHelper + + let(:email) { 'cool_beagle@example.org' } + let(:password) { '!1a Z@6s' * 16 } # default password from user factory + let(:new_password) { 'some really awesome new password' } + + let(:user) { create(:user, :fully_registered, email: email, password: password) } + + before do + allow(FeatureManagement).to receive(:reveal_gpo_code?).and_return(true) + end + + scenario 'lets them reactivate their profile with their personal key', email: true, js: true do + complete_idv_steps_with_gpo_before_confirmation_step(user) + click_on t('doc_auth.buttons.continue') + + gpo_code = page.get_rack_session_key('last_gpo_confirmation_code') + page.go_back # get_rack_session_key navigates away. + + click_on t('links.sign_out') + + fill_in_credentials_and_submit(email, password) + fill_in I18n.t('components.one_time_code_input.label'), with: last_phone_otp + click_submit_default + + fill_in 'gpo_verify_form_otp', with: gpo_code + click_on t('idv.gpo.form.submit') + + personal_key = scrape_personal_key + check t('forms.personal_key.required_checkbox') + click_continue + + click_on t('links.sign_out') + + trigger_reset_password_and_click_email_link(user.email) + reset_password_and_sign_back_in(user, new_password) + fill_in_code_with_last_phone_otp + click_submit_default + + click_on t('links.account.reactivate.with_key') + + expect(current_path).to eq verify_personal_key_path + fill_in 'personal_key', with: personal_key + click_continue + + expect(current_path).to eq verify_password_path + fill_in 'Password', with: new_password + click_continue + + expect(page).to have_content t('idv.messages.personal_key') + expect(page).to have_content t('headings.account.verified_account') + end +end From 01d619265a28558f90ae264cf536f532e0d24e61 Mon Sep 17 00:00:00 2001 From: Brittany Greaner <35475380+night-jellyfish@users.noreply.github.com> Date: Wed, 20 Dec 2023 12:55:19 -0800 Subject: [PATCH 15/19] Fix analytics spec failures (#9816) * Fix analytics_spec failures Unsure why these passed so many times before merging, but perhaps the interface changed since last rebase to now. * changelog: Internal, Doc Auth, Fix analytics_spec failures Edit: it is indeed because I did not rebase before merging. [See this comment for details](https://github.com/18F/identity-idp/pull/9816#issuecomment-1865117300). --- spec/features/idv/analytics_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/features/idv/analytics_spec.rb b/spec/features/idv/analytics_spec.rb index 78e797a19cd..302d6814ad3 100644 --- a/spec/features/idv/analytics_spec.rb +++ b/spec/features/idv/analytics_spec.rb @@ -547,7 +547,7 @@ proofing_components: { document_check: 'mock', document_type: 'state_id', source_check: 'aamva', resolution_check: 'lexis_nexis', threatmetrix: threatmetrix, threatmetrix_review_status: 'pass', address_check: 'lexis_nexis_address' }, }, 'IdV: phone confirmation otp submitted' => { - success: true, code_expired: false, code_matches: true, second_factor_attempts_count: 0, second_factor_locked_at: nil, errors: {}, + success: true, acuant_sdk_upgrade_ab_test_bucket: :default, lexisnexis_instant_verify_workflow_ab_test_bucket: :default, skip_hybrid_handoff: nil, code_expired: false, code_matches: true, second_factor_attempts_count: 0, second_factor_locked_at: nil, errors: {}, proofing_components: { document_check: 'mock', document_type: 'state_id', source_check: 'aamva', resolution_check: 'lexis_nexis', threatmetrix: threatmetrix, threatmetrix_review_status: 'pass', address_check: 'lexis_nexis_address' } }, :idv_enter_password_visited => { From c2180f3737bd4d2197f9ce1fe1e097e3e99d6d34 Mon Sep 17 00:00:00 2001 From: Sonia Connolly Date: Wed, 20 Dec 2023 13:22:42 -0800 Subject: [PATCH 16/19] Add InPerson::AddressController to FlowPolicy (#9794) * Add FlowPolicy to InPerson::AddressController * Give :pii_from_user a full value in flow_policy_spec Setting :pii_from_user in flow_policy_spec is a temporary workaround while the in person flow still has steps in the Flow State Machine. * Add clear_future_steps_from! which takes a controller argument * Remove unneeded let's from in person address_controller_spec [skip changelog] --- app/controllers/concerns/idv_step_concern.rb | 6 +- .../idv/in_person/address_controller.rb | 21 ++++++ app/policies/idv/flow_policy.rb | 1 + app/services/idv/session.rb | 8 ++- .../idv/in_person/address_controller_spec.rb | 65 ++++++++++--------- spec/policies/idv/flow_policy_spec.rb | 4 +- 6 files changed, 72 insertions(+), 33 deletions(-) diff --git a/app/controllers/concerns/idv_step_concern.rb b/app/controllers/concerns/idv_step_concern.rb index 537b8cb33be..7701aca863f 100644 --- a/app/controllers/concerns/idv_step_concern.rb +++ b/app/controllers/concerns/idv_step_concern.rb @@ -119,6 +119,10 @@ def url_for_latest_step end def clear_future_steps! - flow_policy.undo_future_steps_from_controller!(controller: self.class) + clear_future_steps_from!(controller: self.class) + end + + def clear_future_steps_from!(controller:) + flow_policy.undo_future_steps_from_controller!(controller: controller) end end diff --git a/app/controllers/idv/in_person/address_controller.rb b/app/controllers/idv/in_person/address_controller.rb index 506b2614246..8d143bea89a 100644 --- a/app/controllers/idv/in_person/address_controller.rb +++ b/app/controllers/idv/in_person/address_controller.rb @@ -6,6 +6,7 @@ class AddressController < ApplicationController before_action :render_404_if_in_person_residential_address_controller_enabled_not_set before_action :confirm_in_person_state_id_step_complete + ## before_action :confirm_step_allowed # pending FSM removal of state id step before_action :confirm_in_person_address_step_needed, only: :show def show @@ -15,6 +16,8 @@ def show end def update + # don't clear the ssn when updating address, clear after SsnController + clear_future_steps_from!(controller: Idv::InPerson::SsnController) attrs = Idv::InPerson::AddressForm::ATTRIBUTES.difference([:same_address_as_id]) pii_from_user[:same_address_as_id] = 'false' if updating_address? form_result = form.submit(flow_params) @@ -42,6 +45,24 @@ def extra_view_variables } end + # update Idv::DocumentCaptureController.step_info.next_steps to include + # :ipp_address instead of :ipp_ssn in delete PR + def self.step_info + Idv::StepInfo.new( + key: :ipp_address, + controller: self, + next_steps: [:ipp_ssn], + preconditions: ->(idv_session:, user:) { idv_session.ipp_state_id_complete? }, + undo_step: ->(idv_session:, user:) do + flow_session[:pii_from_user][:address1] = nil + flow_session[:pii_from_user][:address2] = nil + flow_session[:pii_from_user][:city] = nil + flow_session[:pii_from_user][:zipcode] = nil + flow_session[:pii_from_user][:state] = nil + end, + ) + end + private def flow_session diff --git a/app/policies/idv/flow_policy.rb b/app/policies/idv/flow_policy.rb index 36483d17e79..c11c1374a9a 100644 --- a/app/policies/idv/flow_policy.rb +++ b/app/policies/idv/flow_policy.rb @@ -56,6 +56,7 @@ def steps hybrid_handoff: Idv::HybridHandoffController.step_info, link_sent: Idv::LinkSentController.step_info, document_capture: Idv::DocumentCaptureController.step_info, + ipp_address: Idv::InPerson::AddressController.step_info, ssn: Idv::SsnController.step_info, ipp_ssn: Idv::InPerson::SsnController.step_info, verify_info: Idv::VerifyInfoController.step_info, diff --git a/app/services/idv/session.rb b/app/services/idv/session.rb index 0a4d56c2798..ec9fc3380e9 100644 --- a/app/services/idv/session.rb +++ b/app/services/idv/session.rb @@ -183,7 +183,13 @@ def remote_document_capture_complete? end def ipp_document_capture_complete? - has_pii_from_user_in_flow_session + has_pii_from_user_in_flow_session && + user_session['idv/in_person'][:pii_from_user].has_key?(:address1) + end + + def ipp_state_id_complete? + has_pii_from_user_in_flow_session && + user_session['idv/in_person'][:pii_from_user].has_key?(:identity_doc_address1) end def verify_info_step_complete? diff --git a/spec/controllers/idv/in_person/address_controller_spec.rb b/spec/controllers/idv/in_person/address_controller_spec.rb index 9f50201c762..086d7af1491 100644 --- a/spec/controllers/idv/in_person/address_controller_spec.rb +++ b/spec/controllers/idv/in_person/address_controller_spec.rb @@ -1,31 +1,32 @@ require 'rails_helper' RSpec.describe Idv::InPerson::AddressController do + include FlowPolicyHelper include InPersonHelper - let(:pii_from_user) { Idp::Constants::MOCK_IPP_APPLICANT_SAME_ADDRESS_AS_ID_FALSE.dup } let(:user) { build(:user) } - let(:flow_session) do - { pii_from_user: pii_from_user } - end - let(:ssn) { nil } before do allow(IdentityConfig.store).to receive(:in_person_residential_address_controller_enabled). and_return(true) allow(IdentityConfig.store).to receive(:usps_ipp_transliteration_enabled). and_return(true) - allow(subject).to receive(:current_user). - and_return(user) - allow(subject).to receive(:pii_from_user).and_return(pii_from_user) - allow(subject).to receive(:flow_session).and_return(flow_session) stub_sign_in(user) - subject.idv_session.flow_path = 'standard' - subject.idv_session.ssn = ssn + stub_up_to(:hybrid_handoff, idv_session: subject.idv_session) + subject.user_session['idv/in_person'] = { + pii_from_user: Idp::Constants::MOCK_IPP_APPLICANT_SAME_ADDRESS_AS_ID_FALSE.dup, + } + subject.idv_session.ssn = nil stub_analytics allow(@analytics).to receive(:track_event) end + describe '#step_info' do + it 'returns a valid StepInfo object' do + expect(Idv::InPerson::AddressController.step_info).to be_valid + end + end + describe 'before_actions' do context '#render_404_if_in_person_residential_address_controller_enabled not set' do context 'flag not set' do @@ -55,7 +56,7 @@ context '#confirm_in_person_state_id_step_complete' do it 'redirects to state id page if not complete' do - flow_session[:pii_from_user].delete(:identity_doc_address1) + subject.user_session['idv/in_person'][:pii_from_user].delete(:identity_doc_address1) get :show expect(response).to redirect_to idv_in_person_step_url(step: :state_id) @@ -99,7 +100,7 @@ end it 'redirects to ssn page when address1 present' do - flow_session[:pii_from_user][:address1] = '123 Main St' + subject.user_session['idv/in_person'][:pii_from_user][:address1] = '123 Main St' get :show @@ -143,7 +144,6 @@ state: state, } } end - let(:ssn) { '900123456' } let(:analytics_name) { 'IdV: in person proofing residential address submitted' } let(:analytics_args) do { @@ -165,7 +165,7 @@ it 'sets values in the flow session' do put :update, params: params - expect(flow_session[:pii_from_user]).to include( + expect(subject.user_session['idv/in_person'][:pii_from_user]).to include( address1:, address2:, city:, @@ -184,48 +184,53 @@ context 'when updating the residential address' do before do - flow_session[:pii_from_user][:address1] = '123 New Residential Ave' + subject.user_session['idv/in_person'][:pii_from_user][:address1] = + '123 New Residential Ave' end context 'user previously selected that the residential address matched state ID' do before do - flow_session[:pii_from_user][:same_address_as_id] = 'true' + subject.user_session['idv/in_person'][:pii_from_user][:same_address_as_id] = 'true' end it 'infers and sets the "same_address_as_id" in the flow session to false' do put :update, params: params - expect(flow_session[:pii_from_user][:same_address_as_id]).to eq('false') + expect(subject.user_session['idv/in_person'][:pii_from_user][:same_address_as_id]). + to eq('false') end end context 'user previously selected that the residential address did not match state ID' do before do - flow_session[:pii_from_user][:same_address_as_id] = 'false' + subject.user_session['idv/in_person'][:pii_from_user][:same_address_as_id] = 'false' end it 'leaves the "same_address_as_id" in the flow session as false' do put :update, params: params - expect(flow_session[:pii_from_user][:same_address_as_id]).to eq('false') + expect(subject.user_session['idv/in_person'][:pii_from_user][:same_address_as_id]). + to eq('false') end end end + + it 'invalidates future steps, but does not clear ssn' do + subject.idv_session.ssn = '123-45-6789' + expect(subject).to receive(:clear_future_steps_from!).and_call_original + + expect { put :update, params: params }.not_to change { subject.idv_session.ssn } + end end context 'invalid address details' do - let(:address1) { '1 F@KE RD' } - let(:address2) { '@?T 1B' } - let(:city) { 'GR3AT F&LLS' } - let(:zipcode) { '59010' } - let(:state) { 'Montana' } let(:params) do { in_person_address: { - address1: address1, - address2: address2, - city: city, - zipcode: zipcode, - state: state, + address1: '1 F@KE RD', + address2: '@?T 1B', + city: 'GR3AT F&LLS', + zipcode: '59010', + state: 'Montana', } } end let(:analytics_name) { 'IdV: in person proofing residential address submitted' } diff --git a/spec/policies/idv/flow_policy_spec.rb b/spec/policies/idv/flow_policy_spec.rb index cf0463d7cfd..8e9cf7d20a9 100644 --- a/spec/policies/idv/flow_policy_spec.rb +++ b/spec/policies/idv/flow_policy_spec.rb @@ -222,7 +222,9 @@ context 'preconditions for in_person ssn are present' do before do stub_up_to(:hybrid_handoff, idv_session: idv_session) - idv_session.send(:user_session)['idv/in_person'] = { pii_from_user: { pii: 'value' } } + idv_session.send(:user_session)['idv/in_person'] = { + pii_from_user: Idp::Constants::MOCK_IDV_APPLICANT_SAME_ADDRESS_AS_ID.dup, + } end it 'returns ipp_ssn' do From b157a242d9362050929e47c4f9fd93ec1cfd8bc6 Mon Sep 17 00:00:00 2001 From: Jack Ryan Date: Wed, 20 Dec 2023 16:25:49 -0500 Subject: [PATCH 17/19] LG-11904 Add 50/50 state tests for opt in navigation (#9798) * Adding in appropriate feature specs for opt-in ipp navigation around the 50 50 state * changelog: Internal, In-Person Proofing, Update specs for Opt In IPP to check for navigation issues during 50 50 state * Reformatting it blocks and editing test descriptions * Appeasing linter * Adding tests for Document Capture page and addressing github comments * Fixing line length * Making remote vs ipp flow explicit by taking advantage of keyword arg * Refactor spec * Lint fix --- .../idv/doc_auth/how_to_verify_spec.rb | 147 +++++++++++++++++- 1 file changed, 144 insertions(+), 3 deletions(-) diff --git a/spec/features/idv/doc_auth/how_to_verify_spec.rb b/spec/features/idv/doc_auth/how_to_verify_spec.rb index 532f0319c6d..8b1b06cf1fa 100644 --- a/spec/features/idv/doc_auth/how_to_verify_spec.rb +++ b/spec/features/idv/doc_auth/how_to_verify_spec.rb @@ -1,6 +1,6 @@ require 'rails_helper' -RSpec.feature 'how to verify step' do +RSpec.feature 'how to verify step', js: true do include IdvHelper include DocAuthHelper @@ -14,7 +14,7 @@ complete_agreement_step end - it 'skips when disabled and redirects to hybird handoff)' do + it 'skips when disabled and redirects to hybrid handoff' do expect(page).to have_current_path(idv_hybrid_handoff_url) end end @@ -68,8 +68,149 @@ expect(page).to have_current_path(idv_how_to_verify_path) expect(page).to have_content(t('errors.doc_auth.how_to_verify_form')) - complete_how_to_verify_step + complete_how_to_verify_step(remote: true) expect(page).to have_current_path(idv_hybrid_handoff_url) end end + + describe 'navigating to How To Verify from Agreement page in 50/50 state' do + before do + allow(IdentityConfig.store).to receive(:in_person_proofing_enabled) { true } + allow(IdentityConfig.store).to receive(:in_person_proofing_opt_in_enabled) { + initial_opt_in_enabled + } + + sign_in_and_2fa_user + complete_doc_auth_steps_before_agreement_step + complete_agreement_step + end + + context 'opt in false at start but true during navigation' do + let(:initial_opt_in_enabled) { false } + + it 'should not be bounced back from Hybrid Handoff to How to Verify' do + expect(page).to have_current_path(idv_hybrid_handoff_url) + allow(IdentityConfig.store).to receive(:in_person_proofing_opt_in_enabled) { true } + page.refresh + expect(page).to have_current_path(idv_hybrid_handoff_url) + end + end + + context 'opt in true at start but false during navigation' do + let(:initial_opt_in_enabled) { true } + + it 'should be redirected to Hybrid Handoff page when opt in is false' do + expect(page).to have_current_path(idv_how_to_verify_url) + allow(IdentityConfig.store).to receive(:in_person_proofing_opt_in_enabled) { false } + page.refresh + expect(page).to have_current_path(idv_hybrid_handoff_url) + end + end + + context 'Going back from Hybrid Handoff with opt in disabled midstream' do + let(:initial_opt_in_enabled) { true } + before do + complete_how_to_verify_step(remote: true) + end + + it 'should not be bounced back to How to Verify with opt in disabled midstream' do + expect(page).to have_current_path(idv_hybrid_handoff_url) + allow(IdentityConfig.store).to receive(:in_person_proofing_opt_in_enabled) { false } + page.go_back + expect(page).to have_current_path(idv_hybrid_handoff_url) + page.go_back + expect(page).to have_current_path(idv_agreement_url) + end + end + + context 'Going back from Hybrid Handoff with opt in enabled midstream' do + let(:initial_opt_in_enabled) { false } + + it 'should go back to the Agreement step from Hybrid Handoff with opt in toggled midstream' do + expect(page).to have_current_path(idv_hybrid_handoff_url) + allow(IdentityConfig.store).to receive(:in_person_proofing_opt_in_enabled) { true } + page.go_back + expect(page).to have_current_path(idv_agreement_url) + end + end + + context 'Going back from Hybrid Handoff with opt in enabled the whole time' do + let(:initial_opt_in_enabled) { true } + before do + complete_how_to_verify_step(remote: true) + end + + it 'should be bounced back to How to Verify' do + expect(page).to have_current_path(idv_hybrid_handoff_url) + page.go_back + expect(page).to have_current_path(idv_how_to_verify_url) + end + end + + context 'Going back from Hybrid Handoff with opt in disabled the whole time' do + let(:initial_opt_in_enabled) { false } + + it 'should be not be bounced back to How to Verify' do + expect(page).to have_current_path(idv_hybrid_handoff_url) + page.go_back + expect(page).to have_current_path(idv_agreement_url) + end + end + + context 'Going back from Document Capture with opt in disabled midstream' do + let(:initial_opt_in_enabled) { true } + before do + complete_how_to_verify_step(remote: false) + end + + it 'should not be bounced back to How to Verify with opt in disabled midstream' do + expect(page).to have_current_path(idv_document_capture_path) + allow(IdentityConfig.store).to receive(:in_person_proofing_opt_in_enabled) { false } + page.go_back + expect(page).to have_current_path(idv_document_capture_path) + page.go_back + expect(page).to have_current_path(idv_agreement_url) + end + end + + context 'Going back from Document Capture with opt in enabled midstream' do + let(:initial_opt_in_enabled) { false } + before do + complete_hybrid_handoff_step + end + + it 'should go to Hybrid Handoff from Document Capture with opt in toggled midstream' do + expect(page).to have_current_path(idv_document_capture_path) + allow(IdentityConfig.store).to receive(:in_person_proofing_opt_in_enabled) { true } + page.go_back + expect(page).to have_current_path(idv_hybrid_handoff_url) + end + end + + context 'Going back from Document Capture with opt in enabled the whole time' do + let(:initial_opt_in_enabled) { true } + before do + complete_how_to_verify_step(remote: false) + end + + it 'should be bounced back to How to Verify' do + expect(page).to have_current_path(idv_document_capture_path) + page.go_back + expect(page).to have_current_path(idv_how_to_verify_url) + end + end + + context 'Going back from Document Capture with opt in disabled the whole time' do + let(:initial_opt_in_enabled) { false } + before do + complete_hybrid_handoff_step + end + + it 'should be not be bounced back to how to verify' do + expect(page).to have_current_path(idv_document_capture_path) + page.go_back + expect(page).to have_current_path(idv_hybrid_handoff_url) + end + end + end end From 0297e22a06d2ea8566d86d304051795cc3ffdb3d Mon Sep 17 00:00:00 2001 From: Andrew Duthie <1779930+aduth@users.noreply.github.com> Date: Thu, 21 Dec 2023 08:24:40 -0500 Subject: [PATCH 18/19] Improve accuracy of Frontend packages, events documentation (#9814) * Improve accuracy of Frontend packages, events documentation changelog: Internal, Documentation, Improve accuracy of frontend architecture documentation * Restore bare import recommendation * Use consistent line length convention --- docs/frontend.md | 35 +++++++++++++++++------------------ 1 file changed, 17 insertions(+), 18 deletions(-) diff --git a/docs/frontend.md b/docs/frontend.md index af4f5c431a4..a1fc13ebe57 100644 --- a/docs/frontend.md +++ b/docs/frontend.md @@ -66,8 +66,8 @@ workflow to apply formatting automatically on save. [Workspaces](https://classic.yarnpkg.com/en/docs/workspaces/) allow a developer to create and organize code which is used just like any other NPM package, but which doesn't require the overhead -involved in publishing those modules and keeping versions in sync across multiple repositories. The -IDP uses Yarn workspaces to keep JavaScript code organized, reusable, and to encourage good coding +involved in publishing those modules and keeping versions in sync across multiple repositories. We +use Yarn workspaces to keep JavaScript code organized, reusable, and to encourage good coding practices in abstractions. In practice: @@ -75,21 +75,20 @@ In practice: - All folders within `app/javascript/packages` are treated as workspace packages. - Each package should have its own `package.json` that includes... - ...a `name` starting with `@18f/identity-` and ending with the name of the package folder. - - ...a listing of its own dependencies, including to other workspace packages using - [`file:` prefix](https://classic.yarnpkg.com/en/docs/cli/add/). - - ...[`"private": true`](https://docs.npmjs.com/files/package.json#private) if the workspace - package is not intended to be published to NPM. + - ...a [`private`](https://docs.npmjs.com/files/package.json#private) value indicating whether the + package is intended to be published to NPM. - ...a value for the `version` field, since it is required. The value value can be anything, and `"1.0.0"` is a good default. -- Each package should include an `index.js` which serves as the entry-point and public API for the - package. - -A package might have a corresponding file by the same package name contained within -`app/javascript/packs` that serves as the integration point between packages and the Rails -application. This is to encourage packages to be reusable, where the file in `packs` contains any -logic required to wire the package to the running Rails application. Because Yarn will alias -workspace packages using symlinks, you can reference a package using the name you assigned using the -guidelines above for `package.json` `name` field (for example, +- The package should be importable by its bare name, either with an `index.ts` or equivalent + [package entrypoints](https://nodejs.org/api/packages.html#package-entry-points) + +As with any public NPM package, a workspace package should ideally be reusable and avoid direct +references to page elements. In order to integrate a package within a particular page, you should +either reference it within [a ViewComponent component's accompanying script](https://github.com/18F/identity-idp/blob/main/app/components/README.md), +or by creating a new `app/javascript/packs` file to be loaded on a page. + +Because Yarn will alias workspace packages using symlinks, you can reference a package using the +name you assigned using the guidelines above for `package.json` `name` field (for example, `import { Button } from '@18f/identity-components';`). ### Dependencies @@ -138,9 +137,9 @@ See [`@18f/identity-analytics` package documentation][analytics_package] for cod how to track an event in JavaScript. Any event logged from the frontend must be added to the `ALLOWED_EVENTS` allowlist in [`FrontendLogController`][frontend_log_controller.rb]. -This mapping associates the event name logged from the frontend with the corresponding method from -[AnalyticsEvents][analytics_events.rb] to be called. All properties will be passed automatically to -the event from the frontend as long as they are defined in the method argument signature. +This is an allowlist of events defined in [AnalyticsEvents][analytics_events.rb] which are allowed +to be logged from the frontend. All properties will be passed automatically to the event from the +frontend as long as they are defined in the method argument signature. There may be some situations where you need to append a value known by the server to an event logged in the frontend, such as an A/B test bucket descriptor. In these scenarios, you have a few options: From f00bb67d61d288bec7274b0ce25e8a2be90d68dc Mon Sep 17 00:00:00 2001 From: Andrew Duthie <1779930+aduth@users.noreply.github.com> Date: Thu, 21 Dec 2023 08:41:58 -0500 Subject: [PATCH 19/19] Use design sytem colors for password strength meter (#9811) changelog: User-Facing Improvements, Password Strength, Use consistent colors for password strength feedback --- .../stylesheets/components/_password.scss | 49 +++++++++---------- .../devise/shared/_password_strength.html.erb | 10 ++-- 2 files changed, 27 insertions(+), 32 deletions(-) diff --git a/app/assets/stylesheets/components/_password.scss b/app/assets/stylesheets/components/_password.scss index 9b537c3e1fb..34ea8217bb2 100644 --- a/app/assets/stylesheets/components/_password.scss +++ b/app/assets/stylesheets/components/_password.scss @@ -1,39 +1,34 @@ @use 'uswds-core' as *; -$weak: #e80e0e; -$average: #ffac00; -$good: #9ac056; -$great: #00b200; - -.pw-bar { - background-color: #e9e9e9; - border: units(0.5) solid #fff; - border-radius: 6px; - float: left; - height: 16px; - width: 25%; +.password-strength__meter { + display: flex; + margin-top: units(1); + margin-bottom: units(0.5); } -.pw-weak { - .pw-bar:nth-child(-n + 1) { - background-color: $weak; +.password-strength__meter-bar { + flex-basis: 25%; + background-color: color('base-lighter'); + border-radius: 2px; + height: units(1); + + & + & { + margin-left: units(1); } -} -.pw-average { - .pw-bar:nth-child(-n + 2) { - background-color: $average; + .pw-weak &:nth-child(-n + 1) { + background-color: color('error'); } -} -.pw-good { - .pw-bar:nth-child(-n + 3) { - background-color: $good; + .pw-average &:nth-child(-n + 2) { + background-color: color('warning'); + } + + .pw-good &:nth-child(-n + 3) { + background-color: color('success-light'); } -} -.pw-great { - .pw-bar { - background-color: $great; + .pw-great &:nth-child(-n + 4) { + background-color: color('success'); } } diff --git a/app/views/devise/shared/_password_strength.html.erb b/app/views/devise/shared/_password_strength.html.erb index 98eae167a2f..f1e3d3a77a5 100644 --- a/app/views/devise/shared/_password_strength.html.erb +++ b/app/views/devise/shared/_password_strength.html.erb @@ -1,10 +1,10 @@