diff --git a/src/core/errors.py b/src/core/errors.py index 5fe54376..f8de6070 100644 --- a/src/core/errors.py +++ b/src/core/errors.py @@ -231,6 +231,22 @@ class ForbiddenError(ProblemDetailError): _default_status_code = HTTPStatus.FORBIDDEN +class UserNotFoundError(ProblemDetailError): + """Raised when a user id does not exist in the user database.""" + + uri = "https://openml.org/problems/user-not-found" + title = "User Not Found" + _default_status_code = HTTPStatus.NOT_FOUND + + +class AccountHasResourcesError(ProblemDetailError): + """Raised when account deletion is blocked because the user still owns resources.""" + + uri = "https://openml.org/problems/account-has-resources" + title = "Account Has Active Resources" + _default_status_code = HTTPStatus.CONFLICT + + # ============================================================================= # Tag Errors # ============================================================================= diff --git a/src/database/users.py b/src/database/users.py index bc2d645b..5f87d161 100644 --- a/src/database/users.py +++ b/src/database/users.py @@ -92,3 +92,52 @@ async def get_groups(self) -> list[UserGroup]: async def is_admin(self) -> bool: return UserGroup.ADMIN in await self.get_groups() + + +async def exists_by_id(*, user_id: int, connection: AsyncConnection) -> bool: + row = await connection.execute( + text("SELECT 1 FROM users WHERE id = :user_id LIMIT 1"), + parameters={"user_id": user_id}, + ) + return row.one_or_none() is not None + + +async def has_user_references(*, user_id: int, expdb: AsyncConnection) -> bool: + """Return ``True`` if any ``expdb`` row still references ``user_id``.""" + row = await expdb.execute( + text( + """ + SELECT EXISTS ( + SELECT 1 FROM dataset WHERE uploader = :uid + UNION ALL SELECT 1 FROM dataset_description WHERE uploader = :uid + UNION ALL SELECT 1 FROM dataset_status WHERE user_id = :uid + UNION ALL SELECT 1 FROM dataset_tag WHERE uploader = :uid + UNION ALL SELECT 1 FROM dataset_topic WHERE uploader = :uid + UNION ALL SELECT 1 FROM implementation WHERE uploader = :uid + UNION ALL SELECT 1 FROM implementation_tag WHERE uploader = :uid + UNION ALL SELECT 1 FROM `run` WHERE uploader = :uid + UNION ALL SELECT 1 FROM run_study WHERE uploader = :uid + UNION ALL SELECT 1 FROM run_tag WHERE uploader = :uid + UNION ALL SELECT 1 FROM setup_tag WHERE uploader = :uid + UNION ALL SELECT 1 FROM study WHERE creator = :uid + UNION ALL SELECT 1 FROM task WHERE creator = :uid + UNION ALL SELECT 1 FROM task_study WHERE uploader = :uid + UNION ALL SELECT 1 FROM task_tag WHERE uploader = :uid + ) AS has_refs + """, + ), + parameters={"uid": user_id}, + ) + return bool(row.scalar_one()) + + +async def delete_user_rows(*, user_id: int, userdb: AsyncConnection) -> None: + """Remove group memberships then the user row (openml user database).""" + await userdb.execute( + text("DELETE FROM users_groups WHERE user_id = :user_id"), + parameters={"user_id": user_id}, + ) + await userdb.execute( + text("DELETE FROM users WHERE id = :user_id"), + parameters={"user_id": user_id}, + ) diff --git a/src/main.py b/src/main.py index 498a77a3..b4f7dc80 100644 --- a/src/main.py +++ b/src/main.py @@ -37,6 +37,7 @@ from routers.openml.study import router as study_router from routers.openml.tasks import router as task_router from routers.openml.tasktype import router as ttype_router +from routers.openml.users import router as users_router @asynccontextmanager @@ -110,6 +111,7 @@ def create_api(configuration: Configuration | None = None) -> FastAPI: app.include_router(study_router) app.include_router(setup_router) app.include_router(run_router) + app.include_router(users_router) logger.info("App setup completed.") logger.remove(setup_sink) diff --git a/src/routers/openml/users.py b/src/routers/openml/users.py new file mode 100644 index 00000000..43ea9bbd --- /dev/null +++ b/src/routers/openml/users.py @@ -0,0 +1,71 @@ +"""User account HTTP endpoints.""" + +from http import HTTPStatus +from typing import Annotated + +from fastapi import APIRouter, Depends, Path, Response +from loguru import logger +from sqlalchemy.exc import IntegrityError +from sqlalchemy.ext.asyncio import AsyncConnection # noqa: TC002 used at runtime by FastAPI Depends + +import database.users +from core.errors import AccountHasResourcesError, ForbiddenError, UserNotFoundError +from database.users import User +from routers.dependencies import expdb_connection, fetch_user_or_raise, userdb_connection + +_ACCOUNT_HAS_RESOURCES_MSG = ( + "Cannot delete this account while records still reference the user " + "(datasets, flows, runs, studies, tags, etc.). Remove or transfer them first." +) + +router = APIRouter(prefix="/users", tags=["users"]) + + +@router.delete( + "/{user_id}", + responses={ + HTTPStatus.NO_CONTENT: {"description": "User account deleted."}, + HTTPStatus.UNAUTHORIZED: {"description": "Authentication failed or missing."}, + HTTPStatus.FORBIDDEN: {"description": "Not allowed to delete this account."}, + HTTPStatus.NOT_FOUND: {"description": "User id not found."}, + HTTPStatus.CONFLICT: { + "description": "User still has datasets, flows, runs, or studies.", + }, + }, +) +async def delete_user_account( + user_id: Annotated[int, Path(description="Numeric user id to delete.", gt=0)], + current_user: Annotated[User, Depends(fetch_user_or_raise)], + expdb: Annotated[AsyncConnection, Depends(expdb_connection)], + userdb: Annotated[AsyncConnection, Depends(userdb_connection)], +) -> Response: + """Delete the user account if they have no associated resources. + + The account to be deleted must not have associated resources (such as + datasets, tasks, or tags). Users may only delete their own account. + Administrators may delete any account that satisfies the no-resources rule. + """ + # How to handle users that do have associated resources is an ongoing discussion, + # see also: https://github.com/openml/server-api/issues/194 + if current_user.user_id != user_id and not await current_user.is_admin(): + msg = "You may only delete your own user account." + raise ForbiddenError(msg) + + if not await database.users.exists_by_id(user_id=user_id, connection=userdb): + msg = f"User {user_id} not found." + raise UserNotFoundError(msg) + + if await database.users.has_user_references(user_id=user_id, expdb=expdb): + raise AccountHasResourcesError(_ACCOUNT_HAS_RESOURCES_MSG) + + try: + await database.users.delete_user_rows(user_id=user_id, userdb=userdb) + except IntegrityError as exc: + logger.error( + "Delete of user {user_id} failed with integrity error after pre-check.", + user_id=user_id, + ) + raise AccountHasResourcesError(_ACCOUNT_HAS_RESOURCES_MSG) from exc + + logger.info("User account {user_id} was removed.", user_id=user_id) + return Response(status_code=HTTPStatus.NO_CONTENT) diff --git a/tests/routers/openml/users_delete_test.py b/tests/routers/openml/users_delete_test.py new file mode 100644 index 00000000..127e89e7 --- /dev/null +++ b/tests/routers/openml/users_delete_test.py @@ -0,0 +1,202 @@ +"""Tests for DELETE /users/{user_id}.""" + +import uuid +from http import HTTPStatus +from typing import NamedTuple + +import httpx # noqa: TC002 used at runtime by pytest fixtures +import pytest +import pytest_mock # noqa: TC002 used at runtime by pytest fixtures +from sqlalchemy import text +from sqlalchemy.exc import IntegrityError +from sqlalchemy.ext.asyncio import AsyncConnection # noqa: TC002 used at runtime by pytest fixtures + +from core.errors import AccountHasResourcesError, ForbiddenError, UserNotFoundError +from database.users import UserGroup +from routers.openml.users import delete_user_account +from tests.users import ADMIN_USER, OWNER_USER, SOME_USER, ApiKey + + +async def test_delete_user_missing_auth(py_api: httpx.AsyncClient) -> None: + response = await py_api.delete("/users/1") + assert response.status_code == HTTPStatus.UNAUTHORIZED + body = response.json() + assert body["code"] == "103" + assert body["detail"] == "No API key provided." + + +class DisposableUser(NamedTuple): + user_id: int + api_key: str + + +@pytest.fixture +async def disposable_user(user_test: AsyncConnection) -> DisposableUser: + api_key = uuid.uuid4().hex + suffix = uuid.uuid4().hex[:10] + username = f"tmp_user_{suffix}" + email = f"{suffix}@openml-delete.test" + + await user_test.execute( + text( + """ + INSERT INTO users ( + ip_address, username, password, email, created_on, + company, country, bio, session_hash + ) VALUES ( + '127.0.0.1', :username, 'x', :email, UNIX_TIMESTAMP(), + '', '', '', :api_key + ) + """, + ), + parameters={"username": username, "email": email, "api_key": api_key}, + ) + uid_row = await user_test.execute(text("SELECT LAST_INSERT_ID() AS id")) + (new_id,) = uid_row.one() + await user_test.execute( + text("INSERT INTO users_groups (user_id, group_id) VALUES (:uid, :gid)"), + parameters={"uid": new_id, "gid": UserGroup.READ_WRITE.value}, + ) + return DisposableUser(user_id=new_id, api_key=api_key) + # No explicit teardown: the ``user_test`` fixture rolls back at the end + # of the test, which removes the rows inserted above. + + +@pytest.mark.mut +async def test_delete_user_api_success_self_delete( + py_api: httpx.AsyncClient, + user_test: AsyncConnection, + disposable_user: DisposableUser, + mocker: pytest_mock.MockerFixture, +) -> None: + log_info = mocker.patch("routers.openml.users.logger.info") + + response = await py_api.delete( + f"/users/{disposable_user.user_id}", + params={"api_key": disposable_user.api_key}, + ) + assert response.status_code == HTTPStatus.NO_CONTENT + assert response.content == b"" + + exists = await user_test.execute( + text("SELECT 1 FROM users WHERE id = :id LIMIT 1"), + parameters={"id": disposable_user.user_id}, + ) + assert exists.one_or_none() is None + + log_info.assert_any_call( + "User account {user_id} was removed.", + user_id=disposable_user.user_id, + ) + + +@pytest.mark.mut +async def test_delete_user_api_success_admin_deletes_disposable_user( + py_api: httpx.AsyncClient, + user_test: AsyncConnection, + disposable_user: DisposableUser, +) -> None: + response = await py_api.delete( + f"/users/{disposable_user.user_id}", + params={"api_key": ApiKey.ADMIN}, + ) + assert response.status_code == HTTPStatus.NO_CONTENT + assert response.content == b"" + + exists = await user_test.execute( + text("SELECT 1 FROM users WHERE id = :id LIMIT 1"), + parameters={"id": disposable_user.user_id}, + ) + assert exists.one_or_none() is None + + +# ── Direct handler tests ── + + +async def test_delete_user_direct_not_found( + user_test: AsyncConnection, + expdb_test: AsyncConnection, +) -> None: + with pytest.raises(UserNotFoundError, match=r"User 888888888 not found\.") as exc_info: + await delete_user_account( + user_id=888888888, + current_user=ADMIN_USER, + expdb=expdb_test, + userdb=user_test, + ) + assert exc_info.value.status_code == HTTPStatus.NOT_FOUND + assert exc_info.value.uri == UserNotFoundError.uri + + +async def test_delete_user_direct_forbidden( + user_test: AsyncConnection, + expdb_test: AsyncConnection, +) -> None: + with pytest.raises( + ForbiddenError, match=r"You may only delete your own user account\." + ) as exc_info: + await delete_user_account( + user_id=ADMIN_USER.user_id, + current_user=SOME_USER, + expdb=expdb_test, + userdb=user_test, + ) + assert exc_info.value.status_code == HTTPStatus.FORBIDDEN + assert exc_info.value.uri == ForbiddenError.uri + + admin_row = await user_test.execute( + text("SELECT 1 FROM users WHERE id = :id LIMIT 1"), + parameters={"id": ADMIN_USER.user_id}, + ) + assert admin_row.one_or_none() is not None + + +async def test_delete_user_direct_conflict_has_resources( + user_test: AsyncConnection, + expdb_test: AsyncConnection, +) -> None: + with pytest.raises(AccountHasResourcesError, match="Cannot delete this account") as exc_info: + await delete_user_account( + user_id=OWNER_USER.user_id, + current_user=ADMIN_USER, + expdb=expdb_test, + userdb=user_test, + ) + assert exc_info.value.status_code == HTTPStatus.CONFLICT + assert exc_info.value.uri == AccountHasResourcesError.uri + + owner_row = await user_test.execute( + text("SELECT 1 FROM users WHERE id = :id LIMIT 1"), + parameters={"id": OWNER_USER.user_id}, + ) + assert owner_row.one_or_none() is not None + + +@pytest.mark.mut +async def test_delete_user_integrity_error_logs_and_raises_conflict( + user_test: AsyncConnection, + expdb_test: AsyncConnection, + disposable_user: DisposableUser, + mocker: pytest_mock.MockerFixture, +) -> None: + mocker.patch( + "database.users.delete_user_rows", + side_effect=IntegrityError( + "DELETE FROM users", {"user_id": disposable_user.user_id}, Exception("fk") + ), + ) + log_error = mocker.patch("routers.openml.users.logger.error") + + with pytest.raises(AccountHasResourcesError, match="Cannot delete this account") as exc_info: + await delete_user_account( + user_id=disposable_user.user_id, + current_user=ADMIN_USER, + expdb=expdb_test, + userdb=user_test, + ) + + assert exc_info.value.status_code == HTTPStatus.CONFLICT + log_error.assert_called_once_with( + "Delete of user {user_id} failed with integrity error after pre-check.", + user_id=disposable_user.user_id, + )