-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Add permission check for view-outside-room in Directory screen #6939
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Add permission check for view-outside-room in Directory screen #6939
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.