Skip to content

Fix downloading personal keys outside of IDV (LG-5882)#6161

Merged
zachmargolis merged 3 commits intomainfrom
margolis-download-personal-key
Apr 5, 2022
Merged

Fix downloading personal keys outside of IDV (LG-5882)#6161
zachmargolis merged 3 commits intomainfrom
margolis-download-personal-key

Conversation

@zachmargolis
Copy link
Contributor

Problem: the personal key partial is used in a few places outside of IDV, but always linked to the IDV version's of the #download action (which means it would trigger an IDV proofing process if used outside of IDV... very undesirable)

Solution: stop using the #download action

  • Another solution would be to add a #download action to other personal key controllers
  • However, we've had multiple bugs in the past due to the #download action and its dependency on data in the session, so I think we're better off completely removing it

Next steps: completely remove #download

link_to(idv_download_personal_key_path, **tag_options, &block)
link_to(
"data:text/plain;charset=utf-8,#{CGI.escape(code)}",
download: 'personal_key.txt',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

one thought is maybe we can translate this filename? not that urgent IMO

Copy link
Contributor

Choose a reason for hiding this comment

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

I think fine for a follow-up, yeah

redirect_to next_step
end

# Remove this after the next deploy
Copy link
Contributor

Choose a reason for hiding this comment

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

👍🏼

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
Copy link
Contributor Author

Choose a reason for hiding this comment

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

another thought --- I think we can convert the backup codes download functionality that does something similar

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't quite follow?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We do the same "download" stuff in another controller and I think if we link this client-only approach, we should consider it there, too: https://github.com/18F/identity-idp/blob/main/app/controllers/users/backup_code_setup_controller.rb#L31-L34

Copy link
Contributor

Choose a reason for hiding this comment

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

ohhh got it 👍🏼

formEl.addEventListener('submit', handleSubmit);

if (window.navigator.msSaveBlob) {
downloadLink.addEventListener('click', downloadForIE);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was not able to test this myself, but @mitchellhenke said in slack the download worked

@zachmargolis
Copy link
Contributor Author

Just wanted to flag this that I noticed in the removal PR, we won't have any events.log for this action anymore, I think it's an OK tradeoff: https://github.com/18F/identity-idp/pull/6162/files#r843305759

@zachmargolis zachmargolis merged commit 14d6b9d into main Apr 5, 2022
@zachmargolis zachmargolis deleted the margolis-download-personal-key branch April 5, 2022 22:29
zachmargolis added a commit that referenced this pull request Apr 5, 2022
* download personal key via data: URL or JS fallback for Internet Explorer
* add specs and removal notes

changelog: Bug Fixes, Personal keys, Fixes downloading personal keys outside of identity verification
zachmargolis added a commit that referenced this pull request Apr 5, 2022
**Why**: Unused as of #6161

changelog: Internal, Source code, Remove unused download code
zachmargolis added a commit that referenced this pull request Apr 6, 2022
**Why**: Unused as of #6161

changelog: Internal, Source code, Remove unused download code
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants