rusackas commented on code in PR #38584:
URL: https://github.com/apache/superset/pull/38584#discussion_r3662397187
##########
superset/commands/chart/update.py:
##########
@@ -87,14 +87,20 @@ 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 ownership
+ # 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))
unauthorized_dashboard_ids = new_dashboard_ids - {
d.id for d in accessible_dashboards
}
if unauthorized_dashboard_ids:
exceptions.append(DashboardsNotFoundValidationError())
+ return
+
+ for dash in accessible_dashboards:
+ if dash.is_managed_externally or not
security_manager.is_owner(dash):
+ raise DashboardsForbiddenError()
Review Comment:
This doesn't match what's in the file now, `is_owner` isn't used in
`update.py` at all. Looks like it got flagged on an older revision before the
duplicate loop was refactored away in favor of `is_editor`.
##########
superset-frontend/src/explore/components/SaveModal.test.tsx:
##########
@@ -36,14 +36,29 @@ import SaveModal, {
import { CHART_WIDTH } from 'src/dashboard/constants';
import { GRID_COLUMN_COUNT } from 'src/dashboard/util/constants';
+// Captures the AsyncSelect `options` loader (SaveModal's loadDashboards) so
+// tests can invoke it directly and assert on the request it issues.
+let mockLoadDashboards:
+ | ((search: string, page: number, pageSize: number) => Promise<unknown>)
+ | undefined;
Review Comment:
This is handled already, there's an `afterEach` that resets
`mockLoadDashboards` along with the fetch routes.
##########
superset-frontend/src/explore/components/SaveModal.test.tsx:
##########
@@ -266,6 +281,87 @@ 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.removeRoutes();
+ fetchMock.get(fetchDashboardsEndpoint, mockDashboardData);
+ fetchMock.get(fetchChartEndpoint, { id: 1, dashboards: [1] });
+ fetchMock.get(`glob:*/api/v1/dashboard/${dashboardId}`, {
+ result: {
+ id: dashboardId,
+ dashboard_title: 'Managed Dashboard',
+ owners: [{ id: 1 }],
+ is_managed_externally: 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).toBeInTheDocument();
+ expect(
+ fetchMock.callHistory.calls(`glob:*/api/v1/dashboard/${dashboardId}/tabs`),
+ ).toHaveLength(0);
+
+ // Restore default mocks
+ fetchMock.removeRoutes();
+ fetchMock.clearHistory();
+ fetchMock.get(fetchDashboardsEndpoint, mockDashboardData);
+ fetchMock.get(fetchChartEndpoint, { id: 1, dashboards: [1] });
+ fetchMock.get(fetchDashboardEndpoint, {
+ result: [{ id: 'id', dashboard_title: 'dashboard title' }],
+ });
Review Comment:
Same here, the manual end-of-test restore is gone, cleanup now happens in a
guaranteed `afterEach` so a failed assertion can't leak routes.
##########
superset-frontend/src/explore/components/SaveModal.test.tsx:
##########
@@ -36,14 +36,29 @@ import SaveModal, {
import { CHART_WIDTH } from 'src/dashboard/constants';
import { GRID_COLUMN_COUNT } from 'src/dashboard/util/constants';
+// Captures the AsyncSelect `options` loader (SaveModal's loadDashboards) so
+// tests can invoke it directly and assert on the request it issues.
+let mockLoadDashboards:
+ | ((search: string, page: number, pageSize: number) => Promise<unknown>)
+ | undefined;
+
jest.mock('@superset-ui/core/components/Select', () => ({
...jest.requireActual('@superset-ui/core/components/Select/AsyncSelect'),
- AsyncSelect: ({ onChange }: { onChange: (val: any) => void }) => (
- <input
- data-test="mock-async-select"
- onChange={({ target: { value } }) => onChange({ label: value, value })}
- />
- ),
+ AsyncSelect: ({
+ onChange,
+ options,
+ }: {
+ onChange: (val: any) => void;
Review Comment:
Good catch, swapped that for the real `SelectValue` type.
--
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]
