fix: kick event not being propagated to federated servers#341
fix: kick event not being propagated to federated servers#341aleksandernsilva wants to merge 1 commit intomainfrom
Conversation
WalkthroughThe changes exclude outlier events from event queries, expand room service test coverage for power-level validations, and refactor the power-level update logic in the room service to use simplified in-place updates with centralized error handling instead of multi-step authorization checks. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant RoomService
participant FederationHandler
participant StateResolver
Client->>RoomService: kickUser/banUser/updateUserPowerLevel
RoomService->>RoomService: Read current room state
RoomService->>RoomService: Merge new power-level data
RoomService->>FederationHandler: handlePdu (with state event)
FederationHandler->>StateResolver: Resolve & authorize state
alt Authorization Fails
StateResolver-->>FederationHandler: StateResolverAuthorizationError
FederationHandler-->>RoomService: HTTP 403 (Forbidden)
RoomService-->>Client: Error response
else Authorization Succeeds
StateResolver-->>FederationHandler: State validated
FederationHandler->>FederationHandler: Emit federation event
FederationHandler->>FederationHandler: Emit membership event (once)
FederationHandler-->>RoomService: Success
RoomService-->>Client: Success response
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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)
packages/federation-sdk/src/repositories/event.repository.ts (1)
150-158:⚠️ Potential issue | 🔴 CriticalPromoted outlier events remain hidden from latest-event consumers.
insertOutlierEvent()setsoutlier: true, andinsertOrUpdateEventWithStateId()later promotes outliers viaupdateOnewithout clearing the flag. The new filter infindLatestEvents()at line 157 ('outlier': { $ne: true }) will then exclude promoted outliers from selection, breaking prev-event lookup (line 279), state-partial checks (line 489), and branch merging (line 915). Either clear theoutlierflag when promoting viainsertOrUpdateEventWithStateId(), or use a replacement strategy for outlier promotion.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/federation-sdk/src/repositories/event.repository.ts` around lines 150 - 158, findLatestEvents currently filters out documents with 'outlier': true which causes promoted outliers to remain hidden; to fix, update the promotion logic in insertOrUpdateEventWithStateId so that when an outlier is promoted you either unset the outlier field or set outlier: false (e.g. include $unset: { outlier: "" } or $set: { outlier: false } in the updateOne call), or alternatively change insertOutlierEvent/insertOrUpdateEventWithStateId to replace the document on promotion (replaceOne) so the outlier flag is not preserved; ensure the change is applied in the code paths inside insertOrUpdateEventWithStateId that perform the promotion so prev-event lookup, state-partial checks, and branch merging work with findLatestEvents as written.
🧹 Nitpick comments (1)
packages/federation-sdk/src/services/room.service.spec.ts (1)
246-266: These auth-failure tests are too broad.All three new cases only assert
.rejects.toThrow(), so any failure path will satisfy them. For the kick/ban cases, the target user also never joins the room, which makes it even easier for an unrelated membership-state rejection to pass the test. Please assert the specific status/message, and make the kick/ban target a joined or invited member so the suite proves the intended authorization branch.Also applies to: 290-310, 344-364
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/federation-sdk/src/services/room.service.spec.ts` around lines 246 - 266, Update the tests to assert specific authorization failures and ensure the target is actually in the room: for the tests using federationSDK.kickUser (and analogous ban tests) modify the setup so the target user is joined or invited (use roomService.joinUser or appropriate invite helper) and replace the generic .rejects.toThrow() with an assertion that checks the exact error type/message or status (e.g., match on the authorization error string or code returned by federationSDK.kickUser). Apply the same changes to the other failing cases referenced (the tests around lines 290-310 and 344-364) so each test verifies the intended "insufficient power" or "equal/higher power" authorization branch rather than any membership/state rejection.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/federation-sdk/src/services/room.service.spec.ts`:
- Line 225: Remove the focused test suites by replacing any describe.only usages
with plain describe; specifically update the "describe.only('kickUser', ...)"
and the other focused describe at the second occurrence to "describe('kickUser',
...)" (and similarly for the other suite) so the full test file runs and
ESLint/my-workflow no longer fails; search for describe.only in
room.service.spec.ts and remove ".only" from each occurrence.
In `@packages/federation-sdk/src/services/room.service.ts`:
- Around line 348-366: The m.room.power_levels event is being built with
PersistentEventFactory.defaultRoomVersion which can cause rejection on rooms
with other versions; instead obtain the actual room version for roomId (for
example by querying the room state or reusing the room/event version already
available in scope) and pass that version into this.stateService.buildEvent
(replace PersistentEventFactory.defaultRoomVersion with the roomVersion
variable). Update the code around stateService.buildEvent to retrieve
roomVersion (e.g., const roomVersion = await
this.stateService.getRoomVersion(roomId) or use the version from the current
room/event state if present) and use that when calling buildEvent for the
power_levels replacement.
---
Outside diff comments:
In `@packages/federation-sdk/src/repositories/event.repository.ts`:
- Around line 150-158: findLatestEvents currently filters out documents with
'outlier': true which causes promoted outliers to remain hidden; to fix, update
the promotion logic in insertOrUpdateEventWithStateId so that when an outlier is
promoted you either unset the outlier field or set outlier: false (e.g. include
$unset: { outlier: "" } or $set: { outlier: false } in the updateOne call), or
alternatively change insertOutlierEvent/insertOrUpdateEventWithStateId to
replace the document on promotion (replaceOne) so the outlier flag is not
preserved; ensure the change is applied in the code paths inside
insertOrUpdateEventWithStateId that perform the promotion so prev-event lookup,
state-partial checks, and branch merging work with findLatestEvents as written.
---
Nitpick comments:
In `@packages/federation-sdk/src/services/room.service.spec.ts`:
- Around line 246-266: Update the tests to assert specific authorization
failures and ensure the target is actually in the room: for the tests using
federationSDK.kickUser (and analogous ban tests) modify the setup so the target
user is joined or invited (use roomService.joinUser or appropriate invite
helper) and replace the generic .rejects.toThrow() with an assertion that checks
the exact error type/message or status (e.g., match on the authorization error
string or code returned by federationSDK.kickUser). Apply the same changes to
the other failing cases referenced (the tests around lines 290-310 and 344-364)
so each test verifies the intended "insufficient power" or "equal/higher power"
authorization branch rather than any membership/state rejection.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4afa81f8-0bdf-4bba-af60-496a69525c3c
📒 Files selected for processing (3)
packages/federation-sdk/src/repositories/event.repository.tspackages/federation-sdk/src/services/room.service.spec.tspackages/federation-sdk/src/services/room.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/room.service.ts
🪛 ESLint
packages/federation-sdk/src/services/room.service.spec.ts
[error] 225-225: 'describe.only' is restricted from being used.
(no-restricted-properties)
[error] 270-270: 'describe.only' is restricted from being used.
(no-restricted-properties)
🪛 GitHub Actions: my-workflow
packages/federation-sdk/src/services/room.service.spec.ts
[error] 225-225: ESLint: 'describe.only' is restricted from being used (no-restricted-properties).
[error] 270-270: ESLint: 'describe.only' is restricted from being used (no-restricted-properties).
🔇 Additional comments (1)
packages/federation-sdk/src/services/room.service.ts (1)
504-520: Nice: the kick path now federates only afterhandlePdu()succeeds.Gating the outbound send behind state resolution/auth keeps rejected kicks local and directly addresses the propagation gap this PR is targeting.
aleksandernsilva
force-pushed
the
fix/kick-synapse-propagation
branch
from
7ac7a9b to
a1c75ff
Compare
aleksandernsilva
changed the title
1 participant
Summary by CodeRabbit
Bug Fixes
Refactor
Tests