fix: include banned and invited servers in event destination set#390
fix: include banned and invited servers in event destination set#390
Conversation
getServerSetInRoom only considered servers with 'join' membership, excluding servers whose users were banned or had pending invites. This caused unban (leave) events to never reach the banned user's server, breaking the state chain and preventing re-invites. Include 'ban' and 'invite' memberships so these servers continue receiving room events needed to maintain a consistent state.
The test previously asserted that banned and invited servers were excluded from the destination set. Updated to match the new behavior where these servers are included to ensure they receive room events.
WalkthroughThe Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Suggested labels
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #390 +/- ##
==========================================
+ Coverage 50.60% 50.86% +0.25%
==========================================
Files 99 101 +2
Lines 11259 11470 +211
==========================================
+ Hits 5698 5834 +136
- Misses 5561 5636 +75 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ee2bf8a5-a9f6-4e61-8232-784ce7da4125
📒 Files selected for processing (2)
packages/federation-sdk/src/services/state.service.spec.tspackages/federation-sdk/src/services/state.service.ts
📜 Review details
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-10T22:18:31.655Z
Learnt from: sampaiodiego
Repo: RocketChat/homeserver PR: 224
File: packages/federation-sdk/src/services/event-authorization.service.ts:261-268
Timestamp: 2025-10-10T22:18:31.655Z
Learning: In packages/federation-sdk/src/services/state.service.ts, the method `StateService.getLatestRoomState(roomId: string)` has return type `Promise<State>` and never returns undefined. If the state is not found, it throws an error with message "No state found for room ${roomId}" instead of returning undefined.
Applied to files:
packages/federation-sdk/src/services/state.service.tspackages/federation-sdk/src/services/state.service.spec.ts
🔇 Additional comments (1)
packages/federation-sdk/src/services/state.service.spec.ts (1)
2265-2289: Coverage update matches the new membership-destination behavior.This test now correctly validates the intended include/exclude matrix (
join/invite/banincluded,leaveexcluded).
| const residentMemberships = new Set(['join', 'invite', 'ban']); | ||
|
|
||
| for (const event of state.values()) { | ||
| if (!event.isMembershipEvent() || event.getMembership() !== 'join') { | ||
| if (!event.isMembershipEvent() || !residentMemberships.has(event.getMembership() ?? '')) { | ||
| continue; |
There was a problem hiding this comment.
Scope ban inclusion to federation destination usage; current change can authorize banned servers.
getServerSetInRoom is also used by EventAuthorizationService.serverHasAccessToResource (packages/federation-sdk/src/services/event-authorization.service.ts:254-256), where membership in this set immediately grants access. Including ban here can let banned servers pass authorization checks.
🔧 Proposed fix (make memberships explicit per call site)
- async getServerSetInRoom(roomId: RoomID, roomState?: State) {
+ async getServerSetInRoom(
+ roomId: RoomID,
+ roomState?: State,
+ residentMemberships: ReadonlySet<string> = new Set(['join']),
+ ) {
const state = roomState ?? (await this.getLatestRoomState(roomId));
const servers = new Set<string>();
- const residentMemberships = new Set(['join', 'invite', 'ban']);
-
for (const event of state.values()) {
if (!event.isMembershipEvent() || !residentMemberships.has(event.getMembership() ?? '')) {
continue;
}Then only federation fanout paths that require state-chain continuity should pass:
new Set(['join', 'invite', 'ban']).
2 participants
Summary
getServerSetInRoomnow includes servers withbanorinvitemembership in addition tojoinContext
Closes #388
getServerSetInRoomonly consideredjoinmembership, so after banning a user, their server stopped receiving events — including the unban event. This broke the state chain and made re-invites fail with "no previous state for event".Test plan
getServerSetInRoomtests pass🤖 Generated with Claude Code
Summary by CodeRabbit