Skip to content

refactor: prevent deletion of the current user in user management #7848

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: prevent deletion of the current user in user management #7848

Show file tree
Hide file tree
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
14 changes: 10 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 @@ -626,11 +626,14 @@ def _check_user_exists(self):
raise AppApiException(1004, _("User does not exist"))

@transaction.atomic
def delete(self, with_valid=True):
def delete(self, current_user_id, with_valid=True):
if with_valid:
self.is_valid(raise_exception=True)
self._check_not_admin()
user_id = self.data.get("id")
if str(user_id) == str(current_user_id):
raise AppApiException(1004, _("Cannot delete current user"))
if with_valid:
self._check_not_admin()
# TODO 需要删除授权关系
User.objects.filter(id=user_id).delete()
return True
Expand Down Expand Up @@ -788,11 +791,14 @@ def get_user_members(self, workspace_id, nick_name=None):
class BatchDelete(serializers.Serializer):
ids = serializers.ListField(required=True, label=_("User IDs"))

def batch_delete(self, with_valid=True):
def batch_delete(self, current_user_id, with_valid=True):
user_ids = self.data.get("ids")
if not user_ids:
raise AppApiException(1004, _("User IDs cannot be empty"))
User.objects.filter(id__in=user_ids).exclude(id="f0dd8f71-e4ee-11ee-8c84-a8a1595801ab").delete()
# 批量删除时排除当前用户和系统内置用户
User.objects.filter(id__in=user_ids).exclude(
id__in=["f0dd8f71-e4ee-11ee-8c84-a8a1595801ab", str(current_user_id)]
).delete()
return True

def get_all_user_list(self, nick_name=None):

@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.

Code Review

Summary of Changes

  1. delete() now takes current_user_id and blocks self-deletion.
  2. batch_delete() now excludes both the built-in system user and the current user.

Issues & Suggestions

1. Security check ordering in delete()

The self-deletion check was moved before _check_not_admin(). This means a non-admin caller could potentially receive a different error than expected (e.g. "Cannot delete current user" before "not admin"), which is a minor information leak and behavior change. Consider keeping the admin authorization check first:

if with_valid:
    self.is_valid(raise_exception=True)
    self._check_not_admin()
user_id = self.data.get("id")
if str(user_id) == str(current_user_id):
    raise AppApiException(1004, _("Cannot delete current user"))

2. Reused error code 1004

AppApiException(1004, ...) is used for multiple distinct conditions ("User does not exist", "Cannot delete current user"). Consider using distinct error codes for better client-side handling and observability.

3. batch_delete() has critical gaps

  • No silence / feedback on skipped IDs: When current_user_id or the system user is included, they're silently skipped. Callers get True even though not all requested deletions happened. Consider returning deleted vs. skipped counts, or raising if the current user is in the list.
  • Missing validation / auth check: Unlike delete(), batch_delete() doesn't call is_valid() or _check_not_admin(). Even if guarded at the view layer, this is asymmetric and easy to regress — add an explicit authorization guard for defense in depth.
  • Hardcoded UUID: The system user ID "f0dd8f71-e4ee-11ee-8c84-a8a1595801ab" is a magic string. Extract it to a module constant (e.g. SYSTEM_USER_ID) so both delete() and batch_delete() share it — also note delete() currently does not protect the system user, only batch_delete() does. That's an inconsistency bug.
  • Simplification: str(current_user_id) inside id__in is fine, but building the exclusion list once improves readability.

Suggested refactor:

SYSTEM_USER_ID = "f0dd8f71-e4ee-11ee-8c84-a8a1595801ab"

def batch_delete(self, current_user_id, with_valid=True):
    if with_valid:
        self.is_valid(raise_exception=True)
        self._check_not_admin()
    user_ids = self.data.get("ids")
    if not user_ids:
        raise AppApiException(1004, _("User IDs cannot be empty"))
    excluded = {SYSTEM_USER_ID, str(current_user_id)}
    User.objects.filter(id__in=user_ids).exclude(id__in=excluded).delete()
    return True

And make delete() use the same SYSTEM_USER_ID guard.

4. Missing cascade cleanup

The existing # TODO 需要删除授权关系 (delete authorization relations) still applies. Both delete paths leave orphaned permission/workspace relations. Deletions should be wrapped consistently so relations are cleaned up; otherwise batch delete compounds the orphan issue.

5. Race condition on mass delete

filter(...).delete() is atomic per-Django, but there's no transaction decorator on BatchDelete.batch_delete (unlike delete's @transaction.atomic). If relation cleanup is added later, wrap it in transaction.atomic for consistency.

