Add permission check for view-outside-room in Directory screen#6939
Add permission check for view-outside-room in Directory screen#6939deepak0x wants to merge 3 commits intoRocketChat:developfrom
Conversation
- Disable Users filter option when user lacks view-outside-room permission - Show error message when user tries to access Users without permission - Fallback to channels view if users is default but permission is missing - Add hasViewOutsideRoomPermission method to check user roles
WalkthroughDirectory view now enforces a "view outside room" permission: DirectoryView derives the permission from redux, initializes the default view accordingly, prevents switching to the "users" view when unauthorized (showing an error), and Options receives a prop to visually and functionally disable the "users" option. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Options as DirectoryOptions (UI)
participant DirView as DirectoryView
participant Perm as Redux/PermissionChecker
participant Alert as showErrorAlert
User->>Options: tap "Users" option
Options->>DirView: request changeType('users')
DirView->>Perm: hasViewOutsideRoomPermission()
alt permission granted
DirView-->>Options: update state to 'users'
Options-->>User: users view selected
else permission denied
DirView->>Alert: showErrorAlert(...)
DirView-->>Options: no state change (keep previous view)
Options-->>User: "Users" option remains disabled
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@app/views/DirectoryView/index.tsx`:
- Around line 134-136: The sort option is hardcoded to { usersCount: -1 } for
all directory types; update the code that builds the query/options (the object
containing count: 50 and sort) to choose the sort key based on the directory
"type" value: for type === 'users' use a user-appropriate sort (e.g., {
username: 1 } or { name: 1 }), and for type === 'channels' or 'teams' use {
usersCount: -1 }; modify the block that currently sets "sort" so it
conditionally assigns the sort object based on the "type" variable.
🧹 Nitpick comments (2)
app/views/DirectoryView/index.tsx (2)
57-60: DRY violation: Permission check logic is duplicated.The permission check in the constructor duplicates the logic in
hasViewOutsideRoomPermission()method (lines 93-99). Consider reusing the method or extracting a static helper.♻️ Suggested refactor
class DirectoryView extends React.Component<IDirectoryViewProps, IDirectoryViewState> { + private static checkViewOutsideRoomPermission( + viewOutsideRoomPermission?: string[], + userRoles?: string[] + ): boolean { + if (!viewOutsideRoomPermission || !userRoles) { + return false; + } + return userRoles.some(role => viewOutsideRoomPermission.includes(role)); + } + constructor(props: IDirectoryViewProps) { super(props); - const hasPermission = props.viewOutsideRoomPermission && props.user?.roles - ? props.user.roles.some(role => props.viewOutsideRoomPermission!.includes(role)) - : false; + const hasPermission = DirectoryView.checkViewOutsideRoomPermission( + props.viewOutsideRoomPermission, + props.user?.roles + ); const defaultType = props.directoryDefaultView === 'users' && !hasPermission ? 'channels' : props.directoryDefaultView;Then update
hasViewOutsideRoomPermissionto use the static method:hasViewOutsideRoomPermission = (): boolean => { const { viewOutsideRoomPermission, user } = this.props; - if (!viewOutsideRoomPermission || !user?.roles) { - return false; - } - return user.roles.some(role => viewOutsideRoomPermission.includes(role)); + return DirectoryView.checkViewOutsideRoomPermission(viewOutsideRoomPermission, user?.roles); };
349-350: Avoidas anytype cast for better type safety.The
(state.permissions as any)['view-outside-room']cast bypasses TypeScript's type checking. Consider extending the permissions type definition inIApplicationStateto include this permission key properly.#!/bin/bash # Description: Check how permissions are typed in the application state # Find the IApplicationState interface or permissions type definition rg -n "permissions.*:" --type ts -C5 | head -80 # Check if there's a permissions reducer or type ast-grep --pattern 'interface $_ { $$$ permissions$_ $$$ }'
- Sort users by username (ascending) - Sort channels/teams by usersCount (descending) - Prevents sorting users by non-existent usersCount field
- Remove viewOutsideRoomPermission prop, access permission from Redux state - Use same permission checking logic as usePermissions hook - Access permission via reduxStore.getState() for class component compatibility
This PR fixes the Directory screen issue where users without the
view-outside-roompermission experience a continuous loading indicator when trying to access the "Users" filter. The fix adds client-side permission checks to prevent API calls that would result in 400 errors and provides better user feedback.The changes include:
view-outside-roombefore allowing access to Users directoryIssue(s)
Closes #6877
How to test or reproduce
view-outside-roompermission (or disable this permission for the user role)Screenshots
Types of changes
Checklist
Further comments
This fix implements client-side permission validation to prevent the 400 Bad Request error that was occurring when users without the
view-outside-roompermission tried to access the Users directory. By checking permissions before making API callsSummary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.