[DOP-19926] Provide guest role when changing group owner

Author: Ilyas Gasanov

docs/changelog/next_release/135.feature.rst

@@ -0,0 +1 @@
+Grant read-only rights for the previous group owner when ownership is transferred

syncmaster/backend/api/v1/groups.py

@@ -4,10 +4,11 @@
 
 from syncmaster.backend.services import UnitOfWork, get_user
 from syncmaster.db.models import User
+from syncmaster.db.models.group import GroupMemberRole
 from syncmaster.db.utils import Permission
 from syncmaster.errors.registration import get_error_responses
 from syncmaster.exceptions import ActionNotAllowedError
-from syncmaster.exceptions.group import GroupNotFoundError
+from syncmaster.exceptions.group import AlreadyIsGroupOwnerError, GroupNotFoundError
 from syncmaster.schemas.v1.groups import (
     AddUserSchema,
     CreateGroupSchema,
@@ -104,12 +105,29 @@ async def update_group(
         raise ActionNotAllowedError
 
     async with unit_of_work:
+        group = await unit_of_work.group.read_by_id(group_id=group_id)
+        previous_owner_id = group.owner_id
+
         group = await unit_of_work.group.update(
             group_id=group_id,
             owner_id=group_data.owner_id,
             name=group_data.name,
             description=group_data.description,
         )
+
+        if previous_owner_id != group_data.owner_id:
+            new_owner_user_group = await unit_of_work.group.get_user_group(
+                group_id=group_id,
+                user_id=group_data.owner_id,
+            )
+            if new_owner_user_group:
+                await unit_of_work.group.delete_user(group_id=group_id, target_user_id=group_data.owner_id)
+
+            await unit_of_work.group.add_user(
+                group_id=group_id,
+                new_user_id=previous_owner_id,
+                role=GroupMemberRole.Guest,
+            )
     return ReadGroupSchema.from_orm(group)
 
 
@@ -196,6 +214,10 @@ async def add_user_to_group(
         raise ActionNotAllowedError
 
     async with unit_of_work:
+        group = await unit_of_work.group.read_by_id(group_id=group_id)
+        if group.owner_id == user_id:
+            raise AlreadyIsGroupOwnerError
+
         await unit_of_work.group.add_user(
             group_id=group_id,
             new_user_id=user_id,

syncmaster/backend/handler.py

@@ -22,6 +22,7 @@
 from syncmaster.exceptions.credentials import AuthDataNotFoundError
 from syncmaster.exceptions.group import (
     AlreadyIsGroupMemberError,
+    AlreadyIsGroupOwnerError,
     AlreadyIsNotGroupMemberError,
     GroupAdminNotFoundError,
     GroupAlreadyExistsError,
@@ -194,6 +195,14 @@ async def syncmsater_exception_handler(request: Request, exc: SyncmasterError):
             content=content,
         )
 
+    if isinstance(exc, AlreadyIsGroupOwnerError):
+        content.code = "conflict"
+        content.message = "User already is group owner"
+        return exception_json_response(
+            status=status.HTTP_409_CONFLICT,
+            content=content,
+        )
+
     if isinstance(exc, UserNotFoundError):
         content.code = "not_found"
         content.message = "User not found"

syncmaster/db/repositories/group.py

@@ -337,6 +337,15 @@ async def get_group_permission(self, user: User, group_id: int) -> Permission:
 
         return Permission.READ
 
+    async def get_user_group(self, group_id: int, user_id: int) -> UserGroup | None:
+        return await self._session.get(
+            UserGroup,
+            {
+                "group_id": group_id,
+                "user_id": user_id,
+            },
+        )
+
     async def delete_user(
         self,
         group_id: int,

syncmaster/exceptions/group.py

@@ -23,5 +23,9 @@ class AlreadyIsNotGroupMemberError(SyncmasterError):
     pass
 
 
+class AlreadyIsGroupOwnerError(SyncmasterError):
+    pass
+
+
 class GroupNotFoundError(SyncmasterError):
     pass

tests/test_unit/test_connections/connection_fixtures/group_connections_fixture.py

@@ -41,7 +41,6 @@ async def group_connections(
             )
         elif conn_type in [
             ConnectionType.POSTGRES,
-            ConnectionType.ORACLE,
             ConnectionType.CLICKHOUSE,
             ConnectionType.MSSQL,
             ConnectionType.MYSQL,

tests/test_unit/test_groups/test_add_user_to_group.py

@@ -337,6 +337,34 @@ async def test_owner_add_unknown_user_to_group_error(
     }
 
 
+async def test_add_exiting_owner_as_a_group_member(
+    client: AsyncClient,
+    empty_group: MockGroup,
+    role_maintainer_or_below: UserTestRoles,
+):
+    # Arrange
+    user = empty_group.get_member_of_role(UserTestRoles.Owner)
+
+    # Act
+    result = await client.post(
+        f"v1/groups/{empty_group.id}/users/{empty_group.owner_id}",
+        headers={"Authorization": f"Bearer {user.token}"},
+        json={
+            "role": role_maintainer_or_below,
+        },
+    )
+
+    # Assert
+    assert result.status_code == 409
+    assert result.json() == {
+        "error": {
+            "code": "conflict",
+            "message": "User already is group owner",
+            "details": None,
+        },
+    }
+
+
 async def test_superuser_add_user_to_unknown_group_error(
     client: AsyncClient,
     superuser: MockUser,

tests/test_unit/test_groups/test_update_group_by_id.py

@@ -192,24 +192,89 @@ async def test_validation_on_update_group(
 
 
 async def test_owner_change_group_owner(client: AsyncClient, empty_group: MockGroup, simple_user: MockUser):
+    # Arrange
+    previous_owner = empty_group.owner
+    new_owner = simple_user
+    user = empty_group.get_member_of_role(UserTestRoles.Owner)
     # Act
-    result = await client.patch(
+    patch_result = await client.patch(
         f"v1/groups/{empty_group.id}",
-        headers={"Authorization": f"Bearer {empty_group.get_member_of_role(UserTestRoles.Owner).token}"},
+        headers={"Authorization": f"Bearer {user.token}"},
         json={
             "name": empty_group.name,
-            "owner_id": simple_user.id,
+            "owner_id": new_owner.id,
             "description": empty_group.description,
         },
     )
+    group_users_result = await client.get(
+        f"v1/groups/{empty_group.id}/users",
+        headers={"Authorization": f"Bearer {user.token}"},
+    )
     # Assert
-    assert result.json() == {
+    assert patch_result.status_code == 200
+    assert patch_result.json() == {
         "id": empty_group.id,
         "name": empty_group.name,
-        "owner_id": simple_user.id,
+        "owner_id": new_owner.id,
         "description": empty_group.description,
     }
-    assert result.status_code == 200
+    assert group_users_result.status_code == 200
+    assert group_users_result.json()["items"] == [
+        {
+            "id": previous_owner.id,
+            "username": previous_owner.username,
+            "role": UserTestRoles.Guest,
+        },
+    ]
+
+
+async def test_owner_change_group_owner_with_existing_role(
+    client: AsyncClient,
+    empty_group: MockGroup,
+    simple_user: MockUser,
+    role_maintainer_or_below: UserTestRoles,
+):
+    # Arrange
+    previous_owner = empty_group.owner
+    new_owner = simple_user
+    user = empty_group.get_member_of_role(UserTestRoles.Owner)
+    # Act
+    await client.post(
+        f"v1/groups/{empty_group.id}/users/{new_owner.id}",
+        headers={"Authorization": f"Bearer {user.token}"},
+        json={
+            "role": role_maintainer_or_below,
+        },
+    )
+    patch_result = await client.patch(
+        f"v1/groups/{empty_group.id}",
+        headers={"Authorization": f"Bearer {user.token}"},
+        json={
+            "name": empty_group.name,
+            "owner_id": new_owner.id,
+            "description": empty_group.description,
+        },
+    )
+    group_users_result = await client.get(
+        f"v1/groups/{empty_group.id}/users",
+        headers={"Authorization": f"Bearer {user.token}"},
+    )
+    # Assert
+    assert patch_result.status_code == 200
+    assert patch_result.json() == {
+        "id": empty_group.id,
+        "name": empty_group.name,
+        "owner_id": new_owner.id,
+        "description": empty_group.description,
+    }
+    assert group_users_result.status_code == 200
+    assert group_users_result.json()["items"] == [
+        {
+            "id": previous_owner.id,
+            "username": previous_owner.username,
+            "role": UserTestRoles.Guest,
+        },
+    ]
 
 
 async def test_maintainer_or_below_cannot_change_group_owner(