diff --git a/CHANGELOG.md b/CHANGELOG.md index ec0a149b52..eaa1811d8d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -28,8 +28,9 @@ access-control rules (which will be introduced later). * Add creation of default user group into `fractalctl set-db` command (\#1738). * Create `update-db-script` for current version, that adds all users to default group (\#1738). * API: - * Added `/auth/group/` and `/auth/group-names/` routers (\#1738). - * Implement `/auth/users/{id}/` POST/PATCH routes in `fractal-server` (\#1738). + * Added `/auth/group/` and `/auth/group-names/` routers (\#1738, \#1752). + * Implement `/auth/users/{id}/` POST/PATCH routes in `fractal-server` (\#1738, \#1747, \#1752). + * Introduce `UserUpdateWithNewGroupIds` schema for `PATCH /auth/users/{id}/` (\#1747, \#1752). * Add `UserManager.on_after_register` hook to add new users to default user group (\#1738). * Database: * Added new `usergroup` and `linkusergroup` tables (\#1738). diff --git a/fractal_server/__init__.py b/fractal_server/__init__.py index e918bc48ad..7a4aa420f5 100644 --- a/fractal_server/__init__.py +++ b/fractal_server/__init__.py @@ -1 +1 @@ -__VERSION__ = "2.4.0a0" +__VERSION__ = "2.4.0a2" diff --git a/fractal_server/app/routes/auth/group.py b/fractal_server/app/routes/auth/group.py index d158596eb5..4bc489b604 100644 --- a/fractal_server/app/routes/auth/group.py +++ b/fractal_server/app/routes/auth/group.py @@ -5,8 +5,10 @@ from fastapi import Depends from fastapi import HTTPException from fastapi import status +from sqlalchemy.exc import IntegrityError from sqlalchemy.ext.asyncio import AsyncSession from sqlmodel import col +from sqlmodel import func from sqlmodel import select from . import current_active_superuser @@ -18,7 +20,9 @@ from fractal_server.app.models import LinkUserGroup from fractal_server.app.models import UserGroup from fractal_server.app.models import UserOAuth +from fractal_server.logger import set_logger +logger = set_logger(__name__) router_group = APIRouter() @@ -31,9 +35,6 @@ async def get_list_user_groups( user: UserOAuth = Depends(current_active_superuser), db: AsyncSession = Depends(get_async_db), ) -> list[UserGroupRead]: - """ - FIXME docstring - """ # Get all groups stm_all_groups = select(UserGroup) @@ -46,7 +47,8 @@ async def get_list_user_groups( res = await db.execute(stm_all_links) links = res.scalars().all() - # FIXME GROUPS: this must be optimized + # TODO: possible optimizations for this construction are listed in + # https://github.com/fractal-analytics-platform/fractal-server/issues/1742 for ind, group in enumerate(groups): groups[ind] = dict( group.model_dump(), @@ -68,9 +70,6 @@ async def get_single_user_group( user: UserOAuth = Depends(current_active_superuser), db: AsyncSession = Depends(get_async_db), ) -> UserGroupRead: - """ - FIXME docstring - """ group = await _get_single_group_with_user_ids(group_id=group_id, db=db) return group @@ -85,9 +84,6 @@ async def create_single_group( user: UserOAuth = Depends(current_active_superuser), db: AsyncSession = Depends(get_async_db), ) -> UserGroupRead: - """ - FIXME docstring - """ # Check that name is not already in use existing_name_str = select(UserGroup).where( @@ -119,24 +115,21 @@ async def update_single_group( user: UserOAuth = Depends(current_active_superuser), db: AsyncSession = Depends(get_async_db), ) -> UserGroupRead: - """ - FIXME docstring - """ # Check that all required users exist # Note: The reason for introducing `col` is as in # https://sqlmodel.tiangolo.com/tutorial/where/#type-annotations-and-errors, - stm = select(UserOAuth).where( + stm = select(func.count()).where( col(UserOAuth.id).in_(group_update.new_user_ids) ) res = await db.execute(stm) - matching_users = res.scalars().unique().all() - if not len(matching_users) == len(group_update.new_user_ids): + number_matching_users = res.scalar() + if number_matching_users != len(group_update.new_user_ids): raise HTTPException( status_code=status.HTTP_404_NOT_FOUND, detail=( - f"At least user with IDs {group_update.new_user_ids} " - "does not exist." + f"Not all requested users (IDs {group_update.new_user_ids}) " + "exist." ), ) @@ -144,7 +137,20 @@ async def update_single_group( for user_id in group_update.new_user_ids: link = LinkUserGroup(user_id=user_id, group_id=group_id) db.add(link) - await db.commit() + try: + await db.commit() + except IntegrityError as e: + error_msg = ( + f"Cannot link users with IDs {group_update.new_user_ids} " + f"to group {group_id}. " + "Likely reason: one of these links already exists.\n" + f"Original error: {str(e)}" + ) + logger.info(error_msg) + raise HTTPException( + status_code=status.HTTP_422_UNPROCESSABLE_ENTITY, + detail=error_msg, + ) updated_group = await _get_single_group_with_user_ids( group_id=group_id, db=db @@ -161,9 +167,6 @@ async def delete_single_group( user: UserOAuth = Depends(current_active_superuser), db: AsyncSession = Depends(get_async_db), ) -> UserGroupRead: - """ - FIXME docstring - """ raise HTTPException( status_code=status.HTTP_405_METHOD_NOT_ALLOWED, detail=( diff --git a/fractal_server/app/routes/auth/users.py b/fractal_server/app/routes/auth/users.py index 1a70ba4bc0..fb2b2e8a3f 100644 --- a/fractal_server/app/routes/auth/users.py +++ b/fractal_server/app/routes/auth/users.py @@ -8,23 +8,32 @@ from fastapi_users import exceptions from fastapi_users import schemas from fastapi_users.router.common import ErrorCode +from sqlalchemy.exc import IntegrityError from sqlalchemy.ext.asyncio import AsyncSession +from sqlmodel import col +from sqlmodel import func from sqlmodel import select from . import current_active_superuser from ...db import get_async_db from ...schemas.user import UserRead from ...schemas.user import UserUpdate +from ...schemas.user import UserUpdateWithNewGroupIds from ._aux_auth import _get_single_user_with_group_ids from fractal_server.app.models import LinkUserGroup +from fractal_server.app.models import UserGroup from fractal_server.app.models import UserOAuth from fractal_server.app.routes.auth._aux_auth import _user_or_404 from fractal_server.app.security import get_user_manager from fractal_server.app.security import UserManager +from fractal_server.logger import set_logger router_users = APIRouter() +logger = set_logger(__name__) + + @router_users.get("/users/{user_id}/", response_model=UserRead) async def get_user( user_id: int, @@ -43,31 +52,110 @@ async def get_user( @router_users.patch("/users/{user_id}/", response_model=UserRead) async def patch_user( user_id: int, - user_update: UserUpdate, + user_update: UserUpdateWithNewGroupIds, current_superuser: UserOAuth = Depends(current_active_superuser), user_manager: UserManager = Depends(get_user_manager), db: AsyncSession = Depends(get_async_db), ): """ Custom version of the PATCH-user route from `fastapi-users`. + + In order to keep the fastapi-users logic in place (which is convenient to + update user attributes), we split the endpoint into two branches. We either + go through the fastapi-users-based attribute-update branch, or through the + branch where we establish new user/group relationships. + + Note that we prevent making both changes at the same time, since it would + be more complex to guarantee that endpoint error would leave the database + in the same state as before the API call. """ + # We prevent simultaneous editing of both user attributes and user/group + # associations + user_update_dict_without_groups = user_update.dict( + exclude_unset=True, exclude={"new_group_ids"} + ) + edit_attributes = user_update_dict_without_groups != {} + edit_groups = user_update.new_group_ids is not None + if edit_attributes and edit_groups: + raise HTTPException( + status_code=status.HTTP_422_UNPROCESSABLE_ENTITY, + detail=( + "Cannot modify both user attributes and group membership. " + "Please make two independent PATCH calls" + ), + ) + + # Check that user exists user_to_patch = await _user_or_404(user_id, db) - try: - user = await user_manager.update( - user_update, user_to_patch, safe=False, request=None - ) - patched_user = schemas.model_validate(UserOAuth, user) - except exceptions.InvalidPasswordException as e: - raise HTTPException( - status_code=status.HTTP_400_BAD_REQUEST, - detail={ - "code": ErrorCode.UPDATE_USER_INVALID_PASSWORD, - "reason": e.reason, - }, + if edit_groups: + # Establish new user/group relationships + + # Check that all required groups exist + # Note: The reason for introducing `col` is as in + # https://sqlmodel.tiangolo.com/tutorial/where/#type-annotations-and-errors, + stm = select(func.count()).where( + col(UserGroup.id).in_(user_update.new_group_ids) ) + res = await db.execute(stm) + number_matching_groups = res.scalar() + if number_matching_groups != len(user_update.new_group_ids): + raise HTTPException( + status_code=status.HTTP_404_NOT_FOUND, + detail=( + "Not all requested groups (IDs: " + f"{user_update.new_group_ids}) exist." + ), + ) + + for new_group_id in user_update.new_group_ids: + link = LinkUserGroup(user_id=user_id, group_id=new_group_id) + db.add(link) + + try: + await db.commit() + except IntegrityError as e: + error_msg = ( + f"Cannot link groups with IDs {user_update.new_group_ids} " + f"to user {user_id}. " + "Likely reason: one of these links already exists.\n" + f"Original error: {str(e)}" + ) + logger.info(error_msg) + raise HTTPException( + status_code=status.HTTP_422_UNPROCESSABLE_ENTITY, + detail=error_msg, + ) + + patched_user = user_to_patch + + elif edit_attributes: + # Modify user attributes + try: + user_update_without_groups = UserUpdate( + **user_update_dict_without_groups + ) + user = await user_manager.update( + user_update_without_groups, + user_to_patch, + safe=False, + request=None, + ) + patched_user = schemas.model_validate(UserOAuth, user) + except exceptions.InvalidPasswordException as e: + raise HTTPException( + status_code=status.HTTP_400_BAD_REQUEST, + detail={ + "code": ErrorCode.UPDATE_USER_INVALID_PASSWORD, + "reason": e.reason, + }, + ) + else: + # Nothing to do, just continue + patched_user = user_to_patch + # Enrich user object with `group_ids` attribute patched_user_with_group_ids = await _get_single_user_with_group_ids( patched_user, db ) @@ -92,7 +180,8 @@ async def list_users( res = await db.execute(stm_all_links) links = res.scalars().all() - # FIXME GROUPS: this must be optimized + # TODO: possible optimizations for this construction are listed in + # https://github.com/fractal-analytics-platform/fractal-server/issues/1742 for ind, user in enumerate(user_list): user_list[ind] = dict( user.model_dump(), diff --git a/fractal_server/app/schemas/user.py b/fractal_server/app/schemas/user.py index 2f967ddf5e..270797e0f2 100644 --- a/fractal_server/app/schemas/user.py +++ b/fractal_server/app/schemas/user.py @@ -16,6 +16,7 @@ "UserRead", "UserUpdate", "UserCreate", + "UserUpdateWithNewGroupIds", ) @@ -102,6 +103,14 @@ class UserUpdateStrict(BaseModel, extra=Extra.forbid): ) +class UserUpdateWithNewGroupIds(UserUpdate): + new_group_ids: Optional[list[int]] = None + + _val_unique = validator("new_group_ids", allow_reuse=True)( + val_unique_list("new_group_ids") + ) + + class UserCreate(schemas.BaseUserCreate): """ Schema for `User` creation. diff --git a/fractal_server/app/schemas/user_group.py b/fractal_server/app/schemas/user_group.py index e67764188b..eb0cdf85d2 100644 --- a/fractal_server/app/schemas/user_group.py +++ b/fractal_server/app/schemas/user_group.py @@ -4,6 +4,10 @@ from pydantic import BaseModel from pydantic import Extra from pydantic import Field +from pydantic import validator + +from ._validators import val_unique_list + __all__ = ( "UserGroupRead", @@ -55,3 +59,7 @@ class UserGroupUpdate(BaseModel, extra=Extra.forbid): """ new_user_ids: list[int] = Field(default_factory=list) + + _val_unique = validator("new_user_ids", allow_reuse=True)( + val_unique_list("new_user_ids") + ) diff --git a/pyproject.toml b/pyproject.toml index bdbc02e863..dc5da90047 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -1,6 +1,6 @@ [tool.poetry] name = "fractal-server" -version = "2.4.0a0" +version = "2.4.0a2" description = "Server component of the Fractal analytics platform" authors = [ "Tommaso Comparin ", @@ -92,7 +92,7 @@ filterwarnings = [ ] [tool.bumpver] -current_version = "2.4.0a0" +current_version = "2.4.0a2" version_pattern = "MAJOR.MINOR.PATCH[PYTAGNUM]" commit_message = "bump version {old_version} -> {new_version}" commit = true diff --git a/tests/no_version/test_auth_api.py b/tests/no_version/test_auth_api.py index a573cd65b2..239ca21c7c 100644 --- a/tests/no_version/test_auth_api.py +++ b/tests/no_version/test_auth_api.py @@ -411,3 +411,100 @@ async def test_delete_user(registered_client, registered_superuser_client): f"{PREFIX}/users/THIS-IS-NOT-AN-ID" ) assert res.status_code == 404 + + +async def test_add_groups_to_user_as_superuser(registered_superuser_client): + + # Create user + res = await registered_superuser_client.post( + f"{PREFIX}/register/", + json=dict( + email="test@fractal.xy", + password="12345", + slurm_accounts=["foo", "bar"], + ), + ) + assert res.status_code == 201 + user_id = res.json()["id"] + res = await registered_superuser_client.get(f"{PREFIX}/users/{user_id}/") + assert res.status_code == 200 + user = res.json() + debug(user) + assert user["group_ids"] == [] + + # Create group + res = await registered_superuser_client.post( + f"{PREFIX}/group/", + json=dict(name="groupname"), + ) + assert res.status_code == 201 + group_id = res.json()["id"] + + # Create user/group link and fail because of invalid `group_id`` + invalid_group_id = 999999 + res = await registered_superuser_client.patch( + f"{PREFIX}/users/{user_id}/", + json=dict(new_group_ids=[invalid_group_id]), + ) + assert res.status_code == 404 + + # Create user/group link and succeed + res = await registered_superuser_client.patch( + f"{PREFIX}/users/{user_id}/", + json=dict(new_group_ids=[group_id]), + ) + assert res.status_code == 200 + assert res.json()["group_ids"] == [group_id] + + # Create user/group link and fail because it already exists + res = await registered_superuser_client.patch( + f"{PREFIX}/users/{user_id}/", + json=dict(new_group_ids=[group_id]), + ) + assert res.status_code == 422 + hint_msg = "Likely reason: one of these links already exists" + assert hint_msg in res.json()["detail"] + + # Create user/group link and fail because of repeated IDs + res = await registered_superuser_client.patch( + f"{PREFIX}/users/{user_id}/", + json=dict(new_group_ids=[99, 99]), + ) + assert res.status_code == 422 + assert "`new_group_ids` list has repetitions'" in str(res.json()) + + +async def test_edit_user_and_fail(registered_superuser_client): + + # Create user + res = await registered_superuser_client.post( + f"{PREFIX}/register/", + json=dict( + email="test@fractal.xy", + password="12345", + slurm_accounts=["foo", "bar"], + ), + ) + assert res.status_code == 201 + user_id = res.json()["id"] + + # Patch both user attributes and user/group relationship, and fail + res = await registered_superuser_client.patch( + f"{PREFIX}/users/{user_id}/", + json=dict( + slurm_user="new-slurm-user", + new_group_ids=[], + ), + ) + assert res.status_code == 422 + expected_detail = ( + "Cannot modify both user attributes and group membership." + ) + assert expected_detail in res.json()["detail"] + + # Make a dummy patch to user, and succeed + res = await registered_superuser_client.patch( + f"{PREFIX}/users/{user_id}/", + json={}, + ) + assert res.status_code == 200 diff --git a/tests/no_version/test_auth_groups_api.py b/tests/no_version/test_auth_groups_api.py index 4bd6701600..0855837f18 100644 --- a/tests/no_version/test_auth_groups_api.py +++ b/tests/no_version/test_auth_groups_api.py @@ -73,7 +73,7 @@ async def test_update_group(registered_superuser_client): assert res.status_code == 200 assert res.json()["user_ids"] == [] - # Patch an existing group by adding a valid users + # Patch an existing group by adding a valid user res = await registered_superuser_client.patch( f"{PREFIX}/group/{group_id}/", json=dict(new_user_ids=[user_A_id]), @@ -151,6 +151,31 @@ async def test_user_group_crud(registered_superuser_client): for group in groups_data: assert group["user_ids"] is None + # Add users A and B to group 2, and fail because user B is already there + res = await registered_superuser_client.patch( + f"{PREFIX}/group/{group_2_id}/", + json=dict(new_user_ids=[user_A_id, user_B_id]), + ) + assert res.status_code == 422 + hint_msg = "Likely reason: one of these links already exists" + assert hint_msg in res.json()["detail"] + + # After the previous 422, verify that user A was not added to group 2 + # (that is, verify that `db.commit` is atomic) + res = await registered_superuser_client.get( + f"{PREFIX}/group/{group_2_id}/" + ) + assert res.status_code == 200 + assert user_A_id not in res.json()["user_ids"] + + # Create user/group link and fail because of repeated IDs + res = await registered_superuser_client.patch( + f"{PREFIX}/group/{group_1_id}/", + json=dict(new_user_ids=[99, 99]), + ) + assert res.status_code == 422 + assert "`new_user_ids` list has repetitions'" in str(res.json()) + async def test_get_user_group_names( client, registered_client, registered_superuser_client