chore(client): prevent federation actions for old federation #36957
Conversation
|
Looks like this PR is ready to merge! 🎉 |
|
WalkthroughAdds native-federation typings and guards across core typings, client UI/actions, cached stores, composer, contextual bar, and server hooks; blocks non-native federation runtime paths and surfaces an invalid-version composer notice; refactors server checks to rely on native-federation and removes a unit test. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant UI as Client UI
participant RC as RoomContext
participant TG as Typings Guards
User->>UI: Open message actions
UI->>RC: useRoom()
RC-->>UI: room
UI->>TG: isRoomFederated(room)?
TG-->>UI: true/false
alt Not federated
UI-->>User: Render actions (normal)
else Federated
UI->>TG: isRoomNativeFederated(room)?
TG-->>UI: true/false
alt Native federated
UI-->>User: Render actions (Quote/React/Reply)
else Non-native federated
UI-->>User: Hide/disable actions / show invalid-version callout in composer
end
end
sequenceDiagram
autonumber
participant Hook as Server Hook
participant R as Room
participant M as Message
participant TG as Typing Guards
participant FE as isFederationEnabled
Hook->>TG: isRoomFederated(R)?
TG-->>Hook: true/false
alt Not federated
Hook-->>Hook: shouldPerformAction = true
else Federated
Hook->>TG: isRoomNativeFederated(R)?
TG-->>Hook: true/false
alt Native room
Hook->>TG: isMessageFromNativeFederation(M)?
TG-->>Hook: true/false
alt Native message
Hook->>FE: isFederationEnabled()
FE-->>Hook: true/false
Hook-->>Hook: shouldPerformAction = FE
else Non-native message
Hook-->>Hook: shouldPerformAction = false
end
else Non-native room
Hook-->>Hook: shouldPerformAction = false
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests
Comment |
ggazzo
force-pushed
the
chore/feat/federation
branch
3 times, most recently
from
988feca to
2640138
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #36957 +/- ##
===========================================
+ Coverage 66.21% 66.37% +0.16%
===========================================
Files 3384 3391 +7
Lines 115025 115308 +283
Branches 21066 21140 +74
===========================================
+ Hits 76165 76541 +376
+ Misses 36255 36155 -100
- Partials 2605 2612 +7
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
ggazzo
force-pushed
the
chore/feat/federation
branch
from
3557851 to
56161ef
Compare
ggazzo
changed the title
ggazzo
force-pushed
the
chore/feat/federation
branch
from
56161ef to
b448402
Compare
ggazzo
force-pushed
the
chore/feat/federation
branch
2 times, most recently
from
29a751a to
142d9e9
Compare
… blocking non-native federated rooms
…or enhanced federation support
…ive and non-native federated rooms
…nsure it is a string
ggazzo
force-pushed
the
chore/feat/federation
branch
from
142d9e9 to
772a086
Compare
ggazzo
force-pushed
the
chore/feat/federation
branch
2 times, most recently
from
562284c to
f0184b6
Compare
…k and improved translation for invalid version message
apps/meteor/server/services/messages/hooks/BeforeFederationActions.ts
Outdated
Show resolved
Hide resolved
apps/meteor/server/services/room/hooks/BeforeFederationActions.ts
Outdated
Show resolved
Hide resolved
Co-authored-by: Diego Sampaio <chinello@gmail.com>
sampaiodiego
added
the
stat: QA assured
dionisio-bot
bot
added
the
stat: ready to merge
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (16)
packages/i18n/src/locales/en.i18n.json (1)
2131-2131: Fix grammar and improve link text for accessibility.
- "indeterminately" → "indefinitely"
- Prefer descriptive link text over "Click here".
Apply:
- "Federation_Matrix_Federated_Description_invalid_version": "This room was created by an old Federation version and it's blocked indeterminately. <1>Click here</1> for more information about Matrix Federation support", + "Federation_Matrix_Federated_Description_invalid_version": "This room was created with an older Federation version and is blocked indefinitely. <1>Learn more</1> about Matrix Federation support",packages/core-typings/src/IUser.ts (1)
263-272: Tighten native-federation type guard to avoid false positives.
Currently it only checks for a string; consider validating the dotted triplet to better align with the${number}.${number}.${number}intent.Apply:
-export const isUserNativeFederated = (user: Partial<IUser>): user is IUserNativeFederated => - isUserFederated(user) && 'federation' in user && typeof user.federation?.version === 'string'; +export const isUserNativeFederated = (user: Partial<IUser>): user is IUserNativeFederated => + isUserFederated(user) && + !!user.federation && + typeof user.federation.version === 'string' && + /^\d+\.\d+\.\d+$/.test(user.federation.version);packages/core-typings/src/IMessage/IMessage.ts (1)
279-281: Strengthen native-federation guard to check type, not just property presence.
'version' in message.federationcan pass whenversionexists but is undefined. Align with the user guard by checking string type.Apply:
-export const isMessageFromNativeFederation = (message: IMessage): message is INativeFederatedMessage => - isMessageFromMatrixFederation(message) && 'version' in message.federation; +export const isMessageFromNativeFederation = (message: IMessage): message is INativeFederatedMessage => + isMessageFromMatrixFederation(message) && + typeof ((message as IFederatedMessage & { federation: { version?: unknown } }).federation.version) === 'string';apps/meteor/client/cachedStores/RoomsCachedStore.ts (1)
57-59: Prefer ternary over&&in object spread for TS soundness.
Some TS configs flag spreading a boolean. The ternary avoids that and is clearer.Apply:
- ...(isRoomNativeFederated(room) && { - federation: room.federation, - }), + ...(isRoomNativeFederated(room) ? { federation: room.federation } : {}),apps/meteor/client/views/room/hooks/useUserInfoActions/actions/useAddUserAction.tsx (1)
49-54: Minor clarity tweak: derive block flag from roomIsFederated.
Keeps intent explicit and avoids evaluating native check for non‑federated rooms.Apply:
- const isFederationBlocked = room && !isRoomNativeFederated(room); + const isFederationBlocked = roomIsFederated && !isRoomNativeFederated(room);apps/meteor/client/views/room/hooks/useUserInfoActions/actions/useRemoveUserAction.tsx (1)
43-48: Old-federation block: good; simplify boolean and tighten intentLogic correctly blocks non‑native federation. Minor nit: avoid
room &&and couple the flag toroomIsFederatedto prevent accidental misuse elsewhere.-const isFederationBlocked = room && !isRoomNativeFederated(room); -const userCanRemove = roomIsFederated - ? !isFederationBlocked && Federation.isEditableByTheUser(currentUser || undefined, room, subscription) +const isFederationBlocked = roomIsFederated && !isRoomNativeFederated(room); +const userCanRemove = roomIsFederated + ? !isFederationBlocked && Federation.isEditableByTheUser(currentUser || undefined, room, subscription) : hasPermissionToRemove;Confirm server-side also rejects remove/kick on non‑native federated rooms; this UI gate shouldn’t be a security boundary.
apps/meteor/client/components/message/toolbar/items/actions/ReplyInThreadMessageAction.tsx (1)
28-33: Guard is correct; drop redundant truthiness checkSince
roomis a required prop,room &&is unnecessary and can confuse types.-const isFederated = room && isRoomFederated(room); +const isFederated = isRoomFederated(room); const isFederationBlocked = isFederated && !isRoomNativeFederated(room); if (isFederationBlocked) { return null; }apps/meteor/client/views/room/hooks/useUserInfoActions/actions/useChangeModeratorAction.tsx (1)
148-161: Option gating is right; align flag computation with federated stateTie
isFederationBlockedtoroomIsFederatedand avoidroom &&for clarity and stricter boolean typing.-const isFederationBlocked = room && !isRoomNativeFederated(room); +const isFederationBlocked = roomIsFederated && !isRoomNativeFederated(room);apps/meteor/client/views/room/composer/messageBox/MessageBox.tsx (1)
465-469: Avoid showing “Join” when send is blocked due to old federationCurrently
!canSendrenders a Join button. For non‑native federated rooms, this is misleading. Hide Join when the block is federation‑version related.-{!canSend && ( +{!canSend && (!isRoomFederated(room) || isRoomNativeFederated(room)) && ( <MessageComposerButton primary onClick={onJoin} loading={joinMutation.isPending}> {t('Join')} </MessageComposerButton> )}Confirm MessageBox never renders for non‑native federated rooms in containers (e.g., ComposerContainer shows the invalid‑version notice). If guaranteed, this change is optional.
apps/meteor/client/views/room/hooks/useUserInfoActions/actions/useChangeOwnerAction.tsx (1)
135-147: Owner action gating: good; minor boolean cleanupSame nit: compute the block flag based on federated state to keep semantics obvious.
-const isFederationBlocked = room && !isRoomNativeFederated(room); +const isFederationBlocked = roomIsFederated && !isRoomNativeFederated(room);apps/meteor/client/components/message/toolbar/items/actions/ReactionMessageAction.tsx (1)
35-37: Simplify federation flags and tighten typing.
roomis a required prop; avoidroom &&to keep booleans strictly typed and remove redundancy.-const isFederated = room && isRoomFederated(room); -const isFederationBlocked = isFederated && !isRoomNativeFederated(room); +const isFederated = isRoomFederated(room); +const isFederationBlocked = isFederated && !isRoomNativeFederated(room);apps/meteor/client/views/room/composer/ComposerContainer.tsx (1)
39-44: Minor: compute the blocked flag fromisFederationto avoid redundant checks.Keeps intent explicit and avoids calling
isRoomNativeFederatedfor non‑federated rooms.-const isFederation = isRoomFederated(room); -const isFederationBlocked = !isRoomNativeFederated(room); +const isFederation = isRoomFederated(room); +const isFederationBlocked = isFederation && !isRoomNativeFederated(room);Also applies to: 59-63
apps/meteor/client/cachedStores/SubscriptionsCachedStore.ts (1)
69-73: Stabilize object shape: always includefederationkey.Spreading a conditional object can yield
falseand omit the key entirely, causing avoidable re-renders and TS narrowing friction. Prefer a single property withundefinedwhen absent.- ...(room && - isRoomNativeFederated(room) && { - federation: room.federation, - }), + federation: room && isRoomNativeFederated(room) ? room.federation : undefined,apps/meteor/client/views/room/contextualBar/RoomMembers/AddUsers/AddUsers.tsx (1)
74-82: Show a clear disabled state/message when legacy federation is blocked.Right now the input and button simply disappear; provide feedback to the user to avoid confusion.
-{roomIsFederated ? ( - !isFederationBlocked && ( - <Controller - name='users' - control={control} - render={({ field }) => <UserAutoCompleteMultipleFederated {...field} placeholder={t('Choose_users')} />} - /> - ) -) : ( +{roomIsFederated ? ( + !isFederationBlocked ? ( + <Controller + name='users' + control={control} + render={({ field }) => <UserAutoCompleteMultipleFederated {...field} placeholder={t('Choose_users')} />} + /> + ) : ( + <FieldLabel color='status-font-annotation'> + {t('Federation_Matrix_Federated_Description_invalid_version')} + </FieldLabel> + ) +) : ( <Controller name='users' control={control} render={({ field }) => <UserAutoCompleteMultiple {...field} placeholder={t('Choose_users')} />} /> )}-{roomIsFederated ? ( - !isFederationBlocked && ( - <Button - primary - disabled={addClickHandler.isPending} - onClick={() => - addClickHandler.mutate({ - users: getValues('users'), - handleSave, - }) - } - > - {t('Add_users')} - </Button> - ) -) : ( +{roomIsFederated ? ( + !isFederationBlocked ? ( + <Button + primary + disabled={addClickHandler.isPending} + onClick={() => + addClickHandler.mutate({ + users: getValues('users'), + handleSave, + }) + } + > + {t('Add_users')} + </Button> + ) : ( + <Button primary disabled> + {t('Add_users')} + </Button> + ) +) : ( <Button primary loading={isSubmitting} disabled={!isDirty} onClick={handleSubmit(handleSave)}> {t('Add_users')} </Button> )}Also applies to: 94-108
apps/meteor/client/components/message/toolbar/items/actions/QuoteMessageAction.tsx (1)
27-29: Deduplicate federation-block logic via a tiny hook.This pattern repeats across actions; consider a
useIsFederationBlocked(room)helper to reduce drift.// packages/ui-client/src/hooks/useIsFederationBlocked.ts export const useIsFederationBlocked = (room: IRoom) => isRoomFederated(room) && !isRoomNativeFederated(room);packages/core-typings/src/IRoom.ts (1)
109-114: Version type may be too strict for SemVer (blocks pre-release/build metadata)
version: \${number}.${number}.${number}`excludes valid semver like1.2.3-rc.1` or build metadata. If native federation can surface pre-release/build versions, widen the type.Apply this diff in this block:
export interface IRoomNativeFederated extends IRoom { federated: true; federation: { - version: `${number}.${number}.${number}`; + version: SemVerString; }; }Add this helper type (outside the selected lines, near other shared types):
type SemVerString = `${number}.${number}.${number}${'' | `-${string}` | `+${string}`}`;Do pre-release/build-tagged versions occur in production telemetry? If yes, the above change is required; otherwise, please confirm they’ll never appear.
📜 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.
📒 Files selected for processing (22)
-
apps/meteor/client/cachedStores/RoomsCachedStore.ts(2 hunks) -
apps/meteor/client/cachedStores/SubscriptionsCachedStore.ts(2 hunks) -
apps/meteor/client/components/message/toolbar/items/actions/QuoteMessageAction.tsx(2 hunks) -
apps/meteor/client/components/message/toolbar/items/actions/ReactionMessageAction.tsx(3 hunks) -
apps/meteor/client/components/message/toolbar/items/actions/ReplyInThreadMessageAction.tsx(2 hunks) -
apps/meteor/client/hooks/roomActions/useMembersListRoomAction.ts(3 hunks) -
apps/meteor/client/views/room/composer/ComposerContainer.tsx(4 hunks) -
apps/meteor/client/views/room/composer/ComposerFederation/ComposerFederationInvalidVersion.tsx(1 hunks) -
apps/meteor/client/views/room/composer/messageBox/MessageBox.tsx(2 hunks) -
apps/meteor/client/views/room/contextualBar/RoomMembers/AddUsers/AddUsers.tsx(4 hunks) -
apps/meteor/client/views/room/contextualBar/RoomMembers/RoomMembersWithData.tsx(3 hunks) -
apps/meteor/client/views/room/hooks/useUserInfoActions/actions/useAddUserAction.tsx(2 hunks) -
apps/meteor/client/views/room/hooks/useUserInfoActions/actions/useChangeModeratorAction.tsx(2 hunks) -
apps/meteor/client/views/room/hooks/useUserInfoActions/actions/useChangeOwnerAction.tsx(2 hunks) -
apps/meteor/client/views/room/hooks/useUserInfoActions/actions/useRemoveUserAction.tsx(2 hunks) -
apps/meteor/server/services/messages/hooks/BeforeFederationActions.ts(1 hunks) -
apps/meteor/server/services/room/hooks/BeforeFederationActions.ts(1 hunks) -
apps/meteor/tests/unit/server/services/messages/hooks/BeforeFederationActions.tests.ts(0 hunks) -
packages/core-typings/src/IMessage/IMessage.ts(1 hunks) -
packages/core-typings/src/IRoom.ts(1 hunks) -
packages/core-typings/src/IUser.ts(2 hunks) -
packages/i18n/src/locales/en.i18n.json(1 hunks)
💤 Files with no reviewable changes (1)
- apps/meteor/tests/unit/server/services/messages/hooks/BeforeFederationActions.tests.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-19T15:15:04.613Z
Learnt from: rodrigok
PR: RocketChat/Rocket.Chat#36991
File: apps/meteor/server/services/federation/infrastructure/rocket-chat/adapters/Settings.ts:219-221
Timestamp: 2025-09-19T15:15:04.613Z
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:
packages/i18n/src/locales/en.i18n.jsonapps/meteor/client/views/room/contextualBar/RoomMembers/RoomMembersWithData.tsxapps/meteor/server/services/messages/hooks/BeforeFederationActions.tsapps/meteor/server/services/room/hooks/BeforeFederationActions.tspackages/core-typings/src/IUser.ts
🧬 Code graph analysis (16)
apps/meteor/client/views/room/hooks/useUserInfoActions/actions/useRemoveUserAction.tsx (2)
packages/core-typings/src/IRoom.ts (1)
isRoomNativeFederated(118-119)packages/core-services/src/index.ts (1)
Federation(181-181)
apps/meteor/client/views/room/hooks/useUserInfoActions/actions/useChangeOwnerAction.tsx (1)
packages/core-typings/src/IRoom.ts (1)
isRoomNativeFederated(118-119)
apps/meteor/client/views/room/contextualBar/RoomMembers/AddUsers/AddUsers.tsx (1)
packages/core-typings/src/IRoom.ts (2)
isRoomFederated(116-116)isRoomNativeFederated(118-119)
apps/meteor/client/cachedStores/RoomsCachedStore.ts (1)
packages/core-typings/src/IRoom.ts (1)
isRoomNativeFederated(118-119)
apps/meteor/client/views/room/hooks/useUserInfoActions/actions/useAddUserAction.tsx (2)
packages/core-typings/src/IRoom.ts (1)
isRoomNativeFederated(118-119)packages/core-services/src/index.ts (1)
Federation(181-181)
apps/meteor/client/components/message/toolbar/items/actions/QuoteMessageAction.tsx (1)
packages/core-typings/src/IRoom.ts (2)
isRoomFederated(116-116)isRoomNativeFederated(118-119)
apps/meteor/client/views/room/contextualBar/RoomMembers/RoomMembersWithData.tsx (2)
packages/core-typings/src/IRoom.ts (1)
isRoomNativeFederated(118-119)apps/meteor/client/lib/federation/Federation.ts (1)
canCreateInviteLinks(91-100)
apps/meteor/client/components/message/toolbar/items/actions/ReactionMessageAction.tsx (1)
packages/core-typings/src/IRoom.ts (2)
isRoomFederated(116-116)isRoomNativeFederated(118-119)
apps/meteor/client/views/room/composer/ComposerContainer.tsx (1)
packages/core-typings/src/IRoom.ts (1)
isRoomNativeFederated(118-119)
apps/meteor/server/services/messages/hooks/BeforeFederationActions.ts (3)
apps/meteor/server/services/room/hooks/BeforeFederationActions.ts (1)
FederationActions(6-18)packages/core-typings/src/IMessage/IMessage.ts (2)
IMessage(138-238)isMessageFromNativeFederation(279-280)packages/core-typings/src/IRoom.ts (3)
IRoom(21-95)isRoomFederated(116-116)isRoomNativeFederated(118-119)
apps/meteor/client/cachedStores/SubscriptionsCachedStore.ts (1)
packages/core-typings/src/IRoom.ts (1)
isRoomNativeFederated(118-119)
apps/meteor/client/views/room/composer/messageBox/MessageBox.tsx (1)
packages/core-typings/src/IRoom.ts (1)
isRoomNativeFederated(118-119)
apps/meteor/client/views/room/hooks/useUserInfoActions/actions/useChangeModeratorAction.tsx (1)
packages/core-typings/src/IRoom.ts (1)
isRoomNativeFederated(118-119)
apps/meteor/server/services/room/hooks/BeforeFederationActions.ts (2)
apps/meteor/server/services/messages/hooks/BeforeFederationActions.ts (1)
FederationActions(7-19)packages/core-typings/src/IRoom.ts (3)
IRoom(21-95)isRoomNativeFederated(118-119)isRoomFederated(116-116)
apps/meteor/client/components/message/toolbar/items/actions/ReplyInThreadMessageAction.tsx (1)
packages/core-typings/src/IRoom.ts (2)
isRoomFederated(116-116)isRoomNativeFederated(118-119)
apps/meteor/client/hooks/roomActions/useMembersListRoomAction.ts (1)
packages/core-typings/src/IRoom.ts (2)
isRoomFederated(116-116)isRoomNativeFederated(118-119)
🔇 Additional comments (15)
apps/meteor/client/views/room/composer/ComposerFederation/ComposerFederationInvalidVersion.tsx (2)
6-19: LGTM — concise, reusable notice component.
No functional concerns. The ExternalLink via Trans placeholder is appropriate.
10-15: i18n placeholder verified — no change needed.
packages/i18n/src/locales/en.i18n.json contains a single <1>...</1> placeholder for "Federation_Matrix_Federated_Description_invalid_version", which matches the Trans components.packages/core-typings/src/IUser.ts (1)
230-233: LGTM — version typed as template literal.
The${number}.${number}.${number}narrows well for native federation data.packages/core-typings/src/IMessage/IMessage.ts (1)
263-274: LGTM — explicit message typings for (native) federation.
Clear separation between federated and native‑federated messages.apps/meteor/client/cachedStores/RoomsCachedStore.ts (1)
2-2: LGTM — import of isRoomNativeFederated.
Matches usage below.apps/meteor/client/views/room/hooks/useUserInfoActions/actions/useAddUserAction.tsx (1)
2-2: LGTM — native federation awareness imported.apps/meteor/client/views/room/composer/messageBox/MessageBox.tsx (1)
296-305: Correctly blocks send for non‑native federationThe early return makes composer non‑sendable in old federation rooms. Looks good.
apps/meteor/client/components/message/toolbar/items/actions/ReactionMessageAction.tsx (1)
40-43: Good early guard to block reactions on legacy federation.Prevents UI from exposing reactions where they shouldn't be available.
apps/meteor/client/views/room/composer/ComposerContainer.tsx (1)
59-65: Composer correctly blocks legacy federation.Branching to a dedicated invalid-version component is the right UX.
apps/meteor/client/components/message/toolbar/items/actions/QuoteMessageAction.tsx (1)
30-32: Correctly hides Quote action on legacy federation.Consistent with other message actions.
apps/meteor/client/hooks/roomActions/useMembersListRoomAction.ts (1)
23-25: Hides Members list for legacy federation rooms.Matches the PR objective and prevents surfacing invite/add paths.
apps/meteor/server/services/room/hooks/BeforeFederationActions.ts (1)
7-17: Verify federation gating parity (room vs messages hooks)Room guard (apps/meteor/server/services/room/hooks/BeforeFederationActions.ts) checks isRoomNativeFederated || isRoomFederated, returns early if !isFederationEnabled(), then calls throwIfFederationNotEnabledOrNotReady(). Message hook (apps/meteor/server/services/messages/hooks/BeforeFederationActions.ts) checks only isRoomFederated(room) and returns true when false. This creates a divergence: a native‑federated room (isRoomNativeFederated=true, isRoomFederated=false) would be treated as federated by the room guard but as non‑federated by the message hook. Confirm intended semantics and parity. If parity is desired, either:
- update the message hook to consider isRoomNativeFederated, or
- change this guard to match the message hook (only check isRoomFederated and gate native on isFederationEnabled()).
Files to inspect: apps/meteor/server/services/room/hooks/BeforeFederationActions.ts, apps/meteor/server/services/messages/hooks/BeforeFederationActions.ts, callers in apps/meteor/server/services/room/service.ts.
packages/core-typings/src/IRoom.ts (1)
116-116: LGTM: clearer federated guardUsing
room.federated === trueafter the'in'check is explicit and safer than cast-based truthiness.apps/meteor/server/services/messages/hooks/BeforeFederationActions.ts (2)
9-15: Policy matches PR goal: block legacy federation pathsAllowing actions on non-federated rooms and blocking when either the room or message isn’t native-federated aligns with “prevent old federation method”.
Please confirm all client/server call sites interpret
falseas “block action” and do not proceed onfalsewithout additional checks.
1-1: LGTM: imports updated to native-federation helpersImports reflect the new guards consistently.
| const canCreateInviteLinks = | ||
| room && user && isFederated ? Federation.canCreateInviteLinks(user, room, subscription) : hasPermissionToCreateInviteLinks; | ||
| room && user && isFederated && !isFederationBlocked | ||
| ? Federation.canCreateInviteLinks(user, room, subscription) | ||
| : hasPermissionToCreateInviteLinks; | ||
|
|
There was a problem hiding this comment.
Blocked legacy federation still falls back to non‑federated permissions (allows Invite/Add).
When isFederationBlocked is true, we should not fall back to local permissions—deep links can still expose these actions.
-const canCreateInviteLinks =
- room && user && isFederated && !isFederationBlocked
- ? Federation.canCreateInviteLinks(user, room, subscription)
- : hasPermissionToCreateInviteLinks;
+const canCreateInviteLinks =
+ room && user && isFederated
+ ? (!isFederationBlocked && Federation.canCreateInviteLinks(user, room, subscription))
+ : hasPermissionToCreateInviteLinks;
-const canAddUsers =
- room && user && isFederated && !isFederationBlocked
- ? Federation.isEditableByTheUser(user, room, subscription)
- : hasPermissionToAddUsers;
+const canAddUsers =
+ room && user && isFederated
+ ? (!isFederationBlocked && Federation.isEditableByTheUser(user, room, subscription))
+ : hasPermissionToAddUsers;Also applies to: 62-66
🤖 Prompt for AI Agents
In
apps/meteor/client/views/room/contextualBar/RoomMembers/RoomMembersWithData.tsx
around lines 41-45 (and similarly 62-66), the current ternary falls back to
non-federated local permissions when isFederationBlocked is true, which
incorrectly allows invite/add actions; change the logic to explicitly return
false when isFederationBlocked is true, otherwise if federated evaluate
Federation.canCreateInviteLinks(user, room, subscription), and only if not
federated evaluate hasPermissionToCreateInviteLinks; apply the same explicit
isFederationBlocked check to the other block at lines 62-66.
| return false; | ||
| } | ||
|
|
||
| return isFederationEnabled(); |
There was a problem hiding this comment.
🧩 Analysis chain
Readiness gating removed — risk of actions when service is enabled but not ready
Returning only isFederationEnabled() can allow actions while the federation service isn’t ready. Elsewhere (room hooks) we still throw on “not enabled or not ready,” creating inconsistent behavior.
Options:
- Reintroduce readiness gating here (boolean form), e.g.,
return isFederationEnabled() && isFederationReady();, or - Call the same readiness guard used in room hooks, or
- Guarantee upstream callers invoke a readiness check before reaching this method.
If keeping this file boolean-only, prefer mirroring the room hook’s policy to avoid drift.
Run to locate call sites and confirm upstream readiness checks exist:
🏁 Script executed:
#!/bin/bash
# Find all usages of shouldPerformAction and nearby guards
rg -nP -C3 --type=ts 'shouldPerformAction\s*\(' apps/meteor | sed -n '1,200p'
# Check for readiness checks used around message hooks
rg -nP -C3 --type=ts 'throwIfFederationNotEnabledOrNotReady|isFederationReady' apps/meteor/server | sed -n '1,200p'Length of output: 17048
Reintroduce readiness gating in message federation hook
shouldPerformAction (apps/meteor/server/services/messages/hooks/BeforeFederationActions.ts) currently returns only isFederationEnabled(), allowing actions when federation is enabled but not ready — room hooks use throwIfFederationNotEnabledOrNotReady(), creating inconsistent behavior.
- Fix options:
- return isFederationEnabled() && isFederationReady();
- or call throwIfFederationNotEnabledOrNotReady() (mirror room hooks);
- or ensure all upstream callers check readiness before calling shouldPerformAction.
Locations: apps/meteor/server/services/messages/hooks/BeforeFederationActions.ts (shouldPerformAction); callers: apps/meteor/server/services/messages/service.ts (multiple call sites); reference: apps/meteor/server/services/room/hooks/BeforeFederationActions.ts and apps/meteor/server/services/federation/utils.ts.
🤖 Prompt for AI Agents
In apps/meteor/server/services/messages/hooks/BeforeFederationActions.ts at line
17, shouldPerformAction currently returns only isFederationEnabled(), which
allows actions when federation is enabled but not ready; change it to mirror the
room hooks by invoking throwIfFederationNotEnabledOrNotReady() (or, if you
prefer a boolean, return isFederationEnabled() && isFederationReady()) so
readiness is enforced consistently across message hooks and callers in
apps/meteor/server/services/messages/service.ts.
| export const isRoomNativeFederated = (room: Partial<IRoom>): room is IRoomNativeFederated => | ||
| isRoomFederated(room) && 'federation' in room && room.federation !== undefined; |
There was a problem hiding this comment.
Fix TS compile error: accessing a non-declared property on IRoom
room.federation is not part of IRoom, so direct dot-access on a Partial<IRoom> will fail type-checking. Use a narrowed/bracketed access and validate the expected shape instead.
Apply this diff:
-export const isRoomNativeFederated = (room: Partial<IRoom>): room is IRoomNativeFederated =>
- isRoomFederated(room) && 'federation' in room && room.federation !== undefined;
+export const isRoomNativeFederated = (room: Partial<IRoom>): room is IRoomNativeFederated => {
+ if (!isRoomFederated(room)) {
+ return false;
+ }
+ const federation = (room as { federation?: { version?: unknown } }).federation;
+ return typeof federation?.version === 'string';
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export const isRoomNativeFederated = (room: Partial<IRoom>): room is IRoomNativeFederated => | |
| isRoomFederated(room) && 'federation' in room && room.federation !== undefined; | |
| export const isRoomNativeFederated = (room: Partial<IRoom>): room is IRoomNativeFederated => { | |
| if (!isRoomFederated(room)) { | |
| return false; | |
| } | |
| const federation = (room as { federation?: { version?: unknown } }).federation; | |
| return typeof federation?.version === 'string'; | |
| } |
🤖 Prompt for AI Agents
In packages/core-typings/src/IRoom.ts around lines 118 to 119, the helper
accesses room.federation on a Partial<IRoom> even though federation is not
declared on IRoom which triggers a TS compile error; change the check to use a
safe, narrowed/bracketed access (e.g. check 'federation' in room and then access
(room as unknown as Record<string, unknown>)['federation'] or cast to any) so
TypeScript does not complain, and keep the existing isRoomFederated(room)
predicate as part of the conjunction.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
packages/core-typings/src/IRoom.ts (1)
118-119: Fix TS error: access to non-declared property room.federationIRoom doesn’t declare
federation; direct dot access onPartial<IRoom>fails type-checking. Use a narrowed/casted access and check the version type.-export const isRoomNativeFederated = (room: Partial<IRoom>): room is IRoomNativeFederated => - isRoomFederated(room) && 'federation' in room && room.federation !== undefined; +export const isRoomNativeFederated = (room: Partial<IRoom>): room is IRoomNativeFederated => { + if (!isRoomFederated(room)) { + return false; + } + const federation = (room as { federation?: { version?: unknown } }).federation; + return typeof federation?.version === 'string'; +};apps/meteor/server/services/messages/hooks/BeforeFederationActions.ts (1)
12-16: Reintroduce readiness gating to match room hooksReturning only
isFederationEnabled()allows actions when federation is enabled but not ready; room hook throws when not enabled or not ready. Make behavior consistent to prevent premature actions.-import { isFederationEnabled } from '../../federation/utils'; +import { throwIfFederationNotEnabledOrNotReady } from '../../federation/utils'; @@ - if (!isRoomNativeFederated(room) || !isMessageFromNativeFederation(message)) { + if (!isRoomNativeFederated(room) || !isMessageFromNativeFederation(message)) { return false; } - - return isFederationEnabled(); + try { + throwIfFederationNotEnabledOrNotReady(); + return true; + } catch { + return false; + }
🧹 Nitpick comments (3)
packages/core-typings/src/IUser.ts (1)
270-271: Align guard parameter with existing isUserFederated unionConsider accepting Serialized in the guard too for parity with isUserFederated.
-export const isUserNativeFederated = (user: Partial<IUser>): user is IUserNativeFederated => - isUserFederated(user) && 'federation' in user && typeof user.federation?.version === 'string'; +export const isUserNativeFederated = ( + user: Partial<IUser> | Partial<Serialized<IUser>>, +): user is IUserNativeFederated => + isUserFederated(user) && 'federation' in user && typeof (user as any).federation?.version === 'string';packages/core-typings/src/IMessage/IMessage.ts (1)
279-281: Strengthen predicate and avoid fragile'in'checkPrefer a concrete type check on
versionto reduce false positives and improve TS narrowing.-export const isMessageFromNativeFederation = (message: IMessage): message is INativeFederatedMessage => - isMessageFromMatrixFederation(message) && 'version' in message.federation; +export const isMessageFromNativeFederation = (message: IMessage): message is INativeFederatedMessage => { + if (!isMessageFromMatrixFederation(message)) { + return false; + } + const version = (message as { federation?: { version?: unknown } }).federation?.version; + return typeof version === 'string'; +};apps/meteor/server/services/room/hooks/BeforeFederationActions.ts (1)
7-9: Simplify the initial guardSince
isRoomNativeFederated(room)impliesisRoomFederated(room), the compound check can be reduced to!isRoomFederated(room).- if (!isRoomNativeFederated(room) && !isRoomFederated(room)) { + if (!isRoomFederated(room)) { return; }
📜 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.
📒 Files selected for processing (5)
apps/meteor/server/services/messages/hooks/BeforeFederationActions.ts(1 hunks)apps/meteor/server/services/room/hooks/BeforeFederationActions.ts(1 hunks)packages/core-typings/src/IMessage/IMessage.ts(1 hunks)packages/core-typings/src/IRoom.ts(1 hunks)packages/core-typings/src/IUser.ts(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-19T15:15:04.613Z
Learnt from: rodrigok
PR: RocketChat/Rocket.Chat#36991
File: apps/meteor/server/services/federation/infrastructure/rocket-chat/adapters/Settings.ts:219-221
Timestamp: 2025-09-19T15:15:04.613Z
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:
packages/core-typings/src/IUser.tsapps/meteor/server/services/room/hooks/BeforeFederationActions.tsapps/meteor/server/services/messages/hooks/BeforeFederationActions.ts
🧬 Code graph analysis (2)
apps/meteor/server/services/room/hooks/BeforeFederationActions.ts (2)
apps/meteor/server/services/messages/hooks/BeforeFederationActions.ts (1)
FederationActions(6-18)packages/core-typings/src/IRoom.ts (3)
IRoom(21-95)isRoomNativeFederated(118-119)isRoomFederated(116-116)
apps/meteor/server/services/messages/hooks/BeforeFederationActions.ts (3)
apps/meteor/server/services/room/hooks/BeforeFederationActions.ts (1)
FederationActions(5-17)packages/core-typings/src/IMessage/IMessage.ts (2)
IMessage(138-238)isMessageFromNativeFederation(279-280)packages/core-typings/src/IRoom.ts (3)
IRoom(21-95)isRoomFederated(116-116)isRoomNativeFederated(118-119)
⏰ 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 (6)
packages/core-typings/src/IUser.ts (2)
230-233: Typing for federation.version looks goodUsing the
${number}template literal keeps the intent clear and consistent across the codebase.
263-268: Solid discriminated native federated user typeThe shape aligns with room/message native federation and will narrow well at call sites.
packages/core-typings/src/IRoom.ts (1)
109-114: Native federated room type LGTMConsistent with
${number}versioning and discriminant field.packages/core-typings/src/IMessage/IMessage.ts (1)
269-274: Message native federation shape is consistentInterfaces align with room/user native federation. No issues.
apps/meteor/server/services/messages/hooks/BeforeFederationActions.ts (1)
8-10: Early return for non-federated rooms is correctKeeps non-federated paths unaffected.
apps/meteor/server/services/room/hooks/BeforeFederationActions.ts (1)
11-15: LGTM: readiness guard mirrors policy elsewhereShort-circuit on disabled, then throw on not ready keeps behavior predictable and consistent.
https://rocketchat.atlassian.net/browse/FDR-144
Proposed changes (including videos or screenshots)
Issue(s)
Steps to test or reproduce
Further comments
Summary by CodeRabbit
New Features
Changes
Bug Fixes