From a2ffea53f278d6101ec430bd7c026ef0c9204226 Mon Sep 17 00:00:00 2001 From: Rushabh Mehta Date: Mon, 29 Mar 2021 21:44:08 +0530 Subject: [PATCH 01/14] fix(refactor): lockdown frappe.desk.reportview --- frappe/__init__.py | 2 +- frappe/desk/reportview.py | 153 ++++++++++++++++++++++++---------- frappe/model/db_query.py | 41 +-------- frappe/public/js/frappe/db.js | 2 +- 4 files changed, 114 insertions(+), 84 deletions(-) diff --git a/frappe/__init__.py b/frappe/__init__.py index 844a9238e3..d20e856177 100644 --- a/frappe/__init__.py +++ b/frappe/__init__.py @@ -1378,7 +1378,7 @@ def get_list(doctype, *args, **kwargs): frappe.get_list("ToDo", fields="*", filters = {"description": ("like", "test%")}) """ import frappe.model.db_query - return frappe.model.db_query.DatabaseQuery(doctype).execute(None, *args, **kwargs) + return frappe.model.db_query.DatabaseQuery(doctype).execute(*args, **kwargs) def get_all(doctype, *args, **kwargs): """List database query via `frappe.model.db_query`. Will **not** check for permissions. diff --git a/frappe/desk/reportview.py b/frappe/desk/reportview.py index 3003385601..e5c87d9603 100644 --- a/frappe/desk/reportview.py +++ b/frappe/desk/reportview.py @@ -8,6 +8,7 @@ import frappe, json from six.moves import range import frappe.permissions from frappe.model.db_query import DatabaseQuery +from frappe.model import default_fields, optional_fields from frappe import _ from six import string_types, StringIO from frappe.core.doctype.access_log.access_log import make_access_log @@ -18,10 +19,13 @@ from frappe.utils import cstr, format_duration @frappe.read_only() def get(): args = get_form_params() + return compress(execute(**args), args=args) - data = compress(execute(**args), args = args) - - return data +@frappe.whitelist() +@frappe.read_only() +def get_list(): + # uncompressed (refactored from frappe.model.db_query.get_list) + return execute(**get_form_params()) def execute(doctype, *args, **kwargs): return DatabaseQuery(doctype).execute(*args, **kwargs) @@ -29,9 +33,104 @@ def execute(doctype, *args, **kwargs): def get_form_params(): """Stringify GET request parameters.""" data = frappe._dict(frappe.local.form_dict) + clean_params(data) + parse_json(data) - is_report = data.get('view') == 'Report' + validate_fields(data) + if data.filters: + validate_filters(data, data.filters) + if data.or_filters: + validate_filters(data, data.or_filters) + data.strict = None + + return data + +def get_fields(data): + if ((isinstance(fields, string_types) and fields == "*") + or (isinstance(fields, (list, tuple)) and len(fields) == 1 and fields[0] == "*")): + data["fields"] = frappe.db.get_table_columns(data.doctype) + fields = data["fields"] + +def validate_fields(data): + update_star_field_param(data) + + for field in data.fields: + fieldname = extract_fieldname(field) + if is_standard(fieldname): continue + + meta, df = get_meta_and_docfield(fieldname, data) + + if not df: + raise_invalid_field(fieldname) + + # remove the field from the query if the report hide flag is set and current view is Report + if df.report_hide and data.view == 'Report': + data.fields.remove(field) + continue + + if df.fieldname in [_df.fieldname for _df in meta.get_high_permlevel_fields()]: + if df.get('permlevel') not in meta.get_permlevel_access(parenttype=data.doctype): + data.fields.remove(field) + +def validate_filters(data, filters): + if isinstance(filters, list): + # filters as list + for condition in filters: + if len(condition)==3: + # [fieldname, condition, value] + fieldname = condition[0] + if is_standard(fieldname): continue + meta, df = get_meta_and_docfield(fieldname, data) + if not df: + raise_invalid_field(condition[0]) + else: + # [doctype, fieldname, condition, value] + fieldname = condition[1] + if is_standard(fieldname): continue + meta = frappe.get_meta(condition[0]) + if not meta.get_field(fieldname): + raise_invalid_field(fieldname) + + else: + for fieldname in filters: + if is_standard(fieldname): continue + meta, df = get_meta_and_docfield(fieldname, data) + if not df: + raise_invalid_field(fieldname) + +def raise_invalid_field(fieldname): + frappe.throw(_('Field not permitted in query') + ': {0}'.format(fieldname), frappe.DataError) + +def is_standard(fieldname): + if '.' in fieldname: + parenttype, fieldname = get_parenttype_and_fieldname(fieldname, None) + return fieldname in default_fields or fieldname in optional_fields + +def extract_fieldname(field): + fieldname = field.split(" as ")[0] + + # certain functions allowed, extract the fieldname from the function + if (fieldname.startswith('count(') + or fieldname.startswith('sum(') + or fieldname.startswith('avg(')): + fieldname = fieldname.split('(', 1)[1].split(')')[0] + + return fieldname + +def get_meta_and_docfield(fieldname, data): + parenttype, fieldname = get_parenttype_and_fieldname(fieldname, data) + meta = frappe.get_meta(parenttype) + df = meta.get_field(fieldname) + return meta, df + +def update_star_field_param(data): + if ((isinstance(data.fields, string_types) and data.fields == "*") + or (isinstance(data.fields, (list, tuple)) and len(data.fields) == 1 and data.fields[0] == "*")): + data.fields = frappe.db.get_table_columns(data.doctype) + + +def clean_params(data): data.pop('cmd', None) data.pop('data', None) data.pop('ignore_permissions', None) @@ -41,8 +140,12 @@ def get_form_params(): if "csrf_token" in data: del data["csrf_token"] + +def parse_json(data): if isinstance(data.get("filters"), string_types): data["filters"] = json.loads(data["filters"]) + if isinstance(data.get("or_filters"), string_types): + data["or_filters"] = json.loads(data["or_filters"]) if isinstance(data.get("fields"), string_types): data["fields"] = json.loads(data["fields"]) if isinstance(data.get("docstatus"), string_types): @@ -52,47 +155,8 @@ def get_form_params(): else: data["save_user_settings"] = True - fields = data["fields"] - if ((isinstance(fields, string_types) and fields == "*") - or (isinstance(fields, (list, tuple)) and len(fields) == 1 and fields[0] == "*")): - parenttype = data.doctype - data["fields"] = frappe.db.get_table_columns(parenttype) - fields = data["fields"] - - for field in fields: - key = field.split(" as ")[0] - - if key.startswith('count('): continue - if key.startswith('sum('): continue - if key.startswith('avg('): continue - - parenttype, fieldname = get_parent_dt_and_field(key, data) - - if fieldname == "*": - # * inside list is not allowed with other fields - fields.remove(field) - - meta = frappe.get_meta(parenttype) - df = meta.get_field(fieldname) - - report_hide = df.report_hide if df else None - - # remove the field from the query if the report hide flag is set and current view is Report - if report_hide and is_report: - fields.remove(field) - - if df and fieldname in [df.fieldname for df in meta.get_high_permlevel_fields()]: - if df.get('permlevel') not in meta.get_permlevel_access(parenttype=data.doctype) and field in fields: - fields.remove(field) - - # queries must always be server side - data.query = None - data.strict = None - - return data - -def get_parent_dt_and_field(field, data): +def get_parenttype_and_fieldname(field, data): if "." in field: parenttype, fieldname = field.split(".")[0][4:-1], field.split(".")[1].strip("`") else: @@ -101,7 +165,6 @@ def get_parent_dt_and_field(field, data): return parenttype, fieldname - def compress(data, args = {}): """separate keys and values""" from frappe.desk.query_report import add_total_row diff --git a/frappe/model/db_query.py b/frappe/model/db_query.py index 8eac75eb65..924686299a 100644 --- a/frappe/model/db_query.py +++ b/frappe/model/db_query.py @@ -32,7 +32,7 @@ class DatabaseQuery(object): self.flags = frappe._dict() self.reference_doctype = None - def execute(self, query=None, fields=None, filters=None, or_filters=None, + def execute(self, fields=None, filters=None, or_filters=None, docstatus=None, group_by=None, order_by=None, limit_start=False, limit_page_length=None, as_list=False, with_childnames=False, debug=False, ignore_permissions=False, user=None, with_comment_count=False, @@ -104,12 +104,9 @@ class DatabaseQuery(object): # no table & ignore_ddl, return if not self.columns: return [] - if query: - result = self.run_custom_query(query) - else: - result = self.build_and_run() - if return_query: - return result + result = self.build_and_run() + if return_query: + return result if with_comment_count and not as_list and self.doctype: self.add_comment_count(result) @@ -707,12 +704,6 @@ class DatabaseQuery(object): return " and ".join(conditions) if conditions else "" - - def run_custom_query(self, query): - if '%(key)s' in query: - query = query.replace('%(key)s', '`name`') - return frappe.db.sql(query, as_dict = (not self.as_list)) - def set_order_by(self, args): meta = frappe.get_meta(self.doctype) @@ -819,30 +810,6 @@ def get_order_by(doctype, meta): return order_by - -@frappe.whitelist() -def get_list(doctype, *args, **kwargs): - '''wrapper for DatabaseQuery''' - kwargs.pop('cmd', None) - kwargs.pop('ignore_permissions', None) - kwargs.pop('data', None) - kwargs.pop('strict', None) - kwargs.pop('user', None) - - # If doctype is child table - if frappe.is_table(doctype): - # Example frappe.db.get_list('Purchase Receipt Item', {'parent': 'Purchase Receipt'}) - # Here purchase receipt is the parent doctype of the child doctype Purchase Receipt Item - - if not kwargs.get('parent'): - frappe.flags.error_message = _('Parent is required to get child table data') - raise frappe.PermissionError(doctype) - - check_parent_permission(kwargs.get('parent'), doctype) - del kwargs['parent'] - - return DatabaseQuery(doctype).execute(None, *args, **kwargs) - def is_parent_only_filter(doctype, filters): #check if filters contains only parent doctype only_parent_doctype = True diff --git a/frappe/public/js/frappe/db.js b/frappe/public/js/frappe/db.js index cf716c67e5..1cfc6f3696 100644 --- a/frappe/public/js/frappe/db.js +++ b/frappe/public/js/frappe/db.js @@ -15,7 +15,7 @@ frappe.db = { } return new Promise ((resolve) => { frappe.call({ - method: 'frappe.model.db_query.get_list', + method: 'frappe.desk.reportview.get_list', args: args, type: 'GET', callback: function(r) { From 511a5ddc4bbb9bf3652b75c7f1ba644be6ac2d7e Mon Sep 17 00:00:00 2001 From: Rushabh Mehta Date: Mon, 29 Mar 2021 22:02:21 +0530 Subject: [PATCH 02/14] fix(minor): fieldnames can't have commas --- frappe/desk/reportview.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/frappe/desk/reportview.py b/frappe/desk/reportview.py index e5c87d9603..69bc94f383 100644 --- a/frappe/desk/reportview.py +++ b/frappe/desk/reportview.py @@ -108,6 +108,9 @@ def is_standard(fieldname): return fieldname in default_fields or fieldname in optional_fields def extract_fieldname(field): + if ',' in field: + raise_invalid_field(field) + fieldname = field.split(" as ")[0] # certain functions allowed, extract the fieldname from the function From 0c995940c126ba8685111bbba5d2b2d3b5249de2 Mon Sep 17 00:00:00 2001 From: Rushabh Mehta Date: Mon, 29 Mar 2021 22:04:04 +0530 Subject: [PATCH 03/14] fix(minor): functions must end with bracket --- frappe/desk/reportview.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/frappe/desk/reportview.py b/frappe/desk/reportview.py index 69bc94f383..72c452ce0c 100644 --- a/frappe/desk/reportview.py +++ b/frappe/desk/reportview.py @@ -117,6 +117,8 @@ def extract_fieldname(field): if (fieldname.startswith('count(') or fieldname.startswith('sum(') or fieldname.startswith('avg(')): + if not fieldname.endswith(')'): + raise_invalid_field(field) fieldname = fieldname.split('(', 1)[1].split(')')[0] return fieldname From 0def97543b2d16c75acb9449d7558a4ced39b8c1 Mon Sep 17 00:00:00 2001 From: Rushabh Mehta Date: Mon, 29 Mar 2021 22:22:20 +0530 Subject: [PATCH 04/14] fix(minor): handle as fieldnames --- frappe/desk/reportview.py | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/frappe/desk/reportview.py b/frappe/desk/reportview.py index 72c452ce0c..8bad1a2612 100644 --- a/frappe/desk/reportview.py +++ b/frappe/desk/reportview.py @@ -111,15 +111,18 @@ def extract_fieldname(field): if ',' in field: raise_invalid_field(field) - fieldname = field.split(" as ")[0] + fieldname = field + for sep in (' as ', ' AS '): + if sep in fieldname: + fieldname = fieldname.split(sep)[0] # certain functions allowed, extract the fieldname from the function if (fieldname.startswith('count(') or fieldname.startswith('sum(') or fieldname.startswith('avg(')): - if not fieldname.endswith(')'): + if not fieldname.strip().endswith(')'): raise_invalid_field(field) - fieldname = fieldname.split('(', 1)[1].split(')')[0] + fieldname = fieldname.split('(', 1)[1][:-1] return fieldname From c9b367933a3de7e587acd12060a6da53cce4de3f Mon Sep 17 00:00:00 2001 From: Rushabh Mehta Date: Mon, 29 Mar 2021 23:16:33 +0530 Subject: [PATCH 05/14] fix(minor): make group_by validation tighter --- frappe/model/db_query.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frappe/model/db_query.py b/frappe/model/db_query.py index 924686299a..2d553335ee 100644 --- a/frappe/model/db_query.py +++ b/frappe/model/db_query.py @@ -745,7 +745,7 @@ class DatabaseQuery(object): return _lower = parameters.lower() - if 'select' in _lower and ' from ' in _lower: + if 'select' in _lower and 'from' in _lower: frappe.throw(_('Cannot use sub-query in order by')) if re.compile(r".*[^a-z0-9-_ ,`'\"\.\(\)].*").match(_lower): From 6d978a1df0b7c7cbebf244fa134a7fde6ad88a79 Mon Sep 17 00:00:00 2001 From: Rushabh Mehta Date: Tue, 30 Mar 2021 09:47:07 +0530 Subject: [PATCH 06/14] fix(report): move count, aggregation to serverside --- frappe/desk/reportview.py | 24 +++++++++++++++++++ frappe/public/js/frappe/db.js | 14 +++++------ .../public/js/frappe/ui/group_by/group_by.js | 16 ++----------- 3 files changed, 33 insertions(+), 21 deletions(-) diff --git a/frappe/desk/reportview.py b/frappe/desk/reportview.py index 8bad1a2612..ae58f3baea 100644 --- a/frappe/desk/reportview.py +++ b/frappe/desk/reportview.py @@ -27,6 +27,13 @@ def get_list(): # uncompressed (refactored from frappe.model.db_query.get_list) return execute(**get_form_params()) +@frappe.whitelist() +@frappe.read_only() +def get_count(): + args = get_form_params() + args.fields = ['{distinct}count(name) as total_count'.format(distinct = 'distinct ' if args.distinct=='true' else '')] + return execute(**args)[0].get('total_count') + def execute(doctype, *args, **kwargs): return DatabaseQuery(doctype).execute(*args, **kwargs) @@ -35,6 +42,7 @@ def get_form_params(): data = frappe._dict(frappe.local.form_dict) clean_params(data) parse_json(data) + setup_group_by(data) validate_fields(data) if data.filters: @@ -99,6 +107,22 @@ def validate_filters(data, filters): if not df: raise_invalid_field(fieldname) +def setup_group_by(data): + ''' + Add columns for aggregated values e.g. count(name) + ''' + if data.group_by: + if data.aggregate_function.lower() not in ('count', 'sum', 'avg'): + frappe.throw('Invalid aggregate function') + if '`' in data.aggregate_on: + raise_invalid_field(data.aggregate_on) + data.fields.append('{aggregate_function}(`tab{doctype}`.`{aggregate_on}`) AS _aggregate_column'.format(**data)) + if data.aggregate_on: + data.fields.append(data.aggregate_on) + + data.pop('aggregate_on') + data.pop('aggregate_function') + def raise_invalid_field(fieldname): frappe.throw(_('Field not permitted in query') + ': {0}'.format(fieldname), frappe.DataError) diff --git a/frappe/public/js/frappe/db.js b/frappe/public/js/frappe/db.js index 1cfc6f3696..6073c7d3f0 100644 --- a/frappe/public/js/frappe/db.js +++ b/frappe/public/js/frappe/db.js @@ -92,25 +92,25 @@ frappe.db = { }, count: function(doctype, args={}) { let filters = args.filters || {}; - const with_child_table_filter = Array.isArray(filters) && filters.some(filter => { + + // has a filter with childtable? + const distinct = Array.isArray(filters) && filters.some(filter => { return filter[0] !== doctype; }); - const fields = [ - // cannot break this line as it adds extra \n's and \t's which breaks the query - `count(${with_child_table_filter ? 'distinct': ''} ${frappe.model.get_full_column_name('name', doctype)}) AS total_count` - ]; + const fields = []; return frappe.call({ type: 'GET', - method: 'frappe.desk.reportview.get', + method: 'frappe.desk.reportview.get_count', args: { doctype, filters, fields, + distinct, } }).then(r => { - return r.message.values[0][0]; + return r.message.values; }); }, get_link_options(doctype, txt = '', filters={}) { diff --git a/frappe/public/js/frappe/ui/group_by/group_by.js b/frappe/public/js/frappe/ui/group_by/group_by.js index fa699b1a91..53e4914f0d 100644 --- a/frappe/public/js/frappe/ui/group_by/group_by.js +++ b/frappe/public/js/frappe/ui/group_by/group_by.js @@ -286,15 +286,6 @@ frappe.ui.GroupBy = class { set_args(args) { if (this.aggregate_function && this.group_by) { - let aggregate_column, aggregate_on_field; - - if (this.aggregate_function === 'count') { - aggregate_column = 'count(`tab' + this.doctype + '`.`name`)'; - } else { - aggregate_column = `${this.aggregate_function}(${this.aggregate_on})`; - aggregate_on_field = this.aggregate_on; - } - this.report_view.group_by = this.group_by; this.report_view.sort_by = '_aggregate_column'; this.report_view.sort_order = 'desc'; @@ -316,17 +307,14 @@ frappe.ui.GroupBy = class { '_aggregate_column', this.aggregate_on_doctype || this.doctype, ]); - args.fields.push(aggregate_column + ' as _aggregate_column'); - - if (aggregate_on_field) { - args.fields.push(aggregate_on_field); - } // setup columns in datatable this.report_view.setup_columns(); Object.assign(args, { with_comment_count: false, + aggregate_on: this.aggregate_on || 'name', + aggregate_function: this.aggregate_function || 'count', group_by: this.report_view.group_by || null, order_by: '_aggregate_column desc', }); From f1f64772a5bfae07583c8a9217f6d895ba46c6af Mon Sep 17 00:00:00 2001 From: Rushabh Mehta Date: Tue, 30 Mar 2021 10:30:20 +0530 Subject: [PATCH 07/14] fix(minor): disallow comments in fields --- frappe/desk/reportview.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/frappe/desk/reportview.py b/frappe/desk/reportview.py index ae58f3baea..1e6b4e35b5 100644 --- a/frappe/desk/reportview.py +++ b/frappe/desk/reportview.py @@ -132,8 +132,9 @@ def is_standard(fieldname): return fieldname in default_fields or fieldname in optional_fields def extract_fieldname(field): - if ',' in field: - raise_invalid_field(field) + for text in (',', '/*', '#'): + if text in field: + raise_invalid_field(field) fieldname = field for sep in (' as ', ' AS '): From 2248c6c410d8cd9db18e6bc71942a3e42d9af3d8 Mon Sep 17 00:00:00 2001 From: Rushabh Mehta Date: Tue, 30 Mar 2021 11:25:40 +0530 Subject: [PATCH 08/14] fix(minor): lockdown frappe.client.get_list --- frappe/client.py | 34 +++++++++++++++++----------------- frappe/desk/reportview.py | 3 +++ frappe/model/db_query.py | 13 ++++++++++++- 3 files changed, 32 insertions(+), 18 deletions(-) diff --git a/frappe/client.py b/frappe/client.py index 2217b53673..62317055fb 100644 --- a/frappe/client.py +++ b/frappe/client.py @@ -8,6 +8,8 @@ import frappe.model import frappe.utils import json, os from frappe.utils import get_safe_filters +from frappe.desk.reportview import validate_args +from frappe.model.db_query import check_parent_permission from six import iteritems, string_types, integer_types @@ -31,8 +33,18 @@ def get_list(doctype, fields=None, filters=None, order_by=None, if frappe.is_table(doctype): check_parent_permission(parent, doctype) - return frappe.get_list(doctype, fields=fields, filters=filters, order_by=order_by, - limit_start=limit_start, limit_page_length=limit_page_length, ignore_permissions=False) + args = dict( + doctype=doctype, + fields=fields, + filters=filters, + order_by=order_by, + limit_start=limit_start, + limit_page_length=limit_page_length, + ) + + validate_args(args) + + return frappe.get_list(**args) @frappe.whitelist() def get_count(doctype, filters=None, debug=False, cache=False): @@ -91,12 +103,12 @@ 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 = frappe.get_list(doctype, filters=filters, fields=fields, debug=debug, limit=1) + value = get_list(doctype, filters=filters, fields=fields, limit_page_length=1) if as_dict: value = value[0] if value else {} else: - value = value[0].fieldname + value = value[0][fieldname] return value @@ -378,18 +390,6 @@ def attach_file(filename=None, filedata=None, doctype=None, docname=None, folder def get_hooks(hook, app_name=None): return frappe.get_hooks(hook, app_name) -def check_parent_permission(parent, child_doctype): - if parent: - # User may pass fake parent and get the information from the child table - if child_doctype and not frappe.db.exists('DocField', - {'parent': parent, 'options': child_doctype}): - raise frappe.PermissionError - - if frappe.permissions.has_permission(parent): - return - # Either parent not passed or the user doesn't have permission on parent doctype of child table! - raise frappe.PermissionError - @frappe.whitelist() def is_document_amended(doctype, docname): if frappe.permissions.has_permission(doctype): @@ -400,4 +400,4 @@ def is_document_amended(doctype, docname): except frappe.db.InternalError: pass - return False \ No newline at end of file + return False diff --git a/frappe/desk/reportview.py b/frappe/desk/reportview.py index 1e6b4e35b5..7f1fec826d 100644 --- a/frappe/desk/reportview.py +++ b/frappe/desk/reportview.py @@ -41,6 +41,9 @@ def get_form_params(): """Stringify GET request parameters.""" data = frappe._dict(frappe.local.form_dict) clean_params(data) + validate_args(data) + +def validate_args(data): parse_json(data) setup_group_by(data) diff --git a/frappe/model/db_query.py b/frappe/model/db_query.py index 2d553335ee..b29e143759 100644 --- a/frappe/model/db_query.py +++ b/frappe/model/db_query.py @@ -14,7 +14,6 @@ import frappe.permissions from datetime import datetime import frappe, json, copy, re from frappe.model import optional_fields -from frappe.client import check_parent_permission from frappe.model.utils.user_settings import get_user_settings, update_user_settings from frappe.utils import flt, cint, get_time, make_filter_tuple, get_filter, add_to_date, cstr, get_timespan_date_range from frappe.model.meta import get_table_columns @@ -786,6 +785,18 @@ class DatabaseQuery(object): update_user_settings(self.doctype, user_settings) +def check_parent_permission(parent, child_doctype): + if parent: + # User may pass fake parent and get the information from the child table + if child_doctype and not frappe.db.exists('DocField', + {'parent': parent, 'options': child_doctype}): + raise frappe.PermissionError + + if frappe.permissions.has_permission(parent): + return + # Either parent not passed or the user doesn't have permission on parent doctype of child table! + raise frappe.PermissionError + def get_order_by(doctype, meta): order_by = "" From d959fc7310fc0315d7983a4eb919a8bd69b741e1 Mon Sep 17 00:00:00 2001 From: Rushabh Mehta Date: Tue, 30 Mar 2021 11:52:46 +0530 Subject: [PATCH 09/14] fix(minor): tests and linting --- frappe/client.py | 2 +- frappe/desk/reportview.py | 28 +++++++++++++++------------- 2 files changed, 16 insertions(+), 14 deletions(-) diff --git a/frappe/client.py b/frappe/client.py index 62317055fb..156c31e554 100644 --- a/frappe/client.py +++ b/frappe/client.py @@ -33,7 +33,7 @@ def get_list(doctype, fields=None, filters=None, order_by=None, if frappe.is_table(doctype): check_parent_permission(parent, doctype) - args = dict( + args = frappe._dict( doctype=doctype, fields=fields, filters=filters, diff --git a/frappe/desk/reportview.py b/frappe/desk/reportview.py index 7f1fec826d..9244023a5a 100644 --- a/frappe/desk/reportview.py +++ b/frappe/desk/reportview.py @@ -57,18 +57,14 @@ def validate_args(data): return data -def get_fields(data): - if ((isinstance(fields, string_types) and fields == "*") - or (isinstance(fields, (list, tuple)) and len(fields) == 1 and fields[0] == "*")): - data["fields"] = frappe.db.get_table_columns(data.doctype) - fields = data["fields"] - def validate_fields(data): + expand_fields(data) update_star_field_param(data) for field in data.fields: fieldname = extract_fieldname(field) - if is_standard(fieldname): continue + if is_standard(fieldname): + continue meta, df = get_meta_and_docfield(fieldname, data) @@ -91,29 +87,30 @@ def validate_filters(data, filters): if len(condition)==3: # [fieldname, condition, value] fieldname = condition[0] - if is_standard(fieldname): continue + if is_standard(fieldname): + continue meta, df = get_meta_and_docfield(fieldname, data) if not df: raise_invalid_field(condition[0]) else: # [doctype, fieldname, condition, value] fieldname = condition[1] - if is_standard(fieldname): continue + if is_standard(fieldname): + continue meta = frappe.get_meta(condition[0]) if not meta.get_field(fieldname): raise_invalid_field(fieldname) else: for fieldname in filters: - if is_standard(fieldname): continue + if is_standard(fieldname): + continue meta, df = get_meta_and_docfield(fieldname, data) if not df: raise_invalid_field(fieldname) def setup_group_by(data): - ''' - Add columns for aggregated values e.g. count(name) - ''' + '''Add columns for aggregated values e.g. count(name)''' if data.group_by: if data.aggregate_function.lower() not in ('count', 'sum', 'avg'): frappe.throw('Invalid aggregate function') @@ -126,6 +123,11 @@ def setup_group_by(data): data.pop('aggregate_on') data.pop('aggregate_function') +def expand_fields(data): + if ((isinstance(data.fields, string_types) and data.fields == "*") + or (isinstance(data.fields, (list, tuple)) and len(data.fields) == 1 and data.fields[0] == "*")): + data.fields = frappe.db.get_table_columns(data.doctype) + def raise_invalid_field(fieldname): frappe.throw(_('Field not permitted in query') + ': {0}'.format(fieldname), frappe.DataError) From 1aa8adadcb7eae05b452f78ef5d30cfea8840213 Mon Sep 17 00:00:00 2001 From: Rushabh Mehta Date: Tue, 30 Mar 2021 12:57:15 +0530 Subject: [PATCH 10/14] fix(minor): return value in reportview.py --- frappe/desk/reportview.py | 1 + 1 file changed, 1 insertion(+) diff --git a/frappe/desk/reportview.py b/frappe/desk/reportview.py index 9244023a5a..64413b2728 100644 --- a/frappe/desk/reportview.py +++ b/frappe/desk/reportview.py @@ -42,6 +42,7 @@ def get_form_params(): data = frappe._dict(frappe.local.form_dict) clean_params(data) validate_args(data) + return data def validate_args(data): parse_json(data) From 868228bdea0b043402d53223685a28eaa2760980 Mon Sep 17 00:00:00 2001 From: Rushabh Mehta Date: Tue, 30 Mar 2021 15:50:47 +0530 Subject: [PATCH 11/14] fix(minor): tests --- frappe/desk/reportview.py | 11 ++++++++--- frappe/frappeclient.py | 2 +- 2 files changed, 9 insertions(+), 4 deletions(-) diff --git a/frappe/desk/reportview.py b/frappe/desk/reportview.py index 64413b2728..fd61224b19 100644 --- a/frappe/desk/reportview.py +++ b/frappe/desk/reportview.py @@ -60,9 +60,11 @@ def validate_args(data): def validate_fields(data): expand_fields(data) - update_star_field_param(data) + if update_wildcard_field_param(data): + # no need to validate wildcard fields + return - for field in data.fields: + for field in data.fields or []: fieldname = extract_fieldname(field) if is_standard(fieldname): continue @@ -163,10 +165,13 @@ def get_meta_and_docfield(fieldname, data): df = meta.get_field(fieldname) return meta, df -def update_star_field_param(data): +def update_wildcard_field_param(data): if ((isinstance(data.fields, string_types) and data.fields == "*") or (isinstance(data.fields, (list, tuple)) and len(data.fields) == 1 and data.fields[0] == "*")): data.fields = frappe.db.get_table_columns(data.doctype) + return True + + return False def clean_params(data): diff --git a/frappe/frappeclient.py b/frappe/frappeclient.py index 919c334e51..054a8c9369 100644 --- a/frappe/frappeclient.py +++ b/frappe/frappeclient.py @@ -86,7 +86,7 @@ class FrappeClient(object): 'cmd': 'logout', }, verify=self.verify, headers=self.headers) - def get_list(self, doctype, fields='"*"', filters=None, limit_start=0, limit_page_length=0): + def get_list(self, doctype, fields='["name"]', filters=None, limit_start=0, limit_page_length=0): """Returns list of records of a particular type""" if not isinstance(fields, string_types): fields = json.dumps(fields) From 82399ddfba3d03b08491aa82d23aec45df9e5663 Mon Sep 17 00:00:00 2001 From: Rushabh Mehta Date: Tue, 30 Mar 2021 16:37:53 +0530 Subject: [PATCH 12/14] fix(minor): tests --- frappe/desk/reportview.py | 6 ------ 1 file changed, 6 deletions(-) diff --git a/frappe/desk/reportview.py b/frappe/desk/reportview.py index fd61224b19..e9914ef4cf 100644 --- a/frappe/desk/reportview.py +++ b/frappe/desk/reportview.py @@ -59,7 +59,6 @@ def validate_args(data): return data def validate_fields(data): - expand_fields(data) if update_wildcard_field_param(data): # no need to validate wildcard fields return @@ -126,11 +125,6 @@ def setup_group_by(data): data.pop('aggregate_on') data.pop('aggregate_function') -def expand_fields(data): - if ((isinstance(data.fields, string_types) and data.fields == "*") - or (isinstance(data.fields, (list, tuple)) and len(data.fields) == 1 and data.fields[0] == "*")): - data.fields = frappe.db.get_table_columns(data.doctype) - def raise_invalid_field(fieldname): frappe.throw(_('Field not permitted in query') + ': {0}'.format(fieldname), frappe.DataError) From 337bdc976c2a2c60c50b0924c0d9fad0d8935c6a Mon Sep 17 00:00:00 2001 From: Rushabh Mehta Date: Tue, 30 Mar 2021 18:35:44 +0530 Subject: [PATCH 13/14] fix(reportview): test --- frappe/desk/reportview.py | 9 +++++---- frappe/tests/test_db_query.py | 3 +++ 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/frappe/desk/reportview.py b/frappe/desk/reportview.py index e9914ef4cf..2d6c18440a 100644 --- a/frappe/desk/reportview.py +++ b/frappe/desk/reportview.py @@ -59,9 +59,7 @@ def validate_args(data): return data def validate_fields(data): - if update_wildcard_field_param(data): - # no need to validate wildcard fields - return + wildcard = update_wildcard_field_param(data) for field in data.fields or []: fieldname = extract_fieldname(field) @@ -71,7 +69,10 @@ def validate_fields(data): meta, df = get_meta_and_docfield(fieldname, data) if not df: - raise_invalid_field(fieldname) + if wildcard: + continue + else: + raise_invalid_field(fieldname) # remove the field from the query if the report hide flag is set and current view is Report if df.report_hide and data.view == 'Report': diff --git a/frappe/tests/test_db_query.py b/frappe/tests/test_db_query.py index 836bb4bbf5..085c257550 100644 --- a/frappe/tests/test_db_query.py +++ b/frappe/tests/test_db_query.py @@ -17,6 +17,9 @@ from frappe.utils.testutils import add_custom_field, clear_custom_fields test_dependencies = ['User', 'Blog Post', 'Blog Category', 'Blogger'] class TestReportview(unittest.TestCase): + def setUp(self): + frappe.set_user("Administrator") + def test_basic(self): self.assertTrue({"name":"DocType"} in DatabaseQuery("DocType").execute(limit_page_length=None)) From 375cdea1458ee6ef24d76b3120d4378c36312d55 Mon Sep 17 00:00:00 2001 From: Rushabh Mehta Date: Wed, 31 Mar 2021 10:10:46 +0530 Subject: [PATCH 14/14] Update frappe/desk/reportview.py Co-authored-by: Suraj Shetty <13928957+surajshetty3416@users.noreply.github.com> --- frappe/desk/reportview.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frappe/desk/reportview.py b/frappe/desk/reportview.py index 2d6c18440a..296dfb94f1 100644 --- a/frappe/desk/reportview.py +++ b/frappe/desk/reportview.py @@ -116,7 +116,7 @@ def setup_group_by(data): '''Add columns for aggregated values e.g. count(name)''' if data.group_by: if data.aggregate_function.lower() not in ('count', 'sum', 'avg'): - frappe.throw('Invalid aggregate function') + frappe.throw(_('Invalid aggregate function')) if '`' in data.aggregate_on: raise_invalid_field(data.aggregate_on) data.fields.append('{aggregate_function}(`tab{doctype}`.`{aggregate_on}`) AS _aggregate_column'.format(**data))