Skip to content

Add Document Encryption Service for Doc Escrow#11714

Merged
Sgtpluck merged 6 commits intomainfrom
dmm/doc-escrow-infrastructure
Jan 10, 2025
Merged

Add Document Encryption Service for Doc Escrow#11714
Sgtpluck merged 6 commits intomainfrom
dmm/doc-escrow-infrastructure

Conversation

@Sgtpluck
Copy link
Contributor

@Sgtpluck Sgtpluck commented Jan 7, 2025

🎫 Ticket

Link to the relevant ticket:
Add Document Encryption Service for Doc Escrow

🛠 Summary of changes

This change adds an EncryptedDocStorage service that will be used for Doc Escrow.

  • encrypts the front and back images of a document
  • stores the images locally by default
  • returns the image UUIDs and the encryption key

Open questions:
This implementation stole liberally from @jmhooper's initial implementation of this service. It currently relies on two images being passed in. Do we think that will always be the case? Is it possible that there might be single images (like a selfie?)

I am wondering if this updated implementation should:

  • Only accept a single image so that the caller of this method has to call it multiple times for each image they want encrypted, and each image has its own encryption key, OR;
  • Accept an array of images and then loop over them, and return all the uuids with a single encryption key, OR;
  • Make the same assumptions the first implementation made about there always being a front/back image.

I think I like the first option best, as it makes less assumptions, but does mean multiple encryption keys. Would love to hear other thoughts!

@zachmargolis
Copy link
Contributor

  • Only accept a single image so that the caller of this method has to call it multiple times for each image they want encrypted, and each image has its own encryption key, OR;

I also vote for one image at a time, I think it would simplify the structure and allow for flexibility across multiple flows (with and without selfie)

@Sgtpluck Sgtpluck force-pushed the dmm/doc-escrow-infrastructure branch from adafeeb to 9fc4634 Compare January 7, 2025 18:42
Copy link
Contributor

@mitchellhenke mitchellhenke left a comment

Choose a reason for hiding this comment

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

I lean towards the first option in that documents should be stored and referenced individually

Sgtpluck and others added 3 commits January 7, 2025 16:21
Comment on lines +23 to +24
# cleanup
File.delete(file_path(result.name))
Copy link
Contributor

Choose a reason for hiding this comment

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

another approach to cleanup and writing files to disk would be to make the whole example use a tmpdir, like we do here. We could update the class to take an optional dir argument and pass the path in here, the default value would be the Rails.root.join('tmp') like have it now

around do |ex|
Dir.mktmpdir('/locales') do |dir|
@tmpdir = dir
ex.run
end
end

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 considered a couple different approaches to cleanup, but since it was just two specs in different spec files, it seemed fine to just inline it for now. will definitely consider other approaches as we extend it. (unless you feel very strongly about it, then i can update it now!)

Copy link
Contributor

Choose a reason for hiding this comment

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

Not that strongly!

@jmhooper
Copy link
Contributor

jmhooper commented Jan 8, 2025

I will note the passports only have a single image. I don't have a strong opinion about how we handle a single image or 3 images but I think we do need to support it.

Copy link
Contributor

@jmhooper jmhooper left a comment

Choose a reason for hiding this comment

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

This does look familiar to me. I feel good about this approach.

@Sgtpluck Sgtpluck merged commit b4ade25 into main Jan 10, 2025
@Sgtpluck Sgtpluck deleted the dmm/doc-escrow-infrastructure branch January 10, 2025 18:19
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.

4 participants