feat: Add warning when removing access-permissions from last granted role
#37073
Conversation
|
Looks like this PR is not ready to merge, because of the following issues:
Please fix the issues and try again If you have any trouble, please check the PR guidelines |
🦋 Changeset detectedLatest commit: c483dc5 The changes in this PR will be included in the next version bump. This PR includes changesets to release 42 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
WalkthroughAdds a guarded confirmation modal preventing removal of the last granted role for protected permissions (notably Changes
Sequence Diagram(s)sequenceDiagram
actor Admin as Admin User
participant PermPage as PermissionsPage
participant Table as PermissionsTable
participant RoleCell as RoleCell
participant Auth as AuthLib
participant Modal as ConfirmationModal
Admin->>PermPage: Open Permissions tab (guarded)
PermPage->>Table: Render permissions & roles
Admin->>RoleCell: Toggle role checkbox
RoleCell->>Auth: Check confirmationRequiredPermissions
RoleCell->>RoleCell: Determine if last granted role?
alt Confirmation required (protected permission + last role)
RoleCell->>Modal: Show `Last_role_in_permission_warning`
Admin-->>Modal: Confirm removal
Modal->>RoleCell: Submit
RoleCell->>RoleCell: handleConfirmChange -> call onChange, update state
else No confirmation
RoleCell->>RoleCell: handleConfirmChange -> call onChange, update state
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #37073 +/- ##
===========================================
+ Coverage 67.32% 67.35% +0.02%
===========================================
Files 3278 3276 -2
Lines 112000 111878 -122
Branches 20315 20282 -33
===========================================
- Hits 75402 75350 -52
+ Misses 33996 33933 -63
+ Partials 2602 2595 -7
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
dougfabris
changed the title
access-permissions from being remove from adminaccess-permissions from being removed from admin
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
apps/meteor/client/views/root/hooks/loggedIn/useRestrictedRoles.ts (1)
20-20: Consider centralizing hardcoded permission identifiers.The hardcoded string
'access-permissions'creates a maintenance concern if this permission identifier changes or if additional critical permissions need similar protection in the future.Consider extracting this to a constant:
// At the top of the file or in a shared constants file const CRITICAL_ADMIN_PERMISSIONS = ['access-permissions'] as const; // Then use it in the effect: AuthorizationUtils.addRolePermissionDisabledList('admin', CRITICAL_ADMIN_PERMISSIONS);apps/meteor/app/authorization/lib/AuthorizationUtils.spec.ts (2)
6-9: Consider adding cleanup in afterEach/afterAll.The
beforeAllhook sets up shared state inAuthorizationUtils, but there's no cleanup. If these maps persist between test suites, it could lead to test pollution.Consider adding cleanup:
afterAll(() => { // Reset the internal maps if AuthorizationUtils provides a reset method // or ensure tests are isolated });Note: If
AuthorizationUtilsuses module-level state (as suggested by the static methods), you may need to add areset()orclear()method for testing purposes.
11-33: Expand test coverage for edge cases and error conditions.The current tests cover the happy path but miss several important scenarios:
- Invalid parameters (null, undefined, empty strings)
- Multiple permissions in the disabled/whitelist
- Role that doesn't exist in either map
- Permission that exists for one role but not another
Consider adding tests for:
it('should throw error for invalid parameters', () => { expect(() => AuthorizationUtils.isPermissionDisabledForRole('', fakeRole)).toThrow('invalid-param'); expect(() => AuthorizationUtils.isPermissionDisabledForRole(fakePermissionId, '')).toThrow('invalid-param'); }); it('should return false for role without any disabled permissions', () => { const result = AuthorizationUtils.isPermissionDisabledForRole(fakePermissionId, 'non-existent-role'); expect(result).toBe(false); }); it('should handle multiple permissions in disabled list', () => { AuthorizationUtils.addRolePermissionDisabledList('test-role', ['perm1', 'perm2']); expect(AuthorizationUtils.isPermissionDisabledForRole('perm1', 'test-role')).toBe(true); expect(AuthorizationUtils.isPermissionDisabledForRole('perm2', 'test-role')).toBe(true); });apps/meteor/app/authorization/lib/AuthorizationUtils.ts (1)
29-50: Consider renaming the modifier parameter for clarity.The
modifierparameter with values'allow'and'deny'is semantically confusing:
'deny'actually implements whitelist logic (returns true when permission is NOT in the list)'allow'actually implements blacklist logic (returns true when permission IS in the list)This inversion can lead to maintenance errors.
Consider renaming to better reflect the actual behavior:
private static checkPermissionForRole( permissionId: string, roleId: string, permissionMap: Map<string, Set<string>>, - modifier: 'allow' | 'deny' = 'deny', + listType: 'whitelist' | 'blacklist' = 'whitelist', ): boolean { // ... validation ... const hasPermission = rules.has(permissionId); - return modifier === 'deny' ? !hasPermission : hasPermission; + return listType === 'whitelist' ? !hasPermission : hasPermission; }Then update the call sites:
static isPermissionRestrictedForRole(permissionId: string, roleId: string): boolean { - return this.checkPermissionForRole(permissionId, roleId, restrictedRolePermissions); + return this.checkPermissionForRole(permissionId, roleId, restrictedRolePermissions, 'whitelist'); } static isPermissionDisabledForRole(permissionId: string, roleId: string): boolean { - return this.checkPermissionForRole(permissionId, roleId, disabledRolePermissions, 'allow'); + return this.checkPermissionForRole(permissionId, roleId, disabledRolePermissions, 'blacklist'); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (8)
.changeset/spicy-zebras-deliver.md(1 hunks)apps/meteor/app/authorization/lib/AuthorizationUtils.spec.ts(1 hunks)apps/meteor/app/authorization/lib/AuthorizationUtils.ts(1 hunks)apps/meteor/client/views/admin/permissions/PermissionsPage.tsx(1 hunks)apps/meteor/client/views/admin/permissions/PermissionsTable/PermissionRow.tsx(1 hunks)apps/meteor/client/views/admin/permissions/PermissionsTable/RoleCell.tsx(2 hunks)apps/meteor/client/views/root/hooks/loggedIn/useRestrictedRoles.ts(1 hunks)apps/meteor/jest.config.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
PR: RocketChat/Rocket.Chat#0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : All Playwright test files must be located under apps/meteor/tests/e2e/ and use the .spec.ts extension (e.g., login.spec.ts)
Applied to files:
apps/meteor/jest.config.ts
🧬 Code graph analysis (4)
apps/meteor/app/authorization/lib/AuthorizationUtils.spec.ts (1)
apps/meteor/app/authorization/lib/AuthorizationUtils.ts (1)
AuthorizationUtils(4-77)
apps/meteor/client/views/admin/permissions/PermissionsTable/RoleCell.tsx (1)
apps/meteor/app/authorization/lib/AuthorizationUtils.ts (1)
AuthorizationUtils(4-77)
apps/meteor/client/views/root/hooks/loggedIn/useRestrictedRoles.ts (1)
apps/meteor/app/authorization/lib/AuthorizationUtils.ts (1)
AuthorizationUtils(4-77)
apps/meteor/client/views/admin/permissions/PermissionsTable/PermissionRow.tsx (2)
apps/meteor/app/utils/lib/i18n.ts (1)
t(6-6)packages/ui-contexts/src/index.ts (1)
TranslationKey(103-103)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: 📦 Build Packages
- GitHub Check: Builds matrix rust bindings against alpine
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🔇 Additional comments (10)
.changeset/spicy-zebras-deliver.md (1)
1-5: LGTM! Clear changeset entry.The changeset correctly documents the hardening measure and clearly explains the purpose of preventing administrators from accidentally removing their own access to the permission management screen.
apps/meteor/jest.config.ts (1)
16-16: LGTM! Appropriate test coverage extension.The new testMatch pattern correctly includes authorization specs under the client project configuration, aligning with the new unit tests for
AuthorizationUtils.apps/meteor/app/authorization/lib/AuthorizationUtils.ts (3)
5-19: LGTM! Good refactoring to centralize map manipulation.The
addRolePermissionRulehelper effectively consolidates the logic for adding permissions to both the restricted and disabled maps, reducing code duplication.
21-27: LGTM! Clear public API for managing role permissions.The new methods
addRolePermissionWhiteListandaddRolePermissionDisabledListprovide a clear and consistent interface for managing both restricted (whitelisted) and disabled (blacklisted) permissions.
52-58: LGTM! Consistent permission checking methods.Both
isPermissionRestrictedForRoleandisPermissionDisabledForRolecorrectly leverage the centralizedcheckPermissionForRolehelper with appropriate modifiers to implement whitelist and blacklist semantics respectively.apps/meteor/client/views/root/hooks/loggedIn/useRestrictedRoles.ts (1)
20-20: Permission identifier is correct. The'access-permissions'string matches its_idin the permissions constant and is used consistently in server checks, client hooks, and tests.apps/meteor/client/views/admin/permissions/PermissionsPage.tsx (1)
65-65: LGTM! Tab behavior correctly prevents unauthorized navigation.The conditional
onClickhandler ensures users withoutaccess-permissionscannot activate the Permissions tab, complementing the existingdisabledstate.apps/meteor/client/views/admin/permissions/PermissionsTable/PermissionRow.tsx (2)
37-54: LGTM! Semantic renaming improves clarity and enables per-role permission checks.The refactoring from
_idtopermissionIdand role's_idtoroleIdmakes the code more readable and correctly propagatespermissionIdtoRoleCell, which is now required for the newisPermissionDisabledForRolecheck.
37-54: Runtime initialization confirmed. The call toAuthorizationUtils.addRolePermissionDisabledList('admin', ['access-permissions'])exists inapps/meteor/client/views/root/hooks/loggedIn/useRestrictedRoles.ts, so the disabled-permissions list is correctly initialized.apps/meteor/client/views/admin/permissions/PermissionsTable/RoleCell.tsx (1)
24-34: UI now honors per-role disabled permissions.
Combining the newisPermissionDisabledForRolecheck with the existing restricted/loading flags cleanly prevents the adminaccess-permissionscheckbox from being toggled. Looks solid.
gabriellsh
left a comment
gabriellsh
left a comment
There was a problem hiding this comment.
Summary from discussion in DM:
- It would be nice if instead of disabling change, we show a warning modal of some sort.
- We found it a little hard to justify all these changes for just one permission, specially since this will only be needed in the UI for now.
- It might make more sense to warn the user if he's removing the last role from the permission, or identify if HE will lose access to the page, instead of limiting the warning to admins. I could have this permission in another role and not on the admin role for example, so the same issue would exist in that case.
apps/meteor/client/views/admin/permissions/PermissionsTable/RoleCell.tsx
Outdated
Show resolved
Hide resolved
apps/meteor/client/views/admin/permissions/PermissionsTable/RoleCell.tsx
Outdated
Show resolved
Hide resolved
apps/meteor/client/views/root/hooks/loggedIn/useRestrictedRoles.ts
Outdated
Show resolved
Hide resolved
dougfabris
force-pushed
the
feat/permission-screen-checkbox
branch
from
ff70be8 to
8fe2acf
Compare
dougfabris
changed the title
access-permissions from being removed from adminaccess-permissions from last granted role
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/meteor/client/views/admin/permissions/PermissionsTable/RoleCell.tsx (1)
48-53: Add error handling to prevent stuck loading state.If
onChangethrows an error or the promise rejects,setLoading(false)will never be called, leaving the checkbox in a perpetually disabled loading state.Apply this diff to add error handling:
const handleConfirmChange = useEffectEvent(async () => { setLoading(true); - const result = await onChange(_id, granted); - setGranted(result); - setLoading(false); + try { + const result = await onChange(_id, granted); + setGranted(result); + } finally { + setLoading(false); + } });
🧹 Nitpick comments (4)
apps/meteor/app/authorization/lib/index.ts (1)
12-13: Consider addingas constfor stronger type safety.The
confirmationRequiredPermissionsconstant is currently inferred asstring[], making it mutable. Addingas constwould make it a readonly tuplereadonly ['access-permissions'], preventing accidental modifications and enabling more precise type checking in consuming code.Apply this diff:
-export const confirmationRequiredPermissions = ['access-permissions']; +export const confirmationRequiredPermissions = ['access-permissions'] as const;Additionally, consider adding a JSDoc comment to document the purpose of this constant:
+/** + * List of permissions that require user confirmation before removal from the last role. + * Prevents admins from accidentally locking themselves out of critical features. + */ export const confirmationRequiredPermissions = ['access-permissions'] as const;packages/i18n/src/locales/en.i18n.json (1)
2907-2907: Clarify warning copy and include permission placeholder for context“soft-lock” is jargon and “this page” is ambiguous. Suggest clearer wording and exposing the permission name (if available) to improve UX.
Apply:
- "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_role_in_permission_warning": "You are removing the last role with the '{{permission}}' permission. This will lock the Permissions page in the UI and require database access to revert. Do you want to continue?"If the UI doesn’t have the permission name handy, keep the structure but drop the placeholder:
- "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_role_in_permission_warning": "You are removing the last role with this permission. This will lock the Permissions page in the UI and require database access to revert. Do you want to continue?"
- Confirm RoleCell.tsx uses the exact key "Last_role_in_permission_warning".
- If you add the {{permission}} placeholder, ensure the component passes it.
- Consider adding this key to other locales or rely on fallback.
apps/meteor/client/views/admin/permissions/PermissionsTable/RoleCell.tsx (2)
38-42: Consider cleanup effect for modal on unmount.If the component unmounts while the modal is open (e.g., user navigates away), the modal should be cleaned up to prevent memory leaks or stale UI state.
Add a cleanup effect:
useEffect(() => { return () => setModal(null); }, [setModal]);
55-55: Remove unnecessary double negation.The double negation (
!!) is unnecessary sinceloadingis already a boolean andisRestrictedForRoleis used in a boolean context.-const isDisabled = !!loading || !!isRestrictedForRole; +const isDisabled = loading || isRestrictedForRole;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
.changeset/spicy-zebras-deliver.md(1 hunks)apps/meteor/app/authorization/lib/index.ts(1 hunks)apps/meteor/client/views/admin/permissions/PermissionsTable/RoleCell.tsx(2 hunks)packages/i18n/src/locales/en.i18n.json(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
apps/meteor/client/views/admin/permissions/PermissionsTable/RoleCell.tsx (2)
packages/ui-contexts/src/index.ts (1)
useSetModal(68-68)apps/meteor/app/authorization/lib/index.ts (2)
AuthorizationUtils(14-14)confirmationRequiredPermissions(12-12)
🔇 Additional comments (4)
apps/meteor/client/views/admin/permissions/PermissionsTable/RoleCell.tsx (4)
4-10: LGTM!The new imports are appropriate for the confirmation modal functionality.
GenericModalanduseSetModalenable the modal UI,useTranslationprovides i18n support, andconfirmationRequiredPermissionsdefines which permissions require confirmation.
29-29: Confirmation logic is correct for the use case.The
shouldDisplayConfirmationcheck correctly identifies the scenario where removing a permission from the last role should trigger a warning. The logic ensures the modal only appears when:
- The permission is critical (in
confirmationRequiredPermissions)- Only one role has the permission (
grantedRoles.length === 1)- The current role has the permission (
granted === true)This prevents admins from accidentally removing their ability to manage permissions.
28-28: Don’t addisPermissionRequiredForRolecheck: missing API
NoisPermissionRequiredForRoleexists inAuthorizationUtils; implement that method first if you need a permission-required check.Likely an incorrect or invalid review comment.
31-46: Verify checkbox change handling
• Ensure CheckBox’s onChange signature aligns with handleChange (no extra args passed)
• Confirm rapid clicks can’t open multiple modals or bypass the confirmation step
dougfabris
force-pushed
the
feat/permission-screen-checkbox
branch
from
7ad90fb to
7d116f4
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/meteor/client/views/admin/permissions/PermissionsTable/RoleCell.tsx (1)
32-47: Optional: Remove unnecessary return statement.The modal confirmation flow is well-implemented with proper danger styling and translation support. However, line 46 has an unnecessary
returnstatement sincehandleConfirmChangedoesn't return a meaningful value for this context.Consider this minor cleanup:
} - return handleConfirmChange(); + handleConfirmChange(); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
apps/meteor/client/views/admin/permissions/PermissionsTable/__snapshots__/PermissionsTable.spec.tsx.snapis excluded by!**/*.snap
📒 Files selected for processing (6)
.changeset/spicy-zebras-deliver.md(1 hunks)apps/meteor/client/views/admin/permissions/PermissionsTable/PermissionRow.tsx(1 hunks)apps/meteor/client/views/admin/permissions/PermissionsTable/PermissionsTable.spec.tsx(2 hunks)apps/meteor/client/views/admin/permissions/PermissionsTable/PermissionsTable.stories.tsx(0 hunks)apps/meteor/client/views/admin/permissions/PermissionsTable/PermissionsTable.tsx(1 hunks)apps/meteor/client/views/admin/permissions/PermissionsTable/RoleCell.tsx(2 hunks)
💤 Files with no reviewable changes (1)
- apps/meteor/client/views/admin/permissions/PermissionsTable/PermissionsTable.stories.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/meteor/client/views/admin/permissions/PermissionsTable/PermissionRow.tsx
🧰 Additional context used
🧬 Code graph analysis (2)
apps/meteor/client/views/admin/permissions/PermissionsTable/PermissionsTable.spec.tsx (1)
packages/mock-providers/src/index.ts (1)
mockAppRoot(3-3)
apps/meteor/client/views/admin/permissions/PermissionsTable/RoleCell.tsx (2)
packages/ui-contexts/src/index.ts (1)
useSetModal(68-68)apps/meteor/app/authorization/lib/index.ts (2)
AuthorizationUtils(14-14)confirmationRequiredPermissions(12-12)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🔇 Additional comments (11)
.changeset/spicy-zebras-deliver.md (1)
1-5: LGTM!The changeset is well-formatted and accurately describes the feature. The grammatical issue from the previous review has been successfully addressed.
apps/meteor/client/views/admin/permissions/PermissionsTable/PermissionsTable.tsx (1)
19-19: LGTM!Making
paginationPropsoptional improves component flexibility. The implementation already safely handles the undefined case with optional chaining (lines 81-82) and safe spreading (line 83).apps/meteor/client/views/admin/permissions/PermissionsTable/PermissionsTable.spec.tsx (4)
1-8: LGTM!The new imports are appropriate for the added tests. The testing utilities (
screen,userEvent) and types (IPermission,IRole) support the modal behavior verification tests.
27-50: LGTM!The test data is well-structured and appropriate for testing the modal behavior:
defaultPermissionscorrectly represents a permission with a single granted rolerolesarray includes all necessary fields with realistic values- Using a fixed date for
_updatedAtensures test stability
64-79: LGTM!The test correctly validates that no modal appears when the permission has multiple granted roles. Good use of
queryByRolefor asserting non-existence, which returnsnullinstead of throwing.
52-62: Approve modal display test; aria-label format is correct
The aria-label uses${permissionName} - ${description || name}, matching the test’sgetByRolequery, and the test follows accessible best practices.apps/meteor/client/views/admin/permissions/PermissionsTable/RoleCell.tsx (5)
4-5: LGTM!The new imports are appropriate for implementing the modal confirmation feature. All dependencies are from established libraries and internal modules.
Also applies to: 8-8, 10-10
19-19: LGTM!The new
permissionNameprop enables better accessibility and user-facing text. The type signature is appropriate.Also applies to: 23-23
30-30: LGTM!The
shouldDisplayConfirmationlogic correctly identifies when to show the warning modal. All three conditions are necessary to prevent administrators from removing the last role with access to permissions management.
57-57: LGTM!The
aria-labelconstruction is excellent for accessibility. The format (permissionName - description/name) provides clear context for screen reader users and matches the test expectations.Also applies to: 62-62
49-56: UtilityisPermissionRequiredForRolemissing. The method doesn’t exist inAuthorizationUtils; implement it or remove this check.Likely an incorrect or invalid review comment.
gabriellsh
added
the
stat: QA assured
dionisio-bot
bot
added
the
stat: ready to merge
Proposed changes (including videos or screenshots)
This PR introduces a helper to function to store some permission we wan't to disabled based on specific roles in order to prevent admins from removing themselves the permission to access the permission table.
Issue(s)
Steps to test or reproduce
Further comments
CORE-267
Summary by CodeRabbit