refactor(Shared_url_query): Fix shared query URL access for SQL Lab users.

[LevisNgigi]

fix(sql-lab): fix the copy url to use the permalink api and allow query sharing for users with appropriate permissions.

SUMMARY

This update fixes the copy sharing to use the newly introduced permalink.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

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 with sqllab_role can successfully load the shared query.

ADDITIONAL INFORMATION

• Has associated issue: Fixes Unable to share saved SQL queries - user gets an error wen trying to load the query #31367
• Required feature flags:
• Changes UI
• Includes DB Migration (follow approval process in SIP-59)
  • 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

[codecov]

Codecov Report

Attention: Patch coverage is 83.33333% with 4 lines in your changes missing coverage. Please review.

Project coverage is 83.45%. Comparing base (76d897e) to head (bb871c6).
Report is 1337 commits behind head on master.

Files with missing lines	Patch %	Lines
superset/queries/saved_queries/api.py	83.33%	4 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           master   #31421       +/-   ##
===========================================
+ Coverage   60.48%   83.45%   +22.97%     
===========================================
  Files        1931      546     -1385     
  Lines       76236    39273    -36963     
  Branches     8568        0     -8568     
===========================================
- Hits        46114    32777    -13337     
+ Misses      28017     6496    -21521     
+ Partials     2105        0     -2105     

Flag	Coverage Δ
hive	48.84% <45.83%> (-0.32%)	⬇️
javascript	?
mysql	76.03% <83.33%> (?)
postgres	76.10% <83.33%> (?)
presto	53.34% <45.83%> (-0.47%)	⬇️
python	83.45% <83.33%> (+19.97%)	⬆️
sqlite	75.57% <83.33%> (?)
unit	61.05% <45.83%> (+3.43%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[rusackas]

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.

[villebro]

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.

[LevisNgigi]

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

[villebro]

Thanks for the clarification @LevisNgigi 👍

[LevisNgigi]

Thanks for the clarification @LevisNgigi 👍

@villebro Much Welcome.

[justinpark]

@LevisNgigi Has there has been discussion about making queries shareable via permalinks (using "copy link")?

[LevisNgigi]

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

[geido]

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!

[LevisNgigi]

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.

[dpgaspar]

This looks like a duplicate class definition inside the same class

[dpgaspar]

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

[LevisNgigi]

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 left until query sharing is added in Superset?

[geido]

@LevisNgigi how hard would it be to implement the UUID approach now?

[LevisNgigi]

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

[legiondean]

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

[LevisNgigi]

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