Skip to content

Conversation

@KevLehman
Copy link
Member

@KevLehman KevLehman commented Oct 20, 2025

Proposed changes (including videos or screenshots)

Issue(s)

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

Steps to test or reproduce

Further comments

Summary by CodeRabbit

  • New Features

    • Added validations that block updates when a room is managed by ABAC.
  • Bug Fixes

    • Prevented ABAC-managed rooms from being set as default or team-default.
    • Prevented converting default/team-default rooms into ABAC-managed rooms.
    • Skipped adding users to ABAC-managed default rooms during onboarding.
  • Tests

    • Added unit and end-to-end tests covering ABAC room restrictions and default-room conversion rules.

@dionisio-bot
Copy link
Contributor

dionisio-bot bot commented Oct 20, 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 Oct 20, 2025

⚠️ No Changeset found

Latest commit: bab7701

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 Oct 20, 2025

Walkthrough

Adds checks preventing default and team-default rooms from becoming ABAC-managed or being set as defaults when ABAC is enabled, with API guards, channel-settings validation, subscription skipping, ABAC core guards, and new unit and end-to-end tests.

Changes

Cohort / File(s) Summary
API Team Guard
apps/meteor/app/api/server/v1/teams.ts
Adds ABAC feature-flagged validation in teams.updateRoom to fetch the private room and reject updates with error-room-is-abac-managed if the room has abacAttributes.
Channel Settings Validation
apps/meteor/app/channel-settings/server/methods/saveRoomSettings.ts
Imports settings and extends validators to block marking an ABAC-managed room as default when ABAC is enabled (raises action-not-allowed).
Default Subscription Logic
apps/meteor/app/lib/server/functions/addUserToDefaultChannels.ts
Imports settings and skips subscribing users to default rooms that are ABAC-managed when ABAC is enabled.
End-to-End Tests
apps/meteor/tests/end-to-end/api/abac.ts
Adds multiple E2E suites verifying default/team-default rooms cannot be assigned ABAC attributes or converted to ABAC-managed, covering setup, teardown, enterprise scenarios, and cleanup.
ABAC Core Guards
ee/packages/abac/src/index.ts
Extends room projections to include default and teamDefault; adds guards across ABAC operations (set, update values, remove, add-by-key, replace-by-key) to reject conversions with error-cannot-convert-default-room-to-abac.
ABAC Unit Tests
ee/packages/abac/src/index.spec.ts
Adds unit tests asserting ABAC operations fail for default/teamDefault rooms and verifying mutation hooks/updates are not invoked.

Sequence Diagram(s)

sequenceDiagram
    participant User
    participant API as API / Methods
    participant ABAC as ABAC Service
    participant DB as Rooms DB

    User->>API: request (set default / update team / modify ABAC)
    API->>DB: fetch room (include abacAttributes, default, teamDefault)
    DB-->>API: room metadata

    alt Room has abacAttributes AND (default or teamDefault) AND ABAC enabled
        API-->>User: reject (error-cannot-convert-default-room-to-abac / error-room-is-abac-managed)
    else
        API->>ABAC: proceed with ABAC operation or update
        ABAC-->>API: result
        API-->>User: result
    end
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

Suggested labels

stat: ready to merge, stat: QA assured

Suggested reviewers

  • d-gubert
  • lucas-a-pelegrino
  • tassoevan

Poem

🐰
I hopped through checks both wise and spry,
Kept default burrows safe and dry.
No ABAC roots in hearths that glow—
Quiet tunnels, steady flow. ✨

Pre-merge checks and finishing touches

✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The pull request title "chore: Prevent default room & team channels from becoming abac managed (and viceversa)" directly and accurately reflects the primary objective of the changeset. The title is specific and descriptive, clearly communicating that the changes address bidirectional restrictions between default rooms and ABAC-managed channels. The code changes across all modified files consistently implement mechanisms to prevent these conversions in both directions, making the title a precise summary of the main change.
Linked Issues Check ✅ Passed The code changes comprehensively address the objective from ABAC-64 to prevent default room and team channels from becoming ABAC-managed and vice versa. The implementation includes guards in ee/packages/abac/src/index.ts to block conversion of default/teamDefault rooms to ABAC-managed status, validation in teams.ts and saveRoomSettings.ts to prevent setting ABAC-managed rooms as default, and appropriate handling in addUserToDefaultChannels.ts. The test coverage in both end-to-end and unit tests validates these restrictions across multiple scenarios, demonstrating full compliance with the linked issue requirements.
Docstring Coverage ✅ Passed No functions found in the changes. Docstring coverage check skipped.
✨ 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 chore/prevent-default-room-abac

📜 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 77f01e0 and bab7701.

📒 Files selected for processing (1)
  • apps/meteor/app/api/server/v1/teams.ts (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • apps/meteor/app/api/server/v1/teams.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

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 force-pushed the chore/prevent-default-room-abac branch from a17f4ac to 9103d1e Compare October 20, 2025 16:14
@kody-ai
Copy link

kody-ai bot commented Oct 20, 2025

Your trial has ended! 😢

To keep getting reviews, activate your plan here.

Got questions about plans or want to see if we can extend your trial? Talk to our founders here.😎

@kody-ai
Copy link

kody-ai bot commented Oct 20, 2025

Your trial has ended! 😢

To keep getting reviews, activate your plan here.

Got questions about plans or want to see if we can extend your trial? Talk to our founders here.😎

@codecov
Copy link

codecov bot commented Oct 20, 2025

Codecov Report

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

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##             feat/abac   #37264   +/-   ##
============================================
  Coverage             ?   70.11%           
============================================
  Files                ?     3035           
  Lines                ?   104276           
  Branches             ?    18501           
============================================
  Hits                 ?    73118           
  Misses               ?    29211           
  Partials             ?     1947           
Flag Coverage Δ
e2e 57.33% <ø> (?)
unit 71.67% <100.00%> (?)

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 force-pushed the chore/prevent-default-room-abac branch from 6fa04b9 to aa54532 Compare October 20, 2025 20:49
@KevLehman KevLehman marked this pull request as ready for review October 21, 2025 13:24
@KevLehman KevLehman requested a review from a team as a code owner October 21, 2025 13:24
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: 5

♻️ Duplicate comments (1)
apps/meteor/app/lib/server/functions/addUserToDefaultChannels.ts (1)

17-19: Skip ABAC‑managed rooms: OK, but cache flag and confirm settings key.

  • Compute the flag once before the loop to avoid repeated lookups.
  • Verify the setting id; prior reviews referenced Abac_Enabled/Abac_Enable. Align repo‑wide on a single key and consider a shared helper to read it.

Apply:

-	for await (const room of defaultRooms) {
-		if (settings.get('ABAC_Enabled') && room?.abacAttributes?.length) {
+	const abacEnabled = settings.get('ABAC_Enabled');
+	for await (const room of defaultRooms) {
+		if (abacEnabled && room?.abacAttributes?.length) {
 			continue;
 		}

Run to check for mixed keys:

#!/bin/bash
rg -n -C1 -S "settings\.get\(['\"]A?B?A?C?_?E?n?a?b?l?e?d?['\"]" \
  | sed 's/.*settings\.get(\x27\|\x22\)\(.*\)\(\x27\|\x22\).*/\1/' \
  | sort -u
🧹 Nitpick comments (7)
apps/meteor/app/channel-settings/server/methods/saveRoomSettings.ts (1)

66-79: Validator correctly blocks default=true on ABAC rooms.

Logic and placement are sound. Consider standardizing the error id to match routes (e.g., error-room-is-abac-managed) and a more precise action string for easier i18n/handling.

ee/packages/abac/src/index.ts (3)

172-179: Block converting default/teamDefault rooms to ABAC: LGTM.

Projection includes needed fields; guard throws early as intended. Consider logging a warn before throwing to aid ops diagnostics.

Also applies to: 183-186


222-241: Normalize ABAC attributes: dedupe values to avoid redundant churn.

Current normalize allows duplicate values (tests rely on this), which can create noisy diffs and unnecessary writes. Safely dedupe while preserving order.

Apply:

-	private validateAndNormalizeAttributes(attributes: Record<string, string[]>): IAbacAttributeDefinition[] {
+	private validateAndNormalizeAttributes(attributes: Record<string, string[]>): IAbacAttributeDefinition[] {
 		const keyPattern = /^[A-Za-z0-9_-]+$/;
 		const normalized: IAbacAttributeDefinition[] = [];
 
 		if (Object.keys(attributes).length > 10) {
 			throw new Error('error-invalid-attribute-values');
 		}
 
 		for (const [key, values] of Object.entries(attributes)) {
 			if (!keyPattern.test(key)) {
 				throw new Error('error-invalid-attribute-key');
 			}
 			if (values.length > 10) {
 				throw new Error('error-invalid-attribute-values');
 			}
-			normalized.push({ key, values });
+			// de-duplicate values while preserving order
+			const seen = new Set<string>();
+			const deduped = values.filter((v) => (seen.has(v) ? false : (seen.add(v), true)));
+			normalized.push({ key, values: deduped });
 		}
 
 		return normalized;
 	}

10-12: Variable shadowing of ‘limit’.

Module‑scope limit from p‑limit and local pagination limit share the same name. It’s harmless but confusing. Rename the pagination variable (e.g., pageSize) for clarity.

Also applies to: 52-55

ee/packages/abac/src/index.spec.ts (1)

1-818: Add tests for team default toggling semantics.

After adjusting teams.updateRoom to only block when isDefault=true, add tests to ensure isDefault=false succeeds for ABAC rooms and isDefault=true is blocked.

apps/meteor/tests/end-to-end/api/abac.ts (2)

410-462: Consider consolidating setup logic.

While multiple before hooks are functionally correct, the static analysis tool flags this pattern. Consider consolidating related setup steps into fewer hooks for better readability, or add comments explaining why they're separated.

For example:

-before('create team main room for rooms.saveRoomSettings default restriction test', async () => {
-	// ... setup code ...
-});
-
-before('create local ABAC attribute definition for tests', async () => {
-	// ... setup code ...
-});
-
-before('create private room and try to set it as default', async () => {
-	// ... setup code ...
-});
-
-before('create private team, private room inside it and set as team default', async () => {
-	// ... setup code ...
-});
+before('setup test fixtures for default room restrictions', async () => {
+	// Create team main room for rooms.saveRoomSettings default restriction test
+	// ... consolidated setup code ...
+	
+	// Create local ABAC attribute definition for tests
+	// ... consolidated setup code ...
+	
+	// Create private room and try to set it as default
+	// ... consolidated setup code ...
+	
+	// Create private team, private room inside it and set as team default
+	// ... consolidated setup code ...
+});

611-648: Consider consolidating setup hooks.

Similar to the previous suite, multiple before hooks are flagged by static analysis. While functionally correct, consolidating these two related setup steps into a single hook would improve readability.

Based on learnings

📜 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 0722382 and d28393e.

📒 Files selected for processing (6)
  • apps/meteor/app/api/server/v1/teams.ts (2 hunks)
  • apps/meteor/app/channel-settings/server/methods/saveRoomSettings.ts (2 hunks)
  • apps/meteor/app/lib/server/functions/addUserToDefaultChannels.ts (2 hunks)
  • apps/meteor/tests/end-to-end/api/abac.ts (3 hunks)
  • ee/packages/abac/src/index.spec.ts (5 hunks)
  • ee/packages/abac/src/index.ts (5 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
ee/packages/abac/src/index.ts (1)
packages/core-typings/src/IRoom.ts (1)
  • IRoom (22-98)
apps/meteor/tests/end-to-end/api/abac.ts (2)
apps/meteor/tests/data/api-data.ts (2)
  • request (10-10)
  • credentials (39-42)
apps/meteor/tests/e2e/utils/create-target-channel.ts (2)
  • deleteTeam (66-68)
  • deleteRoom (48-50)
🪛 Biome (2.1.2)
apps/meteor/tests/end-to-end/api/abac.ts

[error] 424-430: Disallow duplicate setup and teardown hooks.

Disallow before duplicacy inside the describe function.

(lint/suspicious/noDuplicateTestHooks)


[error] 432-440: Disallow duplicate setup and teardown hooks.

Disallow before duplicacy inside the describe function.

(lint/suspicious/noDuplicateTestHooks)


[error] 442-462: Disallow duplicate setup and teardown hooks.

Disallow before duplicacy inside the describe function.

(lint/suspicious/noDuplicateTestHooks)


[error] 545-550: Disallow duplicate setup and teardown hooks.

Disallow after duplicacy inside the describe function.

(lint/suspicious/noDuplicateTestHooks)


[error] 631-648: Disallow duplicate setup and teardown hooks.

Disallow before duplicacy inside the describe function.

(lint/suspicious/noDuplicateTestHooks)

🔇 Additional comments (9)
apps/meteor/app/lib/server/functions/addUserToDefaultChannels.ts (1)

8-8: Import looks good.

Consistent with other modules using server settings.

apps/meteor/app/api/server/v1/teams.ts (1)

23-23: Import looks good.

apps/meteor/app/channel-settings/server/methods/saveRoomSettings.ts (1)

14-14: Import looks good.

ee/packages/abac/src/index.ts (4)

270-276: Update values guard for default/teamDefault: LGTM.

Also applies to: 281-284


323-325: Remove attribute guard for default/teamDefault: LGTM.

Also applies to: 330-333


351-357: Add attribute-by-key guard for default/teamDefault: LGTM.

Also applies to: 362-365


384-390: Replace attribute-by-key guard for default/teamDefault: LGTM.

Also applies to: 395-398

ee/packages/abac/src/index.spec.ts (1)

392-397: Great negative‑path coverage for default/teamDefault.

Tests assert the new guards across all entry points. Nicely done.

Also applies to: 398-403, 530-536, 537-543, 601-606, 607-613, 655-661, 662-668, 739-745, 746-752

apps/meteor/tests/end-to-end/api/abac.ts (1)

400-677: Excellent test coverage for ABAC default room restrictions.

The new test suites thoroughly validate that:

  1. Default and team-default rooms cannot become ABAC-managed
  2. ABAC-managed rooms cannot become default or team-default
  3. Restrictions are properly lifted when flags are removed

The test scenarios cover the key edge cases outlined in the PR objectives (ABAC-64).

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: 0

🧹 Nitpick comments (2)
apps/meteor/tests/end-to-end/api/abac.ts (2)

411-462: Multiple before hooks are intentional but could be consolidated.

The static analysis tool flags multiple before hooks, but each performs a distinct setup step (team creation, attribute definition, room setup). While this is valid, combining them into a single before hook might improve readability and make the test setup flow clearer.

Example consolidation:

-before('create team main room for rooms.saveRoomSettings default restriction test', async () => {
+before('setup test environment', async () => {
+	// Create team with main room
 	const createTeamMain = await request
 		.post(`${v1}/teams.create`)
 		.set(credentials)
 		.send({ name: teamNameMainRoom, type: 1 })
 		.expect(200);
 	teamIdMainRoom = createTeamMain.body.team._id;
-
 	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 () => {
+
+	// Create ABAC attribute definition
 	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 () => {
+
+	// Create private default room
 	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 () => {
+
+	// Create team with default room
 	const createTeamRes = await request.post(`${v1}/teams.create`).set(credentials).send({ name: teamName, type: 0 }).expect(200);
 	teamId = createTeamRes.body.team._id;
-
 	const roomRes = await createRoom({
 		type: 'p',
 		name: `abac-team-room-${Date.now()}`,
 		extraData: { teamId },
 	});
 	teamPrivateRoomId = roomRes.body.group._id;
-
 	const setDefaultRes = await request
 		.post(`${v1}/teams.updateRoom`)
 		.set(credentials)
 		.send({ teamId, roomId: teamPrivateRoomId, isDefault: true })
 		.expect(200);
-
 	if (setDefaultRes.body?.room?.teamDefault) {
 		teamDefaultRoomId = teamPrivateRoomId;
 	}
 });

606-643: Multiple before hooks could be consolidated.

Similar to the first test suite, these before hooks perform distinct setup steps but could be combined into a single hook for better readability. The current approach is valid but consolidation would improve test organization.

📜 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 d28393e and 74a009e.

📒 Files selected for processing (1)
  • apps/meteor/tests/end-to-end/api/abac.ts (3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
apps/meteor/tests/end-to-end/api/abac.ts (2)
apps/meteor/tests/data/api-data.ts (2)
  • request (10-10)
  • credentials (39-42)
apps/meteor/tests/e2e/utils/create-target-channel.ts (2)
  • deleteRoom (48-50)
  • deleteTeam (66-68)
🪛 Biome (2.1.2)
apps/meteor/tests/end-to-end/api/abac.ts

[error] 424-430: Disallow duplicate setup and teardown hooks.

Disallow before duplicacy inside the describe function.

(lint/suspicious/noDuplicateTestHooks)


[error] 432-440: Disallow duplicate setup and teardown hooks.

Disallow before duplicacy inside the describe function.

(lint/suspicious/noDuplicateTestHooks)


[error] 442-462: Disallow duplicate setup and teardown hooks.

Disallow before duplicacy inside the describe function.

(lint/suspicious/noDuplicateTestHooks)


[error] 626-643: 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). (3)
  • GitHub Check: 📦 Build Packages
  • GitHub Check: CodeQL-Build
  • GitHub Check: CodeQL-Build
🔇 Additional comments (5)
apps/meteor/tests/end-to-end/api/abac.ts (5)

9-9: LGTM!

The deleteTeam import is correct, and all usages in the file (lines 543, 544, 646) properly pass team names as strings, matching the expected signature.


464-539: LGTM!

The test cases comprehensively verify that:

  • Default rooms cannot be converted to ABAC-managed rooms
  • Team default rooms cannot be converted to ABAC-managed rooms
  • ABAC attributes can be added after removing default flags
  • The restrictions apply correctly to team main rooms

The error assertions and success conditions are appropriate.


541-545: LGTM!

The cleanup correctly uses team names (teamName and teamNameMainRoom) instead of IDs, addressing the previous review concerns. All resources created during test setup are properly cleaned up.


649-671: LGTM!

The tests correctly verify the inverse restriction: ABAC-managed rooms (both regular and team rooms) cannot be converted to default rooms. The error assertions properly check for the expected error messages.


645-647: LGTM!

The cleanup properly addresses the previous review comment by including abacRoomId deletion. The teamRoomId is likely cascade-deleted when the team is deleted. Using Promise.all for concurrent cleanup is appropriate.

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: 0

🧹 Nitpick comments (2)
apps/meteor/tests/end-to-end/api/abac.ts (2)

410-460: Consider consolidating duplicate before hooks (linter warning).

Biome flags duplicate before hooks at lines 410-420, 422-428, 430-438, and 440-460. While your descriptive labels improve readability, consolidating these into a single before hook or using nested describe blocks would resolve the linter warnings and follow testing best practices.

Example consolidation:

-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(async () => {
+	// Create team main room for rooms.saveRoomSettings default restriction test
+	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);
+
+	// Create local ABAC attribute definition for tests
+	await request
+		.post(`${v1}/abac/attributes`)
+		.set(credentials)
+		.send({ key: localAbacKey, values: ['red', 'green'] })
+		.expect(200);
+
+	// Create private room and set as default
+	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);
+
+	// Create private team, private room inside it and set as team default
 	const createTeamRes = await request.post(`${v1}/teams.create`).set(credentials).send({ name: teamName, type: 0 }).expect(200);
 	teamId = createTeamRes.body.team._id;
 
 	const roomRes = await createRoom({
 		type: 'p',
 		name: `abac-team-room-${Date.now()}`,
 		extraData: { teamId },
 	});
 	teamPrivateRoomId = roomRes.body.group._id;
 
 	const setDefaultRes = await request
 		.post(`${v1}/teams.updateRoom`)
 		.set(credentials)
 		.send({ teamId, roomId: teamPrivateRoomId, isDefault: true })
 		.expect(200);
 
 	if (setDefaultRes.body?.room?.teamDefault) {
 		teamDefaultRoomId = teamPrivateRoomId;
 	}
 });

604-641: Consider consolidating duplicate before hooks (linter warning).

Similar to the previous test block, Biome flags duplicate before hooks at lines 604-622 and 624-641. Consider consolidating them into a single before hook for cleaner code and to resolve linter warnings.

Example consolidation:

-before('create attribute definition and ABAC-managed private room', async () => {
+before(async () => {
+	// Create attribute definition and ABAC-managed private room
 	await request
 		.post(`${v1}/abac/attributes`)
 		.set(credentials)
 		.send({ key: conversionAttrKey, values: ['alpha', 'beta'] })
 		.expect(200);
 
 	const roomRes = await createRoom({
 		type: 'p',
 		name: `abac-conversion-room-${Date.now()}`,
 	});
 	abacRoomId = roomRes.body.group._id;
 
 	await request
 		.post(`${v1}/abac/room/${abacRoomId}/attributes/${conversionAttrKey}`)
 		.set(credentials)
 		.send({ values: ['alpha'] })
 		.expect(200);
-});
-
-before('create team, private room inside, add ABAC attribute', async () => {
-	// Public team
+
+	// Create team, private room inside, add ABAC attribute
 	const teamRes = await request.post(`${v1}/teams.create`).set(credentials).send({ name: teamName, type: 0 }).expect(200);
 	teamIdForConversion = teamRes.body.team._id;
 
 	const teamRoomRes = await createRoom({
 		type: 'p',
 		name: `abac-team-conversion-room-${Date.now()}`,
 		extraData: { teamId: teamIdForConversion },
 	});
 	teamRoomId = teamRoomRes.body.group._id;
 
 	await request
 		.post(`${v1}/abac/room/${teamRoomId}/attributes/${conversionAttrKey}`)
 		.set(credentials)
 		.send({ values: ['beta'] })
 		.expect(200);
 });
📜 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 74a009e and 77f01e0.

📒 Files selected for processing (1)
  • apps/meteor/tests/end-to-end/api/abac.ts (3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
apps/meteor/tests/end-to-end/api/abac.ts (2)
apps/meteor/tests/data/api-data.ts (2)
  • request (10-10)
  • credentials (39-42)
apps/meteor/tests/e2e/utils/create-target-channel.ts (2)
  • deleteRoom (48-50)
  • deleteTeam (66-68)
🪛 Biome (2.1.2)
apps/meteor/tests/end-to-end/api/abac.ts

[error] 422-428: Disallow duplicate setup and teardown hooks.

Disallow before duplicacy inside the describe function.

(lint/suspicious/noDuplicateTestHooks)


[error] 430-438: Disallow duplicate setup and teardown hooks.

Disallow before duplicacy inside the describe function.

(lint/suspicious/noDuplicateTestHooks)


[error] 440-460: Disallow duplicate setup and teardown hooks.

Disallow before duplicacy inside the describe function.

(lint/suspicious/noDuplicateTestHooks)


[error] 624-641: 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). (3)
  • GitHub Check: 📦 Build Packages
  • GitHub Check: CodeQL-Build
  • GitHub Check: CodeQL-Build
🔇 Additional comments (5)
apps/meteor/tests/end-to-end/api/abac.ts (5)

9-9: LGTM! Import is correct.

The deleteTeam import and its usage throughout the file correctly match the helper signature (credentials, teamName).


462-537: Excellent test coverage for default room ABAC restrictions.

The test cases comprehensively validate the PR objective:

  • Blocking ABAC attribute assignment to default and team-default rooms (lines 462-484)
  • Allowing ABAC after removing default flags (lines 486-514)
  • Enforcing restrictions on team main rooms (lines 516-537)

Error messages are properly validated, and both negative and positive paths are tested.


539-543: LGTM! Cleanup is complete.

The after hook properly cleans up all created resources. Team deletions will cascade to associated rooms (mainRoomIdSaveSettings and teamPrivateRoomId).


647-669: Excellent coverage for ABAC-to-default conversion restrictions.

These tests properly validate the reverse direction: preventing ABAC-managed rooms from being converted to default or team-default. Error messages are validated, completing the bidirectional protection required by the PR objective.


643-645: LGTM! Cleanup properly handles all resources.

The after hook correctly cleans up both the team (which cascades to teamRoomId) and the standalone ABAC-managed room.

@KevLehman KevLehman requested a review from tassoevan October 22, 2025 16:34
@tassoevan tassoevan merged commit 7c32811 into feat/abac Oct 22, 2025
86 of 89 checks passed
@tassoevan tassoevan deleted the chore/prevent-default-room-abac branch October 22, 2025 18:07
KevLehman added a commit that referenced this pull request Oct 22, 2025
…d (and viceversa) (#37264)

Co-authored-by: Tasso <tasso.evangelista@rocket.chat>
KevLehman added a commit that referenced this pull request Oct 30, 2025
…d (and viceversa) (#37264)

Co-authored-by: Tasso <tasso.evangelista@rocket.chat>
KevLehman added a commit that referenced this pull request Nov 5, 2025
…d (and viceversa) (#37264)

Co-authored-by: Tasso <tasso.evangelista@rocket.chat>
KevLehman added a commit that referenced this pull request Nov 6, 2025
…d (and viceversa) (#37264)

Co-authored-by: Tasso <tasso.evangelista@rocket.chat>
KevLehman added a commit that referenced this pull request Nov 12, 2025
…d (and viceversa) (#37264)

Co-authored-by: Tasso <tasso.evangelista@rocket.chat>
KevLehman added a commit that referenced this pull request Nov 18, 2025
…d (and viceversa) (#37264)

Co-authored-by: Tasso <tasso.evangelista@rocket.chat>
KevLehman added a commit that referenced this pull request Nov 24, 2025
…d (and viceversa) (#37264)

Co-authored-by: Tasso <tasso.evangelista@rocket.chat>
KevLehman added a commit that referenced this pull request Nov 27, 2025
…d (and viceversa) (#37264)

Co-authored-by: Tasso <tasso.evangelista@rocket.chat>
KevLehman added a commit that referenced this pull request Dec 1, 2025
…d (and viceversa) (#37264)

Co-authored-by: Tasso <tasso.evangelista@rocket.chat>
KevLehman added a commit that referenced this pull request Dec 2, 2025
…d (and viceversa) (#37264)

Co-authored-by: Tasso <tasso.evangelista@rocket.chat>
KevLehman added a commit that referenced this pull request Dec 8, 2025
…d (and viceversa) (#37264)

Co-authored-by: Tasso <tasso.evangelista@rocket.chat>
KevLehman added a commit that referenced this pull request Dec 10, 2025
…d (and viceversa) (#37264)

Co-authored-by: Tasso <tasso.evangelista@rocket.chat>
KevLehman added a commit that referenced this pull request Dec 15, 2025
…d (and viceversa) (#37264)

Co-authored-by: Tasso <tasso.evangelista@rocket.chat>
MartinSchoeler pushed a commit that referenced this pull request Dec 17, 2025
…d (and viceversa) (#37264)

Co-authored-by: Tasso <tasso.evangelista@rocket.chat>
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