From b9414b7a2177f576e2b145a3d29483ad2130f23f Mon Sep 17 00:00:00 2001 From: Carmine Di Monaco Date: Thu, 4 Jul 2024 09:43:51 +0200 Subject: [PATCH] Tags policy permissions (#2738) * Pill component disabled state * Extracted tag delete button into separate component with disabled state * Tags abilities migration * Tag policy * Tags controller protected with policy * DisabledGuard tooltip wrapping as props * Frontend tag component supports abilities * ClusterList with tag abilities * DatabaseOverview tags with abilities * HostsList with tags with abilities * SapSytemOverview tags with abilities * Tests refactor * Addressing review feedbacks --- .../js/common/DisabledGuard/DisabledGuard.jsx | 3 +- assets/js/common/Pill/Pill.jsx | 7 +- assets/js/common/Pill/Pill.stories.jsx | 9 ++ assets/js/common/Pill/Pill.test.jsx | 24 +++++ assets/js/common/Tags/Tags.jsx | 93 ++++++++++++------- assets/js/common/Tags/Tags.stories.jsx | 47 ++++++++-- assets/js/common/Tags/Tags.test.jsx | 31 ++++++- assets/js/lib/test-utils/index.jsx | 4 +- .../js/pages/ClusterDetails/ClustersList.jsx | 6 ++ .../ClusterDetails/ClustersList.test.jsx | 65 +++++++++---- .../DatabasesOverview/DatabasesOverview.jsx | 3 + .../DatabasesOverview.stories.jsx | 11 ++- .../DatabasesOverview.test.jsx | 42 ++++++++- .../DatabasesOverviewPage.jsx | 5 +- assets/js/pages/HostsList/HostsList.jsx | 3 + assets/js/pages/HostsList/HostsList.test.jsx | 30 ++++++ .../SapSystemsOverview.jsx | 3 + .../SapSystemsOverview.stories.jsx | 13 +-- .../SapSystemsOverview.test.jsx | 37 +++++++- .../SapSystemsOverviewPage.jsx | 2 +- lib/trento/tags/policy.ex | 28 ++++++ lib/trento/tags/tag.ex | 2 + .../controllers/v1/tags_controller.ex | 16 ++++ .../20240701074943_add_tags_abilities.exs | 15 +++ test/trento/tags/policy_test.exs | 48 ++++++++++ .../controllers/v1/tags_controller_test.exs | 61 ++++++++++++ 26 files changed, 527 insertions(+), 81 deletions(-) create mode 100644 lib/trento/tags/policy.ex create mode 100644 priv/repo/migrations/20240701074943_add_tags_abilities.exs create mode 100644 test/trento/tags/policy_test.exs diff --git a/assets/js/common/DisabledGuard/DisabledGuard.jsx b/assets/js/common/DisabledGuard/DisabledGuard.jsx index 925374ca49..bde1e11b40 100644 --- a/assets/js/common/DisabledGuard/DisabledGuard.jsx +++ b/assets/js/common/DisabledGuard/DisabledGuard.jsx @@ -15,6 +15,7 @@ function DisabledGuard({ userAbilities, permitted, withTooltip = true, + tooltipWrap = false, tooltipMessage = DEFAULT_TOOLTIP_MESSAGE, tooltipPlace = 'bottom', children, @@ -41,7 +42,7 @@ function DisabledGuard({ isEnabled={withTooltip} content={tooltipMessage} place={tooltipPlace} - wrap={false} + wrap={tooltipWrap} > {disabledElement} diff --git a/assets/js/common/Pill/Pill.jsx b/assets/js/common/Pill/Pill.jsx index c43c0973fa..9f05a01abb 100644 --- a/assets/js/common/Pill/Pill.jsx +++ b/assets/js/common/Pill/Pill.jsx @@ -13,6 +13,7 @@ function Pill({ size = 'sm', roundedMode = 'rounded-full', display = 'inline-flex', + disabled, }) { return ( diff --git a/assets/js/common/Pill/Pill.stories.jsx b/assets/js/common/Pill/Pill.stories.jsx index 62118f620a..7e1ce9f055 100644 --- a/assets/js/common/Pill/Pill.stories.jsx +++ b/assets/js/common/Pill/Pill.stories.jsx @@ -17,6 +17,15 @@ export function ExtraSmall() { return Extra small!; } +export function Disabled() { + return ( + + {' '} + Disabled + + ); +} + export function WithIcon() { return ( diff --git a/assets/js/common/Pill/Pill.test.jsx b/assets/js/common/Pill/Pill.test.jsx index f2fed238a5..a1034ee6c1 100644 --- a/assets/js/common/Pill/Pill.test.jsx +++ b/assets/js/common/Pill/Pill.test.jsx @@ -1,5 +1,6 @@ import React from 'react'; import { render, screen } from '@testing-library/react'; +import userEvent from '@testing-library/user-event'; import '@testing-library/jest-dom'; import Pill from '.'; @@ -12,6 +13,29 @@ describe('Pill', () => { ); }); + it('should display a pill with the disabled behaviour when disabled is passed as props', async () => { + const onClick = jest.fn(); + const user = userEvent.setup(); + render( + + Content + + ); + expect(screen.getByText('Content')).toHaveClass( + 'opacity-50 pointer-events-none' + ); + + await user.click(screen.getByText('Content')); + expect(onClick).not.toHaveBeenCalled(); + }); + it('should display a pill with provided props', () => { render( ); +function ExistingTag({ onClick, disabled }) { + return ( + + ); +} + function Tags({ className, tags, @@ -22,6 +40,9 @@ function Tags({ onAdd, onRemove, resourceId, + userAbilities, + tagAdditionPermittedFor = [], + tagDeletionPermittedFor = [], validationMessage = tagValidationDefaultMessage, }) { const [renderedTags, setTags] = useState(tags); @@ -75,21 +96,23 @@ function Tags({ }} > {tag} - + { + const newTagsList = renderedTags.reduce( + (acc, current) => (current === tag ? acc : [...acc, current]), + [] + ); + setTags(newTagsList); + onChange(newTagsList); + onRemove(tag); + }} + /> + ))} {addingTag ? ( @@ -123,25 +146,31 @@ function Tags({ ) : ( - { - e.stopPropagation(); - setAddingTag(true); - }} + - Add Tag - + { + e.stopPropagation(); + setAddingTag(true); + }} + > + Add Tag + + )} ); diff --git a/assets/js/common/Tags/Tags.stories.jsx b/assets/js/common/Tags/Tags.stories.jsx index 82328fb7d4..8c9fca3c4f 100644 --- a/assets/js/common/Tags/Tags.stories.jsx +++ b/assets/js/common/Tags/Tags.stories.jsx @@ -1,17 +1,46 @@ -import React from 'react'; - import Tags from '.'; export default { title: 'Components/Tags', component: Tags, - argTypes: { onChange: { action: 'tag changed' } }, + args: { + tags: ['carbonara', 'Amatriciana'], + userAbilities: [{ name: 'all', resource: 'all' }], + tagAdditionPermittedFor: ['all:all'], + tagDeletionPermittedFor: ['all:all'], + }, + argTypes: { + onChange: { action: 'tag changed' }, + tagAdditionPermittedFor: { + control: 'array', + description: 'Abilities that allow tag creation', + }, + tagDeletionPermittedFor: { + control: 'array', + description: 'Abilities that allow tag deletion', + }, + }, }; -export function Populated(args) { - return ; -} +export const Default = { + args: { + tags: ['carbonara', 'Amatriciana'], + userAbilities: [{ name: 'all', resource: 'all' }], + tagAdditionPermittedFor: ['all:all'], + tagDeletionPermittedFor: ['all:all'], + }, +}; -export function Empty(args) { - return ; -} +export const Empty = { + args: { + ...Default.args, + tags: [], + }, +}; + +export const Disabled = { + args: { + ...Default.args, + userAbilities: [], + }, +}; diff --git a/assets/js/common/Tags/Tags.test.jsx b/assets/js/common/Tags/Tags.test.jsx index 2d2a6aa1cf..a38d05cff2 100644 --- a/assets/js/common/Tags/Tags.test.jsx +++ b/assets/js/common/Tags/Tags.test.jsx @@ -7,11 +7,34 @@ import '@testing-library/jest-dom'; import Tags from '.'; describe('Tags', () => { + it('should disable creation and deletion when the proper user abilities are not present', async () => { + render( + + ); + + expect(screen.queryByText('Add Tag')).toHaveClass('opacity-50'); + // grab the X + expect( + screen.queryByText('thetag').children.item(0).children.item(0) + ).toHaveClass('opacity-50'); + }); + it('should show validation tooltip when inserting an unsupported character in a tag', async () => { const user = userEvent.setup(); const msg = 'invalid character'; - render(); + render( + + ); expect(screen.queryByText(msg)).toBeNull(); @@ -39,7 +62,11 @@ describe('Tags', () => { render( <>
sibling
- + ); diff --git a/assets/js/lib/test-utils/index.jsx b/assets/js/lib/test-utils/index.jsx index 155bf87925..8d0d24fbd2 100644 --- a/assets/js/lib/test-utils/index.jsx +++ b/assets/js/lib/test-utils/index.jsx @@ -16,6 +16,9 @@ const middlewares = []; const mockStore = configureStore(middlewares); export const defaultInitialState = { + user: { + abilities: [{ name: 'all', resource: 'all' }], + }, hostsList: { hosts }, clustersList: { clusters }, sapSystemsList: { @@ -35,7 +38,6 @@ export const defaultInitialState = { }, checksSelection: { host: {}, cluster: {} }, catalog: { loading: false, data: [], error: null }, - user: { abilities: [{ name: 'all', resource: 'all' }] }, }; export const withState = (component, initialState = {}) => { diff --git a/assets/js/pages/ClusterDetails/ClustersList.jsx b/assets/js/pages/ClusterDetails/ClustersList.jsx index 5d37d20da7..23d3fbea01 100644 --- a/assets/js/pages/ClusterDetails/ClustersList.jsx +++ b/assets/js/pages/ClusterDetails/ClustersList.jsx @@ -9,6 +9,8 @@ import { addTagToCluster, removeTagFromCluster } from '@state/clusters'; import { getAllSAPInstances } from '@state/selectors/sapSystem'; import { getInstanceID } from '@state/instances'; +import { getUserProfile } from '@state/selectors/user'; + import PageHeader from '@common/PageHeader'; import SapSystemLink from '@common/SapSystemLink'; import Table from '@common/Table'; @@ -38,6 +40,7 @@ function ClustersList() { const allInstances = useSelector(getAllSAPInstances); const dispatch = useDispatch(); const [searchParams, setSearchParams] = useSearchParams(); + const { abilities } = useSelector(getUserProfile); const config = { pagination: true, @@ -118,6 +121,9 @@ function ClustersList() { element[key].some((tag) => filter.includes(tag)), render: (content, item) => ( {}} diff --git a/assets/js/pages/ClusterDetails/ClustersList.test.jsx b/assets/js/pages/ClusterDetails/ClustersList.test.jsx index 7f95561684..d8aabd8d82 100644 --- a/assets/js/pages/ClusterDetails/ClustersList.test.jsx +++ b/assets/js/pages/ClusterDetails/ClustersList.test.jsx @@ -8,25 +8,56 @@ import { renderWithRouter, withState } from '@lib/test-utils'; import ClustersList from './ClustersList'; +const cleanInitialState = { + hostsList: { + hosts: [], + }, + clustersList: { + clusters: [], + }, + sapSystemsList: { + sapSystems: [], + applicationInstances: [], + databaseInstances: [], + }, + databasesList: { + databaseInstances: [], + }, + user: { + abilities: [{ name: 'all', resource: 'all' }], + }, +}; + describe('ClustersList component', () => { - describe('filtering', () => { - const cleanInitialState = { - hostsList: { - hosts: [], - }, - clustersList: { - clusters: [], - }, - sapSystemsList: { - sapSystems: [], - applicationInstances: [], - databaseInstances: [], - }, - databasesList: { - databaseInstances: [], - }, - }; + describe('tags operations', () => { + it('should disable tag creation and deletion if the user abilities are not compatible', async () => { + const state = { + ...cleanInitialState, + clustersList: { + clusters: clusterFactory.buildList(1, { + tags: [{ value: 'Tag2' }, { value: 'Tag1' }], + }), + }, + user: { + abilities: [{ name: 'all', resource: 'a_resource' }], + }, + }; + + const [StatefulClustersList] = withState(, state); + renderWithRouter(StatefulClustersList); + expect(screen.queryByText('Add Tag')).toHaveClass('opacity-50'); + // grab the X + expect( + screen.queryByText('Tag1').children.item(0).children.item(0) + ).toHaveClass('opacity-50'); + expect( + screen.queryByText('Tag2').children.item(0).children.item(0) + ).toHaveClass('opacity-50'); + }); + }); + + describe('filtering', () => { const scenarios = [ { filter: 'Health', diff --git a/assets/js/pages/DatabasesOverview/DatabasesOverview.jsx b/assets/js/pages/DatabasesOverview/DatabasesOverview.jsx index efc810e01b..b0153577f3 100644 --- a/assets/js/pages/DatabasesOverview/DatabasesOverview.jsx +++ b/assets/js/pages/DatabasesOverview/DatabasesOverview.jsx @@ -105,6 +105,9 @@ function DatabasesOverview({ element[key].some((tag) => filters.includes(tag)), render: (content, item) => ( {}} diff --git a/assets/js/pages/DatabasesOverview/DatabasesOverview.stories.jsx b/assets/js/pages/DatabasesOverview/DatabasesOverview.stories.jsx index d321f38ebb..8aeeee66bd 100644 --- a/assets/js/pages/DatabasesOverview/DatabasesOverview.stories.jsx +++ b/assets/js/pages/DatabasesOverview/DatabasesOverview.stories.jsx @@ -11,6 +11,7 @@ import { import DatabasesOverview from './DatabasesOverview'; +const userAbilities = [{ name: 'all', resource: 'all' }]; const databases = databaseFactory.buildList(3); const enrichedInstances = databases[0].database_instances @@ -79,8 +80,8 @@ export default { }, }, userAbilities: { - control: 'array', - description: 'Current user abilities', + control: { type: 'array' }, + description: 'User profile abilities', }, onTagAdd: { action: 'Add tag', @@ -111,28 +112,28 @@ export default { export const Databases = { args: { + userAbilities, databases, databaseInstances: enrichedInstances, loading: false, - userAbilities: [{ name: 'all', resource: 'all' }], }, }; export const WithSystemReplication = { args: { + userAbilities, databases: [databaseWithSR], databaseInstances: systemReplicationInstances, loading: false, - userAbilities: [{ name: 'all', resource: 'all' }], }, }; export const WithAbsentInstances = { args: { + userAbilities, databases: [databaseWithAbsentInstances], databaseInstances: absentInstance, loading: false, - userAbilities: [{ name: 'all', resource: 'all' }], }, }; diff --git a/assets/js/pages/DatabasesOverview/DatabasesOverview.test.jsx b/assets/js/pages/DatabasesOverview/DatabasesOverview.test.jsx index 529105e815..54d11175c3 100644 --- a/assets/js/pages/DatabasesOverview/DatabasesOverview.test.jsx +++ b/assets/js/pages/DatabasesOverview/DatabasesOverview.test.jsx @@ -12,10 +12,36 @@ import { filterTable, clearFilter } from '@common/Table/Table.test'; import DatabasesOverview from './DatabasesOverview'; describe('DatabasesOverview component', () => { + describe('tag operations', () => { + it('should disable tag creation and delete when user abilities are not compatible', () => { + const database = databaseFactory.build({ + tags: [{ value: 'Tag1' }, { value: 'Tag2' }], + }); + const userAbilities = [{ name: 'all', resource: 'another_resource' }]; + + renderWithRouter( + + ); + + expect(screen.queryByText('Add Tag')).toHaveClass('opacity-50'); + // grab the X + expect( + screen.queryByText('Tag1').children.item(0).children.item(0) + ).toHaveClass('opacity-50'); + expect( + screen.queryByText('Tag2').children.item(0).children.item(0) + ).toHaveClass('opacity-50'); + }); + }); describe('instance cleanup', () => { it('should clean up database instance on request', async () => { const user = userEvent.setup(); const mockedCleanUp = jest.fn(); + const userAbilities = [{ name: 'all', resource: 'all' }]; const database = databaseFactory.build(); @@ -26,8 +52,8 @@ describe('DatabasesOverview component', () => { renderWithRouter( ); @@ -81,6 +107,8 @@ describe('DatabasesOverview component', () => { }); describe('filtering', () => { + const userAbilities = [{ name: 'all', resource: 'all' }]; + const scenarios = [ { filter: 'Health', @@ -119,7 +147,11 @@ describe('DatabasesOverview component', () => { 'should filter the table content by $filter filter', ({ filter, options, databases, expectedRows }) => { renderWithRouter( - + ); options.forEach(async (option) => { @@ -144,7 +176,11 @@ describe('DatabasesOverview component', () => { const { health, sid, tags } = databases[0]; renderWithRouter( - + ); [ diff --git a/assets/js/pages/DatabasesOverview/DatabasesOverviewPage.jsx b/assets/js/pages/DatabasesOverview/DatabasesOverviewPage.jsx index 7332b58850..b7f9f864c9 100644 --- a/assets/js/pages/DatabasesOverview/DatabasesOverviewPage.jsx +++ b/assets/js/pages/DatabasesOverview/DatabasesOverviewPage.jsx @@ -4,6 +4,7 @@ import { useSelector, useDispatch } from 'react-redux'; import { getEnrichedDatabaseInstances } from '@state/selectors/sapSystem'; import { getUserProfile } from '@state/selectors/user'; + import { addTagToDatabase, removeTagFromDatabase, @@ -28,15 +29,15 @@ function DatabasesOverviewPage() { const enrichedDatabaseInstances = useSelector((state) => getEnrichedDatabaseInstances(state) ); - const { abilities } = useSelector(getUserProfile); const dispatch = useDispatch(); + const { abilities } = useSelector(getUserProfile); return ( { addTag(tag, databaseID); dispatch(addTagToDatabase({ tags: [{ value: tag }], id: databaseID })); diff --git a/assets/js/pages/HostsList/HostsList.jsx b/assets/js/pages/HostsList/HostsList.jsx index c860cd8e1c..e4999801e8 100644 --- a/assets/js/pages/HostsList/HostsList.jsx +++ b/assets/js/pages/HostsList/HostsList.jsx @@ -177,6 +177,9 @@ function HostsList() { element[key].some((tag) => filter.includes(tag)), render: (content, item) => ( {}} diff --git a/assets/js/pages/HostsList/HostsList.test.jsx b/assets/js/pages/HostsList/HostsList.test.jsx index c6a438542e..6784e062f1 100644 --- a/assets/js/pages/HostsList/HostsList.test.jsx +++ b/assets/js/pages/HostsList/HostsList.test.jsx @@ -432,4 +432,34 @@ describe('HostsLists component', () => { ); }); }); + describe('tag operations', () => { + it('should disable tag creation and deletion if the user abilities are not compatible', async () => { + const host1 = hostFactory.build({ + agent_version: '1.0.0', + tags: [{ value: 'Tag1' }, { value: 'Tag2' }], + }); + const state = { + ...defaultInitialState, + hostsList: { + hosts: [host1], + }, + user: { + abilities: [{ name: 'all', resource: 'a_resource' }], + }, + }; + + const [StatefulHostsList] = withState(, state); + + renderWithRouter(StatefulHostsList); + + expect(screen.queryByText('Add Tag')).toHaveClass('opacity-50'); + // grab the X + expect( + screen.queryByText('Tag1').children.item(0).children.item(0) + ).toHaveClass('opacity-50'); + expect( + screen.queryByText('Tag2').children.item(0).children.item(0) + ).toHaveClass('opacity-50'); + }); + }); }); diff --git a/assets/js/pages/SapSystemsOverviewPage/SapSystemsOverview.jsx b/assets/js/pages/SapSystemsOverviewPage/SapSystemsOverview.jsx index cecd2f17ff..d00c0d00f7 100644 --- a/assets/js/pages/SapSystemsOverviewPage/SapSystemsOverview.jsx +++ b/assets/js/pages/SapSystemsOverviewPage/SapSystemsOverview.jsx @@ -95,6 +95,9 @@ function SapSystemsOverview({ element[key].some((tag) => filters.includes(tag)), render: (content, item) => ( {}} diff --git a/assets/js/pages/SapSystemsOverviewPage/SapSystemsOverview.stories.jsx b/assets/js/pages/SapSystemsOverviewPage/SapSystemsOverview.stories.jsx index afe21cc45b..4b5431d3a1 100644 --- a/assets/js/pages/SapSystemsOverviewPage/SapSystemsOverview.stories.jsx +++ b/assets/js/pages/SapSystemsOverviewPage/SapSystemsOverview.stories.jsx @@ -10,6 +10,7 @@ import { import SapSystemsOverview from './SapSystemsOverview'; +const userAbilities = [{ name: 'all', resource: 'all' }]; const enrichInstances = (systems, instanceType) => systems .map((system) => system[instanceType]) @@ -70,6 +71,10 @@ export default { control: { type: 'array' }, description: 'Application instances', }, + userAbilities: { + control: { type: 'array' }, + description: 'User profile abilities', + }, databaseInstances: { control: { type: 'array' }, description: 'Database instances', @@ -82,10 +87,6 @@ export default { defaultValue: { summary: false }, }, }, - userAbilities: { - control: 'array', - description: 'Current user abilities', - }, onTagAdd: { action: 'Add tag', description: 'Called when a new tag is added', @@ -115,21 +116,21 @@ export default { export const SapSystems = { args: { + userAbilities, sapSystems, applicationInstances: enrichedApplicationInstances, databaseInstances: enrichedDatabaseInstances, loading: false, - userAbilities: [{ name: 'all', resource: 'all' }], }, }; export const WithAbsentInstances = { args: { + userAbilities, sapSystems: sapSystemsWithAbsentInstances, applicationInstances: enrichedAbsentApplicationInstances, databaseInstances: enrichedAbsentDatabaseInstances, loading: false, - userAbilities: [{ name: 'all', resource: 'all' }], }, }; diff --git a/assets/js/pages/SapSystemsOverviewPage/SapSystemsOverview.test.jsx b/assets/js/pages/SapSystemsOverviewPage/SapSystemsOverview.test.jsx index e6464ba4e6..edd759c2ec 100644 --- a/assets/js/pages/SapSystemsOverviewPage/SapSystemsOverview.test.jsx +++ b/assets/js/pages/SapSystemsOverviewPage/SapSystemsOverview.test.jsx @@ -16,6 +16,8 @@ import { filterTable, clearFilter } from '@common/Table/Table.test'; import SapSystemsOverview from './SapSystemsOverview'; +const userAbilities = [{ name: 'all', resource: 'all' }]; + describe('SapSystemsOverviews component', () => { describe('overview content', () => { it('should display the correct number of SAP systems', () => { @@ -26,6 +28,7 @@ describe('SapSystemsOverviews component', () => { renderWithRouter( { renderWithRouter( @@ -122,6 +126,7 @@ describe('SapSystemsOverviews component', () => { }); renderWithRouter( { renderWithRouter( ); @@ -325,6 +330,7 @@ describe('SapSystemsOverviews component', () => { ({ filter, options, sapSystems, expectedRows }) => { renderWithRouter( { renderWithRouter( { ); }); }); + + describe('tag operations', () => { + it('should disable tag creation and deletion if the user abilities are not compatible', () => { + const abilities = [{ name: 'all', resource: 'another_resource' }]; + + const sapSystem = sapSystemFactory.build({ + tags: [{ value: 'Tag1' }, { value: 'Tag2' }], + }); + + renderWithRouter( + + ); + + expect(screen.queryByText('Add Tag')).toHaveClass('opacity-50'); + // grab the X + expect( + screen.queryByText('Tag1').children.item(0).children.item(0) + ).toHaveClass('opacity-50'); + expect( + screen.queryByText('Tag2').children.item(0).children.item(0) + ).toHaveClass('opacity-50'); + }); + }); }); diff --git a/assets/js/pages/SapSystemsOverviewPage/SapSystemsOverviewPage.jsx b/assets/js/pages/SapSystemsOverviewPage/SapSystemsOverviewPage.jsx index 1953753179..bba0bb686d 100644 --- a/assets/js/pages/SapSystemsOverviewPage/SapSystemsOverviewPage.jsx +++ b/assets/js/pages/SapSystemsOverviewPage/SapSystemsOverviewPage.jsx @@ -41,11 +41,11 @@ function SapSystemOverviewPage() { return ( { addTag(tag, sapSystemID); dispatch( diff --git a/lib/trento/tags/policy.ex b/lib/trento/tags/policy.ex new file mode 100644 index 0000000000..2bfa47a9be --- /dev/null +++ b/lib/trento/tags/policy.ex @@ -0,0 +1,28 @@ +defmodule Trento.Tags.Policy do + @moduledoc """ + Policy for the Tag resource + + User with the ability all:all can perform any operation on the tags. + User with the ability all:_tags can perform any operations on the tags of the permitted resource. + + Resource type can be one of: + - host + - cluster + - sap_system + - database + """ + @behaviour Bodyguard.Policy + + import Trento.Support.PolicyHelper + alias Trento.Tags.Tag + alias Trento.Users.User + + def authorize(action, %User{} = user, %{tag_resource: tag_resource, resource: Tag}) + when action in [:add_tag, :remove_tag], + do: has_global_ability?(user) or has_all_ability_on_tag_resorce?(user, tag_resource) + + def authorize(_, _, _), do: false + + def has_all_ability_on_tag_resorce?(%User{} = user, tag_resource), + do: user_has_ability?(user, %{name: "all", resource: "#{tag_resource}_tags"}) +end diff --git a/lib/trento/tags/tag.ex b/lib/trento/tags/tag.ex index 9d182db520..8f5eb3c304 100644 --- a/lib/trento/tags/tag.ex +++ b/lib/trento/tags/tag.ex @@ -5,6 +5,8 @@ defmodule Trento.Tags.Tag do import Ecto.Changeset + defdelegate authorize(action, user, params), to: Trento.Tags.Policy + @forbidden_tag_chars_regex ~r/^[\+\-=.,_:@\p{L}\w]*$/u @type t :: %__MODULE__{} diff --git a/lib/trento_web/controllers/v1/tags_controller.ex b/lib/trento_web/controllers/v1/tags_controller.ex index 2973bad380..2374f2938b 100644 --- a/lib/trento_web/controllers/v1/tags_controller.ex +++ b/lib/trento_web/controllers/v1/tags_controller.ex @@ -7,6 +7,15 @@ defmodule TrentoWeb.V1.TagsController do alias TrentoWeb.OpenApi.V1.Schema + plug TrentoWeb.Plugs.LoadUserPlug + + plug Bodyguard.Plug.Authorize, + policy: Trento.Tags.Policy, + action: {Phoenix.Controller, :action_name}, + user: {Pow.Plug, :current_user}, + params: {__MODULE__, :get_policy_resource}, + fallback: TrentoWeb.FallbackController + action_fallback TrentoWeb.FallbackController plug OpenApiSpex.Plug.CastAndValidate, json_render_error_v2: true @@ -83,4 +92,11 @@ defmodule TrentoWeb.V1.TagsController do send_resp(conn, :no_content, "") end end + + def get_policy_resource(%{ + assigns: %{ + resource_type: resource_type + } + }), + do: %{tag_resource: resource_type, resource: Tag} end diff --git a/priv/repo/migrations/20240701074943_add_tags_abilities.exs b/priv/repo/migrations/20240701074943_add_tags_abilities.exs new file mode 100644 index 0000000000..81e9ff99af --- /dev/null +++ b/priv/repo/migrations/20240701074943_add_tags_abilities.exs @@ -0,0 +1,15 @@ +defmodule Trento.Repo.Migrations.AddTagsAbilities do + use Ecto.Migration + + def up do + for tag_resource <- [:host, :cluster, :database, :sap_system] do + execute "INSERT INTO abilities(name, resource, label, inserted_at, updated_at) VALUES ('all', '#{tag_resource}_tags', 'Permits all operation on #{tag_resource} tags', NOW(), NOW())" + end + end + + def down do + for tag_resource <- [:host, :cluster, :database, :sap_system] do + execute "DELETE FROM abilities WHERE name = 'all' AND resource = '#{tag_resource}_tags'" + end + end +end diff --git a/test/trento/tags/policy_test.exs b/test/trento/tags/policy_test.exs new file mode 100644 index 0000000000..1d0fe31d8f --- /dev/null +++ b/test/trento/tags/policy_test.exs @@ -0,0 +1,48 @@ +defmodule Trento.Tags.PolicyTest do + use ExUnit.Case + + alias Trento.Tags.Policy + alias Trento.Tags.Tag + alias Trento.Users.User + + test "should allow any action when the user has global ability" do + user = %User{abilities: [%{resource: "all", name: "all"}]} + + Enum.each([:cluster, :host, :sap_system, :database], fn tag_resource -> + assert Policy.authorize(:add_tag, user, %{tag_resource: tag_resource, resource: Tag}) + assert Policy.authorize(:remove_tag, user, %{tag_resource: tag_resource, resource: Tag}) + end) + end + + test "should not allow add_tag action on the resource when the user does not have the right ability" do + user = %User{abilities: []} + + Enum.each([:cluster, :host, :sap_system, :database], fn tag_resource -> + refute Policy.authorize(:add_tag, user, %{tag_resource: tag_resource, resource: Tag}) + end) + end + + test "should not allow remove_tag action on the resource when the user does not have the right ability" do + user = %User{abilities: []} + + Enum.each([:cluster, :host, :sap_system, :database], fn tag_resource -> + refute Policy.authorize(:remove_tag, user, %{tag_resource: tag_resource, resource: Tag}) + end) + end + + test "should allow add_tag action on the resource when the user have the right ability" do + Enum.each([:cluster, :host, :sap_system, :database], fn tag_resource -> + user = %User{abilities: [%{resource: "#{tag_resource}_tags", name: "all"}]} + + assert Policy.authorize(:add_tag, user, %{tag_resource: tag_resource, resource: Tag}) + end) + end + + test "should allow remove_tag action on the resource when the user have the right ability" do + Enum.each([:cluster, :host, :sap_system, :database], fn tag_resource -> + user = %User{abilities: [%{resource: "#{tag_resource}_tags", name: "all"}]} + + assert Policy.authorize(:remove_tag, user, %{tag_resource: tag_resource, resource: Tag}) + end) + end +end diff --git a/test/trento_web/controllers/v1/tags_controller_test.exs b/test/trento_web/controllers/v1/tags_controller_test.exs index 1f0a42a48a..1552de0d68 100644 --- a/test/trento_web/controllers/v1/tags_controller_test.exs +++ b/test/trento_web/controllers/v1/tags_controller_test.exs @@ -1,10 +1,15 @@ defmodule TrentoWeb.V1.TagsControllerTest do use TrentoWeb.ConnCase, async: true + import OpenApiSpex.TestAssertions import Trento.Factory + import Trento.Support.Helpers.AbilitiesTestHelper alias Faker.Color alias Trento.Tags.Tag + setup :setup_api_spec_v1 + setup :setup_user + describe "Tag Validation" do test "should decline tag with whitespace", %{conn: conn} do conn = @@ -183,4 +188,60 @@ defmodule TrentoWeb.V1.TagsControllerTest do assert 404 == conn.status end end + + describe "forbidden actions" do + test "should not return forbidden on any controller action if the user have the right ability for the tag resource", + %{conn: conn} do + %{id: user_id} = insert(:user) + + for tag_resource <- [:host, :sap_system, :cluster, :database] do + %{id: ability_id} = insert(:ability, name: "all", resource: "#{tag_resource}_tags") + insert(:users_abilities, user_id: user_id, ability_id: ability_id) + %{id: resource_id} = insert(tag_resource) + + conn = + conn + |> Pow.Plug.assign_current_user(%{"user_id" => user_id}, Pow.Plug.fetch_config(conn)) + |> put_req_header("content-type", "application/json") + + resp = + post(conn, "/api/v1/#{tag_resource}s/#{resource_id}/tags", %{ + "value" => "thetag" + }) + + assert resp.status == 201 + + resp = + delete(conn, "/api/v1/#{tag_resource}s/#{resource_id}/tags/thetag") + + assert resp.status == 204 + end + end + + test "should return forbidden on any controller action if the user does not have the right permission", + %{conn: conn, api_spec: api_spec} do + %{id: user_id} = insert(:user) + + conn = + conn + |> Pow.Plug.assign_current_user(%{"user_id" => user_id}, Pow.Plug.fetch_config(conn)) + |> put_req_header("content-type", "application/json") + + for tag_resource <- [:host, :sap_system, :cluster, :database] do + Enum.each( + [ + post(conn, "/api/v1/#{tag_resource}s/#{Faker.UUID.v4()}/tags", %{ + "value" => "thetag" + }), + delete(conn, "/api/v1/#{tag_resource}s/#{Faker.UUID.v4()}/tags/thetag") + ], + fn conn -> + conn + |> json_response(:forbidden) + |> assert_schema("Forbidden", api_spec) + end + ) + end + end + end end