Repository navigation
fix(sqllab): let schema grants authorize SQL Lab again - #44968
AmoghAtreya wants to merge 13 commits into
Conversation
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
The review comment is correct. The documentation in To resolve this, the sentence should be updated to clarify that - `schema_access` and `catalog_access` let the user browse that schema in SQL Lab and still authorize charts, but they do not authorize query execution.
+ `schema_access` lets the user browse that schema in SQL Lab, and `catalog_access` lets the user browse the catalog, and both still authorize charts, but they do not authorize query execution.I have checked the other comments on this PR. Would you like me to fetch all comments, validate them, and implement fixes for the rest as well? docs/admin_docs/security/security.mdx |
There was a problem hiding this comment.
Code Review Agent Run #1629cf
Actionable Suggestions - 3
-
tests/unit_tests/sqllab/test_requires_dataset_match.py - 3
- Missing test docstring · Line 39-39
- Untyped mock variables · Line 46-49
- Function-local import violates rule · Line 63-63
Additional Suggestions - 4
-
superset/security/manager.py - 1
-
Duplicated error-msg builder · Line 2820-2838`get_dataset_match_access_error_msg` duplicates the structure of `get_table_access_error_msg` (both build `quoted_tables` and format a `%(tables)s` message). Extract the shared quoting/formatting into one helper so the two messages can't drift apart.
-
-
docs/admin_docs/security/security.mdx - 1
-
Doc overstates flag coverage · Line 72-72The doc lists SQL formatting as covered by `SQLLAB_REQUIRE_DATASET_MATCH`, but `SqllabApi.format_sql` (superset/sqllab/api.py:253) only calls `raise_for_access(..., force_dataset_match=requires_dataset_match())` inside `if template_params:`. Formatting without template_params performs no dataset/schema/catalog check at all, so the flag does not gate that case. Consider qualifying the claim.
-
-
tests/integration_tests/security_tests.py - 2
-
Missing local variable annotation · Line 1930-1930BITO.md rule 13347 mandates explicit type annotations on all local variables in test files, including inferable ones. The newly added `message` at line 1930 is unannotated; annotate it as `message: str = str(ctx.exception)` to comply with the project's typing standard.
-
Brittle i18n message assertions · Line 1931-1933These substring assertions match the `_()`-wrapped, translatable message built by `get_dataset_match_access_error_msg` (`superset/security/manager.py:2832`), so they break under a non-English test locale or future rewording. Asserting `ctx.exception.error.error_type` and `error.extra["tables"]` would be locale-independent and also pin the denied table.
-
Review Details
-
Files reviewed - 15 · Commit Range:
d11f470..d11f470- docs/admin_docs/configuration/configuring-superset.mdx
- docs/admin_docs/security/security.mdx
- superset/commands/sql_lab/estimate.py
- superset/config.py
- superset/mcp_service/sql_lab/tool/execute_sql.py
- superset/models/sql_lab.py
- superset/security/manager.py
- superset/sqllab/api.py
- superset/sqllab/utils.py
- superset/sqllab/validators.py
- tests/integration_tests/security_tests.py
- tests/unit_tests/commands/sql_lab/test_estimate.py
- tests/unit_tests/security/manager_test.py
- tests/unit_tests/sqllab/api_test.py
- tests/unit_tests/sqllab/test_requires_dataset_match.py
-
Files skipped - 2
- SECURITY.md - Reason: Filter setting
- UPDATING.md - Reason: Filter setting
-
Tools
- MyPy (Static Code Analysis) - ✔︎ Successful
- Astral Ruff (Static Code Analysis) - ✔︎ Successful
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
Bito Usage Guide
Commands
Type the following command in the pull request comment and save the comment.
-
/review- Manually triggers an incremental AI Review. -
/review full- Manually triggers a full AI Review. -
/pause- Pauses automatic reviews on this pull request. -
/resume- Resumes automatic reviews. -
/resolve- Marks all Bito-posted review comments as resolved. -
/abort- Cancels all in-progress reviews.
Refer to the documentation for additional commands.
Configuration
This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.
Documentation & Help
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #44968 +/- ##
===========================================
+ Coverage 65.35% 84.14% +18.78%
===========================================
Files 3057 3056 -1
Lines 201579 201522 -57
Branches 47149 47106 -43
===========================================
+ Hits 131747 169568 +37821
+ Misses 66866 28711 -38155
- Partials 2966 3243 +277
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Code Review Agent Run #483b91
Actionable Suggestions - 1
-
superset/mcp_service/sql_lab/tool/execute_sql.py - 1
- Redundant double authorization · Line 251-251
Additional Suggestions - 15
-
superset/translations/ca/LC_MESSAGES/messages.po - 1
-
Empty Catalan translation · Line 20733-20739This new msgid (matching `manager.py:2833`) is added with an empty `msgstr ""`, so Catalan users will fall back to the English message. The surrounding backfilled entries carry real translations; this one is incomplete. Add the Catalan translation or drop the entry until it is ready.
-
-
superset/translations/de/LC_MESSAGES/messages.po - 1
-
Untranslated German msgstr · Line 19638-19644The new entry adds a German msgid but leaves `msgstr ""` empty, so German users will fall back to the English text from `superset/security/manager.py` (`get_table_access_error_message`). Fill in the German translation so the localized message is actually delivered.
-
-
superset/translations/zh/LC_MESSAGES/messages.po - 1
-
Empty translation msgstr · Line 20121-20121The new entry for the error message from `superset/security/manager.py:2833` has an empty `msgstr ""`. Chinese users will see the untranslated English text. Fill in the translation before merging.
-
-
superset/translations/fr/LC_MESSAGES/messages.po - 1
-
Missing French translation · Line 20017-20023The new msgid (matching `manager.py` `get_table_access_error_message`) is added with an empty `msgstr ""`, so French users will fall back to the English text. Provide a French translation to complete the localization for this new error message.
-
-
superset/translations/zh_TW/LC_MESSAGES/messages.po - 1
-
Empty translation msgstr · Line 21449-21449New entry's `msgstr` is empty, so zh_TW users see the untranslated English access-denied message from `manager.py:2833`. Per BITO rules [12743]/[13343], fill the msgstr with a verified Traditional Chinese translation before merging.
-
-
superset/translations/lv/LC_MESSAGES/messages.po - 1
-
Missing translation · Line 19643-19649Newly added msgid has an empty `msgstr ""`, so Latvian users will see the untranslated English text for this access-denied message. Fill in the Latvian translation before merging (BITO.md rule 12743).
-
-
superset/translations/es/LC_MESSAGES/messages.po - 1
-
Empty translation msgstr · Line 20613-20619The new entry's `msgstr` is empty, so Spanish users will see the English `msgid` (gettext falls back to the source string). The msgid matches `manager.py:2832` (`get_dataset_match_access_error_msg`) exactly and the `#, python-format` flag is correct, but the translation itself is missing. Fill in the Spanish `msgstr` to complete the entry.
-
-
superset/translations/pt_BR/LC_MESSAGES/messages.po - 1
-
Missing Portuguese translation · Line 21400-21406The new entry's `msgstr` is empty, so pt_BR users will fall back to the English source string from `manager.py` `get_table_access_error_message`. Since this entry was added to the translation file, provide the Portuguese translation to complete it.
-
-
superset/translations/ru/LC_MESSAGES/messages.po - 1
-
Empty translation msgstr · Line 19307-19307New user-facing error message (from `manager.py` `get_table_access_error_message`) is added with an empty `msgstr ""`, so Russian users will see the untranslated English text. Neighboring entries (e.g. line 19298) are translated. Fill in the Russian translation before merging.
-
-
superset/translations/th/LC_MESSAGES/messages.po - 1
-
Empty Thai translation · Line 32017-32017This new entry adds the datasource_access error message (from `manager.py:2833`) but leaves `msgstr ""` empty. Thai users will see the raw English error instead of a translation. Per BITO.md rules 12743/13343, fill in the Thai `msgstr` before merging.
-
-
superset/translations/ar/LC_MESSAGES/messages.po - 1
-
Missing Arabic translation · Line 21337-21337The new entry's `msgstr` is empty, so Arabic users will see the untranslated English error text. The msgid matches the source string in `manager.py` `get_table_access_error_object`. Fill in the Arabic translation before merging (BITO.md rules 12743/13343).
-
-
superset/translations/cs/LC_MESSAGES/messages.po - 1
-
Missing Czech translation · Line 20015-20015The newly added msgid (matching `manager.py:2832` `get_table_access_error_message`) has an empty `msgstr`, so Czech users will see the English fallback for this access-denied message. Add the Czech translation so the new string is actually localized.
-
-
superset/translations/ro/LC_MESSAGES/messages.po - 1
-
Empty msgstr translation · Line 20955-20955New entry for the error message from `superset/security/manager.py:2833` is added with an empty `msgstr ""`, so Romanian users will see the untranslated English text. Per BITO rules 13343/12743, user-facing error messages must have complete translations before merge. Please fill and verify the Romanian `msgstr`.
-
-
superset/translations/pl/LC_MESSAGES/messages.po - 1
-
Missing Polish translation · Line 21706-21706The new msgid (matching `manager.py:2833`) is added with an empty `msgstr`, so Polish users will see the English fallback for this permission-denied message. Provide the Polish translation to complete the localization.
-
-
tests/unit_tests/commands/sql_lab/test_estimate.py - 1
-
Fragile call index · Line 235-235`call_args_list[1]` hardcodes the assumption that exactly two `raise_for_access` calls occur, in a fixed order (validate's source check, then run's rendered check). If `validate()` ever adds another authorization call, this index silently selects the wrong call and the test fails confusingly. `call_args_list[-1]` is equally clear and robust to call-count changes.
-
Filtered by Review Rules
Bito filtered these suggestions based on rules created automatically for your feedback. Manage rules.
-
superset/translations/fi/LC_MESSAGES/messages.po - 1
- Empty msgstr translation · Line 32408-32408
-
superset/translations/tr/LC_MESSAGES/messages.po - 1
- Missing Turkish translation · Line 20475-20481
Review Details
-
Files reviewed - 43 · Commit Range:
d11f470..10892e9- docs/admin_docs/configuration/configuring-superset.mdx
- docs/admin_docs/security/security.mdx
- superset/config.py
- superset/sqllab/utils.py
- superset/commands/sql_lab/estimate.py
- tests/unit_tests/commands/sql_lab/test_estimate.py
- superset/mcp_service/sql_lab/tool/execute_sql.py
- tests/unit_tests/mcp_service/sql_lab/tool/test_execute_sql.py
- tests/unit_tests/sqllab/test_requires_dataset_match.py
- superset/security/manager.py
- tests/unit_tests/security/manager_test.py
- superset/translations/ar/LC_MESSAGES/messages.po
- superset/translations/ca/LC_MESSAGES/messages.po
- superset/translations/cs/LC_MESSAGES/messages.po
- superset/translations/de/LC_MESSAGES/messages.po
- superset/translations/en/LC_MESSAGES/messages.po
- superset/translations/es/LC_MESSAGES/messages.po
- superset/translations/fa/LC_MESSAGES/messages.po
- superset/translations/fi/LC_MESSAGES/messages.po
- superset/translations/fr/LC_MESSAGES/messages.po
- superset/translations/it/LC_MESSAGES/messages.po
- superset/translations/ja/LC_MESSAGES/messages.po
- superset/translations/ko/LC_MESSAGES/messages.po
- superset/translations/lv/LC_MESSAGES/messages.po
- superset/translations/messages.pot
- superset/translations/mi/LC_MESSAGES/messages.po
- superset/translations/nl/LC_MESSAGES/messages.po
- superset/translations/pl/LC_MESSAGES/messages.po
- superset/translations/pt/LC_MESSAGES/messages.po
- superset/translations/pt_BR/LC_MESSAGES/messages.po
- superset/translations/ro/LC_MESSAGES/messages.po
- superset/translations/ru/LC_MESSAGES/messages.po
- superset/translations/sk/LC_MESSAGES/messages.po
- superset/translations/sl/LC_MESSAGES/messages.po
- superset/translations/sr/LC_MESSAGES/messages.po
- superset/translations/sr_Latn/LC_MESSAGES/messages.po
- superset/translations/ta/LC_MESSAGES/messages.po
- superset/translations/th/LC_MESSAGES/messages.po
- superset/translations/tr/LC_MESSAGES/messages.po
- superset/translations/uk/LC_MESSAGES/messages.po
- superset/translations/zh/LC_MESSAGES/messages.po
- superset/translations/zh_TW/LC_MESSAGES/messages.po
- tests/unit_tests/subjects/test_raise_for_access.py
-
Files skipped - 2
- SECURITY.md - Reason: Filter setting
- UPDATING.md - Reason: Filter setting
-
Tools
- MyPy (Static Code Analysis) - ✔︎ Successful
- Astral Ruff (Static Code Analysis) - ✔︎ Successful
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
Bito Usage Guide
Commands
Type the following command in the pull request comment and save the comment.
-
/review- Manually triggers an incremental AI Review. -
/review full- Manually triggers a full AI Review. -
/pause- Pauses automatic reviews on this pull request. -
/resume- Resumes automatic reviews. -
/resolve- Marks all Bito-posted review comments as resolved. -
/abort- Cancels all in-progress reviews.
Refer to the documentation for additional commands.
Configuration
This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.
Documentation & Help
…ccess-opt-out Co-authored-by: Cursor <cursoragent@cursor.com>
CodeAnt PR Risk: Low Risk
Assessed commit: |
…ccess-opt-out Co-authored-by: Cursor <cursoragent@cursor.com>
…ccess-opt-out Co-authored-by: Cursor <cursoragent@cursor.com>
SUMMARY
Fixes #44903. After #40409, SQL Lab required
datasource_accesson a registered dataset for every referenced table. A Gamma user withsql_labplusschema_accessandcatalog_accesscould still browse the schema and open charts, but every SQL Lab query was denied. The error read as if those schema grants were missing, and there was no configuration to restore the previous behavior. Registering every table as a dataset does not scale for schemas where tables are added and renamed often, anddatabase_accessis broader than a single schema.SQLLAB_REQUIRE_DATASET_MATCHdefaults toTrue, so the dataset-match requirement stays on unless an admin opts out. Set it toFalseto authorize SQL Lab fromschema_accessorcatalog_accessalone. That grant then covers every table in the schema, including tables that are not registered as datasets.The flag covers the SQL Lab paths that execute a query or return its rows:
execute_sqltoolCharts and Explore already honor
schema_accessandcatalog_accessand are unchanged. Reports, alerts, MetaDB, and SQL validation keep requiring a dataset match.When the requirement is left on, a denial names the dataset requirement. It says
schema_accessandcatalog_accessdo not authorize the query, instead of reading as if those grants are absent.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A. Backend permission check and configuration only.
TESTING INSTRUCTIONS
sql_labrole and only these data grants:catalog_accesson[Database].[catalog]schema_accesson[Database].[catalog].[schema]database_accessdatasource_accessSQLLAB_REQUIRE_DATASET_MATCHunset (defaultTrue). In SQL Lab, runSELECT * FROM schema.table LIMIT 100. The query is denied. The error says a registered dataset withdatasource_accessis required, and thatschema_accessandcatalog_accessdo not authorize the query.SQLLAB_REQUIRE_DATASET_MATCH = Falseinsuperset_config.pyand restart. Run the same query. It succeeds.True. Grantdatasource_accesson a registered dataset for that table and run the query again. It succeeds.schema_access.pytest tests/unit_tests/sqllab/test_requires_dataset_match.py \ tests/unit_tests/sqllab/api_test.py \ tests/unit_tests/commands/sql_lab/test_estimate.py \ tests/unit_tests/security/manager_test.py \ -k "dataset_match or format_sql or catalog_self_reference"ADDITIONAL INFORMATION