Skip to content

Conversation

@ricardogarim
Copy link
Contributor

@ricardogarim ricardogarim commented Nov 4, 2025

Proposed changes (including videos or screenshots)

As per FDR-251, this change introduces a new permission called access-federation. This permission controls whether a user is allowed to perform federation-related actions — specifically creating or joining a federated room, which are the main entry points for users interacting with federation resources.

If a user does not have this permission, an error indicating a lack of permission will be displayed. For users without the permission who receive an invite, the system will return controller level error. Implementing a rejection response would require refactoring parts of the code, so for now, the invite will remain open without a response.

Issue(s)

Steps to test or reproduce

Screen.Recording.2025-11-06.at.08.27.53.mov

Summary by CodeRabbit

  • New Features

    • Added a new "Access Federation" permission.
  • Bug Fixes / Security

    • Enforced federation authorization for creating, joining, and inviting to federated rooms; unauthorized users now receive a federation-specific authorization error.
  • Documentation

    • Added localized strings for the new permission and its authorization error message.

@dionisio-bot
Copy link
Contributor

dionisio-bot bot commented Nov 4, 2025

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

@changeset-bot
Copy link

changeset-bot bot commented Nov 4, 2025

⚠️ No Changeset found

Latest commit: d2e735c

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
Copy link
Contributor

coderabbitai bot commented Nov 4, 2025

Walkthrough

Adds an access-federation permission and enforces it at federation entry points (create room, join, invites, federation hooks, Matrix invite API), returning error-not-authorized-federation / NotAllowedError when the permission is missing.

Changes

Cohort / File(s) Summary
Permissions & i18n
apps/meteor/app/authorization/server/constant/permissions.ts, packages/i18n/src/locales/en.i18n.json
Adds access-federation permission (roles: admin, user) and i18n keys access-federation, access-federation_description, and error-not-authorized-federation.
Room creation flow
apps/meteor/app/lib/server/functions/createRoom.ts
Adds federation detection (isRoomNativeFederated), checks hasPermissionAsync for access-federation, throws error-not-authorized-federation when unauthorized, and passes federation flag into subscription creation.
Room join service
apps/meteor/server/services/room/service.ts
Adds federation authorization gate in RoomService.join, throwing error-not-authorized-federation when user lacks access-federation.
EE federation hooks
apps/meteor/ee/server/hooks/federation/index.ts
Adds Authorization.hasPermission check before inviting users to federated rooms; throws Meteor.Error('error-not-authorized-federation', ...) on denial.
Matrix invite API (EE)
ee/packages/federation-matrix/src/api/_matrix/invite.ts
Adds Authorization import and module logger; enforces access-federation check returning 403/M_FORBIDDEN on denial; treats NotAllowedError as non-retriable in backoff logic and logs authorizations.

Sequence Diagram(s)

sequenceDiagram
    participant User
    participant App as AppServer
    participant Auth as Authorization
    participant Fed as FederationService

    User->>App: Request federated action (create / join / invite)
    App->>Auth: hasPermission(userId, "access-federation")?
    alt allowed
        Auth-->>App: allowed
        App->>Fed: perform federation operation
        Fed-->>App: success
        App-->>User: success
    else denied
        Auth-->>App: denied
        App-->>User: error-not-authorized-federation / 403 (NotAllowedError)
    end
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

  • Check consistent error codes/messages across createRoom.ts, RoomService.join, federation hooks, and Matrix API.
  • Review that all federation entry points are covered and cannot be bypassed.
  • Inspect backoff/retry handling in ee/packages/federation-matrix/src/api/_matrix/invite.ts to ensure NotAllowedError is not retried.

Possibly related PRs

Suggested labels

stat: ready to merge, stat: QA assured

Suggested reviewers

  • rodrigok
  • ggazzo
  • sampaiodiego

Poem

🐰 I hopped through code to plant a key,
"access-federation" now guards the tree,
At create, join, and invite I stand,
A polite gatekeeper with carrot in hand,
Hops of safety across the land.

Pre-merge checks and finishing touches

✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The PR title accurately describes the main change: adding a new access-federation permission to govern federation-related actions.
Linked Issues check ✅ Passed The PR introduces the access-federation permission as required by FDR-251, implementing authorization checks for federation operations across multiple entry points.
Out of Scope Changes check ✅ Passed All changes are scoped to implementing the access-federation permission; localization, permission constants, and authorization checks are all directly related to the stated objective.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
✨ 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 feat/federation-permissions

📜 Recent review details

Configuration used: CodeRabbit 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.

📥 Commits

Reviewing files that changed from the base of the PR and between 6daa046 and d2e735c.

📒 Files selected for processing (6)
  • apps/meteor/app/authorization/server/constant/permissions.ts (1 hunks)
  • apps/meteor/app/lib/server/functions/createRoom.ts (3 hunks)
  • apps/meteor/ee/server/hooks/federation/index.ts (2 hunks)
  • apps/meteor/server/services/room/service.ts (1 hunks)
  • ee/packages/federation-matrix/src/api/_matrix/invite.ts (4 hunks)
  • packages/i18n/src/locales/en.i18n.json (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
  • apps/meteor/server/services/room/service.ts
  • packages/i18n/src/locales/en.i18n.json
🧰 Additional context used
🧠 Learnings (9)
📓 Common learnings
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: 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-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:

  • apps/meteor/app/authorization/server/constant/permissions.ts
  • ee/packages/federation-matrix/src/api/_matrix/invite.ts
  • apps/meteor/app/lib/server/functions/createRoom.ts
  • apps/meteor/ee/server/hooks/federation/index.ts
📚 Learning: 2025-09-19T15:15:04.642Z
Learnt from: rodrigok
Repo: RocketChat/Rocket.Chat PR: 36991
File: apps/meteor/server/services/federation/infrastructure/rocket-chat/adapters/Settings.ts:219-221
Timestamp: 2025-09-19T15:15:04.642Z
Learning: The Federation_Matrix_homeserver_domain setting in apps/meteor/server/services/federation/infrastructure/rocket-chat/adapters/Settings.ts is part of the old federation system and is being deprecated/removed, so configuration issues with this setting should not be flagged for improvement.

Applied to files:

  • apps/meteor/app/authorization/server/constant/permissions.ts
  • ee/packages/federation-matrix/src/api/_matrix/invite.ts
  • apps/meteor/app/lib/server/functions/createRoom.ts
  • apps/meteor/ee/server/hooks/federation/index.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/federation-matrix/src/api/_matrix/invite.ts
  • apps/meteor/app/lib/server/functions/createRoom.ts
  • apps/meteor/ee/server/hooks/federation/index.ts
📚 Learning: 2025-09-25T09:59:26.461Z
Learnt from: Dnouv
Repo: RocketChat/Rocket.Chat PR: 37057
File: packages/apps-engine/src/definition/accessors/IUserRead.ts:23-27
Timestamp: 2025-09-25T09:59:26.461Z
Learning: AppUserBridge.getUserRoomIds in apps/meteor/app/apps/server/bridges/users.ts always returns an array of strings by mapping subscription documents to room IDs, never undefined, even when user has no room subscriptions.

Applied to files:

  • ee/packages/federation-matrix/src/api/_matrix/invite.ts
  • apps/meteor/app/lib/server/functions/createRoom.ts
📚 Learning: 2025-09-25T09:59:26.461Z
Learnt from: Dnouv
Repo: RocketChat/Rocket.Chat PR: 37057
File: packages/apps-engine/src/definition/accessors/IUserRead.ts:23-27
Timestamp: 2025-09-25T09:59:26.461Z
Learning: AppUserBridge.getUserRoomIds in apps/meteor/app/apps/server/bridges/users.ts always returns an array of strings (mapping subscription documents to room IDs), never undefined, even when user has no room subscriptions.

Applied to files:

  • ee/packages/federation-matrix/src/api/_matrix/invite.ts
  • apps/meteor/app/lib/server/functions/createRoom.ts
📚 Learning: 2025-09-25T09:59:26.461Z
Learnt from: Dnouv
Repo: RocketChat/Rocket.Chat PR: 37057
File: packages/apps-engine/src/definition/accessors/IUserRead.ts:23-27
Timestamp: 2025-09-25T09:59:26.461Z
Learning: UserBridge.doGetUserRoomIds in packages/apps-engine/src/server/bridges/UserBridge.ts has a bug where it implicitly returns undefined when the app lacks read permission (missing return statement in the else case of the permission check).

Applied to files:

  • ee/packages/federation-matrix/src/api/_matrix/invite.ts
  • apps/meteor/app/lib/server/functions/createRoom.ts
  • apps/meteor/ee/server/hooks/federation/index.ts
📚 Learning: 2025-11-05T21:04:35.767Z
Learnt from: sampaiodiego
Repo: RocketChat/Rocket.Chat PR: 37357
File: ee/packages/federation-matrix/src/setup.ts:103-120
Timestamp: 2025-11-05T21:04:35.767Z
Learning: In Rocket.Chat's federation-matrix setup (ee/packages/federation-matrix/src/setup.ts and apps/meteor/ee/server/startup/federation.ts), configureFederationMatrixSettings does not need to be called before setupFederationMatrix. The SDK's init() establishes infrastructure (database, event handlers, APIs) first, and the configuration can be applied later via settings watchers before actual federation events are processed. The config only matters when events actually occur, at which point all infrastructure is already configured.

Applied to files:

  • ee/packages/federation-matrix/src/api/_matrix/invite.ts
  • apps/meteor/app/lib/server/functions/createRoom.ts
  • apps/meteor/ee/server/hooks/federation/index.ts
📚 Learning: 2025-09-15T06:21:00.139Z
Learnt from: Dnouv
Repo: RocketChat/Rocket.Chat PR: 36868
File: apps/meteor/app/apps/server/bridges/serverEndpoints.ts:35-48
Timestamp: 2025-09-15T06:21:00.139Z
Learning: In ServerEndpointsBridge.ts, the impersonation permission (`server-endpoints.impersonate`) is enforced at lines 48-49 specifically when `info.user` is provided without `info.user.token`, which represents the impersonation scenario where a token needs to be minted. Token pass-through (when both user ID and token are provided) doesn't require the impersonate permission since the app already possesses a valid token.

Applied to files:

  • apps/meteor/ee/server/hooks/federation/index.ts
🧬 Code graph analysis (3)
ee/packages/federation-matrix/src/api/_matrix/invite.ts (2)
ee/packages/media-calls/src/logger.ts (1)
  • logger (3-3)
apps/meteor/app/livechat/server/methods/closeRoom.ts (1)
  • roomId (46-96)
apps/meteor/app/lib/server/functions/createRoom.ts (1)
packages/core-typings/src/IRoom.ts (1)
  • isRoomNativeFederated (124-125)
apps/meteor/ee/server/hooks/federation/index.ts (1)
packages/core-services/src/index.ts (1)
  • MeteorError (56-56)
⏰ 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). (8)
  • GitHub Check: 🔨 Test Unit / Unit Tests
  • GitHub Check: 🔨 Test Storybook / Test Storybook
  • GitHub Check: 🔎 Code Check / Code Lint
  • GitHub Check: 🔎 Code Check / TypeScript
  • GitHub Check: 📦 Meteor Build (coverage)
  • GitHub Check: 📦 Meteor Build (production)
  • GitHub Check: CodeQL-Build
  • GitHub Check: CodeQL-Build
