codeant-ai-for-open-source[bot] commented on code in PR #38584:
URL: https://github.com/apache/superset/pull/38584#discussion_r2960992688
##########
superset-frontend/src/explore/components/SaveModal.test.tsx:
##########
@@ -240,6 +240,83 @@ test('renders a message when saving as with new
dashboard', () => {
);
});
+test('does not preselect an externally managed dashboard on mount', async ()
=> {
+ const dashboardId = 1;
+ fetchMock.get(
+ `glob:*/api/v1/dashboard/${dashboardId}`,
+ {
+ result: {
+ id: dashboardId,
+ dashboard_title: 'Managed Dashboard',
+ owners: [{ id: 1 }],
+ is_managed_externally: true,
+ },
+ },
+ { overwriteRoutes: true },
+ );
+
+ const store = mockStore({
+ ...initialState,
+ explore: {
+ ...initialState.explore,
+ slice: {
+ ...initialState.explore.slice,
+ dashboards: [dashboardId],
+ },
+ },
+ });
+
+ const { queryByTestId } = setup(
+ {
+ ...defaultProps,
+ dashboardId,
+ },
+ store,
+ );
+
+ await waitFor(() => {
+ expect(
+ fetchMock.callHistory.calls(`glob:*/api/v1/dashboard/${dashboardId}`),
+ ).toHaveLength(1);
+ });
+
+ const selectInput = queryByTestId('mock-async-select') as HTMLInputElement;
+ expect(selectInput?.value).toBeFalsy();
Review Comment:
**Suggestion:** The assertion checks the mocked input's `value`, but this
`AsyncSelect` mock never receives/binds a `value` prop, so the field stays
empty regardless of whether a dashboard was preselected. This can let
regressions slip through with a false-positive test. Assert the behavioral side
effect instead (no tabs request for that dashboard), which only happens when
preselection occurs. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ SaveModal preselection regressions can pass CI unnoticed.
- ⚠️ Externally managed dashboard guard may silently break.
- ⚠️ Test suite gives false confidence on mount behavior.
```
</details>
```suggestion
expect(selectInput).toBeInTheDocument();
expect(
fetchMock.callHistory.calls(`glob:*/api/v1/dashboard/${dashboardId}/tabs`),
).toHaveLength(0);
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. In `superset-frontend/src/explore/components/SaveModal.test.tsx:42-46`,
the mocked
`AsyncSelect` renders `<input data-test="mock-async-select" ...>` and only
forwards
`onChange`; it does not bind any `value` prop.
2. In production component code,
`superset-frontend/src/explore/components/SaveModal.tsx:675-679`,
`AsyncSelect` receives
`value={this.state.dashboard}`, so preselection is represented through
component state,
not raw DOM input typing.
3. On mount,
`superset-frontend/src/explore/components/SaveModal.tsx:148-159` preselects a
dashboard and calls `loadTabs(dashboardId)` when conditions pass; this is
the behavior
that indicates an actual preselection side effect.
4. The current test assertion at `SaveModal.test.tsx:283-284` checks
`selectInput?.value`,
but because the mock input is uncontrolled and never receives bound value,
that assertion
remains falsy even if preselection logic regresses—creating a false-positive
test.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/explore/components/SaveModal.test.tsx
**Line:** 284:284
**Comment:**
*Logic Error: The assertion checks the mocked input's `value`, but this
`AsyncSelect` mock never receives/binds a `value` prop, so the field stays
empty regardless of whether a dashboard was preselected. This can let
regressions slip through with a false-positive test. Assert the behavioral side
effect instead (no tabs request for that dashboard), which only happens when
preselection occurs.
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%2F38584&comment_hash=ae3c8b9781e83c42d6d7c9cf5805b467df976029b6aa2055cb317b1fafbc11c8&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38584&comment_hash=ae3c8b9781e83c42d6d7c9cf5805b467df976029b6aa2055cb317b1fafbc11c8&reaction=dislike'>👎</a>
##########
superset/commands/chart/update.py:
##########
@@ -86,13 +87,19 @@ def _validate_new_dashboard_access(
requested_dashboard_ids = {d.id for d in requested_dashboards}
if new_dashboard_ids := requested_dashboard_ids -
existing_dashboard_ids:
- # For NEW dashboard relationships, verify user has access
+ # For NEW dashboard relationships, verify user has access first
+ # to avoid leaking information about inaccessible dashboards
accessible_dashboards =
DashboardDAO.find_by_ids(list(new_dashboard_ids))
accessible_dashboard_ids = {d.id for d in accessible_dashboards}
unauthorized_dashboard_ids = new_dashboard_ids -
accessible_dashboard_ids
if unauthorized_dashboard_ids:
exceptions.append(DashboardsNotFoundValidationError())
+ return
+
+ for dash in accessible_dashboards:
+ if dash.is_managed_externally:
Review Comment:
**Suggestion:** New dashboard relationships are only checked for visibility,
not edit permission. A user with read access to a published dashboard can still
attach a chart to it, which modifies that dashboard without ownership rights.
Align this with chart creation logic by also requiring dashboard ownership
before allowing the relationship. [logic error]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ PUT /api/v1/chart allows unauthorized dashboard chart additions.
- ⚠️ Published dashboards become mutable by non-owner chart editors.
- ⚠️ Update/create permission behavior diverges across chart commands.
```
</details>
```suggestion
if dash.is_managed_externally or not
security_manager.is_owner(dash):
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Use a non-admin user who owns a chart but does not own a published
dashboard; this is
realistic because `DashboardAccessFilter` explicitly includes published
dashboards for
non-owners (`superset/dashboards/filters.py:104-109`, `:132-139`,
`:185-190`).
2. Call chart update endpoint `PUT /api/v1/chart/<pk>`
(`superset/charts/api.py:389`),
sending `dashboards` containing that published dashboard ID; request is
executed via
`UpdateChartCommand(pk, item).run()` at `superset/charts/api.py:437`.
3. In validation, requested dashboard IDs are first loaded with
`skip_base_filter=True`
(`superset/commands/chart/update.py:154-156`), then
`_validate_new_dashboard_access()` is
called (`superset/commands/chart/update.py:162`).
4. `_validate_new_dashboard_access()` re-fetches only new dashboard IDs with
base filter
(`superset/commands/chart/update.py:92`), and only blocks
`is_managed_externally`
(`superset/commands/chart/update.py:101-102`), so non-owner writable check
is missing and
request succeeds (no `DashboardsForbiddenError`), unlike create flow which
enforces
ownership (`superset/commands/chart/create.py:74-75`).
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/commands/chart/update.py
**Line:** 101:101
**Comment:**
*Logic Error: New dashboard relationships are only checked for
visibility, not edit permission. A user with read access to a published
dashboard can still attach a chart to it, which modifies that dashboard without
ownership rights. Align this with chart creation logic by also requiring
dashboard ownership before allowing the relationship.
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%2F38584&comment_hash=07847d94e03a40760f80e7f5f1bda043e4c368ba9ee0b638833e025b13de70da&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38584&comment_hash=07847d94e03a40760f80e7f5f1bda043e4c368ba9ee0b638833e025b13de70da&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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
