From e1fcab6b9db408b1f7ce8e1f9eca1eb78901672f Mon Sep 17 00:00:00 2001 From: Manish Shah <97545428+gsa-manish@users.noreply.github.com> Date: Wed, 1 Jun 2022 14:04:48 -0400 Subject: [PATCH 01/28] LG-5938-document-analytics-19 (#6432) changelog: Improvements, Analytics, Updated analytics events Co-authored-by: Manish Shah --- .../two_factor_authenticatable_methods.rb | 4 +- .../options_controller.rb | 4 +- .../users/phone_setup_controller.rb | 2 +- app/services/analytics.rb | 5 -- app/services/analytics_events.rb | 63 +++++++++++++++++++ ...ackup_code_verification_controller_spec.rb | 3 +- .../options_controller_spec.rb | 4 +- .../otp_verification_controller_spec.rb | 3 +- ...rsonal_key_verification_controller_spec.rb | 3 +- .../piv_cac_verification_controller_spec.rb | 3 +- .../totp_verification_controller_spec.rb | 3 +- .../users/phone_setup_controller_spec.rb | 8 +-- 12 files changed, 84 insertions(+), 21 deletions(-) diff --git a/app/controllers/concerns/two_factor_authenticatable_methods.rb b/app/controllers/concerns/two_factor_authenticatable_methods.rb index 7cfc8442e4a..6f773ae6ca5 100644 --- a/app/controllers/concerns/two_factor_authenticatable_methods.rb +++ b/app/controllers/concerns/two_factor_authenticatable_methods.rb @@ -18,14 +18,14 @@ def authenticate_user end def handle_second_factor_locked_user(type) - analytics.track_event(Analytics::MULTI_FACTOR_AUTH_MAX_ATTEMPTS) + analytics.multi_factor_auth_max_attempts event = PushNotification::MfaLimitAccountLockedEvent.new(user: current_user) PushNotification::HttpPush.deliver(event) handle_max_attempts(type + '_login_attempts') end def handle_too_many_otp_sends - analytics.track_event(Analytics::MULTI_FACTOR_AUTH_MAX_SENDS) + analytics.multi_factor_auth_max_sends handle_max_attempts('otp_requests') end diff --git a/app/controllers/two_factor_authentication/options_controller.rb b/app/controllers/two_factor_authentication/options_controller.rb index 45088f1d816..ad53b75b695 100644 --- a/app/controllers/two_factor_authentication/options_controller.rb +++ b/app/controllers/two_factor_authentication/options_controller.rb @@ -26,13 +26,13 @@ class OptionsController < ApplicationController def index @two_factor_options_form = TwoFactorLoginOptionsForm.new(current_user) @presenter = two_factor_options_presenter - analytics.track_event(Analytics::MULTI_FACTOR_AUTH_OPTION_LIST_VISIT) + analytics.multi_factor_auth_option_list_visit end def create @two_factor_options_form = TwoFactorLoginOptionsForm.new(current_user) result = @two_factor_options_form.submit(two_factor_options_form_params) - analytics.track_event(Analytics::MULTI_FACTOR_AUTH_OPTION_LIST, result.to_h) + analytics.multi_factor_auth_option_list(**result.to_h) if result.success? process_valid_form diff --git a/app/controllers/users/phone_setup_controller.rb b/app/controllers/users/phone_setup_controller.rb index 5cdc0548b76..907f1ff6e37 100644 --- a/app/controllers/users/phone_setup_controller.rb +++ b/app/controllers/users/phone_setup_controller.rb @@ -16,7 +16,7 @@ def index def create @new_phone_form = NewPhoneForm.new(current_user) result = @new_phone_form.submit(new_phone_form_params) - analytics.track_event(Analytics::MULTI_FACTOR_AUTH_PHONE_SETUP, result.to_h) + analytics.multi_factor_auth_phone_setup(**result.to_h) if result.success? handle_create_success(@new_phone_form.phone) diff --git a/app/services/analytics.rb b/app/services/analytics.rb index 68dbc048ee1..987a615af16 100644 --- a/app/services/analytics.rb +++ b/app/services/analytics.rb @@ -137,11 +137,6 @@ def session_started_at # rubocop:disable Layout/LineLength DOC_AUTH = 'Doc Auth' # visited or submitted is appended - MULTI_FACTOR_AUTH_MAX_ATTEMPTS = 'Multi-Factor Authentication: max attempts reached' - MULTI_FACTOR_AUTH_OPTION_LIST = 'Multi-Factor Authentication: option list' - MULTI_FACTOR_AUTH_OPTION_LIST_VISIT = 'Multi-Factor Authentication: option list visited' - MULTI_FACTOR_AUTH_PHONE_SETUP = 'Multi-Factor Authentication: phone setup' - MULTI_FACTOR_AUTH_MAX_SENDS = 'Multi-Factor Authentication: max otp sends reached' MULTI_FACTOR_AUTH_SETUP = 'Multi-Factor Authentication Setup' OPENID_CONNECT_BEARER_TOKEN = 'OpenID Connect: bearer token authentication' OPENID_CONNECT_REQUEST_AUTHORIZATION = 'OpenID Connect: authorization request' diff --git a/app/services/analytics_events.rb b/app/services/analytics_events.rb index a9c78062875..112ef05680e 100644 --- a/app/services/analytics_events.rb +++ b/app/services/analytics_events.rb @@ -1137,6 +1137,69 @@ def multi_factor_auth_enter_webauthn_visit( ) end + # Max multi factor auth attempts met + def multi_factor_auth_max_attempts + track_event('Multi-Factor Authentication: max attempts reached') + end + + # Multi factor selected from auth options list + # @param [Boolean] success + # @param [Hash] errors + # @param [String] selection + def multi_factor_auth_option_list(success:, errors:, selection:, **extra) + track_event( + 'Multi-Factor Authentication: option list', + success: success, + errors: errors, + selection: selection, + **extra, + ) + end + + # User visited the list of multi-factor options to use + def multi_factor_auth_option_list_visit + track_event('Multi-Factor Authentication: option list visited') + end + + # Multi factor auth phone setup + # @param [Boolean] success + # @param [Hash] errors + # @param [String] otp_delivery_preference + # @param [String] area_code + # @param [String] carrier + # @param [String] country_code + # @param [String] phone_type + # @param [Hash] types + # @param [Hash] pii_like_keypaths + def multi_factor_auth_phone_setup(success:, + errors:, + otp_delivery_preference:, + area_code:, + carrier:, + country_code:, + phone_type:, + types:, + **extra) + + track_event( + 'Multi-Factor Authentication: phone setup', + success: success, + errors: errors, + otp_delivery_preference: otp_delivery_preference, + area_code: area_code, + carrier: carrier, + country_code: country_code, + phone_type: phone_type, + types: types, + **extra, + ) + end + + # Max multi factor max otp sends reached + def multi_factor_auth_max_sends + track_event('Multi-Factor Authentication: max otp sends reached') + end + # @param [Boolean] success # @param [Hash] errors # The user updated their password diff --git a/spec/controllers/two_factor_authentication/backup_code_verification_controller_spec.rb b/spec/controllers/two_factor_authentication/backup_code_verification_controller_spec.rb index b5654fd5cb1..89a9d282d98 100644 --- a/spec/controllers/two_factor_authentication/backup_code_verification_controller_spec.rb +++ b/spec/controllers/two_factor_authentication/backup_code_verification_controller_spec.rb @@ -121,7 +121,8 @@ expect(@analytics).to receive(:track_mfa_submit_event). with(properties) - expect(@analytics).to receive(:track_event).with(Analytics::MULTI_FACTOR_AUTH_MAX_ATTEMPTS) + expect(@analytics).to receive(:track_event). + with('Multi-Factor Authentication: max attempts reached') expect(PushNotification::HttpPush).to receive(:deliver). with(PushNotification::MfaLimitAccountLockedEvent.new(user: subject.current_user)) diff --git a/spec/controllers/two_factor_authentication/options_controller_spec.rb b/spec/controllers/two_factor_authentication/options_controller_spec.rb index b0a7c9813ff..52770aedf5b 100644 --- a/spec/controllers/two_factor_authentication/options_controller_spec.rb +++ b/spec/controllers/two_factor_authentication/options_controller_spec.rb @@ -15,7 +15,7 @@ stub_analytics expect(@analytics).to receive(:track_event). - with(Analytics::MULTI_FACTOR_AUTH_OPTION_LIST_VISIT) + with('Multi-Factor Authentication: option list visited') get :index end @@ -87,7 +87,7 @@ } expect(@analytics).to receive(:track_event). - with(Analytics::MULTI_FACTOR_AUTH_OPTION_LIST, result) + with('Multi-Factor Authentication: option list', result) post :create, params: { two_factor_options_form: { selection: 'sms' } } end diff --git a/spec/controllers/two_factor_authentication/otp_verification_controller_spec.rb b/spec/controllers/two_factor_authentication/otp_verification_controller_spec.rb index 96a457c3670..7209c9fe27d 100644 --- a/spec/controllers/two_factor_authentication/otp_verification_controller_spec.rb +++ b/spec/controllers/two_factor_authentication/otp_verification_controller_spec.rb @@ -147,7 +147,8 @@ expect(@analytics).to receive(:track_mfa_submit_event). with(properties) - expect(@analytics).to receive(:track_event).with(Analytics::MULTI_FACTOR_AUTH_MAX_ATTEMPTS) + expect(@analytics).to receive(:track_event). + with('Multi-Factor Authentication: max attempts reached') expect(PushNotification::HttpPush).to receive(:deliver). with(PushNotification::MfaLimitAccountLockedEvent.new(user: subject.current_user)) diff --git a/spec/controllers/two_factor_authentication/personal_key_verification_controller_spec.rb b/spec/controllers/two_factor_authentication/personal_key_verification_controller_spec.rb index 93fd44df368..ee5018ecf41 100644 --- a/spec/controllers/two_factor_authentication/personal_key_verification_controller_spec.rb +++ b/spec/controllers/two_factor_authentication/personal_key_verification_controller_spec.rb @@ -155,7 +155,8 @@ expect(@analytics).to receive(:track_mfa_submit_event). with(properties) - expect(@analytics).to receive(:track_event).with(Analytics::MULTI_FACTOR_AUTH_MAX_ATTEMPTS) + expect(@analytics).to receive(:track_event). + with('Multi-Factor Authentication: max attempts reached') expect(PushNotification::HttpPush).to receive(:deliver). with(PushNotification::MfaLimitAccountLockedEvent.new(user: subject.current_user)) diff --git a/spec/controllers/two_factor_authentication/piv_cac_verification_controller_spec.rb b/spec/controllers/two_factor_authentication/piv_cac_verification_controller_spec.rb index 0ba1f392468..2f69c558cbc 100644 --- a/spec/controllers/two_factor_authentication/piv_cac_verification_controller_spec.rb +++ b/spec/controllers/two_factor_authentication/piv_cac_verification_controller_spec.rb @@ -194,7 +194,8 @@ expect(@analytics).to receive(:track_mfa_submit_event). with(submit_attributes) - expect(@analytics).to receive(:track_event).with(Analytics::MULTI_FACTOR_AUTH_MAX_ATTEMPTS) + expect(@analytics).to receive(:track_event). + with('Multi-Factor Authentication: max attempts reached') expect(PushNotification::HttpPush).to receive(:deliver). with(PushNotification::MfaLimitAccountLockedEvent.new(user: subject.current_user)) diff --git a/spec/controllers/two_factor_authentication/totp_verification_controller_spec.rb b/spec/controllers/two_factor_authentication/totp_verification_controller_spec.rb index 99447075c8b..de3a9337678 100644 --- a/spec/controllers/two_factor_authentication/totp_verification_controller_spec.rb +++ b/spec/controllers/two_factor_authentication/totp_verification_controller_spec.rb @@ -89,7 +89,8 @@ expect(@analytics).to receive(:track_mfa_submit_event). with(attributes) - expect(@analytics).to receive(:track_event).with(Analytics::MULTI_FACTOR_AUTH_MAX_ATTEMPTS) + expect(@analytics).to receive(:track_event). + with('Multi-Factor Authentication: max attempts reached') expect(PushNotification::HttpPush).to receive(:deliver). with(PushNotification::MfaLimitAccountLockedEvent.new(user: subject.current_user)) diff --git a/spec/controllers/users/phone_setup_controller_spec.rb b/spec/controllers/users/phone_setup_controller_spec.rb index 3bf91ad5b96..ee6a58a9282 100644 --- a/spec/controllers/users/phone_setup_controller_spec.rb +++ b/spec/controllers/users/phone_setup_controller_spec.rb @@ -63,7 +63,7 @@ } expect(@analytics).to receive(:track_event). - with(Analytics::MULTI_FACTOR_AUTH_PHONE_SETUP, result) + with('Multi-Factor Authentication: phone setup', result) patch :create, params: { new_phone_form: { @@ -95,7 +95,7 @@ } expect(@analytics).to receive(:track_event). - with(Analytics::MULTI_FACTOR_AUTH_PHONE_SETUP, result) + with('Multi-Factor Authentication: phone setup', result) patch( :create, @@ -135,7 +135,7 @@ } expect(@analytics).to receive(:track_event). - with(Analytics::MULTI_FACTOR_AUTH_PHONE_SETUP, result) + with('Multi-Factor Authentication: phone setup', result) patch( :create, @@ -174,7 +174,7 @@ } expect(@analytics).to receive(:track_event). - with(Analytics::MULTI_FACTOR_AUTH_PHONE_SETUP, result) + with('Multi-Factor Authentication: phone setup', result) patch( :create, From 518535db8d52f72c0aef38939cd3f72305cd3b02 Mon Sep 17 00:00:00 2001 From: Andrew Duthie Date: Thu, 2 Jun 2022 08:43:16 -0400 Subject: [PATCH 02/28] LG-6499: Update missing GPO-specific content (IdV app) (#6429) * Add AddressVerificationMethodContext for GPO content (IdV app) **Why**: For parity with the existing experience, users should expect to see content specific for the GPO verification method (e.g. status alerts, "Come back later" page after completion, pending step indicator status). Also, this addresses an issue where previously, the absence of a phone value was used in determining method, but this isn't accurate, since phone may be assigned if the user already have a verified phone number. changelog: Upcoming Features, Identity Verification, Add content for GPO verification to password confirm step * List used translation keys inside component implementation Webpack may be stripping comments for the topmost scope (tree shaking?), irrespective of optimization settings. * Document, test AddressVerificationMethodContextProvider * Add spec for step indicator component GPO progress * Add spec for GPO VerifyController#show app data completion URL * Update address verification method context to object See: https://github.com/18F/identity-idp/pull/6429#discussion_r886100527 * Fix completion URL for various GPO/active profile scenarios 1. GPO path should always direct to the "Come back later" screen. This was done in previous PersonalKeyController using "pending profile" logic. However, at the time that this controller method is called, the profile will never be activated. Instead, check GPO as the first condition path. 2. Similarly, "after sign in path" also considers whether the profile is pending, which will always be true at the time the controller is called, even if the user didn't go through the GPO flow. Instead, choose the specific account path fallback for no-GPO, no-SP IdV * Remove unused pending profile method --- app/controllers/verify_controller.rb | 6 +- ...dress-verification-method-context.spec.tsx | 59 ++++++++++++++++ .../address-verification-method-context.tsx | 69 +++++++++++++++++++ app/javascript/packages/verify-flow/index.ts | 1 + .../password-confirm-step.spec.tsx | 15 ++-- .../password-confirm-step.tsx | 4 +- .../personal-key/personal-key-step.spec.tsx | 29 ++++++-- .../steps/personal-key/personal-key-step.tsx | 7 +- .../verify-flow-step-indicator.spec.tsx | 14 ++++ .../verify-flow-step-indicator.tsx | 41 +++++++++-- .../packages/verify-flow/verify-flow.tsx | 38 ++++++---- app/javascript/packs/verify-flow.tsx | 32 ++++++--- spec/controllers/verify_controller_spec.rb | 41 ++++++++++- .../idv/steps/confirmation_step_spec.rb | 2 +- .../support/idv_examples/confirmation_step.rb | 17 ++--- 15 files changed, 318 insertions(+), 57 deletions(-) create mode 100644 app/javascript/packages/verify-flow/context/address-verification-method-context.spec.tsx create mode 100644 app/javascript/packages/verify-flow/context/address-verification-method-context.tsx diff --git a/app/controllers/verify_controller.rb b/app/controllers/verify_controller.rb index b9eebf22b7b..702ea1284b4 100644 --- a/app/controllers/verify_controller.rb +++ b/app/controllers/verify_controller.rb @@ -77,10 +77,12 @@ def generate_personal_key end def completion_url - if session[:sp] + if idv_session.address_verification_mechanism == 'gpo' + idv_come_back_later_url + elsif session[:sp] sign_up_completed_url else - after_sign_in_path_for(current_user) + account_url end end diff --git a/app/javascript/packages/verify-flow/context/address-verification-method-context.spec.tsx b/app/javascript/packages/verify-flow/context/address-verification-method-context.spec.tsx new file mode 100644 index 00000000000..b51b41b20eb --- /dev/null +++ b/app/javascript/packages/verify-flow/context/address-verification-method-context.spec.tsx @@ -0,0 +1,59 @@ +import { useContext } from 'react'; +import { render } from '@testing-library/react'; +import userEvent from '@testing-library/user-event'; +import AddressVerificationMethodContext, { + AddressVerificationMethodContextProvider, +} from './address-verification-method-context'; + +describe('AddressVerificationMethodContextProvider', () => { + function TestComponent() { + const { addressVerificationMethod, setAddressVerificationMethod } = useContext( + AddressVerificationMethodContext, + ); + return ( + <> +
Current value: {addressVerificationMethod}
+ + + ); + } + + it('initializes with default value', () => { + const { getByText } = render( + + + , + ); + + expect(getByText('Current value: phone')).to.exist(); + }); + + it('can be overridden with an initial method', () => { + const { getByText } = render( + + + , + ); + + expect(getByText('Current value: gpo')).to.exist(); + }); + + it('exposes a setter to change the value', async () => { + const { getByText } = render( + + + , + ); + + await userEvent.click(getByText('Update')); + + expect(getByText('Current value: phone')).to.exist(); + }); +}); diff --git a/app/javascript/packages/verify-flow/context/address-verification-method-context.tsx b/app/javascript/packages/verify-flow/context/address-verification-method-context.tsx new file mode 100644 index 00000000000..66a1a7d092a --- /dev/null +++ b/app/javascript/packages/verify-flow/context/address-verification-method-context.tsx @@ -0,0 +1,69 @@ +import { createContext, useState } from 'react'; +import { useObjectMemo } from '@18f/identity-react-hooks'; +import type { ReactNode } from 'react'; + +/** + * Mechanisms by which a user can verify their address. + */ +export type AddressVerificationMethod = 'phone' | 'gpo'; + +/** + * Context provider props. + */ +interface AddressVerificationMethodContextProviderProps { + /** + * Optional initial context value. + */ + initialMethod?: AddressVerificationMethod; + + /** + * Context children. + */ + children?: ReactNode; +} + +/** + * Context value. + */ +interface AddressVerificationMethodContextValue { + /** + * Current address verification method. + */ + addressVerificationMethod: AddressVerificationMethod; + + /** + * Setter to update to a new address verification method. + */ + setAddressVerificationMethod: (nextMethod: AddressVerificationMethod) => void; +} + +/** + * Default address verification method. + */ +const DEFAULT_METHOD: AddressVerificationMethod = 'phone'; + +/** + * Address verification method context container. + */ +const AddressVerificationMethodContext = createContext({ + addressVerificationMethod: DEFAULT_METHOD, + setAddressVerificationMethod: () => {}, +}); + +AddressVerificationMethodContext.displayName = 'AddressVerificationMethodContext'; + +export function AddressVerificationMethodContextProvider({ + initialMethod = DEFAULT_METHOD, + children, +}: AddressVerificationMethodContextProviderProps) { + const [addressVerificationMethod, setAddressVerificationMethod] = useState(initialMethod); + const value = useObjectMemo({ addressVerificationMethod, setAddressVerificationMethod }); + + return ( + + {children} + + ); +} + +export default AddressVerificationMethodContext; diff --git a/app/javascript/packages/verify-flow/index.ts b/app/javascript/packages/verify-flow/index.ts index 26ca86f8c73..a4d10d989ad 100644 --- a/app/javascript/packages/verify-flow/index.ts +++ b/app/javascript/packages/verify-flow/index.ts @@ -4,4 +4,5 @@ export { default as StartOverOrCancel } from './start-over-or-cancel'; export { default as VerifyFlow } from './verify-flow'; export type { SecretValues } from './context/secrets-context'; +export type { AddressVerificationMethod } from './context/address-verification-method-context'; export type { VerifyFlowValues } from './verify-flow'; diff --git a/app/javascript/packages/verify-flow/steps/password-confirm/password-confirm-step.spec.tsx b/app/javascript/packages/verify-flow/steps/password-confirm/password-confirm-step.spec.tsx index 22d0c9a1007..b32bfd01dc1 100644 --- a/app/javascript/packages/verify-flow/steps/password-confirm/password-confirm-step.spec.tsx +++ b/app/javascript/packages/verify-flow/steps/password-confirm/password-confirm-step.spec.tsx @@ -8,6 +8,7 @@ import { FormSteps } from '@18f/identity-form-steps'; import { t, i18n } from '@18f/identity-i18n'; import PasswordConfirmStep from './password-confirm-step'; import submit, { PasswordSubmitError } from './submit'; +import { AddressVerificationMethodContextProvider } from '../../context/address-verification-method-context'; describe('PasswordConfirmStep', () => { const sandbox = useSandbox(); @@ -118,18 +119,24 @@ describe('PasswordConfirmStep', () => { }); describe('alert', () => { - context('without phone value', () => { + context('with gpo as address verification method', () => { it('does not render success alert', () => { - const { queryByRole } = render(); + const { queryByRole } = render( + + + , + ); expect(queryByRole('status')).to.not.exist(); }); }); - context('with phone value', () => { + context('with phone as address verification method', () => { it('renders success alert', () => { const { queryByRole } = render( - , + + + , ); const status = queryByRole('status')!; diff --git a/app/javascript/packages/verify-flow/steps/password-confirm/password-confirm-step.tsx b/app/javascript/packages/verify-flow/steps/password-confirm/password-confirm-step.tsx index c51531e2cc5..f783bd938dc 100644 --- a/app/javascript/packages/verify-flow/steps/password-confirm/password-confirm-step.tsx +++ b/app/javascript/packages/verify-flow/steps/password-confirm/password-confirm-step.tsx @@ -17,6 +17,7 @@ import type { FormStepComponentProps } from '@18f/identity-form-steps'; import { ForgotPassword } from './forgot-password'; import PersonalInfoSummary from './personal-info-summary'; import StartOverOrCancel from '../../start-over-or-cancel'; +import AddressVerificationMethodContext from '../../context/address-verification-method-context'; import type { VerifyFlowValues } from '../..'; import { PasswordSubmitError } from './submit'; @@ -27,6 +28,7 @@ const FORGOT_PASSWORD_PATH = 'forgot_password'; function PasswordConfirmStep({ errors, registerField, onChange, value }: PasswordConfirmStepProps) { const { basePath } = useContext(FlowContext); const { onPageTransition } = useContext(FormStepsContext); + const { addressVerificationMethod } = useContext(AddressVerificationMethodContext); const stepPath = `${basePath}/password_confirm`; const [path] = useHistoryParam(undefined, { basePath: stepPath }); useDidUpdateEffect(onPageTransition, [path]); @@ -39,7 +41,7 @@ function PasswordConfirmStep({ errors, registerField, onChange, value }: Passwor return ( <> - {value.phone && ( + {addressVerificationMethod === 'phone' && ( {formatHTML( t('idv.messages.review.info_verified_html', { diff --git a/app/javascript/packages/verify-flow/steps/personal-key/personal-key-step.spec.tsx b/app/javascript/packages/verify-flow/steps/personal-key/personal-key-step.spec.tsx index 9536b7d6116..d4a207065fe 100644 --- a/app/javascript/packages/verify-flow/steps/personal-key/personal-key-step.spec.tsx +++ b/app/javascript/packages/verify-flow/steps/personal-key/personal-key-step.spec.tsx @@ -3,6 +3,7 @@ import * as analytics from '@18f/identity-analytics'; import { render } from '@testing-library/react'; import userEvent from '@testing-library/user-event'; import PersonalKeyStep from './personal-key-step'; +import { AddressVerificationMethodContextProvider } from '../../context/address-verification-method-context'; describe('PersonalKeyStep', () => { const sandbox = sinon.createSandbox(); @@ -49,11 +50,31 @@ describe('PersonalKeyStep', () => { expect(analytics.trackEvent).to.have.been.calledWith('IdV: print personal key'); }); - it('renders success alert', () => { - const { getByRole } = render(); + context('with gpo as address verification method', () => { + it('renders success alert', () => { + const { getByRole } = render( + + + , + ); - const status = getByRole('status'); + const status = getByRole('status'); - expect(status.textContent).to.equal('idv.messages.confirm'); + expect(status.textContent).to.equal('idv.messages.mail_sent'); + }); + }); + + context('with phone as address verification method', () => { + it('renders success alert', () => { + const { getByRole } = render( + + + , + ); + + const status = getByRole('status'); + + expect(status.textContent).to.equal('idv.messages.confirm'); + }); }); }); diff --git a/app/javascript/packages/verify-flow/steps/personal-key/personal-key-step.tsx b/app/javascript/packages/verify-flow/steps/personal-key/personal-key-step.tsx index 45dceb738cc..f8827dacff1 100644 --- a/app/javascript/packages/verify-flow/steps/personal-key/personal-key-step.tsx +++ b/app/javascript/packages/verify-flow/steps/personal-key/personal-key-step.tsx @@ -1,3 +1,4 @@ +import { useContext } from 'react'; import { Alert, PageHeading } from '@18f/identity-components'; import { ClipboardButton } from '@18f/identity-clipboard-button'; import { PrintButton } from '@18f/identity-print-button'; @@ -8,17 +9,21 @@ import type { FormStepComponentProps } from '@18f/identity-form-steps'; import { getAssetPath } from '@18f/identity-assets'; import { trackEvent } from '@18f/identity-analytics'; import type { VerifyFlowValues } from '../../verify-flow'; +import AddressVerificationMethodContext from '../../context/address-verification-method-context'; import DownloadButton from './download-button'; interface PersonalKeyStepProps extends FormStepComponentProps {} function PersonalKeyStep({ value }: PersonalKeyStepProps) { const personalKey = value.personalKey!; + const { addressVerificationMethod } = useContext(AddressVerificationMethodContext); return ( <> - {t('idv.messages.confirm')} + {addressVerificationMethod === 'phone' + ? t('idv.messages.confirm') + : t('idv.messages.mail_sent')} {t('headings.personal_key')}

{t('instructions.personal_key.info')}

diff --git a/app/javascript/packages/verify-flow/verify-flow-step-indicator.spec.tsx b/app/javascript/packages/verify-flow/verify-flow-step-indicator.spec.tsx index 7c2086a2257..136b1e6cf31 100644 --- a/app/javascript/packages/verify-flow/verify-flow-step-indicator.spec.tsx +++ b/app/javascript/packages/verify-flow/verify-flow-step-indicator.spec.tsx @@ -1,5 +1,6 @@ import { render } from '@testing-library/react'; import { StepStatus } from '@18f/identity-step-indicator'; +import { AddressVerificationMethodContextProvider } from './context/address-verification-method-context'; import VerifyFlowStepIndicator, { getStepStatus } from './verify-flow-step-indicator'; describe('getStepStatus', () => { @@ -32,4 +33,17 @@ describe('VerifyFlowStepIndicator', () => { const previous = getByText('step_indicator.flows.idv.verify_phone_or_address'); expect(previous.closest('.step-indicator__step--complete')).to.exist(); }); + + context('with gpo as address verification method', () => { + it('renders address verification as pending', () => { + const { getByText } = render( + + + , + ); + + const previous = getByText('step_indicator.flows.idv.verify_phone_or_address'); + expect(previous.closest('.step-indicator__step--pending')).to.exist(); + }); + }); }); diff --git a/app/javascript/packages/verify-flow/verify-flow-step-indicator.tsx b/app/javascript/packages/verify-flow/verify-flow-step-indicator.tsx index 16bf41696ff..1932a531a60 100644 --- a/app/javascript/packages/verify-flow/verify-flow-step-indicator.tsx +++ b/app/javascript/packages/verify-flow/verify-flow-step-indicator.tsx @@ -1,11 +1,8 @@ +import { useContext } from 'react'; import { StepIndicator, StepIndicatorStep, StepStatus } from '@18f/identity-step-indicator'; import { t } from '@18f/identity-i18n'; - -// i18n-tasks-use t('step_indicator.flows.idv.getting_started') -// i18n-tasks-use t('step_indicator.flows.idv.verify_id') -// i18n-tasks-use t('step_indicator.flows.idv.verify_info') -// i18n-tasks-use t('step_indicator.flows.idv.verify_phone_or_address') -// i18n-tasks-use t('step_indicator.flows.idv.secure_account') +import AddressVerificationMethodContext from './context/address-verification-method-context'; +import type { AddressVerificationMethod } from './context/address-verification-method-context'; type VerifyFlowStepIndicatorStep = | 'getting_started' @@ -62,8 +59,38 @@ export function getStepStatus(index, currentStepIndex): StepStatus { return StepStatus.INCOMPLETE; } +/** + * Given contextual details of the current flow path, returns explicit statuses which should be used + * at particular steps. + * + * @param details Flow details + * + * @return Step status overrides. + */ +function getStatusOverrides({ + addressVerificationMethod, +}: { + addressVerificationMethod: AddressVerificationMethod; +}) { + const statuses: Partial> = {}; + + if (addressVerificationMethod === 'gpo') { + statuses.verify_phone_or_address = StepStatus.PENDING; + } + + return statuses; +} + function VerifyFlowStepIndicator({ currentStep }: VerifyFlowStepIndicatorProps) { const currentStepIndex = STEP_INDICATOR_STEPS.indexOf(FLOW_STEP_STEP_MAPPING[currentStep]); + const { addressVerificationMethod } = useContext(AddressVerificationMethodContext); + const statusOverrides = getStatusOverrides({ addressVerificationMethod }); + + // i18n-tasks-use t('step_indicator.flows.idv.getting_started') + // i18n-tasks-use t('step_indicator.flows.idv.verify_id') + // i18n-tasks-use t('step_indicator.flows.idv.verify_info') + // i18n-tasks-use t('step_indicator.flows.idv.verify_phone_or_address') + // i18n-tasks-use t('step_indicator.flows.idv.secure_account') return ( @@ -71,7 +98,7 @@ function VerifyFlowStepIndicator({ currentStep }: VerifyFlowStepIndicatorProps) ))} diff --git a/app/javascript/packages/verify-flow/verify-flow.tsx b/app/javascript/packages/verify-flow/verify-flow.tsx index 654dadf78bb..e49dc4e5961 100644 --- a/app/javascript/packages/verify-flow/verify-flow.tsx +++ b/app/javascript/packages/verify-flow/verify-flow.tsx @@ -8,6 +8,10 @@ import VerifyFlowStepIndicator from './verify-flow-step-indicator'; import { useSyncedSecretValues } from './context/secrets-context'; import FlowContext from './context/flow-context'; import useInitialStepValidation from './hooks/use-initial-step-validation'; +import { + AddressVerificationMethod, + AddressVerificationMethodContextProvider, +} from './context/address-verification-method-context'; export interface VerifyFlowValues { userBundleToken?: string; @@ -65,6 +69,11 @@ interface VerifyFlowProps { */ cancelURL?: string; + /** + * Initial value for address verification method. + */ + initialAddressVerificationMethod?: AddressVerificationMethod; + /** * Callback invoked after completing the form. */ @@ -98,6 +107,7 @@ function VerifyFlow({ basePath, startOverURL = '', cancelURL = '', + initialAddressVerificationMethod, onComplete, }: VerifyFlowProps) { let steps = STEPS; @@ -125,19 +135,21 @@ function VerifyFlow({ return ( - - + + + + ); } diff --git a/app/javascript/packs/verify-flow.tsx b/app/javascript/packs/verify-flow.tsx index 2b4094472bc..79835984d90 100644 --- a/app/javascript/packs/verify-flow.tsx +++ b/app/javascript/packs/verify-flow.tsx @@ -1,5 +1,9 @@ import { render } from 'react-dom'; -import { VerifyFlow, SecretsContextProvider } from '@18f/identity-verify-flow'; +import { + VerifyFlow, + SecretsContextProvider, + AddressVerificationMethod, +} from '@18f/identity-verify-flow'; import SecretSessionStorage, { s2ab } from '@18f/identity-secret-session-storage'; import type { SecretValues, VerifyFlowValues } from '@18f/identity-verify-flow'; @@ -45,6 +49,16 @@ interface AppRootValues { userBundleToken: string; } +interface UserBundleMetadata { + address_verification_mechanism: AddressVerificationMethod; +} + +interface UserBundle { + pii: Record; + + metadata: UserBundleMetadata; +} + interface AppRootElement extends HTMLElement { dataset: DOMStringMap & AppRootValues; } @@ -78,15 +92,14 @@ const storage = new SecretSessionStorage('verify'); ]); storage.key = cryptoKey; await storage.load(); - if (initialValues.userBundleToken) { - await storage.setItem('userBundleToken', initialValues.userBundleToken); - } - const userBundleToken = storage.getItem('userBundleToken'); - if (userBundleToken) { - const jwtData = JSON.parse(atob(userBundleToken.split('.')[1])); - const pii = Object.fromEntries(mapKeys(jwtData.pii, camelCase)); - Object.assign(initialValues, pii); + let initialAddressVerificationMethod: AddressVerificationMethod | undefined; + if (initialValues.userBundleToken) { + const { userBundleToken } = initialValues; + await storage.setItem('userBundleToken', userBundleToken); + const { pii, metadata } = JSON.parse(atob(userBundleToken.split('.')[1])) as UserBundle; + Object.assign(initialValues, Object.fromEntries(mapKeys(pii, camelCase))); + initialAddressVerificationMethod = metadata.address_verification_mechanism; } function onComplete() { @@ -103,6 +116,7 @@ const storage = new SecretSessionStorage('verify'); cancelURL={cancelURL} basePath={basePath} onComplete={onComplete} + initialAddressVerificationMethod={initialAddressVerificationMethod} /> , appRoot, diff --git a/spec/controllers/verify_controller_spec.rb b/spec/controllers/verify_controller_spec.rb index 38461cf851e..39376d80ce8 100644 --- a/spec/controllers/verify_controller_spec.rb +++ b/spec/controllers/verify_controller_spec.rb @@ -18,6 +18,8 @@ end let(:profile) { subject.idv_session.profile } let(:step) { '' } + let(:sp) { build(:service_provider) } + let(:sp_session) { { issuer: sp.issuer } } subject(:response) { get :show, params: { step: step } } @@ -26,6 +28,7 @@ and_return(idv_api_enabled_steps) stub_sign_in(user) stub_idv_session + session[:sp] = sp_session if sp_session end it 'renders 404' do @@ -70,7 +73,7 @@ base_path: idv_app_path, start_over_url: idv_session_path, cancel_url: idv_cancel_path, - completion_url: idv_gpo_verify_url, + completion_url: sign_up_completed_url, enabled_step_names: idv_api_enabled_steps, initial_values: { 'personalKey' => kind_of(String) }, store_key: kind_of(String), @@ -84,6 +87,38 @@ expect(response).to render_template(:show) end end + + context 'without associated sp' do + let(:sp_session) { nil } + + it 'sets completion url' do + response + + expect(assigns[:app_data][:completion_url]).to eq(account_url) + end + end + + context 'with gpo as address verification method' do + before do + controller.idv_session.address_verification_mechanism = 'gpo' + end + + it 'sets completion url' do + response + + expect(assigns[:app_data][:completion_url]).to eq(idv_come_back_later_url) + end + + context 'without associated sp' do + let(:sp_session) { nil } + + it 'sets completion url' do + response + + expect(assigns[:app_data][:completion_url]).to eq(idv_come_back_later_url) + end + end + end end context 'with password confirmation step enabled' do @@ -101,7 +136,7 @@ base_path: idv_app_path, start_over_url: idv_session_path, cancel_url: idv_cancel_path, - completion_url: account_url, + completion_url: sign_up_completed_url, enabled_step_names: idv_api_enabled_steps, initial_values: { 'userBundleToken' => kind_of(String) }, store_key: kind_of(String), @@ -122,7 +157,7 @@ def stub_idv_session idv_session = Idv::Session.new( user_session: controller.user_session, current_user: user, - service_provider: nil, + service_provider: sp, ) idv_session.applicant = applicant idv_session.resolution_successful = true diff --git a/spec/features/idv/steps/confirmation_step_spec.rb b/spec/features/idv/steps/confirmation_step_spec.rb index 165e98c8e6e..6fa9a067290 100644 --- a/spec/features/idv/steps/confirmation_step_spec.rb +++ b/spec/features/idv/steps/confirmation_step_spec.rb @@ -28,7 +28,7 @@ context 'with idv app feature enabled' do before do allow(IdentityConfig.store).to receive(:idv_api_enabled_steps). - and_return(['personal_key', 'personal_key_confirm']) + and_return(['password_confirm', 'personal_key', 'personal_key_confirm']) end it_behaves_like 'idv confirmation step' diff --git a/spec/support/idv_examples/confirmation_step.rb b/spec/support/idv_examples/confirmation_step.rb index 81436859616..1d355a231ba 100644 --- a/spec/support/idv_examples/confirmation_step.rb +++ b/spec/support/idv_examples/confirmation_step.rb @@ -21,7 +21,8 @@ end context 'user selected gpo verification' do - it 'shows step indicator progress with pending verify phone step' do + it 'shows status content for gpo verification progress' do + expect(page).to have_content(t('idv.messages.mail_sent')) expect(page).to have_css( '.step-indicator__step--current', text: t('step_indicator.flows.idv.secure_account'), @@ -40,7 +41,8 @@ complete_idv_steps_with_phone_before_confirmation_step end - it 'shows step indicator progress with complete verify phone step' do + it 'shows status content for phone verification progress' do + expect(page).to have_content(t('idv.messages.confirm')) expect(page).to have_css( '.step-indicator__step--current', text: t('step_indicator.flows.idv.secure_account'), @@ -49,6 +51,7 @@ '.step-indicator__step--complete', text: t('step_indicator.flows.idv.verify_phone_or_address'), ) + expect(page).not_to have_css('.step-indicator__step--pending') end it 'redirects to the completions page and then to the SP', if: sp.present? do @@ -71,15 +74,5 @@ expect(page).to have_content(t('headings.account.verified_account')) expect(page).to have_current_path(account_path) end - - context 'user selected gpo verification' do - it 'shows step indicator progress without pending verify step' do - expect(page).to have_css( - '.step-indicator__step--current', - text: t('step_indicator.flows.idv.secure_account'), - ) - expect(page).not_to have_css('.step-indicator__step--pending') - end - end end end From 377febef66ba43760a5618d628a1d768b4209131 Mon Sep 17 00:00:00 2001 From: Andrew Duthie Date: Thu, 2 Jun 2022 09:32:42 -0400 Subject: [PATCH 03/28] Add step feature flag association to IdV API endpoints (#6435) * Evaluate render condition in context of instance So that the proc can reference instance methods * Add step association to IdV API endpoints **Why**: - Since we'll have future in-progress endpoints, the enabling of a single step should not make all endpoints available - Since BaseController is not practically reusable outside of identity verification, update module grouping to identity as specific to identity verification changelog: Upcoming Features, Identity Verification, Isolate step feature flags for submission handling * Use constant for BaseController required step See: https://github.com/18F/identity-idp/pull/6435#discussion_r886985298 * Use class property for required step * Define required_step using class_attribute See: - https://github.com/18F/identity-idp/pull/6435#commitcomment-75106263 - https://github.com/18F/identity-idp/pull/6435#issuecomment-1143962258 --- app/controllers/api/base_controller.rb | 15 ------ app/controllers/api/verify/base_controller.rb | 26 +++++++++ .../api/verify/password_confirm_controller.rb | 4 +- .../api/verify/password_reset_controller.rb | 4 +- .../concerns/render_condition_concern.rb | 2 +- .../api/verify/base_controller_spec.rb | 53 +++++++++++++++++++ .../password_confirm_controller_spec.rb | 11 ++-- .../verify/password_reset_controller_spec.rb | 4 +- .../concerns/render_condition_concern_spec.rb | 7 +++ 9 files changed, 98 insertions(+), 28 deletions(-) delete mode 100644 app/controllers/api/base_controller.rb create mode 100644 app/controllers/api/verify/base_controller.rb create mode 100644 spec/controllers/api/verify/base_controller_spec.rb diff --git a/app/controllers/api/base_controller.rb b/app/controllers/api/base_controller.rb deleted file mode 100644 index 5248571b1a2..00000000000 --- a/app/controllers/api/base_controller.rb +++ /dev/null @@ -1,15 +0,0 @@ -module Api - class BaseController < ApplicationController - include RenderConditionConcern - - check_or_render_not_found -> { FeatureManagement.idv_api_enabled? } - before_action :confirm_two_factor_authenticated_for_api - - respond_to :json - - def confirm_two_factor_authenticated_for_api - return if user_fully_authenticated? - render json: { error: 'user is not fully authenticated' }, status: :unauthorized - end - end -end diff --git a/app/controllers/api/verify/base_controller.rb b/app/controllers/api/verify/base_controller.rb new file mode 100644 index 00000000000..2b54c602cf0 --- /dev/null +++ b/app/controllers/api/verify/base_controller.rb @@ -0,0 +1,26 @@ +module Api + module Verify + class BaseController < ApplicationController + include RenderConditionConcern + + class_attribute :required_step + + check_or_render_not_found -> do + if self.class.required_step.blank? + raise NotImplementedError, 'Controller must define required_step' + end + IdentityConfig.store.idv_api_enabled_steps.include?(self.class.required_step) + end + before_action :confirm_two_factor_authenticated_for_api + + respond_to :json + + private + + def confirm_two_factor_authenticated_for_api + return if user_fully_authenticated? + render json: { error: 'user is not fully authenticated' }, status: :unauthorized + end + end + end +end diff --git a/app/controllers/api/verify/password_confirm_controller.rb b/app/controllers/api/verify/password_confirm_controller.rb index d05f2f40e4c..ceae90b1d51 100644 --- a/app/controllers/api/verify/password_confirm_controller.rb +++ b/app/controllers/api/verify/password_confirm_controller.rb @@ -1,6 +1,8 @@ module Api module Verify - class PasswordConfirmController < Api::BaseController + class PasswordConfirmController < BaseController + self.required_step = 'password_confirm' + def create result, personal_key = Api::ProfileCreationForm.new( password: verify_params[:password], diff --git a/app/controllers/api/verify/password_reset_controller.rb b/app/controllers/api/verify/password_reset_controller.rb index a480eed117a..4fb2be30dd4 100644 --- a/app/controllers/api/verify/password_reset_controller.rb +++ b/app/controllers/api/verify/password_reset_controller.rb @@ -1,6 +1,8 @@ module Api module Verify - class PasswordResetController < Api::BaseController + class PasswordResetController < BaseController + self.required_step = 'password_confirm' + def create analytics.idv_forgot_password_confirmed request_id = sp_session[:request_id] diff --git a/app/controllers/concerns/render_condition_concern.rb b/app/controllers/concerns/render_condition_concern.rb index e68fcfaebc0..e2d69c37690 100644 --- a/app/controllers/concerns/render_condition_concern.rb +++ b/app/controllers/concerns/render_condition_concern.rb @@ -3,7 +3,7 @@ module RenderConditionConcern module ClassMethods def check_or_render_not_found(callable, **kwargs) - before_action(**kwargs) { render_not_found if !callable.call } + before_action(**kwargs) { render_not_found if !instance_exec(&callable) } end end end diff --git a/spec/controllers/api/verify/base_controller_spec.rb b/spec/controllers/api/verify/base_controller_spec.rb new file mode 100644 index 00000000000..6ec69686c12 --- /dev/null +++ b/spec/controllers/api/verify/base_controller_spec.rb @@ -0,0 +1,53 @@ +require 'rails_helper' + +describe Api::Verify::BaseController do + describe '#show' do + subject(:response) { get :show } + + context 'without required_step defined' do + controller Api::Verify::BaseController do + def show; end + end + + before { routes.draw { get '/' => 'api/verify/base#show' } } + + it 'raises an exception' do + expect { response }.to raise_error(NotImplementedError) + end + end + + context 'with required_step defined' do + controller Api::Verify::BaseController do + self.required_step = 'example' + + def show + render json: {} + end + end + + before { routes.draw { get '/' => 'api/verify/base#show' } } + + it 'renders as not found (404)' do + expect(response.status).to eq(404) + end + + context 'with step enabled' do + before do + allow(IdentityConfig.store).to receive(:idv_api_enabled_steps).and_return(['example']) + end + + it 'renders as unauthorized (401)' do + expect(response.status).to eq(401) + end + + context 'with authenticated user' do + before { stub_sign_in } + + it 'renders as ok (200)' do + expect(response.status).to eq(200) + end + end + end + end + end +end diff --git a/spec/controllers/api/verify/password_confirm_controller_spec.rb b/spec/controllers/api/verify/password_confirm_controller_spec.rb index ab00ade7b6d..da778bdf00f 100644 --- a/spec/controllers/api/verify/password_confirm_controller_spec.rb +++ b/spec/controllers/api/verify/password_confirm_controller_spec.rb @@ -31,16 +31,11 @@ def stub_idv_session let(:jwt) { JWT.encode({ pii: pii, metadata: {} }, key, 'RS256', sub: user.uuid) } before do - allow(IdentityConfig.store).to receive(:idv_api_enabled_steps).and_return(['personal_key']) + allow(IdentityConfig.store).to receive(:idv_api_enabled_steps).and_return(['password_confirm']) end - describe 'before_actions' do - it 'includes before_actions from Api::BaseController' do - expect(subject).to have_actions( - :before, - :confirm_two_factor_authenticated_for_api, - ) - end + it 'extends behavior of base api class' do + expect(subject).to be_kind_of Api::Verify::BaseController end describe '#create' do diff --git a/spec/controllers/api/verify/password_reset_controller_spec.rb b/spec/controllers/api/verify/password_reset_controller_spec.rb index 814f412e3c3..d12d4fab7e2 100644 --- a/spec/controllers/api/verify/password_reset_controller_spec.rb +++ b/spec/controllers/api/verify/password_reset_controller_spec.rb @@ -6,13 +6,13 @@ let(:sp_session) { { request_id: request_id } } before do - allow(IdentityConfig.store).to receive(:idv_api_enabled_steps).and_return(['personal_key']) + allow(IdentityConfig.store).to receive(:idv_api_enabled_steps).and_return(['password_confirm']) allow(controller).to receive(:sp_session).and_return(sp_session) stub_sign_in(user) end it 'extends behavior of base api class' do - expect(subject).to be_kind_of Api::BaseController + expect(subject).to be_kind_of Api::Verify::BaseController end describe '#create' do diff --git a/spec/controllers/concerns/render_condition_concern_spec.rb b/spec/controllers/concerns/render_condition_concern_spec.rb index 985b6ff25d2..13ca98d7384 100644 --- a/spec/controllers/concerns/render_condition_concern_spec.rb +++ b/spec/controllers/concerns/render_condition_concern_spec.rb @@ -9,6 +9,7 @@ check_or_render_not_found -> { FeatureManagement.all_feature? } check_or_render_not_found -> { FeatureManagement.show_feature? }, only: [:show] + check_or_render_not_found -> { instance_condition? } def index render plain: '' @@ -17,6 +18,12 @@ def index def show render plain: '' end + + private + + def instance_condition? + true + end end before do From cdea08fd55ff2f8a1e92a60009dad5b57a491e63 Mon Sep 17 00:00:00 2001 From: Malick Diarra Date: Thu, 2 Jun 2022 09:35:24 -0400 Subject: [PATCH 04/28] LG-6043: first change over to new url (#6353) * Changelog: Future Feature, Identity authentication, create more descriptive url for MFA selection page * changelog: Improvements, change MFA setup url to be more descriptive, LG-6043 * LG-6043: change url to authentication method setup * fix cancellation spec * update routes * fix test --- app/controllers/account_reset/request_controller.rb | 2 +- app/controllers/application_controller.rb | 5 +++-- app/controllers/sign_up/passwords_controller.rb | 2 +- .../personal_key_verification_controller.rb | 4 ++-- .../two_factor_authentication/sms_opt_in_controller.rb | 2 +- .../users/two_factor_authentication_controller.rb | 4 ++-- .../users/two_factor_authentication_setup_controller.rb | 4 ++-- app/presenters/cancellation_presenter.rb | 2 +- app/views/idv/doc_auth/welcome.html.erb | 2 +- app/views/shared/_cancel_or_back_to_options.html.erb | 2 +- app/views/users/phone_setup/index.html.erb | 2 +- app/views/users/piv_cac_authentication_setup/error.html.erb | 2 +- .../users/two_factor_authentication_setup/index.html.erb | 2 +- config/routes.rb | 3 ++- spec/controllers/account_reset/request_controller_spec.rb | 4 ++-- spec/controllers/application_controller_spec.rb | 2 +- .../personal_key_verification_controller_spec.rb | 4 ++-- .../users/two_factor_authentication_controller_spec.rb | 6 +++--- .../two_factor_authentication_setup_controller_spec.rb | 2 +- spec/features/accessibility/user_pages_spec.rb | 2 +- spec/features/openid_connect/aal3_required_spec.rb | 4 ++-- spec/features/phone/confirmation_spec.rb | 2 +- spec/features/saml/aal3_required_spec.rb | 2 +- spec/features/saml/ial1_sso_spec.rb | 2 +- spec/features/saml/saml_spec.rb | 2 +- .../two_factor_authentication/multiple_mfa_sign_up_spec.rb | 6 +++--- spec/features/two_factor_authentication/sign_in_spec.rb | 4 ++-- spec/features/users/sign_in_spec.rb | 4 ++-- spec/features/users/sign_up_spec.rb | 6 +++--- spec/features/visitors/email_confirmation_spec.rb | 2 +- spec/features/visitors/password_recovery_spec.rb | 4 ++-- spec/features/webauthn/sign_up_spec.rb | 2 +- spec/presenters/cancellation_presenter_spec.rb | 2 +- spec/support/features/session_helper.rb | 2 +- spec/views/phone_setup/index.html.erb_spec.rb | 2 +- .../users/piv_cac_authentication_setup/new.html.erb_spec.rb | 2 +- spec/views/users/totp_setup/new.html.erb_spec.rb | 2 +- 37 files changed, 55 insertions(+), 53 deletions(-) diff --git a/app/controllers/account_reset/request_controller.rb b/app/controllers/account_reset/request_controller.rb index 05c18b926a4..0ecc00c604e 100644 --- a/app/controllers/account_reset/request_controller.rb +++ b/app/controllers/account_reset/request_controller.rb @@ -24,7 +24,7 @@ def create_account_reset_request def confirm_two_factor_enabled return if MfaPolicy.new(current_user).two_factor_enabled? - redirect_to two_factor_options_url + redirect_to authentication_methods_setup_url end def analytics_attributes diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index 830d7596552..59707cdce32 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -186,7 +186,8 @@ def add_piv_cac_setup_url end def service_provider_mfa_setup_url - service_provider_mfa_policy.user_needs_sp_auth_method_setup? ? two_factor_options_url : nil + service_provider_mfa_policy.user_needs_sp_auth_method_setup? ? + authentication_methods_setup_url : nil end def fix_broken_personal_key_url @@ -312,7 +313,7 @@ def prompt_to_sign_in_with_request_id(request_id) end def prompt_to_setup_mfa - redirect_to two_factor_options_url + redirect_to authentication_methods_setup_url end def prompt_to_verify_mfa diff --git a/app/controllers/sign_up/passwords_controller.rb b/app/controllers/sign_up/passwords_controller.rb index b5699926e3a..9441308eec8 100644 --- a/app/controllers/sign_up/passwords_controller.rb +++ b/app/controllers/sign_up/passwords_controller.rb @@ -75,7 +75,7 @@ def process_unsuccessful_password_creation def sign_in_and_redirect_user sign_in @user - redirect_to two_factor_options_url + redirect_to authentication_methods_setup_url end end end diff --git a/app/controllers/two_factor_authentication/personal_key_verification_controller.rb b/app/controllers/two_factor_authentication/personal_key_verification_controller.rb index eb56cad8773..9237f4bab0e 100644 --- a/app/controllers/two_factor_authentication/personal_key_verification_controller.rb +++ b/app/controllers/two_factor_authentication/personal_key_verification_controller.rb @@ -26,7 +26,7 @@ def create def check_personal_key_enabled return if TwoFactorAuthentication::PersonalKeyPolicy.new(current_user).enabled? - redirect_to two_factor_options_url + redirect_to authentication_methods_setup_url end def presenter_for_two_factor_authentication_method @@ -97,7 +97,7 @@ def handle_valid_otp elsif MfaPolicy.new(current_user).two_factor_enabled? redirect_to after_mfa_setup_path else - redirect_to two_factor_options_url + redirect_to authentication_methods_setup_url end reset_otp_session_data end diff --git a/app/controllers/two_factor_authentication/sms_opt_in_controller.rb b/app/controllers/two_factor_authentication/sms_opt_in_controller.rb index ea324fc7030..2b66d632443 100644 --- a/app/controllers/two_factor_authentication/sms_opt_in_controller.rb +++ b/app/controllers/two_factor_authentication/sms_opt_in_controller.rb @@ -65,7 +65,7 @@ def load_phone def other_options_mfa_url if new_user? - two_factor_options_path + authentication_methods_setup_path elsif has_other_auth_methods? && !user_fully_authenticated? login_two_factor_options_path end diff --git a/app/controllers/users/two_factor_authentication_controller.rb b/app/controllers/users/two_factor_authentication_controller.rb index 2101e98867c..f9ef0680840 100644 --- a/app/controllers/users/two_factor_authentication_controller.rb +++ b/app/controllers/users/two_factor_authentication_controller.rb @@ -54,7 +54,7 @@ def redirect_on_nothing_enabled if MfaPolicy.new(current_user).two_factor_enabled? redirect_to login_two_factor_options_path else - redirect_to two_factor_options_url + redirect_to authentication_methods_setup_url end end @@ -308,7 +308,7 @@ def handle_too_many_confirmation_sends if user_fully_authenticated? redirect_to account_url else - redirect_to two_factor_options_url + redirect_to authentication_methods_setup_url end end end diff --git a/app/controllers/users/two_factor_authentication_setup_controller.rb b/app/controllers/users/two_factor_authentication_setup_controller.rb index 0ed30946a07..2177883bc12 100644 --- a/app/controllers/users/two_factor_authentication_setup_controller.rb +++ b/app/controllers/users/two_factor_authentication_setup_controller.rb @@ -22,14 +22,14 @@ def create elsif (result.errors[:selection].include? 'phone') || IdentityConfig.store.kantara_2fa_phone_restricted flash[:phone_error] = t('errors.two_factor_auth_setup.must_select_additional_option') - redirect_to two_factor_options_path(anchor: 'select_phone') + redirect_to authentication_methods_setup_path(anchor: 'select_phone') else @presenter = two_factor_options_presenter render :index end rescue ActionController::ParameterMissing flash[:error] = t('errors.two_factor_auth_setup.must_select_option') - redirect_back(fallback_location: two_factor_options_path, allow_other_host: false) + redirect_back(fallback_location: authentication_methods_setup_path, allow_other_host: false) end private diff --git a/app/presenters/cancellation_presenter.rb b/app/presenters/cancellation_presenter.rb index 209e04529f6..8172dacb267 100644 --- a/app/presenters/cancellation_presenter.rb +++ b/app/presenters/cancellation_presenter.rb @@ -9,7 +9,7 @@ def initialize(referer:, url_options:) end def go_back_path - referer_path || two_factor_options_path + referer_path || authentication_methods_setup_path end def url_options diff --git a/app/views/idv/doc_auth/welcome.html.erb b/app/views/idv/doc_auth/welcome.html.erb index f93e9253cc8..dd10278335d 100644 --- a/app/views/idv/doc_auth/welcome.html.erb +++ b/app/views/idv/doc_auth/welcome.html.erb @@ -89,7 +89,7 @@ <%= render 'shared/cancel', link: idv_cancel_path(step: 'welcome') %> <% else %>
- <%= link_to(t('two_factor_authentication.choose_another_option'), two_factor_options_path) %> + <%= link_to(t('two_factor_authentication.choose_another_option'), authentication_methods_setup_path) %>
<% end %> <% end %> diff --git a/app/views/shared/_cancel_or_back_to_options.html.erb b/app/views/shared/_cancel_or_back_to_options.html.erb index 24e1043e479..0e8e4222890 100644 --- a/app/views/shared/_cancel_or_back_to_options.html.erb +++ b/app/views/shared/_cancel_or_back_to_options.html.erb @@ -2,6 +2,6 @@ <% if MfaPolicy.new(current_user).two_factor_enabled? %> <%= link_to t('links.cancel'), account_path %> <% else %> - <%= link_to t('two_factor_authentication.choose_another_option'), two_factor_options_path %> + <%= link_to t('two_factor_authentication.choose_another_option'), authentication_methods_setup_path %> <% end %> <% end %> diff --git a/app/views/users/phone_setup/index.html.erb b/app/views/users/phone_setup/index.html.erb index 26db6c61c61..cb8091f16fd 100644 --- a/app/views/users/phone_setup/index.html.erb +++ b/app/views/users/phone_setup/index.html.erb @@ -40,5 +40,5 @@ <% end %> <%= render PageFooterComponent.new do %> - <%= link_to t('two_factor_authentication.choose_another_option'), two_factor_options_path %> + <%= link_to t('two_factor_authentication.choose_another_option'), authentication_methods_setup_path %> <% end %> diff --git a/app/views/users/piv_cac_authentication_setup/error.html.erb b/app/views/users/piv_cac_authentication_setup/error.html.erb index 1854d359069..4556e2298f7 100644 --- a/app/views/users/piv_cac_authentication_setup/error.html.erb +++ b/app/views/users/piv_cac_authentication_setup/error.html.erb @@ -12,6 +12,6 @@ <% if MfaPolicy.new(current_user).two_factor_enabled? %> <%= link_to t('links.cancel'), account_path %> <% else %> - <%= link_to t('two_factor_authentication.choose_another_option'), two_factor_options_path %> + <%= link_to t('two_factor_authentication.choose_another_option'), authentication_methods_setup_path %> <% end %> <% end %> diff --git a/app/views/users/two_factor_authentication_setup/index.html.erb b/app/views/users/two_factor_authentication_setup/index.html.erb index 09efca23017..a77bcfc27f0 100644 --- a/app/views/users/two_factor_authentication_setup/index.html.erb +++ b/app/views/users/two_factor_authentication_setup/index.html.erb @@ -17,7 +17,7 @@ <%= validated_form_for @two_factor_options_form, html: { autocomplete: 'off' }, method: :patch, - url: two_factor_options_path do |f| %> + url: authentication_methods_setup_path do |f| %>
<%= @presenter.intro %> diff --git a/config/routes.rb b/config/routes.rb index 28e67cec754..b89f9f8cf8f 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -222,9 +222,10 @@ post '/account/personal_key' => 'accounts/personal_keys#create' get '/otp/send' => 'users/two_factor_authentication#send_code' + get '/authentication_methods_setup' => 'users/two_factor_authentication_setup#index' patch '/authentication_methods_setup' => 'users/two_factor_authentication_setup#create' - get '/two_factor_options' => 'users/two_factor_authentication_setup#index' + get '/two_factor_options', to: redirect('/authentication_methods_setup') patch '/two_factor_options' => 'users/two_factor_authentication_setup#create' get '/second_mfa_setup' => 'users/mfa_selection#index' patch '/second_mfa_setup' => 'users/mfa_selection#update' diff --git a/spec/controllers/account_reset/request_controller_spec.rb b/spec/controllers/account_reset/request_controller_spec.rb index 4c5b54290f0..cf4cd94dd66 100644 --- a/spec/controllers/account_reset/request_controller_spec.rb +++ b/spec/controllers/account_reset/request_controller_spec.rb @@ -21,7 +21,7 @@ stub_sign_in_before_2fa get :show - expect(response).to redirect_to two_factor_options_url + expect(response).to redirect_to authentication_methods_setup_url end it 'logs the visit to analytics' do @@ -100,7 +100,7 @@ stub_sign_in_before_2fa post :create - expect(response).to redirect_to two_factor_options_url + expect(response).to redirect_to authentication_methods_setup_url end end end diff --git a/spec/controllers/application_controller_spec.rb b/spec/controllers/application_controller_spec.rb index bef613d7310..70d74a5c73a 100644 --- a/spec/controllers/application_controller_spec.rb +++ b/spec/controllers/application_controller_spec.rb @@ -184,7 +184,7 @@ def index get :index - expect(response).to redirect_to two_factor_options_url + expect(response).to redirect_to authentication_methods_setup_url end end diff --git a/spec/controllers/two_factor_authentication/personal_key_verification_controller_spec.rb b/spec/controllers/two_factor_authentication/personal_key_verification_controller_spec.rb index ee5018ecf41..fb0fc9f30b8 100644 --- a/spec/controllers/two_factor_authentication/personal_key_verification_controller_spec.rb +++ b/spec/controllers/two_factor_authentication/personal_key_verification_controller_spec.rb @@ -35,7 +35,7 @@ get :show expect(response.status).to eq(302) - expect(response.location).to eq(two_factor_options_url) + expect(response.location).to eq(authentication_methods_setup_url) end end @@ -90,7 +90,7 @@ post :create, params: { personal_key_form: { personal_key: raw_key } } expect(response.status).to eq(302) - expect(response.location).to eq(two_factor_options_url) + expect(response.location).to eq(authentication_methods_setup_url) end context 'when the personal key field is empty' do diff --git a/spec/controllers/users/two_factor_authentication_controller_spec.rb b/spec/controllers/users/two_factor_authentication_controller_spec.rb index 84e9a9b01c4..01c478e2526 100644 --- a/spec/controllers/users/two_factor_authentication_controller_spec.rb +++ b/spec/controllers/users/two_factor_authentication_controller_spec.rb @@ -236,7 +236,7 @@ def index stub_sign_in_before_2fa(build(:user)) get :show - expect(response).to redirect_to two_factor_options_url + expect(response).to redirect_to authentication_methods_setup_url end end @@ -265,7 +265,7 @@ def index it 'redirects to MFA setup if no PIV/CAC is enabled' do get :show - expect(response).to redirect_to(two_factor_options_url) + expect(response).to redirect_to(authentication_methods_setup_url) end end end @@ -506,7 +506,7 @@ def index timeout: timeout, ), ) - expect(response).to redirect_to two_factor_options_url + expect(response).to redirect_to authentication_methods_setup_url end end diff --git a/spec/controllers/users/two_factor_authentication_setup_controller_spec.rb b/spec/controllers/users/two_factor_authentication_setup_controller_spec.rb index 139e3700ee6..d0c9ea0d814 100644 --- a/spec/controllers/users/two_factor_authentication_setup_controller_spec.rb +++ b/spec/controllers/users/two_factor_authentication_setup_controller_spec.rb @@ -113,7 +113,7 @@ end it 'the redirect to the form page with an anchor' do - expect(response).to redirect_to(two_factor_options_path(anchor: 'select_phone')) + expect(response).to redirect_to(authentication_methods_setup_path(anchor: 'select_phone')) end it 'contains a flash message' do expect(flash[:phone_error]).to eq( diff --git a/spec/features/accessibility/user_pages_spec.rb b/spec/features/accessibility/user_pages_spec.rb index d96d5120b57..788fe3e789a 100644 --- a/spec/features/accessibility/user_pages_spec.rb +++ b/spec/features/accessibility/user_pages_spec.rb @@ -37,7 +37,7 @@ scenario 'two factor options page' do sign_up_and_set_password - expect(current_path).to eq(two_factor_options_path) + expect(current_path).to eq(authentication_methods_setup_path) expect(page).to be_axe_clean.according_to :section508, :"best-practice", :wcag21aa expect(page).to label_required_fields expect(page).to be_uniquely_titled diff --git a/spec/features/openid_connect/aal3_required_spec.rb b/spec/features/openid_connect/aal3_required_spec.rb index 78d6795c931..1d7e77734c5 100644 --- a/spec/features/openid_connect/aal3_required_spec.rb +++ b/spec/features/openid_connect/aal3_required_spec.rb @@ -11,7 +11,7 @@ visit_idp_from_ial1_oidc_sp_requesting_aal3(prompt: 'select_account') sign_in_live_with_2fa(user) - expect(current_url).to eq(two_factor_options_url) + expect(current_url).to eq(authentication_methods_setup_url) expect(page).to have_content(t('two_factor_authentication.two_factor_aal3_choice')) expect(page).to have_xpath("//img[@alt='important alert icon']") end @@ -43,7 +43,7 @@ visit_idp_from_ial1_oidc_sp_defaulting_to_aal3(prompt: 'select_account') sign_in_live_with_2fa(user) - expect(current_url).to eq(two_factor_options_url) + expect(current_url).to eq(authentication_methods_setup_url) expect(page).to have_content(t('two_factor_authentication.two_factor_aal3_choice')) expect(page).to have_xpath("//img[@alt='important alert icon']") end diff --git a/spec/features/phone/confirmation_spec.rb b/spec/features/phone/confirmation_spec.rb index faf84bb34cc..ad503c58c32 100644 --- a/spec/features/phone/confirmation_spec.rb +++ b/spec/features/phone/confirmation_spec.rb @@ -28,7 +28,7 @@ def expect_successful_otp_confirmation(delivery_method) def expect_failed_otp_confirmation(_delivery_method) visit account_path - expect(current_path).to eq(two_factor_options_path) + expect(current_path).to eq(authentication_methods_setup_path) expect(phone_configuration).to be_nil end end diff --git a/spec/features/saml/aal3_required_spec.rb b/spec/features/saml/aal3_required_spec.rb index 2b72e43d460..1f56478d872 100644 --- a/spec/features/saml/aal3_required_spec.rb +++ b/spec/features/saml/aal3_required_spec.rb @@ -21,7 +21,7 @@ sign_in_and_2fa_user(user_with_2fa) visit_saml_authn_request_url(overrides: { issuer: aal3_issuer, authn_context: nil }) - expect(current_url).to eq(two_factor_options_url) + expect(current_url).to eq(authentication_methods_setup_url) expect(page).to have_content(t('two_factor_authentication.two_factor_aal3_choice')) expect(page).to have_xpath("//img[@alt='important alert icon']") end diff --git a/spec/features/saml/ial1_sso_spec.rb b/spec/features/saml/ial1_sso_spec.rb index c61b645dd4c..6608886644f 100644 --- a/spec/features/saml/ial1_sso_spec.rb +++ b/spec/features/saml/ial1_sso_spec.rb @@ -148,7 +148,7 @@ visit saml_authn_request_url - expect(current_path).to eq two_factor_options_path + expect(current_path).to eq authentication_methods_setup_path end end diff --git a/spec/features/saml/saml_spec.rb b/spec/features/saml/saml_spec.rb index 9b8a2df2485..0905d80b170 100644 --- a/spec/features/saml/saml_spec.rb +++ b/spec/features/saml/saml_spec.rb @@ -74,7 +74,7 @@ class MockSession; end end it 'prompts the user to set up 2FA' do - expect(current_path).to eq two_factor_options_path + expect(current_path).to eq authentication_methods_setup_path end it 'prompts the user to confirm phone after setting up 2FA' do diff --git a/spec/features/two_factor_authentication/multiple_mfa_sign_up_spec.rb b/spec/features/two_factor_authentication/multiple_mfa_sign_up_spec.rb index 44f35030338..6158711d305 100644 --- a/spec/features/two_factor_authentication/multiple_mfa_sign_up_spec.rb +++ b/spec/features/two_factor_authentication/multiple_mfa_sign_up_spec.rb @@ -10,7 +10,7 @@ scenario 'user can set up 2 MFA methods properly' do sign_in_before_2fa - expect(current_path).to eq two_factor_options_path + expect(current_path).to eq authentication_methods_setup_path click_2fa_option('phone') click_2fa_option('backup_code') @@ -43,7 +43,7 @@ scenario 'user can select 1 MFA methods and will be prompted to add another method' do sign_in_before_2fa - expect(current_path).to eq two_factor_options_path + expect(current_path).to eq authentication_methods_setup_path click_2fa_option('backup_code') @@ -81,7 +81,7 @@ to have_content(t('errors.two_factor_auth_setup.must_select_additional_option')) expect( URI.parse(current_url).path + '#' + URI.parse(current_url).fragment, - ).to eq two_factor_options_path(anchor: 'select_phone') + ).to eq authentication_methods_setup_path(anchor: 'select_phone') end scenario 'clears the error when another mfa method is selected' do diff --git a/spec/features/two_factor_authentication/sign_in_spec.rb b/spec/features/two_factor_authentication/sign_in_spec.rb index 78d898b5c91..a91ff93fd0d 100644 --- a/spec/features/two_factor_authentication/sign_in_spec.rb +++ b/spec/features/two_factor_authentication/sign_in_spec.rb @@ -7,7 +7,7 @@ attempt_to_bypass_2fa_setup - expect(current_path).to eq two_factor_options_path + expect(current_path).to eq authentication_methods_setup_path select_2fa_option('phone') @@ -55,7 +55,7 @@ click_on t('two_factor_authentication.choose_another_option') - expect(current_path).to eq two_factor_options_path + expect(current_path).to eq authentication_methods_setup_path end end diff --git a/spec/features/users/sign_in_spec.rb b/spec/features/users/sign_in_spec.rb index 3b8ae651141..0003f022f1b 100644 --- a/spec/features/users/sign_in_spec.rb +++ b/spec/features/users/sign_in_spec.rb @@ -143,7 +143,7 @@ submit_form_with_valid_email(email) click_confirmation_link_in_email(email) submit_form_with_valid_password - expect(page).to have_current_path(two_factor_options_path) + expect(page).to have_current_path(authentication_methods_setup_path) select_2fa_option('phone') fill_in :new_phone_form_phone, with: '2025551314' @@ -930,7 +930,7 @@ fill_in_code_with_last_phone_otp click_submit_default - expect(current_path).to eq two_factor_options_path + expect(current_path).to eq authentication_methods_setup_path select_2fa_option('piv_cac') expect(page).to have_current_path setup_piv_cac_path diff --git a/spec/features/users/sign_up_spec.rb b/spec/features/users/sign_up_spec.rb index aaa40ea4cc6..8bb943b45b8 100644 --- a/spec/features/users/sign_up_spec.rb +++ b/spec/features/users/sign_up_spec.rb @@ -124,7 +124,7 @@ Throttle.attempt_window_in_minutes(:phone_confirmation).minutes, ) - expect(current_path).to eq(two_factor_options_path) + expect(current_path).to eq(authentication_methods_setup_path) expect(page).to have_content( I18n.t( 'errors.messages.phone_confirmation_throttled', @@ -303,7 +303,7 @@ click_confirmation_link_in_email(email) submit_form_with_valid_password - expect(page).to have_current_path(two_factor_options_path) + expect(page).to have_current_path(authentication_methods_setup_path) end end end @@ -322,7 +322,7 @@ it 'redirects back with an error if the user does not select 2FA option' do sign_in_user - visit two_factor_options_path + visit authentication_methods_setup_path click_on 'Continue' expect(page).to have_content(t('errors.two_factor_auth_setup.must_select_option')) diff --git a/spec/features/visitors/email_confirmation_spec.rb b/spec/features/visitors/email_confirmation_spec.rb index 7ad04872442..622efcc2f32 100644 --- a/spec/features/visitors/email_confirmation_spec.rb +++ b/spec/features/visitors/email_confirmation_spec.rb @@ -17,7 +17,7 @@ fill_in t('forms.password'), with: Features::SessionHelper::VALID_PASSWORD click_button t('forms.buttons.continue') - expect(current_url).to eq two_factor_options_url + expect(current_url).to eq authentication_methods_setup_url expect(page).to_not have_content t('devise.confirmations.confirmed_but_must_set_password') end diff --git a/spec/features/visitors/password_recovery_spec.rb b/spec/features/visitors/password_recovery_spec.rb index 131bf942438..8d2ec6fe37f 100644 --- a/spec/features/visitors/password_recovery_spec.rb +++ b/spec/features/visitors/password_recovery_spec.rb @@ -54,7 +54,7 @@ fill_in_credentials_and_submit(user.email, 'NewVal!dPassw0rd') - expect(current_path).to eq two_factor_options_path + expect(current_path).to eq authentication_methods_setup_path end end @@ -91,7 +91,7 @@ it 'prompts user to set up their 2FA options after signing back in' do reset_password_and_sign_back_in(@user) - expect(current_path).to eq two_factor_options_path + expect(current_path).to eq authentication_methods_setup_path end end diff --git a/spec/features/webauthn/sign_up_spec.rb b/spec/features/webauthn/sign_up_spec.rb index 8446481ef18..c421c1c284f 100644 --- a/spec/features/webauthn/sign_up_spec.rb +++ b/spec/features/webauthn/sign_up_spec.rb @@ -18,7 +18,7 @@ def expect_webauthn_setup_success def expect_webauthn_setup_error expect(page).to have_content t('errors.webauthn_setup.general_error') - expect(page).to have_current_path(two_factor_options_path) + expect(page).to have_current_path(authentication_methods_setup_path) end it_behaves_like 'webauthn setup' diff --git a/spec/presenters/cancellation_presenter_spec.rb b/spec/presenters/cancellation_presenter_spec.rb index d56f7794acc..3200f402a01 100644 --- a/spec/presenters/cancellation_presenter_spec.rb +++ b/spec/presenters/cancellation_presenter_spec.rb @@ -8,7 +8,7 @@ subject { described_class.new(referer: referer_header, url_options: {}) } describe '#go_back_link' do - let(:sign_up_path) { '/two_factor_options' } + let(:sign_up_path) { '/authentication_methods_setup' } context 'without a referer header' do let(:referer_header) { nil } diff --git a/spec/support/features/session_helper.rb b/spec/support/features/session_helper.rb index 34ffe9b3e77..fd55a2e2cb1 100644 --- a/spec/support/features/session_helper.rb +++ b/spec/support/features/session_helper.rb @@ -549,7 +549,7 @@ def set_up_2fa_with_authenticator_app def register_user_with_piv_cac(email = 'test@test.com') confirm_email_and_password(email) - expect(page).to have_current_path two_factor_options_path + expect(page).to have_current_path authentication_methods_setup_path expect(page).to have_content( t('two_factor_authentication.two_factor_choice_options.piv_cac'), ) diff --git a/spec/views/phone_setup/index.html.erb_spec.rb b/spec/views/phone_setup/index.html.erb_spec.rb index c8fd92fc000..7f1f223a877 100644 --- a/spec/views/phone_setup/index.html.erb_spec.rb +++ b/spec/views/phone_setup/index.html.erb_spec.rb @@ -23,7 +23,7 @@ it 'renders a link to choose a different option' do expect(render).to have_link( t('two_factor_authentication.choose_another_option'), - href: two_factor_options_path, + href: authentication_methods_setup_path, ) end diff --git a/spec/views/users/piv_cac_authentication_setup/new.html.erb_spec.rb b/spec/views/users/piv_cac_authentication_setup/new.html.erb_spec.rb index 3f3bcf9ab25..0bce6af8bb4 100644 --- a/spec/views/users/piv_cac_authentication_setup/new.html.erb_spec.rb +++ b/spec/views/users/piv_cac_authentication_setup/new.html.erb_spec.rb @@ -27,7 +27,7 @@ expect(rendered).to have_link( t('two_factor_authentication.choose_another_option'), - href: two_factor_options_path, + href: authentication_methods_setup_path, ) end end diff --git a/spec/views/users/totp_setup/new.html.erb_spec.rb b/spec/views/users/totp_setup/new.html.erb_spec.rb index d0a669b075f..22220251bb1 100644 --- a/spec/views/users/totp_setup/new.html.erb_spec.rb +++ b/spec/views/users/totp_setup/new.html.erb_spec.rb @@ -71,7 +71,7 @@ expect(rendered).to have_link( t('two_factor_authentication.choose_another_option'), - href: two_factor_options_path, + href: authentication_methods_setup_path, ) end end From 22e940ce8ff7bc55f7674b7782d18c88e16f7618 Mon Sep 17 00:00:00 2001 From: Andrew Duthie Date: Thu, 2 Jun 2022 10:33:26 -0400 Subject: [PATCH 05/28] Generate completion URL on password confirm submission (IdV app) (#6433) * Generate completion URL on password confirm submission (IdV app) **Why**: As we continue to absorb more steps into the IdV application flow, it will not be possible to determine where the user should be redirected, since it can depend on actions the user takes during the flow itself (e.g. opting to verify address by mail). Therefore, we should wait to determine completion URL until those factors have been accounted for. changelog: Upcoming Features, Identity Verification, Add password confirmation step * Remove outdated code concerning completion URL Per intent of changeset, completion URL is no longer handled in VerifyController --- .../api/verify/password_confirm_controller.rb | 17 ++++++++-- app/controllers/verify_controller.rb | 11 ------ .../steps/password-confirm/submit.spec.ts | 11 ++++-- .../steps/password-confirm/submit.ts | 15 ++++++-- .../packages/verify-flow/verify-flow.spec.tsx | 8 +++-- .../packages/verify-flow/verify-flow.tsx | 8 +++-- app/javascript/packs/verify-flow.tsx | 12 +++---- .../password_confirm_controller_spec.rb | 33 ++++++++++++++++-- spec/controllers/verify_controller_spec.rb | 34 ------------------- 9 files changed, 81 insertions(+), 68 deletions(-) diff --git a/app/controllers/api/verify/password_confirm_controller.rb b/app/controllers/api/verify/password_confirm_controller.rb index ceae90b1d51..19bd41c51ee 100644 --- a/app/controllers/api/verify/password_confirm_controller.rb +++ b/app/controllers/api/verify/password_confirm_controller.rb @@ -14,9 +14,10 @@ def create if result.success? user = User.find_by(uuid: result.extra[:user_uuid]) add_proofing_component(user) - render json: { personal_key: personal_key, - profile_pending: result.extra[:profile_pending] }, - status: :ok + render json: { + personal_key: personal_key, + completion_url: completion_url(result), + } else render json: { error: result.errors }, status: :bad_request end @@ -31,6 +32,16 @@ def verify_params def add_proofing_component(user) ProofingComponent.create_or_find_by(user: user).update(verified_at: Time.zone.now) end + + def completion_url(result) + if result.extra[:profile_pending] + idv_come_back_later_url + elsif current_sp + sign_up_completed_url + else + account_url + end + end end end end diff --git a/app/controllers/verify_controller.rb b/app/controllers/verify_controller.rb index 702ea1284b4..7ea0dbbc818 100644 --- a/app/controllers/verify_controller.rb +++ b/app/controllers/verify_controller.rb @@ -26,7 +26,6 @@ def app_data base_path: idv_app_path, start_over_url: idv_session_path, cancel_url: idv_cancel_path, - completion_url: completion_url, initial_values: initial_values, reset_password_url: forgot_password_url, enabled_step_names: IdentityConfig.store.idv_api_enabled_steps, @@ -76,16 +75,6 @@ def generate_personal_key idv_session.profile.encrypt_recovery_pii(cacher.fetch) end - def completion_url - if idv_session.address_verification_mechanism == 'gpo' - idv_come_back_later_url - elsif session[:sp] - sign_up_completed_url - else - account_url - end - end - def user_bundle_token Idv::UserBundleTokenizer.new( user: current_user, diff --git a/app/javascript/packages/verify-flow/steps/password-confirm/submit.spec.ts b/app/javascript/packages/verify-flow/steps/password-confirm/submit.spec.ts index 8238940081c..ebf6d59c81b 100644 --- a/app/javascript/packages/verify-flow/steps/password-confirm/submit.spec.ts +++ b/app/javascript/packages/verify-flow/steps/password-confirm/submit.spec.ts @@ -14,14 +14,21 @@ describe('submit', () => { sandbox.match({ body: JSON.stringify({ user_bundle_token: '..', password: 'hunter2' }) }), ) .resolves({ - json: () => Promise.resolve({ personal_key: '0000-0000-0000-0000' }), + json: () => + Promise.resolve({ + personal_key: '0000-0000-0000-0000', + completion_url: 'http://example.com', + }), } as Response); }); it('sends with password confirmation values', async () => { const patch = await submit({ userBundleToken: '..', password: 'hunter2' }); - expect(patch).to.deep.equal({ personalKey: '0000-0000-0000-0000' }); + expect(patch).to.deep.equal({ + personalKey: '0000-0000-0000-0000', + completionURL: 'http://example.com', + }); }); }); diff --git a/app/javascript/packages/verify-flow/steps/password-confirm/submit.ts b/app/javascript/packages/verify-flow/steps/password-confirm/submit.ts index 0212e28b1f9..34db4f51a6f 100644 --- a/app/javascript/packages/verify-flow/steps/password-confirm/submit.ts +++ b/app/javascript/packages/verify-flow/steps/password-confirm/submit.ts @@ -13,7 +13,15 @@ export class PasswordSubmitError extends FormError {} * Successful API response shape. */ interface PasswordConfirmSuccessResponse { + /** + * Personal key generated for the user profile. + */ personal_key: string; + + /** + * Final redirect URL for this verification session. + */ + completion_url: string; } /** @@ -26,7 +34,10 @@ type PasswordConfirmErrorResponse = ErrorResponse<'password'>; */ type PasswordConfirmResponse = PasswordConfirmSuccessResponse | PasswordConfirmErrorResponse; -async function submit({ userBundleToken, password }: VerifyFlowValues) { +async function submit({ + userBundleToken, + password, +}: VerifyFlowValues): Promise> { const payload = { user_bundle_token: userBundleToken, password }; const json = await post(API_ENDPOINT, payload, { json: true, @@ -38,7 +49,7 @@ async function submit({ userBundleToken, password }: VerifyFlowValues) { throw new PasswordSubmitError(error, { field }); } - return { personalKey: json.personal_key }; + return { personalKey: json.personal_key, completionURL: json.completion_url }; } export default submit; diff --git a/app/javascript/packages/verify-flow/verify-flow.spec.tsx b/app/javascript/packages/verify-flow/verify-flow.spec.tsx index 80f48d8b575..8fc5a5f9ce1 100644 --- a/app/javascript/packages/verify-flow/verify-flow.spec.tsx +++ b/app/javascript/packages/verify-flow/verify-flow.spec.tsx @@ -1,3 +1,4 @@ +import sinon from 'sinon'; import { render } from '@testing-library/react'; import userEvent from '@testing-library/user-event'; import * as analytics from '@18f/identity-analytics'; @@ -12,7 +13,8 @@ describe('VerifyFlow', () => { beforeEach(() => { sandbox.spy(analytics, 'trackEvent'); sandbox.stub(window, 'fetch').resolves({ - json: () => Promise.resolve({ personal_key: personalKey }), + json: () => + Promise.resolve({ personal_key: personalKey, completion_url: 'http://example.com' }), } as Response); document.body.innerHTML = ``; }); @@ -50,7 +52,9 @@ describe('VerifyFlow', () => { await userEvent.type(getByLabelText('forms.personal_key.confirmation_label'), personalKey); await userEvent.keyboard('{Enter}'); - expect(onComplete).to.have.been.called(); + expect(onComplete).to.have.been.calledWith( + sinon.match({ completionURL: 'http://example.com' }), + ); expect(sessionStorage.getItem('completedStep')).to.be.null(); }); diff --git a/app/javascript/packages/verify-flow/verify-flow.tsx b/app/javascript/packages/verify-flow/verify-flow.tsx index e49dc4e5961..99aaf291ddc 100644 --- a/app/javascript/packages/verify-flow/verify-flow.tsx +++ b/app/javascript/packages/verify-flow/verify-flow.tsx @@ -41,6 +41,8 @@ export interface VerifyFlowValues { password?: string; dob?: string; + + completionURL?: string; } interface VerifyFlowProps { @@ -77,7 +79,7 @@ interface VerifyFlowProps { /** * Callback invoked after completing the form. */ - onComplete: () => void; + onComplete: (values: VerifyFlowValues) => void; } /** @@ -128,9 +130,9 @@ function VerifyFlow({ setCompletedStep(stepName); } - function onFormComplete() { + function onFormComplete(values: VerifyFlowValues) { setCompletedStep(null); - onComplete(); + onComplete(values); } return ( diff --git a/app/javascript/packs/verify-flow.tsx b/app/javascript/packs/verify-flow.tsx index 79835984d90..9523e222c25 100644 --- a/app/javascript/packs/verify-flow.tsx +++ b/app/javascript/packs/verify-flow.tsx @@ -33,11 +33,6 @@ interface AppRootValues { */ cancelUrl: string; - /** - * URL to which user should be redirected after completing the form. - */ - completionUrl: string; - /** * Base64-encoded encryption key for secret session store. */ @@ -70,7 +65,6 @@ const { basePath, startOverUrl: startOverURL, cancelUrl: cancelURL, - completionUrl: completionURL, storeKey: storeKeyBase64, } = appRoot.dataset; const storeKey = s2ab(atob(storeKeyBase64)); @@ -102,9 +96,11 @@ const storage = new SecretSessionStorage('verify'); initialAddressVerificationMethod = metadata.address_verification_mechanism; } - function onComplete() { + function onComplete({ completionURL }: VerifyFlowValues) { storage.clear(); - window.location.href = completionURL; + if (completionURL) { + window.location.href = completionURL; + } } render( diff --git a/spec/controllers/api/verify/password_confirm_controller_spec.rb b/spec/controllers/api/verify/password_confirm_controller_spec.rb index da778bdf00f..80b9287235d 100644 --- a/spec/controllers/api/verify/password_confirm_controller_spec.rb +++ b/spec/controllers/api/verify/password_confirm_controller_spec.rb @@ -28,7 +28,8 @@ def stub_idv_session let(:profile) { subject.idv_session.profile } let(:key) { OpenSSL::PKey::RSA.new(Base64.strict_decode64(IdentityConfig.store.idv_private_key)) } - let(:jwt) { JWT.encode({ pii: pii, metadata: {} }, key, 'RS256', sub: user.uuid) } + let(:jwt_metadata) { { vendor_phone_confirmation: true, user_phone_confirmation: true } } + let(:jwt) { JWT.encode({ pii: pii, metadata: jwt_metadata }, key, 'RS256', sub: user.uuid) } before do allow(IdentityConfig.store).to receive(:idv_api_enabled_steps).and_return(['password_confirm']) @@ -53,9 +54,13 @@ def stub_idv_session stub_idv_session end - it 'creates a profile and returns a key' do + it 'creates a profile and returns a key and completion url' do post :create, params: { password: 'iambatman', user_bundle_token: jwt } - expect(JSON.parse(response.body)['personal_key']).not_to be_nil + parsed_body = JSON.parse(response.body) + expect(parsed_body).to include( + 'personal_key' => kind_of(String), + 'completion_url' => account_url, + ) expect(response.status).to eq 200 end @@ -66,6 +71,28 @@ def stub_idv_session expect(response_json['error']['password']).to eq([I18n.t('idv.errors.incorrect_password')]) expect(response.status).to eq 400 end + + context 'with associated sp session' do + before do + session[:sp] = { issuer: create(:service_provider).issuer } + end + + it 'creates a profile and returns completion url' do + post :create, params: { password: 'iambatman', user_bundle_token: jwt } + + expect(JSON.parse(response.body)['completion_url']).to eq(sign_up_completed_url) + end + end + + context 'with pending profile' do + let(:jwt_metadata) { { vendor_phone_confirmation: false, user_phone_confirmation: false } } + + it 'creates a profile and returns completion url' do + post :create, params: { password: 'iambatman', user_bundle_token: jwt } + + expect(JSON.parse(response.body)['completion_url']).to eq(idv_come_back_later_url) + end + end end context 'when the idv api is not enabled' do diff --git a/spec/controllers/verify_controller_spec.rb b/spec/controllers/verify_controller_spec.rb index 39376d80ce8..c043d29ad4e 100644 --- a/spec/controllers/verify_controller_spec.rb +++ b/spec/controllers/verify_controller_spec.rb @@ -73,7 +73,6 @@ base_path: idv_app_path, start_over_url: idv_session_path, cancel_url: idv_cancel_path, - completion_url: sign_up_completed_url, enabled_step_names: idv_api_enabled_steps, initial_values: { 'personalKey' => kind_of(String) }, store_key: kind_of(String), @@ -87,38 +86,6 @@ expect(response).to render_template(:show) end end - - context 'without associated sp' do - let(:sp_session) { nil } - - it 'sets completion url' do - response - - expect(assigns[:app_data][:completion_url]).to eq(account_url) - end - end - - context 'with gpo as address verification method' do - before do - controller.idv_session.address_verification_mechanism = 'gpo' - end - - it 'sets completion url' do - response - - expect(assigns[:app_data][:completion_url]).to eq(idv_come_back_later_url) - end - - context 'without associated sp' do - let(:sp_session) { nil } - - it 'sets completion url' do - response - - expect(assigns[:app_data][:completion_url]).to eq(idv_come_back_later_url) - end - end - end end context 'with password confirmation step enabled' do @@ -136,7 +103,6 @@ base_path: idv_app_path, start_over_url: idv_session_path, cancel_url: idv_cancel_path, - completion_url: sign_up_completed_url, enabled_step_names: idv_api_enabled_steps, initial_values: { 'userBundleToken' => kind_of(String) }, store_key: kind_of(String), From 43d49f97a5e5ea9546dc0bef4c9451e98c67acf8 Mon Sep 17 00:00:00 2001 From: Andrew Duthie Date: Thu, 2 Jun 2022 10:50:18 -0400 Subject: [PATCH 06/28] Fix missing reactivation prompt for password reset with prior cancelled profile (#6439) * Fix missing reactivation prompt for password reset with prior cancelled profile **Why**: So that a user who resets their password with an active profile always has the chance to reactivate the profile. changelog: Bug Fix, Password Reset, Allow user to reactivate profile after password reset if they had cancelled a previous proofing attempt * Add timestamps for password reset profile factory To match how it works in the real-world, and to avoid being skipped for updated logic in UserDecorator#password_reset_profile * Use profile factory traits consistently Consistency, convenience, assurance that behavior of a "password reset" profile is accurate (with timestamps) --- app/decorators/user_decorator.rb | 2 +- spec/controllers/idv_controller_spec.rb | 2 +- spec/controllers/reactivate_account_controller_spec.rb | 4 ++-- .../controllers/users/verify_password_controller_spec.rb | 2 +- .../users/verify_personal_key_controller_spec.rb | 9 +++++---- spec/decorators/user_decorator_spec.rb | 8 ++++++++ spec/factories/profiles.rb | 6 ++++++ spec/forms/verify_password_form_spec.rb | 4 ++-- spec/helpers/application_helper_spec.rb | 4 ++-- spec/models/user_spec.rb | 6 +++--- 10 files changed, 31 insertions(+), 16 deletions(-) diff --git a/app/decorators/user_decorator.rb b/app/decorators/user_decorator.rb index 5245f618991..b709f1af0ec 100644 --- a/app/decorators/user_decorator.rb +++ b/app/decorators/user_decorator.rb @@ -73,7 +73,7 @@ def active_profile_newer_than_pending_profile? # This user's most recently activated profile that has also been deactivated # due to a password reset, or nil if there is no such profile def password_reset_profile - profile = user.profiles.order(activated_at: :desc).first + profile = user.profiles.where.not(activated_at: nil).order(activated_at: :desc).first profile if profile&.password_reset? end diff --git a/spec/controllers/idv_controller_spec.rb b/spec/controllers/idv_controller_spec.rb index 7ad3e74026a..a2d35c8d9b3 100644 --- a/spec/controllers/idv_controller_spec.rb +++ b/spec/controllers/idv_controller_spec.rb @@ -38,7 +38,7 @@ end it 'redirects to account recovery if user has a password reset profile' do - profile = create(:profile, deactivation_reason: :password_reset) + profile = create(:profile, :password_reset) stub_sign_in(profile.user) allow(subject.reactivate_account_session).to receive(:started?).and_return(true) diff --git a/spec/controllers/reactivate_account_controller_spec.rb b/spec/controllers/reactivate_account_controller_spec.rb index 2cbfe9662b4..b441eed9ea5 100644 --- a/spec/controllers/reactivate_account_controller_spec.rb +++ b/spec/controllers/reactivate_account_controller_spec.rb @@ -14,7 +14,7 @@ describe '#index' do context 'with a password reset profile' do - let(:profiles) { [create(:profile, deactivation_reason: :password_reset)] } + let(:profiles) { [create(:profile, :password_reset)] } it 'renders the index template' do get :index @@ -34,7 +34,7 @@ end describe '#update' do - let(:profiles) { [create(:profile, deactivation_reason: :password_reset)] } + let(:profiles) { [create(:profile, :password_reset)] } it 'redirects user to idv_url' do put :update diff --git a/spec/controllers/users/verify_password_controller_spec.rb b/spec/controllers/users/verify_password_controller_spec.rb index 5ae08df32ed..45da44b7327 100644 --- a/spec/controllers/users/verify_password_controller_spec.rb +++ b/spec/controllers/users/verify_password_controller_spec.rb @@ -21,7 +21,7 @@ end context 'with password reset profile' do - let(:profiles) { [create(:profile, deactivation_reason: :password_reset)] } + let(:profiles) { [create(:profile, :password_reset)] } let(:response_ok) { FormResponse.new(success: true, errors: {}, extra: { personal_key: key }) } let(:response_bad) { FormResponse.new(success: false, errors: {}) } let(:key) { 'key' } diff --git a/spec/controllers/users/verify_personal_key_controller_spec.rb b/spec/controllers/users/verify_personal_key_controller_spec.rb index c797d2f0fc7..c109a9d4dd2 100644 --- a/spec/controllers/users/verify_personal_key_controller_spec.rb +++ b/spec/controllers/users/verify_personal_key_controller_spec.rb @@ -22,7 +22,7 @@ end context 'with password reset profile' do - let!(:profiles) { [create(:profile, user: user, deactivation_reason: :password_reset)] } + let!(:profiles) { [create(:profile, :password_reset, user: user)] } it 'renders the `new` template' do get :new @@ -46,7 +46,7 @@ end context 'with throttle reached' do - let!(:profiles) { [create(:profile, user: user, deactivation_reason: :password_reset)] } + let!(:profiles) { [create(:profile, :password_reset, user: user)] } before do Throttle.new(throttle_type: :verify_personal_key, user: user).increment_to_throttled! @@ -74,8 +74,9 @@ [ create( :profile, - user: user, deactivation_reason: :password_reset, - pii: { ssn: '123456789' } + :password_reset, + user: user, + pii: { ssn: '123456789' }, ), ] } diff --git a/spec/decorators/user_decorator_spec.rb b/spec/decorators/user_decorator_spec.rb index 1b9925d6df9..1885026e80e 100644 --- a/spec/decorators/user_decorator_spec.rb +++ b/spec/decorators/user_decorator_spec.rb @@ -295,6 +295,14 @@ before { active_profile.deactivate(:password_reset) } it { expect(decorated_user.password_reset_profile).to eq(active_profile) } + + context 'with a previously-cancelled pending profile' do + before do + user.profiles << build(:profile, :verification_cancelled) + end + + it { expect(decorated_user.password_reset_profile).to eq(active_profile) } + end end end end diff --git a/spec/factories/profiles.rb b/spec/factories/profiles.rb index f00067b82be..843228c56f8 100644 --- a/spec/factories/profiles.rb +++ b/spec/factories/profiles.rb @@ -16,9 +16,15 @@ end trait :password_reset do + activated_at { Time.zone.now } + verified_at { Time.zone.now } deactivation_reason { :password_reset } end + trait :verification_cancelled do + deactivation_reason { :verification_cancelled } + end + trait :with_liveness do proofing_components { { liveness_check: 'vendor' } } end diff --git a/spec/forms/verify_password_form_spec.rb b/spec/forms/verify_password_form_spec.rb index b46008443f3..d6c0fbeae10 100644 --- a/spec/forms/verify_password_form_spec.rb +++ b/spec/forms/verify_password_form_spec.rb @@ -7,7 +7,7 @@ password = 'cab123DZN456' user = create(:user, password: password) pii = { ssn: '111111111' } - create(:profile, user: user, deactivation_reason: :password_reset, pii: pii) + create(:profile, :password_reset, user: user, pii: pii) form = VerifyPasswordForm.new( user: user, password: password, @@ -25,7 +25,7 @@ password = 'cab123DZN456' user = create(:user, password: password) pii = { ssn: '111111111' } - create(:profile, user: user, deactivation_reason: :password_reset, pii: pii) + create(:profile, :password_reset, user: user, pii: pii) form = VerifyPasswordForm.new( user: user, password: "#{password}a", diff --git a/spec/helpers/application_helper_spec.rb b/spec/helpers/application_helper_spec.rb index dff0b640f51..a57181a1589 100644 --- a/spec/helpers/application_helper_spec.rb +++ b/spec/helpers/application_helper_spec.rb @@ -95,7 +95,7 @@ create( :profile, :verified, - deactivation_reason: :password_reset, + :password_reset, ), ], ) @@ -115,7 +115,7 @@ :profile, :active, :verified, - deactivation_reason: :password_reset, + :password_reset, proofing_components: { liveness_check: DocAuthRouter.doc_auth_vendor, address_check: :lexis_nexis_address, diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index 4310f274a54..5fad44370b1 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -377,13 +377,13 @@ user = User.new _old_profile = create( :profile, - deactivation_reason: :verification_pending, + :verification_pending, created_at: 1.day.ago, user: user, ) new_profile = create( :profile, - deactivation_reason: :verification_pending, + :verification_pending, user: user, ) @@ -396,7 +396,7 @@ user = User.new create( :profile, - deactivation_reason: :password_reset, + :password_reset, created_at: 1.day.ago, user: user, ) From 3c13a51f3947cc85f913ee1f98cebbdc8392846d Mon Sep 17 00:00:00 2001 From: Mitchell Henke Date: Thu, 2 Jun 2022 13:22:45 -0500 Subject: [PATCH 07/28] Drop deprecated user columns (#6438) changelog: Internal, Database, Drop deprecated user columns --- app/controllers/users_controller.rb | 32 --------- .../concerns/deprecated_user_attributes.rb | 2 +- app/models/user.rb | 2 +- app/views/sign_up/cancellations/new.html.erb | 18 ++--- config/routes.rb | 3 - ...0602005747_drop_deprecated_user_columns.rb | 19 ++++++ db/schema.rb | 5 +- spec/controllers/users_controller_spec.rb | 67 ------------------- spec/support/features/session_helper.rb | 2 +- 9 files changed, 28 insertions(+), 122 deletions(-) delete mode 100644 app/controllers/users_controller.rb create mode 100644 db/primary_migrate/20220602005747_drop_deprecated_user_columns.rb delete mode 100644 spec/controllers/users_controller_spec.rb diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb deleted file mode 100644 index 10992662a80..00000000000 --- a/app/controllers/users_controller.rb +++ /dev/null @@ -1,32 +0,0 @@ -class UsersController < ApplicationController - before_action :ensure_in_setup - - def destroy - track_account_deletion_event - url_after_cancellation = decorated_session.cancel_link_url - destroy_user - flash[:success] = t('sign_up.cancel.success') - redirect_to url_after_cancellation - end - - private - - def track_account_deletion_event - properties = ParseControllerFromReferer.new(request.referer).call - analytics.account_deletion(**properties) - end - - def destroy_user - user = current_user || User.find_by(confirmation_token: session[:user_confirmation_token]) - user&.destroy! - sign_out if user - end - - def ensure_in_setup - redirect_to root_url if !session[:user_confirmation_token] && two_factor_enabled - end - - def two_factor_enabled - current_user && MfaPolicy.new(current_user).two_factor_enabled? - end -end diff --git a/app/models/concerns/deprecated_user_attributes.rb b/app/models/concerns/deprecated_user_attributes.rb index 147404d4f0d..3bb60134096 100644 --- a/app/models/concerns/deprecated_user_attributes.rb +++ b/app/models/concerns/deprecated_user_attributes.rb @@ -2,7 +2,7 @@ module DeprecatedUserAttributes extend ActiveSupport::Concern DEPRECATED_ATTRIBUTES = %i[ - email_fingerprint encrypted_email email confirmed_at confirmation_token confirmation_sent_at + email_fingerprint encrypted_email email confirmed_at ].freeze def []=(attribute, value) diff --git a/app/models/user.rb b/app/models/user.rb index 8eec4e70361..65dcc450a86 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -1,5 +1,5 @@ class User < ApplicationRecord - self.ignored_columns = %w[totp_timestamp confirmation_token confirmation_sent_at] + self.ignored_columns = %w[totp_timestamp] include NonNullUuid include ::NewRelic::Agent::MethodTracer diff --git a/app/views/sign_up/cancellations/new.html.erb b/app/views/sign_up/cancellations/new.html.erb index 386c62d24b5..0d8d6f75094 100644 --- a/app/views/sign_up/cancellations/new.html.erb +++ b/app/views/sign_up/cancellations/new.html.erb @@ -14,19 +14,11 @@
  • <%= t('users.delete.bullet_4', app_name: APP_NAME) %>
    • - <% if IdentityConfig.store.new_sign_up_cancellation_url_enabled %> - <% c.action_button( - action: ->(**tag_options, &block) do - button_to(sign_up_destroy_path, method: :delete, **tag_options, &block) - end, - ) { t('forms.buttons.cancel') } %> - <% else %> - <% c.action_button( - action: ->(**tag_options, &block) do - button_to(destroy_user_path, method: :delete, **tag_options, &block) - end, - ) { t('forms.buttons.cancel') } %> - <% end %> + <% c.action_button( + action: ->(**tag_options, &block) do + button_to(sign_up_destroy_path, method: :delete, **tag_options, &block) + end, + ) { t('forms.buttons.cancel') } %> <% c.action_button( action: ->(**tag_options, &block) do diff --git a/config/routes.rb b/config/routes.rb index b89f9f8cf8f..69a71bffffe 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -273,9 +273,6 @@ match '/sign_out' => 'sign_out#destroy', via: %i[get post delete] - # Deprecated - delete '/users' => 'users#destroy', as: :destroy_user - get '/restricted' => 'banned_user#show', as: :banned_user scope '/verify', as: 'idv' do diff --git a/db/primary_migrate/20220602005747_drop_deprecated_user_columns.rb b/db/primary_migrate/20220602005747_drop_deprecated_user_columns.rb new file mode 100644 index 00000000000..fdd37e7bb70 --- /dev/null +++ b/db/primary_migrate/20220602005747_drop_deprecated_user_columns.rb @@ -0,0 +1,19 @@ +class DropDeprecatedUserColumns < ActiveRecord::Migration[6.1] + disable_ddl_transaction! + + def up + remove_index :users, column: [:confirmation_token], name: 'index_users_on_confirmation_token', algorithm: :concurrently + + safety_assured do + remove_column :users, :confirmation_token + remove_column :users, :confirmation_sent_at + end + end + + def down + add_column :users, :confirmation_token, :text + add_column :users, :confirmation_sent_at, :datetime + + add_index :users, ['confirmation_token'], name: 'index_users_on_confirmation_token', unique: true + end +end diff --git a/db/schema.rb b/db/schema.rb index b163470de3c..2110bd7575e 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 2022_05_17_103312) do +ActiveRecord::Schema.define(version: 2022_06_02_005747) do # These are extensions that must be enabled in order to support this database enable_extension "pg_stat_statements" @@ -581,9 +581,7 @@ t.datetime "remember_created_at" t.datetime "created_at" t.datetime "updated_at" - t.string "confirmation_token", limit: 255 t.datetime "confirmed_at" - t.datetime "confirmation_sent_at" t.integer "second_factor_attempts_count", default: 0 t.string "uuid", limit: 255, null: false t.datetime "second_factor_locked_at" @@ -602,7 +600,6 @@ t.string "email_language", limit: 10 t.datetime "accepted_terms_at" t.datetime "encrypted_recovery_code_digest_generated_at" - t.index ["confirmation_token"], name: "index_users_on_confirmation_token", unique: true t.index ["reset_password_token"], name: "index_users_on_reset_password_token", unique: true t.index ["uuid"], name: "index_users_on_uuid", unique: true end diff --git a/spec/controllers/users_controller_spec.rb b/spec/controllers/users_controller_spec.rb deleted file mode 100644 index 9c8edf6cfa6..00000000000 --- a/spec/controllers/users_controller_spec.rb +++ /dev/null @@ -1,67 +0,0 @@ -require 'rails_helper' - -describe UsersController do - describe '#destroy' do - it 'redirects and displays the flash message if no user is present' do - delete :destroy - - expect(response).to redirect_to(root_url) - expect(flash.now[:success]).to eq t('sign_up.cancel.success') - end - - it 'destroys the current user and redirects to sign in page, with a helpful flash message' do - sign_in_as_user - subject.session[:user_confirmation_token] = '1' - - expect { delete :destroy }.to change(User, :count).by(-1) - expect(response).to redirect_to(root_url) - expect(flash.now[:success]).to eq t('sign_up.cancel.success') - end - - it 'does not destroy the user if the user is not in setup mode and is after 2fa' do - sign_in_as_user - - expect { delete :destroy }.to change(User, :count).by(0) - end - - it 'does not destroy the user if the user is not in setup mode and is before 2fa' do - sign_in_before_2fa - - expect { delete :destroy }.to change(User, :count).by(0) - end - - it 'redirects to the branded start page if the user came from an SP' do - session[:sp] = { issuer: 'http://localhost:3000', request_id: 'foo' } - - delete :destroy - - expect(response). - to redirect_to new_user_session_path(request_id: 'foo') - end - - it 'tracks the event in analytics when referer is nil' do - stub_analytics - properties = { request_came_from: 'no referer' } - - expect(@analytics).to receive(:track_event).with('Account Deletion Requested', properties) - - delete :destroy - end - - it 'tracks the event in analytics when referer is present' do - stub_analytics - request.env['HTTP_REFERER'] = 'http://example.com/' - properties = { request_came_from: 'users/sessions#new' } - - expect(@analytics).to receive(:track_event).with('Account Deletion Requested', properties) - - delete :destroy - end - - it 'calls ParseControllerFromReferer' do - expect_any_instance_of(ParseControllerFromReferer).to receive(:call).and_call_original - - delete :destroy - end - end -end diff --git a/spec/support/features/session_helper.rb b/spec/support/features/session_helper.rb index fd55a2e2cb1..34fc441520c 100644 --- a/spec/support/features/session_helper.rb +++ b/spec/support/features/session_helper.rb @@ -227,7 +227,7 @@ def user_with_piv_cac end def confirm_last_user - @raw_confirmation_token, = Devise.token_generator.generate(User, :confirmation_token) + @raw_confirmation_token, = Devise.token_generator.generate(EmailAddress, :confirmation_token) User.last.email_addresses.first.update( confirmation_token: @raw_confirmation_token, confirmation_sent_at: Time.zone.now, From 322a5c3a8e27877af7b79df6a61da013ea1d5c71 Mon Sep 17 00:00:00 2001 From: Andrew Duthie Date: Fri, 3 Jun 2022 08:40:07 -0400 Subject: [PATCH 08/28] Fix paste for personal key confirmation (IdV app) (#6443) * Fix paste for personal key confirmation (IdV app) **Why**: Because the primary user interaction we expect here is for the user to paste the personal key after clicking the "Copy" button on the personal key display page. changelog: Upcoming Features, Identity Verification, Add personal key step screen * Create separate variable for modifier Improve clarity See: https://github.com/18F/identity-idp/pull/6443#discussion_r888356715 --- .../personal-key-input.spec.tsx | 11 +++++++++++ .../personal-key-confirm/personal-key-input.tsx | 11 +++++++++++ spec/support/shared_examples_for_personal_keys.rb | 14 ++++++++++++++ 3 files changed, 36 insertions(+) diff --git a/app/javascript/packages/verify-flow/steps/personal-key-confirm/personal-key-input.spec.tsx b/app/javascript/packages/verify-flow/steps/personal-key-confirm/personal-key-input.spec.tsx index 828de0d5291..67616a4a328 100644 --- a/app/javascript/packages/verify-flow/steps/personal-key-confirm/personal-key-input.spec.tsx +++ b/app/javascript/packages/verify-flow/steps/personal-key-confirm/personal-key-input.spec.tsx @@ -49,6 +49,17 @@ describe('PersonalKeyInput', () => { expect(input.value).to.equal('1234-1234-1234-1234'); }); + it('allows the user to paste the personal key from their clipboard', async () => { + const { getByRole } = render(); + + const input = getByRole('textbox') as HTMLInputElement; + + input.focus(); + await userEvent.paste('1234-1234-1234-1234'); + + expect(input.value).to.equal('1234-1234-1234-1234'); + }); + it('validates the input value against the expected value (case-insensitive, crockford)', async () => { const { getByRole } = render(); diff --git a/app/javascript/packages/verify-flow/steps/personal-key-confirm/personal-key-input.tsx b/app/javascript/packages/verify-flow/steps/personal-key-confirm/personal-key-input.tsx index 03d2a835c3f..21b7bde05a0 100644 --- a/app/javascript/packages/verify-flow/steps/personal-key-confirm/personal-key-input.tsx +++ b/app/javascript/packages/verify-flow/steps/personal-key-confirm/personal-key-input.tsx @@ -1,10 +1,18 @@ import { forwardRef, useCallback } from 'react'; import type { ForwardedRef } from 'react'; import Cleave from 'cleave.js/react'; +import type { ReactInstanceWithCleave } from 'cleave.js/react/props'; import { t } from '@18f/identity-i18n'; import { ValidatedField } from '@18f/identity-validated-field'; import type { ValidatedFieldValidator } from '@18f/identity-validated-field'; +/** + * Internal Cleave.js React instance API methods. + */ +interface CleaveInstanceInternalAPI { + updateValueState: () => void; +} + interface PersonalKeyInputProps { /** * The correct personal key to validate against. @@ -42,6 +50,9 @@ function PersonalKeyInput( return ( { + (owner as ReactInstanceWithCleave & CleaveInstanceInternalAPI).updateValueState(); + }} options={{ blocks: [4, 4, 4, 4], delimiter: '-', diff --git a/spec/support/shared_examples_for_personal_keys.rb b/spec/support/shared_examples_for_personal_keys.rb index 67c32ad0010..fbaa47105a5 100644 --- a/spec/support/shared_examples_for_personal_keys.rb +++ b/spec/support/shared_examples_for_personal_keys.rb @@ -1,3 +1,5 @@ +require 'rbconfig' + shared_examples_for 'personal key page' do include PersonalKeyHelper include JavascriptDriverHelper @@ -36,6 +38,14 @@ copied_text = page.evaluate_async_script('navigator.clipboard.readText().then(arguments[0])') expect(copied_text).to eq(scrape_personal_key) + + click_continue + mod = mac? ? :meta : :control + page.find(':focus').send_keys [mod, 'v'] + + path_before_submit = current_path + within('[role=dialog]') { click_on t('forms.buttons.continue') } + expect(current_path).not_to eq path_before_submit end it 'validates as case-insensitive, crockford-normalized, length-limited, dash-flexible' do @@ -61,4 +71,8 @@ expect(current_path).not_to eq path_before_submit end end + + def mac? + RbConfig::CONFIG['host_os'].match? 'darwin' + end end From 92e2fe738c21d3cb2571ad44276db84e3977dacd Mon Sep 17 00:00:00 2001 From: Andrew Duthie Date: Fri, 3 Jun 2022 08:40:20 -0400 Subject: [PATCH 09/28] Remove support for password confirm as first step of IdV app (#6434) **Why**: It was originally implemented with the expectation that we might ship the personal key step independently, but with stabilization of the password re-entry step, it's most likely we'd ship them together now. changelog: Upcoming Features, Identity Verification, Add password confirmation step --- app/controllers/idv/review_controller.rb | 11 +-- app/controllers/verify_controller.rb | 20 ----- app/javascript/packs/verify-flow.tsx | 20 ++--- .../controllers/idv/review_controller_spec.rb | 15 +--- spec/controllers/verify_controller_spec.rb | 82 ++++--------------- 5 files changed, 25 insertions(+), 123 deletions(-) diff --git a/app/controllers/idv/review_controller.rb b/app/controllers/idv/review_controller.rb index b451c974572..78fa290f07a 100644 --- a/app/controllers/idv/review_controller.rb +++ b/app/controllers/idv/review_controller.rb @@ -120,16 +120,7 @@ def need_personal_key_confirmation? end def next_step - if idv_api_personal_key_step_enabled? - idv_app_url - else - idv_personal_key_url - end - end - - def idv_api_personal_key_step_enabled? - return false if idv_session.address_verification_mechanism == 'gpo' - IdentityConfig.store.idv_api_enabled_steps.include?('personal_key') + idv_personal_key_url end end end diff --git a/app/controllers/verify_controller.rb b/app/controllers/verify_controller.rb index 7ea0dbbc818..f172fbe3eb7 100644 --- a/app/controllers/verify_controller.rb +++ b/app/controllers/verify_controller.rb @@ -7,7 +7,6 @@ class VerifyController < ApplicationController before_action :validate_step before_action :confirm_two_factor_authenticated before_action :confirm_idv_vendor_session_started - before_action :confirm_profile_has_been_created, if: :first_step_is_personal_key? def show @app_data = app_data @@ -37,8 +36,6 @@ def initial_values case first_step when 'password_confirm' { 'userBundleToken' => user_bundle_token } - when 'personal_key' - { 'personalKey' => personal_key } end end @@ -46,10 +43,6 @@ def first_step enabled_steps.detect { |step| step_enabled?(step) } end - def first_step_is_personal_key? - first_step == 'personal_key' - end - def enabled_steps IdentityConfig.store.idv_api_enabled_steps end @@ -62,19 +55,6 @@ def random_encryption_key Encryption::AesCipher.encryption_cipher.random_key end - def confirm_profile_has_been_created - redirect_to account_url if idv_session.profile.blank? - end - - def personal_key - idv_session.personal_key || generate_personal_key - end - - def generate_personal_key - cacher = Pii::Cacher.new(current_user, user_session) - idv_session.profile.encrypt_recovery_pii(cacher.fetch) - end - def user_bundle_token Idv::UserBundleTokenizer.new( user: current_user, diff --git a/app/javascript/packs/verify-flow.tsx b/app/javascript/packs/verify-flow.tsx index 9523e222c25..a61ff269b64 100644 --- a/app/javascript/packs/verify-flow.tsx +++ b/app/javascript/packs/verify-flow.tsx @@ -37,11 +37,6 @@ interface AppRootValues { * Base64-encoded encryption key for secret session store. */ storeKey: string; - - /** - * Signed JWT containing user data. - */ - userBundleToken: string; } interface UserBundleMetadata { @@ -86,15 +81,10 @@ const storage = new SecretSessionStorage('verify'); ]); storage.key = cryptoKey; await storage.load(); - - let initialAddressVerificationMethod: AddressVerificationMethod | undefined; - if (initialValues.userBundleToken) { - const { userBundleToken } = initialValues; - await storage.setItem('userBundleToken', userBundleToken); - const { pii, metadata } = JSON.parse(atob(userBundleToken.split('.')[1])) as UserBundle; - Object.assign(initialValues, Object.fromEntries(mapKeys(pii, camelCase))); - initialAddressVerificationMethod = metadata.address_verification_mechanism; - } + const userBundleToken = initialValues.userBundleToken as string; + await storage.setItem('userBundleToken', userBundleToken); + const { pii, metadata } = JSON.parse(atob(userBundleToken.split('.')[1])) as UserBundle; + Object.assign(initialValues, Object.fromEntries(mapKeys(pii, camelCase))); function onComplete({ completionURL }: VerifyFlowValues) { storage.clear(); @@ -112,7 +102,7 @@ const storage = new SecretSessionStorage('verify'); cancelURL={cancelURL} basePath={basePath} onComplete={onComplete} - initialAddressVerificationMethod={initialAddressVerificationMethod} + initialAddressVerificationMethod={metadata.address_verification_mechanism} /> , appRoot, diff --git a/spec/controllers/idv/review_controller_spec.rb b/spec/controllers/idv/review_controller_spec.rb index bf9eb9d893e..efc94e05bd9 100644 --- a/spec/controllers/idv/review_controller_spec.rb +++ b/spec/controllers/idv/review_controller_spec.rb @@ -365,7 +365,7 @@ def show context 'with idv app personal key step enabled' do before do allow(IdentityConfig.store).to receive(:idv_api_enabled_steps). - and_return(['personal_key']) + and_return(['password_confirm', 'personal_key', 'personal_key_confirm']) end it 'redirects to idv app personal key path' do @@ -389,19 +389,6 @@ def show expect(profile).to_not be_active end - - context 'with idv api personal key step enabled' do - before do - allow(IdentityConfig.store).to receive(:idv_api_enabled_steps). - and_return(['personal_key']) - end - - it 'redirects to personal key path' do - put :create, params: { user: { password: ControllerHelper::VALID_PASSWORD } } - - expect(response).to redirect_to idv_personal_key_path - end - end end end end diff --git a/spec/controllers/verify_controller_spec.rb b/spec/controllers/verify_controller_spec.rb index c043d29ad4e..a9699062c70 100644 --- a/spec/controllers/verify_controller_spec.rb +++ b/spec/controllers/verify_controller_spec.rb @@ -36,7 +36,8 @@ end context 'with idv api enabled' do - let(:idv_api_enabled_steps) { ['something'] } + let(:idv_api_enabled_steps) { ['password_confirm', 'personal_key', 'personal_key_confirm'] } + let(:step) { 'password_confirm' } context 'invalid step' do let(:step) { 'bad' } @@ -46,76 +47,29 @@ end end - context 'with personal key step enabled' do - let(:idv_api_enabled_steps) { ['personal_key', 'personal_key_confirm'] } - let(:step) { 'personal_key' } - - before do - profile_maker = Idv::ProfileMaker.new( - applicant: applicant, - user: user, - user_password: password, - ) - profile = profile_maker.save_profile - controller.idv_session.pii = profile_maker.pii_attributes - controller.idv_session.profile_id = profile.id - controller.idv_session.personal_key = profile.personal_key - end - - it 'renders view' do - expect(response).to render_template(:show) - end - - it 'sets app data' do - response - - expect(assigns[:app_data]).to include( - base_path: idv_app_path, - start_over_url: idv_session_path, - cancel_url: idv_cancel_path, - enabled_step_names: idv_api_enabled_steps, - initial_values: { 'personalKey' => kind_of(String) }, - store_key: kind_of(String), - ) - end - - context 'empty step' do - let(:step) { nil } + it 'renders view' do + expect(response).to render_template(:show) + end - it 'renders view' do - expect(response).to render_template(:show) - end - end + it 'sets app data' do + response + + expect(assigns[:app_data]).to include( + base_path: idv_app_path, + start_over_url: idv_session_path, + cancel_url: idv_cancel_path, + enabled_step_names: idv_api_enabled_steps, + initial_values: { 'userBundleToken' => kind_of(String) }, + store_key: kind_of(String), + ) end - context 'with password confirmation step enabled' do - let(:idv_api_enabled_steps) { ['password_confirm', 'personal_key', 'personal_key_confirm'] } - let(:step) { 'password_confirm' } + context 'empty step' do + let(:step) { nil } it 'renders view' do expect(response).to render_template(:show) end - - it 'sets app data' do - response - - expect(assigns[:app_data]).to include( - base_path: idv_app_path, - start_over_url: idv_session_path, - cancel_url: idv_cancel_path, - enabled_step_names: idv_api_enabled_steps, - initial_values: { 'userBundleToken' => kind_of(String) }, - store_key: kind_of(String), - ) - end - - context 'empty step' do - let(:step) { nil } - - it 'renders view' do - expect(response).to render_template(:show) - end - end end end From d9907b433bfa87dae28f090ea212830db772dc9b Mon Sep 17 00:00:00 2001 From: Andrew Duthie Date: Fri, 3 Jun 2022 10:26:07 -0400 Subject: [PATCH 10/28] Merge content-only JavaScript-enabled feature spec cases (#6449) **Why**: Since the concern of these assertions is common, and to reduce the runtime of the specs, since each test case will spin up a fresh browser tab and complete all the pre-steps required to get to these screens. changelog: Internal, Automated Testing, Improve performance of automated tests --- .../doc_auth/document_capture_step_spec.rb | 9 ++++--- .../doc_capture/document_capture_step_spec.rb | 24 +++++++++---------- spec/features/visitors/set_password_spec.rb | 16 ++++--------- .../shared_examples_for_personal_keys.rb | 13 ++++------ 4 files changed, 23 insertions(+), 39 deletions(-) diff --git a/spec/features/idv/doc_auth/document_capture_step_spec.rb b/spec/features/idv/doc_auth/document_capture_step_spec.rb index 2ddab9ff766..7881d37c007 100644 --- a/spec/features/idv/doc_auth/document_capture_step_spec.rb +++ b/spec/features/idv/doc_auth/document_capture_step_spec.rb @@ -55,20 +55,19 @@ ) end - it 'shows the selfie upload option' do + it 'shows expected content' do + # Selfie upload option expect(page).to have_content(t('doc_auth.headings.document_capture_selfie')) - end - it 'displays doc capture tips' do + # Document capture tips expect(page).to have_content(I18n.t('doc_auth.tips.document_capture_header_text')) expect(page).to have_content(I18n.t('doc_auth.tips.document_capture_id_text1')) expect(page).to have_content(I18n.t('doc_auth.tips.document_capture_id_text2')) expect(page).to have_content(I18n.t('doc_auth.tips.document_capture_id_text3')) expect(page).to have_content(I18n.t('doc_auth.tips.document_capture_id_text4')) expect(page).to have_content(I18n.t('doc_auth.tips.document_capture_hint')) - end - it 'displays selfie tips' do + # Selfie tips expect(page).to have_content(I18n.t('doc_auth.tips.document_capture_selfie_text1')) expect(page).to have_content(I18n.t('doc_auth.tips.document_capture_selfie_text2')) expect(page).to have_content(I18n.t('doc_auth.tips.document_capture_selfie_text3')) diff --git a/spec/features/idv/doc_capture/document_capture_step_spec.rb b/spec/features/idv/doc_capture/document_capture_step_spec.rb index adc8b35965f..d466f01b729 100644 --- a/spec/features/idv/doc_capture/document_capture_step_spec.rb +++ b/spec/features/idv/doc_capture/document_capture_step_spec.rb @@ -196,16 +196,16 @@ expect(page).not_to have_content(t('doc_auth.headings.document_capture_selfie')) end - it 'displays doc capture tips' do + it 'displays expected content' do + # Doc capture tips expect(page).to have_content(I18n.t('doc_auth.tips.document_capture_header_text')) expect(page).to have_content(I18n.t('doc_auth.tips.document_capture_id_text1')) expect(page).to have_content(I18n.t('doc_auth.tips.document_capture_id_text2')) expect(page).to have_content(I18n.t('doc_auth.tips.document_capture_id_text3')) expect(page).to have_content(I18n.t('doc_auth.tips.document_capture_id_text4')) expect(page).to have_content(I18n.t('doc_auth.tips.document_capture_hint')) - end - it 'does not display selfie tips' do + # No selfie tips expect(page).not_to have_content(I18n.t('doc_auth.tips.document_capture_selfie_text1')) expect(page).not_to have_content(I18n.t('doc_auth.tips.document_capture_selfie_text2')) expect(page).not_to have_content(I18n.t('doc_auth.tips.document_capture_selfie_text3')) @@ -218,20 +218,19 @@ expect(page).to have_content(t('doc_auth.headings.document_capture_back')) end - it 'shows the selfie upload option' do + it 'displays expected content' do + # Selfie upload option expect(page).to have_content(t('doc_auth.headings.document_capture_selfie')) - end - it 'displays doc capture tips' do + # Doc capture tips expect(page).to have_content(I18n.t('doc_auth.tips.document_capture_header_text')) expect(page).to have_content(I18n.t('doc_auth.tips.document_capture_id_text1')) expect(page).to have_content(I18n.t('doc_auth.tips.document_capture_id_text2')) expect(page).to have_content(I18n.t('doc_auth.tips.document_capture_id_text3')) expect(page).to have_content(I18n.t('doc_auth.tips.document_capture_id_text4')) expect(page).to have_content(I18n.t('doc_auth.tips.document_capture_hint')) - end - it 'displays selfie tips' do + # Selfie tips expect(page).to have_content(I18n.t('doc_auth.tips.document_capture_selfie_text1')) expect(page).to have_content(I18n.t('doc_auth.tips.document_capture_selfie_text2')) expect(page).to have_content(I18n.t('doc_auth.tips.document_capture_selfie_text3')) @@ -331,20 +330,19 @@ ) end - it 'does not show the selfie upload option' do + it 'displays expected content' do + # No selfie upload option expect(page).not_to have_content(t('doc_auth.headings.document_capture_selfie')) - end - it 'displays document capture tips' do + # Document capture tips expect(page).to have_content(I18n.t('doc_auth.tips.document_capture_header_text')) expect(page).to have_content(I18n.t('doc_auth.tips.document_capture_id_text1')) expect(page).to have_content(I18n.t('doc_auth.tips.document_capture_id_text2')) expect(page).to have_content(I18n.t('doc_auth.tips.document_capture_id_text3')) expect(page).to have_content(I18n.t('doc_auth.tips.document_capture_id_text4')) expect(page).to have_content(I18n.t('doc_auth.tips.document_capture_hint')) - end - it 'does not display selfie tips' do + # No selfie tips expect(page).not_to have_content(I18n.t('doc_auth.tips.document_capture_selfie_text1')) expect(page).not_to have_content(I18n.t('doc_auth.tips.document_capture_selfie_text2')) expect(page).not_to have_content(I18n.t('doc_auth.tips.document_capture_selfie_text3')) diff --git a/spec/features/visitors/set_password_spec.rb b/spec/features/visitors/set_password_spec.rb index 9cffa3706a7..2b505cd909d 100644 --- a/spec/features/visitors/set_password_spec.rb +++ b/spec/features/visitors/set_password_spec.rb @@ -37,25 +37,17 @@ confirm_last_user end - it 'is visible on page (not have "display-none" class)' do - expect(page).to_not have_css('#pw-strength-cntnr.display-none') - end - - it 'updates as password changes' do + it 'updates strength feedback as password changes' do expect(page).to have_content '...' fill_in t('forms.password'), with: 'password' expect(page).to have_content 'Very weak' - fill_in t('forms.password'), with: 'this is a great sentence' - expect(page).to have_content 'Great!' - end - - it 'has dynamic password strength feedback' do - expect(page).to have_content '...' - fill_in t('forms.password'), with: '123456789' expect(page).to have_content t('zxcvbn.feedback.this_is_a_top_10_common_password') + + fill_in t('forms.password'), with: 'this is a great sentence' + expect(page).to have_content 'Great!' end end diff --git a/spec/support/shared_examples_for_personal_keys.rb b/spec/support/shared_examples_for_personal_keys.rb index fbaa47105a5..23fa93e881f 100644 --- a/spec/support/shared_examples_for_personal_keys.rb +++ b/spec/support/shared_examples_for_personal_keys.rb @@ -4,19 +4,14 @@ include PersonalKeyHelper include JavascriptDriverHelper - context 'informational text' do + describe 'confirmation modal' do before do click_continue if javascript_enabled? end - context 'modal content' do - it 'displays the modal title' do - expect(page).to have_content t('forms.personal_key.title') - end - - it 'displays the modal instructions' do - expect(page).to have_content t('forms.personal_key.instructions') - end + it 'displays modal content' do + expect(page).to have_content t('forms.personal_key.title') + expect(page).to have_content t('forms.personal_key.instructions') end end From 92f1ba8c3e736f756ae29185dfa72918b6eb4172 Mon Sep 17 00:00:00 2001 From: Andrew Duthie Date: Fri, 3 Jun 2022 10:26:22 -0400 Subject: [PATCH 11/28] Fix flakey OTP lockout spec (#6445) **Why**: Assertions which rely on precise timing are likely to fail due to slow processing. Instead, freeze time to avoid bounding timeframes. changelog: Internal, Automated Testing, Improve test reliability for timing precision --- ...wo_factor_authentication_controller_spec.rb | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/spec/controllers/users/two_factor_authentication_controller_spec.rb b/spec/controllers/users/two_factor_authentication_controller_spec.rb index 01c478e2526..0a33381b73a 100644 --- a/spec/controllers/users/two_factor_authentication_controller_spec.rb +++ b/spec/controllers/users/two_factor_authentication_controller_spec.rb @@ -337,14 +337,18 @@ def index it 'marks the user as locked out after too many attempts' do expect(@user.second_factor_locked_at).to be_nil - (IdentityConfig.store.otp_delivery_blocklist_maxretry + 1).times do - get :send_code, params: { - otp_delivery_selection_form: { otp_delivery_preference: 'sms', - otp_make_default_number: nil }, - } - end + freeze_time do + (IdentityConfig.store.otp_delivery_blocklist_maxretry + 1).times do + get :send_code, params: { + otp_delivery_selection_form: { + otp_delivery_preference: 'sms', + otp_make_default_number: nil, + }, + } + end - expect(@user.reload.second_factor_locked_at.to_f).to be_within(0.1).of(Time.zone.now.to_f) + expect(@user.reload.second_factor_locked_at).to eq Time.zone.now + end end context 'when the phone has been marked as opted out in the DB' do From 935cabee4a658767159e2b423889263de29b9da5 Mon Sep 17 00:00:00 2001 From: Andrew Duthie Date: Fri, 3 Jun 2022 10:30:55 -0400 Subject: [PATCH 12/28] Use IdV mock PII data consistently (#6448) * Use IdV mock PII data consistently **Why**: - We have automated tests which attempt to detect PII leaks in event logging, and it's only effective if we use the same PII consistently - Convenient single source of truth for applicant data at various stages of the proofing process changelog: Internal, Automated Testing, Harden automated PII leak detection testing * Update requested attributes phone number to formatted --- .../doc_auth/mock/result_response_builder.rb | 2 +- lib/idp/constants.rb | 6 +++++- lib/session_encryptor.rb | 2 +- .../verify/password_confirm_controller_spec.rb | 18 ++---------------- spec/factories/profiles.rb | 16 +--------------- spec/features/idv/doc_auth/verify_step_spec.rb | 4 ++-- spec/lib/session_encryptor_spec.rb | 2 +- spec/services/analytics_spec.rb | 2 +- .../mock/result_response_builder_spec.rb | 4 ++-- spec/support/fake_analytics.rb | 2 +- spec/support/features/doc_auth_helper.rb | 4 ++-- .../idv_examples/sp_requested_attributes.rb | 16 +++++++--------- 12 files changed, 26 insertions(+), 52 deletions(-) diff --git a/app/services/doc_auth/mock/result_response_builder.rb b/app/services/doc_auth/mock/result_response_builder.rb index 04d128b0964..091baae7f1f 100644 --- a/app/services/doc_auth/mock/result_response_builder.rb +++ b/app/services/doc_auth/mock/result_response_builder.rb @@ -100,7 +100,7 @@ def pii_from_doc raw_pii = parsed_data_from_uploaded_file['document'] raw_pii&.symbolize_keys || {} else - Idp::Constants::DEFAULT_MOCK_PII_FROM_DOC + Idp::Constants::MOCK_IDV_APPLICANT end end diff --git a/lib/idp/constants.rb b/lib/idp/constants.rb index 6860cdcde23..4a4034232c1 100644 --- a/lib/idp/constants.rb +++ b/lib/idp/constants.rb @@ -11,7 +11,7 @@ module Constants AAL2 = 2 AAL3 = 3 - DEFAULT_MOCK_PII_FROM_DOC = { + MOCK_IDV_APPLICANT = { first_name: 'FAKEY', middle_name: nil, last_name: 'MCFAKERSON', @@ -27,5 +27,9 @@ module Constants state_id_expiration: '2099-12-31', phone: nil, }.freeze + + MOCK_IDV_APPLICANT_WITH_SSN = MOCK_IDV_APPLICANT.merge(ssn: '900-66-1234').freeze + + MOCK_IDV_APPLICANT_WITH_PHONE = MOCK_IDV_APPLICANT_WITH_SSN.merge(phone: '14065551234').freeze end end diff --git a/lib/session_encryptor.rb b/lib/session_encryptor.rb index 8805a9b4cef..3d7ebd68501 100644 --- a/lib/session_encryptor.rb +++ b/lib/session_encryptor.rb @@ -23,7 +23,7 @@ class SensitiveValueError < StandardError; end ['email'], ] - SENSITIVE_DEFAULT_FIELDS = Idp::Constants::DEFAULT_MOCK_PII_FROM_DOC.slice( + SENSITIVE_DEFAULT_FIELDS = Idp::Constants::MOCK_IDV_APPLICANT.slice( :last_name, :address1, :city, diff --git a/spec/controllers/api/verify/password_confirm_controller_spec.rb b/spec/controllers/api/verify/password_confirm_controller_spec.rb index 80b9287235d..ed1db9f33b3 100644 --- a/spec/controllers/api/verify/password_confirm_controller_spec.rb +++ b/spec/controllers/api/verify/password_confirm_controller_spec.rb @@ -10,26 +10,12 @@ def stub_idv_session let(:password) { 'iambatman' } let(:user) { create(:user, :signed_up, password: password) } - let(:applicant) do - { first_name: 'Bruce', - last_name: 'Wayne', - address1: '123 Mansion St', - address2: 'Ste 456', - city: 'Gotham City', - state: 'NY', - zipcode: '10015' } - end - - let(:pii) do - { first_name: 'Bruce', - last_name: 'Wayne', - ssn: '900-90-1234' } - end + let(:applicant) { Idp::Constants::MOCK_IDV_APPLICANT_WITH_PHONE } let(:profile) { subject.idv_session.profile } let(:key) { OpenSSL::PKey::RSA.new(Base64.strict_decode64(IdentityConfig.store.idv_private_key)) } let(:jwt_metadata) { { vendor_phone_confirmation: true, user_phone_confirmation: true } } - let(:jwt) { JWT.encode({ pii: pii, metadata: jwt_metadata }, key, 'RS256', sub: user.uuid) } + let(:jwt) { JWT.encode({ pii: applicant, metadata: jwt_metadata }, key, 'RS256', sub: user.uuid) } before do allow(IdentityConfig.store).to receive(:idv_api_enabled_steps).and_return(['password_confirm']) diff --git a/spec/factories/profiles.rb b/spec/factories/profiles.rb index 843228c56f8..a8152e138e4 100644 --- a/spec/factories/profiles.rb +++ b/spec/factories/profiles.rb @@ -30,21 +30,7 @@ end trait :with_pii do - pii do - Idp::Constants::DEFAULT_MOCK_PII_FROM_DOC.merge( - ssn: DocAuthHelper::GOOD_SSN, - phone: '+1 (555) 555-1234', - ) - end - end - - trait :with_pii do - pii do - Idp::Constants::DEFAULT_MOCK_PII_FROM_DOC.merge( - ssn: DocAuthHelper::GOOD_SSN, - phone: '+1 (555) 555-1234', - ) - end + pii { Idp::Constants::MOCK_IDV_APPLICANT_WITH_PHONE } end after(:build) do |profile, evaluator| diff --git a/spec/features/idv/doc_auth/verify_step_spec.rb b/spec/features/idv/doc_auth/verify_step_spec.rb index 88679951e13..7141e343d46 100644 --- a/spec/features/idv/doc_auth/verify_step_spec.rb +++ b/spec/features/idv/doc_auth/verify_step_spec.rb @@ -162,7 +162,7 @@ stub_const( 'Idv::Steps::VerifyBaseStep::AAMVA_SUPPORTED_JURISDICTIONS', Idv::Steps::VerifyBaseStep::AAMVA_SUPPORTED_JURISDICTIONS + - [Idp::Constants::DEFAULT_MOCK_PII_FROM_DOC[:state_id_jurisdiction]], + [Idp::Constants::MOCK_IDV_APPLICANT[:state_id_jurisdiction]], ) sign_in_and_2fa_user complete_doc_auth_steps_before_verify_step @@ -185,7 +185,7 @@ stub_const( 'Idv::Steps::VerifyBaseStep::AAMVA_SUPPORTED_JURISDICTIONS', Idv::Steps::VerifyBaseStep::AAMVA_SUPPORTED_JURISDICTIONS - - [Idp::Constants::DEFAULT_MOCK_PII_FROM_DOC[:state_id_jurisdiction]], + [Idp::Constants::MOCK_IDV_APPLICANT[:state_id_jurisdiction]], ) sign_in_and_2fa_user complete_doc_auth_steps_before_verify_step diff --git a/spec/lib/session_encryptor_spec.rb b/spec/lib/session_encryptor_spec.rb index 7c5d56cc8dc..cd1c6742393 100644 --- a/spec/lib/session_encryptor_spec.rb +++ b/spec/lib/session_encryptor_spec.rb @@ -171,7 +171,7 @@ it 'raises if sensitive value is not KMS encrypted' do session = { - 'new_key' => Idp::Constants::DEFAULT_MOCK_PII_FROM_DOC[:last_name], + 'new_key' => Idp::Constants::MOCK_IDV_APPLICANT[:last_name], } expect { diff --git a/spec/services/analytics_spec.rb b/spec/services/analytics_spec.rb index 4b5ac42f887..dc02d7e9c97 100644 --- a/spec/services/analytics_spec.rb +++ b/spec/services/analytics_spec.rb @@ -186,7 +186,7 @@ it 'does not alert when pii values are inside words' do expect(ahoy).to receive(:track) - stub_const('Idp::Constants::DEFAULT_MOCK_PII_FROM_DOC', zipcode: '12345') + stub_const('Idp::Constants::MOCK_IDV_APPLICANT', zipcode: '12345') expect do analytics.track_event( diff --git a/spec/services/doc_auth/mock/result_response_builder_spec.rb b/spec/services/doc_auth/mock/result_response_builder_spec.rb index 617aaba227f..73d92defa75 100644 --- a/spec/services/doc_auth/mock/result_response_builder_spec.rb +++ b/spec/services/doc_auth/mock/result_response_builder_spec.rb @@ -24,7 +24,7 @@ expect(response.errors).to eq({}) expect(response.exception).to eq(nil) expect(response.pii_from_doc). - to eq(Idp::Constants::DEFAULT_MOCK_PII_FROM_DOC) + to eq(Idp::Constants::MOCK_IDV_APPLICANT) end end @@ -178,7 +178,7 @@ expect(response.errors).to eq({}) expect(response.exception).to eq(nil) expect(response.pii_from_doc). - to eq(Idp::Constants::DEFAULT_MOCK_PII_FROM_DOC) + to eq(Idp::Constants::MOCK_IDV_APPLICANT) end end diff --git a/spec/support/fake_analytics.rb b/spec/support/fake_analytics.rb index 8a327c5c5ce..cdfce6a6f6c 100644 --- a/spec/support/fake_analytics.rb +++ b/spec/support/fake_analytics.rb @@ -20,7 +20,7 @@ def track_event(event, original_attributes = {}) ERROR end - Idp::Constants::DEFAULT_MOCK_PII_FROM_DOC.slice( + Idp::Constants::MOCK_IDV_APPLICANT.slice( :first_name, :last_name, :address1, diff --git a/spec/support/features/doc_auth_helper.rb b/spec/support/features/doc_auth_helper.rb index 66decf7074a..683f11acadb 100644 --- a/spec/support/features/doc_auth_helper.rb +++ b/spec/support/features/doc_auth_helper.rb @@ -3,7 +3,7 @@ module DocAuthHelper include DocumentCaptureStepHelper - GOOD_SSN = '900-66-1234' + GOOD_SSN = Idp::Constants::MOCK_IDV_APPLICANT_WITH_SSN[:ssn] def session_from_completed_flow_steps(finished_step) session = { doc_auth: {} } @@ -185,7 +185,7 @@ def complete_proofing_steps end def mock_doc_auth_no_name_pii(method) - pii_with_no_name = Idp::Constants::DEFAULT_MOCK_PII_FROM_DOC.dup + pii_with_no_name = Idp::Constants::MOCK_IDV_APPLICANT.dup pii_with_no_name[:last_name] = nil DocAuth::Mock::DocAuthMockClient.mock_response!( method: method, diff --git a/spec/support/idv_examples/sp_requested_attributes.rb b/spec/support/idv_examples/sp_requested_attributes.rb index 629a93b7d0d..059a6e76ddf 100644 --- a/spec/support/idv_examples/sp_requested_attributes.rb +++ b/spec/support/idv_examples/sp_requested_attributes.rb @@ -3,14 +3,8 @@ include IdvStepHelper let(:user) { user_with_2fa } - let(:good_ssn) { DocAuthHelper::GOOD_SSN } let(:profile) { create(:profile, :active, :verified, user: user, pii: saved_pii) } - let(:saved_pii) do - Idp::Constants::DEFAULT_MOCK_PII_FROM_DOC.merge( - ssn: good_ssn, - phone: '+1 (555) 555-1234', - ) - end + let(:saved_pii) { Idp::Constants::MOCK_IDV_APPLICANT_WITH_PHONE } context 'visiting an SP for the first time' do it 'requires the user to verify the attributes submitted to the SP', js: true do @@ -41,7 +35,11 @@ expect(page).to have_content t('help_text.requested_attributes.phone') expect(page).to have_content '+1 202-555-1212' expect(page).to have_content t('help_text.requested_attributes.social_security_number') - expect(page).to have_css '.masked-text__text', text: good_ssn, visible: :hidden + expect(page).to have_css( + '.masked-text__text', + text: DocAuthHelper::GOOD_SSN, + visible: :hidden, + ) end end end @@ -103,7 +101,7 @@ expect(page).to have_content t('help_text.requested_attributes.full_name') expect(page).to have_content 'FAKEY MCFAKERSON' expect(page).to have_content t('help_text.requested_attributes.phone') - expect(page).to have_content '+15555551234' + expect(page).to have_content '+1 406-555-1234' expect(page).to have_content t('help_text.requested_attributes.social_security_number') expect(page).to have_content DocAuthHelper::GOOD_SSN end From d9ae660f5208570e12d1c81997b499015496fbfe Mon Sep 17 00:00:00 2001 From: Steve Urciuoli Date: Fri, 3 Jun 2022 11:47:11 -0500 Subject: [PATCH 13/28] LG-5939 Document the rest of analytics events #20 (#6437) --- .../authorization_controller.rb | 4 +- .../openid_connect/token_controller.rb | 2 +- .../openid_connect/user_info_controller.rb | 2 +- .../otp_verification_controller.rb | 4 +- ...piv_cac_authentication_setup_controller.rb | 2 +- .../piv_cac_setup_from_sign_in_controller.rb | 2 +- .../users/totp_setup_controller.rb | 2 +- .../two_factor_authentication_controller.rb | 4 +- .../users/webauthn_setup_controller.rb | 2 +- app/services/analytics.rb | 5 -- app/services/analytics_events.rb | 87 +++++++++++++++++++ .../authorization_controller_spec.rb | 14 +-- .../openid_connect/token_controller_spec.rb | 10 ++- .../user_info_controller_spec.rb | 8 +- .../otp_verification_controller_spec.rb | 6 +- .../users/totp_setup_controller_spec.rb | 16 ++-- ...o_factor_authentication_controller_spec.rb | 4 +- .../users/webauthn_setup_controller_spec.rb | 2 +- 18 files changed, 128 insertions(+), 48 deletions(-) diff --git a/app/controllers/openid_connect/authorization_controller.rb b/app/controllers/openid_connect/authorization_controller.rb index 0040e9683da..906f74d359b 100644 --- a/app/controllers/openid_connect/authorization_controller.rb +++ b/app/controllers/openid_connect/authorization_controller.rb @@ -83,9 +83,7 @@ def track_authorize_analytics(result) analytics_attributes = result.to_h.except(:redirect_uri). merge(user_fully_authenticated: user_fully_authenticated?) - analytics.track_event( - Analytics::OPENID_CONNECT_REQUEST_AUTHORIZATION, analytics_attributes - ) + analytics.openid_connect_request_authorization(**analytics_attributes) end def identity_needs_verification? diff --git a/app/controllers/openid_connect/token_controller.rb b/app/controllers/openid_connect/token_controller.rb index 4dd97b29e50..92299cac934 100644 --- a/app/controllers/openid_connect/token_controller.rb +++ b/app/controllers/openid_connect/token_controller.rb @@ -8,7 +8,7 @@ def create @token_form = OpenidConnectTokenForm.new(token_params) result = @token_form.submit - analytics.track_event(Analytics::OPENID_CONNECT_TOKEN, result.to_h) + analytics.openid_connect_token(**result.to_h) render json: @token_form.response, status: (result.success? ? :ok : :bad_request) diff --git a/app/controllers/openid_connect/user_info_controller.rb b/app/controllers/openid_connect/user_info_controller.rb index 3d5a9b34b47..23c36b1b022 100644 --- a/app/controllers/openid_connect/user_info_controller.rb +++ b/app/controllers/openid_connect/user_info_controller.rb @@ -16,7 +16,7 @@ def show def authenticate_identity_via_bearer_token verifier = AccessTokenVerifier.new(request.env['HTTP_AUTHORIZATION']) response = verifier.submit - analytics.track_event(Analytics::OPENID_CONNECT_BEARER_TOKEN, response.to_h) + analytics.openid_connect_bearer_token(**response.to_h) if response.success? @current_identity = verifier.identity diff --git a/app/controllers/two_factor_authentication/otp_verification_controller.rb b/app/controllers/two_factor_authentication/otp_verification_controller.rb index 06e45f4ed28..c6c10a3ef2f 100644 --- a/app/controllers/two_factor_authentication/otp_verification_controller.rb +++ b/app/controllers/two_factor_authentication/otp_verification_controller.rb @@ -78,9 +78,7 @@ def form_params def post_analytics(result) properties = result.to_h.merge(analytics_properties) - if context == 'confirmation' - analytics.track_event(Analytics::MULTI_FACTOR_AUTH_SETUP, properties) - end + analytics.multi_factor_auth_setup(**properties) if context == 'confirmation' analytics.track_mfa_submit_event(properties) end diff --git a/app/controllers/users/piv_cac_authentication_setup_controller.rb b/app/controllers/users/piv_cac_authentication_setup_controller.rb index 741eee77bfa..20e27bc76d9 100644 --- a/app/controllers/users/piv_cac_authentication_setup_controller.rb +++ b/app/controllers/users/piv_cac_authentication_setup_controller.rb @@ -75,7 +75,7 @@ def piv_cac_service_url_with_redirect def process_piv_cac_setup result = user_piv_cac_form.submit - analytics.track_event(Analytics::MULTI_FACTOR_AUTH_SETUP, result.to_h) + analytics.multi_factor_auth_setup(**result.to_h) if result.success? process_valid_submission else diff --git a/app/controllers/users/piv_cac_setup_from_sign_in_controller.rb b/app/controllers/users/piv_cac_setup_from_sign_in_controller.rb index 04b1511232a..85b42f70c99 100644 --- a/app/controllers/users/piv_cac_setup_from_sign_in_controller.rb +++ b/app/controllers/users/piv_cac_setup_from_sign_in_controller.rb @@ -35,7 +35,7 @@ def render_prompt def process_piv_cac_setup result = user_piv_cac_form.submit - analytics.track_event(Analytics::MULTI_FACTOR_AUTH_SETUP, result.to_h) + analytics.multi_factor_auth_setup(**result.to_h) if result.success? process_valid_submission else diff --git a/app/controllers/users/totp_setup_controller.rb b/app/controllers/users/totp_setup_controller.rb index 2a4454b830b..ccce1677efe 100644 --- a/app/controllers/users/totp_setup_controller.rb +++ b/app/controllers/users/totp_setup_controller.rb @@ -22,7 +22,7 @@ def new def confirm result = totp_setup_form.submit - analytics.track_event(Analytics::MULTI_FACTOR_AUTH_SETUP, result.to_h) + analytics.multi_factor_auth_setup(**result.to_h) if result.success? process_valid_code diff --git a/app/controllers/users/two_factor_authentication_controller.rb b/app/controllers/users/two_factor_authentication_controller.rb index f9ef0680840..1f677e8a2b4 100644 --- a/app/controllers/users/two_factor_authentication_controller.rb +++ b/app/controllers/users/two_factor_authentication_controller.rb @@ -14,7 +14,7 @@ def show def send_code result = otp_delivery_selection_form.submit(delivery_params) - analytics.track_event(Analytics::OTP_DELIVERY_SELECTION, result.to_h) + analytics.otp_delivery_selection(**result.to_h) if result.success? handle_valid_otp_params(user_select_delivery_preference, user_selected_default_number) update_otp_delivery_preference_if_needed @@ -68,7 +68,7 @@ def phone_configuration def validate_otp_delivery_preference_and_send_code result = otp_delivery_selection_form.submit(otp_delivery_preference: delivery_preference) - analytics.track_event(Analytics::OTP_DELIVERY_SELECTION, result.to_h) + analytics.otp_delivery_selection(**result.to_h) phone_is_confirmed = UserSessionContext.authentication_context?(context) phone_capabilities = PhoneNumberCapabilities.new( parsed_phone, diff --git a/app/controllers/users/webauthn_setup_controller.rb b/app/controllers/users/webauthn_setup_controller.rb index 1da5870cc90..85c112c64c6 100644 --- a/app/controllers/users/webauthn_setup_controller.rb +++ b/app/controllers/users/webauthn_setup_controller.rb @@ -37,7 +37,7 @@ def confirm remember_device_default: remember_device_default, platform_authenticator: @platform_authenticator, ) - analytics.track_event(Analytics::MULTI_FACTOR_AUTH_SETUP, result.to_h) + analytics.multi_factor_auth_setup(**result.to_h) if result.success? process_valid_webauthn(form) else diff --git a/app/services/analytics.rb b/app/services/analytics.rb index 987a615af16..773cca74999 100644 --- a/app/services/analytics.rb +++ b/app/services/analytics.rb @@ -137,11 +137,6 @@ def session_started_at # rubocop:disable Layout/LineLength DOC_AUTH = 'Doc Auth' # visited or submitted is appended - MULTI_FACTOR_AUTH_SETUP = 'Multi-Factor Authentication Setup' - OPENID_CONNECT_BEARER_TOKEN = 'OpenID Connect: bearer token authentication' - OPENID_CONNECT_REQUEST_AUTHORIZATION = 'OpenID Connect: authorization request' - OPENID_CONNECT_TOKEN = 'OpenID Connect: token' - OTP_DELIVERY_SELECTION = 'OTP: Delivery Selection' PASSWORD_RESET_TOKEN = 'Password Reset: Token Submitted' PASSWORD_RESET_VISIT = 'Password Reset: Email Form Visited' PENDING_ACCOUNT_RESET_CANCELLED = 'Pending account reset cancelled' diff --git a/app/services/analytics_events.rb b/app/services/analytics_events.rb index 112ef05680e..1dd1be60cd8 100644 --- a/app/services/analytics_events.rb +++ b/app/services/analytics_events.rb @@ -1200,6 +1200,93 @@ def multi_factor_auth_max_sends track_event('Multi-Factor Authentication: max otp sends reached') end + # Tracks when a user sets up a multi factor auth method + # @param [String] multi_factor_auth_method + def multi_factor_auth_setup(multi_factor_auth_method:, **extra) + track_event( + 'Multi-Factor Authentication Setup', + multi_factor_auth_method: multi_factor_auth_method, + **extra, + ) + end + + # Tracks when an openid connect bearer token authentication request is made + # @param [Boolean] success + # @param [Hash] errors + def openid_connect_bearer_token(success:, errors:, **extra) + track_event( + 'OpenID Connect: bearer token authentication', + success: success, + errors: errors, + **extra, + ) + end + + # Tracks when openid authorization request is made + # @param [String] client_id + # @param [String] scope + # @param [Array] acr_values + # @param [Boolean] unauthorized_scope + # @param [Boolean] user_fully_authenticated + def openid_connect_request_authorization( + client_id:, + scope:, + acr_values:, + unauthorized_scope:, + user_fully_authenticated:, + **extra + ) + track_event( + 'OpenID Connect: authorization request', + client_id: client_id, + scope: scope, + acr_values: acr_values, + unauthorized_scope: unauthorized_scope, + user_fully_authenticated: user_fully_authenticated, + **extra, + ) + end + + # Tracks when an openid connect token request is made + # @param [String] client_id + # @param [String] user_id + def openid_connect_token(client_id:, user_id:, **extra) + track_event( + 'OpenID Connect: token', + client_id: client_id, + user_id: user_id, + **extra, + ) + end + + # Tracks when user makes an otp delivery selection + # @param [String] otp_delivery_preference (sms or voice) + # @param [Boolean] resend + # @param [String] country_code + # @param [String] area_code + # @param ["authentication","reauthentication","confirmation"] context user session context + # @param [Hash] pii_like_keypaths + def otp_delivery_selection( + otp_delivery_preference:, + resend:, + country_code:, + area_code:, + context:, + pii_like_keypaths:, + **extra + ) + track_event( + 'OTP: Delivery Selection', + otp_delivery_preference: otp_delivery_preference, + resend: resend, + country_code: country_code, + area_code: area_code, + context: context, + pii_like_keypaths: pii_like_keypaths, + **extra, + ) + end + # @param [Boolean] success # @param [Hash] errors # The user updated their password diff --git a/spec/controllers/openid_connect/authorization_controller_spec.rb b/spec/controllers/openid_connect/authorization_controller_spec.rb index 8c20d909644..b9b68abdb67 100644 --- a/spec/controllers/openid_connect/authorization_controller_spec.rb +++ b/spec/controllers/openid_connect/authorization_controller_spec.rb @@ -50,7 +50,7 @@ it 'tracks IAL1 authentication event' do stub_analytics expect(@analytics).to receive(:track_event). - with(Analytics::OPENID_CONNECT_REQUEST_AUTHORIZATION, + with('OpenID Connect: authorization request', success: true, client_id: client_id, errors: {}, @@ -109,7 +109,7 @@ it 'tracks IAL2 authentication event' do stub_analytics expect(@analytics).to receive(:track_event). - with(Analytics::OPENID_CONNECT_REQUEST_AUTHORIZATION, + with('OpenID Connect: authorization request', success: true, client_id: client_id, errors: {}, @@ -208,7 +208,7 @@ it 'tracks IAL2 authentication event' do stub_analytics expect(@analytics).to receive(:track_event). - with(Analytics::OPENID_CONNECT_REQUEST_AUTHORIZATION, + with('OpenID Connect: authorization request', success: true, client_id: client_id, errors: {}, @@ -249,7 +249,7 @@ it 'tracks IAL1 authentication event' do stub_analytics expect(@analytics).to receive(:track_event). - with(Analytics::OPENID_CONNECT_REQUEST_AUTHORIZATION, + with('OpenID Connect: authorization request', success: true, client_id: client_id, errors: {}, @@ -291,7 +291,7 @@ it 'tracks IAL1 authentication event' do stub_analytics expect(@analytics).to receive(:track_event). - with(Analytics::OPENID_CONNECT_REQUEST_AUTHORIZATION, + with('OpenID Connect: authorization request', success: true, client_id: client_id, errors: {}, @@ -368,7 +368,7 @@ it 'tracks the event with errors' do stub_analytics expect(@analytics).to receive(:track_event). - with(Analytics::OPENID_CONNECT_REQUEST_AUTHORIZATION, + with('OpenID Connect: authorization request', success: false, client_id: client_id, unauthorized_scope: true, @@ -396,7 +396,7 @@ it 'tracks the event with errors' do stub_analytics expect(@analytics).to receive(:track_event). - with(Analytics::OPENID_CONNECT_REQUEST_AUTHORIZATION, + with('OpenID Connect: authorization request', success: false, client_id: nil, unauthorized_scope: true, diff --git a/spec/controllers/openid_connect/token_controller_spec.rb b/spec/controllers/openid_connect/token_controller_spec.rb index c0354bb3969..03be4f8b7a8 100644 --- a/spec/controllers/openid_connect/token_controller_spec.rb +++ b/spec/controllers/openid_connect/token_controller_spec.rb @@ -53,9 +53,11 @@ it 'tracks a successful event in analytics' do stub_analytics expect(@analytics).to receive(:track_event). - with(Analytics::OPENID_CONNECT_TOKEN, success: true, client_id: client_id, - user_id: user.uuid, errors: {}) - + with('OpenID Connect: token', + success: true, + client_id: client_id, + user_id: user.uuid, + errors: {}) action end end @@ -75,7 +77,7 @@ it 'tracks an unsuccessful event in analytics' do stub_analytics expect(@analytics).to receive(:track_event). - with(Analytics::OPENID_CONNECT_TOKEN, + with('OpenID Connect: token', success: false, client_id: client_id, user_id: user.uuid, diff --git a/spec/controllers/openid_connect/user_info_controller_spec.rb b/spec/controllers/openid_connect/user_info_controller_spec.rb index 4dcacdc4dda..179154e8a03 100644 --- a/spec/controllers/openid_connect/user_info_controller_spec.rb +++ b/spec/controllers/openid_connect/user_info_controller_spec.rb @@ -20,7 +20,7 @@ it 'tracks analytics' do stub_analytics expect(@analytics).to receive(:track_event). - with(Analytics::OPENID_CONNECT_BEARER_TOKEN, + with('OpenID Connect: bearer token authentication', success: false, errors: hash_including(:access_token), error_details: hash_including(:access_token)) @@ -42,7 +42,7 @@ it 'tracks analytics' do stub_analytics expect(@analytics).to receive(:track_event). - with(Analytics::OPENID_CONNECT_BEARER_TOKEN, + with('OpenID Connect: bearer token authentication', success: false, errors: hash_including(:access_token), error_details: hash_including(:access_token)) @@ -63,7 +63,7 @@ it 'tracks analytics' do stub_analytics expect(@analytics).to receive(:track_event). - with(Analytics::OPENID_CONNECT_BEARER_TOKEN, + with('OpenID Connect: bearer token authentication', success: false, errors: hash_including(:access_token), error_details: hash_including(:access_token)) @@ -96,7 +96,7 @@ it 'tracks analytics' do stub_analytics expect(@analytics).to receive(:track_event). - with(Analytics::OPENID_CONNECT_BEARER_TOKEN, success: true, errors: {}) + with('OpenID Connect: bearer token authentication', success: true, errors: {}) action end diff --git a/spec/controllers/two_factor_authentication/otp_verification_controller_spec.rb b/spec/controllers/two_factor_authentication/otp_verification_controller_spec.rb index 7209c9fe27d..5a7d02e4e9f 100644 --- a/spec/controllers/two_factor_authentication/otp_verification_controller_spec.rb +++ b/spec/controllers/two_factor_authentication/otp_verification_controller_spec.rb @@ -318,7 +318,7 @@ } expect(@analytics).to receive(:track_event). - with(Analytics::MULTI_FACTOR_AUTH_SETUP, properties) + with('Multi-Factor Authentication Setup', properties) controller.user_session[:phone_id] = phone_id post( :create, @@ -381,7 +381,7 @@ } expect(@analytics).to have_received(:track_event). - with(Analytics::MULTI_FACTOR_AUTH_SETUP, properties) + with('Multi-Factor Authentication Setup', properties) end end @@ -425,7 +425,7 @@ } expect(@analytics).to have_received(:track_event). - with(Analytics::MULTI_FACTOR_AUTH_SETUP, properties) + with('Multi-Factor Authentication Setup', properties) expect(subject).to have_received(:create_user_event).with(:phone_confirmed) expect(subject).to have_received(:create_user_event).exactly(:once) diff --git a/spec/controllers/users/totp_setup_controller_spec.rb b/spec/controllers/users/totp_setup_controller_spec.rb index e53cdfc8f06..d1eccba9ff1 100644 --- a/spec/controllers/users/totp_setup_controller_spec.rb +++ b/spec/controllers/users/totp_setup_controller_spec.rb @@ -103,7 +103,7 @@ } expect(@analytics).to have_received(:track_event). - with(Analytics::MULTI_FACTOR_AUTH_SETUP, result) + with('Multi-Factor Authentication Setup', result) end end @@ -132,7 +132,7 @@ } expect(@analytics).to have_received(:track_event). - with(Analytics::MULTI_FACTOR_AUTH_SETUP, result) + with('Multi-Factor Authentication Setup', result) end end @@ -162,7 +162,7 @@ } expect(@analytics).to have_received(:track_event). - with(Analytics::MULTI_FACTOR_AUTH_SETUP, result) + with('Multi-Factor Authentication Setup', result) end end @@ -193,7 +193,7 @@ } expect(@analytics).to have_received(:track_event). - with(Analytics::MULTI_FACTOR_AUTH_SETUP, result) + with('Multi-Factor Authentication Setup', result) end end end @@ -222,7 +222,7 @@ auth_app_configuration_id: nil, } expect(@analytics).to have_received(:track_event). - with(Analytics::MULTI_FACTOR_AUTH_SETUP, result) + with('Multi-Factor Authentication Setup', result) end end @@ -254,7 +254,7 @@ } expect(@analytics).to have_received(:track_event). - with(Analytics::MULTI_FACTOR_AUTH_SETUP, result) + with('Multi-Factor Authentication Setup', result) end end @@ -273,7 +273,7 @@ } expect(@analytics).to have_received(:track_event). - with(Analytics::MULTI_FACTOR_AUTH_SETUP, result) + with('Multi-Factor Authentication Setup', result) end end end @@ -301,7 +301,7 @@ } expect(@analytics).to have_received(:track_event). - with(Analytics::MULTI_FACTOR_AUTH_SETUP, result) + with('Multi-Factor Authentication Setup', result) end end end diff --git a/spec/controllers/users/two_factor_authentication_controller_spec.rb b/spec/controllers/users/two_factor_authentication_controller_spec.rb index 0a33381b73a..1c08896a2f8 100644 --- a/spec/controllers/users/two_factor_authentication_controller_spec.rb +++ b/spec/controllers/users/two_factor_authentication_controller_spec.rb @@ -313,7 +313,7 @@ def index expect(@analytics).to receive(:track_event). ordered. - with(Analytics::OTP_DELIVERY_SELECTION, analytics_hash) + with('OTP: Delivery Selection', analytics_hash) expect(@analytics).to receive(:track_event). ordered. with(Analytics::TELEPHONY_OTP_SENT, hash_including(success: true)) @@ -434,7 +434,7 @@ def index expect(@analytics).to receive(:track_event). ordered. - with(Analytics::OTP_DELIVERY_SELECTION, analytics_hash) + with('OTP: Delivery Selection', analytics_hash) expect(@analytics).to receive(:track_event). ordered. with(Analytics::TELEPHONY_OTP_SENT, hash_including(success: true)) diff --git a/spec/controllers/users/webauthn_setup_controller_spec.rb b/spec/controllers/users/webauthn_setup_controller_spec.rb index 62832d4f03c..e9f66f3a72a 100644 --- a/spec/controllers/users/webauthn_setup_controller_spec.rb +++ b/spec/controllers/users/webauthn_setup_controller_spec.rb @@ -77,7 +77,7 @@ multi_factor_auth_method: 'webauthn', } expect(@analytics).to receive(:track_event). - with(Analytics::MULTI_FACTOR_AUTH_SETUP, result) + with('Multi-Factor Authentication Setup', result) patch :confirm, params: params end From 2fb6294af5735f32946222518995e9e60e3e9bd6 Mon Sep 17 00:00:00 2001 From: Andrew Duthie Date: Fri, 3 Jun 2022 13:35:18 -0400 Subject: [PATCH 14/28] Scroll alert into view on password confirm error (IdV app) (#6450) * Scroll alert into view on password confirm error (IdV app) **Why**: So that the user isn't left confused about why the page hasn't changed when submitting the password confirmation step with an incorrect password. This was discovered in a recent bug bash, and regresses the current production behavior, where the server-side form submission would reset the scroll position of the page. changelog: Upcoming Features, Identity Verification, Add password confirmation step * Ensure scrollIntoView on each error submission --- app/javascript/packages/components/index.ts | 1 + .../components/scroll-into-view.spec.tsx | 28 +++++++++++++++++++ .../packages/components/scroll-into-view.tsx | 18 ++++++++++++ .../password-confirm-step.spec.tsx | 12 +++++++- .../password-confirm-step.tsx | 19 +++++++------ 5 files changed, 69 insertions(+), 9 deletions(-) create mode 100644 app/javascript/packages/components/scroll-into-view.spec.tsx create mode 100644 app/javascript/packages/components/scroll-into-view.tsx diff --git a/app/javascript/packages/components/index.ts b/app/javascript/packages/components/index.ts index 4a591e100be..60715ffb7cb 100644 --- a/app/javascript/packages/components/index.ts +++ b/app/javascript/packages/components/index.ts @@ -7,6 +7,7 @@ export { default as Icon } from './icon'; export { default as FullScreen } from './full-screen'; export { default as Link } from './link'; export { default as PageHeading } from './page-heading'; +export { default as ScrollIntoView } from './scroll-into-view'; export { default as SpinnerDots } from './spinner-dots'; export { default as TextInput } from './text-input'; export { default as TroubleshootingOptions } from './troubleshooting-options'; diff --git a/app/javascript/packages/components/scroll-into-view.spec.tsx b/app/javascript/packages/components/scroll-into-view.spec.tsx new file mode 100644 index 00000000000..97e44e665ff --- /dev/null +++ b/app/javascript/packages/components/scroll-into-view.spec.tsx @@ -0,0 +1,28 @@ +import { render } from '@testing-library/react'; +import type { SinonSpy } from 'sinon'; +import { useSandbox } from '@18f/identity-test-helpers'; +import ScrollIntoView from './scroll-into-view'; + +describe('ScrollIntoView', () => { + const sandbox = useSandbox(); + + it('scrolls content into view', () => { + sandbox.spy(Element.prototype, 'scrollIntoView'); + + const { getByText } = render(Content); + + const text = getByText('Content'); + const call = (Element.prototype.scrollIntoView as SinonSpy).getCall(0); + + expect((call.thisValue as Element).contains(text)).to.be.true(); + }); + + it('only scrolls into view on initial render', () => { + sandbox.spy(Element.prototype, 'scrollIntoView'); + + const { rerender } = render(Content1); + rerender(Content2); + + expect(Element.prototype.scrollIntoView).to.have.been.calledOnce(); + }); +}); diff --git a/app/javascript/packages/components/scroll-into-view.tsx b/app/javascript/packages/components/scroll-into-view.tsx new file mode 100644 index 00000000000..dfc3853136f --- /dev/null +++ b/app/javascript/packages/components/scroll-into-view.tsx @@ -0,0 +1,18 @@ +import { useRef, useEffect } from 'react'; +import type { ReactNode } from 'react'; + +interface ScrollIntoViewProps { + children: ReactNode; +} + +/** + * Scrolls content into the user's viewport when mounted. + */ +function ScrollIntoView({ children }: ScrollIntoViewProps) { + const ref = useRef(null); + useEffect(() => ref.current?.scrollIntoView(), []); + + return
    {children}
    ; +} + +export default ScrollIntoView; diff --git a/app/javascript/packages/verify-flow/steps/password-confirm/password-confirm-step.spec.tsx b/app/javascript/packages/verify-flow/steps/password-confirm/password-confirm-step.spec.tsx index b32bfd01dc1..05fde8acf62 100644 --- a/app/javascript/packages/verify-flow/steps/password-confirm/password-confirm-step.spec.tsx +++ b/app/javascript/packages/verify-flow/steps/password-confirm/password-confirm-step.spec.tsx @@ -2,6 +2,7 @@ import { render } from '@testing-library/react'; import userEvent from '@testing-library/user-event'; import { computeAccessibleDescription } from 'dom-accessibility-api'; import { accordion } from 'identity-style-guide'; +import type { SinonSpy } from 'sinon'; import * as analytics from '@18f/identity-analytics'; import { useSandbox, usePropertyValue } from '@18f/identity-test-helpers'; import { FormSteps } from '@18f/identity-form-steps'; @@ -83,15 +84,24 @@ describe('PasswordConfirmStep', () => { , ); + sandbox.spy(Element.prototype, 'scrollIntoView'); + const continueButton = getByRole('button', { name: 'forms.buttons.continue' }); + await userEvent.type(getByLabelText('components.password_toggle.label'), 'password'); - await userEvent.click(getByRole('button', { name: 'forms.buttons.continue' })); + await userEvent.click(continueButton); // There should not be a field-specific error, only a top-level alert. const alert = await findByRole('alert'); + expect(Element.prototype.scrollIntoView).to.have.been.calledOnce(); + const { thisValue: scrollElement } = (Element.prototype.scrollIntoView as SinonSpy).getCall(0); + expect((scrollElement as Element).contains(alert)).to.be.true(); expect(alert.textContent).to.equal('Incorrect password'); const input = getByLabelText('components.password_toggle.label'); const description = computeAccessibleDescription(input); expect(description).to.be.empty(); + + await userEvent.click(continueButton); + expect(Element.prototype.scrollIntoView).to.have.been.calledTwice(); }); describe('forgot password', () => { diff --git a/app/javascript/packages/verify-flow/steps/password-confirm/password-confirm-step.tsx b/app/javascript/packages/verify-flow/steps/password-confirm/password-confirm-step.tsx index f783bd938dc..8f6d566b114 100644 --- a/app/javascript/packages/verify-flow/steps/password-confirm/password-confirm-step.tsx +++ b/app/javascript/packages/verify-flow/steps/password-confirm/password-confirm-step.tsx @@ -10,7 +10,7 @@ import { import { PasswordToggle } from '@18f/identity-password-toggle'; import { FlowContext } from '@18f/identity-verify-flow'; import { formatHTML } from '@18f/identity-react-i18n'; -import { PageHeading, Accordion, Alert, Link } from '@18f/identity-components'; +import { PageHeading, Accordion, Alert, Link, ScrollIntoView } from '@18f/identity-components'; import { getConfigValue } from '@18f/identity-config'; import type { ChangeEvent } from 'react'; import type { FormStepComponentProps } from '@18f/identity-form-steps'; @@ -38,6 +38,7 @@ function PasswordConfirmStep({ errors, registerField, onChange, value }: Passwor } const appName = getConfigValue('appName'); + const stepErrors = errors.filter(({ error }) => error instanceof PasswordSubmitError); return ( <> @@ -51,13 +52,15 @@ function PasswordConfirmStep({ errors, registerField, onChange, value }: Passwor )} )} - {errors - .filter(({ error }) => error instanceof PasswordSubmitError) - .map(({ error }) => ( - - {error.message} - - ))} + {stepErrors.length > 0 && ( + + {stepErrors.map(({ error }) => ( + + {error.message} + + ))} + + )} {t('idv.titles.session.review', { app_name: appName })}

    {t('idv.messages.sessions.review_message', { app_name: appName })}

    From d169fc74726ba9fd0aa1497a6c60500f50be30fa Mon Sep 17 00:00:00 2001 From: Mitchell Henke Date: Fri, 3 Jun 2022 13:11:40 -0500 Subject: [PATCH 15/28] Fix logging of webauthn authentications (#6451) * add failing spec * fix logging of non-platform webauthn auths changelog: Bug Fixes, Logging, Fix logging of roaming webauthn authentications --- .../webauthn_verification_controller.rb | 3 ++- .../webauthn_verification_controller_spec.rb | 1 + 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/app/controllers/two_factor_authentication/webauthn_verification_controller.rb b/app/controllers/two_factor_authentication/webauthn_verification_controller.rb index 7d0df1f6adb..94bbd395a64 100644 --- a/app/controllers/two_factor_authentication/webauthn_verification_controller.rb +++ b/app/controllers/two_factor_authentication/webauthn_verification_controller.rb @@ -100,7 +100,8 @@ def credential_ids end def analytics_properties - auth_method = if form&.webauthn_configuration&.platform_authenticator || params[:platform] + auth_method = if form&.webauthn_configuration&.platform_authenticator || + params[:platform].to_s == 'true' 'webauthn_platform' else 'webauthn' diff --git a/spec/controllers/two_factor_authentication/webauthn_verification_controller_spec.rb b/spec/controllers/two_factor_authentication/webauthn_verification_controller_spec.rb index 81c5c6a25df..07c9744dd05 100644 --- a/spec/controllers/two_factor_authentication/webauthn_verification_controller_spec.rb +++ b/spec/controllers/two_factor_authentication/webauthn_verification_controller_spec.rb @@ -58,6 +58,7 @@ client_data_json: verification_client_data_json, signature: signature, credential_id: credential_id, + platform: '', } end before do From fe11234a14ba5426212e37ff0106f35c5f14711a Mon Sep 17 00:00:00 2001 From: Andrew Duthie Date: Fri, 3 Jun 2022 15:10:19 -0400 Subject: [PATCH 16/28] Try skipping screens in IdV feature spec helpers (#6447) * Try skipping screens in IdV feature spec helpers **Why**: Improve performance by skipping most of the proofing process, since these helpers exist to satisfy the necessary requirements to get to the point in the process where the spec is concerned with. It should not be the expectation that the helpers themselves are verifying that the experience is functional, since that is the responsibility of the individual feature spec dedicated to each step of the process. changelog: Internal, Automated Testing, Improve performance of automated integration tests * More complete stubbing for phone-based IdV session * Try stubbing GPO as well * Improve accuracy of GPO stubbing See: https://github.com/18F/identity-idp/blob/d9ae660f5208570e12d1c81997b499015496fbfe/app/controllers/idv/gpo_controller.rb#L100 * Create stub_idv_session helper See: https://github.com/18F/identity-idp/pull/6447#discussion_r889191026 --- lib/idp/constants.rb | 2 +- spec/support/features/idv_step_helper.rb | 40 +++++++++++++++++-- .../idv_examples/sp_requested_attributes.rb | 2 +- 3 files changed, 38 insertions(+), 6 deletions(-) diff --git a/lib/idp/constants.rb b/lib/idp/constants.rb index 4a4034232c1..417c9f481a1 100644 --- a/lib/idp/constants.rb +++ b/lib/idp/constants.rb @@ -30,6 +30,6 @@ module Constants MOCK_IDV_APPLICANT_WITH_SSN = MOCK_IDV_APPLICANT.merge(ssn: '900-66-1234').freeze - MOCK_IDV_APPLICANT_WITH_PHONE = MOCK_IDV_APPLICANT_WITH_SSN.merge(phone: '14065551234').freeze + MOCK_IDV_APPLICANT_WITH_PHONE = MOCK_IDV_APPLICANT_WITH_SSN.merge(phone: '12025551212').freeze end end diff --git a/spec/support/features/idv_step_helper.rb b/spec/support/features/idv_step_helper.rb index 8116e513c14..d2cd8f98a2c 100644 --- a/spec/support/features/idv_step_helper.rb +++ b/spec/support/features/idv_step_helper.rb @@ -43,8 +43,20 @@ def complete_idv_steps_before_phone_otp_verification_step(user = user_with_2fa) end def complete_idv_steps_with_phone_before_review_step(user = user_with_2fa) - complete_idv_steps_before_phone_step(user) - complete_phone_step(user) + if IdentityConfig.store.idv_api_enabled_steps.include?('password_confirm') + sign_in_and_2fa_user(user) + stub_idv_session( + applicant: Idp::Constants::MOCK_IDV_APPLICANT_WITH_PHONE, + resolution_successful: true, + address_verification_mechanism: 'phone', + user_phone_confirmation: true, + vendor_phone_confirmation: true, + ) + visit idv_app_path(step: :password_confirm) + else + complete_idv_steps_before_phone_step(user) + complete_phone_step(user) + end end def complete_review_step(user) @@ -64,8 +76,18 @@ def complete_idv_steps_with_phone_before_confirmation_step(user = user_with_2fa) # rubocop:enable Layout/LineLength def complete_idv_steps_with_gpo_before_review_step(user = user_with_2fa) - complete_idv_steps_before_gpo_step(user) - click_on t('idv.buttons.mail.send') + if IdentityConfig.store.idv_api_enabled_steps.include?('password_confirm') + sign_in_and_2fa_user(user) + stub_idv_session( + applicant: Idp::Constants::MOCK_IDV_APPLICANT_WITH_SSN, + resolution_successful: 'phone', + address_verification_mechanism: 'gpo', + ) + visit idv_app_path(step: :password_confirm) + else + complete_idv_steps_before_gpo_step(user) + click_on t('idv.buttons.mail.send') + end end def complete_idv_steps_with_gpo_before_confirmation_step(user = user_with_2fa) @@ -78,4 +100,14 @@ def complete_idv_steps_with_gpo_before_confirmation_step(user = user_with_2fa) def complete_idv_steps_before_step(step, user = user_with_2fa) send("complete_idv_steps_before_#{step}_step", user) end + + private + + def stub_idv_session(**session_attributes) + allow(Idv::Session).to receive(:new).and_wrap_original do |original, kwargs| + result = original.call(**kwargs) + kwargs[:user_session][:idv].merge!(session_attributes) + result + end + end end diff --git a/spec/support/idv_examples/sp_requested_attributes.rb b/spec/support/idv_examples/sp_requested_attributes.rb index 059a6e76ddf..ffe899cc569 100644 --- a/spec/support/idv_examples/sp_requested_attributes.rb +++ b/spec/support/idv_examples/sp_requested_attributes.rb @@ -101,7 +101,7 @@ expect(page).to have_content t('help_text.requested_attributes.full_name') expect(page).to have_content 'FAKEY MCFAKERSON' expect(page).to have_content t('help_text.requested_attributes.phone') - expect(page).to have_content '+1 406-555-1234' + expect(page).to have_content '+1 202-555-1212' expect(page).to have_content t('help_text.requested_attributes.social_security_number') expect(page).to have_content DocAuthHelper::GOOD_SSN end From 175666f56786c51428cf1c92394ab115e9a0cc06 Mon Sep 17 00:00:00 2001 From: Mitchell Henke Date: Fri, 3 Jun 2022 16:46:30 -0500 Subject: [PATCH 17/28] Only show reset personal key if user has an active profile (#6452) * add failing spec * Only show reset personal key if user has an active profile changelog: Bug Fixes, Account Management, Only show reset personal key if user has an active profile Co-authored-by: Andrew Duthie Co-authored-by: Andrew Duthie --- app/presenters/navigation_presenter.rb | 2 +- spec/factories/users.rb | 8 ++++++++ spec/presenters/navigation_presenter_spec.rb | 14 +++++++++++++- 3 files changed, 22 insertions(+), 2 deletions(-) diff --git a/app/presenters/navigation_presenter.rb b/app/presenters/navigation_presenter.rb index c4f5c2f4918..80539314dd9 100644 --- a/app/presenters/navigation_presenter.rb +++ b/app/presenters/navigation_presenter.rb @@ -17,7 +17,7 @@ def navigation_items NavItem.new(I18n.t('account.navigation.add_email'), add_email_path), NavItem.new(I18n.t('account.navigation.edit_password'), manage_password_path), NavItem.new(I18n.t('account.navigation.delete_account'), account_delete_path), - user.encrypted_recovery_code_digest.present? ? NavItem.new( + user.encrypted_recovery_code_digest.present? && user.active_profile ? NavItem.new( I18n.t('account.navigation.reset_personal_key'), create_new_personal_key_path ) : nil, ].compact diff --git a/spec/factories/users.rb b/spec/factories/users.rb index 4119cbea806..16506e617a1 100644 --- a/spec/factories/users.rb +++ b/spec/factories/users.rb @@ -191,5 +191,13 @@ create(:profile, :active, :verified, :with_pii, user: user) end end + + trait :deactivated_password_reset_profile do + signed_up + + after :build do |user| + create(:profile, :password_reset, :with_pii, user: user) + end + end end end diff --git a/spec/presenters/navigation_presenter_spec.rb b/spec/presenters/navigation_presenter_spec.rb index 2e4badc2880..035567e49ec 100644 --- a/spec/presenters/navigation_presenter_spec.rb +++ b/spec/presenters/navigation_presenter_spec.rb @@ -8,7 +8,7 @@ describe '#navigation_items' do describe 'personal key' do context 'for a user with a personal key' do - let(:user) { build(:user, personal_key: 'abcdef') } + let(:user) { build(:user, :proofed) } it 'has a link to reset personal key' do nav_item = navigation.navigation_items.first.children.last @@ -25,6 +25,18 @@ expect(has_reset_personal_key).to eq(false) end end + + context 'for a proofed user with deactivated profile due to password reset' do + let(:user) { build(:user, :deactivated_password_reset_profile) } + + it 'does not have a link to reset personal key' do + has_reset_personal_key = navigation.navigation_items.first.children.any? do |item| + item.title == I18n.t('account.navigation.reset_personal_key') + end + + expect(has_reset_personal_key).to eq(false) + end + end end end end From c5c8fe6cf2546392d90b9f7a4c2065848bb7868f Mon Sep 17 00:00:00 2001 From: Andrew Duthie Date: Mon, 6 Jun 2022 10:31:02 -0400 Subject: [PATCH 18/28] Consolidate IdV cancellation feature specs (#6454) * Consolidate IdV cancellation feature specs **Why**: To improve performance of feature specs, run 3 specs for IdV cancellation instead of 24. * Add changelog changelog: Internal, Automated Testing, Improve performance of automated integration tests --- .../idv/cancel_spec.rb} | 83 +++++++++---------- .../phone_otp_delivery_selection_step_spec.rb | 5 -- .../steps/phone_otp_verification_step_spec.rb | 5 -- spec/features/idv/steps/phone_step_spec.rb | 5 -- spec/features/idv/steps/review_step_spec.rb | 5 -- 5 files changed, 38 insertions(+), 65 deletions(-) rename spec/{support/idv_examples/cancel_at_idv_step.rb => features/idv/cancel_spec.rb} (50%) diff --git a/spec/support/idv_examples/cancel_at_idv_step.rb b/spec/features/idv/cancel_spec.rb similarity index 50% rename from spec/support/idv_examples/cancel_at_idv_step.rb rename to spec/features/idv/cancel_spec.rb index 3f6f855b875..0e4869276cd 100644 --- a/spec/support/idv_examples/cancel_at_idv_step.rb +++ b/spec/features/idv/cancel_spec.rb @@ -1,11 +1,16 @@ -shared_examples 'cancel at idv step' do |step, sp| - include SamlAuthHelper +require 'rails_helper' +describe 'cancel IdV', :js do + include IdvStepHelper + include DocAuthHelper + + let(:sp) { nil } let(:fake_analytics) { FakeAnalytics.new } before do start_idv_from_sp(sp) - complete_idv_steps_before_step(step) + sign_in_and_2fa_user + complete_doc_auth_steps_before_agreement_step allow_any_instance_of(ApplicationController).to receive(:analytics).and_return(fake_analytics) end @@ -16,21 +21,40 @@ expect(page).to have_content(t('headings.cancellations.prompt')) expect(current_path).to eq(idv_cancel_path) - expect(fake_analytics).to have_logged_event( - 'IdV: cancellation visited', - step: step.to_s, - ) + expect(fake_analytics).to have_logged_event('IdV: cancellation visited', step: 'agreement') click_on t('links.go_back') expect(current_path).to eq(original_path) - expect(fake_analytics).to have_logged_event( - 'IdV: cancellation go back', - step: step.to_s, + expect(fake_analytics).to have_logged_event('IdV: cancellation go back', step: 'agreement') + end + + it 'shows a cancellation message with option to cancel and reset idv' do + click_link t('links.cancel') + + expect(page).to have_content(t('headings.cancellations.prompt')) + expect(current_path).to eq(idv_cancel_path) + expect(fake_analytics).to have_logged_event('IdV: cancellation visited', step: 'agreement') + + click_on t('forms.buttons.cancel') + + expect(page).to have_content(t('headings.cancellations.confirmation', app_name: APP_NAME)) + expect(current_path).to eq(idv_cancel_path) + expect(page).to have_link( + "‹ #{t('links.back_to_sp', sp: t('links.my_account'))}", + href: account_url, ) + expect(fake_analytics).to have_logged_event('IdV: cancellation confirmed', step: 'agreement') + + # After visiting /verify, expect to redirect to the jurisdiction step, + # the first step in the IdV flow + visit idv_path + expect(current_path).to eq(idv_doc_auth_step_path(step: :welcome)) end - context 'with an sp', if: sp do + context 'with an sp' do + let(:sp) { :oidc } + it 'shows the user a cancellation message with the option to cancel and reset idv' do sp_name = 'Test SP' allow_any_instance_of(ServiceProviderSessionDecorator).to receive(:sp_name). @@ -40,48 +64,17 @@ expect(page).to have_content(t('headings.cancellations.prompt')) expect(current_path).to eq(idv_cancel_path) - expect(fake_analytics).to have_logged_event('IdV: cancellation visited', step: step.to_s) + expect(fake_analytics).to have_logged_event('IdV: cancellation visited', step: 'agreement') click_on t('forms.buttons.cancel') expect(page).to have_content(t('headings.cancellations.confirmation', app_name: APP_NAME)) expect(current_path).to eq(idv_cancel_path) - expect(fake_analytics).to have_logged_event( - 'IdV: cancellation confirmed', - step: step.to_s, - ) + expect(fake_analytics).to have_logged_event('IdV: cancellation confirmed', step: 'agreement') expect(page).to have_link( "‹ #{t('links.back_to_sp', sp: sp_name)}", - href: return_to_sp_failure_to_proof_path(step: step, location: 'cancel'), - ) - - # After visiting /verify, expect to redirect to the jurisdiction step, - # the first step in the IdV flow - visit idv_path - expect(current_path).to eq(idv_doc_auth_step_path(step: :welcome)) - end - end - - context 'without an sp' do - it 'shows a cancellation message with option to cancel and reset idv', if: sp.nil? do - click_link t('links.cancel') - - expect(page).to have_content(t('headings.cancellations.prompt')) - expect(current_path).to eq(idv_cancel_path) - expect(fake_analytics).to have_logged_event('IdV: cancellation visited', step: step.to_s) - - click_on t('forms.buttons.cancel') - - expect(page).to have_content(t('headings.cancellations.confirmation', app_name: APP_NAME)) - expect(current_path).to eq(idv_cancel_path) - expect(page).to have_link( - "‹ #{t('links.back_to_sp', sp: t('links.my_account'))}", - href: account_url, - ) - expect(fake_analytics).to have_logged_event( - 'IdV: cancellation confirmed', - step: step.to_s, + href: return_to_sp_failure_to_proof_path(step: 'agreement', location: 'cancel'), ) # After visiting /verify, expect to redirect to the jurisdiction step, diff --git a/spec/features/idv/steps/phone_otp_delivery_selection_step_spec.rb b/spec/features/idv/steps/phone_otp_delivery_selection_step_spec.rb index 04802285ed1..d639b17dce4 100644 --- a/spec/features/idv/steps/phone_otp_delivery_selection_step_spec.rb +++ b/spec/features/idv/steps/phone_otp_delivery_selection_step_spec.rb @@ -110,9 +110,4 @@ expect(page).to have_content(I18n.t('telephony.error.friendly_message.invalid_calling_area')) expect(page).to have_current_path(idv_phone_path) end - - context 'cancelling IdV' do - it_behaves_like 'cancel at idv step', :phone_otp_delivery_selection - it_behaves_like 'cancel at idv step', :phone_otp_delivery_selection, :oidc - end end diff --git a/spec/features/idv/steps/phone_otp_verification_step_spec.rb b/spec/features/idv/steps/phone_otp_verification_step_spec.rb index 07295d2a3bb..d424a51c9e1 100644 --- a/spec/features/idv/steps/phone_otp_verification_step_spec.rb +++ b/spec/features/idv/steps/phone_otp_verification_step_spec.rb @@ -97,9 +97,4 @@ expect(page).to have_content(calling_area_error.friendly_message) expect(page).to have_current_path(idv_phone_path) end - - context 'cancelling IdV' do - it_behaves_like 'cancel at idv step', :phone_otp_verification - it_behaves_like 'cancel at idv step', :phone_otp_verification, :oidc - end end diff --git a/spec/features/idv/steps/phone_step_spec.rb b/spec/features/idv/steps/phone_step_spec.rb index 23337e0f9b5..712e0f23935 100644 --- a/spec/features/idv/steps/phone_step_spec.rb +++ b/spec/features/idv/steps/phone_step_spec.rb @@ -148,11 +148,6 @@ it_behaves_like 'async timed out' end - context 'cancelling IdV' do - it_behaves_like 'cancel at idv step', :phone - it_behaves_like 'cancel at idv step', :phone, :oidc - end - context "when the user's information cannot be verified" do it_behaves_like 'fail to verify idv info', :phone diff --git a/spec/features/idv/steps/review_step_spec.rb b/spec/features/idv/steps/review_step_spec.rb index 8c0ce2dbd57..90343834f46 100644 --- a/spec/features/idv/steps/review_step_spec.rb +++ b/spec/features/idv/steps/review_step_spec.rb @@ -145,9 +145,4 @@ expect(page).to have_current_path(idv_app_path(step: :personal_key)) end end - - context 'cancelling IdV' do - it_behaves_like 'cancel at idv step', :review - it_behaves_like 'cancel at idv step', :review, :oidc - end end From 7c29e66beea028a5fc757bc0b4fecc9f0a2d460e Mon Sep 17 00:00:00 2001 From: Mitchell Henke Date: Mon, 6 Jun 2022 11:24:13 -0500 Subject: [PATCH 19/28] Remove references to database throttles (#6428) changelog: Internal, Rate Limiting, Remove deprecated reading and writing of Postgres-based rate limiting --- app/jobs/remove_old_throttles_job.rb | 44 ---- app/models/user.rb | 1 - app/services/throttle.rb | 77 ++---- config/initializers/job_configurations.rb | 6 - spec/jobs/remove_old_throttles_job_spec.rb | 57 ----- spec/models/user_spec.rb | 1 - spec/services/throttle_spec.rb | 277 ++++++++++----------- 7 files changed, 145 insertions(+), 318 deletions(-) delete mode 100644 app/jobs/remove_old_throttles_job.rb delete mode 100644 spec/jobs/remove_old_throttles_job_spec.rb diff --git a/app/jobs/remove_old_throttles_job.rb b/app/jobs/remove_old_throttles_job.rb deleted file mode 100644 index 56be2bb65d9..00000000000 --- a/app/jobs/remove_old_throttles_job.rb +++ /dev/null @@ -1,44 +0,0 @@ -class RemoveOldThrottlesJob < ApplicationJob - queue_as :low - - WINDOW = 30.days.freeze - - include GoodJob::ActiveJobExtensions::Concurrency - - good_job_control_concurrency_with( - total_limit: 1, - key: -> do - rounded = TimeService.round_time(time: arguments.first, interval: 1.hour) - "remove-old-throttles-#{rounded.to_i}" - end, - ) - - discard_on GoodJob::ActiveJobExtensions::Concurrency::ConcurrencyExceededError - - def perform(now, limit: 500, total_limit: 50_000) - max_window = Throttle::THROTTLE_CONFIG.map { |_, config| config[:attempt_window] }.max - total_removed = 0 - - loop do - removed_count = DatabaseThrottle. - where('updated_at < ?', now - (WINDOW + max_window.minutes)). - or(DatabaseThrottle.where(updated_at: nil)). - limit(limit). - delete_all - - total_removed += removed_count - - Rails.logger.info( - { - name: 'remove_old_throttles', - removed_count: removed_count, - total_removed: total_removed, - total_limit: total_limit, - }.to_json, - ) - - break if removed_count.zero? - break if total_removed >= total_limit - end - end -end diff --git a/app/models/user.rb b/app/models/user.rb index 65dcc450a86..87f13543af6 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -41,7 +41,6 @@ class User < ApplicationRecord has_many :auth_app_configurations, dependent: :destroy, inverse_of: :user has_many :backup_code_configurations, dependent: :destroy has_many :document_capture_sessions, dependent: :destroy - has_many :database_throttles, dependent: :destroy has_one :registration_log, dependent: :destroy has_one :proofing_component, dependent: :destroy has_many :service_providers, diff --git a/app/services/throttle.rb b/app/services/throttle.rb index 7d35261b7af..df0e89c150e 100644 --- a/app/services/throttle.rb +++ b/app/services/throttle.rb @@ -75,19 +75,15 @@ def initialize(throttle_type:, user: nil, target: nil) end def attempts - if IdentityConfig.store.redis_throttle_enabled - redis_attempts.to_i - else - postgres_throttle.attempts - end + return @redis_attempts.to_i if defined?(@redis_attempts) + + fetch_state! + + @redis_attempts.to_i end def throttled? - if IdentityConfig.store.redis_throttle_enabled - !expired? && maxed? - else - postgres_throttle.throttled? - end + !expired? && maxed? end def throttled_else_increment? @@ -100,46 +96,22 @@ def throttled_else_increment? end def attempted_at - if IdentityConfig.store.redis_throttle_enabled - redis_attempted_at - else - postgres_throttle.attempted_at - end + return @redis_attempted_at if defined?(@redis_attempted_at) + + fetch_state! + + @redis_attempted_at end def expires_at - if IdentityConfig.store.redis_throttle_enabled - return Time.zone.now if redis_attempted_at.blank? - redis_attempted_at + Throttle.attempt_window_in_minutes(throttle_type).minutes - else - postgres_throttle.expires_at - end + return Time.zone.now if attempted_at.blank? + attempted_at + Throttle.attempt_window_in_minutes(throttle_type).minutes end def remaining_count return 0 if throttled? - if IdentityConfig.store.redis_throttle_enabled - Throttle.max_attempts(throttle_type) - attempts - else - postgres_throttle.remaining_count - end - end - - def redis_attempts - return @redis_attempts if defined?(@redis_attempts) - - fetch_state! - - @redis_attempts - end - - def redis_attempted_at - return @redis_attempted_at if defined?(@redis_attempted_at) - - fetch_state! - - @redis_attempted_at + Throttle.max_attempts(throttle_type) - attempts end def expired? @@ -147,7 +119,7 @@ def expired? end def maxed? - redis_attempts && redis_attempts >= Throttle.max_attempts(throttle_type) + attempts && attempts >= Throttle.max_attempts(throttle_type) end def increment! @@ -165,8 +137,6 @@ def increment! @redis_attempts = value.to_i @redis_attempted_at = Time.zone.now - postgres_throttle.increment - attempts end @@ -205,8 +175,6 @@ def reset! client.del(key) end - postgres_throttle.reset - @redis_attempts = 0 @redis_attempted_at = nil end @@ -225,11 +193,6 @@ def increment_to_throttled! @redis_attempts = value.to_i @redis_attempted_at = Time.zone.now - postgres_throttle.update( - attempts: Throttle.max_attempts(throttle_type), - attempted_at: Time.zone.now, - ) - attempts end @@ -241,16 +204,6 @@ def key end end - def postgres_throttle - return @postgres_throttle if @postgres_throttle - - @postgres_throttle ||= DatabaseThrottle.for( - throttle_type: throttle_type, - user: @user, - target: @target, - ) - end - def self.attempt_window_in_minutes(throttle_type) THROTTLE_CONFIG.dig(throttle_type, :attempt_window) end diff --git a/config/initializers/job_configurations.rb b/config/initializers/job_configurations.rb index 21fe8f3dac3..c51dbe76413 100644 --- a/config/initializers/job_configurations.rb +++ b/config/initializers/job_configurations.rb @@ -189,12 +189,6 @@ cron: cron_24h, args: -> { [Time.zone.yesterday] }, }, - # Removes old rows from the Throttles table - remove_old_throttles: { - class: 'RemoveOldThrottlesJob', - cron: cron_1h, - args: -> { [Time.zone.now] }, - }, # Sync opted out phone numbers from AWS phone_number_opt_out_sync_job: { class: 'PhoneNumberOptOutSyncJob', diff --git a/spec/jobs/remove_old_throttles_job_spec.rb b/spec/jobs/remove_old_throttles_job_spec.rb deleted file mode 100644 index c912f5239e1..00000000000 --- a/spec/jobs/remove_old_throttles_job_spec.rb +++ /dev/null @@ -1,57 +0,0 @@ -require 'rails_helper' - -RSpec.describe RemoveOldThrottlesJob do - let(:limit) { 1 } - let(:total_limit) { 10 } - let(:now) { Time.zone.now } - - subject(:job) { RemoveOldThrottlesJob.new } - - describe '#perform' do - subject(:perform) { job.perform(now, limit: limit, total_limit: total_limit) } - - it 'deletes throttles that are older than WINDOW' do - _old_throttle = create( - :database_throttle, - target: SecureRandom.hex, - updated_at: now - 45.days, - ) - new_throttle = create(:database_throttle, target: SecureRandom.hex, updated_at: now) - - perform - - expect(DatabaseThrottle.all.map(&:id)).to match_array([new_throttle.id]) - end - - it 'deletes legacy rows with updated_at: nil' do - legacy_throttle = create(:database_throttle, target: SecureRandom.hex) - legacy_throttle.update(updated_at: nil) - expect(legacy_throttle.reload.updated_at).to be_nil - - expect { perform }.to(change { DatabaseThrottle.count }.to(0)) - end - - it 'stops after total_limit jobs' do - (total_limit + 1).times do - create(:database_throttle, target: SecureRandom.hex, updated_at: now - 45.days) - end - - expect { perform }.to(change { DatabaseThrottle.count }.to(1)) - end - end - - describe '#good_job_concurrency_key' do - it 'is the job name and the current time, rounded to the nearest hour' do - now = Time.zone.at(1629817200) - - job_now = RemoveOldThrottlesJob.new(now) - expect(job_now.good_job_concurrency_key).to eq("remove-old-throttles-#{now.to_i}") - - job_plus_30m = RemoveOldThrottlesJob.new(now + 30.minutes) - expect(job_plus_30m.good_job_concurrency_key).to eq(job_now.good_job_concurrency_key) - - job_plus_1h = RemoveOldThrottlesJob.new(now + 1.hour) - expect(job_plus_1h.good_job_concurrency_key).to_not eq(job_now.good_job_concurrency_key) - end - end -end diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index 5fad44370b1..24ea6542c2e 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -11,7 +11,6 @@ it { is_expected.to have_many(:phone_configurations) } it { is_expected.to have_many(:webauthn_configurations) } it { is_expected.to have_one(:proofing_component) } - it { is_expected.to have_many(:database_throttles) } end it 'does not send an email when #create is called' do diff --git a/spec/services/throttle_spec.rb b/spec/services/throttle_spec.rb index 66ff794ad2a..e45965d0f13 100644 --- a/spec/services/throttle_spec.rb +++ b/spec/services/throttle_spec.rb @@ -14,211 +14,194 @@ ) end - shared_examples 'throttle' do - describe '.new' do - context 'when target is a string' do - subject(:for_target) { Throttle.new(target: target, throttle_type: throttle_type) } - - context 'target is not a string' do - it 'raises an error' do - expect { Throttle.new(target: 3, throttle_type: throttle_type) }. - to raise_error(ArgumentError) - end + describe '.new' do + context 'when target is a string' do + subject(:for_target) { Throttle.new(target: target, throttle_type: throttle_type) } + + context 'target is not a string' do + it 'raises an error' do + expect { Throttle.new(target: 3, throttle_type: throttle_type) }. + to raise_error(ArgumentError) end end + end - it 'throws an error when neither user nor target are provided' do - expect { Throttle.new(throttle_type: throttle_type) }. - to raise_error( - ArgumentError, - 'Throttle must have a user or a target, but neither were provided', - ) - end + it 'throws an error when neither user nor target are provided' do + expect { Throttle.new(throttle_type: throttle_type) }. + to raise_error( + ArgumentError, + 'Throttle must have a user or a target, but neither were provided', + ) + end - it 'throws an error when both user and target are provided' do - expect { Throttle.new(throttle_type: throttle_type) }. - to raise_error( - ArgumentError, - 'Throttle must have a user or a target, but neither were provided', - ) - end + it 'throws an error when both user and target are provided' do + expect { Throttle.new(throttle_type: throttle_type) }. + to raise_error( + ArgumentError, + 'Throttle must have a user or a target, but neither were provided', + ) + end - it 'throws an error for an invalid throttle_type' do - expect { Throttle.new(throttle_type: :abc_123, target: '1') }. - to raise_error( - ArgumentError, - 'throttle_type is not valid', - ) - end + it 'throws an error for an invalid throttle_type' do + expect { Throttle.new(throttle_type: :abc_123, target: '1') }. + to raise_error( + ArgumentError, + 'throttle_type is not valid', + ) end + end - describe '.attempt_window_in_minutes' do - it 'returns configured attempt window for throttle type' do - expect(Throttle.attempt_window_in_minutes(throttle_type)).to eq(attempt_window) - end + describe '.attempt_window_in_minutes' do + it 'returns configured attempt window for throttle type' do + expect(Throttle.attempt_window_in_minutes(throttle_type)).to eq(attempt_window) + end - it 'is indifferent to throttle type stringiness' do - expect(Throttle.attempt_window_in_minutes(throttle_type.to_s)).to eq(attempt_window) - end + it 'is indifferent to throttle type stringiness' do + expect(Throttle.attempt_window_in_minutes(throttle_type.to_s)).to eq(attempt_window) end + end - describe '.max_attempts' do - it 'returns configured attempt window for throttle type' do - expect(Throttle.max_attempts(throttle_type)).to eq(max_attempts) - end + describe '.max_attempts' do + it 'returns configured attempt window for throttle type' do + expect(Throttle.max_attempts(throttle_type)).to eq(max_attempts) + end - it 'is indifferent to throttle type stringiness' do - expect(Throttle.max_attempts(throttle_type.to_s)).to eq(max_attempts) - end + it 'is indifferent to throttle type stringiness' do + expect(Throttle.max_attempts(throttle_type.to_s)).to eq(max_attempts) end + end - describe '#increment!' do - subject(:throttle) { Throttle.new(target: 'aaa', throttle_type: :idv_doc_auth) } + describe '#increment!' do + subject(:throttle) { Throttle.new(target: 'aaa', throttle_type: :idv_doc_auth) } - it 'increments db and redis attempts' do - expect(throttle.attempts).to eq 0 - throttle.increment! - expect(throttle.attempts).to eq 1 - expect(throttle.postgres_throttle.attempts).to eq 1 - end + it 'increments attempts' do + expect(throttle.attempts).to eq 0 + throttle.increment! + expect(throttle.attempts).to eq 1 end + end - describe '#throttled?' do - let(:throttle_type) { :idv_doc_auth } - let(:max_attempts) { IdentityConfig.store.doc_auth_max_attempts } - let(:attempt_window_in_minutes) { IdentityConfig.store.doc_auth_attempt_window_in_minutes } - - subject(:throttle) { Throttle.new(target: '1', throttle_type: throttle_type) } + describe '#throttled?' do + let(:throttle_type) { :idv_doc_auth } + let(:max_attempts) { IdentityConfig.store.doc_auth_max_attempts } + let(:attempt_window_in_minutes) { IdentityConfig.store.doc_auth_attempt_window_in_minutes } - it 'returns true if throttled' do - max_attempts.times do - throttle.increment! - end + subject(:throttle) { Throttle.new(target: '1', throttle_type: throttle_type) } - expect(throttle.throttled?).to eq(true) + it 'returns true if throttled' do + max_attempts.times do + throttle.increment! end - it 'returns false if the attempts < max_attempts' do - (max_attempts - 1).times do - expect(throttle.throttled?).to eq(false) - throttle.increment! - end + expect(throttle.throttled?).to eq(true) + end + it 'returns false if the attempts < max_attempts' do + (max_attempts - 1).times do expect(throttle.throttled?).to eq(false) + throttle.increment! end - it 'returns false if the attempts <= max_attempts but the window is expired' do - max_attempts.times do - throttle.increment! - end + expect(throttle.throttled?).to eq(false) + end - travel(attempt_window_in_minutes.minutes) do - expect(throttle.throttled?).to eq(false) - end + it 'returns false if the attempts <= max_attempts but the window is expired' do + max_attempts.times do + throttle.increment! + end + + travel(attempt_window_in_minutes.minutes) do + expect(throttle.throttled?).to eq(false) end end + end - describe '#throttled_else_increment?' do - subject(:throttle) { Throttle.new(target: 'aaaa', throttle_type: :idv_doc_auth) } + describe '#throttled_else_increment?' do + subject(:throttle) { Throttle.new(target: 'aaaa', throttle_type: :idv_doc_auth) } - context 'throttle has hit limit' do - before do - (IdentityConfig.store.doc_auth_max_attempts + 1).times do - throttle.increment! - end + context 'throttle has hit limit' do + before do + (IdentityConfig.store.doc_auth_max_attempts + 1).times do + throttle.increment! end + end - it 'is true' do - expect(throttle.throttled_else_increment?).to eq(true) - end + it 'is true' do + expect(throttle.throttled_else_increment?).to eq(true) end + end - context 'throttle has not hit limit' do - it 'is false' do - expect(throttle.throttled_else_increment?).to eq(false) - end + context 'throttle has not hit limit' do + it 'is false' do + expect(throttle.throttled_else_increment?).to eq(false) + end - it 'increments the throttle' do - expect { throttle.throttled_else_increment? }.to change { throttle.attempts }.by(1) - end + it 'increments the throttle' do + expect { throttle.throttled_else_increment? }.to change { throttle.attempts }.by(1) end end + end - describe '#expires_at' do - let(:attempted_at) { nil } - let(:throttle) { Throttle.new(target: '1', throttle_type: throttle_type) } + describe '#expires_at' do + let(:attempted_at) { nil } + let(:throttle) { Throttle.new(target: '1', throttle_type: throttle_type) } - context 'without having attempted' do - it 'returns current time' do - freeze_time do - expect(throttle.expires_at).to eq(Time.zone.now) - end + context 'without having attempted' do + it 'returns current time' do + freeze_time do + expect(throttle.expires_at).to eq(Time.zone.now) end end + end - context 'with expired throttle' do - it 'returns expiration time' do - throttle.increment! + context 'with expired throttle' do + it 'returns expiration time' do + throttle.increment! - travel_to(throttle.attempted_at + 3.days) do - expect(throttle.expires_at).to be_within(1.second). - of(throttle.attempted_at + attempt_window.minutes) - end + travel_to(throttle.attempted_at + 3.days) do + expect(throttle.expires_at).to be_within(1.second). + of(throttle.attempted_at + attempt_window.minutes) end end + end - context 'with active throttle' do - it 'returns expiration time' do - freeze_time do - throttle.increment! - expect(throttle.expires_at).to be_within(1.second). - of(throttle.attempted_at + attempt_window.minutes) - end + context 'with active throttle' do + it 'returns expiration time' do + freeze_time do + throttle.increment! + expect(throttle.expires_at).to be_within(1.second). + of(throttle.attempted_at + attempt_window.minutes) end end end + end - describe '#reset' do - let(:target) { '1' } - let(:subject) { described_class } + describe '#reset' do + let(:target) { '1' } + let(:subject) { described_class } - subject(:throttle) { Throttle.new(target: target, throttle_type: throttle_type) } + subject(:throttle) { Throttle.new(target: target, throttle_type: throttle_type) } - it 'resets attempt count to 0' do - throttle.increment! + it 'resets attempt count to 0' do + throttle.increment! - expect { throttle.reset! }.to change { throttle.attempts }.to(0) - end + expect { throttle.reset! }.to change { throttle.attempts }.to(0) end + end - describe '#remaining_count' do - let(:target) { '1' } - let(:subject) { described_class } - - subject(:throttle) { Throttle.new(target: target, throttle_type: throttle_type) } + describe '#remaining_count' do + let(:target) { '1' } + let(:subject) { described_class } - it 'returns maximium remaining attempts with zero attempts' do - expect(throttle.remaining_count).to eq(Throttle.max_attempts(throttle_type)) - end + subject(:throttle) { Throttle.new(target: target, throttle_type: throttle_type) } - it 'returns zero when throttle limit is reached' do - throttle.increment_to_throttled! - expect(throttle.remaining_count).to eq(0) - end + it 'returns maximium remaining attempts with zero attempts' do + expect(throttle.remaining_count).to eq(Throttle.max_attempts(throttle_type)) end - end - - context 'using Redis as throttle data store' do - before do - allow(IdentityConfig.store).to receive(:redis_throttle_enabled).and_return(true) - end - it_behaves_like 'throttle' - end - context 'using Postgres as throttle data store' do - before do - allow(IdentityConfig.store).to receive(:redis_throttle_enabled).and_return(false) + it 'returns zero when throttle limit is reached' do + throttle.increment_to_throttled! + expect(throttle.remaining_count).to eq(0) end - it_behaves_like 'throttle' end end From 019d5c3e86a6030ebeab320b4b97c21fb5c21daf Mon Sep 17 00:00:00 2001 From: Andrew Duthie Date: Mon, 6 Jun 2022 12:34:27 -0400 Subject: [PATCH 20/28] Remove duplicate IdV cancellation spec (#6458) **Why**: - The described behavior is already covered in `spec/features/idv/cancel_spec.rb`. - Improve test performance changelog: Internal, Automated Testing, Improve performance of automated integration tests --- spec/features/idv/cancel_spec.rb | 6 ++--- spec/features/idv/doc_auth/cancel_spec.rb | 30 ----------------------- 2 files changed, 2 insertions(+), 34 deletions(-) delete mode 100644 spec/features/idv/doc_auth/cancel_spec.rb diff --git a/spec/features/idv/cancel_spec.rb b/spec/features/idv/cancel_spec.rb index 0e4869276cd..f8120adaf7f 100644 --- a/spec/features/idv/cancel_spec.rb +++ b/spec/features/idv/cancel_spec.rb @@ -46,8 +46,7 @@ ) expect(fake_analytics).to have_logged_event('IdV: cancellation confirmed', step: 'agreement') - # After visiting /verify, expect to redirect to the jurisdiction step, - # the first step in the IdV flow + # After visiting /verify, expect to redirect to the first step in the IdV flow. visit idv_path expect(current_path).to eq(idv_doc_auth_step_path(step: :welcome)) end @@ -77,8 +76,7 @@ href: return_to_sp_failure_to_proof_path(step: 'agreement', location: 'cancel'), ) - # After visiting /verify, expect to redirect to the jurisdiction step, - # the first step in the IdV flow + # After visiting /verify, expect to redirect to the first step in the IdV flow. visit idv_path expect(current_path).to eq(idv_doc_auth_step_path(step: :welcome)) end diff --git a/spec/features/idv/doc_auth/cancel_spec.rb b/spec/features/idv/doc_auth/cancel_spec.rb deleted file mode 100644 index 0514a030bdf..00000000000 --- a/spec/features/idv/doc_auth/cancel_spec.rb +++ /dev/null @@ -1,30 +0,0 @@ -require 'rails_helper' - -feature 'doc auth cancel' do - include IdvStepHelper - include DocAuthHelper - - before do - sign_in_and_2fa_user - complete_doc_auth_steps_before_verify_step - end - - it 'correctly restarts doc auth flow upon cancel and revisit' do - expect(page).to have_current_path(idv_doc_auth_verify_step) - - click_link t('links.cancel') - - expect(page).to have_current_path(idv_cancel_path(step: 'verify')) - - click_button t('forms.buttons.cancel') - - expect(page).to have_content(t('headings.cancellations.confirmation', app_name: APP_NAME)) - expect(current_path).to eq(idv_cancel_path) - - visit account_path - expect(current_path).to eq(account_path) - - visit(idv_doc_auth_verify_step) - expect(current_path).to eq(idv_doc_auth_welcome_step) - end -end From 26161f132f4a5c99403e50606792d4d9f0fe48e1 Mon Sep 17 00:00:00 2001 From: Mitchell Henke Date: Mon, 6 Jun 2022 11:51:29 -0500 Subject: [PATCH 21/28] Enable Session Encryption v2 everywhere (#6455) [skip changelog] --- config/application.yml.default | 1 - 1 file changed, 1 deletion(-) diff --git a/config/application.yml.default b/config/application.yml.default index ff751701e70..92929e04bb5 100644 --- a/config/application.yml.default +++ b/config/application.yml.default @@ -380,7 +380,6 @@ production: piv_cac_verify_token_secret: platform_authentication_enabled: false session_encryptor_alert_enabled: true - session_encryptor_v2_enabled: false recurring_jobs_disabled_names: "[]" redis_irs_attempt_api_url: redis://redis.login.gov.internal:6379/2 redis_throttle_url: redis://redis.login.gov.internal:6379/1 From 6df2940105dba8fd982cd197df1b7b24b2ff5a3e Mon Sep 17 00:00:00 2001 From: Zach Margolis Date: Mon, 6 Jun 2022 10:19:32 -0700 Subject: [PATCH 22/28] Only include "billable" events in billing reports (LG-6539) (#6459) A bug report highlighted an inconsistency in our reports, digging in, I found that we create both "billable: true" and "billable: false" rows in sp_return_logs so the reports that added up sp_return_logs for billing were counting some extra. changelog: Internal, Reporting, Only track billable events in billing reports --- .../total_monthly_auth_counts_within_iaa_window.rb | 1 + .../unique_monthly_auth_counts_by_iaa.rb | 1 + .../agency_invoice_iaa_supplement_report_spec.rb | 1 + .../combined_invoice_supplement_report_spec.rb | 1 + ...al_monthly_auth_counts_within_iaa_window_spec.rb | 12 ++++++++++++ .../unique_monthly_auth_counts_by_iaa_spec.rb | 13 +++++++++++++ 6 files changed, 29 insertions(+) diff --git a/app/services/db/monthly_sp_auth_count/total_monthly_auth_counts_within_iaa_window.rb b/app/services/db/monthly_sp_auth_count/total_monthly_auth_counts_within_iaa_window.rb index eb16b4651fe..e7c090d1507 100644 --- a/app/services/db/monthly_sp_auth_count/total_monthly_auth_counts_within_iaa_window.rb +++ b/app/services/db/monthly_sp_auth_count/total_monthly_auth_counts_within_iaa_window.rb @@ -128,6 +128,7 @@ def partial_month_subqueries(issuer:, partial_months:) sp_return_logs.requested_at::date BETWEEN %{range_start} AND %{range_end} AND sp_return_logs.returned_at IS NOT NULL AND sp_return_logs.issuer = %{issuer} + AND sp_return_logs.billable = true GROUP BY sp_return_logs.user_id , sp_return_logs.ial diff --git a/app/services/db/monthly_sp_auth_count/unique_monthly_auth_counts_by_iaa.rb b/app/services/db/monthly_sp_auth_count/unique_monthly_auth_counts_by_iaa.rb index 498ead6e2f3..df505c918e8 100644 --- a/app/services/db/monthly_sp_auth_count/unique_monthly_auth_counts_by_iaa.rb +++ b/app/services/db/monthly_sp_auth_count/unique_monthly_auth_counts_by_iaa.rb @@ -119,6 +119,7 @@ def partial_month_subqueries(issuers:, partial_months:) sp_return_logs.requested_at::date BETWEEN %{range_start} AND %{range_end} AND sp_return_logs.returned_at IS NOT NULL AND sp_return_logs.issuer IN %{issuers} + AND sp_return_logs.billable = true GROUP BY sp_return_logs.user_id , sp_return_logs.ial diff --git a/spec/jobs/reports/agency_invoice_iaa_supplement_report_spec.rb b/spec/jobs/reports/agency_invoice_iaa_supplement_report_spec.rb index bf659fe3bb8..0af4b89e442 100644 --- a/spec/jobs/reports/agency_invoice_iaa_supplement_report_spec.rb +++ b/spec/jobs/reports/agency_invoice_iaa_supplement_report_spec.rb @@ -97,6 +97,7 @@ ial: 1, requested_at: inside_iaa1, returned_at: inside_iaa1, + billable: true, ) # 1 unique user in month 1 at IAA 2 @ IAL 2 diff --git a/spec/jobs/reports/combined_invoice_supplement_report_spec.rb b/spec/jobs/reports/combined_invoice_supplement_report_spec.rb index f34d6133c10..e95582d36b7 100644 --- a/spec/jobs/reports/combined_invoice_supplement_report_spec.rb +++ b/spec/jobs/reports/combined_invoice_supplement_report_spec.rb @@ -107,6 +107,7 @@ ial: 1, requested_at: inside_iaa1, returned_at: inside_iaa1, + billable: true, ) # 1 unique user in month 1 at IAA 2 sp 1 @ IAL 2 diff --git a/spec/services/db/monthly_sp_auth_count/total_monthly_auth_counts_within_iaa_window_spec.rb b/spec/services/db/monthly_sp_auth_count/total_monthly_auth_counts_within_iaa_window_spec.rb index 5097f66776e..e73cdde3894 100644 --- a/spec/services/db/monthly_sp_auth_count/total_monthly_auth_counts_within_iaa_window_spec.rb +++ b/spec/services/db/monthly_sp_auth_count/total_monthly_auth_counts_within_iaa_window_spec.rb @@ -49,9 +49,21 @@ service_provider: service_provider, requested_at: partial_month_date, returned_at: partial_month_date, + billable: true, ) end + # non-billable event during partial month, should be ignored + create( + :sp_return_log, + user: user, + ial: 1, + service_provider: service_provider, + requested_at: partial_month_date, + returned_at: partial_month_date, + billable: false, + ) + # 11 IAL 1 auths during full month create( :monthly_sp_auth_count, diff --git a/spec/services/db/monthly_sp_auth_count/unique_monthly_auth_counts_by_iaa_spec.rb b/spec/services/db/monthly_sp_auth_count/unique_monthly_auth_counts_by_iaa_spec.rb index 3f223aae845..cb20ee443a0 100644 --- a/spec/services/db/monthly_sp_auth_count/unique_monthly_auth_counts_by_iaa_spec.rb +++ b/spec/services/db/monthly_sp_auth_count/unique_monthly_auth_counts_by_iaa_spec.rb @@ -61,6 +61,18 @@ ial: 1, requested_at: inside_partial_month, returned_at: inside_partial_month, + billable: true, + ) + + # non-billable event in partial month, should be ignored + create( + :sp_return_log, + user_id: user1.id, + issuer: issuer1, + ial: 1, + requested_at: inside_partial_month, + returned_at: inside_partial_month, + billable: false, ) # 2 unique user in partial month @ IAL2 @@ -72,6 +84,7 @@ ial: 2, requested_at: inside_partial_month, returned_at: inside_partial_month, + billable: true, ) end From 94731a993979edd5e9e6a9cd4620ecd6ba315e69 Mon Sep 17 00:00:00 2001 From: Nadya Primak Date: Mon, 6 Jun 2022 13:25:22 -0400 Subject: [PATCH 23/28] Update config to add support for additional AAMVA DLDV states (#6444) * add all states except AK, OK, WV, NH * move state values to config as json * fix lint errors * Add changelog changelog: Improvements, AAMVA, LG-6506 Support for additional AAMVA DLDV States * get rid of constant * lint again * Update app/services/idv/steps/verify_base_step.rb Co-authored-by: Andrew Duthie * replace additional constants to fix ruby tests * lint again * Update spec/features/idv/doc_auth/verify_step_spec.rb Co-authored-by: Andrew Duthie * Update spec/features/idv/doc_auth/verify_step_spec.rb Co-authored-by: Andrew Duthie * Update spec/features/idv/doc_auth/verify_step_spec.rb Co-authored-by: Andrew Duthie * extra parenthesis deleted Co-authored-by: Andrew Duthie --- app/services/idv/steps/verify_base_step.rb | 9 +++------ app/services/proofing/mock/state_id_mock_client.rb | 2 +- config/application.yml.default | 1 + lib/identity_config.rb | 1 + spec/features/idv/doc_auth/verify_step_spec.rb | 13 ++++++------- 5 files changed, 12 insertions(+), 14 deletions(-) diff --git a/app/services/idv/steps/verify_base_step.rb b/app/services/idv/steps/verify_base_step.rb index 1fa97d28854..ee66c7118bd 100644 --- a/app/services/idv/steps/verify_base_step.rb +++ b/app/services/idv/steps/verify_base_step.rb @@ -1,11 +1,6 @@ module Idv module Steps class VerifyBaseStep < DocAuthBaseStep - AAMVA_SUPPORTED_JURISDICTIONS = %w[ - AR AZ CO CT DC DE FL GA IA ID IL IN KS KY MA MD ME MI MO MS MT NC ND NE - NJ NM OH OR PA RI SC SD TN TX VA VT WA WI WY - ].to_set.freeze - private def summarize_result_and_throttle_failures(summary_result) @@ -78,7 +73,9 @@ def should_use_aamva?(pii_from_doc) end def aamva_state?(pii_from_doc) - AAMVA_SUPPORTED_JURISDICTIONS.include?(pii_from_doc['state_id_jurisdiction']) + IdentityConfig.store.aamva_supported_jurisdictions.include?( + pii_from_doc['state_id_jurisdiction'], + ) end def aamva_disallowed_for_service_provider? diff --git a/app/services/proofing/mock/state_id_mock_client.rb b/app/services/proofing/mock/state_id_mock_client.rb index 51c09db834a..e87f1ed7f2b 100644 --- a/app/services/proofing/mock/state_id_mock_client.rb +++ b/app/services/proofing/mock/state_id_mock_client.rb @@ -42,7 +42,7 @@ class StateIdMockClient < Proofing::Base private def state_not_supported?(state_id_jurisdiction) - !Idv::Steps::VerifyBaseStep::AAMVA_SUPPORTED_JURISDICTIONS.include? state_id_jurisdiction + !IdentityConfig.store.aamva_supported_jurisdictions.include? state_id_jurisdiction end def invalid_state_id_number?(state_id_number) diff --git a/config/application.yml.default b/config/application.yml.default index 92929e04bb5..709421d0568 100644 --- a/config/application.yml.default +++ b/config/application.yml.default @@ -18,6 +18,7 @@ aamva_auth_request_timeout: 5.0 aamva_auth_url: 'https://example.org:12345/auth/url' aamva_cert_enabled: true aamva_sp_banlist_issuers: '[]' +aamva_supported_jurisdictions: '["AL", "AR", "AZ", "CA", "CO", "CT", "DC", "DE", "FL", "GA", "HI", "IA", "ID", "IL", "IN", "KS", "KY", "LA", "MA", "MD", "ME", "MI", "MN", "MO", "MS", "MT", "NC", "ND", "NE", "NJ", "NM", "NV", "NY", "OH", "OR", "PA", "RI", "SC", "SD", "TN", "TX", "UT", "VA", "VT", "WA", "WI", "WY"]' aamva_verification_request_timeout: 5.0 aamva_verification_url: https://example.org:12345/verification/url all_redirect_uris_cache_duration_minutes: 2 diff --git a/lib/identity_config.rb b/lib/identity_config.rb index fd015990fcc..d78bbb321cc 100644 --- a/lib/identity_config.rb +++ b/lib/identity_config.rb @@ -73,6 +73,7 @@ def self.build_store(config_map) config.add(:aamva_private_key, type: :string) config.add(:aamva_public_key, type: :string) config.add(:aamva_sp_banlist_issuers, type: :json) + config.add(:aamva_supported_jurisdictions, type: :json) config.add(:aamva_verification_request_timeout, type: :float) config.add(:aamva_verification_url) config.add(:all_redirect_uris_cache_duration_minutes, type: :integer) diff --git a/spec/features/idv/doc_auth/verify_step_spec.rb b/spec/features/idv/doc_auth/verify_step_spec.rb index 7141e343d46..607cfc18894 100644 --- a/spec/features/idv/doc_auth/verify_step_spec.rb +++ b/spec/features/idv/doc_auth/verify_step_spec.rb @@ -159,11 +159,10 @@ success: true, errors: {}, context: { stages: [] }, ) - stub_const( - 'Idv::Steps::VerifyBaseStep::AAMVA_SUPPORTED_JURISDICTIONS', - Idv::Steps::VerifyBaseStep::AAMVA_SUPPORTED_JURISDICTIONS + - [Idp::Constants::MOCK_IDV_APPLICANT[:state_id_jurisdiction]], + allow(IdentityConfig.store).to receive(:aamva_supported_jurisdictions).and_return( + [Idp::Constants::MOCK_IDV_APPLICANT[:state_id_jurisdiction]], ) + sign_in_and_2fa_user complete_doc_auth_steps_before_verify_step click_idv_continue @@ -182,11 +181,11 @@ success: true, errors: {}, context: { stages: [] }, ) - stub_const( - 'Idv::Steps::VerifyBaseStep::AAMVA_SUPPORTED_JURISDICTIONS', - Idv::Steps::VerifyBaseStep::AAMVA_SUPPORTED_JURISDICTIONS - + allow(IdentityConfig.store).to receive(:aamva_supported_jurisdictions).and_return( + IdentityConfig.store.aamva_supported_jurisdictions - [Idp::Constants::MOCK_IDV_APPLICANT[:state_id_jurisdiction]], ) + sign_in_and_2fa_user complete_doc_auth_steps_before_verify_step click_idv_continue From 220d02a6b6e186253b9ab6a1079e1a463a85f2b4 Mon Sep 17 00:00:00 2001 From: Sheldon Bachstein Date: Mon, 6 Jun 2022 13:52:04 -0400 Subject: [PATCH 24/28] LG-6330: Route IPP users to the idv phone flow (#6446) * Route users to the idv phone flow Removes the remainder of the IPP flow since the user will now continue on the idv flow * Don't return redirect url. Fix spec. * Remove reptitive comments * Update IPP feature test * Bump gitlab * Use consistent applicant mock data * Don't bother returning nil * Update comment * Add changelog changelog: Upcoming Features, In-Person Proofing, Add address verification flow flow using existing idv pages * Fix failing lint errors * Re-add missing translations * Add comment to pass i18n-unused * Use correct heading keys in flow spec --- app/services/idv/flows/in_person_flow.rb | 24 +++++++++------- app/services/idv/steps/ipp/barcode_step.rb | 11 -------- .../idv/steps/ipp/password_confirm_step.rb | 10 ------- .../idv/steps/ipp/personal_key_step.rb | 10 ------- app/services/idv/steps/ipp/phone_step.rb | 10 ------- app/services/idv/steps/ipp/verify_step.rb | 6 +++- .../idv/in_person/password_confirm.html.erb | 15 ---------- app/views/idv/in_person/personal_key.html.erb | 15 ---------- app/views/idv/in_person/phone.html.erb | 15 ---------- config/locales/in_person_proofing/en.yml | 3 -- config/locales/in_person_proofing/es.yml | 3 -- config/locales/in_person_proofing/fr.yml | 3 -- spec/features/idv/in_person_spec.rb | 16 +++++------ .../idv/steps/ipp/barcode_step_spec.rb | 28 ------------------- .../steps/ipp/password_confirm_step_spec.rb | 28 ------------------- .../idv/steps/ipp/personal_key_step_spec.rb | 28 ------------------- .../services/idv/steps/ipp/phone_step_spec.rb | 28 ------------------- .../idv/steps/ipp/verify_step_spec.rb | 4 +-- 18 files changed, 28 insertions(+), 229 deletions(-) delete mode 100644 app/services/idv/steps/ipp/barcode_step.rb delete mode 100644 app/services/idv/steps/ipp/password_confirm_step.rb delete mode 100644 app/services/idv/steps/ipp/personal_key_step.rb delete mode 100644 app/services/idv/steps/ipp/phone_step.rb delete mode 100644 app/views/idv/in_person/password_confirm.html.erb delete mode 100644 app/views/idv/in_person/personal_key.html.erb delete mode 100644 app/views/idv/in_person/phone.html.erb delete mode 100644 spec/services/idv/steps/ipp/barcode_step_spec.rb delete mode 100644 spec/services/idv/steps/ipp/password_confirm_step_spec.rb delete mode 100644 spec/services/idv/steps/ipp/personal_key_step_spec.rb delete mode 100644 spec/services/idv/steps/ipp/phone_step_spec.rb diff --git a/app/services/idv/flows/in_person_flow.rb b/app/services/idv/flows/in_person_flow.rb index 8f236ae02db..69f24e4fc7b 100644 --- a/app/services/idv/flows/in_person_flow.rb +++ b/app/services/idv/flows/in_person_flow.rb @@ -8,20 +8,15 @@ class InPersonFlow < Flow::BaseFlow state_id: Idv::Steps::Ipp::StateIdStep, # info from state id ssn: Idv::Steps::Ipp::SsnStep, # enter SSN verify: Idv::Steps::Ipp::VerifyStep, # verify entered info - # WILLFIX: add the failure branch for verify step - # WILLFIX: add the verify by mail flow - phone: Idv::Steps::Ipp::PhoneStep, # phone finder - # WILLFIX: add the failure branch for phone step - # WILLFIX: re-use existing password confirm step - password_confirm: Idv::Steps::Ipp::PasswordConfirmStep, - # WILLFIX: re-use existing personal key step - personal_key: Idv::Steps::Ipp::PersonalKeyStep, - barcode: Idv::Steps::Ipp::BarcodeStep, }.freeze ACTIONS = { }.freeze + # WILLFIX: (LG-6308) move this to the barcode page when we finish setting up IPP step + # indicators + # i18n-tasks-use t('step_indicator.flows.idv.go_to_the_post_office') + STEP_INDICATOR_STEPS = [ { name: :find_a_post_office }, { name: :verify_info }, @@ -32,11 +27,20 @@ class InPersonFlow < Flow::BaseFlow def initialize(controller, session, name) @idv_session = self.class.session_idv(session) - super(controller, STEPS, {}, session[name]) + super(controller, STEPS, ACTIONS, session[name]) end def self.session_idv(session) session[:idv] ||= { params: {}, step_attempts: { phone: 0 } } + # WILLFIX: remove this line when we begin collecting user data + session[:idv][:applicant] ||= Idp::Constants::MOCK_IDV_APPLICANT_WITH_SSN + + # WILLFIX: (LG-6349) remove this block when we implement the verify page + session[:idv]['profile_confirmation'] = true + session[:idv]['vendor_phone_confirmation'] = false + session[:idv]['user_phone_confirmation'] = false + session[:idv]['address_verification_mechanism'] = 'phone' + session[:idv]['resolution_successful'] = 'phone' end end end diff --git a/app/services/idv/steps/ipp/barcode_step.rb b/app/services/idv/steps/ipp/barcode_step.rb deleted file mode 100644 index ff6cea38fae..00000000000 --- a/app/services/idv/steps/ipp/barcode_step.rb +++ /dev/null @@ -1,11 +0,0 @@ -module Idv - module Steps - module Ipp - class BarcodeStep < DocAuthBaseStep - # i18n-tasks-use t('step_indicator.flows.idv.go_to_the_post_office') - STEP_INDICATOR_STEP = :go_to_the_post_office - def call; end - end - end - end -end diff --git a/app/services/idv/steps/ipp/password_confirm_step.rb b/app/services/idv/steps/ipp/password_confirm_step.rb deleted file mode 100644 index 8fd40b46ffe..00000000000 --- a/app/services/idv/steps/ipp/password_confirm_step.rb +++ /dev/null @@ -1,10 +0,0 @@ -module Idv - module Steps - module Ipp - class PasswordConfirmStep < DocAuthBaseStep - STEP_INDICATOR_STEP = :secure_account - def call; end - end - end - end -end diff --git a/app/services/idv/steps/ipp/personal_key_step.rb b/app/services/idv/steps/ipp/personal_key_step.rb deleted file mode 100644 index 0ca4189392b..00000000000 --- a/app/services/idv/steps/ipp/personal_key_step.rb +++ /dev/null @@ -1,10 +0,0 @@ -module Idv - module Steps - module Ipp - class PersonalKeyStep < DocAuthBaseStep - STEP_INDICATOR_STEP = :secure_account - def call; end - end - end - end -end diff --git a/app/services/idv/steps/ipp/phone_step.rb b/app/services/idv/steps/ipp/phone_step.rb deleted file mode 100644 index bbfa77b81c0..00000000000 --- a/app/services/idv/steps/ipp/phone_step.rb +++ /dev/null @@ -1,10 +0,0 @@ -module Idv - module Steps - module Ipp - class PhoneStep < DocAuthBaseStep - STEP_INDICATOR_STEP = :verify_phone_or_address - def call; end - end - end - end -end diff --git a/app/services/idv/steps/ipp/verify_step.rb b/app/services/idv/steps/ipp/verify_step.rb index 6b49fc9db2b..025308b2b51 100644 --- a/app/services/idv/steps/ipp/verify_step.rb +++ b/app/services/idv/steps/ipp/verify_step.rb @@ -3,7 +3,11 @@ module Steps module Ipp class VerifyStep < DocAuthBaseStep STEP_INDICATOR_STEP = :verify_info - def call; end + def call + # send the user to the phone page where they'll continue the remainder of + # the idv flow + redirect_to idv_phone_url + end end end end diff --git a/app/views/idv/in_person/password_confirm.html.erb b/app/views/idv/in_person/password_confirm.html.erb deleted file mode 100644 index 6be83dfccef..00000000000 --- a/app/views/idv/in_person/password_confirm.html.erb +++ /dev/null @@ -1,15 +0,0 @@ -<% title t('titles.doc_auth.verify') %> - -

    - <%= t('in_person_proofing.headings.password_confirm', app_name: APP_NAME) %> -

    - -<%= validated_form_for :doc_auth, - url: url_for, - method: 'put', - html: { autocomplete: 'off', class: 'margin-y-5' } do |f| %> - <%= f.button :button, - t('doc_auth.buttons.continue'), - type: :submit, - class: 'usa-button--big usa-button--wide' %> -<% end %> diff --git a/app/views/idv/in_person/personal_key.html.erb b/app/views/idv/in_person/personal_key.html.erb deleted file mode 100644 index 79313e2f127..00000000000 --- a/app/views/idv/in_person/personal_key.html.erb +++ /dev/null @@ -1,15 +0,0 @@ -<% title t('titles.doc_auth.verify') %> - -

    - <%= t('in_person_proofing.headings.personal_key') %> -

    - -<%= validated_form_for :doc_auth, - url: url_for, - method: 'put', - html: { autocomplete: 'off', class: 'margin-y-5' } do |f| %> - <%= f.button :button, - t('doc_auth.buttons.continue'), - type: :submit, - class: 'usa-button--big usa-button--wide' %> -<% end %> diff --git a/app/views/idv/in_person/phone.html.erb b/app/views/idv/in_person/phone.html.erb deleted file mode 100644 index 51100ad4d1d..00000000000 --- a/app/views/idv/in_person/phone.html.erb +++ /dev/null @@ -1,15 +0,0 @@ -<% title t('titles.doc_auth.verify') %> - -

    - <%= t('in_person_proofing.headings.phone') %> -

    - -<%= validated_form_for :doc_auth, - url: url_for, - method: 'put', - html: { autocomplete: 'off', class: 'margin-y-5' } do |f| %> - <%= f.button :button, - t('doc_auth.buttons.continue'), - type: :submit, - class: 'usa-button--big usa-button--wide' %> -<% end %> diff --git a/config/locales/in_person_proofing/en.yml b/config/locales/in_person_proofing/en.yml index a48e1ce774f..22a6358ced2 100644 --- a/config/locales/in_person_proofing/en.yml +++ b/config/locales/in_person_proofing/en.yml @@ -5,9 +5,6 @@ en: address: Enter your current address barcode: You’re ready to verify your identity in person location: Select a location to verify your ID - password_confirm: Re-enter your %{app_name} password to protect your data - personal_key: Save your personal key - phone: Enter a phone number with your name on the plan ssn: Enter your Social Security number state_id: Enter the information on your ID verify: Please verify your information diff --git a/config/locales/in_person_proofing/es.yml b/config/locales/in_person_proofing/es.yml index 63b35b9d2ec..7e64684e067 100644 --- a/config/locales/in_person_proofing/es.yml +++ b/config/locales/in_person_proofing/es.yml @@ -5,9 +5,6 @@ es: address: Ingrese su dirección actual barcode: Estás listo para verificar tu identidad en persona location: Seleccione una ubicación para verificar su identificación - password_confirm: Vuelve a ingresar tu contraseña de %{app_name} para encriptar tus datos - personal_key: Guarda tu clave personal - phone: Ingresa un número de teléfono para ayudar a verificar tu identidad ssn: Ingresa tu número del seguro social state_id: Introduce los datos de su documento de identidad verify: Por favor verifique su información diff --git a/config/locales/in_person_proofing/fr.yml b/config/locales/in_person_proofing/fr.yml index c5894dd20b9..2ca75fc2863 100644 --- a/config/locales/in_person_proofing/fr.yml +++ b/config/locales/in_person_proofing/fr.yml @@ -5,9 +5,6 @@ fr: address: Entrez votre adresse actuelle barcode: Vous êtes prêt à vérifier votre identité en personne location: Sélectionnez un emplacement pour vérifier votre identité - password_confirm: Entrez à nouveau votre mot de passe %{app_name} pour crypter vos données - personal_key: Enregistrez votre clé personnelle - phone: Entrez un numéro de téléphone pour vous aider à vérifier votre identité ssn: Entrez votre numéro de sécurité sociale state_id: Entrez les informations sur votre pièce d’identité verify: Veuillez vérifier vos informations diff --git a/spec/features/idv/in_person_spec.rb b/spec/features/idv/in_person_spec.rb index c337b1b0fda..8b4406ae9a4 100644 --- a/spec/features/idv/in_person_spec.rb +++ b/spec/features/idv/in_person_spec.rb @@ -5,7 +5,7 @@ include IdvHelper it 'works for a happy path', js: true, allow_browser_log: true do - sign_in_and_2fa_user + user = sign_in_and_2fa_user # welcome step visit idv_doc_auth_welcome_step # only thing used from DocAuthHelper @@ -52,22 +52,20 @@ click_idv_continue # phone page - expect(page).to have_content(t('in_person_proofing.headings.phone')) + expect(page).to have_content(t('idv.titles.session.phone')) + fill_out_phone_form_mfa_phone(user) click_idv_continue # password confirm page expect(page).to have_content( - t('in_person_proofing.headings.password_confirm', app_name: APP_NAME), + t('idv.titles.session.review', app_name: APP_NAME), ) + fill_in t('idv.form.password'), with: Features::SessionHelper::VALID_PASSWORD click_idv_continue # personal key page - expect(page).to have_content(t('in_person_proofing.headings.personal_key')) - click_idv_continue - - # barcode page - expect(page).to have_content(t('in_person_proofing.headings.barcode')) - click_idv_continue + expect(page).to have_content(t('titles.idv.personal_key')) + acknowledge_and_confirm_personal_key # returns to account page expect(page).to have_content(t('headings.account.login_info')) diff --git a/spec/services/idv/steps/ipp/barcode_step_spec.rb b/spec/services/idv/steps/ipp/barcode_step_spec.rb deleted file mode 100644 index cc6641a418e..00000000000 --- a/spec/services/idv/steps/ipp/barcode_step_spec.rb +++ /dev/null @@ -1,28 +0,0 @@ -require 'rails_helper' - -describe Idv::Steps::Ipp::BarcodeStep do - let(:user) { build(:user) } - let(:service_provider) { create(:service_provider) } - let(:controller) do - instance_double( - 'controller', - session: { sp: { issuer: service_provider.issuer } }, - current_user: user, - ) - end - - let(:flow) do - Idv::Flows::InPersonFlow.new(controller, {}, 'idv/in_person') - end - - subject(:step) do - Idv::Steps::Ipp::BarcodeStep.new(flow) - end - - describe '#call' do - it 'works' do - result = step.call - expect(result).to be_nil - end - end -end diff --git a/spec/services/idv/steps/ipp/password_confirm_step_spec.rb b/spec/services/idv/steps/ipp/password_confirm_step_spec.rb deleted file mode 100644 index 582091a3a75..00000000000 --- a/spec/services/idv/steps/ipp/password_confirm_step_spec.rb +++ /dev/null @@ -1,28 +0,0 @@ -require 'rails_helper' - -describe Idv::Steps::Ipp::PasswordConfirmStep do - let(:user) { build(:user) } - let(:service_provider) { create(:service_provider) } - let(:controller) do - instance_double( - 'controller', - session: { sp: { issuer: service_provider.issuer } }, - current_user: user, - ) - end - - let(:flow) do - Idv::Flows::InPersonFlow.new(controller, {}, 'idv/in_person') - end - - subject(:step) do - Idv::Steps::Ipp::PasswordConfirmStep.new(flow) - end - - describe '#call' do - it 'works' do - result = step.call - expect(result).to be_nil - end - end -end diff --git a/spec/services/idv/steps/ipp/personal_key_step_spec.rb b/spec/services/idv/steps/ipp/personal_key_step_spec.rb deleted file mode 100644 index a667e1f5603..00000000000 --- a/spec/services/idv/steps/ipp/personal_key_step_spec.rb +++ /dev/null @@ -1,28 +0,0 @@ -require 'rails_helper' - -describe Idv::Steps::Ipp::PersonalKeyStep do - let(:user) { build(:user) } - let(:service_provider) { create(:service_provider) } - let(:controller) do - instance_double( - 'controller', - session: { sp: { issuer: service_provider.issuer } }, - current_user: user, - ) - end - - let(:flow) do - Idv::Flows::InPersonFlow.new(controller, {}, 'idv/in_person') - end - - subject(:step) do - Idv::Steps::Ipp::PersonalKeyStep.new(flow) - end - - describe '#call' do - it 'works' do - result = step.call - expect(result).to be_nil - end - end -end diff --git a/spec/services/idv/steps/ipp/phone_step_spec.rb b/spec/services/idv/steps/ipp/phone_step_spec.rb deleted file mode 100644 index 28fcb3a4ecd..00000000000 --- a/spec/services/idv/steps/ipp/phone_step_spec.rb +++ /dev/null @@ -1,28 +0,0 @@ -require 'rails_helper' - -describe Idv::Steps::Ipp::PhoneStep do - let(:user) { build(:user) } - let(:service_provider) { create(:service_provider) } - let(:controller) do - instance_double( - 'controller', - session: { sp: { issuer: service_provider.issuer } }, - current_user: user, - ) - end - - let(:flow) do - Idv::Flows::InPersonFlow.new(controller, {}, 'idv/in_person') - end - - subject(:step) do - Idv::Steps::Ipp::PhoneStep.new(flow) - end - - describe '#call' do - it 'works' do - result = step.call - expect(result).to be_nil - end - end -end diff --git a/spec/services/idv/steps/ipp/verify_step_spec.rb b/spec/services/idv/steps/ipp/verify_step_spec.rb index 07fd28e5f04..bfc949757b5 100644 --- a/spec/services/idv/steps/ipp/verify_step_spec.rb +++ b/spec/services/idv/steps/ipp/verify_step_spec.rb @@ -8,6 +8,7 @@ 'controller', session: { sp: { issuer: service_provider.issuer } }, current_user: user, + url_options: {}, ) end @@ -21,8 +22,7 @@ describe '#call' do it 'works' do - result = step.call - expect(result).to be_nil + step.call end end end From 563321857c5bab7a846ecb9a53f5cad6b505a21e Mon Sep 17 00:00:00 2001 From: Malick Diarra Date: Mon, 6 Jun 2022 20:01:42 -0400 Subject: [PATCH 25/28] changelog: Upcoming Feature, Authentication, update to not cancel, but resend user to after mfa path (#6462) * changelog: Upcoming Features, Authentication, update to not cancel but resend user to path LG-6542 * remove params * add test for cancel functionality --- .../users/mfa_selection_controller.rb | 1 + app/views/users/mfa_selection/index.html.erb | 4 ++- .../multiple_mfa_sign_up_spec.rb | 32 +++++++++++++++++++ 3 files changed, 36 insertions(+), 1 deletion(-) diff --git a/app/controllers/users/mfa_selection_controller.rb b/app/controllers/users/mfa_selection_controller.rb index 6a0cca2e26c..0cfc76e1b09 100644 --- a/app/controllers/users/mfa_selection_controller.rb +++ b/app/controllers/users/mfa_selection_controller.rb @@ -9,6 +9,7 @@ class MfaSelectionController < ApplicationController def index @two_factor_options_form = TwoFactorOptionsForm.new(current_user) + @after_setup_path = after_mfa_setup_path @presenter = two_factor_options_presenter analytics.user_registration_2fa_additional_setup_visit end diff --git a/app/views/users/mfa_selection/index.html.erb b/app/views/users/mfa_selection/index.html.erb index b51cda2bb9b..8da0ce458b6 100644 --- a/app/views/users/mfa_selection/index.html.erb +++ b/app/views/users/mfa_selection/index.html.erb @@ -2,6 +2,8 @@ <%= render PageHeadingComponent.new.with_content(t('two_factor_authentication.two_factor_choice')) %> +

    <%= @presenter.intro %>

    + <%= validated_form_for @two_factor_options_form, html: { autocomplete: 'off' }, method: :patch, @@ -21,6 +23,6 @@ <%= f.button :submit, t('forms.buttons.continue'), class: 'usa-button--big usa-button--wide margin-bottom-1' %> <% end %> -<%= render 'shared/cancel', link: destroy_user_session_path %> +<%= render 'shared/cancel', link: @after_setup_path %> <%= javascript_packs_tag_once('webauthn-unhide') %> diff --git a/spec/features/two_factor_authentication/multiple_mfa_sign_up_spec.rb b/spec/features/two_factor_authentication/multiple_mfa_sign_up_spec.rb index 6158711d305..fb0e496ab2f 100644 --- a/spec/features/two_factor_authentication/multiple_mfa_sign_up_spec.rb +++ b/spec/features/two_factor_authentication/multiple_mfa_sign_up_spec.rb @@ -67,6 +67,38 @@ expect(current_path).to eq account_path end + + scenario 'user can select 1 MFA methods and cancels selecting second mfa' do + sign_in_before_2fa + + expect(current_path).to eq authentication_methods_setup_path + + click_2fa_option('backup_code') + + click_continue + + expect(current_path).to eq backup_code_setup_path + + click_continue + + expect(page).to have_link(t('forms.backup_code.download')) + + click_continue + + expect(page).to have_content(t('notices.backup_codes_configured')) + + expect(page).to have_current_path( + auth_method_confirmation_path, + ) + + click_link t('mfa.add') + + expect(page).to have_current_path(second_mfa_setup_path) + + click_link t('links.cancel') + + expect(page).to have_current_path(account_path) + end end describe 'user attempts to submit with only the phone MFA method selected', js: true do From 66c3a5ad89bb8758fa1441988ae93fe7e89d1217 Mon Sep 17 00:00:00 2001 From: Mitchell Henke Date: Mon, 6 Jun 2022 19:23:33 -0500 Subject: [PATCH 26/28] Include more context when logging telephony sent events (#6460) * Include more context when logging telephony sent events changelog: Internal, Logging, Include more context when logging telephony sent events * Update app/controllers/users/two_factor_authentication_controller.rb Co-authored-by: Zach Margolis * Update app/services/analytics_events.rb Co-authored-by: Zach Margolis * fix handling of resend parameter * describe phone fingerprint Co-authored-by: Zach Margolis Co-authored-by: Zach Margolis --- .../two_factor_authentication_controller.rb | 15 +++++-- app/services/analytics_events.rb | 40 +++++++++++++++++++ .../idv/send_phone_confirmation_otp.rb | 1 + .../idv/resend_otp_controller_spec.rb | 1 + ...o_factor_authentication_controller_spec.rb | 14 +++++-- 5 files changed, 64 insertions(+), 7 deletions(-) diff --git a/app/controllers/users/two_factor_authentication_controller.rb b/app/controllers/users/two_factor_authentication_controller.rb index 1f677e8a2b4..55006ae0373 100644 --- a/app/controllers/users/two_factor_authentication_controller.rb +++ b/app/controllers/users/two_factor_authentication_controller.rb @@ -180,7 +180,7 @@ def handle_valid_otp_params(method, default = nil) end def handle_telephony_result(method:, default:) - track_events + track_events(otp_delivery_preference: method) if @telephony_result.success? redirect_to login_two_factor_url( otp_delivery_preference: method, @@ -197,8 +197,17 @@ def handle_telephony_result(method:, default:) end end - def track_events - analytics.track_event(Analytics::TELEPHONY_OTP_SENT, @telephony_result.to_h) + def track_events(otp_delivery_preference:) + analytics.telephony_otp_sent( + area_code: parsed_phone.area_code, + country_code: parsed_phone.country_code, + phone_fingerprint: Pii::Fingerprinter.fingerprint(parsed_phone.e164), + context: context, + otp_delivery_preference: otp_delivery_preference, + resend: params.dig(:otp_delivery_selection_form, :resend), + telephony_response: @telephony_result.to_h, + success: @telephony_result.success?, + ) end def exceeded_otp_send_limit? diff --git a/app/services/analytics_events.rb b/app/services/analytics_events.rb index 1dd1be60cd8..48a3cef1536 100644 --- a/app/services/analytics_events.rb +++ b/app/services/analytics_events.rb @@ -777,6 +777,7 @@ def idv_phone_confirmation_otp_resent( # @param [String] country_code country code of phone number # @param [String] area_code area code of phone number # @param [Boolean] rate_limit_exceeded whether or not the rate limit was exceeded by this attempt + # @param [String] phone_fingerprint the hmac fingerprint of the phone number formatted as e164 # @param [Hash] telephony_response response from Telephony gem # The user requested an OTP to confirm their phone during the IDV phone step def idv_phone_confirmation_otp_sent( @@ -786,6 +787,7 @@ def idv_phone_confirmation_otp_sent( country_code:, area_code:, rate_limit_exceeded:, + phone_fingerprint:, telephony_response:, **extra ) @@ -797,6 +799,7 @@ def idv_phone_confirmation_otp_sent( country_code: country_code, area_code: area_code, rate_limit_exceeded: rate_limit_exceeded, + phone_fingerprint: phone_fingerprint, telephony_response: telephony_response, **extra, ) @@ -1541,6 +1544,43 @@ def saml_auth_request( ) end + # @param [String] area_code + # @param [String] country_code + # @param [String] phone_fingerprint the hmac fingerprint of the phone number formatted as e164 + # @param [String] context the context of the OTP, either "authentication" for confirmed phones + # or "confirmation" for unconfirmed + # @param ["sms","voice"] otp_delivery_preference the channel used to send the message + # @param [Boolean] resend + # @param [Hash] telephony_response + # @param [Boolean] success + # A phone one-time password send was attempted + def telephony_otp_sent( + area_code:, + country_code:, + phone_fingerprint:, + context:, + otp_delivery_preference:, + resend:, + telephony_response:, + success:, + **extra + ) + track_event( + 'Telephony: OTP sent', + { + area_code: area_code, + country_code: country_code, + phone_fingerprint: phone_fingerprint, + context: context, + otp_delivery_preference: otp_delivery_preference, + resend: resend, + telephony_response: telephony_response, + success: success, + **extra, + }, + ) + end + # @param [Boolean] success # @param [Hash] errors # Tracks when the the user has selected and submitted additional MFA methods on user registration diff --git a/app/services/idv/send_phone_confirmation_otp.rb b/app/services/idv/send_phone_confirmation_otp.rb index abc9e8bc235..4a9c79cdaa6 100644 --- a/app/services/idv/send_phone_confirmation_otp.rb +++ b/app/services/idv/send_phone_confirmation_otp.rb @@ -80,6 +80,7 @@ def extra_analytics_attributes otp_delivery_preference: delivery_method, country_code: parsed_phone.country, area_code: parsed_phone.area_code, + phone_fingerprint: Pii::Fingerprinter.fingerprint(parsed_phone.e164), rate_limit_exceeded: rate_limit_exceeded?, telephony_response: @telephony_response, } diff --git a/spec/controllers/idv/resend_otp_controller_spec.rb b/spec/controllers/idv/resend_otp_controller_spec.rb index ea6eada9999..a12f37ea8d2 100644 --- a/spec/controllers/idv/resend_otp_controller_spec.rb +++ b/spec/controllers/idv/resend_otp_controller_spec.rb @@ -54,6 +54,7 @@ expected_result = { success: true, + phone_fingerprint: Pii::Fingerprinter.fingerprint(Phonelib.parse(phone).e164), errors: {}, otp_delivery_preference: :sms, country_code: 'US', diff --git a/spec/controllers/users/two_factor_authentication_controller_spec.rb b/spec/controllers/users/two_factor_authentication_controller_spec.rb index 1c08896a2f8..2ecc2aa6931 100644 --- a/spec/controllers/users/two_factor_authentication_controller_spec.rb +++ b/spec/controllers/users/two_factor_authentication_controller_spec.rb @@ -304,7 +304,7 @@ def index success: true, errors: {}, otp_delivery_preference: 'sms', - resend: nil, + resend: 'true', context: 'authentication', country_code: 'US', area_code: '202', @@ -316,9 +316,12 @@ def index with('OTP: Delivery Selection', analytics_hash) expect(@analytics).to receive(:track_event). ordered. - with(Analytics::TELEPHONY_OTP_SENT, hash_including(success: true)) + with('Telephony: OTP sent', hash_including( + resend: 'true', success: true, otp_delivery_preference: 'sms', + )) - get :send_code, params: { otp_delivery_selection_form: { otp_delivery_preference: 'sms' } } + get :send_code, params: { otp_delivery_selection_form: + { otp_delivery_preference: 'sms', resend: 'true' } } end it 'calls OtpRateLimiter#exceeded_otp_send_limit? and #increment' do @@ -437,7 +440,10 @@ def index with('OTP: Delivery Selection', analytics_hash) expect(@analytics).to receive(:track_event). ordered. - with(Analytics::TELEPHONY_OTP_SENT, hash_including(success: true)) + with('Telephony: OTP sent', hash_including( + success: true, + otp_delivery_preference: 'voice', + )) get :send_code, params: { otp_delivery_selection_form: { otp_delivery_preference: 'voice', From 29dbf386cc543ba1eca5a83aa85b0cc9a35556a7 Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Tue, 7 Jun 2022 08:58:53 -0400 Subject: [PATCH 27/28] Bump identity-style-guide from 6.4.2 to 6.5.0 (#6461) Bumps [identity-style-guide](https://github.com/18F/identity-style-guide) from 6.4.2 to 6.5.0. - [Release notes](https://github.com/18F/identity-style-guide/releases) - [Changelog](https://github.com/18F/identity-style-guide/blob/main/CHANGELOG.md) - [Commits](https://github.com/18F/identity-style-guide/compare/v6.4.2...v6.5.0) --- updated-dependencies: - dependency-name: identity-style-guide dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> --- package.json | 2 +- yarn.lock | 8 ++++---- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/package.json b/package.json index 3959967d335..593e30ce77c 100644 --- a/package.json +++ b/package.json @@ -34,7 +34,7 @@ "fast-glob": "^3.2.7", "focus-trap": "^6.7.1", "foundation-emails": "^2.3.1", - "identity-style-guide": "^6.4.2", + "identity-style-guide": "^6.5.0", "intl-tel-input": "^17.0.8", "react": "^17.0.2", "react-dom": "^17.0.2", diff --git a/yarn.lock b/yarn.lock index 1faf55c3436..e0ee148e1de 100644 --- a/yarn.lock +++ b/yarn.lock @@ -3791,10 +3791,10 @@ iconv-lite@0.6.3, iconv-lite@^0.6.3: dependencies: safer-buffer ">= 2.1.2 < 3.0.0" -identity-style-guide@^6.4.2: - version "6.4.2" - resolved "https://registry.yarnpkg.com/identity-style-guide/-/identity-style-guide-6.4.2.tgz#3efcc311132de24fbe37cada262d3f84c23e72fe" - integrity sha512-NUIFXpoKjVI+Pout3MPx9F0rZ1/dbiDomvecv/VOvZaebD0by2iWtJ6JGm00I5Mhg74G5ePCGjnJeVkD15L7AQ== +identity-style-guide@^6.5.0: + version "6.5.0" + resolved "https://registry.yarnpkg.com/identity-style-guide/-/identity-style-guide-6.5.0.tgz#41c869c2540faa22f80ad89ae9e0c1753e8940df" + integrity sha512-P/ZsOodZn3XyX6LYgrtDKzWe447rPkTUxAYADIGAcl13wv7ghQZJjaL1g1PySzG5PAvxeeoyOJYfRW58NRMUCw== dependencies: domready "1.0.8" uswds "^2.13.3" From 172391d5d4cbd39cb58bb71a4221db6c66c94cd0 Mon Sep 17 00:00:00 2001 From: Andrew Duthie Date: Tue, 7 Jun 2022 14:21:43 -0400 Subject: [PATCH 28/28] Revert AAMVA supported states (#6470) * Revert AAMVA supported states **Why**: There was a misunderstanding about support coverage, which is in-fact unchanged. This keeps the config-based implementation introduced in #6444, while reverting the list of states to its original values. * Add changelog [skip changelog] * Add Hawaii to supported AAMVA states --- config/application.yml.default | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/config/application.yml.default b/config/application.yml.default index 709421d0568..cfb51a0311e 100644 --- a/config/application.yml.default +++ b/config/application.yml.default @@ -18,7 +18,7 @@ aamva_auth_request_timeout: 5.0 aamva_auth_url: 'https://example.org:12345/auth/url' aamva_cert_enabled: true aamva_sp_banlist_issuers: '[]' -aamva_supported_jurisdictions: '["AL", "AR", "AZ", "CA", "CO", "CT", "DC", "DE", "FL", "GA", "HI", "IA", "ID", "IL", "IN", "KS", "KY", "LA", "MA", "MD", "ME", "MI", "MN", "MO", "MS", "MT", "NC", "ND", "NE", "NJ", "NM", "NV", "NY", "OH", "OR", "PA", "RI", "SC", "SD", "TN", "TX", "UT", "VA", "VT", "WA", "WI", "WY"]' +aamva_supported_jurisdictions: '["AR","AZ","CO","CT","DC","DE","FL","GA","HI","IA","ID","IL","IN","KS","KY","MA","MD","ME","MI","MO","MS","MT","NC","ND","NE","NJ","NM","OH","OR","PA","RI","SC","SD","TN","TX","VA","VT","WA","WI","WY"]' aamva_verification_request_timeout: 5.0 aamva_verification_url: https://example.org:12345/verification/url all_redirect_uris_cache_duration_minutes: 2