gabotorresruiz commented on code in PR #44338:
URL: https://github.com/apache/superset/pull/44338#discussion_r4048760144
##########
superset/mcp_service/chart/tool/get_chart_info.py:
##########
@@ -86,6 +91,125 @@ def _build_unsaved_chart_info(form_data_key: str) ->
ChartInfo | ChartError:
)
+def _get_explore_permalink(
+ permalink_key: str,
+) -> ExplorePermalinkValue | ChartError:
+ """Read an Explore permalink, enforcing the same access checks as Explore.
+
+ ``GetExplorePermalinkCommand`` checks access to the permalink's datasource
+ and, when it references a saved chart, to that chart.
+ """
+ from superset.commands.explore.permalink.get import
GetExplorePermalinkCommand
+
+ try:
+ value = GetExplorePermalinkCommand(permalink_key).run()
+ except (ForbiddenError, SupersetSecurityException):
+ # Tables raise ForbiddenError subclasses; SQL Lab queries go through
+ # security_manager.raise_for_access, which raises
+ # SupersetSecurityException.
+ return ChartError(
+ error="You do not have access to the chart or dataset in this
permalink.",
+ error_type="PermalinkAccessDenied",
+ )
+ except SupersetTemplateException as ex:
Review Comment:
Not a blocker, and a follow-up is fine.
`SupersetParseError` is a sibling of `SupersetSecurityException` under
`SupersetErrorException`, so it misses both clauses above and still escapes as
a raw `ToolError`. It comes out of the same `raise_for_access` you just
handled: that call parses the query SQL to find the tables it touches, so SQL
that `sqlglot` cannot parse raises this instead of `SupersetTemplateException`.
Repro on this branch, a SQL Lab query whose stored SQL is `SELECT FROM WHERE
((( ;;; )))`, read by a user without database access:
`ToolError: Error calling tool 'get_chart_info': Error parsing near 'WHERE'
at line 1:17`
Widening this clause covers it, with `SupersetParseError` added to the
`superset.exceptions` import:
```python
except (SupersetTemplateException, SupersetParseError) as ex:
```
The message would want to grow past "template error" then, since a parse
failure is not a template failure. I checked that this returns
`InvalidPermalink` for the case above and leaves all 21 tests green, and
`test_permalink_query_with_template_error` looks like the natural home for a
twin.
--
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]
