search

integrallis / stripe_event

Stripe webhook integration for Rails applications.
https://rubygems.org/gems/stripe_event
MIT License
841 stars 107 forks source link

add ability to trust incoming event without additional request to stripe #67

Closed gingray closed 8 years ago

gingray commented 8 years ago

I think its good idea do not make additional request to stripe API if you want to trust incoming events. Default behavior is same as was but i've add option like trust incoming stripe events also i've add tests for this. If you have any suggestions plz let me know hope you find this changes is useful. Thanks.

coveralls commented 8 years ago

Coverage Status

Changes Unknown when pulling 132b96ee8d3e653d61607325a6a80e9b4d39e23d on gingray:feature/trust_incoming_event into \ on integrallis:master**.

rmm5t commented 8 years ago

First, this is a very clean PR. Thank you for that.

However, additional options like this introduce unnecessary liability for us, because we have to continue to maintain it over the long-haul. Thank you for the effort, but I vote to reject this feature request.

  1. You really should never trust incoming events.
  2. Even if you wanted to trust incoming events, the gem already has support for that by simply overriding the event_retriever. For example:
StripeEvent.configure do |events|
  events.event_retriever = lambda { |params| Stripe::Event.construct_from(params.deep_symbolize_keys) }
end

While I realize this feature request is trying to introduce a facilitating option to do the exact same thing, I actually prefer that enabling trusted incoming events requires adding a bit of syntactic vinegar to a project. Because it is generally considered bad practice to trust these incoming events, there should be some minor pain if you don't want to heed Stripe's recommended best practices.

gingray commented 8 years ago

@rmm5t thanks for your comment that make sense. During writing pull request i can suggest minor improvements like using

  routes { StripeEvent::Engine.routes }

instead of 

post :event, params.merge(use_route: :stripe_event)

also move restore state to a shared context and fix path bug in require support.

rmm5t commented 8 years ago

@gingray Personally, I'd be willing to review separate (smaller) pull-requests for the two refactors that you mentioned.

The routes change sounds good.

I'm not a huge fan of rspec shared contexts in general (I prefer straight ruby over rspec DSL magic), and given this codebase, I don't think the shared context adds much value over the configured before and after blocks, but if others feel strongly about it, it doesn't hurt my head that much.