From cf8425f1d81a73d1d97ae009c1bcd3b16c9b9b21 Mon Sep 17 00:00:00 2001 From: Matt Wagner Date: Thu, 20 Jun 2024 16:24:05 -0400 Subject: [PATCH 01/10] LG-13421 | Adds GpoVerifyByMailPolicy This will become the source of truth for whether the user can make use of GPO's verify-by-mail functionality. changelog: Internal, GPO Verification, Establishes GpoVerifyByMailPolicy class --- app/policies/idv/gpo_verify_by_mail_policy.rb | 16 ++++ .../idv/gpo_verify_by_mail_policy_spec.rb | 81 +++++++++++++++++++ 2 files changed, 97 insertions(+) create mode 100644 app/policies/idv/gpo_verify_by_mail_policy.rb create mode 100644 spec/policies/idv/gpo_verify_by_mail_policy_spec.rb diff --git a/app/policies/idv/gpo_verify_by_mail_policy.rb b/app/policies/idv/gpo_verify_by_mail_policy.rb new file mode 100644 index 00000000000..d0a2c5fda64 --- /dev/null +++ b/app/policies/idv/gpo_verify_by_mail_policy.rb @@ -0,0 +1,16 @@ +# frozen_string_literal: true + +module Idv + class GpoVerifyByMailPolicy + def initialize(user) + @user = user + @gpo_mail = Idv::GpoMail.new(user) + end + + def gpo_available_for_user? + FeatureManagement.gpo_verification_enabled? && + !@gpo_mail.rate_limited? && + !@gpo_mail.profile_too_old? + end + end +end diff --git a/spec/policies/idv/gpo_verify_by_mail_policy_spec.rb b/spec/policies/idv/gpo_verify_by_mail_policy_spec.rb new file mode 100644 index 00000000000..7284d8478d8 --- /dev/null +++ b/spec/policies/idv/gpo_verify_by_mail_policy_spec.rb @@ -0,0 +1,81 @@ +# frozen_string_literal: true +require 'rails_helper' + +RSpec.describe Idv::GpoVerifyByMailPolicy do + describe '#gpo_available_for_user?' do + let(:subject) { described_class.new(user).gpo_available_for_user? } + let(:user) { create(:user) } + + context 'when the feature flag is off' do + before do + allow(IdentityConfig.store).to receive(:enable_usps_verification) + .and_return false + end + + it 'returns false' do + expect(subject).to eq false + end + end + + context 'when the feature flag is on' do + before do + allow(IdentityConfig.store).to receive(:enable_usps_verification) + .and_return true + end + + context 'when the user is rate limited' do + # copypasta + before do + enqueue_gpo_letter_for(user, at_time: 4.days.ago) + enqueue_gpo_letter_for(user, at_time: 3.days.ago) + enqueue_gpo_letter_for(user, at_time: 2.days.ago) + end + + it 'returns false' do + expect(subject).to eq false + end + end + + context 'when the user has a too-old profile' do + before do + create(:profile, :verify_by_mail_pending, user: user, created_at: 90.days.ago) + end + + it 'returns false' do + expect(subject).to eq(false) + end + end + + context 'when the user has a current profile and is not rate limited' do + before do + create(:profile, :verify_by_mail_pending, user: user) + end + + it 'returns true' do + expect(subject).to eq(true) + end + end + end + end + + # FIXME: Straight-up copied and pasted from GpoMailSpec + def enqueue_gpo_letter_for(user, at_time: Time.zone.now) + profile = create( + :profile, + user: user, + gpo_verification_pending_at: at_time, + ) + + GpoConfirmationMaker.new( + pii: Idp::Constants::MOCK_IDV_APPLICANT, + service_provider: nil, + profile: profile, + ).perform + + profile.gpo_confirmation_codes.last.update( + created_at: at_time, + updated_at: at_time, + ) + end + +end From 7960db0b5072fe56c5f811cd78f40a33a56e303d Mon Sep 17 00:00:00 2001 From: Matt Wagner Date: Thu, 20 Jun 2024 17:35:05 -0400 Subject: [PATCH 02/10] Use our methods in a few places --- app/controllers/idv/by_mail/enter_code_controller.rb | 7 ++----- app/controllers/idv/by_mail/request_letter_controller.rb | 2 ++ app/controllers/vendor_outage_controller.rb | 5 ++--- app/policies/idv/gpo_verify_by_mail_policy.rb | 8 +++++++- spec/policies/idv/gpo_verify_by_mail_policy_spec.rb | 4 ++-- 5 files changed, 15 insertions(+), 11 deletions(-) diff --git a/app/controllers/idv/by_mail/enter_code_controller.rb b/app/controllers/idv/by_mail/enter_code_controller.rb index 2f8eeaf1b86..c4c34d1ba2c 100644 --- a/app/controllers/idv/by_mail/enter_code_controller.rb +++ b/app/controllers/idv/by_mail/enter_code_controller.rb @@ -148,11 +148,8 @@ def user_did_not_receive_letter? def user_can_request_another_letter? return @user_can_request_another_letter if defined?(@user_can_request_another_letter) - gpo_mail = Idv::GpoMail.new(current_user) - @user_can_request_another_letter = - FeatureManagement.gpo_verification_enabled? && - !gpo_mail.rate_limited? && - !gpo_mail.profile_too_old? + policy = Idv::GpoVerifyByMailPolicy.new(current_user) + @user_can_request_another_letter = policy.resend_letter_available? end def last_date_letter_was_sent diff --git a/app/controllers/idv/by_mail/request_letter_controller.rb b/app/controllers/idv/by_mail/request_letter_controller.rb index 4ed84020b46..fcda5ed8416 100644 --- a/app/controllers/idv/by_mail/request_letter_controller.rb +++ b/app/controllers/idv/by_mail/request_letter_controller.rb @@ -87,6 +87,8 @@ def first_letter_requested_at current_user.gpo_verification_pending_profile&.gpo_verification_pending_at end + # So some of these things check constituent parts and take action. + # Does that kinda undermine the point of some of this? def confirm_mail_not_rate_limited redirect_to idv_enter_password_url if gpo_mail_service.rate_limited? end diff --git a/app/controllers/vendor_outage_controller.rb b/app/controllers/vendor_outage_controller.rb index 8593d74f648..2a8a75d0aae 100644 --- a/app/controllers/vendor_outage_controller.rb +++ b/app/controllers/vendor_outage_controller.rb @@ -16,8 +16,7 @@ def from_idv_phone? end def gpo_letter_available? - FeatureManagement.gpo_verification_enabled? && - current_user && - !Idv::GpoMail.new(current_user).rate_limited? + policy = Idv::GpoVerifyByMailPolicy.new(current_user) + policy.send_letter_available? end end diff --git a/app/policies/idv/gpo_verify_by_mail_policy.rb b/app/policies/idv/gpo_verify_by_mail_policy.rb index d0a2c5fda64..3e9b7e1cd10 100644 --- a/app/policies/idv/gpo_verify_by_mail_policy.rb +++ b/app/policies/idv/gpo_verify_by_mail_policy.rb @@ -7,10 +7,16 @@ def initialize(user) @gpo_mail = Idv::GpoMail.new(user) end - def gpo_available_for_user? + def resend_letter_available? FeatureManagement.gpo_verification_enabled? && !@gpo_mail.rate_limited? && !@gpo_mail.profile_too_old? end + + def send_letter_available? + FeatureManagement.gpo_verification_enabled? && + user && + !@gpo_mail.rate_limited? + end end end diff --git a/spec/policies/idv/gpo_verify_by_mail_policy_spec.rb b/spec/policies/idv/gpo_verify_by_mail_policy_spec.rb index 7284d8478d8..b4cdce1a546 100644 --- a/spec/policies/idv/gpo_verify_by_mail_policy_spec.rb +++ b/spec/policies/idv/gpo_verify_by_mail_policy_spec.rb @@ -2,8 +2,8 @@ require 'rails_helper' RSpec.describe Idv::GpoVerifyByMailPolicy do - describe '#gpo_available_for_user?' do - let(:subject) { described_class.new(user).gpo_available_for_user? } + describe '#resend_letter_available?' do + let(:subject) { described_class.new(user).resend_letter_available? } let(:user) { create(:user) } context 'when the feature flag is off' do From d9b21f7cf56fac361a449cba067c51e06977c2d2 Mon Sep 17 00:00:00 2001 From: Matt Wagner Date: Fri, 21 Jun 2024 13:22:34 -0400 Subject: [PATCH 03/10] Lotsa changes --- app/controllers/concerns/idv_step_concern.rb | 9 ++++++--- app/controllers/idv/phone_controller.rb | 4 ++-- app/controllers/idv/phone_errors_controller.rb | 4 ++-- app/controllers/vendor_outage_controller.rb | 1 + app/forms/gpo_verify_form.rb | 3 ++- app/policies/idv/gpo_verify_by_mail_policy.rb | 1 - app/services/idv/gpo_mail.rb | 1 + 7 files changed, 14 insertions(+), 9 deletions(-) diff --git a/app/controllers/concerns/idv_step_concern.rb b/app/controllers/concerns/idv_step_concern.rb index 3a31407fecd..82a08bace1d 100644 --- a/app/controllers/concerns/idv_step_concern.rb +++ b/app/controllers/concerns/idv_step_concern.rb @@ -50,9 +50,12 @@ def check_for_mail_only_outage end def redirect_for_mail_only - return redirect_to vendor_outage_url unless FeatureManagement.gpo_verification_enabled? - - redirect_to idv_mail_only_warning_url + policy = Idv::GpoVerifyByMailPolicy.new(current_user) + if policy.send_letter_available? + redirect_to idv_mail_only_warning_url + else + redirect_to vendor_outage_url + end end def pii_from_user diff --git a/app/controllers/idv/phone_controller.rb b/app/controllers/idv/phone_controller.rb index 1ba7c799603..b1348e61425 100644 --- a/app/controllers/idv/phone_controller.rb +++ b/app/controllers/idv/phone_controller.rb @@ -225,8 +225,8 @@ def formatted_previous_phone_step_params_phone def gpo_letter_available return @gpo_letter_available if defined?(@gpo_letter_available) - @gpo_letter_available ||= FeatureManagement.gpo_verification_enabled? && - !Idv::GpoMail.new(current_user).rate_limited? + policy = Idv::GpoVerifyByMailPolicy.new(current_user) + @gpo_letter_available ||= policy.send_letter_available? end # Migrated from otp_delivery_method_controller diff --git a/app/controllers/idv/phone_errors_controller.rb b/app/controllers/idv/phone_errors_controller.rb index 5f82ed7ff48..d510e711644 100644 --- a/app/controllers/idv/phone_errors_controller.rb +++ b/app/controllers/idv/phone_errors_controller.rb @@ -74,8 +74,8 @@ def track_event(type:) # rubocop:disable Naming/MemoizedInstanceVariableName def set_gpo_letter_available return @gpo_letter_available if defined?(@gpo_letter_available) - @gpo_letter_available ||= FeatureManagement.gpo_verification_enabled? && - !Idv::GpoMail.new(current_user).rate_limited? + policy = Idv::GpoVerifyByMailPolicy.new(current_user) + @gpo_letter_available ||= policy.send_letter_available? end # rubocop:enable Naming/MemoizedInstanceVariableName end diff --git a/app/controllers/vendor_outage_controller.rb b/app/controllers/vendor_outage_controller.rb index 2a8a75d0aae..76b22c2447f 100644 --- a/app/controllers/vendor_outage_controller.rb +++ b/app/controllers/vendor_outage_controller.rb @@ -16,6 +16,7 @@ def from_idv_phone? end def gpo_letter_available? + return false unless current_user policy = Idv::GpoVerifyByMailPolicy.new(current_user) policy.send_letter_available? end diff --git a/app/forms/gpo_verify_form.rb b/app/forms/gpo_verify_form.rb index 2253e49a2de..e9ca623efc9 100644 --- a/app/forms/gpo_verify_form.rb +++ b/app/forms/gpo_verify_form.rb @@ -127,6 +127,7 @@ def activate_profile end def user_can_request_another_letter? - !Idv::GpoMail.new(user).rate_limited? + policy = Idv::GpoVerifyByMailPolicy.new(user) + policy.resend_letter_available? end end diff --git a/app/policies/idv/gpo_verify_by_mail_policy.rb b/app/policies/idv/gpo_verify_by_mail_policy.rb index 3e9b7e1cd10..7cdaf1073b7 100644 --- a/app/policies/idv/gpo_verify_by_mail_policy.rb +++ b/app/policies/idv/gpo_verify_by_mail_policy.rb @@ -15,7 +15,6 @@ def resend_letter_available? def send_letter_available? FeatureManagement.gpo_verification_enabled? && - user && !@gpo_mail.rate_limited? end end diff --git a/app/services/idv/gpo_mail.rb b/app/services/idv/gpo_mail.rb index 1d14ca87fa8..fcc10d8bfbf 100644 --- a/app/services/idv/gpo_mail.rb +++ b/app/services/idv/gpo_mail.rb @@ -63,6 +63,7 @@ def last_letter_request_too_recent? ) > 0 end + # Maybe this goes in the GpoConfirmationCode class? def number_of_letter_requests_within(time_window, maximum:, for_profile: nil) profile_query_conditions = { user: current_user } profile_query_conditions[:id] = for_profile.id if for_profile From cc0be4d96ff7db110bcb8fa9b2e3378ca7e736e3 Mon Sep 17 00:00:00 2001 From: Matt Wagner Date: Fri, 21 Jun 2024 13:24:21 -0400 Subject: [PATCH 04/10] lintfix --- .../idv/gpo_verify_by_mail_policy_spec.rb | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/spec/policies/idv/gpo_verify_by_mail_policy_spec.rb b/spec/policies/idv/gpo_verify_by_mail_policy_spec.rb index b4cdce1a546..cba40377f07 100644 --- a/spec/policies/idv/gpo_verify_by_mail_policy_spec.rb +++ b/spec/policies/idv/gpo_verify_by_mail_policy_spec.rb @@ -1,4 +1,5 @@ # frozen_string_literal: true + require 'rails_helper' RSpec.describe Idv::GpoVerifyByMailPolicy do @@ -8,8 +9,8 @@ context 'when the feature flag is off' do before do - allow(IdentityConfig.store).to receive(:enable_usps_verification) - .and_return false + allow(IdentityConfig.store).to receive(:enable_usps_verification). + and_return false end it 'returns false' do @@ -19,8 +20,8 @@ context 'when the feature flag is on' do before do - allow(IdentityConfig.store).to receive(:enable_usps_verification) - .and_return true + allow(IdentityConfig.store).to receive(:enable_usps_verification). + and_return true end context 'when the user is rate limited' do @@ -64,18 +65,17 @@ def enqueue_gpo_letter_for(user, at_time: Time.zone.now) :profile, user: user, gpo_verification_pending_at: at_time, - ) + ) GpoConfirmationMaker.new( pii: Idp::Constants::MOCK_IDV_APPLICANT, service_provider: nil, profile: profile, - ).perform + ).perform profile.gpo_confirmation_codes.last.update( created_at: at_time, updated_at: at_time, - ) + ) end - end From e0dd0e4e0e84cc202520a9fbb928bf4565d13137 Mon Sep 17 00:00:00 2001 From: Matt Wagner Date: Fri, 21 Jun 2024 13:28:04 -0400 Subject: [PATCH 05/10] Use attr_reader, silly --- app/policies/idv/gpo_verify_by_mail_policy.rb | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/app/policies/idv/gpo_verify_by_mail_policy.rb b/app/policies/idv/gpo_verify_by_mail_policy.rb index 7cdaf1073b7..e04dbe8f671 100644 --- a/app/policies/idv/gpo_verify_by_mail_policy.rb +++ b/app/policies/idv/gpo_verify_by_mail_policy.rb @@ -2,6 +2,9 @@ module Idv class GpoVerifyByMailPolicy + + attr_reader :gpo_mail + def initialize(user) @user = user @gpo_mail = Idv::GpoMail.new(user) @@ -9,13 +12,13 @@ def initialize(user) def resend_letter_available? FeatureManagement.gpo_verification_enabled? && - !@gpo_mail.rate_limited? && - !@gpo_mail.profile_too_old? + !gpo_mail.rate_limited? && + !gpo_mail.profile_too_old? end def send_letter_available? FeatureManagement.gpo_verification_enabled? && - !@gpo_mail.rate_limited? + !gpo_mail.rate_limited? end end end From 3927ea14f2d27a174da694bc57e15dcec437dc3b Mon Sep 17 00:00:00 2001 From: Matt Wagner Date: Mon, 24 Jun 2024 16:24:12 -0400 Subject: [PATCH 06/10] lintfix --- app/policies/idv/gpo_verify_by_mail_policy.rb | 1 - 1 file changed, 1 deletion(-) diff --git a/app/policies/idv/gpo_verify_by_mail_policy.rb b/app/policies/idv/gpo_verify_by_mail_policy.rb index e04dbe8f671..b056c22337e 100644 --- a/app/policies/idv/gpo_verify_by_mail_policy.rb +++ b/app/policies/idv/gpo_verify_by_mail_policy.rb @@ -2,7 +2,6 @@ module Idv class GpoVerifyByMailPolicy - attr_reader :gpo_mail def initialize(user) From b3bf27a74586ace09cf98bdc8deca8c6e2808301 Mon Sep 17 00:00:00 2001 From: Matt Wagner Date: Wed, 26 Jun 2024 14:57:34 -0400 Subject: [PATCH 07/10] Hollows out GpoMail class --- .../concerns/rate_limit_concern.rb | 3 +- .../idv/by_mail/request_letter_controller.rb | 8 +- .../idv/by_mail/resend_letter_controller.rb | 15 +- app/policies/idv/gpo_verify_by_mail_policy.rb | 52 ++++++- app/services/idv/gpo_mail.rb | 39 ----- .../by_mail/request_letter_controller_spec.rb | 4 +- .../idv/gpo_verify_by_mail_policy_spec.rb | 147 +++++++++++++++--- spec/services/idv/gpo_mail_spec.rb | 68 -------- 8 files changed, 185 insertions(+), 151 deletions(-) diff --git a/app/controllers/concerns/rate_limit_concern.rb b/app/controllers/concerns/rate_limit_concern.rb index f2202b56b9b..db5ecd55aa4 100644 --- a/app/controllers/concerns/rate_limit_concern.rb +++ b/app/controllers/concerns/rate_limit_concern.rb @@ -29,7 +29,8 @@ def confirm_not_rate_limited_for_phone_address_verification private def confirm_not_rate_limited_for_phone_and_letter_address_verification - if idv_attempter_rate_limited?(:proof_address) && Idv::GpoMail.new(current_user).rate_limited? + gpo_policy = Idv::GpoVerifyByMailPolicy.new(current_user) + if idv_attempter_rate_limited?(:proof_address) && gpo_policy.rate_limited? rate_limit_redirect!(:proof_address) return true end diff --git a/app/controllers/idv/by_mail/request_letter_controller.rb b/app/controllers/idv/by_mail/request_letter_controller.rb index 189682018a1..eae902c80d4 100644 --- a/app/controllers/idv/by_mail/request_letter_controller.rb +++ b/app/controllers/idv/by_mail/request_letter_controller.rb @@ -39,8 +39,8 @@ def create end end - def gpo_mail_service - @gpo_mail_service ||= Idv::GpoMail.new(current_user) + def gpo_mail_policy + @gpo_mail_policy ||= Idv::GpoVerifyByMailPolicy.new(current_user) end def self.step_info @@ -59,7 +59,7 @@ def self.step_info private def confirm_profile_not_too_old - redirect_to idv_path if gpo_mail_service.profile_too_old? + redirect_to idv_path if gpo_mail_policy.profile_too_old? end def update_tracking @@ -96,7 +96,7 @@ def hours_since_first_letter(first_letter_requested_at) end def confirm_mail_not_rate_limited - redirect_to idv_enter_password_url if gpo_mail_service.rate_limited? + redirect_to idv_enter_password_url if gpo_mail_policy.rate_limited? end def resend_letter diff --git a/app/controllers/idv/by_mail/resend_letter_controller.rb b/app/controllers/idv/by_mail/resend_letter_controller.rb index 8b1bd002ef7..32ad95c0082 100644 --- a/app/controllers/idv/by_mail/resend_letter_controller.rb +++ b/app/controllers/idv/by_mail/resend_letter_controller.rb @@ -9,8 +9,7 @@ class ResendLetterController < ApplicationController before_action :confirm_two_factor_authenticated before_action :confirm_verification_needed - before_action :confirm_mail_not_rate_limited - before_action :confirm_profile_not_too_old + before_action :confirm_resend_letter_available def new analytics.idv_resend_letter_visited @@ -28,8 +27,8 @@ def create end end - def gpo_mail_service - @gpo_mail_service ||= Idv::GpoMail.new(current_user) + def gpo_mail_policy + @gpo_mail_policy ||= Idv::GpoVerifyByMailPolicy.new(current_user) end private @@ -39,12 +38,8 @@ def confirm_verification_needed redirect_to account_url end - def confirm_profile_not_too_old - redirect_to idv_verify_by_mail_enter_code_path if gpo_mail_service.profile_too_old? - end - - def confirm_mail_not_rate_limited - redirect_to idv_verify_by_mail_enter_code_path if gpo_mail_service.rate_limited? + def confirm_resend_letter_available + redirect_to idv_verify_by_mail_enter_code_path unless gpo_mail_policy.resend_letter_available? end def update_tracking diff --git a/app/policies/idv/gpo_verify_by_mail_policy.rb b/app/policies/idv/gpo_verify_by_mail_policy.rb index b056c22337e..4981d0398fe 100644 --- a/app/policies/idv/gpo_verify_by_mail_policy.rb +++ b/app/policies/idv/gpo_verify_by_mail_policy.rb @@ -2,22 +2,64 @@ module Idv class GpoVerifyByMailPolicy - attr_reader :gpo_mail + attr_reader :user def initialize(user) @user = user - @gpo_mail = Idv::GpoMail.new(user) end def resend_letter_available? FeatureManagement.gpo_verification_enabled? && - !gpo_mail.rate_limited? && - !gpo_mail.profile_too_old? + !rate_limited? && + !profile_too_old? end def send_letter_available? FeatureManagement.gpo_verification_enabled? && - !gpo_mail.rate_limited? + !rate_limited? end + def rate_limited? + too_many_letter_requests_within_window? || last_letter_request_too_recent? + end + + def profile_too_old? + return false if !user.pending_profile + + min_creation_date = IdentityConfig.store. + gpo_max_profile_age_to_send_letter_in_days.days.ago + + user.pending_profile.created_at < min_creation_date + end + + private + + def window_limit_enabled? + IdentityConfig.store.max_mail_events != 0 && + IdentityConfig.store.max_mail_events_window_in_days != 0 + end + + def last_not_too_recent_enabled? + IdentityConfig.store.minimum_wait_before_another_usps_letter_in_hours != 0 + end + + def too_many_letter_requests_within_window? + return false unless window_limit_enabled? + user.gpo_confirmation_codes.where( + created_at: IdentityConfig.store.max_mail_events_window_in_days.days.ago..Time.zone.now, + ).count >= IdentityConfig.store.max_mail_events + end + + def last_letter_request_too_recent? + return false unless last_not_too_recent_enabled? + return false unless user.gpo_verification_pending_profile? + + user.gpo_verification_pending_profile.gpo_confirmation_codes.exists?( + [ + 'created_at > ?', + IdentityConfig.store.minimum_wait_before_another_usps_letter_in_hours.hours.ago, + ], + ) + end + end end diff --git a/app/services/idv/gpo_mail.rb b/app/services/idv/gpo_mail.rb index 1d0359d8451..5201562a8f1 100644 --- a/app/services/idv/gpo_mail.rb +++ b/app/services/idv/gpo_mail.rb @@ -8,47 +8,8 @@ def initialize(current_user) @current_user = current_user end - def rate_limited? - too_many_letter_requests_within_window? || last_letter_request_too_recent? - end - - def profile_too_old? - return false if !current_user.pending_profile - - min_creation_date = IdentityConfig.store. - gpo_max_profile_age_to_send_letter_in_days.days.ago - - current_user.pending_profile.created_at < min_creation_date - end - - private - def window_limit_enabled? - IdentityConfig.store.max_mail_events != 0 && - IdentityConfig.store.max_mail_events_window_in_days != 0 - end - - def last_not_too_recent_enabled? - IdentityConfig.store.minimum_wait_before_another_usps_letter_in_hours != 0 - end - def too_many_letter_requests_within_window? - return false unless window_limit_enabled? - current_user.gpo_confirmation_codes.where( - created_at: IdentityConfig.store.max_mail_events_window_in_days.days.ago..Time.zone.now, - ).count >= IdentityConfig.store.max_mail_events - end - - def last_letter_request_too_recent? - return false unless last_not_too_recent_enabled? - return false unless current_user.gpo_verification_pending_profile? - current_user.gpo_verification_pending_profile.gpo_confirmation_codes.exists?( - [ - 'created_at > ?', - IdentityConfig.store.minimum_wait_before_another_usps_letter_in_hours.hours.ago, - ], - ) - end end end diff --git a/spec/controllers/idv/by_mail/request_letter_controller_spec.rb b/spec/controllers/idv/by_mail/request_letter_controller_spec.rb index 1d24cc311a0..8a5b0170dc8 100644 --- a/spec/controllers/idv/by_mail/request_letter_controller_spec.rb +++ b/spec/controllers/idv/by_mail/request_letter_controller_spec.rb @@ -60,7 +60,7 @@ end it 'redirects if the user has sent too much mail' do - allow(controller.gpo_mail_service).to receive(:rate_limited?).and_return(true) + allow(controller.gpo_mail_policy).to receive(:rate_limited?).and_return(true) allow(subject.idv_session).to receive(:address_mechanism_chosen?). and_return(true) get :index @@ -69,7 +69,7 @@ end it 'allows a user to request another letter' do - allow(controller.gpo_mail_service).to receive(:rate_limited?).and_return(false) + allow(controller.gpo_mail_policy).to receive(:rate_limited?).and_return(false) get :index expect(response).to be_ok diff --git a/spec/policies/idv/gpo_verify_by_mail_policy_spec.rb b/spec/policies/idv/gpo_verify_by_mail_policy_spec.rb index cba40377f07..012d4330bc5 100644 --- a/spec/policies/idv/gpo_verify_by_mail_policy_spec.rb +++ b/spec/policies/idv/gpo_verify_by_mail_policy_spec.rb @@ -3,10 +3,52 @@ require 'rails_helper' RSpec.describe Idv::GpoVerifyByMailPolicy do + + let(:subject) { described_class.new(user) } + let(:user) { create(:user) } + + describe '#resend_letter_available?' do - let(:subject) { described_class.new(user).resend_letter_available? } - let(:user) { create(:user) } + context 'when the feature flag is off' do + before do + allow(IdentityConfig.store).to receive(:enable_usps_verification). + and_return false + end + + it 'returns false' do + expect(subject.resend_letter_available?).to eq false + end + end + + context 'when the feature flag is on' do + before do + allow(IdentityConfig.store).to receive(:enable_usps_verification). + and_return true + end + + it 'returns false when the user is rate-limited' do + enqueue_gpo_letter_for(user, at_time: 4.days.ago) + enqueue_gpo_letter_for(user, at_time: 3.days.ago) + enqueue_gpo_letter_for(user, at_time: 2.days.ago) + + expect(subject.resend_letter_available?).to eq false + end + + it 'returns false when the profile is too old' do + create(:profile, :verify_by_mail_pending, user: user, created_at: 90.days.ago) + + expect(subject.resend_letter_available?).to eq false + end + it 'returns true if not rate-limited and the profile is current' do + create(:profile, :verify_by_mail_pending, user: user) + + expect(subject.resend_letter_available?).to eq true + end + end + end + + describe '#send_letter_available?' do context 'when the feature flag is off' do before do allow(IdentityConfig.store).to receive(:enable_usps_verification). @@ -14,7 +56,7 @@ end it 'returns false' do - expect(subject).to eq false + expect(subject.send_letter_available?).to eq false end end @@ -24,42 +66,103 @@ and_return true end - context 'when the user is rate limited' do - # copypasta - before do - enqueue_gpo_letter_for(user, at_time: 4.days.ago) - enqueue_gpo_letter_for(user, at_time: 3.days.ago) - enqueue_gpo_letter_for(user, at_time: 2.days.ago) - end + it 'returns false when the user is rate-limited' do + enqueue_gpo_letter_for(user, at_time: 4.days.ago) + enqueue_gpo_letter_for(user, at_time: 3.days.ago) + enqueue_gpo_letter_for(user, at_time: 2.days.ago) - it 'returns false' do - expect(subject).to eq false + expect(subject.send_letter_available?).to eq false + end + + it 'returns true even if the profile is too old' do + create(:profile, :verify_by_mail_pending, user: user, created_at: 90.days.ago) + expect(subject.send_letter_available?).to eq true + end + end + end + + describe '#rate_limited?' do + let(:max_letter_request_events) { 2 } + let(:letter_request_events_window_days) { 30 } + let(:minimum_wait_before_another_usps_letter_in_hours) { 24 } + + before do + allow(IdentityConfig.store).to receive(:max_mail_events). + and_return(max_letter_request_events) + allow(IdentityConfig.store).to receive(:max_mail_events_window_in_days). + and_return(letter_request_events_window_days) + allow(IdentityConfig.store).to receive(:minimum_wait_before_another_usps_letter_in_hours). + and_return(minimum_wait_before_another_usps_letter_in_hours) + end + + context 'when no letters have been requested' do + it 'returns false' do + expect(subject.rate_limited?).to eq false + end + end + + context 'when too many letters have been requested within the limiting window' do + before do + enqueue_gpo_letter_for(user, at_time: 4.days.ago) + enqueue_gpo_letter_for(user, at_time: 3.days.ago) + enqueue_gpo_letter_for(user, at_time: 2.days.ago) + end + + it 'is true' do + expect(subject.rate_limited?).to eq true + end + + context 'but the window limit is disabled due to a 0 window size' do + let(:letter_request_events_window_days) { 0 } + + it 'is false' do + expect(subject.rate_limited?).to eq false end end - context 'when the user has a too-old profile' do - before do - create(:profile, :verify_by_mail_pending, user: user, created_at: 90.days.ago) + context 'but the window limit is disabled due to a 0 window count' do + let(:max_letter_request_events) { 0 } + + it 'is false' do + expect(subject.rate_limited?).to eq false end + end + end - it 'returns false' do - expect(subject).to eq(false) + context 'when a letter has been requested too recently' do + before do + enqueue_gpo_letter_for(user) + end + + it 'is true' do + expect(subject.rate_limited?).to eq true + end + + context 'but the too-recent limit is disabled' do + let(:minimum_wait_before_another_usps_letter_in_hours) { 0 } + + it 'is false' do + expect(subject.rate_limited?).to eq false end end - context 'when the user has a current profile and is not rate limited' do + context 'but the letter is not attached to their pending profile' do + # This can happen if the user resets their password while a GPO + # letter is pending. + before do - create(:profile, :verify_by_mail_pending, user: user) + user.gpo_verification_pending_profile.update( + gpo_verification_pending_at: nil, + ) end - it 'returns true' do - expect(subject).to eq(true) + it 'returns false' do + expect(subject.rate_limited?).to be false end end end end - # FIXME: Straight-up copied and pasted from GpoMailSpec def enqueue_gpo_letter_for(user, at_time: Time.zone.now) profile = create( :profile, diff --git a/spec/services/idv/gpo_mail_spec.rb b/spec/services/idv/gpo_mail_spec.rb index 97cbb7ecb0b..900b7dbe079 100644 --- a/spec/services/idv/gpo_mail_spec.rb +++ b/spec/services/idv/gpo_mail_spec.rb @@ -16,74 +16,6 @@ and_return(minimum_wait_before_another_usps_letter_in_hours) end - describe '#rate_limited?' do - context 'when no letters have been requested' do - it 'returns false' do - expect(subject.rate_limited?).to eq false - end - end - - context 'when too many letters have been requested within the limiting window' do - before do - enqueue_gpo_letter_for(user, at_time: 4.days.ago) - enqueue_gpo_letter_for(user, at_time: 3.days.ago) - enqueue_gpo_letter_for(user, at_time: 2.days.ago) - end - - it 'is true' do - expect(subject.rate_limited?).to eq true - end - - context 'but the window limit is disabled due to a 0 window size' do - let(:letter_request_events_window_days) { 0 } - - it 'is false' do - expect(subject.rate_limited?).to eq false - end - end - - context 'but the window limit is disabled due to a 0 window count' do - let(:max_letter_request_events) { 0 } - - it 'is false' do - expect(subject.rate_limited?).to eq false - end - end - end - - context 'when a letter has been requested too recently' do - before do - enqueue_gpo_letter_for(user) - end - - it 'is true' do - expect(subject.rate_limited?).to eq true - end - - context 'but the too-recent limit is disabled' do - let(:minimum_wait_before_another_usps_letter_in_hours) { 0 } - - it 'is false' do - expect(subject.rate_limited?).to eq false - end - end - - context 'but the letter is not attached to their pending profile' do - # This can happen if the user resets their password while a GPO - # letter is pending. - - before do - user.gpo_verification_pending_profile.update( - gpo_verification_pending_at: nil, - ) - end - - it 'returns false' do - expect(subject.rate_limited?).to be false - end - end - end - end def enqueue_gpo_letter_for(user, at_time: Time.zone.now) profile = create( From 84a9b055cd68d504ac3ce609f8989a539658c548 Mon Sep 17 00:00:00 2001 From: Matt Wagner Date: Wed, 26 Jun 2024 14:58:03 -0400 Subject: [PATCH 08/10] Removes GpoMail and GpoMailSpec --- app/services/idv/gpo_mail.rb | 15 ------------ spec/services/idv/gpo_mail_spec.rb | 38 ------------------------------ 2 files changed, 53 deletions(-) delete mode 100644 app/services/idv/gpo_mail.rb delete mode 100644 spec/services/idv/gpo_mail_spec.rb diff --git a/app/services/idv/gpo_mail.rb b/app/services/idv/gpo_mail.rb deleted file mode 100644 index 5201562a8f1..00000000000 --- a/app/services/idv/gpo_mail.rb +++ /dev/null @@ -1,15 +0,0 @@ -# frozen_string_literal: true - -module Idv - class GpoMail - attr_reader :current_user - - def initialize(current_user) - @current_user = current_user - end - - - - - end -end diff --git a/spec/services/idv/gpo_mail_spec.rb b/spec/services/idv/gpo_mail_spec.rb deleted file mode 100644 index 900b7dbe079..00000000000 --- a/spec/services/idv/gpo_mail_spec.rb +++ /dev/null @@ -1,38 +0,0 @@ -require 'rails_helper' - -RSpec.describe Idv::GpoMail do - let(:user) { create(:user) } - let(:subject) { Idv::GpoMail.new(user) } - let(:max_letter_request_events) { 2 } - let(:letter_request_events_window_days) { 30 } - let(:minimum_wait_before_another_usps_letter_in_hours) { 24 } - - before do - allow(IdentityConfig.store).to receive(:max_mail_events). - and_return(max_letter_request_events) - allow(IdentityConfig.store).to receive(:max_mail_events_window_in_days). - and_return(letter_request_events_window_days) - allow(IdentityConfig.store).to receive(:minimum_wait_before_another_usps_letter_in_hours). - and_return(minimum_wait_before_another_usps_letter_in_hours) - end - - - def enqueue_gpo_letter_for(user, at_time: Time.zone.now) - profile = create( - :profile, - user: user, - gpo_verification_pending_at: at_time, - ) - - GpoConfirmationMaker.new( - pii: Idp::Constants::MOCK_IDV_APPLICANT, - service_provider: nil, - profile: profile, - ).perform - - profile.gpo_confirmation_codes.last.update( - created_at: at_time, - updated_at: at_time, - ) - end -end From a04a88c8f68ee363914cecc2046f5e36781d2d70 Mon Sep 17 00:00:00 2001 From: Matt Wagner Date: Wed, 26 Jun 2024 15:19:06 -0400 Subject: [PATCH 09/10] lintfixes Some day I am going to run rubocop _before_ pushing changes... --- app/controllers/idv/by_mail/resend_letter_controller.rb | 4 +++- app/policies/idv/gpo_verify_by_mail_policy.rb | 6 +++--- spec/policies/idv/gpo_verify_by_mail_policy_spec.rb | 4 +--- 3 files changed, 7 insertions(+), 7 deletions(-) diff --git a/app/controllers/idv/by_mail/resend_letter_controller.rb b/app/controllers/idv/by_mail/resend_letter_controller.rb index 32ad95c0082..86efadac256 100644 --- a/app/controllers/idv/by_mail/resend_letter_controller.rb +++ b/app/controllers/idv/by_mail/resend_letter_controller.rb @@ -39,7 +39,9 @@ def confirm_verification_needed end def confirm_resend_letter_available - redirect_to idv_verify_by_mail_enter_code_path unless gpo_mail_policy.resend_letter_available? + unless gpo_mail_policy.resend_letter_available? + redirect_to idv_verify_by_mail_enter_code_path + end end def update_tracking diff --git a/app/policies/idv/gpo_verify_by_mail_policy.rb b/app/policies/idv/gpo_verify_by_mail_policy.rb index 4981d0398fe..6f2fd464dd9 100644 --- a/app/policies/idv/gpo_verify_by_mail_policy.rb +++ b/app/policies/idv/gpo_verify_by_mail_policy.rb @@ -18,6 +18,7 @@ def send_letter_available? FeatureManagement.gpo_verification_enabled? && !rate_limited? end + def rate_limited? too_many_letter_requests_within_window? || last_letter_request_too_recent? end @@ -46,7 +47,7 @@ def too_many_letter_requests_within_window? return false unless window_limit_enabled? user.gpo_confirmation_codes.where( created_at: IdentityConfig.store.max_mail_events_window_in_days.days.ago..Time.zone.now, - ).count >= IdentityConfig.store.max_mail_events + ).count >= IdentityConfig.store.max_mail_events end def last_letter_request_too_recent? @@ -58,8 +59,7 @@ def last_letter_request_too_recent? 'created_at > ?', IdentityConfig.store.minimum_wait_before_another_usps_letter_in_hours.hours.ago, ], - ) + ) end - end end diff --git a/spec/policies/idv/gpo_verify_by_mail_policy_spec.rb b/spec/policies/idv/gpo_verify_by_mail_policy_spec.rb index 012d4330bc5..179c321b6a8 100644 --- a/spec/policies/idv/gpo_verify_by_mail_policy_spec.rb +++ b/spec/policies/idv/gpo_verify_by_mail_policy_spec.rb @@ -3,11 +3,9 @@ require 'rails_helper' RSpec.describe Idv::GpoVerifyByMailPolicy do - let(:subject) { described_class.new(user) } let(:user) { create(:user) } - describe '#resend_letter_available?' do context 'when the feature flag is off' do before do @@ -153,7 +151,7 @@ before do user.gpo_verification_pending_profile.update( gpo_verification_pending_at: nil, - ) + ) end it 'returns false' do From 82e4875bc84c36977d1774fcabc6f130d92ee81a Mon Sep 17 00:00:00 2001 From: Matt Wagner Date: Wed, 26 Jun 2024 15:26:05 -0400 Subject: [PATCH 10/10] Fix useless ||= usage --- app/controllers/idv/phone_controller.rb | 2 +- app/controllers/idv/phone_errors_controller.rb | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/app/controllers/idv/phone_controller.rb b/app/controllers/idv/phone_controller.rb index b1348e61425..97db1d19f08 100644 --- a/app/controllers/idv/phone_controller.rb +++ b/app/controllers/idv/phone_controller.rb @@ -226,7 +226,7 @@ def formatted_previous_phone_step_params_phone def gpo_letter_available return @gpo_letter_available if defined?(@gpo_letter_available) policy = Idv::GpoVerifyByMailPolicy.new(current_user) - @gpo_letter_available ||= policy.send_letter_available? + @gpo_letter_available = policy.send_letter_available? end # Migrated from otp_delivery_method_controller diff --git a/app/controllers/idv/phone_errors_controller.rb b/app/controllers/idv/phone_errors_controller.rb index d510e711644..57670946238 100644 --- a/app/controllers/idv/phone_errors_controller.rb +++ b/app/controllers/idv/phone_errors_controller.rb @@ -75,7 +75,7 @@ def track_event(type:) def set_gpo_letter_available return @gpo_letter_available if defined?(@gpo_letter_available) policy = Idv::GpoVerifyByMailPolicy.new(current_user) - @gpo_letter_available ||= policy.send_letter_available? + @gpo_letter_available = policy.send_letter_available? end # rubocop:enable Naming/MemoizedInstanceVariableName end