search

dj-stripe / dj-stripe

dj-stripe automatically syncs your Stripe Data to your local database as pre-implemented Django Models allowing you to use the Django ORM, in your code, to work with the data making it easier and faster.
https://dj-stripe.dev
MIT License
1.56k stars 476 forks source link

All webhooks coming from connected accounts don't work #740

Closed stygmate closed 2 years ago

stygmate commented 5 years ago

I get errors recorded like so:

Traceback (most recent call last):
  File "/code/djstripe/models/webhooks.py", line 96, in from_request
    obj.valid = obj.validate()
  File "/code/djstripe/models/webhooks.py", line 167, in validate
    id=local_data["id"], api_key=api_key
  File "/usr/local/lib/python3.6/site-packages/stripe/api_resources/abstract/api_resource.py", line 13, in retrieve
    instance.refresh()
  File "/usr/local/lib/python3.6/site-packages/stripe/api_resources/abstract/api_resource.py", line 17, in refresh
    self.refresh_from(self.request('get', self.instance_url()))
  File "/usr/local/lib/python3.6/site-packages/stripe/stripe_object.py", line 207, in request
    response, api_key = requestor.request(method, url, params, headers)
  File "/usr/local/lib/python3.6/site-packages/stripe/api_requestor.py", line 95, in request
    resp = self.interpret_response(rbody, rcode, rheaders)
  File "/usr/local/lib/python3.6/site-packages/stripe/api_requestor.py", line 312, in interpret_response
    self.handle_error_response(rbody, rcode, resp.data, rheaders)
  File "/usr/local/lib/python3.6/site-packages/stripe/api_requestor.py", line 120, in handle_error_response
    raise err
stripe.error.InvalidRequestError: Request req_ixdzPQf0tRtlF9: No such event: evt_1DNJpBJVingG2kkjLvemGfyR
stygmate commented 5 years ago

I think I have found the base of the problem: a request to stripe is made for checking the Event but without "account" key the check is done on the platform account. line 165 of webhooks.py:

        with stripe_temporary_api_version(local_data["api_version"], validate=False):
            remote_data = Event.stripe_class.retrieve(
                id=local_data["id"], api_key=api_key
            )

a bug or connected account are not handled in webhooks ?

jleclanche commented 5 years ago

It sounds like this wasn't tested with connected accounts before, yeah. You want to submit a PR with a test?

stygmate commented 5 years ago

I'm not sure how to handle of that...

some questions: are stripe objects ids really unique cross account ?