🔇 Additional comments (7)
apps/meteor/app/lib/server/functions/createRoom.ts (2)

29-63: LGTM! Federation subscription handling is well-structured.

The shouldBeHandledByFederation parameter properly gates the subscription creation logic, creating federation-specific subscriptions with owner role for federated rooms while maintaining the existing multi-member subscription flow for non-federated rooms.


167-172: Now let me verify who actually calls createRoom with type='d' to confirm if owner can realistically be undefined:

Based on my thorough verification of the codebase, I can now generate the rewritten review comment.

Federation permission check doesn't account for direct messages with undefined owner.

The permission check at lines 167-172 requires both shouldBeHandledByFederation AND owner to be defined:

if (shouldBeHandledByFederation && owner && !(await hasPermissionAsync(owner._id, 'access-federation'))) {

However, the function signature (line 126) permits owner: IUser | undefined for direct messages (type === 'd'). When:

  1. A federated direct message is created with an undefined owner
  2. options?.creator is also undefined
  3. The permission check is skipped (short-circuit evaluation of && owner)
  4. The DM is created and passed to createDirectRoom (line 175)
  5. The afterCreateDirectRoom callback runs with creatorId: undefined
  6. The federation hook fails at runtime when trying to access the undefined user

Move the federation permission check to createDirectRoom.ts after line 181 when the callback runs, ensuring the check applies regardless of how createRoom was called. Alternatively, require owner to always be defined when shouldBeHandledByFederation is true, updating the function's type constraints.

apps/meteor/ee/server/hooks/federation/index.ts (1)

86-88: LGTM! Permission check correctly throws error for user-initiated actions.

The permission enforcement is correct and follows the established pattern: throwing MeteorError for local user-initiated actions to inform users they lack the required permission. Remote server-initiated federation events are handled separately by the federation Matrix package with silent skipping logic.

Based on learnings.

ee/packages/federation-matrix/src/api/_matrix/invite.ts (3)

144-148: LGTM! NotAllowedError handling prevents unnecessary retries.

The enhanced error handling correctly identifies authorization errors (NotAllowedError) as non-retriable, preventing exponential backoff loops for permission-denied scenarios. The logging provides clear visibility into authorization failures.


357-368: LGTM! Federation permission properly enforced before invite processing.

The permission check correctly gates federation invite processing, returning a 403 with an appropriate error code (M_FORBIDDEN) when the user lacks access-federation permission. The informational logging aids in debugging authorization issues.


13-13: The logger at line 313 is actively used and not redundant.

The function-scoped logger at line 313 ('matrix-invite') is used within the getMatrixInviteRoutes function at lines 359 and 407. The module-level logger at line 13 ('federation-matrix:invite') is used separately in the retry/backoff logic at lines 146 and 151. Both loggers serve distinct purposes with proper scoping—this is not code duplication.

Likely an incorrect or invalid review comment.

apps/meteor/app/authorization/server/constant/permissions.ts (1)

247-247: LGTM! New federation permission properly defined.

The access-federation permission is correctly added with appropriate roles (admin and user), enabling fine-grained control over federation access. The placement after view-federation-data maintains logical grouping of federation-related permissions.


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 Nov 4, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 67.08%. Comparing base (276ec51) to head (d2e735c).
⚠️ Report is 14 commits behind head on develop.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##           develop   #37377      +/-   ##
===========================================
- Coverage    67.08%   67.08%   -0.01%     
===========================================
  Files         3419     3419              
  Lines       117938   117942       +4     
  Branches     21578    21578              
===========================================
+ Hits         79114    79116       +2     
- Misses       36132    36138       +6     
+ Partials      2692     2688       -4     
Flag Coverage Δ
e2e 57.49% <ø> (+0.01%) ⬆️
unit 72.14% <ø> (-0.01%) ⬇️

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.

@ricardogarim ricardogarim marked this pull request as ready for review November 4, 2025 16:07
@ricardogarim ricardogarim requested a review from a team as a code owner November 4, 2025 16:07
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Nitpick comments (1)
packages/i18n/src/locales/en.i18n.json (1)

5960-5960: Optional: align capitalization with similar labels

Consider “Access federation” to match “Access marketplace” style, unless “Federation” is a proper feature name you want capitalized.

📜 Review details

Configuration used: CodeRabbit 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.

📥 Commits

Reviewing files that changed from the base of the PR and between c61d14d and ea30f0e.

📒 Files selected for processing (6)
  • apps/meteor/app/authorization/server/constant/permissions.ts (1 hunks)
  • apps/meteor/app/lib/server/functions/createRoom.ts (3 hunks)
  • apps/meteor/ee/server/hooks/federation/index.ts (2 hunks)
  • apps/meteor/server/services/room/service.ts (1 hunks)
  • ee/packages/federation-matrix/src/api/_matrix/invite.ts (4 hunks)
  • packages/i18n/src/locales/en.i18n.json (2 hunks)
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
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-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:

  • apps/meteor/ee/server/hooks/federation/index.ts
  • packages/i18n/src/locales/en.i18n.json
  • apps/meteor/server/services/room/service.ts
  • ee/packages/federation-matrix/src/api/_matrix/invite.ts
  • apps/meteor/app/lib/server/functions/createRoom.ts
📚 Learning: 2025-09-19T15:15:04.642Z
Learnt from: rodrigok
Repo: RocketChat/Rocket.Chat PR: 36991
File: apps/meteor/server/services/federation/infrastructure/rocket-chat/adapters/Settings.ts:219-221
Timestamp: 2025-09-19T15:15:04.642Z
Learning: The Federation_Matrix_homeserver_domain setting in apps/meteor/server/services/federation/infrastructure/rocket-chat/adapters/Settings.ts is part of the old federation system and is being deprecated/removed, so configuration issues with this setting should not be flagged for improvement.

Applied to files:

  • apps/meteor/ee/server/hooks/federation/index.ts
  • apps/meteor/app/authorization/server/constant/permissions.ts
  • packages/i18n/src/locales/en.i18n.json
  • apps/meteor/server/services/room/service.ts
  • ee/packages/federation-matrix/src/api/_matrix/invite.ts
  • apps/meteor/app/lib/server/functions/createRoom.ts
📚 Learning: 2025-09-25T09:59:26.461Z
Learnt from: Dnouv
Repo: RocketChat/Rocket.Chat PR: 37057
File: packages/apps-engine/src/definition/accessors/IUserRead.ts:23-27
Timestamp: 2025-09-25T09:59:26.461Z
Learning: UserBridge.doGetUserRoomIds in packages/apps-engine/src/server/bridges/UserBridge.ts has a bug where it implicitly returns undefined when the app lacks read permission (missing return statement in the else case of the permission check).

Applied to files:

  • apps/meteor/server/services/room/service.ts
  • apps/meteor/app/lib/server/functions/createRoom.ts
📚 Learning: 2025-09-25T09:59:26.461Z
Learnt from: Dnouv
Repo: RocketChat/Rocket.Chat PR: 37057
File: packages/apps-engine/src/definition/accessors/IUserRead.ts:23-27
Timestamp: 2025-09-25T09:59:26.461Z
Learning: AppUserBridge.getUserRoomIds in apps/meteor/app/apps/server/bridges/users.ts always returns an array of strings by mapping subscription documents to room IDs, never undefined, even when user has no room subscriptions.

Applied to files:

  • ee/packages/federation-matrix/src/api/_matrix/invite.ts
  • apps/meteor/app/lib/server/functions/createRoom.ts
📚 Learning: 2025-09-25T09:59:26.461Z
Learnt from: Dnouv
Repo: RocketChat/Rocket.Chat PR: 37057
File: packages/apps-engine/src/definition/accessors/IUserRead.ts:23-27
Timestamp: 2025-09-25T09:59:26.461Z
Learning: AppUserBridge.getUserRoomIds in apps/meteor/app/apps/server/bridges/users.ts always returns an array of strings (mapping subscription documents to room IDs), never undefined, even when user has no room subscriptions.

Applied to files:

  • ee/packages/federation-matrix/src/api/_matrix/invite.ts
  • apps/meteor/app/lib/server/functions/createRoom.ts
🧬 Code graph analysis (3)
apps/meteor/ee/server/hooks/federation/index.ts (1)
packages/core-services/src/index.ts (1)
  • MeteorError (56-56)
apps/meteor/server/services/room/service.ts (1)
packages/core-services/src/index.ts (1)
  • Authorization (158-158)
apps/meteor/app/lib/server/functions/createRoom.ts (1)
packages/core-typings/src/IRoom.ts (1)
  • isRoomNativeFederated (124-125)
🔇 Additional comments (7)
packages/i18n/src/locales/en.i18n.json (2)

5960-5961: Permission strings look good

Labels/descriptions are clear and consistent with nearby entries.


6260-6260: Verification complete — error key and permission properly integrated

The new error key error-not-authorized-federation is correctly used across the codebase:

  • Referenced in 3 error throws with matching message text
  • Permission access-federation is checked at 4 gateway points (room join, room create, federation hooks, matrix invite)
  • Permission properly defined with roles ['admin', 'user']
  • I18n entries present for both permission and error message

No inconsistencies or missing usages found.

apps/meteor/ee/server/hooks/federation/index.ts (1)

86-88: Confirm federated stub users retain access permission

This hook also runs when we invite a remote Matrix user that we store as a local stub. Those stubs historically don’t inherit our user role, so Authorization.hasPermission could return false and block outbound invites. Please double-check that createOrUpdateFederatedUser (or the equivalent path) assigns a role carrying access-federation; otherwise cross-server invites will start failing.

apps/meteor/app/authorization/server/constant/permissions.ts (1)

247-248: Permission wiring looks good

Registering access-federation for the default admin and user roles keeps existing behaviour while allowing admins to tighten access when needed.

apps/meteor/server/services/room/service.ts (1)

117-119: Join flow guard in the right spot

Placing the access-federation check ahead of join-code validation ensures unauthorized users fail fast with the dedicated error.

ee/packages/federation-matrix/src/api/_matrix/invite.ts (1)

181-185: Invite acceptance respects the new permission

Guarding joinRoom with Authorization.hasPermission keeps unattended backoff retries from persisting invites for users who shouldn’t federate.

apps/meteor/app/lib/server/functions/createRoom.ts (1)

167-172: Creation gate aligns with federation policy

The isRoomNativeFederated check paired with hasPermissionAsync cleanly blocks users lacking federation access before we fan out to the rest of the create pipeline.

@ricardogarim ricardogarim force-pushed the feat/federation-permissions branch from ea30f0e to b9d4c21 Compare November 4, 2025 16:32
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

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

279-279: Critical: Undefined variable shouldBeHandledByFederation.

The variable shouldBeHandledByFederation is used on lines 279 and 294 but is never defined in this function. According to the AI summary, the logic that computed this variable was removed. This will cause a ReferenceError at runtime.

You need to either:

  1. Recompute shouldBeHandledByFederation before line 279, likely as const shouldBeHandledByFederation = isRoomNativeFederated(extraData);
  2. Or inline the check where needed

Apply this diff to fix the issue:

 	});
 
