From b8ab4bb44e3a645982760df4996b9accfec982dc Mon Sep 17 00:00:00 2001 From: Hiep Le <69354317+hieptl@users.noreply.github.com> Date: Fri, 27 Feb 2026 16:01:05 +0700 Subject: [PATCH] feat(backend): allow owners to edit owners and admins to edit admins (org project) (#13095) --- .../server/services/org_member_service.py | 28 ++-- .../services/test_org_member_service.py | 120 +++++++++++++----- 2 files changed, 96 insertions(+), 52 deletions(-) diff --git a/enterprise/server/services/org_member_service.py b/enterprise/server/services/org_member_service.py index 639594cbb9..5777ab0d5a 100644 --- a/enterprise/server/services/org_member_service.py +++ b/enterprise/server/services/org_member_service.py @@ -2,7 +2,7 @@ from uuid import UUID -from server.constants import ROLE_ADMIN, ROLE_MEMBER, ROLE_OWNER +from server.constants import ROLE_ADMIN, ROLE_OWNER from server.routes.org_models import ( CannotModifySelfError, InsufficientPermissionError, @@ -263,10 +263,9 @@ class OrgMemberService: """Update a member's role in an organization. Permission rules: - - Admins can change roles of users (rank > ADMIN_RANK) to Admin or User - - Admins cannot modify other Admins or Owners - - Owners can change roles of non-owners (rank > OWNER_RANK) to any role - - Owners cannot modify other Owners + - Owners can modify anyone (including other owners), can set any role + - Admins can modify other admins and users + - Admins can only set admin or user roles (not owner) Args: org_id: Organization ID @@ -375,26 +374,21 @@ class OrgMemberService: """Check if requester can change target's role to new_role. Permission rules: - - Owners can modify admins and users, can set any role - - Owners cannot modify other owners - - Admins can only modify users + - Owners can modify anyone (including other owners), can set any role + - Admins can modify other admins and users - Admins can only set admin or user roles (not owner) """ is_requester_owner = requester_role_name == ROLE_OWNER is_requester_admin = requester_role_name == ROLE_ADMIN is_target_owner = target_role_name == ROLE_OWNER - is_target_admin = target_role_name == ROLE_ADMIN is_new_role_owner = new_role_name == ROLE_OWNER if is_requester_owner: - # Owners cannot modify other owners - if is_target_owner: - return False - # Owners can set any role (owner, admin, user) + # Owners can modify anyone (including other owners) return True elif is_requester_admin: - # Admins cannot modify owners or other admins - if is_target_owner or is_target_admin: + # Admins cannot modify owners + if is_target_owner: return False # Admins can only set admin or user roles (not owner) return not is_new_role_owner @@ -406,8 +400,8 @@ class OrgMemberService: if requester_role_name == ROLE_OWNER: return True elif requester_role_name == ROLE_ADMIN: - # Admins can only remove members (not owners or other admins) - return target_role_name == ROLE_MEMBER + # Admins can remove admins and members (not owners) + return target_role_name != ROLE_OWNER return False @staticmethod diff --git a/enterprise/tests/unit/server/services/test_org_member_service.py b/enterprise/tests/unit/server/services/test_org_member_service.py index 82b14ae3d2..f3f4aadc13 100644 --- a/enterprise/tests/unit/server/services/test_org_member_service.py +++ b/enterprise/tests/unit/server/services/test_org_member_service.py @@ -7,7 +7,6 @@ import pytest from pydantic import SecretStr from server.routes.org_models import ( CannotModifySelfError, - InsufficientPermissionError, InvalidRoleError, LastOwnerError, MeResponse, @@ -910,7 +909,7 @@ class TestOrgMemberServiceRemoveOrgMember: assert error == 'role_not_found' @pytest.mark.asyncio - async def test_admin_cannot_remove_admin_returns_error( + async def test_admin_can_remove_admin_succeeds( self, org_id, current_user_id, @@ -919,7 +918,7 @@ class TestOrgMemberServiceRemoveOrgMember: target_membership_admin, admin_role, ): - """Test that an admin cannot remove another admin.""" + """Test that an admin can remove another admin.""" # Arrange with ( patch( @@ -928,12 +927,24 @@ class TestOrgMemberServiceRemoveOrgMember: patch( 'server.services.org_member_service.RoleStore.get_role_by_id' ) as mock_get_role, + patch( + 'server.services.org_member_service.OrgMemberStore.remove_user_from_org' + ) as mock_remove, + patch( + 'server.services.org_member_service.UserStore.get_user_by_id' + ) as mock_get_user, + patch( + 'server.services.org_member_service.LiteLlmManager.remove_user_from_team' + ) as mock_remove_litellm, ): mock_get_member.side_effect = [ requester_membership_admin, target_membership_admin, ] mock_get_role.side_effect = [admin_role, admin_role] + mock_remove.return_value = True + mock_get_user.return_value = None + mock_remove_litellm.return_value = None # Act success, error = await OrgMemberService.remove_org_member( @@ -941,8 +952,8 @@ class TestOrgMemberServiceRemoveOrgMember: ) # Assert - assert success is False - assert error == 'insufficient_permission' + assert success is True + assert error is None @pytest.mark.asyncio async def test_admin_cannot_remove_owner_returns_error( @@ -1480,13 +1491,13 @@ class TestOrgMemberServiceCanRemoveMember: # Assert assert result is True - def test_admin_cannot_remove_admin(self): - """Test that admin cannot remove another admin.""" + def test_admin_can_remove_admin(self): + """Test that admin can remove another admin.""" # Act result = OrgMemberService._can_remove_member('admin', 'admin') # Assert - assert result is False + assert result is True def test_admin_cannot_remove_owner(self): """Test that admin cannot remove owner.""" @@ -1620,7 +1631,7 @@ class TestOrgMemberServiceUpdateOrgMember: mock_update.assert_called_once_with(org_id, target_user_id, admin_role.id) @pytest.mark.asyncio - async def test_admin_cannot_update_admin_raises_insufficient_permission( + async def test_admin_can_update_admin_to_member_succeeds( self, org_id, current_user_id, @@ -1630,8 +1641,14 @@ class TestOrgMemberServiceUpdateOrgMember: admin_role, member_role, ): - """GIVEN admin and target admin WHEN admin tries to change target role THEN raises InsufficientPermissionError.""" + """GIVEN admin and target admin WHEN admin changes target role to member THEN update succeeds.""" # Arrange + updated_member = MagicMock(spec=OrgMember) + updated_member.user_id = target_user_id + updated_member.role_id = member_role.id + updated_member.status = 'active' + mock_user = MagicMock() + mock_user.email = 'target@example.com' with ( patch( 'server.services.org_member_service.OrgMemberStore.get_org_member' @@ -1642,6 +1659,12 @@ class TestOrgMemberServiceUpdateOrgMember: patch( 'server.services.org_member_service.RoleStore.get_role_by_name' ) as mock_get_role_by_name, + patch( + 'server.services.org_member_service.OrgMemberStore.update_user_role_in_org' + ) as mock_update, + patch( + 'server.services.org_member_service.UserStore.get_user_by_id' + ) as mock_get_user, ): mock_get_member.side_effect = [ requester_membership_admin, @@ -1649,18 +1672,24 @@ class TestOrgMemberServiceUpdateOrgMember: ] mock_get_role.side_effect = [admin_role, admin_role] mock_get_role_by_name.return_value = member_role + mock_update.return_value = updated_member + mock_get_user.return_value = mock_user - # Act & Assert - with pytest.raises(InsufficientPermissionError): - await OrgMemberService.update_org_member( - org_id, - target_user_id, - current_user_id, - OrgMemberUpdate(role='member'), - ) + # Act + data = await OrgMemberService.update_org_member( + org_id, + target_user_id, + current_user_id, + OrgMemberUpdate(role='member'), + ) + + # Assert + assert isinstance(data, OrgMemberResponse) + assert data.role == 'member' + mock_update.assert_called_once_with(org_id, target_user_id, member_role.id) @pytest.mark.asyncio - async def test_owner_cannot_update_owner_raises_insufficient_permission( + async def test_owner_can_update_owner_to_admin_succeeds( self, org_id, current_user_id, @@ -1670,8 +1699,14 @@ class TestOrgMemberServiceUpdateOrgMember: owner_role, admin_role, ): - """GIVEN owner and target owner WHEN owner tries to change target role THEN raises InsufficientPermissionError.""" + """GIVEN owner and target owner WHEN owner changes target role to admin THEN update succeeds.""" # Arrange + updated_member = MagicMock(spec=OrgMember) + updated_member.user_id = target_user_id + updated_member.role_id = admin_role.id + updated_member.status = 'active' + mock_user = MagicMock() + mock_user.email = 'target@example.com' with ( patch( 'server.services.org_member_service.OrgMemberStore.get_org_member' @@ -1682,6 +1717,13 @@ class TestOrgMemberServiceUpdateOrgMember: patch( 'server.services.org_member_service.RoleStore.get_role_by_name' ) as mock_get_role_by_name, + patch( + 'server.services.org_member_service.OrgMemberStore.update_user_role_in_org' + ) as mock_update, + patch( + 'server.services.org_member_service.UserStore.get_user_by_id' + ) as mock_get_user, + patch.object(OrgMemberService, '_is_last_owner', return_value=False), ): mock_get_member.side_effect = [ requester_membership_owner, @@ -1689,15 +1731,21 @@ class TestOrgMemberServiceUpdateOrgMember: ] mock_get_role.side_effect = [owner_role, owner_role] mock_get_role_by_name.return_value = admin_role + mock_update.return_value = updated_member + mock_get_user.return_value = mock_user - # Act & Assert - with pytest.raises(InsufficientPermissionError): - await OrgMemberService.update_org_member( - org_id, - target_user_id, - current_user_id, - OrgMemberUpdate(role='admin'), - ) + # Act + data = await OrgMemberService.update_org_member( + org_id, + target_user_id, + current_user_id, + OrgMemberUpdate(role='admin'), + ) + + # Assert + assert isinstance(data, OrgMemberResponse) + assert data.role == 'admin' + mock_update.assert_called_once_with(org_id, target_user_id, admin_role.id) @pytest.mark.asyncio async def test_requester_not_a_member_raises_error( @@ -1912,10 +1960,10 @@ class TestOrgMemberServiceCanUpdateMemberRole: OrgMemberService._can_update_member_role('owner', 'member', 'owner') is True ) - def test_owner_cannot_modify_owner(self): - """Owner cannot change another owner's role.""" + def test_owner_can_modify_owner(self): + """Owner can change another owner's role.""" assert ( - OrgMemberService._can_update_member_role('owner', 'owner', 'admin') is False + OrgMemberService._can_update_member_role('owner', 'owner', 'admin') is True ) def test_admin_can_set_admin_or_member_for_member(self): @@ -1928,12 +1976,14 @@ class TestOrgMemberServiceCanUpdateMemberRole: is True ) - def test_admin_cannot_modify_admin_or_owner(self): - """Admin cannot modify admin or owner targets.""" + def test_admin_can_modify_admin(self): + """Admin can modify another admin's role to member.""" assert ( - OrgMemberService._can_update_member_role('admin', 'admin', 'member') - is False + OrgMemberService._can_update_member_role('admin', 'admin', 'member') is True ) + + def test_admin_cannot_modify_owner(self): + """Admin cannot modify owner targets.""" assert ( OrgMemberService._can_update_member_role('admin', 'owner', 'admin') is False )