pt
/
superset
mirror of https://github.com/apache/superset.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

fix(tags): Polish + Better messaging for skipped tags with bad permissions (#25578)

This commit is contained in:
Hugh A. Miles II 2023-10-13 13:13:59 -04:00 committed by GitHub
parent b370c66308
commit 9074f72959
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
12 changed files with 254 additions and 65 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

13
superset-frontend/src/features/tags/BulkTagModal.tsx
View File

@ -67,7 +67,18 @@ const BulkTagModal: React.FC<BulkTagModalProps> = ({
},
})
.then(({ json = {} }) => {
addSuccessToast(t('Tagged %s %ss', selected.length, resourceName));
const skipped = json.result.objects_skipped;
const tagged = json.result.objects_tagged;
if (skipped.length > 0) {
addSuccessToast(
t(
'%s items could not be tagged because you don’t have edit permissions to all selected objects.',
skipped.length,
resourceName,
),
);
}
addSuccessToast(t('Tagged %s %ss', tagged.length, resourceName));
})
.catch(err => {
addDangerToast(t('Failed to tag items'));

31
superset-frontend/src/pages/AllEntities/index.tsx
View File

@ -164,21 +164,25 @@ function AllEntities() {
);
};
const fetchTag = (tagId: number) => {
fetchSingleTag(
tagId,
(tag: Tag) => {
setTag(tag);
setLoading(false);
},
(error: Response) => {
addDangerToast(t('Error Fetching Tagged Objects'));
setLoading(false);
},
);
};
useEffect(() => {
// fetch single tag met
if (tagId) {
setLoading(true);
fetchSingleTag(
tagId,
(tag: Tag) => {
setTag(tag);
setLoading(false);
},
(error: Response) => {
addDangerToast(t('Error Fetching Tagged Objects'));
setLoading(false);
},
);
fetchTag(tagId);
}
}, [tagId]);
@ -197,7 +201,10 @@ function AllEntities() {
editTag={tag}
addSuccessToast={addSuccessToast}
addDangerToast={addDangerToast}
refreshData={fetchTaggedObjects}
refreshData={() => {
fetchTaggedObjects();
if (tagId) fetchTag(tagId);
}}
/>
<AllEntitiesNav>
<PageHeaderWithActions

42
superset-frontend/src/pages/ChartList/ChartList.test.jsx
View File

@ -20,7 +20,7 @@ import React from 'react';
import { MemoryRouter } from 'react-router-dom';
import thunk from 'redux-thunk';
import configureStore from 'redux-mock-store';
import { Provider } from 'react-redux';
import * as reactRedux from 'react-redux';
import fetchMock from 'fetch-mock';
import * as uiCore from '@superset-ui/core';
import waitForComponentToPaint from 'spec/helpers/waitForComponentToPaint';
@ -38,9 +38,6 @@ import ListViewCard from 'src/components/ListViewCard';
import FaveStar from 'src/components/FaveStar';
import TableCollection from 'src/components/TableCollection';
import CardCollection from 'src/components/ListView/CardCollection';
// store needed for withToasts(ChartTable)
const mockStore = configureStore([thunk]);
const store = mockStore({});
const chartsInfoEndpoint = 'glob:*/api/v1/chart/_info*';
const chartsOwnersEndpoint = 'glob:*/api/v1/chart/related/owners*';
@ -99,6 +96,29 @@ fetchMock.get(datasetEndpoint, {});
global.URL.createObjectURL = jest.fn();
fetchMock.get('/thumbnail', { body: new Blob(), sendAsJson: false });
const user = {
createdOn: '2021-04-27T18:12:38.952304',
email: 'admin',
firstName: 'admin',
isActive: true,
lastName: 'admin',
permissions: {},
roles: {
Admin: [
['can_sqllab', 'Superset'],
['can_write', 'Dashboard'],
['can_write', 'Chart'],
],
},
userId: 1,
username: 'admin',
};
// store needed for withToasts(DatabaseList)
const mockStore = configureStore([thunk]);
const store = mockStore({ user });
const useSelectorMock = jest.spyOn(reactRedux, 'useSelector');
describe('ChartList', () => {
const isFeatureEnabledMock = jest
.spyOn(uiCore, 'isFeatureEnabled')
@ -107,6 +127,12 @@ describe('ChartList', () => {
afterAll(() => {
isFeatureEnabledMock.restore();
});
beforeEach(() => {
// setup a DOM element as a render target
useSelectorMock.mockClear();
});
const mockedProps = {};
let wrapper;
@ -114,9 +140,9 @@ describe('ChartList', () => {
beforeAll(async () => {
wrapper = mount(
<MemoryRouter>
<Provider store={store}>
<reactRedux.Provider store={store}>
<ChartList {...mockedProps} user={mockUser} />
</Provider>
</reactRedux.Provider>
</MemoryRouter>,
);
@ -231,9 +257,9 @@ describe('ChartList - anonymous view', () => {
fetchMock.resetHistory();
wrapper = mount(
<MemoryRouter>
<Provider store={store}>
<reactRedux.Provider store={store}>
<ChartList {...mockedProps} user={mockUserLoggedOut} />
</Provider>
</reactRedux.Provider>
</MemoryRouter>,
);

9
superset-frontend/src/pages/ChartList/index.tsx
View File

@ -30,6 +30,7 @@ import React, { useState, useMemo, useCallback } from 'react';
import rison from 'rison';
import { uniqBy } from 'lodash';
import moment from 'moment';
import { useSelector } from 'react-redux';
import {
createErrorHandler,
createFetchRelated,
@ -71,6 +72,8 @@ import { GenericLink } from 'src/components/GenericLink/GenericLink';
import Owner from 'src/types/Owner';
import { loadTags } from 'src/components/Tags/utils';
import ChartCard from 'src/features/charts/ChartCard';
import { UserWithPermissionsAndRoles } from 'src/types/bootstrapTypes';
import { findPermission } from 'src/utils/findPermission';
const FlexRowContainer = styled.div`
align-items: center;
@ -179,6 +182,10 @@ function ChartList(props: ChartListProps) {
} = useListViewResource<Chart>('chart', t('chart'), addDangerToast);
const chartIds = useMemo(() => charts.map(c => c.id), [charts]);
const { roles } = useSelector<any, UserWithPermissionsAndRoles>(
state => state.user,
);
const canReadTag = findPermission('can_read', 'Tag', roles);
const [saveFavoriteStatus, favoriteStatus] = useFavoriteStatus(
'chart',
@ -701,7 +708,7 @@ function ChartList(props: ChartListProps) {
],
},
] as Filters;
if (isFeatureEnabled(FeatureFlag.TAGGING_SYSTEM)) {
if (isFeatureEnabled(FeatureFlag.TAGGING_SYSTEM) && canReadTag) {
filters_list.push({
Header: t('Tags'),
key: 'tags',

41
superset-frontend/src/pages/DashboardList/DashboardList.test.jsx
View File

@ -21,7 +21,7 @@ import { MemoryRouter } from 'react-router-dom';
import thunk from 'redux-thunk';
import configureStore from 'redux-mock-store';
import fetchMock from 'fetch-mock';
import { Provider } from 'react-redux';
import * as reactRedux from 'react-redux';
import * as uiCore from '@superset-ui/core';
import waitForComponentToPaint from 'spec/helpers/waitForComponentToPaint';
@ -40,10 +40,6 @@ import FaveStar from 'src/components/FaveStar';
import TableCollection from 'src/components/TableCollection';
import CardCollection from 'src/components/ListView/CardCollection';
// store needed for withToasts(DashboardTable)
const mockStore = configureStore([thunk]);
const store = mockStore({});
const dashboardsInfoEndpoint = 'glob:*/api/v1/dashboard/_info*';
const dashboardOwnersEndpoint = 'glob:*/api/v1/dashboard/related/owners*';
const dashboardCreatedByEndpoint =
@ -95,6 +91,28 @@ fetchMock.get(dashboardEndpoint, {
global.URL.createObjectURL = jest.fn();
fetchMock.get('/thumbnail', { body: new Blob(), sendAsJson: false });
const user = {
createdOn: '2021-04-27T18:12:38.952304',
email: 'admin',
firstName: 'admin',
isActive: true,
lastName: 'admin',
permissions: {},
roles: {
Admin: [
['can_sqllab', 'Superset'],
['can_write', 'Dashboard'],
['can_write', 'Chart'],
],
},
userId: 1,
username: 'admin',
};
// store needed for withToasts(DatabaseList)
const mockStore = configureStore([thunk]);
const store = mockStore({ user });
const useSelectorMock = jest.spyOn(reactRedux, 'useSelector');
describe('DashboardList', () => {
const isFeatureEnabledMock = jest
@ -105,6 +123,11 @@ describe('DashboardList', () => {
isFeatureEnabledMock.restore();
});
beforeEach(() => {
// setup a DOM element as a render target
useSelectorMock.mockClear();
});
const mockedProps = {};
let wrapper;
@ -112,9 +135,9 @@ describe('DashboardList', () => {
fetchMock.resetHistory();
wrapper = mount(
<MemoryRouter>
<Provider store={store}>
<reactRedux.Provider store={store}>
<DashboardList {...mockedProps} user={mockUser} />
</Provider>
</reactRedux.Provider>
</MemoryRouter>,
);
@ -249,9 +272,9 @@ describe('DashboardList - anonymous view', () => {
fetchMock.resetHistory();
wrapper = mount(
<MemoryRouter>
<Provider store={store}>
<reactRedux.Provider store={store}>
<DashboardList {...mockedProps} user={mockUserLoggedOut} />
</Provider>
</reactRedux.Provider>
</MemoryRouter>,
);

10
superset-frontend/src/pages/DashboardList/index.tsx
View File

@ -23,6 +23,7 @@ import {
SupersetClient,
t,
} from '@superset-ui/core';
import { useSelector } from 'react-redux';
import React, { useState, useMemo, useCallback } from 'react';
import { Link } from 'react-router-dom';
import rison from 'rison';
@ -61,6 +62,8 @@ import CertifiedBadge from 'src/components/CertifiedBadge';
import { loadTags } from 'src/components/Tags/utils';
import DashboardCard from 'src/features/dashboards/DashboardCard';
import { DashboardStatus } from 'src/features/dashboards/types';
import { UserWithPermissionsAndRoles } from 'src/types/bootstrapTypes';
import { findPermission } from 'src/utils/findPermission';
const PAGE_SIZE = 25;
const PASSWORDS_NEEDED_MESSAGE = t(
@ -111,6 +114,11 @@ function DashboardList(props: DashboardListProps) {
user: { userId },
} = props;
const { roles } = useSelector<any, UserWithPermissionsAndRoles>(
state => state.user,
);
const canReadTag = findPermission('can_read', 'Tag', roles);
const {
state: {
loading,
@ -578,7 +586,7 @@ function DashboardList(props: DashboardListProps) {
],
},
] as Filters;
if (isFeatureEnabled(FeatureFlag.TAGGING_SYSTEM)) {
if (isFeatureEnabled(FeatureFlag.TAGGING_SYSTEM) && canReadTag) {
filters_list.push({
Header: t('Tags'),
key: 'tags',

39
superset-frontend/src/pages/SavedQueryList/SavedQueryList.test.jsx
View File

@ -18,9 +18,9 @@
*/
import React from 'react';
import thunk from 'redux-thunk';
import * as reactRedux from 'react-redux';
import { BrowserRouter } from 'react-router-dom';
import configureStore from 'redux-mock-store';
import { Provider } from 'react-redux';
import fetchMock from 'fetch-mock';
import { styledMount as mount } from 'spec/helpers/theming';
import { render, screen, cleanup, waitFor } from 'spec/helpers/testing-library';
@ -38,10 +38,6 @@ import Button from 'src/components/Button';
import IndeterminateCheckbox from 'src/components/IndeterminateCheckbox';
import waitForComponentToPaint from 'spec/helpers/waitForComponentToPaint';
// store needed for withToasts(DatabaseList)
const mockStore = configureStore([thunk]);
const store = mockStore({});
const queriesInfoEndpoint = 'glob:*/api/v1/saved_query/_info*';
const queriesEndpoint = 'glob:*/api/v1/saved_query/?*';
const queryEndpoint = 'glob:*/api/v1/saved_query/*';
@ -75,6 +71,30 @@ const mockqueries = [...new Array(3)].map((_, i) => ({
],
}));
const user = {
createdOn: '2021-04-27T18:12:38.952304',
email: 'admin',
firstName: 'admin',
isActive: true,
lastName: 'admin',
permissions: {},
roles: {
Admin: [
['can_sqllab', 'Superset'],
['can_write', 'Dashboard'],
['can_write', 'Chart'],
],
},
userId: 1,
username: 'admin',
};
// store needed for withToasts(DatabaseList)
const mockStore = configureStore([thunk]);
const store = mockStore({ user });
const useSelectorMock = jest.spyOn(reactRedux, 'useSelector');
// ---------- For import testing ----------
// Create an one more mocked query than the original mocked query array
const mockOneMoreQuery = [...new Array(mockqueries.length + 1)].map((_, i) => ({
@ -137,11 +157,16 @@ jest.mock('src/views/CRUD/utils');
describe('SavedQueryList', () => {
const wrapper = mount(
<Provider store={store}>
<reactRedux.Provider store={store}>
<SavedQueryList />
</Provider>,
</reactRedux.Provider>,
);
beforeEach(() => {
// setup a DOM element as a render target
useSelectorMock.mockClear();
});
beforeAll(async () => {
await waitForComponentToPaint(wrapper);
});

29
superset-frontend/src/pages/SavedQueryList/index.tsx
View File

@ -33,6 +33,7 @@ import {
createFetchDistinct,
createErrorHandler,
} from 'src/views/CRUD/utils';
import { useSelector } from 'react-redux';
import Popover from 'src/components/Popover';
import withToasts from 'src/components/MessageToasts/withToasts';
import { useListViewResource } from 'src/views/CRUD/hooks';
@ -55,8 +56,12 @@ import copyTextToClipboard from 'src/utils/copy';
import Tag from 'src/types/TagType';
import ImportModelsModal from 'src/components/ImportModal/index';
import Icons from 'src/components/Icons';
import { BootstrapUser } from 'src/types/bootstrapTypes';
import {
BootstrapUser,
UserWithPermissionsAndRoles,
} from 'src/types/bootstrapTypes';
import SavedQueryPreviewModal from 'src/features/queries/SavedQueryPreviewModal';
import { findPermission } from 'src/utils/findPermission';
const PAGE_SIZE = 25;
const PASSWORDS_NEEDED_MESSAGE = t(
@ -111,6 +116,10 @@ function SavedQueryList({
t('Saved queries'),
addDangerToast,
);
const { roles } = useSelector<any, UserWithPermissionsAndRoles>(
state => state.user,
);
const canReadTag = findPermission('can_read', 'Tag', roles);
const [queryCurrentlyDeleting, setQueryCurrentlyDeleting] =
useState<SavedQueryObject | null>(null);
const [savedQueryCurrentlyPreviewing, setSavedQueryCurrentlyPreviewing] =
@ -488,13 +497,7 @@ function SavedQueryList({
),
paginate: true,
},
{
Header: t('Tags'),
id: 'tags',
key: 'tags',
input: 'search',
operator: FilterOperator.savedQueryTags,
},
{
Header: t('Search'),
id: 'label',
@ -506,6 +509,16 @@ function SavedQueryList({
[addDangerToast],
);
if (isFeatureEnabled(FeatureFlag.TAGGING_SYSTEM) && canReadTag) {
filters.push({
Header: t('Tags'),
id: 'tags',
key: 'tags',
input: 'search',
operator: FilterOperator.savedQueryTags,
});
}
return (
<>
<SubMenu {...menuData} />

21
superset/tags/api.py
View File

@ -258,6 +258,8 @@ class TagRestApi(BaseSupersetModelRestApi):
except ValidationError as error:
return self.response_400(message=error.messages)
try:
all_tagged_objects: set[tuple[str, int]] = set()
all_skipped_tagged_objects: set[tuple[str, int]] = set()
for tag in item.get("tags"):
tagged_item: dict[str, Any] = self.add_model_schema.load(
{
@ -265,10 +267,25 @@ class TagRestApi(BaseSupersetModelRestApi):
"objects_to_tag": tag.get("objects_to_tag"),
}
)
CreateCustomTagWithRelationshipsCommand(
(
objects_tagged,
objects_skipped,
) = CreateCustomTagWithRelationshipsCommand(
tagged_item, bulk_create=True
).run()
return self.response(201)
all_tagged_objects = all_tagged_objects | objects_tagged
all_skipped_tagged_objects = (
all_skipped_tagged_objects | objects_skipped
)
return self.response(
200,
result={
"objects_tagged": list(
all_tagged_objects - all_skipped_tagged_objects
),
"objects_skipped": list(all_skipped_tagged_objects),
},
)
except TagNotFoundError:
return self.response_404()
except TagInvalidError as ex:

32
superset/tags/commands/create.py
View File

@ -69,8 +69,9 @@ class CreateCustomTagWithRelationshipsCommand(CreateMixin, BaseCommand):
def __init__(self, data: dict[str, Any], bulk_create: bool = False):
self._properties = data.copy()
self._bulk_create = bulk_create
self._skipped_tagged_objects: set[tuple[str, int]] = set()
def run(self) -> None:
def run(self) -> tuple[set[tuple[str, int]], set[tuple[str, int]]]:
self.validate()
try:
@ -86,6 +87,8 @@ class CreateCustomTagWithRelationshipsCommand(CreateMixin, BaseCommand):
db.session.commit()
return set(self._properties["objects_to_tag"]), self._skipped_tagged_objects
except DAOCreateFailedError as ex:
logger.exception(ex.exception)
raise TagCreateFailedError() from ex
@ -93,20 +96,27 @@ class CreateCustomTagWithRelationshipsCommand(CreateMixin, BaseCommand):
def validate(self) -> None:
exceptions = []
objects_to_tag = set(self._properties.get("objects_to_tag", []))
skipped_tagged_objects: set[tuple[str, int]] = set()
for obj_type, obj_id in objects_to_tag:
object_type = to_object_type(obj_type)
if not object_type:
exceptions.append(TagInvalidError(f"invalid object type {object_type}"))
try:
model = to_object_model(object_type, obj_id) # type: ignore
security_manager.raise_for_ownership(model)
except SupersetSecurityException:
# skip the object if the user doesn't have access
skipped_tagged_objects.add((obj_type, obj_id))
# Validate object type
for obj_type, obj_id in objects_to_tag:
object_type = to_object_type(obj_type)
self._properties["objects_to_tag"] = objects_to_tag - skipped_tagged_objects
if not object_type:
exceptions.append(
TagInvalidError(f"invalid object type {object_type}")
)
try:
if model := to_object_model(object_type, obj_id): # type: ignore
security_manager.raise_for_ownership(model)
except SupersetSecurityException:
# skip the object if the user doesn't have access
self._skipped_tagged_objects.add((obj_type, obj_id))
self._properties["objects_to_tag"] = (
set(objects_to_tag) - self._skipped_tagged_objects
)
if exceptions:
raise TagInvalidError(exceptions=exceptions)

6
superset/tags/commands/update.py
View File

@ -38,14 +38,12 @@ class UpdateTagCommand(UpdateMixin, BaseCommand):
def run(self) -> Model:
self.validate()
if self._model:
self._model.name = self._properties["name"]
TagDAO.create_tag_relationship(
objects_to_tag=self._properties.get("objects_to_tag", []),
tag=self._model,
)
if description := self._properties.get("description"):
self._model.description = description
if tag_name := self._properties.get("name"):
self._model.name = tag_name
self._model.description = self._properties.get("description")
db.session.add(self._model)
db.session.commit()

46
tests/integration_tests/tags/api_tests.py
View File

@ -550,7 +550,7 @@ class TestTagApi(SupersetTestCase):
},
)
self.assertEqual(rv.status_code, 201)
self.assertEqual(rv.status_code, 200)
result = TagDAO.get_tagged_objects_for_tags(tags, ["dashboard"])
assert len(result) == 1
@ -569,3 +569,47 @@ class TestTagApi(SupersetTestCase):
TaggedObject.object_type == ObjectTypes.chart,
)
assert tagged_objects.count() == 2
@pytest.mark.usefixtures("load_world_bank_dashboard_with_slices")
def test_post_bulk_tag_skipped_tags_perm(self):
alpha = self.get_user("alpha")
self.insert_dashboard("titletag", "slugtag", [alpha.id])
self.login(username="alpha")
uri = "api/v1/tag/bulk_create"
dashboard = (
db.session.query(Dashboard)
.filter(Dashboard.dashboard_title == "World Bank's Data")
.first()
)
alpha_dash = (
db.session.query(Dashboard)
.filter(Dashboard.dashboard_title == "titletag")
.first()
)
chart = db.session.query(Slice).first()
rv = self.client.post(
uri,
json={
"tags": [
{
"name": "tag1",
"objects_to_tag": [
["dashboard", alpha_dash.id],
],
},
{
"name": "tag2",
"objects_to_tag": [["dashboard", dashboard.id]],
},
{
"name": "tag3",
"objects_to_tag": [["chart", chart.id]],
},
]
},
)
self.assertEqual(rv.status_code, 200)
result = rv.json["result"]
assert len(result["objects_tagged"]) == 2
assert len(result["objects_skipped"]) == 1