Skip to content

LG-14749 Cancel enrollments on profile encryption error #11585

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.

Sign up for GitHub

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

Merged
shanechesnutt-ft merged 1 commit into from
Dec 6, 2024
Merged

LG-14749 Cancel enrollments on profile encryption error #11585

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion app/controllers/concerns/ial2_profile_concern.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ def cache_profile_and_handle_errors(raw_password, profile)
cacher.save(raw_password, profile)
rescue Encryption::EncryptionError => err
if profile
profile.deactivate(:encryption_error)
profile.deactivate_due_to_encryption_error
analytics.profile_encryption_invalid(error: err.message)
end
end
Expand Down
14 changes: 14 additions & 0 deletions app/models/profile.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -191,6 +191,20 @@ def deactivate(reason)
update!(active: false, deactivation_reason: reason)
end

# Update the profile's deactivation reason to "encryption_error". As a
# side-effect, when the profile has an associated pending in-person
# enrollment it will be updated to have a status of "cancelled".
def deactivate_due_to_encryption_error
update!(
active: false,
deactivation_reason: :encryption_error,
)

if in_person_enrollment&.pending?
in_person_enrollment.cancelled!
end
Comment on lines 203 to 205
Copy link
Copy Markdown
Contributor

@matthinz matthinz Dec 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would there be any harm in doing the canceling of the enrollment in the background job? Like when we go to check on the status of the enrollment, notice the profile is deactivated, and deactivate the enrollment. If the profile is out of sync with the enrollment in the time between when the profile is deactivated and we next check on the enrollment, is that bad?

Copy link
Copy Markdown
Contributor Author

@shanechesnutt-ft shanechesnutt-ft Dec 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is logic in place that handles cancelling enrollments with profiles that are deactivated in the background job. The issue with it is that users are able to attempt to schedule an enrollment before the pending enrollment is cancelled because of the 30 min / 1 hour window of the background job. If it doesn't make sense to cancel it at the point of encryption error, maybe it would be better to have a check during the ID-IPP workflow to cancel any existing pending enrollments like we do with existing establishing enrollments.

Copy link
Copy Markdown
Contributor Author

@shanechesnutt-ft shanechesnutt-ft Dec 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The profile and in-person enrollment models are tightly coupled which is why I thought it is important that we try to keep them in sync. So when one becomes in a dead state the other should also be in a dead state.

Copy link
Copy Markdown
Contributor

@matthinz matthinz Dec 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gotcha, that makes sense to me

end

def fraud_deactivation_reason?
fraud_review_pending? || fraud_rejection?
end
Expand Down
213 changes: 213 additions & 0 deletions spec/controllers/concerns/ial2_profile_concern_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,213 @@
require 'rails_helper'

RSpec.describe Ial2ProfileConcern do
Copy link
Copy Markdown
Contributor

@n1zyy n1zyy Dec 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👏 for adding tests for this class!

let(:test_controller) do
Class.new do
include Ial2ProfileConcern

attr_accessor :current_user, :user_session, :analytics

def initialize(current_user:, user_session:, analytics:)
@current_user = current_user
@user_session = user_session
@analytics = analytics
end
end
end

let(:user) { create(:user) }
let(:user_session) { {} }
let(:analytics) { double(Analytics) }
let(:encryption_error) { Encryption::EncryptionError.new }
let(:cacher) { double(Pii::Cacher) }
let(:password) { 'PraiseTheSun!' }
Comment thread
gina-yamada marked this conversation as resolved.
Outdated

describe '#cache_profiles' do
subject { test_controller.new(current_user: user, user_session:, analytics:) }

context 'when the user has a pending profile' do
let!(:profile) { create(:profile, :in_person_verification_pending, user:) }

context 'when the profile can be saved to cache' do
before do
allow(cacher).to receive(:save)
allow(Pii::Cacher).to receive(:new).and_return(cacher)
subject.cache_profiles(password)
end

it 'stores the decrypted profile in cache' do
expect(cacher).to have_received(:save).with(password, profile)
end
end

context 'when the profile can not be saved to cache' do
before do
allow(cacher).to receive(:save).and_raise(encryption_error)
allow(Pii::Cacher).to receive(:new).and_return(cacher)
allow(analytics).to receive(:profile_encryption_invalid)
subject.cache_profiles(password)
end

