Skip to content

refactor: enhance user role management with admin role checks and batch processing improvements #7845

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: enhance user role management with admin role checks and batch processing improvements #7845

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
16 changes: 10 additions & 6 deletions apps/users/serializers/user.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -465,7 +465,9 @@ 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
if not is_admin and relation.role.type == RoleConstants.ADMIN.name:
# 非内置系统管理员只能看到除内置系统管理员之外的角色
is_builtin_admin_role = relation.role.internal and relation.role.type == RoleConstants.ADMIN.name
if not is_admin and is_builtin_admin_role:
continue
user_role_mapping[user_id].add(relation.role.role_name)
if relation.role.type == RoleConstants.ADMIN.name:
Expand Down Expand Up @@ -504,7 +506,9 @@ def _get_user_roles(user_ids, is_admin=True):
if role_model and user_role_relation_model:
# 获取当前用户的所有角色 判断是不是内置的系统管理员
is_admin = user_role_relation_model.objects.filter(
user_id=user_id, role__type=RoleConstants.ADMIN.name
user_id=user_id,
role__type=RoleConstants.ADMIN.name,
role__internal=True,
).exists()
user_ids = [user["id"] for user in result["records"]]
user_role_mapping, user_role_setting_mapping, user_role_workspace_mapping = _get_user_roles(
Expand All @@ -513,10 +517,10 @@ def _get_user_roles(user_ids, is_admin=True):

# 将角色信息添加回用户数据中
for user in result["records"]:
user_id = str(user["id"])
user["role_name"] = user_role_mapping.get(user_id, [])
user["role_setting"] = user_role_setting_mapping.get(user_id, [])
user["role_workspace"] = user_role_workspace_mapping.get(user_id, [])
record_user_id = str(user["id"])
user["role_name"] = user_role_mapping.get(record_user_id, [])
user["role_setting"] = user_role_setting_mapping.get(record_user_id, [])
user["role_workspace"] = user_role_workspace_mapping.get(record_user_id, [])

# 用户设置用户组
return result

@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 are my observations and suggestions:

1. Correctness / Logic Issues

a) Inconsistent is_admin semantics between the two functions

  • In _get_user_roles, is_admin is a parameter that means "the caller is an admin" (controls filtering).
  • At line ~506, is_admin is reassigned to mean "this particular user is a builtin admin" — then passed into _get_user_roles as the first function's is_admin param.

This works only if the two meanings happen to align (i.e., you're processing a list of users and use one user's admin status to decide visibility for all of them). But since user_ids here is a list of many users, checking is_admin for a single user_id and using it to control visibility for the whole batch is a latent bug:

is_admin = user_role_relation_model.objects.filter(user_id=user_id, ...).exists()
...
_get_user_roles(user_ids, is_admin=is_admin)
  • Problem: _get_user_roles will suppress builtin-admin roles for all users in the batch if the single user_id isn't a builtin admin — meaning actual admins in the list will show no admin role.
  • Fix: Filter per-user inside _get_user_roles, or compute a set of admin user IDs and pass that in, e.g.: 
    admin_user_ids = set(user_role_relation_model.objects.filter(
    user_id__in=user_ids,
    role__type=RoleConstants.ADMIN.name,
    role__internal=True,
).values_list("user_id", flat=True))
    Then inside the loop: if not is_admin and str(relation.user_id) not in admin_user_ids and is_builtin_admin_role: continue.

b) role.internal vs role__internal

  • The added code assumes a field named internal exists on the Role model. Verify the attribute name matches your schema (internal, is_internal, etc.). If it's is_internal, both places need updating.

c) relation.role.internal may cause N+1 queries

  • In the loop, relation.role.internal and relation.role.type each trigger lazy loads unless you had .select_related("role"). Confirm _get_user_roles uses select_related("role"); otherwise adding another relation.role.X access increases query count. Prefer prefetch_related/select_related.

2. Variable Shadowing (fixed, but note context)

The rename user_id → record_user_id in the final block is good cleanup — but double-check there wasn't previously intended reuse. Note the outer user_id now retains the admin-check value. Consider renaming the outer one too (e.g., current_user_id / request_user_id) to avoid confusion with the loop variable user["id"].

3. Style / Minor

  • Comment says "非内置系统管理员只能看到除内置系统管理员之外的角色" — good, but make sure the condition matches: it skips if not is_admin AND is_builtin_admin_role. That's "non-admin callers don't see builtin-admin roles." Consistent. ✅
  • Consider extracting is_builtin_admin_role logic into a helper/mixin (role.is_builtin_admin()) since it's repeated conceptually in both places (DB filter + object check). This avoids drift if RoleConstants changes.

4. Optimization

  • Replace the single-user .exists() query with the batched values_list(...) query above — fewer queries and fixes the batching bug simultaneously.
  • Ensure both role__type and role__internal filters have DB indexes (or a composite index), especially if user_role_relation_model is large.

Summary of key fix

The main irregularity is using a single user's is_admin to gate role visibility for a batch of users. Make admin determination per-user (pass a set of admin IDs), and verify role.internal is the correct field name and that role is select_related to avoid N+1.

Expand Down
Loading