Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions src/core/errors.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
# =============================================================================
Expand Down
49 changes: 49 additions & 0 deletions src/database/users.py
Original file line number Diff line number Diff line change
Expand Up @@ -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},
)
Comment thread
coderabbitai[bot] marked this conversation as resolved.
2 changes: 2 additions & 0 deletions src/main.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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)
Expand Down
66 changes: 66 additions & 0 deletions src/routers/openml/users.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,66 @@
"""User account HTTP endpoints."""

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

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 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.
"""
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
Comment thread
igennova marked this conversation as resolved.

Comment thread
igennova marked this conversation as resolved.
logger.info("User account {user_id} was removed.", user_id=user_id)
return Response(status_code=204)
212 changes: 212 additions & 0 deletions tests/routers/openml/users_delete_test.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,212 @@
"""Tests for DELETE /users/{user_id} (Phase 1: no resources, self or admin)."""

import uuid
from collections.abc import AsyncGenerator
from http import HTTPStatus
from typing import NamedTuple

import httpx
import pytest
import pytest_mock
from sqlalchemy import text
from sqlalchemy.exc import IntegrityError
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."


class DisposableUser(NamedTuple):
user_id: int
api_key: str


@pytest.fixture
async def disposable_user(user_test: AsyncConnection) -> AsyncGenerator[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},
)
yield DisposableUser(user_id=new_id, api_key=api_key)
await user_test.execute(
text("DELETE FROM users_groups WHERE user_id = :uid"),
parameters={"uid": new_id},
)
await user_test.execute(
text("DELETE FROM users WHERE id = :uid"),
parameters={"uid": new_id},
)


@pytest.mark.mut
async def test_delete_user_api_success_self_delete(
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": 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


@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


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=16,
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


@pytest.mark.mut
async def test_delete_user_direct_success_logs_info(
user_test: AsyncConnection,
expdb_test: AsyncConnection,
disposable_user: DisposableUser,
mocker: pytest_mock.MockerFixture,
) -> None:
log_info = mocker.patch("routers.openml.users.logger.info")

response = await delete_user_account(
user_id=disposable_user.user_id,
current_user=ADMIN_USER,
expdb=expdb_test,
userdb=user_test,
)

assert response.status_code == HTTPStatus.NO_CONTENT
log_info.assert_called_once_with(
"User account {user_id} was removed.",
user_id=disposable_user.user_id,
)


@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,
)
Loading