codeant-ai-for-open-source[bot] commented on code in PR #42590:
URL: https://github.com/apache/superset/pull/42590#discussion_r3679211984
##########
tests/integration_tests/explore/form_data/commands_tests.py:
##########
@@ -345,3 +371,74 @@ def test_delete_form_data_command_key_expired(self,
mock_g):
response = delete_command.run()
assert response is False # noqa: E712
+
+ def
test_create_form_data_command_schema_access_no_all_datasource_access(self):
+ """
+ Regression for #39296: a user who has schema_access on the schema a
+ SQL Lab query ran in (but neither all_datasource_access nor
+ datasource_access on a specific registered dataset, since an ad-hoc
+ query result is never registered as one) should still be able to
+ jump straight from SQL Lab to "Create Chart" -- i.e.
+ CreateFormDataCommand.run() with datasource_type=QUERY should not be
+ blocked purely for lacking all_datasource_access, as long as the
+ query's schema is one they're granted schema_access on.
+
+ This exercises the same non-strict (force_dataset_match=False)
+ fallthrough in SupersetSecurityManager.raise_for_access that
+ test_raise_for_access_force_dataset_match_denies_schema_only (in
+ security_tests.py) exercises for the strict SQL Lab path -- here we
+ confirm the *non*-strict Explore/"Create Chart" path grants access on
+ schema_access alone, without needing a registered dataset at all.
+ """
+ schema = get_example_default_schema()
+ database = get_example_database()
+ view_menu_name = f"[{database.database_name}].[{schema}]"
+
+ 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()
+
+ query = Query(
+ sql="SELECT * FROM wb_health_population",
+ client_id="form_data_schema_access_test",
+ database=database,
+ schema=schema,
+ user_id=gamma_user.id,
+ )
+ db.session.add(query)
+ db.session.commit()
+
+ try:
+ with override_user(gamma_user):
+ # Sanity check: gamma should NOT have all_datasource_access,
+ # or this test wouldn't be exercising the reported gap.
+ assert not security_manager.can_access_all_datasources()
+
+ args = CommandParameters(
+ datasource_id=query.id,
+ datasource_type=DatasourceType.QUERY,
+ chart_id=None,
+ tab_id=1,
+ form_data=json.dumps(
+ {"datasource": f"{query.id}__{DatasourceType.QUERY}"}
+ ),
+ )
+ # Should NOT raise: schema_access on the query's schema is
+ # sufficient for the non-strict Explore access check, even
+ # without all_datasource_access or a registered dataset.
+ key = CreateFormDataCommand(args).run()
+ assert isinstance(key, str)
Review 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]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ 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.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=0ca5667039294b2c973455906937b31c&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=0ca5667039294b2c973455906937b31c&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
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 fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42590&comment_hash=74e08128bf222825300e34e5577951dc6321a55660f7c15b1414124329186ca6&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42590&comment_hash=74e08128bf222825300e34e5577951dc6321a55660f7c15b1414124329186ca6&reaction=dislike'>👎</a>
##########
tests/integration_tests/explore/form_data/commands_tests.py:
##########
@@ -345,3 +371,74 @@ def test_delete_form_data_command_key_expired(self,
mock_g):
response = delete_command.run()
assert response is False # noqa: E712
+
+ def
test_create_form_data_command_schema_access_no_all_datasource_access(self):
+ """
+ Regression for #39296: a user who has schema_access on the schema a
+ SQL Lab query ran in (but neither all_datasource_access nor
+ datasource_access on a specific registered dataset, since an ad-hoc
+ query result is never registered as one) should still be able to
+ jump straight from SQL Lab to "Create Chart" -- i.e.
+ CreateFormDataCommand.run() with datasource_type=QUERY should not be
+ blocked purely for lacking all_datasource_access, as long as the
+ query's schema is one they're granted schema_access on.
+
+ This exercises the same non-strict (force_dataset_match=False)
+ fallthrough in SupersetSecurityManager.raise_for_access that
+ test_raise_for_access_force_dataset_match_denies_schema_only (in
+ security_tests.py) exercises for the strict SQL Lab path -- here we
+ confirm the *non*-strict Explore/"Create Chart" path grants access on
+ schema_access alone, without needing a registered dataset at all.
+ """
+ schema = get_example_default_schema()
+ database = get_example_database()
+ view_menu_name = f"[{database.database_name}].[{schema}]"
+
+ 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()
Review 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]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Setup failures can leak shared authorization metadata.
- ⚠️ Later tests may inherit unintended schema access.
- ❌ Leaked permissions can invalidate security-test results.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=7370b92643824ddda807483de668bbed&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=7370b92643824ddda807483de668bbed&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
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 fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42590&comment_hash=3d237bf193f982dcc1f5f81def048266e1a7404b4bf3b32634ba5cbe05cb7059&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42590&comment_hash=3d237bf193f982dcc1f5f81def048266e1a7404b4bf3b32634ba5cbe05cb7059&reaction=dislike'>👎</a>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
