From fadbbf3ae6e608683a5f038c6c043e281d3b4152 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Yeray=20Guti=C3=A9rrez=20Cedr=C3=A9s?=
 <yeray.gutierrez@suse.com>
Date: Fri, 12 Jan 2024 11:13:12 +0000
Subject: [PATCH 1/4] Allow kwargs for fileserver roots update (bsc#1218482)

---
 changelog/65819.fixed.md                    |  1 +
 salt/fileserver/roots.py                    | 16 ++++++++++------
 tests/pytests/unit/fileserver/test_roots.py |  9 ++++++++-
 3 files changed, 19 insertions(+), 7 deletions(-)
 create mode 100644 changelog/65819.fixed.md

diff --git a/changelog/65819.fixed.md b/changelog/65819.fixed.md
new file mode 100644
index 0000000000..27c2e9bc70
--- /dev/null
+++ b/changelog/65819.fixed.md
@@ -0,0 +1 @@
+Allow keyword arguments for the fileserver roots backend update function.
diff --git a/salt/fileserver/roots.py b/salt/fileserver/roots.py
index 4880cbab9b..5f168099c1 100644
--- a/salt/fileserver/roots.py
+++ b/salt/fileserver/roots.py
@@ -138,10 +138,16 @@ def serve_file(load, fnd):
     return ret
 
 
-def update():
+def update(**kwargs):
     """
     When we are asked to update (regular interval) lets reap the cache
     """
+    # __pub_user responsible for the runner can be passed but we don't do anything with it
+    if "__pub_user" in kwargs:
+        del kwargs["__pub_user"]
+    if kwargs:
+        raise ValueError("Unexpected keyword arguments received: %s" % kwargs)
+
     try:
         salt.fileserver.reap_fileserver_cache_dir(
             os.path.join(__opts__["cachedir"], "roots", "hash"), find_file
@@ -193,9 +199,7 @@ def update():
         os.makedirs(mtime_map_path_dir)
     with salt.utils.files.fopen(mtime_map_path, "wb") as fp_:
         for file_path, mtime in new_mtime_map.items():
-            fp_.write(
-                salt.utils.stringutils.to_bytes("{}:{}\n".format(file_path, mtime))
-            )
+            fp_.write(salt.utils.stringutils.to_bytes(f"{file_path}:{mtime}\n"))
 
     if __opts__.get("fileserver_events", False):
         # if there is a change, fire an event
@@ -326,11 +330,11 @@ def _file_lists(load, form):
             return []
     list_cache = os.path.join(
         list_cachedir,
-        "{}.p".format(salt.utils.files.safe_filename_leaf(actual_saltenv)),
+        f"{salt.utils.files.safe_filename_leaf(actual_saltenv)}.p",
     )
     w_lock = os.path.join(
         list_cachedir,
-        ".{}.w".format(salt.utils.files.safe_filename_leaf(actual_saltenv)),
+        f".{salt.utils.files.safe_filename_leaf(actual_saltenv)}.w",
     )
     cache_match, refresh_cache, save_cache = salt.fileserver.check_file_list_cache(
         __opts__, form, list_cache, w_lock
diff --git a/tests/pytests/unit/fileserver/test_roots.py b/tests/pytests/unit/fileserver/test_roots.py
index a8a80eea17..46f83c4f1b 100644
--- a/tests/pytests/unit/fileserver/test_roots.py
+++ b/tests/pytests/unit/fileserver/test_roots.py
@@ -4,6 +4,7 @@
 
 import copy
 import pathlib
+import re
 import shutil
 import textwrap
 
@@ -236,7 +237,7 @@ def test_update_mtime_map():
     # between Python releases.
     lines_written = sorted(mtime_map_mock.write_calls())
     expected = sorted(
-        salt.utils.stringutils.to_bytes("{key}:{val}\n".format(key=key, val=val))
+        salt.utils.stringutils.to_bytes(f"{key}:{val}\n")
         for key, val in new_mtime_map.items()
     )
     assert lines_written == expected, lines_written
@@ -277,3 +278,9 @@ def test_update_mtime_map_unicode_error(tmp_path):
         },
         "backend": "roots",
     }
+
+
+def test_update_unexpected_kwargs():
+    with pytest.raises(ValueError) as exc_info:
+        ret = roots.update(foo="bar")
+    assert re.match(r"Unexpected keyword arguments received:", str(exc_info.value))

From b05d5947b793737549b27903a0bc89f9e7696535 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Pablo=20Su=C3=A1rez=20Hern=C3=A1ndez?=
 <psuarezhernandez@suse.com>
Date: Wed, 17 Jan 2024 15:33:06 +0000
Subject: [PATCH 2/4] Prevent exceptions with fileserver.update when called via
 state

---
 changelog/65819.fixed.md   | 2 +-
 salt/fileserver/roots.py   | 8 +-------
 salt/runners/fileserver.py | 6 ++++++
 3 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/changelog/65819.fixed.md b/changelog/65819.fixed.md
index 27c2e9bc70..432f5c791c 100644
--- a/changelog/65819.fixed.md
+++ b/changelog/65819.fixed.md
@@ -1 +1 @@
-Allow keyword arguments for the fileserver roots backend update function.
+Prevent exceptions with fileserver.update when called via state
diff --git a/salt/fileserver/roots.py b/salt/fileserver/roots.py
index 5f168099c1..a02b597c6f 100644
--- a/salt/fileserver/roots.py
+++ b/salt/fileserver/roots.py
@@ -138,16 +138,10 @@ def serve_file(load, fnd):
     return ret
 
 
-def update(**kwargs):
+def update():
     """
     When we are asked to update (regular interval) lets reap the cache
     """
-    # __pub_user responsible for the runner can be passed but we don't do anything with it
-    if "__pub_user" in kwargs:
-        del kwargs["__pub_user"]
-    if kwargs:
-        raise ValueError("Unexpected keyword arguments received: %s" % kwargs)
-
     try:
         salt.fileserver.reap_fileserver_cache_dir(
             os.path.join(__opts__["cachedir"], "roots", "hash"), find_file
diff --git a/salt/runners/fileserver.py b/salt/runners/fileserver.py
index d75d7de0cf..1ed05b68ca 100644
--- a/salt/runners/fileserver.py
+++ b/salt/runners/fileserver.py
@@ -350,6 +350,12 @@ def update(backend=None, **kwargs):
         salt-run fileserver.update backend=git remotes=myrepo,yourrepo
     """
     fileserver = salt.fileserver.Fileserver(__opts__)
+
+    # Remove possible '__pub_user' in kwargs as it is not expected
+    # on "update" function for the different fileserver backends.
+    if "__pub_user" in kwargs:
+        del kwargs["__pub_user"]
+
     fileserver.update(back=backend, **kwargs)
     return True
 

From d90ed34d08fec4327249045c3b939e61d65d145d Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Pablo=20Su=C3=A1rez=20Hern=C3=A1ndez?=
 <psuarezhernandez@suse.com>
Date: Wed, 17 Jan 2024 16:29:12 +0000
Subject: [PATCH 3/4] Fix wrong logic and enhance tests around
 fileserver.update

---
 tests/integration/runners/test_fileserver.py | 40 ++++++++++++++++++--
 1 file changed, 36 insertions(+), 4 deletions(-)

diff --git a/tests/integration/runners/test_fileserver.py b/tests/integration/runners/test_fileserver.py
index ae8ab766aa..62f0da0c4a 100644
--- a/tests/integration/runners/test_fileserver.py
+++ b/tests/integration/runners/test_fileserver.py
@@ -202,15 +202,31 @@ def test_update(self):
         fileserver.update
         """
         ret = self.run_run_plus(fun="fileserver.update")
-        self.assertTrue(ret["return"])
+        self.assertTrue(ret["return"] is True)
 
         # Backend submitted as a string
         ret = self.run_run_plus(fun="fileserver.update", backend="roots")
-        self.assertTrue(ret["return"])
+        self.assertTrue(ret["return"] is True)
 
         # Backend submitted as a list
         ret = self.run_run_plus(fun="fileserver.update", backend=["roots"])
-        self.assertTrue(ret["return"])
+        self.assertTrue(ret["return"] is True)
+
+        # Possible '__pub_user' is removed from kwargs
+        ret = self.run_run_plus(
+            fun="fileserver.update", backend=["roots"], __pub_user="foo"
+        )
+        self.assertTrue(ret["return"] is True)
+
+        # Unknown arguments
+        ret = self.run_run_plus(
+            fun="fileserver.update", backend=["roots"], unknown_arg="foo"
+        )
+        self.assertIn(
+            "Passed invalid arguments: update() got an unexpected keyword argument"
+            " 'unknown_arg'",
+            ret["return"],
+        )
 
         # Other arguments are passed to backend
         def mock_gitfs_update(remotes=None):
@@ -225,7 +241,23 @@ def mock_gitfs_update(remotes=None):
             ret = self.run_run_plus(
                 fun="fileserver.update", backend="gitfs", remotes="myrepo,yourrepo"
             )
-            self.assertTrue(ret["return"])
+            self.assertTrue(ret["return"] is True)
+            mock_backend_func.assert_called_once_with(remotes="myrepo,yourrepo")
+
+        # Possible '__pub_user' arguments are removed from kwargs
+        mock_backend_func = create_autospec(mock_gitfs_update)
+        mock_return_value = {
+            "gitfs.envs": None,  # This is needed to activate the backend
+            "gitfs.update": mock_backend_func,
+        }
+        with patch("salt.loader.fileserver", MagicMock(return_value=mock_return_value)):
+            ret = self.run_run_plus(
+                fun="fileserver.update",
+                backend="gitfs",
+                remotes="myrepo,yourrepo",
+                __pub_user="foo",
+            )
+            self.assertTrue(ret["return"] is True)
             mock_backend_func.assert_called_once_with(remotes="myrepo,yourrepo")
 
         # Unknown arguments are passed to backend

From e818b126d2034ed0088d9905d239031bcccddd08 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Pablo=20Su=C3=A1rez=20Hern=C3=A1ndez?=
 <psuarezhernandez@suse.com>
Date: Wed, 17 Jan 2024 16:29:41 +0000
Subject: [PATCH 4/4] Remove test which is not longer valid

---
 tests/pytests/unit/fileserver/test_roots.py | 7 -------
 1 file changed, 7 deletions(-)

diff --git a/tests/pytests/unit/fileserver/test_roots.py b/tests/pytests/unit/fileserver/test_roots.py
index 46f83c4f1b..96bceb0fd3 100644
--- a/tests/pytests/unit/fileserver/test_roots.py
+++ b/tests/pytests/unit/fileserver/test_roots.py
@@ -4,7 +4,6 @@
 
 import copy
 import pathlib
-import re
 import shutil
 import textwrap
 
@@ -278,9 +277,3 @@ def test_update_mtime_map_unicode_error(tmp_path):
         },
         "backend": "roots",
     }
-
-
-def test_update_unexpected_kwargs():
-    with pytest.raises(ValueError) as exc_info:
-        ret = roots.update(foo="bar")
-    assert re.match(r"Unexpected keyword arguments received:", str(exc_info.value))
