launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging lp:~cjwatson/launchpad/git-permissions-non-unicode-refs into lp:launchpad.

Commit message:
Begin converting GitAPI.checkRefPermissions to accept ref paths as bytes.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1517559 in Launchpad itself: "git fine-grained permissions"
  https://bugs.launchpad.net/launchpad/+bug/1517559

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/git-permissions-non-unicode-refs/+merge/359088

Git ref paths may not be valid UTF-8, so we need to treat them as bytes.  We can't deal perfectly with non-UTF-8 refs - they won't get scanned and so won't show up in the webservice API or the web UI - but we can at least allow them to round-trip through Launchpad at the git level.  I believe we tested that a while back and it was fine, but then we forgot about it during the permissions work so it regressed.

I have most of a turnip branch to deal with the other end of this, but the webapp needs to support it first.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/git-permissions-non-unicode-refs into lp:launchpad.

=== modified file 'lib/lp/code/interfaces/gitrepository.py'
--- lib/lp/code/interfaces/gitrepository.py	2018-11-09 22:06:43 +0000
+++ lib/lp/code/interfaces/gitrepository.py	2018-11-21 01:09:21 +0000
@@ -824,7 +824,8 @@
         :param person: An `IPerson` to check, or
             `GitGranteeType.REPOSITORY_OWNER` to check an anonymous
             repository owner.
-        :param ref_paths: An iterable of reference paths.
+        :param ref_paths: An iterable of reference paths (each of which may
+            be either bytes or text).
         :return: A dict mapping reference paths to sets of
             `GitPermissionType`, corresponding to the requested person's
             effective permissions on each of the requested references.

=== modified file 'lib/lp/code/model/gitrepository.py'
--- lib/lp/code/model/gitrepository.py	2018-11-09 22:06:43 +0000
+++ lib/lp/code/model/gitrepository.py	2018-11-21 01:09:21 +0000
@@ -1311,8 +1311,12 @@
             grants_for_user[grant.rule].append(grant)
 
         for ref_path in ref_paths:
+            encoded_ref_path = (
+                ref_path if isinstance(ref_path, bytes)
+                else ref_path.encode("UTF-8"))
             matching_rules = [
-                rule for rule in rules if fnmatch(ref_path, rule.ref_pattern)]
+                rule for rule in rules if
+                fnmatch(encoded_ref_path, rule.ref_pattern.encode("UTF-8"))]
             if is_owner and not matching_rules:
                 # If there are no matching rules, then the repository owner
                 # can do anything.

=== modified file 'lib/lp/code/xmlrpc/git.py'
--- lib/lp/code/xmlrpc/git.py	2018-10-25 17:52:43 +0000
+++ lib/lp/code/xmlrpc/git.py	2018-11-21 01:09:21 +0000
@@ -11,6 +11,7 @@
 import sys
 
 from pymacaroons import Macaroon
