Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Example generate signature #913

Closed
wants to merge 1 commit into from
Closed

Conversation

sandstrom
Copy link

@sandstrom sandstrom commented Apr 22, 2020

Spike to illustrate how one could make signature generation easier. See #912

@sandstrom sandstrom mentioned this pull request Apr 22, 2020
@remi-stripe
Copy link
Contributor

Thanks for the PR @sandstrom! It looks like tests are broken, could you look into fixing this? Assigning to @brandur-stripe to help review and merge.

@sandstrom
Copy link
Author

hi @remi-stripe, please have a look at #912 first. This PR is just an example to illustrate my thinking.

@brandur-stripe
Copy link
Contributor

hi @remi-stripe, please have a look at #912 first. This PR is just an example to illustrate my thinking.

@sandstrom Yep, gotcha. I'll be taking a look at this one soon!

@brandur-stripe
Copy link
Contributor

brandur-stripe commented Apr 23, 2020

@sandstrom Looks largely good. Do you think you take a look at a couple things? (There are quite a few words here, but all these points are pretty simple.)

  1. Foremost, I think we should change the API slightly to take in a timestamp instead of a full header like compute_signature(time, payload, secret). Not only does this match what we have in Go, but it also allows the function to be called in a more granular way. Instead of the caller needing to have a preconstructed Stripe header, all they need is their payload, a signing secret, and the current time — the method takes care of building the header for them.
  2. Given it's becoming a public API, I think it makes sense to add a basic test in test/stripe/webhook_test.rb to make sure it works. It's already being tested roundaboutedly by other cases in there, but it'd be nice to have a direct one for compute_signature specifically in case we want to exercise just this one method.
  3. The test suite is failing in your PR. This is because although compute_signature is not public, we do cheat a bit by calling it from the test suite with send. We'll need to update the invocation to account for the new signature.

Anyway, thanks for sending us a PR! Looking good.

@sandstrom
Copy link
Author

@brandur-stripe Thanks for having a look!

I wrote the PR mostly to illustrate my thinking in the issue I opened. Since I'm not paid to work on the Stripe product I didn't really envision doing the work (sorry).

But hopefully the idea and issue itself is useful, and the code example in this PR helped illustrate my point in the issue.

If you think it's a good idea, it would be awesome if you'd make this improvement!

Since signature verification is important for security, it's quite helpful if we can easily generate signatures and use them in tests, to ensure that our code is verifying signatures correctly. So it's a security win.

Anyway, overall happy with Stripe with our without this improvement. Have a good weekend!

@brandur-stripe
Copy link
Contributor

Okay, closing this out.

@sandstrom
Copy link
Author

sandstrom commented Apr 24, 2020

thanks @brandur-stripe! Have a great weekend! 🌞

(fixed in #915)

@sandstrom sandstrom deleted the patch-1 branch April 24, 2020 21:18
@brandur-stripe
Copy link
Contributor

Thanks @sandstrom. You too!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants