diff --git a/CHANGELOG.md b/CHANGELOG.md index 27f0af4128..5aafad7895 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -29,6 +29,7 @@ into pre-release sections below. * Do not process task sources in task/task-group CRUD operations (\#1861). * Do not process task owners in task/task-group CRUD operations (\#1861). * Expand use and validators for `TaskGroupCreateV2` schema (\#1861). + * Add `DELETE /auth/group/{id}` endpoint (\#1885). * Forbid extras in `TaskCollectPipV2` (\#1891). * Database: * Add `taskgroupv2_id` foreign key to `CollectionStateV2` (\#1867). diff --git a/fractal_server/app/routes/api/v2/_aux_functions_tasks.py b/fractal_server/app/routes/api/v2/_aux_functions_tasks.py index 56e5a113d9..8107e3726e 100644 --- a/fractal_server/app/routes/api/v2/_aux_functions_tasks.py +++ b/fractal_server/app/routes/api/v2/_aux_functions_tasks.py @@ -14,7 +14,7 @@ from ....models.v2 import TaskGroupV2 from ....models.v2 import TaskV2 from ....models.v2 import WorkflowTaskV2 -from ...auth._aux_auth import _get_default_user_group_id +from ...auth._aux_auth import _get_default_usergroup_id from ...auth._aux_auth import _verify_user_belongs_to_group @@ -203,7 +203,7 @@ async def _get_valid_user_group_id( elif private is True: user_group_id = None elif user_group_id is None: - user_group_id = await _get_default_user_group_id(db=db) + user_group_id = await _get_default_usergroup_id(db=db) else: await _verify_user_belongs_to_group( user_id=user_id, user_group_id=user_group_id, db=db diff --git a/fractal_server/app/routes/auth/_aux_auth.py b/fractal_server/app/routes/auth/_aux_auth.py index 1644256f8c..fe2b4b930a 100644 --- a/fractal_server/app/routes/auth/_aux_auth.py +++ b/fractal_server/app/routes/auth/_aux_auth.py @@ -66,7 +66,7 @@ async def _get_single_user_with_groups( ) -async def _get_single_group_with_user_ids( +async def _get_single_usergroup_with_user_ids( group_id: int, db: AsyncSession ) -> UserGroupRead: """ @@ -80,15 +80,7 @@ async def _get_single_group_with_user_ids( `UserGroupRead` object, with `user_ids` attribute populated from database. """ - # Get the UserGroup object from the database - stm_group = select(UserGroup).where(UserGroup.id == group_id) - res = await db.execute(stm_group) - group = res.scalars().one_or_none() - if group is None: - raise HTTPException( - status_code=status.HTTP_404_NOT_FOUND, - detail=f"Group {group_id} not found.", - ) + group = await _usergroup_or_404(group_id, db) # Get all user/group links stm_links = select(LinkUserGroup).where(LinkUserGroup.group_id == group_id) @@ -110,12 +102,23 @@ async def _user_or_404(user_id: int, db: AsyncSession) -> UserOAuth: user = await db.get(UserOAuth, user_id, populate_existing=True) if user is None: raise HTTPException( - status_code=status.HTTP_404_NOT_FOUND, detail="User not found." + status_code=status.HTTP_404_NOT_FOUND, + detail=f"User {user_id} not found.", + ) + return user + + +async def _usergroup_or_404(usergroup_id: int, db: AsyncSession) -> UserGroup: + user = await db.get(UserGroup, usergroup_id) + if user is None: + raise HTTPException( + status_code=status.HTTP_404_NOT_FOUND, + detail=f"UserGroup {usergroup_id} not found.", ) return user -async def _get_default_user_group_id(db: AsyncSession) -> int: +async def _get_default_usergroup_id(db: AsyncSession) -> int: stm = select(UserGroup.id).where( UserGroup.name == FRACTAL_DEFAULT_GROUP_NAME ) diff --git a/fractal_server/app/routes/auth/group.py b/fractal_server/app/routes/auth/group.py index 907da51954..c549914e9f 100644 --- a/fractal_server/app/routes/auth/group.py +++ b/fractal_server/app/routes/auth/group.py @@ -4,6 +4,7 @@ from fastapi import APIRouter from fastapi import Depends from fastapi import HTTPException +from fastapi import Response from fastapi import status from sqlalchemy.exc import IntegrityError from sqlalchemy.ext.asyncio import AsyncSession @@ -12,14 +13,17 @@ from sqlmodel import select from . import current_active_superuser -from ...db import get_async_db -from ...schemas.user_group import UserGroupCreate -from ...schemas.user_group import UserGroupRead -from ...schemas.user_group import UserGroupUpdate -from ._aux_auth import _get_single_group_with_user_ids +from ._aux_auth import _get_single_usergroup_with_user_ids +from ._aux_auth import _usergroup_or_404 +from fractal_server.app.db import get_async_db 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.models.v2 import TaskGroupV2 +from fractal_server.app.schemas.user_group import UserGroupCreate +from fractal_server.app.schemas.user_group import UserGroupRead +from fractal_server.app.schemas.user_group import UserGroupUpdate +from fractal_server.app.security import FRACTAL_DEFAULT_GROUP_NAME from fractal_server.logger import set_logger logger = set_logger(__name__) @@ -70,7 +74,7 @@ async def get_single_user_group( user: UserOAuth = Depends(current_active_superuser), db: AsyncSession = Depends(get_async_db), ) -> UserGroupRead: - group = await _get_single_group_with_user_ids(group_id=group_id, db=db) + group = await _get_single_usergroup_with_user_ids(group_id=group_id, db=db) return group @@ -118,12 +122,7 @@ async def update_single_group( db: AsyncSession = Depends(get_async_db), ) -> UserGroupRead: - group = await db.get(UserGroup, group_id) - if group is None: - raise HTTPException( - status_code=status.HTTP_404_NOT_FOUND, - detail=f"UserGroup {group_id} not found.", - ) + group = await _usergroup_or_404(group_id, db) # Check that all required users exist # Note: The reason for introducing `col` is as in @@ -167,25 +166,49 @@ async def update_single_group( db.add(group) await db.commit() - updated_group = await _get_single_group_with_user_ids( + updated_group = await _get_single_usergroup_with_user_ids( group_id=group_id, db=db ) return updated_group -@router_group.delete( - "/group/{group_id}/", status_code=status.HTTP_405_METHOD_NOT_ALLOWED -) +@router_group.delete("/group/{group_id}/", status_code=204) async def delete_single_group( group_id: int, user: UserOAuth = Depends(current_active_superuser), db: AsyncSession = Depends(get_async_db), -) -> UserGroupRead: - raise HTTPException( - status_code=status.HTTP_405_METHOD_NOT_ALLOWED, - detail=( - "Deleting a user group is not allowed, as it may restrict " - "previously-granted access.", - ), +) -> Response: + + group = await _usergroup_or_404(group_id, db) + + if group.name == FRACTAL_DEFAULT_GROUP_NAME: + raise HTTPException( + status_code=status.HTTP_422_UNPROCESSABLE_ENTITY, + detail=( + "Cannot delete default UserGroup " + f"'{FRACTAL_DEFAULT_GROUP_NAME}'." + ), + ) + + # Cascade operations + + res = await db.execute( + select(LinkUserGroup).where(LinkUserGroup.group_id == group_id) + ) + for link in res.scalars().all(): + await db.delete(link) + + res = await db.execute( + select(TaskGroupV2).where(TaskGroupV2.user_group_id == group_id) ) + for task_group in res.scalars().all(): + task_group.user_group_id = None + db.add(task_group) + + # Delete + + await db.delete(group) + await db.commit() + + return Response(status_code=status.HTTP_204_NO_CONTENT) diff --git a/tests/fixtures_server_v2.py b/tests/fixtures_server_v2.py index aebf1a82d6..35b01aff57 100644 --- a/tests/fixtures_server_v2.py +++ b/tests/fixtures_server_v2.py @@ -13,7 +13,7 @@ from fractal_server.app.models.v2 import WorkflowTaskV2 from fractal_server.app.models.v2 import WorkflowV2 from fractal_server.app.routes.api.v2.submit import _encode_as_utc -from fractal_server.app.routes.auth._aux_auth import _get_default_user_group_id +from fractal_server.app.routes.auth._aux_auth import _get_default_usergroup_id from fractal_server.app.routes.auth._aux_auth import ( _verify_user_belongs_to_group, ) @@ -229,7 +229,7 @@ async def __task_factory( args.update(kwargs) task = TaskV2(**args) if user_group_id is None: - user_group_id = await _get_default_user_group_id(db=db) + user_group_id = await _get_default_usergroup_id(db=db) else: await _verify_user_belongs_to_group( user_id=user_id, user_group_id=user_group_id, db=db diff --git a/tests/no_version/test_api_user_groups.py b/tests/no_version/test_api_user_groups.py index ca3bc50032..d403449eb0 100644 --- a/tests/no_version/test_api_user_groups.py +++ b/tests/no_version/test_api_user_groups.py @@ -1,3 +1,8 @@ +from sqlmodel import select + +from fractal_server.app.models import LinkUserGroup +from fractal_server.app.models.v2 import TaskGroupV2 + PREFIX = "/auth" @@ -23,15 +28,6 @@ async def test_no_access_user_group_api(client, registered_client): assert res.status_code == expected_status -async def test_delete_user_group_not_allowed(registered_superuser_client): - """ - Verify that the user-group DELETE endpoint responds with - "405 Method Not Allowed". - """ - res = await registered_superuser_client.delete(f"{PREFIX}/group/1/") - assert res.status_code == 405 - - async def test_update_group(registered_superuser_client): """ Modifying a group with an invalid user ID returns a 404. @@ -93,7 +89,9 @@ async def test_update_group(registered_superuser_client): assert res.json()["viewer_paths"] == ["/new"] -async def test_user_group_crud(registered_superuser_client): +async def test_user_group_crud( + registered_superuser_client, db, default_user_group +): """ Test basic working of POST/GET/PATCH for user groups. """ @@ -127,7 +125,7 @@ async def test_user_group_crud(registered_superuser_client): assert res.status_code == 201 group_2_id = res.json()["id"] - # Add user A to group 1 + # Add user A and B to group 1 res = await registered_superuser_client.patch( f"{PREFIX}/group/{group_1_id}/", json=dict(new_user_ids=[user_A_id, user_B_id]), @@ -145,12 +143,14 @@ async def test_user_group_crud(registered_superuser_client): ) assert res.status_code == 200 groups_data = res.json() - assert len(groups_data) == 2 + assert len(groups_data) == 3 for group in groups_data: if group["name"] == "group 1": assert set(group["user_ids"]) == {user_A_id, user_B_id} elif group["name"] == "group 2": assert group["user_ids"] == [user_B_id] + elif group["name"] == default_user_group.name: + assert set(group["user_ids"]) == {user_A_id, user_B_id} else: raise RuntimeError("Wrong branch.") @@ -158,7 +158,7 @@ async def test_user_group_crud(registered_superuser_client): res = await registered_superuser_client.get(f"{PREFIX}/group/") assert res.status_code == 200 groups_data = res.json() - assert len(groups_data) == 2 + assert len(groups_data) == 3 for group in groups_data: assert group["user_ids"] is None @@ -187,6 +187,41 @@ async def test_user_group_crud(registered_superuser_client): assert res.status_code == 422 assert "`new_user_ids` list has repetitions'" in str(res.json()) + # DELETE (and cascade operations) + + task_group = TaskGroupV2( + user_id=user_A_id, + user_group_id=group_1_id, + origin="pypi", + pkg_name="fractal-tasks-core", + ) + db.add(task_group) + await db.commit() + await db.refresh(task_group) + assert task_group.user_group_id == group_1_id + + res = await registered_superuser_client.delete( # actual DELETE + f"{PREFIX}/group/{group_1_id}/" + ) + assert res.status_code == 204 + res = await registered_superuser_client.delete( + f"{PREFIX}/group/{group_1_id}/" + ) + assert res.status_code == 404 + res = await registered_superuser_client.delete( + f"{PREFIX}/group/{default_user_group.id}/" + ) + assert res.status_code == 422 + + # test cascade operations + res = await db.execute( + select(LinkUserGroup).where(LinkUserGroup.group_id == group_1_id) + ) + links = res.scalars().all() + assert links == [] + await db.refresh(task_group) + assert task_group.user_group_id is None + async def test_create_user_group_same_name(registered_superuser_client): """ diff --git a/tests/no_version/test_unit_auth.py b/tests/no_version/test_unit_auth.py index cd002ed4bd..423f949d4e 100644 --- a/tests/no_version/test_unit_auth.py +++ b/tests/no_version/test_unit_auth.py @@ -5,10 +5,10 @@ from fractal_server.app.models import UserOAuth from fractal_server.app.routes.auth._aux_auth import ( - _get_single_group_with_user_ids, + _get_single_user_with_groups, ) from fractal_server.app.routes.auth._aux_auth import ( - _get_single_user_with_groups, + _get_single_usergroup_with_user_ids, ) from fractal_server.app.routes.auth._aux_auth import _user_or_404 from fractal_server.app.routes.auth._aux_auth import ( @@ -34,7 +34,7 @@ async def test_user_or_404(db): async def test_get_single_group_with_user_ids(db): with pytest.raises(HTTPException) as exc_info: - await _get_single_group_with_user_ids(group_id=9999, db=db) + await _get_single_usergroup_with_user_ids(group_id=9999, db=db) debug(exc_info.value) diff --git a/tests/v2/03_api/test_loss_of_access_to_task.py b/tests/v2/03_api/test_loss_of_access_to_task.py index ce597fc4d8..c17d54f85f 100644 --- a/tests/v2/03_api/test_loss_of_access_to_task.py +++ b/tests/v2/03_api/test_loss_of_access_to_task.py @@ -1,3 +1,5 @@ +import pytest + from fractal_server.app.models import LinkUserGroup from fractal_server.app.models import UserGroup from fractal_server.app.models import UserOAuth @@ -7,7 +9,9 @@ ) +@pytest.mark.parametrize("remove", ("user_from_group", "group")) async def test_loss_of_access_to_task( + remove, MockCurrentUser, task_factory_v2, project_factory_v2, @@ -85,6 +89,7 @@ async def test_loss_of_access_to_task( assert res.status_code == 200 assert res.json()["status"] == "done" + if remove == "user_from_group": # Remove user_A from team group link = await db.get(LinkUserGroup, (team_group.id, user_A.id)) assert link is not None @@ -92,6 +97,15 @@ async def test_loss_of_access_to_task( await db.commit() link = await db.get(LinkUserGroup, (team_group.id, user_A.id)) assert link is None + elif remove == "group": + # Remove the team group + async with MockCurrentUser(user_kwargs=dict(is_superuser=True)): + res = await client.delete(f"/auth/group/{team_group.id}/") + assert res.status_code == 204 + else: + raise RuntimeError("Wrong branch.") + + async with MockCurrentUser(user_kwargs=dict(id=user_A.id)) as user: # User A cannot get task_B any more res = await client.get(f"/api/v2/task/{task_B.id}/")