Skip to content

refactor: simplify role visibility logic for non-admin users #7852

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
wxg0103 merged 1 commit into from
Nov 12, 2026
Merged

refactor: simplify role visibility logic for non-admin users #7852

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
8 changes: 4 additions & 4 deletions apps/users/serializers/user.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -465,9 +465,8 @@ def _get_user_roles(user_ids, is_admin=True):
user_id = str(relation.user_id)
role_id = relation.role_id
workspace_id = relation.workspace_id
# 非内置系统管理员只能看到除内置系统管理员之外的角色
is_builtin_admin_role = relation.role.internal and relation.role.type == RoleConstants.ADMIN.name
if not is_admin and is_builtin_admin_role:
# 非内置系统管理员只能看到系统角色类型（ADMIN）之外的角色
if not is_admin and relation.role.type == RoleConstants.ADMIN.name:
continue
user_role_mapping[user_id].add(relation.role.role_name)
if relation.role.type == RoleConstants.ADMIN.name:
Expand Down Expand Up @@ -827,7 +826,7 @@ def update_user_role(instance, user, user_id=None):
return

is_admin = workspace_user_role_mapping_model.objects.filter(
user_id=user_id, role_id=RoleConstants.ADMIN.name
user_id=user_id, role_id=RoleConstants.ADMIN.name, role__internal=True
).exists()

if str(user.id) == "f0dd8f71-e4ee-11ee-8c84-a8a1595801ab":
Expand Down Expand Up @@ -858,6 +857,7 @@ def update_user_role(instance, user, user_id=None):
if is_admin:
workspace_user_role_mapping_model.objects.filter(user_id=user.id).delete()
else:
# 非内置系统管理员看不到系统角色类型（ADMIN）的角色，保存时保留这些关系，避免被覆盖
workspace_user_role_mapping_model.objects.filter(user_id=user.id).exclude(
role__type=RoleConstants.ADMIN.name
).delete()

@shaohuzhang1 shaohuzhang1 Nov 12, 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 the code review, highlighting potential issues and optimization suggestions:

1. Logic Inconsistency & Potential Bug

Issue: The logic for filtering "ADMIN" roles is inconsistent between _get_user_roles and update_user_role.

  • In _get_user_roles: You previously filtered based on role.internal == True AND role.type == ADMIN. Now you filter based only on role.type == ADMIN. This means if there are non-internal roles with type ADMIN, they are now also hidden from non-admins. Ensure this change in behavior is intentional.
  • In update_user_role: The determination of is_admin checks if the user has a role that is ADMIN type AND internal=True. However, when saving (deleting) mappings, it removes any role where role__type != ADMIN.name.
    • Risk: If a user is not an internal admin but has a valid, assigned "Admin-like" role (type=ADMIN but not internal), is_admin will be False. The code will then delete mappings excluding role__type=ADMIN. This effectively preserves the non-internal admin role assignments. This seems correct based on your comment, but it relies heavily on the distinction between type and internal. Ensure your data model consistently enforces that only system admins use type=ADMIN or explicitly handle the case where custom roles exist with type=ADMIN.

2. N+1 Query Problem in _get_user_roles

Issue: The snippet shows accessing relation.role.internal and relation.role.type. If relation.role is accessed via a loop without select_related, this triggers a database query for every relation.
Suggestion: Ensure the queryset that generates relation uses .select_related('role').

3. Hardcoded User ID

Issue:

if str(user.id) == "f0dd8f71-e4ee-11ee-8c84-a8a1595801ab":

Using hardcoded UUIDs is bad practice. It makes the code brittle, difficult to maintain, and hard to understand why this specific user is treated differently.
Suggestion: Use a constant with a descriptive name (e.g., DEFAULT_SYSTEM_ADMIN_ID) or a configuration flag/setting. Ideally, determine superuser status via user.is_superuser or a similar generic flag unless this specific ID serves a very unique migration legacy purpose.

4. Ambiguous Naming / Magic Strings

Issue: RoleConstants.ADMIN.name likely evaluates to the string "ADMIN".

  • In _get_user_roles: relation.role.type == RoleConstants.ADMIN.name
  • In update_user_role: role_id=RoleConstants.ADMIN.name
    Question: Are you comparing role.type (usually a string category like 'BUILTIN' vs 'CUSTOM'?) with role.role_name?
    If role_id actually maps to the Role Identifier UUID or Name, ensure you are querying the correct field. Comparing role_id to a name string is suspicious unless role_id is genuinely stored as that string. If role_id is a ForeignKey ID, this query will fail or cause a type mismatch error. role__type is a safer check if you are looking for role category.

5. Race Condition in update_user_role

Issue:

if is_admin:
    ...delete()
else:
    ...exclude(...).delete()

Deleting all mappings for user.id can be dangerous if other concurrent processes are adding/updating roles. This is not atomic.
Suggestion: Instead of "Delete All Then Add," consider using bulk_create with ignore_conflicts or performing deletions based on precise IDs of the old mappings you are replacing. Alternatively, wrap the operation in a transaction.atomic() block with appropriate locking if concurrency is a concern.

6. Optimization Suggestion

In update_user_role, the workspace_user_role_mapping_model.objects.filter(...).exists() call to determine is_admin might be slightly inefficient if you need the full role object later. If you don't, .exists() is fine. However, ensure there are proper database indexes on (user_id, role_id) and (user_id, role__type) to speed up the filter/delete operations.

Summary of Recommended Actions:

  1. Confirm the hardcoded UUID usage intent; refactor if possible.
  2. Verify that role_id=RoleConstants.ADMIN.name targets the correct database column (ID vs Name).
  3. Add .select_related('role') to the queryset feeding into _get_user_roles.
  4. Double-check the semantic consistency of role.type filtering: does "ADMIN" strictly mean "System Builtin Admin" in your schema?

Expand Down
Loading