fix: render doctype caster server-script-callable

[blaggacao]

Context

This strategy didn't work

# On Payment Charge Processed Server Script run as Guest
if doc.method == "ecommerce_integrations.ecwid.order.sync_order_from_payment_request":
 	ps = doc.flags.payment_session
 	if ps.changed:
 		payload = json.loads(doc.request_data)
 		returnUrl = payload["returnUrl"]
 		if ps.flags.status_changed_to in ps.flowstates.success:
 			si = frappe.new_doc("Sales Invoice")
 			si.allocate_advances_automatically = True
 			si = frappe.call(
          # Fails with a not-whitelisted error on Guest sessions
 			    "erpnext.selling.doctype.sales_order.sales_order.make_sales_invoice",
          source_name=doc.reference_docname,
 			    target_doc=si,
 			    ignore_permissions=True,
 			)
 			si.insert(ignore_permissions=True)
 			si.submit()
 			doc.flags.payment_result = {
 				"message": "Pago exitoso",
 				"action": {"href": returnUrl, "label": "Revisar Confirmacion"},
 			}
 		else:
 			doc.flags.payment_result = {
 				"message": "Pago fallido",
 				"action": {"href": returnUrl, "label": "Volver al Carrito"},
 			}

Now making things doc methods just for this is probably not the best option, but I couldn't find another sane way to achieve this.

This is probably a design flaw in how whitelisting is implemented in combination with servers scripts.

Non-Solutions include:

• Indiscriminate privilege escalation: frappe.set_user("Administrator")
• Use frappe client and hard-code (admin) passwords into server scripts
• ?

Maybe there's a more elegant way though, which wouldn't require ultimately an involved refactor to solve this problem at the root?

[sagarvora]

si = frappe.call(
          # Fails with a not-whitelisted error on Guest sessions
 			    "erpnext.selling.doctype.sales_order.sales_order.make_sales_invoice",
          source_name=doc.reference_docname,
 			    target_doc=si,
 			    ignore_permissions=True,
 			)

This won't work even if the method was allowed for guest. Passing ignore_permissions to a whitelisted function doesn't work when using frappe.call:
https://github.com/frappe/frappe/blob/28f575e254a57288b4865cd5cefd5153e10520e3/frappe/__init__.py#L1841

[blaggacao]

@sagarvora Correct, which is good. But this also the reason for this PR. I couldn't find another way to implement this. Overall, it seems a overly subtle and thus likely unintended distinction in behaviour between

class MyDocument(Document):

      @frappe.whitelisted()
      def make_something(self):  # this is *also* server script callable
           pass

@frappe.whitelisted()  # this is not
def make_something(*args):
     pass

If I had some design authority on the framework, my recommendation would be to clarify this situation through refactoring. Confusion and subtleness is rarely good for secure practices.

[blaggacao]

What about a (hypothetical, rust like) frappe.new_doc("My Doc").from("Sales Order", "SAL-ORD-...") which doesn't raise only if "My Doc" implements def from_sales_order(self, from_doc).

Which also implements so.into("My Doc"), automatically.

EDIT: or rather both should be implementable interchangably (either, or) so that the domain isn't violated, e.g. payments app can implement into for doctypes which are unbeknownst to erpnext while still implementing frappe.new_doc("My ERPnext Doctype").from("My Payments Doctype").

If I intuit this correctly, permission issues could be wholly encapsulated on the combination of both involved doctypes but without necessity for explicit overrides in most cases.

EDIT2: Here we go -> frappe/frappe#26991

[blaggacao]

For context on the motivation of this PR, see also: #40845 (comment)

[blaggacao]

Sem Grep Rule violation:

    erpnext/selling/doctype/sales_order/sales_order.py
   ❯❯❱ frappe-semgrep-rules.rules.require-permission-decorator-on-conversion-methods
          '_into_sales_invoice' in 'SalesOrder' crosses doctype boundaries. Explicitly declare its extended
          security context with @frappe.requires_permission(<doctype>, <perm>).                            
                                                                                                           
          744┆ def _into_sales_invoice(self):
          745┆    make_sales_invoice(self.name)
                            
  BLOCKING CODE RULES FIRED:
    frappe-semgrep-rules.rules.require-permission-decorator-on-conversion-methods

[blaggacao]

This is a bridge pattern, uncovering — thus far — hidden code smells and performance hits, and will disappear after a few years of using the document read only mode.

[blaggacao]

What the caterpillar calls the end of the world, the master calls a butterfly. - Richard Bach