it 'deactivates the profile with reason encryption_error' do
expect(profile.reload).to have_attributes(
active: false,
deactivation_reason: 'encryption_error',
Comment thread
gina-yamada marked this conversation as resolved.
Outdated
)
end

it 'logs the profile_encryption_invalid analytic' do
expect(analytics).to have_received(:profile_encryption_invalid).with(
error: encryption_error.message,
)
end
end
end

context 'when the user has an active profile' do
let!(:profile) { create(:profile, :active, user:) }

context 'when the profile can be saved to cache' do
before do
allow(cacher).to receive(:save)
allow(Pii::Cacher).to receive(:new).and_return(cacher)
subject.cache_profiles(password)
end

it 'stores the decrypted profile in cache' do
expect(cacher).to have_received(:save).with(password, profile)
end
end

context 'when the profile can not be saved to cache' do
before do
allow(cacher).to receive(:save).and_raise(encryption_error)
allow(Pii::Cacher).to receive(:new).and_return(cacher)
allow(analytics).to receive(:profile_encryption_invalid)
subject.cache_profiles(password)
end

it 'deactivates the profile with reason encryption_error' do
expect(profile.reload).to have_attributes(
active: false,
deactivation_reason: 'encryption_error',
)
end

it 'logs the profile_encryption_invalid analytic' do
expect(analytics).to have_received(:profile_encryption_invalid).with(
error: encryption_error.message,
)
end
end
end

context 'when the user has both an active profile and pending profile' do
Copy link
Copy Markdown
Contributor

@n1zyy n1zyy Dec 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👏 for adding thorough tests to this class that previously had none. 😃

let(:pending_profile) { create(:profile, :in_person_verification_pending, user:) }
let(:active_profile) { create(:profile, :active, user:) }

context 'when the active profile was activated before the pending profile was created' do
before do
pending_profile.update!(created_at: Time.zone.now)
active_profile.update!(activated_at: 1.day.ago)
end

context 'when the profiles can be saved to cache' do
before do
allow(cacher).to receive(:save)
allow(Pii::Cacher).to receive(:new).and_return(cacher)
subject.cache_profiles(password)
end

it 'stores the decrypted pending profile in cache' do
expect(cacher).to have_received(:save).with(password, pending_profile)
end

it 'stores the decrypted active profile in cache' do
expect(cacher).to have_received(:save).with(password, active_profile)
end
end

context 'when the profile can not be saved to cache' do
before do
allow(cacher).to receive(:save).and_raise(encryption_error)
allow(Pii::Cacher).to receive(:new).and_return(cacher)
allow(analytics).to receive(:profile_encryption_invalid)
subject.cache_profiles(password)
end

it 'deactivates the pending profile with reason encryption_error' do
expect(pending_profile.reload).to have_attributes(
active: false,
deactivation_reason: 'encryption_error',
)
end

it 'deactivates the active profile with reason encryption_error' do
expect(active_profile.reload).to have_attributes(
active: false,
deactivation_reason: 'encryption_error',
)
end

it 'logs the profile_encryption_invalid analytic' do
expect(analytics).to have_received(:profile_encryption_invalid).with(
error: encryption_error.message,
).twice
end
end
end

context 'when the active profile was activated after the pending profile was created' do
before do
pending_profile.update!(created_at: 1.day.ago)
active_profile.update!(activated_at: Time.zone.now)
end

context 'when the profiles can be saved to cache' do
before do
allow(cacher).to receive(:save)
allow(Pii::Cacher).to receive(:new).and_return(cacher)
subject.cache_profiles(password)
end

it 'does not store the decrypted pending profile in cache' do
expect(cacher).not_to have_received(:save).with(password, pending_profile)
end

it 'stores the decrypted active profile in cache' do
expect(cacher).to have_received(:save).with(password, active_profile)
end
end

context 'when the profile can not be saved to cache' do
before do
allow(cacher).to receive(:save).and_raise(encryption_error)
allow(Pii::Cacher).to receive(:new).and_return(cacher)
allow(analytics).to receive(:profile_encryption_invalid)
subject.cache_profiles(password)
end

it 'does not deactivate the pending profile with reason encryption_error' do
expect(pending_profile.reload).to have_attributes(
active: false,
deactivation_reason: nil,
)
end

