Add bandit and pyright checks to test code

Use tenacity where it is useful to fix pyright and bandit errors.

Change pylint target to lint target in top level Makefile, since the
target also includes using bandit and pyright.

Signed-off-by: mulhern <[email protected]>

.githooks/pre-commit

@@ -12,7 +12,7 @@ make fmt-ci &&
 	make clippy &&
 	make yamllint &&
 	make tmtlint &&
-	make pylint &&
+	make lint &&
 	make check-typos || exit 1
 
 export PYTHONPATH=$PWD/tests/client-dbus/src

.github/workflows/main.yml

@@ -192,11 +192,14 @@ jobs:
       - name: Install dependencies
         run: >
           dnf install -y
+          bandit
           make
           pylint
           python3-dbus
-      - name: Run pylint
-        run: make -f Makefile pylint
+      - name: Install pyright
+        run: pip install --user pyright
+      - name: Run lint
+        run: make -f Makefile lint
 
   python-based-tests:
     runs-on: ubuntu-22.04

.github/workflows/support.yml

@@ -39,6 +39,7 @@ jobs:
       matrix:
         include:
           - dependencies: >
+              bandit
               pylint
               python3-dbus-client-gen
               python3-dbus-python-client-gen
@@ -65,6 +66,8 @@ jobs:
           make
           ncurses
           ${{ matrix.dependencies }}
+      - name: Install pyright
+        run: pip install --user pyright
       - name: Run test
         run: ${{ matrix.task }}
         working-directory: ${{ matrix.working-directory }}

.github/workflows/weekly.yml

@@ -17,6 +17,7 @@ jobs:
         include:
           # PYTHON CHECKS ON NEXT FEDORA PYTHON AND PYTHON TOOLS VERSION
           - dependencies: >
+              bandit
               pylint
               python3-dbus-client-gen
               python3-dbus-python-client-gen
@@ -49,6 +50,8 @@ jobs:
           ${{ matrix.dependencies }}
       - name: Display Python version
         run: python --version
+      - name: Install pyright
+        run: pip install --user pyright
       - name: ${{ matrix.task }}
         run: ${{ matrix.task }}
         working-directory: ${{ matrix.working-directory }}

Makefile

@@ -430,8 +430,10 @@ clippy: clippy-macros clippy-min clippy-udev-utils clippy-no-ipc clippy-utils cl
 	cargo clippy ${CLIPPY_OPTS}
 
 ## Lint Python parts of the source code
-pylint:
+lint:
 	pylint --disable=invalid-name ./src/bin/utils/stratis-decode-dm
+	bandit ./src/bin/utils/stratis-decode-dm --skip B101
+	pyright ./src/bin/utils/stratis-decode-dm
 
 .PHONY:
 	audit
@@ -477,7 +479,7 @@ pylint:
 	install-udev-binaries
 	install-udev-cfg
 	license
-	pylint
+	lint
 	test
 	test-valgrind
 	test-loop

tests/client-dbus/Makefile

@@ -1,9 +1,17 @@
 UNITTEST_OPTS = --verbose
 
+# Ignore bandit B404 errors. Any import of the subprocess module causes this
+# error. We know what we are doing when we import that module and do not
+# need to be warned.
+BANDIT_SKIP = --skip B404,B603,B311
+
 .PHONY: lint
 lint:
 	pylint src/stratisd_client_dbus --ignore=_introspect.py
 	pylint tests --disable=R0801
+	bandit --recursive ./src ${BANDIT_SKIP}
+	bandit --recursive ./tests ${BANDIT_SKIP},B101
+	pyright
 
 .PHONY: startup-tests
 startup-tests:

tests/client-dbus/src/stratisd_client_dbus/_implementation.py

