Skip to content

Conversation

@KevLehman
Copy link
Member

@KevLehman KevLehman commented Nov 21, 2025

Proposed changes (including videos or screenshots)

Issue(s)

Steps to test or reproduce

Further comments

Summary by CodeRabbit

  • New Features
    • Added support for a new room administration field, expanding available configuration options.

✏️ Tip: You can customize this high-level summary in your review settings.

@dionisio-bot
Copy link
Contributor

dionisio-bot bot commented Nov 21, 2025

Looks like this PR is not ready to merge, because of the following issues:

  • This PR is missing the 'stat: QA assured' label
  • This PR is missing the required milestone or project

Please fix the issues and try again

If you have any trouble, please check the PR guidelines

@changeset-bot
Copy link

changeset-bot bot commented Nov 21, 2025

⚠️ No Changeset found

Latest commit: 9cd314b

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Nov 21, 2025

Walkthrough

Adds abacAttributes field to room admin fields export and extends the RoomAdminFieldsType union type to include this new field. Two files updated with minimal, localized type and constant changes.

Changes

Cohort / File(s) Change Summary
Room ABAC field exposure
apps/meteor/lib/rooms/adminFields.ts, packages/core-typings/src/IRoom.ts
Adds abacAttributes: 1 to the adminFields constant and extends RoomAdminFieldsType union to include 'abacAttributes' as an allowed admin field key

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~3 minutes

  • Verify field name consistency between the constant export and type definition
  • Confirm abacAttributes is the correct field name (not a typo)

Possibly related PRs

Suggested labels

stat: ready to merge, stat: QA assured

Suggested reviewers

  • tassoevan
  • MartinSchoeler

Poem

🐰 A field takes root in admin's keep,
Where abacAttributes now runs deep,
Type-safe and true, both files aligned,
Control and access, gently refined! ✨

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and specifically summarizes the main change: adding 'abacAttributes' to the adminRooms projection, which directly matches the modifications shown in both modified files.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch fix/return-abacattrs-adminrooms

📜 Recent 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.

📥 Commits

Reviewing files that changed from the base of the PR and between 2588011 and 9cd314b.

📒 Files selected for processing (2)
  • apps/meteor/lib/rooms/adminFields.ts (1 hunks)
  • packages/core-typings/src/IRoom.ts (1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 37303
File: apps/meteor/tests/end-to-end/api/abac.ts:1125-1137
Timestamp: 2025-10-27T14:38:46.994Z
Learning: In Rocket.Chat ABAC feature, when ABAC is disabled globally (ABAC_Enabled setting is false), room-level ABAC attributes are not evaluated when changing room types. This means converting a private room to public will succeed even if the room has ABAC attributes, as long as the global ABAC setting is disabled.
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 37299
File: apps/meteor/ee/server/lib/ldap/Manager.ts:438-454
Timestamp: 2025-10-24T17:32:05.348Z
Learning: In Rocket.Chat, ABAC attributes can only be set on private rooms and teams (type 'p'), not on public rooms (type 'c'). Therefore, when checking for ABAC-protected rooms/teams during LDAP sync or similar operations, it's sufficient to query only private rooms using methods like `findPrivateRoomsByIdsWithAbacAttributes`.
📚 Learning: 2025-10-27T14:38:46.994Z
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 37303
File: apps/meteor/tests/end-to-end/api/abac.ts:1125-1137
Timestamp: 2025-10-27T14:38:46.994Z
Learning: In Rocket.Chat ABAC feature, when ABAC is disabled globally (ABAC_Enabled setting is false), room-level ABAC attributes are not evaluated when changing room types. This means converting a private room to public will succeed even if the room has ABAC attributes, as long as the global ABAC setting is disabled.

Applied to files:

  • packages/core-typings/src/IRoom.ts
  • apps/meteor/lib/rooms/adminFields.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:

  • packages/core-typings/src/IRoom.ts
  • apps/meteor/lib/rooms/adminFields.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/rooms/adminFields.ts
⏰ 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). (5)
  • GitHub Check: 🔨 Test Unit / Unit Tests
  • GitHub Check: 🔨 Test Storybook / Test Storybook
  • GitHub Check: 🔎 Code Check / Code Lint
  • GitHub Check: 🔎 Code Check / TypeScript
  • GitHub Check: 📦 Meteor Build (coverage)
🔇 Additional comments (2)
apps/meteor/lib/rooms/adminFields.ts (1)

32-32: LGTM! Field addition is type-safe and properly aligned.

The addition of abacAttributes to the admin fields projection is correct and type-safe. This properly synchronizes with the RoomAdminFieldsType extension in the type definitions.

Based on learnings: ABAC attributes are used for attribute-based access control on private rooms and teams.

packages/core-typings/src/IRoom.ts (1)

405-406: LGTM! Type definition correctly extended.

The addition of 'abacAttributes' to the RoomAdminFieldsType union is correct and properly synchronized with the projection object in apps/meteor/lib/rooms/adminFields.ts.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

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

@KevLehman KevLehman marked this pull request as ready for review November 21, 2025 18:22
@KevLehman KevLehman requested a review from a team as a code owner November 21, 2025 18:22
@github-actions
Copy link
Contributor

📦 Docker Image Size Report

📈 Changes

Service Current Baseline Change Percent
sum of all images 1.2GiB 1.2GiB +38MiB
rocketchat 367MiB 347MiB +21MiB
omnichannel-transcript-service 141MiB 132MiB +8.7MiB
queue-worker-service 141MiB 132MiB +8.7MiB
ddp-streamer-service 127MiB 127MiB +1.3KiB
account-service 114MiB 114MiB +2.6KiB
authorization-service 111MiB 111MiB +50KiB
stream-hub-service 111MiB 111MiB +731B
presence-service 111MiB 111MiB +912B

📊 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/21 19:09 (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]
  line "authorization-service" [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]
  line "omnichannel-transcript-service" [0.14, 0.14, 0.14, 0.14, 0.14, 0.13, 0.14]
  line "presence-service" [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.14]
  line "rocketchat" [0.36, 0.36, 0.35, 0.35, 0.35, 0.34, 0.36]
  line "stream-hub-service" [0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11]
Loading

Statistics (last 6 days):

  • 📊 Average: 1.5GiB
  • ⬇️ Minimum: 1.2GiB
  • ⬆️ Maximum: 1.6GiB
  • 🎯 Current PR: 1.2GiB
ℹ️ About this report

This report compares Docker image sizes from this build against the develop baseline.

  • Tag: pr-37576
  • Baseline: develop
  • Timestamp: 2025-11-21 19:09:51 UTC
  • Historical data points: 6

Updated: Fri, 21 Nov 2025 19:09:52 GMT

@codecov
Copy link

codecov bot commented Nov 21, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
⚠️ Please upload report for BASE (feat/abac@2588011). Learn more about missing BASE report.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##             feat/abac   #37576   +/-   ##
============================================
  Coverage             ?   54.26%           
============================================
  Files                ?     2659           
  Lines                ?    49968           
  Branches             ?    11125           
============================================
  Hits                 ?    27114           
  Misses               ?    20721           
  Partials             ?     2133           
Flag Coverage Δ
e2e 57.41% <ø> (?)
e2e-api 42.83% <ø> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@KevLehman KevLehman merged commit 5730c69 into feat/abac Nov 21, 2025
72 of 74 checks passed
@KevLehman KevLehman deleted the fix/return-abacattrs-adminrooms branch November 21, 2025 20:53
@coderabbitai coderabbitai bot mentioned this pull request Dec 9, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants