From 52ace3bc346b7740c3a735b779187dd81332436f Mon Sep 17 00:00:00 2001 From: Shane Lee Date: Tue, 19 Mar 2024 12:56:07 -0600 Subject: [PATCH] Fix issue with win_user.add win_useradd.add now allows you to add new users that have only integers in the username. It also adds tests for the win_useradd module. --- changelog/53363.fixed.md | 2 + salt/modules/win_useradd.py | 125 ++++--- .../pytests/unit/modules/test_win_useradd.py | 344 ++++++++++++++++++ 3 files changed, 419 insertions(+), 52 deletions(-) create mode 100644 changelog/53363.fixed.md create mode 100644 tests/pytests/unit/modules/test_win_useradd.py diff --git a/changelog/53363.fixed.md b/changelog/53363.fixed.md new file mode 100644 index 00000000000..9ab50a6424c --- /dev/null +++ b/changelog/53363.fixed.md @@ -0,0 +1,2 @@ +``user.add`` on Windows now allows you to add user names that contain all +numeric characters diff --git a/salt/modules/win_useradd.py b/salt/modules/win_useradd.py index a9e9b2629b6..7fe48727d08 100644 --- a/salt/modules/win_useradd.py +++ b/salt/modules/win_useradd.py @@ -22,6 +22,7 @@ Module for managing Windows Users. This currently only works with local user accounts, not domain accounts """ +import ctypes import logging import shlex import time @@ -30,6 +31,7 @@ from datetime import datetime import salt.utils.args import salt.utils.dateutils import salt.utils.platform +import salt.utils.win_reg import salt.utils.winapi from salt.exceptions import CommandExecutionError @@ -82,7 +84,7 @@ def add( Add a user to the minion. Args: - name (str): User name + name (str): The username for the new account password (str, optional): User's password in plain text. @@ -106,7 +108,7 @@ def add( logs on. Returns: - bool: True if successful. False is unsuccessful. + bool: ``True`` if successful, otherwise ``False``. CLI Example: @@ -116,10 +118,10 @@ def add( """ user_info = {} if name: - user_info["name"] = name + user_info["name"] = str(name) else: return False - user_info["password"] = password + user_info["password"] = str(password) user_info["priv"] = win32netcon.USER_PRIV_USER user_info["home_dir"] = home user_info["comment"] = description @@ -160,13 +162,13 @@ def update( ): # pylint: disable=anomalous-backslash-in-string """ - Updates settings for the windows user. Name is the only required parameter. + Updates settings for the Windows user. Name is the only required parameter. Settings will only be changed if the parameter is passed a value. .. versionadded:: 2015.8.0 Args: - name (str): The user name to update. + name (str): The username to update. password (str, optional): New user password in plain text. @@ -206,7 +208,7 @@ def update( changing the password. False allows the user to change the password. Returns: - bool: True if successful. False is unsuccessful. + bool: ``True`` if successful, otherwise ``False``. CLI Example: @@ -219,7 +221,7 @@ def update( # Make sure the user exists # Return an object containing current settings for the user try: - user_info = win32net.NetUserGetInfo(None, name, 4) + user_info = win32net.NetUserGetInfo(None, str(name), 4) except win32net.error as exc: log.error("Failed to update user %s", name) log.error("nbr: %s", exc.winerror) @@ -230,7 +232,7 @@ def update( # Check parameters to update # Update the user object with new settings if password: - user_info["password"] = password + user_info["password"] = str(password) if home: user_info["home_dir"] = home if homedrive: @@ -251,7 +253,7 @@ def update( dt_obj = salt.utils.dateutils.date_cast(expiration_date) except (ValueError, RuntimeError): return f"Invalid Date/Time Format: {expiration_date}" - user_info["acct_expires"] = time.mktime(dt_obj.timetuple()) + user_info["acct_expires"] = int(dt_obj.timestamp()) if expired is not None: if expired: user_info["password_expired"] = 1 @@ -263,6 +265,7 @@ def update( else: user_info["flags"] &= ~win32netcon.UF_ACCOUNTDISABLE if unlock_account is not None: + # We can only unlock with this flag... we can't unlock if unlock_account: user_info["flags"] &= ~win32netcon.UF_LOCKOUT if password_never_expires is not None: @@ -278,7 +281,7 @@ def update( # Apply new settings try: - win32net.NetUserSetInfo(None, name, 4, user_info) + win32net.NetUserSetInfo(None, str(name), 4, user_info) except win32net.error as exc: log.error("Failed to update user %s", name) log.error("nbr: %s", exc.winerror) @@ -305,7 +308,7 @@ def delete(name, purge=False, force=False): user out and delete user. Returns: - bool: True if successful, otherwise False + bool: ``True`` if successful, otherwise ``False``. CLI Example: @@ -315,7 +318,7 @@ def delete(name, purge=False, force=False): """ # Check if the user exists try: - user_info = win32net.NetUserGetInfo(None, name, 4) + user_info = win32net.NetUserGetInfo(None, str(name), 4) except win32net.error as exc: log.error("User not found: %s", name) log.error("nbr: %s", exc.winerror) @@ -382,7 +385,7 @@ def delete(name, purge=False, force=False): # And finally remove the user account try: - win32net.NetUserDel(None, name) + win32net.NetUserDel(None, str(name)) except win32net.error as exc: log.error("Failed to delete user %s", name) log.error("nbr: %s", exc.winerror) @@ -398,7 +401,7 @@ def getUserSid(username): Get the Security ID for the user Args: - username (str): The user name for which to look up the SID + username (str): The username for which to look up the SID Returns: str: The user SID @@ -424,12 +427,12 @@ def setpassword(name, password): Set the user's password Args: - name (str): The user name for which to set the password + name (str): The username for which to set the password password (str): The new password Returns: - bool: True if successful, otherwise False + bool: ``True`` if successful, otherwise ``False``. CLI Example: @@ -445,12 +448,12 @@ def addgroup(name, group): Add user to a group Args: - name (str): The user name to add to the group + name (str): The username to add to the group group (str): The name of the group to which to add the user Returns: - bool: True if successful, otherwise False + bool: ``True`` if successful, otherwise ``False``. CLI Example: @@ -458,7 +461,7 @@ def addgroup(name, group): salt '*' user.addgroup jsnuffy 'Power Users' """ - name = shlex.quote(name) + name = shlex.quote(str(name)) group = shlex.quote(group).lstrip("'").rstrip("'") user = info(name) @@ -478,12 +481,12 @@ def removegroup(name, group): Remove user from a group Args: - name (str): The user name to remove from the group + name (str): The username to remove from the group group (str): The name of the group from which to remove the user Returns: - bool: True if successful, otherwise False + bool: ``True`` if successful, otherwise ``False``. CLI Example: @@ -491,7 +494,7 @@ def removegroup(name, group): salt '*' user.removegroup jsnuffy 'Power Users' """ - name = shlex.quote(name) + name = shlex.quote(str(name)) group = shlex.quote(group).lstrip("'").rstrip("'") user = info(name) @@ -519,7 +522,7 @@ def chhome(name, home, **kwargs): home (str): The new location of the home directory Returns: - bool: True if successful, otherwise False + bool: ``True`` if successful, otherwise ``False``. CLI Example: @@ -562,7 +565,7 @@ def chprofile(name, profile): profile (str): The new location of the profile Returns: - bool: True if successful, otherwise False + bool: ``True`` if successful, otherwise ``False``. CLI Example: @@ -578,12 +581,12 @@ def chfullname(name, fullname): Change the full name of the user Args: - name (str): The user name for which to change the full name + name (str): The username for which to change the full name fullname (str): The new value for the full name Returns: - bool: True if successful, otherwise False + bool: ``True`` if successful, otherwise ``False``. CLI Example: @@ -600,7 +603,7 @@ def chgroups(name, groups, append=True): member of only the specified groups Args: - name (str): The user name for which to change groups + name (str): The username for which to change groups groups (str, list): A single group or a list of groups to assign to the user. For multiple groups this can be a comma delimited string or a @@ -611,7 +614,7 @@ def chgroups(name, groups, append=True): only. Default is True. Returns: - bool: True if successful, otherwise False + bool: ``True`` if successful, otherwise ``False``. CLI Example: @@ -623,21 +626,31 @@ def chgroups(name, groups, append=True): groups = groups.split(",") groups = [x.strip(" *") for x in groups] - ugrps = set(list_groups(name)) - if ugrps == set(groups): - return True + current_groups = set(list_groups(name)) + expected_groups = set() - name = shlex.quote(name) + name = shlex.quote(str(name)) if not append: - for group in ugrps: + # We don't want to append to the list, remove groups not in the new set + # of groups + for group in current_groups: group = shlex.quote(group).lstrip("'").rstrip("'") if group not in groups: cmd = f'net localgroup "{group}" {name} /delete' __salt__["cmd.run_all"](cmd, python_shell=True) + else: + expected_groups.add(group) + else: + # We're appending to the current list of groups. If they already match + # then bail + if current_groups == set(groups): + return True + else: + expected_groups = current_groups.union(set(groups)) for group in groups: - if group in ugrps: + if group in current_groups: continue group = shlex.quote(group).lstrip("'").rstrip("'") cmd = f'net localgroup "{group}" {name} /add' @@ -646,8 +659,9 @@ def chgroups(name, groups, append=True): log.error(out["stdout"]) return False - agrps = set(list_groups(name)) - return len(ugrps - agrps) == 0 + new_groups = set(list_groups(name)) + + return len(expected_groups - new_groups) == 0 def info(name): @@ -677,6 +691,7 @@ def info(name): - last_logon - account_disabled - account_locked + - expiration_date - password_never_expires - disallow_change_password - gid @@ -690,14 +705,14 @@ def info(name): ret = {} items = {} try: - items = win32net.NetUserGetInfo(None, name, 4) + items = win32net.NetUserGetInfo(None, str(name), 4) except win32net.error: pass if items: groups = [] try: - groups = win32net.NetUserGetLocalGroups(None, name) + groups = win32net.NetUserGetLocalGroups(None, str(name)) except win32net.error: pass @@ -722,9 +737,15 @@ def info(name): ret["last_logon"] = datetime.fromtimestamp(items["last_logon"]).strftime( "%Y-%m-%d %H:%M:%S" ) - ret["expiration_date"] = datetime.fromtimestamp(items["acct_expires"]).strftime( - "%Y-%m-%d %H:%M:%S" - ) + + # If the value is -1 or 0xFFFFFFFF, it is set to never expire + if items["acct_expires"] == ctypes.c_ulong(win32netcon.TIMEQ_FOREVER).value: + ret["expiration_date"] = "Never" + else: + ret["expiration_date"] = datetime.fromtimestamp( + items["acct_expires"] + ).strftime("%Y-%m-%d %H:%M:%S") + ret["expired"] = items["password_expired"] == 1 if not ret["profile"]: ret["profile"] = _get_userprofile_from_registry(name, ret["uid"]) @@ -765,17 +786,17 @@ def _get_userprofile_from_registry(user, sid): registry Args: - user (str): The user name, used in debug message + user (str): The username, used in debug message sid (str): The sid to lookup in the registry Returns: str: Profile directory """ - profile_dir = __utils__["reg.read_value"]( - "HKEY_LOCAL_MACHINE", - f"SOFTWARE\\Microsoft\\Windows NT\\CurrentVersion\\ProfileList\\{sid}", - "ProfileImagePath", + profile_dir = salt.utils.win_reg.read_value( + hive="HKEY_LOCAL_MACHINE", + key=f"SOFTWARE\\Microsoft\\Windows NT\\CurrentVersion\\ProfileList\\{sid}", + vname="ProfileImagePath", )["vdata"] log.debug('user %s with sid=%s profile is located at "%s"', user, sid, profile_dir) return profile_dir @@ -786,7 +807,7 @@ def list_groups(name): Return a list of groups the named user belongs to Args: - name (str): The user name for which to list groups + name (str): The username for which to list groups Returns: list: A list of groups to which the user belongs @@ -829,9 +850,9 @@ def getent(refresh=False): return __context__["user.getent"] ret = [] - for user in __salt__["user.list_users"](): + for user in list_users(): stuff = {} - user_info = __salt__["user.info"](user) + user_info = info(user) stuff["gid"] = "" stuff["groups"] = user_info["groups"] @@ -885,12 +906,12 @@ def rename(name, new_name): Change the username for a named user Args: - name (str): The user name to change + name (str): The username to change new_name (str): The new name for the current user Returns: - bool: True if successful, otherwise False + bool: ``True`` if successful, otherwise ``False``. CLI Example: diff --git a/tests/pytests/unit/modules/test_win_useradd.py b/tests/pytests/unit/modules/test_win_useradd.py new file mode 100644 index 00000000000..6084c5bc566 --- /dev/null +++ b/tests/pytests/unit/modules/test_win_useradd.py @@ -0,0 +1,344 @@ +import pytest +from saltfactories.utils import random_string + +import salt.modules.cmdmod +import salt.modules.win_useradd as user +import salt.utils.data +from salt.exceptions import CommandExecutionError + +pytestmark = [ + pytest.mark.destructive_test, + pytest.mark.skip_unless_on_windows, + pytest.mark.windows_whitelisted, +] + + +@pytest.fixture +def configure_loader_modules(): + return {user: {"__salt__": {"cmd.run_all": salt.modules.cmdmod.run_all}}} + + +@pytest.fixture +def username_str(): + _username = random_string("test-account-", uppercase=False) + try: + yield _username + finally: + try: + user.delete(_username, purge=True, force=True) + except Exception: # pylint: disable=broad-except + # The point here is just system cleanup. It can fail if no account was created + pass + + +@pytest.fixture +def username_int(): + _username = random_string("", uppercase=False, lowercase=False, digits=True) + try: + yield _username + finally: + try: + user.delete(_username, purge=True, force=True) + except Exception: # pylint: disable=broad-except + # The point here is just system cleanup. It can fail if no account was created + pass + + +@pytest.fixture +def account_str(username_str): + with pytest.helpers.create_account(username=username_str) as account: + user.addgroup(account.username, "Users") + yield account + + +@pytest.fixture +def account_int(username_int): + with pytest.helpers.create_account(username=username_int) as account: + user.addgroup(account.username, "Users") + yield account + + +def test_add_str(username_str): + ret = user.add(name=username_str) + assert ret is True + assert username_str in user.list_users() + + +def test_add_int(username_int): + ret = user.add(name=username_int) + assert ret is True + assert username_int in user.list_users() + + +def test_addgroup_str(account_str): + ret = user.addgroup(account_str.username, "Backup Operators") + assert ret is True + ret = user.info(account_str.username) + assert "Backup Operators" in ret["groups"] + + +def test_addgroup_int(account_int): + ret = user.addgroup(account_int.username, "Backup Operators") + assert ret is True + ret = user.info(account_int.username) + assert "Backup Operators" in ret["groups"] + + +def test_chfullname_str(account_str): + ret = user.chfullname(account_str.username, "New Full Name") + assert ret is True + ret = user.info(account_str.username) + assert ret["fullname"] == "New Full Name" + + +def test_chfullname_int(account_int): + ret = user.chfullname(account_int.username, "New Full Name") + assert ret is True + ret = user.info(account_int.username) + assert ret["fullname"] == "New Full Name" + + +def test_chgroups_single_str(account_str): + groups = ["Backup Operators"] + ret = user.chgroups(account_str.username, groups=groups) + assert ret is True + ret = user.info(account_str.username) + groups.append("Users") + assert salt.utils.data.compare_lists(ret["groups"], groups) == {} + + +def test_chgroups_single_int(account_int): + groups = ["Backup Operators"] + ret = user.chgroups(account_int.username, groups=groups) + assert ret is True + ret = user.info(account_int.username) + groups.append("Users") + assert salt.utils.data.compare_lists(ret["groups"], groups) == {} + + +def test_chgroups_list_str(account_str): + groups = ["Backup Operators", "Guests"] + ret = user.chgroups(account_str.username, groups=groups) + assert ret is True + ret = user.info(account_str.username) + groups.append("Users") + assert salt.utils.data.compare_lists(ret["groups"], groups) == {} + + +def test_chgroups_list_int(account_int): + groups = ["Backup Operators", "Guests"] + ret = user.chgroups(account_int.username, groups=groups) + assert ret is True + ret = user.info(account_int.username) + groups.append("Users") + assert salt.utils.data.compare_lists(ret["groups"], groups) == {} + + +def test_chgroups_list_append_false_str(account_str): + groups = ["Backup Operators", "Guests"] + ret = user.chgroups(account_str.username, groups=groups, append=False) + assert ret is True + ret = user.info(account_str.username) + assert salt.utils.data.compare_lists(ret["groups"], groups) == {} + + +def test_chgroups_list_append_false_int(account_int): + groups = ["Backup Operators", "Guests"] + ret = user.chgroups(account_int.username, groups=groups, append=False) + assert ret is True + ret = user.info(account_int.username) + assert salt.utils.data.compare_lists(ret["groups"], groups) == {} + + +def test_chhome_str(account_str): + home = r"C:\spongebob\squarepants" + ret = user.chhome(name=account_str.username, home=home) + assert ret is True + ret = user.info(name=account_str.username) + assert ret["home"] == home + + +def test_chhome_int(account_int): + home = r"C:\spongebob\squarepants" + ret = user.chhome(name=account_int.username, home=home) + assert ret is True + ret = user.info(name=account_int.username) + assert ret["home"] == home + + +def test_chprofile_str(account_str): + profile = r"C:\spongebob\squarepants" + ret = user.chprofile(name=account_str.username, profile=profile) + assert ret is True + ret = user.info(name=account_str.username) + assert ret["profile"] == profile + + +def test_chprofile_int(account_int): + profile = r"C:\spongebob\squarepants" + ret = user.chprofile(name=account_int.username, profile=profile) + assert ret is True + ret = user.info(name=account_int.username) + assert ret["profile"] == profile + + +def test_delete_str(account_str): + ret = user.delete(name=account_str.username) + assert ret is True + assert user.info(name=account_str.username) == {} + + +def test_delete_int(account_int): + ret = user.delete(name=account_int.username) + assert ret is True + assert user.info(name=account_int.username) == {} + + +def test_getUserSig_str(account_str): + ret = user.getUserSid(account_str.username) + assert ret.startswith("S-1-5") + + +def test_getUserSig_int(account_int): + ret = user.getUserSid(account_int.username) + assert ret.startswith("S-1-5") + + +def test_info_str(account_str): + ret = user.info(account_str.username) + assert ret["name"] == account_str.username + assert ret["uid"].startswith("S-1-5") + + +def test_info_int(account_int): + ret = user.info(account_int.username) + assert ret["name"] == account_int.username + assert ret["uid"].startswith("S-1-5") + + +def test_list_groups_str(account_str): + ret = user.list_groups(account_str.username) + assert ret == ["Users"] + + +def test_list_groups_int(account_int): + ret = user.list_groups(account_int.username) + assert ret == ["Users"] + + +def test_list_users(): + ret = user.list_users() + assert "Administrator" in ret + + +def test_removegroup_str(account_str): + ret = user.removegroup(account_str.username, "Users") + assert ret is True + ret = user.info(account_str.username) + assert ret["groups"] == [] + + +def test_removegroup_int(account_int): + ret = user.removegroup(account_int.username, "Users") + assert ret is True + ret = user.info(account_int.username) + assert ret["groups"] == [] + + +def test_rename_str(account_str): + new_name = random_string("test-account-", uppercase=False) + ret = user.rename(name=account_str.username, new_name=new_name) + assert ret is True + assert new_name in user.list_users() + # Let's set it back so that it gets cleaned up... + ret = user.rename(name=new_name, new_name=account_str.username) + assert ret is True + + +def test_rename_str_missing(account_str): + missing = random_string("test-account-", uppercase=False) + with pytest.raises(CommandExecutionError): + user.rename(name=missing, new_name="spongebob") + + +def test_rename_str_existing(account_str): + new_existing = random_string("test-account-", uppercase=False) + ret = user.add(name=new_existing) + assert ret is True + with pytest.raises(CommandExecutionError): + user.rename(name=account_str.username, new_name=new_existing) + # We need to clean this up because it wasn't created in a fixture + ret = user.delete(name=new_existing, purge=True, force=True) + assert ret is True + assert new_existing not in user.list_users() + + +def test_rename_int(account_int): + new_name = random_string("", uppercase=False, lowercase=False, digits=True) + ret = user.rename(name=account_int.username, new_name=new_name) + assert ret is True + assert new_name in user.list_users() + # Let's set it back so that it gets cleaned up... + ret = user.rename(name=new_name, new_name=account_int.username) + assert ret is True + + +def test_rename_int_missing(account_int): + missing = random_string("", uppercase=False, lowercase=False, digits=True) + with pytest.raises(CommandExecutionError): + user.rename(name=missing, new_name="spongebob") + + +def test_rename_int_existing(account_int): + new_existing = random_string("", uppercase=False, lowercase=False, digits=True) + ret = user.add(name=new_existing) + assert ret is True + with pytest.raises(CommandExecutionError): + user.rename(name=account_int.username, new_name=new_existing) + # We need to clean this up because it wasn't created in a fixture + ret = user.delete(name=new_existing, purge=True, force=True) + assert ret is True + assert new_existing not in user.list_users() + + +def test_setpassword_str(account_str): + ret = user.setpassword(account_str.username, password="Sup3rS3cret") + # We have no way of verifying the password was changed on Windows, so the + # best we can do is check that the command completed successfully + assert ret is True + + +def test_setpassword_int(account_int): + ret = user.setpassword(account_int.username, password="Sup3rS3cret") + # We have no way of verifying the password was changed on Windows, so the + # best we can do is check that the command completed successfully + assert ret is True + + +@pytest.mark.parametrize( + "value_name, new_value, info_field, expected", + [ + ("description", "New description", "", None), + ("homedrive", "H:", "", None), + ("logonscript", "\\\\server\\script.cmd", "", None), + ("expiration_date", "3/19/2024", "", "2024-03-19 00:00:00"), + ("expiration_date", "Never", "", None), + ("expired", True, "", None), + ("expired", False, "", None), + ("account_disabled", True, "", None), + ("account_disabled", False, "", None), + ("unlock_account", True, "account_locked", False), + ("password_never_expires", True, "", None), + ("password_never_expires", False, "", None), + ("disallow_change_password", True, "", None), + ("disallow_change_password", False, "", None), + ], +) +def test_update_str(value_name, new_value, info_field, expected, account_str): + setting = {value_name: new_value} + ret = user.update(account_str.username, **setting) + assert ret is True + ret = user.info(account_str.username) + info_field = info_field if info_field else value_name + expected = expected if expected is not None else new_value + assert ret[info_field] == expected