Skip to content

Commit

Permalink
fix(ESSNTL-5239): Improve RBAC check for bulk group actions (#1974)
Browse files Browse the repository at this point in the history 
Implements https://issues.redhat.com/browse/ESSNTL-5239.
  • Loading branch information
@gkarat
gkarat authored Aug 15, 2023
1 parent 070d9e2 commit ed49f99
Show file tree
Hide file tree
Showing 3 changed files with 136 additions and 57 deletions.
94 changes: 54 additions & 40 deletions src/components/InventoryGroups/Modals/RemoveHostsFromGroupModal.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,27 +7,56 @@ import { removeHostsFromGroup } from '../utils/api';
import componentTypes from '@data-driven-forms/react-form-renderer/component-types';
import { Text } from '@patternfly/react-core';

const schema = (groupName, hosts) => ({
fields: [
{
component: componentTypes.PLAIN_TEXT,
name: 'warning-message',
label:
hosts.length === 1 ? (
<Text>
<strong>{hosts[0].display_name}</strong> will no longer be part of{' '}
<strong>{groupName}</strong> and its configuration will be impacted.
</Text>
) : (
<Text>
<strong>{hosts.length}</strong> systems will no longer be part of{' '}
<strong>{groupName}</strong> and their configuration will be
impacted.
</Text>
),
},
],
});
const schema = (hosts) => {
const hostsInGroup = hosts.filter(({ groups }) => groups.length > 0); // selection can contain ungroupped hosts
const groupName = hostsInGroup[0].groups[0].name;

return {
fields: [
{
component: componentTypes.PLAIN_TEXT,
name: 'warning-message',
label:
hostsInGroup.length === 1 ? (
<Text>
<strong>{hostsInGroup[0].display_name}</strong> will no longer be
part of <strong>{groupName}</strong> and its configuration will be
impacted.
</Text>
) : (
<Text>
<strong>{hostsInGroup.length}</strong> systems will no longer be
part of <strong>{groupName}</strong> and their configuration will
be impacted.
</Text>
),
},
],
};
};

const statusMessages = (hosts) => {
const hostsInGroup = hosts.filter(({ groups }) => groups.length > 0);
const groupName = hostsInGroup[0].groups[0].name;

return hostsInGroup.length === 1
? {
onSuccess: {
title: `1 system removed from ${groupName}`,
},
onError: {
title: `Failed to remove 1 system from ${groupName}`,
},
}
: {
onSuccess: {
title: `${hostsInGroup.length} systems removed from ${groupName}`,
},
onError: {
title: `Failed to remove ${hostsInGroup.length} systems from ${groupName}`,
},
};
};

const RemoveHostsFromGroupModal = ({
isModalOpen,
Expand All @@ -37,33 +66,18 @@ const RemoveHostsFromGroupModal = ({
reloadTimeout,
}) => {
const dispatch = useDispatch();
// the current iteration of groups feature a host can be in at maximum one group
const { name: groupName, id: groupId } = hosts[0].groups[0];

const handleRemoveHosts = () => {
const statusMessages = {
onSuccess: {
title: `${hosts.length} ${
hosts.length > 1 ? 'systems' : 'system'
} removed from ${groupName}`,
},
onError: {
title: `Failed to remove ${hosts.length} ${
hosts.length > 1 ? 'systems' : 'system'
} from ${groupName}`,
},
};
const groupId = hosts.find(({ groups }) => groups.length > 0).groups[0].id;

const handleRemoveHosts = () =>
apiWithToast(
dispatch,
async () =>
await removeHostsFromGroup(
groupId,
hosts.map(({ id }) => id)
),
statusMessages
statusMessages(hosts)
);
};

return (
<Modal
Expand All @@ -72,7 +86,7 @@ const RemoveHostsFromGroupModal = ({
title="Remove from group"
variant="danger"
submitLabel="Remove"
schema={schema(groupName, hosts)}
schema={schema(hosts)}
onSubmit={handleRemoveHosts}
reloadData={reloadData}
reloadTimeout={reloadTimeout}
Expand Down
56 changes: 53 additions & 3 deletions src/routes/InventoryTable.cy.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -66,8 +66,9 @@ describe('test data', () => {
.to.be.true;
});

it('the fourth host is not in a group', () => {
it('the fourth and fifth hosts are not in a group', () => {
expect(hostsFixtures.results[3].groups.length === 0).to.be.true;
expect(hostsFixtures.results[4].groups.length === 0).to.be.true;
});
});

Expand Down Expand Up @@ -387,8 +388,57 @@ describe('inventory table', () => {
.contains('Remove from group')
.shouldHaveAriaDisabled();
});
});

// TODO: test group bulk actions once granular RBAC is implemented there too
it('can bulk remove from the permitted group', () => {
cy.get(ROW).find('[type="checkbox"]').eq(0).click();
// TODO: implement ouia selector for this component
cy.get(
'.ins-c-primary-toolbar__actions [aria-label="Actions"]'
).click();
cy.get(DROPDOWN_ITEM)
.contains('Remove from group')
.shouldHaveAriaEnabled();
cy.get(ROW).find('[type="checkbox"]').eq(1).click();
// TODO: implement ouia selector for this component
cy.get(
'.ins-c-primary-toolbar__actions [aria-label="Actions"]'
).click();
cy.get(DROPDOWN_ITEM)
.contains('Remove from group')
.shouldHaveAriaEnabled();
});

it('can bulk remove from group together with ungroupped hosts', () => {
cy.get(ROW).find('[type="checkbox"]').eq(0).click();
cy.get(ROW).find('[type="checkbox"]').eq(3).click();
// TODO: implement ouia selector for this component
cy.get(
'.ins-c-primary-toolbar__actions [aria-label="Actions"]'
).click();
cy.get(DROPDOWN_ITEM)
.contains('Remove from group')
.shouldHaveAriaEnabled();
});

it('can bulk add hosts to the permitted group', () => {
cy.get(ROW).find('[type="checkbox"]').eq(3).click();
cy.get(ROW).find('[type="checkbox"]').eq(4).click();
// TODO: implement ouia selector for this component
cy.get(
'.ins-c-primary-toolbar__actions [aria-label="Actions"]'
).click();
cy.get(DROPDOWN_ITEM).contains('Add to group').shouldHaveAriaEnabled();
});

it('cannot bulk add to group if groupped hosts selected', () => {
cy.get(ROW).find('[type="checkbox"]').eq(0).click();
cy.get(ROW).find('[type="checkbox"]').eq(3).click();
// TODO: implement ouia selector for this component
cy.get(
'.ins-c-primary-toolbar__actions [aria-label="Actions"]'
).click();
cy.get(DROPDOWN_ITEM).contains('Add to group').shouldHaveAriaDisabled();
});
});
});
});
43 changes: 29 additions & 14 deletions src/routes/InventoryTable.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ import {
ActionButton,
ActionDropdownItem,
} from '../components/InventoryTable/ActionWithRBAC';
import uniq from 'lodash/uniq';

const mapTags = ({ category, values }) =>
values.map(
Expand Down Expand Up @@ -272,17 +273,16 @@ const Inventory = ({
const calculateSelected = () => (selected ? selected.size : 0);

const isBulkRemoveFromGroupsEnabled = () => {
if (calculateSelected() > 0) {
const selectedHosts = Array.from(selected.values());

return selectedHosts.every(
({ groups }) =>
groups.length !== 0 &&
groups[0].name === selectedHosts[0].groups[0].name
);
}

return false;
return (
calculateSelected() > 0 &&
Array.from(selected.values()).some(({ groups }) => groups.length > 0) &&
uniq(
// can remove from at maximum one group at a time
Array.from(selected.values())
.filter(({ groups }) => groups.length > 0)
.map(({ groups }) => groups[0].name)
).length === 1
);
};

const isBulkAddHostsToGroupsEnabled = () => {
Expand Down Expand Up @@ -448,6 +448,7 @@ const Inventory = ({
setCurrentSystem(Array.from(selected.values()));
setAddHostGroupModalOpen(true);
}}
ignoreResourceDefinitions
>
Add to group
</ActionDropdownItem>
Expand All @@ -462,15 +463,29 @@ const Inventory = ({
label: (
<ActionDropdownItem
key="bulk-remove-from-group"
requiredPermissions={[
GENERAL_GROUPS_WRITE_PERMISSION,
]}
requiredPermissions={
selected !== undefined
? Array.from(selected.values())
.flatMap(({ groups }) =>
groups?.[0]?.id !== undefined
? REQUIRED_PERMISSIONS_TO_MODIFY_GROUP(
groups[0].id
)
: null
)
.filter(Boolean) // don't check ungroupped hosts
: []
}
isAriaDisabled={!isBulkRemoveFromGroupsEnabled()}
noAccessTooltip={NO_MODIFY_GROUPS_TOOLTIP_MESSAGE}
onClick={() => {
setCurrentSystem(Array.from(selected.values()));
setRemoveHostsFromGroupModalOpen(true);
}}
{...(selected === undefined // when nothing is selected, no access must be checked
? { override: true }
: {})}
checkAll
>
Remove from group
</ActionDropdownItem>
Expand Down

0 comments on commit ed49f99

Please sign in to comment.