@@ -16,7 +16,7 @@
 """
 
 # isort: STDLIB
-import xml.etree.ElementTree as ET
+import xml.etree.ElementTree as ET  # nosec B405
 
 # isort: FIRSTPARTY
 from dbus_client_gen import managed_object_class, mo_query_builder
@@ -31,9 +31,9 @@
 )
 from ._introspect import SPECS
 
-_POOL_SPEC = ET.fromstring(SPECS[POOL_INTERFACE])
-_FILESYSTEM_SPEC = ET.fromstring(SPECS[FILESYSTEM_INTERFACE])
-_BLOCKDEV_SPEC = ET.fromstring(SPECS[BLOCKDEV_INTERFACE])
+_POOL_SPEC = ET.fromstring(SPECS[POOL_INTERFACE])  # nosec B314
+_FILESYSTEM_SPEC = ET.fromstring(SPECS[FILESYSTEM_INTERFACE])  # nosec B314
+_BLOCKDEV_SPEC = ET.fromstring(SPECS[BLOCKDEV_INTERFACE])  # nosec B314
 
 pools = mo_query_builder(_POOL_SPEC)
 filesystems = mo_query_builder(_FILESYSTEM_SPEC)
@@ -46,11 +46,15 @@
 
 ObjectManager = make_class(
     "ObjectManager",
-    ET.fromstring(SPECS["org.freedesktop.DBus.ObjectManager"]),
+    ET.fromstring(SPECS["org.freedesktop.DBus.ObjectManager"]),  # nosec B314
     TIME_OUT,
 )
-Report = make_class("Report", ET.fromstring(SPECS[REPORT_INTERFACE]), TIME_OUT)
-Manager = make_class("Manager", ET.fromstring(SPECS[MANAGER_INTERFACE]), TIME_OUT)
+Report = make_class(
+    "Report", ET.fromstring(SPECS[REPORT_INTERFACE]), TIME_OUT  # nosec B314
+)
+Manager = make_class(
+    "Manager", ET.fromstring(SPECS[MANAGER_INTERFACE]), TIME_OUT  # nosec B314
+)
 Filesystem = make_class("Filesystem", _FILESYSTEM_SPEC, TIME_OUT)
 Pool = make_class("Pool", _POOL_SPEC, TIME_OUT)
 Blockdev = make_class("Blockdev", _BLOCKDEV_SPEC, TIME_OUT)

tests/client-dbus/tests/udev/_loopback.py

@@ -105,6 +105,8 @@ def create_devices(self, number):
         :return: list of keys for the devices
         :rtype: list of uuid.UUID
         """
+        assert self.dir is not None
+
         tokens = []
 
         first_creation = self.count == 0
@@ -220,6 +222,8 @@ def destroy_all(self):
         Detach all the devices and delete the file(s) and directory!
         :return: None
         """
+        assert self.dir is not None
+
         self.destroy_devices()
         os.rmdir(self.dir)
         self.dir = None

tests/client-dbus/tests/udev/_utils.py

@@ -31,7 +31,14 @@
 import dbus
 import psutil
 import pyudev
-from tenacity import Retrying, retry_if_exception_type, stop_after_delay, wait_fixed
+from tenacity import (
+    RetryError,
+    Retrying,
+    retry_if_exception_type,
+    stop_after_attempt,
+    stop_after_delay,
+    wait_fixed,
+)
 
 # isort: LOCAL
 from stratisd_client_dbus import (
@@ -235,7 +242,7 @@ def settle():
     :return: None
     """
     time.sleep(2)
-    subprocess.check_call(["udevadm", "settle"])
+    subprocess.check_call(["/usr/bin/udevadm", "settle"])
 
 
 def wait_for_udev_count(expected_num):
@@ -288,25 +295,28 @@ def wait_for_udev(fs_type, expected_paths):
     :raises RuntimeError: if unexpected device nodes are found
     """
     expected_devnodes = frozenset((os.path.realpath(x) for x in expected_paths))
-    found_devnodes = None
 
     context = pyudev.Context()
-    end_time = time.time() + 10.0
 
-    while time.time() < end_time and not expected_devnodes == found_devnodes:
-        found_devnodes = frozenset(
+    def find_devnodes():
+        return frozenset(
             [
                 x.device_node
                 for x in context.list_devices(subsystem="block", ID_FS_TYPE=fs_type)
+                if x.device_node is not None
             ]
         )
-        time.sleep(1)
 
-    if expected_devnodes != found_devnodes:
+    try:
+        for attempt in Retrying(wait=wait_fixed(1), stop=stop_after_delay(10)):
+            with attempt:
+                if expected_devnodes == find_devnodes():
+                    break
+    except RetryError as err:
         raise RuntimeError(
             f'Found unexpected devnodes: expected devnodes: {", ".join(expected_devnodes)} '
-            f'!= found_devnodes: {", ".join(found_devnodes)}'
-        )
+            f'!= found_devnodes: {", ".join(find_devnodes())}'
+        ) from err
 
 
 def processes(name):
@@ -538,32 +548,24 @@ def wait_for_pools(self, expected_num, *, name=None):
         :return: list of pool information found
         :rtype: list of (str * MOPool)
         """
-        (count, limit, dbus_err, found_num, known_pools, start_time) = (
-            0,
-            expected_num + 1,
-            None,
-            None,
-            None,
-            time.time(),
-        )
-        while count < limit and not expected_num == found_num:
-            try:
-                known_pools = get_pools(name=name)
-            except dbus.exceptions.DBusException as err:
-                dbus_err = err
-
-            if known_pools is not None:
-                found_num = len(known_pools)
-
-            time.sleep(3)
-            count += 1
-
-        if found_num is None and dbus_err is not None:
+        try:
+            for attempt in Retrying(
+                retry=(retry_if_exception_type(dbus.exceptions.DBusException)),
+                wait=wait_fixed(3),
+                stop=stop_after_attempt(expected_num + 1),
+                reraise=True,
+            ):
+                with attempt:
+                    known_pools = get_pools(name=name)
+                    if len(known_pools) == expected_num:
+                        break
+        except dbus.exceptions.DBusException as err:
             raise RuntimeError(
-                f"After {time.time() - start_time:.2f} seconds, the only "
-                "response is a D-Bus exception"
-            ) from dbus_err
+                "Failed to obtain any information about pools from the D-Bus"
+            ) from err
 
