fix: handle empty catalog when DB supports them #29840

betodealmeida · 2024-08-02T22:04:50Z

SUMMARY

Handle edge cases when catalog is passed as null but the database supports catalog. This happens when "Allow changing catalogs" is disabled in the database. In these cases we need to use the default catalog, so that permissions checks work as expected.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

N/A

TESTING INSTRUCTIONS

Add a database with catalogs (eg, Postgres)

Catalog permissions are created (eg, [Postgres].[superset])
Schema permissions are created (eg, [Postgres].[superset].[public])

Create a user A with catalog-level permissions to the default catalog

User can see schemas in SQL Lab
User can choose schema in SQL Lab
User can choose table in SQL Lab
User can run queries
User can create a new dataset
User can see charts built from the dataset

Create a user B with schema-level permissions to the default catalog

User can see only 1 schema in SQL Lab
User can choose table in SQL Lab
User can run queries
User can create a new dataset
User can see charts built from the dataset

Create a user C with catalog-level permissions to a non-default catalog

User can't see schemas/tables in SQL Lab
User can't run queries in SQL Lab

Create a user D with schema-level permissions to a non-default catalog

User can't see schemas/tables in SQL Lab
User can't run queries in SQL Lab

Enable multi-catalog

Admin should see all catalogs
A, B, C, D should only see the catalogs they have access to
Repeat steps from (2)

ADDITIONAL INFORMATION

Has associated issue:
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

rusackas · 2024-08-05T16:47:13Z

I just put this in draft mode whilst it doesn't have a title/description :) Best of luck with whatever it is! :D

betodealmeida · 2024-08-06T22:51:15Z

scripts/change_detector.py

@@ -52,7 +52,7 @@
 def fetch_files_github_api(url: str):  # type: ignore
    """Fetches data using GitHub API."""
    req = Request(url)
-    req.add_header("Authorization", f"token {GITHUB_TOKEN}")
+    req.add_header("Authorization", f"Bearer {GITHUB_TOKEN}")


Note: In most cases, you can use Authorization: Bearer or Authorization: token to pass a token. However, if you are passing a JSON web token (JWT), you must use Authorization: Bearer. [source]

sadpandajoe · 2024-08-07T19:00:28Z

@supersetbot label 4.1

Vitor-Avila · 2024-08-09T18:48:13Z

superset/databases/api.py

@@ -1159,7 +1159,7 @@ def select_star(
        self.incr_stats("init", self.select_star.__name__)
        try:
            result = database.select_star(
-                Table(table_name, schema_name),
+                Table(table_name, schema_name, database.get_default_catalog()),


Should catalog be added to the API path as well (when the table is not from the default one)? Not related with this PR, just asking if it's something we need to do.

Yeah, but I I couldn't find any API calls to this endpoint — it seems like we return this in the database request now, instead of calling /select_star/, which is why I didn't change the code to pass a parameter. I think we should keep this endpoint until the next major version and then get rid of it.

gotcha! thank you

Vitor-Avila

LGTM!

Vitor-Avila · 2024-08-09T18:53:51Z

tests/integration_tests/databases/api_tests.py

@@ -1563,34 +1563,6 @@ def test_get_select_star_not_allowed(self):
        rv = self.client.get(uri)
        self.assertEqual(rv.status_code, 404)

-    def test_get_select_star_datasource_access(self):


Non-blocking comment: would be good if you could mention why these tests were removed.

These tests were poorly written and relied on weird side effects.

(cherry picked from commit 39209c2)

WIP

1618903

pull-request-size bot added the size/M label Aug 2, 2024

Fixing tests

944df5c

pull-request-size bot added size/L and removed size/M labels Aug 4, 2024

rusackas marked this pull request as draft August 5, 2024 16:46

betodealmeida changed the title ~~WIP~~ fix: handle empty catalog when DB supports them Aug 6, 2024

github-actions bot added the preset-io label Aug 6, 2024

betodealmeida added 2 commits August 6, 2024 11:42

Fix more tests

23dc294

Fix more tests

fd9290a

michael-s-molina added the review:draft label Aug 6, 2024

Remove broken tests

03059e8

github-actions bot added the api Related to the REST API label Aug 6, 2024

Fix SQL Lab

5e7d979

betodealmeida force-pushed the use-default-catalog branch from 013d840 to 5e7d979 Compare August 6, 2024 20:26

Fix cache keys

9023b48

betodealmeida force-pushed the use-default-catalog branch from c03577f to 9023b48 Compare August 6, 2024 21:22

betodealmeida marked this pull request as ready for review August 6, 2024 21:49

dosubot bot added authentication:access-control Rlated to access control data Namespace | Anything related to data, including databases configurations, datasets, etc. labels Aug 6, 2024

betodealmeida commented Aug 6, 2024

View reviewed changes

Undo test change

c5a9797

github-actions bot added the v4.1 Label added by the release manager to track PRs to be included in the 4.1 branch label Aug 7, 2024

betodealmeida mentioned this pull request Aug 9, 2024

fix: set default catalog #29608

Closed

9 tasks

Vitor-Avila reviewed Aug 9, 2024

View reviewed changes

Vitor-Avila approved these changes Aug 9, 2024

View reviewed changes

Vitor-Avila reviewed Aug 9, 2024

View reviewed changes

betodealmeida merged commit 39209c2 into master Aug 13, 2024
35 checks passed

sadpandajoe pushed a commit that referenced this pull request Aug 13, 2024

fix: handle empty catalog when DB supports them (#29840)

9677fa9

(cherry picked from commit 39209c2)

rusackas deleted the use-default-catalog branch September 27, 2024 20:55

github-actions bot added 🍒 4.1.0 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels labels Nov 14, 2024

mistercrunch added the 🍒 4.1.1 label Nov 27, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: handle empty catalog when DB supports them #29840

fix: handle empty catalog when DB supports them #29840

betodealmeida commented Aug 2, 2024 •

edited

Loading

rusackas commented Aug 5, 2024

betodealmeida Aug 6, 2024

Vitor-Avila Aug 9, 2024

sadpandajoe commented Aug 7, 2024

Vitor-Avila Aug 9, 2024

betodealmeida Aug 13, 2024

Vitor-Avila Aug 13, 2024

Vitor-Avila left a comment

Vitor-Avila Aug 9, 2024

betodealmeida Aug 13, 2024

Vitor-Avila Aug 13, 2024

fix: handle empty catalog when DB supports them #29840

fix: handle empty catalog when DB supports them #29840

Conversation

betodealmeida commented Aug 2, 2024 • edited Loading

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

rusackas commented Aug 5, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sadpandajoe commented Aug 7, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Vitor-Avila left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

betodealmeida commented Aug 2, 2024 •

edited

Loading