diff --git a/india_compliance/gst_india/overrides/company.py b/india_compliance/gst_india/overrides/company.py index a9c0d6ca4a..d6306c0e85 100644 --- a/india_compliance/gst_india/overrides/company.py +++ b/india_compliance/gst_india/overrides/company.py @@ -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 @@ -64,7 +65,25 @@ def make_default_gst_expense_accounts(company): @frappe.whitelist() def make_default_tax_templates(company: str, gst_rate: float | None = None): - frappe.has_permission("Company", ptype="write", doc=company, throw=True) + # 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. + # The try/except guards against a TOCTOU race condition where the record is + # deleted between the existence check above and the get_doc() call inside + # has_permission — ensuring DoesNotExistError never surfaces to the client. + try: + frappe.has_permission("Company", ptype="write", doc=company, throw=True) + except frappe.DoesNotExistError: + frappe.throw( + _("Company {0} no longer exists. It may have been deleted.").format(frappe.bold(company)) + ) default_taxes = get_tax_defaults(gst_rate) from_detailed_data(company, default_taxes) diff --git a/india_compliance/gst_india/overrides/test_company.py b/india_compliance/gst_india/overrides/test_company.py index a8e7bd7b27..3891bab4d8 100644 --- a/india_compliance/gst_india/overrides/test_company.py +++ b/india_compliance/gst_india/overrides/test_company.py @@ -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): @@ -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) diff --git a/india_compliance/gst_india/utils/__init__.py b/india_compliance/gst_india/utils/__init__.py index 5e1920f9e5..0a3740b92a 100644 --- a/india_compliance/gst_india/utils/__init__.py +++ b/india_compliance/gst_india/utils/__init__.py @@ -100,7 +100,26 @@ 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. + # The try/except guards against a TOCTOU race condition where the record is + # deleted between the existence check above and the get_doc() call inside + # has_permission — ensuring DoesNotExistError never surfaces to the client. + try: + frappe.has_permission(party_type, ptype="read", doc=party, throw=True) + except frappe.DoesNotExistError: + return [] filters = { "link_doctype": party_type, diff --git a/india_compliance/gst_india/utils/test_utils.py b/india_compliance/gst_india/utils/test_utils.py index b3cf5c7869..374b7b049b 100644 --- a/india_compliance/gst_india/utils/test_utils.py +++ b/india_compliance/gst_india/utils/test_utils.py @@ -10,6 +10,7 @@ class TestUtils(IntegrationTestCase): @classmethod def setUpClass(cls): super().setUpClass() + frappe.db.savepoint("before_test_utils") # create old fiscal years fiscal_year = frappe.new_doc("Fiscal Year") @@ -30,6 +31,36 @@ def setUpClass(cls): } ).insert(ignore_if_duplicate=True) + # Restricted user for permission boundary tests + cls.restricted_user = "test_no_perms@example.com" + if not frappe.db.exists("User", cls.restricted_user): + frappe.get_doc( + { + "doctype": "User", + "email": cls.restricted_user, + "first_name": "Test", + "send_welcome_email": 0, + } + ).insert(ignore_permissions=True) + + # User with Purchase Manager role (has Supplier read access) + cls.purchase_user = "test_purchase_mgr@example.com" + if not frappe.db.exists("User", cls.purchase_user): + frappe.get_doc( + { + "doctype": "User", + "email": cls.purchase_user, + "first_name": "Purchase", + "send_welcome_email": 0, + "roles": [{"role": "Purchase Manager"}], + } + ).insert(ignore_permissions=True) + + @classmethod + def tearDownClass(cls): + frappe.set_user("Administrator") + frappe.db.rollback(save_point="before_test_utils") + @patch("india_compliance.gst_india.utils.getdate", return_value=getdate("2023-06-20")) def test_timespan_date_range(self, getdate_mock): from india_compliance.gst_india.utils import get_timespan_date_range @@ -46,3 +77,50 @@ 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) + + def test_get_gstin_list_returns_list_for_saved_document(self): + """get_gstin_list should return a list (not raise) for a real saved Supplier.""" + from india_compliance.gst_india.utils import get_gstin_list + + original_user = frappe.session.user + try: + frappe.set_user(self.purchase_user) + # Uses a Supplier fixture defined in india_compliance/tests/test_records.json + result = get_gstin_list("_Test Registered Supplier", "Supplier") + self.assertIsInstance(result, list) + finally: + frappe.set_user(original_user) + + def test_get_gstin_list_raises_permission_error_for_restricted_user(self): + """get_gstin_list should raise PermissionError for users without doctype access.""" + from india_compliance.gst_india.utils import get_gstin_list + + original_user = frappe.session.user + try: + frappe.set_user(self.restricted_user) + self.assertRaises(frappe.PermissionError, get_gstin_list, "new-supplier-1", "Supplier") + finally: + frappe.set_user(original_user) + + def test_make_default_tax_templates_raises_permission_error_for_restricted_user(self): + """make_default_tax_templates should raise PermissionError for users without write access.""" + from india_compliance.gst_india.overrides.company import make_default_tax_templates + + original_user = frappe.session.user + try: + frappe.set_user(self.restricted_user) + self.assertRaises(frappe.PermissionError, make_default_tax_templates, "new-company-1") + finally: + frappe.set_user(original_user)