From 9071cd0abbf8db059aa34e6cc21612da244e0684 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tom=C3=A1=C5=A1=20Hozza?= Date: Thu, 21 Nov 2024 15:46:53 +0100 Subject: [PATCH] Stages/users: use Chroot from osbuild.util.chroot MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- stages/org.osbuild.users | 14 ++-- stages/test/test_users.py | 137 +++++++++++++++++++++----------------- 2 files changed, 85 insertions(+), 66 deletions(-) diff --git a/stages/org.osbuild.users b/stages/org.osbuild.users index 08fbf287..40392eeb 100755 --- a/stages/org.osbuild.users +++ b/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 diff --git a/stages/test/test_users.py b/stages/test/test_users.py index 58b681e5..7915696b 100644 --- a/stages/test/test_users.py +++ b/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 - args, kwargs = mocked_run.call_args_list[0] - assert args[0] == ["chroot", tmp_path, "useradd"] + expected_args + ["foo"] + # We expect 7 calls to run(): 3 mount + chroot + 3 umount + 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") +@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): - with mock_command("chroot", "") as mocked_chroot: - make_fake_tree(tmp_path, { - "/etc/passwd": "", - }) +def test_users_mock_bin(mocked_run, tmp_path, stage_module, user_opts, expected_args): + make_fake_tree(tmp_path, { + "/etc/passwd": "", + }) - options = { - "users": { - "foo": {}, - } + options = { + "users": { + "foo": {}, } - options["users"]["foo"].update(user_opts) + } + options["users"]["foo"].update(user_opts) - stage_module.main(tmp_path, options) - assert len(mocked_chroot.call_args_list) == 1 - assert mocked_chroot.call_args_list[0][2:] == expected_args + ["foo"] + stage_module.main(tmp_path.as_posix(), options) + # We expect 7 calls to run(): 3 mount + chroot + 3 umount + 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): - with mock_command("chroot", "") as mocked_chroot: - make_fake_tree(tmp_path, { - "/etc/passwd": "", - }) - - options = { - "users": { - "foo": { - }, - } + options = { + "users": { + "foo": { + }, } + } - stage_module.main(tmp_path, options) - assert len(mocked_chroot.call_args_list) == 1 - assert mocked_chroot.call_args_list[0][1:] == ["useradd", "foo"] + stage_module.main(tmp_path.as_posix(), options) + # We expect 7 calls to run(): 3 mount + chroot + 3 umount + 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): - with mock_command("chroot", "") as mocked_chroot: - make_fake_tree(tmp_path, { - "/etc/passwd": "", - }) +@patch("subprocess.run") +def test_users_with_password_reset_false(mocked_run, tmp_path, stage_module): + make_fake_tree(tmp_path, { + "/etc/passwd": "", + }) - options = { - "users": { - "foo": { - "force_password_reset": False, - }, - } + options = { + "users": { + "foo": { + "force_password_reset": False, + }, } + } - stage_module.main(tmp_path, options) - assert len(mocked_chroot.call_args_list) == 1 - assert mocked_chroot.call_args_list[0][1:] == ["useradd", "foo"] + stage_module.main(tmp_path.as_posix(), options) + # We expect 7 calls to run(): 3 mount + chroot + 3 umount + 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): - with mock_command("chroot", "") as mocked_chroot: - make_fake_tree(tmp_path, { - "/etc/passwd": "", - }) +@patch("subprocess.run") +def test_users_with_password_reset_true(mocked_run, tmp_path, stage_module): + make_fake_tree(tmp_path, { + "/etc/passwd": "", + }) - options = { - "users": { - "foo": { - "force_password_reset": True, - }, - } + options = { + "users": { + "foo": { + "force_password_reset": True, + }, } + } - stage_module.main(tmp_path, options) - assert len(mocked_chroot.call_args_list) == 2 - assert mocked_chroot.call_args_list[0][1:] == ["useradd", "foo"] - assert mocked_chroot.call_args_list[1][1:] == ["passwd", "--expire", "foo"] + stage_module.main(tmp_path.as_posix(), options) + # We expect 14 calls to run(): 2x (3 mount + chroot + 3 umount) + assert len(mocked_run.call_args_list) == 14 + + 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")