@jleclanche En français ? pour en parler par tel ? (un coup de main de quelques minutes pourrais beaucoup m'aider)

ryancausey commented 5 years ago

Related to this, I've found that Stripe treats Account and Connect webhooks as separate entities, and thus there are two different signing keys. That doesn't appear to be supported by djstripe at this time. Is multi-webhook support for Connect vs Account webhooks worthy of a separate issue?

jleclanche commented 5 years ago

I thought there was an issue regarding this. There's some 3.0-milestoned stuff to help with it as well, iirc the Account fk needs to be added to all the stripe objects.

kavdev commented 4 years ago

Stripe doesn't provide an account id for every object in the api (at least last time I checked), so there's no way for us to know to which account a model instance belongs, and thus no easy way to segment data by account. We were considering possible hacks around this limitation, but decided to push that off (as @jleclanche mentioned)

I brought this up to one of our stripe reps, and she said she'd relay the message to their devs. We'll see what happens

kavdev commented 4 years ago

Is multi-webhook support for Connect vs Account webhooks worthy of a separate issue?

@ryancausey yes. We then can track it as blocked

therefromhere commented 4 years ago

This came up again in reference to #960/#962. I think we're unlikely to support this in the upcoming 2.1 release (since getting PaymentIntent support live is pressing), but it could be a focus for 2.2 - I've not looked into how much work it would be (@ryancausey has made a start in #962).

therefromhere commented 4 years ago

Provisionally putting this into 2.2 milestone, needs some investigation as to how much work it'd be / if it'll break much.

therefromhere commented 4 years ago

Bumping this to 2.3 since I'd like to get a 2.2 release done soon.

jayvdb commented 3 years ago

I can confirm this still exists on master. The webhooks code ignores the account field in the payload, and then tries to get objects from the API using the platform account instead of the connected account.

@jleclanche , this seems tightly related around the recent Connected Account features that you have completed. it would be a shame to leave this out of 2.4.0 , and I think it is impossible to not fix this while https://github.com/dj-stripe/dj-stripe/issues/1132 is in the 2.4.0 milestone. It seems this issue is blocking that one (or at least, this issue will haunt anyone attempting to use new functionality provided by issue 1132).

jleclanche commented 3 years ago

@jayvdb I don't have a Stripe Connect setup to test this with.

However, I think a good approach might be to implement the WebhookEndpoint model: https://stripe.com/docs/api/webhook_endpoints

When creating it, we receive a secret parameter. Same as APIKeys, we can store that in the model in the db and use that instead to figure out what secret to verify the signature with.

jayvdb commented 3 years ago

https://github.com/dj-stripe/dj-stripe/pull/1196 is the first step IMO. Without the ability to have a customer connected to more than one account, it is hard to have webhooks or sync working without unique key constraint failures.

jleclanche commented 3 years ago

Landed.

jleclanche commented 3 years ago

@jayvdb Would you know what the next step is on this?

sp576 commented 3 years ago

Hi, I encountered this problem too when I try to integrate Stripe for my new project. I am still new to Stripe and dj-stripe, so not sure what I am doing is correct. However as the others on this thread pointed out, seems like when Webhook Event is being handled by _handle_crud_like_event, account attribute from Stripe's webhook event (https://stripe.com/docs/api/events/object#event_object-account) is ignored when making api_retrieve call. Thus in my case, I ended up implementing the following hack for now:

def webhook_event_callback_for_connect(webhookevent): transaction.on_commit(lambda: process_webhook_event.delay(webhookevent.id))


- edit `_handle_crud_like_event` in `djstripe/event_handler.py`

line 359:

data = target_cls(**kwargs).api_retrieve()

to the following:

account_id = data.get('connected_account_id') data = target_cls(**kwargs).api_retrieve(stripe_account=account_id)

- also edit `process()` in `djstripe/models/webhooks.py` WebhookEventTrigger just in case to be like:

line 187

self.event = Event.process(self.json_body)

to following:

json_body = self.json_body connected_account_id = json_body.get('account') json_body['data']['connected_account_id'] = connected_account_id self.event = Event.process(json_body)


(https://github.com/sp576/dj-stripe/commit/f8a54d4ca016d02fca4c8169089b198560f54a0f)

I was thinking of adding `account` field that saves account id to `Event` model and using that, but I ended up with this hack to avoid adding any field to existing dj-stripe models on my own.

I was also wondering whether I should edit `Event.process` so that it gets optional argument `stripe_account : str` so that it can pass `stripe_account` value when calling `ret = cls._create_from_stripe_object(data)` like `ret = cls._create_from_stripe_object(data, stripe_account=stripe_account)` but wasn't sure.

If anyone could advise the correct approach or any update on this issue that would be great.
jayvdb commented 3 years ago

@sp576 , I have PR https://github.com/dj-stripe/dj-stripe/pull/1232 which mostly fixes this, but you've rightly pointed towards a few bits that I havent added stripe_account to yet.

I also have a bunch of other Connect fixes, mixed in with other misc enhancements, in https://github.com/jayvdb/dj-stripe/tree/pr_1109-and-sync-and-update . The DRF there is #1109 + some Connect fixes.

sp576 commented 3 years ago

@jayvdb thank you for the updates!

arnav13081994 commented 2 years ago

@sp576 @stygmate This issue should be resolved starting in 2.5.0