Webhook Signature Verification

[mjc-gh]

Closes #83.

This to do for this PR:

• [x] Update the README
• [x] Get my specs running and add a few for this feature

[coveralls]

Coverage decreased (-3.0%) to 96.269% when pulling 546e4545a5bac5832271f6c38d8ae61c63504cfb on mikeycgto:webhook-signature-verification into 7e5ed1e8cb15c2cef38b17ad7dc8d5664782a9eb on integrallis:master.

[coveralls]

Coverage decreased (-3.0%) to 96.269% when pulling 546e4545a5bac5832271f6c38d8ae61c63504cfb on mikeycgto:webhook-signature-verification into 7e5ed1e8cb15c2cef38b17ad7dc8d5664782a9eb on integrallis:master.

[coveralls]

Coverage decreased (-3.0%) to 96.269% when pulling 546e4545a5bac5832271f6c38d8ae61c63504cfb on mikeycgto:webhook-signature-verification into 7e5ed1e8cb15c2cef38b17ad7dc8d5664782a9eb on integrallis:master.

[rmm5t]

• Add deprecation errors and/or warnings for removal of StripeEvent.authentication_secret

Let's keep things focused. Please don't remove or or change the behavior of the authentication_secret as part of this pull-request. Let's only consider deprecating it after we get signature verification fully working and released. The deprecation deserves to be part of a separate PR later.

[mjc-gh]

Sounds good. Had only included it in this PR cause the original issue mentioned that authentication_secret can be deprecated. I will remove that portion of the PR and keep both features in place.

Need to get the specs running locally so I can add some for this feature. Was having bundler version issues when I tried to run them initially.

[coveralls]

Coverage decreased (-0.7%) to 98.571% when pulling d2dbb83212fb66e17e48e5a977c3cb2ef111fca9 on mikeycgto:webhook-signature-verification into 7e5ed1e8cb15c2cef38b17ad7dc8d5664782a9eb on integrallis:master.

[coveralls]

Coverage decreased (-0.7%) to 98.571% when pulling 4b59c76f2deb776336737a1e60c5487eb085cd91 on mikeycgto:webhook-signature-verification into 7e5ed1e8cb15c2cef38b17ad7dc8d5664782a9eb on integrallis:master.

[rmm5t]

Thanks for making the adjustments to this PR. Looks clean. Have you tried this branch against a live Stripe endpoint yet?

My only critique is that I'd prefer the tests to pass an actual HMAC signature header instead of mocking the call to Stripe::Webhook::Signature.verify_header (I prefer a classical style of testing where possible vs a mockist style), but there's no need for that to hold up this PR.

I can merge and release a new version later today if no one else objects.

Good work @mikeycgto!

[mjc-gh]

Thanks!

Yeah I do agree with you and prefer to avoid mocking when possible. My concern there was the specs would become to specific to actual the HMAC implementation in Stripe. Figured mocks would just be quicker to write and more future-proof if their verification process changes (assuming the API doesn't change :smiley:).

[brandur]

@rmm5t @mikeycgto Nice! I'd +1 trying to go a level deeper than the mock if possible. What you can do in this case is to call Stripe::Webhook::Signature.compute_signature(payload, secret) to generate a valid signature before validating it.

That way you can test more deeply than the mock and you don't have to rely on any particular crypto implementation — if we update to something beyond HMAC-SHA256, we'll be updating .compute_signature as well .verify_header within stripe-ruby.

[mjc-gh]

@brandur this was more involved than I thought it would be (have to include a timestamp and version scheme). I do agree it's better to use real signatures here though. Thanks for the feedback!

[coveralls]

Coverage decreased (-0.7%) to 98.571% when pulling 806c572c2ea187eda99372c56712ba96e261afbf on mikeycgto:webhook-signature-verification into 7e5ed1e8cb15c2cef38b17ad7dc8d5664782a9eb on integrallis:master.

[mjc-gh]

Refactored the specs some to hopefully make more sense. Thanks again for the feedback!

[coveralls]

Coverage decreased (-0.7%) to 98.571% when pulling 8b53943986708f10d78a220eeac72bba53d6e831 on mikeycgto:webhook-signature-verification into 7e5ed1e8cb15c2cef38b17ad7dc8d5664782a9eb on integrallis:master.

[coveralls]

Coverage decreased (-0.7%) to 98.571% when pulling 8b53943986708f10d78a220eeac72bba53d6e831 on mikeycgto:webhook-signature-verification into 7e5ed1e8cb15c2cef38b17ad7dc8d5664782a9eb on integrallis:master.

[coveralls]

Coverage decreased (-0.7%) to 98.571% when pulling 8b53943986708f10d78a220eeac72bba53d6e831 on mikeycgto:webhook-signature-verification into 7e5ed1e8cb15c2cef38b17ad7dc8d5664782a9eb on integrallis:master.

[coveralls]

Coverage decreased (-0.7%) to 98.571% when pulling 01ffaa80c336960ead0d51d866e21a4354006b26 on mikeycgto:webhook-signature-verification into 6fdee504ed780ffd522d18753d270bfab9ba6c44 on integrallis:master.

[rmm5t]

@mikeycgto Looks good to me. Are you done with your refactoring? I'm happy to merge now when ready.

[mjc-gh]

I'm good with merging. Thanks.