test: Fix abac tests #37658
test: Fix abac tests #37658
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 |
|
WalkthroughThe changes modify two ABAC test files by randomizing user identifiers, adjusting test isolation mechanisms (removing global beforeEach cleanup), introducing dynamic room identifiers (rid), reorganizing test initialization order, and expanding test coverage with granular scenarios for user-auto-removal behavior including new key/value additions, multi-attribute enforcement, and idempotency cases. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–25 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 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 |
📦 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 15:59", "12/01 19:32 (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]
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]
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]
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]
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]
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]
line "rocketchat" [0.36, 0.36, 0.35, 0.35, 0.35, 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]
Statistics (last 10 days):
ℹ️ About this reportThis report compares Docker image sizes from this build against the
Updated: Mon, 01 Dec 2025 19:32:22 GMT |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## feat/abac #37658 +/- ##
============================================
Coverage ? 54.30%
============================================
Files ? 2657
Lines ? 50112
Branches ? 11207
============================================
Hits ? 27212
Misses ? 20727
Partials ? 2173
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
ee/packages/abac/src/subject-attributes-validations.spec.ts (1)
37-47: Randomized test user IDs/username look good; rely on overrides when neededUsing
Math.random()inmakeUserfor_idandusernameis a reasonable way to avoid cross-test collisions now that you’re not globally clearing the users collection. Overrides still win, so scenarios like “no _id” remain testable. If you ever need fully deterministic runs across environments, consider swapping to a simple counter-based generator, but that’s a low‑priority refinement.ee/packages/abac/src/user-auto-removal.spec.ts (1)
46-55: Dynamicridand explicit__roomswiring improve isolation;memberflag now unused
insertRoomreturning a dynamically generatedridand using it consistently (including inname) avoids hard‑coded IDs and cross‑test coupling, while__rooms: u.extraRooms || []makes room membership explicit at the call site. Moving theabac_dummy_initinsert afterregisterServiceModelsalso keeps the model registration order clean. The only minor nit is that themember?: booleanfield in theinsertUsersparameter type is no longer used; you could drop it (and the corresponding arguments) to avoid confusion, but it doesn’t affect correctness.Also applies to: 75-76, 83-93
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
ee/packages/abac/src/subject-attributes-validations.spec.ts(1 hunks)ee/packages/abac/src/user-auto-removal.spec.ts(9 hunks)
🧰 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:
ee/packages/abac/src/subject-attributes-validations.spec.tsee/packages/abac/src/user-auto-removal.spec.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/subject-attributes-validations.spec.tsee/packages/abac/src/user-auto-removal.spec.ts
🧠 Learnings (13)
📓 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.
📚 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/subject-attributes-validations.spec.tsee/packages/abac/src/user-auto-removal.spec.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Maintain test isolation between test cases in Playwright tests
Applied to files:
ee/packages/abac/src/subject-attributes-validations.spec.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Ensure clean state for each test execution in Playwright tests
Applied to files:
ee/packages/abac/src/subject-attributes-validations.spec.tsee/packages/abac/src/user-auto-removal.spec.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Utilize Playwright fixtures (`test`, `page`, `expect`) for consistency in test files
Applied to files:
ee/packages/abac/src/subject-attributes-validations.spec.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Group related tests in the same file
Applied to files:
ee/packages/abac/src/subject-attributes-validations.spec.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Use `expect` matchers for assertions (`toEqual`, `toContain`, `toBeTruthy`, `toHaveLength`, etc.) instead of `assert` statements in Playwright tests
Applied to files:
ee/packages/abac/src/subject-attributes-validations.spec.ts
📚 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:
ee/packages/abac/src/user-auto-removal.spec.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:
ee/packages/abac/src/user-auto-removal.spec.ts
📚 Learning: 2025-10-28T16:53:42.761Z
Learnt from: ricardogarim
Repo: RocketChat/Rocket.Chat PR: 37205
File: ee/packages/federation-matrix/src/FederationMatrix.ts:296-301
Timestamp: 2025-10-28T16:53:42.761Z
Learning: In the Rocket.Chat federation-matrix integration (ee/packages/federation-matrix/), the createRoom method from rocket.chat/federation-sdk will support a 4-argument signature (userId, roomName, visibility, displayName) in newer versions. Code using this 4-argument call is forward-compatible with planned library updates and should not be flagged as an error.
Applied to files:
ee/packages/abac/src/user-auto-removal.spec.ts
📚 Learning: 2025-09-25T09:59:26.461Z
Learnt from: Dnouv
Repo: RocketChat/Rocket.Chat PR: 37057
File: packages/apps-engine/src/definition/accessors/IUserRead.ts:23-27
Timestamp: 2025-09-25T09:59:26.461Z
Learning: AppUserBridge.getUserRoomIds in apps/meteor/app/apps/server/bridges/users.ts always returns an array of strings (mapping subscription documents to room IDs), never undefined, even when user has no room subscriptions.
Applied to files:
ee/packages/abac/src/user-auto-removal.spec.ts
📚 Learning: 2025-09-25T09:59:26.461Z
Learnt from: Dnouv
Repo: RocketChat/Rocket.Chat PR: 37057
File: packages/apps-engine/src/definition/accessors/IUserRead.ts:23-27
Timestamp: 2025-09-25T09:59:26.461Z
Learning: AppUserBridge.getUserRoomIds in apps/meteor/app/apps/server/bridges/users.ts always returns an array of strings by mapping subscription documents to room IDs, never undefined, even when user has no room subscriptions.
Applied to files:
ee/packages/abac/src/user-auto-removal.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/user-auto-removal.spec.ts
🔇 Additional comments (7)
ee/packages/abac/src/user-auto-removal.spec.ts (7)
112-124: “New key addition” scenario reads well and validates all expected user outcomesThe
setRoomAbacAttributes“new key addition” test now uses a per-testridandextraRooms: [rid]to seed membership, and the audited/remaining sets (u2_newkey,u3_newkeyremoved;u1_newkey,u4_newkeykept;u5_newkeyignored as non‑member) match the described semantics for adding a new required key/value set. The spy ononRoomAttributesChangedalso validates that the hook sees the correct room document. This is solid coverage.Also applies to: 135-151
153-160: Duplicate room attribute values test correctly guards set semanticsThe “duplicate values” test around
setRoomAbacAttributeswith['eng', 'eng', 'sales']nicely asserts that duplicates don’t change behavior: onlyu2_dupvals(missingsales) is audited and removed, whileu1_dupvalsstays in__rooms. The expectations onauditSpyand the per‑user__roomsprojections line up with the intended set‑based evaluation.Also applies to: 166-175
178-188: New‑value vs removal‑only paths are clearly distinguishedThe
updateRoomAbacAttributeValuestests cleanly separate:
- adding a new required value (
['eng']→['eng', 'sales']), whereu2_newvalis correctly audited and removed while superset users remain; and- removal‑only updates (
['eng', 'sales']→['eng']), whereauditSpyis not called and bothu1_rmval/u2_rmvalkeep theirridmembership.This gives good signal that the implementation only triggers reevaluation/user removals on “growth” of requirements, not on relaxations.
Also applies to: 193-205, 207-227
230-288: Multi‑attribute (AND) enforcement scenario is comprehensiveThe multi‑attribute
setRoomAbacAttributestest covers:
- a user becoming compliant after expansion (
u1_multi),- missing secondary key (
u2_multi),- missing primary key (
u3_multi),- full superset (
u4_multi),- and partial mismatch (
u5_multi).The assertions on
auditSpy(exactlyu2_multi,u3_multi,u5_multi) and themembershipsmap (onlyu1_multiandu4_multiretainingrid) provide strong coverage for AND semantics across multiple keys.Also applies to: 291-298, 299-307
311-345: Idempotency test correctly proves no extra removals or logs on repeated callsThe idempotency test demonstrates the intended behavior well: first call to
setRoomAbacAttributesaudits and removes onlyu2_idem, and the second identical call produces no newAudit.actionPerformedcalls and leaves__roomsunchanged. Clearing the spies between calls keeps the expectations sharp. This is a good guard against accidental non‑idempotent logic inonRoomAttributesChanged.
348-371: Superset and missing‑key edge cases are covered cleanlyThe “superset” test confirms that a user with exactly the required values plus extras (
u1_superset) remains while a user missing one required value (u2_superset) is audited and removed. The “missing attribute key” test then exercises users with a different key (dept), no ABAC attributes at all, and a correctregion, asserting that onlyu2_misskeyandu3_misskeyare audited/removed andu1_misskeystays. Together these cover both value‑subset and key‑presence edge conditions.Also applies to: 373-402
405-441: Large population test is a good lightweight performance and correctness sanity checkThe “large member set” test with 300 users (half compliant, half missing a value) is a nice balance between realism and runtime: it validates that exactly 150 users are audited and removed, specific odd/even IDs behave as expected, and
usersCol.countDocuments({ __rooms: rid })returns 150. This should help catch regressions in both correctness and any obvious O(N²) behavior without being too heavy for the test suite.
Proposed changes (including videos or screenshots)
Issue(s)
Steps to test or reproduce
Further comments
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.