feat: add ABAC admin settings #37139
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 |
|
WalkthroughAdds an ABAC admin area: new route, page, tabs, settings UI (toggle + warning modal), upsell modal test updates and mocks, sidebar entry, server setting description, i18n keys, test data update, and Storybook stories. New components, tests, route wiring, and translations are introduced. (50 words) Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User
participant Router
participant AdminRoute as AdminABACRoute
participant ModalMgr as ModalManager
participant Page as AdminABACPage
participant Settings as AdminABACSettings
participant Toggle as AdminABACSettingToggle
User->>Router: Navigate to /admin/ABAC
Router->>AdminRoute: mount
AdminRoute->>AdminRoute: check permissions & useHasLicenseModule('abac')
alt missing tab
AdminRoute->>Router: redirect -> /admin/ABAC/settings
end
alt upsell needed (no license/module)
AdminRoute->>ModalMgr: schedule/show upsell modal
ModalMgr->>User: display ABACUpsellModal
AdminRoute->>Page: render PageSkeleton while modal open
else license/module available
AdminRoute->>Page: render AdminABACPage(shouldShowWarning)
Page->>Settings: mount AdminABACSettings
Settings->>Toggle: render with hasABAC
User->>Toggle: toggle off or click reset
Toggle->>User: show AdminABACWarningModal (confirm/cancel)
alt confirmed
Toggle->>Settings: dispatch setting update
else cancelled
Toggle->>Toggle: revert UI state
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ 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 with no reviewable 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 @@
## feat/abac #37139 +/- ##
=============================================
- Coverage 72.18% 68.17% -4.01%
=============================================
Files 1502 3368 +1866
Lines 77602 116067 +38465
Branches 10979 20918 +9939
=============================================
+ Hits 56015 79130 +23115
- Misses 21182 34256 +13074
- Partials 405 2681 +2276
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
MartinSchoeler
force-pushed
the
feat/ABAC-admin-settings
branch
from
89e3585 to
60730f2
Compare
MartinSchoeler
force-pushed
the
feat/ABAC-admin-settings
branch
2 times, most recently
from
52e0188 to
5cd184d
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (12)
apps/meteor/client/views/admin/ABAC/AdminABACTabs.tsx (1)
5-22: Consider extracting hardcoded route name.The component structure is clean and follows established patterns. However, the hardcoded route name
'admin-ABAC'on line 11 could be extracted as a constant if referenced elsewhere in the ABAC admin flow.Example refactor:
+const ADMIN_ABAC_ROUTE = 'admin-ABAC'; + const AdminABACTabs = () => { const { t } = useTranslation(); const router = useRouter(); const tab = useRouteParameter('tab'); const handleTabClick = (tab: string) => { router.navigate({ - name: 'admin-ABAC', + name: ADMIN_ABAC_ROUTE, params: { tab }, }); };apps/meteor/client/views/admin/ABAC/AdminABACSettingToggle.spec.tsx (3)
53-56: Address TODO: Modal cleanup between tests.The TODO comments indicate manual modal cleanup is required between tests. This suggests a gap in test infrastructure.
Consider implementing a global
afterEachhook to clean up modals automatically:afterEach(() => { // Clean up any open modals const modals = document.querySelectorAll('[role="dialog"]'); modals.forEach(modal => modal.remove()); });Would you like me to generate a more comprehensive modal cleanup solution or open an issue to track this infrastructure improvement?
Also applies to: 84-86
97-108: Test assertion could be more robust.The test verifies toggle state changes but doesn't confirm that the underlying setting dispatch was called or that the warning modal flow was bypassed (since enabling ABAC shouldn't show a modal).
Consider adding assertions to verify the expected side effects:
// Mock the dispatch to verify it was called const mockDispatch = jest.fn(); // ... in test setup await user.click(toggle); expect(toggle).toBeChecked(); expect(mockDispatch).toHaveBeenCalledWith([{ _id: 'ABAC_Enabled', value: true }]); expect(screen.queryByText('Disable ABAC')).not.toBeInTheDocument();
103-103: Avoid implicit busy state check.The
busy: falsequery parameter might be fragile if the component's busy state implementation changes.Consider using
waitForwith a more explicit assertion:-const toggle = await screen.findByRole('checkbox', { busy: false }); +const toggle = screen.getByRole('checkbox'); +await waitFor(() => expect(toggle).not.toBeDisabled());apps/meteor/client/views/admin/ABAC/AdminABACPage.tsx (1)
23-23: Hardcoded documentation URL should be externalized.The ABAC documentation URL is hardcoded in the button. If this URL changes, you'll need to update it in multiple places.
Consider moving the URL to a constant or configuration:
const ABAC_DOCS_URL = 'https://rocket.chat/docs/abac'; // Then use it: <Button icon='new-window' secondary onClick={() => learnMore(ABAC_DOCS_URL)}>This same URL appears again on line 33, reinforcing the need for a single source of truth.
apps/meteor/client/views/admin/ABAC/AdminABACSettingToggle.tsx (3)
22-26: State synchronization could cause issues.The component maintains local state that syncs with the setting via
useEffect. This pattern can lead to stale closures and race conditions if the setting updates frequently.Consider deriving the value directly from the setting instead of maintaining separate state:
-const [value, setValue] = useState<boolean>(setting?.value === true); - -useEffect(() => { - setValue(setting?.value === true); -}, [setting]); +const value = setting?.value === true;Then update the handlers to work without
setValue:const handleChange = (value: boolean, setting: EditableSetting) => { - setValue(value); dispatch([{ _id: setting._id, value }]); };This eliminates the sync issue and simplifies the component.
28-53: Handler logic is correct but verbose.The
onchangehandler correctly shows the warning modal only when disabling ABAC. However, the nestedhandleChangefunction could be simplified.Consider extracting the change logic:
const handleChange = useCallback((value: boolean) => { if (!setting) return; dispatch([{ _id: setting._id, value }]); }, [dispatch, setting]); const onchange = useCallback((value: boolean) => { if (!setting) return; if (value === false) { setModal( <AdminABACWarningModal onConfirm={() => { handleChange(value); setModal(); }} onCancel={() => setModal()} /> ); return; } handleChange(value); }, [handleChange, setModal, setting]);
84-87: Translation handling could be more defensive.The
i18nDescriptionfield might be missing, and passing an empty string to the translation function could cause issues.Consider:
-hint={t(setting.i18nDescription || '')} +hint={setting.i18nDescription ? t(setting.i18nDescription) : undefined}apps/meteor/client/views/admin/ABAC/AdminABACWarningModal.tsx (1)
27-28: Confirm text styling approach needs review.The TODO comment questions whether this is the best approach for styling the confirm text. Using
Palette.text['font-danger'].toString()works but feels indirect.Consider using Fuselage's built-in styling or a semantic prop if available:
// Option 1: If GenericModal supports a danger variant confirmText={t('ABAC_Warning_Modal_Confirm_Text')} variant='danger' // Option 2: Use a semantic Fuselage component confirmText={<Button danger>{t('ABAC_Warning_Modal_Confirm_Text')}</Button>}Check Fuselage documentation for recommended patterns.
apps/meteor/client/views/admin/ABAC/AdminABACSettingToggle.stories.tsx (1)
49-65: Story coverage could be expanded.While the existing stories cover basic states, consider adding stories for:
- Reset button visible state (when value differs from packageValue)
- Setting blocked state
- Warning modal interaction flow
Example additional story:
export const WithReset: Story = { decorators: [ (Story) => { const AppRoot = mockAppRoot() .wrap((children) => <EditableSettingsProvider>{children}</EditableSettingsProvider>) .withTranslations('en', 'core', { /* ... */ }) .withSetting('ABAC_Enabled', true, { packageValue: false, // Different from value // ... }) .build(); return <AppRoot><Story /></AppRoot>; }, ], args: { hasABAC: true, }, };apps/meteor/client/views/admin/ABAC/AdminABACRoute.tsx (2)
10-11: Remove the commented-out import.The commented import on line 10 is redundant as the same import is active on line 11.
Apply this diff to remove the dead code:
-// import { useHasLicenseModule } from '../../../hooks/useHasLicenseModule'; import { useHasLicenseModule } from '../../../hooks/useHasLicenseModule';
39-44: Remove unusedtdependency from useEffect.The
tfunction fromuseTranslation()is included in the dependency array but is not used within the effect body.Apply this diff to remove the unused dependency:
useEffect(() => { // WS has never activated ABAC if (shouldShowUpsell && ABACEnabledSetting === undefined) { setModal(<ABACUpsellModal onClose={() => setModal(null)} onConfirm={handleManageSubscription} />); } - }, [shouldShowUpsell, setModal, t, handleManageSubscription, ABACEnabledSetting]); + }, [shouldShowUpsell, setModal, handleManageSubscription, ABACEnabledSetting]);
📜 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/ABAC/__snapshots__/AdminABACSettingToggle.spec.tsx.snapis excluded by!**/*.snap
📒 Files selected for processing (15)
apps/meteor/client/components/ABAC/ABACUpsellModal/ABACUpsellModal.spec.tsx(2 hunks)apps/meteor/client/views/admin/ABAC/AdminABACPage.tsx(1 hunks)apps/meteor/client/views/admin/ABAC/AdminABACRoute.tsx(1 hunks)apps/meteor/client/views/admin/ABAC/AdminABACSettingToggle.spec.tsx(1 hunks)apps/meteor/client/views/admin/ABAC/AdminABACSettingToggle.stories.tsx(1 hunks)apps/meteor/client/views/admin/ABAC/AdminABACSettingToggle.tsx(1 hunks)apps/meteor/client/views/admin/ABAC/AdminABACSettings.tsx(1 hunks)apps/meteor/client/views/admin/ABAC/AdminABACTabs.tsx(1 hunks)apps/meteor/client/views/admin/ABAC/AdminABACWarningModal.tsx(1 hunks)apps/meteor/client/views/admin/routes.tsx(2 hunks)apps/meteor/client/views/admin/sidebarItems.ts(1 hunks)apps/meteor/ee/server/settings/abac.ts(1 hunks)apps/meteor/tests/mocks/data.ts(1 hunks)packages/i18n/src/locales/en.i18n.json(1 hunks)packages/mock-providers/src/MockedAppRootBuilder.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (7)
apps/meteor/client/components/ABAC/ABACUpsellModal/ABACUpsellModal.spec.tsx (1)
apps/meteor/tests/mocks/data.ts (1)
createFakeLicenseInfo(213-261)
apps/meteor/client/views/admin/ABAC/AdminABACSettingToggle.spec.tsx (1)
packages/mock-providers/src/index.ts (1)
mockAppRoot(3-3)
apps/meteor/client/views/admin/ABAC/AdminABACRoute.tsx (1)
packages/ui-contexts/src/index.ts (6)
usePermission(55-55)useCurrentModal(29-29)useRouteParameter(62-62)useRouter(60-60)useSettingStructure(73-73)useSetModal(68-68)
apps/meteor/client/views/admin/ABAC/AdminABACSettingToggle.stories.tsx (1)
packages/mock-providers/src/index.ts (1)
mockAppRoot(3-3)
apps/meteor/client/views/admin/ABAC/AdminABACTabs.tsx (1)
packages/ui-contexts/src/index.ts (2)
useRouter(60-60)useRouteParameter(62-62)
apps/meteor/client/views/admin/ABAC/AdminABACSettingToggle.tsx (2)
apps/meteor/client/views/admin/EditableSettingsContext.ts (2)
useEditableSetting(81-85)EditableSetting(7-11)packages/ui-contexts/src/index.ts (2)
useSetModal(68-68)useSettingsDispatch(71-71)
apps/meteor/client/views/admin/ABAC/AdminABACWarningModal.tsx (1)
packages/ui-contexts/src/index.ts (1)
useRouter(60-60)
🪛 Biome (2.1.2)
apps/meteor/client/views/admin/ABAC/AdminABACSettings.tsx
[error] 19-19: Avoid using target="_blank" without rel="noopener" or rel="noreferrer".
Opening external links in new tabs without rel="noopener" is a security risk. See the explanation for more details.
Safe fix: Add the rel="noopener" attribute.
(lint/security/noBlankTarget)
apps/meteor/client/views/admin/ABAC/AdminABACPage.tsx
[error] 33-33: Avoid using target="_blank" without rel="noopener" or rel="noreferrer".
Opening external links in new tabs without rel="noopener" is a security risk. See the explanation for more details.
Safe fix: Add the rel="noopener" attribute.
(lint/security/noBlankTarget)
🔇 Additional comments (18)
apps/meteor/tests/mocks/data.ts (1)
236-236: LGTM!The addition of 'abac' to the mock license modules appropriately supports ABAC-related test scenarios.
apps/meteor/ee/server/settings/abac.ts (1)
16-16: LGTM!Adding
i18nDescriptionmetadata enables the UI to display a localized description for the ABAC_Enabled setting, improving user experience.packages/mock-providers/src/MockedAppRootBuilder.tsx (1)
471-476: LGTM!The addition of an optional
settingStructureparameter cleanly extends thewithSettingAPI while maintaining backward compatibility. This enables tests to pre-populate additionalISettingfields as needed for ABAC and other scenarios.apps/meteor/client/components/ABAC/ABACUpsellModal/ABACUpsellModal.spec.tsx (2)
7-7: LGTM!Switching from mocking the
useHasLicenseModulehook to providing license info via a mocked endpoint creates a more realistic and maintainable test setup. This data-driven approach better simulates the actual runtime behavior.Also applies to: 33-35
18-21: LGTM!The
getURLutility mock appropriately passes through URLs unchanged, simplifying test setup for components that construct URLs.apps/meteor/client/views/admin/routes.tsx (1)
111-114: LGTM!The ABAC route registration follows the established pattern used by other admin routes, with appropriate lazy loading and optional parameters for tab, context, and id navigation.
Also applies to: 249-252
apps/meteor/client/views/admin/ABAC/AdminABACSettings.tsx (1)
16-19: Replace placeholder documentation URL.The TODO comment on line 16 and the hardcoded
'https://rocket.chat'URL indicate this is placeholder content. Please replace with the actual ABAC documentation URL before merging.Do you want me to help locate the correct documentation URL, or would you like to open an issue to track this task?
apps/meteor/client/views/admin/ABAC/AdminABACSettingToggle.spec.tsx (2)
1-17: Test setup is well-structured.The test file properly imports necessary testing utilities and defines a reusable
settingStructuremock andbaseAppRootwrapper. This approach promotes DRY principles and makes tests maintainable.
30-35: Verify snapshot testing strategy.Snapshot tests can be brittle and may not catch meaningful regressions. Consider whether snapshot testing adds value here or if targeted assertions would be more maintainable.
Additionally, the snapshot captures
baseElement(the entire DOM tree including portals), which might include unrelated elements. Consider usingcontainerinstead if you only want to snapshot the component itself.apps/meteor/client/views/admin/ABAC/AdminABACPage.tsx (2)
30-30: Address TODO: Documentation URL.The TODO indicates the documentation URL needs to be confirmed. Ensure this is resolved before merging.
Please verify the correct documentation URL for ABAC capabilities and remove the TODO comment once confirmed.
14-46: Component structure is clean and well-organized.The component properly uses hooks, handles conditional rendering appropriately, and follows React best practices. The page layout with header, optional warning, tabs, and content is logical.
apps/meteor/client/views/admin/ABAC/AdminABACSettingToggle.tsx (1)
76-78: Loading state handling is appropriate.The component correctly renders a skeleton during loading, providing good UX.
apps/meteor/client/views/admin/ABAC/AdminABACWarningModal.tsx (1)
23-45: Modal structure and callbacks are correctly wired.The modal properly handles all interaction callbacks (onConfirm, onCancel, onClose, onDismiss) consistently, ensuring good UX regardless of how the user dismisses the modal.
apps/meteor/client/views/admin/ABAC/AdminABACSettingToggle.stories.tsx (1)
7-44: Storybook configuration is well-structured.The stories properly set up the necessary context (EditableSettingsProvider, translations, settings) and provide good coverage of the component's states (enabled, loading, disabled). The decorator pattern is appropriate for this use case.
apps/meteor/client/views/admin/ABAC/AdminABACRoute.tsx (4)
50-52: LGTM: Authorization logic is well-structured.The access control correctly restricts the page to users with permission and ensures that either the ABAC setting exists or a valid license is present. This prevents unauthorized access when the feature hasn't been activated and no license is available.
54-60: LGTM: Provider structure and warning condition are appropriate.The component correctly wraps the page in the necessary providers and passes
shouldShowWarningwhen the ABAC setting exists but the license is not available, which appropriately alerts users who may have lost access to the feature.
63-63: LGTM: Appropriate use of memo for performance.Wrapping the route component with
memois a good optimization to prevent unnecessary re-renders.
18-19: Confirm ABAC admin page permission
The TODO indicates uncertainty about whethermanage-cloudis the correct permission. Remove the TODO if it’s accurate, or replace it with the exact permission name required for ABAC access.
MartinSchoeler
force-pushed
the
feat/ABAC-admin-settings
branch
from
25b6f00 to
998da9b
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
apps/meteor/client/views/admin/sidebarItems.ts (1)
73-78: Gate ABAC sidebar item by license as well as permissionGood add on manage-cloud. Also guard by ABAC license so non-licensed workspaces don’t see the entry.
{ href: '/admin/ABAC', i18nLabel: 'ABAC', icon: 'team-lock', - permissionGranted: (): boolean => hasPermission('manage-cloud'), + permissionGranted: (): boolean => + hasPermission('manage-cloud') && hasLicenseModule?.('abac') === true, },Replace hasLicenseModule with your actual license check util used elsewhere in client.
Run to locate the license check helper in client code:
#!/bin/bash rg -nP -C3 --type=ts --type=tsx '\b(hasLicense|hasLicenseModule|useHasLicense|useHasLicenseModule)\b'apps/meteor/client/views/admin/ABAC/AdminABACWarningModal.tsx (2)
14-21: Close modal before navigatingCall onCancel first to ensure the modal is dismissed.
const handleNavigate = () => { + onCancel(); router.navigate({ name: 'admin-ABAC', params: { tab: 'rooms', }, }); };
38-41: Use an accessible interactive element and fix stray apostropheBox as anchor without href isn’t keyboard-accessible; use a semantic button styled as a link, and remove the trailing apostrophe.
- <Box is='a' onClick={handleNavigate}> - {' '} - ABAC {'>'} Rooms' - </Box> + <Box + is='button' + type='button' + onClick={handleNavigate} + aria-label='Go to ABAC Rooms' + style={{ background: 'none', border: 0, padding: 0, color: 'inherit', textDecoration: 'underline', cursor: 'pointer' }} +> + {' '}ABAC {'>'} Rooms + </Box>apps/meteor/client/views/admin/ABAC/AdminABACSettingToggle.tsx (1)
55-70: Show warning only when reset disables ABACReset currently always opens the warning modal. Only warn when packageValue is false; otherwise apply reset immediately.
const onReset = useCallback(() => { if (!setting) { return; } - const value = setting.packageValue as boolean; - setModal( - <AdminABACWarningModal - onConfirm={() => { - setValue(value); - dispatch([{ _id: setting._id, value }]); - setModal(); - }} - onCancel={() => setModal()} - />, - ); + const newValue = setting.packageValue as boolean; + const apply = () => { + setValue(newValue); + dispatch([{ _id: setting._id, value: newValue }]); + setModal(); + }; + if (newValue === false) { + setModal(<AdminABACWarningModal onConfirm={apply} onCancel={() => setModal()} />); + } else { + apply(); + } }, [dispatch, setModal, setting]);
🧹 Nitpick comments (5)
apps/meteor/client/views/admin/ABAC/AdminABACTabs.tsx (1)
9-14: Avoid shadowing “tab” variable for clarityRename the handler parameter to avoid shadowing the route param.
- const handleTabClick = (tab: string) => { + const handleTabClick = (nextTab: string) => { router.navigate({ name: 'admin-ABAC', - params: { tab }, + params: { tab: nextTab }, }); };apps/meteor/client/views/admin/ABAC/AdminABACSettingToggle.spec.tsx (1)
53-56: Clean up modals after each test to avoid cross-test leakageAdd a global cleanup so portals/modals don’t persist between tests.
import { cleanup } from '@testing-library/react'; afterEach(() => { cleanup(); });apps/meteor/client/views/admin/ABAC/AdminABACWarningModal.tsx (1)
27-29: Use GenericModal’s danger variant instead of inline red text — addvariant="danger"to the<GenericModal>and simplifyconfirmTextto{t('ABAC_Warning_Modal_Confirm_Text')}, removing the inline<Box>and the TODO.apps/meteor/client/views/admin/ABAC/AdminABACRoute.tsx (2)
18-19: Verify the permission requirement.The TODO comment indicates uncertainty about the correct permission for viewing the ABAC page. Currently using
'manage-cloud', but this should be confirmed.Please verify with the team whether
'manage-cloud'is the appropriate permission for ABAC administration. If this needs to be tracked long-term, would you like me to open an issue for it?
41-46: Remove unused dependency from useEffect.The
tdependency is not used within the effect body and should be removed from the dependency array to avoid unnecessary re-renders.Apply this diff:
- }, [shouldShowUpsell, setModal, t, handleManageSubscription, ABACEnabledSetting]); + }, [shouldShowUpsell, setModal, handleManageSubscription, ABACEnabledSetting]);
📜 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/ABAC/__snapshots__/AdminABACSettingToggle.spec.tsx.snapis excluded by!**/*.snap
📒 Files selected for processing (15)
apps/meteor/client/components/ABAC/ABACUpsellModal/ABACUpsellModal.spec.tsx(2 hunks)apps/meteor/client/views/admin/ABAC/AdminABACPage.tsx(1 hunks)apps/meteor/client/views/admin/ABAC/AdminABACRoute.tsx(1 hunks)apps/meteor/client/views/admin/ABAC/AdminABACSettingToggle.spec.tsx(1 hunks)apps/meteor/client/views/admin/ABAC/AdminABACSettingToggle.stories.tsx(1 hunks)apps/meteor/client/views/admin/ABAC/AdminABACSettingToggle.tsx(1 hunks)apps/meteor/client/views/admin/ABAC/AdminABACSettings.tsx(1 hunks)apps/meteor/client/views/admin/ABAC/AdminABACTabs.tsx(1 hunks)apps/meteor/client/views/admin/ABAC/AdminABACWarningModal.tsx(1 hunks)apps/meteor/client/views/admin/routes.tsx(2 hunks)apps/meteor/client/views/admin/sidebarItems.ts(1 hunks)apps/meteor/ee/server/settings/abac.ts(1 hunks)apps/meteor/tests/mocks/data.ts(1 hunks)packages/i18n/src/locales/en.i18n.json(1 hunks)packages/mock-providers/src/MockedAppRootBuilder.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- apps/meteor/ee/server/settings/abac.ts
- apps/meteor/client/views/admin/ABAC/AdminABACSettings.tsx
- apps/meteor/client/components/ABAC/ABACUpsellModal/ABACUpsellModal.spec.tsx
- packages/mock-providers/src/MockedAppRootBuilder.tsx
- apps/meteor/client/views/admin/ABAC/AdminABACSettingToggle.stories.tsx
🧰 Additional context used
🧬 Code graph analysis (5)
apps/meteor/client/views/admin/ABAC/AdminABACWarningModal.tsx (1)
packages/ui-contexts/src/index.ts (1)
useRouter(60-60)
apps/meteor/client/views/admin/ABAC/AdminABACRoute.tsx (1)
packages/ui-contexts/src/index.ts (6)
usePermission(55-55)useCurrentModal(29-29)useRouteParameter(62-62)useRouter(60-60)useSettingStructure(73-73)useSetModal(68-68)
apps/meteor/client/views/admin/ABAC/AdminABACTabs.tsx (1)
packages/ui-contexts/src/index.ts (2)
useRouter(60-60)useRouteParameter(62-62)
apps/meteor/client/views/admin/ABAC/AdminABACSettingToggle.spec.tsx (1)
packages/mock-providers/src/index.ts (1)
mockAppRoot(3-3)
apps/meteor/client/views/admin/ABAC/AdminABACSettingToggle.tsx (2)
apps/meteor/client/views/admin/EditableSettingsContext.ts (2)
useEditableSetting(81-85)EditableSetting(7-11)packages/ui-contexts/src/index.ts (2)
useSetModal(68-68)useSettingsDispatch(71-71)
⏰ 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 (4)
apps/meteor/tests/mocks/data.ts (1)
236-236: Approve addition of ‘abac’ to mock license modules
Verified that ‘abac’ is the correct production module identifier (used in license typings, services, and checks); the mock update is accurate.apps/meteor/client/views/admin/routes.tsx (1)
111-114: LGTM: route typing and registration look consistentadmin-ABAC typing and registerAdminRoute align with the sidebar path and navigation.
Also applies to: 249-252
apps/meteor/client/views/admin/ABAC/AdminABACPage.tsx (2)
33-37: LGTM: external link hardenedAnchor includes rel="noopener noreferrer" with target="_blank".
23-25:useExternalLinkalready appliesnoopener noreferrerwhen opening external links.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/meteor/client/views/admin/ABAC/AdminABACRoute.tsx (1)
16-16: Remove unused translation function from dependencies.The
tfunction fromuseTranslation()is declared on line 16 and included in theuseEffectdependencies on line 45, but it's never actually used in the component. This creates an unnecessary dependency.Apply this diff to remove the unused import and dependency:
- const { t } = useTranslation(); // TODO: Check what permission is needed to view the ABAC page const canViewABACPage = usePermission('manage-cloud');- }, [shouldShowUpsell, setModal, t, handleManageSubscription, ABACEnabledSetting]); + }, [shouldShowUpsell, setModal, handleManageSubscription, ABACEnabledSetting]);Also applies to: 45-45
📜 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 (1)
apps/meteor/client/views/admin/ABAC/AdminABACRoute.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
apps/meteor/client/views/admin/ABAC/AdminABACRoute.tsx (1)
packages/ui-contexts/src/index.ts (6)
usePermission(55-55)useCurrentModal(29-29)useRouteParameter(62-62)useRouter(60-60)useSettingStructure(73-73)useSetModal(68-68)
⏰ 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). (3)
- GitHub Check: 📦 Build Packages
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🔇 Additional comments (5)
apps/meteor/client/views/admin/ABAC/AdminABACRoute.tsx (5)
17-18: Verify the ABAC page permission requirement.The TODO comment indicates that the permission for viewing the ABAC page needs verification. Currently using
manage-cloud, but this should be confirmed as the correct permission for ABAC administrative access.Consider reviewing with the security/permissions team to ensure
manage-cloudis the appropriate permission for ABAC administration, or if a more specific ABAC-related permission should be created.
27-34: LGTM! Proper use of useLayoutEffect for navigation.The navigation redirect is correctly implemented using
useLayoutEffectto avoid flickering and properly prevent side effects during render. The dependencies[tab, router]are appropriate.
40-45: Upsell modal logic is sound.The conditional display of the upsell modal when
shouldShowUpsell && ABACEnabledSetting === undefinedcorrectly handles the case where the workspace has never activated ABAC.
64-64: LGTM! Appropriate use of memoization.Wrapping the component with
memois a good practice for this route component to prevent unnecessary re-renders.
25-25: No action needed:useSettingStructurereturnsundefined(nevernull), so existing=== undefinedchecks are correct.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
apps/meteor/client/views/admin/ABAC/AdminABACWarningModal.tsx (2)
14-21: Navigation handler doesn't close the modal.The
handleNavigatefunction navigates to a different route but doesn't close the modal first. This could leave the modal open on the new page, creating a poor user experience.Consider calling
onCancelbefore navigation:const handleNavigate = () => { + onCancel(); router.navigate({ name: 'admin-ABAC', params: { tab: 'rooms', }, }); };
38-41: Link accessibility concern.Using
Box is='a'withonClickcreates an element that looks like a link but isn't keyboard accessible or screen reader friendly. Users who navigate by keyboard won't be able to activate this element.Use a semantic button styled as a link or Fuselage's Link component:
-<Box is='a' onClick={handleNavigate}> +<Box is='button' type='button' onClick={handleNavigate} style={{ background: 'none', border: 'none', padding: 0, color: 'inherit', textDecoration: 'underline', cursor: 'pointer' }}> {' '} ABAC {'>'} Rooms' </Box>
🧹 Nitpick comments (1)
apps/meteor/client/views/admin/ABAC/AdminABACWarningModal.tsx (1)
1-1: Remove unused import.
Paletteis imported but never used in this file.Apply this diff:
-import { Box, Palette } from '@rocket.chat/fuselage'; +import { Box } from '@rocket.chat/fuselage';
📜 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 (1)
apps/meteor/client/views/admin/ABAC/AdminABACWarningModal.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
apps/meteor/client/views/admin/ABAC/AdminABACWarningModal.tsx (1)
packages/ui-contexts/src/index.ts (1)
useRouter(60-60)
⏰ 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). (7)
- GitHub Check: 🔨 Test Storybook / Test Storybook
- GitHub Check: 🔨 Test Unit / Unit Tests
- GitHub Check: 🔎 Code Check / Code Lint
- GitHub Check: 🔎 Code Check / TypeScript
- GitHub Check: 📦 Meteor Build - coverage
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🔇 Additional comments (3)
apps/meteor/client/views/admin/ABAC/AdminABACWarningModal.tsx (3)
6-9: LGTM!The type definition is clear and appropriate for the modal's callback props.
24-33: LGTM!The modal configuration is appropriate. The
ts-expect-erroris documented as temporary pending PR #37223. UsingonCancelfor all dismiss actions (onCancel,onClose,onDismiss) ensures consistent behavior regardless of how the modal is dismissed.
40-40: Verify the trailing apostrophe.The text
ABAC {'>'} Rooms'ends with an apostrophe. Confirm whether this is intentional (possessive form) or a typo. If it's meant to be "Rooms" without the apostrophe, this should be corrected in the translation string.Please verify the intended text format with the design or content team.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/meteor/client/views/admin/ABAC/AdminABACWarningModal.tsx (1)
48-48: Consider using named export.While default exports work fine, named exports are generally preferred in modern React codebases for better IDE support, easier refactoring, and improved tree-shaking.
-export default AdminABACWarningModal; +export { AdminABACWarningModal };Then update imports in consuming files accordingly:
import { AdminABACWarningModal } from './AdminABACWarningModal';
📜 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 (1)
apps/meteor/client/views/admin/ABAC/AdminABACWarningModal.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
apps/meteor/client/views/admin/ABAC/AdminABACWarningModal.tsx (1)
packages/ui-contexts/src/index.ts (1)
useRouter(60-60)
⏰ 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). (3)
- GitHub Check: 📦 Build Packages
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🔇 Additional comments (3)
apps/meteor/client/views/admin/ABAC/AdminABACWarningModal.tsx (3)
1-9: LGTM: Clean imports and type definition.The imports are appropriate for the component's functionality, using the correct Fuselage, UI client, context, and i18n packages. The
AdminABACWarningModalPropstype is clear and straightforward, defining the required callback props.
11-13: Component structure and modal setup look good.The component correctly uses hooks (
useTranslation,useRouter) and properly wires the modal's event handlers to the provided callbacks. The GenericModal receives appropriate props for title, confirmText, cancelText, and all dismissal handlers.Note: The
ts-expect-erroron lines 26-27 for thevariantprop is temporary technical debt that's tracked in PR #37223.Also applies to: 23-34
35-43: Trans component usage is correct.The
Transcomponent from react-i18next is used appropriately to handle i18n with an embedded interactive element. This is the standard pattern for translations containing dynamic components.However, the accessibility issue with the
Box is='a'element noted in previous comments remains valid and should be addressed.
MartinSchoeler
force-pushed
the
feat/ABAC-admin-settings
branch
from
9db5d74 to
8073b71
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
apps/meteor/client/views/admin/ABAC/AdminABACWarningModal.tsx (2)
38-41: Replace non-interactive Box with accessible element.Using
Box is='a'withonClickcreates an element that appears interactive but lacks keyboard accessibility and screen reader support. This creates an accessibility barrier.Replace with a semantic button styled as a link:
-<Box is='a' onClick={handleNavigate}> +<Box is='button' type='button' onClick={handleNavigate} style={{ background: 'none', border: 'none', padding: 0, color: 'inherit', textDecoration: 'underline', cursor: 'pointer' }}> {' '} ABAC {'>'} Rooms' </Box>Or better yet, use Fuselage's Button or Link component if available with the appropriate styling.
14-21: Close modal before navigation.The
handleNavigatefunction navigates to a new route but doesn't close the modal first, which could leave the modal open on the new page.Apply this diff to close the modal before navigating:
const handleNavigate = () => { + onCancel(); router.navigate({ name: 'admin-ABAC', params: { tab: 'rooms', }, }); };apps/meteor/client/views/admin/ABAC/AdminABACSettingToggle.tsx (1)
55-70: Fix reset handler to only warn when disabling ABAC.The
onResetcallback always shows the warning modal, even when resetting totrue(enabled). This is inconsistent with theonChangelogic at line 39, which only shows the warning when disabling ABAC.Apply this diff to align reset behavior with toggle behavior:
const onReset = useCallback(() => { if (!setting) { return; } const value = setting.packageValue as boolean; + + const applyReset = () => { + setValue(value); + dispatch([{ _id: setting._id, value }]); + setModal(); + }; + + if (value === false) { - setModal( - <AdminABACWarningModal - onConfirm={() => { - setValue(value); - dispatch([{ _id: setting._id, value }]); - setModal(); - }} - onCancel={() => setModal()} - />, - ); + setModal( + <AdminABACWarningModal + onConfirm={applyReset} + onCancel={() => setModal()} + /> + ); + } else { + applyReset(); + } }, [dispatch, setModal, setting]);
📜 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/ABAC/__snapshots__/AdminABACSettingToggle.spec.tsx.snapis excluded by!**/*.snap
📒 Files selected for processing (15)
apps/meteor/client/components/ABAC/ABACUpsellModal/ABACUpsellModal.spec.tsx(2 hunks)apps/meteor/client/views/admin/ABAC/AdminABACPage.tsx(1 hunks)apps/meteor/client/views/admin/ABAC/AdminABACRoute.tsx(1 hunks)apps/meteor/client/views/admin/ABAC/AdminABACSettingToggle.spec.tsx(1 hunks)apps/meteor/client/views/admin/ABAC/AdminABACSettingToggle.stories.tsx(1 hunks)apps/meteor/client/views/admin/ABAC/AdminABACSettingToggle.tsx(1 hunks)apps/meteor/client/views/admin/ABAC/AdminABACSettings.tsx(1 hunks)apps/meteor/client/views/admin/ABAC/AdminABACTabs.tsx(1 hunks)apps/meteor/client/views/admin/ABAC/AdminABACWarningModal.tsx(1 hunks)apps/meteor/client/views/admin/routes.tsx(2 hunks)apps/meteor/client/views/admin/sidebarItems.ts(1 hunks)apps/meteor/ee/server/settings/abac.ts(1 hunks)apps/meteor/tests/mocks/data.ts(1 hunks)packages/http-router/src/Router.ts(1 hunks)packages/i18n/src/locales/en.i18n.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- apps/meteor/client/views/admin/ABAC/AdminABACSettingToggle.stories.tsx
- apps/meteor/client/views/admin/ABAC/AdminABACSettingToggle.spec.tsx
- apps/meteor/client/views/admin/sidebarItems.ts
- apps/meteor/client/components/ABAC/ABACUpsellModal/ABACUpsellModal.spec.tsx
- apps/meteor/tests/mocks/data.ts
🧰 Additional context used
🧬 Code graph analysis (4)
apps/meteor/client/views/admin/ABAC/AdminABACRoute.tsx (1)
packages/ui-contexts/src/index.ts (6)
usePermission(55-55)useCurrentModal(29-29)useRouteParameter(62-62)useRouter(60-60)useSettingStructure(73-73)useSetModal(68-68)
apps/meteor/client/views/admin/ABAC/AdminABACWarningModal.tsx (1)
packages/ui-contexts/src/index.ts (1)
useRouter(60-60)
apps/meteor/client/views/admin/ABAC/AdminABACTabs.tsx (1)
packages/ui-contexts/src/index.ts (2)
useRouter(60-60)useRouteParameter(62-62)
apps/meteor/client/views/admin/ABAC/AdminABACSettingToggle.tsx (2)
apps/meteor/client/views/admin/EditableSettingsContext.ts (2)
useEditableSetting(81-85)EditableSetting(7-11)packages/ui-contexts/src/index.ts (2)
useSetModal(68-68)useSettingsDispatch(71-71)
⏰ 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). (3)
- GitHub Check: 📦 Build Packages
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🔇 Additional comments (12)
packages/i18n/src/locales/en.i18n.json (1)
14-23: ABAC copy looks consistentString tweaks and new keys line up with the UI usage and placeholder style. 👍
apps/meteor/ee/server/settings/abac.ts (1)
16-16: LGTM! Good addition of localization support.Adding
i18nDescriptionenables the UI to display a localized description for the ABAC_Enabled setting, improving user experience and maintainability.apps/meteor/client/views/admin/ABAC/AdminABACTabs.tsx (1)
1-24: LGTM! Clean tab navigation component.The component correctly implements tab navigation using Fuselage components and routing hooks. The structure supports future tab additions while maintaining a clean, focused implementation.
apps/meteor/client/views/admin/routes.tsx (2)
111-114: LGTM! Type definition follows established patterns.The route type definition correctly specifies the pathname and pattern with optional parameters, consistent with other admin routes.
249-252: LGTM! Route registration is properly configured.The route registration follows the established pattern with lazy loading and correct path/name mapping.
apps/meteor/client/views/admin/ABAC/AdminABACSettings.tsx (1)
16-22: Verify documentation URL before release.The TODO comment and placeholder URL (
https://rocket.chat) should be replaced with the actual ABAC documentation URL before this feature is released to users.apps/meteor/client/views/admin/ABAC/AdminABACPage.tsx (1)
30-36: Verify documentation URL before release.Similar to the other component, the TODO comment and placeholder URL (
https://rocket.chat/docs/abac) should be replaced with the actual ABAC documentation URL before release.apps/meteor/client/views/admin/ABAC/AdminABACWarningModal.tsx (1)
26-27: LGTM! Temporary type suppression is acceptable.The
@ts-expect-errorwith a reference to PR #37223 is an acceptable temporary solution while waiting for the type definitions to be updated.apps/meteor/client/views/admin/ABAC/AdminABACRoute.tsx (3)
17-18: Verify the appropriate permission for ABAC page access.The TODO comment indicates uncertainty about the correct permission. The current
manage-cloudpermission should be verified with the team to ensure it aligns with ABAC's security model.
27-34: LGTM! Navigation side effect is properly handled.The
useLayoutEffectcorrectly handles the default tab navigation, addressing the previous review concern about side effects during render.
40-45: LGTM! Upsell modal logic is sound.The modal is conditionally shown when upsell is needed and the setting has never been activated, with all dependencies properly tracked.
apps/meteor/client/views/admin/ABAC/AdminABACSettingToggle.tsx (1)
28-53: LGTM! Toggle change logic correctly handles warnings.The
onChangecallback properly shows the warning modal only when disabling ABAC (value === false), providing appropriate user protection for potentially disruptive changes.
tassoevan
force-pushed
the
feat/ABAC-admin-settings
branch
from
8073b71 to
c271949
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/meteor/ee/server/settings/abac.ts (1)
11-17: Add missing i18nLabel for complete localization.The i18nDescription addition is correct and the i18n key exists. However, the setting is incomplete—all other EE settings use both
i18nLabel(for the setting name) andi18nDescription(for the description). This setting currently only has the description.Add
i18nLabel: 'ABAC_Enabled'alongside the existingi18nDescriptionto match the pattern used throughout the EE settings.
♻️ Duplicate comments (2)
apps/meteor/client/views/admin/ABAC/AdminABACWarningModal.tsx (2)
14-21: Navigation handler still doesn't close the modal.This issue was previously flagged but remains unresolved. The
handleNavigatefunction navigates to a different route without closing the modal first, which could leave the modal open on the new page.Apply this diff to close the modal before navigation:
const handleNavigate = () => { + onCancel(); router.navigate({ name: 'admin-ABAC', params: { tab: 'rooms', }, }); };
38-41: Link accessibility concern remains unaddressed.This issue was previously flagged but not fixed. Using
Box is='a'withonClickcreates an element that looks like a link but isn't keyboard accessible or screen reader friendly.Use a button styled as a link or Fuselage's Link component:
-<Box is='a' onClick={handleNavigate}> +<Box is='button' type='button' onClick={handleNavigate} style={{ background: 'none', border: 'none', color: 'inherit', textDecoration: 'underline', cursor: 'pointer', padding: 0 }}> {' '} ABAC {'>'} Rooms' </Box>
🧹 Nitpick comments (4)
apps/meteor/client/components/ABAC/ABACUpsellModal/ABACUpsellModal.spec.tsx (1)
9-16: Consider testing different upsell hook states.The
useUpsellActionsmock always returnsshouldShowUpsell: trueandcloudWorkspaceHadTrial: false. Consider adding tests that verify behavior when these values differ (e.g.,shouldShowUpsell: false,cloudWorkspaceHadTrial: true) to ensure comprehensive coverage of modal states.apps/meteor/client/views/admin/ABAC/AdminABACSettingToggle.spec.tsx (1)
53-53: Address modal cleanup in test teardown.The repeated TODO comments indicate that modals are not automatically unmounted between tests, requiring manual dismissal. This could lead to test interference or flakiness if not handled consistently.
Consider adding a cleanup utility in your test setup or using a custom render wrapper that ensures modals are unmounted after each test. Do you want me to suggest an implementation for automatic modal cleanup in your test infrastructure?
Also applies to: 84-84
apps/meteor/client/views/admin/ABAC/AdminABACRoute.tsx (2)
19-19: Consider explicit handling of license loading state.The strict equality
=== trueconverts the'loading'state fromuseHasLicenseModuletofalse. While this may be intentional, it could cause the authorization check on Line 51 to incorrectly showNotAuthorizedPageduring the loading phase, resulting in a brief flicker before the license state is resolved.Consider handling the loading state explicitly:
-const hasABAC = useHasLicenseModule('abac') === true; +const hasABACModule = useHasLicenseModule('abac'); +const hasABAC = hasABACModule === true; +const isLoadingLicense = hasABACModule === 'loading';Then update the authorization check:
+if (isLoadingLicense) { + return <PageSkeleton />; +} + if (!canViewABACPage || (ABACEnabledSetting === undefined && !hasABAC)) { return <NotAuthorizedPage />; }
51-53: Consider adding explanatory comment for authorization logic.The authorization condition combines permission and license checks in a non-obvious way. While the logic appears correct, a brief comment would help future maintainers understand the different access scenarios.
Consider adding a comment to clarify the authorization cases:
+// Deny access if: +// 1. User lacks permission, or +// 2. ABAC was never activated (setting undefined) AND user has no license (upsell already shown) if (!canViewABACPage || (ABACEnabledSetting === undefined && !hasABAC)) { return <NotAuthorizedPage />; }
📜 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/ABAC/__snapshots__/AdminABACSettingToggle.spec.tsx.snapis excluded by!**/*.snap
📒 Files selected for processing (14)
apps/meteor/client/components/ABAC/ABACUpsellModal/ABACUpsellModal.spec.tsx(2 hunks)apps/meteor/client/views/admin/ABAC/AdminABACPage.tsx(1 hunks)apps/meteor/client/views/admin/ABAC/AdminABACRoute.tsx(1 hunks)apps/meteor/client/views/admin/ABAC/AdminABACSettingToggle.spec.tsx(1 hunks)apps/meteor/client/views/admin/ABAC/AdminABACSettingToggle.stories.tsx(1 hunks)apps/meteor/client/views/admin/ABAC/AdminABACSettingToggle.tsx(1 hunks)apps/meteor/client/views/admin/ABAC/AdminABACSettings.tsx(1 hunks)apps/meteor/client/views/admin/ABAC/AdminABACTabs.tsx(1 hunks)apps/meteor/client/views/admin/ABAC/AdminABACWarningModal.tsx(1 hunks)apps/meteor/client/views/admin/routes.tsx(2 hunks)apps/meteor/client/views/admin/sidebarItems.ts(1 hunks)apps/meteor/ee/server/settings/abac.ts(1 hunks)apps/meteor/tests/mocks/data.ts(1 hunks)packages/i18n/src/locales/en.i18n.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- apps/meteor/client/views/admin/routes.tsx
- apps/meteor/client/views/admin/sidebarItems.ts
- apps/meteor/client/views/admin/ABAC/AdminABACPage.tsx
- apps/meteor/tests/mocks/data.ts
- apps/meteor/client/views/admin/ABAC/AdminABACSettingToggle.tsx
- apps/meteor/client/views/admin/ABAC/AdminABACTabs.tsx
- apps/meteor/client/views/admin/ABAC/AdminABACSettingToggle.stories.tsx
- packages/i18n/src/locales/en.i18n.json
🧰 Additional context used
🧬 Code graph analysis (3)
apps/meteor/client/views/admin/ABAC/AdminABACRoute.tsx (1)
packages/ui-contexts/src/index.ts (6)
usePermission(55-55)useCurrentModal(29-29)useRouteParameter(62-62)useRouter(60-60)useSettingStructure(73-73)useSetModal(68-68)
apps/meteor/client/views/admin/ABAC/AdminABACWarningModal.tsx (1)
packages/ui-contexts/src/index.ts (1)
useRouter(60-60)
apps/meteor/client/components/ABAC/ABACUpsellModal/ABACUpsellModal.spec.tsx (1)
apps/meteor/tests/mocks/data.ts (1)
createFakeLicenseInfo(213-261)
⏰ 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 (2)
apps/meteor/client/components/ABAC/ABACUpsellModal/ABACUpsellModal.spec.tsx (2)
7-7: LGTM! Import aligns with new endpoint mock.The import of
createFakeLicenseInfosupports the license endpoint mock added below.
91-109: Verify modal behavior supports multiple sequential button clicks.This test clicks the upgrade button and then the cancel button, expecting both callbacks to fire. Typically, clicking "Upgrade" in a modal triggers the action and closes the modal, making subsequent button clicks impossible. Verify that the modal remains open after the upgrade button is clicked, or adjust the test to match actual modal behavior.
apps/meteor/client/components/ABAC/ABACUpsellModal/ABACUpsellModal.spec.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
apps/meteor/client/views/admin/routes.tsx (2)
107-110: Consider using lowercase for route naming consistency.The route name 'admin-ABAC' and path '/admin/ABAC' use uppercase, which is inconsistent with all other admin routes in this file (e.g., 'admin-settings', 'custom-sounds', 'emoji-custom', 'moderation-console'). URLs typically follow lowercase conventions for better consistency and readability.
Consider changing to lowercase:
- 'admin-ABAC': { - pathname: '/admin/ABAC'; - pattern: '/admin/ABAC/:tab?/:context?/:id?'; + 'admin-abac': { + pathname: '/admin/abac'; + pattern: '/admin/abac/:tab?/:context?/:id?'; };Note: This would also require updating the route registration below and the component directory/import path to match.
245-248: Route registration follows the interface naming.The route registration uses uppercase 'ABAC' consistent with the interface declaration above (lines 107-110), but as noted, this diverges from the lowercase convention used throughout the file. If you decide to adopt lowercase naming, this registration and the component directory structure ('./ABAC/AdminABACRoute') would also need to be updated accordingly.
Functionally, the implementation is correct:
- Pattern correctly omits '/admin' prefix (handled by route group base path)
- Optional parameters (:tab?/:context?/:id?) are properly structured
- Lazy loading follows established patterns
apps/meteor/client/views/admin/ABAC/AdminABACSettingToggle.spec.tsx (1)
34-34: Consider complementing snapshots with specific assertions.While snapshot tests can catch unintended changes, they can be brittle and don't clearly communicate intent. Consider adding specific assertions alongside or instead of snapshots to verify expected DOM structure or attributes.
For example, instead of or in addition to the snapshot at line 117:
it('should be disabled when abac license is not installed', () => { render(<AdminABACSettingToggle hasABAC={false} />, { wrapper: baseAppRoot.withSetting('ABAC_Enabled', true, settingStructure).build(), }); const toggle = screen.getByRole('checkbox'); expect(toggle).toBeDisabled(); expect(toggle).toBeChecked(); // Still shows as checked but disabled });Also applies to: 117-117, 124-124
📜 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/ABAC/__snapshots__/AdminABACSettingToggle.spec.tsx.snapis excluded by!**/*.snap
📒 Files selected for processing (13)
apps/meteor/client/components/ABAC/ABACUpsellModal/ABACUpsellModal.spec.tsx(2 hunks)apps/meteor/client/views/admin/ABAC/AdminABACPage.tsx(1 hunks)apps/meteor/client/views/admin/ABAC/AdminABACRoute.tsx(1 hunks)apps/meteor/client/views/admin/ABAC/AdminABACSettingToggle.spec.tsx(1 hunks)apps/meteor/client/views/admin/ABAC/AdminABACSettingToggle.stories.tsx(1 hunks)apps/meteor/client/views/admin/ABAC/AdminABACSettingToggle.tsx(1 hunks)apps/meteor/client/views/admin/ABAC/AdminABACSettings.tsx(1 hunks)apps/meteor/client/views/admin/ABAC/AdminABACTabs.tsx(1 hunks)apps/meteor/client/views/admin/ABAC/AdminABACWarningModal.tsx(1 hunks)apps/meteor/client/views/admin/routes.tsx(2 hunks)apps/meteor/client/views/admin/sidebarItems.ts(1 hunks)apps/meteor/ee/server/settings/abac.ts(1 hunks)apps/meteor/tests/mocks/data.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (10)
- apps/meteor/tests/mocks/data.ts
- apps/meteor/client/views/admin/ABAC/AdminABACRoute.tsx
- apps/meteor/client/views/admin/ABAC/AdminABACWarningModal.tsx
- apps/meteor/client/components/ABAC/ABACUpsellModal/ABACUpsellModal.spec.tsx
- apps/meteor/client/views/admin/ABAC/AdminABACSettingToggle.tsx
- apps/meteor/client/views/admin/ABAC/AdminABACPage.tsx
- apps/meteor/client/views/admin/ABAC/AdminABACSettings.tsx
- apps/meteor/client/views/admin/ABAC/AdminABACSettingToggle.stories.tsx
- apps/meteor/client/views/admin/sidebarItems.ts
- apps/meteor/client/views/admin/ABAC/AdminABACTabs.tsx
🧰 Additional context used
🧠 Learnings (12)
📓 Common learnings
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 37303
File: apps/meteor/tests/end-to-end/api/abac.ts:1125-1137
Timestamp: 2025-10-27T14:38:46.994Z
Learning: In Rocket.Chat ABAC feature, when ABAC is disabled globally (ABAC_Enabled setting is false), room-level ABAC attributes are not evaluated when changing room types. This means converting a private room to public will succeed even if the room has ABAC attributes, as long as the global ABAC setting is disabled.
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Use descriptive test names that clearly communicate expected behavior
Applied to files:
apps/meteor/client/views/admin/ABAC/AdminABACSettingToggle.spec.tsx
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Group related tests in the same file
Applied to files:
apps/meteor/client/views/admin/ABAC/AdminABACSettingToggle.spec.tsx
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Ensure tests run reliably in parallel without shared state conflicts
Applied to files:
apps/meteor/client/views/admin/ABAC/AdminABACSettingToggle.spec.tsx
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Maintain test isolation between test cases
Applied to files:
apps/meteor/client/views/admin/ABAC/AdminABACSettingToggle.spec.tsx
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 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/client/views/admin/ABAC/AdminABACSettingToggle.spec.tsx
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Prefer web-first assertions (e.g., toBeVisible, toHaveText)
Applied to files:
apps/meteor/client/views/admin/ABAC/AdminABACSettingToggle.spec.tsx
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Utilize Playwright fixtures (test, page, expect) consistently
Applied to files:
apps/meteor/client/views/admin/ABAC/AdminABACSettingToggle.spec.tsx
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Applies to apps/meteor/tests/e2e/**/*.{ts,tsx,js,jsx} : Write concise, technical TypeScript/JavaScript with accurate typing
Applied to files:
apps/meteor/client/views/admin/ABAC/AdminABACSettingToggle.spec.tsx
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Ensure a clean state for each test execution
Applied to files:
apps/meteor/client/views/admin/ABAC/AdminABACSettingToggle.spec.tsx
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Use expect matchers (toEqual, toContain, toBeTruthy, toHaveLength, etc.) instead of assert statements
Applied to files:
apps/meteor/client/views/admin/ABAC/AdminABACSettingToggle.spec.tsx
📚 Learning: 2025-10-27T14:38:46.994Z
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 37303
File: apps/meteor/tests/end-to-end/api/abac.ts:1125-1137
Timestamp: 2025-10-27T14:38:46.994Z
Learning: In Rocket.Chat ABAC feature, when ABAC is disabled globally (ABAC_Enabled setting is false), room-level ABAC attributes are not evaluated when changing room types. This means converting a private room to public will succeed even if the room has ABAC attributes, as long as the global ABAC setting is disabled.
Applied to files:
apps/meteor/ee/server/settings/abac.ts
🧬 Code graph analysis (1)
apps/meteor/client/views/admin/ABAC/AdminABACSettingToggle.spec.tsx (2)
packages/core-typings/src/ISetting.ts (1)
ISetting(27-27)packages/mock-providers/src/index.ts (1)
mockAppRoot(3-3)
⏰ 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). (3)
- GitHub Check: 📦 Build Packages
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🔇 Additional comments (1)
apps/meteor/ee/server/settings/abac.ts (1)
16-16: LGTM! Good addition of user-facing documentation.Adding the
i18nDescriptionfield provides translatable help text for administrators configuring the ABAC feature. This follows best practices for admin settings.
| @@ -0,0 +1,142 @@ | |||
| import type { ISetting } from '@rocket.chat/apps-engine/definition/settings'; | |||
There was a problem hiding this comment.
Import ISetting from the correct package.
The import path for ISetting should be from @rocket.chat/core-typings, not from @rocket.chat/apps-engine/definition/settings.
Apply this diff to fix the import:
-import type { ISetting } from '@rocket.chat/apps-engine/definition/settings';
+import type { ISetting } from '@rocket.chat/core-typings';🤖 Prompt for AI Agents
In apps/meteor/client/views/admin/ABAC/AdminABACSettingToggle.spec.tsx around
line 1, the ISetting type is imported from the wrong package; replace the import
from '@rocket.chat/apps-engine/definition/settings' with an import from
'@rocket.chat/core-typings' so the file imports ISetting from the correct
package.
| it('should show warning modal when disabling ABAC', async () => { | ||
| const user = userEvent.setup(); | ||
| render(<AdminABACSettingToggle hasABAC={true} />, { | ||
| wrapper: baseAppRoot.withSetting('ABAC_Enabled', true, settingStructure).build(), | ||
| }); | ||
|
|
||
| const toggle = screen.getByRole('checkbox'); | ||
| await waitFor(() => { | ||
| expect(toggle).not.toBeDisabled(); | ||
| }); | ||
| await user.click(toggle); | ||
|
|
||
| await waitFor(() => { | ||
| expect(screen.getByText('Disable ABAC')).toBeInTheDocument(); | ||
| }); | ||
|
|
||
| // TODO: discover how to automatically unmount all modals after each test | ||
| const cancelButton = screen.getByRole('button', { name: /cancel/i }); | ||
| await user.click(cancelButton); | ||
| }); | ||
|
|
||
| it('should not show warning modal when enabling ABAC', async () => { | ||
| const user = userEvent.setup(); | ||
| render(<AdminABACSettingToggle hasABAC={true} />, { | ||
| wrapper: baseAppRoot.withSetting('ABAC_Enabled', false, settingStructure).build(), | ||
| }); | ||
|
|
||
| const toggle = screen.getByRole('checkbox'); | ||
| await user.click(toggle); | ||
|
|
||
| // The modal should not appear when enabling ABAC | ||
| expect(screen.queryByText('Disable ABAC')).not.toBeInTheDocument(); | ||
| }); | ||
|
|
||
| it('should show warning modal when resetting setting', async () => { | ||
| const user = userEvent.setup(); | ||
| render(<AdminABACSettingToggle hasABAC={true} />, { | ||
| wrapper: baseAppRoot.withSetting('ABAC_Enabled', true, settingStructure).build(), | ||
| }); | ||
|
|
||
| const resetButton = screen.getByRole('button', { name: /reset/i }); | ||
| await user.click(resetButton); | ||
|
|
||
| await waitFor(() => { | ||
| expect(screen.getByText('Disable ABAC')).toBeInTheDocument(); | ||
| }); | ||
|
|
||
| // TODO: discover how to automatically unmount all modals after each test | ||
| const cancelButton = screen.getByRole('button', { name: /cancel/i }); | ||
| await user.click(cancelButton); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add test coverage for the confirmation flow.
The existing tests verify that the warning modal appears when disabling or resetting, but they don't test the actual confirmation path. Consider adding a test that clicks the "Disable" button in the modal and verifies the setting is actually changed.
Add a test case like:
it('should disable ABAC when confirmation button is clicked', async () => {
const user = userEvent.setup();
render(<AdminABACSettingToggle hasABAC={true} />, {
wrapper: baseAppRoot.withSetting('ABAC_Enabled', true, settingStructure).build(),
});
const toggle = screen.getByRole('checkbox');
await user.click(toggle);
await waitFor(() => {
expect(screen.getByText('Disable ABAC')).toBeInTheDocument();
});
const disableButton = screen.getByRole('button', { name: /disable/i });
await user.click(disableButton);
// Verify the setting was actually changed
await waitFor(() => {
expect(toggle).not.toBeChecked();
});
});🤖 Prompt for AI Agents
In apps/meteor/client/views/admin/ABAC/AdminABACSettingToggle.spec.tsx around
lines 37-87, add a new test that exercises the confirmation path: render the
component with ABAC_Enabled true, open the disable modal by clicking the
checkbox (or reset), wait for the "Disable ABAC" modal, find and click the
modal's confirm ("Disable") button, then wait and assert the toggle is no longer
checked (or the underlying setting changed); mirror the same pattern for the
reset flow if desired and ensure modal dismissal/cleanup after the assertion.
| it('should show warning modal when disabling ABAC', async () => { | ||
| const user = userEvent.setup(); | ||
| render(<AdminABACSettingToggle hasABAC={true} />, { | ||
| wrapper: baseAppRoot.withSetting('ABAC_Enabled', true, settingStructure).build(), | ||
| }); | ||
|
|
||
| const toggle = screen.getByRole('checkbox'); | ||
| await waitFor(() => { | ||
| expect(toggle).not.toBeDisabled(); | ||
| }); | ||
| await user.click(toggle); | ||
|
|
||
| await waitFor(() => { | ||
| expect(screen.getByText('Disable ABAC')).toBeInTheDocument(); | ||
| }); | ||
|
|
||
| // TODO: discover how to automatically unmount all modals after each test | ||
| const cancelButton = screen.getByRole('button', { name: /cancel/i }); | ||
| await user.click(cancelButton); | ||
| }); |
There was a problem hiding this comment.
Implement proper modal cleanup to ensure test isolation.
The TODO comments at lines 54 and 85 highlight a test isolation concern. Manually dismissing modals in each test is fragile and could cause test pollution if the modal persists between tests.
Consider implementing an afterEach hook to clean up modals:
afterEach(() => {
// Close any open modals
const cancelButtons = screen.queryAllByRole('button', { name: /cancel/i });
cancelButtons.forEach(button => button.click());
});Alternatively, investigate if the mockAppRoot builder or testing library provides a cleanup mechanism for portaled elements.
Also applies to: 71-87
🤖 Prompt for AI Agents
In apps/meteor/client/views/admin/ABAC/AdminABACSettingToggle.spec.tsx around
lines 37 to 56 (and similarly 71 to 87), tests manually dismiss modals which
risks test pollution; add an afterEach hook in this spec file that reliably
cleans up any open modal before each test ends by either calling the
testing-library cleanup/unmount helpers or by querying for modal close/cancel
buttons and programmatically clicking them, ensuring portaled elements are
removed so each test runs in isolation.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/meteor/client/lib/links.ts (1)
34-37: Replace TODO placeholder URLs before production merge and consider naming consistency improvements.The three new ABAC link entries use TODO placeholder URLs that will result in broken links if accessed by users in production. Ensure these are updated with actual documentation URLs before merging.
Additionally, the naming patterns should align with existing entries in the file:
abacLicenseRenewalUrluses a "Url" suffix unlike other entries (ldapDocs,omnichannelDocs, etc.) — consider renaming toabacLicenseRenewalabacLDAPDocsuses uppercase "LDAP" while the existing entry uses lowercase (ldapDocs) — consider renaming toabacLdapDocs
📜 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 (3)
apps/meteor/client/lib/links.ts(1 hunks)apps/meteor/client/views/admin/ABAC/AdminABACPage.tsx(1 hunks)apps/meteor/client/views/admin/ABAC/AdminABACSettings.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/meteor/client/views/admin/ABAC/AdminABACPage.tsx
- apps/meteor/client/views/admin/ABAC/AdminABACSettings.tsx
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 37303
File: apps/meteor/tests/end-to-end/api/abac.ts:1125-1137
Timestamp: 2025-10-27T14:38:46.994Z
Learning: In Rocket.Chat ABAC feature, when ABAC is disabled globally (ABAC_Enabled setting is false), room-level ABAC attributes are not evaluated when changing room types. This means converting a private room to public will succeed even if the room has ABAC attributes, as long as the global ABAC setting is disabled.
⏰ 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). (3)
- GitHub Check: 📦 Build Packages
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
Co-authored-by: Kevin Aleman <kaleman960@gmail.com> Co-authored-by: Tasso <tasso.evangelista@rocket.chat>
Co-authored-by: Kevin Aleman <kaleman960@gmail.com> Co-authored-by: Tasso <tasso.evangelista@rocket.chat>
Co-authored-by: Kevin Aleman <kaleman960@gmail.com> Co-authored-by: Tasso <tasso.evangelista@rocket.chat>
Co-authored-by: Kevin Aleman <kaleman960@gmail.com> Co-authored-by: Tasso <tasso.evangelista@rocket.chat>
Co-authored-by: Kevin Aleman <kaleman960@gmail.com> Co-authored-by: Tasso <tasso.evangelista@rocket.chat>
Co-authored-by: Kevin Aleman <kaleman960@gmail.com> Co-authored-by: Tasso <tasso.evangelista@rocket.chat>
Co-authored-by: Kevin Aleman <kaleman960@gmail.com> Co-authored-by: Tasso <tasso.evangelista@rocket.chat>
Co-authored-by: Kevin Aleman <kaleman960@gmail.com> Co-authored-by: Tasso <tasso.evangelista@rocket.chat>
Co-authored-by: Kevin Aleman <kaleman960@gmail.com> Co-authored-by: Tasso <tasso.evangelista@rocket.chat>
Co-authored-by: Kevin Aleman <kaleman960@gmail.com> Co-authored-by: Tasso <tasso.evangelista@rocket.chat>
Co-authored-by: Kevin Aleman <kaleman960@gmail.com> Co-authored-by: Tasso <tasso.evangelista@rocket.chat>
4 participants
Proposed changes (including videos or screenshots)
Issue(s)
ABAC-48
Steps to test or reproduce
Further comments
Contains #37162
Summary by CodeRabbit
New Features
Tests
Localization
Chores