mikebridge commented on code in PR #43887:
URL: https://github.com/apache/superset/pull/43887#discussion_r4048236709
##########
superset/semantic_layers/api.py:
##########
@@ -1083,10 +1087,12 @@ def _fetch_connection_items(
source_type: str,
name_filter: str | None,
) -> list[tuple[str, Any]]:
- """Fetch database and semantic layer items based on filters."""
+ """Fetch permitted sources using the same FAB identity as the route
gate."""
db_items: list[tuple[str, Database]] = []
- if source_type in ("all", "database"):
- db_q = db.session.query(Database).options(
+ if source_type in ("all", "database") and security_manager.has_access(
+ "can_read", "Database"
+ ):
Review Comment:
`has_access` already handles anonymous public grants in the pinned
Flask-AppBuilder 5.2.3: after its API-key, session and JWT checks it [returns
`is_item_public`](https://github.com/dpgaspar/Flask-AppBuilder/blob/v5.2.3/flask_appbuilder/security/manager.py#L1932).
I verified the combined endpoint with an anonymous client and real
`protect_read`/`has_access`/`is_item_public`: both Database-only and
SemanticLayer-only public grants queried exactly the permitted source, never
the denied source (2 passing cases). The local venv uses 5.2.2; its complete
`has_access` method is AST-identical to pinned 5.2.3. No code change is needed
for this finding.
##########
superset-frontend/src/pages/DatabaseList/index.tsx:
##########
@@ -798,13 +823,13 @@ function DatabaseList({
const isSemanticLayer = original.source_type === 'semantic_layer';
if (isSemanticLayer) {
- if (!canEdit && !canDelete) return null;
+ if (!canWriteLayer) return null;
const isLoadingDependents =
slDeletePreview?.status === 'loading' &&
slDeletePreview.item.uuid === original.uuid;
return (
<div className="actions">
- {canDelete && (
+ {canWriteLayer && (
Review Comment:
Confirmed in
[d5b760fc](https://github.com/apache/superset/commit/d5b760fc253301706e52c47ef7fb05dc551c9770):
the semantic row retains the `canWriteLayer` early return and removes the
redundant Delete wrapper. Loading and confirmation behavior are unchanged.
##########
superset-frontend/src/pages/DatabaseList/index.tsx:
##########
@@ -824,7 +849,7 @@ function DatabaseList({
onClick={() => openSemanticLayerDeleteModal(original)}
/>
)}
- {canEdit && (
+ {canWriteLayer && (
Review Comment:
Confirmed in
[d5b760fc](https://github.com/apache/superset/commit/d5b760fc253301706e52c47ef7fb05dc551c9770):
Edit relies on the same `canWriteLayer` early return; the redundant inner
wrapper is removed.
##########
superset-frontend/src/pages/DatabaseList/index.tsx:
##########
@@ -894,7 +919,7 @@ function DatabaseList({
},
Header: t('Actions'),
id: 'actions',
- hidden: !canEdit && !canDelete,
+ hidden: !canEdit && !canDelete && !canWriteLayer,
Review Comment:
Confirmed in
[d5b760fc](https://github.com/apache/superset/commit/d5b760fc253301706e52c47ef7fb05dc551c9770):
`canExport` keeps the Actions column visible for export-only users. The
regression checks Export is present while Edit and Delete are absent.
##########
superset-frontend/src/pages/DatasetList/DatasetList.connectionPermissions.test.tsx:
##########
@@ -0,0 +1,69 @@
+/**
+ * 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.
+ */
+import fetchMock from 'fetch-mock';
+import { screen, waitFor } from '@testing-library/react';
+import userEvent from '@testing-library/user-event';
+import {
+ setupMocks,
+ renderDatasetList,
+ mockAdminUser,
+} from './DatasetList.testHelpers';
+
+beforeEach(() => {
+ setupMocks();
+ window.featureFlags = { SEMANTIC_LAYERS: true } as never;
+ fetchMock.get('glob:*/api/v1/semantic_layer/?*', { result: [], count: 0 });
+});
+
+afterEach(() => {
+ window.featureFlags = {} as never;
+ fetchMock.clearHistory().removeRoutes();
+ jest.restoreAllMocks();
+});
+
+test.each([false, true])(
+ 'dataset connection options respect independent layer read (%s)',
+ async canReadLayer => {
+ const user = {
+ ...mockAdminUser,
+ roles: {
+ Admin: [
+ ...mockAdminUser.roles.Admin,
+ ...(canReadLayer ? [['can_read', 'SemanticLayer']] : []),
+ ],
+ },
+ };
+ renderDatasetList(user);
+ await screen.findByTestId('search-filter-container');
+ const filter = screen
+ .getAllByTestId('compact-filter-pill')
+ .find(item => item.textContent?.includes('Data connection'));
+ expect(filter).toBeDefined();
+ await userEvent.click(filter!);
+ await waitFor(() =>
+ expect(
+ fetchMock.callHistory.calls('glob:*/api/v1/dataset/related/database*')
+ .length,
+ ).toBeGreaterThan(0),
+ );
+ expect(
+ fetchMock.callHistory.calls('glob:*/api/v1/semantic_layer/?*').length >
0,
Review Comment:
Confirmed in
[d5b760fc](https://github.com/apache/superset/commit/d5b760fc253301706e52c47ef7fb05dc551c9770):
the semantic-layer route glob is shared through `API_ENDPOINTS`, and the
assertion reuses the related-database endpoint constant. Both affected frontend
suites pass (15 tests).
--
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]
