From 61c5e6685f87ee383941d4924ecabd1fab823513 Mon Sep 17 00:00:00 2001 From: Matt Hinz Date: Tue, 2 Jan 2024 12:23:58 -0800 Subject: [PATCH 1/7] LG-11883: Add validation to GpoConfirmation entries (#9823) * Add validation to GpoConfirmation model Validate that: - entry is present - entry contains all required fields - entry contains a valid zipcode * Ensure we throw a 500 in EnterPasswordController Since enforcement is with rails validation, this would be a 4XX error by default. Wrapping it in another error ensures it'll get passed back to the user as a 500 * Don't allow GpoConfirmationMaker to accept nil zipcode * Fix GpoMail spec Ensure we're passing now-required fields in. * Update GpoConfirmationMaker spec to use more examples * Don't mint a profile when GpoConfirmation write fails If we mint the profile, the user will see the "your letter is on the way" screen when they refresh. * Validate individual attributes Introduce phantom attributes on GpoConfirmation that we can validate with traditional means This should allow better error messages for logging purposes * Destroy IdV session on invalid gpo data Ideally we'd show an error and have the user confirm their address, but as a stopgap we can destroy the idv session and force the user to restart. * changelog: Bug Fixes, Identity verification, Prevent telling the user their letter is on the way if it actually isn't * Move zip code normalization into GpoConfirmation It will normalize its own zip code values. (This lets zip validation just piggy back on zip normalization) * Watch for invalid GPO confirmations when uploading - Validate confirmations before sending to GPO - Notify NewRelic if there are invalid confirmations present * Add invalid_gpo_confirmation_zipcode config Allow configuring a certain zipcode to cause GPO confirmation validation to blow up * Don't bother killing IdvSession on error I think the session is not committed due to the 500 error, so we need to take a different path that doesn't depend on saving state in session * Don't use method_missing * Add a couple more zip code examples --- app/models/gpo_confirmation.rb | 48 +++++++- app/services/gpo_confirmation_maker.rb | 25 ++-- app/services/gpo_confirmation_uploader.rb | 20 ++- app/services/idv/session.rb | 24 ++-- config/application.yml.default | 2 + lib/identity_config.rb | 1 + .../idv/enter_password_controller_spec.rb | 19 +++ spec/services/gpo_confirmation_maker_spec.rb | 62 ++++++---- spec/services/gpo_confirmation_spec.rb | 114 +++++++++++++++++- .../gpo_confirmation_uploader_spec.rb | 50 ++++++++ spec/services/idv/gpo_mail_spec.rb | 2 +- 11 files changed, 315 insertions(+), 52 deletions(-) diff --git a/app/models/gpo_confirmation.rb b/app/models/gpo_confirmation.rb index 63dc8aa8105..8fa29039aaf 100644 --- a/app/models/gpo_confirmation.rb +++ b/app/models/gpo_confirmation.rb @@ -1,14 +1,44 @@ class GpoConfirmation < ApplicationRecord self.table_name = 'usps_confirmations' + ENTRY_ATTRIBUTES = %i[otp address1 city state zipcode] + ENTRY_ATTRIBUTES.each do |attr| + define_method("entry_#{attr}".to_sym) do + entry[attr] + end + end + + validates :entry, presence: true + validates :entry_otp, :entry_address1, :entry_city, :entry_state, :entry_zipcode, presence: true + validate :entry_zipcode_is_valid + + ZIP_REGEX = /^(\d{5})[-+]?(\d+)?$/ + # Store the pii as encrypted json def entry=(entry_hash) - self[:entry] = encryptor.encrypt(entry_hash.to_json) + @entry = nil + self[:entry] = encryptor.encrypt( + entry_hash. + dup. + tap do |h| + h[:zipcode] = self.class.normalize_zipcode(h[:zipcode]) if h[:zipcode].present? + end. + to_json, + ) end # Read the pii as a decrypted hash def entry - JSON.parse(encryptor.decrypt(self[:entry]), symbolize_names: true) + @entry ||= JSON.parse(encryptor.decrypt(self[:entry]), symbolize_names: true) + end + + def self.normalize_zipcode(zipcode) + _, zip, plus4 = ZIP_REGEX.match(zipcode&.gsub(/\s/, '')).to_a + if plus4&.length == 4 + "#{zip}-#{plus4}" + else + zip + end end private @@ -16,4 +46,18 @@ def entry def encryptor Encryption::Encryptors::BackgroundProofingArgEncryptor.new end + + def entry_zipcode_should_be_rejected_because_we_are_testing? + # We reserve a certain zipcode to be used to test this value in lower environments. + entry_zipcode.present? && + entry_zipcode == IdentityConfig.store.invalid_gpo_confirmation_zipcode + end + + def entry_zipcode_is_valid + normalized = self.class.normalize_zipcode(entry_zipcode) + + if normalized.nil? || entry_zipcode_should_be_rejected_because_we_are_testing? + errors.add(:entry_zipcode, :invalid) + end + end end diff --git a/app/services/gpo_confirmation_maker.rb b/app/services/gpo_confirmation_maker.rb index 8ce56d1dbf8..be87a77705b 100644 --- a/app/services/gpo_confirmation_maker.rb +++ b/app/services/gpo_confirmation_maker.rb @@ -1,4 +1,12 @@ class GpoConfirmationMaker + class InvalidEntryError < StandardError + def initialize(reason) + @reason = reason + super("InvalidEntryError: #{reason}") + end + attr_reader :reason + end + def initialize(pii:, service_provider:, profile: nil, profile_id: nil, otp: nil) raise ArgumentError 'must have either profile or profile_id' if !profile && !profile_id @@ -14,7 +22,12 @@ def otp end def perform - GpoConfirmation.create!(entry: attributes) + begin + GpoConfirmation.create!(entry: attributes) + rescue ActiveRecord::RecordInvalid => err + raise InvalidEntryError.new(err) + end + GpoConfirmationCode.create!( profile_id: profile&.id || profile_id, otp_fingerprint: Pii::Fingerprinter.fingerprint(otp), @@ -36,7 +49,7 @@ def attributes first_name: pii[:first_name], last_name: pii[:last_name], state: pii[:state], - zipcode: force_zipcode_format(pii[:zipcode]), + zipcode: pii[:zipcode], issuer: service_provider&.issuer, } end @@ -51,12 +64,4 @@ def generate_otp def update_proofing_cost Db::SpCost::AddSpCost.call(service_provider, 2, :gpo_letter) end - - def force_zipcode_format(raw_zipcode) - return raw_zipcode if raw_zipcode.nil? - return raw_zipcode if raw_zipcode.match?(/^\d{5}$/) - return raw_zipcode if raw_zipcode.match?(/^\d{5}-\d{4}$/) - - raw_zipcode[0..4] - end end diff --git a/app/services/gpo_confirmation_uploader.rb b/app/services/gpo_confirmation_uploader.rb index b3cda1ebbf2..e3c682da106 100644 --- a/app/services/gpo_confirmation_uploader.rb +++ b/app/services/gpo_confirmation_uploader.rb @@ -1,10 +1,11 @@ class GpoConfirmationUploader + class InvalidGpoConfirmationsPresent < StandardError; end + def initialize(now = Time.zone.now) @now = now end def run - confirmations = GpoConfirmation.all.to_a export = generate_export(confirmations) upload_export(export) LetterRequestsToGpoFtpLog.create(ftp_at: @now, letter_requests_count: confirmations.count) @@ -16,6 +17,23 @@ def run private + def confirmations + return @confirmations if defined?(@confirmations) + + all_confirmations = GpoConfirmation.all.to_a + invalid_confirmations = all_confirmations.filter { |c| !c.valid? } + + if !invalid_confirmations.empty? + NewRelic::Agent.notice_error( + InvalidGpoConfirmationsPresent.new( + "Found #{invalid_confirmations.length} invalid GPO confirmations.", + ), + ) + end + + @confirmations = all_confirmations - invalid_confirmations + end + def generate_export(confirmations) GpoConfirmationExporter.new(confirmations).run end diff --git a/app/services/idv/session.rb b/app/services/idv/session.rb index ec9fc3380e9..8d5ef75fdda 100644 --- a/app/services/idv/session.rb +++ b/app/services/idv/session.rb @@ -78,7 +78,7 @@ def create_profile_from_applicant_with_password(user_password) associate_in_person_enrollment_with_profile if profile.in_person_verification_pending? if profile.gpo_verification_pending? - create_gpo_entry(profile_maker.pii_attributes) + create_gpo_entry(profile_maker.pii_attributes, profile) elsif profile.in_person_verification_pending? UspsInPersonProofing::EnrollmentHelper.schedule_in_person_enrollment( current_user, @@ -121,20 +121,28 @@ def profile def clear user_session.delete(:idv) + @profile = nil + @gpo_otp = nil end def associate_in_person_enrollment_with_profile current_user.establishing_in_person_enrollment.update(profile: profile) end - def create_gpo_entry(pii) - confirmation_maker = GpoConfirmationMaker.new( - pii: pii, service_provider: service_provider, - profile: profile - ) - confirmation_maker.perform + def create_gpo_entry(pii, profile) + begin + confirmation_maker = GpoConfirmationMaker.new( + pii: pii, service_provider: service_provider, + profile: profile + ) + confirmation_maker.perform - @gpo_otp = confirmation_maker.otp + @gpo_otp = confirmation_maker.otp + rescue + # We don't have what we need to actually generate a GPO letter. + profile.deactivate(:encryption_error) + raise + end end def phone_otp_sent? diff --git a/config/application.yml.default b/config/application.yml.default index 2b9d59b06f6..2ffdedae096 100644 --- a/config/application.yml.default +++ b/config/application.yml.default @@ -156,6 +156,7 @@ in_person_outage_emailed_by_date: 'November 1, 2024' in_person_send_proofing_notifications_enabled: false in_person_stop_expiring_enrollments: false include_slo_in_saml_metadata: false +invalid_gpo_confirmation_zipcode: '00001' logins_per_ip_track_only_mode: false # LexisNexis ##################################################### lexisnexis_base_url: https://www.example.com @@ -468,6 +469,7 @@ production: hmac_fingerprinter_key: hmac_fingerprinter_key_queue: '[]' idv_sp_required: true + invalid_gpo_confirmation_zipcode: '' lexisnexis_threatmetrix_mock_enabled: false logins_per_ip_limit: 20 logins_per_ip_period: 20 diff --git a/lib/identity_config.rb b/lib/identity_config.rb index 62b79b8fd54..88aab8e842f 100644 --- a/lib/identity_config.rb +++ b/lib/identity_config.rb @@ -266,6 +266,7 @@ def self.build_store(config_map) config.add(:in_person_send_proofing_notifications_enabled, type: :boolean) config.add(:in_person_stop_expiring_enrollments, type: :boolean) config.add(:include_slo_in_saml_metadata, type: :boolean) + config.add(:invalid_gpo_confirmation_zipcode, type: :string) config.add(:lexisnexis_account_id, type: :string) config.add(:lexisnexis_base_url, type: :string) config.add(:lexisnexis_hmac_auth_enabled, type: :boolean) diff --git a/spec/controllers/idv/enter_password_controller_spec.rb b/spec/controllers/idv/enter_password_controller_spec.rb index bbc9eafc80f..c61dd934811 100644 --- a/spec/controllers/idv/enter_password_controller_spec.rb +++ b/spec/controllers/idv/enter_password_controller_spec.rb @@ -882,6 +882,25 @@ def show expect(response).to redirect_to idv_letter_enqueued_url end + + context 'applicant data is corrupted' do + let(:applicant) do + Idp::Constants::MOCK_IDV_APPLICANT_WITH_PHONE.dup.tap do |a| + a.delete(:zipcode) + end + end + + before do + expect do + put :create, + params: { user: { password: ControllerHelper::VALID_PASSWORD } } + end.to raise_error(GpoConfirmationMaker::InvalidEntryError) + end + + it 'does not mint a GPO pending profile' do + expect(user.reload.gpo_verification_pending_profile).to be_nil + end + end end end end diff --git a/spec/services/gpo_confirmation_maker_spec.rb b/spec/services/gpo_confirmation_maker_spec.rb index 0823c881a5c..5e4c8d0e79d 100644 --- a/spec/services/gpo_confirmation_maker_spec.rb +++ b/spec/services/gpo_confirmation_maker_spec.rb @@ -28,7 +28,9 @@ end let(:profile) { create(:profile) } - subject { described_class.new(pii: pii, service_provider: service_provider, profile: profile) } + subject do + described_class.new(pii: pii, service_provider: service_provider, profile: profile) + end describe '#perform' do before do @@ -72,30 +74,40 @@ end end - context 'with a nil zipcode' do - let(:zipcode) { nil } - - describe '#perform' do - it 'accepts the zipcode' do - subject.perform - - gpo_confirmation = GpoConfirmation.first - entry_hash = gpo_confirmation.entry - expect(entry_hash[:zipcode]).to be_nil - end - end - end - - context 'with a (bogus) zip+1 zipcode' do - let(:zipcode) { '12345+0' } - - describe '#perform' do - it 'strips the +0 from the zipcode' do - subject.perform - - gpo_confirmation = GpoConfirmation.first - entry_hash = gpo_confirmation.entry - expect(entry_hash[:zipcode]).to eq '12345' + [ + [nil, false], + ['1234', false], + ['12345+0', '12345'], + ['12345 - 0', '12345'], + ['12345- 0', '12345'], + ['12345-0', '12345'], + ['12345-6', '12345'], + ['12345-67', '12345'], + ['12345-678', '12345'], + ['12345-6789', '12345-6789'], + ['12345-67890', '12345'], + ].each do |input, expected| + context "when zipcode = #{input.inspect}" do + let(:zipcode) { input } + describe '#perform' do + if expected + it 'accepts the zipcode' do + expect { subject.perform }.not_to raise_error + end + else + it 'raises an error' do + expect { subject.perform }.to raise_error(GpoConfirmationMaker::InvalidEntryError) + end + end + + if expected.is_a?(String) + it "formats the zipcode as #{expected.inspect}" do + subject.perform + gpo_confirmation = GpoConfirmation.first + entry_hash = gpo_confirmation.entry + expect(entry_hash[:zipcode]).to eq expected + end + end end end end diff --git a/spec/services/gpo_confirmation_spec.rb b/spec/services/gpo_confirmation_spec.rb index 53482a901a3..9b5f67e1fc7 100644 --- a/spec/services/gpo_confirmation_spec.rb +++ b/spec/services/gpo_confirmation_spec.rb @@ -1,16 +1,27 @@ require 'rails_helper' RSpec.describe GpoConfirmation do - let(:attributes) do + let(:valid_attributes) do { - first_name: 'Homer', - last_name: 'Simpson', - ssn: '123-456-7890', + address1: '1234 Imaginary Ave', + address2: '', + city: 'Anywhere', + otp: 'ABCD1234', + first_name: 'Pat', + last_name: 'Person', + state: 'OH', + zipcode: '56789', + issuer: nil, } end + + let(:attributes) { valid_attributes } + let(:encryptor) { Encryption::Encryptors::BackgroundProofingArgEncryptor.new } - subject { GpoConfirmation.create!(entry: attributes) } + subject do + GpoConfirmation.create!(entry: attributes) + end describe '#entry' do it 'stores the entry as an encrypted json string' do @@ -25,6 +36,99 @@ it 'retrieves the entry as an unencrypted hash with symbolized keys' do expect(subject.entry).to eq(attributes) end + + describe 'validation' do + it 'passes when valid' do + expect { subject }.not_to raise_error + end + + %i[otp address1 city state zipcode].each do |prop| + context "#{prop} not present" do + let(:attributes) { valid_attributes.tap { |a| a.delete(prop) } } + it 'fails validation' do + expect { subject }.to raise_error(ActiveRecord::RecordInvalid) + end + end + + context "#{prop} all whitespace" do + let(:attributes) { valid_attributes.tap { |a| a[prop] = "\n\t\t " } } + it 'fails validation' do + expect { subject }.to raise_error(ActiveRecord::RecordInvalid) + end + end + end + + describe 'invalid_gpo_confirmation_zipcode' do + let(:invalid_gpo_confirmation_zipcode) { '00001' } + let(:attributes) do + valid_attributes.dup.tap { |a| a[:zipcode] = '00001' } + end + before do + allow(IdentityConfig.store).to receive(:invalid_gpo_confirmation_zipcode). + and_return(invalid_gpo_confirmation_zipcode) + end + + it 'does not validate' do + expect { subject }.to raise_error + end + + context 'when option is not set' do + let(:invalid_gpo_confirmation_zipcode) { '' } + it 'validates' do + expect { subject }.not_to raise_error + end + end + end + + describe 'zipcode' do + [ + ['0184', false], + ['982251', true], + [' 98225 ', true], + [' 98225 - 3938 ', true], + [' 98225 - 393 ', true], + ['98225-3938', true], + ['982253938', true], + ['1234', false], + ].each do |input, should_pass| + context input.inspect do + let(:attributes) { valid_attributes.dup.tap { |a| a[:zipcode] = input } } + if should_pass + it 'validates' do + expect { subject }.not_to raise_error + end + else + it 'does not validate' do + expect { subject }.to raise_error(ActiveRecord::RecordInvalid) + end + end + end + end + end + end + end + + describe '#normalize_zipcode' do + [ + [nil, nil], + ['', nil], + [" \t\t\t\r\n ", nil], + ['98225', '98225'], + [' 98225 ', '98225'], + ['1234', nil], + ['12345-0', '12345'], + ['12345-6', '12345'], + ['12345-67', '12345'], + ['12345-678', '12345'], + ['12345-6789', '12345-6789'], + ['12345 6789', '12345-6789'], + ['123456789', '12345-6789'], + ['1234567890', '12345'], + ].each do |input, expected| + it "normalizes #{input.inspect} to #{expected.inspect}" do + expect(GpoConfirmation.normalize_zipcode(input)).to eql(expected) + end + end end def parse(json) diff --git a/spec/services/gpo_confirmation_uploader_spec.rb b/spec/services/gpo_confirmation_uploader_spec.rb index 294311ac9e9..4612d1882fc 100644 --- a/spec/services/gpo_confirmation_uploader_spec.rb +++ b/spec/services/gpo_confirmation_uploader_spec.rb @@ -105,6 +105,56 @@ expect(GpoConfirmation.count).to eq 1 end end + + context 'when an a confirmation is not valid' do + let!(:valid_confirmation) do + GpoConfirmation.create!( + entry: { + first_name: 'John', + last_name: 'Johnson', + address1: '123 Sesame St', + address2: '', + city: 'Anytown', + state: 'WA', + zipcode: '98021', + otp: 'ZYX987', + issuer: '', + }, + ) + end + let!(:invalid_confirmation) do + confirmation = GpoConfirmation.new + confirmation.entry = { + first_name: 'John', + last_name: 'Johnson', + address1: '123 Sesame St', + address2: '', + city: 'Anytown', + state: 'WA', + zipcode: nil, + otp: 'ZYX654', + issuer: '', + } + confirmation.save(validate: false) + end + + it 'excludes the invalid confirmation from the set to be considered' do + expect(uploader).to receive(:generate_export).with([valid_confirmation]).and_return(export) + expect(uploader).to receive(:upload_export).with(export) + expect(uploader).to receive(:clear_confirmations).with([valid_confirmation]) + subject + end + + it 'tells New Relic that there are invalid records' do + expect(NewRelic::Agent).to receive(:notice_error). + with(GpoConfirmationUploader::InvalidGpoConfirmationsPresent) + + expect(uploader).to receive(:generate_export).with([valid_confirmation]).and_return(export) + expect(uploader).to receive(:upload_export).with(export) + expect(uploader).to receive(:clear_confirmations).with([valid_confirmation]) + subject + end + end end def sftp_options diff --git a/spec/services/idv/gpo_mail_spec.rb b/spec/services/idv/gpo_mail_spec.rb index 56d06e754e6..97cbb7ecb0b 100644 --- a/spec/services/idv/gpo_mail_spec.rb +++ b/spec/services/idv/gpo_mail_spec.rb @@ -93,7 +93,7 @@ def enqueue_gpo_letter_for(user, at_time: Time.zone.now) ) GpoConfirmationMaker.new( - pii: {}, + pii: Idp::Constants::MOCK_IDV_APPLICANT, service_provider: nil, profile: profile, ).perform From 3058a3b353815102986d29984d29c527dc16ceac Mon Sep 17 00:00:00 2001 From: Mitchell Henke Date: Wed, 3 Jan 2024 10:41:38 -0600 Subject: [PATCH 2/7] Add addtional bounce information to email-deliveries script (#9843) changelog: Internal, Scripts, Add addtional bounce information to email-deliveries script --- bin/oncall/email-deliveries | 8 ++++---- spec/bin/oncall/email-deliveries_spec.rb | 5 +++-- 2 files changed, 7 insertions(+), 6 deletions(-) diff --git a/bin/oncall/email-deliveries b/bin/oncall/email-deliveries index e7a9801af5c..432286279c0 100755 --- a/bin/oncall/email-deliveries +++ b/bin/oncall/email-deliveries @@ -113,10 +113,10 @@ class EmailDeliveries email_events = cloudwatch_client('/aws/lambda/SESAllEvents_Lambda').fetch( query: <<~EOS, fields - eventType AS event_type + eventType AS event_type, mail.commonHeaders.messageId as ses_message_id, + bounce.bounceType as bounce_type, bounce.bounceSubType as bounce_sub_type | filter #{message_id_filters} - | parse '"messageId": "*"' as ses_message_id - | display @timestamp, event_type, ses_message_id + | display @timestamp, event_type, ses_message_id, bounce_type, bounce_sub_type | limit 10000 EOS from: 1.week.ago, @@ -131,7 +131,7 @@ class EmailDeliveries email_action: events_by_message_id[message_id]['email_action'], timestamp: events_by_message_id[message_id]['@timestamp'], message_id: message_id, - events: events.sort_by { |e| e['@timestamp'] }.map { |e| e['event_type'] }, + events: events.sort_by { |e| e['@timestamp'] }.map { |e| [e['event_type'], e['bounce_type'], e['bounce_sub_type']].compact.join('-') }, ) end end diff --git a/spec/bin/oncall/email-deliveries_spec.rb b/spec/bin/oncall/email-deliveries_spec.rb index 3bee52b0643..136506978e4 100644 --- a/spec/bin/oncall/email-deliveries_spec.rb +++ b/spec/bin/oncall/email-deliveries_spec.rb @@ -68,7 +68,7 @@ { '@timestamp' => '2023-01-01 00:00:01', 'ses_message_id' => 'message-1', 'event_type' => 'Send' }, { '@timestamp' => '2023-01-01 00:00:02', 'ses_message_id' => 'message-1', 'event_type' => 'Delivery' }, { '@timestamp' => '2023-01-01 00:00:03', 'ses_message_id' => 'message-2', 'event_type' => 'Send' }, - { '@timestamp' => '2023-01-01 00:00:04', 'ses_message_id' => 'message-2', 'event_type' => 'Bounce' }, + { '@timestamp' => '2023-01-01 00:00:04', 'ses_message_id' => 'message-2', 'event_type' => 'Bounce', 'bounce_type' => 'Transient', 'bounce_sub_type' => 'MailboxFull' }, ] end # rubocop:enable Layout/LineLength @@ -82,7 +82,8 @@ [ ['user_id', 'timestamp', 'message_id', 'email_action', 'events'], ['abc123', '2023-01-01 00:00:01', 'message-1', 'forgot_password', 'Send, Delivery'], - ['def456', '2023-01-01 00:00:02', 'message-2', 'forgot_password', 'Send, Bounce'], + ['def456', '2023-01-01 00:00:02', 'message-2', 'forgot_password', + 'Send, Bounce-Transient-MailboxFull'], ], ) end From e60ff9fa2c0fbeac9f01016428c28e7048f8e8bf Mon Sep 17 00:00:00 2001 From: Alex Bradley Date: Wed, 3 Jan 2024 15:53:03 -0500 Subject: [PATCH 3/7] Update the PR template for easier JIRA ticket entry (#9847) [skip changelog] We always find ourselves copy and pasting the same JIRA link for the relevant ticket. Adding this to the PR template allows us to just replace the X's with the ticket number. --- .github/pull_request_template.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/.github/pull_request_template.md b/.github/pull_request_template.md index a441111fc46..613a0fb5e23 100644 --- a/.github/pull_request_template.md +++ b/.github/pull_request_template.md @@ -3,7 +3,8 @@