-
Notifications
You must be signed in to change notification settings - Fork 131
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
LG-15216 Handle password reset for in-person enrollments waiting for fraud review #11855
base: main
Are you sure you want to change the base?
Changes from 6 commits
924badc
0531588
a894405
6352e2d
b9ec889
38a2b86
fc44f72
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -70,12 +70,12 @@ def mark_profile_as_password_reset | |
|
||
def password_reset_profile | ||
FeatureManagement.pending_in_person_password_reset_enabled? ? | ||
find_pending_in_person_or_active_profile : | ||
find_in_progress_in_person_or_active_profile : | ||
active_profile | ||
end | ||
|
||
def find_pending_in_person_or_active_profile | ||
user.pending_in_person_enrollment&.profile || active_profile | ||
def find_in_progress_in_person_or_active_profile | ||
user.current_in_progress_in_person_enrollment_profile || active_profile | ||
end | ||
|
||
# It is possible for an account that is resetting their password to be "invalid". | ||
|
@@ -104,7 +104,9 @@ def extra_analytics_attributes | |
|
||
def pending_profile_invalidated? | ||
if FeatureManagement.pending_in_person_password_reset_enabled? | ||
pending_profile.present? && !pending_profile.in_person_verification_pending? | ||
pending_profile.present? && | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for updating this. |
||
!pending_profile.in_person_verification_pending? && | ||
!pending_profile.fraud_deactivation_reason? | ||
else | ||
pending_profile.present? | ||
end | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,6 +5,7 @@ class FraudRejectionDailyJob < ApplicationJob | |
|
||
def perform(_date) | ||
profiles_eligible_for_fraud_rejection.find_each do |profile| | ||
profile.in_person_enrollment&.failed! | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💯 |
||
profile.reject_for_fraud(notify_user: false) | ||
analytics(user: profile.user).automatic_fraud_rejection( | ||
fraud_rejection_at: profile.fraud_rejection_at, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -34,6 +34,7 @@ def perform(_now) | |
enrollments_cancelled: 0, | ||
enrollments_in_progress: 0, | ||
enrollments_passed: 0, | ||
enrollments_in_fraud_review: 0, | ||
enrollments_skipped: 0, | ||
} | ||
|
||
|
@@ -142,8 +143,7 @@ def check_enrollment(enrollment) | |
|
||
def cancel_enrollment(enrollment) | ||
enrollment_outcomes[:enrollments_cancelled] += 1 | ||
enrollment.cancelled! | ||
enrollment.profile.deactivate_due_to_in_person_verification_cancelled | ||
enrollment.cancel | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍🏻 |
||
end | ||
|
||
def skip_enrollment(enrollment, profile_deactivation_reason) | ||
|
@@ -484,7 +484,7 @@ def handle_successful_status_update(enrollment, response) | |
|
||
def handle_passed_with_fraud_review_pending(enrollment, response) | ||
proofed_at = parse_usps_timestamp(response['transactionEndDateTime']) | ||
enrollment_outcomes[:enrollments_passed] += 1 | ||
enrollment_outcomes[:enrollments_in_fraud_review] += 1 | ||
log_enrollment_updated_analytics( | ||
enrollment: enrollment, | ||
enrollment_passed: true, | ||
|
@@ -493,7 +493,7 @@ def handle_passed_with_fraud_review_pending(enrollment, response) | |
reason: 'Passed with fraud pending', | ||
) | ||
enrollment.update( | ||
status: :passed, | ||
status: :in_fraud_review, | ||
proofed_at: proofed_at, | ||
status_check_completed_at: Time.zone.now, | ||
) | ||
|
@@ -641,7 +641,7 @@ def send_enrollment_status_sms_notification(enrollment:) | |
end | ||
|
||
def notification_delivery_params(enrollment) | ||
return {} unless enrollment.passed? || enrollment.failed? | ||
return {} unless enrollment.passed? || enrollment.failed? || enrollment.in_fraud_review? | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I feel like I've gotten rusty in this section of code. Is the intent here to start sending delayed notices of fraud-review status? It's just not obvious to me what the significance of this is. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah this isn't very obvious, but in the code before when the user passed at the post office and had been flagged for fraud review we would update the in-person enrollment to have a status of passed and send the please call email. In this PR we are changing things up a little bit by adding the new |
||
|
||
wait_until = enrollment.status_check_completed_at + ( | ||
IdentityConfig.store.in_person_results_delay_in_hours || DEFAULT_EMAIL_DELAY_IN_HOURS | ||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -223,9 +223,14 @@ def in_person_enrollment_status | |||||
pending_profile&.in_person_enrollment&.status | ||||||
end | ||||||
|
||||||
def ipp_enrollment_status_not_passed? | ||||||
# Whether the user's in person enrollment status is not passed or in_fraud_review. Enrollments use | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
The most trivial nit ever, but my eyes noticed it. |
||||||
# to go to passed status when profiles were marked as in fraud review. Since LG-15216, this will | ||||||
# no longer be the case. | ||||||
def ipp_enrollment_status_not_passed_or_in_fraud_review? | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This method could benefit from a comment / usage doc since it's public. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. fixed in 2edd2c2 |
||||||
!in_person_enrollment_status.blank? && | ||||||
in_person_enrollment_status != 'passed' | ||||||
[InPersonEnrollment::STATUS_PASSED, InPersonEnrollment::STATUS_IN_FRAUD_REVIEW].exclude?( | ||||||
in_person_enrollment_status, | ||||||
) | ||||||
end | ||||||
|
||||||
def has_in_person_enrollment? | ||||||
|
@@ -533,21 +538,28 @@ def last_sign_in_email_address | |||||
email_addresses.confirmed.last_sign_in | ||||||
end | ||||||
|
||||||
# Find the user's most recent in-progress enrollment profile. | ||||||
def current_in_progress_in_person_enrollment_profile | ||||||
in_person_enrollments | ||||||
.where(status: InPersonEnrollment::IN_PROGRESS_ENROLLMENT_STATUSES) | ||||||
.order(created_at: :desc) | ||||||
.first&.profile | ||||||
end | ||||||
|
||||||
private | ||||||
|
||||||
def find_password_reset_profile | ||||||
FeatureManagement.pending_in_person_password_reset_enabled? ? | ||||||
find_pending_in_person_or_active_profile : | ||||||
find_in_person_in_progress_or_active_profile : | ||||||
find_active_profile | ||||||
end | ||||||
|
||||||
def find_active_profile | ||||||
profiles.where.not(activated_at: nil).order(activated_at: :desc).first | ||||||
end | ||||||
|
||||||
def find_pending_in_person_or_active_profile | ||||||
pending_in_person_enrollment&.profile || | ||||||
profiles.where.not(activated_at: nil).order(activated_at: :desc).first | ||||||
def find_in_person_in_progress_or_active_profile | ||||||
current_in_progress_in_person_enrollment_profile || find_active_profile | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Originally, the only behavior was to find an active profile. When we enabled the epic, the default behavior will be to find an in person in progress or active profile. What do you think about changing the order of this so the default is to first look for an active profile because that was the original behavior? I think it would have a smaller affect on pw reset. (If for some reason in person enrollments was not getting cleaned up properly at some point- it would have a small impact because it would only happen if there was not an active profile.) I did not consider this in your previous pr. What are your thoughts?
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So in the last Password Reset PR I prioritized the pending profile first because when the personal key is generated for a profile it will replace the previous personal key. This can occur when the user has an active legacy profile and a pending non-legacy profile. My logic was that since the personal key is replaced, the previous active account is dead when the user performs a password reset. So, rather than having them lose both profiles I would prioritize the latest profile (the pending profile) which would allow the user to use their latest personal key. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @jmhooper does this sound reasonable? I know you reviewed the last PR and this didn't come up. I just wanted to make sure the way this is implemented makes sense. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @gina-yamada Good question! I agree with Shane's explanation and logic. Please feel free to say if you have more questions about this. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Shane and I walked through this together prior to his post yesterday. He posted for visibility. I am okay with this now that I have more information. Thank you |
||||||
end | ||||||
|
||||||
def lockout_period | ||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -91,13 +91,15 @@ def cancel_stale_establishing_enrollments_for_user(user) | |
.find_each(&:cancelled!) | ||
end | ||
|
||
# Cancel a user's associated establishing and pending in-person enrollments. | ||
# Cancel a user's associated establishing, pending, and in_fraud_review in-person enrollments. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oxford comma FTW! |
||
# | ||
# @param user [User] The user model | ||
def cancel_establishing_and_pending_enrollments(user) | ||
def cancel_establishing_and_in_progress_enrollments(user) | ||
user | ||
.in_person_enrollments | ||
.where(status: [:establishing, :pending]) | ||
.where(status: | ||
[InPersonEnrollment::STATUS_ESTABLISHING] + | ||
InPersonEnrollment::IN_PROGRESS_ENROLLMENT_STATUSES.to_a) | ||
.find_each(&:cancel) | ||
end | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -190,6 +190,7 @@ def run(args:, config:) | |
elsif FraudReviewChecker.new(user).fraud_review_eligible? | ||
profile = user.fraud_review_pending_profile | ||
profile_fraud_review_pending_at = profile.fraud_review_pending_at | ||
profile.in_person_enrollment&.failed! | ||
profile.reject_for_fraud(notify_user: true) | ||
success = true | ||
|
||
|
@@ -281,6 +282,7 @@ def run(args:, config:) | |
elsif FraudReviewChecker.new(user).fraud_review_eligible? | ||
profile = user.fraud_review_pending_profile | ||
profile_fraud_review_pending_at = profile.fraud_review_pending_at | ||
profile.in_person_enrollment&.passed! | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is another "Apologies that I don't remember this so well..." one, but I'm trying to make sure I understand what happens here in non-obvious cases.
This is a case where I think the logic is good but I just want to make sure we're not accidentally creating a vulnerability. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
profile.activate_after_passing_review | ||
success = true | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -127,4 +127,66 @@ | |
end | ||
end | ||
end | ||
|
||
describe '#ipp_enabled_and_enrollment_passed_or_in_fraud_review?' do | ||
context 'when in person tmx is enabled' do | ||
before do | ||
allow(IdentityConfig.store).to receive(:in_person_proofing_enforce_tmx).and_return(true) | ||
end | ||
|
||
context 'when ipp is enabled' do | ||
before do | ||
allow(IdentityConfig.store).to receive(:in_person_proofing_enabled).and_return(true) | ||
end | ||
|
||
context 'when user has a passed enrollment' do | ||
let!(:enrollment) { create(:in_person_enrollment, :passed, user: user, profile: profile) } | ||
|
||
it 'returns true' do | ||
expect(subject.ipp_enabled_and_enrollment_passed_or_in_fraud_review?).to be(true) | ||
end | ||
end | ||
|
||
context 'when user has an in_fraud_review enrollment' do | ||
let!(:enrollment) do | ||
create(:in_person_enrollment, :in_fraud_review, user: user, profile: profile) | ||
end | ||
|
||
it 'returns true' do | ||
expect(subject.ipp_enabled_and_enrollment_passed_or_in_fraud_review?).to be(true) | ||
end | ||
end | ||
|
||
context 'when user has a non passed or in_fraud_review enrollment' do | ||
let!(:enrollment) do | ||
create(:in_person_enrollment, :pending, user: user, profile: profile) | ||
end | ||
|
||
it 'returns false' do | ||
expect(subject.ipp_enabled_and_enrollment_passed_or_in_fraud_review?).to be(false) | ||
end | ||
end | ||
end | ||
|
||
context 'when ipp is disabled' do | ||
before do | ||
allow(IdentityConfig.store).to receive(:in_person_proofing_enabled).and_return(false) | ||
end | ||
|
||
it 'returns false' do | ||
expect(subject.ipp_enabled_and_enrollment_passed_or_in_fraud_review?).to be(false) | ||
end | ||
end | ||
end | ||
|
||
context 'when in person tmx is disabled' do | ||
before do | ||
allow(IdentityConfig.store).to receive(:in_person_proofing_enforce_tmx).and_return(false) | ||
end | ||
|
||
it 'returns nil' do | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Tests look clear and thorough. |
||
expect(subject.ipp_enabled_and_enrollment_passed_or_in_fraud_review?).to be_nil | ||
end | ||
end | ||
end | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is check
IdentityConfig.store.in_person_proofing_enabled
needed? (I know this code predates you and you did not change it.)If we disable in_person_proofing_enabled because of an outage and the user begins again, wouldn't we want a clean slate even if they are not able to move through ipp? Can you think of any disadvantages to deleting the check?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a good question. I'm not (yet) sure where I fall on this topic, but I am curious to hear more about it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cc @matthinz because I feel like this is the type of thing we discussed recently. This method is called from the welcome controller's update method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that makes sense, but I feel that this change would be better in a different PR/User Story.
cc: @eileen-nava
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@shanechesnutt-ft Would you be up for writing a ticket to address this feedback in a separate PR?