+	const shouldBeHandledByFederation = isRoomNativeFederated(extraData);
+
 	await createUsersSubscriptions({ room, members, now, owner, options, shouldBeHandledByFederation });

Also applies to: 294-294

📜 Review details

Configuration used: CodeRabbit 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.

📥 Commits

Reviewing files that changed from the base of the PR and between ea30f0e and b9d4c21.

📒 Files selected for processing (6)
  • apps/meteor/app/authorization/server/constant/permissions.ts (1 hunks)
  • apps/meteor/app/lib/server/functions/createRoom.ts (3 hunks)
  • apps/meteor/ee/server/hooks/federation/index.ts (2 hunks)
  • apps/meteor/server/services/room/service.ts (1 hunks)
  • ee/packages/federation-matrix/src/api/_matrix/invite.ts (4 hunks)
  • packages/i18n/src/locales/en.i18n.json (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
  • apps/meteor/app/authorization/server/constant/permissions.ts
  • ee/packages/federation-matrix/src/api/_matrix/invite.ts
  • apps/meteor/server/services/room/service.ts
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
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-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:

  • apps/meteor/app/lib/server/functions/createRoom.ts
  • apps/meteor/ee/server/hooks/federation/index.ts
📚 Learning: 2025-09-19T15:15:04.642Z
Learnt from: rodrigok
Repo: RocketChat/Rocket.Chat PR: 36991
File: apps/meteor/server/services/federation/infrastructure/rocket-chat/adapters/Settings.ts:219-221
Timestamp: 2025-09-19T15:15:04.642Z
Learning: The Federation_Matrix_homeserver_domain setting in apps/meteor/server/services/federation/infrastructure/rocket-chat/adapters/Settings.ts is part of the old federation system and is being deprecated/removed, so configuration issues with this setting should not be flagged for improvement.

Applied to files:

  • apps/meteor/app/lib/server/functions/createRoom.ts
  • apps/meteor/ee/server/hooks/federation/index.ts
📚 Learning: 2025-09-25T09:59:26.461Z
Learnt from: Dnouv
Repo: RocketChat/Rocket.Chat PR: 37057
File: packages/apps-engine/src/definition/accessors/IUserRead.ts:23-27
Timestamp: 2025-09-25T09:59:26.461Z
Learning: UserBridge.doGetUserRoomIds in packages/apps-engine/src/server/bridges/UserBridge.ts has a bug where it implicitly returns undefined when the app lacks read permission (missing return statement in the else case of the permission check).

Applied to files:

  • apps/meteor/app/lib/server/functions/createRoom.ts
📚 Learning: 2025-09-25T09:59:26.461Z
Learnt from: Dnouv
Repo: RocketChat/Rocket.Chat PR: 37057
File: packages/apps-engine/src/definition/accessors/IUserRead.ts:23-27
Timestamp: 2025-09-25T09:59:26.461Z
Learning: AppUserBridge.getUserRoomIds in apps/meteor/app/apps/server/bridges/users.ts always returns an array of strings (mapping subscription documents to room IDs), never undefined, even when user has no room subscriptions.

Applied to files:

  • apps/meteor/app/lib/server/functions/createRoom.ts
📚 Learning: 2025-09-25T09:59:26.461Z
Learnt from: Dnouv
Repo: RocketChat/Rocket.Chat PR: 37057
File: packages/apps-engine/src/definition/accessors/IUserRead.ts:23-27
Timestamp: 2025-09-25T09:59:26.461Z
Learning: AppUserBridge.getUserRoomIds in apps/meteor/app/apps/server/bridges/users.ts always returns an array of strings by mapping subscription documents to room IDs, never undefined, even when user has no room subscriptions.

Applied to files:

  • apps/meteor/app/lib/server/functions/createRoom.ts
🧬 Code graph analysis (2)
apps/meteor/app/lib/server/functions/createRoom.ts (1)
packages/core-typings/src/IRoom.ts (1)
  • isRoomNativeFederated (124-125)
apps/meteor/ee/server/hooks/federation/index.ts (1)
packages/core-services/src/index.ts (1)
  • MeteorError (56-56)
⏰ 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). (6)
  • GitHub Check: 🔨 Test Storybook / Test Storybook
  • GitHub Check: 🔨 Test Unit / Unit Tests
  • GitHub Check: 🔎 Code Check / TypeScript
  • GitHub Check: 📦 Meteor Build - coverage
  • GitHub Check: CodeQL-Build
  • GitHub Check: CodeQL-Build
