launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #23095
[Merge] ~cjwatson/turnip:non-ascii-ref-permissions into turnip:master
Colin Watson has proposed merging ~cjwatson/turnip:non-ascii-ref-permissions into turnip:master with ~cjwatson/turnip:hookrpc-test as a prerequisite.
Commit message:
Use new bytes-capable checkRefPermissions protocol
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. This
migrates to a new version of the checkRefPermissions protocol which can
cope with ref paths being bytes rather than text.
LP: #1517559
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #1517559 in turnip: "git fine-grained permissions"
https://bugs.launchpad.net/turnip/+bug/1517559
For more details, see:
https://code.launchpad.net/~cjwatson/turnip/+git/turnip/+merge/359152
https://code.launchpad.net/~cjwatson/launchpad/git-permissions-non-unicode-refs/+merge/359088 must be on production before landing this.
--
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/turnip:non-ascii-ref-permissions into turnip:master.
diff --git a/turnip/pack/hookrpc.py b/turnip/pack/hookrpc.py
index 7d818bf..b024734 100644
--- a/turnip/pack/hookrpc.py
+++ b/turnip/pack/hookrpc.py
@@ -1,4 +1,4 @@
-# Copyright 2015 Canonical Ltd. This software is licensed under the
+# Copyright 2015-2018 Canonical Ltd. This software is licensed under the
# GNU Affero General Public License version 3 (see the file LICENSE).
"""RPC server for Git hooks.
@@ -20,8 +20,10 @@ from __future__ import (
unicode_literals,
)
+import base64
import json
+from six.moves import xmlrpc_client
from twisted.internet import (
defer,
protocol,
@@ -124,22 +126,24 @@ class HookRPCHandler(object):
def checkRefPermissions(self, proto, args):
"""Get permissions for a set of refs."""
cached_permissions = self.ref_permissions[args['key']]
- missing = [x for x in args['paths']
- if x not in cached_permissions]
+ paths = [
+ base64.b64decode(path.encode('UTF-8')) for path in args['paths']]
+ missing = [x for x in paths if x not in cached_permissions]
if missing:
proxy = xmlrpc.Proxy(self.virtinfo_url, allowNone=True)
result = yield proxy.callRemote(
b'checkRefPermissions',
self.ref_paths[args['key']],
- missing,
+ [xmlrpc_client.Binary(path) for path in missing],
self.auth_params[args['key']]
)
- for ref, permission in result.items():
- cached_permissions[ref] = permission
+ for ref, permission in result:
+ cached_permissions[ref.data] = permission
# cached_permissions is a shallow copy of the key index for
# self.ref_permissions, so changes will be updated in that.
defer.returnValue(
- {ref: cached_permissions[ref] for ref in args['paths']})
+ {base64.b64encode(ref).decode('UTF-8'): cached_permissions[ref]
+ for ref in paths})
@defer.inlineCallbacks
def notify(self, path):
diff --git a/turnip/pack/hooks/hook.py b/turnip/pack/hooks/hook.py
index 1594a4b..22251a2 100755
--- a/turnip/pack/hooks/hook.py
+++ b/turnip/pack/hooks/hook.py
@@ -1,6 +1,6 @@
#!/usr/bin/python
-# Copyright 2015 Canonical Ltd. This software is licensed under the
+# Copyright 2015-2018 Canonical Ltd. This software is licensed under the
# GNU Affero General Public License version 3 (see the file LICENSE).
from __future__ import (
@@ -9,6 +9,7 @@ from __future__ import (
unicode_literals,
)
+import base64
import json
import os
import socket
@@ -41,13 +42,13 @@ def determine_permissions_outcome(old, ref, rule_lines):
if 'create' in rule:
return
else:
- return 'You do not have permission to create %s.' % ref
+ return b'You do not have permission to create ' + ref + b'.'
# We have push permission, everything is okay
# force_push is checked later (in update-hook)
if 'push' in rule:
return
# If we're here, there are no matching rules
- return "You do not have permission to push to %s." % ref
+ return b"You do not have permission to push to " + ref + b"."
def match_rules(rule_lines, ref_lines):
@@ -89,7 +90,7 @@ def match_update_rules(rule_lines, ref_line):
rule = rule_lines.get(ref, [])
if 'force_push' in rule:
return []
- return ['You do not have permission to force-push to %s.' % ref]
+ return [b'You do not have permission to force-push to ' + ref + b'.']
def netstring_send(sock, s):
@@ -120,6 +121,16 @@ def rpc_invoke(sock, method, args):
return res['result']
+def check_ref_permissions(sock, rpc_key, ref_paths):
+ ref_paths = [base64.b64encode(path).decode('UTF-8') for path in ref_paths]
+ rule_lines = rpc_invoke(
+ sock, b'check_ref_permissions',
+ {'key': rpc_key, 'paths': ref_paths})
+ return {
+ base64.b64decode(path.encode('UTF-8')): permissions
+ for path, permissions in rule_lines.items()}
+
+
if __name__ == '__main__':
# Connect to the RPC server, authenticating using the random key
# from the environment.
@@ -132,27 +143,23 @@ if __name__ == '__main__':
# Verify the proposed changes against rules from the server.
raw_paths = sys.stdin.readlines()
ref_paths = [p.rstrip(b'\n').split(b' ', 2)[2] for p in raw_paths]
- rule_lines = rpc_invoke(
- sock, b'check_ref_permissions',
- {'key': rpc_key, 'paths': ref_paths})
+ rule_lines = check_ref_permissions(sock, rpc_key, ref_paths)
errors = match_rules(rule_lines, raw_paths)
for error in errors:
- sys.stdout.write(error + '\n')
+ sys.stdout.write(error + b'\n')
sys.exit(1 if errors else 0)
elif hook == 'post-receive':
# Notify the server about the push if there were any changes.
# Details of the changes aren't currently included.
if sys.stdin.readlines():
- rule_lines = rpc_invoke(sock, b'notify_push', {'key': rpc_key})
+ rpc_invoke(sock, b'notify_push', {'key': rpc_key})
sys.exit(0)
elif hook == 'update':
ref = sys.argv[1]
- rule_lines = rpc_invoke(
- sock, b'check_ref_permissions',
- {'key': rpc_key, 'paths': [ref]})
+ rule_lines = check_ref_permissions(sock, rpc_key, [ref])
errors = match_update_rules(rule_lines, sys.argv[1:4])
for error in errors:
- sys.stdout.write(error + '\n')
+ sys.stdout.write(error + b'\n')
sys.exit(1 if errors else 0)
else:
sys.stderr.write('Invalid hook name: %s' % hook)
diff --git a/turnip/pack/tests/fake_servers.py b/turnip/pack/tests/fake_servers.py
index 1e0f310..e5951f0 100644
--- a/turnip/pack/tests/fake_servers.py
+++ b/turnip/pack/tests/fake_servers.py
@@ -11,6 +11,7 @@ from collections import defaultdict
import hashlib
from lazr.sshserver.auth import NoSuchPersonWithName
+from six.moves import xmlrpc_client
from twisted.web import xmlrpc
__all__ = [
@@ -88,4 +89,6 @@ class FakeVirtInfoService(xmlrpc.XMLRPC):
def xmlrpc_checkRefPermissions(self, path, ref_paths, auth_params):
self.ref_permissions_checks.append((path, ref_paths, auth_params))
- return self.ref_permissions
+ return [
+ (xmlrpc_client.Binary(ref), permissions)
+ for ref, permissions in self.ref_permissions.items()]
diff --git a/turnip/pack/tests/test_functional.py b/turnip/pack/tests/test_functional.py
index 2fa1782..7d80cdf 100644
--- a/turnip/pack/tests/test_functional.py
+++ b/turnip/pack/tests/test_functional.py
@@ -81,11 +81,13 @@ class FunctionalTestMixin(object):
self.virtinfo_url = b'http://localhost:%d/' % self.virtinfo_port
self.addCleanup(self.virtinfo_listener.stopListening)
self.virtinfo.ref_permissions = {
- 'refs/heads/master': ['create', 'push']}
+ b'refs/heads/master': ['create', 'push']}
def startHookRPC(self):
self.hookrpc_handler = HookRPCHandler(self.virtinfo_url)
- dir = tempfile.mkdtemp(prefix='turnip-test-hook-')
+ # XXX cjwatson 2018-11-20: Use bytes so that shutil.rmtree doesn't
+ # get confused on Python 2.
+ dir = tempfile.mkdtemp(prefix=b'turnip-test-hook-')
self.addCleanup(shutil.rmtree, dir, ignore_errors=True)
self.hookrpc_path = os.path.join(dir, 'hookrpc_sock')
@@ -94,7 +96,9 @@ class FunctionalTestMixin(object):
self.addCleanup(self.hookrpc_listener.stopListening)
def startPackBackend(self):
- self.root = tempfile.mkdtemp(prefix='turnip-test-root-')
+ # XXX cjwatson 2018-11-20: Use bytes so that shutil.rmtree doesn't
+ # get confused on Python 2.
+ self.root = tempfile.mkdtemp(prefix=b'turnip-test-root-')
self.addCleanup(shutil.rmtree, self.root, ignore_errors=True)
self.backend_listener = reactor.listenTCP(
0,
@@ -224,7 +228,7 @@ class FunctionalTestMixin(object):
@defer.inlineCallbacks
def test_no_permissions(self):
# Update the test ref_permissions
- self.virtinfo.ref_permissions = {'refs/heads/master': ['push']}
+ self.virtinfo.ref_permissions = {b'refs/heads/master': ['push']}
# Test a push fails if the user has no permissions to that ref
test_root = self.useFixture(TempDir()).path
clone1 = os.path.join(test_root, 'clone1')
@@ -248,7 +252,7 @@ class FunctionalTestMixin(object):
error)
# add create, disable push
- self.virtinfo.ref_permissions = {'refs/heads/master': ['create']}
+ self.virtinfo.ref_permissions = {b'refs/heads/master': ['create']}
# Can now create the ref
yield self.assertCommandSuccess(
(b'git', b'push', b'origin', b'master'), path=clone1)
@@ -263,10 +267,44 @@ class FunctionalTestMixin(object):
b"You do not have permission to push to refs/heads/master", error)
@defer.inlineCallbacks
+ def test_push_non_ascii_refs(self):
+ # Pushing non-ASCII refs works.
+ self.virtinfo.ref_permissions = {
+ b'refs/heads/\x80': ['create', 'push'],
+ u'refs/heads/\N{SNOWMAN}'.encode('UTF-8'): ['create', 'push'],
+ }
+ test_root = self.useFixture(TempDir()).path
+ clone1 = os.path.join(test_root, 'clone1')
+ clone2 = os.path.join(test_root, 'clone2')
+ yield self.assertCommandSuccess((b'git', b'clone', self.url, clone1))
+ yield self.assertCommandSuccess(
+ (b'git', b'config', b'user.name', b'Test User'), path=clone1)
+ yield self.assertCommandSuccess(
+ (b'git', b'config', b'user.email', b'test@xxxxxxxxxxx'),
+ path=clone1)
+ yield self.assertCommandSuccess(
+ (b'git', b'commit', b'--allow-empty', b'-m', b'Non-ASCII test'),
+ path=clone1)
+ yield self.assertCommandSuccess(
+ (b'git', b'push', b'origin', b'master:\x80',
+ u'master:\N{SNOWMAN}'.encode('UTF-8')), path=clone1)
+ # We get the new branches when we re-clone.
+ yield self.assertCommandSuccess((b'git', b'clone', self.url, clone2))
+ out = yield self.assertCommandSuccess(
+ (b'git', b'for-each-ref', b'--format=%(refname)',
+ b'refs/remotes/origin/*'),
+ path=clone2)
+ self.assertEqual(
+ sorted([
+ b'refs/remotes/origin/\x80',
+ u'refs/remotes/origin/\N{SNOWMAN}'.encode('UTF-8')]),
+ sorted(out.splitlines()))
+
+ @defer.inlineCallbacks
def test_force_push(self):
# Update the test ref_permissions
self.virtinfo.ref_permissions = {
- 'refs/heads/master': ['create', 'push']}
+ b'refs/heads/master': ['create', 'push']}
# Test a force-push fails if the user has no permissions
test_root = self.useFixture(TempDir()).path
@@ -347,7 +385,7 @@ class FunctionalTestMixin(object):
@defer.inlineCallbacks
def test_delete_ref(self):
self.virtinfo.ref_permissions = {
- 'refs/heads/newref': ['create', 'push', 'force_push']}
+ b'refs/heads/newref': ['create', 'push', 'force_push']}
test_root = self.useFixture(TempDir()).path
clone1 = os.path.join(test_root, 'clone1')
@@ -383,7 +421,7 @@ class FunctionalTestMixin(object):
@defer.inlineCallbacks
def test_delete_ref_without_permission(self):
self.virtinfo.ref_permissions = {
- 'refs/heads/newref': ['create', 'push']}
+ b'refs/heads/newref': ['create', 'push']}
test_root = self.useFixture(TempDir()).path
clone1 = os.path.join(test_root, 'clone1')
@@ -471,7 +509,7 @@ class FrontendFunctionalTestMixin(FunctionalTestMixin):
b'localhost', self.backend_port, self.virtinfo_url))
self.virt_port = self.virt_listener.getHost().port
self.virtinfo.ref_permissions = {
- 'refs/heads/master': ['create', 'push']}
+ b'refs/heads/master': ['create', 'push']}
@defer.inlineCallbacks
def tearDown(self):
@@ -482,7 +520,7 @@ class FrontendFunctionalTestMixin(FunctionalTestMixin):
@defer.inlineCallbacks
def test_read_only(self):
self.virtinfo.ref_permissions = {
- 'refs/heads/master': ['create', 'push']}
+ b'refs/heads/master': ['create', 'push']}
test_root = self.useFixture(TempDir()).path
clone1 = os.path.join(test_root, 'clone1')
clone2 = os.path.join(test_root, 'clone2')
diff --git a/turnip/pack/tests/test_hookrpc.py b/turnip/pack/tests/test_hookrpc.py
index 9af0408..5109ecd 100644
--- a/turnip/pack/tests/test_hookrpc.py
+++ b/turnip/pack/tests/test_hookrpc.py
@@ -7,11 +7,20 @@ from __future__ import (
unicode_literals,
)
+import base64
import contextlib
import uuid
+from six.moves import xmlrpc_client
from testtools import TestCase
from testtools.deferredruntest import AsynchronousDeferredRunTest
+from testtools.matchers import (
+ Equals,
+ IsInstance,
+ MatchesAll,
+ MatchesListwise,
+ MatchesStructure,
+ )
from twisted.internet import (
defer,
reactor,
@@ -164,22 +173,39 @@ class TestHookRPCHandler(TestCase):
finally:
self.hookrpc_handler.unregisterKey(key)
+ def encodeRefPath(self, ref_path):
+ return base64.b64encode(ref_path).decode('UTF-8')
+
def assertCheckedRefPermissions(self, path, ref_paths, auth_params):
- self.assertEqual(
- [(path, ref_paths, auth_params)],
- self.virtinfo.ref_permissions_checks)
+ self.assertThat(self.virtinfo.ref_permissions_checks, MatchesListwise([
+ MatchesListwise([
+ Equals(path),
+ MatchesListwise([
+ MatchesAll(
+ IsInstance(xmlrpc_client.Binary),
+ MatchesStructure.byEquality(data=ref_path))
+ for ref_path in ref_paths
+ ]),
+ Equals(auth_params),
+ ]),
+ ]))
@defer.inlineCallbacks
def test_checkRefPermissions_fresh(self):
self.virtinfo.ref_permissions = {
- 'refs/heads/master': ['push'],
- 'refs/heads/next': ['force_push'],
+ b'refs/heads/master': ['push'],
+ b'refs/heads/next': ['force_push'],
}
+ encoded_paths = [
+ self.encodeRefPath(ref_path)
+ for ref_path in sorted(self.virtinfo.ref_permissions)]
with self.registeredKey('/translated', auth_params={'uid': 42}) as key:
permissions = yield self.hookrpc_handler.checkRefPermissions(
- None,
- {'key': key, 'paths': sorted(self.virtinfo.ref_permissions)})
- self.assertEqual(self.virtinfo.ref_permissions, permissions)
+ None, {'key': key, 'paths': encoded_paths})
+ expected_permissions = {
+ self.encodeRefPath(ref_path): perms
+ for ref_path, perms in self.virtinfo.ref_permissions.items()}
+ self.assertEqual(expected_permissions, permissions)
self.assertCheckedRefPermissions(
'/translated', [b'refs/heads/master', b'refs/heads/next'],
{'uid': 42})
@@ -187,15 +213,16 @@ class TestHookRPCHandler(TestCase):
@defer.inlineCallbacks
def test_checkRefPermissions_cached(self):
cached_ref_permissions = {
- 'refs/heads/master': ['push'],
- 'refs/heads/next': ['force_push'],
+ b'refs/heads/master': ['push'],
+ b'refs/heads/next': ['force_push'],
}
+ encoded_master = self.encodeRefPath(b'refs/heads/master')
with self.registeredKey(
'/translated', auth_params={'uid': 42},
permissions=cached_ref_permissions) as key:
permissions = yield self.hookrpc_handler.checkRefPermissions(
- None, {'key': key, 'paths': ['refs/heads/master']})
- expected_permissions = {'refs/heads/master': ['push']}
+ None, {'key': key, 'paths': [encoded_master]})
+ expected_permissions = {encoded_master: ['push']}
self.assertEqual(expected_permissions, permissions)
self.assertEqual([], self.virtinfo.ref_permissions_checks)
diff --git a/turnip/pack/tests/test_hooks.py b/turnip/pack/tests/test_hooks.py
index ebf9c80..7aaf356 100644
--- a/turnip/pack/tests/test_hooks.py
+++ b/turnip/pack/tests/test_hooks.py
@@ -1,4 +1,5 @@
-# Copyright 2015 Canonical Ltd. This software is licensed under the
+# -*- coding: utf-8 -*-
+# Copyright 2015-2018 Canonical Ltd. This software is licensed under the
# GNU Affero General Public License version 3 (see the file LICENSE).
from __future__ import (
@@ -7,6 +8,7 @@ from __future__ import (
unicode_literals,
)
+import base64
import os.path
import uuid
@@ -61,7 +63,9 @@ class MockHookRPCHandler(hookrpc.HookRPCHandler):
self.notifications.append(self.ref_paths[args['key']])
def checkRefPermissions(self, proto, args):
- return self.ref_permissions[args['key']]
+ return {
+ base64.b64encode(ref).decode('UTF-8'): permissions
+ for ref, permissions in self.ref_permissions[args['key']].items()}
class MockRef(object):
@@ -147,7 +151,15 @@ class TestPreReceiveHook(HookTestMixin, TestCase):
# A single valid ref is accepted.
yield self.assertAccepted(
[(b'refs/heads/master', self.old_sha1, self.new_sha1)],
- {'refs/heads/master': ['push']})
+ {b'refs/heads/master': ['push']})
+
+ @defer.inlineCallbacks
+ def test_accepted_non_ascii(self):
+ # Valid non-ASCII refs are accepted.
+ paths = [b'refs/heads/\x80', u'refs/heads/géag'.encode('UTF-8')]
+ yield self.assertAccepted(
+ [(path, self.old_sha1, self.new_sha1) for path in paths],
+ {path: ['push'] for path in paths})
@defer.inlineCallbacks
def test_rejected(self):
@@ -171,6 +183,16 @@ class TestPreReceiveHook(HookTestMixin, TestCase):
b"You do not have permission to push "
b"to refs/heads/super-verboten.\n")
+ @defer.inlineCallbacks
+ def test_rejected_non_ascii(self):
+ # Invalid non-ASCII refs are rejected.
+ paths = [b'refs/heads/\x80', u'refs/heads/géag'.encode('UTF-8')]
+ yield self.assertRejected(
+ [(path, self.old_sha1, self.new_sha1) for path in paths],
+ {path: [] for path in paths},
+ b"You do not have permission to push to refs/heads/\x80.\n"
+ b"You do not have permission to push to refs/heads/g\xc3\xa9ag.\n")
+
class TestPostReceiveHook(HookTestMixin, TestCase):
"""Tests for the git post-receive hook."""
@@ -212,12 +234,12 @@ class TestUpdateHook(TestCase):
# Creation is determined by an all 0 base sha
self.assertEqual(
[], hook.match_update_rules(
- [], ['ref', pygit2.GIT_OID_HEX_ZERO, 'new']))
+ {}, [b'ref', pygit2.GIT_OID_HEX_ZERO, 'new']))
def test_fast_forward(self):
# If the old sha is a merge ancestor of the new
self.assertEqual(
- [], hook.match_update_rules([], ['ref', 'somehex', 'new']))
+ [], hook.match_update_rules({}, ['ref', 'somehex', 'new']))
def test_rules_fall_through(self):
# The default is to deny
@@ -229,10 +251,27 @@ class TestUpdateHook(TestCase):
# No matches means deny by default
output = hook.match_update_rules(
{'notamatch': []},
- ['ref', 'old', 'new'])
+ [b'ref', 'old', 'new'])
self.assertEqual(
[b'You do not have permission to force-push to ref.'], output)
+ def test_no_matching_non_utf8_ref(self):
+ # An unmatched non-UTF-8 ref is denied.
+ output = hook.match_update_rules(
+ {}, [b'refs/heads/\x80', 'old', 'new'])
+ self.assertEqual(
+ [b'You do not have permission to force-push to refs/heads/\x80.'],
+ output)
+
+ def test_no_matching_utf8_ref(self):
+ # An unmatched UTF-8 ref is denied.
+ output = hook.match_update_rules(
+ {}, [u'refs/heads/géag'.encode('UTF-8'), 'old', 'new'])
+ self.assertEqual(
+ [b'You do not have permission to force-push to '
+ b'refs/heads/g\xc3\xa9ag.'],
+ output)
+
def test_matching_ref(self):
# Permission given to force-push
output = hook.match_update_rules(
@@ -240,6 +279,20 @@ class TestUpdateHook(TestCase):
['ref', 'old', 'new'])
self.assertEqual([], output)
+ def test_matching_non_utf8_ref(self):
+ # A non-UTF-8 ref with force-push permission is accepted.
+ output = hook.match_update_rules(
+ {b'refs/heads/\x80': ['force_push']},
+ [b'refs/heads/\x80', 'old', 'new'])
+ self.assertEqual([], output)
+
+ def test_matching_utf8_ref(self):
+ # A UTF-8 ref with force-push permission is accepted.
+ output = hook.match_update_rules(
+ {u'refs/heads/géag'.encode('UTF-8'): ['force_push']},
+ [u'refs/heads/géag'.encode('UTF-8'), 'old', 'new'])
+ self.assertEqual([], output)
+
def test_no_permission(self):
# User does not have permission to force-push
output = hook.match_update_rules(
Follow ups
