From 729c954f3d315d8c6de0fcdc8373375d904b510a Mon Sep 17 00:00:00 2001
From: Jana Librova <jcupova@redhat.com>
Date: Nov 13 2024 19:12:18 +0000
Subject: [PATCH 1/2] Fix for reading config files when contains UTF-8 chars


Fixes: https://pagure.io/koji/issue/4191

---

diff --git a/koji/__init__.py b/koji/__init__.py
index 7a8111e..39ee74c 100644
--- a/koji/__init__.py
+++ b/koji/__init__.py
@@ -25,6 +25,7 @@
 from __future__ import absolute_import, division
 
 import base64
+import codecs
 import datetime
 import errno
 import hashlib
@@ -2339,7 +2340,11 @@ def read_config_files(config_files, raw=False):
         elif strict:
             raise ConfigurationError("Config file %s can't be opened."
                                      % config_file)
-    config.read(cfgs)
+    if six.PY3:
+        config.read(cfgs, encoding="utf8")
+    else:
+        for cfg in cfgs:
+            config.readfp(codecs.open(cfg, 'r', 'utf8'))
     return config
 
 
diff --git a/tests/test_lib/test_utils.py b/tests/test_lib/test_utils.py
index 691dcb4..42dd145 100644
--- a/tests/test_lib/test_utils.py
+++ b/tests/test_lib/test_utils.py
@@ -272,7 +272,6 @@ class ConfigFileTestCase(unittest.TestCase):
         mock.patch.stopall()
 
     def test_read_config_files(self):
-
         # bad config_files
         for files in [0,
                       False,
@@ -298,7 +297,7 @@ class ConfigFileTestCase(unittest.TestCase):
             self.assertTrue(isinstance(conf,
                                        six.moves.configparser.ConfigParser.__class__))
         self.real_parser_clz.assert_called_once()
-        self.real_parser_clz.return_value.read.assert_called_once_with([files])
+        self.real_parser_clz.return_value.read.assert_called_once_with([files], encoding='utf8')
 
         # list as config_files
         self.reset_mock()
@@ -306,7 +305,7 @@ class ConfigFileTestCase(unittest.TestCase):
         koji.read_config_files(files)
 
         self.real_parser_clz.assert_called_once()
-        self.real_parser_clz.return_value.read.assert_called_once_with(files)
+        self.real_parser_clz.return_value.read.assert_called_once_with(files, encoding='utf8')
 
         # tuple as config_files
         self.reset_mock()
@@ -369,7 +368,8 @@ class ConfigFileTestCase(unittest.TestCase):
             ['test1.conf',
              'gooddir/test1-1.conf',
              'gooddir/test1-2.conf',
-             'test2.conf'])
+             'test2.conf'],
+            encoding='utf8')
         self.assertEqual(self.manager.isdir.call_count, 5)
         self.assertEqual(self.manager.isfile.call_count, 6)
         self.assertEqual(self.manager.access.call_count, 4)
@@ -1282,7 +1282,8 @@ class TestRmtree(unittest.TestCase):
 
     @patch('koji.util._rmtree')
     def test_rmtree_directory(self, _rmtree):
