diff --git a/.changeset/spicy-zebras-deliver.md b/.changeset/spicy-zebras-deliver.md new file mode 100644 index 0000000000000..632edbb298cba --- /dev/null +++ b/.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. diff --git a/apps/meteor/app/authorization/lib/index.ts b/apps/meteor/app/authorization/lib/index.ts index 8e2a37a64138a..c8e5e7bbdc017 100644 --- a/apps/meteor/app/authorization/lib/index.ts +++ b/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'; diff --git a/apps/meteor/client/views/admin/permissions/PermissionsPage.tsx b/apps/meteor/client/views/admin/permissions/PermissionsPage.tsx index 43b192054968d..96c0e9950433a 100644 --- a/apps/meteor/client/views/admin/permissions/PermissionsPage.tsx +++ b/apps/meteor/client/views/admin/permissions/PermissionsPage.tsx @@ -62,7 +62,7 @@ const PermissionsPage = ({ isEnterprise }: { isEnterprise: boolean }): ReactElem {t('Permissions')} diff --git a/apps/meteor/client/views/admin/permissions/PermissionsTable/PermissionRow.tsx b/apps/meteor/client/views/admin/permissions/PermissionsTable/PermissionRow.tsx index 2fb8853610246..2c0392e7f761b 100644 --- a/apps/meteor/client/views/admin/permissions/PermissionsTable/PermissionRow.tsx +++ b/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 ( - - - {getName(t, permission)} + + + {permissionName} - {roleList.map(({ _id, name, description }) => ( - + {roleList.map(({ _id: roleId, name, description }) => ( + ))} ); diff --git a/apps/meteor/client/views/admin/permissions/PermissionsTable/PermissionsTable.spec.tsx b/apps/meteor/client/views/admin/permissions/PermissionsTable/PermissionsTable.spec.tsx index 3922eab005c32..c1116b09798cc 100644 --- a/apps/meteor/client/views/admin/permissions/PermissionsTable/PermissionsTable.spec.tsx +++ b/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( + 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( undefined} roleList={roles} />, { + wrapper: mockAppRoot().build(), + }); + + await userEvent.click(screen.getByRole('checkbox', { name: 'access-permissions - Administrator' })); + expect(screen.queryByRole('dialog')).not.toBeInTheDocument(); +}); diff --git a/apps/meteor/client/views/admin/permissions/PermissionsTable/PermissionsTable.stories.tsx b/apps/meteor/client/views/admin/permissions/PermissionsTable/PermissionsTable.stories.tsx index 7ef2e4dfc01fe..c553c19f36fb3 100644 --- a/apps/meteor/client/views/admin/permissions/PermissionsTable/PermissionsTable.stories.tsx +++ b/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, }, ]; diff --git a/apps/meteor/client/views/admin/permissions/PermissionsTable/PermissionsTable.tsx b/apps/meteor/client/views/admin/permissions/PermissionsTable/PermissionsTable.tsx index c3752505e7afe..e034adfcf1f46 100644 --- a/apps/meteor/client/views/admin/permissions/PermissionsTable/PermissionsTable.tsx +++ b/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; + paginationProps?: ReturnType; }; const PermissionsTable = ({ roleList, permissions, setFilter, total, paginationProps }: PermissionsTableProps) => { diff --git a/apps/meteor/client/views/admin/permissions/PermissionsTable/RoleCell.tsx b/apps/meteor/client/views/admin/permissions/PermissionsTable/RoleCell.tsx index cc9fec9dab43e..70eddbafcc098 100644 --- a/apps/meteor/client/views/admin/permissions/PermissionsTable/RoleCell.tsx +++ b/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,16 +16,37 @@ type RoleCellProps = { description: IRole['description']; onChange: (roleId: IRole['_id'], granted: boolean) => Promise; 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( + setModal(null)} confirmText={t('Remove')} variant='danger'> + {t('Last_role_in_permission_warning')} + , + ); + } + + return handleConfirmChange(); + }); + + const handleConfirmChange = useEffectEvent(async () => { setLoading(true); const result = await onChange(_id, granted); setGranted(result); @@ -30,11 +54,12 @@ const RoleCell = ({ _id, name, description, onChange, permissionId, grantedRoles }); const isDisabled = !!loading || !!isRestrictedForRole; + const checkboxLabel = `${permissionName} - ${description || name}`; return ( - + {!loading && ( {description || name} diff --git a/apps/meteor/client/views/admin/permissions/PermissionsTable/__snapshots__/PermissionsTable.spec.tsx.snap b/apps/meteor/client/views/admin/permissions/PermissionsTable/__snapshots__/PermissionsTable.spec.tsx.snap index c3685ddaaf682..397abc9b43e5d 100644 --- a/apps/meteor/client/views/admin/permissions/PermissionsTable/__snapshots__/PermissionsTable.spec.tsx.snap +++ b/apps/meteor/client/views/admin/permissions/PermissionsTable/__snapshots__/PermissionsTable.spec.tsx.snap @@ -270,7 +270,7 @@ exports[`renders Default without crashing 1`] = ` class="rcx-box rcx-box--full rcx-table__cell rcx-css-117bfyn" title="0_description" > - admin > General > general_settings + 0 @@ -300,6 +301,7 @@ exports[`renders Default without crashing 1`] = ` class="rcx-box rcx-box--full rcx-check-box rcx-css-17ztjgq rcx-css-wcp0mp" > @@ -343,6 +346,7 @@ exports[`renders Default without crashing 1`] = ` class="rcx-box rcx-box--full rcx-check-box rcx-css-17ztjgq rcx-css-wcp0mp" > @@ -364,6 +368,7 @@ exports[`renders Default without crashing 1`] = ` class="rcx-box rcx-box--full rcx-check-box rcx-css-17ztjgq rcx-css-wcp0mp" > @@ -385,6 +390,7 @@ exports[`renders Default without crashing 1`] = ` class="rcx-box rcx-box--full rcx-check-box rcx-css-17ztjgq rcx-css-wcp0mp" > @@ -406,6 +412,7 @@ exports[`renders Default without crashing 1`] = ` class="rcx-box rcx-box--full rcx-check-box rcx-css-17ztjgq rcx-css-wcp0mp" > @@ -427,6 +434,7 @@ exports[`renders Default without crashing 1`] = ` class="rcx-box rcx-box--full rcx-check-box rcx-css-17ztjgq rcx-css-wcp0mp" > @@ -451,7 +459,7 @@ exports[`renders Default without crashing 1`] = ` class="rcx-box rcx-box--full rcx-table__cell rcx-css-117bfyn" title="1_description" > - admin > General > general_settings + 1 @@ -481,6 +490,7 @@ exports[`renders Default without crashing 1`] = ` class="rcx-box rcx-box--full rcx-check-box rcx-css-17ztjgq rcx-css-wcp0mp" > @@ -502,6 +512,7 @@ exports[`renders Default without crashing 1`] = ` class="rcx-box rcx-box--full rcx-check-box rcx-css-17ztjgq rcx-css-wcp0mp" > @@ -523,6 +534,7 @@ exports[`renders Default without crashing 1`] = ` class="rcx-box rcx-box--full rcx-check-box rcx-css-17ztjgq rcx-css-wcp0mp" > @@ -544,6 +556,7 @@ exports[`renders Default without crashing 1`] = ` class="rcx-box rcx-box--full rcx-check-box rcx-css-17ztjgq rcx-css-wcp0mp" > @@ -587,6 +601,7 @@ exports[`renders Default without crashing 1`] = ` class="rcx-box rcx-box--full rcx-check-box rcx-css-17ztjgq rcx-css-wcp0mp" > @@ -608,6 +623,7 @@ exports[`renders Default without crashing 1`] = ` class="rcx-box rcx-box--full rcx-check-box rcx-css-17ztjgq rcx-css-wcp0mp" > @@ -632,7 +648,7 @@ exports[`renders Default without crashing 1`] = ` class="rcx-box rcx-box--full rcx-table__cell rcx-css-117bfyn" title="2_description" > - admin > General > general_settings + 2 @@ -662,6 +679,7 @@ exports[`renders Default without crashing 1`] = ` class="rcx-box rcx-box--full rcx-check-box rcx-css-17ztjgq rcx-css-wcp0mp" > @@ -683,6 +701,7 @@ exports[`renders Default without crashing 1`] = ` class="rcx-box rcx-box--full rcx-check-box rcx-css-17ztjgq rcx-css-wcp0mp" > @@ -704,6 +723,7 @@ exports[`renders Default without crashing 1`] = ` class="rcx-box rcx-box--full rcx-check-box rcx-css-17ztjgq rcx-css-wcp0mp" > @@ -725,6 +745,7 @@ exports[`renders Default without crashing 1`] = ` class="rcx-box rcx-box--full rcx-check-box rcx-css-17ztjgq rcx-css-wcp0mp" > @@ -768,6 +790,7 @@ exports[`renders Default without crashing 1`] = ` class="rcx-box rcx-box--full rcx-check-box rcx-css-17ztjgq rcx-css-wcp0mp" > @@ -789,6 +812,7 @@ exports[`renders Default without crashing 1`] = ` class="rcx-box rcx-box--full rcx-check-box rcx-css-17ztjgq rcx-css-wcp0mp" > @@ -813,7 +837,7 @@ exports[`renders Default without crashing 1`] = ` class="rcx-box rcx-box--full rcx-table__cell rcx-css-117bfyn" title="3_description" > - admin > General > general_settings + 3 @@ -843,6 +868,7 @@ exports[`renders Default without crashing 1`] = ` class="rcx-box rcx-box--full rcx-check-box rcx-css-17ztjgq rcx-css-wcp0mp" > @@ -864,6 +890,7 @@ exports[`renders Default without crashing 1`] = ` class="rcx-box rcx-box--full rcx-check-box rcx-css-17ztjgq rcx-css-wcp0mp" > @@ -885,6 +912,7 @@ exports[`renders Default without crashing 1`] = ` class="rcx-box rcx-box--full rcx-check-box rcx-css-17ztjgq rcx-css-wcp0mp" > @@ -906,6 +934,7 @@ exports[`renders Default without crashing 1`] = ` class="rcx-box rcx-box--full rcx-check-box rcx-css-17ztjgq rcx-css-wcp0mp" > @@ -949,6 +979,7 @@ exports[`renders Default without crashing 1`] = ` class="rcx-box rcx-box--full rcx-check-box rcx-css-17ztjgq rcx-css-wcp0mp" > @@ -970,6 +1001,7 @@ exports[`renders Default without crashing 1`] = ` class="rcx-box rcx-box--full rcx-check-box rcx-css-17ztjgq rcx-css-wcp0mp" > diff --git a/packages/i18n/src/locales/en.i18n.json b/packages/i18n/src/locales/en.i18n.json index 1ad8a3beeac6f..768add4842834 100644 --- a/packages/i18n/src/locales/en.i18n.json +++ b/packages/i18n/src/locales/en.i18n.json @@ -2930,6 +2930,7 @@ "Language_Version": "English Version", "Language_setting_warning": "Server language setting does not affect user's client
Each user has their own preference for language, that will be kept if this setting is changed.", "Larger_amounts_of_active_connections": "For larger amounts of active connections you can consider our <1>multiple instance solutions.", + "Last_role_in_permission_warning": "This is the last role with this permission. Removing it will soft-lock this page from the UI, requiring database access to revert it. Do you want to proceed?", "Last_5_minutes": "Last 5 minutes", "Last_15_minutes": "Last 15 minutes", "Last_30_minutes": "Last 30 minutes",