KC-1346: Fix misleading communication error when assigning team to admin role

keepercommander/commands/enterprise.py

@@ -2384,7 +2384,10 @@ def execute(self, params, **kwargs):
 
             elif add_team or remove_team or add_user or remove_user:
                 if add_team or remove_team:
-                    non_batch_update_msgs += self.change_role_teams(params, matched_roles, add_team, remove_team)
+                    team_msgs, blocked_admin = self.change_role_teams(params, matched_roles, add_team, remove_team)
+                    non_batch_update_msgs += team_msgs
+                    if blocked_admin:
+                        skip_display = True
 
                 if add_user or remove_user:
                     request_batch += self.get_role_users_change_batch(
@@ -3499,9 +3502,11 @@ def execute(self, params, **kwargs):
                         request_batch.append(rq)
 
             if kwargs.get('add_role') or kwargs.get('remove_role'):
-                non_batch_update_msgs = self.change_team_roles(
+                non_batch_update_msgs, blocked_admin = self.change_team_roles(
                     params, matched_teams, kwargs.get('add_role'), kwargs.get('remove_role')
                 )
+                if blocked_admin:
+                    has_warnings = True
 
             if kwargs.get('add_user') or kwargs.get('remove_user'):
                 users = {}
@@ -3705,7 +3710,7 @@ def execute(self, params, **kwargs):
                         created_teams.append(team_data)
 
                 if created_teams:
-                    role_msgs = self.change_team_roles(params, created_teams, role_list, None)
+                    role_msgs, _ = self.change_team_roles(params, created_teams, role_list, None)
                     for msg in role_msgs:
                         logging.info(msg)
         elif not has_warnings:

keepercommander/commands/enterprise_common.py

@@ -139,6 +139,7 @@ def decrypt_role_key(params, rk):
     def change_role_teams(params, roles, add_team, remove_team):
         """Change enterprise role teams"""
         update_msgs = []
+        blocked_admin_assignment = False
         add_teams = None
         remove_teams = None
         team_changes = {}
@@ -171,6 +172,7 @@ def change_role_teams(params, roles, add_team, remove_team):
 
                 if is_managed_role:
                     logging.warning('Teams cannot be assigned to roles with administrative permissions.')
+                    blocked_admin_assignment = True
                 else:
                     role_team = enterprise_pb2.RoleTeam()
                     role_team.role_id = role_id
@@ -182,15 +184,16 @@ def change_role_teams(params, roles, add_team, remove_team):
                     else:
                         remove_teams.role_team.append(role_team)
                         update_msgs.append(f"'{role_name}' role removed from team '{team_name}'")
-        if remove_teams:
+        if remove_teams and remove_teams.role_team:
             api.communicate_rest(params, remove_teams, 'enterprise/role_team_remove')
-        if add_teams:
+        if add_teams and add_teams.role_team:
             api.communicate_rest(params, add_teams, 'enterprise/role_team_add')
-        return update_msgs
+        return update_msgs, blocked_admin_assignment
 
     @staticmethod
     def change_team_roles(params, teams, add_roles, remove_roles):
         update_msgs = []
+        blocked_admin_assignment = False
         add_role_teams = None
         remove_role_teams = None
         role_changes = {}
@@ -224,6 +227,7 @@ def change_team_roles(params, teams, add_roles, remove_roles):
 
                 if is_managed_role:
                     logging.warning('Teams cannot be assigned to roles with administrative permissions.')
+                    blocked_admin_assignment = True
                 else:
                     for team in teams:
                         if is_add and team['team_uid'] in role_teams:
@@ -245,11 +249,11 @@ def change_team_roles(params, teams, add_roles, remove_roles):
                             else:
                                 remove_role_teams.role_team.append(role_team)
                                 update_msgs.append(f"'{role_name}' role removed from team '{team_name}'")
-        if remove_role_teams:
+        if remove_role_teams and remove_role_teams.role_team:
             api.communicate_rest(params, remove_role_teams, 'enterprise/role_team_remove')
-        if add_role_teams:
+        if add_role_teams and add_role_teams.role_team:
             api.communicate_rest(params, add_role_teams, 'enterprise/role_team_add')
-        return update_msgs
+        return update_msgs, blocked_admin_assignment
 
     @staticmethod
     def get_enterprise_id(params):

tests/test_enterprise_role_team_validation.py

@@ -0,0 +1,42 @@
+import unittest
+from unittest import mock
+
+from keepercommander.commands.enterprise_common import EnterpriseCommand
+from keepercommander.params import KeeperParams
+
+
+class TestEnterpriseRoleTeamValidation(unittest.TestCase):
+    def test_change_team_roles_skips_api_for_admin_role(self):
+        params = KeeperParams()
+        admin_role_id = 894448414228484
+        params.enterprise = {
+            'roles': [{'role_id': admin_role_id, 'data': {'displayname': 'Admin Role'}}],
+            'managed_nodes': [{'role_id': admin_role_id}],
+            'role_teams': [],
+        }
+        teams = [{'team_uid': '4GjeorSt3FiI2KBhgCKI2Q', 'name': 'Test Team'}]
+
+        with mock.patch('keepercommander.api.communicate_rest') as communicate_rest:
+            msgs, blocked_admin = EnterpriseCommand.change_team_roles(
+                params, teams, add_roles=[str(admin_role_id)], remove_roles=None)
+
+        self.assertEqual(msgs, [])
+        self.assertTrue(blocked_admin)
+        communicate_rest.assert_not_called()
+
+    def test_change_role_teams_skips_api_for_admin_role(self):
+        params = KeeperParams()
+        admin_role_id = 894448414228484
+        params.enterprise = {
+            'teams': [{'team_uid': '4GjeorSt3FiI2KBhgCKI2Q', 'name': 'Test Team'}],
+            'managed_nodes': [{'role_id': admin_role_id}],
+        }
+        roles = [{'role_id': admin_role_id, 'data': {'displayname': 'Admin Role'}}]
+
+        with mock.patch('keepercommander.api.communicate_rest') as communicate_rest:
+            msgs, blocked_admin = EnterpriseCommand.change_role_teams(
+                params, roles, add_team=['4GjeorSt3FiI2KBhgCKI2Q'], remove_team=None)
+
+        self.assertEqual(msgs, [])
+        self.assertTrue(blocked_admin)
+        communicate_rest.assert_not_called()