Draft
Conversation
Contributor
|
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 |
|
Contributor
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 @@
## develop #40232 +/- ##
========================================
Coverage 69.83% 69.84%
========================================
Files 3296 3296
Lines 119165 119217 +52
Branches 21493 21511 +18
========================================
+ Hits 83224 83264 +40
- Misses 32638 32660 +22
+ Partials 3303 3293 -10
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
coderabbitai
Bot
added
the
type: feature
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/meteor/client/views/admin/ABAC/ABACRoomsTab/RoomsContextualBar.tsx (1)
44-55:⚠️ Potential issue | 🟡 MinorCREATE operations bypass UI confirmation entirely while UPDATE operations are properly gated by the GenericModal.
The control flow shows asymmetry:
- Updates (line 68–69):
roomInfoexists →updateAction→ GenericModal confirmation required beforehandleSubmit(onSave)fires- Creates (line 70–71):
roomInfoabsent →handleSubmit(onSave)()executes directly without any confirmation dialogSince the mutation hardcodes
confirmed: trueunconditionally, the server-sideerror-abac-modification-not-confirmedguard is properly bypassed for updates (where the UI dialog already confirmed), but creates completely lack confirmation at both UI and network layers. If the intent is to require explicit user confirmation before creation (consistent with the update UX), add a confirmation modal to the create path as well, or derive theconfirmedflag from a tracked user interaction rather than hardcoding it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/client/views/admin/ABAC/ABACRoomsTab/RoomsContextualBar.tsx` around lines 44 - 55, The saveMutation currently hardcodes confirmed: true which lets CREATE requests bypass confirmation; update flows use roomInfo + updateAction + GenericModal to require confirmation. Modify the component so the create path also triggers the same confirmation flow (e.g., open the GenericModal when roomInfo is absent before calling handleSubmit(onSave)) or change saveMutation to accept a confirmed boolean from the UI and pass that value through instead of hardcoding; locate saveMutation, createOrUpdateABACRoom, roomInfo checks, updateAction, GenericModal, and handleSubmit(onSave) to implement the modal invocation or propagate the user-confirmed flag into the payload.
🧹 Nitpick comments (7)
ee/packages/abac/src/pdp/VirtruPDP.ts (2)
511-523: Consider logging members silently dropped from PDP evaluation.Members without an
entityKeyare pushed intoenrichedascompliant: falsewithout any log line. Elsewhere in this file (canAccessObject,onRoomAttributesChanged,evaluateUserRooms,onSubjectAttributesChanged) the same branch emits apdpLogger.warn({ msg: 'User has no entity key ...' }). For parity and to help operators debug surprising dry-run results on misconfigured deployments, consider adding the same warning here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ee/packages/abac/src/pdp/VirtruPDP.ts` around lines 511 - 523, When iterating members in the block that calls this.getUserEntityKey(m), add a pdpLogger.warn call for members where entityKey is falsy before pushing { ...rest, compliant: false } into enriched so dropped users are logged the same way as in canAccessObject/onRoomAttributesChanged/evaluateUserRooms/onSubjectAttributesChanged; include identifying info from the member (e.g., m.id or m.username/email) in the warning message to aid debugging and keep the existing flow of pushing to enriched unchanged.
461-504:MemberRowtype overstates what the aggregation returns.
MemberRow = IDryRunMember & { emails?: ... }andIDryRunMemberrequirescompliant: boolean, but the pipeline does not projectcompliant— it's assigned later when buildingenriched. Typing it asIDryRunMemberlets downstream code treatm.compliantas defined when at this point it'sundefined.🔧 Proposed type tightening
- type MemberRow = IDryRunMember & { emails?: IUser['emails'] }; + type MemberRow = Omit<IDryRunMember, 'compliant'> & { emails?: IUser['emails'] };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ee/packages/abac/src/pdp/VirtruPDP.ts` around lines 461 - 504, The MemberRow type currently declares MemberRow = IDryRunMember & { emails?: IUser['emails'] } inside dryRunRoomAttributes, which falsely promises a compliant:boolean property that the aggregation pipeline does not produce; change MemberRow to a tighter type that omits or makes compliant optional (e.g., Omit<IDryRunMember, 'compliant'> & { emails?: IUser['emails'] } or Partial<Pick<IDryRunMember, 'compliant'>> combined) so the compiler and callers cannot assume compliant is present until you later build the enriched objects.ee/packages/abac/src/index.ts (1)
431-463: Looks good, minor note on auditing.The flow (availability check → room lookup → optional attribute normalization/definition existence → PDP delegation → aggregate counters) is consistent with the rest of the service. Two small notes:
_actoris accepted but not used — since this is a non-mutating preview, skippingAuditis fine; just be aware the admin who previewed a potentially destructive change (e.g., attributes that would mark most members non-compliant) won't appear in the audit trail. If that's desired for compliance posture, a lightweightAudit.previewPerformed(...)-style entry could be added later.- When
attributesis an empty object,normalizedis[]and the PDP returns every active member ascompliant: true. That matches the "what happens if I clear attributes" preview intent, but worth a quick E2E assertion to lock it in.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ee/packages/abac/src/index.ts` around lines 431 - 463, The method dryRunRoomAttributes currently accepts _actor but doesn't record a preview audit; if you want this action tracked, add a lightweight audit call (e.g., Audit.previewPerformed or Audit.logPreview) inside dryRunRoomAttributes after room lookup (use symbols: dryRunRoomAttributes, getAbacRoom, room._id, attributes, _actor) passing the actor id, room id and the attributes payload; also add an end-to-end test that calls dryRunRoomAttributes with attributes = {} and asserts the PDP returns all active members as compliant to lock in the expected behavior (use PDP/dryRunRoomAttributes and ensureAttributeDefinitionsExist flows in the test).ee/packages/abac/src/pdp/types.ts (1)
10-15: Remove the unusedIDryRunResulttype and its re-export.The interface is never imported or used in the codebase. The service layer uses
IAbacDryRunResultfrom@rocket.chat/core-servicesfor the aggregated result, while PDPs return onlyIDryRunMember[]. SinceIDryRunResultexists only in the definition and re-export without any consumers, remove it along with the re-export frompdp/index.tsto eliminate unnecessary duplication.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ee/packages/abac/src/pdp/types.ts` around lines 10 - 15, Remove the unused IDryRunResult interface and its re-export: delete the IDryRunResult declaration in types.ts and remove any export of IDryRunResult from pdp/index.ts; leave IDryRunMember and the PDP return types intact and ensure imports/use of IAbacDryRunResult from `@rocket.chat/core-services` remain unchanged so aggregated results still come from the service layer.ee/packages/abac/src/dry-run.spec.ts (1)
76-79: Consider scoping the afterEach cleanup to the current describe's fixtures.
deleteMany({ _id: /^dr_/ })/deleteMany({ _id: /^r-dr/ })wipes alldr_*users andr-dr*rooms, which works today because every seeded id uses those prefixes. If a future test uses the same shared in-memory DB concurrently (per theacquireSharedInMemoryMongopattern) and shares thedr_prefix, cross-test pollution would silently delete its data. Prefixing with the current describe's run-id (e.g.,r-dr-${suiteId}) is more defensive.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ee/packages/abac/src/dry-run.spec.ts` around lines 76 - 79, The global cleanup in the afterEach using usersCol.deleteMany({ _id: /^dr_/ }) and roomsCol.deleteMany({ _id: /^r-dr/ }) is too broad and may remove other tests' data; update the cleanup to target only this describe's fixtures by using the run/suite identifier (e.g., suiteId or runId used when seeding) in the regex (e.g., build /^dr-<suiteId>/ and /^r-dr-<suiteId>/) and call deleteMany with those scoped regexes in the afterEach; locate the afterEach block and the seeding point that defines the suite/run id (the variables referenced when creating test ids) and use that same id to scope deletions so only this describe's dr_* and r-dr* documents are removed.apps/meteor/ee/server/api/abac/schemas.ts (1)
309-330: Refactor: reuseRoomAttributesRecordfor the dry-run body to avoid drift.
PostRoomAbacAttributesDryRunBody.attributesis effectivelyRoomAttributesRecordminusminProperties: 1. Duplicating the schema means a future tweak (e.g., changing the value pattern orMAX_ROOM_ATTRIBUTE_VALUES) has to be applied in two places. Extracting a factory keeps them in lockstep.♻️ Proposed refactor
-const PostRoomAbacAttributesDryRunBody = { - type: 'object', - properties: { - attributes: { - type: 'object', - propertyNames: { type: 'string', pattern: ATTRIBUTE_KEY_PATTERN }, - maxProperties: MAX_ROOM_ATTRIBUTE_KEYS, - additionalProperties: { - type: 'array', - items: { type: 'string', minLength: 1, pattern: ATTRIBUTE_KEY_PATTERN }, - maxItems: MAX_ROOM_ATTRIBUTE_VALUES, - uniqueItems: true, - }, - }, - }, - required: ['attributes'], - additionalProperties: false, -}; +const { minProperties: _omit, ...RoomAttributesRecordNoMin } = RoomAttributesRecord; +const PostRoomAbacAttributesDryRunBody = { + type: 'object', + properties: { + attributes: RoomAttributesRecordNoMin, + }, + required: ['attributes'], + additionalProperties: false, +};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/ee/server/api/abac/schemas.ts` around lines 309 - 330, Replace the duplicated attributes definition in PostRoomAbacAttributesDryRunBody with the existing RoomAttributesRecord (or a small factory that returns RoomAttributesRecord with configurable minProperties) so both schemas share the same attribute rules; update PostRoomAbacAttributesDryRunBody to set attributes: RoomAttributesRecord (or call the factory with minProperties omitted/0 for dry-run) and keep required: ['attributes'] and additionalProperties: false, and then recompile POSTRoomAbacAttributesDryRunBodySchema using the shared RoomAttributesRecord type to avoid divergence between PostRoomAbacAttributesDryRunBody and RoomAttributesRecord.apps/meteor/tests/end-to-end/api/abac.ts (1)
3171-3184: Suggestion: also assert the room's stored ABAC attributes are unchanged.The test verifies membership is untouched but not that
room.abacAttributesdidn't pick up the dry-run payload. A quickrooms.infofetch assertingabacAttributesis empty/unchanged would close the loop on the "dry-run has no side effects" contract. The unit spec covers this, but an e2e guard is cheap.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/tests/end-to-end/api/abac.ts` around lines 3171 - 3184, Before invoking the dry-run, capture the room's current ABAC attributes (e.g., call rooms.info via request.get('/api/v1/rooms.info').query({ roomId: room._id }) and store body.room.abacAttributes), then after the dry-run request and membership checks perform another rooms.info fetch and assert that body.room.abacAttributes strictly equals the previously captured value (empty or unchanged). Update the test "does not mutate room membership" to use the existing request helper and room._id, and reference seedBulkDecisionByEntity and the dry-run POST to ensure the dry-run produced no side effects on room.abacAttributes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@apps/meteor/client/views/admin/ABAC/ABACRoomsTab/RoomsContextualBar.tsx`:
- Around line 44-55: The saveMutation currently hardcodes confirmed: true which
lets CREATE requests bypass confirmation; update flows use roomInfo +
updateAction + GenericModal to require confirmation. Modify the component so the
create path also triggers the same confirmation flow (e.g., open the
GenericModal when roomInfo is absent before calling handleSubmit(onSave)) or
change saveMutation to accept a confirmed boolean from the UI and pass that
value through instead of hardcoding; locate saveMutation,
createOrUpdateABACRoom, roomInfo checks, updateAction, GenericModal, and
handleSubmit(onSave) to implement the modal invocation or propagate the
user-confirmed flag into the payload.
---
Nitpick comments:
In `@apps/meteor/ee/server/api/abac/schemas.ts`:
- Around line 309-330: Replace the duplicated attributes definition in
PostRoomAbacAttributesDryRunBody with the existing RoomAttributesRecord (or a
small factory that returns RoomAttributesRecord with configurable minProperties)
so both schemas share the same attribute rules; update
PostRoomAbacAttributesDryRunBody to set attributes: RoomAttributesRecord (or
call the factory with minProperties omitted/0 for dry-run) and keep required:
['attributes'] and additionalProperties: false, and then recompile
POSTRoomAbacAttributesDryRunBodySchema using the shared RoomAttributesRecord
type to avoid divergence between PostRoomAbacAttributesDryRunBody and
RoomAttributesRecord.
In `@apps/meteor/tests/end-to-end/api/abac.ts`:
- Around line 3171-3184: Before invoking the dry-run, capture the room's current
ABAC attributes (e.g., call rooms.info via
request.get('/api/v1/rooms.info').query({ roomId: room._id }) and store
body.room.abacAttributes), then after the dry-run request and membership checks
perform another rooms.info fetch and assert that body.room.abacAttributes
strictly equals the previously captured value (empty or unchanged). Update the
test "does not mutate room membership" to use the existing request helper and
room._id, and reference seedBulkDecisionByEntity and the dry-run POST to ensure
the dry-run produced no side effects on room.abacAttributes.
In `@ee/packages/abac/src/dry-run.spec.ts`:
- Around line 76-79: The global cleanup in the afterEach using
usersCol.deleteMany({ _id: /^dr_/ }) and roomsCol.deleteMany({ _id: /^r-dr/ })
is too broad and may remove other tests' data; update the cleanup to target only
this describe's fixtures by using the run/suite identifier (e.g., suiteId or
runId used when seeding) in the regex (e.g., build /^dr-<suiteId>/ and
/^r-dr-<suiteId>/) and call deleteMany with those scoped regexes in the
afterEach; locate the afterEach block and the seeding point that defines the
suite/run id (the variables referenced when creating test ids) and use that same
id to scope deletions so only this describe's dr_* and r-dr* documents are
removed.
In `@ee/packages/abac/src/index.ts`:
- Around line 431-463: The method dryRunRoomAttributes currently accepts _actor
but doesn't record a preview audit; if you want this action tracked, add a
lightweight audit call (e.g., Audit.previewPerformed or Audit.logPreview) inside
dryRunRoomAttributes after room lookup (use symbols: dryRunRoomAttributes,
getAbacRoom, room._id, attributes, _actor) passing the actor id, room id and the
attributes payload; also add an end-to-end test that calls dryRunRoomAttributes
with attributes = {} and asserts the PDP returns all active members as compliant
to lock in the expected behavior (use PDP/dryRunRoomAttributes and
ensureAttributeDefinitionsExist flows in the test).
In `@ee/packages/abac/src/pdp/types.ts`:
- Around line 10-15: Remove the unused IDryRunResult interface and its
re-export: delete the IDryRunResult declaration in types.ts and remove any
export of IDryRunResult from pdp/index.ts; leave IDryRunMember and the PDP
return types intact and ensure imports/use of IAbacDryRunResult from
`@rocket.chat/core-services` remain unchanged so aggregated results still come
from the service layer.
In `@ee/packages/abac/src/pdp/VirtruPDP.ts`:
- Around line 511-523: When iterating members in the block that calls
this.getUserEntityKey(m), add a pdpLogger.warn call for members where entityKey
is falsy before pushing { ...rest, compliant: false } into enriched so dropped
users are logged the same way as in
canAccessObject/onRoomAttributesChanged/evaluateUserRooms/onSubjectAttributesChanged;
include identifying info from the member (e.g., m.id or m.username/email) in the
warning message to aid debugging and keep the existing flow of pushing to
enriched unchanged.
- Around line 461-504: The MemberRow type currently declares MemberRow =
IDryRunMember & { emails?: IUser['emails'] } inside dryRunRoomAttributes, which
falsely promises a compliant:boolean property that the aggregation pipeline does
not produce; change MemberRow to a tighter type that omits or makes compliant
optional (e.g., Omit<IDryRunMember, 'compliant'> & { emails?: IUser['emails'] }
or Partial<Pick<IDryRunMember, 'compliant'>> combined) so the compiler and
callers cannot assume compliant is present until you later build the enriched
objects.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4a9592a9-f97b-4ea5-bc0a-e1bcadc7005f
📒 Files selected for processing (13)
apps/meteor/client/views/admin/ABAC/ABACRoomsTab/DeleteRoomModal.tsxapps/meteor/client/views/admin/ABAC/ABACRoomsTab/RoomsContextualBar.tsxapps/meteor/ee/server/api/abac/index.tsapps/meteor/ee/server/api/abac/schemas.tsapps/meteor/tests/end-to-end/api/abac.tsee/packages/abac/src/dry-run.spec.tsee/packages/abac/src/index.tsee/packages/abac/src/pdp/LocalPDP.tsee/packages/abac/src/pdp/VirtruPDP.tsee/packages/abac/src/pdp/index.tsee/packages/abac/src/pdp/types.tspackages/core-services/src/index.tspackages/core-services/src/types/IAbacService.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). (7)
- GitHub Check: 🚢 Build Docker (amd64, rocketchat, coverage)
- GitHub Check: 🚢 Build Docker (arm64, account-service, presence-service, omnichannel-transcript-service, cove...
- GitHub Check: 🚢 Build Docker (arm64, authorization-service, queue-worker-service, ddp-streamer-service, cove...
- GitHub Check: 🚢 Build Docker (amd64, account-service, presence-service, omnichannel-transcript-service, cove...
- GitHub Check: 🚢 Build Docker (amd64, authorization-service, queue-worker-service, ddp-streamer-service, cove...
- GitHub Check: 🚢 Build Docker (arm64, rocketchat, coverage)
- GitHub Check: 🔎 Code Check / Code Lint
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
apps/meteor/client/views/admin/ABAC/ABACRoomsTab/RoomsContextualBar.tsxpackages/core-services/src/index.tsee/packages/abac/src/pdp/index.tsapps/meteor/client/views/admin/ABAC/ABACRoomsTab/DeleteRoomModal.tsxee/packages/abac/src/pdp/types.tsee/packages/abac/src/pdp/LocalPDP.tsee/packages/abac/src/index.tsee/packages/abac/src/pdp/VirtruPDP.tsapps/meteor/ee/server/api/abac/index.tspackages/core-services/src/types/IAbacService.tsee/packages/abac/src/dry-run.spec.tsapps/meteor/tests/end-to-end/api/abac.tsapps/meteor/ee/server/api/abac/schemas.ts
**/*.spec.ts
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.spec.ts: Use descriptive test names that clearly communicate expected behavior in Playwright tests
Use.spec.tsextension for test files (e.g.,login.spec.ts)
Files:
ee/packages/abac/src/dry-run.spec.ts
🧠 Learnings (42)
📓 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: MartinSchoeler
Repo: RocketChat/Rocket.Chat PR: 37557
File: apps/meteor/client/views/admin/ABAC/AdminABACRooms.tsx:115-116
Timestamp: 2025-11-27T17:56:26.050Z
Learning: In Rocket.Chat, the GET /v1/abac/rooms endpoint (implemented in ee/packages/abac/src/index.ts) only returns rooms where abacAttributes exists and is not an empty array (query: { abacAttributes: { $exists: true, $ne: [] } }). Therefore, in components consuming this endpoint (like AdminABACRooms.tsx), room.abacAttributes is guaranteed to be defined for all returned rooms, and optional chaining before calling array methods like .join() is sufficient without additional null coalescing.
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-11-27T17:56:26.050Z
Learnt from: MartinSchoeler
Repo: RocketChat/Rocket.Chat PR: 37557
File: apps/meteor/client/views/admin/ABAC/AdminABACRooms.tsx:115-116
Timestamp: 2025-11-27T17:56:26.050Z
Learning: In Rocket.Chat, the GET /v1/abac/rooms endpoint (implemented in ee/packages/abac/src/index.ts) only returns rooms where abacAttributes exists and is not an empty array (query: { abacAttributes: { $exists: true, $ne: [] } }). Therefore, in components consuming this endpoint (like AdminABACRooms.tsx), room.abacAttributes is guaranteed to be defined for all returned rooms, and optional chaining before calling array methods like .join() is sufficient without additional null coalescing.
Applied to files:
apps/meteor/client/views/admin/ABAC/ABACRoomsTab/RoomsContextualBar.tsxapps/meteor/client/views/admin/ABAC/ABACRoomsTab/DeleteRoomModal.tsxee/packages/abac/src/pdp/types.tsee/packages/abac/src/pdp/LocalPDP.tsee/packages/abac/src/index.tsee/packages/abac/src/pdp/VirtruPDP.tsapps/meteor/ee/server/api/abac/index.tspackages/core-services/src/types/IAbacService.tsee/packages/abac/src/dry-run.spec.tsapps/meteor/tests/end-to-end/api/abac.tsapps/meteor/ee/server/api/abac/schemas.ts
📚 Learning: 2026-04-17T18:33:24.670Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 39858
File: apps/meteor/tests/e2e/apps/uikit-interactions.spec.ts:123-151
Timestamp: 2026-04-17T18:33:24.670Z
Learning: In RocketChat/Rocket.Chat (`apps/meteor/tests/e2e/apps/uikit-interactions.spec.ts`), `executeBlockActionHandler` invocations originating from a **modal** surface intentionally do NOT include a `block_action_room` (room property) in the interaction payload. Modals are not scoped to a room, so no room id is available in that context. Do not flag the absence of a room assertion in the modal block-action test as a missing coverage bug; instead, document it explicitly with a `test.step` asserting the room entry is `undefined`.
Applied to files:
apps/meteor/client/views/admin/ABAC/ABACRoomsTab/RoomsContextualBar.tsxapps/meteor/client/views/admin/ABAC/ABACRoomsTab/DeleteRoomModal.tsxee/packages/abac/src/dry-run.spec.tsapps/meteor/tests/end-to-end/api/abac.ts
📚 Learning: 2025-10-30T19:30:46.541Z
Learnt from: MartinSchoeler
Repo: RocketChat/Rocket.Chat PR: 37244
File: apps/meteor/client/views/admin/ABAC/AdminABACRoomAttributesForm.spec.tsx:125-146
Timestamp: 2025-10-30T19:30:46.541Z
Learning: In the AdminABACRoomAttributesForm component (apps/meteor/client/views/admin/ABAC/AdminABACRoomAttributesForm.tsx), the first attribute value field is mandatory and does not have a Remove button. Only additional values beyond the first have Remove buttons. This means trashButtons[0] corresponds to the second value's Remove button, not the first value's.
Applied to files:
apps/meteor/client/views/admin/ABAC/ABACRoomsTab/RoomsContextualBar.tsxapps/meteor/client/views/admin/ABAC/ABACRoomsTab/DeleteRoomModal.tsx
📚 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:
apps/meteor/client/views/admin/ABAC/ABACRoomsTab/RoomsContextualBar.tsxee/packages/abac/src/pdp/LocalPDP.tsee/packages/abac/src/index.tsee/packages/abac/src/pdp/VirtruPDP.tsapps/meteor/ee/server/api/abac/index.tspackages/core-services/src/types/IAbacService.tsee/packages/abac/src/dry-run.spec.tsapps/meteor/tests/end-to-end/api/abac.tsapps/meteor/ee/server/api/abac/schemas.ts
📚 Learning: 2026-04-17T17:38:12.974Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 39858
File: packages/ui-kit/src/interactions/UserInteraction.ts:33-33
Timestamp: 2026-04-17T17:38:12.974Z
Learning: In RocketChat/Rocket.Chat (`packages/ui-kit/src/interactions/UserInteraction.ts`), `ViewSubmitUserInteraction` and `ViewClosedUserInteraction` intentionally do NOT include `rid` when the interaction originates from a **modal** surface. Modals are not scoped to a room, so no room id is available in that context. The `rid?: string` field is optional to support the contextual bar surface (where room context exists) while remaining absent for modals. Do not flag the absence of `rid` in modal viewSubmit/viewClosed interactions as a missing-context bug.
Applied to files:
apps/meteor/client/views/admin/ABAC/ABACRoomsTab/RoomsContextualBar.tsxapps/meteor/client/views/admin/ABAC/ABACRoomsTab/DeleteRoomModal.tsx
📚 Learning: 2026-01-17T01:51:47.764Z
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 38219
File: packages/core-typings/src/cloud/Announcement.ts:5-6
Timestamp: 2026-01-17T01:51:47.764Z
Learning: In packages/core-typings/src/cloud/Announcement.ts, the AnnouncementSchema.createdBy field intentionally overrides IBannerSchema.createdBy (object with _id and optional username) with a string enum ['cloud', 'system'] to match existing runtime behavior. This is documented as technical debt with a FIXME comment at apps/meteor/app/cloud/server/functions/syncWorkspace/handleCommsSync.ts:53 and should not be flagged as an error until the runtime behavior is corrected.
Applied to files:
apps/meteor/client/views/admin/ABAC/ABACRoomsTab/RoomsContextualBar.tsxapps/meteor/ee/server/api/abac/schemas.ts
📚 Learning: 2026-03-20T13:51:23.302Z
Learnt from: ggazzo
Repo: RocketChat/Rocket.Chat PR: 39553
File: apps/meteor/app/integrations/server/methods/incoming/updateIncomingIntegration.ts:179-181
Timestamp: 2026-03-20T13:51:23.302Z
Learning: In `apps/meteor/app/integrations/server/methods/incoming/updateIncomingIntegration.ts`, the truthiness guards `...(integration.avatar && { avatar })`, `...(integration.emoji && { emoji })`, `...(integration.alias && { alias })`, and `...(integration.script && { script })` in the `$set` payload of `updateIncomingIntegration` are intentional. Empty-string values for these fields should NOT overwrite the stored value — only truthy values are persisted. Do not flag these as bugs preventing explicit clears.
Applied to files:
apps/meteor/client/views/admin/ABAC/ABACRoomsTab/RoomsContextualBar.tsx
📚 Learning: 2026-03-10T08:13:52.153Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 39414
File: apps/meteor/app/api/server/v1/rooms.ts:1241-1297
Timestamp: 2026-03-10T08:13:52.153Z
Learning: In the RocketChat/Rocket.Chat OpenAPI migration PRs for endpoints under apps/meteor/app/api/server/v1/rooms.ts, the pattern `ajv.compile<void>({...})` is intentionally used for the 200 response schema even when the endpoint returns `{ success: true }`. This is an established convention across all migrated endpoints (rooms.leave, rooms.favorite, rooms.delete, rooms.muteUser, rooms.unmuteUser). Do not flag this as a type mismatch during reviews of these migration PRs.
Applied to files:
apps/meteor/client/views/admin/ABAC/ABACRoomsTab/RoomsContextualBar.tsxapps/meteor/client/views/admin/ABAC/ABACRoomsTab/DeleteRoomModal.tsxapps/meteor/tests/end-to-end/api/abac.tsapps/meteor/ee/server/api/abac/schemas.ts
📚 Learning: 2026-03-27T14:52:56.865Z
Learnt from: dougfabris
Repo: RocketChat/Rocket.Chat PR: 39892
File: apps/meteor/client/views/room/contextualBar/Threads/Thread.tsx:150-155
Timestamp: 2026-03-27T14:52:56.865Z
Learning: In Rocket.Chat, there are two different `ModalBackdrop` components with different prop APIs. During review, confirm the import source: (1) `rocket.chat/fuselage` `ModalBackdrop` uses `ModalBackdropProps` based on `BoxProps` (so it supports `onClick` and other Box/DOM props) and does not have an `onDismiss` prop; (2) `rocket.chat/ui-client` `ModalBackdrop` uses a narrower props interface like `{ children?: ReactNode; onDismiss?: () => void }` and handles Escape keypress and outside mouse-up, and it does not forward arbitrary DOM props such as `onClick`. Flag mismatched props (e.g., `onDismiss` passed to the fuselage component or `onClick` passed to the ui-client component) and ensure the usage matches the correct component being imported.
Applied to files:
apps/meteor/client/views/admin/ABAC/ABACRoomsTab/RoomsContextualBar.tsxapps/meteor/client/views/admin/ABAC/ABACRoomsTab/DeleteRoomModal.tsx
📚 Learning: 2026-02-25T20:10:16.987Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 38913
File: packages/ddp-client/src/legacy/types/SDKLegacy.ts:34-34
Timestamp: 2026-02-25T20:10:16.987Z
Learning: In the RocketChat/Rocket.Chat monorepo, packages/ddp-client and apps/meteor do not use TypeScript project references. Module augmentations in apps/meteor (e.g., declare module 'rocket.chat/rest-typings') are not visible when compiling packages/ddp-client in isolation, which is why legacy SDK methods that depend on OperationResult types for OpenAPI-migrated endpoints must remain commented out.
Applied to files:
packages/core-services/src/index.tsee/packages/abac/src/index.tsapps/meteor/ee/server/api/abac/schemas.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In the Rocket.Chat repository, do not reference Biome lint rules in code review feedback. Biome is not used even if biome.json exists; only reference Biome rules if there is explicit, project-wide usage documented. For TypeScript files, review lint implications without Biome guidance unless the project enables Biome rules.
Applied to files:
packages/core-services/src/index.tsee/packages/abac/src/pdp/index.tsee/packages/abac/src/pdp/types.tsee/packages/abac/src/pdp/LocalPDP.tsee/packages/abac/src/index.tsee/packages/abac/src/pdp/VirtruPDP.tsapps/meteor/ee/server/api/abac/index.tspackages/core-services/src/types/IAbacService.tsee/packages/abac/src/dry-run.spec.tsapps/meteor/tests/end-to-end/api/abac.tsapps/meteor/ee/server/api/abac/schemas.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In this repository (RocketChat/Rocket.Chat), Biome lint rules are not used even if a biome.json exists. When reviewing TypeScript files (e.g., packages/ui-voip/src/providers/useMediaSession.ts), ensure lint suggestions do not reference Biome-specific rules. Rely on general ESLint/TypeScript lint rules and project conventions instead.
Applied to files:
packages/core-services/src/index.tsee/packages/abac/src/pdp/index.tsee/packages/abac/src/pdp/types.tsee/packages/abac/src/pdp/LocalPDP.tsee/packages/abac/src/index.tsee/packages/abac/src/pdp/VirtruPDP.tsapps/meteor/ee/server/api/abac/index.tspackages/core-services/src/types/IAbacService.tsee/packages/abac/src/dry-run.spec.tsapps/meteor/tests/end-to-end/api/abac.tsapps/meteor/ee/server/api/abac/schemas.ts
📚 Learning: 2026-04-10T22:42:05.539Z
Learnt from: dougfabris
Repo: RocketChat/Rocket.Chat PR: 40075
File: apps/meteor/client/views/room/modals/FileUploadModal/FileUploadModal.tsx:69-71
Timestamp: 2026-04-10T22:42:05.539Z
Learning: In `apps/meteor/client/views/room/modals/FileUploadModal/FileUploadModal.tsx`, the submit handler converts an empty/whitespace-only description to `undefined` (`description?.trim() || undefined`) intentionally. All downstream image-rendering components (`AttachmentImage`, `ImagePreview`, `ImageItem`, `ImageGallery`) default `undefined` alt to `''`, so the `<img alt="">` attribute is always present. Do not flag this `undefined` conversion as a bug preventing alt text from being cleared.
Applied to files:
apps/meteor/client/views/admin/ABAC/ABACRoomsTab/DeleteRoomModal.tsx
📚 Learning: 2026-03-14T14:58:58.834Z
Learnt from: smirk-dev
Repo: RocketChat/Rocket.Chat PR: 39625
File: apps/meteor/app/api/server/v1/push.ts:85-97
Timestamp: 2026-03-14T14:58:58.834Z
Learning: In RocketChat/Rocket.Chat, the `push.token` POST/DELETE endpoints in `apps/meteor/app/api/server/v1/push.ts` were already migrated to the chained router API pattern on `develop` prior to PR `#39625`. `cleanTokenResult` (which strips `authToken` and returns `PushTokenResult`) and `isPushTokenPOSTProps`/`isPushTokenDELETEProps` validators already exist on `develop`. PR `#39625` only migrates `push.get` and `push.info` to the chained pattern. Do not flag `cleanTokenResult` or `PushTokenResult` as newly introduced behavior-breaking changes when reviewing this PR.
Applied to files:
apps/meteor/client/views/admin/ABAC/ABACRoomsTab/DeleteRoomModal.tsx
📚 Learning: 2026-04-18T12:32:50.305Z
Learnt from: rodrigok
Repo: RocketChat/Rocket.Chat PR: 38623
File: apps/meteor/app/lib/server/functions/cleanRoomHistory.ts:146-149
Timestamp: 2026-04-18T12:32:50.305Z
Learning: In `apps/meteor/app/lib/server/functions/cleanRoomHistory.ts` (PR `#38623`), the read-receipt cleanup (both `ReadReceipts.removeByMessageIds` and `ReadReceiptsArchive.removeByMessageIds`) is intentionally only performed in the limited prune path (`limit && selectedMessageIds`). The unlimited/delete-all path (`limit === 0`) deliberately skips cleaning up orphaned read receipts in both hot and cold storage — this is by design. Do not flag this as a bug or missing cleanup in future reviews.
Applied to files:
apps/meteor/client/views/admin/ABAC/ABACRoomsTab/DeleteRoomModal.tsx
📚 Learning: 2026-03-16T21:50:42.118Z
Learnt from: amitb0ra
Repo: RocketChat/Rocket.Chat PR: 39676
File: .changeset/migrate-users-register-openapi.md:3-3
Timestamp: 2026-03-16T21:50:42.118Z
Learning: In RocketChat/Rocket.Chat OpenAPI migration PRs, removing endpoint types and validators from `rocket.chat/rest-typings` (e.g., `UserRegisterParamsPOST`, `/v1/users.register` entry) is the *required* migration pattern per RocketChat/Rocket.Chat-Open-API#150 Rule 7 ("No More rest-typings or Manual Typings"). The endpoint type is re-exposed via a module augmentation `.d.ts` file in the consuming package (e.g., `packages/web-ui-registration/src/users-register.d.ts`). This is NOT a breaking change — the correct changeset bump for `rocket.chat/rest-typings` in this scenario is `minor`, not `major`. Do not flag this as a breaking change during OpenAPI migration reviews.
Applied to files:
apps/meteor/client/views/admin/ABAC/ABACRoomsTab/DeleteRoomModal.tsxapps/meteor/ee/server/api/abac/index.ts
📚 Learning: 2026-04-20T17:11:57.810Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 40225
File: apps/meteor/ee/server/apps/communication/endpoints/appLogsHandler.ts:55-71
Timestamp: 2026-04-20T17:11:57.810Z
Learning: In `apps/meteor/ee/server/apps/communication/endpoints/appLogsHandler.ts`, the concern about an empty `?appId=` query param bypassing the truthy check and overriding the path `appId` in the `makeAppLogsQuery` spread is not relevant. The AJV query schema (`isAppLogsProps`) validates and rejects invalid/empty `appId` values before the action handler is reached, making the in-handler guard sufficient as-is. Do not flag this pattern as a vulnerability in future reviews of this file.
Applied to files:
apps/meteor/client/views/admin/ABAC/ABACRoomsTab/DeleteRoomModal.tsx
📚 Learning: 2026-03-04T14:16:49.202Z
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 39304
File: packages/ui-contexts/src/ActionManagerContext.ts:26-26
Timestamp: 2026-03-04T14:16:49.202Z
Learning: In `packages/ui-contexts/src/ActionManagerContext.ts` (TypeScript, RocketChat/Rocket.Chat), the `disposeView` method in `IActionManager` uses an intentionally explicit union `UiKit.ModalView['id'] | UiKit.BannerView['viewId'] | UiKit.ContextualBarView['id']` to document which view types are accepted, even though all constituents resolve to the same primitive. The inline `// eslint-disable-next-line typescript-eslint/no-duplicate-type-constituents` comment is intentional and should not be flagged or removed.
Applied to files:
apps/meteor/client/views/admin/ABAC/ABACRoomsTab/DeleteRoomModal.tsx
📚 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:
ee/packages/abac/src/pdp/types.tsee/packages/abac/src/pdp/LocalPDP.tsee/packages/abac/src/index.tsee/packages/abac/src/pdp/VirtruPDP.tsapps/meteor/ee/server/api/abac/index.tspackages/core-services/src/types/IAbacService.tsee/packages/abac/src/dry-run.spec.tsapps/meteor/tests/end-to-end/api/abac.tsapps/meteor/ee/server/api/abac/schemas.ts
📚 Learning: 2026-03-09T18:39:21.178Z
Learnt from: Harxhit
Repo: RocketChat/Rocket.Chat PR: 39476
File: apps/meteor/server/methods/addAllUserToRoom.ts:0-0
Timestamp: 2026-03-09T18:39:21.178Z
Learning: In apps/meteor/server/methods/addAllUserToRoom.ts, the implementation uses a single cursor pass (Users.find(userFilter).batchSize(100)) that collects both the full user objects (collectedUsers: IUser[]) and their usernames (usernames: string[]) in one iteration. `beforeAddUserToRoom` is then called once with the full usernames batch (preserving batch-validation semantics), and the subsequent subscription/message processing loop iterates over the same stable `collectedUsers` array — no second DB query is made. This avoids any race condition between validation and processing while preserving the original batch-validation behavior.
Applied to files:
ee/packages/abac/src/pdp/LocalPDP.tsee/packages/abac/src/pdp/VirtruPDP.ts
📚 Learning: 2025-09-25T09:59:26.461Z
Learnt from: Dnouv
Repo: RocketChat/Rocket.Chat PR: 37057
File: packages/apps-engine/src/definition/accessors/IUserRead.ts:23-27
Timestamp: 2025-09-25T09:59:26.461Z
Learning: AppUserBridge.getUserRoomIds in apps/meteor/app/apps/server/bridges/users.ts always returns an array of strings (mapping subscription documents to room IDs), never undefined, even when user has no room subscriptions.
Applied to files:
ee/packages/abac/src/pdp/LocalPDP.tsee/packages/abac/src/pdp/VirtruPDP.ts
📚 Learning: 2025-09-25T09:59:26.461Z
Learnt from: Dnouv
Repo: RocketChat/Rocket.Chat PR: 37057
File: packages/apps-engine/src/definition/accessors/IUserRead.ts:23-27
Timestamp: 2025-09-25T09:59:26.461Z
Learning: AppUserBridge.getUserRoomIds in apps/meteor/app/apps/server/bridges/users.ts always returns an array of strings by mapping subscription documents to room IDs, never undefined, even when user has no room subscriptions.
Applied to files:
ee/packages/abac/src/pdp/LocalPDP.tsee/packages/abac/src/pdp/VirtruPDP.ts
📚 Learning: 2025-10-28T16:53:42.761Z
Learnt from: ricardogarim
Repo: RocketChat/Rocket.Chat PR: 37205
File: ee/packages/federation-matrix/src/FederationMatrix.ts:296-301
Timestamp: 2025-10-28T16:53:42.761Z
Learning: In the Rocket.Chat federation-matrix integration (ee/packages/federation-matrix/), the createRoom method from rocket.chat/federation-sdk will support a 4-argument signature (userId, roomName, visibility, displayName) in newer versions. Code using this 4-argument call is forward-compatible with planned library updates and should not be flagged as an error.
Applied to files:
ee/packages/abac/src/index.tsee/packages/abac/src/pdp/VirtruPDP.ts
📚 Learning: 2025-11-07T14:50:33.544Z
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 37423
File: packages/i18n/src/locales/en.i18n.json:18-18
Timestamp: 2025-11-07T14:50:33.544Z
Learning: Rocket.Chat settings: in apps/meteor/ee/server/settings/abac.ts, the Abac_Cache_Decision_Time_Seconds setting uses invalidValue: 0 as the fallback when ABAC is unlicensed. With a valid license, admins can still set the value to 0 to intentionally disable the ABAC decision cache.
Applied to files:
apps/meteor/ee/server/api/abac/index.ts
📚 Learning: 2026-03-12T10:26:26.697Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 39340
File: apps/meteor/app/api/server/v1/im.ts:1349-1398
Timestamp: 2026-03-12T10:26:26.697Z
Learning: In `apps/meteor/app/api/server/v1/im.ts` (PR `#39340`), the `DmEndpoints` type intentionally includes temporary stub entries for `/v1/im.kick`, `/v1/dm.kick`, `/v1/im.leave`, and `/v1/dm.leave` (using `DmKickProps` and `DmLeaveProps`) even though no route handlers exist for them yet. These stubs were added to preserve type compatibility after removing the original `DmLeaveProps` and related files. They are planned for cleanup in a follow-up PR. Do not flag these as missing implementations when reviewing this file until the follow-up is merged.
Applied to files:
apps/meteor/ee/server/api/abac/index.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Ensure tests run reliably in parallel without shared state conflicts
Applied to files:
ee/packages/abac/src/dry-run.spec.tsapps/meteor/tests/end-to-end/api/abac.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Group related tests in the same file
Applied to files:
ee/packages/abac/src/dry-run.spec.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Utilize Playwright fixtures (`test`, `page`, `expect`) for consistency in test files
Applied to files:
ee/packages/abac/src/dry-run.spec.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : All test files must be created in `apps/meteor/tests/e2e/` directory
Applied to files:
ee/packages/abac/src/dry-run.spec.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Maintain test isolation between test cases in Playwright tests
Applied to files:
ee/packages/abac/src/dry-run.spec.ts
📚 Learning: 2025-12-10T21:00:43.645Z
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 37091
File: ee/packages/abac/jest.config.ts:4-7
Timestamp: 2025-12-10T21:00:43.645Z
Learning: Adopt the monorepo-wide Jest testMatch pattern: <rootDir>/src/**/*.spec.{ts,js,mjs} (represented here as '**/src/**/*.spec.{ts,js,mjs}') to ensure spec files under any package's src directory are picked up consistently across all packages in the Rocket.Chat monorepo. Apply this pattern in jest.config.ts for all relevant packages to maintain uniform test discovery.
Applied to files:
ee/packages/abac/src/dry-run.spec.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to **/*.spec.ts : Use descriptive test names that clearly communicate expected behavior in Playwright tests
Applied to files:
ee/packages/abac/src/dry-run.spec.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Ensure clean state for each test execution in Playwright tests
Applied to files:
ee/packages/abac/src/dry-run.spec.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Use `expect` matchers for assertions (`toEqual`, `toContain`, `toBeTruthy`, `toHaveLength`, etc.) instead of `assert` statements in Playwright tests
Applied to files:
ee/packages/abac/src/dry-run.spec.ts
📚 Learning: 2026-02-24T19:22:48.358Z
Learnt from: juliajforesti
Repo: RocketChat/Rocket.Chat PR: 38493
File: apps/meteor/tests/e2e/omnichannel/omnichannel-send-pdf-transcript.spec.ts:66-67
Timestamp: 2026-02-24T19:22:48.358Z
Learning: In Playwright end-to-end tests (e.g., under apps/meteor/tests/e2e/...), prefer locating elements by translated text (getByText) and ARIA roles (getByRole) over data-qa attributes. If translation values change, update the corresponding test locators accordingly. Never use data-qa locators. This guideline applies to all Playwright e2e test specs in the repository and helps keep tests robust to UI text changes and accessible semantics.
Applied to files:
ee/packages/abac/src/dry-run.spec.ts
📚 Learning: 2026-03-06T18:10:15.268Z
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 39397
File: packages/gazzodown/src/code/CodeBlock.spec.tsx:47-68
Timestamp: 2026-03-06T18:10:15.268Z
Learning: In tests (especially those using testing-library/dom/jsdom) for Rocket.Chat components, the HTML <code> element has an implicit ARIA role of 'code'. Therefore, screen.getByRole('code') or screen.findByRole('code') will locate <code> elements even without a role attribute. Do not flag findByRole('code') as invalid in reviews; prefer using the implicit role instead of adding role="code" unless necessary for accessibility.
Applied to files:
ee/packages/abac/src/dry-run.spec.ts
📚 Learning: 2025-09-16T13:33:49.237Z
Learnt from: cardoso
Repo: RocketChat/Rocket.Chat PR: 36890
File: apps/meteor/tests/e2e/e2e-encryption/e2ee-otr.spec.ts:21-26
Timestamp: 2025-09-16T13:33:49.237Z
Learning: In Rocket.Chat test files, the im.delete API endpoint accepts either a `roomId` parameter (requiring the actual DM room _id) or a `username` parameter (for the DM partner's username). It does not accept slug-like constructions such as concatenating usernames together.
Applied to files:
apps/meteor/tests/end-to-end/api/abac.ts
📚 Learning: 2025-09-16T13:33:49.237Z
Learnt from: cardoso
Repo: RocketChat/Rocket.Chat PR: 36890
File: apps/meteor/tests/e2e/e2e-encryption/e2ee-otr.spec.ts:21-26
Timestamp: 2025-09-16T13:33:49.237Z
Learning: The im.delete API endpoint accepts either a `roomId` parameter (requiring the actual DM room _id) or a `username` parameter (for the DM partner's username). Constructing slug-like identifiers like `user2${Users.userE2EE.data.username}` doesn't work for this endpoint.
Applied to files:
apps/meteor/tests/end-to-end/api/abac.ts
📚 Learning: 2026-03-15T14:31:28.969Z
Learnt from: amitb0ra
Repo: RocketChat/Rocket.Chat PR: 39647
File: apps/meteor/app/api/server/v1/users.ts:710-757
Timestamp: 2026-03-15T14:31:28.969Z
Learning: In RocketChat/Rocket.Chat, the `UserCreateParamsPOST` type in `apps/meteor/app/api/server/v1/users.ts` (migrated from `packages/rest-typings/src/v1/users/UserCreateParamsPOST.ts`) intentionally has `fields: string` (non-optional) and `settings?: IUserSettings` without a corresponding AJV schema entry. This is a pre-existing divergence carried over verbatim from the original rest-typings source (PR `#39647`). Do not flag this type/schema misalignment during the OpenAPI migration review — it is tracked as a separate follow-up fix.
Applied to files:
apps/meteor/ee/server/api/abac/schemas.ts
📚 Learning: 2026-03-16T23:33:15.721Z
Learnt from: amitb0ra
Repo: RocketChat/Rocket.Chat PR: 39676
File: apps/meteor/app/api/server/v1/users.ts:862-869
Timestamp: 2026-03-16T23:33:15.721Z
Learning: In RocketChat/Rocket.Chat OpenAPI migration PRs (e.g., PR `#39676` for users.register in apps/meteor/app/api/server/v1/users.ts), calls to `this.parseJsonQuery()` inside migrated handlers are intentionally preserved without adding a corresponding `query` AJV schema to the route options. Adding query-param schemas for the `fields`/`sort`/`query` parameters consumed by `parseJsonQuery()` is a separate cross-cutting concern shared by many endpoints (e.g., users.create, users.update, users.list) and is explicitly out of scope for individual endpoint migration PRs. Do not flag the absence of a `query` schema for `parseJsonQuery()` usage as a violation of OpenAPI/AJV contract during migration reviews.
Applied to files:
apps/meteor/ee/server/api/abac/schemas.ts
📚 Learning: 2026-03-20T13:52:29.575Z
Learnt from: ggazzo
Repo: RocketChat/Rocket.Chat PR: 39553
File: apps/meteor/app/api/server/v1/stats.ts:98-117
Timestamp: 2026-03-20T13:52:29.575Z
Learning: In `apps/meteor/app/api/server/v1/stats.ts`, the `statistics.telemetry` POST endpoint intentionally has no `body` AJV schema in its route options. The proper request body shape (a `params` array of telemetry event objects) has not been formally defined yet, so body validation is deferred to a follow-up. Do not flag the missing body schema for this endpoint during OpenAPI migration reviews.
Applied to files:
apps/meteor/ee/server/api/abac/schemas.ts
🔇 Additional comments (11)
ee/packages/abac/src/pdp/index.ts (1)
3-3: Re-export ofIDryRunResultmay be unnecessary.Same concern as in
pdp/types.ts— if no consumer importsIDryRunResultfrom this module, the re-export can be dropped (see previous comment).packages/core-services/src/types/IAbacService.ts (1)
13-50: LGTM.
IAbacDryRunResultcleanly mirrors the PDP-sideIDryRunMemberprojection and the new service method signature is consistent with the existing mutation methods (setRoomAbacAttributes, etc.).packages/core-services/src/index.ts (1)
58-58: LGTM.Re-export aligns with the new type in
./types/IAbacService.ee/packages/abac/src/pdp/LocalPDP.ts (1)
103-169: The dry-run compliance semantics matchbuildCompliantConditions.Both enforce the same AND-across-keys / ALL-values-required semantic:
buildCompliantConditionsreturns an array of conditions (implicitly ANDed), each requiring a$elemMatchwherevalues: { $all: values }— the user's array must contain all requested values.- The aggregation filters with
$setIsSubset: ['$$req.values', '$$ua.values']and requires a match for every attribute via$allElementsTrue— semantically identical (subset is equivalent to containing all requested values).The sort
{ compliant: 1, _sortName: 1 }correctly places non-compliant members first (booleanfalse < truein MongoDB), consistent with VirtruPDP's ordering.apps/meteor/client/views/admin/ABAC/ABACRoomsTab/DeleteRoomModal.tsx (1)
21-35: LGTM — confirmed flag wired correctly and error path now handled.Adding the
onErrortoast and passing{ confirmed: true }through the DELETE query matches the server contract inDELETERoomAbacAttributesQuerySchema(ajvQuery coerces the boolean). One small observation: sinceridis supplied at hook construction andconfirmedis the only query parameter, the intermediate arrow wrapper could simplify tomutationFn: () => deleteRoomAttributes({ confirmed: true })— which is already what's there, so nothing to change.apps/meteor/ee/server/api/abac/index.ts (2)
65-69: LGTM — explicit confirmation guard is consistently applied.All four mutating room-attribute endpoints (bulk POST, single POST/PUT, bulk DELETE, single DELETE) reject requests missing
confirmed: truewith the same error, matching the new schema additions and the e2e tests. Minor nit: for POST/PUT the confirm guard runs beforeABAC_Enabled, so a stale client hitting a disabled workspace would see "not-confirmed" instead of "not-enabled" — not worth reordering, just noting for future UX.Also applies to: 124-128, 154-158, 185-189, 215-219
81-108: Remove the verification request—the asymmetry is intentional.The sync is only required for the dry-run preview response (which includes role-based sorting via
rolePriorityin the aggregation pipeline). The enforcement path—onRoomAttributesChanged—determines compliance and kicks users based purely on attribute conditions and policy evaluation, without needingroomRolePriorities. Mutating endpoints correctly omit the sync call because they invokeonRoomAttributesChanged, which does not depend on synchronized role priority data.apps/meteor/tests/end-to-end/api/abac.ts (2)
356-420: Nice — confirmation guard covered for all five mutating endpoints.The suite asserts 400 +
not-confirmedfor every modified endpoint and the existing specs were consistently updated withconfirmed: true/?confirmed=true. Consider extending the bulk-attributes negative test at Line 367-377 with a variant whereconfirmed: falseis explicitly sent (distinct from "missing"), just to guard against a future loosening of the!== truecheck.
3132-3169: Review comment is incorrect—the test semantics are sound.The
permitValuesparameter lists entities that receive a PERMIT decision from the PDP (resulting incompliant: true). All other entities receive thedefaultDecision(DECISION_DENY in this case, resulting incompliant: false).At line 3133,
seedBulkDecisionByEntity([adminEmail, permitUser], 'DECISION_DENY')correctly sets up:
adminEmailandpermitUser→ PERMIT → compliantdenyUser(not listed) → default to DECISION_DENY → non-compliantThe expectations at lines 3143–3151 (compliantCount=2, permitUser and admin as true, denyUser as false) are correct. Variable names align with the test intent.
> Likely an incorrect or invalid review comment.apps/meteor/ee/server/api/abac/schemas.ts (2)
332-363: LGTM — dry-run response schema aligns withIAbacDryRunResult.
additionalProperties: trueonDryRunMemberis appropriate since member objects carry the full user projection. Requiring only_id,rolePriority, andcompliantkeeps the schema tolerant while the typed handle (ajv.compile<IAbacDryRunResult>) retains compile-time safety at call sites.
299-307: Schema correctly validates query parameters; no stray params observed in client-side code.The DELETE endpoint is called only from DeleteRoomModal.tsx (line 24) with
{ confirmed: true }, which matches the schema expectation. TheadditionalProperties: falseconstraint correctly rejects any unexpected query keys. Since this is an internal-only endpoint (no public/SDK consumers found), the configuration is appropriate. Note: the concern about reverse proxies adding tracking/locale parameters is valid from an operations standpoint but falls outside the scope of schema design—if a proxy were to inject params, the schema validation would reject them as intended.
KevLehman
force-pushed
the
feat/dry-run-abac
branch
from
dbd5aa9 to
cf917c9
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Proposed changes (including videos or screenshots)
Issue(s)
https://rocketchat.atlassian.net/browse/PRD-331
Steps to test or reproduce
Further comments
Summary by CodeRabbit
New Features
Tests