feat: people_admin role with group management permissions#144
Merged
Conversation
People admins can now create groups and manage members, managers, and exports across all groups. Legal officers gain group.export to retain access to the admin groups page. groups.view_all is replaced by a derived gate check across the three group-scoped actions, with group-scoped permissions now supporting an optional-scope pattern for gate checks without a specific group ID.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe PR removes the global ChangesPermission system: unscoped group authorization and groups.view_all removal
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 skipped: no ESLint configuration detected in root package.json. To enable, add Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/app/(authenticated)/(app)/(default)/admin/groups/layout.tsx (1)
11-15: ⚡ Quick winParallelize independent permission checks in the layout gate.
Line 11–15 performs three independent async calls sequentially, which adds unnecessary server latency for this route.
Proposed change
- if ( - !(await can("group.members.manage")) && - !(await can("group.managers.manage")) && - !(await can("group.export")) - ) { + const [canManageMembers, canManageManagers, canExport] = await Promise.all([ + can("group.members.manage"), + can("group.managers.manage"), + can("group.export"), + ]); + if (!canManageMembers && !canManageManagers && !canExport) { redirect("/membership"); }🤖 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 `@src/app/`(authenticated)/(app)/(default)/admin/groups/layout.tsx around lines 11 - 15, The three independent permission checks using can("group.members.manage"), can("group.managers.manage"), and can("group.export") are executed sequentially, causing extra latency; change the logic in layout.tsx to run these calls in parallel (e.g., via Promise.all or Promise.allSettled), await them together, destructure the boolean results, and then use the combined values in the existing conditional to decide access.src/app/(authenticated)/(app)/(default)/admin/people/[id]/groups-card.tsx (1)
17-20: ⚡ Quick winRun permission checks concurrently before loading groups.
Line 17–20 awaits independent checks one-by-one; use
Promise.allto reduce TTFB on this server component path.Proposed change
- const canViewGroups = - (await can("group.members.manage")) || - (await can("group.managers.manage")) || - (await can("group.export")); + const [canManageMembers, canManageManagers, canExport] = await Promise.all([ + can("group.members.manage"), + can("group.managers.manage"), + can("group.export"), + ]); + const canViewGroups = canManageMembers || canManageManagers || canExport;🤖 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 `@src/app/`(authenticated)/(app)/(default)/admin/people/[id]/groups-card.tsx around lines 17 - 20, The three sequential awaits when computing canViewGroups (await can("group.members.manage"), await can("group.managers.manage"), await can("group.export")) should be run concurrently; replace them with a Promise.all call to execute can(...) in parallel, then compute canViewGroups from the resulting boolean array (e.g., const [members, managers, exportPerm] = await Promise.all([...]) and set canViewGroups = members || managers || exportPerm) so the server component in groups-card.tsx performs permission checks concurrently.
🤖 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 `@src/lib/permissions/permissions.typecheck.ts`:
- Around line 30-32: The assertion calls evaluateAuth(authority,
"group.members.manage") without the required GroupScope even though evaluateAuth
currently requires a GroupScope for group-scoped actions; fix by either (A)
updating the call site to pass a proper GroupScope object (e.g.,
evaluateAuth(authority, "group.members.manage", { groupId: "<id>" } as
GroupScope)) so the runtime and types align, or (B) change evaluateAuth's
signature/implementation to accept an optional scope for group.* actions (adjust
the parameter type, add runtime handling when scope is undefined, and update any
type guards) so unscoped group permissions are permitted—choose one approach and
make the corresponding type and runtime changes around evaluateAuth and
GroupScope usage.
---
Nitpick comments:
In `@src/app/`(authenticated)/(app)/(default)/admin/groups/layout.tsx:
- Around line 11-15: The three independent permission checks using
can("group.members.manage"), can("group.managers.manage"), and
can("group.export") are executed sequentially, causing extra latency; change the
logic in layout.tsx to run these calls in parallel (e.g., via Promise.all or
Promise.allSettled), await them together, destructure the boolean results, and
then use the combined values in the existing conditional to decide access.
In `@src/app/`(authenticated)/(app)/(default)/admin/people/[id]/groups-card.tsx:
- Around line 17-20: The three sequential awaits when computing canViewGroups
(await can("group.members.manage"), await can("group.managers.manage"), await
can("group.export")) should be run concurrently; replace them with a Promise.all
call to execute can(...) in parallel, then compute canViewGroups from the
resulting boolean array (e.g., const [members, managers, exportPerm] = await
Promise.all([...]) and set canViewGroups = members || managers || exportPerm) so
the server component in groups-card.tsx performs permission checks concurrently.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 69569e39-2726-4a68-bca4-c4ccd1716d55
📒 Files selected for processing (8)
src/app/(authenticated)/(app)/(default)/admin/groups/layout.tsxsrc/app/(authenticated)/(app)/(default)/admin/people/[id]/groups-card.tsxsrc/components/can.tsxsrc/components/nav-main.tsxsrc/lib/permissions/evaluate.tssrc/lib/permissions/permissions.test.tssrc/lib/permissions/permissions.typecheck.tssrc/lib/permissions/server.ts
Contributor
Author
Fixes Applied SuccessfullyFixed 1 file based on 1 CodeRabbit feedback item. Files modified:
Commit: The fix corrects a typecheck assertion that called |
Contributor
|
Actionable comments posted: 0 |
1 task
This was referenced May 28, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
people_admingrant with permissions to create groups and manage members, managers, and exports across all groupsusers.create/users.importfrompeople_admin(admin-only)groups.view_allglobal action with a derived gate check usinggroup.members.manage || group.managers.manage || group.exportgroup.exportto legal officers so they retain access to the admin groups pageTest plan
people_adminuser can access the admin groups page and sees the nav itempeople_adminuser can create a group, add/remove members, appoint/remove managers, and exportpeople_adminuser cannot create or import usersnpm test— all 89 permission tests passSummary by CodeRabbit
Refactor
Tests