Skip to content

Comments

fix: check deleting user's unmuted status instead of message sender#38415

Merged
kodiakhq[bot] merged 6 commits intodevelopfrom
fix/delete-message-unmuted-check
Feb 5, 2026
Merged

fix: check deleting user's unmuted status instead of message sender#38415
kodiakhq[bot] merged 6 commits intodevelopfrom
fix/delete-message-unmuted-check

Conversation

@ricardogarim
Copy link
Contributor

@ricardogarim ricardogarim commented Jan 29, 2026

Proposed changes (including videos or screenshots)

Fixes incorrect permission check when deleting messages in read-only rooms with unmuted users. The canDeleteMessageAsync function was checking if the message sender (u.username) was in the room's unmuted list, when it should check if the user trying to delete is unmuted

Issue(s)

Steps to test or reproduce

Further comments

Summary by CodeRabbit

  • Bug Fixes

    • Fixed message deletion permission validation in read-only rooms to check the unmuted status of the user attempting to delete, rather than the message sender's status.
  • Tests

    • Added test coverage for message deletion in read-only rooms with unmuted and muted users.

✏️ Tip: You can customize this high-level summary in your review settings.

@dionisio-bot
Copy link
Contributor

dionisio-bot bot commented Jan 29, 2026

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

  • This PR is targeting the wrong base branch. It should target 8.2.0, but it targets 8.1.0

Please fix the issues and try again

If you have any trouble, please check the PR guidelines

@changeset-bot
Copy link

changeset-bot bot commented Jan 29, 2026

🦋 Changeset detected

Latest commit: 61dff8b

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 40 packages
Name Type
@rocket.chat/core-services Patch
@rocket.chat/meteor Patch
@rocket.chat/account-service Patch
@rocket.chat/authorization-service Patch
@rocket.chat/ddp-streamer Patch
@rocket.chat/omnichannel-transcript Patch
@rocket.chat/presence-service Patch
@rocket.chat/queue-worker Patch
@rocket.chat/abac Patch
@rocket.chat/federation-matrix Patch
@rocket.chat/network-broker Patch
@rocket.chat/omni-core-ee Patch
@rocket.chat/omnichannel-services Patch
@rocket.chat/presence Patch
rocketchat-services Patch
@rocket.chat/core-typings Patch
@rocket.chat/rest-typings Patch
@rocket.chat/uikit-playground Patch
@rocket.chat/api-client Patch
@rocket.chat/apps Patch
@rocket.chat/cron Patch
@rocket.chat/ddp-client Patch
@rocket.chat/fuselage-ui-kit Patch
@rocket.chat/gazzodown Patch
@rocket.chat/http-router Patch
@rocket.chat/livechat Patch
@rocket.chat/model-typings Patch
@rocket.chat/ui-avatar Patch
@rocket.chat/ui-client Patch
@rocket.chat/ui-contexts Patch
@rocket.chat/ui-voip Patch
@rocket.chat/web-ui-registration Patch
@rocket.chat/license Patch
@rocket.chat/media-calls Patch
@rocket.chat/pdf-worker Patch
@rocket.chat/models Patch
@rocket.chat/mock-providers Patch
@rocket.chat/ui-video-conf Patch
@rocket.chat/instance-status Patch
@rocket.chat/omni-core Patch

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

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Jan 29, 2026

Walkthrough

This pull request fixes a permission validation bug in read-only rooms by changing authorization functions to pass the full user object instead of just the userId. This ensures permission checks validate the deleting user's unmuted status rather than the message sender's status. The change propagates through upload service, deletion functions, and related APIs.

Changes

Cohort / File(s) Summary
Authorization Logic
apps/meteor/app/authorization/server/functions/canDeleteMessage.ts, apps/meteor/app/lib/server/functions/deleteMessage.ts
Updated canDeleteMessageAsync to receive deletingUser object instead of uid string; all internal references now use deletingUser._id and deletingUser.username. deleteMessage call site updated to pass user object instead of userId.
Upload Service
apps/meteor/server/services/upload/service.ts, apps/meteor/app/api/server/v1/uploads.ts
Changed canDeleteFile method signature to accept full user object instead of userId; all permission checks and user identity comparisons updated accordingly. Upload API endpoint updated to pass user object to canDeleteFile.
Service Interface
packages/core-services/src/types/IUploadService.ts
Updated IUploadService.canDeleteFile signature to accept user object instead of user ID as first parameter.
Test Coverage
apps/meteor/tests/end-to-end/api/chat.ts
Added comprehensive e2e test suite for read-only rooms with unmuted users, validating deletion permissions for both unmuted and non-unmuted users in read-only contexts.
Release Management
.changeset/smooth-dodos-add.md
Added changeset entry for patch releases documenting the fix to delete message permission validation in read-only rooms.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

  • RocketChat/Rocket.Chat#38173 — Originally introduced Upload.canDeleteFile with userId parameter; this PR updates that method signature to accept full user object to fix the permission check bug.

Suggested labels

stat: ready to merge, stat: QA assured

Suggested reviewers

  • pierre-lehnen-rc
  • tassoevan
  • sampaiodiego

Poem

