You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "sha174n (via GitHub)" <gi...@apache.org> on 2026/03/14 11:17:47 UTC
[PR] Fix/datasource idor access checks [superset]
sha174n opened a new pull request, #38647:
URL: https://github.com/apache/superset/pull/38647
<!---
Please write the PR title following the conventions at https://www.conventionalcommits.org/en/v1.0.0/
Example:
fix(dashboard): load charts correctly
-->
### SUMMARY
<!--- Describe the change below, including rationale and design decisions -->
This PR addresses a security vulnerability (IDOR) in the legacy /datasource endpoints where metadata could be disclosed to unauthorized users. It also closes a logic gap in the save endpoint that allowed users to bypass ownership checks by omitting specific fields from the request payload.
**Changes**:
- Access Control: Added security_manager.raise_for_access to the get, external_metadata, and external_metadata_by_name endpoints in Datasource view.
- Ownership Enforcement: Refactored the save method to ensure security_manager.raise_for_ownership is always called before updates, regardless of whether the owners field is present in the request.
- Legacy Hardening: While these endpoints are deprecated in favor of the V1 API, they remain active and are now properly secured against unauthorized metadata harvesting.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
<!--- Skip this if not applicable -->
### TESTING INSTRUCTIONS
<!--- Required! What steps can be taken to manually verify the changes? -->
To verify the fix for the reported IDOR:
1. Identify a Datasource ID that your current user does not have access to (e.g., owned by another user or in a private database).
2. Attempt to access the metadata via the legacy endpoint:
`GET /datasource/get/table/<unauthorized_id>/`
3. Verify the response:
- Before Fix: Returns the datasource metadata JSON.
- After Fix: Returns a 403 Forbidden (or redirects to login if unauthenticated).
4. Test the Save Bypass:
- Attempt a POST to /datasource/save/ with a payload containing the ID of a datasource you don't own, but omit the owners key from the JSON data.
- Verify: The request should fail with a 403 Forbidden because ownership is now checked regardless of the payload structure.
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [ ] Has associated issue:
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] Fix/datasource idor access checks [superset]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #38647:
URL: https://github.com/apache/superset/pull/38647#issuecomment-4060315254
## [Codecov](https://app.codecov.io/gh/apache/superset/pull/38647?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `0%` with `5 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 54.25%. Comparing base ([`39cd1cd`](https://app.codecov.io/gh/apache/superset/commit/39cd1cdd43617854e2a0e836a2004862523c98a8?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`7eaf8f0`](https://app.codecov.io/gh/apache/superset/commit/7eaf8f07fdd54638dc34009c45dbf2e917ce83a3?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
| [Files with missing lines](https://app.codecov.io/gh/apache/superset/pull/38647?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [superset/views/datasource/views.py](https://app.codecov.io/gh/apache/superset/pull/38647?src=pr&el=tree&filepath=superset%2Fviews%2Fdatasource%2Fviews.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvdmlld3MvZGF0YXNvdXJjZS92aWV3cy5weQ==) | 0.00% | [5 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/38647?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
:x: Your project check has failed because the head coverage (99.92%) is below the target coverage (100.00%). You can increase the head coverage or adjust the [target](https://docs.codecov.com/docs/commit-status#target) coverage.
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #38647 +/- ##
===========================================
- Coverage 64.99% 54.25% -10.74%
===========================================
Files 1819 2532 +713
Lines 72515 129547 +57032
Branches 23149 29905 +6756
===========================================
+ Hits 47128 70289 +23161
- Misses 25387 58915 +33528
- Partials 0 343 +343
```
| [Flag](https://app.codecov.io/gh/apache/superset/pull/38647/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [presto](https://app.codecov.io/gh/apache/superset/pull/38647/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `40.61% <0.00%> (?)` | |
| [python](https://app.codecov.io/gh/apache/superset/pull/38647/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `40.61% <0.00%> (?)` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/superset/pull/38647?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(datasource): align access validation in legacy views [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on PR #38647:
URL: https://github.com/apache/superset/pull/38647#issuecomment-4102965573
## **Sequence Diagram**
This PR aligns legacy datasource endpoints with modern resource level authorization. Read requests now enforce access checks before returning metadata, and save requests always enforce ownership checks even when owners are omitted from the payload.
```mermaid
sequenceDiagram
participant Client
participant LegacyDatasourceView
participant SecurityManager
participant MetadataBackend
Client->>LegacyDatasourceView: Request datasource metadata
LegacyDatasourceView->>LegacyDatasourceView: Resolve datasource or table by name
LegacyDatasourceView->>SecurityManager: Check access for datasource or database table
SecurityManager-->>LegacyDatasourceView: Access granted
LegacyDatasourceView->>MetadataBackend: Fetch metadata
MetadataBackend-->>LegacyDatasourceView: Metadata result
LegacyDatasourceView-->>Client: Return metadata response
Client->>LegacyDatasourceView: Submit datasource save request
LegacyDatasourceView->>SecurityManager: Verify ownership for update
SecurityManager-->>LegacyDatasourceView: Ownership granted
LegacyDatasourceView-->>Client: Save success response
```
---
*Generated by [CodeAnt AI](https://codeant.ai)*
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] Fix/datasource idor access checks [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #38647:
URL: https://github.com/apache/superset/pull/38647#issuecomment-4060309166
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(datasource): align access validation in legacy views [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #38647:
URL: https://github.com/apache/superset/pull/38647#issuecomment-4103058455
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**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 <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(datasource): align access validation in legacy views [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #38647:
URL: https://github.com/apache/superset/pull/38647#discussion_r2969394455
##########
superset/views/datasource/views.py:
##########
@@ -182,6 +184,14 @@ def external_metadata_by_name(self, **kwargs: Any) -> FlaskResponse:
.filter_by(database_name=params["database_name"])
.one()
)
+ security_manager.raise_for_access(
+ database=database,
+ table=Table(
+ params["table_name"],
+ params["schema_name"],
+ params.get("catalog_name"),
+ ),
+ )
external_metadata = get_physical_table_metadata(
database=database,
table=Table(params["table_name"], params["schema_name"]),
Review Comment:
**Suggestion:** Access is validated against a `Table` that includes `catalog_name`, but metadata is fetched using a different `Table` without catalog, so the authorization target and fetched target can diverge. Build one `Table` object and reuse it for both checks and metadata retrieval to avoid returning/looking up the wrong table in multi-catalog databases. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ `/external_metadata_by_name` may fetch wrong catalog metadata.
- ❌ Dataset column sync can reflect unintended table structure.
- ⚠️ Multi-catalog authorization and retrieval become inconsistent.
```
</details>
```suggestion
table = Table(
params["table_name"],
params["schema_name"],
params.get("catalog_name"),
)
security_manager.raise_for_access(
database=database,
table=table,
)
external_metadata = get_physical_table_metadata(
database=database,
table=table,
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Call `GET /datasource/external_metadata_by_name/?q=...` (endpoint at
`superset/views/datasource/views.py:155`; widely exercised in integration tests at
`tests/integration_tests/datasource_tests.py:187`, `:213`, `:234`, `:253`, `:268`, `:286`,
`:300`).
2. Use parameters that take the "no saved datasource" branch
(`SqlaTable.get_datasource_by_name(...)` returns `None` at
`superset/views/datasource/views.py:169-181`), which is the branch unit-tested in
`tests/unit_tests/views/datasource/views_test.py:186-216`.
3. In that branch, authorization checks `Table(table, schema, catalog)`
(`superset/views/datasource/views.py:187-194`) but metadata fetch uses `Table(table,
schema)` without catalog (`superset/views/datasource/views.py:195-199`).
4. `get_physical_table_metadata()` calls `database.has_table(table)` and
`database.get_columns(table)` (`superset/connectors/sqla/utils.py:50-64`), and those
database methods use `table.catalog` (`superset/models/core.py:1184-1193`, `:1087-1091`),
so missing catalog can inspect a different/default catalog than the one authorized.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/views/datasource/views.py
**Line:** 187:197
**Comment:**
*Logic Error: Access is validated against a `Table` that includes `catalog_name`, but metadata is fetched using a different `Table` without catalog, so the authorization target and fetched target can diverge. Build one `Table` object and reuse it for both checks and metadata retrieval to avoid returning/looking up the wrong table in multi-catalog databases.
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.
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38647&comment_hash=ce79452cdfd7dfb88046939e2b8f93000f958f03c0fc5a062f4df5cad5b28db8&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38647&comment_hash=ce79452cdfd7dfb88046939e2b8f93000f958f03c0fc5a062f4df5cad5b28db8&reaction=dislike'>👎</a>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(datasource): align access validation in legacy views [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #38647:
URL: https://github.com/apache/superset/pull/38647#issuecomment-4104571699
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**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 <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(datasource): align access validation in legacy views [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #38647:
URL: https://github.com/apache/superset/pull/38647#discussion_r2969443446
##########
tests/unit_tests/views/datasource/views_test.py:
##########
@@ -0,0 +1,310 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""Unit tests for resource-level authorization in superset/views/datasource/views.py.
+
+Tests use ``inspect.unwrap`` to call the underlying view logic directly,
+bypassing the Flask-AppBuilder permission decorator machinery.
+"""
+
+import inspect
+from unittest.mock import MagicMock, patch
+
+import pytest
+
+from superset.errors import ErrorLevel, SupersetError, SupersetErrorType
+from superset.exceptions import SupersetSecurityException
+from superset.utils import json as superset_json
+
+
+def _security_exception() -> SupersetSecurityException:
+ return SupersetSecurityException(
+ SupersetError(
+ message="Access denied",
+ error_type=SupersetErrorType.DATASOURCE_SECURITY_ACCESS_ERROR,
+ level=ErrorLevel.WARNING,
+ )
+ )
+
+
+def _get_view_func(name: str):
+ """Return the unwrapped body of a Datasource view method."""
+ from superset.views.datasource.views import Datasource
+
+ return inspect.unwrap(getattr(Datasource, name))
+
+
+def _view_self() -> MagicMock:
+ """Create a minimal stand-in for a Datasource view instance."""
+ self = MagicMock()
+ self.json_response = MagicMock(return_value="ok")
+ return self
+
+
+# ---------------------------------------------------------------------------
+# Datasource.get
+# ---------------------------------------------------------------------------
+
+
+@patch("superset.views.datasource.views.security_manager", new_callable=MagicMock)
+@patch("superset.views.datasource.views.DatasourceDAO.get_datasource")
+def test_get_raises_when_access_denied(
+ mock_get_datasource: MagicMock,
+ mock_security_manager: MagicMock,
+) -> None:
+ """raise_for_access is called and propagates for unauthorised callers."""
+ mock_datasource = MagicMock()
+ mock_get_datasource.return_value = mock_datasource
+ mock_security_manager.raise_for_access.side_effect = _security_exception()
+
+ raw_get = _get_view_func("get")
+ with pytest.raises(SupersetSecurityException):
+ raw_get(_view_self(), "table", 1)
+
+ mock_security_manager.raise_for_access.assert_called_once_with(
+ datasource=mock_datasource
+ )
+
+
+@patch("superset.views.datasource.views.sanitize_datasource_data")
+@patch("superset.views.datasource.views.security_manager", new_callable=MagicMock)
+@patch("superset.views.datasource.views.DatasourceDAO.get_datasource")
+def test_get_succeeds_for_authorised_user(
+ mock_get_datasource: MagicMock,
+ mock_security_manager: MagicMock,
+ mock_sanitize: MagicMock,
+) -> None:
+ """raise_for_access is called without raising for authorised callers."""
+ mock_datasource = MagicMock()
+ mock_datasource.data = {"id": 1}
+ mock_get_datasource.return_value = mock_datasource
+ mock_security_manager.raise_for_access.return_value = None
+ mock_sanitize.return_value = {"id": 1}
+
+ raw_get = _get_view_func("get")
+ raw_get(_view_self(), "table", 1)
+
+ mock_security_manager.raise_for_access.assert_called_once_with(
+ datasource=mock_datasource
+ )
+
+
+# ---------------------------------------------------------------------------
+# Datasource.external_metadata
+# ---------------------------------------------------------------------------
+
+
+@patch("superset.views.datasource.views.security_manager", new_callable=MagicMock)
+@patch("superset.views.datasource.views.DatasourceDAO.get_datasource")
+def test_external_metadata_raises_when_access_denied(
+ mock_get_datasource: MagicMock,
+ mock_security_manager: MagicMock,
+) -> None:
+ mock_datasource = MagicMock()
+ mock_get_datasource.return_value = mock_datasource
+ mock_security_manager.raise_for_access.side_effect = _security_exception()
+
+ raw_fn = _get_view_func("external_metadata")
+ with pytest.raises(SupersetSecurityException):
+ raw_fn(_view_self(), "table", 1)
+
+ mock_security_manager.raise_for_access.assert_called_once_with(
+ datasource=mock_datasource
+ )
+
+
+@patch("superset.views.datasource.views.security_manager", new_callable=MagicMock)
+@patch("superset.views.datasource.views.DatasourceDAO.get_datasource")
+def test_external_metadata_succeeds_for_authorised_user(
+ mock_get_datasource: MagicMock,
+ mock_security_manager: MagicMock,
+) -> None:
+ mock_datasource = MagicMock()
+ mock_datasource.external_metadata.return_value = [{"name": "col1"}]
+ mock_get_datasource.return_value = mock_datasource
+ mock_security_manager.raise_for_access.return_value = None
+
+ raw_fn = _get_view_func("external_metadata")
+ raw_fn(_view_self(), "table", 1)
+
+ mock_security_manager.raise_for_access.assert_called_once_with(
+ datasource=mock_datasource
+ )
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Missing behavior assertion</b></div>
<div id="fix">
Per BITO rule [6262], assert the method's output to verify external_metadata logic, not just security.
</div>
</div>
<small><i>Code Review Run #cbe1f3</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
tests/unit_tests/views/datasource/views_test.py:
##########
@@ -0,0 +1,310 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""Unit tests for resource-level authorization in superset/views/datasource/views.py.
+
+Tests use ``inspect.unwrap`` to call the underlying view logic directly,
+bypassing the Flask-AppBuilder permission decorator machinery.
+"""
+
+import inspect
+from unittest.mock import MagicMock, patch
+
+import pytest
+
+from superset.errors import ErrorLevel, SupersetError, SupersetErrorType
+from superset.exceptions import SupersetSecurityException
+from superset.utils import json as superset_json
+
+
+def _security_exception() -> SupersetSecurityException:
+ return SupersetSecurityException(
+ SupersetError(
+ message="Access denied",
+ error_type=SupersetErrorType.DATASOURCE_SECURITY_ACCESS_ERROR,
+ level=ErrorLevel.WARNING,
+ )
+ )
+
+
+def _get_view_func(name: str):
+ """Return the unwrapped body of a Datasource view method."""
+ from superset.views.datasource.views import Datasource
+
+ return inspect.unwrap(getattr(Datasource, name))
+
+
+def _view_self() -> MagicMock:
+ """Create a minimal stand-in for a Datasource view instance."""
+ self = MagicMock()
+ self.json_response = MagicMock(return_value="ok")
+ return self
+
+
+# ---------------------------------------------------------------------------
+# Datasource.get
+# ---------------------------------------------------------------------------
+
+
+@patch("superset.views.datasource.views.security_manager", new_callable=MagicMock)
+@patch("superset.views.datasource.views.DatasourceDAO.get_datasource")
+def test_get_raises_when_access_denied(
+ mock_get_datasource: MagicMock,
+ mock_security_manager: MagicMock,
+) -> None:
+ """raise_for_access is called and propagates for unauthorised callers."""
+ mock_datasource = MagicMock()
+ mock_get_datasource.return_value = mock_datasource
+ mock_security_manager.raise_for_access.side_effect = _security_exception()
+
+ raw_get = _get_view_func("get")
+ with pytest.raises(SupersetSecurityException):
+ raw_get(_view_self(), "table", 1)
+
+ mock_security_manager.raise_for_access.assert_called_once_with(
+ datasource=mock_datasource
+ )
+
+
+@patch("superset.views.datasource.views.sanitize_datasource_data")
+@patch("superset.views.datasource.views.security_manager", new_callable=MagicMock)
+@patch("superset.views.datasource.views.DatasourceDAO.get_datasource")
+def test_get_succeeds_for_authorised_user(
+ mock_get_datasource: MagicMock,
+ mock_security_manager: MagicMock,
+ mock_sanitize: MagicMock,
+) -> None:
+ """raise_for_access is called without raising for authorised callers."""
+ mock_datasource = MagicMock()
+ mock_datasource.data = {"id": 1}
+ mock_get_datasource.return_value = mock_datasource
+ mock_security_manager.raise_for_access.return_value = None
+ mock_sanitize.return_value = {"id": 1}
+
+ raw_get = _get_view_func("get")
+ raw_get(_view_self(), "table", 1)
+
+ mock_security_manager.raise_for_access.assert_called_once_with(
+ datasource=mock_datasource
+ )
+
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Missing behavior assertion</b></div>
<div id="fix">
Per BITO rule [6262], tests should assert actual behavior logic, not just security calls. Here, verify the method returns sanitized data via json_response.
</div>
</div>
<small><i>Code Review Run #cbe1f3</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(datasource): align access validation in legacy views [superset]
Posted by "dpgaspar (via GitHub)" <gi...@apache.org>.
dpgaspar merged PR #38647:
URL: https://github.com/apache/superset/pull/38647
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(datasource): align access validation in legacy views [superset]
Posted by "netlify[bot] (via GitHub)" <gi...@apache.org>.
netlify[bot] commented on PR #38647:
URL: https://github.com/apache/superset/pull/38647#issuecomment-4119083128
### <span aria-hidden="true">✅</span> Deploy Preview for *superset-docs-preview* ready!
| Name | Link |
|:-:|------------------------|
|<span aria-hidden="true">🔨</span> Latest commit | bbbc99c6fd7a2c8de4477f6b2a150c2969c7039e |
|<span aria-hidden="true">🔍</span> Latest deploy log | https://app.netlify.com/projects/superset-docs-preview/deploys/69c2a93add7c7500086a7794 |
|<span aria-hidden="true">😎</span> Deploy Preview | [https://deploy-preview-38647--superset-docs-preview.netlify.app](https://deploy-preview-38647--superset-docs-preview.netlify.app) |
|<span aria-hidden="true">📱</span> Preview on mobile | <details><summary> Toggle QR Code... </summary><br /><br /><br /><br />_Use your smartphone camera to open QR code link._</details> |
---
<!-- [superset-docs-preview Preview](https://deploy-preview-38647--superset-docs-preview.netlify.app) -->
_To edit notification comments on pull requests, go to your [Netlify project configuration](https://app.netlify.com/projects/superset-docs-preview/configuration/notifications#deploy-notifications)._
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org