From 58645998c0dee47f644e2bc78be2091ed109dd59 Mon Sep 17 00:00:00 2001 From: Aradhya Date: Wed, 29 Jun 2022 15:03:37 +0530 Subject: [PATCH 01/80] fix: fixed fields with operators and added abs --- frappe/database/query.py | 10 +++++++++- frappe/query_builder/functions.py | 1 + 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/frappe/database/query.py b/frappe/database/query.py index 60eeaee8c2..10cb62b4a2 100644 --- a/frappe/database/query.py +++ b/frappe/database/query.py @@ -389,7 +389,14 @@ class Engine: for arg in args: field = literal_eval_(arg.strip()) if to_cast: - field = Field(field) + try: + operator_fields = arg.split() + field = OPERATOR_MAP[operator_fields[1]]( + Field(operator_fields[0]), + Field(operator_fields[2]), + ) + except IndexError: + field = Field(field) _args.append(field) return getattr(functions, func)(*_args) @@ -410,6 +417,7 @@ class Engine: if not issubclass(type(field), Criterion): if any([func in field and f"{func}(" in field for func in SQL_FUNCTIONS]): functions.append(field) + return [self.get_function_object(function) for function in functions] def remove_string_functions(self, fields, function_objects): diff --git a/frappe/query_builder/functions.py b/frappe/query_builder/functions.py index d2debd6da1..6481c24442 100644 --- a/frappe/query_builder/functions.py +++ b/frappe/query_builder/functions.py @@ -95,6 +95,7 @@ class SqlFunctions(Enum): Avg = "avg" Max = "max" Min = "min" + Abs = "abs" def _max(dt, fieldname, filters=None, **kwargs): From 4da5fdcd02d0aa5e7bc761b329d04fd61900a25d Mon Sep 17 00:00:00 2001 From: Aradhya Date: Wed, 29 Jun 2022 15:45:38 +0530 Subject: [PATCH 02/80] fix: fixed spaces in args fix: lint --- frappe/database/database.py | 4 +++- frappe/database/query.py | 22 +++++++++++++--------- frappe/tests/test_db_query.py | 4 +++- frappe/tests/test_query.py | 11 ++++++++++- 4 files changed, 29 insertions(+), 12 deletions(-) diff --git a/frappe/database/database.py b/frappe/database/database.py index 3d48f1ebe9..4c5d54f857 100644 --- a/frappe/database/database.py +++ b/frappe/database/database.py @@ -1050,7 +1050,9 @@ class Database(object): cache_count = frappe.cache().get_value("doctype:count:{}".format(dt)) if cache_count is not None: return cache_count - query = frappe.qb.engine.get_query(table=dt, filters=filters, fields=Count("*"), distinct=distinct) + query = frappe.qb.engine.get_query( + table=dt, filters=filters, fields=Count("*"), distinct=distinct + ) count = self.sql(query, debug=debug)[0][0] if not filters and cache: frappe.cache().set_value("doctype:count:{}".format(dt), count, expires_in_sec=86400) diff --git a/frappe/database/query.py b/frappe/database/query.py index 10cb62b4a2..b69ab7958f 100644 --- a/frappe/database/query.py +++ b/frappe/database/query.py @@ -387,16 +387,20 @@ class Engine: _args = [] for arg in args: - field = literal_eval_(arg.strip()) + has_operator = False + initial_fields = literal_eval_(arg.strip()) if to_cast: - try: - operator_fields = arg.split() - field = OPERATOR_MAP[operator_fields[1]]( - Field(operator_fields[0]), - Field(operator_fields[2]), - ) - except IndexError: - field = Field(field) + for _operator in OPERATOR_MAP.keys(): + if _operator in initial_fields: + has_operator = True + field = OPERATOR_MAP[_operator]( + *map(lambda field: Field(field.strip()), arg.split(_operator)) + ) + + field = Field(initial_fields) if not has_operator else field + else: + field = initial_fields + _args.append(field) return getattr(functions, func)(*_args) diff --git a/frappe/tests/test_db_query.py b/frappe/tests/test_db_query.py index 8727951f4a..ad9f59b3cd 100644 --- a/frappe/tests/test_db_query.py +++ b/frappe/tests/test_db_query.py @@ -143,7 +143,9 @@ class TestReportview(unittest.TestCase): ) def test_none_filter(self): - query = frappe.qb.engine.get_query("DocType", fields="name", filters={"restrict_to_domain": None}) + query = frappe.qb.engine.get_query( + "DocType", fields="name", filters={"restrict_to_domain": None} + ) sql = str(query).replace("`", "").replace('"', "") condition = "restrict_to_domain IS NULL" self.assertIn(condition, sql) diff --git a/frappe/tests/test_query.py b/frappe/tests/test_query.py index e7682a0d0c..88a631ca67 100644 --- a/frappe/tests/test_query.py +++ b/frappe/tests/test_query.py @@ -42,7 +42,7 @@ class TestQuery(unittest.TestCase): ) def test_functions_fields(self): - from frappe.query_builder.functions import Count, Max + from frappe.query_builder.functions import Abs, Count, Max self.assertEqual( frappe.qb.engine.get_query("User", fields="Count(name)", filters={}).get_sql(), @@ -54,6 +54,15 @@ class TestQuery(unittest.TestCase): frappe.qb.from_("User").select(Count(Field("name")), Max(Field("name"))).get_sql(), ) + self.assertEqual( + frappe.qb.engine.get_query( + "User", fields=["abs(name-email)", "Count(name)"], filters={} + ).get_sql(), + frappe.qb.from_("User") + .select(Abs(Field("name") - Field("email")), Count(Field("name"))) + .get_sql(), + ) + self.assertEqual( frappe.qb.engine.get_query("User", fields=[Count("*")], filters={}).get_sql(), frappe.qb.from_("User").select(Count("*")).get_sql(), From 25bb945de724a898bd4c22923e0ce004a102359a Mon Sep 17 00:00:00 2001 From: Aradhya Date: Wed, 29 Jun 2022 17:21:49 +0530 Subject: [PATCH 03/80] feat: Added truediv & mul operators --- frappe/database/query.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/frappe/database/query.py b/frappe/database/query.py index b69ab7958f..337c9d892a 100644 --- a/frappe/database/query.py +++ b/frappe/database/query.py @@ -169,6 +169,8 @@ OPERATOR_MAP: Dict[str, Callable] = { "=<": operator.le, ">=": operator.ge, "=>": operator.ge, + "/": operator.truediv, + "*": operator.mul, "in": func_in, "not in": func_not_in, "like": like, From 701bf2ede6ed04c1cf0a899fdcd2a8008d9e01ab Mon Sep 17 00:00:00 2001 From: Aradhya Date: Wed, 29 Jun 2022 22:33:51 +0530 Subject: [PATCH 04/80] fix: fixed false operator placements in query --- frappe/database/query.py | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/frappe/database/query.py b/frappe/database/query.py index 337c9d892a..4a7d0e8a67 100644 --- a/frappe/database/query.py +++ b/frappe/database/query.py @@ -2,6 +2,7 @@ import operator import re from ast import literal_eval from functools import cached_property +from types import BuiltinFunctionType from typing import TYPE_CHECKING, Any, Callable, Dict, List, Tuple, Union import frappe @@ -389,17 +390,20 @@ class Engine: _args = [] for arg in args: - has_operator = False initial_fields = literal_eval_(arg.strip()) if to_cast: + has_primitive_operator = False for _operator in OPERATOR_MAP.keys(): if _operator in initial_fields: - has_operator = True - field = OPERATOR_MAP[_operator]( - *map(lambda field: Field(field.strip()), arg.split(_operator)) - ) + operator_mapping = OPERATOR_MAP[_operator] + # Only perform this if operator is of primitive type. + if isinstance(operator_mapping, BuiltinFunctionType): + has_primitive_operator = True + field = operator_mapping( + *map(lambda field: Field(field.strip()), arg.split(_operator)), + ) - field = Field(initial_fields) if not has_operator else field + field = Field(initial_fields) if not has_primitive_operator else field else: field = initial_fields From 7edc20dd1788b562f56d89ccb8bfcb3f4c676dd8 Mon Sep 17 00:00:00 2001 From: Aradhya Date: Thu, 30 Jun 2022 15:33:00 +0530 Subject: [PATCH 05/80] Revert "Revert "feat: Adding support to Query engine"" This reverts commit 813dcc1848b6409ac4a734359d54413665c02db8. --- frappe/__init__.py | 9 +- frappe/database/database.py | 32 +--- frappe/database/query.py | 156 +++++++++++++++++- .../desk/doctype/number_card/number_card.py | 2 +- frappe/desk/listview.py | 2 +- frappe/query_builder/__init__.py | 1 + frappe/query_builder/builder.py | 9 + frappe/query_builder/functions.py | 22 ++- frappe/query_builder/utils.py | 6 + frappe/tests/test_db_query.py | 2 +- frappe/tests/test_query.py | 58 ++++++- frappe/utils/goal.py | 2 +- 12 files changed, 257 insertions(+), 44 deletions(-) diff --git a/frappe/__init__.py b/frappe/__init__.py index 063a62c84b..0f55854535 100644 --- a/frappe/__init__.py +++ b/frappe/__init__.py @@ -22,7 +22,12 @@ from typing import TYPE_CHECKING, Any, Callable, Dict, List, Optional, Union import click from werkzeug.local import Local, release_local -from frappe.query_builder import get_query_builder, patch_query_aggregation, patch_query_execute +from frappe.query_builder import ( + get_qb_engine, + get_query_builder, + patch_query_aggregation, + patch_query_execute, +) from frappe.utils.caching import request_cache from frappe.utils.data import cstr, sbool @@ -240,7 +245,7 @@ def init(site, sites_path=None, new_site=False): local.session = _dict() local.dev_server = _dev_server local.qb = get_query_builder(local.conf.db_type or "mariadb") - + local.qb.engine = get_qb_engine() setup_module_map() if not _qb_patched.get(local.conf.db_type): diff --git a/frappe/database/database.py b/frappe/database/database.py index a52264ed6d..3d48f1ebe9 100644 --- a/frappe/database/database.py +++ b/frappe/database/database.py @@ -12,7 +12,7 @@ from contextlib import contextmanager from time import time from typing import Dict, List, Optional, Tuple, Union -from pypika.terms import Criterion, NullValue, PseudoColumn +from pypika.terms import Criterion, NullValue import frappe import frappe.defaults @@ -75,15 +75,6 @@ class Database(object): self.password = password or frappe.conf.db_password self.value_cache = {} - @property - def query(self): - if not hasattr(self, "_query"): - from .query import Query - - self._query = Query() - del Query - return self._query - def setup_type_map(self): pass @@ -600,7 +591,7 @@ class Database(object): return [map(values.get, fields)] else: - r = self.query.get_sql( + r = frappe.qb.engine.get_query( "Singles", filters={"field": ("in", tuple(fields)), "doctype": doctype}, fields=["field", "value"], @@ -633,7 +624,7 @@ class Database(object): # Get coulmn and value of the single doctype Accounts Settings account_settings = frappe.db.get_singles_dict("Accounts Settings") """ - queried_result = self.query.get_sql( + queried_result = frappe.qb.engine.get_query( "Singles", filters={"doctype": doctype}, fields=["field", "value"], @@ -706,7 +697,7 @@ class Database(object): if cache and fieldname in self.value_cache[doctype]: return self.value_cache[doctype][fieldname] - val = self.query.get_sql( + val = frappe.qb.engine.get_query( table="Singles", filters={"doctype": doctype, "field": fieldname}, fields="value", @@ -748,14 +739,7 @@ class Database(object): ): field_objects = [] - if not isinstance(fields, Criterion): - for field in fields: - if "(" in str(field) or " as " in str(field): - field_objects.append(PseudoColumn(field)) - else: - field_objects.append(field) - - query = self.query.get_sql( + query = frappe.qb.engine.get_query( table=doctype, filters=filters, orderby=order_by, @@ -865,7 +849,7 @@ class Database(object): frappe.clear_document_cache(dt, docname) else: - query = self.query.build_conditions(table=dt, filters=dn, update=True) + query = frappe.qb.engine.build_conditions(table=dt, filters=dn, update=True) # TODO: Fix this; doesn't work rn - gavin@frappe.io # frappe.cache().hdel_keys(dt, "document_cache") # Workaround: clear all document caches @@ -1066,7 +1050,7 @@ class Database(object): cache_count = frappe.cache().get_value("doctype:count:{}".format(dt)) if cache_count is not None: return cache_count - query = self.query.get_sql(table=dt, filters=filters, fields=Count("*"), distinct=distinct) + query = frappe.qb.engine.get_query(table=dt, filters=filters, fields=Count("*"), distinct=distinct) count = self.sql(query, debug=debug)[0][0] if not filters and cache: frappe.cache().set_value("doctype:count:{}".format(dt), count, expires_in_sec=86400) @@ -1206,7 +1190,7 @@ class Database(object): Doctype name can be passed directly, it will be pre-pended with `tab`. """ filters = filters or kwargs.get("conditions") - query = self.query.build_conditions(table=doctype, filters=filters).delete() + query = frappe.qb.engine.build_conditions(table=doctype, filters=filters).delete() if "debug" not in kwargs: kwargs["debug"] = debug return query.run(**kwargs) diff --git a/frappe/database/query.py b/frappe/database/query.py index f7cc143cf7..60eeaee8c2 100644 --- a/frappe/database/query.py +++ b/frappe/database/query.py @@ -1,16 +1,23 @@ import operator import re +from ast import literal_eval from functools import cached_property -from typing import Any, Callable, Dict, List, Tuple, Union +from typing import TYPE_CHECKING, Any, Callable, Dict, List, Tuple, Union import frappe from frappe import _ from frappe.boot import get_additional_filters_from_hooks from frappe.model.db_query import get_timespan_date_range -from frappe.query_builder import Criterion, Field, Order, Table +from frappe.query_builder import Criterion, Field, Order, Table, functions +from frappe.query_builder.functions import SqlFunctions TAB_PATTERN = re.compile("^tab") WORDS_PATTERN = re.compile(r"\w+") +BRACKETS_PATTERN = re.compile(r"\(.*?\)|$") +SQL_FUNCTIONS = [sql_function.value for sql_function in SqlFunctions] + +if TYPE_CHECKING: + from pypika.functions import Function def like(key: Field, value: str) -> frappe.qb: @@ -93,7 +100,7 @@ def func_between(key: Field, value: Union[List, Tuple]) -> frappe.qb: def func_is(key, value): "Wrapper for IS" - return Field(key).isnotnull() if value.lower() == "set" else Field(key).isnull() + return key.isnotnull() if value.lower() == "set" else key.isnull() def func_timespan(key: Field, value: str) -> frappe.qb: @@ -143,6 +150,13 @@ def change_orderby(order: str): return order[0], Order.desc +def literal_eval_(literal): + try: + return literal_eval(literal) + except (ValueError, SyntaxError): + return literal + + # default operators OPERATOR_MAP: Dict[str, Callable] = { "+": operator.add, @@ -168,7 +182,7 @@ OPERATOR_MAP: Dict[str, Callable] = { } -class Query: +class Engine: tables: dict = {} @cached_property @@ -238,7 +252,7 @@ class Query: Returns: conditions (frappe.qb): frappe.qb object """ - if kwargs.get("orderby"): + if kwargs.get("orderby") and kwargs.get("orderby") != "KEEP_DEFAULT_ORDERING": orderby = kwargs.get("orderby") if isinstance(orderby, str) and len(orderby.split()) > 1: for ordby in orderby.split(","): @@ -250,6 +264,7 @@ class Query: if kwargs.get("limit"): conditions = conditions.limit(kwargs.get("limit")) + conditions = conditions.offset(kwargs.get("offset", 0)) if kwargs.get("distinct"): conditions = conditions.distinct() @@ -257,6 +272,9 @@ class Query: if kwargs.get("for_update"): conditions = conditions.for_update() + if kwargs.get("groupby"): + conditions = conditions.groupby(kwargs.get("groupby")) + return conditions def misc_query(self, table: str, filters: Union[List, Tuple] = None, **kwargs): @@ -308,6 +326,10 @@ class Query: conditions = self.add_conditions(conditions, **kwargs) return conditions + for key, value in filters.items(): + if isinstance(value, bool): + filters.update({key: str(int(value))}) + for key in filters: value = filters.get(key) _operator = self.OPERATOR_MAP["="] @@ -317,7 +339,8 @@ class Query: continue if isinstance(value, (list, tuple)): _operator = self.OPERATOR_MAP[value[0].casefold()] - conditions = conditions.where(_operator(Field(key), value[1])) + _value = value[1] if value[1] else ("",) + conditions = conditions.where(_operator(Field(key), _value)) else: if value is not None: conditions = conditions.where(_operator(Field(key), value)) @@ -354,7 +377,117 @@ class Query: return criterion - def get_sql( + def get_function_object(self, field: str) -> "Function": + """Expects field to look like 'SUM(*)' or 'name' or something similar. Returns PyPika Function object""" + func = field.split("(", maxsplit=1)[0].capitalize() + args_start, args_end = len(func) + 1, field.index(")") + args = field[args_start:args_end].split(",") + + to_cast = "*" not in args + _args = [] + + for arg in args: + field = literal_eval_(arg.strip()) + if to_cast: + field = Field(field) + _args.append(field) + + return getattr(functions, func)(*_args) + + def function_objects_from_string(self, fields): + functions = "" + for func in SQL_FUNCTIONS: + if f"{func}(" in fields: + functions = str(func) + str(BRACKETS_PATTERN.search(fields).group()) + return [self.get_function_object(functions)] + if not functions: + return [] + + def function_objects_from_list(self, fields): + functions = [] + for field in fields: + field = field.casefold() if isinstance(field, str) else field + if not issubclass(type(field), Criterion): + if any([func in field and f"{func}(" in field for func in SQL_FUNCTIONS]): + functions.append(field) + return [self.get_function_object(function) for function in functions] + + def remove_string_functions(self, fields, function_objects): + """Remove string functions from fields which have already been converted to function objects""" + for function in function_objects: + if isinstance(fields, str): + fields = BRACKETS_PATTERN.sub("", fields.replace(function.name.casefold(), "")) + else: + updated_fields = [] + for field in fields: + if isinstance(field, str): + updated_fields.append( + BRACKETS_PATTERN.sub("", field).strip().casefold().replace(function.name.casefold(), "") + ) + else: + updated_fields.append(field) + + fields = [field for field in updated_fields if field] + + return fields + + def set_fields(self, fields, **kwargs): + fields = kwargs.get("pluck") if kwargs.get("pluck") else fields or "name" + if isinstance(fields, list) and None in fields and Field not in fields: + return None + + function_objects = [] + + is_list = isinstance(fields, (list, tuple, set)) + if is_list and len(fields) == 1: + fields = fields[0] + is_list = False + + if is_list: + function_objects += self.function_objects_from_list(fields=fields) + + is_str = isinstance(fields, str) + if is_str: + fields = fields.casefold() + function_objects += self.function_objects_from_string(fields=fields) + + fields = self.remove_string_functions(fields, function_objects) + + if is_str and "," in fields: + fields = [field.replace(" ", "") if "as" not in field else field for field in fields.split(",")] + is_list, is_str = True, False + + if is_str: + if fields == "*": + return fields + if " as " in fields: + fields, reference = fields.split(" as ") + fields = Field(fields).as_(reference) + + if not is_str and fields: + if issubclass(type(fields), Criterion): + return fields + updated_fields = [] + if "*" in fields: + return fields + for field in fields: + if not isinstance(field, Criterion) and field: + if " as " in field: + field, reference = field.split(" as ") + updated_fields.append(Field(field.strip()).as_(reference)) + else: + updated_fields.append(Field(field)) + + fields = updated_fields + + # Need to check instance again since fields modified. + if not isinstance(fields, (list, tuple, set)): + fields = [fields] if fields else [] + + fields.extend(function_objects) + return fields + + def get_query( self, table: str, fields: Union[List, Tuple], @@ -364,15 +497,20 @@ class Query: # Clean up state before each query self.tables = {} criterion = self.build_conditions(table, filters, **kwargs) + fields = self.set_fields(kwargs.get("field_objects") or fields, **kwargs) + + join = kwargs.get("join").replace(" ", "_") if kwargs.get("join") else "left_join" if len(self.tables) > 1: primary_table = self.tables[table] del self.tables[table] for table_object in self.tables.values(): - criterion = criterion.left_join(table_object).on(table_object.parent == primary_table.name) + criterion = getattr(criterion, join)(table_object).on( + table_object.parent == primary_table.name + ) if isinstance(fields, (list, tuple)): - query = criterion.select(*kwargs.get("field_objects", fields)) + query = criterion.select(*fields) elif isinstance(fields, Criterion): query = criterion.select(fields) diff --git a/frappe/desk/doctype/number_card/number_card.py b/frappe/desk/doctype/number_card/number_card.py index 74c8e9eb99..8d031aac01 100644 --- a/frappe/desk/doctype/number_card/number_card.py +++ b/frappe/desk/doctype/number_card/number_card.py @@ -204,7 +204,7 @@ def get_cards_for_user(doctype, txt, searchfield, start, page_len, filters): if txt: search_conditions = [numberCard[field].like("%{txt}%".format(txt=txt)) for field in searchfields] - condition_query = frappe.db.query.build_conditions(doctype, filters) + condition_query = frappe.qb.engine.build_conditions(doctype, filters) return ( condition_query.select(numberCard.name, numberCard.label, numberCard.document_type) diff --git a/frappe/desk/listview.py b/frappe/desk/listview.py index 5149f8bf86..11c985d1ff 100644 --- a/frappe/desk/listview.py +++ b/frappe/desk/listview.py @@ -37,7 +37,7 @@ def get_group_by_count(doctype: str, current_filters: str, field: str) -> List[D ToDo = DocType("ToDo") User = DocType("User") count = Count("*").as_("count") - filtered_records = frappe.db.query.build_conditions(doctype, current_filters).select("name") + filtered_records = frappe.qb.engine.build_conditions(doctype, current_filters).select("name") return ( frappe.qb.from_(ToDo) diff --git a/frappe/query_builder/__init__.py b/frappe/query_builder/__init__.py index 1bf9ec97d9..eb1d9df08f 100644 --- a/frappe/query_builder/__init__.py +++ b/frappe/query_builder/__init__.py @@ -7,6 +7,7 @@ from frappe.query_builder.terms import ParameterizedFunction, ParameterizedValue from frappe.query_builder.utils import ( Column, DocType, + get_qb_engine, get_query_builder, patch_query_aggregation, patch_query_execute, diff --git a/frappe/query_builder/builder.py b/frappe/query_builder/builder.py index d2fdeab324..c23d76974c 100644 --- a/frappe/query_builder/builder.py +++ b/frappe/query_builder/builder.py @@ -1,3 +1,5 @@ +import typing + from pypika import MySQLQuery, Order, PostgreSQLQuery, terms from pypika.dialects import MySQLQueryBuilder, PostgreSQLQueryBuilder from pypika.queries import QueryBuilder, Schema, Table @@ -13,6 +15,13 @@ class Base: Schema = Schema Table = Table + # Added dynamic type hints for engine attribute + # which is to be assigned later. + if typing.TYPE_CHECKING: + from frappe.database.query import Engine + + engine: Engine + @staticmethod def functions(name: str, *args, **kwargs) -> Function: return Function(name, *args, **kwargs) diff --git a/frappe/query_builder/functions.py b/frappe/query_builder/functions.py index f03c139f57..d2debd6da1 100644 --- a/frappe/query_builder/functions.py +++ b/frappe/query_builder/functions.py @@ -1,8 +1,9 @@ +from enum import Enum + from pypika.functions import * from pypika.terms import Arithmetic, ArithmeticExpression, CustomFunction, Function import frappe -from frappe.database.query import Query from frappe.query_builder.custom import GROUP_CONCAT, MATCH, STRING_AGG, TO_TSVECTOR from frappe.query_builder.utils import ImportMapper, db_type_is @@ -14,6 +15,11 @@ class Concat_ws(Function): super(Concat_ws, self).__init__("CONCAT_WS", *terms, **kwargs) +class Locate(Function): + def __init__(self, *terms, **kwargs): + super(Locate, self).__init__("LOCATE", *terms, **kwargs) + + GroupConcat = ImportMapper({db_type_is.MARIADB: GROUP_CONCAT, db_type_is.POSTGRES: STRING_AGG}) Match = ImportMapper({db_type_is.MARIADB: MATCH, db_type_is.POSTGRES: TO_TSVECTOR}) @@ -73,14 +79,24 @@ class Cast_(Function): def _aggregate(function, dt, fieldname, filters, **kwargs): return ( - Query() - .build_conditions(dt, filters) + frappe.qb.engine.build_conditions(dt, filters) .select(function(PseudoColumn(fieldname))) .run(**kwargs)[0][0] or 0 ) +class SqlFunctions(Enum): + DayOfYear = "dayofyear" + Extract = "extract" + Locate = "locate" + Count = "count" + Sum = "sum" + Avg = "avg" + Max = "max" + Min = "min" + + def _max(dt, fieldname, filters=None, **kwargs): return _aggregate(Max, dt, fieldname, filters, **kwargs) diff --git a/frappe/query_builder/utils.py b/frappe/query_builder/utils.py index 10bab38a63..b601665dee 100644 --- a/frappe/query_builder/utils.py +++ b/frappe/query_builder/utils.py @@ -45,6 +45,12 @@ def get_query_builder(type_of_db: str) -> Union[Postgres, MariaDB]: return picks[db] +def get_qb_engine(): + from frappe.database.query import Engine + + return Engine() + + def get_attr(method_string): modulename = ".".join(method_string.split(".")[:-1]) methodname = method_string.split(".")[-1] diff --git a/frappe/tests/test_db_query.py b/frappe/tests/test_db_query.py index c1b2e05266..8727951f4a 100644 --- a/frappe/tests/test_db_query.py +++ b/frappe/tests/test_db_query.py @@ -143,7 +143,7 @@ class TestReportview(unittest.TestCase): ) def test_none_filter(self): - query = frappe.db.query.get_sql("DocType", fields="name", filters={"restrict_to_domain": None}) + query = frappe.qb.engine.get_query("DocType", fields="name", filters={"restrict_to_domain": None}) sql = str(query).replace("`", "").replace('"', "") condition = "restrict_to_domain IS NULL" self.assertIn(condition, sql) diff --git a/frappe/tests/test_query.py b/frappe/tests/test_query.py index 949c3e9433..e7682a0d0c 100644 --- a/frappe/tests/test_query.py +++ b/frappe/tests/test_query.py @@ -1,14 +1,15 @@ import unittest import frappe +from frappe.query_builder import Field from frappe.tests.test_query_builder import db_type_is, run_only_if -@run_only_if(db_type_is.MARIADB) class TestQuery(unittest.TestCase): + @run_only_if(db_type_is.MARIADB) def test_multiple_tables_in_filters(self): self.assertEqual( - frappe.db.query.get_sql( + frappe.qb.engine.get_query( "DocType", ["*"], [ @@ -18,3 +19,56 @@ class TestQuery(unittest.TestCase): ).get_sql(), "SELECT * FROM `tabDocType` LEFT JOIN `tabBOM Update Log` ON `tabBOM Update Log`.`parent`=`tabDocType`.`name` WHERE `tabBOM Update Log`.`name` LIKE 'f%' AND `tabDocType`.`parent`='something'", ) + + def test_string_fields(self): + self.assertEqual( + frappe.qb.engine.get_query( + "User", fields="name, email", filters={"name": "Administrator"} + ).get_sql(), + frappe.qb.from_("User") + .select(Field("name"), Field("email")) + .where(Field("name") == "Administrator") + .get_sql(), + ) + + self.assertEqual( + frappe.qb.engine.get_query( + "User", fields=["name, email"], filters={"name": "Administrator"} + ).get_sql(), + frappe.qb.from_("User") + .select(Field("name"), Field("email")) + .where(Field("name") == "Administrator") + .get_sql(), + ) + + def test_functions_fields(self): + from frappe.query_builder.functions import Count, Max + + self.assertEqual( + frappe.qb.engine.get_query("User", fields="Count(name)", filters={}).get_sql(), + frappe.qb.from_("User").select(Count(Field("name"))).get_sql(), + ) + + self.assertEqual( + frappe.qb.engine.get_query("User", fields=["Count(name)", "Max(name)"], filters={}).get_sql(), + frappe.qb.from_("User").select(Count(Field("name")), Max(Field("name"))).get_sql(), + ) + + self.assertEqual( + frappe.qb.engine.get_query("User", fields=[Count("*")], filters={}).get_sql(), + frappe.qb.from_("User").select(Count("*")).get_sql(), + ) + + def test_qb_fields(self): + user_doctype = frappe.qb.DocType("User") + self.assertEqual( + frappe.qb.engine.get_query( + user_doctype, fields=[user_doctype.name, user_doctype.email], filters={} + ).get_sql(), + frappe.qb.from_(user_doctype).select(user_doctype.name, user_doctype.email).get_sql(), + ) + + self.assertEqual( + frappe.qb.engine.get_query(user_doctype, fields=user_doctype.email, filters={}).get_sql(), + frappe.qb.from_(user_doctype).select(user_doctype.email).get_sql(), + ) diff --git a/frappe/utils/goal.py b/frappe/utils/goal.py index fb348496da..9273b83cbf 100644 --- a/frappe/utils/goal.py +++ b/frappe/utils/goal.py @@ -25,7 +25,7 @@ def get_monthly_results( date_format = "%m-%Y" if frappe.db.db_type != "postgres" else "MM-YYYY" return dict( - frappe.db.query.build_conditions(table=goal_doctype, filters=filters) + frappe.qb.engine.build_conditions(table=goal_doctype, filters=filters) .select( DateFormat(Table[date_col], date_format).as_("month_year"), Function(aggregation, goal_field), From 88faad640969a7878e6267a609507ea8c2a298d5 Mon Sep 17 00:00:00 2001 From: Aradhya Date: Fri, 1 Jul 2022 13:30:18 +0530 Subject: [PATCH 06/80] fix: pyupgrade --- frappe/query_builder/functions.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frappe/query_builder/functions.py b/frappe/query_builder/functions.py index ec36baae19..c1c7657b86 100644 --- a/frappe/query_builder/functions.py +++ b/frappe/query_builder/functions.py @@ -17,7 +17,7 @@ class Concat_ws(Function): class Locate(Function): def __init__(self, *terms, **kwargs): - super(Locate, self).__init__("LOCATE", *terms, **kwargs) + super().__init__("LOCATE", *terms, **kwargs) GroupConcat = ImportMapper({db_type_is.MARIADB: GROUP_CONCAT, db_type_is.POSTGRES: STRING_AGG}) From c97562b736e19ac442fcf37d593277e3dc371364 Mon Sep 17 00:00:00 2001 From: Aradhya Date: Fri, 8 Jul 2022 21:05:42 +0530 Subject: [PATCH 07/80] feat: Added Timestamp support --- frappe/query_builder/functions.py | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/frappe/query_builder/functions.py b/frappe/query_builder/functions.py index c1c7657b86..824de7fbf5 100644 --- a/frappe/query_builder/functions.py +++ b/frappe/query_builder/functions.py @@ -20,6 +20,14 @@ class Locate(Function): super().__init__("LOCATE", *terms, **kwargs) +class Timestamp(Function): + def __init__(self, term: str, time=None, alias=None): + if time: + super().__init__("TIMESTAMP", term, time, alias=alias) + else: + super().__init__("TIMESTAMP", term, alias=alias) + + GroupConcat = ImportMapper({db_type_is.MARIADB: GROUP_CONCAT, db_type_is.POSTGRES: STRING_AGG}) Match = ImportMapper({db_type_is.MARIADB: MATCH, db_type_is.POSTGRES: TO_TSVECTOR}) @@ -96,6 +104,7 @@ class SqlFunctions(Enum): Max = "max" Min = "min" Abs = "abs" + Timestamp = "timestamp" def _max(dt, fieldname, filters=None, **kwargs): From 2fac4f807433cacb4380e56dd530214c89d41202 Mon Sep 17 00:00:00 2001 From: Aradhya Date: Fri, 8 Jul 2022 21:33:58 +0530 Subject: [PATCH 08/80] test: Added tests for timestamp --- frappe/tests/test_query.py | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/frappe/tests/test_query.py b/frappe/tests/test_query.py index 88a631ca67..5c523ac555 100644 --- a/frappe/tests/test_query.py +++ b/frappe/tests/test_query.py @@ -42,7 +42,7 @@ class TestQuery(unittest.TestCase): ) def test_functions_fields(self): - from frappe.query_builder.functions import Abs, Count, Max + from frappe.query_builder.functions import Abs, Count, Max, Timestamp self.assertEqual( frappe.qb.engine.get_query("User", fields="Count(name)", filters={}).get_sql(), @@ -68,6 +68,13 @@ class TestQuery(unittest.TestCase): frappe.qb.from_("User").select(Count("*")).get_sql(), ) + self.assertEqual( + frappe.qb.engine.get_query( + "User", fields="timestamp(creation, modified)", filters={} + ).get_sql(), + frappe.qb.from_("User").select(Timestamp(Field("creation"), Field("modified"))).get_sql(), + ) + def test_qb_fields(self): user_doctype = frappe.qb.DocType("User") self.assertEqual( From 613065fa2e65cfab8ef04c03afd66e072965bc25 Mon Sep 17 00:00:00 2001 From: Aradhya Date: Sat, 9 Jul 2022 22:27:29 +0530 Subject: [PATCH 09/80] feat: Added support for aliasing in function objects --- frappe/database/query.py | 21 +++++++++++++++++---- frappe/tests/test_query.py | 32 ++++++++++++++++++++++++++++++-- 2 files changed, 47 insertions(+), 6 deletions(-) diff --git a/frappe/database/query.py b/frappe/database/query.py index 553160afc6..3169bc52a5 100644 --- a/frappe/database/query.py +++ b/frappe/database/query.py @@ -186,7 +186,7 @@ OPERATOR_MAP: dict[str, Callable] = { class Engine: - tables: dict = {} + tables: dict[str, str] = {} @cached_property def OPERATOR_MAP(self): @@ -384,6 +384,7 @@ class Engine: args_start, args_end = len(func) + 1, field.index(")") args = field[args_start:args_end].split(",") + _, alias = field.split(" as ") if " as " in field else (None, None) to_cast = "*" not in args _args = [] @@ -406,14 +407,15 @@ class Engine: field = initial_fields _args.append(field) - - return getattr(functions, func)(*_args) + return getattr(functions, func)(*_args, alias=alias or None) def function_objects_from_string(self, fields): functions = "" for func in SQL_FUNCTIONS: if f"{func}(" in fields: + _, alias = fields.split(" as ") if " as " in fields else ("", "") functions = str(func) + str(BRACKETS_PATTERN.search(fields).group()) + functions += " as " + alias return [self.get_function_object(functions)] if not functions: return [] @@ -432,14 +434,25 @@ class Engine: """Remove string functions from fields which have already been converted to function objects""" for function in function_objects: if isinstance(fields, str): + has_alias = False + if function.alias: + has_alias = True fields = BRACKETS_PATTERN.sub("", fields.replace(function.name.casefold(), "")) + if has_alias: + fields = fields.replace(" as " + function.alias.casefold(), "") else: updated_fields = [] for field in fields: + has_alias = False + if function.alias: + has_alias = True if isinstance(field, str): - updated_fields.append( + _field = ( BRACKETS_PATTERN.sub("", field).strip().casefold().replace(function.name.casefold(), "") ) + if has_alias: + _field = _field.replace(" as " + function.alias.casefold(), "") + updated_fields.append(_field) else: updated_fields.append(field) diff --git a/frappe/tests/test_query.py b/frappe/tests/test_query.py index 5c523ac555..10208fec70 100644 --- a/frappe/tests/test_query.py +++ b/frappe/tests/test_query.py @@ -2,6 +2,7 @@ import unittest import frappe from frappe.query_builder import Field +from frappe.query_builder.functions import Abs, Count, Max, Timestamp from frappe.tests.test_query_builder import db_type_is, run_only_if @@ -42,8 +43,6 @@ class TestQuery(unittest.TestCase): ) def test_functions_fields(self): - from frappe.query_builder.functions import Abs, Count, Max, Timestamp - self.assertEqual( frappe.qb.engine.get_query("User", fields="Count(name)", filters={}).get_sql(), frappe.qb.from_("User").select(Count(Field("name"))).get_sql(), @@ -88,3 +87,32 @@ class TestQuery(unittest.TestCase): frappe.qb.engine.get_query(user_doctype, fields=user_doctype.email, filters={}).get_sql(), frappe.qb.from_(user_doctype).select(user_doctype.email).get_sql(), ) + + def test_aliasing(self): + user_doctype = frappe.qb.DocType("User") + self.assertEqual( + frappe.qb.engine.get_query( + user_doctype, fields=["name as owner", "email as id"], filters={} + ).get_sql(), + frappe.qb.from_(user_doctype) + .select(user_doctype.name.as_("owner"), user_doctype.email.as_("id")) + .get_sql(), + ) + + self.assertEqual( + frappe.qb.engine.get_query( + user_doctype, fields="name as owner, email as id", filters={} + ).get_sql(), + frappe.qb.from_(user_doctype) + .select(user_doctype.name.as_("owner"), user_doctype.email.as_("id")) + .get_sql(), + ) + + self.assertEqual( + frappe.qb.engine.get_query( + user_doctype, fields=["Count(name) as c", "email as id"], filters={} + ).get_sql(), + frappe.qb.from_(user_doctype) + .select(user_doctype.email.as_("id"), Count(Field("name")).as_("c")) + .get_sql(), + ) From da52098bd351fde9ee719f9b034a16b8718e417c Mon Sep 17 00:00:00 2001 From: Suraj Shetty Date: Mon, 11 Jul 2022 12:24:25 +0530 Subject: [PATCH 10/80] perf: Batch email queuing while sending emails to large no. of recipients separately - Batch email queue - Re-use smtp_session while sending bulk emails (from same sender) instead of creating new session for everytime. Note: creation of new email session is very expensive. --- .../email/doctype/email_queue/email_queue.py | 91 ++++++++++++------- 1 file changed, 58 insertions(+), 33 deletions(-) diff --git a/frappe/email/doctype/email_queue/email_queue.py b/frappe/email/doctype/email_queue/email_queue.py index 221f3fbb31..dfad6d9ce8 100644 --- a/frappe/email/doctype/email_queue/email_queue.py +++ b/frappe/email/doctype/email_queue/email_queue.py @@ -16,6 +16,7 @@ from frappe.core.utils import html2text from frappe.email.doctype.email_account.email_account import EmailAccount from frappe.email.email_body import add_attachment, get_email, get_formatted_html from frappe.email.queue import get_unsubcribed_url, get_unsubscribe_message +from frappe.email.smtp import SMTPServer from frappe.model.document import Document from frappe.query_builder import DocType, Interval from frappe.query_builder.functions import Now @@ -99,7 +100,7 @@ class EmailQueue(Document): def get_email_account(self): if self.email_account: - return frappe.get_doc("Email Account", self.email_account) + return frappe.get_cached_doc("Email Account", self.email_account) return EmailAccount.find_outgoing( match_by_email=self.sender, match_by_doctype=self.reference_doctype @@ -115,12 +116,12 @@ class EmailQueue(Document): return True - def send(self, is_background_task=False): + def send(self, is_background_task: bool = False, smtp_server_instance: SMTPServer = None): """Send emails to recipients.""" if not self.can_send_now(): return - with SendMailContext(self, is_background_task) as ctx: + with SendMailContext(self, is_background_task, smtp_server_instance) as ctx: message = None for recipient in self.recipients: if not recipient.is_mail_to_be_sent(): @@ -169,21 +170,32 @@ class EmailQueue(Document): @task(queue="short") -def send_mail(email_queue_name, is_background_task=False): - """This is equalent to EmqilQueue.send. +def send_mail(email_queue_name, is_background_task=False, smtp_server_instance: SMTPServer = None): + """This is equivalent to EmailQueue.send. This provides a way to make sending mail as a background job. """ record = EmailQueue.find(email_queue_name) - record.send(is_background_task=is_background_task) + record.send(is_background_task=is_background_task, smtp_server_instance=smtp_server_instance) class SendMailContext: - def __init__(self, queue_doc: Document, is_background_task: bool = False): + def __init__( + self, + queue_doc: Document, + is_background_task: bool = False, + smtp_server_instance: SMTPServer = None, + ): self.queue_doc: EmailQueue = queue_doc self.is_background_task = is_background_task self.email_account_doc = queue_doc.get_email_account() - self.smtp_server = self.email_account_doc.get_smtp_server() + + self.smtp_server = smtp_server_instance or self.email_account_doc.get_smtp_server() + + # if smtp_server_instance is passed, then retain smtp session + # Note: smtp session will have to be manually closed + self.retain_smtp_session = bool(smtp_server_instance) + self.sent_to = [rec.recipient for rec in self.queue_doc.recipients if rec.is_main_sent()] def __enter__(self): @@ -200,11 +212,13 @@ class SendMailContext: JobTimeoutException, ] - self.smtp_server.quit() + if not self.retain_smtp_session: + self.smtp_server.quit() + self.log_exception(exc_type, exc_val, exc_tb) if exc_type in exceptions: - email_status = (self.sent_to and "Partially Sent") or "Not Sent" + email_status = "Partially Sent" if self.sent_to else "Not Sent" self.queue_doc.update_status(status=email_status, commit=True) elif exc_type: if self.queue_doc.retry < get_email_retry_limit(): @@ -216,12 +230,12 @@ class SendMailContext: email_status = self.is_mail_sent_to_all() and "Sent" email_status = email_status or (self.sent_to and "Partially Sent") or "Not Sent" - update_fields = {"status": email_status} - if self.email_account_doc.is_exists_in_db(): - update_fields["email_account"] = self.email_account_doc.name - else: - update_fields["email_account"] = None - + update_fields = { + "status": email_status, + "email_account": self.email_account_doc.name + if self.email_account_doc.is_exists_in_db() + else None, + } self.queue_doc.update_status(**update_fields, commit=True) def log_exception(self, exc_type, exc_val, exc_tb): @@ -249,6 +263,7 @@ class SendMailContext: return Parser(policy=SMTPUTF8).parsestr(message) def message_placeholder(self, placeholder_key): + # sourcery skip: avoid-builtin-shadow map = { "tracker": "", "unsubscribe_url": "", @@ -269,7 +284,7 @@ class SendMailContext: ) message = message.replace(self.message_placeholder("cc"), self.get_receivers_str()) message = message.replace( - self.message_placeholder("recipient"), self.get_receipient_str(recipient_email) + self.message_placeholder("recipient"), self.get_recipient_str(recipient_email) ) message = self.include_attachments(message) return message @@ -304,14 +319,11 @@ class SendMailContext: to_str = ", ".join(self.queue_doc.to) cc_str = ", ".join(self.queue_doc.cc) message = f"This email was sent to {to_str}" - message = message + f" and copied to {cc_str}" if cc_str else message + message = f"{message} and copied to {cc_str}" if cc_str else message return message - def get_receipient_str(self, recipient_email): - message = "" - if self.queue_doc.expose_recipients != "header": - message = recipient_email - return message + def get_recipient_str(self, recipient_email): + return recipient_email if self.queue_doc.expose_recipients != "header" else "" def include_attachments(self, message): message_obj = self.get_message_object(message) @@ -628,7 +640,6 @@ class QueueBuilder: if not (final_recipients + self.final_cc()): return [] - email_queues = [] queue_data = self.as_dict(include_recipients=False) if not queue_data: return [] @@ -636,17 +647,31 @@ class QueueBuilder: if not queue_separately: recipients = list(set(final_recipients + self.final_cc() + self.bcc)) q = EmailQueue.new({**queue_data, **{"recipients": recipients}}, ignore_permissions=True) - email_queues.append(q) + send_now and q.send() else: - for r in final_recipients: - recipients = [r] if email_queues else list(set([r] + self.final_cc() + self.bcc)) - q = EmailQueue.new({**queue_data, **{"recipients": recipients}}, ignore_permissions=True) - email_queues.append(q) + if send_now and len(recipients) >= 1000: + # force queueing if there are too many recipients to avoid timeouts + send_now = False + for recipients in frappe.utils.create_batch(final_recipients, 1000): + frappe.enqueue( + self.send_emails, + queue_data=queue_data, + final_recipients=recipients, + now=send_now, + ) - if send_now: - for doc in email_queues: - doc.send() - return email_queues + def send_emails(self, queue_data, final_recipients): + # This is used to bulk send emails from same sender to multiple recipients separately + # This re-uses smtp server instance to minimize the cost of new session creation + smtp_server_instance = None + for r in final_recipients: + recipients = list(set([r] + self.final_cc() + self.bcc)) + q = EmailQueue.new({**queue_data, **{"recipients": recipients}}, ignore_permissions=True) + if not smtp_server_instance: + email_account = q.get_email_account() + smtp_server_instance = email_account.get_smtp_server() + q.send(smtp_server_instance=smtp_server_instance) + smtp_server_instance.quit() def as_dict(self, include_recipients=True): email_account = self.get_outgoing_email_account() From 802a1ac993eaf448ed386a814bc88b95d38ef634 Mon Sep 17 00:00:00 2001 From: Suraj Shetty Date: Mon, 11 Jul 2022 13:42:15 +0530 Subject: [PATCH 11/80] refactor: Newsletter status updator - Freeze newsletter till emails are queued - Show newsletter status as "Sending" if emails are in queue - Remove unused dashboard code --- frappe/email/doctype/newsletter/newsletter.js | 96 ++++++------------- frappe/email/doctype/newsletter/newsletter.py | 1 - 2 files changed, 29 insertions(+), 68 deletions(-) diff --git a/frappe/email/doctype/newsletter/newsletter.js b/frappe/email/doctype/newsletter/newsletter.js index 55805ad485..7a47bbb615 100644 --- a/frappe/email/doctype/newsletter/newsletter.js +++ b/frappe/email/doctype/newsletter/newsletter.js @@ -30,12 +30,12 @@ frappe.ui.form.on('Newsletter', { frm.add_custom_button(__('Send now'), () => { if (frm.doc.schedule_send) { frappe.confirm(__("This newsletter was scheduled to send on a later date. Are you sure you want to send it now?"), function () { - frm.call('send_emails').then(() => frm.refresh()); + frm.events.send_emails(frm); }); return; } - frappe.confirm(__("Are you sure you want to send this newsletter now?"), function () { - frm.call('send_emails').then(() => frm.refresh()); + frappe.confirm(__("Are you sure you want to send this newsletter now?"), () => { + frm.events.send_emails(frm); }); }, __('Send')); @@ -44,8 +44,7 @@ frappe.ui.form.on('Newsletter', { }, __('Send')); } - frm.events.setup_dashboard(frm); - frm.events.setup_sending_status(frm); + frm.events.update_sending_status(frm); if (frm.is_new() && !doc.sender_email) { let { fullname, email } = frappe.user_info(doc.owner); @@ -56,6 +55,15 @@ frappe.ui.form.on('Newsletter', { frm.trigger('update_schedule_message'); }, + send_emails(frm) { + frappe.dom.freeze(__("Queuing emails...")); + frm.call('send_emails').then(() => { + frm.refresh(); + frappe.dom.unfreeze(); + frappe.show_alert(__("Queued {0} emails", [frappe.utils.shorten_number(frm.doc.total_recipients)])); + }); + }, + schedule_send_dialog(frm) { let hours = frappe.utils.range(24); let time_slots = hours.map(hour => { @@ -128,77 +136,31 @@ frappe.ui.form.on('Newsletter', { d.show(); }, - setup_dashboard(frm) { - if (!frm.doc.__islocal && cint(frm.doc.email_sent) - && frm.doc.__onload && frm.doc.__onload.status_count) { - var stat = frm.doc.__onload.status_count; - var total = frm.doc.scheduled_to_send; - if (total) { - $.each(stat, function (k, v) { - stat[k] = flt(v * 100 / total, 2) + '%'; - }); - - frm.dashboard.add_progress("Status", [ - { - title: stat["Not Sent"] + " Queued", - width: stat["Not Sent"], - progress_class: "progress-bar-info" - }, - { - title: stat["Sent"] + " Sent", - width: stat["Sent"], - progress_class: "progress-bar-success" - }, - { - title: stat["Sending"] + " Sending", - width: stat["Sending"], - progress_class: "progress-bar-warning" - }, - { - title: stat["Error"] + "% Error", - width: stat["Error"], - progress_class: "progress-bar-danger" - } - ]); + async update_sending_status(frm) { + if (frm.doc.email_sent && frm.$wrapper.is(':visible')) { + let res = await frm.call('get_sending_status'); + let stats = res.message; + stats && frm.events.update_sending_progress(frm, stats.sent, stats.total); + if (stats.sent >= frm.doc.total_recipients || !stats.total) { + frm.sending_status && clearInterval(frm.sending_status); + frm.sending_status = null; } } - }, - setup_sending_status(frm) { - frm.call('get_sending_status').then(r => { - if (r.message) { - frm.events.update_sending_progress(frm, r.message.sent, r.message.total); - } - if (r.message.sent >= r.message.total) { - return; - } - if (frm.sending_status) return; - - frm.sending_status = setInterval(() => { - if (frm.doc.email_sent && frm.$wrapper.is(':visible')) { - frm.call('get_sending_status').then(r => { - if (r.message) { - let { sent, total } = r.message; - frm.events.update_sending_progress(frm, sent, total); - - if (sent >= total) { - clearInterval(frm.sending_status); - frm.sending_status = null; - return; - } - } - }); - } - }, 5000); - }); + if (frm.sending_status) return; + frm.sending_status = setInterval(() => frm.events.update_sending_status(frm), 5000); }, update_sending_progress(frm, sent, total) { - if (sent >= total) { + if (sent >= frm.doc.total_recipients || !frm.doc.email_sent) { + frm.doc.email_sent && frm.page.set_indicator(__("Sent"), "green"); frm.dashboard.hide_progress(); return; } - frm.dashboard.show_progress(__('Sending emails'), sent * 100 / total, __("{0} of {1} sent", [sent, total])); + if (total) { + frm.page.set_indicator(__("Sending"), "blue"); + frm.dashboard.show_progress(__('Sending emails'), sent * 100 / frm.doc.total_recipients, __("{0} of {1} sent", [sent, frm.doc.total_recipients])); + } }, on_hide(frm) { diff --git a/frappe/email/doctype/newsletter/newsletter.py b/frappe/email/doctype/newsletter/newsletter.py index 757a8c875b..913553fc57 100644 --- a/frappe/email/doctype/newsletter/newsletter.py +++ b/frappe/email/doctype/newsletter/newsletter.py @@ -75,7 +75,6 @@ class Newsletter(WebsiteGenerator): self.schedule_sending = False self.schedule_send = None self.queue_all() - frappe.msgprint(_("Email queued to {0} recipients").format(self.total_recipients)) def validate_send(self): """Validate if Newsletter can be sent.""" From e8623dbc19c7eab3598d0e5f9bd5bd9e30a2e8ff Mon Sep 17 00:00:00 2001 From: Sagar Vora Date: Mon, 11 Jul 2022 14:10:59 +0530 Subject: [PATCH 12/80] fix: simplify condition --- frappe/handler.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/frappe/handler.py b/frappe/handler.py index d74125a9e0..b1bbb6bf10 100644 --- a/frappe/handler.py +++ b/frappe/handler.py @@ -206,9 +206,7 @@ def upload_file(): frappe.local.uploaded_file = content frappe.local.uploaded_filename = filename - if (not file_url or content) and ( - frappe.session.user == "Guest" or (user and not user.has_desk_access()) - ): + if content and (frappe.session.user == "Guest" or (user and not user.has_desk_access())): filetype = guess_type(filename)[0] if filetype not in ALLOWED_MIMETYPES: frappe.throw(_("You can only upload JPG, PNG, PDF, TXT or Microsoft documents.")) From a9fd5f50017491de4a8576364c20a3ac2d77f3ea Mon Sep 17 00:00:00 2001 From: Sagar Vora Date: Mon, 11 Jul 2022 14:23:10 +0530 Subject: [PATCH 13/80] fix: dont allow blank content of unsupported type --- frappe/handler.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/frappe/handler.py b/frappe/handler.py index b1bbb6bf10..cee6d3fbde 100644 --- a/frappe/handler.py +++ b/frappe/handler.py @@ -206,7 +206,9 @@ def upload_file(): frappe.local.uploaded_file = content frappe.local.uploaded_filename = filename - if content and (frappe.session.user == "Guest" or (user and not user.has_desk_access())): + if content is not None and ( + frappe.session.user == "Guest" or (user and not user.has_desk_access()) + ): filetype = guess_type(filename)[0] if filetype not in ALLOWED_MIMETYPES: frappe.throw(_("You can only upload JPG, PNG, PDF, TXT or Microsoft documents.")) From bae2db278d5f7fdeafd23a01d7b5bcb591748236 Mon Sep 17 00:00:00 2001 From: Suraj Shetty Date: Mon, 11 Jul 2022 15:27:43 +0530 Subject: [PATCH 14/80] fix: Update status color Because both "Not Sent" & "Not Saved" had same indicator color and it was hard to distinguish --- frappe/email/doctype/newsletter/newsletter_list.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/frappe/email/doctype/newsletter/newsletter_list.js b/frappe/email/doctype/newsletter/newsletter_list.js index 9ded6148e0..0b82f1c9e4 100644 --- a/frappe/email/doctype/newsletter/newsletter_list.js +++ b/frappe/email/doctype/newsletter/newsletter_list.js @@ -4,9 +4,9 @@ frappe.listview_settings['Newsletter'] = { if (doc.email_sent) { return [__("Sent"), "green", "email_sent,=,Yes"]; } else if (doc.schedule_sending) { - return [__("Scheduled"), "orange", "email_sent,=,No|schedule_sending,=,Yes"]; + return [__("Scheduled"), "purple", "email_sent,=,No|schedule_sending,=,Yes"]; } else { - return [__("Not Sent"), "orange", "email_sent,=,No"]; + return [__("Not Sent"), "gray", "email_sent,=,No"]; } } }; From a1bd1b3b23f446f7373c46562d4efe5a7f2b8d80 Mon Sep 17 00:00:00 2001 From: Suraj Shetty Date: Mon, 11 Jul 2022 17:42:41 +0530 Subject: [PATCH 15/80] perf: Index status of recipeint & unsubscribed & email_group of member - This will improve the performance of few get queries newsletter --- .../email_group_member.json | 199 ++++++------------ .../email_queue_recipient.json | 154 ++++---------- frappe/email/doctype/newsletter/newsletter.py | 3 +- 3 files changed, 100 insertions(+), 256 deletions(-) diff --git a/frappe/email/doctype/email_group_member/email_group_member.json b/frappe/email/doctype/email_group_member/email_group_member.json index b3b87c6211..0e32135b72 100644 --- a/frappe/email/doctype/email_group_member/email_group_member.json +++ b/frappe/email/doctype/email_group_member/email_group_member.json @@ -1,151 +1,70 @@ { - "allow_copy": 0, - "allow_import": 1, - "allow_rename": 0, - "autoname": "hash", - "beta": 0, - "creation": "2015-03-18 06:15:59.321619", - "custom": 0, - "docstatus": 0, - "doctype": "DocType", - "document_type": "Document", - "editable_grid": 0, - "engine": "InnoDB", + "actions": [], + "allow_import": 1, + "autoname": "hash", + "creation": "2015-03-18 06:15:59.321619", + "doctype": "DocType", + "document_type": "Document", + "engine": "InnoDB", + "field_order": [ + "email_group", + "email", + "unsubscribed" + ], "fields": [ { - "allow_on_submit": 0, - "bold": 0, - "collapsible": 0, - "columns": 0, - "fieldname": "email_group", - "fieldtype": "Link", - "hidden": 0, - "ignore_user_permissions": 0, - "ignore_xss_filter": 0, - "in_filter": 0, - "in_global_search": 0, - "in_list_view": 1, - "in_standard_filter": 1, - "label": "Email Group", - "length": 0, - "no_copy": 0, - "options": "Email Group", - "permlevel": 0, - "precision": "", - "print_hide": 0, - "print_hide_if_no_value": 0, - "read_only": 0, - "remember_last_selected_value": 0, - "report_hide": 0, - "reqd": 1, - "search_index": 0, - "set_only_once": 0, - "unique": 0 - }, + "fieldname": "email_group", + "fieldtype": "Link", + "in_list_view": 1, + "in_standard_filter": 1, + "label": "Email Group", + "options": "Email Group", + "reqd": 1, + "search_index": 1 + }, { - "allow_on_submit": 0, - "bold": 0, - "collapsible": 0, - "columns": 0, - "fieldname": "email", - "fieldtype": "Data", - "hidden": 0, - "ignore_user_permissions": 0, - "ignore_xss_filter": 0, - "in_filter": 0, - "in_global_search": 1, - "in_list_view": 1, - "in_standard_filter": 0, - "label": "Email", - "length": 0, - "no_copy": 0, - "permlevel": 0, - "precision": "", - "print_hide": 0, - "print_hide_if_no_value": 0, - "read_only": 0, - "remember_last_selected_value": 0, - "report_hide": 0, - "reqd": 1, - "search_index": 0, - "set_only_once": 0, - "unique": 0 - }, + "fieldname": "email", + "fieldtype": "Data", + "in_global_search": 1, + "in_list_view": 1, + "label": "Email", + "reqd": 1 + }, { - "allow_on_submit": 0, - "bold": 0, - "collapsible": 0, - "columns": 0, - "fieldname": "unsubscribed", - "fieldtype": "Check", - "hidden": 0, - "ignore_user_permissions": 0, - "ignore_xss_filter": 0, - "in_filter": 0, - "in_global_search": 0, - "in_list_view": 1, - "in_standard_filter": 0, - "label": "Unsubscribed", - "length": 0, - "no_copy": 0, - "permlevel": 0, - "precision": "", - "print_hide": 0, - "print_hide_if_no_value": 0, - "read_only": 0, - "remember_last_selected_value": 0, - "report_hide": 0, - "reqd": 0, - "search_index": 0, - "set_only_once": 0, - "unique": 0 + "default": "0", + "fieldname": "unsubscribed", + "fieldtype": "Check", + "in_list_view": 1, + "label": "Unsubscribed", + "search_index": 1 } - ], - "hide_heading": 0, - "hide_toolbar": 0, - "idx": 0, - "image_view": 0, - "in_create": 0, - - "is_submittable": 0, - "issingle": 0, - "istable": 0, - "max_attachments": 0, - "modified": "2017-02-17 17:00:42.551806", - "modified_by": "Administrator", - "module": "Email", - "name": "Email Group Member", - "name_case": "", - "owner": "Administrator", + ], + "links": [], + "modified": "2022-07-11 16:38:34.165271", + "modified_by": "Administrator", + "module": "Email", + "name": "Email Group Member", + "naming_rule": "Random", + "owner": "Administrator", "permissions": [ { - "amend": 0, - "apply_user_permissions": 0, - "cancel": 0, - "create": 1, - "delete": 1, - "email": 1, - "export": 1, - "if_owner": 0, - "import": 1, - "permlevel": 0, - "print": 1, - "read": 1, - "report": 1, - "role": "Newsletter Manager", - "set_user_permissions": 0, - "share": 1, - "submit": 0, + "create": 1, + "delete": 1, + "email": 1, + "export": 1, + "import": 1, + "print": 1, + "read": 1, + "report": 1, + "role": "Newsletter Manager", + "share": 1, "write": 1 } - ], - "quick_entry": 1, - "read_only": 0, - "read_only_onload": 0, - "show_name_in_global_search": 0, - "sort_field": "modified", - "sort_order": "DESC", - "title_field": "email", - "track_changes": 1, - "track_seen": 0 + ], + "quick_entry": 1, + "sort_field": "modified", + "sort_order": "DESC", + "states": [], + "title_field": "email", + "track_changes": 1 } \ No newline at end of file diff --git a/frappe/email/doctype/email_queue_recipient/email_queue_recipient.json b/frappe/email/doctype/email_queue_recipient/email_queue_recipient.json index 8d1f77e818..c217886ce6 100644 --- a/frappe/email/doctype/email_queue_recipient/email_queue_recipient.json +++ b/frappe/email/doctype/email_queue_recipient/email_queue_recipient.json @@ -1,122 +1,46 @@ { - "allow_copy": 0, - "allow_import": 0, - "allow_rename": 0, - "beta": 0, - "creation": "2016-12-08 12:01:07.993900", - "custom": 0, - "docstatus": 0, - "doctype": "DocType", - "document_type": "", - "editable_grid": 0, - "engine": "InnoDB", + "actions": [], + "creation": "2016-12-08 12:01:07.993900", + "doctype": "DocType", + "engine": "InnoDB", + "field_order": [ + "recipient", + "status", + "error" + ], "fields": [ { - "allow_on_submit": 0, - "bold": 0, - "collapsible": 0, - "columns": 0, - "fieldname": "recipient", - "fieldtype": "Data", - "hidden": 0, - "ignore_user_permissions": 0, - "ignore_xss_filter": 0, - "in_filter": 0, - "in_list_view": 1, - "label": "Recipient", - "length": 0, - "no_copy": 0, - "options": "Email", - "permlevel": 0, - "precision": "", - "print_hide": 0, - "print_hide_if_no_value": 0, - "read_only": 0, - "remember_last_selected_value": 0, - "report_hide": 0, - "reqd": 0, - "search_index": 0, - "set_only_once": 0, - "unique": 0 - }, + "fieldname": "recipient", + "fieldtype": "Data", + "in_list_view": 1, + "label": "Recipient", + "options": "Email" + }, { - "allow_on_submit": 0, - "bold": 0, - "collapsible": 0, - "columns": 0, - "default": "Not Sent", - "fieldname": "status", - "fieldtype": "Select", - "hidden": 0, - "ignore_user_permissions": 0, - "ignore_xss_filter": 0, - "in_filter": 0, - "in_list_view": 1, - "label": "Status", - "length": 0, - "no_copy": 0, - "options": "\nNot Sent\nSending\nSent\nError\nExpired", - "permlevel": 0, - "precision": "", - "print_hide": 0, - "print_hide_if_no_value": 0, - "read_only": 0, - "remember_last_selected_value": 0, - "report_hide": 0, - "reqd": 0, - "search_index": 0, - "set_only_once": 0, - "unique": 0 - }, + "default": "Not Sent", + "fieldname": "status", + "fieldtype": "Select", + "in_list_view": 1, + "label": "Status", + "options": "\nNot Sent\nSending\nSent\nError\nExpired", + "search_index": 1 + }, { - "allow_on_submit": 0, - "bold": 0, - "collapsible": 0, - "columns": 0, - "fieldname": "error", - "fieldtype": "Code", - "hidden": 0, - "ignore_user_permissions": 0, - "ignore_xss_filter": 0, - "in_filter": 0, - "in_list_view": 0, - "label": "Error", - "length": 0, - "no_copy": 0, - "permlevel": 0, - "precision": "", - "print_hide": 0, - "print_hide_if_no_value": 0, - "read_only": 0, - "remember_last_selected_value": 0, - "report_hide": 0, - "reqd": 0, - "search_index": 0, - "set_only_once": 0, - "unique": 0 + "fieldname": "error", + "fieldtype": "Code", + "label": "Error" } - ], - "hide_heading": 0, - "hide_toolbar": 0, - "idx": 0, - "image_view": 0, - "in_create": 0, - - "is_submittable": 0, - "issingle": 0, - "istable": 1, - "max_attachments": 0, - "modified": "2016-12-08 14:05:33.578240", - "modified_by": "Administrator", - "module": "Email", - "name": "Email Queue Recipient", - "name_case": "", - "owner": "Administrator", - "permissions": [], - "quick_entry": 1, - "read_only": 0, - "read_only_onload": 0, - "sort_field": "modified", - "sort_order": "DESC", - "track_seen": 0 + ], + "istable": 1, + "links": [], + "modified": "2022-07-11 16:38:10.644417", + "modified_by": "Administrator", + "module": "Email", + "name": "Email Queue Recipient", + "owner": "Administrator", + "permissions": [], + "quick_entry": 1, + "sort_field": "modified", + "sort_order": "DESC", + "states": [] } \ No newline at end of file diff --git a/frappe/email/doctype/newsletter/newsletter.py b/frappe/email/doctype/newsletter/newsletter.py index 913553fc57..137529f39b 100644 --- a/frappe/email/doctype/newsletter/newsletter.py +++ b/frappe/email/doctype/newsletter/newsletter.py @@ -139,7 +139,8 @@ class Newsletter(WebsiteGenerator): """Get list of pending recipients of the newsletter. These recipients may not have receive the newsletter in the previous iteration. """ - return [x for x in self.newsletter_recipients if x not in self.get_success_recipients()] + success_recipients = set(self.get_success_recipients()) + return [x for x in self.newsletter_recipients if x not in success_recipients] def queue_all(self): """Queue Newsletter to all the recipients generated from the `Email Group` table""" From 8f5650de5b8c050168cc516907954c70bf837e19 Mon Sep 17 00:00:00 2001 From: Suraj Shetty Date: Mon, 11 Jul 2022 17:43:56 +0530 Subject: [PATCH 16/80] fix: Use errored email stats while showing send status --- frappe/email/doctype/newsletter/newsletter.js | 12 ++++++------ frappe/email/doctype/newsletter/newsletter.py | 5 ++++- 2 files changed, 10 insertions(+), 7 deletions(-) diff --git a/frappe/email/doctype/newsletter/newsletter.js b/frappe/email/doctype/newsletter/newsletter.js index 7a47bbb615..3701a08dbe 100644 --- a/frappe/email/doctype/newsletter/newsletter.js +++ b/frappe/email/doctype/newsletter/newsletter.js @@ -140,8 +140,8 @@ frappe.ui.form.on('Newsletter', { if (frm.doc.email_sent && frm.$wrapper.is(':visible')) { let res = await frm.call('get_sending_status'); let stats = res.message; - stats && frm.events.update_sending_progress(frm, stats.sent, stats.total); - if (stats.sent >= frm.doc.total_recipients || !stats.total) { + stats && frm.events.update_sending_progress(frm, stats); + if (stats.sent + stats.error >= frm.doc.total_recipients || !stats.total) { frm.sending_status && clearInterval(frm.sending_status); frm.sending_status = null; } @@ -151,15 +151,15 @@ frappe.ui.form.on('Newsletter', { frm.sending_status = setInterval(() => frm.events.update_sending_status(frm), 5000); }, - update_sending_progress(frm, sent, total) { - if (sent >= frm.doc.total_recipients || !frm.doc.email_sent) { + update_sending_progress(frm, stats) { + if (stats.sent + stats.error >= frm.doc.total_recipients || !frm.doc.email_sent) { frm.doc.email_sent && frm.page.set_indicator(__("Sent"), "green"); frm.dashboard.hide_progress(); return; } - if (total) { + if (stats.total) { frm.page.set_indicator(__("Sending"), "blue"); - frm.dashboard.show_progress(__('Sending emails'), sent * 100 / frm.doc.total_recipients, __("{0} of {1} sent", [sent, frm.doc.total_recipients])); + frm.dashboard.show_progress(__('Sending emails'), stats.sent * 100 / frm.doc.total_recipients, __("{0} of {1} sent", [stats.sent, frm.doc.total_recipients])); } }, diff --git a/frappe/email/doctype/newsletter/newsletter.py b/frappe/email/doctype/newsletter/newsletter.py index 137529f39b..8611f51a52 100644 --- a/frappe/email/doctype/newsletter/newsletter.py +++ b/frappe/email/doctype/newsletter/newsletter.py @@ -35,13 +35,16 @@ class Newsletter(WebsiteGenerator): order_by="status", ) sent = 0 + error = 0 total = 0 for row in count_by_status: if row.status == "Sent": sent = row.count + elif row.status == "Error": + error = row.count total += row.count - return {"sent": sent, "total": total} + return {"sent": sent, "error": error, "total": total} @frappe.whitelist() def send_test_email(self, email): From d4fe14267456ff67009ff4092ed2b1683a57aa1d Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Mon, 11 Jul 2022 16:18:12 +0530 Subject: [PATCH 17/80] fix(UX): correctly validate python condition --- frappe/integrations/doctype/webhook/webhook.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frappe/integrations/doctype/webhook/webhook.py b/frappe/integrations/doctype/webhook/webhook.py index 27fdd662ed..40ca9dfc20 100644 --- a/frappe/integrations/doctype/webhook/webhook.py +++ b/frappe/integrations/doctype/webhook/webhook.py @@ -47,7 +47,7 @@ class Webhook(Document): try: frappe.safe_eval(self.condition, eval_locals=get_context(temp_doc)) except Exception as e: - frappe.throw(_(e)) + frappe.throw(_("Invalid Condition: {}").format(e)) def validate_request_url(self): try: From 1531e360049ca83289737bd672fadbdc0b3760a1 Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Mon, 11 Jul 2022 16:46:34 +0530 Subject: [PATCH 18/80] fix: allow JSON array as request body --- .../integrations/doctype/webhook/__init__.py | 1 + .../doctype/webhook/test_webhook.py | 40 +++++++++++++++++++ .../integrations/doctype/webhook/webhook.py | 6 +-- 3 files changed, 44 insertions(+), 3 deletions(-) diff --git a/frappe/integrations/doctype/webhook/__init__.py b/frappe/integrations/doctype/webhook/__init__.py index 8e9c72c1a9..192cd2fa12 100644 --- a/frappe/integrations/doctype/webhook/__init__.py +++ b/frappe/integrations/doctype/webhook/__init__.py @@ -17,6 +17,7 @@ def run_webhooks(doc, method): if frappe.flags.webhooks_executed is None: frappe.flags.webhooks_executed = {} + # TODO: remove this hazardous unnecessary cache in flags if frappe.flags.webhooks is None: # load webhooks from cache webhooks = frappe.cache().get_value("webhooks") diff --git a/frappe/integrations/doctype/webhook/test_webhook.py b/frappe/integrations/doctype/webhook/test_webhook.py index 5ac75f5b52..7d9d05cd9e 100644 --- a/frappe/integrations/doctype/webhook/test_webhook.py +++ b/frappe/integrations/doctype/webhook/test_webhook.py @@ -1,6 +1,8 @@ # Copyright (c) 2017, Frappe Technologies and Contributors # License: MIT. See LICENSE +import json import unittest +from contextlib import contextmanager import frappe from frappe.integrations.doctype.webhook.webhook import ( @@ -10,6 +12,16 @@ from frappe.integrations.doctype.webhook.webhook import ( ) +@contextmanager +def get_test_webhook(config): + wh = frappe.get_doc(config).insert() + wh.reload() + try: + yield wh + finally: + wh.delete() + + class TestWebhook(unittest.TestCase): @classmethod def setUpClass(cls): @@ -165,3 +177,31 @@ class TestWebhook(unittest.TestCase): enqueue_webhook(user, webhook) self.assertTrue(frappe.db.get_all("Webhook Request Log", pluck="name")) + + def test_webhook_with_array_body(self): + """Check if array request body are supported.""" + wh_config = { + "doctype": "Webhook", + "webhook_doctype": "Note", + "webhook_docevent": "after_insert", + "enabled": 1, + "request_url": "https://httpbin.org/post", + "request_method": "POST", + "request_structure": "JSON", + "webhook_json": '[\r\n{% for n in range(3) %}\r\n {\r\n "title": "{{ doc.title }}",\r\n "n": {{ n }}\r\n }\r\n {%- if not loop.last -%}\r\n , \r\n {%endif%}\r\n{%endfor%}\r\n]', + "meets_condition": "Yes", + "webhook_headers": [ + { + "key": "Content-Type", + "value": "application/json", + } + ], + } + + with get_test_webhook(wh_config) as wh: + doc = frappe.new_doc("Note") + doc.title = "Test Webhook Note" + + enqueue_webhook(doc, wh) + log = frappe.get_last_doc("Webhook Request Log") + self.assertEqual(len(json.loads(log.response)["json"]), 3) diff --git a/frappe/integrations/doctype/webhook/webhook.py b/frappe/integrations/doctype/webhook/webhook.py index 40ca9dfc20..faebfbf78e 100644 --- a/frappe/integrations/doctype/webhook/webhook.py +++ b/frappe/integrations/doctype/webhook/webhook.py @@ -118,9 +118,9 @@ def log_request(url: str, headers: dict, data: dict, res: requests.Response | No "doctype": "Webhook Request Log", "user": frappe.session.user if frappe.session.user else None, "url": url, - "headers": json.dumps(headers, indent=4) if headers else None, - "data": json.dumps(data, indent=4) if isinstance(data, dict) else data, - "response": json.dumps(res.json(), indent=4) if res else None, + "headers": frappe.as_json(headers) if headers else None, + "data": frappe.as_json(data) if data else None, + "response": frappe.as_json(res.json()) if res else None, } ) From d5820213f05211e415820aaf909d8da070df948a Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Mon, 11 Jul 2022 17:01:06 +0530 Subject: [PATCH 19/80] feat: preview Webhook request data --- .../integrations/doctype/webhook/webhook.js | 11 +++++ .../integrations/doctype/webhook/webhook.json | 43 ++++++++++++++++++- .../integrations/doctype/webhook/webhook.py | 34 ++++++++++++++- 3 files changed, 85 insertions(+), 3 deletions(-) diff --git a/frappe/integrations/doctype/webhook/webhook.js b/frappe/integrations/doctype/webhook/webhook.js index 0953e60625..f4cb4373ea 100644 --- a/frappe/integrations/doctype/webhook/webhook.js +++ b/frappe/integrations/doctype/webhook/webhook.js @@ -72,6 +72,17 @@ frappe.ui.form.on('Webhook', { enable_security: (frm) => { frm.toggle_reqd('webhook_secret', frm.doc.enable_security); + }, + + preview_document: (frm) => { + frappe.call({ + method: "generate_preview", + doc: frm.doc, + callback: (r) => { + frm.refresh_field("meets_condition"); + frm.refresh_field("preview_request_body"); + }, + }); } }); diff --git a/frappe/integrations/doctype/webhook/webhook.json b/frappe/integrations/doctype/webhook/webhook.json index 880874cb25..a21e460659 100644 --- a/frappe/integrations/doctype/webhook/webhook.json +++ b/frappe/integrations/doctype/webhook/webhook.json @@ -28,7 +28,13 @@ "webhook_headers", "sb_webhook_data", "webhook_data", - "webhook_json" + "webhook_json", + "preview_tab", + "preview_document", + "column_break_26", + "meets_condition", + "section_break_28", + "preview_request_body" ], "fields": [ { @@ -163,13 +169,45 @@ "label": "Request Method", "options": "POST\nPUT\nDELETE", "reqd": 1 + }, + { + "fieldname": "preview_tab", + "fieldtype": "Tab Break", + "label": "Preview" + }, + { + "fieldname": "preview_document", + "fieldtype": "Dynamic Link", + "label": "Select Document", + "options": "webhook_doctype" + }, + { + "fieldname": "preview_request_body", + "fieldtype": "Code", + "is_virtual": 1, + "label": "Request Body" + }, + { + "fieldname": "meets_condition", + "fieldtype": "Data", + "is_virtual": 1, + "label": "Meets Condition?" + }, + { + "fieldname": "column_break_26", + "fieldtype": "Column Break" + }, + { + "fieldname": "section_break_28", + "fieldtype": "Section Break" } ], "links": [], - "modified": "2021-05-25 11:11:28.555291", + "modified": "2022-07-11 08:54:10.740512", "modified_by": "Administrator", "module": "Integrations", "name": "Webhook", + "naming_rule": "By \"Naming Series\" field", "owner": "Administrator", "permissions": [ { @@ -187,6 +225,7 @@ ], "sort_field": "modified", "sort_order": "DESC", + "states": [], "title_field": "webhook_doctype", "track_changes": 1 } \ No newline at end of file diff --git a/frappe/integrations/doctype/webhook/webhook.py b/frappe/integrations/doctype/webhook/webhook.py index faebfbf78e..64a98a61e1 100644 --- a/frappe/integrations/doctype/webhook/webhook.py +++ b/frappe/integrations/doctype/webhook/webhook.py @@ -2,7 +2,6 @@ # License: MIT. See LICENSE import base64 -import datetime import hashlib import hmac import json @@ -27,6 +26,7 @@ class Webhook(Document): self.validate_request_url() self.validate_request_body() self.validate_repeating_fields() + self.preview_document = None def on_update(self): frappe.cache().delete_value("webhooks") @@ -74,6 +74,38 @@ class Webhook(Document): if len(webhook_data) != len(set(webhook_data)): frappe.throw(_("Same Field is entered more than once")) + @frappe.whitelist() + def generate_preview(self): + # This function doesn't need to do anything specific as virtual fields + # get evaluated automatically. + pass + + @property + def meets_condition(self): + if not self.condition: + return _("Yes") + + if not (self.preview_document and self.webhook_doctype): + return _("Select a document to check if it meets conditions.") + + try: + doc = frappe.get_cached_doc(self.webhook_doctype, self.preview_document) + met_condition = frappe.safe_eval(self.condition, eval_locals=get_context(doc)) + except Exception as e: + return _("Failed to evaluate conditions: {}").format(e) + return _("Yes") if met_condition else _("No") + + @property + def preview_request_body(self): + if not (self.preview_document and self.webhook_doctype): + return _("Select a document to preview request data") + + try: + doc = frappe.get_cached_doc(self.webhook_doctype, self.preview_document) + return frappe.as_json(get_webhook_data(doc, self)) + except Exception as e: + return _("Failed to compute request body: {}").format(e) + def get_context(doc): return {"doc": doc, "utils": get_safe_globals().get("frappe").get("utils")} From e652811d5567548acb35a0653a10067b7a6dc25f Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Mon, 11 Jul 2022 18:33:50 +0530 Subject: [PATCH 20/80] fix: ignore virtual fields when doing db_update --- frappe/model/base_document.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/frappe/model/base_document.py b/frappe/model/base_document.py index d3e7656d6d..0207571e14 100644 --- a/frappe/model/base_document.py +++ b/frappe/model/base_document.py @@ -539,7 +539,9 @@ class BaseDocument: return d = self.get_valid_dict( - convert_dates_to_str=True, ignore_nulls=self.doctype in DOCTYPES_FOR_DOCTYPE + convert_dates_to_str=True, + ignore_nulls=self.doctype in DOCTYPES_FOR_DOCTYPE, + ignore_virtual=True, ) # don't update name, as case might've been changed From a574e3c09e49e0e092b21ba4a6bc57f68c9f9272 Mon Sep 17 00:00:00 2001 From: Suraj Shetty Date: Mon, 11 Jul 2022 18:55:43 +0530 Subject: [PATCH 21/80] fix: Send emails immediately during tests --- frappe/email/doctype/email_queue/email_queue.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/frappe/email/doctype/email_queue/email_queue.py b/frappe/email/doctype/email_queue/email_queue.py index dfad6d9ce8..f9124a3667 100644 --- a/frappe/email/doctype/email_queue/email_queue.py +++ b/frappe/email/doctype/email_queue/email_queue.py @@ -649,7 +649,7 @@ class QueueBuilder: q = EmailQueue.new({**queue_data, **{"recipients": recipients}}, ignore_permissions=True) send_now and q.send() else: - if send_now and len(recipients) >= 1000: + if send_now and len(final_recipients) >= 1000: # force queueing if there are too many recipients to avoid timeouts send_now = False for recipients in frappe.utils.create_batch(final_recipients, 1000): @@ -657,7 +657,7 @@ class QueueBuilder: self.send_emails, queue_data=queue_data, final_recipients=recipients, - now=send_now, + now=frappe.flags.in_test or send_now, ) def send_emails(self, queue_data, final_recipients): From 2c2368e973493daa0c82a91ac3c5393f849509b8 Mon Sep 17 00:00:00 2001 From: Shariq Ansari <30859809+shariquerik@users.noreply.github.com> Date: Tue, 12 Jul 2022 12:12:39 +0530 Subject: [PATCH 22/80] fix: Datetime field not getting saved if use NOW button. (#17452) --- frappe/public/js/frappe/form/controls/datetime.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frappe/public/js/frappe/form/controls/datetime.js b/frappe/public/js/frappe/form/controls/datetime.js index 43873b3b1e..a086b1b879 100644 --- a/frappe/public/js/frappe/form/controls/datetime.js +++ b/frappe/public/js/frappe/form/controls/datetime.js @@ -85,6 +85,6 @@ frappe.ui.form.ControlDatetime = class ControlDatetime extends frappe.ui.form.Co if (!value && !this.doc) { value = this.last_value; } - return frappe.datetime.get_datetime_as_string(value); + return !value ? "" : frappe.datetime.get_datetime_as_string(value); } }; From 2b0a3533d33caf7871880d4e154cd801ab5fad91 Mon Sep 17 00:00:00 2001 From: Suraj Shetty Date: Tue, 12 Jul 2022 12:58:15 +0530 Subject: [PATCH 23/80] test: Update test case according to changes --- frappe/email/test_email_body.py | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/frappe/email/test_email_body.py b/frappe/email/test_email_body.py index 3ff4245924..d5b1013a73 100644 --- a/frappe/email/test_email_body.py +++ b/frappe/email/test_email_body.py @@ -5,6 +5,7 @@ import base64 import os import unittest +import frappe from frappe import safe_decode from frappe.email.doctype.email_queue.email_queue import QueueBuilder, SendMailContext from frappe.email.email_body import ( @@ -54,26 +55,27 @@ This is the text version of this email uni_chr1 = chr(40960) uni_chr2 = chr(1972) - queue_doc = QueueBuilder( + QueueBuilder( recipients=["test@example.com"], sender="me@example.com", subject="Test Subject", - message="

" + uni_chr1 + "abcd" + uni_chr2 + "

", + message=f"

{uni_chr1}abcd{uni_chr2}

", text_content="whatever", - ).process()[0] + ).process() + queue_doc = frappe.get_last_doc("Email Queue") mail_ctx = SendMailContext(queue_doc=queue_doc) result = mail_ctx.build_message(recipient_email="test@test.com") self.assertTrue(b"

=EA=80=80abcd=DE=B4

" in result) def test_prepare_message_returns_cr_lf(self): - queue_doc = QueueBuilder( + QueueBuilder( recipients=["test@example.com"], sender="me@example.com", subject="Test Subject", message="

\n this is a test of newlines\n" + "

", text_content="whatever", - ).process()[0] - + ).process() + queue_doc = frappe.get_last_doc("Email Queue") mail_ctx = SendMailContext(queue_doc=queue_doc) result = safe_decode(mail_ctx.build_message(recipient_email="test@test.com")) From 5c72181f2291a2c2f0c5b7b1b66117195ea11417 Mon Sep 17 00:00:00 2001 From: Suraj Shetty Date: Tue, 12 Jul 2022 12:59:53 +0530 Subject: [PATCH 24/80] perf(email queue): Index status, reference_doctype & reference_docname --- frappe/email/doctype/email_queue/email_queue.json | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/frappe/email/doctype/email_queue/email_queue.json b/frappe/email/doctype/email_queue/email_queue.json index f251786c90..7a4b8dec45 100644 --- a/frappe/email/doctype/email_queue/email_queue.json +++ b/frappe/email/doctype/email_queue/email_queue.json @@ -58,7 +58,8 @@ "in_list_view": 1, "in_standard_filter": 1, "label": "Status", - "options": "\nNot Sent\nSending\nSent\nError\nExpired" + "options": "\nNot Sent\nSending\nSent\nError\nExpired", + "search_index": 1 }, { "fieldname": "error", @@ -77,13 +78,15 @@ "fieldtype": "Link", "label": "Reference Document Type", "options": "DocType", - "read_only": 1 + "read_only": 1, + "search_index": 1 }, { "fieldname": "reference_name", "fieldtype": "Data", "label": "Reference DocName", - "read_only": 1 + "read_only": 1, + "search_index": 1 }, { "fieldname": "communication", @@ -152,10 +155,11 @@ "idx": 1, "in_create": 1, "links": [], - "modified": "2021-04-29 06:33:25.191729", + "modified": "2022-07-12 12:52:26.290072", "modified_by": "Administrator", "module": "Email", "name": "Email Queue", + "naming_rule": "Random", "owner": "Administrator", "permissions": [ { @@ -169,5 +173,6 @@ ], "sort_field": "modified", "sort_order": "DESC", + "states": [], "track_changes": 1 } \ No newline at end of file From 34b1ea57f9b76286af2d19542b7d811e2dfc6bf0 Mon Sep 17 00:00:00 2001 From: Suraj Shetty Date: Tue, 12 Jul 2022 13:03:06 +0530 Subject: [PATCH 25/80] fix: Show "Queued" status on newsletter Show "Queued" status on newsletter if emails are queued and "email sending" is not yet started. --- frappe/email/doctype/email_queue/email_queue.py | 4 ++++ frappe/email/doctype/newsletter/newsletter.js | 4 +++- frappe/email/doctype/newsletter/newsletter.py | 8 ++++++-- frappe/utils/data.py | 9 +++++++++ 4 files changed, 22 insertions(+), 3 deletions(-) diff --git a/frappe/email/doctype/email_queue/email_queue.py b/frappe/email/doctype/email_queue/email_queue.py index f9124a3667..eb07be0b38 100644 --- a/frappe/email/doctype/email_queue/email_queue.py +++ b/frappe/email/doctype/email_queue/email_queue.py @@ -657,7 +657,11 @@ class QueueBuilder: self.send_emails, queue_data=queue_data, final_recipients=recipients, + job_name=frappe.utils.get_job_name( + "send_bulk_emails_for", self.reference_doctype, self.reference_name + ), now=frappe.flags.in_test or send_now, + queue="long", ) def send_emails(self, queue_data, final_recipients): diff --git a/frappe/email/doctype/newsletter/newsletter.js b/frappe/email/doctype/newsletter/newsletter.js index 3701a08dbe..9cfdca1a68 100644 --- a/frappe/email/doctype/newsletter/newsletter.js +++ b/frappe/email/doctype/newsletter/newsletter.js @@ -141,7 +141,7 @@ frappe.ui.form.on('Newsletter', { let res = await frm.call('get_sending_status'); let stats = res.message; stats && frm.events.update_sending_progress(frm, stats); - if (stats.sent + stats.error >= frm.doc.total_recipients || !stats.total) { + if (stats.sent + stats.error >= frm.doc.total_recipients || (!stats.total && !stats.emails_queued)) { frm.sending_status && clearInterval(frm.sending_status); frm.sending_status = null; } @@ -160,6 +160,8 @@ frappe.ui.form.on('Newsletter', { if (stats.total) { frm.page.set_indicator(__("Sending"), "blue"); frm.dashboard.show_progress(__('Sending emails'), stats.sent * 100 / frm.doc.total_recipients, __("{0} of {1} sent", [stats.sent, frm.doc.total_recipients])); + } else if (stats.emails_queued) { + frm.page.set_indicator(__("Queued"), "blue"); } }, diff --git a/frappe/email/doctype/newsletter/newsletter.py b/frappe/email/doctype/newsletter/newsletter.py index 8611f51a52..b0cbb1993d 100644 --- a/frappe/email/doctype/newsletter/newsletter.py +++ b/frappe/email/doctype/newsletter/newsletter.py @@ -6,6 +6,7 @@ import frappe import frappe.utils from frappe import _ from frappe.email.doctype.email_group.email_group import add_subscribers +from frappe.utils.safe_exec import is_job_queued from frappe.utils.verified_command import get_signed_params, verify_request from frappe.website.website_generator import WebsiteGenerator @@ -43,8 +44,11 @@ class Newsletter(WebsiteGenerator): elif row.status == "Error": error = row.count total += row.count - - return {"sent": sent, "error": error, "total": total} + emails_queued = is_job_queued( + job_name=frappe.utils.get_job_name("send_bulk_emails_for", self.doctype, self.name), + queue="long", + ) + return {"sent": sent, "error": error, "total": total, "emails_queued": emails_queued} @frappe.whitelist() def send_test_email(self, email): diff --git a/frappe/utils/data.py b/frappe/utils/data.py index 7a1f05220c..81ca143682 100644 --- a/frappe/utils/data.py +++ b/frappe/utils/data.py @@ -2114,3 +2114,12 @@ def parse_timedelta(s: str) -> datetime.timedelta: m = TIMEDELTA_BASE_PATTERN.match(s) return datetime.timedelta(**{key: float(val) for key, val in m.groupdict().items()}) + + +def get_job_name(key: str, doctype: str = None, doc_name: str = None) -> str: + job_name = key + if doctype: + job_name += f"_{doctype}" + if doc_name: + job_name += f"_{doc_name}" + return job_name From dd9fbeff4b1858fd24c0a85dcc020b22140857fd Mon Sep 17 00:00:00 2001 From: ruthra kumar Date: Tue, 12 Jul 2022 14:03:24 +0530 Subject: [PATCH 26/80] fix: Preserve data type in excel export except for composite --- frappe/desk/query_report.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/frappe/desk/query_report.py b/frappe/desk/query_report.py index 3faa6d84c1..e075d9a8ea 100644 --- a/frappe/desk/query_report.py +++ b/frappe/desk/query_report.py @@ -407,7 +407,8 @@ def build_xlsx_data(data, visible_idx, include_indentation, ignore_visible_idx=F continue label = column.get("label") fieldname = column.get("fieldname") - cell_value = cstr(row.get(fieldname, row.get(label, ""))) + cell_value = row.get(fieldname, row.get(label, "")) + cell_value = cstr(cell_value) if isinstance(cell_value, list) else cell_value if cint(include_indentation) and "indent" in row and col_idx == 0: cell_value = (" " * cint(row["indent"])) + cstr(cell_value) row_data.append(cell_value) From 6bcef3781ea157cb628e44aee24f7e16e7c0820c Mon Sep 17 00:00:00 2001 From: Suraj Shetty Date: Tue, 12 Jul 2022 15:20:44 +0530 Subject: [PATCH 27/80] fix(email queue): Remove unnecessary indexing --- frappe/email/doctype/email_queue/email_queue.json | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/frappe/email/doctype/email_queue/email_queue.json b/frappe/email/doctype/email_queue/email_queue.json index 7a4b8dec45..c9ec374687 100644 --- a/frappe/email/doctype/email_queue/email_queue.json +++ b/frappe/email/doctype/email_queue/email_queue.json @@ -58,8 +58,7 @@ "in_list_view": 1, "in_standard_filter": 1, "label": "Status", - "options": "\nNot Sent\nSending\nSent\nError\nExpired", - "search_index": 1 + "options": "\nNot Sent\nSending\nSent\nError\nExpired" }, { "fieldname": "error", @@ -78,8 +77,7 @@ "fieldtype": "Link", "label": "Reference Document Type", "options": "DocType", - "read_only": 1, - "search_index": 1 + "read_only": 1 }, { "fieldname": "reference_name", @@ -155,7 +153,7 @@ "idx": 1, "in_create": 1, "links": [], - "modified": "2022-07-12 12:52:26.290072", + "modified": "2022-07-12 15:17:37.934316", "modified_by": "Administrator", "module": "Email", "name": "Email Queue", From 0a73a3c708380afe8b28fb09f00aeca7e6f007aa Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Tue, 12 Jul 2022 15:47:36 +0530 Subject: [PATCH 28/80] refactor: stringify all non-excel types style: incorrect EOF test: excel export types --- frappe/desk/query_report.py | 17 +++- frappe/tests/test_query_report.py | 139 ++++++++++++++++-------------- 2 files changed, 88 insertions(+), 68 deletions(-) diff --git a/frappe/desk/query_report.py b/frappe/desk/query_report.py index e075d9a8ea..e88a453e64 100644 --- a/frappe/desk/query_report.py +++ b/frappe/desk/query_report.py @@ -1,6 +1,7 @@ # Copyright (c) 2015, Frappe Technologies Pvt. Ltd. and Contributors # License: MIT. See LICENSE +import datetime import json import os from datetime import timedelta @@ -384,6 +385,18 @@ def format_duration_fields(data: frappe._dict) -> None: def build_xlsx_data(data, visible_idx, include_indentation, ignore_visible_idx=False): + EXCEL_TYPES = ( + str, + bool, + type(None), + int, + float, + datetime.datetime, + datetime.date, + datetime.time, + datetime.timedelta, + ) + result = [[]] column_widths = [] @@ -408,7 +421,9 @@ def build_xlsx_data(data, visible_idx, include_indentation, ignore_visible_idx=F label = column.get("label") fieldname = column.get("fieldname") cell_value = row.get(fieldname, row.get(label, "")) - cell_value = cstr(cell_value) if isinstance(cell_value, list) else cell_value + if not isinstance(cell_value, EXCEL_TYPES): + cell_value = cstr(cell_value) + if cint(include_indentation) and "indent" in row and col_idx == 0: cell_value = (" " * cint(row["indent"])) + cstr(cell_value) row_data.append(cell_value) diff --git a/frappe/tests/test_query_report.py b/frappe/tests/test_query_report.py index 364cc49e97..ae96898795 100644 --- a/frappe/tests/test_query_report.py +++ b/frappe/tests/test_query_report.py @@ -1,67 +1,72 @@ -# Copyright (c) 2019, Frappe Technologies Pvt. Ltd. and Contributors -# License: MIT. See LICENSE - -import unittest - -import frappe -import frappe.utils -from frappe.desk.query_report import build_xlsx_data -from frappe.utils.xlsxutils import make_xlsx - - -class TestQueryReport(unittest.TestCase): - def test_xlsx_data_with_multiple_datatypes(self): - """Test exporting report using rows with multiple datatypes (list, dict)""" - - # Create mock data - data = frappe._dict() - data.columns = [ - {"label": "Column A", "fieldname": "column_a", "fieldtype": "Float"}, - {"label": "Column B", "fieldname": "column_b", "width": 100, "fieldtype": "Float"}, - {"label": "Column C", "fieldname": "column_c", "width": 150, "fieldtype": "Duration"}, - ] - data.result = [ - [1.0, 3.0, 600], - {"column_a": 22.1, "column_b": 21.8, "column_c": 86412}, - {"column_b": 5.1, "column_c": 53234, "column_a": 11.1}, - [3.0, 1.5, 333], - ] - - # Define the visible rows - visible_idx = [0, 2, 3] - - # Build the result - xlsx_data, column_widths = build_xlsx_data(data, visible_idx, include_indentation=0) - - self.assertEqual(type(xlsx_data), list) - self.assertEqual(len(xlsx_data), 4) # columns + data - # column widths are divided by 10 to match the scale that is supported by openpyxl - self.assertListEqual(column_widths, [0, 10, 15]) - - for row in xlsx_data: - self.assertEqual(type(row), list) - - def test_xlsx_export_with_composite_cell_value(self): - """Test excel export using rows with composite cell value""" - - data = frappe._dict() - data.columns = [ - {"label": "Column A", "fieldname": "column_a", "fieldtype": "Float"}, - {"label": "Column B", "fieldname": "column_b", "width": 150, "fieldtype": "Data"}, - ] - data.result = [ - [1.0, "Dummy 1"], - {"column_a": 22.1, "column_b": ["Dummy 1", "Dummy 2"]}, # composite value in column_b - ] - - # Define the visible rows - visible_idx = [0, 1] - - # Build the result - xlsx_data, column_widths = build_xlsx_data(data, visible_idx, include_indentation=0) - # Export to excel - make_xlsx(xlsx_data, "Query Report", column_widths=column_widths) - - for row in xlsx_data: - # column_b should be 'str' even with composite cell value - self.assertEqual(type(row[1]), str) +# Copyright (c) 2019, Frappe Technologies Pvt. Ltd. and Contributors +# License: MIT. See LICENSE + +import unittest + +import frappe +import frappe.utils +from frappe.desk.query_report import build_xlsx_data +from frappe.utils.xlsxutils import make_xlsx + + +class TestQueryReport(unittest.TestCase): + def test_xlsx_data_with_multiple_datatypes(self): + """Test exporting report using rows with multiple datatypes (list, dict)""" + + # Create mock data + data = frappe._dict() + data.columns = [ + {"label": "Column A", "fieldname": "column_a", "fieldtype": "Float"}, + {"label": "Column B", "fieldname": "column_b", "width": 100, "fieldtype": "Float"}, + {"label": "Column C", "fieldname": "column_c", "width": 150, "fieldtype": "Duration"}, + ] + data.result = [ + [1.0, 3.0, 600], + {"column_a": 22.1, "column_b": 21.8, "column_c": 86412}, + {"column_b": 5.1, "column_c": 53234, "column_a": 11.1}, + [3.0, 1.5, 333], + ] + + # Define the visible rows + visible_idx = [0, 2, 3] + + # Build the result + xlsx_data, column_widths = build_xlsx_data(data, visible_idx, include_indentation=0) + + self.assertEqual(type(xlsx_data), list) + self.assertEqual(len(xlsx_data), 4) # columns + data + # column widths are divided by 10 to match the scale that is supported by openpyxl + self.assertListEqual(column_widths, [0, 10, 15]) + + for row in xlsx_data: + self.assertIsInstance(row, list) + + # ensure all types are preserved + for row in xlsx_data[1:]: + for cell in row: + self.assertIsInstance(cell, (int, float)) + + def test_xlsx_export_with_composite_cell_value(self): + """Test excel export using rows with composite cell value""" + + data = frappe._dict() + data.columns = [ + {"label": "Column A", "fieldname": "column_a", "fieldtype": "Float"}, + {"label": "Column B", "fieldname": "column_b", "width": 150, "fieldtype": "Data"}, + ] + data.result = [ + [1.0, "Dummy 1"], + {"column_a": 22.1, "column_b": ["Dummy 1", "Dummy 2"]}, # composite value in column_b + ] + + # Define the visible rows + visible_idx = [0, 1] + + # Build the result + xlsx_data, column_widths = build_xlsx_data(data, visible_idx, include_indentation=0) + # Export to excel + make_xlsx(xlsx_data, "Query Report", column_widths=column_widths) + + for row in xlsx_data: + # column_b should be 'str' even with composite cell value + self.assertEqual(type(row[1]), str) From e6043ef4279ecb054a51e412a2c2c93db8a9fc06 Mon Sep 17 00:00:00 2001 From: Suraj Shetty <13928957+surajshetty3416@users.noreply.github.com> Date: Tue, 12 Jul 2022 16:57:20 +0530 Subject: [PATCH 29/80] fix: Wait for request to complete before firing new request Sometimes it might take more than 5s --- frappe/email/doctype/newsletter/newsletter.js | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/frappe/email/doctype/newsletter/newsletter.js b/frappe/email/doctype/newsletter/newsletter.js index 9cfdca1a68..d2e5a3c047 100644 --- a/frappe/email/doctype/newsletter/newsletter.js +++ b/frappe/email/doctype/newsletter/newsletter.js @@ -137,8 +137,10 @@ frappe.ui.form.on('Newsletter', { }, async update_sending_status(frm) { - if (frm.doc.email_sent && frm.$wrapper.is(':visible')) { + if (frm.doc.email_sent && frm.$wrapper.is(':visible') && !frm.waiting_for_request) { + frm.waiting_for_request = true; let res = await frm.call('get_sending_status'); + frm.waiting_for_request = false; let stats = res.message; stats && frm.events.update_sending_progress(frm, stats); if (stats.sent + stats.error >= frm.doc.total_recipients || (!stats.total && !stats.emails_queued)) { From 4671460b650f6054ff21191f8012bd221eae84dd Mon Sep 17 00:00:00 2001 From: Sagar Vora Date: Tue, 12 Jul 2022 11:36:06 +0000 Subject: [PATCH 30/80] fix: dont scrub key if found in defaults, but falsy (#17467) * fix: dont scrub key if found in defaults, but falsy * fix: remove redundant use of `.get` --- frappe/database/database.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/frappe/database/database.py b/frappe/database/database.py index 490b923678..6f2c1a4a1c 100644 --- a/frappe/database/database.py +++ b/frappe/database/database.py @@ -937,7 +937,10 @@ class Database: if not key: return defaults - return defaults.get(key) or defaults.get(frappe.scrub(key)) + if key in defaults: + return defaults[key] + + return defaults.get(frappe.scrub(key)) def begin(self): self.sql("START TRANSACTION") From dbbd9e90953476de6d88d492fa4a9e31605b8b53 Mon Sep 17 00:00:00 2001 From: Aradhya Date: Tue, 12 Jul 2022 17:14:42 +0530 Subject: [PATCH 31/80] fix: removing functions from strings when alias is same as function name --- frappe/database/query.py | 19 +++++-------------- frappe/tests/test_query.py | 4 ++-- 2 files changed, 7 insertions(+), 16 deletions(-) diff --git a/frappe/database/query.py b/frappe/database/query.py index 3169bc52a5..c742df9c62 100644 --- a/frappe/database/query.py +++ b/frappe/database/query.py @@ -385,6 +385,7 @@ class Engine: args = field[args_start:args_end].split(",") _, alias = field.split(" as ") if " as " in field else (None, None) + to_cast = "*" not in args _args = [] @@ -413,9 +414,7 @@ class Engine: functions = "" for func in SQL_FUNCTIONS: if f"{func}(" in fields: - _, alias = fields.split(" as ") if " as " in fields else ("", "") functions = str(func) + str(BRACKETS_PATTERN.search(fields).group()) - functions += " as " + alias return [self.get_function_object(functions)] if not functions: return [] @@ -434,26 +433,18 @@ class Engine: """Remove string functions from fields which have already been converted to function objects""" for function in function_objects: if isinstance(fields, str): - has_alias = False if function.alias: - has_alias = True - fields = BRACKETS_PATTERN.sub("", fields.replace(function.name.casefold(), "")) - if has_alias: fields = fields.replace(" as " + function.alias.casefold(), "") + fields = BRACKETS_PATTERN.sub("", fields.replace(function.name.casefold(), "")) else: updated_fields = [] for field in fields: - has_alias = False - if function.alias: - has_alias = True if isinstance(field, str): - _field = ( + if function.alias: + field = field.replace(" as " + function.alias.casefold(), "") + field = ( BRACKETS_PATTERN.sub("", field).strip().casefold().replace(function.name.casefold(), "") ) - if has_alias: - _field = _field.replace(" as " + function.alias.casefold(), "") - updated_fields.append(_field) - else: updated_fields.append(field) fields = [field for field in updated_fields if field] diff --git a/frappe/tests/test_query.py b/frappe/tests/test_query.py index 10208fec70..cdaba2ce77 100644 --- a/frappe/tests/test_query.py +++ b/frappe/tests/test_query.py @@ -110,9 +110,9 @@ class TestQuery(unittest.TestCase): self.assertEqual( frappe.qb.engine.get_query( - user_doctype, fields=["Count(name) as c", "email as id"], filters={} + user_doctype, fields=["Count(name) as count", "email as id"], filters={} ).get_sql(), frappe.qb.from_(user_doctype) - .select(user_doctype.email.as_("id"), Count(Field("name")).as_("c")) + .select(user_doctype.email.as_("id"), Count(Field("name")).as_("count")) .get_sql(), ) From 9530c90c09ae8d70160f13eab03dfd0890727b78 Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Tue, 12 Jul 2022 18:19:39 +0530 Subject: [PATCH 32/80] fix(quill): don't go beyond container for bubble (#17475) Container uses bubble type quill editor, options for which seem to spill out and hide behind the commend container. This fixes the UI glitch. ref: https://quilljs.com/docs/configuration/#bounds --- frappe/public/js/frappe/form/controls/comment.js | 1 + 1 file changed, 1 insertion(+) diff --git a/frappe/public/js/frappe/form/controls/comment.js b/frappe/public/js/frappe/form/controls/comment.js index b9b2d6a987..4e1c7f97a4 100644 --- a/frappe/public/js/frappe/form/controls/comment.js +++ b/frappe/public/js/frappe/form/controls/comment.js @@ -71,6 +71,7 @@ frappe.ui.form.ControlComment = class ControlComment extends frappe.ui.form.Cont const options = super.get_quill_options(); return Object.assign(options, { theme: 'bubble', + bounds: this.quill_container[0], modules: Object.assign(options.modules, { mention: this.get_mention_options() }) From ca39cfb11d886c92287677e7e2135aec47523485 Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Tue, 12 Jul 2022 18:38:28 +0530 Subject: [PATCH 33/80] feat: support strikethrough in text-editor/comments (#17478) IMO strikethrough is useful way more often (than many other options that we already support in editor) so adding it in default config. closes #17470 --- frappe/public/js/frappe/form/controls/comment.js | 2 +- frappe/public/js/frappe/form/controls/text_editor.js | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/frappe/public/js/frappe/form/controls/comment.js b/frappe/public/js/frappe/form/controls/comment.js index 4e1c7f97a4..4550d7045f 100644 --- a/frappe/public/js/frappe/form/controls/comment.js +++ b/frappe/public/js/frappe/form/controls/comment.js @@ -103,7 +103,7 @@ frappe.ui.form.ControlComment = class ControlComment extends frappe.ui.form.Cont get_toolbar_options() { return [ - ['bold', 'italic', 'underline'], + ['bold', 'italic', 'underline', 'strike'], ['blockquote', 'code-block'], [{ 'direction': "rtl" }], ['link', 'image'], diff --git a/frappe/public/js/frappe/form/controls/text_editor.js b/frappe/public/js/frappe/form/controls/text_editor.js index d190e11cea..e4e1fff18a 100644 --- a/frappe/public/js/frappe/form/controls/text_editor.js +++ b/frappe/public/js/frappe/form/controls/text_editor.js @@ -184,7 +184,7 @@ frappe.ui.form.ControlTextEditor = class ControlTextEditor extends frappe.ui.for return [ [{ header: [1, 2, 3, false] }], [{ size: font_sizes }], - ['bold', 'italic', 'underline', 'clean'], + ['bold', 'italic', 'underline', 'strike', 'clean'], [{ 'color': [] }, { 'background': [] }], ['blockquote', 'code-block'], // Adding Direction tool to give the user the ability to change text direction. From fbb89bdfe9f91e7628759b048cc159461eb3ae72 Mon Sep 17 00:00:00 2001 From: Ritwik Puri Date: Tue, 12 Jul 2022 18:58:07 +0530 Subject: [PATCH 34/80] fix: delete user mention cache when a user is disabled or enabled (#17451) User Mention cache is deleted on 3 occasions: * when a new user is inserted * when allowed_in_mention or user_type value has changed * when a user is deleted But we didn't delete it when a user was enabled or disabled as we maintain the mention cache for enabled users --- frappe/core/doctype/user/user.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/frappe/core/doctype/user/user.py b/frappe/core/doctype/user/user.py index bc5c20eb92..d6fe883fae 100644 --- a/frappe/core/doctype/user/user.py +++ b/frappe/core/doctype/user/user.py @@ -134,11 +134,11 @@ class User(Document): if self.time_zone: frappe.defaults.set_default("time_zone", self.time_zone, self.name) - if self.has_value_changed("allow_in_mentions") or self.has_value_changed("user_type"): - frappe.cache().delete_key("users_for_mentions") - if self.has_value_changed("enabled"): + frappe.cache().delete_key("users_for_mentions") frappe.cache().delete_key("enabled_users") + elif self.has_value_changed("allow_in_mentions") or self.has_value_changed("user_type"): + frappe.cache().delete_key("users_for_mentions") def has_website_permission(self, ptype, user, verbose=False): """Returns true if current user is the session user""" From 7a59fc7ecf20f97ba97097f167fb0eee5b3787a3 Mon Sep 17 00:00:00 2001 From: Sagar Vora Date: Tue, 12 Jul 2022 15:15:49 +0000 Subject: [PATCH 35/80] fix: simplify `defaults._clear_cache` (#17485) --- frappe/defaults.py | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/frappe/defaults.py b/frappe/defaults.py index c2f4a3fe56..d0a49ef692 100644 --- a/frappe/defaults.py +++ b/frappe/defaults.py @@ -241,8 +241,4 @@ def get_defaults_for(parent="__default"): def _clear_cache(parent): - if parent in common_default_keys: - frappe.clear_cache() - else: - clear_notifications(user=parent) - frappe.clear_cache(user=parent) + frappe.clear_cache(user=parent if parent not in common_default_keys else None) From ff734532aad97ca5abbb7bbbe75f02fdc3a3979e Mon Sep 17 00:00:00 2001 From: vorasmit Date: Tue, 12 Jul 2022 21:06:52 +0530 Subject: [PATCH 36/80] fix: set primary action after clearing previous action (#17454) --- frappe/public/js/frappe/ui/dialog.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/frappe/public/js/frappe/ui/dialog.js b/frappe/public/js/frappe/ui/dialog.js index 1618db9939..3e2d22ffa2 100644 --- a/frappe/public/js/frappe/ui/dialog.js +++ b/frappe/public/js/frappe/ui/dialog.js @@ -145,7 +145,8 @@ frappe.ui.Dialog = class Dialog extends frappe.ui.FieldGroup { return this.get_primary_btn() .removeClass("hide") .html(label) - .click(function() { + .off('click') + .on('click', function() { me.primary_action_fulfilled = true; // get values and send it // as first parameter to click callback From 64463791a18e4fc9c76698d72a69e841952b4e7a Mon Sep 17 00:00:00 2001 From: Aradhya Date: Tue, 12 Jul 2022 22:07:11 +0530 Subject: [PATCH 37/80] feat: Added support for multiple functions in string fields & fixed aliasing --- frappe/database/query.py | 13 ++++++------- frappe/tests/test_query.py | 9 +++++++++ 2 files changed, 15 insertions(+), 7 deletions(-) diff --git a/frappe/database/query.py b/frappe/database/query.py index c742df9c62..10824f4c91 100644 --- a/frappe/database/query.py +++ b/frappe/database/query.py @@ -16,6 +16,7 @@ TAB_PATTERN = re.compile("^tab") WORDS_PATTERN = re.compile(r"\w+") BRACKETS_PATTERN = re.compile(r"\(.*?\)|$") SQL_FUNCTIONS = [sql_function.value for sql_function in SqlFunctions] +COMMA_PATTERN = re.compile(r",\s*(?![^()]*\))") if TYPE_CHECKING: from pypika.functions import Function @@ -411,13 +412,8 @@ class Engine: return getattr(functions, func)(*_args, alias=alias or None) def function_objects_from_string(self, fields): - functions = "" - for func in SQL_FUNCTIONS: - if f"{func}(" in fields: - functions = str(func) + str(BRACKETS_PATTERN.search(fields).group()) - return [self.get_function_object(functions)] - if not functions: - return [] + fields = list(map(lambda str: str.strip(), COMMA_PATTERN.split(fields))) + return self.function_objects_from_list(fields=fields) def function_objects_from_list(self, fields): functions = [] @@ -436,6 +432,9 @@ class Engine: if function.alias: fields = fields.replace(" as " + function.alias.casefold(), "") fields = BRACKETS_PATTERN.sub("", fields.replace(function.name.casefold(), "")) + # Check if only comma is left in fields after stripping functions. + if "," in fields and (len(fields.strip()) == 1): + fields = "" else: updated_fields = [] for field in fields: diff --git a/frappe/tests/test_query.py b/frappe/tests/test_query.py index cdaba2ce77..8afcaf07d0 100644 --- a/frappe/tests/test_query.py +++ b/frappe/tests/test_query.py @@ -74,6 +74,15 @@ class TestQuery(unittest.TestCase): frappe.qb.from_("User").select(Timestamp(Field("creation"), Field("modified"))).get_sql(), ) + self.assertEqual( + frappe.qb.engine.get_query( + "User", fields="Count(name) as count, Max(email) as max_email", filters={} + ).get_sql(), + frappe.qb.from_("User") + .select(Count(Field("name")).as_("count"), Max(Field("email")).as_("max_email")) + .get_sql(), + ) + def test_qb_fields(self): user_doctype = frappe.qb.DocType("User") self.assertEqual( From 7658c60f574377b7d33144c3c544fe3551facf4b Mon Sep 17 00:00:00 2001 From: Aradhya Date: Wed, 13 Jul 2022 03:19:58 +0530 Subject: [PATCH 38/80] feat: Added fall back for custom functions --- frappe/database/query.py | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/frappe/database/query.py b/frappe/database/query.py index 10824f4c91..0f0b7362f7 100644 --- a/frappe/database/query.py +++ b/frappe/database/query.py @@ -10,7 +10,7 @@ from frappe import _ from frappe.boot import get_additional_filters_from_hooks from frappe.model.db_query import get_timespan_date_range from frappe.query_builder import Criterion, Field, Order, Table, functions -from frappe.query_builder.functions import SqlFunctions +from frappe.query_builder.functions import Function, SqlFunctions TAB_PATTERN = re.compile("^tab") WORDS_PATTERN = re.compile(r"\w+") @@ -18,9 +18,6 @@ BRACKETS_PATTERN = re.compile(r"\(.*?\)|$") SQL_FUNCTIONS = [sql_function.value for sql_function in SqlFunctions] COMMA_PATTERN = re.compile(r",\s*(?![^()]*\))") -if TYPE_CHECKING: - from pypika.functions import Function - def like(key: Field, value: str) -> frappe.qb: """Wrapper method for `LIKE` @@ -409,7 +406,11 @@ class Engine: field = initial_fields _args.append(field) - return getattr(functions, func)(*_args, alias=alias or None) + try: + return getattr(functions, func)(*_args, alias=alias or None) + except AttributeError: + # Fall back for functions not present in `SqlFunctions`` + return Function(func, *_args, alias=alias or None) def function_objects_from_string(self, fields): fields = list(map(lambda str: str.strip(), COMMA_PATTERN.split(fields))) @@ -420,7 +421,7 @@ class Engine: for field in fields: field = field.casefold() if isinstance(field, str) else field if not issubclass(type(field), Criterion): - if any([func in field and f"{func}(" in field for func in SQL_FUNCTIONS]): + if any([f"{func}(" in field for func in SQL_FUNCTIONS]) or "(" in field: functions.append(field) return [self.get_function_object(function) for function in functions] From 572d9cc6967647aabc6ac8894964f504c689866d Mon Sep 17 00:00:00 2001 From: Suraj Shetty <13928957+surajshetty3416@users.noreply.github.com> Date: Wed, 13 Jul 2022 08:54:52 +0530 Subject: [PATCH 39/80] fix: Return from function after clearing interval --- frappe/email/doctype/newsletter/newsletter.js | 1 + 1 file changed, 1 insertion(+) diff --git a/frappe/email/doctype/newsletter/newsletter.js b/frappe/email/doctype/newsletter/newsletter.js index d2e5a3c047..3c52e61cbb 100644 --- a/frappe/email/doctype/newsletter/newsletter.js +++ b/frappe/email/doctype/newsletter/newsletter.js @@ -146,6 +146,7 @@ frappe.ui.form.on('Newsletter', { if (stats.sent + stats.error >= frm.doc.total_recipients || (!stats.total && !stats.emails_queued)) { frm.sending_status && clearInterval(frm.sending_status); frm.sending_status = null; + return; } } From 3fdd89a737f178391b2cb6262b71c1a4c1d83f32 Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Wed, 13 Jul 2022 11:02:56 +0530 Subject: [PATCH 40/80] refactor!: remove old weekly cleanup for route history (#17493) This is now configurable with log settings. This also fixes circular import issue that occurs when restoring backup --- frappe/database/query.py | 3 +- .../doctype/route_history/route_history.py | 33 ++----------------- frappe/hooks.py | 1 - 3 files changed, 5 insertions(+), 32 deletions(-) diff --git a/frappe/database/query.py b/frappe/database/query.py index c87117466b..38da501645 100644 --- a/frappe/database/query.py +++ b/frappe/database/query.py @@ -5,7 +5,6 @@ from typing import Any, Callable import frappe from frappe import _ -from frappe.boot import get_additional_filters_from_hooks from frappe.model.db_query import get_timespan_date_range from frappe.query_builder import Criterion, Field, Order, Table @@ -173,6 +172,8 @@ class Query: @cached_property def OPERATOR_MAP(self): + from frappe.boot import get_additional_filters_from_hooks + # default operators all_operators = OPERATOR_MAP.copy() diff --git a/frappe/desk/doctype/route_history/route_history.py b/frappe/desk/doctype/route_history/route_history.py index c62311ae02..a576ac73f5 100644 --- a/frappe/desk/doctype/route_history/route_history.py +++ b/frappe/desk/doctype/route_history/route_history.py @@ -4,45 +4,18 @@ import frappe from frappe.deferred_insert import deferred_insert as _deferred_insert from frappe.model.document import Document -from frappe.query_builder import DocType, Interval -from frappe.query_builder.functions import Count, Now class RouteHistory(Document): @staticmethod def clear_old_logs(days=30): + from frappe.query_builder import Interval + from frappe.query_builder.functions import Now + table = frappe.qb.DocType("Route History") frappe.db.delete(table, filters=(table.modified < (Now() - Interval(days=days)))) -def flush_old_route_records(): - """Deletes all route records except last 500 records per user""" - records_to_keep_limit = 500 - RouteHistory = DocType("Route History") - - users = ( - frappe.qb.from_(RouteHistory) - .select(RouteHistory.user) - .groupby(RouteHistory.user) - .having(Count(RouteHistory.name) > records_to_keep_limit) - ).run(pluck=True) - - for user in users: - last_record_to_keep = frappe.get_all( - "Route History", - filters={"user": user}, - limit_start=500, - fields=["modified"], - order_by="modified desc", - limit=1, - ) - - frappe.db.delete( - "Route History", - {"modified": ("<=", last_record_to_keep[0].modified), "user": user}, - ) - - @frappe.whitelist() def deferred_insert(routes): routes = [ diff --git a/frappe/hooks.py b/frappe/hooks.py index 54d068fa9d..a337d8e0d3 100644 --- a/frappe/hooks.py +++ b/frappe/hooks.py @@ -242,7 +242,6 @@ scheduler_events = { "weekly_long": [ "frappe.integrations.doctype.dropbox_settings.dropbox_settings.take_backups_weekly", "frappe.integrations.doctype.s3_backup_settings.s3_backup_settings.take_backups_weekly", - "frappe.desk.doctype.route_history.route_history.flush_old_route_records", "frappe.desk.form.document_follow.send_weekly_updates", "frappe.social.doctype.energy_point_log.energy_point_log.send_weekly_summary", "frappe.integrations.doctype.google_drive.google_drive.weekly_backup", From 26dd606831519d080c41f87afe43408fedba53e3 Mon Sep 17 00:00:00 2001 From: phot0n Date: Thu, 19 May 2022 18:46:30 +0530 Subject: [PATCH 41/80] refactor: GoogleOAuth * refactor: single callback method for google oauth --- .../doctype/email_account/email_account.json | 3 +- .../google_calendar/google_calendar.py | 8 +- .../google_contacts/google_contacts.js | 7 +- .../google_contacts/google_contacts.py | 121 +++----------- .../doctype/google_drive/google_drive.js | 7 +- .../doctype/google_drive/google_drive.py | 115 +++---------- .../google_settings/google_settings.py | 4 - frappe/integrations/google_oauth.py | 158 ++++++++++++++++++ .../website_settings/google_indexing.py | 95 +++-------- .../website_settings/website_settings.js | 8 +- .../website_settings/website_settings.py | 33 +--- 11 files changed, 247 insertions(+), 312 deletions(-) create mode 100644 frappe/integrations/google_oauth.py diff --git a/frappe/email/doctype/email_account/email_account.json b/frappe/email/doctype/email_account/email_account.json index 65053bab3d..1e3e995669 100644 --- a/frappe/email/doctype/email_account/email_account.json +++ b/frappe/email/doctype/email_account/email_account.json @@ -603,5 +603,6 @@ ], "sort_field": "modified", "sort_order": "DESC", + "states": [], "track_changes": 1 -} +} \ No newline at end of file diff --git a/frappe/integrations/doctype/google_calendar/google_calendar.py b/frappe/integrations/doctype/google_calendar/google_calendar.py index d8dc7fab1d..09ed012454 100644 --- a/frappe/integrations/doctype/google_calendar/google_calendar.py +++ b/frappe/integrations/doctype/google_calendar/google_calendar.py @@ -13,7 +13,7 @@ from googleapiclient.errors import HttpError import frappe from frappe import _ -from frappe.integrations.doctype.google_settings.google_settings import get_auth_url +from frappe.integrations.google_oauth import GoogleOAuth from frappe.model.document import Document from frappe.utils import ( add_days, @@ -90,7 +90,7 @@ class GoogleCalendar(Document): } try: - r = requests.post(get_auth_url(), data=data).json() + r = requests.post(GoogleOAuth.OAUTH_URL, data=data).json() except requests.exceptions.HTTPError: button_label = frappe.bold(_("Allow Google Calendar Access")) frappe.throw( @@ -130,7 +130,7 @@ def authorize_access(g_calendar, reauthorize=None): "redirect_uri": redirect_uri, "grant_type": "authorization_code", } - r = requests.post(get_auth_url(), data=data).json() + r = requests.post(GoogleOAuth.OAUTH_URL, data=data).json() if "refresh_token" in r: frappe.db.set_value( @@ -191,7 +191,7 @@ def get_google_calendar_object(g_calendar): credentials_dict = { "token": account.get_access_token(), "refresh_token": account.get_password(fieldname="refresh_token", raise_exception=False), - "token_uri": get_auth_url(), + "token_uri": GoogleOAuth.OAUTH_URL, "client_id": google_settings.client_id, "client_secret": google_settings.get_password(fieldname="client_secret", raise_exception=False), "scopes": "https://www.googleapis.com/auth/calendar/v3", diff --git a/frappe/integrations/doctype/google_contacts/google_contacts.js b/frappe/integrations/doctype/google_contacts/google_contacts.js index 7cbef46699..6e8035f38d 100644 --- a/frappe/integrations/doctype/google_contacts/google_contacts.js +++ b/frappe/integrations/doctype/google_contacts/google_contacts.js @@ -37,16 +37,11 @@ frappe.ui.form.on('Google Contacts', { } }, authorize_google_contacts_access: function(frm) { - let reauthorize = 0; - if(frm.doc.authorization_code) { - reauthorize = 1; - } - frappe.call({ method: "frappe.integrations.doctype.google_contacts.google_contacts.authorize_access", args: { "g_contact": frm.doc.name, - "reauthorize": reauthorize + "reauthorize": frm.doc.authorization_code ? 1 : 0 }, callback: function(r) { if(!r.exc) { diff --git a/frappe/integrations/doctype/google_contacts/google_contacts.py b/frappe/integrations/doctype/google_contacts/google_contacts.py index 5e4869be43..51ebcdd730 100644 --- a/frappe/integrations/doctype/google_contacts/google_contacts.py +++ b/frappe/integrations/doctype/google_contacts/google_contacts.py @@ -2,19 +2,14 @@ # License: MIT. See LICENSE -import google.oauth2.credentials -import requests -from googleapiclient.discovery import build from googleapiclient.errors import HttpError import frappe from frappe import _ -from frappe.integrations.doctype.google_settings.google_settings import get_auth_url +from frappe.integrations.google_oauth import GoogleOAuth from frappe.model.document import Document from frappe.utils import get_request_site_address -SCOPES = "https://www.googleapis.com/auth/contacts" - class GoogleContacts(Document): def validate(self): @@ -22,120 +17,56 @@ class GoogleContacts(Document): frappe.throw(_("Enable Google API in Google Settings.")) def get_access_token(self): - google_settings = frappe.get_doc("Google Settings") - - if not google_settings.enable: - frappe.throw(_("Google Contacts Integration is disabled.")) - if not self.refresh_token: button_label = frappe.bold(_("Allow Google Contacts Access")) raise frappe.ValidationError(_("Click on {0} to generate Refresh Token.").format(button_label)) - data = { - "client_id": google_settings.client_id, - "client_secret": google_settings.get_password(fieldname="client_secret", raise_exception=False), - "refresh_token": self.get_password(fieldname="refresh_token", raise_exception=False), - "grant_type": "refresh_token", - "scope": SCOPES, - } - - try: - r = requests.post(get_auth_url(), data=data).json() - except requests.exceptions.HTTPError: - button_label = frappe.bold(_("Allow Google Contacts Access")) - frappe.throw( - _( - "Something went wrong during the token generation. Click on {0} to generate a new one." - ).format(button_label) - ) + oauth_obj = GoogleOAuth("contacts") + r = oauth_obj.refresh_access_token( + self.get_password(fieldname="refresh_token", raise_exception=False) + ) return r.get("access_token") -@frappe.whitelist() -def authorize_access(g_contact, reauthorize=None): +@frappe.whitelist(methods=["POST", "GET"]) +def authorize_access(g_contact, reauthorize=False, code=None): """ If no Authorization code get it from Google and then request for Refresh Token. Google Contact Name is set to flags to set_value after Authorization Code is obtained. """ - google_settings = frappe.get_doc("Google Settings") - google_contact = frappe.get_doc("Google Contacts", g_contact) - - redirect_uri = ( - get_request_site_address(True) - + "?cmd=frappe.integrations.doctype.google_contacts.google_contacts.google_callback" + oauth_code = ( + frappe.db.get_value("Google Contacts", g_contact, "authorization_code") if not code else code ) + oauth_obj = GoogleOAuth("contacts") - if not google_contact.authorization_code or reauthorize: - frappe.cache().hset("google_contacts", "google_contact", google_contact.name) - return get_authentication_url(client_id=google_settings.client_id, redirect_uri=redirect_uri) - else: - try: - data = { - "code": google_contact.authorization_code, - "client_id": google_settings.client_id, - "client_secret": google_settings.get_password( - fieldname="client_secret", raise_exception=False - ), - "redirect_uri": redirect_uri, - "grant_type": "authorization_code", - } - r = requests.post(get_auth_url(), data=data).json() - - if "refresh_token" in r: - frappe.db.set_value( - "Google Contacts", google_contact.name, "refresh_token", r.get("refresh_token") - ) - frappe.db.commit() - - frappe.local.response["type"] = "redirect" - frappe.local.response["location"] = f"/app/Form/Google%20Contacts/{google_contact.name}" - - frappe.msgprint(_("Google Contacts has been configured.")) - except Exception as e: - frappe.throw(e) - - -def get_authentication_url(client_id=None, redirect_uri=None): - return { - "url": "https://accounts.google.com/o/oauth2/v2/auth?access_type=offline&response_type=code&prompt=consent&client_id={}&include_granted_scopes=true&scope={}&redirect_uri={}".format( - client_id, SCOPES, redirect_uri + if not oauth_code or reauthorize: + return oauth_obj.get_authentication_url( + get_request_site_address(True), + state={ + "method": "frappe.integrations.doctype.google_contacts.google_contacts.authorize_access", + "g_contact": g_contact, + "redirect": "/app/Form/Google%20Contacts/{}".format(g_contact), + }, ) - } - -@frappe.whitelist() -def google_callback(code=None): - """ - Authorization code is sent to callback as per the API configuration - """ - google_contact = frappe.cache().hget("google_contacts", "google_contact") - frappe.db.set_value("Google Contacts", google_contact, "authorization_code", code) - frappe.db.commit() - - authorize_access(google_contact) + frappe.db.set_value("Google Contacts", g_contact, "authorization_code", oauth_code) + r = oauth_obj.authorize(oauth_code, get_request_site_address(True)) + if r: + frappe.db.set_value("Google Contacts", g_contact, "refresh_token", r.get("refresh_token")) def get_google_contacts_object(g_contact): """ Returns an object of Google Calendar along with Google Calendar doc. """ - google_settings = frappe.get_doc("Google Settings") account = frappe.get_doc("Google Contacts", g_contact) + oauth_obj = GoogleOAuth("contacts") - credentials_dict = { - "token": account.get_access_token(), - "refresh_token": account.get_password(fieldname="refresh_token", raise_exception=False), - "token_uri": get_auth_url(), - "client_id": google_settings.client_id, - "client_secret": google_settings.get_password(fieldname="client_secret", raise_exception=False), - "scopes": "https://www.googleapis.com/auth/contacts", - } - - credentials = google.oauth2.credentials.Credentials(**credentials_dict) - google_contacts = build( - serviceName="people", version="v1", credentials=credentials, static_discovery=False + google_contacts = oauth_obj.get_google_service_object( + account.get_access_token(), + account.get_password(fieldname="indexing_refresh_token", raise_exception=False), ) return google_contacts, account diff --git a/frappe/integrations/doctype/google_drive/google_drive.js b/frappe/integrations/doctype/google_drive/google_drive.js index c314d02e7e..b38c0fb8e6 100644 --- a/frappe/integrations/doctype/google_drive/google_drive.js +++ b/frappe/integrations/doctype/google_drive/google_drive.js @@ -41,15 +41,10 @@ frappe.ui.form.on('Google Drive', { } }, authorize_google_drive_access: function(frm) { - let reauthorize = 0; - if (frm.doc.authorization_code) { - reauthorize = 1; - } - frappe.call({ method: "frappe.integrations.doctype.google_drive.google_drive.authorize_access", args: { - "reauthorize": reauthorize + "reauthorize": frm.doc.authorization_code ? 1 : 0 }, callback: function(r) { if (!r.exc) { diff --git a/frappe/integrations/doctype/google_drive/google_drive.py b/frappe/integrations/doctype/google_drive/google_drive.py index c472cbc741..b9416e6669 100644 --- a/frappe/integrations/doctype/google_drive/google_drive.py +++ b/frappe/integrations/doctype/google_drive/google_drive.py @@ -4,15 +4,12 @@ import os from urllib.parse import quote -import google.oauth2.credentials -import requests from apiclient.http import MediaFileUpload -from googleapiclient.discovery import build from googleapiclient.errors import HttpError import frappe from frappe import _ -from frappe.integrations.doctype.google_settings.google_settings import get_auth_url +from frappe.integrations.google_oauth import GoogleOAuth from frappe.integrations.offsite_backup_utils import ( get_latest_backup_file, send_email, @@ -23,8 +20,6 @@ from frappe.utils import get_backups_path, get_bench_path, get_request_site_addr from frappe.utils.background_jobs import enqueue from frappe.utils.backups import new_backup -SCOPES = "https://www.googleapis.com/auth/drive" - class GoogleDrive(Document): def validate(self): @@ -33,118 +28,56 @@ class GoogleDrive(Document): self.backup_folder_id = "" def get_access_token(self): - google_settings = frappe.get_doc("Google Settings") - - if not google_settings.enable: - frappe.throw(_("Google Integration is disabled.")) - if not self.refresh_token: button_label = frappe.bold(_("Allow Google Drive Access")) raise frappe.ValidationError(_("Click on {0} to generate Refresh Token.").format(button_label)) - data = { - "client_id": google_settings.client_id, - "client_secret": google_settings.get_password(fieldname="client_secret", raise_exception=False), - "refresh_token": self.get_password(fieldname="refresh_token", raise_exception=False), - "grant_type": "refresh_token", - "scope": SCOPES, - } - - try: - r = requests.post(get_auth_url(), data=data).json() - except requests.exceptions.HTTPError: - button_label = frappe.bold(_("Allow Google Drive Access")) - frappe.throw( - _( - "Something went wrong during the token generation. Click on {0} to generate a new one." - ).format(button_label) - ) + oauth_obj = GoogleOAuth("drive") + r = oauth_obj.refresh_access_token( + self.get_password(fieldname="refresh_token", raise_exception=False) + ) return r.get("access_token") -@frappe.whitelist() -def authorize_access(reauthorize=None): +@frappe.whitelist(methods=["POST", "GET"]) +def authorize_access(reauthorize=False, code=None): """ If no Authorization code get it from Google and then request for Refresh Token. Google Contact Name is set to flags to set_value after Authorization Code is obtained. """ - google_settings = frappe.get_doc("Google Settings") - google_drive = frappe.get_doc("Google Drive") - - redirect_uri = ( - get_request_site_address(True) - + "?cmd=frappe.integrations.doctype.google_drive.google_drive.google_callback" + oauth_code = ( + frappe.db.get_value("Google Drive", "Google Drive", "authorization_code") if not code else code ) + oauth_obj = GoogleOAuth("drive") - if not google_drive.authorization_code or reauthorize: + if not oauth_code or reauthorize: if reauthorize: frappe.db.set_value("Google Drive", None, "backup_folder_id", "") - return get_authentication_url(client_id=google_settings.client_id, redirect_uri=redirect_uri) - else: - try: - data = { - "code": google_drive.authorization_code, - "client_id": google_settings.client_id, - "client_secret": google_settings.get_password( - fieldname="client_secret", raise_exception=False - ), - "redirect_uri": redirect_uri, - "grant_type": "authorization_code", - } - r = requests.post(get_auth_url(), data=data).json() - - if "refresh_token" in r: - frappe.db.set_value("Google Drive", google_drive.name, "refresh_token", r.get("refresh_token")) - frappe.db.commit() - - frappe.local.response["type"] = "redirect" - frappe.local.response["location"] = "/app/Form/{}".format(quote("Google Drive")) - - frappe.msgprint(_("Google Drive has been configured.")) - except Exception as e: - frappe.throw(e) - - -def get_authentication_url(client_id, redirect_uri): - return { - "url": "https://accounts.google.com/o/oauth2/v2/auth?access_type=offline&response_type=code&prompt=consent&client_id={}&include_granted_scopes=true&scope={}&redirect_uri={}".format( - client_id, SCOPES, redirect_uri + return oauth_obj.get_authentication_url( + get_request_site_address(True), + state={ + "method": "frappe.integrations.doctype.google_drive.google_drive.authorize_access", + "redirect": "/app/Form/{0}".format(quote("Google Drive")), + }, ) - } - -@frappe.whitelist() -def google_callback(code=None): - """ - Authorization code is sent to callback as per the API configuration - """ - frappe.db.set_value("Google Drive", None, "authorization_code", code) - frappe.db.commit() - - authorize_access() + frappe.db.set_value("Google Drive", None, "authorization_code", oauth_code) + r = oauth_obj.authorize(oauth_code, get_request_site_address(True)) + frappe.db.set_value("Google Drive", "Google Drive", "refresh_token", r.get("refresh_token")) def get_google_drive_object(): """ Returns an object of Google Drive. """ - google_settings = frappe.get_doc("Google Settings") account = frappe.get_doc("Google Drive") + oauth_obj = GoogleOAuth("drive") - credentials_dict = { - "token": account.get_access_token(), - "refresh_token": account.get_password(fieldname="refresh_token", raise_exception=False), - "token_uri": get_auth_url(), - "client_id": google_settings.client_id, - "client_secret": google_settings.get_password(fieldname="client_secret", raise_exception=False), - "scopes": "https://www.googleapis.com/auth/drive/v3", - } - - credentials = google.oauth2.credentials.Credentials(**credentials_dict) - google_drive = build( - serviceName="drive", version="v3", credentials=credentials, static_discovery=False + google_drive = oauth_obj.get_google_service_object( + account.get_access_token(), + account.get_password(fieldname="indexing_refresh_token", raise_exception=False), ) return google_drive, account diff --git a/frappe/integrations/doctype/google_settings/google_settings.py b/frappe/integrations/doctype/google_settings/google_settings.py index c70a4b531f..e464e0d090 100644 --- a/frappe/integrations/doctype/google_settings/google_settings.py +++ b/frappe/integrations/doctype/google_settings/google_settings.py @@ -9,10 +9,6 @@ class GoogleSettings(Document): pass -def get_auth_url(): - return "https://www.googleapis.com/oauth2/v4/token" - - @frappe.whitelist() def get_file_picker_settings(): """Return all the data FileUploader needs to start the Google Drive Picker.""" diff --git a/frappe/integrations/google_oauth.py b/frappe/integrations/google_oauth.py new file mode 100644 index 0000000000..f8731a1e6a --- /dev/null +++ b/frappe/integrations/google_oauth.py @@ -0,0 +1,158 @@ +import json +from typing import Dict, Union + +from google.oauth2.credentials import Credentials +from googleapiclient.discovery import build +from requests import post, get + +import frappe + +CALLBACK_METHOD = "/api/method/frappe.integrations.google_oauth.callback" +_SCOPES = { + "mail": ("https://mail.google.com/"), + "contacts": ("https://www.googleapis.com/auth/contacts"), + "drive": ("https://www.googleapis.com/auth/drive"), + "indexing": ("https://www.googleapis.com/auth/indexing") +} +_SERVICES = { + "contacts": ("people", "v1"), + "drive": ("drive", "v3"), + "indexing": ("indexing", "v3") +} + + +class GoogleOAuth: + OAUTH_URL = "https://oauth2.googleapis.com/token" + + def __init__(self, domain: str, validate: bool=True): + self.google_settings = frappe.get_single("Google Settings") + self.domain = domain.lower() + self.scopes = " ".join(_SCOPES[self.domain]) if isinstance(_SCOPES[self.domain], (list, tuple)) else _SCOPES[self.domain] + + if validate: + self.validate_google_settings() + + def validate_google_settings(self): + if not self.google_settings.enable: + frappe.throw(frappe._("Please enable Google Settings before continuing.")) + + if not (self.google_settings.client_id and self.google_settings.client_secret): + frappe.throw(frappe._("Please update Google Settings before continuing.")) + + def authorize(self, oauth_code: str, site_address: str) -> Dict[str, Union[str, int]]: + data = { + "code": oauth_code, + "client_id": self.google_settings.client_id, + "client_secret": self.google_settings.get_password( + fieldname="client_secret", raise_exception=False + ), + "grant_type": "authorization_code", + "scope": self.scopes, + "redirect_uri": site_address + CALLBACK_METHOD, + } + + return handle_response( + post(self.OAUTH_URL, data=data).json(), + "Google Oauth Authorization Error", + "Something went wrong during the authorization.", + ) + + def refresh_access_token(self, refresh_token: str) -> Dict[str, Union[str, int]]: + data = { + "client_id": self.google_settings.client_id, + "client_secret": self.google_settings.get_password( + fieldname="client_secret", raise_exception=False + ), + "refresh_token": refresh_token, + "grant_type": "refresh_token", + "scope": self.scopes, + } + + return handle_response( + post(self.OAUTH_URL, data=data).json(), + "Google Oauth Access Token Refresh Error", + "Something went wrong during the access token generation.", + raise_err=True, + ) + + def get_authentication_url( + self, site_address: str, state: Dict[str, str] = None + ) -> Dict[str, str]: + """Return authentication url with the client id and redirect uri.""" + + state = json.dumps(state) + callback_url = site_address + CALLBACK_METHOD + + return { + "url": "https://accounts.google.com/o/oauth2/v2/auth?" + + "access_type=offline&response_type=code&prompt=consent&include_granted_scopes=true&" + + "client_id={0}&scope={1}&redirect_uri={2}&state={3}".format( + self.google_settings.client_id, self.scopes, callback_url, state + ) + } + + def get_google_service_object(self, access_token: str, refresh_token: str): + credentials_dict = { + "token": access_token, + "refresh_token": refresh_token, + "token_uri": self.OAUTH_URL, + "client_id": self.google_settings.client_id, + "client_secret": self.google_settings.get_password( + fieldname="client_secret", raise_exception=False + ), + "scopes": self.scopes, + } + + return build( + serviceName=_SERVICES[self.domain][0], + version=_SERVICES[self.domain][1], + credentials=Credentials(**credentials_dict), + static_discovery=False, + ) + + +def handle_response( + response: Dict[str, Union[str, int]], + error_title: str, + error_message: str, + raise_err: bool = False, +): + if "error" in response: + frappe.log_error( + frappe._(error_title), frappe._(response.get("error_description", error_message)) + ) + + if raise_err: + frappe.throw(frappe._(error_title), frappe._(error_message)) + + return {} + + return response + + +def is_valid_access_token(access_token: str) -> bool: + response = get( + "https://oauth2.googleapis.com/tokeninfo", + params={'access_token': access_token} + ).json() + + if "error" in response: + return False + + return True + + +@frappe.whitelist(methods=["GET"]) +def callback(state: str, code: str = None, error: str = None) -> None: + state = json.loads(state) + redirect = state.pop("redirect", "/app") + + if not error: + state.update({"code": code}) + frappe.get_attr(state.pop("method"))(**state) + + # GET request, hence using commit to persist changes + frappe.db.commit() + + frappe.local.response["type"] = "redirect" + frappe.local.response["location"] = redirect diff --git a/frappe/website/doctype/website_settings/google_indexing.py b/frappe/website/doctype/website_settings/google_indexing.py index d0657a1928..b89918dff2 100644 --- a/frappe/website/doctype/website_settings/google_indexing.py +++ b/frappe/website/doctype/website_settings/google_indexing.py @@ -4,99 +4,52 @@ from urllib.parse import quote -import google.oauth2.credentials -import requests -from googleapiclient.discovery import build from googleapiclient.errors import HttpError import frappe from frappe import _ -from frappe.integrations.doctype.google_settings.google_settings import get_auth_url +from frappe.integrations.google_oauth import GoogleOAuth from frappe.utils import get_request_site_address -SCOPES = "https://www.googleapis.com/auth/indexing" - -@frappe.whitelist() -def authorize_access(reauthorize=None): +@frappe.whitelist(methods=["POST", "GET"]) +def authorize_access(reauthorize=False, code=None): """If no Authorization code get it from Google and then request for Refresh Token.""" - google_settings = frappe.get_doc("Google Settings") - website_settings = frappe.get_doc("Website Settings") - - redirect_uri = ( - get_request_site_address(True) - + "?cmd=frappe.website.doctype.website_settings.google_indexing.google_callback" + oauth_code = ( + frappe.db.get_value("Website Settings", "Website Settings", "indexing_authorization_code") + if not code + else code ) - if not website_settings.indexing_authorization_code or reauthorize: - return get_authentication_url(client_id=google_settings.client_id, redirect_uri=redirect_uri) - else: - try: - data = { - "code": website_settings.indexing_authorization_code, - "client_id": google_settings.client_id, - "client_secret": google_settings.get_password( - fieldname="client_secret", raise_exception=False - ), - "redirect_uri": redirect_uri, - "grant_type": "authorization_code", - } - res = requests.post(get_auth_url(), data=data).json() + oauth_obj = GoogleOAuth("indexing") - if "refresh_token" in res: - frappe.db.set_value( - "Website Settings", website_settings.name, "indexing_refresh_token", res.get("refresh_token") - ) - frappe.db.commit() - - frappe.local.response["type"] = "redirect" - frappe.local.response["location"] = "/app/Form/{}".format(quote("Website Settings")) - - frappe.msgprint(_("Google Indexing has been configured.")) - except Exception as e: - frappe.throw(e) - - -def get_authentication_url(client_id, redirect_uri): - """Return authentication url with the client id and redirect uri.""" - return { - "url": "https://accounts.google.com/o/oauth2/v2/auth?access_type=offline&response_type=code&prompt=consent&client_id={}&include_granted_scopes=true&scope={}&redirect_uri={}".format( - client_id, SCOPES, redirect_uri + if not oauth_code or reauthorize: + return oauth_obj.get_authentication_url( + get_request_site_address(True), + state={ + "method": "frappe.website.doctype.website_settings.google_indexing.authorize_access", + "redirect": "/app/Form/{0}".format(quote("Website Settings")), + }, ) - } - -@frappe.whitelist() -def google_callback(code=None): - """Authorization code is sent to callback as per the API configuration.""" - frappe.db.set_value("Website Settings", None, "indexing_authorization_code", code) - frappe.db.commit() - - authorize_access() + frappe.db.set_value("Website Settings", None, "indexing_authorization_code", oauth_code) + res = oauth_obj.authorize(oauth_code, get_request_site_address(True)) + frappe.db.set_value( + "Website Settings", "Website Settings", "indexing_refresh_token", res.get("refresh_token") + ) def get_google_indexing_object(): """Returns an object of Google Indexing object.""" - google_settings = frappe.get_doc("Google Settings") account = frappe.get_doc("Website Settings") + oauth_obj = GoogleOAuth("indexing") - credentials_dict = { - "token": account.get_access_token(), - "refresh_token": account.get_password(fieldname="indexing_refresh_token", raise_exception=False), - "token_uri": get_auth_url(), - "client_id": google_settings.client_id, - "client_secret": google_settings.get_password(fieldname="client_secret", raise_exception=False), - "scopes": "https://www.googleapis.com/auth/indexing", - } - - credentials = google.oauth2.credentials.Credentials(**credentials_dict) - google_indexing = build( - serviceName="indexing", version="v3", credentials=credentials, static_discovery=False + return oauth_obj.get_google_service_object( + account.get_access_token(), + account.get_password(fieldname="indexing_refresh_token", raise_exception=False), ) - return google_indexing - def publish_site(url, operation_type="URL_UPDATED"): """Send an update/remove url request.""" diff --git a/frappe/website/doctype/website_settings/website_settings.js b/frappe/website/doctype/website_settings/website_settings.js index 2f15b4c00e..17ff4bed6e 100644 --- a/frappe/website/doctype/website_settings/website_settings.js +++ b/frappe/website/doctype/website_settings/website_settings.js @@ -42,16 +42,10 @@ frappe.ui.form.on('Website Settings', { }, authorize_api_indexing_access: function(frm) { - let reauthorize = 0; - if (frm.doc.authorization_code) { - reauthorize = 1; - } - frappe.call({ method: "frappe.website.doctype.website_settings.google_indexing.authorize_access", args: { - "g_indexing": frm.doc.name, - "reauthorize": reauthorize + "reauthorize": frm.doc.indexing_authorization_code ? 1 : 0 }, callback: function(r) { if (!r.exc) { diff --git a/frappe/website/doctype/website_settings/website_settings.py b/frappe/website/doctype/website_settings/website_settings.py index 4088be88c2..31e452c03f 100644 --- a/frappe/website/doctype/website_settings/website_settings.py +++ b/frappe/website/doctype/website_settings/website_settings.py @@ -4,12 +4,10 @@ from urllib.parse import quote import frappe from frappe import _ -from frappe.integrations.doctype.google_settings.google_settings import get_auth_url +from frappe.integrations.google_oauth import GoogleOAuth from frappe.model.document import Document from frappe.utils import encode, get_request_site_address -INDEXING_SCOPES = "https://www.googleapis.com/auth/indexing" - class WebsiteSettings(Document): def validate(self): @@ -89,34 +87,15 @@ class WebsiteSettings(Document): frappe.clear_cache() def get_access_token(self): - import requests - - google_settings = frappe.get_doc("Google Settings") - - if not google_settings.enable: - frappe.throw(_("Google Integration is disabled.")) - if not self.indexing_refresh_token: button_label = frappe.bold(_("Allow API Indexing Access")) raise frappe.ValidationError(_("Click on {0} to generate Refresh Token.").format(button_label)) - data = { - "client_id": google_settings.client_id, - "client_secret": google_settings.get_password(fieldname="client_secret", raise_exception=False), - "refresh_token": self.get_password(fieldname="indexing_refresh_token", raise_exception=False), - "grant_type": "refresh_token", - "scope": INDEXING_SCOPES, - } - - try: - res = requests.post(get_auth_url(), data=data).json() - except requests.exceptions.HTTPError: - button_label = frappe.bold(_("Allow Google Indexing Access")) - frappe.throw( - _( - "Something went wrong during the token generation. Click on {0} to generate a new one." - ).format(button_label) - ) + oauth_obj = GoogleOAuth("indexing") + res = oauth_obj.refresh_access_token( + self.get_password(fieldname="indexing_refresh_token", raise_exception=False) + ) + print(res) return res.get("access_token") From 07a577af867fe7c4c2486106a7e559b5e9c9717a Mon Sep 17 00:00:00 2001 From: phot0n Date: Fri, 27 May 2022 11:56:34 +0530 Subject: [PATCH 42/80] feat: google oauth for google emails * used unique constraint on email_id in Email Account Doctype --- frappe/core/doctype/communication/mixins.py | 4 +- frappe/core/doctype/user/user.py | 10 +-- .../doctype/email_account/email_account.js | 17 +++++ .../doctype/email_account/email_account.json | 36 ++++++++- .../doctype/email_account/email_account.py | 74 ++++++++++++++----- frappe/email/receive.py | 27 ++++++- frappe/email/smtp.py | 31 +++++++- frappe/email/utils.py | 58 +++++++++++++++ frappe/integrations/google_oauth.py | 21 ++++-- .../website_settings/website_settings.py | 1 - 10 files changed, 233 insertions(+), 46 deletions(-) diff --git a/frappe/core/doctype/communication/mixins.py b/frappe/core/doctype/communication/mixins.py index 695b8bebae..ad74b47026 100644 --- a/frappe/core/doctype/communication/mixins.py +++ b/frappe/core/doctype/communication/mixins.py @@ -92,7 +92,7 @@ class CommunicationEmailMixin: cc_list = self.mail_cc( is_inbound_mail_communcation=is_inbound_mail_communcation, include_sender=include_sender ) - return [self.get_email_with_displayname(email) for email in cc_list] + return [self.get_email_with_displayname(email) for email in cc_list if email] def mail_bcc(self, is_inbound_mail_communcation=False): """ @@ -120,7 +120,7 @@ class CommunicationEmailMixin: def get_mail_bcc_with_displayname(self, is_inbound_mail_communcation=False): bcc_list = self.mail_bcc(is_inbound_mail_communcation=is_inbound_mail_communcation) - return [self.get_email_with_displayname(email) for email in bcc_list] + return [self.get_email_with_displayname(email) for email in bcc_list if email] def mail_sender(self): email_account = self.get_outgoing_email_account() diff --git a/frappe/core/doctype/user/user.py b/frappe/core/doctype/user/user.py index d6fe883fae..eb3c11a4db 100644 --- a/frappe/core/doctype/user/user.py +++ b/frappe/core/doctype/user/user.py @@ -763,19 +763,11 @@ def has_email_account(email): @frappe.whitelist(allow_guest=False) def get_email_awaiting(user): - waiting = frappe.get_all( + return frappe.get_all( "User Email", fields=["email_account", "email_id"], filters={"awaiting_password": 1, "parent": user}, ) - if waiting: - return waiting - else: - user_email_table = DocType("User Email") - frappe.qb.update(user_email_table).set(user_email_table.user_email_table, 0).where( - user_email_table.parent == user - ).run() - return False def ask_pass_update(): diff --git a/frappe/email/doctype/email_account/email_account.js b/frappe/email/doctype/email_account/email_account.js index a357126a48..af6947c07c 100644 --- a/frappe/email/doctype/email_account/email_account.js +++ b/frappe/email/doctype/email_account/email_account.js @@ -1,6 +1,7 @@ frappe.email_defaults = { "GMail": { "email_server": "imap.gmail.com", + "incoming_port": 993, "use_ssl": 1, "enable_outgoing": 1, "smtp_server": "smtp.gmail.com", @@ -142,6 +143,22 @@ frappe.ui.form.on("Email Account", { } }, + authorize_google_api_access: function(frm) { + frappe.call({ + method: "frappe.email.doctype.email_account.email_account.authorize_google_access", + args: { + "email_account": frm.doc.name, + "reauthorize": frm.doc.refresh_token ? 1 : 0 + }, + callback: function(r) { + if (!r.exc) { + frm.save(); + window.open(r.message.url); + } + } + }); + }, + email_id:function(frm) { //pull domain and if no matching domain go create one frm.events.update_domain(frm); diff --git a/frappe/email/doctype/email_account/email_account.json b/frappe/email/doctype/email_account/email_account.json index 1e3e995669..34572c600d 100644 --- a/frappe/email/doctype/email_account/email_account.json +++ b/frappe/email/doctype/email_account/email_account.json @@ -18,6 +18,10 @@ "awaiting_password", "ascii_encode_password", "column_break_10", + "use_google_oauth", + "authorize_google_api_access", + "google_refresh_token", + "google_access_token", "login_id_is_different", "login_id", "mailbox_settings", @@ -79,7 +83,8 @@ "in_list_view": 1, "label": "Email Address", "options": "Email", - "reqd": 1 + "reqd": 1, + "unique": 1 }, { "default": "0", @@ -576,12 +581,39 @@ "fieldname": "section_break_25", "fieldtype": "Section Break", "label": "IMAP Details" + }, + { + "default": "0", + "depends_on": "eval: doc.service === \"GMail\"", + "fieldname": "use_google_oauth", + "fieldtype": "Check", + "label": "Use Google OAuth" + }, + { + "depends_on": "eval: doc.service === \"GMail\" && doc.use_google_oauth", + "fieldname": "authorize_google_api_access", + "fieldtype": "Button", + "label": "Authorize API Access" + }, + { + "fieldname": "google_refresh_token", + "fieldtype": "Data", + "hidden": 1, + "label": "Google Refresh Token", + "read_only": 1 + }, + { + "fieldname": "google_access_token", + "fieldtype": "Small Text", + "hidden": 1, + "label": "Google Access Token", + "read_only": 1 } ], "icon": "fa fa-inbox", "index_web_pages_for_search": 1, "links": [], - "modified": "2021-11-30 09:03:25.728637", + "modified": "2022-05-29 18:11:06.463553", "modified_by": "Administrator", "module": "Email", "name": "Email Account", diff --git a/frappe/email/doctype/email_account/email_account.py b/frappe/email/doctype/email_account/email_account.py index 02afe4f4b5..f73aa02183 100755 --- a/frappe/email/doctype/email_account/email_account.py +++ b/frappe/email/doctype/email_account/email_account.py @@ -8,6 +8,7 @@ import socket import time from datetime import datetime, timedelta from poplib import error_proto +from urllib.parse import quote import frappe from frappe import _, are_emails_muted, safe_encode @@ -15,8 +16,16 @@ from frappe.desk.form import assign_to from frappe.email.receive import EmailServer, InboundMail, SentEmailInInboxError from frappe.email.smtp import SMTPServer from frappe.email.utils import get_port +from frappe.integrations.google_oauth import GoogleOAuth from frappe.model.document import Document -from frappe.utils import cint, comma_or, cstr, parse_addr, validate_email_address +from frappe.utils import ( + cint, + comma_or, + cstr, + get_request_site_address, + parse_addr, + validate_email_address, +) from frappe.utils.background_jobs import enqueue, get_jobs from frappe.utils.error import raise_error_on_no_output from frappe.utils.jinja import render_template @@ -63,6 +72,7 @@ class EmailAccount(Document): def validate(self): """Validate Email Address and check POP3/IMAP and SMTP connections is enabled.""" + if self.email_id: validate_email_address(self.email_id, True) @@ -76,25 +86,11 @@ class EmailAccount(Document): if self.enable_incoming and self.use_imap and len(self.imap_folder) <= 0: frappe.throw(_("You need to set one IMAP folder for {0}").format(frappe.bold(self.email_id))) - duplicate_email_account = frappe.get_all( - "Email Account", filters={"email_id": self.email_id, "name": ("!=", self.name)} - ) - if duplicate_email_account: - frappe.throw( - _("Email ID must be unique, Email Account already exists for {0}").format( - frappe.bold(self.email_id) - ) - ) - if frappe.local.flags.in_patch or frappe.local.flags.in_test: return - if ( - not self.awaiting_password - and not frappe.local.flags.in_install - and not frappe.local.flags.in_patch - ): - if self.password or self.smtp_server in ("127.0.0.1", "localhost"): + if not frappe.local.flags.in_install and (self.use_google_oauth or not self.awaiting_password): + if self.google_refresh_token or self.password or self.smtp_server in ("127.0.0.1", "localhost"): if self.enable_incoming: self.get_incoming_server() self.no_failed = 0 @@ -103,7 +99,10 @@ class EmailAccount(Document): self.validate_smtp_conn() else: if self.enable_incoming or (self.enable_outgoing and not self.no_smtp_authentication): - frappe.throw(_("Password is required or select Awaiting Password")) + if self.use_google_oauth: + frappe.throw(_("Please Authorize Google by using `Authorize API access` button")) + else: + frappe.throw(_("Password is required or select Awaiting Password")) if self.notify_if_unreplied: if not self.send_notification_to: @@ -208,6 +207,9 @@ class EmailAccount(Document): "email_sync_rule": email_sync_rule, "incoming_port": get_port(self), "initial_sync_count": self.initial_sync_count or 100, + "use_google_oauth": self.use_google_oauth or 0, + "google_refresh_token": getattr(self, "google_refresh_token", None), + "google_access_token": getattr(self, "google_access_token", None), } ) @@ -274,7 +276,9 @@ class EmailAccount(Document): @property def _password(self): - raise_exception = not (self.no_smtp_authentication or frappe.flags.in_test) + raise_exception = not ( + self.use_google_oauth or self.no_smtp_authentication or frappe.flags.in_test + ) return self.get_password(raise_exception=raise_exception) @property @@ -405,12 +409,16 @@ class EmailAccount(Document): def sendmail_config(self): return { + "email_account": self.name, "server": self.smtp_server, "port": cint(self.smtp_port), "login": getattr(self, "login_id", None) or self.email_id, "password": self._password, "use_ssl": cint(self.use_ssl_for_outgoing), "use_tls": cint(self.use_tls), + "use_google_oauth": self.use_google_oauth or 0, + "google_refresh_token": getattr(self, "google_refresh_token", None), + "google_access_token": getattr(self, "google_access_token", None), } def get_smtp_server(self): @@ -491,6 +499,7 @@ class EmailAccount(Document): def process_mail(messages, append_to=None): for index, message in enumerate(messages.get("latest_messages", [])): uid = messages["uid_list"][index] if messages.get("uid_list") else None + uid = uid.decode() if isinstance(uid, bytes) else uid seen_status = messages.get("seen_status", {}).get(uid) if self.email_sync_option != "UNSEEN" or seen_status != "SEEN": # only append the emails with status != 'SEEN' if sync option is set to 'UNSEEN' @@ -771,14 +780,18 @@ def notify_unreplied(): def pull(now=False): """Will be called via scheduler, pull emails from all enabled Email accounts.""" + if frappe.cache().get_value("workers:no-internet") == True: if test_internet(): frappe.cache().set_value("workers:no-internet", False) else: return + queued_jobs = get_jobs(site=frappe.local.site, key="job_name")[frappe.local.site] for email_account in frappe.get_list( - "Email Account", filters={"enable_incoming": 1, "awaiting_password": 0} + "Email Account", + filters={"enable_incoming": 1}, + or_filters={"awaiting_password": 0, "use_google_oauth": 1}, ): if now: pull_from_email_account(email_account.name) @@ -906,3 +919,24 @@ def set_email_password(email_account, user, password): return False return True + + +@frappe.whitelist(methods=["POST"]) +def authorize_google_access(email_account, reauthorize=False, code=None): + doctype = "Email Account" + refresh_token = frappe.db.get_value(doctype, email_account, "google_refresh_token") + oauth_obj = GoogleOAuth("mail") + + if not (refresh_token or code) or reauthorize: + return oauth_obj.get_authentication_url( + get_request_site_address(True), + state={ + "method": "frappe.email.doctype.email_account.email_account.authorize_google_access", + "redirect": "/app/Form/{0}/{1}".format(quote(doctype), quote(email_account)), + "email_account": email_account, + }, + ) + + res = oauth_obj.authorize(code, get_request_site_address(True)) + frappe.db.set_value(doctype, email_account, "google_refresh_token", res.get("refresh_token")) + frappe.db.set_value(doctype, email_account, "google_access_token", res.get("access_token")) diff --git a/frappe/email/receive.py b/frappe/email/receive.py index 93e1a68285..81ef7927c1 100644 --- a/frappe/email/receive.py +++ b/frappe/email/receive.py @@ -18,6 +18,7 @@ from email_reply_parser import EmailReplyParser import frappe from frappe import _, safe_decode, safe_encode from frappe.core.doctype.file import MaxFileSizeReachedError, get_random_filename +from frappe.email.utils import connect_google_oauth from frappe.utils import ( add_days, cint, @@ -98,7 +99,18 @@ class EmailServer: self.imap = Timed_IMAP4( self.settings.host, self.settings.incoming_port, timeout=frappe.conf.get("pop_timeout") ) - self.imap.login(self.settings.username, self.settings.password) + + if self.settings.use_google_oauth and self.settings.google_refresh_token: + connect_google_oauth( + self.imap, + self.settings.email_account, + self.settings.username, + self.settings.google_access_token, + self.settings.google_refresh_token, + ) + else: + self.imap.login(self.settings.username, self.settings.password) + # connection established! return True @@ -119,8 +131,17 @@ class EmailServer: self.settings.host, self.settings.incoming_port, timeout=frappe.conf.get("pop_timeout") ) - self.pop.user(self.settings.username) - self.pop.pass_(self.settings.password) + if self.settings.use_google_oauth and self.settings.google_refresh_token: + connect_google_oauth( + self.pop, + self.settings.email_account, + self.settings.username, + self.settings.google_access_token, + self.settings.google_refresh_token, + ) + else: + self.pop.user(self.settings.username) + self.pop.pass_(self.settings.password) # connection established! return True diff --git a/frappe/email/smtp.py b/frappe/email/smtp.py index 1211419de1..cd31aa0d5c 100644 --- a/frappe/email/smtp.py +++ b/frappe/email/smtp.py @@ -5,6 +5,7 @@ import smtplib import frappe from frappe import _ +from frappe.email.utils import connect_google_oauth from frappe.utils import cint, cstr @@ -43,13 +44,29 @@ def send(email, append_to=None, retry=1): class SMTPServer: - def __init__(self, server, login=None, password=None, port=None, use_tls=None, use_ssl=None): + def __init__( + self, + server, + login=None, + email_account=None, + password=None, + port=None, + use_tls=None, + use_ssl=None, + use_google_oauth=0, + google_refresh_token=None, + google_access_token=None, + ): self.login = login + self.email_account = email_account self.password = password self._server = server self._port = port self.use_tls = use_tls self.use_ssl = use_ssl + self.use_google_oauth = use_google_oauth + self.google_refresh_token = google_refresh_token + self.google_access_token = google_access_token self._session = None if not self.server: @@ -91,7 +108,17 @@ class SMTPServer: ) self.secure_session(_session) - if self.login and self.password: + + if self.use_google_oauth and self.google_refresh_token: + connect_google_oauth( + _session, + self.email_account, + self.login, + self.google_access_token, + self.google_refresh_token, + ) + + elif self.password: res = _session.login(str(self.login or ""), str(self.password or "")) # check if logged correctly diff --git a/frappe/email/utils.py b/frappe/email/utils.py index 147284a625..97cfaaa1e7 100644 --- a/frappe/email/utils.py +++ b/frappe/email/utils.py @@ -1,8 +1,13 @@ # Copyright (c) 2019, Frappe Technologies Pvt. Ltd. and contributors # License: MIT. See LICENSE +import base64 import imaplib import poplib +import smtplib +from typing import Union +from frappe import _, db, log_error, throw +from frappe.integrations.google_oauth import GoogleAuthenticationError, GoogleOAuth from frappe.utils import cint @@ -15,3 +20,56 @@ def get_port(doc): doc.incoming_port = poplib.POP3_SSL_PORT if doc.use_ssl else poplib.POP3_PORT return cint(doc.incoming_port) + + +def connect_google_oauth( + connection_obj: Union[imaplib.IMAP4, poplib.POP3, smtplib.SMTP], + email_account: str, + email: str, + google_access_token: str, + google_refresh_token: str, + retry: int = 0, +) -> None: + auth_string = "user=%s\1auth=Bearer %s\1\1" % (email, google_access_token) + mechanism = "XOAUTH2" + _func = lambda x: auth_string # noqa: E731 + + try: + if isinstance(connection_obj, poplib.POP3): + # poplib doesn't have AUTH command implementation + res = connection_obj._shortcmd( + "AUTH {0} {1}".format(mechanism, base64.b64encode(bytes(auth_string, "utf-8")).decode("utf-8")) + ) + + if not res.startswith(b"+OK"): + raise + + elif isinstance(connection_obj, imaplib.IMAP4): + connection_obj.authenticate(mechanism, _func) + + else: + # SMTP + connection_obj.auth(mechanism, _func, initial_response_ok=False) + + except Exception: + # maybe the access token expired - refreshing + access_token = refresh_google_access_token(email_account, google_refresh_token) + + if not access_token or retry > 0: + throw( + _("Google Authentication Failed. Please Check and Update the credentials."), + GoogleAuthenticationError, + _("Google Authentication Error"), + ) + + connect_google_oauth( + connection_obj, email_account, email, access_token, google_refresh_token, retry + 1 + ) + + +def refresh_google_access_token(email_account: str, google_refresh_token: str) -> str: + oauth_obj = GoogleOAuth("mail") + res = oauth_obj.refresh_access_token(google_refresh_token) + db.set_value("Email Account", email_account, "google_access_token", res.get("access_token")) + + return res.get("access_token") diff --git a/frappe/integrations/google_oauth.py b/frappe/integrations/google_oauth.py index f8731a1e6a..da67f2ccf5 100644 --- a/frappe/integrations/google_oauth.py +++ b/frappe/integrations/google_oauth.py @@ -3,7 +3,7 @@ from typing import Dict, Union from google.oauth2.credentials import Credentials from googleapiclient.discovery import build -from requests import post, get +from requests import get, post import frappe @@ -12,22 +12,30 @@ _SCOPES = { "mail": ("https://mail.google.com/"), "contacts": ("https://www.googleapis.com/auth/contacts"), "drive": ("https://www.googleapis.com/auth/drive"), - "indexing": ("https://www.googleapis.com/auth/indexing") + "indexing": ("https://www.googleapis.com/auth/indexing"), } _SERVICES = { "contacts": ("people", "v1"), "drive": ("drive", "v3"), - "indexing": ("indexing", "v3") + "indexing": ("indexing", "v3"), } +class GoogleAuthenticationError(Exception): + pass + + class GoogleOAuth: OAUTH_URL = "https://oauth2.googleapis.com/token" - def __init__(self, domain: str, validate: bool=True): + def __init__(self, domain: str, validate: bool = True): self.google_settings = frappe.get_single("Google Settings") self.domain = domain.lower() - self.scopes = " ".join(_SCOPES[self.domain]) if isinstance(_SCOPES[self.domain], (list, tuple)) else _SCOPES[self.domain] + self.scopes = ( + " ".join(_SCOPES[self.domain]) + if isinstance(_SCOPES[self.domain], (list, tuple)) + else _SCOPES[self.domain] + ) if validate: self.validate_google_settings() @@ -132,8 +140,7 @@ def handle_response( def is_valid_access_token(access_token: str) -> bool: response = get( - "https://oauth2.googleapis.com/tokeninfo", - params={'access_token': access_token} + "https://oauth2.googleapis.com/tokeninfo", params={"access_token": access_token} ).json() if "error" in response: diff --git a/frappe/website/doctype/website_settings/website_settings.py b/frappe/website/doctype/website_settings/website_settings.py index 31e452c03f..bd634b4f32 100644 --- a/frappe/website/doctype/website_settings/website_settings.py +++ b/frappe/website/doctype/website_settings/website_settings.py @@ -95,7 +95,6 @@ class WebsiteSettings(Document): res = oauth_obj.refresh_access_token( self.get_password(fieldname="indexing_refresh_token", raise_exception=False) ) - print(res) return res.get("access_token") From cc878377be039e6f92d3044088d8fb83613b8c01 Mon Sep 17 00:00:00 2001 From: phot0n Date: Mon, 30 May 2022 16:04:08 +0530 Subject: [PATCH 43/80] fix: allow adding new emails with google oauth checked kinda works in a similar way as awaiting_password for new emails --- frappe/email/doctype/email_account/email_account.js | 3 +-- frappe/email/doctype/email_account/email_account.json | 4 ++-- frappe/email/doctype/email_account/email_account.py | 10 +++++++++- 3 files changed, 12 insertions(+), 5 deletions(-) diff --git a/frappe/email/doctype/email_account/email_account.js b/frappe/email/doctype/email_account/email_account.js index af6947c07c..4392e1db1d 100644 --- a/frappe/email/doctype/email_account/email_account.js +++ b/frappe/email/doctype/email_account/email_account.js @@ -135,7 +135,7 @@ frappe.ui.form.on("Email Account", { show_gmail_message_for_less_secure_apps: function(frm) { frm.dashboard.clear_headline(); - let msg = __("GMail will only work if you enable 2-step authentication and use app-specific password."); + let msg = __("GMail will only work if you enable 2-step authentication and use app-specific password OR use Google OAuth."); let cta = __("Read the step by step guide here."); msg += ` ${cta}`; if (frm.doc.service==="GMail") { @@ -152,7 +152,6 @@ frappe.ui.form.on("Email Account", { }, callback: function(r) { if (!r.exc) { - frm.save(); window.open(r.message.url); } } diff --git a/frappe/email/doctype/email_account/email_account.json b/frappe/email/doctype/email_account/email_account.json index 34572c600d..0f62e103e8 100644 --- a/frappe/email/doctype/email_account/email_account.json +++ b/frappe/email/doctype/email_account/email_account.json @@ -590,7 +590,7 @@ "label": "Use Google OAuth" }, { - "depends_on": "eval: doc.service === \"GMail\" && doc.use_google_oauth", + "depends_on": "eval: doc.service === \"GMail\" && doc.use_google_oauth && !doc.__islocal", "fieldname": "authorize_google_api_access", "fieldtype": "Button", "label": "Authorize API Access" @@ -613,7 +613,7 @@ "icon": "fa fa-inbox", "index_web_pages_for_search": 1, "links": [], - "modified": "2022-05-29 18:11:06.463553", + "modified": "2022-05-30 15:45:05.282867", "modified_by": "Administrator", "module": "Email", "name": "Email Account", diff --git a/frappe/email/doctype/email_account/email_account.py b/frappe/email/doctype/email_account/email_account.py index f73aa02183..7779dea38c 100755 --- a/frappe/email/doctype/email_account/email_account.py +++ b/frappe/email/doctype/email_account/email_account.py @@ -100,7 +100,8 @@ class EmailAccount(Document): else: if self.enable_incoming or (self.enable_outgoing and not self.no_smtp_authentication): if self.use_google_oauth: - frappe.throw(_("Please Authorize Google by using `Authorize API access` button")) + if not self.is_new(): + frappe.throw(_("Please Authorize Google by using `Authorize API access` button")) else: frappe.throw(_("Password is required or select Awaiting Password")) @@ -156,6 +157,13 @@ class EmailAccount(Document): enable_outgoing=self.enable_outgoing, ) + def after_insert(self): + if self.use_google_oauth and not self.google_refresh_token: + frappe.msgprint( + _("Please Authorize Google by using `Authorize API access` button"), + indicator="orange" + ) + def there_must_be_only_one_default(self): """If current Email Account is default, un-default all other accounts.""" for field in ("default_incoming", "default_outgoing"): From b930cb923b91c80fd3b33538c15ae161f863b76c Mon Sep 17 00:00:00 2001 From: phot0n Date: Mon, 30 May 2022 17:25:08 +0530 Subject: [PATCH 44/80] fix: set X-Original-From header before replacing sender email/name This will help in knowing the original email id and sender of email and also bring consistency. --- frappe/email/email_body.py | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/frappe/email/email_body.py b/frappe/email/email_body.py index b493ca2cb5..20f81cb89b 100755 --- a/frappe/email/email_body.py +++ b/frappe/email/email_body.py @@ -267,6 +267,7 @@ class EMail: validate_email_address(strip(self.sender), True) self.reply_to = validate_email_address(strip(self.reply_to) or self.sender, True) + self.set_header("X-Original-From", self.sender) self.replace_sender() self.replace_sender_name() @@ -279,16 +280,14 @@ class EMail: def replace_sender(self): if cint(self.email_account.always_use_account_email_id_as_sender): - self.set_header("X-Original-From", self.sender) - sender_name, sender_email = parse_addr(self.sender) + sender_name, _ = parse_addr(self.sender) self.sender = email.utils.formataddr( (str(Header(sender_name or self.email_account.name, "utf-8")), self.email_account.email_id) ) def replace_sender_name(self): if cint(self.email_account.always_use_account_name_as_sender_name): - self.set_header("X-Original-From", self.sender) - sender_name, sender_email = parse_addr(self.sender) + _, sender_email = parse_addr(self.sender) self.sender = email.utils.formataddr( (str(Header(self.email_account.name, "utf-8")), sender_email) ) From ebc586121054f3762be66d3f072da92158c8bc9c Mon Sep 17 00:00:00 2001 From: phot0n Date: Tue, 31 May 2022 01:14:04 +0530 Subject: [PATCH 45/80] feat: generic OAuth for email --- .../doctype/email_account/email_account.js | 7 +- .../doctype/email_account/email_account.json | 24 ++--- .../doctype/email_account/email_account.py | 47 ++++---- frappe/email/oauth.py | 100 ++++++++++++++++++ frappe/email/receive.py | 26 +++-- frappe/email/smtp.py | 27 ++--- frappe/email/utils.py | 59 +---------- 7 files changed, 176 insertions(+), 114 deletions(-) create mode 100644 frappe/email/oauth.py diff --git a/frappe/email/doctype/email_account/email_account.js b/frappe/email/doctype/email_account/email_account.js index 4392e1db1d..4d84f69395 100644 --- a/frappe/email/doctype/email_account/email_account.js +++ b/frappe/email/doctype/email_account/email_account.js @@ -135,7 +135,7 @@ frappe.ui.form.on("Email Account", { show_gmail_message_for_less_secure_apps: function(frm) { frm.dashboard.clear_headline(); - let msg = __("GMail will only work if you enable 2-step authentication and use app-specific password OR use Google OAuth."); + let msg = __("GMail will only work if you enable 2-step authentication and use app-specific password OR use OAuth."); let cta = __("Read the step by step guide here."); msg += ` ${cta}`; if (frm.doc.service==="GMail") { @@ -143,11 +143,12 @@ frappe.ui.form.on("Email Account", { } }, - authorize_google_api_access: function(frm) { + authorize_api_access: function(frm) { frappe.call({ - method: "frappe.email.doctype.email_account.email_account.authorize_google_access", + method: "frappe.email.doctype.email_account.email_account.oauth_access", args: { "email_account": frm.doc.name, + "service": frm.doc.service, "reauthorize": frm.doc.refresh_token ? 1 : 0 }, callback: function(r) { diff --git a/frappe/email/doctype/email_account/email_account.json b/frappe/email/doctype/email_account/email_account.json index 0f62e103e8..72be4f2e42 100644 --- a/frappe/email/doctype/email_account/email_account.json +++ b/frappe/email/doctype/email_account/email_account.json @@ -18,10 +18,10 @@ "awaiting_password", "ascii_encode_password", "column_break_10", - "use_google_oauth", - "authorize_google_api_access", - "google_refresh_token", - "google_access_token", + "use_oauth", + "authorize_api_access", + "refresh_token", + "access_token", "login_id_is_different", "login_id", "mailbox_settings", @@ -585,28 +585,28 @@ { "default": "0", "depends_on": "eval: doc.service === \"GMail\"", - "fieldname": "use_google_oauth", + "fieldname": "use_oauth", "fieldtype": "Check", - "label": "Use Google OAuth" + "label": "Use OAuth" }, { - "depends_on": "eval: doc.service === \"GMail\" && doc.use_google_oauth && !doc.__islocal", - "fieldname": "authorize_google_api_access", + "depends_on": "eval: doc.service === \"GMail\" && doc.use_oauth && !doc.__islocal", + "fieldname": "authorize_api_access", "fieldtype": "Button", "label": "Authorize API Access" }, { - "fieldname": "google_refresh_token", + "fieldname": "refresh_token", "fieldtype": "Data", "hidden": 1, - "label": "Google Refresh Token", + "label": "Refresh Token", "read_only": 1 }, { - "fieldname": "google_access_token", + "fieldname": "access_token", "fieldtype": "Small Text", "hidden": 1, - "label": "Google Access Token", + "label": "Access Token", "read_only": 1 } ], diff --git a/frappe/email/doctype/email_account/email_account.py b/frappe/email/doctype/email_account/email_account.py index 7779dea38c..b2f8e1ed64 100755 --- a/frappe/email/doctype/email_account/email_account.py +++ b/frappe/email/doctype/email_account/email_account.py @@ -89,8 +89,11 @@ class EmailAccount(Document): if frappe.local.flags.in_patch or frappe.local.flags.in_test: return - if not frappe.local.flags.in_install and (self.use_google_oauth or not self.awaiting_password): - if self.google_refresh_token or self.password or self.smtp_server in ("127.0.0.1", "localhost"): + if getattr(self, "service", "") != "GMail" and self.use_oauth: + self.use_oauth = 0 + + if not frappe.local.flags.in_install and (self.use_oauth or not self.awaiting_password): + if self.refresh_token or self.password or self.smtp_server in ("127.0.0.1", "localhost"): if self.enable_incoming: self.get_incoming_server() self.no_failed = 0 @@ -99,9 +102,9 @@ class EmailAccount(Document): self.validate_smtp_conn() else: if self.enable_incoming or (self.enable_outgoing and not self.no_smtp_authentication): - if self.use_google_oauth: + if self.use_oauth: if not self.is_new(): - frappe.throw(_("Please Authorize Google by using `Authorize API access` button")) + frappe.throw(_("Please Enable OAuth by using `Authorize API access` button")) else: frappe.throw(_("Password is required or select Awaiting Password")) @@ -158,9 +161,9 @@ class EmailAccount(Document): ) def after_insert(self): - if self.use_google_oauth and not self.google_refresh_token: + if self.use_oauth and not self.refresh_token: frappe.msgprint( - _("Please Authorize Google by using `Authorize API access` button"), + _("Please Enable OAuth by using `Authorize API access` button"), indicator="orange" ) @@ -211,13 +214,14 @@ class EmailAccount(Document): "host": self.email_server, "use_ssl": self.use_ssl, "username": getattr(self, "login_id", None) or self.email_id, + "service": getattr(self, "service", None), "use_imap": self.use_imap, "email_sync_rule": email_sync_rule, "incoming_port": get_port(self), "initial_sync_count": self.initial_sync_count or 100, - "use_google_oauth": self.use_google_oauth or 0, - "google_refresh_token": getattr(self, "google_refresh_token", None), - "google_access_token": getattr(self, "google_access_token", None), + "use_oauth": self.use_oauth or 0, + "refresh_token": getattr(self, "refresh_token", None), + "access_token": getattr(self, "access_token", None), } ) @@ -285,7 +289,7 @@ class EmailAccount(Document): @property def _password(self): raise_exception = not ( - self.use_google_oauth or self.no_smtp_authentication or frappe.flags.in_test + self.use_oauth or self.no_smtp_authentication or frappe.flags.in_test ) return self.get_password(raise_exception=raise_exception) @@ -424,9 +428,10 @@ class EmailAccount(Document): "password": self._password, "use_ssl": cint(self.use_ssl_for_outgoing), "use_tls": cint(self.use_tls), - "use_google_oauth": self.use_google_oauth or 0, - "google_refresh_token": getattr(self, "google_refresh_token", None), - "google_access_token": getattr(self, "google_access_token", None), + "service": getattr(self, "service", None), + "use_oauth": self.use_oauth or 0, + "refresh_token": getattr(self, "refresh_token", None), + "access_token": getattr(self, "access_token", None), } def get_smtp_server(self): @@ -799,7 +804,7 @@ def pull(now=False): for email_account in frappe.get_list( "Email Account", filters={"enable_incoming": 1}, - or_filters={"awaiting_password": 0, "use_google_oauth": 1}, + or_filters={"awaiting_password": 0, "use_oauth": 1}, ): if now: pull_from_email_account(email_account.name) @@ -930,9 +935,15 @@ def set_email_password(email_account, user, password): @frappe.whitelist(methods=["POST"]) -def authorize_google_access(email_account, reauthorize=False, code=None): +def oauth_access(email_account: str, reauthorize: bool = False, service: str = None): doctype = "Email Account" - refresh_token = frappe.db.get_value(doctype, email_account, "google_refresh_token") + refresh_token = frappe.db.get_value(doctype, email_account, "refresh_token") + + if service == "GMail": + return authorize_google_access(email_account, reauthorize, refresh_token, doctype) + + +def authorize_google_access(email_account, reauthorize: bool = False, refresh_token: str = None, doctype: str = "Email Account", code: str = None): oauth_obj = GoogleOAuth("mail") if not (refresh_token or code) or reauthorize: @@ -946,5 +957,5 @@ def authorize_google_access(email_account, reauthorize=False, code=None): ) res = oauth_obj.authorize(code, get_request_site_address(True)) - frappe.db.set_value(doctype, email_account, "google_refresh_token", res.get("refresh_token")) - frappe.db.set_value(doctype, email_account, "google_access_token", res.get("access_token")) + frappe.db.set_value(doctype, email_account, "refresh_token", res.get("refresh_token")) + frappe.db.set_value(doctype, email_account, "access_token", res.get("access_token")) diff --git a/frappe/email/oauth.py b/frappe/email/oauth.py new file mode 100644 index 0000000000..b4efac2e07 --- /dev/null +++ b/frappe/email/oauth.py @@ -0,0 +1,100 @@ +import base64 +from typing import Callable, Union +from imaplib import IMAP4 +from poplib import POP3 +from smtplib import SMTP + +import frappe +from frappe.integrations.google_oauth import GoogleOAuth + + +class OAuthenticationError(Exception): + pass + + +class Oauth: + def __init__(self, + conn: Union[IMAP4, POP3, SMTP], + email_account: str, + email: str, + access_token: str, + refresh_token: str, + service: str, + mechanism: str = "XOAUTH2", + ) -> None: + + self.email_account = email_account + self.email = email + self.service = service + self._mechanism = mechanism + self._conn = conn + self._access_token = access_token + self._refresh_token = refresh_token + + self.validate_implementation() + + def validate_implementation(self) -> None: + if self.service != "GMail": + raise NotImplementedError(f"Service {self.service} currently doesn't have oauth implementation.") + + @property + def _auth_string(self) -> str: + return "user=%s\1auth=Bearer %s\1\1" % (self.email, self._access_token) + + def connect(self, _retry: int = 0) -> None: + try: + if isinstance(self._conn, POP3): + res = self._connect_pop() + + if not res.startswith(b"+OK"): + raise + + elif isinstance(self._conn, IMAP4): + self._connect_imap() + + else: + # SMTP + self._connect_smtp() + + except Exception: + # maybe the access token expired - refreshing + access_token = self._refresh_access_token() + print(self._auth_string) + + if not access_token or _retry > 0: + frappe.throw( + frappe._("Authentication Failed. Please Check and Update the credentials."), + OAuthenticationError, + frappe._("OAuth Error"), + ) + + self._access_token = access_token + self.connect(_retry + 1) + + def _connect_pop(self) -> bytes: + # poplib doesn't have AUTH command implementation + res = self._conn._shortcmd( + "AUTH {0} {1}".format(self._mechanism, base64.b64encode(bytes(self._auth_string, "utf-8")).decode("utf-8")) + ) + + return res + + def _connect_imap(self) -> None: + self._conn.authenticate(self._mechanism, lambda x: self._auth_string) + + def _connect_smtp(self) -> None: + self._conn.auth(self._mechanism, lambda x: self._auth_string, initial_response_ok=False) + + def _refresh_access_token(self) -> str: + service_obj = self._get_service_object() + access_token = service_obj.refresh_access_token(self._refresh_token).get("access_token", None) + + # set the new access token in db + frappe.db.set_value("Email Account", self.email_account, "access_token", access_token) + frappe.db.commit() + return access_token + + def _get_service_object(self): + return { + "GMail": GoogleOAuth("mail"), + }[self.service] diff --git a/frappe/email/receive.py b/frappe/email/receive.py index 81ef7927c1..074ec8e77d 100644 --- a/frappe/email/receive.py +++ b/frappe/email/receive.py @@ -18,7 +18,7 @@ from email_reply_parser import EmailReplyParser import frappe from frappe import _, safe_decode, safe_encode from frappe.core.doctype.file import MaxFileSizeReachedError, get_random_filename -from frappe.email.utils import connect_google_oauth +from frappe.email.oauth import Oauth from frappe.utils import ( add_days, cint, @@ -100,14 +100,16 @@ class EmailServer: self.settings.host, self.settings.incoming_port, timeout=frappe.conf.get("pop_timeout") ) - if self.settings.use_google_oauth and self.settings.google_refresh_token: - connect_google_oauth( + if self.settings.use_oauth and self.settings.refresh_token: + Oauth( self.imap, self.settings.email_account, self.settings.username, - self.settings.google_access_token, - self.settings.google_refresh_token, - ) + self.settings.access_token, + self.settings.refresh_token, + self.settings.service + ).connect() + else: self.imap.login(self.settings.username, self.settings.password) @@ -131,14 +133,16 @@ class EmailServer: self.settings.host, self.settings.incoming_port, timeout=frappe.conf.get("pop_timeout") ) - if self.settings.use_google_oauth and self.settings.google_refresh_token: - connect_google_oauth( + if self.settings.use_oauth and self.settings.refresh_token: + Oauth( self.pop, self.settings.email_account, self.settings.username, - self.settings.google_access_token, - self.settings.google_refresh_token, - ) + self.settings.access_token, + self.settings.refresh_token, + self.settings.service + ).connect() + else: self.pop.user(self.settings.username) self.pop.pass_(self.settings.password) diff --git a/frappe/email/smtp.py b/frappe/email/smtp.py index cd31aa0d5c..df88ddb986 100644 --- a/frappe/email/smtp.py +++ b/frappe/email/smtp.py @@ -5,7 +5,7 @@ import smtplib import frappe from frappe import _ -from frappe.email.utils import connect_google_oauth +from frappe.email.oauth import Oauth from frappe.utils import cint, cstr @@ -53,9 +53,10 @@ class SMTPServer: port=None, use_tls=None, use_ssl=None, - use_google_oauth=0, - google_refresh_token=None, - google_access_token=None, + use_oauth=0, + refresh_token=None, + access_token=None, + service=None ): self.login = login self.email_account = email_account @@ -64,9 +65,10 @@ class SMTPServer: self._port = port self.use_tls = use_tls self.use_ssl = use_ssl - self.use_google_oauth = use_google_oauth - self.google_refresh_token = google_refresh_token - self.google_access_token = google_access_token + self.use_oauth = use_oauth + self.refresh_token = refresh_token + self.access_token = access_token + self.service = service self._session = None if not self.server: @@ -109,14 +111,15 @@ class SMTPServer: self.secure_session(_session) - if self.use_google_oauth and self.google_refresh_token: - connect_google_oauth( + if self.use_oauth and self.refresh_token: + Oauth( _session, self.email_account, self.login, - self.google_access_token, - self.google_refresh_token, - ) + self.access_token, + self.refresh_token, + self.service + ).connect() elif self.password: res = _session.login(str(self.login or ""), str(self.password or "")) diff --git a/frappe/email/utils.py b/frappe/email/utils.py index 97cfaaa1e7..7fc2e0ff89 100644 --- a/frappe/email/utils.py +++ b/frappe/email/utils.py @@ -1,13 +1,9 @@ # Copyright (c) 2019, Frappe Technologies Pvt. Ltd. and contributors # License: MIT. See LICENSE -import base64 + import imaplib import poplib -import smtplib -from typing import Union -from frappe import _, db, log_error, throw -from frappe.integrations.google_oauth import GoogleAuthenticationError, GoogleOAuth from frappe.utils import cint @@ -20,56 +16,3 @@ def get_port(doc): doc.incoming_port = poplib.POP3_SSL_PORT if doc.use_ssl else poplib.POP3_PORT return cint(doc.incoming_port) - - -def connect_google_oauth( - connection_obj: Union[imaplib.IMAP4, poplib.POP3, smtplib.SMTP], - email_account: str, - email: str, - google_access_token: str, - google_refresh_token: str, - retry: int = 0, -) -> None: - auth_string = "user=%s\1auth=Bearer %s\1\1" % (email, google_access_token) - mechanism = "XOAUTH2" - _func = lambda x: auth_string # noqa: E731 - - try: - if isinstance(connection_obj, poplib.POP3): - # poplib doesn't have AUTH command implementation - res = connection_obj._shortcmd( - "AUTH {0} {1}".format(mechanism, base64.b64encode(bytes(auth_string, "utf-8")).decode("utf-8")) - ) - - if not res.startswith(b"+OK"): - raise - - elif isinstance(connection_obj, imaplib.IMAP4): - connection_obj.authenticate(mechanism, _func) - - else: - # SMTP - connection_obj.auth(mechanism, _func, initial_response_ok=False) - - except Exception: - # maybe the access token expired - refreshing - access_token = refresh_google_access_token(email_account, google_refresh_token) - - if not access_token or retry > 0: - throw( - _("Google Authentication Failed. Please Check and Update the credentials."), - GoogleAuthenticationError, - _("Google Authentication Error"), - ) - - connect_google_oauth( - connection_obj, email_account, email, access_token, google_refresh_token, retry + 1 - ) - - -def refresh_google_access_token(email_account: str, google_refresh_token: str) -> str: - oauth_obj = GoogleOAuth("mail") - res = oauth_obj.refresh_access_token(google_refresh_token) - db.set_value("Email Account", email_account, "google_access_token", res.get("access_token")) - - return res.get("access_token") From 06c5a7226d5c74f627ad8c2a400d47b264685a78 Mon Sep 17 00:00:00 2001 From: phot0n Date: Tue, 31 May 2022 11:24:45 +0530 Subject: [PATCH 46/80] chore: fix linter --- .../email/doctype/email_account/email_account.py | 15 +++++++++------ frappe/email/oauth.py | 13 +++++++++---- frappe/email/receive.py | 4 ++-- frappe/email/smtp.py | 9 ++------- 4 files changed, 22 insertions(+), 19 deletions(-) diff --git a/frappe/email/doctype/email_account/email_account.py b/frappe/email/doctype/email_account/email_account.py index b2f8e1ed64..9d011d3671 100755 --- a/frappe/email/doctype/email_account/email_account.py +++ b/frappe/email/doctype/email_account/email_account.py @@ -163,8 +163,7 @@ class EmailAccount(Document): def after_insert(self): if self.use_oauth and not self.refresh_token: frappe.msgprint( - _("Please Enable OAuth by using `Authorize API access` button"), - indicator="orange" + _("Please Enable OAuth by using `Authorize API access` button"), indicator="orange" ) def there_must_be_only_one_default(self): @@ -288,9 +287,7 @@ class EmailAccount(Document): @property def _password(self): - raise_exception = not ( - self.use_oauth or self.no_smtp_authentication or frappe.flags.in_test - ) + raise_exception = not (self.use_oauth or self.no_smtp_authentication or frappe.flags.in_test) return self.get_password(raise_exception=raise_exception) @property @@ -943,7 +940,13 @@ def oauth_access(email_account: str, reauthorize: bool = False, service: str = N return authorize_google_access(email_account, reauthorize, refresh_token, doctype) -def authorize_google_access(email_account, reauthorize: bool = False, refresh_token: str = None, doctype: str = "Email Account", code: str = None): +def authorize_google_access( + email_account, + reauthorize: bool = False, + refresh_token: str = None, + doctype: str = "Email Account", + code: str = None, +): oauth_obj = GoogleOAuth("mail") if not (refresh_token or code) or reauthorize: diff --git a/frappe/email/oauth.py b/frappe/email/oauth.py index b4efac2e07..740aefc6a6 100644 --- a/frappe/email/oauth.py +++ b/frappe/email/oauth.py @@ -1,8 +1,8 @@ import base64 -from typing import Callable, Union from imaplib import IMAP4 from poplib import POP3 from smtplib import SMTP +from typing import Union import frappe from frappe.integrations.google_oauth import GoogleOAuth @@ -13,7 +13,8 @@ class OAuthenticationError(Exception): class Oauth: - def __init__(self, + def __init__( + self, conn: Union[IMAP4, POP3, SMTP], email_account: str, email: str, @@ -35,7 +36,9 @@ class Oauth: def validate_implementation(self) -> None: if self.service != "GMail": - raise NotImplementedError(f"Service {self.service} currently doesn't have oauth implementation.") + raise NotImplementedError( + f"Service {self.service} currently doesn't have oauth implementation." + ) @property def _auth_string(self) -> str: @@ -74,7 +77,9 @@ class Oauth: def _connect_pop(self) -> bytes: # poplib doesn't have AUTH command implementation res = self._conn._shortcmd( - "AUTH {0} {1}".format(self._mechanism, base64.b64encode(bytes(self._auth_string, "utf-8")).decode("utf-8")) + "AUTH {0} {1}".format( + self._mechanism, base64.b64encode(bytes(self._auth_string, "utf-8")).decode("utf-8") + ) ) return res diff --git a/frappe/email/receive.py b/frappe/email/receive.py index 074ec8e77d..8bfd177579 100644 --- a/frappe/email/receive.py +++ b/frappe/email/receive.py @@ -107,7 +107,7 @@ class EmailServer: self.settings.username, self.settings.access_token, self.settings.refresh_token, - self.settings.service + self.settings.service, ).connect() else: @@ -140,7 +140,7 @@ class EmailServer: self.settings.username, self.settings.access_token, self.settings.refresh_token, - self.settings.service + self.settings.service, ).connect() else: diff --git a/frappe/email/smtp.py b/frappe/email/smtp.py index df88ddb986..f5e2af7194 100644 --- a/frappe/email/smtp.py +++ b/frappe/email/smtp.py @@ -56,7 +56,7 @@ class SMTPServer: use_oauth=0, refresh_token=None, access_token=None, - service=None + service=None, ): self.login = login self.email_account = email_account @@ -113,12 +113,7 @@ class SMTPServer: if self.use_oauth and self.refresh_token: Oauth( - _session, - self.email_account, - self.login, - self.access_token, - self.refresh_token, - self.service + _session, self.email_account, self.login, self.access_token, self.refresh_token, self.service ).connect() elif self.password: From 064ffef8b9f202c1d3db257a26bac722cecc9dec Mon Sep 17 00:00:00 2001 From: phot0n Date: Tue, 31 May 2022 12:25:28 +0530 Subject: [PATCH 47/80] minor: throw exception if refresh_token is not present --- frappe/email/oauth.py | 11 +++++++++-- frappe/email/receive.py | 4 ++-- frappe/email/smtp.py | 2 +- 3 files changed, 12 insertions(+), 5 deletions(-) diff --git a/frappe/email/oauth.py b/frappe/email/oauth.py index 740aefc6a6..c432bcf45a 100644 --- a/frappe/email/oauth.py +++ b/frappe/email/oauth.py @@ -32,14 +32,21 @@ class Oauth: self._access_token = access_token self._refresh_token = refresh_token - self.validate_implementation() + self.validate() - def validate_implementation(self) -> None: + def validate(self) -> None: if self.service != "GMail": raise NotImplementedError( f"Service {self.service} currently doesn't have oauth implementation." ) + if not self._refresh_token: + frappe.throw( + frappe._("Please Authorize OAuth."), + OAuthenticationError, + frappe._("OAuth Error"), + ) + @property def _auth_string(self) -> str: return "user=%s\1auth=Bearer %s\1\1" % (self.email, self._access_token) diff --git a/frappe/email/receive.py b/frappe/email/receive.py index 8bfd177579..e26748dd07 100644 --- a/frappe/email/receive.py +++ b/frappe/email/receive.py @@ -100,7 +100,7 @@ class EmailServer: self.settings.host, self.settings.incoming_port, timeout=frappe.conf.get("pop_timeout") ) - if self.settings.use_oauth and self.settings.refresh_token: + if self.settings.use_oauth: Oauth( self.imap, self.settings.email_account, @@ -133,7 +133,7 @@ class EmailServer: self.settings.host, self.settings.incoming_port, timeout=frappe.conf.get("pop_timeout") ) - if self.settings.use_oauth and self.settings.refresh_token: + if self.settings.use_oauth: Oauth( self.pop, self.settings.email_account, diff --git a/frappe/email/smtp.py b/frappe/email/smtp.py index f5e2af7194..687b8318b6 100644 --- a/frappe/email/smtp.py +++ b/frappe/email/smtp.py @@ -111,7 +111,7 @@ class SMTPServer: self.secure_session(_session) - if self.use_oauth and self.refresh_token: + if self.use_oauth: Oauth( _session, self.email_account, self.login, self.access_token, self.refresh_token, self.service ).connect() From 67730b7b268728c16f0df11cf92c9be3ef1befb1 Mon Sep 17 00:00:00 2001 From: phot0n Date: Tue, 31 May 2022 12:43:24 +0530 Subject: [PATCH 48/80] chore: fix sider --- frappe/email/doctype/email_account/email_account.js | 2 +- frappe/email/doctype/email_account/email_account.py | 4 ++-- frappe/email/oauth.py | 1 - 3 files changed, 3 insertions(+), 4 deletions(-) diff --git a/frappe/email/doctype/email_account/email_account.js b/frappe/email/doctype/email_account/email_account.js index 4d84f69395..97d93a63c2 100644 --- a/frappe/email/doctype/email_account/email_account.js +++ b/frappe/email/doctype/email_account/email_account.js @@ -148,7 +148,7 @@ frappe.ui.form.on("Email Account", { method: "frappe.email.doctype.email_account.email_account.oauth_access", args: { "email_account": frm.doc.name, - "service": frm.doc.service, + "service": frm.doc.service || "", "reauthorize": frm.doc.refresh_token ? 1 : 0 }, callback: function(r) { diff --git a/frappe/email/doctype/email_account/email_account.py b/frappe/email/doctype/email_account/email_account.py index 9d011d3671..2cf8ec69fb 100755 --- a/frappe/email/doctype/email_account/email_account.py +++ b/frappe/email/doctype/email_account/email_account.py @@ -213,7 +213,7 @@ class EmailAccount(Document): "host": self.email_server, "use_ssl": self.use_ssl, "username": getattr(self, "login_id", None) or self.email_id, - "service": getattr(self, "service", None), + "service": getattr(self, "service", ""), "use_imap": self.use_imap, "email_sync_rule": email_sync_rule, "incoming_port": get_port(self), @@ -425,7 +425,7 @@ class EmailAccount(Document): "password": self._password, "use_ssl": cint(self.use_ssl_for_outgoing), "use_tls": cint(self.use_tls), - "service": getattr(self, "service", None), + "service": getattr(self, "service", ""), "use_oauth": self.use_oauth or 0, "refresh_token": getattr(self, "refresh_token", None), "access_token": getattr(self, "access_token", None), diff --git a/frappe/email/oauth.py b/frappe/email/oauth.py index c432bcf45a..3dfbfaf97d 100644 --- a/frappe/email/oauth.py +++ b/frappe/email/oauth.py @@ -69,7 +69,6 @@ class Oauth: except Exception: # maybe the access token expired - refreshing access_token = self._refresh_access_token() - print(self._auth_string) if not access_token or _retry > 0: frappe.throw( From de6f1326f79345e58641dfafdc701e010d32ac0a Mon Sep 17 00:00:00 2001 From: phot0n Date: Tue, 31 May 2022 19:04:04 +0530 Subject: [PATCH 49/80] minor: move oauth access functions from email_account --- .../doctype/email_account/email_account.js | 2 +- .../doctype/email_account/email_account.py | 36 ---------------- frappe/email/oauth.py | 42 ++++++++++++++++++- 3 files changed, 42 insertions(+), 38 deletions(-) diff --git a/frappe/email/doctype/email_account/email_account.js b/frappe/email/doctype/email_account/email_account.js index 97d93a63c2..5cb7f1e497 100644 --- a/frappe/email/doctype/email_account/email_account.js +++ b/frappe/email/doctype/email_account/email_account.js @@ -145,7 +145,7 @@ frappe.ui.form.on("Email Account", { authorize_api_access: function(frm) { frappe.call({ - method: "frappe.email.doctype.email_account.email_account.oauth_access", + method: "frappe.email.oauth.oauth_access", args: { "email_account": frm.doc.name, "service": frm.doc.service || "", diff --git a/frappe/email/doctype/email_account/email_account.py b/frappe/email/doctype/email_account/email_account.py index 2cf8ec69fb..ea19c01f4a 100755 --- a/frappe/email/doctype/email_account/email_account.py +++ b/frappe/email/doctype/email_account/email_account.py @@ -8,7 +8,6 @@ import socket import time from datetime import datetime, timedelta from poplib import error_proto -from urllib.parse import quote import frappe from frappe import _, are_emails_muted, safe_encode @@ -16,13 +15,11 @@ from frappe.desk.form import assign_to from frappe.email.receive import EmailServer, InboundMail, SentEmailInInboxError from frappe.email.smtp import SMTPServer from frappe.email.utils import get_port -from frappe.integrations.google_oauth import GoogleOAuth from frappe.model.document import Document from frappe.utils import ( cint, comma_or, cstr, - get_request_site_address, parse_addr, validate_email_address, ) @@ -929,36 +926,3 @@ def set_email_password(email_account, user, password): return False return True - - -@frappe.whitelist(methods=["POST"]) -def oauth_access(email_account: str, reauthorize: bool = False, service: str = None): - doctype = "Email Account" - refresh_token = frappe.db.get_value(doctype, email_account, "refresh_token") - - if service == "GMail": - return authorize_google_access(email_account, reauthorize, refresh_token, doctype) - - -def authorize_google_access( - email_account, - reauthorize: bool = False, - refresh_token: str = None, - doctype: str = "Email Account", - code: str = None, -): - oauth_obj = GoogleOAuth("mail") - - if not (refresh_token or code) or reauthorize: - return oauth_obj.get_authentication_url( - get_request_site_address(True), - state={ - "method": "frappe.email.doctype.email_account.email_account.authorize_google_access", - "redirect": "/app/Form/{0}/{1}".format(quote(doctype), quote(email_account)), - "email_account": email_account, - }, - ) - - res = oauth_obj.authorize(code, get_request_site_address(True)) - frappe.db.set_value(doctype, email_account, "refresh_token", res.get("refresh_token")) - frappe.db.set_value(doctype, email_account, "access_token", res.get("access_token")) diff --git a/frappe/email/oauth.py b/frappe/email/oauth.py index 3dfbfaf97d..5ebb231cf6 100644 --- a/frappe/email/oauth.py +++ b/frappe/email/oauth.py @@ -3,9 +3,11 @@ from imaplib import IMAP4 from poplib import POP3 from smtplib import SMTP from typing import Union +from urllib.parse import quote import frappe from frappe.integrations.google_oauth import GoogleOAuth +from frappe.utils import get_request_site_address class OAuthenticationError(Exception): @@ -107,5 +109,43 @@ class Oauth: def _get_service_object(self): return { - "GMail": GoogleOAuth("mail"), + "GMail": GoogleOAuth("mail", validate=False), }[self.service] + + +@frappe.whitelist(methods=["POST"]) +def oauth_access(email_account: str, reauthorize: bool = False, service: str = None): + doctype = "Email Account" + refresh_token = frappe.db.get_value(doctype, email_account, "refresh_token") + + # NOTE: setting this here, since we redirect to the service's auth page, + # we lose the use_oauth value in the emal account form + frappe.db.set_value(doctype, email_account, "use_oauth", 1) + + if service: + if service == "GMail": + return authorize_google_access(email_account, reauthorize, refresh_token, doctype) + + +def authorize_google_access( + email_account, + reauthorize: bool = False, + refresh_token: str = None, + doctype: str = "Email Account", + code: str = None, +): + oauth_obj = GoogleOAuth("mail") + + if not (refresh_token or code) or reauthorize: + return oauth_obj.get_authentication_url( + get_request_site_address(True), + state={ + "method": "frappe.email.oauth.authorize_google_access", + "redirect": "/app/Form/{0}/{1}".format(quote(doctype), quote(email_account)), + "email_account": email_account, + }, + ) + + res = oauth_obj.authorize(code, get_request_site_address(True)) + frappe.db.set_value(doctype, email_account, "refresh_token", res.get("refresh_token")) + frappe.db.set_value(doctype, email_account, "access_token", res.get("access_token")) From ed0a255353637069fe7d8f181a6263b1281fe9cd Mon Sep 17 00:00:00 2001 From: phot0n Date: Tue, 31 May 2022 21:00:11 +0530 Subject: [PATCH 50/80] minor: fetch oauth fields from site config --- frappe/email/doctype/email_account/email_account.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/frappe/email/doctype/email_account/email_account.py b/frappe/email/doctype/email_account/email_account.py index ea19c01f4a..18c13ed2c4 100755 --- a/frappe/email/doctype/email_account/email_account.py +++ b/frappe/email/doctype/email_account/email_account.py @@ -403,6 +403,9 @@ class EmailAccount(Document): "default": 0, }, "name": {"conf_names": ("email_sender_name",), "default": "Frappe"}, + "use_oauth": {"conf_names": ("use_oauth"), "default": 0}, + "access_token": {"conf_names": ("mail_access_token")}, + "refresh_token": {"conf_names": ("mail_refresh_token")}, "from_site_config": {"default": True}, } From e58afca3f6470d0cb505c61f96e0777cde91e7bd Mon Sep 17 00:00:00 2001 From: phot0n Date: Wed, 1 Jun 2022 00:44:38 +0530 Subject: [PATCH 51/80] minor: simplify authorize_google_access --- .../email/doctype/email_account/email_account.js | 3 +-- .../email/doctype/email_account/email_account.py | 8 +------- frappe/email/oauth.py | 15 ++++----------- 3 files changed, 6 insertions(+), 20 deletions(-) diff --git a/frappe/email/doctype/email_account/email_account.js b/frappe/email/doctype/email_account/email_account.js index 5cb7f1e497..fda4966b90 100644 --- a/frappe/email/doctype/email_account/email_account.js +++ b/frappe/email/doctype/email_account/email_account.js @@ -148,8 +148,7 @@ frappe.ui.form.on("Email Account", { method: "frappe.email.oauth.oauth_access", args: { "email_account": frm.doc.name, - "service": frm.doc.service || "", - "reauthorize": frm.doc.refresh_token ? 1 : 0 + "service": frm.doc.service || "" }, callback: function(r) { if (!r.exc) { diff --git a/frappe/email/doctype/email_account/email_account.py b/frappe/email/doctype/email_account/email_account.py index 18c13ed2c4..e17c021e52 100755 --- a/frappe/email/doctype/email_account/email_account.py +++ b/frappe/email/doctype/email_account/email_account.py @@ -16,13 +16,7 @@ from frappe.email.receive import EmailServer, InboundMail, SentEmailInInboxError from frappe.email.smtp import SMTPServer from frappe.email.utils import get_port from frappe.model.document import Document -from frappe.utils import ( - cint, - comma_or, - cstr, - parse_addr, - validate_email_address, -) +from frappe.utils import cint, comma_or, cstr, parse_addr, validate_email_address from frappe.utils.background_jobs import enqueue, get_jobs from frappe.utils.error import raise_error_on_no_output from frappe.utils.jinja import render_template diff --git a/frappe/email/oauth.py b/frappe/email/oauth.py index 5ebb231cf6..ccb3cac3f0 100644 --- a/frappe/email/oauth.py +++ b/frappe/email/oauth.py @@ -114,9 +114,8 @@ class Oauth: @frappe.whitelist(methods=["POST"]) -def oauth_access(email_account: str, reauthorize: bool = False, service: str = None): +def oauth_access(email_account: str, service: str = None): doctype = "Email Account" - refresh_token = frappe.db.get_value(doctype, email_account, "refresh_token") # NOTE: setting this here, since we redirect to the service's auth page, # we lose the use_oauth value in the emal account form @@ -124,19 +123,13 @@ def oauth_access(email_account: str, reauthorize: bool = False, service: str = N if service: if service == "GMail": - return authorize_google_access(email_account, reauthorize, refresh_token, doctype) + return authorize_google_access(email_account, doctype) -def authorize_google_access( - email_account, - reauthorize: bool = False, - refresh_token: str = None, - doctype: str = "Email Account", - code: str = None, -): +def authorize_google_access(email_account, doctype: str = "Email Account", code: str = None): oauth_obj = GoogleOAuth("mail") - if not (refresh_token or code) or reauthorize: + if not code: return oauth_obj.get_authentication_url( get_request_site_address(True), state={ From 221423c71808636d60e4b3da228785f9d6fea368 Mon Sep 17 00:00:00 2001 From: phot0n Date: Wed, 1 Jun 2022 14:49:53 +0530 Subject: [PATCH 52/80] chore: added docstrings --- frappe/email/oauth.py | 23 ++++++++++++++++++----- frappe/integrations/google_oauth.py | 20 +++++++++++++++++++- 2 files changed, 37 insertions(+), 6 deletions(-) diff --git a/frappe/email/oauth.py b/frappe/email/oauth.py index ccb3cac3f0..44e69d8d85 100644 --- a/frappe/email/oauth.py +++ b/frappe/email/oauth.py @@ -34,9 +34,9 @@ class Oauth: self._access_token = access_token self._refresh_token = refresh_token - self.validate() + self._validate() - def validate(self) -> None: + def _validate(self) -> None: if self.service != "GMail": raise NotImplementedError( f"Service {self.service} currently doesn't have oauth implementation." @@ -54,6 +54,7 @@ class Oauth: return "user=%s\1auth=Bearer %s\1\1" % (self.email, self._access_token) def connect(self, _retry: int = 0) -> None: + """Connection method with retry on exception for Oauth""" try: if isinstance(self._conn, POP3): res = self._connect_pop() @@ -99,6 +100,7 @@ class Oauth: self._conn.auth(self._mechanism, lambda x: self._auth_string, initial_response_ok=False) def _refresh_access_token(self) -> str: + """Refreshes access token via calling `refresh_access_token` method of oauth service object""" service_obj = self._get_service_object() access_token = service_obj.refresh_access_token(self._refresh_token).get("access_token", None) @@ -108,6 +110,8 @@ class Oauth: return access_token def _get_service_object(self): + """Get Oauth service object""" + return { "GMail": GoogleOAuth("mail", validate=False), }[self.service] @@ -115,18 +119,27 @@ class Oauth: @frappe.whitelist(methods=["POST"]) def oauth_access(email_account: str, service: str = None): + """Used as a default endpoint/caller for all oauth services. + Returns authorization url for redirection""" + + if not service: + frappe.throw(frappe._("No Service is selected. Please select one and try again!")) + doctype = "Email Account" # NOTE: setting this here, since we redirect to the service's auth page, # we lose the use_oauth value in the emal account form frappe.db.set_value(doctype, email_account, "use_oauth", 1) - if service: - if service == "GMail": - return authorize_google_access(email_account, doctype) + if service == "GMail": + return authorize_google_access(email_account, doctype) def authorize_google_access(email_account, doctype: str = "Email Account", code: str = None): + """Facilitates google oauth for email. + This is invoked 2 times - first time when user clicks `Authorze API Access` for getting the authorization url + and second time for setting the refresh and access token in db when google redirects back with oauth code.""" + oauth_obj = GoogleOAuth("mail") if not code: diff --git a/frappe/integrations/google_oauth.py b/frappe/integrations/google_oauth.py index da67f2ccf5..c720a22d28 100644 --- a/frappe/integrations/google_oauth.py +++ b/frappe/integrations/google_oauth.py @@ -48,6 +48,12 @@ class GoogleOAuth: frappe.throw(frappe._("Please update Google Settings before continuing.")) def authorize(self, oauth_code: str, site_address: str) -> Dict[str, Union[str, int]]: + """Returns a dict with access and refresh token. + + :param oauth_code: code got back from google upon successful auhtorization + :param site_address: side address from which the request is being made + """ + data = { "code": oauth_code, "client_id": self.google_settings.client_id, @@ -66,6 +72,8 @@ class GoogleOAuth: ) def refresh_access_token(self, refresh_token: str) -> Dict[str, Union[str, int]]: + """Refreshes google access token using refresh token""" + data = { "client_id": self.google_settings.client_id, "client_secret": self.google_settings.get_password( @@ -86,7 +94,11 @@ class GoogleOAuth: def get_authentication_url( self, site_address: str, state: Dict[str, str] = None ) -> Dict[str, str]: - """Return authentication url with the client id and redirect uri.""" + """Returns google authentication url. + + :param site_address: side address from which the request is being made (for redirect back to site) + :param state: [optional] dict of values which you need on callback (for calling methods, redirection back to the form, doc name, etc) + """ state = json.dumps(state) callback_url = site_address + CALLBACK_METHOD @@ -100,6 +112,8 @@ class GoogleOAuth: } def get_google_service_object(self, access_token: str, refresh_token: str): + """Returns google service object""" + credentials_dict = { "token": access_token, "refresh_token": refresh_token, @@ -151,6 +165,10 @@ def is_valid_access_token(access_token: str) -> bool: @frappe.whitelist(methods=["GET"]) def callback(state: str, code: str = None, error: str = None) -> None: + """Common callback for google integrations. + Invokes functions using `frappe.get_attr` and also adds required (keyworded) arguments + along with committing and redirecting us back to frappe site.""" + state = json.loads(state) redirect = state.pop("redirect", "/app") From ab9a577474cb26f22559392d83c2c7070c0b45bf Mon Sep 17 00:00:00 2001 From: phot0n Date: Wed, 1 Jun 2022 17:57:51 +0530 Subject: [PATCH 53/80] minor: better oauth flow --- .../doctype/email_account/email_account.js | 43 +++++++++++++------ .../doctype/email_account/email_account.json | 2 +- .../doctype/email_account/email_account.py | 21 +++++---- frappe/integrations/google_oauth.py | 2 +- 4 files changed, 43 insertions(+), 25 deletions(-) diff --git a/frappe/email/doctype/email_account/email_account.js b/frappe/email/doctype/email_account/email_account.js index fda4966b90..d553b58e28 100644 --- a/frappe/email/doctype/email_account/email_account.js +++ b/frappe/email/doctype/email_account/email_account.js @@ -67,6 +67,21 @@ frappe.email_defaults_pop = { }; +function oauth_access(frm) { + return frappe.call({ + method: "frappe.email.oauth.oauth_access", + args: { + "email_account": frm.doc.name, + "service": frm.doc.service || "" + }, + callback: function(r) { + if (!r.exc) { + window.open(r.message.url); + } + } + }); +} + frappe.ui.form.on("Email Account", { service: function(frm) { $.each(frappe.email_defaults[frm.doc.service], function(key, value) { @@ -126,6 +141,7 @@ frappe.ui.form.on("Email Account", { frm.events.enable_incoming(frm); frm.events.notify_if_unreplied(frm); frm.events.show_gmail_message_for_less_secure_apps(frm); + frm.events.show_oauth_authorization_message(frm); if (frappe.route_flags.delete_user_from_locals && frappe.route_flags.linked_user) { delete frappe.route_flags.delete_user_from_locals; @@ -133,6 +149,12 @@ frappe.ui.form.on("Email Account", { } }, + after_save(frm) { + if (frm.doc.use_oauth && !frm.doc.refresh_token) { + oauth_access(frm); + } + }, + show_gmail_message_for_less_secure_apps: function(frm) { frm.dashboard.clear_headline(); let msg = __("GMail will only work if you enable 2-step authentication and use app-specific password OR use OAuth."); @@ -143,19 +165,16 @@ frappe.ui.form.on("Email Account", { } }, + show_oauth_authorization_message(frm) { + let msg = __("Oauth Enabled but not Authorized. Please use `Authorize API Access` Button to do the same."); + if (frm.doc.use_oauth && !frm.doc.refresh_token) { + frm.dashboard.clear_headline(); + frm.dashboard.set_headline_alert(msg, "yellow"); + } + }, + authorize_api_access: function(frm) { - frappe.call({ - method: "frappe.email.oauth.oauth_access", - args: { - "email_account": frm.doc.name, - "service": frm.doc.service || "" - }, - callback: function(r) { - if (!r.exc) { - window.open(r.message.url); - } - } - }); + oauth_access(frm); }, email_id:function(frm) { diff --git a/frappe/email/doctype/email_account/email_account.json b/frappe/email/doctype/email_account/email_account.json index 72be4f2e42..f76bacd044 100644 --- a/frappe/email/doctype/email_account/email_account.json +++ b/frappe/email/doctype/email_account/email_account.json @@ -613,7 +613,7 @@ "icon": "fa fa-inbox", "index_web_pages_for_search": 1, "links": [], - "modified": "2022-05-30 15:45:05.282867", + "modified": "2022-06-01 17:51:37.878446", "modified_by": "Administrator", "module": "Email", "name": "Email Account", diff --git a/frappe/email/doctype/email_account/email_account.py b/frappe/email/doctype/email_account/email_account.py index e17c021e52..71cc16bb21 100755 --- a/frappe/email/doctype/email_account/email_account.py +++ b/frappe/email/doctype/email_account/email_account.py @@ -83,8 +83,16 @@ class EmailAccount(Document): if getattr(self, "service", "") != "GMail" and self.use_oauth: self.use_oauth = 0 + if not self.use_oauth and self.refresh_token: + # clear access & refresh token + self.refresh_token = self.access_token = None + if not frappe.local.flags.in_install and (self.use_oauth or not self.awaiting_password): - if self.refresh_token or self.password or self.smtp_server in ("127.0.0.1", "localhost"): + if ( + (self.use_oauth and self.refresh_token) + or self.password + or self.smtp_server in ("127.0.0.1", "localhost") + ): if self.enable_incoming: self.get_incoming_server() self.no_failed = 0 @@ -93,10 +101,7 @@ class EmailAccount(Document): self.validate_smtp_conn() else: if self.enable_incoming or (self.enable_outgoing and not self.no_smtp_authentication): - if self.use_oauth: - if not self.is_new(): - frappe.throw(_("Please Enable OAuth by using `Authorize API access` button")) - else: + if not self.use_oauth: frappe.throw(_("Password is required or select Awaiting Password")) if self.notify_if_unreplied: @@ -151,12 +156,6 @@ class EmailAccount(Document): enable_outgoing=self.enable_outgoing, ) - def after_insert(self): - if self.use_oauth and not self.refresh_token: - frappe.msgprint( - _("Please Enable OAuth by using `Authorize API access` button"), indicator="orange" - ) - def there_must_be_only_one_default(self): """If current Email Account is default, un-default all other accounts.""" for field in ("default_incoming", "default_outgoing"): diff --git a/frappe/integrations/google_oauth.py b/frappe/integrations/google_oauth.py index c720a22d28..c920fc31fa 100644 --- a/frappe/integrations/google_oauth.py +++ b/frappe/integrations/google_oauth.py @@ -145,7 +145,7 @@ def handle_response( ) if raise_err: - frappe.throw(frappe._(error_title), frappe._(error_message)) + frappe.throw(frappe._(error_title), GoogleAuthenticationError, frappe._(error_message)) return {} From 834410a2a56b44c280cb5c6f0a375c8dcc238a8b Mon Sep 17 00:00:00 2001 From: phot0n Date: Wed, 1 Jun 2022 18:01:27 +0530 Subject: [PATCH 54/80] fix: dont update modified timestamp on generating and refreshing tokens --- frappe/email/oauth.py | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/frappe/email/oauth.py b/frappe/email/oauth.py index 44e69d8d85..2e328dc2b6 100644 --- a/frappe/email/oauth.py +++ b/frappe/email/oauth.py @@ -105,7 +105,9 @@ class Oauth: access_token = service_obj.refresh_access_token(self._refresh_token).get("access_token", None) # set the new access token in db - frappe.db.set_value("Email Account", self.email_account, "access_token", access_token) + frappe.db.set_value( + "Email Account", self.email_account, "access_token", access_token, update_modified=True + ) frappe.db.commit() return access_token @@ -129,7 +131,7 @@ def oauth_access(email_account: str, service: str = None): # NOTE: setting this here, since we redirect to the service's auth page, # we lose the use_oauth value in the emal account form - frappe.db.set_value(doctype, email_account, "use_oauth", 1) + frappe.db.set_value(doctype, email_account, "use_oauth", 1, update_modified=False) if service == "GMail": return authorize_google_access(email_account, doctype) @@ -153,5 +155,9 @@ def authorize_google_access(email_account, doctype: str = "Email Account", code: ) res = oauth_obj.authorize(code, get_request_site_address(True)) - frappe.db.set_value(doctype, email_account, "refresh_token", res.get("refresh_token")) - frappe.db.set_value(doctype, email_account, "access_token", res.get("access_token")) + frappe.db.set_value( + doctype, email_account, "refresh_token", res.get("refresh_token"), update_modified=False + ) + frappe.db.set_value( + doctype, email_account, "access_token", res.get("access_token"), update_modified=False + ) From 4d5dec4048d5b4d83a5fd3c5c9ece2ab2bd39df2 Mon Sep 17 00:00:00 2001 From: phot0n Date: Wed, 1 Jun 2022 18:39:07 +0530 Subject: [PATCH 55/80] fix: open authorization url on the current browsing context --- frappe/email/doctype/email_account/email_account.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frappe/email/doctype/email_account/email_account.js b/frappe/email/doctype/email_account/email_account.js index d553b58e28..5d65a8efe6 100644 --- a/frappe/email/doctype/email_account/email_account.js +++ b/frappe/email/doctype/email_account/email_account.js @@ -76,7 +76,7 @@ function oauth_access(frm) { }, callback: function(r) { if (!r.exc) { - window.open(r.message.url); + window.open(r.message.url, "_self"); } } }); From 5521abd40cf8f70d6e0826a6c86d2283f665e799 Mon Sep 17 00:00:00 2001 From: phot0n Date: Thu, 2 Jun 2022 13:06:48 +0530 Subject: [PATCH 56/80] fix: use safe decode for uid --- frappe/email/doctype/email_account/email_account.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/frappe/email/doctype/email_account/email_account.py b/frappe/email/doctype/email_account/email_account.py index 71cc16bb21..4eb230c372 100755 --- a/frappe/email/doctype/email_account/email_account.py +++ b/frappe/email/doctype/email_account/email_account.py @@ -502,11 +502,10 @@ class EmailAccount(Document): def process_mail(messages, append_to=None): for index, message in enumerate(messages.get("latest_messages", [])): uid = messages["uid_list"][index] if messages.get("uid_list") else None - uid = uid.decode() if isinstance(uid, bytes) else uid seen_status = messages.get("seen_status", {}).get(uid) if self.email_sync_option != "UNSEEN" or seen_status != "SEEN": # only append the emails with status != 'SEEN' if sync option is set to 'UNSEEN' - mails.append(InboundMail(message, self, uid, seen_status, append_to)) + mails.append(InboundMail(message, self, frappe.safe_decode(uid), seen_status, append_to)) if not self.enable_incoming: return [] From 6d3dfca2148dbdde73cc00c413063474df2e8410 Mon Sep 17 00:00:00 2001 From: phot0n Date: Sun, 5 Jun 2022 22:34:02 +0530 Subject: [PATCH 57/80] fix: consider oauth usage as well for asking/updating user email password --- frappe/core/doctype/user/user.py | 4 +-- .../core/doctype/user_email/user_email.json | 15 +++++++++-- .../doctype/email_account/email_account.py | 26 +++++++++++++------ frappe/public/js/frappe/desk.js | 1 - 4 files changed, 33 insertions(+), 13 deletions(-) diff --git a/frappe/core/doctype/user/user.py b/frappe/core/doctype/user/user.py index eb3c11a4db..12a48afe7e 100644 --- a/frappe/core/doctype/user/user.py +++ b/frappe/core/doctype/user/user.py @@ -766,7 +766,7 @@ def get_email_awaiting(user): return frappe.get_all( "User Email", fields=["email_account", "email_id"], - filters={"awaiting_password": 1, "parent": user}, + filters={"awaiting_password": 1, "parent": user, "used_oauth": 0}, ) @@ -775,7 +775,7 @@ def ask_pass_update(): from frappe.utils import set_default password_list = frappe.get_all( - "User Email", filters={"awaiting_password": True}, pluck="parent", distinct=True + "User Email", filters={"awaiting_password": 1, "used_oauth": 0}, pluck="parent", distinct=True ) set_default("email_user_password", ",".join(password_list)) diff --git a/frappe/core/doctype/user_email/user_email.json b/frappe/core/doctype/user_email/user_email.json index b106ed4a19..43dc3ace8d 100644 --- a/frappe/core/doctype/user_email/user_email.json +++ b/frappe/core/doctype/user_email/user_email.json @@ -9,6 +9,7 @@ "email_id", "column_break_3", "awaiting_password", + "used_oauth", "enable_outgoing" ], "fields": [ @@ -48,16 +49,26 @@ "fieldtype": "Check", "label": "Enable Outgoing", "read_only": 1 + }, + { + "default": "0", + "fetch_from": "email_account.use_oauth", + "fieldname": "used_oauth", + "fieldtype": "Check", + "in_list_view": 1, + "label": "Used OAuth", + "read_only": 1 } ], "istable": 1, "links": [], - "modified": "2020-04-06 19:19:12.130246", + "modified": "2022-06-03 14:25:46.944733", "modified_by": "Administrator", "module": "Core", "name": "User Email", "owner": "Administrator", "permissions": [], "sort_field": "modified", - "sort_order": "DESC" + "sort_order": "DESC", + "states": [] } \ No newline at end of file diff --git a/frappe/email/doctype/email_account/email_account.py b/frappe/email/doctype/email_account/email_account.py index 4eb230c372..278aeb698b 100755 --- a/frappe/email/doctype/email_account/email_account.py +++ b/frappe/email/doctype/email_account/email_account.py @@ -83,13 +83,17 @@ class EmailAccount(Document): if getattr(self, "service", "") != "GMail" and self.use_oauth: self.use_oauth = 0 - if not self.use_oauth and self.refresh_token: + if self.use_oauth: + # no need for awaiting password for oauth + self.awaiting_password = 0 + + elif self.refresh_token: # clear access & refresh token self.refresh_token = self.access_token = None if not frappe.local.flags.in_install and (self.use_oauth or not self.awaiting_password): if ( - (self.use_oauth and self.refresh_token) + self.refresh_token or self.password or self.smtp_server in ("127.0.0.1", "localhost") ): @@ -154,6 +158,7 @@ class EmailAccount(Document): awaiting_password=self.awaiting_password, email_id=self.email_id, enable_outgoing=self.enable_outgoing, + used_oauth=self.use_oauth, ) def there_must_be_only_one_default(self): @@ -839,7 +844,7 @@ def get_max_email_uid(email_account): return max_uid -def setup_user_email_inbox(email_account, awaiting_password, email_id, enable_outgoing): +def setup_user_email_inbox(email_account, awaiting_password, email_id, enable_outgoing, used_oauth): """setup email inbox for user""" from frappe.core.doctype.user.user import ask_pass_update @@ -850,6 +855,7 @@ def setup_user_email_inbox(email_account, awaiting_password, email_id, enable_ou row.email_id = email_id row.email_account = email_account row.awaiting_password = awaiting_password or 0 + row.used_oauth = used_oauth or 0 row.enable_outgoing = enable_outgoing or 0 user.save(ignore_permissions=True) @@ -881,8 +887,12 @@ def setup_user_email_inbox(email_account, awaiting_password, email_id, enable_ou if update_user_email_settings: UserEmail = frappe.qb.DocType("User Email") - frappe.qb.update(UserEmail).set(UserEmail.awaiting_password, (awaiting_password or 0)).set( - UserEmail.enable_outgoing, enable_outgoing + frappe.qb.update(UserEmail).set( + UserEmail.awaiting_password, (awaiting_password or 0) + ).set( + UserEmail.enable_outgoing, (enable_outgoing or 0) + ).set( + UserEmail.used_oauth, (used_oauth or 0) ).where(UserEmail.email_account == email_account).run() else: @@ -908,10 +918,10 @@ def remove_user_email_inbox(email_account): doc.save(ignore_permissions=True) -@frappe.whitelist(allow_guest=False) -def set_email_password(email_account, user, password): +@frappe.whitelist() +def set_email_password(email_account, password): account = frappe.get_doc("Email Account", email_account) - if account.awaiting_password: + if account.awaiting_password and not account.use_oauth: account.awaiting_password = 0 account.password = password try: diff --git a/frappe/public/js/frappe/desk.js b/frappe/public/js/frappe/desk.js index a8cbe020f3..f1a5d00dfd 100644 --- a/frappe/public/js/frappe/desk.js +++ b/frappe/public/js/frappe/desk.js @@ -251,7 +251,6 @@ frappe.Application = class Application { method: 'frappe.email.doctype.email_account.email_account.set_email_password', args: { "email_account": email_account[i]["email_account"], - "user": user, "password": d.get_value("password") }, callback: function(passed) { From 6848c93770e5d15fada4e5c6a4eaca6890d7ad19 Mon Sep 17 00:00:00 2001 From: phot0n Date: Mon, 13 Jun 2022 13:25:20 +0530 Subject: [PATCH 58/80] chore: remove GET method whitelisting from google integrations --- frappe/integrations/doctype/google_contacts/google_contacts.py | 2 +- frappe/integrations/doctype/google_drive/google_drive.py | 2 +- frappe/website/doctype/website_settings/google_indexing.py | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/frappe/integrations/doctype/google_contacts/google_contacts.py b/frappe/integrations/doctype/google_contacts/google_contacts.py index 51ebcdd730..8a4fc7d9bb 100644 --- a/frappe/integrations/doctype/google_contacts/google_contacts.py +++ b/frappe/integrations/doctype/google_contacts/google_contacts.py @@ -29,7 +29,7 @@ class GoogleContacts(Document): return r.get("access_token") -@frappe.whitelist(methods=["POST", "GET"]) +@frappe.whitelist(methods=["POST"]) def authorize_access(g_contact, reauthorize=False, code=None): """ If no Authorization code get it from Google and then request for Refresh Token. diff --git a/frappe/integrations/doctype/google_drive/google_drive.py b/frappe/integrations/doctype/google_drive/google_drive.py index b9416e6669..dd3d87ea71 100644 --- a/frappe/integrations/doctype/google_drive/google_drive.py +++ b/frappe/integrations/doctype/google_drive/google_drive.py @@ -40,7 +40,7 @@ class GoogleDrive(Document): return r.get("access_token") -@frappe.whitelist(methods=["POST", "GET"]) +@frappe.whitelist(methods=["POST"]) def authorize_access(reauthorize=False, code=None): """ If no Authorization code get it from Google and then request for Refresh Token. diff --git a/frappe/website/doctype/website_settings/google_indexing.py b/frappe/website/doctype/website_settings/google_indexing.py index b89918dff2..25779967e4 100644 --- a/frappe/website/doctype/website_settings/google_indexing.py +++ b/frappe/website/doctype/website_settings/google_indexing.py @@ -12,7 +12,7 @@ from frappe.integrations.google_oauth import GoogleOAuth from frappe.utils import get_request_site_address -@frappe.whitelist(methods=["POST", "GET"]) +@frappe.whitelist(methods=["POST"]) def authorize_access(reauthorize=False, code=None): """If no Authorization code get it from Google and then request for Refresh Token.""" From 484758d6e0b38160be84702ac7b8ac1e761cf2b3 Mon Sep 17 00:00:00 2001 From: phot0n Date: Mon, 13 Jun 2022 13:31:57 +0530 Subject: [PATCH 59/80] chore: remove additional/unnecessary set_value calls --- frappe/email/oauth.py | 8 ++++---- .../doctype/google_contacts/google_contacts.py | 8 +++++--- frappe/integrations/doctype/google_drive/google_drive.py | 7 +++++-- .../website/doctype/website_settings/google_indexing.py | 5 +++-- 4 files changed, 17 insertions(+), 11 deletions(-) diff --git a/frappe/email/oauth.py b/frappe/email/oauth.py index 2e328dc2b6..5793d84a13 100644 --- a/frappe/email/oauth.py +++ b/frappe/email/oauth.py @@ -156,8 +156,8 @@ def authorize_google_access(email_account, doctype: str = "Email Account", code: res = oauth_obj.authorize(code, get_request_site_address(True)) frappe.db.set_value( - doctype, email_account, "refresh_token", res.get("refresh_token"), update_modified=False - ) - frappe.db.set_value( - doctype, email_account, "access_token", res.get("access_token"), update_modified=False + doctype, + email_account, + {"refresh_token": res.get("refresh_token"), "access_token": res.get("access_token")}, + update_modified=False, ) diff --git a/frappe/integrations/doctype/google_contacts/google_contacts.py b/frappe/integrations/doctype/google_contacts/google_contacts.py index 8a4fc7d9bb..1b309a9e16 100644 --- a/frappe/integrations/doctype/google_contacts/google_contacts.py +++ b/frappe/integrations/doctype/google_contacts/google_contacts.py @@ -51,10 +51,12 @@ def authorize_access(g_contact, reauthorize=False, code=None): }, ) - frappe.db.set_value("Google Contacts", g_contact, "authorization_code", oauth_code) r = oauth_obj.authorize(oauth_code, get_request_site_address(True)) - if r: - frappe.db.set_value("Google Contacts", g_contact, "refresh_token", r.get("refresh_token")) + frappe.db.set_value( + "Google Contacts", + g_contact, + {"authorization_code": oauth_code, "refresh_token": r.get("refresh_token")}, + ) def get_google_contacts_object(g_contact): diff --git a/frappe/integrations/doctype/google_drive/google_drive.py b/frappe/integrations/doctype/google_drive/google_drive.py index dd3d87ea71..22b3759266 100644 --- a/frappe/integrations/doctype/google_drive/google_drive.py +++ b/frappe/integrations/doctype/google_drive/google_drive.py @@ -63,9 +63,12 @@ def authorize_access(reauthorize=False, code=None): }, ) - frappe.db.set_value("Google Drive", None, "authorization_code", oauth_code) r = oauth_obj.authorize(oauth_code, get_request_site_address(True)) - frappe.db.set_value("Google Drive", "Google Drive", "refresh_token", r.get("refresh_token")) + frappe.db.set_value( + "Google Drive", + "Google Drive", + {"authorization_code": oauth_code, "refresh_token": r.get("refresh_token")}, + ) def get_google_drive_object(): diff --git a/frappe/website/doctype/website_settings/google_indexing.py b/frappe/website/doctype/website_settings/google_indexing.py index 25779967e4..3b36bb5360 100644 --- a/frappe/website/doctype/website_settings/google_indexing.py +++ b/frappe/website/doctype/website_settings/google_indexing.py @@ -33,10 +33,11 @@ def authorize_access(reauthorize=False, code=None): }, ) - frappe.db.set_value("Website Settings", None, "indexing_authorization_code", oauth_code) res = oauth_obj.authorize(oauth_code, get_request_site_address(True)) frappe.db.set_value( - "Website Settings", "Website Settings", "indexing_refresh_token", res.get("refresh_token") + "Website Settings", + "Website Settings", + {"indexing_authorization_code": oauth_code, "indexing_refresh_token": res.get("refresh_token")}, ) From 84ad7b74b950c69f419724eec54eeed78f2c422d Mon Sep 17 00:00:00 2001 From: phot0n Date: Thu, 30 Jun 2022 16:50:30 +0530 Subject: [PATCH 60/80] fix: don't show authorize api access button if the form is unsaved --- frappe/email/doctype/email_account/email_account.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frappe/email/doctype/email_account/email_account.json b/frappe/email/doctype/email_account/email_account.json index f76bacd044..e8540c90d9 100644 --- a/frappe/email/doctype/email_account/email_account.json +++ b/frappe/email/doctype/email_account/email_account.json @@ -590,7 +590,7 @@ "label": "Use OAuth" }, { - "depends_on": "eval: doc.service === \"GMail\" && doc.use_oauth && !doc.__islocal", + "depends_on": "eval: doc.service === \"GMail\" && doc.use_oauth && !doc.__islocal && !doc.__unsaved", "fieldname": "authorize_api_access", "fieldtype": "Button", "label": "Authorize API Access" From 431afaeee43d8dc8f50ab49bfb22610e0a80b533 Mon Sep 17 00:00:00 2001 From: phot0n Date: Thu, 30 Jun 2022 17:00:06 +0530 Subject: [PATCH 61/80] fix: remove commit * chore: raise not implemented error for services other than gmail * chore: use fstring for _auth_string property --- frappe/email/oauth.py | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/frappe/email/oauth.py b/frappe/email/oauth.py index 5793d84a13..d5e74afc9f 100644 --- a/frappe/email/oauth.py +++ b/frappe/email/oauth.py @@ -37,11 +37,6 @@ class Oauth: self._validate() def _validate(self) -> None: - if self.service != "GMail": - raise NotImplementedError( - f"Service {self.service} currently doesn't have oauth implementation." - ) - if not self._refresh_token: frappe.throw( frappe._("Please Authorize OAuth."), @@ -51,7 +46,7 @@ class Oauth: @property def _auth_string(self) -> str: - return "user=%s\1auth=Bearer %s\1\1" % (self.email, self._access_token) + return f"user={self.email}\1auth=Bearer {self._access_token}\1\1" def connect(self, _retry: int = 0) -> None: """Connection method with retry on exception for Oauth""" @@ -106,9 +101,8 @@ class Oauth: # set the new access token in db frappe.db.set_value( - "Email Account", self.email_account, "access_token", access_token, update_modified=True + "Email Account", self.email_account, "access_token", access_token, update_modified=False ) - frappe.db.commit() return access_token def _get_service_object(self): @@ -120,7 +114,7 @@ class Oauth: @frappe.whitelist(methods=["POST"]) -def oauth_access(email_account: str, service: str = None): +def oauth_access(email_account: str, service: str): """Used as a default endpoint/caller for all oauth services. Returns authorization url for redirection""" @@ -136,6 +130,8 @@ def oauth_access(email_account: str, service: str = None): if service == "GMail": return authorize_google_access(email_account, doctype) + raise NotImplementedError(f"Service {service} currently doesn't have oauth implementation.") + def authorize_google_access(email_account, doctype: str = "Email Account", code: str = None): """Facilitates google oauth for email. From 8b38fcb4383831b90725695045515b13d2a9d0f8 Mon Sep 17 00:00:00 2001 From: phot0n Date: Thu, 30 Jun 2022 17:12:25 +0530 Subject: [PATCH 62/80] chore: move getting site address to GoogleOAuth --- frappe/email/oauth.py | 6 ++---- .../doctype/google_contacts/google_contacts.py | 6 ++---- .../integrations/doctype/google_drive/google_drive.py | 7 +++---- frappe/integrations/google_oauth.py | 11 +++++------ .../doctype/website_settings/google_indexing.py | 6 ++---- 5 files changed, 14 insertions(+), 22 deletions(-) diff --git a/frappe/email/oauth.py b/frappe/email/oauth.py index d5e74afc9f..cebb370b1f 100644 --- a/frappe/email/oauth.py +++ b/frappe/email/oauth.py @@ -7,7 +7,6 @@ from urllib.parse import quote import frappe from frappe.integrations.google_oauth import GoogleOAuth -from frappe.utils import get_request_site_address class OAuthenticationError(Exception): @@ -142,15 +141,14 @@ def authorize_google_access(email_account, doctype: str = "Email Account", code: if not code: return oauth_obj.get_authentication_url( - get_request_site_address(True), - state={ + { "method": "frappe.email.oauth.authorize_google_access", "redirect": "/app/Form/{0}/{1}".format(quote(doctype), quote(email_account)), "email_account": email_account, }, ) - res = oauth_obj.authorize(code, get_request_site_address(True)) + res = oauth_obj.authorize(code) frappe.db.set_value( doctype, email_account, diff --git a/frappe/integrations/doctype/google_contacts/google_contacts.py b/frappe/integrations/doctype/google_contacts/google_contacts.py index 1b309a9e16..eb2a8af647 100644 --- a/frappe/integrations/doctype/google_contacts/google_contacts.py +++ b/frappe/integrations/doctype/google_contacts/google_contacts.py @@ -8,7 +8,6 @@ import frappe from frappe import _ from frappe.integrations.google_oauth import GoogleOAuth from frappe.model.document import Document -from frappe.utils import get_request_site_address class GoogleContacts(Document): @@ -43,15 +42,14 @@ def authorize_access(g_contact, reauthorize=False, code=None): if not oauth_code or reauthorize: return oauth_obj.get_authentication_url( - get_request_site_address(True), - state={ + { "method": "frappe.integrations.doctype.google_contacts.google_contacts.authorize_access", "g_contact": g_contact, "redirect": "/app/Form/Google%20Contacts/{}".format(g_contact), }, ) - r = oauth_obj.authorize(oauth_code, get_request_site_address(True)) + r = oauth_obj.authorize(oauth_code) frappe.db.set_value( "Google Contacts", g_contact, diff --git a/frappe/integrations/doctype/google_drive/google_drive.py b/frappe/integrations/doctype/google_drive/google_drive.py index 22b3759266..f4a6f1f46e 100644 --- a/frappe/integrations/doctype/google_drive/google_drive.py +++ b/frappe/integrations/doctype/google_drive/google_drive.py @@ -16,7 +16,7 @@ from frappe.integrations.offsite_backup_utils import ( validate_file_size, ) from frappe.model.document import Document -from frappe.utils import get_backups_path, get_bench_path, get_request_site_address +from frappe.utils import get_backups_path, get_bench_path from frappe.utils.background_jobs import enqueue from frappe.utils.backups import new_backup @@ -56,14 +56,13 @@ def authorize_access(reauthorize=False, code=None): if reauthorize: frappe.db.set_value("Google Drive", None, "backup_folder_id", "") return oauth_obj.get_authentication_url( - get_request_site_address(True), - state={ + { "method": "frappe.integrations.doctype.google_drive.google_drive.authorize_access", "redirect": "/app/Form/{0}".format(quote("Google Drive")), }, ) - r = oauth_obj.authorize(oauth_code, get_request_site_address(True)) + r = oauth_obj.authorize(oauth_code) frappe.db.set_value( "Google Drive", "Google Drive", diff --git a/frappe/integrations/google_oauth.py b/frappe/integrations/google_oauth.py index c920fc31fa..2bb1aaca1f 100644 --- a/frappe/integrations/google_oauth.py +++ b/frappe/integrations/google_oauth.py @@ -6,6 +6,7 @@ from googleapiclient.discovery import build from requests import get, post import frappe +from frappe.utils import get_request_site_address CALLBACK_METHOD = "/api/method/frappe.integrations.google_oauth.callback" _SCOPES = { @@ -47,7 +48,7 @@ class GoogleOAuth: if not (self.google_settings.client_id and self.google_settings.client_secret): frappe.throw(frappe._("Please update Google Settings before continuing.")) - def authorize(self, oauth_code: str, site_address: str) -> Dict[str, Union[str, int]]: + def authorize(self, oauth_code: str) -> Dict[str, Union[str, int]]: """Returns a dict with access and refresh token. :param oauth_code: code got back from google upon successful auhtorization @@ -62,7 +63,7 @@ class GoogleOAuth: ), "grant_type": "authorization_code", "scope": self.scopes, - "redirect_uri": site_address + CALLBACK_METHOD, + "redirect_uri": get_request_site_address(True) + CALLBACK_METHOD, } return handle_response( @@ -91,9 +92,7 @@ class GoogleOAuth: raise_err=True, ) - def get_authentication_url( - self, site_address: str, state: Dict[str, str] = None - ) -> Dict[str, str]: + def get_authentication_url(self, state: Dict[str, str]) -> Dict[str, str]: """Returns google authentication url. :param site_address: side address from which the request is being made (for redirect back to site) @@ -101,7 +100,7 @@ class GoogleOAuth: """ state = json.dumps(state) - callback_url = site_address + CALLBACK_METHOD + callback_url = get_request_site_address(True) + CALLBACK_METHOD return { "url": "https://accounts.google.com/o/oauth2/v2/auth?" diff --git a/frappe/website/doctype/website_settings/google_indexing.py b/frappe/website/doctype/website_settings/google_indexing.py index 3b36bb5360..3577f688f5 100644 --- a/frappe/website/doctype/website_settings/google_indexing.py +++ b/frappe/website/doctype/website_settings/google_indexing.py @@ -9,7 +9,6 @@ from googleapiclient.errors import HttpError import frappe from frappe import _ from frappe.integrations.google_oauth import GoogleOAuth -from frappe.utils import get_request_site_address @frappe.whitelist(methods=["POST"]) @@ -26,14 +25,13 @@ def authorize_access(reauthorize=False, code=None): if not oauth_code or reauthorize: return oauth_obj.get_authentication_url( - get_request_site_address(True), - state={ + { "method": "frappe.website.doctype.website_settings.google_indexing.authorize_access", "redirect": "/app/Form/{0}".format(quote("Website Settings")), }, ) - res = oauth_obj.authorize(oauth_code, get_request_site_address(True)) + res = oauth_obj.authorize(oauth_code) frappe.db.set_value( "Website Settings", "Website Settings", From 5bf26819a8d44dd3619f35aa8011104ee87f616d Mon Sep 17 00:00:00 2001 From: phot0n Date: Fri, 1 Jul 2022 11:00:06 +0530 Subject: [PATCH 63/80] fix: better/reduced exception handling for email oauth Since the places where connection methods are called already have a lot of exception handling, we can just raise and let them handle all the probable cases. --- frappe/email/oauth.py | 11 ++++++----- frappe/email/smtp.py | 2 +- 2 files changed, 7 insertions(+), 6 deletions(-) diff --git a/frappe/email/oauth.py b/frappe/email/oauth.py index cebb370b1f..9d456daf81 100644 --- a/frappe/email/oauth.py +++ b/frappe/email/oauth.py @@ -63,16 +63,17 @@ class Oauth: # SMTP self._connect_smtp() - except Exception: + except Exception as e: # maybe the access token expired - refreshing access_token = self._refresh_access_token() if not access_token or _retry > 0: - frappe.throw( - frappe._("Authentication Failed. Please Check and Update the credentials."), - OAuthenticationError, - frappe._("OAuth Error"), + frappe.log_error( + "OAuth Error - Authentication Failed", str(e), "Email Account", self.email_account ) + # raising a bare exception here as we have a lot of exception handling present + # where the connect method is called from - hence just logging and raising. + raise self._access_token = access_token self.connect(_retry + 1) diff --git a/frappe/email/smtp.py b/frappe/email/smtp.py index 687b8318b6..10eb2f7681 100644 --- a/frappe/email/smtp.py +++ b/frappe/email/smtp.py @@ -147,7 +147,7 @@ class SMTPServer: @classmethod def throw_invalid_credentials_exception(cls): frappe.throw( - _("Incorrect email or password. Please check your login credentials."), + _("Please check your email login credentials."), title=_("Invalid Credentials"), exc=InvalidEmailCredentials, ) From 2907571098e81875916542e902a559c09bf56bd3 Mon Sep 17 00:00:00 2001 From: phot0n Date: Mon, 11 Jul 2022 16:27:52 +0530 Subject: [PATCH 64/80] minor: encrypt email oauth refresh and access token --- .../doctype/email_account/email_account.json | 4 +-- .../doctype/email_account/email_account.py | 36 ++++++++++--------- frappe/email/oauth.py | 12 ++++--- 3 files changed, 29 insertions(+), 23 deletions(-) diff --git a/frappe/email/doctype/email_account/email_account.json b/frappe/email/doctype/email_account/email_account.json index e8540c90d9..bd4023d62b 100644 --- a/frappe/email/doctype/email_account/email_account.json +++ b/frappe/email/doctype/email_account/email_account.json @@ -597,7 +597,7 @@ }, { "fieldname": "refresh_token", - "fieldtype": "Data", + "fieldtype": "Small Text", "hidden": 1, "label": "Refresh Token", "read_only": 1 @@ -613,7 +613,7 @@ "icon": "fa fa-inbox", "index_web_pages_for_search": 1, "links": [], - "modified": "2022-06-01 17:51:37.878446", + "modified": "2022-07-11 15:11:15.196935", "modified_by": "Administrator", "module": "Email", "name": "Email Account", diff --git a/frappe/email/doctype/email_account/email_account.py b/frappe/email/doctype/email_account/email_account.py index 278aeb698b..d3273ab89d 100755 --- a/frappe/email/doctype/email_account/email_account.py +++ b/frappe/email/doctype/email_account/email_account.py @@ -20,6 +20,7 @@ from frappe.utils import cint, comma_or, cstr, parse_addr, validate_email_addres from frappe.utils.background_jobs import enqueue, get_jobs from frappe.utils.error import raise_error_on_no_output from frappe.utils.jinja import render_template +from frappe.utils.password import decrypt, encrypt from frappe.utils.user import get_system_managers @@ -92,11 +93,7 @@ class EmailAccount(Document): self.refresh_token = self.access_token = None if not frappe.local.flags.in_install and (self.use_oauth or not self.awaiting_password): - if ( - self.refresh_token - or self.password - or self.smtp_server in ("127.0.0.1", "localhost") - ): + if self.refresh_token or self.password or self.smtp_server in ("127.0.0.1", "localhost"): if self.enable_incoming: self.get_incoming_server() self.no_failed = 0 @@ -214,8 +211,8 @@ class EmailAccount(Document): "incoming_port": get_port(self), "initial_sync_count": self.initial_sync_count or 100, "use_oauth": self.use_oauth or 0, - "refresh_token": getattr(self, "refresh_token", None), - "access_token": getattr(self, "access_token", None), + "refresh_token": decrypt(self.refresh_token) if self.refresh_token else None, + "access_token": decrypt(self.access_token) if self.access_token else None, } ) @@ -411,7 +408,12 @@ class EmailAccount(Document): for doc_field_name, d in field_to_conf_name_map.items(): conf_names, default = d.get("conf_names") or [], d.get("default") value = [frappe.conf.get(k) for k in conf_names if frappe.conf.get(k)] - account_details[doc_field_name] = (value and value[0]) or default + + if doc_field_name in ("refresh_token", "access_token"): + account_details[doc_field_name] = value and encrypt(value[0]) + else: + account_details[doc_field_name] = (value and value[0]) or default + return account_details def sendmail_config(self): @@ -425,8 +427,8 @@ class EmailAccount(Document): "use_tls": cint(self.use_tls), "service": getattr(self, "service", ""), "use_oauth": self.use_oauth or 0, - "refresh_token": getattr(self, "refresh_token", None), - "access_token": getattr(self, "access_token", None), + "refresh_token": decrypt(self.refresh_token) if self.refresh_token else None, + "access_token": decrypt(self.access_token) if self.access_token else None, } def get_smtp_server(self): @@ -844,7 +846,9 @@ def get_max_email_uid(email_account): return max_uid -def setup_user_email_inbox(email_account, awaiting_password, email_id, enable_outgoing, used_oauth): +def setup_user_email_inbox( + email_account, awaiting_password, email_id, enable_outgoing, used_oauth +): """setup email inbox for user""" from frappe.core.doctype.user.user import ask_pass_update @@ -887,13 +891,11 @@ def setup_user_email_inbox(email_account, awaiting_password, email_id, enable_ou if update_user_email_settings: UserEmail = frappe.qb.DocType("User Email") - frappe.qb.update(UserEmail).set( - UserEmail.awaiting_password, (awaiting_password or 0) - ).set( + frappe.qb.update(UserEmail).set(UserEmail.awaiting_password, (awaiting_password or 0)).set( UserEmail.enable_outgoing, (enable_outgoing or 0) - ).set( - UserEmail.used_oauth, (used_oauth or 0) - ).where(UserEmail.email_account == email_account).run() + ).set(UserEmail.used_oauth, (used_oauth or 0)).where( + UserEmail.email_account == email_account + ).run() else: users = " and ".join([frappe.bold(user.get("name")) for user in user_names]) diff --git a/frappe/email/oauth.py b/frappe/email/oauth.py index 9d456daf81..cb9dcbbc85 100644 --- a/frappe/email/oauth.py +++ b/frappe/email/oauth.py @@ -7,6 +7,7 @@ from urllib.parse import quote import frappe from frappe.integrations.google_oauth import GoogleOAuth +from frappe.utils.password import encrypt class OAuthenticationError(Exception): @@ -16,7 +17,7 @@ class OAuthenticationError(Exception): class Oauth: def __init__( self, - conn: Union[IMAP4, POP3, SMTP], + conn: IMAP4 | POP3 | SMTP, email_account: str, email: str, access_token: str, @@ -81,7 +82,7 @@ class Oauth: def _connect_pop(self) -> bytes: # poplib doesn't have AUTH command implementation res = self._conn._shortcmd( - "AUTH {0} {1}".format( + "AUTH {} {}".format( self._mechanism, base64.b64encode(bytes(self._auth_string, "utf-8")).decode("utf-8") ) ) @@ -144,7 +145,7 @@ def authorize_google_access(email_account, doctype: str = "Email Account", code: return oauth_obj.get_authentication_url( { "method": "frappe.email.oauth.authorize_google_access", - "redirect": "/app/Form/{0}/{1}".format(quote(doctype), quote(email_account)), + "redirect": f"/app/Form/{quote(doctype)}/{quote(email_account)}", "email_account": email_account, }, ) @@ -153,6 +154,9 @@ def authorize_google_access(email_account, doctype: str = "Email Account", code: frappe.db.set_value( doctype, email_account, - {"refresh_token": res.get("refresh_token"), "access_token": res.get("access_token")}, + { + "refresh_token": encrypt(res.get("refresh_token")), + "access_token": encrypt(res.get("access_token")), + }, update_modified=False, ) From 2b7bd4eef08591e098068f4d08f6de8f6e8a00de Mon Sep 17 00:00:00 2001 From: phot0n Date: Mon, 11 Jul 2022 17:47:45 +0530 Subject: [PATCH 65/80] minor(ux): select field for alternating between basic and oauth authentication --- .../doctype/email_account/email_account.js | 17 +++++++++-- .../doctype/email_account/email_account.json | 25 +++++++++------- .../doctype/email_account/email_account.py | 29 +++++++++++-------- frappe/email/oauth.py | 5 ---- 4 files changed, 45 insertions(+), 31 deletions(-) diff --git a/frappe/email/doctype/email_account/email_account.js b/frappe/email/doctype/email_account/email_account.js index 5d65a8efe6..9fd9c1a0bf 100644 --- a/frappe/email/doctype/email_account/email_account.js +++ b/frappe/email/doctype/email_account/email_account.js @@ -93,6 +93,7 @@ frappe.ui.form.on("Email Account", { }); } frm.events.show_gmail_message_for_less_secure_apps(frm); + frm.events.toggle_auth_method(frm); }, use_imap: function(frm) { @@ -134,6 +135,7 @@ frappe.ui.form.on("Email Account", { frm.add_child("imap_folder", {"folder_name": "INBOX"}); frm.refresh_field("imap_folder"); } + frm.toggle_display(['auth_method'], frm.doc.service === "GMail"); }, refresh: function(frm) { @@ -150,11 +152,20 @@ frappe.ui.form.on("Email Account", { }, after_save(frm) { - if (frm.doc.use_oauth && !frm.doc.refresh_token) { + if (frm.doc.auth_method === "Oauth" && !frm.doc.refresh_token) { oauth_access(frm); } }, + toggle_auth_method: function(frm) { + if (frm.doc.service !== "GMail") { + frm.toggle_display(['auth_method'], false); + frm.doc.auth_method = "Basic"; + } else { + frm.toggle_display(['auth_method'], true); + } + }, + show_gmail_message_for_less_secure_apps: function(frm) { frm.dashboard.clear_headline(); let msg = __("GMail will only work if you enable 2-step authentication and use app-specific password OR use OAuth."); @@ -166,8 +177,8 @@ frappe.ui.form.on("Email Account", { }, show_oauth_authorization_message(frm) { - let msg = __("Oauth Enabled but not Authorized. Please use `Authorize API Access` Button to do the same."); - if (frm.doc.use_oauth && !frm.doc.refresh_token) { + if (frm.doc.auth_method === "Oauth" && !frm.doc.refresh_token) { + let msg = __("Oauth Enabled but not Authorized. Please use Authorize API Access Button to do the same."); frm.dashboard.clear_headline(); frm.dashboard.set_headline_alert(msg, "yellow"); } diff --git a/frappe/email/doctype/email_account/email_account.json b/frappe/email/doctype/email_account/email_account.json index bd4023d62b..2c42010897 100644 --- a/frappe/email/doctype/email_account/email_account.json +++ b/frappe/email/doctype/email_account/email_account.json @@ -14,12 +14,12 @@ "domain", "service", "authentication_column", + "auth_method", + "authorize_api_access", "password", "awaiting_password", "ascii_encode_password", "column_break_10", - "use_oauth", - "authorize_api_access", "refresh_token", "access_token", "login_id_is_different", @@ -103,6 +103,7 @@ "label": "Email Login ID" }, { + "depends_on": "eval: doc.auth_method === \"Basic\"", "fieldname": "password", "fieldtype": "Password", "hide_days": 1, @@ -111,6 +112,7 @@ }, { "default": "0", + "depends_on": "eval: doc.auth_method === \"Basic\"", "fieldname": "awaiting_password", "fieldtype": "Check", "hide_days": 1, @@ -119,6 +121,7 @@ }, { "default": "0", + "depends_on": "eval: doc.auth_method === \"Basic\"", "fieldname": "ascii_encode_password", "fieldtype": "Check", "hide_days": 1, @@ -583,14 +586,7 @@ "label": "IMAP Details" }, { - "default": "0", - "depends_on": "eval: doc.service === \"GMail\"", - "fieldname": "use_oauth", - "fieldtype": "Check", - "label": "Use OAuth" - }, - { - "depends_on": "eval: doc.service === \"GMail\" && doc.use_oauth && !doc.__islocal && !doc.__unsaved", + "depends_on": "eval: doc.service === \"GMail\" && doc.auth_method === \"Oauth\" && !doc.__islocal && !doc.__unsaved", "fieldname": "authorize_api_access", "fieldtype": "Button", "label": "Authorize API Access" @@ -608,12 +604,19 @@ "hidden": 1, "label": "Access Token", "read_only": 1 + }, + { + "default": "Basic", + "fieldname": "auth_method", + "fieldtype": "Select", + "label": "Method", + "options": "Basic\nOauth" } ], "icon": "fa fa-inbox", "index_web_pages_for_search": 1, "links": [], - "modified": "2022-07-11 15:11:15.196935", + "modified": "2022-07-11 17:29:13.651583", "modified_by": "Administrator", "module": "Email", "name": "Email Account", diff --git a/frappe/email/doctype/email_account/email_account.py b/frappe/email/doctype/email_account/email_account.py index d3273ab89d..a3d4b3ba32 100755 --- a/frappe/email/doctype/email_account/email_account.py +++ b/frappe/email/doctype/email_account/email_account.py @@ -81,10 +81,13 @@ class EmailAccount(Document): if frappe.local.flags.in_patch or frappe.local.flags.in_test: return - if getattr(self, "service", "") != "GMail" and self.use_oauth: - self.use_oauth = 0 + use_oauth = self.auth_method == "Oauth" - if self.use_oauth: + if getattr(self, "service", "") != "GMail" and use_oauth: + self.auth_method = "Basic" + use_oauth = False + + if use_oauth: # no need for awaiting password for oauth self.awaiting_password = 0 @@ -92,7 +95,7 @@ class EmailAccount(Document): # clear access & refresh token self.refresh_token = self.access_token = None - if not frappe.local.flags.in_install and (self.use_oauth or not self.awaiting_password): + if not frappe.local.flags.in_install and (use_oauth or not self.awaiting_password): if self.refresh_token or self.password or self.smtp_server in ("127.0.0.1", "localhost"): if self.enable_incoming: self.get_incoming_server() @@ -102,7 +105,7 @@ class EmailAccount(Document): self.validate_smtp_conn() else: if self.enable_incoming or (self.enable_outgoing and not self.no_smtp_authentication): - if not self.use_oauth: + if not use_oauth: frappe.throw(_("Password is required or select Awaiting Password")) if self.notify_if_unreplied: @@ -155,7 +158,7 @@ class EmailAccount(Document): awaiting_password=self.awaiting_password, email_id=self.email_id, enable_outgoing=self.enable_outgoing, - used_oauth=self.use_oauth, + used_oauth=self.auth_method == "Oauth", ) def there_must_be_only_one_default(self): @@ -210,7 +213,7 @@ class EmailAccount(Document): "email_sync_rule": email_sync_rule, "incoming_port": get_port(self), "initial_sync_count": self.initial_sync_count or 100, - "use_oauth": self.use_oauth or 0, + "use_oauth": self.auth_method == "Oauth", "refresh_token": decrypt(self.refresh_token) if self.refresh_token else None, "access_token": decrypt(self.access_token) if self.access_token else None, } @@ -279,7 +282,9 @@ class EmailAccount(Document): @property def _password(self): - raise_exception = not (self.use_oauth or self.no_smtp_authentication or frappe.flags.in_test) + raise_exception = not ( + self.auth_method == "Oauth" or self.no_smtp_authentication or frappe.flags.in_test + ) return self.get_password(raise_exception=raise_exception) @property @@ -398,7 +403,7 @@ class EmailAccount(Document): "default": 0, }, "name": {"conf_names": ("email_sender_name",), "default": "Frappe"}, - "use_oauth": {"conf_names": ("use_oauth"), "default": 0}, + "auth_method": {"conf_names": ("auth_method"), "default": "Basic"}, "access_token": {"conf_names": ("mail_access_token")}, "refresh_token": {"conf_names": ("mail_refresh_token")}, "from_site_config": {"default": True}, @@ -426,7 +431,7 @@ class EmailAccount(Document): "use_ssl": cint(self.use_ssl_for_outgoing), "use_tls": cint(self.use_tls), "service": getattr(self, "service", ""), - "use_oauth": self.use_oauth or 0, + "use_oauth": self.auth_method == "Oauth", "refresh_token": decrypt(self.refresh_token) if self.refresh_token else None, "access_token": decrypt(self.access_token) if self.access_token else None, } @@ -800,7 +805,7 @@ def pull(now=False): for email_account in frappe.get_list( "Email Account", filters={"enable_incoming": 1}, - or_filters={"awaiting_password": 0, "use_oauth": 1}, + or_filters={"awaiting_password": 0, "auth_method": "Oauth"}, ): if now: pull_from_email_account(email_account.name) @@ -923,7 +928,7 @@ def remove_user_email_inbox(email_account): @frappe.whitelist() def set_email_password(email_account, password): account = frappe.get_doc("Email Account", email_account) - if account.awaiting_password and not account.use_oauth: + if account.awaiting_password and not account.auth_method == "Oauth": account.awaiting_password = 0 account.password = password try: diff --git a/frappe/email/oauth.py b/frappe/email/oauth.py index cb9dcbbc85..140da6afed 100644 --- a/frappe/email/oauth.py +++ b/frappe/email/oauth.py @@ -2,7 +2,6 @@ import base64 from imaplib import IMAP4 from poplib import POP3 from smtplib import SMTP -from typing import Union from urllib.parse import quote import frappe @@ -124,10 +123,6 @@ def oauth_access(email_account: str, service: str): doctype = "Email Account" - # NOTE: setting this here, since we redirect to the service's auth page, - # we lose the use_oauth value in the emal account form - frappe.db.set_value(doctype, email_account, "use_oauth", 1, update_modified=False) - if service == "GMail": return authorize_google_access(email_account, doctype) From a826f4cc5320d62f782c8d2c5f0c15d8bb05909c Mon Sep 17 00:00:00 2001 From: phot0n Date: Mon, 11 Jul 2022 17:51:56 +0530 Subject: [PATCH 66/80] fix(ux): consistent field names for ssl and server * keep server and port together --- .../email/doctype/email_account/email_account.js | 4 ++++ .../email/doctype/email_account/email_account.json | 14 +++++++------- frappe/email/oauth.py | 5 +++++ 3 files changed, 16 insertions(+), 7 deletions(-) diff --git a/frappe/email/doctype/email_account/email_account.js b/frappe/email/doctype/email_account/email_account.js index 9fd9c1a0bf..46c3742fde 100644 --- a/frappe/email/doctype/email_account/email_account.js +++ b/frappe/email/doctype/email_account/email_account.js @@ -145,6 +145,10 @@ frappe.ui.form.on("Email Account", { frm.events.show_gmail_message_for_less_secure_apps(frm); frm.events.show_oauth_authorization_message(frm); + if (frm.doc.service !== "Gmail") { + frm.doc.auth_method = "Basic"; + } + if (frappe.route_flags.delete_user_from_locals && frappe.route_flags.linked_user) { delete frappe.route_flags.delete_user_from_locals; delete locals['User'][frappe.route_flags.linked_user]; diff --git a/frappe/email/doctype/email_account/email_account.json b/frappe/email/doctype/email_account/email_account.json index 2c42010897..feb1a998f6 100644 --- a/frappe/email/doctype/email_account/email_account.json +++ b/frappe/email/doctype/email_account/email_account.json @@ -48,9 +48,9 @@ "send_notification_to", "outgoing_mail_settings", "enable_outgoing", - "smtp_server", "use_tls", "use_ssl_for_outgoing", + "smtp_server", "smtp_port", "column_break_38", "default_outgoing", @@ -92,7 +92,7 @@ "fieldtype": "Check", "hide_days": 1, "hide_seconds": 1, - "label": "Use different login" + "label": "Use different Email ID" }, { "depends_on": "login_id_is_different", @@ -100,7 +100,7 @@ "fieldtype": "Data", "hide_days": 1, "hide_seconds": 1, - "label": "Email Login ID" + "label": "Alternative Email ID" }, { "depends_on": "eval: doc.auth_method === \"Basic\"", @@ -190,7 +190,7 @@ "fieldtype": "Data", "hide_days": 1, "hide_seconds": 1, - "label": "Email Server" + "label": "Incoming Server" }, { "default": "0", @@ -312,7 +312,7 @@ "fieldtype": "Data", "hide_days": 1, "hide_seconds": 1, - "label": "SMTP Server" + "label": "Outgoing Server" }, { "default": "0", @@ -532,7 +532,7 @@ "fieldtype": "Check", "hide_days": 1, "hide_seconds": 1, - "label": "Use SSL for Outgoing" + "label": "Use SSL" }, { "default": "1", @@ -616,7 +616,7 @@ "icon": "fa fa-inbox", "index_web_pages_for_search": 1, "links": [], - "modified": "2022-07-11 17:29:13.651583", + "modified": "2022-07-11 18:34:06.945668", "modified_by": "Administrator", "module": "Email", "name": "Email Account", diff --git a/frappe/email/oauth.py b/frappe/email/oauth.py index 140da6afed..74708bf2ec 100644 --- a/frappe/email/oauth.py +++ b/frappe/email/oauth.py @@ -36,6 +36,11 @@ class Oauth: self._validate() def _validate(self) -> None: + if self.service != "GMail": + raise NotImplementedError( + f"Service {self.service} currently doesn't have oauth implementation." + ) + if not self._refresh_token: frappe.throw( frappe._("Please Authorize OAuth."), From da33f6e6d8102a26ca92ee33021f97165c4f7caf Mon Sep 17 00:00:00 2001 From: phot0n Date: Mon, 11 Jul 2022 18:08:19 +0530 Subject: [PATCH 67/80] fix: remove fetch-from from user email for used_oauth --- frappe/core/doctype/user_email/user_email.json | 1 - 1 file changed, 1 deletion(-) diff --git a/frappe/core/doctype/user_email/user_email.json b/frappe/core/doctype/user_email/user_email.json index 43dc3ace8d..6e3f813035 100644 --- a/frappe/core/doctype/user_email/user_email.json +++ b/frappe/core/doctype/user_email/user_email.json @@ -52,7 +52,6 @@ }, { "default": "0", - "fetch_from": "email_account.use_oauth", "fieldname": "used_oauth", "fieldtype": "Check", "in_list_view": 1, From 5ea642ef7fe23a502d88a69954ee751632c4f776 Mon Sep 17 00:00:00 2001 From: phot0n Date: Tue, 12 Jul 2022 15:11:51 +0530 Subject: [PATCH 68/80] fix(ux): update form header upon successful authorization * minor: simplified validations for email account --- frappe/email/doctype/email_account/email_account.js | 13 ++++++------- frappe/email/doctype/email_account/email_account.py | 13 +++++++------ 2 files changed, 13 insertions(+), 13 deletions(-) diff --git a/frappe/email/doctype/email_account/email_account.js b/frappe/email/doctype/email_account/email_account.js index 46c3742fde..9ce72b055c 100644 --- a/frappe/email/doctype/email_account/email_account.js +++ b/frappe/email/doctype/email_account/email_account.js @@ -145,10 +145,6 @@ frappe.ui.form.on("Email Account", { frm.events.show_gmail_message_for_less_secure_apps(frm); frm.events.show_oauth_authorization_message(frm); - if (frm.doc.service !== "Gmail") { - frm.doc.auth_method = "Basic"; - } - if (frappe.route_flags.delete_user_from_locals && frappe.route_flags.linked_user) { delete frappe.route_flags.delete_user_from_locals; delete locals['User'][frappe.route_flags.linked_user]; @@ -181,10 +177,13 @@ frappe.ui.form.on("Email Account", { }, show_oauth_authorization_message(frm) { - if (frm.doc.auth_method === "Oauth" && !frm.doc.refresh_token) { - let msg = __("Oauth Enabled but not Authorized. Please use Authorize API Access Button to do the same."); + if (frm.doc.auth_method === "Oauth") { + let msg = { + message: !frm.doc.refresh_token ? "Oauth Enabled but not Authorized. Please use Authorize API Access Button to do the same." : "Oauth Authorized. Re-Authorization can be done using Authorize API Access Button.", + indicator: !frm.doc.refresh_token ? "yellow" : "green" + } frm.dashboard.clear_headline(); - frm.dashboard.set_headline_alert(msg, "yellow"); + frm.dashboard.set_headline_alert(__(msg.message), msg.indicator); } }, diff --git a/frappe/email/doctype/email_account/email_account.py b/frappe/email/doctype/email_account/email_account.py index a3d4b3ba32..8f2107e165 100755 --- a/frappe/email/doctype/email_account/email_account.py +++ b/frappe/email/doctype/email_account/email_account.py @@ -95,7 +95,7 @@ class EmailAccount(Document): # clear access & refresh token self.refresh_token = self.access_token = None - if not frappe.local.flags.in_install and (use_oauth or not self.awaiting_password): + if not frappe.local.flags.in_install and not self.awaiting_password: if self.refresh_token or self.password or self.smtp_server in ("127.0.0.1", "localhost"): if self.enable_incoming: self.get_incoming_server() @@ -114,11 +114,12 @@ class EmailAccount(Document): for e in self.get_unreplied_notification_emails(): validate_email_address(e, True) - for folder in self.imap_folder: - if self.enable_incoming and folder.append_to: - valid_doctypes = [d[0] for d in get_append_to()] - if folder.append_to not in valid_doctypes: - frappe.throw(_("Append To can be one of {0}").format(comma_or(valid_doctypes))) + if self.enable_incoming: + for folder in self.imap_folder: + if folder.append_to: + valid_doctypes = [d[0] for d in get_append_to()] + if folder.append_to not in valid_doctypes: + frappe.throw(_("Append To can be one of {0}").format(comma_or(valid_doctypes))) def validate_smtp_conn(self): if not self.smtp_server: From a50568596acaa5a1820c5ff093784c531483c5d2 Mon Sep 17 00:00:00 2001 From: phot0n Date: Tue, 12 Jul 2022 16:22:45 +0530 Subject: [PATCH 69/80] minor: fetch attachment size from get_max_file_size api --- .../email/doctype/email_account/email_account.js | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/frappe/email/doctype/email_account/email_account.js b/frappe/email/doctype/email_account/email_account.js index 9ce72b055c..eabc8c19aa 100644 --- a/frappe/email/doctype/email_account/email_account.js +++ b/frappe/email/doctype/email_account/email_account.js @@ -82,6 +82,19 @@ function oauth_access(frm) { }); } +function set_default_max_attachment_size(frm, field) { + if (frm.doc.__islocal && !frm.doc[field]) { + frappe.call({ + method: "frappe.core.api.file.get_max_file_size", + callback: function(r) { + if (!r.exc) { + frm.set_value(field, Number(r.message)/(1024*1024)); + } + }, + }); + } +} + frappe.ui.form.on("Email Account", { service: function(frm) { $.each(frappe.email_defaults[frm.doc.service], function(key, value) { @@ -136,6 +149,7 @@ frappe.ui.form.on("Email Account", { frm.refresh_field("imap_folder"); } frm.toggle_display(['auth_method'], frm.doc.service === "GMail"); + set_default_max_attachment_size(frm, "attachment_limit"); }, refresh: function(frm) { From 81bb9c411ad384163fa0200a04e409d631eb1c2a Mon Sep 17 00:00:00 2001 From: phot0n Date: Tue, 12 Jul 2022 16:50:02 +0530 Subject: [PATCH 70/80] chore: fix sider --- frappe/email/doctype/email_account/email_account.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frappe/email/doctype/email_account/email_account.js b/frappe/email/doctype/email_account/email_account.js index eabc8c19aa..f86ec12344 100644 --- a/frappe/email/doctype/email_account/email_account.js +++ b/frappe/email/doctype/email_account/email_account.js @@ -195,7 +195,7 @@ frappe.ui.form.on("Email Account", { let msg = { message: !frm.doc.refresh_token ? "Oauth Enabled but not Authorized. Please use Authorize API Access Button to do the same." : "Oauth Authorized. Re-Authorization can be done using Authorize API Access Button.", indicator: !frm.doc.refresh_token ? "yellow" : "green" - } + }; frm.dashboard.clear_headline(); frm.dashboard.set_headline_alert(__(msg.message), msg.indicator); } From d1a199258daa62d2a79d99ccde23e6013b14302d Mon Sep 17 00:00:00 2001 From: phot0n Date: Tue, 12 Jul 2022 17:47:47 +0530 Subject: [PATCH 71/80] fix: pull from accounts for oauth whose refresh_token is not null * chore: rename Oauth to OAuth --- .../doctype/email_account/email_account.js | 6 ++-- .../doctype/email_account/email_account.json | 4 +-- .../doctype/email_account/email_account.py | 31 ++++++++++++------- 3 files changed, 24 insertions(+), 17 deletions(-) diff --git a/frappe/email/doctype/email_account/email_account.js b/frappe/email/doctype/email_account/email_account.js index f86ec12344..34d6e6acd8 100644 --- a/frappe/email/doctype/email_account/email_account.js +++ b/frappe/email/doctype/email_account/email_account.js @@ -166,7 +166,7 @@ frappe.ui.form.on("Email Account", { }, after_save(frm) { - if (frm.doc.auth_method === "Oauth" && !frm.doc.refresh_token) { + if (frm.doc.auth_method === "OAuth" && !frm.doc.refresh_token) { oauth_access(frm); } }, @@ -191,9 +191,9 @@ frappe.ui.form.on("Email Account", { }, show_oauth_authorization_message(frm) { - if (frm.doc.auth_method === "Oauth") { + if (frm.doc.auth_method === "OAuth") { let msg = { - message: !frm.doc.refresh_token ? "Oauth Enabled but not Authorized. Please use Authorize API Access Button to do the same." : "Oauth Authorized. Re-Authorization can be done using Authorize API Access Button.", + message: !frm.doc.refresh_token ? "OAuth Enabled but not Authorized. Please use Authorize API Access Button to do the same." : "OAuth Authorized. Re-Authorization can be done using Authorize API Access Button.", indicator: !frm.doc.refresh_token ? "yellow" : "green" }; frm.dashboard.clear_headline(); diff --git a/frappe/email/doctype/email_account/email_account.json b/frappe/email/doctype/email_account/email_account.json index feb1a998f6..ecb5af7378 100644 --- a/frappe/email/doctype/email_account/email_account.json +++ b/frappe/email/doctype/email_account/email_account.json @@ -586,7 +586,7 @@ "label": "IMAP Details" }, { - "depends_on": "eval: doc.service === \"GMail\" && doc.auth_method === \"Oauth\" && !doc.__islocal && !doc.__unsaved", + "depends_on": "eval: doc.service === \"GMail\" && doc.auth_method === \"OAuth\" && !doc.__islocal && !doc.__unsaved", "fieldname": "authorize_api_access", "fieldtype": "Button", "label": "Authorize API Access" @@ -610,7 +610,7 @@ "fieldname": "auth_method", "fieldtype": "Select", "label": "Method", - "options": "Basic\nOauth" + "options": "Basic\nOAuth" } ], "icon": "fa fa-inbox", diff --git a/frappe/email/doctype/email_account/email_account.py b/frappe/email/doctype/email_account/email_account.py index 8f2107e165..589ddf42f0 100755 --- a/frappe/email/doctype/email_account/email_account.py +++ b/frappe/email/doctype/email_account/email_account.py @@ -81,7 +81,7 @@ class EmailAccount(Document): if frappe.local.flags.in_patch or frappe.local.flags.in_test: return - use_oauth = self.auth_method == "Oauth" + use_oauth = self.auth_method == "OAuth" if getattr(self, "service", "") != "GMail" and use_oauth: self.auth_method = "Basic" @@ -159,7 +159,7 @@ class EmailAccount(Document): awaiting_password=self.awaiting_password, email_id=self.email_id, enable_outgoing=self.enable_outgoing, - used_oauth=self.auth_method == "Oauth", + used_oauth=self.auth_method == "OAuth", ) def there_must_be_only_one_default(self): @@ -214,7 +214,7 @@ class EmailAccount(Document): "email_sync_rule": email_sync_rule, "incoming_port": get_port(self), "initial_sync_count": self.initial_sync_count or 100, - "use_oauth": self.auth_method == "Oauth", + "use_oauth": self.auth_method == "OAuth", "refresh_token": decrypt(self.refresh_token) if self.refresh_token else None, "access_token": decrypt(self.access_token) if self.access_token else None, } @@ -284,7 +284,7 @@ class EmailAccount(Document): @property def _password(self): raise_exception = not ( - self.auth_method == "Oauth" or self.no_smtp_authentication or frappe.flags.in_test + self.auth_method == "OAuth" or self.no_smtp_authentication or frappe.flags.in_test ) return self.get_password(raise_exception=raise_exception) @@ -432,7 +432,7 @@ class EmailAccount(Document): "use_ssl": cint(self.use_ssl_for_outgoing), "use_tls": cint(self.use_tls), "service": getattr(self, "service", ""), - "use_oauth": self.auth_method == "Oauth", + "use_oauth": self.auth_method == "OAuth", "refresh_token": decrypt(self.refresh_token) if self.refresh_token else None, "access_token": decrypt(self.access_token) if self.access_token else None, } @@ -802,12 +802,18 @@ def pull(now=False): else: return - queued_jobs = get_jobs(site=frappe.local.site, key="job_name")[frappe.local.site] - for email_account in frappe.get_list( - "Email Account", - filters={"enable_incoming": 1}, - or_filters={"awaiting_password": 0, "auth_method": "Oauth"}, - ): + doctype = frappe.qb.DocType("Email Account") + email_accounts = ( + frappe.qb.from_(doctype) + .select(doctype.name) + .where(doctype.enable_incoming == 1) + .where( + (doctype.awaiting_password == 0) + | ((doctype.auth_method == "OAuth") & (doctype.refresh_token.isnotnull())) + ) + .run(as_dict=1) + ) + for email_account in email_accounts: if now: pull_from_email_account(email_account.name) @@ -815,6 +821,7 @@ def pull(now=False): # job_name is used to prevent duplicates in queue job_name = f"pull_from_email_account|{email_account.name}" + queued_jobs = get_jobs(site=frappe.local.site, key="job_name")[frappe.local.site] if job_name not in queued_jobs: enqueue( pull_from_email_account, @@ -929,7 +936,7 @@ def remove_user_email_inbox(email_account): @frappe.whitelist() def set_email_password(email_account, password): account = frappe.get_doc("Email Account", email_account) - if account.awaiting_password and not account.auth_method == "Oauth": + if account.awaiting_password and not account.auth_method == "OAuth": account.awaiting_password = 0 account.password = password try: From 5b7d37477ee64e0012a2d59b04a3500903f6ea69 Mon Sep 17 00:00:00 2001 From: phot0n Date: Tue, 12 Jul 2022 17:48:49 +0530 Subject: [PATCH 72/80] chore: add link for google settings when throwing error --- frappe/integrations/google_oauth.py | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/frappe/integrations/google_oauth.py b/frappe/integrations/google_oauth.py index 2bb1aaca1f..9ae4461cc1 100644 --- a/frappe/integrations/google_oauth.py +++ b/frappe/integrations/google_oauth.py @@ -42,13 +42,15 @@ class GoogleOAuth: self.validate_google_settings() def validate_google_settings(self): + google_settings = "Google Settings" + if not self.google_settings.enable: - frappe.throw(frappe._("Please enable Google Settings before continuing.")) + frappe.throw(frappe._("Please enable {} before continuing.").format(google_settings)) if not (self.google_settings.client_id and self.google_settings.client_secret): - frappe.throw(frappe._("Please update Google Settings before continuing.")) + frappe.throw(frappe._("Please update {} before continuing.").format(google_settings)) - def authorize(self, oauth_code: str) -> Dict[str, Union[str, int]]: + def authorize(self, oauth_code: str) -> dict[str, str | int]: """Returns a dict with access and refresh token. :param oauth_code: code got back from google upon successful auhtorization @@ -72,7 +74,7 @@ class GoogleOAuth: "Something went wrong during the authorization.", ) - def refresh_access_token(self, refresh_token: str) -> Dict[str, Union[str, int]]: + def refresh_access_token(self, refresh_token: str) -> dict[str, str | int]: """Refreshes google access token using refresh token""" data = { @@ -92,7 +94,7 @@ class GoogleOAuth: raise_err=True, ) - def get_authentication_url(self, state: Dict[str, str]) -> Dict[str, str]: + def get_authentication_url(self, state: dict[str, str]) -> dict[str, str]: """Returns google authentication url. :param site_address: side address from which the request is being made (for redirect back to site) @@ -105,7 +107,7 @@ class GoogleOAuth: return { "url": "https://accounts.google.com/o/oauth2/v2/auth?" + "access_type=offline&response_type=code&prompt=consent&include_granted_scopes=true&" - + "client_id={0}&scope={1}&redirect_uri={2}&state={3}".format( + + "client_id={}&scope={}&redirect_uri={}&state={}".format( self.google_settings.client_id, self.scopes, callback_url, state ) } @@ -133,7 +135,7 @@ class GoogleOAuth: def handle_response( - response: Dict[str, Union[str, int]], + response: dict[str, str | int], error_title: str, error_message: str, raise_err: bool = False, From 01a1860491201bb5d942a0a175fae4281cf3e5a6 Mon Sep 17 00:00:00 2001 From: phot0n Date: Wed, 13 Jul 2022 11:31:11 +0530 Subject: [PATCH 73/80] fix(ux): better ux for successful oauthorization --- .../doctype/email_account/email_account.js | 18 ++++++++++++------ frappe/email/oauth.py | 2 ++ frappe/integrations/google_oauth.py | 5 ++++- 3 files changed, 18 insertions(+), 7 deletions(-) diff --git a/frappe/email/doctype/email_account/email_account.js b/frappe/email/doctype/email_account/email_account.js index 34d6e6acd8..093c2a3820 100644 --- a/frappe/email/doctype/email_account/email_account.js +++ b/frappe/email/doctype/email_account/email_account.js @@ -137,6 +137,15 @@ frappe.ui.form.on("Email Account", { }, onload: function(frm) { + let oauth_authorization_state = frappe.utils.get_query_params().successful_authorization + if (oauth_authorization_state === '1') { + frappe.show_alert("Successfully Authorized"); + // FIXME: find better alternative + window.history.replaceState(null, "", window.location.pathname); + } else if (oauth_authorization_state === '0') { + window.history.replaceState(null, "", window.location.pathname); + } + frm.set_df_property("append_to", "only_select", true); frm.set_query("append_to", "frappe.email.doctype.email_account.email_account.get_append_to"); frm.set_query("append_to", "imap_folder", function() { @@ -191,13 +200,10 @@ frappe.ui.form.on("Email Account", { }, show_oauth_authorization_message(frm) { - if (frm.doc.auth_method === "OAuth") { - let msg = { - message: !frm.doc.refresh_token ? "OAuth Enabled but not Authorized. Please use Authorize API Access Button to do the same." : "OAuth Authorized. Re-Authorization can be done using Authorize API Access Button.", - indicator: !frm.doc.refresh_token ? "yellow" : "green" - }; + if (frm.doc.auth_method === "OAuth" && !frm.doc.refresh_token) { + let msg = __('OAuth has been enabled but not authorised. Please use "Authorise API Access" button to do the same.') frm.dashboard.clear_headline(); - frm.dashboard.set_headline_alert(__(msg.message), msg.indicator); + frm.dashboard.set_headline_alert(msg, "yellow"); } }, diff --git a/frappe/email/oauth.py b/frappe/email/oauth.py index 74708bf2ec..8cf7a42ab5 100644 --- a/frappe/email/oauth.py +++ b/frappe/email/oauth.py @@ -146,6 +146,8 @@ def authorize_google_access(email_account, doctype: str = "Email Account", code: { "method": "frappe.email.oauth.authorize_google_access", "redirect": f"/app/Form/{quote(doctype)}/{quote(email_account)}", + "success_query_param": "successful_authorization=1", + "failure_query_param": "successful_authorization=0", "email_account": email_account, }, ) diff --git a/frappe/integrations/google_oauth.py b/frappe/integrations/google_oauth.py index 9ae4461cc1..f88a47ba6a 100644 --- a/frappe/integrations/google_oauth.py +++ b/frappe/integrations/google_oauth.py @@ -1,5 +1,4 @@ import json -from typing import Dict, Union from google.oauth2.credentials import Credentials from googleapiclient.discovery import build @@ -172,6 +171,8 @@ def callback(state: str, code: str = None, error: str = None) -> None: state = json.loads(state) redirect = state.pop("redirect", "/app") + success_query_param = state.pop("success_query_param", "") + failure_query_param = state.pop("failure_query_param", "") if not error: state.update({"code": code}) @@ -180,5 +181,7 @@ def callback(state: str, code: str = None, error: str = None) -> None: # GET request, hence using commit to persist changes frappe.db.commit() + redirect = f"{redirect}?{failure_query_param if error else success_query_param}" + frappe.local.response["type"] = "redirect" frappe.local.response["location"] = redirect From 31c5f260d7504d8c2bd151d98f3331b78806f016 Mon Sep 17 00:00:00 2001 From: phot0n Date: Wed, 13 Jul 2022 12:29:25 +0530 Subject: [PATCH 74/80] chore: use f-strings --- .../integrations/doctype/google_contacts/google_contacts.py | 4 +++- frappe/integrations/doctype/google_drive/google_drive.py | 2 +- frappe/website/doctype/website_settings/google_indexing.py | 2 +- 3 files changed, 5 insertions(+), 3 deletions(-) diff --git a/frappe/integrations/doctype/google_contacts/google_contacts.py b/frappe/integrations/doctype/google_contacts/google_contacts.py index eb2a8af647..c1f445b599 100644 --- a/frappe/integrations/doctype/google_contacts/google_contacts.py +++ b/frappe/integrations/doctype/google_contacts/google_contacts.py @@ -2,6 +2,8 @@ # License: MIT. See LICENSE +from urllib.parse import quote + from googleapiclient.errors import HttpError import frappe @@ -45,7 +47,7 @@ def authorize_access(g_contact, reauthorize=False, code=None): { "method": "frappe.integrations.doctype.google_contacts.google_contacts.authorize_access", "g_contact": g_contact, - "redirect": "/app/Form/Google%20Contacts/{}".format(g_contact), + "redirect": f"/app/Form/{quote('Google Contacts')}/{quote(g_contact)}", }, ) diff --git a/frappe/integrations/doctype/google_drive/google_drive.py b/frappe/integrations/doctype/google_drive/google_drive.py index f4a6f1f46e..62100ae7c5 100644 --- a/frappe/integrations/doctype/google_drive/google_drive.py +++ b/frappe/integrations/doctype/google_drive/google_drive.py @@ -58,7 +58,7 @@ def authorize_access(reauthorize=False, code=None): return oauth_obj.get_authentication_url( { "method": "frappe.integrations.doctype.google_drive.google_drive.authorize_access", - "redirect": "/app/Form/{0}".format(quote("Google Drive")), + "redirect": f"/app/Form/{quote('Google Drive')}", }, ) diff --git a/frappe/website/doctype/website_settings/google_indexing.py b/frappe/website/doctype/website_settings/google_indexing.py index 3577f688f5..4f67949f86 100644 --- a/frappe/website/doctype/website_settings/google_indexing.py +++ b/frappe/website/doctype/website_settings/google_indexing.py @@ -27,7 +27,7 @@ def authorize_access(reauthorize=False, code=None): return oauth_obj.get_authentication_url( { "method": "frappe.website.doctype.website_settings.google_indexing.authorize_access", - "redirect": "/app/Form/{0}".format(quote("Website Settings")), + "redirect": f"/app/Form/{quote('Website Settings')}", }, ) From e762fe9ce168ea7bb144ff08ad30fa0281b05627 Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Wed, 13 Jul 2022 12:29:41 +0530 Subject: [PATCH 75/80] test: clear filters on ToDo before running test (#17494) --- cypress/integration/form.js | 3 ++- cypress/integration/list_paging.js | 1 + cypress/integration/list_view.js | 2 ++ cypress/integration/timeline.js | 5 +++-- 4 files changed, 8 insertions(+), 3 deletions(-) diff --git a/cypress/integration/form.js b/cypress/integration/form.js index 4d50a5f66a..b395ff77b2 100644 --- a/cypress/integration/form.js +++ b/cypress/integration/form.js @@ -17,7 +17,8 @@ context('Form', () => { cy.get('.primary-action').click(); cy.wait('@form_save').its('response.statusCode').should('eq', 200); - cy.visit('/app/todo'); + cy.go_to_list('ToDo'); + cy.clear_filters() cy.get('.page-head').findByTitle('To Do').should('exist'); cy.get('.list-row').should('contain', 'this is a test todo'); }); diff --git a/cypress/integration/list_paging.js b/cypress/integration/list_paging.js index 4a59024a7b..0cf6f2e565 100644 --- a/cypress/integration/list_paging.js +++ b/cypress/integration/list_paging.js @@ -9,6 +9,7 @@ context('List Paging', () => { it('test load more with count selection buttons', () => { cy.visit('/app/todo/view/report'); + cy.clear_filters() cy.get('.list-paging-area .list-count').should('contain.text', '20 of'); cy.get('.list-paging-area .btn-more').click(); diff --git a/cypress/integration/list_view.js b/cypress/integration/list_view.js index 3e0d1c9d50..ee12b37638 100644 --- a/cypress/integration/list_view.js +++ b/cypress/integration/list_view.js @@ -9,6 +9,7 @@ context('List View', () => { it('Keep checkbox checked after Refresh', () => { cy.go_to_list('ToDo'); + cy.clear_filters() cy.get('.list-row-container .list-row-checkbox').click({ multiple: true, force: true }); cy.get('.actions-btn-group button').contains('Actions').should('be.visible'); cy.intercept('/api/method/frappe.desk.reportview.get').as('list-refresh'); @@ -21,6 +22,7 @@ context('List View', () => { it('enables "Actions" button', () => { const actions = ['Approve', 'Reject', 'Edit', 'Export', 'Assign To', 'Apply Assignment Rule', 'Add Tags', 'Print', 'Delete']; cy.go_to_list('ToDo'); + cy.clear_filters() cy.get('.list-row-container:contains("Pending") .list-row-checkbox').click({ multiple: true, force: true }); cy.get('.actions-btn-group button').contains('Actions').should('be.visible').click(); cy.get('.dropdown-menu li:visible .dropdown-item').should('have.length', 9).each((el, index) => { diff --git a/cypress/integration/timeline.js b/cypress/integration/timeline.js index cb4d43a96a..e7308fbaa7 100644 --- a/cypress/integration/timeline.js +++ b/cypress/integration/timeline.js @@ -12,7 +12,8 @@ context('Timeline', () => { cy.get('[data-fieldname="description"] .ql-editor.ql-blank').type('Test ToDo', {force: true}).wait(200); cy.get('.page-head .page-actions').findByRole('button', {name: 'Save'}).click(); - cy.visit('/app/todo'); + cy.go_to_list('ToDo'); + cy.clear_filters() cy.click_listview_row_item(0); //To check if the comment box is initially empty and tying some text into it @@ -79,4 +80,4 @@ context('Timeline', () => { cy.get('.page-actions .actions-btn-group [data-label="Delete"]').click(); cy.click_modal_primary_button('Yes'); }); -}); \ No newline at end of file +}); From 9090b0fe3a50cfb612e1d1a1dc0c3f3b118157d8 Mon Sep 17 00:00:00 2001 From: phot0n Date: Wed, 13 Jul 2022 12:41:32 +0530 Subject: [PATCH 76/80] chore: remove unnecessary failure_query_param for email oauth --- frappe/email/doctype/email_account/email_account.js | 5 +---- frappe/email/oauth.py | 1 - 2 files changed, 1 insertion(+), 5 deletions(-) diff --git a/frappe/email/doctype/email_account/email_account.js b/frappe/email/doctype/email_account/email_account.js index 093c2a3820..155c4775fb 100644 --- a/frappe/email/doctype/email_account/email_account.js +++ b/frappe/email/doctype/email_account/email_account.js @@ -137,13 +137,10 @@ frappe.ui.form.on("Email Account", { }, onload: function(frm) { - let oauth_authorization_state = frappe.utils.get_query_params().successful_authorization - if (oauth_authorization_state === '1') { + if (frappe.utils.get_query_params().successful_authorization === '1') { frappe.show_alert("Successfully Authorized"); // FIXME: find better alternative window.history.replaceState(null, "", window.location.pathname); - } else if (oauth_authorization_state === '0') { - window.history.replaceState(null, "", window.location.pathname); } frm.set_df_property("append_to", "only_select", true); diff --git a/frappe/email/oauth.py b/frappe/email/oauth.py index 8cf7a42ab5..46d1565275 100644 --- a/frappe/email/oauth.py +++ b/frappe/email/oauth.py @@ -147,7 +147,6 @@ def authorize_google_access(email_account, doctype: str = "Email Account", code: "method": "frappe.email.oauth.authorize_google_access", "redirect": f"/app/Form/{quote(doctype)}/{quote(email_account)}", "success_query_param": "successful_authorization=1", - "failure_query_param": "successful_authorization=0", "email_account": email_account, }, ) From 7d5262f5e0c6b9b1c6ae0d7a467982bbe26490b9 Mon Sep 17 00:00:00 2001 From: phot0n Date: Wed, 13 Jul 2022 12:50:04 +0530 Subject: [PATCH 77/80] chore: translate authorization message and add nosemgrep --- frappe/email/doctype/email_account/email_account.js | 2 +- frappe/integrations/google_oauth.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/frappe/email/doctype/email_account/email_account.js b/frappe/email/doctype/email_account/email_account.js index 155c4775fb..d22009963d 100644 --- a/frappe/email/doctype/email_account/email_account.js +++ b/frappe/email/doctype/email_account/email_account.js @@ -138,7 +138,7 @@ frappe.ui.form.on("Email Account", { onload: function(frm) { if (frappe.utils.get_query_params().successful_authorization === '1') { - frappe.show_alert("Successfully Authorized"); + frappe.show_alert(__("Successfully Authorized")); // FIXME: find better alternative window.history.replaceState(null, "", window.location.pathname); } diff --git a/frappe/integrations/google_oauth.py b/frappe/integrations/google_oauth.py index f88a47ba6a..edce63493e 100644 --- a/frappe/integrations/google_oauth.py +++ b/frappe/integrations/google_oauth.py @@ -179,7 +179,7 @@ def callback(state: str, code: str = None, error: str = None) -> None: frappe.get_attr(state.pop("method"))(**state) # GET request, hence using commit to persist changes - frappe.db.commit() + frappe.db.commit() # nosemgrep redirect = f"{redirect}?{failure_query_param if error else success_query_param}" From 843f241c136dac748c0c0d30d5df8e98316c363e Mon Sep 17 00:00:00 2001 From: Sagar Vora Date: Wed, 13 Jul 2022 09:41:37 +0000 Subject: [PATCH 78/80] fix: copy global defaults before updating to avoid cache mutation (#17497) Co-authored-by: Ankush Menat --- frappe/defaults.py | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/frappe/defaults.py b/frappe/defaults.py index d0a49ef692..02076b1fda 100644 --- a/frappe/defaults.py +++ b/frappe/defaults.py @@ -85,18 +85,19 @@ def get_user_permissions(user=None): def get_defaults(user=None): - globald = get_defaults_for() + global_defaults = get_defaults_for() if not user: user = frappe.session.user if frappe.session else "Guest" - if user: - userd = {} - userd.update(get_defaults_for(user)) - userd.update({"user": user, "owner": user}) - globald.update(userd) + if not user: + return global_defaults - return globald + defaults = global_defaults.copy() + defaults.update(get_defaults_for(user)) + defaults.update(user=user, owner=user) + + return defaults def clear_user_default(key, user=None): From 468a5c55a32f92763c68a64cf16141e17c1eb5bd Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Wed, 13 Jul 2022 18:08:20 +0530 Subject: [PATCH 79/80] ci: fix weird version check in cypress tests (#17499) --- frappe/commands/utils.py | 1 - 1 file changed, 1 deletion(-) diff --git a/frappe/commands/utils.py b/frappe/commands/utils.py index 1b63914030..585478c0ff 100644 --- a/frappe/commands/utils.py +++ b/frappe/commands/utils.py @@ -872,7 +872,6 @@ def run_ui_tests( and os.path.exists(real_events_plugin_path) and os.path.exists(testing_library_path) and os.path.exists(coverage_plugin_path) - and cint(subprocess.getoutput("npm view cypress version")[:1]) >= 6 ): # install cypress click.secho("Installing Cypress...", fg="yellow") From 2791066bb2f6801713734b1f21e633212efa2c81 Mon Sep 17 00:00:00 2001 From: uepselon <49870752+uepselon@users.noreply.github.com> Date: Wed, 13 Jul 2022 16:05:41 +0200 Subject: [PATCH 80/80] fix: allow System Manager to reset OTP secret * squashed: Change Admin based OTP reset to role based reset (System Manager) * fix: show `Reset OTP Secret` button only if applicable * chore: flatten code, use `only_for` API Co-authored-by: Leonard Goertz Co-authored-by: Sagar Vora --- frappe/core/doctype/user/user.js | 18 +++++----- frappe/twofactor.py | 58 +++++++++++++++++--------------- 2 files changed, 40 insertions(+), 36 deletions(-) diff --git a/frappe/core/doctype/user/user.js b/frappe/core/doctype/user/user.js index 001aae4da0..41b7e7fb38 100644 --- a/frappe/core/doctype/user/user.js +++ b/frappe/core/doctype/user/user.js @@ -173,14 +173,16 @@ frappe.ui.form.on('User', { }); } - frm.add_custom_button(__("Reset OTP Secret"), function() { - frappe.call({ - method: "frappe.twofactor.reset_otp_secret", - args: { - "user": frm.doc.name - } - }); - }, __("Password")); + if (frappe.session.user == doc.name || frappe.user.has_role("System Manager")) { + frm.add_custom_button(__("Reset OTP Secret"), function() { + frappe.call({ + method: "frappe.twofactor.reset_otp_secret", + args: { + "user": frm.doc.name + } + }); + }, __("Password")); + } frm.trigger('enabled'); diff --git a/frappe/twofactor.py b/frappe/twofactor.py index 55c27e2bac..26fc3ad619 100644 --- a/frappe/twofactor.py +++ b/frappe/twofactor.py @@ -461,33 +461,35 @@ def disable(): @frappe.whitelist() def reset_otp_secret(user): + if frappe.session.user != user: + frappe.only_for("System Manager", message=True) + otp_issuer = frappe.db.get_value("System Settings", "System Settings", "otp_issuer_name") user_email = frappe.db.get_value("User", user, "email") - if frappe.session.user in ["Administrator", user]: - frappe.defaults.clear_default(user + "_otplogin") - frappe.defaults.clear_default(user + "_otpsecret") - email_args = { - "recipients": user_email, - "sender": None, - "subject": _("OTP Secret Reset - {0}").format(otp_issuer or "Frappe Framework"), - "message": _( - "

Your OTP secret on {0} has been reset. If you did not perform this reset and did not request it, please contact your System Administrator immediately.

" - ).format(otp_issuer or "Frappe Framework"), - "delayed": False, - "retry": 3, - } - enqueue( - method=frappe.sendmail, - queue="short", - timeout=300, - event=None, - is_async=True, - job_name=None, - now=False, - **email_args, - ) - return frappe.msgprint( - _("OTP Secret has been reset. Re-registration will be required on next login.") - ) - else: - return frappe.throw(_("OTP secret can only be reset by the Administrator.")) + + frappe.defaults.clear_default(user + "_otplogin") + frappe.defaults.clear_default(user + "_otpsecret") + + email_args = { + "recipients": user_email, + "sender": None, + "subject": _("OTP Secret Reset - {0}").format(otp_issuer or "Frappe Framework"), + "message": _( + "

Your OTP secret on {0} has been reset. If you did not perform this reset and did not request it, please contact your System Administrator immediately.

" + ).format(otp_issuer or "Frappe Framework"), + "delayed": False, + "retry": 3, + } + + enqueue( + method=frappe.sendmail, + queue="short", + timeout=300, + event=None, + is_async=True, + job_name=None, + now=False, + **email_args, + ) + + frappe.msgprint(_("OTP Secret has been reset. Re-registration will be required on next login."))