chore!: remove addUserToRole and removeUserFromRole

[juliajforesti]

ARCH-1801

Proposed changes (including videos or screenshots)

Issue(s)

Steps to test or reproduce

Further comments

Summary by CodeRabbit

• Bug Fixes

  • Stricter input validation for adding/removing roles with clearer errors when role or user is missing or invalid.

• Refactor

  • Role resolution now strictly uses role ID (no name-based fallback); permissions, admin protections, and UI/broadcast behavior unchanged.

• Chores

  • Added a deprecation warning for the setAdminStatus pathway.

[dionisio-bot]

Looks like this PR is ready to merge! 🎉
If you have any trouble, please check the PR guidelines

[changeset-bot]

⚠️ No Changeset found

Latest commit: b7d3c13

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

[coderabbitai]

Walkthrough

Removed ddp-client method augmentations for add/remove role methods, added strict argument validation, and changed role lookup to ID-only resolution (no name fallback). Added a deprecation log call to setAdminStatus. User resolution, admin protections, broadcasts, and return values are unchanged.

Changes

Cohort / File(s)	Summary
Authorization — add/remove methods apps/meteor/app/authorization/server/methods/addUserToRole.ts, apps/meteor/app/authorization/server/methods/removeUserFromRole.ts	Removed @rocket.chat/ddp-client server method augmentations and deprecation-logger fallback; added strict argument validation for roleId and username; replaced role lookup with ID-only resolution via Roles.findOneById and throw error-invalid-role if missing; preserved user resolution, admin protections, permission checks, role add/remove operations, UI/federation broadcasts, and return values.
Admin status deprecation logging apps/meteor/app/lib/server/methods/setAdminStatus.ts	Added methodDeprecationLogger.method call to log setAdminStatus as deprecated (points to REST endpoints /v1/roles.addUserToRole and /v1/roles.removeUserFromRole, version 8.0.0) before input validation; no signature or behavioral changes beyond the added side-effect.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  actor C as Client
  participant S as addUserToRole
  participant R as Roles
  participant U as Users
  participant A as Authorization
  participant B as Broadcast/UI

  C->>S: invoke(roleId, username, scope)
  S->>S: validate roleId, username (types)
  S->>R: findOneById(roleId)
  R-->>S: role | null
  alt role not found
    S-->>C: error-invalid-role
  else role found
    S->>U: findOneByUsernameInsensitive(username)
    U-->>S: user | null
    alt user not found
      S-->>C: error-invalid-user
    else user found
      S->>A: check canAddUserToRole(user, role, scope)
      alt not allowed
        S-->>C: error-not-allowed
      else allowed
        S->>R: addUserRolesAsync(user, role, scope)
        R-->>S: result
        S->>B: UI update + federation broadcast
        S-->>C: result
      end
    end
  end

Loading

sequenceDiagram
  autonumber
  actor C as Client
  participant S as removeUserFromRole
  participant R as Roles
  participant U as Users
  participant A as Authorization
  participant B as Broadcast/UI

  C->>S: invoke(roleId, username, scope)
  S->>S: validate roleId, username (types)
  S->>R: findOneById(roleId)
  R-->>S: role | null
  alt role not found
    S-->>C: error-invalid-role
  else role found
    S->>U: findOneByUsernameInsensitive(username)
    U-->>S: user | null
    alt user not found
      S-->>C: error-invalid-user
    else user found
      S->>A: check canRemoveUserFromRole(user, role, scope)
      alt not allowed or last-admin guard
        S-->>C: error-not-allowed
      else allowed
        S->>R: removeUserFromRoleAsync(user, role, scope)
        R-->>S: result
        S->>B: UI update + federation broadcast
        S-->>C: result
      end
    end
  end

Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Poem

I twitch my nose at tidy code tonight,
IDs now lead where names took flight.
A gentle log reminds of paths retired,
Roles shifted clean, broadcasts inspired.
Hop on, small changes—carrots bright. 🥕🐇

✨ Finishing touches

• 📝 Generate Docstrings

🧪 Generate unit tests

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch remove/removeUserFromRole

---

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 674ac07 and fd5e184.

📒 Files selected for processing (1)

• apps/meteor/app/lib/server/methods/setAdminStatus.ts (2 hunks)

🚧 Files skipped from review as they are similar to previous changes (1)

• apps/meteor/app/lib/server/methods/setAdminStatus.ts

⏰ 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). (2)

• GitHub Check: CodeQL-Build
• GitHub Check: CodeQL-Build

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

Pre-merge checks

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Docstring Coverage	✅ Passed	No functions found in the changes. Docstring coverage check skipped.
Title Check	✅ Passed	The title is concise and accurately summarizes the primary change — removal of the addUserToRole and removeUserFromRole methods — which matches the changeset that removes those server method declarations and associated deprecation/fallback logic, so it clearly communicates the main intent of the PR.

[coderabbitai]

Actionable comments posted: 2

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 5467e46 and 05eefba.

📒 Files selected for processing (5)

• .changeset/cold-moons-kiss.md (1 hunks)
• apps/meteor/app/authorization/server/methods/removeUserFromRole.ts (0 hunks)
• apps/meteor/app/bot-helpers/server/index.ts (0 hunks)
• apps/meteor/app/lib/server/methods/setAdminStatus.ts (0 hunks)
• apps/meteor/server/methods/removeUserFromRoom.ts (0 hunks)

💤 Files with no reviewable changes (4)

• apps/meteor/app/lib/server/methods/setAdminStatus.ts
• apps/meteor/app/bot-helpers/server/index.ts
• apps/meteor/app/authorization/server/methods/removeUserFromRole.ts
• apps/meteor/server/methods/removeUserFromRoom.ts

