betodealmeida commented on a change in pull request #19242:
URL: https://github.com/apache/superset/pull/19242#discussion_r829600607
##########
File path: superset/connectors/sqla/models.py
##########
@@ -1165,7 +1169,7 @@ def get_sqla_query( # pylint:
disable=too-many-arguments,too-many-locals,too-ma
# if groupby field equals a selected column
elif selected in columns_by_name:
outer = columns_by_name[selected].get_sqla_col()
- else:
+ elif validate_adhoc_subquery(selected):
Review comment:
Same here:
```suggestion
else:
validate_adhoc_subquery(selected)
```
##########
File path: superset/connectors/sqla/utils.py
##########
@@ -119,3 +120,29 @@ def get_virtual_table_metadata(dataset: "SqlaTable") ->
List[Dict[str, str]]:
except Exception as ex:
raise SupersetGenericDBErrorException(message=str(ex)) from ex
return cols
+
+
+def validate_adhoc_subquery(raw_sql: str) -> bool:
+ """
+ check adhoc SQL contains sub-queries or nested sub-queries with table
+ :param raw_sql: adhoc sql expression
+ :return True if sql contains no sub-queries or nested sub-queries with
table
+ :raise SupersetSecurityException if sql contains sub-queries or
+ nested sub-queries with table
+ """
+ # pylint: disable=import-outside-toplevel
+ from superset import is_feature_enabled
+
+ if is_feature_enabled("ALLOW_ADHOC_SUBQUERY"):
+ return True
+
+ for statement in sqlparse.parse(raw_sql):
+ if has_table_query(statement):
+ raise SupersetSecurityException(
+ SupersetError(
+
error_type=SupersetErrorType.ADHOC_SUBQUERY_NOT_ALLOWED_ERROR,
+ message=_("Custom SQL fields cannot contain sub-queries."),
+ level=ErrorLevel.ERROR,
+ )
+ )
+ return True
Review comment:
My suggestion here would be:
```suggestion
def validate_adhoc_subquery(raw_sql: str) -> None:
"""
Check if adhoc SQL contains sub-queries or nested sub-queries with table
:param raw_sql: adhoc sql expression
:raise SupersetSecurityException if sql contains sub-queries or
nested sub-queries with table
"""
# pylint: disable=import-outside-toplevel
from superset import is_feature_enabled
if is_feature_enabled("ALLOW_ADHOC_SUBQUERY"):
return
for statement in sqlparse.parse(raw_sql):
if has_table_query(statement):
raise SupersetSecurityException(
SupersetError(
error_type=SupersetErrorType.ADHOC_SUBQUERY_NOT_ALLOWED_ERROR,
message=_("Custom SQL fields cannot contain
sub-queries."),
level=ErrorLevel.ERROR,
)
)
return
```
##########
File path: superset/connectors/sqla/models.py
##########
@@ -1180,7 +1184,7 @@ def get_sqla_query( # pylint:
disable=too-many-arguments,too-many-locals,too-ma
for selected in columns:
select_exprs.append(
columns_by_name[selected].get_sqla_col()
- if selected in columns_by_name
+ if selected in columns_by_name and
validate_adhoc_subquery(selected)
Review comment:
This and line 1395 would also require some modification. Here something
like:
```python
for selected in columns:
validate_adhoc_subquery(selected)
selec_exprs.append(...)
```
##########
File path: superset/connectors/sqla/models.py
##########
@@ -906,9 +908,11 @@ def adhoc_column_to_sqla(
"""
label = utils.get_column_name(col)
expression = col["sqlExpression"]
+ sqla_metric = None
if template_processor and expression:
expression = template_processor.process_template(expression)
- sqla_metric = literal_column(expression)
+ if expression and validate_adhoc_subquery(expression):
+ sqla_metric = literal_column(expression)
Review comment:
Not sure if my explanation made sense, but my suggestion was to change
this to:
```suggestion
validate_adhoc_subquery(expression)
sqla_metric = literal_column(expression)
```
Because when I read the code `if expression and
validate_adhoc_subquery(expression):` I wonder "what happens when
`validate_adhoc_subquery(expression)` is false?", but that's a situation that
can never happen, because the function currently can only return `True` (or
raise an exception).
Having the function return `None` instead makes it easier to understand
what's going on where it's being used.
(Alternatively, you could modify the function to return `True` or `False`,
and raise the exception at the call site based on its return value. But because
you're calling it from multiple places I think it's not worth it.)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
