You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "jfrag1 (via GitHub)" <gi...@apache.org> on 2023/06/06 18:18:47 UTC

[GitHub] [superset] jfrag1 opened a new pull request, #24301: fix: Ability to share saved queries

jfrag1 opened a new pull request, #24301:
URL: https://github.com/apache/superset/pull/24301

   <!---
   Please write the PR title following the conventions at https://www.conventionalcommits.org/en/v1.0.0/
   Example:
   fix(dashboard): load charts correctly
   -->
   
   ### SUMMARY
   <!--- Describe the change below, including rationale and design decisions -->
   Currently, when a user tries to access a shared query link created by another user, they cannot access the query and only see a "This query couldn't be loaded" toast.  This was caused by [this PR](https://github.com/apache/superset/pull/21682/files#diff-1fe9f23910bda67f66ff7b5e9127cdaba0d304153a5246dbcb2d76d70d2e5a4fR1365), which updated the endpoint used to fetch the query.  The new endpoint currently only allows users to fetch their own saved queries, while the old one allowed any.
   
   My solution to this is to also allow looking up a saved query by its UUID for non-creators so that we can more securely enable sharing saved queries in the v1 API.
   
   I also formally marked the old APIs as deprecated for removal in 4.0 and replaced its final usage with the v1 API
   
   ### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
   <!--- Skip this if not applicable -->
   
   ### TESTING INSTRUCTIONS
   <!--- Required! What steps can be taken to manually verify the changes? -->
   
   ### ADDITIONAL INFORMATION
   <!--- Check any relevant boxes with "x" -->
   <!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
   - [ ] Has associated issue:
   - [ ] 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


[GitHub] [superset] jfrag1 commented on a diff in pull request #24301: fix: Ability to share saved queries

Posted by "jfrag1 (via GitHub)" <gi...@apache.org>.
jfrag1 commented on code in PR #24301:
URL: https://github.com/apache/superset/pull/24301#discussion_r1221939254


##########
superset/queries/saved_queries/dao.py:
##########
@@ -44,3 +47,20 @@ def bulk_delete(models: Optional[list[SavedQuery]], commit: bool = True) -> None
         except SQLAlchemyError as ex:
             db.session.rollback()
             raise DAODeleteFailedError() from ex
+
+    @classmethod
+    def get_by_id(cls, _id: str) -> Optional[SavedQuery]:

Review Comment:
   Bypassing the security check if a UUID is passed is intentional, it allows users to access saved queries via the API only if the UUID link has been shared with them.  There is precedence for this approach in the codebase, see the dashboard DAO: https://github.com/apache/superset/blob/master/superset/dashboards/dao.py#L47



-- 
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


[GitHub] [superset] jfrag1 commented on a diff in pull request #24301: fix: Ability to share saved queries

Posted by "jfrag1 (via GitHub)" <gi...@apache.org>.
jfrag1 commented on code in PR #24301:
URL: https://github.com/apache/superset/pull/24301#discussion_r1221933270


##########
superset-frontend/src/SqlLab/components/ShareSqlLabQuery/index.tsx:
##########
@@ -76,10 +77,10 @@ const ShareSqlLabQuery = ({
   const getCopyUrlForSavedQuery = (callback: Function) => {
     let savedQueryToastContent;
 
-    if (remoteId) {
+    if (uuid) {
       savedQueryToastContent = `${
         window.location.origin + window.location.pathname
-      }?savedQueryId=${remoteId}`;
+      }?savedQueryId=${uuid}`;

Review Comment:
   Existing URLs will still works just fine as the get saved query endpoint still accepts numerical id's



##########
superset-frontend/src/SqlLab/components/ShareSqlLabQuery/index.tsx:
##########
@@ -76,10 +77,10 @@ const ShareSqlLabQuery = ({
   const getCopyUrlForSavedQuery = (callback: Function) => {
     let savedQueryToastContent;
 
-    if (remoteId) {
+    if (uuid) {
       savedQueryToastContent = `${
         window.location.origin + window.location.pathname
-      }?savedQueryId=${remoteId}`;
+      }?savedQueryId=${uuid}`;

Review Comment:
   Existing URLs will still work just fine as the get saved query endpoint still accepts numerical id's



-- 
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


[GitHub] [superset] jfrag1 commented on a diff in pull request #24301: fix: Ability to share saved queries

Posted by "jfrag1 (via GitHub)" <gi...@apache.org>.
jfrag1 commented on code in PR #24301:
URL: https://github.com/apache/superset/pull/24301#discussion_r1221934734


##########
superset/queries/saved_queries/api.py:
##########
@@ -174,6 +182,52 @@ def pre_add(self, item: SavedQuery) -> None:
     def pre_update(self, item: SavedQuery) -> None:
         self.pre_add(item)
 
+    @expose("/<_id>", methods=("GET",))
+    @protect()
+    @safe
+    @permission_name("get")
+    @statsd_metrics
+    @event_logger.log_this_with_context(
+        action=lambda self, *args, **kwargs: f"{self.__class__.__name__}.get",
+        log_to_statsd=False,
+    )
+    def get(self, _id: str, **kwargs: Any) -> Response:

Review Comment:
   the string type captures both integer id's and UUIDs. The dao will then cast the type appropriately depending on which is passed



-- 
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


[GitHub] [superset] michael-s-molina commented on a diff in pull request #24301: fix: Ability to share saved queries

Posted by "michael-s-molina (via GitHub)" <gi...@apache.org>.
michael-s-molina commented on code in PR #24301:
URL: https://github.com/apache/superset/pull/24301#discussion_r1221959356


##########
superset/queries/saved_queries/dao.py:
##########
@@ -44,3 +47,20 @@ def bulk_delete(models: Optional[list[SavedQuery]], commit: bool = True) -> None
         except SQLAlchemyError as ex:
             db.session.rollback()
             raise DAODeleteFailedError() from ex
+
+    @classmethod
+    def get_by_id(cls, _id: str) -> Optional[SavedQuery]:

Review Comment:
   I would use the permalink logic for this purpose and probably migrate the dashboard DAO too. Keeping multiple strategies for sharing resources only increases code complexity and opens the door for inconsistent security checks.
   



-- 
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


[GitHub] [superset] john-bodley commented on a diff in pull request #24301: fix: Ability to share saved queries

Posted by "john-bodley (via GitHub)" <gi...@apache.org>.
john-bodley commented on code in PR #24301:
URL: https://github.com/apache/superset/pull/24301#discussion_r1231653460


##########
superset/queries/saved_queries/dao.py:
##########
@@ -44,3 +47,20 @@ def bulk_delete(models: Optional[list[SavedQuery]], commit: bool = True) -> None
         except SQLAlchemyError as ex:
             db.session.rollback()
             raise DAODeleteFailedError() from ex
+
+    @classmethod
+    def get_by_id(cls, _id: str) -> Optional[SavedQuery]:

Review Comment:
   @eschutho I’m not sure that the UUID seems correct. It feels like an internal representation that we’re now exposing to circumvent security, i.e., it feels akin to saying users enter a bar and present their ID for verification, but if you wave a UUID everything is fine.
   
   In my opinion the precedence which was set previously (the example @jfrag1 linked to—which is the sam one I was referring to in the town hall) should be thought of as the exception rather than the rule, i.e., that logic would also need to be updated in a future PR.



-- 
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


[GitHub] [superset] jfrag1 commented on a diff in pull request #24301: fix: Ability to share saved queries

Posted by "jfrag1 (via GitHub)" <gi...@apache.org>.
jfrag1 commented on code in PR #24301:
URL: https://github.com/apache/superset/pull/24301#discussion_r1220115126


##########
superset/views/sql_lab/views.py:
##########
@@ -127,9 +132,32 @@ class SavedQueryViewApi(SavedQueryView):  # pylint: disable=too-many-ancestors
     show_columns = add_columns + ["id"]
 
     @has_access_api
-    @expose("show/<pk>")
-    def show(self, pk: int) -> FlaskResponse:

Review Comment:
   this endpoint didn't actually exist, not included in `include_route_methods`



-- 
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


[GitHub] [superset] jfrag1 closed pull request #24301: fix: Ability to share saved queries

Posted by "jfrag1 (via GitHub)" <gi...@apache.org>.
jfrag1 closed pull request #24301: fix: Ability to share saved queries
URL: https://github.com/apache/superset/pull/24301


-- 
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


[GitHub] [superset] jfrag1 commented on a diff in pull request #24301: fix: Ability to share saved queries

Posted by "jfrag1 (via GitHub)" <gi...@apache.org>.
jfrag1 commented on code in PR #24301:
URL: https://github.com/apache/superset/pull/24301#discussion_r1220291590


##########
superset-frontend/src/SqlLab/types.ts:
##########
@@ -49,6 +49,7 @@ export interface QueryEditor {
   selectedText?: string;
   queryLimit?: number;
   description?: string;
+  uuid: string | null;

Review Comment:
   My understanding is that `uuid: string | null` means the key will always be there, but may be null, and `uuid?: string` means that the key may or may not be there, but if it is it's a string.  This way seemed more accurate to me, I copied from the type def for the `remoteId` here (which is the saved query numerical id), since it should be the same for UUID



-- 
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


[GitHub] [superset] jfrag1 commented on a diff in pull request #24301: fix: Ability to share saved queries

Posted by "jfrag1 (via GitHub)" <gi...@apache.org>.
jfrag1 commented on code in PR #24301:
URL: https://github.com/apache/superset/pull/24301#discussion_r1234683640


##########
superset/queries/saved_queries/dao.py:
##########
@@ -44,3 +47,20 @@ def bulk_delete(models: Optional[list[SavedQuery]], commit: bool = True) -> None
         except SQLAlchemyError as ex:
             db.session.rollback()
             raise DAODeleteFailedError() from ex
+
+    @classmethod
+    def get_by_id(cls, _id: str) -> Optional[SavedQuery]:

Review Comment:
   opened https://github.com/apache/superset/pull/24434 to revert the original change which broke this functionality



-- 
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


[GitHub] [superset] john-bodley commented on a diff in pull request #24301: fix: Ability to share saved queries

Posted by "john-bodley (via GitHub)" <gi...@apache.org>.
john-bodley commented on code in PR #24301:
URL: https://github.com/apache/superset/pull/24301#discussion_r1231653460


##########
superset/queries/saved_queries/dao.py:
##########
@@ -44,3 +47,20 @@ def bulk_delete(models: Optional[list[SavedQuery]], commit: bool = True) -> None
         except SQLAlchemyError as ex:
             db.session.rollback()
             raise DAODeleteFailedError() from ex
+
+    @classmethod
+    def get_by_id(cls, _id: str) -> Optional[SavedQuery]:

Review Comment:
   @eschutho I’m not sure that the UUID seems correct both from a security and consistency perspective.
   
   It feels like an internal representation that we’re now exposing to circumvent security, i.e., it feels akin to saying you enter a bar and have to present your ID (an integer or slug) for verification, however if you merely wave a UUID you bypass the bouncer. Granted it’s harder to forge a UUID as opposed to guess a valid ID number, but that still doesn’t seem secure.
   
   In my opinion the precedence which was set previously (the example @jfrag1 linked to—which is the sam one I was referring to in the town hall) should be thought of as the exception rather than the rule, i.e., that logic would also need to be updated in a future PR.



##########
superset/queries/saved_queries/dao.py:
##########
@@ -44,3 +47,20 @@ def bulk_delete(models: Optional[list[SavedQuery]], commit: bool = True) -> None
         except SQLAlchemyError as ex:
             db.session.rollback()
             raise DAODeleteFailedError() from ex
+
+    @classmethod
+    def get_by_id(cls, _id: str) -> Optional[SavedQuery]:

Review Comment:
   @eschutho I’m not sure that the UUID seems correct both from a security and consistency perspective.
   
   It feels like an internal representation that we’re now exposing to circumvent security, i.e., it feels akin to saying you enter a bar and have to present your ID (an integer or slug) for verification, however if you merely wave a UUID you bypass the bouncer. Granted it’s harder to forge a UUID as opposed to guess a valid ID number, but that still doesn’t seem secure.
   
   In my opinion the precedence which was set previously (the example @jfrag1 linked to—which is the same one I was referring to in the town hall) should be thought of as the exception rather than the rule, i.e., that logic would also need to be updated in a future PR.



-- 
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


[GitHub] [superset] zephyring commented on a diff in pull request #24301: fix: Ability to share saved queries

Posted by "zephyring (via GitHub)" <gi...@apache.org>.
zephyring commented on code in PR #24301:
URL: https://github.com/apache/superset/pull/24301#discussion_r1232581425


##########
superset/queries/saved_queries/dao.py:
##########
@@ -44,3 +47,20 @@ def bulk_delete(models: Optional[list[SavedQuery]], commit: bool = True) -> None
         except SQLAlchemyError as ex:
             db.session.rollback()
             raise DAODeleteFailedError() from ex
+
+    @classmethod
+    def get_by_id(cls, _id: str) -> Optional[SavedQuery]:

Review Comment:
   I agreed with using permalink as long term solution for `copy link` or any other share with link feature as opposed to directly sharing with raw uuid. Even though uuid is impossible to guess, it's still security through obscurity(link can be leaked unintentionally).
   And we should be consistent for authorization policy on a resource regardless of the format of their IDs.
   But like @jfrag1 said this PR is to fix a breaking changes of the other PR. We will need to either revert that breaking PR or allow exception of this PR for now until formal SIP is made around how we represent/export resource application wide.



-- 
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


[GitHub] [superset] michael-s-molina commented on pull request #24301: fix: Ability to share saved queries

Posted by "michael-s-molina (via GitHub)" <gi...@apache.org>.
michael-s-molina commented on PR #24301:
URL: https://github.com/apache/superset/pull/24301#issuecomment-1650340301

   Closing this as we reverted the original PR. @jfrag1 feel free to reopen if necessary.


-- 
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


[GitHub] [superset] jfrag1 commented on a diff in pull request #24301: fix: Ability to share saved queries

Posted by "jfrag1 (via GitHub)" <gi...@apache.org>.
jfrag1 commented on code in PR #24301:
URL: https://github.com/apache/superset/pull/24301#discussion_r1221935663


##########
superset-frontend/src/pages/SavedQueryList/index.tsx:
##########
@@ -216,10 +216,10 @@ function SavedQueryList({
   };
 
   const copyQueryLink = useCallback(
-    (id: number) => {
+    (uuid: string) => {

Review Comment:
   not that I'm aware of, didn't find anything on a quick google search either



-- 
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


[GitHub] [superset] john-bodley commented on a diff in pull request #24301: fix: Ability to share saved queries

Posted by "john-bodley (via GitHub)" <gi...@apache.org>.
john-bodley commented on code in PR #24301:
URL: https://github.com/apache/superset/pull/24301#discussion_r1232578110


##########
superset/queries/saved_queries/dao.py:
##########
@@ -44,3 +47,20 @@ def bulk_delete(models: Optional[list[SavedQuery]], commit: bool = True) -> None
         except SQLAlchemyError as ex:
             db.session.rollback()
             raise DAODeleteFailedError() from ex
+
+    @classmethod
+    def get_by_id(cls, _id: str) -> Optional[SavedQuery]:

Review Comment:
   I agree with @michael-s-molina's comment regarding making this a SIP. I understand that this feels somewhat like a blocker to what feels like a somewhat benign fix, but I think it's important that we flush out (and detangle) the key concepts that Michael and myself raised to set the right precedence, as opposed to a bandaid solution.
   
   Historically UUIDs were obfuscated and solely used to make charts, dashboards, etc. globally unique across shared Superset instances, however their use within the API is more of an organic phenomenon and thus I think there's merit in pausing, taking a step back, and evaluating how we think about UUIDs, shareable links, etc. in a more holistic manner.
   
   I agree that finding a matching UUID is near impossible, but using UUIDs for security (via cryptography) isn't valid. Note this is in no way a criticism of your work, but more with the state of the Superset code base. At Airbnb we support thousands of weekly active users and are constantly fixing issues related to problematic features and/or inconsistent/non-coherent/complex functionality (you can see the [vast majority](https://github.com/apache/superset/pulls/john-bodley) of my commits to this repo are actually fixes and/or mechanism to improve quality).



-- 
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


[GitHub] [superset] eschutho commented on a diff in pull request #24301: fix: Ability to share saved queries

Posted by "eschutho (via GitHub)" <gi...@apache.org>.
eschutho commented on code in PR #24301:
URL: https://github.com/apache/superset/pull/24301#discussion_r1232637339


##########
superset/queries/saved_queries/dao.py:
##########
@@ -44,3 +47,20 @@ def bulk_delete(models: Optional[list[SavedQuery]], commit: bool = True) -> None
         except SQLAlchemyError as ex:
             db.session.rollback()
             raise DAODeleteFailedError() from ex
+
+    @classmethod
+    def get_by_id(cls, _id: str) -> Optional[SavedQuery]:

Review Comment:
   Agree that a SIP on this functionality is a good idea. We were planning to write one up, I think @john-bodley is also interested in writing it. If we think this PR is too premature at this point, what do you all think about us reverting [this one line](https://github.com/apache/superset/pull/21682/files#diff-1fe9f23910bda67f66ff7b5e9127cdaba0d304153a5246dbcb2d76d70d2e5a4fR1365) back to the old api so that we can fix the sharing capabilities that are currently broken, and then we can talk about a longer-term solution in an upcoming SIP? cc @hughhhh who is also helping 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


[GitHub] [superset] codecov[bot] commented on pull request #24301: fix: Ability to share saved queries

Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #24301:
URL: https://github.com/apache/superset/pull/24301#issuecomment-1579308530

   ## [Codecov](https://app.codecov.io/gh/apache/superset/pull/24301?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
   > Merging [#24301](https://app.codecov.io/gh/apache/superset/pull/24301?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (5bef557) into [master](https://app.codecov.io/gh/apache/superset/commit/ede6acdb3afdf4003359b0c6e2aa8c4b154773ae?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (ede6acd) will **decrease** coverage by `9.27%`.
   > The diff coverage is `73.40%`.
   
   > :exclamation: Current head 5bef557 differs from pull request most recent head 36052ed. Consider uploading reports for the commit 36052ed to get more accurate results
   
   ```diff
   @@            Coverage Diff             @@
   ##           master   #24301      +/-   ##
   ==========================================
   - Coverage   66.53%   57.26%   -9.27%     
   ==========================================
     Files        1957     1957              
     Lines       75623    75697      +74     
     Branches     8224     8223       -1     
   ==========================================
   - Hits        50312    43349    -6963     
   - Misses      23202    30240    +7038     
   + Partials     2109     2108       -1     
   ```
   
   | Flag | Coverage Δ | |
   |---|---|---|
   | hive | `53.44% <73.40%> (?)` | |
   | mysql | `?` | |
   | postgres | `?` | |
   | presto | `53.36% <73.40%> (?)` | |
   | python | `59.97% <73.40%> (-19.17%)` | :arrow_down: |
   | sqlite | `?` | |
   | unit | `53.49% <73.40%> (?)` | |
   
   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.
   
   | [Impacted Files](https://app.codecov.io/gh/apache/superset/pull/24301?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
   |---|---|---|
   | [superset-frontend/src/SqlLab/actions/sqlLab.js](https://app.codecov.io/gh/apache/superset/pull/24301?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9hY3Rpb25zL3NxbExhYi5qcw==) | `66.66% <ø> (ø)` | |
   | [...d/src/SqlLab/components/ShareSqlLabQuery/index.tsx](https://app.codecov.io/gh/apache/superset/pull/24301?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL1NoYXJlU3FsTGFiUXVlcnkvaW5kZXgudHN4) | `84.61% <ø> (+3.13%)` | :arrow_up: |
   | [superset-frontend/src/SqlLab/fixtures.ts](https://app.codecov.io/gh/apache/superset/pull/24301?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9maXh0dXJlcy50cw==) | `100.00% <ø> (ø)` | |
   | [superset-frontend/src/SqlLab/types.ts](https://app.codecov.io/gh/apache/superset/pull/24301?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi90eXBlcy50cw==) | `57.14% <ø> (ø)` | |
   | [...perset-frontend/src/pages/SavedQueryList/index.tsx](https://app.codecov.io/gh/apache/superset/pull/24301?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3BhZ2VzL1NhdmVkUXVlcnlMaXN0L2luZGV4LnRzeA==) | `64.60% <ø> (-0.32%)` | :arrow_down: |
   | [superset/models/sql\_lab.py](https://app.codecov.io/gh/apache/superset/pull/24301?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvbW9kZWxzL3NxbF9sYWIucHk=) | `74.20% <0.00%> (-2.39%)` | :arrow_down: |
   | [superset/utils/core.py](https://app.codecov.io/gh/apache/superset/pull/24301?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvdXRpbHMvY29yZS5weQ==) | `65.10% <16.66%> (-23.38%)` | :arrow_down: |
   | [superset/queries/saved\_queries/dao.py](https://app.codecov.io/gh/apache/superset/pull/24301?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvcXVlcmllcy9zYXZlZF9xdWVyaWVzL2Rhby5weQ==) | `54.28% <38.46%> (-32.08%)` | :arrow_down: |
   | [superset/queries/saved\_queries/api.py](https://app.codecov.io/gh/apache/superset/pull/24301?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvcXVlcmllcy9zYXZlZF9xdWVyaWVzL2FwaS5weQ==) | `60.90% <68.75%> (-31.48%)` | :arrow_down: |
   | [superset/views/sql\_lab/views.py](https://app.codecov.io/gh/apache/superset/pull/24301?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvdmlld3Mvc3FsX2xhYi92aWV3cy5weQ==) | `59.88% <83.33%> (-3.94%)` | :arrow_down: |
   | ... and [4 more](https://app.codecov.io/gh/apache/superset/pull/24301?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | |
   
   ... and [352 files with indirect coverage changes](https://app.codecov.io/gh/apache/superset/pull/24301/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
   
   :mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?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


[GitHub] [superset] Antonio-RiveroMartnez commented on a diff in pull request #24301: fix: Ability to share saved queries

Posted by "Antonio-RiveroMartnez (via GitHub)" <gi...@apache.org>.
Antonio-RiveroMartnez commented on code in PR #24301:
URL: https://github.com/apache/superset/pull/24301#discussion_r1220248265


##########
superset-frontend/src/SqlLab/types.ts:
##########
@@ -49,6 +49,7 @@ export interface QueryEditor {
   selectedText?: string;
   queryLimit?: number;
   description?: string;
+  uuid: string | null;

Review Comment:
   Could we use an optional string like the ones 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


[GitHub] [superset] eschutho commented on a diff in pull request #24301: fix: Ability to share saved queries

Posted by "eschutho (via GitHub)" <gi...@apache.org>.
eschutho commented on code in PR #24301:
URL: https://github.com/apache/superset/pull/24301#discussion_r1228645952


##########
superset/queries/saved_queries/dao.py:
##########
@@ -44,3 +47,20 @@ def bulk_delete(models: Optional[list[SavedQuery]], commit: bool = True) -> None
         except SQLAlchemyError as ex:
             db.session.rollback()
             raise DAODeleteFailedError() from ex
+
+    @classmethod
+    def get_by_id(cls, _id: str) -> Optional[SavedQuery]:

Review Comment:
   @michael-s-molina or @john-bodley given the discussion in the all hands meetup, did we come to a decision on whether we can move towards UUIDs here and save the permalink feature for a moment in time state that you want to share?



-- 
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


[GitHub] [superset] john-bodley commented on a diff in pull request #24301: fix: Ability to share saved queries

Posted by "john-bodley (via GitHub)" <gi...@apache.org>.
john-bodley commented on code in PR #24301:
URL: https://github.com/apache/superset/pull/24301#discussion_r1231653460


##########
superset/queries/saved_queries/dao.py:
##########
@@ -44,3 +47,20 @@ def bulk_delete(models: Optional[list[SavedQuery]], commit: bool = True) -> None
         except SQLAlchemyError as ex:
             db.session.rollback()
             raise DAODeleteFailedError() from ex
+
+    @classmethod
+    def get_by_id(cls, _id: str) -> Optional[SavedQuery]:

Review Comment:
   @eschutho I’m not sure that the UUID seems correct both from a security and consistency perspective.
   
   It feels like an internal representation that we’re now exposing to circumvent security, i.e., it feels akin to saying users enter a bar and present their ID for verification, but if you wave a UUID everything is fine. Granted it’s harder to forge a UUID as opposed to guess a valid ID number, but that still doesn’t see secure.
   
   In my opinion the precedence which was set previously (the example @jfrag1 linked to—which is the sam one I was referring to in the town hall) should be thought of as the exception rather than the rule, i.e., that logic would also need to be updated in a future PR.



-- 
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


[GitHub] [superset] michael-s-molina commented on a diff in pull request #24301: fix: Ability to share saved queries

Posted by "michael-s-molina (via GitHub)" <gi...@apache.org>.
michael-s-molina commented on code in PR #24301:
URL: https://github.com/apache/superset/pull/24301#discussion_r1232179622


##########
superset/queries/saved_queries/dao.py:
##########
@@ -44,3 +47,20 @@ def bulk_delete(models: Optional[list[SavedQuery]], commit: bool = True) -> None
         except SQLAlchemyError as ex:
             db.session.rollback()
             raise DAODeleteFailedError() from ex
+
+    @classmethod
+    def get_by_id(cls, _id: str) -> Optional[SavedQuery]:

Review Comment:
   If we think we need to change the way we identify resources because of security or portability concerns, we need to write a SIP about it because it will greatly impact the application (URL representation, APIs, storage, etc). We also need to check if UUIDs meet the security criteria or if we need a more secure approach that is also portable.
   
   UUIDs are merely resource identifiers, similar to a sequential ID. They shouldn't have a different meaning in terms of security like @john-bodley mentioned 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


[GitHub] [superset] jfrag1 commented on a diff in pull request #24301: fix: Ability to share saved queries

Posted by "jfrag1 (via GitHub)" <gi...@apache.org>.
jfrag1 commented on code in PR #24301:
URL: https://github.com/apache/superset/pull/24301#discussion_r1220113720


##########
superset/queries/saved_queries/api.py:
##########
@@ -174,6 +182,52 @@ def pre_add(self, item: SavedQuery) -> None:
     def pre_update(self, item: SavedQuery) -> None:
         self.pre_add(item)
 
+    @expose("/<id>", methods=("GET",))
+    @protect()
+    @safe
+    @permission_name("get")
+    @statsd_metrics
+    @event_logger.log_this_with_context(
+        action=lambda self, *args, **kwargs: f"{self.__class__.__name__}.get",
+        log_to_statsd=False,
+    )
+    def get(self, id: str, **kwargs: Any) -> Response:

Review Comment:
   endpoint was previously FAB-generated



-- 
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


[GitHub] [superset] john-bodley commented on a diff in pull request #24301: fix: Ability to share saved queries

Posted by "john-bodley (via GitHub)" <gi...@apache.org>.
john-bodley commented on code in PR #24301:
URL: https://github.com/apache/superset/pull/24301#discussion_r1221911649


##########
superset-frontend/src/SqlLab/components/ShareSqlLabQuery/index.tsx:
##########
@@ -76,10 +77,10 @@ const ShareSqlLabQuery = ({
   const getCopyUrlForSavedQuery = (callback: Function) => {
     let savedQueryToastContent;
 
-    if (remoteId) {
+    if (uuid) {
       savedQueryToastContent = `${
         window.location.origin + window.location.pathname
-      }?savedQueryId=${remoteId}`;
+      }?savedQueryId=${uuid}`;

Review Comment:
   Is there a concern that by switching to UUID existing URLs will break?



##########
superset-frontend/src/pages/SavedQueryList/index.tsx:
##########
@@ -216,10 +216,10 @@ function SavedQueryList({
   };
 
   const copyQueryLink = useCallback(
-    (id: number) => {
+    (uuid: string) => {

Review Comment:
   Does TypeScript support a UUID type?



##########
superset/queries/saved_queries/api.py:
##########
@@ -174,6 +182,52 @@ def pre_add(self, item: SavedQuery) -> None:
     def pre_update(self, item: SavedQuery) -> None:
         self.pre_add(item)
 
+    @expose("/<_id>", methods=("GET",))
+    @protect()
+    @safe
+    @permission_name("get")
+    @statsd_metrics
+    @event_logger.log_this_with_context(
+        action=lambda self, *args, **kwargs: f"{self.__class__.__name__}.get",
+        log_to_statsd=False,
+    )
+    def get(self, _id: str, **kwargs: Any) -> Response:

Review Comment:
   ```suggestion
       def get(self, id_: str, **kwargs: Any) -> Response:
   ```
   
   The convention is to add (rather than prepend) an underscore to variable names which conflict with the global namespace.
   
   Additionally can't the ID be an integer (rather than a string) or UUID.



##########
superset/queries/saved_queries/dao.py:
##########
@@ -44,3 +47,20 @@ def bulk_delete(models: Optional[list[SavedQuery]], commit: bool = True) -> None
         except SQLAlchemyError as ex:
             db.session.rollback()
             raise DAODeleteFailedError() from ex
+
+    @classmethod
+    def get_by_id(cls, _id: str) -> Optional[SavedQuery]:

Review Comment:
   I don't believe this is the right approach, i.e., per this code block the UUID option is bypass the security check (by way of the filter on line #63). The correct approach would be to generate a permalink similar to charts and dashboards.
   
   cc @michael-s-molina and @villebro as we've discussed this previously.



-- 
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


[GitHub] [superset] john-bodley commented on a diff in pull request #24301: fix: Ability to share saved queries

Posted by "john-bodley (via GitHub)" <gi...@apache.org>.
john-bodley commented on code in PR #24301:
URL: https://github.com/apache/superset/pull/24301#discussion_r1232888725


##########
superset/queries/saved_queries/dao.py:
##########
@@ -44,3 +47,20 @@ def bulk_delete(models: Optional[list[SavedQuery]], commit: bool = True) -> None
         except SQLAlchemyError as ex:
             db.session.rollback()
             raise DAODeleteFailedError() from ex
+
+    @classmethod
+    def get_by_id(cls, _id: str) -> Optional[SavedQuery]:

Review Comment:
   @zephyring, @jfrag1, @eschutho, et al. I'm supportive of "reverting" the original PR to get us back to square one and then with said SIP we can proceed. I suspect the old API endpoint is a legacy [FAB REST API](https://flask-appbuilder.readthedocs.io/en/latest/quickhowto.html#rest-api) which was slated for deletion in 2.3.X (we're now using FAB 4.3.2+).
   
   If the one line revert to the old API isn't via I think the alternative is something akin to what we do at Airbnb, where our PR description states:
   
   > Though this logic,
   >
   >
   > ```python
   > class SavedQueryFilter(BaseFilter):  # pylint: disable=too-few-public-methods
   >    def apply(self, query: BaseQuery, value: Any) -> BaseQuery:
   >        """
   >        Filter saved queries to only those created by current user.
   >
   >        :returns: flask-sqlalchemy query
   >        """
   >        
   >        return query.filter(
   >            SavedQuery.created_by == g.user  # pylint: disable=comparison-with-callable
   >        )
   >```
   >which, restricts only the creator of the saved query to access it, has been around for [years](https://github.com/apache/superset/pull/10793) we suspect it only came into force when the /api/v1/saved_query/{pk} API was enabled in Q4 of 2022 which prevented people from sharing saved queries.
   > 
   > This PR simply relaxes the filter to allow anyone to access a saved query. 
   
   Thus I _think_ an alternative to changing the API endpoint would be to simply replace [these lines](https://github.com/apache/superset/blob/884a8b52d6a8535ae54c7dab3bfcad3388658877/superset/queries/saved_queries/filters.py#L78-L80) with `return query`.



##########
superset/queries/saved_queries/dao.py:
##########
@@ -44,3 +47,20 @@ def bulk_delete(models: Optional[list[SavedQuery]], commit: bool = True) -> None
         except SQLAlchemyError as ex:
             db.session.rollback()
             raise DAODeleteFailedError() from ex
+
+    @classmethod
+    def get_by_id(cls, _id: str) -> Optional[SavedQuery]:

Review Comment:
   @zephyring, @jfrag1, @eschutho, @michael-s-molina, et al. I'm supportive of "reverting" the original PR to get us back to square one and then with said SIP we can proceed. I suspect the old API endpoint is a legacy [FAB REST API](https://flask-appbuilder.readthedocs.io/en/latest/quickhowto.html#rest-api) which was slated for deletion in 2.3.X (we're now using FAB 4.3.2+).
   
   If the one line revert to the old API isn't via I think the alternative is something akin to what we do at Airbnb, where our PR description states:
   
   > Though this logic,
   >
   >
   > ```python
   > class SavedQueryFilter(BaseFilter):  # pylint: disable=too-few-public-methods
   >    def apply(self, query: BaseQuery, value: Any) -> BaseQuery:
   >        """
   >        Filter saved queries to only those created by current user.
   >
   >        :returns: flask-sqlalchemy query
   >        """
   >        
   >        return query.filter(
   >            SavedQuery.created_by == g.user  # pylint: disable=comparison-with-callable
   >        )
   >```
   >which, restricts only the creator of the saved query to access it, has been around for [years](https://github.com/apache/superset/pull/10793) we suspect it only came into force when the /api/v1/saved_query/{pk} API was enabled in Q4 of 2022 which prevented people from sharing saved queries.
   > 
   > This PR simply relaxes the filter to allow anyone to access a saved query. 
   
   Thus I _think_ an alternative to changing the API endpoint would be to simply replace [these lines](https://github.com/apache/superset/blob/884a8b52d6a8535ae54c7dab3bfcad3388658877/superset/queries/saved_queries/filters.py#L78-L80) with `return query`.



-- 
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


[GitHub] [superset] jfrag1 commented on a diff in pull request #24301: fix: Ability to share saved queries

Posted by "jfrag1 (via GitHub)" <gi...@apache.org>.
jfrag1 commented on code in PR #24301:
URL: https://github.com/apache/superset/pull/24301#discussion_r1232481129


##########
superset/queries/saved_queries/dao.py:
##########
@@ -44,3 +47,20 @@ def bulk_delete(models: Optional[list[SavedQuery]], commit: bool = True) -> None
         except SQLAlchemyError as ex:
             db.session.rollback()
             raise DAODeleteFailedError() from ex
+
+    @classmethod
+    def get_by_id(cls, _id: str) -> Optional[SavedQuery]:

Review Comment:
   I strongly disagree with the idea that there's no justification for UUID's being treated differently for security.  
   > Granted it’s harder to forge a UUID as opposed to guess a valid ID number, but that still doesn’t seem secure.
   
   Guesswork has nothing to do with it; sequential ID's are vulnerable to enumeration, meaning a user of the API could easily get access to all saved queries. This is the case with the old API with no security filter. Consider a script like the following:
   ```python
   for i in range(1000):
      res = requests.get(f'savedqueryapi/get/{i}')
      # save response data somewhere
   ```
   UUID's on the other hand, aren't "harder" to guess, they're _impossible_ to guess.  In the v1 API, the only way to obtain a saved query's UUID is by being logged in as the user who created the saved query, so the contents of the saved query are as secure as the user keeps the UUID/link to it.  I'd argue that this was the intent that saved queries were created with.  It's the way they've worked until [this change](https://github.com/apache/superset/pull/21682/files#diff-1fe9f23910bda67f66ff7b5e9127cdaba0d304153a5246dbcb2d76d70d2e5a4fR1365) was made. There's a "Copy link" button prominently in the UI, which seems to suggest that the idea is that you can then send that link to someone else to view.
   
   While yes, the old API which enabled this functionality was not secure because it allowed enumeration, it was still a great feature to use.  I believe this PR brings the saved query feature back to what it was originally supposed to be in a much more secure way than before.
   



-- 
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


[GitHub] [superset] michael-s-molina closed pull request #24301: fix: Ability to share saved queries

Posted by "michael-s-molina (via GitHub)" <gi...@apache.org>.
michael-s-molina closed pull request #24301: fix: Ability to share saved queries
URL: https://github.com/apache/superset/pull/24301


-- 
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