fix(datasource): map combined_list to can_read so Gamma can list datasets#39947
fix(datasource): map combined_list to can_read so Gamma can list datasets#39947msyavuz wants to merge 2 commits into
Conversation
…sets The combined_list endpoint added in #37815 used the auto-derived `can_combined_list on Datasource` permission, which is not in READ_ONLY_PERMISSION and is therefore excluded from the Gamma role by _is_alpha_only. Map it to `can_read on Datasource` instead so Gamma inherits access alongside Alpha and Admin.
Code Review Agent Run #5bd230Actionable 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 |
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| security_manager.add_permission_view_menu("can_read", "Datasource") | ||
| perm = security_manager.find_permission_view_menu("can_read", "Datasource") |
There was a problem hiding this comment.
Suggestion: This test was migrated to grant can_read on Datasource, but other combined_list tests in the same file still grant can_combined_list. Because the endpoint permission is now can_read, those tests can fail with 403 when run in isolation (or become order-dependent if this test runs first and mutates role permissions). Update the remaining combined_list tests to grant can_read (or move permission setup to a shared fixture) so each test is self-contained. [logic error]
Severity Level: Major ⚠️
- ❌ `test_combined_list_semantic_layers_off` fails when run in isolation.
- ⚠️ Test suite becomes order-dependent for combined_list tests.
- ⚠️ Flaky CI possible if test ordering or selection changes.Steps of Reproduction ✅
1. Note the new permission mapping for the endpoint in `superset/datasource/api.py`:
`DatasourceRestApi` defines `method_permission_name = {"combined_list": "read"}` at lines
40–48 and the `combined_list` view at lines 11–20 and 42–48 (second snippet, lines 11–20
and 42–48) is decorated with `@protect()`, which enforces `can_read` on class permission
`Datasource`.
2. In `tests/integration_tests/datasource/api_tests.py`, inspect
`test_combined_list_invalid_order_column` at lines 208–223: it creates and grants
`("can_read", "Datasource")` to the Admin role via
`security_manager.add_permission_view_menu("can_read", "Datasource")` and
`security_manager.add_permission_role(admin_role, perm)` (lines 215–218), so this test
aligns with the new mapping.
3. In the same file, inspect `test_combined_list_semantic_layers_off` at lines 231–255: it
still creates and grants `("can_combined_list", "Datasource")` (lines 238–243) and never
creates or assigns `("can_read", "Datasource")`. When this test is run in isolation, the
Admin role lacks `can_read` on `Datasource`, so the `@protect()` check for `combined_list`
fails with 403 before the body executes, even though `security_manager.can_access` is
patched to return True (lines 231–244).
4. Run this test alone, e.g. `pytest
tests/integration_tests/datasource/api_tests.py::TestDatasourceApi::test_combined_list_semantic_layers_off`.
The request to `GET
/api/v1/datasource/?q=(order_column:changed_on_delta_humanized,order_direction:desc,page:0,page_size:25)`
at lines 248–251 returns HTTP 403 instead of the expected 200 at line 253, causing the
test to fail; if `test_combined_list_invalid_order_column` runs first in the same process
it seeds `can_read` and masks the bug, making the behavior order-dependent.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** tests/integration_tests/datasource/api_tests.py
**Line:** 215:216
**Comment:**
*Logic Error: This test was migrated to grant `can_read` on `Datasource`, but other `combined_list` tests in the same file still grant `can_combined_list`. Because the endpoint permission is now `can_read`, those tests can fail with 403 when run in isolation (or become order-dependent if this test runs first and mutates role permissions). Update the remaining `combined_list` tests to grant `can_read` (or move permission setup to a shared fixture) so each test is self-contained.
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
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #39947 +/- ##
=======================================
Coverage 63.88% 63.89%
=======================================
Files 2583 2583
Lines 136604 136605 +1
Branches 31502 31502
=======================================
+ Hits 87276 87277 +1
Misses 47812 47812
Partials 1516 1516
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…s_off The other combined_list test was migrated to grant can_read on Datasource, but this one still granted can_combined_list, causing 403 when run in isolation now that the endpoint maps to can_read.
Code Review Agent Run #abc845Actionable 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 |
2 participants
SUMMARY
DatasourceRestApi.combined_list(added in #37815) relied on the auto-derivedcan_combined_list on Datasourcepermission. Becausecan_combined_listis not inSupersetSecurityManager.READ_ONLY_PERMISSION,_is_alpha_onlyflags it as Alpha-only and excludes it from the Gamma role, so Gamma users get a 403 on the dataset list page (/api/v1/datasource/?q=...).Map the method to
readviamethod_permission_nameso the FAB check usescan_read on Datasource, which is already aREAD_ONLY_PERMISSIONand gets granted to Admin/Alpha/Gamma via the standard role sync.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A — backend permission mapping change, no UI difference.
TESTING INSTRUCTIONS
superset initto registercan_read on Datasourceand re-sync role grants./api/v1/datasource/?q=...should return 200 instead of 403.pytest tests/integration_tests/datasource/api_tests.py::TestDatasourceApi::test_combined_list_invalid_order_column tests/integration_tests/datasource/api_tests.py::TestDatasourceApi::test_combined_list_semantic_layers_off.ADDITIONAL INFORMATION
Note: deployments must run
superset initafter upgrading so the newcan_read on Datasourcepermission is registered and granted to existing roles.