fix: add missing permission check to feature-preview admin sidebar item#38934
fix: add missing permission check to feature-preview admin sidebar item#38934Naetiksoni08 wants to merge 6 commits into
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: ae989eb The changes in this PR will be included in the next version bump. This PR includes changesets to release 41 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 |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a missing permission check for the Feature Preview admin sidebar item: the item now requires the new Changes
Sequence Diagram(s)sequenceDiagram
participant Sidebar as SidebarItem (Client)
participant Auth as PermissionService (Client)
participant Server as Permissions Registry (Server)
Sidebar->>Auth: permissionGranted? hasPermission('manage-feature-preview')
Auth->>Server: check permission for current user
Server-->>Auth: allow / deny (based on `{_id: 'manage-feature-preview'}`)
Auth-->>Sidebar: boolean result
Sidebar->>Sidebar: render item if result && defaultFeaturesPreview?.length > 0
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
dougfabris
left a comment
dougfabris
left a comment
There was a problem hiding this comment.
This permission doesn't exists
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/meteor/client/views/admin/sidebarItems.ts`:
- Around line 138-139: The sidebar permission check uses the string
'manage-feature-preview' (seen in permissionGranted: () =>
hasPermission('manage-feature-preview') && defaultFeaturesPreview?.length > 0)
but that permission is not registered in the permissions registry; add an entry
for 'manage-feature-preview' to the permissions export (the permissions
constant/registry in permissions.ts) and assign the appropriate roles (e.g.,
['admin']) following the same shape and pattern as other feature-management
permissions so hasPermission('manage-feature-preview') can succeed.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.changeset/funny-houses-peel.mdapps/meteor/client/views/admin/sidebarItems.ts
📜 Review details
⏰ 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). (1)
- GitHub Check: cubic · AI code reviewer
🧰 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/sidebarItems.ts
🧠 Learnings (2)
📚 Learning: 2025-11-04T16:49:19.107Z
Learnt from: ricardogarim
Repo: RocketChat/Rocket.Chat PR: 37377
File: apps/meteor/ee/server/hooks/federation/index.ts:86-88
Timestamp: 2025-11-04T16:49:19.107Z
Learning: In Rocket.Chat's federation system (apps/meteor/ee/server/hooks/federation/), permission checks follow two distinct patterns: (1) User-initiated federation actions (creating rooms, adding users to federated rooms, joining from invites) should throw MeteorError to inform users they lack 'access-federation' permission. (2) Remote server-initiated federation events should silently skip/ignore when users lack permission. The beforeAddUserToRoom hook only executes for local user-initiated actions, so throwing an error there is correct. Remote federation events are handled separately by the federation Matrix package with silent skipping logic.
Applied to files:
.changeset/funny-houses-peel.md
📚 Learning: 2026-02-10T16:32:42.586Z
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 38528
File: apps/meteor/client/startup/roles.ts:14-14
Timestamp: 2026-02-10T16:32:42.586Z
Learning: In Rocket.Chat's Meteor client code, DDP streams use EJSON and Date fields arrive as Date objects; do not manually construct new Date() in stream handlers (for example, in sdk.stream()). Only REST API responses return plain JSON where dates are strings, so implement explicit conversion there if needed. Apply this guidance to all TypeScript files under apps/meteor/client to ensure consistent date handling in DDP streams and REST responses.
Applied to files:
apps/meteor/client/views/admin/sidebarItems.ts
🧬 Code graph analysis (1)
apps/meteor/client/views/admin/sidebarItems.ts (1)
packages/ui-client/src/hooks/useFeaturePreviewList.ts (1)
defaultFeaturesPreview(21-31)
🔇 Additional comments (1)
.changeset/funny-houses-peel.md (1)
1-5: LGTM!Changeset is correctly formatted —
patchbump is appropriate for a bug fix, and the description accurately reflects the change.
|
Hi @dougfabris , thank you for the quick review! You're absolutely right. I used 'manage-feature-preview' without verifying if it exists in the permissions registry. I'll push an updated fix shortly. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/meteor/app/authorization/server/constant/permissions.ts (1)
208-214:⚠️ Potential issue | 🟡 MinorPre-existing duplicate:
remove-livechat-departmentis registered twice.Lines 208 and 214 contain identical entries for
remove-livechat-department. While harmless today because the roles are the same, this is dead weight and could silently cause a divergence bug if either copy's roles are edited independently in the future. Worth cleaning up in this pass since you're already touching the file.🧹 Proposed fix
- { _id: 'remove-livechat-department', roles: ['livechat-manager', 'admin'] }, - - // New Media calls permissions - { _id: 'allow-internal-voice-calls', roles: ['admin', 'user'] }, - { _id: 'allow-external-voice-calls', roles: ['admin', 'user'] }, - - { _id: 'remove-livechat-department', roles: ['livechat-manager', 'admin'] }, + { _id: 'remove-livechat-department', roles: ['livechat-manager', 'admin'] }, + + // New Media calls permissions + { _id: 'allow-internal-voice-calls', roles: ['admin', 'user'] }, + { _id: 'allow-external-voice-calls', roles: ['admin', 'user'] },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/app/authorization/server/constant/permissions.ts` around lines 208 - 214, The permission entry '_id: remove-livechat-department' is duplicated; remove the redundant duplicate so only one { _id: 'remove-livechat-department', roles: ['livechat-manager', 'admin'] } entry remains (remove the second occurrence under the "New Media calls permissions" block) to avoid future divergence if roles are edited independently.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@apps/meteor/app/authorization/server/constant/permissions.ts`:
- Around line 208-214: The permission entry '_id: remove-livechat-department' is
duplicated; remove the redundant duplicate so only one { _id:
'remove-livechat-department', roles: ['livechat-manager', 'admin'] } entry
remains (remove the second occurrence under the "New Media calls permissions"
block) to avoid future divergence if roles are edited independently.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/meteor/app/authorization/server/constant/permissions.ts
📜 Review details
🧰 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/app/authorization/server/constant/permissions.ts
🧠 Learnings (1)
📓 Common learnings
Learnt from: ggazzo
Repo: RocketChat/Rocket.Chat PR: 35995
File: apps/meteor/app/api/server/v1/rooms.ts:1107-1112
Timestamp: 2026-02-23T17:53:06.802Z
Learning: In Rocket.Chat PR reviews, maintain strict scope boundaries—when a PR is focused on a specific endpoint (e.g., rooms.favorite), avoid reviewing or suggesting changes to other endpoints that were incidentally refactored (e.g., rooms.invite) unless explicitly requested by maintainers.
🔇 Additional comments (1)
apps/meteor/app/authorization/server/constant/permissions.ts (1)
221-221: LGTM —manage-feature-previewpermission correctly registered.The new entry follows the established
manage-*naming convention, androles: ['admin']is the right default for this admin-only feature.
|
Hi @dougfabris, I have now registered 'manage-feature-preview' in the permissions registry with admin role, following the same pattern as other manage-* permissions like manage-sounds and manage-emoji. |
Naetiksoni08
force-pushed
the
fix/feature-preview-sidebar-permission-check
branch
from
e8b4dd4 to
a18e835
Compare
Naetiksoni08
force-pushed
the
fix/feature-preview-sidebar-permission-check
branch
from
a18e835 to
681f5a6
Compare
|
Hi @dougfabris, gentle ping on this PR! I have addressed your feedback by registering 'manage-feature-preview' in the permissions registry with admin role in permissions.ts, following the same pattern as manage-sounds and manage-emoji. Would really appreciate a re-review when you get a chance. Thank you! |
Naetiksoni08
force-pushed
the
fix/feature-preview-sidebar-permission-check
branch
from
681f5a6 to
4d8e529
Compare
Naetiksoni08
force-pushed
the
fix/feature-preview-sidebar-permission-check
branch
2 times, most recently
from
bb2c5a1 to
c470272
Compare
|
Hi @dougfabris , gentle ping on this PR! The permission has been registered following the same pattern as manage-sounds and manage-emoji. Would really appreciate a re-review when you get a chance. Thank you! |
|
Hi there, thanks for the contribution 🚀 Please, hold on! We're going to handle your pull request as soon as possible, theres a lot of contributions besides yours. |
dougfabris
left a comment
dougfabris
left a comment
There was a problem hiding this comment.
please, replace this new permission in the AdminFeaturePreviewRoute as well
|
/jira COMM |
Naetiksoni08
force-pushed
the
fix/feature-preview-sidebar-permission-check
branch
from
c470272 to
b7f8c61
Compare
|
@dougfabris, Done I've also replaced the permission in AdminFeaturePreviewRoute to use manage-feature-preview instead of manage-cloud, so both the sidebar item and the route now check the same permission consistently. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #38934 +/- ##
===========================================
+ Coverage 69.79% 69.84% +0.04%
===========================================
Files 3296 3296
Lines 119173 119173
Branches 21484 21485 +1
===========================================
+ Hits 83182 83241 +59
+ Misses 32692 32622 -70
- Partials 3299 3310 +11
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
| { _id: 'set-react-when-readonly', roles: ['admin', 'owner'] }, | ||
| { _id: 'manage-cloud', roles: ['admin'] }, | ||
| { _id: 'manage-sounds', roles: ['admin'] }, | ||
| { _id: 'manage-feature-preview', roles: ['admin'] }, |
There was a problem hiding this comment.
Missing i18n key for this new permission
There was a problem hiding this comment.
@pierre-lehnen-rc Done! Added the missing i18n key for the manage-feature-preview permission.
|
Hey @dougfabris, just a gentle ping on this PR! 😊 I get that u guys might be busy. Happy to address any feedback or make changes if needed. |
3 participants
Proposed changes
The
feature-previewsidebar item in the admin panel was visible to regular users (role: user) because itspermissionGrantedfunction only checked whether feature previews exist, without verifying if theuser actually has permission to access them.
Every other admin sidebar item uses
hasPermission()to control visibility, butfeature-previewwas only doing:defaultFeaturesPreview?.length > 0This caused the sidebar item to appear for all users as long as any feature previews were configured — regardless of their role or permissions.
Code Fix:
// Before
permissionGranted: () => defaultFeaturesPreview?.length > 0// After
Before Fix
url-admin/feature-preview
url - admin/subscription or any other sidebar item
After Fix
url-admin/feature-preview
url - admin/subscription or any other sidebar item
Steps to test or reproduce
in sidebar (before fix).
Further comments
During testing, an inconsistency was discovered in the admin
sidebar permission model:
Every other sidebar item uses hasPermission() — feature-preview was the only exception. This also creates a security concern: any user who knows the URL can discover this admin feature exists, even without access. Sidebar items should never leak information about admin features to unauthorized users.
Summary by CodeRabbit
Bug Fixes
Chores
Task: COMM-157