diff --git a/src/backend/api/routers/users.py b/src/backend/api/routers/users.py index 367573f..3abc742 100644 --- a/src/backend/api/routers/users.py +++ b/src/backend/api/routers/users.py @@ -1,6 +1,6 @@ """Users router - CRUD operations (admin only).""" from fastapi import APIRouter, Depends, HTTPException, status -from sqlalchemy import select +from sqlalchemy import func, select from sqlalchemy.ext.asyncio import AsyncSession from src.backend.database import get_db @@ -78,6 +78,21 @@ async def update_user( raise HTTPException(status_code=status.HTTP_404_NOT_FOUND, detail="User not found") update_data = data.model_dump(exclude_unset=True) + + # roles and is_admin are NOT NULL columns: an explicit null passed the + # Optional[...] schema check but blew up at flush time as an unhandled + # 500 (#36 point 4). + if "roles" in update_data and update_data["roles"] is None: + raise HTTPException( + status_code=status.HTTP_422_UNPROCESSABLE_ENTITY, + detail="roles cannot be null", + ) + if "is_admin" in update_data and update_data["is_admin"] is None: + raise HTTPException( + status_code=status.HTTP_422_UNPROCESSABLE_ENTITY, + detail="is_admin cannot be null", + ) + # Validate roles if provided if "roles" in update_data: for role in update_data["roles"]: @@ -86,6 +101,27 @@ async def update_user( status_code=status.HTTP_422_UNPROCESSABLE_ENTITY, detail=f"Invalid role '{role}'. Valid roles: {VALID_ROLES}", ) + + # Losing the last admin locks everyone out of user management; losing + # your own admin flag while acting as admin is the same mistake aimed + # at yourself. Neither is reversible without direct DB access. + if update_data.get("is_admin") is False and user.is_admin: + if user.id == admin.id: + raise HTTPException( + status_code=status.HTTP_409_CONFLICT, + detail="Cannot remove your own admin privilege", + ) + other_admins = await db.execute( + select(func.count()).select_from(User).where( + User.is_admin == True, User.id != user.id # noqa: E712 + ) + ) + if other_admins.scalar_one() == 0: + raise HTTPException( + status_code=status.HTTP_409_CONFLICT, + detail="Cannot remove admin privilege from the last admin", + ) + for field, value in update_data.items(): setattr(user, field, value) await db.flush() diff --git a/src/backend/tests/test_users.py b/src/backend/tests/test_users.py index 36c3860..e5a93a6 100644 --- a/src/backend/tests/test_users.py +++ b/src/backend/tests/test_users.py @@ -107,6 +107,61 @@ class TestUpdateUser: assert resp.json()["display_name"] == "Updated Maker" +class TestUpdateUserValidation: + """gitea#36 point 4: roles/is_admin cannot be null, admin self/last-admin guard.""" + + async def test_roles_null_is_422( + self, client: AsyncClient, admin_user: User, maker_user: User, + ): + """roles is Optional in the schema but NOT NULL in the DB: an explicit + null used to reach setattr() and blow up as an unhandled 500.""" + resp = await client.put( + f"/api/users/{maker_user.id}", + headers=auth_headers(admin_user), + json={"roles": None}, + ) + assert resp.status_code == 422 + + async def test_is_admin_null_is_422( + self, client: AsyncClient, admin_user: User, maker_user: User, + ): + resp = await client.put( + f"/api/users/{maker_user.id}", + headers=auth_headers(admin_user), + json={"is_admin": None}, + ) + assert resp.status_code == 422 + + async def test_admin_cannot_remove_own_admin_flag( + self, client: AsyncClient, admin_user: User, + ): + """admin_user is the only admin: this is also the only reachable + "last admin" case, since only an admin can call this endpoint at + all — removing someone else's flag always leaves the caller.""" + resp = await client.put( + f"/api/users/{admin_user.id}", + headers=auth_headers(admin_user), + json={"is_admin": False}, + ) + assert resp.status_code == 409 + + async def test_admin_can_demote_another_admin_when_not_last( + self, client: AsyncClient, admin_user: User, db_session: AsyncSession, + ): + """Demoting someone else is fine as long as an admin remains (the + caller): the guard must not be broader than the two stated cases.""" + second_admin = await _create_user( + db_session, username="second_admin", roles=[], is_admin=True, + ) + resp = await client.put( + f"/api/users/{second_admin.id}", + headers=auth_headers(admin_user), + json={"is_admin": False}, + ) + assert resp.status_code == 200 + assert resp.json()["is_admin"] is False + + class TestDeleteUser: """DELETE /api/users/{user_id} tests."""