evendis / mandrill-rails

Webhook processing and event decoration to make using Mandrill with Rails just that much easier
MIT License
288 stars 36 forks source link

Refactored Mandrill Signature Authentication #4

Closed zshannon closed 10 years ago

zshannon commented 10 years ago

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.

zshannon commented 10 years ago

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 commented 10 years ago

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

zshannon commented 10 years ago

@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?

tardate commented 10 years ago

@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?

zshannon commented 10 years ago

@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.

tardate commented 10 years ago

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

zshannon commented 10 years ago

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!

tardate commented 10 years ago

Cheers Zane, likewise!