Stages/users: use Chroot from osbuild.util.chroot

Use Chroot class from osbuild.util.chroot module, instead of calling
`chroot` directly. The class handles mounting of various paths in the
chroot to make us more usable. This resolves new failure when running
the stage test on F41 results in `mkhomedir_heper` failing with `6`
return code, meaning permissions denied.

Adjust the stage unit tests, because `chroot.Chroot` can't work with
`pathlib.Path`.

Signed-off-by: Tomáš Hozza <thozza@redhat.com>

stages/org.osbuild.users

@@ -1,9 +1,9 @@
 #!/usr/bin/python3
 import os
-import subprocess
 import sys
 
 import osbuild.api
+from osbuild.util.chroot import Chroot
 
 
 def getpwnam(root, name):
@@ -48,7 +48,8 @@ def useradd(
     if expiredate is not None:
         arguments += ["--expiredate", str(expiredate)]
 
-    subprocess.run(["chroot", root, "useradd", *arguments, name], check=True)
+    with Chroot(root) as chroot:
+        chroot.run(["useradd", *arguments, name], check=True)
 
 
 def usermod(root, name, gid=None, groups=None, description=None, home=None, shell=None, password=None, expiredate=None):
@@ -69,7 +70,8 @@ def usermod(root, name, gid=None, groups=None, description=None, home=None, shel
         arguments += ["--expiredate", str(expiredate)]
 
     if arguments:
-        subprocess.run(["chroot", root, "usermod", *arguments, name], check=True)
+        with Chroot(root) as chroot:
+            chroot.run(["usermod", *arguments, name], check=True)
 
 
 def add_ssh_keys(root, user, keys):
@@ -95,7 +97,8 @@ def ensure_homedir(root, name, home):
     if os.path.exists(os.path.join(root, home.lstrip("/"))):
         return
 
-    subprocess.run(["chroot", root, "mkhomedir_helper", name], check=True)
+    with Chroot(root) as chroot:
+        chroot.run(["mkhomedir_helper", name], check=True)
 
 
 def main(tree, options):
@@ -127,7 +130,8 @@ def main(tree, options):
             useradd(tree, name, uid, gid, groups, description, home, shell, password, expiredate)
 
         if force_password_reset:
-            subprocess.run(["chroot", tree, "passwd", "--expire", name], check=True)
+            with Chroot(tree) as chroot:
+                chroot.run(["passwd", "--expire", name], check=True)
 
         # following maintains backwards compatibility for handling a single ssh key
         key = user_options.get("key")   # Public SSH key

stages/test/test_users.py

@@ -7,7 +7,6 @@ import pytest
 from osbuild.testutil import (
     assert_jsonschema_error_contains,
     make_fake_tree,
-    mock_command,
 )
 
 STAGE_NAME = "org.osbuild.users"
@@ -57,95 +56,111 @@ def test_users_happy(mocked_run, tmp_path, stage_module, user_opts, expected_arg
     }
     options["users"]["foo"].update(user_opts)
 
-    stage_module.main(tmp_path, options)
+    stage_module.main(tmp_path.as_posix(), options)
 
-    assert len(mocked_run.call_args_list) == 1
+    # We expect 7 calls to run(): 3 mount + chroot + 3 umount
-    args, kwargs = mocked_run.call_args_list[0]
+    assert len(mocked_run.call_args_list) == 7
-    assert args[0] == ["chroot", tmp_path, "useradd"] + expected_args + ["foo"]
+    args, kwargs = mocked_run.call_args_list[3]  # chroot is the 4th call
+    assert args[0] == ["chroot", tmp_path.as_posix(), "useradd"] + expected_args + ["foo"]
     assert kwargs.get("check")
 
 
+@patch("subprocess.run")
 @pytest.mark.parametrize("user_opts,expected_args", TEST_CASES)
-def test_users_mock_bin(tmp_path, stage_module, user_opts, expected_args):
+def test_users_mock_bin(mocked_run, tmp_path, stage_module, user_opts, expected_args):
-    with mock_command("chroot", "") as mocked_chroot:
+    make_fake_tree(tmp_path, {
-        make_fake_tree(tmp_path, {
+        "/etc/passwd": "",
-            "/etc/passwd": "",
+    })
-        })
 
-        options = {
+    options = {
-            "users": {
+        "users": {
-                "foo": {},
+            "foo": {},
-            }
         }
-        options["users"]["foo"].update(user_opts)
+    }
+    options["users"]["foo"].update(user_opts)
 
-        stage_module.main(tmp_path, options)
+    stage_module.main(tmp_path.as_posix(), options)
-        assert len(mocked_chroot.call_args_list) == 1
+    # We expect 7 calls to run(): 3 mount + chroot + 3 umount
-        assert mocked_chroot.call_args_list[0][2:] == expected_args + ["foo"]
+    assert len(mocked_run.call_args_list) == 7
+    args, kwargs = mocked_run.call_args_list[3]  # chroot is the 4th call
+    assert args[0] == ["chroot", tmp_path.as_posix(), "useradd"] + expected_args + ["foo"]
+    assert kwargs.get("check")
 
 
 # separate test right now as it results in two binaries being called
 # (adduser,passwd) which our parameter tests cannot do yet
 
+@patch("subprocess.run")
+def test_users_with_password_reset_none(mocked_run, tmp_path, stage_module):
+    make_fake_tree(tmp_path, {
+        "/etc/passwd": "",
+    })
 
-def test_users_with_password_reset_none(tmp_path, stage_module):
+    options = {
-    with mock_command("chroot", "") as mocked_chroot:
+        "users": {
-        make_fake_tree(tmp_path, {
+            "foo": {
-            "/etc/passwd": "",
+            },
-        })
-
-        options = {
-            "users": {
-                "foo": {
-                },
-            }
         }
+    }
 
-        stage_module.main(tmp_path, options)
+    stage_module.main(tmp_path.as_posix(), options)
-        assert len(mocked_chroot.call_args_list) == 1
+    # We expect 7 calls to run(): 3 mount + chroot + 3 umount
-        assert mocked_chroot.call_args_list[0][1:] == ["useradd", "foo"]
+    assert len(mocked_run.call_args_list) == 7
+    args, kwargs = mocked_run.call_args_list[3]  # chroot is the 4th call
+    assert args[0] == ["chroot", tmp_path.as_posix(), "useradd", "foo"]
+    assert kwargs.get("check")
 
 # separate test right now as it results in two binaries being called
 # (adduser,passwd) which our parameter tests cannot do yet
 
 
-def test_users_with_password_reset_false(tmp_path, stage_module):
+@patch("subprocess.run")
-    with mock_command("chroot", "") as mocked_chroot:
+def test_users_with_password_reset_false(mocked_run, tmp_path, stage_module):
-        make_fake_tree(tmp_path, {
+    make_fake_tree(tmp_path, {
-            "/etc/passwd": "",
+        "/etc/passwd": "",
-        })
+    })
 
-        options = {
+    options = {
-            "users": {
+        "users": {
-                "foo": {
+            "foo": {
-                    "force_password_reset": False,
+                "force_password_reset": False,
-                },
+            },
-            }
         }
+    }
 
-        stage_module.main(tmp_path, options)
+    stage_module.main(tmp_path.as_posix(), options)
-        assert len(mocked_chroot.call_args_list) == 1
+    # We expect 7 calls to run(): 3 mount + chroot + 3 umount
-        assert mocked_chroot.call_args_list[0][1:] == ["useradd", "foo"]
+    assert len(mocked_run.call_args_list) == 7
+    args, kwargs = mocked_run.call_args_list[3]  # chroot is the 4th call
+    assert args[0] == ["chroot", tmp_path.as_posix(), "useradd", "foo"]
+    assert kwargs.get("check")
 
 # separate test right now as it results in two binaries being called
 # (adduser,passwd) which our parameter tests cannot do yet
 
 
-def test_users_with_password_reset_true(tmp_path, stage_module):
+@patch("subprocess.run")
-    with mock_command("chroot", "") as mocked_chroot:
+def test_users_with_password_reset_true(mocked_run, tmp_path, stage_module):
-        make_fake_tree(tmp_path, {
+    make_fake_tree(tmp_path, {
-            "/etc/passwd": "",
+        "/etc/passwd": "",
-        })
+    })
 
-        options = {
+    options = {
-            "users": {
+        "users": {
-                "foo": {
+            "foo": {
-                    "force_password_reset": True,
+                "force_password_reset": True,
-                },
+            },
-            }
         }
+    }
 
-        stage_module.main(tmp_path, options)
+    stage_module.main(tmp_path.as_posix(), options)
-        assert len(mocked_chroot.call_args_list) == 2
+    # We expect 14 calls to run(): 2x (3 mount + chroot + 3 umount)
-        assert mocked_chroot.call_args_list[0][1:] == ["useradd", "foo"]
+    assert len(mocked_run.call_args_list) == 14
-        assert mocked_chroot.call_args_list[1][1:] == ["passwd", "--expire", "foo"]
+
+    args, kwargs = mocked_run.call_args_list[3]  # chroot is the 4th call
+    assert args[0] == ["chroot", tmp_path.as_posix(), "useradd", "foo"]
+    assert kwargs.get("check")
+
+    args, kwargs = mocked_run.call_args_list[10]  # chroot is the 11th call
+    assert args[0] == ["chroot", tmp_path.as_posix(), "passwd", "--expire", "foo"]
+    assert kwargs.get("check")