6. Callers must be updated

Both signatures changed (added a required positional param). Verify all call sites — views, serializers, tests, and any internal helpers — pass current_user_id. A missing arg would raise TypeError at runtime.

7. Type coercion

str(user_id) == str(current_user_id) handles UUID vs string mismatches, but it's cleaner to normalize IDs at input validation (self.is_valid) so downstream comparisons are type-safe.

Priority fixes: shared SYSTEM_USER_ID constant + apply system-user protection in delete() (#3), authorization guard in batch_delete (#3), and update all callers (#6).

Expand Down
12 changes: 10 additions & 2 deletions apps/users/views/user.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -277,7 +277,11 @@ class Operate(APIView):
get_operation_object=lambda r, k: get_user_operation_object(k.get("user_id")),
)
def delete(self, request: Request, user_id):
return result.success(UserManageSerializer.Operate(data={"id": user_id}).delete(with_valid=True))
return result.success(
UserManageSerializer.Operate(data={"id": user_id}).delete(
current_user_id=str(request.user.id), with_valid=True
)
)

@extend_schema(
methods=["GET"],
Expand Down Expand Up @@ -334,7 +338,11 @@ class BatchDelete(APIView):
get_operation_object=lambda r, k: get_user_operation_object(r.data.get("ids", [])),
)
def post(self, request: Request):
return result.success(UserManageSerializer.BatchDelete({"ids": request.data}).batch_delete(with_valid=True))
return result.success(
UserManageSerializer.BatchDelete({"ids": request.data}).batch_delete(
current_user_id=str(request.user.id), with_valid=True
)
)

class RePassword(APIView):
authentication_classes = [TokenAuth]

@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 review of the provided code patch:

1. Irregularities & Bugs

  • BatchDelete Data Parsing Issue:
    • In delete (single), the data is correctly constructed as {"id": user_id}.
    • In batch_delete, the code uses UserManageSerializer.BatchDelete({"ids": request.data}). It is highly likely that request.data for this endpoint is already a dictionary containing "ids" (e.g., {"ids": [...]}) or a list itself.
    • Risk: If request.data is {"ids": [1, 2]}, then {"ids": {"ids": [1, 2]}} is passed to the serializer, causing validation errors.
    • Fix: Check how request.data is structured. If it's a list, use {"ids": request.data}. If it's a dict with an ids key, pass request.data directly or extract request.data["ids"]. Assuming standard DRF behavior where a list is passed to a field expecting many, using {"ids": request.data} is only correct if request.data is a raw list. If the frontend sends JSON {"ids": [...]}, you must use request.data["ids"] or just request.data depending on the Serializer definition.

2. Potential Issues

  • Serialization/Validation Timing:
    • Both instances call .with_valid=True after instantiating the serializer but before calling .delete(). Ensure the underlying library supports chaining validity checks this way and that exceptions are raised before any destructive operation occurs.
  • Type Consistency (current_user_id):
    • str(request.user.id) is used. Ensure the Serializer.Operate.delete() and BatchDelete.batch_delete() methods explicitly expect a string for current_user_id. If they expect an int, this will cause type mismatch errors or unnecessary casting.

3. Optimization & Improvement Suggestions

  • Reduce Duplication:
    • The pattern result.success(Model.Action(...).method(...)) is repeated. Consider extracting this into a helper if more similar endpoints appear.
  • Explicit Data Construction in BatchDelete:
    • Instead of ambiguous {"ids": request.data}, explicitly handle the input structure.
  • Security/Logic Check:
    • Passing current_user_id suggests a check like "prevent deleting yourself" or "check permissions." Ensure this logic is indeed handled inside the Serializer method. The view layer should remain thin.

Suggested Corrected Code (Example for BatchDelete robustness)

# Assuming request.data is expected to contain 'ids' key OR is a list of ids
ids = request.data.get("ids") if isinstance(request.data, dict) else request.data

return result.success(
    UserManageSerializer.BatchDelete(data={"ids": ids}).batch_delete(
        current_user_id=str(request.user.id), 
        with_valid=True
    )
)

Summary of Actions

  1. Verify BatchDelete Input: Confirm if request.data is a list or a dict. The current code likely double-wraps the data.
  2. Check Type Expectations: Confirm if current_user_id needs to be str or int.
  3. Code Style: The expansion of long lines across multiple lines (PEP 8 compliance) is good for readability. Keep it.

Expand Down
Loading