Copilot commented on code in PR #39303:
URL: https://github.com/apache/superset/pull/39303#discussion_r3190473252
##########
superset/views/datasource/views.py:
##########
@@ -223,6 +224,25 @@ def samples(self) -> FlaskResponse:
dashboard,
):
return json_error_response(_("Forbidden"), status=403)
+ else:
+ # Pre-fetch and access-check only for table-type datasources.
+ # Non-table types (query, saved_query) use a different access
model;
+ # passing them to raise_for_access(datasource=...) would check the
+ # wrong attributes. Let get_samples() handle the lookup for those
types.
+ if params["datasource_type"] == DatasourceType.TABLE:
+ try:
+ dataset = DatasourceDAO.get_datasource(
+ params["datasource_type"],
+ params["datasource_id"],
+ )
+ except (DatasourceNotFound, DatasourceTypeNotSupportedError):
+ return self.response_404()
+ try:
+ security_manager.raise_for_access(datasource=dataset)
+ except SupersetSecurityException:
+ return json_error_response(_("Forbidden"), status=403)
Review Comment:
This compares `params[\"datasource_type\"]` to `DatasourceType.TABLE`, but
`params[\"datasource_type\"]` is commonly a string (e.g. `'table'`). If it’s a
string here, this branch never executes, so the intended prefetch/access-check
and redundant-lookup avoidance won’t happen. Normalize the type before
comparing (e.g., compare to `DatasourceType.TABLE.value` or cast/convert
`params[\"datasource_type\"]` to the enum). Also consider using keyword args
for `DatasourceDAO.get_datasource(...)` to avoid signature/ordering issues and
keep consistent with the helper usage.
##########
superset/views/datasource/utils.py:
##########
@@ -24,7 +24,7 @@
from superset.common.query_context_factory import QueryContextFactory
from superset.common.utils.query_cache_manager import QueryCacheManager
from superset.constants import CacheRegion
-from superset.daos.datasource import DatasourceDAO
+from superset.daos.datasource import Datasource, DatasourceDAO
Review Comment:
Importing `Datasource` at runtime just for typing can introduce import
cycles and runtime failures if `Datasource` isn’t a concrete export in that
module. Prefer a typing-only import (via `typing.TYPE_CHECKING`) and/or
postponed evaluation of annotations, or type against a stable protocol/base
class to keep this helper robust.
##########
superset/commands/report/base.py:
##########
@@ -50,6 +53,34 @@ def run(self) -> Any:
def validate(self) -> None:
pass
+ def _check_chart_access(
+ self, chart_id: int, exceptions: list[ValidationError]
+ ) -> None:
+ """Validate chart exists and the current user can access it."""
+ chart = ChartDAO.find_by_id(chart_id)
+ if not chart:
+ exceptions.append(ChartNotFoundValidationError())
+ else:
+ try:
+ security_manager.raise_for_access(viz=chart)
Review Comment:
`security_manager.raise_for_access(viz=chart)` is inconsistent with the rest
of this PR (which uses `chart=...` for `Slice` access checks). If
`raise_for_access` doesn’t accept `viz` in this deployment, this will raise
`TypeError` and return a 500. Use the same keyword used elsewhere (e.g.,
`chart=chart`) to ensure the access check is actually applied and doesn’t crash.
##########
superset/sqllab/api.py:
##########
@@ -338,7 +338,6 @@ def export_csv(self, client_id: str) -> CsvResponse:
@expose("/export_streaming/", methods=("POST",))
@protect()
Review Comment:
Removing `@permission_name(\"read\")` changes how Flask-AppBuilder derives
the permission for this endpoint (often defaulting to the method name). That
can be a breaking change for existing roles/permissions and may unintentionally
tighten/loosen access. If the intent is to change the permission model,
add/update an integration test asserting the expected permission behavior;
otherwise, consider restoring an explicit `@permission_name(...)`.
##########
superset/views/users/api.py:
##########
@@ -169,9 +169,11 @@ class UserRestApi(BaseSupersetApi):
resource_name = "user"
openapi_spec_tag = "User"
+ allow_browser_login = True
openapi_spec_component_schemas = (UserResponseSchema,)
@expose("/<int:user_id>/avatar.png", methods=("GET",))
+ @protect()
Review Comment:
The PR description mentions adding `@protect()` to the avatar endpoint, but
this change also sets `allow_browser_login = True` at the API class level
(affecting all `UserRestApi` endpoints, not just avatar). Either document this
broader behavioral change in the PR description or scope it more narrowly if
only needed for the avatar route.
--
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]
