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

added webhook verification to mux_ruby + helper-func infrastructure #46

Open
wants to merge 14 commits into
base: master
Choose a base branch
from

Conversation

eropple
Copy link
Contributor

@eropple eropple commented Mar 18, 2022

  • added webhook verification to Ruby SDK
  • added unit tests for happy and sad paths
  • deleted all the old stub unit tests
  • unit tests (real ones) now run in CI
  • updated build system to copy lib-manual to lib on build (has to be done this way because OAS won't remove old files for renamed/deleted modules, so we have to delete lib every build)

@eropple eropple requested review from mmcc and a team and removed request for mmcc March 18, 2022 15:34
Copy link

@mmcc mmcc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewing this took me way back, but I think this looks good to me! I had a couple of questions mostly about naming, but functionally this lgtm.

lib-manual/helpers/webhook_verifier.rb Show resolved Hide resolved
# @param [Array<Symbol>] header_schemes the list of accepted header schemes for this verifier
def initialize(secret: nil, tolerance: DEFAULT_TOLERANCE, header_schemes: [:v1])
raise "secret '#{secret.inspect}' must be a String" unless secret.is_a?(String)
raise "tolerance '#{tolerance.inspect}' must be a positive number." \
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure we use tolerance anywhere else, typically it's expiration. Why the change here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's tolerance in the node SDK.

# @param [String] header the Mux-Signature header
# @param [Time] current_timestamp (for test purposes) the current time expected for this webhook (defaults to `Time.utc`)
# @return [Boolean] true if webhook is verified; false otherwise
def verify(request_body:, header:, current_timestamp: Time.now.getutc)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't remember this from Ruby land circa 2013, but is arg:, just specifying a nullish default value?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It specifies a named argument rather than a positional argument. I think they're clearer.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mmcc this must have come after your Ruby time (it was right near the end of my Ruby time) -- keyword args, which are a fantastic language feature:

https://thoughtbot.com/blog/ruby-2-keyword-arguments

In this case calling verify without request_body: and header: args would cause a runtime error.

In the past we would sometimes use an options hash and then have conditionals inside method to check for things that were passed into the options hash.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, this just replaces the options hash.

@dylanjha , I didn't know you were a Ruby guy. Oh, do I have code for you to review...!

@dylanjha
Copy link
Contributor

Just dropping in here to note that we have wanted this for so long. Thanks @eropple for coming up with a pattern to pull in ad-hoc helper functionality into generated SDKs ❤️

@eropple
Copy link
Contributor Author

eropple commented Mar 18, 2022

Thanks, @dylanjha . I got more of these to do. We'll get there. 😎

@eropple eropple removed the request for review from a team March 22, 2022 17:25
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.

3 participants