feat(backend): allow owners to edit owners and admins to edit admins (org project) (#13095)

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

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
         )