Refactored Mandrill Signature Authentication #4

Merged
merged 4 commits into from Sep 17, 2013

Projects

None yet

2 participants

@zshannon
Contributor

Here's the alternate syntax I'm proposing. I think separating out the setting of the keys from the enforcement of request authenticity into a before_filter makes what's happening in the implementing controller much more readily apparent.

Contributor

Added 5ee943e to address a bug caused by Heroku passing HEAD requests on as GET requests. Should work the same way since Mandrill only uses POST for real webhook requests.

@tardate tardate added a commit that referenced this pull request Sep 17, 2013
@tardate tardate \#4 make before_filter authentication automatic and restricted to :cr…
…eate

* refactor and separate rails/non-rails concerns between Mandrill::Rails::WebHookProcessor and Mandrill::WebHook::Processor
3877548
@tardate tardate merged commit 5ee943e into evendis:master Sep 17, 2013

1 check failed

default The Travis CI build failed
Details
Owner
tardate commented Sep 17, 2013

@zshannon thanks again. I refactored further to make it a bit more automatic and separate some concerns:

  • before_filter setup automatically since I can't think of a good reason why you wouldn't want this
  • authentication filter is specifically scoped to :create so ignore the HEAD/GET issues
  • elevated the authentication algorithms to clas-level facility in Mandrill::WebHook::Processor so we avoid double instantiation and allow it to remain ignorant of Rails
  • make tests green
Contributor

@tardate nice work; those tests are extensive. I still think if the behavior is going to mutate the normal control flow (require a verified signature or exit) then the method should have an exclamation mark. It just makes things easier to understand at a glance and debug. See here: http://stackoverflow.com/questions/612189/why-are-exclamation-marks-used-in-ruby-methods

Perhaps one could call mandrill_webhook_keys! to both set the keys and include the before_filter?

Owner
tardate commented Sep 17, 2013

@zshannon I actually kept the bang! on :authenticate_mandrill_request! .. but just removed the need for the gem user to set it themselves in the controller. The before filter gets included automatically when you include the module.

But you might have a point about mandrill_webhook_keys - I was originally thinking of it as simply a config declaration so hence no exclamation mark.

Maybe this should be more active, like :authenticate_with_mandrill_keys! 'my_key', but here the bang! is a little unconventional in its usage - this method is a 'promise' to mutate the normal control flow, and not something that occurs when the line is executed. What do you think?

Contributor

@tardate I think adding the implementing user should see a bang somewhere in their controller so that they understand the normal control flow of is being modified by (at least) mandrill-rails. If there's only going to be one method call used by the implementer, then that call should terminate with a bang, as in your example authenticate_with_mandrill_keys! 'my_key', 'my_other_key'. I think the bang terminated option should aggregate setting the keys and adding the before_filter, and that the implementer should be able to select (for whatever unknowable reason) using the setter and/or the before_filter separately.

Owner
tardate commented Sep 18, 2013

PS: I've pushed this out as 1.0.0 now. Let me know it works OK for you?

Contributor

Just ran some tests on the 1.0.0 from rubygems and everything worked perfectly. Just pushed using the 1.0.0 gem to production. Great working with you!

Owner
tardate commented Sep 18, 2013

Cheers Zane, likewise!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment