Repository: saltstack/salt
Pull request: Fixed user.present which was breaking when updating workphone,homephone, fullname and "other" fields in case int was passed instead of string
### What does this PR do?
Fixed user.present which was breaking when updating workphone,homephone, fullname and "other" fields in case int was passed instead of string
### What issues does this PR fix or reference?
Fixes #53961

### Previous Behavior
user.present (for updating the user) was breaking when any of the above-mentioned fields were passed in as an integer (eg 123) and not string (eg: '123') 

### New Behavior
It does not break anymore

### Merge requirements satisfied?
- [x] Docs - Not needed
- [x] Changelog - No sure if its needed for this change.
- [x] Tests written/updated - No.

### Commits signed with GPG?
No


Problem statement:
user.present updating workphone, homephone fails
A state definition like this:

foo:
  user:
    - present
    - workphone: 123

fails:
  comment: These values could not be changed: {u'workphone': 123}

I think the problem is that "123" is (internally) of type int instead of string and the final check if the update worked then fails.

I changed ./salt/states/user.py and it works for me.

In version 2018.3.4:

`    if workphone is not None:`
`-        workphone = sdecode(workphone)`
`+        workphone = sdecode(str(workphone))`
`    if homephone is not None:`
`-        homephone = sdecode(homephone)`
`+        homephone = sdecode(str(homephone))`

In version 2019.2.0:

`     if roomnumber is not None:`
`         roomnumber = salt.utils.data.decode(roomnumber)`
`     if workphone is not None:`
`-        workphone = salt.utils.data.decode(workphone)`
`+        workphone = salt.utils.data.decode(str(workphone))`
`     if homephone is not None:`
`-        homephone = salt.utils.data.decode(homephone)`
`+        homephone = salt.utils.data.decode(str(homephone))`
`     if other is not None:`
`         other = salt.utils.data.decode(other)`

roomnumber might also be affected, but didn't try.

Regards,
Alexander

Hints from issue discussion:
@sotho 

Does the state work without these changes if you modify your state to this?

```
foo:
  user:
    - present
    - workphone: '123'
```

I still think we might want to make the change to always cast these values to a string but I am just wondering if this is a reasonable work around for now.

Ultimately we need to have better input validation for both states and modules.
Yes, adding quotes seems to help indeed.
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

If this issue is closed prematurely, please leave a comment and we will gladly reopen the issue.

Thank you for updating this issue. It is no longer marked as stale.

FYI, I've made the modification suggested by @sotho on the following fields:
- fullname
- workphone
- homephone
- roomnumber
- other

Commit message: Removed some comments

Patch to review:
diff --git a/changelog/53961.fixed b/changelog/53961.fixed
new file mode 100644
index 000000000000..a5235eada7f0
--- /dev/null
+++ b/changelog/53961.fixed
@@ -0,0 +1 @@
+Fixed user.present which was breaking when updating workphone,homephone, fullname and "other" fields in case int was passed instead of string
\ No newline at end of file
diff --git a/salt/states/user.py b/salt/states/user.py
index be99cf755a30..294145110632 100644
--- a/salt/states/user.py
+++ b/salt/states/user.py
@@ -496,15 +496,15 @@ def present(
             password = __salt__["shadow.gen_password"](password)
 
     if fullname is not None:
-        fullname = salt.utils.data.decode(fullname)
+        fullname = salt.utils.data.decode(str(fullname))
     if roomnumber is not None:
-        roomnumber = salt.utils.data.decode(roomnumber)
+        roomnumber = salt.utils.data.decode(str(roomnumber))
     if workphone is not None:
-        workphone = salt.utils.data.decode(workphone)
+        workphone = salt.utils.data.decode(str(workphone))
     if homephone is not None:
-        homephone = salt.utils.data.decode(homephone)
+        homephone = salt.utils.data.decode(str(homephone))
     if other is not None:
-        other = salt.utils.data.decode(other)
+        other = salt.utils.data.decode(str(other))
 
     # createhome not supported on Windows
     if __grains__["kernel"] == "Windows":
diff --git a/tests/pytests/unit/states/test_user.py b/tests/pytests/unit/states/test_user.py
index 32bb8a2485eb..eca86c972191 100644
--- a/tests/pytests/unit/states/test_user.py
+++ b/tests/pytests/unit/states/test_user.py
@@ -13,7 +13,18 @@
 
 @pytest.fixture
 def configure_loader_modules():
-    return {user: {}}
+    shadow_info = MagicMock(
+        return_value={"min": 2, "max": 88888, "inact": 77, "warn": 14, "passwd": ""}
+    )
+    shadow_hash = MagicMock(return_value="abcd")
+    dunder_salt = {
+        "shadow.info": shadow_info,
+        "shadow.default_hash": shadow_hash,
+        "file.group_to_gid": MagicMock(side_effect=["foo"]),
+        "file.gid_to_group": MagicMock(side_effect=[5000, 5000]),
+        # "user.chhomephone": MagicMock(return_value=True),
+    }
+    return {user: {"__salt__": dunder_salt}}
 
 
 def test_present():
@@ -264,3 +275,41 @@ def mock_exists(*args):
             "warndays": 7,
             "inactdays": 0,
         }
+
+
+def test_gecos_field_changes_in_user_present():
+    """
+    Test if the gecos fields change in salt.states.user.present
+    """
+    mock_info = MagicMock(
+        side_effect=[
+            {
+                "uid": 5000,
+                "gid": 5000,
+                "groups": ["foo"],
+                "home": "/home/foo",
+                "fullname": "Foo Bar",
+                "homephone": "667788",
+            },
+            {
+                "uid": 5000,
+                "gid": 5000,
+                "groups": ["foo"],
+                "home": "/home/foo",
+                "fullname": "Bar Bar",
+                "homephone": "44566",
+            },
+        ]
+    )
+    mock_changes = MagicMock(side_effect=[{"homephone": "667788"}, None])
+    dunder_salt = {
+        "user.info": mock_info,
+        "user.chhomephone": MagicMock(return_value=True),
+    }
+    with patch.dict(user.__grains__, {"kernel": "Linux"}), patch.dict(
+        user.__salt__, dunder_salt
+    ), patch.dict(user.__opts__, {"test": False}), patch.object(
+        user, "_changes", mock_changes
+    ):
+        res = user.present("Foo", homephone=44566, fullname="Bar Bar")
+        assert res["changes"] == {"homephone": "44566", "fullname": "Bar Bar"}


Code review comments:
diff_hunk: @@ -13,7 +13,18 @@
 
 @pytest.fixture
 def configure_loader_modules():
-    return {user: {}}
+    shadow_info = MagicMock(
+        return_value={"min": 2, "max": 88888, "inact": 77, "warn": 14, "passwd": ""}
+    )
+    shadow_hash = MagicMock(return_value="abcd")
+    dunder_salt = {
+        "shadow.info": shadow_info,
+        "shadow.default_hash": shadow_hash,
+        "file.group_to_gid": MagicMock(side_effect=["foo"]),
+        "file.gid_to_group": MagicMock(side_effect=[5000, 5000]),
+        # "user.chhomephone": MagicMock(return_value=True),
+    }
+    return {user: {"__salt__": dunder_salt}}
path: tests/pytests/unit/states/test_user.py
text: @user1:
```suggestion
    return {user: {}}
```