LevisNgigi commented on code in PR #31421:
URL: https://github.com/apache/superset/pull/31421#discussion_r1905845835
##########
superset/queries/saved_queries/api.py:
##########
@@ -196,6 +199,102 @@ def pre_add(self, item: SavedQuery) -> None:
def pre_update(self, item: SavedQuery) -> None:
self.pre_add(item)
+ class SavedQueryRestApi(BaseSupersetModelRestApi):
+ datamodel = SQLAInterface(SavedQuery)
+
+ @has_access_api
+ @expose("/<int:pk>", methods=["GET"])
+ @protect()
+ @safe
+ @statsd_metrics
+ def get(self, pk: int, **kwargs: Any) -> Response:
+ """
+ Retrieve a specific saved query.
+
+ ---
+ get:
+ summary: Retrieve a specific saved query
+ description: >
+ Fetches the details of a saved query based on its primary key (ID).
+ The endpoint ensures that only the query owner or users with the
+ appropriate permissions ('can_read') can access the saved query.
+ parameters:
+ - in: path
+ name: pk
+ required: true
+ schema:
+ type: integer
+ description: The primary key (ID) of the saved query to retrieve.
+ responses:
+ 200:
+ description: Details of the saved query
+ content:
+ application/json:
+ schema:
+ type: object
+ properties:
+ database:
+ type: object
+ description: The database associated with the query.
+ result:
+ type: object
+ description: Details of the saved query.
+ 401:
+ $ref: '#/components/responses/401'
+ 403:
+ $ref: '#/components/responses/403'
+ 404:
+ $ref: '#/components/responses/404'
+ 500:
+ $ref: '#/components/responses/500'
+ """
+ try:
+ saved_query = db.session.query(SavedQuery).get(pk)
+ if not saved_query:
+ return self.response_404()
+
+ is_owner = saved_query.user_id == g.user.id
+ has_access = security_manager.can_access("can_read", "SavedQuery")
Review Comment:
Thank you for the review and feedback.To clarify, the above implementation
does not automatically grant all users access to all saved queries. Instead, it
ensures that only users with can_read permissions for SavedQuery can view the
query(via a shared url)which is a default role for sqllab. This means that
access is already restricted, and not all users will have visibility unless
they explicitly have the required permissions.The ownership check was included
as an additional layer of security, but I understand your concern about
redundancy given the @protect decorator. As you rightly pointed out, one
potential vulnerability is the predictability of query IDs in URLs (e.g.,
http://localhost:9000/sqllab?savedQueryId=3), which makes it possible to guess
query IDs. This could expose queries to unauthorized users who have can_read
permissions but should not have access to specific queries. Implementing UUIDs
for query IDs would mitigate this issue and improve security.Should this be le
ft until query sharing is added in Superset?
--
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]
