-
Notifications
You must be signed in to change notification settings - Fork 520
Add WebhookSignature.generate_test_header_string helper #1810
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
base: master
Are you sure you want to change the base?
Changes from all commits
e0a7ca4
44e57c7
fa9d9dc
4644bab
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,6 +3,7 @@ | |
| import time | ||
| from collections import OrderedDict | ||
| from hashlib import sha256 | ||
| from typing import Optional | ||
|
|
||
| # Used for global variables | ||
| import stripe # noqa: IMP101 | ||
|
|
@@ -39,6 +40,41 @@ def construct_event( | |
| ) | ||
| return event | ||
|
|
||
| @staticmethod | ||
| def generate_test_header_string( | ||
| payload: str, | ||
| secret: str, | ||
| timestamp: Optional[int] = None, | ||
| scheme: Optional[str] = None, | ||
| signature: Optional[str] = None, | ||
| ) -> str: | ||
| """ | ||
| Generates a value for the `Stripe-Signature` header that can be used | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The docstring uses a mix of markdown and RST syntax. I would suggest sticking to markdown to be consistent with the rest of the code base. You can specify parameters as:
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. switched to a markdown |
||
| when testing code that calls `Webhook.construct_event` or | ||
| `WebhookSignature.verify_header`. Mirrors `generateTestHeaderString` | ||
| from stripe-node. | ||
|
|
||
| Args: | ||
| payload: The webhook payload to sign, as a string. | ||
| secret: The webhook signing secret (`whsec_...`). | ||
| timestamp: Unix timestamp to embed in the header. Defaults to | ||
| the current time. | ||
| scheme: Signature scheme. Defaults to | ||
| `WebhookSignature.EXPECTED_SCHEME`. | ||
| signature: Pre-computed signature to embed in the header. If | ||
| omitted, a signature is computed from `payload` and `secret`. | ||
| """ | ||
| if timestamp is None: | ||
| timestamp = int(time.time()) | ||
| if scheme is None: | ||
| scheme = WebhookSignature.EXPECTED_SCHEME | ||
| if signature is None: | ||
| signed_payload = "%d.%s" % (timestamp, payload) | ||
| signature = WebhookSignature._compute_signature( | ||
| signed_payload, secret | ||
| ) | ||
| return "t=%d,%s=%s" % (timestamp, scheme, signature) | ||
|
|
||
|
|
||
| class WebhookSignature(object): | ||
| EXPECTED_SCHEME = "v1" | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question: Did you consider
Webhook.generate_test_header_string? stripe-node puts the helper on the primarywebhooksobject. I'm curious why you choseWebhookSignatureinstead ofWebhookThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right that
Webhookis the better home. My original reasoning was internal cohesion (co-locating with_compute_signatureandverify_header), but that's outweighed by:Webhook.construct_eventin tests; sitting next to it is a stronger signal than sitting next to the primitive it wraps.Webhookis the user-facing entry point;WebhookSignatureis closer to plumbing. A testing helper belongs on the surface being tested.Moved it to
Webhook.generate_test_header_string, with the implementation still delegating toWebhookSignature._compute_signatureinternally. Tests updated accordingly. Thanks for pushing back on this.