[gentoo-commits] proj/pkgcore/pkgcheck:master commit in: tests/checks/, src/pkgcheck/checks/

[Arthur Zamarin]

commit:     4e69c9d901c37d631cfff6316103e4be53b214c3
Author:     Arthur Zamarin <arthurzam <AT> gentoo <DOT> org>
AuthorDate: Sun Oct 30 18:36:27 2022 +0000
Commit:     Arthur Zamarin <arthurzam <AT> gentoo <DOT> org>
CommitDate: Mon Oct 31 18:29:55 2022 +0000
URL:        https://gitweb.gentoo.org/proj/pkgcore/pkgcheck.git/commit/?id=4e69c9d9

AcctCheck: use metadata/qa-policy.conf for dynamic id range

Load UID and GID range from metadata/qa-policy.conf under the repo,
validate the format is as expected, and use the values to set expected
ids.

Resolves: https://github.com/pkgcore/pkgcheck/issues/356
Signed-off-by: Arthur Zamarin <arthurzam <AT> gentoo.org>

 src/pkgcheck/checks/acct.py | 60 +++++++++++++++++++++++++++++----------
 tests/checks/test_acct.py   | 69 ++++++++++++++++++++++++++++++++++++++++-----
 2 files changed, 107 insertions(+), 22 deletions(-)

diff --git a/src/pkgcheck/checks/acct.py b/src/pkgcheck/checks/acct.py
index cb0053e3..4f144023 100644
--- a/src/pkgcheck/checks/acct.py
+++ b/src/pkgcheck/checks/acct.py
@@ -2,14 +2,16 @@
 
 import re
 from collections import defaultdict
+from configparser import ConfigParser
 from functools import partial
 from itertools import chain
 
 from pkgcore.ebuild import restricts
 from pkgcore.restrictions import packages
+from snakeoil.osutils import pjoin
 
 from .. import results, sources
-from . import GentooRepoCheck, RepoCheck
+from . import GentooRepoCheck, RepoCheck, SkipCheck
 
 
 class MissingAccountIdentifier(results.VersionResult, results.Warning):
@@ -35,13 +37,16 @@ class ConflictingAccountIdentifiers(results.Error):
 
     @property
     def desc(self):
-        return (
-            f"conflicting {self.kind} id {self.identifier} usage: "
-            f"[ {', '.join(self.pkgs)} ]")
+        pkgs = ', '.join(self.pkgs)
+        return f"conflicting {self.kind} id {self.identifier} usage: [ {pkgs} ]"
 
 
 class OutsideRangeAccountIdentifier(results.VersionResult, results.Error):
-    """UID/GID outside allowed allocation range."""
+    """UID/GID outside allowed allocation range.
+
+    To view the range accepted for this repository, look at the file
+    ``metadata/qa-policy.conf`` in the section ``user-group-ids``.
+    """
 
     def __init__(self, kind, identifier, **kwargs):
         super().__init__(**kwargs)
@@ -50,16 +55,20 @@ class OutsideRangeAccountIdentifier(results.VersionResult, results.Error):
 
     @property
     def desc(self):