🔇 Additional comments (5)
packages/i18n/src/locales/en.i18n.json (1)

5960-5961: Permission strings and references verified — all consistent.

The new i18n keys are used correctly across the codebase with exact matching in all 5 permission checks. The permission is properly registered with appropriate roles and error handling is in place. No issues found.

apps/meteor/ee/server/hooks/federation/index.ts (1)

1-1: LGTM!

The import additions correctly bring in Authorization and MeteorError needed for the permission check.

apps/meteor/app/lib/server/functions/createRoom.ts (3)

6-6: LGTM!

The import of isRoomNativeFederated is necessary for the federation permission check.


17-17: LGTM!

The import of hasPermissionAsync is necessary for checking the federation permission.


167-171: LGTM!

The permission check correctly gates federation room creation, ensuring only users with the access-federation permission can create federated rooms. The error message is clear and includes the method context.

@ricardogarim ricardogarim force-pushed the feat/federation-permissions branch from 408bdfe to 6daa046 Compare November 4, 2025 16:59
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Nitpick comments (1)
ee/packages/federation-matrix/src/api/_matrix/invite.ts (1)

152-155: Adjust log level for expected NotAllowed errors.

These denials are the new, expected outcome when a user lacks access-federation, so logging them as errors will create unnecessary noise. Dropping this to info (or warn) keeps the signal clean while still capturing the context.

