From 2caa38a38a22eccf7ce45534e690f57b3f8d8e7a Mon Sep 17 00:00:00 2001 From: Yuri Chiucconi Date: Thu, 10 Oct 2024 12:44:07 +0200 Subject: [PATCH 1/8] delete endpoint and test --- fractal_server/app/routes/auth/group.py | 62 ++++++++++++++++++------ tests/no_version/test_api_user_groups.py | 50 +++++++++++++++++-- 2 files changed, 94 insertions(+), 18 deletions(-) diff --git a/fractal_server/app/routes/auth/group.py b/fractal_server/app/routes/auth/group.py index 907da51954..cfa10729a7 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,16 @@ 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 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__) @@ -174,18 +177,49 @@ async def update_single_group( 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 db.get(UserGroup, group_id) + + # Preliminary checks + + if group is None: + raise HTTPException( + status_code=status.HTTP_404_NOT_FOUND, + detail=f"UserGroup {group_id} not found.", + ) + 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/no_version/test_api_user_groups.py b/tests/no_version/test_api_user_groups.py index ca3bc50032..a04c0cecb3 100644 --- a/tests/no_version/test_api_user_groups.py +++ b/tests/no_version/test_api_user_groups.py @@ -93,7 +93,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 +129,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 +147,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: + pass else: raise RuntimeError("Wrong branch.") @@ -158,7 +162,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 +191,44 @@ 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) + from sqlmodel import select + from fractal_server.app.models import LinkUserGroup + from fractal_server.app.models.v2 import TaskGroupV2 + + 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): """ From 90b923a2e0de93e9af1ec0469a25984f5f3a8e78 Mon Sep 17 00:00:00 2001 From: Yuri Chiucconi Date: Thu, 10 Oct 2024 12:49:49 +0200 Subject: [PATCH 2/8] improve test --- tests/no_version/test_api_user_groups.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/no_version/test_api_user_groups.py b/tests/no_version/test_api_user_groups.py index a04c0cecb3..45901ea255 100644 --- a/tests/no_version/test_api_user_groups.py +++ b/tests/no_version/test_api_user_groups.py @@ -154,7 +154,7 @@ async def test_user_group_crud( elif group["name"] == "group 2": assert group["user_ids"] == [user_B_id] elif group["name"] == default_user_group.name: - pass + assert set(group["user_ids"]) == {user_A_id, user_B_id} else: raise RuntimeError("Wrong branch.") From 929dec719972b43033a8e145e6d1a7fa8ddb0ea4 Mon Sep 17 00:00:00 2001 From: Yuri Chiucconi Date: Thu, 10 Oct 2024 14:12:34 +0200 Subject: [PATCH 3/8] move imports [skip ci] --- tests/no_version/test_api_user_groups.py | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/tests/no_version/test_api_user_groups.py b/tests/no_version/test_api_user_groups.py index 45901ea255..5986683eb7 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" @@ -192,9 +197,6 @@ async def test_user_group_crud( assert "`new_user_ids` list has repetitions'" in str(res.json()) # DELETE (and cascade operations) - from sqlmodel import select - from fractal_server.app.models import LinkUserGroup - from fractal_server.app.models.v2 import TaskGroupV2 task_group = TaskGroupV2( user_id=user_A_id, From 7f1924fb14695e54ef6c660c7ae03271b206455c Mon Sep 17 00:00:00 2001 From: Yuri Chiucconi Date: Thu, 10 Oct 2024 14:17:50 +0200 Subject: [PATCH 4/8] rename usergroup functions --- fractal_server/app/routes/api/v2/_aux_functions_tasks.py | 4 ++-- fractal_server/app/routes/auth/_aux_auth.py | 4 ++-- fractal_server/app/routes/auth/group.py | 6 +++--- tests/fixtures_server_v2.py | 4 ++-- tests/no_version/test_unit_auth.py | 6 +++--- 5 files changed, 12 insertions(+), 12 deletions(-) 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..15e949a594 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: """ @@ -115,7 +115,7 @@ async def _user_or_404(user_id: int, db: AsyncSession) -> UserOAuth: 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 cfa10729a7..eeb70d68a0 100644 --- a/fractal_server/app/routes/auth/group.py +++ b/fractal_server/app/routes/auth/group.py @@ -13,7 +13,7 @@ from sqlmodel import select from . import current_active_superuser -from ._aux_auth import _get_single_group_with_user_ids +from ._aux_auth import _get_single_usergroup_with_user_ids from fractal_server.app.db import get_async_db from fractal_server.app.models import LinkUserGroup from fractal_server.app.models import UserGroup @@ -73,7 +73,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 @@ -170,7 +170,7 @@ 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 ) 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_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) From 006539ca1cdf63a1e4f8ea351ad103f49aa49bac Mon Sep 17 00:00:00 2001 From: Yuri Chiucconi Date: Thu, 10 Oct 2024 14:24:12 +0200 Subject: [PATCH 5/8] use 404 function --- fractal_server/app/routes/auth/_aux_auth.py | 23 ++++++++++++--------- fractal_server/app/routes/auth/group.py | 17 +++------------ 2 files changed, 16 insertions(+), 24 deletions(-) diff --git a/fractal_server/app/routes/auth/_aux_auth.py b/fractal_server/app/routes/auth/_aux_auth.py index 15e949a594..fe2b4b930a 100644 --- a/fractal_server/app/routes/auth/_aux_auth.py +++ b/fractal_server/app/routes/auth/_aux_auth.py @@ -80,15 +80,7 @@ async def _get_single_usergroup_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,7 +102,18 @@ 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 diff --git a/fractal_server/app/routes/auth/group.py b/fractal_server/app/routes/auth/group.py index eeb70d68a0..c549914e9f 100644 --- a/fractal_server/app/routes/auth/group.py +++ b/fractal_server/app/routes/auth/group.py @@ -14,6 +14,7 @@ from . import current_active_superuser 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 @@ -121,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 @@ -184,15 +180,8 @@ async def delete_single_group( db: AsyncSession = Depends(get_async_db), ) -> Response: - group = await db.get(UserGroup, group_id) - - # Preliminary checks + group = await _usergroup_or_404(group_id, db) - if group is None: - raise HTTPException( - status_code=status.HTTP_404_NOT_FOUND, - detail=f"UserGroup {group_id} not found.", - ) if group.name == FRACTAL_DEFAULT_GROUP_NAME: raise HTTPException( status_code=status.HTTP_422_UNPROCESSABLE_ENTITY, From f3ca4df348c89a3c8ed583daa787cb4dc4870463 Mon Sep 17 00:00:00 2001 From: Yuri Chiucconi Date: Thu, 10 Oct 2024 15:09:00 +0200 Subject: [PATCH 6/8] test access to task with group deletion --- tests/v2/03_api/test_loss_of_access_to_task.py | 14 ++++++++++++++ 1 file changed, 14 insertions(+) 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}/") From 3d1498e2958775ca29ce8e668c4704725289e87d Mon Sep 17 00:00:00 2001 From: Yuri Chiucconi Date: Thu, 10 Oct 2024 15:15:28 +0200 Subject: [PATCH 7/8] changelog [skip ci] --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8ab7d800f5..40c2ab6d12 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). * Database: * Add `taskgroupv2_id` foreign key to `CollectionStateV2` (\#1867). * Make `TaskV2.source` nullable and drop its uniqueness constraint (\#1861). From 89f78d6a45da7008961b81d2faf8183eff6b43a2 Mon Sep 17 00:00:00 2001 From: Yuri Chiucconi Date: Thu, 10 Oct 2024 15:22:57 +0200 Subject: [PATCH 8/8] delete test --- tests/no_version/test_api_user_groups.py | 9 --------- 1 file changed, 9 deletions(-) diff --git a/tests/no_version/test_api_user_groups.py b/tests/no_version/test_api_user_groups.py index 5986683eb7..d403449eb0 100644 --- a/tests/no_version/test_api_user_groups.py +++ b/tests/no_version/test_api_user_groups.py @@ -28,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.