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

Created Route and Client for Stripe Webhook. Created Stripe Signature… #102

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Agent123983
Copy link
Collaborator

…Validator, ProcessWebhookJob, and Storable Event (IssuingAuthorization).

…Validator, ProcessWebhookJob, and Storable Event (IssuingAuthorization).
@Agent123983 Agent123983 requested a review from Jnesselr December 9, 2024 21:53
parse_str(str_replace(',', '&', $signatureHeader), $parsedResult);

if (! array_key_exists('t', $parsedResult)) {
Log::info("Stripe webhook doesn't have timestamp");
Copy link
Member

Choose a reason for hiding this comment

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

We probably shouldn't be logging this like this. It'll just show up in the log as that one line with no context around. Maybe this as a code comment for the developer would be better?

return false;
}

if (! array_key_exists('v1', $parsedResult)) {
Copy link
Member

Choose a reason for hiding this comment

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

I noticed the key you're checking for is v1, but the log comment suggests it's the signature and the parameter you're checking below is signature. Looking at the stripe docs, it suggests that v1 is correct, but I'm wondering if we shouldn't use the built in libraries to validate this since it's more likely it'll be correct than if we do it all manually.


$timestamp = $parsedResult['t'];
$expectedSignature = $parsedResult['signature'];
$signed_payload = "$timestamp,'.',$payload";
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure this does what you want, based on reading the Stripe docs. If $timestamp was 1 and $payload was 2, then $signed_payload would be 1,'.',2 instead of what I think should be 1.2. I suspect what you want is $signed_payload = $timestamp . '.' . $payload;

case 'issuing_authorization.created':
event(new IssuingAuthorization($payload));
// Not Sure what to do to update an authorization if its already been created
// Would we just add it to the db as another event and handle it somewhere else?
Copy link
Member

Choose a reason for hiding this comment

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

More or less, yeah. Depending on what all we want to do with this, it might make sense to talk about an aggregate. For example, we do that for each denhac membership since we need to process events and then make decisions about what to do next.

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.

2 participants