Add Amazon SES/SNS ingress to ActionMailbox

[bobf]

Summary

Provides an ingress for Amazon SES/SNS.

Includes:

• /rails/action_mailbox/amazon/inbound_emails ingress route
• Signature verification
• Auto-subscribe
• Permitted SNS topic configuration

Tests for all of the above are included.

Other Information

This commit (f480cfa) from @georgeclaghorn states that SES integration was removed and will be re-written for 6.1.

I needed this sooner so I wrote a gem here: https://github.com/bobf/action_mailbox_amazon_ingress

I emailed George to discuss and he suggested a PR. I have adapted the tests from RSpec to Minitest.

Please let me know if any adjustments are needed.

[bobf]

I've just deployed an app running Rails from this branch and set up an SES/SNS topic. Confirmation and receipt both appear to be working fine.

[georgeclaghorn]

Thanks for the PR! Here’s the first round of feedback.

[georgeclaghorn]

Do we need to parse the request body manually, or can we rely on Rails parsing into params based on Content-Type?

[bobf]

The reason for this is that the signature verification requires the exact structure provided in the post body. The params object contains extra parameters (controller, action) that invalidate the signature. I opted to use the same object in all places rather than mixing params and my parsed object.

Is there a way to extract just the post body JSON-extracted params from the params object ?

[kaspth]

params.except(:controller, :action) think that would be clearer too.

[bobf]

@kaspth I considered this but it seems quite flaky to me. If any extra "special" parameters are added to Rails then this code will need to be updated. Since the SNS verifier depends on the exact input parameters included in the POST body, this [my implementation] seems the most correct way to do it, albeit with an extra JSON.parse.

I'm happy to be overruled !

[kaspth]

Don't recall us having added any extra default params in the last …10 years? My memory is a bit spotty beyond that, but don't think it's happened. I think the clarity of not sidestepping the default params parsing is overall better.

Is this something we could exercise in a test? Perhaps even with just a no-op action with render json: params and does assert_empty params.except(:controller, :action)?

[bobf]

Thanks for the PR! Here’s the first round of feedback.

Thanks a lot for the feedback. I have addressed all of your points. I think there is only one item outstanding (pending your feedback).

Let me know if you prefer squashed commits when the PR is ready.

[kaspth]

I still like @georgeclaghorn's idea of a separate ConfirmationsController. I also think that would force us to consider a model extraction.

Here's my take on that.

# frozen_string_literal: true

module ActionMailbox
  module Ingresses
    class Amazon::BaseController < ActionMailbox::BaseController
      before_action :set_notification

      private
        def set_notification
          @notification = SNSNotification.new params.except(:controller, :action)
        end

        class SNSNotification
          def initialize(params)
            @params = params
          end

          def subscription_confirmed?
            Net::HTTP.get_response(URI(params[:SubscribeURL])).code&.start_with?("2")
          end

          def verified?
            require "aws-sdk-sns"
            Aws::SNS::MessageVerifier.new.authentic?(params.to_json)
          end

          def topic
            params[:TopicArn]
          end

          def message_content
            params.dig(:Message, :content) if receipt?
          end

          private
            attr_reader :params

            def receipt?
              params[:Type] == "Notification" || params[:notificationType] == "Received"
            end
        end
    end

    class Amazon::ConfirmationsController < Amazon::BaseController
      def create
        if @notification.subscription_confirmed?
          head :ok
        else
          Rails.logger.error "SNS subscription confirmation request rejected."
          head :unprocessable_entity
        end
      end
    end

    class Amazon::InboundEmailsController < Amazon::BaseController
      before_action :ensure_message_content, :ensure_valid_topic, :ensure_verified

      def create
        ActionMailbox::InboundEmail.create_and_extract_message_id! @notification.message_content
      end

      private
        def ensure_message_content
          head :bad_request if @notification.message_content.blank?  
        end

        def ensure_valid_topic
          unless @notification.topic.in? Array(ActionMailbox.amazon.subscribed_topics)
            Rails.logger.warn "Ignoring unknown topic: #{@notification.topic}"
            head :unauthorized
          end
        end

        def ensure_verified
          head :unathorized unless @notification.verified?
        end
    end
  end
end

[bobf]

I still like @georgeclaghorn's idea of a separate ConfirmationsController. I also think that would force us to consider a model extraction.

Here's my take on that.

@kaspth Thanks - agree this is much nicer. One minor point is that I think @georgeclaghorn suggested separate actions and not necessarily controllers (unless I have missed a message).

I think this is better either way but wanted to highlight just in case this made any difference.

I'll update to reflect your suggested changes tomorrow if there's no further discussion needed.

[kaspth]

One minor point is that I think @georgeclaghorn suggested separate actions and not necessarily controllers (unless I have missed a message).

George suggested a separate controller in #39364 (comment) that's where I got the name from 😄

[georgeclaghorn]

Sorry, I think “the different actions” in my comment may have confused. I meant “the two different actions in the separate controllers.”

[bobf]

