Enforce the role group allowance when users are created and modified - #754
Enforce the role group allowance when users are created and modified#754Free-Guy-IR wants to merge 1 commit 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.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
WalkthroughThe operation layer now checks requested groups against an administrator’s allowed groups. User creation, modification, and bulk template creation pass administrator context to this validation. Existing user groups can remain through exemptions. ChangesAdmin group validation
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
What
RoleAccess.allowed_group_idsis not consulted when a user is created or modified, so an admin whose role restricts them to a subset of groups can still assign any group by calling the API directly.Why this looks like a gap rather than intended behaviour
The allowance is already enforced in
app/operation/group.pyin five places —get_all_groups,get_groups_simple,_get_group_with_access,bulk_remove_groups_by_id,bulk_set_groups_disabled— all viaapply_group_access. So a restricted admin cannot list, fetch, or bulk-modify a group outside their allowance.But
validate_all_groupsinapp/operation/__init__.py, which every user write path resolves groups through, only checks that the requested groups exist:The result is that the restriction holds in the dashboard, because the group never appears in the picker, but not against a direct request:
The change
validate_all_groupsnow takes an optional acting admin and appliesget_allowed_group_ids— the same function the listing is filtered by, so what an admin can be offered and what they can assign cannot drift apart. It is the single pointcreate_user,modify_userandbulk_create_users_from_templateall resolve groups through (and the Telegram handlers by way of those), so one check covers every write path.The argument is optional because a few callers resolve groups only in order to render them; enforcement applies wherever an acting admin is passed. Owners are unaffected, and an admin with no restriction is unaffected.
The exemption, and why it is needed
Groups already present on the user being edited are exempt from the check.
Without it, restricting a role would stop its admins from saving any edit to a user who happens to sit in a group outside the allowance — not just group changes, but the data limit, the expiry, anything — because the edit form loads the user's current groups and posts them back unchanged. The check would then be refusing the state that was already there rather than the change being made.
Adding such a group is still refused, including adding it back after removing it.
Testing
Verified against a running panel with a role limited to one group and an admin with no per-admin restriction:
POST /api/userwith a group outside the allowance →403 You are not allowed to use these groups: <name>403201The panel's existing test suite passes unchanged (501 passed, 2 skipped).
Summary by CodeRabbit