🐰 A user was wrongly denied their right,
To delete in rooms locked tight,
But now with the fix in place,
The unmuted find their grace,
Permission checks now see the light! ✨

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately and concisely summarizes the primary fix: changing the permission check to validate the deleting user's unmuted status instead of the message sender's.
Linked Issues check ✅ Passed The PR fully addresses CORE-1670 requirements by changing canDeleteMessageAsync to check the deleting user's username in the unmuted list, allowing unmuted users with delete permissions to remove messages.
Out of Scope Changes check ✅ Passed All changes directly support the fix: updating canDeleteMessageAsync and related functions to pass full user objects, refactoring Upload.canDeleteFile to accept user objects, and adding comprehensive tests for unmuted users in read-only rooms.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch fix/delete-message-unmuted-check

Tip

🧪 Unit Test Generation v2 is now available!

We have significantly improved our unit test generation capabilities.

To enable: Add this to your .coderabbit.yaml configuration:

reviews:
  finishing_touches:
    unit_tests:
      enabled: true

Try it out by using the @coderabbitai generate unit tests command on your code files or under ✨ Finishing Touches on the walkthrough!

Have feedback? Share your thoughts on our Discord thread!


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.

❤️ Share

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

@codecov
Copy link

codecov bot commented Jan 29, 2026

Codecov Report

❌ Patch coverage is 0% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 70.34%. Comparing base (aaec97f) to head (61dff8b).
⚠️ Report is 1 commits behind head on develop.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##           develop   #38415      +/-   ##
===========================================
- Coverage    70.36%   70.34%   -0.02%     
===========================================
  Files         3161     3161              
  Lines       110653   110653              
  Branches     19857    19879      +22     
===========================================
- Hits         77860    77838      -22     
- Misses       30761    30778      +17     
- Partials      2032     2037       +5     
Flag Coverage Δ
e2e 60.30% <ø> (+0.01%) ⬆️
e2e-api 47.80% <0.00%> (-0.06%) ⬇️
unit 71.34% <ø> (-0.04%) ⬇️

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.

@github-actions
Copy link
Contributor

github-actions bot commented Jan 29, 2026

📦 Docker Image Size Report

➡️ Changes

Service Current Baseline Change Percent
sum of all images 0B 0B 0B
account-service 0B 0B 0B
authorization-service 0B 0B 0B
ddp-streamer-service 0B 0B 0B
omnichannel-transcript-service 0B 0B 0B
presence-service 0B 0B 0B
queue-worker-service 0B 0B 0B
rocketchat 0B 0B 0B

📊 Historical Trend

---
config:
  theme: "dark"
  xyChart:
    width: 900
    height: 400
---
xychart
  title "Image Size Evolution by Service (Last 30 Days + This PR)"
  x-axis ["11/18 22:53", "11/19 23:02", "11/21 16:49", "11/24 17:34", "11/27 22:32", "11/28 19:05", "12/01 23:01", "12/02 21:57", "12/03 21:00", "12/04 18:17", "12/05 21:56", "12/08 20:15", "12/09 22:17", "12/10 23:26", "12/11 21:56", "12/12 22:45", "12/13 01:34", "12/15 22:31", "12/16 22:18", "12/17 21:04", "12/18 23:12", "12/19 23:27", "12/20 21:03", "12/22 18:54", "12/23 16:16", "12/24 19:38", "12/25 17:51", "12/26 13:18", "12/29 19:01", "12/30 20:52", "02/05 10:15 (PR)"]
  y-axis "Size (GB)" 0 --> 0.5
  line "account-service" [0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.00]
  line "authorization-service" [0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.00]
  line "ddp-streamer-service" [0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.00]
  line "omnichannel-transcript-service" [0.14, 0.14, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.00]
  line "presence-service" [0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.00]
  line "queue-worker-service" [0.14, 0.14, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.00]
  line "rocketchat" [0.35, 0.35, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.00]
Loading

Statistics (last 30 days):

  • 📊 Average: 1.5GiB
  • ⬇️ Minimum: 1.4GiB
  • ⬆️ Maximum: 1.6GiB
  • 🎯 Current PR: 0B
ℹ️ About this report

This report compares Docker image sizes from this build against the develop baseline.

  • Tag: pr-38415
  • Baseline: develop
  • Timestamp: 2026-02-05 10:15:08 UTC
  • Historical data points: 30

Updated: Thu, 05 Feb 2026 10:15:09 GMT

@ricardogarim ricardogarim force-pushed the fix/delete-message-unmuted-check branch 2 times, most recently from 02169c6 to e5715fb Compare January 29, 2026 17:16
@ricardogarim ricardogarim force-pushed the fix/delete-message-unmuted-check branch from e5715fb to f3b02b6 Compare January 29, 2026 17:18
@ricardogarim ricardogarim force-pushed the fix/delete-message-unmuted-check branch from 4753784 to 7ac8c14 Compare January 29, 2026 17:35
@ricardogarim ricardogarim added this to the 8.2.0 milestone Jan 29, 2026
@ricardogarim ricardogarim marked this pull request as ready for review January 30, 2026 11:38
@ricardogarim ricardogarim requested a review from a team as a code owner January 30, 2026 11:38
Copy link
Contributor

@cubic-dev-ai cubic-dev-ai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No issues found across 7 files

@FragaKrummenauer FragaKrummenauer added the stat: QA assured Means it has been tested and approved by a company insider label Feb 4, 2026
@dionisio-bot dionisio-bot bot added the stat: ready to merge PR tested and approved waiting for merge label Feb 4, 2026
@kodiakhq kodiakhq bot merged commit 87faec1 into develop Feb 5, 2026
44 checks passed
@kodiakhq kodiakhq bot deleted the fix/delete-message-unmuted-check branch February 5, 2026 10:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

stat: QA assured Means it has been tested and approved by a company insider stat: ready to merge PR tested and approved waiting for merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants