[IMP] payment_stripe: support webhooks #69809
Conversation
There was a problem hiding this comment.
I think test should work (testing it with --test-tags payment_stripe_checkout_webhook --stop-after-init) with these changes:
https://gist.github.com/nle-odoo/532d4c81a6122997531543e349be1426
to run the test in 14.0 or 12.0 in local, I also needed to use MockRequest (https://gist.github.com/nle-odoo/532d4c81a6122997531543e349be1426/ab003672a3d684312bde78658574c4fffc7cecfc) but it seems to work without that on runbot.
nle-odoo
force-pushed
the
12.0-payment_stripe-checkout-webhook-backport
branch
from
ec9c3c0
to
859ecd0
Compare
|
Hello @AntoineVDV What do you think about it? |
Hello, it's in my review pipe but I can't have a look at it right now. Tomorrow at best :| |
There was a problem hiding this comment.
As this is a backport of an already tested feature, I only made a quick review without thorough testing but I found nothing to worry about.
Side note: I think that you should set yourself as the author of the commit since it's not an exact backport of dc4f6ad.
| <base_url>/payment/stripe/webhook and should only subscribe to | ||
| checkout.session.completed events to avoid spamming the Odoo server with | ||
| useless notifications.""", | ||
| 'depends': ['payment', 'payment_stripe', 'payment_stripe_sca'], |
There was a problem hiding this comment.
| 'depends': ['payment', 'payment_stripe', 'payment_stripe_sca'], | |
| 'depends': ['payment_stripe', 'payment_stripe_sca'], |
payment_stripe already depends on payment but it's not a big deal in this case.
it is being backported by aho, sig just created the PR for his work : https://www.odoo.com/web?debug#id=2449738&model=project.task |
simongoffin
force-pushed
the
12.0-payment_stripe-checkout-webhook-backport
branch
from
859ecd0
to
b2730ed
Compare
|
Hello @mart-e Is is possible to have the security review? |
| _inherit = 'payment.acquirer' | ||
|
|
||
| stripe_webhook_secret = fields.Char( | ||
| string='Stripe Webhook Secret', groups='base.group_user', |
There was a problem hiding this comment.
base.group_user ? is there a reason to allow employee and not only admin?
|
|
||
| @http.route('/payment/stripe/webhook', type='json', auth='public', csrf=False) | ||
| def stripe_webhook(self, **kwargs): | ||
| data = json.loads(request.httprequest.data) |
There was a problem hiding this comment.
controller type is already json
|
|
||
| stripe_object = data.get('data', {}).get('object') | ||
| if not stripe_object: | ||
| raise ValidationError('Stripe Webhook data does not conform to the expected API.') |
There was a problem hiding this comment.
Is this something displayed to user? If so it needs to be translated
| 'AUTHORIZATION': 'Bearer %s' % self.sudo().stripe_secret_key, | ||
| 'Stripe-Version': '2019-05-16', # SetupIntent need a specific version | ||
| } | ||
| resp = requests.request(method, url, data=data, headers=headers) |
| except HTTPError: | ||
| _logger.error(resp.text) | ||
| stripe_error = resp.json().get('error', {}).get('message', '') | ||
| error_msg = " " + (_("Stripe gave us the following info about the problem: '%s'", stripe_error)) |
There was a problem hiding this comment.
the notation _("foo %s", bar) for translations was introduced in 14.0
| <field name="arch" type="xml"> | ||
| <xpath expr='//group[@name="acquirer"]' position='after'> | ||
| <group attrs="{'invisible': [('provider', '!=', 'stripe')]}"> | ||
| <field name="stripe_webhook_secret" password="True"/> |
There was a problem hiding this comment.
if we are modifying the groups attribute in python, need to add one here too
simongoffin
force-pushed
the
12.0-payment_stripe-checkout-webhook-backport
branch
2 times, most recently
from
8438e69
to
3173f11
Compare
|
@robodoo override=ci/security |
Allow configuring a webhook in Stripe to send s2s notifications to Odoo when a Checkout payment is completed. Note that SetupIntent and PaymentIntent events are not listened to, since they are handled 'live' with the customer actively present; the main use case for Stripe webhooks is a Checkout session that gets interrupted before the customer is redirected to Odoo (e.g. network loss, browser crash, closing the tab, etc.). The webhook should be configured to send its events to <base_url>/payment/stripe/webhook and should only subscribe to checkout.session.completed events to avoid spamming the Odoo server with useless notifications. opw-2488452 opw-2451463 opw-2449738 BACKPORT of commit: dc4f6ad Should not be merged beyond 14.0 (14.0 excluded)
simongoffin
force-pushed
the
12.0-payment_stripe-checkout-webhook-backport
branch
from
3173f11
to
a1ff4dc
Compare
|
robodoo r+ |
Allow configuring a webhook in Stripe to send s2s notifications to Odoo when a Checkout payment is completed. Note that SetupIntent and PaymentIntent events are not listened to, since they are handled 'live' with the customer actively present; the main use case for Stripe webhooks is a Checkout session that gets interrupted before the customer is redirected to Odoo (e.g. network loss, browser crash, closing the tab, etc.). The webhook should be configured to send its events to <base_url>/payment/stripe/webhook and should only subscribe to checkout.session.completed events to avoid spamming the Odoo server with useless notifications. opw-2488452 opw-2451463 opw-2449738 BACKPORT of commit: dc4f6ad Should not be merged beyond 14.0 (14.0 excluded) closes #69809 Signed-off-by: Simon Goffin (sig) <sig@openerp.com>
|
This pull request has forward-port PRs awaiting action (not merged or closed): #71558 |
1 similar comment
|
This pull request has forward-port PRs awaiting action (not merged or closed): #71558 |
|
This pull request has forward-port PRs awaiting action (not merged or closed): #71558 |
3 similar comments
|
This pull request has forward-port PRs awaiting action (not merged or closed): #71558 |
|
This pull request has forward-port PRs awaiting action (not merged or closed): #71558 |
|
This pull request has forward-port PRs awaiting action (not merged or closed): #71558 |
|
@simongoffin don't forget your forward ports |
|
Wow, I did not recognize that soo many people collaborated on this showstopper 😝 |
|
This pull request has forward-port PRs awaiting action (not merged or closed): #71558 |
1 similar comment
|
This pull request has forward-port PRs awaiting action (not merged or closed): #71558 |
|
This pull request has forward-port PRs awaiting action (not merged or closed): #71558 |
Allow configuring a webhook in Stripe to send s2s notifications to Odoo when a Checkout payment is completed. Note that SetupIntent and PaymentIntent events are not listened to, since they are handled 'live' with the customer actively present; the main use case for Stripe webhooks is a Checkout session that gets interrupted before the customer is redirected to Odoo (e.g. network loss, browser crash, closing the tab, etc.). The webhook should be configured to send its events to <base_url>/payment/stripe/webhook and should only subscribe to checkout.session.completed events to avoid spamming the Odoo server with useless notifications. opw-2488452 opw-2451463 opw-2449738 BACKPORT of commit: dc4f6ad Should not be merged beyond 14.0 (14.0 excluded) closes odoo#69809 Signed-off-by: Simon Goffin (sig) <sig@openerp.com>
Allow configuring a webhook in Stripe to send s2s notifications to Odoo
when a Checkout payment is completed. Note that SetupIntent and
PaymentIntent events are not listened to, since they are handled 'live'
with the customer actively present; the main use case for Stripe
webhooks is a Checkout session that gets interrupted before the customer
is redirected to Odoo (e.g. network loss, browser crash, closing the
tab, etc.).
The webhook should be configured to send its events to
<base_url>/payment/stripe/webhook and should only subscribe to
checkout.session.completed events to avoid spamming the Odoo server with
useless notifications.
opw-2488452
opw-2451463
opw-2449738
BACKPORT of commit: dc4f6ad
Should not be merged beyond 14.0 (14.0 excluded)