fix: Wrong invite error message #37851
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
|
Looks like this PR is not ready to merge, because of the following issues:
Please fix the issues and try again If you have any trouble, please check the PR guidelines |
|
Contributor
WalkthroughRefactored ABAC error handling by introducing a dedicated Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ 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 |
Contributor
📦 Docker Image Size Report📈 Changes
📊 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/15 22:28", "11/16 01:28", "11/17 23:50", "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 15:31", "12/17 16:12 (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]
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]
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]
line "omnichannel-transcript-service" [0.14, 0.14, 0.14, 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]
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]
line "queue-worker-service" [0.14, 0.14, 0.14, 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]
line "rocketchat" [0.36, 0.36, 0.35, 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.35]
line "stream-hub-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]
Statistics (last 23 days):
ℹ️ About this reportThis report compares Docker image sizes from this build against the
Updated: Wed, 17 Dec 2025 16:12:50 GMT |
tassoevan
approved these changes
Dec 17, 2025
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
apps/meteor/app/slashcommands-invite/server/server.ts (1)
115-123: Error handling correctly differentiates ABAC error translation.The conditional logic appropriately handles the new ABAC error code by using
e.erroras the translation key (which contains the error code) instead ofe.message. This ensures the user-facing message is pulled from the i18n locales correctly.Optional future consideration: If additional ABAC error codes are introduced that require similar handling, consider refactoring to a more generic pattern (e.g., checking if
e.errorstarts with'error-'and exists in i18n keys).packages/i18n/src/locales/en.i18n.json (1)
6355-6382: Align ABAC error message style with existing locale conventionsBoth new strings work functionally, but you may want to tweak wording for consistency with nearby entries:
- Line 6355 (
error-only-compliant-users-can-be-added-to-abac-rooms): most short error messages in this file don’t end with a period (e.g., “Access not authorized”, “Cannot invite users to direct rooms”). Consider dropping the trailing.:- "error-only-compliant-users-can-be-added-to-abac-rooms": "Only compliant users can be added to ABAC rooms.", + "error-only-compliant-users-can-be-added-to-abac-rooms": "Only compliant users can be added to ABAC rooms",
- Line 6382 (
error-room-is-abac-managed): elsewhere in this file ABAC wording uses the hyphenated form “ABAC-managed” (e.g., “ABAC-managed rooms”). For consistency (and to mirror that usage), consider:- "error-room-is-abac-managed": "This room is ABAC managed and new users cannot be added", + "error-room-is-abac-managed": "This room is ABAC-managed and new users cannot be added",Both are purely style/copy suggestions; behavior is otherwise solid.
ee/packages/abac/src/index.ts (1)
466-490: Good refactoring to use a dedicated ABAC error.The replacement of
MeteorErrorwithOnlyCompliantCanBeAddedToRoomErrorimproves error handling by providing a specific, user-friendly error that aligns with design patterns. The error is thrown without details, which is appropriate for user-facing messages and prevents leaking internal information.Optional: Consider auditing denied access attempts.
Currently, only successful access grants are audited (lines 487-489). For compliance and security monitoring, you might want to audit denied attempts as well:
if (nonCompliantSet.size) { + nonCompliantUsersFromList.forEach((username) => { + void Audit.actionPerformed({ username }, { _id: objectId }, 'system', 'denied-object-access'); + }); throw new OnlyCompliantCanBeAddedToRoomError(); }This would provide a complete audit trail of both granted and denied access attempts.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
apps/meteor/app/slashcommands-invite/server/server.ts(1 hunks)apps/meteor/tests/end-to-end/api/abac.ts(1 hunks)ee/packages/abac/src/errors.ts(2 hunks)ee/packages/abac/src/index.ts(3 hunks)ee/packages/abac/src/service.spec.ts(3 hunks)packages/i18n/src/locales/en.i18n.json(2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
ee/packages/abac/src/errors.tsee/packages/abac/src/index.tsapps/meteor/app/slashcommands-invite/server/server.tsapps/meteor/tests/end-to-end/api/abac.tsee/packages/abac/src/service.spec.ts
**/*.spec.ts
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.spec.ts: Use descriptive test names that clearly communicate expected behavior in Playwright tests
Use.spec.tsextension for test files (e.g.,login.spec.ts)
Files:
ee/packages/abac/src/service.spec.ts
🧠 Learnings (15)
📓 Common learnings
Learnt from: MartinSchoeler
Repo: RocketChat/Rocket.Chat PR: 37557
File: apps/meteor/client/views/admin/ABAC/AdminABACRooms.tsx:115-116
Timestamp: 2025-11-27T17:56:26.050Z
Learning: In Rocket.Chat, the GET /v1/abac/rooms endpoint (implemented in ee/packages/abac/src/index.ts) only returns rooms where abacAttributes exists and is not an empty array (query: { abacAttributes: { $exists: true, $ne: [] } }). Therefore, in components consuming this endpoint (like AdminABACRooms.tsx), room.abacAttributes is guaranteed to be defined for all returned rooms, and optional chaining before calling array methods like .join() is sufficient without additional null coalescing.
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 37303
File: apps/meteor/tests/end-to-end/api/abac.ts:1125-1137
Timestamp: 2025-10-27T14:38:46.994Z
Learning: In Rocket.Chat ABAC feature, when ABAC is disabled globally (ABAC_Enabled setting is false), room-level ABAC attributes are not evaluated when changing room types. This means converting a private room to public will succeed even if the room has ABAC attributes, as long as the global ABAC setting is disabled.
Learnt from: ricardogarim
Repo: RocketChat/Rocket.Chat PR: 37377
File: apps/meteor/ee/server/hooks/federation/index.ts:86-88
Timestamp: 2025-11-04T16:49:19.107Z
Learning: In Rocket.Chat's federation system (apps/meteor/ee/server/hooks/federation/), permission checks follow two distinct patterns: (1) User-initiated federation actions (creating rooms, adding users to federated rooms, joining from invites) should throw MeteorError to inform users they lack 'access-federation' permission. (2) Remote server-initiated federation events should silently skip/ignore when users lack permission. The beforeAddUserToRoom hook only executes for local user-initiated actions, so throwing an error there is correct. Remote federation events are handled separately by the federation Matrix package with silent skipping logic.
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 37299
File: apps/meteor/ee/server/lib/ldap/Manager.ts:438-454
Timestamp: 2025-10-24T17:32:05.348Z
Learning: In Rocket.Chat, ABAC attributes can only be set on private rooms and teams (type 'p'), not on public rooms (type 'c'). Therefore, when checking for ABAC-protected rooms/teams during LDAP sync or similar operations, it's sufficient to query only private rooms using methods like `findPrivateRoomsByIdsWithAbacAttributes`.
Learnt from: ricardogarim
Repo: RocketChat/Rocket.Chat PR: 37205
File: ee/packages/federation-matrix/src/FederationMatrix.ts:296-301
Timestamp: 2025-10-28T16:53:42.761Z
Learning: In the Rocket.Chat federation-matrix integration (ee/packages/federation-matrix/), the createRoom method from rocket.chat/federation-sdk will support a 4-argument signature (userId, roomName, visibility, displayName) in newer versions. Code using this 4-argument call is forward-compatible with planned library updates and should not be flagged as an error.
Learnt from: ricardogarim
Repo: RocketChat/Rocket.Chat PR: 37205
File: ee/packages/federation-matrix/src/FederationMatrix.ts:296-301
Timestamp: 2025-10-28T16:53:42.761Z
Learning: In the Rocket.Chat federation-matrix integration (ee/packages/federation-matrix/), the createRoom method from rocket.chat/federation-sdk will support a 4-argument signature (userId, roomName, visibility, displayName) in newer versions. Code using this 4-argument call is forward-compatible with planned library updates and should not be flagged as an error.
📚 Learning: 2025-11-27T17:56:26.050Z
Learnt from: MartinSchoeler
Repo: RocketChat/Rocket.Chat PR: 37557
File: apps/meteor/client/views/admin/ABAC/AdminABACRooms.tsx:115-116
Timestamp: 2025-11-27T17:56:26.050Z
Learning: In Rocket.Chat, the GET /v1/abac/rooms endpoint (implemented in ee/packages/abac/src/index.ts) only returns rooms where abacAttributes exists and is not an empty array (query: { abacAttributes: { $exists: true, $ne: [] } }). Therefore, in components consuming this endpoint (like AdminABACRooms.tsx), room.abacAttributes is guaranteed to be defined for all returned rooms, and optional chaining before calling array methods like .join() is sufficient without additional null coalescing.
Applied to files:
ee/packages/abac/src/errors.tsee/packages/abac/src/index.tsapps/meteor/app/slashcommands-invite/server/server.tspackages/i18n/src/locales/en.i18n.jsonapps/meteor/tests/end-to-end/api/abac.tsee/packages/abac/src/service.spec.ts
📚 Learning: 2025-10-27T14:38:46.994Z
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 37303
File: apps/meteor/tests/end-to-end/api/abac.ts:1125-1137
Timestamp: 2025-10-27T14:38:46.994Z
Learning: In Rocket.Chat ABAC feature, when ABAC is disabled globally (ABAC_Enabled setting is false), room-level ABAC attributes are not evaluated when changing room types. This means converting a private room to public will succeed even if the room has ABAC attributes, as long as the global ABAC setting is disabled.
Applied to files:
ee/packages/abac/src/errors.tsee/packages/abac/src/index.tsapps/meteor/tests/end-to-end/api/abac.ts
📚 Learning: 2025-10-24T17:32:05.348Z
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 37299
File: apps/meteor/ee/server/lib/ldap/Manager.ts:438-454
Timestamp: 2025-10-24T17:32:05.348Z
Learning: In Rocket.Chat, ABAC attributes can only be set on private rooms and teams (type 'p'), not on public rooms (type 'c'). Therefore, when checking for ABAC-protected rooms/teams during LDAP sync or similar operations, it's sufficient to query only private rooms using methods like `findPrivateRoomsByIdsWithAbacAttributes`.
Applied to files:
ee/packages/abac/src/errors.tsee/packages/abac/src/index.tsapps/meteor/tests/end-to-end/api/abac.tsee/packages/abac/src/service.spec.ts
📚 Learning: 2025-11-04T16:49:19.107Z
Learnt from: ricardogarim
Repo: RocketChat/Rocket.Chat PR: 37377
File: apps/meteor/ee/server/hooks/federation/index.ts:86-88
Timestamp: 2025-11-04T16:49:19.107Z
Learning: In Rocket.Chat's federation system (apps/meteor/ee/server/hooks/federation/), permission checks follow two distinct patterns: (1) User-initiated federation actions (creating rooms, adding users to federated rooms, joining from invites) should throw MeteorError to inform users they lack 'access-federation' permission. (2) Remote server-initiated federation events should silently skip/ignore when users lack permission. The beforeAddUserToRoom hook only executes for local user-initiated actions, so throwing an error there is correct. Remote federation events are handled separately by the federation Matrix package with silent skipping logic.
Applied to files:
ee/packages/abac/src/index.tsapps/meteor/app/slashcommands-invite/server/server.ts
📚 Learning: 2025-10-28T16:53:42.761Z
Learnt from: ricardogarim
Repo: RocketChat/Rocket.Chat PR: 37205
File: ee/packages/federation-matrix/src/FederationMatrix.ts:296-301
Timestamp: 2025-10-28T16:53:42.761Z
Learning: In the Rocket.Chat federation-matrix integration (ee/packages/federation-matrix/), the createRoom method from rocket.chat/federation-sdk will support a 4-argument signature (userId, roomName, visibility, displayName) in newer versions. Code using this 4-argument call is forward-compatible with planned library updates and should not be flagged as an error.
Applied to files:
ee/packages/abac/src/index.tsapps/meteor/app/slashcommands-invite/server/server.ts
📚 Learning: 2025-11-07T14:50:33.544Z
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 37423
File: packages/i18n/src/locales/en.i18n.json:18-18
Timestamp: 2025-11-07T14:50:33.544Z
Learning: Rocket.Chat settings: in apps/meteor/ee/server/settings/abac.ts, the Abac_Cache_Decision_Time_Seconds setting uses invalidValue: 0 as the fallback when ABAC is unlicensed. With a valid license, admins can still set the value to 0 to intentionally disable the ABAC decision cache.
Applied to files:
ee/packages/abac/src/index.tsapps/meteor/tests/end-to-end/api/abac.ts
📚 Learning: 2025-12-02T22:23:49.593Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 37654
File: apps/meteor/client/hooks/useAppSlashCommands.ts:32-38
Timestamp: 2025-12-02T22:23:49.593Z
Learning: In apps/meteor/client/hooks/useAppSlashCommands.ts, the `data?.forEach((command) => slashCommands.add(command))` call during render is intentional. The query is configured with `structuralSharing: false` to prevent React Query from keeping stable data references, and `slashCommands.add` is idempotent, so executing on every render is acceptable and ensures the command registry stays current.
Applied to files:
apps/meteor/app/slashcommands-invite/server/server.ts
📚 Learning: 2025-11-19T12:32:29.696Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 37547
File: packages/i18n/src/locales/en.i18n.json:634-634
Timestamp: 2025-11-19T12:32:29.696Z
Learning: Repo: RocketChat/Rocket.Chat
Context: i18n workflow
Learning: In this repository, new translation keys should be added to packages/i18n/src/locales/en.i18n.json only; other locale files are populated via the external translation pipeline and/or fall back to English. Do not request adding the same key to all locale files in future reviews.
Applied to files:
packages/i18n/src/locales/en.i18n.json
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Use `expect` matchers for assertions (`toEqual`, `toContain`, `toBeTruthy`, `toHaveLength`, etc.) instead of `assert` statements in Playwright tests
Applied to files:
apps/meteor/tests/end-to-end/api/abac.tsee/packages/abac/src/service.spec.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Prefer web-first assertions (`toBeVisible`, `toHaveText`, etc.) in Playwright tests
Applied to files:
apps/meteor/tests/end-to-end/api/abac.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Ensure tests run reliably in parallel without shared state conflicts
Applied to files:
apps/meteor/tests/end-to-end/api/abac.tsee/packages/abac/src/service.spec.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Utilize Playwright fixtures (`test`, `page`, `expect`) for consistency in test files
Applied to files:
apps/meteor/tests/end-to-end/api/abac.tsee/packages/abac/src/service.spec.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to **/*.spec.ts : Use descriptive test names that clearly communicate expected behavior in Playwright tests
Applied to files:
ee/packages/abac/src/service.spec.ts
📚 Learning: 2025-12-10T21:00:43.645Z
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 37091
File: ee/packages/abac/jest.config.ts:4-7
Timestamp: 2025-12-10T21:00:43.645Z
Learning: Adopt the monorepo-wide Jest testMatch pattern: <rootDir>/src/**/*.spec.{ts,js,mjs} (represented here as '**/src/**/*.spec.{ts,js,mjs}') to ensure spec files under any package's src directory are picked up consistently across all packages in the Rocket.Chat monorepo. Apply this pattern in jest.config.ts for all relevant packages to maintain uniform test discovery.
Applied to files:
ee/packages/abac/src/service.spec.ts
🧬 Code graph analysis (1)
ee/packages/abac/src/index.ts (1)
ee/packages/abac/src/errors.ts (1)
OnlyCompliantCanBeAddedToRoomError(89-93)
⏰ 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: 🔨 Test UI (CE) / MongoDB 8.2 (1/4)
- GitHub Check: 🔨 Test UI (EE) / MongoDB 8.2 [legacy watchers] coverage (2/5)
🔇 Additional comments (5)
apps/meteor/tests/end-to-end/api/abac.ts (1)
1564-1564: LGTM! Test assertion updated to match new error payload structure.The test now correctly validates the
errorTypefield with the new standardized error codeerror-only-compliant-users-can-be-added-to-abac-rooms, which is more descriptive than the previous error message check.ee/packages/abac/src/service.spec.ts (2)
1079-1091: LGTM! Test updated to validate new error structure.The test now correctly expects a
codefield with valueerror-only-compliant-users-can-be-added-to-abac-roomsinstead of the previous error/message/details structure, aligning with the newOnlyCompliantCanBeAddedToRoomErrorclass.
1119-1121: LGTM! Consistent error validation.The assertion correctly validates the error code in the rejection scenario, maintaining consistency with the other updated test case.
ee/packages/abac/src/errors.ts (1)
12-12: LGTM! Well-structured error handling.The new
OnlyCompliantCanBeAddedToRoomErrorclass and corresponding error code follow the established patterns in this file. The error code is descriptive and will serve as the i18n key for user-facing messages.Also applies to: 89-93
ee/packages/abac/src/index.ts (1)
28-28: LGTM! Correct import for the new error class.
MartinSchoeler
pushed a commit
that referenced
this pull request
Dec 17, 2025
This was referenced Dec 18, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Proposed changes (including videos or screenshots)
Issue(s)
https://rocketchat.atlassian.net/browse/ABAC-100
Steps to test or reproduce
Further comments
Summary by CodeRabbit
Release Notes
Bug Fixes
New Features
✏️ Tip: You can customize this high-level summary in your review settings.