---

_{Kindly help move this PR forward. Many thanks from Yours Sincerely!}

[rmehta]

where are these methods used?

[blaggacao]

This particular method can be used through their entry point(s) like so:

pe = frappe.new_doc_from("Payment Entry", "Sales Order", "SO-001")
# or
so = frappe.get_doc("Sales Order", "SO-001")
pe = so.into("Payment Entry")
# or
pe = frappe.new_doc("Payment Entry")
pe.flags.some_flag = True
# with either
pe.from_doc("Sales Order", "SO-001")
# or with
so = frappe.get_doc("Sales Order", "SO-001")
pe.from_doc(so)

Note, how this _from_* method also implements into() in the opposite direction.

While I did originally consider to mangle these methods __* so that they would be hard to directly invoke outside of the instance, that approach wasn't particularly forward-compatible with inheritance/overrides so I dropped it again and reverted to content with only a conventionalized _* private method marker.

It should be made clear from this, docs and initial code precedents that users are supposed to use these only through their entry point.

[blaggacao]

The decorators (@frappe.requires_permission) enable fine-grained access control at the doctype level, not at the record level, as we can't effectively declare record-level ACL in generic code.

This approach achieves two main objectives:

• It slightly enhances the principle of least privilege compared to using a blanket self.flags.ignore_permissions. However, the impact is somewhat limited because we only gain granularity in the permission dimension ("read" vs "write"), while doctype granularity was already implemented at the code level before.
• More significantly, it establishes a clear security context for conversion methods, which can be understood as a self-contained and well-defined abstraction. This allows code readers to rely on this declaration (assuming correct implementation). This second point has several important implications and benefits:
  • It improves code readability and maintainability by making security assumptions explicit.
  • It facilitates easier security audits and reviews.
  • It reduces the risk of unintended permission escalation or data exposure.
  • It promotes a more modular and secure code structure.
  • It enables better documentation * and understanding of the security model for each conversion method. _{* potentially automatable}
  • It thereby clarifies the nature and scope of core business logic transitions within the framework.

Ensuring the presence of such decorators throughout the code base (unfortunately without being able to check for semantic correctness), was the goal of this sem-grep rule: frappe/semgrep-rules#28

[blaggacao]

Additional note, which triggered this PR series:

Unlike combinations of make_* methods (even if whitelisted), this now works with server scripts, such as the post payment processing methods of: frappe/frappe#25952

Coincidentally, this is also the reason why this PR is heavy on the Payment Entry doctype which is most often involved in these particular hooks.

Code Example from our Production:

if doc.method == "ecommerce_integrations.ecwid.order.sync_order_from_payment_request":
 	ps = doc.flags.payment_session
 	if ps.changed and ps.is_success:
 		si = frappe.new_doc_from("Sales Invoice", doc.reference_doctype, doc.reference_docname, whitelist_permissions=True)
 		si.naming_series = "EC-INV-.YYYY.-"
 		si.flags.ignore_permissions=True
 		si.insert().submit()
 		gateway = ps.psl.get_gateway()
 		pe = frappe.new_doc_from("Payment Entry", si, whitelist_permissions=True)
 		pe.naming_series = "EC-PAY-.YYYY.-"
 		pe.paid_amount = ps.state.tx_data.amount
 		pe.receive_amount = ps.state.tx_data.amount
 		pe.reference_no = doc.name
 		pe.bank_account = gateway.payment_account
 		pe.flags.ignore_permissions=True
 		pe.insert().submit()
 	
 	payload = json.loads(doc.request_data)
 	returnUrl = payload["returnUrl"]
 	if ps.is_success:
 		doc.flags.payment_session.result = {
 			"message": "Pago exitoso",
 			"action": {
 			    "href": returnUrl,
 			    "label": "Revisar Confirmacion",
 			    "redirect_after_milliseconds": 5000,
 			},
 		}
 	else:
 		doc.flags.payment_session.result = {
 			"message": "Pago fallido",
 			"action": {
 			    "href": returnUrl,
 			    "label": "Volver al Carrito",
 			    "redirect_after_milliseconds": 5000,
 			},
 		}

Note the use of whitelist_permissions=True when escalating into the declared security scope.

[stale]

This pull request has been automatically marked as inactive because it has not had recent activity. It will be closed within 3 days if no further activity occurs, but it only takes a comment to keep a contribution alive :) Also, even if it is closed, you can always reopen the PR when you're ready. Thank you for contributing.