fix: Fix admin delete group API call

[sudhir-sars]

API Enhancement: Improve groups.delete Permission Handling and Error Messages

Proposed changes

This pull request modifies the behavior of the API call for deleting a group in Rocket.Chat to handle cases where the user is not found or does not have access to the room or user is a admin and is not part of the group. Specifically, we updated the following:

• Role Check Improvement: The logic for checking if the user has sufficient permissions (either admin or room access) has been refined.
• Error Handling: We now explicitly throw a user-not-found error if the user is not found and room-not-found if the user does not have the required permissions for the room.
  This change should improve the reliability of the groups.delete API by providing clearer error messages when an operation is not permitted.

Issue(s)

Fixes #16954 - Groups deletion API fails for admin users when they're not members of the private group, even with proper admin permissions.

Steps to Reproduce Original Issue

1. Setup Prerequisites

   • Have a Rocket.Chat instance running (tested on version 3.0.4 and above)
   • Create an admin user with appropriate permissions:
     • Delete Private Channels
     • View Private Room

2. Create Test Environment

   • Create a private group/channel through the UI
   • Ensure the admin user is NOT a member of this private group
   • Note down the room ID of the created group

3. API Testing Steps

   # 1. First authenticate as admin
   POST /api/v1/login
   {
     "username": "admin_user",
     "password": "password"
   }
   # Save the authToken and userId from response
   
   # 2. Verify group exists using groups.listAll
   GET /api/v1/groups.listAll
   Headers:
     X-Auth-Token: your_auth_token
     X-User-Id: your_user_id
   
   # 3. Attempt to delete the group
   POST /api/v1/groups.delete
   Headers:
     X-Auth-Token: your_auth_token
     X-User-Id: your_user_id
   Body:
     {
       "roomId": "your_room_id"
     }

4. Expected vs Actual Behavior

   • Expected: Group should be deleted successfully since user has admin permissions
   • Actual: Receives error:

   {
     "success": false,
     "error": "The required \"roomId\" or \"roomName\" param provided does not match any group [error-room-not-found]",
     "errorType": "error-room-not-found"
   }

Steps to Test Fix

1. Ensure you are logged in as a user with the correct permissions.
2. Call the groups.listAll API to retrieve a list of groups.
3. Attempt to delete a group using the groups.delete API, passing in the group ID.
4. Verify that:
   • If the user is not found, a user-not-found error is thrown.
   • If the user is not an admin or does not have the correct access to the room, a room-not-found error is thrown.

Implementation Details

Code Changes:

• Before:

if (!room || !user || !(await canAccessRoomAsync(room, user))) {
  throw new Meteor.Error('error-room-not-found', 'The required "roomId" or "roomName" param provided does not match any group');
}

• After:

if (!user) {
  throw new Meteor.Error('error-user-not-found', 'User not found');
}
if (user.roles?.includes('admin') || (room && await canAccessRoomAsync(room, user))) {
  // Continue if the user is an admin or has access to the room
} else {
  throw new Meteor.Error('error-room-not-found', 'The required "roomId" or "roomName" param provided does not match any group');
}

Further comments

The key change in this PR is the role check condition, ensuring that the user is either an admin or has specific access to the room. This should prevent unnecessary errors and provide more explicit feedback to the user when attempting to delete a group.

Testing Checklist

• Tested with admin user not in private group
• Tested with admin user in private group
• Tested with non-admin user
• Tested with invalid user ID
• Tested with invalid room ID
• Tested with user lacking permissions

[dionisio-bot]

Looks like this PR is not ready to merge, because of the following issues:

• This PR is missing the 'stat: QA assured' label
• This PR is missing the required milestone or project

Please fix the issues and try again

If you have any trouble, please check the PR guidelines

[changeset-bot]

⚠️ No Changeset found

Latest commit: e98fd87

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

[reetp]

Before you submit a PR you should check in open whether your PR would be accepted.

It will save you a lot of wasted time and energy.

I suspect this is another case of stopping admins 'meddling' with other peoples stuff in the name of 'privacy'.

However, I do believe Rocket should remember who is actually owner of the server and let them decide what they can or can't do. What happens if the private chat is NSFW and the admin wants to delete it because it is hosting illegal material ?

So it may be appropriate to restrict admin permissions in a 'public' server.

But many orgs there are no 'public' users, and there is no such thing as 'private'.

If it is my company I will view what I want. I may be legally forced to do so (think GDPR etc). In which the many 'admin' restrictions are inappropriate.

Possibly there should be an unrestricted 'super admin' role above admin.

[sudhir-sars]

Thank you for your feedback and for raising the important issue of balancing admin permissions with privacy. The intent of this PR is to resolve a specific bug (#16954) by refining the permission checks and error handling for the groups.delete API call.

I also want to mention that I’m new to this open-source community and still learning how to submit PRs that align with the community’s standards and expectations. If there are specific changes or improvements I can make to this PR to ensure it gets accepted, I would be incredibly grateful for your guidance.

I completely understand the need to consider broader implications, such as scenarios involving sensitive or illegal content in private groups. Your suggestion of a "super admin" role is a valuable idea and could be worth exploring as part of a future enhancement.

For now, this PR aims to address the immediate issue without making broader changes to the permissions model. I’d appreciate your support in moving this forward, and I’m happy to contribute to further discussions or proposals on enhancing admin roles.

[casalsgh]

We'll review this with Product and Engineering