From 3c1db7881a3839b8752283a4c47a4f1a374f7aa2 Mon Sep 17 00:00:00 2001 From: Suraj Shetty Date: Tue, 21 Dec 2021 15:43:10 +0530 Subject: [PATCH 001/340] fix: Maintain document signature in Document Key table --- frappe/core/doctype/communication/mixins.py | 2 +- frappe/core/doctype/document_key/__init__.py | 0 .../core/doctype/document_key/document_key.js | 8 +++ .../doctype/document_key/document_key.json | 65 +++++++++++++++++++ .../core/doctype/document_key/document_key.py | 8 +++ .../doctype/document_key/test_document_key.py | 8 +++ frappe/model/document.py | 15 +++++ frappe/www/printview.py | 2 +- 8 files changed, 106 insertions(+), 2 deletions(-) create mode 100644 frappe/core/doctype/document_key/__init__.py create mode 100644 frappe/core/doctype/document_key/document_key.js create mode 100644 frappe/core/doctype/document_key/document_key.json create mode 100644 frappe/core/doctype/document_key/document_key.py create mode 100644 frappe/core/doctype/document_key/test_document_key.py diff --git a/frappe/core/doctype/communication/mixins.py b/frappe/core/doctype/communication/mixins.py index b6d8070d00..1d1d162728 100644 --- a/frappe/core/doctype/communication/mixins.py +++ b/frappe/core/doctype/communication/mixins.py @@ -149,7 +149,7 @@ class CommunicationEmailMixin: "doctype": self.reference_doctype, "name": self.reference_name, "print_format": print_format, - "key": get_parent_doc(self).get_signature() + "key": get_parent_doc(self).get_new_document_key() }) def get_outgoing_email_account(self): diff --git a/frappe/core/doctype/document_key/__init__.py b/frappe/core/doctype/document_key/__init__.py new file mode 100644 index 0000000000..e69de29bb2 diff --git a/frappe/core/doctype/document_key/document_key.js b/frappe/core/doctype/document_key/document_key.js new file mode 100644 index 0000000000..e8bda74ad2 --- /dev/null +++ b/frappe/core/doctype/document_key/document_key.js @@ -0,0 +1,8 @@ +// Copyright (c) 2021, Frappe Technologies and contributors +// For license information, please see license.txt + +frappe.ui.form.on('Document Key', { + // refresh: function(frm) { + + // } +}); diff --git a/frappe/core/doctype/document_key/document_key.json b/frappe/core/doctype/document_key/document_key.json new file mode 100644 index 0000000000..780432a730 --- /dev/null +++ b/frappe/core/doctype/document_key/document_key.json @@ -0,0 +1,65 @@ +{ + "actions": [], + "allow_rename": 1, + "autoname": "format:DOCUMENT-KEY-{########}", + "creation": "2021-12-20 03:34:58.744797", + "doctype": "DocType", + "editable_grid": 1, + "engine": "InnoDB", + "field_order": [ + "reference_doctype", + "reference_docname", + "key" + ], + "fields": [ + { + "fieldname": "reference_doctype", + "fieldtype": "Link", + "label": "Reference Document Type", + "options": "DocType", + "read_only": 1, + "search_index": 1 + }, + { + "fieldname": "reference_docname", + "fieldtype": "Dynamic Link", + "label": "Reference Document Name", + "options": "reference_doctype", + "read_only": 1, + "search_index": 1 + }, + { + "fieldname": "key", + "fieldtype": "Data", + "label": "Key", + "read_only": 1 + } + ], + "in_create": 1, + "index_web_pages_for_search": 1, + "links": [], + "modified": "2021-12-20 04:24:19.320505", + "modified_by": "Administrator", + "module": "Core", + "name": "Document Key", + "naming_rule": "Expression", + "owner": "Administrator", + "permissions": [ + { + "create": 1, + "delete": 1, + "email": 1, + "export": 1, + "print": 1, + "read": 1, + "report": 1, + "role": "System Manager", + "share": 1, + "write": 1 + } + ], + "read_only": 1, + "sort_field": "modified", + "sort_order": "DESC", + "states": [] +} \ No newline at end of file diff --git a/frappe/core/doctype/document_key/document_key.py b/frappe/core/doctype/document_key/document_key.py new file mode 100644 index 0000000000..3440545db6 --- /dev/null +++ b/frappe/core/doctype/document_key/document_key.py @@ -0,0 +1,8 @@ +# Copyright (c) 2021, Frappe Technologies and contributors +# For license information, please see license.txt + +# import frappe +from frappe.model.document import Document + +class DocumentKey(Document): + pass diff --git a/frappe/core/doctype/document_key/test_document_key.py b/frappe/core/doctype/document_key/test_document_key.py new file mode 100644 index 0000000000..5930ce72b4 --- /dev/null +++ b/frappe/core/doctype/document_key/test_document_key.py @@ -0,0 +1,8 @@ +# Copyright (c) 2021, Frappe Technologies and Contributors +# See license.txt + +# import frappe +import unittest + +class TestDocumentKey(unittest.TestCase): + pass diff --git a/frappe/model/document.py b/frappe/model/document.py index bbba9b1492..8cbb3d5975 100644 --- a/frappe/model/document.py +++ b/frappe/model/document.py @@ -1271,6 +1271,21 @@ class Document(BaseDocument): """Returns signature (hash) for private URL.""" return hashlib.sha224(get_datetime_str(self.creation).encode()).hexdigest() + def get_new_document_key(self): + doc = frappe.new_doc("Document Key") + doc.reference_doctype = self.doctype + doc.reference_docname = self.name + doc.key = frappe.generate_hash(length=32) + doc.insert(ignore_permissions=True) + return doc.key + + def is_document_key_valid(self, key): + return frappe.db.exists("Document Key", { + "reference_doctype": self.doctype, + "reference_docname": self.name, + "key": key + }) + def get_liked_by(self): liked_by = getattr(self, "_liked_by", None) if liked_by: diff --git a/frappe/www/printview.py b/frappe/www/printview.py index 569ebe27d6..549e69ff3b 100644 --- a/frappe/www/printview.py +++ b/frappe/www/printview.py @@ -227,7 +227,7 @@ def get_rendered_raw_commands(doc, name=None, print_format=None, meta=None, lang def validate_print_permission(doc): if frappe.form_dict.get("key"): - if frappe.form_dict.key == doc.get_signature(): + if doc.is_document_key_valid(frappe.form_dict.key): return for ptype in ("read", "print"): From 499625a85f7165f648a3e93c7e36d8015f43a027 Mon Sep 17 00:00:00 2001 From: Suraj Shetty Date: Mon, 27 Dec 2021 23:45:21 +0530 Subject: [PATCH 002/340] fix: Allow old signature as well --- frappe/www/printview.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/frappe/www/printview.py b/frappe/www/printview.py index 549e69ff3b..4251a448bc 100644 --- a/frappe/www/printview.py +++ b/frappe/www/printview.py @@ -230,6 +230,9 @@ def validate_print_permission(doc): if doc.is_document_key_valid(frappe.form_dict.key): return + if frappe.form_dict.key == doc.get_signature(): + return + for ptype in ("read", "print"): if (not frappe.has_permission(doc.doctype, ptype, doc) and not frappe.has_website_permission(doc)): From 64985a5d1a21911c93ea4d82798cd6481199fd59 Mon Sep 17 00:00:00 2001 From: Suraj Shetty Date: Tue, 28 Dec 2021 15:19:01 +0530 Subject: [PATCH 003/340] feat: Document key expiry --- frappe/core/doctype/doctype/doctype.json | 13 +++++++++- .../doctype/document_key/document_key.json | 25 +++++++++++++++++-- .../core/doctype/document_key/document_key.py | 16 ++++++++++-- .../customize_form/customize_form.json | 11 +++++++- frappe/exceptions.py | 1 + frappe/hooks.py | 1 + frappe/model/document.py | 6 +++-- 7 files changed, 65 insertions(+), 8 deletions(-) diff --git a/frappe/core/doctype/doctype/doctype.json b/frappe/core/doctype/doctype/doctype.json index 03e3b65ea1..9a777c740a 100644 --- a/frappe/core/doctype/doctype/doctype.json +++ b/frappe/core/doctype/doctype/doctype.json @@ -5,6 +5,7 @@ "creation": "2013-02-18 13:36:19", "description": "DocType is a Table / Form in the application.", "doctype": "DocType", + "document_key_expiry": 90, "document_type": "Document", "engine": "InnoDB", "field_order": [ @@ -56,6 +57,7 @@ "color", "show_preview_popup", "show_name_in_global_search", + "document_key_expiry", "email_settings_sb", "default_email_template", "column_break_51", @@ -582,6 +584,13 @@ "fieldname": "document_states_section", "fieldtype": "Section Break", "label": "Document States" + }, + { + "default": "90", + "description": "Set number of days after which a document key will be expired", + "fieldname": "document_key_expiry", + "fieldtype": "Int", + "label": "Document Key Expiry (in Days)" } ], "icon": "fa fa-bolt", @@ -663,10 +672,11 @@ "link_fieldname": "reference_doctype" } ], - "modified": "2021-12-09 14:53:10.717788", + "modified": "2021-12-28 03:19:19.033347", "modified_by": "Administrator", "module": "Core", "name": "DocType", + "naming_rule": "Set by user", "owner": "Administrator", "permissions": [ { @@ -696,5 +706,6 @@ "show_name_in_global_search": 1, "sort_field": "modified", "sort_order": "DESC", + "states": [], "track_changes": 1 } \ No newline at end of file diff --git a/frappe/core/doctype/document_key/document_key.json b/frappe/core/doctype/document_key/document_key.json index 780432a730..b9f26b2a7e 100644 --- a/frappe/core/doctype/document_key/document_key.json +++ b/frappe/core/doctype/document_key/document_key.json @@ -4,12 +4,15 @@ "autoname": "format:DOCUMENT-KEY-{########}", "creation": "2021-12-20 03:34:58.744797", "doctype": "DocType", + "document_key_expiry": 90, "editable_grid": 1, "engine": "InnoDB", "field_order": [ "reference_doctype", "reference_docname", - "key" + "key", + "expires_on", + "status" ], "fields": [ { @@ -33,12 +36,30 @@ "fieldtype": "Data", "label": "Key", "read_only": 1 + }, + { + "fieldname": "expires_on", + "fieldtype": "Date", + "in_list_view": 1, + "label": "Expires On", + "read_only": 1 + }, + { + "default": "Active", + "fieldname": "status", + "fieldtype": "Select", + "in_list_view": 1, + "in_standard_filter": 1, + "label": "Status", + "options": "Active\nExpired", + "read_only": 1, + "search_index": 1 } ], "in_create": 1, "index_web_pages_for_search": 1, "links": [], - "modified": "2021-12-20 04:24:19.320505", + "modified": "2021-12-28 00:43:57.811188", "modified_by": "Administrator", "module": "Core", "name": "Document Key", diff --git a/frappe/core/doctype/document_key/document_key.py b/frappe/core/doctype/document_key/document_key.py index 3440545db6..5b8fefcb80 100644 --- a/frappe/core/doctype/document_key/document_key.py +++ b/frappe/core/doctype/document_key/document_key.py @@ -1,8 +1,20 @@ # Copyright (c) 2021, Frappe Technologies and contributors # For license information, please see license.txt -# import frappe +import frappe from frappe.model.document import Document class DocumentKey(Document): - pass + def before_insert(self): + self.key = frappe.generate_hash(length=32) + if not self.expires_on: + meta = frappe.get_meta("DocType", self.reference_doctype) + self.expires_on = frappe.utils.add_days(None, days=meta.get("document_key_expiry") or 90) + + +def expire_document_keys(): + # called from hooks + frappe.db.set_value("Document Key", { + "expired_on": ["<", frappe.utils.nowdate()], + "status": "Active" + }, "status", "Expired") \ No newline at end of file diff --git a/frappe/custom/doctype/customize_form/customize_form.json b/frappe/custom/doctype/customize_form/customize_form.json index bdf95ad351..b1c67345d9 100644 --- a/frappe/custom/doctype/customize_form/customize_form.json +++ b/frappe/custom/doctype/customize_form/customize_form.json @@ -3,6 +3,7 @@ "autoname": "DL.####", "creation": "2013-01-29 17:55:08", "doctype": "DocType", + "document_key_expiry": 90, "document_type": "Document", "editable_grid": 1, "engine": "InnoDB", @@ -31,6 +32,7 @@ "default_print_format", "column_break_29", "show_preview_popup", + "document_key_expiry", "email_settings_section", "default_email_template", "column_break_26", @@ -296,6 +298,13 @@ "fieldtype": "Table", "label": "States", "options": "DocType State" + }, + { + "default": "90", + "description": "Set number of days after which a document key will be expired", + "fieldname": "document_key_expiry", + "fieldtype": "Int", + "label": "Document Key Expiry (in Days)" } ], "hide_toolbar": 1, @@ -304,7 +313,7 @@ "index_web_pages_for_search": 1, "issingle": 1, "links": [], - "modified": "2021-12-14 16:45:04.308690", + "modified": "2021-12-28 00:30:47.840699", "modified_by": "Administrator", "module": "Custom", "name": "Customize Form", diff --git a/frappe/exceptions.py b/frappe/exceptions.py index 8449425bc1..8993539603 100644 --- a/frappe/exceptions.py +++ b/frappe/exceptions.py @@ -82,6 +82,7 @@ class DocstatusTransitionError(ValidationError): pass class TimestampMismatchError(ValidationError): pass class EmptyTableError(ValidationError): pass class LinkExistsError(ValidationError): pass +class LinkExpiredError(ValidationError): pass class InvalidEmailAddressError(ValidationError): pass class InvalidNameError(ValidationError): pass class InvalidPhoneNumberError(ValidationError): pass diff --git a/frappe/hooks.py b/frappe/hooks.py index 4895c97200..d729e2ab55 100644 --- a/frappe/hooks.py +++ b/frappe/hooks.py @@ -241,6 +241,7 @@ scheduler_events = { "frappe.email.doctype.unhandled_email.unhandled_email.remove_old_unhandled_emails", "frappe.core.doctype.prepared_report.prepared_report.delete_expired_prepared_reports", "frappe.core.doctype.log_settings.log_settings.run_log_clean_up", + "frappe.core.doctype.document_key.document_key.expire_document_keys", "frappe.website.doctype.personal_data_deletion_request.personal_data_deletion_request.process_data_deletion_request" ], "daily_long": [ diff --git a/frappe/model/document.py b/frappe/model/document.py index 8cbb3d5975..ec83f5f7b0 100644 --- a/frappe/model/document.py +++ b/frappe/model/document.py @@ -1271,11 +1271,12 @@ class Document(BaseDocument): """Returns signature (hash) for private URL.""" return hashlib.sha224(get_datetime_str(self.creation).encode()).hexdigest() - def get_new_document_key(self): + @frappe.whitelist() + def get_new_document_key(self, expires_on=None): doc = frappe.new_doc("Document Key") doc.reference_doctype = self.doctype doc.reference_docname = self.name - doc.key = frappe.generate_hash(length=32) + doc.expires_on = expires_on doc.insert(ignore_permissions=True) return doc.key @@ -1283,6 +1284,7 @@ class Document(BaseDocument): return frappe.db.exists("Document Key", { "reference_doctype": self.doctype, "reference_docname": self.name, + "status": "Active", "key": key }) From 4ee99ce7e159018d654cf6f818e1d2c080430f3e Mon Sep 17 00:00:00 2001 From: Suraj Shetty Date: Thu, 13 Jan 2022 11:45:45 +0530 Subject: [PATCH 004/340] feat: Set expiry to document link --- frappe/model/document.py | 2 +- frappe/public/icons/timeless/symbol-defs.svg | 3 ++ frappe/public/js/frappe/form/controls/data.js | 14 ++++++- frappe/public/js/frappe/form/controls/date.js | 2 + frappe/public/js/frappe/form/form.js | 41 +++++++++++++++++++ frappe/public/js/frappe/form/toolbar.js | 7 ++++ frappe/public/scss/common/controls.scss | 26 ++++++++---- frappe/public/scss/common/css_variables.scss | 2 +- frappe/www/printview.html | 13 ++++-- frappe/www/printview.py | 11 +++-- 10 files changed, 104 insertions(+), 17 deletions(-) diff --git a/frappe/model/document.py b/frappe/model/document.py index ec83f5f7b0..633c6531b4 100644 --- a/frappe/model/document.py +++ b/frappe/model/document.py @@ -1286,7 +1286,7 @@ class Document(BaseDocument): "reference_docname": self.name, "status": "Active", "key": key - }) + }, cache=True) def get_liked_by(self): liked_by = getattr(self, "_liked_by", None) diff --git a/frappe/public/icons/timeless/symbol-defs.svg b/frappe/public/icons/timeless/symbol-defs.svg index b878f713e9..dcba913836 100644 --- a/frappe/public/icons/timeless/symbol-defs.svg +++ b/frappe/public/icons/timeless/symbol-defs.svg @@ -712,4 +712,7 @@ + + + diff --git a/frappe/public/js/frappe/form/controls/data.js b/frappe/public/js/frappe/form/controls/data.js index f4c9849528..2323ccee2f 100644 --- a/frappe/public/js/frappe/form/controls/data.js +++ b/frappe/public/js/frappe/form/controls/data.js @@ -58,7 +58,7 @@ frappe.ui.form.ControlData = class ControlData extends frappe.ui.form.ControlInp this.has_input = true; this.bind_change_event(); this.setup_autoname_check(); - + this.setup_copy_button(); if (this.df.options == 'URL') { this.setup_url_field(); } @@ -112,6 +112,18 @@ frappe.ui.form.ControlData = class ControlData extends frappe.ui.form.ControlInp }); } + setup_copy_button() { + if (this.df.with_copy_button) { + this.$wrapper.find('.control-input').append( + `` + ).find(".action-btn").click(() => { + frappe.utils.copy_to_clipboard(this.value); + }); + } + } + setup_barcode_field() { this.$wrapper.find('.control-input').append( ` diff --git a/frappe/public/js/frappe/form/controls/date.js b/frappe/public/js/frappe/form/controls/date.js index 28e7f2a478..5667fd05a4 100644 --- a/frappe/public/js/frappe/form/controls/date.js +++ b/frappe/public/js/frappe/form/controls/date.js @@ -62,6 +62,8 @@ frappe.ui.form.ControlDate = class ControlDate extends frappe.ui.form.ControlDat dateFormat: date_format, startDate: this.get_start_date(), keyboardNav: false, + minDate: this.df.min_date, + maxDate: this.df.max_date, onSelect: () => { this.$input.trigger('change'); }, diff --git a/frappe/public/js/frappe/form/form.js b/frappe/public/js/frappe/form/form.js index 8af1631b48..3ce7ff5777 100644 --- a/frappe/public/js/frappe/form/form.js +++ b/frappe/public/js/frappe/form/form.js @@ -1139,6 +1139,47 @@ frappe.ui.form.Form = class FrappeForm { }); } + share_doc_link() { + const share_document_url_dialog = new frappe.ui.Dialog({ + fields: [{ + fieldname: "visibility", + label: "Visibility", + fieldtype: "Select", + options: "Public\nPrivate", + default: "Public", + read_only_depends_on: "eval: doc.link" + }, { + fieldname: "link_expiration_date", + label: "Link Expiration Date", + fieldtype: "Date", + min_date: new Date(), + read_only_depends_on: "eval: doc.link" + }, { + fieldtype: "Button", + label: "Generate Link", + click: () => { + this.call("get_new_document_key").then(res => { + let key = res.message; + share_document_url_dialog.set_value("link", this.get_share_link(key)) + }) + } + }, { + fieldname: "link", + label: "Link", + fieldtype: "Data", + depends_on: "eval: doc.link", + with_copy_button: true + }], + size: "small", + title: __("Share {} Link", [this.doctype]), + }) + share_document_url_dialog.show() + } + + get_share_link(key) { + return `${window.origin}/${this.doctype}/${this.docname}?key=${key}`; + } + copy_doc(onload, from_amend) { this.validate_form_action("Create"); var newdoc = frappe.model.copy_doc(this.doc, from_amend); diff --git a/frappe/public/js/frappe/form/toolbar.js b/frappe/public/js/frappe/form/toolbar.js index ffcfc349ef..f5153eb67e 100644 --- a/frappe/public/js/frappe/form/toolbar.js +++ b/frappe/public/js/frappe/form/toolbar.js @@ -265,6 +265,13 @@ frappe.ui.form.Toolbar = class Toolbar { }); } + // share print view link + if (frappe.model.can_email(null, me.frm) && me.frm.doc.docstatus < 2) { + this.page.add_menu_item(__("Share Link", [me.frm.doctype]), function() { + me.frm.share_doc_link(); + }, true); + } + // go to field modal this.page.add_menu_item(__("Jump to field"), function() { me.show_jump_to_field_dialog(); diff --git a/frappe/public/scss/common/controls.scss b/frappe/public/scss/common/controls.scss index 954916c911..3cbc447d8f 100644 --- a/frappe/public/scss/common/controls.scss +++ b/frappe/public/scss/common/controls.scss @@ -288,13 +288,25 @@ textarea.form-control { position: relative; } -.link-btn { - position: absolute; - top: 4px; - right: 4px; - padding: 3px; - display: none; - z-index: 3; +.frappe-control { + .action-btn { + position: absolute; + top: 4px; + right: 4px; + padding: 3px; + z-index: 3; + } + + button.action-btn { + padding: 3px 5px; + background-color: var(--fg-color); + } + + .link-btn { + @extend .action-btn; + background-color: none; + display: none; + } } .phone-btn { diff --git a/frappe/public/scss/common/css_variables.scss b/frappe/public/scss/common/css_variables.scss index a14c19af2a..4293d6834b 100644 --- a/frappe/public/scss/common/css_variables.scss +++ b/frappe/public/scss/common/css_variables.scss @@ -219,7 +219,7 @@ --border-radius-full: 999px; --primary-color: #2490EF; - --btn-height: 28px; + --btn-height: 30px; // Checkbox --checkbox-right-margin: var(--margin-xs); diff --git a/frappe/www/printview.html b/frappe/www/printview.html index 3f8d4201fb..691c0cf3cf 100644 --- a/frappe/www/printview.html +++ b/frappe/www/printview.html @@ -11,11 +11,16 @@ -
-
- {{ body }} -
+
+ {{ _("Your key has been expired!") }}
+ {% if hideewe %} +
+
+ {{ body }} +
+
+ {% endif %} + {% endif %} {%- if comment -%} diff --git a/frappe/www/printview.py b/frappe/www/printview.py index cc31b27e93..ee990b5fca 100644 --- a/frappe/www/printview.py +++ b/frappe/www/printview.py @@ -37,24 +37,31 @@ def get_context(context): make_access_log(doctype=frappe.form_dict.doctype, document=frappe.form_dict.name, file_type='PDF', method='Print') - link_expired = False + is_invalid_print = False + print_style = None try: body = get_rendered_template(doc, print_format = print_format, meta=meta, trigger_print = frappe.form_dict.trigger_print, no_letterhead=frappe.form_dict.no_letterhead, letterhead=letterhead, settings=settings) + print_style = get_print_style(frappe.form_dict.style, print_format) except frappe.exceptions.LinkExpiredError: - body = "Link Expired" - link_expired = True + body = frappe.get_template("templates/print_format/print_key_expired.html").render({}) + context.http_status_code = 410 + is_invalid_print = True + except frappe.exceptions.InvalidKey: + body = frappe.get_template("templates/print_format/print_key_invalid.html").render({}) + context.http_status_code = 401 + is_invalid_print = True return { "body": body, - "css": get_print_style(frappe.form_dict.style, print_format), + "print_style": print_style, "comment": frappe.session.user, - "title": doc.get(meta.title_field) if meta.title_field else doc.name, + "title": frappe.utils.strip_html(doc.get_title()), "lang": frappe.local.lang, "layout_direction": "rtl" if is_rtl() else "ltr", - "link_expired": link_expired + "is_invalid_print": is_invalid_print } def get_print_format_doc(print_format_name, meta): @@ -234,25 +241,33 @@ def get_rendered_raw_commands(doc, name=None, print_format=None, meta=None, lang } def validate_print_permission(doc): - if frappe.form_dict.get("key"): - document_key = frappe.db.exists("Document Key", { - "reference_doctype": doc.doctype, - "reference_docname": doc.name, - "key": frappe.form_dict.key - }, cache=True) - if document_key: - if frappe.get_cached_doc("Document Key", document_key).is_expired(): - raise frappe.exceptions.LinkExpiredError - else: - return - - if frappe.form_dict.key == doc.get_signature(): + for ptype in ("read", "print"): + if (frappe.has_permission(doc.doctype, ptype, doc) or frappe.has_website_permission(doc)): return - for ptype in ("read", "print"): - if (not frappe.has_permission(doc.doctype, ptype, doc) - and not frappe.has_website_permission(doc)): - raise frappe.PermissionError(_("No {0} permission").format(ptype)) + key = frappe.form_dict.get("key") + if key: + validate_key(key, doc) + else: + raise frappe.PermissionError(_("You do not have permission to view this document")) + +def validate_key(key, doc): + document_key = frappe.db.exists("Document Key", { + "reference_doctype": doc.doctype, + "reference_docname": doc.name, + "key": key + }, cache=True) + if document_key: + if frappe.get_cached_doc("Document Key", document_key).is_expired(): + raise frappe.exceptions.LinkExpiredError + else: + return + + # TODO: Deprecate this! kept it for backward compatibility + if key == doc.get_signature(): + return + + raise frappe.exceptions.InvalidKey def get_letter_head(doc, no_letterhead, letterhead=None): if no_letterhead: From b7f156f58074bd5318ce07753cca3313bf94caa5 Mon Sep 17 00:00:00 2001 From: Suraj Shetty Date: Mon, 7 Feb 2022 13:25:39 +0530 Subject: [PATCH 010/340] fix: Rename "Document Key" to "Document Share Key" --- frappe/core/doctype/communication/mixins.py | 2 +- frappe/core/doctype/doctype/doctype.json | 10 +++++----- .../{document_key => document_share_key}/__init__.py | 0 .../document_share_key.js} | 2 +- .../document_share_key.json} | 6 +++--- .../document_share_key.py} | 4 ++-- .../test_document_share_key.py} | 2 +- .../custom/doctype/customize_form/customize_form.json | 10 +++++----- frappe/model/document.py | 4 ++-- frappe/public/js/frappe/form/form.js | 2 +- frappe/www/printview.py | 6 +++--- 11 files changed, 24 insertions(+), 24 deletions(-) rename frappe/core/doctype/{document_key => document_share_key}/__init__.py (100%) rename frappe/core/doctype/{document_key/document_key.js => document_share_key/document_share_key.js} (78%) rename frappe/core/doctype/{document_key/document_key.json => document_share_key/document_share_key.json} (91%) rename frappe/core/doctype/{document_key/document_key.py => document_share_key/document_share_key.py} (87%) rename frappe/core/doctype/{document_key/test_document_key.py => document_share_key/test_document_share_key.py} (71%) diff --git a/frappe/core/doctype/communication/mixins.py b/frappe/core/doctype/communication/mixins.py index 1d1d162728..0247354d1c 100644 --- a/frappe/core/doctype/communication/mixins.py +++ b/frappe/core/doctype/communication/mixins.py @@ -149,7 +149,7 @@ class CommunicationEmailMixin: "doctype": self.reference_doctype, "name": self.reference_name, "print_format": print_format, - "key": get_parent_doc(self).get_new_document_key() + "key": get_parent_doc(self).get_new_document_share_key() }) def get_outgoing_email_account(self): diff --git a/frappe/core/doctype/doctype/doctype.json b/frappe/core/doctype/doctype/doctype.json index 9a777c740a..0b643eb03d 100644 --- a/frappe/core/doctype/doctype/doctype.json +++ b/frappe/core/doctype/doctype/doctype.json @@ -5,7 +5,7 @@ "creation": "2013-02-18 13:36:19", "description": "DocType is a Table / Form in the application.", "doctype": "DocType", - "document_key_expiry": 90, + "document_share_key_expiry": 90, "document_type": "Document", "engine": "InnoDB", "field_order": [ @@ -57,7 +57,7 @@ "color", "show_preview_popup", "show_name_in_global_search", - "document_key_expiry", + "document_share_key_expiry", "email_settings_sb", "default_email_template", "column_break_51", @@ -587,10 +587,10 @@ }, { "default": "90", - "description": "Set number of days after which a document key will be expired", - "fieldname": "document_key_expiry", + "description": "Set number of days after which a Document Share Key will be expired", + "fieldname": "document_share_key_expiry", "fieldtype": "Int", - "label": "Document Key Expiry (in Days)" + "label": "Document Share Key Expiry (in Days)" } ], "icon": "fa fa-bolt", diff --git a/frappe/core/doctype/document_key/__init__.py b/frappe/core/doctype/document_share_key/__init__.py similarity index 100% rename from frappe/core/doctype/document_key/__init__.py rename to frappe/core/doctype/document_share_key/__init__.py diff --git a/frappe/core/doctype/document_key/document_key.js b/frappe/core/doctype/document_share_key/document_share_key.js similarity index 78% rename from frappe/core/doctype/document_key/document_key.js rename to frappe/core/doctype/document_share_key/document_share_key.js index e8bda74ad2..c51233e10f 100644 --- a/frappe/core/doctype/document_key/document_key.js +++ b/frappe/core/doctype/document_share_key/document_share_key.js @@ -1,7 +1,7 @@ // Copyright (c) 2021, Frappe Technologies and contributors // For license information, please see license.txt -frappe.ui.form.on('Document Key', { +frappe.ui.form.on('Document Share Key', { // refresh: function(frm) { // } diff --git a/frappe/core/doctype/document_key/document_key.json b/frappe/core/doctype/document_share_key/document_share_key.json similarity index 91% rename from frappe/core/doctype/document_key/document_key.json rename to frappe/core/doctype/document_share_key/document_share_key.json index e24d786ce5..116f3c68e4 100644 --- a/frappe/core/doctype/document_key/document_key.json +++ b/frappe/core/doctype/document_share_key/document_share_key.json @@ -1,10 +1,10 @@ { "actions": [], "allow_rename": 1, - "autoname": "format:DOCUMENT-KEY-{########}", + "autoname": "format:DOCUMENT-SHARE-KEY-{########}", "creation": "2022-01-14 13:40:49.487646", "doctype": "DocType", - "document_key_expiry": 90, + "document_share_key_expiry": 90, "editable_grid": 1, "engine": "InnoDB", "field_order": [ @@ -50,7 +50,7 @@ "modified": "2022-01-14 13:57:28.050678", "modified_by": "Administrator", "module": "Core", - "name": "Document Key", + "name": "Document Share Key", "naming_rule": "Expression", "owner": "Administrator", "permissions": [ diff --git a/frappe/core/doctype/document_key/document_key.py b/frappe/core/doctype/document_share_key/document_share_key.py similarity index 87% rename from frappe/core/doctype/document_key/document_key.py rename to frappe/core/doctype/document_share_key/document_share_key.py index 86ae4f3a68..1b4471f50b 100644 --- a/frappe/core/doctype/document_key/document_key.py +++ b/frappe/core/doctype/document_share_key/document_share_key.py @@ -4,12 +4,12 @@ import frappe from frappe.model.document import Document -class DocumentKey(Document): +class DocumentShareKey(Document): def before_insert(self): self.key = frappe.generate_hash(length=32) if not self.expires_on: meta = frappe.get_meta("DocType", self.reference_doctype) - self.expires_on = frappe.utils.add_days(None, days=meta.get("document_key_expiry") or 90) + self.expires_on = frappe.utils.add_days(None, days=meta.get("document_share_key_expiry") or 90) def is_expired(self): return self.expires_on < frappe.utils.getdate() diff --git a/frappe/core/doctype/document_key/test_document_key.py b/frappe/core/doctype/document_share_key/test_document_share_key.py similarity index 71% rename from frappe/core/doctype/document_key/test_document_key.py rename to frappe/core/doctype/document_share_key/test_document_share_key.py index 5930ce72b4..43ebf7caff 100644 --- a/frappe/core/doctype/document_key/test_document_key.py +++ b/frappe/core/doctype/document_share_key/test_document_share_key.py @@ -4,5 +4,5 @@ # import frappe import unittest -class TestDocumentKey(unittest.TestCase): +class TestDocumentShareKey(unittest.TestCase): pass diff --git a/frappe/custom/doctype/customize_form/customize_form.json b/frappe/custom/doctype/customize_form/customize_form.json index b1c67345d9..63a9637327 100644 --- a/frappe/custom/doctype/customize_form/customize_form.json +++ b/frappe/custom/doctype/customize_form/customize_form.json @@ -3,7 +3,7 @@ "autoname": "DL.####", "creation": "2013-01-29 17:55:08", "doctype": "DocType", - "document_key_expiry": 90, + "document_share_key_expiry": 90, "document_type": "Document", "editable_grid": 1, "engine": "InnoDB", @@ -32,7 +32,7 @@ "default_print_format", "column_break_29", "show_preview_popup", - "document_key_expiry", + "document_share_key_expiry", "email_settings_section", "default_email_template", "column_break_26", @@ -301,10 +301,10 @@ }, { "default": "90", - "description": "Set number of days after which a document key will be expired", - "fieldname": "document_key_expiry", + "description": "Set number of days after which a Document Share Key will be expired", + "fieldname": "document_share_key_expiry", "fieldtype": "Int", - "label": "Document Key Expiry (in Days)" + "label": "Document Share Key Expiry (in Days)" } ], "hide_toolbar": 1, diff --git a/frappe/model/document.py b/frappe/model/document.py index a36d1efa0e..6b27049d08 100644 --- a/frappe/model/document.py +++ b/frappe/model/document.py @@ -1278,8 +1278,8 @@ class Document(BaseDocument): return hashlib.sha224(get_datetime_str(self.creation).encode()).hexdigest() @frappe.whitelist() - def get_new_document_key(self, expires_on=None): - doc = frappe.new_doc("Document Key") + def get_new_document_share_key(self, expires_on=None): + doc = frappe.new_doc("Document Share Key") doc.reference_doctype = self.doctype doc.reference_docname = self.name doc.expires_on = expires_on diff --git a/frappe/public/js/frappe/form/form.js b/frappe/public/js/frappe/form/form.js index 81677c519f..5bc25fbfd5 100644 --- a/frappe/public/js/frappe/form/form.js +++ b/frappe/public/js/frappe/form/form.js @@ -1170,7 +1170,7 @@ frappe.ui.form.Form = class FrappeForm { fieldtype: "Button", label: "Generate Link", click: () => { - this.call("get_new_document_key").then(res => { + this.call("get_new_document_share_key").then(res => { let key = res.message; share_document_url_dialog.set_value("link", this.get_share_link(key)); }); diff --git a/frappe/www/printview.py b/frappe/www/printview.py index ee990b5fca..ac575baf0d 100644 --- a/frappe/www/printview.py +++ b/frappe/www/printview.py @@ -252,13 +252,13 @@ def validate_print_permission(doc): raise frappe.PermissionError(_("You do not have permission to view this document")) def validate_key(key, doc): - document_key = frappe.db.exists("Document Key", { + document_share_key = frappe.db.exists("Document Share Key", { "reference_doctype": doc.doctype, "reference_docname": doc.name, "key": key }, cache=True) - if document_key: - if frappe.get_cached_doc("Document Key", document_key).is_expired(): + if document_share_key: + if frappe.get_cached_doc("Document Share Key", document_share_key).is_expired(): raise frappe.exceptions.LinkExpiredError else: return From 24908e31a890d557e0ae37b000972698ba952542 Mon Sep 17 00:00:00 2001 From: Suraj Shetty Date: Mon, 7 Feb 2022 13:27:57 +0530 Subject: [PATCH 011/340] fix: Allow document sharing who has share rights --- frappe/public/js/frappe/form/toolbar.js | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/frappe/public/js/frappe/form/toolbar.js b/frappe/public/js/frappe/form/toolbar.js index f5153eb67e..431aa68501 100644 --- a/frappe/public/js/frappe/form/toolbar.js +++ b/frappe/public/js/frappe/form/toolbar.js @@ -266,7 +266,9 @@ frappe.ui.form.Toolbar = class Toolbar { } // share print view link - if (frappe.model.can_email(null, me.frm) && me.frm.doc.docstatus < 2) { + if (frappe.model.can_email(null, me.frm) + && frappe.model.can_share(null, me.frm) + && me.frm.doc.docstatus < 2) { this.page.add_menu_item(__("Share Link", [me.frm.doctype]), function() { me.frm.share_doc_link(); }, true); From a0af8d577fc5e39a65d3096f1d484db0d65507b0 Mon Sep 17 00:00:00 2001 From: Suraj Shetty Date: Mon, 7 Feb 2022 13:42:28 +0530 Subject: [PATCH 012/340] fix: Move document_share_key_expiry to System Settings - Having `document_share_key_expiry` on each doctype is overkill so kept it at System Settings --- frappe/core/doctype/doctype/doctype.json | 11 +---------- .../document_share_key/document_share_key.json | 1 - .../document_share_key/document_share_key.py | 3 +-- .../doctype/system_settings/system_settings.json | 13 +++++++++++-- .../doctype/customize_form/customize_form.json | 9 --------- 5 files changed, 13 insertions(+), 24 deletions(-) diff --git a/frappe/core/doctype/doctype/doctype.json b/frappe/core/doctype/doctype/doctype.json index 0b643eb03d..167d0320d5 100644 --- a/frappe/core/doctype/doctype/doctype.json +++ b/frappe/core/doctype/doctype/doctype.json @@ -5,7 +5,6 @@ "creation": "2013-02-18 13:36:19", "description": "DocType is a Table / Form in the application.", "doctype": "DocType", - "document_share_key_expiry": 90, "document_type": "Document", "engine": "InnoDB", "field_order": [ @@ -57,7 +56,6 @@ "color", "show_preview_popup", "show_name_in_global_search", - "document_share_key_expiry", "email_settings_sb", "default_email_template", "column_break_51", @@ -584,13 +582,6 @@ "fieldname": "document_states_section", "fieldtype": "Section Break", "label": "Document States" - }, - { - "default": "90", - "description": "Set number of days after which a Document Share Key will be expired", - "fieldname": "document_share_key_expiry", - "fieldtype": "Int", - "label": "Document Share Key Expiry (in Days)" } ], "icon": "fa fa-bolt", @@ -672,7 +663,7 @@ "link_fieldname": "reference_doctype" } ], - "modified": "2021-12-28 03:19:19.033347", + "modified": "2022-02-07 03:03:03.847356", "modified_by": "Administrator", "module": "Core", "name": "DocType", diff --git a/frappe/core/doctype/document_share_key/document_share_key.json b/frappe/core/doctype/document_share_key/document_share_key.json index 116f3c68e4..6142050c0c 100644 --- a/frappe/core/doctype/document_share_key/document_share_key.json +++ b/frappe/core/doctype/document_share_key/document_share_key.json @@ -4,7 +4,6 @@ "autoname": "format:DOCUMENT-SHARE-KEY-{########}", "creation": "2022-01-14 13:40:49.487646", "doctype": "DocType", - "document_share_key_expiry": 90, "editable_grid": 1, "engine": "InnoDB", "field_order": [ diff --git a/frappe/core/doctype/document_share_key/document_share_key.py b/frappe/core/doctype/document_share_key/document_share_key.py index 1b4471f50b..69dbf70ade 100644 --- a/frappe/core/doctype/document_share_key/document_share_key.py +++ b/frappe/core/doctype/document_share_key/document_share_key.py @@ -8,8 +8,7 @@ class DocumentShareKey(Document): def before_insert(self): self.key = frappe.generate_hash(length=32) if not self.expires_on: - meta = frappe.get_meta("DocType", self.reference_doctype) - self.expires_on = frappe.utils.add_days(None, days=meta.get("document_share_key_expiry") or 90) + self.expires_on = frappe.utils.add_days(None, days=frappe.get_system_settings("document_share_key_expiry") or 90) def is_expired(self): return self.expires_on < frappe.utils.getdate() diff --git a/frappe/core/doctype/system_settings/system_settings.json b/frappe/core/doctype/system_settings/system_settings.json index 61410fb1a8..ec5858dfb6 100644 --- a/frappe/core/doctype/system_settings/system_settings.json +++ b/frappe/core/doctype/system_settings/system_settings.json @@ -1,7 +1,8 @@ { "actions": [], - "creation": "2014-04-17 16:53:52.640856", + "creation": "2022-01-06 03:18:16.326761", "doctype": "DocType", + "document_share_key_expiry": 90, "document_type": "System", "engine": "InnoDB", "field_order": [ @@ -35,6 +36,7 @@ "security", "session_expiry", "session_expiry_mobile", + "document_share_key_expiry", "column_break_13", "deny_multiple_sessions", "allow_login_using_mobile_number", @@ -485,12 +487,19 @@ "fieldtype": "Select", "label": "First Day of the Week", "options": "Sunday\nMonday\nTuesday\nWednesday\nThursday\nFriday\nSaturday" + }, + { + "default": "90", + "description": "Set number of days after which a Document Share Key will be expired", + "fieldname": "document_share_key_expiry", + "fieldtype": "Int", + "label": "Document Share Key Expiry (in Days)" } ], "icon": "fa fa-cog", "issingle": 1, "links": [], - "modified": "2022-01-04 11:28:34.881192", + "modified": "2022-02-07 03:07:20.782827", "modified_by": "Administrator", "module": "Core", "name": "System Settings", diff --git a/frappe/custom/doctype/customize_form/customize_form.json b/frappe/custom/doctype/customize_form/customize_form.json index 63a9637327..88695f5cd9 100644 --- a/frappe/custom/doctype/customize_form/customize_form.json +++ b/frappe/custom/doctype/customize_form/customize_form.json @@ -3,7 +3,6 @@ "autoname": "DL.####", "creation": "2013-01-29 17:55:08", "doctype": "DocType", - "document_share_key_expiry": 90, "document_type": "Document", "editable_grid": 1, "engine": "InnoDB", @@ -32,7 +31,6 @@ "default_print_format", "column_break_29", "show_preview_popup", - "document_share_key_expiry", "email_settings_section", "default_email_template", "column_break_26", @@ -298,13 +296,6 @@ "fieldtype": "Table", "label": "States", "options": "DocType State" - }, - { - "default": "90", - "description": "Set number of days after which a Document Share Key will be expired", - "fieldname": "document_share_key_expiry", - "fieldtype": "Int", - "label": "Document Share Key Expiry (in Days)" } ], "hide_toolbar": 1, From 7b69497841be6dfc7553a28e793ca678b31d68e2 Mon Sep 17 00:00:00 2001 From: Suraj Shetty Date: Mon, 7 Feb 2022 16:25:17 +0530 Subject: [PATCH 013/340] fix: Add setting "allow_older_web_view_links" in System Settings --- .../doctype/system_settings/system_settings.json | 12 +++++++++--- frappe/www/printview.py | 2 +- 2 files changed, 10 insertions(+), 4 deletions(-) diff --git a/frappe/core/doctype/system_settings/system_settings.json b/frappe/core/doctype/system_settings/system_settings.json index ec5858dfb6..0fad42b3af 100644 --- a/frappe/core/doctype/system_settings/system_settings.json +++ b/frappe/core/doctype/system_settings/system_settings.json @@ -2,7 +2,6 @@ "actions": [], "creation": "2022-01-06 03:18:16.326761", "doctype": "DocType", - "document_share_key_expiry": 90, "document_type": "System", "engine": "InnoDB", "field_order": [ @@ -43,6 +42,7 @@ "allow_login_using_user_name", "allow_error_traceback", "strip_exif_metadata_from_uploaded_images", + "allow_older_web_view_links", "password_settings", "logout_on_password_reset", "force_user_to_reset_password", @@ -490,16 +490,22 @@ }, { "default": "90", - "description": "Set number of days after which a Document Share Key will be expired", + "description": "Number of days after which the shared document link will be expired", "fieldname": "document_share_key_expiry", "fieldtype": "Int", "label": "Document Share Key Expiry (in Days)" + }, + { + "default": "1", + "fieldname": "allow_older_web_view_links", + "fieldtype": "Check", + "label": "Allow Older Web View Links (Insecure)" } ], "icon": "fa fa-cog", "issingle": 1, "links": [], - "modified": "2022-02-07 03:07:20.782827", + "modified": "2022-02-07 03:36:17.319666", "modified_by": "Administrator", "module": "Core", "name": "System Settings", diff --git a/frappe/www/printview.py b/frappe/www/printview.py index afd38000bc..dabeb1462b 100644 --- a/frappe/www/printview.py +++ b/frappe/www/printview.py @@ -263,7 +263,7 @@ def validate_key(key, doc): return # TODO: Deprecate this! kept it for backward compatibility - if key == doc.get_signature(): + if frappe.get_system_settings("allow_older_web_view_links") and key == doc.get_signature(): return raise frappe.exceptions.InvalidKey From e6c149ee168160a58af2457e4a4627f9bc0bbd7d Mon Sep 17 00:00:00 2001 From: Suraj Shetty Date: Mon, 7 Feb 2022 17:21:16 +0530 Subject: [PATCH 014/340] fix: Re-use document key if key with same expiry exists --- frappe/model/document.py | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-) diff --git a/frappe/model/document.py b/frappe/model/document.py index b686135d15..8c970bf71f 100644 --- a/frappe/model/document.py +++ b/frappe/model/document.py @@ -1284,11 +1284,20 @@ class Document(BaseDocument): @frappe.whitelist() def get_new_document_share_key(self, expires_on=None): - doc = frappe.new_doc("Document Share Key") - doc.reference_doctype = self.doctype - doc.reference_docname = self.name - doc.expires_on = expires_on - doc.insert(ignore_permissions=True) + document_key_exist = frappe.db.exists("Document Share Key", { + "reference_doctype": self.doctype, + "reference_docname": self.name, + "expires_on": expires_on, + }) + if document_key_exist: + doc = frappe.get_doc("Document Share Key", document_key_exist) + else: + doc = frappe.new_doc("Document Share Key") + doc.reference_doctype = self.doctype + doc.reference_docname = self.name + doc.expires_on = expires_on + doc.insert(ignore_permissions=True) + return doc.key def get_liked_by(self): From 4a89081cc97e0703e6a2ace3a5bafe9b2ba9e114 Mon Sep 17 00:00:00 2001 From: Suraj Shetty Date: Mon, 7 Feb 2022 17:28:14 +0530 Subject: [PATCH 015/340] fix: Move share link functionality to sidebar (WIP) --- frappe/public/js/frappe/form/sidebar/share.js | 58 +++++++++++++++++++ .../frappe/form/templates/form_sidebar.html | 7 +++ 2 files changed, 65 insertions(+) diff --git a/frappe/public/js/frappe/form/sidebar/share.js b/frappe/public/js/frappe/form/sidebar/share.js index 8b73684475..766bfb5359 100644 --- a/frappe/public/js/frappe/form/sidebar/share.js +++ b/frappe/public/js/frappe/form/sidebar/share.js @@ -7,6 +7,7 @@ frappe.ui.form.Share = class Share { constructor(opts) { $.extend(this, opts); this.shares = this.parent.find('.shares'); + this.share_link = this.parent.find('.share-link'); } refresh() { this.render_sidebar(); @@ -14,6 +15,9 @@ frappe.ui.form.Share = class Share { render_sidebar() { const shared = this.shared || this.frm.get_docinfo().shared; const shared_users = shared.filter(Boolean).map(s => s.user); + this.share_link.click(() => { + this.share_modal(); + }); if (this.frm.is_new()) { this.parent.find(".share-doc-btn").hide(); @@ -187,4 +191,58 @@ frappe.ui.form.Share = class Share { }); }); } + async share_modal() { + let document_links = await frappe.db.get_list("Document Share Key", { + filters: { + reference_doctype: this.frm.doctype, + reference_docname: this.frm.docname + }, fields: ['key', 'expires_on'] + }); + const share_modal = new frappe.ui.Dialog({ + title: __("Share Link"), + fields: [{ + fieldname: "link_expiration_date", + label: __("Link Expiration Date"), + fieldtype: "Date", + min_date: new Date(), + read_only_depends_on: "eval: doc.link" + }, { + fieldtype: "Button", + label: __("Generate Link"), + click: () => { + this.frm.call("get_new_document_share_key", { + expires_on: share_modal.get_value("link_expiration_date") + }).then(res => { + let key = res.message; + share_modal.set_value("link", this.frm.get_share_link(key)); + }); + } + }, { + fieldname: "link", + label: __("Link"), + fieldtype: "Data", + depends_on: "eval: doc.link", + with_copy_button: true + }, { + fieldname: "links", + label: __("Previous Links"), + fieldtype: "Table", + data: document_links, + read_only: 1, + fields: [{ + fieldname: "key", + label: __("Key"), + fieldtype: "Data", + in_list_view: true, + columns: 6 + }, { + fieldname: "expires_on", + label: __("Expires On"), + fieldtype: "Date", + in_list_view: true + }] + }], + }); + share_modal.show(); + } }; diff --git a/frappe/public/js/frappe/form/templates/form_sidebar.html b/frappe/public/js/frappe/form/templates/form_sidebar.html index dcea2f4647..c11913c7f0 100644 --- a/frappe/public/js/frappe/form/templates/form_sidebar.html +++ b/frappe/public/js/frappe/form/templates/form_sidebar.html @@ -87,6 +87,13 @@ +
    From a4e7b2a2629dba680e07a5292e4507bb56cdd202 Mon Sep 17 00:00:00 2001 From: Suraj Shetty Date: Fri, 18 Feb 2022 14:25:39 +0530 Subject: [PATCH 016/340] fix: Share link style --- frappe/public/scss/desk/sidebar.scss | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/frappe/public/scss/desk/sidebar.scss b/frappe/public/scss/desk/sidebar.scss index 0bb6ba5f40..f906ce980f 100644 --- a/frappe/public/scss/desk/sidebar.scss +++ b/frappe/public/scss/desk/sidebar.scss @@ -493,3 +493,11 @@ body[data-route^="Module"] .main-menu { .shared-user { margin-bottom: 10px; } + +.share-link { + margin-left: 7px; + .avatar-frame { + padding: 2px; + border: 1px solid var(--gray-200); + } +} \ No newline at end of file From af6784992848bb1f51f5dfe5ed460fd4d120a64a Mon Sep 17 00:00:00 2001 From: Suraj Shetty Date: Fri, 18 Feb 2022 14:26:33 +0530 Subject: [PATCH 017/340] feat: Option to Print or download PDF in print view --- frappe/public/scss/print.bundle.scss | 8 ++++++++ frappe/utils/print_format.py | 7 ++++++- frappe/www/printview.html | 7 +++++++ frappe/www/printview.py | 5 ++++- 4 files changed, 25 insertions(+), 2 deletions(-) diff --git a/frappe/public/scss/print.bundle.scss b/frappe/public/scss/print.bundle.scss index 506f0670a2..53f2c463dd 100644 --- a/frappe/public/scss/print.bundle.scss +++ b/frappe/public/scss/print.bundle.scss @@ -13,8 +13,16 @@ svg[data-barcode-value] > g { fill: black !important; } + .print-hide { + display: none !important; + } } +.action-banner { + display: flex; + justify-content: flex-end; + padding-right: 20px; +} .invalid-state { display: grid; diff --git a/frappe/utils/print_format.py b/frappe/utils/print_format.py index 06f15ced27..b41fedf201 100644 --- a/frappe/utils/print_format.py +++ b/frappe/utils/print_format.py @@ -10,6 +10,8 @@ no_cache = 1 base_template_path = "www/printview.html" standard_format = "templates/print_formats/standard.html" +from frappe.www.printview import validate_print_permission + @frappe.whitelist() def download_multi_pdf(doctype, name, format=None, no_letterhead=False, options=None): """ @@ -86,8 +88,11 @@ def read_multi_pdf(output): return filedata -@frappe.whitelist() +@frappe.whitelist(allow_guest=True) def download_pdf(doctype, name, format=None, doc=None, no_letterhead=0): + doc = frappe.get_doc(doctype, name) + doc.doctype = doctype + validate_print_permission(doc) html = frappe.get_print(doctype, name, format, doc=doc, no_letterhead=no_letterhead) frappe.local.response.filename = "{name}.pdf".format(name=name.replace(" ", "-").replace("/", "-")) frappe.local.response.filecontent = get_pdf(html) diff --git a/frappe/www/printview.html b/frappe/www/printview.html index 73d2bdb731..f42c423dce 100644 --- a/frappe/www/printview.html +++ b/frappe/www/printview.html @@ -16,6 +16,13 @@ {% if is_invalid_print %} {{ body }} {% else %} +
    + Print + + Get PDF + +
    {{ body }} diff --git a/frappe/www/printview.py b/frappe/www/printview.py index a3e73ba6ca..b9eb3888a4 100644 --- a/frappe/www/printview.py +++ b/frappe/www/printview.py @@ -60,7 +60,10 @@ def get_context(context): "title": frappe.utils.strip_html(doc.get_title()), "lang": frappe.local.lang, "layout_direction": "rtl" if is_rtl() else "ltr", - "is_invalid_print": is_invalid_print + "is_invalid_print": is_invalid_print, + "doctype": frappe.form_dict.doctype, + "name": frappe.form_dict.name, + "key": frappe.form_dict.get("key") } def get_print_format_doc(print_format_name, meta): From 0ce8f83343e5199154d8b954d5ed52978bf1fba8 Mon Sep 17 00:00:00 2001 From: Suraj Shetty Date: Fri, 18 Feb 2022 14:28:00 +0530 Subject: [PATCH 018/340] feat: Ability to create share link without expiry --- .../doctype/document_share_key/document_share_key.py | 4 ++-- frappe/model/document.py | 3 ++- frappe/public/js/frappe/form/sidebar/share.js | 11 +++++++++++ 3 files changed, 15 insertions(+), 3 deletions(-) diff --git a/frappe/core/doctype/document_share_key/document_share_key.py b/frappe/core/doctype/document_share_key/document_share_key.py index 69dbf70ade..a2e6556249 100644 --- a/frappe/core/doctype/document_share_key/document_share_key.py +++ b/frappe/core/doctype/document_share_key/document_share_key.py @@ -7,8 +7,8 @@ from frappe.model.document import Document class DocumentShareKey(Document): def before_insert(self): self.key = frappe.generate_hash(length=32) - if not self.expires_on: + if not self.expires_on and not self.flags.no_expiry: self.expires_on = frappe.utils.add_days(None, days=frappe.get_system_settings("document_share_key_expiry") or 90) def is_expired(self): - return self.expires_on < frappe.utils.getdate() + return self.expires_on and self.expires_on < frappe.utils.getdate() diff --git a/frappe/model/document.py b/frappe/model/document.py index e8ea34b22b..2eecac2a57 100644 --- a/frappe/model/document.py +++ b/frappe/model/document.py @@ -1279,7 +1279,7 @@ class Document(BaseDocument): return hashlib.sha224(get_datetime_str(self.creation).encode()).hexdigest() @frappe.whitelist() - def get_new_document_share_key(self, expires_on=None): + def get_new_document_share_key(self, expires_on=None, no_expiry=False): document_key_exist = frappe.db.exists("Document Share Key", { "reference_doctype": self.doctype, "reference_docname": self.name, @@ -1292,6 +1292,7 @@ class Document(BaseDocument): doc.reference_doctype = self.doctype doc.reference_docname = self.name doc.expires_on = expires_on + doc.flags.no_expiry = no_expiry doc.insert(ignore_permissions=True) return doc.key diff --git a/frappe/public/js/frappe/form/sidebar/share.js b/frappe/public/js/frappe/form/sidebar/share.js index 766bfb5359..f7ab619a86 100644 --- a/frappe/public/js/frappe/form/sidebar/share.js +++ b/frappe/public/js/frappe/form/sidebar/share.js @@ -217,6 +217,17 @@ frappe.ui.form.Share = class Share { share_modal.set_value("link", this.frm.get_share_link(key)); }); } + }, { + fieldtype: "Button", + label: __("Generate Link without Expiry"), + click: () => { + this.frm.call("get_new_document_share_key", { + expires_on: null + }).then(res => { + let key = res.message; + share_modal.set_value("link", this.frm.get_share_link(key)); + }); + } }, { fieldname: "link", label: __("Link"), From 54b6bbce96941d4f73dc09d30434cb29fc3de3ca Mon Sep 17 00:00:00 2001 From: Suraj Shetty Date: Sat, 19 Feb 2022 10:57:58 +0530 Subject: [PATCH 019/340] wip --- frappe/public/js/frappe/form/sidebar/share.js | 7 +++++++ frappe/utils/print_format.py | 14 +++++++++++++- 2 files changed, 20 insertions(+), 1 deletion(-) diff --git a/frappe/public/js/frappe/form/sidebar/share.js b/frappe/public/js/frappe/form/sidebar/share.js index f7ab619a86..44dcb2aaca 100644 --- a/frappe/public/js/frappe/form/sidebar/share.js +++ b/frappe/public/js/frappe/form/sidebar/share.js @@ -240,6 +240,7 @@ frappe.ui.form.Share = class Share { fieldtype: "Table", data: document_links, read_only: 1, + hidden: 1, fields: [{ fieldname: "key", label: __("Key"), @@ -252,8 +253,14 @@ frappe.ui.form.Share = class Share { fieldtype: "Date", in_list_view: true }] + }, { + fieldname: "links", + label: __("Previous Links"), + fieldtype: "HTML", + options: `
    Links
    `, }], }); + share_modal.show(); } }; diff --git a/frappe/utils/print_format.py b/frappe/utils/print_format.py index b41fedf201..da2d5e4893 100644 --- a/frappe/utils/print_format.py +++ b/frappe/utils/print_format.py @@ -92,7 +92,19 @@ def read_multi_pdf(output): def download_pdf(doctype, name, format=None, doc=None, no_letterhead=0): doc = frappe.get_doc(doctype, name) doc.doctype = doctype - validate_print_permission(doc) + try: + validate_print_permission(doc) + except frappe.exceptions.LinkExpiredError: + frappe.local.response.http_status_code = 410 + frappe.local.response = frappe.get_template("templates/print_format/print_key_invalid.html").render({}) + return + except frappe.exceptions.InvalidKey: + frappe.local.response.type = "page" + frappe.local.response.message = frappe.get_template("templates/print_format/print_key_invalid.html").render({}) + frappe.local.response.http_status_code = 401 + # frappe.local.response.route = "/api/method/frappe.utils.print_format.download_pdf" + return + html = frappe.get_print(doctype, name, format, doc=doc, no_letterhead=no_letterhead) frappe.local.response.filename = "{name}.pdf".format(name=name.replace(" ", "-").replace("/", "-")) frappe.local.response.filecontent = get_pdf(html) From eb1ed9da9dd4f59680549386807298f99e634f43 Mon Sep 17 00:00:00 2001 From: Suraj Shetty Date: Tue, 1 Mar 2022 19:23:01 +0530 Subject: [PATCH 020/340] fix: Option set no_expiry --- frappe/model/document.py | 17 ++++-- frappe/public/js/frappe/form/sidebar/share.js | 60 ++++++------------- frappe/public/scss/desk/sidebar.scss | 1 + 3 files changed, 30 insertions(+), 48 deletions(-) diff --git a/frappe/model/document.py b/frappe/model/document.py index 6218f267d5..b8d4b8fc71 100644 --- a/frappe/model/document.py +++ b/frappe/model/document.py @@ -1280,12 +1280,17 @@ class Document(BaseDocument): @frappe.whitelist() def get_new_document_share_key(self, expires_on=None, no_expiry=False): - document_key_exist = frappe.db.exists("Document Share Key", { - "reference_doctype": self.doctype, - "reference_docname": self.name, - "expires_on": expires_on, - }) - if document_key_exist: + if no_expiry: + expires_on = None + + if document_key_exist := frappe.db.exists( + "Document Share Key", + { + "reference_doctype": self.doctype, + "reference_docname": self.name, + "expires_on": expires_on, + }, + ): doc = frappe.get_doc("Document Share Key", document_key_exist) else: doc = frappe.new_doc("Document Share Key") diff --git a/frappe/public/js/frappe/form/sidebar/share.js b/frappe/public/js/frappe/form/sidebar/share.js index 44dcb2aaca..42f09f9ad6 100644 --- a/frappe/public/js/frappe/form/sidebar/share.js +++ b/frappe/public/js/frappe/form/sidebar/share.js @@ -192,37 +192,37 @@ frappe.ui.form.Share = class Share { }); } async share_modal() { - let document_links = await frappe.db.get_list("Document Share Key", { - filters: { - reference_doctype: this.frm.doctype, - reference_docname: this.frm.docname - }, fields: ['key', 'expires_on'] - }); + const default_expiry = frappe.boot.sysdefaults.document_share_key_expiry; const share_modal = new frappe.ui.Dialog({ title: __("Share Link"), fields: [{ fieldname: "link_expiration_date", label: __("Link Expiration Date"), fieldtype: "Date", + default: frappe.datetime.add_days(moment(), default_expiry), min_date: new Date(), + description: __("The default expiration date is {0} days from today.", [default_expiry]), read_only_depends_on: "eval: doc.link" }, { - fieldtype: "Button", - label: __("Generate Link"), - click: () => { - this.frm.call("get_new_document_share_key", { - expires_on: share_modal.get_value("link_expiration_date") - }).then(res => { - let key = res.message; - share_modal.set_value("link", this.frm.get_share_link(key)); - }); - } + fieldtype: "Check", + label: __("No Expiry"), + fieldname: "no_expiry", + change: () => { + const no_expiry_warning = ` + ${__('It is not recommended to send links without expiry')} + `; + const no_expiry = share_modal.get_value('no_expiry'); + share_modal.get_field('link_expiration_date').toggle(!no_expiry); + share_modal.get_field('no_expiry') + .set_description(`${no_expiry ? no_expiry_warning : ''}`); + }, }, { fieldtype: "Button", - label: __("Generate Link without Expiry"), + label: __("Get Link"), click: () => { this.frm.call("get_new_document_share_key", { - expires_on: null + expires_on: share_modal.get_value("link_expiration_date"), + no_expiry: share_modal.get_value("no_expiry") }).then(res => { let key = res.message; share_modal.set_value("link", this.frm.get_share_link(key)); @@ -234,30 +234,6 @@ frappe.ui.form.Share = class Share { fieldtype: "Data", depends_on: "eval: doc.link", with_copy_button: true - }, { - fieldname: "links", - label: __("Previous Links"), - fieldtype: "Table", - data: document_links, - read_only: 1, - hidden: 1, - fields: [{ - fieldname: "key", - label: __("Key"), - fieldtype: "Data", - in_list_view: true, - columns: 6 - }, { - fieldname: "expires_on", - label: __("Expires On"), - fieldtype: "Date", - in_list_view: true - }] - }, { - fieldname: "links", - label: __("Previous Links"), - fieldtype: "HTML", - options: `
    Links
    `, }], }); diff --git a/frappe/public/scss/desk/sidebar.scss b/frappe/public/scss/desk/sidebar.scss index f906ce980f..cca3c2cc50 100644 --- a/frappe/public/scss/desk/sidebar.scss +++ b/frappe/public/scss/desk/sidebar.scss @@ -496,6 +496,7 @@ body[data-route^="Module"] .main-menu { .share-link { margin-left: 7px; + cursor: pointer; .avatar-frame { padding: 2px; border: 1px solid var(--gray-200); From 99aa2ec7f63235a61333212779efd31708a6b38e Mon Sep 17 00:00:00 2001 From: Suraj Shetty Date: Sun, 13 Mar 2022 19:08:35 +0530 Subject: [PATCH 021/340] fix: Add patch to set document expiry --- frappe/core/doctype/system_settings/system_settings.json | 4 ++-- frappe/patches.txt | 1 + frappe/patches/v14_0/set_document_expiry_default.py | 5 +++++ 3 files changed, 8 insertions(+), 2 deletions(-) create mode 100644 frappe/patches/v14_0/set_document_expiry_default.py diff --git a/frappe/core/doctype/system_settings/system_settings.json b/frappe/core/doctype/system_settings/system_settings.json index 0fad42b3af..3525cf9acc 100644 --- a/frappe/core/doctype/system_settings/system_settings.json +++ b/frappe/core/doctype/system_settings/system_settings.json @@ -489,14 +489,14 @@ "options": "Sunday\nMonday\nTuesday\nWednesday\nThursday\nFriday\nSaturday" }, { - "default": "90", + "default": "30", "description": "Number of days after which the shared document link will be expired", "fieldname": "document_share_key_expiry", "fieldtype": "Int", "label": "Document Share Key Expiry (in Days)" }, { - "default": "1", + "default": "0", "fieldname": "allow_older_web_view_links", "fieldtype": "Check", "label": "Allow Older Web View Links (Insecure)" diff --git a/frappe/patches.txt b/frappe/patches.txt index a666480c90..35d773cce8 100644 --- a/frappe/patches.txt +++ b/frappe/patches.txt @@ -198,3 +198,4 @@ frappe.patches.v14_0.update_github_endpoints #08-11-2021 frappe.patches.v14_0.remove_db_aggregation frappe.patches.v14_0.update_color_names_in_kanban_board_column frappe.patches.v14_0.update_auto_account_deletion_duration +frappe.patches.v14_0.set_document_expiry_default diff --git a/frappe/patches/v14_0/set_document_expiry_default.py b/frappe/patches/v14_0/set_document_expiry_default.py new file mode 100644 index 0000000000..dccaa3a6d9 --- /dev/null +++ b/frappe/patches/v14_0/set_document_expiry_default.py @@ -0,0 +1,5 @@ +import frappe + +def execute(): + frappe.db.set_value("System Settings", "System Settings", "document_share_key_expiry", 30) + frappe.db.set_value("System Settings", "System Settings", "allow_older_web_view_links", 1) From ff5a909d2884895206535e4c3bcf8e9797aaa14e Mon Sep 17 00:00:00 2001 From: Suraj Shetty Date: Sun, 13 Mar 2022 19:11:04 +0530 Subject: [PATCH 022/340] refactor: Update method name get_new_document_share_key -> get_document_share_key --- frappe/core/doctype/communication/mixins.py | 2 +- frappe/model/document.py | 2 +- frappe/public/js/frappe/form/sidebar/share.js | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/frappe/core/doctype/communication/mixins.py b/frappe/core/doctype/communication/mixins.py index f52ba1b394..f3bff8ad65 100644 --- a/frappe/core/doctype/communication/mixins.py +++ b/frappe/core/doctype/communication/mixins.py @@ -150,7 +150,7 @@ class CommunicationEmailMixin: "doctype": self.reference_doctype, "name": self.reference_name, "print_format": print_format, - "key": get_parent_doc(self).get_new_document_share_key() + "key": get_parent_doc(self).get_document_share_key() }) def get_outgoing_email_account(self): diff --git a/frappe/model/document.py b/frappe/model/document.py index a0551d2194..3e7a474969 100644 --- a/frappe/model/document.py +++ b/frappe/model/document.py @@ -1277,7 +1277,7 @@ class Document(BaseDocument): return hashlib.sha224(get_datetime_str(self.creation).encode()).hexdigest() @frappe.whitelist() - def get_new_document_share_key(self, expires_on=None, no_expiry=False): + def get_document_share_key(self, expires_on=None, no_expiry=False): if no_expiry: expires_on = None diff --git a/frappe/public/js/frappe/form/sidebar/share.js b/frappe/public/js/frappe/form/sidebar/share.js index 42f09f9ad6..114983e1d1 100644 --- a/frappe/public/js/frappe/form/sidebar/share.js +++ b/frappe/public/js/frappe/form/sidebar/share.js @@ -220,7 +220,7 @@ frappe.ui.form.Share = class Share { fieldtype: "Button", label: __("Get Link"), click: () => { - this.frm.call("get_new_document_share_key", { + this.frm.call("get_document_share_key", { expires_on: share_modal.get_value("link_expiration_date"), no_expiry: share_modal.get_value("no_expiry") }).then(res => { From b3ef493aa26bbe38bdda771a5ee5d6a35493c1e6 Mon Sep 17 00:00:00 2001 From: Suraj Shetty Date: Sun, 13 Mar 2022 19:13:07 +0530 Subject: [PATCH 023/340] refactor: Remove unnecessary code --- frappe/public/js/frappe/form/form.js | 39 +------------------------ frappe/public/js/frappe/form/toolbar.js | 9 ------ 2 files changed, 1 insertion(+), 47 deletions(-) diff --git a/frappe/public/js/frappe/form/form.js b/frappe/public/js/frappe/form/form.js index 2e9e40f66d..db92e9ab07 100644 --- a/frappe/public/js/frappe/form/form.js +++ b/frappe/public/js/frappe/form/form.js @@ -1149,45 +1149,8 @@ frappe.ui.form.Form = class FrappeForm { }); } - share_doc_link() { - const share_document_url_dialog = new frappe.ui.Dialog({ - fields: [{ - fieldname: "visibility", - label: "Visibility", - fieldtype: "Select", - options: "Public\nPrivate", - default: "Public", - read_only_depends_on: "eval: doc.link" - }, { - fieldname: "link_expiration_date", - label: "Link Expiration Date", - fieldtype: "Date", - min_date: new Date(), - read_only_depends_on: "eval: doc.link" - }, { - fieldtype: "Button", - label: "Generate Link", - click: () => { - this.call("get_new_document_share_key").then(res => { - let key = res.message; - share_document_url_dialog.set_value("link", this.get_share_link(key)); - }); - } - }, { - fieldname: "link", - label: "Link", - fieldtype: "Data", - depends_on: "eval: doc.link", - with_copy_button: true - }], - size: "small", - title: __("Share {} Link", [this.doctype]), - }); - share_document_url_dialog.show(); - } - get_share_link(key) { - return `${window.origin}/${this.doctype}/${this.docname}?key=${key}`; + return frappe.urllib.get_full_url(`${this.doctype}/${this.docname}?key=${key}`); } copy_doc(onload, from_amend) { diff --git a/frappe/public/js/frappe/form/toolbar.js b/frappe/public/js/frappe/form/toolbar.js index e33c401cde..016390a4e1 100644 --- a/frappe/public/js/frappe/form/toolbar.js +++ b/frappe/public/js/frappe/form/toolbar.js @@ -266,15 +266,6 @@ frappe.ui.form.Toolbar = class Toolbar { }); } - // share print view link - if (frappe.model.can_email(null, me.frm) - && frappe.model.can_share(null, me.frm) - && me.frm.doc.docstatus < 2) { - this.page.add_menu_item(__("Share Link", [me.frm.doctype]), function() { - me.frm.share_doc_link(); - }, true); - } - // go to field modal this.page.add_menu_item(__("Jump to field"), function() { me.show_jump_to_field_dialog(); From 8dfb1d27f8766160ee259ae44617ad06e1ab10c6 Mon Sep 17 00:00:00 2001 From: Suraj Shetty Date: Mon, 14 Mar 2022 10:10:57 +0530 Subject: [PATCH 024/340] fix: Get shareable link style --- frappe/public/js/frappe/form/sidebar/share.js | 8 ++++++-- frappe/public/scss/desk/sidebar.scss | 7 ++++++- 2 files changed, 12 insertions(+), 3 deletions(-) diff --git a/frappe/public/js/frappe/form/sidebar/share.js b/frappe/public/js/frappe/form/sidebar/share.js index 114983e1d1..c1f8407c0c 100644 --- a/frappe/public/js/frappe/form/sidebar/share.js +++ b/frappe/public/js/frappe/form/sidebar/share.js @@ -15,6 +15,10 @@ frappe.ui.form.Share = class Share { render_sidebar() { const shared = this.shared || this.frm.get_docinfo().shared; const shared_users = shared.filter(Boolean).map(s => s.user); + + this.share_link.attr("title", __("Get Shareable Link")) + .tooltip({ delay: { "show": 600, "hide": 100 }}); + this.share_link.click(() => { this.share_modal(); }); @@ -208,8 +212,8 @@ frappe.ui.form.Share = class Share { label: __("No Expiry"), fieldname: "no_expiry", change: () => { - const no_expiry_warning = ` - ${__('It is not recommended to send links without expiry')} + const no_expiry_warning = ` + ${__('Note: Person with the link user will be able to see all changes to this document as long as the key is not deleted manually.')} `; const no_expiry = share_modal.get_value('no_expiry'); share_modal.get_field('link_expiration_date').toggle(!no_expiry); diff --git a/frappe/public/scss/desk/sidebar.scss b/frappe/public/scss/desk/sidebar.scss index cca3c2cc50..26c9f9a86d 100644 --- a/frappe/public/scss/desk/sidebar.scss +++ b/frappe/public/scss/desk/sidebar.scss @@ -497,8 +497,13 @@ body[data-route^="Module"] .main-menu { .share-link { margin-left: 7px; cursor: pointer; + padding-top: 2px; + .icon { + width: 18px; + height: 18px; + } .avatar-frame { padding: 2px; - border: 1px solid var(--gray-200); + background: transparent; } } \ No newline at end of file From 92f98b24eb1718f200ea75874b932e8335dbb35c Mon Sep 17 00:00:00 2001 From: Suraj Shetty Date: Mon, 14 Mar 2022 11:11:43 +0530 Subject: [PATCH 025/340] refactor: Set values in a single query --- frappe/patches/v14_0/set_document_expiry_default.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/frappe/patches/v14_0/set_document_expiry_default.py b/frappe/patches/v14_0/set_document_expiry_default.py index dccaa3a6d9..1325ae2466 100644 --- a/frappe/patches/v14_0/set_document_expiry_default.py +++ b/frappe/patches/v14_0/set_document_expiry_default.py @@ -1,5 +1,7 @@ import frappe def execute(): - frappe.db.set_value("System Settings", "System Settings", "document_share_key_expiry", 30) - frappe.db.set_value("System Settings", "System Settings", "allow_older_web_view_links", 1) + frappe.db.set_value("System Settings", "System Settings", { + "document_share_key_expiry": 30, + "allow_older_web_view_links": 1 + }) From 9b54e3d0c0e8043bb9a6a611bc114bc346d6ae55 Mon Sep 17 00:00:00 2001 From: Gavin D'souza Date: Fri, 14 Jan 2022 14:33:21 +0530 Subject: [PATCH 026/340] fix: Disallow sites to have file access beyond site folder --- frappe/core/doctype/file/file.py | 5 ++++- frappe/utils/file_manager.py | 8 ++++++++ 2 files changed, 12 insertions(+), 1 deletion(-) diff --git a/frappe/core/doctype/file/file.py b/frappe/core/doctype/file/file.py index 50a7b31bca..8a1b2e516f 100755 --- a/frappe/core/doctype/file/file.py +++ b/frappe/core/doctype/file/file.py @@ -29,7 +29,7 @@ from frappe import _, conf, safe_decode from frappe.model.document import Document from frappe.utils import call_hook_method, cint, cstr, encode, get_files_path, get_hook_method, random_string, strip from frappe.utils.image import strip_exif_data, optimize_image -from frappe.utils.file_manager import safe_b64decode +from frappe.utils.file_manager import is_safe_path, safe_b64decode if TYPE_CHECKING: from PIL.ImageFile import ImageFile @@ -412,6 +412,9 @@ class File(Document): elif not self.file_url: frappe.throw(_("There is some problem with the file url: {0}").format(file_path)) + if not is_safe_path(file_path): + frappe.throw(f"Cannot access file path {file_path}") + return file_path def write_file(self): diff --git a/frappe/utils/file_manager.py b/frappe/utils/file_manager.py index 1e654d7881..15ba5d3d25 100644 --- a/frappe/utils/file_manager.py +++ b/frappe/utils/file_manager.py @@ -397,3 +397,11 @@ def add_attachments(doctype, name, attachments): files.append(f) return files + +def is_safe_path(path): + basedir = frappe.get_site_path() + # ref: https://docs.python.org/3/library/os.path.html#os.path.commonpath + matchpath = os.path.realpath(os.path.abspath(path)) + basedir = os.path.realpath(os.path.abspath(basedir)) + + return basedir == os.path.commonpath((basedir, matchpath)) From 48c448160d725c21ddf253c3c1bb59a979a4fe95 Mon Sep 17 00:00:00 2001 From: Gavin D'souza Date: Mon, 14 Mar 2022 20:24:16 +0530 Subject: [PATCH 027/340] refactor: Restructure File module * Separate out utils, exceptions & core / maintain import paths [except API endpoints] * Add type hints * Simplify logic --- frappe/core/doctype/file/exceptions.py | 7 + frappe/core/doctype/file/file.py | 317 +------------------------ frappe/core/doctype/file/utils.py | 315 ++++++++++++++++++++++++ frappe/utils/install.py | 2 +- 4 files changed, 334 insertions(+), 307 deletions(-) create mode 100644 frappe/core/doctype/file/exceptions.py create mode 100644 frappe/core/doctype/file/utils.py diff --git a/frappe/core/doctype/file/exceptions.py b/frappe/core/doctype/file/exceptions.py new file mode 100644 index 0000000000..53ff4dad63 --- /dev/null +++ b/frappe/core/doctype/file/exceptions.py @@ -0,0 +1,7 @@ +import frappe + +class MaxFileSizeReachedError(frappe.ValidationError): + pass + +class FolderNotEmpty(frappe.ValidationError): + pass diff --git a/frappe/core/doctype/file/file.py b/frappe/core/doctype/file/file.py index 8a1b2e516f..2a3824d41e 100755 --- a/frappe/core/doctype/file/file.py +++ b/frappe/core/doctype/file/file.py @@ -1,14 +1,6 @@ # Copyright (c) 2022, Frappe Technologies Pvt. Ltd. and Contributors # License: MIT. See LICENSE -""" -record of files - -naming for same name files: file.gif, file-1.gif, file-2.gif etc -""" - -import hashlib -import imghdr import io import json import mimetypes @@ -16,32 +8,22 @@ import os import re import shutil import zipfile -from typing import TYPE_CHECKING, Tuple -import requests from requests.exceptions import HTTPError, SSLError from PIL import Image, ImageFile, ImageOps -from io import BytesIO from urllib.parse import quote, unquote import frappe -from frappe import _, conf, safe_decode +from frappe import _ from frappe.model.document import Document -from frappe.utils import call_hook_method, cint, cstr, encode, get_files_path, get_hook_method, random_string, strip +from frappe.utils import call_hook_method, cint, cstr, encode, get_files_path, get_hook_method from frappe.utils.image import strip_exif_data, optimize_image from frappe.utils.file_manager import is_safe_path, safe_b64decode -if TYPE_CHECKING: - from PIL.ImageFile import ImageFile - from requests.models import Response +from .exceptions import MaxFileSizeReachedError, FolderNotEmpty +from .utils import * -class MaxFileSizeReachedError(frappe.ValidationError): - pass - -class FolderNotEmpty(frappe.ValidationError): - pass - exclude_from_linked_with = True ImageFile.LOAD_TRUNCATED_IMAGES = True @@ -513,7 +495,6 @@ class File(Document): return file_size - def delete_file_data_content(self, only_thumbnail=False): method = get_hook_method('delete_file_data_content') if method: @@ -521,7 +502,6 @@ class File(Document): else: self.delete_file_from_filesystem(only_thumbnail=only_thumbnail) - def delete_file_from_filesystem(self, only_thumbnail=False): """Delete file, thumbnail from File document""" if only_thumbnail: @@ -597,22 +577,6 @@ class File(Document): def on_doctype_update(): frappe.db.add_index("File", ["attached_to_doctype", "attached_to_name"]) -def make_home_folder(): - home = frappe.get_doc({ - "doctype": "File", - "is_folder": 1, - "is_home_folder": 1, - "file_name": _("Home") - }).insert() - - frappe.get_doc({ - "doctype": "File", - "folder": home.name, - "is_folder": 1, - "is_attachments_folder": 1, - "file_name": _("Attachments") - }).insert() - @frappe.whitelist() def create_new_folder(file_name, folder): @@ -624,6 +588,7 @@ def create_new_folder(file_name, folder): file.insert(ignore_if_duplicate=True) return file + @frappe.whitelist() def move_file(file_list, new_parent, old_parent): @@ -641,130 +606,14 @@ def move_file(file_list, new_parent, old_parent): @frappe.whitelist() def zip_files(files): files = frappe.parse_json(files) - zipped_files = File.zip_files(files) frappe.response["filename"] = "files.zip" - frappe.response["filecontent"] = zipped_files + frappe.response["filecontent"] = File.zip_files(files) frappe.response["type"] = "download" -def setup_folder_path(filename, new_parent): - file = frappe.get_doc("File", filename) - file.folder = new_parent - file.save() - - if file.is_folder: - from frappe.model.rename_doc import rename_doc - rename_doc("File", file.name, file.get_name_based_on_parent_folder(), ignore_permissions=True) - -def get_extension(filename, extn, content: bytes = None, response: "Response" = None) -> str: - mimetype = None - - if response: - content_type = response.headers.get("Content-Type") - - if content_type: - _extn = mimetypes.guess_extension(content_type) - if _extn: - return _extn[1:] - - if extn: - # remove '?' char and parameters from extn if present - if '?' in extn: - extn = extn.split('?', 1)[0] - - mimetype = mimetypes.guess_type(filename + "." + extn)[0] - - if mimetype is None or not mimetype.startswith("image/") and content: - # detect file extension by reading image header properties - extn = imghdr.what(filename + "." + (extn or ""), h=content) - - return extn - -def get_local_image(file_url): - if file_url.startswith("/private"): - file_url_path = (file_url.lstrip("/"), ) - else: - file_url_path = ("public", file_url.lstrip("/")) - - file_path = frappe.get_site_path(*file_url_path) - - try: - image = Image.open(file_path) - except IOError: - frappe.throw(_("Unable to read file format for {0}").format(file_url)) - - content = None - - try: - filename, extn = file_url.rsplit(".", 1) - except ValueError: - # no extn - with open(file_path, "r") as f: - content = f.read() - - filename = file_url - extn = None - - extn = get_extension(filename, extn, content) - - return image, filename, extn - -def get_web_image(file_url: str) -> Tuple["ImageFile", str, str]: - # download - file_url = frappe.utils.get_url(file_url) - r = requests.get(file_url, stream=True) - try: - r.raise_for_status() - except HTTPError: - if r.status_code == 404: - frappe.msgprint(_("File '{0}' not found").format(file_url)) - else: - frappe.msgprint(_("Unable to read file format for {0}").format(file_url)) - raise - - try: - image = Image.open(BytesIO(r.content)) - except Exception as e: - frappe.msgprint(_("Image link '{0}' is not valid").format(file_url), raise_exception=e) - - try: - filename, extn = file_url.rsplit("/", 1)[1].rsplit(".", 1) - except ValueError: - # the case when the file url doesn't have filename or extension - # but is fetched due to a query string. example: https://encrypted-tbn3.gstatic.com/images?q=something - filename = get_random_filename() - extn = None - - extn = get_extension(filename, extn, response=r) - if extn == "bin": - extn = get_extension(filename, extn, content=r.content) or "png" - - filename = "/files/" + strip(unquote(filename)) - - return image, filename, extn - - -def delete_file(path): - """Delete file from `public folder`""" - if path: - if ".." in path.split("/"): - frappe.throw(_("It is risky to delete this file: {0}. Please contact your System Manager.").format(path)) - - parts = os.path.split(path.strip("/")) - if parts[0]=="files": - path = frappe.utils.get_site_path("public", "files", parts[-1]) - - else: - path = frappe.utils.get_site_path("private", "files", parts[-1]) - - path = encode(path) - if os.path.exists(path): - os.remove(path) - - @frappe.whitelist() def get_max_file_size(): - return cint(conf.get('max_file_size')) or 10485760 + return cint(frappe.conf.get('max_file_size')) or 10485760 def has_permission(doc, ptype=None, user=None): @@ -801,38 +650,6 @@ def has_permission(doc, ptype=None, user=None): return has_access -def remove_file_by_url(file_url, doctype=None, name=None): - if doctype and name: - fid = frappe.db.get_value("File", { - "file_url": file_url, - "attached_to_doctype": doctype, - "attached_to_name": name}) - else: - fid = frappe.db.get_value("File", {"file_url": file_url}) - - if fid: - from frappe.utils.file_manager import remove_file - return remove_file(fid=fid) - - -def get_content_hash(content): - if isinstance(content, str): - content = content.encode() - return hashlib.md5(content).hexdigest() #nosec - - -def get_file_name(fname, optional_suffix): - # convert to unicode - fname = cstr(fname) - - f = fname.rsplit('.', 1) - if len(f) == 1: - partial, extn = f[0], "" - else: - partial, extn = f[0], "." + f[1] - return '{partial}{suffix}{extn}'.format(partial=partial, extn=extn, suffix=optional_suffix) - - @frappe.whitelist() def download_file(file_url): """ @@ -850,77 +667,12 @@ def download_file(file_url): frappe.local.response.filecontent = file_doc.get_content() frappe.local.response.type = "download" -def extract_images_from_doc(doc, fieldname): - content = doc.get(fieldname) - content = extract_images_from_html(doc, content) - if frappe.flags.has_dataurl: - doc.set(fieldname, content) - - -def extract_images_from_html(doc, content, is_private=False): - frappe.flags.has_dataurl = False - - def _save_file(match): - data = match.group(1) - data = data.split("data:")[1] - headers, content = data.split(",") - mtype = headers.split(";")[0] - - if isinstance(content, str): - content = content.encode("utf-8") - if b"," in content: - content = content.split(b",")[1] - content = safe_b64decode(content) - - content = optimize_image(content, mtype) - - if "filename=" in headers: - filename = headers.split("filename=")[-1] - filename = safe_decode(filename).split(";")[0] - - else: - filename = get_random_filename(content_type=mtype) - - # attaching a file to a child table doc, attaches it to the parent doc - doctype = doc.parenttype if doc.get("parent") else doc.doctype - name = doc.get("parent") or doc.name - - _file = frappe.get_doc({ - "doctype": "File", - "file_name": filename, - "attached_to_doctype": doctype, - "attached_to_name": name, - "content": content, - "decode": False, - "is_private": is_private - }) - _file.save(ignore_permissions=True) - file_url = _file.file_url - if not frappe.flags.has_dataurl: - frappe.flags.has_dataurl = True - - return ']*src\s*=\s*["\'](?=data:)(.*?)["\']', _save_file, content) - - return content - - -def get_random_filename(content_type=None): - extn = None - if content_type: - extn = mimetypes.guess_extension(content_type) - - return random_string(7) + (extn or "") - @frappe.whitelist() def unzip_file(name): '''Unzip the given file and make file records for each of the extracted files''' - file_obj = frappe.get_doc('File', name) - files = file_obj.unzip() - return files + file_obj: File = frappe.get_doc('File', name) + return file_obj.unzip() @frappe.whitelist() @@ -971,64 +723,17 @@ def get_files_in_folder(folder, start=0, page_length=20): 'has_more': len(files) > page_length } + @frappe.whitelist() def get_files_by_search_text(text): if not text: return [] text = '%' + cstr(text).lower() + '%' - return frappe.db.get_all('File', + return frappe.get_all('File', fields=['name', 'file_name', 'file_url', 'is_folder', 'modified'], filters={'is_folder': False}, or_filters={'file_name': ('like', text), 'file_url': text, 'name': ('like', text)}, order_by='modified desc', limit=20 ) - -def update_existing_file_docs(doc): - # Update is private and file url of all file docs that point to the same file - file_doctype = frappe.qb.DocType("File") - ( - frappe.qb.update(file_doctype) - .set(file_doctype.file_url, doc.file_url) - .set(file_doctype.is_private, doc.is_private) - .where(file_doctype.content_hash == doc.content_hash) - .where(file_doctype.name != doc.name) - ).run() - -def attach_files_to_document(doc, event): - """ Runs on on_update hook of all documents. - Goes through every Attach and Attach Image field and attaches - the file url to the document if it is not already attached. - """ - - attach_fields = doc.meta.get( - "fields", {"fieldtype": ["in", ["Attach", "Attach Image"]]} - ) - - for df in attach_fields: - # this method runs in on_update hook of all documents - # we dont want the update to fail if file cannot be attached for some reason - try: - value = doc.get(df.fieldname) - if not (value or '').startswith(("/files", "/private/files")): - return - - if frappe.db.exists("File", { - "file_url": value, - "attached_to_name": doc.name, - "attached_to_doctype": doc.doctype, - "attached_to_field": df.fieldname, - }): - return - - frappe.get_doc( - doctype="File", - file_url=value, - attached_to_name=doc.name, - attached_to_doctype=doc.doctype, - attached_to_field=df.fieldname, - folder="Home/Attachments", - ).insert() - except Exception: - frappe.log_error(title=_("Error Attaching File")) diff --git a/frappe/core/doctype/file/utils.py b/frappe/core/doctype/file/utils.py new file mode 100644 index 0000000000..d9c267d466 --- /dev/null +++ b/frappe/core/doctype/file/utils.py @@ -0,0 +1,315 @@ +import hashlib +import imghdr +import mimetypes +import os +import re +from io import BytesIO +from typing import Optional, Union, Tuple, TYPE_CHECKING +from urllib.parse import unquote + +import requests +import requests.exceptions +from PIL import Image + +import frappe +from frappe import _, safe_decode +from frappe.utils import cstr, encode, random_string, strip, get_files_path +from frappe.utils.file_manager import safe_b64decode +from frappe.utils.image import optimize_image + +if TYPE_CHECKING: + from .file import File + from PIL.ImageFile import ImageFile + from frappe.model.document import Document + from requests.models import Response + + +def make_home_folder() -> None: + home = frappe.get_doc({ + "doctype": "File", + "is_folder": 1, + "is_home_folder": 1, + "file_name": _("Home") + }).insert() + + frappe.get_doc({ + "doctype": "File", + "folder": home.name, + "is_folder": 1, + "is_attachments_folder": 1, + "file_name": _("Attachments") + }).insert() + + +def setup_folder_path(filename: str, new_parent: str) -> None: + file: "File" = frappe.get_doc("File", filename) + file.folder = new_parent + file.save() + + if file.is_folder: + from frappe.model.rename_doc import rename_doc + rename_doc("File", file.name, file.get_name_based_on_parent_folder(), ignore_permissions=True) + + +def get_extension(filename, extn: Optional[str] = None, content: Optional[bytes] = None, response: Optional["Response"] = None) -> str: + mimetype = None + + if response: + content_type = response.headers.get("Content-Type") + + if content_type: + _extn = mimetypes.guess_extension(content_type) + if _extn: + return _extn[1:] + + if extn: + # remove '?' char and parameters from extn if present + if '?' in extn: + extn = extn.split('?', 1)[0] + + mimetype = mimetypes.guess_type(filename + "." + extn)[0] + + if mimetype is None or not mimetype.startswith("image/") and content: + # detect file extension by reading image header properties + extn = imghdr.what(filename + "." + (extn or ""), h=content) + + return extn + + +def get_local_image(file_url: str) -> Tuple["ImageFile", str, str]: + if file_url.startswith("/private"): + file_url_path = (file_url.lstrip("/"), ) + else: + file_url_path = ("public", file_url.lstrip("/")) + + file_path = frappe.get_site_path(*file_url_path) + + try: + image = Image.open(file_path) + except IOError: + frappe.throw(_("Unable to read file format for {0}").format(file_url)) + + content = None + + try: + filename, extn = file_url.rsplit(".", 1) + except ValueError: + # no extn + with open(file_path, "r") as f: + content = f.read() + + filename = file_url + extn = None + + extn = get_extension(filename, extn, content) + + return image, filename, extn + + +def get_web_image(file_url: str) -> Tuple["ImageFile", str, str]: + # download + file_url = frappe.utils.get_url(file_url) + r = requests.get(file_url, stream=True) + try: + r.raise_for_status() + except requests.exceptions.HTTPError as e: + if "404" in e.args[0]: + frappe.msgprint(_("File '{0}' not found").format(file_url)) + else: + frappe.msgprint(_("Unable to read file format for {0}").format(file_url)) + raise + + try: + image = Image.open(BytesIO(r.content)) + except Exception as e: + frappe.msgprint(_("Image link '{0}' is not valid").format(file_url), raise_exception=e) + + try: + filename, extn = file_url.rsplit("/", 1)[1].rsplit(".", 1) + except ValueError: + # the case when the file url doesn't have filename or extension + # but is fetched due to a query string. example: https://encrypted-tbn3.gstatic.com/images?q=something + filename = get_random_filename() + extn = None + + extn = get_extension(filename, extn, r.content) + filename = "/files/" + strip(unquote(filename)) + + return image, filename, extn + + +def delete_file(path: str) -> None: + """Delete file from `public folder`""" + if path: + if ".." in path.split("/"): + frappe.throw(_("It is risky to delete this file: {0}. Please contact your System Manager.").format(path)) + + parts = os.path.split(path.strip("/")) + if parts[0]=="files": + path = frappe.utils.get_site_path("public", "files", parts[-1]) + + else: + path = frappe.utils.get_site_path("private", "files", parts[-1]) + + path = encode(path) + if os.path.exists(path): + os.remove(path) + + +def remove_file_by_url(file_url: str, doctype: str = None, name: str = None) -> "Document": + if doctype and name: + fid = frappe.db.get_value("File", { + "file_url": file_url, + "attached_to_doctype": doctype, + "attached_to_name": name}) + else: + fid = frappe.db.get_value("File", {"file_url": file_url}) + + if fid: + from frappe.utils.file_manager import remove_file + return remove_file(fid=fid) + + +def get_content_hash(content: Union[bytes, str]) -> str: + if isinstance(content, str): + content = content.encode() + return hashlib.md5(content).hexdigest() #nosec + + +def generate_file_name(name: str, suffix: Optional[str] = None, is_private: bool = False) -> str: + """Generate conflict-free file name. Suffix will be ignored if name available. If the + provided suffix doesn't result in an available path, a random suffix will be picked. + """ + def path_exists(name, is_private): + return os.path.exists(encode(get_files_path(name, is_private=is_private))) + + if not path_exists(name, is_private): + return name + + candidate_path = get_file_name(name, suffix) + + if path_exists(candidate_path, is_private): + return generate_file_name(name, is_private=is_private) + return candidate_path + + +def get_file_name(fname: str, optional_suffix: Optional[str] = None) -> str: + # convert to unicode + fname = cstr(fname) + partial, extn = os.path.splitext(fname) + suffix = optional_suffix or frappe.generate_hash(length=6) + + return f"{partial}{suffix}{extn}" + + +def extract_images_from_doc(doc: "Document", fieldname: str): + content = doc.get(fieldname) + content = extract_images_from_html(doc, content) + if frappe.flags.has_dataurl: + doc.set(fieldname, content) + + +def extract_images_from_html(doc: "Document", content: str, is_private: bool = False): + frappe.flags.has_dataurl = False + + def _save_file(match): + data = match.group(1) + data = data.split("data:")[1] + headers, content = data.split(",") + mtype = headers.split(";")[0] + + if isinstance(content, str): + content = content.encode("utf-8") + if b"," in content: + content = content.split(b",")[1] + content = safe_b64decode(content) + + content = optimize_image(content, mtype) + + if "filename=" in headers: + filename = headers.split("filename=")[-1] + filename = safe_decode(filename).split(";")[0] + + else: + filename = get_random_filename(content_type=mtype) + + doctype = doc.parenttype if doc.parent else doc.doctype + name = doc.parent or doc.name + + _file = frappe.get_doc({ + "doctype": "File", + "file_name": filename, + "attached_to_doctype": doctype, + "attached_to_name": name, + "content": content, + "decode": False, + "is_private": is_private + }) + _file.save(ignore_permissions=True) + file_url = _file.file_url + if not frappe.flags.has_dataurl: + frappe.flags.has_dataurl = True + + return ']*src\s*=\s*["\'](?=data:)(.*?)["\']', _save_file, content) + + return content + + +def get_random_filename(content_type: str = None) -> str: + extn = None + if content_type: + extn = mimetypes.guess_extension(content_type) + + return random_string(7) + (extn or "") + + +def update_existing_file_docs(doc: "File") -> None: + # Update is private and file url of all file docs that point to the same file + file_doctype = frappe.qb.DocType("File") + ( + frappe.qb.update(file_doctype) + .set(file_doctype.file_url, doc.file_url) + .set(file_doctype.is_private, doc.is_private) + .where(file_doctype.content_hash == doc.content_hash) + .where(file_doctype.name != doc.name) + ).run() + +def attach_files_to_document(doc: "File", event) -> None: + """Runs on on_update hook of all documents. + Goes through every Attach and Attach Image field and attaches + the file url to the document if it is not already attached. + """ + + attach_fields = doc.meta.get( + "fields", {"fieldtype": ["in", ["Attach", "Attach Image"]]} + ) + + for df in attach_fields: + # this method runs in on_update hook of all documents + # we dont want the update to fail if file cannot be attached for some reason + try: + value = doc.get(df.fieldname) + if not (value or '').startswith(("/files", "/private/files")): + return + + if frappe.db.exists("File", { + "file_url": value, + "attached_to_name": doc.name, + "attached_to_doctype": doc.doctype, + "attached_to_field": df.fieldname, + }): + return + + frappe.get_doc( + doctype="File", + file_url=value, + attached_to_name=doc.name, + attached_to_doctype=doc.doctype, + attached_to_field=df.fieldname, + folder="Home/Attachments", + ).insert() + except Exception: + frappe.log_error(title=_("Error Attaching File")) diff --git a/frappe/utils/install.py b/frappe/utils/install.py index a5fd39994f..9704aae367 100644 --- a/frappe/utils/install.py +++ b/frappe/utils/install.py @@ -21,7 +21,7 @@ def after_install(): create_user_type() install_basic_docs() - from frappe.core.doctype.file.file import make_home_folder + from frappe.core.doctype.file.utils import make_home_folder make_home_folder() import_country_and_currency() From af673f0082b2d0a3e1eef5a00a160c6a99bd86cd Mon Sep 17 00:00:00 2001 From: Gavin D'souza Date: Mon, 14 Mar 2022 20:26:53 +0530 Subject: [PATCH 028/340] fix: Generate non-conflicting names in File.save_file Other fix: * Don't set File.file_name (too) different from File.file_url --- frappe/core/doctype/file/file.py | 26 +++++++++++++------------- 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/frappe/core/doctype/file/file.py b/frappe/core/doctype/file/file.py index 2a3824d41e..8e5a30ab5e 100755 --- a/frappe/core/doctype/file/file.py +++ b/frappe/core/doctype/file/file.py @@ -419,8 +419,14 @@ class File(Document): def save_file(self, content=None, decode=False, ignore_existing_file_check=False): file_exists = False - self.content = content + duplicate_file = None + self.content = content + self.is_private = cint(self.is_private) + self.content_type = mimetypes.guess_type(self.file_name)[0] + self.file_size = self.check_max_file_size() + + # decode self.content if decode: if isinstance(content, str): self.content = content.encode("utf-8") @@ -429,13 +435,7 @@ class File(Document): self.content = self.content.split(b",")[1] self.content = safe_b64decode(self.content) - if not self.is_private: - self.is_private = 0 - - self.content_type = mimetypes.guess_type(self.file_name)[0] - - self.file_size = self.check_max_file_size() - + # transform file content based on site settings if ( self.content_type and self.content_type == "image/jpeg" and frappe.get_system_settings("strip_exif_metadata_from_uploaded_images") @@ -444,8 +444,6 @@ class File(Document): self.content_hash = get_content_hash(self.content) - duplicate_file = None - # check if a file exists with the same content hash and is also in the same folder (public or private) if not ignore_existing_file_check: duplicate_file = frappe.get_value("File", { @@ -460,10 +458,12 @@ class File(Document): self.file_url = duplicate_file.file_url file_exists = True - if os.path.exists(encode(get_files_path(self.file_name, is_private=self.is_private))): - self.file_name = get_file_name(self.file_name, self.content_hash[-6:]) - if not file_exists: + self.file_name = generate_file_name( + name=self.file_name, + suffix=self.content_hash[-6:], + is_private=self.is_private + ) call_hook_method("before_write_file", file_size=self.file_size) write_file_method = get_hook_method('write_file') if write_file_method: From 6ae3b6542597d230ff8027af54721c3b1d8acb4c Mon Sep 17 00:00:00 2001 From: Gavin D'souza Date: Mon, 14 Mar 2022 20:30:07 +0530 Subject: [PATCH 029/340] chore: Use standard APIs --- frappe/core/doctype/file/file.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/frappe/core/doctype/file/file.py b/frappe/core/doctype/file/file.py index 8e5a30ab5e..1defb23d42 100755 --- a/frappe/core/doctype/file/file.py +++ b/frappe/core/doctype/file/file.py @@ -43,7 +43,7 @@ class File(Document): def get_name_based_on_parent_folder(self): if self.folder: - return "/".join([self.folder, self.file_name]) + return os.path.join(self.folder, self.file_name) def autoname(self): """Set name for folder""" @@ -54,7 +54,7 @@ class File(Document): # home self.name = self.file_name else: - self.name = frappe.generate_hash("", 10) + self.name = frappe.generate_hash(length=10) def after_insert(self): if not self.is_folder: From aaf50d28ee762b08a6b1c6aad56bb7a630a1718c Mon Sep 17 00:00:00 2001 From: Gavin D'souza Date: Mon, 14 Mar 2022 20:30:55 +0530 Subject: [PATCH 030/340] fix!: Raise 403 if Guest isn't allowed to upload file --- frappe/handler.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frappe/handler.py b/frappe/handler.py index 3fd1c096e4..f6e12a3027 100755 --- a/frappe/handler.py +++ b/frappe/handler.py @@ -147,7 +147,7 @@ def upload_file(): if frappe.get_system_settings('allow_guests_to_upload_files'): ignore_permissions = True else: - return + raise frappe.PermissionError else: user = frappe.get_doc("User", frappe.session.user) ignore_permissions = False From 59e45a2e2f411fc85f06c5faeef61de58febf95f Mon Sep 17 00:00:00 2001 From: Gavin D'souza Date: Tue, 15 Mar 2022 15:04:01 +0530 Subject: [PATCH 031/340] refactor: File APIs Restructured and moved most APIs under frappe.core.api.file namespace. Changed some obvious security gaps (like using get_list instead of get_all for an endpoint), styled, added type hints and made minor performance enhancements. Changes * download_file API * Move API to handler.py * Check for permissions via File.is_downloadable instead * Moved APIs to new namespace: `frappe.core.api.file` * Backwards compatibility * Added APIs to override_whitelisted_methods to maintain existing client endpoints * Imported APIs to controller's namespace to avoid breaking external app usages --- frappe/core/api/__init__.py | 0 frappe/core/api/file.py | 123 ++++++++++++++++++++++++++++ frappe/core/doctype/file/file.py | 134 +------------------------------ frappe/handler.py | 40 +++++++-- frappe/hooks.py | 12 +++ 5 files changed, 172 insertions(+), 137 deletions(-) create mode 100644 frappe/core/api/__init__.py create mode 100644 frappe/core/api/file.py diff --git a/frappe/core/api/__init__.py b/frappe/core/api/__init__.py new file mode 100644 index 0000000000..e69de29bb2 diff --git a/frappe/core/api/file.py b/frappe/core/api/file.py new file mode 100644 index 0000000000..38a044787d --- /dev/null +++ b/frappe/core/api/file.py @@ -0,0 +1,123 @@ +import json +from typing import Dict, List + +import frappe +from frappe.utils import cint, cstr + +from frappe.core.doctype.file.file import File, setup_folder_path + + +@frappe.whitelist() +def unzip_file(name: str): + """Unzip the given file and make file records for each of the extracted files""" + file: File = frappe.get_doc("File", name) + return file.unzip() + + +@frappe.whitelist() +def get_attached_images(doctype: str, names: List[str]) -> frappe._dict: + """get list of image urls attached in form + returns {name: ['image.jpg', 'image.png']}""" + + if isinstance(names, str): + names = json.loads(names) + + img_urls = frappe.db.get_list( + "File", + filters={ + "attached_to_doctype": doctype, + "attached_to_name": ("in", names), + "is_folder": 0, + }, + fields=["file_url", "attached_to_name as docname"], + ) + + out = frappe._dict() + for i in img_urls: + out[i.docname] = out.get(i.docname, []) + out[i.docname].append(i.file_url) + + return out + + +@frappe.whitelist() +def get_files_in_folder(folder: str, start: int = 0, page_length: int = 20) -> Dict: + start = cint(start) + page_length = cint(page_length) + + attachment_folder = frappe.db.get_value( + "File", + "Home/Attachments", + ["name", "file_name", "file_url", "is_folder", "modified"], + as_dict=1, + ) + + files = frappe.get_list( + "File", + {"folder": folder}, + ["name", "file_name", "file_url", "is_folder", "modified"], + start=start, + page_length=page_length + 1, + ) + + if folder == "Home" and attachment_folder not in files: + files.insert(0, attachment_folder) + + return {"files": files[:page_length], "has_more": len(files) > page_length} + + +@frappe.whitelist() +def get_files_by_search_text(text: str) -> List[Dict]: + if not text: + return [] + + text = "%" + cstr(text).lower() + "%" + return frappe.get_list( + "File", + fields=["name", "file_name", "file_url", "is_folder", "modified"], + filters={"is_folder": False}, + or_filters={ + "file_name": ("like", text), + "file_url": text, + "name": ("like", text), + }, + order_by="modified desc", + limit=20, + ) + + +@frappe.whitelist(allow_guest=True) +def get_max_file_size() -> int: + return cint(frappe.conf.get("max_file_size")) or 10485760 + + +@frappe.whitelist() +def create_new_folder(file_name: str, folder: str) -> File: + """create new folder under current parent folder""" + file = frappe.new_doc("File") + file.file_name = file_name + file.is_folder = 1 + file.folder = folder + file.insert(ignore_if_duplicate=True) + return file + + +@frappe.whitelist() +def move_file(file_list: List[File], new_parent: str, old_parent: str) -> None: + if isinstance(file_list, str): + file_list = json.loads(file_list) + + for file_obj in file_list: + setup_folder_path(file_obj.get("name"), new_parent) + + # recalculate sizes + frappe.get_doc("File", old_parent).save() + frappe.get_doc("File", new_parent).save() + + +@frappe.whitelist() +def zip_files(files: str): + files = frappe.parse_json(files) + frappe.response["filename"] = "files.zip" + frappe.response["filecontent"] = File.zip_files(files) + frappe.response["type"] = "download" diff --git a/frappe/core/doctype/file/file.py b/frappe/core/doctype/file/file.py index 1defb23d42..c34bdfc9bc 100755 --- a/frappe/core/doctype/file/file.py +++ b/frappe/core/doctype/file/file.py @@ -2,11 +2,11 @@ # License: MIT. See LICENSE import io -import json import mimetypes import os import re import shutil +from typing import List import zipfile from requests.exceptions import HTTPError, SSLError @@ -16,9 +16,10 @@ from urllib.parse import quote, unquote import frappe from frappe import _ from frappe.model.document import Document -from frappe.utils import call_hook_method, cint, cstr, encode, get_files_path, get_hook_method +from frappe.utils import call_hook_method, cint, encode, get_files_path, get_hook_method from frappe.utils.image import strip_exif_data, optimize_image from frappe.utils.file_manager import is_safe_path, safe_b64decode +from frappe.core.api.file import * from .exceptions import MaxFileSizeReachedError, FolderNotEmpty from .utils import * @@ -313,7 +314,7 @@ class File(Document): self.flags.on_rollback = True self.on_trash() - def unzip(self): + def unzip(self) -> List[File]: '''Unzip current file and replace it by its children''' if not self.file_url.endswith(".zip"): frappe.throw(_("{0} is not a zip file").format(self.file_name)) @@ -578,44 +579,6 @@ def on_doctype_update(): frappe.db.add_index("File", ["attached_to_doctype", "attached_to_name"]) -@frappe.whitelist() -def create_new_folder(file_name, folder): - """ create new folder under current parent folder """ - file = frappe.new_doc("File") - file.file_name = file_name - file.is_folder = 1 - file.folder = folder - file.insert(ignore_if_duplicate=True) - return file - - -@frappe.whitelist() -def move_file(file_list, new_parent, old_parent): - - if isinstance(file_list, str): - file_list = json.loads(file_list) - - for file_obj in file_list: - setup_folder_path(file_obj.get("name"), new_parent) - - # recalculate sizes - frappe.get_doc("File", old_parent).save() - frappe.get_doc("File", new_parent).save() - - -@frappe.whitelist() -def zip_files(files): - files = frappe.parse_json(files) - frappe.response["filename"] = "files.zip" - frappe.response["filecontent"] = File.zip_files(files) - frappe.response["type"] = "download" - - -@frappe.whitelist() -def get_max_file_size(): - return cint(frappe.conf.get('max_file_size')) or 10485760 - - def has_permission(doc, ptype=None, user=None): has_access = False user = user or frappe.session.user @@ -648,92 +611,3 @@ def has_permission(doc, ptype=None, user=None): pass return has_access - - -@frappe.whitelist() -def download_file(file_url): - """ - Download file using token and REST API. Valid session or - token is required to download private files. - - Method : GET - Endpoint : frappe.core.doctype.file.file.download_file - URL Params : file_name = /path/to/file relative to site path - """ - file_doc = frappe.get_doc("File", {"file_url": file_url}) - file_doc.check_permission("read") - - frappe.local.response.filename = os.path.basename(file_url) - frappe.local.response.filecontent = file_doc.get_content() - frappe.local.response.type = "download" - - -@frappe.whitelist() -def unzip_file(name): - '''Unzip the given file and make file records for each of the extracted files''' - file_obj: File = frappe.get_doc('File', name) - return file_obj.unzip() - - -@frappe.whitelist() -def get_attached_images(doctype, names): - '''get list of image urls attached in form - returns {name: ['image.jpg', 'image.png']}''' - - if isinstance(names, str): - names = json.loads(names) - - img_urls = frappe.db.get_list('File', filters={ - 'attached_to_doctype': doctype, - 'attached_to_name': ('in', names), - 'is_folder': 0 - }, fields=['file_url', 'attached_to_name as docname']) - - out = frappe._dict() - for i in img_urls: - out[i.docname] = out.get(i.docname, []) - out[i.docname].append(i.file_url) - - return out - - -@frappe.whitelist() -def get_files_in_folder(folder, start=0, page_length=20): - start = cint(start) - page_length = cint(page_length) - - attachment_folder = frappe.db.get_value('File', - 'Home/Attachments', - ['name', 'file_name', 'file_url', 'is_folder', 'modified'], - as_dict=1 - ) - - files = frappe.db.get_list('File', - { 'folder': folder }, - ['name', 'file_name', 'file_url', 'is_folder', 'modified'], - start=start, - page_length=page_length + 1 - ) - - if folder == 'Home' and attachment_folder not in files: - files.insert(0, attachment_folder) - - return { - 'files': files[:page_length], - 'has_more': len(files) > page_length - } - - -@frappe.whitelist() -def get_files_by_search_text(text): - if not text: - return [] - - text = '%' + cstr(text).lower() + '%' - return frappe.get_all('File', - fields=['name', 'file_name', 'file_url', 'is_folder', 'modified'], - filters={'is_folder': False}, - or_filters={'file_name': ('like', text), 'file_url': text, 'name': ('like', text)}, - order_by='modified desc', - limit=20 - ) diff --git a/frappe/handler.py b/frappe/handler.py index f6e12a3027..bba2f3c057 100755 --- a/frappe/handler.py +++ b/frappe/handler.py @@ -1,18 +1,24 @@ -# Copyright (c) 2015, Frappe Technologies Pvt. Ltd. and Contributors +# Copyright (c) 2022, Frappe Technologies Pvt. Ltd. and Contributors # License: MIT. See LICENSE +import os +from mimetypes import guess_type +from typing import TYPE_CHECKING + from werkzeug.wrappers import Response import frappe -import frappe.utils import frappe.sessions -from frappe.utils import cint +import frappe.utils from frappe import _, is_whitelisted -from frappe.utils.response import build_response +from frappe.core.doctype.server_script.server_script_utils import get_server_script_map +from frappe.utils import cint from frappe.utils.csvutils import build_csv_response from frappe.utils.image import optimize_image -from mimetypes import guess_type -from frappe.core.doctype.server_script.server_script_utils import get_server_script_map +from frappe.utils.response import build_response + +if TYPE_CHECKING: + from frappe.core.doctype.file.file import File ALLOWED_MIMETYPES = ('image/png', 'image/jpeg', 'application/pdf', 'application/msword', @@ -209,6 +215,25 @@ def upload_file(): return ret +@frappe.whitelist(allow_guest=True) +def download_file(file_url: str): + """ + Download file using token and REST API. Valid session or + token is required to download private files. + + Method : GET + Endpoints : download_file, frappe.core.doctype.file.file.download_file + URL Params : file_name = /path/to/file relative to site path + """ + file: "File" = frappe.get_doc("File", {"file_url": file_url}) + if not file.is_downloadable(): + raise frappe.PermissionError + + frappe.local.response.filename = os.path.basename(file_url) + frappe.local.response.filecontent = file.get_content() + frappe.local.response.type = "download" + + def get_attr(cmd): """get method object from cmd""" if '.' in cmd: @@ -218,6 +243,7 @@ def get_attr(cmd): frappe.log("method:" + cmd) return method + @frappe.whitelist(allow_guest=True) def ping(): return "pong" @@ -225,8 +251,8 @@ def ping(): def run_doc_method(method, docs=None, dt=None, dn=None, arg=None, args=None): """run a whitelisted controller method""" - import json import inspect + import json if not args: args = arg or "" diff --git a/frappe/hooks.py b/frappe/hooks.py index be1b0134c1..6b8cef31e9 100644 --- a/frappe/hooks.py +++ b/frappe/hooks.py @@ -383,3 +383,15 @@ global_search_doctypes = { {"doctype": "Web Form"} ] } + +override_whitelisted_methods = { + "frappe.core.doctype.file.file.download_file": "download_file", + "frappe.core.doctype.file.file.unzip_file": "frappe.core.api.file.unzip_file", + "frappe.core.doctype.file.file.get_attached_images": "frappe.core.api.file.get_attached_images", + "frappe.core.doctype.file.file.get_files_in_folder": "frappe.core.api.file.get_files_in_folder", + "frappe.core.doctype.file.file.get_files_by_search_text": "frappe.core.api.file.get_files_by_search_text", + "frappe.core.doctype.file.file.get_max_file_size": "frappe.core.api.file.get_max_file_size", + "frappe.core.doctype.file.file.create_new_folder": "frappe.core.api.file.create_new_folder", + "frappe.core.doctype.file.file.move_file": "frappe.core.api.file.move_file", + "frappe.core.doctype.file.file.zip_files": "frappe.core.api.file.zip_files", +} \ No newline at end of file From c6bad81f243ee0ab3d89da805cc31845ae77bfb2 Mon Sep 17 00:00:00 2001 From: Gavin D'souza Date: Tue, 15 Mar 2022 15:31:33 +0530 Subject: [PATCH 032/340] fix: Use new direct import paths The old paths worked too, but it's just better to use the new paths and not go in circles ;) --- frappe/core/doctype/file/__init__.py | 2 ++ frappe/core/doctype/file/file.js | 2 +- frappe/core/doctype/file/file.py | 6 ++++-- frappe/core/doctype/file/test_file.py | 8 ++++---- frappe/desk/form/utils.py | 2 +- frappe/email/receive.py | 3 +-- frappe/model/base_document.py | 2 +- frappe/printing/doctype/print_format/print_format.py | 2 +- frappe/public/js/frappe/file_uploader/FileBrowser.vue | 4 ++-- frappe/public/js/frappe/file_uploader/FileUploader.vue | 2 +- frappe/public/js/frappe/utils/file_manager.js | 2 +- frappe/public/js/frappe/views/file/file_view.js | 6 +++--- frappe/public/js/frappe/views/image/image_view.js | 2 +- frappe/utils/data.py | 2 +- frappe/website/doctype/web_form/web_form.py | 3 ++- 15 files changed, 26 insertions(+), 22 deletions(-) diff --git a/frappe/core/doctype/file/__init__.py b/frappe/core/doctype/file/__init__.py index e69de29bb2..4be524ee92 100644 --- a/frappe/core/doctype/file/__init__.py +++ b/frappe/core/doctype/file/__init__.py @@ -0,0 +1,2 @@ +from .utils import * +from .exceptions import * diff --git a/frappe/core/doctype/file/file.js b/frappe/core/doctype/file/file.js index d40328d3cd..828865a161 100644 --- a/frappe/core/doctype/file/file.js +++ b/frappe/core/doctype/file/file.js @@ -38,7 +38,7 @@ frappe.ui.form.on("File", "refresh", function(frm) { if(frm.doc.file_name && frm.doc.file_name.split('.').splice(-1)[0]==='zip') { frm.add_custom_button(__('Unzip'), function() { frappe.call({ - method: "frappe.core.doctype.file.file.unzip_file", + method: "frappe.core.api.file.unzip_file", args: { name: frm.doc.name, }, diff --git a/frappe/core/doctype/file/file.py b/frappe/core/doctype/file/file.py index c34bdfc9bc..effa475651 100755 --- a/frappe/core/doctype/file/file.py +++ b/frappe/core/doctype/file/file.py @@ -19,7 +19,6 @@ from frappe.model.document import Document from frappe.utils import call_hook_method, cint, encode, get_files_path, get_hook_method from frappe.utils.image import strip_exif_data, optimize_image from frappe.utils.file_manager import is_safe_path, safe_b64decode -from frappe.core.api.file import * from .exceptions import MaxFileSizeReachedError, FolderNotEmpty from .utils import * @@ -314,7 +313,7 @@ class File(Document): self.flags.on_rollback = True self.on_trash() - def unzip(self) -> List[File]: + def unzip(self) -> List["File"]: '''Unzip current file and replace it by its children''' if not self.file_url.endswith(".zip"): frappe.throw(_("{0} is not a zip file").format(self.file_name)) @@ -611,3 +610,6 @@ def has_permission(doc, ptype=None, user=None): pass return has_access + +# Note: kept at the end to not cause circular, partial imports & maintain backwards compatibility +from frappe.core.api.file import * diff --git a/frappe/core/doctype/file/test_file.py b/frappe/core/doctype/file/test_file.py index d8e748a518..a130f2caff 100644 --- a/frappe/core/doctype/file/test_file.py +++ b/frappe/core/doctype/file/test_file.py @@ -2,12 +2,13 @@ # License: MIT. See LICENSE import base64 import json -import frappe import os import unittest +import frappe from frappe import _ -from frappe.core.doctype.file.file import File, get_attached_images, move_file, get_files_in_folder, unzip_file +from frappe.core.api.file import create_new_folder, get_attached_images, get_files_in_folder, move_file, unzip_file +from frappe.core.doctype.file.file import File from frappe.utils import get_files_path test_content1 = 'Hello' @@ -559,7 +560,6 @@ class TestFileUtils(unittest.TestCase): self.assertIn(f' { this.restrictions.max_file_size = Number(res.message); }); diff --git a/frappe/public/js/frappe/utils/file_manager.js b/frappe/public/js/frappe/utils/file_manager.js index f57cb8200a..72dec2c47b 100644 --- a/frappe/public/js/frappe/utils/file_manager.js +++ b/frappe/public/js/frappe/utils/file_manager.js @@ -20,7 +20,7 @@ frappe.file_manager = function() { new_folder = new_folder_; frappe.call({ - method:"frappe.core.doctype.file.file.move_file", + method:"frappe.core.api.file.move_file", args: { file_list: files_to_move, new_parent: new_folder, diff --git a/frappe/public/js/frappe/views/file/file_view.js b/frappe/public/js/frappe/views/file/file_view.js index b351ce6109..5d3927c9c9 100644 --- a/frappe/public/js/frappe/views/file/file_view.js +++ b/frappe/public/js/frappe/views/file/file_view.js @@ -109,7 +109,7 @@ frappe.views.FileView = class FileView extends frappe.views.ListView { }; frappe.call({ method: - "frappe.core.doctype.file.file.create_new_folder", + "frappe.core.api.file.create_new_folder", args: data }); }, @@ -130,7 +130,7 @@ frappe.views.FileView = class FileView extends frappe.views.ListView { frappe.show_alert(__("Unzipping files...")); frappe .call( - "frappe.core.doctype.file.file.unzip_file", + "frappe.core.api.file.unzip_file", { name: file.name } @@ -173,7 +173,7 @@ frappe.views.FileView = class FileView extends frappe.views.ListView { this.page.add_actions_menu_item(__('Export as zip'), () => { let docnames = this.get_checked_items(true); if (docnames.length) { - open_url_post('/api/method/frappe.core.doctype.file.file.zip_files', { + open_url_post('/api/method/frappe.core.api.file.zip_files', { files: JSON.stringify(docnames) }); } diff --git a/frappe/public/js/frappe/views/image/image_view.js b/frappe/public/js/frappe/views/image/image_view.js index c499f122a9..57647642a3 100644 --- a/frappe/public/js/frappe/views/image/image_view.js +++ b/frappe/public/js/frappe/views/image/image_view.js @@ -145,7 +145,7 @@ frappe.views.ImageView = class ImageView extends frappe.views.ListView { get_attached_images() { return frappe .call({ - method: "frappe.core.doctype.file.file.get_attached_images", + method: "frappe.core.api.file.get_attached_images", args: { doctype: this.doctype, names: this.items.map(i => i.name) diff --git a/frappe/utils/data.py b/frappe/utils/data.py index 50c71bdc2e..89eedf7f40 100644 --- a/frappe/utils/data.py +++ b/frappe/utils/data.py @@ -1034,7 +1034,7 @@ def get_thumbnail_base64_for_image(src): from PIL import Image from frappe import cache, safe_decode - from frappe.core.doctype.file.file import get_local_image + from frappe.core.doctype.file.utils import get_local_image if not src: frappe.throw('Invalid source for image: {0}'.format(src)) diff --git a/frappe/website/doctype/web_form/web_form.py b/frappe/website/doctype/web_form/web_form.py index d891ceb205..dfa3b92a02 100644 --- a/frappe/website/doctype/web_form/web_form.py +++ b/frappe/website/doctype/web_form/web_form.py @@ -5,7 +5,8 @@ import json import os import frappe from frappe import _, scrub -from frappe.core.doctype.file.file import get_max_file_size, remove_file_by_url +from frappe.core.doctype.file import remove_file_by_url +from frappe.core.api.file import get_max_file_size from frappe.custom.doctype.customize_form.customize_form import docfield_properties from frappe.desk.form.meta import get_code_files_via_hooks from frappe.integrations.utils import get_payment_gateway_controller From b87f31a94ea1afe3c74db2e17ac3fee0ac0c2948 Mon Sep 17 00:00:00 2001 From: Gavin D'souza Date: Tue, 15 Mar 2022 19:34:26 +0530 Subject: [PATCH 033/340] refactor: File controller * Simplify object lifecycle flows * Re-organize and separate distinct actions / APIs * Style black-ish * Simplify logic in methods "touched" --- frappe/core/doctype/file/file.py | 404 +++++++++++++++++-------------- 1 file changed, 217 insertions(+), 187 deletions(-) diff --git a/frappe/core/doctype/file/file.py b/frappe/core/doctype/file/file.py index effa475651..3f4c2069e1 100755 --- a/frappe/core/doctype/file/file.py +++ b/frappe/core/doctype/file/file.py @@ -6,7 +6,7 @@ import mimetypes import os import re import shutil -from typing import List +from typing import Union, List import zipfile from requests.exceptions import HTTPError, SSLError @@ -31,19 +31,10 @@ ImageFile.LOAD_TRUNCATED_IMAGES = True class File(Document): no_feed_on_delete = True - def before_insert(self): - frappe.local.rollback_observers.append(self) - self.set_folder_name() - if self.file_name: - self.file_name = re.sub(r'/', '', self.file_name) + def __init__(self, *args, **kwargs): + super().__init__(*args, **kwargs) self.content = self.get("content", None) self.decode = self.get("decode", False) - if self.content: - self.save_file(content=self.content, decode=self.decode) - - def get_name_based_on_parent_folder(self): - if self.folder: - return os.path.join(self.folder, self.file_name) def autoname(self): """Set name for folder""" @@ -56,90 +47,109 @@ class File(Document): else: self.name = frappe.generate_hash(length=10) + def before_insert(self): + frappe.local.rollback_observers.append(self) + self.set_folder_name() + self.set_file_name() + self.save_file(content=self.content, decode=self.decode) + def after_insert(self): if not self.is_folder: - self.add_comment_in_reference_doc('Attachment', - _('Added {0}').format("{file_name}{icon}".format(**{ - "icon": ' ' if self.is_private else "", - "file_url": quote(frappe.safe_encode(self.file_url)) if self.file_url else self.file_name, - "file_name": self.file_name or self.file_url - }))) - - def after_rename(self, olddn, newdn, merge=False): - for successor in self.get_successor(): - setup_folder_path(successor[0], self.name) - - def get_successor(self): - return frappe.db.get_values(doctype='File', - filters={'folder': self.name}, - fieldname='name') + self.create_attachment_record() + self.set_is_private() + self.set_file_name() + self.validate_duplicate_entry() + self.validate_attachment_limit() def validate(self): - if self.is_new(): - self.set_is_private() - self.set_file_name() - self.validate_duplicate_entry() - self.validate_attachment_limit() + # Ensure correct formatting and type + self.file_url = unquote(self.file_url) - self.validate_folder() + if not self.is_new() and self.has_value_changed("is_private"): + self.handle_is_private_changed() if self.is_folder: self.file_url = "" else: - self.validate_url() + self.validate_file_on_disk() + self.validate_file_url() + self.validate_file_path() self.file_size = frappe.form_dict.file_size or self.file_size - def validate_url(self): - if not self.file_url or self.file_url.startswith(("http://", "https://")): - if not self.flags.ignore_file_validate: - self.validate_file() + def after_rename(self, *args, **kwargs): + for successor in self.get_successors(): + setup_folder_path(successor, self.name) - return - - # Probably an invalid web URL - if not self.file_url.startswith(("/files/", "/private/files/")): - frappe.throw( - _("URL must start with http:// or https://"), - title=_('Invalid URL') + def on_trash(self): + if self.is_home_folder or self.is_attachments_folder: + frappe.throw(_("Cannot delete Home and Attachments folders")) + self.validate_empty_folder() + self._delete_file_on_disk() + if not self.is_folder: + self.add_comment_in_reference_doc( + "Attachment Removed", _("Removed {0}").format(self.file_name) ) - # Ensure correct formatting and type - self.file_url = unquote(self.file_url) - self.is_private = cint(self.is_private) + def on_rollback(self): + # if original_content flag is set, this rollback should revert the file to its original state + if self.flags.original_content: + file_path = self.get_full_path() + with open(file_path, "wb+") as f: + f.write(self.flags.original_content) - self.handle_is_private_changed() + # following condition is only executed when an insert has been rolledback + else: + self.flags.on_rollback = True + self.on_trash() + + def get_name_based_on_parent_folder(self) -> Union[str, None]: + if self.folder: + return os.path.join(self.folder, self.file_name) + + def get_successors(self): + return frappe.get_all( + "File", filters={"folder": self.name}, pluck="name" + ) + + def validate_file_path(self): + if self.file_url.startswith(("http://", "https://")): + return base_path = os.path.realpath(get_files_path(is_private=self.is_private)) if not os.path.realpath(self.get_full_path()).startswith(base_path): frappe.throw( _("The File URL you've entered is incorrect"), - title=_('Invalid File URL') + title=_("Invalid File URL"), + ) + + def validate_file_url(self): + if self.file_url.startswith(("http://", "https://")): + return + + if not self.file_url: + return + + if not self.file_url.startswith(("/files/", "/private/files/")): + # Probably an invalid URL since it doesn't start with http either + frappe.throw( + _("URL must start with http:// or https://"), title=_("Invalid URL") ) def handle_is_private_changed(self): - if not frappe.db.exists( - 'File', { - 'name': self.name, - 'is_private': cint(not self.is_private) - } - ): - return - old_file_url = self.file_url + file_name = self.file_url.split("/")[-1] + private_file_path = frappe.get_site_path("private", "files", file_name) + public_file_path = frappe.get_site_path("public", "files", file_name) - file_name = self.file_url.split('/')[-1] - private_file_path = frappe.get_site_path('private', 'files', file_name) - public_file_path = frappe.get_site_path('public', 'files', file_name) - - if self.is_private: + if cint(self.is_private): shutil.move(public_file_path, private_file_path) url_starts_with = "/private/files/" else: shutil.move(private_file_path, public_file_path) url_starts_with = "/files/" - self.file_url = "{0}{1}".format(url_starts_with, file_name) + self.file_url = f"{url_starts_with}{file_name}" update_existing_file_docs(self) if ( @@ -149,8 +159,12 @@ class File(Document): ): return - frappe.db.set_value(self.attached_to_doctype, self.attached_to_name, - self.attached_to_field, self.file_url) + frappe.db.set_value( + self.attached_to_doctype, + self.attached_to_name, + self.attached_to_field, + self.file_url, + ) def fetch_attached_to_field(self, old_file_url): if self.attached_to_field: @@ -186,21 +200,21 @@ class File(Document): def set_folder_name(self): """Make parent folders if not exists based on reference doctype and name""" - if self.attached_to_doctype and not self.folder: + if self.folder: + return + + if self.attached_to_doctype: self.folder = frappe.db.get_value("File", {"is_attachments_folder": 1}) - def validate_folder(self): - if not self.is_home_folder and not self.folder and \ - not self.flags.ignore_folder_validate: + if not self.is_home_folder: self.folder = "Home" - def validate_file(self): - """Validates existence of public file - TODO: validate for private file + def validate_file_on_disk(self): + """Validates existence file """ full_path = self.get_full_path() - if full_path.startswith('http'): + if full_path.startswith("http"): return True if not os.path.exists(full_path): @@ -214,33 +228,37 @@ class File(Document): # check duplicate name # check duplicate assignment filters = { - 'content_hash': self.content_hash, - 'is_private': self.is_private, - 'name': ('!=', self.name) + "content_hash": self.content_hash, + "is_private": self.is_private, + "name": ("!=", self.name), } if self.attached_to_doctype and self.attached_to_name: - filters.update({ - 'attached_to_doctype': self.attached_to_doctype, - 'attached_to_name': self.attached_to_name - }) - duplicate_file = frappe.db.get_value('File', filters, ['name', 'file_url'], as_dict=1) + filters.update( + { + "attached_to_doctype": self.attached_to_doctype, + "attached_to_name": self.attached_to_name, + } + ) + duplicate_file = frappe.db.get_value( + "File", filters, ["name", "file_url"], as_dict=1 + ) if duplicate_file: - duplicate_file_doc = frappe.get_cached_doc('File', duplicate_file.name) + duplicate_file_doc = frappe.get_cached_doc("File", duplicate_file.name) if duplicate_file_doc.exists_on_disk(): - # just use the url, to avoid uploading a duplicate - self.file_url = duplicate_file.file_url + # just use the url, to avoid uploading a duplicate + self.file_url = duplicate_file.file_url def set_file_name(self): if not self.file_name and self.file_url: - self.file_name = self.file_url.split('/')[-1] + self.file_name = self.file_url.split("/")[-1] else: - self.file_name = re.sub(r'/', '', self.file_name) + self.file_name = re.sub(r"/", "", self.file_name) def generate_content_hash(self): - if self.content_hash or not self.file_url or self.file_url.startswith('http'): + if self.content_hash or not self.file_url or self.file_url.startswith("http"): return - file_name = self.file_url.split('/')[-1] + file_name = self.file_url.split("/")[-1] try: file_path = get_files_path(file_name, is_private=self.is_private) with open(file_path, "rb") as f: @@ -248,73 +266,68 @@ class File(Document): except IOError: frappe.throw(_("File {0} does not exist").format(file_path)) - def on_trash(self): - if self.is_home_folder or self.is_attachments_folder: - frappe.throw(_("Cannot delete Home and Attachments folders")) - self.check_folder_is_empty() - self.call_delete_file() - if not self.is_folder: - self.add_comment_in_reference_doc('Attachment Removed', _("Removed {0}").format(self.file_name)) + def make_thumbnail( + self, + set_as_thumbnail: bool = True, + width: int = 300, + height: int = 300, + suffix: str = "small", + crop: bool = False, + ) -> str: + if not self.file_url: + return - def make_thumbnail(self, set_as_thumbnail=True, width=300, height=300, suffix="small", crop=False): - if self.file_url: - try: - if self.file_url.startswith(("/files", "/private/files")): - image, filename, extn = get_local_image(self.file_url) - else: - image, filename, extn = get_web_image(self.file_url) - except (HTTPError, SSLError, IOError, TypeError): - return - - size = width, height - if crop: - image = ImageOps.fit(image, size, Image.ANTIALIAS) + try: + if self.file_url.startswith(("/files", "/private/files")): + image, filename, extn = get_local_image(self.file_url) else: - image.thumbnail(size, Image.ANTIALIAS) + image, filename, extn = get_web_image(self.file_url) + except (HTTPError, SSLError, IOError, TypeError): + return - thumbnail_url = filename + "_" + suffix + "." + extn - path = os.path.abspath(frappe.get_site_path("public", thumbnail_url.lstrip("/"))) + size = width, height + if crop: + image = ImageOps.fit(image, size, Image.ANTIALIAS) + else: + image.thumbnail(size, Image.ANTIALIAS) - try: - image.save(path) - if set_as_thumbnail: - self.db_set("thumbnail_url", thumbnail_url) + thumbnail_url = f"{filename}_{suffix}.{extn}" + path = os.path.abspath( + frappe.get_site_path("public", thumbnail_url.lstrip("/")) + ) - except IOError: - frappe.msgprint(_("Unable to write file format for {0}").format(path)) - return + try: + image.save(path) + if set_as_thumbnail: + self.db_set("thumbnail_url", thumbnail_url) - return thumbnail_url + except IOError: + frappe.msgprint(_("Unable to write file format for {0}").format(path)) + return - def check_folder_is_empty(self): + return thumbnail_url + + def validate_empty_folder(self): """Throw exception if folder is not empty""" - files = frappe.get_all("File", filters={"folder": self.name}, fields=("name", "file_name")) - - if self.is_folder and files: + if self.is_folder and frappe.get_all("File", filters={"folder": self.name}, limit=1): frappe.throw(_("Folder {0} is not empty").format(self.name), FolderNotEmpty) - def call_delete_file(self): + def _delete_file_on_disk(self): """If file not attached to any other record, delete it""" - if self.file_name and self.content_hash and (not frappe.db.count("File", - {"content_hash": self.content_hash, "name": ["!=", self.name]})): - self.delete_file_data_content() + on_disk_file_not_shared = ( + self.content_hash + and not frappe.get_all("File", + filters={"content_hash": self.content_hash, "name": ["!=", self.name]}, + limit=1, + ) + ) + if self.file_name and on_disk_file_not_shared: + self.delete_file_data_content() elif self.file_url: self.delete_file_data_content(only_thumbnail=True) - def on_rollback(self): - # if original_content flag is set, this rollback should revert the file to its original state - if self.flags.original_content: - file_path = self.get_full_path() - with open(file_path, "wb+") as f: - f.write(self.flags.original_content) - - # following condition is only executed when an insert has been rolledback - else: - self.flags.on_rollback = True - self.on_trash() - def unzip(self) -> List["File"]: - '''Unzip current file and replace it by its children''' + """Unzip current file and replace it by its children""" if not self.file_url.endswith(".zip"): frappe.throw(_("{0} is not a zip file").format(self.file_name)) @@ -323,16 +336,16 @@ class File(Document): files = [] with zipfile.ZipFile(zip_path) as z: for file in z.filelist: - if file.is_dir() or file.filename.startswith('__MACOSX/'): + if file.is_dir() or file.filename.startswith("__MACOSX/"): # skip directories and macos hidden directory continue filename = os.path.basename(file.filename) - if filename.startswith('.'): + if filename.startswith("."): # skip hidden files continue - file_doc = frappe.new_doc('File') + file_doc = frappe.new_doc("File") file_doc.content = z.read(file.filename) file_doc.file_name = filename file_doc.folder = self.folder @@ -342,28 +355,25 @@ class File(Document): file_doc.save() files.append(file_doc) - frappe.delete_doc('File', self.name) + frappe.delete_doc("File", self.name) return files - def exists_on_disk(self): - exists = os.path.exists(self.get_full_path()) - return exists + return os.path.exists(self.get_full_path()) - - def get_content(self): + def get_content(self) -> bytes: """Returns [`file_name`, `content`] for given file name `fname`""" if self.is_folder: frappe.throw(_("Cannot get file contents of a Folder")) - if self.get('content'): + if self.get("content"): return self.content - self.validate_url() + self.validate_file_url() file_path = self.get_full_path() # read the file - with io.open(encode(file_path), mode='rb') as f: + with io.open(encode(file_path), mode="rb") as f: content = f.read() try: # for plain text files @@ -383,7 +393,9 @@ class File(Document): file_path = "/files/" + file_path if file_path.startswith("/private/files/"): - file_path = get_files_path(*file_path.split("/private/files/", 1)[1].split("/"), is_private=1) + file_path = get_files_path( + *file_path.split("/private/files/", 1)[1].split("/"), is_private=1 + ) elif file_path.startswith("/files/"): file_path = get_files_path(*file_path.split("/files/", 1)[1].split("/")) @@ -392,7 +404,9 @@ class File(Document): pass elif not self.file_url: - frappe.throw(_("There is some problem with the file url: {0}").format(file_path)) + frappe.throw( + _("There is some problem with the file url: {0}").format(file_path) + ) if not is_safe_path(file_path): frappe.throw(f"Cannot access file path {file_path}") @@ -404,7 +418,7 @@ class File(Document): file_path = get_files_path(is_private=self.is_private) if os.path.sep in self.file_name: - frappe.throw(_('File name cannot have {0}').format(os.path.sep)) + frappe.throw(_("File name cannot have {0}").format(os.path.sep)) # create directory (if not exists) frappe.create_folder(file_path) @@ -412,12 +426,16 @@ class File(Document): self.content = self.get_content() if isinstance(self.content, str): self.content = self.content.encode() - with open(os.path.join(file_path.encode('utf-8'), self.file_name.encode('utf-8')), 'wb+') as f: + _file_path = os.path.join(file_path.encode("utf-8"), self.file_name.encode("utf-8")) + with open(_file_path, "wb+") as f: f.write(self.content) return get_files_path(self.file_name, is_private=self.is_private) def save_file(self, content=None, decode=False, ignore_existing_file_check=False): + if not self.content: + return + file_exists = False duplicate_file = None @@ -437,7 +455,8 @@ class File(Document): # transform file content based on site settings if ( - self.content_type and self.content_type == "image/jpeg" + self.content_type + and self.content_type == "image/jpeg" and frappe.get_system_settings("strip_exif_metadata_from_uploaded_images") ): self.content = strip_exif_data(self.content, self.content_type) @@ -446,57 +465,57 @@ class File(Document): # check if a file exists with the same content hash and is also in the same folder (public or private) if not ignore_existing_file_check: - duplicate_file = frappe.get_value("File", { - "content_hash": self.content_hash, - "is_private": self.is_private - }, - ["file_url", "name"], as_dict=True) + duplicate_file = frappe.get_value( + "File", + {"content_hash": self.content_hash, "is_private": self.is_private}, + ["file_url", "name"], + as_dict=True, + ) if duplicate_file: - file_doc = frappe.get_cached_doc('File', duplicate_file.name) + file_doc = frappe.get_cached_doc("File", duplicate_file.name) if file_doc.exists_on_disk(): - self.file_url = duplicate_file.file_url + self.file_url = duplicate_file.file_url file_exists = True if not file_exists: self.file_name = generate_file_name( name=self.file_name, suffix=self.content_hash[-6:], - is_private=self.is_private + is_private=self.is_private, ) call_hook_method("before_write_file", file_size=self.file_size) - write_file_method = get_hook_method('write_file') + write_file_method = get_hook_method("write_file") if write_file_method: return write_file_method(self) return self.save_file_on_filesystem() - def save_file_on_filesystem(self): fpath = self.write_file() if self.is_private: - self.file_url = "/private/files/{0}".format(self.file_name) + self.file_url = f"/private/files/{self.file_name}" else: - self.file_url = "/files/{0}".format(self.file_name) + self.file_url = f"/files/{self.file_name}" - return { - 'file_name': os.path.basename(fpath), - 'file_url': self.file_url - } + return {"file_name": os.path.basename(fpath), "file_url": self.file_url} def check_max_file_size(self): max_file_size = get_max_file_size() file_size = len(self.content) if file_size > max_file_size: - frappe.msgprint(_("File size exceeded the maximum allowed size of {0} MB").format( - max_file_size / 1048576), - raise_exception=MaxFileSizeReachedError) + frappe.throw( + _("File size exceeded the maximum allowed size of {0} MB").format( + max_file_size / 1048576 + ), + exc=MaxFileSizeReachedError, + ) return file_size def delete_file_data_content(self, only_thumbnail=False): - method = get_hook_method('delete_file_data_content') + method = get_hook_method("delete_file_data_content") if method: method(self, only_thumbnail=only_thumbnail) else: @@ -511,12 +530,22 @@ class File(Document): delete_file(self.thumbnail_url) def is_downloadable(self): - return has_permission(self, 'read') + return has_permission(self, "read") def get_extension(self): - '''returns split filename and extension''' + """returns split filename and extension""" return os.path.splitext(self.file_name) + def create_attachment_record(self): + icon = ' ' if self.is_private else "" + file_url = quote(frappe.safe_encode(self.file_url)) if self.file_url else self.file_name + file_name = self.file_name or self.file_url + + self.add_comment_in_reference_doc( + "Attachment", + _("Added {0}").format(f"{file_name}{icon}"), + ) + def add_comment_in_reference_doc(self, comment_type, text): if self.attached_to_doctype and self.attached_to_name: try: @@ -527,28 +556,28 @@ class File(Document): def set_is_private(self): if self.file_url: - self.is_private = cint(self.file_url.startswith('/private')) + self.is_private = cint(self.file_url.startswith("/private")) @frappe.whitelist() def optimize_file(self): if self.is_folder: - raise TypeError('Folders cannot be optimized') + raise TypeError("Folders cannot be optimized") content_type = mimetypes.guess_type(self.file_name)[0] - is_local_image = content_type.startswith('image/') and self.file_size > 0 - is_svg = content_type == 'image/svg+xml' + is_local_image = content_type.startswith("image/") and self.file_size > 0 + is_svg = content_type == "image/svg+xml" if not is_local_image: - raise NotImplementedError('Only local image files can be optimized') + raise NotImplementedError("Only local image files can be optimized") if is_svg: - raise TypeError('Optimization of SVG images is not supported') + raise TypeError("Optimization of SVG images is not supported") content = self.get_content() file_path = self.get_full_path() optimized_content = optimize_image(content, content_type) - with open(file_path, 'wb+') as f: + with open(file_path, "wb+") as f: f.write(optimized_content) self.file_size = len(optimized_content) @@ -611,5 +640,6 @@ def has_permission(doc, ptype=None, user=None): return has_access + # Note: kept at the end to not cause circular, partial imports & maintain backwards compatibility from frappe.core.api.file import * From 3110c42b80d4485e848dcc950775da8930ed097b Mon Sep 17 00:00:00 2001 From: Gavin D'souza Date: Tue, 15 Mar 2022 19:38:01 +0530 Subject: [PATCH 034/340] fix(utils): URLs to return True via is_safe_path checks --- frappe/utils/file_manager.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/frappe/utils/file_manager.py b/frappe/utils/file_manager.py index 15ba5d3d25..c8ad365152 100644 --- a/frappe/utils/file_manager.py +++ b/frappe/utils/file_manager.py @@ -398,7 +398,10 @@ def add_attachments(doctype, name, attachments): return files -def is_safe_path(path): +def is_safe_path(path: str) -> bool: + if path.startswith(("http://", "https://")): + return True + basedir = frappe.get_site_path() # ref: https://docs.python.org/3/library/os.path.html#os.path.commonpath matchpath = os.path.realpath(os.path.abspath(path)) From 5f66f610c36e21b9e42a8361a9a9c3909ea1cb60 Mon Sep 17 00:00:00 2001 From: Gavin D'souza Date: Wed, 16 Mar 2022 17:42:10 +0530 Subject: [PATCH 035/340] fix: File.file_url as empty string as fallback --- frappe/core/doctype/file/file.py | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/frappe/core/doctype/file/file.py b/frappe/core/doctype/file/file.py index 3f4c2069e1..732b1853d2 100755 --- a/frappe/core/doctype/file/file.py +++ b/frappe/core/doctype/file/file.py @@ -63,14 +63,12 @@ class File(Document): def validate(self): # Ensure correct formatting and type - self.file_url = unquote(self.file_url) + self.file_url = unquote(self.file_url) if self.file_url else "" if not self.is_new() and self.has_value_changed("is_private"): self.handle_is_private_changed() - if self.is_folder: - self.file_url = "" - else: + if not self.is_folder: self.validate_file_on_disk() self.validate_file_url() self.validate_file_path() From 3d25f6939ef91049ca8b45e39b9840120c6ff423 Mon Sep 17 00:00:00 2001 From: Gavin D'souza Date: Mon, 21 Mar 2022 15:46:34 +0530 Subject: [PATCH 036/340] refactor: File.optimize_file * Generalize file content reverting on rollback * Overwrite current file instead of adding a dangling new file * Use File.save_file instead of additional "custom" logic * Set File.content on calling get_content method --- frappe/core/doctype/file/file.py | 37 +++++++++++++++----------------- 1 file changed, 17 insertions(+), 20 deletions(-) diff --git a/frappe/core/doctype/file/file.py b/frappe/core/doctype/file/file.py index 732b1853d2..6d82312a42 100755 --- a/frappe/core/doctype/file/file.py +++ b/frappe/core/doctype/file/file.py @@ -372,15 +372,15 @@ class File(Document): # read the file with io.open(encode(file_path), mode="rb") as f: - content = f.read() + self.content = f.read() try: # for plain text files - content = content.decode() + self.content = self.content.decode() except UnicodeDecodeError: # for .png, .jpg, etc pass - return content + return self.content def get_full_path(self): """Returns file path from given file name""" @@ -427,10 +427,12 @@ class File(Document): _file_path = os.path.join(file_path.encode("utf-8"), self.file_name.encode("utf-8")) with open(_file_path, "wb+") as f: f.write(self.content) + frappe.local.rollback_observers.append(self) return get_files_path(self.file_name, is_private=self.is_private) - def save_file(self, content=None, decode=False, ignore_existing_file_check=False): + def save_file(self, content=None, decode=False, ignore_existing_file_check=False, overwrite=False): + self.flags.original_content = self.get_content() if not self.content: return @@ -477,11 +479,12 @@ class File(Document): file_exists = True if not file_exists: - self.file_name = generate_file_name( - name=self.file_name, - suffix=self.content_hash[-6:], - is_private=self.is_private, - ) + if not overwrite: + self.file_name = generate_file_name( + name=self.file_name, + suffix=self.content_hash[-6:], + is_private=self.is_private, + ) call_hook_method("before_write_file", file_size=self.file_size) write_file_method = get_hook_method("write_file") if write_file_method: @@ -571,18 +574,12 @@ class File(Document): if is_svg: raise TypeError("Optimization of SVG images is not supported") - content = self.get_content() - file_path = self.get_full_path() - optimized_content = optimize_image(content, content_type) + optimized_content = optimize_image( + content=self.get_content(), + content_type=content_type, + ) - with open(file_path, "wb+") as f: - f.write(optimized_content) - - self.file_size = len(optimized_content) - self.content_hash = get_content_hash(optimized_content) - # if rolledback, revert back to original - self.flags.original_content = content - frappe.local.rollback_observers.append(self) + self.save_file(content=optimized_content, overwrite=True) self.save() @staticmethod From e19ced52216c52ff362279adc95768a56481f637 Mon Sep 17 00:00:00 2001 From: Gavin D'souza Date: Mon, 21 Mar 2022 15:52:11 +0530 Subject: [PATCH 037/340] refactor: File.handle_is_private_changed * Make file renaming atomic * Add rollback observer to maintain filesystem consistency * Simplify logic --- frappe/core/doctype/file/file.py | 34 ++++++++++++++++++++++++++++---- 1 file changed, 30 insertions(+), 4 deletions(-) diff --git a/frappe/core/doctype/file/file.py b/frappe/core/doctype/file/file.py index 6d82312a42..03aef95dc4 100755 --- a/frappe/core/doctype/file/file.py +++ b/frappe/core/doctype/file/file.py @@ -96,6 +96,12 @@ class File(Document): with open(file_path, "wb+") as f: f.write(self.flags.original_content) + # used in case file path (File.file_url) has been changed + if self.flags.original_path: + target = self.flags.original_path["old"] + source = self.flags.original_path["new"] + shutil.move(source, target) + # following condition is only executed when an insert has been rolledback else: self.flags.on_rollback = True @@ -135,18 +141,38 @@ class File(Document): ) def handle_is_private_changed(self): + from pathlib import Path + old_file_url = self.file_url file_name = self.file_url.split("/")[-1] - private_file_path = frappe.get_site_path("private", "files", file_name) - public_file_path = frappe.get_site_path("public", "files", file_name) + private_file_path = Path(frappe.get_site_path("private", "files", file_name)) + public_file_path = Path(frappe.get_site_path("public", "files", file_name)) if cint(self.is_private): - shutil.move(public_file_path, private_file_path) + source = public_file_path + target = private_file_path url_starts_with = "/private/files/" else: - shutil.move(private_file_path, public_file_path) + source = private_file_path + target = public_file_path url_starts_with = "/files/" + if not source.exists(): + frappe.throw( + _("Cannot find file {} on disk").format(source), + exc=FileNotFoundError, + ) + if target.exists(): + frappe.throw( + _("A file with same name {} already exists").format(target), + exc=FileExistsError, + ) + + # Uses os.rename which is an atomic operation + shutil.move(source, target) + self.flags.original_path = {"old": source, "new": target} + frappe.local.rollback_observers.append(self) + self.file_url = f"{url_starts_with}{file_name}" update_existing_file_docs(self) From e1851f26d033598be87116ea504ccb9268522e55 Mon Sep 17 00:00:00 2001 From: Gavin D'souza Date: Mon, 21 Mar 2022 16:27:08 +0530 Subject: [PATCH 038/340] fix: os.fsync to flush internal buffers to disk ref: https://docs.python.org/3/library/os.html#os.fsync --- frappe/core/doctype/file/file.py | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/frappe/core/doctype/file/file.py b/frappe/core/doctype/file/file.py index 03aef95dc4..71e94158c8 100755 --- a/frappe/core/doctype/file/file.py +++ b/frappe/core/doctype/file/file.py @@ -48,6 +48,7 @@ class File(Document): self.name = frappe.generate_hash(length=10) def before_insert(self): + self.flags.new_file = True frappe.local.rollback_observers.append(self) self.set_folder_name() self.set_file_name() @@ -69,9 +70,9 @@ class File(Document): self.handle_is_private_changed() if not self.is_folder: - self.validate_file_on_disk() - self.validate_file_url() self.validate_file_path() + self.validate_file_url() + self.validate_file_on_disk() self.file_size = frappe.form_dict.file_size or self.file_size @@ -95,6 +96,7 @@ class File(Document): file_path = self.get_full_path() with open(file_path, "wb+") as f: f.write(self.flags.original_content) + os.fsync(f.fileno()) # used in case file path (File.file_url) has been changed if self.flags.original_path: @@ -103,9 +105,8 @@ class File(Document): shutil.move(source, target) # following condition is only executed when an insert has been rolledback - else: - self.flags.on_rollback = True - self.on_trash() + if self.flags.new_file: + self._delete_file_on_disk() def get_name_based_on_parent_folder(self) -> Union[str, None]: if self.folder: @@ -137,7 +138,8 @@ class File(Document): if not self.file_url.startswith(("/files/", "/private/files/")): # Probably an invalid URL since it doesn't start with http either frappe.throw( - _("URL must start with http:// or https://"), title=_("Invalid URL") + _("URL must start with http:// or https://"), + title=_("Invalid URL"), ) def handle_is_private_changed(self): @@ -453,6 +455,7 @@ class File(Document): _file_path = os.path.join(file_path.encode("utf-8"), self.file_name.encode("utf-8")) with open(_file_path, "wb+") as f: f.write(self.content) + os.fsync(f.fileno()) frappe.local.rollback_observers.append(self) return get_files_path(self.file_name, is_private=self.is_private) @@ -528,6 +531,8 @@ class File(Document): return {"file_name": os.path.basename(fpath), "file_url": self.file_url} def check_max_file_size(self): + from frappe.core.api.file import get_max_file_size + max_file_size = get_max_file_size() file_size = len(self.content) From 287543311cc41412f33bf0c499c1fdbd4fb584bf Mon Sep 17 00:00:00 2001 From: Gavin D'souza Date: Mon, 21 Mar 2022 16:28:57 +0530 Subject: [PATCH 039/340] fix: Remove duplicate rollback observers and maintain order --- frappe/database/database.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frappe/database/database.py b/frappe/database/database.py index a4eca64d8d..4152e32cd7 100644 --- a/frappe/database/database.py +++ b/frappe/database/database.py @@ -848,7 +848,7 @@ class Database(object): else: self.sql("rollback") self.begin() - for obj in frappe.local.rollback_observers: + for obj in dict.fromkeys(frappe.local.rollback_observers): if hasattr(obj, "on_rollback"): obj.on_rollback() frappe.local.rollback_observers = [] From 4caf7bcd123f7dacc0d5bc2178b389d8a94ff31a Mon Sep 17 00:00:00 2001 From: Gavin D'souza Date: Mon, 21 Mar 2022 17:31:56 +0530 Subject: [PATCH 040/340] fix: save_file only for non folder records --- frappe/core/doctype/file/file.py | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/frappe/core/doctype/file/file.py b/frappe/core/doctype/file/file.py index 71e94158c8..f1b2a2f235 100755 --- a/frappe/core/doctype/file/file.py +++ b/frappe/core/doctype/file/file.py @@ -48,11 +48,13 @@ class File(Document): self.name = frappe.generate_hash(length=10) def before_insert(self): - self.flags.new_file = True - frappe.local.rollback_observers.append(self) self.set_folder_name() self.set_file_name() - self.save_file(content=self.content, decode=self.decode) + + if not self.is_folder: + self.flags.new_file = True + frappe.local.rollback_observers.append(self) + self.save_file(content=self.content, decode=self.decode) def after_insert(self): if not self.is_folder: From f8f0402afdc1a94368b104423527673b09ae8723 Mon Sep 17 00:00:00 2001 From: Gavin D'souza Date: Wed, 23 Mar 2022 16:59:37 +0530 Subject: [PATCH 041/340] fix: Skip handling is_private updates for remote files --- frappe/core/doctype/file/file.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/frappe/core/doctype/file/file.py b/frappe/core/doctype/file/file.py index f1b2a2f235..c1e35b57d6 100755 --- a/frappe/core/doctype/file/file.py +++ b/frappe/core/doctype/file/file.py @@ -145,6 +145,9 @@ class File(Document): ) def handle_is_private_changed(self): + if not self.file_url.startswith(("/files/", "/private/files/")): + return + from pathlib import Path old_file_url = self.file_url From 60ef5363933780efba7e54f6223a274ded259fed Mon Sep 17 00:00:00 2001 From: Gavin D'souza Date: Thu, 24 Mar 2022 19:31:02 +0530 Subject: [PATCH 042/340] fix: Strip HTML tags in traceback shown in the terminal --- frappe/__init__.py | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/frappe/__init__.py b/frappe/__init__.py index 8a8b70afe3..1f5c7811a0 100644 --- a/frappe/__init__.py +++ b/frappe/__init__.py @@ -10,6 +10,7 @@ be used to build database driven apps. Read the documentation: https://frappeframework.com/docs """ +import functools import os, warnings STANDARD_USERS = ('Guest', 'Administrator') @@ -366,16 +367,20 @@ def msgprint(msg, title=None, raise_exception=0, as_table=False, as_list=False, :param is_minimizable: [optional] Allow users to minimize the modal :param wide: [optional] Show wide modal """ + import inspect from frappe.utils import strip_html_tags msg = safe_decode(msg) out = _dict(message=msg) + @functools.lru_cache(maxsize=1024) + def _strip_html_tags(message): + return strip_html_tags(message) + def _raise_exception(): if raise_exception: if flags.rollback_on_exception: db.rollback() - import inspect if inspect.isclass(raise_exception) and issubclass(raise_exception, Exception): raise raise_exception(msg) @@ -392,8 +397,11 @@ def msgprint(msg, title=None, raise_exception=0, as_table=False, as_list=False, if as_list and type(msg) in (list, tuple) and len(msg) > 1: out.as_list = 1 + if sys.stdin.isatty(): + msg = _strip_html_tags(out.message) + if flags.print_messages and out.message: - print(f"Message: {strip_html_tags(out.message)}") + print(f"Message: {_strip_html_tags(out.message)}") out.title = title or _("Message", context="Default title of the message dialog") From 8ec96dab694dbd8475b37ee83e0f7ad5d03423c2 Mon Sep 17 00:00:00 2001 From: Gavin D'souza Date: Tue, 23 Mar 2021 10:16:19 +0530 Subject: [PATCH 043/340] fix: Misc fixes * Set default Falsy value for content * Extend Frappe exceptions into file's --- frappe/core/doctype/file/exceptions.py | 2 ++ frappe/core/doctype/file/file.py | 9 ++++++--- 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/frappe/core/doctype/file/exceptions.py b/frappe/core/doctype/file/exceptions.py index 53ff4dad63..20e9dc588c 100644 --- a/frappe/core/doctype/file/exceptions.py +++ b/frappe/core/doctype/file/exceptions.py @@ -5,3 +5,5 @@ class MaxFileSizeReachedError(frappe.ValidationError): class FolderNotEmpty(frappe.ValidationError): pass + +from frappe.exceptions import * \ No newline at end of file diff --git a/frappe/core/doctype/file/file.py b/frappe/core/doctype/file/file.py index c1e35b57d6..79186d3a58 100755 --- a/frappe/core/doctype/file/file.py +++ b/frappe/core/doctype/file/file.py @@ -20,7 +20,7 @@ from frappe.utils import call_hook_method, cint, encode, get_files_path, get_hoo from frappe.utils.image import strip_exif_data, optimize_image from frappe.utils.file_manager import is_safe_path, safe_b64decode -from .exceptions import MaxFileSizeReachedError, FolderNotEmpty +from .exceptions import MaxFileSizeReachedError, FolderNotEmpty, AttachmentLimitReached from .utils import * @@ -33,7 +33,10 @@ class File(Document): def __init__(self, *args, **kwargs): super().__init__(*args, **kwargs) - self.content = self.get("content", None) + # if content is set, file_url will be generated + # decode comes in the picture if content passed has to be decoded before writing to disk + + self.content = self.get("content") or b"" self.decode = self.get("decode", False) def autoname(self): @@ -225,7 +228,7 @@ class File(Document): _("Maximum Attachment Limit of {0} has been reached for {1} {2}.").format( frappe.bold(attachment_limit), self.attached_to_doctype, self.attached_to_name ), - exc=frappe.exceptions.AttachmentLimitReached, + exc=AttachmentLimitReached, title=_('Attachment Limit Reached') ) From d53b05fcb7a5477bb75c37e1eee7e2ac64430771 Mon Sep 17 00:00:00 2001 From: Gavin D'souza Date: Thu, 24 Mar 2022 21:15:57 +0530 Subject: [PATCH 044/340] refactor(file): File.is_remote_file instead of startswith http --- frappe/core/doctype/file/file.py | 27 +++++++++++++++------------ 1 file changed, 15 insertions(+), 12 deletions(-) diff --git a/frappe/core/doctype/file/file.py b/frappe/core/doctype/file/file.py index 79186d3a58..163e79294e 100755 --- a/frappe/core/doctype/file/file.py +++ b/frappe/core/doctype/file/file.py @@ -26,6 +26,7 @@ from .utils import * exclude_from_linked_with = True ImageFile.LOAD_TRUNCATED_IMAGES = True +URL_PREFIXES = ("http://", "https://") class File(Document): @@ -39,6 +40,10 @@ class File(Document): self.content = self.get("content") or b"" self.decode = self.get("decode", False) + @property + def is_remote_file(self): + return not self.content or (self.file_url or "").startswith(URL_PREFIXES) + def autoname(self): """Set name for folder""" if self.is_folder: @@ -54,10 +59,10 @@ class File(Document): self.set_folder_name() self.set_file_name() - if not self.is_folder: + if not self.is_folder and not self.is_remote_file: + self.save_file(content=self.get_content()) self.flags.new_file = True frappe.local.rollback_observers.append(self) - self.save_file(content=self.content, decode=self.decode) def after_insert(self): if not self.is_folder: @@ -123,7 +128,7 @@ class File(Document): ) def validate_file_path(self): - if self.file_url.startswith(("http://", "https://")): + if self.is_remote_file: return base_path = os.path.realpath(get_files_path(is_private=self.is_private)) @@ -134,10 +139,7 @@ class File(Document): ) def validate_file_url(self): - if self.file_url.startswith(("http://", "https://")): - return - - if not self.file_url: + if self.is_remote_file or not self.file_url: return if not self.file_url.startswith(("/files/", "/private/files/")): @@ -148,7 +150,7 @@ class File(Document): ) def handle_is_private_changed(self): - if not self.file_url.startswith(("/files/", "/private/files/")): + if self.is_remote_file: return from pathlib import Path @@ -248,7 +250,7 @@ class File(Document): """ full_path = self.get_full_path() - if full_path.startswith("http"): + if full_path.startswith(URL_PREFIXES): return True if not os.path.exists(full_path): @@ -290,7 +292,7 @@ class File(Document): self.file_name = re.sub(r"/", "", self.file_name) def generate_content_hash(self): - if self.content_hash or not self.file_url or self.file_url.startswith("http"): + if self.content_hash or not self.file_url or self.is_remote_file: return file_name = self.file_url.split("/")[-1] try: @@ -434,7 +436,7 @@ class File(Document): elif file_path.startswith("/files/"): file_path = get_files_path(*file_path.split("/files/", 1)[1].split("/")) - elif file_path.startswith("http"): + elif file_path.startswith(URL_PREFIXES): pass elif not self.file_url: @@ -449,7 +451,8 @@ class File(Document): def write_file(self): """write file to disk with a random name (to compare)""" - file_path = get_files_path(is_private=self.is_private) + if self.is_remote_file: + return if os.path.sep in self.file_name: frappe.throw(_("File name cannot have {0}").format(os.path.sep)) From e18025751017a79b826e360ab0bdee899aaf86b1 Mon Sep 17 00:00:00 2001 From: Gavin D'souza Date: Tue, 23 Mar 2021 12:16:19 +0530 Subject: [PATCH 045/340] fix: Handle File on rollback Delete, Move or Restore files in the filesystem by setting Document flags in the on_rollback method --- frappe/core/doctype/file/file.py | 20 +++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-) diff --git a/frappe/core/doctype/file/file.py b/frappe/core/doctype/file/file.py index 163e79294e..866bcba287 100755 --- a/frappe/core/doctype/file/file.py +++ b/frappe/core/doctype/file/file.py @@ -101,22 +101,32 @@ class File(Document): ) def on_rollback(self): + # following condition is only executed when an insert has been rolledback + if self.flags.new_file: + self._delete_file_on_disk() + self.flags.pop("new_file") + return + # if original_content flag is set, this rollback should revert the file to its original state if self.flags.original_content: file_path = self.get_full_path() - with open(file_path, "wb+") as f: + + if isinstance(self.flags.original_content, bytes): + mode = "wb+" + elif isinstance(self.flags.original_content, str): + mode = "w+" + + with open(file_path, mode) as f: f.write(self.flags.original_content) os.fsync(f.fileno()) + self.flags.pop("original_content") # used in case file path (File.file_url) has been changed if self.flags.original_path: target = self.flags.original_path["old"] source = self.flags.original_path["new"] shutil.move(source, target) - - # following condition is only executed when an insert has been rolledback - if self.flags.new_file: - self._delete_file_on_disk() + self.flags.pop("original_path") def get_name_based_on_parent_folder(self) -> Union[str, None]: if self.folder: From e6192f2232b222def6276fc67c62ab6f87909ecb Mon Sep 17 00:00:00 2001 From: Gavin D'souza Date: Thu, 24 Mar 2022 21:21:44 +0530 Subject: [PATCH 046/340] refactor(File): Use _content attribute instead of overwriting content --- frappe/core/doctype/file/file.py | 76 ++++++++++++++++--------------- frappe/core/doctype/file/utils.py | 8 ++++ 2 files changed, 47 insertions(+), 37 deletions(-) diff --git a/frappe/core/doctype/file/file.py b/frappe/core/doctype/file/file.py index 866bcba287..8748564741 100755 --- a/frappe/core/doctype/file/file.py +++ b/frappe/core/doctype/file/file.py @@ -413,22 +413,28 @@ class File(Document): frappe.throw(_("Cannot get file contents of a Folder")) if self.get("content"): - return self.content + self._content = self.content + if self.decode: + self._content = decode_file_content(self._content) + self.decode = False + # self.content = None # TODO: This needs to happen; make it happen somehow + return self._content - self.validate_file_url() + if self.file_url: + self.validate_file_url() file_path = self.get_full_path() # read the file - with io.open(encode(file_path), mode="rb") as f: - self.content = f.read() + with open(file_path, mode="rb") as f: + self._content = f.read() try: # for plain text files - self.content = self.content.decode() + self._content = self._content.decode() except UnicodeDecodeError: # for .png, .jpg, etc pass - return self.content + return self._content def get_full_path(self): """Returns file path from given file name""" @@ -464,44 +470,39 @@ class File(Document): if self.is_remote_file: return - if os.path.sep in self.file_name: - frappe.throw(_("File name cannot have {0}").format(os.path.sep)) + file_path = self.get_full_path() - # create directory (if not exists) - frappe.create_folder(file_path) - # write the file - self.content = self.get_content() - if isinstance(self.content, str): - self.content = self.content.encode() - _file_path = os.path.join(file_path.encode("utf-8"), self.file_name.encode("utf-8")) - with open(_file_path, "wb+") as f: - f.write(self.content) + if isinstance(self._content, str): + self._content = self._content.encode() + + with open(file_path, "wb+") as f: + f.write(self._content) os.fsync(f.fileno()) + frappe.local.rollback_observers.append(self) - return get_files_path(self.file_name, is_private=self.is_private) + return file_path - def save_file(self, content=None, decode=False, ignore_existing_file_check=False, overwrite=False): - self.flags.original_content = self.get_content() - if not self.content: + def save_file(self, content: Optional[Union[bytes, str]] = None, decode=False, ignore_existing_file_check=False, overwrite=False): + if self.is_remote_file: + return + + if not self.flags.new_file: + self.flags.original_content = self.get_content() + + if content: + self.content = content + self.decode = decode + self.get_content() + + if not self._content: return file_exists = False duplicate_file = None - self.content = content self.is_private = cint(self.is_private) self.content_type = mimetypes.guess_type(self.file_name)[0] - self.file_size = self.check_max_file_size() - - # decode self.content - if decode: - if isinstance(content, str): - self.content = content.encode("utf-8") - - if b"," in self.content: - self.content = self.content.split(b",")[1] - self.content = safe_b64decode(self.content) # transform file content based on site settings if ( @@ -509,9 +510,10 @@ class File(Document): and self.content_type == "image/jpeg" and frappe.get_system_settings("strip_exif_metadata_from_uploaded_images") ): - self.content = strip_exif_data(self.content, self.content_type) + self._content = strip_exif_data(self._content, self.content_type) - self.content_hash = get_content_hash(self.content) + self.file_size = self.check_max_file_size() + self.content_hash = get_content_hash(self._content) # check if a file exists with the same content hash and is also in the same folder (public or private) if not ignore_existing_file_check: @@ -542,20 +544,20 @@ class File(Document): return self.save_file_on_filesystem() def save_file_on_filesystem(self): - fpath = self.write_file() - if self.is_private: self.file_url = f"/private/files/{self.file_name}" else: self.file_url = f"/files/{self.file_name}" + fpath = self.write_file() + return {"file_name": os.path.basename(fpath), "file_url": self.file_url} def check_max_file_size(self): from frappe.core.api.file import get_max_file_size max_file_size = get_max_file_size() - file_size = len(self.content) + file_size = len(self._content or b"") if file_size > max_file_size: frappe.throw( diff --git a/frappe/core/doctype/file/utils.py b/frappe/core/doctype/file/utils.py index d9c267d466..583f769d64 100644 --- a/frappe/core/doctype/file/utils.py +++ b/frappe/core/doctype/file/utils.py @@ -313,3 +313,11 @@ def attach_files_to_document(doc: "File", event) -> None: ).insert() except Exception: frappe.log_error(title=_("Error Attaching File")) + + +def decode_file_content(content: bytes) -> bytes: + if isinstance(content, str): + content = content.encode("utf-8") + if b"," in content: + content = content.split(b",")[1] + return safe_b64decode(content) From 654f744e63f40200ffa5c81ec7d539df11e81f8f Mon Sep 17 00:00:00 2001 From: Gavin D'souza Date: Thu, 24 Mar 2022 21:27:21 +0530 Subject: [PATCH 047/340] fix(File): Misc fixes * Move checks into common methods * Refactor to use latest APIs * Remove seemingly unnecessary code * Fix previously undhanled use cases --- frappe/core/doctype/file/file.py | 18 ++++++++++++------ frappe/core/doctype/file/utils.py | 5 +++-- frappe/hooks.py | 2 +- 3 files changed, 16 insertions(+), 9 deletions(-) diff --git a/frappe/core/doctype/file/file.py b/frappe/core/doctype/file/file.py index 8748564741..f3c00eb2a2 100755 --- a/frappe/core/doctype/file/file.py +++ b/frappe/core/doctype/file/file.py @@ -252,7 +252,7 @@ class File(Document): if self.attached_to_doctype: self.folder = frappe.db.get_value("File", {"is_attachments_folder": 1}) - if not self.is_home_folder: + elif not self.is_home_folder: self.folder = "Home" def validate_file_on_disk(self): @@ -367,9 +367,9 @@ class File(Document): limit=1, ) ) - if self.file_name and on_disk_file_not_shared: + if on_disk_file_not_shared: self.delete_file_data_content() - elif self.file_url: + else: self.delete_file_data_content(only_thumbnail=True) def unzip(self) -> List["File"]: @@ -442,7 +442,10 @@ class File(Document): file_path = self.file_url or self.file_name if "/" not in file_path: - file_path = "/files/" + file_path + if self.is_private: + file_path = f"/private/files/{file_path}" + else: + file_path = f"/files/{file_path}" if file_path.startswith("/private/files/"): file_path = get_files_path( @@ -461,7 +464,10 @@ class File(Document): ) if not is_safe_path(file_path): - frappe.throw(f"Cannot access file path {file_path}") + frappe.throw(_("Cannot access file path {0}").format(file_path)) + + if os.path.sep in self.file_name: + frappe.throw(_("File name cannot have {0}").format(os.path.sep)) return file_path @@ -525,7 +531,7 @@ class File(Document): ) if duplicate_file: - file_doc = frappe.get_cached_doc("File", duplicate_file.name) + file_doc: "File" = frappe.get_cached_doc("File", duplicate_file.name) if file_doc.exists_on_disk(): self.file_url = duplicate_file.file_url file_exists = True diff --git a/frappe/core/doctype/file/utils.py b/frappe/core/doctype/file/utils.py index 583f769d64..be8d067a50 100644 --- a/frappe/core/doctype/file/utils.py +++ b/frappe/core/doctype/file/utils.py @@ -233,8 +233,8 @@ def extract_images_from_html(doc: "Document", content: str, is_private: bool = F else: filename = get_random_filename(content_type=mtype) - doctype = doc.parenttype if doc.parent else doc.doctype - name = doc.parent or doc.name + doctype = doc.parenttype if doc.get("parent") else doc.doctype + name = doc.get("parent") or doc.name _file = frappe.get_doc({ "doctype": "File", @@ -277,6 +277,7 @@ def update_existing_file_docs(doc: "File") -> None: .where(file_doctype.name != doc.name) ).run() + def attach_files_to_document(doc: "File", event) -> None: """Runs on on_update hook of all documents. Goes through every Attach and Attach Image field and attaches diff --git a/frappe/hooks.py b/frappe/hooks.py index 6b8cef31e9..1ecd47b535 100644 --- a/frappe/hooks.py +++ b/frappe/hooks.py @@ -154,7 +154,7 @@ doc_events = { "frappe.core.doctype.activity_log.feed.update_feed", "frappe.workflow.doctype.workflow_action.workflow_action.process_workflow_actions", "frappe.automation.doctype.assignment_rule.assignment_rule.apply", - "frappe.core.doctype.file.file.attach_files_to_document", + "frappe.core.doctype.file.utils.attach_files_to_document", "frappe.event_streaming.doctype.event_update_log.event_update_log.notify_consumers", "frappe.automation.doctype.assignment_rule.assignment_rule.update_due_date", "frappe.core.doctype.user_type.user_type.apply_permissions_for_non_standard_user_type" From cec4c92a29d59221d71ac0e45d511927b9df3233 Mon Sep 17 00:00:00 2001 From: Gavin D'souza Date: Thu, 24 Mar 2022 21:31:28 +0530 Subject: [PATCH 048/340] fix(test): Update file tests for the new API changes --- frappe/core/doctype/file/test_file.py | 61 +++++++++++++++------------ 1 file changed, 33 insertions(+), 28 deletions(-) diff --git a/frappe/core/doctype/file/test_file.py b/frappe/core/doctype/file/test_file.py index a130f2caff..f6310ef454 100644 --- a/frappe/core/doctype/file/test_file.py +++ b/frappe/core/doctype/file/test_file.py @@ -9,6 +9,7 @@ import frappe from frappe import _ from frappe.core.api.file import create_new_folder, get_attached_images, get_files_in_folder, move_file, unzip_file from frappe.core.doctype.file.file import File +from frappe.exceptions import ValidationError from frappe.utils import get_files_path test_content1 = 'Hello' @@ -46,7 +47,7 @@ class TestBase64File(unittest.TestCase): def setUp(self): self.attached_to_doctype, self.attached_to_docname = make_test_doc() self.test_content = base64.b64encode(test_content1.encode('utf-8')) - _file = frappe.get_doc({ + _file: File = frappe.get_doc({ "doctype": "File", "file_name": "test_base64.txt", "attached_to_doctype": self.attached_to_doctype, @@ -297,7 +298,7 @@ class TestFile(unittest.TestCase): _file.save() folder = frappe.get_doc("File", "Home/Test Folder 1/Test Folder 3") - self.assertRaises(frappe.ValidationError, folder.delete) + self.assertRaises(ValidationError, folder.delete) def test_same_file_url_update(self): attached_to_doctype1, attached_to_docname1 = make_test_doc() @@ -336,21 +337,20 @@ class TestFile(unittest.TestCase): file1 = frappe.get_doc({ "doctype": "File", "file_name": 'parent_dir.txt', - "attached_to_doctype": "", - "attached_to_name": "", "is_private": 1, - "content": test_content1}).insert() + "content": test_content1, + }).insert() file1.file_url = '/private/files/../test.txt' - self.assertRaises(frappe.exceptions.ValidationError, file1.save) + self.assertRaises(ValidationError, file1.save) # No validation to see if file exists file1.reload() file1.file_url = '/private/files/parent_dir2.txt' - file1.save() + self.assertRaises(OSError, file1.save) def test_file_url_validation(self): - test_file = frappe.get_doc({ + test_file: File = frappe.get_doc({ "doctype": "File", "file_name": 'logo', "file_url": 'https://frappe.io/files/frappe.png' @@ -360,11 +360,11 @@ class TestFile(unittest.TestCase): # bad path test_file.file_url = "/usr/bin/man" - self.assertRaisesRegex(frappe.exceptions.ValidationError, "URL must start with http:// or https://", test_file.validate) + self.assertRaisesRegex(ValidationError, f"Cannot access file path {test_file.file_url}", test_file.validate) test_file.file_url = None test_file.file_name = "/usr/bin/man" - self.assertRaisesRegex(frappe.exceptions.ValidationError, "There is some problem with the file url", test_file.validate) + self.assertRaisesRegex(ValidationError, "There is some problem with the file url", test_file.validate) test_file.file_url = None test_file.file_name = "_file" @@ -372,7 +372,7 @@ class TestFile(unittest.TestCase): test_file.file_url = None test_file.file_name = "/private/files/_file" - self.assertRaisesRegex(IOError, "does not exist", test_file.validate) + self.assertRaisesRegex(ValidationError, "File name cannot have", test_file.validate) def test_make_thumbnail(self): # test web image @@ -431,29 +431,29 @@ class TestFile(unittest.TestCase): "doctype": "File", "file_url": frappe.utils.get_url('/_test/assets/image.jpg'), }).insert(ignore_permissions=True) - self.assertRaisesRegex(frappe.exceptions.ValidationError, 'not a zip file', test_file.unzip) + self.assertRaisesRegex(ValidationError, 'not a zip file', test_file.unzip) class TestAttachment(unittest.TestCase): test_doctype = 'Test For Attachment' - def setUp(self): - if frappe.db.exists('DocType', self.test_doctype): - return - + @classmethod + def setUpClass(cls): frappe.get_doc( doctype='DocType', - name=self.test_doctype, + name=cls.test_doctype, module='Custom', custom=1, fields=[ {'label': 'Title', 'fieldname': 'title', 'fieldtype': 'Data'}, {'label': 'Attachment', 'fieldname': 'attachment', 'fieldtype': 'Attach'}, - ] - ).insert() + ], + ).insert(ignore_if_duplicate=True) - def tearDown(self): - frappe.delete_doc('DocType', self.test_doctype) + @classmethod + def tearDownClass(cls): + frappe.db.rollback() + frappe.delete_doc('DocType', cls.test_doctype) def test_file_attachment_on_update(self): doc = frappe.get_doc( @@ -465,8 +465,7 @@ class TestAttachment(unittest.TestCase): 'doctype': 'File', 'file_name': 'test_attach.txt', 'content': 'Test Content' - }) - file.save() + }).save() doc.attachment = file.file_url doc.save() @@ -483,9 +482,10 @@ class TestAttachment(unittest.TestCase): class TestAttachmentsAccess(unittest.TestCase): + def setUp(self) -> None: + frappe.db.delete("File", {"is_folder": False}) def test_attachments_access(self): - frappe.set_user('test4@example.com') self.attached_to_doctype, self.attached_to_docname = make_test_doc() @@ -536,6 +536,7 @@ class TestAttachmentsAccess(unittest.TestCase): self.assertIn('test_user.txt', system_manager_attachments_files) self.assertIn('test_user.txt', user_attachments_files) + def tearDown(self) -> None: frappe.set_user('Administrator') frappe.db.rollback() @@ -614,16 +615,20 @@ class TestFileOptimization(unittest.TestCase): file_path = frappe.get_app_path("frappe", "tests/data/sample_image_for_optimization.jpg") with open(file_path, "rb") as f: file_content = f.read() - test_file = frappe.get_doc({ + + test_file: File = frappe.get_doc({ "doctype": "File", "file_name": "sample_image_for_optimization.jpg", - "content": file_content + "content": file_content, }).insert() + test_file.flags.new_file = False # so that we can get away with committing this for now image_path = test_file.get_full_path() - size_before_optimization = os.stat(image_path).st_size + size_before_optimization = os.stat(image_path).st_size test_file.optimize_file() - frappe.db.rollback() + + test_file.on_rollback() + size_after_rollback = os.stat(image_path).st_size self.assertEqual(size_before_optimization, size_after_rollback) test_file.delete() From f23298fc16e9d7c57c45c0c208d1e76ba00a129e Mon Sep 17 00:00:00 2001 From: Gavin D'souza Date: Thu, 24 Mar 2022 21:32:03 +0530 Subject: [PATCH 049/340] chore: Drop unmaintained file To check out the up-to-date version of hooks.md, see https://frappeframework.com/docs/v13/user/en/python-api/hooks --- hooks.md | 38 -------------------------------------- 1 file changed, 38 deletions(-) delete mode 100644 hooks.md diff --git a/hooks.md b/hooks.md deleted file mode 100644 index 5ff70471c9..0000000000 --- a/hooks.md +++ /dev/null @@ -1,38 +0,0 @@ -### List of Hooks - -#### Application Name and Details - -1. `app_name` - slugified name e.g. "frappe" -1. `app_title` - full title name e.g. "Frappe" -1. `app_publisher` -1. `app_description` -1. `app_version` -1. `app_icon` - font-awesome icon or image url -1. `app_color` - hex colour background of the app icon - -#### Install - -1. `before_install` - method -1. `after_install` - method - - -#### Javascript / CSS Builds - -1. `app_include_js` - include in "app" -1. `app_include_css` - assets/frappe/css/splash.css - -1. `web_include_js` - assets/js/frappe-web.min.js -1. `web_include_css` - assets/css/frappe-web.css - -#### Desktop - -1. `get_desktop_icons` - method to get list of desktop icons - -#### Notifications - -1. `notification_config` - method to get notification configuration - -#### Permissions - -1. `permission_query_conditions:[doctype]` - method to return additional query conditions at time of report / list etc. -1. `has_permission:[doctype]` - method to call permissions to check at individual level From 2f413cab53eee13fdff69f31b7134f304cf2343d Mon Sep 17 00:00:00 2001 From: Gavin D'souza Date: Mon, 28 Mar 2022 18:13:18 +0530 Subject: [PATCH 050/340] feat(FileUploader): Added "max_number_of_files" restriction FileUploader will accept N files and pop the rest, followed by a console.warn and frappe.show_alert --- .../js/frappe/file_uploader/FileUploader.vue | 38 ++++++++++++++++--- .../js/frappe/form/sidebar/attachments.js | 3 ++ 2 files changed, 35 insertions(+), 6 deletions(-) diff --git a/frappe/public/js/frappe/file_uploader/FileUploader.vue b/frappe/public/js/frappe/file_uploader/FileUploader.vue index c671b28160..10acf89ab8 100644 --- a/frappe/public/js/frappe/file_uploader/FileUploader.vue +++ b/frappe/public/js/frappe/file_uploader/FileUploader.vue @@ -36,7 +36,7 @@ ref="file_input" @change="on_file_input" :multiple="allow_multiple" - :accept="restrictions.allowed_file_types.join(', ')" + :accept="restrictions.allowed_file_types && restrictions.allowed_file_types.join(', ')" >
    `; } - const comment_count = ` + const comment_count = ` ${frappe.utils.icon('small-message')} ${doc._comment_count > 99 ? "99+" : doc._comment_count} `; From ec798598efb9915abbc2b517c6b3cccd43f21c25 Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Sat, 21 May 2022 17:34:59 +0530 Subject: [PATCH 077/340] fix: pagination button contrast --- frappe/public/scss/desk/list.scss | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/frappe/public/scss/desk/list.scss b/frappe/public/scss/desk/list.scss index c71dbdca89..07f6636825 100644 --- a/frappe/public/scss/desk/list.scss +++ b/frappe/public/scss/desk/list.scss @@ -204,8 +204,8 @@ $level-margin-right: 8px; border: 1px solid var(--dark-border-color); &.btn-info { - background-color: var(--gray-400); - border-color: var(--gray-400); + background-color: var(--gray-600); + border-color: var(--gray-600); color: var(--white); font-weight: var(--text-bold); } From 552ea50a733a6a2bdcbcf43668379bb339f514ab Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Sat, 21 May 2022 17:51:23 +0530 Subject: [PATCH 078/340] fix: bump shade of muted text to gray-700 --- frappe/public/scss/common/css_variables.scss | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frappe/public/scss/common/css_variables.scss b/frappe/public/scss/common/css_variables.scss index 92e505442d..d19502d1aa 100644 --- a/frappe/public/scss/common/css_variables.scss +++ b/frappe/public/scss/common/css_variables.scss @@ -143,7 +143,7 @@ --btn-shadow: var(--shadow-xs); // Type Colors - --text-muted: var(--gray-600); + --text-muted: var(--gray-700); --text-light: var(--gray-800); --text-color: var(--gray-900); --heading-color: var(--gray-900); From ae6657c66b9b489ab4c11d69bf23e5acfde49785 Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Sat, 21 May 2022 17:57:06 +0530 Subject: [PATCH 079/340] fix: darken text color on light blue buttons --- frappe/public/scss/desk/list.scss | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frappe/public/scss/desk/list.scss b/frappe/public/scss/desk/list.scss index 07f6636825..31d1661abb 100644 --- a/frappe/public/scss/desk/list.scss +++ b/frappe/public/scss/desk/list.scss @@ -401,7 +401,7 @@ input.list-check-all { } .filter-button.btn-primary-light { - color: var(--blue-500); + color: var(--text-on-blue); } .sort-selector { From 2800923d1b1b090d4f959e7ebc9e4e6d52d4817d Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Mon, 23 May 2022 14:31:32 +0530 Subject: [PATCH 080/340] fix: darken blue 600 Co-Authored-By: Shariq Ansari --- frappe/public/scss/common/css_variables.scss | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/frappe/public/scss/common/css_variables.scss b/frappe/public/scss/common/css_variables.scss index d19502d1aa..3593469e68 100644 --- a/frappe/public/scss/common/css_variables.scss +++ b/frappe/public/scss/common/css_variables.scss @@ -16,7 +16,7 @@ --blue-900: #1A4469; --blue-800: #154875; --blue-700: #1366AE; - --blue-600: #1579D0; + --blue-600: #1673C5; --blue-500: #2490EF; --blue-400: #50A6F2; --blue-300: #7CBCF5; @@ -178,7 +178,7 @@ --text-3xl: 22px; --text-on-blue: var(--blue-700); - --text-on-light-blue: var(--blue-700); + --text-on-light-blue: var(--blue-600); --text-on-dark-blue: var(--blue-700); --text-on-green: var(--dark-green-700); --text-on-yellow: var(--yellow-700); From 57b2f755e59b26e7c3f39e634615d65d41cc7230 Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Tue, 24 May 2022 11:50:51 +0530 Subject: [PATCH 081/340] fix: use darker shade for text on dark blue Co-authored-by: Shariq Ansari <30859809+shariquerik@users.noreply.github.com> --- frappe/public/scss/common/css_variables.scss | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frappe/public/scss/common/css_variables.scss b/frappe/public/scss/common/css_variables.scss index 3593469e68..cadb5bf330 100644 --- a/frappe/public/scss/common/css_variables.scss +++ b/frappe/public/scss/common/css_variables.scss @@ -179,7 +179,7 @@ --text-on-blue: var(--blue-700); --text-on-light-blue: var(--blue-600); - --text-on-dark-blue: var(--blue-700); + --text-on-dark-blue: var(--blue-800); --text-on-green: var(--dark-green-700); --text-on-yellow: var(--yellow-700); --text-on-orange: var(--orange-600); From b61e1d8e55fd727d506bf2df90ce252a79243fae Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Tue, 24 May 2022 12:19:36 +0530 Subject: [PATCH 082/340] fix: same stroke color as text on list filters --- frappe/public/scss/desk/filters.scss | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frappe/public/scss/desk/filters.scss b/frappe/public/scss/desk/filters.scss index 0552311ee2..ffaea7a9bd 100644 --- a/frappe/public/scss/desk/filters.scss +++ b/frappe/public/scss/desk/filters.scss @@ -1,6 +1,6 @@ .filter-icon.active { use { - stroke: var(--blue-500); + stroke: var(--text-on-blue); } } From 5bd3fd8d6515b45df11295dad8eaed64fffa3e56 Mon Sep 17 00:00:00 2001 From: Shariq Ansari Date: Tue, 24 May 2022 18:18:03 +0530 Subject: [PATCH 083/340] fix: prioritize report custom roles --- frappe/core/doctype/report/report.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/frappe/core/doctype/report/report.py b/frappe/core/doctype/report/report.py index 9e6cc73f11..efb45f41c8 100644 --- a/frappe/core/doctype/report/report.py +++ b/frappe/core/doctype/report/report.py @@ -89,7 +89,9 @@ class Report(Document): ] custom_roles = get_custom_allowed_roles("report", self.name) - allowed.extend(custom_roles) + + if custom_roles: + allowed = custom_roles if not allowed: return True From a3d9d8028f03009ecc6ceab490b4967d310232e0 Mon Sep 17 00:00:00 2001 From: Shariq Ansari Date: Tue, 24 May 2022 20:14:05 +0530 Subject: [PATCH 084/340] test: added unit test for report custom role permissions --- frappe/core/doctype/report/test_report.py | 32 +++++++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/frappe/core/doctype/report/test_report.py b/frappe/core/doctype/report/test_report.py index 7b17a5a8d5..e490dd5c01 100644 --- a/frappe/core/doctype/report/test_report.py +++ b/frappe/core/doctype/report/test_report.py @@ -186,6 +186,38 @@ class TestReport(FrappeTestCase): self.assertNotEqual(report.is_permitted(), True) frappe.set_user("Administrator") + def test_report_custom_permissions(self): + frappe.set_user("test@example.com") + frappe.db.delete("Custom Role", {"report": "Test Custom Role Report"}) + frappe.db.commit() + if not frappe.db.exists("Report", "Test Custom Role Report"): + report = frappe.get_doc( + { + "doctype": "Report", + "ref_doctype": "User", + "report_name": "Test Custom Role Report", + "report_type": "Query Report", + "is_standard": "No", + "roles": [{"role": "_Test Role"}, {"role": "System Manager"}], + } + ).insert(ignore_permissions=True) + else: + report = frappe.get_doc("Report", "Test Custom Role Report") + + self.assertEqual(report.is_permitted(), True) + + custom_role = frappe.get_doc( + { + "doctype": "Custom Role", + "report": "Test Custom Role Report", + "roles": [{"role": "_Test Role 2"}], + "ref_doctype": "User", + } + ).insert(ignore_permissions=True) + + self.assertNotEqual(report.is_permitted(), True) + frappe.set_user("Administrator") + # test for the `_format` method if report data doesn't have sort_by parameter def test_format_method(self): if frappe.db.exists("Report", "User Activity Report Without Sort"): From 95921c870762a9ef7fbff0a7f9f184df2e223fd5 Mon Sep 17 00:00:00 2001 From: Shariq Ansari Date: Tue, 24 May 2022 21:49:49 +0530 Subject: [PATCH 085/340] chore: removed unused variable & added nosemgrep on db commit --- frappe/core/doctype/report/test_report.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/frappe/core/doctype/report/test_report.py b/frappe/core/doctype/report/test_report.py index e490dd5c01..bbae616e93 100644 --- a/frappe/core/doctype/report/test_report.py +++ b/frappe/core/doctype/report/test_report.py @@ -189,7 +189,7 @@ class TestReport(FrappeTestCase): def test_report_custom_permissions(self): frappe.set_user("test@example.com") frappe.db.delete("Custom Role", {"report": "Test Custom Role Report"}) - frappe.db.commit() + frappe.db.commit() # nosemgrep if not frappe.db.exists("Report", "Test Custom Role Report"): report = frappe.get_doc( { @@ -206,7 +206,7 @@ class TestReport(FrappeTestCase): self.assertEqual(report.is_permitted(), True) - custom_role = frappe.get_doc( + frappe.get_doc( { "doctype": "Custom Role", "report": "Test Custom Role Report", From 576af52ec6039beb344388030112ad3de6f6d2d9 Mon Sep 17 00:00:00 2001 From: Raffael Meyer <14891507+barredterra@users.noreply.github.com> Date: Tue, 24 May 2022 18:26:34 +0200 Subject: [PATCH 086/340] fix: mention only users that can be read (#16978) Full Name and Email ID of a **User** are sensitive data. With **Role and User Permissions** we can restrict which other users a user can see, thus maintaining data protection. This could be circumvented by using the `@mention` functionality in comments. It allows us to see all users and their names, regardless of our permissions. This PR aims to fix this issue by using `get_list` instead of `get_all` while retrieving options for `@mention`. --- frappe/desk/search.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/frappe/desk/search.py b/frappe/desk/search.py index eb1a2e82ba..639077da5e 100644 --- a/frappe/desk/search.py +++ b/frappe/desk/search.py @@ -348,7 +348,7 @@ def get_names_for_mentions(search_term): def get_users_for_mentions(): - return frappe.get_all( + return frappe.get_list( "User", fields=["name as id", "full_name as value"], filters={ @@ -361,7 +361,7 @@ def get_users_for_mentions(): def get_user_groups(): - return frappe.get_all( + return frappe.get_list( "User Group", fields=["name as id", "name as value"], update={"is_group": True} ) From a0e8983199cf9c4f176377458025dafb6929b0e6 Mon Sep 17 00:00:00 2001 From: barredterra <14891507+barredterra@users.noreply.github.com> Date: Tue, 24 May 2022 19:23:45 +0200 Subject: [PATCH 087/340] feat: delete User Permission when User is deleted --- frappe/core/doctype/user/user.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/frappe/core/doctype/user/user.py b/frappe/core/doctype/user/user.py index 1ff5c98a91..1413e0449f 100644 --- a/frappe/core/doctype/user/user.py +++ b/frappe/core/doctype/user/user.py @@ -424,6 +424,9 @@ class User(Document): frappe.cache().delete_key("enabled_users") + # delete user permissions + frappe.db.delete("User Permission", {"user": self.name}) + def before_rename(self, old_name, new_name, merge=False): frappe.clear_cache(user=old_name) self.validate_rename(old_name, new_name) From a7c706672f4eb28bdaecb6109e042070db1da188 Mon Sep 17 00:00:00 2001 From: barredterra <14891507+barredterra@users.noreply.github.com> Date: Wed, 25 May 2022 01:12:48 +0200 Subject: [PATCH 088/340] fix: allow All to select a User --- frappe/core/doctype/user/user.json | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/frappe/core/doctype/user/user.json b/frappe/core/doctype/user/user.json index 42122ebfda..82e3fa71f3 100644 --- a/frappe/core/doctype/user/user.json +++ b/frappe/core/doctype/user/user.json @@ -722,7 +722,7 @@ "link_fieldname": "user" } ], - "modified": "2022-03-09 01:47:56.745069", + "modified": "2022-05-25 01:00:51.345319", "modified_by": "Administrator", "module": "Core", "name": "User", @@ -747,6 +747,10 @@ "read": 1, "role": "System Manager", "write": 1 + }, + { + "role": "All", + "select": 1 } ], "quick_entry": 1, From 00f665c634d670f0f71d9f5a0c76229b8decbfb6 Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Tue, 24 May 2022 15:07:19 +0530 Subject: [PATCH 089/340] fix: email queue cleanup in pure SQL Currently this background job constantly fails because of the way query is written: 1. It tries to find all docs to delete using select query 2. Deletes them by using `in query` with a HUGE amount of docs 3. Deletes child table with parent, again using `IN` query with huge amount of docs. This times out and never finishes on old sites. Solution: 1. Modified deletion to straightaway delete all main table rows that are older 2. Apply same deletion logic to child table rows. PS: This has potential to leave some orphan child table rows behind for few more days iff modified time was later than parent doc (this is quite rare). But it's safe since child table doesn't contain "links" anyway. --- .../doctype/email_queue/test_email_queue.py | 39 +++++++++++++++++-- frappe/email/queue.py | 24 +++++++----- 2 files changed, 50 insertions(+), 13 deletions(-) diff --git a/frappe/email/doctype/email_queue/test_email_queue.py b/frappe/email/doctype/email_queue/test_email_queue.py index b3c5467085..96c566a041 100644 --- a/frappe/email/doctype/email_queue/test_email_queue.py +++ b/frappe/email/doctype/email_queue/test_email_queue.py @@ -1,10 +1,41 @@ # -*- coding: utf-8 -*- # Copyright (c) 2015, Frappe Technologies and Contributors # License: MIT. See LICENSE -import unittest -# test_records = frappe.get_test_records('Email Queue') +import frappe +from frappe.email.queue import clear_outbox +from frappe.tests.utils import FrappeTestCase -class TestEmailQueue(unittest.TestCase): - pass +class TestEmailQueue(FrappeTestCase): + def test_email_queue_deletion_based_on_modified_date(self): + old_record = frappe.get_doc( + { + "doctype": "Email Queue", + "sender": "Test ", + "show_as_cc": "", + "message": "Test message", + "status": "Sent", + "priority": 1, + "recipients": [ + { + "recipient": "test_auth@test.com", + } + ], + } + ).insert() + + old_record.modified = "2010-01-01 00:00:01" + old_record.recipients[0].modified = old_record.modified + old_record.db_update_all() + + new_record = frappe.copy_doc(old_record) + new_record.insert() + + clear_outbox() + + self.assertFalse(frappe.db.exists("Email Queue", old_record.name)) + self.assertFalse(frappe.db.exists("Email Queue Recipient", {"parent": old_record.name})) + + self.assertTrue(frappe.db.exists("Email Queue", new_record.name)) + self.assertTrue(frappe.db.exists("Email Queue Recipient", {"parent": new_record.name})) diff --git a/frappe/email/queue.py b/frappe/email/queue.py index b92dea3e65..07731417d8 100755 --- a/frappe/email/queue.py +++ b/frappe/email/queue.py @@ -195,18 +195,24 @@ def clear_outbox(days: int = None) -> None: Note: Used separate query to avoid deadlock """ days = days or 31 - email_queue = DocType("Email Queue") + email_queue = frappe.qb.DocType("Email Queue") + email_recipient = frappe.qb.DocType("Email Queue Recipient") - email_queues = ( + # Delete queue table + ( frappe.qb.from_(email_queue) - .select(email_queue.name) - .where(email_queue.modified < (Now() - Interval(days=days))) - .run(pluck=True) - ) + .delete() + .where((email_queue.modified < (Now() - Interval(days=days)))) + ).run() - if email_queues: - frappe.db.delete("Email Queue", {"name": ("in", email_queues)}) - frappe.db.delete("Email Queue Recipient", {"parent": ("in", email_queues)}) + # delete child tables, note that this has potential to leave some orphan + # child table behind if modified time was later than parent doc (rare). + # But it's safe since child table doesn't contain links. + ( + frappe.qb.from_(email_recipient) + .delete() + .where((email_recipient.modified < (Now() - Interval(days=days)))) + ).run() def set_expiry_for_email_queue(): From 29710621d8675f9a45dddde5a0774787ebd8638a Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Tue, 24 May 2022 15:14:18 +0530 Subject: [PATCH 090/340] fix: commit after each log cleanup This preserves "progress" and releases locks held on log tables --- frappe/core/doctype/log_settings/log_settings.py | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/frappe/core/doctype/log_settings/log_settings.py b/frappe/core/doctype/log_settings/log_settings.py index 0fde168532..5632f05a36 100644 --- a/frappe/core/doctype/log_settings/log_settings.py +++ b/frappe/core/doctype/log_settings/log_settings.py @@ -10,8 +10,13 @@ from frappe.query_builder.functions import Now class LogSettings(Document): - def clear_logs(self): + def clear_logs(self, commit=False): self.clear_email_queue() + if commit: + # Since since deleting many logs can take significant amount of time, commit is required to relase locks. + # Error log table doesn't require commit - myisam + # activity logs are deleted last so background job finishes and commits. + frappe.db.commit() self.clear_error_logs() self.clear_activity_logs() @@ -34,7 +39,7 @@ class LogSettings(Document): def run_log_clean_up(): doc = frappe.get_doc("Log Settings") - doc.clear_logs() + doc.clear_logs(commit=True) @frappe.whitelist() From 3a05c0f0933c0b8012b58cf2a8b22566ab81b4e6 Mon Sep 17 00:00:00 2001 From: HENRY Florian Date: Wed, 25 May 2022 07:06:19 +0200 Subject: [PATCH 091/340] fix: update franch translation (#16974) remove plurals (as other translation) --- frappe/translations/fr.csv | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/frappe/translations/fr.csv b/frappe/translations/fr.csv index 06a2473d3b..d1dba64a44 100644 --- a/frappe/translations/fr.csv +++ b/frappe/translations/fr.csv @@ -799,9 +799,9 @@ DESC,DESC, Daily Event Digest is sent for Calendar Events where reminders are set.,Un Récapitulatif Quotidien est envoyé pour les Événements du Calendrier ayant des rappels., Danger,Danger, Dark Color,Couleur sombre, -Dashboard Chart,Tableau de bord, -Dashboard Chart Link,Lien de tableau de bord, -Dashboard Chart Source,Source du graphique du tableau de bord, +Dashboard Chart,Tableau de bord - indicateur, +Dashboard Chart Link,Lien de tableau de bord - indicateur, +Dashboard Chart Source,Source du graphique du tableau de bord - indicateur, Dashboard Name,Nom du tableau de bord, Dashboards,Tableaux de bord, Data,Données, From 1c864159f22c6c4ce05ae34daab0f5c6ce80d53a Mon Sep 17 00:00:00 2001 From: Rushabh Mehta Date: Wed, 25 May 2022 12:38:29 +0530 Subject: [PATCH 092/340] fix(minor): remove top margins for headings on standard templates --- frappe/website/doctype/web_form/templates/web_form.html | 2 +- .../web_template/section_with_cards/section_with_cards.html | 2 +- .../website/web_template/section_with_cta/section_with_cta.html | 2 +- .../section_with_features/section_with_features.html | 2 +- .../web_template/section_with_videos/section_with_videos.html | 2 +- .../split_section_with_image/split_section_with_image.html | 2 +- 6 files changed, 6 insertions(+), 6 deletions(-) diff --git a/frappe/website/doctype/web_form/templates/web_form.html b/frappe/website/doctype/web_form/templates/web_form.html index 7dd532421d..96072a19ea 100644 --- a/frappe/website/doctype/web_form/templates/web_form.html +++ b/frappe/website/doctype/web_form/templates/web_form.html @@ -22,7 +22,7 @@ data-web-form="{{ name }}" data-web-form-doctype="{{ doc_type }}" data-login-req {% if is_list %}
    -

    {{ _(title) }}

    +

    {{ _(title) }}

    diff --git a/frappe/website/web_template/section_with_cards/section_with_cards.html b/frappe/website/web_template/section_with_cards/section_with_cards.html index d0eb164288..b5577795a8 100644 --- a/frappe/website/web_template/section_with_cards/section_with_cards.html +++ b/frappe/website/web_template/section_with_cards/section_with_cards.html @@ -18,7 +18,7 @@ }) }} {%- endif -%}
    -

    {{ title or '' }}

    +

    {{ title or '' }}

    {{ content or '' }}

    diff --git a/frappe/website/web_template/section_with_cta/section_with_cta.html b/frappe/website/web_template/section_with_cta/section_with_cta.html index 4015c05517..80924d9396 100644 --- a/frappe/website/web_template/section_with_cta/section_with_cta.html +++ b/frappe/website/web_template/section_with_cta/section_with_cta.html @@ -1,6 +1,6 @@
    -

    {{ title }}

    +

    {{ title }}

    {%- if subtitle -%}

    {{ subtitle }}

    {%- endif -%} diff --git a/frappe/website/web_template/section_with_features/section_with_features.html b/frappe/website/web_template/section_with_features/section_with_features.html index b8cc3c2b90..b97106e32b 100644 --- a/frappe/website/web_template/section_with_features/section_with_features.html +++ b/frappe/website/web_template/section_with_features/section_with_features.html @@ -15,7 +15,7 @@ Icon for {{ feature.title }} {%- endif -%} {%- if feature.title -%} -

    {{ feature.title }}

    +

    {{ feature.title }}

    {%- endif -%} {%- if feature.content -%}

    {{ frappe.utils.md_to_html(feature.content) }}

    diff --git a/frappe/website/web_template/section_with_videos/section_with_videos.html b/frappe/website/web_template/section_with_videos/section_with_videos.html index 30bea5dead..304f77993c 100644 --- a/frappe/website/web_template/section_with_videos/section_with_videos.html +++ b/frappe/website/web_template/section_with_videos/section_with_videos.html @@ -13,7 +13,7 @@
    {%- if video.title -%} -

    {{ video.title }}

    +

    {{ video.title }}

    {%- endif -%} {%- if video.content -%}

    {{ video.content }}

    diff --git a/frappe/website/web_template/split_section_with_image/split_section_with_image.html b/frappe/website/web_template/split_section_with_image/split_section_with_image.html index 5ebeef3912..ced68ccf0a 100644 --- a/frappe/website/web_template/split_section_with_image/split_section_with_image.html +++ b/frappe/website/web_template/split_section_with_image/split_section_with_image.html @@ -15,7 +15,7 @@
    {%- endif -%}
    -

    {{ title }}

    +

    {{ title }}

    {%- if content -%}

    {{ content }}

    {%- endif -%} From 9ffb0db80c0680ebd35a566bd21e7c19483dd732 Mon Sep 17 00:00:00 2001 From: Byju Abraham <65185610+polycurve@users.noreply.github.com> Date: Wed, 25 May 2022 15:14:31 +0530 Subject: [PATCH 093/340] Fix: Change currency fraction for Singapore from Sen to Cent (#16987) --- frappe/geo/country_info.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frappe/geo/country_info.json b/frappe/geo/country_info.json index 735dcddac3..94d1f3ed37 100644 --- a/frappe/geo/country_info.json +++ b/frappe/geo/country_info.json @@ -2412,7 +2412,7 @@ "Singapore": { "code": "sg", "currency": "SGD", - "currency_fraction": "Sen", + "currency_fraction": "Cent", "currency_fraction_units": 100, "currency_name": "Singapore Dollar", "currency_symbol": "$", From e42fa8de056acb3c76b3f2096ee02758ea131f52 Mon Sep 17 00:00:00 2001 From: Ritwik Puri Date: Wed, 25 May 2022 15:18:08 +0530 Subject: [PATCH 094/340] test: fix newsletter tests setup due to duplicate entry of email group member for postgres (#16989) --- frappe/email/doctype/newsletter/test_newsletter.py | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/frappe/email/doctype/newsletter/test_newsletter.py b/frappe/email/doctype/newsletter/test_newsletter.py index 9ec61194ef..0cf388564f 100644 --- a/frappe/email/doctype/newsletter/test_newsletter.py +++ b/frappe/email/doctype/newsletter/test_newsletter.py @@ -1,7 +1,6 @@ # Copyright (c) 2021, Frappe Technologies Pvt. Ltd. and Contributors # MIT License. See LICENSE -import unittest from random import choice from typing import Union from unittest.mock import MagicMock, PropertyMock, patch @@ -17,9 +16,9 @@ from frappe.email.doctype.newsletter.newsletter import ( send_scheduled_email, ) from frappe.email.queue import flush +from frappe.tests.utils import FrappeTestCase from frappe.utils import add_days, getdate -test_dependencies = ["Email Group"] emails = [ "test_subscriber1@example.com", "test_subscriber2@example.com", @@ -63,6 +62,10 @@ class TestNewsletterMixin: for email in emails: doctype = "Email Group Member" email_filters = {"email": email, "email_group": "_Test Email Group"} + + savepoint = "setup_email_group" + frappe.db.savepoint(savepoint) + try: frappe.get_doc( { @@ -71,8 +74,11 @@ class TestNewsletterMixin: } ).insert() except Exception: + frappe.db.rollback(save_point=savepoint) frappe.db.update(doctype, email_filters, "unsubscribed", 0) + frappe.db.release_savepoint(savepoint) + def send_newsletter(self, published=0, schedule_send=None) -> Union[str, None]: frappe.db.delete("Email Queue") frappe.db.delete("Email Queue Recipient") @@ -127,7 +133,7 @@ class TestNewsletterMixin: return newsletter -class TestNewsletter(TestNewsletterMixin, unittest.TestCase): +class TestNewsletter(TestNewsletterMixin, FrappeTestCase): def test_send(self): self.send_newsletter() From c38219627c99a71e3627426e0622eaf0e26f5c2b Mon Sep 17 00:00:00 2001 From: Raffael Meyer <14891507+barredterra@users.noreply.github.com> Date: Wed, 25 May 2022 11:50:28 +0200 Subject: [PATCH 095/340] feat: Display button "Create User Email" only if current user can create an Email Account (#16979) --- frappe/core/doctype/user/user.js | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/frappe/core/doctype/user/user.js b/frappe/core/doctype/user/user.js index 77c199cdd4..6bbab0fdb3 100644 --- a/frappe/core/doctype/user/user.js +++ b/frappe/core/doctype/user/user.js @@ -194,14 +194,14 @@ frappe.ui.form.on('User', { } } } - if (frm.doc.user_emails){ - var found =0; - for (var i = 0;i Date: Wed, 25 May 2022 15:22:30 +0530 Subject: [PATCH 096/340] test: flaky test due to stale translation cache (#16985) --- frappe/tests/utils.py | 2 ++ .../test_personal_data_download_request.py | 6 +++--- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/frappe/tests/utils.py b/frappe/tests/utils.py index 7d00a0c1f9..fc26694d46 100644 --- a/frappe/tests/utils.py +++ b/frappe/tests/utils.py @@ -82,6 +82,8 @@ def _restore_thread_locals(flags): frappe.local.realtime_log = [] frappe.local.conf = frappe._dict(frappe.get_site_config()) frappe.local.cache = {} + frappe.local.lang = "en" + frappe.local.lang_full_dict = None @contextmanager diff --git a/frappe/website/doctype/personal_data_download_request/test_personal_data_download_request.py b/frappe/website/doctype/personal_data_download_request/test_personal_data_download_request.py index 4f115325df..6518cda5ed 100644 --- a/frappe/website/doctype/personal_data_download_request/test_personal_data_download_request.py +++ b/frappe/website/doctype/personal_data_download_request/test_personal_data_download_request.py @@ -2,17 +2,17 @@ # Copyright (c) 2019, Frappe Technologies and Contributors # License: MIT. See LICENSE import json -import unittest import frappe from frappe.contacts.doctype.contact.contact import get_contact_name from frappe.core.doctype.user.user import create_contact +from frappe.tests.utils import FrappeTestCase from frappe.website.doctype.personal_data_download_request.personal_data_download_request import ( get_user_data, ) -class TestRequestPersonalData(unittest.TestCase): +class TestRequestPersonalData(FrappeTestCase): def setUp(self): create_user_if_not_exists(email="test_privacy@example.com") @@ -48,7 +48,7 @@ class TestRequestPersonalData(unittest.TestCase): email_queue = frappe.get_all( "Email Queue", fields=["message"], order_by="creation DESC", limit=1 ) - self.assertTrue("Subject: Download Your Data" in email_queue[0].message) + self.assertIn(frappe._("Download Your Data"), email_queue[0].message) frappe.db.delete("Email Queue") From 8557cff2bb45b1c20a6beb4b36f6ff83e73fbc03 Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Sun, 22 May 2022 22:30:29 +0530 Subject: [PATCH 097/340] perf: faster auth ~ validate_ip_address from redis --- frappe/auth.py | 14 ++++++++++---- frappe/core/doctype/user/user.py | 12 ++++++++---- 2 files changed, 18 insertions(+), 8 deletions(-) diff --git a/frappe/auth.py b/frappe/auth.py index dc53c20f28..9bab8b8bf3 100644 --- a/frappe/auth.py +++ b/frappe/auth.py @@ -412,10 +412,16 @@ def clear_cookies(): def validate_ip_address(user): """check if IP Address is valid""" - user = ( - frappe.get_cached_doc("User", user) if not frappe.flags.in_test else frappe.get_doc("User", user) + from frappe.core.doctype.user.user import get_restricted_ip_list + + # Only fetch required fields - for perf + user_fields = ["restrict_ip", "bypass_restrict_ip_check_if_2fa_enabled"] + user_info = ( + frappe.get_cached_value("User", user, user_fields, as_dict=True) + if not frappe.flags.in_test + else frappe.db.get_value("User", user, user_fields, as_dict=True) ) - ip_list = user.get_restricted_ip_list() + ip_list = get_restricted_ip_list(user_info) if not ip_list: return @@ -430,7 +436,7 @@ def validate_ip_address(user): # check if two factor auth is enabled if system_settings.enable_two_factor_auth and not bypass_restrict_ip_check: # check if bypass restrict ip is enabled for login user - bypass_restrict_ip_check = user.bypass_restrict_ip_check_if_2fa_enabled + bypass_restrict_ip_check = user_info.bypass_restrict_ip_check_if_2fa_enabled for ip in ip_list: if frappe.local.request_ip.startswith(ip) or bypass_restrict_ip_check: diff --git a/frappe/core/doctype/user/user.py b/frappe/core/doctype/user/user.py index 1ff5c98a91..d9c67aaaaf 100644 --- a/frappe/core/doctype/user/user.py +++ b/frappe/core/doctype/user/user.py @@ -586,10 +586,7 @@ class User(Document): self.append("social_logins", social_logins) def get_restricted_ip_list(self): - if not self.restrict_ip: - return - - return [i.strip() for i in self.restrict_ip.split(",")] + return get_restricted_ip_list(self) @classmethod def find_by_credentials(cls, user_name: str, password: str, validate_password: bool = True): @@ -1156,6 +1153,13 @@ def create_contact(user, ignore_links=False, ignore_mandatory=False): contact.save(ignore_permissions=True) +def get_restricted_ip_list(user): + if not user.restrict_ip: + return + + return [i.strip() for i in user.restrict_ip.split(",")] + + @frappe.whitelist() def generate_keys(user): """ From 5c9421b750e9baf7f192ceb48763ab658c663ca2 Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Mon, 23 May 2022 11:26:39 +0530 Subject: [PATCH 098/340] perf: use redis cache for user_info --- frappe/auth.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frappe/auth.py b/frappe/auth.py index 9bab8b8bf3..80141d1d6c 100644 --- a/frappe/auth.py +++ b/frappe/auth.py @@ -165,7 +165,7 @@ class LoginManager: self.set_user_info() def get_user_info(self): - self.info = frappe.db.get_value( + self.info = frappe.get_cached_value( "User", self.user, ["user_type", "first_name", "last_name", "user_image"], as_dict=1 ) From e6460b293019bed6a1e6155f6be656314aaf44ea Mon Sep 17 00:00:00 2001 From: gavin Date: Wed, 25 May 2022 14:39:25 +0530 Subject: [PATCH 099/340] fix: Safe get_filter_value str replacing This would cause Custom Operator to stay in a broken state. Eg: ERPNext's Item list with while selecting filters as <"Created On", "Fiscal Year"> would not set correct option type or fetch options. Console would show the following error message: base_list.js:431 Uncaught TypeError: Cannot read properties of undefined (reading 'replace') at ListView.get_filter_value (base_list.js:431:22) at frappe.ui.Filter.set_field (filter.js:249:43) at HTMLSelectElement. (filter.js:122:9) at HTMLSelectElement.dispatch (jquery.js:5430:27) at HTMLSelectElement.elemData.handle (jquery.js:5234:28) --- frappe/public/js/frappe/list/base_list.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frappe/public/js/frappe/list/base_list.js b/frappe/public/js/frappe/list/base_list.js index f64ae39d1b..26d1668da0 100644 --- a/frappe/public/js/frappe/list/base_list.js +++ b/frappe/public/js/frappe/list/base_list.js @@ -428,7 +428,7 @@ frappe.views.BaseList = class BaseList { const filter = this.get_filters_for_args().filter(f => f[1] == fieldname)[0]; if (!filter) return; return { - 'like': filter[3].replace(/^%?|%$/g, ''), + 'like': filter[3]?.replace(/^%?|%$/g, ''), 'not set': null }[filter[2]] || filter[3]; } From c9504f3bfc93295fbb749083b97f050b4c5de9e1 Mon Sep 17 00:00:00 2001 From: gavin Date: Fri, 27 May 2022 14:24:21 +0530 Subject: [PATCH 100/340] perf: Make request for filters_config once Prior to this, a request was made on every filter change for a filters_config entry. The options would not change based on the List / filters context since no such data is shared with the API. Hence, it can be stored upon first request for re-use. Eg: Current usage in ERPNext - fiscal year operator's options don't change frequently. --- frappe/public/js/frappe/ui/filters/filter.js | 21 +++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-) diff --git a/frappe/public/js/frappe/ui/filters/filter.js b/frappe/public/js/frappe/ui/filters/filter.js index 4c21cacf90..0dce380c84 100644 --- a/frappe/public/js/frappe/ui/filters/filter.js +++ b/frappe/public/js/frappe/ui/filters/filter.js @@ -249,14 +249,21 @@ frappe.ui.Filter = class { const filter_value = this.filter_list.get_filter_value(fieldname); args[field_name] = filter_value; } - frappe - .xcall(this.filters_config[condition].get_field, args) - .then(field => { - df.fieldtype = field.fieldtype; - df.options = field.options; - df.fieldname = fieldname; - this.make_field(df, cur.fieldtype); + let setup_field = (field) => { + df.fieldtype = field.fieldtype; + df.options = field.options; + df.fieldname = fieldname; + this.make_field(df, cur.fieldtype); + } + if (this.filters_config[condition].data) { + let field = this.filters_config[condition].data; + setup_field(field); + } else { + frappe.xcall(this.filters_config[condition].get_field, args).then(field => { + this.filters_config[condition].data = field; + setup_field(field); }); + } } else { this.make_field(df, cur.fieldtype); } From 54ea02b5735f916df6f084c248703c4d78bab217 Mon Sep 17 00:00:00 2001 From: gavin Date: Fri, 27 May 2022 14:28:22 +0530 Subject: [PATCH 101/340] fix: Maintain filters from additional_filters_config on refresh Custom / Dynamic filters like "Fiscal Year" type would be in broken state upon Page refresh. This was because get_selected_value didn't handle that. Perf change: Don't call util to recompute value each time when value exists in current context. The UI feels a lot less clunkier after this change - reduced repeated seemingly unnecessary computations. --- frappe/public/js/frappe/ui/filters/filter.js | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/frappe/public/js/frappe/ui/filters/filter.js b/frappe/public/js/frappe/ui/filters/filter.js index 0dce380c84..a64fa6b6a9 100644 --- a/frappe/public/js/frappe/ui/filters/filter.js +++ b/frappe/public/js/frappe/ui/filters/filter.js @@ -318,7 +318,7 @@ frappe.ui.Filter = class { } get_selected_value() { - return this.utils.get_selected_value(this.field, this.get_condition()); + return this.value || this.utils.get_selected_value(this.field, this.get_condition()); } get_selected_label() { @@ -452,6 +452,8 @@ frappe.ui.filter_utils = { if (val) { val = val.split(',').map((v) => strip(v)); } + } else if(frappe.boot.additional_filters_config[condition]) { + val = field.value || val; } if (val === '%') { val = ''; From e50dfbc859830f2ca0fc2493ee0715014ebf1e3c Mon Sep 17 00:00:00 2001 From: gavin Date: Fri, 27 May 2022 14:35:14 +0530 Subject: [PATCH 102/340] refactor(minor): Consistent usage & styling --- frappe/public/js/frappe/list/base_list.js | 5 +---- frappe/public/js/frappe/list/list_sidebar.js | 1 - frappe/public/js/frappe/ui/filters/filter.js | 6 +++--- 3 files changed, 4 insertions(+), 8 deletions(-) diff --git a/frappe/public/js/frappe/list/base_list.js b/frappe/public/js/frappe/list/base_list.js index 26d1668da0..76fb782045 100644 --- a/frappe/public/js/frappe/list/base_list.js +++ b/frappe/public/js/frappe/list/base_list.js @@ -270,7 +270,6 @@ frappe.views.BaseList = class BaseList { doctype: this.doctype, stats: this.stats, parent: this.$page.find(".layout-side-section"), - // set_filter: this.set_filter.bind(this), page: this.page, list_view: this, }); @@ -620,9 +619,7 @@ class FilterArea { filters = [filter]; } - filters = filters.filter((f) => { - return !this.exists(f); - }); + filters = filters.filter(f => !this.exists(f)); const { non_standard_filters, promise } = this.set_standard_filter( filters diff --git a/frappe/public/js/frappe/list/list_sidebar.js b/frappe/public/js/frappe/list/list_sidebar.js index 28c6eaab0b..5feeb1928e 100644 --- a/frappe/public/js/frappe/list/list_sidebar.js +++ b/frappe/public/js/frappe/list/list_sidebar.js @@ -7,7 +7,6 @@ frappe.provide('frappe.views'); // stats = list of fields // doctype // parent -// set_filter = function called on click frappe.views.ListSidebar = class ListSidebar { constructor(opts) { diff --git a/frappe/public/js/frappe/ui/filters/filter.js b/frappe/public/js/frappe/ui/filters/filter.js index a64fa6b6a9..dac585be94 100644 --- a/frappe/public/js/frappe/ui/filters/filter.js +++ b/frappe/public/js/frappe/ui/filters/filter.js @@ -443,16 +443,16 @@ frappe.ui.filter_utils = { val = val == 'Yes' ? 1 : 0; } - if (condition.indexOf('like', 'not like') !== -1) { + if (['like', 'not like'].includes(condition)) { // automatically append wildcards if (val && !(val.startsWith('%') || val.endsWith('%'))) { val = '%' + val + '%'; } - } else if (in_list(['in', 'not in'], condition)) { + } else if (['in', 'not in'].includes(condition)) { if (val) { val = val.split(',').map((v) => strip(v)); } - } else if(frappe.boot.additional_filters_config[condition]) { + } else if (frappe.boot.additional_filters_config[condition]) { val = field.value || val; } if (val === '%') { From 96b30e714c19fa935187f344a46fd306cdcb21ce Mon Sep 17 00:00:00 2001 From: Faris Ansari Date: Fri, 27 May 2022 16:04:22 +0530 Subject: [PATCH 103/340] feat: table_field.fieldname field syntax for db_query --- frappe/desk/reportview.py | 13 +++++++++++-- frappe/model/db_query.py | 15 +++++++++++++++ frappe/tests/test_db_query.py | 16 ++++++++++++++++ 3 files changed, 42 insertions(+), 2 deletions(-) diff --git a/frappe/desk/reportview.py b/frappe/desk/reportview.py index b45f80f6ff..cf34e1e986 100644 --- a/frappe/desk/reportview.py +++ b/frappe/desk/reportview.py @@ -171,7 +171,7 @@ def raise_invalid_field(fieldname): def is_standard(fieldname): if "." in fieldname: - parenttype, fieldname = get_parenttype_and_fieldname(fieldname, None) + fieldname = fieldname.split(".")[1].strip("`") return ( fieldname in default_fields or fieldname in optional_fields or fieldname in child_table_fields ) @@ -235,7 +235,16 @@ def parse_json(data): def get_parenttype_and_fieldname(field, data): if "." in field: - parenttype, fieldname = field.split(".")[0][4:-1], field.split(".")[1].strip("`") + parts = field.split(".") + parenttype = parts[0] + fieldname = parts[1] + if parenttype.startswith("`tab"): + # `tabChild DocType`.`fieldname` + parenttype = parenttype[4:-1] + fieldname = fieldname.strip("`") + else: + # tablefield.fieldname + parenttype = frappe.get_meta(data.doctype).get_field(parenttype).options else: parenttype = data.doctype fieldname = field.strip("`") diff --git a/frappe/model/db_query.py b/frappe/model/db_query.py index 54331f5124..8b2475de52 100644 --- a/frappe/model/db_query.py +++ b/frappe/model/db_query.py @@ -287,6 +287,21 @@ class DatabaseQuery(object): # remove empty strings / nulls in fields self.fields = [f for f in self.fields if f] + # convert child_table.fieldname to `tabChild DocType`.`fieldname` + for field in self.fields: + if "." in field and "tab" not in field: + original_field = field + alias = None + if " as " in field: + field, alias = field.split(" as ") + tablefield, fieldname = field.split(".") + child_doctype = frappe.get_meta(self.doctype).get_field(tablefield).options + field = field.replace(tablefield, f"`tab{child_doctype}`") + field = field.replace(fieldname, f"`{fieldname}`") + if alias: + field = f"{field} as {alias}" + self.fields[self.fields.index(original_field)] = field + for filter_name in ["filters", "or_filters"]: filters = getattr(self, filter_name) if isinstance(filters, str): diff --git a/frappe/tests/test_db_query.py b/frappe/tests/test_db_query.py index dd67d68cd2..e2a32a020c 100644 --- a/frappe/tests/test_db_query.py +++ b/frappe/tests/test_db_query.py @@ -35,6 +35,22 @@ class TestReportview(unittest.TestCase): clear_custom_fields("DocType") + def test_child_table_field_syntax(self): + note = frappe.get_doc( + doctype="Note", + title=f"Test {frappe.utils.random_string(8)}", + content="test", + seen_by=[{"user": "Administrator"}], + ).insert() + result = frappe.db.get_all( + "Note", + filters={"name": note.name}, + fields=["name", "seen_by.user as seen_by"], + limit=1, + ) + self.assertEqual(result[0].seen_by, "Administrator") + note.delete() + def test_build_match_conditions(self): clear_user_permissions_for_doctype("Blog Post", "test2@example.com") From f416e2a1d2ddeaa314cc6b2ec31c1e5ec727e1dc Mon Sep 17 00:00:00 2001 From: gavin Date: Fri, 27 May 2022 16:36:37 +0530 Subject: [PATCH 104/340] feat(website-settings): Configurable Splash Image Configurable Splash Image option which will override app's splash_image hook set. --- .../doctype/website_settings/website_settings.json | 10 ++++++++-- .../doctype/website_settings/website_settings.py | 4 +++- 2 files changed, 11 insertions(+), 3 deletions(-) diff --git a/frappe/website/doctype/website_settings/website_settings.json b/frappe/website/doctype/website_settings/website_settings.json index b628437315..aa17fa261f 100644 --- a/frappe/website/doctype/website_settings/website_settings.json +++ b/frappe/website/doctype/website_settings/website_settings.json @@ -21,6 +21,7 @@ "website_theme_image_link", "brand", "banner_image", + "splash_image", "brand_html", "set_banner_from_image", "favicon", @@ -413,6 +414,11 @@ "fieldname": "footer_powered", "fieldtype": "Small Text", "label": "Footer \"Powered By\"" + }, + { + "fieldname": "splash_image", + "fieldtype": "Attach Image", + "label": "Splash Image" } ], "icon": "fa fa-cog", @@ -420,7 +426,7 @@ "index_web_pages_for_search": 1, "issingle": 1, "links": [], - "modified": "2022-03-09 01:47:31.094462", + "modified": "2022-05-27 12:33:29.019998", "modified_by": "Administrator", "module": "Website", "name": "Website Settings", @@ -445,4 +451,4 @@ "sort_order": "ASC", "states": [], "track_changes": 1 -} +} \ No newline at end of file diff --git a/frappe/website/doctype/website_settings/website_settings.py b/frappe/website/doctype/website_settings/website_settings.py index e8f15290c4..1042b97efa 100644 --- a/frappe/website/doctype/website_settings/website_settings.py +++ b/frappe/website/doctype/website_settings/website_settings.py @@ -136,7 +136,7 @@ def get_website_settings(context=None): } ) - settings = frappe.get_single("Website Settings") + settings: "WebsiteSettings" = frappe.get_single("Website Settings") for k in [ "banner_html", "banner_image", @@ -203,6 +203,8 @@ def get_website_settings(context=None): context["hide_login"] = settings.hide_login + context["splash_image"] = settings.splash_image or context["splash_image"] + return context From 6db103850105206e7272cfba877b90f900c77933 Mon Sep 17 00:00:00 2001 From: gavin Date: Fri, 27 May 2022 16:55:54 +0530 Subject: [PATCH 105/340] refactor(qb): Rename term subqry to SubQuery "SubQuery" conforms with terms naming in PyPika. Kept subqry that points to same def for maintaining APIs. --- frappe/query_builder/terms.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/frappe/query_builder/terms.py b/frappe/query_builder/terms.py index 8d64d2ddcd..64a4707983 100644 --- a/frappe/query_builder/terms.py +++ b/frappe/query_builder/terms.py @@ -100,7 +100,7 @@ class ParameterizedFunction(Function): return function_sql -class subqry(Criterion): +class SubQuery(Criterion): def __init__( self, subq: QueryBuilder, @@ -112,3 +112,6 @@ class subqry(Criterion): def get_sql(self, **kwg: Any) -> str: kwg["subquery"] = True return self.subq.get_sql(**kwg) + + +subqry = SubQuery From f355034e7c77d4aca4fa7c2aaaea09f067839ea6 Mon Sep 17 00:00:00 2001 From: gavin Date: Fri, 27 May 2022 17:00:57 +0530 Subject: [PATCH 106/340] refactor: get_group_by_count Refactored raw query and partial get_all building with db.query + qb notation equivalent. Added type hints & f-strings. Intent: This particular API (for field "assigned_to") was taking a while to run so decided to refactor this in hopes of perf improvemnts. Result: 50% reduction in response times :D --- frappe/desk/listview.py | 66 ++++++++++++++++++++++------------------- 1 file changed, 35 insertions(+), 31 deletions(-) diff --git a/frappe/desk/listview.py b/frappe/desk/listview.py index 88216d3998..bb1ac4fac4 100644 --- a/frappe/desk/listview.py +++ b/frappe/desk/listview.py @@ -1,6 +1,11 @@ -# Copyright (c) 2019, Frappe Technologies Pvt. Ltd. and Contributors +# Copyright (c) 2022, Frappe Technologies Pvt. Ltd. and Contributors # License: MIT. See LICENSE +from typing import Dict, List + import frappe +from frappe.query_builder.functions import Count +from frappe.query_builder.terms import subqry +from frappe.query_builder.utils import DocType @frappe.whitelist() @@ -24,37 +29,36 @@ def set_list_settings(doctype, values): @frappe.whitelist() -def get_group_by_count(doctype, current_filters, field): +def get_group_by_count(doctype: str, current_filters: str, field: str) -> List[Dict]: current_filters = frappe.parse_json(current_filters) - subquery_condition = "" - subquery = frappe.get_all(doctype, filters=current_filters, run=False) if field == "assigned_to": - subquery_condition = " and `tabToDo`.reference_name in ({subquery})".format(subquery=subquery) - return frappe.db.sql( - """select `tabToDo`.allocated_to as name, count(*) as count - from - `tabToDo`, `tabUser` - where - `tabToDo`.status!='Cancelled' and - `tabToDo`.allocated_to = `tabUser`.name and - `tabUser`.user_type = 'System User' - {subquery_condition} - group by - `tabToDo`.allocated_to - order by - count desc - limit 50""".format( - subquery_condition=subquery_condition - ), - as_dict=True, - ) - else: - return frappe.db.get_list( - doctype, - filters=current_filters, - group_by="`tab{0}`.{1}".format(doctype, field), - fields=["count(*) as count", "`{}` as name".format(field)], - order_by="count desc", - limit=50, + ToDo = DocType("ToDo") + User = DocType("User") + count = Count("*").as_("count") + filtered_records = frappe.db.query.build_conditions(doctype, current_filters).select("name") + + return ( + frappe.qb.from_(ToDo) + .from_(User) + .select(ToDo.allocated_to.as_("name"), count) + .where( + (ToDo.status != "Cancelled") + & (ToDo.allocated_to == User.name) + & (User.user_type == "System User") + & (ToDo.reference_name.isin(subqry(filtered_records))) + ) + .groupby(ToDo.allocated_to) + .orderby(count) + .limit(50) + .run(as_dict=True) ) + + return frappe.get_list( + doctype, + filters=current_filters, + group_by=f"`tab{doctype}`.{field}", + fields=["count(*) as count", f"`{field}` as name"], + order_by="count desc", + limit=50, + ) From e92ead1257245290774b74851de9012505b32fdc Mon Sep 17 00:00:00 2001 From: gavin Date: Fri, 27 May 2022 17:24:40 +0530 Subject: [PATCH 107/340] refactor: Re-write queries using SubQuery --- frappe/boot.py | 8 ++++---- frappe/utils/nestedset.py | 8 +++++--- 2 files changed, 9 insertions(+), 7 deletions(-) diff --git a/frappe/boot.py b/frappe/boot.py index a23a7e6ac3..6cd86dc4fc 100644 --- a/frappe/boot.py +++ b/frappe/boot.py @@ -15,7 +15,7 @@ from frappe.geo.country_info import get_all from frappe.model.base_document import get_controller from frappe.query_builder import DocType from frappe.query_builder.functions import Count -from frappe.query_builder.terms import subqry +from frappe.query_builder.terms import SubQuery from frappe.social.doctype.energy_point_log.energy_point_log import get_energy_points from frappe.social.doctype.energy_point_settings.energy_point_settings import ( is_energy_point_enabled, @@ -211,7 +211,7 @@ def get_user_pages_or_reports(parent, cache=False): if parent == "Report": has_role[p.name].update({"ref_doctype": p.ref_doctype}) - no_of_roles = ( + no_of_roles = SubQuery( frappe.qb.from_(hasRole).select(Count("*")).where(hasRole.parent == parentTable.name) ) @@ -221,7 +221,7 @@ def get_user_pages_or_reports(parent, cache=False): pages_with_no_roles = ( frappe.qb.from_(parentTable) .select(parentTable.name, parentTable.modified, *columns) - .where(subqry(no_of_roles) == 0) + .where(no_of_roles == 0) ).run(as_dict=True) for p in pages_with_no_roles: @@ -327,7 +327,7 @@ def get_unseen_notes(): (note.notify_on_every_login == 1) & (note.expire_notification_on > frappe.utils.now()) & ( - subqry(frappe.qb.from_(nsb).select(nsb.user).where(nsb.parent == note.name)).notin( + SubQuery(frappe.qb.from_(nsb).select(nsb.user).where(nsb.parent == note.name)).notin( [frappe.session.user] ) ) diff --git a/frappe/utils/nestedset.py b/frappe/utils/nestedset.py index d3067973ef..681cd6439d 100644 --- a/frappe/utils/nestedset.py +++ b/frappe/utils/nestedset.py @@ -17,6 +17,7 @@ from frappe import _ from frappe.model.document import Document from frappe.query_builder import Order from frappe.query_builder.functions import Coalesce, Max +from frappe.query_builder.terms import SubQuery from frappe.query_builder.utils import DocType @@ -336,14 +337,15 @@ class NestedSet(Document): def get_root_of(doctype): """Get root element of a DocType with a tree structure""" from frappe.query_builder.functions import Count - from frappe.query_builder.terms import subqry Table = DocType(doctype) t1 = Table.as_("t1") t2 = Table.as_("t2") - subq = frappe.qb.from_(t2).select(Count("*")).where((t2.lft < t1.lft) & (t2.rgt > t1.rgt)) - result = frappe.qb.from_(t1).select(t1.name).where((subqry(subq) == 0) & (t1.rgt > t1.lft)).run() + node_query = SubQuery( + frappe.qb.from_(t2).select(Count("*")).where((t2.lft < t1.lft) & (t2.rgt > t1.rgt)) + ) + result = frappe.qb.from_(t1).select(t1.name).where((node_query == 0) & (t1.rgt > t1.lft)).run() return result[0][0] if result else None From 8f245be46025360a19f952b22119220ab0321075 Mon Sep 17 00:00:00 2001 From: Shariq Ansari <30859809+shariquerik@users.noreply.github.com> Date: Fri, 27 May 2022 21:06:07 +0530 Subject: [PATCH 108/340] fix: Strip all spacing characters from Message-ID & In-Reply-To (#16999) --- frappe/core/doctype/communication/email.py | 3 ++- .../doctype/email_account/test_email_account.py | 2 +- frappe/email/doctype/email_queue/email_queue.py | 12 ++++++++++-- frappe/email/receive.py | 8 ++++++-- frappe/utils/data.py | 10 ++++++++++ 5 files changed, 29 insertions(+), 6 deletions(-) diff --git a/frappe/core/doctype/communication/email.py b/frappe/core/doctype/communication/email.py index 464bc35a1c..887037dca1 100755 --- a/frappe/core/doctype/communication/email.py +++ b/frappe/core/doctype/communication/email.py @@ -12,6 +12,7 @@ from frappe.utils import ( cint, get_datetime, get_formatted_email, + get_string_between, list_to_str, split_emails, validate_email_address, @@ -152,7 +153,7 @@ def _make( "reference_doctype": doctype, "reference_name": name, "email_template": email_template, - "message_id": get_message_id().strip(" <>"), + "message_id": get_string_between("<", get_message_id(), ">"), "read_receipt": read_receipt, "has_attachment": 1 if attachments else 0, "communication_type": communication_type, diff --git a/frappe/email/doctype/email_account/test_email_account.py b/frappe/email/doctype/email_account/test_email_account.py index a027a81bd7..537bf9eb7f 100644 --- a/frappe/email/doctype/email_account/test_email_account.py +++ b/frappe/email/doctype/email_account/test_email_account.py @@ -284,7 +284,7 @@ class TestEmailAccount(unittest.TestCase): messages = { # append_to = ToDo '"INBOX"': { - "latest_messages": [f.read().replace("{{ message_id }}", last_mail.message_id)], + "latest_messages": [f.read().replace("{{ message_id }}", "<" + last_mail.message_id + ">")], "seen_status": {2: "UNSEEN"}, "uid_list": [2], } diff --git a/frappe/email/doctype/email_queue/email_queue.py b/frappe/email/doctype/email_queue/email_queue.py index db2ca9e32b..662ba1b2ed 100644 --- a/frappe/email/doctype/email_queue/email_queue.py +++ b/frappe/email/doctype/email_queue/email_queue.py @@ -19,7 +19,15 @@ from frappe.email.email_body import add_attachment, get_email, get_formatted_htm from frappe.email.queue import get_unsubcribed_url, get_unsubscribe_message from frappe.model.document import Document from frappe.query_builder.utils import DocType -from frappe.utils import add_days, cint, cstr, get_hook_method, nowdate, split_emails +from frappe.utils import ( + add_days, + cint, + cstr, + get_hook_method, + get_string_between, + nowdate, + split_emails, +) MAX_RETRY_COUNT = 3 @@ -635,7 +643,7 @@ class QueueBuilder: d = { "priority": self.send_priority, "attachments": json.dumps(self.get_attachments()), - "message_id": mail.msg_root["Message-Id"].strip(" <>"), + "message_id": get_string_between("<", mail.msg_root["Message-Id"], ">"), "message": mail_to_string, "sender": self.sender, "reference_doctype": self.reference_doctype, diff --git a/frappe/email/receive.py b/frappe/email/receive.py index 80c413faa1..d39eaa5213 100644 --- a/frappe/email/receive.py +++ b/frappe/email/receive.py @@ -25,6 +25,7 @@ from frappe.utils import ( cstr, extract_email_id, get_datetime, + get_string_between, markdown, now, parse_addr, @@ -425,7 +426,9 @@ class Email: self.set_content_and_type() self.set_subject() self.set_from() - self.message_id = (self.mail.get("Message-ID") or "").strip(" <>") + + message_id = self.mail.get("Message-ID") or "" + self.message_id = get_string_between("<", message_id, ">") if self.mail["Date"]: try: @@ -441,7 +444,8 @@ class Email: @property def in_reply_to(self): - return (self.mail.get("In-Reply-To") or "").strip(" <>") + in_reply_to = self.mail.get("In-Reply-To") or "" + return get_string_between("<", in_reply_to, ">") def parse(self): """Walk and process multi-part email.""" diff --git a/frappe/utils/data.py b/frappe/utils/data.py index 6a9ffc81a6..bf030020ac 100644 --- a/frappe/utils/data.py +++ b/frappe/utils/data.py @@ -1887,6 +1887,16 @@ def strip(val: str, chars: Optional[str] = None) -> str: return (val or "").replace("\ufeff", "").replace("\u200b", "").strip(chars) +def get_string_between(start: str, string: str, end: str) -> str: + if not string: + return "" + + regex = "{0}(.*){1}".format(start, end) + out = re.search(regex, string) + + return out.group(1) if out else string + + def to_markdown(html: str) -> str: from html.parser import HTMLParser From 507a45d8f2ce20e57c1c25f16f312b9f8b0a43d3 Mon Sep 17 00:00:00 2001 From: Faris Ansari Date: Sat, 28 May 2022 13:57:44 +0530 Subject: [PATCH 109/340] fix: dot syntax support for link fields MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ✅ test --- frappe/model/db_query.py | 25 ++++++++++++++++++++----- frappe/tests/test_db_query.py | 13 +++++++++++++ 2 files changed, 33 insertions(+), 5 deletions(-) diff --git a/frappe/model/db_query.py b/frappe/model/db_query.py index 8b2475de52..dd7227df47 100644 --- a/frappe/model/db_query.py +++ b/frappe/model/db_query.py @@ -131,6 +131,7 @@ class DatabaseQuery(object): self.run = run self.strict = strict self.ignore_ddl = ignore_ddl + self.link_tables = [] # for contextual user permission check # to determine which user permission is applicable on link field of specific doctype @@ -216,6 +217,10 @@ class DatabaseQuery(object): parent_name = cast_name(f"{self.tables[0]}.name") args.tables += f" {self.join} {child} on ({child}.parent = {parent_name})" + # left join link tables + for link in self.link_tables: + args.tables += f" {self.join} `tab{link.doctype}` on (`tab{link.doctype}`.`name` = {self.tables[0]}.`{link.fieldname}`)" + if self.grouped_or_conditions: self.conditions.append(f"({' or '.join(self.grouped_or_conditions)})") @@ -294,9 +299,17 @@ class DatabaseQuery(object): alias = None if " as " in field: field, alias = field.split(" as ") - tablefield, fieldname = field.split(".") - child_doctype = frappe.get_meta(self.doctype).get_field(tablefield).options - field = field.replace(tablefield, f"`tab{child_doctype}`") + linked_fieldname, fieldname = field.split(".") + linked_field = frappe.get_meta(self.doctype).get_field(linked_fieldname) + linked_doctype = linked_field.options + if linked_field.fieldtype == "Link": + self.link_tables.append( + frappe._dict( + doctype=linked_doctype, fieldname=linked_fieldname, table_name=f"`tab{linked_doctype}`" + ) + ) + + field = field.replace(linked_fieldname, f"`tab{linked_doctype}`") field = field.replace(fieldname, f"`{fieldname}`") if alias: field = f"{field} as {alias}" @@ -411,7 +424,9 @@ class DatabaseQuery(object): table_name = table_name[13:] if not table_name[0] == "`": table_name = f"`{table_name}`" - if table_name not in self.tables: + if table_name not in self.tables and table_name not in ( + d.table_name for d in self.link_tables + ): self.append_table(table_name) def append_table(self, table_name): @@ -433,7 +448,7 @@ class DatabaseQuery(object): methods = ("count(", "avg(", "sum(", "extract(", "dayofyear(") return field.lower().startswith(methods) - if len(self.tables) > 1: + if len(self.tables) > 1 or len(self.link_tables) > 0: for idx, field in enumerate(self.fields): if "." not in field and not _in_standard_sql_methods(field): self.fields[idx] = f"{self.tables[0]}.{field}" diff --git a/frappe/tests/test_db_query.py b/frappe/tests/test_db_query.py index e2a32a020c..df518ce3cb 100644 --- a/frappe/tests/test_db_query.py +++ b/frappe/tests/test_db_query.py @@ -51,6 +51,19 @@ class TestReportview(unittest.TestCase): self.assertEqual(result[0].seen_by, "Administrator") note.delete() + def test_link_field_syntax(self): + todo = frappe.get_doc( + doctype="ToDo", description=f"Test ToDo", allocated_to="Administrator" + ).insert() + result = frappe.db.get_all( + "ToDo", + filters={"name": todo.name}, + fields=["name", "allocated_to.email as allocated_user_email"], + limit=1, + ) + self.assertEqual(result[0].allocated_user_email, "admin@example.com") + todo.delete() + def test_build_match_conditions(self): clear_user_permissions_for_doctype("Blog Post", "test2@example.com") From a0ecb912db1432faa5e1b8ddd99636cd6c2a574e Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Fri, 27 May 2022 19:17:12 +0530 Subject: [PATCH 110/340] fix!: dont delete customizations when doctypes are deleted If someone deletes doctype and restores it back all customization are lost, there's no "real" reason to delete all these customization. They are only ever active if the doctype is being used. Explanations: - Custom field: is used by meta when doctype meta is requested, if meta isn't requested then custom field is effectively inactive. - Client script: loaded by meta when doctype is requested by desk. So inactive in deleted state. - Property setter: loaded by meta, so inactive when doctype isn't present. - Report: will break doctype isn't present, but user should delete them manually to avoid loss of "scripts" or anything special they might have done. Also report's doctype don't 100% indicate that it's based solely on that doctype. --- frappe/core/doctype/doctype/test_doctype.py | 55 +++++++++++++++++++-- frappe/model/delete_doc.py | 4 -- 2 files changed, 52 insertions(+), 7 deletions(-) diff --git a/frappe/core/doctype/doctype/test_doctype.py b/frappe/core/doctype/doctype/test_doctype.py index 11f5ef8a69..0bcd972c68 100644 --- a/frappe/core/doctype/doctype/test_doctype.py +++ b/frappe/core/doctype/doctype/test_doctype.py @@ -1,10 +1,14 @@ # -*- coding: utf-8 -*- # Copyright (c) 2015, Frappe Technologies Pvt. Ltd. and Contributors # License: MIT. See LICENSE +import random +import string import unittest from typing import Dict, List, Optional +from unittest.mock import patch import frappe +from frappe.cache_manager import clear_doctype_cache from frappe.core.doctype.doctype.doctype import ( CannotIndexedError, DoctypeLinkError, @@ -15,8 +19,8 @@ from frappe.core.doctype.doctype.doctype import ( WrongOptionsDoctypeLinkError, validate_links_table_fieldnames, ) - -# test_records = frappe.get_test_records('DocType') +from frappe.custom.doctype.custom_field.custom_field import create_custom_fields +from frappe.desk.form.load import getdoc class TestDocType(unittest.TestCase): @@ -628,10 +632,55 @@ class TestDocType(unittest.TestCase): self.assertEqual(test_json.test_json_field["hello"], "world") + @patch.dict(frappe.conf, {"developer_mode": 1}) + def test_delete_doctype_with_customization(self): + from frappe.custom.doctype.property_setter.property_setter import make_property_setter + + custom_field = "customfield" + + doctype = new_doctype(custom=0).insert().name + + # Create property setter and custom field + field = "some_fieldname" + make_property_setter(doctype, field, "default", "DELETETHIS", "Data") + create_custom_fields({doctype: [{"fieldname": custom_field, "fieldtype": "Data"}]}) + + # Create 1 record + original_doc = frappe.get_doc(doctype=doctype, custom_field_name="wat").insert() + self.assertEqual(original_doc.some_fieldname, "DELETETHIS") + + # delete doctype + frappe.delete_doc("DocType", doctype) + clear_doctype_cache(doctype) + + # "restore" doctype by inserting doctype with same schema again + new_doctype(doctype, custom=0).insert() + + # Ensure basically same doctype getting "restored" + restored_doc = frappe.get_last_doc(doctype) + verify_fields = ["doctype", field, custom_field] + for f in verify_fields: + self.assertEqual(original_doc.get(f), restored_doc.get(f)) + + # Check form load of restored doctype + getdoc(doctype, restored_doc.name) + + # ensure meta - property setter + self.assertEqual(frappe.get_meta(doctype).get_field(field).default, "DELETETHIS") + frappe.delete_doc("DocType", doctype) + def new_doctype( - name, unique: bool = False, depends_on: str = "", fields: Optional[List[Dict]] = None, **kwargs + name: Optional[str] = None, + unique: bool = False, + depends_on: str = "", + fields: Optional[List[Dict]] = None, + **kwargs, ): + if not name: + # Test prefix is required to avoid coverage + name = "Test " + "".join(random.sample(string.ascii_lowercase, 10)) + doc = frappe.get_doc( { "doctype": "DocType", diff --git a/frappe/model/delete_doc.py b/frappe/model/delete_doc.py index 2eccc1e717..b53455134d 100644 --- a/frappe/model/delete_doc.py +++ b/frappe/model/delete_doc.py @@ -88,10 +88,6 @@ def delete_doc( update_flags(doc, flags, ignore_permissions) check_permission_and_not_submitted(doc) - frappe.db.delete("Custom Field", {"dt": name}) - frappe.db.delete("Client Script", {"dt": name}) - frappe.db.delete("Property Setter", {"doc_type": name}) - frappe.db.delete("Report", {"ref_doctype": name}) frappe.db.delete("Custom DocPerm", {"parent": name}) frappe.db.delete("__global_search", {"doctype": name}) From 076eb593e320a585a5cdf0880ff81c64d7833b6c Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Fri, 27 May 2022 19:07:56 +0530 Subject: [PATCH 111/340] refactor: delete_from_table Whole lot of unnecessary complexity, closure, multiple function calls, comprehensions all that can be replaced with single `get_all` --- frappe/model/delete_doc.py | 38 ++++++++++++-------------------------- 1 file changed, 12 insertions(+), 26 deletions(-) diff --git a/frappe/model/delete_doc.py b/frappe/model/delete_doc.py index b53455134d..a6d29ed190 100644 --- a/frappe/model/delete_doc.py +++ b/frappe/model/delete_doc.py @@ -3,6 +3,7 @@ import os import shutil +from typing import List import frappe import frappe.defaults @@ -188,39 +189,24 @@ def update_naming_series(doc): revert_series_if_last(doc.meta.autoname, doc.name, doc) -def delete_from_table(doctype, name, ignore_doctypes, doc): +def delete_from_table(doctype: str, name: str, ignore_doctypes: List[str], doc): if doctype != "DocType" and doctype == name: frappe.db.delete("Singles", {"doctype": name}) else: frappe.db.delete(doctype, {"name": name}) - # get child tables if doc: - tables = [d.options for d in doc.meta.get_table_fields()] - + child_doctypes = [d.options for d in doc.meta.get_table_fields()] else: + child_doctypes = frappe.get_all( + "DocField", + fields="options", + filters={"fieldtype": ["in", frappe.model.table_fields], "parent": doctype}, + pluck="options", + ) - def get_table_fields(field_doctype): - if field_doctype == "Custom Field": - return [] - - return [ - r[0] - for r in frappe.get_all( - field_doctype, - fields="options", - filters={"fieldtype": ["in", frappe.model.table_fields], "parent": doctype}, - as_list=1, - ) - ] - - tables = get_table_fields("DocField") - if not frappe.flags.in_install == "frappe": - tables += get_table_fields("Custom Field") - - # delete from child tables - for t in list(set(tables)): - if t not in ignore_doctypes: - frappe.db.delete(t, {"parenttype": doctype, "parent": name}) + child_doctypes_to_delete = set(child_doctypes) - set(ignore_doctypes) + for child_doctype in child_doctypes_to_delete: + frappe.db.delete(child_doctype, {"parenttype": doctype, "parent": name}) def update_flags(doc, flags=None, ignore_permissions=False): From 1e64abe9a56fdd8a33e478e5a2e641d97717640e Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Sun, 29 May 2022 11:27:12 +0530 Subject: [PATCH 112/340] fix!: meaningful error messages over `KeyError` --- frappe/__init__.py | 6 ++++-- frappe/modules/utils.py | 28 ++++++++++++++++++---------- 2 files changed, 22 insertions(+), 12 deletions(-) diff --git a/frappe/__init__.py b/frappe/__init__.py index 17a945c875..a5f4aafce4 100644 --- a/frappe/__init__.py +++ b/frappe/__init__.py @@ -1258,8 +1258,10 @@ def get_module_path(module, *joins): :param module: Module name. :param *joins: Join additional path elements using `os.path.join`.""" - module = scrub(module) - return get_pymodule_path(local.module_app[module] + "." + module, *joins) + from frappe.modules.utils import get_module_app + + app = get_module_app(module) + return get_pymodule_path(app + "." + scrub(module), *joins) def get_app_path(app_name, *joins): diff --git a/frappe/modules/utils.py b/frappe/modules/utils.py index 60eaefddc5..ad6a75e900 100644 --- a/frappe/modules/utils.py +++ b/frappe/modules/utils.py @@ -207,13 +207,18 @@ def export_doc(doctype, name, module=None): write_document_file(frappe.get_doc(doctype, name), module) -def get_doctype_module(doctype): +def get_doctype_module(doctype) -> str: """Returns **Module Def** name of given doctype.""" def make_modules_dict(): return dict(frappe.db.sql("select name, module from tabDocType")) - return frappe.cache().get_value("doctype_modules", make_modules_dict)[doctype] + doctype_module_map = frappe.cache().get_value("doctype_modules", make_modules_dict) + + if module_name := doctype_module_map.get(doctype): + return module_name + else: + frappe.throw(_("DocType {} not found").format(doctype), exc=frappe.DoesNotExistError) doctype_python_modules = {} @@ -234,9 +239,9 @@ def load_doctype_module(doctype, module=None, prefix="", suffix=""): if key not in doctype_python_modules: doctype_python_modules[key] = frappe.get_module(module_name) except ImportError as e: - raise ImportError( - "Module import failed for {0} ({1})".format(doctype, module_name + " Error: " + str(e)) - ) + msg = f"Module import failed for {doctype}, the DocType you're trying to open might be deleted." + msg += f"
    Error: {e}" + raise ImportError(msg) from e return doctype_python_modules[key] @@ -251,12 +256,15 @@ def get_module_name(doctype, module, prefix="", suffix="", app=None): ) -def get_module_app(module): - return frappe.local.module_app[scrub(module)] +def get_module_app(module: str) -> str: + app = frappe.local.module_app.get(scrub(module)) + if app is None: + frappe.throw(_("Module {} not found").format(module), exc=frappe.DoesNotExistError) + return app -def get_app_publisher(module): - app = frappe.local.module_app[scrub(module)] +def get_app_publisher(module: str) -> str: + app = get_module_app(module) if not app: frappe.throw(_("App not found")) app_publisher = frappe.get_hooks(hook="app_publisher", app_name=app)[0] @@ -321,7 +329,7 @@ def make_boilerplate(template, doc, opts=None): base_class=base_class, doctype=doc.name, **opts, - custom_controller=custom_controller + custom_controller=custom_controller, ) ) ) From 82cc98366d80fda6cd727a7fc4b51c66f1eab6ad Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Sun, 29 May 2022 12:57:39 +0530 Subject: [PATCH 113/340] fix: dont attempt to init singles that dont exist This is done during app install stage and causes unnecessary failures where stale doctypes are left in db. --- frappe/installer.py | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/frappe/installer.py b/frappe/installer.py index 5cd46e618d..c8373ff06f 100644 --- a/frappe/installer.py +++ b/frappe/installer.py @@ -473,13 +473,20 @@ def set_all_patches_as_completed(app): def init_singles(): - singles = [single["name"] for single in frappe.get_all("DocType", filters={"issingle": True})] + singles = frappe.get_all("DocType", filters={"issingle": True}, pluck="name") for single in singles: - if not frappe.db.get_singles_dict(single): + if frappe.db.get_singles_dict(single): + continue + + try: doc = frappe.new_doc(single) doc.flags.ignore_mandatory = True doc.flags.ignore_validate = True doc.save() + except ImportError: + # The doctype exists, but controller is deleted, + # no need to attempt to init such single, ref: #16917 + continue def make_conf( From c3918e8e347669fc9f2d4773304cec4594026dd6 Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Sun, 29 May 2022 16:11:07 +0530 Subject: [PATCH 114/340] test: test get_newargs --- frappe/tests/test_utils.py | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/frappe/tests/test_utils.py b/frappe/tests/test_utils.py index 04f9d16fd1..f4636fc026 100644 --- a/frappe/tests/test_utils.py +++ b/frappe/tests/test_utils.py @@ -615,3 +615,18 @@ class TestAppParser(unittest.TestCase): self.assertEqual("healthcare", parse_app_name("https://github.com/frappe/healthcare.git")) self.assertEqual("healthcare", parse_app_name("git@github.com:frappe/healthcare.git")) self.assertEqual("healthcare", parse_app_name("frappe/healthcare@develop")) + + +class TestIntrospectionMagic(unittest.TestCase): + """Test utils that inspect live objects""" + + def test_get_newargs(self): + def f(a, b=2, **args): + pass + + safe_kwargs = {"company": "Wind Power", "b": 1} + self.assertEqual(frappe.get_newargs(f, safe_kwargs), safe_kwargs) + + unsafe_args = dict(safe_kwargs) + unsafe_args.update({"ignore_permissions": True, "flags": {"ignore_mandatory": True}}) + self.assertEqual(frappe.get_newargs(f, unsafe_args), safe_kwargs) From 4085646495acd559ab16225cf998c7363f80d277 Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Sun, 29 May 2022 16:39:25 +0530 Subject: [PATCH 115/340] fix: identify varkw in get_newargs --- frappe/__init__.py | 16 ++++++++++++---- frappe/tests/test_utils.py | 11 +++++++++++ 2 files changed, 23 insertions(+), 4 deletions(-) diff --git a/frappe/__init__.py b/frappe/__init__.py index 17a945c875..a172c7412d 100644 --- a/frappe/__init__.py +++ b/frappe/__init__.py @@ -1501,18 +1501,26 @@ def call(fn, *args, **kwargs): def get_newargs(fn, kwargs): + + # if function has any **kwargs parameter that capture arbitrary keyword arguments + # Ref: https://docs.python.org/3/library/inspect.html#inspect.Parameter.kind + varkw_exist = False + if hasattr(fn, "fnargs"): fnargs = fn.fnargs else: signature = inspect.signature(fn) fnargs = list(signature.parameters) - varkw = "kwargs" in fnargs - if varkw: - fnargs.pop(-1) + + for param_name, parameter in signature.parameters.items(): + if parameter.kind == inspect.Parameter.VAR_KEYWORD: + varkw_exist = True + fnargs.remove(param_name) + break newargs = {} for a in kwargs: - if (a in fnargs) or varkw: + if (a in fnargs) or varkw_exist: newargs[a] = kwargs.get(a) newargs.pop("ignore_permissions", None) diff --git a/frappe/tests/test_utils.py b/frappe/tests/test_utils.py index f4636fc026..4f4fca8bbf 100644 --- a/frappe/tests/test_utils.py +++ b/frappe/tests/test_utils.py @@ -621,6 +621,7 @@ class TestIntrospectionMagic(unittest.TestCase): """Test utils that inspect live objects""" def test_get_newargs(self): + # `kwargs` is just convention any **varname should work. def f(a, b=2, **args): pass @@ -630,3 +631,13 @@ class TestIntrospectionMagic(unittest.TestCase): unsafe_args = dict(safe_kwargs) unsafe_args.update({"ignore_permissions": True, "flags": {"ignore_mandatory": True}}) self.assertEqual(frappe.get_newargs(f, unsafe_args), safe_kwargs) + + def test_strip_off_kwargs_when_not_supported(self): + def f(a, b=2): + pass + + args = {"company": "Wind Power", "b": 1} + self.assertEqual(frappe.get_newargs(f, args), {"b": 1}) + + # No args + self.assertEqual(frappe.get_newargs(lambda: None, args), {}) From 87ec6d4fb9c56f11795cbd9b7e5c4334a3dda7b6 Mon Sep 17 00:00:00 2001 From: Faris Ansari Date: Mon, 30 May 2022 10:41:08 +0530 Subject: [PATCH 116/340] fix: initialize link_tables in constructor --- frappe/model/db_query.py | 2 +- frappe/tests/test_db_query.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/frappe/model/db_query.py b/frappe/model/db_query.py index dd7227df47..3013483ca9 100644 --- a/frappe/model/db_query.py +++ b/frappe/model/db_query.py @@ -34,6 +34,7 @@ class DatabaseQuery(object): def __init__(self, doctype, user=None): self.doctype = doctype self.tables = [] + self.link_tables = [] self.conditions = [] self.or_conditions = [] self.fields = None @@ -131,7 +132,6 @@ class DatabaseQuery(object): self.run = run self.strict = strict self.ignore_ddl = ignore_ddl - self.link_tables = [] # for contextual user permission check # to determine which user permission is applicable on link field of specific doctype diff --git a/frappe/tests/test_db_query.py b/frappe/tests/test_db_query.py index df518ce3cb..c1b2e05266 100644 --- a/frappe/tests/test_db_query.py +++ b/frappe/tests/test_db_query.py @@ -53,7 +53,7 @@ class TestReportview(unittest.TestCase): def test_link_field_syntax(self): todo = frappe.get_doc( - doctype="ToDo", description=f"Test ToDo", allocated_to="Administrator" + doctype="ToDo", description="Test ToDo", allocated_to="Administrator" ).insert() result = frappe.db.get_all( "ToDo", From a22b22c70327f601239132e6f3fc1da13f787c53 Mon Sep 17 00:00:00 2001 From: Suraj Shetty Date: Mon, 30 May 2022 13:22:51 +0530 Subject: [PATCH 117/340] fix: Remove shareable link feature --- .../document_share_key.json | 2 +- .../system_settings/system_settings.json | 2 +- frappe/model/document.py | 8 +-- frappe/public/js/frappe/form/form.js | 4 -- frappe/public/js/frappe/form/sidebar/share.js | 56 ------------------- .../frappe/form/templates/form_sidebar.html | 7 --- frappe/public/scss/desk/sidebar.scss | 14 ----- 7 files changed, 6 insertions(+), 87 deletions(-) diff --git a/frappe/core/doctype/document_share_key/document_share_key.json b/frappe/core/doctype/document_share_key/document_share_key.json index 6142050c0c..b96fe09f0b 100644 --- a/frappe/core/doctype/document_share_key/document_share_key.json +++ b/frappe/core/doctype/document_share_key/document_share_key.json @@ -1,7 +1,7 @@ { "actions": [], "allow_rename": 1, - "autoname": "format:DOCUMENT-SHARE-KEY-{########}", + "autoname": "hash", "creation": "2022-01-14 13:40:49.487646", "doctype": "DocType", "editable_grid": 1, diff --git a/frappe/core/doctype/system_settings/system_settings.json b/frappe/core/doctype/system_settings/system_settings.json index 940704eb31..c954e41202 100644 --- a/frappe/core/doctype/system_settings/system_settings.json +++ b/frappe/core/doctype/system_settings/system_settings.json @@ -485,7 +485,7 @@ }, { "default": "30", - "description": "Number of days after which the shared document link will be expired", + "description": "Number of days after which the document Web View link shared on email will be expired", "fieldname": "document_share_key_expiry", "fieldtype": "Int", "label": "Document Share Key Expiry (in Days)" diff --git a/frappe/model/document.py b/frappe/model/document.py index 93d1115df0..0cd7b737b6 100644 --- a/frappe/model/document.py +++ b/frappe/model/document.py @@ -1383,20 +1383,20 @@ class Document(BaseDocument): """Returns signature (hash) for private URL.""" return hashlib.sha224(get_datetime_str(self.creation).encode()).hexdigest() - @frappe.whitelist() def get_document_share_key(self, expires_on=None, no_expiry=False): if no_expiry: expires_on = None - if document_key_exist := frappe.db.exists( + existing_key = frappe.db.exists( "Document Share Key", { "reference_doctype": self.doctype, "reference_docname": self.name, "expires_on": expires_on, }, - ): - doc = frappe.get_doc("Document Share Key", document_key_exist) + ) + if existing_key: + doc = frappe.get_doc("Document Share Key", existing_key) else: doc = frappe.new_doc("Document Share Key") doc.reference_doctype = self.doctype diff --git a/frappe/public/js/frappe/form/form.js b/frappe/public/js/frappe/form/form.js index 7ec13e410c..a997287a52 100644 --- a/frappe/public/js/frappe/form/form.js +++ b/frappe/public/js/frappe/form/form.js @@ -1177,10 +1177,6 @@ frappe.ui.form.Form = class FrappeForm { }); } - get_share_link(key) { - return frappe.urllib.get_full_url(`${this.doctype}/${this.docname}?key=${key}`); - } - copy_doc(onload, from_amend) { this.validate_form_action("Create"); var newdoc = frappe.model.copy_doc(this.doc, from_amend); diff --git a/frappe/public/js/frappe/form/sidebar/share.js b/frappe/public/js/frappe/form/sidebar/share.js index c1f8407c0c..8b73684475 100644 --- a/frappe/public/js/frappe/form/sidebar/share.js +++ b/frappe/public/js/frappe/form/sidebar/share.js @@ -7,7 +7,6 @@ frappe.ui.form.Share = class Share { constructor(opts) { $.extend(this, opts); this.shares = this.parent.find('.shares'); - this.share_link = this.parent.find('.share-link'); } refresh() { this.render_sidebar(); @@ -16,13 +15,6 @@ frappe.ui.form.Share = class Share { const shared = this.shared || this.frm.get_docinfo().shared; const shared_users = shared.filter(Boolean).map(s => s.user); - this.share_link.attr("title", __("Get Shareable Link")) - .tooltip({ delay: { "show": 600, "hide": 100 }}); - - this.share_link.click(() => { - this.share_modal(); - }); - if (this.frm.is_new()) { this.parent.find(".share-doc-btn").hide(); } @@ -195,52 +187,4 @@ frappe.ui.form.Share = class Share { }); }); } - async share_modal() { - const default_expiry = frappe.boot.sysdefaults.document_share_key_expiry; - const share_modal = new frappe.ui.Dialog({ - title: __("Share Link"), - fields: [{ - fieldname: "link_expiration_date", - label: __("Link Expiration Date"), - fieldtype: "Date", - default: frappe.datetime.add_days(moment(), default_expiry), - min_date: new Date(), - description: __("The default expiration date is {0} days from today.", [default_expiry]), - read_only_depends_on: "eval: doc.link" - }, { - fieldtype: "Check", - label: __("No Expiry"), - fieldname: "no_expiry", - change: () => { - const no_expiry_warning = ` - ${__('Note: Person with the link user will be able to see all changes to this document as long as the key is not deleted manually.')} - `; - const no_expiry = share_modal.get_value('no_expiry'); - share_modal.get_field('link_expiration_date').toggle(!no_expiry); - share_modal.get_field('no_expiry') - .set_description(`${no_expiry ? no_expiry_warning : ''}`); - }, - }, { - fieldtype: "Button", - label: __("Get Link"), - click: () => { - this.frm.call("get_document_share_key", { - expires_on: share_modal.get_value("link_expiration_date"), - no_expiry: share_modal.get_value("no_expiry") - }).then(res => { - let key = res.message; - share_modal.set_value("link", this.frm.get_share_link(key)); - }); - } - }, { - fieldname: "link", - label: __("Link"), - fieldtype: "Data", - depends_on: "eval: doc.link", - with_copy_button: true - }], - }); - - share_modal.show(); - } }; diff --git a/frappe/public/js/frappe/form/templates/form_sidebar.html b/frappe/public/js/frappe/form/templates/form_sidebar.html index c11913c7f0..dcea2f4647 100644 --- a/frappe/public/js/frappe/form/templates/form_sidebar.html +++ b/frappe/public/js/frappe/form/templates/form_sidebar.html @@ -87,13 +87,6 @@ -