@kaspth @georgeclaghorn Thanks both for the feedback. I have adapted Kasper's refactor and addressed a couple of issues. A couple of things I think worth noting:

• The Message param in the POST body is a JSON string that must be manually decoded. i.e. it is double-encoded. It contains the original email event data structure.
• ActionMailbox::BaseController#ingress_name (called by #ensure_configured) does not expect a controller that is not named InboundEmailsController so I have defined ActionMailbox::Ingresses::Amazon::BaseController#ingress_name which just returns :amazon. This feels kludgey but I didn't want to modify the base controller method. (Otherwise subscription confirmations fail).

Let me know if any changes are needed.

[mwesigwa]

@bobf, I'd suggest adding some form of documentation, alerting users not to enable the raw_message_delivery option when using this Ingress. I found out the hard way after spending a couple of hours trying to get this solution to work. When you enable raw message delivery for an Amazon SQS endpoint, any Amazon SNS metadata is stripped from the published message and the message is sent as is which results in the confounding error saying the topic is being ignored since it does not exist in the sent message.

Here is the Amazon documntation link that finally explained this.

[bobf]

@mwesigwa Thanks for the idea - I was tempted to select this option myself so I'm sure others will as well.

I've updated with this commit: 809f53b

Let me know if you think it needs adjusting !

[mwesigwa]

The change is great! For what it's worth, I can confirm that the code worked as expected 🥇. Thank you for implementing this.

[joshkinabrew]

Really looking forward to this being released! Thanks for your great work @bobf

[bobf]

@georgeclaghorn Is there anything here that needs changing ? I have a conflict with the main branch but would rather wait until I had some feedback before fixing in case this gets left out in the cold.

The conflict is only tiny in this case so will be no trouble to resolve but some of the tests (e.g. routing) can be a bit of a pain to deal with if things start to diverge. Let me know how you want to proceed.

[ssunday]

@zzak Are you able to see exactly what the error is for https://github.com/rails/rails/actions/runs/8265465928/job/22611174061?pr=39364 ? I'm not familiar with how to find the exact logs/issue is. I tried but found zilch in terms of trace.

I wonder if this does belong with aws-sdk-rails rather than Rails itself 🤔 What determines what belongs inside Rails vs. not?

Thanks by the way!

[ssunday]

based on this on the storage s3 service

[ssunday]

I removed this since it seemed like it wasn't being done elsewhere with these type of requirements

[bobf]

@ssunday Thanks again for picking this up, and apologies for the drive-by collab invite - saw that you were active and blocked by what looked like a very unamusing PR so wanted to unblock ASAP but was in the middle of a crisis at work so could only spare a few seconds to do the bare minimum. Looking forward to getting this merged, wherever it ends up !

[ssunday]

@bobf Oh jeez, no worries, you gave me what I needed, haha. Thank you! Also eager to get this merged too.

[ssunday]

I moved the non-controller files to lib and lazy-loaded the file so rails doesn't melt on launch if the aws-sdk-sns is not part of the bundle (which was what was causing the rails-new-docker step to fail a lot).

I haven't ever tested this code in the wild to make sure it works e2e, assuming test and previous work covers that.

Added myself as part of the authors for this work 😂

[ssunday]

Failure looks flaky, when I rebase next I assume it will resolve.

[ssunday]

@bobf Okay, I have confirmed this is working in the real world haha (without doing a rebase). Can you re-request review from folks? I can figure out how to contact people if not.

[bobf]

@zzak I don't see your name in the reviewers list, are you the right person to pass this to ?

@ssunday Thanks again for the work on this. 🙏

[ssunday]

@bobf No problem!

For reviewers: the CI failure definitely looks flaky/unrelated. I can rebase if everything else looks OK.

[rafaelfranca]

Thank you for the pull request and sorry for the delay on reaching a decision.

After talking with other members of the Rails Core, I decided to not accept this pull request. We don't use Amazon SES/SNS in our applications and I don't feel we are the best maintainers for this code. This could live in a plugin for Rails like the one the author of this pull request already published..

We are happy to improve the system to allow plugins like those to exist if it isn't possible right now, but looking at the linked library it seems it already is.

Thank you again for you contributions to the community.

[bobf]

Just under a week short of the pull request's 4th birthday. :)

Thanks for considering the PR and the explanation, I understand you have to limit inflation to keep the framework manageable. ActionMailbox is indeed quite well suited for extending through plugins so I'm happy to go that route.

Have a nice week !

[mullermp]

@bobf I'd love to get this into aws-sdk-rails. Would you like to open a PR and I can help shepherd that in?

[bobf]

@mullermp I think that would be a sensible path forward here, and I have no objections at all.

Maybe pick this up in this conversation: bobf/action_mailbox_amazon_ingress#14 (comment) - @ssunday has access to that repository so I believe would be able to make PRs, but I'm happy to help if needed, just might be a little slow to respond this week so trying to prevent myself being a blocker.

Thanks for your interest and enthusiasm, it sounds like ActionMailbox SES integration will find a good home. :)