fix(dataset): validate datasource access during import#39998
Conversation
Add a raise_for_access(datasource=dataset) call in import_dataset() after the dataset is created or updated. This ensures the importing user has the necessary datasource-level permissions in addition to the existing can_write and ownership checks. The check is skipped when ignore_permissions=True (used by admin bulk-import workflows). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Code Review Agent Run #685e4bActionable 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 |
dosubot
Bot
added
authentication:access-control
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #39998 +/- ##
==========================================
- Coverage 63.83% 63.83% -0.01%
==========================================
Files 2589 2589
Lines 137821 137827 +6
Branches 31928 31929 +1
==========================================
+ Hits 87978 87981 +3
- Misses 48327 48329 +2
- Partials 1516 1517 +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:
|
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| except SupersetSecurityException as ex: | ||
| raise ImportFailedError( | ||
| "User does not have access to the target dataset" | ||
| ) from ex |
There was a problem hiding this comment.
Suggestion: This new access-denial path raises ImportFailedError, which is a 500-class command exception in this codebase. As a result, permission-denied imports are reported as internal server errors instead of client validation/authorization failures (the PR expectation says 422). Raise a 4xx-mapped exception type for this branch so unauthorized datasource imports return the correct HTTP status. [api mismatch]
Severity Level: Major ⚠️
- ❌ Dataset import endpoint returns 500 on permission-denied imports.
- ⚠️ Clients cannot distinguish auth failures from server crashes.
- ⚠️ API contract/docs advertising 4xx on import mismatched.Steps of Reproduction ✅
1. Trigger the dataset import API `POST /api/v1/dataset/import/` implemented in
`superset/datasets/api.py:930-1032` by uploading a ZIP/YAML bundle that includes a dataset
whose `database_id` points to a database/schema the current user cannot access under
normal security rules.
2. In `DatasetRestApi.import_` (`superset/datasets/api.py:930-1032`), the request file is
parsed into `contents`, and an `ImportDatasetsCommand` is instantiated and executed at
`superset/datasets/api.py:1022-1032` via `command = ImportDatasetsCommand(contents, ...)`
followed by `command.run()`.
3. `ImportDatasetsCommand.run()` in
`superset/commands/dataset/importers/dispatcher.py:51-68` dispatches to
`v1.ImportDatasetsCommand`, whose `_import` method in
`superset/commands/dataset/importers/v1/__init__.py:36-43` iterates dataset configs and
calls `import_dataset(config, overwrite=overwrite)` from
`superset/commands/dataset/importers/v1/utils.py:106-112`.
4. Inside `import_dataset` in `superset/commands/dataset/importers/v1/utils.py:84-93`,
after the dataset is created/updated and flushed, the new block at lines 176-182 executes
`security_manager.raise_for_access(datasource=dataset)`; for a user without
datasource-level access this raises `SupersetSecurityException`, which is caught at line
179 and converted to `ImportFailedError` (lines 179-182). `ImportFailedError` is defined
in `superset/commands/exceptions.py:8-10` with `status = 500`, and the global
`CommandException` handler in `superset/views/error_handling.py:184-212` returns a JSON
error response with `status=ex.status`, i.e. HTTP 500, so the permission-denied dataset
import surfaces as a 500 Internal Server Error instead of a 4xx (e.g. 422)
client/authorization error.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:** superset/commands/dataset/importers/v1/utils.py
**Line:** 179:182
**Comment:**
*Api Mismatch: This new access-denial path raises `ImportFailedError`, which is a 500-class command exception in this codebase. As a result, permission-denied imports are reported as internal server errors instead of client validation/authorization failures (the PR expectation says 422). Raise a 4xx-mapped exception type for this branch so unauthorized datasource imports return the correct HTTP status.
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
|
The flagged issue is correct: the code catches SupersetSecurityException and raises ImportFailedError (status 500), but permission-denied imports should return a 4xx status like 422. To resolve, remove the try-except block and directly call security_manager.raise_for_access(datasource=dataset) when not ignoring permissions. This lets SupersetSecurityException propagate, which maps to a 4xx error. The added test in the PR will need updating to expect SupersetSecurityException instead of ImportFailedError. No other comments in the PR. superset/commands/dataset/importers/v1/utils.py |
…ailedError (500) on access check failure Permission-denied imports should return 403, not 500. Use the existing DatasetAccessDeniedError(ForbiddenError) which already maps to HTTP 403. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Code Review Agent Run #e4df51Actionable 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 |
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
3 participants
SUMMARY
import_dataset()in the V1 importer had an ownership gate that prevented unauthorized overwrites of existing datasets, but lacked a call tosecurity_manager.raise_for_access(datasource=dataset)after the dataset was created or updated.Without this check, a user with
can_writeon the Dataset resource could import a YAML bundle that creates or overwrites a dataset pointing to a database/schema they do not have access to under the normal access rules.This PR adds
raise_for_access(datasource=dataset)after the dataset is persisted, mirroring the pattern used byUpdateDatasetCommand. The check is skipped whenignore_permissions=True(used by admin bulk-import flows).BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A — backend-only change.
TESTING INSTRUCTIONS
Run the unit tests:
All 17 tests should pass, including the new
test_import_dataset_access_check.Attempt to import a dataset YAML for a table in a schema the importing user does not have access to (e.g., a Gamma user importing a dataset on a restricted database) — should return a 422 error.
ADDITIONAL INFORMATION