Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: render doctype caster server-script-callable #42160

Closed
wants to merge 6 commits into from
Closed

fix: render doctype caster server-script-callable #42160

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
1b40ce4
feat(so): render make_sales_invoice server-script-callable
blaggacao Jul 3, 2024
0a8d711
feat(pe): render make_payment_entry server-script-callable
blaggacao Jul 4, 2024
887789e
refactor: adopt new and shiny document caster
blaggacao Jul 4, 2024
43fdb97
ci: preview; DO NOT MERGE
blaggacao Jul 4, 2024
b5c4b49
fix: security context as per semgrep rule
blaggacao Jul 4, 2024
67994de
fix: document read only mode during conversion
blaggacao Jul 7, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/workflows/linters.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ jobs:
cache: pip

- name: Download Semgrep rules
run: git clone --depth 1 https://github.com/frappe/semgrep-rules.git frappe-semgrep-rules
run: git clone --depth 1 https://github.com/blaggacao/semgrep-rules.git frappe-semgrep-rules

- name: Download semgrep
run: pip install semgrep
Expand Down
2 changes: 2 additions & 0 deletions .github/workflows/patch.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,8 @@ jobs:
env:
DB: mariadb
TYPE: server
FRAPPE_USER: blaggacao
FRAPPE_BRANCH: feat/document-caster

- name: Run Patch Tests
run: |
Expand Down
4 changes: 2 additions & 2 deletions .github/workflows/server-tests-mariadb.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,8 +116,8 @@ jobs:
env:
DB: mariadb
TYPE: server
FRAPPE_USER: ${{ github.event.inputs.user }}
FRAPPE_BRANCH: ${{ github.event.inputs.branch }}
FRAPPE_USER: blaggacao
FRAPPE_BRANCH: feat/document-caster

- name: Run Tests
run: 'cd ~/frappe-bench/ && bench --site test_site run-parallel-tests --app erpnext --total-builds 4 --build-number ${{ matrix.container }}'
Expand Down
2 changes: 2 additions & 0 deletions .github/workflows/server-tests-postgres.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,8 @@ jobs:
env:
DB: postgres
TYPE: server
FRAPPE_USER: blaggacao
FRAPPE_BRANCH: feat/document-caster

- name: Run Tests
run: cd ~/frappe-bench/ && bench --site test_site run-parallel-tests --app erpnext --use-orchestrator
Expand Down
62 changes: 60 additions & 2 deletions erpnext/accounts/doctype/payment_entry/payment_entry.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -525,7 +525,10 @@ def set_missing_ref_details(
continue

if field == "exchange_rate" or not d.get(field) or force:
d.db_set(field, value)
if self.get("_action") in ("submit", "cancel"):
d.db_set(field, value)
else:
d.set(field, value)
Comment on lines -528 to +531
Copy link
Collaborator Author

@blaggacao blaggacao Jul 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


def validate_payment_type(self):
if self.payment_type not in ("Receive", "Pay", "Internal Transfer"):
Expand Down Expand Up @@ -1170,6 +1173,61 @@ def set_remarks(self):

self.set("remarks", "\n".join(remarks))

@frappe.requires_permission("Account", "read")
@frappe.requires_permission("Sales Order", "read")
@frappe.requires_permission("Payment Entry", "create")
def _from_sales_order(self, so):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

where are these methods used?

Copy link
Collaborator Author

@blaggacao blaggacao Jul 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

@blaggacao blaggacao Jul 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

@blaggacao blaggacao Jul 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return get_payment_entry(
so.doctype,
so.name,
payment_type="Receive",
party_type="Customer",
)

@frappe.requires_permission("Account", "read")
@frappe.requires_permission("Sales Invoice", "read")
@frappe.requires_permission("Payment Entry", "create")
def _from_sales_invoice(self, si):
frappe.flags.new_payment_entry = self
return get_payment_entry(
si.doctype,
si.name,
payment_type="Receive",
party_type="Customer",
)

@frappe.requires_permission("Account", "read")
@frappe.requires_permission("Purchase Order", "read")
@frappe.requires_permission("Payment Entry", "create")
def _from_purchase_order(self, po):
frappe.flags.new_payment_entry = self
return get_payment_entry(
po.doctype,
po.name,
payment_type="Receive",
party_type="Supplier",
)

@frappe.requires_permission("Account", "read")
@frappe.requires_permission("Purchase Invoice", "read")
@frappe.requires_permission("Payment Entry", "create")
def _from_purchase_invoice(self, pi):
frappe.flags.new_payment_entry = self
return get_payment_entry(
pi.doctype,
pi.name,
payment_type="Receive",
party_type="Supplier",
)

@frappe.requires_permission("Account", "read")
@frappe.requires_permission("Dunning", "read")
@frappe.requires_permission("Payment Entry", "create")
def _from_dunning(self, d):
frappe.flags.ignore_account_permission = frappe.flags.ignore_permissions
frappe.flags.new_payment_entry = self
return get_payment_entry(d.doctype, d.name)

def build_gl_map(self):
if self.payment_type in ("Receive", "Pay") and not self.get("party_account_field"):
self.setup_party_account_field()
Expand Down Expand Up @@ -2350,7 +2408,7 @@ def get_payment_entry(
paid_amount, received_amount, doc, party_account_currency, reference_date
)

pe = frappe.new_doc("Payment Entry")
pe = frappe.flags.new_payment_entry or frappe.new_doc("Payment Entry")
pe.payment_type = payment_type
pe.company = doc.company
pe.cost_center = doc.get("cost_center")
Expand Down
5 changes: 5 additions & 0 deletions erpnext/selling/doctype/sales_order/sales_order.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -741,6 +741,11 @@ def cancel_stock_reservation_entries(self, sre_list=None, notify=True) -> None:
voucher_type=self.doctype, voucher_no=self.name, sre_list=sre_list, notify=notify
)

@frappe.requires_permission("Sales Order", "read")
@frappe.requires_permission("Sales Invoice", "create")
def _into_sales_invoice(self, si):
make_sales_invoice(self.name, target_doc=si)


def get_unreserved_qty(item: object, reserved_qty_details: dict) -> float:
"""Returns the unreserved quantity for the Sales Order Item."""
Expand Down
Loading