Skip to content
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.

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

Handles s3 action w/ additional kms encryption #8

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

armiiller
Copy link

Adds the ability to get the message from S3.

Very large emails can actually get omitted from SNS if they are too large over 256KB. There is event a special library in Java for playing with large message.

That being said, "easy" the solution is to store the SES email to an S3 bucket, get an SNS message saying its been stored in a bucket, then download the content from the bucket object.

I've added an "opinionated" version of this here. It also supports decrypting via KMS (happens under the hood).

Open to making edits if it needs massaging.

@bobf
Copy link
Owner

bobf commented Dec 16, 2021

Hi, @armiiller , I really like the look of this - thanks a lot for submitting the PR.

The only feedback I have here is:

  • Your changes have introduced a little bit of breakage in tests (only 2 tests failed, for the RSpec helpers - should hopefully be quite easy to fix)
  • Your code could be refactored into smaller, more digestible methods (e.g. the S3 key could be calculated in its own method)
  • It would be nice to see at least one test added for the new functionality (let me know if you need a hand with this)
  • Please run make test and ensure that everything passes - at the moment RSpec and Rubocop are both failing. Running rubocop -A auto-corrects the majority of issues so just a few remaining issues to resolve.

Overall though I am really happy with the quality of the work you've done here so I would have no problem merging it and putting out a new release if you could attend to the above points.

@armiiller
Copy link
Author

Thanks @bobf for the details reply - I'll work on adding your recommendations and put a mention to you here when done :)

bucket_name = receipt.dig("action", "bucketName")
object_key_prefix = receipt.dig("action", "objectKeyPrefix")
object_key = receipt.dig("action", "objectKey")
key = [object_key_prefix, object_key].compact_blank.join("/")
Copy link

Choose a reason for hiding this comment

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

why compact_blank and not just compact ?

@bobf
Copy link
Owner

bobf commented Oct 10, 2022

@armiiller Thanks for the fixes on this. How close do you think you are for being ready to merge ? Take as much time as you need, no rush !

@ACPK
Copy link

ACPK commented Feb 20, 2023

@bobf I saw your PR to the Rails core and am looking forward to it. In the meantime, how does this gem handle downloading emails via S3? I saw that this PR was closed: https://github.com/bobf/action_mailbox_amazon_ingress/pull/12/files

@bobf
Copy link
Owner

bobf commented Mar 10, 2023

@ACPK Sorry for the slow response - currently this gem does not provide that functionality. It's not something I personally need (I use the gem for a product at work and it doesn't use the S3 pipeline).

I don't have any immediate plans to port that functionality in the gem but, if the Rails PR ends up getting rejected, I will be more incentivised to do it then. Otherwise, PRs are welcome ! The code is all there in the Rails PR so at least there's a functional reference to work from.

@ssunday
Copy link
Contributor

ssunday commented Jun 24, 2024

@ACPK this gem now has that

I guess the decrypting from KMS can be introduced too.

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.

5 participants