Merge pull request #55828 from rohitwaghchaure/fixed-bom-creator-security-issues-v15

fix: multiple issues related to BOM Creator

erpnext/manufacturing/doctype/bom_creator/bom_creator.py

@@ -152,6 +152,7 @@ class BOMCreator(Document):
 
 	@frappe.whitelist()
 	def add_boms(self):
+		self.check_permission("submit")
 		self.submit()
 
 	def set_rate_for_items(self):
@@ -209,10 +210,14 @@ class BOMCreator(Document):
 				frappe.throw(_("Please set {0} in BOM Creator {1}").format(_(label), self.name))
 
 	def on_submit(self):
-		self.enqueue_create_boms()
+		self.enqueue_bom_creation()
 
 	@frappe.whitelist()
 	def enqueue_create_boms(self):
+		self.check_permission("submit")
+		self.enqueue_bom_creation()
+
+	def enqueue_bom_creation(self):
 		frappe.enqueue(
 			self.create_boms,
 			queue="short",
@@ -281,6 +286,23 @@ class BOMCreator(Document):
 
 			frappe.msgprint(_("BOMs creation failed"))
 
+	@frappe.whitelist()
+	def edit_qty(self, docname: str, qty: float):
+		self.check_permission("write")
+
+		if not frappe.db.exists("BOM Creator Item", {"name": docname, "parent": self.name}):
+			frappe.throw(_("BOM Creator Item {0} does not exist").format(docname))
+
+		for row in self.items:
+			if row.name == docname:
+				row.qty = flt(qty)
+				break
+
+		self.set_rate_for_items()
+		self.save()
+
+		return self
+
 	def create_bom(self, row, production_item_wise_rm):
 		bom_creator_item = row.name if row.name != self.name else ""
 		if frappe.db.exists(
@@ -336,18 +358,163 @@ class BOMCreator(Document):
 		production_item_wise_rm[(row.item_code, row.name)].bom_no = bom.name
 
 	@frappe.whitelist()
-	def get_default_bom(self, item_code) -> str:
+	def get_default_bom(self, item_code: str) -> str:
+		self.check_permission("read")
 		return frappe.get_cached_value("Item", item_code, "default_bom")
 
+	@frappe.whitelist()
+	def add_item(self, **kwargs):
+		self.check_permission("write")
+
+		if isinstance(kwargs, str):
+			kwargs = frappe.parse_json(kwargs)
+
+		if isinstance(kwargs, dict):
+			kwargs = frappe._dict(kwargs)
+
+		item_info = get_item_details(kwargs.item_code)
+
+		parent_row_no = ""
+		if kwargs.fg_reference_id and self.name != kwargs.fg_reference_id:
+			parent_row_no = get_parent_row_no(self, kwargs.fg_reference_id)
+
+		kwargs.update(
+			{
+				"uom": item_info.stock_uom,
+				"stock_uom": item_info.stock_uom,
+				"conversion_factor": 1,
+			}
+		)
+
+		if parent_row_no:
+			kwargs.update({"parent_row_no": parent_row_no})
+
+		for key in BOM_ITEM_FIELDS:
+			if key not in kwargs:
+				kwargs[key] = ""
+
+		self.append("items", kwargs)
+		self.save()
+
+		return self
+
+	@frappe.whitelist()
+	def add_sub_assembly(self, **kwargs):
+		self.check_permission("write")
+
+		if isinstance(kwargs, str):
+			kwargs = frappe.parse_json(kwargs)
+
+		if isinstance(kwargs, dict):
+			kwargs = frappe._dict(kwargs)
+
+		bom_item = frappe.parse_json(kwargs.bom_item)
+
+		name = kwargs.fg_reference_id
+		parent_row_no = ""
+		if not kwargs.convert_to_sub_assembly:
+			item_info = get_item_details(bom_item.item_code)
+			parent_row_no = get_parent_row_no(self, kwargs.fg_reference_id)
+
+			item_row = self.append(
+				"items",
+				{
+					"item_code": bom_item.item_code,
+					"qty": bom_item.qty,
+					"uom": item_info.stock_uom,
+					"fg_item": kwargs.fg_item,
+					"conversion_factor": 1,
+					"parent_row_no": parent_row_no,
+					"fg_reference_id": name,
+					"stock_qty": bom_item.qty,
+					"do_not_explode": 1,
+					"is_expandable": 1,
+					"stock_uom": item_info.stock_uom,
+					"allow_alternative_item": kwargs.allow_alternative_item,
+				},
+			)
+
+			parent_row_no = item_row.idx
+			name = ""
+		else:
+			parent_row_no = get_parent_row_no(self, kwargs.fg_reference_id)
+
+		for row in bom_item.get("items"):
+			row = frappe._dict(row)
+			item_info = get_item_details(row.item_code)
+			self.append(
+				"items",
+				{
+					"item_code": row.item_code,
+					"qty": row.qty,
+					"fg_item": bom_item.item_code,
+					"uom": item_info.stock_uom,
+					"fg_reference_id": name,
+					"parent_row_no": parent_row_no,
+					"conversion_factor": 1,
+					"do_not_explode": 1,
+					"stock_qty": row.qty,
+					"stock_uom": item_info.stock_uom,
+				},
+			)
+
+		self.save()
+
+		return self
+
+	@frappe.whitelist()
+	def delete_node(self, **kwargs):
+		self.check_permission("write")
+
+		if isinstance(kwargs, str):
+			kwargs = frappe.parse_json(kwargs)
+
+		if isinstance(kwargs, dict):
+			kwargs = frappe._dict(kwargs)
+
+		updated = False
+		if kwargs.docname:
+			row = next((row for row in self.items if row.name == kwargs.docname), None)
+			if not row:
+				frappe.throw(_("BOM Creator Item with name {0} does not exist").format(kwargs.docname))
+
+			row.delete()
+			self.remove(row)
+			updated = True
+
+		items = get_children(parent=kwargs.fg_item, parent_id=self.name)
+		if items:
+			for item in items:
+				updated = True
+				child_row = next((row for row in self.items if row.name == item.name), None)
+				if child_row:
+					child_row.delete()
+					self.remove(child_row)
+
+				if item.expandable:
+					self.delete_node(fg_item=item.value)
+
+		if updated:
+			self.set_rate_for_items()
+			self.save()
+
+			return self
+
+		return frappe._dict()
+
 
 @frappe.whitelist()
-def get_children(doctype=None, parent=None, **kwargs):
+def get_children(doctype: str | None = None, parent: str | None = None, **kwargs):
+	# by default get_children takes first parameter as doctype, so added in the function
+
 	if isinstance(kwargs, str):
 		kwargs = frappe.parse_json(kwargs)
 
 	if isinstance(kwargs, dict):
 		kwargs = frappe._dict(kwargs)
 
+	frappe.has_permission("BOM Creator", "read", doc=kwargs.parent_id, throw=True)
+
 	fields = [
 		"item_code as value",
 		"item_name as title",
@@ -373,102 +540,6 @@ def get_children(doctype=None, parent=None, **kwargs):
 	return frappe.get_all("BOM Creator Item", fields=fields, filters=query_filters, order_by="idx")
 
 
-@frappe.whitelist()
-def add_item(**kwargs):
-	if isinstance(kwargs, str):
-		kwargs = frappe.parse_json(kwargs)
-
-	if isinstance(kwargs, dict):
-		kwargs = frappe._dict(kwargs)
-
-	doc = frappe.get_doc("BOM Creator", kwargs.parent)
-	item_info = get_item_details(kwargs.item_code)
-
-	parent_row_no = ""
-	if kwargs.fg_reference_id and doc.name != kwargs.fg_reference_id:
-		parent_row_no = get_parent_row_no(doc, kwargs.fg_reference_id)
-
-	kwargs.update(
-		{
-			"uom": item_info.stock_uom,
-			"stock_uom": item_info.stock_uom,
-			"conversion_factor": 1,
-		}
-	)
-
-	if parent_row_no:
-		kwargs.update({"parent_row_no": parent_row_no})
-
-	doc.append("items", kwargs)
-	doc.save()
-
-	return doc
-
-
-@frappe.whitelist()
-def add_sub_assembly(**kwargs):
-	if isinstance(kwargs, str):
-		kwargs = frappe.parse_json(kwargs)
-
-	if isinstance(kwargs, dict):
-		kwargs = frappe._dict(kwargs)
-
-	doc = frappe.get_doc("BOM Creator", kwargs.parent)
-	bom_item = frappe.parse_json(kwargs.bom_item)
-
-	name = kwargs.fg_reference_id
-	parent_row_no = ""
-	if not kwargs.convert_to_sub_assembly:
-		item_info = get_item_details(bom_item.item_code)
-		parent_row_no = get_parent_row_no(doc, kwargs.fg_reference_id)
-
-		item_row = doc.append(
-			"items",
-			{
-				"item_code": bom_item.item_code,
-				"qty": bom_item.qty,
-				"uom": item_info.stock_uom,
-				"fg_item": kwargs.fg_item,
-				"conversion_factor": 1,
-				"parent_row_no": parent_row_no,
-				"fg_reference_id": name,
-				"stock_qty": bom_item.qty,
-				"do_not_explode": 1,
-				"is_expandable": 1,
-				"stock_uom": item_info.stock_uom,
-				"allow_alternative_item": kwargs.allow_alternative_item,
-			},
-		)
-
-		parent_row_no = item_row.idx
-		name = ""
-	else:
-		parent_row_no = get_parent_row_no(doc, kwargs.fg_reference_id)
-
-	for row in bom_item.get("items"):
-		row = frappe._dict(row)
-		item_info = get_item_details(row.item_code)
-		doc.append(
-			"items",
-			{
-				"item_code": row.item_code,
-				"qty": row.qty,
-				"fg_item": bom_item.item_code,
-				"uom": item_info.stock_uom,
-				"fg_reference_id": name,
-				"parent_row_no": parent_row_no,
-				"conversion_factor": 1,
-				"do_not_explode": 1,
-				"stock_qty": row.qty,
-				"stock_uom": item_info.stock_uom,
-			},
-		)
-
-	doc.save()
-
-	return doc
-
-
 def get_item_details(item_code):
 	return frappe.get_cached_value(
 		"Item", item_code, ["item_name", "description", "image", "stock_uom", "default_bom"], as_dict=1
@@ -486,37 +557,3 @@ def get_parent_row_no(doc, name):
 	frappe.msgprint(_("Parent Row No not found for {0}").format(name), alert=True)
 
 	return None
-
-
-@frappe.whitelist()
-def delete_node(**kwargs):
-	if isinstance(kwargs, str):
-		kwargs = frappe.parse_json(kwargs)
-
-	if isinstance(kwargs, dict):
-		kwargs = frappe._dict(kwargs)
-
-	items = get_children(parent=kwargs.fg_item, parent_id=kwargs.parent)
-	if kwargs.docname:
-		frappe.delete_doc("BOM Creator Item", kwargs.docname)
-
-	for item in items:
-		frappe.delete_doc("BOM Creator Item", item.name)
-		if item.expandable:
-			delete_node(fg_item=item.value, parent=item.parent_id)
-
-	doc = frappe.get_doc("BOM Creator", kwargs.parent)
-	doc.set_rate_for_items()
-	doc.save()
-
-	return doc
-
-
-@frappe.whitelist()
-def edit_qty(doctype, docname, qty, parent):
-	frappe.db.set_value(doctype, docname, "qty", qty)
-	doc = frappe.get_doc("BOM Creator", parent)
-	doc.set_rate_for_items()
-	doc.save()
-
-	return doc

erpnext/manufacturing/doctype/bom_creator/test_bom_creator.py

@@ -6,10 +6,6 @@ import random
 import frappe
 from frappe.tests.utils import FrappeTestCase
 
-from erpnext.manufacturing.doctype.bom_creator.bom_creator import (
-	add_item,
-	add_sub_assembly,
-)
 from erpnext.stock.doctype.item.test_item import make_item
 
 
@@ -38,8 +34,7 @@ class TestBOMCreator(FrappeTestCase):
 			conversion_rate=1,
 		)
 
-		add_sub_assembly(
-			parent=doc.name,
+		doc.add_sub_assembly(
 			fg_item=final_product,
 			fg_reference_id=doc.name,
 			bom_item={
@@ -91,8 +86,7 @@ class TestBOMCreator(FrappeTestCase):
 			conversion_rate=1,
 		)
 
-		add_item(
-			parent=doc.name,
+		doc.add_item(
 			fg_item=final_product,
 			fg_reference_id=doc.name,
 			item_code="Pedal Assembly",
@@ -133,8 +127,7 @@ class TestBOMCreator(FrappeTestCase):
 			conversion_rate=1,
 		)
 
-		add_item(
-			parent=doc.name,
+		doc.add_item(
 			fg_item=final_product,
 			fg_reference_id=doc.name,
 			item_code="Pedal Assembly",
@@ -144,9 +137,8 @@ class TestBOMCreator(FrappeTestCase):
 		doc.reload()
 		self.assertEqual(doc.items[0].is_expandable, 0)
 
-		add_sub_assembly(
+		doc.add_sub_assembly(
 			convert_to_sub_assembly=1,
-			parent=doc.name,
 			fg_item=final_product,
 			fg_reference_id=doc.items[0].name,
 			bom_item={
@@ -199,8 +191,7 @@ class TestBOMCreator(FrappeTestCase):
 			conversion_rate=1,
 		)
 
-		add_item(
-			parent=doc.name,
+		doc.add_item(
 			fg_item=final_product,
 			fg_reference_id=doc.name,
 			item_code="Pedal Assembly",
@@ -210,9 +201,8 @@ class TestBOMCreator(FrappeTestCase):
 		doc.reload()
 		self.assertEqual(doc.items[0].is_expandable, 0)
 
-		add_sub_assembly(
+		doc.add_sub_assembly(
 			convert_to_sub_assembly=1,
-			parent=doc.name,
 			fg_item=final_product,
 			fg_reference_id=doc.items[0].name,
 			bom_item={

erpnext/public/js/bom_configurator/bom_configurator.bundle.js

@@ -219,14 +219,10 @@ class BOMConfigurator {
 				},
 			],
 			(data) => {
-				if (!node.data.parent_id) {
-					node.data.parent_id = this.frm.doc.name;
-				}
-
 				frappe.call({
-					method: "erpnext.manufacturing.doctype.bom_creator.bom_creator.add_item",
+					method: "add_item",
+					doc: this.frm.doc,
 					args: {
-						parent: node.data.parent_id,
 						fg_item: node.data.value,
 						item_code: data.item_code,
 						fg_reference_id: node.data.name || this.frm.doc.name,
@@ -255,14 +251,10 @@ class BOMConfigurator {
 		dialog.set_primary_action(__("Add"), () => {
 			let bom_item = dialog.get_values();
 
-			if (!node.data?.parent_id) {
-				node.data.parent_id = this.frm.doc.name;
-			}
-
 			frappe.call({
-				method: "erpnext.manufacturing.doctype.bom_creator.bom_creator.add_sub_assembly",
+				method: "add_sub_assembly",
+				doc: this.frm.doc,
 				args: {
-					parent: node.data.parent_id,
 					fg_item: node.data.value,
 					fg_reference_id: node.data.name || this.frm.doc.name,
 					bom_item: bom_item,
@@ -357,9 +349,9 @@ class BOMConfigurator {
 			let bom_item = dialog.get_values();
 
 			frappe.call({
-				method: "erpnext.manufacturing.doctype.bom_creator.bom_creator.add_sub_assembly",
+				method: "add_sub_assembly",
+				doc: this.frm.doc,
 				args: {
-					parent: node.data.parent_id,
 					fg_item: node.data.value,
 					bom_item: bom_item,
 					fg_reference_id: node.data.name || this.frm.doc.name,
@@ -389,11 +381,10 @@ class BOMConfigurator {
 	delete_node(node, view) {
 		frappe.confirm(__("Are you sure you want to delete this Item?"), () => {
 			frappe.call({
-				method: "erpnext.manufacturing.doctype.bom_creator.bom_creator.delete_node",
+				method: "delete_node",
+				doc: this.frm.doc,
 				args: {
-					parent: node.data.parent_id,
 					fg_item: node.data.value,
-					doctype: node.data.doctype,
 					docname: node.data.name,
 				},
 				callback: (r) => {
@@ -408,16 +399,14 @@ class BOMConfigurator {
 		frappe.prompt(
 			[{ label: __("Qty"), fieldname: "qty", default: qty, fieldtype: "Float", reqd: 1 }],
 			(data) => {
-				let doctype = node.data.doctype || this.frm.doc.doctype;
 				let docname = node.data.name || this.frm.doc.name;
 
 				frappe.call({
-					method: "erpnext.manufacturing.doctype.bom_creator.bom_creator.edit_qty",
+					method: "edit_qty",
+					doc: this.frm.doc,
 					args: {
-						doctype: doctype,
 						docname: docname,
 						qty: data.qty,
-						parent: node.data.parent_id ? node.data.parent_id : this.frm.doc.name,
 					},
 					callback: (r) => {
 						node.data.qty = data.qty;