core: group api#1168
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughDomainApi.groups argument schema now accepts domainId, uid, names, search, and limit. The resolver checks permissions and calls UserModel.listGroup(domainId, uid, names, search, args.limit ?? 20). UserModel.listGroup builds a MongoDB filter from domainId, optionally includes uid(s), name {$in: names}, and a case-insensitive escaped regex for search, applies limit when provided, and returns the matched groups. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install timed out. The project may have too many dependencies for the sandbox. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/hydrooj/src/handler/domain.ts`:
- Around line 447-455: The permission checks currently use ctx.user but the
resource being queried is args.domainId; update the code so authorization is
performed against the target domain's context before returning groups: obtain
the domain-specific user/authorization context for args.domainId (e.g., call a
helper like ctx.forDomain(args.domainId) / ctx.getDomainUser(args.domainId) or
use an existing domain authorization method), then call hasPerm/hasPriv on that
domain-scoped user instead of ctx.user, and only after that call user.listGroup,
user.searchGroups or user.listGroup with args.domainId; apply the same change to
the other similar block that calls user.listGroup/user.searchGroups to ensure
checks use the target domain context.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f4e0a869-3bfa-4dea-be58-969853e8808d
📒 Files selected for processing (2)
packages/hydrooj/src/handler/domain.tspackages/hydrooj/src/model/user.ts
| if (!ctx.user.hasPerm(PERM.PERM_VIEW) && !ctx.user.hasPriv(PRIV.PRIV_VIEW_ALL_DOMAIN)) throw new PermissionError(PERM.PERM_VIEW); | ||
| const groups = await user.listGroup(args.domainId); | ||
| if (args.names?.length) { | ||
| return groups.filter((g) => args.names.includes(g.name)); | ||
| return user.listGroup(args.domainId, undefined, args.names); | ||
| } | ||
| if (args.search) { | ||
| const searchLower = args.search.toLowerCase(); | ||
| return groups.filter((g) => g.name.toLowerCase().includes(searchLower)); | ||
| return user.searchGroups(args.domainId, args.search, args.limit ?? 10); | ||
| } | ||
| return groups; | ||
| return user.listGroup(args.domainId); | ||
| }, |
There was a problem hiding this comment.
Authorize using the target domain context before returning groups.
Line 447 and Line 463 validate permissions on ctx.user, but the queried resource is args.domainId. This can expose another domain’s groups/user-groups when a caller passes a different domainId.
🔒 Suggested fix
async (ctx, args) => {
- if (!ctx.user.hasPerm(PERM.PERM_VIEW) && !ctx.user.hasPriv(PRIV.PRIV_VIEW_ALL_DOMAIN)) throw new PermissionError(PERM.PERM_VIEW);
+ const targetUser = await user.getById(args.domainId, ctx.user._id);
+ if (!targetUser || (!targetUser.hasPerm(PERM.PERM_VIEW) && !targetUser.hasPriv(PRIV.PRIV_VIEW_ALL_DOMAIN))) {
+ throw new PermissionError(PERM.PERM_VIEW);
+ }
if (args.names?.length) {
return user.listGroup(args.domainId, undefined, args.names);
}
@@
async (ctx, args) => {
- if (!ctx.user.hasPerm(PERM.PERM_VIEW) && !ctx.user.hasPriv(PRIV.PRIV_VIEW_ALL_DOMAIN)) throw new PermissionError(PERM.PERM_VIEW);
+ const targetUser = await user.getById(args.domainId, ctx.user._id);
+ if (!targetUser || (!targetUser.hasPerm(PERM.PERM_VIEW) && !targetUser.hasPriv(PRIV.PRIV_VIEW_ALL_DOMAIN))) {
+ throw new PermissionError(PERM.PERM_VIEW);
+ }
return user.listGroup(args.domainId, args.uid);
},Also applies to: 463-465
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/hydrooj/src/handler/domain.ts` around lines 447 - 455, The
permission checks currently use ctx.user but the resource being queried is
args.domainId; update the code so authorization is performed against the target
domain's context before returning groups: obtain the domain-specific
user/authorization context for args.domainId (e.g., call a helper like
ctx.forDomain(args.domainId) / ctx.getDomainUser(args.domainId) or use an
existing domain authorization method), then call hasPerm/hasPriv on that
domain-scoped user instead of ctx.user, and only after that call user.listGroup,
user.searchGroups or user.listGroup with args.domainId; apply the same change to
the other similar block that calls user.listGroup/user.searchGroups to ensure
checks use the target domain context.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/hydrooj/src/model/user.ts (1)
490-494:⚠️ Potential issue | 🟠 Major | ⚡ Quick winApply the active filters and cap to the synthetic uid group too.
The fallback
{ name: uid.toString() }entry is appended after the Mongo query, sosearch,names, andlimitare not enforced on that item. For example, a filtered search can still return the uid group even when it does not match, and the final array can exceedlimit.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/hydrooj/src/model/user.ts` around lines 490 - 494, The synthetic fallback group currently unconditionally pushes an entry (using uid, groups, ObjectId, domainId, uids, name) after the Mongo query, bypassing the same search/names filtering and the limit cap; modify the logic so you only add this synthetic group if it passes the same filters (search/names predicates used for the query) and only if groups.length is still below the requested limit, i.e., evaluate the same match predicate(s) against the synthetic { _id: new ObjectId(), domainId, uids: [uid], name: uid.toString() } and push it only when it matches and groups.length < limit.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/hydrooj/src/model/user.ts`:
- Around line 485-486: The code overwrites filter.name when both names and
search are provided; update the logic so both constraints are applied instead of
dropped: if both names and search are present, set filter.$and = [{ name: { $in:
names } }, { name: { $regex: escapeRegExp(search), $options: 'i' } }], otherwise
keep the existing single-condition assignments (the identifiers to change are
filter, names, search, and escapeRegExp).
- Around line 487-488: The current check uses "if (limit)" so limit = 0 is
treated as no limit; update the code that builds the Mongo cursor (the
collGroup.find(filter) and subsequent .limit() call) to treat non-positive
values as invalid by checking if (typeof limit === 'number' && limit > 0) before
calling cursor.limit(limit), or clamp negative/zero to a safe minimum (e.g., 1)
or explicitly throw/bail; ensure the change references the same
variables/functions (collGroup.find, cursor, limit, .limit()) so callers cannot
supply 0 to create an unbounded query.
---
Outside diff comments:
In `@packages/hydrooj/src/model/user.ts`:
- Around line 490-494: The synthetic fallback group currently unconditionally
pushes an entry (using uid, groups, ObjectId, domainId, uids, name) after the
Mongo query, bypassing the same search/names filtering and the limit cap; modify
the logic so you only add this synthetic group if it passes the same filters
(search/names predicates used for the query) and only if groups.length is still
below the requested limit, i.e., evaluate the same match predicate(s) against
the synthetic { _id: new ObjectId(), domainId, uids: [uid], name: uid.toString()
} and push it only when it matches and groups.length < limit.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2e4d3d6e-8c44-4ba3-a433-93f571493446
📒 Files selected for processing (2)
packages/hydrooj/src/handler/domain.tspackages/hydrooj/src/model/user.ts
| let cursor = collGroup.find(filter); | ||
| if (limit) cursor = cursor.limit(limit); |
There was a problem hiding this comment.
Treat 0 as invalid instead of "no limit".
This truthy check makes limit = 0 skip .limit() entirely, so a caller can turn the new search path into an unbounded query. Reject or clamp non-positive values before building the cursor.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/hydrooj/src/model/user.ts` around lines 487 - 488, The current check
uses "if (limit)" so limit = 0 is treated as no limit; update the code that
builds the Mongo cursor (the collGroup.find(filter) and subsequent .limit()
call) to treat non-positive values as invalid by checking if (typeof limit ===
'number' && limit > 0) before calling cursor.limit(limit), or clamp
negative/zero to a safe minimum (e.g., 1) or explicitly throw/bail; ensure the
change references the same variables/functions (collGroup.find, cursor, limit,
.limit()) so callers cannot supply 0 to create an unbounded query.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/hydrooj/src/model/user.ts (1)
492-496:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftSynthetic group violates
limitand bypassesnames/searchfilters.The synthetic group is appended after the cursor limit is applied and doesn't respect the
namesorsearchfilters:
- Limit violation: When the database returns the maximum number of groups (up to
limit), appending the synthetic group causes the result to exceed the requested limit. For example, iflimit=20and the DB returns 20 groups, the caller receives 21 groups.- Filter bypass: When
namesorsearchare provided, the synthetic group (withname: uid.toString()) is appended regardless of whether it matches those filter criteria.Suggested fix
Option 1: Only append the synthetic group when no filters or limits are provided:
const groups = await cursor.toArray(); -if (uid) { +if (uid && !names && !search && !limit) { groups.push({ _id: new ObjectId(), domainId, uids: [uid], name: uid.toString(), }); }Option 2: Count the synthetic group toward the limit and validate it against filters:
const groups = await cursor.toArray(); if (uid) { + const syntheticName = uid.toString(); + const matchesFilters = (!names || names.includes(syntheticName)) + && (!search || syntheticName.match(new RegExp(escapeRegExp(search), 'i'))); + const withinLimit = !limit || groups.length < limit; + if (matchesFilters && withinLimit) { - groups.push({ - _id: new ObjectId(), domainId, uids: [uid], name: uid.toString(), - }); + groups.push({ + _id: new ObjectId(), domainId, uids: [uid], name: syntheticName, + }); + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/hydrooj/src/model/user.ts` around lines 492 - 496, The synthetic group appended in the block that creates an ObjectId for uid must respect the original query filters and the requested limit: change the logic around the groups.push({ _id: new ObjectId(), domainId, uids: [uid], name: uid.toString() }) so that you only append the synthetic group when it passes the same `names` and `search` predicates used to query the DB and when doing so will not exceed the caller's `limit` (i.e., consider the number of groups already returned and count the synthetic group toward the limit); alternatively apply the filters to the synthetic group's name first and append it only if (1) it matches and (2) currentResultCount < limit, ensuring `names`/`search` and `limit` are honored.
♻️ Duplicate comments (1)
packages/hydrooj/src/handler/domain.ts (1)
448-448:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAuthorize using the target domain context, not the current user's domain.
The permission check validates
ctx.user.hasPerm(PERM.PERM_VIEW), butctx.user's permissions are scoped toctx.domain(the current request domain), notargs.domainId(the target domain being queried). This allows a user to query groups from any domain they specify, bypassing domain-specific access controls.🔒 Recommended fix
Obtain the user's permissions in the target domain context before authorizing:
async (ctx, args) => { - if (!ctx.user.hasPerm(PERM.PERM_VIEW) && !ctx.user.hasPriv(PRIV.PRIV_VIEW_ALL_DOMAIN)) throw new PermissionError(PERM.PERM_VIEW); + const targetUser = await user.getById(args.domainId, ctx.user._id); + if (!targetUser || (!targetUser.hasPerm(PERM.PERM_VIEW) && !targetUser.hasPriv(PRIV.PRIV_VIEW_ALL_DOMAIN))) { + throw new PermissionError(PERM.PERM_VIEW); + } return user.listGroup(args.domainId, args.uid, args.names, args.search, args.limit ?? 20); },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/hydrooj/src/handler/domain.ts` at line 448, The current check uses ctx.user.hasPerm(PERM.PERM_VIEW) which checks permissions in ctx.domain (request domain) instead of the target args.domainId; change the authorization to evaluate the user's permissions in the target domain context (args.domainId) and keep the global privilege check. Concretely, replace the existing check with one that obtains the user's target-domain principal (e.g., something like const targetUser = ctx.user.forDomain(args.domainId) or otherwise call ctx.user.hasPerm(PERM.PERM_VIEW, args.domainId)) and then throw PermissionError if !targetUser.hasPerm(PERM.PERM_VIEW) && !ctx.user.hasPriv(PRIV.PRIV_VIEW_ALL_DOMAIN). Ensure you reference and use args.domainId and preserve the existing PRIV.PRIV_VIEW_ALL_DOMAIN check.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@packages/hydrooj/src/model/user.ts`:
- Around line 492-496: The synthetic group appended in the block that creates an
ObjectId for uid must respect the original query filters and the requested
limit: change the logic around the groups.push({ _id: new ObjectId(), domainId,
uids: [uid], name: uid.toString() }) so that you only append the synthetic group
when it passes the same `names` and `search` predicates used to query the DB and
when doing so will not exceed the caller's `limit` (i.e., consider the number of
groups already returned and count the synthetic group toward the limit);
alternatively apply the filters to the synthetic group's name first and append
it only if (1) it matches and (2) currentResultCount < limit, ensuring
`names`/`search` and `limit` are honored.
---
Duplicate comments:
In `@packages/hydrooj/src/handler/domain.ts`:
- Line 448: The current check uses ctx.user.hasPerm(PERM.PERM_VIEW) which checks
permissions in ctx.domain (request domain) instead of the target args.domainId;
change the authorization to evaluate the user's permissions in the target domain
context (args.domainId) and keep the global privilege check. Concretely, replace
the existing check with one that obtains the user's target-domain principal
(e.g., something like const targetUser = ctx.user.forDomain(args.domainId) or
otherwise call ctx.user.hasPerm(PERM.PERM_VIEW, args.domainId)) and then throw
PermissionError if !targetUser.hasPerm(PERM.PERM_VIEW) &&
!ctx.user.hasPriv(PRIV.PRIV_VIEW_ALL_DOMAIN). Ensure you reference and use
args.domainId and preserve the existing PRIV.PRIV_VIEW_ALL_DOMAIN check.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7dde73e4-c612-455e-aba7-17a81ee7f518
📒 Files selected for processing (2)
packages/hydrooj/src/handler/domain.tspackages/hydrooj/src/model/user.ts
2 participants
Summary by CodeRabbit
New Features
Improvements
Bug Fixes