fix(security): grant SQL Lab query authors an explore access bypass (#39296) - #42590
fix(security): grant SQL Lab query authors an explore access bypass (#39296)#42590rusackas wants to merge 10 commits into
Conversation
Code Review Agent Run #0e03dfActionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
|
The suggestion to rename the test is correct, as the current name contains a typo. You can resolve this by renaming the test function in There are no other comments on this pull request to address. tests/unit_tests/explore/utils_test.py |
rusackas
changed the title
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #42590 +/- ##
==========================================
- Coverage 65.57% 65.57% -0.01%
==========================================
Files 2818 2818
Lines 160023 160027 +4
Branches 36556 36557 +1
==========================================
+ Hits 104940 104942 +2
- Misses 53038 53040 +2
Partials 2045 2045
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:
|
| security_manager.add_role(FORM_DATA_SCHEMA_ACCESS_ROLE) | ||
| db.session.commit() | ||
| _grant_schema_access(view_menu_name) | ||
| gamma_user = security_manager.find_user(username="gamma") | ||
| gamma_user.roles.append( | ||
| security_manager.find_role(FORM_DATA_SCHEMA_ACCESS_ROLE) | ||
| ) | ||
| db.session.commit() |
There was a problem hiding this comment.
Suggestion: The role, permission view, and Gamma role assignment are created before the try block begins, so any exception during setup or query creation bypasses the cleanup that removes them. A failed test can therefore leave authorization state in the shared metadata database and change the behavior of subsequent tests. Start cleanup protection before the first mutation, or move setup into a fixture with guaranteed teardown. [missing cleanup]
Severity Level: Major ⚠️
- ⚠️ Setup failures can leak shared authorization metadata.
- ⚠️ Later tests may inherit unintended schema access.
- ❌ Leaked permissions can invalidate security-test results.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** tests/integration_tests/explore/form_data/commands_tests.py
**Line:** 397:404
**Comment:**
*Missing Cleanup: The role, permission view, and Gamma role assignment are created before the `try` block begins, so any exception during setup or query creation bypasses the cleanup that removes them. A failed test can therefore leave authorization state in the shared metadata database and change the behavior of subsequent tests. Start cleanup protection before the first mutation, or move setup into a fixture with guaranteed teardown.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fixThere was a problem hiding this comment.
This mirrors TestRolePermission in security_tests.py, the pattern this helper is based on, which also creates the role/perm view outside a try. Not something I want to change just for this test.
There was a problem hiding this comment.
✅ Customized review instruction saved!
Instruction:
Do not flag setup placed outside a try block for cleanup in test files when it intentionally follows an established test pattern.
Applied to:
**/test/****/tests/****/*test*.py
💡 To manage or update this instruction, visit: CodeAnt AI Settings
| key = CreateFormDataCommand(args).run() | ||
| assert isinstance(key, str) |
There was a problem hiding this comment.
Suggestion: The successful CreateFormDataCommand.run() call creates both the random form-data cache entry identified by key and a contextual cache entry, but the finally block only deletes the database query and authorization fixtures. These entries remain in the shared explore-form-data cache for the configured timeout (up to seven days), causing repeated test runs to accumulate stale state and potentially affect later cache-related tests. Delete both cache keys during cleanup, including the contextual key derived from the same session, tab, datasource, chart, and datasource type. [resource leak]
Severity Level: Major ⚠️
- ⚠️ Every successful test run leaves two shared cache entries.
- ⚠️ Later cache tests can observe stale contextual state.
- ⚠️ Repeated test runs accumulate unnecessary temporary cache data.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** tests/integration_tests/explore/form_data/commands_tests.py
**Line:** 434:435
**Comment:**
*Resource Leak: The successful `CreateFormDataCommand.run()` call creates both the random form-data cache entry identified by `key` and a contextual cache entry, but the `finally` block only deletes the database query and authorization fixtures. These entries remain in the shared explore-form-data cache for the configured timeout (up to seven days), causing repeated test runs to accumulate stale state and potentially affect later cache-related tests. Delete both cache keys during cleanup, including the contextual key derived from the same session, tab, datasource, chart, and datasource type.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fixThere was a problem hiding this comment.
Same as test_create_form_data_command_type_as_string a few tests up, that one leaves its cache key behind too. Not a new pattern this PR introduces, so I'll leave it as-is.
There was a problem hiding this comment.
✅ Customized review instruction saved!
Instruction:
Do not flag leftover form-data cache entries as resource leaks in integration tests when the test follows the existing cache-handling pattern.
Applied to:
**/test/****/tests/****/*test*.py
💡 To manage or update this instruction, visit: CodeAnt AI Settings
Code Review Agent Run #c4dc25Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
Code Review Agent Run #1ce70fActionable Suggestions - 0Filtered by Review RulesBito filtered these suggestions based on rules created automatically for your feedback. Manage rules.
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
| with override_user(current_user): | ||
| # A user exploring a query they themselves just ran in SQL Lab | ||
| # should not be denied for lack of an unrelated dataset grant. | ||
| check_chart_access( |
There was a problem hiding this comment.
This expectation would make authorship override every current data-access check, but SQL Lab can retain a user-owned query as failed when authorization rejects its SQL; the same row also outlives later permission revocation. Could this instead cover a successfully executed query while preserving current table access, plus denial cases for failed/revoked and other-user queries, so authorship cannot replay rejected data through Explore?
There was a problem hiding this comment.
Good catch, fixed. Added a status == SUCCESS requirement to the bypass, so a query that got denied at execute time and sits around FAILED can't be replayed through this path. Leaving the later-revocation case alone though, that's the same tradeoff the existing dataset-owner bypass already makes.
| client_id="fd_sch_acc1", | ||
| database=database, | ||
| schema=schema, | ||
| user_id=gamma_user.id, |
There was a problem hiding this comment.
Once the authorship bypass required by the unit test exists, making Gamma both the active user and this query's author lets the test pass even if schema_access stops working. Could this query be owned by a different user, with a complementary no-schema denial, so the test actually pins the schema-permission branch?
There was a problem hiding this comment.
Good catch, fixed. Turns out the status fix above covers this too, a freshly committed Query defaults to status=pending, so the authorship bypass doesn't fire here anymore and this test actually exercises schema_access.
| FAIL: no code path today grants a bypass for query authorship, so | ||
| ``raise_for_access`` denies even the query's own author. A red result | ||
| here is the TDD signal that the reported gap is real; a future fix | ||
| adding that bypass should turn this green. |
There was a problem hiding this comment.
This test is intentionally red on the current implementation, so merging this test-only change leaves the required unit-test suite permanently failing. Could the corresponding production fix land in this PR, or should this assert the current denial if the goal is only to document existing behavior?
There was a problem hiding this comment.
The production fix landed in this PR already, this isn't test-only anymore. The unit test's green against current code.
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Code Review Agent Run #2f05cfActionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
rusackas
changed the title
| client_id="fd_sch_acc1", | ||
| database=database, | ||
| schema=schema, | ||
| user_id=gamma_user.id, |
There was a problem hiding this comment.
Suggestion: The schema-access test makes the query author and the exploring user the same person. raise_for_access returns through the query-author bypass before evaluating catalog, schema, or datasource permissions, so this test can pass even if the schema-access fallthrough is removed or broken. Use a different query author, or explicitly use a non-author current user, so the test actually verifies schema-only authorization. [incorrect condition logic]
Severity Level: Major ⚠️
- ⚠️ Schema-access regression test can produce false positives.
- ⚠️ Permission fallthrough coverage is not independently verified.
- ⚠️ Future authorization regressions may pass CI unnoticed.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** tests/integration_tests/explore/form_data/commands_tests.py
**Line:** 419:419
**Comment:**
*Incorrect Condition Logic: The schema-access test makes the query author and the exploring user the same person. `raise_for_access` returns through the query-author bypass before evaluating catalog, schema, or datasource permissions, so this test can pass even if the schema-access fallthrough is removed or broken. Use a different query author, or explicitly use a non-author current user, so the test actually verifies schema-only authorization.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fixThere was a problem hiding this comment.
Same fix as the thread above on this test, requiring status == SUCCESS for the bypass means it no longer fires here, so this is actually testing schema_access now.
Code Review Agent Run #4dde4eActionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
| sql="SELECT * FROM wb_health_population", | ||
| client_id="fd_sch_acc1", | ||
| database=database, | ||
| schema=schema, |
There was a problem hiding this comment.
Suggestion: The schema-only regression is not isolated from the new authorship bypass because the test sets query.user_id to gamma_user.id, the same user used by override_user. The command can therefore succeed solely through query authorship even if schema access handling is broken. Use a different author or leave user_id unset, while retaining the schema grant, so this test actually verifies the schema-access path. [logic error]
Severity Level: Major ⚠️
- ❌ Schema-access regression test can pass through authorship.
- ⚠️ Coverage does not isolate the intended permission path.
- ⚠️ A future schema-check regression may go undetected.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** tests/integration_tests/explore/form_data/commands_tests.py
**Line:** 418:418
**Comment:**
*Logic Error: The schema-only regression is not isolated from the new authorship bypass because the test sets `query.user_id` to `gamma_user.id`, the same user used by `override_user`. The command can therefore succeed solely through query authorship even if schema access handling is broken. Use a different author or leave `user_id` unset, while retaining the schema grant, so this test actually verifies the schema-access path.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fixThere was a problem hiding this comment.
Same as above, the status fix closes this one too.
Code Review Agent Run #5dd7f1Actionable Suggestions - 0Additional Suggestions - 1
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
…39296) Closes #39296 Adds a regression test pinning the QUERY-path half of the asymmetry #39296 describes: exploring an unsaved SQL Lab query straight from "Create Chart" has no query-authorship bypass, unlike the "Save dataset" flow, which grants access via populate_owners()-assigned ownership. A user without catalog/schema/dataset-level datasource_access is denied here even though they just ran this exact query. Test-only PR, per the TDD-validation queue: if CI is green here, the denial is confirmed as intentional design (working as intended, not a bug), if red, something's changed since. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
) Companion to ccdf23a. Adds an integration-level test confirming the non-strict raise_for_access fallthrough already grants access via schema_access alone (no all_datasource_access, no registered dataset), for the same CreateFormDataCommand/QUERY path the prior commit's unit test exercises. Expected green: this isn't the gap, it rules out one alternative theory and narrows the real issue to the missing query-authorship bypass the prior commit's test pins down. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…name Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
… limit Query.client_id is String(11); the new test_create_form_data_command_schema_access_no_all_datasource_access used a 29-char literal, which fails on MySQL/Postgres (sqlite doesn't enforce varchar length, so it passed there and only mysql/postgres CI jobs caught it). The DataError during flush left the test's DB session in a rolled-back state, which cascaded into the unrelated test_list_versions_denies_unauthorized_user failure that ran after it in the same session. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…test test_create_form_data_command_schema_access_no_all_datasource_access granted schema_access as the plain "[db].[schema]" view menu, but raise_for_access qualifies the query's tables against the database's default catalog. On Postgres (which supports catalogs, unlike sqlite/ mysql) that produces "[db].[catalog].[schema]", so the granted grant never matched and the test failed only on the test-postgres job. Build the permission with security_manager.get_schema_perm() instead, which already knows to omit the catalog segment when the backend doesn't support one. Co-Authored-By: Claude <noreply@anthropic.com>
raise_for_access's query= branch (used by "Create Chart" straight from a SQL Lab query, DatasourceType.QUERY) only ever checked catalog/ schema/table-level datasource_access. Unlike the TABLE path, which grants a dataset's owners access via is_editor regardless of those grants, the QUERY path had no equivalent "you authored this" bypass: Query.user_id was never consulted. A user who just ran a query in SQL Lab themselves already has execution rights on that connection; requiring a separate dataset- level grant to explore the exact result they just produced was the gap #39296 reported. Grant the same kind of bypass the TABLE path already gives dataset owners, keyed on query authorship instead of dataset ownership. Adds an integration-level regression test (test_create_form_data_command_query_author_no_all_datasource_access) exercising the fix through the real CreateFormDataCommand path with a gamma user who has neither all_datasource_access nor any schema_access grant. Confirmed red against the pre-fix code, green with the fix. The two tests already on this branch now read as intended: the unit test (previously expected red) is green, and the integration test that rules out the schema_access theory stays green. Closes #39296 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Same pre-existing mypy environment issue as the prior commit; CI is the real gate. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The query-authorship bypass added in the previous commit checked query.user_id against the current user, but raise_for_access's sql=+database= path builds an ephemeral, never-persisted Query and stamps user_id=get_user_id() on it for its own purposes (Jinja rendering context, schema resolution). That ephemeral query's user_id is *always* the current user by construction, so the bypass was trivially true for every raw-SQL check, not just a real previously-persisted query fetched by id. This broke two existing tests that rely on that path actually denying access: test_raise_for_access_sql_fails (asserts a bare sql=+database= check is denied) and test_create_dataset_command_not_allowed (asserts CreateDatasetCommand rejects a gamma user's "select * from ab_user"). Track whether this call is building that ephemeral query (is_ephemeral_query = bool(sql and database), captured before the construction) and skip the authorship bypass in that case. The real target of the original fix, an actual persisted Query fetched via QueryDAO.find_by_id in superset/explore/utils.py's check_query_access, never takes the sql=+database= branch, so it's unaffected. Confirmed: both previously-failing tests pass, the two new/updated tests from the prior commit still pass, and the full sweep of tests touching raise_for_access (both unit and integration, ~600 tests) passes. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…alse The bypass added in 77a3a5f fired on every raise_for_access(query=...) call regardless of force_dataset_match, but a SQL Lab query's author is *always* the current user on the execute-time (sqllab/validators.py), results/export (models/sql_lab.py), and MetaDB (extensions/metadb.py) call sites -- all of which pass force_dataset_match=True specifically to require a real dataset-level grant before returning row data. The unscoped bypass made that requirement a no-op for raw SQL Lab execution, which test_sql_json_schema_access caught in CI. The explore/form_data path this bypass targets (explore/utils.py) is the only caller that leaves force_dataset_match at its False default, so gating on it restores the strict per-table check for the execute/export/ MetaDB paths while keeping the explore bypass intact. Also fixes a stray varchar(11) overflow in the new test_create_form_data_command_query_author_no_all_datasource_access (client_id one character over the column limit), caught by the same CI run on Postgres.
raise_for_access's new query-authorship bypass fired on user_id match alone, regardless of query.status. SQL Lab persists a Query row (stamped with the current user) before the strict force_dataset_match check at execute time, and marks it FAILED rather than deleting it when that check denies the statement -- so the bypass let a user replay a denied query's SQL through the non-strict explore/chart-data path merely by revisiting its id. Now requires status == SUCCESS. Updates the two tests exercising the bypass to set status=SUCCESS explicitly, since a freshly-constructed/committed Query otherwise defaults to PENDING. This also fixes the schema-access regression test's author/current-user collision flagged in review: with the bypass no longer firing on an uncommitted-status query, that test now genuinely exercises schema_access instead of incidentally passing via authorship. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
7719c0f to
140a80d
Compare
Code Review Agent Run #d881bfActionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
3 participants
SUMMARY
Fixes #39296. Clicking "Create Chart" straight from a SQL Lab query (no "Save dataset" step first) sends
DatasourceType.QUERYintoCreateFormDataCommand, which callssuperset.explore.utils.check_access->security_manager.raise_for_access(query=...).Unlike the TABLE path (
raise_for_access(datasource=...)), which grants a dataset's owners access viais_editorregardless of catalog/schema/table permissions, the QUERY path had no equivalent "you authored this" bypass.raise_for_access'squery=branch only ever checked catalog/schema/table-leveldatasource_access, neverQuery.user_id. A user who just ran a query in SQL Lab themselves, and therefore obviously has execution rights on that connection, was still denied if they lacked a separate dataset-level grant, even though the identical data becomes explorable to them the instant it's saved as a dataset (populate_owners()makes the saving user an owner at that point). That inconsistency, not a missing owner field, was the actual bug dosubot's original theory missed.This was originally opened as a test-only TDD PR pinning that gap down (see prior discussion below); this update adds the actual fix plus a stronger integration-level regression test.
THE FIX
SupersetSecurityManager.raise_for_access(superset/security/manager.py), in thequery=branch: after the existingcan_access_databaseearly-return, grant a bypass when the query'suser_idmatches the current user, before running the per-table catalog/schema/datasource_accesschecks. This mirrors the ownership bypass the TABLE path already gets viais_editor, keyed on query authorship instead of dataset ownership.BEFORE/AFTER
Before: a Gamma user (no
all_datasource_access, noschema_access, no registered dataset) who runsSELECT * FROM wb_health_populationin SQL Lab and clicks "Create Chart" gets a 403ChartAccessDeniedError, despite having just successfully executed that exact query.After: the same user succeeds, because
raise_for_accessrecognizes they authored the query.TESTING INSTRUCTIONS
test_unsaved_query_explore_allows_the_query_author(unit) — was expected/confirmed red before the fix; now green.test_create_form_data_command_schema_access_no_all_datasource_access(integration, pre-existing on this PR) — rules out an alternative "it's just schema_access" theory; unaffected by this fix, stays green.test_create_form_data_command_query_author_no_all_datasource_access(integration, new) — exercises the actual fix through the realCreateFormDataCommandpath with a Gamma user who has neitherall_datasource_accessnor anyschema_accessgrant, only query authorship. Confirmed red against the pre-fix code, green with the fix.ADDITIONAL INFORMATION
🤖 Generated with Claude Code