Skip to content

Conversation

@KevLehman
Copy link
Member

@KevLehman KevLehman commented Nov 10, 2025

Proposed changes (including videos or screenshots)

Issue(s)

https://rocketchat.atlassian.net/browse/ABAC-72

Steps to test or reproduce

Further comments

Summary by CodeRabbit

  • New Features
    • Added a new setting to display ABAC attributes in the room contextual bar. When enabled alongside the ABAC feature, users can view attributes assigned to each room directly in the sidebar, providing transparency into room access control settings and attribute configurations.

@dionisio-bot
Copy link
Contributor

dionisio-bot bot commented Nov 10, 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 10, 2025

⚠️ No Changeset found

Latest commit: a43ad1c

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 10, 2025

Walkthrough

The PR introduces a new ABAC setting ABAC_ShowAttributesInRooms to display ABAC attributes in room contextual bars, conditioned by ABAC_Enabled status. Additionally, the cache decision time setting is now gated behind ABAC_Enabled. English locale strings are added for the new setting.

Changes

Cohort / File(s) Change Summary
ABAC Settings Configuration
apps/meteor/ee/server/settings/abac.ts
Added new boolean setting ABAC_ShowAttributesInRooms gated by ABAC_Enabled. Added enableQuery condition to Abac_Cache_Decision_Time_Seconds to be effective only when ABAC is enabled.
Internationalization
packages/i18n/src/locales/en.i18n.json
Added two new locale entries: ABAC_ShowAttributesInRooms ("Show ABAC attributes in rooms") and ABAC_ShowAttributesInRooms_Description (describing room attribute display in contextual bar).

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~3 minutes

  • These are straightforward configuration additions with no logic modifications or signature changes.

Possibly related PRs

  • #37423: Directly modifies the same ABAC settings surface, adding ABAC_ShowAttributesInRooms and gating behaviors.
  • #37139: Related ABAC settings and i18n modifications affecting the same files and locale keys.

Suggested labels

stat: ready to merge, stat: QA assured

Suggested reviewers

  • MartinSchoeler
  • tassoevan

Poem

🐰 In rooms where attributes now shine bright,
ABAC's secrets revealed to the light,
Admin controls what the members can see,
Toggled with ease, as gated can be! ✨

Pre-merge checks and finishing touches

❌ Failed checks (1 warning, 1 inconclusive)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
Linked Issues check ❓ Inconclusive The PR implements the required boolean admin setting (ABAC_ShowAttributesInRooms) with correct gating logic. However, UI implementation for Room Attributes section in Room Info contextual bar is not evident in the code changes provided. Verify that Room Attributes section UI has been implemented in the Room Info contextual bar component. Additional files may need to be reviewed to confirm complete implementation of ABAC-72.
✅ 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 concisely describes the main feature: adding a new setting for displaying ABAC attributes in rooms.
Out of Scope Changes check ✅ Passed All code changes are directly related to the ABAC-72 requirements: adding the new setting, implementing enableQuery gating, and providing i18n translations for the feature.
✨ 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 feat/abac-attr-setting

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.

@codecov
Copy link

codecov bot commented Nov 10, 2025

Codecov Report

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

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##             feat/abac   #37465   +/-   ##
============================================
  Coverage             ?   70.61%           
============================================
  Files                ?     3064           
  Lines                ?   106367           
  Branches             ?    18719           
============================================
  Hits                 ?    75113           
  Misses               ?    29286           
  Partials             ?     1968           
Flag Coverage Δ
e2e 57.49% <ø> (?)
unit 72.27% <ø> (?)

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 marked this pull request as ready for review November 11, 2025 14:45
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (1)
apps/meteor/ee/server/settings/abac.ts (1)

18-24: Consider adding i18nDescription property.

The locale strings ABAC_ShowAttributesInRooms_Description were added (per the AI summary), but the setting doesn't reference them via i18nDescription. The ABAC_Enabled setting on line 16 uses i18nDescription to display its description in the admin UI. Adding it here would provide helpful context to administrators.

Apply this diff to add the description reference:

 await this.add('ABAC_ShowAttributesInRooms', false, {
 	type: 'boolean',
 	public: true,
 	invalidValue: false,
 	section: 'ABAC',
+	i18nDescription: 'ABAC_ShowAttributesInRooms_Description',
 	enableQuery: { _id: 'ABAC_Enabled', value: true },
 });
📜 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 f6a9fab and a43ad1c.

📒 Files selected for processing (2)
  • apps/meteor/ee/server/settings/abac.ts (1 hunks)
  • packages/i18n/src/locales/en.i18n.json (1 hunks)
🧰 Additional context used
🧠 Learnings (5)
📓 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:

  • apps/meteor/ee/server/settings/abac.ts
  • packages/i18n/src/locales/en.i18n.json
📚 Learning: 2025-11-07T14:50:33.544Z
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 37423
File: packages/i18n/src/locales/en.i18n.json:18-18
Timestamp: 2025-11-07T14:50:33.544Z
Learning: Rocket.Chat settings: in apps/meteor/ee/server/settings/abac.ts, the Abac_Cache_Decision_Time_Seconds setting uses invalidValue: 0 as the fallback when ABAC is unlicensed. With a valid license, admins can still set the value to 0 to intentionally disable the ABAC decision cache.

Applied to files:

  • apps/meteor/ee/server/settings/abac.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:

  • apps/meteor/ee/server/settings/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/ee/server/settings/abac.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). (3)
  • GitHub Check: 📦 Build Packages
  • GitHub Check: CodeQL-Build
  • GitHub Check: CodeQL-Build
🔇 Additional comments (1)
apps/meteor/ee/server/settings/abac.ts (1)

30-30: Good addition to gate cache setting behind ABAC_Enabled.

Making Abac_Cache_Decision_Time_Seconds conditional on ABAC_Enabled is logical—when ABAC is disabled, the cache decision time is irrelevant. This improves consistency across ABAC settings and prevents administrators from configuring settings that have no effect.

@KevLehman KevLehman merged commit 3618811 into feat/abac Nov 11, 2025
64 checks passed
@KevLehman KevLehman deleted the feat/abac-attr-setting branch November 11, 2025 19:58
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.

3 participants