diff --git a/superset-frontend/src/features/tags/BulkTagModal.tsx b/superset-frontend/src/features/tags/BulkTagModal.tsx index 9854cf3d2a118..643dcdb4322c4 100644 --- a/superset-frontend/src/features/tags/BulkTagModal.tsx +++ b/superset-frontend/src/features/tags/BulkTagModal.tsx @@ -67,7 +67,18 @@ const BulkTagModal: React.FC = ({ }, }) .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')); diff --git a/superset-frontend/src/pages/AllEntities/index.tsx b/superset-frontend/src/pages/AllEntities/index.tsx index e131e7e6fb865..ca815795d6fbb 100644 --- a/superset-frontend/src/pages/AllEntities/index.tsx +++ b/superset-frontend/src/pages/AllEntities/index.tsx @@ -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); + }} /> { 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( - + - + , ); @@ -231,9 +257,9 @@ describe('ChartList - anonymous view', () => { fetchMock.resetHistory(); wrapper = mount( - + - + , ); diff --git a/superset-frontend/src/pages/ChartList/index.tsx b/superset-frontend/src/pages/ChartList/index.tsx index 14ff62695fe36..d13113158e778 100644 --- a/superset-frontend/src/pages/ChartList/index.tsx +++ b/superset-frontend/src/pages/ChartList/index.tsx @@ -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', t('chart'), addDangerToast); const chartIds = useMemo(() => charts.map(c => c.id), [charts]); + const { roles } = useSelector( + 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', diff --git a/superset-frontend/src/pages/DashboardList/DashboardList.test.jsx b/superset-frontend/src/pages/DashboardList/DashboardList.test.jsx index 811f51afba9c8..fe307ee791448 100644 --- a/superset-frontend/src/pages/DashboardList/DashboardList.test.jsx +++ b/superset-frontend/src/pages/DashboardList/DashboardList.test.jsx @@ -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( - + - + , ); @@ -249,9 +272,9 @@ describe('DashboardList - anonymous view', () => { fetchMock.resetHistory(); wrapper = mount( - + - + , ); diff --git a/superset-frontend/src/pages/DashboardList/index.tsx b/superset-frontend/src/pages/DashboardList/index.tsx index 97898b3720c42..6542d85129722 100644 --- a/superset-frontend/src/pages/DashboardList/index.tsx +++ b/superset-frontend/src/pages/DashboardList/index.tsx @@ -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( + 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', diff --git a/superset-frontend/src/pages/SavedQueryList/SavedQueryList.test.jsx b/superset-frontend/src/pages/SavedQueryList/SavedQueryList.test.jsx index 3804010e26f1e..56fe3808d9074 100644 --- a/superset-frontend/src/pages/SavedQueryList/SavedQueryList.test.jsx +++ b/superset-frontend/src/pages/SavedQueryList/SavedQueryList.test.jsx @@ -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( - + - , + , ); + beforeEach(() => { + // setup a DOM element as a render target + useSelectorMock.mockClear(); + }); + beforeAll(async () => { await waitForComponentToPaint(wrapper); }); diff --git a/superset-frontend/src/pages/SavedQueryList/index.tsx b/superset-frontend/src/pages/SavedQueryList/index.tsx index f9af490bb44b3..3ee62c2ce6533 100644 --- a/superset-frontend/src/pages/SavedQueryList/index.tsx +++ b/superset-frontend/src/pages/SavedQueryList/index.tsx @@ -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( + state => state.user, + ); + const canReadTag = findPermission('can_read', 'Tag', roles); const [queryCurrentlyDeleting, setQueryCurrentlyDeleting] = useState(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 ( <> diff --git a/superset/tags/api.py b/superset/tags/api.py index 7224d1423a241..e9842f5a6a658 100644 --- a/superset/tags/api.py +++ b/superset/tags/api.py @@ -258,6 +258,8 @@ def bulk_create(self) -> Response: 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 @@ def bulk_create(self) -> Response: "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: diff --git a/superset/tags/commands/create.py b/superset/tags/commands/create.py index 883c498bc3b17..cd3bcc176b2b6 100644 --- a/superset/tags/commands/create.py +++ b/superset/tags/commands/create.py @@ -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 @@ def run(self) -> None: 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 @@ def run(self) -> None: 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)) - - self._properties["objects_to_tag"] = objects_to_tag - skipped_tagged_objects + # Validate object type + 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: + 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) diff --git a/superset/tags/commands/update.py b/superset/tags/commands/update.py index cc1c9a2be788d..182376438b6c5 100644 --- a/superset/tags/commands/update.py +++ b/superset/tags/commands/update.py @@ -38,14 +38,12 @@ def __init__(self, model_id: int, data: dict[str, Any]): 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() diff --git a/tests/integration_tests/tags/api_tests.py b/tests/integration_tests/tags/api_tests.py index 444d52078e7ca..e7f35c6b5d55b 100644 --- a/tests/integration_tests/tags/api_tests.py +++ b/tests/integration_tests/tags/api_tests.py @@ -550,7 +550,7 @@ def test_post_bulk_tag(self): }, ) - 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 @@ def test_post_bulk_tag(self): 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