feat: Add warning when removing access-permissions from last granted role (RocketChat#37073)

Author: dougfabris

.changeset/spicy-zebras-deliver.md

@@ -0,0 +1,5 @@
+---
+'@rocket.chat/meteor': minor
+---
+
+Adds a warning modal in order to prevent the `access-permissions` permission from being removed from the last granted role in the permissions table. This hardening measure ensures that administrators cannot accidentally lock themselves out of the system's permission management screen.

apps/meteor/app/authorization/lib/index.ts

@@ -9,4 +9,6 @@ export const CONSTANTS = {
 	SETTINGS_LEVEL: 'settings',
 } as const;
 
+export const confirmationRequiredPermissions = ['access-permissions'];
+
 export { AuthorizationUtils } from './AuthorizationUtils';

apps/meteor/client/views/admin/permissions/PermissionsPage.tsx

@@ -62,7 +62,7 @@ const PermissionsPage = ({ isEnterprise }: { isEnterprise: boolean }): ReactElem
 						<Tabs.Item
 							data-qa='PermissionTable-Permissions'
 							selected={type === 'permissions'}
-							onClick={handlePermissionsTab}
+							onClick={canViewPermission ? handlePermissionsTab : undefined}
 							disabled={!canViewPermission}
 						>
 							{t('Permissions')}

apps/meteor/client/views/admin/permissions/PermissionsTable/PermissionRow.tsx

@@ -34,16 +34,26 @@ type PermissionRowProps = {
 
 const PermissionRow = ({ permission, roleList, onGrant, onRemove }: PermissionRowProps): ReactElement => {
 	const { t } = useTranslation();
-	const { _id, roles } = permission;
-	const changeRole = useChangeRole({ onGrant, onRemove, permissionId: _id });
+	const { _id: permissionId, roles } = permission;
+	const changeRole = useChangeRole({ onGrant, onRemove, permissionId });
+	const permissionName = getName(t, permission);
 
 	return (
-		<GenericTableRow key={_id} role='link' action tabIndex={0}>
-			<GenericTableCell maxWidth='x300' withTruncatedText title={t(`${_id}_description` as TranslationKey)}>
-				{getName(t, permission)}
+		<GenericTableRow key={permissionId} role='link' action tabIndex={0}>
+			<GenericTableCell maxWidth='x300' withTruncatedText title={t(`${permissionId}_description` as TranslationKey)}>
+				{permissionName}
 			</GenericTableCell>
-			{roleList.map(({ _id, name, description }) => (
-				<RoleCell key={_id} _id={_id} name={name} description={description} grantedRoles={roles} onChange={changeRole} permissionId={_id} />
+			{roleList.map(({ _id: roleId, name, description }) => (
+				<RoleCell
+					key={roleId}
+					_id={roleId}
+					name={name}
+					description={description}
+					grantedRoles={roles}
+					onChange={changeRole}
+					permissionId={permissionId}
+					permissionName={permissionName}
+				/>
 			))}
 		</GenericTableRow>
 	);

apps/meteor/client/views/admin/permissions/PermissionsTable/PermissionsTable.spec.tsx

@@ -1,8 +1,11 @@
+import type { IPermission, IRole } from '@rocket.chat/core-typings';
 import { mockAppRoot } from '@rocket.chat/mock-providers';
 import { composeStories } from '@storybook/react';
-import { render } from '@testing-library/react';
+import { render, screen } from '@testing-library/react';
+import userEvent from '@testing-library/user-event';
 import { axe } from 'jest-axe';
 
+import PermissionsTable from './PermissionsTable';
 import * as stories from './PermissionsTable.stories';
 
 const testCases = Object.values(composeStories(stories)).map((Story) => [Story.storyName || 'Story', Story]);
@@ -20,3 +23,57 @@ test.each(testCases)('%s should have no a11y violations', async (_storyname, Sto
 	const results = await axe(container, { rules: { 'label': { enabled: false }, 'button-name': { enabled: false } } });
 	expect(results).toHaveNoViolations();
 });
+
+const defaultPermissions: IPermission[] = [
+	{
+		_id: 'access-permissions',
+		_updatedAt: new Date('2023-01-01'),
+		roles: ['admin'],
+	},
+];
+
+const roles: IRole[] = [
+	{
+		description: 'Owner of the workspace',
+		name: 'owner',
+		protected: true,
+		scope: 'Users',
+		_id: 'owner',
+	},
+	{
+		description: 'Administrator',
+		name: 'admin',
+		protected: true,
+		scope: 'Users',
+		_id: 'admin',
+	},
+];
+
+test('should display modal if the permission is access-permissions and has only one granted role', async () => {
+	render(
+		<PermissionsTable permissions={defaultPermissions} total={defaultPermissions.length} setFilter={() => undefined} roleList={roles} />,
+		{
+			wrapper: mockAppRoot().build(),
+		},
+	);
+
+	await userEvent.click(screen.getByRole('checkbox', { name: 'access-permissions - Administrator' }));
+	expect(screen.getByRole('dialog')).toBeInTheDocument();
+});
+
+test('should NOT display modal if the permission is access-permissions and has more than one granted role', async () => {
+	const morePermissions: IPermission[] = [
+		{
+			_id: 'access-permissions',
+			_updatedAt: new Date('2023-01-01'),
+			roles: ['admin', 'owner'],
+		},
+	];
+
+	render(<PermissionsTable permissions={morePermissions} total={morePermissions.length} setFilter={() => undefined} roleList={roles} />, {
+		wrapper: mockAppRoot().build(),
+	});
+
+	await userEvent.click(screen.getByRole('checkbox', { name: 'access-permissions - Administrator' }));
+	expect(screen.queryByRole('dialog')).not.toBeInTheDocument();
+});

apps/meteor/client/views/admin/permissions/PermissionsTable/PermissionsTable.stories.tsx

@@ -80,41 +80,21 @@ const permissions: IPermission[] = [
 		_id: '0',
 		_updatedAt: new Date('2023-01-01'),
 		roles: ['admin'],
-		group: 'admin',
-		level: 'settings',
-		section: 'General',
-		settingId: 'general_settings',
-		sorter: 1,
 	},
 	{
 		_id: '1',
 		_updatedAt: new Date('2023-01-01'),
 		roles: ['user'],
-		group: 'admin',
-		level: 'settings',
-		section: 'General',
-		settingId: 'general_settings',
-		sorter: 2,
 	},
 	{
 		_id: '2',
 		_updatedAt: new Date('2023-01-01'),
 		roles: ['user'],
-		group: 'admin',
-		level: 'settings',
-		section: 'General',
-		settingId: 'general_settings',
-		sorter: 3,
 	},
 	{
 		_id: '3',
 		_updatedAt: new Date('2023-01-01'),
 		roles: ['user'],
-		group: 'admin',
-		level: 'settings',
-		section: 'General',
-		settingId: 'general_settings',
-		sorter: 4,
 	},
 ];
 

apps/meteor/client/views/admin/permissions/PermissionsTable/PermissionsTable.tsx

@@ -16,7 +16,7 @@ type PermissionsTableProps = {
 	permissions: IPermission[];
 	setFilter: (filter: string) => void;
 	total: number;
-	paginationProps: ReturnType<typeof usePagination>;
+	paginationProps?: ReturnType<typeof usePagination>;
 };
 
 const PermissionsTable = ({ roleList, permissions, setFilter, total, paginationProps }: PermissionsTableProps) => {

apps/meteor/client/views/admin/permissions/PermissionsTable/RoleCell.tsx

@@ -1,10 +1,13 @@
 import type { IRole } from '@rocket.chat/core-typings';
 import { Margins, Box, CheckBox, Throbber } from '@rocket.chat/fuselage';
 import { useEffectEvent } from '@rocket.chat/fuselage-hooks';
+import { GenericModal } from '@rocket.chat/ui-client';
+import { useSetModal } from '@rocket.chat/ui-contexts';
 import type { ReactElement } from 'react';
 import { useState, memo } from 'react';
+import { useTranslation } from 'react-i18next';
 
-import { AuthorizationUtils } from '../../../../../app/authorization/lib';
+import { AuthorizationUtils, confirmationRequiredPermissions } from '../../../../../app/authorization/lib';
 import { GenericTableCell } from '../../../../components/GenericTable';
 
 type RoleCellProps = {
@@ -13,28 +16,50 @@ type RoleCellProps = {
 	description: IRole['description'];
 	onChange: (roleId: IRole['_id'], granted: boolean) => Promise<boolean>;
 	permissionId: string;
+	permissionName: string;
 	grantedRoles: IRole['_id'][];
 };
 
-const RoleCell = ({ _id, name, description, onChange, permissionId, grantedRoles = [] }: RoleCellProps): ReactElement => {
+const RoleCell = ({ _id, name, description, onChange, permissionId, permissionName, grantedRoles = [] }: RoleCellProps): ReactElement => {
+	const { t } = useTranslation();
+	const setModal = useSetModal();
 	const [granted, setGranted] = useState(() => !!grantedRoles.includes(_id));
 	const [loading, setLoading] = useState(false);
 
 	const isRestrictedForRole = AuthorizationUtils.isPermissionRestrictedForRole(permissionId, _id);
+	const shouldDisplayConfirmation = confirmationRequiredPermissions.includes(permissionId) && grantedRoles.length === 1 && granted;
 
-	const handleChange = useEffectEvent(async () => {
+	const handleChange = useEffectEvent(() => {
+		if (shouldDisplayConfirmation) {
+			const handleSubmit = () => {
+				handleConfirmChange();
+				setModal(null);
+			};
+
+			return setModal(
+				<GenericModal onConfirm={handleSubmit} onCancel={() => setModal(null)} confirmText={t('Remove')} variant='danger'>
+					{t('Last_role_in_permission_warning')}
+				</GenericModal>,
+			);
+		}
+
+		return handleConfirmChange();
+	});
+
+	const handleConfirmChange = useEffectEvent(async () => {
 		setLoading(true);
 		const result = await onChange(_id, granted);
 		setGranted(result);
 		setLoading(false);
 	});
 
 	const isDisabled = !!loading || !!isRestrictedForRole;
+	const checkboxLabel = `${permissionName} - ${description || name}`;
 
 	return (
 		<GenericTableCell withTruncatedText>
 			<Margins inline={2}>
-				<CheckBox checked={granted} onChange={handleChange} disabled={isDisabled} />
+				<CheckBox aria-label={checkboxLabel} checked={granted} onChange={handleChange} disabled={isDisabled} />
 				{!loading && (
 					<Box display='inline' color='hint' invisible>
 						{description || name}