it 'deactivates the active profile with reason encryption_error' do
expect(active_profile.reload).to have_attributes(
active: false,
deactivation_reason: 'encryption_error',
)
end

it 'logs the profile_encryption_invalid analytic' do
expect(analytics).to have_received(:profile_encryption_invalid).with(
error: encryption_error.message,
).once
end
end
end
end
end
end
24 changes: 12 additions & 12 deletions spec/features/idv/get_proofing_results_job_scenarios_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,9 +51,9 @@

# Then the user is taken to the /verify/welcome page
expect(current_path).to eq(idv_welcome_path)
# And the user has an InPersonEnrollment with status "pending"
# And the user has an InPersonEnrollment with status "cancelled"
expect(@user.in_person_enrollments.first).to have_attributes(
status: 'pending',
status: 'cancelled',
)
# And the user has a Profile that is deactivated with reason "encryption_error"
expect(@user.in_person_enrollments.first.profile).to have_attributes(
Expand All @@ -78,7 +78,7 @@
expect(@user.in_person_enrollments.first.profile).to have_attributes(
active: false,
deactivation_reason: 'encryption_error',
in_person_verification_pending_at: nil,
in_person_verification_pending_at: be_kind_of(Time),
Copy link
Copy Markdown
Contributor

@n1zyy n1zyy Dec 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Irrelevant mumblings: my brain always reads this as though it means be_somewhat, like be_kind_of(:confusing) or such. It obviously doesn't make any sense that that would be a matcher, though.

I was going to comment that I prefer be_a_kind_of which reads much better, but in searching the code, be_kind_of is much more common so we should stick with it. (It's kind_of(:a_bummer) though.)

)

# When the user logs in
Expand All @@ -94,7 +94,7 @@
expect(@user.in_person_enrollments.first.profile).to have_attributes(
active: false,
deactivation_reason: 'encryption_error',
in_person_verification_pending_at: nil,
in_person_verification_pending_at: be_kind_of(Time),
)
end

Expand Down Expand Up @@ -130,9 +130,9 @@

# Then the user is taken to the /verify/welcome page
expect(current_path).to eq(idv_welcome_path)
# And the user has an InPersonEnrollment with status "pending"
# And the user has an InPersonEnrollment with status "cancelled"
expect(@user.in_person_enrollments.first).to have_attributes(
status: 'pending',
status: 'cancelled',
)
# And the user has a Profile that is deactivated with reason "encryption_error"
expect(@user.in_person_enrollments.first.profile).to have_attributes(
Expand All @@ -157,7 +157,7 @@
expect(@user.in_person_enrollments.first.profile).to have_attributes(
active: false,
deactivation_reason: 'encryption_error',
in_person_verification_pending_at: nil,
in_person_verification_pending_at: be_kind_of(Time),
)

# When the user logs in
Expand All @@ -173,7 +173,7 @@
expect(@user.in_person_enrollments.first.profile).to have_attributes(
active: false,
deactivation_reason: 'encryption_error',
in_person_verification_pending_at: nil,
in_person_verification_pending_at: be_kind_of(Time),
)
end
end
Expand Down Expand Up @@ -537,9 +537,9 @@

# Then the user is taken to the /verify/welcome page
expect(current_path).to eq(idv_welcome_path)
# And the user has an InPersonEnrollment with status "pending"
# And the user has an InPersonEnrollment with status "cancelled"
expect(@user.in_person_enrollments.first).to have_attributes(
status: 'pending',
status: 'cancelled',
)
# And the user has a Profile that is deactivated with reason "encryption_error" and
# pending in person verification and fraud review
Expand Down Expand Up @@ -568,7 +568,7 @@
expect(@user.in_person_enrollments.first.profile).to have_attributes(
active: false,
deactivation_reason: 'encryption_error',
in_person_verification_pending_at: nil,
in_person_verification_pending_at: be_kind_of(Time),
fraud_pending_reason: 'threatmetrix_review',
fraud_review_pending_at: be_kind_of(Time),
)
Expand All @@ -587,7 +587,7 @@
expect(@user.in_person_enrollments.first.profile).to have_attributes(
active: false,
deactivation_reason: 'encryption_error',
in_person_verification_pending_at: nil,
in_person_verification_pending_at: be_kind_of(Time),
fraud_pending_reason: 'threatmetrix_review',
fraud_review_pending_at: be_kind_of(Time),
)
Expand Down
Loading