feat: Prevent invite links from being generated on abac rooms #37325
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 |
|
WalkthroughABAC validation is added to invite link creation and validation flows. When ABAC is enabled and a room has ABAC attributes, invite operations are rejected early with an error before permission checks. Test coverage validates the new behavior including setup/teardown with direct database manipulation. Changes
Sequence DiagramsequenceDiagram
participant Client
participant InviteFunc as Invite Function
participant RoomDB as Room Storage
participant ABAC as ABAC Check
rect rgb(200, 220, 240)
Note over Client,ABAC: NEW: Early ABAC Validation
Client->>InviteFunc: Create/Validate Invite
InviteFunc->>RoomDB: Fetch Room
RoomDB-->>InviteFunc: Room + abacAttributes
InviteFunc->>ABAC: Check if ABAC_Enabled<br/>and has abacAttributes
alt ABAC Match
ABAC-->>InviteFunc: Reject
InviteFunc-->>Client: error-invalid-room ❌
else ABAC No Match
ABAC-->>InviteFunc: Continue
InviteFunc->>InviteFunc: Permission & Expiry Checks
InviteFunc-->>Client: Success ✓
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## feat/abac #37325 +/- ##
=============================================
- Coverage 70.27% 70.10% -0.18%
=============================================
Files 3039 3032 -7
Lines 104736 103987 -749
Branches 18586 18509 -77
=============================================
- Hits 73602 72895 -707
+ Misses 29177 29146 -31
+ Partials 1957 1946 -11
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
feat/block-invite-link
branch
from
e63f7bc to
5ae7f4e
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 (3)
apps/meteor/app/invites/server/functions/validateInviteToken.ts (1)
30-35: ABAC short-circuit is correct; minor type/helper nits.
- Type the flag:
settings.get<boolean>('ABAC_Enabled').- Optional: extract a shared
isRoomAbacManaged(room)helper to avoid duplication with findOrCreateInvite.Apply:
- if (settings.get('ABAC_Enabled') && room?.abacAttributes?.length) { + if (settings.get<boolean>('ABAC_Enabled') && room?.abacAttributes?.length) {apps/meteor/tests/end-to-end/api/abac.ts (2)
15-26: Add cleanup for user ABAC attributes to avoid cross-test leakage.Provide a counterpart helper to unset/reset
abacAttributesand call it in the relevantafterhook.Apply:
const addAbacAttributesToUserDirectly = async (userId: string, abacAttributes: IAbacAttributeDefinition[]) => { await connection.db().collection('users').updateOne( { // @ts-expect-error - collection types for _id _id: userId, }, { $set: { abacAttributes } }, ); }; + +const removeAbacAttributesFromUserDirectly = async (userId: string) => { + await connection.db().collection('users').updateOne( + { _id: userId as any }, + { $unset: { abacAttributes: 1 } }, + ); +};
1258-1412: Align tests with ABAC-8 (403 on creation/usage), add “use invite” test, and clean user ABAC attrs.
- Expect 403 (not 400) when creating invites for ABAC rooms after changing server to
error-action-not-allowed.- Add a test that attempts to use/join via the invite token against an ABAC room and asserts HTTP 403 (usage must be blocked).
- Call
removeAbacAttributesFromUserDirectly(credentials['X-User-Id'])in this block’safterto revert DB changes.Proposed minimal diffs:
- .expect(400) + .expect(403) .expect((res) => { - expect(res.body).to.have.property('errorType', 'error-invalid-room'); - expect(res.body).to.have.property('error').that.includes('Room is ABAC managed'); + expect(res.body).to.have.property('errorType', 'error-action-not-allowed'); + expect(res.body).to.have.property('error').that.includes('error-room-is-abac-managed'); });after(async () => { - await Promise.all(createdInviteIds.map((id) => request.delete(`${v1}/removeInvite/${id}`).set(credentials))); + await Promise.all(createdInviteIds.map((id) => request.delete(`${v1}/removeInvite/${id}`).set(credentials))); + await removeAbacAttributesFromUserDirectly(credentials['X-User-Id']); });And please add a new case:
- “Using invite for ABAC room returns 403”: attempt the join/usage endpoint with the token and assert 403. If the endpoint name differs (e.g.,
/api/v1/invites.useor equivalent), use the appropriate one. 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.
📒 Files selected for processing (3)
apps/meteor/app/invites/server/functions/findOrCreateInvite.ts(1 hunks)apps/meteor/app/invites/server/functions/validateInviteToken.ts(2 hunks)apps/meteor/tests/end-to-end/api/abac.ts(4 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: KevLehman
PR: RocketChat/Rocket.Chat#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-10-24T17:32:05.348Z
Learnt from: KevLehman
PR: RocketChat/Rocket.Chat#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/tests/end-to-end/api/abac.ts
🧬 Code graph analysis (1)
apps/meteor/tests/end-to-end/api/abac.ts (2)
packages/core-typings/src/IAbacAttribute.ts (1)
IAbacAttributeDefinition(3-14)apps/meteor/tests/data/api-data.ts (2)
request(10-10)credentials(39-42)
🔇 Additional comments (4)
apps/meteor/app/invites/server/functions/validateInviteToken.ts (1)
4-5: LGTM on settings import.apps/meteor/tests/end-to-end/api/abac.ts (2)
2-5: Imports look good.
45-46: Connection lifecycle is fine.Also applies to: 59-62
apps/meteor/app/invites/server/functions/findOrCreateInvite.ts (1)
66-71: The review comment contains a factually incorrect assertion about HTTP status codes.The review claims that using
error-action-not-allowedwill yield HTTP 403 per ABAC-8 spec. However, the codebase shows:
All thrown
Meteor.Errorinstances (regardless of error code) are caught by a generic exception handler inApiClass.ts:failure()which hardcodesstatusCode: 400.To return 403, code must explicitly call
API.v1.forbidden()(line 378–380). This is not used forMeteor.Errorexceptions.Both
error-invalid-roomanderror-action-not-allowedthrown asMeteor.Errorreturn 400, not 403.The suggested change would not achieve the claimed 403 HTTP response. Additionally, the existing ABAC checks elsewhere in the codebase (e.g.,
saveRoomSettings.ts:73-74, 108-109) already useerror-action-not-allowedfor similar scenarios, showing no precedent for a special 403 requirement. The currenterror-invalid-roomusage is consistent with other "room not accessible" patterns and is validated by existing tests.Likely an incorrect or invalid review comment.
Proposed changes (including videos or screenshots)
Issue(s)
https://rocketchat.atlassian.net/browse/ABAC-8
Steps to test or reproduce
Further comments
Summary by CodeRabbit
New Features
Tests