codeant-ai-for-open-source[bot] commented on code in PR #42590:
URL: https://github.com/apache/superset/pull/42590#discussion_r3707412737
##########
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:
✅ **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](https://app.codeant.ai/org/settings/learnings)*
##########
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:
✅ **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](https://app.codeant.ai/org/settings/learnings)*
--
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]
