From 1613fd12cf8fad2310943286f90ea23768c4e58b Mon Sep 17 00:00:00 2001 From: Bob Clough Date: Fri, 8 Dec 2023 12:32:20 +0000 Subject: [PATCH 1/4] target_suitecrm: add functionality to disable accounts instead of delete Currently we delete any accounts which no longer exist in ldap. We suspect this will cause historic reporting to fail, and the recommendation from suitecrm is to disable users instead. --- lifecycle/target_suitecrm.py | 68 +++++++++++++++++++++++++++--------- setup.py | 1 + 2 files changed, 53 insertions(+), 16 deletions(-) diff --git a/lifecycle/target_suitecrm.py b/lifecycle/target_suitecrm.py index e60c8ac..153b23e 100644 --- a/lifecycle/target_suitecrm.py +++ b/lifecycle/target_suitecrm.py @@ -40,6 +40,7 @@ def __init__(self, *args): "users_cleanup", "excluded_usernames", "admin_groups", + "delete_absent_users", } supported_user_fields = { @@ -60,6 +61,7 @@ def __init__(self, *args): "api_page_size": 20, "stages": ["users_create", "users_sync", "users_disable", "users_cleanup"], "excluded_usernames": [], + "delete_absent_users": True, } def process_groups_patterns(self, groups_patterns: list[str]) -> list[str]: @@ -477,23 +479,57 @@ def users_cleanup(self, diff: ModelDifference): logging.debug("Excluded usernames: %s", self.config["excluded_usernames"]) for user in diff.removed_users.values(): _id = self._users_data[user.username]["id"] - logging.debug( - "Attempting to delete: %s. Is user excluded: %s", - user.username, - user.username in self.config["excluded_usernames"], - ) - if user.username not in self.config["excluded_usernames"]: - deletion_record = { - "data": { - "type": "User", - "id": _id, - "attributes": { - "deleted": 1, - }, + if self.config["delete_absent_users"]: + if user.username not in self.config["excluded_usernames"]: + logging.debug( + "Attempting to delete %s.", + user.username, + ) + deletion_record = { + "data": { + "type": "User", + "id": _id, + "attributes": { + "deleted": 1, + }, + } } - } - logging.debug("Deleting user: %s", user.username) - self._request("/Api/V8/module", method="PATCH", json=deletion_record) + logging.debug("Deleting user: %s", user.username) + self._request( + "/Api/V8/module", method="PATCH", json=deletion_record + ) + else: + logging.debug( + "Not attempting to delete %s as they are in excluded_usernames", + user.username, + ) + else: + if not user.locked: + if user.username not in self.config["excluded_usernames"]: + logging.debug("Attempting to disable: %s.", user.username) + disablement_record = { + "data": { + "type": "User", + "id": _id, + "attributes": { + "status": "Inactive", + }, + } + } + logging.debug("Disabling account for %s", user.username) + self._request( + "/Api/V8/module", method="PATCH", json=disablement_record + ) + else: + logging.debug( + "Not attempting to disable %s as they are in excluded_usernames", + user.username, + ) + else: + logging.debug( + "Not attempting to disable %s as they are already locked", + user.username, + ) def _sync_emails_for_users(self, diff: ModelDifference): for user in diff.changed_users.values(): diff --git a/setup.py b/setup.py index 5d1800d..2c0f6d7 100644 --- a/setup.py +++ b/setup.py @@ -15,6 +15,7 @@ "pytest-cov", "pytest-mock", "pytest-pylint", + "black", ], }, python_requires=">=3.7", From 4c9d994c89acdcc9bc0bfbd357f1451cd8ce752d Mon Sep 17 00:00:00 2001 From: Bob Clough Date: Fri, 8 Dec 2023 14:46:31 +0000 Subject: [PATCH 2/4] target_suitecrm: add tests for disabling users instead of deleting them --- tests/test_target_suitecrm.py | 64 ++++++++++++++++++++++++++++++++++- 1 file changed, 63 insertions(+), 1 deletion(-) diff --git a/tests/test_target_suitecrm.py b/tests/test_target_suitecrm.py index 19a6150..fd17fd8 100644 --- a/tests/test_target_suitecrm.py +++ b/tests/test_target_suitecrm.py @@ -13,7 +13,7 @@ @pytest.fixture(name="basic_config") -def fixture_config(): +def fixture_basic_config(): """Create a config""" config = { "url": "127.0.0.1:8080", @@ -24,6 +24,18 @@ def fixture_config(): } return config +@pytest.fixture(name="users_disable_config") +def fixture_users_disable_config(): + """Create a config""" + config = { + "url": "127.0.0.1:8080", + "api_username": "user", + "api_password": "bitnami", + "api_client_id": "asd", + "api_client_secret": "secret", + "delete_absent_users": False, + } + return config def test_basic_config_creation(basic_config): """Make sure that a target can be created from a basic config""" @@ -397,6 +409,50 @@ def test_user_delete(basic_target, suitecrm_server): users = server.search_by_type("User") assert users[0]["attributes"]["first_name"] == "Ad" +def test_users_disable(users_disable_target, suitecrm_server): + """Delete a user and check it's been deleted""" + server = suitecrm_server([]) + server.create_user( + { + "user_name": "adalice", + "first_name": "Ad", + "last_name": "Alice", + "email1": "ad.alice@example.org", + "status": "Active", + } + ) + server.create_user( + { + "user_name": "basicuser", + "first_name": "Basic", + "last_name": "Bob", + "full_name": "Basic Bob", + "email1": "basic.bob@example.org", + "status": "Active", + } + ) + + deleted_user = User( + "basicuser", forename="Basic", surname="Bob", email=("basic.bob@example.org",) + ) + remaining_user = User( + "adalice", forename="Ad", surname="Alice", email=("ad.alice@example.org",) + ) + + diff = ModelDifference( + source_users={"adAlice": remaining_user, "basicuser": deleted_user}, + target_users={"adAlice": remaining_user}, + added_users={}, + changed_users={}, + unchanged_users={"adAlice": remaining_user}, + removed_users={"basicuser": deleted_user}, + ) + + users_disable_target.users_cleanup(diff) + users = server.search_by_type("User") + assert users[0]["attributes"]["first_name"] == "Ad" + assert users[1]["attributes"]["first_name"] == "Basic" + assert users[1]["attributes"]["status"] == "Inactive" def test_groups_emails_sync_no_changes(basic_config, suitecrm_server): """Test that when a user is part of a group with an E-mail address, @@ -595,3 +651,9 @@ def fixture_basic_target(basic_config): """Create a TargetSuiteCRM with default config""" target = TargetSuiteCRM(basic_config, None) return target + +@pytest.fixture(name="users_disable_target") +def fixture_users_disable_target(users_disable_config): + """Create a TargetSuiteCRM with config set up to disable users instead of deleting them""" + target = TargetSuiteCRM(users_disable_config, None) + return target From 13bd2a3eeb842a0f5bedb548b14ae4a81eb29ae6 Mon Sep 17 00:00:00 2001 From: Bob Clough Date: Fri, 8 Dec 2023 15:06:56 +0000 Subject: [PATCH 3/4] target_suitecrm: add tests for excluding users from user cleanup --- tests/test_target_suitecrm.py | 65 ++++++++++++++++++++++++++++++----- 1 file changed, 57 insertions(+), 8 deletions(-) diff --git a/tests/test_target_suitecrm.py b/tests/test_target_suitecrm.py index fd17fd8..a2d182c 100644 --- a/tests/test_target_suitecrm.py +++ b/tests/test_target_suitecrm.py @@ -21,9 +21,11 @@ def fixture_basic_config(): "api_password": "bitnami", "api_client_id": "asd", "api_client_secret": "secret", + "excluded_usernames": ["excluded"], } return config + @pytest.fixture(name="users_disable_config") def fixture_users_disable_config(): """Create a config""" @@ -34,9 +36,11 @@ def fixture_users_disable_config(): "api_client_id": "asd", "api_client_secret": "secret", "delete_absent_users": False, + "excluded_usernames": ["excluded"], } return config + def test_basic_config_creation(basic_config): """Make sure that a target can be created from a basic config""" target = TargetSuiteCRM(basic_config, None) @@ -388,26 +392,47 @@ def test_user_delete(basic_target, suitecrm_server): "status": "Active", } ) + server.create_user( + { + "user_name": "excluded", + "first_name": "Ernie", + "last_name": "Excluded", + "email1": "ernie.excluded@example.org", + "status": "Active", + } + ) + remaining_user = User( + "adalice", forename="Ad", surname="Alice", email=("ad.alice@example.org",) + ) deleted_user = User( "basicuser", forename="Basic", surname="Bob", email=("basic.bob@example.org",) ) - remaining_user = User( - "adalice", forename="Ad", surname="Alice", email=("ad.alice@example.org",) + excluded_user = User( + "excluded", + forename="Ernie", + surname="Excluded", + email=("ernie.excluded@example.org",), ) diff = ModelDifference( - source_users={"basicuser": deleted_user, "adAlice": remaining_user}, + source_users={ + "adAlice": remaining_user, + "basicuser": deleted_user, + "excluded": excluded_user, + }, target_users={"adAlice": remaining_user}, added_users={}, changed_users={}, unchanged_users={"adAlice": remaining_user}, - removed_users={"basicuser": deleted_user}, + removed_users={"basicuser": deleted_user, "excluded": excluded_user}, ) basic_target.users_cleanup(diff) users = server.search_by_type("User") assert users[0]["attributes"]["first_name"] == "Ad" + assert users[1]["attributes"]["first_name"] == "Ernie" + def test_users_disable(users_disable_target, suitecrm_server): """Delete a user and check it's been deleted""" @@ -431,28 +456,51 @@ def test_users_disable(users_disable_target, suitecrm_server): "status": "Active", } ) + server.create_user( + { + "user_name": "excluded", + "first_name": "Ernie", + "last_name": "Excluded", + "email1": "ernie.excluded@example.org", + "status": "Active", + } + ) + remaining_user = User( + "adalice", forename="Ad", surname="Alice", email=("ad.alice@example.org",) + ) deleted_user = User( "basicuser", forename="Basic", surname="Bob", email=("basic.bob@example.org",) ) - remaining_user = User( - "adalice", forename="Ad", surname="Alice", email=("ad.alice@example.org",) + excluded_user = User( + "excluded", + forename="Ernie", + surname="Excluded", + email=("ernie.excluded@example.org",), ) diff = ModelDifference( - source_users={"adAlice": remaining_user, "basicuser": deleted_user}, + source_users={ + "adAlice": remaining_user, + "basicuser": deleted_user, + "excluded": excluded_user, + }, target_users={"adAlice": remaining_user}, added_users={}, changed_users={}, unchanged_users={"adAlice": remaining_user}, - removed_users={"basicuser": deleted_user}, + removed_users={"basicuser": deleted_user, "excluded": excluded_user}, ) users_disable_target.users_cleanup(diff) users = server.search_by_type("User") assert users[0]["attributes"]["first_name"] == "Ad" + assert users[0]["attributes"]["status"] == "Active" assert users[1]["attributes"]["first_name"] == "Basic" assert users[1]["attributes"]["status"] == "Inactive" + assert users[2]["attributes"]["first_name"] == "Ernie" + assert users[2]["attributes"]["status"] == "Active" + def test_groups_emails_sync_no_changes(basic_config, suitecrm_server): """Test that when a user is part of a group with an E-mail address, @@ -652,6 +700,7 @@ def fixture_basic_target(basic_config): target = TargetSuiteCRM(basic_config, None) return target + @pytest.fixture(name="users_disable_target") def fixture_users_disable_target(users_disable_config): """Create a TargetSuiteCRM with config set up to disable users instead of deleting them""" From 328b5900817c07385c1af56439c2eea29581af62 Mon Sep 17 00:00:00 2001 From: Bob Clough Date: Fri, 8 Dec 2023 16:57:21 +0000 Subject: [PATCH 4/4] apply readability suggestions from code review Co-Authored-By: richardmaw-codethink --- tests/test_target_suitecrm.py | 25 +++++++++++++++++-------- 1 file changed, 17 insertions(+), 8 deletions(-) diff --git a/tests/test_target_suitecrm.py b/tests/test_target_suitecrm.py index a2d182c..1808153 100644 --- a/tests/test_target_suitecrm.py +++ b/tests/test_target_suitecrm.py @@ -430,8 +430,8 @@ def test_user_delete(basic_target, suitecrm_server): basic_target.users_cleanup(diff) users = server.search_by_type("User") - assert users[0]["attributes"]["first_name"] == "Ad" - assert users[1]["attributes"]["first_name"] == "Ernie" + assert any(user["attributes"]["first_name"] == "Ad" for user in users) + assert any(user["attributes"]["first_name"] == "Ernie" for user in users) def test_users_disable(users_disable_target, suitecrm_server): @@ -494,12 +494,21 @@ def test_users_disable(users_disable_target, suitecrm_server): users_disable_target.users_cleanup(diff) users = server.search_by_type("User") - assert users[0]["attributes"]["first_name"] == "Ad" - assert users[0]["attributes"]["status"] == "Active" - assert users[1]["attributes"]["first_name"] == "Basic" - assert users[1]["attributes"]["status"] == "Inactive" - assert users[2]["attributes"]["first_name"] == "Ernie" - assert users[2]["attributes"]["status"] == "Active" + assert any( + user["attributes"]["first_name"] == "Ad" + and user["attributes"]["status"] == "Active" + for user in users + ) + assert any( + user["attributes"]["first_name"] == "Basic" + and user["attributes"]["status"] == "Inactive" + for user in users + ) + assert any( + user["attributes"]["first_name"] == "Ernie" + and user["attributes"]["status"] == "Active" + for user in users + ) def test_groups_emails_sync_no_changes(basic_config, suitecrm_server):