Apply this diff:

-			logger.error('Authorization error, not retrying:', e);
+			logger.info('Authorization check failed, not retrying:', e);
📜 Review details

Configuration used: CodeRabbit 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.

📥 Commits

Reviewing files that changed from the base of the PR and between 408bdfe and 6daa046.

📒 Files selected for processing (2)
  • apps/meteor/app/lib/server/functions/createRoom.ts (3 hunks)
  • ee/packages/federation-matrix/src/api/_matrix/invite.ts (4 hunks)
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
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.097Z
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.
📚 Learning: 2025-11-04T16:49:19.097Z
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.097Z
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:

  • apps/meteor/app/lib/server/functions/createRoom.ts
  • ee/packages/federation-matrix/src/api/_matrix/invite.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:

  • apps/meteor/app/lib/server/functions/createRoom.ts
  • ee/packages/federation-matrix/src/api/_matrix/invite.ts
📚 Learning: 2025-09-19T15:15:04.642Z
Learnt from: rodrigok
Repo: RocketChat/Rocket.Chat PR: 36991
File: apps/meteor/server/services/federation/infrastructure/rocket-chat/adapters/Settings.ts:219-221
Timestamp: 2025-09-19T15:15:04.642Z
Learning: The Federation_Matrix_homeserver_domain setting in apps/meteor/server/services/federation/infrastructure/rocket-chat/adapters/Settings.ts is part of the old federation system and is being deprecated/removed, so configuration issues with this setting should not be flagged for improvement.

