feat: ABAC #37091
feat: ABAC #37091
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 |
🦋 Changeset detectedLatest commit: 0f3d776 The changes in this PR will be included in the next version bump. This PR includes changesets to release 46 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
WalkthroughAdds a full Attribute-Based Access Control (ABAC) feature: new Abac service package, REST API and AJV schemas, LDAP sync, authorization integration, admin UI (attributes, rooms, logs, settings), client/server field mappings, hooks to enforce ABAC during invites/joins, and many tests and infra updates. Changes
Sequence Diagram(s)sequenceDiagram
actor Client
participant API as API Server
participant Auth as Authorization Service
participant Abac as AbacService
participant DB as Database
participant Cache as Decision Cache
Client->>API: Request access /room or invite action
API->>Auth: canAccessRoom(user, room)
Auth->>Abac: canAccessObject(room, user, READ, ROOM)
alt ABAC enabled & room has attributes
Abac->>Cache: lookup decision (user, room)
alt cached valid
Cache-->>Abac: decision
else
Abac->>DB: fetch user.abacAttributes
Abac->>DB: fetch room.abacAttributes
Abac->>DB: evaluate compliance (query builders)
alt non-compliant
Abac->>DB: remove user from room / update subscription
Abac->>DB: emit audit event
Abac-->>Auth: deny
else
Abac->>Cache: store decision with TTL
Abac-->>Auth: allow
end
end
else
Abac-->>Auth: bypass (legacy permission flow)
end
Auth-->>API: return decision
API-->>Client: success or access denied
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120+ minutes Areas needing extra attention:
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 #37091 +/- ##
============================================
- Coverage 67.61% 54.26% -13.36%
============================================
Files 3463 2598 -865
Lines 113694 49950 -63744
Branches 20902 11196 -9706
============================================
- Hits 76875 27104 -49771
+ Misses 34687 20673 -14014
- Partials 2132 2173 +41
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
KevLehman
force-pushed
the
feat/abac
branch
2 times, most recently
from
21899fa to
7b27c93
Compare
KevLehman
force-pushed
the
feat/abac
branch
3 times, most recently
from
60b89c5 to
5216368
Compare
KevLehman
force-pushed
the
feat/abac
branch
2 times, most recently
from
d9c7701 to
c88aeff
Compare
KevLehman
force-pushed
the
feat/abac
branch
2 times, most recently
from
3618811 to
c8039d9
Compare
📦 Docker Image Size Report📈 Changes
📊 Historical Trend---
config:
theme: "dark"
xyChart:
width: 900
height: 400
---
xychart
title "Image Size Evolution by Service (Last 30 Days + This PR)"
x-axis ["11/15 22:28", "11/16 01:28", "11/17 23:50", "11/18 22:53", "11/19 23:02", "11/21 16:49", "11/24 17:34", "11/27 22:32", "11/28 19:05", "12/01 23:01", "12/02 21:57", "12/03 21:00", "12/04 18:17", "12/05 21:56", "12/08 20:15", "12/09 22:17", "12/10 23:26", "12/11 21:56", "12/12 22:45", "12/13 01:34", "12/15 22:31", "12/16 22:18", "12/17 17:50", "12/17 20:08 (PR)"]
y-axis "Size (GB)" 0 --> 0.5
line "account-service" [0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11]
line "authorization-service" [0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11]
line "ddp-streamer-service" [0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12]
line "omnichannel-transcript-service" [0.14, 0.14, 0.14, 0.14, 0.14, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13]
line "presence-service" [0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11]
line "queue-worker-service" [0.14, 0.14, 0.14, 0.14, 0.14, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13]
line "rocketchat" [0.36, 0.36, 0.35, 0.35, 0.35, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.35]
line "stream-hub-service" [0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11]
Statistics (last 23 days):
ℹ️ About this reportThis report compares Docker image sizes from this build against the
Updated: Wed, 17 Dec 2025 20:08:16 GMT |
KevLehman
force-pushed
the
feat/abac
branch
2 times, most recently
from
5730c69 to
3a37b8b
Compare
KevLehman
force-pushed
the
feat/abac
branch
3 times, most recently
from
e760262 to
4fc24bd
Compare
KevLehman
force-pushed
the
feat/abac
branch
2 times, most recently
from
cdc3da5 to
b3fd9fb
Compare
|
@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: 17
🧹 Nitpick comments (66)
docker-compose-ci.yml (1)
235-249: Consider adding a healthcheck or startup verification.The OpenLDAP service lacks a healthcheck or readiness probe, unlike other services in this compose file (e.g., rocketchat). For LDAP-dependent operations during ABAC sync, adding a startup verification would ensure the service is ready before downstream services attempt to connect. Additionally, verify that the relative volume path
./development/ldapexists or is created as part of setup.Consider adding a healthcheck block:
openldap: image: bitnamilegacy/openldap:<pinned-version> volumes: - ./development/ldap:/opt/bitnami/openldap/data/ environment: - LDAP_ADMIN_USERNAME=admin - LDAP_ADMIN_PASSWORD=adminpassword - LDAP_ROOT=dc=space,dc=air - LDAP_ADMIN_DN=cn=admin,dc=space,dc=air - LDAP_CUSTOM_LDIF_DIR=/opt/bitnami/openldap/data - LDAP_LOGLEVEL=-1 - BITNAMI_DEBUG=false ports: - 1389:1389 - 1636:1636 + healthcheck: + test: ldapwhoami -x -D "cn=admin,dc=space,dc=air" -w adminpassword -h localhost || exit 1 + interval: 10s + timeout: 5s + retries: 3 + start_period: 10sapps/meteor/client/views/room/contextualBar/Info/RoomInfo/RoomInfo.stories.tsx (1)
73-91: Consider using a private room type for consistency with ABAC behavior.The ABAC story uses a public room (
t: 'c') with ABAC attributes. Based on learnings, ABAC attributes can only be set on private rooms (type 'p') in the actual implementation. For better alignment with real-world behavior, consider overriding the room type in the story args.Apply this diff to align the story with ABAC's actual behavior:
ABAC.args = { ...Default.args, room: { ...roomArgs, + t: 'p', abacAttributes: [ { key: 'Chat-sensitivity', values: ['Classified', 'Top-Secret'] }, { key: 'Country', values: ['US-only'] }, { key: 'Project', values: ['Ruminator-2000'] }, ], }, };Based on learnings, ABAC attributes are only applicable to private rooms in production.
apps/meteor/app/lib/server/functions/addUserToDefaultChannels.ts (1)
17-19: ABAC gating logic is correct.The implementation correctly prevents users from being auto-subscribed to ABAC-protected default channels when ABAC is enabled. The optional chaining safely handles potentially undefined properties.
Consider caching
settings.get('ABAC_Enabled')outside the loop for a minor performance improvement:export const addUserToDefaultChannels = async function (user: IUser, silenced?: boolean): Promise<void> { await callbacks.run('beforeJoinDefaultChannels', user); const defaultRooms = await getDefaultChannels(); + const isAbacEnabled = settings.get('ABAC_Enabled'); for await (const room of defaultRooms) { - if (settings.get('ABAC_Enabled') && room?.abacAttributes?.length) { + if (isAbacEnabled && room?.abacAttributes?.length) { continue; }ee/apps/authorization-service/src/service.ts (1)
24-27: LGTM! Conditional service registration follows the correct pattern.The conditional registration allows flexible deployment where ABAC can run as a separate service or locally within the authorization service. The logic correctly registers
AbacServiceonly when an external ABAC service is not configured.Consider documenting the
USE_EXTERNAL_ABAC_SERVICEenvironment variable in deployment documentation or a README to help operators understand when and how to use distributed ABAC service deployment.apps/meteor/server/lib/dataExport/exportRoomMessagesToFile.ts (1)
158-160: ABAC message mapping looks good; consider consistent HTML stylingThe new
abac-removed-user-from-roommapping toi18n.t('abac_removed_user_from_the_room')is straightforward and safe for exports. For HTML exports, though, this system-style event is not currently rendered in italics, unlike other system messages initalicTypes.If you want consistent styling for all system/audit messages in HTML exports, consider adding this type to
italicTypes:- const italicTypes: IMessage['t'][] = ['uj', 'ul', 'au', 'r', 'ru', 'wm', 'livechat-close']; + const italicTypes: IMessage['t'][] = ['uj', 'ul', 'au', 'r', 'ru', 'wm', 'livechat-close', 'abac-removed-user-from-room'];Also applies to: 176-178
apps/meteor/app/slashcommands-invite/server/server.ts (1)
114-117: Empty backticks displayed when details is non-array or empty.When
e.detailsis not an array (e.g., a string, object, or undefined),detailsbecomes an empty string, resulting in the message displaying empty backticks (` `). Consider handling this more gracefully:- const details = Array.isArray(e.details) ? e.details.join(', ') : ''; + const details = Array.isArray(e.details) ? e.details.join(', ') : String(e.details ?? ''); void api.broadcast('notify.ephemeralMessage', userId, message.rid, { - msg: i18n.t(e.message, { lng: settings.get('Language') || 'en', details: `\`${details}\`` }), + msg: i18n.t(e.message, { lng: settings.get('Language') || 'en', details: details ? `\`${details}\`` : '' }), });development/ldap/02-data.ldif (1)
1-204: LDIF structure looks sound; only minor data/formatting nits
- Entries, objectClasses, required attributes, and group
memberDNs all look consistent and should import cleanly into a typical LDAP server.sncurrently holds given names (e.g.,sn: Alan) instead of surnames (Bean). If any code/tests rely onsnas “last name,” consider aligning these values for clarity.- There are stretches of multiple blank lines between some entries (around lines 136–143). Most parsers ignore them, but you may want to normalize to a single blank line between entries for readability and future diffs.
apps/meteor/app/invites/server/functions/findOrCreateInvite.ts (1)
66-71: ABAC guard logic is correct, but consider error code consistency.The ABAC guard correctly prevents invite creation for ABAC-managed rooms. The check appropriately uses optional chaining and validates both the global ABAC setting and room-specific attributes.
However, note that
apps/meteor/app/api/server/v1/teams.tsuses the error code'error-room-is-abac-managed'for the same scenario (line 242), while this file uses'error-invalid-room'. Consider using the more specific error code for consistency across the codebase.Apply this diff to align error codes:
- throw new Meteor.Error('error-invalid-room', 'Room is ABAC managed', { + throw new Meteor.Error('error-room-is-abac-managed', 'Room is ABAC managed', {apps/meteor/app/api/server/v1/teams.ts (1)
239-246: Consider renaming the ABAC check variable to avoid shadowing.The variable
roomis declared at line 240 inside the if-block, thenroomis declared again at line 246 in the outer scope. While technically correct (different scopes), this could reduce clarity.Apply this diff to improve clarity:
if (settings.get('ABAC_Enabled') && isDefault) { - const room = await Rooms.findOneByIdAndType(roomId, 'p', { projection: { abacAttributes: 1 } }); - if (room?.abacAttributes?.length) { + const targetRoom = await Rooms.findOneByIdAndType(roomId, 'p', { projection: { abacAttributes: 1 } }); + if (targetRoom?.abacAttributes?.length) { return API.v1.failure('error-room-is-abac-managed'); } }apps/meteor/app/invites/server/functions/validateInviteToken.ts (1)
30-35: ABAC guard logic is correct, but consider error code consistency.The ABAC guard correctly prevents invite token validation for ABAC-managed rooms. The positioning after room lookup and before expiration checks is appropriate.
However, this file uses the error code
'error-invalid-room', whileapps/meteor/app/api/server/v1/teams.tsuses'error-room-is-abac-managed'for the same scenario (line 242). Consider aligning with the more descriptive error code for consistency.Apply this diff to align error codes:
- throw new Meteor.Error('error-invalid-room', 'Room is ABAC managed', { + throw new Meteor.Error('error-room-is-abac-managed', 'Room is ABAC managed', {apps/meteor/tests/mocks/data.ts (1)
213-237: Consider makingactiveModulesdeterministic to always includeabac
createFakeLicenseInfobuildsactiveModulesusingfaker.helpers.arrayElements([... 'abac']), so'abac'is only present if the random subset happens to include it. Any tests that rely on ABAC being licensed by default when callingcreateFakeLicenseInfo()could become flaky or silently test the wrong branch.Given that tests can still override
activeModulesvia thepartialargument, it may be simpler and more predictable to return a fixed list here (including'abac') instead of a random subset:-export const createFakeLicenseInfo = (partial: Partial<Omit<LicenseInfo, 'license'>> = {}): Omit<LicenseInfo, 'license'> => ({ - activeModules: faker.helpers.arrayElements([ +export const createFakeLicenseInfo = (partial: Partial<Omit<LicenseInfo, 'license'>> = {}): Omit<LicenseInfo, 'license'> => ({ + activeModules: [ 'auditing', 'canned-responses', 'ldap-enterprise', 'livechat-enterprise', 'voip-enterprise', 'omnichannel-mobile-enterprise', 'engagement-dashboard', 'push-privacy', 'scalability', 'teams-mention', 'saml-enterprise', 'oauth-enterprise', 'device-management', 'federation', 'videoconference-enterprise', 'message-read-receipt', 'outlook-calendar', 'hide-watermark', 'custom-roles', 'accessibility-certification', 'outbound-messaging', 'abac', - ]), + ],
...partialat the end will still allow individual tests to overrideactiveModuleswhen they explicitly need a different license shape.apps/meteor/ee/server/hooks/abac/beforeAddUserToRoom.ts (1)
10-21: Type-safe filtering for usernames.The
filter(Boolean)call removes falsy values but doesn't narrow the TypeScript type. The castas string[]works at runtime but bypasses type checking. Consider using a type guard for safer narrowing.- const validUsers = users.filter(Boolean); - // No need to check ABAC when theres no users or when room is not private or when room is not ABAC managed - if (!validUsers.length || room.t !== 'p' || !room?.abacAttributes?.length) { + const validUsers = users.filter((u): u is string => Boolean(u)); + if (!validUsers.length || room.t !== 'p' || !room?.abacAttributes?.length) { return; } // Throw error (prevent add) if ABAC is disabled (setting, license) but room is ABAC managed if (!settings.get('ABAC_Enabled') || !License.hasModule('abac')) { throw new Error('error-room-is-abac-managed'); } - await Abac.checkUsernamesMatchAttributes(validUsers as string[], room.abacAttributes); + await Abac.checkUsernamesMatchAttributes(validUsers, room.abacAttributes);apps/meteor/app/lib/server/functions/removeUserFromRoom.ts (2)
18-22: Consider removing unusedskipAppPreEventsparameter.The
skipAppPreEventsoption is defined in theperformUserRemovalsignature but never used in the function body. It's only utilized inremoveUserFromRoom(line 92). SinceperformUserRemovalis documented as a low-level function that executes "only the necessary database operations, with no callbacks," the skip-pre-events logic doesn't apply here.If API consistency between the two functions is desired, consider adding a brief comment explaining that
skipAppPreEventsis unused here but kept for signature consistency. Otherwise, remove it from this function's signature:export const performUserRemoval = async function ( room: IRoom, user: IUser, - options?: { byUser?: IUser; skipAppPreEvents?: boolean; customSystemMessage?: MessageTypesValues }, + options?: { byUser?: IUser; customSystemMessage?: MessageTypesValues }, ): Promise<void> {
91-102: Consider clarifying the comment to reflect generic usage.The comment states "for an abac room" but the implementation doesn't verify whether the room has ABAC attributes—it simply checks the
skipAppPreEventsflag. While ABAC is likely the primary use case, the flag is a generic mechanism that delegates the decision to the caller.Consider revising the comment to be more accurate:
- // Rationale: for an abac room, we don't want apps to be able to prevent a user from leaving + // Skip app pre-events when requested by caller (e.g., ABAC-protected rooms where apps shouldn't prevent user removal)Otherwise, the gating logic and error handling are correctly implemented.
apps/meteor/ee/server/lib/abac/index.ts (1)
6-7: Consider awaitingPermissions.createto catch errors.Using
voidmakes this fire-and-forget, meaning any errors during permission creation are silently swallowed. SincecreatePermissionsisasyncand likely awaited by callers (e.g., in the configuration module), consider awaiting the permission creation to ensure errors propagate:for (const permission of permissions) { - void Permissions.create(permission._id, permission.roles); + await Permissions.create(permission._id, permission.roles); }apps/meteor/ee/server/lib/ldap/Manager.ts (1)
742-772: Consider caching the parsed mapping to avoid redundant JSON parsing.Both
updateUserAbacAttributesandsyncUserAbacAttributeparseLDAP_ABAC_AttributeMapfor each invocation. WhileupdateUserAbacAttributesparses once per sync operation (acceptable),syncUserAbacAttributeparses on each call - and it's called per-user in a loop fromsyncUsersAbacAttributes.Consider passing the pre-parsed mapping as a parameter to
syncUserAbacAttribute:- private static async syncUserAbacAttribute(ldap: LDAPConnection, user: IUser): Promise<void> { - const mapping = this.parseJson(settings.get('LDAP_ABAC_AttributeMap')); - if (!mapping) { - logger.error('LDAP to ABAC attribute mapping is not valid JSON'); - return; - } + private static async syncUserAbacAttribute(ldap: LDAPConnection, user: IUser, mapping: Record<string, string>): Promise<void> { const ldapUser = await this.findLDAPUser(ldap, user); if (!ldapUser) { return; } await Abac.addSubjectAttributes(user, ldapUser, mapping, undefined); }Then update
syncUsersAbacAttributesto parse once and pass the mapping:const mapping = this.parseJson(settings.get('LDAP_ABAC_AttributeMap')); if (!mapping) { logger.error('LDAP to ABAC attribute mapping is not valid JSON'); return; } for await (const user of users) { await this.syncUserAbacAttribute(ldap, user, mapping); }apps/meteor/tests/end-to-end/api/abac.ts (4)
516-566: Consolidate duplicatebeforehooks into a single hook.Static analysis correctly flags multiple
beforehooks within the samedescribeblock. While Mocha executes them sequentially, consolidating them improves readability and follows best practices.Apply this diff to consolidate:
- before('create team main room for rooms.saveRoomSettings default restriction test', async () => { - const createTeamMain = await request - .post(`${v1}/teams.create`) - .set(credentials) - .send({ name: teamNameMainRoom, type: 1 }) - .expect(200); - - mainRoomIdSaveSettings = createTeamMain.body.team?.roomId; - - await request.post(`${v1}/rooms.saveRoomSettings`).set(credentials).send({ rid: mainRoomIdSaveSettings, default: true }).expect(200); - }); - - before('create local ABAC attribute definition for tests', async () => { - await request - .post(`${v1}/abac/attributes`) - .set(credentials) - .send({ key: localAbacKey, values: ['red', 'green'] }) - .expect(200); - }); - - before('create private room and try to set it as default', async () => { - const res = await createRoom({ - type: 'p', - name: `abac-default-room-${Date.now()}`, - }); - privateDefaultRoomId = res.body.group._id; - - await request.post(`${v1}/rooms.saveRoomSettings`).set(credentials).send({ rid: privateDefaultRoomId, default: true }).expect(200); - }); - - before('create private team, private room inside it and set as team default', async () => { + before('setup test data: team main room, attribute definition, default rooms, and team', async () => { + const createTeamMain = await request + .post(`${v1}/teams.create`) + .set(credentials) + .send({ name: teamNameMainRoom, type: 1 }) + .expect(200); + mainRoomIdSaveSettings = createTeamMain.body.team?.roomId; + await request.post(`${v1}/rooms.saveRoomSettings`).set(credentials).send({ rid: mainRoomIdSaveSettings, default: true }).expect(200); + + await request + .post(`${v1}/abac/attributes`) + .set(credentials) + .send({ key: localAbacKey, values: ['red', 'green'] }) + .expect(200); + + const res = await createRoom({ + type: 'p', + name: `abac-default-room-${Date.now()}`, + }); + privateDefaultRoomId = res.body.group._id; + await request.post(`${v1}/rooms.saveRoomSettings`).set(credentials).send({ rid: privateDefaultRoomId, default: true }).expect(200); + const createTeamRes = await request.post(`${v1}/teams.create`).set(credentials).send({ name: teamName, type: 0 }).expect(200);
741-778: Consolidate duplicatebeforehooks.Same issue as above - multiple
beforehooks in the "ABAC Managed Room Default Conversion Restrictions" block should be merged into a single setup hook.
2113-2152: Consolidate duplicatebeforehooks in LDAP integration tests.The LDAP integration section has two
beforehooks that can be consolidated. The second hook already hasthis.timeout(10000)which would apply to the combined hook.- before(async () => { - await Promise.all([ - updateSetting('LDAP_Enable', true), - ... - ]); - }); - - before(async function () { - this.timeout(10000); + before(async function () { + this.timeout(10000); + await Promise.all([ + updateSetting('LDAP_Enable', true), + ... + ]); + // Wait for background sync to run once before tests start await request.post(`${v1}/ldap.syncNow`).set(credentials);
19-27: Consider using type assertion instead of@ts-expect-error.The
@ts-expect-errorsuppresses all type errors on the line. A more targeted approach would be a type assertion.- await connection.db().collection('users').updateOne( - { - // @ts-expect-error - collection types for _id - _id: userId, - }, + await connection.db().collection('users').updateOne( + { + _id: userId as unknown as import('mongodb').ObjectId, + }, { $set: { abacAttributes } }, );Alternatively, if the collection actually accepts string IDs, the comment is fine as-is for test code.
apps/meteor/ee/server/api/abac/index.ts (1)
374-398: Potential invalid date handling and redundant code.
- Line 379: Redundant optional chaining -
sort?.tsis evaluated twice.- Lines 385-386: If
startorendare malformed date strings,new Date()returns anInvalid Dateobject rather than throwing. This could silently produce unexpected query results.Consider simplifying the sort handling:
-const _sort = { ts: sort?.ts ? sort?.ts : -1 }; +const _sort = { ts: sort?.ts ?? -1 };For date validation, consider adding checks or relying on schema-level
format: 'date-time'validation to reject invalid strings before reaching this handler.apps/meteor/ee/server/api/abac/schemas.ts (6)
9-17: Type mismatch: Schema typed asvoidbut defines object structure.The schema defines an object with
success: boolean, but the TypeScript type isvoid. This won't cause runtime issues since AJV validates the actual response, but it reduces type safety when usingGenericSuccessSchema.Consider using a proper type:
-export const GenericSuccessSchema = ajv.compile<void>(GenericSuccess); +export const GenericSuccessSchema = ajv.compile<{ success: true }>(GenericSuccess);
20-36: Type mismatch: Schema allows partial updates but TypeScript type requires both fields.The schema uses
anyOf: [{ required: ['key'] }, { required: ['values'] }]allowing either field alone, but the TypeScript typeIAbacAttributeDefinitionrequires bothkeyandvalues. This creates a mismatch between runtime validation and compile-time types.Use a type that reflects the partial update semantics:
-export const PUTAbacAttributeUpdateBodySchema = ajv.compile<IAbacAttributeDefinition>(UpdateAbacAttributeBody); +export const PUTAbacAttributeUpdateBodySchema = ajv.compile<{ key?: string; values?: string[] }>(UpdateAbacAttributeBody);
57-70: Query parametersoffsetandcountshould likely be optional.The TypeScript type declares
offset: number; count: numberas required, but pagination parameters are typically optional with defaults. The schema doesn't include them in arequiredarray, so AJV allows them to be omitted, but the TypeScript type doesn't reflect this.Align the TypeScript type with schema behavior:
-export const GETAbacAttributesQuerySchema = ajv.compile<{ key?: string; values?: string; offset: number; count: number }>( +export const GETAbacAttributesQuerySchema = ajv.compile<{ key?: string; values?: string; offset?: number; count?: number }>( GetAbacAttributesQuery, );
112-138: Schema-type inconsistency:_idin schema but not in TypeScript type.The schema includes
_idandusageproperties, but onlykeyandvaluesare in therequiredarray. The TypeScript type includesusageas required but omits_id. This inconsistency could cause confusion.Either add
_idto the TypeScript type or remove it from the schema if it's not needed:export const GETAbacAttributeByIdResponseSchema = ajv.compile<{ + _id?: string; key: string; values: string[]; - usage: Record<string, boolean>; + usage?: Record<string, boolean>; }>(GetAbacAttributeByIdResponse);
262-294: Duplicate schema definitions for POST and PUT single attribute values.
PostSingleRoomAbacAttributeBodyandPutRoomAbacAttributeValuesBodyare identical. Consider consolidating into a shared schema to reduce duplication and maintenance burden.-const PostSingleRoomAbacAttributeBody = { +const SingleRoomAbacAttributeValuesBody = { type: 'object', properties: { values: { type: 'array', items: { type: 'string', minLength: 1, pattern: ATTRIBUTE_KEY_PATTERN }, minItems: 1, maxItems: MAX_ROOM_ATTRIBUTE_VALUES, uniqueItems: true, }, }, required: ['values'], additionalProperties: false, }; -export const POSTSingleRoomAbacAttributeBodySchema = ajv.compile<{ values: string[] }>(PostSingleRoomAbacAttributeBody); +export const POSTSingleRoomAbacAttributeBodySchema = ajv.compile<{ values: string[] }>(SingleRoomAbacAttributeValuesBody); -const PutRoomAbacAttributeValuesBody = { - type: 'object', - properties: { - values: { - type: 'array', - items: { type: 'string', minLength: 1, pattern: ATTRIBUTE_KEY_PATTERN }, - minItems: 1, - maxItems: MAX_ROOM_ATTRIBUTE_VALUES, - uniqueItems: true, - }, - }, - required: ['values'], - additionalProperties: false, -}; - -export const PUTRoomAbacAttributeValuesBodySchema = ajv.compile<{ values: string[] }>(PutRoomAbacAttributeValuesBody); +export const PUTRoomAbacAttributeValuesBodySchema = ajv.compile<{ values: string[] }>(SingleRoomAbacAttributeValuesBody);
296-306: Consider addingrequiredarray to GenericError schema.The
GenericErrorschema has norequiredarray, making bothsuccessandmessageoptional. While this may be intentional for flexibility, typical error responses should guarantee at leastsuccess: false.const GenericError = { type: 'object', properties: { success: { type: 'boolean', }, message: { type: 'string', }, }, + required: ['success'], };apps/meteor/client/components/UserInfo/UserInfoABACAttributes.tsx (1)
10-20: Optionally pass both key and value to the leaf component for richer displayIf
UserInfoABACAttributeis (or might be) expected to show both the attribute key and value (e.g.,department: HR), consider passing them separately instead of only the value. This keeps the rendering flexible without changing the layout here.- attribute.values.map((value, valueIndex) => ( + attribute.values.map((value, valueIndex) => ( <Margins inline={2} blockEnd={4} key={`${attribute.key}-${value}-${attributeIndex}-${valueIndex}`} > - <UserInfoABACAttribute attribute={value} /> + <UserInfoABACAttribute attributeKey={attribute.key} attributeValue={value} /> </Margins> )),apps/meteor/app/channel-settings/server/methods/saveRoomSettings.ts (2)
2-2: ABAC helper predicates and imports look consistent with ABAC semanticsThe
isAbacManagedRoom/isAbacManagedTeamhelpers plus the newITeamandsettingsimports are wired correctly: you only treat a room/team as ABAC-managed when it is private ('p'/TEAM_TYPE.PRIVATE),ABAC_Enabledis true, andabacAttributesis a non‑empty array. This matches the documented behavior that ABAC attributes are only relevant for private rooms/teams and should not be evaluated when the globalABAC_Enabledflag is off. Based on learnings, this preserves the “ABAC disabled → ignore room attributes” behavior.If you want to de‑duplicate the checks a bit, you could optionally express
isAbacManagedTeamin terms ofisAbacManagedRoom, e.g.:-const isAbacManagedTeam = (team: Partial<ITeam> | null, teamRoom: IRoom): boolean => { - return ( - team?.type === TEAM_TYPE.PRIVATE && - settings.get<boolean>('ABAC_Enabled') && - Array.isArray(teamRoom?.abacAttributes) && - teamRoom.abacAttributes.length > 0 - ); -}; +const isAbacManagedTeam = (team: Partial<ITeam> | null, teamRoom: IRoom): boolean => + team?.type === TEAM_TYPE.PRIVATE && isAbacManagedRoom(teamRoom);This keeps behavior the same while centralizing the ABAC‑enabled + attributes logic.
Also applies to: 14-14, 65-77
121-127: Room type ABAC guards: clarify intended behavior for team roomsThe two ABAC checks in the
roomTypevalidator behave as follows:
- Line 121:
isAbacManagedRoom(room) && value !== 'p'blocks changing any ABAC‑managed private room (including team rooms withabacAttributes) to a non‑private type whileABAC_Enabledis true.- Lines 147‑152:
isAbacManagedTeam(team, room) && value !== 'p'is intended to block changing ABAC‑managed private team rooms to public.Because the
isAbacManagedRoom(room)check runs before the team‑specific section and does not exclude team rooms, any team room that hasabacAttributeswill already be blocked by the firstif. The team‑specific check therefore only has an effect if you ever expect ABAC constraints to apply to rooms without their ownabacAttributesbut based on team metadata, which is not currently encoded inisAbacManagedTeam(it still inspectsteamRoom.abacAttributes).Please confirm that the intended behavior is:
- to restrict type changes only for the rooms that themselves carry ABAC attributes (in which case the second
ifis mostly redundant and could be simplified/removed), or- to also restrict type changes for rooms in an ABAC‑managed team based on attributes on the team main room (in which case
isAbacManagedTeammight need to look up the team main room viateam.roomIdinstead of using the current room).Once that intent is clear, it may be worth either dropping one of the checks or tightening
isAbacManagedTeamto express the team‑level rule explicitly. Based on learnings, ABAC attributes are attached to private rooms/teams, so this distinction can affect which rooms end up locked to'p'.Also applies to: 147-152
apps/meteor/client/views/admin/ABAC/hooks/useDeleteRoomModal.spec.tsx (1)
20-20: Remove redundantmockClear()call.The
mockSetModal.mockClear()on line 20 is redundant sincejest.clearAllMocks()on line 19 already clears all mocks, includingmockSetModal.Apply this diff to remove the redundant call:
beforeEach(() => { jest.clearAllMocks(); - mockSetModal.mockClear(); });apps/meteor/client/views/admin/ABAC/hooks/useRoomItems.spec.tsx (1)
42-123: Robust hook test coverage; only tiny cleanup opportunitiesThe suite nicely covers structure, ABAC availability gating, navigation, and delete-modal wiring for
useRoomItems, and the mocking ofuseRouter,useIsABACAvailable, anduseDeleteRoomModalis consistent with the hook implementation.Two minor nits you can consider (non-blocking):
- In
beforeEach(Line 42–47),jest.clearAllMocks()already clearsnavigateMock, sonavigateMock.mockClear()is redundant.- The
waitForusages aroundresult.current[0].disabled(Lines 67–75 and 100–108) are not strictly necessary since the flag is derived synchronously, but they don’t hurt correctness.apps/meteor/client/views/admin/moderation/helpers/DateRangePicker.tsx (1)
8-11: Effect only usesdefaultSelectedKeyon first mount; consider dependency alignmentRight now the initialization effect is:
useEffect(() => { handleOptionClick(defaultSelectedKey); }, []);This means:
- The initial range is applied once on mount (good for a default), but
- Later changes to
defaultSelectedKey(if any) are ignored, and- React hooks lint will typically warn about
defaultSelectedKeyandhandleOptionClickmissing from the dependency array.If consumers might change
defaultSelectedKeyover the lifetime of the component, or you want to stay aligned with hooks lint, consider:- useEffect(() => { - handleOptionClick(defaultSelectedKey); - }, []); + useEffect(() => { + handleOptionClick(defaultSelectedKey); + }, [defaultSelectedKey, handleOptionClick]);If the “run only once” behavior is intentional and
defaultSelectedKeyis truly static, it’d be good to document that assumption or explicitly suppress the lint warning.Also applies to: 27-27, 73-75, 79-79
apps/meteor/client/views/admin/ABAC/ABACRoomsTab/RoomsContextualBarWithData.tsx (1)
13-31: Data wiring looks correct; consider explicit error / empty-state handlingThe query + rendering logic is straightforward and matches the
RoomsContextualBarprops: you block onisLoading || isFetching, then passrid, a sensible name fallback, anddata?.abacAttributesonce the request resolves.One optional improvement would be to explicitly handle error or
!datacases—for example, by rendering an error state or a “room not found” view instead of always instantiatingRoomsContextualBarwith potentially undefinedattributesData. That would make the UX clearer when/v1/rooms.adminRooms.getRoomfails or returns no room.apps/meteor/client/views/admin/ABAC/ABACRoomsTab/RoomForm.tsx (1)
26-32: SimplifyhandleSubmit/handleSavewiring to avoid double wrappingThe current submit flow works but is more convoluted than necessary:
- The form uses
onSubmit={handleSubmit(handleSave)}.- Inside
handleSave, you callhandleSubmit(onSave)again (new room) or passhandleSubmit(onSave)intoupdateAction(edit room).- This leads to nested
handleSubmitcalls, validation potentially running twice, andonSavenot being directly tied to thedataargument thathandleSubmitprovides.You can simplify by letting
handleSaveaccept the validatedRoomFormDatadirectly and only deciding how to invokeonSave(immediately vs. via confirmation modal). For example:-const updateAction = useEffectEvent(async (action: () => void) => { +const updateAction = useEffectEvent((action: () => void) => { setModal( <GenericModal variant='info' icon={null} title={t('ABAC_Update_room_confirmation_modal_title')} annotation={t('ABAC_Update_room_confirmation_modal_annotation')} confirmText={t('Save_changes')} onConfirm={() => { action(); setModal(null); }} onCancel={() => setModal(null)} > <Trans i18nKey='ABAC_Update_room_content' values={{ roomName: roomInfo?.name }} components={{ bold: <Box is='span' fontWeight='bold' /> }} /> </GenericModal>, ); }); -const handleSave = useEffectEvent(() => { - if (roomInfo) { - updateAction(handleSubmit(onSave)); - } else { - handleSubmit(onSave)(); - } -}); +const handleSave = useEffectEvent((data: RoomFormData) => { + const doSave = () => onSave(data); + + if (roomInfo) { + updateAction(doSave); + } else { + doSave(); + } +}); return ( <> <ContextualbarScrollableContent> - <Box is='form' onSubmit={handleSubmit(handleSave)} id={formId}> + <Box is='form' onSubmit={handleSubmit(handleSave)} id={formId}>This keeps all validation in a single
handleSubmitcall, wiresRoomFormDatacleanly intoonSave, and still provides the confirm‑before‑save behavior when editing.Also applies to: 37-45, 67-73, 75-79, 131-136
ee/packages/abac/src/errors.ts (1)
1-87: Error hierarchy is clean; consider naming + missing subclass consistencyThe enum + subclass pattern looks solid and should make ABAC errors much easier to reason about. Two small polish points you might consider:
- Set the error
nameso stack traces and logging show the concrete class instead of plainError:export class AbacError extends Error { public readonly code: AbacErrorCode; public readonly details?: unknown; constructor(code: AbacErrorCode, details?: unknown) { - super(code); - this.code = code; - this.details = details; - - Object.setPrototypeOf(this, new.target.prototype); + super(code); + this.name = new.target.name; + this.code = code; + this.details = details; + + Object.setPrototypeOf(this, new.target.prototype); } }
- There is an
AbacErrorCode.UsernamesNotMatchingAbacAttributesentry without a dedicated subclass. If you expect to throw that condition in multiple places, adding a parallel subclass would keep the API symmetrical:export enum AbacErrorCode { InvalidAttributeValues = 'error-invalid-attribute-values', InvalidAttributeKey = 'error-invalid-attribute-key', AttributeNotFound = 'error-attribute-not-found', AttributeInUse = 'error-attribute-in-use', DuplicateAttributeKey = 'error-duplicate-attribute-key', AttributeDefinitionNotFound = 'error-attribute-definition-not-found', RoomNotFound = 'error-room-not-found', CannotConvertDefaultRoomToAbac = 'error-cannot-convert-default-room-to-abac', UsernamesNotMatchingAbacAttributes = 'error-usernames-not-matching-abac-attributes', AbacUnsupportedObjectType = 'error-abac-unsupported-object-type', AbacUnsupportedOperation = 'error-abac-unsupported-operation', } @@ export class AbacUnsupportedOperationError extends AbacError { constructor(details?: unknown) { super(AbacErrorCode.AbacUnsupportedOperation, details); } } + +export class AbacUsernamesNotMatchingAbacAttributesError extends AbacError { + constructor(details?: unknown) { + super(AbacErrorCode.UsernamesNotMatchingAbacAttributes, details); + } +}apps/meteor/client/views/admin/ABAC/ABACRoomsTab/RoomFormAttributeField.spec.tsx (1)
6-42: Story-based snapshot coverage looks fine; consider dropping the unnecessaryasyncThe jest mock for
useAttributeListand thecomposeStories-driven snapshot loop look good and should keep the component in sync with Storybook. The test body is markedasyncbut doesn’tawaitanything; you can safely make it sync to avoid implying there’s asynchronous behavior here:-test.each(testCases)(`renders %s without crashing`, async (_storyname, Story) => { +test.each(testCases)(`renders %s without crashing`, (_storyname, Story) => { const { baseElement } = render(<Story />); expect(baseElement).toMatchSnapshot(); });apps/meteor/client/views/admin/ABAC/ABACSettingTab/SettingsPage.tsx (1)
8-30: Settings page wiring is straightforward; watch the fallback copy spacingThe license gating via
useHasLicenseModule('abac')and delegation toSettingTogglelook good, and the LDAP docs link correctly useslinks.go.abacLDAPDocs. One minor UX nit: depending on howABAC_Enabled_calloutis translated, the inlineTransfallback string:User attributes are synchronized via LDAP <a>Learn more</a>may render as “LDAPLearn more” without a space if the i18n key is missing. If you expect the fallback to be visible in some locales, you might want an explicit space before the
<a>.apps/meteor/client/views/admin/ABAC/hooks/useDeleteRoomModal.tsx (1)
5-11: Hook is correct; consider stabilizing the returned callbackThe hook does what it says and composes nicely with
DeleteRoomModal. If the returned function is passed down as a prop (e.g., into lists), you might want to wrap it inuseCallbackto avoid changing identity on every render:-import { useSetModal } from '@rocket.chat/ui-contexts'; +import { useSetModal } from '@rocket.chat/ui-contexts'; +import { useCallback } from 'react'; @@ -export const useDeleteRoomModal = (room: { rid: string; name: string }) => { - const setModal = useSetModal(); - - return () => { - setModal(<DeleteRoomModal rid={room.rid} roomName={room.name} onClose={() => setModal(null)} />); - }; -}; +export const useDeleteRoomModal = (room: { rid: string; name: string }) => { + const setModal = useSetModal(); + + return useCallback(() => { + setModal(<DeleteRoomModal rid={room.rid} roomName={room.name} onClose={() => setModal(null)} />); + }, [room.rid, room.name, setModal]); +};apps/meteor/client/views/admin/ABAC/AdminABACTabs.tsx (1)
5-31: Tab routing is wired correctly; parameter name shadowing is a small readability nitThe tab selection logic (
selected={tab === '...'}) androuter.navigate({ name: 'admin-ABAC', params: { tab } })look consistent with the route shape.For readability, you might avoid shadowing the outer
tabvariable inhandleTabClick:- const tab = useRouteParameter('tab'); - const handleTabClick = (tab: string) => { - router.navigate({ - name: 'admin-ABAC', - params: { tab }, - }); - }; + const tab = useRouteParameter('tab'); + const handleTabClick = (nextTab: string) => { + router.navigate({ + name: 'admin-ABAC', + params: { tab: nextTab }, + }); + };apps/meteor/client/views/admin/ABAC/ABACRoomsTab/RoomFormAutocomplete.spec.tsx (1)
48-58: ConsolidatewaitForcalls into a single assertion block.Multiple separate
waitForcalls for related assertions add unnecessary overhead. Since these are all checking for elements that should appear together after the same async operation, consolidate them.- await waitFor(() => { - expect(screen.getByText('Room 1')).toBeInTheDocument(); - }); - - await waitFor(() => { - expect(screen.getByText('Room 2')).toBeInTheDocument(); - }); - - await waitFor(() => { - expect(screen.getByText('Room 3')).toBeInTheDocument(); - }); + await waitFor(() => { + expect(screen.getByText('Room 1')).toBeInTheDocument(); + expect(screen.getByText('Room 2')).toBeInTheDocument(); + expect(screen.getByText('Room 3')).toBeInTheDocument(); + });apps/meteor/client/views/admin/ABAC/ABACRoomsTab/RoomFormAutocomplete.stories.tsx (1)
11-37: Consider adding atitleto the meta configuration for consistency.Other story files in this PR (e.g.,
SettingToggle.stories.tsx) include atitleproperty in the meta configuration. Adding one here would improve Storybook navigation consistency.const meta: Meta<typeof RoomFormAutocomplete> = { + title: 'Admin/ABAC/RoomFormAutocomplete', component: RoomFormAutocomplete, parameters: {apps/meteor/client/views/admin/ABAC/ABACSettingTab/SettingToggle.spec.tsx (1)
53-56: Consider usingcleanuporafterEachfor modal teardown.The TODO comments indicate modals persist between tests. React Testing Library's
cleanupruns automatically after each test, but modals rendered via portals may need explicit cleanup.You could add an
afterEachhook to close modals:afterEach(() => { // If modals persist, consider resetting modal state here });Alternatively, investigate if the modal portal root needs to be cleared between tests.
apps/meteor/client/lib/queryKeys.ts (1)
144-157: Inconsistent query key structure betweenlogs.listand otherlistmethods.
logs.listincludes a'list'segment in the key, butroomAttributes.listandrooms.listdo not. This inconsistency could lead to confusion and potential cache key collisions (e.g.,roomAttributes.list(undefined)produces the same key asroomAttributes.all()).Consider aligning the patterns:
roomAttributes: { all: () => [...ABACQueryKeys.all, 'room-attributes'] as const, - list: (query?: PaginatedRequest) => [...ABACQueryKeys.roomAttributes.all(), query] as const, + list: (query?: PaginatedRequest) => [...ABACQueryKeys.roomAttributes.all(), 'list', query] as const, attribute: (attributeId: string) => [...ABACQueryKeys.roomAttributes.all(), attributeId] as const, }, rooms: { all: () => [...ABACQueryKeys.all, 'rooms'] as const, - list: (query?: PaginatedRequest) => [...ABACQueryKeys.rooms.all(), query] as const, + list: (query?: PaginatedRequest) => [...ABACQueryKeys.rooms.all(), 'list', query] as const, autocomplete: (query?: PaginatedRequest) => [...ABACQueryKeys.rooms.all(), 'autocomplete', query] as const, room: (roomId: string) => [...ABACQueryKeys.rooms.all(), roomId] as const, },apps/meteor/client/views/admin/ABAC/AdminABACRoute.tsx (1)
40-45: Remove unusedtfrom the dependency array.The translation function
tis listed in the dependency array but is not used within the effect body. This could cause unnecessary re-runs.useEffect(() => { // WS has never activated ABAC if (shouldShowUpsell && ABACEnabledSetting === undefined) { setModal(<ABACUpsellModal onClose={() => setModal(null)} onConfirm={handleManageSubscription} />); } - }, [shouldShowUpsell, setModal, t, handleManageSubscription, ABACEnabledSetting]); + }, [shouldShowUpsell, setModal, handleManageSubscription, ABACEnabledSetting]);apps/meteor/client/views/admin/ABAC/hooks/useAttributeOptions.tsx (2)
77-79: PrefermutateovermutateAsyncwhen not awaiting the result.Since the promise returned by
mutateAsyncis not being awaited or used,mutateis more appropriate and avoids an unhandled promise.onConfirm={() => { - deleteMutation.mutateAsync(undefined); + deleteMutation.mutate(undefined); }}
41-43: Consider extracting error message for toast display.Passing the error object directly to the toast may not display a user-friendly message. Extract the message property or provide a fallback.
onError: (error) => { - dispatchToastMessage({ type: 'error', message: error }); + dispatchToastMessage({ type: 'error', message: error instanceof Error ? error.message : String(error) }); },apps/meteor/client/views/admin/ABAC/ABACSettingTab/WarningModal.tsx (1)
38-41: Improve accessibility for the clickable link.The
Boxelement used as a clickable link lacks proper accessibility attributes. Screen readers and keyboard users won't be able to interact with it properly.- <Box is='a' onClick={handleNavigate}> - {' '} - ABAC {'>'} Rooms - </Box> + <Box is='a' href='#' role='button' tabIndex={0} onClick={handleNavigate} onKeyDown={(e) => e.key === 'Enter' && handleNavigate()}> + {' '} + ABAC {'>'} Rooms + </Box>Alternatively, consider using a
Buttonwithis='a'variant for better semantic meaning.apps/meteor/client/views/admin/ABAC/ABACAttributesTab/AttributesContextualBarWithData.tsx (1)
21-25: Consider separating initial load from background refetch states.Using
isLoading || isFetchingshows the skeleton on every refetch, causing UI flicker during background updates. Typically,isLoadingis sufficient for the initial skeleton, whileisFetchingcan be used for a subtler indicator.Also, there's no handling for the error case—if the query fails,
datawill be undefined andAttributesContextualBarmay receive unexpected props.-const { data, isLoading, isFetching } = useQuery({ +const { data, isLoading, isError } = useQuery({ queryKey: ABACQueryKeys.roomAttributes.attribute(id), queryFn: () => getAttributes(), staleTime: 0, }); -if (isLoading || isFetching) { +if (isLoading) { return <ContextualbarSkeletonBody />; } +if (isError || !data) { + // Consider rendering an error state or calling onClose + return null; +} + return <AttributesContextualBar attributeData={data} onClose={onClose} />;apps/meteor/client/views/admin/ABAC/ABACLogsTab/LogsPage.tsx (1)
80-101: Consider reducing duplication in getEventInfo.Both branches share identical logic for
userAvatar,user,action, andtimestamp. Extract the common parts to reduce repetition.const getEventInfo = ( event: Serialized<OperationResult<'GET', '/v1/abac/audit'>>['events'][number], ): { element: string; userAvatar: ReactNode; user: string; name: string; action: string; timestamp: Date } => { + const userAvatar = event.actor?.type === 'user' ? <UserAvatar size='x28' userId={event.actor._id} /> : null; + const user = event.actor?.type === 'user' ? event.actor.username : t('System'); + const action = getActionLabel(event.data?.find((item) => item.key === 'change')?.value); + const timestamp = new Date(event.ts); + if (event.t === 'abac.attribute.changed') { return { element: t('ABAC_Room_Attribute'), - userAvatar: event.actor?.type === 'user' ? <UserAvatar size='x28' userId={event.actor._id} /> : null, - user: event.actor?.type === 'user' ? event.actor.username : t('System'), + userAvatar, + user, name: event.data?.find((item) => item.key === 'attributeKey')?.value ?? '', - action: getActionLabel(event.data?.find((item) => item.key === 'change')?.value), - timestamp: new Date(event.ts), + action, + timestamp, }; } return { element: t('ABAC_Room'), - userAvatar: event.actor?.type === 'user' ? <UserAvatar size='x28' userId={event.actor._id} /> : null, - user: event.actor?.type === 'user' ? event.actor.username : t('System'), + userAvatar, + user, name: event.data?.find((item) => item.key === 'room')?.value?.name ?? '', - action: getActionLabel(event.data?.find((item) => item.key === 'change')?.value), - timestamp: new Date(event.ts), + action, + timestamp, }; };apps/meteor/client/views/admin/ABAC/hooks/useAttributeList.ts (1)
14-16: Address or track the TODO comment.The TODO comment indicates uncertainty about endpoint types. This should be resolved before merging or tracked in an issue to prevent technical debt.
Would you like me to help investigate the endpoint types, or should I open an issue to track this?
apps/meteor/client/views/admin/ABAC/hooks/useAttributeOptions.spec.tsx (2)
56-62: Redundant mock clearing inbeforeEach.
jest.clearAllMocks()already clears all mock functions includingmockNavigate,mockSetModal, andmockDispatchToastMessage. The individual.mockClear()calls are redundant.describe('useAttributeOptions', () => { beforeEach(() => { jest.clearAllMocks(); - mockNavigate.mockClear(); - mockSetModal.mockClear(); - mockDispatchToastMessage.mockClear(); });
124-145: Reset mock to default value to prevent test order dependency.The mock
useIsABACAvailableMockis set to returnfalsebut never reset totrue. If test execution order changes, subsequent tests expectingtruemay fail.Add reset in
beforeEach:beforeEach(() => { jest.clearAllMocks(); + useIsABACAvailableMock.mockReturnValue(true); });apps/meteor/client/views/admin/ABAC/ABACSettingTab/SettingToggle.tsx (1)
55-70: Unsafe cast ofpackageValueto boolean.
setting.packageValuehas typeSettingValuewhich could be various types. Casting directly tobooleanon line 59 may produce unexpected results if the setting is misconfigured.Use the same pattern as line 22 for consistency and safety:
const onReset = useCallback(() => { if (!setting) { return; } - const value = setting.packageValue as boolean; + const value = setting.packageValue === true; setModal(apps/meteor/client/views/admin/ABAC/AdminABACPage.tsx (1)
77-77: Unnecessary arrow function wrapper.
handleCloseContextualbaris already a stable reference fromuseEffectEvent. The arrow function wrapper creates a new function on each render.- <ContextualbarDialog onClose={() => handleCloseContextualbar()}> + <ContextualbarDialog onClose={handleCloseContextualbar}>The same applies to other usages on lines 80, 82, 88, and 90.
apps/meteor/client/views/admin/ABAC/ABACAttributesTab/AttributesPage.tsx (1)
71-105: Missing loading state indicator.When
isLoadingistrueanddataisundefined(initial load), the table structure renders with an empty body. Consider showing a loading skeleton or spinner for better UX during the initial fetch.+ {isLoading && ( + <Box display='flex' justifyContent='center' p={24}> + {/* Consider using a GenericTableLoadingState or Skeleton component */} + </Box> + )} - {(!data || data.attributes?.length === 0) && !isLoading ? ( + {!isLoading && (!data || data.attributes?.length === 0) ? ( <Box display='flex' justifyContent='center' height='full'>apps/meteor/client/views/admin/ABAC/ABACRoomsTab/RoomFormAttributeField.tsx (1)
49-57: Potential undefined access onoptionswhen deriving value options.The
optionsvariable fromuseAttributeListcould beundefinedwhen the query is disabled or in certain error states, yet theuseMemoaccessesoptions.find()without a guard. WhileisLoadingguards the render,optionsmay still be undefined in edge cases.const valueOptions: [string, string][] = useMemo(() => { if (!keyField.value) { return []; } - const selectedAttributeData = options.find((option) => option.value === keyField.value); + const selectedAttributeData = options?.find((option) => option.value === keyField.value); return selectedAttributeData?.attributeValues.map((value) => [value, value]) || []; }, [keyField.value, options]);apps/meteor/client/views/admin/ABAC/hooks/useRoomItems.tsx (1)
31-31: Simplify the disabled condition.The condition
isABACAvailable !== truecan be simplified to!isABACAvailablesinceuseIsABACAvailablereturns a boolean. The current form is functionally correct but unnecessarily verbose.- { id: 'edit', icon: 'edit' as const, content: t('Edit'), onClick: () => editAction(), disabled: isABACAvailable !== true }, + { id: 'edit', icon: 'edit' as const, content: t('Edit'), onClick: () => editAction(), disabled: !isABACAvailable },apps/meteor/client/views/admin/ABAC/ABACRoomsTab/RoomFormAutocomplete.tsx (1)
54-54: Add fallback for undefined options.When the query is loading or fails,
result.datacould be undefined, which would passundefinedto theoptionsprop. Consider providing an empty array fallback.- options={result.data} + options={result.data ?? []}apps/meteor/client/views/admin/ABAC/ABACAttributesTab/AttributesForm.tsx (2)
55-60: Error array length check may not behave as expected.The condition
errors.attributeValues?.length && errors.attributeValues?.length > 0checks if the errors object has alengthproperty greater than 0. However,errors.attributeValuesfrom react-hook-form is an object (sparse array) where only indices with errors are populated. Thelengthproperty may not accurately reflect the presence of errors.Consider using a check similar to
hasValuesErrors:const getAttributeValuesError = useCallback(() => { - if (errors.attributeValues?.length && errors.attributeValues?.length > 0) { + if (Array.isArray(errors?.attributeValues) && errors.attributeValues.some((error) => !!error?.value?.message)) { return t('Required_field', { field: t('Values') }); } return ''; }, [errors.attributeValues, t]);
121-121: Extract magic number 10 to a named constant.The maximum attribute values limit of 10 is hardcoded. Consider extracting this to a constant for clarity and consistency with
MAX_ABAC_ATTRIBUTE_VALUESused elsewhere in the codebase.+const MAX_ATTRIBUTE_VALUES = 10; + const AttributesForm = ({ onSave, onCancel, description }: AttributesFormProps) => {Then update the check:
- lockedAttributesFields.length + fields.length >= 10 + lockedAttributesFields.length + fields.length >= MAX_ATTRIBUTE_VALUESapps/meteor/client/views/admin/ABAC/ABACRoomsTab/RoomsPage.tsx (2)
90-94: Empty state shown prematurely during initial load.The condition
(!data || data.rooms?.length === 0) && !isLoadingcorrectly excludes loading state, but on initial render before the first fetch completes,datais undefined andisLoadingmay briefly be false (before the query starts), potentially causing a flash of the empty state.Consider using
isLoadingorisFetchingmore defensively:- {(!data || data.rooms?.length === 0) && !isLoading ? ( + {!isLoading && (!data || data.rooms?.length === 0) ? (Or add a loading skeleton state when
isLoadingis true.
85-85: Simplify disabled condition (same as useRoomItems).The condition
isABACAvailable !== truecan be simplified to!isABACAvailable.- <Button onClick={handleNewAttribute} primary mis={8} disabled={isABACAvailable !== true}> + <Button onClick={handleNewAttribute} primary mis={8} disabled={!isABACAvailable}>apps/meteor/client/views/admin/ABAC/ABACAttributesTab/AttributesContextualBar.tsx (1)
12-22: Unused propattributeIdin component signature.The
attributeIdprop is defined inAttributesContextualBarPropsbut not destructured or used in the component. Instead, the component retrieves it viauseRouteParameter('id')on line 46. Either remove the unused prop from the type or use the prop instead of the route parameter for consistency and testability.-type AttributesContextualBarProps = { - attributeId?: string; - attributeData?: { +type AttributesContextualBarProps = { + attributeData?: { key: string; values: string[]; usage: Record<string, boolean>; }; onClose: () => void; };ee/packages/abac/src/helper.ts (2)
145-151: Cache key may produce inconsistent results for reordered arrays.
JSON.stringifyproduces different strings for arrays with the same elements in different orders (e.g.,["a","b"]vs["b","a"]). If callers pass keys in varying orders, separate cache entries will be created for logically equivalent queries, reducing cache effectiveness.Consider sorting keys before caching:
-const getAttributeDefinitionsCached = mem(getAttributeDefinitionsFromDb, { - maxAge: 30_000, - cacheKey: JSON.stringify, -}); +const getAttributeDefinitionsCached = mem(getAttributeDefinitionsFromDb, { + maxAge: 30_000, + cacheKey: (keys: string[]) => JSON.stringify([...keys].sort()), +});
279-321: Duplicate code blocks for checking additions.The logic for detecting additions (lines 279-298) is repeated in the else branch (lines 300-320). Refactor to eliminate duplication.
- // Check additions (new keys or new values on existing keys) - if (!removed) { - for (const [key, nextValues] of nextMap) { - const currentValues = currentMap.get(key); - - if (!currentValues) { - // New key added - added = true; - break; - } - - for (const v of nextValues) { - if (!currentValues.has(v)) { - added = true; - break; - } - } - - if (added) { - break; - } - } - } else { - // Even if we've already seen removals, we might still want to know if additions happened too - for (const [key, nextValues] of nextMap) { - const currentValues = currentMap.get(key); - - if (!currentValues) { - added = true; - break; - } - - for (const v of nextValues) { - if (!currentValues.has(v)) { - added = true; - break; - } - } - - if (added) { - break; - } - } - } + // Check additions (new keys or new values on existing keys) + for (const [key, nextValues] of nextMap) { + const currentValues = currentMap.get(key); + + if (!currentValues) { + added = true; + break; + } + + for (const v of nextValues) { + if (!currentValues.has(v)) { + added = true; + break; + } + } + + if (added) { + break; + } + }
apps/meteor/client/views/admin/ABAC/ABACAttributesTab/AttributesContextualBar.tsx
Show resolved
Hide resolved
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 8
♻️ Duplicate comments (3)
apps/meteor/client/views/admin/ABAC/ABACAttributesTab/AttributesContextualBar.tsx (1)
54-65: Consider defensive filtering of empty values in payload.While form validation (required fields and disabled Save button) should prevent empty values from being submitted, adding defensive filtering would make the mutation more resilient to potential validation bypasses.
Apply the suggested fix from the previous review:
const payload = { key: data.name, - values: [...data.lockedAttributes.map((attribute) => attribute.value), ...data.attributeValues.map((attribute) => attribute.value)], + values: [ + ...data.lockedAttributes.map((attribute) => attribute.value), + ...data.attributeValues.map((attribute) => attribute.value), + ].filter((v) => v.trim().length > 0), };apps/meteor/client/views/admin/ABAC/ABACRoomsTab/RoomsContextualBar.tsx (2)
12-20: UnusedattributeIdprop still defined in type.The
attributeIdprop is declared inRoomsContextualBarPropsbut is not destructured or used by the component. Instead, line 39 usesuseRouteParameter('id'). Remove the unused prop from the type definition.type RoomsContextualBarProps = { - attributeId?: string; roomInfo?: { rid: string; name: string }; attributesData?: { key: string; values: string[] }[]; onClose: () => void; };
63-65: Error object still passed directly to toast message.Despite being marked as addressed, line 64 still passes the
errorobject directly todispatchToastMessage. Themessageproperty expects a string, buterroris anErrorobject, which will display[object Object]or cause type issues.onError: (error) => { - dispatchToastMessage({ type: 'error', message: error }); + dispatchToastMessage({ type: 'error', message: error instanceof Error ? error.message : String(error) }); },
🧹 Nitpick comments (12)
apps/meteor/client/views/admin/ABAC/ABACSettingTab/AbacEnabledToggle.tsx (1)
55-70: Consider conditionally showing the warning modal on reset.The
onResetfunction always shows theWarningModalregardless of whetherpackageValueistrueorfalse. If the package value istrue(enabling ABAC), showing a warning modal titled "Disable ABAC" would be confusing to users since they're actually enabling the feature.const onReset = useCallback(() => { if (!setting) { return; } const value = setting.packageValue as boolean; + if (value === true) { + setValue(value); + dispatch([{ _id: setting._id, value }]); + return; + } setModal( <WarningModal onConfirm={() => { setValue(value); dispatch([{ _id: setting._id, value }]); setModal(); }} onCancel={() => setModal()} />, ); }, [dispatch, setModal, setting]);apps/meteor/client/views/admin/ABAC/ABACAttributesTab/AttributesPage.tsx (1)
84-92: Consider optional chaining for attribute.values.While the backend likely ensures
valuesis always defined for returned attributes, adding optional chaining (attribute.values?.join(', ')) would make the component more resilient to unexpected API responses.-<GenericTableCell withTruncatedText>{attribute.values.join(', ')}</GenericTableCell> +<GenericTableCell withTruncatedText>{attribute.values?.join(', ') ?? ''}</GenericTableCell>apps/meteor/client/views/admin/ABAC/ABACAttributesTab/AttributesForm.tsx (1)
141-147: Remove implementation comment.The comment on line 143 explains RHF behavior but violates the coding guideline to avoid implementation comments. The logic is self-explanatory from the conditional check.
<Button onClick={() => append({ value: '' })} - // Checking for values since rhf does consider the newly added field as dirty after an append() call disabled={!!getAttributeValuesError() || attributeValues?.some((value: { value: string }) => value?.value === '')} > {t('Add_Value')} </Button>As per coding guidelines.
apps/meteor/client/views/admin/ABAC/ABACRoomsTab/RoomsContextualBar.tsx (1)
44-69: Consider adding loading state handling for the mutation.The
saveMutationdoesn't expose or useisPendingstate. Consider disabling form submission or showing a loading indicator while the mutation is in progress to prevent duplicate submissions.apps/meteor/tests/end-to-end/api/abac.ts (1)
507-557: Consider consolidating multiplebeforehooks for clarity.This describe block has three separate
beforehooks (lines 507, 519, 527, 537). While valid in Mocha, consolidating them into a single hook improves readability and makes the setup order explicit.- before('create team main room for rooms.saveRoomSettings default restriction test', async () => { + before('setup test fixtures', async () => { + // Create team main room for rooms.saveRoomSettings default restriction test const createTeamMain = await request .post(`${v1}/teams.create`) // ... - }); - before('create local ABAC attribute definition for tests', async () => { + // Create local ABAC attribute definition for tests await request .post(`${v1}/abac/attributes`) // ... - }); - before('create private room and try to set it as default', async () => { + // Create private room and try to set it as default const res = await createRoom({ // ... - }); - before('create private team, private room inside it and set as team default', async () => { + // Create private team, private room inside it and set as team default const createTeamRes = await request // ... });ee/packages/abac/src/user-auto-removal.spec.ts (1)
46-55: Consider adding cleanup between test suites.The
insertRoomandinsertUsershelpers insert data but there's no explicit cleanup between describe blocks. While MongoMemoryServer provides isolation, leftover data from one suite could potentially affect another if they share attribute keys.apps/meteor/client/views/admin/ABAC/ABACRoomsTab/RoomsPage.tsx (2)
74-83: Clarify Select options format.The Select options use a tuple with three elements
[value, label, boolean]. The third boolean's purpose isn't immediately clear. If it representsdisabled, consider using an object format or adding a comment for clarity.// Current format: [value, label, isSelected?] ['all', t('All'), true],
90-94: Consider adding a loading skeleton.When
isLoadingis true butdatais undefined (initial load), the component shows the "no results" state briefly before data arrives. Consider showing a loading skeleton or spinner during the initial fetch.- {(!data || data.rooms?.length === 0) && !isLoading ? ( + {isLoading ? ( + <Box display='flex' justifyContent='center' height='full'> + {/* Add loading skeleton or spinner */} + </Box> + ) : !data || data.rooms?.length === 0 ? ( <Box display='flex' justifyContent='center' height='full'> <GenericNoResults icon='list-alt' title={t('ABAC_No_rooms')} description={t('ABAC_No_rooms_description')} /> </Box>ee/packages/abac/src/index.ts (2)
495-506: Typo in comment and consider removing comments per guidelines.Line 499 has a typo: "subsciprtion" should be "subscription". Per coding guidelines, consider removing or minimizing code comments.
private shouldUseCache(decisionCacheTimeout: number, userSub: ISubscription) { - // Cases: - // 1) Never checked before -> check now - // 2) Checked before, but cache expired -> check now - // 3) Checked before, and cache valid -> use cached decision (subsciprtion exists) - // 4) Cache disabled (0) -> always check return ( decisionCacheTimeout > 0 && userSub.abacLastTimeChecked && Date.now() - userSub.abacLastTimeChecked.getTime() < decisionCacheTimeout * 1000 ); }
533-536: Minor clarity improvement:!!userSubis always truthy here.Since line 529-531 already returns early if
userSubis falsy, the expression!!userSubon line 535 will always evaluate totrue. Consider returningtruedirectly for clarity.if (this.shouldUseCache(decisionCacheTimeout, userSub)) { this.logger.debug({ msg: 'Using cached ABAC decision', userId: user._id, roomId: room._id }); - return !!userSub; + return true; }apps/meteor/client/views/admin/ABAC/ABACRoomsTab/RoomForm.tsx (1)
122-122: Consider extracting the attribute limit to a named constant.The value
10is hard-coded here and may be used elsewhere in the ABAC feature. Extracting it to a named constant (e.g.,MAX_ABAC_ATTRIBUTES = 10) would improve maintainability and make the intent clearer.apps/meteor/client/views/admin/ABAC/ABACSettingTab/SettingField.tsx (1)
88-97: Reset handler can sync directly to package defaults and drop the ESLint suppression
onResetButtonClickfirst restores local state from the currentsettingand then dispatchespackageValue/packageEditor. You can simplify this by updating both local state and the dispatched payload frompersistedSetting.packageValue/packageEditor, removing the need for thereact-hooks/exhaustive-depssuppression and avoiding a brief mismatch:- const onResetButtonClick = useCallback(() => { - setValue(setting.value); - setEditor(isSettingColor(setting) ? setting.editor : undefined); - update({ - value: persistedSetting.packageValue, - ...(isSettingColor(persistedSetting) && { editor: persistedSetting.packageEditor }), - }); - // eslint-disable-next-line react-hooks/exhaustive-deps - }, [setting.value, (setting as ISettingColor).editor, update, persistedSetting]); + const onResetButtonClick = useCallback(() => { + setValue(persistedSetting.packageValue); + setEditor(isSettingColor(persistedSetting) ? persistedSetting.packageEditor : undefined); + update({ + value: persistedSetting.packageValue, + ...(isSettingColor(persistedSetting) && { editor: persistedSetting.packageEditor }), + }); + }, [update, persistedSetting]);
📜 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 ignored due to path filters (3)
apps/meteor/client/views/admin/ABAC/ABACAttributesTab/__snapshots__/AttributesForm.spec.tsx.snapis excluded by!**/*.snapapps/meteor/client/views/admin/ABAC/ABACRoomsTab/__snapshots__/RoomFormAttributeField.spec.tsx.snapis excluded by!**/*.snapapps/meteor/client/views/admin/ABAC/ABACSettingTab/__snapshots__/SettingToggle.spec.tsx.snapis excluded by!**/*.snap
📒 Files selected for processing (22)
apps/meteor/client/views/admin/ABAC/ABACAttributesTab/AttributesContextualBar.tsx(1 hunks)apps/meteor/client/views/admin/ABAC/ABACAttributesTab/AttributesForm.spec.tsx(13 hunks)apps/meteor/client/views/admin/ABAC/ABACAttributesTab/AttributesForm.tsx(1 hunks)apps/meteor/client/views/admin/ABAC/ABACAttributesTab/AttributesPage.tsx(1 hunks)apps/meteor/client/views/admin/ABAC/ABACRoomsTab/RoomForm.tsx(1 hunks)apps/meteor/client/views/admin/ABAC/ABACRoomsTab/RoomFormAttributeField.tsx(1 hunks)apps/meteor/client/views/admin/ABAC/ABACRoomsTab/RoomsContextualBar.tsx(1 hunks)apps/meteor/client/views/admin/ABAC/ABACRoomsTab/RoomsPage.tsx(1 hunks)apps/meteor/client/views/admin/ABAC/ABACSettingTab/AbacEnabledToggle.tsx(1 hunks)apps/meteor/client/views/admin/ABAC/ABACSettingTab/SettingField.tsx(1 hunks)apps/meteor/client/views/admin/ABAC/ABACSettingTab/SettingToggle.spec.tsx(1 hunks)apps/meteor/client/views/admin/ABAC/ABACSettingTab/SettingToggle.stories.tsx(1 hunks)apps/meteor/client/views/admin/ABAC/ABACSettingTab/SettingsPage.tsx(1 hunks)apps/meteor/ee/server/api/abac/schemas.ts(1 hunks)apps/meteor/lib/publishFields.ts(1 hunks)apps/meteor/tests/end-to-end/api/abac.ts(1 hunks)ee/packages/abac/src/helper.spec.ts(1 hunks)ee/packages/abac/src/index.ts(1 hunks)ee/packages/abac/src/service.spec.ts(1 hunks)ee/packages/abac/src/user-auto-removal.spec.ts(1 hunks)packages/core-services/src/types/IAbacService.ts(1 hunks)packages/i18n/src/locales/en.i18n.json(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- ee/packages/abac/src/helper.spec.ts
- apps/meteor/ee/server/api/abac/schemas.ts
- apps/meteor/client/views/admin/ABAC/ABACSettingTab/SettingToggle.spec.tsx
- apps/meteor/client/views/admin/ABAC/ABACSettingTab/SettingsPage.tsx
- apps/meteor/client/views/admin/ABAC/ABACRoomsTab/RoomFormAttributeField.tsx
🧰 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/lib/publishFields.tsapps/meteor/client/views/admin/ABAC/ABACSettingTab/AbacEnabledToggle.tsxee/packages/abac/src/user-auto-removal.spec.tsapps/meteor/client/views/admin/ABAC/ABACRoomsTab/RoomsContextualBar.tsxapps/meteor/client/views/admin/ABAC/ABACAttributesTab/AttributesForm.tsxapps/meteor/client/views/admin/ABAC/ABACAttributesTab/AttributesContextualBar.tsxapps/meteor/client/views/admin/ABAC/ABACSettingTab/SettingField.tsxapps/meteor/client/views/admin/ABAC/ABACSettingTab/SettingToggle.stories.tsxapps/meteor/client/views/admin/ABAC/ABACRoomsTab/RoomForm.tsxee/packages/abac/src/service.spec.tsapps/meteor/client/views/admin/ABAC/ABACAttributesTab/AttributesPage.tsxapps/meteor/client/views/admin/ABAC/ABACAttributesTab/AttributesForm.spec.tsxpackages/core-services/src/types/IAbacService.tsapps/meteor/client/views/admin/ABAC/ABACRoomsTab/RoomsPage.tsxee/packages/abac/src/index.tsapps/meteor/tests/end-to-end/api/abac.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/user-auto-removal.spec.tsee/packages/abac/src/service.spec.ts
🧠 Learnings (18)
📓 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`.
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.
📚 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/lib/publishFields.tsee/packages/abac/src/user-auto-removal.spec.tsapps/meteor/client/views/admin/ABAC/ABACRoomsTab/RoomsContextualBar.tsxapps/meteor/client/views/admin/ABAC/ABACAttributesTab/AttributesContextualBar.tsxapps/meteor/client/views/admin/ABAC/ABACRoomsTab/RoomForm.tsxee/packages/abac/src/service.spec.tsapps/meteor/client/views/admin/ABAC/ABACAttributesTab/AttributesPage.tsxpackages/core-services/src/types/IAbacService.tsapps/meteor/client/views/admin/ABAC/ABACRoomsTab/RoomsPage.tsxee/packages/abac/src/index.tsapps/meteor/tests/end-to-end/api/abac.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:
apps/meteor/lib/publishFields.tsee/packages/abac/src/user-auto-removal.spec.tsee/packages/abac/src/service.spec.tspackages/core-services/src/types/IAbacService.tsee/packages/abac/src/index.tspackages/i18n/src/locales/en.i18n.jsonapps/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/lib/publishFields.tsapps/meteor/client/views/admin/ABAC/ABACSettingTab/AbacEnabledToggle.tsxapps/meteor/client/views/admin/ABAC/ABACRoomsTab/RoomsContextualBar.tsxapps/meteor/client/views/admin/ABAC/ABACAttributesTab/AttributesForm.tsxapps/meteor/client/views/admin/ABAC/ABACAttributesTab/AttributesContextualBar.tsxapps/meteor/client/views/admin/ABAC/ABACRoomsTab/RoomForm.tsxapps/meteor/client/views/admin/ABAC/ABACAttributesTab/AttributesPage.tsxapps/meteor/client/views/admin/ABAC/ABACAttributesTab/AttributesForm.spec.tsxapps/meteor/client/views/admin/ABAC/ABACRoomsTab/RoomsPage.tsx
📚 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/client/views/admin/ABAC/ABACSettingTab/AbacEnabledToggle.tsxapps/meteor/client/views/admin/ABAC/ABACSettingTab/SettingField.tsxapps/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 : Ensure tests run reliably in parallel without shared state conflicts
Applied to files:
ee/packages/abac/src/user-auto-removal.spec.tsee/packages/abac/src/service.spec.tsapps/meteor/client/views/admin/ABAC/ABACAttributesTab/AttributesForm.spec.tsxapps/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/user-auto-removal.spec.tsee/packages/abac/src/service.spec.tsapps/meteor/client/views/admin/ABAC/ABACAttributesTab/AttributesForm.spec.tsxapps/meteor/tests/end-to-end/api/abac.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/user-auto-removal.spec.tsee/packages/abac/src/service.spec.ts
📚 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/service.spec.tspackages/core-services/src/types/IAbacService.tsee/packages/abac/src/index.tspackages/i18n/src/locales/en.i18n.jsonapps/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 : Utilize Playwright fixtures (`test`, `page`, `expect`) for consistency in test files
Applied to files:
apps/meteor/client/views/admin/ABAC/ABACAttributesTab/AttributesForm.spec.tsxapps/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 : Use `expect` matchers for assertions (`toEqual`, `toContain`, `toBeTruthy`, `toHaveLength`, etc.) instead of `assert` statements in Playwright tests
Applied to files:
apps/meteor/client/views/admin/ABAC/ABACAttributesTab/AttributesForm.spec.tsx
📚 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/page-objects/**/*.ts : Utilize existing page objects pattern from `apps/meteor/tests/e2e/page-objects/`
Applied to files:
apps/meteor/client/views/admin/ABAC/ABACAttributesTab/AttributesForm.spec.tsxapps/meteor/client/views/admin/ABAC/ABACRoomsTab/RoomsPage.tsxapps/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 : Prefer web-first assertions (`toBeVisible`, `toHaveText`, etc.) in Playwright tests
Applied to files:
apps/meteor/client/views/admin/ABAC/ABACAttributesTab/AttributesForm.spec.tsx
📚 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/**/*.{ts,spec.ts} : Follow Page Object Model pattern consistently in Playwright tests
Applied to files:
apps/meteor/client/views/admin/ABAC/ABACAttributesTab/AttributesForm.spec.tsx
📚 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:
apps/meteor/client/views/admin/ABAC/ABACAttributesTab/AttributesForm.spec.tsx
📚 Learning: 2025-11-19T12:32:29.696Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 37547
File: packages/i18n/src/locales/en.i18n.json:634-634
Timestamp: 2025-11-19T12:32:29.696Z
Learning: Repo: RocketChat/Rocket.Chat
Context: i18n workflow
Learning: In this repository, new translation keys should be added to packages/i18n/src/locales/en.i18n.json only; other locale files are populated via the external translation pipeline and/or fall back to English. Do not request adding the same key to all locale files in future reviews.
Applied to files:
packages/i18n/src/locales/en.i18n.json
📚 Learning: 2025-11-19T18:20:07.720Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 37419
File: packages/i18n/src/locales/en.i18n.json:918-921
Timestamp: 2025-11-19T18:20:07.720Z
Learning: Repo: RocketChat/Rocket.Chat — i18n/formatting
Learning: This repository uses a custom message formatting parser in UI blocks/messages; do not assume standard Markdown rules. For keys like Call_ended_bold, Call_not_answered_bold, Call_failed_bold, and Call_transferred_bold in packages/i18n/src/locales/en.i18n.json, retain the existing single-asterisk emphasis unless maintainers request otherwise.
Applied to files:
packages/i18n/src/locales/en.i18n.json
📚 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:
apps/meteor/tests/end-to-end/api/abac.ts
🧬 Code graph analysis (7)
ee/packages/abac/src/user-auto-removal.spec.ts (3)
packages/models/src/index.ts (2)
Subscriptions(213-213)registerServiceModels(234-269)packages/core-typings/src/IAbacAttribute.ts (1)
IAbacAttributeDefinition(3-14)ee/packages/abac/src/audit.ts (1)
Audit(29-142)
apps/meteor/client/views/admin/ABAC/ABACSettingTab/SettingToggle.stories.tsx (1)
packages/mock-providers/src/index.ts (1)
mockAppRoot(3-3)
apps/meteor/client/views/admin/ABAC/ABACRoomsTab/RoomForm.tsx (2)
packages/ui-contexts/src/index.ts (2)
useTranslation(81-81)useSetModal(71-71)packages/ui-client/src/components/Contextualbar/index.ts (2)
ContextualbarScrollableContent(35-35)ContextualbarFooter(32-32)
ee/packages/abac/src/service.spec.ts (1)
ee/packages/abac/src/index.ts (1)
AbacService(45-671)
apps/meteor/client/views/admin/ABAC/ABACAttributesTab/AttributesForm.spec.tsx (2)
apps/meteor/client/views/admin/ABAC/ABACAttributesTab/AttributesForm.tsx (1)
AttributesFormFormData(18-22)apps/meteor/tests/e2e/page-objects/fragments/admin-flextab-emoji.ts (1)
nameInput(10-12)
packages/core-services/src/types/IAbacService.ts (3)
packages/core-typings/src/IUser.ts (1)
IUser(187-259)packages/core-typings/src/IAbacAttribute.ts (2)
IAbacAttributeDefinition(3-14)IAbacAttribute(16-16)packages/core-typings/src/IRoom.ts (1)
IRoom(22-98)
apps/meteor/tests/end-to-end/api/abac.ts (3)
packages/core-typings/src/IAbacAttribute.ts (1)
IAbacAttributeDefinition(3-14)packages/core-typings/src/IRoom.ts (1)
IRoom(22-98)packages/core-typings/src/IUser.ts (1)
IUser(187-259)
🪛 ast-grep (0.40.0)
apps/meteor/client/views/admin/ABAC/ABACSettingTab/SettingField.tsx
[warning] 111-111: Usage of dangerouslySetInnerHTML detected. This bypasses React's built-in XSS protection. Always sanitize HTML content using libraries like DOMPurify before injecting it into the DOM to prevent XSS attacks.
Context: dangerouslySetInnerHTML
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://reactjs.org/docs/dom-elements.html#dangerouslysetinnerhtml
- https://cwe.mitre.org/data/definitions/79.html
(react-unsafe-html-injection)
🪛 Biome (2.1.2)
ee/packages/abac/src/user-auto-removal.spec.ts
[error] 11-11: Expected an identifier, an array pattern, or an object pattern but instead found 'class'.
Expected an identifier, an array pattern, or an object pattern here.
(parse)
[error] 11-11: expected : but instead found {
Remove {
(parse)
[error] 14-14: ';' expected'
An explicit or implicit semicolon is expected here...
(parse)
[error] 14-14: ';' expected'
An explicit or implicit semicolon is expected here...
(parse)
[error] 15-15: await is only allowed within async functions and at the top levels of modules.
(parse)
[error] 16-16: await is only allowed within async functions and at the top levels of modules.
(parse)
[error] 18-18: Expected an expression but instead found '}'.
Expected an expression here.
(parse)
[error] 11-11: Unexpected empty object pattern.
(lint/correctness/noEmptyPattern)
apps/meteor/client/views/admin/ABAC/ABACSettingTab/SettingField.tsx
[error] 112-112: Avoid passing content using the dangerouslySetInnerHTML prop.
Setting content using code can expose users to cross-site scripting (XSS) attacks
(lint/security/noDangerouslySetInnerHtml)
apps/meteor/tests/end-to-end/api/abac.ts
[error] 519-525: Disallow duplicate setup and teardown hooks.
Disallow before duplicacy inside the describe function.
(lint/suspicious/noDuplicateTestHooks)
[error] 527-535: Disallow duplicate setup and teardown hooks.
Disallow before duplicacy inside the describe function.
(lint/suspicious/noDuplicateTestHooks)
[error] 537-557: Disallow duplicate setup and teardown hooks.
Disallow before duplicacy inside the describe function.
(lint/suspicious/noDuplicateTestHooks)
[error] 752-769: Disallow duplicate setup and teardown hooks.
Disallow before duplicacy inside the describe function.
(lint/suspicious/noDuplicateTestHooks)
[error] 2128-2141: Disallow duplicate setup and teardown hooks.
Disallow before duplicacy inside the describe function.
(lint/suspicious/noDuplicateTestHooks)
⏰ 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). (14)
- GitHub Check: 🔨 Test UI (EE) / MongoDB 5.0 (4/5)
- GitHub Check: 🔨 Test UI (EE) / MongoDB 8.2 [legacy watchers] coverage (3/5)
- GitHub Check: 🔨 Test UI (EE) / MongoDB 8.2 [legacy watchers] coverage (2/5)
- GitHub Check: 🔨 Test UI (EE) / MongoDB 8.2 [legacy watchers] coverage (5/5)
- GitHub Check: 🔨 Test UI (EE) / MongoDB 5.0 (1/5)
- GitHub Check: 🔨 Test UI (EE) / MongoDB 8.2 [legacy watchers] coverage (4/5)
- GitHub Check: 🔨 Test UI (CE) / MongoDB 8.2 (1/4)
- GitHub Check: 🔨 Test UI (EE) / MongoDB 8.2 [legacy watchers] coverage (1/5)
- GitHub Check: 🔨 Test UI (EE) / MongoDB 5.0 (3/5)
- GitHub Check: 🔨 Test UI (EE) / MongoDB 5.0 (2/5)
- GitHub Check: 🔨 Test API (EE) / MongoDB 8.2 coverage (1/1)
- GitHub Check: 🔨 Test UI (EE) / MongoDB 5.0 (5/5)
- GitHub Check: 🔨 Test UI (CE) / MongoDB 8.2 (2/4)
- GitHub Check: 🔨 Test UI (CE) / MongoDB 8.2 (4/4)
🔇 Additional comments (29)
apps/meteor/client/views/admin/ABAC/ABACSettingTab/SettingToggle.stories.tsx (1)
1-64: LGTM!The Storybook stories are well-structured with proper mock providers, translations, and setting configuration. The three stories (Default, Loading, False) provide good coverage for the component's main states.
apps/meteor/client/views/admin/ABAC/ABACSettingTab/AbacEnabledToggle.tsx (1)
16-93: LGTM on overall component structure.The component correctly manages local state synchronized with the setting, shows warning modals for disabling ABAC, and properly handles loading and disabled states. The integration with
MemoizedSettingand the modal flow is well-implemented.apps/meteor/lib/publishFields.ts (1)
130-132: Verify exposure scope ofabacAttributesin generic room publicationsAdding
abacAttributes: 1toroomFieldsmeans every publication that usesroomFieldswill now send ABAC policy data down to subscribed clients, not just the ABAC admin endpoints (which already expose these via/v1/abac/rooms). Please double‑check:
- Which publications consume
roomFieldsand which user roles can subscribe to them.- That it’s acceptable for all of those clients to see the full ABAC attribute configuration for a room (no sensitive policy or internal data leakage).
If this should be admin‑only, consider a separate field set or a conditional publication for ABAC data instead of adding it to the shared
roomFieldsprojection. Based on learnings, ABAC room data has so far been surfaced via a dedicated endpoint.packages/i18n/src/locales/en.i18n.json (6)
417-418: New generic UI labels look consistent
Add_ValueandAdd_roomlabels are clear, casing matches surrounding strings, and they reuse existing “Add …” patterns appropriately.
767-768: Attribute-related labels are coherent with nearby keys
Attribute_ValuesandAttributesfit well withAttribute_handling; wording and capitalization are consistent and generic enough to be reused across ABAC and non‑ABAC UIs.
2841-2846: LDAP ABAC mapping descriptions are clear and technically preciseThe LDAP→ABAC mapping help text is accurate, the JSON example is valid, and placeholder usage is correct. This should be understandable to admins configuring sync.
5530-5530:UpdatedlabelThe generic
Updatedstring is fine and reusable across logs / status messages; no issues spotted.
5720-5723:Value/ValueslabelsSingular/plural variants are correct and align with other generic labels in this file; good for use in tables and forms.
5768-5768:View_roomslabel“View rooms” is consistent with adjacent
View_channels/View fileslabels and reads naturally as a permission description.apps/meteor/client/views/admin/ABAC/ABACAttributesTab/AttributesForm.spec.tsx (1)
1-258: LGTM!The test suite has been properly updated to reflect the component refactoring from
AdminABACRoomAttributesFormtoAttributesForm. All expectations align with the new component API, including updated button labels and form data types.apps/meteor/client/views/admin/ABAC/ABACAttributesTab/AttributesPage.tsx (1)
42-49: Backend correctly handles both key and values search parameters with OR logic.The
/v1/abac/attributesendpoint supports simultaneouskeyandvaluesparameters. When both are provided, the backend applies OR matching: attributes are returned where either the key matches OR the values match the search term. This behavior is intentional and verified by test cases. The client implementation is correct.apps/meteor/tests/end-to-end/api/abac.ts (3)
29-63: Well-structured test setup with proper isolation.The test suite properly:
- Uses unique identifiers with
Date.now()to avoid conflicts- Connects to MongoDB for direct manipulation when needed
- Cleans up resources in
afterhooks- Configures permissions and settings before tests
1629-1758: Excellent coverage of ABAC access control scenarios.The "Room access (after subscribed)" tests thoroughly cover:
- Initial compliant access
- Cache behavior after attribute loss
- Access revocation after cache expiry
- User removal verification
- Re-invitation flow
- Attribute removal scenarios
This provides strong confidence in the ABAC access control lifecycle.
2101-2125: The LDAP settings are already reset properly in theafterhook.The test suite has an
afterhook at line 2407 that resets all LDAP settings configured in thebeforehook. Mocha guarantees thatafterhooks execute after all tests complete, even if tests fail. The try/finally pattern is unnecessary here.Likely an incorrect or invalid review comment.
ee/packages/abac/src/user-auto-removal.spec.ts (3)
10-19: Static analysis errors are false positives.The Biome parse errors on lines 10-19 are false positives. The
jest.mockfactory function with an anonymous class (ServiceClass: class {}) is valid Jest syntax and works correctly at runtime. This pattern is commonly used to mock ES6 class exports.
83-100: Test infrastructure properly manages database lifecycle.The
beforeAllhook correctly:
- Creates an in-memory MongoDB instance
- Registers service models
- Sets up spies for debugging and auditing
- Initializes collections for rooms and users
The 30-second timeout is appropriate for MongoDB startup.
435-476: Good performance sanity test with realistic scale.Testing with 300 users (150 compliant, 150 non-compliant) provides confidence that the ABAC removal logic handles larger populations correctly. The assertions verify:
- Correct number of removals (150)
- Specific user IDs removed/retained
- All removals audited to correct room
- Remaining membership count
apps/meteor/client/views/admin/ABAC/ABACRoomsTab/RoomsPage.tsx (2)
105-119: Table rendering follows established patterns.The table correctly:
- Uses optional chaining on
abacAttributes(safe even though endpoint guarantees non-empty array per learnings)- Displays room name with fallback (
fname || name)- Includes proper
keyprop on rows- Delegates row actions to
RoomMenucomponentBased on learnings, the
/v1/abac/roomsendpoint guaranteesabacAttributesexists and is non-empty.
22-41: Component structure and routing logic is clean.The component properly:
- Uses
useEffectEventfor stable callback reference- Leverages
useDebouncedValueto reduce API calls- Integrates with the router for navigation
- Gates the "Add room" button on ABAC availability
ee/packages/abac/src/service.spec.ts (3)
25-58: Well-structured mock setup for model layer.The mock setup comprehensively covers all model methods used by AbacService. The pattern of defining mock functions at module scope and wiring them in jest.mock allows for flexible per-test configuration.
84-249: Thorough coverage ofaddSubjectAttributesbehavior.The test suite covers:
- Value merging from multiple LDAP keys
- Deduplication across arrays
- Unsetting attributes when LDAP values are empty
- Hook invocations on attribute changes (gain vs loss)
- Mixed array and string LDAP values
- Invalid/empty string filtering
This provides strong confidence in the LDAP-to-ABAC attribute mapping logic.
1033-1095: Good edge case coverage forcheckUsernamesMatchAttributes.Tests properly verify:
- Early return when usernames array is empty
- Early return when attributes array is empty
- Query structure for non-compliant user detection
- Error details contain non-compliant usernames
packages/core-services/src/types/IAbacService.ts (1)
1-49: LGTM!The interface is well-structured with clear method signatures covering ABAC attribute lifecycle, room attribute management, and access control operations. The
AbacActortype appropriately picks minimal user fields needed for audit trails.ee/packages/abac/src/index.ts (4)
42-53: Well-designed service setup with appropriate concurrency control.Good use of
pLimit(20)to prevent overwhelming the server during bulk user removals.
168-171: Correct restriction to private rooms.The base query correctly filters for
t: 'p'and rooms with non-emptyabacAttributes, aligning with the ABAC design that attributes can only be set on private rooms. Based on learnings, this is the expected behavior.
611-625: Solid error handling for user removal.Good use of
skipAppPreEventsfor automated ABAC removals and proper error logging when removal fails.
354-358: Verify if attribute value order is significant.The comparison checks array equality by position. If the same values in different order should be considered equal, this early return would fail to detect actual no-op updates.
If order is not significant, consider using a Set-based comparison:
const prevValues = previous[existingIndex].values; -if (prevValues.length === values.length && prevValues.every((v, i) => v === values[i])) { +const prevSet = new Set(prevValues); +if (prevValues.length === values.length && values.every((v) => prevSet.has(v))) { return; }apps/meteor/client/views/admin/ABAC/ABACSettingTab/SettingField.tsx (2)
110-114: HTML callout is sanitized correctly; static warning is effectively a false positiveThe
alertcontent is passed throughDOMPurify.sanitizebefore being injected viadangerouslySetInnerHTML, which addresses the XSS concern raised by the linter/static analysis. Given this sanitization and that the source is a setting-defined string, this pattern is acceptable unless the project enforces a strict “nodangerouslySetInnerHTMLat all” policy, in which case you’d need a higher-level wrapper (e.g., a shared “SafeHtml” component) to satisfy the rule.
116-124: Enterprise gating and reset logic align with ABAC cache decision semanticsThe combination of
shouldDisableEnterpriseandhasResetButtonensures that:
- Enterprise-only settings are disabled and cannot be reset when the module isn’t available.
- The reset button is keyed off equality with
packageValue/packageEditor, so values like0(used asinvalidValueforAbac_Cache_Decision_Time_Seconds) are treated as legitimate when the module is present and only reset when they differ from the package default.This matches the documented ABAC behavior where
0is a valid admin-selected value while also being used as a fallback when unlicensed. Based on learnings, this looks correct.
apps/meteor/client/views/admin/ABAC/ABACAttributesTab/AttributesContextualBar.tsx
Show resolved
Hide resolved
| const updateAction = useEffectEvent(async (action: () => void) => { | ||
| setModal( | ||
| <GenericModal | ||
| variant='info' | ||
| icon={null} | ||
| title={t('ABAC_Update_room_confirmation_modal_title')} | ||
| annotation={t('ABAC_Update_room_confirmation_modal_annotation')} | ||
| confirmText={t('Save_changes')} | ||
| onConfirm={() => { | ||
| action(); | ||
| setModal(null); | ||
| }} | ||
| onCancel={() => setModal(null)} | ||
| > | ||
| <Trans | ||
| i18nKey='ABAC_Update_room_content' | ||
| values={{ roomName: roomInfo?.name }} | ||
| components={{ bold: <Box is='span' fontWeight='bold' /> }} | ||
| /> | ||
| </GenericModal>, | ||
| ); | ||
| }); |
There was a problem hiding this comment.
Modal closes before save completes, hiding potential errors.
The action parameter is typed as () => void but receives handleSubmit(onSave) (line 69), which returns Promise<void>. On line 53, action() is called without await, and the modal closes immediately via setModal(null) on line 54. If the save operation fails or takes time, the user won't see errors or feedback because the modal has already closed.
Apply this diff to await the action and fix the type signature:
-const updateAction = useEffectEvent(async (action: () => void) => {
+const updateAction = useEffectEvent(async (action: () => Promise<void>) => {
setModal(
<GenericModal
variant='info'
icon={null}
title={t('ABAC_Update_room_confirmation_modal_title')}
annotation={t('ABAC_Update_room_confirmation_modal_annotation')}
confirmText={t('Save_changes')}
- onConfirm={() => {
- action();
- setModal(null);
- }}
+ onConfirm={async () => {
+ await action();
+ setModal(null);
+ }}
onCancel={() => setModal(null)}
>
<Trans
i18nKey='ABAC_Update_room_content'
values={{ roomName: roomInfo?.name }}
components={{ bold: <Box is='span' fontWeight='bold' /> }}
/>
</GenericModal>,
);
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const updateAction = useEffectEvent(async (action: () => void) => { | |
| setModal( | |
| <GenericModal | |
| variant='info' | |
| icon={null} | |
| title={t('ABAC_Update_room_confirmation_modal_title')} | |
| annotation={t('ABAC_Update_room_confirmation_modal_annotation')} | |
| confirmText={t('Save_changes')} | |
| onConfirm={() => { | |
| action(); | |
| setModal(null); | |
| }} | |
| onCancel={() => setModal(null)} | |
| > | |
| <Trans | |
| i18nKey='ABAC_Update_room_content' | |
| values={{ roomName: roomInfo?.name }} | |
| components={{ bold: <Box is='span' fontWeight='bold' /> }} | |
| /> | |
| </GenericModal>, | |
| ); | |
| }); | |
| const updateAction = useEffectEvent(async (action: () => Promise<void>) => { | |
| setModal( | |
| <GenericModal | |
| variant='info' | |
| icon={null} | |
| title={t('ABAC_Update_room_confirmation_modal_title')} | |
| annotation={t('ABAC_Update_room_confirmation_modal_annotation')} | |
| confirmText={t('Save_changes')} | |
| onConfirm={async () => { | |
| await action(); | |
| setModal(null); | |
| }} | |
| onCancel={() => setModal(null)} | |
| > | |
| <Trans | |
| i18nKey='ABAC_Update_room_content' | |
| values={{ roomName: roomInfo?.name }} | |
| components={{ bold: <Box is='span' fontWeight='bold' /> }} | |
| /> | |
| </GenericModal>, | |
| ); | |
| }); |
🤖 Prompt for AI Agents
In apps/meteor/client/views/admin/ABAC/ABACRoomsTab/RoomForm.tsx around lines 44
to 65, the updateAction currently types the parameter as () => void and calls
action() without awaiting it, closing the modal immediately; change the
parameter type to () => Promise<void>, call await action() inside the onConfirm
handler, and only call setModal(null) after the awaited action completes
(optionally wrap in try/catch if you want to keep the modal open on errors and
surface them).
| const handleSave = useEffectEvent(() => { | ||
| if (roomInfo) { | ||
| updateAction(handleSubmit(onSave)); | ||
| } else { | ||
| handleSubmit(onSave)(); | ||
| } | ||
| }); | ||
|
|
||
| return ( | ||
| <> | ||
| <ContextualbarScrollableContent> | ||
| <Box is='form' onSubmit={handleSubmit(handleSave)} id={formId}> |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Remove double validation by accepting form data in handleSave.
The form's onSubmit handler (line 78) wraps handleSave with handleSubmit, which validates the form and passes data to handleSave. However, handleSave is defined without parameters (line 67) and internally calls handleSubmit(onSave) again (lines 69, 71), causing the form to be validated twice. This is inefficient and creates confusing logic flow.
Apply this diff to refactor handleSave to accept the validated form data directly:
-const handleSave = useEffectEvent(() => {
+const handleSave = useEffectEvent((data: RoomFormData) => {
if (roomInfo) {
- updateAction(handleSubmit(onSave));
+ updateAction(() => onSave(data));
} else {
- handleSubmit(onSave)();
+ onSave(data);
}
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const handleSave = useEffectEvent(() => { | |
| if (roomInfo) { | |
| updateAction(handleSubmit(onSave)); | |
| } else { | |
| handleSubmit(onSave)(); | |
| } | |
| }); | |
| return ( | |
| <> | |
| <ContextualbarScrollableContent> | |
| <Box is='form' onSubmit={handleSubmit(handleSave)} id={formId}> | |
| const handleSave = useEffectEvent((data: RoomFormData) => { | |
| if (roomInfo) { | |
| updateAction(() => onSave(data)); | |
| } else { | |
| onSave(data); | |
| } | |
| }); | |
| return ( | |
| <> | |
| <ContextualbarScrollableContent> | |
| <Box is='form' onSubmit={handleSubmit(handleSave)} id={formId}> |
🤖 Prompt for AI Agents
In apps/meteor/client/views/admin/ABAC/ABACRoomsTab/RoomForm.tsx around lines 67
to 78, handleSave is defined without parameters but calls handleSubmit(onSave)
internally while the form onSubmit already wraps handleSave with handleSubmit,
causing duplicate validation; refactor handleSave to accept the validated form
data parameter (e.g., data) and call onSave(data) (or
updateAction(onSave.bind(null, data)) when roomInfo is present) instead of
calling handleSubmit again, and update any updateAction invocation to use the
data directly so the form is validated only once.
| const update = useDebouncedCallback( | ||
| ({ value, editor }: { value?: SettingValue; editor?: SettingEditor }) => { | ||
| if (!persistedSetting) { | ||
| return; | ||
| } | ||
|
|
||
| dispatch([ | ||
| { | ||
| _id: persistedSetting._id, | ||
| ...(value !== undefined && { value }), | ||
| ...(editor !== undefined && { editor }), | ||
| changed: | ||
| JSON.stringify(persistedSetting.value) !== JSON.stringify(value) || | ||
| (isSettingColor(persistedSetting) && JSON.stringify(persistedSetting.editor) !== JSON.stringify(editor)), | ||
| }, | ||
| ]); | ||
| }, | ||
| 230, | ||
| [persistedSetting, dispatch], | ||
| ); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let me locate and read the SettingField.tsx file
fd -type f -name "SettingField.tsx"Repository: RocketChat/Rocket.Chat
Length of output: 236
🏁 Script executed:
# Read the file to see the full context around lines 37-56
cat -n apps/meteor/client/views/admin/ABAC/ABACSettingTab/SettingField.tsx | head -100Repository: RocketChat/Rocket.Chat
Length of output: 4159
🏁 Script executed:
# Search for where the update callback is used to understand call patterns
rg "update\(" apps/meteor/client/views/admin/ABAC/ABACSettingTab/SettingField.tsx -A 2 -B 2Repository: RocketChat/Rocket.Chat
Length of output: 486
🏁 Script executed:
# Check the isSettingColor function to understand the color setting logic
rg "isSettingColor" apps/meteor/client/views/admin/ABAC/ABACSettingTab/SettingField.tsx -B 2 -A 2Repository: RocketChat/Rocket.Chat
Length of output: 1501
🏁 Script executed:
# Look for the persistedSetting structure and how it's used
rg "persistedSetting" apps/meteor/client/views/admin/ABAC/ABACSettingTab/SettingField.tsx -B 1 -A 1 | head -50Repository: RocketChat/Rocket.Chat
Length of output: 1246
Fix changed flag when only value or editor is omitted
When update({ value }) or update({ editor }) is called with only one parameter, the other parameter is undefined. The changed flag comparison currently uses these undefined values directly, causing it to incorrectly mark the setting as changed even when nothing changed. For example, calling update({ editor }) on a color setting compares the persisted value against undefined, always returning true.
Compute the next state by falling back to persistedSetting.value / persistedSetting.editor when the corresponding argument is undefined, and compare against that:
- const update = useDebouncedCallback(
- ({ value, editor }: { value?: SettingValue; editor?: SettingEditor }) => {
- if (!persistedSetting) {
- return;
- }
-
- dispatch([
- {
- _id: persistedSetting._id,
- ...(value !== undefined && { value }),
- ...(editor !== undefined && { editor }),
- changed:
- JSON.stringify(persistedSetting.value) !== JSON.stringify(value) ||
- (isSettingColor(persistedSetting) && JSON.stringify(persistedSetting.editor) !== JSON.stringify(editor)),
- },
- ]);
- },
- 230,
- [persistedSetting, dispatch],
- );
+ const update = useDebouncedCallback(
+ ({ value, editor }: { value?: SettingValue; editor?: SettingEditor }) => {
+ if (!persistedSetting) {
+ return;
+ }
+
+ const nextValue = value !== undefined ? value : persistedSetting.value;
+ const nextEditor = isSettingColor(persistedSetting)
+ ? editor !== undefined
+ ? editor
+ : persistedSetting.editor
+ : undefined;
+
+ dispatch([
+ {
+ _id: persistedSetting._id,
+ ...(value !== undefined && { value }),
+ ...(editor !== undefined && { editor }),
+ changed:
+ JSON.stringify(persistedSetting.value) !== JSON.stringify(nextValue) ||
+ (isSettingColor(persistedSetting) &&
+ JSON.stringify(persistedSetting.editor) !== JSON.stringify(nextEditor)),
+ },
+ ]);
+ },
+ 230,
+ [persistedSetting, dispatch],
+ );📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const update = useDebouncedCallback( | |
| ({ value, editor }: { value?: SettingValue; editor?: SettingEditor }) => { | |
| if (!persistedSetting) { | |
| return; | |
| } | |
| dispatch([ | |
| { | |
| _id: persistedSetting._id, | |
| ...(value !== undefined && { value }), | |
| ...(editor !== undefined && { editor }), | |
| changed: | |
| JSON.stringify(persistedSetting.value) !== JSON.stringify(value) || | |
| (isSettingColor(persistedSetting) && JSON.stringify(persistedSetting.editor) !== JSON.stringify(editor)), | |
| }, | |
| ]); | |
| }, | |
| 230, | |
| [persistedSetting, dispatch], | |
| ); | |
| const update = useDebouncedCallback( | |
| ({ value, editor }: { value?: SettingValue; editor?: SettingEditor }) => { | |
| if (!persistedSetting) { | |
| return; | |
| } | |
| const nextValue = value !== undefined ? value : persistedSetting.value; | |
| const nextEditor = isSettingColor(persistedSetting) | |
| ? editor !== undefined | |
| ? editor | |
| : persistedSetting.editor | |
| : undefined; | |
| dispatch([ | |
| { | |
| _id: persistedSetting._id, | |
| ...(value !== undefined && { value }), | |
| ...(editor !== undefined && { editor }), | |
| changed: | |
| JSON.stringify(persistedSetting.value) !== JSON.stringify(nextValue) || | |
| (isSettingColor(persistedSetting) && | |
| JSON.stringify(persistedSetting.editor) !== JSON.stringify(nextEditor)), | |
| }, | |
| ]); | |
| }, | |
| 230, | |
| [persistedSetting, dispatch], | |
| ); |
🤖 Prompt for AI Agents
In apps/meteor/client/views/admin/ABAC/ABACSettingTab/SettingField.tsx around
lines 37 to 56, the computed `changed` flag uses incoming `value` and `editor`
directly, so when one is omitted (undefined) the comparison against
persistedSetting always reports a change; fix by computing the nextValue = value
=== undefined ? persistedSetting.value : value and nextEditor = editor ===
undefined ? persistedSetting.editor : editor, then use
JSON.stringify(persistedSetting.value) !== JSON.stringify(nextValue) ||
(isSettingColor(persistedSetting) && JSON.stringify(persistedSetting.editor) !==
JSON.stringify(nextEditor)) to set `changed`.
MartinSchoeler
force-pushed
the
feat/abac
branch
from
678a248 to
0f3d776
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (7)
apps/meteor/app/lib/server/functions/addUserToRoom.ts (1)
65-68: Non-null assertion onusernameremains unaddressed.The
userToBeAdded.username!assertion on line 67 is risky sinceIUser.usernameis optional. While the user existence is validated at line 47, some users (app users, service accounts) may lack a username. Consider adding explicit validation before calling the patch:try { const inviterUser = inviter && ((await Users.findOneById(inviter._id)) || undefined); - // Not "duplicated": we're moving away from callbacks so this is a patch function. We should migrate the next one to be a patch or use this same patch, instead of calling both - await beforeAddUserToRoomPatch([userToBeAdded.username!], room, inviterUser); + // Not "duplicated": we're moving away from callbacks so this is a patch function. We should migrate the next one to be a patch or use this same patch, instead of calling both + if (userToBeAdded.username) { + await beforeAddUserToRoomPatch([userToBeAdded.username], room, inviterUser); + } await beforeAddUserToRoom.run({ user: userToBeAdded, inviter: inviterUser }, room);Alternatively, if the patch function must always run, throw a descriptive error when username is missing.
apps/meteor/app/slashcommands-invite/server/server.ts (1)
113-141: Missing early returns cause potential double-execution and silent error swallowing.The two
ifblocks (lines 114 and 126) can both execute for the same error if it's aMeteorErrorthat also matches theisStringErrorshape. Additionally, errors matching neither condition are silently swallowed.} catch (e: unknown) { if (isMeteorError(e)) { if (e.error === 'error-only-compliant-users-can-be-added-to-abac-rooms') { void api.broadcast('notify.ephemeralMessage', userId, message.rid, { msg: i18n.t(e.error, { lng: settings.get('Language') || 'en' }), }); } else { void api.broadcast('notify.ephemeralMessage', userId, message.rid, { msg: i18n.t(e.message, { lng: settings.get('Language') || 'en' }), }); } + return; } if (isStringError(e)) { const { error } = e; if (error === 'error-federated-users-in-non-federated-rooms') { void api.broadcast('notify.ephemeralMessage', userId, message.rid, { msg: i18n.t('You_cannot_add_external_users_to_non_federated_room', { lng: settings.get('Language') || 'en' }), }); } else if (error === 'cant-invite-for-direct-room') { void api.broadcast('notify.ephemeralMessage', userId, message.rid, { msg: i18n.t('Cannot_invite_users_to_direct_rooms', { lng: settings.get('Language') || 'en' }), }); } else { void api.broadcast('notify.ephemeralMessage', userId, message.rid, { msg: i18n.t(error, { lng: settings.get('Language') || 'en' }), }); } + return; } + + console.error('Unexpected error in /invite command:', e); }apps/meteor/client/views/admin/ABAC/ABACSettingTab/SettingToggle.spec.tsx (1)
1-9: Import issue has been resolved.The
ISettingtype is now correctly imported from@rocket.chat/core-typingsinstead of the Apps SDK package. The test setup withmockAppRoot,EditableSettingsProvider, and translation mocks is well-structured.apps/meteor/client/views/admin/ABAC/ABACSettingTab/SettingField.tsx (1)
37-53: Debounced update logic is simplified.The previous issue with the
changedflag has been addressed by removing it from the dispatch payload. The callback now only sends the updated fields (valueand/oreditor), letting the settings context handle change detection.apps/meteor/client/views/admin/ABAC/AdminABACPage.tsx (1)
79-97: Empty contextual bar rendered when ABAC is unavailable.When
tabandcontextare defined butisABACAvailableis false, theContextualbarDialogrenders with empty content. Guard the entire dialog with ABAC availability to prevent an empty sidebar.- {tab !== undefined && context !== undefined && ( + {tab !== undefined && context !== undefined && isABACAvailable && ( <ContextualbarDialog onClose={() => handleCloseContextualbar()}>apps/meteor/client/views/admin/ABAC/ABACRoomsTab/RoomsContextualBar.tsx (2)
12-18: UnusedattributeIdprop in type definition.
attributeIdis defined inRoomsContextualBarPropsbut not destructured or used in the component. The component readsattributeIdfromuseRouteParameter('id')instead (line 39). Remove the unused prop.type RoomsContextualBarProps = { - attributeId?: string; roomInfo?: { rid: string; name: string }; attributesData?: { key: string; values: string[] }[]; onClose: () => void; };
63-65: Error object passed to toast message instead of string.
dispatchToastMessageexpectsmessageto be a string, buterroris anErrorobject. This will display[object Object]or cause runtime issues. The past review marked this as addressed, but the code still shows the original issue.onError: (error) => { - dispatchToastMessage({ type: 'error', message: error }); + dispatchToastMessage({ type: 'error', message: error instanceof Error ? error.message : String(error) }); },
🧹 Nitpick comments (23)
apps/meteor/client/components/UserInfo/UserInfo.stories.tsx (1)
45-54: LGTM! Story data correctly refactored to match updated component signature.The nested ABAC attribute structure with
keyandvaluesproperties is correct and aligns with the UserInfo component's updated expectations.Minor optional suggestion: Consider using different values for each attribute to make the story more illustrative. For example:
abacAttributes: [ { key: 'Classified', - values: ['Top Secret', 'Confidential'], + values: ['Top Secret'], }, { key: 'Security_Clearance', - values: ['Top Secret', 'Confidential'], + values: ['Level 3', 'Level 4'], }, ],apps/meteor/client/views/admin/ABAC/ABACRoomsTab/DeleteRoomModal.spec.tsx (2)
15-23: Consider test isolation: shared baseAppRoot is mutated.The
baseAppRootis mutated in tests with.withEndpoint(). While Testing Library tests run sequentially by default, it's better practice to create the wrapper within each test for clearer isolation.Consider refactoring to create the wrapper per test:
-const baseAppRoot = mockAppRoot().withTranslations('en', 'core', { +const createAppRoot = () => mockAppRoot().withTranslations('en', 'core', { Edit: 'Edit', Remove: 'Remove', ABAC_Room_removed: 'Room {{roomName}} removed from ABAC management', ABAC_Delete_room: 'Remove room from ABAC management', ABAC_Delete_room_annotation: 'Proceed with caution', ABAC_Delete_room_content: 'Removing <bold>{{roomName}}</bold> from ABAC management may result in unintended users gaining access.', Cancel: 'Cancel', });Then use
createAppRoot()in each test instead ofbaseAppRoot.
33-70: Expand test coverage with error and interaction cases.The current tests cover the happy path well but are missing several important scenarios:
- Error handling when the DELETE request fails
- Verification that
onCloseis called after successful deletion- Cancel button behavior (translation is provided but not tested)
Consider adding these test cases:
it('should show error toast when delete fails', async () => { const deleteEndpointMock = jest.fn().mockRejectedValue(new Error('Delete failed')); render(<DeleteRoomModal rid={rid} roomName={roomName} onClose={jest.fn()} />, { wrapper: createAppRoot().withEndpoint('DELETE', '/v1/abac/rooms/:rid/attributes', deleteEndpointMock).build(), }); await userEvent.click(screen.getByRole('button', { name: 'Remove' })); await waitFor(() => { expect(mockDispatchToastMessage).toHaveBeenCalledWith({ type: 'error', message: expect.any(String), }); }); }); it('should call onClose after successful deletion', async () => { const onClose = jest.fn(); const deleteEndpointMock = jest.fn().mockResolvedValue(null); render(<DeleteRoomModal rid={rid} roomName={roomName} onClose={onClose} />, { wrapper: createAppRoot().withEndpoint('DELETE', '/v1/abac/rooms/:rid/attributes', deleteEndpointMock).build(), }); await userEvent.click(screen.getByRole('button', { name: 'Remove' })); await waitFor(() => { expect(onClose).toHaveBeenCalled(); }); }); it('should call onClose when cancel is clicked', async () => { const onClose = jest.fn(); render(<DeleteRoomModal rid={rid} roomName={roomName} onClose={onClose} />, { wrapper: createAppRoot().build(), }); await userEvent.click(screen.getByRole('button', { name: 'Cancel' })); expect(onClose).toHaveBeenCalled(); });apps/meteor/client/views/admin/ABAC/ABACRoomsTab/RoomFormAttributeField.spec.tsx (1)
9-12: Remove unnecessaryasynckeyword.The test function is declared as
asyncbut performs no asynchronous operations—render()is synchronous and there are noawaitcalls.Apply this diff to simplify the test:
- test.each(testCases)(`renders %s without crashing`, async (_storyname, Story) => { + test.each(testCases)(`renders %s without crashing`, (_storyname, Story) => { const { baseElement } = render(<Story />); expect(baseElement).toMatchSnapshot(); });apps/meteor/client/views/admin/ABAC/ABACRoomsTab/RoomFormAttributeFields.spec.tsx (1)
9-38: Jest mock hoisting may cause undefined references.
jest.mockcalls are hoisted above imports by Jest. The mock factory referencesmockAttribute1,mockAttribute2, andmockAttribute3which are defined after the mock in source order. While this often works due to lazy factory evaluation, it's fragile and can fail unpredictably.Consider moving mock data inside the factory or using inline values:
+const mockAttributes = [ + { _id: 'attr1', key: 'Department', values: ['Engineering', 'Sales', 'Marketing'] }, + { _id: 'attr2', key: 'Security-Level', values: ['Public', 'Internal', 'Confidential'] }, + { _id: 'attr3', key: 'Location', values: ['US', 'EU', 'APAC'] }, +]; + jest.mock('../hooks/useAttributeList', () => ({ useAttributeList: jest.fn(() => ({ data: { - attributes: [ - { value: mockAttribute1.key, label: mockAttribute1.key, attributeValues: mockAttribute1.values }, - { value: mockAttribute2.key, label: mockAttribute2.key, attributeValues: mockAttribute2.values }, - { value: mockAttribute3.key, label: mockAttribute3.key, attributeValues: mockAttribute3.values }, - ], + attributes: mockAttributes.map((attr) => ({ + value: attr.key, + label: attr.key, + attributeValues: attr.values, + })), }, isLoading: false, })), }));apps/meteor/client/views/admin/ABAC/hooks/useAttributeOptions.tsx (1)
59-60: TODO: Route to rooms tab implementation pending.There's a TODO comment about routing to the rooms tab when an attribute is in use.
Would you like me to generate the navigation implementation once the rooms tab route is available, or open an issue to track this task?
apps/meteor/client/views/admin/ABAC/hooks/useAttributeList.ts (1)
20-27: Consider batching concurrent pagination requests for large attribute sets.The pagination logic fires all remaining page requests simultaneously via
Promise.all, which could create many concurrent requests if the total attribute count grows significantly. For typical ABAC usage this is acceptable, but if attribute counts scale large, implementing request batching would prevent potential server overload or rate-limit issues.apps/meteor/client/views/admin/ABAC/ABACSettingTab/SettingToggle.stories.tsx (1)
40-52: Remove redundant args in Default story.The
argsat the meta level (lines 40-42) are automatically inherited by all stories. TheDefaultstory duplicates them unnecessarily.export const Default: Story = { - args: { - hasABAC: true, - }, };apps/meteor/client/views/admin/ABAC/ABACSettingTab/SettingField.spec.tsx (1)
31-39: ResetdispatchMockbetween tests.The
dispatchMockis not cleared between test runs. While there's currently only one test, this will cause assertion failures or false positives when additional tests are added.describe('SettingField', () => { beforeEach(() => { + dispatchMock.mockClear(); jest.useFakeTimers(); });apps/meteor/app/invites/server/functions/validateInviteToken.ts (1)
30-35: Consider a more specific error code for ABAC-managed rooms.The error code
error-invalid-roomis generic and may confuse users or make debugging harder. A distinct error code likeerror-room-abac-managedwould provide clearer feedback and allow for specific i18n translations.if (settings.get('ABAC_Enabled') && room?.abacAttributes?.length) { - throw new Meteor.Error('error-invalid-room', 'Room is ABAC managed', { + throw new Meteor.Error('error-room-abac-managed', 'Room is ABAC managed', { method: 'validateInviteToken', field: 'rid', }); }apps/meteor/client/views/admin/ABAC/ABACSettingTab/AbacEnabledToggle.tsx (1)
55-70: Consider skipping the warning modal whenpackageValueis true.The
onResethandler always shows the warning modal, but the warning is specifically about disabling ABAC. IfpackageValuewere evertrue(enabling ABAC on reset), showing the "disable" warning would be misleading. Currently this isn't an issue sincepackageValuedefaults tofalse, but for robustness:const onReset = useCallback(() => { if (!setting) { return; } const value = setting.packageValue as boolean; + if (value === true) { + // Resetting to enabled state doesn't need a warning + setValue(value); + dispatch([{ _id: setting._id, value }]); + return; + } setModal( <WarningModalapps/meteor/client/views/admin/ABAC/ABACSettingTab/SettingField.tsx (1)
26-33: Throwing errors during render may cause unexpected crashes.Throwing errors in the render path without an error boundary will crash the component tree. Consider returning a fallback UI or null instead:
- if (!setting || !persistedSetting) { - throw new Error(`Setting ${settingId} not found`); - } - - // Checks if setting has at least required fields before doing anything - if (!isSetting(setting)) { - throw new Error(`Setting ${settingId} is not valid`); - } + if (!setting || !persistedSetting) { + return null; + } + + // Checks if setting has at least required fields before doing anything + if (!isSetting(setting)) { + console.error(`Setting ${settingId} is not valid`); + return null; + }Alternatively, wrap the consuming components with an error boundary to gracefully handle these cases.
apps/meteor/client/views/admin/ABAC/ABACRoomsTab/RoomFormAutocompleteDummy.tsx (1)
3-5: Consider removing unusedridfrom props type.The
ridproperty is defined inroomInfobut never used within this component. If it's only needed for type consistency with other components, this is fine; otherwise, simplify the type.type RoomFormAutocompleteDummyProps = { - roomInfo: { rid: string; name: string }; + roomInfo: { name: string }; };apps/meteor/client/views/admin/ABAC/ABACRoomsTab/RoomsContextualBarWithData.tsx (1)
21-23: UsingisFetchingmay cause unnecessary skeleton flicker during background refetches.When data is already loaded and a background refetch occurs, showing the skeleton again can cause poor UX. Consider using only
isLoading(which is true only when there's no cached data) orisPendingfor initial loads.- if (isLoading || isFetching) { + if (isLoading) { return <ContextualbarSkeletonBody />; }apps/meteor/client/views/admin/ABAC/AdminABACRoute.tsx (2)
17-18: Address or remove the TODO comment.Per coding guidelines, avoid code comments in implementation. This TODO should either be resolved or tracked in an issue.
Would you like me to open an issue to track this permission verification task?
45-45: Unusedtin dependency array.The
tfunction fromuseTranslationis included in the dependency array but is not used within the effect body.- }, [shouldShowUpsell, setModal, t, handleManageSubscription, ABACEnabledSetting]); + }, [shouldShowUpsell, setModal, handleManageSubscription, ABACEnabledSetting]);apps/meteor/client/views/admin/ABAC/ABACLogsTab/LogsPage.tsx (1)
48-75: Consider memoizinggetActionLabelor moving it outside the component.This function is recreated on every render. Since it only depends on
t, consider memoizing it withuseCallbackor moving it outside if translations are stable.apps/meteor/client/lib/queryKeys.ts (1)
148-157: Inconsistent key structure betweenlogs.listand other list methods.
logs.listincludes a'list'segment ([...all(), 'list', query]) whileroomAttributes.listandrooms.listdo not ([...all(), query]). This inconsistency could cause cache key collisions if aqueryobject matches anattributeIdorroomId. Consider adding'list'for consistency.roomAttributes: { all: () => [...ABACQueryKeys.all, 'room-attributes'] as const, - list: (query?: PaginatedRequest) => [...ABACQueryKeys.roomAttributes.all(), query] as const, + list: (query?: PaginatedRequest) => [...ABACQueryKeys.roomAttributes.all(), 'list', query] as const, attribute: (attributeId: string) => [...ABACQueryKeys.roomAttributes.all(), attributeId] as const, }, rooms: { all: () => [...ABACQueryKeys.all, 'rooms'] as const, - list: (query?: PaginatedRequest) => [...ABACQueryKeys.rooms.all(), query] as const, + list: (query?: PaginatedRequest) => [...ABACQueryKeys.rooms.all(), 'list', query] as const, autocomplete: (query?: PaginatedRequest) => [...ABACQueryKeys.rooms.all(), 'autocomplete', query] as const, room: (roomId: string) => [...ABACQueryKeys.rooms.all(), roomId] as const, },apps/meteor/client/views/admin/ABAC/ABACRoomsTab/RoomFormAutocomplete.tsx (1)
25-55: Consider adding a default empty array for options.The
optionsprop receivesresult.datawhich can beundefinedduring loading or error states. WhileAutoCompletelikely handles this gracefully, explicitly providing a fallback improves clarity and safety.Apply this diff:
- options={result.data} + options={result.data ?? []}apps/meteor/client/views/admin/ABAC/ABACAttributesTab/AttributesPage.tsx (2)
51-54: Add error handling for the query.The component doesn't handle API errors. If the attributes fetch fails, users won't see any error message or indication of what went wrong.
Apply this diff to add error handling:
-const { data, isLoading } = useQuery({ +const { data, isLoading, error } = useQuery({ queryKey: ABACQueryKeys.roomAttributes.list(query), queryFn: () => getAttributes(query), });Then add error state rendering before the empty state check:
return ( <> <Margins block={24}> {/* ... search and button ... */} </Margins> + {error ? ( + <Box display='flex' justifyContent='center' height='full'> + <GenericNoResults + icon='warning' + title={t('Error')} + description={t('ABAC_Error_loading_attributes')} + /> + </Box> + ) : {(!data || data.attributes?.length === 0) && !isLoading ? (
71-108: Add TypeScript types and loading indicator.The render logic is functional but lacks type safety and a proper loading state:
Type safety: No explicit types for the API response shape. The code assumes
data.attributesexists and each attribute has_id,key, andvaluesproperties.Loading indicator: While data is loading, the UI might briefly flash the empty state or show nothing. A loading skeleton or spinner would improve UX.
Solution 1: Add TypeScript types
Define the API response type at the top of the file or in a shared types file:
type ABACAttribute = { _id: string; key: string; values: string[]; }; type ABACAttributesResponse = { attributes: ABACAttribute[]; total: number; };Then type the query:
-const { data, isLoading, error } = useQuery({ +const { data, isLoading, error } = useQuery<ABACAttributesResponse>({ queryKey: ABACQueryKeys.roomAttributes.list(query), queryFn: () => getAttributes(query), });Solution 2: Add loading indicator
+{isLoading ? ( + <Box display='flex' justifyContent='center' height='full'> + <GenericTable> + <GenericTableHeader> + <GenericTableHeaderCell>{t('Name')}</GenericTableHeaderCell> + <GenericTableHeaderCell>{t('Value')}</GenericTableHeaderCell> + <GenericTableHeaderCell key='spacer' w={40} /> + </GenericTableHeader> + <GenericTableBody> + {/* Show skeleton rows or loading state */} + </GenericTableBody> + </GenericTable> + </Box> +) : {(!data || data.attributes?.length === 0) && !isLoading ? (apps/meteor/client/views/admin/ABAC/ABACAttributesTab/AttributesForm.spec.tsx (1)
16-23: Consider adding missing translation keys for completeness.The component uses
ABAC_Remove_attributeandABAC_No_repeated_valuestranslation keys (from AttributesForm.tsx lines 51, 117, 135), but these are not included in the mock translations. While tests work because they search for the fallback key, adding these translations would improve test clarity and prevent confusion.Apply this diff to add the missing translations:
.withTranslations('en', 'core', { Name: 'Name', Values: 'Values', Add_Value: 'Add Value', Cancel: 'Cancel', Save: 'Save', Required_field: '{{field}} is required', + ABAC_Remove_attribute: 'Remove attribute', + ABAC_No_repeated_values: 'No repeated values allowed', })apps/meteor/client/views/admin/ABAC/ABACAttributesTab/AttributesForm.stories.tsx (1)
16-26: Add missing translation keys for better Storybook presentation.The translations provided in the decorator are missing
ABAC_Remove_attributeandABAC_No_repeated_values, which are used by the component (AttributesForm.tsx lines 51, 117, 135). In Storybook, this will cause buttons and error messages to display translation keys instead of user-friendly text.Apply this diff:
.withTranslations('en', 'core', { Name: 'Name', Values: 'Values', Add_Value: 'Add Value', Cancel: 'Cancel', Save: 'Save', Required_field: '{{field}} is required', + ABAC_Remove_attribute: 'Remove attribute', + ABAC_No_repeated_values: 'No repeated values allowed', })
| select: (data) => ({ | ||
| events: data.events.map((event) => { | ||
| const eventInfo = { | ||
| id: event._id, | ||
| user: event.actor?.type === 'user' ? event.actor.username : t('System'), | ||
| ...(event.actor?.type === 'user' && { userAvatar: <UserAvatar size='x28' userId={event.actor._id} /> }), | ||
| timestamp: new Date(event.ts), | ||
| element: t('ABAC_Room'), | ||
| action: getActionLabel(event.data?.find((item) => item.key === 'change')?.value), | ||
| room: undefined, | ||
| }; | ||
| switch (event.t) { | ||
| case 'abac.attribute.changed': | ||
| return { | ||
| ...eventInfo, | ||
| element: t('ABAC_Room_Attribute'), | ||
| name: event.data?.find((item) => item.key === 'attributeKey')?.value ?? '', | ||
| }; | ||
| case 'abac.action.performed': | ||
| return { | ||
| ...eventInfo, | ||
| name: event.data?.find((item) => item.key === 'subject')?.value?.username ?? '', | ||
| action: getActionLabel(event.data?.find((item) => item.key === 'action')?.value), | ||
| room: event.data?.find((item) => item.key === 'object')?.value?.name ?? '', | ||
| element: t('ABAC_room_membership'), | ||
| }; | ||
| case 'abac.object.attribute.changed': | ||
| case 'abac.object.attributes.removed': | ||
| return { | ||
| ...eventInfo, | ||
| name: | ||
| event.data | ||
| ?.find((item) => item.key === 'current') | ||
| ?.value?.map((item) => item.key) | ||
| .join(', ') ?? t('Empty'), | ||
| room: event.data?.find((item) => item.key === 'room')?.value?.name ?? '', | ||
| }; | ||
| default: | ||
| return null; | ||
| } | ||
| }), | ||
| count: data.count, | ||
| offset: data.offset, | ||
| total: data.total, | ||
| }), | ||
| }); |
There was a problem hiding this comment.
Avoid creating React elements inside select callback.
The select callback creates UserAvatar React elements (line 85) which get stored in React Query's cache. This is an anti-pattern because React elements should not be cached as data—they should be created during render. Store only the data needed and render the avatar in the table body.
select: (data) => ({
events: data.events.map((event) => {
const eventInfo = {
id: event._id,
user: event.actor?.type === 'user' ? event.actor.username : t('System'),
- ...(event.actor?.type === 'user' && { userAvatar: <UserAvatar size='x28' userId={event.actor._id} /> }),
+ ...(event.actor?.type === 'user' && { userId: event.actor._id }),
timestamp: new Date(event.ts),Then render the avatar in the table body:
{eventInfo.userId && (
<Box is='span' mie={4}>
<UserAvatar size='x28' userId={eventInfo.userId} />
</Box>
)}🤖 Prompt for AI Agents
In apps/meteor/client/views/admin/ABAC/ABACLogsTab/LogsPage.tsx around lines 80
to 125, the select mapper currently creates a React element (UserAvatar) and
places it into the React Query cache; remove that element from the mapped
eventInfo and instead store the primitive user id (e.g. userId:
event.actor?._id) or a boolean flag indicating a user exists. Update the
returned event objects to include only serializable data (user, userId,
timestamp, element, action, name, room, etc.) and not JSX; then render the
avatar during component render/table body using the stored userId to create
<UserAvatar> there. Ensure all switch cases propagate the userId field
consistently and adjust any consuming code to render the avatar from data rather
than reading a cached React element.
| <GenericTableCell>{room.fname || room.name}</GenericTableCell> | ||
| <GenericTableCell>{room.usersCount}</GenericTableCell> | ||
| <GenericTableCell withTruncatedText> | ||
| {room.abacAttributes?.flatMap((attribute) => attribute.key ?? []).join(', ')} |
There was a problem hiding this comment.
Fix incorrect use of flatMap for attribute keys.
Using flatMap with attribute.key ?? [] will cause incorrect behavior if attribute.key is a string. When a string is used with flatMap, it iterates over individual characters, resulting in comma-separated characters instead of attribute keys.
Apply this diff:
- {room.abacAttributes?.flatMap((attribute) => attribute.key ?? []).join(', ')}
+ {room.abacAttributes?.map((attribute) => attribute.key).join(', ')}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| {room.abacAttributes?.flatMap((attribute) => attribute.key ?? []).join(', ')} | |
| {room.abacAttributes?.map((attribute) => attribute.key).join(', ')} |
🤖 Prompt for AI Agents
In apps/meteor/client/views/admin/ABAC/ABACRoomsTab/RoomsPage.tsx around line
110, the code uses flatMap with attribute.key which is a string so flatMap will
iterate characters; replace flatMap with map and then filter out missing keys
before joining. Specifically, map each attribute to attribute.key (or
attribute.key ?? ''), filter(Boolean) to remove empty/undefined values, and then
join(', ') so you get comma-separated attribute keys rather than characters.
| const onResetButtonClick = useCallback(() => { | ||
| setValue(setting.value); | ||
| setEditor(isSettingColor(setting) ? setting.editor : undefined); | ||
| update({ | ||
| value: persistedSetting.packageValue, | ||
| ...(isSettingColor(persistedSetting) && { editor: persistedSetting.packageEditor }), | ||
| }); | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| }, [setting.value, (setting as ISettingColor).editor, update, persistedSetting]); |
There was a problem hiding this comment.
Bug: Local state set incorrectly on reset.
The reset handler sets local state to the current setting.value and setting.editor, but dispatches packageValue. This causes the UI to briefly show the current value before the setting update propagates back. The local state should be set to the package values:
const onResetButtonClick = useCallback(() => {
- setValue(setting.value);
- setEditor(isSettingColor(setting) ? setting.editor : undefined);
+ setValue(persistedSetting.packageValue);
+ setEditor(isSettingColor(persistedSetting) ? persistedSetting.packageEditor : undefined);
update({
value: persistedSetting.packageValue,
...(isSettingColor(persistedSetting) && { editor: persistedSetting.packageEditor }),
});
- // eslint-disable-next-line react-hooks/exhaustive-deps
-}, [setting.value, (setting as ISettingColor).editor, update, persistedSetting]);
+}, [update, persistedSetting]);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const onResetButtonClick = useCallback(() => { | |
| setValue(setting.value); | |
| setEditor(isSettingColor(setting) ? setting.editor : undefined); | |
| update({ | |
| value: persistedSetting.packageValue, | |
| ...(isSettingColor(persistedSetting) && { editor: persistedSetting.packageEditor }), | |
| }); | |
| // eslint-disable-next-line react-hooks/exhaustive-deps | |
| }, [setting.value, (setting as ISettingColor).editor, update, persistedSetting]); | |
| const onResetButtonClick = useCallback(() => { | |
| setValue(persistedSetting.packageValue); | |
| setEditor(isSettingColor(persistedSetting) ? persistedSetting.packageEditor : undefined); | |
| update({ | |
| value: persistedSetting.packageValue, | |
| ...(isSettingColor(persistedSetting) && { editor: persistedSetting.packageEditor }), | |
| }); | |
| }, [update, persistedSetting]); |
🤖 Prompt for AI Agents
In apps/meteor/client/views/admin/ABAC/ABACSettingTab/SettingField.tsx around
lines 85-93, the reset handler incorrectly sets local state from the current
setting instead of the persisted package values; change the local setters to use
persistedSetting.packageValue and, for color settings,
persistedSetting.packageEditor (i.e., call
setValue(persistedSetting.packageValue) and
setEditor(isSettingColor(persistedSetting) ? persistedSetting.packageEditor :
undefined)), keep the update call as-is, and update the useCallback dependency
array to include persistedSetting (or its packageValue/packageEditor) so the
hook is correct.
| useEffect(() => { | ||
| // WS has never activated ABAC | ||
| if (shouldShowUpsell && ABACEnabledSetting === undefined) { | ||
| setModal(<ABACUpsellModal onClose={() => setModal(null)} onConfirm={handleManageSubscription} />); | ||
| } | ||
| }, [shouldShowUpsell, setModal, t, handleManageSubscription, ABACEnabledSetting]); |
There was a problem hiding this comment.
Missing dependency cleanup for modal effect.
The effect sets a modal but doesn't clean it up when the component unmounts or when dependencies change. This could leave a stale modal open.
useEffect(() => {
// WS has never activated ABAC
if (shouldShowUpsell && ABACEnabledSetting === undefined) {
setModal(<ABACUpsellModal onClose={() => setModal(null)} onConfirm={handleManageSubscription} />);
}
+ return () => setModal(null);
}, [shouldShowUpsell, setModal, t, handleManageSubscription, ABACEnabledSetting]);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| useEffect(() => { | |
| // WS has never activated ABAC | |
| if (shouldShowUpsell && ABACEnabledSetting === undefined) { | |
| setModal(<ABACUpsellModal onClose={() => setModal(null)} onConfirm={handleManageSubscription} />); | |
| } | |
| }, [shouldShowUpsell, setModal, t, handleManageSubscription, ABACEnabledSetting]); | |
| useEffect(() => { | |
| // WS has never activated ABAC | |
| if (shouldShowUpsell && ABACEnabledSetting === undefined) { | |
| setModal(<ABACUpsellModal onClose={() => setModal(null)} onConfirm={handleManageSubscription} />); | |
| } | |
| return () => setModal(null); | |
| }, [shouldShowUpsell, setModal, t, handleManageSubscription, ABACEnabledSetting]); |
🤖 Prompt for AI Agents
In apps/meteor/client/views/admin/ABAC/AdminABACRoute.tsx around lines 40–45,
the useEffect sets a modal but provides no cleanup, risking a stale modal when
the component unmounts or dependencies change; add a cleanup function that calls
setModal(null) (so the modal is cleared on unmount or when the effect re-runs)
and keep the existing dependency array to ensure setModal is available in the
cleanup.
Co-authored-by: Tasso <tasso.evangelista@rocket.chat> Co-authored-by: Martin Schoeler <martin.schoeler@rocket.chat> Co-authored-by: MartinSchoeler <martinschoeler8@gmail.com> Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Proposed changes (including videos or screenshots)
Issue(s)
Steps to test or reproduce
Further comments
Summary by CodeRabbit
New Features
Bug Fixes / Behavior
Tests & Docs
✏️ Tip: You can customize this high-level summary in your review settings.