From e2f1ecb780b3eb179e468194a47044a45cbb6ce4 Mon Sep 17 00:00:00 2001 From: venkat102 Date: Tue, 11 Feb 2025 12:13:43 +0530 Subject: [PATCH 01/26] fix: add custom column based on link column --- .../js/frappe/views/reports/query_report.js | 40 ++++++++++--------- 1 file changed, 21 insertions(+), 19 deletions(-) diff --git a/frappe/public/js/frappe/views/reports/query_report.js b/frappe/public/js/frappe/views/reports/query_report.js index 37c328b835..4dae1c7271 100644 --- a/frappe/public/js/frappe/views/reports/query_report.js +++ b/frappe/public/js/frappe/views/reports/query_report.js @@ -1702,11 +1702,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) @@ -1747,6 +1752,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 @@ -1774,17 +1781,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 ); @@ -1866,13 +1870,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; @@ -1881,9 +1879,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]]; } }); @@ -1927,14 +1925,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]); }); }); From bd345d7872206b1b0ff88ec88e566c8d74f6ae94 Mon Sep 17 00:00:00 2001 From: sokumon Date: Tue, 11 Feb 2025 14:56:34 +0530 Subject: [PATCH 02/26] fix: replace espresso icon --- .../public/js/frappe/form/templates/timeline_message_box.html | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) 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) { %} - - + + {% } %} From 701e744211021caf9b85327d8c6b76644cf18739 Mon Sep 17 00:00:00 2001 From: Ejaaz Khan <67804911+iamejaaz@users.noreply.github.com> Date: Thu, 13 Feb 2025 23:32:55 +0530 Subject: [PATCH 03/26] Revert "fix(_get_communications): clean email content before returning" --- frappe/desk/form/load.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/frappe/desk/form/load.py b/frappe/desk/form/load.py index 79b35bac4a..ba8fc2ac24 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.utils.data import cstr -from frappe.utils.html_utils import clean_email_html if typing.TYPE_CHECKING: from frappe.model.document import Document @@ -263,7 +262,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", From ef27287c258d0869fe2f52121ad136d1ba2ec5f6 Mon Sep 17 00:00:00 2001 From: Ejaaz Khan Date: Fri, 14 Feb 2025 11:15:00 +0530 Subject: [PATCH 04/26] fix: link field option disapper --- frappe/public/scss/common/grid.scss | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) 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); From b5ee3b29cf2ca97673b88c6dd43e60ee73165ca4 Mon Sep 17 00:00:00 2001 From: Akhil Narang Date: Fri, 14 Feb 2025 16:25:48 +0530 Subject: [PATCH 05/26] fix: set `report_ref_doctype` for custom reports Signed-off-by: Akhil Narang --- frappe/desk/desktop.py | 1 + 1 file changed, 1 insertion(+) 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 ] From 051e0a6d4eccb1d10a5359b205b4022390b2f3dd Mon Sep 17 00:00:00 2001 From: Akhil Narang Date: Fri, 14 Feb 2025 16:30:59 +0530 Subject: [PATCH 06/26] Revert "fix: Improper routing for Report links when added to cards in workspaces" --- frappe/public/js/frappe/utils/utils.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) 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; } From 5fe0742ab9561fe8156be5c05d1b9b6afc0ddd56 Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Mon, 17 Feb 2025 09:57:53 +0530 Subject: [PATCH 07/26] fix: Avoid recency filters on dashboard connection filters (#31280) --- frappe/public/js/frappe/list/list_view.js | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) 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) { From d0c3a8ee5675d8dc323192ac3772ecbfaf345012 Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Mon, 17 Feb 2025 10:34:07 +0530 Subject: [PATCH 08/26] fix: check scheduler process status in health report (#31284) Currently it's just checking if the scheduler is enabled or not. This PR also adds a check to see if the process is running or not. --- .../system_health_report.py | 5 +++-- frappe/utils/scheduler.py | 21 ++++++++++++++++++- 2 files changed, 23 insertions(+), 3 deletions(-) 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/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. From af39cc43adf5f3793f4715af6abfc54b01d1c16e Mon Sep 17 00:00:00 2001 From: venkat102 Date: Mon, 17 Feb 2025 12:29:14 +0530 Subject: [PATCH 09/26] fix: update doctype value --- cypress/integration/query_report.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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() From 3b4e1bbb48606efa1458b606cdc8877c4f41005e Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Mon, 17 Feb 2025 09:57:12 +0530 Subject: [PATCH 10/26] perf: Support running RQ worker without forking Frappe does everything required to run jobs without forking. - We cleanup locals - Global caches are invalidated appropriately - We destroy connections and open a new one for job/request So we don't have any safety benefit much from forking for every job. There can still be some code out in wild that relies on this behaviour though! Pros: - Almost zero-overhead background jobs, just like web requests. Cons: - Timeout need to be implemented separately now - Unknown side effects! - Some features like "stop job" will need separate implementations. --- frappe/utils/background_jobs.py | 24 ++++++++++++++++++++++-- 1 file changed, 22 insertions(+), 2 deletions(-) diff --git a/frappe/utils/background_jobs.py b/frappe/utils/background_jobs.py index 86b71c91a7..d1fb1428da 100644 --- a/frappe/utils/background_jobs.py +++ b/frappe/utils/background_jobs.py @@ -14,10 +14,11 @@ 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.worker import DequeueStrategy, WorkerStatus from rq.worker_pool import WorkerPool from tenacity import retry, retry_if_exception_type, stop_after_attempt, wait_fixed @@ -332,7 +333,8 @@ def start_worker( class FrappeWorker(Worker): - def work(self, *args, **kwargs): + def work(self, *args, disable_forking=True, **kwargs): + self.disable_forking = disable_forking self.start_frappe_scheduler() kwargs["with_scheduler"] = False # Always disable RQ scheduler return super().work(*args, **kwargs) @@ -367,6 +369,24 @@ class FrappeWorker(Worker): self.pubsub.subscribe(**{self.pubsub_channel_name: self.handle_payload}) self.pubsub_thread = self.pubsub.run_in_thread(sleep_time=2, daemon=True) + def execute_job(self, job: "Job", queue: "Queue"): + """Execute job in same thread/process, do not fork()""" + if not self.disable_forking: + return super().execute_job(job, queue) + + self.prepare_execution(job) + self.perform_job(job, queue) + self.set_state(WorkerStatus.IDLE) + + def get_heartbeat_ttl(self, job: "Job") -> int: + if not self.disable_forking: + return super().get_heartbeat_ttl(job) + + if job.timeout == -1: + return DEFAULT_WORKER_TTL + else: + return int(job.timeout or DEFAULT_WORKER_TTL) + 60 + def start_worker_pool( queue: str | None = None, From 24c2dfdf8853bb3bd6f7894b253c0c35d1de5951 Mon Sep 17 00:00:00 2001 From: Akhil Narang Date: Mon, 17 Feb 2025 17:44:02 +0530 Subject: [PATCH 11/26] fix(report_view): don't disallow editing just if a table exists If a table exists, go with the previous result. Else check and parse read_only conditions Signed-off-by: Akhil Narang --- .../js/frappe/views/reports/report_view.js | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) 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) { From 0cebc61de1b1e6805706fd0d30f97c4604aef00a Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Mon, 17 Feb 2025 17:57:21 +0530 Subject: [PATCH 12/26] fix: Exit worker on timeout exception - Timeout in same process can leave bad state, so start a new worker. - This isn't exactly costly, earlier we were forking all the time. - Timeouts by definition can't happen too frequently. --- frappe/utils/background_jobs.py | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/frappe/utils/background_jobs.py b/frappe/utils/background_jobs.py index d1fb1428da..c9cb306f55 100644 --- a/frappe/utils/background_jobs.py +++ b/frappe/utils/background_jobs.py @@ -1,4 +1,3 @@ -import gc import os import socket import time @@ -18,7 +17,8 @@ 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, WorkerStatus +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 @@ -337,6 +337,7 @@ class FrappeWorker(Worker): self.disable_forking = disable_forking self.start_frappe_scheduler() kwargs["with_scheduler"] = False # Always disable RQ scheduler + self.push_exc_handler(self.no_fork_exception_handler) return super().work(*args, **kwargs) def run_maintenance_tasks(self, *args, **kwargs): @@ -387,6 +388,12 @@ class FrappeWorker(Worker): else: return int(job.timeout or DEFAULT_WORKER_TTL) + 60 + def no_fork_exception_handler(self, job, exc_type, exc_value, traceback): + if self.disable_forking and 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 start_worker_pool( queue: str | None = None, From 9dd005b21e16c2b5696ee3211a236aeb0694ff45 Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Mon, 17 Feb 2025 18:19:46 +0530 Subject: [PATCH 13/26] refactor: separate worker type for nofork --- frappe/utils/background_jobs.py | 48 ++++++++++++++++++++------------- 1 file changed, 29 insertions(+), 19 deletions(-) diff --git a/frappe/utils/background_jobs.py b/frappe/utils/background_jobs.py index c9cb306f55..2758268495 100644 --- a/frappe/utils/background_jobs.py +++ b/frappe/utils/background_jobs.py @@ -27,6 +27,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. @@ -333,11 +334,9 @@ def start_worker( class FrappeWorker(Worker): - def work(self, *args, disable_forking=True, **kwargs): - self.disable_forking = disable_forking + def work(self, *args, **kwargs): self.start_frappe_scheduler() kwargs["with_scheduler"] = False # Always disable RQ scheduler - self.push_exc_handler(self.no_fork_exception_handler) return super().work(*args, **kwargs) def run_maintenance_tasks(self, *args, **kwargs): @@ -370,29 +369,33 @@ class FrappeWorker(Worker): self.pubsub.subscribe(**{self.pubsub_channel_name: self.handle_payload}) self.pubsub_thread = self.pubsub.run_in_thread(sleep_time=2, daemon=True) - def execute_job(self, job: "Job", queue: "Queue"): - """Execute job in same thread/process, do not fork()""" - if not self.disable_forking: - return super().execute_job(job, queue) - self.prepare_execution(job) - self.perform_job(job, queue) - self.set_state(WorkerStatus.IDLE) +class FrappeWorkerNoFork(FrappeWorker): + def kill_horse(self, sig): + # TODO: need to handle separately + pass + + def __init__(self, *args, **kwargs): + super().__init__(*args, **kwargs) + self.push_exc_handler(self.no_fork_exception_handler) + + 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 not self.disable_forking: - return super().get_heartbeat_ttl(job) - if job.timeout == -1: return DEFAULT_WORKER_TTL else: return int(job.timeout or DEFAULT_WORKER_TTL) + 60 - def no_fork_exception_handler(self, job, exc_type, exc_value, traceback): - if self.disable_forking and 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 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 start_worker_pool( @@ -433,11 +436,18 @@ def start_worker_pool( if quiet: logging_level = "WARNING" + worker_klass = ( + FrappeWorker + # TODO: Make this true by default eventually. It's limited to RQ WorkerPool + if sbool(os.environ.get("FRAPPE_BACKGROUND_WORKERS_NOFORK", False)) + else FrappeWorkerNoFork + ) + 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) From 644bc5530e55acfd55b334a94a6f882b8c3f909f Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Mon, 17 Feb 2025 18:40:11 +0530 Subject: [PATCH 14/26] fix: Support stop-job, kill-horse etc --- frappe/utils/background_jobs.py | 23 ++++++++++++----------- 1 file changed, 12 insertions(+), 11 deletions(-) diff --git a/frappe/utils/background_jobs.py b/frappe/utils/background_jobs.py index 2758268495..b4a6d5a374 100644 --- a/frappe/utils/background_jobs.py +++ b/frappe/utils/background_jobs.py @@ -1,4 +1,5 @@ import os +import signal import socket import time from collections import defaultdict @@ -371,14 +372,16 @@ class FrappeWorker(Worker): class FrappeWorkerNoFork(FrappeWorker): - def kill_horse(self, sig): - # TODO: need to handle separately - pass - def __init__(self, *args, **kwargs): super().__init__(*args, **kwargs) self.push_exc_handler(self.no_fork_exception_handler) + 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. @@ -391,11 +394,9 @@ class FrappeWorkerNoFork(FrappeWorker): else: return int(job.timeout or DEFAULT_WORKER_TTL) + 60 - 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 kill_horse(self, sig=signal.SIGKILL): + # Horse = self when we are not forking + os.kill(os.getpid(), sig) def start_worker_pool( @@ -437,10 +438,10 @@ def start_worker_pool( logging_level = "WARNING" worker_klass = ( - FrappeWorker + FrappeWorkerNoFork # TODO: Make this true by default eventually. It's limited to RQ WorkerPool if sbool(os.environ.get("FRAPPE_BACKGROUND_WORKERS_NOFORK", False)) - else FrappeWorkerNoFork + else FrappeWorker ) pool = WorkerPool( From de7e6d0ba5d0b38a97d63c3766e8923516cbaeb0 Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Mon, 17 Feb 2025 18:47:59 +0530 Subject: [PATCH 15/26] fix: force init background jobs --- frappe/utils/background_jobs.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/frappe/utils/background_jobs.py b/frappe/utils/background_jobs.py index b4a6d5a374..2306bc9280 100644 --- a/frappe/utils/background_jobs.py +++ b/frappe/utils/background_jobs.py @@ -214,7 +214,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 @@ -281,7 +281,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) From e2bd3abd625d30ea9fd26504939ccb91263e90b5 Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Mon, 17 Feb 2025 18:54:59 +0530 Subject: [PATCH 16/26] fix: make nofork workers exit periodically --- frappe/utils/background_jobs.py | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/frappe/utils/background_jobs.py b/frappe/utils/background_jobs.py index 2306bc9280..a9d65e9f39 100644 --- a/frappe/utils/background_jobs.py +++ b/frappe/utils/background_jobs.py @@ -1,4 +1,5 @@ import os +import random import signal import socket import time @@ -36,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 @@ -376,6 +380,10 @@ class FrappeWorkerNoFork(FrappeWorker): 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) @@ -437,13 +445,10 @@ def start_worker_pool( if quiet: logging_level = "WARNING" - worker_klass = ( - FrappeWorkerNoFork - # TODO: Make this true by default eventually. It's limited to RQ WorkerPool - if sbool(os.environ.get("FRAPPE_BACKGROUND_WORKERS_NOFORK", False)) - else FrappeWorker - ) + # 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, From aed69f92f75a263b555279aab4d5cf4a8ee652a7 Mon Sep 17 00:00:00 2001 From: "Nihantra C. Patel" <141945075+Nihantra-Patel@users.noreply.github.com> Date: Tue, 18 Feb 2025 14:55:39 +0530 Subject: [PATCH 17/26] fix: hide create button for cancelled doc in dashboard links (#31295) --- frappe/public/js/frappe/form/form.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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; From 85f7fb7922e4599d13256127f9d9eb3e88277397 Mon Sep 17 00:00:00 2001 From: s-aga-r Date: Tue, 18 Feb 2025 16:29:02 +0530 Subject: [PATCH 18/26] refactor: Frappe Mail API (#29200) --- .../doctype/email_account/email_account.py | 2 +- frappe/email/frappemail.py | 30 +++++++++---------- 2 files changed, 15 insertions(+), 17 deletions(-) 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"])) From e16f3b1c8413e1850c5fa66098911a53bd5c43a5 Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Wed, 19 Feb 2025 11:28:34 +0530 Subject: [PATCH 19/26] fix: set default, don't override cache headers (#31306) Oof. --- frappe/app.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frappe/app.py b/frappe/app.py index 6ed100792d..5fc5a5d956 100644 --- a/frappe/app.py +++ b/frappe/app.py @@ -242,7 +242,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"): From f4062b4d7ad2e4a56ed165b7c2ef09fb9a0ba52e Mon Sep 17 00:00:00 2001 From: Sagar Vora Date: Wed, 19 Feb 2025 12:10:59 +0530 Subject: [PATCH 20/26] 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: From eded5eac240091fbc9c01ea1e66a66422e2a0634 Mon Sep 17 00:00:00 2001 From: Sagar Vora Date: Wed, 19 Feb 2025 12:17:31 +0530 Subject: [PATCH 21/26] fix: explicitly check type of form name --- frappe/website/doctype/web_form/web_form.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/frappe/website/doctype/web_form/web_form.py b/frappe/website/doctype/web_form/web_form.py index 6d13dafe76..f02688044d 100644 --- a/frappe/website/doctype/web_form/web_form.py +++ b/frappe/website/doctype/web_form/web_form.py @@ -158,6 +158,8 @@ def get_context(context): # check permissions if frappe.form_dict.name: + assert isinstance(frappe.form_dict.name, str) + if frappe.session.user == "Guest": frappe.throw( _("You need to be logged in to access this {0}.").format(self.doc_type), From d3bb578c2fc8f67108786ffbfc9cec974139a4b5 Mon Sep 17 00:00:00 2001 From: Sagar Vora Date: Wed, 19 Feb 2025 12:28:41 +0530 Subject: [PATCH 22/26] fix: update type hints to allow integers --- frappe/website/doctype/web_form/web_form.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/frappe/website/doctype/web_form/web_form.py b/frappe/website/doctype/web_form/web_form.py index f02688044d..6ed0d97ba6 100644 --- a/frappe/website/doctype/web_form/web_form.py +++ b/frappe/website/doctype/web_form/web_form.py @@ -158,7 +158,7 @@ def get_context(context): # check permissions if frappe.form_dict.name: - assert isinstance(frappe.form_dict.name, str) + assert isinstance(frappe.form_dict.name, str | int) if frappe.session.user == "Guest": frappe.throw( @@ -615,7 +615,7 @@ def accept(web_form, data): @frappe.whitelist() -def delete(web_form_name: str, docname: str): +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") @@ -626,7 +626,7 @@ def delete(web_form_name: str, docname: str): @frappe.whitelist() -def delete_multiple(web_form_name: str, docnames: list[str]): +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) From 09459d1d275a3d3651cf940be4f6ee2aa626ac41 Mon Sep 17 00:00:00 2001 From: Sagar Vora Date: Wed, 19 Feb 2025 12:38:43 +0530 Subject: [PATCH 23/26] fix: ensure exception is always returned --- frappe/permissions.py | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/frappe/permissions.py b/frappe/permissions.py index 3583589b3a..b502470965 100644 --- a/frappe/permissions.py +++ b/frappe/permissions.py @@ -853,11 +853,12 @@ def check_doctype_permission(doctype: str, ptype: str = "read") -> None: 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 +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) - try: - check_doctype_permission(doctype) - except frappe.PermissionError as _e: - return _e + except frappe.PermissionError as _e: + return _e + + return e From fa252691b6dc7f0e99258236d0e5c4e3da91eca1 Mon Sep 17 00:00:00 2001 From: Sagar Vora Date: Wed, 19 Feb 2025 12:41:17 +0530 Subject: [PATCH 24/26] test: update status codes --- frappe/tests/test_document.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/frappe/tests/test_document.py b/frappe/tests/test_document.py index e6fac5ef68..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}) From 1693991702a0777a559e8915b3187724368d37b7 Mon Sep 17 00:00:00 2001 From: Sagar Vora Date: Wed, 19 Feb 2025 12:54:21 +0530 Subject: [PATCH 25/26] fix: remove dev server condition --- frappe/www/error.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/frappe/www/error.py b/frappe/www/error.py index 0de15fc6ab..a7d3a12334 100644 --- a/frappe/www/error.py +++ b/frappe/www/error.py @@ -16,8 +16,8 @@ def get_context(context): 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 "" + "error": frappe.get_traceback().replace("<", "<").replace(">", ">") + if is_traceback_allowed() + else "" } From 4f374ee1c94c4fa4f3b82ebf4dd2bd25061df172 Mon Sep 17 00:00:00 2001 From: Sagar Vora Date: Wed, 19 Feb 2025 12:59:14 +0530 Subject: [PATCH 26/26] fix: ignore `disable_traceback` if `_dev_server` is True --- frappe/utils/response.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frappe/utils/response.py b/frappe/utils/response.py index 4fad55e3e1..6849f3bbb3 100644 --- a/frappe/utils/response.py +++ b/frappe/utils/response.py @@ -62,7 +62,7 @@ def is_traceback_allowed(): return ( frappe.db and frappe.get_system_settings("allow_error_traceback") - and not frappe.local.flags.disable_traceback + and (not frappe.local.flags.disable_traceback or frappe._dev_server) )