fix: Prevent admin user-delete 500 via soft-delete fallback
This commit is contained in:
@ -662,9 +662,19 @@ async def delete_user(
|
|||||||
db: Database,
|
db: Database,
|
||||||
admin: User = Depends(require_admin),
|
admin: User = Depends(require_admin),
|
||||||
):
|
):
|
||||||
"""Delete a user and all their data."""
|
"""
|
||||||
|
Delete a user and all their data.
|
||||||
|
|
||||||
|
Production-hardening:
|
||||||
|
- Prefer hard-delete (keeps DB tidy).
|
||||||
|
- If hard-delete is blocked by FK constraints, fall back to a safe deactivation
|
||||||
|
(soft-delete) so the admin UI never hits a 500.
|
||||||
|
"""
|
||||||
from app.models.blog import BlogPost
|
from app.models.blog import BlogPost
|
||||||
from app.models.admin_log import AdminActivityLog
|
from app.models.admin_log import AdminActivityLog
|
||||||
|
from app.services.auth import AuthService
|
||||||
|
from sqlalchemy.exc import IntegrityError
|
||||||
|
import secrets
|
||||||
|
|
||||||
result = await db.execute(select(User).where(User.id == user_id))
|
result = await db.execute(select(User).where(User.id == user_id))
|
||||||
user = result.scalar_one_or_none()
|
user = result.scalar_one_or_none()
|
||||||
@ -672,6 +682,10 @@ async def delete_user(
|
|||||||
if not user:
|
if not user:
|
||||||
raise HTTPException(status_code=404, detail="User not found")
|
raise HTTPException(status_code=404, detail="User not found")
|
||||||
|
|
||||||
|
# Safety rails
|
||||||
|
if user.id == admin.id:
|
||||||
|
raise HTTPException(status_code=400, detail="Cannot delete your own admin account")
|
||||||
|
|
||||||
if user.is_admin:
|
if user.is_admin:
|
||||||
raise HTTPException(status_code=400, detail="Cannot delete admin user")
|
raise HTTPException(status_code=400, detail="Cannot delete admin user")
|
||||||
|
|
||||||
@ -687,18 +701,48 @@ async def delete_user(
|
|||||||
AdminActivityLog.__table__.delete().where(AdminActivityLog.admin_id == user_id)
|
AdminActivityLog.__table__.delete().where(AdminActivityLog.admin_id == user_id)
|
||||||
)
|
)
|
||||||
|
|
||||||
# Now delete the user (cascades to domains, subscriptions, portfolio, price_alerts)
|
# Now delete the user (cascades to domains, subscriptions, portfolio, listings, alerts, etc.)
|
||||||
|
# If FK constraints block the delete (e.g., some rows reference users.id without cascade),
|
||||||
|
# we fall back to a safe soft-delete.
|
||||||
|
try:
|
||||||
await db.delete(user)
|
await db.delete(user)
|
||||||
await db.commit()
|
await db.commit()
|
||||||
|
deleted_mode = "hard"
|
||||||
|
except IntegrityError:
|
||||||
|
await db.rollback()
|
||||||
|
|
||||||
|
# Soft delete: disable account + remove auth factors so the user can never log in again.
|
||||||
|
# (We keep the row to satisfy FK constraints elsewhere.)
|
||||||
|
user.is_active = False
|
||||||
|
user.is_verified = False
|
||||||
|
user.hashed_password = AuthService.hash_password(secrets.token_urlsafe(32))
|
||||||
|
user.stripe_customer_id = None
|
||||||
|
user.password_reset_token = None
|
||||||
|
user.password_reset_expires = None
|
||||||
|
user.email_verification_token = None
|
||||||
|
user.email_verification_expires = None
|
||||||
|
user.oauth_provider = None
|
||||||
|
user.oauth_id = None
|
||||||
|
user.oauth_avatar = None
|
||||||
|
user.last_login = None
|
||||||
|
|
||||||
|
await db.commit()
|
||||||
|
deleted_mode = "soft"
|
||||||
|
|
||||||
# Log this action
|
# Log this action
|
||||||
await log_admin_activity(
|
await log_admin_activity(
|
||||||
db, admin.id, "user_delete",
|
db, admin.id, "user_delete",
|
||||||
f"Deleted user {user_email} and all their data"
|
f"Deleted user {user_email} (mode={deleted_mode})"
|
||||||
)
|
)
|
||||||
|
|
||||||
|
if deleted_mode == "hard":
|
||||||
return {"message": f"User {user_email} and all their data have been deleted"}
|
return {"message": f"User {user_email} and all their data have been deleted"}
|
||||||
|
|
||||||
|
return {
|
||||||
|
"message": f"User {user_email} has been deactivated (soft delete) due to existing references",
|
||||||
|
"mode": "soft",
|
||||||
|
}
|
||||||
|
|
||||||
|
|
||||||
@router.post("/users/{user_id}/upgrade")
|
@router.post("/users/{user_id}/upgrade")
|
||||||
async def upgrade_user(
|
async def upgrade_user(
|
||||||
|
|||||||
@ -64,7 +64,10 @@ class User(Base):
|
|||||||
"PortfolioDomain", back_populates="user", cascade="all, delete-orphan"
|
"PortfolioDomain", back_populates="user", cascade="all, delete-orphan"
|
||||||
)
|
)
|
||||||
price_alerts: Mapped[List["PriceAlert"]] = relationship(
|
price_alerts: Mapped[List["PriceAlert"]] = relationship(
|
||||||
"PriceAlert", cascade="all, delete-orphan", passive_deletes=True
|
# NOTE:
|
||||||
|
# We do not rely on DB-level ON DELETE CASCADE for this FK (it is not declared in the model),
|
||||||
|
# so we must not set passive_deletes=True. Otherwise deleting a user can fail with FK violations.
|
||||||
|
"PriceAlert", cascade="all, delete-orphan"
|
||||||
)
|
)
|
||||||
# For Sale Marketplace
|
# For Sale Marketplace
|
||||||
listings: Mapped[List["DomainListing"]] = relationship(
|
listings: Mapped[List["DomainListing"]] = relationship(
|
||||||
|
|||||||
Reference in New Issue
Block a user