From 589537902e7653c1544cbfd1a6533a034dad6d44 Mon Sep 17 00:00:00 2001 From: AldarisPale <30774432+AldarisPale@users.noreply.github.com> Date: Thu, 5 Nov 2020 00:28:04 +0200 Subject: [PATCH 01/16] Make extmod not found error message mean more When module is places in the incorrect directory, the loading of the module will fail. Give more meaningful error message and a possible solution to the problem. I've hit this problem personally and debugging it took more than 3 hours. --- salt/pillar/__init__.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/salt/pillar/__init__.py b/salt/pillar/__init__.py index cb9103078711..403afb2700ed 100644 --- a/salt/pillar/__init__.py +++ b/salt/pillar/__init__.py @@ -1212,7 +1212,7 @@ def ext_pillar(self, pillar, errors=None): for key, val in run.items(): if key not in self.ext_pillars: log.critical( - "Specified ext_pillar interface %s is unavailable", key + "ext_pillar interface named %s is unavailable. Make sure it is placed in the correct directory/location. Check https://docs.saltstack.com/en/latest/ref/configuration/master.html#extension-modules for details.", key ) continue try: From bfc78d7646fd12443337d5840dfb2927dd889f37 Mon Sep 17 00:00:00 2001 From: "Daniel A. Wozniak" Date: Sun, 10 Dec 2023 17:52:44 -0700 Subject: [PATCH 02/16] Fix pre-commit --- salt/pillar/__init__.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/salt/pillar/__init__.py b/salt/pillar/__init__.py index 403afb2700ed..6899f8516790 100644 --- a/salt/pillar/__init__.py +++ b/salt/pillar/__init__.py @@ -1212,7 +1212,9 @@ def ext_pillar(self, pillar, errors=None): for key, val in run.items(): if key not in self.ext_pillars: log.critical( - "ext_pillar interface named %s is unavailable. Make sure it is placed in the correct directory/location. Check https://docs.saltstack.com/en/latest/ref/configuration/master.html#extension-modules for details.", key + "ext_pillar interface named %s is unavailable. Make sure it is placed in the correct " + "directory/location. Check https://docs.saltstack.com/en/latest/ref/configuration/master.html#extension-modules for details.", + key, ) continue try: From 2aa213123b85e657b2b34736678d364df06caa27 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pablo=20Su=C3=A1rez=20Hern=C3=A1ndez?= Date: Tue, 9 Jul 2024 13:29:51 +0100 Subject: [PATCH 03/16] Add changelog entry --- changelog/61235.fixed.md | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog/61235.fixed.md diff --git a/changelog/61235.fixed.md b/changelog/61235.fixed.md new file mode 100644 index 000000000000..7ae9bb40800e --- /dev/null +++ b/changelog/61235.fixed.md @@ -0,0 +1 @@ +- firewalld: normalize new rich rules before comparing to old ones From f96ecd141cc78b916e71c1aa07efa5969cf9f695 Mon Sep 17 00:00:00 2001 From: Marek Czernek Date: Mon, 13 May 2024 11:29:48 +0200 Subject: [PATCH 04/16] Normalize new rich rules before comparing to old Firewallcmd rich rule output quotes each assigned part of the rich rule, for example: rule family="ipv4" source port port="161" ... The firewalld module must first normalize the user defined rich rules to match the firewallcmd output before comparison to ensure idempotency. --- salt/states/firewalld.py | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/salt/states/firewalld.py b/salt/states/firewalld.py index d3e69560a25e..fe530674eb9b 100644 --- a/salt/states/firewalld.py +++ b/salt/states/firewalld.py @@ -376,6 +376,27 @@ def service(name, ports=None, protocols=None): return ret +def _normalize_rich_rules(rich_rules): + normalized_rules = [] + for rich_rule in rich_rules: + normalized_rule = "" + for cmd in rich_rule.split(" "): + cmd_components = cmd.split("=", 1) + if len(cmd_components) == 2: + assigned_component = cmd_components[1] + if not assigned_component.startswith( + '"' + ) and not assigned_component.endswith('"'): + if assigned_component.startswith( + "'" + ) and assigned_component.endswith("'"): + assigned_component = assigned_component[1:-1] + cmd_components[1] = f'"{assigned_component}"' + normalized_rule = f"{normalized_rule} {'='.join(cmd_components)}" + normalized_rules.append(normalized_rule.lstrip()) + return normalized_rules + + def _present( name, block_icmp=None, @@ -767,6 +788,7 @@ def _present( if rich_rules or prune_rich_rules: rich_rules = rich_rules or [] + rich_rules = _normalize_rich_rules(rich_rules) try: _current_rich_rules = __salt__["firewalld.get_rich_rules"]( name, permanent=True From bd60c9b40a02e9b181cb8d6e34e05df872bfcddf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pablo=20Su=C3=A1rez=20Hern=C3=A1ndez?= Date: Tue, 9 Jul 2024 16:42:35 +0100 Subject: [PATCH 05/16] Enhance documentation for normalization function --- salt/states/firewalld.py | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/salt/states/firewalld.py b/salt/states/firewalld.py index fe530674eb9b..d94b937ea9e3 100644 --- a/salt/states/firewalld.py +++ b/salt/states/firewalld.py @@ -377,6 +377,21 @@ def service(name, ports=None, protocols=None): def _normalize_rich_rules(rich_rules): + """ + Make sure rich rules are normalized and attributes + are quoted with double quotes so it matches the output + from firewall-cmd + + Example: + + rule family="ipv4" source address="192.168.0.0/16" port port=22 protocol=tcp accept + rule family="ipv4" source address="192.168.0.0/16" port port='22' protocol=tcp accept + rule family='ipv4' source address='192.168.0.0/16' port port='22' protocol=tcp accept + + normalized to: + + rule family="ipv4" source address="192.168.0.0/16" port port="22" protocol="tcp" accept + """ normalized_rules = [] for rich_rule in rich_rules: normalized_rule = "" From 22611256aa84601edc84d77e5f448b15b9b774f6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pablo=20Su=C3=A1rez=20Hern=C3=A1ndez?= Date: Tue, 9 Jul 2024 16:43:52 +0100 Subject: [PATCH 06/16] Add unit tests to cover rich rules normalization --- tests/pytests/unit/states/test_firewalld.py | 51 +++++++++++++++++++++ 1 file changed, 51 insertions(+) diff --git a/tests/pytests/unit/states/test_firewalld.py b/tests/pytests/unit/states/test_firewalld.py index 2f01ef6c6e06..2c01d3943fd9 100644 --- a/tests/pytests/unit/states/test_firewalld.py +++ b/tests/pytests/unit/states/test_firewalld.py @@ -59,3 +59,54 @@ def test_masquerade(): "comment": "'public' is already in the desired state.", "name": "public", } + + +@pytest.mark.parametrize( + "rich_rule", + [ + ( + [ + 'rule family="ipv4" source address="192.168.0.0/16" port port=22 protocol=tcp accept' + ] + ), + ( + [ + 'rule family="ipv4" source address="192.168.0.0/16" port port=\'22\' protocol=tcp accept' + ] + ), + ( + [ + "rule family='ipv4' source address='192.168.0.0/16' port port='22' protocol=tcp accept" + ] + ), + ], +) +def test_present_rich_rules_normalized(rich_rule): + firewalld_reload_rules = MagicMock(return_value={}) + firewalld_rich_rules = [ + 'rule family="ipv4" source address="192.168.0.0/16" port port="22" protocol="tcp" accept', + ] + + firewalld_get_zones = MagicMock( + return_value=[ + "block", + "public", + ] + ) + firewalld_get_masquerade = MagicMock(return_value=True) + firewalld_get_rich_rules = MagicMock(return_value=firewalld_rich_rules) + + __salt__ = { + "firewalld.reload_rules": firewalld_reload_rules, + "firewalld.get_zones": firewalld_get_zones, + "firewalld.get_masquerade": firewalld_get_masquerade, + "firewalld.get_rich_rules": firewalld_get_rich_rules, + } + with patch.dict(firewalld.__dict__, {"__salt__": __salt__}): + ret = firewalld.present("public", rich_rules=rich_rule) + assert ret == { + "changes": {}, + "result": True, + "comment": "'public' is already in the desired state.", + "name": "public", + } From 19cbf0952147485f395983bb6a254e0df48da945 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20=C3=81lvaro?= Date: Sun, 5 May 2024 20:47:34 +0200 Subject: [PATCH 07/16] feat(macdefaults): Improve macOS defaults support --- salt/modules/macdefaults.py | 300 +++++++++++++++-- salt/states/macdefaults.py | 71 ++-- .../pytests/unit/modules/test_macdefaults.py | 310 ++++++++++++++++-- tests/pytests/unit/states/test_macdefaults.py | 6 +- 4 files changed, 609 insertions(+), 78 deletions(-) diff --git a/salt/modules/macdefaults.py b/salt/modules/macdefaults.py index 77f0e580afc8..5df22a51734a 100644 --- a/salt/modules/macdefaults.py +++ b/salt/modules/macdefaults.py @@ -1,11 +1,21 @@ """ -Set defaults on Mac OS +Set defaults on macOS. + +This module uses defaults cli under the hood to read and write defaults on macOS. + +So the module is limited to the capabilities of the defaults command. + +Read macOS defaults help page for more information on the defaults command. """ import logging +import re +import salt.utils.data import salt.utils.platform +import salt.utils.versions +from salt.exceptions import CommandExecutionError log = logging.getLogger(__name__) __virtualname__ = "macdefaults" @@ -13,16 +23,21 @@ def __virtual__(): """ - Only work on Mac OS + Only works on macOS + """ if salt.utils.platform.is_darwin(): return __virtualname__ return False -def write(domain, key, value, type="string", user=None): +def write(domain, key, value, vtype=None, user=None, type=None): """ - Write a default to the system + Write a default to the system. + + Limitations: + - There is no multi-level support for arrays and dictionaries. + - Internal values types for arrays and dictionaries cannot be specified. CLI Example: @@ -33,31 +48,58 @@ def write(domain, key, value, type="string", user=None): salt '*' macdefaults.write NSGlobalDomain ApplePersistence True type=bool domain - The name of the domain to write to + The name of the domain to write to. key - The key of the given domain to write to + The key of the given domain to write to. value - The value to write to the given key + The value to write to the given key. - type + vtype The type of value to be written, valid types are string, data, int[eger], float, bool[ean], date, array, array-add, dict, dict-add - user - The user to write the defaults to + type + Deprecated! Use vtype instead. + type collides with Python's built-in type() function. + This parameter will be removed in 3009. + user + The user to write the defaults to. """ - if type == "bool" or type == "boolean": - if value is True: - value = "TRUE" - elif value is False: - value = "FALSE" - - cmd = f'defaults write "{domain}" "{key}" -{type} "{value}"' - return __salt__["cmd.run_all"](cmd, runas=user) + if type is not None: + salt.utils.versions.warn_until( + 3009, + "The 'type' argument in macdefaults.write is deprecated. Use 'vtype' instead.", + ) + if vtype is None: + vtype = type + else: + log.warning( + "The 'vtype' argument in macdefaults.write takes precedence over 'type'." + ) + + if vtype is None: + vtype = "string" + + if vtype in ("bool", "boolean"): + value = _convert_to_defaults_boolean(value) + + if isinstance(value, dict): + value = list((k, v) for k, v in value.items()) + value = salt.utils.data.flatten(value) + elif isinstance(value, (int, float, bool, str)): + value = [value] + elif not isinstance(value, list): + raise ValueError("Value must be a list, dict, int, float, bool, or string") + + # Quote values that are not integers or floats + value = map(lambda v: str(v) if isinstance(v, (int, float)) else f'"{v}"', value) + + cmd = f'write "{domain}" "{key}" -{vtype} {" ".join(value)}' + return _run_defaults_cmd(cmd, runas=user) def read(domain, key, user=None): @@ -82,8 +124,21 @@ def read(domain, key, user=None): The user to read the defaults as """ - cmd = f'defaults read "{domain}" "{key}"' - return __salt__["cmd.run"](cmd, runas=user) + cmd = f'read "{domain}" "{key}"' + ret = _run_defaults_cmd(cmd, runas=user) + + if ret["retcode"] != 0: + if "does not exist" in ret["stderr"]: + return None + raise CommandExecutionError(f"Failed to read default: {ret['stderr']}") + + # Type cast the value + try: + vtype = read_type(domain, key, user) + except CommandExecutionError: + vtype = None + + return _default_to_python(ret["stdout"].strip(), vtype) def delete(domain, key, user=None): @@ -108,5 +163,206 @@ def delete(domain, key, user=None): The user to delete the defaults with """ - cmd = f'defaults delete "{domain}" "{key}"' - return __salt__["cmd.run_all"](cmd, runas=user, output_loglevel="debug") + cmd = f'delete "{domain}" "{key}"' + return _run_defaults_cmd(cmd, runas=user) + + +def read_type(domain, key, user=None): + """ + Read the type of the given type. + If the given key is not found, then return None. + + CLI Example: + + .. code-block:: bash + + salt '*' macdefaults.read-type com.apple.CrashReporter DialogType + + salt '*' macdefaults.read_type NSGlobalDomain ApplePersistence + + domain + The name of the domain to read from. + + key + The key of the given domain to read the type of. + + user + The user to read the defaults as. + + """ + cmd = f'read-type "{domain}" "{key}"' + ret = _run_defaults_cmd(cmd, runas=user) + + if ret["retcode"] != 0: + if "does not exist" in ret["stderr"]: + return None + raise CommandExecutionError(f"Failed to read type: {ret['stderr']}") + + return re.sub(r"^Type is ", "", ret["stdout"].strip()) + + +def _default_to_python(value, vtype=None): + """ + Cast a value returned by defaults in vytpe to Python type. + + CLI Example: + + .. code-block:: bash + + salt '*' macdefaults.cast_value_to_type "1" int + + salt '*' macdefaults.cast_value_to_type "1.0" float + + salt '*' macdefaults.cast_value_to_type "TRUE" bool + + value + The value to cast. + + vtype + The type to cast the value to. + + """ + if vtype in ["integer", "int"]: + return int(value) + if vtype == "float": + return float(value) + if vtype in ["boolean", "bool"]: + return value in ["1", "TRUE", "YES"] + if vtype == "array": + return _parse_defaults_array(value) + if vtype in ["dict", "dictionary"]: + return _parse_defaults_dict(value) + return value + + +def _parse_defaults_array(value): + """ + Parse an array from a string returned by `defaults read` + and returns the array content as a list. + + value + A multiline string with the array content, including the surrounding parenthesis. + + """ + lines = value.splitlines() + if not re.match(r"\s*\(", lines[0]) or not re.match(r"\s*\)", lines[-1]): + raise ValueError("Invalid array format") + + lines = lines[1:-1] + + # Remove leading and trailing spaces + lines = list(map(lambda line: line.strip(), lines)) + + # Remove trailing commas + lines = list(map(lambda line: re.sub(r",?$", "", line), lines)) + + # Remove quotes + lines = list(map(lambda line: line.strip('"'), lines)) + + # Convert to numbers if possible + lines = list(map(_convert_to_number_if_possible, lines)) + + return lines + + +def _parse_defaults_dict(value): + """ + Parse a dictionary from a string returned by `defaults read` + + value (str): + A multiline string with the dictionary content, including the surrounding curly braces. + + Returns: + dict: The dictionary content as a Python dictionary. + """ + lines = value.splitlines() + if not re.match(r"\s*\{", lines[0]) or not re.match(r"\s*\}", lines[-1]): + raise ValueError("Invalid dictionary format") + + contents = {} + lines = list(map(lambda line: line.strip(), lines[1:-1])) + for line in lines: + key, value = re.split(r"\s*=\s*", line.strip()) + if re.match(r"\s*(\(|\{)", value): + raise ValueError("Nested arrays and dictionaries are not supported") + + value = re.sub(r";?$", "", value) + contents[key] = _convert_to_number_if_possible(value.strip('"')) + + return contents + + +def _convert_to_number_if_possible(value): + """ + Convert a string to a number if possible. + + value + The string to convert. + + """ + try: + return int(value) + except ValueError: + try: + return float(value) + except ValueError: + return value + + +def _convert_to_defaults_boolean(value): + """ + Convert a boolean to a string that can be used with the defaults command. + + value + The boolean value to convert. + + """ + if value is True: + return "TRUE" + if value is False: + return "FALSE" + + BOOLEAN_ALLOWED_VALUES = ["TRUE", "YES", "FALSE", "NO"] + if value not in BOOLEAN_ALLOWED_VALUES: + msg = "Value must be a boolean or a string of " + msg += ", ".join(BOOLEAN_ALLOWED_VALUES) + raise ValueError(msg) + + return value + + +def _run_defaults_cmd(action, runas=None): + """ + Run a 'defaults' command. + + action + The action to perform with all of its parameters. + Example: 'write com.apple.CrashReporter DialogType "Server"' + + runas + The user to run the command as. + + """ + ret = __salt__["cmd.run_all"](f"defaults {action}", runas=runas) + + # Remove timestamp from stderr if found + if ret["retcode"] != 0: + ret["stderr"] = _remove_timestamp(ret["stderr"]) + + return ret + + +def _remove_timestamp(text): + """ + Remove the timestamp from the output of the defaults command. + + text + The text to remove the timestamp from. + + """ + pattern = r"\d{4}-\d{2}-\d{2} \d{2}:\d{2}:\d{2}(?:\.\d{3})?\s+defaults\[\d+\:\d+\]" + if re.match(pattern, text): + text_lines = text.strip().splitlines() + return "\n".join(text_lines[1:]) + + return text diff --git a/salt/states/macdefaults.py b/salt/states/macdefaults.py index 05e83c2a4d65..421561ecc7f4 100644 --- a/salt/states/macdefaults.py +++ b/salt/states/macdefaults.py @@ -5,6 +5,7 @@ """ import logging +import re import salt.utils.platform @@ -15,11 +16,11 @@ def __virtual__(): """ - Only work on Mac OS + Only work on macOS """ if salt.utils.platform.is_darwin(): return __virtualname__ - return (False, "Only supported on Mac OS") + return (False, "Only supported on macOS") def write(name, domain, value, vtype="string", user=None): @@ -46,30 +47,16 @@ def write(name, domain, value, vtype="string", user=None): """ ret = {"name": name, "result": True, "comment": "", "changes": {}} - def safe_cast(val, to_type, default=None): - try: - return to_type(val) - except ValueError: - return default - current_value = __salt__["macdefaults.read"](domain, name, user) + value = _cast_value(value, vtype) - if (vtype in ["bool", "boolean"]) and ( - (value in [True, "TRUE", "YES"] and current_value == "1") - or (value in [False, "FALSE", "NO"] and current_value == "0") - ): - ret["comment"] += f"{domain} {name} is already set to {value}" - elif vtype in ["int", "integer"] and safe_cast(current_value, int) == safe_cast( - value, int - ): - ret["comment"] += f"{domain} {name} is already set to {value}" - elif current_value == value: + if _compare_values(value, current_value, strict=re.match(r"-add$", vtype) is None): ret["comment"] += f"{domain} {name} is already set to {value}" else: out = __salt__["macdefaults.write"](domain, name, value, vtype, user) if out["retcode"] != 0: ret["result"] = False - ret["comment"] = "Failed to write default. {}".format(out["stdout"]) + ret["comment"] = f"Failed to write default. {out['stdout']}" else: ret["changes"]["written"] = f"{domain} {name} is set to {value}" @@ -101,3 +88,49 @@ def absent(name, domain, user=None): ret["changes"]["absent"] = f"{domain} {name} is now absent" return ret + + +def _compare_values(new, current, strict=True): + """ + Compare two values + + new + The new value to compare + + current + The current value to compare + + strict + If True, the values must be exactly the same, if False, the new value + must be in the current value + """ + if strict: + return new == current + return new in current + + +def _cast_value(value, vtype): + def safe_cast(val, to_type, default=None): + try: + return to_type(val) + except ValueError: + return default + + if vtype in ("bool", "boolean"): + if value not in [True, "TRUE", "YES", False, "FALSE", "NO"]: + raise ValueError(f"Invalid value for boolean: {value}") + return value in [True, "TRUE", "YES"] + + if vtype in ("int", "integer"): + return safe_cast(value, int) + + if vtype == "float": + return safe_cast(value, float) + + if vtype in ("dict", "dict-add"): + return safe_cast(value, dict) + + if vtype in ["array", "array-add"]: + return safe_cast(value, list) + + return value diff --git a/tests/pytests/unit/modules/test_macdefaults.py b/tests/pytests/unit/modules/test_macdefaults.py index dc0fa6495054..bf1b3c7b1fc3 100644 --- a/tests/pytests/unit/modules/test_macdefaults.py +++ b/tests/pytests/unit/modules/test_macdefaults.py @@ -1,7 +1,7 @@ import pytest import salt.modules.macdefaults as macdefaults -from tests.support.mock import MagicMock, patch +from tests.support.mock import MagicMock, call, patch @pytest.fixture @@ -9,15 +9,45 @@ def configure_loader_modules(): return {macdefaults: {}} +def test_run_defaults_cmd(): + """ + Test caling _run_defaults_cmd + """ + mock = MagicMock(return_value={"retcode": 0, "stdout": "Server", "stderr": ""}) + with patch.dict(macdefaults.__salt__, {"cmd.run_all": mock}): + result = macdefaults._run_defaults_cmd( + 'read "com.apple.CrashReporter" "DialogType"' + ) + mock.assert_called_once_with( + 'defaults read "com.apple.CrashReporter" "DialogType"', runas=None + ) + assert result == {"retcode": 0, "stdout": "Server", "stderr": ""} + + +def test_run_defaults_cmd_with_user(): + """ + Test caling _run_defaults_cmd + """ + mock = MagicMock(return_value={"retcode": 0, "stdout": "Server", "stderr": ""}) + with patch.dict(macdefaults.__salt__, {"cmd.run_all": mock}): + result = macdefaults._run_defaults_cmd( + 'read "com.apple.CrashReporter" "DialogType"', runas="frank" + ) + mock.assert_called_once_with( + 'defaults read "com.apple.CrashReporter" "DialogType"', runas="frank" + ) + assert result == {"retcode": 0, "stdout": "Server", "stderr": ""} + + def test_write_default(): """ Test writing a default setting """ - mock = MagicMock() - with patch.dict(macdefaults.__salt__, {"cmd.run_all": mock}): + mock = MagicMock(return_value={"retcode": 0}) + with patch("salt.modules.macdefaults._run_defaults_cmd", mock): macdefaults.write("com.apple.CrashReporter", "DialogType", "Server") mock.assert_called_once_with( - 'defaults write "com.apple.CrashReporter" "DialogType" -string "Server"', + 'write "com.apple.CrashReporter" "DialogType" -string "Server"', runas=None, ) @@ -26,26 +56,108 @@ def test_write_with_user(): """ Test writing a default setting with a specific user """ - mock = MagicMock() - with patch.dict(macdefaults.__salt__, {"cmd.run_all": mock}): + mock = MagicMock(return_value={"retcode": 0}) + with patch("salt.modules.macdefaults._run_defaults_cmd", mock): macdefaults.write( "com.apple.CrashReporter", "DialogType", "Server", user="frank" ) mock.assert_called_once_with( - 'defaults write "com.apple.CrashReporter" "DialogType" -string "Server"', + 'write "com.apple.CrashReporter" "DialogType" -string "Server"', runas="frank", ) -def test_write_default_boolean(): +def test_write_true_boolean(): """ - Test writing a default setting + Test writing a True boolean setting """ - mock = MagicMock() - with patch.dict(macdefaults.__salt__, {"cmd.run_all": mock}): - macdefaults.write("com.apple.CrashReporter", "Crash", True, type="boolean") + mock = MagicMock(return_value={"retcode": 0}) + with patch("salt.modules.macdefaults._run_defaults_cmd", mock): + macdefaults.write("com.apple.CrashReporter", "Crash", True, vtype="boolean") + mock.assert_called_once_with( + 'write "com.apple.CrashReporter" "Crash" -boolean "TRUE"', + runas=None, + ) + + +def test_write_false_bool(): + """ + Test writing a False boolean setting + """ + mock = MagicMock(return_value={"retcode": 0}) + with patch("salt.modules.macdefaults._run_defaults_cmd", mock): + macdefaults.write("com.apple.CrashReporter", "Crash", False, vtype="bool") + mock.assert_called_once_with( + 'write "com.apple.CrashReporter" "Crash" -bool "FALSE"', + runas=None, + ) + + +def test_write_int(): + """ + Test writing an int setting + """ + mock = MagicMock(return_value={"retcode": 0}) + with patch("salt.modules.macdefaults._run_defaults_cmd", mock): + macdefaults.write("com.apple.CrashReporter", "Crash", 1, vtype="int") + mock.assert_called_once_with( + 'write "com.apple.CrashReporter" "Crash" -int 1', + runas=None, + ) + + +def test_write_integer(): + """ + Test writing an integer setting + """ + mock = MagicMock(return_value={"retcode": 0}) + with patch("salt.modules.macdefaults._run_defaults_cmd", mock): + macdefaults.write("com.apple.CrashReporter", "Crash", 1, vtype="integer") + mock.assert_called_once_with( + 'write "com.apple.CrashReporter" "Crash" -integer 1', + runas=None, + ) + + +def test_write_float(): + """ + Test writing a float setting + """ + mock = MagicMock(return_value={"retcode": 0}) + with patch("salt.modules.macdefaults._run_defaults_cmd", mock): + macdefaults.write("com.apple.CrashReporter", "Crash", 0.85, vtype="float") + mock.assert_called_once_with( + 'write "com.apple.CrashReporter" "Crash" -float 0.85', + runas=None, + ) + + +def test_write_array(): + """ + Test writing an array setting + """ + mock = MagicMock(return_value={"retcode": 0}) + with patch("salt.modules.macdefaults._run_defaults_cmd", mock): + macdefaults.write( + "com.apple.CrashReporter", "Crash", [0.1, 0.2, 0.4], vtype="array" + ) + mock.assert_called_once_with( + 'write "com.apple.CrashReporter" "Crash" -array 0.1 0.2 0.4', + runas=None, + ) + + +def test_write_dictionary(): + """ + Test writing a dictionary setting + """ + mock = MagicMock(return_value={"retcode": 0}) + with patch("salt.modules.macdefaults._run_defaults_cmd", mock): + macdefaults.write( + "com.apple.CrashReporter", "Crash", {"foo": "bar", "baz": 0}, vtype="dict" + ) mock.assert_called_once_with( - 'defaults write "com.apple.CrashReporter" "Crash" -boolean "TRUE"', + 'write "com.apple.CrashReporter" "Crash" -dict "foo" "bar" "baz" 0', runas=None, ) @@ -54,49 +166,179 @@ def test_read_default(): """ Test reading a default setting """ - mock = MagicMock() - with patch.dict(macdefaults.__salt__, {"cmd.run": mock}): - macdefaults.read("com.apple.CrashReporter", "Crash") - mock.assert_called_once_with( - 'defaults read "com.apple.CrashReporter" "Crash"', runas=None + + def custom_run_defaults_cmd(action, runas=None): + if action == 'read-type "com.apple.CrashReporter" "Crash"': + return {"retcode": 0, "stdout": "string"} + elif action == 'read "com.apple.CrashReporter" "Crash"': + return {"retcode": 0, "stdout": "Server"} + return {"retcode": 1, "stderr": f"Unknown action: {action}", "stdout": ""} + + mock = MagicMock(side_effect=custom_run_defaults_cmd) + with patch("salt.modules.macdefaults._run_defaults_cmd", mock): + result = macdefaults.read("com.apple.CrashReporter", "Crash") + mock.assert_has_calls( + [ + call('read "com.apple.CrashReporter" "Crash"', runas=None), + call('read-type "com.apple.CrashReporter" "Crash"', runas=None), + ] ) + assert result == "Server" -def test_read_default_with_user(): +def test_read_with_user(): """ Test reading a default setting as a specific user """ - mock = MagicMock() - with patch.dict(macdefaults.__salt__, {"cmd.run": mock}): - macdefaults.read("com.apple.CrashReporter", "Crash", user="frank") - mock.assert_called_once_with( - 'defaults read "com.apple.CrashReporter" "Crash"', runas="frank" + + def custom_run_defaults_cmd(action, runas=None): + if action == 'read-type "com.apple.CrashReporter" "Crash"': + return {"retcode": 0, "stdout": "string"} + elif action == 'read "com.apple.CrashReporter" "Crash"': + return {"retcode": 0, "stdout": "Server"} + return {"retcode": 1, "stderr": f"Unknown action: {action}", "stdout": ""} + + mock = MagicMock(side_effect=custom_run_defaults_cmd) + with patch("salt.modules.macdefaults._run_defaults_cmd", mock): + result = macdefaults.read("com.apple.CrashReporter", "Crash", user="frank") + mock.assert_has_calls( + [ + call('read "com.apple.CrashReporter" "Crash"', runas="frank"), + call('read-type "com.apple.CrashReporter" "Crash"', runas="frank"), + ] + ) + assert result == "Server" + + +def test_read_integer(): + """ + Test reading an integer setting + """ + + def custom_run_defaults_cmd(action, runas=None): + if action == 'read-type "com.apple.CrashReporter" "Crash"': + return {"retcode": 0, "stdout": "integer"} + elif action == 'read "com.apple.CrashReporter" "Crash"': + return {"retcode": 0, "stdout": "12"} + return {"retcode": 1, "stderr": f"Unknown action: {action}", "stdout": ""} + + mock = MagicMock(side_effect=custom_run_defaults_cmd) + with patch("salt.modules.macdefaults._run_defaults_cmd", mock): + result = macdefaults.read("com.apple.CrashReporter", "Crash") + mock.assert_has_calls( + [ + call('read "com.apple.CrashReporter" "Crash"', runas=None), + call('read-type "com.apple.CrashReporter" "Crash"', runas=None), + ] + ) + assert result == 12 + + +def test_read_float(): + """ + Test reading a float setting + """ + + def custom_run_defaults_cmd(action, runas=None): + if action == 'read-type "com.apple.CrashReporter" "Crash"': + return {"retcode": 0, "stdout": "float"} + elif action == 'read "com.apple.CrashReporter" "Crash"': + return {"retcode": 0, "stdout": "0.85"} + return {"retcode": 1, "stderr": f"Unknown action: {action}", "stdout": ""} + + mock = MagicMock(side_effect=custom_run_defaults_cmd) + with patch("salt.modules.macdefaults._run_defaults_cmd", mock): + result = macdefaults.read("com.apple.CrashReporter", "Crash") + mock.assert_has_calls( + [ + call('read "com.apple.CrashReporter" "Crash"', runas=None), + call('read-type "com.apple.CrashReporter" "Crash"', runas=None), + ] + ) + assert result == 0.85 + + +def test_read_array(): + """ + Test reading an array setting + """ + + defaults_output = """( + element 1, + element 2, + 0.1, + 1 + )""" + + def custom_run_defaults_cmd(action, runas=None): + if action == 'read-type "com.apple.CrashReporter" "Crash"': + return {"retcode": 0, "stdout": "array"} + elif action == 'read "com.apple.CrashReporter" "Crash"': + return {"retcode": 0, "stdout": defaults_output} + return {"retcode": 1, "stderr": f"Unknown action: {action}", "stdout": ""} + + mock = MagicMock(side_effect=custom_run_defaults_cmd) + with patch("salt.modules.macdefaults._run_defaults_cmd", mock): + result = macdefaults.read("com.apple.CrashReporter", "Crash") + mock.assert_has_calls( + [ + call('read "com.apple.CrashReporter" "Crash"', runas=None), + call('read-type "com.apple.CrashReporter" "Crash"', runas=None), + ] ) + assert result == ["element 1", "element 2", 0.1, 1] + + +def test_read_dictionary(): + """ + Test reading a dictionary setting + """ + + defaults_output = """{ + keyCode = 36; + modifierFlags = 786432; + }""" + + def custom_run_defaults_cmd(action, runas=None): + if action == 'read-type "com.apple.CrashReporter" "Crash"': + return {"retcode": 0, "stdout": "dictionary"} + elif action == 'read "com.apple.CrashReporter" "Crash"': + return {"retcode": 0, "stdout": defaults_output} + return {"retcode": 1, "stderr": f"Unknown action: {action}", "stdout": ""} + + mock = MagicMock(side_effect=custom_run_defaults_cmd) + with patch("salt.modules.macdefaults._run_defaults_cmd", mock): + result = macdefaults.read("com.apple.CrashReporter", "Crash") + mock.assert_has_calls( + [ + call('read "com.apple.CrashReporter" "Crash"', runas=None), + call('read-type "com.apple.CrashReporter" "Crash"', runas=None), + ] + ) + assert result == {"keyCode": 36, "modifierFlags": 786432} def test_delete_default(): """ Test delete a default setting """ - mock = MagicMock() - with patch.dict(macdefaults.__salt__, {"cmd.run_all": mock}): + mock = MagicMock(return_value={"retcode": 0}) + with patch("salt.modules.macdefaults._run_defaults_cmd", mock): macdefaults.delete("com.apple.CrashReporter", "Crash") mock.assert_called_once_with( - 'defaults delete "com.apple.CrashReporter" "Crash"', - output_loglevel="debug", + 'delete "com.apple.CrashReporter" "Crash"', runas=None, ) -def test_delete_default_with_user(): +def test_delete_with_user(): """ - Test delete a default setting as a specific user + Test delete a setting as a specific user """ - mock = MagicMock() - with patch.dict(macdefaults.__salt__, {"cmd.run_all": mock}): + mock = MagicMock(return_value={"retcode": 0}) + with patch("salt.modules.macdefaults._run_defaults_cmd", mock): macdefaults.delete("com.apple.CrashReporter", "Crash", user="frank") mock.assert_called_once_with( - 'defaults delete "com.apple.CrashReporter" "Crash"', - output_loglevel="debug", + 'delete "com.apple.CrashReporter" "Crash"', runas="frank", ) diff --git a/tests/pytests/unit/states/test_macdefaults.py b/tests/pytests/unit/states/test_macdefaults.py index a9d402888589..43699af2426d 100644 --- a/tests/pytests/unit/states/test_macdefaults.py +++ b/tests/pytests/unit/states/test_macdefaults.py @@ -88,12 +88,12 @@ def test_write_boolean_match(): """ expected = { "changes": {}, - "comment": "com.apple.something Key is already set to YES", + "comment": "com.apple.something Key is already set to True", "name": "Key", "result": True, } - read_mock = MagicMock(return_value="1") + read_mock = MagicMock(return_value=True) write_mock = MagicMock(return_value={"retcode": 0}) with patch.dict( macdefaults.__salt__, @@ -141,7 +141,7 @@ def test_write_integer_match(): "result": True, } - read_mock = MagicMock(return_value="1337") + read_mock = MagicMock(return_value=1337) write_mock = MagicMock(return_value={"retcode": 0}) with patch.dict( macdefaults.__salt__, From 6e51f3b755f83c7a22d65de328ba16a385864a38 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20=C3=81lvaro?= Date: Mon, 6 May 2024 18:41:05 +0200 Subject: [PATCH 08/16] doc: Add changelog entry --- changelog/66466.added.md | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog/66466.added.md diff --git a/changelog/66466.added.md b/changelog/66466.added.md new file mode 100644 index 000000000000..631c4ecd5e57 --- /dev/null +++ b/changelog/66466.added.md @@ -0,0 +1 @@ +Improve macOS defaults support From aaba6250c0f22e8ae5f45cddc3b4bdf39aaf196d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20=C3=81lvaro?= Date: Wed, 8 May 2024 11:14:55 +0200 Subject: [PATCH 09/16] feat(macdefaults): Add basic support for array-add and dict-add --- salt/modules/macdefaults.py | 89 +++-- salt/states/macdefaults.py | 76 ++++- .../pytests/unit/modules/test_macdefaults.py | 63 ++-- tests/pytests/unit/states/test_macdefaults.py | 317 +++++++++++++++++- 4 files changed, 437 insertions(+), 108 deletions(-) diff --git a/salt/modules/macdefaults.py b/salt/modules/macdefaults.py index 5df22a51734a..4a457e1beb6c 100644 --- a/salt/modules/macdefaults.py +++ b/salt/modules/macdefaults.py @@ -1,11 +1,11 @@ """ -Set defaults on macOS. +Set defaults settings on macOS. This module uses defaults cli under the hood to read and write defaults on macOS. -So the module is limited to the capabilities of the defaults command. +Thus, the module is limited to the capabilities of the defaults command. -Read macOS defaults help page for more information on the defaults command. +Read macOS defaults help page for more information on defaults command. """ @@ -33,11 +33,11 @@ def __virtual__(): def write(domain, key, value, vtype=None, user=None, type=None): """ - Write a default to the system. + Write a default to the system Limitations: - - There is no multi-level support for arrays and dictionaries. - - Internal values types for arrays and dictionaries cannot be specified. + - There is no multi-level support for arrays and dictionaries + - Internal values types for arrays and dictionaries cannot be specified CLI Example: @@ -45,28 +45,28 @@ def write(domain, key, value, vtype=None, user=None, type=None): salt '*' macdefaults.write com.apple.CrashReporter DialogType Server - salt '*' macdefaults.write NSGlobalDomain ApplePersistence True type=bool + salt '*' macdefaults.write NSGlobalDomain ApplePersistence True vtype=bool domain - The name of the domain to write to. + The name of the domain to write to key - The key of the given domain to write to. + The key of the given domain to write to value - The value to write to the given key. + The value to write to the given key vtype The type of value to be written, valid types are string, data, int[eger], float, bool[ean], date, array, array-add, dict, dict-add type - Deprecated! Use vtype instead. - type collides with Python's built-in type() function. - This parameter will be removed in 3009. + Deprecated! Use vtype instead + type collides with Python's built-in type() function + This parameter will be removed in 3009 user - The user to write the defaults to. + The user to write the defaults to """ if type is not None: @@ -95,7 +95,7 @@ def write(domain, key, value, vtype=None, user=None, type=None): elif not isinstance(value, list): raise ValueError("Value must be a list, dict, int, float, bool, or string") - # Quote values that are not integers or floats + # Quote values that are neither integers nor floats value = map(lambda v: str(v) if isinstance(v, (int, float)) else f'"{v}"', value) cmd = f'write "{domain}" "{key}" -{vtype} {" ".join(value)}' @@ -169,25 +169,25 @@ def delete(domain, key, user=None): def read_type(domain, key, user=None): """ - Read the type of the given type. - If the given key is not found, then return None. + Read a default type from the system + If the key is not found, None is returned. CLI Example: .. code-block:: bash - salt '*' macdefaults.read-type com.apple.CrashReporter DialogType + salt '*' macdefaults.read_type com.apple.CrashReporter DialogType salt '*' macdefaults.read_type NSGlobalDomain ApplePersistence domain - The name of the domain to read from. + The name of the domain to read from key - The key of the given domain to read the type of. + The key of the given domain to read the type of user - The user to read the defaults as. + The user to read the defaults as """ cmd = f'read-type "{domain}" "{key}"' @@ -203,23 +203,13 @@ def read_type(domain, key, user=None): def _default_to_python(value, vtype=None): """ - Cast a value returned by defaults in vytpe to Python type. - - CLI Example: - - .. code-block:: bash - - salt '*' macdefaults.cast_value_to_type "1" int - - salt '*' macdefaults.cast_value_to_type "1.0" float - - salt '*' macdefaults.cast_value_to_type "TRUE" bool + Cast the value returned by the defaults command in vytpe to Python type value - The value to cast. + The value to cast vtype - The type to cast the value to. + The type to cast the value to """ if vtype in ["integer", "int"]: @@ -238,10 +228,10 @@ def _default_to_python(value, vtype=None): def _parse_defaults_array(value): """ Parse an array from a string returned by `defaults read` - and returns the array content as a list. + and returns the array content as a list value - A multiline string with the array content, including the surrounding parenthesis. + A multiline string with the array content, including the surrounding parenthesis """ lines = value.splitlines() @@ -268,12 +258,11 @@ def _parse_defaults_array(value): def _parse_defaults_dict(value): """ Parse a dictionary from a string returned by `defaults read` + and returns the dictionary content as a Python dictionary value (str): - A multiline string with the dictionary content, including the surrounding curly braces. + A multiline string with the dictionary content, including the surrounding curly braces - Returns: - dict: The dictionary content as a Python dictionary. """ lines = value.splitlines() if not re.match(r"\s*\{", lines[0]) or not re.match(r"\s*\}", lines[-1]): @@ -294,10 +283,10 @@ def _parse_defaults_dict(value): def _convert_to_number_if_possible(value): """ - Convert a string to a number if possible. + Convert a string to a number if possible value - The string to convert. + The string to convert """ try: @@ -311,15 +300,15 @@ def _convert_to_number_if_possible(value): def _convert_to_defaults_boolean(value): """ - Convert a boolean to a string that can be used with the defaults command. + Convert a boolean to a string that can be used with the defaults command value - The boolean value to convert. + The boolean value to convert """ - if value is True: + if value in (True, 1): return "TRUE" - if value is False: + if value in (False, 0): return "FALSE" BOOLEAN_ALLOWED_VALUES = ["TRUE", "YES", "FALSE", "NO"] @@ -333,14 +322,14 @@ def _convert_to_defaults_boolean(value): def _run_defaults_cmd(action, runas=None): """ - Run a 'defaults' command. + Run the 'defaults' command with the given action action - The action to perform with all of its parameters. + The action to perform with all of its parameters Example: 'write com.apple.CrashReporter DialogType "Server"' runas - The user to run the command as. + The user to run the command as """ ret = __salt__["cmd.run_all"](f"defaults {action}", runas=runas) @@ -354,10 +343,10 @@ def _run_defaults_cmd(action, runas=None): def _remove_timestamp(text): """ - Remove the timestamp from the output of the defaults command. + Remove the timestamp from the output of the defaults command if found text - The text to remove the timestamp from. + The text to remove the timestamp from """ pattern = r"\d{4}-\d{2}-\d{2} \d{2}:\d{2}:\d{2}(?:\.\d{3})?\s+defaults\[\d+\:\d+\]" diff --git a/salt/states/macdefaults.py b/salt/states/macdefaults.py index 421561ecc7f4..20207ec5ee64 100644 --- a/salt/states/macdefaults.py +++ b/salt/states/macdefaults.py @@ -4,13 +4,8 @@ """ -import logging -import re - import salt.utils.platform -log = logging.getLogger(__name__) - __virtualname__ = "macdefaults" @@ -43,14 +38,13 @@ def write(name, domain, value, vtype="string", user=None): user The user to write the defaults to - """ ret = {"name": name, "result": True, "comment": "", "changes": {}} current_value = __salt__["macdefaults.read"](domain, name, user) value = _cast_value(value, vtype) - if _compare_values(value, current_value, strict=re.match(r"-add$", vtype) is None): + if _compare_values(value, current_value, vtype): ret["comment"] += f"{domain} {name} is already set to {value}" else: out = __salt__["macdefaults.write"](domain, name, value, vtype, user) @@ -76,7 +70,6 @@ def absent(name, domain, user=None): user The user to write the defaults to - """ ret = {"name": name, "result": True, "comment": "", "changes": {}} @@ -90,9 +83,9 @@ def absent(name, domain, user=None): return ret -def _compare_values(new, current, strict=True): +def _compare_values(new, current, vtype): """ - Compare two values + Compare two values based on their type new The new value to compare @@ -100,24 +93,73 @@ def _compare_values(new, current, strict=True): current The current value to compare - strict - If True, the values must be exactly the same, if False, the new value - must be in the current value + vtype + The type of default value to be compared + """ - if strict: - return new == current - return new in current + if vtype == "array-add": + return _is_subarray(new, current) + if vtype == "dict-add": + return all([key in current and new[key] == current[key] for key in new.keys()]) + + return new == current + + +def _is_subarray(new, current): + """ + Check if new is a subarray of current array. + + This method does not check only whether all elements in new array + are present in current array, but also whether the elements are in + the same order. + + new + The new array to compare + + current + The current array to compare + + """ + current_len = len(current) + new_len = len(new) + + if new_len == 0: + return True + if new_len > current_len: + return False + + for i in range(current_len - new_len + 1): + # Check if the new array is found at this position + if current[i : i + new_len] == new: + return True + + return False def _cast_value(value, vtype): + """ + Cast the given macOS default value to Python type + + value + The value to cast from macOS default + + vtype + The type to cast the value from + + """ + def safe_cast(val, to_type, default=None): + """ + Auxiliary function to safely cast a value to a given type + + """ try: return to_type(val) except ValueError: return default if vtype in ("bool", "boolean"): - if value not in [True, "TRUE", "YES", False, "FALSE", "NO"]: + if value not in [True, 1, "TRUE", "YES", False, 0, "FALSE", "NO"]: raise ValueError(f"Invalid value for boolean: {value}") return value in [True, "TRUE", "YES"] diff --git a/tests/pytests/unit/modules/test_macdefaults.py b/tests/pytests/unit/modules/test_macdefaults.py index bf1b3c7b1fc3..7cf0d0a1c5f0 100644 --- a/tests/pytests/unit/modules/test_macdefaults.py +++ b/tests/pytests/unit/modules/test_macdefaults.py @@ -52,7 +52,7 @@ def test_write_default(): ) -def test_write_with_user(): +def test_write_default_with_user(): """ Test writing a default setting with a specific user """ @@ -67,9 +67,9 @@ def test_write_with_user(): ) -def test_write_true_boolean(): +def test_write_default_true_boolean(): """ - Test writing a True boolean setting + Test writing a default True boolean setting """ mock = MagicMock(return_value={"retcode": 0}) with patch("salt.modules.macdefaults._run_defaults_cmd", mock): @@ -80,9 +80,9 @@ def test_write_true_boolean(): ) -def test_write_false_bool(): +def test_write_default_false_bool(): """ - Test writing a False boolean setting + Test writing a default False boolean setting """ mock = MagicMock(return_value={"retcode": 0}) with patch("salt.modules.macdefaults._run_defaults_cmd", mock): @@ -93,9 +93,9 @@ def test_write_false_bool(): ) -def test_write_int(): +def test_write_default_int(): """ - Test writing an int setting + Test writing a default int setting """ mock = MagicMock(return_value={"retcode": 0}) with patch("salt.modules.macdefaults._run_defaults_cmd", mock): @@ -106,9 +106,9 @@ def test_write_int(): ) -def test_write_integer(): +def test_write_default_integer(): """ - Test writing an integer setting + Test writing a default integer setting """ mock = MagicMock(return_value={"retcode": 0}) with patch("salt.modules.macdefaults._run_defaults_cmd", mock): @@ -119,9 +119,9 @@ def test_write_integer(): ) -def test_write_float(): +def test_write_default_float(): """ - Test writing a float setting + Test writing a default float setting """ mock = MagicMock(return_value={"retcode": 0}) with patch("salt.modules.macdefaults._run_defaults_cmd", mock): @@ -132,9 +132,9 @@ def test_write_float(): ) -def test_write_array(): +def test_write_default_array(): """ - Test writing an array setting + Test writing a default array setting """ mock = MagicMock(return_value={"retcode": 0}) with patch("salt.modules.macdefaults._run_defaults_cmd", mock): @@ -147,9 +147,9 @@ def test_write_array(): ) -def test_write_dictionary(): +def test_write_default_dictionary(): """ - Test writing a dictionary setting + Test writing a default dictionary setting """ mock = MagicMock(return_value={"retcode": 0}) with patch("salt.modules.macdefaults._run_defaults_cmd", mock): @@ -186,7 +186,7 @@ def custom_run_defaults_cmd(action, runas=None): assert result == "Server" -def test_read_with_user(): +def test_read_default_with_user(): """ Test reading a default setting as a specific user """ @@ -210,9 +210,9 @@ def custom_run_defaults_cmd(action, runas=None): assert result == "Server" -def test_read_integer(): +def test_read_default_integer(): """ - Test reading an integer setting + Test reading a default integer setting """ def custom_run_defaults_cmd(action, runas=None): @@ -234,9 +234,9 @@ def custom_run_defaults_cmd(action, runas=None): assert result == 12 -def test_read_float(): +def test_read_default_float(): """ - Test reading a float setting + Test reading a default float setting """ def custom_run_defaults_cmd(action, runas=None): @@ -258,15 +258,15 @@ def custom_run_defaults_cmd(action, runas=None): assert result == 0.85 -def test_read_array(): +def test_read_default_array(): """ - Test reading an array setting + Test reading a default array setting """ defaults_output = """( element 1, element 2, - 0.1, + 0.1000, 1 )""" @@ -289,14 +289,16 @@ def custom_run_defaults_cmd(action, runas=None): assert result == ["element 1", "element 2", 0.1, 1] -def test_read_dictionary(): +def test_read_default_dictionary(): """ - Test reading a dictionary setting + Test reading a default dictionary setting """ defaults_output = """{ keyCode = 36; modifierFlags = 786432; + anotherKey = "another value with spaces"; + floatNumber = 0.8500; }""" def custom_run_defaults_cmd(action, runas=None): @@ -315,7 +317,12 @@ def custom_run_defaults_cmd(action, runas=None): call('read-type "com.apple.CrashReporter" "Crash"', runas=None), ] ) - assert result == {"keyCode": 36, "modifierFlags": 786432} + assert result == { + "keyCode": 36, + "modifierFlags": 786432, + "anotherKey": "another value with spaces", + "floatNumber": 0.85, + } def test_delete_default(): @@ -331,9 +338,9 @@ def test_delete_default(): ) -def test_delete_with_user(): +def test_delete_default_with_user(): """ - Test delete a setting as a specific user + Test delete a default setting as a specific user """ mock = MagicMock(return_value={"retcode": 0}) with patch("salt.modules.macdefaults._run_defaults_cmd", mock): diff --git a/tests/pytests/unit/states/test_macdefaults.py b/tests/pytests/unit/states/test_macdefaults.py index 43699af2426d..b293027ace83 100644 --- a/tests/pytests/unit/states/test_macdefaults.py +++ b/tests/pytests/unit/states/test_macdefaults.py @@ -9,7 +9,7 @@ def configure_loader_modules(): return {macdefaults: {}} -def test_write(): +def test_write_default(): """ Test writing a default setting """ @@ -34,7 +34,7 @@ def test_write(): assert out == expected -def test_write_set(): +def test_write_default_already_set(): """ Test writing a default setting that is already set """ @@ -57,7 +57,7 @@ def test_write_set(): assert out == expected -def test_write_boolean(): +def test_write_default_boolean(): """ Test writing a default setting with a boolean """ @@ -68,7 +68,7 @@ def test_write_boolean(): "result": True, } - read_mock = MagicMock(return_value="0") + read_mock = MagicMock(return_value=False) write_mock = MagicMock(return_value={"retcode": 0}) with patch.dict( macdefaults.__salt__, @@ -82,9 +82,9 @@ def test_write_boolean(): assert out == expected -def test_write_boolean_match(): +def test_write_default_boolean_already_set(): """ - Test writing a default setting with a boolean that is already set to the same value + Test writing a default setting with a boolean that is already set """ expected = { "changes": {}, @@ -105,7 +105,7 @@ def test_write_boolean_match(): assert out == expected -def test_write_integer(): +def test_write_default_integer(): """ Test writing a default setting with a integer """ @@ -116,7 +116,7 @@ def test_write_integer(): "result": True, } - read_mock = MagicMock(return_value="99") + read_mock = MagicMock(return_value=99) write_mock = MagicMock(return_value={"retcode": 0}) with patch.dict( macdefaults.__salt__, @@ -130,9 +130,9 @@ def test_write_integer(): assert out == expected -def test_write_integer_match(): +def test_write_default_integer_already_set(): """ - Test writing a default setting with a integer that is already set to the same value + Test writing a default setting with an integer that is already set """ expected = { "changes": {}, @@ -153,7 +153,298 @@ def test_write_integer_match(): assert out == expected -def test_absent_already(): +def test_write_default_float(): + """ + Test writing a default setting with a float + """ + expected = { + "changes": {"written": "com.apple.something Key is set to 0.865"}, + "comment": "", + "name": "Key", + "result": True, + } + + read_mock = MagicMock(return_value=0.4) + write_mock = MagicMock(return_value={"retcode": 0}) + with patch.dict( + macdefaults.__salt__, + {"macdefaults.read": read_mock, "macdefaults.write": write_mock}, + ): + out = macdefaults.write("Key", "com.apple.something", 0.865, vtype="float") + read_mock.assert_called_once_with("com.apple.something", "Key", None) + write_mock.assert_called_once_with( + "com.apple.something", "Key", 0.865, "float", None + ) + assert out == expected + + +def test_write_default_float_already_set(): + """ + Test writing a default setting with a float that is already set_default + """ + expected = { + "changes": {}, + "comment": "com.apple.something Key is already set to 0.865", + "name": "Key", + "result": True, + } + + read_mock = MagicMock(return_value=0.865) + write_mock = MagicMock(return_value={"retcode": 0}) + with patch.dict( + macdefaults.__salt__, + {"macdefaults.read": read_mock, "macdefaults.write": write_mock}, + ): + out = macdefaults.write("Key", "com.apple.something", 0.86500, vtype="float") + read_mock.assert_called_once_with("com.apple.something", "Key", None) + assert not write_mock.called + assert out == expected + + +def test_write_default_array(): + """ + Test writing a default setting with an array + """ + value = ["a", 1, 0.5, True] + expected = { + "changes": {"written": f"com.apple.something Key is set to {value}"}, + "comment": "", + "name": "Key", + "result": True, + } + + read_mock = MagicMock(return_value=None) + write_mock = MagicMock(return_value={"retcode": 0}) + with patch.dict( + macdefaults.__salt__, + {"macdefaults.read": read_mock, "macdefaults.write": write_mock}, + ): + out = macdefaults.write("Key", "com.apple.something", value, vtype="array") + read_mock.assert_called_once_with("com.apple.something", "Key", None) + write_mock.assert_called_once_with( + "com.apple.something", "Key", value, "array", None + ) + assert out == expected + + +def test_write_default_array_already_set(): + """ + Test writing a default setting with an array that is already set + """ + value = ["a", 1, 0.5, True] + expected = { + "changes": {}, + "comment": f"com.apple.something Key is already set to {value}", + "name": "Key", + "result": True, + } + + read_mock = MagicMock(return_value=value) + write_mock = MagicMock(return_value={"retcode": 0}) + with patch.dict( + macdefaults.__salt__, + {"macdefaults.read": read_mock, "macdefaults.write": write_mock}, + ): + out = macdefaults.write("Key", "com.apple.something", value, vtype="array") + read_mock.assert_called_once_with("com.apple.something", "Key", None) + assert not write_mock.called + assert out == expected + + +def test_write_default_array_add(): + """ + Test writing a default setting adding an array to another + """ + write_value = ["a", 1] + read_value = ["b", 2] + expected = { + "changes": {"written": f"com.apple.something Key is set to {write_value}"}, + "comment": "", + "name": "Key", + "result": True, + } + + read_mock = MagicMock(return_value=read_value) + write_mock = MagicMock(return_value={"retcode": 0}) + with patch.dict( + macdefaults.__salt__, + {"macdefaults.read": read_mock, "macdefaults.write": write_mock}, + ): + out = macdefaults.write( + "Key", "com.apple.something", write_value, vtype="array-add" + ) + read_mock.assert_called_once_with("com.apple.something", "Key", None) + write_mock.assert_called_once_with( + "com.apple.something", "Key", write_value, "array-add", None + ) + assert out == expected + + +def test_write_default_array_add_already_set_distinct_order(): + """ + Test writing a default setting adding an array to another that is already set + The new array is in a different order than the existing one + """ + write_value = ["a", 1] + read_value = ["b", 1, "a", 2] + expected = { + "changes": {"written": f"com.apple.something Key is set to {write_value}"}, + "comment": "", + "name": "Key", + "result": True, + } + + read_mock = MagicMock(return_value=read_value) + write_mock = MagicMock(return_value={"retcode": 0}) + with patch.dict( + macdefaults.__salt__, + {"macdefaults.read": read_mock, "macdefaults.write": write_mock}, + ): + out = macdefaults.write( + "Key", "com.apple.something", write_value, vtype="array-add" + ) + read_mock.assert_called_once_with("com.apple.something", "Key", None) + write_mock.assert_called_once_with( + "com.apple.something", "Key", write_value, "array-add", None + ) + assert out == expected + + +def test_write_default_array_add_already_set_same_order(): + """ + Test writing a default setting adding an array to another that is already set + The new array is already in the same order as the existing one + """ + write_value = ["a", 1] + read_value = ["b", "a", 1, 2] + expected = { + "changes": {}, + "comment": f"com.apple.something Key is already set to {write_value}", + "name": "Key", + "result": True, + } + + read_mock = MagicMock(return_value=read_value) + write_mock = MagicMock(return_value={"retcode": 0}) + with patch.dict( + macdefaults.__salt__, + {"macdefaults.read": read_mock, "macdefaults.write": write_mock}, + ): + out = macdefaults.write( + "Key", "com.apple.something", write_value, vtype="array-add" + ) + read_mock.assert_called_once_with("com.apple.something", "Key", None) + assert not write_mock.called + assert out == expected + + +def test_write_default_dict(): + """ + Test writing a default setting with a dictionary + """ + value = {"string": "bar", "integer": 1, "float": 0.5, "boolean": True} + expected = { + "changes": {"written": f"com.apple.something Key is set to {value}"}, + "comment": "", + "name": "Key", + "result": True, + } + + read_mock = MagicMock(return_value=None) + write_mock = MagicMock(return_value={"retcode": 0}) + with patch.dict( + macdefaults.__salt__, + {"macdefaults.read": read_mock, "macdefaults.write": write_mock}, + ): + out = macdefaults.write("Key", "com.apple.something", value, vtype="dict") + read_mock.assert_called_once_with("com.apple.something", "Key", None) + write_mock.assert_called_once_with( + "com.apple.something", "Key", value, "dict", None + ) + assert out == expected + + +def test_write_default_dict_already_set(): + """ + Test writing a default setting with a dictionary that is already set + """ + value = {"string": "bar", "integer": 1, "float": 0.5, "boolean": True} + expected = { + "changes": {}, + "comment": f"com.apple.something Key is already set to {value}", + "name": "Key", + "result": True, + } + + read_mock = MagicMock(return_value=value) + write_mock = MagicMock(return_value={"retcode": 0}) + with patch.dict( + macdefaults.__salt__, + {"macdefaults.read": read_mock, "macdefaults.write": write_mock}, + ): + out = macdefaults.write("Key", "com.apple.something", value, vtype="dict") + read_mock.assert_called_once_with("com.apple.something", "Key", None) + assert not write_mock.called + assert out == expected + + +def test_write_default_dict_add(): + """ + Test writing a default setting adding elements to a dictionary + """ + write_value = {"string": "bar", "integer": 1} + read_value = {"integer": 1, "float": 0.5, "boolean": True} + expected = { + "changes": {"written": f"com.apple.something Key is set to {write_value}"}, + "comment": "", + "name": "Key", + "result": True, + } + + read_mock = MagicMock(return_value=read_value) + write_mock = MagicMock(return_value={"retcode": 0}) + with patch.dict( + macdefaults.__salt__, + {"macdefaults.read": read_mock, "macdefaults.write": write_mock}, + ): + out = macdefaults.write( + "Key", "com.apple.something", write_value, vtype="dict-add" + ) + read_mock.assert_called_once_with("com.apple.something", "Key", None) + write_mock.assert_called_once_with( + "com.apple.something", "Key", write_value, "dict-add", None + ) + assert out == expected + + +def test_write_default_dict_add_already_set(): + """ + Test writing a default setting adding elements to a dictionary that is already set + """ + write_value = {"string": "bar", "integer": 1} + read_value = {"string": "bar", "integer": 1, "float": 0.5, "boolean": True} + expected = { + "changes": {}, + "comment": f"com.apple.something Key is already set to {write_value}", + "name": "Key", + "result": True, + } + + read_mock = MagicMock(return_value=read_value) + write_mock = MagicMock(return_value={"retcode": 0}) + with patch.dict( + macdefaults.__salt__, + {"macdefaults.read": read_mock, "macdefaults.write": write_mock}, + ): + out = macdefaults.write( + "Key", "com.apple.something", write_value, vtype="dict-add" + ) + read_mock.assert_called_once_with("com.apple.something", "Key", None) + assert not write_mock.called + assert out == expected + + +def test_absent_default_already(): """ Test ensuring non-existent defaults value is absent """ @@ -171,9 +462,9 @@ def test_absent_already(): assert out == expected -def test_absent_deleting_existing(): +def test_absent_default_deleting_existing(): """ - Test removing an existing value + Test removing an existing default value """ expected = { "changes": {"absent": "com.apple.something Key is now absent"}, From 7c5baf4bbe2d6439db040d655531f62c258c7d82 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20=C3=81lvaro?= Date: Wed, 8 May 2024 11:47:05 +0200 Subject: [PATCH 10/16] enh(macdefaults): Use generator instead of list --- salt/states/macdefaults.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/salt/states/macdefaults.py b/salt/states/macdefaults.py index 20207ec5ee64..617874cdbb7a 100644 --- a/salt/states/macdefaults.py +++ b/salt/states/macdefaults.py @@ -100,7 +100,7 @@ def _compare_values(new, current, vtype): if vtype == "array-add": return _is_subarray(new, current) if vtype == "dict-add": - return all([key in current and new[key] == current[key] for key in new.keys()]) + return all(key in current and new[key] == current[key] for key in new.keys()) return new == current From f5067e7853e98005cce2d7c3ffd06191cf31b26a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20=C3=81lvaro?= Date: Thu, 27 Jun 2024 11:41:58 +0200 Subject: [PATCH 11/16] feat(macdefaults): Add support for copmplex keys and nested dictionaries --- salt/modules/macdefaults.py | 417 ++++---- salt/states/macdefaults.py | 92 +- .../pytests/unit/modules/test_macdefaults.py | 946 ++++++++++++++---- tests/pytests/unit/states/test_macdefaults.py | 31 +- 4 files changed, 1051 insertions(+), 435 deletions(-) diff --git a/salt/modules/macdefaults.py b/salt/modules/macdefaults.py index 4a457e1beb6c..31688a484a23 100644 --- a/salt/modules/macdefaults.py +++ b/salt/modules/macdefaults.py @@ -1,18 +1,23 @@ """ Set defaults settings on macOS. -This module uses defaults cli under the hood to read and write defaults on macOS. +This module uses defaults cli under the hood to import and export defaults on macOS. -Thus, the module is limited to the capabilities of the defaults command. +However, it uses the plistlib package to handle the conversion between the defaults +output and Python dictionaries. It is also used to create the plist files to import +the defaults. -Read macOS defaults help page for more information on defaults command. +Read plistlib documentation for more information on how the conversion is done: +https://docs.python.org/3/library/plistlib.html """ import logging +import plistlib import re +import tempfile +from datetime import datetime -import salt.utils.data import salt.utils.platform import salt.utils.versions from salt.exceptions import CommandExecutionError @@ -31,34 +36,55 @@ def __virtual__(): return False -def write(domain, key, value, vtype=None, user=None, type=None): +def write( + domain, + key, + value, + vtype=None, + user=None, + dict_merge=False, + array_add=False, + type=None, +): """ Write a default to the system - Limitations: - - There is no multi-level support for arrays and dictionaries - - Internal values types for arrays and dictionaries cannot be specified - CLI Example: .. code-block:: bash - salt '*' macdefaults.write com.apple.CrashReporter DialogType Server + salt '*' macdefaults.write com.apple.Finder DownloadsFolderListViewSettingsVersion 1 + + salt '*' macdefaults.write com.apple.Finder ComputerViewSettings.CustomViewStyle "icnv" - salt '*' macdefaults.write NSGlobalDomain ApplePersistence True vtype=bool + salt '*' macdefaults.write com.apple.Dock lastShowIndicatorTime 737720347.089987 vtype=date domain The name of the domain to write to key - The key of the given domain to write to + The key of the given domain to write to. + It can be a nested key/index separated by dots. value - The value to write to the given key + The value to write to the given key. + Dates should be in the format 'YYYY-MM-DDTHH:MM:SSZ' vtype - The type of value to be written, valid types are string, data, int[eger], - float, bool[ean], date, array, array-add, dict, dict-add + The type of value to be written, valid types are string, int[eger], + float, bool[ean], date and data. + + dict and array are also valid types but are only used for validation. + + dict-add and array-add are supported for backward compatibility. + However, their corresponding sibling options dict_merge and array_add + are recommended. + + This parameter is optional. It will be used to cast the values to the + specified type before writing them to the system. If not provided, the + type will be inferred from the value. + + Useful when writing values such as dates or binary data. type Deprecated! Use vtype instead @@ -68,6 +94,20 @@ def write(domain, key, value, vtype=None, user=None, type=None): user The user to write the defaults to + dict_merge + Merge the value into the existing dictionary. + If current value is not a dictionary this option will be ignored. + This option will be set to True if vtype is dict-add. + + array_add + Append the value to the array. + If current value is not a list this option will be ignored. + This option will be set to True if vtype is array-add. + + Raises: + KeyError: When the key is not found in the domain + IndexError: When the key is not a valid array index + """ if type is not None: salt.utils.versions.warn_until( @@ -81,25 +121,44 @@ def write(domain, key, value, vtype=None, user=None, type=None): "The 'vtype' argument in macdefaults.write takes precedence over 'type'." ) - if vtype is None: - vtype = "string" - - if vtype in ("bool", "boolean"): - value = _convert_to_defaults_boolean(value) - - if isinstance(value, dict): - value = list((k, v) for k, v in value.items()) - value = salt.utils.data.flatten(value) - elif isinstance(value, (int, float, bool, str)): - value = [value] - elif not isinstance(value, list): - raise ValueError("Value must be a list, dict, int, float, bool, or string") - - # Quote values that are neither integers nor floats - value = map(lambda v: str(v) if isinstance(v, (int, float)) else f'"{v}"', value) + plist = _load_plist(domain, user=user) or {} + keys = key.split(".") + last_key = keys[-1] + + # Traverse the plist + container = _traverse_keys(plist, keys[:-1]) + if container is None: + raise KeyError(f"Key not found: {key} for domain: {domain}") + + current_value = None + if isinstance(container, dict): + current_value = container.get(last_key) + elif isinstance(container, list) and last_key.isdigit(): + last_key = int(last_key) + if -len(container) <= last_key < len(container): + current_value = container[last_key] + else: + raise IndexError(f"Index {last_key} is out of range for domain: {domain}") + + # Write/Update the new value + if vtype is not None: + if vtype == "array-add": + array_add = True + elif vtype == "dict-add": + dict_merge = True + value = cast_value_to_vtype(value, vtype) + + if isinstance(current_value, dict) and isinstance(value, dict) and dict_merge: + container[last_key].update(value) + elif isinstance(current_value, list) and array_add: + if isinstance(value, list): + container[last_key].extend(value) + else: + container[last_key].append(value) + else: + container[last_key] = value - cmd = f'write "{domain}" "{key}" -{vtype} {" ".join(value)}' - return _run_defaults_cmd(cmd, runas=user) + return _save_plist(domain, plist, user=user) def read(domain, key, user=None): @@ -110,7 +169,7 @@ def read(domain, key, user=None): .. code-block:: bash - salt '*' macdefaults.read com.apple.CrashReporter DialogType + salt '*' macdefaults.read com.apple.Dock persistent-apps.1.title-data.file-label salt '*' macdefaults.read NSGlobalDomain ApplePersistence @@ -118,27 +177,21 @@ def read(domain, key, user=None): The name of the domain to read from key - The key of the given domain to read from + The key of the given domain to read from. + It can be a nested key/index separated by dots. user - The user to read the defaults as - - """ - cmd = f'read "{domain}" "{key}"' - ret = _run_defaults_cmd(cmd, runas=user) + The user to read the defaults from - if ret["retcode"] != 0: - if "does not exist" in ret["stderr"]: - return None - raise CommandExecutionError(f"Failed to read default: {ret['stderr']}") + Returns: + The current value for the given key, or None if the key does not exist. - # Type cast the value - try: - vtype = read_type(domain, key, user) - except CommandExecutionError: - vtype = None + """ + plist = _load_plist(domain, user) + if plist is None: + return None - return _default_to_python(ret["stdout"].strip(), vtype) + return _traverse_keys(plist, key.split(".")) def delete(domain, key, user=None): @@ -157,165 +210,195 @@ def delete(domain, key, user=None): The name of the domain to delete from key - The key of the given domain to delete + The key of the given domain to delete. + It can be a nested key separated by dots. user The user to delete the defaults with """ - cmd = f'delete "{domain}" "{key}"' - return _run_defaults_cmd(cmd, runas=user) + plist = _load_plist(domain, user=user) + if plist is None: + return None + keys = key.split(".") -def read_type(domain, key, user=None): - """ - Read a default type from the system - If the key is not found, None is returned. + # Traverse the plist til the penultimate key. + # Last key must be handled separately since we + # need the parent dictionary to delete that key. + target = _traverse_keys(plist, keys[:-1]) + if target is None: + return None - CLI Example: + # Delete the last key if it exists and update defaults + last_key = keys[-1] + key_in_plist = False + if isinstance(target, dict) and last_key in target: + key_in_plist = True - .. code-block:: bash + elif ( + isinstance(target, list) + and last_key.isdigit() + and -len(target) <= int(last_key) < len(target) + ): + key_in_plist = True + last_key = int(last_key) - salt '*' macdefaults.read_type com.apple.CrashReporter DialogType + if not key_in_plist: + return None - salt '*' macdefaults.read_type NSGlobalDomain ApplePersistence + del target[last_key] + return _save_plist(domain, plist, user=user) - domain - The name of the domain to read from - - key - The key of the given domain to read the type of - - user - The user to read the defaults as +def cast_value_to_vtype(value, vtype): """ - cmd = f'read-type "{domain}" "{key}"' - ret = _run_defaults_cmd(cmd, runas=user) - - if ret["retcode"] != 0: - if "does not exist" in ret["stderr"]: - return None - raise CommandExecutionError(f"Failed to read type: {ret['stderr']}") - - return re.sub(r"^Type is ", "", ret["stdout"].strip()) - - -def _default_to_python(value, vtype=None): - """ - Cast the value returned by the defaults command in vytpe to Python type + Convert the value to the specified vtype. + If the value cannot be converted, it will be returned as is. value - The value to cast + The value to be converted vtype - The type to cast the value to + The type to convert the value to - """ - if vtype in ["integer", "int"]: - return int(value) - if vtype == "float": - return float(value) - if vtype in ["boolean", "bool"]: - return value in ["1", "TRUE", "YES"] - if vtype == "array": - return _parse_defaults_array(value) - if vtype in ["dict", "dictionary"]: - return _parse_defaults_dict(value) - return value + Returns: + The converted value -def _parse_defaults_array(value): """ - Parse an array from a string returned by `defaults read` - and returns the array content as a list - - value - A multiline string with the array content, including the surrounding parenthesis - - """ - lines = value.splitlines() - if not re.match(r"\s*\(", lines[0]) or not re.match(r"\s*\)", lines[-1]): - raise ValueError("Invalid array format") - - lines = lines[1:-1] - - # Remove leading and trailing spaces - lines = list(map(lambda line: line.strip(), lines)) - - # Remove trailing commas - lines = list(map(lambda line: re.sub(r",?$", "", line), lines)) + # Boolean + if vtype in ("bool", "boolean"): + if isinstance(value, str): + if value.lower() in ("true", "yes", "1"): + value = True + elif value.lower() in ("false", "no", "0"): + value = False + else: + raise ValueError(f"Invalid value for boolean: '{value}'") + elif value in (0, 1): + value = bool(value) + elif not isinstance(value, bool): + raise ValueError(f"Invalid value for boolean: '{value}'") + # String + elif vtype == "string": + if isinstance(value, bool): + value = "YES" if value else "NO" + elif isinstance(value, (int, float)): + value = str(value) + elif isinstance(value, datetime): + value = value.strftime("%Y-%m-%dT%H:%M:%SZ") + elif isinstance(value, bytes): + value = value.decode() + # Integer + elif vtype in ("int", "integer"): + value = int(value) + # Float + elif vtype == "float": + value = float(value) + # Date + elif vtype == "date": + if not isinstance(value, datetime): + try: + value = datetime.fromtimestamp(float(value)) + except ValueError: + if re.match(r"\d{4}-\d{2}-\d{2}T\d{2}:\d{2}:\d{2}Z", value): + value = datetime.strptime(value, "%Y-%m-%dT%H:%M:%SZ") + else: + raise ValueError(f"Invalid date format: '{value}'") + # Data + elif vtype == "data": + if isinstance(value, str): + value = value.encode() + elif not isinstance(value, bytes): + raise ValueError(f"Invalid value for data: '{value}'") + # Dictionary + elif vtype in ("dict", "dict-add"): + if not isinstance(value, dict): + raise ValueError(f"Invalid value for dictionary: '{value}'") + # Array + elif vtype in ("array", "array-add"): + if not isinstance(value, list): + raise ValueError(f"Invalid value for array: '{value}'") + else: + raise ValueError(f"Invalid type: '{vtype}'") - # Remove quotes - lines = list(map(lambda line: line.strip('"'), lines)) + return value - # Convert to numbers if possible - lines = list(map(_convert_to_number_if_possible, lines)) - return lines +def _load_plist(domain, user=None): + """ + Load a plist from the system and return it as a dictionary + domain + The name of the domain to read from -def _parse_defaults_dict(value): - """ - Parse a dictionary from a string returned by `defaults read` - and returns the dictionary content as a Python dictionary + user + The user to read the defaults as. Defaults to root (None). - value (str): - A multiline string with the dictionary content, including the surrounding curly braces + Raises: + CommandExecutionError: When the defaults command fails + Other exceptions thrown by plistlib.loads + Returns: + A dictionary with the plist contents, or None if the domain does not exist. """ - lines = value.splitlines() - if not re.match(r"\s*\{", lines[0]) or not re.match(r"\s*\}", lines[-1]): - raise ValueError("Invalid dictionary format") + cmd = f'export "{domain}" -' + ret = _run_defaults_cmd(cmd, runas=user) - contents = {} - lines = list(map(lambda line: line.strip(), lines[1:-1])) - for line in lines: - key, value = re.split(r"\s*=\s*", line.strip()) - if re.match(r"\s*(\(|\{)", value): - raise ValueError("Nested arrays and dictionaries are not supported") + if ret["retcode"] != 0: + raise CommandExecutionError(f"Failed to export defaults: {ret['stderr']}") - value = re.sub(r";?$", "", value) - contents[key] = _convert_to_number_if_possible(value.strip('"')) + plist = plistlib.loads(ret["stdout"].encode()) + if not plist: + return None - return contents + return plist -def _convert_to_number_if_possible(value): +def _save_plist(domain, plist, user=None): """ - Convert a string to a number if possible - - value - The string to convert + Save a plist dictionary to the system - """ - try: - return int(value) - except ValueError: - try: - return float(value) - except ValueError: - return value + domain + The name of the domain to read from + plist + The dictionary to export as a plist -def _convert_to_defaults_boolean(value): - """ - Convert a boolean to a string that can be used with the defaults command + user + The user to export the defaults to. Defaults to root (None). - value - The boolean value to convert + Raises: + CommandExecutionError: When the defaults command fails + Other exceptions thrown by plistlib.dump + Returns: + A dictionary with the defaults command result """ - if value in (True, 1): - return "TRUE" - if value in (False, 0): - return "FALSE" - - BOOLEAN_ALLOWED_VALUES = ["TRUE", "YES", "FALSE", "NO"] - if value not in BOOLEAN_ALLOWED_VALUES: - msg = "Value must be a boolean or a string of " - msg += ", ".join(BOOLEAN_ALLOWED_VALUES) - raise ValueError(msg) + with tempfile.NamedTemporaryFile() as tmpfile: + plistlib.dump(plist, tmpfile) + tmpfile.flush() + cmd = f'import "{domain}" "{tmpfile.name}"' + return _run_defaults_cmd(cmd, runas=user) + + +def _traverse_keys(plist, keys): + value = plist + for k in keys: + if isinstance(value, dict): + value = value.get(k) + elif ( + isinstance(value, list) + and k.isdigit() + and -len(value) <= int(k) < len(value) + ): + value = value[int(k)] + else: + value = None + + if value is None: + return None return value diff --git a/salt/states/macdefaults.py b/salt/states/macdefaults.py index 617874cdbb7a..cd0e6df633e2 100644 --- a/salt/states/macdefaults.py +++ b/salt/states/macdefaults.py @@ -18,12 +18,13 @@ def __virtual__(): return (False, "Only supported on macOS") -def write(name, domain, value, vtype="string", user=None): +def write(name, domain, value, vtype=None, user=None): """ Write a default to the system name - The key of the given domain to write to + The key of the given domain to write to. + It can be a nested key/index separated by dots. domain The name of the domain to write to @@ -42,7 +43,9 @@ def write(name, domain, value, vtype="string", user=None): ret = {"name": name, "result": True, "comment": "", "changes": {}} current_value = __salt__["macdefaults.read"](domain, name, user) - value = _cast_value(value, vtype) + + if vtype is not None: + value = __salt__["macdefaults.cast_value_to_vtype"](value, vtype) if _compare_values(value, current_value, vtype): ret["comment"] += f"{domain} {name} is already set to {value}" @@ -62,7 +65,8 @@ def absent(name, domain, user=None): Make sure the defaults value is absent name - The key of the given domain to remove + The key of the given domain to remove. + It can be a nested key/index separated by dots. domain The name of the domain to remove from @@ -75,7 +79,7 @@ def absent(name, domain, user=None): out = __salt__["macdefaults.delete"](domain, name, user) - if out["retcode"] != 0: + if out is None or out["retcode"] != 0: ret["comment"] += f"{domain} {name} is already absent" else: ret["changes"]["absent"] = f"{domain} {name} is now absent" @@ -98,81 +102,11 @@ def _compare_values(new, current, vtype): """ if vtype == "array-add": - return _is_subarray(new, current) + if isinstance(new, list): + return new == current[-len(new) :] + return new == current[-1] + if vtype == "dict-add": return all(key in current and new[key] == current[key] for key in new.keys()) return new == current - - -def _is_subarray(new, current): - """ - Check if new is a subarray of current array. - - This method does not check only whether all elements in new array - are present in current array, but also whether the elements are in - the same order. - - new - The new array to compare - - current - The current array to compare - - """ - current_len = len(current) - new_len = len(new) - - if new_len == 0: - return True - if new_len > current_len: - return False - - for i in range(current_len - new_len + 1): - # Check if the new array is found at this position - if current[i : i + new_len] == new: - return True - - return False - - -def _cast_value(value, vtype): - """ - Cast the given macOS default value to Python type - - value - The value to cast from macOS default - - vtype - The type to cast the value from - - """ - - def safe_cast(val, to_type, default=None): - """ - Auxiliary function to safely cast a value to a given type - - """ - try: - return to_type(val) - except ValueError: - return default - - if vtype in ("bool", "boolean"): - if value not in [True, 1, "TRUE", "YES", False, 0, "FALSE", "NO"]: - raise ValueError(f"Invalid value for boolean: {value}") - return value in [True, "TRUE", "YES"] - - if vtype in ("int", "integer"): - return safe_cast(value, int) - - if vtype == "float": - return safe_cast(value, float) - - if vtype in ("dict", "dict-add"): - return safe_cast(value, dict) - - if vtype in ["array", "array-add"]: - return safe_cast(value, list) - - return value diff --git a/tests/pytests/unit/modules/test_macdefaults.py b/tests/pytests/unit/modules/test_macdefaults.py index 7cf0d0a1c5f0..f2511b308462 100644 --- a/tests/pytests/unit/modules/test_macdefaults.py +++ b/tests/pytests/unit/modules/test_macdefaults.py @@ -1,7 +1,9 @@ +from datetime import datetime + import pytest import salt.modules.macdefaults as macdefaults -from tests.support.mock import MagicMock, call, patch +from tests.support.mock import MagicMock, patch @pytest.fixture @@ -9,9 +11,74 @@ def configure_loader_modules(): return {macdefaults: {}} +@pytest.fixture +def PLIST_OUTPUT(): + return """ + + + + AdjustWindowForFontSizeChange + + AllowClipboardAccess + + AppleAntiAliasingThreshold + 1024 + Default Bookmark Guid + C7EED71F-6B5F-4822-B735-D20CAE8AD57D + NSNavPanelExpandedSizeForOpenMode + {800, 448} + NSSplitView Subview Frames NSColorPanelSplitView + + 0.000000, 0.000000, 224.000000, 222.000000, NO, NO + 0.000000, 223.000000, 224.000000, 48.000000, NO, NO + + NSToolbar Configuration com.apple.NSColorPanel + + TB Is Shown + 1 + + NeverWarnAboutShortLivedSessions_C7EED71F-6B5F-4822-B735-D20CAE8AD57D_selection + 0 + NoSyncBFPRecents + + MonoLisa Variable + Menlo + MonoLisa + + NoSyncFrame_SharedPreferences + + screenFrame + {{0, 0}, {1920, 1200}} + topLeft + {668, 1004} + + NoSyncNextAnnoyanceTime + 699041775.9120981 + NoSyncTipOfTheDayEligibilityBeganTime + 697356008.723986 + PointerActions + + Button,1,1,, + + Action + kContextMenuPointerAction + + Button,2,1,, + + Action + kPasteFromSelectionPointerAction + + + SULastCheckTime + 2024-06-23T09:33:44Z + + +""" + + def test_run_defaults_cmd(): """ - Test caling _run_defaults_cmd + Test calling _run_defaults_cmd """ mock = MagicMock(return_value={"retcode": 0, "stdout": "Server", "stderr": ""}) with patch.dict(macdefaults.__salt__, {"cmd.run_all": mock}): @@ -39,16 +106,190 @@ def test_run_defaults_cmd_with_user(): assert result == {"retcode": 0, "stdout": "Server", "stderr": ""} +def test_load_plist(PLIST_OUTPUT): + """ + Test loading a plist + """ + expected_result = {"Crash": "Server"} + run_defaults_cmd_mock = MagicMock( + return_value={"retcode": 0, "stdout": PLIST_OUTPUT, "stderr": ""} + ) + + with patch( + "salt.modules.macdefaults._run_defaults_cmd", run_defaults_cmd_mock + ), patch("plistlib.loads", return_value=expected_result) as plist_mock: + result = macdefaults._load_plist("com.googlecode.iterm2") + run_defaults_cmd_mock.assert_called_once_with( + 'export "com.googlecode.iterm2" -', runas=None + ) + plist_mock.assert_called_once_with(PLIST_OUTPUT.encode()) + assert result == expected_result + + +def test_load_plist_no_domain(): + """ + Test loading a plist with a non-existent domain + """ + empty_plist = """ + + + +""" + + run_defaults_cmd_mock = MagicMock( + return_value={"retcode": 0, "stdout": empty_plist, "stderr": ""} + ) + + with patch("salt.modules.macdefaults._run_defaults_cmd", run_defaults_cmd_mock): + result = macdefaults._load_plist("com.googlecode.iterm2", user="cdalvaro") + run_defaults_cmd_mock.assert_called_once_with( + 'export "com.googlecode.iterm2" -', runas="cdalvaro" + ) + assert result is None + + +def test_save_plist(): + """ + Test saving a plist + """ + new_plist = {"Crash": "Server"} + expected_result = {"retcode": 0, "stdout": "", "stderr": ""} + + calls = [] + + def side_effect_run_defaults_command(*args, **kwargs): + calls.append("macdefaults._run_defaults_cmd") + return expected_result + + run_defaults_cmd_mock = MagicMock(side_effect=side_effect_run_defaults_command) + + tempfile_mock = MagicMock() + tempfile_mock.name = "/tmp/tmpfile" + tempfile_mock.flush = MagicMock(side_effect=lambda: calls.append("tempfile.flush")) + + named_temporary_file_mock = MagicMock() + named_temporary_file_mock.return_value.__enter__.return_value = tempfile_mock + + plist_mock = MagicMock( + side_effect=lambda _dict, _file: calls.append("plistlib.dump") + ) + + with patch( + "salt.modules.macdefaults._run_defaults_cmd", run_defaults_cmd_mock + ), patch("tempfile.NamedTemporaryFile", named_temporary_file_mock), patch( + "plistlib.dump", plist_mock + ): + result = macdefaults._save_plist("com.googlecode.iterm2", new_plist) + tempfile_mock.flush.assert_called() + plist_mock.assert_called_once_with(new_plist, tempfile_mock) + run_defaults_cmd_mock.assert_called_once_with( + 'import "com.googlecode.iterm2" "/tmp/tmpfile"', runas=None + ) + assert result == expected_result + assert calls == [ + "plistlib.dump", + "tempfile.flush", + "macdefaults._run_defaults_cmd", + ] + + +def test_cast_value_to_bool(): + assert macdefaults.cast_value_to_vtype("TRUE", "bool") is True + assert macdefaults.cast_value_to_vtype("YES", "bool") is True + assert macdefaults.cast_value_to_vtype("1", "bool") is True + assert macdefaults.cast_value_to_vtype(1, "bool") is True + + assert macdefaults.cast_value_to_vtype("FALSE", "boolean") is False + assert macdefaults.cast_value_to_vtype("NO", "boolean") is False + assert macdefaults.cast_value_to_vtype("0", "boolean") is False + assert macdefaults.cast_value_to_vtype(0, "boolean") is False + + with pytest.raises(ValueError): + macdefaults.cast_value_to_vtype("foo", "bool") + + with pytest.raises(ValueError): + macdefaults.cast_value_to_vtype(1.1, "bool") + + +def test_cast_value_to_string(): + assert macdefaults.cast_value_to_vtype(124.120, "string") == "124.12" + assert macdefaults.cast_value_to_vtype(True, "string") == "YES" + assert macdefaults.cast_value_to_vtype(False, "string") == "NO" + assert macdefaults.cast_value_to_vtype(124120, "string") == "124120" + + expected_date = "2024-06-26T16:45:05Z" + test_date = datetime.strptime(expected_date, "%Y-%m-%dT%H:%M:%SZ") + assert macdefaults.cast_value_to_vtype(test_date, "string") == expected_date + + expected_data = "foo" + test_data = expected_data.encode() + assert macdefaults.cast_value_to_vtype(test_data, "string") == expected_data + + +def test_cast_value_to_int(): + assert macdefaults.cast_value_to_vtype("124", "int") == 124 + assert macdefaults.cast_value_to_vtype(124.0, "int") == 124 + assert macdefaults.cast_value_to_vtype(True, "integer") == 1 + assert macdefaults.cast_value_to_vtype(False, "integer") == 0 + + with pytest.raises(ValueError): + macdefaults.cast_value_to_vtype("foo", "int") + + +def test_cast_value_to_float(): + assert macdefaults.cast_value_to_vtype("124.12", "float") == 124.12 + assert macdefaults.cast_value_to_vtype(124, "float") == 124.0 + assert macdefaults.cast_value_to_vtype(True, "float") == 1.0 + assert macdefaults.cast_value_to_vtype(False, "float") == 0.0 + + with pytest.raises(ValueError): + macdefaults.cast_value_to_vtype("foo", "float") + + +def test_cast_value_to_date(): + expected_date = datetime(2024, 6, 26, 16, 45, 5) + + # Date -> Date + assert macdefaults.cast_value_to_vtype(expected_date, "date") == expected_date + + # String -> Date + test_date = datetime.strftime(expected_date, "%Y-%m-%dT%H:%M:%SZ") + assert macdefaults.cast_value_to_vtype(test_date, "date") == expected_date + + # Timestamp -> Date + test_timestamp = expected_date.timestamp() + assert macdefaults.cast_value_to_vtype(test_timestamp, "date") == expected_date + + with pytest.raises(ValueError): + macdefaults.cast_value_to_vtype("foo", "date") + + +def test_cast_value_to_data(): + expected_data = b"foo" + + # String -> Data + test_data = expected_data.decode() + assert macdefaults.cast_value_to_vtype(test_data, "data") == expected_data + + # Data -> Data + assert macdefaults.cast_value_to_vtype(expected_data, "data") == expected_data + + with pytest.raises(ValueError): + macdefaults.cast_value_to_vtype(123, "data") + + def test_write_default(): """ Test writing a default setting """ - mock = MagicMock(return_value={"retcode": 0}) - with patch("salt.modules.macdefaults._run_defaults_cmd", mock): + with patch("salt.modules.macdefaults._load_plist", return_value={}), patch( + "salt.modules.macdefaults._save_plist", return_value={"retcode": 0} + ) as save_plist_mock: macdefaults.write("com.apple.CrashReporter", "DialogType", "Server") - mock.assert_called_once_with( - 'write "com.apple.CrashReporter" "DialogType" -string "Server"', - runas=None, + save_plist_mock.assert_called_once_with( + "com.apple.CrashReporter", + {"DialogType": "Server"}, + user=None, ) @@ -56,14 +297,16 @@ def test_write_default_with_user(): """ Test writing a default setting with a specific user """ - mock = MagicMock(return_value={"retcode": 0}) - with patch("salt.modules.macdefaults._run_defaults_cmd", mock): + with patch("salt.modules.macdefaults._load_plist", return_value={}), patch( + "salt.modules.macdefaults._save_plist", return_value={"retcode": 0} + ) as save_plist_mock: macdefaults.write( - "com.apple.CrashReporter", "DialogType", "Server", user="frank" + "com.apple.CrashReporter", "DialogType", "Server", user="cdalvaro" ) - mock.assert_called_once_with( - 'write "com.apple.CrashReporter" "DialogType" -string "Server"', - runas="frank", + save_plist_mock.assert_called_once_with( + "com.apple.CrashReporter", + {"DialogType": "Server"}, + user="cdalvaro", ) @@ -71,12 +314,28 @@ def test_write_default_true_boolean(): """ Test writing a default True boolean setting """ - mock = MagicMock(return_value={"retcode": 0}) - with patch("salt.modules.macdefaults._run_defaults_cmd", mock): - macdefaults.write("com.apple.CrashReporter", "Crash", True, vtype="boolean") - mock.assert_called_once_with( - 'write "com.apple.CrashReporter" "Crash" -boolean "TRUE"', - runas=None, + with patch( + "salt.modules.macdefaults._load_plist", return_value={"Crash": False} + ), patch( + "salt.modules.macdefaults._save_plist", return_value={"retcode": 0} + ) as save_plist_mock: + macdefaults.write("com.apple.CrashReporter", "Crash", True) + save_plist_mock.assert_called_once_with( + "com.apple.CrashReporter", + {"Crash": True}, + user=None, + ) + + with patch( + "salt.modules.macdefaults._load_plist", return_value={"Crash": False} + ), patch( + "salt.modules.macdefaults._save_plist", return_value={"retcode": 0} + ) as save_plist_mock: + macdefaults.write("com.apple.CrashReporter", "Crash", "YES", vtype="boolean") + save_plist_mock.assert_called_once_with( + "com.apple.CrashReporter", + {"Crash": True}, + user=None, ) @@ -84,12 +343,28 @@ def test_write_default_false_bool(): """ Test writing a default False boolean setting """ - mock = MagicMock(return_value={"retcode": 0}) - with patch("salt.modules.macdefaults._run_defaults_cmd", mock): - macdefaults.write("com.apple.CrashReporter", "Crash", False, vtype="bool") - mock.assert_called_once_with( - 'write "com.apple.CrashReporter" "Crash" -bool "FALSE"', - runas=None, + with patch( + "salt.modules.macdefaults._load_plist", return_value={"Crash": True} + ), patch( + "salt.modules.macdefaults._save_plist", return_value={"retcode": 0} + ) as save_plist_mock: + macdefaults.write("com.apple.CrashReporter", "Crash", False) + save_plist_mock.assert_called_once_with( + "com.apple.CrashReporter", + {"Crash": False}, + user=None, + ) + + with patch( + "salt.modules.macdefaults._load_plist", return_value={"Crash": True} + ), patch( + "salt.modules.macdefaults._save_plist", return_value={"retcode": 0} + ) as save_plist_mock: + macdefaults.write("com.apple.CrashReporter", "Crash", "NO", vtype="bool") + save_plist_mock.assert_called_once_with( + "com.apple.CrashReporter", + {"Crash": False}, + user=None, ) @@ -97,25 +372,40 @@ def test_write_default_int(): """ Test writing a default int setting """ - mock = MagicMock(return_value={"retcode": 0}) - with patch("salt.modules.macdefaults._run_defaults_cmd", mock): - macdefaults.write("com.apple.CrashReporter", "Crash", 1, vtype="int") - mock.assert_called_once_with( - 'write "com.apple.CrashReporter" "Crash" -int 1', - runas=None, + with patch( + "salt.modules.macdefaults._load_plist", return_value={"Crash": 0} + ), patch( + "salt.modules.macdefaults._save_plist", return_value={"retcode": 0} + ) as save_plist_mock: + macdefaults.write("com.apple.CrashReporter", "Crash", 1) + save_plist_mock.assert_called_once_with( + "com.apple.CrashReporter", + {"Crash": 1}, + user=None, ) + with patch( + "salt.modules.macdefaults._load_plist", return_value={"Crash": 3} + ), patch( + "salt.modules.macdefaults._save_plist", return_value={"retcode": 0} + ) as save_plist_mock: + macdefaults.write("com.apple.CrashReporter", "Crash", "3", vtype="int") + save_plist_mock.assert_called_once_with( + "com.apple.CrashReporter", + {"Crash": 3}, + user=None, + ) -def test_write_default_integer(): - """ - Test writing a default integer setting - """ - mock = MagicMock(return_value={"retcode": 0}) - with patch("salt.modules.macdefaults._run_defaults_cmd", mock): - macdefaults.write("com.apple.CrashReporter", "Crash", 1, vtype="integer") - mock.assert_called_once_with( - 'write "com.apple.CrashReporter" "Crash" -integer 1', - runas=None, + with patch( + "salt.modules.macdefaults._load_plist", return_value={"Crash": 0} + ), patch( + "salt.modules.macdefaults._save_plist", return_value={"retcode": 0} + ) as save_plist_mock: + macdefaults.write("com.apple.CrashReporter", "Crash", "15", vtype="integer") + save_plist_mock.assert_called_once_with( + "com.apple.CrashReporter", + {"Crash": 15}, + user=None, ) @@ -123,12 +413,28 @@ def test_write_default_float(): """ Test writing a default float setting """ - mock = MagicMock(return_value={"retcode": 0}) - with patch("salt.modules.macdefaults._run_defaults_cmd", mock): - macdefaults.write("com.apple.CrashReporter", "Crash", 0.85, vtype="float") - mock.assert_called_once_with( - 'write "com.apple.CrashReporter" "Crash" -float 0.85', - runas=None, + with patch( + "salt.modules.macdefaults._load_plist", return_value={"Crash": 0} + ), patch( + "salt.modules.macdefaults._save_plist", return_value={"retcode": 0} + ) as save_plist_mock: + macdefaults.write("com.apple.CrashReporter", "Crash", 1.234) + save_plist_mock.assert_called_once_with( + "com.apple.CrashReporter", + {"Crash": 1.234}, + user=None, + ) + + with patch( + "salt.modules.macdefaults._load_plist", return_value={"Crash": 0} + ), patch( + "salt.modules.macdefaults._save_plist", return_value={"retcode": 0} + ) as save_plist_mock: + macdefaults.write("com.apple.CrashReporter", "Crash", "14.350", vtype="float") + save_plist_mock.assert_called_once_with( + "com.apple.CrashReporter", + {"Crash": 14.35}, + user=None, ) @@ -136,205 +442,351 @@ def test_write_default_array(): """ Test writing a default array setting """ - mock = MagicMock(return_value={"retcode": 0}) - with patch("salt.modules.macdefaults._run_defaults_cmd", mock): + + # Key does not exist + with patch("salt.modules.macdefaults._load_plist", return_value={}), patch( + "salt.modules.macdefaults._save_plist", return_value={"retcode": 0} + ) as save_plist_mock: + macdefaults.write("com.apple.CrashReporter", "Crash", [0.7, 0.9, 1.0]) + save_plist_mock.assert_called_once_with( + "com.apple.CrashReporter", + {"Crash": [0.7, 0.9, 1.0]}, + user=None, + ) + + # Key already exists with different values + with patch( + "salt.modules.macdefaults._load_plist", return_value={"Crash": [0.1, 0.2, 0.4]} + ), patch( + "salt.modules.macdefaults._save_plist", return_value={"retcode": 0} + ) as save_plist_mock: + macdefaults.write("com.apple.CrashReporter", "Crash", [0.7, 0.9, 1.0]) + save_plist_mock.assert_called_once_with( + "com.apple.CrashReporter", + {"Crash": [0.7, 0.9, 1.0]}, + user=None, + ) + + # Array already exists and the new value (float) is appended + with patch( + "salt.modules.macdefaults._load_plist", return_value={"Crash": [0.1, 0.2, 0.4]} + ), patch( + "salt.modules.macdefaults._save_plist", return_value={"retcode": 0} + ) as save_plist_mock: macdefaults.write( - "com.apple.CrashReporter", "Crash", [0.1, 0.2, 0.4], vtype="array" + "com.apple.CrashReporter", "Crash", "0.5", vtype="float", array_add=True ) - mock.assert_called_once_with( - 'write "com.apple.CrashReporter" "Crash" -array 0.1 0.2 0.4', - runas=None, + save_plist_mock.assert_called_once_with( + "com.apple.CrashReporter", + {"Crash": [0.1, 0.2, 0.4, 0.5]}, + user=None, + ) + + # Array already exists and the new value (array) is appended (using array_add=True) + with patch( + "salt.modules.macdefaults._load_plist", return_value={"Crash": [0.1, 0.2, 0.4]} + ), patch( + "salt.modules.macdefaults._save_plist", return_value={"retcode": 0} + ) as save_plist_mock: + macdefaults.write( + "com.apple.CrashReporter", "Crash", [2.0, 4.0], array_add=True + ) + save_plist_mock.assert_called_once_with( + "com.apple.CrashReporter", + {"Crash": [0.1, 0.2, 0.4, 2.0, 4.0]}, + user=None, ) + # Array already exists and the new value (array) is appended (using vtype="array-add") + with patch( + "salt.modules.macdefaults._load_plist", return_value={"Crash": [0.1, 0.2, 0.4]} + ), patch( + "salt.modules.macdefaults._save_plist", return_value={"retcode": 0} + ) as save_plist_mock: + macdefaults.write( + "com.apple.CrashReporter", "Crash", [2.0, 4.0], vtype="array-add" + ) + save_plist_mock.assert_called_once_with( + "com.apple.CrashReporter", + {"Crash": [0.1, 0.2, 0.4, 2.0, 4.0]}, + user=None, + ) + + with patch( + "salt.modules.macdefaults._load_plist", return_value={"Crash": [0.1, 0.2, 0.4]} + ), patch( + "salt.modules.macdefaults._save_plist", return_value={"retcode": 0} + ) as save_plist_mock, pytest.raises( + ValueError + ) as excinfo: + macdefaults.write("com.apple.CrashReporter", "Crash", "0.5", vtype="array") + save_plist_mock.assert_not_called() + excinfo.match("Invalid value for array") + def test_write_default_dictionary(): """ Test writing a default dictionary setting """ - mock = MagicMock(return_value={"retcode": 0}) - with patch("salt.modules.macdefaults._run_defaults_cmd", mock): + + # Key does not exist + with patch("salt.modules.macdefaults._load_plist", return_value={}), patch( + "salt.modules.macdefaults._save_plist", return_value={"retcode": 0} + ) as save_plist_mock: + macdefaults.write("com.apple.CrashReporter", "Crash", {"foo:": "bar", "baz": 0}) + save_plist_mock.assert_called_once_with( + "com.apple.CrashReporter", + {"Crash": {"foo:": "bar", "baz": 0}}, + user=None, + ) + + # Key already exists with different values + with patch( + "salt.modules.macdefaults._load_plist", return_value={"Crash": {"foo": "bar"}} + ), patch( + "salt.modules.macdefaults._save_plist", return_value={"retcode": 0} + ) as save_plist_mock: + macdefaults.write("com.apple.CrashReporter", "Crash", {"baz": 1}) + save_plist_mock.assert_called_once_with( + "com.apple.CrashReporter", + {"Crash": {"baz": 1}}, + user=None, + ) + + # Dictionary already exists and the new value is merged (using dict_merge=True) + with patch( + "salt.modules.macdefaults._load_plist", return_value={"Crash": {"foo": "bar"}} + ), patch( + "salt.modules.macdefaults._save_plist", return_value={"retcode": 0} + ) as save_plist_mock: macdefaults.write( - "com.apple.CrashReporter", "Crash", {"foo": "bar", "baz": 0}, vtype="dict" + "com.apple.CrashReporter", "Crash", {"baz": 10}, dict_merge=True ) - mock.assert_called_once_with( - 'write "com.apple.CrashReporter" "Crash" -dict "foo" "bar" "baz" 0', - runas=None, + save_plist_mock.assert_called_once_with( + "com.apple.CrashReporter", + {"Crash": {"foo": "bar", "baz": 10}}, + user=None, + ) + + # Dictionary already exists and the new value is merged (using vtype="dict-add") + with patch( + "salt.modules.macdefaults._load_plist", return_value={"Crash": {"foo": "bar"}} + ), patch( + "salt.modules.macdefaults._save_plist", return_value={"retcode": 0} + ) as save_plist_mock: + macdefaults.write( + "com.apple.CrashReporter", "Crash", {"baz": 10}, vtype="dict-add" + ) + save_plist_mock.assert_called_once_with( + "com.apple.CrashReporter", + {"Crash": {"foo": "bar", "baz": 10}}, + user=None, ) + with patch( + "salt.modules.macdefaults._load_plist", return_value={"Crash": {}} + ), patch( + "salt.modules.macdefaults._save_plist", return_value={"retcode": 0} + ) as save_plist_mock, pytest.raises( + ValueError + ) as excinfo: + macdefaults.write("com.apple.CrashReporter", "Crash", "0.5", vtype="dict") + save_plist_mock.assert_not_called() + excinfo.match("Invalid value for dictionary") + -def test_read_default(): +def test_read_default_string(PLIST_OUTPUT): """ - Test reading a default setting + Test reading a default string setting """ - def custom_run_defaults_cmd(action, runas=None): - if action == 'read-type "com.apple.CrashReporter" "Crash"': - return {"retcode": 0, "stdout": "string"} - elif action == 'read "com.apple.CrashReporter" "Crash"': - return {"retcode": 0, "stdout": "Server"} - return {"retcode": 1, "stderr": f"Unknown action: {action}", "stdout": ""} + def custom_run_defaults_cmd(action, runas): + return {"retcode": 0, "stdout": PLIST_OUTPUT} mock = MagicMock(side_effect=custom_run_defaults_cmd) with patch("salt.modules.macdefaults._run_defaults_cmd", mock): - result = macdefaults.read("com.apple.CrashReporter", "Crash") - mock.assert_has_calls( - [ - call('read "com.apple.CrashReporter" "Crash"', runas=None), - call('read-type "com.apple.CrashReporter" "Crash"', runas=None), - ] - ) - assert result == "Server" + result = macdefaults.read("com.googlecode.iterm2", "Default Bookmark Guid") + mock.assert_called_once_with('export "com.googlecode.iterm2" -', runas=None) + assert result == "C7EED71F-6B5F-4822-B735-D20CAE8AD57D" -def test_read_default_with_user(): +def test_read_default_string_with_user(PLIST_OUTPUT): """ - Test reading a default setting as a specific user + Test reading a default string setting as a specific user """ - def custom_run_defaults_cmd(action, runas=None): - if action == 'read-type "com.apple.CrashReporter" "Crash"': - return {"retcode": 0, "stdout": "string"} - elif action == 'read "com.apple.CrashReporter" "Crash"': - return {"retcode": 0, "stdout": "Server"} - return {"retcode": 1, "stderr": f"Unknown action: {action}", "stdout": ""} + def custom_run_defaults_cmd(action, runas): + return {"retcode": 0, "stdout": PLIST_OUTPUT} mock = MagicMock(side_effect=custom_run_defaults_cmd) with patch("salt.modules.macdefaults._run_defaults_cmd", mock): - result = macdefaults.read("com.apple.CrashReporter", "Crash", user="frank") - mock.assert_has_calls( - [ - call('read "com.apple.CrashReporter" "Crash"', runas="frank"), - call('read-type "com.apple.CrashReporter" "Crash"', runas="frank"), - ] + result = macdefaults.read( + "com.googlecode.iterm2", "NSNavPanelExpandedSizeForOpenMode", user="frank" ) - assert result == "Server" + mock.assert_called_once_with('export "com.googlecode.iterm2" -', runas="frank") + assert result == "{800, 448}" -def test_read_default_integer(): +def test_read_default_integer(PLIST_OUTPUT): """ Test reading a default integer setting """ - def custom_run_defaults_cmd(action, runas=None): - if action == 'read-type "com.apple.CrashReporter" "Crash"': - return {"retcode": 0, "stdout": "integer"} - elif action == 'read "com.apple.CrashReporter" "Crash"': - return {"retcode": 0, "stdout": "12"} - return {"retcode": 1, "stderr": f"Unknown action: {action}", "stdout": ""} + def custom_run_defaults_cmd(action, runas): + return {"retcode": 0, "stdout": PLIST_OUTPUT} mock = MagicMock(side_effect=custom_run_defaults_cmd) with patch("salt.modules.macdefaults._run_defaults_cmd", mock): - result = macdefaults.read("com.apple.CrashReporter", "Crash") - mock.assert_has_calls( - [ - call('read "com.apple.CrashReporter" "Crash"', runas=None), - call('read-type "com.apple.CrashReporter" "Crash"', runas=None), - ] - ) - assert result == 12 + result = macdefaults.read("com.googlecode.iterm2", "AppleAntiAliasingThreshold") + mock.assert_called_once_with('export "com.googlecode.iterm2" -', runas=None) + assert result == 1024 -def test_read_default_float(): +def test_read_default_float(PLIST_OUTPUT): """ Test reading a default float setting """ - def custom_run_defaults_cmd(action, runas=None): - if action == 'read-type "com.apple.CrashReporter" "Crash"': - return {"retcode": 0, "stdout": "float"} - elif action == 'read "com.apple.CrashReporter" "Crash"': - return {"retcode": 0, "stdout": "0.85"} - return {"retcode": 1, "stderr": f"Unknown action: {action}", "stdout": ""} + def custom_run_defaults_cmd(action, runas): + return {"retcode": 0, "stdout": PLIST_OUTPUT} mock = MagicMock(side_effect=custom_run_defaults_cmd) with patch("salt.modules.macdefaults._run_defaults_cmd", mock): - result = macdefaults.read("com.apple.CrashReporter", "Crash") - mock.assert_has_calls( - [ - call('read "com.apple.CrashReporter" "Crash"', runas=None), - call('read-type "com.apple.CrashReporter" "Crash"', runas=None), - ] - ) - assert result == 0.85 + result = macdefaults.read("com.googlecode.iterm2", "NoSyncNextAnnoyanceTime") + mock.assert_called_once_with('export "com.googlecode.iterm2" -', runas=None) + assert result == 699041775.9120981 -def test_read_default_array(): +def test_read_default_array(PLIST_OUTPUT): """ Test reading a default array setting """ - defaults_output = """( - element 1, - element 2, - 0.1000, - 1 - )""" - - def custom_run_defaults_cmd(action, runas=None): - if action == 'read-type "com.apple.CrashReporter" "Crash"': - return {"retcode": 0, "stdout": "array"} - elif action == 'read "com.apple.CrashReporter" "Crash"': - return {"retcode": 0, "stdout": defaults_output} - return {"retcode": 1, "stderr": f"Unknown action: {action}", "stdout": ""} + def custom_run_defaults_cmd(action, runas): + return {"retcode": 0, "stdout": PLIST_OUTPUT} mock = MagicMock(side_effect=custom_run_defaults_cmd) with patch("salt.modules.macdefaults._run_defaults_cmd", mock): - result = macdefaults.read("com.apple.CrashReporter", "Crash") - mock.assert_has_calls( - [ - call('read "com.apple.CrashReporter" "Crash"', runas=None), - call('read-type "com.apple.CrashReporter" "Crash"', runas=None), - ] - ) - assert result == ["element 1", "element 2", 0.1, 1] + result = macdefaults.read("com.googlecode.iterm2", "NoSyncBFPRecents") + mock.assert_called_once_with('export "com.googlecode.iterm2" -', runas=None) + assert result == ["MonoLisa Variable", "Menlo", "MonoLisa"] -def test_read_default_dictionary(): +def test_read_default_dictionary(PLIST_OUTPUT): """ Test reading a default dictionary setting """ - defaults_output = """{ - keyCode = 36; - modifierFlags = 786432; - anotherKey = "another value with spaces"; - floatNumber = 0.8500; - }""" - - def custom_run_defaults_cmd(action, runas=None): - if action == 'read-type "com.apple.CrashReporter" "Crash"': - return {"retcode": 0, "stdout": "dictionary"} - elif action == 'read "com.apple.CrashReporter" "Crash"': - return {"retcode": 0, "stdout": defaults_output} - return {"retcode": 1, "stderr": f"Unknown action: {action}", "stdout": ""} + def custom_run_defaults_cmd(action, runas): + return {"retcode": 0, "stdout": PLIST_OUTPUT} mock = MagicMock(side_effect=custom_run_defaults_cmd) with patch("salt.modules.macdefaults._run_defaults_cmd", mock): - result = macdefaults.read("com.apple.CrashReporter", "Crash") - mock.assert_has_calls( - [ - call('read "com.apple.CrashReporter" "Crash"', runas=None), - call('read-type "com.apple.CrashReporter" "Crash"', runas=None), - ] + result = macdefaults.read( + "com.googlecode.iterm2", "NoSyncFrame_SharedPreferences" ) + mock.assert_called_once_with('export "com.googlecode.iterm2" -', runas=None) assert result == { - "keyCode": 36, - "modifierFlags": 786432, - "anotherKey": "another value with spaces", - "floatNumber": 0.85, + "screenFrame": "{{0, 0}, {1920, 1200}}", + "topLeft": "{668, 1004}", } +def test_read_default_dictionary_nested_key(PLIST_OUTPUT): + """ + Test reading a default dictionary setting with a nested key + """ + + def custom_run_defaults_cmd(action, runas): + return {"retcode": 0, "stdout": PLIST_OUTPUT} + + mock = MagicMock(side_effect=custom_run_defaults_cmd) + with patch("salt.modules.macdefaults._run_defaults_cmd", mock): + result = macdefaults.read( + "com.googlecode.iterm2", "PointerActions.Button,1,1,,.Action" + ) + mock.assert_called_once_with('export "com.googlecode.iterm2" -', runas=None) + assert result == "kContextMenuPointerAction" + + +def test_read_default_dictionary_nested_key_with_array_indexes(PLIST_OUTPUT): + """ + Test reading a default dictionary setting with a nested key and array indexes + """ + + def custom_run_defaults_cmd(action, runas): + return {"retcode": 0, "stdout": PLIST_OUTPUT} + + mock = MagicMock(side_effect=custom_run_defaults_cmd) + with patch("salt.modules.macdefaults._run_defaults_cmd", mock): + # First index (exists) + result = macdefaults.read( + "com.googlecode.iterm2", + "NSSplitView Subview Frames NSColorPanelSplitView.0", + ) + mock.assert_called_once_with('export "com.googlecode.iterm2" -', runas=None) + assert result == "0.000000, 0.000000, 224.000000, 222.000000, NO, NO" + + # Second index (exists) + result = macdefaults.read( + "com.googlecode.iterm2", + "NSSplitView Subview Frames NSColorPanelSplitView.1", + ) + assert result == "0.000000, 223.000000, 224.000000, 48.000000, NO, NO" + + # Third index (does not exist) + result = macdefaults.read( + "com.googlecode.iterm2", + "NSSplitView Subview Frames NSColorPanelSplitView.2", + ) + assert result is None + + +def test_read_default_date(PLIST_OUTPUT): + """ + Test reading a default date setting + """ + + def custom_run_defaults_cmd(action, runas): + return {"retcode": 0, "stdout": PLIST_OUTPUT} + + mock = MagicMock(side_effect=custom_run_defaults_cmd) + with patch("salt.modules.macdefaults._run_defaults_cmd", mock): + result = macdefaults.read("com.googlecode.iterm2", "SULastCheckTime") + mock.assert_called_once_with('export "com.googlecode.iterm2" -', runas=None) + assert result == datetime.strptime("2024-06-23T09:33:44Z", "%Y-%m-%dT%H:%M:%SZ") + + def test_delete_default(): """ Test delete a default setting """ - mock = MagicMock(return_value={"retcode": 0}) - with patch("salt.modules.macdefaults._run_defaults_cmd", mock): - macdefaults.delete("com.apple.CrashReporter", "Crash") - mock.assert_called_once_with( - 'delete "com.apple.CrashReporter" "Crash"', - runas=None, + original_plist = { + "Crash": "bar", + "foo": 0, + } + + updated_plist = { + "foo": 0, + } + + result = {"retcode": 0, "stdout": "Removed key", "stderr": ""} + + load_plist_mock = MagicMock(return_value=original_plist) + export_plist_mock = MagicMock(return_value=result) + + with patch("salt.modules.macdefaults._load_plist", load_plist_mock), patch( + "salt.modules.macdefaults._save_plist", export_plist_mock + ): + assert result == macdefaults.delete("com.apple.CrashReporter", "Crash") + load_plist_mock.assert_called_once_with( + "com.apple.CrashReporter", + user=None, + ) + export_plist_mock.assert_called_once_with( + "com.apple.CrashReporter", + updated_plist, + user=None, ) @@ -342,10 +794,150 @@ def test_delete_default_with_user(): """ Test delete a default setting as a specific user """ - mock = MagicMock(return_value={"retcode": 0}) - with patch("salt.modules.macdefaults._run_defaults_cmd", mock): + load_plist_mock = MagicMock(return_value={"Crash": "bar"}) + export_plist_mock = MagicMock(return_value={"retcode": 0}) + + with patch("salt.modules.macdefaults._load_plist", load_plist_mock), patch( + "salt.modules.macdefaults._save_plist", export_plist_mock + ): macdefaults.delete("com.apple.CrashReporter", "Crash", user="frank") - mock.assert_called_once_with( - 'delete "com.apple.CrashReporter" "Crash"', - runas="frank", + load_plist_mock.assert_called_once_with( + "com.apple.CrashReporter", + user="frank", + ) + export_plist_mock.assert_called_once_with( + "com.apple.CrashReporter", + {}, + user="frank", + ) + + +def test_delete_default_with_nested_key(): + """ + Test delete a default setting with a nested key + """ + original_plist = { + "Crash": { + "foo": "bar", + "baz": 0, + } + } + + updated_plist = { + "Crash": { + "foo": "bar", + } + } + + result = {"retcode": 0, "stdout": "Removed key", "stderr": ""} + + load_plist_mock = MagicMock(return_value=original_plist) + export_plist_mock = MagicMock(return_value=result) + + with patch("salt.modules.macdefaults._load_plist", load_plist_mock), patch( + "salt.modules.macdefaults._save_plist", export_plist_mock + ): + assert result == macdefaults.delete("com.apple.CrashReporter", "Crash.baz") + load_plist_mock.assert_called_once_with( + "com.apple.CrashReporter", + user=None, + ) + export_plist_mock.assert_called_once_with( + "com.apple.CrashReporter", + updated_plist, + user=None, + ) + + +def test_delete_default_dictionary_nested_key_with_array_indexes(): + """ + Test delete a default dictionary setting with a nested key and array indexes + """ + original_plist = { + "Crash": { + "foo": "bar", + "baz": [ + {"internalKey1": 1, "internalKey2": "a"}, + {"internalKey1": 2, "internalKey2": "b"}, + {"internalKey1": 3, "internalKey2": "c"}, + ], + } + } + + updated_plist = { + "Crash": { + "foo": "bar", + "baz": [ + {"internalKey1": 1, "internalKey2": "a"}, + {"internalKey2": "b"}, + {"internalKey1": 3, "internalKey2": "c"}, + ], + } + } + + result = {"retcode": 0, "stdout": "Removed key", "stderr": ""} + + load_plist_mock = MagicMock(return_value=original_plist) + export_plist_mock = MagicMock(return_value=result) + + with patch("salt.modules.macdefaults._load_plist", load_plist_mock), patch( + "salt.modules.macdefaults._save_plist", export_plist_mock + ): + assert result == macdefaults.delete( + "com.apple.CrashReporter", "Crash.baz.1.internalKey1" + ) + load_plist_mock.assert_called_once_with( + "com.apple.CrashReporter", + user=None, + ) + export_plist_mock.assert_called_once_with( + "com.apple.CrashReporter", + updated_plist, + user=None, + ) + + +def test_delete_default_dictionary_nested_key_with_array_index_as_last_key(): + """ + Test delete a default dictionary setting with a nested key and array indexes + and the last element of the key is an array index + """ + original_plist = { + "Crash": { + "foo": "bar", + "baz": [ + {"internalKey1": 1, "internalKey2": "a"}, + {"internalKey1": 2, "internalKey2": "b"}, + {"internalKey1": 3, "internalKey2": "c"}, + ], + } + } + + updated_plist = { + "Crash": { + "foo": "bar", + "baz": [ + {"internalKey1": 1, "internalKey2": "a"}, + {"internalKey1": 3, "internalKey2": "c"}, + ], + } + } + + result = {"retcode": 0, "stdout": "Removed key", "stderr": ""} + + load_plist_mock = MagicMock(return_value=original_plist) + export_plist_mock = MagicMock(return_value=result) + + with patch("salt.modules.macdefaults._load_plist", load_plist_mock), patch( + "salt.modules.macdefaults._save_plist", export_plist_mock + ): + assert result == macdefaults.delete("com.apple.CrashReporter", "Crash.baz.1") + load_plist_mock.assert_called_once_with( + "com.apple.CrashReporter", + user=None, + ) + export_plist_mock.assert_called_once_with( + "com.apple.CrashReporter", + updated_plist, + user=None, ) diff --git a/tests/pytests/unit/states/test_macdefaults.py b/tests/pytests/unit/states/test_macdefaults.py index b293027ace83..ad1c8118ddc1 100644 --- a/tests/pytests/unit/states/test_macdefaults.py +++ b/tests/pytests/unit/states/test_macdefaults.py @@ -1,12 +1,19 @@ import pytest +import salt.modules.macdefaults as macdefaults_module import salt.states.macdefaults as macdefaults from tests.support.mock import MagicMock, patch @pytest.fixture def configure_loader_modules(): - return {macdefaults: {}} + return { + macdefaults: { + "__salt__": { + "macdefaults.cast_value_to_vtype": macdefaults_module.cast_value_to_vtype + } + } + } def test_write_default(): @@ -29,7 +36,7 @@ def test_write_default(): out = macdefaults.write("DialogType", "com.apple.CrashReporter", "Server") read_mock.assert_called_once_with("com.apple.CrashReporter", "DialogType", None) write_mock.assert_called_once_with( - "com.apple.CrashReporter", "DialogType", "Server", "string", None + "com.apple.CrashReporter", "DialogType", "Server", None, None ) assert out == expected @@ -74,7 +81,7 @@ def test_write_default_boolean(): macdefaults.__salt__, {"macdefaults.read": read_mock, "macdefaults.write": write_mock}, ): - out = macdefaults.write("Key", "com.apple.something", True, vtype="boolean") + out = macdefaults.write("Key", "com.apple.something", "YES", vtype="boolean") read_mock.assert_called_once_with("com.apple.something", "Key", None) write_mock.assert_called_once_with( "com.apple.something", "Key", True, "boolean", None @@ -122,10 +129,10 @@ def test_write_default_integer(): macdefaults.__salt__, {"macdefaults.read": read_mock, "macdefaults.write": write_mock}, ): - out = macdefaults.write("Key", "com.apple.something", 1337, vtype="integer") + out = macdefaults.write("Key", "com.apple.something", 1337) read_mock.assert_called_once_with("com.apple.something", "Key", None) write_mock.assert_called_once_with( - "com.apple.something", "Key", 1337, "integer", None + "com.apple.something", "Key", 1337, None, None ) assert out == expected @@ -170,7 +177,7 @@ def test_write_default_float(): macdefaults.__salt__, {"macdefaults.read": read_mock, "macdefaults.write": write_mock}, ): - out = macdefaults.write("Key", "com.apple.something", 0.865, vtype="float") + out = macdefaults.write("Key", "com.apple.something", "0.8650", vtype="float") read_mock.assert_called_once_with("com.apple.something", "Key", None) write_mock.assert_called_once_with( "com.apple.something", "Key", 0.865, "float", None @@ -195,7 +202,7 @@ def test_write_default_float_already_set(): macdefaults.__salt__, {"macdefaults.read": read_mock, "macdefaults.write": write_mock}, ): - out = macdefaults.write("Key", "com.apple.something", 0.86500, vtype="float") + out = macdefaults.write("Key", "com.apple.something", 0.86500) read_mock.assert_called_once_with("com.apple.something", "Key", None) assert not write_mock.called assert out == expected @@ -245,7 +252,7 @@ def test_write_default_array_already_set(): macdefaults.__salt__, {"macdefaults.read": read_mock, "macdefaults.write": write_mock}, ): - out = macdefaults.write("Key", "com.apple.something", value, vtype="array") + out = macdefaults.write("Key", "com.apple.something", value) read_mock.assert_called_once_with("com.apple.something", "Key", None) assert not write_mock.called assert out == expected @@ -283,9 +290,9 @@ def test_write_default_array_add(): def test_write_default_array_add_already_set_distinct_order(): """ Test writing a default setting adding an array to another that is already set - The new array is in a different order than the existing one + The new array is in a different order than the last elements of the existing one """ - write_value = ["a", 1] + write_value = [2, "a"] read_value = ["b", 1, "a", 2] expected = { "changes": {"written": f"com.apple.something Key is set to {write_value}"}, @@ -315,7 +322,7 @@ def test_write_default_array_add_already_set_same_order(): Test writing a default setting adding an array to another that is already set The new array is already in the same order as the existing one """ - write_value = ["a", 1] + write_value = [1, 2] read_value = ["b", "a", 1, 2] expected = { "changes": {}, @@ -455,7 +462,7 @@ def test_absent_default_already(): "result": True, } - mock = MagicMock(return_value={"retcode": 1}) + mock = MagicMock(return_value=None) with patch.dict(macdefaults.__salt__, {"macdefaults.delete": mock}): out = macdefaults.absent("Key", "com.apple.something") mock.assert_called_once_with("com.apple.something", "Key", None) From 8742299e986b672ef1da0ea05d1acce84ed95362 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20=C3=81lvaro?= Date: Thu, 27 Jun 2024 17:04:48 +0200 Subject: [PATCH 12/16] feat(macdefaults): catch write exceptions to return safely --- salt/modules/macdefaults.py | 24 +++++++++++++++++++----- salt/states/macdefaults.py | 21 ++++++++++++++++----- 2 files changed, 35 insertions(+), 10 deletions(-) diff --git a/salt/modules/macdefaults.py b/salt/modules/macdefaults.py index 31688a484a23..91318214b90e 100644 --- a/salt/modules/macdefaults.py +++ b/salt/modules/macdefaults.py @@ -254,18 +254,32 @@ def delete(domain, key, user=None): def cast_value_to_vtype(value, vtype): """ Convert the value to the specified vtype. - If the value cannot be converted, it will be returned as is. + If the value cannot be converted, a ValueError is raised. + + CLI Example: + + .. code-block:: bash + + salt '*' macdefaults.cast_value_to_vtype "1.35" "float" + + salt '*' macdefaults.cast_value_to_vtype "2024-06-23T09:33:44Z" "date" + + salt '*' macdefaults.cast_value_to_vtype "737720347.089987" "date" value The value to be converted vtype - The type to convert the value to + The type to convert the value to. + Valid types are string, int[eger], float, bool[ean], date, data, + array, array-add, dict, dict-add + + Raises: + ValueError: When the value cannot be converted to the specified type Returns: The converted value - """ # Boolean if vtype in ("bool", "boolean"): @@ -301,11 +315,11 @@ def cast_value_to_vtype(value, vtype): if not isinstance(value, datetime): try: value = datetime.fromtimestamp(float(value)) - except ValueError: + except ValueError as e: if re.match(r"\d{4}-\d{2}-\d{2}T\d{2}:\d{2}:\d{2}Z", value): value = datetime.strptime(value, "%Y-%m-%dT%H:%M:%SZ") else: - raise ValueError(f"Invalid date format: '{value}'") + raise ValueError(f"Invalid date format: '{value}'") from e # Data elif vtype == "data": if isinstance(value, str): diff --git a/salt/states/macdefaults.py b/salt/states/macdefaults.py index cd0e6df633e2..2aa5701df0de 100644 --- a/salt/states/macdefaults.py +++ b/salt/states/macdefaults.py @@ -33,7 +33,7 @@ def write(name, domain, value, vtype=None, user=None): The value to write to the given key vtype - The type of value to be written, valid types are string, data, int[eger], + The type of value to be written. Valid types are string, data, int[eger], float, bool[ean], date, array, array-add, dict, dict-add user @@ -45,17 +45,28 @@ def write(name, domain, value, vtype=None, user=None): current_value = __salt__["macdefaults.read"](domain, name, user) if vtype is not None: - value = __salt__["macdefaults.cast_value_to_vtype"](value, vtype) + try: + value = __salt__["macdefaults.cast_value_to_vtype"](value, vtype) + except ValueError as e: + ret["result"] = False + ret["comment"] = f"Failed to cast value {value} to {vtype}. {e}" + return ret if _compare_values(value, current_value, vtype): - ret["comment"] += f"{domain} {name} is already set to {value}" - else: + ret["comment"] = f"{domain} {name} is already set to {value}" + return ret + + try: out = __salt__["macdefaults.write"](domain, name, value, vtype, user) if out["retcode"] != 0: ret["result"] = False - ret["comment"] = f"Failed to write default. {out['stdout']}" + ret["comment"] = f"Failed to write default. {out['stderr']}" else: ret["changes"]["written"] = f"{domain} {name} is set to {value}" + # pylint: disable-next=broad-exception-caught + except Exception as e: + ret["result"] = False + ret["comment"] = f"Failed to write default. {e}" return ret From 5124936f9e66afae181114c85fa41eab91fde375 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20=C3=81lvaro?= Date: Thu, 27 Jun 2024 21:03:00 +0200 Subject: [PATCH 13/16] feat(macdefaults): Allow selecting key separator --- salt/modules/macdefaults.py | 43 +++-- salt/states/macdefaults.py | 24 ++- .../pytests/unit/modules/test_macdefaults.py | 73 ++++++-- tests/pytests/unit/states/test_macdefaults.py | 160 +++++++++++++----- 4 files changed, 229 insertions(+), 71 deletions(-) diff --git a/salt/modules/macdefaults.py b/salt/modules/macdefaults.py index 91318214b90e..1ebe394f9538 100644 --- a/salt/modules/macdefaults.py +++ b/salt/modules/macdefaults.py @@ -42,6 +42,7 @@ def write( value, vtype=None, user=None, + key_separator=None, dict_merge=False, array_add=False, type=None, @@ -55,10 +56,12 @@ def write( salt '*' macdefaults.write com.apple.Finder DownloadsFolderListViewSettingsVersion 1 - salt '*' macdefaults.write com.apple.Finder ComputerViewSettings.CustomViewStyle "icnv" + salt '*' macdefaults.write com.apple.Finder ComputerViewSettings.CustomViewStyle "icnv" key_separator='.' salt '*' macdefaults.write com.apple.Dock lastShowIndicatorTime 737720347.089987 vtype=date + salt '*' macdefaults.write NSGlobalDomain com.apple.sound.beep.sound "/System/Library/Sounds/Blow.aiff" + domain The name of the domain to write to @@ -66,6 +69,10 @@ def write( The key of the given domain to write to. It can be a nested key/index separated by dots. + key_separator + The separator to use when splitting the key into a list of keys. + If None, the key will not be split (Default). + value The value to write to the given key. Dates should be in the format 'YYYY-MM-DDTHH:MM:SSZ' @@ -122,7 +129,7 @@ def write( ) plist = _load_plist(domain, user=user) or {} - keys = key.split(".") + keys = [key] if key_separator is None else key.split(key_separator) last_key = keys[-1] # Traverse the plist @@ -161,7 +168,7 @@ def write( return _save_plist(domain, plist, user=user) -def read(domain, key, user=None): +def read(domain, key, user=None, key_separator=None): """ Read a default from the system @@ -169,10 +176,12 @@ def read(domain, key, user=None): .. code-block:: bash - salt '*' macdefaults.read com.apple.Dock persistent-apps.1.title-data.file-label - salt '*' macdefaults.read NSGlobalDomain ApplePersistence + salt '*' macdefaults.read NSGlobalDomain key.with.dots-subKey key_separator="-" + + salt '*' macdefaults.read com.apple.Dock persistent-apps.1.title-data.file-label key_separator='.' + domain The name of the domain to read from @@ -180,6 +189,10 @@ def read(domain, key, user=None): The key of the given domain to read from. It can be a nested key/index separated by dots. + key_separator + The separator to use when splitting the key into a list of keys. + If None, the key will not be split (Default). + user The user to read the defaults from @@ -191,10 +204,12 @@ def read(domain, key, user=None): if plist is None: return None - return _traverse_keys(plist, key.split(".")) + keys = [key] if key_separator is None else key.split(key_separator) + + return _traverse_keys(plist, keys) -def delete(domain, key, user=None): +def delete(domain, key, user=None, key_separator=None): """ Delete a default from the system @@ -206,6 +221,8 @@ def delete(domain, key, user=None): salt '*' macdefaults.delete NSGlobalDomain ApplePersistence + salt '*' macdefaults.delete NSGlobalDomain key.with.dots key_separator='.'' + domain The name of the domain to delete from @@ -213,6 +230,10 @@ def delete(domain, key, user=None): The key of the given domain to delete. It can be a nested key separated by dots. + key_separator + The separator to use when splitting the key into a list of keys. + If None, the key will not be split (Default). + user The user to delete the defaults with @@ -221,7 +242,7 @@ def delete(domain, key, user=None): if plist is None: return None - keys = key.split(".") + keys = [key] if key_separator is None else key.split(key_separator) # Traverse the plist til the penultimate key. # Last key must be handled separately since we @@ -390,9 +411,11 @@ def _save_plist(domain, plist, user=None): Returns: A dictionary with the defaults command result """ - with tempfile.NamedTemporaryFile() as tmpfile: - plistlib.dump(plist, tmpfile) + with tempfile.TemporaryFile(suffix=".plist") as tmpfile: + contents = plistlib.dumps(plist) + tmpfile.write(contents) tmpfile.flush() + tmpfile.seek(0) cmd = f'import "{domain}" "{tmpfile.name}"' return _run_defaults_cmd(cmd, runas=user) diff --git a/salt/states/macdefaults.py b/salt/states/macdefaults.py index 2aa5701df0de..a2491cce5efc 100644 --- a/salt/states/macdefaults.py +++ b/salt/states/macdefaults.py @@ -18,7 +18,7 @@ def __virtual__(): return (False, "Only supported on macOS") -def write(name, domain, value, vtype=None, user=None): +def write(name, domain, value, vtype=None, name_separator=None, user=None): """ Write a default to the system @@ -26,6 +26,10 @@ def write(name, domain, value, vtype=None, user=None): The key of the given domain to write to. It can be a nested key/index separated by dots. + name_separator + The separator to use when splitting the name into a list of keys. + If None, the name will not be split (Default). + domain The name of the domain to write to @@ -42,7 +46,9 @@ def write(name, domain, value, vtype=None, user=None): """ ret = {"name": name, "result": True, "comment": "", "changes": {}} - current_value = __salt__["macdefaults.read"](domain, name, user) + current_value = __salt__["macdefaults.read"]( + domain, name, user, key_separator=name_separator + ) if vtype is not None: try: @@ -57,7 +63,9 @@ def write(name, domain, value, vtype=None, user=None): return ret try: - out = __salt__["macdefaults.write"](domain, name, value, vtype, user) + out = __salt__["macdefaults.write"]( + domain, name, value, vtype, user, key_separator=name_separator + ) if out["retcode"] != 0: ret["result"] = False ret["comment"] = f"Failed to write default. {out['stderr']}" @@ -71,7 +79,7 @@ def write(name, domain, value, vtype=None, user=None): return ret -def absent(name, domain, user=None): +def absent(name, domain, user=None, name_separator=None): """ Make sure the defaults value is absent @@ -79,6 +87,10 @@ def absent(name, domain, user=None): The key of the given domain to remove. It can be a nested key/index separated by dots. + name_separator + The separator to use when splitting the name into a list of keys. + If None, the name will not be split (Default). + domain The name of the domain to remove from @@ -88,7 +100,9 @@ def absent(name, domain, user=None): """ ret = {"name": name, "result": True, "comment": "", "changes": {}} - out = __salt__["macdefaults.delete"](domain, name, user) + out = __salt__["macdefaults.delete"]( + domain, name, user, key_separator=name_separator + ) if out is None or out["retcode"] != 0: ret["comment"] += f"{domain} {name} is already absent" diff --git a/tests/pytests/unit/modules/test_macdefaults.py b/tests/pytests/unit/modules/test_macdefaults.py index f2511b308462..a88ef71c3cdd 100644 --- a/tests/pytests/unit/modules/test_macdefaults.py +++ b/tests/pytests/unit/modules/test_macdefaults.py @@ -165,30 +165,39 @@ def side_effect_run_defaults_command(*args, **kwargs): tempfile_mock = MagicMock() tempfile_mock.name = "/tmp/tmpfile" + tempfile_mock.write = MagicMock( + side_effect=lambda contents: calls.append("tempfile.write") + ) tempfile_mock.flush = MagicMock(side_effect=lambda: calls.append("tempfile.flush")) + tempfile_mock.seek = MagicMock( + side_effect=lambda x: calls.append(f"tempfile.seek({x})") + ) named_temporary_file_mock = MagicMock() named_temporary_file_mock.return_value.__enter__.return_value = tempfile_mock - plist_mock = MagicMock( - side_effect=lambda _dict, _file: calls.append("plistlib.dump") - ) + def side_effect_plistlib_dumps(plist): + calls.append("plistlib.dumps") + return str(plist).encode() + + plist_mock = MagicMock(side_effect=side_effect_plistlib_dumps) with patch( "salt.modules.macdefaults._run_defaults_cmd", run_defaults_cmd_mock - ), patch("tempfile.NamedTemporaryFile", named_temporary_file_mock), patch( - "plistlib.dump", plist_mock + ), patch("tempfile.TemporaryFile", named_temporary_file_mock), patch( + "plistlib.dumps", plist_mock ): result = macdefaults._save_plist("com.googlecode.iterm2", new_plist) - tempfile_mock.flush.assert_called() - plist_mock.assert_called_once_with(new_plist, tempfile_mock) + plist_mock.assert_called_once_with(new_plist) run_defaults_cmd_mock.assert_called_once_with( 'import "com.googlecode.iterm2" "/tmp/tmpfile"', runas=None ) assert result == expected_result assert calls == [ - "plistlib.dump", + "plistlib.dumps", + "tempfile.write", "tempfile.flush", + "tempfile.seek(0)", "macdefaults._run_defaults_cmd", ] @@ -703,7 +712,9 @@ def custom_run_defaults_cmd(action, runas): mock = MagicMock(side_effect=custom_run_defaults_cmd) with patch("salt.modules.macdefaults._run_defaults_cmd", mock): result = macdefaults.read( - "com.googlecode.iterm2", "PointerActions.Button,1,1,,.Action" + "com.googlecode.iterm2", + "PointerActions.Button,1,1,,.Action", + key_separator=".", ) mock.assert_called_once_with('export "com.googlecode.iterm2" -', runas=None) assert result == "kContextMenuPointerAction" @@ -723,6 +734,7 @@ def custom_run_defaults_cmd(action, runas): result = macdefaults.read( "com.googlecode.iterm2", "NSSplitView Subview Frames NSColorPanelSplitView.0", + key_separator=".", ) mock.assert_called_once_with('export "com.googlecode.iterm2" -', runas=None) assert result == "0.000000, 0.000000, 224.000000, 222.000000, NO, NO" @@ -731,6 +743,7 @@ def custom_run_defaults_cmd(action, runas): result = macdefaults.read( "com.googlecode.iterm2", "NSSplitView Subview Frames NSColorPanelSplitView.1", + key_separator=".", ) assert result == "0.000000, 223.000000, 224.000000, 48.000000, NO, NO" @@ -738,6 +751,7 @@ def custom_run_defaults_cmd(action, runas): result = macdefaults.read( "com.googlecode.iterm2", "NSSplitView Subview Frames NSColorPanelSplitView.2", + key_separator=".", ) assert result is None @@ -794,20 +808,37 @@ def test_delete_default_with_user(): """ Test delete a default setting as a specific user """ - load_plist_mock = MagicMock(return_value={"Crash": "bar"}) - export_plist_mock = MagicMock(return_value={"retcode": 0}) + original_plist = { + "Crash": { + "foo": "bar", + "baz": 0, + }, + "Crash.baz": 0, + } + + updated_plist = { + "Crash": { + "foo": "bar", + "baz": 0, + }, + } + + result = {"retcode": 0, "stdout": "Removed key", "stderr": ""} + + load_plist_mock = MagicMock(return_value=original_plist) + export_plist_mock = MagicMock(return_value=result) with patch("salt.modules.macdefaults._load_plist", load_plist_mock), patch( "salt.modules.macdefaults._save_plist", export_plist_mock ): - macdefaults.delete("com.apple.CrashReporter", "Crash", user="frank") + macdefaults.delete("com.apple.CrashReporter", "Crash.baz", user="frank") load_plist_mock.assert_called_once_with( "com.apple.CrashReporter", user="frank", ) export_plist_mock.assert_called_once_with( "com.apple.CrashReporter", - {}, + updated_plist, user="frank", ) @@ -837,7 +868,11 @@ def test_delete_default_with_nested_key(): with patch("salt.modules.macdefaults._load_plist", load_plist_mock), patch( "salt.modules.macdefaults._save_plist", export_plist_mock ): - assert result == macdefaults.delete("com.apple.CrashReporter", "Crash.baz") + assert result == macdefaults.delete( + "com.apple.CrashReporter", + "Crash.baz", + key_separator=".", + ) load_plist_mock.assert_called_once_with( "com.apple.CrashReporter", user=None, @@ -884,7 +919,9 @@ def test_delete_default_dictionary_nested_key_with_array_indexes(): "salt.modules.macdefaults._save_plist", export_plist_mock ): assert result == macdefaults.delete( - "com.apple.CrashReporter", "Crash.baz.1.internalKey1" + "com.apple.CrashReporter", + "Crash.baz.1.internalKey1", + key_separator=".", ) load_plist_mock.assert_called_once_with( "com.apple.CrashReporter", @@ -931,7 +968,11 @@ def test_delete_default_dictionary_nested_key_with_array_index_as_last_key(): with patch("salt.modules.macdefaults._load_plist", load_plist_mock), patch( "salt.modules.macdefaults._save_plist", export_plist_mock ): - assert result == macdefaults.delete("com.apple.CrashReporter", "Crash.baz.1") + assert result == macdefaults.delete( + "com.apple.CrashReporter", + "Crash.baz.1", + key_separator=".", + ) load_plist_mock.assert_called_once_with( "com.apple.CrashReporter", user=None, diff --git a/tests/pytests/unit/states/test_macdefaults.py b/tests/pytests/unit/states/test_macdefaults.py index ad1c8118ddc1..4de60bb2d398 100644 --- a/tests/pytests/unit/states/test_macdefaults.py +++ b/tests/pytests/unit/states/test_macdefaults.py @@ -34,9 +34,16 @@ def test_write_default(): {"macdefaults.read": read_mock, "macdefaults.write": write_mock}, ): out = macdefaults.write("DialogType", "com.apple.CrashReporter", "Server") - read_mock.assert_called_once_with("com.apple.CrashReporter", "DialogType", None) + read_mock.assert_called_once_with( + "com.apple.CrashReporter", "DialogType", None, key_separator=None + ) write_mock.assert_called_once_with( - "com.apple.CrashReporter", "DialogType", "Server", None, None + "com.apple.CrashReporter", + "DialogType", + "Server", + None, + None, + key_separator=None, ) assert out == expected @@ -45,10 +52,12 @@ def test_write_default_already_set(): """ Test writing a default setting that is already set """ + key = "DialogType.Value" + expected = { "changes": {}, - "comment": "com.apple.CrashReporter DialogType is already set to Server", - "name": "DialogType", + "comment": f"com.apple.CrashReporter {key} is already set to Server", + "name": key, "result": True, } @@ -58,8 +67,12 @@ def test_write_default_already_set(): macdefaults.__salt__, {"macdefaults.read": read_mock, "macdefaults.write": write_mock}, ): - out = macdefaults.write("DialogType", "com.apple.CrashReporter", "Server") - read_mock.assert_called_once_with("com.apple.CrashReporter", "DialogType", None) + out = macdefaults.write( + key, "com.apple.CrashReporter", "Server", name_separator="." + ) + read_mock.assert_called_once_with( + "com.apple.CrashReporter", key, None, key_separator="." + ) assert not write_mock.called assert out == expected @@ -82,9 +95,11 @@ def test_write_default_boolean(): {"macdefaults.read": read_mock, "macdefaults.write": write_mock}, ): out = macdefaults.write("Key", "com.apple.something", "YES", vtype="boolean") - read_mock.assert_called_once_with("com.apple.something", "Key", None) + read_mock.assert_called_once_with( + "com.apple.something", "Key", None, key_separator=None + ) write_mock.assert_called_once_with( - "com.apple.something", "Key", True, "boolean", None + "com.apple.something", "Key", True, "boolean", None, key_separator=None ) assert out == expected @@ -107,7 +122,9 @@ def test_write_default_boolean_already_set(): {"macdefaults.read": read_mock, "macdefaults.write": write_mock}, ): out = macdefaults.write("Key", "com.apple.something", "YES", vtype="boolean") - read_mock.assert_called_once_with("com.apple.something", "Key", None) + read_mock.assert_called_once_with( + "com.apple.something", "Key", None, key_separator=None + ) assert not write_mock.called assert out == expected @@ -130,9 +147,11 @@ def test_write_default_integer(): {"macdefaults.read": read_mock, "macdefaults.write": write_mock}, ): out = macdefaults.write("Key", "com.apple.something", 1337) - read_mock.assert_called_once_with("com.apple.something", "Key", None) + read_mock.assert_called_once_with( + "com.apple.something", "Key", None, key_separator=None + ) write_mock.assert_called_once_with( - "com.apple.something", "Key", 1337, None, None + "com.apple.something", "Key", 1337, None, None, key_separator=None ) assert out == expected @@ -141,10 +160,12 @@ def test_write_default_integer_already_set(): """ Test writing a default setting with an integer that is already set """ + key = "Key.subKey" + expected = { "changes": {}, - "comment": "com.apple.something Key is already set to 1337", - "name": "Key", + "comment": f"com.apple.something {key} is already set to 1337", + "name": key, "result": True, } @@ -154,8 +175,16 @@ def test_write_default_integer_already_set(): macdefaults.__salt__, {"macdefaults.read": read_mock, "macdefaults.write": write_mock}, ): - out = macdefaults.write("Key", "com.apple.something", 1337, vtype="integer") - read_mock.assert_called_once_with("com.apple.something", "Key", None) + out = macdefaults.write( + key, + "com.apple.something", + 1337, + vtype="integer", + name_separator=".", + ) + read_mock.assert_called_once_with( + "com.apple.something", key, None, key_separator="." + ) assert not write_mock.called assert out == expected @@ -178,9 +207,11 @@ def test_write_default_float(): {"macdefaults.read": read_mock, "macdefaults.write": write_mock}, ): out = macdefaults.write("Key", "com.apple.something", "0.8650", vtype="float") - read_mock.assert_called_once_with("com.apple.something", "Key", None) + read_mock.assert_called_once_with( + "com.apple.something", "Key", None, key_separator=None + ) write_mock.assert_called_once_with( - "com.apple.something", "Key", 0.865, "float", None + "com.apple.something", "Key", 0.865, "float", None, key_separator=None ) assert out == expected @@ -203,7 +234,9 @@ def test_write_default_float_already_set(): {"macdefaults.read": read_mock, "macdefaults.write": write_mock}, ): out = macdefaults.write("Key", "com.apple.something", 0.86500) - read_mock.assert_called_once_with("com.apple.something", "Key", None) + read_mock.assert_called_once_with( + "com.apple.something", "Key", None, key_separator=None + ) assert not write_mock.called assert out == expected @@ -212,11 +245,13 @@ def test_write_default_array(): """ Test writing a default setting with an array """ + key = "Key.subKey" + value = ["a", 1, 0.5, True] expected = { - "changes": {"written": f"com.apple.something Key is set to {value}"}, + "changes": {"written": f"com.apple.something {key} is set to {value}"}, "comment": "", - "name": "Key", + "name": key, "result": True, } @@ -226,10 +261,18 @@ def test_write_default_array(): macdefaults.__salt__, {"macdefaults.read": read_mock, "macdefaults.write": write_mock}, ): - out = macdefaults.write("Key", "com.apple.something", value, vtype="array") - read_mock.assert_called_once_with("com.apple.something", "Key", None) + out = macdefaults.write( + key, + "com.apple.something", + value, + vtype="array", + name_separator=".", + ) + read_mock.assert_called_once_with( + "com.apple.something", key, None, key_separator="." + ) write_mock.assert_called_once_with( - "com.apple.something", "Key", value, "array", None + "com.apple.something", key, value, "array", None, key_separator="." ) assert out == expected @@ -253,7 +296,9 @@ def test_write_default_array_already_set(): {"macdefaults.read": read_mock, "macdefaults.write": write_mock}, ): out = macdefaults.write("Key", "com.apple.something", value) - read_mock.assert_called_once_with("com.apple.something", "Key", None) + read_mock.assert_called_once_with( + "com.apple.something", "Key", None, key_separator=None + ) assert not write_mock.called assert out == expected @@ -280,9 +325,16 @@ def test_write_default_array_add(): out = macdefaults.write( "Key", "com.apple.something", write_value, vtype="array-add" ) - read_mock.assert_called_once_with("com.apple.something", "Key", None) + read_mock.assert_called_once_with( + "com.apple.something", "Key", None, key_separator=None + ) write_mock.assert_called_once_with( - "com.apple.something", "Key", write_value, "array-add", None + "com.apple.something", + "Key", + write_value, + "array-add", + None, + key_separator=None, ) assert out == expected @@ -310,9 +362,16 @@ def test_write_default_array_add_already_set_distinct_order(): out = macdefaults.write( "Key", "com.apple.something", write_value, vtype="array-add" ) - read_mock.assert_called_once_with("com.apple.something", "Key", None) + read_mock.assert_called_once_with( + "com.apple.something", "Key", None, key_separator=None + ) write_mock.assert_called_once_with( - "com.apple.something", "Key", write_value, "array-add", None + "com.apple.something", + "Key", + write_value, + "array-add", + None, + key_separator=None, ) assert out == expected @@ -340,7 +399,9 @@ def test_write_default_array_add_already_set_same_order(): out = macdefaults.write( "Key", "com.apple.something", write_value, vtype="array-add" ) - read_mock.assert_called_once_with("com.apple.something", "Key", None) + read_mock.assert_called_once_with( + "com.apple.something", "Key", None, key_separator=None + ) assert not write_mock.called assert out == expected @@ -364,9 +425,11 @@ def test_write_default_dict(): {"macdefaults.read": read_mock, "macdefaults.write": write_mock}, ): out = macdefaults.write("Key", "com.apple.something", value, vtype="dict") - read_mock.assert_called_once_with("com.apple.something", "Key", None) + read_mock.assert_called_once_with( + "com.apple.something", "Key", None, key_separator=None + ) write_mock.assert_called_once_with( - "com.apple.something", "Key", value, "dict", None + "com.apple.something", "Key", value, "dict", None, key_separator=None ) assert out == expected @@ -390,7 +453,9 @@ def test_write_default_dict_already_set(): {"macdefaults.read": read_mock, "macdefaults.write": write_mock}, ): out = macdefaults.write("Key", "com.apple.something", value, vtype="dict") - read_mock.assert_called_once_with("com.apple.something", "Key", None) + read_mock.assert_called_once_with( + "com.apple.something", "Key", None, key_separator=None + ) assert not write_mock.called assert out == expected @@ -417,9 +482,16 @@ def test_write_default_dict_add(): out = macdefaults.write( "Key", "com.apple.something", write_value, vtype="dict-add" ) - read_mock.assert_called_once_with("com.apple.something", "Key", None) + read_mock.assert_called_once_with( + "com.apple.something", "Key", None, key_separator=None + ) write_mock.assert_called_once_with( - "com.apple.something", "Key", write_value, "dict-add", None + "com.apple.something", + "Key", + write_value, + "dict-add", + None, + key_separator=None, ) assert out == expected @@ -446,7 +518,9 @@ def test_write_default_dict_add_already_set(): out = macdefaults.write( "Key", "com.apple.something", write_value, vtype="dict-add" ) - read_mock.assert_called_once_with("com.apple.something", "Key", None) + read_mock.assert_called_once_with( + "com.apple.something", "Key", None, key_separator=None + ) assert not write_mock.called assert out == expected @@ -465,7 +539,9 @@ def test_absent_default_already(): mock = MagicMock(return_value=None) with patch.dict(macdefaults.__salt__, {"macdefaults.delete": mock}): out = macdefaults.absent("Key", "com.apple.something") - mock.assert_called_once_with("com.apple.something", "Key", None) + mock.assert_called_once_with( + "com.apple.something", "Key", None, key_separator=None + ) assert out == expected @@ -473,15 +549,19 @@ def test_absent_default_deleting_existing(): """ Test removing an existing default value """ + key = "Key.subKey" + expected = { - "changes": {"absent": "com.apple.something Key is now absent"}, + "changes": {"absent": f"com.apple.something {key} is now absent"}, "comment": "", - "name": "Key", + "name": key, "result": True, } mock = MagicMock(return_value={"retcode": 0}) with patch.dict(macdefaults.__salt__, {"macdefaults.delete": mock}): - out = macdefaults.absent("Key", "com.apple.something") - mock.assert_called_once_with("com.apple.something", "Key", None) + out = macdefaults.absent(key, "com.apple.something", name_separator=".") + mock.assert_called_once_with( + "com.apple.something", key, None, key_separator="." + ) assert out == expected From dd5575e90f62e19b5a9da5adac9c7c219416789d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20=C3=81lvaro?= Date: Fri, 28 Jun 2024 10:42:01 +0200 Subject: [PATCH 14/16] fix(macdefaults): Use TemporaryDirectory instead of TemporaryFile TemporaryFile has no name, so NamedTemporaryFile would be a better option. However, until Python 3.12, this file is deleted on close, which is not valid for this purpose. TemporaryDirectory is used instead. --- salt/modules/macdefaults.py | 18 +++-- .../pytests/unit/modules/test_macdefaults.py | 65 +++++++++---------- 2 files changed, 41 insertions(+), 42 deletions(-) diff --git a/salt/modules/macdefaults.py b/salt/modules/macdefaults.py index 1ebe394f9538..aac184fe71fc 100644 --- a/salt/modules/macdefaults.py +++ b/salt/modules/macdefaults.py @@ -411,12 +411,18 @@ def _save_plist(domain, plist, user=None): Returns: A dictionary with the defaults command result """ - with tempfile.TemporaryFile(suffix=".plist") as tmpfile: - contents = plistlib.dumps(plist) - tmpfile.write(contents) - tmpfile.flush() - tmpfile.seek(0) - cmd = f'import "{domain}" "{tmpfile.name}"' + with tempfile.TemporaryDirectory(prefix="salt_macdefaults") as tempdir: + # File must exist until the defaults command is run. + # That's why a temporary directory is used instead of a temporary file. + # NOTE: Starting with Python 3.12 NamedTemporaryFile has the parameter + # delete_on_close which can be set to False. It would simplify this method. + file_name = __salt__["file.join"](tempdir, f"{domain}.plist") + with open(file_name, "wb") as fp: + plistlib.dump(plist, fp) + if user is not None: + __salt__["file.chown"](tempdir, user, None) + __salt__["file.chown"](file_name, user, None) + cmd = f'import "{domain}" "{file_name}"' return _run_defaults_cmd(cmd, runas=user) diff --git a/tests/pytests/unit/modules/test_macdefaults.py b/tests/pytests/unit/modules/test_macdefaults.py index a88ef71c3cdd..af17aa1cde74 100644 --- a/tests/pytests/unit/modules/test_macdefaults.py +++ b/tests/pytests/unit/modules/test_macdefaults.py @@ -1,9 +1,11 @@ +import tempfile from datetime import datetime import pytest +import salt.modules.file import salt.modules.macdefaults as macdefaults -from tests.support.mock import MagicMock, patch +from tests.support.mock import ANY, MagicMock, call, patch @pytest.fixture @@ -155,51 +157,42 @@ def test_save_plist(): new_plist = {"Crash": "Server"} expected_result = {"retcode": 0, "stdout": "", "stderr": ""} - calls = [] + chown_mock = MagicMock() - def side_effect_run_defaults_command(*args, **kwargs): - calls.append("macdefaults._run_defaults_cmd") - return expected_result - - run_defaults_cmd_mock = MagicMock(side_effect=side_effect_run_defaults_command) + tempdir = tempfile.TemporaryDirectory() + tempdir_name = tempdir.name tempfile_mock = MagicMock() - tempfile_mock.name = "/tmp/tmpfile" - tempfile_mock.write = MagicMock( - side_effect=lambda contents: calls.append("tempfile.write") - ) - tempfile_mock.flush = MagicMock(side_effect=lambda: calls.append("tempfile.flush")) - tempfile_mock.seek = MagicMock( - side_effect=lambda x: calls.append(f"tempfile.seek({x})") - ) + tempfile_mock.__enter__.return_value = tempdir_name - named_temporary_file_mock = MagicMock() - named_temporary_file_mock.return_value.__enter__.return_value = tempfile_mock + user = "cdalvaro" + domain = "com.googlecode.iterm2" + tmp_file_name = salt.modules.file.join(tempdir_name, f"{domain}.plist") - def side_effect_plistlib_dumps(plist): - calls.append("plistlib.dumps") - return str(plist).encode() - - plist_mock = MagicMock(side_effect=side_effect_plistlib_dumps) + chown_expected_calls = [ + call(tempdir_name, user, None), + call(tmp_file_name, user, None), + ] with patch( - "salt.modules.macdefaults._run_defaults_cmd", run_defaults_cmd_mock - ), patch("tempfile.TemporaryFile", named_temporary_file_mock), patch( - "plistlib.dumps", plist_mock + "salt.modules.macdefaults._run_defaults_cmd", return_value=expected_result + ) as run_defaults_cmd_mock, patch( + "tempfile.TemporaryDirectory", return_value=tempfile_mock + ), patch( + "plistlib.dump" + ) as plist_mock, patch.dict( + macdefaults.__salt__, + {"file.chown": chown_mock, "file.join": salt.modules.file.join}, ): - result = macdefaults._save_plist("com.googlecode.iterm2", new_plist) - plist_mock.assert_called_once_with(new_plist) + result = macdefaults._save_plist("com.googlecode.iterm2", new_plist, user=user) + assert result == expected_result + + plist_mock.assert_called_once_with(new_plist, ANY) run_defaults_cmd_mock.assert_called_once_with( - 'import "com.googlecode.iterm2" "/tmp/tmpfile"', runas=None + f'import "{domain}" "{tmp_file_name}"', + runas=user, ) - assert result == expected_result - assert calls == [ - "plistlib.dumps", - "tempfile.write", - "tempfile.flush", - "tempfile.seek(0)", - "macdefaults._run_defaults_cmd", - ] + chown_mock.assert_has_calls(chown_expected_calls) def test_cast_value_to_bool(): From 83225aca7d9c7c3b735cdc05e4afd635e79d78fc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20=C3=81lvaro?= Date: Fri, 28 Jun 2024 11:05:48 +0200 Subject: [PATCH 15/16] fix(macdefaults): Use salt.utils.files.open --- salt/modules/macdefaults.py | 34 ++++++++++----- salt/states/macdefaults.py | 4 +- .../pytests/unit/modules/test_macdefaults.py | 43 ++++++++++++++++++- 3 files changed, 67 insertions(+), 14 deletions(-) diff --git a/salt/modules/macdefaults.py b/salt/modules/macdefaults.py index aac184fe71fc..237aa09f183b 100644 --- a/salt/modules/macdefaults.py +++ b/salt/modules/macdefaults.py @@ -18,6 +18,7 @@ import tempfile from datetime import datetime +import salt.utils.files import salt.utils.platform import salt.utils.versions from salt.exceptions import CommandExecutionError @@ -67,7 +68,7 @@ def write( key The key of the given domain to write to. - It can be a nested key/index separated by dots. + It can be a nested key/index separated by `key_separator`. key_separator The separator to use when splitting the key into a list of keys. @@ -78,14 +79,15 @@ def write( Dates should be in the format 'YYYY-MM-DDTHH:MM:SSZ' vtype - The type of value to be written, valid types are string, int[eger], - float, bool[ean], date and data. + The type of value to be written. + + Valid types are string, int[eger], float, bool[ean], date and data. dict and array are also valid types but are only used for validation. - dict-add and array-add are supported for backward compatibility. - However, their corresponding sibling options dict_merge and array_add - are recommended. + dict-add and array-add are supported too. + They will behave as their counterparts dict and array but will set + their corresponding sibling options dict_merge and array_add to True. This parameter is optional. It will be used to cast the values to the specified type before writing them to the system. If not provided, the @@ -187,7 +189,7 @@ def read(domain, key, user=None, key_separator=None): key The key of the given domain to read from. - It can be a nested key/index separated by dots. + It can be a nested key/index separated by `key_separator`. key_separator The separator to use when splitting the key into a list of keys. @@ -221,14 +223,14 @@ def delete(domain, key, user=None, key_separator=None): salt '*' macdefaults.delete NSGlobalDomain ApplePersistence - salt '*' macdefaults.delete NSGlobalDomain key.with.dots key_separator='.'' + salt '*' macdefaults.delete NSGlobalDomain key.with.dots key_separator='.' domain The name of the domain to delete from key The key of the given domain to delete. - It can be a nested key separated by dots. + It can be a nested key separated by `key_separator`. key_separator The separator to use when splitting the key into a list of keys. @@ -417,7 +419,7 @@ def _save_plist(domain, plist, user=None): # NOTE: Starting with Python 3.12 NamedTemporaryFile has the parameter # delete_on_close which can be set to False. It would simplify this method. file_name = __salt__["file.join"](tempdir, f"{domain}.plist") - with open(file_name, "wb") as fp: + with salt.utils.files.fopen(file_name, "wb") as fp: plistlib.dump(plist, fp) if user is not None: __salt__["file.chown"](tempdir, user, None) @@ -427,6 +429,18 @@ def _save_plist(domain, plist, user=None): def _traverse_keys(plist, keys): + """ + Returns the value of the given keys in the plist + + plist + The plist dictionary to retrieve the value from + + keys + An array with the sequence of keys to traverse + + Returns: + The value of the given keys in the plist, or None if the keys do not exist. + """ value = plist for k in keys: if isinstance(value, dict): diff --git a/salt/states/macdefaults.py b/salt/states/macdefaults.py index a2491cce5efc..09a1fde04b7d 100644 --- a/salt/states/macdefaults.py +++ b/salt/states/macdefaults.py @@ -24,7 +24,7 @@ def write(name, domain, value, vtype=None, name_separator=None, user=None): name The key of the given domain to write to. - It can be a nested key/index separated by dots. + It can be a nested key/index separated by `name_separator`. name_separator The separator to use when splitting the name into a list of keys. @@ -85,7 +85,7 @@ def absent(name, domain, user=None, name_separator=None): name The key of the given domain to remove. - It can be a nested key/index separated by dots. + It can be a nested key/index separated by `name_separator`. name_separator The separator to use when splitting the name into a list of keys. diff --git a/tests/pytests/unit/modules/test_macdefaults.py b/tests/pytests/unit/modules/test_macdefaults.py index af17aa1cde74..8210331eb4fe 100644 --- a/tests/pytests/unit/modules/test_macdefaults.py +++ b/tests/pytests/unit/modules/test_macdefaults.py @@ -150,7 +150,46 @@ def test_load_plist_no_domain(): assert result is None -def test_save_plist(): +def test_save_plist_no_user(): + """ + Test saving a plist + """ + new_plist = {"Crash": "Server"} + expected_result = {"retcode": 0, "stdout": "", "stderr": ""} + + chown_mock = MagicMock() + + tempdir = tempfile.TemporaryDirectory() + tempdir_name = tempdir.name + + tempfile_mock = MagicMock() + tempfile_mock.__enter__.return_value = tempdir_name + + domain = "com.googlecode.iterm2" + tmp_file_name = salt.modules.file.join(tempdir_name, f"{domain}.plist") + + with patch( + "salt.modules.macdefaults._run_defaults_cmd", return_value=expected_result + ) as run_defaults_cmd_mock, patch( + "tempfile.TemporaryDirectory", return_value=tempfile_mock + ), patch( + "plistlib.dump" + ) as plist_mock, patch.dict( + macdefaults.__salt__, + {"file.chown": chown_mock, "file.join": salt.modules.file.join}, + ): + result = macdefaults._save_plist("com.googlecode.iterm2", new_plist) + assert result == expected_result + + plist_mock.assert_called_once_with(new_plist, ANY) # ANY for filehandler + run_defaults_cmd_mock.assert_called_once_with( + f'import "{domain}" "{tmp_file_name}"', + runas=None, + ) + chown_mock.assert_not_called() + + +def test_save_plist_with_user(): """ Test saving a plist """ @@ -187,7 +226,7 @@ def test_save_plist(): result = macdefaults._save_plist("com.googlecode.iterm2", new_plist, user=user) assert result == expected_result - plist_mock.assert_called_once_with(new_plist, ANY) + plist_mock.assert_called_once_with(new_plist, ANY) # ANY for filehandler run_defaults_cmd_mock.assert_called_once_with( f'import "{domain}" "{tmp_file_name}"', runas=user, From 18ca4fdfa9e9c16fb10006f1221254707bece308 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20A=CC=81lvaro?= Date: Wed, 3 Jul 2024 10:15:07 +0100 Subject: [PATCH 16/16] doc(macdefaults): Add ..versionadded:: --- salt/modules/macdefaults.py | 11 +++++++++++ salt/states/macdefaults.py | 4 ++++ 2 files changed, 15 insertions(+) diff --git a/salt/modules/macdefaults.py b/salt/modules/macdefaults.py index 237aa09f183b..864cd1877bb8 100644 --- a/salt/modules/macdefaults.py +++ b/salt/modules/macdefaults.py @@ -74,6 +74,8 @@ def write( The separator to use when splitting the key into a list of keys. If None, the key will not be split (Default). + .. versionadded:: 3008.0 + value The value to write to the given key. Dates should be in the format 'YYYY-MM-DDTHH:MM:SSZ' @@ -108,11 +110,15 @@ def write( If current value is not a dictionary this option will be ignored. This option will be set to True if vtype is dict-add. + .. versionadded:: 3008.0 + array_add Append the value to the array. If current value is not a list this option will be ignored. This option will be set to True if vtype is array-add. + .. versionadded:: 3008.0 + Raises: KeyError: When the key is not found in the domain IndexError: When the key is not a valid array index @@ -195,6 +201,8 @@ def read(domain, key, user=None, key_separator=None): The separator to use when splitting the key into a list of keys. If None, the key will not be split (Default). + .. versionadded:: 3008.0 + user The user to read the defaults from @@ -236,6 +244,8 @@ def delete(domain, key, user=None, key_separator=None): The separator to use when splitting the key into a list of keys. If None, the key will not be split (Default). + .. versionadded:: 3008.0 + user The user to delete the defaults with @@ -303,6 +313,7 @@ def cast_value_to_vtype(value, vtype): Returns: The converted value + .. versionadded:: 3008.0 """ # Boolean if vtype in ("bool", "boolean"): diff --git a/salt/states/macdefaults.py b/salt/states/macdefaults.py index 09a1fde04b7d..088e2ced12e9 100644 --- a/salt/states/macdefaults.py +++ b/salt/states/macdefaults.py @@ -30,6 +30,8 @@ def write(name, domain, value, vtype=None, name_separator=None, user=None): The separator to use when splitting the name into a list of keys. If None, the name will not be split (Default). + .. versionadded:: 3008.0 + domain The name of the domain to write to @@ -91,6 +93,8 @@ def absent(name, domain, user=None, name_separator=None): The separator to use when splitting the name into a list of keys. If None, the name will not be split (Default). + .. versionadded:: 3008.0 + domain The name of the domain to remove from