+from six.moves import xmlrpc_client
 from storm.store import Store
 import transaction
 from zope.component import (
@@ -364,11 +365,29 @@
             assert repository.repository_type == GitRepositoryType.IMPORTED
             requester = GitGranteeType.REPOSITORY_OWNER
 
-        return {
-            ref_path: self._renderPermissions(permissions)
-            for ref_path, permissions in repository.checkRefPermissions(
-                requester, ref_paths).items()
-            }
+        if all(isinstance(ref_path, xmlrpc_client.Binary)
+               for ref_path in ref_paths):
+            # New protocol: caller sends paths as bytes; Launchpad returns a
+            # list of (path, permissions) tuples.  (XML-RPC doesn't support
+            # dict keys being bytes.)
+            ref_paths = [ref_path.data for ref_path in ref_paths]
+            return [
+                (xmlrpc_client.Binary(ref_path),
+                 self._renderPermissions(permissions))
+                for ref_path, permissions in repository.checkRefPermissions(
+                    requester, ref_paths).items()
+                ]
+        else:
+            # Old protocol: caller sends paths as text; Launchpad returns a
+            # dict of {path: permissions}.
+            # XXX cjwatson 2018-11-21: Remove this once turnip has migrated
+            # to the new protocol.  git ref paths are not required to be
+            # valid UTF-8.
+            return {
+                ref_path: self._renderPermissions(permissions)
+                for ref_path, permissions in repository.checkRefPermissions(
+                    requester, ref_paths).items()
+                }
 
     def checkRefPermissions(self, translated_path, ref_paths, auth_params):
         """ See `IGitAPI`"""

=== modified file 'lib/lp/code/xmlrpc/tests/test_git.py'
--- lib/lp/code/xmlrpc/tests/test_git.py	2018-10-24 00:42:39 +0000
+++ lib/lp/code/xmlrpc/tests/test_git.py	2018-11-21 01:09:21 +0000
@@ -6,9 +6,15 @@
 __metaclass__ = type
 
 from pymacaroons import Macaroon
+from six.moves import xmlrpc_client
 from testtools.matchers import (
     Equals,
+    IsInstance,
+    MatchesAll,
     MatchesDict,
+    MatchesListwise,
+    MatchesSetwise,
+    MatchesStructure,
     )
 from zope.component import getUtility
 from zope.security.proxy import removeSecurityProxy
@@ -194,11 +200,25 @@
         if macaroon_raw is not None:
             auth_params["macaroon"] = macaroon_raw
         translated_path = removeSecurityProxy(repository).getInternalPath()
-        results = self.git_api.checkRefPermissions(
-            translated_path, ref_paths, auth_params)
-        self.assertThat(results, MatchesDict({
-            ref_path: Equals(ref_permissions)
-            for ref_path, ref_permissions in permissions.items()}))
+        if all(isinstance(ref_path, bytes) for ref_path in ref_paths):
+            ref_paths = [
+                xmlrpc_client.Binary(ref_path) for ref_path in ref_paths]
+            results = self.git_api.checkRefPermissions(
+                translated_path, ref_paths, auth_params)
+            self.assertThat(results, MatchesSetwise(*(
+                MatchesListwise([
+                    MatchesAll(
+                        IsInstance(xmlrpc_client.Binary),
+                        MatchesStructure.byEquality(data=ref_path)),
+                    Equals(ref_permissions),
+                    ])
+                for ref_path, ref_permissions in permissions.items())))
+        else:
+            results = self.git_api.checkRefPermissions(
+                translated_path, ref_paths, auth_params)
+            self.assertThat(results, MatchesDict({
+                ref_path: Equals(ref_permissions)
+                for ref_path, ref_permissions in permissions.items()}))
 
     def test_translatePath_private_repository(self):
         requester = self.factory.makePerson()
@@ -519,6 +539,50 @@
             'refs/other': Equals([]),
         }))
 
+    def test_checkRefPermissions_bytes(self):
+        owner = self.factory.makePerson()
+        grantee = self.factory.makePerson()
+        no_privileges = self.factory.makePerson()
+        repository = removeSecurityProxy(
+            self.factory.makeGitRepository(owner=owner))
+        self.factory.makeGitRuleGrant(
+            repository=repository, ref_pattern=u"refs/heads/next/*",
+            grantee=grantee, can_push=True)
+        paths = [
+            # Properly-encoded UTF-8.
+            u"refs/heads/next/\N{BLACK HEART SUIT}".encode("UTF-8"),
+            # Non-UTF-8.  (git does not require any particular encoding for
+            # ref paths; non-UTF-8 ones won't work well everywhere, but it's
+            # at least possible to round-trip them through Launchpad.)
+            b"refs/heads/next/\x80",
+            ]
+
+        self.assertHasRefPermissions(
+            grantee, repository, paths, {path: ["push"] for path in paths})
+        login(ANONYMOUS)
+        self.assertHasRefPermissions(
+            no_privileges, repository, paths, {path: [] for path in paths})
+
+    def test_checkRefPermissions_unicode(self):
+        # Actual Unicode ref paths work too.
+        # XXX cjwatson 2018-11-21: Remove this when the transition to the
+        # new protocol is complete.
+        owner = self.factory.makePerson()
+        grantee = self.factory.makePerson()
+        no_privileges = self.factory.makePerson()
+        repository = removeSecurityProxy(
+            self.factory.makeGitRepository(owner=owner))
+        self.factory.makeGitRuleGrant(
+            repository=repository, ref_pattern=u"refs/heads/next/*",
+            grantee=grantee, can_push=True)
+        path = u"refs/heads/next/\N{SNOWMAN}"
+
+        self.assertHasRefPermissions(
+            grantee, repository, [path], {path: ["push"]})
+        login(ANONYMOUS)
+        self.assertHasRefPermissions(
+            no_privileges, repository, [path], {path: []})
+
 
 class TestGitAPI(TestGitAPIMixin, TestCaseWithFactory):
     """Tests for the implementation of `IGitAPI`."""