Skip to content

fix: system user group permission error #7812

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
peslyn merged 1 commit into from
Nov 10, 2026
Merged

fix: system user group permission error #7812

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 13 additions & 4 deletions apps/users/views/system_user_group.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,10 @@ class SystemUserGroupView(APIView):
tags=["IAM/System User Group"],
)
@has_permissions(
PermissionConstants.SYSTEM_USER_GROUP_CREATE, PermissionConstants.SYSTEM_USER_GROUP_EDIT, RoleConstants.ADMIN
PermissionConstants.SYSTEM_USER_GROUP_CREATE,
PermissionConstants.SYSTEM_USER_GROUP_EDIT,
RoleConstants.ADMIN,
RoleConstants.WORKSPACE_MANAGE,
)
@log(
menu="IAM/System User Group",
Expand Down Expand Up @@ -100,7 +103,9 @@ class Delete(APIView):
responses=DefaultModelResponse.get_response(),
tags=["IAM/System User Group"],
)
@has_permissions(PermissionConstants.SYSTEM_USER_GROUP_DELETE, RoleConstants.ADMIN)
@has_permissions(
PermissionConstants.SYSTEM_USER_GROUP_DELETE, RoleConstants.ADMIN, RoleConstants.WORKSPACE_MANAGE
)
@log(
menu="IAM/System User Group",
operate="Delete System User Group",
Expand All @@ -126,7 +131,9 @@ class AddMember(APIView):
responses=DefaultModelResponse.get_response(),
tags=["IAM/System User Group"],
)
@has_permissions(PermissionConstants.SYSTEM_USER_GROUP_ADD_MEMBER, RoleConstants.ADMIN)
@has_permissions(
PermissionConstants.SYSTEM_USER_GROUP_ADD_MEMBER, RoleConstants.ADMIN, RoleConstants.WORKSPACE_MANAGE
)
@log(
menu="IAM/System User Group",
operate="Add members to System User Group",
Expand Down Expand Up @@ -155,7 +162,9 @@ class RemoveMember(APIView):
responses=DefaultModelResponse.get_response(),
tags=["IAM/System User Group"],
)
@has_permissions(PermissionConstants.SYSTEM_USER_GROUP_REMOVE_MEMBER, RoleConstants.ADMIN)
@has_permissions(
PermissionConstants.SYSTEM_USER_GROUP_REMOVE_MEMBER, RoleConstants.ADMIN, RoleConstants.WORKSPACE_MANAGE
)
@log(
menu="IAM/System User Group",
operate="Remove members from System User Group",

@shaohuzhang1 shaohuzhang1 Nov 10, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Main finding

The diff changes only authorization policy, but the added RoleConstants.WORKSPACE_MANAGE is a high-risk change.

If has_permissions(...) uses OR semantics — which is common — then any user with WORKSPACE_MANAGE can now:

  • create/edit system user groups
  • delete system user groups
  • add members to system user groups
  • remove members from system user groups

That is a potential vertical privilege escalation, especially because group membership changes can grant or remove access. A workspace-scoped role should not normally grant control over system-level IAM groups unless that is explicitly intended and enforced with object-level scoping.

If has_permissions(...) uses AND semantics, adding WORKSPACE_MANAGE may instead make these endpoints inaccessible to users who have ADMIN but not WORKSPACE_MANAGE. Either way, verify the decorator semantics and the intended policy.

Recommendations

  1. Confirm has_permissions semantics — OR vs AND — before merging.

  2. Avoid granting WORKSPACE_MANAGE to system-level group endpoints unless explicitly intended.

    • If workspace managers must manage only workspace-scoped groups, add object-level checks and queryset filtering.
    • Prefer a dedicated permission such as SYSTEM_USER_GROUP_*_WORKSPACE rather than mixing a workspace role into system IAM endpoints.

  3. Verify RoleConstants.WORKSPACE_MANAGE exists and is imported. If it does not exist, this will fail at import/runtime with AttributeError.

  4. Check the existing SystemUserGroupView decorator. It lists both SYSTEM_USER_GROUP_CREATE and SYSTEM_USER_GROUP_EDIT. If the decorator is OR-based, a user with only create permission can edit, and a user with only edit permission can create. Split permissions by HTTP method if possible.

  5. Add authorization tests for:

    • ADMIN
    • WORKSPACE_MANAGE
    • users with only individual SYSTEM_USER_GROUP_* permissions
    • users with no relevant permissions
    • expected 200/403 results

  6. Fix line length/formatting. The new one-line decorators are likely over 120 characters:

    • Delete line is approximately 123 chars
    • AddMember line is approximately 127 chars
    • RemoveMember line is approximately 130 chars

    Use the project formatter, e.g. Black/Ruff, and keep the decorator arguments consistently multiline.

  7. Consider audit logging. Since membership changes can affect access, ensure the existing @log captures actor, target group, and affected members.

Overall: the formatting changes are minor, but the authorization change needs explicit security review before merge.

Expand Down
Loading