Allow the group allowance to be set per admin, not only per role - #755
Closed
Free-Guy-IR wants to merge 2 commits into
Closed
Allow the group allowance to be set per admin, not only per role#755Free-Guy-IR wants to merge 2 commits into
Free-Guy-IR wants to merge 2 commits into
Conversation
RoleAccess.allowed_group_ids is applied in five places in the group
operations - listing, single fetch, bulk remove, bulk disable - but nothing
consults it on the path that actually assigns groups to a user. An admin whose
role restricts them to a subset of groups is therefore only restricted in the
UI: POST /api/user or PUT /api/user/{username} with any group_id succeeds, and
the resulting user is placed in a group that admin was never granted.
validate_all_groups is the single point every write path resolves groups
through - user create, user modify, bulk create from template, and the
Telegram handlers by way of those - so the check belongs there, and one check
covers all of them. It reads get_allowed_group_ids, the same function the
listing is filtered by, so what an admin can be shown and what they can
actually assign cannot drift apart. Owners are unaffected, and an admin with
no restriction is unaffected.
The admin argument is optional because a few callers resolve groups only in
order to render them; enforcement applies wherever an acting admin is passed.
Groups already present on the user being edited are exempt. Without that,
restricting a role would block its admins from saving *any* change to a user
who happens to sit in a group outside the allowance - not the groups, but the
data limit, the expiry, anything - because the edit form posts the user's
current groups back unchanged. The check should refuse the change being made,
not the state that was already there. Adding such a group is still refused,
including adding it back after removing it.
The group allowance currently lives on the role, so two admins sharing a role necessarily share it. Giving one reseller access to a different set of groups than another means creating a role per reseller, and the roles then differ in nothing but that list. This adds allowed_group_ids to the admin itself, editable from an "Inbound access" section in the admin dialog. Each group is listed with the inbounds it grants, because granting a group is really granting its inbounds, and that is the decision being made. It is folded into get_allowed_group_ids rather than run as a second mechanism beside the role's, so everything that already respects the allowance - the group listing, the single fetch, the bulk operations, and the check on user create and modify - picks it up with no further change. The two intersect: a per-admin list can narrow what the role grants but never widen it, so the role stays an upper bound on what its admins can reach. null means no per-admin narrowing, which is what every existing admin gets, so behaviour is unchanged until the field is set. Because null is meaningful here, update_admin goes by model_fields_set rather than the "is not None" test the neighbouring fields use - otherwise an allowance could be set but never cleared. The migration adds one nullable column and nothing else.
WalkthroughThe change adds nullable per-admin group restrictions, persists and exposes them through admin APIs, enforces them during user group workflows, and adds dashboard controls with localized labels. ChangesPer-admin group access
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant AdminModal
participant AdminAPI
participant BaseOperation
participant AdminDatabase
AdminModal->>AdminAPI: submit allowed_group_ids
AdminAPI->>AdminDatabase: create or update admin restriction
AdminAPI-->>AdminModal: return AdminDetails
BaseOperation->>AdminDatabase: read admin and role restrictions
AdminDatabase-->>BaseOperation: return effective group data
BaseOperation-->>AdminAPI: allow groups or return HTTP 403
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@dashboard/public/statics/locales/ru.json`:
- Around line 3547-3553: Translate the new inbound-access locale values in
dashboard/public/statics/locales/ru.json lines 3547-3553 into Russian and in
dashboard/public/statics/locales/zh.json lines 3602-3608 into Chinese,
preserving the existing keys and {{count}} placeholder.
In `@dashboard/src/features/admins/dialogs/admin-modal.tsx`:
- Around line 137-138: Update the group-loading logic around useGetAllGroups and
allGroups so it fetches and combines every page, continuing pagination until the
API indicates no more groups remain. Ensure the “Select all groups” handler uses
the complete aggregated group ID set, and only enable that action after all
pages have loaded.
🪄 Autofix
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 Plus
Run ID: 7217c317-2e7a-4e1d-9506-bb9b75235a07
📒 Files selected for processing (15)
app/db/crud/admin.pyapp/db/migrations/versions/d1f4a92c7e08_add_per_admin_allowed_group_ids.pyapp/db/models.pyapp/models/admin.pyapp/operation/__init__.pyapp/operation/permissions.pyapp/operation/user.pydashboard/public/statics/locales/en.jsondashboard/public/statics/locales/fa.jsondashboard/public/statics/locales/ru.jsondashboard/public/statics/locales/zh.jsondashboard/src/features/admins/dialogs/admin-modal.tsxdashboard/src/features/admins/forms/admin-form.tsdashboard/src/pages/_dashboard.admins.tsxdashboard/src/service/api/index.ts
Comment on lines
+3547
to
+3553
| "admins.groupAccess.title": "Inbound access", | ||
| "admins.groupAccess.all": "All", | ||
| "admins.groupAccess.description": "Choose which groups this admin may assign. Their users can only use the inbounds those groups contain.", | ||
| "admins.groupAccess.allowAll": "Access to all inbounds", | ||
| "admins.groupAccess.selectAll": "Select all groups", | ||
| "admins.groupAccess.noneSelected": "With no group selected, this admin's users get no inbounds at all.", | ||
| "admins.groupAccess.inboundCount": "{{count}} inbounds" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Translate the new Russian and Chinese locale entries.
These locale files display the new inbound-access controls in English.
dashboard/public/statics/locales/ru.json#L3547-L3553: replace the English values with Russian translations.dashboard/public/statics/locales/zh.json#L3602-L3608: replace the English values with Chinese translations.
📍 Affects 2 files
dashboard/public/statics/locales/ru.json#L3547-L3553(this comment)dashboard/public/statics/locales/zh.json#L3602-L3608
🤖 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 `@dashboard/public/statics/locales/ru.json` around lines 3547 - 3553, Translate
the new inbound-access locale values in dashboard/public/statics/locales/ru.json
lines 3547-3553 into Russian and in dashboard/public/statics/locales/zh.json
lines 3602-3608 into Chinese, preserving the existing keys and {{count}}
placeholder.
Comment on lines
+137
to
+138
| const { data: groupsData, isLoading: groupsLoading } = useGetAllGroups({ limit: 500 }, { query: { enabled: isDialogOpen } }) | ||
| const allGroups = useMemo(() => groupsData?.groups ?? [], [groupsData]) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the generated group endpoint for pagination parameters and response shape.
rg -n -C 6 'function getAllGroups|type GetAllGroupsParams|interface GetAllGroupsParams|limit|offset|page' dashboard/src/service/api/index.ts
# Find other dashboard callers that handle group pagination.
rg -n -C 5 'useGetAllGroups\s*\(' dashboard/srcRepository: PasarGuard/panel
Length of output: 37831
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the OpenAPI/generated API contract for the group response structure and any "get all groups" operation.
rg -n -C 8 '`@summary`.*[Gg]roup|get all|groups: |Get Groups|All Groups|GET /groups|offset|limit' dashboard/src/service/api/index.ts
# Locate GroupResponse and allowed_group types to see whether pagination metadata is part of the client contract.
rg -n -C 5 'interface Group|type Group|GetGroupsResponse|GetAllGroups|allowed_group_ids|groups:' dashboard/src/service/api/index.ts
# Inspect the relevant admin modal section where groups are selected and saved.
wc -l dashboard/src/features/admins/dialogs/admin-modal.tsx
sed -n '120,190p' dashboard/src/features/admins/dialogs/admin-modal.tsx
sed -n '740,795p' dashboard/src/features/admins/dialogs/admin-modal.tsxRepository: PasarGuard/panel
Length of output: 50372
🏁 Script executed:
#!/bin/bash
set -euo pipefail
wc -l dashboard/src/features/admins/dialogs/admin-modal.tsx
sed -n '120,190p' dashboard/src/features/admins/dialogs/admin-modal.tsx
sed -n '740,795p' dashboard/src/features/admins/dialogs/admin-modal.tsx
# Look for any helpers that paginate group queries or expose total/count metadata.
rg -n -C 4 'groupsData\.total|groupsData\.groups|useGetAllGroups|offset|total|setPages|fetchAll|page|fetchInfinite|disabled={!groupsLoading}' dashboard/src/features/admins/dashboard/src/features/admins/dialogs/admin-modal.tsxRepository: PasarGuard/panel
Length of output: 7653
Load every group before applying “Select all groups”.
useGetAllGroups({ limit: 500 }) returns only the first page, and the select-all handler saves only those IDs. If the deployment has more than 500 groups, this restricts the admin’s users to the first page and excludes later groups. Paginate until the API no longer returns more groups before enabling this action.
🤖 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 `@dashboard/src/features/admins/dialogs/admin-modal.tsx` around lines 137 -
138, Update the group-loading logic around useGetAllGroups and allGroups so it
fetches and combines every page, continuing pagination until the API indicates
no more groups remain. Ensure the “Select all groups” handler uses the complete
aggregated group ID set, and only enable that action after all pages have
loaded.
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
2 participants
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.
What
allowed_group_idscurrently lives on the role, so two admins sharing a role necessarily share the same group allowance. Giving one reseller access to a different set of groups than another means creating a role per reseller, and those roles then differ in nothing but that list.This adds the same allowance per admin, editable from an "Inbound access" section in the admin dialog.
How it composes with the role
It is folded into
get_allowed_group_idsrather than run as a mechanism beside the role's:So everything that already respects the allowance picks this up with no further change — the group listing, the single fetch, the bulk operations, and the check on user create/modify from #754.
The two intersect: a per-admin list can narrow what the role grants but never widen it, so a role remains an upper bound on what its admins can reach.
Backwards compatibility
nullmeans no per-admin narrowing, which is what every existing admin gets after the migration, so behaviour is unchanged until the field is deliberately set. The migration adds one nullable column and nothing else.Because
nullis meaningful here — it clears the restriction —update_admingoes bymodel_fields_setrather than theis not Nonetest the neighbouring fields use. Without that an allowance could be set but never cleared:UI
A new accordion section in the admin dialog, following the notification-filter section beside it — same count badge, same toggle-all row. A switch chooses between "access to all inbounds" (
null) and a selected list.Each group is shown with the inbounds it grants, since granting a group is really granting its inbounds and that is the decision being made. Selecting nothing is allowed but called out, because it leaves that admin's users with no inbounds.
The generated API client is included, hand-trimmed to only the three added fields — re-running the generator reorders ~290 unrelated lines, which seemed unhelpful to put in front of a reviewer. Happy to replace it with the full generator output if you would rather have that.
Testing
Verified against a running panel:
403, naming the group403201nullrestores the previous state exactly, nothing having been destroyedMigration applies and reverses cleanly on SQLite, and
alembic checkreports no drift from the models. The existing test suite passes (501 passed, 2 skipped), and the dashboard builds.Summary by CodeRabbit
New Features
Localization