⏰ 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). (2)

• GitHub Check: 📦 Build Packages
• GitHub Check: CodeQL-Build

🔇 Additional comments (1)

.changeset/cold-moons-kiss.md (1)

1-3: Changeset header looks correct.

Package scope and release bump are well-formed for a major change.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 66.38%. Comparing base (3c42f0d) to head (fd5e184).
⚠️ Report is 94 commits behind head on release-8.0.0.

Additional details and impacted files

@@                Coverage Diff                @@
##           release-8.0.0   #36935      +/-   ##
=================================================
- Coverage          66.43%   66.38%   -0.06%     
=================================================
  Files               3276     3312      +36     
  Lines             109596   111670    +2074     
  Branches           20860    20944      +84     
=================================================
+ Hits               72814    74133    +1319     
- Misses             34116    34860     +744     
- Partials            2666     2677      +11     

Flag	Coverage Δ
unit	70.88% <ø> (-0.69%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[coderabbitai]

Actionable comments posted: 2

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/methods/removeUserFromRole.ts (1)

44-59: Missing permission gate when removing the admin role.

Users with generic 'access-permissions' can strip 'admin' from others unless blocked by "last admin" rule. Require 'assign-admin-role' here, mirroring addUserToRole.

   // prevent removing last user from admin role
   if (role._id === 'admin') {
+    // require elevated permission to modify the 'admin' role
+    if (!(await hasPermissionAsync(userId, 'assign-admin-role'))) {
+      throw new Meteor.Error('error-action-not-allowed', 'Removing admin is not allowed', {
+        method: 'authorization:removeUserFromRole',
+        action: 'Remove_admin',
+      });
+    }
+
     const adminCount = await Users.countDocuments({
       roles: {
         $in: ['admin'],
       },
     });
 
-    const userIsAdmin = user.roles?.indexOf('admin') > -1;
+    const userIsAdmin = user.roles?.includes('admin') === true;
     if (adminCount === 1 && userIsAdmin) {
       throw new Meteor.Error('error-action-not-allowed', 'Leaving the app without admins is not allowed', {
-        method: 'removeUserFromRole',
+        method: 'authorization:removeUserFromRole',
         action: 'Remove_last_admin',
       });
     }
   }

🧹 Nitpick comments (3)

apps/meteor/app/authorization/server/methods/addUserToRole.ts (1)

18-22: Simplify argument validation; avoid valueOf().

Prefer direct string checks for clarity and to reject empty strings.

- if (!roleId || typeof roleId.valueOf() !== 'string' || !username || typeof username.valueOf() !== 'string') {
+ if (typeof roleId !== 'string' || roleId.length === 0 || typeof username !== 'string' || username.length === 0) {

apps/meteor/app/authorization/server/methods/removeUserFromRole.ts (2)

10-16: Minor copy fix in error message.

Grammar tweak for consistency with addUserToRole.

- throw new Meteor.Error('error-action-not-allowed', 'Access permissions is not allowed', {
+ throw new Meteor.Error('error-action-not-allowed', 'Accessing permissions is not allowed', {

---

54-57: Align 'method' field naming.

Use the same 'authorization:removeUserFromRole' identifier as elsewhere.

- method: 'removeUserFromRole',
+ method: 'authorization:removeUserFromRole',

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 05eefba and b7d3c13.

📒 Files selected for processing (2)

• apps/meteor/app/authorization/server/methods/addUserToRole.ts (1 hunks)
• apps/meteor/app/authorization/server/methods/removeUserFromRole.ts (1 hunks)

⏰ 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). (5)

• GitHub Check: check
• GitHub Check: 📦 Build Packages
• GitHub Check: CodeQL-Build
• GitHub Check: Builds matrix rust bindings against alpine
• GitHub Check: CodeQL-Build

[coderabbitai]

Actionable comments posted: 0

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/lib/server/methods/setAdminStatus.ts (1)

45-51: Pass roleId (not role name) to addUserToRole/removeUserFromRole

addUserToRole/removeUserFromRole signatures are (userId: string, roleId: string, username: IUser['username'], scope?: string) (see apps/meteor/app/authorization/server/methods/{addUserToRole,removeUserFromRole}.ts). The code in apps/meteor/app/lib/server/methods/setAdminStatus.ts (lines ~45–51) still passes the literal 'admin' as the role — resolve the admin role's id and pass that roleId as the second argument; the username argument (user?.username) is correct.

🧹 Nitpick comments (1)

apps/meteor/app/lib/server/methods/setAdminStatus.ts (1)

21-25: Confirm deprecation target version (8.0.0) and placement of the log

• If this ships in release-8.0.0, “will be removed on version 8.0.0” can be misleading. Confirm whether removal is actually in 8.0.0 or the next major (e.g., 9.0.0) and adjust accordingly; otherwise remove the method now or gate it.
• Optional: consider logging after auth/permission checks to avoid unauthenticated probes inflating deprecation metrics.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between b7d3c13 and 674ac07.

📒 Files selected for processing (1)

• apps/meteor/app/lib/server/methods/setAdminStatus.ts (2 hunks)

🧰 Additional context used

🧬 Code graph analysis (1)

apps/meteor/app/lib/server/methods/setAdminStatus.ts (1)

apps/meteor/app/lib/server/lib/deprecationWarningLogger.ts (1)

• methodDeprecationLogger (90-137)

⏰ 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). (3)

• GitHub Check: 📦 Build Packages
• GitHub Check: CodeQL-Build
• GitHub Check: CodeQL-Build

🔇 Additional comments (1)

apps/meteor/app/lib/server/methods/setAdminStatus.ts (1)

10-10: Deprecation logger import — LGTM

Import path and named export look correct for the existing helper.