From 3ef9310ec36f1d5abd3f55e92eedb2ac7133d97a Mon Sep 17 00:00:00 2001 From: "Antoine Vandevenne (anv)" Date: Tue, 10 Oct 2023 16:05:24 +0200 Subject: [PATCH] [FIX] (account_)payment, sale: check document access in payment routes Commit 4d1a1f1c introduced a systematic check on the kwargs passed to transaction routes of modules integrating with online payments, but failed to check the access token of documents whose ID is passed to payment routes. This allowed retrieving the access token of such documents by visiting a route that did not check the document access (e.g., /my/payment_method) and passing an arbitrary document ID (e.g, sale_order_id=123). The route's controller would reroute the payment flow to the document's portal page and render the landing route of the flow on the payment form, with the access token included. This commit makes sure that we always check the access token of a document before reading rerouting a payment flow. closes odoo/odoo#138238 Signed-off-by: Antoine Vandevenne (anv) --- addons/account_payment/controllers/payment.py | 22 ++++++-- .../tests/test_payment_flows.py | 51 ++++++++++++++++++- addons/payment/controllers/portal.py | 4 +- addons/sale/controllers/portal.py | 21 ++++++-- addons/sale/tests/test_payment_flow.py | 49 +++++++++++++++++- 5 files changed, 137 insertions(+), 10 deletions(-) diff --git a/addons/account_payment/controllers/payment.py b/addons/account_payment/controllers/payment.py index c6f9f52b2d6..43ab7f001ec 100644 --- a/addons/account_payment/controllers/payment.py +++ b/addons/account_payment/controllers/payment.py @@ -83,18 +83,32 @@ class PaymentPortal(payment_portal.PaymentPortal): }) return super().payment_pay(*args, amount=amount, access_token=access_token, **kwargs) - def _get_extra_payment_form_values(self, invoice_id=None, **kwargs): + def _get_extra_payment_form_values(self, invoice_id=None, access_token=None, **kwargs): """ Override of `payment` to reroute the payment flow to the portal view of the invoice. :param str invoice_id: The invoice for which a payment id made, as an `account.move` id. - :param dict kwargs: Optional data. This parameter is not used here. + :param str access_token: The portal or payment access token, respectively if we are in a + portal or payment link flow. :return: The extended rendering context values. :rtype: dict """ - form_values = super()._get_extra_payment_form_values(invoice_id=invoice_id, **kwargs) + form_values = super()._get_extra_payment_form_values( + invoice_id=invoice_id, access_token=access_token, **kwargs + ) if invoice_id: invoice_id = self._cast_as_int(invoice_id) - invoice_sudo = request.env['account.move'].sudo().browse(invoice_id) + + try: # Check document access against what could be a portal access token. + invoice_sudo = self._document_check_access('account.move', invoice_id, access_token) + except AccessError: # It is a payment access token computed on the payment context. + if not payment_utils.check_access_token( + access_token, + kwargs.get('partner_id'), + kwargs.get('amount'), + kwargs.get('currency_id'), + ): + raise + invoice_sudo = request.env['account.move'].sudo().browse(invoice_id) # Interrupt the payment flow if the invoice has been canceled. if invoice_sudo.state == 'cancel': diff --git a/addons/account_payment/tests/test_payment_flows.py b/addons/account_payment/tests/test_payment_flows.py index 67f64e85b79..744df0db4cd 100644 --- a/addons/account_payment/tests/test_payment_flows.py +++ b/addons/account_payment/tests/test_payment_flows.py @@ -1,10 +1,15 @@ # Part of Odoo. See LICENSE file for full copyright and licensing details. +from unittest.mock import patch + +from odoo.exceptions import AccessError from odoo.tests import tagged, JsonRpcException from odoo.tools import mute_logger -from odoo.addons.payment.tests.http_common import PaymentHttpCommon +from odoo.addons.account_payment.controllers.payment import PaymentPortal from odoo.addons.account_payment.tests.common import AccountPaymentCommon +from odoo.addons.payment.tests.http_common import PaymentHttpCommon +from odoo.addons.portal.controllers.portal import CustomerPortal @tagged('post_install', '-at_install') @@ -40,6 +45,50 @@ class TestFlows(AccountPaymentCommon, PaymentHttpCommon): self.invoice.invalidate_recordset(['transaction_ids']) self.assertEqual(self.invoice.transaction_ids, tx_sudo) + def test_check_portal_access_token_before_rerouting_flow(self): + """ Test that access to the provided invoice is checked against the portal access token + before rerouting the payment flow. """ + payment_portal_controller = PaymentPortal() + + with patch.object(CustomerPortal, '_document_check_access') as mock: + payment_portal_controller._get_extra_payment_form_values() + self.assertEqual( + mock.call_count, 0, msg="No check should be made when invoice_id is not provided." + ) + + mock.reset_mock() + + payment_portal_controller._get_extra_payment_form_values( + invoice_id=self.invoice.id, access_token='whatever' + ) + self.assertEqual( + mock.call_count, 1, msg="The check should be made as invoice_id is provided." + ) + + def test_check_payment_access_token_before_rerouting_flow(self): + """ Test that access to the provided invoice is checked against the payment access token + before rerouting the payment flow. """ + payment_portal_controller = PaymentPortal() + + def _document_check_access_mock(*_args, **_kwargs): + raise AccessError('') + + with patch.object( + CustomerPortal, '_document_check_access', _document_check_access_mock + ), patch('odoo.addons.payment.utils.check_access_token') as check_payment_access_token_mock: + try: + payment_portal_controller._get_extra_payment_form_values( + invoice_id=self.invoice.id, access_token='whatever' + ) + except Exception: + pass # We don't care if it runs or not; we only count the calls. + self.assertEqual( + check_payment_access_token_mock.call_count, + 1, + msg="The access token should be checked again as a payment access token if the" + " check as a portal access token failed.", + ) + @mute_logger('odoo.http') def test_transaction_route_rejects_unexpected_kwarg(self): url = self._build_url(f'/invoice/transaction/{self.invoice.id}/') diff --git a/addons/payment/controllers/portal.py b/addons/payment/controllers/portal.py index ae4d2fd036e..7af7c39b852 100644 --- a/addons/payment/controllers/portal.py +++ b/addons/payment/controllers/portal.py @@ -154,7 +154,9 @@ class PaymentPortal(portal.CustomerPortal): **portal_page_values, **payment_form_values, **payment_context, - **self._get_extra_payment_form_values(**kwargs), + **self._get_extra_payment_form_values( + **payment_context, currency_id=currency.id, **kwargs + ), # Pass the payment context to allow overriding modules to check document access. } return request.render(self._get_payment_page_template_xmlid(**kwargs), rendering_context) diff --git a/addons/sale/controllers/portal.py b/addons/sale/controllers/portal.py index d3ee6f96d38..5206880e0d3 100644 --- a/addons/sale/controllers/portal.py +++ b/addons/sale/controllers/portal.py @@ -418,17 +418,32 @@ class PaymentPortal(payment_portal.PaymentPortal): }) return super().payment_pay(*args, amount=amount, access_token=access_token, **kwargs) - def _get_extra_payment_form_values(self, sale_order_id=None, **kwargs): + def _get_extra_payment_form_values(self, sale_order_id=None, access_token=None, **kwargs): """ Override of `payment` to reroute the payment flow to the portal view of the sales order. :param str sale_order_id: The sale order for which a payment is made, as a `sale.order` id. + :param str access_token: The portal or payment access token, respectively if we are in a + portal or payment link flow. :return: The extended rendering context values. :rtype: dict """ - form_values = super()._get_extra_payment_form_values(sale_order_id=sale_order_id, **kwargs) + form_values = super()._get_extra_payment_form_values( + sale_order_id=sale_order_id, access_token=access_token, **kwargs + ) if sale_order_id: sale_order_id = self._cast_as_int(sale_order_id) - order_sudo = request.env['sale.order'].sudo().browse(sale_order_id) + + try: # Check document access against what could be a portal access token. + order_sudo = self._document_check_access('sale.order', sale_order_id, access_token) + except AccessError: # It is a payment access token computed on the payment context. + if not payment_utils.check_access_token( + access_token, + kwargs.get('partner_id'), + kwargs.get('amount'), + kwargs.get('currency_id'), + ): + raise + order_sudo = request.env['sale.order'].sudo().browse(sale_order_id) # Interrupt the payment flow if the sales order has been canceled. if order_sudo.state == 'cancel': diff --git a/addons/sale/tests/test_payment_flow.py b/addons/sale/tests/test_payment_flow.py index aa3d698f49a..f382792081b 100644 --- a/addons/sale/tests/test_payment_flow.py +++ b/addons/sale/tests/test_payment_flow.py @@ -1,12 +1,15 @@ # Part of Odoo. See LICENSE file for full copyright and licensing details. + from unittest.mock import ANY, patch -from odoo.fields import Command +from odoo.exceptions import AccessError from odoo.tests import tagged, JsonRpcException from odoo.tools import mute_logger from odoo.addons.account_payment.tests.common import AccountPaymentCommon from odoo.addons.payment.tests.http_common import PaymentHttpCommon +from odoo.addons.portal.controllers.portal import CustomerPortal +from odoo.addons.sale.controllers.portal import PaymentPortal from odoo.addons.sale.tests.common import SaleCommon @@ -385,6 +388,50 @@ class TestSalePayment(AccountPaymentCommon, SaleCommon, PaymentHttpCommon): self.assertTrue(len(invoice) == 1) self.assertTrue(invoice.line_ids[0].is_downpayment) + def test_check_portal_access_token_before_rerouting_flow(self): + """ Test that access to the provided sales order is checked against the portal access token + before rerouting the payment flow. """ + payment_portal_controller = PaymentPortal() + + with patch.object(CustomerPortal, '_document_check_access') as mock: + payment_portal_controller._get_extra_payment_form_values() + self.assertEqual( + mock.call_count, 0, msg="No check should be made when sale_order_id is not provided." + ) + + mock.reset_mock() + + payment_portal_controller._get_extra_payment_form_values( + sale_order_id=self.sale_order.id, access_token='whatever' + ) + self.assertEqual( + mock.call_count, 1, msg="The check should be made as sale_order_id is provided." + ) + + def test_check_payment_access_token_before_rerouting_flow(self): + """ Test that access to the provided sales order is checked against the payment access token + before rerouting the payment flow. """ + payment_portal_controller = PaymentPortal() + + def _document_check_access_mock(*_args, **_kwargs): + raise AccessError('') + + with patch.object( + CustomerPortal, '_document_check_access', _document_check_access_mock + ), patch('odoo.addons.payment.utils.check_access_token') as check_payment_access_token_mock: + try: + payment_portal_controller._get_extra_payment_form_values( + sale_order_id=self.sale_order.id, access_token='whatever' + ) + except Exception: + pass # We don't care if it runs or not; we only count the calls. + self.assertEqual( + check_payment_access_token_mock.call_count, + 1, + msg="The access token should be checked again as a payment access token if the" + " check as a portal access token failed.", + ) + @mute_logger('odoo.http') def test_transaction_route_rejects_unexpected_kwarg(self): url = self._build_url(f'/my/orders/{self.sale_order.id}/transaction')