Skip to content

Allow idv/confirmations_controller #show and #download out of order (LG-4847) #5230

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
zachmargolis merged 2 commits into from
Jul 23, 2021
Merged

Allow idv/confirmations_controller #show and #download out of order (LG-4847) #5230

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d59f306
Allow idv/confirmations_controller #show and #download out of order (…
zachmargolis Jul 22, 2021
5a0d1d6
Remove redundant assignment
zachmargolis Jul 22, 2021
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
16 changes: 4 additions & 12 deletions app/controllers/idv/confirmations_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,16 +19,10 @@ def update
end

def download
personal_key = user_session[:personal_key]
code = personal_key

analytics.track_event(Analytics::IDV_DOWNLOAD_PERSONAL_KEY, success: personal_key.present?)

if personal_key.present?
data = personal_key + "\r\n"
send_data data, filename: 'personal_key.txt'
else
head :bad_request
end
analytics.track_event(Analytics::IDV_DOWNLOAD_PERSONAL_KEY, success: code.present?)
Copy link
Copy Markdown
Contributor

@aduth aduth Jul 23, 2021

Choose a reason for hiding this comment

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

This logging was introduced in #5215, I assume to be able to find the sorts of issues we were seeing then. If we're addressing that and assuming code will always be set, do we still need the logging? Or at least the success property?

Copy link
Copy Markdown
Contributor Author

@zachmargolis zachmargolis Jul 23, 2021

Choose a reason for hiding this comment

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

We still want the event to help us track when people do download, but I guess now the success would help us detect some not-expected degenerate cases where random key generation fails

send_data "#{code}\r\n", filename: 'personal_key.txt'
end

private
Expand Down Expand Up @@ -71,8 +65,6 @@ def add_proofing_component

def finish_idv_session
@code = personal_key
user_session[:personal_key] = @code
idv_session.personal_key = nil
Comment on lines -74 to -75
Copy link
Copy Markdown
Contributor Author

@zachmargolis zachmargolis Jul 22, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

it seemed like Idv::Session#personal_key was being written to a bunch but never actually used?

I just decided to remove it to simplify, since we already store the data across requests in user_session


if idv_session.address_verification_mechanism == 'gpo'
flash.now[:success] = t('idv.messages.mail_sent')
Expand All @@ -83,7 +75,7 @@ def finish_idv_session
end

def personal_key
idv_session.personal_key || generate_personal_key
user_session[:personal_key] ||= generate_personal_key
end

def generate_personal_key
Expand Down
2 changes: 0 additions & 2 deletions app/services/idv/session.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@ class Session
profile_confirmation
profile_id
profile_step_params
personal_key
resolution_successful
step_attempts
].freeze
Expand Down Expand Up @@ -52,7 +51,6 @@ def create_profile_from_applicant_with_password(user_password)
profile = profile_maker.save_profile
self.pii = profile_maker.pii_attributes
self.profile_id = profile.id
self.personal_key = profile.personal_key
end

def cache_encrypted_pii(password)
Expand Down
26 changes: 20 additions & 6 deletions spec/controllers/idv/confirmations_controller_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ def stub_idv_session
profile = profile_maker.save_profile
idv_session.pii = profile_maker.pii_attributes
idv_session.profile_id = profile.id
idv_session.personal_key = profile.personal_key
subject.user_session[:personal_key] = profile.personal_key
allow(subject).to receive(:idv_session).and_return(idv_session)
end

Expand Down Expand Up @@ -95,7 +95,7 @@ def index

it 'sets code instance variable' do
subject.idv_session.create_profile_from_applicant_with_password(password)
code = subject.idv_session.personal_key
code = subject.user_session[:personal_key]

get :show

Expand Down Expand Up @@ -226,7 +226,7 @@ def index

it 'allows download of code' do
subject.idv_session.create_profile_from_applicant_with_password(password)
code = subject.idv_session.personal_key
code = subject.user_session[:personal_key]

get :show
get :download
Expand All @@ -236,11 +236,25 @@ def index
expect(@analytics).to have_logged_event(Analytics::IDV_DOWNLOAD_PERSONAL_KEY, success: true)
end

it 'is a bad request when there is no personal_key in the session' do
it 'can be called separately from #show' do
get :download

expect(response).to be_bad_request
expect(@analytics).to have_logged_event(Analytics::IDV_DOWNLOAD_PERSONAL_KEY, success: false)
expect(response).to be_ok

code = subject.user_session[:personal_key]
expect(response.body).to eq(code + "\r\n")
end

it 'can be called out of order and have the same code as #show' do
subject.user_session[:personal_key] = nil

expect { get :download }.to change { subject.user_session[:personal_key] }.from(nil)

expect(response).to be_ok
code = response.body.chomp

get :show
expect(assigns(:code)).to eq(code)
end
end
end