Merge pull request #1885 from fractal-analytics-platform/1845-allow-d…

…eleting-a-usergroup-apart-from-the-default-all-one

Allow deleting a UserGroup (except for the default one)

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).

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

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
     )

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)

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

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,20 +143,22 @@ 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.")
 
     # Get all groups (group 1, group 2) without user_ids
     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):
     """

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)
 
 

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,13 +89,23 @@ 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
         await db.delete(link)
         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}/")