search

integrallis / stripe_event

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

Basic authentication support #55

Closed beccadax closed 9 years ago

beccadax commented 9 years ago

Pursuant to #53, this branch adds basic authentication support to StripeEvent.

Basic authentication is supported in the simplest way possible:

  1. The module's user sets a StripeEvent.authentication_secret attribute to their desired password.
  2. If authentication_secret is set, a new before_action in WebhookController ensures that the request is appropriately authenticated, and returns a 401 otherwise.
  3. If authentication_secret is not set, the module behaves as it does currently. All previously-existing tests pass unmodified.

This branch does not add a default way to determine the secret, a deprecation warning if the user doesn't specify a secret, or anything else of that sort. I'm by no means opposed to such things, but since this is my first pull request, I don't feel qualified to make those sorts of design decisions. It does, however, include new tests to ensure this feature is working as intended and a section in the README explaining how to enable it.

Suggestions welcome.

coveralls commented 9 years ago

Coverage Status

Coverage remained the same at 100.0% when pulling dc0f9625c582d2eb4734de0d83f0a784fae3f172 on brentdax:feature/basic-auth into 742fd9fc4ec11ee51b46a6634e69ec262b021792 on integrallis:master.

coveralls commented 9 years ago

Coverage Status

Coverage remained the same at 100.0% when pulling 15029c87311156a6d6e16fd30483a2fad45b11a8 on brentdax:feature/basic-auth into 742fd9fc4ec11ee51b46a6634e69ec262b021792 on integrallis:master.

coveralls commented 9 years ago

Coverage Status

Coverage remained the same at 100.0% when pulling 15029c87311156a6d6e16fd30483a2fad45b11a8 on brentdax:feature/basic-auth into 742fd9fc4ec11ee51b46a6634e69ec262b021792 on integrallis:master.

coveralls commented 9 years ago

Coverage Status

Coverage remained the same at 100.0% when pulling 15029c87311156a6d6e16fd30483a2fad45b11a8 on brentdax:feature/basic-auth into 742fd9fc4ec11ee51b46a6634e69ec262b021792 on integrallis:master.

coveralls commented 9 years ago

Coverage Status

Coverage remained the same at 100.0% when pulling ee3a4174750c0ae6dbcc4058122830e733a6173e on brentdax:feature/basic-auth into 742fd9fc4ec11ee51b46a6634e69ec262b021792 on integrallis:master.

invisiblefunnel commented 9 years ago

This is a great start! Thanks for putting it together.

@rmm5t @peterkeen what do you think about specifying a secret vs. authenticate_with_http_basic?

rmm5t commented 9 years ago

@rmm5t @peterkeen what do you think about specifying a secret vs. authenticate_with_http_basic?

Good question. authenticate_with_http_basic certainly gives more flexibility, but I'm not sure what flexibility buys us here. Let's go for simplicity instead and take an opinionated approach about how the replay attack prevention should be handled. SSL/TLS + BasicAuth-Secret == Secure

If we don't keep this simple, it's less likely that people will implement this protective feature. I think that's more important.

Afterall, if we realize this a limitation, it's easier to add more flexibility and maintain the simpler backwards compatibility than remove it later.

invisiblefunnel commented 9 years ago

@rmm5t very good point about simplicity for the end user. Let's proceed with the authentication_secret approach. Things are coming together!

coveralls commented 9 years ago

Coverage Status

Coverage remained the same at 100.0% when pulling bed6d16b72743770b374147da5a2f6faf83fcff1 on brentdax:feature/basic-auth into 742fd9fc4ec11ee51b46a6634e69ec262b021792 on integrallis:master.

coveralls commented 9 years ago

Coverage Status

Coverage remained the same at 100.0% when pulling bed6d16b72743770b374147da5a2f6faf83fcff1 on brentdax:feature/basic-auth into 742fd9fc4ec11ee51b46a6634e69ec262b021792 on integrallis:master.

rmm5t commented 9 years ago

I think we're close enough to get this merged in. Any objections?

invisiblefunnel commented 9 years ago

Looks good to me. Thank you @brentdax @rmm5t! @rmm5t I'd also like to add you as an author in the gemspec, sound good?

rmm5t commented 9 years ago

Sweet. Merging in 3...2...1

Great work @brentdax! Thanks for taking the lead on getting this PR going.

@invisiblefunnel Sure thing. I'm glad to handle the version bump and release too if you want to grant gem push rights. My rubygems email is the same as on my github profile. I'll be securing my webhook endpoints to use the new auth secret soon after.

peterkeen commented 9 years ago

Sorry I totally dropped off this thread, but I think this is a great addition to the project. Thanks @brentdax, @rmm5t, and @invisiblefunnel

invisiblefunnel commented 9 years ago

@rmm5t you're all set.

Thanks again everyone!

rmm5t commented 9 years ago

v1.5.0 is in the wild. :beers:

invisiblefunnel commented 9 years ago

Cheers!

beccadax commented 9 years ago

Yeah, I had to update my Gemfile twice in the span of a couple hours as the pull request was merged and then released. Thanks for working with me on this!