Skip to content

fix(explore): apply per-datasource access check on the legacy explore view#41425

Open
sha174n wants to merge 4 commits into
apache:masterfrom
sha174n:fix/explore-view-datasource-permission
Open

fix(explore): apply per-datasource access check on the legacy explore view#41425
sha174n wants to merge 4 commits into
apache:masterfrom
sha174n:fix/explore-view-datasource-permission

Conversation

@sha174n

@sha174n sha174n commented Jun 25, 2026

Copy link
Copy Markdown
Contributor

SUMMARY

Applies the same per-datasource access check the /api/v1/explore command uses (security_manager.raise_for_access(datasource=...)) to the deprecated /superset/explore/ view, so both paths apply a consistent per-object datasource check before rendering datasource metadata. The check is guarded for the placeholder (missing-dataset) case the view already supports, and runs ahead of the save-as/overwrite action paths so they share the same check.

TESTING INSTRUCTIONS

Adds a regression test asserting the view runs the per-datasource check on the loaded datasource:

pytest tests/integration_tests/core_tests.py::TestCore::test_explore_view_checks_datasource_access

ADDITIONAL INFORMATION

  • Has associated issue:
  • Required feature flags:
  • Changes UI
  • Includes DB Migration
  • Introduces new feature or API
  • Removes existing feature or API

… view

Mirror the per-datasource access check the /api/v1/explore command already
performs (security_manager.raise_for_access(datasource=...)) on the deprecated
/superset/explore/ view, so both paths apply the same per-object datasource
check before rendering datasource metadata. Guarded for the placeholder
(missing-dataset) case the view already supports. Adds a regression test.
@github-actions github-actions Bot added the api Related to the REST API label Jun 25, 2026
@codecov

codecov Bot commented Jun 25, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 50.00000% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 64.45%. Comparing base (aac02ab) to head (d9c315a).
⚠️ Report is 3 commits behind head on master.

Files with missing lines Patch % Lines
superset/views/core.py 50.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master   #41425      +/-   ##
==========================================
- Coverage   64.47%   64.45%   -0.03%     
==========================================
  Files        2662     2662              
  Lines      145905   145817      -88     
  Branches    33669    33638      -31     
==========================================
- Hits        94073    93982      -91     
+ Misses      50128    50127       -1     
- Partials     1704     1708       +4     
Flag Coverage Δ
hive 39.22% <0.00%> (-0.01%) ⬇️
mysql 57.96% <50.00%> (+0.02%) ⬆️
postgres 58.02% <50.00%> (+0.02%) ⬆️
presto 40.80% <0.00%> (-0.01%) ⬇️
python 59.45% <50.00%> (+0.02%) ⬆️
sqlite 57.68% <50.00%> (+0.02%) ⬆️
unit 100.00% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@sha174n sha174n marked this pull request as ready for review June 25, 2026 19:51
@dosubot dosubot Bot added authentication:access-control Rlated to access control change:backend Requires changing the backend explore Namespace | Anything related to Explore labels Jun 25, 2026
Comment thread tests/integration_tests/core_tests.py Outdated
@bito-code-review

Copy link
Copy Markdown
Contributor

The flagged issue is correct. The test method test_explore_view_checks_datasource_access lacks type annotations for the mock_raise_for_access parameter and the return type. You can resolve this by updating the method signature as follows:

    @mock.patch("superset.security.SupersetSecurityManager.raise_for_access")
    def test_explore_view_checks_datasource_access(self, mock_raise_for_access: mock.Mock) -> None:

There are no other comments on this pull request to address.

tests/integration_tests/core_tests.py

@mock.patch("superset.security.SupersetSecurityManager.raise_for_access")
    def test_explore_view_checks_datasource_access(self, mock_raise_for_access: mock.Mock) -> None:

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

@bito-code-review bito-code-review Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review Agent Run #1d61bf

Actionable Suggestions - 1
  • superset/views/core.py - 1
Review Details
  • Files reviewed - 2 · Commit Range: 614848f..8dd1f7b
    • superset/views/core.py
    • tests/integration_tests/core_tests.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 Bito Logo

Comment thread superset/views/core.py
Condense the explanatory comment on the per-datasource access check to
match the file's terse style for security calls.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@sha174n

sha174n commented Jun 25, 2026

Copy link
Copy Markdown
Contributor Author

@bito-code-review The CWE-552 item is a false positive and I've resolved the inline thread. SupersetSecurityException subclasses SupersetErrorException, which has a global Flask error handler (set_app_error_handlersshow_superset_error); Flask dispatches by class MRO, so the exception is returned as a structured JSON 403, not a generic 500. No per-method decorator or try/except is needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

api Related to the REST API authentication:access-control Rlated to access control change:backend Requires changing the backend explore Namespace | Anything related to Explore size/S

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants