From 1f8b2c5a1bb5577b29b64f74fc8146ff12788a9b Mon Sep 17 00:00:00 2001 From: Damien Bouvy Date: Fri, 23 Aug 2019 14:10:34 +0000 Subject: [PATCH] [IMP] payment, sale: access token for payment link wizard When generating a link for a payment through the website_payment/pay route, including the partner_id 'blindly' is somewhat of a security issue since it means you could potentially create payments for any partner of your choosing. This commit instead introduces an access token mechanism where the partner_id, amount and currency_id are used to generate a unique access token that can be checked upon accessing the payment page. This token is only checked if the partner_id field is set (otherwise this is just an anonymous payment like any other). --- addons/payment/controllers/portal.py | 33 ++++++++++++++++--- addons/payment/wizards/payment_link_wizard.py | 29 ++++++++++++++-- .../wizards/payment_link_wizard_views.xml | 1 + addons/sale/wizard/sale_payment_link.py | 11 +++---- 4 files changed, 61 insertions(+), 13 deletions(-) diff --git a/addons/payment/controllers/portal.py b/addons/payment/controllers/portal.py index a674c4e97fc..2d29278cd7d 100644 --- a/addons/payment/controllers/portal.py +++ b/addons/payment/controllers/portal.py @@ -132,10 +132,27 @@ class WebsitePayment(http.Controller): return request.render("payment.pay_methods", values) @http.route(['/website_payment/pay'], type='http', auth='public', website=True, sitemap=False) - def pay(self, reference='', order_id=None, amount=False, currency_id=None, acquirer_id=None, partner_id=False, **kw): + def pay(self, reference='', order_id=None, amount=False, currency_id=None, acquirer_id=None, partner_id=False, access_token=None, **kw): + """ + Generic payment page allowing public and logged in users to pay an arbitrary amount. + + In the case of a public user access, we need to ensure that the payment is made anonymously - e.g. it should not be + possible to pay for a specific partner simply by setting the partner_id GET param to a random id. In the case where + a partner_id is set, we do an access_token check based on the payment.link.wizard model (since links for specific + partners should be created from there and there only). Also noteworthy is the filtering of s2s payment methods - + we don't want to create payment tokens for public users. + + In the case of a logged in user, then we let access rights and security rules do their job. + """ env = request.env user = env.user.sudo() reference = normalize('NFKD', reference).encode('ascii','ignore').decode('utf-8') + if partner_id and not access_token: + raise werkzeug.exceptions.NotFound + if partner_id and access_token: + token_ok = request.env['payment.link.wizard'].check_token(access_token, int(partner_id), float(amount), int(currency_id)) + if not token_ok: + raise werkzeug.exceptions.NotFound # Default values values = { @@ -185,6 +202,11 @@ class WebsitePayment(http.Controller): # Check partner if not user._is_public(): + # NOTE: this means that if the partner was set in the GET param, it gets overwritten here + # This is something we want, since security rules are based on the partner - assuming the + # access_token checked out at the start, this should have no impact on the payment itself + # existing besides making reconciliation possibly more difficult (if the payment partner is + # not the same as the invoice partner, for example) partner_id = user.partner_id.id elif partner_id: partner_id = int(partner_id) @@ -195,6 +217,7 @@ class WebsitePayment(http.Controller): 'error_msg': kw.get('error_msg') }) + # s2s mode will always generate a token, which we don't want for public users valid_flows = ['form', 's2s'] if not user._is_public() else ['form'] values['acquirers'] = [acq for acq in acquirers if acq.payment_flow in valid_flows] values['pms'] = request.env['payment.token'].search([('acquirer_id', 'in', acquirers.ids)]) @@ -255,14 +278,14 @@ class WebsitePayment(http.Controller): 'reference': reference, 'amount': float(amount), 'currency_id': int(currency_id), - 'partner_id': partner_id, - 'payment_token_id': pm_id, + 'partner_id': int(partner_id), + 'payment_token_id': int(pm_id), 'type': 'server2server', 'return_url': return_url, } if order_id: - values['sale_order_ids'] = [(6, 0, [order_id])] + values['sale_order_ids'] = [(6, 0, [int(order_id)])] tx = request.env['payment.transaction'].sudo().with_context(lang=None).create(values) PaymentProcessing.add_payment_transaction(tx) @@ -272,7 +295,7 @@ class WebsitePayment(http.Controller): secret = request.env['ir.config_parameter'].sudo().get_param('database.secret') token_str = '%s%s%s' % (tx.id, tx.reference, tx.amount) token = hmac.new(secret.encode('utf-8'), token_str.encode('utf-8'), hashlib.sha256).hexdigest() - tx.return_url = return_url or '/website_payment/confirm?tx_id=%d&access_token=%s' % tx.id + tx.return_url = return_url or '/website_payment/confirm?tx_id=%d&access_token=%s' % (tx.id, token) except Exception as e: _logger.exception(e) return request.redirect('/payment/process') diff --git a/addons/payment/wizards/payment_link_wizard.py b/addons/payment/wizards/payment_link_wizard.py index 4d24315153c..fd068e896b3 100644 --- a/addons/payment/wizards/payment_link_wizard.py +++ b/addons/payment/wizards/payment_link_wizard.py @@ -1,8 +1,11 @@ # -*- coding: utf-8 -*- # Part of Odoo. See LICENSE file for full copyright and licensing details. +import hashlib +import hmac from odoo import api, fields, models, _ from odoo.exceptions import ValidationError +from odoo.tools import ustr, consteq class PaymentLinkWizard(models.TransientModel): @@ -32,8 +35,9 @@ class PaymentLinkWizard(models.TransientModel): amount_max = fields.Monetary(currency_field='currency_id') currency_id = fields.Many2one('res.currency') partner_id = fields.Many2one('res.partner') - link = fields.Char(string='Payment Link') + link = fields.Char(string='Payment Link', compute='_compute_values') description = fields.Char('Payment Ref') + access_token = fields.Char(compute='_compute_values') @api.onchange('amount', 'description') def _onchange_amount(self): @@ -41,5 +45,26 @@ class PaymentLinkWizard(models.TransientModel): raise ValidationError(_("Please set an amount smaller than %s.") % (self.amount_max)) if self.amount <= 0: raise ValidationError(_("The value of the payment amount must be positive.")) + + @api.depends('amount', 'description', 'partner_id', 'currency_id') + def _compute_values(self): + secret = self.env['ir.config_parameter'].sudo().get_param('database.secret') + for payment_link in self: + token_str = '%s%s%s' % (payment_link.partner_id.id, payment_link.amount, payment_link.currency_id.id) + payment_link.access_token = hmac.new(secret.encode('utf-8'), token_str.encode('utf-8'), hashlib.sha256).hexdigest() + # must be called after token generation, obvsly - the link needs an up-to-date token + self._generate_link() + + def _generate_link(self): base_url = self.env['ir.config_parameter'].sudo().get_param('web.base.url') - self.link = '%s/website_payment/pay?reference=%s&amount=%s¤cy_id=%s&partner_id=%s' % (base_url, self.description, self.amount, self.currency_id.id, self.partner_id.id) + for payment_link in self: + payment_link.link = '%s/website_payment/pay?reference=%s&amount=%s¤cy_id=%s&partner_id=%s&access_token=%s' % (base_url, payment_link.description, payment_link.amount, payment_link.currency_id.id, payment_link.partner_id.id, payment_link.access_token) + + @api.model + def check_token(self, access_token, partner_id, amount, currency_id): + secret = self.env['ir.config_parameter'].sudo().get_param('database.secret') + token_str = '%s%s%s' % (partner_id, amount, currency_id) + correct_token = hmac.new(secret.encode('utf-8'), token_str.encode('utf-8'), hashlib.sha256).hexdigest() + if consteq(ustr(access_token), correct_token): + return True + return False \ No newline at end of file diff --git a/addons/payment/wizards/payment_link_wizard_views.xml b/addons/payment/wizards/payment_link_wizard_views.xml index 3db3945eb10..500e9b0a181 100644 --- a/addons/payment/wizards/payment_link_wizard_views.xml +++ b/addons/payment/wizards/payment_link_wizard_views.xml @@ -13,6 +13,7 @@ + diff --git a/addons/sale/wizard/sale_payment_link.py b/addons/sale/wizard/sale_payment_link.py index 7c338ac6405..6ea2930fbd3 100644 --- a/addons/sale/wizard/sale_payment_link.py +++ b/addons/sale/wizard/sale_payment_link.py @@ -22,9 +22,8 @@ class SalePaymentLink(models.TransientModel): }) return res - @api.onchange('amount', 'description') - def _onchange_amount(self): - res = super(SalePaymentLink, self)._onchange_amount() - if self.res_model == 'sale.order': - self.link = "%s&order_id=%d" % (self.link, self.res_id) - return res + def _generate_link(self): + """ Override of the base method to add the order_id in the link. """ + base_url = self.env['ir.config_parameter'].sudo().get_param('web.base.url') + for payment_link in self: + payment_link.link = '%s/website_payment/pay?reference=%s&amount=%s¤cy_id=%s&partner_id=%s&order_id=%s&access_token=%s' % (base_url, payment_link.description, payment_link.amount, payment_link.currency_id.id, payment_link.partner_id.id, payment_link.res_id, payment_link.access_token)