diff --git a/docs/changelog/next_release/135.feature.rst b/docs/changelog/next_release/135.feature.rst new file mode 100644 index 00000000..7cb0da29 --- /dev/null +++ b/docs/changelog/next_release/135.feature.rst @@ -0,0 +1 @@ +Grant read-only rights for the previous group owner when ownership is transferred \ No newline at end of file diff --git a/syncmaster/backend/api/v1/groups.py b/syncmaster/backend/api/v1/groups.py index 7b5dbd83..ce335abc 100644 --- a/syncmaster/backend/api/v1/groups.py +++ b/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) @@ -195,6 +213,10 @@ async def add_user_to_group( if resource_rule < Permission.DELETE: raise ActionNotAllowedError + group = await unit_of_work.group.read_by_id(group_id=group_id) + if group.owner_id == user_id: + raise AlreadyIsGroupOwnerError + async with unit_of_work: await unit_of_work.group.add_user( group_id=group_id, diff --git a/syncmaster/backend/handler.py b/syncmaster/backend/handler.py index 9ddbdda1..9ef50252 100644 --- a/syncmaster/backend/handler.py +++ b/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" diff --git a/syncmaster/db/repositories/group.py b/syncmaster/db/repositories/group.py index 8c593c76..3ebedf65 100644 --- a/syncmaster/db/repositories/group.py +++ b/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, diff --git a/syncmaster/exceptions/group.py b/syncmaster/exceptions/group.py index 90c61466..5e2c308c 100644 --- a/syncmaster/exceptions/group.py +++ b/syncmaster/exceptions/group.py @@ -23,5 +23,9 @@ class AlreadyIsNotGroupMemberError(SyncmasterError): pass +class AlreadyIsGroupOwnerError(SyncmasterError): + pass + + class GroupNotFoundError(SyncmasterError): pass diff --git a/tests/test_unit/test_connections/connection_fixtures/group_connections_fixture.py b/tests/test_unit/test_connections/connection_fixtures/group_connections_fixture.py index 3c599207..b9b2527e 100644 --- a/tests/test_unit/test_connections/connection_fixtures/group_connections_fixture.py +++ b/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, diff --git a/tests/test_unit/test_groups/test_add_user_to_group.py b/tests/test_unit/test_groups/test_add_user_to_group.py index bea5790d..569aabb0 100644 --- a/tests/test_unit/test_groups/test_add_user_to_group.py +++ b/tests/test_unit/test_groups/test_add_user_to_group.py @@ -337,6 +337,31 @@ 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, +): + user = empty_group.get_member_of_role(UserTestRoles.Owner) + + 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 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, diff --git a/tests/test_unit/test_groups/test_update_group_by_id.py b/tests/test_unit/test_groups/test_update_group_by_id.py index 865cc690..59e633f4 100644 --- a/tests/test_unit/test_groups/test_update_group_by_id.py +++ b/tests/test_unit/test_groups/test_update_group_by_id.py @@ -192,24 +192,93 @@ async def test_validation_on_update_group( async def test_owner_change_group_owner(client: AsyncClient, empty_group: MockGroup, simple_user: MockUser): - # Act - result = await client.patch( + previous_owner = empty_group.owner + new_owner = simple_user + user = empty_group.get_member_of_role(UserTestRoles.Owner) + + # Change a group owner + 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, }, ) - # Assert - assert result.json() == { + group_users_result = await client.get( + f"v1/groups/{empty_group.id}/users", + headers={"Authorization": f"Bearer {user.token}"}, + ) + + 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 + # Make sure previous owner became a guest in group + 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, +): + previous_owner = empty_group.owner + new_owner = simple_user + user = empty_group.get_member_of_role(UserTestRoles.Owner) + + # Make user a group member + 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, + }, + ) + # Upgrade user to a group owner + 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 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, + } + # Make sure previous owner became a guest in group + # As well as upgraded owner is no longer considered a group member + 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(