Applied to files:

  • apps/meteor/app/lib/server/functions/createRoom.ts
  • ee/packages/federation-matrix/src/api/_matrix/invite.ts
📚 Learning: 2025-09-25T09:59:26.461Z
Learnt from: Dnouv
Repo: RocketChat/Rocket.Chat PR: 37057
File: packages/apps-engine/src/definition/accessors/IUserRead.ts:23-27
Timestamp: 2025-09-25T09:59:26.461Z
Learning: UserBridge.doGetUserRoomIds in packages/apps-engine/src/server/bridges/UserBridge.ts has a bug where it implicitly returns undefined when the app lacks read permission (missing return statement in the else case of the permission check).

Applied to files:

  • apps/meteor/app/lib/server/functions/createRoom.ts
📚 Learning: 2025-09-25T09:59:26.461Z
Learnt from: Dnouv
Repo: RocketChat/Rocket.Chat PR: 37057
File: packages/apps-engine/src/definition/accessors/IUserRead.ts:23-27
Timestamp: 2025-09-25T09:59:26.461Z
Learning: AppUserBridge.getUserRoomIds in apps/meteor/app/apps/server/bridges/users.ts always returns an array of strings (mapping subscription documents to room IDs), never undefined, even when user has no room subscriptions.

Applied to files:

  • apps/meteor/app/lib/server/functions/createRoom.ts
  • ee/packages/federation-matrix/src/api/_matrix/invite.ts
