You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "LevisNgigi (via GitHub)" <gi...@apache.org> on 2024/12/12 15:53:27 UTC
[PR] refactor(Shared_url_query): Fix shared query URL access for SQL Lab users. [superset]
LevisNgigi opened a new pull request, #31421:
URL: https://github.com/apache/superset/pull/31421
<!---
Fixes #31367
fix(sql-lab): allow shared access to saved queries
-->
### SUMMARY
<!---
This update removes restrictions on accessing shared saved queries in SQL Lab, enabling any user with SQL Lab permissions to load and view shared queries. The changes specifically address the issue where only the creator of a saved query could access it, improving collaboration among SQL Lab users.
-->
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
<!--- Skip this section if not applicable -->
### TESTING INSTRUCTIONS
1. Log in and save a query in SQL Lab.
2. Copy the query URL using the "Copy Query URL" option.
3. Share the URL with a different user (one with the `sqllab` role and access to the dataset).
4. Have the other user try to load the shared query URL.
5. Observe that the second user can successfully load the shared query.
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
- [x] Has associated issue: Fixes #31367
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor(Shared_url_query): Fix shared query URL access for SQL Lab users. [superset]
Posted by "LevisNgigi (via GitHub)" <gi...@apache.org>.
LevisNgigi commented on PR #31421:
URL: https://github.com/apache/superset/pull/31421#issuecomment-2539863562
> Thanks for the clarification @LevisNgigi 👍
@villebro Much Welcome.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor(Shared_url_query): Fix shared query URL access for SQL Lab users. [superset]
Posted by "villebro (via GitHub)" <gi...@apache.org>.
villebro commented on PR #31421:
URL: https://github.com/apache/superset/pull/31421#issuecomment-2539840888
Thanks for the clarification @LevisNgigi 👍
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor(Shared_url_query): Fix shared query URL access for SQL Lab users. [superset]
Posted by "drummerwolli (via GitHub)" <gi...@apache.org>.
drummerwolli commented on PR #31421:
URL: https://github.com/apache/superset/pull/31421#issuecomment-2903846686
@sadpandajoe since i see you are [managing](https://lists.apache.org/thread/m659odpm4mmgsnjrnzvc6c5hwy7qz3pf) the release of 4.1.3, any chance to include this fix still there or in 4.1.4? Here in the PR and in the linked issue, this sounds very severe. And we have in our organization with ca. 2000 users now also this Problem after upgrading to 4.1.1 just last week. I dont think this is a feature release (as someone wrote in the issue) but rather a pretty hefty bug. happy to hear your thoughts.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor(Shared_url_query): Fix shared query URL access for SQL Lab users. [superset]
Posted by "betodealmeida (via GitHub)" <gi...@apache.org>.
betodealmeida commented on code in PR #31421:
URL: https://github.com/apache/superset/pull/31421#discussion_r1927667721
##########
superset-frontend/src/pages/SavedQueryList/index.tsx:
##########
@@ -233,16 +232,31 @@ function SavedQueryList({
};
const copyQueryLink = useCallback(
- (id: number) => {
- copyTextToClipboard(() =>
- Promise.resolve(`${window.location.origin}/sqllab?savedQueryId=${id}`),
- )
- .then(() => {
- addSuccessToast(t('Link Copied!'));
- })
- .catch(() => {
- addDangerToast(t('Sorry, your browser does not support copying.'));
+ async (savedQuery: SavedQueryObject) => {
+ try {
+ const payload = {
+ dbId: savedQuery.db_id,
+ name: savedQuery.label,
+ schema: savedQuery.schema,
Review Comment:
OK, I tested and it you add `catalog` to `superset-frontend/src/views/CRUD/types.ts` it will be populated automatically by the hook, so you just need to add:
```js
catalog: string | null;
```
to `SavedQueryObject`.
Also, it needs to be added to the Schemas and Types as well, because if a database supports multiple catalogs (like BigQuery or Trino) the query with just the schema is not enough information.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor(Shared_url_query): Fix shared query URL access for SQL Lab users. [superset]
Posted by "sadpandajoe (via GitHub)" <gi...@apache.org>.
sadpandajoe commented on PR #31421:
URL: https://github.com/apache/superset/pull/31421#issuecomment-2917560737
> @sadpandajoe since i see you are [managing](https://lists.apache.org/thread/m659odpm4mmgsnjrnzvc6c5hwy7qz3pf) the release of 4.1.3, any chance to include this fix still there or in 4.1.4? Here in the PR and in the linked issue, this sounds very severe. And we have in our organization with ca. 2000 users now also this Problem after upgrading to 4.1.1 just last week. I dont think this is a feature release (as someone wrote in the issue) but rather a pretty hefty bug. happy to hear your thoughts.
@drummerwolli we don't normally throw refactors into official releases as there could be regression bugs coming out of it. Have you checked out the 5.0rc? This should be part of that official release
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor(Shared_url_query): Fix shared query URL access for SQL Lab users. [superset]
Posted by "LevisNgigi (via GitHub)" <gi...@apache.org>.
LevisNgigi commented on code in PR #31421:
URL: https://github.com/apache/superset/pull/31421#discussion_r1927406040
##########
superset-frontend/src/pages/SavedQueryList/index.tsx:
##########
@@ -233,16 +232,31 @@ function SavedQueryList({
};
const copyQueryLink = useCallback(
- (id: number) => {
- copyTextToClipboard(() =>
- Promise.resolve(`${window.location.origin}/sqllab?savedQueryId=${id}`),
- )
- .then(() => {
- addSuccessToast(t('Link Copied!'));
- })
- .catch(() => {
- addDangerToast(t('Sorry, your browser does not support copying.'));
+ async (savedQuery: SavedQueryObject) => {
+ try {
+ const payload = {
+ dbId: savedQuery.db_id,
+ name: savedQuery.label,
+ schema: savedQuery.schema,
Review Comment:
Hey, so currently the catalog property does not exist on the SavedQueryObject type [https://github.com/apache/superset/blob/master/superset-frontend/src/views/CRUD/types.ts](Here).
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor(Shared_url_query): Fix shared query URL access for SQL Lab users. [superset]
Posted by "villebro (via GitHub)" <gi...@apache.org>.
villebro commented on PR #31421:
URL: https://github.com/apache/superset/pull/31421#issuecomment-2539767716
Tagging @yousoph @geido as you were active on the original issue. Has there has been discussion about making queries shareable via permalinks? Was this considered as a solution? As @rusackas commented, exposing saved queries to anyone with a link to it can be problematic, as per the current design they are private objects. Implementing this is of course more work, but is aligned with how sharing is done in Explore and the Dashboard views.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor(Shared_url_query): Fix shared query URL access for SQL Lab users. [superset]
Posted by "dpgaspar (via GitHub)" <gi...@apache.org>.
dpgaspar commented on code in PR #31421:
URL: https://github.com/apache/superset/pull/31421#discussion_r1905517635
##########
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:
The `can_read` on `SavedQuery` is already guaranteed by the `@protect` decorator, so it seem redundant to check if the saved query belongs to the user, since it will grant access to all queries to all users.
This does not seem like the correct approach, currently SavedQueries are not shared on Superset, if we want to add this functionality, we can share links using a UUID or wait until we have use group entities and expose queries to groups
##########
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)
Review Comment:
This looks like a duplicate class definition inside the same class
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor(Shared_url_query): Fix shared query URL access for SQL Lab users. [superset]
Posted by "geido (via GitHub)" <gi...@apache.org>.
geido commented on code in PR #31421:
URL: https://github.com/apache/superset/pull/31421#discussion_r1908504648
##########
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:
@LevisNgigi how hard would it be to implement the UUID approach now?
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor(Shared_url_query): Fix shared query URL access for SQL Lab users. [superset]
Posted by "LevisNgigi (via GitHub)" <gi...@apache.org>.
LevisNgigi commented on code in PR #31421:
URL: https://github.com/apache/superset/pull/31421#discussion_r1927406040
##########
superset-frontend/src/pages/SavedQueryList/index.tsx:
##########
@@ -233,16 +232,31 @@ function SavedQueryList({
};
const copyQueryLink = useCallback(
- (id: number) => {
- copyTextToClipboard(() =>
- Promise.resolve(`${window.location.origin}/sqllab?savedQueryId=${id}`),
- )
- .then(() => {
- addSuccessToast(t('Link Copied!'));
- })
- .catch(() => {
- addDangerToast(t('Sorry, your browser does not support copying.'));
+ async (savedQuery: SavedQueryObject) => {
+ try {
+ const payload = {
+ dbId: savedQuery.db_id,
+ name: savedQuery.label,
+ schema: savedQuery.schema,
Review Comment:
Hey, so currently the catalog property does not exist on the SavedQueryObject type [Here](https://github.com/apache/superset/blob/master/superset-frontend/src/views/CRUD/types.ts).It is an individual objects within sql_tables.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor(Shared_url_query): Fix shared query URL access for SQL Lab users. [superset]
Posted by "LevisNgigi (via GitHub)" <gi...@apache.org>.
LevisNgigi commented on code in PR #31421:
URL: https://github.com/apache/superset/pull/31421#discussion_r1927406040
##########
superset-frontend/src/pages/SavedQueryList/index.tsx:
##########
@@ -233,16 +232,31 @@ function SavedQueryList({
};
const copyQueryLink = useCallback(
- (id: number) => {
- copyTextToClipboard(() =>
- Promise.resolve(`${window.location.origin}/sqllab?savedQueryId=${id}`),
- )
- .then(() => {
- addSuccessToast(t('Link Copied!'));
- })
- .catch(() => {
- addDangerToast(t('Sorry, your browser does not support copying.'));
+ async (savedQuery: SavedQueryObject) => {
+ try {
+ const payload = {
+ dbId: savedQuery.db_id,
+ name: savedQuery.label,
+ schema: savedQuery.schema,
Review Comment:
Hey, so currently the catalog property does not exist on the SavedQueryObject type [Here](https://github.com/apache/superset/blob/master/superset-frontend/src/views/CRUD/types.ts).It is an individual object within sql_tables.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor(Shared_url_query): Fix shared query URL access for SQL Lab users. [superset]
Posted by "LevisNgigi (via GitHub)" <gi...@apache.org>.
LevisNgigi commented on PR #31421:
URL: https://github.com/apache/superset/pull/31421#issuecomment-2608013640
Hey @justinpark I made queries shareable via permalink here as well.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor(Shared_url_query): Fix shared query URL access for SQL Lab users. [superset]
Posted by "betodealmeida (via GitHub)" <gi...@apache.org>.
betodealmeida commented on code in PR #31421:
URL: https://github.com/apache/superset/pull/31421#discussion_r1925967501
##########
superset-frontend/src/pages/SavedQueryList/index.tsx:
##########
@@ -233,16 +232,31 @@ function SavedQueryList({
};
const copyQueryLink = useCallback(
- (id: number) => {
- copyTextToClipboard(() =>
- Promise.resolve(`${window.location.origin}/sqllab?savedQueryId=${id}`),
- )
- .then(() => {
- addSuccessToast(t('Link Copied!'));
- })
- .catch(() => {
- addDangerToast(t('Sorry, your browser does not support copying.'));
+ async (savedQuery: SavedQueryObject) => {
+ try {
+ const payload = {
+ dbId: savedQuery.db_id,
+ name: savedQuery.label,
+ schema: savedQuery.schema,
Review Comment:
We need to store the catalog as well, for databases that support it.
```suggestion
catalog: savedQuery.catalog,
schema: savedQuery.schema,
```
##########
superset-frontend/src/pages/SavedQueryList/index.tsx:
##########
@@ -233,16 +232,31 @@ function SavedQueryList({
};
const copyQueryLink = useCallback(
- (id: number) => {
- copyTextToClipboard(() =>
- Promise.resolve(`${window.location.origin}/sqllab?savedQueryId=${id}`),
- )
- .then(() => {
- addSuccessToast(t('Link Copied!'));
- })
- .catch(() => {
- addDangerToast(t('Sorry, your browser does not support copying.'));
+ async (savedQuery: SavedQueryObject) => {
+ try {
+ const payload = {
+ dbId: savedQuery.db_id,
+ name: savedQuery.label,
+ schema: savedQuery.schema,
+ sql: savedQuery.sql,
+ autorun: false,
+ templateParams: null,
+ };
+
+ const response = await SupersetClient.post({
+ endpoint: '/api/v1/sqllab/permalink',
+ headers: { 'Content-Type': 'application/json' },
+ body: JSON.stringify(payload),
});
+
+ const { key } = response.json;
+ const permalink = `${window.location.origin}/sqllab/p/${key}`;
Review Comment:
The response payload already includes the computed URL:
```suggestion
const { url: permalink } = response.json;
```
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor(Shared_url_query): Fix shared query URL access for SQL Lab users. [superset]
Posted by "LevisNgigi (via GitHub)" <gi...@apache.org>.
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor(Shared_url_query): Fix shared query URL access for SQL Lab users. [superset]
Posted by "LevisNgigi (via GitHub)" <gi...@apache.org>.
LevisNgigi commented on PR #31421:
URL: https://github.com/apache/superset/pull/31421#issuecomment-2539818152
> Tagging @yousoph @geido as you were active on the original issue. Has there has been discussion about making queries shareable via permalinks? Was this considered as a solution? As @rusackas commented, exposing saved queries to anyone with a link to it can be problematic, as per the current design they are private objects. Implementing this is of course more work, but is aligned with how sharing is done in Explore and the Dashboard views.
Hey, just a clarification.Not anyone with the link can view the query.Any user with public, Gamma, or Alpha roles cannot view the saved query link. Access is strictly limited to users who have been explicitly granted permission by the admin under the SQL Lab role. This ensures that only those with appropriate SQL Lab permissions and dataset access are able to load and view the query. All other users, including those with general roles like Gamma or Alpha, will receive an "Access Denied" message when attempting to access the link.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor(Shared_url_query): Fix shared query URL access for SQL Lab users. [superset]
Posted by "justinpark (via GitHub)" <gi...@apache.org>.
justinpark commented on code in PR #31421:
URL: https://github.com/apache/superset/pull/31421#discussion_r1926127434
##########
superset-frontend/src/pages/SavedQueryList/index.tsx:
##########
@@ -233,16 +232,31 @@ function SavedQueryList({
};
const copyQueryLink = useCallback(
- (id: number) => {
- copyTextToClipboard(() =>
- Promise.resolve(`${window.location.origin}/sqllab?savedQueryId=${id}`),
- )
- .then(() => {
- addSuccessToast(t('Link Copied!'));
- })
- .catch(() => {
- addDangerToast(t('Sorry, your browser does not support copying.'));
+ async (savedQuery: SavedQueryObject) => {
+ try {
+ const payload = {
+ dbId: savedQuery.db_id,
+ name: savedQuery.label,
+ schema: savedQuery.schema,
+ sql: savedQuery.sql,
+ autorun: false,
+ templateParams: null,
+ };
+
+ const response = await SupersetClient.post({
+ endpoint: '/api/v1/sqllab/permalink',
+ headers: { 'Content-Type': 'application/json' },
+ body: JSON.stringify(payload),
});
+
+ const { key } = response.json;
+ const permalink = `${window.location.origin}/sqllab/p/${key}`;
Review Comment:
+1
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor(Shared_url_query): Fix shared query URL access for SQL Lab users. [superset]
Posted by "LevisNgigi (via GitHub)" <gi...@apache.org>.
LevisNgigi commented on code in PR #31421:
URL: https://github.com/apache/superset/pull/31421#discussion_r1927529296
##########
superset-frontend/src/pages/SavedQueryList/index.tsx:
##########
@@ -233,16 +232,31 @@ function SavedQueryList({
};
const copyQueryLink = useCallback(
- (id: number) => {
- copyTextToClipboard(() =>
- Promise.resolve(`${window.location.origin}/sqllab?savedQueryId=${id}`),
- )
- .then(() => {
- addSuccessToast(t('Link Copied!'));
- })
- .catch(() => {
- addDangerToast(t('Sorry, your browser does not support copying.'));
+ async (savedQuery: SavedQueryObject) => {
+ try {
+ const payload = {
+ dbId: savedQuery.db_id,
+ name: savedQuery.label,
+ schema: savedQuery.schema,
Review Comment:
okay sounds good.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor(Shared_url_query): Fix shared query URL access for SQL Lab users. [superset]
Posted by "legiondean (via GitHub)" <gi...@apache.org>.
legiondean commented on PR #31421:
URL: https://github.com/apache/superset/pull/31421#issuecomment-2587800680
@LevisNgigi do you know if there's any workaround setup to make the sharing work before this fix released?
Or any chance the fix could be released soon? Thanks!
The issue is pretty annoying when come to team collaboration.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor(Shared_url_query): Fix shared query URL access for SQL Lab users. [superset]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #31421:
URL: https://github.com/apache/superset/pull/31421#issuecomment-2539362526
## [Codecov](https://app.codecov.io/gh/apache/superset/pull/31421?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
Attention: Patch coverage is `50.00000%` with `3 lines` in your changes missing coverage. Please review.
> Project coverage is 65.35%. Comparing base [(`76d897e`)](https://app.codecov.io/gh/apache/superset/commit/76d897eaa2f9e137102bc194c2e3109c29d0348f?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) to head [(`0702ac1`)](https://app.codecov.io/gh/apache/superset/commit/0702ac10be0bc08a99b37b7d21921a74f13bb8b0?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
> Report is 1171 commits behind head on master.
| [Files with missing lines](https://app.codecov.io/gh/apache/superset/pull/31421?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [superset/queries/saved\_queries/api.py](https://app.codecov.io/gh/apache/superset/pull/31421?src=pr&el=tree&filepath=superset%2Fqueries%2Fsaved_queries%2Fapi.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvcXVlcmllcy9zYXZlZF9xdWVyaWVzL2FwaS5weQ==) | 60.00% | [2 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/31421?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [superset/queries/saved\_queries/filters.py](https://app.codecov.io/gh/apache/superset/pull/31421?src=pr&el=tree&filepath=superset%2Fqueries%2Fsaved_queries%2Ffilters.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvcXVlcmllcy9zYXZlZF9xdWVyaWVzL2ZpbHRlcnMucHk=) | 0.00% | [1 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/31421?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #31421 +/- ##
==========================================
+ Coverage 60.48% 65.35% +4.86%
==========================================
Files 1931 537 -1394
Lines 76236 39060 -37176
Branches 8568 0 -8568
==========================================
- Hits 46114 25527 -20587
+ Misses 28017 13533 -14484
+ Partials 2105 0 -2105
```
| [Flag](https://app.codecov.io/gh/apache/superset/pull/31421/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [hive](https://app.codecov.io/gh/apache/superset/pull/31421/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `48.74% <50.00%> (-0.42%)` | :arrow_down: |
| [javascript](https://app.codecov.io/gh/apache/superset/pull/31421/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [presto](https://app.codecov.io/gh/apache/superset/pull/31421/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `53.26% <50.00%> (-0.55%)` | :arrow_down: |
| [python](https://app.codecov.io/gh/apache/superset/pull/31421/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `65.35% <50.00%> (+1.86%)` | :arrow_up: |
| [unit](https://app.codecov.io/gh/apache/superset/pull/31421/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `60.87% <50.00%> (+3.24%)` | :arrow_up: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/superset/pull/31421?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor(Shared_url_query): Fix shared query URL access for SQL Lab users. [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #31421:
URL: https://github.com/apache/superset/pull/31421#issuecomment-2539497852
Pinging @dpgaspar to add more folks who might be interested in a security discussion here. Some might be bothered by the fact that queries can contain PII (like email addresses or names in WHERE clauses) and unlocking these might pose an issue for some deployments/companies.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor(Shared_url_query): Fix shared query URL access for SQL Lab users. [superset]
Posted by "justinpark (via GitHub)" <gi...@apache.org>.
justinpark commented on PR #31421:
URL: https://github.com/apache/superset/pull/31421#issuecomment-2541984523
@LevisNgigi Has there has been discussion about making queries shareable via permalinks (using "copy link")?
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor(Shared_url_query): Fix shared query URL access for SQL Lab users. [superset]
Posted by "LevisNgigi (via GitHub)" <gi...@apache.org>.
LevisNgigi commented on code in PR #31421:
URL: https://github.com/apache/superset/pull/31421#discussion_r1927529296
##########
superset-frontend/src/pages/SavedQueryList/index.tsx:
##########
@@ -233,16 +232,31 @@ function SavedQueryList({
};
const copyQueryLink = useCallback(
- (id: number) => {
- copyTextToClipboard(() =>
- Promise.resolve(`${window.location.origin}/sqllab?savedQueryId=${id}`),
- )
- .then(() => {
- addSuccessToast(t('Link Copied!'));
- })
- .catch(() => {
- addDangerToast(t('Sorry, your browser does not support copying.'));
+ async (savedQuery: SavedQueryObject) => {
+ try {
+ const payload = {
+ dbId: savedQuery.db_id,
+ name: savedQuery.label,
+ schema: savedQuery.schema,
Review Comment:
okay sounds good. Will the [Schemas](https://github.com/apache/superset/blob/2874096e2724938e46cd86f3ae386f385379cddf/superset/sqllab/permalink/schemas.py) and [Types](https://github.com/apache/superset/blob/2874096e2724938e46cd86f3ae386f385379cddf/superset/sqllab/permalink/types.py) need updating as well?
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor(Shared_url_query): Fix shared query URL access for SQL Lab users. [superset]
Posted by "LevisNgigi (via GitHub)" <gi...@apache.org>.
LevisNgigi commented on code in PR #31421:
URL: https://github.com/apache/superset/pull/31421#discussion_r1927529296
##########
superset-frontend/src/pages/SavedQueryList/index.tsx:
##########
@@ -233,16 +232,31 @@ function SavedQueryList({
};
const copyQueryLink = useCallback(
- (id: number) => {
- copyTextToClipboard(() =>
- Promise.resolve(`${window.location.origin}/sqllab?savedQueryId=${id}`),
- )
- .then(() => {
- addSuccessToast(t('Link Copied!'));
- })
- .catch(() => {
- addDangerToast(t('Sorry, your browser does not support copying.'));
+ async (savedQuery: SavedQueryObject) => {
+ try {
+ const payload = {
+ dbId: savedQuery.db_id,
+ name: savedQuery.label,
+ schema: savedQuery.schema,
Review Comment:
okay sounds good. Will the [Schemas](https://github.com/apache/superset/blob/2874096e2724938e46cd86f3ae386f385379cddf/superset/sqllab/permalink/schemas.py) and [https://github.com/apache/superset/blob/2874096e2724938e46cd86f3ae386f385379cddf/superset/sqllab/permalink/types.py](Types) here need updating as well?
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor(Shared_url_query): Fix shared query URL access for SQL Lab users. [superset]
Posted by "LevisNgigi (via GitHub)" <gi...@apache.org>.
LevisNgigi commented on PR #31421:
URL: https://github.com/apache/superset/pull/31421#issuecomment-2542178381
> @LevisNgigi Has there has been discussion about making queries shareable via permalinks (using "copy link")?
Hey @justinpark I do not think so. I'm not aware of any discussion in reference to the above.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor(Shared_url_query): Fix shared query URL access for SQL Lab users. [superset]
Posted by "LevisNgigi (via GitHub)" <gi...@apache.org>.
LevisNgigi commented on PR #31421:
URL: https://github.com/apache/superset/pull/31421#issuecomment-2587879065
> @LevisNgigi do you know if there's any workaround setup to make the sharing work before this fix released? Or any chance the fix could be released soon? Thanks! The issue is pretty annoying when come to team collaboration.
@legiondean Thanks for bringing this up! I am not aware of any workaround as queries were not meant to be shared originally, but I'm actively working on resolving the issue by implementing a UUID-based approach, which will address the predictability of query IDs and make sharing more secure and reliable.Hopefully it will be released and approved soon. I appreciate your patience and understanding while we work on this!
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor(Shared_url_query): Fix shared query URL access for SQL Lab users. [superset]
Posted by "justinpark (via GitHub)" <gi...@apache.org>.
justinpark commented on PR #31421:
URL: https://github.com/apache/superset/pull/31421#issuecomment-2608583028
LGTM
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor(Shared_url_query): Fix shared query URL access for SQL Lab users. [superset]
Posted by "geido (via GitHub)" <gi...@apache.org>.
geido commented on PR #31421:
URL: https://github.com/apache/superset/pull/31421#issuecomment-2575081563
Hey @LevisNgigi I am a bit confused about the latest changes where it is just showing a newly introduced endpoint. Would you clarify the approach here? Thank you!
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor(Shared_url_query): Fix shared query URL access for SQL Lab users. [superset]
Posted by "LevisNgigi (via GitHub)" <gi...@apache.org>.
LevisNgigi commented on PR #31421:
URL: https://github.com/apache/superset/pull/31421#issuecomment-2575116648
> Hey @LevisNgigi I am a bit confused about the latest changes where it is just showing a newly introduced endpoint. Would you clarify the approach here? Thank you!
Hey @geido This endpoint already existed in the project "/api/v1/query/{pk}". I added custom implementation on the endpoint to add ownership validation to ensure only the query owner or authorized users can access it.Those with "can_read", "SavedQuery" permission. The custom implementation I added addresses this gap,by ensuring that users with sqllab role can view shared query via the url.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor(Shared_url_query): Fix shared query URL access for SQL Lab users. [superset]
Posted by "dpgaspar (via GitHub)" <gi...@apache.org>.
dpgaspar commented on code in PR #31421:
URL: https://github.com/apache/superset/pull/31421#discussion_r1905517635
##########
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:
The `can_read` on `SavedQuery` is already guaranteed by the `@protect` decorator, so it seem redundant to check if the saved query belongs to the user, since it will grant access to all queries to all users.
This does not seem like the correct approach, currently SavedQueries are not shared on Superset, if we want to add this functionality, we can share links using a UUID or wait until we have use group entities and expose queries to groups.
Security for this is implemented at the filter level here: https://github.com/apache/superset/blob/master/superset/queries/saved_queries/filters.py#L89
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor(Shared_url_query): Fix shared query URL access for SQL Lab users. [superset]
Posted by "LevisNgigi (via GitHub)" <gi...@apache.org>.
LevisNgigi commented on code in PR #31421:
URL: https://github.com/apache/superset/pull/31421#discussion_r1908881912
##########
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:
> @LevisNgigi how hard would it be to implement the UUID approach now?
Implementing the UUID approach would require running a migration script to update existing query IDs to UUIDs in the database while maintaining backward compatibility. Additionally, the logic to verify if the user has the sqllab role would need to be integrated to ensure the correct permissions are applied.I think this adds some complexity especially with the migrations. Unless there is a different approach?
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor(Shared_url_query): Fix shared query URL access for SQL Lab users. [superset]
Posted by "LevisNgigi (via GitHub)" <gi...@apache.org>.
LevisNgigi commented on code in PR #31421:
URL: https://github.com/apache/superset/pull/31421#discussion_r1908881912
##########
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:
> @LevisNgigi how hard would it be to implement the UUID approach now?
@geido Implementing the UUID approach would require running a migration script to update existing query IDs to UUIDs in the database while maintaining backward compatibility. Additionally, the logic to verify if the user has the sqllab role would need to be integrated to ensure the correct permissions are applied.I think this adds some complexity especially with the migrations. Unless there is a different approach?
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor(Shared_url_query): Fix shared query URL access for SQL Lab users. [superset]
Posted by "LevisNgigi (via GitHub)" <gi...@apache.org>.
LevisNgigi commented on PR #31421:
URL: https://github.com/apache/superset/pull/31421#issuecomment-2612974032
> Awesome! Thanks for making sure it works with catalogs as well!
Pleasure is mine.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor(Shared_url_query): Fix shared query URL access for SQL Lab users. [superset]
Posted by "geido (via GitHub)" <gi...@apache.org>.
geido merged PR #31421:
URL: https://github.com/apache/superset/pull/31421
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor(Shared_url_query): Fix shared query URL access for SQL Lab users. [superset]
Posted by "betodealmeida (via GitHub)" <gi...@apache.org>.
betodealmeida commented on code in PR #31421:
URL: https://github.com/apache/superset/pull/31421#discussion_r1927465857
##########
superset-frontend/src/pages/SavedQueryList/index.tsx:
##########
@@ -233,16 +232,31 @@ function SavedQueryList({
};
const copyQueryLink = useCallback(
- (id: number) => {
- copyTextToClipboard(() =>
- Promise.resolve(`${window.location.origin}/sqllab?savedQueryId=${id}`),
- )
- .then(() => {
- addSuccessToast(t('Link Copied!'));
- })
- .catch(() => {
- addDangerToast(t('Sorry, your browser does not support copying.'));
+ async (savedQuery: SavedQueryObject) => {
+ try {
+ const payload = {
+ dbId: savedQuery.db_id,
+ name: savedQuery.label,
+ schema: savedQuery.schema,
Review Comment:
Ok, let me add it before you merge this, we need 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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org