[shaohuzhang1]

refactor: prevent deletion of the current user in user management

[shaohuzhang1]

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

[shaohuzhang1]

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.