📚 Learning: 2025-09-25T09:59:26.461Z
Learnt from: Dnouv
Repo: RocketChat/Rocket.Chat PR: 37057
File: packages/apps-engine/src/definition/accessors/IUserRead.ts:23-27
Timestamp: 2025-09-25T09:59:26.461Z
Learning: AppUserBridge.getUserRoomIds in apps/meteor/app/apps/server/bridges/users.ts always returns an array of strings by mapping subscription documents to room IDs, never undefined, even when user has no room subscriptions.

Applied to files:

  • apps/meteor/app/lib/server/functions/createRoom.ts
  • ee/packages/federation-matrix/src/api/_matrix/invite.ts
🧬 Code graph analysis (1)
apps/meteor/app/lib/server/functions/createRoom.ts (1)
packages/core-typings/src/IRoom.ts (1)
  • isRoomNativeFederated (124-125)
⏰ 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/functions/createRoom.ts (1)

168-172: Federation permission guard looks correct.

Great to see the create-room path enforcing access-federation up front and surfacing the Meteor error to the caller. This aligns with the documented pattern for user-initiated federation flows. Based on learnings

Copy link
Member

@ggazzo ggazzo left a comment

Choose a reason for hiding this comment

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

missing the logic behind the auto accept

@ricardogarim ricardogarim marked this pull request as draft November 5, 2025 17:07
@ricardogarim ricardogarim force-pushed the feat/federation-permissions branch from 6daa046 to d2e735c Compare November 6, 2025 11:31
@ricardogarim ricardogarim marked this pull request as ready for review November 6, 2025 11:33
@ricardogarim ricardogarim requested a review from ggazzo November 6, 2025 11:36
@ggazzo ggazzo modified the milestones: 7.10.3, 7.13.0 Nov 7, 2025
@ggazzo ggazzo added the stat: QA assured Means it has been tested and approved by a company insider label Nov 7, 2025
@dionisio-bot dionisio-bot bot added the stat: ready to merge PR tested and approved waiting for merge label Nov 7, 2025
@ggazzo ggazzo merged commit 22c6e0b into develop Nov 7, 2025
118 of 120 checks passed
@ggazzo ggazzo deleted the feat/federation-permissions branch November 7, 2025 16:26
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