From f4062b4d7ad2e4a56ed165b7c2ef09fb9a0ba52e Mon Sep 17 00:00:00 2001 From: Sagar Vora Date: Wed, 19 Feb 2025 12:10:59 +0530 Subject: [PATCH] fix: ensure consistent error in response --- frappe/__init__.py | 11 +++++--- frappe/app.py | 3 +++ frappe/client.py | 8 ++++-- frappe/core/doctype/communication/email.py | 6 +++-- frappe/desk/form/load.py | 3 ++- frappe/exceptions.py | 10 +++---- frappe/handler.py | 25 ++++++++++------- frappe/model/delete_doc.py | 2 +- frappe/model/document.py | 11 ++++++-- frappe/permissions.py | 30 ++++++++++++++++++++- frappe/realtime.py | 5 ++-- frappe/tests/test_document.py | 2 +- frappe/utils/response.py | 17 +++++++----- frappe/website/doctype/web_form/web_form.py | 7 +++-- frappe/website/page_renderers/print_page.py | 7 +++-- frappe/website/serve.py | 26 ++++++++++-------- frappe/www/error.py | 7 +++-- frappe/www/printview.py | 24 ++++++++--------- 18 files changed, 131 insertions(+), 73 deletions(-) diff --git a/frappe/__init__.py b/frappe/__init__.py index ca8c61b0b0..9dd0a388e9 100644 --- a/frappe/__init__.py +++ b/frappe/__init__.py @@ -481,7 +481,7 @@ Action: TypeAlias = ServerAction | ClientAction def msgprint( msg: str, title: str | None = None, - raise_exception: bool | type[Exception] = False, + raise_exception: bool | type[Exception] | Exception = False, as_table: bool = False, as_list: bool = False, indicator: Literal["blue", "green", "orange", "red", "yellow"] | None = None, @@ -515,6 +515,9 @@ def msgprint( if raise_exception: if inspect.isclass(raise_exception) and issubclass(raise_exception, Exception): exc = raise_exception(msg) + elif isinstance(raise_exception, Exception): + exc = raise_exception + exc.args = (msg,) else: exc = ValidationError(msg) if out.__frappe_exc_id: @@ -590,7 +593,7 @@ def clear_last_message(): def throw( msg: str, - exc: type[Exception] = ValidationError, + exc: type[Exception] | Exception = ValidationError, title: str | None = None, is_minimizable: bool = False, wide: bool = False, @@ -987,6 +990,7 @@ def has_permission( *, parent_doctype=None, debug=False, + ignore_share_permissions=False, ): """ Return True if the user has permission `ptype` for given `doctype` or `doc`. @@ -1012,6 +1016,7 @@ def has_permission( print_logs=throw, parent_doctype=parent_doctype, debug=debug, + ignore_share_permissions=ignore_share_permissions, ) if throw and not out: @@ -1275,7 +1280,7 @@ def get_last_doc( if d: return get_doc(doctype, d[0], for_update=for_update) else: - raise DoesNotExistError + raise DoesNotExistError(doctype=doctype) def get_single(doctype): diff --git a/frappe/app.py b/frappe/app.py index 5fc5a5d956..2d0931af64 100644 --- a/frappe/app.py +++ b/frappe/app.py @@ -22,6 +22,7 @@ import frappe.utils.response from frappe import _ from frappe.auth import SAFE_HTTP_METHODS, UNSAFE_HTTP_METHODS, HTTPRequest, check_request_ip, validate_auth from frappe.middlewares import StaticDataMiddleware +from frappe.permissions import handle_does_not_exist_error from frappe.utils import CallbackManager, cint, get_site_name from frappe.utils.data import escape_html from frappe.utils.error import log_error, log_error_snapshot @@ -324,6 +325,8 @@ def make_form_dict(request: Request): def handle_exception(e): + e = handle_does_not_exist_error(e) + response = None http_status_code = getattr(e, "http_status_code", 500) accept_header = frappe.get_request_header("Accept") or "" diff --git a/frappe/client.py b/frappe/client.py index 0bebd93092..d7b1286b7a 100644 --- a/frappe/client.py +++ b/frappe/client.py @@ -484,13 +484,17 @@ def delete_doc(doctype, name): if frappe.is_table(doctype): values = frappe.db.get_value(doctype, name, ["parenttype", "parent", "parentfield"]) if not values: - raise frappe.DoesNotExistError + raise frappe.DoesNotExistError(doctype=doctype) + parenttype, parent, parentfield = values parent = frappe.get_doc(parenttype, parent) + if not parent.has_permission("write"): + raise frappe.DoesNotExistError(doctype=doctype) + for row in parent.get(parentfield): if row.name == name: parent.remove(row) - parent.save() + parent.save(ignore_permissions=True) break else: frappe.delete_doc(doctype, name, ignore_missing=False) diff --git a/frappe/core/doctype/communication/email.py b/frappe/core/doctype/communication/email.py index aefbe6f662..2bcba512d0 100755 --- a/frappe/core/doctype/communication/email.py +++ b/frappe/core/doctype/communication/email.py @@ -9,6 +9,7 @@ import frappe import frappe.email.smtp from frappe import _ from frappe.email.email_body import get_message_id +from frappe.permissions import check_doctype_permission from frappe.utils import ( cint, get_datetime, @@ -78,8 +79,9 @@ def make( category=DeprecationWarning, ) - if doctype and name and not frappe.has_permission(doctype=doctype, ptype="email", doc=name): - raise frappe.PermissionError(f"You are not allowed to send emails related to: {doctype} {name}") + if doctype and name: + doc = frappe.get_doc(doctype, name) + doc.check_permission("email") return _make( doctype=doctype, diff --git a/frappe/desk/form/load.py b/frappe/desk/form/load.py index ba8fc2ac24..065d286ddb 100644 --- a/frappe/desk/form/load.py +++ b/frappe/desk/form/load.py @@ -12,7 +12,7 @@ import frappe.utils from frappe import _, _dict from frappe.desk.form.document_follow import is_document_followed from frappe.model.utils.user_settings import get_user_settings -from frappe.permissions import get_doc_permissions, has_permission +from frappe.permissions import check_doctype_permission, get_doc_permissions, has_permission from frappe.utils.data import cstr if typing.TYPE_CHECKING: @@ -33,6 +33,7 @@ def getdoc(doctype, name): try: doc = frappe.get_doc(doctype, name) except frappe.DoesNotExistError: + check_doctype_permission(doctype) frappe.clear_last_message() return [] diff --git a/frappe/exceptions.py b/frappe/exceptions.py index 3f59b7a865..18c935fe76 100644 --- a/frappe/exceptions.py +++ b/frappe/exceptions.py @@ -44,6 +44,10 @@ class PermissionError(Exception): class DoesNotExistError(ValidationError): http_status_code = 404 + def __init__(self, *args, doctype=None): + super().__init__(*args) + self.doctype = doctype + class PageDoesNotExistError(ValidationError): http_status_code = 404 @@ -302,12 +306,6 @@ class LinkExpired(ValidationError): message = "The link has expired" -class InvalidKeyError(ValidationError): - http_status_code = 401 - title = "Invalid Key" - message = "The document key is invalid" - - class CommandFailedError(Exception): def __init__(self, message: str, out: str, err: str): super().__init__(message) diff --git a/frappe/handler.py b/frappe/handler.py index 64b15ccc7d..7f03e1db4f 100644 --- a/frappe/handler.py +++ b/frappe/handler.py @@ -13,6 +13,7 @@ import frappe.utils from frappe import _, is_whitelisted, ping from frappe.core.doctype.server_script.server_script_utils import get_server_script_map from frappe.monitor import add_data_to_monitor +from frappe.permissions import check_doctype_permission from frappe.utils import cint from frappe.utils.csvutils import build_csv_response from frappe.utils.deprecations import deprecated @@ -204,18 +205,22 @@ def upload_file(): def check_write_permission(doctype: str | None = None, name: str | None = None): - check_doctype = doctype and not name - if doctype and name: - try: - doc = frappe.get_doc(doctype, name) - doc.check_permission("write") - except frappe.DoesNotExistError: - # doc has not been inserted yet, name is set to "new-some-doctype" - # If doc inserts fine then only this attachment will be linked see file/utils.py:relink_mismatched_files - return + if not doctype: + return - if check_doctype: + if not name: frappe.has_permission(doctype, "write", throw=True) + return + + try: + doc = frappe.get_doc(doctype, name) + except frappe.DoesNotExistError: + # doc has not been inserted yet, name is set to "new-some-doctype" + # If doc inserts fine then only this attachment will be linked see file/utils.py:relink_mismatched_files + check_doctype_permission(doctype, "write") + return + + doc.check_permission("write") @frappe.whitelist(allow_guest=True) diff --git a/frappe/model/delete_doc.py b/frappe/model/delete_doc.py index 50b7b5bb1d..66c1e99289 100644 --- a/frappe/model/delete_doc.py +++ b/frappe/model/delete_doc.py @@ -55,7 +55,7 @@ def delete_doc( # already deleted..? if not frappe.db.exists(doctype, name): if not ignore_missing: - raise frappe.DoesNotExistError + raise frappe.DoesNotExistError(doctype=doctype) else: return False diff --git a/frappe/model/document.py b/frappe/model/document.py index 72a6b09ad1..0f12928524 100644 --- a/frappe/model/document.py +++ b/frappe/model/document.py @@ -254,7 +254,8 @@ class Document(BaseDocument, DocRef): if not d: frappe.throw( - _("{0} {1} not found").format(_(self.doctype), self.name), frappe.DoesNotExistError + _("{0} {1} not found").format(_(self.doctype), self.name), + frappe.DoesNotExistError(doctype=self.doctype), ) super().__init__(d) @@ -320,7 +321,7 @@ class Document(BaseDocument, DocRef): def check_permission(self, permtype="read", permlevel=None): """Raise `frappe.PermissionError` if not permitted""" if not self.has_permission(permtype): - self.raise_no_permission_to(permtype) + self._handle_permission_failure(permtype) def has_permission(self, permtype="read", *, debug=False, user=None) -> bool: """ @@ -336,6 +337,12 @@ class Document(BaseDocument, DocRef): return frappe.permissions.has_permission(self.doctype, permtype, self, debug=debug, user=user) + def _handle_permission_failure(self, perm_type): + from frappe.permissions import check_doctype_permission + + check_doctype_permission(self.doctype, perm_type) + self.raise_no_permission_to(perm_type) + def raise_no_permission_to(self, perm_type): """Raise `frappe.PermissionError`.""" frappe.flags.error_message = _( diff --git a/frappe/permissions.py b/frappe/permissions.py index c86067b067..3583589b3a 100644 --- a/frappe/permissions.py +++ b/frappe/permissions.py @@ -83,6 +83,7 @@ def has_permission( parent_doctype=None, print_logs=True, debug=False, + ignore_share_permissions=False, ) -> bool: """Return True if user has permission `ptype` for given `doctype`. If `doc` is passed, also check user, share and owner permissions. @@ -190,7 +191,7 @@ def has_permission( return False - if not perm: + if not perm and not ignore_share_permissions: debug and _debug_log("Checking if document/doctype is explicitly shared with user") perm = false_if_not_shared() @@ -833,3 +834,30 @@ def has_child_permission( def is_system_user(user: str | None = None) -> bool: return frappe.get_cached_value("User", user or frappe.session.user, "user_type") == "System User" + + +def check_doctype_permission(doctype: str, ptype: str = "read") -> None: + """ + Designed specfically to override DoesNotExistError in some scenarios. + Ignores share permissions. + """ + + _message_log = frappe.local.message_log + frappe.local.message_log = [] + try: + frappe.has_permission(doctype, ptype, throw=True, ignore_share_permissions=True) + except frappe.PermissionError: + frappe.flags.disable_traceback = True + raise + + frappe.local.message_log = _message_log + + +def handle_does_not_exist_error(e: Exception) -> None: + if not isinstance(e, frappe.DoesNotExistError) or not (doctype := getattr(e, "doctype", None)): + return e + + try: + check_doctype_permission(doctype) + except frappe.PermissionError as _e: + return _e diff --git a/frappe/realtime.py b/frappe/realtime.py index ce2f344e33..28f11abb9f 100644 --- a/frappe/realtime.py +++ b/frappe/realtime.py @@ -115,9 +115,8 @@ def emit_via_redis(event, message, room): @frappe.whitelist(allow_guest=True) def has_permission(doctype: str, name: str) -> bool: - if not frappe.has_permission(doctype=doctype, doc=name, ptype="read"): - raise frappe.PermissionError - + doc = frappe.get_doc(doctype, name) + doc.check_permission("read") return True diff --git a/frappe/tests/test_document.py b/frappe/tests/test_document.py index 6aa91487e6..e6fac5ef68 100644 --- a/frappe/tests/test_document.py +++ b/frappe/tests/test_document.py @@ -562,7 +562,7 @@ class TestDocumentWebView(IntegrationTestCase): # without key url_without_key = f"/ToDo/{todo.name}" - self.assertEqual(self.get(url_without_key).status, "404 NOT FOUND") + self.assertEqual(self.get(url_without_key).status, "403 FORBIDDEN") # Logged-in user can access the page without key self.assertEqual(self.get(url_without_key, "Administrator").status, "200 OK") diff --git a/frappe/utils/response.py b/frappe/utils/response.py index a9fb61e3ef..4fad55e3e1 100644 --- a/frappe/utils/response.py +++ b/frappe/utils/response.py @@ -34,11 +34,7 @@ def report_error(status_code): """Build error. Show traceback in developer mode""" from frappe.api import ApiVersion, get_api_version - allow_traceback = ( - (frappe.get_system_settings("allow_error_traceback") if frappe.db else False) - and not frappe.local.flags.disable_traceback - and (status_code != 404 or frappe.conf.logging) - ) + allow_traceback = is_traceback_allowed() and (status_code != 404 or frappe.conf.logging) traceback = frappe.utils.get_traceback() exc_type, exc_value, _ = sys.exc_info() @@ -62,6 +58,14 @@ def report_error(status_code): return response +def is_traceback_allowed(): + return ( + frappe.db + and frappe.get_system_settings("allow_error_traceback") + and not frappe.local.flags.disable_traceback + ) + + def _link_error_with_message_log(error_log, exception, message_logs): for message in list(message_logs): if message.get("__frappe_exc_id") == getattr(exception, "__frappe_exc_id", None): @@ -175,9 +179,8 @@ def _make_logs_v1(): from frappe.utils.error import guess_exception_source response = frappe.local.response - allow_traceback = frappe.get_system_settings("allow_error_traceback") if frappe.db else False - if frappe.error_log and allow_traceback: + if frappe.error_log and is_traceback_allowed(): if source := guess_exception_source(frappe.local.error_log and frappe.local.error_log[0]["exc"]): response["_exc_source"] = source response["exc"] = json.dumps([frappe.utils.cstr(d["exc"]) for d in frappe.local.error_log]) diff --git a/frappe/website/doctype/web_form/web_form.py b/frappe/website/doctype/web_form/web_form.py index 1678d24350..6d13dafe76 100644 --- a/frappe/website/doctype/web_form/web_form.py +++ b/frappe/website/doctype/web_form/web_form.py @@ -10,6 +10,7 @@ from frappe.core.api.file import get_max_file_size from frappe.core.doctype.file.utils import remove_file_by_url from frappe.desk.form.meta import get_code_files_via_hooks from frappe.modules.utils import export_module_json, get_doc_module +from frappe.permissions import check_doctype_permission from frappe.rate_limiter import rate_limit from frappe.utils import dict_with_keys, strip_html from frappe.utils.caching import redis_cache @@ -164,9 +165,11 @@ def get_context(context): ) if not frappe.db.exists(self.doc_type, frappe.form_dict.name): + check_doctype_permission(self.doc_type) raise frappe.PageDoesNotExistError() if not self.has_web_form_permission(self.doc_type, frappe.form_dict.name): + check_doctype_permission(self.doc_type) frappe.throw( _("You don't have the permissions to access this document"), frappe.PermissionError ) @@ -610,7 +613,7 @@ def accept(web_form, data): @frappe.whitelist() -def delete(web_form_name, docname): +def delete(web_form_name: str, docname: str): web_form = frappe.get_doc("Web Form", web_form_name) owner = frappe.db.get_value(web_form.doc_type, docname, "owner") @@ -621,7 +624,7 @@ def delete(web_form_name, docname): @frappe.whitelist() -def delete_multiple(web_form_name, docnames): +def delete_multiple(web_form_name: str, docnames: list[str]): web_form = frappe.get_doc("Web Form", web_form_name) docnames = json.loads(docnames) diff --git a/frappe/website/page_renderers/print_page.py b/frappe/website/page_renderers/print_page.py index e8a9d9d9a4..50e2d5704f 100644 --- a/frappe/website/page_renderers/print_page.py +++ b/frappe/website/page_renderers/print_page.py @@ -10,11 +10,10 @@ class PrintPage(TemplatePage): def can_render(self): parts = self.path.split("/", 1) - if len(parts) == 2: - if frappe.db.exists("DocType", parts[0], True) and frappe.db.exists(parts[0], parts[1], True): - return True + if len(parts) != 2 or not frappe.db.exists("DocType", parts[0], True): + return False - return False + return True def render(self): parts = self.path.split("/", 1) diff --git a/frappe/website/serve.py b/frappe/website/serve.py index 0c91f94abc..3df257aa25 100644 --- a/frappe/website/serve.py +++ b/frappe/website/serve.py @@ -1,6 +1,7 @@ from werkzeug.wrappers import Response import frappe +from frappe.permissions import handle_does_not_exist_error from frappe.website.page_renderers.error_page import ErrorPage from frappe.website.page_renderers.not_found_page import NotFoundPage from frappe.website.page_renderers.not_permitted_page import NotPermittedPage @@ -10,24 +11,27 @@ from frappe.website.path_resolver import PathResolver def get_response(path=None, http_status_code=200) -> Response: """Resolves path and renders page""" - response = None path = path or frappe.local.request.path endpoint = path try: path_resolver = PathResolver(path, http_status_code) endpoint, renderer_instance = path_resolver.resolve() - response = renderer_instance.render() - except frappe.Redirect as e: - return RedirectPage(endpoint or path, e.http_status_code).render() - except frappe.PermissionError as e: - response = NotPermittedPage(endpoint, http_status_code, exception=e).render() - except frappe.PageDoesNotExistError: - response = NotFoundPage(endpoint, http_status_code).render() - except Exception as e: - response = ErrorPage(exception=e).render() + return renderer_instance.render() - return response + except Exception as e: + e = handle_does_not_exist_error(e) + + if isinstance(e, frappe.Redirect): + return RedirectPage(endpoint or path, e.http_status_code).render() + + if isinstance(e, frappe.PermissionError): + return NotPermittedPage(endpoint, http_status_code, exception=e).render() + + if isinstance(e, frappe.PageDoesNotExistError): + return NotFoundPage(endpoint, http_status_code).render() + + return ErrorPage(exception=e).render() def get_response_content(path=None, http_status_code=200) -> str: diff --git a/frappe/www/error.py b/frappe/www/error.py index ef9f6a1beb..0de15fc6ab 100644 --- a/frappe/www/error.py +++ b/frappe/www/error.py @@ -2,6 +2,7 @@ # License: MIT. See LICENSE import frappe from frappe import _ +from frappe.utils.response import is_traceback_allowed no_cache = 1 @@ -10,15 +11,13 @@ def get_context(context): if frappe.flags.in_migrate: return - allow_traceback = frappe.get_system_settings("allow_error_traceback") if frappe.db else False - if frappe.local.flags.disable_traceback and not frappe.local.dev_server: - allow_traceback = False - if not context.title: context.title = _("Server Error") if not context.message: context.message = _("There was an error building this page") + allow_traceback = is_traceback_allowed() and not frappe.local.dev_server + return { "error": frappe.get_traceback().replace("<", "<").replace(">", ">") if allow_traceback else "" } diff --git a/frappe/www/printview.py b/frappe/www/printview.py index ebe382ecc5..83450d46e9 100644 --- a/frappe/www/printview.py +++ b/frappe/www/printview.py @@ -72,10 +72,6 @@ def get_context(context) -> PrintContext: print_format = get_print_format_doc(None, meta=meta) - make_access_log( - doctype=frappe.form_dict.doctype, document=frappe.form_dict.name, file_type="PDF", method="Print" - ) - body = get_rendered_template( doc, print_format=print_format, @@ -85,11 +81,14 @@ def get_context(context) -> PrintContext: letterhead=letterhead, settings=settings, ) - print_style = get_print_style(frappe.form_dict.style, print_format) + + make_access_log( + doctype=frappe.form_dict.doctype, document=frappe.form_dict.name, file_type="PDF", method="Print" + ) return { "body": body, - "print_style": print_style, + "print_style": get_print_style(frappe.form_dict.style, print_format), "comment": frappe.session.user, "title": frappe.utils.strip_html(cstr(doc.get_title() or doc.name)), "lang": frappe.local.lang, @@ -127,6 +126,9 @@ def get_rendered_template( trigger_print: bool = False, settings: dict | None = None, ) -> str: + if not frappe.flags.ignore_print_permissions: + validate_print_permission(doc) + print_settings = frappe.get_single("Print Settings").as_dict() print_settings.update(settings or {}) @@ -139,9 +141,6 @@ def get_rendered_template( doc.flags.in_print = True doc.flags.print_settings = print_settings - if not frappe.flags.ignore_print_permissions: - validate_print_permission(doc) - if doc.meta.is_submittable: if doc.docstatus.is_draft() and not cint(print_settings.allow_print_for_draft): frappe.throw(_("Not allowed to print draft documents"), frappe.PermissionError) @@ -382,11 +381,10 @@ def validate_print_permission(doc: "Document") -> None: if frappe.has_website_permission(doc): return - if (key := frappe.form_dict.key) and isinstance(key, str): - validate_key(key, doc) + if (key := frappe.form_dict.key) and isinstance(key, str) and validate_key(key, doc) is not False: return - frappe.throw(_("{0} {1} not found").format(_(doc.doctype), doc.name), frappe.DoesNotExistError) + doc._handle_permission_failure("print") def validate_key(key: str, doc: "Document") -> None: @@ -405,7 +403,7 @@ def validate_key(key: str, doc: "Document") -> None: if frappe.get_system_settings("allow_older_web_view_links") and key == doc.get_signature(): return - raise frappe.exceptions.InvalidKeyError + return False def get_letter_head(doc: "Document", no_letterhead: bool, letterhead: str | None = None) -> dict: