Skip to content

Remove idv/personal_key#download action #6162

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 1 commit into from
Apr 6, 2022
Merged

Remove idv/personal_key#download action #6162

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
14 changes: 0 additions & 14 deletions app/controllers/idv/personal_key_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,20 +22,6 @@ def update
redirect_to next_step
end

# Remove this after the next deploy
def download
personal_key = user_session[:personal_key]

analytics.track_event(Analytics::IDV_PERSONAL_KEY_DOWNLOADED, success: personal_key.present?)
Copy link
Copy Markdown
Contributor Author

@zachmargolis zachmargolis Apr 5, 2022

Choose a reason for hiding this comment

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

🌶️ one downside to not having a server action is we no longer get the analytics

However, I still feel strongly enough that having this action on the server is a liability, so I am OK not having analytics if it means we can have a more bug-free frontend

Copy link
Copy Markdown
Contributor Author

@zachmargolis zachmargolis Apr 5, 2022

Choose a reason for hiding this comment

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

https://cm-jira.usa.gov/browse/LG-6111 so we can track this

Copy link
Copy Markdown
Contributor

@aduth aduth Apr 6, 2022

Choose a reason for hiding this comment

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

We have some precedent for front-end logging with #5875, which might be useful for this.

(Also commented as such on related ticket)


if personal_key.present?
data = personal_key + "\r\n"
send_data data, filename: 'personal_key.txt'
else
head :bad_request
end
end

private

def step_indicator_steps
Expand Down
1 change: 0 additions & 1 deletion app/services/analytics.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -154,7 +154,6 @@ def browser_attributes
IDV_INTRO_VISIT = 'IdV: intro visited'
IDV_JURISDICTION_VISIT = 'IdV: jurisdiction visited'
IDV_JURISDICTION_FORM = 'IdV: jurisdiction form submitted'
IDV_PERSONAL_KEY_DOWNLOADED = 'IdV: personal key downloaded' # previously "IdV: download personal key"
IDV_PERSONAL_KEY_VISITED = 'IdV: personal key visited'
IDV_PERSONAL_KEY_SUBMITTED = 'IdV: personal key submitted'
IDV_PHONE_CONFIRMATION_FORM = 'IdV: phone confirmation form'
Expand Down
8 changes: 8 additions & 0 deletions app/services/analytics_events.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -179,6 +179,14 @@ def email_and_password_auth(
)
end

# @deprecated
# A user has downloaded their personal key. This event is no longer emitted.
Comment on lines +182 to +183
Copy link
Copy Markdown
Contributor

@aduth aduth Apr 6, 2022

Choose a reason for hiding this comment

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

YAGNI?

Or is this just so that we have a reference in the analytics documentation for historical lookups?

Copy link
Copy Markdown
Contributor Author

@zachmargolis zachmargolis Apr 6, 2022

Choose a reason for hiding this comment

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

Yes, I figured it would be good to have a reference. And if we choose to implement frontend logging, we can link these two as "previous event names" for that or something

# @identity.idp.event_name IdV: personal key downloaded
# @identity.idp.previous_event_name IdV: download personal key
def idv_personal_key_downloaded
track_event('IdV: personal key downloaded')
end

# @identity.idp.event_name IdV: phone confirmation otp submitted
# @param [Boolean] success
# @param [Hash] errors
Expand Down
2 changes: 0 additions & 2 deletions config/routes.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -276,8 +276,6 @@
get '/come_back_later' => 'come_back_later#show'
get '/personal_key' => 'personal_key#show'
post '/personal_key' => 'personal_key#update'
# Remove this after the next deploy
get '/download_personal_key' => 'personal_key#download'
get '/forgot_password' => 'forgot_password#new'
post '/forgot_password' => 'forgot_password#update'
get '/otp_delivery_method' => 'otp_delivery_method#new'
Expand Down
41 changes: 0 additions & 41 deletions spec/controllers/idv/personal_key_controller_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -187,45 +187,4 @@ def index
end
end
end

describe '#download' do
before do
stub_idv_session
stub_analytics
end

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

get :show
get :download

expect(response.body).to eq(code + "\r\n")
expect(response.header['Content-Type']).to eq('text/plain')
expect(@analytics).to have_logged_event(Analytics::IDV_PERSONAL_KEY_DOWNLOADED, success: true)
end

it 'recovers pii and verifies personal key digest with the code' do
get :show
get :download

code = response.body.chomp

expect(PersonalKeyGenerator.new(user).verify(code)).to eq true
expect(user.profiles.first.recover_pii(normalize_personal_key(code))).to eq(
subject.idv_session.pii,
)
end

it 'is a bad request when there is no personal_key in the session' do
get :download

expect(response).to be_bad_request
expect(@analytics).to have_logged_event(
Analytics::IDV_PERSONAL_KEY_DOWNLOADED,
success: false,
)
end
end
end