Skip to content

fix: gracefully handle ephemeral document permissions during GST validation #4167

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

Open
Aakash1o1 wants to merge 6 commits into
base: develop
Choose a base branch
from
+133 −3
Open

fix: gracefully handle ephemeral document permissions during GST validation #4167

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
7fcf787
fix: gracefully handle ephemeral document permissions during GST vali…
Aakash-Gupta101 Apr 2, 2026
066e9b4
fix: refine ephemeral document detection to avoid false positives
Aakash-Gupta101 Apr 2, 2026
b874a34
fix: address AI review feedback for PR #4166
Aakash-Gupta101 Apr 2, 2026
1b53488
fix: add multi-role permission boundary test for get_gstin_list
Aakash-Gupta101 Apr 3, 2026
5324341
fix: close TOCTOU race condition in GST permission checks
Aakash-Gupta101 Apr 3, 2026
5009ea2
fix: resolve all audit violations in GST permission checks
Aakash-Gupta101 Apr 3, 2026
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
11 changes: 11 additions & 0 deletions india_compliance/gst_india/overrides/company.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
from erpnext.setup.setup_wizard.operations.taxes_setup import (
from_detailed_data,
)
from frappe import _
from frappe.utils import flt

from india_compliance.gst_india.utils import get_data_file_path
Expand Down Expand Up @@ -64,6 +65,16 @@ def make_default_gst_expense_accounts(company):

@frappe.whitelist()
def make_default_tax_templates(company: str, gst_rate: float | None = None):
# 1. Doctype-level permission check first (no doc lookup, safe from disclosure)
frappe.has_permission("Company", ptype="write", doc=None, throw=True)

# 2. Gracefully handle unsaved or missing documents.
# Case A: ephemeral UI name (new-*) that isn't a real saved record
# Case B: record deleted by another user or a stale/invalid name (race condition guard)
if not frappe.db.exists("Company", company):
frappe.throw(_("Please save the Company before creating tax templates."))

# 3. Record confirmed to exist — run full document-level permission check
frappe.has_permission("Company", ptype="write", doc=company, throw=True)

default_taxes = get_tax_defaults(gst_rate)
Expand Down
16 changes: 15 additions & 1 deletion india_compliance/gst_india/overrides/test_company.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import frappe
from frappe.tests import IntegrationTestCase

from india_compliance.gst_india.overrides.company import get_tax_defaults
from india_compliance.gst_india.overrides.company import get_tax_defaults, make_default_tax_templates


class TestCompanyFixtures(IntegrationTestCase):
Expand Down Expand Up @@ -34,6 +34,20 @@ def test_tax_defaults_setup(self):
# Check for tax category creations.
self.assertTrue(frappe.db.exists("Tax Category", "Reverse Charge In-State"))

def test_make_default_tax_templates_with_ephemeral_company(self):
"""make_default_tax_templates should throw a user-friendly error for unsaved companies."""
original_user = frappe.session.user
try:
frappe.set_user("Administrator")
self.assertRaisesRegex(
frappe.ValidationError,
"Please save the Company before creating tax templates.",
make_default_tax_templates,
"new-company-1",
)
finally:
frappe.set_user(original_user)

def test_get_tax_defaults(self):
gst_rate = 12
default_taxes = get_tax_defaults(gst_rate)
Expand Down
15 changes: 14 additions & 1 deletion india_compliance/gst_india/utils/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,20 @@ def get_gstin_list(party: str, party_type: str = "Company", exclude_isd: bool =
"""
Returns a list the party's GSTINs.
"""
frappe.has_permission(party_type, doc=party, throw=True)
# 1. Doctype-level permission check first (no doc lookup, safe from disclosure)
frappe.has_permission(party_type, ptype="read", doc=None, throw=True)

# 2. Gracefully handle unsaved or missing documents.
# Returning [] is an intentional architectural choice: the frontend calls this
# function in real-time as the user types, including before the document is saved.
# An empty list signals "no GSTINs yet" without blocking the UI or leaking existence info.
# Case A: ephemeral UI name (new-*) that isn't a real saved record
# Case B: record deleted by another user or a stale/invalid name (race condition guard)
if not frappe.db.exists(party_type, party):
return []

# 3. Record confirmed to exist — run full document-level permission check
frappe.has_permission(party_type, ptype="read", doc=party, throw=True)
Copy link
Copy Markdown

@greptile-apps greptile-apps bot Apr 2, 2026

Choose a reason for hiding this comment

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

P1 Residual TOCTOU window in document-level permission check

The db.exists() guard in step 2 closes the ephemeral-name case, but the document-level has_permission call on line 116 still passes doc=party as a string. Frappe resolves a string doc by calling frappe.get_doc(doctype, name) internally, so if the record is deleted between the db.exists() check and this call (a race condition), the original DoesNotExistError can still surface — the very error this PR is intended to prevent.

For get_gstin_list the impact is low (the subsequent DB queries would simply return no rows anyway), but the error would still propagate as an unhandled 404. Consider wrapping the step-3 check in a guard, or catching frappe.DoesNotExistError:

    # 3. Record confirmed to exist — run full document-level permission check
    try:
        frappe.has_permission(party_type, ptype="read", doc=party, throw=True)
    except frappe.DoesNotExistError:
        return []

The same pattern applies to make_default_tax_templates in company.py (line 78).

Copy link
Copy Markdown

@greptile-apps greptile-apps bot Apr 2, 2026

Choose a reason for hiding this comment

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

P2 Verbose inline comments add noise to production code

The numbered step headings and multi-line rationale blocks (e.g. "Returning [] is an intentional architectural choice: the frontend calls this function in real-time…", the Case A / Case B explanations) are thorough design documentation, but they are more appropriate in the PR description or a contributing guide than in production source code. Consider condensing to a brief single-line comment per step:

    # Doctype-level permission check (no doc lookup)
    frappe.has_permission(party_type, ptype="read", doc=None, throw=True)

    # Return early for ephemeral/deleted documents instead of raising
    if not frappe.db.exists(party_type, party):
        return []

    frappe.has_permission(party_type, ptype="read", doc=party, throw=True)

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!


filters = {
"link_doctype": party_type,
Expand Down
12 changes: 12 additions & 0 deletions india_compliance/gst_india/utils/test_utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,3 +46,15 @@ def test_timespan_date_range(self, getdate_mock):

for i, expected_date in enumerate(expected_date_range):
self.assertEqual(expected_date, actual_date_range[i])

def test_get_gstin_list_with_ephemeral_document(self):
"""get_gstin_list should return empty list for unsaved (ephemeral) documents."""
from india_compliance.gst_india.utils import get_gstin_list

original_user = frappe.session.user
try:
frappe.set_user("Administrator")
result = get_gstin_list("new-supplier-1", "Supplier")
self.assertEqual(result, [])
finally:
frappe.set_user(original_user)
Comment on lines +81 to +91
Copy link
Copy Markdown

@greptile-apps greptile-apps bot Apr 2, 2026

Choose a reason for hiding this comment

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

P2 Test only exercises the Administrator path

The test sets the user to "Administrator" before calling get_gstin_list. In Frappe, Administrator bypasses most row-level permission checks, so this test does not verify the behaviour for a regular user who has doctype-level read permission but no access to a specific Supplier document. It is worth adding a case with a guest or restricted user to confirm that the doctype-level check (step 1) still fires and raises PermissionError when the user has no access, ensuring the early-return of [] does not silently bypass authorization for non-admin users.