CRITICAL: Add authorization checks to delete_user() function

[DogukanUrker]

Problem

The delete_user() function in app/utils/delete.py has NO authorization check. It accepts a username and immediately deletes the user without verifying:

• Whether the calling user is an admin
• Whether the calling user is deleting themselves
• Whether the user being deleted is an admin

The perpetrator_role variable is assigned but never checked. While admin_panel_users.py and account_settings.py currently check admin role before calling delete_user(), this is fragile — any new caller could bypass authorization.

Requirements

1. Add authorization check inside delete_user() itself:

   • Non-admin users should only be allowed to delete their own account
   • Admins should not be able to delete themselves
   • Return a boolean or appropriate response on failure (matching the pattern in delete_post() and delete_comment())

2. Add logging for unauthorized deletion attempts

3. Add unit tests covering:

   • Admin deleting another admin
   • Non-admin trying to delete another user
   • Successful self-deletion by non-admin
   • Successful deletion by admin (non-self)

[DogukanUrker]

🤖 AI-generated implementation plan — local LLM (ornith-35b) via Hermes Agent.
Not human-reviewed. Do NOT merge or act on this without human approval.

Problem

delete_user() in app/utils/delete.py:64 has no authorization check. It accepts a username parameter and immediately deletes the user from the database without verifying:

• Whether the calling user is an admin
• Whether the calling user is deleting themselves
• Whether the user being deleted is an admin

The perpetrator and perpetrator_role variables at app/utils/delete.py:82-83 are assigned but never checked — dead code. The function also returns redirect() instead of False like delete_post() (app/utils/delete.py:24) and delete_comment() (app/utils/delete.py:103), which is an inconsistent API.

Current state in main

• app/utils/delete.py:64-100 — delete_user() performs no auth checks. perpetrator and perpetrator_role are assigned but never used.
• app/routes/admin_panel_users.py:19 — decorated with @admin_required, so only admins reach this route.
• app/routes/account_settings.py:11 — decorated with @login_required, so only the logged-in user reaches this route.
• Two callers currently have decorator-level protection, but the function itself has no defense-in-depth. A new caller without the decorator would bypass authorization entirely.

Root cause

The authorization logic was placed in route decorators rather than inside delete_user() itself. This means:

1. The function has no self-contained security guarantee — it relies entirely on callers having the right decorator.
2. perpetrator and perpetrator_role at lines 82-83 were clearly intended for authorization but the check was never written.
3. The return type is inconsistent with delete_post() and delete_comment() (returns redirect() instead of bool).

Proposed fix

Refactor delete_user() in app/utils/delete.py to mirror the authorization pattern used by delete_post() and delete_comment():

1. Accept a username parameter for the calling user (not just the target).
2. Look up the perpetrator from session["username"].
3. Add authorization checks:
   • If perpetrator_role != "admin", only allow deletion of own account (perpetrator.username.lower() == target.username.lower()).
   • If the target is an admin, refuse deletion (admins cannot delete themselves, even via admin panel).
4. On failure: log the attempt via Log.error() and return False (matching delete_post/delete_comment pattern).
5. On success: delete user, flash message, log success, return True.
6. Update callers:
   • app/routes/admin_panel_users.py:29 — pass session["username"] as the second argument. The @admin_required decorator still applies at the route level.
   • app/routes/account_settings.py:19 — pass session["username"] as the second argument. The @login_required decorator still applies.

Files to change

• app/utils/delete.py — rewrite delete_user() to include authorization checks, change return type to bool, accept perpetrator_username parameter
• app/routes/admin_panel_users.py — update delete_user() call to pass session["username"]
• app/routes/account_settings.py — update delete_user() call to pass session["username"]

Test plan

Add to tests/e2e/admin/test_admin.py (or a new tests/e2e/auth/test_delete_user.py):

1. test_admin_cannot_delete_self — admin attempts to delete their own account via /admin/users; should be refused.
2. test_non_admin_cannot_delete_other_user — regular user crafts a forged POST to /admin/users with another user's username; should be refused.
3. test_admin_can_delete_other_user — admin deletes a regular user via /admin/users; should succeed.
4. test_user_can_delete_own_account — regular user deletes their own account via /account-settings; should succeed and clear session.
5. test_admin_cannot_delete_admin — admin attempts to delete another admin; should be refused.

Reference existing tests: tests/e2e/admin/test_admin.py:146 (existing test_admin_can_delete_user), tests/e2e/post/test_post.py:324 (pattern for forged-request auth tests).

Risks & edge cases

• Admin self-deletion via admin panel: The @admin_required decorator on /admin/users means an admin can reach the delete endpoint. The new check inside delete_user() must block admin self-deletion regardless of route.
• Session cleanup for self-deletion: When a non-admin deletes their own account, the session must be cleared (currently done at app/utils/delete.py:99). With the new bool return, the caller in account_settings.py needs to handle session clearing on success.
• Flash message on failure: Currently no flash message is set on unauthorized attempts. Should add one (e.g., "You are not authorized to delete this user") so the user gets feedback.
• Backward compatibility: The function signature changes from delete_user(username) to delete_user(target_username, perpetrator_username=None). Both callers are updated. No other callers exist in the codebase.

Out of scope

• Adding authorization to delete_post() or delete_comment() (they already have checks).
• Adding rate limiting or confirmation dialogs (UI-level concern).
• Adding audit logging to a database table (would require a new migration).
• Handling cascade deletion of posts/comments owned by the deleted user (handled by SQLAlchemy cascade="all, delete-orphan" on the User model).

[DogukanUrker]

🤖 AI-assisted unified plan — supersedes the separate plans on this issue and #260.
Not human-reviewed beyond author triage. Do not merge without approval.

Unifying #259 + #260 into one delete_user() refactor

#259 (authz checks) and #260 (last-admin guard) both rewrite the same
delete_user() function with conflicting self-delete rules. Implementing them
separately would produce contradictory PRs. Merging into one spec.

Core issue: delete_user() is called from two places with different intent —
admin deleting someone else (admin_panel_users.py) vs user deleting their own
account (account_settings.py) — and currently can't tell them apart.

Invariant that actually matters: admin count can never reach zero.

Two gates, in order:

1. Authz: allow only if is_self OR perpetrator.role == "admin". Else log + flash + return False.
2. Last-admin: if target.role == "admin" AND admin count == 1, refuse — regardless of caller or page.

If both pass: delete, flash, log, clear session if self, return bool.

Caller	Target	Allowed?
Admin	non-admin	yes
Admin	other admin, >=2 admins	yes
Admin	other admin, only 1 left	no
Admin (self)	self, >=2 admins	yes
Admin (self)	self, last admin	no
User	self	yes
User	anyone else	no

Corrections to the original plans:

• CRITICAL: Add authorization checks to delete_user() function #259's "refuse all admin deletion" is too broad — it breaks the legit
  "admin removes another admin" flow. Replaced with the last-admin count guard.
• Prevent deletion of the last admin account #260 allowed unconditional self-delete from account settings — that leaves a
  last-admin-self-delete lockout. The guard now applies to ALL paths.

Change surface (one PR):

• app/utils/delete.py — rewrite delete_user(target, perpetrator), both gates, return bool, log rejects.
• app/routes/admin_panel_users.py + app/routes/account_settings.py — pass session["username"], keep decorators.
• app/translations/*.json — add not_authorized + last_admin keys once, all 12 files.
• Tests: the matrix above is the test suite.

Out of scope: change_user_role() self-demotion lockout — same invariant, different door. Follow-up issue.

Line numbers in the original AI plans are unverified — confirm against the real file before coding.