diff --git a/keepercommander/commands/enterprise.py b/keepercommander/commands/enterprise.py index 110e0d237..512c4fce1 100644 --- a/keepercommander/commands/enterprise.py +++ b/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: diff --git a/keepercommander/commands/enterprise_common.py b/keepercommander/commands/enterprise_common.py index acf5663d8..eb0bba7e5 100644 --- a/keepercommander/commands/enterprise_common.py +++ b/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): diff --git a/tests/test_enterprise_role_team_validation.py b/tests/test_enterprise_role_team_validation.py new file mode 100644 index 000000000..4214f2950 --- /dev/null +++ b/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()