-        self.assertEqual(found_num, expected_num)
+        time.sleep(3)  # Wait for the D-Bus to add an additional pool
+        known_pools = get_pools(name=name)
+        self.assertEqual(len(known_pools), expected_num)
 
         return known_pools

tests/client-dbus/tests/udev/test_predict.py

@@ -94,7 +94,7 @@ def _call_blockdev_size(dev):
     :rtype: str
     """
     with subprocess.Popen(
-        ["blockdev", "--getsize64", dev],
+        ["/usr/sbin/blockdev", "--getsize64", dev],
         stdout=subprocess.PIPE,
     ) as command:
         outs, _ = command.communicate()

tests/client-dbus/tests/udev/test_revert.py

@@ -42,6 +42,9 @@
     settle,
 )
 
+_MOUNT = "/usr/bin/mount"
+_UMOUNT = "/usr/bin/umount"
+
 
 def write_file(mountdir, filename):
     """
@@ -99,7 +102,7 @@ def test_revert(self):  # pylint: disable=too-many-locals
             settle()
 
             filepath = f"/dev/stratis/{pool_name}/{fs_name}"
-            subprocess.check_call(["mount", filepath, mountdir])
+            subprocess.check_call([_MOUNT, filepath, mountdir])
 
             file1 = "file1.txt"
             write_file(mountdir, file1)
@@ -119,7 +122,7 @@ def test_revert(self):  # pylint: disable=too-many-locals
             write_file(mountdir, file2)
 
             Filesystem.Properties.MergeScheduled.Set(get_object(snap_object_path), True)
-            subprocess.check_call(["umount", mountdir])
+            subprocess.check_call([_UMOUNT, mountdir])
 
             self.assertTrue(os.path.exists(f"/dev/stratis/{pool_name}/{snap_name}"))
 
@@ -131,12 +134,12 @@ def test_revert(self):  # pylint: disable=too-many-locals
 
             settle()
 
-            subprocess.check_call(["mount", filepath, mountdir])
+            subprocess.check_call([_MOUNT, filepath, mountdir])
 
             self.read_file(mountdir, file1)
             self.read_file(mountdir, file2)
 
-            subprocess.check_call(["umount", mountdir])
+            subprocess.check_call([_UMOUNT, mountdir])
 
             # Now stop the pool, which should tear down the devices
             (_, return_code, message) = Manager.Methods.StopPool(
@@ -167,14 +170,14 @@ def test_revert(self):  # pylint: disable=too-many-locals
 
             settle()
 
-            subprocess.check_call(["mount", filepath, mountdir])
+            subprocess.check_call([_MOUNT, filepath, mountdir])
 
             self.read_file(mountdir, file1)
 
             self.assertFalse(os.path.exists(os.path.join(mountdir, file2)))
             self.assertFalse(os.path.exists(f"/dev/stratis/{pool_name}/{snap_name}"))
 
-            subprocess.check_call(["umount", mountdir])
+            subprocess.check_call([_UMOUNT, mountdir])
 
     def test_revert_snapshot_chain(self):  # pylint: disable=too-many-locals
         """
@@ -211,7 +214,7 @@ def test_revert_snapshot_chain(self):  # pylint: disable=too-many-locals
             settle()
 
             filepath = f"/dev/stratis/{pool_name}/{fs_name}"
-            subprocess.check_call(["mount", filepath, mountdir])
+            subprocess.check_call([_MOUNT, filepath, mountdir])
 
             file1 = "file1.txt"
             write_file(mountdir, file1)
@@ -233,7 +236,7 @@ def test_revert_snapshot_chain(self):  # pylint: disable=too-many-locals
             Filesystem.Properties.MergeScheduled.Set(
                 get_object(snap_object_path_1), True
             )
-            subprocess.check_call(["umount", mountdir])
+            subprocess.check_call([_UMOUNT, mountdir])
 
             self.assertTrue(os.path.exists(f"/dev/stratis/{pool_name}/{snap_name_1}"))
 
@@ -285,12 +288,12 @@ def test_revert_snapshot_chain(self):  # pylint: disable=too-many-locals
 
             settle()
 
-            subprocess.check_call(["mount", filepath, mountdir])
+            subprocess.check_call([_MOUNT, filepath, mountdir])
 
             self.read_file(mountdir, file1)
 
             self.assertFalse(os.path.exists(os.path.join(mountdir, file2)))
             self.assertFalse(os.path.exists(f"/dev/stratis/{pool_name}/{snap_name_1}"))
             self.assertTrue(os.path.exists(f"/dev/stratis/{pool_name}/{snap_name_2}"))
 
-            subprocess.check_call(["umount", mountdir])
+            subprocess.check_call([_UMOUNT, mountdir])