Skip to content

fix: utilize room state for kick validation#340

Merged
sampaiodiego merged 1 commit intomainfrom
fix/kick-permissions
Mar 10, 2026
Merged

fix: utilize room state for kick validation#340
sampaiodiego merged 1 commit intomainfrom
fix/kick-permissions

Conversation

@ggazzo
Copy link
Member

@ggazzo ggazzo commented Mar 10, 2026

https://rocketchat.atlassian.net/browse/FGA-32

Summary by CodeRabbit

Release Notes

  • Refactor
    • Improved room permission validation for kick and ban operations by refining how power level information is retrieved and validated from room state, ensuring more consistent permission checks.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Mar 10, 2026

Walkthrough

The pull request modifies the power levels event retrieval mechanism in the room service, replacing direct event fetching with state-based lookup using getStateByMapKey. It adds validation of the located power levels event and updates the permission check logic accordingly.

Changes

Cohort / File(s) Summary
State-based Power Levels Lookup
packages/federation-sdk/src/services/room.service.ts
Replaced direct event service fetching with state map lookup for power_levels events. Added isPowerLevelEvent() validation guard and getContent() access pattern. Simplified error handling by removing event data missing path. Added import of getStateByMapKey utility.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Suggested labels

type: bug

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The pull request title clearly and concisely summarizes the main change: utilizing room state for kick validation, which directly aligns with the core modification in the changeset.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ 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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@ggazzo ggazzo force-pushed the fix/kick-permissions branch from 7bacfd1 to 3c138d5 Compare March 10, 2026 18:49
@ggazzo ggazzo changed the title fix: utilize room state for power level updates fix: utilize room state for kick validation Mar 10, 2026
@ggazzo ggazzo marked this pull request as ready for review March 10, 2026 18:56
Copy link

@cubic-dev-ai cubic-dev-ai bot left a comment

Choose a reason for hiding this comment

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

No issues found across 1 file

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.

🧹 Nitpick comments (1)
packages/federation-sdk/src/services/room.service.ts (1)

562-569: Prefer the existing room-state power-level accessor here.

This reimplements lookup/guard logic that already exists in packages/room/src/manager/room-state.ts:123-133, and isPowerLevelEvent() in packages/room/src/manager/event-wrapper.ts:184-186 only proves the event is a state m.room.power_levels event. Pulling state.powerLevels from getLatestRoomState2(roomId) (or extracting a shared helper in RoomService) would keep kick/ban permission checks on one path and avoid maintaining slightly different validation rules in multiple places.

♻️ Suggested simplification
-		const state = await this.stateService.getLatestRoomState(roomId);
-		const powerLevelsEvent = getStateByMapKey(state, { type: 'm.room.power_levels' });
-		if (!powerLevelsEvent?.isPowerLevelEvent?.()) {
+		const state = await this.stateService.getLatestRoomState2(roomId);
+		const powerLevelsContent = state.powerLevels;
+		if (!powerLevelsContent) {
 			logger.warn(`No power_levels event found for room ${roomId}, cannot verify permission to kick.`);
 			throw new HttpException('Cannot verify permission to kick user.', HttpStatus.FORBIDDEN);
 		}
 
-		this.validateKickPermission(powerLevelsEvent.getContent(), senderId, kickedUserId);
+		this.validateKickPermission(powerLevelsContent, senderId, kickedUserId);
🤖 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.ts` around lines 562 - 569,
Replace the custom power-level lookup in this block with the canonical
room-state accessor: call the existing getLatestRoomState2(roomId) (or reuse the
shared RoomState.powerLevels accessor) instead of manually using
getStateByMapKey and isPowerLevelEvent; then guard on the returned
roomState.powerLevels (or the helper's presence) and pass that content into
validateKickPermission(senderId, kickedUserId) so permission checks use the same
power-level logic as other code paths (avoid duplicating the lookup/guard logic
currently implemented in room-state manager and event-wrapper).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@packages/federation-sdk/src/services/room.service.ts`:
- Around line 562-569: Replace the custom power-level lookup in this block with
the canonical room-state accessor: call the existing getLatestRoomState2(roomId)
(or reuse the shared RoomState.powerLevels accessor) instead of manually using
getStateByMapKey and isPowerLevelEvent; then guard on the returned
roomState.powerLevels (or the helper's presence) and pass that content into
validateKickPermission(senderId, kickedUserId) so permission checks use the same
power-level logic as other code paths (avoid duplicating the lookup/guard logic
currently implemented in room-state manager and event-wrapper).

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: e8515e24-71d2-4eec-b577-ef896ce290db

📥 Commits

Reviewing files that changed from the base of the PR and between a286ad8 and 3c138d5.

📒 Files selected for processing (1)
  • packages/federation-sdk/src/services/room.service.ts
📜 Review details
⏰ 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). (1)
  • GitHub Check: cubic · AI code reviewer
🧰 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

@codecov-commenter
Copy link

Codecov Report

❌ Patch coverage is 80.00000% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 50.34%. Comparing base (a286ad8) to head (3c138d5).

Files with missing lines Patch % Lines
...ckages/federation-sdk/src/services/room.service.ts 80.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #340      +/-   ##
==========================================
- Coverage   50.37%   50.34%   -0.03%     
==========================================
  Files          97       97              
  Lines       11058    11053       -5     
==========================================
- Hits         5570     5565       -5     
  Misses       5488     5488              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 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.

@sampaiodiego sampaiodiego merged commit 2ca978b into main Mar 10, 2026
4 checks passed
@sampaiodiego sampaiodiego deleted the fix/kick-permissions branch March 10, 2026 22:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants