fix: GET/DELETE abac endpoints not allowing actions with setting disabled #37415
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 |
|
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughThis PR removes ABAC enablement guards and license requirements from several API endpoints in the ABAC server implementation, allowing operations to proceed without validating the ABAC_Enabled setting. Corresponding test cases validating ABAC-disabled behavior for these endpoints are also removed. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 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 |
There was a problem hiding this comment.
Pull Request Overview
This PR removes license checks and ABAC feature toggle validations from four specific ABAC API endpoints: listing attributes, getting an attribute by ID, deleting an attribute by ID, and checking if an attribute is in use.
- Removes
license: ['abac']from endpoint configuration objects - Removes runtime
settings.get('ABAC_Enabled')validation checks - Affects only read operations and one delete operation while keeping protections on create/update operations
Comments suppressed due to low confidence (1)
apps/meteor/ee/server/api/abac/index.ts:174
- Inconsistent authorization pattern: this GET endpoint has had license and ABAC_Enabled checks removed, while other endpoints in the same file (POST /abac/attributes at lines 177-198, PUT /abac/attributes/:_id at lines 201-223, and POST /abac/rooms/:rid/attributes at lines 23-49) retain both checks. Consider whether read operations should bypass ABAC feature validation or if this creates an unintended security gap where attributes can be listed even when ABAC is disabled.
async function action() {
const { offset, count } = await getPaginationItems(this.queryParams as Record<string, string | string[] | number | null | undefined>);
const { key, values } = this.queryParams;
return API.v1.success(
await Abac.listAbacAttributes({
key,
values,
offset,
count,
}),
);
},
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## feat/abac #37415 +/- ##
=============================================
+ Coverage 70.62% 70.63% +0.01%
=============================================
Files 3064 3064
Lines 106340 106340
Branches 18717 18718 +1
=============================================
+ Hits 75098 75112 +14
+ Misses 29278 29263 -15
- Partials 1964 1965 +1
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
fix/endpoints
branch
from
0681dc1 to
b848e7d
Compare
![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 (1)
apps/meteor/ee/server/api/abac/index.ts (1)
1-284: Consider documenting the endpoint access policy.The file now implements a clear and consistent policy: POST/PUT mutations require ABAC enablement and license, while GET/DELETE operations work without these checks. This design allows administrators to inspect and clean up ABAC artifacts even when the feature is disabled.
Consider adding a brief comment at the file level documenting this policy to help future maintainers understand the intentional differentiation between mutation and read/cleanup operations.
Example documentation:
/** * ABAC API endpoints * * Access policy: * - POST/PUT (mutations): Require ABAC_Enabled setting and 'abac' license * - GET (reads): Work without ABAC enablement to support inspection/audit * - DELETE (cleanup): Work without ABAC enablement to support artifact removal */
📜 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)
apps/meteor/ee/server/api/abac/index.ts(3 hunks)apps/meteor/tests/end-to-end/api/abac.ts(0 hunks)
💤 Files with no reviewable changes (1)
- apps/meteor/tests/end-to-end/api/abac.ts
🧰 Additional context used
🧠 Learnings (3)
📓 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-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/api/abac/index.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/ee/server/api/abac/index.ts
🔇 Additional comments (3)
apps/meteor/ee/server/api/abac/index.ts (3)
226-244: LGTM! Consistent read-only behavior.The GET endpoint correctly allows retrieving attribute definitions without requiring ABAC to be enabled. This is consistent with the other GET endpoint at lines 149-175 and aligns with the PR objective of allowing read operations when ABAC is disabled.
246-264: Consistent cleanup behavior across DELETE endpoints.The DELETE endpoint allows removing attribute definitions without ABAC enablement checks, matching the pattern established in the DELETE endpoints at lines 51-72 and 128-146. This enables administrators to clean up ABAC artifacts even when the feature is disabled.
266-284: LGTM! Consistent read-only behavior for usage checks.The GET endpoint correctly allows checking attribute usage without requiring ABAC to be enabled. This is consistent with the other GET endpoints and supports administrative workflows where operators need to inspect attribute usage before cleanup operations.
MartinSchoeler
left a comment
MartinSchoeler
left a comment
There was a problem hiding this comment.
tested and it is working with the FE
2 participants
Proposed changes (including videos or screenshots)
Issue(s)
https://rocketchat.atlassian.net/browse/ABAC-80
Steps to test or reproduce
Further comments
Summary by CodeRabbit
Refactor
Tests