Add document escrow fields to Attempts API (LG-8057) by matthinz · Pull Request #7381 · 18F/identity-idp

matthinz · 2022-11-22T00:18:46Z

🎫 Ticket

🛠 Summary of changes

#7351 previously added encrypted document escrow support.

This PR updates the idv_document_upload_submitted event in the IRS attempts API to include the following keys (when document escrow is enabled):

Key	Description
`document_front_image_filename`	S3 filename (composed of a UUID + extension) for the image of the front of the document.
`document_back_image_filename`	S3 filename (composed of a UUID + extension) for the image of the back of the document.
`document_image_encryption_key`	Base64-encoded key used to encrypt the image data written to S3.

zachmargolis

LGTM

app/forms/idv/api_image_upload_form.rb

spec/controllers/idv/image_uploads_controller_spec.rb

app/services/encrypted_document_storage/local_storage.rb

app/services/irs_attempts_api/tracker_events.rb

solipet

A few comments to consider, but looks great!

app/services/irs_attempts_api/tracker_events.rb

changelog: Improvements, IRS Attempts API, Add document escrow fields to IRS attempts API

Write documents to encrypted storage and read them back

Co-authored-by: Zach Margolis <zachmargolis@users.noreply.github.com>

Clarify that these args hold UUIDs identifying the front/back images

avoid polluting global namespace

50% off!

try and make it clearer what these things are

whoops

spec/services/encrypted_document_storage/document_writer_spec.rb

matthinz · 2022-11-23T19:01:58Z

@zachmargolis & @18F/identity-agnes: We had a late-breaking requirements change here, reducing the document escrow fields in the attempts API from 6 to 3. The big changes are:

content_type will now be inferred from the extension added to the S3 key for the image (e.g. ".jpeg" for `image/jpeg)
There's now only one encryption key field (we'd previously left the door open for using separate keys for front/back.

LMK if you need any more context!

zachmargolis

LGTM!

app/services/encrypted_document_storage/document_writer.rb

zachmargolis · 2022-11-23T22:08:55Z

spec/services/encrypted_document_storage/document_writer_spec.rb

+      end
+
+      context 'nonsense' do
+        let(:content_type) { 'yabba/dabbadoo' }


solipet

Looks good. One comment you can take or leave.

solipet · 2022-11-23T21:03:10Z

spec/services/encrypted_document_storage/document_writer_spec.rb

+
+      context 'jpeg' do
+        let(:content_type) { 'image/jpeg' }
+        it { should eql('.jpeg') }


These obviously work, but are written in the older "should" syntax. The more modern format used throughout login would be the "expect" syntax, e.g.,

it 'has the correct extension' do expect(subject).to eq('.jpeg') end

Co-authored-by: Zach Margolis <zachmargolis@users.noreply.github.com>

* Add content_type to DataUrlImage * First pass at wiring doc upload into attempts API changelog: Improvements, IRS Attempts API, Add document escrow fields to IRS attempts API * Additional tests * WIP * Update form test to roundtrip documents Write documents to encrypted storage and read them back * Use front_image & back_image lets for tests * Update ImageUploadsController spec * Test Attempts API includes images on validation failure * Update app/forms/idv/api_image_upload_form.rb Co-authored-by: Zach Margolis <zachmargolis@users.noreply.github.com> * Update app/services/encrypted_document_storage/local_storage.rb Co-authored-by: Zach Margolis <zachmargolis@users.noreply.github.com> * Update *_image -> *_image_uuid Clarify that these args hold UUIDs identifying the front/back images * Use let() for regexes in tests avoid polluting global namespace * Add doc escrow parameter descriptions * Minimize unnecessary verbosity * Lint issues * Reduce number of document escrow fields 50% off! * add "filename" to fields try and make it clearer what these things are * Remove empty test whoops * Update app/services/encrypted_document_storage/document_writer.rb Co-authored-by: Zach Margolis <zachmargolis@users.noreply.github.com> * Update spec from `should` to `is_expected.to` Co-authored-by: Zach Margolis <zachmargolis@users.noreply.github.com>

matthinz requested a review from a team November 22, 2022 00:19

zachmargolis requested a review from a team November 22, 2022 00:26

zachmargolis approved these changes Nov 22, 2022

View reviewed changes

app/forms/idv/api_image_upload_form.rb Outdated Show resolved Hide resolved

spec/controllers/idv/image_uploads_controller_spec.rb Outdated Show resolved Hide resolved

app/services/encrypted_document_storage/local_storage.rb Outdated Show resolved Hide resolved

zachmargolis reviewed Nov 22, 2022

View reviewed changes

app/services/irs_attempts_api/tracker_events.rb Outdated Show resolved Hide resolved

solipet approved these changes Nov 22, 2022

View reviewed changes

app/services/irs_attempts_api/tracker_events.rb Outdated Show resolved Hide resolved

matthinz and others added 11 commits November 22, 2022 11:09

Add content_type to DataUrlImage

4201810

First pass at wiring doc upload into attempts API

3470427

changelog: Improvements, IRS Attempts API, Add document escrow fields to IRS attempts API

Additional tests

183f4bd

WIP

b5652f9

Update form test to roundtrip documents

da30bf5

Write documents to encrypted storage and read them back

Use front_image & back_image lets for tests

c7135cf

Update ImageUploadsController spec

38a7f69

Test Attempts API includes images on validation failure

14d77cd

Update app/forms/idv/api_image_upload_form.rb

4c40136

Co-authored-by: Zach Margolis <zachmargolis@users.noreply.github.com>

Update app/services/encrypted_document_storage/local_storage.rb

081a01a

Co-authored-by: Zach Margolis <zachmargolis@users.noreply.github.com>

Update *_image -> *_image_uuid

0a544bd

Clarify that these args hold UUIDs identifying the front/back images

matthinz force-pushed the matthinz/8057-attempts-api-uploads branch from d6bbdb7 to 0a544bd Compare November 22, 2022 19:21

matthinz added 7 commits November 22, 2022 11:22

Use let() for regexes in tests

c339a47

avoid polluting global namespace

Add doc escrow parameter descriptions

fedb7d9

Minimize unnecessary verbosity

2ae1ff0

Lint issues

dd759dc

Reduce number of document escrow fields

4ac2cd4

50% off!

add "filename" to fields

26c9565

try and make it clearer what these things are

Remove empty test

f5f7831

whoops

matthinz commented Nov 23, 2022

View reviewed changes

spec/services/encrypted_document_storage/document_writer_spec.rb Show resolved Hide resolved

matthinz requested review from a team, solipet and zachmargolis November 23, 2022 19:02

zachmargolis approved these changes Nov 23, 2022

View reviewed changes

solipet approved these changes Nov 23, 2022

View reviewed changes

matthinz and others added 2 commits November 25, 2022 09:53

Update app/services/encrypted_document_storage/document_writer.rb

def1d5e

Co-authored-by: Zach Margolis <zachmargolis@users.noreply.github.com>

Update spec from should to is_expected.to

dfc4c5a

matthinz merged commit 0e16a62 into main Nov 25, 2022

matthinz deleted the matthinz/8057-attempts-api-uploads branch November 25, 2022 18:49

mdiarra3 mentioned this pull request Nov 28, 2022

Deploy RC 231 to Stages/prod #7395

Merged

This was referenced Nov 29, 2022

Deploy RC 232 to Prod #7404

Closed

Deploy RC 232 to production #7414

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add document escrow fields to Attempts API (LG-8057)#7381

Add document escrow fields to Attempts API (LG-8057)#7381
matthinz merged 20 commits intomainfrom
matthinz/8057-attempts-api-uploads

matthinz commented Nov 22, 2022 •

edited

Loading

Uh oh!

zachmargolis left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

solipet left a comment

Uh oh!

Uh oh!

Uh oh!

matthinz commented Nov 23, 2022

Uh oh!

zachmargolis left a comment

Uh oh!

Uh oh!

zachmargolis Nov 23, 2022

Uh oh!

solipet left a comment

Uh oh!

solipet Nov 23, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

matthinz commented Nov 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🎫 Ticket

🛠 Summary of changes

Uh oh!

zachmargolis left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

solipet left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

matthinz commented Nov 23, 2022

Uh oh!

zachmargolis left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

zachmargolis Nov 23, 2022

Choose a reason for hiding this comment

Uh oh!

solipet left a comment

Choose a reason for hiding this comment

Uh oh!

solipet Nov 23, 2022

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

matthinz commented Nov 22, 2022 •

edited

Loading