core: group api#1168
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughDomainApi.groups argument schema now accepts domainId, uid, names, search, and limit. The resolver checks permissions and calls UserModel.listGroup(domainId, uid, names, search, limit) — when search is provided and limit omitted it forwards limit=10; otherwise limit is not forwarded. UserModel.listGroup builds a MongoDB filter from domainId and conditionally includes uid(s), name {$in: names}, and a case-insensitive escaped regex for search, and applies limit when present. 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
packages/hydrooj/src/model/user.tsESLint skipped: missing config or dependency (missing-dependency). The ESLint configuration references a package that is not available in 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.
1 participant
Summary by CodeRabbit
New Features
Improvements
Bug Fixes