Copilot commented on code in PR #39304:
URL: https://github.com/apache/superset/pull/39304#discussion_r3190480387
##########
superset/charts/api.py:
##########
@@ -1023,6 +1025,58 @@ def remove_favorite(self, pk: int) -> Response:
return self.response(200, result="OK")
+ @expose("/related/<column_name>", methods=("GET",))
+ @protect()
+ @safe
+ @statsd_metrics
+ @parse_rison(get_related_schema)
+ @handle_api_exception
+ def related(self, column_name: str, **kwargs: Any) -> Response:
Review Comment:
The write-permission check runs only after `parse_rison(get_related_schema)`
has already parsed and validated the request. That means a read-only caller can
still trigger schema/validation errors on `/related/owners` and receive a 400
instead of a consistent 403, which leaks endpoint behavior and does unnecessary
work before authorization. The access check should happen before request
parsing for this field.
##########
superset/charts/api.py:
##########
@@ -1023,6 +1025,58 @@ def remove_favorite(self, pk: int) -> Response:
return self.response(200, result="OK")
+ @expose("/related/<column_name>", methods=("GET",))
+ @protect()
+ @safe
+ @statsd_metrics
+ @parse_rison(get_related_schema)
+ @handle_api_exception
+ def related(self, column_name: str, **kwargs: Any) -> Response:
+ """Get related fields data, restricting owner lookup to users with
write access.
+ ---
+ get:
+ summary: Get related fields data
+ parameters:
+ - in: path
+ schema:
+ type: string
+ name: column_name
+ - in: query
+ name: q
+ content:
+ application/json:
+ schema:
+ $ref: '#/components/schemas/get_related_schema'
+ responses:
+ 200:
+ description: Related column data
+ content:
+ application/json:
+ schema:
+ $ref: "#/components/schemas/RelatedResponseSchema"
+ 400:
+ $ref: '#/components/responses/400'
+ 401:
+ $ref: '#/components/responses/401'
+ 403:
+ $ref: '#/components/responses/403'
+ 404:
+ $ref: '#/components/responses/404'
+ 500:
+ $ref: '#/components/responses/500'
+ """
+ if response := self.ensure_owners_write_access():
+ return response
Review Comment:
The write-permission check runs only after `parse_rison(get_related_schema)`
has already parsed and validated the request. That means a read-only caller can
still trigger schema/validation errors on `/related/owners` and receive a 400
instead of a consistent 403, which leaks endpoint behavior and does unnecessary
work before authorization. The access check should happen before request
parsing for this field.
##########
superset/charts/api.py:
##########
@@ -1023,6 +1025,58 @@ def remove_favorite(self, pk: int) -> Response:
return self.response(200, result="OK")
+ @expose("/related/<column_name>", methods=("GET",))
+ @protect()
+ @safe
+ @statsd_metrics
+ @parse_rison(get_related_schema)
+ @handle_api_exception
+ def related(self, column_name: str, **kwargs: Any) -> Response:
+ """Get related fields data, restricting owner lookup to users with
write access.
+ ---
+ get:
+ summary: Get related fields data
+ parameters:
+ - in: path
+ schema:
+ type: string
+ name: column_name
+ - in: query
+ name: q
+ content:
+ application/json:
+ schema:
+ $ref: '#/components/schemas/get_related_schema'
+ responses:
+ 200:
+ description: Related column data
+ content:
+ application/json:
+ schema:
+ $ref: "#/components/schemas/RelatedResponseSchema"
+ 400:
+ $ref: '#/components/responses/400'
+ 401:
+ $ref: '#/components/responses/401'
+ 403:
+ $ref: '#/components/responses/403'
+ 404:
+ $ref: '#/components/responses/404'
+ 500:
+ $ref: '#/components/responses/500'
+ """
+ if response := self.ensure_owners_write_access():
+ return response
+ return super().related(column_name, **kwargs)
Review Comment:
This override copies the full route, decorator, and OpenAPI metadata from
the base `related()` implementation just to insert one authorization check. Any
future change to the base endpoint’s docs, decorators, or request handling now
has to be mirrored here manually, which makes this endpoint likely to drift
over time. A smaller hook or a dedicated authorization seam would be easier to
keep correct.
##########
superset/charts/api.py:
##########
@@ -1023,6 +1025,58 @@ def remove_favorite(self, pk: int) -> Response:
return self.response(200, result="OK")
+ @expose("/related/<column_name>", methods=("GET",))
+ @protect()
+ @safe
+ @statsd_metrics
+ @parse_rison(get_related_schema)
+ @handle_api_exception
+ def related(self, column_name: str, **kwargs: Any) -> Response:
+ """Get related fields data, restricting owner lookup to users with
write access.
+ ---
+ get:
+ summary: Get related fields data
+ parameters:
+ - in: path
+ schema:
+ type: string
+ name: column_name
+ - in: query
+ name: q
+ content:
+ application/json:
+ schema:
+ $ref: '#/components/schemas/get_related_schema'
+ responses:
+ 200:
+ description: Related column data
+ content:
+ application/json:
+ schema:
+ $ref: "#/components/schemas/RelatedResponseSchema"
+ 400:
+ $ref: '#/components/responses/400'
+ 401:
+ $ref: '#/components/responses/401'
+ 403:
+ $ref: '#/components/responses/403'
+ 404:
+ $ref: '#/components/responses/404'
+ 500:
+ $ref: '#/components/responses/500'
+ """
+ if response := self.ensure_owners_write_access():
+ return response
+ return super().related(column_name, **kwargs)
Review Comment:
The new `related()` override is not exercised directly by the added tests.
The test file only calls `ensure_owners_write_access()` on a partially
initialized object, so it does not verify the actual `/related/<column_name>`
route behavior, the copied decorator stack, or the delegation to
`super().related()`. That leaves the endpoint-level 403/200 contract unverified
despite the PR description claiming those paths are covered.
##########
superset/charts/api.py:
##########
@@ -1023,6 +1025,58 @@ def remove_favorite(self, pk: int) -> Response:
return self.response(200, result="OK")
+ @expose("/related/<column_name>", methods=("GET",))
+ @protect()
+ @safe
+ @statsd_metrics
+ @parse_rison(get_related_schema)
+ @handle_api_exception
+ def related(self, column_name: str, **kwargs: Any) -> Response:
+ """Get related fields data, restricting owner lookup to users with
write access.
+ ---
+ get:
+ summary: Get related fields data
+ parameters:
+ - in: path
+ schema:
+ type: string
+ name: column_name
+ - in: query
+ name: q
+ content:
+ application/json:
+ schema:
+ $ref: '#/components/schemas/get_related_schema'
+ responses:
+ 200:
+ description: Related column data
+ content:
+ application/json:
+ schema:
+ $ref: "#/components/schemas/RelatedResponseSchema"
+ 400:
+ $ref: '#/components/responses/400'
+ 401:
+ $ref: '#/components/responses/401'
+ 403:
+ $ref: '#/components/responses/403'
+ 404:
+ $ref: '#/components/responses/404'
+ 500:
+ $ref: '#/components/responses/500'
+ """
+ if response := self.ensure_owners_write_access():
+ return response
+ return super().related(column_name, **kwargs)
+
+ def ensure_owners_write_access(self) -> Optional[Response]:
+ """Restrict the owners related field to users with write access."""
+ if request.view_args.get("column_name") == "owners" and not (
+ security_manager.can_access("can_write",
self.class_permission_name)
+ ):
+ return self.response_403()
+ return None
Review Comment:
The helper ignores the `column_name` already available in `related()` and
instead reaches into `request.view_args`. That hidden dependency on Flask
routing state makes the check harder to reuse and test, and it is why the tests
need to synthesize `view_args` manually. Passing `column_name` into this helper
would keep the authorization logic explicit and less brittle.
##########
superset/charts/api.py:
##########
@@ -1023,6 +1025,58 @@ def remove_favorite(self, pk: int) -> Response:
return self.response(200, result="OK")
+ @expose("/related/<column_name>", methods=("GET",))
+ @protect()
+ @safe
+ @statsd_metrics
+ @parse_rison(get_related_schema)
+ @handle_api_exception
+ def related(self, column_name: str, **kwargs: Any) -> Response:
Review Comment:
The new `related()` override is not exercised directly by the added tests.
The test file only calls `ensure_owners_write_access()` on a partially
initialized object, so it does not verify the actual `/related/<column_name>`
route behavior, the copied decorator stack, or the delegation to
`super().related()`. That leaves the endpoint-level 403/200 contract unverified
despite the PR description claiming those paths are covered.
--
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]
