From 737f0035c5f66c97c4e095e8f86579d2f4a543a4 Mon Sep 17 00:00:00 2001 From: hpadalkar-ks Date: Wed, 28 Oct 2026 12:18:59 +0530 Subject: [PATCH 1/6] Feature / MM-26 Added functionality for interactive safe selection (#2464) * MM-26 Added functionality for interactive safe selection * MM-26 improved time complexity, added edge cases to test case * MM-26 decoupled input prompts on non-interactive environments * MM-26 added _load_folders helper function, to avoid logical branching --- keepercommander/importer/thycotic/thycotic.py | 260 ++++++++--- ...test_thycotic_interactive_folder_picker.py | 402 ++++++++++++++++++ 2 files changed, 614 insertions(+), 48 deletions(-) create mode 100644 unit-tests/test_thycotic_interactive_folder_picker.py diff --git a/keepercommander/importer/thycotic/thycotic.py b/keepercommander/importer/thycotic/thycotic.py index f7409609c..16e1c5803 100644 --- a/keepercommander/importer/thycotic/thycotic.py +++ b/keepercommander/importer/thycotic/thycotic.py @@ -16,6 +16,7 @@ import json import logging import os +import sys import time import warnings from contextlib import contextmanager @@ -25,7 +26,7 @@ import requests -from urllib.parse import urlparse, urlunparse +from urllib.parse import parse_qsl, urlencode, urlparse, urlunparse from typing import Tuple, Optional, Iterable, Union, Dict, Callable, Any, List from ..importer import (BaseImporter, BaseDownloadMembership, Record, SharedFolder, Folder, Attachment, Permission, @@ -100,17 +101,172 @@ def get_group_lookup(auth, on_progress=None): pass @staticmethod - def get_folders(auth, skip_permissions=False): # type: (ThycoticAuth, bool) -> Optional[Dict[int, Dict]] - logging.debug('Enter get_folders') + def _parse_choice(choice, count): + if not isinstance(choice, str): + return None + choice = choice.strip() + if not choice or any(c not in '0123456789,- ' for c in choice): + return None + selected = set() + for part in choice.split(','): + part = part.strip() + if not part: + return None + if '-' in part: + bounds = part.split('-') + if len(bounds) != 2 or not all(b.strip().isdigit() for b in bounds): + return None + start, end = int(bounds[0]), int(bounds[1]) + if start < 1 or end > count or start > end: + return None + selected.update(range(start, end + 1)) + elif part.isdigit(): + number = int(part) + if number < 1 or number > count: + return None + selected.add(number) + else: + return None + return selected or None + + @staticmethod + def _compose_folder_path(folder, folders): + # type: (Dict, Dict[int, Dict]) -> str + folder_path = folder.get('folderName') or '' + parent = folder + seen = {folder.get('id')} + while parent: + parent_id = parent.get('parentFolderId') + if isinstance(parent_id, int) and parent_id > 1 and parent_id in folders and parent_id not in seen: + seen.add(parent_id) + parent = folders[parent_id] + folder_path = (parent.get('folderName') or '') + '\\' + folder_path + else: + parent = None + return folder_path + + @staticmethod + def _apply_folder_paths(folders): + # type: (Dict[int, Dict]) -> None + for folder in folders.values(): + folder['folderPath'] = ThycoticMixin._compose_folder_path(folder, folders) + + @staticmethod + def interactive_folder_picker(folders): + if not isinstance(folders, list): + return None + if not folders: + return folders + indexed = {} + for folder in folders: + if isinstance(folder, dict) and folder.get('id') is not None and 'folderName' in folder: + indexed.setdefault(folder['id'], folder) + if indexed: + ThycoticMixin._apply_folder_paths(indexed) + print('\nFolders Found:') + print('─' * 50) + for i, folder in enumerate(folders, start=1): + if not isinstance(folder, dict): + folder = {} + name = folder.get('folderName') or '' + folder_path = folder.get('folderPath') + if isinstance(folder_path, str) and folder_path: + path = folder_path.strip('\\').replace('\\', ' / ') + else: + path = name + print(f" [{i}] {name} - Path -> {path}") + print() + try: + while True: + print("Enter the folder numbers to import, separated by commas (e.g. 1-4,6,8-10).") + print("Press Enter or 'A' to import all.") + selected = ThycoticMixin._select_folders(folders, input('Enter your choice: ')) + if selected is None: + print('Invalid choice. Enter folder numbers, press Enter, or A to import all.') + else: + return selected + except EOFError: + return folders + except KeyboardInterrupt: + raise Exception('Import canceled') from None + + @staticmethod + def _select_folders(folders, choice): + # type: (List[Dict], str) -> Optional[List[Dict]] + choice = choice.strip() if isinstance(choice, str) else '' + if choice == '' or choice.upper() == 'A': + return folders + selected = ThycoticMixin._parse_choice(choice, len(folders)) + if selected is None: + return None + return [folder for index, folder in enumerate(folders, start=1) if index in selected] + + @staticmethod + def expand_folder_ids(folders, root_ids): + # type: (Union[Dict[int, Dict], Iterable[Dict]], Iterable) -> set + if not folders or not root_ids: + return set() + if isinstance(folders, dict): + values = folders.values() + else: + values = folders + children_by_parent = {} + for folder in values: + if not isinstance(folder, dict) or folder.get('id') is None: + continue + children_by_parent.setdefault(folder.get('parentFolderId'), []).append(folder.get('id')) + seen = set() + pending = [] + for root_id in root_ids: + if root_id is not None and root_id not in seen: + seen.add(root_id) + pending.append(root_id) + pos = 0 + while pos < len(pending): + folder_id = pending[pos] + pos += 1 + for child_id in children_by_parent.get(folder_id, ()): + if child_id not in seen: + seen.add(child_id) + pending.append(child_id) + return seen + + @staticmethod + def _construct_folders(folder_rs): + # type: (Any) -> Dict[int, Dict] + if not isinstance(folder_rs, list): + return {} + folders = {} # type: Dict[int, Dict[str, Any]] + for x in folder_rs: + if not isinstance(x, dict) or x.get('id') is None or x['id'] in folders: + continue + folders[x['id']] = { + 'id': x['id'], + 'folderName': x.get('folderName') or '', + 'inheritPermissions': x.get('inheritPermissions', True), + 'parentFolderId': x.get('parentFolderId'), + } + ThycoticMixin._apply_folder_paths(folders) + return folders + + @staticmethod + def _load_folders(auth): + # type: (ThycoticAuth) -> Dict[int, Dict] folder_rs = auth.thycotic_search('/v1/folders') - folders = {x['id']: { - 'id': x['id'], - 'folderName': x['folderName'], - 'folderPath': x['folderPath'], - 'inheritPermissions': x['inheritPermissions'], - 'parentFolderId': x['parentFolderId'], - } for x in folder_rs} # type: Dict[int, Dict[str, Any]] + folders = ThycoticMixin._construct_folders(folder_rs) logging.debug('Loaded %d folders', len(folders)) + logging.debug('Folder path built') + return folders + + @staticmethod + def get_folders(auth, skip_permissions=False): + # type: (ThycoticAuth, bool) -> Optional[Dict[int, Dict]] + logging.debug('Enter get_folders') + folders = ThycoticMixin._load_folders(auth) + + if 1 in folders: + del folders[1] + if not skip_permissions: # load permissions for shared folders folder_ids = [x['id'] for x in folders.values() if (x.get('inheritPermissions') is False) and x['id'] != 1] @@ -191,25 +347,23 @@ def get_folders(auth, skip_permissions=False): # type: (ThycoticAuth, bool if are_same: del folder['permissions'] folder['inheritPermissions'] = True - - # build folder path - for folder in folders.values(): - folder_path = folder['folderName'] - parent = folder - while parent: - parent_id = parent.get('parentFolderId') or -1 - if parent_id > 1 and parent_id in folders: - parent = folders[parent_id] - folder_path = parent['folderName'] + '\\' + folder_path - else: - parent = None - folder['folderPath'] = folder_path - logging.debug('Folder path built') - - if 1 in folders: - del folders[1] return folders + @staticmethod + def get_secret_ids(auth, folder_ids, query): + secret_ids = [] + for folder_id in folder_ids or (): + if folder_id is None: + continue + params = [('filter.folderId', folder_id)] + if query: + params.extend(parse_qsl(query)) + secrets = auth.thycotic_search('/v1/secrets/lookup?' + urlencode(params)) + if not isinstance(secrets, list): + continue + secret_ids.extend(secret['id'] for secret in secrets + if isinstance(secret, dict) and secret.get('id') is not None) + return list(dict.fromkeys(secret_ids)) class ThycoticImporter(BaseImporter, ThycoticMixin): def __init__(self): @@ -317,31 +471,41 @@ def do_import(self, filename, **kwargs): print(f'Loaded {len(totp_codes)} code(s)', flush=True) self._send_keep_alive_if_needed(params) - folders = ThycoticImporter.get_folders(auth, skip_permissions=True) filter_folder = kwargs.get('filter_folder') - if filter_folder: - if filter_folder == 'Personal Folders': - matched_folder_ids = [1] + constructed_folders = ThycoticImporter._load_folders(auth) + picked_folder_ids = None + if not filter_folder and sys.stdin and sys.stdin.isatty(): + folder_choices = list(constructed_folders.values()) + picked_folders = ThycoticImporter.interactive_folder_picker(folder_choices) + if picked_folders is not None and picked_folders is not folder_choices: + picked_folder_ids = list(dict.fromkeys(folder['id'] for folder in picked_folders)) + + + if 1 in constructed_folders: + del constructed_folders[1] + folders = constructed_folders + secrets_ids = [] + query = 'filter.includeSubFolders=true' + + if filter_folder or picked_folder_ids is not None: + if filter_folder: + if filter_folder == 'Personal Folders': + matched_folder_ids = [1] + else: + matched_folder_ids = [x['id'] for x in folders.values() if x['folderName'].lower() == filter_folder.lower()] + if len(matched_folder_ids) == 0: + logging.warning('Folder \"%s\" not found', filter_folder) else: - matched_folder_ids = [x['id'] for x in folders.values() if x['folderName'].lower() == filter_folder.lower()] - if len(matched_folder_ids) == 0: - logging.warning('Folder \"%s\" not found', filter_folder) - folder_ids = list(matched_folder_ids) - pos = 0 - while pos < len(folder_ids): - folder_id = folder_ids[pos] - pos += 1 - folder_ids.extend((x['id'] for x in folders.values() if x['parentFolderId'] == folder_id)) - folder_ids = set(folder_ids) + matched_folder_ids = picked_folder_ids + folder_ids = ThycoticImporter.expand_folder_ids(folders, matched_folder_ids) folders = {i: x for i, x in folders.items() if i in folder_ids} - - secrets_ids = [] - for folder_id in matched_folder_ids: - query = f'/v1/secrets/lookup?filter.folderId={folder_id}&filter.includeSubFolders=true' - secrets_ids.extend([x['id'] for x in auth.thycotic_search(query)]) + secrets_ids = ThycoticImporter.get_secret_ids(auth, matched_folder_ids, query) else: - secrets_ids = [x['id'] for x in auth.thycotic_search(f'/v1/secrets/lookup')] - + matched_folder_ids = list(folders.keys()) + secrets_rs = auth.thycotic_search('/v1/secrets/lookup') + secrets_ids = [x['id'] for x in secrets_rs if isinstance(x, dict) and x.get('id') is not None] \ + if isinstance(secrets_rs, list) else [] + # secret_ids arg debug_ids = kwargs.get('secret_ids') if debug_ids is not None: diff --git a/unit-tests/test_thycotic_interactive_folder_picker.py b/unit-tests/test_thycotic_interactive_folder_picker.py new file mode 100644 index 000000000..adf0f867c --- /dev/null +++ b/unit-tests/test_thycotic_interactive_folder_picker.py @@ -0,0 +1,402 @@ +import unittest +from unittest import mock +from keepercommander.importer.thycotic.thycotic import ThycoticImporter, ThycoticMixin + +L1, L2, L3, L4 = 'Test_Folder_l1', r'Test_Folder_l1\Test_Folder_l2', \ + r'Test_Folder_l1\Test_Folder_l2\Test_Folder_L3', r'Test_Folder_l1\Test_Folder_l2\Test_Folder_L3\Test_Folder_l4' + + +def _folder(folder_id, name, parent=-1, with_path=True): + folder = {'id': folder_id, 'folderName': name, 'parentFolderId': parent, 'inheritPermissions': True} + if with_path: + folder['folderPath'] = '\\' + name + return folder + + +def _secret(secret_id, folder_id): + return {'id': secret_id, 'name': f'secret-{secret_id}', 'items': [], 'folderId': folder_id, 'secretTemplateName': ''} + + +def _lookup(folder_id): + return f'/v1/secrets/lookup?filter.folderId={folder_id}&filter.includeSubFolders=true' + + +class ParseChoiceTests(unittest.TestCase): + def test_invalid_input_is_rejected(self): + for choice, count in ((None, 5), (1, 5), (['1'], 5), (b'1', 5), ('', 5), (' ', 5), ('A', 5), + ('0', 5), ('6', 5), ('99999999999', 5), ('3-1', 5), ('5-1', 5), ('1-6', 5), + (',', 5), ('-', 5), ('1-', 5), ('-1', 5), ('1--2', 5), ('1-2-3', 5), (' , ', 5), + ('1,', 5), ('1,,2', 5), ('1 2', 5), ('1.5', 5), ('+1', 5), ('1', 0), ('1', -1)): + with self.subTest(choice=choice, count=count): + self.assertIsNone(ThycoticMixin._parse_choice(choice, count)) + + def test_valid_input_is_parsed(self): + for choice, count, expected in (('1', 1, {1}), ('5', 5, {5}), (' 1 ', 5, {1}), ('01', 5, {1}), + ('3-3', 5, {3}), (' 2 - 4 , 4 ', 4, {2, 3, 4}), + ('1-3,2-4', 4, {1, 2, 3, 4}), ('2,1,1', 3, {1, 2})): + with self.subTest(choice=choice): + self.assertEqual(ThycoticMixin._parse_choice(choice, count), expected) + + +class SelectFoldersTests(unittest.TestCase): + def setUp(self): + self.folders = [_folder(1, 'a'), _folder(2, 'b'), _folder(3, 'c')] + + def test_blank_none_or_a_returns_all(self): + for choice in ('', ' ', 'a', ' A ', None, 5): + with self.subTest(choice=choice): + self.assertIs(ThycoticMixin._select_folders(self.folders, choice), self.folders) + + def test_valid_choice_filters_in_list_order(self): + for choice, expected in (('3,1', [0, 2]), ('2-3', [1, 2]), (' 2 , 3 ', [1, 2]), ('1-3', [0, 1, 2])): + with self.subTest(choice=choice): + self.assertEqual(ThycoticMixin._select_folders(self.folders, choice), + [self.folders[i] for i in expected]) + + def test_invalid_choice_returns_none(self): + for folders, choice in ((self.folders, '0'), (self.folders, '4'), (self.folders, 'x'), + (self.folders, '1,,2'), ([], '1')): + with self.subTest(folders=folders, choice=choice): + self.assertIsNone(ThycoticMixin._select_folders(folders, choice)) + + +class InteractiveFolderPickerTests(unittest.TestCase): + def setUp(self): + self.folders = [_folder(116, 'L3', parent=103), _folder(103, 'l2', parent=101), + _folder(101, 'l1'), _folder(108, 'l100', parent=101)] + + def _pick(self, folders, *inputs): + with mock.patch('builtins.input', side_effect=inputs) as mocked_input, \ + mock.patch('builtins.print') as mocked_print: + result = ThycoticMixin.interactive_folder_picker(folders) + printed = [call.args[0] for call in mocked_print.call_args_list if call.args] + return result, printed, mocked_input + + def assertPrinted(self, printed, text): + self.assertTrue(any(text in line for line in printed), f'{text!r} not in {printed!r}') + + def test_non_list_or_empty_input_does_not_prompt(self): + empty = [] + for folders, expected in ((None, None), ({101: self.folders[2]}, None), (tuple(self.folders), None), + ('folders', None), (empty, empty)): + with self.subTest(folders=folders), mock.patch('builtins.input') as mocked_input: + self.assertIs(ThycoticMixin.interactive_folder_picker(folders), expected) + mocked_input.assert_not_called() + + def test_choice_returns_folders_in_list_order(self): + for inputs, expected in ((('',), self.folders), ((' a ',), self.folders), (('A',), self.folders), + (('1-4',), self.folders), (('4,1,2',), [self.folders[i] for i in (0, 1, 3)])): + with self.subTest(inputs=inputs): + self.assertEqual(self._pick(self.folders, *inputs)[0], expected) + + def test_invalid_entries_are_retried(self): + for inputs, expected in ((('nope', '1,99', '2'), [self.folders[1]]), (('0', '5', 'a'), self.folders)): + with self.subTest(inputs=inputs): + result, printed, mocked_input = self._pick(self.folders, *inputs) + self.assertEqual(result, expected) + self.assertEqual(mocked_input.call_count, 3) + self.assertEqual(len([line for line in printed if line.startswith('Invalid choice')]), 2) + + def test_interrupt_cancels_and_eof_imports_all(self): + for inputs in ([KeyboardInterrupt], ['bad', KeyboardInterrupt]): + with self.subTest(inputs=inputs), self.assertRaisesRegex(Exception, '^Import canceled$'): + self._pick(self.folders, *inputs) + self.assertIs(self._pick(self.folders, EOFError)[0], self.folders) + + def test_path_falls_back_to_name(self): + for folder in ({'folderName': 'Bare'}, {'folderName': 'Bare', 'folderPath': None}, + {'folderName': 'Bare', 'folderPath': ''}, _folder(116, 'Bare', parent=999, with_path=False)): + with self.subTest(folder=folder): + self.assertPrinted(self._pick([folder], '1')[1], '[1] Bare - Path -> Bare') + + def test_path_is_built_from_parents(self): + raw = [_folder(101, 'Test_Folder_l1', with_path=False), _folder(103, 'Test_Folder_l2', parent=101, with_path=False), + _folder(116, 'Test_Folder_L3', parent=103, with_path=False)] + constructed = list(ThycoticMixin._construct_folders([dict(f) for f in raw]).values()) + for folders in (raw, constructed): + with self.subTest(constructed=folders is constructed): + self.assertPrinted(self._pick(folders, '1')[1], + '[3] Test_Folder_L3 - Path -> Test_Folder_l1 / Test_Folder_l2 / Test_Folder_L3') + self.assertIn('folderPath', raw[2]) + + def test_cycles_and_self_parent_do_not_hang(self): + _, printed, _ = self._pick([_folder(10, 'Self', parent=10, with_path=False), + _folder(20, 'A', parent=21, with_path=False), + _folder(21, 'B', parent=20, with_path=False)], '1') + self.assertPrinted(printed, '[1] Self - Path -> Self') + self.assertPrinted(printed, '[2] A - Path -> B / A') + + def test_malformed_and_duplicate_entries_are_still_listed(self): + folders = [None, 'junk', {'id': 5}, {'id': 6, 'folderName': None}, _folder(7, 'Dup'), _folder(7, 'Dup2')] + result, printed, _ = self._pick(folders, '1,4,6') + self.assertEqual(result, [None, folders[3], folders[5]]) + for i in (1, 2, 3, 4): + self.assertPrinted(printed, f'[{i}] - Path -> ') + self.assertPrinted(printed, '[6] Dup2 - Path -> Dup2') + + +class ExpandFolderIdsTests(unittest.TestCase): + TREE = [_folder(101, 'l1'), _folder(103, 'l2', parent=101), _folder(108, 'l100', parent=101), + _folder(116, 'L3', parent=103), _folder(117, 'l4', parent=116)] + + def test_expands_descendants(self): + tree_dict = {f['id']: f for f in self.TREE} + for folders, roots, expected in ( + (self.TREE, [103], {103, 116, 117}), # descendants, not siblings + (tree_dict, [116], {116, 117}), # dict input + (self.TREE, [117], {117}), # leaf + (self.TREE, [999], {999}), # unknown root + (self.TREE, [101, 101, 116], {101, 103, 108, 116, 117}), # duplicate root and nested root + (self.TREE, [None, 103], {103, 116, 117}), # None root ignored + (self.TREE, [None], set()), + (self.TREE, ['101'], {'101'}), # string ids are not converted + ({200: _folder(200, 'Mine', parent=1), 201: _folder(201, 'Sub', parent=200)}, [1], {1, 200, 201}), + ([_folder(10, 'A', parent=10)], [10], {10}), # self-parent + ([_folder(10, 'A', parent=11), _folder(11, 'B', parent=10)], [10], {10, 11}), # cycle + ([_folder(101, 'l1'), _folder(103, 'l2', parent='101')], [101], {101}), # string parent id + ([None, 'junk', {'parentFolderId': 101}, {'id': None, 'parentFolderId': 101}] + self.TREE[:2], + [101], {101, 103}), # malformed entries + ): + with self.subTest(roots=roots, folders=folders): + self.assertEqual(ThycoticMixin.expand_folder_ids(folders, roots), expected) + + def test_empty_and_none_inputs_return_empty_set(self): + for folders, roots in ((self.TREE, []), (self.TREE, None), ([], [101]), ({}, [101]), (None, [101]), (None, None)): + with self.subTest(folders=folders, roots=roots): + self.assertEqual(ThycoticMixin.expand_folder_ids(folders, roots), set()) + + +class ConstructFoldersTests(unittest.TestCase): + def test_non_list_and_empty_inputs_return_empty_dict(self): + for folder_rs in (None, {}, {'error': 'bad'}, 'text', 5, [], (_folder(1, 'a'),)): + with self.subTest(folder_rs=folder_rs): + self.assertEqual(ThycoticMixin._construct_folders(folder_rs), {}) + + def test_entries_are_normalised(self): + folders = ThycoticMixin._construct_folders([ + None, 'junk', {'folderName': 'NoId'}, {'id': None, 'folderName': 'NullId'}, # skipped + {'id': 5}, {'id': 6, 'folderName': None}, # defaults + _folder(7, 'First'), _folder(7, 'Second'), # duplicate keeps first + dict(_folder(8, 'Shared'), inheritPermissions=False, extra='dropped'), + ]) + self.assertEqual(set(folders), {5, 6, 7, 8}) + self.assertEqual(folders[5], {'id': 5, 'folderName': '', 'inheritPermissions': True, + 'parentFolderId': None, 'folderPath': ''}) + self.assertEqual(folders[6]['folderName'], '') + self.assertEqual(folders[7]['folderName'], 'First') + self.assertIs(folders[8]['inheritPermissions'], False) + self.assertNotIn('extra', folders[8]) + + def test_folder_paths(self): + for raw, folder_id, expected in ( + ([_folder(101, 'l1'), _folder(103, 'l2', parent=101)], 103, 'l1\\l2'), + ([_folder(1, 'Personal Folders'), _folder(200, 'Mine', parent=1)], 200, 'Mine'), + ([_folder(10, 'Self', parent=10)], 10, 'Self'), + ([_folder(20, 'A', parent=21), _folder(21, 'B', parent=20)], 20, 'B\\A'), + ([_folder(20, 'A', parent=21), _folder(21, 'B', parent=20)], 21, 'A\\B'), + ) + tuple(([_folder(101, 'l1'), _folder(103, 'l2', parent=p)], 103, 'l2') for p in (None, -1, 0, 1, 999, '101')): + with self.subTest(raw=raw, folder_id=folder_id): + self.assertEqual(ThycoticMixin._construct_folders(raw)[folder_id]['folderPath'], expected) + + +class GetFoldersTests(unittest.TestCase): + def setUp(self): + self.auth = mock.Mock() + self.auth.thycotic_search.return_value = [ + _folder(101, 'Test_Folder_l1'), _folder(103, 'Test_Folder_l2', parent=101), + _folder(116, 'Test_Folder_L3', parent=103), _folder(117, 'Test_Folder_l4', parent=116), + _folder(108, 'Test_Folder_l100', parent=101), _folder(1, 'Personal Folders'), + ] + + def test_load_folders_keeps_personal_and_get_folders_drops_it(self): + for loader, expected in ((ThycoticMixin._load_folders, {1, 101, 103, 116, 117, 108}), + (lambda auth: ThycoticMixin.get_folders(auth, skip_permissions=True), + {101, 103, 116, 117, 108})): + with self.subTest(expected=expected), mock.patch('builtins.input') as mocked_input: + self.auth.thycotic_search.reset_mock() + folders = loader(self.auth) + mocked_input.assert_not_called() + self.auth.thycotic_search.assert_called_once_with('/v1/folders') + self.assertEqual(set(folders), expected) + self.assertEqual(folders[117]['folderPath'], L4) + + def test_paths_are_built_when_source_omits_them(self): + self.auth.thycotic_search.return_value = [ + _folder(101, 'Test_Folder_l1', with_path=False), _folder(103, 'Test_Folder_l2', parent=101, with_path=False), + _folder(116, 'Test_Folder_L3', parent=103, with_path=False), _folder(1, 'Personal Folders', with_path=False)] + self.assertEqual(ThycoticMixin.get_folders(self.auth, skip_permissions=True)[116]['folderPath'], L3) + + def test_odd_responses(self): + for response, loaded, public in ( + (None, set(), set()), ([], set(), set()), ({'error': 'bad'}, set(), set()), ('text', set(), set()), + ([_folder(1, 'Personal Folders')], {1}, set()), + ([None, 'junk', {'folderName': 'NoId'}, _folder(1, 'P'), _folder(1, 'Dup')], {1}, set()), + ([_folder(1, 'P'), _folder(200, 'Mine', parent=1)], {1, 200}, {200}), + ): + with self.subTest(response=response): + self.auth.thycotic_search.return_value = response + self.assertEqual(set(ThycoticMixin._load_folders(self.auth)), loaded) + self.assertEqual(set(ThycoticMixin.get_folders(self.auth, skip_permissions=True)), public) + + def test_search_error_propagates(self): + self.auth.thycotic_search.side_effect = RuntimeError('boom') + for loader in (ThycoticMixin._load_folders, lambda auth: ThycoticMixin.get_folders(auth, skip_permissions=True)): + with self.subTest(loader=loader), self.assertRaisesRegex(RuntimeError, 'boom'): + loader(self.auth) + + +class ImportFolderSelectionTests(unittest.TestCase): + def setUp(self): + # Menu order matters: choice '3' must be Test_Folder_L3 (116), '6' is Personal Folders (1) + self.raw_folders = [ + _folder(101, 'Test_Folder_l1'), _folder(103, 'Test_Folder_l2', parent=101), + _folder(116, 'Test_Folder_L3', parent=103), _folder(117, 'Test_Folder_l4', parent=116), + _folder(108, 'Test_Folder_l100', parent=101), _folder(1, 'Personal Folders'), + ] + + def _import(self, choice, secrets, raw_folders=None, filter_folder=None, input_error=None, interactive=True, + lookup=None): + raw_folders = self.raw_folders if raw_folders is None else raw_folders + secrets_by_id = {secret['id']: secret for secret in secrets} + + def search(endpoint): + if endpoint == '/v1/folders': + return raw_folders + return [{'id': secret['id']} for secret in secrets] if lookup is None else lookup + + def entity(endpoint): + return secrets_by_id[int(endpoint.rstrip('/').rsplit('/', 1)[-1])] + + input_patch = mock.patch('builtins.input', side_effect=input_error, + return_value=None if input_error is not None else choice) + with mock.patch('keepercommander.importer.thycotic.thycotic.ThycoticAuth') as auth_cls, \ + mock.patch('keepercommander.importer.thycotic.thycotic.getpass.getpass', return_value='pw'), \ + mock.patch('keepercommander.importer.thycotic.thycotic.os.path.isfile', return_value=False), \ + mock.patch('keepercommander.importer.thycotic.thycotic.sys.stdin') as mocked_stdin, \ + input_patch as mocked_input, mock.patch('builtins.print'): + mocked_stdin.isatty.return_value = interactive + auth = auth_cls.return_value + auth.thycotic_search.side_effect = search + auth.thycotic_entity.side_effect = entity + kwargs = {} if filter_folder is None else {'filter_folder': filter_folder} + try: + records = list(ThycoticImporter().do_import('user@thycotic.example', **kwargs)) + finally: + self.last_endpoints = [call.args[0] for call in auth.thycotic_search.call_args_list] + return records, mocked_input + + @staticmethod + def _paths(records): + return {record.title: record.folders[0].path if record.folders else None for record in records} + + def test_picked_folders(self): + personal = self.raw_folders + [_folder(200, 'My_Stuff', parent=1)] + for choice, secrets, raw, endpoints, paths in ( + ('3', [_secret(501, 117), _secret(502, 108), _secret(503, 116)], None, [_lookup(116)], + {'secret-501': L4, 'secret-502': None, 'secret-503': L3}), + ('2,3', [_secret(501, 103), _secret(502, 117), _secret(503, 101), _secret(504, 108)], None, + [_lookup(103), _lookup(116)], + {'secret-501': L2, 'secret-502': L4, 'secret-503': None, 'secret-504': None}), + ('6', [_secret(501, 200), _secret(502, 101), _secret(503, 1)], personal, [_lookup(1)], + {'secret-501': 'My_Stuff', 'secret-502': None, 'secret-503': None}), + ('1-6', [_secret(501, 117)], None, [_lookup(i) for i in (101, 103, 116, 117, 108, 1)], + {'secret-501': L4}), + ('5', [], None, [_lookup(108)], {}), + ('1', [_secret(501, 300)], [_folder(300, 'Orphan', parent=999)], [_lookup(300)], {'secret-501': 'Orphan'}), + ): + with self.subTest(choice=choice): + records, _ = self._import(choice, secrets, raw_folders=raw) + self.assertEqual(self.last_endpoints, ['/v1/folders'] + endpoints) + self.assertEqual(self._paths(records), paths) + + def test_imports_all_and_drops_personal(self): + secrets = [_secret(501, 117), _secret(502, 1), _secret(503, 108), _secret(504, 999)] + expected = {'secret-501': L4, 'secret-502': None, 'secret-503': r'Test_Folder_l1\Test_Folder_l100', + 'secret-504': None} + for label, kwargs, prompted in (('blank', {'choice': ''}, True), ('A', {'choice': 'A'}, True), + ('a', {'choice': ' a '}, True), + ('eof', {'choice': None, 'input_error': EOFError}, True), + ('non-interactive', {'choice': '3', 'interactive': False}, False)): + with self.subTest(label): + records, mocked_input = self._import(secrets=secrets, **kwargs) + self.assertEqual(mocked_input.called, prompted) + self.assertEqual(self.last_endpoints, ['/v1/folders', '/v1/secrets/lookup']) + self.assertEqual(self._paths(records), expected) + + def test_no_folders_imports_all_without_prompting(self): + for raw_folders in ([], None, {'error': 'bad'}, [None, 'junk']): + with self.subTest(raw_folders=raw_folders): + records, mocked_input = self._import('1', [_secret(501, 117)], + raw_folders=raw_folders if raw_folders is not None else []) + mocked_input.assert_not_called() + self.assertEqual(self.last_endpoints, ['/v1/folders', '/v1/secrets/lookup']) + self.assertEqual(self._paths(records), {'secret-501': None}) + + def test_non_list_secret_lookup_imports_nothing(self): + for choice in ('', '3'): + with self.subTest(choice=choice): + self.assertEqual(self._import(choice, [_secret(501, 117)], lookup={'error': 'bad'})[0], []) + + def test_filter_folder_skips_the_picker(self): + duplicate = self.raw_folders + [_folder(300, 'Test_Folder_L3')] + for filter_folder, raw, endpoints, paths in ( + ('Test_Folder_L3', None, [_lookup(116)], {'secret-501': L4}), + ('test_folder_l4', None, [_lookup(117)], {'secret-501': L4}), + ('Personal Folders', None, [_lookup(1)], {'secret-501': None}), # 117 is outside the subtree + ('Test_Folder_L3', duplicate, [_lookup(116), _lookup(300)], {'secret-501': L4}), + ('Does_Not_Exist', None, [], {}), + ): + with self.subTest(filter_folder=filter_folder, duplicate=raw is not None): + records, mocked_input = self._import('3', [_secret(501, 117)], raw_folders=raw, + filter_folder=filter_folder) + mocked_input.assert_not_called() + self.assertEqual(self.last_endpoints, ['/v1/folders'] + endpoints) + self.assertEqual(self._paths(records), paths) + + def test_keyboard_interrupt_cancels_before_secret_lookup(self): + with self.assertRaisesRegex(Exception, '^Import canceled$'): + self._import(None, [_secret(501, 117)], input_error=KeyboardInterrupt) + self.assertEqual(self.last_endpoints, ['/v1/folders']) + + +class GetSecretIdsTests(unittest.TestCase): + def setUp(self): + self.auth = mock.Mock() + + def _endpoints(self): + return [call.args[0] for call in self.auth.thycotic_search.call_args_list] + + def test_queries_each_folder_and_removes_duplicates(self): + self.auth.thycotic_search.side_effect = [[{'id': 1}, {'id': 2}], [{'id': 2}, {'id': 3}], [{'id': 3}]] + self.assertEqual(ThycoticMixin.get_secret_ids(self.auth, [103, 116, 116], 'filter.includeSubFolders=true'), + [1, 2, 3]) + self.assertEqual(self._endpoints(), [_lookup(103), _lookup(116), _lookup(116)]) + + def test_no_usable_folder_ids_make_no_calls(self): + for folder_ids in ([], None, (), [None]): + with self.subTest(folder_ids=folder_ids): + self.assertEqual(ThycoticMixin.get_secret_ids(self.auth, folder_ids, 'q'), []) + self.auth.thycotic_search.assert_not_called() + + def test_query_encoding(self): + for folder_ids, query, expected in (([5], '', '/v1/secrets/lookup?filter.folderId=5'), + ([None, 5], None, '/v1/secrets/lookup?filter.folderId=5'), + ([5], 'a=1&b=x y', '/v1/secrets/lookup?filter.folderId=5&a=1&b=x+y')): + with self.subTest(query=query): + self.auth.reset_mock() + self.auth.thycotic_search.return_value = [] + ThycoticMixin.get_secret_ids(self.auth, folder_ids, query) + self.auth.thycotic_search.assert_called_once_with(expected) + + def test_non_list_response_and_malformed_secrets_are_ignored(self): + self.auth.thycotic_search.side_effect = [None, {'error': 'bad'}, 'text', [None, {}, {'id': None}, {'id': 7}]] + self.assertEqual(ThycoticMixin.get_secret_ids(self.auth, [1, 2, 3, 4], ''), [7]) + + def test_search_error_propagates(self): + self.auth.thycotic_search.side_effect = RuntimeError('boom') + with self.assertRaisesRegex(RuntimeError, 'boom'): + ThycoticMixin.get_secret_ids(self.auth, [5], '') + + +if __name__ == '__main__': + unittest.main() From 7c5a308f796d8f117387066eadababcf245a1af4 Mon Sep 17 00:00:00 2001 From: mkanadi-ks Date: Thu, 29 Oct 2026 11:02:23 +0530 Subject: [PATCH 2/6] MM-25 Added Create users functionality in delinea import (#2457) * MM-25 Added Create users functionality in delinea import Added a new flag "--add-thycotic-users" to create users. Added test cases for create users functionality. * MM-25 Enhanced Dry-Run to display a preview of user details eligible for migration. Removed reporting mechanism. Added error handling to continue processing remaining users if a single user fetch fails. * MM-25 implemented add-users as common flag. Improved error handling. Optimized user email extractor function. Updated test cases. * MM-25 removed dependency on global variable, instead added function to check if format supports add users functionality or not. --- keepercommander/importer/commands.py | 5 +- keepercommander/importer/imp_exp.py | 9 +- keepercommander/importer/importer.py | 6 + keepercommander/importer/thycotic/thycotic.py | 174 ++++++++++- unit-tests/test_thycotic_add_users.py | 286 ++++++++++++++++++ 5 files changed, 475 insertions(+), 5 deletions(-) create mode 100644 unit-tests/test_thycotic_add_users.py diff --git a/keepercommander/importer/commands.py b/keepercommander/importer/commands.py index 7a701f988..2d09c3a70 100644 --- a/keepercommander/importer/commands.py +++ b/keepercommander/importer/commands.py @@ -63,6 +63,7 @@ def _cyberark_skip_arg(value): return ",".join(skip_targets) + def _positive_int_arg(value): try: depth = int(value) @@ -123,6 +124,8 @@ def _positive_int_arg(value): help='display skipped records (always enabled for CyberArk imports)') import_parser.add_argument('--secret-ids', dest='secret_ids', action='store', help='Comma separated list of secret IDs to fetch (Thycotic)') +import_parser.add_argument('--add-users', dest='add_users', action='store_true', + help='provision source users as pre-activated Keeper enterprise users') import_parser.add_argument('--target-node', '--node', dest='target_node', action='store', help='node name or ID for CyberArk-provisioned users, teams, and roles (default: root node)') import_parser.add_argument('--skip', dest='skip', action='store', type=_cyberark_skip_arg, @@ -790,4 +793,4 @@ def importer_for_format(input_format): return module.Importer raise Exception('Cannot resolve importer for format {}'.format(input_format)) except ModuleNotFoundError as e: - raise CommandError('', f'The required module is not installed:\n\tpip install {e.name}') + raise CommandError('', f'The required module is not installed:\n\tpip install {e.name}') \ No newline at end of file diff --git a/keepercommander/importer/imp_exp.py b/keepercommander/importer/imp_exp.py index f6c67317d..7367e5219 100644 --- a/keepercommander/importer/imp_exp.py +++ b/keepercommander/importer/imp_exp.py @@ -851,6 +851,7 @@ def _import(params, file_format, filename, **kwargs): skip_team = kwargs.get('skip_team') is True or 'team' in cyberark_skip skip_role = kwargs.get('skip_role') is True or 'role' in cyberark_skip skip_user = kwargs.get('skip_user') is True or 'user' in cyberark_skip + add_users = kwargs.get('add_users') is True import_into_raw = kwargs.get('import_into') or '' import_into = import_into_raw @@ -860,6 +861,9 @@ def _import(params, file_format, filename, **kwargs): no_shortcuts = kwargs.get('no_shortcuts') or False importer = importer_for_format(file_format)() # type: BaseImporter + if add_users and not importer.support_add_users(): + logging.warning('--add-users is not supported for --format=%s; ignoring', file_format) + add_users = False verbose_import_summary = importer.verbose_import_summary show_skipped = show_skipped or verbose_import_summary @@ -879,7 +883,8 @@ def _import(params, file_format, filename, **kwargs): for x in importer.execute(filename, params=params, users_only=import_users, filter_folder=filter_folder, old_domain=old_domain, new_domain=new_domain, tmpdir=tmpdir, secret_ids=secret_ids, dry_run=dry_run, target_node=target_node, use_nsf=use_nsf, record_type=record_type, - skip_team=skip_team, skip_role=skip_role, skip_user=skip_user): + skip_team=skip_team, skip_role=skip_role, skip_user=skip_user, + add_users=add_users): if isinstance(x, ImportRecord): if filter_folder and not importer.support_folder_filter(): if not x.folders: @@ -3073,4 +3078,4 @@ def open(self): rs_http = requests.get(url, proxies=self.params.rest_context.proxies, stream=True) if rs_http.status_code != file.success_status_code: raise KeeperApiError('file_not_found', 'Attachment: file not found') - return self.set_stream(rs_http.raw, for_encrypt=False) + return self.set_stream(rs_http.raw, for_encrypt=False) \ No newline at end of file diff --git a/keepercommander/importer/importer.py b/keepercommander/importer/importer.py index 928a4e10e..b99ad16a1 100644 --- a/keepercommander/importer/importer.py +++ b/keepercommander/importer/importer.py @@ -325,6 +325,12 @@ def extension(self): def support_folder_filter(self): return False + def support_add_users(self): + """ + Indicates if this importer can provision source users as Keeper Enterprise users + """ + return False + @staticmethod def import_field(field_type, field_value): # type: (str, str) -> any if not field_value: diff --git a/keepercommander/importer/thycotic/thycotic.py b/keepercommander/importer/thycotic/thycotic.py index 16e1c5803..aea1df7d7 100644 --- a/keepercommander/importer/thycotic/thycotic.py +++ b/keepercommander/importer/thycotic/thycotic.py @@ -16,6 +16,7 @@ import json import logging import os +import re import sys import time import warnings @@ -25,12 +26,14 @@ import xml.etree.ElementTree as ET import requests +from tabulate import tabulate from urllib.parse import parse_qsl, urlencode, urlparse, urlunparse from typing import Tuple, Optional, Iterable, Union, Dict, Callable, Any, List from ..importer import (BaseImporter, BaseDownloadMembership, Record, SharedFolder, Folder, Attachment, Permission, RecordField, BytesAttachment, Team, BaseDownloadRecordType, RecordType, RecordTypeField) +from ...constants import EMAIL_PATTERN from ...params import KeeperParams from ... import record_types, vault, api @@ -373,7 +376,10 @@ def __init__(self): def support_folder_filter(self): return True - + + def support_add_users(self): + return True + @staticmethod def pop_field(fields, key): # type: (Dict[str, Dict[str, str]], str) -> Tuple[str, Optional[Dict[str, str]]] field = fields.pop(key, None) @@ -398,6 +404,162 @@ def _send_keep_alive_if_needed(self, params): # type: (KeeperParams) -> None except: pass + @staticmethod + def _user_field(user, key): # type: (dict, str) -> str + return str(user.get(key) or '').strip() + + @staticmethod + def _extract_user_email(user): + """Return the user's emailAddress, or an empty string.""" + if not isinstance(user, dict): + return '' + for key, value in user.items(): + if str(key).lower() == 'emailaddress' and isinstance(value, str): + return value.strip() + return '' + + def _prepare_thycotic_users(self, auth): # type: (ThycoticAuth) -> List[dict] + """Fetch Delinea users that have an email address, including inactive users. + + Application accounts are omitted. + When the user list has no email, the user detail endpoint is tried once. + """ + all_users = auth.thycotic_search('/v1/users?filter.includeInactive=true') + result = [] + skipped_application = 0 + skipped_no_email = 0 + for user in all_users or []: + if not isinstance(user, dict): + continue + if user.get('isApplicationAccount') is True: + skipped_application += 1 + continue + email = self._extract_user_email(user) + user_id = user.get('id') + source = user + if not email and user_id not in (None, ''): + try: + detail = auth.thycotic_entity(f'/v1/users/{user_id}') + except Exception as e: + logging.warning('Failed to fetch Thycotic user %s: %s', user_id, e) + detail = None + if isinstance(detail, dict): + if detail.get('isApplicationAccount') is True: + skipped_application += 1 + continue + email = self._extract_user_email(detail) + source = detail + if not email: + skipped_no_email += 1 + continue + username = str(source.get('userName') or user.get('userName') or '').strip() + display = str(source.get('displayName') or user.get('displayName') or '').strip() + result.append({ + 'id': user_id, + 'username': username, + 'name': display or username or email, + 'email': email, + }) + if skipped_application: + logging.info('Skipped %d Thycotic application account(s)', skipped_application) + if skipped_no_email: + logging.info('Skipped %d Thycotic user(s) with no email address', skipped_no_email) + logging.info(f'Found {len(result)} Thycotic user(s) with an email address') + return result + + def _load_existing_keeper_users(self, params): # type: (KeeperParams) -> list + try: + if not getattr(params, 'enterprise', None): + api.query_enterprise(params) + enterprise_data = getattr(params, 'enterprise', None) + if not isinstance(enterprise_data, dict): + raise ValueError('enterprise data is unavailable') + return [ + { + 'email': self._user_field(user, 'email') or self._user_field(user, 'username'), + 'status': user.get('status') or 'active', + } + for user in enterprise_data.get('users') or [] + if isinstance(user, dict) + ] + except Exception as e: + logging.warning('Failed to load existing Keeper users: %s', e) + return [] + + def _create_keeper_user(self, params, email, name): + # type: (KeeperParams, str, str) -> Optional[str] + """Pre-activate one Keeper user.""" + from ...commands.enterprise_create_user import CreateEnterpriseUserCommand + try: + creator = CreateEnterpriseUserCommand() + return creator.execute(params, email=email, name=name) + except Exception as e: + logging.warning('Could not create enterprise user "%s": %s', email, e) + return None + + def _create_enterprise_users(self, params, thycotic_users): + # type: (KeeperParams, List[dict]) -> None + seen = set() + existing_users = self._load_existing_keeper_users(params) + is_valid_email = re.compile(EMAIL_PATTERN).match + for user in thycotic_users or []: + if not isinstance(user, dict): + continue + email = self._user_field(user, 'email') + email_key = email.casefold() + if email_key in seen: + continue + user_exist = next( + (u for u in existing_users if (u.get('email') or '').casefold() == email_key), + None, + ) + if user_exist: + seen.add(email_key) + continue + if not is_valid_email(email): + continue + seen.add(email_key) + name = self._user_field(user, 'name') or self._user_field(user, 'username') or email + self._create_keeper_user(params, email, name) + + def _preview_thycotic_users(self, auth): + # type: (ThycoticAuth) -> None + """Fetch Delinea users and print them. Does not create Keeper users.""" + logging.info('Dry run: fetching Thycotic users (no Keeper users will be created)...') + try: + thycotic_users = self._prepare_thycotic_users(auth) + except Exception as e: + logging.warning('Failed to fetch Thycotic users: %s', e) + return + if not thycotic_users: + logging.info('No Thycotic users are available to migrate.') + return + rows = [ + { + 'Username': user.get('username') or '', + 'Email': user.get('email') or '', + 'Name': user.get('name') or '', + } + for user in thycotic_users + ] + logging.info(f'Thycotic users available to migrate ({len(rows)}):') + logging.info(tabulate(rows, headers='keys')) + + def _migrate_thycotic_users(self, auth, params): + # type: (ThycoticAuth, KeeperParams) -> None + """Provision Delinea users as pre-activated Keeper users, then continue the import. + + A user-fetch failure does not abort secret migration. + """ + logging.info('Migrating Thycotic users to Keeper...') + try: + thycotic_users = self._prepare_thycotic_users(auth) + except Exception as e: + logging.warning('Failed to fetch Thycotic users: %s', e) + logging.warning('Continuing import without creating Keeper users.') + return + self._create_enterprise_users(params, thycotic_users) + def do_import(self, filename, **kwargs): # type: (ThycoticImporter, str, ...) -> Iterable[Union[Record, SharedFolder]] loaded_record_types = {} @@ -443,6 +605,14 @@ def do_import(self, filename, **kwargs): auth.proxy = params.rest_context.proxies if params else None auth.authenticate(username, password, ThycoticImporter.request_totp) + # Pre-activate users before secrets are imported so later migration steps + # can share data to accounts that already exist. + if kwargs.get('add_users') is True: + if kwargs.get('dry_run') is True: + self._preview_thycotic_users(auth) + else: + self._migrate_thycotic_users(auth, params) + totp_codes = {} csv_file = 'secrets-export.csv' if not os.path.isfile('secrets-export.csv'): @@ -1372,4 +1542,4 @@ def download_record_type(self, params, **kwargs): rf = RecordTypeField.create('fileRef', '') rt.fields.append(rf) - yield rt + yield rt \ No newline at end of file diff --git a/unit-tests/test_thycotic_add_users.py b/unit-tests/test_thycotic_add_users.py new file mode 100644 index 000000000..cca52233d --- /dev/null +++ b/unit-tests/test_thycotic_add_users.py @@ -0,0 +1,286 @@ +import logging +import unittest +from types import SimpleNamespace +from unittest import mock + +from keepercommander.importer.commands import import_parser +from keepercommander.importer.thycotic.thycotic import ThycoticImporter + + +class _QuietTestCase(unittest.TestCase): + def setUp(self): + self._patch('builtins.print') + self._patch('keepercommander.importer.thycotic.thycotic.logging') + self.log = self._mocks['logging'] + + def _patch(self, target, **kwargs): + patcher = mock.patch(target, **kwargs) + name = target.rsplit('.', 1)[-1] + self.__dict__.setdefault('_mocks', {})[name] = patcher.start() + self.addCleanup(patcher.stop) + + def importer(self): + return ThycoticImporter() + + +class TestExtractUserEmail(_QuietTestCase): + def test_email_priority_and_edge_cases(self): + cases = [ + ('emailAddress is used', {'emailAddress': 'biz@x.com', 'email': 'other@x.com'}, 'biz@x.com'), + ('email is ignored', {'email': 'other@x.com'}, ''), + ('blank emailAddress is empty', {'emailAddress': ' ', 'email': 'other@x.com'}, ''), + ('keys are case-insensitive', {'EmailAddress': ' Biz@x.com '}, 'Biz@x.com'), + ('non-string is ignored', {'emailAddress': 42, 'email': 'other@x.com'}, ''), + ('no address', {'userName': 'bob'}, ''), + ('None input', None, ''), + ('list input', ['a@x.com'], ''), + ] + importer = self.importer() + for name, user, expected in cases: + with self.subTest(name): + self.assertEqual(importer._extract_user_email(user), expected) + + +class TestPrepareThycoticUsers(_QuietTestCase): + def _prepare(self, users, details=None): + auth = mock.MagicMock() + auth.thycotic_search.return_value = users + auth.thycotic_entity.side_effect = details or (lambda endpoint: None) + found = self.importer()._prepare_thycotic_users(auth) + return found, auth + + def test_list_email_skips_detail_fetch(self): + found, auth = self._prepare([ + {'id': 1, 'userName': 'a', 'displayName': 'Alice', 'emailAddress': 'a@x.com'}, + ]) + self.assertEqual(found, [{ + 'id': 1, 'username': 'a', 'name': 'Alice', 'email': 'a@x.com', + }]) + auth.thycotic_search.assert_called_once_with('/v1/users?filter.includeInactive=true') + auth.thycotic_entity.assert_not_called() + + def test_missing_email_is_fetched_from_user_detail(self): + found, auth = self._prepare( + [{'id': 7, 'userName': 'b', 'displayName': ''}], + details=lambda endpoint: {'emailAddress': 'b@x.com', 'displayName': 'Bob'}, + ) + self.assertEqual(found, [{ + 'id': 7, 'username': 'b', 'name': 'Bob', 'email': 'b@x.com', + }]) + auth.thycotic_entity.assert_called_once_with('/v1/users/7') + + def test_detail_fetch_failure_skips_only_that_user(self): + def details(endpoint): + if str(endpoint).endswith('/1'): + raise RuntimeError('403 Forbidden') + return {'emailAddress': 'ok@x.com', 'displayName': 'Ok'} + + found, _ = self._prepare( + [ + {'id': 1, 'userName': 'bad'}, + {'id': 2, 'userName': 'ok', 'displayName': ''}, + ], + details=details, + ) + self.assertEqual([u['username'] for u in found], ['ok']) + self.log.warning.assert_called() + + def test_application_accounts_are_omitted(self): + found, auth = self._prepare([ + {'id': 1, 'userName': 'svc', 'emailAddress': 'svc@x.com', 'isApplicationAccount': True}, + {'id': 2, 'userName': 'ok', 'emailAddress': 'ok@x.com', 'isApplicationAccount': False}, + ]) + self.assertEqual([u['username'] for u in found], ['ok']) + auth.thycotic_entity.assert_not_called() + self.log.info.assert_called() + + def test_rows_without_usable_email_are_omitted(self): + found, auth = self._prepare([ + {'userName': 'no-email-no-id'}, + {'id': '', 'userName': 'blank-id'}, + {'id': 3, 'userName': 'details-empty'}, + 'not-a-dict', + None, + ]) + self.assertEqual(found, []) + auth.thycotic_entity.assert_called_once_with('/v1/users/3') + + def test_inactive_users_are_included(self): + found, _ = self._prepare([ + {'id': 1, 'userName': 'gone', 'displayName': 'Gone', 'emailAddress': 'gone@x.com', 'enabled': False}, + ]) + self.assertEqual(found[0]['email'], 'gone@x.com') + + def test_name_falls_back_to_username(self): + found, _ = self._prepare([{'id': 1, 'userName': 'carol', 'emailAddress': 'c@x.com'}]) + self.assertEqual(found[0]['name'], 'carol') + self.assertEqual(found[0]['username'], 'carol') + + +class TestLoadExistingKeeperUsers(_QuietTestCase): + def setUp(self): + super().setUp() + self._patch('keepercommander.importer.thycotic.thycotic.api.query_enterprise') + self.query = self._mocks['query_enterprise'] + + def test_cached_enterprise_with_real_keeper_shape(self): + params = SimpleNamespace(enterprise={'users': [ + {'username': ' Alice@X.com ', 'status': 'invited'}, + {'username': 'bob@x.com'}, + 'junk', + ]}) + users = self.importer()._load_existing_keeper_users(params) + self.assertEqual(users, [ + {'email': 'Alice@X.com', 'status': 'invited'}, + {'email': 'bob@x.com', 'status': 'active'}, + ]) + self.query.assert_not_called() + + def test_enterprise_is_queried_when_not_loaded(self): + params = SimpleNamespace(enterprise=None) + self.query.side_effect = lambda p: setattr(p, 'enterprise', {'users': [{'email': 'a@x.com'}]}) + users = self.importer()._load_existing_keeper_users(params) + self.assertEqual(users, [{'email': 'a@x.com', 'status': 'active'}]) + + def test_query_failure_returns_empty_and_warns(self): + self.query.side_effect = RuntimeError('not an admin') + users = self.importer()._load_existing_keeper_users(SimpleNamespace(enterprise=None)) + self.assertEqual(users, []) + self.log.warning.assert_called_once() + + +class TestCreateEnterpriseUsers(_QuietTestCase): + def _run(self, users, existing=(), create_result='uid'): + importer = self.importer() + importer._load_existing_keeper_users = mock.MagicMock(return_value=list(existing)) + importer._create_keeper_user = mock.MagicMock(return_value=create_result) + importer._create_enterprise_users(object(), users) + return importer._create_keeper_user + + def test_creates_only_new_valid_users(self): + create = self._run( + [ + {'username': 'new', 'name': 'New User', 'email': 'new@x.com'}, + {'username': 'old', 'email': 'old@x.com'}, + {'username': 'bad', 'email': 'not-an-email'}, + {'username': 'blank', 'email': ''}, + {}, + '', + None, + ], + existing=[{'email': 'old@x.com', 'status': 'locked'}], + ) + create.assert_called_once_with(mock.ANY, 'new@x.com', 'New User') + + def test_existing_match_ignores_case(self): + create = self._run( + [{'username': 'john', 'email': 'John.Doe@Corp.com'}], + existing=[{'email': 'john.doe@corp.com', 'status': 'active'}], + ) + create.assert_not_called() + + def test_case_variant_duplicates_are_created_once(self): + create = self._run([ + {'username': 'a1', 'email': 'A@corp.com'}, + {'username': 'a2', 'email': 'a@corp.com'}, + ]) + create.assert_called_once() + + def test_email_is_trimmed_and_name_falls_back_to_email(self): + create = self._run([{'username': None, 'email': ' c@x.com '}]) + create.assert_called_once_with(mock.ANY, 'c@x.com', 'c@x.com') + + def test_failed_create_is_not_retried(self): + create = self._run( + [{'username': 'f', 'email': 'f@x.com'}, {'username': 'f', 'email': 'f@x.com'}], + create_result=None, + ) + create.assert_called_once() + + +class TestCreateKeeperUser(_QuietTestCase): + def setUp(self): + super().setUp() + patcher = mock.patch('keepercommander.commands.enterprise_create_user.CreateEnterpriseUserCommand') + self.command_cls = patcher.start() + self.addCleanup(patcher.stop) + + def test_forwards_email_and_name(self): + self.command_cls.return_value.execute.return_value = 'record-uid' + params = object() + self.assertEqual(self.importer()._create_keeper_user(params, 'a@x.com', 'Alice'), 'record-uid') + self.command_cls.return_value.execute.assert_called_once_with(params, email='a@x.com', name='Alice') + + def test_error_returns_none_and_warns(self): + self.command_cls.return_value.execute.side_effect = RuntimeError('reserved domain required') + self.assertIsNone(self.importer()._create_keeper_user(object(), 'a@x.com', 'Alice')) + self.log.warning.assert_called_once() + + +class TestMigrateDoesNotAbortImport(_QuietTestCase): + def test_fetch_failure_is_logged_and_swallowed(self): + importer = self.importer() + auth = mock.MagicMock() + auth.thycotic_search.side_effect = RuntimeError('401') + importer._migrate_thycotic_users(auth, object()) + self.assertGreaterEqual(self.log.warning.call_count, 2) + + +class TestDoImportUserGate(_QuietTestCase): + def _run(self, **kwargs): + importer = self.importer() + auth = mock.MagicMock() + auth.thycotic_search.return_value = [] + params = mock.Mock() + params.rest_context.proxies = None + with mock.patch('keepercommander.importer.thycotic.thycotic.getpass.getpass', return_value='pw'), \ + mock.patch('keepercommander.importer.thycotic.thycotic.ThycoticAuth', return_value=auth), \ + mock.patch.object(ThycoticImporter, 'get_folders', return_value={}), \ + mock.patch.object(importer, '_migrate_thycotic_users') as migrate, \ + mock.patch.object(importer, '_preview_thycotic_users') as preview, \ + mock.patch('os.path.isfile', return_value=False): + list(importer.do_import('user@secret.example.com', params=params, **kwargs)) + return migrate, preview + + def test_flag_provisions_users_before_secrets(self): + migrate, preview = self._run(add_users=True) + migrate.assert_called_once() + preview.assert_not_called() + + def test_flag_absent_does_not_provision(self): + migrate, preview = self._run() + migrate.assert_not_called() + preview.assert_not_called() + + def test_dry_run_prints_users_without_creating_them(self): + migrate, preview = self._run(add_users=True, dry_run=True) + migrate.assert_not_called() + preview.assert_called_once() + + def test_preview_prints_fetched_users(self): + importer = self.importer() + importer._prepare_thycotic_users = mock.Mock(return_value=[ + {'username': 'a', 'email': 'a@x.com', 'name': 'Alice'}, + ]) + importer._create_enterprise_users = mock.Mock() + importer._preview_thycotic_users(mock.Mock()) + printed = ' '.join(str(call) for call in self.log.info.call_args_list) + self.assertIn('Thycotic users available to migrate', printed) + self.assertIn('a@x.com', printed) + self.assertIn('Alice', printed) + importer._create_enterprise_users.assert_not_called() + + +class TestImportParser(unittest.TestCase): + def test_flag_defaults_off_and_parses(self): + off = import_parser.parse_args(['--format', 'thycotic', 'https://secret.example.com']) + self.assertFalse(off.add_users) + on = import_parser.parse_args([ + '--format', 'thycotic', '--add-users', 'https://secret.example.com', + ]) + self.assertTrue(on.add_users) + + +if __name__ == '__main__': + logging.basicConfig() + unittest.main() \ No newline at end of file From 53425ba747e8f8243a335a6585f81fc2e1c27031 Mon Sep 17 00:00:00 2001 From: tgaikwad-ks Date: Thu, 29 Oct 2026 15:11:14 +0530 Subject: [PATCH 3/6] =?UTF-8?q?Add=20support=20for=20thycotic(Delinea)=20r?= =?UTF-8?q?estricted=20secrets=20and=20implement=20re=E2=80=A6=20(#2461)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Add support for thycotic(Delinea) restricted secrets and implement related tests * Enhance Thycotic secret handling with improved error reporting and access parameters * Refactor ThycoticImporter to remove redundant attachment list checks * Refactor error logging in ThycoticAuth to simplify response handling * Add secret retrieval handling in ImportFolderSelectionTests * Enhance secret lookup functionality to include restricted secrets and improve response handling * Add support for retrieving secret IDs with restricted access in ThycoticMixin * Refactor ThycoticMixin to deduplicate folder IDs in secret ID lookup and update tests for improved query handling * Improve Thycotic secret handling: log warnings for legacy queries, refine error handling, and enhance logging for attachment imports * Add SSH key validation limits and improve query parameter handling --- keepercommander/importer/commands.py | 2 +- keepercommander/importer/thycotic/thycotic.py | 363 +++++++++++++---- ...test_thycotic_interactive_folder_picker.py | 33 +- unit-tests/test_thycotic_migration.py | 374 ++++++++++++++++++ 4 files changed, 680 insertions(+), 92 deletions(-) create mode 100644 unit-tests/test_thycotic_migration.py diff --git a/keepercommander/importer/commands.py b/keepercommander/importer/commands.py index 2d09c3a70..062d400f4 100644 --- a/keepercommander/importer/commands.py +++ b/keepercommander/importer/commands.py @@ -121,7 +121,7 @@ def _positive_int_arg(value): import_parser.add_argument('--file-cache', dest='tmpdir', action='store', help='temp directory used to cache encrypted attachment imports') import_parser.add_argument('--show-skipped', dest='show_skipped', action='store_true', - help='display skipped records (always enabled for CyberArk imports)') + help='display skipped records (always enabled for CyberArk and Thycotic imports)') import_parser.add_argument('--secret-ids', dest='secret_ids', action='store', help='Comma separated list of secret IDs to fetch (Thycotic)') import_parser.add_argument('--add-users', dest='add_users', action='store_true', diff --git a/keepercommander/importer/thycotic/thycotic.py b/keepercommander/importer/thycotic/thycotic.py index aea1df7d7..4f29405f0 100644 --- a/keepercommander/importer/thycotic/thycotic.py +++ b/keepercommander/importer/thycotic/thycotic.py @@ -28,7 +28,7 @@ import requests from tabulate import tabulate -from urllib.parse import parse_qsl, urlencode, urlparse, urlunparse +from urllib.parse import parse_qsl, quote, urlencode, urlparse, urlunparse from typing import Tuple, Optional, Iterable, Union, Dict, Callable, Any, List from ..importer import (BaseImporter, BaseDownloadMembership, Record, SharedFolder, Folder, Attachment, Permission, @@ -37,6 +37,93 @@ from ...params import KeeperParams from ... import record_types, vault, api +# Secret Server leaves restricted secrets out of /v1/secrets/lookup unless +# filter.includeRestricted=true. The API user needs this Delinea role permission +# or the lookup still omits them. +DELINEA_RESTRICTED_SECRET_PERMISSION = 'View Restricted Secret' + +# Sent on GET /v1/secrets/{id} and /v1/secrets/{id}/fields/{slug} only when a +# plain read fails. For secrets that require checkout or a comment, they check +# the secret out, read it, check it back in, and supply the comment. Secrets +# that do not need them are read without a checkout or an audit comment. +SECRET_ACCESS_PARAMS = ( + ('autoCheckout', 'true'), + ('autoCheckIn', 'true'), + ('autoComment', 'Checking out'), +) + + +SECRET_ACCESS_PARAM_NAMES = frozenset(name for name, _ in SECRET_ACCESS_PARAMS) + +# SSH key validation limits (in bytes when decoded) +SSH_KEY_MIN_LENGTH = 30 +SSH_KEY_MAX_LENGTH = 5000 + + +def _replace_query(endpoint, params): + # type: (str, List[Tuple[str, str]]) -> str + """Replace query string in endpoint with provided params, using proper URL encoding.""" + parts = urlparse(endpoint) + return urlunparse(parts._replace(query=urlencode(params, quote_via=quote))) + + +def with_secret_access_params(endpoint): + # type: (str) -> str + """Add checkout and comment parameters to a secret read URL. + + Existing query parameters are kept. Any existing checkout or comment + parameter is replaced, not duplicated. + """ + query = parse_qsl(urlparse(endpoint).query, keep_blank_values=True) + params = [(k, v) for k, v in query if k not in SECRET_ACCESS_PARAM_NAMES] + params.extend(SECRET_ACCESS_PARAMS) + return _replace_query(endpoint, params) + + + +def build_secrets_lookup_query(folder_id=None, include_restricted=True): + # type: (Optional[Union[int, str]], bool) -> str + """Build the secrets lookup used by import. + + filter.includeRestricted is always on the endpoint and defaults to true. + Secret Server treats it as a filter: the lookup still succeeds when the + API user cannot see restricted secrets, and those IDs stay out of the list. + Seeing them requires the Delinea role permission View Restricted Secret. + """ + restricted = 'true' if include_restricted else 'false' + if folder_id is None: + return f'/v1/secrets/lookup?filter.includeRestricted={restricted}' + return ( + f'/v1/secrets/lookup?filter.folderId={folder_id}' + f'&filter.includeSubFolders=true&filter.includeRestricted={restricted}' + ) + + +def _ids_from_lookup_response(response): + # type: (Any) -> List + """Extract secret IDs from lookup response, handling non-list responses safely.""" + if not isinstance(response, list): + return [] + return [item['id'] for item in response if isinstance(item, dict) and item.get('id') is not None] + + +def lookup_secret_ids(search, folder_ids=None, include_restricted=True): + # type: (Callable[[str], Iterable[dict]], Optional[Iterable], bool) -> List + """Return secret IDs from a tenant-wide or folder-filtered lookup.""" + if folder_ids: + secret_ids = [] + for folder_id in dict.fromkeys(folder_ids): # deduplicate folder IDs + if folder_id is None: + continue + query = build_secrets_lookup_query(folder_id, include_restricted) + secret_ids.extend(_ids_from_lookup_response(search(query))) + # A secret can be returned by more than one folder lookup: a matched + # folder may be a subfolder of another, and includeSubFolders=true + # returns its secrets again. Keep the first occurrence of each ID. + return list(dict.fromkeys(secret_ids)) + query = build_secrets_lookup_query(include_restricted=include_restricted) + return _ids_from_lookup_response(search(query)) + class ThycoticMixin: @staticmethod @@ -234,6 +321,26 @@ def expand_folder_ids(folders, root_ids): pending.append(child_id) return seen + @staticmethod + def get_secret_ids(auth, folder_ids, query=None): + # type: (ThycoticAuth, Optional[Iterable], Optional[str]) -> List + """Bridge function to get secret IDs for selected folders with restricted secret support. + + Args: + auth: Thycotic authentication object + folder_ids: Folder IDs to query + query: Legacy parameter (ignored; use build_secrets_lookup_query instead) + + Returns: + List of secret IDs from selected folders with includeRestricted=true + """ + if query is not None: + logging.warning('Legacy query parameter provided but is not used; using folder_ids instead') + ids = [folder_id for folder_id in (folder_ids or ()) if folder_id is not None] + if not ids: + return [] + return lookup_secret_ids(auth.thycotic_search, ids, include_restricted=True) + @staticmethod def _construct_folders(folder_rs): # type: (Any) -> Dict[int, Dict] @@ -352,23 +459,12 @@ def get_folders(auth, skip_permissions=False): folder['inheritPermissions'] = True return folders - @staticmethod - def get_secret_ids(auth, folder_ids, query): - secret_ids = [] - for folder_id in folder_ids or (): - if folder_id is None: - continue - params = [('filter.folderId', folder_id)] - if query: - params.extend(parse_qsl(query)) - secrets = auth.thycotic_search('/v1/secrets/lookup?' + urlencode(params)) - if not isinstance(secrets, list): - continue - secret_ids.extend(secret['id'] for secret in secrets - if isinstance(secret, dict) and secret.get('id') is not None) - return list(dict.fromkeys(secret_ids)) + class ThycoticImporter(BaseImporter, ThycoticMixin): + # Same end-of-import terminal summary CyberArk prints (Import finished, skipped rows). + verbose_import_summary = True + def __init__(self): super().__init__() self._last_keep_alive = int(datetime.datetime.utcnow().timestamp()) @@ -650,32 +746,36 @@ def do_import(self, filename, **kwargs): if picked_folders is not None and picked_folders is not folder_choices: picked_folder_ids = list(dict.fromkeys(folder['id'] for folder in picked_folders)) - + # Delete the system folder (ID 1) if 1 in constructed_folders: del constructed_folders[1] folders = constructed_folders - secrets_ids = [] - query = 'filter.includeSubFolders=true' - if filter_folder or picked_folder_ids is not None: - if filter_folder: - if filter_folder == 'Personal Folders': - matched_folder_ids = [1] - else: - matched_folder_ids = [x['id'] for x in folders.values() if x['folderName'].lower() == filter_folder.lower()] + # Determine which folders to query + matched_folder_ids = None + if filter_folder: + # Handle --filter-folder argument + if filter_folder == 'Personal Folders': + matched_folder_ids = [1] + else: + matched_folder_ids = [x['id'] for x in folders.values() if x['folderName'].lower() == filter_folder.lower()] if len(matched_folder_ids) == 0: logging.warning('Folder \"%s\" not found', filter_folder) - else: - matched_folder_ids = picked_folder_ids + return # Fail closed: don't import if filter folder not found + elif picked_folder_ids is not None: + # Use interactively selected folders + matched_folder_ids = picked_folder_ids + + # Query secrets with restricted secret support + if matched_folder_ids: + # Expand selected folder IDs to include subfolders folder_ids = ThycoticImporter.expand_folder_ids(folders, matched_folder_ids) folders = {i: x for i, x in folders.items() if i in folder_ids} - secrets_ids = ThycoticImporter.get_secret_ids(auth, matched_folder_ids, query) + # Use lookup_secret_ids for restricted secret support + secrets_ids = lookup_secret_ids(auth.thycotic_search, matched_folder_ids, include_restricted=True) else: - matched_folder_ids = list(folders.keys()) - secrets_rs = auth.thycotic_search('/v1/secrets/lookup') - secrets_ids = [x['id'] for x in secrets_rs if isinstance(x, dict) and x.get('id') is not None] \ - if isinstance(secrets_rs, list) else [] - + # Tenant-wide lookup with restricted secret support + secrets_ids = lookup_secret_ids(auth.thycotic_search, include_restricted=True) # secret_ids arg debug_ids = kwargs.get('secret_ids') if debug_ids is not None: @@ -703,9 +803,13 @@ def do_import(self, filename, **kwargs): self._send_keep_alive_if_needed(params) print(f'Loading {len(secrets_ids)} Records ', flush=True, end='') secrets = [] + skipped_secrets = [] + skipped_fields = [] for secret_id in secrets_ids: - secret = auth.thycotic_entity(f'/v1/secrets/{secret_id}') - if not secret: + secret, error = auth.thycotic_secret(f'/v1/secrets/{secret_id}') + if secret is None: + status, message = error or ('N/A', 'Empty response') + skipped_secrets.append({'ID': secret_id, 'Status': status, 'Message': message}) continue secrets.append(secret) if len(secrets) % 10 == 9: @@ -792,28 +896,32 @@ def do_import(self, filename, **kwargs): endpoint = f'/v1/secrets/{secret_id}/fields/{slug}' if slug in ('private-key', 'public-key') and skip_ssh_key is not True: - with auth.thycotic_get(endpoint) as rs: - if rs.status_code == 200: - try: - key_text = rs.content.decode() - except: - key_text = '' - is_key = 30 < len(key_text) < 5000 - if is_key: - item['itemValue'] = key_text + try: + with auth.thycotic_secret_get(endpoint) as rs: + if rs.status_code == 200: + try: + key_text = rs.content.decode() + except: + key_text = '' + is_key = SSH_KEY_MIN_LENGTH < len(key_text) < SSH_KEY_MAX_LENGTH + if is_key: + item['itemValue'] = key_text + else: + if not isinstance(record.attachments, list): + record.attachments = [] + attachment = BytesAttachment(file_name, key_text.encode('utf-8')) + record.attachments.append(attachment) else: - if not isinstance(record.attachments, list): - record.attachments = [] - attachment = BytesAttachment(file_name, key_text.encode('utf-8')) - if not isinstance(record.attachments, list): - record.attachments = [] - record.attachments.append(attachment) + status, message = ThycoticAuth.response_error(rs) + skipped_fields.append({'ID': secret_id, 'Secret': record.title, 'Field': slug, + 'Status': status, 'Message': message}) + except requests.exceptions.RequestException as e: + skipped_fields.append({'ID': secret_id, 'Secret': record.title, 'Field': slug, + 'Status': 'N/A', 'Message': str(e)}) else: if not isinstance(record.attachments, list): record.attachments = [] attachment = ThycoticAttachment(auth, endpoint, file_name) - if not isinstance(record.attachments, list): - record.attachments = [] record.attachments.append(attachment) del items[slug] @@ -1039,6 +1147,18 @@ def do_import(self, filename, **kwargs): yield record print(' Done', flush=True) + ThycoticImporter.print_skipped(skipped_secrets, skipped_fields) + + + @staticmethod + def print_skipped(skipped_secrets, skipped_fields): # type: (List[dict], List[dict]) -> None + """Print the secrets and fields that could not be read, like the CyberArk import does.""" + if skipped_secrets: + print(f'\nSkipped {len(skipped_secrets)} Secrets:') + print(tabulate(skipped_secrets, headers='keys', maxcolwidths=[None, None, 80]), end='\n\n') + if skipped_fields: + print(f'\nSkipped {len(skipped_fields)} Fields:') + print(tabulate(skipped_fields, headers='keys', maxcolwidths=[None, 40, None, None, 80]), end='\n\n') class ThycoticAuth: @@ -1067,8 +1187,7 @@ def authenticate(self, username, password, on_totp=None): # type: (str, str, Op verify=False, proxies=self.proxy) if rs.status_code != 200: if rs.status_code == 400: - error_rs = rs.json() - error = error_rs['error'] + error = self._json_message(rs, 'error') if 'totp' in error.lower() and callable(on_totp): code = on_totp() if code: @@ -1082,8 +1201,7 @@ def authenticate(self, username, password, on_totp=None): # type: (str, str, Op self.refresh_token = auth_rs.get('refresh_token') self.last_login = int(datetime.datetime.now().timestamp()) else: - error_rs = rs.json() - raise Exception(error_rs['error']) + raise Exception(self._error_message(rs, 'error')) def refresh_auth_token(self): if not self.refresh_token: @@ -1105,8 +1223,7 @@ def refresh_auth_token(self): self.refresh_token = auth_rs.get('refresh_token') self.last_login = int(datetime.datetime.now().timestamp()) else: - error_rs = rs.json() - raise Exception(error_rs['error']) + raise Exception(self._error_message(rs, 'error')) def ensure_auth_token(self): if self.access_token: @@ -1138,8 +1255,7 @@ def thycotic_search(self, endpoint): # type: (str) -> list time.sleep(10) rs = requests.get(url, headers=headers, verify=False, proxies=self.proxy) if rs.status_code != 200: - error_rs = rs.json() - raise Exception(error_rs['message']) + raise Exception(self._error_message(rs)) chunk_rs = rs.json() records = chunk_rs.get('records') if isinstance(records, list): @@ -1152,7 +1268,77 @@ def thycotic_search(self, endpoint): # type: (str) -> list break return result + @staticmethod + def _json_message(rs, key='message'): # type: (Optional[requests.Response], str) -> str + """Return the given field of a JSON error body, or '' if there is none.""" + if rs is None or not (getattr(rs, 'text', None) or '').strip(): + return '' + try: + payload = rs.json() + except ValueError: + return '' + if isinstance(payload, dict): + return str(payload.get(key) or '') + return '' + + @classmethod + def _error_message(cls, rs, key='message'): # type: (requests.Response, str) -> str + """Describe an error response, whether or not its body is JSON.""" + message = cls._json_message(rs, key) + if message: + return message + body = (rs.text or '').strip().replace('\n', ' ')[:300] + return f'HTTP {rs.status_code} {rs.reason or ""}: {body or ""}' + + def _log_thycotic_response(self, endpoint, rs, error): + if rs is None: + logging.info('"%s" error: %s (no response)', endpoint, error) + return + logging.info( + '"%s" error: %s (HTTP %s %s)', + endpoint, + error, + getattr(rs, 'status_code', ''), + getattr(rs, 'reason', '') or '') + + @classmethod + def response_error(cls, rs): # type: (requests.Response) -> Tuple[Any, str] + """Return (HTTP status, message) describing a failed response.""" + message = cls._json_message(rs) + if not message: + message = (rs.text or '').strip().replace('\n', ' ')[:300] or rs.reason or '' + return rs.status_code, message + + def thycotic_secret(self, endpoint): # type: (str) -> Tuple[Optional[dict], Optional[Tuple[Any, str]]] + """Read a secret, adding checkout parameters only if the plain read fails. + + Returns (secret, None) on success, or (None, (status, message)) describing + the last failed attempt. + """ + secret = None + first_error = None + try: + secret, first_error = self._read_entity(endpoint) + except Exception as e: + logging.info('"%s" error: %s', endpoint, str(e)) + first_error = ('N/A', str(e)) + if secret is not None: + return secret, None + checkout = with_secret_access_params(endpoint) + logging.info('Reading "%s" with checkout parameters', checkout) + try: + secret, error = self._read_entity(checkout) + if secret is not None: + return secret, None + return None, error or first_error + except Exception as e: + logging.info('"%s" error: %s', checkout, str(e)) + return None, ('N/A', str(e)) + def thycotic_entity(self, endpoint): # type: (str) -> Optional[dict] + return self._read_entity(endpoint)[0] + + def _read_entity(self, endpoint): # type: (str) -> Tuple[Optional[dict], Optional[Tuple[Any, str]]] self.ensure_auth_token() headers = { 'Accept': 'application/json', @@ -1168,15 +1354,15 @@ def thycotic_entity(self, endpoint): # type: (str) -> Optional[dict] except Exception as e: logging.warning('Another "%s" error: %s', endpoint, str(e)) time.sleep(10) - return + return None, ('N/A', str(e)) + if rs.status_code != 200: + self._log_thycotic_response(endpoint, rs, f'returned {rs.status_code}') + return None, self.response_error(rs) try: - if rs.status_code == 200: - return rs.json() - else: - error_rs = rs.json() - logging.info('"%s" returned %d: %s', endpoint, rs.status_code, error_rs.get('message', '')) - except Exception as e: - logging.info('"%s" error: %s', endpoint, str(e)) + return rs.json(), None + except ValueError as e: + self._log_thycotic_response(endpoint, rs, f'invalid JSON: {e}') + return None, (rs.status_code, 'Response is not valid JSON') def thycotic_get(self, endpoint): # type: (str) -> requests.Response self.ensure_auth_token() @@ -1193,6 +1379,22 @@ def thycotic_get(self, endpoint): # type: (str) -> requests.Response verify=False, proxies=self.proxy, stream=True) return rs + def thycotic_secret_get(self, endpoint): # type: (str) -> requests.Response + """Read a secret field, adding checkout parameters only if the plain read fails. + + This handles restricted secrets that require checkout parameters. + """ + try: + rs = self.thycotic_get(endpoint) + if rs.status_code == 200: + return rs + rs.close() + except Exception as e: + logging.info('"%s" error: %s', endpoint, str(e)) + checkout = with_secret_access_params(endpoint) + logging.info('Reading "%s" with checkout parameters', checkout) + return self.thycotic_get(checkout) + class ThycoticAttachment(Attachment): def __init__(self, auth, endpoint, name): # type: (ThycoticAuth, str, str) -> None @@ -1208,14 +1410,23 @@ def open(self): self._file_content = b'' def prepare(self): - with self.auth.thycotic_get(self.endpoint) as rs: - if rs.status_code == 200: - length = rs.headers.get('Content-Length', 0) - try: - self.size = int(length) - except: - pass - self._file_content = rs.content + # Runs during upload, after the import summary is printed, so failures + # are reported here as warnings instead of in the skipped fields table. + try: + with self.auth.thycotic_secret_get(self.endpoint) as rs: + if rs.status_code == 200: + length = rs.headers.get('Content-Length', 0) + try: + self.size = int(length) + except: + pass + self._file_content = rs.content + else: + status, message = ThycoticAuth.response_error(rs) + logging.error('Attachment "%s" (%s) was not imported: HTTP %s %s', + self.name, self.endpoint, status, message) + except requests.exceptions.RequestException as e: + logging.error('Attachment "%s" (%s) was not imported: %s', self.name, self.endpoint, str(e)) class ThycoticMembershipDownload(BaseDownloadMembership, ThycoticMixin): diff --git a/unit-tests/test_thycotic_interactive_folder_picker.py b/unit-tests/test_thycotic_interactive_folder_picker.py index adf0f867c..3c44e96f9 100644 --- a/unit-tests/test_thycotic_interactive_folder_picker.py +++ b/unit-tests/test_thycotic_interactive_folder_picker.py @@ -18,7 +18,10 @@ def _secret(secret_id, folder_id): def _lookup(folder_id): - return f'/v1/secrets/lookup?filter.folderId={folder_id}&filter.includeSubFolders=true' + return f'/v1/secrets/lookup?filter.folderId={folder_id}&filter.includeSubFolders=true&filter.includeRestricted=true' + + +TENANT_LOOKUP = '/v1/secrets/lookup?filter.includeRestricted=true' class ParseChoiceTests(unittest.TestCase): @@ -267,6 +270,10 @@ def search(endpoint): def entity(endpoint): return secrets_by_id[int(endpoint.rstrip('/').rsplit('/', 1)[-1])] + def secret(endpoint): + secret_data = entity(endpoint) + return (secret_data, None) if secret_data else (None, ('N/A', 'Not found')) + input_patch = mock.patch('builtins.input', side_effect=input_error, return_value=None if input_error is not None else choice) with mock.patch('keepercommander.importer.thycotic.thycotic.ThycoticAuth') as auth_cls, \ @@ -278,6 +285,7 @@ def entity(endpoint): auth = auth_cls.return_value auth.thycotic_search.side_effect = search auth.thycotic_entity.side_effect = entity + auth.thycotic_secret.side_effect = secret kwargs = {} if filter_folder is None else {'filter_folder': filter_folder} try: records = list(ThycoticImporter().do_import('user@thycotic.example', **kwargs)) @@ -320,7 +328,7 @@ def test_imports_all_and_drops_personal(self): with self.subTest(label): records, mocked_input = self._import(secrets=secrets, **kwargs) self.assertEqual(mocked_input.called, prompted) - self.assertEqual(self.last_endpoints, ['/v1/folders', '/v1/secrets/lookup']) + self.assertEqual(self.last_endpoints, ['/v1/folders', TENANT_LOOKUP]) self.assertEqual(self._paths(records), expected) def test_no_folders_imports_all_without_prompting(self): @@ -329,7 +337,7 @@ def test_no_folders_imports_all_without_prompting(self): records, mocked_input = self._import('1', [_secret(501, 117)], raw_folders=raw_folders if raw_folders is not None else []) mocked_input.assert_not_called() - self.assertEqual(self.last_endpoints, ['/v1/folders', '/v1/secrets/lookup']) + self.assertEqual(self.last_endpoints, ['/v1/folders', TENANT_LOOKUP]) self.assertEqual(self._paths(records), {'secret-501': None}) def test_non_list_secret_lookup_imports_nothing(self): @@ -367,10 +375,10 @@ def _endpoints(self): return [call.args[0] for call in self.auth.thycotic_search.call_args_list] def test_queries_each_folder_and_removes_duplicates(self): - self.auth.thycotic_search.side_effect = [[{'id': 1}, {'id': 2}], [{'id': 2}, {'id': 3}], [{'id': 3}]] + self.auth.thycotic_search.side_effect = [[{'id': 1}, {'id': 2}], [{'id': 2}, {'id': 3}]] self.assertEqual(ThycoticMixin.get_secret_ids(self.auth, [103, 116, 116], 'filter.includeSubFolders=true'), [1, 2, 3]) - self.assertEqual(self._endpoints(), [_lookup(103), _lookup(116), _lookup(116)]) + self.assertEqual(self._endpoints(), [_lookup(103), _lookup(116)]) def test_no_usable_folder_ids_make_no_calls(self): for folder_ids in ([], None, (), [None]): @@ -378,15 +386,10 @@ def test_no_usable_folder_ids_make_no_calls(self): self.assertEqual(ThycoticMixin.get_secret_ids(self.auth, folder_ids, 'q'), []) self.auth.thycotic_search.assert_not_called() - def test_query_encoding(self): - for folder_ids, query, expected in (([5], '', '/v1/secrets/lookup?filter.folderId=5'), - ([None, 5], None, '/v1/secrets/lookup?filter.folderId=5'), - ([5], 'a=1&b=x y', '/v1/secrets/lookup?filter.folderId=5&a=1&b=x+y')): - with self.subTest(query=query): - self.auth.reset_mock() - self.auth.thycotic_search.return_value = [] - ThycoticMixin.get_secret_ids(self.auth, folder_ids, query) - self.auth.thycotic_search.assert_called_once_with(expected) + def test_lookup_uses_restricted_and_subfolder_filters(self): + self.auth.thycotic_search.return_value = [] + ThycoticMixin.get_secret_ids(self.auth, [5], '') + self.auth.thycotic_search.assert_called_once_with(_lookup(5)) def test_non_list_response_and_malformed_secrets_are_ignored(self): self.auth.thycotic_search.side_effect = [None, {'error': 'bad'}, 'text', [None, {}, {'id': None}, {'id': 7}]] @@ -399,4 +402,4 @@ def test_search_error_propagates(self): if __name__ == '__main__': - unittest.main() + unittest.main() \ No newline at end of file diff --git a/unit-tests/test_thycotic_migration.py b/unit-tests/test_thycotic_migration.py new file mode 100644 index 000000000..dc612030c --- /dev/null +++ b/unit-tests/test_thycotic_migration.py @@ -0,0 +1,374 @@ +import contextlib +import io +import unittest +from unittest import mock + +import requests + +from keepercommander.importer import commands +from keepercommander.importer.thycotic.thycotic import ( + ThycoticAttachment, + ThycoticAuth, + ThycoticImporter, + build_secrets_lookup_query, + lookup_secret_ids, + with_secret_access_params, +) + +VISIBLE_ID = 7 +RESTRICTED_ID = 4242 +SHARED_ID = 99 +CHECKOUT_QUERY = 'autoCheckout=true&autoCheckIn=true&autoComment=Checking%20out' +REQUESTS_GET = 'keepercommander.importer.thycotic.thycotic.requests.get' +SLEEP = 'keepercommander.importer.thycotic.thycotic.time.sleep' + + +def _search(query): + rows = [{'id': VISIBLE_ID, 'name': 'visible'}] + if 'filter.includeRestricted=true' in query: + rows.append({'id': RESTRICTED_ID, 'name': 'restricted'}) + return rows + + +def _response(status_code, text='', json_value=None, reason=''): + rs = mock.Mock(spec=requests.Response) + rs.status_code = status_code + rs.text = text + rs.reason = reason + if json_value is None: + rs.json.side_effect = ValueError('Expecting value') + else: + rs.json.return_value = json_value + return rs + + +def _auth(): + auth = ThycoticAuth('https://ss.example.com/SecretServer') + auth.access_token = 'token' + auth.expires_in = 3600 + auth.last_login = 2 ** 40 + return auth + + +class TestRestrictedLookup(unittest.TestCase): + def test_parameter_is_on_every_lookup(self): + tenant = build_secrets_lookup_query() + folder = build_secrets_lookup_query(15) + self.assertEqual(tenant, '/v1/secrets/lookup?filter.includeRestricted=true') + self.assertEqual( + folder, + '/v1/secrets/lookup?filter.folderId=15&filter.includeSubFolders=true&filter.includeRestricted=true', + ) + + def test_restricted_ids_appear_only_when_enabled(self): + self.assertEqual(lookup_secret_ids(_search), [VISIBLE_ID, RESTRICTED_ID]) + self.assertEqual(lookup_secret_ids(_search, include_restricted=False), [VISIBLE_ID]) + self.assertIn(RESTRICTED_ID, lookup_secret_ids(_search, folder_ids=[15])) + self.assertNotIn(RESTRICTED_ID, lookup_secret_ids(_search, folder_ids=[15], include_restricted=False)) + + def test_include_restricted_flag_is_not_on_the_command(self): + flags = [ + option + for action in commands.import_parser._actions + for option in action.option_strings + ] + self.assertNotIn('--include-restricted', flags) + self.assertNotIn('--report', flags) + + +class TestLookupDeduplication(unittest.TestCase): + def test_secret_in_overlapping_folders_is_returned_once(self): + # Folder 20 is a subfolder of 10; both match the folder filter, and + # includeSubFolders=true makes folder 10 return folder 20's secrets too. + by_folder = { + '10': [{'id': VISIBLE_ID}, {'id': SHARED_ID}], + '20': [{'id': SHARED_ID}, {'id': RESTRICTED_ID}], + } + + def search(query): + folder_id = query.split('filter.folderId=')[1].split('&')[0] + return by_folder[folder_id] + + ids = lookup_secret_ids(search, folder_ids=[10, 20]) + self.assertEqual(ids, [VISIBLE_ID, SHARED_ID, RESTRICTED_ID]) + + def test_same_folder_listed_twice_is_returned_once(self): + ids = lookup_secret_ids(_search, folder_ids=[15, 15]) + self.assertEqual(ids, [VISIBLE_ID, RESTRICTED_ID]) + + +class TestSecretAccessParams(unittest.TestCase): + def test_secret_read_sends_checkout_params(self): + self.assertEqual(with_secret_access_params('/v1/secrets/42'), f'/v1/secrets/42?{CHECKOUT_QUERY}') + self.assertEqual( + with_secret_access_params('/v1/secrets/42/fields/private-key'), + f'/v1/secrets/42/fields/private-key?{CHECKOUT_QUERY}', + ) + + def test_space_is_percent_encoded(self): + endpoint = with_secret_access_params('/v1/secrets/42') + self.assertIn('autoComment=Checking%20out', endpoint) + self.assertNotIn('+', endpoint) + + def test_existing_query_params_are_kept(self): + endpoint = with_secret_access_params('/v1/secrets/42?includeInactive=true') + self.assertEqual(endpoint, f'/v1/secrets/42?includeInactive=true&{CHECKOUT_QUERY}') + + def test_existing_checkout_params_are_replaced_not_duplicated(self): + endpoint = with_secret_access_params('/v1/secrets/42?autoCheckout=false&autoComment=old') + self.assertEqual(endpoint.count('autoCheckout='), 1) + self.assertEqual(endpoint.count('autoComment='), 1) + self.assertIn('autoCheckout=true', endpoint) + self.assertNotIn('autoComment=old', endpoint) + + def test_round_trip_is_stable(self): + once = with_secret_access_params('/v1/secrets/42') + self.assertEqual(with_secret_access_params(once), once) + + +class TestThycoticSecret(unittest.TestCase): + def setUp(self): + sleep = mock.patch(SLEEP) + sleep.start() + self.addCleanup(sleep.stop) + + def test_plain_read_succeeds_without_checkout(self): + with mock.patch(REQUESTS_GET, return_value=_response(200, '{"id": 42}', {'id': 42})) as get: + secret, error = _auth().thycotic_secret('/v1/secrets/42') + self.assertEqual(secret, {'id': 42}) + self.assertIsNone(error) + self.assertEqual(get.call_count, 1) + self.assertTrue(get.call_args[0][0].endswith('/api/v1/secrets/42')) + self.assertNotIn('autoCheckout', get.call_args[0][0]) + + def test_checkout_params_added_only_after_plain_read_fails(self): + denied = _response(400, '{"message": "Checkout required"}', {'message': 'Checkout required'}) + ok = _response(200, '{"id": 42}', {'id': 42}) + with mock.patch(REQUESTS_GET, side_effect=[denied, ok]) as get: + secret, error = _auth().thycotic_secret('/v1/secrets/42') + self.assertEqual(secret, {'id': 42}) + self.assertIsNone(error) + first_url = get.call_args_list[0][0][0] + retry_url = get.call_args_list[1][0][0] + self.assertNotIn('autoCheckout', first_url) + self.assertTrue(retry_url.endswith(f'/api/v1/secrets/42?{CHECKOUT_QUERY}')) + + def test_checkout_retry_keeps_existing_query_params(self): + denied = _response(400, '{"message": "Comment required"}', {'message': 'Comment required'}) + ok = _response(200, '{"id": 42}', {'id': 42}) + with mock.patch(REQUESTS_GET, side_effect=[denied, ok]) as get: + _auth().thycotic_secret('/v1/secrets/42?includeInactive=true') + retry_url = get.call_args_list[1][0][0] + self.assertTrue(retry_url.endswith(f'/api/v1/secrets/42?includeInactive=true&{CHECKOUT_QUERY}')) + + def test_returns_error_when_both_reads_fail(self): + denied = _response(403, '{"message": "Access denied"}', {'message': 'Access denied'}) + with mock.patch(REQUESTS_GET, return_value=denied) as get: + secret, error = _auth().thycotic_secret('/v1/secrets/42') + self.assertIsNone(secret) + self.assertEqual(error, (403, 'Access denied')) + self.assertEqual(get.call_count, 2) + + def test_error_uses_body_when_response_is_not_json(self): + html = _response(502, 'Bad Gateway', reason='Bad Gateway') + with mock.patch(REQUESTS_GET, return_value=html): + secret, error = _auth().thycotic_secret('/v1/secrets/42') + self.assertIsNone(secret) + self.assertEqual(error, (502, 'Bad Gateway')) + + def test_error_reports_network_failure(self): + with mock.patch(REQUESTS_GET, side_effect=requests.exceptions.ConnectionError('refused')): + secret, error = _auth().thycotic_secret('/v1/secrets/42') + self.assertIsNone(secret) + self.assertEqual(error, ('N/A', 'refused')) + + def test_error_reports_exception_on_checkout_read(self): + auth = _auth() + with mock.patch.object(auth, 'ensure_auth_token', side_effect=Exception('refresh failed')): + secret, error = auth.thycotic_secret('/v1/secrets/42') + self.assertIsNone(secret) + self.assertEqual(error, ('N/A', 'refresh failed')) + + def test_checkout_retry_runs_when_plain_read_raises(self): + auth = _auth() + ok = _response(200, '{"id": 42}', {'id': 42}) + with mock.patch.object(auth, 'ensure_auth_token', side_effect=[Exception('refresh failed'), None]), \ + mock.patch(REQUESTS_GET, return_value=ok) as get: + secret, _ = auth.thycotic_secret('/v1/secrets/42') + self.assertEqual(secret, {'id': 42}) + self.assertEqual(get.call_count, 1) + self.assertIn('autoCheckout=true', get.call_args[0][0]) + + +class TestThycoticSecretGet(unittest.TestCase): + def test_plain_field_read_succeeds_without_checkout(self): + ok = _response(200, 'key') + with mock.patch(REQUESTS_GET, return_value=ok) as get: + rs = _auth().thycotic_secret_get('/v1/secrets/42/fields/private-key') + self.assertIs(rs, ok) + self.assertEqual(get.call_count, 1) + self.assertNotIn('autoCheckout', get.call_args[0][0]) + + def test_field_read_retries_with_checkout_and_closes_failed_response(self): + denied = _response(400, '{"message": "Checkout required"}', {'message': 'Checkout required'}) + ok = _response(200, 'key') + with mock.patch(REQUESTS_GET, side_effect=[denied, ok]) as get: + rs = _auth().thycotic_secret_get('/v1/secrets/42/fields/private-key') + self.assertIs(rs, ok) + denied.close.assert_called_once() + retry_url = get.call_args_list[1][0][0] + self.assertTrue(retry_url.endswith(f'/api/v1/secrets/42/fields/private-key?{CHECKOUT_QUERY}')) + + def test_field_read_retries_with_checkout_after_exception(self): + ok = _response(200, 'key') + with mock.patch(SLEEP), mock.patch(REQUESTS_GET, side_effect=[ + requests.exceptions.ConnectionError('reset'), requests.exceptions.ConnectionError('reset'), ok]) as get: + rs = _auth().thycotic_secret_get('/v1/secrets/42/fields/private-key') + self.assertIs(rs, ok) + self.assertIn('autoCheckout=true', get.call_args_list[2][0][0]) + + +class TestThycoticEntity(unittest.TestCase): + def setUp(self): + sleep = mock.patch(SLEEP) + sleep.start() + self.addCleanup(sleep.stop) + + def test_entity_never_adds_checkout_params(self): + denied = _response(400, '{"message": "Bad request"}', {'message': 'Bad request'}) + with mock.patch(REQUESTS_GET, return_value=denied) as get: + self.assertIsNone(_auth().thycotic_entity('/v1/secret-templates/5/export')) + self.assertEqual(get.call_count, 1) + + def test_network_error_returns_none(self): + error = requests.exceptions.ConnectionError('connection refused') + with mock.patch(REQUESTS_GET, side_effect=error) as get: + secret = _auth().thycotic_entity('/v1/secrets/42') + self.assertIsNone(secret) + self.assertEqual(get.call_count, 2) + + def test_network_error_recovers_on_retry(self): + error = requests.exceptions.ConnectionError('connection reset') + ok = _response(200, '{"id": 42}', {'id': 42}) + with mock.patch(REQUESTS_GET, side_effect=[error, ok]): + secret = _auth().thycotic_entity('/v1/secrets/42') + self.assertEqual(secret, {'id': 42}) + + def test_non_json_error_response_does_not_crash(self): + html = _response(502, 'Bad Gateway', reason='Bad Gateway') + with mock.patch(REQUESTS_GET, return_value=html): + self.assertIsNone(_auth().thycotic_entity('/v1/secrets/42')) + + def test_non_json_success_response_does_not_crash(self): + with mock.patch(REQUESTS_GET, return_value=_response(200, 'login')): + self.assertIsNone(_auth().thycotic_entity('/v1/secrets/42')) + + def test_json_error_that_is_not_an_object_does_not_crash(self): + with mock.patch(REQUESTS_GET, return_value=_response(500, '["boom"]', ['boom'])): + self.assertIsNone(_auth().thycotic_entity('/v1/secrets/42')) + + +class TestThycoticAttachment(unittest.TestCase): + def test_successful_download_sets_content(self): + ok = _response(200, 'data') + ok.headers = {'Content-Length': '4'} + ok.content = b'data' + ok.__enter__ = mock.Mock(return_value=ok) + ok.__exit__ = mock.Mock(return_value=False) + auth = _auth() + with mock.patch.object(auth, 'thycotic_secret_get', return_value=ok): + attachment = ThycoticAttachment(auth, '/v1/secrets/42/fields/cert', 'cert.pem') + attachment.prepare() + self.assertEqual(attachment.size, 4) + + def test_failed_download_warns_and_leaves_size_unset(self): + denied = _response(403, '{"message": "Access denied"}', {'message': 'Access denied'}) + denied.__enter__ = mock.Mock(return_value=denied) + denied.__exit__ = mock.Mock(return_value=False) + auth = _auth() + with mock.patch.object(auth, 'thycotic_secret_get', return_value=denied), \ + self.assertLogs(level='WARNING') as logs: + attachment = ThycoticAttachment(auth, '/v1/secrets/42/fields/cert', 'cert.pem') + attachment.prepare() + self.assertIsNone(attachment.size) + self.assertIn('cert.pem', logs.output[0]) + self.assertIn('403', logs.output[0]) + self.assertIn('Access denied', logs.output[0]) + + def test_network_error_warns_instead_of_raising(self): + auth = _auth() + with mock.patch.object(auth, 'thycotic_secret_get', + side_effect=requests.exceptions.ConnectionError('reset')), \ + self.assertLogs(level='WARNING') as logs: + ThycoticAttachment(auth, '/v1/secrets/42/fields/cert', 'cert.pem').prepare() + self.assertIn('reset', logs.output[0]) + + +class TestPrintSkipped(unittest.TestCase): + def _output(self, secrets, fields): + out = io.StringIO() + with contextlib.redirect_stdout(out): + ThycoticImporter.print_skipped(secrets, fields) + return out.getvalue() + + def test_nothing_printed_when_nothing_skipped(self): + self.assertEqual(self._output([], []), '') + + def test_skipped_secrets_table(self): + output = self._output([ + {'ID': 42, 'Status': 403, 'Message': 'Access denied'}, + {'ID': 108, 'Status': 'N/A', 'Message': 'Read timed out'}, + ], []) + self.assertIn('Skipped 2 Secrets:', output) + for text in ('ID', 'Status', 'Message', '42', '403', 'Access denied', '108', 'Read timed out'): + self.assertIn(text, output) + self.assertNotIn('Fields', output) + + def test_skipped_fields_table(self): + output = self._output([], [ + {'ID': 77, 'Secret': 'DB Server', 'Field': 'private-key', 'Status': 400, 'Message': 'Checkout required'}, + ]) + self.assertIn('Skipped 1 Fields:', output) + for text in ('Secret', 'Field', 'DB Server', 'private-key', 'Checkout required'): + self.assertIn(text, output) + self.assertNotIn('Secrets:', output) + + +class TestErrorResponses(unittest.TestCase): + def test_log_response_handles_none(self): + with self.assertLogs(level='INFO') as logs: + _auth()._log_thycotic_response('/v1/secrets/42', None, 'boom') + self.assertIn('no response', logs.output[0]) + + def test_search_non_json_error_raises_readable_message(self): + html = _response(503, 'Service Unavailable', reason='Service Unavailable') + with mock.patch(REQUESTS_GET, return_value=html): + with self.assertRaises(Exception) as ctx: + _auth().thycotic_search('/v1/secrets/lookup') + self.assertIn('HTTP 503', str(ctx.exception)) + self.assertNotIsInstance(ctx.exception, ValueError) + + def test_search_json_error_uses_message(self): + denied = _response(403, '{"message": "Access denied"}', {'message': 'Access denied'}) + with mock.patch(REQUESTS_GET, return_value=denied): + with self.assertRaises(Exception) as ctx: + _auth().thycotic_search('/v1/secrets/lookup') + self.assertEqual(str(ctx.exception), 'Access denied') + + def test_refresh_non_json_error_raises_readable_message(self): + auth = _auth() + auth.refresh_token = 'refresh' + html = _response(500, 'Server Error', reason='Internal Server Error') + with mock.patch('keepercommander.importer.thycotic.thycotic.requests.post', return_value=html): + with self.assertRaises(Exception) as ctx: + auth.refresh_auth_token() + self.assertIn('HTTP 500', str(ctx.exception)) + + +class TestThycoticTerminalSummary(unittest.TestCase): + def test_uses_cyberark_finish_summary(self): + self.assertTrue(ThycoticImporter.verbose_import_summary) + + +if __name__ == '__main__': + unittest.main() From 299e1338c8fa54e5e15f1a1b1310214bae4426b0 Mon Sep 17 00:00:00 2001 From: stripathi-ks Date: Thu, 29 Oct 2026 18:11:29 +0530 Subject: [PATCH 4/6] Updated thycotic importer to add support for secrets level permission (#2463) * Updated thycotic importer to add support for secrets level permission inside keeper * MM-28 Address review feedback on secret-level permissions - Normalize Delinea/Keeper folder paths (both separators) for record matching - Index Keeper records once in locate_secret_share_records - Use url_from_tenant when parsing user@host import targets - Wrap per-user detail fetch in try/except - Direct emailAddress/emailaddress lookup; dict lookup for existing users - Replace print with logging Co-Authored-By: Claude Opus 5.5 * MM-28 Cache folder permissions and skip secrets whose fetch fails - Cache folder-permission rows per folder so secrets in the same folder do not refetch them (failed fetches are not cached) - Log and skip a secret whose permission fetch fails instead of aborting the whole load; report the skipped count at the end Co-Authored-By: Claude Opus 5.5 * MM-28 Add tests for failures, invalid data and edge cases - External call failures: secret, lookup, secret/folder permissions, user detail, Keeper user load and create - _create_enterprise_users outcomes: existing, duplicate, invalid, failed, name fallback - Invalid permission rows, email extraction, folder walk cycles, path normalization and tenant URL parsing Co-Authored-By: Claude Opus 5.5 * MM-28 Handle share records in multiple folders; fix user report - Apply the --folder prefix to every folder of a share record, matching the main import (an empty path becomes the prefix) - locate_secret_share_records looks the record up in each of its folders - Membership dedupe key uses all folder paths, not just the first - Remove a stale user-creation loop left in _print_thycotic_user_report by the mm-5-phase-one merge (NameError on any non-empty report) * MM-28 Warn about secret roles that cannot be mapped to Keeper Custom Secret Server roles are still skipped rather than guessed. Each affected secret now logs the role names so the admin can share those users manually, with a total at the end of the load. * MM-28 Retry on rate limits, handle non-JSON errors, split iter_secret_shares - Secret Server GETs retry on 429 up to 3 times, waiting the larger of Retry-After (capped at 300s) and 5s exponential backoff, same pattern as the Keeper API throttle retry - Error responses that are not JSON (proxy or HTML pages) now raise or log "HTTP " instead of a JSON decode error, in login, token refresh, search and entity fetch - iter_secret_shares: move custom-role warning, folder path lookup and record building into helpers; skip unnamed secrets before loading groups; correct the "no permission lists" warning text - Document how group permissions are flattened into per-user shares * MM-28 Load secret permissions in bulk and with a bounded worker pool - One paged GET /v1/secret-permissions (no secretId filter) grouped by secretId replaces a request per secret; falls back to per-secret requests if the bulk call fails or returns nothing - Secret reads run on a small worker pool (download-membership --workers, default 4, 1-16; 1 runs on the calling thread). At most workers * 2 requests are queued, results keep lookup order, and stopping early cancels queued work - Group lookup and record building stay on the calling thread - ensure_auth_token is locked so parallel requests refresh an expired token once instead of reusing a rotated refresh token * MM-28 Lock the shared folder cache, un-nest helpers, log skipped records - FolderPermissionCache: folder permission rows shared by the worker threads are read and written under a lock; first write wins and failed fetches are still not cached - Replace nested functions with methods: _secret_permission_rows, _fetch_secret_entry (worker task), _user_outcome, and module-level _share_record_key in commands - locate_secret_share_records logs a record listed more than once under the same path, and counts records that could not be read --------- Co-authored-by: Shashwat Tripathi --- keepercommander/importer/commands.py | 49 +- keepercommander/importer/imp_exp.py | 184 ++- keepercommander/importer/importer.py | 21 + keepercommander/importer/json/json.py | 26 +- keepercommander/importer/thycotic/thycotic.py | 675 +++++++- unit-tests/test_thycotic_access.py | 1411 +++++++++++++++++ 6 files changed, 2311 insertions(+), 55 deletions(-) create mode 100644 unit-tests/test_thycotic_access.py diff --git a/keepercommander/importer/commands.py b/keepercommander/importer/commands.py index 062d400f4..d065c8776 100644 --- a/keepercommander/importer/commands.py +++ b/keepercommander/importer/commands.py @@ -19,7 +19,7 @@ from . import imp_exp from .. import api, record_types -from .importer import SharedFolder, Team, Permission, PathDelimiter, replace_email_domain, BaseDownloadMembership, BaseDownloadRecordType, RecordType +from .importer import SharedFolder, Team, Permission, Record, PathDelimiter, normalize_folder_path, replace_email_domain, BaseDownloadMembership, BaseDownloadRecordType, RecordType from .json.json import KeeperJsonImporter, KeeperJsonExporter from ..commands.base import dump_report_data, raise_parse_exception, suppress_exit, user_choice, Command from ..commands.enterprise_common import EnterpriseCommand @@ -64,6 +64,16 @@ def _cyberark_skip_arg(value): +def _workers_arg(value): + try: + workers = int(value) + except (TypeError, ValueError): + raise argparse.ArgumentTypeError('must be an integer') + if not 1 <= workers <= 16: + raise argparse.ArgumentTypeError('--workers must be between 1 and 16') + return workers + + def _positive_int_arg(value): try: depth = int(value) @@ -174,6 +184,9 @@ def _positive_int_arg(value): download_membership_parser.add_argument('name', type=str, nargs='?', help='Output file name. "shared_folder_membership.json" if omitted.') download_membership_parser.add_argument('--timeout', dest='timeout', action='store', type=int, default=10, help='Timeout in seconds for network requests. Currently, supported for cyberark. Default: 10') download_membership_parser.add_argument('--api-page-size-limit', dest='api_page_size_limit', action='store', type=int, default=200, help='Page size limit for API requests. Currently, supported for cyberark. Default: 200') +download_membership_parser.add_argument('--workers', dest='workers', action='store', type=_workers_arg, default=None, + help='Parallel requests when loading secret permissions (1-16). ' + 'Currently, supported for thycotic. Default: 4') download_membership_parser.error = raise_parse_exception download_membership_parser.exit = suppress_exit @@ -463,6 +476,12 @@ def set_permission(perm, user, permit, restrict, perm_name): setattr(perm, perm_name, p) +def _share_record_key(record): + """Identity of a share record in the membership file: all its folder paths plus title.""" + paths = tuple(sorted({normalize_folder_path(folder.path) for folder in record.folders or []})) + return (paths or ('',), (record.title or '').casefold()) + + class DownloadMembershipCommand(Command): def get_parser(self): # type: () -> Optional[argparse.ArgumentParser] return download_membership_parser @@ -498,6 +517,7 @@ def execute(self, params, **kwargs): # type: (KeeperParams, **any) -> any added_folders = [] # type: List[SharedFolder] added_teams = [] # type: List[Team] + added_records = [] # type: List[Record] try: full_name = f'keepercommander.importer.{"json" if source == "keeper" else source}' @@ -511,7 +531,7 @@ def execute(self, params, **kwargs): # type: (KeeperParams, **any) -> any logging.warning('Error loading membership plugin: %s', source) return - download_membership = plugin.download_membership(params, folders_only=folders_only,timeout=kwargs.get('timeout'), api_page_size_limit=kwargs.get('api_page_size_limit')) + download_membership = plugin.download_membership(params, folders_only=folders_only,timeout=kwargs.get('timeout'), api_page_size_limit=kwargs.get('api_page_size_limit'), workers=kwargs.get('workers')) for obj in download_membership: if isinstance(obj, SharedFolder): @@ -538,6 +558,14 @@ def execute(self, params, **kwargs): # type: (KeeperParams, **any) -> any obj.members = [replace_email_domain(x, old_domain, new_domain) for x in obj.members] if obj.name: added_teams.append(obj) + elif isinstance(obj, Record) and obj.shares: + if old_domain and new_domain: + for share in obj.shares: + share.name = replace_email_domain(share.name, old_domain, new_domain) + if import_into: + for folder in obj.folders or []: + folder.path = PathDelimiter.join([import_into, folder.path]) if folder.path else import_into + added_records.append(obj) # process shared sub folders sub_folder_action = kwargs.get('sub_folder') or 'ignore' @@ -565,6 +593,7 @@ def execute(self, params, **kwargs): # type: (KeeperParams, **any) -> any shared_folders = {} # type: Dict[str, SharedFolder] teams = {} # type: Dict[str, Team] + existing_records = [] # type: List[Record] json_importer = KeeperJsonImporter() if os.path.exists(file_name): @@ -576,28 +605,36 @@ def execute(self, params, **kwargs): # type: (KeeperParams, **any) -> any elif isinstance(obj, Team): if obj.uid: teams[obj.uid] = obj + elif isinstance(obj, Record) and obj.shares: + existing_records.append(obj) except: pass - if added_folders or added_teams: + if added_folders or added_teams or added_records: for sf in added_folders: if sf.uid and sf.uid in shared_folders: del shared_folders[sf.uid] for t in added_teams: if t.uid and t.uid in teams: del teams[t.uid] + replaced = {_share_record_key(record) for record in added_records} + existing_records = [record for record in existing_records if _share_record_key(record) not in replaced] memberships = [] memberships.extend(shared_folders.values()) memberships.extend(teams.values()) + memberships.extend(existing_records) memberships.extend(added_folders) memberships.extend(added_teams) + memberships.extend(added_records) json_exporter = KeeperJsonExporter() json_exporter.do_export(file_name, memberships) if len(added_folders) > 0: logging.info('%d shared folder memberships added.', len(added_folders)) if len(added_teams) > 0: logging.info('%d team memberships added.', len(added_teams)) + if len(added_records) > 0: + logging.info('%d secret permission lists added.', len(added_records)) else: logging.info('No folder memberships downloaded.') @@ -614,6 +651,7 @@ def execute(self, params, **kwargs): shared_folders = [] # type: List[SharedFolder] teams = [] # type: List[Team] + secret_records = [] # type: List[Record] json_importer = KeeperJsonImporter() for obj in json_importer.do_import(file_name, users_only=True): @@ -621,6 +659,8 @@ def execute(self, params, **kwargs): shared_folders.append(obj) if isinstance(obj, Team): teams.append(obj) + if isinstance(obj, Record) and obj.shares: + secret_records.append(obj) full_sync = kwargs.get('full_sync') is True unsafe = kwargs.get('unsafe') is True @@ -630,6 +670,9 @@ def execute(self, params, **kwargs): if len(teams) > 0: imp_exp.import_teams(params, teams, full_sync) + if len(secret_records) > 0: + imp_exp.apply_secret_share_records(params, secret_records) + class DownloadRecordTypeCommand(EnterpriseCommand): def get_parser(self): diff --git a/keepercommander/importer/imp_exp.py b/keepercommander/importer/imp_exp.py index 7367e5219..c54eda32c 100644 --- a/keepercommander/importer/imp_exp.py +++ b/keepercommander/importer/imp_exp.py @@ -34,7 +34,7 @@ from urllib.parse import urlparse, parse_qs from .encryption_reader import EncryptionReader -from .importer import (importer_for_format, exporter_for_format, path_components, PathDelimiter, BaseExporter, +from .importer import (importer_for_format, exporter_for_format, path_components, PathDelimiter, normalize_folder_path, BaseExporter, BaseImporter, Record as ImportRecord, RecordField as ImportRecordField, Folder as ImportFolder, SharedFolder as ImportSharedFolder, Permission as ImportPermission, BytesAttachment, Attachment as ImportAttachment, RecordSchemaField, File as ImportFile, Team as ImportTeam, @@ -1490,6 +1490,7 @@ def _import(params, file_format, filename, **kwargs): sfu_rqs = None sync_down.sync_down(params) + grant_imported_record_shares(params, records) # upload attachments v2_atts = [] @@ -3000,6 +3001,187 @@ def prepare_folder_permission(params, folders, full_sync, unsafe=False): return folder_permissions +def locate_secret_share_records(params, records): + # type: (KeeperParams, List[ImportRecord]) -> List[ImportRecord] + """Find Keeper records for secret-level shares by folder path and title. + + Record JSON is parsed once while the index is built. Each secret then + looks up (normalized folder path, title) instead of parsing again. + """ + record_cache = getattr(params, 'record_cache', None) or {} + folder_records = getattr(params, 'subfolder_record_cache', None) or {} + record_index = {} + unreadable = 0 + for folder_uid, record_uids in folder_records.items(): + folder_path = normalize_folder_path(get_folder_path(params, folder_uid)) + for record_uid in record_uids or []: + cached = record_cache.get(record_uid) + if not isinstance(cached, dict): + unreadable += 1 + continue + raw = cached.get('data_unencrypted') or '{}' + if isinstance(raw, bytes): + raw = raw.decode('utf-8', errors='replace') + try: + title = (json.loads(raw).get('title') or '').strip().casefold() + except Exception: + unreadable += 1 + continue + bucket = record_index.setdefault((folder_path, title), []) + if record_uid in bucket: + logging.info('Record %s is listed more than once under "%s"; it is matched once', + record_uid, folder_path or '(root)') + continue + bucket.append(record_uid) + if unreadable: + logging.warning('%d Keeper record(s) could not be read and were not matched to secrets', unreadable) + + located = [] + for rec in records: + if not getattr(rec, 'shares', None): + continue + title = (rec.title or '').strip().casefold() + folder_paths = list(dict.fromkeys(normalize_folder_path(folder.path) for folder in rec.folders or [])) or [''] + folder_path = ', '.join(folder_paths) + found = list(dict.fromkeys( + record_uid for path in folder_paths for record_uid in record_index.get((path, title), []))) + if not found: + logging.warning('No Keeper record for secret "%s" in "%s"', rec.title, folder_path) + continue + if len(found) > 1: + logging.warning( + 'Secret "%s" matches %d records in "%s"; sharing each of them', + rec.title, len(found), folder_path) + for record_uid in found: + shared = rec if record_uid == found[0] else copy.copy(rec) + shared.uid = record_uid + located.append(shared) + return located + + +def existing_enterprise_emails(params): + # type: (KeeperParams) -> Optional[set] + """Load Keeper enterprise users before any secret share is granted. + + Returns None when the enterprise user list cannot be loaded. An empty + set means the list loaded and no users are present. + """ + try: + api.query_enterprise(params) + except Exception as exc: + logging.warning('Could not load Keeper users before secret shares: %s', exc) + return None + enterprise_data = getattr(params, 'enterprise', None) + if not isinstance(enterprise_data, dict): + logging.warning('Could not load Keeper users before secret shares') + return None + emails = set() + for user in enterprise_data.get('users') or []: + if not isinstance(user, dict): + continue + email = (user.get('email') or user.get('username') or '').strip() + if email: + emails.add(email.casefold()) + return emails + + +def shares_for_existing_users(records, existing_emails): + # type: (List[ImportRecord], set) -> List[ImportRecord] + """Keep secret shares whose email is already a Keeper user.""" + kept = [] + for rec in records: + shares = [] + for share in rec.shares or []: + email = (getattr(share, 'name', None) or '').strip() + if email and email.casefold() in existing_emails: + shares.append(share) + elif email: + logging.warning('Secret "%s": %s is not a Keeper user; share skipped', rec.title, email) + if not shares: + continue + scoped = copy.copy(rec) + scoped.shares = shares + kept.append(scoped) + return kept + + +def apply_secret_share_records(params, records): + # type: (KeeperParams, List[ImportRecord]) -> None + """Grant secret-level record shares loaded from a membership file. + + The Keeper enterprise user list is loaded first. A share is granted only + for an email that is already in that list. + """ + pending = [rec for rec in records if getattr(rec, 'shares', None)] + if not pending: + return + existing = existing_enterprise_emails(params) + if existing is None: + return + pending = shares_for_existing_users(pending, existing) + if not pending: + return + sync_down.sync_down(params) + grant_imported_record_shares(params, locate_secret_share_records(params, pending)) + + +def grant_imported_record_shares(params, records): + # type: (KeeperParams, List[ImportRecord]) -> None + """Grant per-user record shares from a secret permission list. + + Each share is an email plus can_edit and can_share, which is the same + grant ``share-record`` writes. Folder membership is not changed here. + """ + pending = [rec for rec in records if getattr(rec, 'shares', None) and rec.uid] + if not pending: + return + from ..commands.register import ShareRecordCommand + from ..error import CommandError + + owner_email = (getattr(params, 'user', None) or '').strip().casefold() + + def grant_bucket(record, emails, can_edit, can_share): + request = ShareRecordCommand.prep_request(params, { + 'record': record.uid, + 'email': emails, + 'action': 'grant', + 'can_edit': can_edit, + 'can_share': can_share, + }) + if request is not None: + ShareRecordCommand.send_requests(params, [request]) + + for rec in pending: + buckets = {} + for share in rec.shares: + email = (getattr(share, 'name', None) or '').strip() + if '@' not in email: + continue + if owner_email and email.casefold() == owner_email: + continue + key = (bool(share.can_edit), bool(share.can_share)) + emails = buckets.setdefault(key, []) + if email not in emails: + emails.append(email) + for (can_edit, can_share), emails in buckets.items(): + try: + grant_bucket(rec, emails, can_edit, can_share) + except CommandError as exc: + if len(emails) == 1: + logging.warning('Secret share for "%s": %s', rec.title, getattr(exc, 'message', exc)) + continue + for email in emails: + try: + grant_bucket(rec, [email], can_edit, can_share) + except CommandError as one_exc: + logging.warning( + 'Secret share for "%s": %s', rec.title, getattr(one_exc, 'message', one_exc)) + except Exception as one_exc: + logging.warning('Secret share for "%s": %s', rec.title, one_exc) + except Exception as exc: + logging.warning('Secret share for "%s": %s', rec.title, exc) + + def prepare_record_permission(params, records): # type: (KeeperParams, List[ImportRecord]) -> Dict[str, List[folder_pb2.SharedFolderUpdateRecord]] """Prepare a list of API interactions for changes to record permissions.""" shared_update = {} # type: Dict[str, List[folder_pb2.SharedFolderUpdateRecord]] diff --git a/keepercommander/importer/importer.py b/keepercommander/importer/importer.py index b99ad16a1..99ee7ac07 100644 --- a/keepercommander/importer/importer.py +++ b/keepercommander/importer/importer.py @@ -23,6 +23,17 @@ from ..params import KeeperParams PathDelimiter = '\\' + + +def normalize_folder_path(path): + # type: (Optional[str]) -> str + """Fold a Delinea or Keeper folder path into one comparable key. + + Secret Server paths use a leading backslash. Keeper paths built from + folder names do not. Both separators are treated as backslash. + """ + text = (path or '').replace('/', '\\').strip() + return text.strip('\\').casefold() TWO_FACTOR_CODE = 'TFC:Keeper' FIELD_TYPE_ONE_TIME_CODE = 'oneTimeCode' @@ -122,6 +133,15 @@ def __init__(self): self.role = None # NSF role name: viewer, content-manager, full-manager, … +class RecordShare: + """Direct record share produced by an importer. name is an email.""" + + def __init__(self, name, can_edit, can_share): + self.name = name + self.can_edit = can_edit + self.can_share = can_share + + class SharedFolder: def __init__(self): self.uid = None @@ -248,6 +268,7 @@ def __init__(self): self.last_modified = 0 self.fields = [] # type: List[RecordField] self.folders = None # type: Optional[List[Folder]] + self.shares = None # type: Optional[List[RecordShare]] self.attachments = None # type: Optional[List[Attachment]] self.references = None # type: Optional[List[RecordReferences]] self.schema = None # type: Optional[List[RecordSchemaField]] diff --git a/keepercommander/importer/json/json.py b/keepercommander/importer/json/json.py index 567cf86a2..e0a16f9e1 100644 --- a/keepercommander/importer/json/json.py +++ b/keepercommander/importer/json/json.py @@ -22,7 +22,7 @@ from .. import imp_exp from ..importer import (BaseFileImporter, BaseExporter, Record, RecordField, RecordSchemaField, RecordReferences, - Folder, SharedFolder, Permission, Team, Attachment, + Folder, SharedFolder, Permission, Team, Attachment, RecordShare, BaseDownloadMembership, BaseDownloadRecordType, RecordType, RecordTypeField) from ... import api, utils, record_types from ...proto import enterprise_pb2 @@ -132,6 +132,17 @@ def json_to_record(j_record): # type: (Dict[str, Any]) -> Optional[Record] folder.can_edit = f.get('can_edit') folder.can_share = f.get('can_share') record.folders.append(folder) + shares = j_record.get('shares') + if isinstance(shares, list): + record.shares = [] + for share in shares: + if not isinstance(share, dict): + continue + name = (share.get('name') or '').strip() + if not name: + continue + record.shares.append(RecordShare( + name, bool(share.get('can_edit')), bool(share.get('can_share')))) return record @@ -236,9 +247,13 @@ def do_import(self, filename, **kwargs): team.members = [x for x in ms if isinstance(x, str) and len(x) > 3] yield team - if not users_only and records: + if isinstance(records, list): for r in records: + if users_only and not (isinstance(r, dict) and r.get('shares')): + continue record = KeeperJsonMixin.json_to_record(r) + if users_only and not record.shares: + continue if zip_archive and 'attachments' in r: attachments = r['attachments'] record.attachments = [] @@ -376,6 +391,13 @@ def do_export(self, filename, items, zip_archive=None, **kwargs): ro['references'][ref_name] = refs refs.extend(ref.uids) + if r.shares: + ro['shares'] = [{ + 'name': share.name, + 'can_edit': bool(share.can_edit), + 'can_share': bool(share.can_share), + } for share in r.shares if getattr(share, 'name', None)] + if r.folders: ro['folders'] = [] for folder in r.folders: diff --git a/keepercommander/importer/thycotic/thycotic.py b/keepercommander/importer/thycotic/thycotic.py index 4f29405f0..591bf335d 100644 --- a/keepercommander/importer/thycotic/thycotic.py +++ b/keepercommander/importer/thycotic/thycotic.py @@ -11,6 +11,7 @@ import copy import csv import datetime +import functools import io import getpass import json @@ -18,8 +19,11 @@ import os import re import sys +import threading import time import warnings +from collections import deque +from concurrent.futures import ThreadPoolExecutor from contextlib import contextmanager from urllib3.exceptions import InsecureRequestWarning @@ -32,7 +36,8 @@ from typing import Tuple, Optional, Iterable, Union, Dict, Callable, Any, List from ..importer import (BaseImporter, BaseDownloadMembership, Record, SharedFolder, Folder, Attachment, Permission, - RecordField, BytesAttachment, Team, BaseDownloadRecordType, RecordType, RecordTypeField) + RecordField, RecordShare, BytesAttachment, Team, BaseDownloadRecordType, RecordType, + RecordTypeField) from ...constants import EMAIL_PATTERN from ...params import KeeperParams from ... import record_types, vault, api @@ -125,15 +130,70 @@ def lookup_secret_ids(search, folder_ids=None, include_restricted=True): return _ids_from_lookup_response(search(query)) +DEFAULT_SECRET_SHARE_WORKERS = 4 +MAX_SECRET_SHARE_WORKERS = 16 + + +class FolderPermissionCache: + """Folder id -> folder permission rows, shared by the secret-share worker threads. + + Reads and writes take a lock. The first rows stored for a folder win. + Failed fetches are never stored, so they are retried. + """ + + def __init__(self): + self._rows = {} # type: Dict[Any, list] + self._lock = threading.Lock() + + def get(self, folder_id): # type: (Any) -> Optional[list] + with self._lock: + return self._rows.get(folder_id) + + def put(self, folder_id, rows): # type: (Any, list) -> list + with self._lock: + return self._rows.setdefault(folder_id, rows) + + def __contains__(self, folder_id): # type: (Any) -> bool + with self._lock: + return folder_id in self._rows + + class ThycoticMixin: @staticmethod - def connect_to_server(url=None): # type: (Optional[str]) -> ThycoticAuth + def url_from_tenant(tenant): # type: (Optional[str]) -> str + """Build a Secret Server base URL from a shell variable. Nothing is printed.""" + tenant = (tenant or '').strip().rstrip('/') + if not tenant: + return '' + if tenant.startswith('https://') or tenant.startswith('http://'): + return tenant + if '.' not in tenant: + tenant = '{0}.secretservercloud.com'.format(tenant) + return 'https://{0}/'.format(tenant) + + @staticmethod + def expand_shell_value(value, envvars): # type: (str, Optional[dict]) -> Optional[str] + """Replace ${NAME} from the Commander shell. Do not print the result.""" + from keepercommander.commands.base import expand_cmd_args, parameter_pattern + expanded = expand_cmd_args(value or '', envvars or {}) + missing = parameter_pattern.findall(expanded) + if missing: + logging.warning('Shell variable is not set: %s', ', '.join(missing)) + return None + return expanded + + @staticmethod + def connect_to_server(url=None, envvars=None): # type: (Optional[str], Optional[dict]) -> ThycoticAuth if not url: url = input('...' + 'Thycotic Host or URL'.rjust(30) + ': ') if not url: logging.warning('Thycotic Host or URL is required') return + url = ThycoticMixin.expand_shell_value(url, envvars) + if not url: + return + url = ThycoticMixin.url_from_tenant(url) if not url.startswith('http'): url = f'https://{url}' username = input('...' + 'Thycotic Username'.rjust(30) + ': ') @@ -461,6 +521,438 @@ def get_folders(auth, skip_permissions=False): + @staticmethod + def _secret_role_flags(role): + # type: (str) -> Optional[Tuple[bool, bool]] + """Map a Secret Server secret role to Keeper can_edit, can_share. + + List and View are both read-only. Keeper direct shares have no + title-only role, so List cannot be kept apart from View. + """ + role = (role or '').strip().casefold() + if role == 'owner': + return True, True + if role == 'edit': + return True, False + if role in ('view', 'list'): + return False, False + return None + + @staticmethod + def _remember_email(emails, user_id, email): + email = (email or '').strip() + if not email or user_id in (None, ''): + return + emails[user_id] = email + try: + emails[int(user_id)] = email + except (TypeError, ValueError): + pass + + @staticmethod + def provisioned_emails(auth): + # type: (ThycoticAuth) -> Dict + """Map a Delinea user id to an email. No users are created. + + Folder membership already resolves these ids through the user list. + A failed detail fetch must not throw that map away. + """ + emails = {} + for user_id, user in (ThycoticMixin.get_user_lookup(auth) or {}).items(): + if not isinstance(user, dict): + continue + email = (user.get('emailAddress') or '').strip() + if not email: + username = (user.get('userName') or '').strip() + if '@' in username: + email = username + ThycoticMixin._remember_email(emails, user_id, email) + try: + users = ThycoticImporter()._prepare_thycotic_users(auth) or [] + except Exception as exc: + logging.info('Delinea users for secret shares: %s', exc) + users = [] + for user in users: + ThycoticMixin._remember_email(emails, user.get('id'), user.get('email')) + return emails + + @staticmethod + def _email_for_user(emails, user_id): + email = emails.get(user_id) or '' + if email or user_id in (None, '', 0): + return email + try: + return emails.get(int(user_id)) or '' + except (TypeError, ValueError): + return '' + + @staticmethod + def shares_from_secret_permissions(rows, emails, groups=None): + # type: (list, Dict, Optional[Dict]) -> List[RecordShare] + """Map permission rows to record shares for provisioned emails. + + Group flattening: Keeper record shares are per user, so a group row + becomes one direct share per member that has a provisioned email. + - The group link is not kept. Users added to the Delinea group after + the import get no access, and removing a member does not revoke it. + - Only direct members are shared. Nested groups are not expanded. + - A user reached through several rows (directly and via groups) gets + one share with the highest role: Owner > Edit > View/List. + + Rows with a role outside Owner/Edit/View/List are skipped. This does + not change the shared-folder membership. + """ + if not isinstance(rows, list): + return [] + groups = groups or {} + grants = {} + for row in rows: + if not isinstance(row, dict): + continue + flags = ThycoticMixin._secret_role_flags(row.get('secretAccessRoleName') or '') + if flags is None: + continue + user_ids = [] + user_id = row.get('userId') or 0 + group_id = row.get('groupId') or 0 + direct_email = '' + for key in ('emailAddress', 'userName', 'knownAs'): + candidate = (row.get(key) or '') + if isinstance(candidate, str) and '@' in candidate: + direct_email = candidate.strip() + break + if user_id: + user_ids.append(user_id) + elif direct_email: + previous = grants.get(direct_email) + if previous is None: + grants[direct_email] = flags + else: + grants[direct_email] = (previous[0] or flags[0], previous[1] or flags[1]) + continue + elif group_id: + group = groups.get(group_id) or {} + if not group: + try: + group = groups.get(int(group_id)) or {} + except (TypeError, ValueError): + group = {} + user_ids.extend(group.get('users') or []) + for member_id in user_ids: + email = ThycoticMixin._email_for_user(emails, member_id) + if not email and direct_email and member_id == user_id: + email = direct_email + if not email: + continue + previous = grants.get(email) + if previous is None: + grants[email] = flags + else: + grants[email] = (previous[0] or flags[0], previous[1] or flags[1]) + return [ + RecordShare(email, can_edit, can_share) + for email, (can_edit, can_share) in sorted(grants.items()) + ] + + @staticmethod + def folder_permission_rows(auth, folders, folder_id, cache=None): + # type: (ThycoticAuth, Dict, int, Optional[FolderPermissionCache]) -> list + """Permission rows for a secret that inherits, walking up to a folder that has them. + + cache keeps the rows each folder returned, so secrets in the same + folder do not fetch it again. A failed fetch is not cached. + """ + seen = set() + current = folder_id + while current and current not in seen: + seen.add(current) + rows = cache.get(current) if cache is not None else None + if rows is None: + try: + rows = auth.thycotic_search('/v1/folder-permissions?filter.folderId={0}'.format(current)) + except Exception as exc: + logging.info('folder-permissions %s: %s', current, exc) + rows = [] + else: + if cache is not None and isinstance(rows, list): + rows = cache.put(current, rows) + if isinstance(rows, list) and rows: + return rows + folder = folders.get(current) if isinstance(folders, dict) else None + parent = folder.get('parentFolderId') if isinstance(folder, dict) else 0 + if not parent or parent == current: + break + current = parent + return [] + + @staticmethod + def all_secret_permission_rows(auth): + # type: (ThycoticAuth) -> Optional[Dict[Any, list]] + """Every secret-permission row from one paged search, grouped by secretId. + + Returns None when the search fails or returns nothing, so callers fall + back to one secret-permissions request per secret. + """ + try: + rows = auth.thycotic_search('/v1/secret-permissions') + except Exception as exc: + logging.warning('Bulk secret-permissions failed, loading them per secret: %s', exc) + return None + by_secret = {} # type: Dict[Any, list] + for row in rows if isinstance(rows, list) else []: + if isinstance(row, dict) and row.get('secretId'): + by_secret.setdefault(row['secretId'], []).append(row) + if not by_secret: + logging.info('Bulk secret-permissions returned no rows, loading them per secret') + return None + return by_secret + + @staticmethod + def permission_rows_for_secret(auth, folders, secret, folder_cache=None, secret_permissions=None): + # type: (ThycoticAuth, Dict, dict, Optional[Dict], Optional[Dict[Any, list]]) -> list + """Rows that name who can open this secret. + + Secret Server often omits inheritPermissions on the secret record. + Some secrets then return their users from secret-permissions, and + others only from the folder. Keep whichever list is non-empty. A + secret that inherits prefers the folder's secret roles. + + secret_permissions is the result of all_secret_permission_rows. When + given, it replaces the per-secret secret-permissions request. + """ + folder_id = secret.get('folderId') if isinstance(secret, dict) else None + secret_id = secret.get('id') if isinstance(secret, dict) else None + inherits = isinstance(secret, dict) and secret.get('inheritPermissions') is True + + if inherits: + rows = ThycoticMixin.folder_permission_rows(auth, folders, folder_id, folder_cache) + if rows: + return rows + return ThycoticMixin._secret_permission_rows(auth, secret_id, secret_permissions) + rows = ThycoticMixin._secret_permission_rows(auth, secret_id, secret_permissions) + if rows: + return rows + return ThycoticMixin.folder_permission_rows(auth, folders, folder_id, folder_cache) + + @staticmethod + def _secret_permission_rows(auth, secret_id, secret_permissions=None): + # type: (ThycoticAuth, Any, Optional[Dict[Any, list]]) -> list + """Secret-permission rows for one secret, from the bulk result when available.""" + if not secret_id: + return [] + if secret_permissions is not None: + return list(secret_permissions.get(secret_id) or []) + try: + rows = auth.thycotic_search('/v1/secret-permissions?filter.secretId={0}'.format(secret_id)) + except Exception as exc: + logging.warning('secret-permissions %s: %s', secret_id, exc) + return [] + return rows if isinstance(rows, list) else [] + + @staticmethod + def _fetch_secret_permissions(auth, folders, secret_id, folder_cache, secret_permissions=None): + # type: (ThycoticAuth, Dict, Any, Dict, Optional[Dict[Any, list]]) -> Tuple[Optional[dict], list] + """Fetch one secret and its permission rows. Runs on a worker thread.""" + secret = auth.thycotic_entity('/v1/secrets/{0}'.format(secret_id)) or {} + if not isinstance(secret, dict): + return None, [] + secret['id'] = secret.get('id') or secret_id + return secret, ThycoticMixin.permission_rows_for_secret( + auth, folders, secret, folder_cache, secret_permissions) + + @staticmethod + def _fetch_secret_entry(auth, folders, folder_cache, secret_permissions, entry): + # type: (ThycoticAuth, Dict, FolderPermissionCache, Optional[Dict[Any, list]], Tuple[dict, Any]) -> Tuple[Optional[dict], list, Optional[Exception]] + """Worker task: (secret, rows, None) on success, (None, [], error) on failure. Never raises.""" + try: + secret, rows = ThycoticMixin._fetch_secret_permissions( + auth, folders, entry[1], folder_cache, secret_permissions) + return secret, rows, None + except Exception as exc: + return None, [], exc + + @staticmethod + def secret_share_workers(value): + # type: (Any) -> int + """Worker count for loading secret permissions: default 4, clamped to 1..16.""" + if value is None: + return DEFAULT_SECRET_SHARE_WORKERS + try: + return min(max(int(value), 1), MAX_SECRET_SHARE_WORKERS) + except (TypeError, ValueError): + return DEFAULT_SECRET_SHARE_WORKERS + + @staticmethod + def _fetch_in_order(fetch, items, workers): + # type: (Callable[[Any], Any], Iterable[Any], int) -> Iterable[Tuple[Any, Any]] + """Yield (item, fetch(item)) in input order, with at most `workers` fetches running. + + fetch must not raise. At most workers * 2 fetches are queued, so memory + stays bounded. With one worker everything runs on the calling thread. + If the caller stops early, queued fetches are cancelled and only the + running ones are waited for. + """ + if workers <= 1: + for item in items: + yield item, fetch(item) + return + executor = ThreadPoolExecutor(max_workers=workers, thread_name_prefix='thycotic-shares') + items = iter(items) + pending = deque() + try: + for item in items: + pending.append((item, executor.submit(fetch, item))) + if len(pending) >= workers * 2: + break + while pending: + item, future = pending.popleft() + result = future.result() + for next_item in items: + pending.append((next_item, executor.submit(fetch, next_item))) + break + yield item, result + finally: + for _item, future in pending: + future.cancel() + executor.shutdown(wait=True) + + @staticmethod + def _warn_unmapped_roles(title, secret_id, rows): + # type: (str, Any, list) -> bool + """Log the roles on this secret that have no Keeper equivalent. True if any were found.""" + roles = sorted({ + (item.get('secretAccessRoleName') or '').strip() or '(unnamed)' + for item in rows or [] + if isinstance(item, dict) and ThycoticMixin._secret_role_flags(item.get('secretAccessRoleName')) is None + }) + if roles: + logging.warning( + 'Secret "%s" (%s): role(s) %s cannot be mapped to Keeper permissions; ' + 'those users and groups were not shared, share them manually', + title, secret_id, ', '.join(roles)) + return bool(roles) + + @staticmethod + def _secret_folder_path(folders, folder_id): + # type: (Optional[Dict], Any) -> str + """Delinea folder path for folder_id, whether the folder map is keyed by int or str.""" + if not isinstance(folders, dict): + return '' + folder = folders.get(folder_id) + if not isinstance(folder, dict): + try: + folder = folders.get(int(folder_id)) + except (TypeError, ValueError): + folder = None + return (folder.get('folderPath') or '') if isinstance(folder, dict) else '' + + @staticmethod + def _secret_share_record(title, folder_path, shares): + # type: (str, str, List[RecordShare]) -> Record + record = Record() + record.title = title + if folder_path: + record_folder = Folder() + record_folder.path = folder_path + record.folders = [record_folder] + record.shares = shares + return record + + @staticmethod + def iter_secret_shares(auth, folders, workers=None): + # type: (ThycoticAuth, Optional[Dict], Optional[int]) -> Iterable[Record] + """Yield one record per secret with the users who can open it. + + A secret that inherits uses its folder's secret roles. Any other + secret uses GET /v1/secret-permissions, and falls back to the folder + secret roles when that list is empty. apply-membership grants the + resulting shares onto the Keeper record. Folder membership is not + changed. + + Secret permissions come from one bulk search when the server allows + it, otherwise one request per secret. Secret reads run on a small + worker pool (`workers`, default 4, 1 = sequential); records are + yielded in lookup order. Group lookup and record building stay on the + calling thread. + + A secret whose fetch fails is logged and skipped. The rest still load. + """ + emails = ThycoticMixin.provisioned_emails(auth) + groups = None + try: + lookup = auth.thycotic_search('/v1/secrets/lookup') or [] + except Exception as exc: + logging.warning('secret lookup for permissions: %s', exc) + return + if not isinstance(lookup, list): + lookup = [] + folders = folders or {} + entries = [(row, row.get('id')) for row in lookup if isinstance(row, dict) and row.get('id')] + written = 0 + seen = 0 + unmapped = 0 + failed = 0 + custom_role_secrets = 0 + folder_cache = FolderPermissionCache() + secret_permissions = ThycoticMixin.all_secret_permission_rows(auth) if entries else None + workers = min(ThycoticMixin.secret_share_workers(workers), max(len(entries), 1)) + logging.info('Loading secret permissions for %d secrets (%d worker(s))', len(entries), workers) + + fetch = functools.partial(ThycoticMixin._fetch_secret_entry, auth, folders, folder_cache, secret_permissions) + for (row, secret_id), (secret, perm_rows, error) in ThycoticMixin._fetch_in_order(fetch, entries, workers): + if error is not None: + logging.warning('Secret %s permissions skipped: %s', secret_id, error) + failed += 1 + continue + if secret is None: + continue + seen += 1 + title = secret.get('name') or row.get('name') or '' + if not title: + continue + if ThycoticMixin._warn_unmapped_roles(title, secret_id, perm_rows): + custom_role_secrets += 1 + if groups is None and any(isinstance(item, dict) and item.get('groupId') for item in perm_rows or []): + groups = ThycoticMixin.get_group_lookup(auth) or {} + shares = ThycoticMixin.shares_from_secret_permissions(perm_rows, emails, groups) + if not shares: + if perm_rows: + unmapped += 1 + continue + written += 1 + yield ThycoticMixin._secret_share_record( + title, ThycoticMixin._secret_folder_path(folders, secret.get('folderId')), shares) + logging.info('Loaded secret permissions for %d secrets', written) + if failed: + logging.warning('%d secret(s) were skipped because their permissions could not be fetched.', failed) + if custom_role_secrets: + logging.warning('%d secret(s) have custom roles that were not shared; see the warnings above.', + custom_role_secrets) + if written == 0: + logging.warning( + 'No secret permission lists downloaded (%d secrets, %d had permission rows that matched ' + 'no provisioned Keeper user).', + seen, unmapped) + + +def parse_thycotic_import_target(filename): + """Return (username, base_url) for an import --format=thycotic target. + + user@https://host/ must not be prefixed with another scheme. Doing so + sends the token request to a host named https. + """ + filename = (filename or '').strip() + if filename.startswith('https://') or filename.startswith('http://'): + return '', filename + username, sep, host = filename.partition('@') + if not sep or not host.strip(): + return None + host = ThycoticMixin.url_from_tenant(host.strip()) + if not host: + return None + return username, host + + class ThycoticImporter(BaseImporter, ThycoticMixin): # Same end-of-import terminal summary CyberArk prints (Import finished, skipped rows). verbose_import_summary = True @@ -506,12 +998,19 @@ def _user_field(user, key): # type: (dict, str) -> str @staticmethod def _extract_user_email(user): - """Return the user's emailAddress, or an empty string.""" + """Return the user's emailAddress, or an empty string. + + The list payload uses emailAddress. Other casings of the key are + accepted too. An empty value does not hide another key. + """ if not isinstance(user, dict): return '' + value = user.get('emailAddress') + if isinstance(value, str) and value.strip(): + return value.strip() for key, value in user.items(): - if str(key).lower() == 'emailaddress' and isinstance(value, str): - return value.strip() + if str(key).casefold() == 'emailaddress' and isinstance(value, str) and value.strip(): + return value.strip() return '' def _prepare_thycotic_users(self, auth): # type: (ThycoticAuth) -> List[dict] @@ -535,9 +1034,9 @@ def _prepare_thycotic_users(self, auth): # type: (ThycoticAuth) -> List[dict] source = user if not email and user_id not in (None, ''): try: - detail = auth.thycotic_entity(f'/v1/users/{user_id}') - except Exception as e: - logging.warning('Failed to fetch Thycotic user %s: %s', user_id, e) + detail = auth.thycotic_entity('/v1/users/{0}'.format(user_id)) + except Exception as exc: + logging.warning('Thycotic user %s: %s', user_id, exc) detail = None if isinstance(detail, dict): if detail.get('isApplicationAccount') is True: @@ -560,7 +1059,7 @@ def _prepare_thycotic_users(self, auth): # type: (ThycoticAuth) -> List[dict] logging.info('Skipped %d Thycotic application account(s)', skipped_application) if skipped_no_email: logging.info('Skipped %d Thycotic user(s) with no email address', skipped_no_email) - logging.info(f'Found {len(result)} Thycotic user(s) with an email address') + logging.info('Found %d Thycotic user(s) with an email address', len(result)) return result def _load_existing_keeper_users(self, params): # type: (KeeperParams) -> list @@ -593,10 +1092,27 @@ def _create_keeper_user(self, params, email, name): logging.warning('Could not create enterprise user "%s": %s', email, e) return None + @staticmethod + def _user_outcome(user, flag, **extra): # type: (dict, str, ...) -> dict + """One line of the user report.""" + item = { + 'username': str(user.get('username') or ''), + 'email': ThycoticImporter._user_field(user, 'email'), + 'flag': flag, + } + item.update(extra) + return item + def _create_enterprise_users(self, params, thycotic_users): - # type: (KeeperParams, List[dict]) -> None + # type: (KeeperParams, List[dict]) -> dict seen = set() + outcomes = [] existing_users = self._load_existing_keeper_users(params) + existing_by_email = {} + for existing in existing_users: + key = (existing.get('email') or '').strip().casefold() + if key and key not in existing_by_email: + existing_by_email[key] = existing is_valid_email = re.compile(EMAIL_PATTERN).match for user in thycotic_users or []: if not isinstance(user, dict): @@ -605,18 +1121,44 @@ def _create_enterprise_users(self, params, thycotic_users): email_key = email.casefold() if email_key in seen: continue - user_exist = next( - (u for u in existing_users if (u.get('email') or '').casefold() == email_key), - None, - ) + user_exist = existing_by_email.get(email_key) if user_exist: seen.add(email_key) + status = user_exist.get('status') or 'active' + outcomes.append(self._user_outcome(user, 'existing', status=status)) + elif is_valid_email(email): + seen.add(email_key) + name = self._user_field(user, 'name') or self._user_field(user, 'username') or email + created = self._create_keeper_user(params, email, name) + if created: + outcomes.append(self._user_outcome(user, 'created')) + else: + outcomes.append(self._user_outcome(user, 'failed', reason='user was not created')) + else: + outcomes.append(self._user_outcome(user, 'skipped', reason='invalid email')) + return {'users': outcomes} + + def _print_thycotic_user_report(self, report): # type: (dict) -> None + users = report.get('users') or [] + counts = {'created': 0, 'existing': 0, 'skipped': 0, 'failed': 0} + for user in users: + flag = str((user or {}).get('flag') or 'skipped') + if flag in counts: + counts[flag] += 1 + logging.info( + 'Thycotic users: created %d, existing %d, skipped %d, failed %d', + counts['created'], counts['existing'], counts['skipped'], counts['failed'], + ) + for user in users: + if not isinstance(user, dict): continue - if not is_valid_email(email): - continue - seen.add(email_key) - name = self._user_field(user, 'name') or self._user_field(user, 'username') or email - self._create_keeper_user(params, email, name) + flag = str(user.get('flag') or 'skipped') + label = user.get('email') or user.get('username') or '(no email)' + detail = user.get('reason') or '' + if not detail and flag == 'existing' and user.get('status'): + detail = str(user.get('status')) + suffix = ' ({0})'.format(detail) if detail else '' + logging.info('[%s] %s%s', flag, label, suffix) def _preview_thycotic_users(self, auth): # type: (ThycoticAuth) -> None @@ -647,14 +1189,15 @@ def _migrate_thycotic_users(self, auth, params): A user-fetch failure does not abort secret migration. """ - logging.info('Migrating Thycotic users to Keeper...') + logging.info('Migrating Thycotic users to Keeper') try: thycotic_users = self._prepare_thycotic_users(auth) except Exception as e: logging.warning('Failed to fetch Thycotic users: %s', e) logging.warning('Continuing import without creating Keeper users.') return - self._create_enterprise_users(params, thycotic_users) + report = self._create_enterprise_users(params, thycotic_users) + self._print_thycotic_user_report(report) def do_import(self, filename, **kwargs): # type: (ThycoticImporter, str, ...) -> Iterable[Union[Record, SharedFolder]] @@ -670,20 +1213,14 @@ def do_import(self, filename, **kwargs): except: pass - if filename.startswith('https://'): - username = '' - host = filename - else: - username, sep, host = filename.partition('@') - if sep: - host = f'https://{host}/' - else: - logging.warning('Thycotic connection parameters:') - logging.warning(' @.') - logging.warning(' https:///. Full URL') - raise Exception('Import canceled') + parsed = parse_thycotic_import_target(filename) + if not parsed: + logging.warning('Thycotic connection parameters:') + logging.warning(' @.') + logging.warning(' https:///. Full URL') + raise Exception('Import canceled') + username, host = parsed - print('Secret Server URL: '.rjust(25) + host) username_prompt = 'Thycotic Username: '.rjust(25) if not username: username = input(username_prompt) @@ -1162,6 +1699,10 @@ def print_skipped(skipped_secrets, skipped_fields): # type: (List[dict], List[d class ThycoticAuth: + max_throttle_retries = 3 + throttle_backoff_seconds = 5 + max_throttle_wait_seconds = 300 + def __init__(self, base_url): # type: (str) -> None if not base_url.endswith('/'): base_url += '/' @@ -1171,9 +1712,12 @@ def __init__(self, base_url): # type: (str) -> None self.refresh_token = '' self.last_login = 0 self.proxy = None + self._token_lock = threading.Lock() warnings.simplefilter('ignore', InsecureRequestWarning) def authenticate(self, username, password, on_totp=None): # type: (str, str, Optional[Callable[[], str]]) -> None + if isinstance(username, str): + username = username.strip() request_data = { 'grant_type': 'password', 'username': username, @@ -1183,6 +1727,9 @@ def authenticate(self, username, password, on_totp=None): # type: (str, str, Op 'Accept': 'application/json', 'Content-Type': 'application/x-www-form-urlencoded' } + logging.debug( + 'Secret Server token request password_length=%d proxy=%s', + len(password or ''), 'yes' if self.proxy else 'no') rs = requests.post(self.base_url + 'oauth2/token', data=request_data, headers=headers, verify=False, proxies=self.proxy) if rs.status_code != 200: @@ -1226,11 +1773,35 @@ def refresh_auth_token(self): raise Exception(self._error_message(rs, 'error')) def ensure_auth_token(self): - if self.access_token: - now = int(datetime.datetime.now().timestamp()) - now -= 10 - if self.last_login + self.expires_in < now: - self.refresh_auth_token() + # Locked so parallel workers refresh once; a second refresh would reuse a rotated refresh token. + with self._token_lock: + if self.access_token: + now = int(datetime.datetime.now().timestamp()) + now -= 10 + if self.last_login + self.expires_in < now: + self.refresh_auth_token() + + def _throttle_wait(self, rs, attempt): # type: (requests.Response, int) -> int + """Seconds to wait before retry `attempt`: the larger of Retry-After (capped) and exponential backoff.""" + try: + hint = min(max(int(rs.headers.get('Retry-After')), 0), self.max_throttle_wait_seconds) + except (TypeError, ValueError): + hint = 0 + return max(hint, self.throttle_backoff_seconds * (2 ** (attempt - 1))) + + def _get(self, url, **kwargs): # type: (str, ...) -> requests.Response + """GET that waits and retries when Secret Server answers 429 Too Many Requests.""" + attempt = 0 + while True: + rs = requests.get(url, verify=False, proxies=self.proxy, **kwargs) + if rs.status_code != 429 or attempt >= self.max_throttle_retries: + return rs + attempt += 1 + wait = self._throttle_wait(rs, attempt) + rs.close() + logging.warning('Secret Server rate limit (attempt %d/%d), retrying in %d seconds', + attempt, self.max_throttle_retries, wait) + time.sleep(wait) def thycotic_search(self, endpoint): # type: (str) -> list self.ensure_auth_token() @@ -1250,10 +1821,10 @@ def thycotic_search(self, endpoint): # type: (str) -> list q.append(f'skip={skip}') url = urlunparse((urlp.scheme or 'https', urlp.netloc, urlp.path, None, '&'.join(q), None)) try: - rs = requests.get(url, headers=headers, verify=False, proxies=self.proxy) + rs = self._get(url, headers=headers) except requests.exceptions.ConnectionError: time.sleep(10) - rs = requests.get(url, headers=headers, verify=False, proxies=self.proxy) + rs = self._get(url, headers=headers) if rs.status_code != 200: raise Exception(self._error_message(rs)) chunk_rs = rs.json() @@ -1345,12 +1916,12 @@ def _read_entity(self, endpoint): # type: (str) -> Tuple[Optional[dict], Option 'Authorization': f'Bearer {self.access_token}' } try: - rs = requests.get(self.base_url + 'api' + endpoint, headers=headers, verify=False, proxies=self.proxy) + rs = self._get(self.base_url + 'api' + endpoint, headers=headers) except Exception as e: logging.warning('"%s" error: %s', endpoint, str(e)) time.sleep(10) try: - rs = requests.get(self.base_url + 'api' + endpoint, headers=headers, verify=False, proxies=self.proxy) + rs = self._get(self.base_url + 'api' + endpoint, headers=headers) except Exception as e: logging.warning('Another "%s" error: %s', endpoint, str(e)) time.sleep(10) @@ -1371,12 +1942,10 @@ def thycotic_get(self, endpoint): # type: (str) -> requests.Response } try: - rs = requests.get(self.base_url + 'api' + endpoint, headers=headers, - verify=False, proxies=self.proxy, stream=True) + rs = self._get(self.base_url + 'api' + endpoint, headers=headers, stream=True) except requests.exceptions.ConnectionError: time.sleep(10) - rs = requests.get(self.base_url + 'api' + endpoint, headers=headers, - verify=False, proxies=self.proxy, stream=True) + rs = self._get(self.base_url + 'api' + endpoint, headers=headers, stream=True) return rs def thycotic_secret_get(self, endpoint): # type: (str) -> requests.Response @@ -1431,7 +2000,10 @@ def prepare(self): class ThycoticMembershipDownload(BaseDownloadMembership, ThycoticMixin): def download_membership(self, params, **kwargs): - auth = ThycoticMixin.connect_to_server() + envvars = getattr(params, 'environment_variables', None) if params is not None else None + auth = ThycoticMixin.connect_to_server(envvars=envvars) + if not auth: + return users = ThycoticMembershipDownload.get_user_lookup(auth) @@ -1514,10 +2086,15 @@ def progress(position, total): yield team + yield from ThycoticMembershipDownload.iter_secret_shares(auth, folders, kwargs.get('workers')) + class ThycoticRecordTypeDownload(BaseDownloadRecordType, ThycoticMixin): def download_record_type(self, params, **kwargs): - auth = ThycoticMixin.connect_to_server() + envvars = getattr(params, 'environment_variables', None) if params is not None else None + auth = ThycoticMixin.connect_to_server(envvars=envvars) + if not auth: + return templates = auth.thycotic_search('/v1/secret-templates?filter.includeInactive=false&filter.includeSecretCount=true') logging.debug('Loaded %d secret templates', len(templates)) for template in templates: diff --git a/unit-tests/test_thycotic_access.py b/unit-tests/test_thycotic_access.py new file mode 100644 index 000000000..0a508a2d3 --- /dev/null +++ b/unit-tests/test_thycotic_access.py @@ -0,0 +1,1411 @@ +"""Executable checks for what the Thycotic importer does with access data. + +Fixtures follow field names published on the Secret Server REST API +(SecretPermissionSummary / folder permission rows): userId, groupId, +folderAccessRoleName, secretAccessRoleName, secretId, inheritPermissions. +No tenant is contacted. +""" + +from unittest import TestCase, mock + +from keepercommander.importer import imp_exp +from keepercommander.importer.importer import Folder, Record, RecordShare, normalize_folder_path +from keepercommander.importer.thycotic import thycotic +from keepercommander.importer.thycotic.thycotic import parse_thycotic_import_target + + +def _folders(): + return [ + { + 'id': 1, + 'folderName': 'Root', + 'folderPath': '\\', + 'inheritPermissions': False, + 'parentFolderId': -1, + }, + { + 'id': 10, + 'folderName': 'Apps', + 'folderPath': '\\Apps', + 'inheritPermissions': False, + 'parentFolderId': 1, + }, + { + 'id': 11, + 'folderName': 'Child', + 'folderPath': '\\Apps\\Child', + 'inheritPermissions': False, + 'parentFolderId': 10, + }, + ] + + +def _users(): + return [ + {'id': 5, 'emailAddress': 'owner@example.com', 'userName': 'owner', 'domainName': ''}, + {'id': 6, 'emailAddress': 'view@example.com', 'userName': 'view', 'domainName': ''}, + ] + + +class FakeAuth: + def __init__(self, routes): + self.routes = routes + self.calls = [] + + def thycotic_search(self, endpoint): + self.calls.append(endpoint) + for prefix, payload in self.routes: + if endpoint == prefix or endpoint.startswith(prefix): + return payload + raise AssertionError('unexpected endpoint {0}'.format(endpoint)) + + +class TestThycoticImportTarget(TestCase): + def test_user_at_https_url_is_not_double_prefixed(self): + username, host = parse_thycotic_import_target( + 'user@https://metron.secretservercloud.com/') + self.assertEqual(username, 'user') + self.assertEqual(host, 'https://metron.secretservercloud.com') + auth = thycotic.ThycoticAuth(host) + self.assertEqual(auth.base_url, 'https://metron.secretservercloud.com/') + self.assertNotIn('https://https://', auth.base_url + 'oauth2/token') + + def test_user_at_bare_host_gets_one_scheme(self): + username, host = parse_thycotic_import_target('user@metron.secretservercloud.com') + self.assertEqual(username, 'user') + self.assertEqual(host, 'https://metron.secretservercloud.com/') + + def test_bare_tenant_keeps_username_and_cloud_host(self): + username, host = parse_thycotic_import_target('user@acme') + self.assertEqual(username, 'user') + self.assertEqual(host, 'https://acme.secretservercloud.com/') + + def test_full_url_leaves_username_empty(self): + username, host = parse_thycotic_import_target('https://metron.secretservercloud.com/') + self.assertEqual(username, '') + self.assertEqual(host, 'https://metron.secretservercloud.com/') + + +class TestThycoticFolderAcl(TestCase): + def test_single_user_permission_is_discarded(self): + auth = FakeAuth([ + ('/v1/folders', _folders()), + ('/v1/folder-permissions?filter.folderId=10', [{ + 'userId': 5, + 'groupId': 0, + 'folderAccessRoleId': 1, + 'folderAccessRoleName': 'Owner', + 'secretAccessRoleId': 1, + 'secretAccessRoleName': 'Owner', + }]), + ('/v1/folder-permissions?filter.folderId=11', []), + ]) + folders = thycotic.ThycoticMixin.get_folders(auth, skip_permissions=False) + self.assertNotIn('permissions', folders[10]) + self.assertTrue(folders[10]['inheritPermissions']) + + def test_identical_child_acl_is_collapsed(self): + parent_rows = [ + { + 'userId': 5, 'groupId': 0, + 'folderAccessRoleId': 3, 'folderAccessRoleName': 'Owner', + 'secretAccessRoleId': 3, 'secretAccessRoleName': 'Edit', + }, + { + 'userId': 6, 'groupId': 0, + 'folderAccessRoleId': 4, 'folderAccessRoleName': 'View', + 'secretAccessRoleId': 4, 'secretAccessRoleName': 'View', + }, + ] + auth = FakeAuth([ + ('/v1/folders', _folders()), + ('/v1/folder-permissions?filter.folderId=10', parent_rows), + ('/v1/folder-permissions?filter.folderId=11', [dict(row) for row in parent_rows]), + ]) + folders = thycotic.ThycoticMixin.get_folders(auth, skip_permissions=False) + self.assertIn('permissions', folders[10]) + self.assertNotIn('permissions', folders[11]) + self.assertTrue(folders[11]['inheritPermissions']) + + def test_membership_maps_only_owner_and_edit(self): + rows = [ + { + 'userId': 5, 'groupId': 0, + 'folderAccessRoleId': 3, 'folderAccessRoleName': 'Owner', + 'secretAccessRoleId': 3, 'secretAccessRoleName': 'Edit', + }, + { + 'userId': 6, 'groupId': 0, + 'folderAccessRoleId': 4, 'folderAccessRoleName': 'View', + 'secretAccessRoleId': 5, 'secretAccessRoleName': 'List', + }, + { + 'userId': 0, 'groupId': 0, 'groupName': 'Ops', + 'folderAccessRoleId': 2, 'folderAccessRoleName': 'Add Secret', + 'secretAccessRoleId': 4, 'secretAccessRoleName': 'View', + }, + ] + auth = FakeAuth([ + ('/v1/users', _users()), + ('/v1/folders', [row for row in _folders() if row['id'] != 11]), + ('/v1/folder-permissions?filter.folderId=10', rows), + ('/v1/secrets/lookup', []), + ]) + with mock.patch.object(thycotic.ThycoticMixin, 'connect_to_server', return_value=auth): + shared = list(thycotic.ThycoticMembershipDownload().download_membership(None)) + + self.assertEqual(len(shared), 1) + by_name = {perm.name: perm for perm in shared[0].permissions} + self.assertTrue(by_name['owner@example.com'].manage_users) + self.assertTrue(by_name['owner@example.com'].manage_records) + self.assertFalse(by_name['view@example.com'].manage_users) + self.assertFalse(by_name['view@example.com'].manage_records) + self.assertFalse(by_name['Ops'].manage_users) + self.assertTrue(by_name['Ops'].manage_records) + self.assertFalse(any('secret-permissions' in call for call in auth.calls)) + self.assertFalse(any(call.endswith('/share') or '/share' in call for call in auth.calls)) + + +class TestThycoticSecretRecordPermissions(TestCase): + def _download(self, secret, permission_rows): + calls = [] + + def search(self, endpoint): + calls.append(endpoint) + if endpoint.startswith('/v1/folders'): + return _folders() + if endpoint.startswith('/v1/folder-permissions'): + return [] + if endpoint.startswith('/v1/groups'): + return [] + if endpoint.startswith('/v1/users'): + return _users() + if endpoint.startswith('/v1/secrets/lookup'): + return [{'id': secret['id']}] + if endpoint.startswith('/v1/secret-permissions'): + return permission_rows + raise AssertionError(endpoint) + + def entity_impl(self, endpoint): + calls.append(endpoint) + if endpoint == '/v1/secrets/{0}'.format(secret['id']): + return secret + raise AssertionError(endpoint) + + auth = mock.Mock() + auth.thycotic_search.side_effect = search.__get__(auth, type(auth)) + auth.thycotic_entity.side_effect = entity_impl.__get__(auth, type(auth)) + with mock.patch.object(thycotic.ThycoticMixin, 'connect_to_server', return_value=auth): + downloaded = list(thycotic.ThycoticMembershipDownload().download_membership(None)) + records = [item for item in downloaded if getattr(item, 'shares', None)] + return records, calls + + def _secret(self, inherit): + return { + 'id': 100, + 'name': 'prod-db', + 'folderId': 10, + 'inheritPermissions': inherit, + } + + def _shares(self, record): + return {share.name: (share.can_edit, share.can_share) for share in record.shares or []} + + def test_broken_inheritance_maps_provisioned_email(self): + records, calls = self._download(self._secret(False), [ + {'userId': 5, 'userName': 'owner-login', 'secretId': 100, 'secretAccessRoleName': 'View'}, + {'userId': 6, 'userName': 'view-login', 'secretId': 100, 'secretAccessRoleName': 'Owner'}, + {'groupId': 9, 'groupName': 'Ops', 'secretId': 100, 'secretAccessRoleName': 'Owner'}, + ]) + record = records[0] + self.assertEqual(record.title, 'prod-db') + self.assertEqual(record.folders[0].path, 'Apps') + self.assertEqual(self._shares(record), { + 'owner@example.com': (False, False), + 'view@example.com': (True, True), + }) + self.assertIn('/v1/secret-permissions', calls) + self.assertNotIn('/v1/secret-permissions?filter.secretId=100', calls) + + def test_edit_without_owner_cannot_share(self): + records, _calls = self._download(self._secret(False), [ + {'userId': 6, 'userName': 'view-login', 'secretId': 100, 'secretAccessRoleName': 'Edit'}, + ]) + self.assertEqual(self._shares(records[0]), { + 'view@example.com': (True, False), + }) + + def test_view_and_list_are_read_only(self): + records, _calls = self._download(self._secret(False), [ + {'userId': 6, 'userName': 'view-login', 'secretId': 100, 'secretAccessRoleName': 'List'}, + {'userId': 5, 'userName': 'owner-login', 'secretId': 100, 'secretAccessRoleName': 'View'}, + ]) + self.assertEqual(self._shares(records[0]), { + 'owner@example.com': (False, False), + 'view@example.com': (False, False), + }) + + def test_inheriting_secret_copies_folder_secret_roles(self): + calls = [] + secret = self._secret(True) + folder_rows = [ + {'userId': 5, 'groupId': 0, 'folderAccessRoleId': 3, 'secretAccessRoleId': 4, 'secretAccessRoleName': 'Edit'}, + {'userId': 6, 'groupId': 0, 'folderAccessRoleId': 4, 'secretAccessRoleId': 5, 'secretAccessRoleName': 'View'}, + ] + + def search(self, endpoint): + calls.append(endpoint) + if endpoint.startswith('/v1/folders'): + return _folders() + if endpoint.startswith('/v1/folder-permissions?filter.folderId=10'): + return folder_rows + if endpoint.startswith('/v1/folder-permissions'): + return [] + if endpoint.startswith('/v1/users'): + return _users() + if endpoint.startswith('/v1/secrets/lookup'): + return [{'id': secret['id']}] + if endpoint.startswith('/v1/groups'): + return [] + raise AssertionError(endpoint) + + def entity_impl(self, endpoint): + if endpoint == '/v1/secrets/100': + return secret + raise AssertionError(endpoint) + + auth = mock.Mock() + auth.thycotic_search.side_effect = search.__get__(auth, type(auth)) + auth.thycotic_entity.side_effect = entity_impl.__get__(auth, type(auth)) + with mock.patch.object(thycotic.ThycoticMixin, 'connect_to_server', return_value=auth): + downloaded = list(thycotic.ThycoticMembershipDownload().download_membership(None)) + records = [item for item in downloaded if getattr(item, 'shares', None)] + self.assertEqual(self._shares(records[0]), { + 'owner@example.com': (True, False), + 'view@example.com': (False, False), + }) + self.assertNotIn('/v1/secret-permissions?filter.secretId=100', calls) + + def test_inheriting_secret_uses_secret_permissions_when_folder_acl_is_empty(self): + records, calls = self._download(self._secret(True), [ + {'userId': 5, 'secretId': 100, 'secretAccessRoleName': 'Owner'}, + ]) + self.assertEqual(self._shares(records[0]), { + 'owner@example.com': (True, True), + }) + self.assertIn('/v1/secret-permissions', calls) + self.assertNotIn('/v1/secret-permissions?filter.secretId=100', calls) + + def test_absent_inherit_flag_falls_back_to_folder_roles(self): + calls = [] + secret = self._secret(True) + del secret['inheritPermissions'] + folder_rows = [ + {'userId': 5, 'groupId': 0, 'folderAccessRoleId': 3, 'secretAccessRoleId': 3, 'secretAccessRoleName': 'Edit'}, + {'userId': 6, 'groupId': 0, 'folderAccessRoleId': 4, 'secretAccessRoleId': 4, 'secretAccessRoleName': 'View'}, + ] + + def search(self, endpoint): + calls.append(endpoint) + if endpoint.startswith('/v1/folders'): + return _folders() + if endpoint.startswith('/v1/folder-permissions?filter.folderId=10'): + return folder_rows + if endpoint.startswith('/v1/folder-permissions'): + return [] + if endpoint.startswith('/v1/users'): + return _users() + if endpoint.startswith('/v1/secrets/lookup'): + return [{'id': secret['id']}] + if endpoint.startswith('/v1/secret-permissions'): + return [] + if endpoint.startswith('/v1/groups'): + return [] + raise AssertionError(endpoint) + + def entity_impl(self, endpoint): + if endpoint == '/v1/secrets/100': + return secret + raise AssertionError(endpoint) + + auth = mock.Mock() + auth.thycotic_search.side_effect = search.__get__(auth, type(auth)) + auth.thycotic_entity.side_effect = entity_impl.__get__(auth, type(auth)) + with mock.patch.object(thycotic.ThycoticMixin, 'connect_to_server', return_value=auth): + downloaded = list(thycotic.ThycoticMembershipDownload().download_membership(None)) + records = [item for item in downloaded if getattr(item, 'shares', None)] + self.assertEqual(self._shares(records[0]), { + 'owner@example.com': (True, False), + 'view@example.com': (False, False), + }) + self.assertIn('/v1/secret-permissions?filter.secretId=100', calls) + + def test_missing_inherit_flag_still_pulls_secret_permissions(self): + secret = self._secret(True) + del secret['inheritPermissions'] + records, calls = self._download(secret, [ + {'userId': 5, 'userName': 'owner-login', 'secretId': 100, 'secretAccessRoleName': 'Edit'}, + ]) + self.assertEqual(self._shares(records[0]), { + 'owner@example.com': (True, False), + }) + self.assertIn('/v1/secret-permissions', calls) + self.assertNotIn('/v1/secret-permissions?filter.secretId=100', calls) + + +class TestSecretSharesRequireExistingUsers(TestCase): + def _record(self): + record = Record() + record.title = 'prod-db' + folder = Folder() + folder.path = 'Apps' + record.folders = [folder] + record.shares = [ + RecordShare('owner@example.com', True, True), + RecordShare('missing@example.com', False, False), + ] + return record + + def test_users_are_loaded_before_the_grant(self): + params = mock.Mock() + params.enterprise = None + order = [] + granted = [] + + def load_users(target): + order.append('users') + target.enterprise = {'users': [{'email': 'owner@example.com'}]} + + def grant(target, records): + order.append('grant') + granted.extend(records) + + with mock.patch.object(imp_exp.api, 'query_enterprise', side_effect=load_users), \ + mock.patch.object(imp_exp.sync_down, 'sync_down', side_effect=lambda target: order.append('sync')), \ + mock.patch.object(imp_exp, 'locate_secret_share_records', side_effect=lambda target, records: records), \ + mock.patch.object(imp_exp, 'grant_imported_record_shares', side_effect=grant): + imp_exp.apply_secret_share_records(params, [self._record()]) + + self.assertEqual(order, ['users', 'sync', 'grant']) + self.assertEqual([share.name for share in granted[0].shares], ['owner@example.com']) + + def test_failed_user_load_does_not_grant(self): + params = mock.Mock() + params.enterprise = None + with mock.patch.object(imp_exp.api, 'query_enterprise', side_effect=RuntimeError('offline')), \ + mock.patch.object(imp_exp.sync_down, 'sync_down') as sync_down, \ + mock.patch.object(imp_exp, 'grant_imported_record_shares') as grant: + imp_exp.apply_secret_share_records(params, [self._record()]) + sync_down.assert_not_called() + grant.assert_not_called() + + +def _secret_auth(secrets, secret_permissions=None, folder_permissions=None, failing_secrets=(), + failing_endpoints=()): + """Mock auth for iter_secret_shares. failing_* raise instead of returning.""" + auth = mock.Mock() + auth.calls = [] + secret_permissions = secret_permissions or {} + folder_permissions = folder_permissions or {} + + def search(endpoint): + auth.calls.append(endpoint) + for prefix in failing_endpoints: + if endpoint.startswith(prefix): + raise RuntimeError('boom') + if endpoint.startswith('/v1/secrets/lookup'): + return [{'id': secret['id']} for secret in secrets] + if endpoint == '/v1/secret-permissions': + return [dict(row, secretId=secret_id) for secret_id, rows in secret_permissions.items() for row in rows] + if endpoint.startswith('/v1/secret-permissions?filter.secretId='): + return secret_permissions.get(int(endpoint.rsplit('=', 1)[1]), []) + if endpoint.startswith('/v1/folder-permissions?filter.folderId='): + return folder_permissions.get(int(endpoint.rsplit('=', 1)[1]), []) + if endpoint.startswith('/v1/users'): + return _users() + if endpoint.startswith('/v1/groups'): + return [] + raise AssertionError(endpoint) + + def entity(endpoint): + auth.calls.append(endpoint) + secret_id = int(endpoint.rsplit('/', 1)[1]) + if secret_id in failing_secrets: + raise RuntimeError('secret {0} unavailable'.format(secret_id)) + for secret in secrets: + if secret['id'] == secret_id: + return dict(secret) + return None + + auth.thycotic_search.side_effect = search + auth.thycotic_entity.side_effect = entity + return auth + + +def _folder_map(): + return {row['id']: row for row in _folders()} + + +class TestExternalCallFailures(TestCase): + def test_failed_secret_fetch_skips_only_that_secret(self): + secrets = [ + {'id': 100, 'name': 'broken', 'folderId': 10, 'inheritPermissions': False}, + {'id': 101, 'name': 'ok', 'folderId': 10, 'inheritPermissions': False}, + ] + auth = _secret_auth(secrets, secret_permissions={ + 101: [{'userId': 5, 'secretId': 101, 'secretAccessRoleName': 'Owner'}], + }, failing_secrets=(100,)) + with self.assertLogs(level='WARNING') as logs: + records = list(thycotic.ThycoticMixin.iter_secret_shares(auth, _folder_map())) + self.assertEqual([record.title for record in records], ['ok']) + self.assertTrue(any('Secret 100 permissions skipped' in line for line in logs.output)) + self.assertTrue(any('1 secret(s) were skipped' in line for line in logs.output)) + + def test_failed_secret_lookup_yields_nothing(self): + auth = _secret_auth([], failing_endpoints=('/v1/secrets/lookup',)) + with self.assertLogs(level='WARNING'): + records = list(thycotic.ThycoticMixin.iter_secret_shares(auth, _folder_map())) + self.assertEqual(records, []) + + def test_failed_secret_permissions_falls_back_to_folder(self): + secret = {'id': 100, 'name': 'prod-db', 'folderId': 10, 'inheritPermissions': False} + auth = _secret_auth([secret], folder_permissions={ + 10: [{'userId': 6, 'secretAccessRoleName': 'View'}], + }, failing_endpoints=('/v1/secret-permissions',)) + with self.assertLogs(level='WARNING'): + rows = thycotic.ThycoticMixin.permission_rows_for_secret(auth, _folder_map(), secret) + self.assertEqual(rows, [{'userId': 6, 'secretAccessRoleName': 'View'}]) + + def test_folder_permissions_are_fetched_once_per_folder(self): + auth = _secret_auth([], folder_permissions={10: [{'userId': 5, 'secretAccessRoleName': 'Edit'}]}) + cache = thycotic.FolderPermissionCache() + for _ in range(3): + rows = thycotic.ThycoticMixin.folder_permission_rows(auth, _folder_map(), 10, cache) + self.assertEqual(len(rows), 1) + self.assertEqual(auth.calls.count('/v1/folder-permissions?filter.folderId=10'), 1) + + def test_failed_folder_fetch_is_not_cached(self): + auth = mock.Mock() + auth.thycotic_search.side_effect = [ + RuntimeError('timeout'), + [{'userId': 5, 'secretAccessRoleName': 'Edit'}], + ] + folders = {10: {'id': 10, 'parentFolderId': 0}} + cache = thycotic.FolderPermissionCache() + self.assertEqual(thycotic.ThycoticMixin.folder_permission_rows(auth, folders, 10, cache), []) + self.assertNotIn(10, cache) + rows = thycotic.ThycoticMixin.folder_permission_rows(auth, folders, 10, cache) + self.assertEqual(rows, [{'userId': 5, 'secretAccessRoleName': 'Edit'}]) + self.assertIn(10, cache) + + def test_failed_user_detail_fetch_skips_user(self): + auth = mock.Mock() + auth.thycotic_search.return_value = [ + {'id': 5, 'userName': 'no-email'}, + {'id': 6, 'userName': 'view', 'emailAddress': 'view@example.com'}, + ] + auth.thycotic_entity.side_effect = RuntimeError('403') + with self.assertLogs(level='WARNING'): + users = thycotic.ThycoticImporter()._prepare_thycotic_users(auth) + self.assertEqual([user['email'] for user in users], ['view@example.com']) + + def test_provisioned_emails_keep_user_list_when_detail_load_fails(self): + auth = FakeAuth([('/v1/users', _users())]) + with mock.patch.object(thycotic.ThycoticImporter, '_prepare_thycotic_users', + side_effect=RuntimeError('offline')): + emails = thycotic.ThycoticMixin.provisioned_emails(auth) + self.assertEqual(emails[5], 'owner@example.com') + self.assertEqual(emails[6], 'view@example.com') + + def test_user_fetch_failure_does_not_create_users(self): + importer = thycotic.ThycoticImporter() + with mock.patch.object(importer, '_prepare_thycotic_users', side_effect=RuntimeError('offline')), \ + mock.patch.object(importer, '_create_enterprise_users') as create: + with self.assertLogs(level='WARNING'): + importer._migrate_thycotic_users(mock.Mock(), mock.Mock()) + create.assert_not_called() + + +class TestCreateEnterpriseUsers(TestCase): + def _run(self, users, existing=(), created=True): + importer = thycotic.ThycoticImporter() + with mock.patch.object(importer, '_load_existing_keeper_users', + return_value=list(existing)), \ + mock.patch.object(importer, '_create_keeper_user', + return_value='uid' if created else None) as create: + report = importer._create_enterprise_users(mock.Mock(), users) + return report, create + + def _flags(self, report): + return [(row['email'], row['flag']) for row in report['users']] + + def test_existing_user_matches_case_insensitively(self): + report, create = self._run( + [{'email': 'Owner@Example.com', 'username': 'owner'}], + existing=[{'email': 'owner@example.com', 'status': 'invited'}]) + self.assertEqual(self._flags(report), [('Owner@Example.com', 'existing')]) + self.assertEqual(report['users'][0]['status'], 'invited') + create.assert_not_called() + + def test_duplicate_emails_are_created_once(self): + report, create = self._run([ + {'email': 'new@example.com', 'name': 'New'}, + {'email': 'NEW@example.com', 'name': 'New again'}, + ]) + self.assertEqual(self._flags(report), [('new@example.com', 'created')]) + create.assert_called_once() + + def test_invalid_email_is_skipped(self): + report, create = self._run([{'email': 'not-an-email', 'username': 'bad'}, {'email': ''}]) + self.assertEqual([row['flag'] for row in report['users']], ['skipped', 'skipped']) + create.assert_not_called() + + def test_failed_create_is_reported(self): + report, _create = self._run([{'email': 'new@example.com'}], created=False) + self.assertEqual(self._flags(report), [('new@example.com', 'failed')]) + self.assertEqual(report['users'][0]['reason'], 'user was not created') + + def test_name_falls_back_to_username_then_email(self): + _report, create = self._run([ + {'email': 'a@example.com', 'username': 'alpha'}, + {'email': 'b@example.com'}, + ]) + names = [call.args[2] for call in create.call_args_list] + self.assertEqual(names, ['alpha', 'b@example.com']) + + def test_non_dict_entries_are_ignored(self): + report, create = self._run([None, 'x', 42, {'email': 'ok@example.com'}]) + self.assertEqual(self._flags(report), [('ok@example.com', 'created')]) + create.assert_called_once() + + def test_failed_keeper_user_load_still_creates(self): + importer = thycotic.ThycoticImporter() + params = mock.Mock() + params.enterprise = None + with mock.patch.object(thycotic.api, 'query_enterprise', side_effect=RuntimeError('offline')), \ + mock.patch.object(importer, '_create_keeper_user', return_value='uid') as create: + with self.assertLogs(level='WARNING'): + report = importer._create_enterprise_users(params, [{'email': 'new@example.com'}]) + self.assertEqual(self._flags(report), [('new@example.com', 'created')]) + create.assert_called_once() + + def test_create_keeper_user_swallows_command_errors(self): + with mock.patch('keepercommander.commands.enterprise_create_user.CreateEnterpriseUserCommand') as command: + command.return_value.execute.side_effect = RuntimeError('license full') + with self.assertLogs(level='WARNING'): + result = thycotic.ThycoticImporter()._create_keeper_user(mock.Mock(), 'a@example.com', 'A') + self.assertIsNone(result) + + +class TestInvalidPermissionData(TestCase): + emails = {5: 'owner@example.com', 6: 'view@example.com'} + + def _shares(self, rows, groups=None): + shares = thycotic.ThycoticMixin.shares_from_secret_permissions(rows, self.emails, groups) + return {share.name: (share.can_edit, share.can_share) for share in shares} + + def test_non_list_rows_give_no_shares(self): + for rows in (None, {}, 'rows', 5): + self.assertEqual(self._shares(rows), {}) + + def test_non_dict_rows_and_unknown_roles_are_skipped(self): + self.assertEqual(self._shares([ + None, 'row', 7, + {'userId': 5, 'secretAccessRoleName': 'Approver'}, + {'userId': 5, 'secretAccessRoleName': None}, + {'userId': 6, 'secretAccessRoleName': ' view '}, + ]), {'view@example.com': (False, False)}) + + def test_user_listed_twice_keeps_higher_role(self): + self.assertEqual(self._shares([ + {'userId': 5, 'secretAccessRoleName': 'View'}, + {'userId': 5, 'secretAccessRoleName': 'Owner'}, + {'userId': 5, 'secretAccessRoleName': 'Edit'}, + ]), {'owner@example.com': (True, True)}) + + def test_unknown_user_is_dropped(self): + self.assertEqual(self._shares([{'userId': 99, 'secretAccessRoleName': 'Owner'}]), {}) + + def test_string_user_id_matches(self): + self.assertEqual(self._shares([{'userId': '6', 'secretAccessRoleName': 'Edit'}]), + {'view@example.com': (True, False)}) + + def test_row_without_user_id_uses_email_in_row(self): + self.assertEqual(self._shares([ + {'userId': 0, 'userName': ' ext@example.com ', 'secretAccessRoleName': 'Edit'}, + ]), {'ext@example.com': (True, False)}) + + def test_group_row_expands_to_members(self): + groups = {9: {'name': 'Ops', 'users': [5, 6, 99]}} + self.assertEqual(self._shares([{'groupId': '9', 'secretAccessRoleName': 'View'}], groups), { + 'owner@example.com': (False, False), + 'view@example.com': (False, False), + }) + + def test_group_row_without_lookup_is_dropped(self): + self.assertEqual(self._shares([{'groupId': 9, 'secretAccessRoleName': 'Owner'}]), {}) + + def test_extract_user_email_edge_cases(self): + extract = thycotic.ThycoticImporter._extract_user_email + self.assertEqual(extract(None), '') + self.assertEqual(extract(['a@example.com']), '') + self.assertEqual(extract({'emailAddress': ' ', 'emailaddress': 'b@example.com'}), 'b@example.com') + self.assertEqual(extract({'emailAddress': 5}), '') + self.assertEqual(extract({'emailAddress': ' a@example.com '}), 'a@example.com') + + def test_application_accounts_are_skipped(self): + auth = mock.Mock() + auth.thycotic_search.return_value = [ + {'id': 5, 'emailAddress': 'app@example.com', 'isApplicationAccount': True}, + None, + {'id': 6, 'userName': 'detail'}, + ] + auth.thycotic_entity.return_value = {'isApplicationAccount': True, 'emailAddress': 'x@example.com'} + self.assertEqual(thycotic.ThycoticImporter()._prepare_thycotic_users(auth), []) + + +class TestPermissionEdgeCases(TestCase): + def test_folder_walk_stops_on_parent_cycle(self): + auth = mock.Mock() + auth.thycotic_search.return_value = [] + folders = {10: {'id': 10, 'parentFolderId': 11}, 11: {'id': 11, 'parentFolderId': 10}} + self.assertEqual(thycotic.ThycoticMixin.folder_permission_rows(auth, folders, 10), []) + self.assertEqual(auth.thycotic_search.call_count, 2) + + def test_folder_walk_uses_nearest_parent_with_rows(self): + auth = _secret_auth([], folder_permissions={1: [{'userId': 5, 'secretAccessRoleName': 'Owner'}]}) + rows = thycotic.ThycoticMixin.folder_permission_rows(auth, _folder_map(), 11) + self.assertEqual(rows, [{'userId': 5, 'secretAccessRoleName': 'Owner'}]) + + def test_secret_without_folder_or_id_gives_no_rows(self): + auth = _secret_auth([]) + self.assertEqual(thycotic.ThycoticMixin.permission_rows_for_secret(auth, {}, {}), []) + self.assertEqual(thycotic.ThycoticMixin.permission_rows_for_secret(auth, {}, None), []) + self.assertEqual(auth.calls, []) + + def test_secret_returned_as_non_dict_is_ignored(self): + auth = _secret_auth([{'id': 100, 'name': 'x', 'folderId': 10}]) + auth.thycotic_entity.side_effect = None + auth.thycotic_entity.return_value = ['not', 'a', 'secret'] + records = list(thycotic.ThycoticMixin.iter_secret_shares(auth, _folder_map())) + self.assertEqual(records, []) + + def test_secret_without_name_is_not_yielded(self): + secret = {'id': 100, 'name': '', 'folderId': 10, 'inheritPermissions': False} + auth = _secret_auth([secret], secret_permissions={ + 100: [{'userId': 5, 'secretAccessRoleName': 'Owner'}], + }) + records = list(thycotic.ThycoticMixin.iter_secret_shares(auth, _folder_map())) + self.assertEqual(records, []) + + def test_secret_in_unknown_folder_has_no_folder_path(self): + secret = {'id': 100, 'name': 'loose', 'folderId': 404, 'inheritPermissions': False} + auth = _secret_auth([secret], secret_permissions={ + 100: [{'userId': 5, 'secretAccessRoleName': 'Edit'}], + }) + records = list(thycotic.ThycoticMixin.iter_secret_shares(auth, _folder_map())) + self.assertEqual(len(records), 1) + self.assertFalse(records[0].folders) + + def test_folder_paths_match_across_separators(self): + normalize = normalize_folder_path + self.assertEqual(normalize('\\Apps\\Child'), normalize('Apps/Child/')) + self.assertEqual(normalize(' /APPS/child '), 'apps\\child') + self.assertEqual(normalize(None), '') + self.assertEqual(normalize('\\'), '') + + def test_url_from_tenant_edge_cases(self): + url = thycotic.ThycoticMixin.url_from_tenant + self.assertEqual(url(None), '') + self.assertEqual(url(' '), '') + self.assertEqual(url('acme/'), 'https://acme.secretservercloud.com/') + self.assertEqual(url('https://ss.example.com/'), 'https://ss.example.com') + self.assertEqual(url('ss.example.com'), 'https://ss.example.com/') + + def test_import_target_without_host_is_rejected(self): + self.assertIsNone(parse_thycotic_import_target('user@')) + self.assertIsNone(parse_thycotic_import_target('user@ ')) + + +def _share_record(title, *paths): + record = Record() + record.title = title + record.folders = [] + for path in paths: + folder = Folder() + folder.path = path + record.folders.append(folder) + record.shares = [RecordShare('owner@example.com', True, True)] + return record + + +class TestShareRecordsWithMultipleFolders(TestCase): + def _download(self, downloaded, existing=None, **kwargs): + import json as json_module + import os + import tempfile + from types import SimpleNamespace + from keepercommander.importer import commands + from keepercommander.importer.json.json import KeeperJsonExporter + + directory = tempfile.mkdtemp() + file_name = os.path.join(directory, 'membership.json') + if existing: + KeeperJsonExporter().do_export(file_name, existing) + plugin = mock.Mock() + plugin.download_membership.return_value = iter(downloaded) + module = SimpleNamespace(MembershipDownload=lambda: plugin) + with mock.patch.object(commands.importlib, 'import_module', return_value=module): + commands.DownloadMembershipCommand().execute( + mock.Mock(), source='thycotic', name=file_name, **kwargs) + with open(file_name) as handle: + return json_module.load(handle).get('records') or [] + + def test_folder_prefix_applies_to_every_folder(self): + records = self._download([_share_record('prod-db', 'Apps', 'Ops\\Db', '')], folder='Imported') + self.assertEqual([folder.get('folder') for folder in records[0]['folders']], + ['Imported\\Apps', 'Imported\\Ops\\Db', 'Imported']) + + def test_empty_folder_path_becomes_prefix(self): + records = self._download([_share_record('prod-db', '')], folder='Imported') + self.assertEqual(records[0]['folders'], [{'folder': 'Imported'}]) + + def test_existing_record_is_replaced_only_when_all_folders_match(self): + existing = [ + _share_record('prod-db', 'Apps'), + _share_record('prod-db', 'Apps', 'Ops'), + ] + records = self._download([_share_record('prod-db', 'Ops', 'apps/')], existing=existing) + self.assertEqual(sorted(tuple(folder['folder'] for folder in record['folders']) for record in records), + [('Apps',), ('Ops', 'apps/')]) + + +class TestLocateShareRecordsWithMultipleFolders(TestCase): + def _params(self, folders): + import json as json_module + params = mock.Mock() + params.record_cache = {} + params.subfolder_record_cache = {} + for folder_uid, (path, titles) in folders.items(): + params.subfolder_record_cache[folder_uid] = [] + for title in titles: + record_uid = '{0}-{1}'.format(folder_uid, title) + params.record_cache[record_uid] = {'data_unencrypted': json_module.dumps({'title': title})} + params.subfolder_record_cache[folder_uid].append(record_uid) + paths = {folder_uid: path for folder_uid, (path, _titles) in folders.items()} + return params, paths + + def _locate(self, folders, records): + params, paths = self._params(folders) + with mock.patch.object(imp_exp, 'get_folder_path', side_effect=lambda _params, uid: paths[uid]): + return imp_exp.locate_secret_share_records(params, records) + + def test_record_in_second_folder_is_found(self): + located = self._locate({'f1': ('Apps', []), 'f2': ('Ops', ['prod-db'])}, + [_share_record('prod-db', 'Apps', 'Ops')]) + self.assertEqual([record.uid for record in located], ['f2-prod-db']) + + def test_record_in_several_listed_folders_is_shared_in_each(self): + located = self._locate({'f1': ('Apps', ['prod-db']), 'f2': ('Ops', ['prod-db'])}, + [_share_record('prod-db', 'Apps', 'Ops', 'apps')]) + self.assertEqual(sorted(record.uid for record in located), ['f1-prod-db', 'f2-prod-db']) + + def test_record_in_no_listed_folder_names_every_folder(self): + with self.assertLogs(level='WARNING') as logs: + located = self._locate({'f1': ('Other', ['prod-db'])}, + [_share_record('prod-db', 'Apps', 'Ops')]) + self.assertEqual(located, []) + self.assertTrue(any('"apps, ops"' in line for line in logs.output)) + + +class TestThycoticUserReport(TestCase): + def test_report_logs_counts_and_each_user(self): + report = {'users': [ + {'email': 'a@example.com', 'flag': 'created'}, + {'email': 'b@example.com', 'flag': 'existing', 'status': 'invited'}, + {'email': 'bad', 'flag': 'skipped', 'reason': 'invalid email'}, + None, + ]} + with self.assertLogs(level='INFO') as logs: + thycotic.ThycoticImporter()._print_thycotic_user_report(report) + output = '\n'.join(logs.output) + self.assertIn('created 1, existing 1, skipped 2, failed 0', output) + self.assertIn('[existing] b@example.com (invited)', output) + self.assertIn('[skipped] bad (invalid email)', output) + + def test_empty_report_logs_zero_counts(self): + with self.assertLogs(level='INFO') as logs: + thycotic.ThycoticImporter()._print_thycotic_user_report({}) + self.assertIn('created 0, existing 0, skipped 0, failed 0', '\n'.join(logs.output)) + + +class TestCustomSecretRoles(TestCase): + def _secret(self): + return {'id': 100, 'name': 'prod-db', 'folderId': 10, 'inheritPermissions': False} + + def test_custom_role_is_warned_and_known_roles_still_shared(self): + auth = _secret_auth([self._secret()], secret_permissions={100: [ + {'userId': 5, 'secretAccessRoleName': 'Owner'}, + {'userId': 6, 'secretAccessRoleName': 'Auditor'}, + {'groupId': 9, 'secretAccessRoleName': 'Rotator'}, + ]}) + with self.assertLogs(level='WARNING') as logs: + records = list(thycotic.ThycoticMixin.iter_secret_shares(auth, _folder_map())) + self.assertEqual([share.name for share in records[0].shares], ['owner@example.com']) + output = '\n'.join(logs.output) + self.assertIn('Secret "prod-db" (100): role(s) Auditor, Rotator cannot be mapped', output) + self.assertIn('1 secret(s) have custom roles that were not shared', output) + + def test_secret_with_only_custom_roles_is_warned_and_not_yielded(self): + auth = _secret_auth([self._secret()], secret_permissions={100: [ + {'userId': 5, 'secretAccessRoleName': 'Auditor'}, + {'userId': 6}, + ]}) + with self.assertLogs(level='WARNING') as logs: + records = list(thycotic.ThycoticMixin.iter_secret_shares(auth, _folder_map())) + self.assertEqual(records, []) + self.assertIn('role(s) (unnamed), Auditor cannot be mapped', '\n'.join(logs.output)) + + def test_standard_roles_log_no_custom_role_warning(self): + auth = _secret_auth([self._secret()], secret_permissions={100: [ + {'userId': 5, 'secretAccessRoleName': 'Owner'}, + {'userId': 6, 'secretAccessRoleName': 'list'}, + ]}) + with mock.patch.object(thycotic.logging, 'warning') as warning: + records = list(thycotic.ThycoticMixin.iter_secret_shares(auth, _folder_map())) + self.assertEqual(len(records[0].shares), 2) + warning.assert_not_called() + + +def _response(status, body=None, text='', headers=None, reason=''): + import json as json_module + rs = mock.Mock() + rs.status_code = status + rs.reason = reason + rs.headers = headers or {} + rs.text = text or ('' if body is None else json_module.dumps(body)) + if body is None: + rs.json.side_effect = ValueError('Expecting value: line 1 column 1 (char 0)') + else: + rs.json.return_value = body + return rs + + +class TestSecretServerErrorResponses(TestCase): + def _auth(self): + auth = thycotic.ThycoticAuth('https://ss.example.com') + auth.access_token = 'token' + auth.expires_in = 3600 + auth.last_login = 10 ** 12 + return auth + + def test_error_message_handles_json_and_non_json_bodies(self): + message = thycotic.ThycoticAuth._error_message + self.assertEqual(message(_response(403, {'message': 'Access denied'})), 'Access denied') + self.assertEqual(message(_response(400, {'error': 'invalid_grant'}), 'error'), 'invalid_grant') + self.assertEqual(message(_response(502, text='Bad Gateway', reason='Bad Gateway')), + 'HTTP 502 Bad Gateway: Bad Gateway') + self.assertEqual(message(_response(500, ['x'], reason='Internal Server Error')), + 'HTTP 500 Internal Server Error: ["x"]') + self.assertEqual(message(_response(504, reason='Gateway Timeout')), 'HTTP 504 Gateway Timeout: ') + + def test_search_raises_readable_error_for_html_page(self): + with mock.patch.object(thycotic.requests, 'get', + return_value=_response(502, text='', reason='Bad Gateway')): + with self.assertRaises(Exception) as ctx: + self._auth().thycotic_search('/v1/users') + self.assertEqual(str(ctx.exception), 'HTTP 502 Bad Gateway: ') + + def test_entity_returns_none_for_html_error(self): + with mock.patch.object(thycotic.requests, 'get', return_value=_response(503, text='')): + with self.assertLogs(level='INFO') as logs: + self.assertIsNone(self._auth().thycotic_entity('/v1/secrets/1')) + self.assertTrue(any('returned 503' in line for line in logs.output)) + + def test_login_with_non_json_400_raises_readable_error(self): + auth = thycotic.ThycoticAuth('https://ss.example.com') + on_totp = mock.Mock() + with mock.patch.object(thycotic.requests, 'post', + return_value=_response(400, text='', reason='Bad Request')): + with self.assertRaises(Exception) as ctx: + auth.authenticate('user', 'secret', on_totp=on_totp) + self.assertEqual(str(ctx.exception), 'HTTP 400 Bad Request: ') + on_totp.assert_not_called() + + def test_login_still_asks_for_totp(self): + auth = thycotic.ThycoticAuth('https://ss.example.com') + responses = [ + _response(400, {'error': 'Login failed: TOTP code required'}), + _response(200, {'access_token': 'abc', 'expires_in': 1200, 'refresh_token': 'r'}), + ] + with mock.patch.object(thycotic.requests, 'post', side_effect=responses) as post: + auth.authenticate('user', 'secret', on_totp=lambda: '123456') + self.assertEqual(auth.access_token, 'abc') + self.assertEqual(post.call_args.kwargs['data']['OTP'], '123456') + + def test_refresh_with_non_json_error_raises_readable_error(self): + auth = self._auth() + auth.refresh_token = 'r' + with mock.patch.object(thycotic.requests, 'post', + return_value=_response(500, text='oops', reason='Internal Server Error')): + with self.assertRaises(Exception) as ctx: + auth.refresh_auth_token() + self.assertEqual(str(ctx.exception), 'HTTP 500 Internal Server Error: oops') + + +class TestSecretServerRateLimit(TestCase): + def _auth(self): + auth = thycotic.ThycoticAuth('https://ss.example.com') + auth.access_token = 'token' + auth.expires_in = 3600 + auth.last_login = 10 ** 12 + return auth + + def _ok(self): + return _response(200, {'records': [{'id': 1}], 'hasNext': False}) + + def test_429_is_retried_with_retry_after(self): + responses = [_response(429, headers={'Retry-After': '12'}), self._ok()] + with mock.patch.object(thycotic.requests, 'get', side_effect=responses), \ + mock.patch.object(thycotic.time, 'sleep') as sleep: + with self.assertLogs(level='WARNING') as logs: + result = self._auth().thycotic_search('/v1/users') + self.assertEqual(result, [{'id': 1}]) + sleep.assert_called_once_with(12) + self.assertTrue(any('rate limit (attempt 1/3), retrying in 12 seconds' in line for line in logs.output)) + + def test_backoff_grows_when_no_retry_after(self): + responses = [_response(429), _response(429), _response(429), self._ok()] + with mock.patch.object(thycotic.requests, 'get', side_effect=responses), \ + mock.patch.object(thycotic.time, 'sleep') as sleep: + with self.assertLogs(level='WARNING'): + self._auth().thycotic_search('/v1/users') + self.assertEqual([call.args[0] for call in sleep.call_args_list], [5, 10, 20]) + + def test_backoff_wins_over_small_retry_after_and_large_hint_is_capped(self): + auth = self._auth() + self.assertEqual(auth._throttle_wait(_response(429, headers={'Retry-After': '1'}), 2), 10) + self.assertEqual(auth._throttle_wait(_response(429, headers={'Retry-After': '9999'}), 1), 300) + self.assertEqual(auth._throttle_wait(_response(429, headers={'Retry-After': 'Wed, 21 Oct 2026 07:28:00 GMT'}), 1), 5) + + def test_gives_up_after_max_retries(self): + responses = [_response(429, {'message': 'Too many requests'}) for _ in range(4)] + with mock.patch.object(thycotic.requests, 'get', side_effect=responses) as get, \ + mock.patch.object(thycotic.time, 'sleep') as sleep: + with self.assertLogs(level='WARNING'): + with self.assertRaises(Exception) as ctx: + self._auth().thycotic_search('/v1/users') + self.assertEqual(get.call_count, 4) + self.assertEqual(sleep.call_count, 3) + self.assertEqual(str(ctx.exception), 'Too many requests') + + def test_entity_and_attachment_fetch_also_retry(self): + auth = self._auth() + with mock.patch.object(thycotic.requests, 'get', + side_effect=[_response(429), _response(200, {'id': 7})]), \ + mock.patch.object(thycotic.time, 'sleep'): + with self.assertLogs(level='WARNING'): + self.assertEqual(auth.thycotic_entity('/v1/secrets/7'), {'id': 7}) + with mock.patch.object(thycotic.requests, 'get', + side_effect=[_response(429), _response(200, {})]) as get, \ + mock.patch.object(thycotic.time, 'sleep'): + with self.assertLogs(level='WARNING'): + self.assertEqual(auth.thycotic_get('/v1/secrets/7/fields/file').status_code, 200) + self.assertTrue(get.call_args.kwargs['stream']) + + def test_other_errors_are_not_retried(self): + with mock.patch.object(thycotic.requests, 'get', return_value=_response(403, {'message': 'denied'})) as get, \ + mock.patch.object(thycotic.time, 'sleep') as sleep: + with self.assertRaises(Exception): + self._auth().thycotic_search('/v1/users') + get.assert_called_once() + sleep.assert_not_called() + + +class TestSecretShareHelpers(TestCase): + def test_folder_path_accepts_int_or_str_ids(self): + folders = {10: {'folderPath': '\\Apps'}, '11': {'folderPath': '\\Ops'}} + path = thycotic.ThycoticMixin._secret_folder_path + self.assertEqual(path(folders, 10), '\\Apps') + self.assertEqual(path(folders, '10'), '\\Apps') + self.assertEqual(path(folders, '11'), '\\Ops') + self.assertEqual(path(folders, 404), '') + self.assertEqual(path(folders, None), '') + self.assertEqual(path(None, 10), '') + self.assertEqual(path({10: {'folderPath': None}}, 10), '') + + def test_share_record_without_folder(self): + shares = [RecordShare('owner@example.com', True, True)] + record = thycotic.ThycoticMixin._secret_share_record('prod-db', '', shares) + self.assertEqual(record.title, 'prod-db') + self.assertFalse(record.folders) + self.assertIs(record.shares, shares) + + def test_unnamed_secret_does_not_load_groups(self): + secret = {'id': 100, 'name': '', 'folderId': 10, 'inheritPermissions': False} + auth = _secret_auth([secret], secret_permissions={100: [{'groupId': 9, 'secretAccessRoleName': 'Owner'}]}) + list(thycotic.ThycoticMixin.iter_secret_shares(auth, _folder_map())) + self.assertFalse(any(call.startswith('/v1/groups') for call in auth.calls)) + + +class TestBulkSecretPermissions(TestCase): + def _secrets(self, count): + return [{'id': 100 + i, 'name': 'secret-{0}'.format(i), 'folderId': 10, 'inheritPermissions': False} + for i in range(count)] + + def test_one_bulk_call_replaces_per_secret_calls(self): + secrets = self._secrets(5) + auth = _secret_auth(secrets, secret_permissions={ + secret['id']: [{'userId': 5, 'secretAccessRoleName': 'Owner'}] for secret in secrets}) + records = list(thycotic.ThycoticMixin.iter_secret_shares(auth, _folder_map())) + self.assertEqual(len(records), 5) + self.assertEqual(auth.calls.count('/v1/secret-permissions'), 1) + self.assertFalse(any('filter.secretId' in call for call in auth.calls)) + + def test_rows_are_grouped_by_secret_and_rows_without_secret_id_dropped(self): + auth = mock.Mock() + auth.thycotic_search.return_value = [ + {'secretId': 1, 'userId': 5}, {'secretId': 2, 'userId': 6}, {'secretId': 1, 'userId': 6}, + {'userId': 7}, {'secretId': 0, 'userId': 8}, 'junk', None, + ] + grouped = thycotic.ThycoticMixin.all_secret_permission_rows(auth) + self.assertEqual(grouped, { + 1: [{'secretId': 1, 'userId': 5}, {'secretId': 1, 'userId': 6}], + 2: [{'secretId': 2, 'userId': 6}], + }) + auth.thycotic_search.assert_called_once_with('/v1/secret-permissions') + + def test_failed_bulk_call_falls_back_to_per_secret(self): + auth = mock.Mock() + auth.thycotic_search.side_effect = RuntimeError('403 Forbidden') + with self.assertLogs(level='WARNING') as logs: + self.assertIsNone(thycotic.ThycoticMixin.all_secret_permission_rows(auth)) + self.assertTrue(any('loading them per secret' in line for line in logs.output)) + + def test_empty_bulk_result_falls_back_to_per_secret(self): + secret = self._secrets(1)[0] + calls = [] + + def search(endpoint): + calls.append(endpoint) + if endpoint == '/v1/secret-permissions': + return [] + if endpoint.startswith('/v1/secret-permissions?filter.secretId=100'): + return [{'userId': 5, 'secretId': 100, 'secretAccessRoleName': 'Edit'}] + if endpoint.startswith('/v1/secrets/lookup'): + return [{'id': 100}] + if endpoint.startswith('/v1/users'): + return _users() + return [] + + auth = mock.Mock() + auth.thycotic_search.side_effect = search + auth.thycotic_entity.return_value = dict(secret) + records = list(thycotic.ThycoticMixin.iter_secret_shares(auth, _folder_map())) + self.assertEqual([share.name for share in records[0].shares], ['owner@example.com']) + self.assertIn('/v1/secret-permissions?filter.secretId=100', calls) + + def test_no_secrets_makes_no_bulk_call(self): + auth = _secret_auth([]) + self.assertEqual(list(thycotic.ThycoticMixin.iter_secret_shares(auth, _folder_map())), []) + self.assertNotIn('/v1/secret-permissions', auth.calls) + + +class TestOrderedWorkerPool(TestCase): + def test_results_keep_input_order_with_uneven_latency(self): + import random + import time as time_module + rng = random.Random(7) + delays = {i: rng.uniform(0, 0.01) for i in range(40)} + + def fetch(item): + time_module.sleep(delays[item]) + return item * 10 + + results = list(thycotic.ThycoticMixin._fetch_in_order(fetch, range(40), 4)) + self.assertEqual(results, [(i, i * 10) for i in range(40)]) + + def test_never_more_than_workers_running_or_double_queued(self): + import threading as threading_module + import time as time_module + lock = threading_module.Lock() + state = {'running': 0, 'peak': 0, 'started': 0} + consumed = [] + + def fetch(item): + with lock: + state['running'] += 1 + state['started'] += 1 + state['peak'] = max(state['peak'], state['running']) + ahead = state['started'] - len(consumed) + self.assertLessEqual(ahead, 3 * 2 + 1) + time_module.sleep(0.002) + with lock: + state['running'] -= 1 + return item + + for item, _result in thycotic.ThycoticMixin._fetch_in_order(fetch, range(30), 3): + consumed.append(item) + self.assertEqual(consumed, list(range(30))) + self.assertLessEqual(state['peak'], 3) + self.assertGreater(state['peak'], 1) + + def test_one_worker_runs_on_calling_thread(self): + import threading as threading_module + threads = set() + + def fetch(item): + threads.add(threading_module.get_ident()) + return item + + list(thycotic.ThycoticMixin._fetch_in_order(fetch, range(5), 1)) + self.assertEqual(threads, {threading_module.get_ident()}) + + def test_stopping_early_cancels_queued_work(self): + import threading as threading_module + lock = threading_module.Lock() + started = [] + + def fetch(item): + with lock: + started.append(item) + return item + + results = thycotic.ThycoticMixin._fetch_in_order(fetch, range(1000), 4) + self.assertEqual(next(results), (0, 0)) + results.close() + self.assertLess(len(started), 20) + + def test_worker_count_is_clamped(self): + workers = thycotic.ThycoticMixin.secret_share_workers + self.assertEqual(workers(None), 4) + self.assertEqual(workers(0), 1) + self.assertEqual(workers(-3), 1) + self.assertEqual(workers(8), 8) + self.assertEqual(workers(500), 16) + self.assertEqual(workers('x'), 4) + + +class TestParallelSecretShares(TestCase): + def _secrets(self, count): + return [{'id': 100 + i, 'name': 'secret-{0}'.format(i), 'folderId': 10 + (i % 2), + 'inheritPermissions': i % 3 == 0} for i in range(count)] + + def _permissions(self, secrets): + return {secret['id']: [{'userId': 5 + (secret['id'] % 2), 'secretAccessRoleName': 'Edit'}] + for secret in secrets} + + def _titles_and_shares(self, records): + return [(record.title, [(s.name, s.can_edit, s.can_share) for s in record.shares]) for record in records] + + def test_parallel_matches_sequential(self): + secrets = self._secrets(25) + folder_rows = {10: [{'userId': 5, 'secretAccessRoleName': 'Owner'}], 11: []} + sequential = list(thycotic.ThycoticMixin.iter_secret_shares( + _secret_auth(secrets, self._permissions(secrets), folder_rows), _folder_map(), workers=1)) + parallel = list(thycotic.ThycoticMixin.iter_secret_shares( + _secret_auth(secrets, self._permissions(secrets), folder_rows), _folder_map(), workers=8)) + self.assertEqual(len(sequential), 25) + self.assertEqual(self._titles_and_shares(parallel), self._titles_and_shares(sequential)) + + def test_failed_secret_is_counted_once_in_parallel(self): + secrets = self._secrets(10) + auth = _secret_auth(secrets, self._permissions(secrets), failing_secrets=(103, 107)) + with self.assertLogs(level='WARNING') as logs: + records = list(thycotic.ThycoticMixin.iter_secret_shares(auth, _folder_map(), workers=4)) + self.assertEqual(len(records), 8) + self.assertNotIn('secret-3', [record.title for record in records]) + self.assertTrue(any('2 secret(s) were skipped' in line for line in logs.output)) + + def test_folder_permissions_fetched_at_most_once_per_worker(self): + secrets = [{'id': 100 + i, 'name': 's{0}'.format(i), 'folderId': 10, 'inheritPermissions': True} + for i in range(40)] + auth = _secret_auth(secrets, folder_permissions={10: [{'userId': 5, 'secretAccessRoleName': 'View'}]}) + records = list(thycotic.ThycoticMixin.iter_secret_shares(auth, _folder_map(), workers=4)) + self.assertEqual(len(records), 40) + self.assertLessEqual(auth.calls.count('/v1/folder-permissions?filter.folderId=10'), 4) + + +class TestThreadSafeTokenRefresh(TestCase): + def test_parallel_requests_refresh_expired_token_once(self): + import threading as threading_module + import time as time_module + auth = thycotic.ThycoticAuth('https://ss.example.com') + auth.access_token = 'old' + auth.refresh_token = 'r1' + auth.expires_in = 60 + auth.last_login = 0 + refreshes = [] + + def refresh(): + refreshes.append(auth.refresh_token) + time_module.sleep(0.01) + auth.access_token = 'new' + auth.refresh_token = 'r2' + auth.last_login = int(time_module.time()) + + auth.refresh_auth_token = refresh + threads = [threading_module.Thread(target=auth.ensure_auth_token) for _ in range(8)] + for thread in threads: + thread.start() + for thread in threads: + thread.join() + self.assertEqual(refreshes, ['r1']) + self.assertEqual(auth.access_token, 'new') + + +class TestWorkersOption(TestCase): + def test_parser_accepts_range_and_rejects_others(self): + from keepercommander.commands.base import ParseError + from keepercommander.importer import commands + parser = commands.download_membership_parser + self.assertIsNone(parser.parse_args(['--source', 'thycotic']).workers) + self.assertEqual(parser.parse_args(['--source', 'thycotic', '--workers', '1']).workers, 1) + self.assertEqual(parser.parse_args(['--source', 'thycotic', '--workers', '16']).workers, 16) + for bad in ('0', '17', '-1', 'two'): + with self.subTest(bad): + with self.assertRaises(ParseError): + parser.parse_args(['--source', 'thycotic', '--workers', bad]) + + def test_workers_reach_the_plugin(self): + import os + import tempfile + from types import SimpleNamespace + from keepercommander.importer import commands + plugin = mock.Mock() + plugin.download_membership.return_value = iter([]) + module = SimpleNamespace(MembershipDownload=lambda: plugin) + file_name = os.path.join(tempfile.mkdtemp(), 'membership.json') + with mock.patch.object(commands.importlib, 'import_module', return_value=module): + commands.DownloadMembershipCommand().execute(mock.Mock(), source='thycotic', name=file_name, workers=2) + self.assertEqual(plugin.download_membership.call_args.kwargs['workers'], 2) + + +class TestFolderPermissionCache(TestCase): + def test_first_write_wins_and_empty_rows_are_cached(self): + cache = thycotic.FolderPermissionCache() + self.assertIsNone(cache.get(10)) + self.assertNotIn(10, cache) + first = [{'userId': 5}] + self.assertIs(cache.put(10, first), first) + self.assertIs(cache.put(10, [{'userId': 6}]), first) + self.assertIs(cache.get(10), first) + self.assertEqual(cache.put(11, []), []) + self.assertEqual(cache.get(11), []) + self.assertIn(11, cache) + + def test_concurrent_writers_agree_on_one_value(self): + import threading as threading_module + cache = thycotic.FolderPermissionCache() + barrier = threading_module.Barrier(16) + seen = [] + lock = threading_module.Lock() + + def writer(index): + barrier.wait() + for folder_id in range(200): + stored = cache.put(folder_id, [index]) + with lock: + seen.append((folder_id, id(stored))) + + threads = [threading_module.Thread(target=writer, args=(i,)) for i in range(16)] + for thread in threads: + thread.start() + for thread in threads: + thread.join() + by_folder = {} + for folder_id, stored_id in seen: + by_folder.setdefault(folder_id, set()).add(stored_id) + self.assertTrue(all(len(ids) == 1 for ids in by_folder.values())) + self.assertTrue(all(cache.get(folder_id) is not None for folder_id in range(200))) + + def test_parallel_shares_use_the_locked_cache(self): + secrets = [{'id': 100 + i, 'name': 's{0}'.format(i), 'folderId': 10, 'inheritPermissions': True} + for i in range(30)] + auth = _secret_auth(secrets, folder_permissions={10: [{'userId': 5, 'secretAccessRoleName': 'Edit'}]}) + with mock.patch.object(thycotic, 'FolderPermissionCache', wraps=thycotic.FolderPermissionCache) as cache_class: + records = list(thycotic.ThycoticMixin.iter_secret_shares(auth, _folder_map(), workers=4)) + cache_class.assert_called_once_with() + self.assertEqual(len(records), 30) + + +class TestUnnestedHelpers(TestCase): + def test_secret_permission_rows_prefers_bulk_result(self): + auth = mock.Mock() + rows = thycotic.ThycoticMixin._secret_permission_rows(auth, 100, {100: [{'userId': 5}]}) + self.assertEqual(rows, [{'userId': 5}]) + self.assertEqual(thycotic.ThycoticMixin._secret_permission_rows(auth, 101, {100: []}), []) + auth.thycotic_search.assert_not_called() + + def test_secret_permission_rows_per_secret_and_failures(self): + auth = mock.Mock() + auth.thycotic_search.return_value = [{'userId': 6}] + self.assertEqual(thycotic.ThycoticMixin._secret_permission_rows(auth, 100), [{'userId': 6}]) + auth.thycotic_search.assert_called_once_with('/v1/secret-permissions?filter.secretId=100') + auth.thycotic_search.side_effect = RuntimeError('boom') + with self.assertLogs(level='WARNING'): + self.assertEqual(thycotic.ThycoticMixin._secret_permission_rows(auth, 100), []) + self.assertEqual(thycotic.ThycoticMixin._secret_permission_rows(auth, None), []) + + def test_fetch_secret_entry_never_raises(self): + auth = mock.Mock() + auth.thycotic_entity.side_effect = RuntimeError('down') + secret, rows, error = thycotic.ThycoticMixin._fetch_secret_entry( + auth, {}, thycotic.FolderPermissionCache(), None, ({'id': 1}, 1)) + self.assertIsNone(secret) + self.assertEqual(rows, []) + self.assertIsInstance(error, RuntimeError) + + def test_user_outcome_row(self): + outcome = thycotic.ThycoticImporter._user_outcome + self.assertEqual(outcome({'username': 'a', 'email': ' a@x.com '}, 'created'), + {'username': 'a', 'email': 'a@x.com', 'flag': 'created'}) + self.assertEqual(outcome({'email': 'b@x.com'}, 'existing', status='invited'), + {'username': '', 'email': 'b@x.com', 'flag': 'existing', 'status': 'invited'}) + + def test_share_record_key_uses_all_folders(self): + from keepercommander.importer import commands + key = commands._share_record_key + self.assertEqual(key(_share_record('Prod', 'Apps', 'ops/')), key(_share_record('prod', 'ops', '\\Apps'))) + self.assertNotEqual(key(_share_record('prod', 'Apps')), key(_share_record('prod', 'Apps', 'Ops'))) + self.assertEqual(key(_share_record('prod')), (('',), 'prod')) + + +class TestLocateLogsSkippedRecords(TestCase): + def test_duplicate_record_under_same_path_is_logged_and_matched_once(self): + import json as json_module + params = mock.Mock() + params.record_cache = {'r1': {'data_unencrypted': json_module.dumps({'title': 'prod-db'})}} + params.subfolder_record_cache = {'f1': ['r1'], 'f2': ['r1']} + with mock.patch.object(imp_exp, 'get_folder_path', return_value='Apps'): + with self.assertLogs(level='INFO') as logs: + located = imp_exp.locate_secret_share_records(params, [_share_record('prod-db', 'Apps')]) + self.assertEqual([record.uid for record in located], ['r1']) + self.assertTrue(any('Record r1 is listed more than once under "apps"' in line for line in logs.output)) + + def test_unreadable_records_are_counted(self): + import json as json_module + params = mock.Mock() + params.record_cache = { + 'ok': {'data_unencrypted': json_module.dumps({'title': 'prod-db'})}, + 'bad': {'data_unencrypted': b'\xff not json'}, + } + params.subfolder_record_cache = {'f1': ['ok', 'bad', 'missing']} + with mock.patch.object(imp_exp, 'get_folder_path', return_value='Apps'): + with self.assertLogs(level='WARNING') as logs: + located = imp_exp.locate_secret_share_records(params, [_share_record('prod-db', 'Apps')]) + self.assertEqual([record.uid for record in located], ['ok']) + self.assertTrue(any('2 Keeper record(s) could not be read' in line for line in logs.output)) + + def test_clean_index_logs_nothing_extra(self): + import json as json_module + params = mock.Mock() + params.record_cache = {'r1': {'data_unencrypted': json_module.dumps({'title': 'prod-db'})}} + params.subfolder_record_cache = {'f1': ['r1']} + with mock.patch.object(imp_exp, 'get_folder_path', return_value='Apps'), \ + mock.patch.object(imp_exp.logging, 'warning') as warning, \ + mock.patch.object(imp_exp.logging, 'info') as info: + located = imp_exp.locate_secret_share_records(params, [_share_record('prod-db', 'Apps')]) + self.assertEqual(len(located), 1) + warning.assert_not_called() + info.assert_not_called() From d0bb1b32b37615b6292c7234f931292275bb0433 Mon Sep 17 00:00:00 2001 From: mkanadi-ks Date: Fri, 30 Oct 2026 14:53:15 +0530 Subject: [PATCH 5/6] MM-27 added team creation and user assignment functionality in delinea (#2477) * MM-27 Added `"add-teams"` flag to support creating keeper teams in delinea import Added test cases for add teams feature * MM-27 added a function which was missed while resolving merge conflicts * MM-27 Updated logic of check if user already exists in keeper team. Updated test cases accordingly. * add url func call --------- Co-authored-by: ajaiswal-ks --- keepercommander/commands/helpers/team.py | 68 ++ keepercommander/importer/commands.py | 3 + keepercommander/importer/imp_exp.py | 7 +- keepercommander/importer/importer.py | 8 +- keepercommander/importer/thycotic/thycotic.py | 523 +++++++++++-- unit-tests/test_thycotic_add_teams.py | 729 ++++++++++++++++++ unit-tests/test_thycotic_add_users.py | 3 +- 7 files changed, 1295 insertions(+), 46 deletions(-) create mode 100644 keepercommander/commands/helpers/team.py create mode 100644 unit-tests/test_thycotic_add_teams.py diff --git a/keepercommander/commands/helpers/team.py b/keepercommander/commands/helpers/team.py new file mode 100644 index 000000000..a42d984c8 --- /dev/null +++ b/keepercommander/commands/helpers/team.py @@ -0,0 +1,68 @@ +from ... import api, crypto, utils + + +class MissingTeamEncryptionKey(ValueError): + """The enterprise tree key or data key needed to create a team is missing.""" + + +def generate_team_add_key_fields(team_key, allow_rsa=True): + # type: (bytes, bool) -> dict + """Generate the EC (and, when allowed, RSA) key fields for a team_add request. + + The EC private key is encrypted with AES-v2 and the RSA private key with AES-v1, + both under ``team_key``. Every value is base64url-encoded. Returned names match + the request fields: ``ecc_private_key``, ``ecc_public_key``, and when RSA is + allowed ``private_key`` and ``public_key``. + """ + ec_private_key, ec_public_key = crypto.generate_ec_key() + fields = { + 'ecc_private_key': utils.base64_url_encode( + crypto.encrypt_aes_v2(crypto.unload_ec_private_key(ec_private_key), team_key)), + 'ecc_public_key': utils.base64_url_encode(crypto.unload_ec_public_key(ec_public_key)), + } + if allow_rsa: + rsa_private_key, rsa_public_key = crypto.generate_rsa_key() + fields['private_key'] = utils.base64_url_encode( + crypto.encrypt_aes_v1(crypto.unload_rsa_private_key(rsa_private_key), team_key)) + fields['public_key'] = utils.base64_url_encode(crypto.unload_rsa_public_key(rsa_public_key)) + return fields + + +def build_team_add_request(params, team_name, node_id, + restrict_edit=False, restrict_share=False, restrict_view=False, + manage_only=True, team_uid=None, team_key=None): + # type: (object, str, int, bool, bool, bool, bool, str, bytes) -> tuple + """Build a team_add request and return ``(request, team_key)``. + + ``team_uid`` and the team AES key are generated when the caller does not + supply them. The team key is encrypted with the data key (AES-v1) and the + enterprise tree key (AES-v2). RSA key fields are omitted when + ``params.forbid_rsa`` is set. Raises MissingTeamEncryptionKey when the tree + key or data key is missing. The returned team key is the raw AES key, for + callers that still need it to add members. + """ + tree_key = (getattr(params, 'enterprise', None) or {}).get('unencrypted_tree_key') + data_key = getattr(params, 'data_key', None) + if not tree_key or not data_key: + raise MissingTeamEncryptionKey('Enterprise tree key or data key is not available') + + if team_uid is None: + team_uid = api.generate_record_uid() + if team_key is None: + team_key = api.generate_aes_key() + + request = { + 'command': 'team_add', + 'team_uid': team_uid, + 'team_name': team_name, + 'restrict_edit': restrict_edit, + 'restrict_share': restrict_share, + 'restrict_view': restrict_view, + 'node_id': node_id, + 'team_key': utils.base64_url_encode(crypto.encrypt_aes_v1(team_key, data_key)), + 'encrypted_team_key': utils.base64_url_encode(crypto.encrypt_aes_v2(team_key, tree_key)), + 'manage_only': manage_only, + } + request.update(generate_team_add_key_fields( + team_key, allow_rsa=not getattr(params, 'forbid_rsa', False))) + return request, team_key \ No newline at end of file diff --git a/keepercommander/importer/commands.py b/keepercommander/importer/commands.py index d065c8776..268f02c04 100644 --- a/keepercommander/importer/commands.py +++ b/keepercommander/importer/commands.py @@ -136,6 +136,9 @@ def _positive_int_arg(value): help='Comma separated list of secret IDs to fetch (Thycotic)') import_parser.add_argument('--add-users', dest='add_users', action='store_true', help='provision source users as pre-activated Keeper enterprise users') +import_parser.add_argument('--add-teams', dest='add_teams', action='store_true', + help='create Keeper teams from source groups and add members ' + 'who already exist as Keeper users') import_parser.add_argument('--target-node', '--node', dest='target_node', action='store', help='node name or ID for CyberArk-provisioned users, teams, and roles (default: root node)') import_parser.add_argument('--skip', dest='skip', action='store', type=_cyberark_skip_arg, diff --git a/keepercommander/importer/imp_exp.py b/keepercommander/importer/imp_exp.py index c54eda32c..d6e7d7813 100644 --- a/keepercommander/importer/imp_exp.py +++ b/keepercommander/importer/imp_exp.py @@ -852,6 +852,7 @@ def _import(params, file_format, filename, **kwargs): skip_role = kwargs.get('skip_role') is True or 'role' in cyberark_skip skip_user = kwargs.get('skip_user') is True or 'user' in cyberark_skip add_users = kwargs.get('add_users') is True + add_teams = kwargs.get('add_teams') is True import_into_raw = kwargs.get('import_into') or '' import_into = import_into_raw @@ -864,6 +865,9 @@ def _import(params, file_format, filename, **kwargs): if add_users and not importer.support_add_users(): logging.warning('--add-users is not supported for --format=%s; ignoring', file_format) add_users = False + if add_teams and not importer.support_add_teams(): + logging.warning('--add-teams is not supported for --format=%s; ignoring', file_format) + add_teams = False verbose_import_summary = importer.verbose_import_summary show_skipped = show_skipped or verbose_import_summary @@ -884,7 +888,8 @@ def _import(params, file_format, filename, **kwargs): old_domain=old_domain, new_domain=new_domain, tmpdir=tmpdir, secret_ids=secret_ids, dry_run=dry_run, target_node=target_node, use_nsf=use_nsf, record_type=record_type, skip_team=skip_team, skip_role=skip_role, skip_user=skip_user, - add_users=add_users): + add_users=add_users, + add_teams=add_teams): if isinstance(x, ImportRecord): if filter_folder and not importer.support_folder_filter(): if not x.folders: diff --git a/keepercommander/importer/importer.py b/keepercommander/importer/importer.py index 99ee7ac07..304403125 100644 --- a/keepercommander/importer/importer.py +++ b/keepercommander/importer/importer.py @@ -352,6 +352,12 @@ def support_add_users(self): """ return False + def support_add_teams(self): + """ + Indicates if this importer can create Keeper teams from source groups + """ + return False + @staticmethod def import_field(field_type, field_value): # type: (str, str) -> any if not field_value: @@ -594,4 +600,4 @@ def download_membership(self, params, **kwargs): # type: (KeeperParams, ...) class BaseDownloadRecordType(abc.ABC): @abc.abstractmethod def download_record_type(self, params, **kwargs): # type: (KeeperParams, ...) -> Iterable[RecordType] - pass + pass \ No newline at end of file diff --git a/keepercommander/importer/thycotic/thycotic.py b/keepercommander/importer/thycotic/thycotic.py index 591bf335d..66a2b0a2a 100644 --- a/keepercommander/importer/thycotic/thycotic.py +++ b/keepercommander/importer/thycotic/thycotic.py @@ -25,6 +25,7 @@ from collections import deque from concurrent.futures import ThreadPoolExecutor from contextlib import contextmanager +from dataclasses import dataclass from urllib3.exceptions import InsecureRequestWarning import xml.etree.ElementTree as ET @@ -172,38 +173,33 @@ def url_from_tenant(tenant): # type: (Optional[str]) -> str return 'https://{0}/'.format(tenant) @staticmethod - def expand_shell_value(value, envvars): # type: (str, Optional[dict]) -> Optional[str] - """Replace ${NAME} from the Commander shell. Do not print the result.""" - from keepercommander.commands.base import expand_cmd_args, parameter_pattern - expanded = expand_cmd_args(value or '', envvars or {}) - missing = parameter_pattern.findall(expanded) - if missing: - logging.warning('Shell variable is not set: %s', ', '.join(missing)) - return None - return expanded - - @staticmethod - def connect_to_server(url=None, envvars=None): # type: (Optional[str], Optional[dict]) -> ThycoticAuth + def connect_to_server(url=None, username=None, password=None): # type: (Optional[str], Optional[str], Optional[str]) -> ThycoticAuth + url = url or os.getenv('KEEPER_THYCOTIC_URL') + username = username or os.getenv('KEEPER_THYCOTIC_USER') + password = password or os.getenv('KEEPER_THYCOTIC_PASSWORD') + if not url: url = input('...' + 'Thycotic Host or URL'.rjust(30) + ': ') if not url: logging.warning('Thycotic Host or URL is required') return - url = ThycoticMixin.expand_shell_value(url, envvars) - if not url: - return url = ThycoticMixin.url_from_tenant(url) if not url.startswith('http'): url = f'https://{url}' - username = input('...' + 'Thycotic Username'.rjust(30) + ': ') + if not username: - logging.warning('Thycotic username is required') - return - password = getpass.getpass(prompt='...' + 'Thycotic Password'.rjust(30) + ': ', stream=None) + username = input('...' + 'Thycotic Username'.rjust(30) + ': ') + if not username: + logging.warning('Thycotic username is required') + return + if not password: - logging.warning('Thycotic password is required') - return + password = getpass.getpass( + prompt='...' + 'Thycotic Password'.rjust(30) + ': ', stream=None) + if not password: + logging.warning('Thycotic password is required') + return def request_totp(): return input('...' + 'Enter TOTP Code'.rjust(30) + ': ') @@ -212,6 +208,7 @@ def request_totp(): auth.authenticate(username, password, on_totp=request_totp) return auth + @staticmethod def get_user_lookup(auth): # type: (ThycoticAuth) -> Optional[Dict[int, Dict]] try: @@ -237,14 +234,12 @@ def get_group_lookup(auth, on_progress=None): g = { 'name': group['name'], 'users': [], # type: List[int] - 'groups': [], # type: List[int] } if group.get('memberCount', 0) > 0: member_rs = auth.thycotic_search(f'/v1/groups/{group_id}/users') if on_progress: on_progress(i, total) g['users'].extend((x['userId'] for x in member_rs if 'userId' in x and x['userId'] > 0)) - g['groups'].extend((x['groupId'] for x in member_rs if 'groupId' in x and x['groupId'] > 0)) lookup[group_id] = g return lookup except: @@ -519,7 +514,7 @@ def get_folders(auth, skip_permissions=False): folder['inheritPermissions'] = True return folders - + @staticmethod def _secret_role_flags(role): @@ -952,6 +947,17 @@ def parse_thycotic_import_target(filename): return None return username, host +# Team create and member-add requests carry encrypted keys, so each execute payload stays small. +_TEAM_BATCH_SIZE = 25 + +@dataclass +class TeamMemberCounts: + """Import summary of how members were applied to one Keeper team.""" + added: int = 0 + queued: int = 0 + already: int = 0 + missing: int = 0 + failed: int = 0 class ThycoticImporter(BaseImporter, ThycoticMixin): # Same end-of-import terminal summary CyberArk prints (Import finished, skipped rows). @@ -964,10 +970,13 @@ def __init__(self): def support_folder_filter(self): return True - + def support_add_users(self): return True - + + def support_add_teams(self): + return True + @staticmethod def pop_field(fields, key): # type: (Dict[str, Dict[str, str]], str) -> Tuple[str, Optional[Dict[str, str]]] field = fields.pop(key, None) @@ -1013,11 +1022,12 @@ def _extract_user_email(user): return value.strip() return '' - def _prepare_thycotic_users(self, auth): # type: (ThycoticAuth) -> List[dict] + def _prepare_thycotic_users(self, auth, quiet=False): # type: (ThycoticAuth, bool) -> List[dict] """Fetch Delinea users that have an email address, including inactive users. Application accounts are omitted. When the user list has no email, the user detail endpoint is tried once. + quiet suppresses the summary line used while resolving group members. """ all_users = auth.thycotic_search('/v1/users?filter.includeInactive=true') result = [] @@ -1196,8 +1206,436 @@ def _migrate_thycotic_users(self, auth, params): logging.warning('Failed to fetch Thycotic users: %s', e) logging.warning('Continuing import without creating Keeper users.') return - report = self._create_enterprise_users(params, thycotic_users) - self._print_thycotic_user_report(report) + self._create_enterprise_users(params, thycotic_users) + + def _load_thycotic_groups(self, auth): # type: (ThycoticAuth) -> list + return auth.thycotic_search('/v1/groups') + + def _load_thycotic_user_email_lookup(self, auth): # type: (ThycoticAuth) -> Dict[int, str] + """Map Thycotic user ids to emails. A user-fetch failure continues with no emails.""" + try: + users = self._prepare_thycotic_users(auth, quiet=True) + except Exception as e: + logging.warning('Failed to fetch Thycotic users for group membership: %s', e) + users = [] + email_by_id = {} + for user in users or []: + if not isinstance(user, dict): + continue + user_id = user.get('id') + email = str(user.get('email') or '').strip() + if isinstance(user_id, int) and user_id > 0 and email: + email_by_id[user_id] = email + return email_by_id + + @staticmethod + def _select_thycotic_groups(groups): # type: (list) -> Tuple[List[dict], int, int] + """Keep enabled groups with an int id and a non-blank name, and count the skips.""" + included = [] + skipped_disabled = 0 + skipped_unnamed = 0 + for group in groups or []: + if not isinstance(group, dict): + continue + if group.get('enabled') is False: + skipped_disabled += 1 + continue + group_id = group.get('id') + name = str(group.get('name') or '').strip() + if not isinstance(group_id, int) or not name: + skipped_unnamed += 1 + continue + included.append({ + 'id': group_id, + 'name': name, + 'member_count': group.get('memberCount'), + }) + return included, skipped_disabled, skipped_unnamed + + @staticmethod + def _fetch_thycotic_group_member_ids(auth, group_id, name, member_count): + # type: (ThycoticAuth, int, str, Any) -> List + """Return direct member user ids. A failed member fetch aborts preparation.""" + user_ids = [] + if (member_count or 0) > 0: + try: + member_rs = auth.thycotic_search(f'/v1/groups/{group_id}/users') + except Exception as e: + raise Exception( + f'Failed to fetch members of Thycotic group "{name}": {e}' + ) from e + for member in member_rs or []: + if not isinstance(member, dict): + continue + user_ids.append(member.get('userId')) + return user_ids + + @staticmethod + def _map_thycotic_group_member_emails(name, user_ids, email_by_id): + # type: (str, list, Dict[int, str]) -> List[str] + member_emails = {} + unresolved = 0 + for user_id in user_ids: + email = email_by_id.get(user_id) or '' + if not email: + unresolved += 1 + continue + member_emails.setdefault(email.casefold(), email) + if unresolved: + logging.info( + 'Group "%s": %d member(s) have no email address and will not be added', + name, unresolved, + ) + return [member_emails[key] for key in sorted(member_emails)] + + def _assemble_thycotic_group_results(self, lookup, email_by_id, skipped_disabled, skipped_unnamed): + # type: (Dict[int, Dict], Dict[int, str], int, int) -> List[dict] + result = [] + for group_id, entry in lookup.items(): + result.append({ + 'id': group_id, + 'name': entry['name'], + 'members': self._map_thycotic_group_member_emails( + entry['name'], entry['users'], email_by_id, + ), + }) + if skipped_disabled: + logging.info('Skipped %d disabled Thycotic group(s)', skipped_disabled) + if skipped_unnamed: + logging.info('Skipped %d Thycotic group(s) with no name', skipped_unnamed) + logging.info(f'Found {len(result)} Thycotic group(s)') + return result + + def _prepare_thycotic_groups(self, auth): # type: (ThycoticAuth) -> List[dict] + """Fetch enabled Delinea groups and their direct user members. + Disabled groups are omitted. A failed member fetch aborts preparation + so Keeper teams are not created from an incomplete member list. + """ + groups = self._load_thycotic_groups(auth) + email_by_id = self._load_thycotic_user_email_lookup(auth) + included, skipped_disabled, skipped_unnamed = self._select_thycotic_groups(groups) + lookup = {} # type: Dict[int, Dict] + for group in included: + lookup[group['id']] = { + 'name': group['name'], + 'users': self._fetch_thycotic_group_member_ids( + auth, group['id'], group['name'], group['member_count'], + ), + } + return self._assemble_thycotic_group_results( + lookup, email_by_id, skipped_disabled, skipped_unnamed, + ) + + def _load_existing_keeper_teams(self, params): + # type: (KeeperParams) -> Tuple[Dict[str, dict], Dict[str, set], str] + """Return teams keyed by exact stripped name, member user ids keyed by team uid, and an error string.""" + try: + api.query_enterprise(params) + except Exception as e: + if not isinstance(getattr(params, 'enterprise', None), dict): + logging.warning('Error loading existing Keeper teams: %s', e) + return {}, {}, 'Failed to load existing Keeper teams' + logging.warning('Could not refresh Keeper teams: %s', e) + enterprise_data = getattr(params, 'enterprise', None) + if not isinstance(enterprise_data, dict): + return {}, {}, 'Failed to load existing Keeper teams' + + teams = {} + for source in ('queued_teams', 'teams'): + for team in enterprise_data.get(source) or []: + if not isinstance(team, dict) or not team.get('team_uid'): + continue + name = str(team.get('name') or '').strip() + if name: + teams[name] = team + + members = {} # type: Dict[str, set] + for team_user in enterprise_data.get('team_users') or []: + if not isinstance(team_user, dict): + continue + team_uid = team_user.get('team_uid') + user_id = team_user.get('enterprise_user_id') + if team_uid and user_id: + members.setdefault(team_uid, set()).add(user_id) + for queued in enterprise_data.get('queued_team_users') or []: + if not isinstance(queued, dict): + continue + team_uid = queued.get('team_uid') + if not team_uid: + continue + for user_id in queued.get('users') or []: + if user_id: + members.setdefault(team_uid, set()).add(user_id) + return teams, members, '' + + def _resolve_root_node_id(self, params): # type: (KeeperParams) -> Optional[int] + from ...commands.enterprise_common import EnterpriseCommand + try: + root_nodes = list(EnterpriseCommand.get_user_root_nodes(params)) + except Exception as e: + logging.warning('Failed to resolve the Keeper root node: %s', e) + return None + if not root_nodes: + logging.warning('No Keeper root node is available for new teams') + return None + return root_nodes[0] + + def _build_team_add_request(self, params, name, node_id): + # type: (KeeperParams, str, int) -> Tuple[dict, bytes] + """Build a team_add request in the same shape as enterprise-team --add.""" + from ...commands.helpers.team import build_team_add_request + return build_team_add_request(params, name, node_id) + + def _load_team_key(self, params, team_uid): # type: (KeeperParams, str) -> Optional[bytes] + try: + api.load_team_keys(params, [team_uid]) + except Exception as e: + logging.warning('Failed to load the key for Keeper team %s: %s', team_uid, e) + return None + keys = (getattr(params, 'key_cache', None) or {}).get(team_uid) + return getattr(keys, 'aes', None) or None + + def _add_team_members(self, params, team_uid, team_name, team_key, emails, already_member_ids, is_real_team): + # type: (KeeperParams, str, str, Optional[bytes], List[str], set, bool) -> TeamMemberCounts + """Add Keeper users to a team. Active users of a real team are keyed; everyone else is queued.""" + from ...commands.enterprise import EnterpriseTeamCommand + + users_by_email = {} + for user in ((getattr(params, 'enterprise', None) or {}).get('users') or []): + if not isinstance(user, dict): + continue + email = str(user.get('username') or user.get('email') or '').strip() + if email and user.get('enterprise_user_id'): + users_by_email.setdefault(email.casefold(), user) + + already = set(already_member_ids or []) + active_users = [] + queued_users = [] + missing = [] + already_count = 0 + for email in emails or []: + if not isinstance(email, str) or not email.strip(): + continue + user = users_by_email.get(email.strip().casefold()) + if not user: + missing.append(email.strip()) + continue + user_id = user.get('enterprise_user_id') + if user_id in already: + already_count += 1 + continue + already.add(user_id) + if is_real_team and user.get('status') == 'active': + active_users.append(user) + else: + queued_users.append(user) + + if missing: + logging.warning( + 'Team "%s": skipped %d member(s) who are not Keeper users', + team_name, len(missing), + ) + + if active_users and not team_key: + team_key = self._load_team_key(params, team_uid) + failed = 0 + if active_users and not team_key: + logging.warning('Cannot add active members to team "%s": team key is not available', team_name) + failed += len(active_users) + active_users = [] + + requests = [] + for user in queued_users: + requests.append({ + 'command': 'team_queue_user', + 'team_uid': team_uid, + 'enterprise_user_id': user.get('enterprise_user_id'), + }) + for user in active_users: + request = EnterpriseTeamCommand._create_add_user_request(params, team_uid, team_key, user, '') + if request: + requests.append(request) + else: + failed += 1 + + added = 0 + queued = 0 + for offset in range(0, len(requests), _TEAM_BATCH_SIZE): + chunk = requests[offset:offset + _TEAM_BATCH_SIZE] + try: + responses = api.execute_batch(params, chunk) or [] + except Exception as e: + logging.warning('Failed to add members to Keeper team "%s": %s', team_name, e) + failed += len(chunk) + continue + for index, request in enumerate(chunk): + response = responses[index] if index < len(responses) else None + # result "success" with an empty result_code is a completed add or queue. + succeeded = isinstance(response, dict) and response.get('result') == 'success' + if succeeded: + if request.get('command') == 'team_queue_user': + queued += 1 + else: + added += 1 + continue + failed += 1 + message = (response or {}).get('message') if isinstance(response, dict) else response + if request.get('command') == 'team_queue_user': + logging.warning('Failed to queue a member on team "%s": %s', team_name, message) + else: + logging.warning('Failed to add a member to team "%s": %s', team_name, message) + return TeamMemberCounts( + added=added, + queued=queued, + already=already_count, + missing=len(missing), + failed=failed, + ) + + def _create_enterprise_teams(self, params, groups): + # type: (KeeperParams, List[dict]) -> None + from ...commands.helpers.enterprise import is_valid_name_length + from ...commands.helpers.team import MissingTeamEncryptionKey + + existing_teams, members_by_team, error = self._load_existing_keeper_teams(params) + if error: + logging.warning(error) + logging.warning('Continuing import without creating Keeper teams.') + return + + pending = [] + existing = [] + seen = set() + for group in groups or []: + if not isinstance(group, dict): + continue + name = str(group.get('name') or '').strip() + # A case-only difference (Test123 vs test123) must still be attempted. + if not name or name in seen: + continue + seen.add(name) + members = [ + email.strip() for email in (group.get('members') or []) + if isinstance(email, str) and email.strip() + ] + team = existing_teams.get(name) + if team: + existing.append((team, members)) + continue + if not is_valid_name_length(name, 'Team name', 'import'): + continue + pending.append((name, members)) + + created = [] # type: List[Tuple[str, str, bytes, List[str]]] + if pending: + node_id = self._resolve_root_node_id(params) + tree_key = (getattr(params, 'enterprise', None) or {}).get('unencrypted_tree_key') + if node_id is None or not tree_key or not getattr(params, 'data_key', None): + logging.warning('Cannot create Keeper teams: root node or enterprise keys are not available') + else: + batch = [] + batch_meta = [] + for name, members in pending: + try: + request, team_key = self._build_team_add_request(params, name, node_id) + except MissingTeamEncryptionKey as e: + # The helper rejects every team when these keys are missing. + logging.warning('%s', e) + batch.clear() + batch_meta.clear() + break + except Exception as e: + logging.warning('Could not prepare Keeper team "%s": %s', name, e) + continue + batch.append(request) + batch_meta.append((name, request['team_uid'], team_key, members)) + for offset in range(0, len(batch), _TEAM_BATCH_SIZE): + chunk = batch[offset:offset + _TEAM_BATCH_SIZE] + chunk_meta = batch_meta[offset:offset + _TEAM_BATCH_SIZE] + try: + responses = api.execute_batch(params, chunk) or [] + except Exception as e: + logging.warning('Failed to create Keeper teams: %s', e) + responses = [] + for index, meta in enumerate(chunk_meta): + name, team_uid, team_key, members = meta + response = responses[index] if index < len(responses) else None + if isinstance(response, dict) and response.get('result') == 'success': + created.append(meta) + logging.info('\'%s\' team is created', name) + else: + message = response.get('message') if isinstance(response, dict) else response + logging.warning('\'%s\' team is not created: %s', name, message or 'no response') + + rows = [] + for name, team_uid, team_key, members in created: + counts = self._add_team_members( + params, team_uid, name, team_key, members, set(), True) + rows.append(self._team_result_row(name, 'created', counts)) + for team, members in existing: + team_uid = team.get('team_uid') + team_name = team.get('name') or team_uid + is_real_team = 'restrict_edit' in team + counts = self._add_team_members( + params, team_uid, team_name, None, members, + members_by_team.get(team_uid) or set(), is_real_team) + rows.append(self._team_result_row(team_name, 'already exists', counts)) + if rows: + logging.info(tabulate(rows, headers='keys')) + + @staticmethod + def _team_result_row(name, result, counts): + # type: (str, str, TeamMemberCounts) -> dict + return { + 'Team': name, + 'Result': result, + 'Added': counts.added, + 'Queued': counts.queued, + 'Already on team': counts.already, + 'Not in Keeper': counts.missing, + 'Failed': counts.failed, + } + + def _preview_thycotic_teams(self, auth): + # type: (ThycoticAuth) -> None + """Fetch Delinea groups and print them. Does not create Keeper teams.""" + logging.info('Dry run: fetching Thycotic groups (no Keeper teams will be created)...') + try: + groups = self._prepare_thycotic_groups(auth) + except Exception as e: + logging.warning('Failed to fetch Thycotic groups: %s', e) + return + if not groups: + logging.info('No Thycotic groups are available to migrate.') + return + rows = [ + { + 'Group': group.get('name') or '', + 'Members': ', '.join(group.get('members') or []) or '-', + } + for group in groups + ] + logging.info(f'Thycotic groups available to migrate ({len(rows)}):') + logging.info(tabulate(rows, headers='keys')) + + def _migrate_thycotic_teams(self, auth, params): + # type: (ThycoticAuth, KeeperParams) -> None + """Create Keeper teams from Delinea groups and add members who already exist in Keeper. + + A group-fetch failure does not abort secret migration. + Run this after user provisioning so newly created users can be added. + """ + logging.info('Migrating Thycotic groups to Keeper teams...') + try: + groups = self._prepare_thycotic_groups(auth) + except Exception as e: + logging.warning('Failed to fetch Thycotic groups: %s', e) + logging.warning('Continuing import without creating Keeper teams.') + return + if not groups: + logging.info('No Thycotic groups are available to migrate.') + return + self._create_enterprise_teams(params, groups) def do_import(self, filename, **kwargs): # type: (ThycoticImporter, str, ...) -> Iterable[Union[Record, SharedFolder]] @@ -1246,6 +1684,13 @@ def do_import(self, filename, **kwargs): else: self._migrate_thycotic_users(auth, params) + # Teams are created after users so members provisioned above can be added. + if kwargs.get('add_teams') is True: + if kwargs.get('dry_run') is True: + self._preview_thycotic_teams(auth) + else: + self._migrate_thycotic_teams(auth, params) + totp_codes = {} csv_file = 'secrets-export.csv' if not os.path.isfile('secrets-export.csv'): @@ -1329,7 +1774,7 @@ def do_import(self, filename, **kwargs): found_secrets = [x for x in stringified_debug_ids if x in stringified_secrets_ids] logging.info(f'From the specified {len(debug_ids)} secret IDs, {len(found_secrets)} were found in the secret server lookup.') logging.info(', '.join(found_secrets)) - + # Replace import list with specified IDs secrets_ids = debug_ids else: @@ -2000,8 +2445,7 @@ def prepare(self): class ThycoticMembershipDownload(BaseDownloadMembership, ThycoticMixin): def download_membership(self, params, **kwargs): - envvars = getattr(params, 'environment_variables', None) if params is not None else None - auth = ThycoticMixin.connect_to_server(envvars=envvars) + auth = ThycoticMixin.connect_to_server() if not auth: return @@ -2012,12 +2456,7 @@ def download_membership(self, params, **kwargs): for folder in folders.values(): if folder.get('inheritPermissions', True): continue - if 'permissions' not in folder: - continue - permissions = folder['permissions'] - if not isinstance(permissions, list): - continue - if len(permissions) == 0: + if 'permissions' not in folder or not isinstance(folder['permissions'], list): continue shared_folder = SharedFolder() @@ -2053,8 +2492,7 @@ def download_membership(self, params, **kwargs): sf_groups.add(group_id) yield shared_folder - folders_only = kwargs.get('folders_only') is True - if folders_only is True: + if kwargs.get('folders_only') is True: return if len(sf_groups) > 0: @@ -2091,8 +2529,7 @@ def progress(position, total): class ThycoticRecordTypeDownload(BaseDownloadRecordType, ThycoticMixin): def download_record_type(self, params, **kwargs): - envvars = getattr(params, 'environment_variables', None) if params is not None else None - auth = ThycoticMixin.connect_to_server(envvars=envvars) + auth = ThycoticMixin.connect_to_server() if not auth: return templates = auth.thycotic_search('/v1/secret-templates?filter.includeInactive=false&filter.includeSecretCount=true') @@ -2330,4 +2767,4 @@ def download_record_type(self, params, **kwargs): rf = RecordTypeField.create('fileRef', '') rt.fields.append(rf) - yield rt \ No newline at end of file + yield rt diff --git a/unit-tests/test_thycotic_add_teams.py b/unit-tests/test_thycotic_add_teams.py new file mode 100644 index 000000000..14eae65e2 --- /dev/null +++ b/unit-tests/test_thycotic_add_teams.py @@ -0,0 +1,729 @@ +import logging +import unittest +from types import SimpleNamespace +from unittest import mock + +from keepercommander.commands.helpers.team import MissingTeamEncryptionKey +from keepercommander.importer.commands import import_parser +from keepercommander.importer.thycotic.thycotic import ( + TeamMemberCounts, ThycoticImporter, _TEAM_BATCH_SIZE, +) + + +class _QuietTestCase(unittest.TestCase): + def setUp(self): + self._patch('builtins.print') + self._patch('keepercommander.importer.thycotic.thycotic.logging') + self.log = self._mocks['logging'] + + def _patch(self, target, **kwargs): + patcher = mock.patch(target, **kwargs) + name = target.rsplit('.', 1)[-1] + self.__dict__.setdefault('_mocks', {})[name] = patcher.start() + self.addCleanup(patcher.stop) + + def importer(self): + return ThycoticImporter() + + +class TestPrepareThycoticGroups(_QuietTestCase): + def _groups(self, groups, members=None, users=None, user_error=None): + auth = mock.MagicMock() + + def search(endpoint): + endpoint = str(endpoint) + if endpoint == '/v1/groups': + return groups + return (members or {}).get(endpoint, []) + + auth.thycotic_search.side_effect = search + importer = self.importer() + if user_error is not None: + importer._prepare_thycotic_users = mock.Mock(side_effect=user_error) + else: + importer._prepare_thycotic_users = mock.Mock(return_value=users or []) + found = importer._prepare_thycotic_groups(auth) + return found, importer + + def test_uses_direct_members_and_skips_disabled(self): + found, importer = self._groups( + [ + {'id': 10, 'name': 'Admins', 'enabled': True, 'memberCount': 2}, + {'id': 11, 'name': 'Empty', 'enabled': True, 'memberCount': 0}, + {'id': 12, 'name': 'Disabled', 'enabled': False, 'memberCount': 1}, + {'id': 13, 'name': ' Nested ', 'enabled': True, 'memberCount': 1}, + {'name': 'no-id', 'memberCount': 1}, + 'junk', + ], + members={ + '/v1/groups/10/users': [ + {'userId': 1, 'groupId': 0}, + {'userId': 2}, + {'userId': 9}, + ], + '/v1/groups/13/users': [{'groupId': 10, 'userId': 0}, {'userId': 2}], + }, + users=[ + {'id': 1, 'email': 'a@x.com'}, + {'id': 2, 'email': 'b@x.com'}, + ], + ) + importer._prepare_thycotic_users.assert_called_once_with(mock.ANY, quiet=True) + self.assertEqual( + [(g['name'], g['members']) for g in found], + [ + ('Admins', ['a@x.com', 'b@x.com']), + ('Empty', []), + ('Nested', ['b@x.com']), + ], + ) + self.log.info.assert_called() + + def test_same_names_stay_separate_groups(self): + found, _ = self._groups( + [ + {'id': 1, 'name': 'Ops', 'memberCount': 1}, + {'id': 2, 'name': 'ops', 'memberCount': 1}, + ], + members={ + '/v1/groups/1/users': [{'userId': 1}], + '/v1/groups/2/users': [{'userId': 2}], + }, + users=[{'id': 1, 'email': 'a@x.com'}, {'id': 2, 'email': 'B@x.com'}], + ) + self.assertEqual(found, [ + {'id': 1, 'name': 'Ops', 'members': ['a@x.com']}, + {'id': 2, 'name': 'ops', 'members': ['B@x.com']}, + ]) + + def test_member_fetch_failure_stops_preparation(self): + auth = mock.MagicMock() + error = RuntimeError('403') + + def search(endpoint): + endpoint = str(endpoint) + if endpoint == '/v1/groups': + return [ + {'id': 1, 'name': 'Admins', 'memberCount': 1}, + {'id': 3, 'name': 'Ops', 'memberCount': 4}, + ] + if endpoint == '/v1/groups/1/users': + return [{'userId': 1}] + raise error + + auth.thycotic_search.side_effect = search + importer = self.importer() + importer._prepare_thycotic_users = mock.Mock(return_value=[ + {'id': 1, 'email': 'a@x.com'}, + ]) + importer._create_enterprise_teams = mock.Mock() + + with self.assertRaises(Exception) as caught: + importer._prepare_thycotic_groups(auth) + self.assertIn('Ops', str(caught.exception)) + self.assertIn('403', str(caught.exception)) + self.assertIs(caught.exception.__cause__, error) + + importer._migrate_thycotic_teams(auth, object()) + importer._create_enterprise_teams.assert_not_called() + self.log.warning.assert_any_call( + 'Continuing import without creating Keeper teams.', + ) + fetch_failures = [ + call.args[1] for call in self.log.warning.call_args_list + if call.args and call.args[0] == 'Failed to fetch Thycotic groups: %s' + ] + self.assertTrue(fetch_failures) + self.assertIn('Ops', str(fetch_failures[-1])) + self.assertIn('403', str(fetch_failures[-1])) + + importer._preview_thycotic_teams(auth) + importer._create_enterprise_teams.assert_not_called() + info = ' '.join(str(call.args[0]) for call in self.log.info.call_args_list) + self.assertNotIn('Thycotic groups available to migrate', info) + + def test_user_lookup_failure_still_returns_groups(self): + found, _ = self._groups( + [{'id': 3, 'name': 'Ops', 'memberCount': 0}], + user_error=RuntimeError('users down'), + ) + self.assertEqual(found[0]['name'], 'Ops') + self.log.warning.assert_called() + + +class TestLoadExistingKeeperTeams(_QuietTestCase): + def setUp(self): + super().setUp() + self._patch('keepercommander.importer.thycotic.thycotic.api.query_enterprise') + self.query = self._mocks['query_enterprise'] + + def test_active_team_replaces_queued_team_of_the_same_name(self): + params = SimpleNamespace(enterprise={ + 'queued_teams': [{'name': 'Ops', 'team_uid': 'queued'}], + 'teams': [ + {'name': ' Ops ', 'team_uid': 'real', 'restrict_edit': False}, + {'name': 'ops', 'team_uid': 'lower', 'restrict_edit': False}, + {'name': 'Empty', 'team_uid': ''}, + ], + 'team_users': [ + {'team_uid': 'real', 'enterprise_user_id': 7}, + {'team_uid': 'real', 'enterprise_user_id': None}, + 'junk', + ], + 'queued_team_users': [{'team_uid': 'queued', 'users': [8, 0]}], + }) + teams, members, error = self.importer()._load_existing_keeper_teams(params) + self.assertEqual(error, '') + self.assertEqual(teams['Ops']['team_uid'], 'real') + self.assertEqual(teams['ops']['team_uid'], 'lower') + self.assertEqual(members, {'real': {7}, 'queued': {8}}) + self.query.assert_called_once() + + def test_query_failure_returns_error(self): + self.query.side_effect = RuntimeError('not an admin') + teams, members, error = self.importer()._load_existing_keeper_teams(SimpleNamespace(enterprise=None)) + self.assertEqual((teams, members, bool(error)), ({}, {}, True)) + + +class TestBuildTeamAddRequest(_QuietTestCase): + def _params(self, **extra): + params = { + 'enterprise': {'unencrypted_tree_key': b'tree'}, + 'data_key': b'data', + 'forbid_rsa': False, + } + params.update(extra) + return SimpleNamespace(**params) + + def _patches(self): + targets = { + 'keepercommander.commands.helpers.team.api.generate_record_uid': {'return_value': 'uid-1'}, + 'keepercommander.commands.helpers.team.api.generate_aes_key': {'return_value': b'k' * 32}, + 'keepercommander.commands.helpers.team.crypto.encrypt_aes_v1': {'return_value': b'v1'}, + 'keepercommander.commands.helpers.team.crypto.encrypt_aes_v2': {'return_value': b'v2'}, + 'keepercommander.commands.helpers.team.crypto.generate_ec_key': {'return_value': ('ec-priv', 'ec-pub')}, + 'keepercommander.commands.helpers.team.crypto.unload_ec_private_key': {'return_value': b'ec-priv'}, + 'keepercommander.commands.helpers.team.crypto.unload_ec_public_key': {'return_value': b'ec-pub'}, + 'keepercommander.commands.helpers.team.crypto.generate_rsa_key': {'return_value': ('rsa-priv', 'rsa-pub')}, + 'keepercommander.commands.helpers.team.crypto.unload_rsa_private_key': {'return_value': b'rsa-priv'}, + 'keepercommander.commands.helpers.team.crypto.unload_rsa_public_key': {'return_value': b'rsa-pub'}, + 'keepercommander.commands.helpers.team.utils.base64_url_encode': { + 'side_effect': lambda value: 'b64-' + value.decode()}, + } + started = {} + for target, kwargs in targets.items(): + patcher = mock.patch(target, **kwargs) + started[target.rsplit('.', 1)[-1]] = patcher.start() + self.addCleanup(patcher.stop) + return started + + def test_request_matches_enterprise_team_add(self): + self._patches() + request, team_key = self.importer()._build_team_add_request(self._params(), 'Admins', 9) + self.assertEqual(team_key, b'k' * 32) + self.assertEqual(request['command'], 'team_add') + self.assertEqual(request['team_uid'], 'uid-1') + self.assertEqual(request['team_name'], 'Admins') + self.assertEqual(request['node_id'], 9) + self.assertTrue(request['manage_only']) + self.assertIn('public_key', request) + self.assertIn('private_key', request) + self.assertIn('ecc_public_key', request) + + def test_rsa_keys_omitted_when_forbidden(self): + patched = self._patches() + request, _ = self.importer()._build_team_add_request(self._params(forbid_rsa=True), 'Admins', 9) + self.assertNotIn('public_key', request) + self.assertNotIn('private_key', request) + patched['generate_rsa_key'].assert_not_called() + + def test_missing_keys_raise(self): + with self.assertRaises(MissingTeamEncryptionKey) as caught: + self.importer()._build_team_add_request(SimpleNamespace(enterprise={}, data_key=None), 'Admins', 9) + self.assertIsInstance(caught.exception, ValueError) + self.assertEqual(str(caught.exception), 'Enterprise tree key or data key is not available') + + +class TestAddTeamMembers(_QuietTestCase): + def _params(self): + return SimpleNamespace(enterprise={'users': [ + {'username': 'A@x.com', 'enterprise_user_id': 1, 'status': 'active'}, + {'username': 'b@x.com', 'enterprise_user_id': 2, 'status': 'invited'}, + {'username': 'c@x.com', 'enterprise_user_id': 3, 'status': 'active'}, + ]}) + + def test_active_users_are_added_and_invited_users_are_queued(self): + importer = self.importer() + with mock.patch( + 'keepercommander.commands.enterprise.EnterpriseTeamCommand._create_add_user_request', + return_value={'command': 'team_enterprise_user_add', 'enterprise_user_id': 1}, + ) as add_user, mock.patch( + 'keepercommander.importer.thycotic.thycotic.api.execute_batch', + return_value=[{'result': 'success'}, {'result': 'success'}], + ) as batch: + counts = importer._add_team_members( + self._params(), 'uid', 'Ops', b'key', + ['a@x.com', 'b@x.com', 'c@x.com', 'missing@x.com', ''], + {3}, True, + ) + self.assertEqual(counts, TeamMemberCounts(added=1, queued=1, already=1, missing=1, failed=0)) + queued, active = batch.call_args[0][1] + self.assertEqual(queued['command'], 'team_queue_user') + self.assertEqual(queued['enterprise_user_id'], 2) + self.assertEqual(active['command'], 'team_enterprise_user_add') + add_user.assert_called_once() + self.log.warning.assert_called() + + def test_queued_team_does_not_encrypt_a_team_key(self): + importer = self.importer() + with mock.patch( + 'keepercommander.commands.enterprise.EnterpriseTeamCommand._create_add_user_request', + ) as add_user, mock.patch( + 'keepercommander.importer.thycotic.thycotic.api.execute_batch', + return_value=[{'result': 'success'}], + ) as batch: + counts = importer._add_team_members( + self._params(), 'uid', 'Ops', None, ['a@x.com'], set(), False, + ) + add_user.assert_not_called() + self.assertEqual(batch.call_args[0][1][0]['command'], 'team_queue_user') + self.assertEqual(counts.queued, 1) + + def test_captured_success_counts_as_added_and_duplicate_comes_from_membership(self): + importer = self.importer() + captured = { + 'result': 'success', + 'result_code': '', + 'message': '', + 'command': 'team_enterprise_user_add', + 'revision': 4027138144, + } + queued_success = { + 'result': 'success', + 'result_code': '', + 'message': '', + 'command': 'team_queue_user', + 'revision': 4027138145, + } + with mock.patch( + 'keepercommander.commands.enterprise.EnterpriseTeamCommand._create_add_user_request', + return_value={'command': 'team_enterprise_user_add', 'enterprise_user_id': 1}, + ), mock.patch( + 'keepercommander.importer.thycotic.thycotic.api.execute_batch', + side_effect=[ + [captured], + [queued_success], + [{ + 'result': 'fail', + 'result_code': 'exists', + 'message': 'The object you are trying to create already exists.', + }], + [{ + 'result': 'fail', + 'message': 'The object you are trying to create already exists.', + }], + ], + ) as batch: + added = importer._add_team_members( + self._params(), 'uid', 'Ops', b'key', ['a@x.com'], set(), True, + ) + queued = importer._add_team_members( + self._params(), 'uid', 'Ops', None, ['b@x.com'], set(), False, + ) + coded_failure = importer._add_team_members( + self._params(), 'uid', 'Ops', b'key', ['a@x.com'], set(), True, + ) + message_failure = importer._add_team_members( + self._params(), 'uid', 'Ops', b'key', ['a@x.com'], set(), True, + ) + already = importer._add_team_members( + self._params(), 'uid', 'Ops', b'key', ['a@x.com'], {1}, True, + ) + self.assertEqual(added, TeamMemberCounts(added=1, queued=0, already=0, missing=0, failed=0)) + self.assertEqual(queued, TeamMemberCounts(added=0, queued=1, already=0, missing=0, failed=0)) + self.assertEqual(coded_failure, TeamMemberCounts(added=0, queued=0, already=0, missing=0, failed=1)) + self.assertEqual(message_failure, TeamMemberCounts(added=0, queued=0, already=0, missing=0, failed=1)) + self.assertEqual(already, TeamMemberCounts(added=0, queued=0, already=1, missing=0, failed=0)) + self.assertEqual(batch.call_count, 4) + self.assertEqual( + [call.args[0] for call in self.log.warning.call_args_list], + [ + 'Failed to add a member to team "%s": %s', + 'Failed to add a member to team "%s": %s', + ], + ) + + def test_missing_team_key_counts_as_failed(self): + importer = self.importer() + importer._load_team_key = mock.Mock(return_value=None) + with mock.patch('keepercommander.importer.thycotic.thycotic.api.execute_batch') as batch: + counts = importer._add_team_members( + self._params(), 'uid', 'Ops', None, ['a@x.com'], set(), True, + ) + batch.assert_not_called() + self.assertEqual(counts.failed, 1) + self.log.warning.assert_called() + + def test_member_adds_are_chunked_and_counts_stay_aligned(self): + self.assertEqual(_TEAM_BATCH_SIZE, 25) + importer = self.importer() + params = SimpleNamespace(enterprise={'users': [ + {'username': 'q1@x.com', 'enterprise_user_id': 11, 'status': 'invited'}, + {'username': 'q2@x.com', 'enterprise_user_id': 12, 'status': 'invited'}, + {'username': 'q3@x.com', 'enterprise_user_id': 13, 'status': 'invited'}, + {'username': 'q4@x.com', 'enterprise_user_id': 14, 'status': 'invited'}, + {'username': 'a1@x.com', 'enterprise_user_id': 21, 'status': 'active'}, + {'username': 'a2@x.com', 'enterprise_user_id': 22, 'status': 'active'}, + {'username': 'a3@x.com', 'enterprise_user_id': 23, 'status': 'active'}, + ]}) + + def add_user(_params, _team_uid, _team_key, user, _role): + return { + 'command': 'team_enterprise_user_add', + 'enterprise_user_id': user.get('enterprise_user_id'), + } + + chunks = [] + + def execute(_params, chunk): + chunks.append(list(chunk)) + if len(chunks) == 1: + return [ + {'result': 'success', 'result_code': '', 'message': ''}, + {'result': 'fail', 'result_code': 'exists', 'message': ''}, + ] + if len(chunks) == 2: + raise RuntimeError('timeout') + if len(chunks) == 3: + return [{'result': 'success'}] + return [{'result': 'success'}] + + with mock.patch( + 'keepercommander.commands.enterprise.EnterpriseTeamCommand._create_add_user_request', + side_effect=add_user, + ), mock.patch( + 'keepercommander.importer.thycotic.thycotic._TEAM_BATCH_SIZE', 2, + ), mock.patch( + 'keepercommander.importer.thycotic.thycotic.api.execute_batch', + side_effect=execute, + ) as batch: + counts = importer._add_team_members( + params, 'uid', 'Ops', b'key', + ['q1@x.com', 'a1@x.com', 'q2@x.com', 'q3@x.com', 'a2@x.com', 'q4@x.com', 'a3@x.com'], + set(), True, + ) + + self.assertEqual(batch.call_count, 4) + self.assertEqual( + [[request['enterprise_user_id'] for request in chunk] for chunk in chunks], + [[11, 12], [13, 14], [21, 22], [23]], + ) + self.assertTrue(all(len(chunk) <= 2 for chunk in chunks)) + self.assertEqual(counts, TeamMemberCounts(added=2, queued=1, already=0, missing=0, failed=4)) + warnings = [call.args[0] for call in self.log.warning.call_args_list] + self.assertEqual(warnings.count('Failed to add members to Keeper team "%s": %s'), 1) + self.assertEqual(warnings.count('Failed to queue a member on team "%s": %s'), 1) + self.assertEqual(warnings.count('Failed to add a member to team "%s": %s'), 1) + + +class TestCreateEnterpriseTeams(_QuietTestCase): + def _importer(self, existing=None, error=''): + importer = self.importer() + importer._load_existing_keeper_teams = mock.Mock(return_value=(existing or {}, {}, error)) + importer._resolve_root_node_id = mock.Mock(return_value=5) + importer._build_team_add_request = mock.Mock( + return_value=({'command': 'team_add', 'team_uid': 'uid-1'}, b'key')) + importer._add_team_members = mock.Mock(return_value=TeamMemberCounts( + added=1, queued=0, already=0, missing=0, failed=0, + )) + return importer + + def _params(self): + return SimpleNamespace(enterprise={'unencrypted_tree_key': b'tree'}, data_key=b'data') + + def test_creates_a_team_then_adds_members(self): + importer = self._importer() + with mock.patch( + 'keepercommander.importer.thycotic.thycotic.api.execute_batch', + return_value=[{'result': 'success'}, {'result': 'success'}], + ) as batch: + importer._create_enterprise_teams(self._params(), [ + {'name': 'Admins', 'members': [' a@x.com ']}, + {'name': 'admins', 'members': ['other@x.com']}, + {'name': ' Admins ', 'members': ['dup@x.com']}, + {'name': ' ', 'members': ['x@x.com']}, + None, + ]) + batch.assert_called_once() + created = batch.call_args[0][1] + self.assertEqual([request['command'] for request in created], ['team_add', 'team_add']) + self.assertEqual( + [call.args[1] for call in importer._build_team_add_request.call_args_list], + ['Admins', 'admins'], + ) + self.assertEqual( + [call.args[2] for call in importer._add_team_members.call_args_list], + ['Admins', 'admins'], + ) + self.assertEqual(importer._add_team_members.call_args_list[0].args[4], ['a@x.com']) + self.assertEqual(importer._add_team_members.call_args_list[1].args[4], ['other@x.com']) + + def test_existing_team_is_not_created_and_gains_missing_members(self): + importer = self._importer(existing={ + 'Ops': {'name': 'Ops', 'team_uid': 'uid-ops', 'restrict_edit': False}, + }) + importer._load_existing_keeper_teams.return_value = ( + {'Ops': {'name': 'Ops', 'team_uid': 'uid-ops', 'restrict_edit': False}}, + {'uid-ops': {4}}, + '', + ) + with mock.patch('keepercommander.importer.thycotic.thycotic.api.execute_batch') as batch: + importer._create_enterprise_teams(self._params(), [{'name': ' Ops ', 'members': ['a@x.com']}]) + batch.assert_not_called() + importer._build_team_add_request.assert_not_called() + importer._add_team_members.assert_called_once_with( + mock.ANY, 'uid-ops', 'Ops', None, ['a@x.com'], {4}, True) + + def test_case_distinct_group_is_created_beside_existing_team(self): + importer = self._importer() + importer._load_existing_keeper_teams.return_value = ( + {'Ops': {'name': 'Ops', 'team_uid': 'uid-ops', 'restrict_edit': False}}, + {'uid-ops': {4}}, + '', + ) + with mock.patch( + 'keepercommander.importer.thycotic.thycotic.api.execute_batch', + return_value=[{'result': 'success'}], + ) as batch: + importer._create_enterprise_teams(self._params(), [ + {'name': 'ops', 'members': ['new@x.com']}, + {'name': 'Ops', 'members': ['old@x.com']}, + ]) + importer._build_team_add_request.assert_called_once_with(mock.ANY, 'ops', 5) + self.assertEqual(len(batch.call_args[0][1]), 1) + self.assertEqual( + [(call.args[2], call.args[4]) for call in importer._add_team_members.call_args_list], + [('ops', ['new@x.com']), ('Ops', ['old@x.com'])], + ) + + def test_team_creates_are_chunked_and_a_failed_chunk_does_not_stop_later_teams(self): + importer = self._importer() + names = ['TeamA', 'TeamB', 'TeamC', 'TeamD', 'TeamE'] + + def build(_params, name, _node_id): + return ({'command': 'team_add', 'team_uid': 'uid-' + name, 'team_name': name}, b'key') + + importer._build_team_add_request.side_effect = build + chunks = [] + + def execute(_params, chunk): + chunks.append(list(chunk)) + if len(chunks) == 1: + raise RuntimeError('timeout') + if len(chunks) == 2: + return [{'result': 'success'}] + return [{'result': 'success'}] + + groups = [{'name': name, 'members': [name.lower() + '@x.com']} for name in names] + with mock.patch( + 'keepercommander.importer.thycotic.thycotic._TEAM_BATCH_SIZE', 2, + ), mock.patch( + 'keepercommander.importer.thycotic.thycotic.api.execute_batch', + side_effect=execute, + ) as batch: + importer._create_enterprise_teams(self._params(), groups) + + self.assertEqual(batch.call_count, 3) + self.assertEqual( + [[request['team_name'] for request in chunk] for chunk in chunks], + [['TeamA', 'TeamB'], ['TeamC', 'TeamD'], ['TeamE']], + ) + self.assertTrue(all(len(chunk) <= 2 for chunk in chunks)) + self.assertEqual( + [(call.args[1], call.args[2], call.args[4]) for call in importer._add_team_members.call_args_list], + [('uid-TeamC', 'TeamC', ['teamc@x.com']), ('uid-TeamE', 'TeamE', ['teame@x.com'])], + ) + warnings = [call.args[0] for call in self.log.warning.call_args_list] + self.assertEqual(warnings.count('Failed to create Keeper teams: %s'), 1) + self.assertNotIn('Continuing import without creating Keeper teams.', warnings) + self.assertEqual(warnings.count('\'%s\' team is not created: %s'), 3) + + def test_missing_encryption_key_stops_remaining_teams(self): + importer = self._importer() + + def build(_params, name, _node_id): + if name == 'Admins': + raise MissingTeamEncryptionKey('Enterprise tree key or data key is not available') + return ({'command': 'team_add', 'team_uid': 'uid-' + name, 'team_name': name}, b'key') + + importer._build_team_add_request.side_effect = build + with mock.patch('keepercommander.importer.thycotic.thycotic.api.execute_batch') as batch: + importer._create_enterprise_teams(self._params(), [ + {'name': 'Admins', 'members': ['a@x.com']}, + {'name': 'Ops', 'members': ['b@x.com']}, + {'name': 'Dev', 'members': ['c@x.com']}, + ]) + self.assertEqual( + [call.args[1] for call in importer._build_team_add_request.call_args_list], + ['Admins'], + ) + batch.assert_not_called() + importer._add_team_members.assert_not_called() + self.assertEqual(self.log.warning.call_count, 1) + warning_format, warning_error = self.log.warning.call_args.args + self.assertEqual(warning_format, '%s') + self.assertIsInstance(warning_error, MissingTeamEncryptionKey) + self.assertEqual(str(warning_error), 'Enterprise tree key or data key is not available') + + def test_other_prepare_error_skips_only_that_team(self): + importer = self._importer() + + def build(_params, name, _node_id): + if name == 'Admins': + raise ValueError('team uid could not be generated') + return ({'command': 'team_add', 'team_uid': 'uid-' + name, 'team_name': name}, b'key') + + importer._build_team_add_request.side_effect = build + with mock.patch( + 'keepercommander.importer.thycotic.thycotic.api.execute_batch', + return_value=[{'result': 'success'}], + ) as batch: + importer._create_enterprise_teams(self._params(), [ + {'name': 'Admins', 'members': ['a@x.com']}, + {'name': 'Ops', 'members': ['b@x.com']}, + ]) + self.assertEqual( + [call.args[1] for call in importer._build_team_add_request.call_args_list], + ['Admins', 'Ops'], + ) + self.assertEqual( + [request['team_name'] for request in batch.call_args[0][1]], + ['Ops'], + ) + importer._add_team_members.assert_called_once() + self.assertEqual(importer._add_team_members.call_args.args[2], 'Ops') + self.assertEqual(importer._add_team_members.call_args.args[4], ['b@x.com']) + self.assertEqual(self.log.warning.call_count, 1) + warning_format, team_name, warning_error = self.log.warning.call_args.args + self.assertEqual(warning_format, 'Could not prepare Keeper team "%s": %s') + self.assertEqual(team_name, 'Admins') + self.assertIsInstance(warning_error, ValueError) + self.assertNotIsInstance(warning_error, MissingTeamEncryptionKey) + self.assertEqual(str(warning_error), 'team uid could not be generated') + + def test_failed_create_does_not_add_members(self): + importer = self._importer() + with mock.patch( + 'keepercommander.importer.thycotic.thycotic.api.execute_batch', + return_value=[{'result': 'fail', 'message': 'exists'}], + ): + importer._create_enterprise_teams(self._params(), [{'name': 'Admins', 'members': ['a@x.com']}]) + importer._add_team_members.assert_not_called() + self.log.warning.assert_called() + + def test_loader_error_skips_team_creation(self): + importer = self._importer(error='Failed to load existing Keeper teams') + with mock.patch('keepercommander.importer.thycotic.thycotic.api.execute_batch') as batch: + importer._create_enterprise_teams(self._params(), [{'name': 'Admins', 'members': []}]) + batch.assert_not_called() + importer._add_team_members.assert_not_called() + + def test_missing_root_node_skips_create(self): + importer = self._importer() + importer._resolve_root_node_id.return_value = None + with mock.patch('keepercommander.importer.thycotic.thycotic.api.execute_batch') as batch: + importer._create_enterprise_teams(self._params(), [{'name': 'Admins', 'members': []}]) + batch.assert_not_called() + self.log.warning.assert_called() + + +class TestMigrateTeamsDoesNotAbortImport(_QuietTestCase): + def test_fetch_failure_is_logged_and_swallowed(self): + importer = self.importer() + auth = mock.MagicMock() + auth.thycotic_search.side_effect = RuntimeError('401') + importer._migrate_thycotic_teams(auth, object()) + self.assertGreaterEqual(self.log.warning.call_count, 2) + + +class TestDoImportTeamGate(_QuietTestCase): + def _run(self, **kwargs): + importer = self.importer() + auth = mock.MagicMock() + auth.thycotic_search.return_value = [] + params = mock.Mock() + params.rest_context.proxies = None + with mock.patch('keepercommander.importer.thycotic.thycotic.getpass.getpass', return_value='pw'), \ + mock.patch('keepercommander.importer.thycotic.thycotic.ThycoticAuth', return_value=auth), \ + mock.patch.object(ThycoticImporter, 'get_folders', return_value={}), \ + mock.patch.object(importer, '_migrate_thycotic_users') as migrate_users, \ + mock.patch.object(importer, '_preview_thycotic_users') as preview_users, \ + mock.patch.object(importer, '_migrate_thycotic_teams') as migrate_teams, \ + mock.patch.object(importer, '_preview_thycotic_teams') as preview_teams, \ + mock.patch('os.path.isfile', return_value=False): + list(importer.do_import('user@secret.example.com', params=params, **kwargs)) + return migrate_users, preview_users, migrate_teams, preview_teams + + def test_flag_creates_teams(self): + _, _, migrate, preview = self._run(add_teams=True) + migrate.assert_called_once() + preview.assert_not_called() + + def test_flag_absent_does_not_create_teams(self): + _, _, migrate, preview = self._run() + migrate.assert_not_called() + preview.assert_not_called() + + def test_dry_run_prints_groups_without_creating_them(self): + _, _, migrate, preview = self._run(add_teams=True, dry_run=True) + migrate.assert_not_called() + preview.assert_called_once() + + def test_users_are_provisioned_before_teams(self): + importer = self.importer() + order = [] + auth = mock.MagicMock() + params = mock.Mock() + params.rest_context.proxies = None + with mock.patch('keepercommander.importer.thycotic.thycotic.getpass.getpass', return_value='pw'), \ + mock.patch('keepercommander.importer.thycotic.thycotic.ThycoticAuth', return_value=auth), \ + mock.patch.object(ThycoticImporter, 'get_folders', return_value={}), \ + mock.patch.object(importer, '_migrate_thycotic_users', + side_effect=lambda *args, **kwargs: order.append('users')), \ + mock.patch.object(importer, '_migrate_thycotic_teams', + side_effect=lambda *args, **kwargs: order.append('teams')), \ + mock.patch('os.path.isfile', return_value=False): + list(importer.do_import( + 'user@secret.example.com', params=params, + add_users=True, add_teams=True, + )) + self.assertEqual(order, ['users', 'teams']) + + def test_preview_prints_groups_without_creating_teams(self): + importer = self.importer() + importer._prepare_thycotic_groups = mock.Mock(return_value=[ + {'name': 'Ops', 'members': ['a@x.com']}, + ]) + importer._create_enterprise_teams = mock.Mock() + importer._preview_thycotic_teams(mock.Mock()) + printed = ' '.join(str(call) for call in self.log.info.call_args_list) + self.assertIn('Thycotic groups available to migrate', printed) + self.assertIn('Ops', printed) + self.assertIn('a@x.com', printed) + importer._create_enterprise_teams.assert_not_called() + + +class TestImportParser(unittest.TestCase): + def test_flag_defaults_off_and_parses(self): + off = import_parser.parse_args(['--format', 'thycotic', 'https://secret.example.com']) + self.assertFalse(off.add_teams) + on = import_parser.parse_args([ + '--format', 'thycotic', '--add-users', '--add-teams', + 'https://secret.example.com', + ]) + self.assertTrue(on.add_teams) + + +if __name__ == '__main__': + logging.basicConfig() + unittest.main() \ No newline at end of file diff --git a/unit-tests/test_thycotic_add_users.py b/unit-tests/test_thycotic_add_users.py index cca52233d..3d90b2ead 100644 --- a/unit-tests/test_thycotic_add_users.py +++ b/unit-tests/test_thycotic_add_users.py @@ -276,7 +276,8 @@ def test_flag_defaults_off_and_parses(self): off = import_parser.parse_args(['--format', 'thycotic', 'https://secret.example.com']) self.assertFalse(off.add_users) on = import_parser.parse_args([ - '--format', 'thycotic', '--add-users', 'https://secret.example.com', + '--format', 'thycotic', '--add-users', '--add-teams', + 'https://secret.example.com', ]) self.assertTrue(on.add_users) From 6e317e9ab286edb5f12a17f231c9f9fb1f9bbfae Mon Sep 17 00:00:00 2001 From: Shashwat Tripathi Date: Fri, 30 Oct 2026 17:22:53 +0530 Subject: [PATCH 6/6] Handle missing groups in Thycotic group lookup/membership --- keepercommander/importer/thycotic/thycotic.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/keepercommander/importer/thycotic/thycotic.py b/keepercommander/importer/thycotic/thycotic.py index 66a2b0a2a..ff66b4ab6 100644 --- a/keepercommander/importer/thycotic/thycotic.py +++ b/keepercommander/importer/thycotic/thycotic.py @@ -243,7 +243,8 @@ def get_group_lookup(auth, on_progress=None): lookup[group_id] = g return lookup except: - pass + logging.warning("Failed to get group lookup") + return {} @staticmethod def _parse_choice(choice, count): @@ -2515,7 +2516,7 @@ def progress(position, total): team.uid = group_id team.name = group['name'] user_ids = set(group['users']) - for sub_group_id in group['groups']: + for sub_group_id in group.get('groups', []): if sub_group_id in groups: sub_group = groups[sub_group_id] user_ids.update(sub_group['users'])