diff --git a/app/forms/idv/ssn_form.rb b/app/forms/idv/ssn_form.rb index 524df7bd74b..be3c3140b78 100644 --- a/app/forms/idv/ssn_form.rb +++ b/app/forms/idv/ssn_form.rb @@ -1,12 +1,16 @@ module Idv class SsnForm include ActiveModel::Model - include FormSsnValidator - ATTRIBUTES = [:ssn].freeze attr_accessor :ssn + validates :ssn, presence: true + validates_format_of :ssn, + with: /\A\d{3}-?\d{2}-?\d{4}\z/, + message: I18n.t('idv.errors.pattern_mismatch.ssn'), + allow_blank: false + def self.model_name ActiveModel::Name.new(self, nil, 'Ssn') end @@ -18,7 +22,22 @@ def initialize(user) def submit(params) consume_params(params) - FormResponse.new(success: valid?, errors: errors.messages) + FormResponse.new(success: valid?, errors: errors.messages, extra: extra_analytics_attributes) + end + + def ssn_is_unique? + return false if ssn.nil? + + @ssn_is_unique ||= DuplicateSsnFinder.new( + ssn: ssn, + user: @user, + ).ssn_is_unique? + end + + def extra_analytics_attributes + { + ssn_is_unique: ssn_is_unique?, + } end private diff --git a/app/models/profile.rb b/app/models/profile.rb index ecb9f7d4d69..5429a860048 100644 --- a/app/models/profile.rb +++ b/app/models/profile.rb @@ -5,7 +5,6 @@ class Profile < ApplicationRecord has_many :usps_confirmation_codes, dependent: :destroy validates :active, uniqueness: { scope: :user_id, if: :active? } - validates :ssn_signature, uniqueness: { scope: :active, if: :active? } scope(:active, -> { where(active: true) }) scope(:verified, -> { where.not(verified_at: nil) }) diff --git a/app/services/idv/profile_step.rb b/app/services/idv/profile_step.rb index 606a7993bb6..96bda78fc7d 100644 --- a/app/services/idv/profile_step.rb +++ b/app/services/idv/profile_step.rb @@ -44,16 +44,7 @@ def idv_throttle_params end def success? - idv_result[:success] && ssn_is_unique? - end - - def ssn_is_unique? - ssn = applicant[:ssn] - return false if ssn.nil? - - @ssn_is_unique ||= DuplicateSsnFinder.new( - ssn: ssn, user: idv_session.current_user, - ).ssn_is_unique? + idv_result[:success] end def failed_due_to_timeout_or_exception? @@ -70,7 +61,6 @@ def extra_analytics_attributes { idv_attempts_exceeded: throttled?, vendor: idv_result.except(:errors, :success), - ssn_is_unique: ssn_is_unique?, } end end diff --git a/app/validators/idv/form_ssn_validator.rb b/app/validators/idv/form_ssn_validator.rb deleted file mode 100644 index 60ae58b1a84..00000000000 --- a/app/validators/idv/form_ssn_validator.rb +++ /dev/null @@ -1,41 +0,0 @@ -module Idv - module FormSsnValidator - extend ActiveSupport::Concern - - included do - validates :ssn, presence: true - validate :ssn_is_unique - validates_format_of :ssn, - with: /\A\d{3}-?\d{2}-?\d{4}\z/, - message: I18n.t('idv.errors.pattern_mismatch.ssn'), - allow_blank: false - end - - def duplicate_ssn? - return true if any_matching_ssn_signatures?(ssn_signature) || - ssn_is_duplicate_with_old_key? - false - end - - private - - def ssn_signature(key = Pii::Fingerprinter.current_key) - Pii::Fingerprinter.fingerprint(ssn, key) if ssn - end - - def ssn_is_unique - errors.add :ssn, I18n.t('idv.errors.duplicate_ssn') if duplicate_ssn? - end - - def ssn_is_duplicate_with_old_key? - signatures = KeyRotator::Utils.old_keys(:hmac_fingerprinter_key_queue).map do |key| - ssn_signature(key) - end - any_matching_ssn_signatures?(signatures) - end - - def any_matching_ssn_signatures?(signatures) - Profile.where.not(user_id: @user.id).where(ssn_signature: signatures).any? - end - end -end diff --git a/config/locales/idv/en.yml b/config/locales/idv/en.yml index 323d49c2514..3f6704266d9 100644 --- a/config/locales/idv/en.yml +++ b/config/locales/idv/en.yml @@ -18,7 +18,6 @@ en: warning_5: You can manage or delete that account on your profile page errors: bad_dob: Your date of birth must be a valid date. - duplicate_ssn: An account already exists with the information you provided. incorrect_password: The password you entered is not correct. mail_limit_reached: You have have requested too much mail in the last month. pattern_mismatch: diff --git a/config/locales/idv/es.yml b/config/locales/idv/es.yml index b13f954b9f3..6a49faa3998 100644 --- a/config/locales/idv/es.yml +++ b/config/locales/idv/es.yml @@ -19,7 +19,6 @@ es: warning_5: Puedes gestionar o eliminar esa cuenta desde tu página de perfil errors: bad_dob: Su fecha de nacimiento debe ser una fecha válida. - duplicate_ssn: Ya existe una cuenta con la información que proporcionó. incorrect_password: La contraseña que ingresó no es correcta. mail_limit_reached: Usted ha solicitado demasiado correo en el último mes. pattern_mismatch: diff --git a/config/locales/idv/fr.yml b/config/locales/idv/fr.yml index a2384a48f05..754cce3afb0 100644 --- a/config/locales/idv/fr.yml +++ b/config/locales/idv/fr.yml @@ -19,8 +19,6 @@ fr: warning_5: Vous pouvez gérer ou supprimer ce compte sur votre page de profil errors: bad_dob: Votre date de naissance doit être une date valide. - duplicate_ssn: Un compte existe déjà avec l'information que vous avez fournie. - chiffres. incorrect_password: Le mot de passe que vous avez inscrit est incorrect. mail_limit_reached: Vous avez demandé trop de lettres au cours du dernier mois. pattern_mismatch: diff --git a/db/migrate/20200321210321_drop_ssn_uniqueness_constraint.rb b/db/migrate/20200321210321_drop_ssn_uniqueness_constraint.rb new file mode 100644 index 00000000000..e3c00c3b302 --- /dev/null +++ b/db/migrate/20200321210321_drop_ssn_uniqueness_constraint.rb @@ -0,0 +1,27 @@ +class DropSsnUniquenessConstraint < ActiveRecord::Migration[5.1] + disable_ddl_transaction! + + def up + remove_index :profiles, %i[ssn_signature active] + remove_index :profiles, %i[user_id ssn_signature active] + end + + def down + add_index( + :profiles, + %i[ssn_signature active], + name: :index_profiles_on_ssn_signature_and_active, + unique: true, + where: '(active = true)', + algorithm: :concurrently, + ) + add_index( + :profiles, + %i[user_id ssn_signature active], + name: :index_profiles_on_user_id_and_ssn_signature_and_active, + unique: true, + where: '(active = true)', + algorithm: :concurrently, + ) + end +end diff --git a/db/schema.rb b/db/schema.rb index 679e8d1a25f..1ad86fdbb88 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: 20200319233723) do +ActiveRecord::Schema.define(version: 2020_03_21_210321) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" @@ -321,10 +321,8 @@ t.integer "deactivation_reason" t.boolean "phone_confirmed", default: false, null: false t.jsonb "proofing_components" - t.index ["ssn_signature", "active"], name: "index_profiles_on_ssn_signature_and_active", unique: true, where: "(active = true)" t.index ["ssn_signature"], name: "index_profiles_on_ssn_signature" t.index ["user_id", "active"], name: "index_profiles_on_user_id_and_active", unique: true, where: "(active = true)" - t.index ["user_id", "ssn_signature", "active"], name: "index_profiles_on_user_id_and_ssn_signature_and_active", unique: true, where: "(active = true)" t.index ["user_id"], name: "index_profiles_on_user_id" end diff --git a/spec/controllers/idv/sessions_controller_spec.rb b/spec/controllers/idv/sessions_controller_spec.rb index d740d34807d..248b54a3ea2 100644 --- a/spec/controllers/idv/sessions_controller_spec.rb +++ b/spec/controllers/idv/sessions_controller_spec.rb @@ -85,30 +85,6 @@ expect(subject.idv_session.applicant['uuid']).to eq subject.current_user.uuid end - it 'redirects to failure if the SSN exists' do - create(:profile, pii: { ssn: '666-66-1234' }) - - context = { stages: [{ resolution: 'ResolutionMock' }, { state_id: 'StateIdMock' }] } - result = { - success: false, - idv_attempts_exceeded: false, - errors: {}, - ssn_is_unique: false, - vendor: { messages: [], context: context, exception: nil, timed_out: false }, - } - - expect(@analytics).to receive(:track_event).ordered. - with(Analytics::IDV_BASIC_INFO_SUBMITTED_FORM, hash_including(success: true)) - expect(@analytics).to receive(:track_event).ordered. - with(Analytics::IDV_BASIC_INFO_SUBMITTED_VENDOR, result) - - post :create, params: { profile: user_attrs.merge(ssn: '666-66-1234') } - - expect(response).to redirect_to(idv_session_errors_warning_url) - expect(idv_session.profile_confirmation).to be_falsy - expect(idv_session.resolution_successful).to be_falsy - end - it 'renders the forms if there are missing fields' do partial_attrs = user_attrs.tap { |attrs| attrs.delete :first_name } @@ -139,7 +115,6 @@ errors: { first_name: ['Unverified first name.'], }, - ssn_is_unique: true, vendor: { messages: [], context: context, exception: nil, timed_out: false }, } @@ -161,7 +136,6 @@ success: true, idv_attempts_exceeded: false, errors: {}, - ssn_is_unique: true, vendor: { messages: [], context: context, exception: nil, timed_out: false }, } diff --git a/spec/features/idv/steps/profile_step_spec.rb b/spec/features/idv/steps/profile_step_spec.rb index 6f3cff1f1a8..ef76d534093 100644 --- a/spec/features/idv/steps/profile_step_spec.rb +++ b/spec/features/idv/steps/profile_step_spec.rb @@ -73,34 +73,6 @@ end end - context 'when an account exists with the same SSN' do - it 'renders a warning and locks the user out after 3 attempts' do - ssn = '123-45-6789' - create(:profile, ssn_signature: Pii::Fingerprinter.fingerprint(ssn)) - - start_idv_from_sp - complete_idv_steps_before_profile_step - - 2.times do - fill_out_idv_form_ok - fill_in :profile_ssn, with: ssn - click_continue - - expect(page).to have_content(t('idv.failure.sessions.warning')) - expect(page).to have_current_path(idv_session_errors_warning_path) - - click_on t('idv.failure.button.warning') - end - - fill_out_idv_form_ok - fill_in :profile_ssn, with: ssn - click_continue - - expect(page).to have_content(strip_tags(t('idv.failure.sessions.fail_html'))) - expect(page).to have_current_path(idv_session_errors_failure_path) - end - end - context 'cancelling IdV' do it_behaves_like 'cancel at idv step', :profile it_behaves_like 'cancel at idv step', :profile, :oidc diff --git a/spec/features/users/user_profile_spec.rb b/spec/features/users/user_profile_spec.rb index ad829b57e36..7bbb10b7f47 100644 --- a/spec/features/users/user_profile_spec.rb +++ b/spec/features/users/user_profile_spec.rb @@ -64,17 +64,6 @@ end end - it 'prevents a user from using the same credentials to sign up' do - pii = { ssn: '1234', dob: '1920-01-01' } - profile = create(:profile, :active, :verified, pii: pii) - sign_in_live_with_2fa(profile.user) - click_link(t('links.sign_out'), match: :first) - - expect do - create(:profile, :active, :verified, pii: pii) - end.to raise_error(ActiveRecord::RecordInvalid) - end - context 'ial2 user clicks the delete account button' do it 'deletes the account and signs the user out with a flash message' do profile = create(:profile, :active, :verified, pii: { ssn: '1234', dob: '1920-01-01' }) diff --git a/spec/forms/idv/ssn_form_spec.rb b/spec/forms/idv/ssn_form_spec.rb index 2736cb9841b..5ae101d403f 100644 --- a/spec/forms/idv/ssn_form_spec.rb +++ b/spec/forms/idv/ssn_form_spec.rb @@ -3,16 +3,26 @@ describe Idv::SsnForm do let(:user) { create(:user) } let(:subject) { Idv::SsnForm.new(user) } - let(:ssn) { '111-11-1111' } + let(:ssn) { '111111111' } describe '#submit' do context 'when the form is valid' do it 'returns a successful form response' do - result = subject.submit(ssn: '111111111') + result = subject.submit(ssn: ssn) expect(result).to be_kind_of(FormResponse) expect(result.success?).to eq(true) expect(result.errors).to be_empty + expect(result.extra).to eq(ssn_is_unique: true) + end + + context 'when the SSN is a duplicate' do + before { create(:profile, pii: { ssn: ssn }) } + + it 'logs that there is a duplicate SSN' do + result = subject.submit(ssn: ssn) + expect(result.extra).to eq(ssn_is_unique: false) + end end end @@ -28,7 +38,7 @@ context 'when the form has invalid attributes' do it 'raises an error' do - expect { subject.submit(ssn: '111111111', foo: 1) }. + expect { subject.submit(ssn: ssn, foo: 1) }. to raise_error(ArgumentError, 'foo is an invalid ssn attribute') end end @@ -41,48 +51,4 @@ expect(subject).to_not be_valid end end - - describe 'ssn uniqueness' do - context 'when ssn is already taken by another profile' do - it 'is invalid' do - diff_user = create(:user) - create(:profile, pii: { ssn: ssn }, user: diff_user) - - subject.submit(ssn: ssn) - - expect(subject.valid?).to eq false - expect(subject.errors[:ssn]).to eq [t('idv.errors.duplicate_ssn')] - end - - it 'recognizes fingerprint regardless of HMAC key age' do - diff_user = create(:user) - create(:profile, pii: { ssn: ssn }, user: diff_user) - rotate_hmac_key - - subject.submit(ssn: ssn) - - expect(subject.valid?).to eq false - expect(subject.errors[:ssn]).to eq [t('idv.errors.duplicate_ssn')] - end - end - - context 'when ssn is already taken by same profile' do - it 'is valid' do - create(:profile, pii: { ssn: ssn }, user: user) - - subject.submit(ssn: ssn) - - expect(subject.valid?).to eq true - end - - it 'recognizes fingerprint regardless of HMAC key age' do - create(:profile, pii: { ssn: ssn }, user: user) - rotate_hmac_key - - subject.submit(ssn: ssn) - - expect(subject.valid?).to eq true - end - end - end end diff --git a/spec/models/profile_spec.rb b/spec/models/profile_spec.rb index 14a3cea973b..52a79c8853e 100644 --- a/spec/models/profile_spec.rb +++ b/spec/models/profile_spec.rb @@ -104,38 +104,6 @@ end end - describe 'allows one unique SSN per user' do - it 'allows multiple records per user if only one is active' do - profile.active = true - pii_attrs = Pii::Attributes.new_from_hash(ssn: '1234') - profile.encrypt_pii(pii_attrs, user.password) - profile.save! - expect { create(:profile, pii: { ssn: '1234' }, user: user) }.to_not raise_error - end - - it 'prevents save! via ActiveRecord uniqueness validation' do - profile = Profile.new(active: true, user: user) - profile.encrypt_pii(pii, user.password) - profile.save! - expect do - another_profile = Profile.new(active: true, user: another_user) - another_profile.encrypt_pii(pii, user.password) - another_profile.save! - end.to raise_error(ActiveRecord::RecordInvalid) - end - - it 'prevents save! via psql unique partial index' do - profile = Profile.new(active: true, user: user) - profile.encrypt_pii(pii, user.password) - profile.save! - expect do - another_profile = Profile.new(active: true, user: another_user) - another_profile.encrypt_pii(pii, another_user.password) - another_profile.save!(validate: false) - end.to raise_error(ActiveRecord::RecordNotUnique) - end - end - describe '#activate' do it 'activates current Profile, de-activates all other Profile for the user' do active_profile = Profile.create(user: user, active: true) diff --git a/spec/services/idv/duplitcate_ssn_finder_spec.rb b/spec/services/idv/duplicate_ssn_finder_spec.rb similarity index 100% rename from spec/services/idv/duplitcate_ssn_finder_spec.rb rename to spec/services/idv/duplicate_ssn_finder_spec.rb diff --git a/spec/services/idv/profile_step_spec.rb b/spec/services/idv/profile_step_spec.rb index b384f5e2e36..7db5eabde99 100644 --- a/spec/services/idv/profile_step_spec.rb +++ b/spec/services/idv/profile_step_spec.rb @@ -30,7 +30,6 @@ extra = { idv_attempts_exceeded: false, vendor: { messages: [], context: context, exception: nil, timed_out: false }, - ssn_is_unique: true, } result = subject.submit(user_attrs) @@ -52,7 +51,6 @@ extra = { idv_attempts_exceeded: false, vendor: { messages: [], context: context, exception: nil, timed_out: false }, - ssn_is_unique: true, } result = subject.submit(user_attrs)