diff --git a/src/core/errors.py b/src/core/errors.py index 69d7e0c..fe111c5 100644 --- a/src/core/errors.py +++ b/src/core/errors.py @@ -228,6 +228,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 0b09fb0..4f53968 100644 --- a/src/database/users.py +++ b/src/database/users.py @@ -76,3 +76,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 46cd79c..0219db8 100644 --- a/src/main.py +++ b/src/main.py @@ -34,6 +34,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 @@ -106,6 +107,7 @@ def create_api(configuration_file: Path | 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 0000000..40a0baa --- /dev/null +++ b/src/routers/openml/users.py @@ -0,0 +1,61 @@ +"""User account HTTP endpoints.""" + +from typing import Annotated + +from fastapi import APIRouter, Depends, Path, Response +from sqlalchemy.exc import IntegrityError +from sqlalchemy.ext.asyncio import AsyncConnection + +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={ + 204: {"description": "User account deleted."}, + 401: {"description": "Authentication failed or missing."}, + 403: {"description": "Not allowed to delete this account."}, + 404: {"description": "User id not found."}, + 409: {"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 a user account when they have no uploaded resources (Phase 1). + + Callers may delete their own account. Administrators may delete any account + that satisfies the no-resources rule. + """ + 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: + # Safety net: a user-reference table was added without updating + # ``has_user_references``. Surface a clean 409 instead of a 500. + raise AccountHasResourcesError(_ACCOUNT_HAS_RESOURCES_MSG) from exc + + return Response(status_code=204) diff --git a/tests/routers/openml/users_delete_test.py b/tests/routers/openml/users_delete_test.py new file mode 100644 index 0000000..407a427 --- /dev/null +++ b/tests/routers/openml/users_delete_test.py @@ -0,0 +1,192 @@ +"""Tests for DELETE /users/{user_id} (Phase 1: no resources, self or admin).""" + +import uuid +from http import HTTPStatus + +import httpx +import pytest +from sqlalchemy import text +from sqlalchemy.ext.asyncio import AsyncConnection + +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, 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." + + +async def test_delete_user_not_found(py_api: httpx.AsyncClient) -> None: + response = await py_api.delete( + "/users/999999999", + params={"api_key": ApiKey.ADMIN}, + ) + assert response.status_code == HTTPStatus.NOT_FOUND + assert response.headers["content-type"] == "application/problem+json" + body = response.json() + assert body["type"] == UserNotFoundError.uri + assert body["detail"] == "User 999999999 not found." + + +async def test_delete_user_forbidden_non_admin_deletes_other( + py_api: httpx.AsyncClient, +) -> None: + response = await py_api.delete( + f"/users/{ADMIN_USER.user_id}", + params={"api_key": ApiKey.SOME_USER}, + ) + assert response.status_code == HTTPStatus.FORBIDDEN + body = response.json() + assert body["type"] == ForbiddenError.uri + assert body["detail"] == "You may only delete your own user account." + + +async def test_delete_user_conflict_when_user_has_resources( + py_api: httpx.AsyncClient, +) -> None: + """User 16 owns dataset 130 in the test database.""" + response = await py_api.delete( + "/users/16", + params={"api_key": ApiKey.ADMIN}, + ) + assert response.status_code == HTTPStatus.CONFLICT + assert response.headers["content-type"] == "application/problem+json" + body = response.json() + assert body["type"] == AccountHasResourcesError.uri + assert "datasets" in body["detail"] + + +@pytest.mark.mut +async def test_delete_user_api_success_self_delete( + py_api: httpx.AsyncClient, + user_test: AsyncConnection, +) -> None: + """Disposable user deletes their own account using their API key (session_hash).""" + api_key = "fedcba9876543210fedcba9876543210" + suffix = uuid.uuid4().hex[:10] + username = f"tmp_self_{suffix}" + email = f"{suffix}@openml-self-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}, + ) + + response = await py_api.delete( + f"/users/{new_id}", + params={"api_key": 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": new_id}, + ) + assert exists.one_or_none() is None + + +@pytest.mark.mut +async def test_delete_user_api_success_admin_deletes_disposable_user( + py_api: httpx.AsyncClient, + user_test: AsyncConnection, +) -> None: + suffix = uuid.uuid4().hex[:12] + username = f"tmp_del_{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 + ) VALUES ( + '127.0.0.1', :username, 'x', :email, UNIX_TIMESTAMP(), + '', '', '' + ) + """, + ), + parameters={"username": username, "email": email}, + ) + 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}, + ) + + response = await py_api.delete( + f"/users/{new_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": new_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\."): + await delete_user_account( + user_id=888888888, + current_user=ADMIN_USER, + expdb=expdb_test, + userdb=user_test, + ) + + +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\."): + await delete_user_account( + user_id=ADMIN_USER.user_id, + current_user=SOME_USER, + expdb=expdb_test, + userdb=user_test, + ) + + +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"): + await delete_user_account( + user_id=16, + current_user=ADMIN_USER, + expdb=expdb_test, + userdb=user_test, + )