Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: table schema permissions #23356

Merged
merged 7 commits into from
Mar 14, 2023
Merged

fix: table schema permissions #23356

merged 7 commits into from
Mar 14, 2023

Conversation

betodealmeida
Copy link
Member

@betodealmeida betodealmeida commented Mar 14, 2023

SUMMARY

When a user runs a query in SQL Lab referencing a table without an explicit schema, we check permissions using the query schema. For most databases this will allow users to query schemas that they shouldn't have access to. This happens because the SQLAlchemy connection is never changed to have the query schema set as the default schema in which the query runs; for these databases, we should check permissions using the default database schema instead.

In this example with Postgres, a user has access to only the secret schema. They select the secret schema in SQL Lab and run the following query:

Screen Shot 2023-03-13 at 5 52 50 PM

SELECT * FROM ab_user

The database will run the query against the default schema, public, returning data. Superset will check if the user has access to the secret schema instead, allowing the query to run and returning results to the user:

Screen Shot 2023-03-13 at 5 40 40 PM

With this fix the user is restricted:

Screen Shot 2023-03-13 at 5 52 10 PM

TESTING INSTRUCTIONS

  1. Add a schema called secret to a database.
  2. Create a role called secret that has the permission [schema access on [PostgreSQL].[secret]].
  3. Add a test user with roles sql_lab and secret.
  4. As the test user, select the secret schema in SQL Lab and run SELECT * FROM ab_user.

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

@codecov
Copy link

codecov bot commented Mar 14, 2023

Codecov Report

Merging #23356 (1ec96f6) into master (d415eed) will increase coverage by 0.00%.
The diff coverage is 77.02%.

❗ Current head 1ec96f6 differs from pull request most recent head f700132. Consider uploading reports for the commit f700132 to get more accurate results

@@           Coverage Diff           @@
##           master   #23356   +/-   ##
=======================================
  Coverage   67.52%   67.52%           
=======================================
  Files        1907     1907           
  Lines       73445    73457   +12     
  Branches     7975     7976    +1     
=======================================
+ Hits        49591    49602   +11     
  Misses      21806    21806           
- Partials     2048     2049    +1     
Flag Coverage Δ
hive 52.77% <51.51%> (+<0.01%) ⬆️
mysql 78.43% <78.78%> (+<0.01%) ⬆️
postgres 78.49% <81.81%> (+<0.01%) ⬆️
presto 52.70% <51.51%> (+<0.01%) ⬆️
python 82.27% <93.93%> (+<0.01%) ⬆️
sqlite 76.97% <81.81%> (+<0.01%) ⬆️
unit 52.48% <51.51%> (+0.01%) ⬆️

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

Impacted Files Coverage Δ
...rt-controls/src/shared-controls/customControls.tsx 17.39% <0.00%> (ø)
...frontend/src/components/DatabaseSelector/index.tsx 91.66% <ø> (ø)
...et-frontend/src/components/TableSelector/index.tsx 79.22% <ø> (ø)
...ilters/FilterBar/FilterControls/FilterControls.tsx 69.33% <0.00%> (ø)
...Filters/FilterBar/FiltersDropdownContent/index.tsx 20.00% <0.00%> (ø)
...tersConfigModal/FiltersConfigForm/ColumnSelect.tsx 77.14% <0.00%> (+0.21%) ⬆️
...ersConfigModal/FiltersConfigForm/DatasetSelect.tsx 35.29% <ø> (-4.71%) ⬇️
...rsConfigModal/FiltersConfigForm/DependencyList.tsx 52.45% <ø> (ø)
superset-frontend/src/views/components/Menu.tsx 49.23% <ø> (ø)
...end/src/visualizations/TimeTable/transformProps.ts 0.00% <ø> (ø)
... and 65 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@betodealmeida betodealmeida merged commit 1b95da7 into master Mar 14, 2023
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 3.0.0 labels Mar 13, 2024
@mistercrunch mistercrunch deleted the schema_restriction branch March 26, 2024 16:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels size/L 🚢 3.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants