From 67b1dc4ba42e50c27f5854733c496f670906d456 Mon Sep 17 00:00:00 2001 From: Max Ustinov Date: Fri, 15 Sep 2023 14:31:09 -0700 Subject: [PATCH] REL-4242 - Python SDK v16.6.2 - Empty fields and no dependency on helper (#527) * Update Python SDK to version 16.6.2 and updates change log * KSM-463 - Python SDK - fix for record's data JSON where "fields": null (#525) * KSM-458 Remove core dep on helper module Refactor add_custom_field not to use the helper module. The method will accept an instance of FieldType, however it is not tied to the keeper_secrets_manager_helper.v3.field_type.FieldType module. Allow the ability to set the type, label, and value not using the FieldType instance. The method will still take the param `field`, however it will also take `field_type`, `label`, and `value`. This allows adding a custom field without have to use the helper module. Removed references `keeper-secrets-manager-helper` from setup.py and requirements.txt. This should break the circular reference. --- .../tests/misc_test.py | 2 +- sdk/python/__init__.py | 0 sdk/python/core/README.md | 4 + .../keeper_secrets_manager_core/dto/dtos.py | 35 +++- .../core/keeper_secrets_manager_core/mock.py | 2 +- sdk/python/core/requirements.txt | 1 - sdk/python/core/setup.py | 3 +- sdk/python/core/tests/record_test.py | 176 +++++++++++++++--- 8 files changed, 186 insertions(+), 37 deletions(-) delete mode 100644 sdk/python/__init__.py diff --git a/integration/keeper_secrets_manager_cli/tests/misc_test.py b/integration/keeper_secrets_manager_cli/tests/misc_test.py index c9cf87f3..fdbe4dc6 100644 --- a/integration/keeper_secrets_manager_cli/tests/misc_test.py +++ b/integration/keeper_secrets_manager_cli/tests/misc_test.py @@ -66,7 +66,7 @@ def test_config_mode_dog_food(self): sp = subprocess.run(["icacls.exe", Config.default_ini_file], capture_output=True) if sp.stderr is not None and sp.stderr.decode() != "": self.fail("Could not icacls.exe {}: {}".format(Config.default_ini_file,sp.stderr.decode())) - allowed_users = [user.lower(), "Administrators".lower()] + allowed_users = [user.decode().lower(), "Administrators".lower()] for line in sp.stdout.decode().split("\n"): parts = line[len(Config.default_ini_file):].split(":") if len(parts) == 2: diff --git a/sdk/python/__init__.py b/sdk/python/__init__.py deleted file mode 100644 index e69de29b..00000000 diff --git a/sdk/python/core/README.md b/sdk/python/core/README.md index ae9dc72f..ea621114 100644 --- a/sdk/python/core/README.md +++ b/sdk/python/core/README.md @@ -4,6 +4,10 @@ For more information see our official documentation page https://docs.keeper.io/ # Change Log +## 16.6.2 +* KSM-463 - Python SDK - Fix a bug when fields is null +* KSM-458 - Python SDK - Remove core's dependency on the helper module. Fixes [issue 488](https://github.com/Keeper-Security/secrets-manager/issues/488) + ## 16.6.1 * KSM 444 - Python - Added folderUid and innerFolderUid to Record diff --git a/sdk/python/core/keeper_secrets_manager_core/dto/dtos.py b/sdk/python/core/keeper_secrets_manager_core/dto/dtos.py index f346393c..07cd7513 100644 --- a/sdk/python/core/keeper_secrets_manager_core/dto/dtos.py +++ b/sdk/python/core/keeper_secrets_manager_core/dto/dtos.py @@ -18,7 +18,6 @@ from keeper_secrets_manager_core import utils, helpers from keeper_secrets_manager_core.crypto import CryptoUtils from keeper_secrets_manager_core.exceptions import KeeperError -from keeper_secrets_manager_helper.v3.field_type import FieldType class Record: @@ -54,6 +53,8 @@ def __init__(self, record_dict, secret_key, folder_uid = ''): self.raw_json = record_data_json self.dict = utils.json_to_dict(self.raw_json) + if self.dict and self.dict.get('fields') is None: + self.dict['fields'] = [] self.title = self.dict.get('title') self.type = self.dict.get('type') self.revision = record_dict.get('revision') @@ -184,16 +185,32 @@ def set_custom_field_value(self, field_type, value): field["value"] = value self._update() - def add_custom_field(self, field: FieldType) -> bool: - if issubclass(type(field), FieldType): - if self.dict.get('custom', None) is None: - self.dict['custom'] = [] - custom = self.dict['custom'] + def add_custom_field(self, field=None, field_type=None, label=None, value=None) -> bool: + if self.dict.get('custom', None) is None: + self.dict['custom'] = [] + custom = self.dict['custom'] + + # Make backward compatible. Assumes keeper_secrets_manager_helper.v#.field_type.FieldType is passed in. + if field is not None: + if field.__class__.__name__ != "FieldType": + raise ValueError("The field is not an instance of FieldType") fdict = field.to_dict() custom.append(fdict) - self._update() - return True - return False + else: + if field_type is None: + return False + if isinstance(value, list) is False: + value = [value] + field_dict = { + "type": field_type, + "value": value + } + if label is not None: + field_dict["label"] = label + custom.append(field_dict) + + self._update() + return True # TODO: Deprecate this for better getter and setters def field(self, field_type, value=None, single=False): diff --git a/sdk/python/core/keeper_secrets_manager_core/mock.py b/sdk/python/core/keeper_secrets_manager_core/mock.py index ebe7b605..81889c58 100644 --- a/sdk/python/core/keeper_secrets_manager_core/mock.py +++ b/sdk/python/core/keeper_secrets_manager_core/mock.py @@ -390,7 +390,7 @@ def add_file(self, name, title=None, content_type=None, url=None, content=None, def dump(self, secret, flags=None): - fields = list(self._fields) + fields = list(self._fields) if isinstance(self._fields, list) else self._fields # If no files, the JSON has null files = None diff --git a/sdk/python/core/requirements.txt b/sdk/python/core/requirements.txt index b4d72fb0..f4a996a1 100644 --- a/sdk/python/core/requirements.txt +++ b/sdk/python/core/requirements.txt @@ -1,4 +1,3 @@ -keeper-secrets-manager-helper>=1.0.4 ecdsa cryptography>=39.0.1 requests>=2.28.2 diff --git a/sdk/python/core/setup.py b/sdk/python/core/setup.py index 83287387..f77c050b 100644 --- a/sdk/python/core/setup.py +++ b/sdk/python/core/setup.py @@ -11,7 +11,6 @@ long_description = fp.read() install_requires = [ - 'keeper-secrets-manager-helper>=1.0.4', 'ecdsa', 'requests', 'cryptography>=39.0.1', @@ -20,7 +19,7 @@ setup( name="keeper-secrets-manager-core", - version="16.6.1", + version="16.6.2", description="Keeper Secrets Manager for Python 3", long_description=long_description, long_description_content_type="text/markdown", diff --git a/sdk/python/core/tests/record_test.py b/sdk/python/core/tests/record_test.py index 0f36b53e..050748f3 100644 --- a/sdk/python/core/tests/record_test.py +++ b/sdk/python/core/tests/record_test.py @@ -1,6 +1,6 @@ -import unittest -import tempfile import os +import tempfile +import unittest from keeper_secrets_manager_core.storage import FileKeyValueStorage from keeper_secrets_manager_core import SecretsManager @@ -20,34 +20,40 @@ def tearDown(self): os.chdir(self.orig_working_dir) def test_the_login_record_password(self): - - """ If the record type is login, the password will be placed in the instance attribute. + """ If the record type is login, the password will be placed + in the instance attribute. """ try: with tempfile.NamedTemporaryFile("w", delete=False) as fh: fh.write(MockConfig.make_json()) fh.seek(0) - secrets_manager = SecretsManager(config=FileKeyValueStorage(config_file_location=fh.name)) + secrets_manager = SecretsManager( + config=FileKeyValueStorage(config_file_location=fh.name)) # A good record. - # 'fields': [...{'type': 'password', 'value': ['My Password']}...] + # 'fields':[{'type': 'password', 'value': ['My Password']}...] good_res = mock.Response() - good = good_res.add_record(title="Good Record", record_type='login') + good = good_res.add_record( + title="Good Record", record_type='login') good.field("login", "My Login") good.field("password", "My Password") - # A bad record. This would be like if someone removed a password text from an existing field. + # A bad record. This would be like if someone removed + # a password text from an existing field. # 'fields': [...{'type': 'password', 'value': []}...] bad_res = mock.Response() - bad = bad_res.add_record(title="Bad Record", record_type='login') + bad = bad_res.add_record( + title="Bad Record", record_type='login') bad.field("login", "My Login") bad.field("password", []) - # An ugly record. The application didn't even add the field. We need to set flags to prune empty fields. + # An ugly record. The application didn't even add the field. + # We need to set flags to prune empty fields. # 'fields': [...] ugly_res = mock.Response(flags={"prune_empty_fields": True}) - ugly = ugly_res.add_record(title="Ugly Record", record_type='login') + ugly = ugly_res.add_record( + title="Ugly Record", record_type='login') ugly.field("login", "My Login") # this will be removed from the fields array. @@ -59,16 +65,23 @@ def test_the_login_record_password(self): res_queue.add_response(ugly_res) records = secrets_manager.get_secrets() - self.assertEqual(1, len(records), "didn't get 1 record for the good") - self.assertEqual("My Password", records[0].password, "did not get correct password for the good") + self.assertEqual( + 1, len(records), "didn't get 1 record for the good") + self.assertEqual( + "My Password", records[0].password, + "did not get correct password for the good") records = secrets_manager.get_secrets() - self.assertEqual(1, len(records), "didn't get 1 record for the bad") - self.assertIsNone(records[0].password, "password is defined for the bad") + self.assertEqual( + 1, len(records), "didn't get 1 record for the bad") + self.assertIsNone(records[0].password, + "password is defined for the bad") records = secrets_manager.get_secrets() - self.assertEqual(1, len(records), "didn't get 1 record for the ugly") - self.assertIsNone(records[0].password, "password is defined for the ugly") + self.assertEqual( + 1, len(records), "didn't get 1 record for the ugly") + self.assertIsNone(records[0].password, + "password is defined for the ugly") finally: try: os.unlink(fh.name) @@ -77,7 +90,8 @@ def test_the_login_record_password(self): def test_record_field(self): - rf = RecordField(field_type="login", value="test", label="Test", required=True, enforceGeneration=False, + rf = RecordField(field_type="login", value="test", label="Test", + required=True, enforceGeneration=False, privacyScreen=True, complexity={"foo": "bar"}) value = helpers.obj_to_dict(rf) @@ -85,9 +99,12 @@ def test_record_field(self): self.assertEqual(["test"], value.get("value"), "value is not correct") self.assertEqual("Test", value.get("label"), "label is not correct") self.assertTrue(value.get("required"), "required is not correct") - self.assertFalse(value.get("enforceGeneration"), "enforceGeneration is not correct") - self.assertTrue(value.get("privacyScreen"), "privacyScreen is not correct") - self.assertIsNotNone(value.get("complexity"), "complexity is not correct") + self.assertFalse(value.get("enforceGeneration"), + "enforceGeneration is not correct") + self.assertTrue(value.get("privacyScreen"), + "privacyScreen is not correct") + self.assertIsNotNone(value.get("complexity"), + "complexity is not correct") rf = RecordField(field_type="login", value="test", privacyScreen=None) @@ -96,7 +113,120 @@ def test_record_field(self): self.assertEqual(["test"], value.get("value"), "value is not correct") self.assertIsNone(value.get("label"), "label is not correct") self.assertIsNone(value.get("required"), "required is not correct") - self.assertIsNone(value.get("enforceGeneration"), "enforceGeneration is not correct") + self.assertIsNone(value.get("enforceGeneration"), + "enforceGeneration is not correct") assert "privacyScreen" not in value, "privacyScreen exists in dictionary" - self.assertIsNone(value.get("privacyScreen"), "privacyScreen is not correct") + self.assertIsNone(value.get("privacyScreen"), + "privacyScreen is not correct") self.assertIsNone(value.get("complexity"), "complexity is not correct") + + def test_add_custom_field_by_param(self): + + try: + with tempfile.NamedTemporaryFile("w", delete=False) as fh: + fh.write(MockConfig.make_json()) + fh.seek(0) + secrets_manager = SecretsManager(config=FileKeyValueStorage(config_file_location=fh.name)) + + res = mock.Response() + mock_record = res.add_record(title="Good Record", record_type='login') + mock_record.field("login", "My Login") + mock_record.field("password", "My Password") + + res_queue = mock.ResponseQueue(client=secrets_manager) + res_queue.add_response(res) + + records = secrets_manager.get_secrets() + record = records[0] + + record.add_custom_field( + field_type='text', + label="My Label", + value="My Value" + ) + new_value = record.get_custom_field("My Label") + self.assertEqual("text", new_value.get("type")) + self.assertEqual("My Label", new_value.get("label")) + self.assertEqual(['My Value'], new_value.get("value")) + finally: + try: + os.unlink(fh.name) + except OSError: + pass + + def test_add_custom_field_by_field_type(self): + + class FieldType: + + def __init__(self, field_type, label, value): + self.field_type = field_type + self.label = label + self.value = value + + def to_dict(self): + return { + "type": self.field_type, + "label": self.label, + "value": self.value + } + + try: + with tempfile.NamedTemporaryFile("w", delete=False) as fh: + fh.write(MockConfig.make_json()) + fh.seek(0) + secrets_manager = SecretsManager(config=FileKeyValueStorage(config_file_location=fh.name)) + + res = mock.Response() + mock_record = res.add_record(title="Good Record", record_type='login') + mock_record.field("login", "My Login") + mock_record.field("password", "My Password") + + res_queue = mock.ResponseQueue(client=secrets_manager) + res_queue.add_response(res) + + records = secrets_manager.get_secrets() + record = records[0] + + field = FieldType( + field_type="text", + label="My Label", + value=["My Value"] + ) + + record.add_custom_field(field=field) + + new_value = record.get_custom_field("My Label") + self.assertEqual("text", new_value.get("type")) + self.assertEqual("My Label", new_value.get("label")) + self.assertEqual(['My Value'], new_value.get("value")) + finally: + try: + os.unlink(fh.name) + except OSError: + pass + + def test_missing_fields_section(self): + """ Test for clients that may set "fields": null in JSON data """ + + try: + with tempfile.NamedTemporaryFile("w", delete=False) as fh: + fh.write(MockConfig.make_json()) + fh.seek(0) + secrets_manager = SecretsManager( + config=FileKeyValueStorage(config_file_location=fh.name)) + + res = mock.Response() + rec = res.add_record(title="MyLogin", record_type='login') + res.records[rec.uid]._fields = None + res_queue = mock.ResponseQueue(client=secrets_manager) + res_queue.add_response(res) + + records = secrets_manager.get_secrets() + self.assertEqual( + 1, len(records), "didn't get 1 record for MyLogin") + self.assertEqual([], records[0].dict.get('fields')) + finally: + try: + os.unlink(fh.name) + except OSError: + pass