diff --git a/app/controllers/stripe/webhooks_controller.rb b/app/controllers/stripe/webhooks_controller.rb index c80dc500ae..8ada0361b6 100644 --- a/app/controllers/stripe/webhooks_controller.rb +++ b/app/controllers/stripe/webhooks_controller.rb @@ -3,26 +3,35 @@ require 'stripe/webhook_handler' module Stripe class WebhooksController < BaseController protect_from_forgery except: :create + before_filter :verify_webhook # POST /stripe/webhook def create - # TODO is there a sensible way to confirm this webhook call is actually from Stripe? - event = Event.construct_from(params) - handler = WebhookHandler.new(event) + handler = WebhookHandler.new(@event) result = handler.handle - render nothing: true, status: status_mappings[result] + render nothing: true, status: status_mappings[result] || 200 end private + def verify_webhook + payload = request.raw_post + signature = request.headers["HTTP_STRIPE_SIGNATURE"] + @event = Webhook.construct_event(payload, signature, Stripe.endpoint_secret) + rescue JSON::ParserError + render nothing: true, status: 400 + rescue Stripe::SignatureVerificationError + render nothing: true, status: 401 + end + # Stripe interprets a 4xx or 3xx response as a failure to receive the webhook, - # and will stop sending events if too many of these are returned. + # and will stop sending events if too many of either of these are returned. def status_mappings { - success: 200, - failure: 202, - silent_fail: 204 + success: 200, # The event was handled successfully + unknown: 202, # The event was of an unknown type + ignored: 204 # No action was taken in response to the event } end end diff --git a/config/application.yml.example b/config/application.yml.example index e10402fd79..1d8bf45f2d 100644 --- a/config/application.yml.example +++ b/config/application.yml.example @@ -29,8 +29,9 @@ CURRENCY: AUD # Stripe Connect details for instance account # Find these under 'API keys' and 'Connect' in your Stripe account dashboard -> Account Settings -# Under 'Connect', the Redirect URI should be set to https://YOUR_SERVER_URL/stripe/callback (e.g. https://openfoodnetwork.org.uk/stripe/connect) +# Under 'Connect', the Redirect URI should be set to https://YOUR_SERVER_URL/admin/stripe_accounts/connect_callback (e.g. https://openfoodnetwork.org.uk/admin/stripe_accounts/connect_callback) # Under 'Webhooks', you should set up a Connect endpoint pointing to https://YOUR_SERVER_URL/stripe/webhooks e.g. (https://openfoodnetwork.org.uk/stripe/webhooks) # STRIPE_INSTANCE_SECRET_KEY: "sk_test_xxxxxx" # This can be a test key or a live key # STRIPE_INSTANCE_PUBLISHABLE_KEY: "pk_test_xxxx" # This can be a test key or a live key # STRIPE_CLIENT_ID: "ca_xxxx" # This can be a development ID or a production ID +# STRIPE_ENDPOINT_SECRET: "whsec_xxxx" diff --git a/config/initializers/stripe.rb b/config/initializers/stripe.rb index c7ec6599d8..8237b0dcab 100644 --- a/config/initializers/stripe.rb +++ b/config/initializers/stripe.rb @@ -1,13 +1,14 @@ -# Add the :publishable_key property, to allow us to access this -# property from the object, rather than calling from ENV directly. +# Add some additional properties, to allow us to access these +# properties from the object, rather than calling from ENV directly. # This is mostly useful for stubbing when testing, but also feels # a bit cleaner than accessing keys in different ways. module Stripe class << self - attr_accessor :publishable_key + attr_accessor :publishable_key, :endpoint_secret end end Stripe.api_key = ENV['STRIPE_INSTANCE_SECRET_KEY'] Stripe.publishable_key = ENV['STRIPE_INSTANCE_PUBLISHABLE_KEY'] Stripe.client_id = ENV['STRIPE_CLIENT_ID'] +Stripe.endpoint_secret = ENV['STRIPE_ENDPOINT_SECRET'] diff --git a/spec/controllers/stripe/webhooks_controller_spec.rb b/spec/controllers/stripe/webhooks_controller_spec.rb index b415b6b865..40ec3c625b 100644 --- a/spec/controllers/stripe/webhooks_controller_spec.rb +++ b/spec/controllers/stripe/webhooks_controller_spec.rb @@ -13,34 +13,62 @@ describe Stripe::WebhooksController do } end - context "when an event with an unknown type is received" do - it "responds with a 202" do + context "when invalid json is provided" do + before do + allow(Stripe::Webhook).to receive(:construct_event).and_raise JSON::ParserError, "parsing failed" + end + + it "responds with a 400" do post 'create', params - expect(response.status).to eq 202 + expect(response.status).to eq 400 end end - describe "when an account.application.deauthorized event is received" do - let!(:stripe_account) { create(:stripe_account, stripe_user_id: "webhook_id") } + context "when event signature verification fails" do before do - params["type"] = "account.application.deauthorized" + allow(Stripe::Webhook).to receive(:construct_event).and_raise Stripe::SignatureVerificationError.new("verfication failed", "header") end - context "when the stripe_account id on the event does not match any known accounts" do - it "doesn't delete any Stripe accounts, responds with 204" do + it "responds with a 401" do + post 'create', params + expect(response.status).to eq 401 + end + end + + context "when event signature verification succeeds" do + before do + allow(Stripe::Webhook).to receive(:construct_event) { Stripe::Event.construct_from(params) } + end + + context "when an event with an unknown type is received" do + it "responds with a 202" do post 'create', params - expect(response.status).to eq 204 - expect(StripeAccount.all).to include stripe_account + expect(response.status).to eq 202 end end - context "when the stripe_account id on the event matches a known account" do - before { params["account"] = "webhook_id" } + describe "when an account.application.deauthorized event is received" do + let!(:stripe_account) { create(:stripe_account, stripe_user_id: "webhook_id") } + before do + params["type"] = "account.application.deauthorized" + end - it "deletes Stripe accounts in response to a webhook" do - post 'create', params - expect(response.status).to eq 200 - expect(StripeAccount.all).not_to include stripe_account + context "when the stripe_account id on the event does not match any known accounts" do + it "doesn't delete any Stripe accounts, responds with 204" do + post 'create', params + expect(response.status).to eq 204 + expect(StripeAccount.all).to include stripe_account + end + end + + context "when the stripe_account id on the event matches a known account" do + before { params["account"] = "webhook_id" } + + it "deletes Stripe accounts in response to a webhook" do + post 'create', params + expect(response.status).to eq 200 + expect(StripeAccount.all).not_to include stripe_account + end end end end