regression(ABAC): Missing settings from abac -> Settings tab #37766
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 |
|
WalkthroughThis PR renames the ABAC toggle component from Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
📦 Docker Image Size Report📈 Changes
📊 Historical Trend---
config:
theme: "dark"
xyChart:
width: 900
height: 400
---
xychart
title "Image Size Evolution by Service (Last 30 Days + This PR)"
x-axis ["11/15 22:28", "11/16 01:28", "11/17 23:50", "11/18 22:53", "11/19 23:02", "11/21 16:49", "11/24 17:34", "11/27 22:32", "11/28 19:05", "12/01 23:01", "12/02 21:57", "12/03 21:00", "12/04 18:17", "12/05 21:56", "12/08 20:15", "12/09 22:17", "12/10 19:56", "12/10 20:47 (PR)"]
y-axis "Size (GB)" 0 --> 0.5
line "account-service" [0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11]
line "authorization-service" [0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11]
line "ddp-streamer-service" [0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12]
line "omnichannel-transcript-service" [0.14, 0.14, 0.14, 0.14, 0.14, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13]
line "presence-service" [0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11]
line "queue-worker-service" [0.14, 0.14, 0.14, 0.14, 0.14, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13]
line "rocketchat" [0.36, 0.36, 0.35, 0.35, 0.35, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.35]
line "stream-hub-service" [0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11]
Statistics (last 17 days):
ℹ️ About this reportThis report compares Docker image sizes from this build against the
Updated: Wed, 10 Dec 2025 20:47:55 GMT |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
apps/meteor/client/views/admin/ABAC/ABACSettingTab/SettingToggle.spec.tsx (1)
7-7: Consider renaming the test file.The test file is still named
SettingToggle.spec.tsxbut now testsAbacEnabledToggle. For consistency and discoverability, consider renaming toAbacEnabledToggle.spec.tsx.apps/meteor/client/views/admin/ABAC/ABACSettingTab/SettingToggle.stories.tsx (1)
4-8: Consider renaming the stories file.Similar to the test file, this stories file is still named
SettingToggle.stories.tsxbut now defines stories forAbacEnabledToggle. Consider renaming toAbacEnabledToggle.stories.tsxfor consistency.
📜 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/ABACSettingTab/__snapshots__/SettingToggle.spec.tsx.snapis excluded by!**/*.snap
📒 Files selected for processing (5)
apps/meteor/client/views/admin/ABAC/ABACSettingTab/AbacEnabledToggle.tsx(2 hunks)apps/meteor/client/views/admin/ABAC/ABACSettingTab/SettingField.tsx(1 hunks)apps/meteor/client/views/admin/ABAC/ABACSettingTab/SettingToggle.spec.tsx(8 hunks)apps/meteor/client/views/admin/ABAC/ABACSettingTab/SettingToggle.stories.tsx(2 hunks)apps/meteor/client/views/admin/ABAC/ABACSettingTab/SettingsPage.tsx(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
apps/meteor/client/views/admin/ABAC/ABACSettingTab/SettingsPage.tsxapps/meteor/client/views/admin/ABAC/ABACSettingTab/SettingToggle.stories.tsxapps/meteor/client/views/admin/ABAC/ABACSettingTab/AbacEnabledToggle.tsxapps/meteor/client/views/admin/ABAC/ABACSettingTab/SettingToggle.spec.tsxapps/meteor/client/views/admin/ABAC/ABACSettingTab/SettingField.tsx
🧠 Learnings (14)
📓 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-10-30T19:30:46.541Z
Learnt from: MartinSchoeler
Repo: RocketChat/Rocket.Chat PR: 37244
File: apps/meteor/client/views/admin/ABAC/AdminABACRoomAttributesForm.spec.tsx:125-146
Timestamp: 2025-10-30T19:30:46.541Z
Learning: In the AdminABACRoomAttributesForm component (apps/meteor/client/views/admin/ABAC/AdminABACRoomAttributesForm.tsx), the first attribute value field is mandatory and does not have a Remove button. Only additional values beyond the first have Remove buttons. This means trashButtons[0] corresponds to the second value's Remove button, not the first value's.
Applied to files:
apps/meteor/client/views/admin/ABAC/ABACSettingTab/SettingsPage.tsx
📚 Learning: 2025-11-07T14:50:33.544Z
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 37423
File: packages/i18n/src/locales/en.i18n.json:18-18
Timestamp: 2025-11-07T14:50:33.544Z
Learning: Rocket.Chat settings: in apps/meteor/ee/server/settings/abac.ts, the Abac_Cache_Decision_Time_Seconds setting uses invalidValue: 0 as the fallback when ABAC is unlicensed. With a valid license, admins can still set the value to 0 to intentionally disable the ABAC decision cache.
Applied to files:
apps/meteor/client/views/admin/ABAC/ABACSettingTab/SettingsPage.tsxapps/meteor/client/views/admin/ABAC/ABACSettingTab/AbacEnabledToggle.tsxapps/meteor/client/views/admin/ABAC/ABACSettingTab/SettingToggle.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/client/views/admin/ABAC/ABACSettingTab/SettingsPage.tsx
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Utilize Playwright fixtures (`test`, `page`, `expect`) for consistency in test files
Applied to files:
apps/meteor/client/views/admin/ABAC/ABACSettingTab/SettingToggle.spec.tsx
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Use `expect` matchers for assertions (`toEqual`, `toContain`, `toBeTruthy`, `toHaveLength`, etc.) instead of `assert` statements in Playwright tests
Applied to files:
apps/meteor/client/views/admin/ABAC/ABACSettingTab/SettingToggle.spec.tsx
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Prefer web-first assertions (`toBeVisible`, `toHaveText`, etc.) in Playwright tests
Applied to files:
apps/meteor/client/views/admin/ABAC/ABACSettingTab/SettingToggle.spec.tsx
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
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/ABACSettingTab/SettingToggle.spec.tsx
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/page-objects/**/*.ts : Utilize existing page objects pattern from `apps/meteor/tests/e2e/page-objects/`
Applied to files:
apps/meteor/client/views/admin/ABAC/ABACSettingTab/SettingToggle.spec.tsx
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.{ts,spec.ts} : Follow Page Object Model pattern consistently in Playwright tests
Applied to files:
apps/meteor/client/views/admin/ABAC/ABACSettingTab/SettingToggle.spec.tsx
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
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/ABACSettingTab/SettingToggle.spec.tsx
📚 Learning: 2025-12-10T21:00:43.645Z
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 37091
File: ee/packages/abac/jest.config.ts:4-7
Timestamp: 2025-12-10T21:00:43.645Z
Learning: Rocket.Chat monorepo: Jest testMatch pattern '<rootDir>/src/**/*.spec.(ts|js|mjs)' is valid in this repo and used across multiple packages (e.g., packages/tools, ee/packages/omnichannel-services). Do not flag it as invalid in future reviews.
Applied to files:
apps/meteor/client/views/admin/ABAC/ABACSettingTab/SettingToggle.spec.tsx
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Maintain test isolation between test cases in Playwright tests
Applied to files:
apps/meteor/client/views/admin/ABAC/ABACSettingTab/SettingToggle.spec.tsx
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : All test files must be created in `apps/meteor/tests/e2e/` directory
Applied to files:
apps/meteor/client/views/admin/ABAC/ABACSettingTab/SettingToggle.spec.tsx
🧬 Code graph analysis (1)
apps/meteor/client/views/admin/ABAC/ABACSettingTab/AbacEnabledToggle.tsx (1)
packages/core-typings/src/ISetting.ts (1)
SettingValue(11-20)
🪛 ast-grep (0.40.0)
apps/meteor/client/views/admin/ABAC/ABACSettingTab/SettingField.tsx
[warning] 111-111: Usage of dangerouslySetInnerHTML detected. This bypasses React's built-in XSS protection. Always sanitize HTML content using libraries like DOMPurify before injecting it into the DOM to prevent XSS attacks.
Context: dangerouslySetInnerHTML
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://reactjs.org/docs/dom-elements.html#dangerouslysetinnerhtml
- https://cwe.mitre.org/data/definitions/79.html
(react-unsafe-html-injection)
🪛 Biome (2.1.2)
apps/meteor/client/views/admin/ABAC/ABACSettingTab/SettingField.tsx
[error] 112-112: Avoid passing content using the dangerouslySetInnerHTML prop.
Setting content using code can expose users to cross-site scripting (XSS) attacks
(lint/security/noDangerouslySetInnerHtml)
🔇 Additional comments (7)
apps/meteor/client/views/admin/ABAC/ABACSettingTab/SettingField.tsx (2)
110-114: Sanitization with DOMPurify is correctly applied.The static analysis tools flagged
dangerouslySetInnerHTML, but DOMPurify sanitization is properly applied before injection. This is the standard pattern for safely rendering HTML content.
67-70: Acceptable eslint-disable for conditional property dependency.The cast
(setting as ISettingColor).editoris necessary because theeditorproperty only exists on color-type settings. The eslint-disable is justified here since the dependency is correctly tracking the actual value being used.apps/meteor/client/views/admin/ABAC/ABACSettingTab/SettingsPage.tsx (1)
15-17: Clean integration of ABAC settings components.The new
SettingFieldcomponents correctly expose the previously missing settings (ABAC_ShowAttributesInRoomsandAbac_Cache_Decision_Time_Seconds) in the ABAC settings panel, addressing the PR objective.apps/meteor/client/views/admin/ABAC/ABACSettingTab/SettingToggle.spec.tsx (1)
29-141: Tests properly updated for the component rename.All test cases have been consistently updated to use
AbacEnabledToggle. Test coverage for warning modals, license gating, accessibility, and state management remains intact.apps/meteor/client/views/admin/ABAC/ABACSettingTab/AbacEnabledToggle.tsx (2)
12-16: Clean component rename with consistent naming.The rename from
SettingToggle/SettingTogglePropstoABACEnabledToggle/ABACEnabledTogglePropsimproves clarity by indicating this component is specifically for the ABAC enable/disable toggle rather than a generic setting toggle.
89-89: Good improvement to hasResetButton logic.The updated condition
hasABAC && setting.packageValue !== setting.valuecorrectly ensures the reset button is only shown when the ABAC license is active. This prevents users from seeing a reset button they cannot meaningfully use when unlicensed.apps/meteor/client/views/admin/ABAC/ABACSettingTab/SettingToggle.stories.tsx (1)
45-46: Stories properly updated for the component rename.The Meta configuration and Story type are correctly updated to reference
AbacEnabledToggle. All three story variants (Default, Loading, False) maintain their existing behavior.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## feat/abac #37766 +/- ##
============================================
Coverage ? 54.29%
============================================
Files ? 2633
Lines ? 50105
Branches ? 11224
============================================
Hits ? 27206
Misses ? 20724
Partials ? 2175
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
Proposed changes (including videos or screenshots)
Some settings were not present in the settings tab of the ABAC page, only on the administration -> settings view
Issue(s)
ABAC-92
Steps to test or reproduce
Further comments
Summary by CodeRabbit
New Features
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.