From 497ea861f481c6a3c52fe2aed9d0df1b6c99e9d7 Mon Sep 17 00:00:00 2001 From: Sagar Vora Date: Tue, 30 Mar 2021 12:52:14 +0530 Subject: [PATCH 01/23] feat: frappe.whitelist for class methods --- frappe/__init__.py | 21 +++- .../doctype/auto_repeat/auto_repeat.py | 2 + frappe/core/doctype/report/report.py | 3 +- .../role_permission_for_page_and_report.py | 3 + .../doctype/customize_form/customize_form.py | 3 + .../data_migration_run/data_migration_run.py | 1 + frappe/desk/form/run_method.py | 81 ------------- frappe/desk/search.py | 5 +- frappe/email/doctype/newsletter/newsletter.py | 1 + frappe/handler.py | 111 ++++++++++++------ frappe/model/document.py | 18 +-- .../public/js/frappe/form/controls/button.js | 2 +- frappe/public/js/frappe/request.js | 2 +- .../energy_point_log/energy_point_log.py | 1 + frappe/website/doctype/blog_post/blog_post.py | 1 + .../portal_settings/portal_settings.py | 1 + .../doctype/website_theme/website_theme.py | 2 + 17 files changed, 123 insertions(+), 135 deletions(-) delete mode 100644 frappe/desk/form/run_method.py diff --git a/frappe/__init__.py b/frappe/__init__.py index 844a9238e3..c3667d9637 100644 --- a/frappe/__init__.py +++ b/frappe/__init__.py @@ -555,8 +555,13 @@ def whitelist(allow_guest=False, xss_safe=False, methods=None): def innerfn(fn): global whitelisted, guest_methods, xss_safe_methods, allowed_http_methods_for_whitelisted_func - whitelisted.append(fn) + # get function from the unbound / bound method + # this is needed because functions can be compared, but not methods + if hasattr(fn, '__func__'): + fn = fn.__func__ + + whitelisted.append(fn) allowed_http_methods_for_whitelisted_func[fn] = methods if allow_guest: @@ -569,6 +574,20 @@ def whitelist(allow_guest=False, xss_safe=False, methods=None): return innerfn +def is_whitelisted(method): + from frappe.utils import sanitize_html + + is_guest = session['user'] == 'Guest' + if method not in whitelisted or is_guest and method not in guest_methods: + throw(_("Not permitted"), PermissionError) + + if is_guest and method not in xss_safe_methods: + # strictly sanitize form_dict + # escapes html characters like <> except for predefined tags like a, b, ul etc. + for key, value in form_dict.items(): + if isinstance(value, string_types): + form_dict[key] = sanitize_html(value) + def read_only(): def innfn(fn): def wrapper_fn(*args, **kwargs): diff --git a/frappe/automation/doctype/auto_repeat/auto_repeat.py b/frappe/automation/doctype/auto_repeat/auto_repeat.py index 281e699640..bf05baf5b6 100644 --- a/frappe/automation/doctype/auto_repeat/auto_repeat.py +++ b/frappe/automation/doctype/auto_repeat/auto_repeat.py @@ -118,6 +118,7 @@ class AutoRepeat(Document): def is_completed(self): return self.end_date and getdate(self.end_date) < getdate(today()) + @frappe.whitelist() def get_auto_repeat_schedule(self): schedule_details = [] start_date = getdate(self.start_date) @@ -328,6 +329,7 @@ class AutoRepeat(Document): make(doctype=new_doc.doctype, name=new_doc.name, recipients=recipients, subject=subject, content=message, attachments=attachments, send_email=1) + @frappe.whitelist() def fetch_linked_contacts(self): if self.reference_doctype and self.reference_document: res = get_contacts_linking_to(self.reference_doctype, self.reference_document, fields=['email_id']) diff --git a/frappe/core/doctype/report/report.py b/frappe/core/doctype/report/report.py index 01c32bcb57..fb44e61cc8 100644 --- a/frappe/core/doctype/report/report.py +++ b/frappe/core/doctype/report/report.py @@ -58,6 +58,7 @@ class Report(Document): def get_columns(self): return [d.as_dict(no_default_fields = True) for d in self.columns] + @frappe.whitelist() def set_doctype_roles(self): if not self.get('roles') and self.is_standard == 'No': meta = frappe.get_meta(self.ref_doctype) @@ -304,7 +305,7 @@ class Report(Document): return data - @Document.whitelist + @frappe.whitelist() def toggle_disable(self, disable): self.db_set("disabled", cint(disable)) diff --git a/frappe/core/doctype/role_permission_for_page_and_report/role_permission_for_page_and_report.py b/frappe/core/doctype/role_permission_for_page_and_report/role_permission_for_page_and_report.py index f5081ef595..77b523987c 100644 --- a/frappe/core/doctype/role_permission_for_page_and_report/role_permission_for_page_and_report.py +++ b/frappe/core/doctype/role_permission_for_page_and_report/role_permission_for_page_and_report.py @@ -8,6 +8,7 @@ from frappe.core.doctype.report.report import is_prepared_report_disabled from frappe.model.document import Document class RolePermissionforPageandReport(Document): + @frappe.whitelist() def set_report_page_data(self): self.set_custom_roles() self.check_prepared_report_disabled() @@ -35,12 +36,14 @@ class RolePermissionforPageandReport(Document): doc = frappe.get_doc(doctype, docname) return doc.roles + @frappe.whitelist() def reset_roles(self): roles = self.get_standard_roles() self.set('roles', roles) self.update_custom_roles() self.update_disable_prepared_report() + @frappe.whitelist() def update_report_page_data(self): self.update_custom_roles() self.update_disable_prepared_report() diff --git a/frappe/custom/doctype/customize_form/customize_form.py b/frappe/custom/doctype/customize_form/customize_form.py index ad8d80e675..c79c965aae 100644 --- a/frappe/custom/doctype/customize_form/customize_form.py +++ b/frappe/custom/doctype/customize_form/customize_form.py @@ -24,6 +24,7 @@ class CustomizeForm(Document): frappe.db.sql("delete from tabSingles where doctype='Customize Form'") frappe.db.sql("delete from `tabCustomize Form Field`") + @frappe.whitelist() def fetch_to_customize(self): self.clear_existing_doc() if not self.doc_type: @@ -133,6 +134,7 @@ class CustomizeForm(Document): self.doc_type = doc_type self.name = "Customize Form" + @frappe.whitelist() def save_customization(self): if not self.doc_type: return @@ -448,6 +450,7 @@ class CustomizeForm(Document): self.flags.update_db = True + @frappe.whitelist() def reset_to_defaults(self): if not self.doc_type: return diff --git a/frappe/data_migration/doctype/data_migration_run/data_migration_run.py b/frappe/data_migration/doctype/data_migration_run/data_migration_run.py index 473acfb3d0..aed9c6cb1d 100644 --- a/frappe/data_migration/doctype/data_migration_run/data_migration_run.py +++ b/frappe/data_migration/doctype/data_migration_run/data_migration_run.py @@ -10,6 +10,7 @@ from frappe.utils import cstr from frappe.data_migration.doctype.data_migration_mapping.data_migration_mapping import get_source_value class DataMigrationRun(Document): + @frappe.whitelist() def run(self): self.begin() if self.total_pages > 0: diff --git a/frappe/desk/form/run_method.py b/frappe/desk/form/run_method.py deleted file mode 100644 index 7952f3b68d..0000000000 --- a/frappe/desk/form/run_method.py +++ /dev/null @@ -1,81 +0,0 @@ -# Copyright (c) 2015, Frappe Technologies Pvt. Ltd. and Contributors -# MIT License. See license.txt - -from __future__ import unicode_literals -import json, inspect -import frappe -from frappe import _ -from frappe.utils import cint -from six import text_type, string_types - -@frappe.whitelist() -def runserverobj(method, docs=None, dt=None, dn=None, arg=None, args=None): - """run controller method - old style""" - if not args: args = arg or "" - - if dt: # not called from a doctype (from a page) - if not dn: dn = dt # single - doc = frappe.get_doc(dt, dn) - - else: - doc = frappe.get_doc(json.loads(docs)) - doc._original_modified = doc.modified - doc.check_if_latest() - - if not doc.has_permission("read"): - frappe.msgprint(_("Not permitted"), raise_exception = True) - - if doc: - try: - args = json.loads(args) - except ValueError: - args = args - - try: - fnargs, varargs, varkw, defaults = inspect.getargspec(getattr(doc, method)) - except ValueError: - fnargs = inspect.getfullargspec(getattr(doc, method)).args - varargs = inspect.getfullargspec(getattr(doc, method)).varargs - varkw = inspect.getfullargspec(getattr(doc, method)).varkw - defaults = inspect.getfullargspec(getattr(doc, method)).defaults - - if not fnargs or (len(fnargs)==1 and fnargs[0]=="self"): - r = doc.run_method(method) - - elif "args" in fnargs or not isinstance(args, dict): - r = doc.run_method(method, args) - - else: - r = doc.run_method(method, **args) - - if r: - #build output as csv - if cint(frappe.form_dict.get('as_csv')): - make_csv_output(r, doc.doctype) - else: - frappe.response['message'] = r - - frappe.response.docs.append(doc) - -def make_csv_output(res, dt): - """send method response as downloadable CSV file""" - import frappe - - from six import StringIO - import csv - - f = StringIO() - writer = csv.writer(f) - for r in res: - row = [] - for v in r: - if isinstance(v, string_types): - v = v.encode("utf-8") - row.append(v) - writer.writerow(row) - - f.seek(0) - - frappe.response['result'] = text_type(f.read(), 'utf-8') - frappe.response['type'] = 'csv' - frappe.response['doctype'] = dt.replace(' ','') diff --git a/frappe/desk/search.py b/frappe/desk/search.py index 6faa827dde..6181261fc2 100644 --- a/frappe/desk/search.py +++ b/frappe/desk/search.py @@ -6,8 +6,7 @@ from __future__ import unicode_literals import frappe, json from frappe.utils import cstr, unique, cint from frappe.permissions import has_permission -from frappe.handler import is_whitelisted -from frappe import _ +from frappe import _, is_whitelisted from six import string_types import re import wrapt @@ -221,4 +220,4 @@ def validate_and_sanitize_search_inputs(fn, instance, args, kwargs): if kwargs['doctype'] and not frappe.db.exists('DocType', kwargs['doctype']): return [] - return fn(**kwargs) \ No newline at end of file + return fn(**kwargs) diff --git a/frappe/email/doctype/newsletter/newsletter.py b/frappe/email/doctype/newsletter/newsletter.py index ad985ee20e..c792347c09 100755 --- a/frappe/email/doctype/newsletter/newsletter.py +++ b/frappe/email/doctype/newsletter/newsletter.py @@ -29,6 +29,7 @@ class Newsletter(WebsiteGenerator): self.queue_all(test_email=True) frappe.msgprint(_("Test email sent to {0}").format(self.test_email_id)) + @frappe.whitelist() def send_emails(self): """send emails to leads and customers""" if self.email_sent: diff --git a/frappe/handler.py b/frappe/handler.py index cac9c3a460..44efdde2d4 100755 --- a/frappe/handler.py +++ b/frappe/handler.py @@ -2,17 +2,22 @@ # MIT License. See license.txt from __future__ import unicode_literals + +from werkzeug.wrappers import Response +from six import text_type, string_types, StringIO + import frappe -from frappe import _ import frappe.utils import frappe.sessions import frappe.desk.form.run_method -from frappe.utils.response import build_response -from frappe.api import validate_auth + from frappe.utils import cint +from frappe.api import validate_auth +from frappe import _, is_whitelisted +from frappe.utils.response import build_response +from frappe.utils.csvutils import build_csv_response from frappe.core.doctype.server_script.server_script_utils import run_server_script_api -from werkzeug.wrappers import Response -from six import string_types + ALLOWED_MIMETYPES = ('image/png', 'image/jpeg', 'application/pdf', 'application/msword', 'application/vnd.openxmlformats-officedocument.wordprocessingml.document', @@ -64,8 +69,9 @@ def execute_cmd(cmd, from_async=False): if from_async: method = method.queue - is_whitelisted(method) - is_valid_http_method(method) + if method != run_doc_method: + is_whitelisted(method) + is_valid_http_method(method) return frappe.call(method, **frappe.form_dict) @@ -75,31 +81,10 @@ def is_valid_http_method(method): if http_method not in frappe.allowed_http_methods_for_whitelisted_func[method]: frappe.throw(_("Not permitted"), frappe.PermissionError) -def is_whitelisted(method): - # check if whitelisted - if frappe.session['user'] == 'Guest': - if (method not in frappe.guest_methods): - frappe.throw(_("Not permitted"), frappe.PermissionError) - - if method not in frappe.xss_safe_methods: - # strictly sanitize form_dict - # escapes html characters like <> except for predefined tags like a, b, ul etc. - for key, value in frappe.form_dict.items(): - if isinstance(value, string_types): - frappe.form_dict[key] = frappe.utils.sanitize_html(value) - - else: - if not method in frappe.whitelisted: - frappe.throw(_("Not permitted"), frappe.PermissionError) - @frappe.whitelist(allow_guest=True) def version(): return frappe.__version__ -@frappe.whitelist() -def runserverobj(method, docs=None, dt=None, dn=None, arg=None, args=None): - frappe.desk.form.run_method.runserverobj(method, docs=docs, dt=dt, dn=dn, arg=arg, args=args) - @frappe.whitelist(allow_guest=True) def logout(): frappe.local.login_manager.logout() @@ -112,15 +97,6 @@ def web_logout(): frappe.respond_as_web_page(_("Logged Out"), _("You have been successfully logged out"), indicator_color='green') -@frappe.whitelist(allow_guest=True) -def run_custom_method(doctype, name, custom_method): - """cmd=run_custom_method&doctype={doctype}&name={name}&custom_method={custom_method}""" - doc = frappe.get_doc(doctype, name) - if getattr(doc, custom_method, frappe._dict()).is_whitelisted: - frappe.call(getattr(doc, custom_method), **frappe.local.form_dict) - else: - frappe.throw(_("Not permitted"), frappe.PermissionError) - @frappe.whitelist() def uploadfile(): ret = None @@ -222,6 +198,65 @@ def get_attr(cmd): frappe.log("method:" + cmd) return method -@frappe.whitelist(allow_guest = True) +@frappe.whitelist(allow_guest=True) def ping(): return "pong" + +@frappe.whitelist() +def run_doc_method(method, docs=None, dt=None, dn=None, arg=None, args=None): + """run controller method - old style""" + import json, inspect + + if not args: args = arg or "" + + if dt: # not called from a doctype (from a page) + if not dn: dn = dt # single + doc = frappe.get_doc(dt, dn) + + else: + doc = frappe.get_doc(json.loads(docs)) + doc._original_modified = doc.modified + doc.check_if_latest() + + if not doc.has_permission("read"): + frappe.msgprint(_("Not permitted"), raise_exception = True) + + if not doc: + return + + try: + args = json.loads(args) + except ValueError: + args = args + + method_obj = getattr(doc, method) + is_whitelisted(getattr(method_obj, '__func__', method_obj)) + + try: + fnargs = inspect.getargspec(method_obj)[0] + except ValueError: + fnargs = inspect.getfullargspec(method_obj).args + + if not fnargs or (len(fnargs)==1 and fnargs[0]=="self"): + r = doc.run_method(method) + + elif "args" in fnargs or not isinstance(args, dict): + r = doc.run_method(method, args) + + else: + r = doc.run_method(method, **args) + + frappe.response.docs.append(doc) + + if not r: + return + + # build output as csv + if cint(frappe.form_dict.get('as_csv')): + build_csv_response(r, doc.doctype.replace(' ', '')) + return + + frappe.response['message'] = r + +# for backwards compatibility +runserverobj = run_doc_method diff --git a/frappe/model/document.py b/frappe/model/document.py index 50025597c4..aaa8331d97 100644 --- a/frappe/model/document.py +++ b/frappe/model/document.py @@ -4,7 +4,7 @@ from __future__ import unicode_literals, print_function import frappe import time -from frappe import _, msgprint +from frappe import _, msgprint, is_whitelisted from frappe.utils import flt, cstr, now, get_datetime_str, file_lock, date_diff from frappe.model.base_document import BaseDocument, get_controller from frappe.model.naming import set_new_name @@ -126,10 +126,10 @@ class Document(BaseDocument): raise ValueError('Illegal arguments') @staticmethod - def whitelist(f): + def whitelist(fn): """Decorator: Whitelist method to be called remotely via REST API.""" - f.whitelisted = True - return f + frappe.whitelist()(fn) + return fn def reload(self): """Reload document from database""" @@ -1148,12 +1148,12 @@ class Document(BaseDocument): return composer - def is_whitelisted(self, method): - fn = getattr(self, method, None) + def is_whitelisted(self, method_name): + method = getattr(self, method_name, None) if not fn: - raise NotFound("Method {0} not found".format(method)) - elif not getattr(fn, "whitelisted", False): - raise Forbidden("Method {0} not whitelisted".format(method)) + raise NotFound("Method {0} not found".format(method_name)) + + is_whitelisted(getattr(method, '__func__', method)) def validate_value(self, fieldname, condition, val2, doc=None, raise_exception=None): """Check that value of fieldname should be 'condition' val2 diff --git a/frappe/public/js/frappe/form/controls/button.js b/frappe/public/js/frappe/form/controls/button.js index 28814531da..b44c9d9dcd 100644 --- a/frappe/public/js/frappe/form/controls/button.js +++ b/frappe/public/js/frappe/form/controls/button.js @@ -34,7 +34,7 @@ frappe.ui.form.ControlButton = frappe.ui.form.ControlData.extend({ var me = this; if(this.frm && this.frm.docname) { frappe.call({ - method: "runserverobj", + method: "run_doc_method", args: {'docs': this.frm.doc, 'method': this.df.options }, btn: this.$input, callback: function(r) { diff --git a/frappe/public/js/frappe/request.js b/frappe/public/js/frappe/request.js index 88912e12da..12fa9c8e21 100644 --- a/frappe/public/js/frappe/request.js +++ b/frappe/public/js/frappe/request.js @@ -55,7 +55,7 @@ frappe.call = function(opts) { args.cmd = opts.module+'.page.'+opts.page+'.'+opts.page+'.'+opts.method; } else if(opts.doc) { $.extend(args, { - cmd: "runserverobj", + cmd: "run_doc_method", docs: frappe.get_doc(opts.doc.doctype, opts.doc.name), method: opts.method, args: opts.args, diff --git a/frappe/social/doctype/energy_point_log/energy_point_log.py b/frappe/social/doctype/energy_point_log/energy_point_log.py index e9425cec86..2e2289aed4 100644 --- a/frappe/social/doctype/energy_point_log/energy_point_log.py +++ b/frappe/social/doctype/energy_point_log/energy_point_log.py @@ -52,6 +52,7 @@ class EnergyPointLog(Document): reference_log.reverted = 0 reference_log.save() + @frappe.whitelist() def revert(self, reason, ignore_permissions=False): if not ignore_permissions: frappe.only_for('System Manager') diff --git a/frappe/website/doctype/blog_post/blog_post.py b/frappe/website/doctype/blog_post/blog_post.py index 28549225be..bfccc0bbc7 100644 --- a/frappe/website/doctype/blog_post/blog_post.py +++ b/frappe/website/doctype/blog_post/blog_post.py @@ -18,6 +18,7 @@ class BlogPost(WebsiteGenerator): order_by = "published_on desc" ) + @frappe.whitelist() def make_route(self): if not self.route: return frappe.db.get_value('Blog Category', self.blog_category, diff --git a/frappe/website/doctype/portal_settings/portal_settings.py b/frappe/website/doctype/portal_settings/portal_settings.py index 5c1cee20fb..1bfbc70d60 100644 --- a/frappe/website/doctype/portal_settings/portal_settings.py +++ b/frappe/website/doctype/portal_settings/portal_settings.py @@ -19,6 +19,7 @@ class PortalSettings(Document): self.append('menu', item) return True + @frappe.whitelist() def reset(self): '''Restore defaults''' self.menu = [] diff --git a/frappe/website/doctype/website_theme/website_theme.py b/frappe/website/doctype/website_theme/website_theme.py index bb612c5d3e..d64077cccd 100644 --- a/frappe/website/doctype/website_theme/website_theme.py +++ b/frappe/website/doctype/website_theme/website_theme.py @@ -98,6 +98,7 @@ class WebsiteTheme(Document): else: self.generate_bootstrap_theme() + @frappe.whitelist() def set_as_default(self): self.generate_bootstrap_theme() self.save() @@ -106,6 +107,7 @@ class WebsiteTheme(Document): website_settings.ignore_validate = True website_settings.save() + @frappe.whitelist() def get_apps(self): from frappe.utils.change_log import get_versions apps = get_versions() From 08d88425d27b431d68783bb59d7db10a68584f08 Mon Sep 17 00:00:00 2001 From: Sagar Vora Date: Tue, 30 Mar 2021 13:14:11 +0530 Subject: [PATCH 02/23] fix: sider issues --- frappe/handler.py | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/frappe/handler.py b/frappe/handler.py index 44efdde2d4..ab90e9d164 100755 --- a/frappe/handler.py +++ b/frappe/handler.py @@ -4,7 +4,6 @@ from __future__ import unicode_literals from werkzeug.wrappers import Response -from six import text_type, string_types, StringIO import frappe import frappe.utils @@ -205,12 +204,15 @@ def ping(): @frappe.whitelist() def run_doc_method(method, docs=None, dt=None, dn=None, arg=None, args=None): """run controller method - old style""" - import json, inspect + import json + import inspect - if not args: args = arg or "" + if not args: + args = arg or "" if dt: # not called from a doctype (from a page) - if not dn: dn = dt # single + if not dn: + dn = dt # single doc = frappe.get_doc(dt, dn) else: From 40fa9e277ab5a99625e1143717c1c33a93a556d8 Mon Sep 17 00:00:00 2001 From: Sagar Vora Date: Tue, 30 Mar 2021 13:16:38 +0530 Subject: [PATCH 03/23] fix: remove deleted module from imports --- frappe/handler.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/frappe/handler.py b/frappe/handler.py index ab90e9d164..b545b0792c 100755 --- a/frappe/handler.py +++ b/frappe/handler.py @@ -8,8 +8,6 @@ from werkzeug.wrappers import Response import frappe import frappe.utils import frappe.sessions -import frappe.desk.form.run_method - from frappe.utils import cint from frappe.api import validate_auth from frappe import _, is_whitelisted From f1ed50a64ded138c77c091ddac7bf763e2432e03 Mon Sep 17 00:00:00 2001 From: Sagar Vora Date: Tue, 30 Mar 2021 13:28:55 +0530 Subject: [PATCH 04/23] fix: add valid_http_method validation for methods --- frappe/handler.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/frappe/handler.py b/frappe/handler.py index b545b0792c..0f5b3efc48 100755 --- a/frappe/handler.py +++ b/frappe/handler.py @@ -199,7 +199,7 @@ def get_attr(cmd): def ping(): return "pong" -@frappe.whitelist() + def run_doc_method(method, docs=None, dt=None, dn=None, arg=None, args=None): """run controller method - old style""" import json @@ -230,7 +230,9 @@ def run_doc_method(method, docs=None, dt=None, dn=None, arg=None, args=None): args = args method_obj = getattr(doc, method) - is_whitelisted(getattr(method_obj, '__func__', method_obj)) + fn = getattr(method_obj, '__func__', method_obj) + is_whitelisted(fn) + is_valid_http_method(fn) try: fnargs = inspect.getargspec(method_obj)[0] From aed90126f39353731849d1dc99f54584afd642f3 Mon Sep 17 00:00:00 2001 From: Sagar Vora Date: Tue, 30 Mar 2021 13:32:36 +0530 Subject: [PATCH 05/23] fix: incorrect condition --- frappe/model/document.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frappe/model/document.py b/frappe/model/document.py index aaa8331d97..68ad8c4f3f 100644 --- a/frappe/model/document.py +++ b/frappe/model/document.py @@ -1150,7 +1150,7 @@ class Document(BaseDocument): def is_whitelisted(self, method_name): method = getattr(self, method_name, None) - if not fn: + if not method: raise NotFound("Method {0} not found".format(method_name)) is_whitelisted(getattr(method, '__func__', method)) From 81b65545b753892aae4dce1d18c7fd4970219f11 Mon Sep 17 00:00:00 2001 From: Sagar Vora Date: Tue, 30 Mar 2021 13:37:14 +0530 Subject: [PATCH 06/23] fix: improved docstring for run_doc_method --- frappe/handler.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frappe/handler.py b/frappe/handler.py index 0f5b3efc48..c89749296c 100755 --- a/frappe/handler.py +++ b/frappe/handler.py @@ -201,7 +201,7 @@ def ping(): def run_doc_method(method, docs=None, dt=None, dn=None, arg=None, args=None): - """run controller method - old style""" + """run a whitelisted controller method""" import json import inspect From 1a9a13e4c2a5915852e1920cfff40090ea3726f0 Mon Sep 17 00:00:00 2001 From: Sagar Vora Date: Tue, 30 Mar 2021 13:47:39 +0530 Subject: [PATCH 07/23] fix: better code quality for run_doc_method --- frappe/handler.py | 25 ++++++++++++------------- 1 file changed, 12 insertions(+), 13 deletions(-) diff --git a/frappe/handler.py b/frappe/handler.py index c89749296c..c6ab45be1c 100755 --- a/frappe/handler.py +++ b/frappe/handler.py @@ -76,7 +76,10 @@ def is_valid_http_method(method): http_method = frappe.local.request.method if http_method not in frappe.allowed_http_methods_for_whitelisted_func[method]: - frappe.throw(_("Not permitted"), frappe.PermissionError) + throw_permission_error() + +def throw_permission_error(): + frappe.throw(_("Not permitted"), frappe.PermissionError) @frappe.whitelist(allow_guest=True) def version(): @@ -218,11 +221,8 @@ def run_doc_method(method, docs=None, dt=None, dn=None, arg=None, args=None): doc._original_modified = doc.modified doc.check_if_latest() - if not doc.has_permission("read"): - frappe.msgprint(_("Not permitted"), raise_exception = True) - - if not doc: - return + if not doc or not doc.has_permission("read"): + throw_permission_error() try: args = json.loads(args) @@ -240,25 +240,24 @@ def run_doc_method(method, docs=None, dt=None, dn=None, arg=None, args=None): fnargs = inspect.getfullargspec(method_obj).args if not fnargs or (len(fnargs)==1 and fnargs[0]=="self"): - r = doc.run_method(method) + response = doc.run_method(method) elif "args" in fnargs or not isinstance(args, dict): - r = doc.run_method(method, args) + response = doc.run_method(method, args) else: - r = doc.run_method(method, **args) + response = doc.run_method(method, **args) frappe.response.docs.append(doc) - - if not r: + if not response: return # build output as csv if cint(frappe.form_dict.get('as_csv')): - build_csv_response(r, doc.doctype.replace(' ', '')) + build_csv_response(response, doc.doctype.replace(' ', '')) return - frappe.response['message'] = r + frappe.response['message'] = response # for backwards compatibility runserverobj = run_doc_method From a6fce22c63fbc26eb28ac044b37456bb6f87fa67 Mon Sep 17 00:00:00 2001 From: "hasnain2808@gmail.com" Date: Tue, 30 Mar 2021 14:14:53 +0530 Subject: [PATCH 08/23] fix: filters in share url not working --- frappe/public/js/frappe/router.js | 2 ++ 1 file changed, 2 insertions(+) diff --git a/frappe/public/js/frappe/router.js b/frappe/public/js/frappe/router.js index c800f31d55..fd540fc3cc 100644 --- a/frappe/public/js/frappe/router.js +++ b/frappe/public/js/frappe/router.js @@ -386,6 +386,8 @@ frappe.router = { set_route_options_from_url(route) { // set query parameters as frappe.route_options var last_part = route[route.length - 1]; + // add routing v2 compatability + if (!last_part.includes("?")) last_part = route[route.length - 1] + window.location.search if (last_part.indexOf("?") < last_part.indexOf("=")) { // has ? followed by = let parts = last_part.split("?"); From e10c09c189e87c65dfbf464aba5fc24e4742d80e Mon Sep 17 00:00:00 2001 From: Mohammad Hasnain Mohsin Rajan Date: Tue, 30 Mar 2021 14:23:15 +0530 Subject: [PATCH 09/23] chore: add semicolon --- frappe/public/js/frappe/router.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frappe/public/js/frappe/router.js b/frappe/public/js/frappe/router.js index fd540fc3cc..35292846d0 100644 --- a/frappe/public/js/frappe/router.js +++ b/frappe/public/js/frappe/router.js @@ -387,7 +387,7 @@ frappe.router = { // set query parameters as frappe.route_options var last_part = route[route.length - 1]; // add routing v2 compatability - if (!last_part.includes("?")) last_part = route[route.length - 1] + window.location.search + if (!last_part.includes("?")) last_part = route[route.length - 1] + window.location.search; if (last_part.indexOf("?") < last_part.indexOf("=")) { // has ? followed by = let parts = last_part.split("?"); From 528a7b0f30e489a84791ce663292ae7b47278a2c Mon Sep 17 00:00:00 2001 From: Sagar Vora Date: Tue, 30 Mar 2021 15:07:47 +0530 Subject: [PATCH 10/23] test: add test to ensure run_doc_method works as expected --- frappe/tests/test_api.py | 3 +-- frappe/tests/test_client.py | 47 ++++++++++++++++++++++++++++++++++++- 2 files changed, 47 insertions(+), 3 deletions(-) diff --git a/frappe/tests/test_api.py b/frappe/tests/test_api.py index 01ad11dde3..6453062877 100644 --- a/frappe/tests/test_api.py +++ b/frappe/tests/test_api.py @@ -143,8 +143,7 @@ class TestAPI(unittest.TestCase): self.assertFalse(frappe.db.get_value('Note', {'title': 'delete'})) def test_auth_via_api_key_secret(self): - - # generate api ke and api secret for administrator + # generate API key and API secret for administrator keys = generate_keys("Administrator") frappe.db.commit() generated_secret = frappe.utils.password.get_decrypted_password( diff --git a/frappe/tests/test_client.py b/frappe/tests/test_client.py index 89460203f6..ff384298a4 100644 --- a/frappe/tests/test_client.py +++ b/frappe/tests/test_client.py @@ -2,7 +2,10 @@ from __future__ import unicode_literals -import unittest, frappe +import json +import unittest +import frappe + class TestClient(unittest.TestCase): def test_set_value(self): @@ -55,3 +58,45 @@ class TestClient(unittest.TestCase): }) self.assertRaises(frappe.PermissionError, execute_cmd, 'frappe.client.save') + + def test_run_doc_method(self): + from frappe.handler import execute_cmd + + if not frappe.db.exists('Report', 'Test Run Doc Method'): + report = frappe.get_doc({ + 'doctype': 'Report', + 'ref_doctype': 'User', + 'report_name': 'Test Run Doc Method', + 'report_type': 'Query Report', + 'is_standard': 'No', + 'roles': [ + {'role': 'System Manager'} + ] + }).insert() + else: + report = frappe.get_doc('Report', 'Test Run Doc Method') + + frappe.request = frappe._dict() + frappe.request.method = 'GET' + + # Whitelisted, works as expected + frappe.form_dict = frappe._dict({ + 'dt': report.doctype, + 'dn': report.name, + 'method': 'toggle_disable', + 'cmd': 'run_doc_method', + 'args': 0 + }) + + execute_cmd(frappe.form_dict.cmd) + + # Not whitelisted, throws permission error + frappe.form_dict = frappe._dict({ + 'dt': report.doctype, + 'dn': report.name, + 'method': 'create_report_py', + 'cmd': 'run_doc_method', + 'args': 0 + }) + + self.assertRaises(frappe.PermissionError, execute_cmd, frappe.form_dict.cmd) From 3324f005b7f52ba37a07506b738aa40570b80659 Mon Sep 17 00:00:00 2001 From: Sagar Vora Date: Tue, 30 Mar 2021 15:34:56 +0530 Subject: [PATCH 11/23] test: use frappe.local.attr instead of frappe.attr --- frappe/tests/test_client.py | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/frappe/tests/test_client.py b/frappe/tests/test_client.py index ff384298a4..8db57fc7c6 100644 --- a/frappe/tests/test_client.py +++ b/frappe/tests/test_client.py @@ -76,11 +76,11 @@ class TestClient(unittest.TestCase): else: report = frappe.get_doc('Report', 'Test Run Doc Method') - frappe.request = frappe._dict() - frappe.request.method = 'GET' + frappe.local.request = frappe._dict() + frappe.local.request.method = 'GET' # Whitelisted, works as expected - frappe.form_dict = frappe._dict({ + frappe.local.form_dict = frappe._dict({ 'dt': report.doctype, 'dn': report.name, 'method': 'toggle_disable', @@ -88,10 +88,10 @@ class TestClient(unittest.TestCase): 'args': 0 }) - execute_cmd(frappe.form_dict.cmd) + execute_cmd(frappe.local.form_dict.cmd) # Not whitelisted, throws permission error - frappe.form_dict = frappe._dict({ + frappe.local.form_dict = frappe._dict({ 'dt': report.doctype, 'dn': report.name, 'method': 'create_report_py', @@ -99,4 +99,8 @@ class TestClient(unittest.TestCase): 'args': 0 }) - self.assertRaises(frappe.PermissionError, execute_cmd, frappe.form_dict.cmd) + self.assertRaises( + frappe.PermissionError, + execute_cmd, + frappe.local.form_dict.cmd + ) From 6de2dfe2d98c4acc4479ec5b45904e0d98811233 Mon Sep 17 00:00:00 2001 From: Sagar Vora Date: Tue, 30 Mar 2021 15:35:32 +0530 Subject: [PATCH 12/23] test: remove unused import --- frappe/tests/test_client.py | 1 - 1 file changed, 1 deletion(-) diff --git a/frappe/tests/test_client.py b/frappe/tests/test_client.py index 8db57fc7c6..6be437601b 100644 --- a/frappe/tests/test_client.py +++ b/frappe/tests/test_client.py @@ -2,7 +2,6 @@ from __future__ import unicode_literals -import json import unittest import frappe From ada4ad7514e22e2297bb46cdf62b40a12d57207b Mon Sep 17 00:00:00 2001 From: Sagar Vora Date: Tue, 30 Mar 2021 18:31:01 +0530 Subject: [PATCH 13/23] chore: add whitelist decorator to missed methods --- frappe/core/doctype/data_import/data_import.py | 1 + frappe/integrations/doctype/connected_app/connected_app.py | 1 + frappe/integrations/doctype/social_login_key/social_login_key.py | 1 + .../personal_data_deletion_request.py | 1 + 4 files changed, 4 insertions(+) diff --git a/frappe/core/doctype/data_import/data_import.py b/frappe/core/doctype/data_import/data_import.py index a9761c3430..1c56f54303 100644 --- a/frappe/core/doctype/data_import/data_import.py +++ b/frappe/core/doctype/data_import/data_import.py @@ -38,6 +38,7 @@ class DataImport(Document): return validate_google_sheets_url(self.google_sheets_url) + @frappe.whitelist() def get_preview_from_template(self, import_file=None, google_sheets_url=None): if import_file: self.import_file = import_file diff --git a/frappe/integrations/doctype/connected_app/connected_app.py b/frappe/integrations/doctype/connected_app/connected_app.py index ec08f8e4be..95077ece77 100644 --- a/frappe/integrations/doctype/connected_app/connected_app.py +++ b/frappe/integrations/doctype/connected_app/connected_app.py @@ -44,6 +44,7 @@ class ConnectedApp(Document): scope=self.get_scopes() ) + @frappe.whitelist() def initiate_web_application_flow(self, user=None, success_uri=None): """Return an authorization URL for the user. Save state in Token Cache.""" user = user or frappe.session.user diff --git a/frappe/integrations/doctype/social_login_key/social_login_key.py b/frappe/integrations/doctype/social_login_key/social_login_key.py index d84e6ef11d..dffb730513 100644 --- a/frappe/integrations/doctype/social_login_key/social_login_key.py +++ b/frappe/integrations/doctype/social_login_key/social_login_key.py @@ -49,6 +49,7 @@ class SocialLoginKey(Document): icon_file = icon_map[self.provider_name] self.icon = '/assets/frappe/icons/social/{0}'.format(icon_file) + @frappe.whitelist() def get_social_login_provider(self, provider, initialize=False): providers = {} diff --git a/frappe/website/doctype/personal_data_deletion_request/personal_data_deletion_request.py b/frappe/website/doctype/personal_data_deletion_request/personal_data_deletion_request.py index 8faba41df3..23857a5e66 100644 --- a/frappe/website/doctype/personal_data_deletion_request/personal_data_deletion_request.py +++ b/frappe/website/doctype/personal_data_deletion_request/personal_data_deletion_request.py @@ -101,6 +101,7 @@ class PersonalDataDeletionRequest(Document): if self.status != "Pending Approval": frappe.throw(_("This request has not yet been approved by the user.")) + @frappe.whitelist() def trigger_data_deletion(self): """Redact user data defined in current site's hooks under `user_data_fields`""" self.validate_data_anonymization() From e2a3e0f53a0d40e2f5ce59b1a32abfc7cbf051df Mon Sep 17 00:00:00 2001 From: Sagar Vora Date: Tue, 30 Mar 2021 20:00:05 +0530 Subject: [PATCH 14/23] fix: return method if method is being passed --- frappe/__init__.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/frappe/__init__.py b/frappe/__init__.py index c3667d9637..11c1f324ad 100644 --- a/frappe/__init__.py +++ b/frappe/__init__.py @@ -558,8 +558,10 @@ def whitelist(allow_guest=False, xss_safe=False, methods=None): # get function from the unbound / bound method # this is needed because functions can be compared, but not methods + method = None if hasattr(fn, '__func__'): - fn = fn.__func__ + method = fn + fn = method.__func__ whitelisted.append(fn) allowed_http_methods_for_whitelisted_func[fn] = methods @@ -570,7 +572,7 @@ def whitelist(allow_guest=False, xss_safe=False, methods=None): if xss_safe: xss_safe_methods.append(fn) - return fn + return method or fn return innerfn From 2227b910d3a18f47f2971fcbec894394c215c0c9 Mon Sep 17 00:00:00 2001 From: leela Date: Tue, 30 Mar 2021 20:53:56 +0530 Subject: [PATCH 15/23] fix: Make authentication check mandatory even in case of 2FA --- frappe/auth.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/frappe/auth.py b/frappe/auth.py index 946a8c52d5..cdc68e6822 100644 --- a/frappe/auth.py +++ b/frappe/auth.py @@ -215,9 +215,7 @@ class LoginManager: if not (user and pwd): self.fail(_('Incomplete login details'), user=user) - # Ignore password check if tmp_id is set, 2FA takes care of authentication. - validate_password = not bool(frappe.form_dict.get('tmp_id')) - user = User.find_by_credentials(user, pwd, validate_password=validate_password) + user = User.find_by_credentials(user, pwd) if not user: self.fail('Invalid login credentials') From 9200192c1cbb2125894aa59150f361d031d55afb Mon Sep 17 00:00:00 2001 From: leela Date: Tue, 30 Mar 2021 12:04:09 +0530 Subject: [PATCH 16/23] refactor: Cleanup name confusion Using `delete_session` name for a function and also as a method name is confusing. Cleaned that up. --- frappe/sessions.py | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/frappe/sessions.py b/frappe/sessions.py index 3babf1db12..5f13dfb7af 100644 --- a/frappe/sessions.py +++ b/frappe/sessions.py @@ -296,8 +296,7 @@ class Session: expiry = get_expiry_in_seconds(session_data.get("session_expiry")) if self.time_diff > expiry: - print('deleting...') - self.delete_session() + self._delete_session() data = None return data and data.data @@ -316,12 +315,12 @@ class Session: data = frappe._dict(eval(rec and rec[0][1] or '{}')) data.user = rec[0][0] else: - self.delete_session() + self._delete_session() data = None return data - def delete_session(self): + def _delete_session(self): delete_session(self.sid, reason="Session Expired") def start_as_guest(self): From 1f6f02fd5a7f80842094ce43fefc0e4bb70260d7 Mon Sep 17 00:00:00 2001 From: leela Date: Wed, 31 Mar 2021 09:09:28 +0530 Subject: [PATCH 17/23] fix: Track 2FA OTP attempts using login tracker --- frappe/auth.py | 45 ++++++++++++++++++++------------ frappe/core/doctype/user/user.py | 2 +- frappe/twofactor.py | 8 ++++++ frappe/utils/password.py | 8 ++++-- 4 files changed, 44 insertions(+), 19 deletions(-) diff --git a/frappe/auth.py b/frappe/auth.py index cdc68e6822..32deb1bb48 100644 --- a/frappe/auth.py +++ b/frappe/auth.py @@ -220,28 +220,20 @@ class LoginManager: if not user: self.fail('Invalid login credentials') - sys_settings = frappe.get_doc("System Settings") - track_login_attempts = (sys_settings.allow_consecutive_login_attempts >0) - - tracker_kwargs = {} - if track_login_attempts: - tracker_kwargs['lock_interval'] = sys_settings.allow_login_after_fail - tracker_kwargs['max_consecutive_login_attempts'] = sys_settings.allow_consecutive_login_attempts - - tracker = LoginAttemptTracker(user.name, **tracker_kwargs) - - if track_login_attempts and not tracker.is_user_allowed(): - frappe.throw(_("Your account has been locked and will resume after {0} seconds") - .format(sys_settings.allow_login_after_fail), frappe.SecurityException) + # Current login flow uses cached credentials for authentication while checking OTP. + # Incase of OTP check, tracker for auth needs to be disabled(If not, it can remove tracker history as it is going to succeed anyway) + # Tracker is activated for 2FA incase of OTP. + ignore_tracker = should_run_2fa(user.name) and ('otp' in frappe.form_dict) + tracker = None if ignore_tracker else get_login_attempt_tracker(user.name) if not user.is_authenticated: - tracker.add_failure_attempt() + tracker and tracker.add_failure_attempt() self.fail('Invalid login credentials', user=user.name) elif not (user.name == 'Administrator' or user.enabled): - tracker.add_failure_attempt() + tracker and tracker.add_failure_attempt() self.fail('User disabled or missing', user=user.name) else: - tracker.add_success_attempt() + tracker and tracker.add_success_attempt() self.user = user.name def force_user_to_reset_password(self): @@ -404,6 +396,27 @@ def validate_ip_address(user): frappe.throw(_("Access not allowed from this IP Address"), frappe.AuthenticationError) +def get_login_attempt_tracker(user_name: str, raise_locked_exception: bool=True): + """Get login attempt tracker instance. + + :param user_name: Name of the loggedin user + :param raise_locked_exception: If set, raises an exception incase of user not allowed to login + """ + sys_settings = frappe.get_doc("System Settings") + track_login_attempts = (sys_settings.allow_consecutive_login_attempts >0) + tracker_kwargs = {} + + if track_login_attempts: + tracker_kwargs['lock_interval'] = sys_settings.allow_login_after_fail + tracker_kwargs['max_consecutive_login_attempts'] = sys_settings.allow_consecutive_login_attempts + + tracker = LoginAttemptTracker(user_name, **tracker_kwargs) + + if raise_locked_exception and track_login_attempts and not tracker.is_user_allowed(): + frappe.throw(_("Your account has been locked and will resume after {0} seconds") + .format(sys_settings.allow_login_after_fail), frappe.SecurityException) + return tracker + class LoginAttemptTracker(object): """Track login attemts of a user. diff --git a/frappe/core/doctype/user/user.py b/frappe/core/doctype/user/user.py index c7bc6c43c0..64ac7f5bca 100644 --- a/frappe/core/doctype/user/user.py +++ b/frappe/core/doctype/user/user.py @@ -558,7 +558,7 @@ class User(Document): user['is_authenticated'] = True if validate_password: try: - check_password(user['name'], password) + check_password(user['name'], password, delete_tracker_cache=False) except frappe.AuthenticationError: user['is_authenticated'] = False diff --git a/frappe/twofactor.py b/frappe/twofactor.py index 7ee3b510ba..0a120d5287 100644 --- a/frappe/twofactor.py +++ b/frappe/twofactor.py @@ -118,6 +118,7 @@ def get_verification_method(): def confirm_otp_token(login_manager, otp=None, tmp_id=None): '''Confirm otp matches.''' + from frappe.auth import get_login_attempt_tracker if not otp: otp = frappe.form_dict.get('otp') if not otp: @@ -130,12 +131,17 @@ def confirm_otp_token(login_manager, otp=None, tmp_id=None): otp_secret = frappe.cache().get(tmp_id + '_otp_secret') if not otp_secret: raise ExpiredLoginException(_('Login session expired, refresh page to retry')) + + tracker = get_login_attempt_tracker(login_manager.user) + hotp = pyotp.HOTP(otp_secret) if hotp_token: if hotp.verify(otp, int(hotp_token)): frappe.cache().delete(tmp_id + '_token') + tracker.add_success_attempt() return True else: + tracker.add_failure_attempt() login_manager.fail(_('Incorrect Verification code'), login_manager.user) totp = pyotp.TOTP(otp_secret) @@ -144,8 +150,10 @@ def confirm_otp_token(login_manager, otp=None, tmp_id=None): if not frappe.db.get_default(login_manager.user + '_otplogin'): frappe.db.set_default(login_manager.user + '_otplogin', 1) delete_qrimage(login_manager.user) + tracker.add_success_attempt() return True else: + tracker.add_failure_attempt() login_manager.fail(_('Incorrect Verification code'), login_manager.user) diff --git a/frappe/utils/password.py b/frappe/utils/password.py index 19a538f703..6a63fc39d7 100644 --- a/frappe/utils/password.py +++ b/frappe/utils/password.py @@ -65,7 +65,7 @@ def set_encrypted_password(doctype, name, pwd, fieldname='password'): raise e -def check_password(user, pwd, doctype='User', fieldname='password'): +def check_password(user, pwd, doctype='User', fieldname='password', delete_tracker_cache=True): '''Checks if user and password are correct, else raises frappe.AuthenticationError''' auth = frappe.db.sql("""select `name`, `password` from `__Auth` @@ -77,7 +77,11 @@ def check_password(user, pwd, doctype='User', fieldname='password'): # lettercase agnostic user = auth[0].name - delete_login_failed_cache(user) + + # TODO: This need to be deleted after checking side effects of it. + # We have a `LoginAttemptTracker` that can take care of tracking related cache. + if delete_tracker_cache: + delete_login_failed_cache(user) if not passlibctx.needs_update(auth[0].password): update_password(user, pwd, doctype, fieldname) From 71ecb288c72ea7a78c9013f3efa92c6dd80173bd Mon Sep 17 00:00:00 2001 From: "hasnain2808@gmail.com" Date: Wed, 31 Mar 2021 13:01:33 +0530 Subject: [PATCH 18/23] fix: use pathname on window.location --- frappe/public/js/frappe/router.js | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/frappe/public/js/frappe/router.js b/frappe/public/js/frappe/router.js index fd540fc3cc..f5d13a755c 100644 --- a/frappe/public/js/frappe/router.js +++ b/frappe/public/js/frappe/router.js @@ -361,7 +361,7 @@ frappe.router = { // return clean sub_path from hash or url // supports both v1 and v2 routing if (!route) { - route = window.location.hash || window.location.pathname; + route = window.location.hash || (window.location.pathname + window.location.search); } return this.strip_prefix(route); @@ -386,8 +386,6 @@ frappe.router = { set_route_options_from_url(route) { // set query parameters as frappe.route_options var last_part = route[route.length - 1]; - // add routing v2 compatability - if (!last_part.includes("?")) last_part = route[route.length - 1] + window.location.search if (last_part.indexOf("?") < last_part.indexOf("=")) { // has ? followed by = let parts = last_part.split("?"); From 8fcb97ae31522a36d1fc2a10607aa0702ac9bcdb Mon Sep 17 00:00:00 2001 From: leela Date: Wed, 31 Mar 2021 13:24:18 +0530 Subject: [PATCH 19/23] test: OTP atempt tracker tests --- frappe/auth.py | 2 +- frappe/tests/__init__.py | 9 ++++++++ frappe/tests/test_twofactor.py | 40 +++++++++++++++++++++++++++++++++- 3 files changed, 49 insertions(+), 2 deletions(-) diff --git a/frappe/auth.py b/frappe/auth.py index 32deb1bb48..ca97bbc17d 100644 --- a/frappe/auth.py +++ b/frappe/auth.py @@ -396,7 +396,7 @@ def validate_ip_address(user): frappe.throw(_("Access not allowed from this IP Address"), frappe.AuthenticationError) -def get_login_attempt_tracker(user_name: str, raise_locked_exception: bool=True): +def get_login_attempt_tracker(user_name: str, raise_locked_exception: bool = True): """Get login attempt tracker instance. :param user_name: Name of the loggedin user diff --git a/frappe/tests/__init__.py b/frappe/tests/__init__.py index e69de29bb2..f4dc7f33e2 100644 --- a/frappe/tests/__init__.py +++ b/frappe/tests/__init__.py @@ -0,0 +1,9 @@ +import frappe + +def update_system_settings(args): + doc = frappe.get_doc('System Settings') + doc.update(args) + doc.save() + +def get_system_setting(key): + return frappe.db.get_single_value("System Settings", key) diff --git a/frappe/tests/test_twofactor.py b/frappe/tests/test_twofactor.py index d18a25b43e..7acb0a36e8 100644 --- a/frappe/tests/test_twofactor.py +++ b/frappe/tests/test_twofactor.py @@ -6,23 +6,34 @@ import unittest, frappe, pyotp from frappe.auth import HTTPRequest from frappe.utils import cint from frappe.utils import set_request -from frappe.auth import validate_ip_address +from frappe.auth import validate_ip_address, get_login_attempt_tracker from frappe.twofactor import (should_run_2fa, authenticate_for_2factor, get_cached_user_pass, two_factor_is_enabled_for_, confirm_otp_token, get_otpsecret_for_, get_verification_obj) +from . import update_system_settings, get_system_setting import time class TestTwoFactor(unittest.TestCase): + def __init__(self, *args, **kwargs): + super(TestTwoFactor, self).__init__(*args, **kwargs) + self.default_allowed_login_attempts = get_system_setting('allow_consecutive_login_attempts') + def setUp(self): self.http_requests = create_http_request() self.login_manager = frappe.local.login_manager self.user = self.login_manager.user + update_system_settings({ + 'allow_consecutive_login_attempts': 2 + }) def tearDown(self): frappe.local.response['verification'] = None frappe.local.response['tmp_id'] = None disable_2fa() frappe.clear_cache(user=self.user) + update_system_settings({ + 'allow_consecutive_login_attempts': self.default_allowed_login_attempts + }) def test_should_run_2fa(self): '''Should return true if enabled.''' @@ -153,6 +164,33 @@ class TestTwoFactor(unittest.TestCase): enable_2fa() self.assertIsNone(validate_ip_address(self.user)) + def test_otp_attempt_tracker(self): + """Check that OTP login attempts are tracked. + """ + authenticate_for_2factor(self.user) + tmp_id = frappe.local.response['tmp_id'] + otp = 'wrongotp' + with self.assertRaises(frappe.AuthenticationError): + confirm_otp_token(self.login_manager,otp=otp,tmp_id=tmp_id) + + with self.assertRaises(frappe.AuthenticationError): + confirm_otp_token(self.login_manager,otp=otp,tmp_id=tmp_id) + + # REMOVE ME: current logic allows allow_consecutive_login_attempts+1 attempts + # before raising security exception, remove below line when that is fixed. + with self.assertRaises(frappe.AuthenticationError): + confirm_otp_token(self.login_manager,otp=otp,tmp_id=tmp_id) + + with self.assertRaises(frappe.SecurityException): + confirm_otp_token(self.login_manager,otp=otp,tmp_id=tmp_id) + + # Remove tracking cache so that user can try loging in again + tracker = get_login_attempt_tracker(self.user, raise_locked_exception=False) + tracker.add_success_attempt() + + otp = get_otp(self.user) + self.assertTrue(confirm_otp_token(self.login_manager,otp=otp,tmp_id=tmp_id)) + def create_http_request(): '''Get http request object.''' set_request(method='POST', path='login') From b12714ae20f4e23ee8c1fa14b1b122db6a3efb2b Mon Sep 17 00:00:00 2001 From: "hasnain2808@gmail.com" Date: Wed, 31 Mar 2021 15:43:19 +0530 Subject: [PATCH 20/23] fix: checkbox bleed --- frappe/public/scss/desk/list.scss | 1 - 1 file changed, 1 deletion(-) diff --git a/frappe/public/scss/desk/list.scss b/frappe/public/scss/desk/list.scss index ea3401b09e..0230138e8f 100644 --- a/frappe/public/scss/desk/list.scss +++ b/frappe/public/scss/desk/list.scss @@ -261,7 +261,6 @@ input.list-check-all, input.list-row-checkbox { input[type=checkbox] { margin: 0; margin-right: 5px; - flex: 0 0 12px; } .liked-by, .liked-by-filter-button { From bbcfee9b488e7c5d57d46e592754095495caa74c Mon Sep 17 00:00:00 2001 From: "hasnain2808@gmail.com" Date: Wed, 31 Mar 2021 17:59:51 +0530 Subject: [PATCH 21/23] fix: change z-index even when list is empty --- frappe/public/js/frappe/form/multi_select_dialog.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frappe/public/js/frappe/form/multi_select_dialog.js b/frappe/public/js/frappe/form/multi_select_dialog.js index 26baee05ea..dd96b57fb5 100644 --- a/frappe/public/js/frappe/form/multi_select_dialog.js +++ b/frappe/public/js/frappe/form/multi_select_dialog.js @@ -251,7 +251,6 @@ frappe.ui.form.MultiSelectDialog = class MultiSelectDialog { head ? $row.addClass('list-item--head') : $row = $(`
`).append($row); - $(".modal-dialog .list-item--head").css("z-index", 0); return $row; } @@ -264,6 +263,7 @@ frappe.ui.form.MultiSelectDialog = class MultiSelectDialog { this.empty_list(); } more_btn.hide(); + $(".modal-dialog .list-item--head").css("z-index", 0); if (results.length === 0) return; if (more) more_btn.show(); From 193dbec47cf7f31bd0bd94afe986f36a3d67aa7c Mon Sep 17 00:00:00 2001 From: Walstan Baptista <38958184+walstanb@users.noreply.github.com> Date: Wed, 31 Mar 2021 22:03:34 +0530 Subject: [PATCH 22/23] fix: `frappe.client.get_value()` to work when as_dict is false (#12739) --- frappe/client.py | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/frappe/client.py b/frappe/client.py index 156c31e554..58cfbd2edd 100644 --- a/frappe/client.py +++ b/frappe/client.py @@ -21,7 +21,7 @@ Requests via FrappeClient are also handled here. @frappe.whitelist() def get_list(doctype, fields=None, filters=None, order_by=None, - limit_start=None, limit_page_length=20, parent=None): + limit_start=None, limit_page_length=20, parent=None, debug=False, as_dict=True): '''Returns a list of records by filters, fields, ordering and limit :param doctype: DocType of the data to be queried @@ -40,10 +40,11 @@ def get_list(doctype, fields=None, filters=None, order_by=None, order_by=order_by, limit_start=limit_start, limit_page_length=limit_page_length, + debug=debug, + as_list=not as_dict ) validate_args(args) - return frappe.get_list(**args) @frappe.whitelist() @@ -103,14 +104,15 @@ def get_value(doctype, fieldname, filters=None, as_dict=True, debug=False, paren if frappe.get_meta(doctype).issingle: value = frappe.db.get_values_from_single(fields, filters, doctype, as_dict=as_dict, debug=debug) else: - value = get_list(doctype, filters=filters, fields=fields, limit_page_length=1) + value = get_list(doctype, filters=filters, fields=fields, debug=debug, limit_page_length=1, as_dict=as_dict) if as_dict: - value = value[0] if value else {} - else: - value = value[0][fieldname] + return value[0] if value else {} - return value + if not value: + return + + return value[0] if len(fields) > 1 else value[0][0] @frappe.whitelist() def get_single_value(doctype, field): From b279ca2c0f30cbaad6064e1e10aa1228c24363d4 Mon Sep 17 00:00:00 2001 From: Suraj Shetty <13928957+surajshetty3416@users.noreply.github.com> Date: Wed, 31 Mar 2021 22:15:29 +0530 Subject: [PATCH 23/23] fix: Return correct value from frappe.db.count (#12748) --- frappe/public/js/frappe/db.js | 16 +++++----------- 1 file changed, 5 insertions(+), 11 deletions(-) diff --git a/frappe/public/js/frappe/db.js b/frappe/public/js/frappe/db.js index 6073c7d3f0..89054e3791 100644 --- a/frappe/public/js/frappe/db.js +++ b/frappe/public/js/frappe/db.js @@ -100,17 +100,11 @@ frappe.db = { const fields = []; - return frappe.call({ - type: 'GET', - method: 'frappe.desk.reportview.get_count', - args: { - doctype, - filters, - fields, - distinct, - } - }).then(r => { - return r.message.values; + return frappe.xcall('frappe.desk.reportview.get_count', { + doctype, + filters, + fields, + distinct, }); }, get_link_options(doctype, txt = '', filters={}) {