fix(api): enforce per-object ownership validation in chart, dataset, and report commands#39303
fix(api): enforce per-object ownership validation in chart, dataset, and report commands#39303sha174n wants to merge 10 commits intoapache:masterfrom
Conversation
…and report commands - Chart create/update commands: verify caller has access to the chart - Dataset create/duplicate commands: verify caller has access to the datasource - Report schedule commands: verify caller has access to the target chart/dashboard - TableSchemaView (sqllab): verify caller owns the tab state before modifying its schema - Avatar endpoint: require authentication - Form data helper: check chart access before merging slice data - Samples endpoint: pass pre-loaded dataset to avoid redundant DB lookup Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
dosubot
bot
added
authentication:access-control
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #39303 +/- ##
==========================================
- Coverage 64.45% 64.40% -0.05%
==========================================
Files 2555 2553 -2
Lines 132721 132631 -90
Branches 30802 30752 -50
==========================================
- Hits 85539 85425 -114
- Misses 45696 45719 +23
- Partials 1486 1487 +1
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:
|
![bito-code-review[bot]](https://avatars.githubusercontent.com/in/1061978?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review Agent Run #b7a587
Actionable Suggestions - 1
-
superset/views/sql_lab/views.py - 1
- Error Handling Inconsistency · Line 285-301
Additional Suggestions - 1
-
superset/views/datasource/utils.py - 1
-
Type Hint Violation · Line 98-98The new datasource parameter uses 'Any' type hint, violating the project's type safety standards that require proper typing instead of 'any' types. Change to 'Datasource | None' and import 'Datasource' from superset.daos.datasource for better type checking and MyPy compliance.
Code suggestion
@@ -27,1 +27,1 @@ - from superset.daos.datasource import DatasourceDAO + from superset.daos.datasource import DatasourceDAO, Datasource @@ -98,1 +98,1 @@ - datasource: Any | None = None, + datasource: Datasource | None = None,
-
Review Details
-
Files reviewed - 16 · Commit Range:
6b2d054..6b2d054- superset/charts/api.py
- superset/commands/chart/create.py
- superset/commands/chart/update.py
- superset/commands/chart/warm_up_cache.py
- superset/commands/dataset/create.py
- superset/commands/dataset/duplicate.py
- superset/commands/dataset/warm_up_cache.py
- superset/commands/report/base.py
- superset/sqllab/api.py
- superset/views/datasource/utils.py
- superset/views/datasource/views.py
- superset/views/sql_lab/views.py
- superset/views/users/api.py
- superset/views/utils.py
- tests/unit_tests/commands/chart/warm_up_cache_test.py
- tests/unit_tests/commands/dataset/test_create.py
-
Files skipped - 0
-
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
- MyPy (Static Code Analysis) - ✔︎ Successful
- Astral Ruff (Static Code Analysis) - ✔︎ Successful
Bito Usage Guide
Commands
Type the following command in the pull request comment and save the comment.
-
/review- 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
AI Code Review powered by
…eption guards - Replace DatasetDAO.find_by_id with DatasourceDAO.get_datasource in Datasource.samples() so non-table types (saved_query) are resolved correctly instead of returning 404 - Wrap SupersetSecurityException in DatasetAccessDeniedError in DatasetWarmUpCacheCommand.validate() so the command layer maps it to the correct HTTP status - Add try/except rollback guard to TableSchemaView.expanded() to match the pattern used by post() and delete() in the same class Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…dataset commands - CreateChartCommand: verify ChartForbiddenError raised when caller lacks datasource access - UpdateChartCommand: verify ChartForbiddenError raised when caller lacks access to the new datasource - DuplicateDatasetCommand: verify DatasetAccessDeniedError raised when caller lacks read access to the source dataset; verify access check fires before proceeding - DatasetWarmUpCacheCommand: verify WarmUpCacheTableNotFoundError on missing table, DatasetAccessDeniedError on forbidden access, and that _charts is populated when access is granted Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Code Review Agent Run #41cc0e
Actionable Suggestions - 1
-
superset/views/sql_lab/views.py - 1
- Blind exception catch without specific type · Line 303-303
Filtered by Review Rules
Bito filtered these suggestions based on rules created automatically for your feedback. Manage rules.
-
superset/views/sql_lab/views.py - 1
- Broad Exception Handling · Line 285-305
Review Details
-
Files reviewed - 4 · Commit Range:
6b2d054..4f5c3bc- tests/unit_tests/commands/dataset/test_create.py
- superset/commands/dataset/warm_up_cache.py
- superset/views/datasource/views.py
- superset/views/sql_lab/views.py
-
Files skipped - 0
-
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
- MyPy (Static Code Analysis) - ✔︎ Successful
- Astral Ruff (Static Code Analysis) - ✔︎ Successful
Bito Usage Guide
Commands
Type the following command in the pull request comment and save the comment.
-
/review- 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
AI Code Review powered by
| db.session.commit() | ||
| response = json.dumps({"id": table_schema_id, "expanded": payload}) | ||
| return json_success(response) | ||
| except Exception as ex: # pylint: disable=broad-except |
There was a problem hiding this comment.
Replace the broad Exception catch at line 303 with specific exception types. Consider catching SQLAlchemyError, ValueError, or other specific exceptions relevant to the database operations and JSON parsing in this method.
Code suggestion
Check the AI-generated fix before applying
| except Exception as ex: # pylint: disable=broad-except | |
| except (ValueError, Exception) as ex: # pylint: disable=broad-except |
Code Review Run #41cc0e
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
Code Review Agent Run #d5a74eActionable 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 |
…er login and override_user to fix test failures - In ChartWarmUpCacheCommand.validate(), only call raise_for_access when chart.datasource is not None; charts without datasources are handled gracefully downstream and there's no datasource to protect access to - Add allow_browser_login = True to UserRestApi so @Protect() accepts browser session cookies (same pattern as CurrentUserRestApi) - Replace @patch("superset.commands.utils.g") + mock_g with override_user() in two integration tests; the security manager reads flask.g directly via its own import, so the module-level patch didn't cover it Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
| raise WarmUpCacheChartNotFoundError() | ||
| self._chart_or_id = chart | ||
| if chart.datasource: | ||
| security_manager.raise_for_access(viz=chart) |
There was a problem hiding this comment.
Suggestion: The access check is using viz=chart, which enforces datasource-level access instead of chart-level ownership rules. This can incorrectly deny users who own the chart but don't have direct datasource permission, and it also relies on viz internals that expect a datasource to exist. Use the chart-specific access path instead. [logic error]
Severity Level: Major ⚠️
- ❌ Chart warm_up_cache endpoint can reject chart owners without datasource.
- ⚠️ Inconsistent permission behavior versus security_manager.can_access_chart helper.| security_manager.raise_for_access(viz=chart) | |
| security_manager.raise_for_access(chart=chart) |
Steps of Reproduction ✅
1. Define a `Slice` instance with a valid datasource so `Slice.datasource` returns a
non-null object (see `superset/models/slice.py` `datasource` property at lines 144-147)
and persist it as `chart`.
2. Configure the current security context so `security_manager.is_owner(chart)` would
return True but `security_manager.can_access_schema(chart.datasource)`,
`security_manager.can_access("datasource_access", chart.datasource.perm or "")`, and
`security_manager.is_owner(chart.datasource)` would all return False (these checks are
used in the `viz`/datasource branch of `raise_for_access` in
`superset/security/manager.py` around lines 2688-2721).
3. Instantiate and execute `ChartWarmUpCacheCommand(chart, dashboard_id=None,
extra_filters=None).run()` as done in
`tests/unit_tests/commands/chart/warm_up_cache_test.py`, which first calls `validate()` in
`superset/commands/chart/warm_up_cache.py` lines 36-45.
4. In `validate()`, because `chart.datasource` is truthy,
`security_manager.raise_for_access(viz=chart)` (line 134 in the PR hunk) is invoked;
`raise_for_access` treats this as a viz/datasource check, never reaching the
chart-specific ownership logic at its `if chart:` block (around lines 2838-2850 in
`superset/security/manager.py`), and will raise `SupersetSecurityException` even though
the user is an owner of the chart but not of the datasource, causing the warm-up flow (and
thus the `/api/v1/chart/warm_up_cache` behavior that uses this command) to fail for such
owners.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/commands/chart/warm_up_cache.py
**Line:** 134:134
**Comment:**
*Logic Error: The access check is using `viz=chart`, which enforces datasource-level access instead of chart-level ownership rules. This can incorrectly deny users who own the chart but don't have direct datasource permission, and it also relies on viz internals that expect a datasource to exist. Use the chart-specific access path instead.
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.Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ecks chart= path checks admin/owner first, then datasource access — correctly allowing chart owners through and handling chart.datasource=None without an extra guard. viz= path only checks datasource access and assumes datasource is non-None. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Code Review Agent Run #99f87eActionable 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 |
- ChartWarmUpCacheCommand.validate(): wrap raise_for_access(chart=) with try/except SupersetSecurityException → ChartAccessDeniedError - test_create.py: add -> None return type hints to all test functions Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Code Review Agent Run #e8b6c6Actionable 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 |
…tion Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Code Review Agent Run #c6b530
Actionable Suggestions - 1
-
superset/commands/report/base.py - 1
- Semantic Duplication · Line 56-82
Additional Suggestions - 1
-
superset/commands/chart/update.py - 1
-
Dead code introduced · Line 105-105The added line initializing datasource_type to "" is dead code when datasource_id is None, as it's assigned but never read in that execution path. This creates unnecessary maintenance overhead without functional benefit.
Code suggestion
--- superset/commands/chart/update.py +++ superset/commands/chart/update.py @@ -103,7 +103,6 @@ # Validate if datasource_id is provided datasource_type is required datasource_id = self._properties.get("datasource_id") - datasource_type = "" if datasource_id is not None: datasource_type = self._properties.get("datasource_type", "") if not datasource_type:
-
Review Details
-
Files reviewed - 3 · Commit Range:
cf15d57..c5ecd95- superset/commands/chart/update.py
- superset/commands/report/base.py
- tests/unit_tests/commands/report/test_create_recipients.py
-
Files skipped - 0
-
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
- MyPy (Static Code Analysis) - ✔︎ Successful
- Astral Ruff (Static Code Analysis) - ✔︎ Successful
Bito Usage Guide
Commands
Type the following command in the pull request comment and save the comment.
-
/review- 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
AI Code Review powered by
| def _check_chart_access( | ||
| self, chart_id: int, exceptions: list[ValidationError] | ||
| ) -> None: | ||
| """Validate chart exists and the current user can access it.""" | ||
| chart = ChartDAO.find_by_id(chart_id) | ||
| if not chart: | ||
| exceptions.append(ChartNotFoundValidationError()) | ||
| else: | ||
| try: | ||
| security_manager.raise_for_access(viz=chart) | ||
| except SupersetSecurityException as ex: | ||
| raise ReportScheduleForbiddenError() from ex | ||
| self._properties["chart"] = chart | ||
|
|
||
| def _check_dashboard_access( | ||
| self, dashboard_id: int, exceptions: list[ValidationError] | ||
| ) -> None: | ||
| """Validate dashboard exists and the current user can access it.""" | ||
| dashboard = DashboardDAO.find_by_id(dashboard_id) | ||
| if not dashboard: | ||
| exceptions.append(DashboardNotFoundValidationError()) | ||
| else: | ||
| try: | ||
| security_manager.raise_for_access(dashboard=dashboard) | ||
| except SupersetSecurityException as ex: | ||
| raise ReportScheduleForbiddenError() from ex | ||
| self._properties["dashboard"] = dashboard |
There was a problem hiding this comment.
The newly added _check_chart_access and _check_dashboard_access methods contain nearly identical logic, differing only in the DAO used, exception types raised, and the property key set. This semantic duplication within the diff increases maintenance risk if the validation logic needs changes. Consider consolidating into a shared helper method.
Code Review Run #c6b530
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
1 participant
SUMMARY
Adds ownership validation to several commands and views that were loading objects and acting on them without first verifying the caller has access.
create,update): check that the caller can access the chart before proceedingcreate,duplicate): check that the caller can access the datasource/tabstateview/<id>/tableSchema): verify the caller owns the tab state before POST, DELETE, and expanded endpoints/users/<id>/avatar.png): add@protect()— was missing authenticationget_form_data): check chart access before merging in slice form dataBEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A — API-only changes.
TESTING INSTRUCTIONS
POST /api/v1/chart/with that chart's datasource → expect 403POST /api/v1/dataset/duplicate→ expect 403POST /tabstateview/<id>/tableSchemawith another user's tab state ID → expect 403/users/<id>/avatar.png→ expect 401ADDITIONAL INFORMATION