fix(users): repair create delete and duplicate email handling

This commit is contained in:
Schubert Ferenc 2026-07-11 17:14:41 +02:00
parent 155fdbb16a
commit 0bbcaba211
22 changed files with 773 additions and 107 deletions

View file

@ -0,0 +1,108 @@
from __future__ import annotations
from alembic import op
import sqlalchemy as sa
revision = "202607110006"
down_revision = "202607110005"
branch_labels = None
depends_on = None
ALLOWED_ROLES = ("admin", "pruefer", "mitarbeiter", "leser")
def _other_columns_use_userrole(connection) -> bool:
result = connection.execute(
sa.text(
"""
SELECT EXISTS (
SELECT 1
FROM pg_attribute a
JOIN pg_class c ON c.oid = a.attrelid
JOIN pg_type t ON t.oid = a.atttypid
JOIN pg_namespace n ON n.oid = c.relnamespace
WHERE t.typname = 'userrole'
AND NOT (n.nspname = 'public' AND c.relname = 'users' AND a.attname = 'role')
AND a.attnum > 0
AND NOT a.attisdropped
)
"""
)
)
return bool(result.scalar())
def _normalize_roles(connection) -> None:
connection.execute(
sa.text(
"""
UPDATE users
SET role = CASE upper(role::text)
WHEN 'ADMIN' THEN 'admin'
WHEN 'PRUEFER' THEN 'pruefer'
WHEN 'EMPLOYEE' THEN 'mitarbeiter'
WHEN 'MITARBEITER' THEN 'mitarbeiter'
WHEN 'AUDITOR' THEN 'leser'
WHEN 'LESER' THEN 'leser'
ELSE role::text
END
"""
)
)
invalid_roles = connection.execute(
sa.text(
"""
SELECT DISTINCT role::text
FROM users
WHERE role::text NOT IN ('admin', 'pruefer', 'mitarbeiter', 'leser')
"""
),
).scalars().all()
if invalid_roles:
raise RuntimeError(f"Unknown user roles encountered during migration: {', '.join(sorted(invalid_roles))}")
def upgrade() -> None:
connection = op.get_bind()
op.alter_column(
"users",
"role",
existing_type=sa.Enum(name="userrole"),
type_=sa.String(length=50),
existing_nullable=False,
existing_server_default=None,
postgresql_using="role::text",
nullable=False,
)
_normalize_roles(connection)
op.execute(sa.text("ALTER TABLE users ALTER COLUMN role SET DEFAULT 'mitarbeiter'"))
if not _other_columns_use_userrole(connection):
op.execute(sa.text("DROP TYPE IF EXISTS userrole"))
def downgrade() -> None:
connection = op.get_bind()
op.execute(
sa.text(
"CREATE TYPE userrole AS ENUM ('admin', 'pruefer', 'mitarbeiter', 'leser')"
)
)
invalid_roles = connection.execute(
sa.text(
"""
SELECT DISTINCT role
FROM users
WHERE role NOT IN ('admin', 'pruefer', 'mitarbeiter', 'leser')
"""
),
).scalars().all()
if invalid_roles:
raise RuntimeError(f"Cannot downgrade user roles with invalid values: {', '.join(sorted(invalid_roles))}")
op.alter_column(
"users",
"role",
existing_type=sa.String(length=50),
type_=sa.Enum("admin", "pruefer", "mitarbeiter", "leser", name="userrole"),
nullable=False,
)
op.execute(sa.text("ALTER TABLE users ALTER COLUMN role SET DEFAULT 'mitarbeiter'::userrole"))

View file

@ -0,0 +1,25 @@
"""add deleted_at to users for soft delete
Revision ID: 202607110007
Revises: 202607110006
Create Date: 2026-07-11 00:07:00.000000
"""
from __future__ import annotations
from alembic import op
import sqlalchemy as sa
revision = "202607110007"
down_revision = "202607110006"
branch_labels = None
depends_on = None
def upgrade() -> None:
op.add_column("users", sa.Column("deleted_at", sa.DateTime(timezone=True), nullable=True))
def downgrade() -> None:
op.drop_column("users", "deleted_at")

View file

