fix(sql lab): SQL Lab access restrictions not applied to default schema #20285

diegomedina248 · 2022-06-06T20:20:23Z

SUMMARY

The schema level restrictions are not working properly in SQL Lab, when the default schema is used. Some DB Engines allow users to execute a query without specifying the Schema, in which default Schema will be queried.

In the permission check, we're assigning the schema from two sources: the query itself, or if it's not present, the schema selected on the left hand side in SQL Lab.
It's the second scenario that's not properly checked, because, if the user has schema permission for the one selected, and if the query can be executed in the engine without specifying the schema explicitly, then the permission check will allow said execution, since there's a permission for the schema.

This PR alters the order of checks, so that we validate that the default schema actually contains the table we're trying to query, and then check the permission on said datasource.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Before:

Screen.Recording.2022-06-06.at.17.31.02.mov

After:

Screen.Recording.2022-06-06.at.17.32.41.mov

TESTING INSTRUCTIONS

Create a new user role with the following permissions:

can read on SavedQuery
can write on SavedQuery
can read on Database
can read on Query
can sqllab on Superset
can sqllab history on Superset
can sqllab viz on Superset
can sql json on Superset
can sqllab table viz on Superset
can validate sql json on Superset
can activate on TabStateView
can get on TabStateView
can put on TabStateView
can delete query on TabStateView
can post on TabStateView
can delete on TabStateView
menu access on SQL Lab
menu access on SQL Editor
schema access on [examples].[information_schema]

Create a new user and assign the above role to it.
Log in with that user.
Go to SQL Lab
Select examples as database and information_schema as schema

Execute these two queries:

select * from birth_names

select * from public.birth_names

Ensure both fail with a forbidden error.

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

diegomedina248 · 2022-06-06T20:25:07Z

superset/security/manager.py

@@ -1054,19 +1054,24 @@ def raise_for_access(
            denied = set()

            for table_ in tables:
-                schema_perm = self.get_schema_perm(database, schema=table_.schema)


table._schema could be the one defined in the query itself, or the one selected on the left dropdown.
For that reason, we could be fetching the permission for a schema that does not contain the table, and validate the request (if the user has permission to that schema).
So, we invert the request order here, by querying all the datasources for the database/table combination, and then matching the schema. If there's a match, then we check the permission on that schema.

codecov · 2022-06-07T00:26:21Z

Codecov Report

Merging #20285 (2e63d50) into master (6b0bb80) will decrease coverage by 11.60%.
The diff coverage is 75.00%.

❗ Current head 2e63d50 differs from pull request most recent head 3c7ae32. Consider uploading reports for the commit 3c7ae32 to get more accurate results

@@             Coverage Diff             @@
##           master   #20285       +/-   ##
===========================================
- Coverage   66.83%   55.23%   -11.61%     
===========================================
  Files        1750     1750               
  Lines       65894    65899        +5     
  Branches     7017     7017               
===========================================
- Hits        44041    36399     -7642     
- Misses      20067    27714     +7647     
  Partials     1786     1786

Flag	Coverage Δ
hive	`53.96% <58.33%> (+0.01%)`	⬆️
mysql	`?`
postgres	`?`
presto	`53.82% <58.33%> (+0.01%)`	⬆️
python	`58.80% <75.00%> (-24.19%)`	⬇️
sqlite	`?`
unit	`51.03% <66.66%> (-0.01%)`	⬇️

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

Impacted Files	Coverage Δ
superset/security/manager.py	`64.58% <75.00%> (-30.97%)`	⬇️
superset/utils/dashboard_import_export.py	`0.00% <0.00%> (-100.00%)`	⬇️
superset/key_value/commands/update.py	`0.00% <0.00%> (-88.89%)`	⬇️
superset/key_value/commands/delete.py	`0.00% <0.00%> (-85.30%)`	⬇️
superset/key_value/commands/delete_expired.py	`0.00% <0.00%> (-80.77%)`	⬇️
superset/dashboards/commands/importers/v0.py	`15.62% <0.00%> (-76.25%)`	⬇️
superset/datasets/commands/update.py	`25.30% <0.00%> (-68.68%)`	⬇️
superset/datasets/commands/create.py	`29.41% <0.00%> (-68.63%)`	⬇️
superset/datasets/commands/importers/v0.py	`24.03% <0.00%> (-67.45%)`	⬇️
superset/reports/commands/execute.py	`24.45% <0.00%> (-67.16%)`	⬇️
... and 276 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6b0bb80...3c7ae32. Read the comment docs.

eschutho

I found test case where this won't work.

eschutho · 2022-07-13T23:02:50Z

superset/security/manager.py

-                        self.get_session, database, table_.table, schema=table_.schema
-                    )
+                # Access to any datasource is suffice.
+                for datasource_ in datasources:


