From 2dd3c8cd0c5292d60844379340f2bca19864a70f Mon Sep 17 00:00:00 2001 From: David Murphy < dmurphy@saltstack.com> Date: Tue, 3 Oct 2023 17:43:18 -0600 Subject: [PATCH 01/14] Increase code coverare for salt/client/mixins.py --- tests/pytests/integration/client/test_runner.py | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/tests/pytests/integration/client/test_runner.py b/tests/pytests/integration/client/test_runner.py index 3303277da81..e0b6cb400f5 100644 --- a/tests/pytests/integration/client/test_runner.py +++ b/tests/pytests/integration/client/test_runner.py @@ -1,3 +1,5 @@ +import logging + import pytest import salt.auth @@ -10,6 +12,8 @@ pytestmark = [ pytest.mark.windows_whitelisted, ] +log = logging.getLogger(__name__) + @pytest.fixture def client(client_config): @@ -150,3 +154,14 @@ def test_invalid_kwargs_are_ignored(client, auth_creds): ret = client.cmd_sync(low.copy()) assert ret assert ret[0] == "foo" + + +def test_get_docs(client, auth_creds): + ret = client.get_docs(arg="*") + assert "auth.del_token" in ret + assert "auth.mk_token" in ret + assert "cache.clear_pillar" in ret + assert "cache.grains" in ret + assert "state.soft_kill" in ret + assert "virt.start" in ret + assert "test.arg" in ret From 5765a8b4f633e1745b33183aea0ba14de81b0b79 Mon Sep 17 00:00:00 2001 From: David Murphy < dmurphy@saltstack.com> Date: Wed, 4 Oct 2023 09:43:10 -0600 Subject: [PATCH 02/14] Remove left-over logging from debugging --- tests/pytests/integration/client/test_runner.py | 4 ---- 1 file changed, 4 deletions(-) diff --git a/tests/pytests/integration/client/test_runner.py b/tests/pytests/integration/client/test_runner.py index e0b6cb400f5..adb2cd46449 100644 --- a/tests/pytests/integration/client/test_runner.py +++ b/tests/pytests/integration/client/test_runner.py @@ -1,5 +1,3 @@ -import logging - import pytest import salt.auth @@ -12,8 +10,6 @@ pytestmark = [ pytest.mark.windows_whitelisted, ] -log = logging.getLogger(__name__) - @pytest.fixture def client(client_config): From f5e4c1b886f471c8dfb1349ea29dc37c3e57d19a Mon Sep 17 00:00:00 2001 From: David Murphy Date: Thu, 5 Oct 2023 17:02:49 -0600 Subject: [PATCH 03/14] Update tests/pytests/integration/client/test_runner.py Co-authored-by: Megan Wilhite --- tests/pytests/integration/client/test_runner.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/pytests/integration/client/test_runner.py b/tests/pytests/integration/client/test_runner.py index adb2cd46449..74a25cb1353 100644 --- a/tests/pytests/integration/client/test_runner.py +++ b/tests/pytests/integration/client/test_runner.py @@ -152,7 +152,7 @@ def test_invalid_kwargs_are_ignored(client, auth_creds): assert ret[0] == "foo" -def test_get_docs(client, auth_creds): +def test_get_docs(client): ret = client.get_docs(arg="*") assert "auth.del_token" in ret assert "auth.mk_token" in ret From 84d6c962f6edd4c232d0d41d232a8fdb8b5533c0 Mon Sep 17 00:00:00 2001 From: Pedro Algarvio Date: Tue, 24 Oct 2023 12:36:27 +0100 Subject: [PATCH 04/14] Something changes on GH's Mac's, switch package name Signed-off-by: Pedro Algarvio --- tests/pytests/functional/modules/test_mac_pkgutil.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tests/pytests/functional/modules/test_mac_pkgutil.py b/tests/pytests/functional/modules/test_mac_pkgutil.py index 5ce546d776c..12a07726ad7 100644 --- a/tests/pytests/functional/modules/test_mac_pkgutil.py +++ b/tests/pytests/functional/modules/test_mac_pkgutil.py @@ -56,6 +56,8 @@ def macports_package_url(macports_package_filename): @pytest.fixture(scope="module") def pkg_name(grains): + if grains["osrelease_info"][0] >= 12: + return "com.apple.pkg.XcodeSystemResources" if grains["osrelease_info"][0] >= 11: return "com.apple.pkg.InstallAssistant.macOSBigSur" if grains["osrelease_info"][:2] == (10, 15): From 134ea53a43354829676af2bcf61fae84c8017aa7 Mon Sep 17 00:00:00 2001 From: twangboy Date: Mon, 16 Oct 2023 15:43:04 -0600 Subject: [PATCH 05/14] Fix issue when policy dirs are missing --- salt/modules/win_file.py | 51 +++-- tests/pytests/unit/modules/test_win_file.py | 211 ++++++++++++++++++++ 2 files changed, 243 insertions(+), 19 deletions(-) diff --git a/salt/modules/win_file.py b/salt/modules/win_file.py index d02d4589f2f..2efb15d0c24 100644 --- a/salt/modules/win_file.py +++ b/salt/modules/win_file.py @@ -16,8 +16,10 @@ import stat import sys import tempfile +import salt.utils.files import salt.utils.path import salt.utils.platform +import salt.utils.user from salt.exceptions import CommandExecutionError, SaltInvocationError from salt.modules.file import ( __clean_tmp, @@ -107,6 +109,15 @@ try: except ImportError: HAS_WINDOWS_MODULES = False +HAS_WIN_DACL = False +try: + if salt.utils.platform.is_windows(): + import salt.utils.win_dacl + + HAS_WIN_DACL = True +except ImportError: + HAS_WIN_DACL = False + if salt.utils.platform.is_windows(): if HAS_WINDOWS_MODULES: # namespace functions from file.py @@ -194,6 +205,8 @@ def __virtual__(): """ if not salt.utils.platform.is_windows() or not HAS_WINDOWS_MODULES: return False, "Module win_file: Missing Win32 modules" + if not HAS_WIN_DACL: + return False, "Module win_file: Unable to load salt.utils.win_dacl" return __virtualname__ @@ -305,7 +318,7 @@ def group_to_gid(group): if group is None: return "" - return __utils__["dacl.get_sid_string"](group) + return salt.utils.win_dacl.get_sid_string(group) def get_pgid(path, follow_symlinks=True): @@ -346,8 +359,8 @@ def get_pgid(path, follow_symlinks=True): if follow_symlinks and sys.getwindowsversion().major >= 6: path = _resolve_symlink(path) - group_name = __utils__["dacl.get_primary_group"](path) - return __utils__["dacl.get_sid_string"](group_name) + group_name = salt.utils.win_dacl.get_primary_group(path) + return salt.utils.win_dacl.get_sid_string(group_name) def get_pgroup(path, follow_symlinks=True): @@ -498,7 +511,7 @@ def uid_to_user(uid): if uid is None or uid == "": return "" - return __utils__["dacl.get_name"](uid) + return salt.utils.win_dacl.get_name(uid) def user_to_uid(user): @@ -518,9 +531,9 @@ def user_to_uid(user): salt '*' file.user_to_uid myusername """ if user is None: - user = __utils__["user.get_user"]() + user = salt.utils.user.get_user() - return __utils__["dacl.get_sid_string"](user) + return salt.utils.win_dacl.get_sid_string(user) def get_uid(path, follow_symlinks=True): @@ -558,8 +571,8 @@ def get_uid(path, follow_symlinks=True): if follow_symlinks and sys.getwindowsversion().major >= 6: path = _resolve_symlink(path) - owner_sid = __utils__["dacl.get_owner"](path) - return __utils__["dacl.get_sid_string"](owner_sid) + owner_sid = salt.utils.win_dacl.get_owner(path) + return salt.utils.win_dacl.get_sid_string(owner_sid) def get_user(path, follow_symlinks=True): @@ -597,7 +610,7 @@ def get_user(path, follow_symlinks=True): if follow_symlinks and sys.getwindowsversion().major >= 6: path = _resolve_symlink(path) - return __utils__["dacl.get_owner"](path) + return salt.utils.win_dacl.get_owner(path) def get_mode(path): @@ -735,9 +748,9 @@ def chown(path, user, group=None, pgroup=None, follow_symlinks=True): if not os.path.exists(path): raise CommandExecutionError("Path not found: {}".format(path)) - __utils__["dacl.set_owner"](path, user) + salt.utils.win_dacl.set_owner(path, user) if pgroup: - __utils__["dacl.set_primary_group"](path, pgroup) + salt.utils.win_dacl.set_primary_group(path, pgroup) return True @@ -767,7 +780,7 @@ def chpgrp(path, group): salt '*' file.chpgrp c:\\temp\\test.txt Administrators salt '*' file.chpgrp c:\\temp\\test.txt "'None'" """ - return __utils__["dacl.set_primary_group"](path, group) + return salt.utils.win_dacl.set_primary_group(path, group) def chgrp(path, group): @@ -802,7 +815,7 @@ def chgrp(path, group): .. code-block:: bash - salt '*' file.chpgrp c:\\temp\\test.txt administrators + salt '*' file.chgrp c:\\temp\\test.txt administrators """ func_name = "{}.chgrp".format(__virtualname__) if __opts__.get("fun", "") == func_name: @@ -871,7 +884,7 @@ def stats(path, hash_type="sha256", follow_symlinks=True): ret["mtime"] = pstat.st_mtime ret["ctime"] = pstat.st_ctime ret["size"] = pstat.st_size - ret["mode"] = __utils__["files.normalize_mode"](oct(stat.S_IMODE(pstat.st_mode))) + ret["mode"] = salt.utils.files.normalize_mode(oct(stat.S_IMODE(pstat.st_mode))) if hash_type: ret["sum"] = get_sum(path, hash_type) ret["type"] = "file" @@ -1503,7 +1516,7 @@ def is_link(path): ) try: - return __utils__["path.islink"](path) + return salt.utils.path.islink(path) except Exception as exc: # pylint: disable=broad-except raise CommandExecutionError(exc) @@ -1594,10 +1607,10 @@ def mkdir( # Set owner if owner: - __utils__["dacl.set_owner"](obj_name=path, principal=owner) + salt.utils.win_dacl.set_owner(obj_name=path, principal=owner) # Set permissions - __utils__["dacl.set_perms"]( + salt.utils.win_dacl.set_perms( obj_name=path, obj_type="file", grant_perms=grant_perms, @@ -1916,7 +1929,7 @@ def check_perms( path = os.path.expanduser(path) - return __utils__["dacl.check_perms"]( + return salt.utils.win_dacl.check_perms( obj_name=path, obj_type="file", ret=ret, @@ -2002,7 +2015,7 @@ def set_perms(path, grant_perms=None, deny_perms=None, inheritance=True, reset=F # Specify advanced attributes with a list salt '*' file.set_perms C:\\Temp\\ "{'jsnuffy': {'perms': ['read_attributes', 'read_ea'], 'applies_to': 'this_folder_only'}}" """ - return __utils__["dacl.set_perms"]( + return salt.utils.win_dacl.set_perms( obj_name=path, obj_type="file", grant_perms=grant_perms, diff --git a/tests/pytests/unit/modules/test_win_file.py b/tests/pytests/unit/modules/test_win_file.py index efcdb31a550..ea0f6a547f3 100644 --- a/tests/pytests/unit/modules/test_win_file.py +++ b/tests/pytests/unit/modules/test_win_file.py @@ -1,13 +1,43 @@ +import os import re import pytest import salt.modules.win_file as win_file +import salt.utils.user +import salt.utils.win_dacl from salt.exceptions import CommandExecutionError +from tests.support.mock import patch pytestmark = [pytest.mark.windows_whitelisted, pytest.mark.skip_unless_on_windows] +@pytest.fixture +def configure_loader_modules(): + return { + win_file: {}, + salt.utils.win_dacl: {}, + } + + +def test__virtual__not_windows(): + with patch("salt.utils.platform.is_windows", autospec=True, return_value=False): + expected = (False, "Module win_file: Missing Win32 modules") + result = win_file.__virtual__() + assert result == expected + with patch("salt.modules.win_file.HAS_WINDOWS_MODULES", False): + expected = (False, "Module win_file: Missing Win32 modules") + result = win_file.__virtual__() + assert result == expected + + +def test__virtual__no_dacl(): + with patch("salt.modules.win_file.HAS_WIN_DACL", False): + expected = (False, "Module win_file: Unable to load salt.utils.win_dacl") + result = win_file.__virtual__() + assert result == expected + + def test__get_version_os(): expected = ["32-bit Windows", "Windows NT"] result = win_file._get_version_os(0x00040004) @@ -56,6 +86,187 @@ def test__get_version_sys(): assert regex.search(result) +def test_get_pgid_error(): + with pytest.raises(CommandExecutionError): + win_file.get_pgid("C:\\Path\\That\\Does\\Not\\Exist.txt") + + +def test_get_pgid(): + """ + We can't know what this value is, so we're just making sure it found + something + """ + result = win_file.get_pgid(os.getenv("COMSPEC")) + assert not result == "" + + +def test_group_to_gid(): + with patch.dict(win_file.__opts__, {}): + result = win_file.group_to_gid("Administrators") + expected = "S-1-5-32-544" + assert result == expected + + +def test_group_to_gid_empty(): + with patch.dict(win_file.__opts__, {}): + result = win_file.group_to_gid("") + expected = "S-1-5-32" + assert result == expected + + +def test_uid_to_user(): + result = win_file.uid_to_user("S-1-5-32-544") + expected = "Administrators" + assert result == expected + + +def test_uid_to_user_empty(): + result = win_file.uid_to_user("") + expected = "" + assert result == expected + + +def test_user_to_uid(): + result = win_file.user_to_uid("Administrator") + expected = salt.utils.win_dacl.get_sid_string("Administrator") + assert result == expected + + +def test_user_to_uid_none(): + result = win_file.user_to_uid(None) + expected = salt.utils.win_dacl.get_sid_string(salt.utils.user.get_user()) + assert result == expected + + +def test_get_uid(): + """ + We can't know what this value is, so we're just making sure it found + something + """ + result = win_file.get_uid(os.getenv("WINDIR")) + assert not result == "" + + +def test_get_uid_error(): + with pytest.raises(CommandExecutionError): + win_file.get_uid("C:\\fake\\path") + + +def test_chown(tmp_path): + test_file = tmp_path / "test_file.txt" + test_file.touch() + win_file.chown(path=str(test_file), user="Administrators", pgroup="Guests") + assert win_file.get_user(str(test_file)) == "Administrators" + assert win_file.get_pgroup(str(test_file)) == "Guests" + + +def test_chpgrp(tmp_path): + test_file = tmp_path / "test_file.txt" + test_file.touch() + win_file.chown(path=str(test_file), user="Administrators", pgroup="Guests") + win_file.chpgrp(path=str(test_file), group="Administrators") + assert win_file.get_pgroup(str(test_file)) == "Administrators" + + +def test_stats_mode(tmp_path): + test_file = tmp_path / "test_file.txt" + test_file.touch() + results = win_file.stats(str(test_file)) + assert results["mode"] == "0666" + + +def test_is_link_true(tmp_path): + test_source = tmp_path / "test_source.txt" + test_link = tmp_path / "test_link.txt" + test_source.touch() + test_link.symlink_to(test_source) + results = win_file.is_link(str(test_link)) + expected = True + assert results == expected + + +def test_is_link_false(tmp_path): + test_file = tmp_path / "test_not_link.txt" + test_file.touch() + results = win_file.is_link(str(test_file)) + expected = False + assert results == expected + + +def test_mkdir(tmp_path): + test_dir = tmp_path / "test_dir" + grant_perms = {"Guests": {"perms": "full_control"}} + win_file.mkdir( + path=str(test_dir), + owner="Administrators", + grant_perms=grant_perms, + ) + owner = win_file.get_user(str(test_dir)) + assert owner == "Administrators" + perms = salt.utils.win_dacl.get_permissions(str(test_dir)) + assert perms["Not Inherited"]["Guests"]["grant"]["permissions"] == "Full control" + + +def test_check_perms(tmp_path): + test_dir = tmp_path / "test_dir" + test_dir.mkdir() + grant_perms = {"Guests": {"perms": "full_control"}} + ret = {} + with patch.dict(salt.utils.win_dacl.__opts__, {"test": False}): + result = win_file.check_perms( + path=str(test_dir), + ret=ret, + owner="Guests", + grant_perms=grant_perms, + ) + + expected = { + "changes": { + "grant_perms": { + "Guests": { + "permissions": "full_control", + }, + }, + "owner": "Guests", + }, + "comment": "", + "name": str(test_dir), + "result": True, + } + + assert result == expected + owner = win_file.get_user(str(test_dir)) + assert owner == "Guests" + perms = salt.utils.win_dacl.get_permissions(str(test_dir)) + assert perms["Not Inherited"]["Guests"]["grant"]["permissions"] == "Full control" + + +def test_set_perms(tmp_path): + test_dir = tmp_path / "test_dir" + test_dir.mkdir() + grant_perms = {"Guests": {"perms": "full_control"}} + win_file.set_perms( + path=str(test_dir), + grant_perms=grant_perms, + ) + perms = salt.utils.win_dacl.get_permissions(str(test_dir)) + assert perms["Not Inherited"]["Guests"]["grant"]["permissions"] == "Full control" + + +def test_get_user(): + """ + We can't know what this value is, so we're just making sure it found + something + """ + result = win_file.get_user(os.getenv("WINDIR")) + assert not result == "" + + +def test_get_user_error(): + with pytest.raises(CommandExecutionError): + win_file.get_user("C:\\fake\\path") + + def test_version_missing_file(): with pytest.raises(CommandExecutionError): win_file.version("C:\\Windows\\bogus.exe") From 636eff5fda14d945e757b9ef1c91e5b7ba95415c Mon Sep 17 00:00:00 2001 From: twangboy Date: Mon, 16 Oct 2023 15:45:00 -0600 Subject: [PATCH 06/14] Add changelog --- changelog/65411.fixed.md | 2 ++ 1 file changed, 2 insertions(+) create mode 100644 changelog/65411.fixed.md diff --git a/changelog/65411.fixed.md b/changelog/65411.fixed.md new file mode 100644 index 00000000000..0500a7364ee --- /dev/null +++ b/changelog/65411.fixed.md @@ -0,0 +1,2 @@ +Fixes an issue setting user or machine policy on Windows when the Group Policy +directory is missing From 1e976cf7353471af7340517e8407e8de4afdc975 Mon Sep 17 00:00:00 2001 From: twangboy Date: Tue, 17 Oct 2023 07:56:09 -0600 Subject: [PATCH 07/14] Fix lint --- salt/modules/win_file.py | 1 + salt/utils/win_dacl.py | 5 +-- .../modules/win_file/test_check_perms.py | 14 +++---- .../states/file/test_directory_win.py | 41 ++++--------------- tests/pytests/functional/states/test_reg.py | 1 - .../functional/utils/win_dacl/test_file.py | 31 +++++++------- .../functional/utils/win_dacl/test_reg.py | 40 +++++++----------- .../modules/file/test_file_block_replace.py | 5 ++- tests/pytests/unit/modules/test_win_file.py | 6 +-- tests/unit/modules/test_heat.py | 6 +-- tests/unit/states/test_heat.py | 6 +-- 11 files changed, 57 insertions(+), 99 deletions(-) diff --git a/salt/modules/win_file.py b/salt/modules/win_file.py index 2efb15d0c24..a7a411c93cc 100644 --- a/salt/modules/win_file.py +++ b/salt/modules/win_file.py @@ -1938,6 +1938,7 @@ def check_perms( deny_perms=deny_perms, inheritance=inheritance, reset=reset, + test_mode=__opts__["test"], ) diff --git a/salt/utils/win_dacl.py b/salt/utils/win_dacl.py index ec16f7a22c6..2e9fe87b811 100644 --- a/salt/utils/win_dacl.py +++ b/salt/utils/win_dacl.py @@ -2349,7 +2349,7 @@ def check_perms( deny_perms=None, inheritance=True, reset=False, - test_mode=None, + test_mode=False, ): """ Check owner and permissions for the passed directory. This function checks @@ -2429,9 +2429,6 @@ def check_perms( } }) """ - if test_mode is None: - test_mode = __opts__["test"] - # Validate obj_type if obj_type.lower() not in flags().obj_type: raise SaltInvocationError('Invalid "obj_type" passed: {}'.format(obj_type)) diff --git a/tests/pytests/functional/modules/win_file/test_check_perms.py b/tests/pytests/functional/modules/win_file/test_check_perms.py index 53e656eab18..7b829b35eed 100644 --- a/tests/pytests/functional/modules/win_file/test_check_perms.py +++ b/tests/pytests/functional/modules/win_file/test_check_perms.py @@ -21,9 +21,9 @@ def configure_loader_modules(): "__utils__": { "dacl.check_perms": win_dacl.check_perms, "dacl.set_perms": win_dacl.set_perms, - } + }, + "__opts__": {"test": False}, }, - win_dacl: {"__opts__": {"test": False}}, } @@ -43,7 +43,7 @@ def test_check_perms_set_owner_test_true(test_file): "name": str(test_file), "result": None, } - with patch.dict(win_dacl.__opts__, {"test": True}): + with patch.dict(win_file.__opts__, {"test": True}): result = win_file.check_perms( path=str(test_file), owner="Backup Operators", inheritance=None ) @@ -76,7 +76,7 @@ def test_check_perms_deny_test_true(test_file): "name": str(test_file), "result": None, } - with patch.dict(win_dacl.__opts__, {"test": True}): + with patch.dict(win_file.__opts__, {"test": True}): result = win_file.check_perms( path=str(test_file), deny_perms={"Users": {"perms": "read_execute"}}, @@ -113,7 +113,7 @@ def test_check_perms_grant_test_true(test_file): "name": str(test_file), "result": None, } - with patch.dict(win_dacl.__opts__, {"test": True}): + with patch.dict(win_file.__opts__, {"test": True}): result = win_file.check_perms( path=str(test_file), grant_perms={"Users": {"perms": "read_execute"}}, @@ -150,7 +150,7 @@ def test_check_perms_inheritance_false_test_true(test_file): "name": str(test_file), "result": None, } - with patch.dict(win_dacl.__opts__, {"test": True}): + with patch.dict(win_file.__opts__, {"test": True}): result = win_file.check_perms(path=str(test_file), inheritance=False) assert result == expected @@ -214,7 +214,7 @@ def test_check_perms_reset_test_true(test_file): "name": str(test_file), "result": None, } - with patch.dict(win_dacl.__opts__, {"test": True}): + with patch.dict(win_file.__opts__, {"test": True}): result = win_file.check_perms( path=str(test_file), grant_perms={ diff --git a/tests/pytests/functional/states/file/test_directory_win.py b/tests/pytests/functional/states/file/test_directory_win.py index f8a58dc4fd0..685f48195c3 100644 --- a/tests/pytests/functional/states/file/test_directory_win.py +++ b/tests/pytests/functional/states/file/test_directory_win.py @@ -2,8 +2,6 @@ import os import pytest -import salt.modules.win_file as win_file -import salt.states.file as file import salt.utils.win_dacl as win_dacl import salt.utils.win_functions as win_functions @@ -20,28 +18,7 @@ pytestmark = [ ] -@pytest.fixture(scope="module") -def configure_loader_modules(): - return { - file: { - "__opts__": {"test": False}, - "__salt__": { - "file.mkdir": win_file.mkdir, - "file.check_perms": win_file.check_perms, - }, - }, - win_file: { - "__utils__": { - "dacl.check_perms": win_dacl.check_perms, - "dacl.set_owner": win_dacl.set_owner, - "dacl.set_perms": win_dacl.set_perms, - }, - }, - win_dacl: {"__opts__": {"test": False}}, - } - - -def test_directory_new(tmp_path): +def test_directory_new(file, tmp_path): """ Test file.directory when the directory does not exist Should just return "New Dir" @@ -107,7 +84,7 @@ def test_directory_new(tmp_path): assert permissions == expected -def test_directory_new_no_inherit(tmp_path): +def test_directory_new_no_inherit(file, tmp_path): """ Test file.directory when the directory does not exist Should just return "New Dir" @@ -127,7 +104,7 @@ def test_directory_new_no_inherit(tmp_path): assert permissions["Inherited"] == {} -def test_directory_new_reset(tmp_path): +def test_directory_new_reset(file, tmp_path): """ Test file.directory when the directory does not exist Should just return "New Dir" @@ -182,7 +159,7 @@ def test_directory_new_reset(tmp_path): assert permissions == expected -def test_directory_new_reset_no_inherit(tmp_path): +def test_directory_new_reset_no_inherit(file, tmp_path): """ Test file.directory when the directory does not exist Should just return "New Dir" @@ -219,7 +196,7 @@ def test_directory_new_reset_no_inherit(tmp_path): assert permissions == expected -def test_directory_existing(tmp_path): +def test_directory_existing(file, tmp_path): path = str(tmp_path) ret = file.directory( name=path, @@ -293,7 +270,7 @@ def test_directory_existing(tmp_path): assert permissions == expected -def test_directory_existing_existing_user(tmp_path): +def test_directory_existing_existing_user(file, tmp_path): path = str(tmp_path) win_dacl.set_permissions( obj_name=path, @@ -374,7 +351,7 @@ def test_directory_existing_existing_user(tmp_path): assert permissions == expected -def test_directory_existing_no_inherit(tmp_path): +def test_directory_existing_no_inherit(file, tmp_path): path = str(tmp_path) ret = file.directory( name=path, @@ -398,7 +375,7 @@ def test_directory_existing_no_inherit(tmp_path): assert permissions["Inherited"] == {} -def test_directory_existing_reset(tmp_path): +def test_directory_existing_reset(file, tmp_path): path = str(tmp_path) win_dacl.set_permissions( obj_name=path, @@ -462,7 +439,7 @@ def test_directory_existing_reset(tmp_path): assert permissions == expected -def test_directory_existing_reset_no_inherit(tmp_path): +def test_directory_existing_reset_no_inherit(file, tmp_path): path = str(tmp_path) ret = file.directory( name=path, diff --git a/tests/pytests/functional/states/test_reg.py b/tests/pytests/functional/states/test_reg.py index 10a4d155aa6..def550f55ac 100644 --- a/tests/pytests/functional/states/test_reg.py +++ b/tests/pytests/functional/states/test_reg.py @@ -49,7 +49,6 @@ def configure_loader_modules(): "dacl.check_perms": win_dacl.check_perms, }, }, - win_dacl: {"__opts__": {"test": False}}, } diff --git a/tests/pytests/functional/utils/win_dacl/test_file.py b/tests/pytests/functional/utils/win_dacl/test_file.py index bb6bd123869..7de08f03422 100644 --- a/tests/pytests/functional/utils/win_dacl/test_file.py +++ b/tests/pytests/functional/utils/win_dacl/test_file.py @@ -1,7 +1,6 @@ import pytest import salt.utils.win_dacl as win_dacl -from tests.support.mock import patch pytestmark = [ pytest.mark.windows_whitelisted, @@ -819,22 +818,22 @@ def test_check_perms(test_file): def test_check_perms_test_true(test_file): - with patch.dict(win_dacl.__opts__, {"test": True}): - result = win_dacl.check_perms( - obj_name=str(test_file), - obj_type="file", - ret=None, - owner="Users", - grant_perms={"Backup Operators": {"perms": "read"}}, - deny_perms={ - "NETWORK SERVICE": { - "perms": ["delete", "set_value", "write_dac", "write_owner"] - }, - "Backup Operators": {"perms": ["delete"]}, + result = win_dacl.check_perms( + obj_name=str(test_file), + obj_type="file", + ret=None, + owner="Users", + grant_perms={"Backup Operators": {"perms": "read"}}, + deny_perms={ + "NETWORK SERVICE": { + "perms": ["delete", "set_value", "write_dac", "write_owner"] }, - inheritance=True, - reset=False, - ) + "Backup Operators": {"perms": ["delete"]}, + }, + inheritance=True, + reset=False, + test_mode=True, + ) expected = { "changes": { diff --git a/tests/pytests/functional/utils/win_dacl/test_reg.py b/tests/pytests/functional/utils/win_dacl/test_reg.py index af6de169e6c..870b9765ad6 100644 --- a/tests/pytests/functional/utils/win_dacl/test_reg.py +++ b/tests/pytests/functional/utils/win_dacl/test_reg.py @@ -3,7 +3,6 @@ from saltfactories.utils import random_string import salt.utils.win_dacl as win_dacl import salt.utils.win_reg as win_reg -from tests.support.mock import patch pytestmark = [ pytest.mark.windows_whitelisted, @@ -12,15 +11,6 @@ pytestmark = [ ] -@pytest.fixture -def configure_loader_modules(minion_opts): - return { - win_dacl: { - "__opts__": minion_opts, - }, - } - - @pytest.fixture(scope="module") def fake_key(): return "SOFTWARE\\{}".format(random_string("SaltTesting-", lowercase=False)) @@ -433,22 +423,22 @@ def test_check_perms(reg_key): def test_check_perms_test_true(reg_key): - with patch.dict(win_dacl.__opts__, {"test": True}): - result = win_dacl.check_perms( - obj_name=reg_key, - obj_type="registry", - ret=None, - owner="Users", - grant_perms={"Backup Operators": {"perms": "read"}}, - deny_perms={ - "NETWORK SERVICE": { - "perms": ["delete", "set_value", "write_dac", "write_owner"] - }, - "Backup Operators": {"perms": ["delete"]}, + result = win_dacl.check_perms( + obj_name=reg_key, + obj_type="registry", + ret=None, + owner="Users", + grant_perms={"Backup Operators": {"perms": "read"}}, + deny_perms={ + "NETWORK SERVICE": { + "perms": ["delete", "set_value", "write_dac", "write_owner"] }, - inheritance=True, - reset=False, - ) + "Backup Operators": {"perms": ["delete"]}, + }, + inheritance=True, + reset=False, + test_mode=True, + ) expected = { "changes": { diff --git a/tests/pytests/unit/modules/file/test_file_block_replace.py b/tests/pytests/unit/modules/file/test_file_block_replace.py index 577e6004a42..71e2d970895 100644 --- a/tests/pytests/unit/modules/file/test_file_block_replace.py +++ b/tests/pytests/unit/modules/file/test_file_block_replace.py @@ -57,7 +57,10 @@ def configure_loader_modules(): ret.update( { win_dacl: {"__opts__": opts}, - win_file: {"__utils__": {"dacl.check_perms": win_dacl.check_perms}}, + win_file: { + "__utils__": {"dacl.check_perms": win_dacl.check_perms}, + "__opts__": opts, + }, } ) diff --git a/tests/pytests/unit/modules/test_win_file.py b/tests/pytests/unit/modules/test_win_file.py index ea0f6a547f3..83667bb6377 100644 --- a/tests/pytests/unit/modules/test_win_file.py +++ b/tests/pytests/unit/modules/test_win_file.py @@ -97,7 +97,7 @@ def test_get_pgid(): something """ result = win_file.get_pgid(os.getenv("COMSPEC")) - assert not result == "" + assert result != "" def test_group_to_gid(): @@ -144,7 +144,7 @@ def test_get_uid(): something """ result = win_file.get_uid(os.getenv("WINDIR")) - assert not result == "" + assert result != "" def test_get_uid_error(): @@ -259,7 +259,7 @@ def test_get_user(): something """ result = win_file.get_user(os.getenv("WINDIR")) - assert not result == "" + assert result != "" def test_get_user_error(): diff --git a/tests/unit/modules/test_heat.py b/tests/unit/modules/test_heat.py index 9520085bddf..4aaec60eecd 100644 --- a/tests/unit/modules/test_heat.py +++ b/tests/unit/modules/test_heat.py @@ -4,7 +4,6 @@ import salt.modules.file as file_ import salt.modules.heat as heat import salt.modules.win_file as win_file import salt.utils.platform -import salt.utils.win_dacl as dacl from tests.support.mixins import LoaderModuleMockMixin from tests.support.mock import MagicMock, patch from tests.support.runtests import RUNTIME_VARS @@ -78,10 +77,7 @@ class HeatTestCase(TestCase, LoaderModuleMockMixin): "config.backup_mode": MagicMock(return_value=False), }, }, - win_file: { - "__utils__": {"dacl.check_perms": salt.utils.win_dacl.check_perms} - }, - dacl: {"__opts__": {"test": False}}, + win_file: {"__opts__": {"test": False}}, } def setUp(self): diff --git a/tests/unit/states/test_heat.py b/tests/unit/states/test_heat.py index c36bc2e98b5..b6b644a665f 100644 --- a/tests/unit/states/test_heat.py +++ b/tests/unit/states/test_heat.py @@ -5,7 +5,6 @@ import salt.modules.heat import salt.modules.win_file as win_file import salt.states.heat as heat import salt.utils.platform -import salt.utils.win_dacl as dacl import tests.unit.modules.test_heat from tests.support.mixins import LoaderModuleMockMixin from tests.support.mock import MagicMock, patch @@ -38,10 +37,7 @@ class HeatTestCase(TestCase, LoaderModuleMockMixin): "config.backup_mode": MagicMock(return_value=False), }, }, - win_file: { - "__utils__": {"dacl.check_perms": salt.utils.win_dacl.check_perms} - }, - dacl: {"__opts__": {"test": False}}, + win_file: {"__opts__": {"test": False}}, } def setUp(self): From 9a3b2bd316f098e7fd4995c8ca308b43a9775e7c Mon Sep 17 00:00:00 2001 From: David Murphy < dmurphy@saltstack.com> Date: Mon, 23 Oct 2023 14:50:15 -0600 Subject: [PATCH 08/14] Added pragma no cover since fundamental import --- salt/_compat.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/salt/_compat.py b/salt/_compat.py index ed404e877b0..7d20691f594 100644 --- a/salt/_compat.py +++ b/salt/_compat.py @@ -4,6 +4,8 @@ Salt compatibility code # pylint: disable=unused-import import sys +# pragma: no cover + # The ipaddress module included in Salt is from Python 3.9.5. # When running from Py3.9.5+ use the standard library module, use ours otherwise if sys.version_info >= (3, 9, 5): From 58564f5882a823c6cdd48d104c77d2b7c6b932f5 Mon Sep 17 00:00:00 2001 From: Pedro Algarvio Date: Sun, 22 Oct 2023 12:20:02 +0100 Subject: [PATCH 09/14] Skip test that hangs on PhotonOS 3 Signed-off-by: Pedro Algarvio --- tests/pytests/unit/cloud/test_map.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/tests/pytests/unit/cloud/test_map.py b/tests/pytests/unit/cloud/test_map.py index 7a6d3f5fdca..93d3cde94f5 100644 --- a/tests/pytests/unit/cloud/test_map.py +++ b/tests/pytests/unit/cloud/test_map.py @@ -99,10 +99,12 @@ def salt_cloud_config_file(salt_master_factory): return os.path.join(salt_master_factory.config_dir, "cloud") -def test_cloud_map_merge_conf(salt_cloud_config_file): +def test_cloud_map_merge_conf(salt_cloud_config_file, grains): """ Ensure that nested values can be selectivly overridden in a map file """ + if grains["os"] == "VMware Photon OS" and grains["osmajorrelease"] == 3: + pytest.skip("Test hangs on PhotonOS 3") with patch( "salt.config.check_driver_dependencies", MagicMock(return_value=True) ), patch("salt.cloud.Map.read", MagicMock(return_value=EXAMPLE_MAP)): From 468e6b5763657c513455419b7f9e66d33d770fe9 Mon Sep 17 00:00:00 2001 From: Pedro Algarvio Date: Sat, 21 Oct 2023 18:39:32 +0100 Subject: [PATCH 10/14] Avoid having to resolve DNS for a unittest Signed-off-by: Pedro Algarvio --- tests/pytests/unit/cloud/test_map.py | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/tests/pytests/unit/cloud/test_map.py b/tests/pytests/unit/cloud/test_map.py index 93d3cde94f5..06f71b6d6e5 100644 --- a/tests/pytests/unit/cloud/test_map.py +++ b/tests/pytests/unit/cloud/test_map.py @@ -18,7 +18,7 @@ EXAMPLE_PROVIDERS = { "vmware": { "driver": "vmware", "password": "123456", - "url": "vca1.saltstack.com", + "url": "vca1.localhost", "minion": {"master": "providermaster", "grains": {"providergrain": True}}, "profiles": {}, "user": "root", @@ -31,7 +31,7 @@ EXAMPLE_PROVIDERS = { "profiles": {}, "minion": {"master": "providermaster", "grains": {"providergrain": True}}, "image": "rhel6_64prod", - "url": "vca2.saltstack.com", + "url": "vca2.localhost", "user": "root", } }, @@ -160,7 +160,7 @@ def test_cloud_map_merge_conf(salt_cloud_config_file, grains): "profile": "nyc-vm", "provider": "nyc_vcenter:vmware", "resourcepool": "Resources", - "url": "vca1.saltstack.com", + "url": "vca1.localhost", "user": "root", }, "db2": { @@ -198,7 +198,7 @@ def test_cloud_map_merge_conf(salt_cloud_config_file, grains): "profile": "nyc-vm", "provider": "nj_vcenter:vmware", "resourcepool": "Resources", - "url": "vca2.saltstack.com", + "url": "vca2.localhost", "user": "root", }, "db3": { @@ -218,7 +218,7 @@ def test_cloud_map_merge_conf(salt_cloud_config_file, grains): "profile": "nj-vm", "provider": "nj_vcenter:vmware", "resourcepool": "Resources", - "url": "vca2.saltstack.com", + "url": "vca2.localhost", "user": "root", }, } From bb86351eca998913e428b406b748313dc0b50a0c Mon Sep 17 00:00:00 2001 From: Pedro Algarvio Date: Tue, 24 Oct 2023 21:26:49 +0100 Subject: [PATCH 11/14] Fix variable name Signed-off-by: Pedro Algarvio --- tests/integration/modules/test_mac_power.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/integration/modules/test_mac_power.py b/tests/integration/modules/test_mac_power.py index e8aa0f535f3..aa0de266707 100644 --- a/tests/integration/modules/test_mac_power.py +++ b/tests/integration/modules/test_mac_power.py @@ -230,7 +230,7 @@ class MacPowerModuleTestRestartPowerFailure(ModuleCase): Reset to original value """ if self.RESTART_POWER is not None: - self.run_function("power.set_sleep_on_power_button", [self.SLEEP_ON_BUTTON]) + self.run_function("power.set_sleep_on_power_button", [self.RESTART_POWER]) def test_restart_power_failure(self): """ From 6cb08d68b474df93b670cc42086d7d76d4e53aad Mon Sep 17 00:00:00 2001 From: Pedro Algarvio Date: Mon, 23 Oct 2023 13:04:48 +0100 Subject: [PATCH 12/14] Fix test groups issue when re-running test failures Signed-off-by: Pedro Algarvio --- tests/conftest.py | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/tests/conftest.py b/tests/conftest.py index 4fb52ea090e..edfa61ad422 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -813,6 +813,17 @@ def groups_collection_modifyitems(config, items): # Just one group, don't do any filtering return + terminal_reporter = config.pluginmanager.get_plugin("terminalreporter") + + if config.getoption("--last-failed") or config.getoption("--failed-first"): + # This is a test failure rerun, applying test groups would break this + terminal_reporter.write( + "\nNot splitting collected tests into chunks since --lf/--last-failed or " + "-ff/--failed-first was passed on the CLI.\n", + yellow=True, + ) + return + total_items = len(items) # Devide into test groups @@ -828,7 +839,6 @@ def groups_collection_modifyitems(config, items): if deselected: config.hook.pytest_deselected(items=deselected) - terminal_reporter = config.pluginmanager.get_plugin("terminalreporter") terminal_reporter.write( f"Running test group #{group_id}(out of #{group_count}) ({len(items)} out of {total_items} tests)\n", yellow=True, From 841486c9760d8c709d1d80df50b084e88190171c Mon Sep 17 00:00:00 2001 From: Thomas Phipps Date: Wed, 11 Oct 2023 22:31:40 +0000 Subject: [PATCH 13/14] add code coverage for salt/payload.py --- tests/pytests/functional/test_payload.py | 14 +++- tests/pytests/unit/test_payload.py | 93 ++++++++++++++++++++++++ 2 files changed, 106 insertions(+), 1 deletion(-) diff --git a/tests/pytests/functional/test_payload.py b/tests/pytests/functional/test_payload.py index 9d069eb0f4a..e39e1ecc0fd 100644 --- a/tests/pytests/functional/test_payload.py +++ b/tests/pytests/functional/test_payload.py @@ -140,4 +140,16 @@ def test_destroy(sreq, echo_server): """ # ensure no exceptions when we go to destroy the sreq, since __del__ # swallows exceptions, we have to call destroy directly - sreq.destroy() + assert sreq.send("clear", "foo") == {"enc": "clear", "load": "foo"} + try: + sreq.destroy() + except Exception as exc: # pylint: disable=broad-except + pytest.fail(f"sreq.destroy threw an exception {exc}") + + +@pytest.mark.slow_test +def test_clear_socket(sreq, echo_server): + assert sreq.send("clear", "foo") == {"enc": "clear", "load": "foo"} + assert hasattr(sreq, "_socket") + sreq.clear_socket() + assert hasattr(sreq, "_socket") is False diff --git a/tests/pytests/unit/test_payload.py b/tests/pytests/unit/test_payload.py index ecd77ab4fcf..f35442c7960 100644 --- a/tests/pytests/unit/test_payload.py +++ b/tests/pytests/unit/test_payload.py @@ -7,8 +7,11 @@ import copy import datetime import logging +import zmq + import salt.exceptions import salt.payload +import salt.utils.msgpack from salt.defaults import _Constant from salt.utils import immutabletypes from salt.utils.odict import OrderedDict @@ -210,3 +213,93 @@ def test_constants(): sdata = salt.payload.dumps(constant) odata = salt.payload.loads(sdata) assert odata == constant + + +def test_package(): + value = salt.utils.msgpack.dumps("test") + assert value == salt.payload.package("test") + + +def test_unpackage(): + value = [b"test"] + packed = salt.utils.msgpack.dumps(value) + assert value == salt.payload.unpackage(packed) + + +def test_format_payload(): + expected = salt.utils.msgpack.dumps( + {"enc": [b"test"], "load": {"kwargs": {"foo": "bar"}}} + ) + enc = [b"test"] + kwargs = {"foo": "bar"} + payload = salt.payload.format_payload(enc=enc, kwargs=kwargs) + assert expected == payload + + +def test_SREQ_init(): + req = salt.payload.SREQ( + "tcp://salt:3434", id_=b"id", serial="msgpack", linger=1, opts=None + ) + assert req.master == "tcp://salt:3434" + assert req.id_ == b"id" + assert req.linger == 1 + assert req.opts is None + assert isinstance(req.context, zmq.Context) + assert isinstance(req.poller, zmq.Poller) + + +def test_SREQ_socket(): + req = salt.payload.SREQ( + "tcp://salt:3434", id_=b"id", serial="msgpack", linger=1, opts=None + ) + # socket() is a property that auto creates a socket if a socket is wanted. + socket = req.socket + assert isinstance(socket, zmq.Socket) + + req = salt.payload.SREQ( + "tcp://[2001:db8:85a3:8d3:1319:8a2e:370:7348]:3434", + id_=b"id", + serial="msgpack", + linger=1, + opts=None, + ) + # socket() is a property that auto creates a socket if a socket is wanted. + socket = req.socket + assert isinstance(socket, zmq.Socket) + + req = salt.payload.SREQ( + "tcp://salt:3434", id_=None, serial="msgpack", linger=1, opts=None + ) + # socket() is a property that auto creates a socket if a socket is wanted. + socket = req.socket + assert isinstance(socket, zmq.Socket) + + +def test_SREQ_set_tcp_keepalive(): + opts = {"tcp_keepalive": True} + req = salt.payload.SREQ( + "tcp://salt:3434", id_=b"id", serial="msgpack", linger=1, opts=opts + ) + socket = req.socket + assert req._socket.getsockopt(zmq.TCP_KEEPALIVE) + + opts = {"tcp_keepalive_idle": 100} + req = salt.payload.SREQ( + "tcp://salt:3434", id_=b"id", serial="msgpack", linger=1, opts=opts + ) + socket = req.socket + assert req._socket.getsockopt(zmq.TCP_KEEPALIVE_IDLE) == 100 + + opts = {"tcp_keepalive_cnt": 100} + req = salt.payload.SREQ( + "tcp://salt:3434", id_=b"id", serial="msgpack", linger=1, opts=opts + ) + socket = req.socket + assert req._socket.getsockopt(zmq.TCP_KEEPALIVE_CNT) == 100 + + opts = {"tcp_keepalive_intvl": 100} + req = salt.payload.SREQ( + "tcp://salt:3434", id_=b"id", serial="msgpack", linger=1, opts=opts + ) + socket = req.socket + assert req._socket.getsockopt(zmq.TCP_KEEPALIVE_INTVL) == 100 From 38725e27d7c5e9c871c9a1ea939759dd845a3516 Mon Sep 17 00:00:00 2001 From: Thomas Phipps Date: Mon, 16 Oct 2023 02:47:24 +0000 Subject: [PATCH 14/14] revert the destroy, and use s0undt3chs advise --- tests/pytests/functional/test_payload.py | 11 ++++++----- tests/pytests/unit/test_payload.py | 6 +++--- 2 files changed, 9 insertions(+), 8 deletions(-) diff --git a/tests/pytests/functional/test_payload.py b/tests/pytests/functional/test_payload.py index e39e1ecc0fd..8dcd3fa1a56 100644 --- a/tests/pytests/functional/test_payload.py +++ b/tests/pytests/functional/test_payload.py @@ -138,17 +138,18 @@ def test_destroy(sreq, echo_server): """ Test the __del__ capabilities """ + # ensure we actually have an open socket and not just testing against + # no actual sockets created. + assert sreq.send("clear", "foo") == {"enc": "clear", "load": "foo"} # ensure no exceptions when we go to destroy the sreq, since __del__ # swallows exceptions, we have to call destroy directly - assert sreq.send("clear", "foo") == {"enc": "clear", "load": "foo"} - try: - sreq.destroy() - except Exception as exc: # pylint: disable=broad-except - pytest.fail(f"sreq.destroy threw an exception {exc}") + sreq.destroy() @pytest.mark.slow_test def test_clear_socket(sreq, echo_server): + # ensure we actually have an open socket and not just testing against + # no actual sockets created. assert sreq.send("clear", "foo") == {"enc": "clear", "load": "foo"} assert hasattr(sreq, "_socket") sreq.clear_socket() diff --git a/tests/pytests/unit/test_payload.py b/tests/pytests/unit/test_payload.py index f35442c7960..cf862e256b8 100644 --- a/tests/pytests/unit/test_payload.py +++ b/tests/pytests/unit/test_payload.py @@ -217,13 +217,13 @@ def test_constants(): def test_package(): value = salt.utils.msgpack.dumps("test") - assert value == salt.payload.package("test") + assert salt.payload.package("test") == value def test_unpackage(): value = [b"test"] packed = salt.utils.msgpack.dumps(value) - assert value == salt.payload.unpackage(packed) + assert salt.payload.unpackage(packed) == value def test_format_payload(): @@ -233,7 +233,7 @@ def test_format_payload(): enc = [b"test"] kwargs = {"foo": "bar"} payload = salt.payload.format_payload(enc=enc, kwargs=kwargs) - assert expected == payload + assert payload == expected def test_SREQ_init():