Skip to content

fix: number of members who clicked on the resource authorization #7917

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 13, 2026
Merged

fix: number of members who clicked on the resource authorization #7917

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
4 changes: 4 additions & 0 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 @@ -206,6 +206,10 @@ class UserList(APIView):
PermissionConstants.SYSTEM_USER_GROUP_READ,
RoleConstants.ADMIN,
RoleConstants.WORKSPACE_MANAGE.get_workspace_role(),
RoleConstants.EXTENDS_ADMIN,
RoleConstants.EXTENDS_WORKSPACE_MANAGE.get_workspace_role(),
RoleConstants.USER.get_workspace_role(),
RoleConstants.EXTENDS_USER.get_workspace_role(),
)
def get(self, request: Request, workspace_id: str, user_group_id: str, current_page: int, page_size: int):
d = UserGroupListPageSerializer.Query(

@shaohuzhang1 shaohuzhang1 Nov 13, 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.

Here is an analysis of the code snippet, focusing on irregularities, potential issues and optimization suggestions.

1. Security & Permission Model Concern (Critical)

The most significant issue is likely a privilege escalation or unintended permission widening.

  • Problem: You are adding RoleConstants.USER.get_workspace_role() and RoleConstants.EXTENDS_USER.get_workspace_role() to the list of allowed roles for accessing UserList.
  • Risk:
    • Typically, listing all users in a group (especially an admin/system-level user group) should not be accessible by regular USER roles.
    • If this endpoint exposes sensitive user details (email, phone, department, internal IDs), granting read access to all regular users violates the principle of least privilege and could lead to data leakage or reconnaissance attacks.
  • Question: Was this intentional? If this endpoint is meant to only return non-sensitive metadata or if there’s another layer of filtering inside get(), it might be acceptable. Otherwise, this is a serious security flaw.

2. Code Consistency & Readability

  • Mixed Role Sources: The list mixes direct role constants (PermissionConstants, RoleConstants.ADMIN) with method-call based workspace roles (.get_workspace_role()). This is consistent within itself, but ensure that EXTENDS_ADMIN and EXTENDS_WORKSPACE_MANAGE also follow the same pattern (i.e., whether they need .get_workspace_role() or if they are already role strings). The diff implies they do not call .get_workspace_role(), while the new ones do. Verify this symmetry.

3. Potential Redundancy

  • Overlap Check:
    • RoleConstants.EXTENDS_ADMIN vs RoleConstants.ADMIN
    • RoleConstants.EXTENDS_WORKSPACE_MANAGE vs RoleConstants.WORKSPACE_MANAGE
    • RoleConstants.EXTENDS_USER vs RoleConstants.USER
  • If EXTENDS_* roles are supersets or subsets of the base roles, consider using a single source of truth or a helper utility to determine permissions to avoid maintaining parallel lists that can drift out of sync.

4. Naming Conventions & Type Checking

  • Ensure that RoleConstants.EXTENDS_ADMIN etc. return compatible types. If ADMIN returns a string and EXTENDS_ADMIN returns an enum object, the permission decorator may fail silently or raise runtime errors depending on implementation.
  • Add type annotations to ensure all entries in the permission tuple are consistent (e.g., Sequence[str] or Iterable[Callable]).

5. Optimization Suggestions

  • Use a Role Grouping Utility: If this pattern of "Admin + Workspace Manage + User" repeats across many endpoints, extract it into a constant or helper function: 
    USER_READABLE_ROLES = (
    RoleConstants.EXTENDS_ADMIN,
    RoleConstants.EXTENDS_WORKSPACE_MANAGE.get_workspace_role(),
    RoleConstants.USER.get_workspace_role(),
    RoleConstants.EXTENDS_USER.get_workspace_role(),
)
    Then use: 
    @permission_required(
    PermissionConstants.SYSTEM_USER_GROUP_READ,
    *USER_READABLE_ROLES,
)

Summary of Recommendations

Area Issue Recommendation
Security Granting USER role access to list user group members ✅ Verify intent. If sensitive data is exposed, revert addition of USER roles. Consider if EXTENDS_USER alone was sufficient.
Consistency Mixed usage of .get_workspace_role() ✅ Confirm all "workspace-scoped" roles consistently use .get_workspace_role(). Ensure EXTENDS_ADMIN doesn't need it if ADMIN didn’t.
Maintainability Hardcoded role lists per endpoint ✅ Extract common role combinations into reusable constants/functions.
Documentation Lack of clarity on why EXTENDS_* exist ✅ Add comments explaining the difference between base and extends roles, especially regarding permission inheritance.

Suggested Revised Code (if USER access is truly intended and safe):

@permission_required(
    PermissionConstants.SYSTEM_USER_GROUP_READ,
    RoleConstants.ADMIN,
    RoleConstants.WORKSPACE_MANAGE.get_workspace_role(),
    RoleConstants.EXTENDS_ADMIN,
    RoleConstants.EXTENDS_WORKSPACE_MANAGE.get_workspace_role(),
    # ONLY INCLUDE BELOW IF REGULAR USERS SHOULD SEE THE LIST
    RoleConstants.USER.get_workspace_role(),
    RoleConstants.EXTENDS_USER.get_workspace_role(),
)
def get(self, request: Request, workspace_id: str, user_group_id: str, current_page: int, page_size: int):
    ...

⚠️ If regular users should NOT see other users, remove these two lines:

RoleConstants.USER.get_workspace_role(),
RoleConstants.EXTENDS_USER.get_workspace_role(),

Expand Down
Loading