@diegomedina248 I tested this out what I found is the way this reads now, if datasources is empty, i.e., there are no datasets created for this table, they will hit line 1074 and not get access to the schema.

Good catch! I amended the PR to account for it.

Though.. I think the same happens with the current code, unless I'm reading it wrong

rusackas · 2022-07-27T18:43:05Z

@eschutho if you wouldn't mind checking/accepting/amending the change requests, that would be much appreciated. Looks like this'll need a little rebase action as well :D

eschutho · 2022-07-29T00:34:37Z

I just checked with @yousoph on whether we should be fetching from the schema in the dropdown when not specified in the sql statement, which we're not doing.

rusackas · 2024-02-01T22:06:49Z

Fixed via #23356 I believe (hat tip to @eschutho for the reference)

superset-github-bot bot added the Preset-Patch label Jun 6, 2022

pull-request-size bot added the size/S label Jun 6, 2022

diegomedina248 commented Jun 6, 2022

View reviewed changes

diegomedina248 marked this pull request as ready for review June 6, 2022 20:50

diegomedina248 force-pushed the fix/sql-lab-access-role-not-applied-to-default-schema branch 2 times, most recently from ac26cc5 to da23b76 Compare June 7, 2022 00:03

pull-request-size bot added size/M and removed size/S labels Jun 7, 2022

diegomedina248 force-pushed the fix/sql-lab-access-role-not-applied-to-default-schema branch 2 times, most recently from ec3638d to 0c424f7 Compare June 7, 2022 00:19

diegomedina248 force-pushed the fix/sql-lab-access-role-not-applied-to-default-schema branch 19 times, most recently from d00af63 to 6847399 Compare June 7, 2022 17:32

diegomedina248 force-pushed the fix/sql-lab-access-role-not-applied-to-default-schema branch 4 times, most recently from 9cdd673 to 0869f44 Compare June 7, 2022 23:02

rusackas requested review from eschutho, betodealmeida and dpgaspar June 8, 2022 20:35

eschutho requested changes Jul 13, 2022

View reviewed changes

diegomedina248 force-pushed the fix/sql-lab-access-role-not-applied-to-default-schema branch 6 times, most recently from 56dc739 to ebc8333 Compare July 14, 2022 06:44

diegomedina248 added 2 commits July 14, 2022 10:27

fix(sql lab): SQL Lab access restrictions not applied to default schema

09fc0fc

PR comments

3c7ae32

diegomedina248 force-pushed the fix/sql-lab-access-role-not-applied-to-default-schema branch from ebc8333 to 3c7ae32 Compare July 14, 2022 13:27

rusackas closed this Feb 1, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(sql lab): SQL Lab access restrictions not applied to default schema #20285

fix(sql lab): SQL Lab access restrictions not applied to default schema #20285

diegomedina248 commented Jun 6, 2022 •

edited

Loading

diegomedina248 Jun 6, 2022

codecov bot commented Jun 7, 2022 •

edited

Loading

eschutho left a comment

eschutho Jul 13, 2022

diegomedina248 Jul 14, 2022

diegomedina248 Jul 14, 2022

rusackas commented Jul 27, 2022

eschutho commented Jul 29, 2022

rusackas commented Feb 1, 2024

fix(sql lab): SQL Lab access restrictions not applied to default schema #20285

fix(sql lab): SQL Lab access restrictions not applied to default schema #20285

Conversation

diegomedina248 commented Jun 6, 2022 • edited Loading

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

diegomedina248 Jun 6, 2022

Choose a reason for hiding this comment

codecov bot commented Jun 7, 2022 • edited Loading

Codecov Report

eschutho left a comment

Choose a reason for hiding this comment

eschutho Jul 13, 2022

Choose a reason for hiding this comment

diegomedina248 Jul 14, 2022

Choose a reason for hiding this comment

diegomedina248 Jul 14, 2022

Choose a reason for hiding this comment

rusackas commented Jul 27, 2022

eschutho commented Jul 29, 2022

rusackas commented Feb 1, 2024

diegomedina248 commented Jun 6, 2022 •

edited

Loading

codecov bot commented Jun 7, 2022 •

edited

Loading