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]>
stratis-storage · Jan 7, 2025 · 3541a7d · 3541a7d
1 parent 96dd4b4
commit 3541a7d
Show file tree

Hide file tree

Showing 11 changed files with 65 additions and 26 deletions.
diff --git a/.githooks/pre-commit b/.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

diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml
@@ -192,11 +192,17 @@ jobs:
       - name: Install dependencies
         run: >
           dnf install -y
+          bandit
           make
+          pip
           pylint
           python3-dbus
-      - name: Run pylint
-        run: make -f Makefile pylint
+      - name: Install pyright
+        run: pip install --user pyright
+      - name: Run lint
+        run: >
+          PATH=${PATH}:/github/home/.local/bin
+          make -f Makefile lint
 
   python-based-tests:
     runs-on: ubuntu-22.04

diff --git a/.github/workflows/support.yml b/.github/workflows/support.yml
@@ -39,6 +39,7 @@ jobs:
       matrix:
         include:
           - dependencies: >
+              bandit
               pylint
               python3-dbus-client-gen
               python3-dbus-python-client-gen
@@ -47,7 +48,9 @@ jobs:
               python3-pyudev
               python3-semantic_version
               python3-tenacity
-            task: PYTHONPATH=./src make -f Makefile lint
+            task: >
+              PATH=${PATH}:/github/home/.local/bin
+              PYTHONPATH=./src make -f Makefile lint
             working-directory: ./tests/client-dbus
           - dependencies: black python3-isort
             task: make -f Makefile fmt-ci
@@ -64,7 +67,10 @@ jobs:
           dnf install -y
           make
           ncurses
+          pip
           ${{ matrix.dependencies }}
+      - name: Install pyright
+        run: pip install --user pyright
       - name: Run test
         run: ${{ matrix.task }}
         working-directory: ${{ matrix.working-directory }}
diff --git a/.github/workflows/weekly.yml b/.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
@@ -25,7 +26,9 @@ jobs:
               python3-pyudev
               python3-semantic_version
               python3-tenacity
-            task: PYTHONPATH=./src make -f Makefile lint
+            task: >
+              PATH=${PATH}:/github/home/.local/bin
+              PYTHONPATH=./src make -f Makefile lint
             working-directory: ./tests/client-dbus
           - dependencies: black python3-isort
             task: make -f Makefile fmt-ci
@@ -45,10 +48,13 @@ jobs:
           dnf install -y
           make
           ncurses
+          pip
           python-unversioned-command
           ${{ 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 }}

diff --git a/Makefile b/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

diff --git a/tests/client-dbus/Makefile b/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:

diff --git a/tests/client-dbus/src/stratisd_client_dbus/_implementation.py b/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)
diff --git a/tests/client-dbus/tests/udev/_loopback.py b/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
diff --git a/tests/client-dbus/tests/udev/_utils.py b/tests/client-dbus/tests/udev/_utils.py
@@ -243,7 +243,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):

diff --git a/tests/client-dbus/tests/udev/test_predict.py b/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()

diff --git a/tests/client-dbus/tests/udev/test_revert.py b/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])