diff --git a/app/routes/account_settings.py b/app/routes/account_settings.py index f89ba9935..31c11b2d7 100755 --- a/app/routes/account_settings.py +++ b/app/routes/account_settings.py @@ -16,8 +16,9 @@ def account_settings(): return redirect("/") if request.method == "POST": - delete_user(user.username) - return redirect("/") + result = delete_user(user.username, session["username"]) + if result: + return redirect("/") return render_template( "account_settings.html", diff --git a/app/routes/admin_panel_users.py b/app/routes/admin_panel_users.py index 12bf8c378..a292e3855 100755 --- a/app/routes/admin_panel_users.py +++ b/app/routes/admin_panel_users.py @@ -26,7 +26,12 @@ def admin_panel_users(): f"Admin: {session['username']} deleted user: {request.form['username']}" ) - delete_user(request.form["username"]) + result = delete_user(request.form["username"], session["username"]) + if not result: + Log.warning( + f"Admin: {session['username']} — delete_user rejected for " + f"{request.form['username']}" + ) if "user_role_change_button" in request.form: Log.info( diff --git a/app/translations/de.json b/app/translations/de.json index 5ce13f2b3..743a902a6 100644 --- a/app/translations/de.json +++ b/app/translations/de.json @@ -266,7 +266,9 @@ "delete": { "user": "Benutzer wurde gelöscht.", "post": "Beitrag wurde gelöscht.", - "comment": "Kommentar wurde gelöscht." + "comment": "Kommentar wurde gelöscht.", + "not_authorized": "Sie sind nicht berechtigt, diesen Benutzer zu löschen", + "last_admin": "Diesen Benutzer kann nicht gelöscht werden: Er ist der letzte Administrator im System" }, "set_language": { "success": "Sprache wurde geändert." diff --git a/app/translations/en.json b/app/translations/en.json index 0d977d0ec..54e9421fb 100644 --- a/app/translations/en.json +++ b/app/translations/en.json @@ -266,7 +266,9 @@ "delete": { "user": "User has been deleted.", "post": "Post has been deleted.", - "comment": "Comment has been deleted." + "comment": "Comment has been deleted.", + "not_authorized": "You are not authorized to delete this user.", + "last_admin": "Cannot delete the last administrator account." }, "set_language": { "success": "Language has been changed." diff --git a/app/translations/es.json b/app/translations/es.json index d1cbdfb25..4344e6f80 100644 --- a/app/translations/es.json +++ b/app/translations/es.json @@ -266,7 +266,9 @@ "delete": { "user": "El usuario ha sido eliminado.", "post": "La publicación ha sido eliminada.", - "comment": "El comentario ha sido eliminado." + "comment": "El comentario ha sido eliminado.", + "not_authorized": "No tiene autorización para eliminar este usuario", + "last_admin": "No se puede eliminar este usuario: es el último administrador en el sistema" }, "set_language": { "success": "El idioma ha sido cambiado." diff --git a/app/translations/fr.json b/app/translations/fr.json index b8f24a165..61c454920 100644 --- a/app/translations/fr.json +++ b/app/translations/fr.json @@ -266,7 +266,9 @@ "delete": { "user": "L'utilisateur a été supprimé.", "post": "L'article a été supprimé.", - "comment": "Le commentaire a été supprimé." + "comment": "Le commentaire a été supprimé.", + "not_authorized": "Vous n'êtes pas autorisé à supprimer cet utilisateur", + "last_admin": "Impossible de supprimer cet utilisateur : c'est le dernier administrateur du système" }, "set_language": { "success": "La langue a été modifiée." diff --git a/app/translations/hi.json b/app/translations/hi.json index 82d798aed..ca13309fd 100644 --- a/app/translations/hi.json +++ b/app/translations/hi.json @@ -266,7 +266,9 @@ "delete": { "user": "उपयोगकर्ता हटा दिया गया है।", "post": "पोस्ट हटा दी गई है।", - "comment": "टिप्पणी हटा दी गई है।" + "comment": "टिप्पणी हटा दी गई है।", + "not_authorized": "आप इस उपयोगकर्ता को हटाने के लिए अनुमतित नहीं हैं", + "last_admin": "इस उपयोगकर्ता को हटाया नहीं जा सकता: यह तंत्र का अंतिम व्यवसायक है" }, "set_language": { "success": "भाषा सफलतापूर्वक बदल दी गई है।" diff --git a/app/translations/ja.json b/app/translations/ja.json index cb7c8826e..7f5ad13b7 100644 --- a/app/translations/ja.json +++ b/app/translations/ja.json @@ -266,7 +266,9 @@ "delete": { "user": "ユーザーが削除されました。", "post": "投稿が削除されました。", - "comment": "コメントが削除されました。" + "comment": "コメントが削除されました。", + "not_authorized": "このユーザーを削除する権限がありません", + "last_admin": "このユーザーを削除できません：システム最後の管理者です" }, "set_language": { "success": "言語が変更されました。" diff --git a/app/translations/pl.json b/app/translations/pl.json index db51d3ed0..9f22570bc 100644 --- a/app/translations/pl.json +++ b/app/translations/pl.json @@ -266,7 +266,9 @@ "delete": { "user": "Użytkownik został usunięty.", "post": "Post został usunięty.", - "comment": "Komentarz został usunięty." + "comment": "Komentarz został usunięty.", + "not_authorized": "Nie masz uprawnień do usunięcia tego użytkownika", + "last_admin": "Nie można usunąć tego użytkownika: jest ostatnim administratorem w systemie" }, "set_language": { "success": "Język został zmieniony." diff --git a/app/translations/pt.json b/app/translations/pt.json index 3991b4252..ab62cdeb7 100644 --- a/app/translations/pt.json +++ b/app/translations/pt.json @@ -266,7 +266,9 @@ "delete": { "user": "Usuário foi excluído.", "post": "Postagem foi excluída.", - "comment": "Comentário foi excluído." + "comment": "Comentário foi excluído.", + "not_authorized": "Você não está autorizado a excluir este usuário", + "last_admin": "Não é possível excluir este usuário: é o último administrador do sistema" }, "set_language": { "success": "O idioma foi alterado." diff --git a/app/translations/ru.json b/app/translations/ru.json index cb53559a2..3e94ab8b1 100644 --- a/app/translations/ru.json +++ b/app/translations/ru.json @@ -266,7 +266,9 @@ "delete": { "user": "Пользователь был удален.", "post": "Пост был удален.", - "comment": "Комментарий был удален." + "comment": "Комментарий был удален.", + "not_authorized": "У вас нет прав для удаления этого пользователя", + "last_admin": "Невозможно удалить этого пользователя: это последний администратор в системе" }, "set_language": { "success": "Язык был изменен." diff --git a/app/translations/tr.json b/app/translations/tr.json index 72d28ffaa..66199eee9 100644 --- a/app/translations/tr.json +++ b/app/translations/tr.json @@ -266,7 +266,9 @@ "delete": { "user": "Kullanıcı silindi.", "post": "Gönderi silindi.", - "comment": "Yorum silindi." + "comment": "Yorum silindi.", + "not_authorized": "Bu kullanıcıyı silme yetkiniz yok", + "last_admin": "Bu kullanıcı silinemez: sistemdeki son yöneticidir" }, "set_language": { "success": "Dil değiştirildi." diff --git a/app/translations/uk.json b/app/translations/uk.json index 7ffcdba8c..1ffefbb8b 100644 --- a/app/translations/uk.json +++ b/app/translations/uk.json @@ -266,7 +266,9 @@ "delete": { "user": "Користувача було видалено.", "post": "Пост було видалено.", - "comment": "Коментар було видалено." + "comment": "Коментар було видалено.", + "not_authorized": "У вас немає прав для видалення цього користувача", + "last_admin": "Неможливо видалити цього користувача: це останній адміністратор у системі" }, "set_language": { "success": "Мову було змінено." diff --git a/app/translations/zh.json b/app/translations/zh.json index 2de9633f9..0a5f4a5b5 100644 --- a/app/translations/zh.json +++ b/app/translations/zh.json @@ -265,7 +265,9 @@ "delete": { "user": "用户已删除。", "post": "帖子已删除。", - "comment": "评论已删除。" + "comment": "评论已删除。", + "not_authorized": "您沒有權限刪除此用戶", + "last_admin": "無法刪除此用戶：他是系統中最後的管理員" }, "set_language": { "success": "语言已更改。" diff --git a/app/utils/delete.py b/app/utils/delete.py index 4b45da5de..350d17942 100644 --- a/app/utils/delete.py +++ b/app/utils/delete.py @@ -61,28 +61,81 @@ def delete_post(post_id, username=None): return True -def delete_user(username): +def delete_user(target_username, perpetrator_username=None): """ This function deletes a user and all associated data from the database. + Authorization rules: + - Non-admin users may only delete their own account. + - Admins may delete any non-admin user. + - Admins may delete another admin only if >= 2 admins remain after deletion. + - No user (admin or not) may delete themselves via the admin panel + (admin self-deletion is blocked at the account-settings page). + Parameters: - username (str): The username of the user to be deleted. + target_username (str): The username of the user to be deleted. + perpetrator_username (str, optional): The username of the calling user, + taken from session["username"]. If None, falls back to session. Returns: - None + bool: True if deleted, False if unauthorized, not found, or blocked. """ from sqlalchemy import func - user = User.query.filter(func.lower(User.username) == username.lower()).first() + if perpetrator_username is None: + perpetrator_username = session.get("username") - if not user: - Log.error(f'User: "{username}" not found') - return redirect("/") + target = User.query.filter( + func.lower(User.username) == target_username.lower() + ).first() + + if not target: + Log.error(f'User: "{target_username}" not found') + return False - perpetrator = User.query.filter_by(username=session["username"]).first() - perpetrator_role = perpetrator.role if perpetrator else None + perpetrator = User.query.filter_by(username=perpetrator_username).first() + if not perpetrator: + Log.error(f'Perpetrator: "{perpetrator_username}" not found') + return False + + # Gate 1: Authorization + is_admin = perpetrator.role == "admin" + is_self = target.username.lower() == perpetrator.username.lower() + + if not is_admin and not is_self: + Log.error( + f'User: "{perpetrator_username}" (role={perpetrator.role}) ' + f'tried to delete user: "{target_username}" without authorization' + ) + flash_message( + page="delete", + message="not_authorized", + category="error", + language=session.get("language", "en"), + ) + return False - db.session.delete(user) + # Gate 2: Last-admin guard — never let admin count drop to zero + if target.role == "admin": + admin_count = ( + db.session.query(func.count(User.user_id)) + .filter(User.role == "admin") + .scalar() + ) + if admin_count <= 1: + Log.error( + f'User: "{perpetrator_username}" tried to delete last admin: ' + f'"{target_username}" — would leave no admins' + ) + flash_message( + page="delete", + message="last_admin", + category="error", + language=session.get("language", "en"), + ) + return False + + db.session.delete(target) db.session.commit() flash_message( @@ -91,13 +144,13 @@ def delete_user(username): category="error", language=session.get("language", "en"), ) - Log.success(f'User: "{username}" deleted') + Log.success(f'User: "{target.username}" deleted by "{perpetrator.username}"') - if perpetrator_role == "admin": - return redirect("/admin/users") - else: + if is_self: session.clear() return redirect("/") + else: + return redirect("/admin/users") def delete_comment(comment_id, username=None): diff --git a/tests/e2e/auth/test_delete_user_auth.py b/tests/e2e/auth/test_delete_user_auth.py new file mode 100644 index 000000000..5b1dbfec9 --- /dev/null +++ b/tests/e2e/auth/test_delete_user_auth.py @@ -0,0 +1,260 @@ +""" +E2E tests for delete_user() authorization checks. + +Covers the authorization matrix: +- Admin deleting non-admin +- Admin deleting another admin (with >= 2 admins) +- Admin deleting last admin (blocked) +- Non-admin deleting own account +- Non-admin trying to delete another user (blocked) +""" + +import uuid + +import pytest + +from tests.e2e.helpers.database_helpers import ( + create_test_user, + get_user_by_username, +) +from tests.e2e.pages.login_page import LoginPage + + +def _suffix() -> str: + return uuid.uuid4().hex[:8] + + +def _login(page, flask_server, username: str, password: str): + login_page = LoginPage(page, flask_server["base_url"]) + login_page.navigate("/login/redirect=&") + login_page.login(username, password) + page.wait_for_url("**/", timeout=5000) + + +class TestDeleteUserAuth: + """Tests for delete_user() authorization checks.""" + + @pytest.mark.admin + def test_admin_can_delete_other_user( + self, page, flask_server, app_settings, db_path + ): + """Admin should be able to delete a regular user.""" + seed = _suffix() + username = f"deladmin{seed}" + create_test_user( + db_path=str(db_path), + username=username, + email=f"{username}@test.com", + password="TestPassword123!", + role="user", + ) + + _login( + page, + flask_server, + app_settings["default_admin"]["username"], + app_settings["default_admin"]["password"], + ) + + page.goto( + f"{flask_server['base_url']}/admin/users", wait_until="domcontentloaded" + ) + csrf_token = page.locator('input[name="csrf_token"]').first.get_attribute( + "value" + ) + + response = page.request.post( + f"{flask_server['base_url']}/admin/users", + form={ + "csrf_token": csrf_token, + "username": username, + "user_delete_button": "1", + }, + ) + assert response.ok + + deleted_user = get_user_by_username(str(db_path), username) + assert deleted_user is None, "Deleted user should no longer exist in database" + + @pytest.mark.admin + def test_admin_can_delete_other_admin( + self, page, flask_server, app_settings, db_path + ): + """Admin should be able to delete another admin if >= 2 admins remain.""" + seed = _suffix() + username = f"admin_del{seed}" + # Create a second admin + create_test_user( + db_path=str(db_path), + username=username, + email=f"{username}@test.com", + password="TestPassword123!", + role="admin", + ) + + _login( + page, + flask_server, + app_settings["default_admin"]["username"], + app_settings["default_admin"]["password"], + ) + + page.goto( + f"{flask_server['base_url']}/admin/users", wait_until="domcontentloaded" + ) + csrf_token = page.locator('input[name="csrf_token"]').first.get_attribute( + "value" + ) + + response = page.request.post( + f"{flask_server['base_url']}/admin/users", + form={ + "csrf_token": csrf_token, + "username": username, + "user_delete_button": "1", + }, + ) + assert response.ok + + deleted_user = get_user_by_username(str(db_path), username) + assert deleted_user is None, "Deleted admin should no longer exist in database" + + @pytest.mark.admin + def test_admin_cannot_delete_last_admin( + self, page, flask_server, app_settings, db_path + ): + """Admin cannot delete the last admin (would leave no admins).""" + # Create a test admin + seed = _suffix() + username = f"lastadmin{seed}" + create_test_user( + db_path=str(db_path), + username=username, + email=f"{username}@test.com", + password="TestPassword123!", + role="admin", + ) + + # Verify there are 2 admins now + admin_count = get_user_by_username(str(db_path), username) + assert admin_count is not None + + _login( + page, + flask_server, + username, # Login as the test admin (last admin) + "TestPassword123!", + ) + + page.goto( + f"{flask_server['base_url']}/admin/users", wait_until="domcontentloaded" + ) + csrf_token = page.locator('input[name="csrf_token"]').first.get_attribute( + "value" + ) + + response = page.request.post( + f"{flask_server['base_url']}/admin/users", + form={ + "csrf_token": csrf_token, + "username": username, + "user_delete_button": "1", + }, + ) + # Should be blocked + assert response.ok + + # User should still exist + still_exists = get_user_by_username(str(db_path), username) + assert still_exists is not None, "Last admin should not be deletable" + + @pytest.mark.admin + def test_non_admin_cannot_delete_other_user( + self, page, flask_server, app_settings, db_path + ): + """Non-admin user cannot delete another user.""" + seed = _suffix() + target_username = f"target{seed}" + victim_username = f"victim{seed}" + + # Create a victim user + create_test_user( + db_path=str(db_path), + username=victim_username, + email=f"{victim_username}@test.com", + password="TestPassword123!", + role="user", + ) + + # Login as a non-admin user + _login( + page, + flask_server, + target_username, + "TestPassword123!", + ) + + # Try to delete another user via admin endpoint (forged request) + page.goto( + f"{flask_server['base_url']}/admin/users", wait_until="domcontentloaded" + ) + csrf_token = page.locator('input[name="csrf_token"]').first.get_attribute( + "value" + ) + + response = page.request.post( + f"{flask_server['base_url']}/admin/users", + form={ + "csrf_token": csrf_token, + "username": victim_username, + "user_delete_button": "1", + }, + ) + + # Should be redirected (not authorized) + assert response.status in [302, 303] + + # Victim should still exist + still_exists = get_user_by_username(str(db_path), victim_username) + assert still_exists is not None, "Non-admin cannot delete other users" + + @pytest.mark.admin + def test_user_can_delete_own_account( + self, page, flask_server, app_settings, db_path + ): + """Non-admin user can delete their own account.""" + seed = _suffix() + username = f"selfdel{seed}" + create_test_user( + db_path=str(db_path), + username=username, + email=f"{username}@test.com", + password="TestPassword123!", + role="user", + ) + + _login(page, flask_server, username, "TestPassword123!") + + page.goto( + f"{flask_server['base_url']}/account-settings", + wait_until="domcontentloaded", + ) + csrf_token = page.locator('input[name="csrf_token"]').first.get_attribute( + "value" + ) + + response = page.request.post( + f"{flask_server['base_url']}/account-settings", + form={ + "csrf_token": csrf_token, + "username": username, + "user_delete_button": "1", + }, + ) + + # Should redirect to home + assert response.status in [302, 303] + + # User should be deleted + deleted_user = get_user_by_username(str(db_path), username) + assert deleted_user is None, "Self-deleted user should no longer exist"