diff --git a/cypress/integration/query_report.js b/cypress/integration/query_report.js index 5dd0ab2d53..4a2d753ff0 100644 --- a/cypress/integration/query_report.js +++ b/cypress/integration/query_report.js @@ -38,7 +38,7 @@ context("Query Report", () => { .contains("Add Column") .click({ force: true }); cy.get_open_dialog().get(".modal-title").should("contain", "Add Column"); - cy.get('select[data-fieldname="doctype"]').select("Role", { force: true }); + cy.get('select[data-fieldname="doctype"]').select("Role (Name)", { force: true }); cy.get('select[data-fieldname="field"]').select("Role Name", { force: true }); cy.get('select[data-fieldname="insert_after"]').select("Name", { force: true }); cy.get_open_dialog() diff --git a/frappe/__init__.py b/frappe/__init__.py index 98582c76de..0828630548 100644 --- a/frappe/__init__.py +++ b/frappe/__init__.py @@ -482,7 +482,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, @@ -516,6 +516,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: @@ -591,7 +594,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, @@ -988,6 +991,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`. @@ -1013,6 +1017,7 @@ def has_permission( print_logs=throw, parent_doctype=parent_doctype, debug=debug, + ignore_share_permissions=ignore_share_permissions, ) if throw and not out: @@ -1276,7 +1281,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 6ed100792d..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 @@ -242,7 +243,7 @@ def process_response(response: Response): return # Default for all requests is no-cache unless explicitly opted-in by endpoint - response.headers.update(NO_CACHE_HEADERS) + response.headers.setdefault("Cache-Control", NO_CACHE_HEADERS["Cache-Control"]) # rate limiter headers if hasattr(frappe.local, "rate_limiter"): @@ -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/desktop.py b/frappe/desk/desktop.py index a1969cb3d8..738de62108 100644 --- a/frappe/desk/desktop.py +++ b/frappe/desk/desktop.py @@ -565,6 +565,7 @@ def get_custom_report_list(module): else 0, "label": _(r.name), "link_to": r.name, + "report_ref_doctype": r.ref_doctype, } for r in reports ] diff --git a/frappe/desk/doctype/system_health_report/system_health_report.py b/frappe/desk/doctype/system_health_report/system_health_report.py index abdc1b0272..9cd74546af 100644 --- a/frappe/desk/doctype/system_health_report/system_health_report.py +++ b/frappe/desk/doctype/system_health_report/system_health_report.py @@ -27,7 +27,7 @@ from frappe.model.document import Document from frappe.utils.background_jobs import get_queue, get_queue_list, get_redis_conn from frappe.utils.caching import redis_cache from frappe.utils.data import add_to_date -from frappe.utils.scheduler import get_scheduler_status, get_scheduler_tick +from frappe.utils.scheduler import get_scheduler_status, get_scheduler_tick, is_schduler_process_running @contextmanager @@ -185,7 +185,8 @@ class SystemHealthReport(Document): lower_threshold = add_to_date(None, days=-7, as_datetime=True) # Exclude "maybe" curently executing job upper_threshold = add_to_date(None, minutes=-30, as_datetime=True) - self.scheduler_status = get_scheduler_status().get("status") + scheduler_running = get_scheduler_status().get("status") == "active" and is_schduler_process_running() + self.scheduler_status = "Active" if scheduler_running else "Inactive" mariadb_query = """ SELECT scheduled_job_type, diff --git a/frappe/desk/form/load.py b/frappe/desk/form/load.py index 79b35bac4a..065d286ddb 100644 --- a/frappe/desk/form/load.py +++ b/frappe/desk/form/load.py @@ -9,12 +9,11 @@ import frappe import frappe.defaults import frappe.desk.form.meta import frappe.utils -from frappe import _dict +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 -from frappe.utils.html_utils import clean_email_html if typing.TYPE_CHECKING: from frappe.model.document import Document @@ -34,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 [] @@ -263,7 +263,6 @@ def _get_communications(doctype, name, start=0, limit=20): communications = get_communication_data(doctype, name, start, limit) for c in communications: if c.communication_type in ("Communication", "Automated Message"): - clean_email_html(c.content) c.attachments = json.dumps( frappe.get_all( "File", diff --git a/frappe/email/doctype/email_account/email_account.py b/frappe/email/doctype/email_account/email_account.py index 727d3145c0..9e5d4a26ee 100755 --- a/frappe/email/doctype/email_account/email_account.py +++ b/frappe/email/doctype/email_account/email_account.py @@ -203,7 +203,7 @@ class EmailAccount(Document): def validate_frappe_mail_settings(self): if self.service == "Frappe Mail": frappe_mail_client = self.get_frappe_mail_client() - frappe_mail_client.validate(for_inbound=self.enable_incoming, for_outbound=self.enable_outgoing) + frappe_mail_client.validate() def validate_smtp_conn(self): if not self.smtp_server: diff --git a/frappe/email/frappemail.py b/frappe/email/frappemail.py index e653d46039..adef2d7624 100644 --- a/frappe/email/frappemail.py +++ b/frappe/email/frappemail.py @@ -15,24 +15,22 @@ class FrappeMail: def __init__( self, site: str, - mailbox: str, + email: str, api_key: str | None = None, api_secret: str | None = None, access_token: str | None = None, ) -> None: self.site = site - self.mailbox = mailbox + self.email = email self.api_key = api_key self.api_secret = api_secret self.access_token = access_token - self.client = self.get_client( - self.site, self.mailbox, self.api_key, self.api_secret, self.access_token - ) + self.client = self.get_client(self.site, self.email, self.api_key, self.api_secret, self.access_token) @staticmethod def get_client( site: str, - mailbox: str, + email: str, api_key: str | None = None, api_secret: str | None = None, access_token: str | None = None, @@ -40,7 +38,7 @@ class FrappeMail: """Returns a FrappeClient or FrappeOAuth2Client instance.""" if hasattr(frappe.local, "frappe_mail_clients"): - if client := frappe.local.frappe_mail_clients.get(mailbox): + if client := frappe.local.frappe_mail_clients.get(email): return client else: frappe.local.frappe_mail_clients = {} @@ -50,7 +48,7 @@ class FrappeMail: if access_token else FrappeClient(url=site, api_key=api_key, api_secret=api_secret) ) - frappe.local.frappe_mail_clients[mailbox] = client + frappe.local.frappe_mail_clients[email] = client return client @@ -88,11 +86,11 @@ class FrappeMail: return self.client.post_process(response) - def validate(self, for_outbound: bool = False, for_inbound: bool = False) -> None: - """Validates the mailbox for inbound and outbound emails.""" + def validate(self) -> None: + """Validates if the user is allowed to send or receive emails.""" - endpoint = "/api/method/mail_client.api.auth.validate" - data = {"mailbox": self.mailbox, "for_outbound": for_outbound, "for_inbound": for_inbound} + endpoint = "/api/method/mail.api.auth.validate" + data = {"email": self.email} self.request("POST", endpoint=endpoint, data=data) def send_raw( @@ -100,18 +98,18 @@ class FrappeMail: ) -> None: """Sends an email using the Frappe Mail API.""" - endpoint = "/api/method/mail_client.api.outbound.send_raw" + endpoint = "/api/method/mail.api.outbound.send_raw" data = {"from_": sender, "to": recipients, "is_newsletter": is_newsletter} self.request("POST", endpoint=endpoint, data=data, files={"raw_message": message}) def pull_raw(self, limit: int = 50, last_synced_at: str | None = None) -> dict[str, str | list[str]]: - """Pulls emails from the mailbox using the Frappe Mail API.""" + """Pulls emails for the email using the Frappe Mail API.""" - endpoint = "/api/method/mail_client.api.inbound.pull_raw" + endpoint = "/api/method/mail.api.inbound.pull_raw" if last_synced_at: last_synced_at = add_or_update_tzinfo(last_synced_at) - data = {"mailbox": self.mailbox, "limit": limit, "last_synced_at": last_synced_at} + data = {"email": self.email, "limit": limit, "last_synced_at": last_synced_at} headers = {"X-Site": frappe.utils.get_url()} response = self.request("GET", endpoint=endpoint, data=data, headers=headers) last_synced_at = convert_utc_to_system_timezone(get_datetime(response["last_synced_at"])) 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..b502470965 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,31 @@ 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) -> Exception: + if isinstance(e, frappe.DoesNotExistError) and (doctype := getattr(e, "doctype", None)): + try: + check_doctype_permission(doctype) + + except frappe.PermissionError as _e: + return _e + + return e diff --git a/frappe/public/js/frappe/form/form.js b/frappe/public/js/frappe/form/form.js index e420359655..9fef63f231 100644 --- a/frappe/public/js/frappe/form/form.js +++ b/frappe/public/js/frappe/form/form.js @@ -1954,7 +1954,7 @@ frappe.ui.form.Form = class FrappeForm { if (this.can_make_methods && this.can_make_methods[doctype]) { return this.can_make_methods[doctype](this); } else { - if (this.meta.is_submittable && !this.doc.docstatus == 1) { + if (this.meta.is_submittable && this.doc.docstatus !== 1) { return false; } else { return true; diff --git a/frappe/public/js/frappe/form/templates/timeline_message_box.html b/frappe/public/js/frappe/form/templates/timeline_message_box.html index 8d5fcc1efd..0242c00fdd 100644 --- a/frappe/public/js/frappe/form/templates/timeline_message_box.html +++ b/frappe/public/js/frappe/form/templates/timeline_message_box.html @@ -65,8 +65,8 @@ {% if (doc._url) { %} - - + + {% } %} diff --git a/frappe/public/js/frappe/list/list_view.js b/frappe/public/js/frappe/list/list_view.js index a470f47260..7237485af3 100644 --- a/frappe/public/js/frappe/list/list_view.js +++ b/frappe/public/js/frappe/list/list_view.js @@ -535,7 +535,11 @@ frappe.views.ListView = class ListView extends frappe.views.BaseList { before_refresh() { if (frappe.route_options && this.filter_area) { this.filters = this.parse_filters_from_route_options(); - this.add_recent_filter_on_large_tables(); + if (!this.filters.length || window.location.search) { + // Add recency filters if route options are not used + // Route options are internally used in connections to filter for specific documents. + this.add_recent_filter_on_large_tables(); + } frappe.route_options = null; if (this.filters.length > 0) { diff --git a/frappe/public/js/frappe/utils/utils.js b/frappe/public/js/frappe/utils/utils.js index a5370b89b0..eabf37dada 100644 --- a/frappe/public/js/frappe/utils/utils.js +++ b/frappe/public/js/frappe/utils/utils.js @@ -1304,7 +1304,8 @@ Object.assign(frappe.utils, { if (item.is_query_report) { route = "query-report/" + item.name; } else if (!item.is_query_report && item.report_ref_doctype) { - route = frappe.router.slug(item.report_ref_doctype) + "/view/report/"; + route = + frappe.router.slug(item.report_ref_doctype) + "/view/report/" + item.name; } else { route = "report/" + item.name; } diff --git a/frappe/public/js/frappe/views/reports/query_report.js b/frappe/public/js/frappe/views/reports/query_report.js index 0ddf59109b..5a89bdabfe 100644 --- a/frappe/public/js/frappe/views/reports/query_report.js +++ b/frappe/public/js/frappe/views/reports/query_report.js @@ -1706,11 +1706,16 @@ frappe.views.QueryReport = class QueryReport extends frappe.views.BaseList { fieldname: "doctype", label: __("From Document Type"), options: this.linked_doctypes?.map((df) => ({ - label: df.doctype, - value: df.doctype, + label: df.doctype + " (" + frappe.unscrub(df.fieldname) + ")", + value: JSON.stringify({ + doctype: df.doctype, + fieldname: df.fieldname, + }), })), change: () => { - let doctype = d.get_value("doctype"); + const { doctype, fieldname } = JSON.parse( + d.get_value("doctype") + ); frappe.model.with_doctype(doctype, () => { let options = frappe.meta .get_docfields(doctype) @@ -1751,6 +1756,8 @@ frappe.views.QueryReport = class QueryReport extends frappe.views.BaseList { ], primary_action: (values) => { const custom_columns = []; + const { doctype, fieldname } = JSON.parse(values.doctype); + Object.assign(values, { doctype, fieldname }); let df = frappe.meta.get_docfield(values.doctype, values.field); const insert_after_index = this.columns.findIndex( (column) => column.label === values.insert_after @@ -1778,17 +1785,14 @@ frappe.views.QueryReport = class QueryReport extends frappe.views.BaseList { field: values.field, doctype: values.doctype, names: Array.from( - this.doctype_field_map[values.doctype].names + this.doctype_field_map[values.doctype][fieldname] ), }, callback: (r) => { const custom_data = r.message; - const link_field = - this.doctype_field_map[values.doctype].fieldname; this.add_custom_column( custom_columns, custom_data, - link_field, values, insert_after_index ); @@ -1870,13 +1874,7 @@ frappe.views.QueryReport = class QueryReport extends frappe.views.BaseList { } } - add_custom_column( - custom_column, - custom_data, - link_field, - new_column_data, - insert_after_index - ) { + add_custom_column(custom_column, custom_data, new_column_data, insert_after_index) { const column = this.prepare_columns(custom_column); const column_field = new_column_data.field; @@ -1885,9 +1883,9 @@ frappe.views.QueryReport = class QueryReport extends frappe.views.BaseList { this.data.forEach((row) => { if (column[0].fieldname.includes("-")) { row[column_field + "-" + frappe.scrub(new_column_data.doctype)] = - custom_data[row[link_field]]; + custom_data[row[new_column_data.fieldname]]; } else { - row[column_field] = custom_data[row[link_field]]; + row[column_field] = custom_data[row[new_column_data.fieldname]]; } }); @@ -1931,14 +1929,18 @@ frappe.views.QueryReport = class QueryReport extends frappe.views.BaseList { }; }) ); - doctypes.forEach((doc) => { - this.doctype_field_map[doc.doctype] = { fieldname: doc.fieldname, names: new Set() }; + if (!(doc.doctype in this.doctype_field_map)) + this.doctype_field_map[doc.doctype] = { [doc.fieldname]: new Set() }; + + if (!(doc.fieldname in this.doctype_field_map[doc.doctype])) + this.doctype_field_map[doc.doctype][doc.fieldname] = new Set(); }); this.data.forEach((row) => { doctypes.forEach((doc) => { - this.doctype_field_map[doc.doctype].names.add(row[doc.fieldname]); + row[doc.fieldname] && + this.doctype_field_map[doc.doctype][doc.fieldname].add(row[doc.fieldname]); }); }); diff --git a/frappe/public/js/frappe/views/reports/report_view.js b/frappe/public/js/frappe/views/reports/report_view.js index 9fb875aed5..48971ec918 100644 --- a/frappe/public/js/frappe/views/reports/report_view.js +++ b/frappe/public/js/frappe/views/reports/report_view.js @@ -702,7 +702,7 @@ frappe.views.ReportView = class ReportView extends frappe.views.ListView { } is_editable(df, data) { - return ( + if ( df && frappe.model.can_write(this.doctype) && // not a submitted doc or field is allowed to edit after submit @@ -713,12 +713,16 @@ frappe.views.ReportView = class ReportView extends frappe.views.ListView { !df.is_virtual && !df.hidden && // not a standard field i.e., owner, modified_by, etc. - frappe.model.is_non_std_field(df.fieldname) && + frappe.model.is_non_std_field(df.fieldname) + ) { // don't check read_only_depends_on if there's child table fields - !this.meta.fields.some((df) => df.fieldtype === "Table") && - df.read_only_depends_on && - !this.evaluate_read_only_depends_on(df.read_only_depends_on, data) - ); + return ( + this.meta.fields.some((df) => df.fieldtype === "Table") || + (df.read_only_depends_on && + !this.evaluate_read_only_depends_on(df.read_only_depends_on, data)) + ); + } + return false; } get_data(values) { diff --git a/frappe/public/scss/common/grid.scss b/frappe/public/scss/common/grid.scss index d2c3a49438..27e232a017 100644 --- a/frappe/public/scss/common/grid.scss +++ b/frappe/public/scss/common/grid.scss @@ -367,11 +367,13 @@ height: 40px; } } - .awesomplete > ul { - position: fixed !important; - left: auto !important; - top: auto; - width: 250px !important; + .grid-row:not(.grid-row-open) { + .awesomplete > ul { + position: fixed !important; + left: auto !important; + top: auto; + width: 250px !important; + } } .grid-static-col { background-color: var(--fg-color); 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..b970f8068a 100644 --- a/frappe/tests/test_document.py +++ b/frappe/tests/test_document.py @@ -542,7 +542,7 @@ class TestDocumentWebView(IntegrationTestCase): self.assertEqual(self.get(url).status, "200 OK") with self.change_settings("System Settings", {"allow_older_web_view_links": False}): - self.assertEqual(self.get(url).status, "401 UNAUTHORIZED") + self.assertEqual(self.get(url).status, "403 FORBIDDEN") # with valid key url = f"/ToDo/{todo.name}?key={document_key}" @@ -550,7 +550,7 @@ class TestDocumentWebView(IntegrationTestCase): # with invalid key invalid_key_url = f"/ToDo/{todo.name}?key=INVALID_KEY" - self.assertEqual(self.get(invalid_key_url).status, "401 UNAUTHORIZED") + self.assertEqual(self.get(invalid_key_url).status, "403 FORBIDDEN") # expire the key document_key_doc = frappe.get_doc("Document Share Key", {"key": document_key}) @@ -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/background_jobs.py b/frappe/utils/background_jobs.py index 86b71c91a7..a9d65e9f39 100644 --- a/frappe/utils/background_jobs.py +++ b/frappe/utils/background_jobs.py @@ -1,5 +1,6 @@ -import gc import os +import random +import signal import socket import time from collections import defaultdict @@ -14,10 +15,12 @@ import redis import setproctitle from redis.exceptions import BusyLoadingError, ConnectionError from rq import Callback, Queue, Worker +from rq.defaults import DEFAULT_WORKER_TTL from rq.exceptions import NoSuchJobError from rq.job import Job, JobStatus from rq.logutils import setup_loghandlers -from rq.worker import DequeueStrategy +from rq.timeouts import JobTimeoutException +from rq.worker import DequeueStrategy, StopRequested, WorkerStatus from rq.worker_pool import WorkerPool from tenacity import retry, retry_if_exception_type, stop_after_attempt, wait_fixed @@ -26,6 +29,7 @@ import frappe.monitor from frappe import _ from frappe.utils import CallbackManager, cint, get_bench_id from frappe.utils.commands import log +from frappe.utils.data import sbool from frappe.utils.redis_queue import RedisQueue # TTL to keep RQ job logs in redis for. @@ -33,6 +37,9 @@ RQ_JOB_FAILURE_TTL = 7 * 24 * 60 * 60 # 7 days instead of 1 year (default) RQ_FAILED_JOBS_LIMIT = 1000 # Only keep these many recent failed jobs around RQ_RESULTS_TTL = 10 * 60 +RQ_MAX_JOBS = 5000 # Restart NOFORK workers after every N number of jobs +RQ_MAX_JOBS_JITTER = 50 # Random difference in max jobs to avoid restarting at same time + _redis_queue_conn = None @@ -211,7 +218,7 @@ def execute_job(site, method, event, job_name, kwargs, user=None, is_async=True, retval = None if is_async: - frappe.init(site) + frappe.init(site, force=True) frappe.connect() if os.environ.get("CI"): frappe.flags.in_test = True @@ -278,7 +285,7 @@ def execute_job(site, method, event, job_name, kwargs, user=None, is_async=True, finally: if not hasattr(frappe.local, "site"): - frappe.init(site) + frappe.init(site, force=True) frappe.connect() for after_job_task in frappe.get_hooks("after_job"): frappe.call(after_job_task, method=method_name, kwargs=kwargs, result=retval) @@ -368,6 +375,38 @@ class FrappeWorker(Worker): self.pubsub_thread = self.pubsub.run_in_thread(sleep_time=2, daemon=True) +class FrappeWorkerNoFork(FrappeWorker): + def __init__(self, *args, **kwargs): + super().__init__(*args, **kwargs) + self.push_exc_handler(self.no_fork_exception_handler) + + def work(self, *args, **kwargs): + kwargs["max_jobs"] = RQ_MAX_JOBS + random.randint(0, RQ_MAX_JOBS_JITTER) + return super().work(*args, **kwargs) + + def execute_job(self, job: "Job", queue: "Queue"): + """Execute job in same thread/process, do not fork()""" + self.prepare_execution(job) + self.perform_job(job, queue) + self.set_state(WorkerStatus.IDLE) + + def no_fork_exception_handler(self, job, exc_type, exc_value, traceback): + if isinstance(exc_value, JobTimeoutException): + # This is done to avoid polluting global state from partial executions. + # More such cases MIGHT surface and this is where they should be handled. + raise StopRequested + + def get_heartbeat_ttl(self, job: "Job") -> int: + if job.timeout == -1: + return DEFAULT_WORKER_TTL + else: + return int(job.timeout or DEFAULT_WORKER_TTL) + 60 + + def kill_horse(self, sig=signal.SIGKILL): + # Horse = self when we are not forking + os.kill(os.getpid(), sig) + + def start_worker_pool( queue: str | None = None, num_workers: int = 1, @@ -406,11 +445,15 @@ def start_worker_pool( if quiet: logging_level = "WARNING" + # TODO: Make this true by default eventually. It's limited to RQ WorkerPool + no_fork = sbool(os.environ.get("FRAPPE_BACKGROUND_WORKERS_NOFORK", False)) + + worker_klass = FrappeWorkerNoFork if no_fork else FrappeWorker pool = WorkerPool( queues=queues, connection=redis_connection, num_workers=num_workers, - worker_class=FrappeWorker, # Auto starts scheduler with workerpool + worker_class=worker_klass, ) pool.start(logging_level=logging_level, burst=burst) diff --git a/frappe/utils/response.py b/frappe/utils/response.py index a9fb61e3ef..6849f3bbb3 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 or frappe._dev_server) + ) + + 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/utils/scheduler.py b/frappe/utils/scheduler.py index be3fec64fd..d48506a305 100644 --- a/frappe/utils/scheduler.py +++ b/frappe/utils/scheduler.py @@ -47,7 +47,7 @@ def start_scheduler() -> NoReturn: tick = get_scheduler_tick() set_niceness() - lock_path = os.path.abspath(os.path.join(get_bench_path(), "config", "scheduler_process")) + lock_path = _get_scheduler_lock_file() try: lock = FileLock(lock_path) @@ -62,6 +62,25 @@ def start_scheduler() -> NoReturn: enqueue_events_for_all_sites() +def _get_scheduler_lock_file() -> True: + return os.path.abspath(os.path.join(get_bench_path(), "config", "scheduler_process")) + + +def is_schduler_process_running() -> bool: + """Checks if any other process is holding the lock. + + Note: FLOCK is held by process until it exits, this function just checks if process is + running or not. We can't determine if process is stuck somehwere. + """ + try: + lock = FileLock(_get_scheduler_lock_file()) + lock.acquire(blocking=False) + lock.release() + return False + except Timeout: + return True + + def sleep_duration(tick): if tick != DEFAULT_SCHEDULER_TICK: # Assuming user knows what they want. diff --git a/frappe/website/doctype/web_form/web_form.py b/frappe/website/doctype/web_form/web_form.py index 1678d24350..6ed0d97ba6 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 @@ -157,6 +158,8 @@ def get_context(context): # check permissions if frappe.form_dict.name: + assert isinstance(frappe.form_dict.name, str | int) + if frappe.session.user == "Guest": frappe.throw( _("You need to be logged in to access this {0}.").format(self.doc_type), @@ -164,9 +167,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 +615,7 @@ def accept(web_form, data): @frappe.whitelist() -def delete(web_form_name, docname): +def delete(web_form_name: str, docname: str | int): web_form = frappe.get_doc("Web Form", web_form_name) owner = frappe.db.get_value(web_form.doc_type, docname, "owner") @@ -621,7 +626,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 | int]): 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..a7d3a12334 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") return { - "error": frappe.get_traceback().replace("<", "<").replace(">", ">") if allow_traceback else "" + "error": frappe.get_traceback().replace("<", "<").replace(">", ">") + if is_traceback_allowed() + else "" } diff --git a/frappe/www/printview.py b/frappe/www/printview.py index 62515e482a..484f2b9205 100644 --- a/frappe/www/printview.py +++ b/frappe/www/printview.py @@ -84,11 +84,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 +130,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 +145,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 +385,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 +407,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: