From ccbc833c6c30d527334aa3ed5acd7511665dbaf0 Mon Sep 17 00:00:00 2001 From: Gavin D'souza Date: Thu, 17 Nov 2022 15:59:26 +0530 Subject: [PATCH 01/26] feat: runtime check via pydantic handle localns stringified types --- frappe/__init__.py | 23 ++++++++++++++++++++++- pyproject.toml | 1 + 2 files changed, 23 insertions(+), 1 deletion(-) diff --git a/frappe/__init__.py b/frappe/__init__.py index f1ef9b7493..a783652f0e 100644 --- a/frappe/__init__.py +++ b/frappe/__init__.py @@ -716,6 +716,27 @@ xss_safe_methods = [] allowed_http_methods_for_whitelisted_func = {} +def validate_argument_types(func): + return func + from pydantic import validate_arguments as pyd_validator + + def validator(*args, **kwargs): + return pyd_validator(func)(*args, **kwargs) + + import sys + + # from frappe.model.document import Document + # localns = { + # "Document": Document, + # } + + try: + return validator(func) + except NameError: + sys.stderr.write(f"{func} has unsupported type annotations") + return func + + def whitelist(allow_guest=False, xss_safe=False, methods=None): """ Decorator for whitelisting a function and making it accessible via HTTP. @@ -753,7 +774,7 @@ def whitelist(allow_guest=False, xss_safe=False, methods=None): if xss_safe: xss_safe_methods.append(fn) - return method or fn + return validate_argument_types(method or fn) return innerfn diff --git a/pyproject.toml b/pyproject.toml index 66fef2160a..b17ecdc211 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -52,6 +52,7 @@ dependencies = [ "pyOpenSSL~=22.1.0", "pycryptodome~=3.10.1", "pyotp~=2.6.0", + "pydantic~=1.10.2", "python-dateutil~=2.8.1", "pytz==2022.1", "rauth~=0.7.3", From 3fd74afa479d6ed174c37849e2e1432740577983 Mon Sep 17 00:00:00 2001 From: Gavin D'souza Date: Wed, 23 Nov 2022 15:52:42 +0530 Subject: [PATCH 02/26] feat(whitelisted): Runtime typing hints validation - Run type validations if annotations exist for whitelisted functions - Run validations only on function calls in presense of frappe.local.request In action: ```bash > curl -H 'Content-Type: application/json' 'http://photos:8000/api/method/frappe.handler.download_file' -d '{"file_url": ["!=", "gavin.jpg"]}' ``` Note: This ignores stringified or ForwardRef types. If you want types to be validated make sure they are not imported under `if TYPE_CHECKING` blocks --- frappe/__init__.py | 33 ++++++++---------- frappe/utils/typing_validations.py | 56 ++++++++++++++++++++++++++++++ pyproject.toml | 1 - 3 files changed, 71 insertions(+), 19 deletions(-) create mode 100644 frappe/utils/typing_validations.py diff --git a/frappe/__init__.py b/frappe/__init__.py index a783652f0e..6b9e282f56 100644 --- a/frappe/__init__.py +++ b/frappe/__init__.py @@ -716,25 +716,20 @@ xss_safe_methods = [] allowed_http_methods_for_whitelisted_func = {} -def validate_argument_types(func): - return func - from pydantic import validate_arguments as pyd_validator +def apply_validate_argument_types_wrapper(func): + @functools.wraps(func) + def wrapper(*args, **kwargs): + """Validate argument types of whitelisted functions. - def validator(*args, **kwargs): - return pyd_validator(func)(*args, **kwargs) + :param args: Function arguments. + :param kwargs: Function keyword arguments.""" + from frappe.utils.typing_validations import validate_argument_types - import sys + if getattr(local, "request", None): + validate_argument_types(func, args, kwargs) + return func(*args, **kwargs) - # from frappe.model.document import Document - # localns = { - # "Document": Document, - # } - - try: - return validator(func) - except NameError: - sys.stderr.write(f"{func} has unsupported type annotations") - return func + return wrapper def whitelist(allow_guest=False, xss_safe=False, methods=None): @@ -762,8 +757,10 @@ def whitelist(allow_guest=False, xss_safe=False, methods=None): # this is needed because functions can be compared, but not methods method = None if hasattr(fn, "__func__"): - method = fn + method = apply_validate_argument_types_wrapper(fn) fn = method.__func__ + else: + fn = apply_validate_argument_types_wrapper(fn) whitelisted.append(fn) allowed_http_methods_for_whitelisted_func[fn] = methods @@ -774,7 +771,7 @@ def whitelist(allow_guest=False, xss_safe=False, methods=None): if xss_safe: xss_safe_methods.append(fn) - return validate_argument_types(method or fn) + return method or fn return innerfn diff --git a/frappe/utils/typing_validations.py b/frappe/utils/typing_validations.py new file mode 100644 index 0000000000..921331fa4c --- /dev/null +++ b/frappe/utils/typing_validations.py @@ -0,0 +1,56 @@ +from inspect import isclass +from typing import Callable, ForwardRef, Sequence + + +def qualified_name(obj) -> str: + """ + Return the qualified name (e.g. package.module.Type) for the given object. + + Builtins and types from the :mod:`typing` package get special treatment by having the module + name stripped from the generated name. + + """ + discovered_type = obj if isclass(obj) else type(obj) + module, qualname = discovered_type.__module__, discovered_type.__qualname__ + + if module == "types": + return obj + elif module in {"typing", "builtins"}: + return qualname + else: + return f"{module}.{qualname}" + + +def validate_argument_types(func: Callable, args: tuple, kwargs: dict): + """ + Validate the types of the arguments passed to a function with the type annotations + defined on the function. + + """ + if annotations := func.__annotations__: + # generate kwargs dict from args + arg_names = func.__code__.co_varnames[: func.__code__.co_argcount] + arg_values = args or func.__defaults__ or [] + prepared_args = dict(zip(arg_names, arg_values)) + prepared_args.update(kwargs) + + # check if the argument types are correct + for current_arg, current_arg_type in annotations.items(): + if current_arg not in prepared_args: + continue + + current_arg_value = prepared_args[current_arg] + + # if the type is a ForwardRef or str, ignore it + if isinstance(current_arg_type, (ForwardRef, str)): + continue + + if isinstance(current_arg_type, Sequence): + current_arg_type = tuple( + x for x in current_arg_type.__args__ if not isinstance(x, (ForwardRef, str)) + ) + + if not isinstance(current_arg_value, current_arg_type): + raise TypeError( + f"Argument '{current_arg}' must be of type '{qualified_name(current_arg_type)}' but got '{qualified_name(current_arg_value)}'" + ) diff --git a/pyproject.toml b/pyproject.toml index b17ecdc211..66fef2160a 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -52,7 +52,6 @@ dependencies = [ "pyOpenSSL~=22.1.0", "pycryptodome~=3.10.1", "pyotp~=2.6.0", - "pydantic~=1.10.2", "python-dateutil~=2.8.1", "pytz==2022.1", "rauth~=0.7.3", From 6678007a8da60fb0d26deda0e84bff0b0204f289 Mon Sep 17 00:00:00 2001 From: Gavin D'souza Date: Mon, 28 Nov 2022 18:02:00 +0530 Subject: [PATCH 03/26] fix: Support missing Optional or mismatching annotation types Allow default types as acceptable values if not included in typing hints. A common example of this is: def test_password_strength(new_password: str, user_data: Sequence = None): where Optional[Sequence] is implicit since default value is None. --- frappe/utils/typing_validations.py | 21 +++++++++++++++++---- 1 file changed, 17 insertions(+), 4 deletions(-) diff --git a/frappe/utils/typing_validations.py b/frappe/utils/typing_validations.py index 921331fa4c..b751a37839 100644 --- a/frappe/utils/typing_validations.py +++ b/frappe/utils/typing_validations.py @@ -1,5 +1,5 @@ -from inspect import isclass -from typing import Callable, ForwardRef, Sequence +from inspect import _empty, isclass, signature +from typing import Callable, ForwardRef, Sequence, Union def qualified_name(obj) -> str: @@ -13,9 +13,9 @@ def qualified_name(obj) -> str: discovered_type = obj if isclass(obj) else type(obj) module, qualname = discovered_type.__module__, discovered_type.__qualname__ - if module == "types": + if module == "typing": return obj - elif module in {"typing", "builtins"}: + elif module in {"types", "builtins"}: return qualname else: return f"{module}.{qualname}" @@ -34,6 +34,10 @@ def validate_argument_types(func: Callable, args: tuple, kwargs: dict): prepared_args = dict(zip(arg_names, arg_values)) prepared_args.update(kwargs) + # check if type hints dont match the default values + func_signature = signature(func) + func_params = dict(func_signature.parameters) + # check if the argument types are correct for current_arg, current_arg_type in annotations.items(): if current_arg not in prepared_args: @@ -50,6 +54,15 @@ def validate_argument_types(func: Callable, args: tuple, kwargs: dict): x for x in current_arg_type.__args__ if not isinstance(x, (ForwardRef, str)) ) + param_def = func_params.get(current_arg) + + if param_def.default is not _empty: + if isinstance(current_arg_type, tuple): + if param_def.default not in current_arg_type: + current_arg_type += (type(param_def.default),) + elif param_def.default != current_arg_type: + current_arg_type = Union[current_arg_type, type(param_def.default)] + if not isinstance(current_arg_value, current_arg_type): raise TypeError( f"Argument '{current_arg}' must be of type '{qualified_name(current_arg_type)}' but got '{qualified_name(current_arg_value)}'" From fa88d5f7d9da66db4ece1f753a94e3fa2101c3d9 Mon Sep 17 00:00:00 2001 From: Gavin D'souza Date: Mon, 28 Nov 2022 18:05:58 +0530 Subject: [PATCH 04/26] refactor(minor): User - Add typing hints for enabling endpoint runtime checks - Remove unused parameters from function def and usage - Update docstring in APIs - Remove (now) redundant isinstance checks - Use cached get_system_settings instead of DB call (perf) --- frappe/core/doctype/user/user.py | 55 ++++++++++++++++---------------- 1 file changed, 27 insertions(+), 28 deletions(-) diff --git a/frappe/core/doctype/user/user.py b/frappe/core/doctype/user/user.py index 1568ae8af3..5e11c04380 100644 --- a/frappe/core/doctype/user/user.py +++ b/frappe/core/doctype/user/user.py @@ -1,6 +1,7 @@ # Copyright (c) 2015, Frappe Technologies Pvt. Ltd. and Contributors # License: MIT. See LICENSE from datetime import timedelta +from typing import Optional, Sequence import frappe import frappe.defaults @@ -536,7 +537,7 @@ class User(Document): if self.__new_password: user_data = (self.first_name, self.middle_name, self.last_name, self.email, self.birth_date) - result = test_password_strength(self.__new_password, "", None, user_data) + result = test_password_strength(self.__new_password, user_data) feedback = result.get("feedback", None) if feedback and not feedback.get("password_policy_validation_passed", False): @@ -677,12 +678,19 @@ def get_perm_info(role): @frappe.whitelist(allow_guest=True) -def update_password(new_password, logout_all_sessions=0, key=None, old_password=None): - # validate key to avoid key input like ['like', '%'], '', ['in', ['']] - if key and not isinstance(key, str): - frappe.throw(_("Invalid key type")) +def update_password( + new_password: str, logout_all_sessions: int | bool = 0, key: str = None, old_password: str = None +): + """Update password for the current user. - result = test_password_strength(new_password, key, old_password) + Args: + new_password (str): New password. + logout_all_sessions (int, optional): If set to 1, all other sessions will be logged out. Defaults to 0. + key (str, optional): Password reset key. Defaults to None. + old_password (str, optional): Old password. Defaults to None. + """ + + result = test_password_strength(new_password) feedback = result.get("feedback", None) if feedback and not feedback.get("password_policy_validation_passed", False): @@ -716,22 +724,14 @@ def update_password(new_password, logout_all_sessions=0, key=None, old_password= if user_doc.user_type == "System User": return "/app" else: - return redirect_url if redirect_url else "/" + return redirect_url or "/" @frappe.whitelist(allow_guest=True) -def test_password_strength(new_password, key=None, old_password=None, user_data=None): +def test_password_strength(new_password: str, user_data: tuple = None): from frappe.utils.password_strength import test_password_strength as _test_password_strength - password_policy = ( - frappe.db.get_value( - "System Settings", None, ["enable_password_policy", "minimum_password_score"], as_dict=True - ) - or {} - ) - - enable_password_policy = cint(password_policy.get("enable_password_policy", 0)) - minimum_password_score = cint(password_policy.get("minimum_password_score", 0)) + enable_password_policy = frappe.get_system_settings("enable_password_policy") or 0 if not enable_password_policy: return {} @@ -744,6 +744,7 @@ def test_password_strength(new_password, key=None, old_password=None, user_data= if new_password: result = _test_password_strength(new_password, user_inputs=user_data) password_policy_validation_passed = False + minimum_password_score = frappe.get_system_settings("minimum_password_score") or 0 # score should be greater than 0 and minimum_password_score if result.get("score") and result.get("score") >= minimum_password_score: @@ -753,9 +754,8 @@ def test_password_strength(new_password, key=None, old_password=None, user_data= return result -# for login @frappe.whitelist() -def has_email_account(email): +def has_email_account(email: str): return frappe.get_list("Email Account", filters={"email_id": email}) @@ -822,7 +822,7 @@ def verify_password(password): @frappe.whitelist(allow_guest=True) -def sign_up(email, full_name, redirect_to): +def sign_up(email: str, full_name: str, redirect_to: str) -> tuple[int, str]: if is_signup_disabled(): frappe.throw(_("Sign Up is disabled"), title=_("Not Allowed")) @@ -874,12 +874,12 @@ def sign_up(email, full_name, redirect_to): @frappe.whitelist(allow_guest=True) @rate_limit(limit=get_password_reset_limit, seconds=24 * 60 * 60, methods=["POST"]) -def reset_password(user): +def reset_password(user: str) -> str: if user == "Administrator": return "not allowed" try: - user = frappe.get_doc("User", user) + user: User = frappe.get_doc("User", user) if not user.enabled: return "disabled" @@ -1069,13 +1069,12 @@ def throttle_user_creation(): @frappe.whitelist() -def get_role_profile(role_profile): - roles = frappe.get_doc("Role Profile", {"role_profile": role_profile}) - return roles.roles +def get_role_profile(role_profile: str): + return frappe.get_doc("Role Profile", {"role_profile": role_profile}).roles @frappe.whitelist() -def get_module_profile(module_profile): +def get_module_profile(module_profile: str): module_profile = frappe.get_doc("Module Profile", {"module_profile_name": module_profile}) return module_profile.get("block_modules") @@ -1148,14 +1147,14 @@ def get_restricted_ip_list(user): @frappe.whitelist() -def generate_keys(user): +def generate_keys(user: str): """ generate api key and api secret :param user: str """ frappe.only_for("System Manager") - user_details = frappe.get_doc("User", user) + user_details: User = frappe.get_doc("User", user) api_secret = frappe.generate_hash(length=15) # if api key is not set generate api key if not user_details.api_key: From d3250f650499272b92e3c4e9a9b38cff68a51543 Mon Sep 17 00:00:00 2001 From: Gavin D'souza Date: Tue, 29 Nov 2022 12:57:40 +0530 Subject: [PATCH 05/26] refactor(whitelisted): Add typing hints to APIs * Refactor type checks defined in APIs * Remove dead/deprecated kwargs usages * Added appropriate hints to APIs and consecutive utils defined in the following modules: - frappe.realtime - frappe.translate - frappe.utils.global_search - frappe.www.third_party_apps - frappe.www.search - frappe.www.printview --- frappe/core/doctype/user/user.py | 2 +- frappe/printing/page/print/print.js | 1 - frappe/realtime.py | 7 +-- frappe/translate.py | 4 +- frappe/utils/global_search.py | 2 +- frappe/www/printview.py | 81 +++++++++++++++-------------- frappe/www/search.py | 2 +- frappe/www/third_party_apps.py | 2 +- 8 files changed, 50 insertions(+), 51 deletions(-) diff --git a/frappe/core/doctype/user/user.py b/frappe/core/doctype/user/user.py index 5e11c04380..65100fbefa 100644 --- a/frappe/core/doctype/user/user.py +++ b/frappe/core/doctype/user/user.py @@ -744,7 +744,7 @@ def test_password_strength(new_password: str, user_data: tuple = None): if new_password: result = _test_password_strength(new_password, user_inputs=user_data) password_policy_validation_passed = False - minimum_password_score = frappe.get_system_settings("minimum_password_score") or 0 + minimum_password_score = cint(frappe.get_system_settings("minimum_password_score")) or 0 # score should be greater than 0 and minimum_password_score if result.get("score") and result.get("score") >= minimum_password_score: diff --git a/frappe/printing/page/print/print.js b/frappe/printing/page/print/print.js index 54704467c0..429b081df3 100644 --- a/frappe/printing/page/print/print.js +++ b/frappe/printing/page/print/print.js @@ -673,7 +673,6 @@ frappe.ui.form.PrintView = class { args: { doc: this.frm.doc, print_format: this.selected_format(), - _lang: this.lang_code, }, callback: function (r) { if (!r.exc) { diff --git a/frappe/realtime.py b/frappe/realtime.py index 1d75f9d9e6..eff3ea2b77 100644 --- a/frappe/realtime.py +++ b/frappe/realtime.py @@ -103,10 +103,7 @@ def get_redis_server(): @frappe.whitelist(allow_guest=True) -def can_subscribe_doc(doctype, docname): - if os.environ.get("CI"): - return True - +def can_subscribe_doc(doctype: str, docname: str) -> bool: from frappe.exceptions import PermissionError from frappe.sessions import Session @@ -118,7 +115,7 @@ def can_subscribe_doc(doctype, docname): @frappe.whitelist(allow_guest=True) -def can_subscribe_list(doctype): +def can_subscribe_list(doctype: str) -> bool: from frappe.exceptions import PermissionError if not frappe.has_permission(user=frappe.session.user, doctype=doctype, ptype="read"): diff --git a/frappe/translate.py b/frappe/translate.py index 975d8f07f5..0d6e49114e 100644 --- a/frappe/translate.py +++ b/frappe/translate.py @@ -1279,7 +1279,7 @@ def get_translator_url(): @frappe.whitelist(allow_guest=True) -def get_all_languages(with_language_name=False): +def get_all_languages(with_language_name: bool = False) -> list: """Returns all enabled language codes ar, ch etc""" def get_language_codes(): @@ -1298,7 +1298,7 @@ def get_all_languages(with_language_name=False): @frappe.whitelist(allow_guest=True) -def set_preferred_language_cookie(preferred_language): +def set_preferred_language_cookie(preferred_language: str): frappe.local.cookie_manager.set_cookie("preferred_language", preferred_language) diff --git a/frappe/utils/global_search.py b/frappe/utils/global_search.py index ba22251f6c..9efa9de592 100644 --- a/frappe/utils/global_search.py +++ b/frappe/utils/global_search.py @@ -489,7 +489,7 @@ def search(text, start=0, limit=20, doctype=""): @frappe.whitelist(allow_guest=True) -def web_search(text, scope=None, start=0, limit=20): +def web_search(text: str, scope: str = None, start: int = 0, limit: int = 20): """ Search for given text in __global_search where published = 1 :param text: phrase to be searched diff --git a/frappe/www/printview.py b/frappe/www/printview.py index faf6a02067..8a0ce8fd2c 100644 --- a/frappe/www/printview.py +++ b/frappe/www/printview.py @@ -5,6 +5,7 @@ import copy import json import os import re +from typing import TYPE_CHECKING import frappe from frappe import _, get_module_path @@ -13,6 +14,10 @@ from frappe.core.doctype.document_share_key.document_share_key import is_expired from frappe.utils import cint, sanitize_html, strip_html from frappe.utils.jinja_globals import is_rtl +if TYPE_CHECKING: + from frappe.model.document import Document + from frappe.printing.doctype.print_format.print_format import PrintFormat + no_cache = 1 standard_format = "templates/print_formats/standard.html" @@ -88,13 +93,12 @@ def get_print_format_doc(print_format_name, meta): def get_rendered_template( - doc, - name=None, - print_format=None, + doc: "Document", + print_format: str = None, meta=None, - no_letterhead=None, - letterhead=None, - trigger_print=False, + no_letterhead: bool = None, + letterhead: str = None, + trigger_print: bool = False, settings=None, ): @@ -184,7 +188,7 @@ def get_rendered_template( letter_head.footer, {"doc": doc.as_dict()} ) - convert_markdown(doc, meta) + convert_markdown(doc) args = {} # extract `print_heading_template` from the first field and remove it @@ -257,9 +261,9 @@ def set_title_values_for_table_and_multiselect_fields(meta, doc): set_title_values_for_link_and_dynamic_link_fields(_meta, value, doc) -def convert_markdown(doc, meta): +def convert_markdown(doc: "Document"): """Convert text field values to markdown if necessary""" - for field in meta.fields: + for field in doc.meta.fields: if field.fieldtype == "Text Editor": value = doc.get(field.fieldname) if value and "" in value: @@ -268,34 +272,30 @@ def convert_markdown(doc, meta): @frappe.whitelist() def get_html_and_style( - doc, - name=None, - print_format=None, - meta=None, - no_letterhead=None, - letterhead=None, - trigger_print=False, - style=None, - settings=None, - templates=None, + doc: str, + name: str = None, + print_format: str = None, + no_letterhead: bool = None, + letterhead: str = None, + trigger_print: bool = False, + style: str = None, + settings: str = None, ): """Returns `html` and `style` of print format, used in PDF etc""" - if isinstance(doc, str) and isinstance(name, str): - doc = frappe.get_doc(doc, name) + if isinstance(name, str): + document = frappe.get_doc(doc, name) + else: + document = frappe.get_doc(json.loads(doc)) - if isinstance(doc, str): - doc = frappe.get_doc(json.loads(doc)) - - print_format = get_print_format_doc(print_format, meta=meta or frappe.get_meta(doc.doctype)) - set_link_titles(doc) + print_format = get_print_format_doc(print_format, meta=document.meta) + set_link_titles(document) try: html = get_rendered_template( - doc, - name=name, + doc=document, print_format=print_format, - meta=meta, + meta=document.meta, no_letterhead=no_letterhead, letterhead=letterhead, trigger_print=trigger_print, @@ -309,16 +309,15 @@ def get_html_and_style( @frappe.whitelist() -def get_rendered_raw_commands(doc, name=None, print_format=None, meta=None, lang=None): +def get_rendered_raw_commands(doc: str, name: str = None, print_format: str = None): """Returns Rendered Raw Commands of print format, used to send directly to printer""" - if isinstance(doc, str) and isinstance(name, str): - doc = frappe.get_doc(doc, name) + if isinstance(name, str): + document = frappe.get_doc(doc, name) + else: + document = frappe.get_doc(json.loads(doc)) - if isinstance(doc, str): - doc = frappe.get_doc(json.loads(doc)) - - print_format = get_print_format_doc(print_format, meta=meta or frappe.get_meta(doc.doctype)) + print_format = get_print_format_doc(print_format, meta=document.meta) if not print_format or (print_format and not print_format.raw_printing): frappe.throw( @@ -326,7 +325,9 @@ def get_rendered_raw_commands(doc, name=None, print_format=None, meta=None, lang ) return { - "raw_commands": get_rendered_template(doc, name=name, print_format=print_format, meta=meta) + "raw_commands": get_rendered_template( + doc=document, name=name, print_format=print_format, meta=document.meta + ) } @@ -361,7 +362,7 @@ def validate_key(key, doc): raise frappe.exceptions.InvalidKeyError -def get_letter_head(doc, no_letterhead, letterhead=None): +def get_letter_head(doc: "Document", no_letterhead: bool, letterhead: str = None): if no_letterhead: return {} if letterhead: @@ -519,7 +520,9 @@ def has_value(df, doc): return True -def get_print_style(style=None, print_format=None, for_legacy=False): +def get_print_style( + style: str = None, print_format: "PrintFormat" = None, for_legacy: bool = False +): print_settings = frappe.get_doc("Print Settings") if not style: diff --git a/frappe/www/search.py b/frappe/www/search.py index 8eac7b5cd6..c59e64cf23 100644 --- a/frappe/www/search.py +++ b/frappe/www/search.py @@ -20,7 +20,7 @@ def get_context(context): @frappe.whitelist(allow_guest=True) -def get_search_results(text, scope=None, start=0, as_html=False): +def get_search_results(text: str, scope: str = None, start: int = 0, as_html: bool = False): results = web_search(text, scope, start, limit=21) out = frappe._dict() diff --git a/frappe/www/third_party_apps.py b/frappe/www/third_party_apps.py index e5682961af..ce65a88eb7 100644 --- a/frappe/www/third_party_apps.py +++ b/frappe/www/third_party_apps.py @@ -55,7 +55,7 @@ def get_first_login(client): @frappe.whitelist() -def delete_client(client_id): +def delete_client(client_id: str): active_client_id_tokens = frappe.get_all( "OAuth Bearer Token", filters=[["user", "=", frappe.session.user], ["client", "=", client_id]] ) From d8cd1be23b3fcbd24a51c815dc7125fdcce0fed7 Mon Sep 17 00:00:00 2001 From: Gavin D'souza Date: Tue, 29 Nov 2022 15:05:03 +0530 Subject: [PATCH 06/26] fix: Skip type checking if any allowed type is stringified --- frappe/utils/typing_validations.py | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/frappe/utils/typing_validations.py b/frappe/utils/typing_validations.py index b751a37839..e5742df8ab 100644 --- a/frappe/utils/typing_validations.py +++ b/frappe/utils/typing_validations.py @@ -1,5 +1,5 @@ from inspect import _empty, isclass, signature -from typing import Callable, ForwardRef, Sequence, Union +from typing import Callable, ForwardRef, Union def qualified_name(obj) -> str: @@ -48,11 +48,8 @@ def validate_argument_types(func: Callable, args: tuple, kwargs: dict): # if the type is a ForwardRef or str, ignore it if isinstance(current_arg_type, (ForwardRef, str)): continue - - if isinstance(current_arg_type, Sequence): - current_arg_type = tuple( - x for x in current_arg_type.__args__ if not isinstance(x, (ForwardRef, str)) - ) + elif any(isinstance(x, (ForwardRef, str)) for x in getattr(current_arg_type, "__args__", [])): + continue param_def = func_params.get(current_arg) From 3aa1d61f0de67478cc3126d35867f18d7431a1d8 Mon Sep 17 00:00:00 2001 From: Gavin D'souza Date: Tue, 29 Nov 2022 17:44:15 +0530 Subject: [PATCH 07/26] fix(whitelisted)!: Raise TypeError instead of ValidationError for unaccepted param types --- frappe/app.py | 3 +++ frappe/core/doctype/user/test_user.py | 4 +--- frappe/tests/test_rename_doc.py | 2 +- 3 files changed, 5 insertions(+), 4 deletions(-) diff --git a/frappe/app.py b/frappe/app.py index 2fe9991c4c..c863d86f95 100644 --- a/frappe/app.py +++ b/frappe/app.py @@ -245,6 +245,9 @@ def handle_exception(e): # usually. frappe.session.user = "Guest" + if isinstance(e, TypeError): + http_status_code = 417 + if respond_as_json: # handle ajax responses first # if the request is ajax, send back the trace or error message diff --git a/frappe/core/doctype/user/test_user.py b/frappe/core/doctype/user/test_user.py index 281a9f775d..ef4b171e8e 100644 --- a/frappe/core/doctype/user/test_user.py +++ b/frappe/core/doctype/user/test_user.py @@ -383,9 +383,7 @@ class TestUser(FrappeTestCase): # reset password update_password(old_password, old_password=new_password) - self.assertRaisesRegex( - frappe.exceptions.ValidationError, "Invalid key type", update_password, "test", 1, ["like", "%"] - ) + self.assertRaises(TypeError, update_password, "test", 1, ["like", "%"]) password_strength_response = { "feedback": {"password_policy_validation_passed": False, "suggestions": ["Fix password"]} diff --git a/frappe/tests/test_rename_doc.py b/frappe/tests/test_rename_doc.py index ec3020828e..d85efa79bb 100644 --- a/frappe/tests/test_rename_doc.py +++ b/frappe/tests/test_rename_doc.py @@ -223,7 +223,7 @@ class TestRenameDoc(FrappeTestCase): new_name = f"{dn}-new" # pass invalid types to API - with self.assertRaises(ValidationError): + with self.assertRaises(TypeError): update_document_title(doctype=dt, docname=dn, title={}, name={"hack": "this"}) doc_before = frappe.get_doc(test_doctype, dn) From d78b967f8d9432c69f41b736e773600bfc36f2f3 Mon Sep 17 00:00:00 2001 From: Gavin D'souza Date: Tue, 29 Nov 2022 17:47:09 +0530 Subject: [PATCH 08/26] test: Run type validation checks in test mode for whitelisted APIs --- frappe/__init__.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/frappe/__init__.py b/frappe/__init__.py index 6b9e282f56..6e98b46f04 100644 --- a/frappe/__init__.py +++ b/frappe/__init__.py @@ -725,8 +725,9 @@ def apply_validate_argument_types_wrapper(func): :param kwargs: Function keyword arguments.""" from frappe.utils.typing_validations import validate_argument_types - if getattr(local, "request", None): + if getattr(local, "request", None) or local.flags.in_test: validate_argument_types(func, args, kwargs) + return func(*args, **kwargs) return wrapper From cff0567fafa61b3068ffee3f79e3aadb6febcf78 Mon Sep 17 00:00:00 2001 From: Gavin D'souza Date: Tue, 29 Nov 2022 17:47:45 +0530 Subject: [PATCH 09/26] fix(print): Check doc permission before checking related info --- frappe/www/printview.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/frappe/www/printview.py b/frappe/www/printview.py index 8a0ce8fd2c..aa14f37b0a 100644 --- a/frappe/www/printview.py +++ b/frappe/www/printview.py @@ -288,6 +288,8 @@ def get_html_and_style( else: document = frappe.get_doc(json.loads(doc)) + document.check_permission() + print_format = get_print_format_doc(print_format, meta=document.meta) set_link_titles(document) @@ -317,6 +319,8 @@ def get_rendered_raw_commands(doc: str, name: str = None, print_format: str = No else: document = frappe.get_doc(json.loads(doc)) + document.check_permission() + print_format = get_print_format_doc(print_format, meta=document.meta) if not print_format or (print_format and not print_format.raw_printing): From eb4aa0a1f30422b32fd86e7a6e42dc36e0444c17 Mon Sep 17 00:00:00 2001 From: Gavin D'souza Date: Tue, 29 Nov 2022 19:01:24 +0530 Subject: [PATCH 10/26] fix(handler): Use signature to fetch doc method structure --- frappe/handler.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/frappe/handler.py b/frappe/handler.py index cee6d3fbde..65a7e7695b 100644 --- a/frappe/handler.py +++ b/frappe/handler.py @@ -269,7 +269,7 @@ def ping(): def run_doc_method(method, docs=None, dt=None, dn=None, arg=None, args=None): """run a whitelisted controller method""" - from inspect import getfullargspec + from inspect import signature if not args and arg: args = arg @@ -298,7 +298,7 @@ def run_doc_method(method, docs=None, dt=None, dn=None, arg=None, args=None): is_whitelisted(fn) is_valid_http_method(fn) - fnargs = getfullargspec(method_obj).args + fnargs = list(signature(method_obj).parameters) if not fnargs or (len(fnargs) == 1 and fnargs[0] == "self"): response = doc.run_method(method) From f3250808f06c65db0da6c180b61be9be0884b59b Mon Sep 17 00:00:00 2001 From: Gavin D'souza Date: Tue, 29 Nov 2022 19:43:00 +0530 Subject: [PATCH 11/26] fix: Use typeguard to handle base hints Pre process exceptions that Frappe requires and pass the ruleset to typeguard's check_type API --- frappe/utils/typing_validations.py | 8 ++++---- pyproject.toml | 1 + 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/frappe/utils/typing_validations.py b/frappe/utils/typing_validations.py index e5742df8ab..b470446d26 100644 --- a/frappe/utils/typing_validations.py +++ b/frappe/utils/typing_validations.py @@ -1,6 +1,8 @@ from inspect import _empty, isclass, signature from typing import Callable, ForwardRef, Union +from typeguard import check_type + def qualified_name(obj) -> str: """ @@ -53,6 +55,7 @@ def validate_argument_types(func: Callable, args: tuple, kwargs: dict): param_def = func_params.get(current_arg) + # add default value's type in acceptable types if param_def.default is not _empty: if isinstance(current_arg_type, tuple): if param_def.default not in current_arg_type: @@ -60,7 +63,4 @@ def validate_argument_types(func: Callable, args: tuple, kwargs: dict): elif param_def.default != current_arg_type: current_arg_type = Union[current_arg_type, type(param_def.default)] - if not isinstance(current_arg_value, current_arg_type): - raise TypeError( - f"Argument '{current_arg}' must be of type '{qualified_name(current_arg_type)}' but got '{qualified_name(current_arg_value)}'" - ) + check_type(current_arg, current_arg_value, current_arg_type) diff --git a/pyproject.toml b/pyproject.toml index 66fef2160a..e2dbfd5443 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -66,6 +66,7 @@ dependencies = [ "tenacity~=8.0.1", "terminaltables~=3.1.0", "traceback-with-variables~=2.0.4", + "typeguard~=2.13.3", "xlrd~=2.0.1", "zxcvbn-python~=4.4.24", "markdownify~=0.11.2", From 8e125f577e520966af2d69550b978f83a45d8eb9 Mon Sep 17 00:00:00 2001 From: Gavin D'souza Date: Tue, 29 Nov 2022 20:13:39 +0530 Subject: [PATCH 12/26] fix(type-check): Allow some slack for bool to be int or float --- frappe/utils/typing_validations.py | 25 +++++++------------------ 1 file changed, 7 insertions(+), 18 deletions(-) diff --git a/frappe/utils/typing_validations.py b/frappe/utils/typing_validations.py index b470446d26..97d702cdd5 100644 --- a/frappe/utils/typing_validations.py +++ b/frappe/utils/typing_validations.py @@ -3,24 +3,9 @@ from typing import Callable, ForwardRef, Union from typeguard import check_type - -def qualified_name(obj) -> str: - """ - Return the qualified name (e.g. package.module.Type) for the given object. - - Builtins and types from the :mod:`typing` package get special treatment by having the module - name stripped from the generated name. - - """ - discovered_type = obj if isclass(obj) else type(obj) - module, qualname = discovered_type.__module__, discovered_type.__qualname__ - - if module == "typing": - return obj - elif module in {"types", "builtins"}: - return qualname - else: - return f"{module}.{qualname}" +SLACK_DICT = { + bool: (int, bool, float), +} def validate_argument_types(func: Callable, args: tuple, kwargs: dict): @@ -53,6 +38,10 @@ def validate_argument_types(func: Callable, args: tuple, kwargs: dict): elif any(isinstance(x, (ForwardRef, str)) for x in getattr(current_arg_type, "__args__", [])): continue + # allow slack for Frappe types + if current_arg_type in SLACK_DICT: + current_arg_type = SLACK_DICT[current_arg_type] + param_def = func_params.get(current_arg) # add default value's type in acceptable types From 41d6c791f1893205daf963eb7332af35253ec1e7 Mon Sep 17 00:00:00 2001 From: Gavin D'souza Date: Tue, 29 Nov 2022 20:14:13 +0530 Subject: [PATCH 13/26] fix: Build args dict correctly --- frappe/utils/typing_validations.py | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/frappe/utils/typing_validations.py b/frappe/utils/typing_validations.py index 97d702cdd5..b7708b0a52 100644 --- a/frappe/utils/typing_validations.py +++ b/frappe/utils/typing_validations.py @@ -17,9 +17,17 @@ def validate_argument_types(func: Callable, args: tuple, kwargs: dict): if annotations := func.__annotations__: # generate kwargs dict from args arg_names = func.__code__.co_varnames[: func.__code__.co_argcount] - arg_values = args or func.__defaults__ or [] - prepared_args = dict(zip(arg_names, arg_values)) - prepared_args.update(kwargs) + + if not args: + prepared_args = kwargs + + elif kwargs: + arg_values = args or func.__defaults__ or [] + prepared_args = dict(zip(arg_names, arg_values)) + prepared_args.update(kwargs) + + else: + prepared_args = dict(zip(arg_names, args)) # check if type hints dont match the default values func_signature = signature(func) From 2327a56abccd3a5564ff1067324bd5cb14737308 Mon Sep 17 00:00:00 2001 From: Gavin D'souza Date: Wed, 30 Nov 2022 12:24:38 +0530 Subject: [PATCH 14/26] fix(File): Correct acceptable types in APIs - Allow str types for start, page_length in page_length API - Allow str, list[dict] file_list in move_file API --- frappe/core/api/file.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/frappe/core/api/file.py b/frappe/core/api/file.py index ec305aff4f..9b275ed35a 100644 --- a/frappe/core/api/file.py +++ b/frappe/core/api/file.py @@ -39,7 +39,7 @@ def get_attached_images(doctype: str, names: list[str]) -> frappe._dict: @frappe.whitelist() -def get_files_in_folder(folder: str, start: int = 0, page_length: int = 20) -> dict: +def get_files_in_folder(folder: str, start: int | str = 0, page_length: int | str = 20) -> dict: start = cint(start) page_length = cint(page_length) @@ -101,10 +101,11 @@ def create_new_folder(file_name: str, folder: str) -> File: @frappe.whitelist() -def move_file(file_list: list[File], new_parent: str, old_parent: str) -> None: +def move_file(file_list: list[File | dict] | str, new_parent: str, old_parent: str) -> None: if isinstance(file_list, str): file_list = json.loads(file_list) + # will check for permission on each file & update parent for file_obj in file_list: setup_folder_path(file_obj.get("name"), new_parent) From 73b0971a268f7f97022e8f9926d6a0fe9f7bbf09 Mon Sep 17 00:00:00 2001 From: Gavin D'souza Date: Wed, 30 Nov 2022 17:00:37 +0530 Subject: [PATCH 15/26] test: Add tests for typing validations --- frappe/core/doctype/report/report.py | 3 ++- frappe/tests/test_utils.py | 25 +++++++++++++++++++++++++ frappe/utils/typing_validations.py | 2 +- 3 files changed, 28 insertions(+), 2 deletions(-) diff --git a/frappe/core/doctype/report/report.py b/frappe/core/doctype/report/report.py index ae862184b6..e87292f55c 100644 --- a/frappe/core/doctype/report/report.py +++ b/frappe/core/doctype/report/report.py @@ -2,6 +2,7 @@ # License: MIT. See LICENSE import datetime import json +from typing import Union import frappe import frappe.desk.query_report @@ -326,7 +327,7 @@ class Report(Document): return data @frappe.whitelist() - def toggle_disable(self, disable): + def toggle_disable(self, disable: bool | str | int): if not self.has_permission("write"): frappe.throw(_("You are not allowed to edit the report.")) diff --git a/frappe/tests/test_utils.py b/frappe/tests/test_utils.py index cf6134d101..2e22b28731 100644 --- a/frappe/tests/test_utils.py +++ b/frappe/tests/test_utils.py @@ -919,3 +919,28 @@ class TestMiscUtils(FrappeTestCase): self.assertEqual(safe_json_loads("{}"), {}) self.assertEqual(safe_json_loads("{ /}"), "{ /}") self.assertEqual(safe_json_loads("12"), 12) # this is a quirk + + +class TestTypingValidations(FrappeTestCase): + ERR_REGEX = "^type of .* must be .*; got (object|list) instead$" + + def test_validate_whitelisted_api(self): + from inspect import signature + + whitelisted_fn = next(x for x in frappe.whitelisted if x.__annotations__) + bad_params = (object(),) * len(signature(whitelisted_fn).parameters) + + with self.assertRaisesRegex(TypeError, self.ERR_REGEX): + whitelisted_fn(*bad_params) + + def test_validate_whitelisted_doc_method(self): + report = frappe.get_last_doc("Report") + + with self.assertRaisesRegex(TypeError, self.ERR_REGEX): + report.toggle_disable(["disable"]) + + current_value = report.disabled + changed_value = not current_value + + report.toggle_disable(changed_value) + report.toggle_disable(current_value) diff --git a/frappe/utils/typing_validations.py b/frappe/utils/typing_validations.py index b7708b0a52..b0eae6b7b8 100644 --- a/frappe/utils/typing_validations.py +++ b/frappe/utils/typing_validations.py @@ -1,4 +1,4 @@ -from inspect import _empty, isclass, signature +from inspect import _empty, signature from typing import Callable, ForwardRef, Union from typeguard import check_type From 4fe260e09e5a5cad0b675b0648fc67b382e424cf Mon Sep 17 00:00:00 2001 From: Gavin D'souza Date: Thu, 1 Dec 2022 11:38:44 +0530 Subject: [PATCH 16/26] refactor: transform_parameter_types - Switch to Pydantic which is under continuous development and can support more types - Equivalent Pydantic API will try to transform data if possible - The previous point makes it such that we don't need to explicitly try to parse each stringified int in app code since Pydantic can do this - Drop typeguard since it did not handle 3.10+ native typing definitions --- frappe/__init__.py | 4 +- frappe/tests/test_utils.py | 2 +- frappe/utils/typing_validations.py | 63 ++++++++++++++++++++++++++++-- pyproject.toml | 2 +- 4 files changed, 63 insertions(+), 8 deletions(-) diff --git a/frappe/__init__.py b/frappe/__init__.py index 6e98b46f04..fff4f475f1 100644 --- a/frappe/__init__.py +++ b/frappe/__init__.py @@ -723,10 +723,10 @@ def apply_validate_argument_types_wrapper(func): :param args: Function arguments. :param kwargs: Function keyword arguments.""" - from frappe.utils.typing_validations import validate_argument_types + from frappe.utils.typing_validations import transform_parameter_types if getattr(local, "request", None) or local.flags.in_test: - validate_argument_types(func, args, kwargs) + args, kwargs = transform_parameter_types(func, args, kwargs) return func(*args, **kwargs) diff --git a/frappe/tests/test_utils.py b/frappe/tests/test_utils.py index 2e22b28731..cd03e8285f 100644 --- a/frappe/tests/test_utils.py +++ b/frappe/tests/test_utils.py @@ -922,7 +922,7 @@ class TestMiscUtils(FrappeTestCase): class TestTypingValidations(FrappeTestCase): - ERR_REGEX = "^type of .* must be .*; got (object|list) instead$" + ERR_REGEX = f"^Argument '.*' should be of type '.*' but got '.*' instead.$" def test_validate_whitelisted_api(self): from inspect import signature diff --git a/frappe/utils/typing_validations.py b/frappe/utils/typing_validations.py index b0eae6b7b8..4967882db5 100644 --- a/frappe/utils/typing_validations.py +++ b/frappe/utils/typing_validations.py @@ -1,20 +1,57 @@ -from inspect import _empty, signature +from inspect import _empty, isclass, signature +from types import EllipsisType from typing import Callable, ForwardRef, Union -from typeguard import check_type +from pydantic import parse_obj_as +from pydantic.error_wrappers import ValidationError as PyValidationError SLACK_DICT = { bool: (int, bool, float), } -def validate_argument_types(func: Callable, args: tuple, kwargs: dict): +def qualified_name(obj) -> str: + """ + Return the qualified name (e.g. package.module.Type) for the given object. + + Builtins and types from the :mod:typing package get special treatment by having the module + name stripped from the generated name. + + """ + discovered_type = obj if isclass(obj) else type(obj) + module, qualname = discovered_type.__module__, discovered_type.__qualname__ + + if module in {"typing", "types"}: + return obj + elif module in {"builtins"}: + return qualname + else: + return f"{module}.{qualname}" + + +def raise_type_error( + arg_name: str, arg_type: type, arg_value: object, current_exception: Exception = None +): + """ + Raise a TypeError with a message that includes the name of the argument, the expected type + and the actual type of the value passed. + + """ + raise TypeError( + f"Argument '{arg_name}' should be of type '{qualified_name(arg_type)}' but got " + f"'{qualified_name(arg_value)}' instead." + ) from current_exception + + +def transform_parameter_types(func: Callable, args: tuple, kwargs: dict): """ Validate the types of the arguments passed to a function with the type annotations defined on the function. """ if annotations := func.__annotations__: + new_args, new_kwargs = list(args), kwargs + # generate kwargs dict from args arg_names = func.__code__.co_varnames[: func.__code__.co_argcount] @@ -60,4 +97,22 @@ def validate_argument_types(func: Callable, args: tuple, kwargs: dict): elif param_def.default != current_arg_type: current_arg_type = Union[current_arg_type, type(param_def.default)] - check_type(current_arg, current_arg_value, current_arg_type) + try: + current_arg_value_after = parse_obj_as( + current_arg_type, current_arg_value, type_name=current_arg + ) + except PyValidationError as e: + raise_type_error(current_arg, current_arg_type, current_arg_value, current_exception=e) + + if isinstance(current_arg_value_after, EllipsisType): + raise_type_error(current_arg, current_arg_type, current_arg_value) + + else: + if current_arg in kwargs: + new_kwargs[current_arg] = current_arg_value_after + else: + new_args[arg_names.index(current_arg)] = current_arg_value_after + + return new_args, new_kwargs + + return args, kwargs diff --git a/pyproject.toml b/pyproject.toml index e2dbfd5443..6b9be6e90a 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -51,6 +51,7 @@ dependencies = [ "psycopg2-binary~=2.9.1", "pyOpenSSL~=22.1.0", "pycryptodome~=3.10.1", + "pydantic~=1.10.2", "pyotp~=2.6.0", "python-dateutil~=2.8.1", "pytz==2022.1", @@ -66,7 +67,6 @@ dependencies = [ "tenacity~=8.0.1", "terminaltables~=3.1.0", "traceback-with-variables~=2.0.4", - "typeguard~=2.13.3", "xlrd~=2.0.1", "zxcvbn-python~=4.4.24", "markdownify~=0.11.2", From 299a017081471ff05095a424fa682e197dad6222 Mon Sep 17 00:00:00 2001 From: Gavin D'souza Date: Thu, 1 Dec 2022 12:31:36 +0530 Subject: [PATCH 17/26] fix(type-check): Parse and validate arbitrary objects --- frappe/utils/typing_validations.py | 39 +++++++++++++++++++++++++++--- 1 file changed, 36 insertions(+), 3 deletions(-) diff --git a/frappe/utils/typing_validations.py b/frappe/utils/typing_validations.py index 4967882db5..85f516be44 100644 --- a/frappe/utils/typing_validations.py +++ b/frappe/utils/typing_validations.py @@ -1,13 +1,20 @@ +from functools import lru_cache from inspect import _empty, isclass, signature from types import EllipsisType -from typing import Callable, ForwardRef, Union +from typing import Any, Callable, ForwardRef, Type, TypeVar, Union -from pydantic import parse_obj_as +from pydantic.config import BaseConfig from pydantic.error_wrappers import ValidationError as PyValidationError +from pydantic.tools import NameFactory, _generate_parsing_type_name SLACK_DICT = { bool: (int, bool, float), } +T = TypeVar("T") + + +class FrappePydanticConfig: + arbitrary_types_allowed = True def qualified_name(obj) -> str: @@ -43,6 +50,32 @@ def raise_type_error( ) from current_exception +@lru_cache(maxsize=2048) +def _get_parsing_type( + type_: Any, *, type_name: NameFactory | None = None, config: type[BaseConfig] = None +) -> Any: + # Note: this is a copy of pydantic.tools._get_parsing_type with the addition of allowing a config argument + from pydantic.main import create_model + + if type_name is None: + type_name = _generate_parsing_type_name + if not isinstance(type_name, str): + type_name = type_name(type_) + return create_model(type_name, __root__=(type_, ...), __config__=config) + + +def parse_obj_as( + type_: type[T], + obj: Any, + *, + type_name: NameFactory | None = None, + config: type[BaseConfig] | None = None, +) -> T: + # Note: This is a copy of pydantic.tools.parse_obj_as with the addition of allowing a config argument + model_type = _get_parsing_type(type_, type_name=type_name, config=config) # type: ignore[arg-type] + return model_type(__root__=obj).__root__ + + def transform_parameter_types(func: Callable, args: tuple, kwargs: dict): """ Validate the types of the arguments passed to a function with the type annotations @@ -99,7 +132,7 @@ def transform_parameter_types(func: Callable, args: tuple, kwargs: dict): try: current_arg_value_after = parse_obj_as( - current_arg_type, current_arg_value, type_name=current_arg + current_arg_type, current_arg_value, type_name=current_arg, config=FrappePydanticConfig ) except PyValidationError as e: raise_type_error(current_arg, current_arg_type, current_arg_value, current_exception=e) From 579aa124e12e3140e5d044379e1c2603fb62ec50 Mon Sep 17 00:00:00 2001 From: Gavin D'souza Date: Thu, 1 Dec 2022 14:54:47 +0530 Subject: [PATCH 18/26] fix(type-check): Convert tuple types to Union --- frappe/utils/typing_validations.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/frappe/utils/typing_validations.py b/frappe/utils/typing_validations.py index 85f516be44..ade5fcaa06 100644 --- a/frappe/utils/typing_validations.py +++ b/frappe/utils/typing_validations.py @@ -125,8 +125,10 @@ def transform_parameter_types(func: Callable, args: tuple, kwargs: dict): # add default value's type in acceptable types if param_def.default is not _empty: if isinstance(current_arg_type, tuple): - if param_def.default not in current_arg_type: + if type(param_def.default) not in current_arg_type: current_arg_type += (type(param_def.default),) + current_arg_type = Union[current_arg_type] + elif param_def.default != current_arg_type: current_arg_type = Union[current_arg_type, type(param_def.default)] From ec931004caac440053c8a5ca2bc1057f0e7cf720 Mon Sep 17 00:00:00 2001 From: Gavin D'souza Date: Thu, 1 Dec 2022 14:58:16 +0530 Subject: [PATCH 19/26] refactor: transform_parameter_types Failfast if args and kwargs or annotations don't exist --- frappe/utils/typing_validations.py | 130 +++++++++++++++-------------- 1 file changed, 67 insertions(+), 63 deletions(-) diff --git a/frappe/utils/typing_validations.py b/frappe/utils/typing_validations.py index ade5fcaa06..a2a0331396 100644 --- a/frappe/utils/typing_validations.py +++ b/frappe/utils/typing_validations.py @@ -82,72 +82,76 @@ def transform_parameter_types(func: Callable, args: tuple, kwargs: dict): defined on the function. """ - if annotations := func.__annotations__: - new_args, new_kwargs = list(args), kwargs + if not (args or kwargs) or not func.__annotations__: + return args, kwargs - # generate kwargs dict from args - arg_names = func.__code__.co_varnames[: func.__code__.co_argcount] + annotations = func.__annotations__ + new_args, new_kwargs = list(args), kwargs - if not args: - prepared_args = kwargs + # generate kwargs dict from args + arg_names = func.__code__.co_varnames[: func.__code__.co_argcount] - elif kwargs: - arg_values = args or func.__defaults__ or [] - prepared_args = dict(zip(arg_names, arg_values)) - prepared_args.update(kwargs) + if not args: + prepared_args = kwargs + elif kwargs: + arg_values = args or func.__defaults__ or [] + prepared_args = dict(zip(arg_names, arg_values)) + prepared_args.update(kwargs) + + else: + prepared_args = dict(zip(arg_names, args)) + + # check if type hints dont match the default values + func_signature = signature(func) + func_params = dict(func_signature.parameters) + + # check if the argument types are correct + for current_arg, current_arg_type in annotations.items(): + if current_arg not in prepared_args: + continue + + current_arg_value = prepared_args[current_arg] + + # if the type is a ForwardRef or str, ignore it + if isinstance(current_arg_type, (ForwardRef, str)): + continue + elif any(isinstance(x, (ForwardRef, str)) for x in getattr(current_arg_type, "__args__", [])): + continue + + # allow slack for Frappe types + if current_arg_type in SLACK_DICT: + current_arg_type = SLACK_DICT[current_arg_type] + + param_def = func_params.get(current_arg) + + # add default value's type in acceptable types + if param_def.default is not _empty: + if isinstance(current_arg_type, tuple): + if type(param_def.default) not in current_arg_type: + current_arg_type += (type(param_def.default),) + current_arg_type = Union[current_arg_type] + + elif param_def.default != current_arg_type: + current_arg_type = Union[current_arg_type, type(param_def.default)] + elif isinstance(current_arg_type, tuple): + current_arg_type = Union[current_arg_type] + + # validate the type set using pydantic - raise a TypeError if Validation is raised or Ellipsis is returned + try: + current_arg_value_after = parse_obj_as( + current_arg_type, current_arg_value, type_name=current_arg, config=FrappePydanticConfig + ) + except PyValidationError as e: + raise_type_error(current_arg, current_arg_type, current_arg_value, current_exception=e) + + if isinstance(current_arg_value_after, EllipsisType): + raise_type_error(current_arg, current_arg_type, current_arg_value) + + # update the args and kwargs with possibly casted value + if current_arg in kwargs: + new_kwargs[current_arg] = current_arg_value_after else: - prepared_args = dict(zip(arg_names, args)) + new_args[arg_names.index(current_arg)] = current_arg_value_after - # check if type hints dont match the default values - func_signature = signature(func) - func_params = dict(func_signature.parameters) - - # check if the argument types are correct - for current_arg, current_arg_type in annotations.items(): - if current_arg not in prepared_args: - continue - - current_arg_value = prepared_args[current_arg] - - # if the type is a ForwardRef or str, ignore it - if isinstance(current_arg_type, (ForwardRef, str)): - continue - elif any(isinstance(x, (ForwardRef, str)) for x in getattr(current_arg_type, "__args__", [])): - continue - - # allow slack for Frappe types - if current_arg_type in SLACK_DICT: - current_arg_type = SLACK_DICT[current_arg_type] - - param_def = func_params.get(current_arg) - - # add default value's type in acceptable types - if param_def.default is not _empty: - if isinstance(current_arg_type, tuple): - if type(param_def.default) not in current_arg_type: - current_arg_type += (type(param_def.default),) - current_arg_type = Union[current_arg_type] - - elif param_def.default != current_arg_type: - current_arg_type = Union[current_arg_type, type(param_def.default)] - - try: - current_arg_value_after = parse_obj_as( - current_arg_type, current_arg_value, type_name=current_arg, config=FrappePydanticConfig - ) - except PyValidationError as e: - raise_type_error(current_arg, current_arg_type, current_arg_value, current_exception=e) - - if isinstance(current_arg_value_after, EllipsisType): - raise_type_error(current_arg, current_arg_type, current_arg_value) - - else: - if current_arg in kwargs: - new_kwargs[current_arg] = current_arg_value_after - else: - new_args[arg_names.index(current_arg)] = current_arg_value_after - - return new_args, new_kwargs - - return args, kwargs + return new_args, new_kwargs From d978ed7d069d201d36464c002cb7abd1ffa84dc0 Mon Sep 17 00:00:00 2001 From: Gavin D'souza Date: Thu, 15 Dec 2022 12:55:43 +0530 Subject: [PATCH 20/26] refactor: Raise FrappeTypeError in case of type mismatches --- frappe/app.py | 3 --- frappe/exceptions.py | 4 ++++ frappe/utils/typing_validations.py | 8 +++++--- 3 files changed, 9 insertions(+), 6 deletions(-) diff --git a/frappe/app.py b/frappe/app.py index c863d86f95..2fe9991c4c 100644 --- a/frappe/app.py +++ b/frappe/app.py @@ -245,9 +245,6 @@ def handle_exception(e): # usually. frappe.session.user = "Guest" - if isinstance(e, TypeError): - http_status_code = 417 - if respond_as_json: # handle ajax responses first # if the request is ajax, send back the trace or error message diff --git a/frappe/exceptions.py b/frappe/exceptions.py index 2fe9de6be9..f09583e215 100644 --- a/frappe/exceptions.py +++ b/frappe/exceptions.py @@ -15,6 +15,10 @@ class ValidationError(Exception): http_status_code = 417 +class FrappeTypeError(TypeError): + http_status_code = 417 + + class AuthenticationError(Exception): http_status_code = 401 diff --git a/frappe/utils/typing_validations.py b/frappe/utils/typing_validations.py index a2a0331396..f9773c2c4e 100644 --- a/frappe/utils/typing_validations.py +++ b/frappe/utils/typing_validations.py @@ -1,12 +1,14 @@ from functools import lru_cache from inspect import _empty, isclass, signature from types import EllipsisType -from typing import Any, Callable, ForwardRef, Type, TypeVar, Union +from typing import Any, Callable, ForwardRef, TypeVar, Union from pydantic.config import BaseConfig from pydantic.error_wrappers import ValidationError as PyValidationError from pydantic.tools import NameFactory, _generate_parsing_type_name +from frappe.exceptions import FrappeTypeError + SLACK_DICT = { bool: (int, bool, float), } @@ -44,7 +46,7 @@ def raise_type_error( and the actual type of the value passed. """ - raise TypeError( + raise FrappeTypeError( f"Argument '{arg_name}' should be of type '{qualified_name(arg_type)}' but got " f"'{qualified_name(arg_value)}' instead." ) from current_exception @@ -142,7 +144,7 @@ def transform_parameter_types(func: Callable, args: tuple, kwargs: dict): current_arg_value_after = parse_obj_as( current_arg_type, current_arg_value, type_name=current_arg, config=FrappePydanticConfig ) - except PyValidationError as e: + except (TypeError, PyValidationError) as e: raise_type_error(current_arg, current_arg_type, current_arg_value, current_exception=e) if isinstance(current_arg_value_after, EllipsisType): From 029a1fc902f64534112e206881f0dd3ab38969fd Mon Sep 17 00:00:00 2001 From: Gavin D'souza Date: Thu, 15 Dec 2022 13:25:47 +0530 Subject: [PATCH 21/26] chore: Remove loose types from fn definitions --- frappe/core/api/file.py | 5 +---- frappe/core/doctype/report/report.py | 3 +-- frappe/core/doctype/user/user.py | 2 +- 3 files changed, 3 insertions(+), 7 deletions(-) diff --git a/frappe/core/api/file.py b/frappe/core/api/file.py index 9b275ed35a..c2354516a8 100644 --- a/frappe/core/api/file.py +++ b/frappe/core/api/file.py @@ -39,10 +39,7 @@ def get_attached_images(doctype: str, names: list[str]) -> frappe._dict: @frappe.whitelist() -def get_files_in_folder(folder: str, start: int | str = 0, page_length: int | str = 20) -> dict: - start = cint(start) - page_length = cint(page_length) - +def get_files_in_folder(folder: str, start: int = 0, page_length: int = 20) -> dict: attachment_folder = frappe.db.get_value( "File", "Home/Attachments", diff --git a/frappe/core/doctype/report/report.py b/frappe/core/doctype/report/report.py index e87292f55c..ef38387e57 100644 --- a/frappe/core/doctype/report/report.py +++ b/frappe/core/doctype/report/report.py @@ -2,7 +2,6 @@ # License: MIT. See LICENSE import datetime import json -from typing import Union import frappe import frappe.desk.query_report @@ -327,7 +326,7 @@ class Report(Document): return data @frappe.whitelist() - def toggle_disable(self, disable: bool | str | int): + def toggle_disable(self, disable: bool): if not self.has_permission("write"): frappe.throw(_("You are not allowed to edit the report.")) diff --git a/frappe/core/doctype/user/user.py b/frappe/core/doctype/user/user.py index 65100fbefa..e68a55d1e8 100644 --- a/frappe/core/doctype/user/user.py +++ b/frappe/core/doctype/user/user.py @@ -679,7 +679,7 @@ def get_perm_info(role): @frappe.whitelist(allow_guest=True) def update_password( - new_password: str, logout_all_sessions: int | bool = 0, key: str = None, old_password: str = None + new_password: str, logout_all_sessions: int = 0, key: str = None, old_password: str = None ): """Update password for the current user. From 06cf18d0aa2646b04034373a4032b801fe241b3a Mon Sep 17 00:00:00 2001 From: Gavin D'souza Date: Thu, 15 Dec 2022 13:28:47 +0530 Subject: [PATCH 22/26] chore: Add optional typing hints for params with default None --- frappe/utils/global_search.py | 2 +- frappe/www/printview.py | 26 +++++++++++++------------- 2 files changed, 14 insertions(+), 14 deletions(-) diff --git a/frappe/utils/global_search.py b/frappe/utils/global_search.py index 9efa9de592..391533edc0 100644 --- a/frappe/utils/global_search.py +++ b/frappe/utils/global_search.py @@ -489,7 +489,7 @@ def search(text, start=0, limit=20, doctype=""): @frappe.whitelist(allow_guest=True) -def web_search(text: str, scope: str = None, start: int = 0, limit: int = 20): +def web_search(text: str, scope: str | None = None, start: int = 0, limit: int = 20): """ Search for given text in __global_search where published = 1 :param text: phrase to be searched diff --git a/frappe/www/printview.py b/frappe/www/printview.py index aa14f37b0a..56b5b14ece 100644 --- a/frappe/www/printview.py +++ b/frappe/www/printview.py @@ -5,7 +5,7 @@ import copy import json import os import re -from typing import TYPE_CHECKING +from typing import TYPE_CHECKING, Optional import frappe from frappe import _, get_module_path @@ -94,10 +94,10 @@ def get_print_format_doc(print_format_name, meta): def get_rendered_template( doc: "Document", - print_format: str = None, + print_format: str | None = None, meta=None, - no_letterhead: bool = None, - letterhead: str = None, + no_letterhead: bool | None = None, + letterhead: str | None = None, trigger_print: bool = False, settings=None, ): @@ -273,13 +273,13 @@ def convert_markdown(doc: "Document"): @frappe.whitelist() def get_html_and_style( doc: str, - name: str = None, - print_format: str = None, - no_letterhead: bool = None, - letterhead: str = None, + name: str | None = None, + print_format: str | None = None, + no_letterhead: bool | None = None, + letterhead: str | None = None, trigger_print: bool = False, - style: str = None, - settings: str = None, + style: str | None = None, + settings: str | None = None, ): """Returns `html` and `style` of print format, used in PDF etc""" @@ -311,7 +311,7 @@ def get_html_and_style( @frappe.whitelist() -def get_rendered_raw_commands(doc: str, name: str = None, print_format: str = None): +def get_rendered_raw_commands(doc: str, name: str | None = None, print_format: str | None = None): """Returns Rendered Raw Commands of print format, used to send directly to printer""" if isinstance(name, str): @@ -366,7 +366,7 @@ def validate_key(key, doc): raise frappe.exceptions.InvalidKeyError -def get_letter_head(doc: "Document", no_letterhead: bool, letterhead: str = None): +def get_letter_head(doc: "Document", no_letterhead: bool, letterhead: str | None = None): if no_letterhead: return {} if letterhead: @@ -525,7 +525,7 @@ def has_value(df, doc): def get_print_style( - style: str = None, print_format: "PrintFormat" = None, for_legacy: bool = False + style: str | None = None, print_format: Optional["PrintFormat"] = None, for_legacy: bool = False ): print_settings = frappe.get_doc("Print Settings") From 009936dbac08aeaa9e13ce47a53bc52d9e3f8e04 Mon Sep 17 00:00:00 2001 From: Gavin D'souza Date: Thu, 15 Dec 2022 13:37:11 +0530 Subject: [PATCH 23/26] fix: Pass _lang in get_raw_commands API call for Frappe WSGi Revert unintentional change --- frappe/printing/page/print/print.js | 1 + 1 file changed, 1 insertion(+) diff --git a/frappe/printing/page/print/print.js b/frappe/printing/page/print/print.js index 429b081df3..54704467c0 100644 --- a/frappe/printing/page/print/print.js +++ b/frappe/printing/page/print/print.js @@ -673,6 +673,7 @@ frappe.ui.form.PrintView = class { args: { doc: this.frm.doc, print_format: this.selected_format(), + _lang: this.lang_code, }, callback: function (r) { if (!r.exc) { From e3f82f0175f59254cd617f1dd622d2109d0a0b3e Mon Sep 17 00:00:00 2001 From: Gavin D'souza Date: Thu, 15 Dec 2022 14:11:44 +0530 Subject: [PATCH 24/26] fix: Maintain test_password_strength signature Add deprecation warning instead of removing params directly --- frappe/core/doctype/user/user.py | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/frappe/core/doctype/user/user.py b/frappe/core/doctype/user/user.py index e68a55d1e8..2460720bd8 100644 --- a/frappe/core/doctype/user/user.py +++ b/frappe/core/doctype/user/user.py @@ -537,7 +537,7 @@ class User(Document): if self.__new_password: user_data = (self.first_name, self.middle_name, self.last_name, self.email, self.birth_date) - result = test_password_strength(self.__new_password, user_data) + result = test_password_strength(self.__new_password, user_data=user_data) feedback = result.get("feedback", None) if feedback and not feedback.get("password_policy_validation_passed", False): @@ -728,9 +728,17 @@ def update_password( @frappe.whitelist(allow_guest=True) -def test_password_strength(new_password: str, user_data: tuple = None): +def test_password_strength( + new_password: str, key=None, old_password=None, user_data: tuple | None = None +): + from frappe.utils.deprecations import deprecation_warning from frappe.utils.password_strength import test_password_strength as _test_password_strength + if key is not None or old_password is not None: + deprecation_warning( + "Arguments `key` and `old_password` are deprecated in function `test_password_strength`." + ) + enable_password_policy = frappe.get_system_settings("enable_password_policy") or 0 if not enable_password_policy: From d66eed129cd8b89c571f349c3ba69068a0f5f4fa Mon Sep 17 00:00:00 2001 From: Gavin D'souza Date: Mon, 19 Dec 2022 12:15:35 +0530 Subject: [PATCH 25/26] refactor: validate_argument_types * Rename API for ease of public use * Add validation condition parameter * Move function to utils module instead of frappe namespace --- frappe/__init__.py | 26 +++++++------------------- frappe/utils/typing_validations.py | 18 +++++++++++++++++- 2 files changed, 24 insertions(+), 20 deletions(-) diff --git a/frappe/__init__.py b/frappe/__init__.py index fff4f475f1..9e919a5c08 100644 --- a/frappe/__init__.py +++ b/frappe/__init__.py @@ -716,23 +716,6 @@ xss_safe_methods = [] allowed_http_methods_for_whitelisted_func = {} -def apply_validate_argument_types_wrapper(func): - @functools.wraps(func) - def wrapper(*args, **kwargs): - """Validate argument types of whitelisted functions. - - :param args: Function arguments. - :param kwargs: Function keyword arguments.""" - from frappe.utils.typing_validations import transform_parameter_types - - if getattr(local, "request", None) or local.flags.in_test: - args, kwargs = transform_parameter_types(func, args, kwargs) - - return func(*args, **kwargs) - - return wrapper - - def whitelist(allow_guest=False, xss_safe=False, methods=None): """ Decorator for whitelisting a function and making it accessible via HTTP. @@ -752,16 +735,21 @@ def whitelist(allow_guest=False, xss_safe=False, methods=None): methods = ["GET", "POST", "PUT", "DELETE"] def innerfn(fn): + from frappe.utils.typing_validations import validate_argument_types + global whitelisted, guest_methods, xss_safe_methods, allowed_http_methods_for_whitelisted_func + # validate argument types only if request is present + in_request_or_test = lambda: getattr(local, "request", None) or local.flags.in_test # noqa: E731 + # get function from the unbound / bound method # this is needed because functions can be compared, but not methods method = None if hasattr(fn, "__func__"): - method = apply_validate_argument_types_wrapper(fn) + method = validate_argument_types(fn, apply_condition=in_request_or_test) fn = method.__func__ else: - fn = apply_validate_argument_types_wrapper(fn) + fn = validate_argument_types(fn, apply_condition=in_request_or_test) whitelisted.append(fn) allowed_http_methods_for_whitelisted_func[fn] = methods diff --git a/frappe/utils/typing_validations.py b/frappe/utils/typing_validations.py index f9773c2c4e..e7ebcfbdff 100644 --- a/frappe/utils/typing_validations.py +++ b/frappe/utils/typing_validations.py @@ -1,4 +1,4 @@ -from functools import lru_cache +from functools import lru_cache, wraps from inspect import _empty, isclass, signature from types import EllipsisType from typing import Any, Callable, ForwardRef, TypeVar, Union @@ -19,6 +19,22 @@ class FrappePydanticConfig: arbitrary_types_allowed = True +def validate_argument_types(func: Callable, apply_condition: Callable = lambda: True): + @wraps(func) + def wrapper(*args, **kwargs): + """Validate argument types of whitelisted functions. + + :param args: Function arguments. + :param kwargs: Function keyword arguments.""" + + if apply_condition(): + args, kwargs = transform_parameter_types(func, args, kwargs) + + return func(*args, **kwargs) + + return wrapper + + def qualified_name(obj) -> str: """ Return the qualified name (e.g. package.module.Type) for the given object. From b8da76d483147d9c0b6853436cee14509c6d0c05 Mon Sep 17 00:00:00 2001 From: Gavin D'souza Date: Mon, 19 Dec 2022 15:10:21 +0530 Subject: [PATCH 26/26] test(typing-utils): Specify exc type Co-authored-by: Ankush Menat --- frappe/tests/test_utils.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/frappe/tests/test_utils.py b/frappe/tests/test_utils.py index cd03e8285f..cd656db6cd 100644 --- a/frappe/tests/test_utils.py +++ b/frappe/tests/test_utils.py @@ -930,13 +930,13 @@ class TestTypingValidations(FrappeTestCase): whitelisted_fn = next(x for x in frappe.whitelisted if x.__annotations__) bad_params = (object(),) * len(signature(whitelisted_fn).parameters) - with self.assertRaisesRegex(TypeError, self.ERR_REGEX): + with self.assertRaisesRegex(frappe.FrappeTypeError, self.ERR_REGEX): whitelisted_fn(*bad_params) def test_validate_whitelisted_doc_method(self): report = frappe.get_last_doc("Report") - with self.assertRaisesRegex(TypeError, self.ERR_REGEX): + with self.assertRaisesRegex(frappe.FrappeTypeError, self.ERR_REGEX): report.toggle_disable(["disable"]) current_value = report.disabled