-        """ Tests that the koji.util._rmtree_nofork function returns nothing when the path is a directory.
+        """ Tests that the koji.util._rmtree_nofork function returns nothing
+        when the path is a directory.
         """
         stat = mock.MagicMock()
         stat.st_dev = 'dev'
@@ -1552,7 +1553,8 @@ class TestRmtree(unittest.TestCase):
     @mock.patch('os.unlink')
     @mock.patch('os.waitpid')
     @mock.patch('os._exit')
-    def test_rmtree_parent_logfail(self, _exit, waitpid, unlink, fork, rmtree_nofork, logsend, mkstemp):
+    def test_rmtree_parent_logfail(self, _exit, waitpid, unlink, fork, rmtree_nofork, logsend,
+                                   mkstemp):
         log = self.tempdir + '/rmtree-log.jsonl'
         fd = os.open(log, os.O_RDWR | os.O_CREAT)
         mkstemp.return_value = fd, log
@@ -1844,6 +1846,7 @@ class TestRmtree2(unittest.TestCase):
                         os.makedirs('%s/a/%s/c/d/%s/e/f/%s/g/h' % (dirname, i, j, k))
 
         sync = threading.Event()
+
         def do_rmtree(dirname):
             sync.wait()
             koji.util.rmtree(dirname)
@@ -1870,6 +1873,7 @@ class TestRmtree2(unittest.TestCase):
                     os.makedirs('%s/a/%s/c/d/%s/e/f/%s/g/h' % (dirname, i, j, k))
 
         sync = threading.Event()
+
         def do_rmtree(dirname):
             sync.wait()
             koji.util.rmtree(dirname)
@@ -1895,6 +1899,7 @@ class TestRmtree2(unittest.TestCase):
                     os.makedirs('%s/a/%s/c/d/%s/e/f/%s/g/h' % (dirname, i, j, k))
 
         sync = multiprocessing.Event()
+
         def do_rmtree(dirname):
             sync.wait()
             koji.util.rmtree(dirname)
@@ -2048,6 +2053,7 @@ class TestFormatShellCmd(unittest.TestCase):
         for inp, out in cases:
             self.assertEqual(koji.util.format_shell_cmd(inp, text_width=40), out)
 
+
 class TestExtractBuildTask(unittest.TestCase):
     def test_valid_binfos(self):
         binfos = [
diff --git a/tests/test_plugins/test_runroot_builder.py b/tests/test_plugins/test_runroot_builder.py
index f945925..e63af95 100644
--- a/tests/test_plugins/test_runroot_builder.py
+++ b/tests/test_plugins/test_runroot_builder.py
@@ -24,47 +24,47 @@ else:
 
 
 CONFIG1 = {
-        'paths': {
-            'default_mounts': '/mnt/archive,/mnt/workdir',
-            'safe_roots': '/mnt/workdir/tmp',
-            'path_subs':
-            '/mnt/archive/prehistory/,/mnt/prehistoric_disk/archive/prehistory',
-        },
-        'path0': {
-            'mountpoint': '/mnt/archive',
-            'path': 'archive.org:/vol/archive',
-            'fstype': 'nfs',
-            'options': 'ro,hard,intr,nosuid,nodev,noatime,tcp',
-        }}
+    'paths': {
+        'default_mounts': '/mnt/archive,/mnt/workdir',
+        'safe_roots': '/mnt/workdir/tmp',
+        'path_subs':
+        '/mnt/archive/prehistory/,/mnt/prehistoric_disk/archive/prehistory',
+    },
+    'path0': {
+        'mountpoint': '/mnt/archive',
+        'path': 'archive.org:/vol/archive',
+        'fstype': 'nfs',
+        'options': 'ro,hard,intr,nosuid,nodev,noatime,tcp',
+    }}
 
 
 CONFIG2 = {
-        'paths': {
-            'default_mounts': '/mnt/archive,/mnt/workdir',
-            'safe_roots': '/mnt/workdir/tmp',
-            'path_subs':
-                '\n'
-                '/mnt/archive/prehistory/,/mnt/prehistoric_disk/archive/prehistory\n'
-                '/mnt/archve/workdir,/mnt/workdir\n',
-        },
-        'path0': {
-            'mountpoint': '/mnt/archive',
-            'path': 'archive.org:/vol/archive',
-            'fstype': 'nfs',
-            'options': 'ro,hard,intr,nosuid,nodev,noatime,tcp',
-        },
-        'path1': {
-            'mountpoint': '/mnt/workdir',
-            'path': 'archive.org:/vol/workdir',
-            'fstype': 'nfs',
-            'options': 'ro,hard,intr,nosuid,nodev,noatime,tcp',
-        },
-        'path2': {
-            'mountpoint': '/mnt/prehistoric_disk',
-            'path': 'archive.org:/vol/prehistoric_disk',
-            'fstype': 'nfs',
-            'options': 'ro,hard,intr,nosuid,nodev,noatime,tcp',
-        }}
+    'paths': {
+        'default_mounts': '/mnt/archive,/mnt/workdir',
+        'safe_roots': '/mnt/workdir/tmp',
+        'path_subs':
+            '\n'
+            '/mnt/archive/prehistory/,/mnt/prehistoric_disk/archive/prehistory\n'
+            '/mnt/archve/workdir,/mnt/workdir\n',
+    },
+    'path0': {
+        'mountpoint': '/mnt/archive',
+        'path': 'archive.org:/vol/archive',
+        'fstype': 'nfs',
+        'options': 'ro,hard,intr,nosuid,nodev,noatime,tcp',
+    },
+    'path1': {
+        'mountpoint': '/mnt/workdir',
+        'path': 'archive.org:/vol/workdir',
+        'fstype': 'nfs',
+        'options': 'ro,hard,intr,nosuid,nodev,noatime,tcp',
+    },
+    'path2': {
+        'mountpoint': '/mnt/prehistoric_disk',
+        'path': 'archive.org:/vol/prehistoric_disk',
+        'fstype': 'nfs',
+        'options': 'ro,hard,intr,nosuid,nodev,noatime,tcp',
+    }}
 
 
 class FakeConfigParser(object):
@@ -75,7 +75,7 @@ class FakeConfigParser(object):
         else:
             self.CONFIG = copy.deepcopy(config)
 
-    def read(self, path):
+    def read(self, path, encoding):
         return
 
     def sections(self):
@@ -105,8 +105,7 @@ class TestRunrootConfig(unittest.TestCase):
         options.workdir = '/tmp/nonexistentdirectory'
         with self.assertRaises(koji.GenericError) as cm:
             runroot.RunRootTask(123, 'runroot', {}, session, options)
-        self.assertEqual(cm.exception.args[0],
-            "bad config: missing options in path0 section")
+        self.assertEqual(cm.exception.args[0], "bad config: missing options in path0 section")
 
     @mock.patch(CONFIG_PARSER)
     def test_bad_config_absolute_path(self, config_parser):
@@ -119,7 +118,8 @@ class TestRunrootConfig(unittest.TestCase):
         with self.assertRaises(koji.GenericError) as cm:
             runroot.RunRootTask(123, 'runroot', {}, session, options)
         self.assertEqual(cm.exception.args[0],
-            "bad config: all paths (default_mounts, safe_roots, path_subs) needs to be absolute: ")
+                         "bad config: all paths (default_mounts, safe_roots, path_subs) "
+                         "needs to be absolute: ")
 
     @mock.patch(CONFIG_PARSER)
     def test_valid_config(self, config_parser):
@@ -186,13 +186,14 @@ class TestMounts(unittest.TestCase):
 
         # valid item
         self.assertEqual(self.t._get_path_params('/mnt/archive', 'rw'),
-            ('archive.org:/vol/archive/', '/mnt/archive', 'nfs', 'rw,hard,intr,nosuid,nodev,noatime,tcp'))
+                         ('archive.org:/vol/archive/', '/mnt/archive', 'nfs',
+                          'rw,hard,intr,nosuid,nodev,noatime,tcp'))
 
     @mock.patch('koji._open_text_file')
     @mock.patch('os.path.isdir')
     @mock.patch('runroot.log_output')
     def test_do_mounts(self, log_output, is_dir, open_mock):
-        log_output.return_value = 0 # successful mount
+        log_output.return_value = 0  # successful mount
 
         # no mounts, don't do anything
         self.t.logger = mock.MagicMock()
@@ -202,8 +203,7 @@ class TestMounts(unittest.TestCase):
         # mountpoint has no absolute_path
         with self.assertRaises(koji.GenericError) as cm:
             self.t.do_mounts('rootdir', [('nfs:nfs', 'relative_path', 'nfs', '')])
-        self.assertEqual(cm.exception.args[0],
-                "invalid mount point: relative_path")
+        self.assertEqual(cm.exception.args[0], "invalid mount point: relative_path")
 
         # cover missing opts
         self.t.do_mounts('rootdir', [('nfs:nfs', '/mnt/archive', 'nfs', None)])
@@ -212,23 +212,24 @@ class TestMounts(unittest.TestCase):
         log_output.reset_mock()
         mounts = [self.t._get_path_params('/mnt/archive')]
         self.t.do_mounts('rootdir', mounts)
-        log_output.assert_called_once_with(self.session, 'mount',
-                ['mount', '-t', 'nfs', '-o', 'ro,hard,intr,nosuid,nodev,noatime,tcp',
-                'archive.org:/vol/archive/', 'rootdir/mnt/archive'],
-                '/tmp/nonexistentdirectory/tasks/123/123/do_mounts.log',
-                'tasks/123/123', append=True, logerror=True)
+        log_output.assert_called_once_with(
+            self.session, 'mount',
+            ['mount', '-t', 'nfs', '-o', 'ro,hard,intr,nosuid,nodev,noatime,tcp',
+             'archive.org:/vol/archive/', 'rootdir/mnt/archive'],
+            '/tmp/nonexistentdirectory/tasks/123/123/do_mounts.log', 'tasks/123/123',
+            append=True, logerror=True)
 
         # mount command failed
         log_output.reset_mock()
         log_output.return_value = 1
-        #self.t.undo_mounts = mock.MagicMock()
+        # self.t.undo_mounts = mock.MagicMock()
         mounts = [self.t._get_path_params('/mnt/archive')]
         with self.assertRaises(koji.GenericError) as cm:
             self.t.do_mounts('rootdir', mounts)
         self.assertEqual(cm.exception.args[0],
-            'Unable to mount rootdir/mnt/archive: mount -t nfs -o'
-            ' ro,hard,intr,nosuid,nodev,noatime,tcp archive.org:/vol/archive/'
-            ' rootdir/mnt/archive was killed by signal 1')
+                         'Unable to mount rootdir/mnt/archive: mount -t nfs -o'
+                         ' ro,hard,intr,nosuid,nodev,noatime,tcp archive.org:/vol/archive/'
+                         ' rootdir/mnt/archive was killed by signal 1')
 
         # bind ok
         log_output.return_value = 0
@@ -237,11 +238,12 @@ class TestMounts(unittest.TestCase):
         mount[3] += ',bind'
         is_dir.return_value = True
         self.t.do_mounts('rootdir', [mount])
-        log_output.assert_called_once_with(self.session, 'mount',
-                ['mount', '-t', 'none', '-o', 'ro,hard,intr,nosuid,nodev,noatime,tcp,bind',
-                'archive.org:/vol/archive/', 'rootdir/mnt/archive'],
-                '/tmp/nonexistentdirectory/tasks/123/123/do_mounts.log',
-                'tasks/123/123', append=True, logerror=True)
+        log_output.assert_called_once_with(
+            self.session, 'mount',
+            ['mount', '-t', 'none', '-o', 'ro,hard,intr,nosuid,nodev,noatime,tcp,bind',
+             'archive.org:/vol/archive/', 'rootdir/mnt/archive'],
+            '/tmp/nonexistentdirectory/tasks/123/123/do_mounts.log', 'tasks/123/123',
+            append=True, logerror=True)
 
         # bind - target doesn't exist
         mount = list(self.t._get_path_params('/mnt/archive'))
@@ -250,7 +252,7 @@ class TestMounts(unittest.TestCase):
         with self.assertRaises(koji.GenericError) as cm:
             self.t.do_mounts('rootdir', [mount])
         self.assertEqual(cm.exception.args[0],
-            "No such directory or mount: archive.org:/vol/archive/")
+                         "No such directory or mount: archive.org:/vol/archive/")
 
         # bg option forbidden
         log_output.reset_mock()
@@ -259,8 +261,7 @@ class TestMounts(unittest.TestCase):
         is_dir.return_value = False
         with self.assertRaises(koji.GenericError) as cm:
             self.t.do_mounts('rootdir', [mount])
-        self.assertEqual(cm.exception.args[0],
-            "bad config: background mount not allowed")
+        self.assertEqual(cm.exception.args[0], "bad config: background mount not allowed")
 
     def test_do_extra_mounts(self):
         self.t.do_mounts = mock.MagicMock()
@@ -288,7 +289,6 @@ class TestMounts(unittest.TestCase):
             self.t.do_extra_mounts('rootdir', ['/mnt/workdir/tmp/../xyz'])
         self.t.do_mounts.assert_not_called()
 
-
     @mock.patch('koji._open_text_file')
     @mock.patch('runroot.scan_mounts')
     @mock.patch('os.unlink')
@@ -318,10 +318,11 @@ class TestMounts(unittest.TestCase):
         with self.assertRaises(koji.GenericError) as cm:
             self.t.undo_mounts('rootdir')
         self.assertEqual(cm.exception.args[0],
-            'Unable to unmount: mountpoint: error, mount_2: error, mount_1: error')
+                         'Unable to unmount: mountpoint: error, mount_2: error, mount_1: error')
 
         os_unlink.assert_not_called()
 
+
 class TestHandler(unittest.TestCase):
     @mock.patch(CONFIG_PARSER)
     def setUp(self, config_parser):
@@ -335,7 +336,7 @@ class TestHandler(unittest.TestCase):
 
         options = mock.MagicMock()
         options.workdir = '/tmp/nonexistentdirectory'
-        #options.topurl = 'http://topurl'
+        # options.topurl = 'http://topurl'
         options.topurls = None
         self.t = runroot.RunRootTask(123, 'runroot', {}, self.session, options)
         self.t.config['default_mounts'] = ['default_mount']
@@ -371,10 +372,9 @@ class TestHandler(unittest.TestCase):
             'id': 1,
         }
         self.session.host.getHost.return_value = {'arches': 'x86_64'}
-        self.t.handler('tag_name', 'noarch', 'command', weight=10.0,
-                repo_id=1, packages=['rpm_a', 'rpm_b'], new_chroot=True,
-                mounts=['/mnt/a'], skip_setarch=True,
-                upload_logs=['log_1', 'log_2'])
+        self.t.handler('tag_name', 'noarch', 'command', weight=10.0, repo_id=1,
+                       packages=['rpm_a', 'rpm_b'], new_chroot=True, mounts=['/mnt/a'],
+                       skip_setarch=True, upload_logs=['log_1', 'log_2'])
 
         # calls
         self.session.host.setTaskWeight.assert_called_once_with(self.t.id, 10.0)
@@ -382,15 +382,18 @@ class TestHandler(unittest.TestCase):
         self.session.getBuildConfig.assert_called_once_with('tag_name')
         self.session.repoInfo.assert_called_once_with(1, strict=True)
         self.session.host.subtask.assert_not_called()
-        runroot.BuildRoot.assert_called_once_with(self.session, self.t.options,
-                'tag_name', 'x86_64', self.t.id, repo_id=1, setup_dns=True,
-                internal_dev_setup=None)
+        runroot.BuildRoot.assert_called_once_with(
+            self.session, self.t.options, 'tag_name', 'x86_64', self.t.id, repo_id=1,
+            setup_dns=True, internal_dev_setup=None)
         self.session.host.setBuildRootState.assert_called_once_with(678, 'BUILDING')
         self.br.mock.assert_has_calls([
             mock.call(['--install', 'rpm_a', 'rpm_b']),
-            mock.call(['--new-chroot', '--arch', 'arch', '--chroot', '--', '/bin/sh', '-c', '{ command; } < /dev/null 2>&1 | /usr/bin/tee /builddir/runroot.log; exit ${PIPESTATUS[0]}']),
+            mock.call(['--new-chroot', '--arch', 'arch', '--chroot', '--', '/bin/sh', '-c',
+                       '{ command; } < /dev/null 2>&1 | /usr/bin/tee /builddir/runroot.log; '
+                       'exit ${PIPESTATUS[0]}']),
         ])
-        self.session.host.updateBuildRootList.assert_called_once_with(678, self.br.getPackageList())
+        self.session.host.updateBuildRootList.assert_called_once_with(
+            678, self.br.getPackageList())
         self.t.do_mounts.assert_called_once_with('/rootdir', ['default_mount'])
         self.t.do_extra_mounts.assert_called_once_with('/rootdir', ['/mnt/a'])
         self.t.uploadFile.assert_has_calls([

From e1b55ec23cced14562980f83929a50b8358e99f1 Mon Sep 17 00:00:00 2001
From: Mike McLean <mikem@redhat.com>
Date: Nov 20 2024 14:37:07 +0000
Subject: [PATCH 2/2] add unit test


---

diff --git a/tests/common.py b/tests/common.py
new file mode 100644
index 0000000..47c4fdb
--- /dev/null
+++ b/tests/common.py
@@ -0,0 +1,32 @@
+import locale
+from functools import wraps
+
+
+class mylocale:
+    """Locale context manager. Resets to user default at exit."""
+
+    def __init__(self, value=None):
+        self.value = value
+
+    def __enter__(self):
+        locale.setlocale(locale.LC_ALL, locale=self.value)
+
+    def __exit__(self, _type, value, traceback):
+        # This resets to user default. Implementing a proper save and restore
+        # for locale is quite tricky, and this serves our testing needs.
+        locale.setlocale(locale.LC_ALL, "")
+        # return false so we don't eat exceptions
+        return False
+
+    def __call__(self, func):
+        """When called, act as a decorator"""
+
+        @wraps(func)
+        def wrapped(*args, **kwargs):
+            with self:
+                return func(*args, **kwargs)
+
+        return wrapped
+
+
+# the end
diff --git a/tests/test_lib/data/cfg/uni1.conf b/tests/test_lib/data/cfg/uni1.conf
new file mode 100644
index 0000000..e5b76ea
--- /dev/null
+++ b/tests/test_lib/data/cfg/uni1.conf
@@ -0,0 +1,7 @@
+# comment ěščřčž
+[koji]
+# key_file = äöüß
+; test comment with unicode 🫠
+# test comment with unicode ☢️
+setting = hello
+value = world 🌍
diff --git a/tests/test_lib/test_utils.py b/tests/test_lib/test_utils.py
index 42dd145..9d59ba1 100644
--- a/tests/test_lib/test_utils.py
+++ b/tests/test_lib/test_utils.py
@@ -3,7 +3,6 @@ from __future__ import absolute_import
 import calendar
 from datetime import datetime
 import errno
-import locale
 import logging
 import tempfile
 import threading
@@ -26,6 +25,7 @@ import six
 
 import koji
 import koji.util
+from ..common import mylocale
 
 
 class EnumTestCase(unittest.TestCase):
@@ -407,6 +407,23 @@ class ConfigFileTestCase(unittest.TestCase):
         self.assertEqual(listdir_mock.call_count, 2)
 
 
+class ConfigFileTestCase2(unittest.TestCase):
+    """Additional tests for config file reading functions"""
+
+    def setUp(self):
+        self.datadir = os.path.dirname(__file__) + '/data/cfg'
+
+    def tearDown(self):
+        mock.patch.stopall()
+
+    def test_unicode(self):
+        fn = self.datadir + '/uni1.conf'
+        if not os.path.exists(fn):
+            raise Exception('missing config')
+        with mylocale(value='C'):
+            koji.read_config_files(fn)
+
+
 class MavenUtilTestCase(unittest.TestCase):
     """Test maven relative functions"""
     maxDiff = None
@@ -750,10 +767,9 @@ class MavenUtilTestCase(unittest.TestCase):
                 config.read_file(conf_file)
         return config
 
+    @mylocale(('en_US', 'UTF-8'))
     def test_formatChangelog(self):
         """Test formatChangelog function"""
-        # force locale to compare 'expect' value
-        locale.setlocale(locale.LC_ALL, ('en_US', 'UTF-8'))
         data = [
             {
                 'author': 'Happy Koji User <user1@example.com> - 1.1-1',
@@ -787,8 +803,6 @@ class MavenUtilTestCase(unittest.TestCase):
         result = koji.util.formatChangelog(data)
         self.assertMultiLineEqual(expect, result)
 
-        locale.setlocale(locale.LC_ALL, "")
-
     def test_parseTime(self):
         """Test parseTime function"""
         now = datetime.now()

