From f60c3497940819d4ce21aeae5b0626850df21680 Mon Sep 17 00:00:00 2001 From: Diptanil Saha Date: Tue, 8 Sep 2026 14:23:03 +0530 Subject: [PATCH] fix(buying): restrict linked material requests to permitted documents (#58855) --- .../doctype/purchase_order/purchase_order.js | 2 +- erpnext/buying/test_utils.py | 139 ++++++++++++++++++ erpnext/buying/utils.py | 29 +++- erpnext/public/js/controllers/buying.js | 2 +- .../material_request/material_request.json | 7 +- 5 files changed, 175 insertions(+), 4 deletions(-) create mode 100644 erpnext/buying/test_utils.py diff --git a/erpnext/buying/doctype/purchase_order/purchase_order.js b/erpnext/buying/doctype/purchase_order/purchase_order.js index 63c8265469f..60d40f26fb3 100644 --- a/erpnext/buying/doctype/purchase_order/purchase_order.js +++ b/erpnext/buying/doctype/purchase_order/purchase_order.js @@ -581,7 +581,7 @@ erpnext.buying.PurchaseOrderController = class PurchaseOrderController extends ( var item_length = me.frm.doc.items.length; while (i < item_length) { var qty = me.frm.doc.items[i].qty; - (r.message[0] || []).forEach(function (d) { + (r.message || []).forEach(function (d) { if ( d.qty > 0 && qty > 0 && diff --git a/erpnext/buying/test_utils.py b/erpnext/buying/test_utils.py new file mode 100644 index 00000000000..2499c7d9ce3 --- /dev/null +++ b/erpnext/buying/test_utils.py @@ -0,0 +1,139 @@ +# Copyright (c) 2026, Frappe Technologies Pvt. Ltd. and Contributors +# License: GNU General Public License v3. See license.txt + +import json + +import frappe +import frappe.permissions + +from erpnext.buying.utils import get_linked_material_requests +from erpnext.stock.doctype.item.test_item import make_item +from erpnext.stock.doctype.material_request.test_material_request import make_material_request +from erpnext.tests.utils import ERPNextTestSuite + + +def create_user_with_roles(email, *roles): + if frappe.db.exists("User", email): + user = frappe.get_doc("User", email) + else: + user = frappe.new_doc("User") + user.email = email + user.first_name = email.split("@", 1)[0] + user.insert(ignore_permissions=True) + + user.set("roles", []) + for role in roles: + user.append("roles", {"role": role}) + user.save(ignore_permissions=True) + + # a user left without roles is downgraded to a Website User on save + frappe.db.set_value("User", email, "user_type", "System User") + + return user + + +class TestGetLinkedMaterialRequests(ERPNextTestSuite): + def setUp(self): + self.material_request = make_material_request(item_code="_Test Item") + + def test_permitted_role_can_fetch_linked_material_requests(self): + create_user_with_roles("test_buying_purchase_user@example.com", "Purchase User") + + with self.set_user("test_buying_purchase_user@example.com"): + rows = get_linked_material_requests(["_Test Item"]) + + self.assertIn(self.material_request.name, {row.mr_name for row in rows}) + + def test_populated_result_is_a_flat_list_of_rows(self): + """Both callers iterate the response directly, so it has to stay a flat list of rows + rather than a list of lists.""" + create_user_with_roles("test_buying_purchase_user@example.com", "Purchase User") + + with self.set_user("test_buying_purchase_user@example.com"): + rows = get_linked_material_requests(["_Test Item"]) + + self.assertIsInstance(rows, list) + self.assertTrue(rows) + for row in rows: + self.assertNotIsInstance(row, list | tuple) + self.assertIsInstance(row, dict) + for fieldname in ("mr_name", "mr_item", "item_code", "qty"): + self.assertIn(fieldname, row) + + def test_empty_result_is_a_flat_empty_list(self): + item_without_request = make_item("_Test Item Without Material Request").name + create_user_with_roles("test_buying_purchase_user@example.com", "Purchase User") + + with self.set_user("test_buying_purchase_user@example.com"): + rows = get_linked_material_requests([item_without_request]) + + self.assertEqual(rows, []) + + def test_a_single_item_code_is_treated_as_one_code(self): + """A lone code must be read as one item code, not iterated character by character.""" + create_user_with_roles("test_buying_purchase_user@example.com", "Purchase User") + + with self.set_user("test_buying_purchase_user@example.com"): + rows = get_linked_material_requests(json.dumps("_Test Item")) + + self.assertIn(self.material_request.name, {row.mr_name for row in rows}) + + def test_items_that_are_not_item_codes_are_rejected(self): + """Anything that is not a `str` or a `list` is already refused by the type annotation, + so these are the malformed inputs that reach the method.""" + create_user_with_roles("test_buying_purchase_user@example.com", "Purchase User") + bad_inputs = ( + "not json at all", + [{"item_code": "_Test Item"}], + [["_Test Item"]], + [None], + ) + + with self.set_user("test_buying_purchase_user@example.com"): + for bad_items in bad_inputs: + with self.subTest(items=bad_items): + self.assertRaises(frappe.ValidationError, get_linked_material_requests, bad_items) + + def test_manufacturing_manager_can_fetch_linked_material_requests(self): + """Manufacturing Manager holds write on Supplier Quotation and Request for Quotation, + both of which call this method, so it must hold Material Request read as well.""" + create_user_with_roles("test_buying_mfg_manager@example.com", "Manufacturing Manager") + + with self.set_user("test_buying_mfg_manager@example.com"): + rows = get_linked_material_requests(["_Test Item"]) + + self.assertIn(self.material_request.name, {row.mr_name for row in rows}) + + def test_unpermitted_role_cannot_fetch_linked_material_requests(self): + create_user_with_roles("test_buying_sales_user@example.com", "Sales User") + + with self.set_user("test_buying_sales_user@example.com"): + self.assertRaises(frappe.PermissionError, get_linked_material_requests, ["_Test Item"]) + + def test_role_with_only_select_permission_cannot_fetch_linked_material_requests(self): + """Material Request grants Delivery and Maintenance roles `select` and nothing else. + `select` is enough to list names, so the permitted set must be resolved through a + filter on the child table, which requires `read`.""" + create_user_with_roles("test_buying_delivery_user@example.com", "Delivery User") + + with self.set_user("test_buying_delivery_user@example.com"): + self.assertRaises(frappe.PermissionError, get_linked_material_requests, ["_Test Item"]) + + def test_results_are_restricted_by_user_permissions(self): + other_company_request = make_material_request( + item_code="_Test Item", + company="_Test Company 1", + warehouse="_Test Warehouse 2 - _TC1", + cost_center="Main - _TC1", + ) + user = create_user_with_roles("test_buying_restricted_user@example.com", "Purchase User") + frappe.permissions.add_user_permission("Company", "_Test Company", user.name) + + try: + with self.set_user(user.name): + mr_names = {row.mr_name for row in get_linked_material_requests(["_Test Item"])} + finally: + frappe.permissions.remove_user_permission("Company", "_Test Company", user.name) + + self.assertIn(self.material_request.name, mr_names) + self.assertNotIn(other_company_request.name, mr_names) diff --git a/erpnext/buying/utils.py b/erpnext/buying/utils.py index 293c084520e..cf87cf1697d 100644 --- a/erpnext/buying/utils.py +++ b/erpnext/buying/utils.py @@ -129,7 +129,33 @@ def get_linked_material_requests(items: str | list): Retrieve Material Requests linked to a list of items. """ - items = frappe.parse_json(items) + try: + items = frappe.parse_json(items) + except (TypeError, ValueError): + frappe.throw(_("Items must be a list of Item codes")) + + if isinstance(items, str): + items = [items] + + if not isinstance(items, list | tuple) or any(not isinstance(item, str) for item in items): + frappe.throw(_("Items must be a list of Item codes")) + + permitted_material_requests = frappe.get_list( + "Material Request", + filters=[ + ["material_request_type", "=", "Purchase"], + ["docstatus", "=", 1], + ["status", "!=", "Stopped"], + ["per_ordered", "<", 99.99], + ["Material Request Item", "item_code", "in", items], + ], + pluck="name", + distinct=True, + ) + + if not permitted_material_requests: + return [] + mr_list = [] mr = frappe.qb.DocType("Material Request") @@ -146,6 +172,7 @@ def get_linked_material_requests(items: str | list): mr_item.item_code, mr_item.name.as_("mr_item"), ) + .where(mr.name.isin(permitted_material_requests)) .where(mr_item.item_code == item) .where(mr.material_request_type == "Purchase") .where(mr.per_ordered < 99.99) diff --git a/erpnext/public/js/controllers/buying.js b/erpnext/public/js/controllers/buying.js index e89ee39ac49..42017ec6aff 100644 --- a/erpnext/public/js/controllers/buying.js +++ b/erpnext/public/js/controllers/buying.js @@ -541,7 +541,7 @@ erpnext.buying.link_to_mrs = function (frm) { var item_length = frm.doc.items.length; for (let item of frm.doc.items) { var qty = item.qty; - (r.message[0] || []).forEach(function (d) { + (r.message || []).forEach(function (d) { if ( d.qty > 0 && qty > 0 && diff --git a/erpnext/stock/doctype/material_request/material_request.json b/erpnext/stock/doctype/material_request/material_request.json index 3f1d501f5e2..b3205d20426 100644 --- a/erpnext/stock/doctype/material_request/material_request.json +++ b/erpnext/stock/doctype/material_request/material_request.json @@ -377,7 +377,7 @@ "idx": 70, "is_submittable": 1, "links": [], - "modified": "2026-08-21 23:11:44.554719", + "modified": "2026-09-08 12:00:00.000000", "modified_by": "Administrator", "module": "Stock", "name": "Material Request", @@ -442,6 +442,11 @@ "submit": 1, "write": 1 }, + { + "read": 1, + "report": 1, + "role": "Manufacturing Manager" + }, { "role": "Delivery Manager", "select": 1