Merge branch 'config-options-as-module-level-variables' into 'master'

Draft: clarify that 'config' and 'options' should only be module-level variables

Closes #1128

See merge request fdroid/fdroidserver!1477

.gitlab-ci.yml

@@ -285,9 +285,7 @@ black:
     - black --check --diff --color $CI_PROJECT_DIR
 
 fedora_latest:
-  image: fedora:latest
-  only:
-    - master@fdroid/fdroidserver
+  image: fedora:39  # support ends on 2024-11-12
   script:
     # tricks to hopefully make runs more reliable
     - echo "timeout=600" >> /etc/dnf/dnf.conf

fdroidserver/checkupdates.py

@@ -718,7 +718,7 @@ def main():
     parser.add_argument("--allow-dirty", action="store_true", default=False,
                         help=_("Run on git repo that has uncommitted changes"))
     metadata.add_metadata_arguments(parser)
-    options = parser.parse_args()
+    options = common.parse_args(parser)
     metadata.warnings_action = options.W
 
     config = common.read_config()

fdroidserver/common.py

@@ -2395,6 +2395,8 @@ def prepare_source(vcs, app, build, build_dir, srclib_dir, extlib_dir, onserver=
                 gradlefile = build_gradle
             elif os.path.exists(build_gradle_kts):
                 gradlefile = build_gradle_kts
+            else:
+                raise BuildException("No gradle file found")
             regsub_file(r'compileSdkVersion[ =]+[0-9]+',
                         r'compileSdkVersion %s' % n,
                         gradlefile)

fdroidserver/lint.py

@@ -721,7 +721,13 @@ def check_updates_ucm_http_aum_pattern(app):  # noqa: D403
 
 
 def check_certificate_pinned_binaries(app):
-    if len(app.get('AllowedAPKSigningKeys')) > 0:
+    keys = app.get('AllowedAPKSigningKeys')
+    known_keys = common.config.get('apk_signing_key_block_list', [])
+    if keys:
+        if known_keys:
+            for key in keys:
+                if key in known_keys:
+                    yield _('Known debug key is used in AllowedAPKSigningKeys: ') + key
         return
     if app.get('Binaries') is not None:
         yield _(

tests/common.TestCase

@@ -77,7 +77,8 @@ class CommonTest(unittest.TestCase):
         fdroidserver.common.options = None
         os.chdir(self.basedir)
         self._td.cleanup()
-        shutil.rmtree(self.tmpdir)
+        if os.path.exists(self.tmpdir):
+            shutil.rmtree(self.tmpdir)
 
     def test_parse_human_readable_size(self):
         for k, v in (
@@ -3285,7 +3286,7 @@ class ConfigOptionsScopeTest(unittest.TestCase):
         self.assertFalse('options' in globals())
 
     def test_parse_args_without_args(self):
-        """"""
+        """Test that the parsing function works fine when there are no args."""
         parser = ArgumentParser()
         fdroidserver.common.setup_global_opts(parser)
         with mock.patch('sys.argv', ['$0']):

tests/dump_internal_metadata_format.py

@@ -56,7 +56,7 @@ def _build_yaml_representer(dumper, data):
 parser = ArgumentParser()
 fdroidserver.common.setup_global_opts(parser)
 fdroidserver.metadata.add_metadata_arguments(parser)
-options = common.parse_args(parser)
+options = fdroidserver.common.parse_args(parser)
 fdroidserver.metadata.warnings_action = options.W
 fdroidserver.common.read_config()
 

tests/index.TestCase

@@ -712,8 +712,14 @@ class IndexTest(unittest.TestCase):
         app = apps[appid]
         metadata = index.package_metadata(app, 'repo')
         # files
-        self.assertEqual(36027, metadata['featureGraphic']['en-US']['size'])
-        self.assertEqual(1413, metadata['icon']['en-US']['size'])
+        self.assertEqual(
+            os.path.getsize(f'repo/{appid}/en-US/featureGraphic.png'),
+            metadata['featureGraphic']['en-US']['size'],
+        )
+        self.assertEqual(
+            os.path.getsize(f'repo/{appid}/en-US/icon.png'),
+            metadata['icon']['en-US']['size'],
+        )
         # localized strings
         self.assertEqual({'en-US': 'title'}, metadata['name'])
         self.assertEqual({'en-US': 'video'}, metadata['video'])

tests/lint.TestCase

@@ -437,6 +437,45 @@ class LintTest(unittest.TestCase):
         with self.assertRaises(TypeError):
             fdroidserver.lint.lint_config('mirrors.yml')
 
+    def test_check_certificate_pinned_binaries_empty(self):
+        fdroidserver.common.config = {}
+        app = fdroidserver.metadata.App()
+        app.AllowedAPKSigningKeys = [
+            'a40da80a59d170caa950cf15c18c454d47a39b26989d8b640ecd745ba71bf5dc'
+        ]
+        self.assertEqual(
+            [],
+            list(fdroidserver.lint.check_certificate_pinned_binaries(app)),
+            "when the config is empty, any signing key should be allowed",
+        )
+
+    def test_lint_known_debug_keys_no_match(self):
+        fdroidserver.common.config = {
+            "apk_signing_key_block_list": "a40da80a59d170caa950cf15c18c454d47a39b26989d8b640ecd745ba71bf5dc"
+        }
+        app = fdroidserver.metadata.App()
+        app.AllowedAPKSigningKeys = [
+            '2fd4fd5f54babba4bcb21237809bb653361d0d2583c80964ec89b28a26e9539e'
+        ]
+        self.assertEqual(
+            [],
+            list(fdroidserver.lint.check_certificate_pinned_binaries(app)),
+            "A signing key that does not match one in the config should be allowed",
+        )
+
+    def test_lint_known_debug_keys(self):
+        fdroidserver.common.config = {
+            'apk_signing_key_block_list': 'a40da80a59d170caa950cf15c18c454d47a39b26989d8b640ecd745ba71bf5dc'
+        }
+        app = fdroidserver.metadata.App()
+        app.AllowedAPKSigningKeys = [
+            'a40da80a59d170caa950cf15c18c454d47a39b26989d8b640ecd745ba71bf5dc'
+        ]
+        for warn in fdroidserver.lint.check_certificate_pinned_binaries(app):
+            anywarns = True
+            logging.debug(warn)
+        self.assertTrue(anywarns)
+
 
 class LintAntiFeaturesTest(unittest.TestCase):
     def setUp(self):

tests/nightly.TestCase

@@ -69,7 +69,10 @@ class NightlyTest(unittest.TestCase):
 
     def tearDown(self):
         self.tempdir.cleanup()
-        os.rmdir(self.testroot)
+        try:
+            os.rmdir(self.testroot)
+        except OSError:  # other test modules might have left stuff around
+            pass
 
     def _copy_test_debug_keystore(self):
         self.dot_android.mkdir()

tests/testcommon.py

@@ -18,9 +18,10 @@
 import os
 import sys
 import tempfile
-from unittest import mock
+import unittest
 
-import fdroidserver.common
+from pathlib import Path
+from unittest import mock
 
 
 class TmpCwd:
@@ -65,11 +66,23 @@ def mkdtemp():
         return tempfile.TemporaryDirectory(ignore_cleanup_errors=True)
 
 
+def mkdir_testfiles(localmodule, test):
+    """Keep the test files in a labeled test dir for easy reference"""
+    testroot = Path(localmodule) / '.testfiles'
+    testroot.mkdir(exist_ok=True)
+    testdir = testroot / unittest.TestCase.id(test)
+    testdir.mkdir(exist_ok=True)
+    return tempfile.mkdtemp(dir=testdir)
+
+
 def parse_args_for_test(parser, args):
     """Only send --flags to the ArgumentParser, not test classes, etc."""
+
+    from fdroidserver.common import parse_args
+
     flags = []
     for arg in args:
         if arg[0] == '-':
             flags.append(flags)
     with mock.patch('sys.argv', flags):
-        fdroidserver.common.parse_args(parser)
+        parse_args(parser)