feat: New endpoint for showing ABAC managed rooms #37536
Conversation
|
|
Looks like this PR is not ready to merge, because of the following issues:
Please fix the issues and try again If you have any trouble, please check the PR guidelines |
WalkthroughAdds a paginated, filterable GET endpoint Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API as "GET /abac/rooms"
participant Route as "Route Handler"
participant Abac as "AbacService"
participant DB as "MongoDB"
Client->>API: GET /abac/rooms?filter=&filterType=&offset=&count=
API->>Route: auth & permission check
Route->>Route: validate query (GETAbacRoomsListQueryValidator)
Route->>Abac: listAbacRooms({offset,count,filter,filterType})
Abac->>DB: findPaginated(query with ABAC attrs + regex filter)
DB-->>Abac: cursor + totalCount
Abac->>Abac: toArray() -> rooms[]
Abac-->>Route: { rooms, offset, count, total }
Route-->>Client: 200 GETAbacRoomsResponse
rect rgba(200,150,255,0.14)
note right of API: permission: abac-management
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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)
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✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## feat/abac #37536 +/- ##
============================================
Coverage ? 54.49%
============================================
Files ? 2658
Lines ? 49968
Branches ? 11126
============================================
Hits ? 27232
Misses ? 20590
Partials ? 2146
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
![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
🧹 Nitpick comments (1)
ee/packages/abac/src/index.ts (1)
169-169: Consider using spread operator for clarity.While
Object.assign(baseQuery, condition)works correctly here, using the spread operator would make the code more readable and explicit about creating a merged query object.- Object.assign(baseQuery, condition); + const finalQuery = { ...baseQuery, ...condition };Then update line 172 to use
finalQueryinstead ofbaseQuery:- const { cursor, totalCount } = Rooms.findPaginated(baseQuery, { + const { cursor, totalCount } = Rooms.findPaginated(finalQuery, {
📜 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/ee/server/api/abac/index.ts(2 hunks)apps/meteor/ee/server/api/abac/schemas.ts(2 hunks)apps/meteor/tests/end-to-end/api/abac.ts(1 hunks)ee/packages/abac/src/index.ts(1 hunks)packages/core-services/src/types/IAbacService.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 37303
File: apps/meteor/tests/end-to-end/api/abac.ts:1125-1137
Timestamp: 2025-10-27T14:38:46.994Z
Learning: In Rocket.Chat ABAC feature, when ABAC is disabled globally (ABAC_Enabled setting is false), room-level ABAC attributes are not evaluated when changing room types. This means converting a private room to public will succeed even if the room has ABAC attributes, as long as the global ABAC setting is disabled.
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 37299
File: apps/meteor/ee/server/lib/ldap/Manager.ts:438-454
Timestamp: 2025-10-24T17:32:05.348Z
Learning: In Rocket.Chat, ABAC attributes can only be set on private rooms and teams (type 'p'), not on public rooms (type 'c'). Therefore, when checking for ABAC-protected rooms/teams during LDAP sync or similar operations, it's sufficient to query only private rooms using methods like `findPrivateRoomsByIdsWithAbacAttributes`.
📚 Learning: 2025-10-24T17:32:05.348Z
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 37299
File: apps/meteor/ee/server/lib/ldap/Manager.ts:438-454
Timestamp: 2025-10-24T17:32:05.348Z
Learning: In Rocket.Chat, ABAC attributes can only be set on private rooms and teams (type 'p'), not on public rooms (type 'c'). Therefore, when checking for ABAC-protected rooms/teams during LDAP sync or similar operations, it's sufficient to query only private rooms using methods like `findPrivateRoomsByIdsWithAbacAttributes`.
Applied to files:
packages/core-services/src/types/IAbacService.tsapps/meteor/ee/server/api/abac/schemas.tsapps/meteor/ee/server/api/abac/index.tsapps/meteor/tests/end-to-end/api/abac.tsee/packages/abac/src/index.ts
📚 Learning: 2025-10-27T14:38:46.994Z
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 37303
File: apps/meteor/tests/end-to-end/api/abac.ts:1125-1137
Timestamp: 2025-10-27T14:38:46.994Z
Learning: In Rocket.Chat ABAC feature, when ABAC is disabled globally (ABAC_Enabled setting is false), room-level ABAC attributes are not evaluated when changing room types. This means converting a private room to public will succeed even if the room has ABAC attributes, as long as the global ABAC setting is disabled.
Applied to files:
packages/core-services/src/types/IAbacService.tsapps/meteor/ee/server/api/abac/schemas.tsapps/meteor/ee/server/api/abac/index.tsapps/meteor/tests/end-to-end/api/abac.ts
🧬 Code graph analysis (5)
packages/core-services/src/types/IAbacService.ts (1)
packages/core-typings/src/IRoom.ts (1)
IRoom(22-98)
apps/meteor/ee/server/api/abac/schemas.ts (1)
packages/core-typings/src/IRoom.ts (1)
IRoom(22-98)
apps/meteor/ee/server/api/abac/index.ts (3)
apps/meteor/ee/server/api/abac/schemas.ts (2)
GETAbacRoomsResponseValidator(256-256)GETAbacRoomsListQueryValidator(232-232)packages/rest-typings/src/v1/Ajv.ts (1)
validateUnauthorizedErrorResponse(70-70)packages/core-services/src/index.ts (1)
Abac(202-202)
apps/meteor/tests/end-to-end/api/abac.ts (2)
apps/meteor/tests/data/api-data.ts (2)
request(10-10)credentials(39-42)apps/meteor/tests/e2e/utils/create-target-channel.ts (1)
deleteTeam(66-68)
ee/packages/abac/src/index.ts (1)
packages/core-typings/src/IRoom.ts (1)
IRoom(22-98)
🔇 Additional comments (4)
packages/core-services/src/types/IAbacService.ts (1)
19-24: LGTM! Well-structured interface addition.The new
listAbacRoomsmethod follows the same pagination and filtering pattern aslistAbacAttributes, maintaining consistency across the service interface.apps/meteor/tests/end-to-end/api/abac.ts (1)
1578-1917: LGTM! Comprehensive test coverage.The test suite thoroughly covers the new ABAC rooms listing endpoint including:
- Baseline listing and filtering (by room name, attribute key, value, and all)
- Pagination behavior
- Edge cases (rooms without attributes, public rooms, default rooms, team rooms)
- Authorization checks
- Dynamic behavior (listing updates after attribute removal)
The nested describe block for team main rooms provides additional focused coverage without duplication.
ee/packages/abac/src/index.ts (1)
125-186: LGTM! Solid implementation with good security practices.The
listAbacRoomsmethod correctly:
- Filters for private rooms only (
t: 'p') as per learnings- Uses
escapeRegExpto prevent regex injection- Implements flexible filtering by room name, attribute key, value, or all
- Returns paginated results with proper metadata
apps/meteor/ee/server/api/abac/index.ts (1)
286-313: LGTM! Endpoint follows established patterns.The new GET endpoint correctly:
- Requires authentication and
abac-managementpermission- Uses appropriate validators for query and response
- Extracts pagination and filtering parameters properly
- Delegates to the service layer
Note: Consistent with the GET
/abac/attributesendpoint, this doesn't checkABAC_Enabled, allowing administrators to list ABAC-managed rooms even when the feature is disabled (useful for cleanup/migration scenarios).
KevLehman
force-pushed
the
feat/abac-rooms-endp
branch
from
639ce68 to
61716e0
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/meteor/tests/end-to-end/api/abac.ts (1)
1577-1917: ABAC rooms listing tests are thorough and aligned with ABAC invariantsThis suite does a good job validating the new
/abac/roomsendpoint:
- Baseline visibility: only private rooms with
abacAttributes(including team main rooms) are listed; public rooms, default rooms, and non-ABAC private/team rooms are excluded.- Attribute lifecycle: rooms disappear after attributes are removed and reappear when re‑added.
- Filtering:
filterTypevariants (roomName,attribute,value,all) behave as expected.- Pagination and unauthorized access are exercised.
- Setup/teardown look correct; created rooms and teams are cleaned up.
Optional: consider adding a small test that omits
filterTypeentirely to lock in the “default = all” behavior end‑to‑end.Based on learnings
📜 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/ee/server/api/abac/index.ts(2 hunks)apps/meteor/ee/server/api/abac/schemas.ts(2 hunks)apps/meteor/tests/end-to-end/api/abac.ts(1 hunks)ee/packages/abac/src/index.ts(1 hunks)packages/core-services/src/types/IAbacService.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 37303
File: apps/meteor/tests/end-to-end/api/abac.ts:1125-1137
Timestamp: 2025-10-27T14:38:46.994Z
Learning: In Rocket.Chat ABAC feature, when ABAC is disabled globally (ABAC_Enabled setting is false), room-level ABAC attributes are not evaluated when changing room types. This means converting a private room to public will succeed even if the room has ABAC attributes, as long as the global ABAC setting is disabled.
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 37299
File: apps/meteor/ee/server/lib/ldap/Manager.ts:438-454
Timestamp: 2025-10-24T17:32:05.348Z
Learning: In Rocket.Chat, ABAC attributes can only be set on private rooms and teams (type 'p'), not on public rooms (type 'c'). Therefore, when checking for ABAC-protected rooms/teams during LDAP sync or similar operations, it's sufficient to query only private rooms using methods like `findPrivateRoomsByIdsWithAbacAttributes`.
📚 Learning: 2025-10-24T17:32:05.348Z
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 37299
File: apps/meteor/ee/server/lib/ldap/Manager.ts:438-454
Timestamp: 2025-10-24T17:32:05.348Z
Learning: In Rocket.Chat, ABAC attributes can only be set on private rooms and teams (type 'p'), not on public rooms (type 'c'). Therefore, when checking for ABAC-protected rooms/teams during LDAP sync or similar operations, it's sufficient to query only private rooms using methods like `findPrivateRoomsByIdsWithAbacAttributes`.
Applied to files:
packages/core-services/src/types/IAbacService.tsapps/meteor/ee/server/api/abac/schemas.tsapps/meteor/ee/server/api/abac/index.tsapps/meteor/tests/end-to-end/api/abac.tsee/packages/abac/src/index.ts
📚 Learning: 2025-10-27T14:38:46.994Z
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 37303
File: apps/meteor/tests/end-to-end/api/abac.ts:1125-1137
Timestamp: 2025-10-27T14:38:46.994Z
Learning: In Rocket.Chat ABAC feature, when ABAC is disabled globally (ABAC_Enabled setting is false), room-level ABAC attributes are not evaluated when changing room types. This means converting a private room to public will succeed even if the room has ABAC attributes, as long as the global ABAC setting is disabled.
Applied to files:
packages/core-services/src/types/IAbacService.tsapps/meteor/ee/server/api/abac/schemas.tsapps/meteor/ee/server/api/abac/index.tsapps/meteor/tests/end-to-end/api/abac.ts
🧬 Code graph analysis (5)
packages/core-services/src/types/IAbacService.ts (1)
packages/core-typings/src/IRoom.ts (1)
IRoom(22-98)
apps/meteor/ee/server/api/abac/schemas.ts (1)
packages/core-typings/src/IRoom.ts (1)
IRoom(22-98)
apps/meteor/ee/server/api/abac/index.ts (3)
apps/meteor/ee/server/api/abac/schemas.ts (3)
GETAbacRoomsResponseValidator(262-262)GenericErrorSchema(220-220)GETAbacRoomsListQueryValidator(235-235)packages/rest-typings/src/v1/Ajv.ts (1)
validateUnauthorizedErrorResponse(70-70)packages/core-services/src/index.ts (1)
Abac(202-202)
apps/meteor/tests/end-to-end/api/abac.ts (2)
apps/meteor/tests/data/api-data.ts (2)
request(10-10)credentials(39-42)apps/meteor/tests/e2e/utils/create-target-channel.ts (1)
deleteTeam(66-68)
ee/packages/abac/src/index.ts (1)
packages/core-typings/src/IRoom.ts (1)
IRoom(22-98)
⏰ 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). (7)
- GitHub Check: 🔨 Test Storybook / Test Storybook
- GitHub Check: 🔨 Test Unit / Unit Tests
- GitHub Check: 🔎 Code Check / TypeScript
- GitHub Check: 🔎 Code Check / Code Lint
- GitHub Check: 📦 Meteor Build (coverage)
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🔇 Additional comments (3)
packages/core-services/src/types/IAbacService.ts (1)
19-24: listAbacRooms signature is consistent with service and API usageThe method shape (filters, pagination, return payload) aligns with
AbacService.listAbacRoomsand the/abac/roomsendpoint; thefilterTypeunion also matches the tests and AJV schema. No changes needed.ee/packages/abac/src/index.ts (1)
125-186: listAbacRooms implementation correctly targets private ABAC‑managed roomsThe query restriction (
t: 'p'plus non‑emptyabacAttributes) and the filter modes (roomName/attribute/value/all) are consistent with the ABAC model (only private rooms and team main rooms can be ABAC‑managed). Pagination viaRooms.findPaginatedmirrors the attributes listing and returns a clean{ rooms, offset, count, total }payload. Looks solid.Based on learnings
apps/meteor/ee/server/api/abac/index.ts (1)
16-18: GET /abac/rooms endpoint is correctly wired to the service and validatorsThe new route:
- Enforces auth and
abac-managementpermission.- Uses
GETAbacRoomsListQueryValidatorandgetPaginationItemsfor pagination and filters.- Delegates to
Abac.listAbacRooms({ offset, count, filter, filterType }).- Returns via
API.v1.success(result), matching the service payload.Once
GETAbacRoomsResponseSchemais updated to allow thesuccessproperty (see schemas.ts comment), this endpoint should validate and behave as intended.Also applies to: 286-312
Proposed changes (including videos or screenshots)
Issue(s)
https://rocketchat.atlassian.net/browse/ABAC-82
Steps to test or reproduce
Further comments
Summary by CodeRabbit