@ -6,13 +6,14 @@ import json
import logging
import shutil
import uuid
from datetime import UTC, datetime
from pathlib import Path
from fastapi import APIRouter, Depends, File, Form, Query, Response, UploadFile
from fastapi import APIRouter, Depends, File, Form, HTTPException, Query, Response, UploadFile
from fastapi.encoders import jsonable_encoder
from fastapi.responses import FileResponse, HTMLResponse, JSONResponse
from sqlalchemy import func, select
from sqlalchemy.exc import IntegrityError
from sqlalchemy.exc import IntegrityError, ProgrammingError
from sqlalchemy.orm import Session
from app.api.dependencies import current_admin, current_user
@ -560,7 +561,7 @@ def _user_payload(user: User) -> dict:
"email": user.email,
"first_name": user.first_name,
"last_name": user.last_name,
"role": user.role,
"role": UserRole(user.role),
"is_active": user.is_active,
"must_change_password": user.must_change_password,
"last_login_at": user.last_login_at,
@ -581,6 +582,7 @@ def list_users(
_: User = Depends(current_admin),
):
query = select(User)
query = query.where(User.deleted_at.is_(None))
if search:
term = f"%{search.strip()}%"
query = query.where(
@ -605,8 +607,9 @@ def list_users(
sort_column = sort_columns.get(sort_by, User.created_at)
query = query.order_by(sort_column.asc() if sort_order.lower() == "asc" else sort_column.desc())
total = session.scalar(select(func.count()).select_from(query.order_by(None).subquery())) or 0
pages = max((total + page_size - 1) // page_size, 1)
items = list(session.scalars(query.offset((page - 1) * page_size).limit(page_size)))
return {"items": items, "total": total, "page": page, "page_size": page_size}
return {"items": items, "total": total, "page": page, "page_size": page_size, "pages": pages}
@router.post("/users", status_code=201)
@ -615,26 +618,47 @@ def create_user(payload: UserCreate, session: Session = Depends(get_session), _:
import secrets
from app.models.user import User
normalized_email = payload.email.strip().lower()
logger.debug("create_user request email=%s normalized_email=%s", payload.email, normalized_email)
existing = session.scalar(select(User).where(func.lower(User.email) == normalized_email))
logger.debug("create_user select result=%s count=%s", bool(existing), 1 if existing else 0)
if existing is not None:
raise HTTPException(status_code=409, detail="Die E-Mail-Adresse wird bereits verwendet.")
temporary_password = payload.temporary_password or secrets.token_urlsafe(12)
user = User(
email=payload.email.lower(),
first_name=payload.first_name,
last_name=payload.last_name,
role=payload.role.value,
password_hash=hash_password(temporary_password),
is_active=payload.is_active,
must_change_password=payload.must_change_password,
)
session.add(user)
session.commit()
session.refresh(user)
return JSONResponse(content={"temporary_password": temporary_password, "user": jsonable_encoder(user)}, status_code=201)
try:
user = User(
email=normalized_email,
first_name=payload.first_name,
last_name=payload.last_name,
role=payload.role.value,
password_hash=hash_password(temporary_password),
is_active=payload.is_active,
must_change_password=payload.must_change_password,
)
logger.debug("create_user insert email=%s", normalized_email)
session.add(user)
session.commit()
logger.debug("create_user commit ok email=%s id=%s", normalized_email, user.id)
session.refresh(user)
logger.debug("create_user response email=%s status=201", normalized_email)
return JSONResponse(
content={"temporary_password": temporary_password, "user": jsonable_encoder(user)},
status_code=201,
)
except IntegrityError as exc:
session.rollback()
logger.exception("create_user integrity error email=%s", normalized_email)
raise HTTPException(status_code=409, detail="Die E-Mail-Adresse wird bereits verwendet.") from exc
except ProgrammingError as exc:
session.rollback()
raise HTTPException(status_code=422, detail="Die Benutzerrolle ist ungültig.") from exc
@router.get("/users/{item_id}", response_model=UserRead)
def get_user(item_id: str, session: Session = Depends(get_session), _: User = Depends(current_admin)):
user = session.get(User, item_id)
if user is None:
if user is None or user.deleted_at is not None:
raise HTTPException(status_code=404, detail="Resource not found")
return user
@ -642,7 +666,7 @@ def get_user(item_id: str, session: Session = Depends(get_session), _: User = De
@router.put("/users/{item_id}", response_model=UserRead)
def update_user(item_id: str, payload: UserUpdate, session: Session = Depends(get_session), me: User = Depends(current_admin)):
user = session.get(User, item_id)
if user is None:
if user is None or user.deleted_at is not None:
raise HTTPException(status_code=404, detail="Resource not found")
if user.id == me.id and not payload.is_active:
raise HTTPException(status_code=409, detail="Own admin account cannot be deactivated")
@ -651,44 +675,58 @@ def update_user(item_id: str, payload: UserUpdate, session: Session = Depends(ge
) or 0
if user.role == UserRole.ADMIN.value and active_admins <= 1 and (not payload.is_active or payload.role != UserRole.ADMIN.value):
raise HTTPException(status_code=409, detail="Der letzte aktive ADMIN darf nicht deaktiviert oder herabgestuft werden.")
user.first_name = payload.first_name
user.last_name = payload.last_name
user.email = payload.email.lower()
user.role = payload.role.value
user.is_active = payload.is_active
user.must_change_password = payload.must_change_password
session.commit()
session.refresh(user)
return user
try:
normalized_email = payload.email.strip().lower()
logger.debug("update_user request id=%s email=%s normalized_email=%s", item_id, payload.email, normalized_email)
existing = session.scalar(
select(User).where(func.lower(User.email) == normalized_email, User.id != user.id)
)
logger.debug("update_user select result=%s count=%s", bool(existing), 1 if existing else 0)
if existing is not None:
raise HTTPException(status_code=409, detail="Die E-Mail-Adresse wird bereits verwendet.")
user.first_name = payload.first_name
user.last_name = payload.last_name
user.email = normalized_email
user.role = payload.role.value
user.is_active = payload.is_active
user.must_change_password = payload.must_change_password
logger.debug("update_user commit start id=%s email=%s", user.id, normalized_email)
session.commit()
logger.debug("update_user commit ok id=%s email=%s", user.id, normalized_email)
session.refresh(user)
return user
except IntegrityError as exc:
session.rollback()
logger.exception("update_user integrity error id=%s email=%s", item_id, payload.email)
raise HTTPException(status_code=409, detail="Die E-Mail-Adresse wird bereits verwendet.") from exc
except ProgrammingError as exc:
session.rollback()
raise HTTPException(status_code=422, detail="Die Benutzerrolle ist ungültig.") from exc
@router.post("/users/{item_id}/deactivate", response_model=UserRead)
def deactivate_user(item_id: str, session: Session = Depends(get_session), me: User = Depends(current_admin)):
@router.delete("/users/{item_id}", status_code=204)
def delete_user(item_id: str, session: Session = Depends(get_session), me: User = Depends(current_admin)):
user = session.get(User, item_id)
if user is None:
if user is None or user.deleted_at is not None:
raise HTTPException(status_code=404, detail="Resource not found")
if user.id == me.id:
raise HTTPException(status_code=409, detail="Own admin account cannot be deactivated")
raise HTTPException(status_code=409, detail="Das eigene Benutzerkonto kann nicht geloescht werden.")
active_admins = session.scalar(
select(func.count()).select_from(User).where(User.role == UserRole.ADMIN.value, User.is_active.is_(True))
) or 0
if user.role == UserRole.ADMIN.value and active_admins <= 1:
raise HTTPException(status_code=409, detail="Der letzte aktive ADMIN darf nicht deaktiviert werden.")
raise HTTPException(status_code=409, detail="Der letzte aktive ADMIN darf nicht geloescht werden.")
referenced_validations = session.scalar(
select(func.count()).select_from(Validation).where(Validation.examiner_id == user.id)
) or 0
if referenced_validations == 0:
session.delete(user)
session.commit()
return Response(status_code=204)
user.is_active = False
user.deleted_at = datetime.now(UTC)
session.commit()
session.refresh(user)
return user
@router.post("/users/{item_id}/activate", response_model=UserRead)
def activate_user(item_id: str, session: Session = Depends(get_session), _: User = Depends(current_admin)):
user = session.get(User, item_id)
if user is None:
raise HTTPException(status_code=404, detail="Resource not found")
user.is_active = True
session.commit()
session.refresh(user)
return user
return Response(status_code=204)
@router.post("/users/{item_id}/reset-password")
@ -697,7 +735,7 @@ def reset_user_password(item_id: str, session: Session = Depends(get_session), _
from app.core.security import hash_password
user = session.get(User, item_id)
if user is None:
if user is None or user.deleted_at is not None:
raise HTTPException(status_code=404, detail="Resource not found")
temporary_password = secrets.token_urlsafe(12)
user.password_hash = hash_password(temporary_password)

View file

@ -4,7 +4,7 @@ import enum
from datetime import datetime
from sqlalchemy import Boolean, DateTime, String
from sqlalchemy.orm import Mapped, mapped_column
from sqlalchemy.orm import Mapped, mapped_column, validates
from app.db.base import Base, TimestampMixin, UUIDMixin
@ -20,15 +20,21 @@ class User(Base, UUIDMixin, TimestampMixin):
__tablename__ = "users"
email: Mapped[str] = mapped_column(String(255), unique=True, index=True)
full_name: Mapped[str] = mapped_column(String(160), nullable=False)
first_name: Mapped[str] = mapped_column(String(80))
last_name: Mapped[str] = mapped_column(String(80))
role: Mapped[str] = mapped_column(String(40), default=UserRole.MITARBEITER.value)
role: Mapped[str] = mapped_column(String(50), nullable=False, default=UserRole.MITARBEITER.value)
password_hash: Mapped[str] = mapped_column(String(255))
is_active: Mapped[bool] = mapped_column(Boolean, default=True)
must_change_password: Mapped[bool] = mapped_column(Boolean, default=True)
deleted_at: Mapped[datetime | None] = mapped_column(DateTime(timezone=True), nullable=True)
last_login_at: Mapped[datetime | None] = mapped_column(DateTime(timezone=True), nullable=True)
password_changed_at: Mapped[datetime | None] = mapped_column(DateTime(timezone=True), nullable=True)
@property
def full_name(self) -> str:
return f"{self.first_name} {self.last_name}".strip()
@validates("first_name", "last_name")
def _sync_full_name(self, key: str, value: str) -> str:
first_name = value if key == "first_name" else getattr(self, "first_name", None)
last_name = value if key == "last_name" else getattr(self, "last_name", None)
if first_name is not None and last_name is not None:
self.full_name = f"{first_name} {last_name}".strip()
return value

View file

@ -18,7 +18,7 @@ class UserRepository(Repository[User]):
model = User
def by_email(self, email: str) -> User | None:
return self.session.scalar(select(User).where(User.email == email.lower()))
return self.session.scalar(select(User).where(User.email == email.strip().lower()))
class CustomerRepository(Repository[Customer]):

View file

@ -23,3 +23,4 @@ class PaginatedResponse(BaseModel, Generic[T]):
total: int
page: int
page_size: int
pages: int

View file

@ -23,7 +23,7 @@ class AuthService:
detail="Invalid credentials",
headers={"WWW-Authenticate": "Bearer"},
)
if not user.is_active:
if not user.is_active or user.deleted_at is not None:
raise HTTPException(status_code=status.HTTP_403_FORBIDDEN, detail="Inactive user")
user.last_login_at = datetime.now(UTC)
self.users.session.commit()

View file

@ -1,9 +1,11 @@
from __future__ import annotations
import json
import re
from datetime import date
from pathlib import Path
import pytest
from sqlalchemy import create_engine, select
from sqlalchemy.orm import Session
@ -19,8 +21,9 @@ from app.services.validation_workflow import ValidationWorkflowService
from app.services.auth_service import AuthService
from app.services.reference_masterdata import ReferenceMasterdataImportService
from app.api.v1.auth import login as auth_login
from app.api.v1 import domain as domain_api
from app.schemas.auth import LoginRequest
from app.schemas.domain import ValidationCreate
from app.schemas.domain import UserCreate, UserUpdate, ValidationCreate
from app.modules.orion.service import OrionReportService
from app.modules.orion.assets import SCHUBAMED_LOGO_PATH, schubamed_logo_uri
from app.modules.orion.template_service import REPORT_SECTIONS, ReportTemplateService
@ -212,6 +215,319 @@ def test_user_model_supports_password_management_fields():
assert user.role == UserRole.PRUEFER
def test_admin_appears_in_user_list():
db = session()
admin = User(
email="admin@schubamed.de",
first_name="Admin",
last_name="User",
role=UserRole.ADMIN.value,
password_hash="hash",
is_active=True,
must_change_password=False,
)
db.add(admin)
db.flush()
result = domain_api.list_users(session=db, _=admin, page=1, page_size=20, search=None, role=None, active=None, sort_by="created_at", sort_order="desc")
assert result["total"] == 1
assert result["pages"] == 1
assert any(item.id == admin.id for item in result["items"])
def test_user_list_pagination_and_filters_work():
db = session()
admin = User(
email="admin@schubamed.de",
first_name="Admin",
last_name="User",
role=UserRole.ADMIN.value,
password_hash="hash",
is_active=True,
must_change_password=False,
)
inactive = User(
email="inactive@schubamed.de",
first_name="Inactive",
last_name="User",
role=UserRole.LESER.value,
password_hash="hash",
is_active=False,
must_change_password=False,
)
db.add_all([admin, inactive])
db.flush()
result = domain_api.list_users(session=db, _=admin, page=1, page_size=1, search=None, role=None, active=None, sort_by="created_at", sort_order="desc")
assert result["total"] == 2
assert result["pages"] == 2
assert len(result["items"]) == 1
active_only = domain_api.list_users(session=db, _=admin, page=1, page_size=20, search=None, role=None, active=True, sort_by="created_at", sort_order="desc")
assert active_only["total"] == 1
assert all(item.is_active for item in active_only["items"])
inactive_only = domain_api.list_users(session=db, _=admin, page=1, page_size=20, search=None, role=None, active=False, sort_by="created_at", sort_order="desc")
assert inactive_only["total"] == 1
assert all(not item.is_active for item in inactive_only["items"])
role_filtered = domain_api.list_users(session=db, _=admin, page=1, page_size=20, search=None, role=UserRole.ADMIN.value, active=None, sort_by="created_at", sort_order="desc")
assert role_filtered["total"] == 1
assert role_filtered["items"][0].role == UserRole.ADMIN.value
def test_admin_can_create_users_with_supported_roles():
db = session()
admin = User(
email="admin@schubamed.de",
first_name="Admin",
last_name="User",
role=UserRole.ADMIN.value,
password_hash="hash",
is_active=True,
must_change_password=False,
)
db.add(admin)
db.flush()
for role in (UserRole.MITARBEITER, UserRole.PRUEFER, UserRole.LESER):
response = domain_api.create_user(
UserCreate(
email=f"{role.value}@schubamed.de",
first_name="Test",
last_name="User",
role=role,
is_active=True,
must_change_password=False,
temporary_password="TempPassword123!",
),
session=db,
_=admin,
)
payload = json.loads(response.body)
assert response.status_code == 201
assert payload["user"]["role"] == role.value
assert db.scalar(select(User).where(User.email == f"{role.value}@schubamed.de")) is not None
def test_create_user_trims_and_normalizes_email():
db = session()
admin = User(
email="admin@schubamed.de",
first_name="Admin",
last_name="User",
role=UserRole.ADMIN.value,
password_hash="hash",
is_active=True,
must_change_password=False,
)
db.add(admin)
db.flush()
response = domain_api.create_user(
UserCreate(
email=" New.User@Schubamed.DE ",
first_name="New",
last_name="User",
role=UserRole.MITARBEITER,
is_active=True,
must_change_password=False,
temporary_password="TempPassword123!",
),
session=db,
_=admin,
)
payload = json.loads(response.body)
assert response.status_code == 201
assert payload["user"]["email"] == "new.user@schubamed.de"
assert db.scalar(select(User).where(User.email == "new.user@schubamed.de")) is not None
def test_duplicate_email_returns_409_and_rolls_back_session():
from fastapi import HTTPException
db = session()
admin = User(
email="admin@schubamed.de",
first_name="Admin",
last_name="User",
role=UserRole.ADMIN.value,
password_hash="hash",
is_active=True,
must_change_password=False,
)
db.add(admin)
db.flush()
first = domain_api.create_user(
UserCreate(
email="duplicate@schubamed.de",
first_name="First",
last_name="User",
role=UserRole.MITARBEITER,
is_active=True,
must_change_password=False,
temporary_password="TempPassword123!",
),
session=db,
_=admin,
)
assert first.status_code == 201
with pytest.raises(HTTPException) as exc_info:
domain_api.create_user(
UserCreate(
email="duplicate@schubamed.de",
first_name="Second",
last_name="User",
role=UserRole.PRUEFER,
is_active=True,
must_change_password=False,
temporary_password="TempPassword123!",
),
session=db,
_=admin,
)
assert exc_info.value.status_code == 409
follow_up = domain_api.create_user(
UserCreate(
email="fresh@schubamed.de",
first_name="Fresh",
last_name="User",
role=UserRole.LESER,
is_active=True,
must_change_password=False,
temporary_password="TempPassword123!",
),
session=db,
_=admin,
)
assert follow_up.status_code == 201
def test_update_user_keeps_same_email_without_false_duplicate():
db = session()
admin = User(
email="admin@schubamed.de",
first_name="Admin",
last_name="User",
role=UserRole.ADMIN.value,
password_hash="hash",
is_active=True,
must_change_password=False,
)
target = User(
email="target@schubamed.de",
first_name="Target",
last_name="User",
role=UserRole.MITARBEITER.value,
password_hash="hash",
is_active=True,
must_change_password=False,
)
db.add_all([admin, target])
db.flush()
response = domain_api.update_user(
target.id,
UserUpdate(
first_name="Target",
last_name="User",
email="target@schubamed.de",
role=UserRole.MITARBEITER,
is_active=True,
must_change_password=False,
),
session=db,
me=admin,
)
assert response.email == "target@schubamed.de"
assert response.id == target.id
def test_delete_user_without_references_removes_row():
db = session()
admin = User(
email="admin@schubamed.de",
first_name="Admin",
last_name="User",
role=UserRole.ADMIN.value,
password_hash="hash",
is_active=True,
must_change_password=False,
)
user = User(
email="delete-me@schubamed.de",
first_name="Delete",
last_name="Me",
role=UserRole.MITARBEITER.value,
password_hash="hash",
is_active=True,
must_change_password=False,
)
db.add_all([admin, user])
db.flush()
response = domain_api.delete_user(user.id, session=db, me=admin)
assert response.status_code == 204
assert db.get(User, user.id) is None
def test_delete_user_with_validation_reference_soft_deletes_row():
db = session()
customer, location, device = seed(db)
admin = User(
email="admin@schubamed.de",
first_name="Admin",
last_name="User",
role=UserRole.ADMIN.value,
password_hash="hash",
is_active=True,
must_change_password=False,
)
examiner = User(
email="examiner@schubamed.de",
first_name="Examiner",
last_name="User",
role=UserRole.PRUEFER.value,
password_hash="hash",
is_active=True,
must_change_password=False,
)
validation = valid_validation(customer, location, device)
db.add_all([admin, examiner, validation])
db.flush()
validation.examiner_id = examiner.id
db.flush()
response = domain_api.delete_user(examiner.id, session=db, me=admin)
assert response.status_code == 204
db.refresh(examiner)
assert examiner.deleted_at is not None
assert examiner.is_active is False
listed = domain_api.list_users(session=db, _=admin, page=1, page_size=20, search=None, role=None, active=None, sort_by="created_at", sort_order="desc")
assert all(item.id != examiner.id for item in listed["items"])
def test_invalid_user_role_is_rejected_by_schema():
with pytest.raises(Exception):
UserCreate(
email="bad-role@schubamed.de",
first_name="Bad",
last_name="Role",
role="invalid-role", # type: ignore[arg-type]
is_active=True,
must_change_password=False,
temporary_password="TempPassword123!",
)
def test_login_updates_last_login_at():
from app.core.security import hash_password