-        return (
-            f"{self.kind} id {self.identifier} outside permitted "
-            f"static allocation range (0..499, 60001+)")
+        return (f"{self.kind} id {self.identifier} outside permitted "
+            f"static allocation range")
 
 
 class AcctCheck(GentooRepoCheck, RepoCheck):
     """Various checks for acct-* packages.
 
     Verify that acct-* packages do not use conflicting, invalid or out-of-range
-    UIDs/GIDs.
+    UIDs/GIDs. This check uses a special file ``metadata/qa-policy.conf``
+    located within the repository. It should contain a ``user-group-ids``
+    section containing two keys: ``uid-range`` and ``gid-range``, which consist
+    of a comma separated list, either ``<n>`` for a single value or ``<m>-<n>``
+    for a range of values (including both ends). In case this file doesn't
+    exist or is wrongly defined, this check is skipped.
     """
 
     _restricted_source = (sources.RestrictionRepoSource, (packages.OrRestriction(*(
@@ -76,14 +85,37 @@ class AcctCheck(GentooRepoCheck, RepoCheck):
             r'ACCT_(?P<var>USER|GROUP)_ID=(?P<quot>[\'"]?)(?P<id>[0-9]+)(?P=quot)')
         self.seen_uids = defaultdict(partial(defaultdict, list))
         self.seen_gids = defaultdict(partial(defaultdict, list))
+        uid_range, gid_range = self.load_ids_from_configuration(self.options.target_repo)
         self.category_map = {
-            'acct-user': (self.seen_uids, 'USER', (65534,)),
-            'acct-group': (self.seen_gids, 'GROUP', (65533, 65534)),
+            'acct-user': (self.seen_uids, 'USER', tuple(uid_range)),
+            'acct-group': (self.seen_gids, 'GROUP', tuple(gid_range)),
         }
 
+    def parse_config_id_range(self, config: ConfigParser, config_key: str):
+        id_ranges = config['user-group-ids'].get(config_key, None)
+        if not id_ranges:
+            raise SkipCheck(self, f"metadata/qa-policy.conf: missing value for {config_key}")
+        try:
+            for id_range in map(str.strip, id_ranges.split(',')):
+                start, *end = map(int, id_range.split('-', maxsplit=1))
+                if len(end) == 0:
+                    yield range(start, start + 1)
+                else:
+                    yield range(start, end[0] + 1)
+        except ValueError:
+            raise SkipCheck(self, f"metadata/qa-policy.conf: invalid value for {config_key}")
+
+    def load_ids_from_configuration(self, repo):
+        config = ConfigParser()
+        if not config.read(pjoin(repo.location, 'metadata', 'qa-policy.conf')):
+            raise SkipCheck(self, "failed loading 'metadata/qa-policy.conf'")
+        if 'user-group-ids' not in config:
+            raise SkipCheck(self, "metadata/qa-policy.conf: missing section user-group-ids")
+        return self.parse_config_id_range(config, 'uid-range'), self.parse_config_id_range(config, 'gid-range')
+
     def feed(self, pkg):
         try:
-            seen_id_map, expected_var, extra_allowed_ids = self.category_map[pkg.category]
+            seen_id_map, expected_var, allowed_ids = self.category_map[pkg.category]
         except KeyError:
             return
 
@@ -96,9 +128,7 @@ class AcctCheck(GentooRepoCheck, RepoCheck):
             yield MissingAccountIdentifier(f"ACCT_{expected_var}_ID", pkg=pkg)
             return
 
-        # all UIDs/GIDs must be in <750, with special exception
-        # of nobody/nogroup which use 65534/65533
-        if found_id >= 750 and found_id not in extra_allowed_ids:
+        if not any(found_id in id_range for id_range in allowed_ids):
             yield OutsideRangeAccountIdentifier(expected_var.lower(), found_id, pkg=pkg)
             return
 

diff --git a/tests/checks/test_acct.py b/tests/checks/test_acct.py
index cd8f852f..57273705 100644
--- a/tests/checks/test_acct.py
+++ b/tests/checks/test_acct.py
@@ -1,4 +1,7 @@
-from pkgcheck.checks import acct
+import textwrap
+
+import pytest
+from pkgcheck.checks import acct, SkipCheck
 from pkgcore.test.misc import FakeRepo
 from snakeoil.cli import arghparse
 
@@ -11,17 +14,27 @@ class TestAcctUser(misc.ReportTestCase):
 
     kind = 'user'
 
+    @pytest.fixture(autouse=True)
+    def _setup(self, tmp_path):
+        (metadata := tmp_path / 'metadata').mkdir()
+        (metadata / 'qa-policy.conf').write_text(textwrap.dedent("""\
+            [user-group-ids]
+            uid-range = 0-749,65534
+            gid-range = 0-749,65533,65534
+        """))
+        self.location = str(tmp_path)
+
     def mk_check(self, pkgs):
-        self.repo = FakeRepo(pkgs=pkgs, repo_id='test')
-        check = self.check_kls(arghparse.Namespace(target_repo=self.repo, gentoo_repo=True))
+        repo = FakeRepo(pkgs=pkgs, repo_id='test', location=self.location)
+        check = self.check_kls(arghparse.Namespace(target_repo=repo, gentoo_repo=True))
         return check
 
     def mk_pkg(self, name, identifier, version=1, ebuild=None):
         if ebuild is None:
-            ebuild = f'''
-inherit acct-{self.kind}
-ACCT_{self.kind.upper()}_ID="{identifier}"
-'''
+            ebuild = textwrap.dedent(f'''\
+                inherit acct-{self.kind}
+                ACCT_{self.kind.upper()}_ID="{identifier}"
+            ''')
         return misc.FakePkg(f'acct-{self.kind}/{name}-{version}', ebuild=ebuild)
 
     def test_unmatching_pkgs(self):
@@ -33,6 +46,7 @@ ACCT_{self.kind.upper()}_ID="{identifier}"
     def test_correct_ids(self):
         pkgs = (self.mk_pkg('foo', 100),
                 self.mk_pkg('bar', 200),
+                self.mk_pkg('test', 749),
                 self.mk_pkg('nobody', 65534))
         check = self.mk_check(pkgs)
         self.assertNoReport(check, pkgs)
@@ -110,3 +124,44 @@ class TestAcctGroup(TestAcctUser):
         pkg = self.mk_pkg('nogroup', 65533)
         check = self.mk_check((pkg,))
         self.assertNoReport(check, pkg)
+
+
+class TestQaPolicyValidation(misc.ReportTestCase):
+
+    def mk_check(self, tmp_path, content):
+        if content:
+            (metadata := tmp_path / 'metadata').mkdir()
+            (metadata / 'qa-policy.conf').write_text(textwrap.dedent(content))
+        repo = FakeRepo(repo_id='test', location=str(tmp_path))
+        return acct.AcctCheck(arghparse.Namespace(target_repo=repo, gentoo_repo=True))
+
+    def test_missing_qa_policy(self, tmp_path):
+        with pytest.raises(SkipCheck, match="failed loading 'metadata/qa-policy.conf'"):
+            self.mk_check(tmp_path, None)
+
+    def test_missing_section(self, tmp_path):
+        with pytest.raises(SkipCheck, match="missing section user-group-ids"):
+            self.mk_check(tmp_path, '''\
+                [random]
+                x = 5
+            ''')
+
+    def test_missing_config(self, tmp_path):
+        with pytest.raises(SkipCheck, match="missing value for gid-range"):
+            self.mk_check(tmp_path, '''\
+                [user-group-ids]
+                uid-range = 0-749
+            ''')
+
+    @pytest.mark.parametrize('value', (
+        'start-end',
+        '0-749-1500',
+        ',150',
+    ))
+    def test_invalid_value(self, tmp_path, value):
+        with pytest.raises(SkipCheck, match="invalid value for uid-range"):
+            self.mk_check(tmp_path, f'''\
+                [user-group-ids]
+                uid-range = {value}
+                gid-range = 0-749
+            ''')