From ce8b9750c9dee2f6b342a76d416d0df1169bff4f Mon Sep 17 00:00:00 2001 From: nixocio Date: Fri, 25 Feb 2022 12:30:22 -0500 Subject: [PATCH] Add several changes to Instance Groups Add several changes to API and UI related to Instance Groups. * Update summary_fields for DEFAULT_CONTROL_PLANE_QUEUE_NAME, and DEFAULT_EXECUTION_QUEUE_NAME. Rely on API validation for those fields. * Fix Instance Group list RBAC * Add validation for a couple of fields on the Instance Groups endpoint 1. is_container_group 2. policy_instance_percentage 3. policy_instance_list See: https://github.com/ansible/awx/issues/11130 Also: https://github.com/ansible/awx/issues/11718 --- awx/api/permissions.py | 9 -- awx/api/serializers.py | 15 ++++ awx/api/views/__init__.py | 2 - awx/main/access.py | 7 +- .../screens/InstanceGroup/ContainerGroup.js | 32 ++----- .../ContainerGroupAdd/ContainerGroupAdd.js | 4 +- .../ContainerGroupDetails.js | 5 +- .../ContainerGroupEdit/ContainerGroupEdit.js | 8 +- .../screens/InstanceGroup/InstanceGroup.js | 33 ++----- .../InstanceGroupAdd/InstanceGroupAdd.js | 4 +- .../InstanceGroupDetails.js | 11 +-- .../InstanceGroupEdit/InstanceGroupEdit.js | 8 +- .../InstanceGroupEdit.test.js | 26 +----- .../InstanceGroupList/InstanceGroupList.js | 86 ++----------------- .../screens/InstanceGroup/InstanceGroups.js | 30 +++---- .../InstanceGroup/InstanceGroups.test.js | 14 +++ .../shared/ContainerGroupForm.js | 27 +----- .../InstanceGroup/shared/InstanceGroupForm.js | 32 +------ .../shared/InstanceGroupForm.test.js | 40 --------- awx/ui/src/setupTests.js | 1 + 20 files changed, 80 insertions(+), 314 deletions(-) diff --git a/awx/api/permissions.py b/awx/api/permissions.py index bd6328495b..3608a23d33 100644 --- a/awx/api/permissions.py +++ b/awx/api/permissions.py @@ -4,8 +4,6 @@ # Python import logging -from django.conf import settings - # Django REST Framework from rest_framework.exceptions import MethodNotAllowed, PermissionDenied from rest_framework import permissions @@ -250,13 +248,6 @@ class IsSystemAdminOrAuditor(permissions.BasePermission): return request.user.is_superuser -class InstanceGroupTowerPermission(ModelAccessPermission): - def has_object_permission(self, request, view, obj): - if request.method == 'DELETE' and obj.name in [settings.DEFAULT_EXECUTION_QUEUE_NAME, settings.DEFAULT_CONTROL_PLANE_QUEUE_NAME]: - return False - return super(InstanceGroupTowerPermission, self).has_object_permission(request, view, obj) - - class WebhookKeyPermission(permissions.BasePermission): def has_object_permission(self, request, view, obj): return request.user.can_access(view.model, 'admin', obj, request.data) diff --git a/awx/api/serializers.py b/awx/api/serializers.py index 695f83d8d9..a4ee2ef7f0 100644 --- a/awx/api/serializers.py +++ b/awx/api/serializers.py @@ -4947,6 +4947,9 @@ class InstanceGroupSerializer(BaseSerializer): return res def validate_policy_instance_list(self, value): + if self.instance and self.instance.name in [settings.DEFAULT_EXECUTION_QUEUE_NAME, settings.DEFAULT_CONTROL_PLANE_QUEUE_NAME]: + if self.instance.policy_instance_list != value: + raise serializers.ValidationError(_('%s instance group policy_instance_list may not be changed.' % self.instance.name)) for instance_name in value: if value.count(instance_name) > 1: raise serializers.ValidationError(_('Duplicate entry {}.').format(instance_name)) @@ -4957,6 +4960,11 @@ class InstanceGroupSerializer(BaseSerializer): return value def validate_policy_instance_percentage(self, value): + if self.instance and self.instance.name in [settings.DEFAULT_EXECUTION_QUEUE_NAME, settings.DEFAULT_CONTROL_PLANE_QUEUE_NAME]: + if value != self.instance.policy_instance_percentage: + raise serializers.ValidationError( + _('%s instance group policy_instance_percentage may not be changed from the initial value set by the installer.' % self.instance.name) + ) if value and self.instance and self.instance.is_container_group: raise serializers.ValidationError(_('Containerized instances may not be managed via the API')) return value @@ -4975,6 +4983,13 @@ class InstanceGroupSerializer(BaseSerializer): return value + def validate_is_container_group(self, value): + if self.instance and self.instance.name in [settings.DEFAULT_EXECUTION_QUEUE_NAME, settings.DEFAULT_CONTROL_PLANE_QUEUE_NAME]: + if value != self.instance.is_container_group: + raise serializers.ValidationError(_('%s instance group is_container_group may not be changed.' % self.instance.name)) + + return value + def validate_credential(self, value): if value and not value.kubernetes: raise serializers.ValidationError(_('Only Kubernetes credentials can be associated with an Instance Group')) diff --git a/awx/api/views/__init__.py b/awx/api/views/__init__.py index 163fa4e727..34e442eb5e 100644 --- a/awx/api/views/__init__.py +++ b/awx/api/views/__init__.py @@ -105,7 +105,6 @@ from awx.api.permissions import ( ProjectUpdatePermission, InventoryInventorySourcesUpdatePermission, UserPermission, - InstanceGroupTowerPermission, VariableDataPermission, WorkflowApprovalPermission, IsSystemAdminOrAuditor, @@ -480,7 +479,6 @@ class InstanceGroupDetail(RelatedJobsPreventDeleteMixin, RetrieveUpdateDestroyAP name = _("Instance Group Detail") model = models.InstanceGroup serializer_class = serializers.InstanceGroupSerializer - permission_classes = (InstanceGroupTowerPermission,) def update_raw_data(self, data): if self.get_object().is_container_group: diff --git a/awx/main/access.py b/awx/main/access.py index 06b560b9ae..c608a7aa41 100644 --- a/awx/main/access.py +++ b/awx/main/access.py @@ -465,7 +465,7 @@ class BaseAccess(object): if display_method == 'schedule': user_capabilities['schedule'] = user_capabilities['start'] continue - elif display_method == 'delete' and not isinstance(obj, (User, UnifiedJob, CredentialInputSource, ExecutionEnvironment)): + elif display_method == 'delete' and not isinstance(obj, (User, UnifiedJob, CredentialInputSource, ExecutionEnvironment, InstanceGroup)): user_capabilities['delete'] = user_capabilities['edit'] continue elif display_method == 'copy' and isinstance(obj, (Group, Host)): @@ -575,6 +575,11 @@ class InstanceGroupAccess(BaseAccess): def can_change(self, obj, data): return self.user.is_superuser + def can_delete(self, obj): + if obj.name in [settings.DEFAULT_EXECUTION_QUEUE_NAME, settings.DEFAULT_CONTROL_PLANE_QUEUE_NAME]: + return False + return self.user.is_superuser + class UserAccess(BaseAccess): """ diff --git a/awx/ui/src/screens/InstanceGroup/ContainerGroup.js b/awx/ui/src/screens/InstanceGroup/ContainerGroup.js index 1e1be0b88d..3533b1c97a 100644 --- a/awx/ui/src/screens/InstanceGroup/ContainerGroup.js +++ b/awx/ui/src/screens/InstanceGroup/ContainerGroup.js @@ -13,7 +13,7 @@ import { CaretLeftIcon } from '@patternfly/react-icons'; import { Card, PageSection } from '@patternfly/react-core'; import useRequest from 'hooks/useRequest'; -import { InstanceGroupsAPI, SettingsAPI } from 'api'; +import { InstanceGroupsAPI } from 'api'; import RoutedTabs from 'components/RoutedTabs'; import ContentError from 'components/ContentError'; import ContentLoading from 'components/ContentLoading'; @@ -30,28 +30,15 @@ function ContainerGroup({ setBreadcrumb }) { isLoading, error: contentError, request: fetchInstanceGroups, - result: { instanceGroup, defaultControlPlane, defaultExecution }, + result: { instanceGroup }, } = useRequest( useCallback(async () => { - const [ - { data }, - { - data: { - DEFAULT_EXECUTION_QUEUE_NAME, - DEFAULT_CONTROL_PLANE_QUEUE_NAME, - }, - }, - ] = await Promise.all([ - InstanceGroupsAPI.readDetail(id), - SettingsAPI.readAll(), - ]); + const { data } = await InstanceGroupsAPI.readDetail(id); return { instanceGroup: data, - defaultControlPlane: DEFAULT_CONTROL_PLANE_QUEUE_NAME, - defaultExecution: DEFAULT_EXECUTION_QUEUE_NAME, }; }, [id]), - { instanceGroup: null, defaultExecution: '' } + { instanceGroup: null } ); useEffect(() => { @@ -125,17 +112,10 @@ function ContainerGroup({ setBreadcrumb }) { {instanceGroup && ( <> - + - + )} - {name !== defaultExecution && - instanceGroup.summary_fields.user_capabilities && + {instanceGroup.summary_fields.user_capabilities && instanceGroup.summary_fields.user_capabilities.delete && ( { - const [ - { data }, - { - data: { - DEFAULT_CONTROL_PLANE_QUEUE_NAME, - DEFAULT_EXECUTION_QUEUE_NAME, - }, - }, - ] = await Promise.all([ - InstanceGroupsAPI.readDetail(id), - SettingsAPI.readAll(), - ]); + const { data } = await InstanceGroupsAPI.readDetail(id); return { instanceGroup: data, - defaultControlPlane: DEFAULT_CONTROL_PLANE_QUEUE_NAME, - defaultExecution: DEFAULT_EXECUTION_QUEUE_NAME, }; }, [id]), - { instanceGroup: null, defaultControlPlane: '', defaultExecution: '' } + { instanceGroup: null } ); useEffect(() => { @@ -133,18 +120,10 @@ function InstanceGroup({ setBreadcrumb }) { {instanceGroup && ( <> - + - + @@ -115,8 +109,7 @@ function InstanceGroupDetails({ {t`Edit`} )} - {!isDefaultInstanceGroup && - instanceGroup.summary_fields.user_capabilities && + {instanceGroup.summary_fields.user_capabilities && instanceGroup.summary_fields.user_capabilities.delete && ( ', () => { history = createMemoryHistory(); await act(async () => { wrapper = mountWithContexts( - , + , { context: { router: { history } }, } @@ -70,27 +67,6 @@ describe('', () => { jest.clearAllMocks(); }); - test('controlplane instance group name can not be updated', async () => { - let towerWrapper; - await act(async () => { - towerWrapper = mountWithContexts( - , - { - context: { router: { history } }, - } - ); - }); - expect( - towerWrapper.find('input#instance-group-name').prop('disabled') - ).toBeTruthy(); - expect( - towerWrapper.find('input#instance-group-name').prop('value') - ).toEqual('controlplane'); - }); - test('handleSubmit should call the api and redirect to details page', async () => { await act(async () => { wrapper.find('InstanceGroupForm').invoke('onSubmit')( diff --git a/awx/ui/src/screens/InstanceGroup/InstanceGroupList/InstanceGroupList.js b/awx/ui/src/screens/InstanceGroup/InstanceGroupList/InstanceGroupList.js index 8f6e39edb6..dfce293dbd 100644 --- a/awx/ui/src/screens/InstanceGroup/InstanceGroupList/InstanceGroupList.js +++ b/awx/ui/src/screens/InstanceGroup/InstanceGroupList/InstanceGroupList.js @@ -4,7 +4,7 @@ import { useLocation, useRouteMatch, Link } from 'react-router-dom'; import { t, Plural } from '@lingui/macro'; import { Card, PageSection, DropdownItem } from '@patternfly/react-core'; -import { InstanceGroupsAPI, SettingsAPI } from 'api'; +import { InstanceGroupsAPI } from 'api'; import { getQSConfig, parseQueryString } from 'util/qs'; import useRequest, { useDeleteItems } from 'hooks/useRequest'; import useSelected from 'hooks/useSelected'; @@ -27,28 +27,6 @@ const QS_CONFIG = getQSConfig('instance-group', { page_size: 20, }); -function modifyInstanceGroups( - defaultControlPlane, - defaultExecution, - items = [] -) { - return items.map((item) => { - const clonedItem = { - ...item, - summary_fields: { - ...item.summary_fields, - user_capabilities: { - ...item.summary_fields.user_capabilities, - }, - }, - }; - if (clonedItem.name === (defaultControlPlane || defaultExecution)) { - clonedItem.summary_fields.user_capabilities.delete = false; - } - return clonedItem; - }); -} - function InstanceGroupList({ isKubernetes, isSettingsRequestLoading, @@ -56,30 +34,6 @@ function InstanceGroupList({ }) { const location = useLocation(); const match = useRouteMatch(); - const { - error: protectedItemsError, - isLoading: isLoadingProtectedItems, - request: fetchProtectedItems, - result: { defaultControlPlane, defaultExecution }, - } = useRequest( - useCallback(async () => { - const { - data: { - DEFAULT_CONTROL_PLANE_QUEUE_NAME, - DEFAULT_EXECUTION_QUEUE_NAME, - }, - } = await SettingsAPI.readAll(); - return { - defaultControlPlane: DEFAULT_CONTROL_PLANE_QUEUE_NAME, - defaultExecution: DEFAULT_EXECUTION_QUEUE_NAME, - }; - }, []), - { defaultControlPlane: '', defaultExecution: '' } - ); - - useEffect(() => { - fetchProtectedItems(); - }, [fetchProtectedItems]); const { error: contentError, @@ -127,12 +81,6 @@ function InstanceGroupList({ const { selected, isAllSelected, handleSelect, clearSelected, selectAll } = useSelected(instanceGroups); - const modifiedSelected = modifyInstanceGroups( - defaultControlPlane, - defaultExecution, - selected - ); - const { isLoading: deleteLoading, deletionError, @@ -158,28 +106,10 @@ function InstanceGroupList({ const canAdd = actions && actions.POST; - const cannotDelete = (item) => - !item.summary_fields.user_capabilities.delete || - item.name === defaultExecution || - item.name === defaultControlPlane; + const cannotDelete = (item) => !item.summary_fields.user_capabilities.delete; const pluralizedItemName = t`Instance Groups`; - let errorMessageDelete = ''; - const notdeletedable = selected.filter( - (i) => i.name === defaultControlPlane || i.name === defaultExecution - ); - - if (notdeletedable.length) { - errorMessageDelete = ( - - ); - } - const addContainerGroup = t`Add container group`; const addInstanceGroup = t`Add instance group`; @@ -229,14 +159,9 @@ function InstanceGroupList({ { const { - data: { - IS_K8S, - DEFAULT_CONTROL_PLANE_QUEUE_NAME, - DEFAULT_EXECUTION_QUEUE_NAME, - }, + data: { IS_K8S }, } = await SettingsAPI.readCategory('all'); return { isKubernetes: IS_K8S, - defaultControlPlane: DEFAULT_CONTROL_PLANE_QUEUE_NAME, - defaultExecution: DEFAULT_EXECUTION_QUEUE_NAME, }; }, []), - { isLoading: true } + { isKubernetes: false } ); useEffect(() => { - settingsRequest(); - }, [settingsRequest]); + userCanReadSettings && settingsRequest(); + }, [settingsRequest, userCanReadSettings]); const [breadcrumbConfig, setBreadcrumbConfig] = useState({ '/instance_groups': t`Instance Groups`, @@ -91,20 +89,14 @@ function InstanceGroups() { ) : ( - + {!isKubernetes && ( - + )} diff --git a/awx/ui/src/screens/InstanceGroup/InstanceGroups.test.js b/awx/ui/src/screens/InstanceGroup/InstanceGroups.test.js index fdd53c6e8f..84f269cc0a 100644 --- a/awx/ui/src/screens/InstanceGroup/InstanceGroups.test.js +++ b/awx/ui/src/screens/InstanceGroup/InstanceGroups.test.js @@ -2,6 +2,7 @@ import React from 'react'; import { shallow } from 'enzyme'; import { InstanceGroupsAPI } from 'api'; import InstanceGroups from './InstanceGroups'; +import { useUserProfile } from 'contexts/Config'; const mockUseLocationValue = { pathname: '', @@ -11,6 +12,19 @@ jest.mock('react-router-dom', () => ({ ...jest.requireActual('react-router-dom'), useLocation: () => mockUseLocationValue, })); + +beforeEach(() => { + useUserProfile.mockImplementation(() => { + return { + isSuperUser: true, + isSystemAuditor: false, + isOrgAdmin: false, + isNotificationAdmin: false, + isExecEnvAdmin: false, + }; + }); +}); + describe('', () => { test('should set breadcrumbs', () => { mockUseLocationValue.pathname = '/instance_groups'; diff --git a/awx/ui/src/screens/InstanceGroup/shared/ContainerGroupForm.js b/awx/ui/src/screens/InstanceGroup/shared/ContainerGroupForm.js index 41d7d60ac5..e9feed4e87 100644 --- a/awx/ui/src/screens/InstanceGroup/shared/ContainerGroupForm.js +++ b/awx/ui/src/screens/InstanceGroup/shared/ContainerGroupForm.js @@ -11,7 +11,7 @@ import FormField, { CheckboxField, } from 'components/FormField'; import FormActionGroup from 'components/FormActionGroup'; -import { combine, required, protectedResourceName } from 'util/validators'; +import { required } from 'util/validators'; import { FormColumnLayout, FormFullWidthLayout, @@ -21,21 +21,11 @@ import { import CredentialLookup from 'components/Lookup/CredentialLookup'; import { VariablesField } from 'components/CodeEditor'; -function ContainerGroupFormFields({ - instanceGroup, - defaultControlPlane, - defaultExecution, -}) { +function ContainerGroupFormFields({ instanceGroup }) { const { setFieldValue, setFieldTouched } = useFormikContext(); const [credentialField, credentialMeta, credentialHelpers] = useField('credential'); - const [, { initialValue }] = useField('name'); - - const isProtected = - initialValue === `${defaultControlPlane}` || - initialValue === `${defaultExecution}`; - const [overrideField] = useField('override'); const handleCredentialUpdate = useCallback( @@ -50,21 +40,10 @@ function ContainerGroupFormFields({ <> ', () => { wrapper.find('button[aria-label="Cancel"]').invoke('onClick')(); expect(onCancel).toBeCalled(); }); - - test('Name field should be disabled, default', async () => { - let defaultInstanceGroupWrapper; - await act(async () => { - defaultInstanceGroupWrapper = mountWithContexts( - - ); - }); - expect( - defaultInstanceGroupWrapper - .find('TextInput[name="name"]') - .prop('isDisabled') - ).toBe(true); - }); - - test('Name field should be disabled, controlplane', async () => { - let defaultInstanceGroupWrapper; - await act(async () => { - defaultInstanceGroupWrapper = mountWithContexts( - - ); - }); - expect( - defaultInstanceGroupWrapper - .find('TextInput[name="name"]') - .prop('isDisabled') - ).toBe(true); - }); }); diff --git a/awx/ui/src/setupTests.js b/awx/ui/src/setupTests.js index dc7d49014d..b9d84f3803 100644 --- a/awx/ui/src/setupTests.js +++ b/awx/ui/src/setupTests.js @@ -93,6 +93,7 @@ jest.doMock('./contexts/Config', () => ({ Config: MockConfigContext.Consumer, useConfig: () => React.useContext(MockConfigContext), useAuthorizedPath: jest.fn(), + useUserProfile: jest.fn(), })); // ?