Merge ~cjwatson/turnip:set-symbolic-ref into turnip:master

[Colin Watson]

Add turnip-set-symbolic-ref extension service

This makes it possible to set a repository's HEAD by talking directly to
turnip over HTTPS, which is needed for git-to-git code imports.

LP: #1469459

[William Grant (wgrant)]

review: Approve (code)

[Colin Watson (cjwatson)]

All fixed, thanks.

[Otto Co-Pilot (otto-copilot)]

I tried to merge it but there are some problems. Typically you want to merge or rebase and try again.

review: Needs Fixing

diff --git a/README b/README
index 7b26510..4ebc443 100644
--- a/README
+++ b/README
@@ -68,6 +68,22 @@ The only additional parameters implemented today are
 'turnip-stateless-rpc' and 'turnip-advertise-refs', which are used by
 the smart HTTP server to proxy to the standard pack protocol.
 
+turnip implements one externally-visible extension: a
+'turnip-set-symbolic-ref' service that sets a symbolic ref (currently only
+'HEAD' is permitted) to a given target. This may be used over the various
+protocols (git, SSH, smart HTTP), requesting the service in the same way as
+the existing 'git-upload-pack' and 'git-receive-pack' services.
+
+   turnip-set-symbolic-ref-request = set-symbolic-ref-line
+                                     flush-pkt
+   set-symbolic-ref-line           = PKT-LINE(refname SP refname)
+
+The server replies with an ACK indicating the symbolic ref name that was
+changed, or an error message.
+
+   turnip-set-symbolic-ref-response = set-symbolic-ref-ack / error-line
+   set-symbolic-ref-ack             = PKT-LINE("ACK" SP refname)
+
 
 Development
 ===========
diff --git a/turnip/pack/git.py b/turnip/pack/git.py
index 0e5d928..93bc91b 100644
--- a/turnip/pack/git.py
+++ b/turnip/pack/git.py
@@ -278,7 +278,7 @@ class GitProcessProtocol(protocol.ProcessProtocol):
                 'git exited {code} with no output; synthesising an error',
                 code=code)
             self.peer.sendPacket(ERROR_PREFIX + 'backend exited %d' % code)
-        self.peer.transport.loseConnection()
+        self.peer.processEnded(reason)
 
     def pauseProducing(self):
         self.transport.pauseProducing()
@@ -392,43 +392,117 @@ class PackBackendProtocol(PackServerProtocol):
     """
 
     hookrpc_key = None
+    expect_set_symbolic_ref = False
 
     def requestReceived(self, command, raw_pathname, params):
         self.extractRequestMeta(command, raw_pathname, params)
+        self.command = command
+        self.raw_pathname = raw_pathname
+        self.path = compose_path(self.factory.root, self.raw_pathname)
+
+        if command == b'turnip-set-symbolic-ref':
+            self.expect_set_symbolic_ref = True
+            self.resumeProducing()
+            return
 
-        path = compose_path(self.factory.root, raw_pathname)
+        write_operation = False
         if command == b'git-upload-pack':
             subcmd = b'upload-pack'
         elif command == b'git-receive-pack':
             subcmd = b'receive-pack'
+            write_operation = True
         else:
             self.die(b'Unsupported command in request')
             return
 
-        cmd = b'git'
-        args = [b'git', subcmd]
+        args = []
         if params.pop(b'turnip-stateless-rpc', None):
             args.append(b'--stateless-rpc')
         if params.pop(b'turnip-advertise-refs', None):
             args.append(b'--advertise-refs')
-        args.append(path)
+        args.append(self.path)
+        self.spawnGit(subcmd, args, write_operation=write_operation)
+
+    def spawnGit(self, subcmd, extra_args, write_operation=False,
+                 send_path_as_option=False):
+        cmd = b'git'
+        args = [b'git']
+        if send_path_as_option:
+            args.extend([b'-C', self.path])
+        args.append(subcmd)
+        args.extend(extra_args)
 
         env = {}
-        if subcmd == b'receive-pack' and self.factory.hookrpc_handler:
+        if write_operation and self.factory.hookrpc_handler:
             # This is a write operation, so prepare config, hooks, the hook
             # RPC server, and the environment variables that link them up.
-            ensure_config(path)
+            ensure_config(self.path)
             self.hookrpc_key = str(uuid.uuid4())
             self.factory.hookrpc_handler.registerKey(
-                self.hookrpc_key, raw_pathname, [])
-            ensure_hooks(path)
+                self.hookrpc_key, self.raw_pathname, [])
+            ensure_hooks(self.path)
             env[b'TURNIP_HOOK_RPC_SOCK'] = self.factory.hookrpc_sock
             env[b'TURNIP_HOOK_RPC_KEY'] = self.hookrpc_key
 
         self.log.info('Spawning {args}', args=args)
         self.peer = GitProcessProtocol(self)
+        self.spawnProcess(cmd, args, env=env)
+
+    def spawnProcess(self, cmd, args, env=None):
         reactor.spawnProcess(self.peer, cmd, args, env=env)
 
+    def packetReceived(self, data):
+        if self.expect_set_symbolic_ref:
+            if data is None:
+                self.die(b'Bad request: flush-pkt instead')
+                return
+            self.pauseProducing()
+            self.expect_set_symbolic_ref = False
+            if b' ' not in data:
+                self.die(b'Invalid set-symbolic-ref-line')
+                return
+            name, target = data.split(b' ', 1)
+            # Be careful about extending this to anything other than HEAD.
+            # We use "git symbolic-ref" because it gives us locking and
+            # logging, but it doesn't prevent writing a ref to ../something.
+            # Fortunately it does at least refuse to point HEAD outside of
+            # refs/.
+            if name != b'HEAD':
+                self.die(b'Symbolic ref name must be "HEAD"')
+                return
+            if target.startswith(b'-'):
+                self.die(b'Symbolic ref target may not start with "-"')
+                return
+            elif b' ' in target:
+                self.die(b'Symbolic ref target may not contain " "')
+                return
+            self.symbolic_ref_name = name
+            self.spawnGit(
+                b'symbolic-ref', [name, target], write_operation=True,
+                send_path_as_option=True)
+            return
+
+        PackServerProtocol.packetReceived(self, data)
+
+    @defer.inlineCallbacks
+    def processEnded(self, reason):
+        message = None
+        if self.command == b'turnip-set-symbolic-ref':
+            if reason.check(error.ProcessDone):
+                try:
+                    yield self.factory.hookrpc_handler.notify(self.path)
+                    self.sendPacket(b'ACK %s\n' % self.symbolic_ref_name)
+                except Exception as e:
+                    message = str(e)
+            else:
+                message = (
+                    'git symbolic-ref exited with status %d' %
+                    reason.value.exitCode)
+        if message is None:
+            self.transport.loseConnection()
+        else:
+            self.die(message)
+
     def readConnectionLost(self):
         # Forward the closed stdin down the stack.
         if self.peer is not None:
diff --git a/turnip/pack/hookrpc.py b/turnip/pack/hookrpc.py
index d273a9c..bd3bc0e 100644
--- a/turnip/pack/hookrpc.py
+++ b/turnip/pack/hookrpc.py
@@ -126,11 +126,15 @@ class HookRPCHandler(object):
         return [rule.decode('utf-8') for rule in self.ref_rules[args['key']]]
 
     @defer.inlineCallbacks
+    def notify(self, path):
+        proxy = xmlrpc.Proxy(self.virtinfo_url, allowNone=True)
+        yield proxy.callRemote(b'notify', path)
+
+    @defer.inlineCallbacks
     def notifyPush(self, proto, args):
         """Notify the virtinfo service about a push."""
         path = self.ref_paths[args['key']]
-        proxy = xmlrpc.Proxy(self.virtinfo_url, allowNone=True)
-        yield proxy.callRemote(b'notify', path)
+        yield self.notify(path)
 
 
 class HookRPCServerFactory(RPCServerFactory):
diff --git a/turnip/pack/http.py b/turnip/pack/http.py
index 5e5f87e..42fed27 100644
--- a/turnip/pack/http.py
+++ b/turnip/pack/http.py
@@ -680,7 +680,8 @@ class HTTPAuthResource(resource.Resource):
 class SmartHTTPFrontendResource(resource.Resource):
     """HTTP resource to translate Git smart HTTP requests to pack protocol."""
 
-    allowed_services = frozenset((b'git-upload-pack', b'git-receive-pack'))
+    allowed_services = frozenset((
+        b'git-upload-pack', b'git-receive-pack', b'turnip-set-symbolic-ref'))
 
     def __init__(self, backend_host, config):
         resource.Resource.__init__(self)
@@ -737,7 +738,9 @@ class SmartHTTPFrontendResource(resource.Resource):
         content_type = request.getHeader(b'Content-Type')
         if content_type is None:
             return False
-        return content_type.startswith(b'application/x-git-')
+        return (
+            content_type.startswith(b'application/x-git-') or
+            content_type.startswith(b'application/x-turnip-'))
 
     def getChild(self, path, request):
         if self._isGitRequest(request):
diff --git a/turnip/pack/ssh.py b/turnip/pack/ssh.py
index 006331d..b753220 100644
--- a/turnip/pack/ssh.py
+++ b/turnip/pack/ssh.py
@@ -120,7 +120,8 @@ class SSHPackClientFactory(protocol.ClientFactory):
 class SmartSSHSession(DoNothingSession):
     """SSH session allowing only Git smart SSH requests."""
 
-    allowed_services = frozenset((b'git-upload-pack', b'git-receive-pack'))
+    allowed_services = frozenset((
+        b'git-upload-pack', b'git-receive-pack', b'turnip-set-symbolic-ref'))
 
     def __init__(self, *args, **kwargs):
         super(SmartSSHSession, self).__init__(*args, **kwargs)
diff --git a/turnip/pack/tests/test_functional.py b/turnip/pack/tests/test_functional.py
index 61fd5c4..e6dbd20 100644
--- a/turnip/pack/tests/test_functional.py
+++ b/turnip/pack/tests/test_functional.py
@@ -8,8 +8,10 @@ from __future__ import (
     unicode_literals,
     )
 
+import base64
 from collections import defaultdict
 import hashlib
+import io
 import os
 import random
 import shutil
@@ -33,10 +35,7 @@ from fixtures import (
 from lazr.sshserver.auth import NoSuchPersonWithName
 from testtools import TestCase
 from testtools.content import text_content
-from testtools.deferredruntest import (
-    assert_fails_with,
-    AsynchronousDeferredRunTest,
-    )
+from testtools.deferredruntest import AsynchronousDeferredRunTest
 from testtools.matchers import StartsWith
 from twisted.internet import (
     defer,
@@ -45,11 +44,12 @@ from twisted.internet import (
     )
 from twisted.web import (
     client,
-    error,
+    http_headers,
     server,
     xmlrpc,
     )
 
+from turnip.pack import helpers
 from turnip.pack.git import (
     PackBackendFactory,
     PackFrontendFactory,
@@ -480,14 +480,68 @@ class TestSmartHTTPFrontendFunctional(FrontendFunctionalTestMixin, TestCase):
 
     @defer.inlineCallbacks
     def test_root_revision_header(self):
-        factory = client.HTTPClientFactory(
-            b'http://localhost:%d/' % self.port, method=b'HEAD',
-            followRedirect=False)
-        reactor.connectTCP(b'localhost', self.port, factory)
-        yield assert_fails_with(factory.deferred, error.PageRedirect)
+        response = yield client.Agent(reactor).request(
+            b'HEAD', b'http://localhost:%d/' % self.port)
+        self.assertEqual(302, response.code)
         self.assertEqual(
             [version_info['revision_id']],
-            factory.response_headers[b'x-turnip-revision'])
+            response.headers.getRawHeaders(b'X-Turnip-Revision'))
+
+    def make_set_symbolic_ref_request(self, line):
+        parsed_url = urlsplit(self.url)
+        url = urlunsplit([
+            parsed_url.scheme,
+            b'%s:%d' % (parsed_url.hostname, parsed_url.port),
+            parsed_url.path + b'/turnip-set-symbolic-ref', b'', b''])
+        headers = {
+            b'Content-Type': [
+                b'application/x-turnip-set-symbolic-ref-request',
+                ],
+            }
+        if parsed_url.username:
+            headers[b'Authorization'] = [
+                b'Basic ' + base64.b64encode(
+                    b'%s:%s' % (parsed_url.username, parsed_url.password)),
+                ]
+        data = helpers.encode_packet(line) + helpers.encode_packet(None)
+        return client.Agent(reactor).request(
+            b'POST', url, headers=http_headers.Headers(headers),
+            bodyProducer=client.FileBodyProducer(io.BytesIO(data)))
+
+    @defer.inlineCallbacks
+    def get_symbolic_ref(self, path, name):
+        out = yield utils.getProcessOutput(
+            b'git', (b'symbolic-ref', name), env=os.environ, path=path)
+        defer.returnValue(out.rstrip(b'\n'))
+
+    @defer.inlineCallbacks
+    def test_turnip_set_symbolic_ref(self):
+        repo = os.path.join(self.root, self.internal_name)
+        head_target = yield self.get_symbolic_ref(repo, b'HEAD')
+        self.assertEqual(b'refs/heads/master', head_target)
+        response = yield self.make_set_symbolic_ref_request(
+            b'HEAD refs/heads/new-head')
+        self.assertEqual(200, response.code)
+        body = yield client.readBody(response)
+        self.assertEqual((b'ACK HEAD\n', ''), helpers.decode_packet(body))
+        head_target = yield self.get_symbolic_ref(repo, b'HEAD')
+        self.assertEqual(b'refs/heads/new-head', head_target)
+
+    @defer.inlineCallbacks
+    def test_turnip_set_symbolic_ref_error(self):
+        repo = os.path.join(self.root, self.internal_name)
+        head_target = yield self.get_symbolic_ref(repo, b'HEAD')
+        self.assertEqual(b'refs/heads/master', head_target)
+        response = yield self.make_set_symbolic_ref_request(b'HEAD --evil')
+        # This is a little weird since an error occurred, but it's
+        # consistent with other HTTP pack protocol responses.
+        self.assertEqual(200, response.code)
+        body = yield client.readBody(response)
+        self.assertEqual(
+            (b'ERR Symbolic ref target may not start with "-"\n', ''),
+            helpers.decode_packet(body))
+        head_target = yield self.get_symbolic_ref(repo, b'HEAD')
+        self.assertEqual(b'refs/heads/master', head_target)
 
 
 class TestSmartHTTPFrontendWithAuthFunctional(TestSmartHTTPFrontendFunctional):
diff --git a/turnip/pack/tests/test_git.py b/turnip/pack/tests/test_git.py
index d19574d..4153234 100644
--- a/turnip/pack/tests/test_git.py
+++ b/turnip/pack/tests/test_git.py
@@ -7,13 +7,23 @@ from __future__ import (
     unicode_literals,
     )
 
+import os.path
+
+from fixtures import TempDir
+from pygit2 import init_repository
 from testtools import TestCase
+from testtools.matchers import (
+    ContainsDict,
+    Equals,
+    MatchesListwise,
+    )
 from twisted.test import proto_helpers
 
 from turnip.pack import (
     git,
     helpers,
     )
+from turnip.pack.tests.test_hooks import MockHookRPCHandler
 
 
 class DummyPackServerProtocol(git.PackServerProtocol):
@@ -87,3 +97,111 @@ class TestPackServerProtocol(TestCase):
         # dropped.
         self.proto.dataReceived(b'0000')
         self.assertKilledWith(b'Bad request: flush-pkt instead')
+
+
+class DummyPackBackendProtocol(git.PackBackendProtocol):
+
+    test_process = None
+
+    def spawnProcess(self, cmd, args, env=None):
+        if self.test_process is not None:
+            raise AssertionError('Process already spawned.')
+        self.test_process = (cmd, args, env)
+
+
+class TestPackBackendProtocol(TestCase):
+    """Test the Git pack backend protocol."""
+
+    def setUp(self):
+        super(TestPackBackendProtocol, self).setUp()
+        self.root = self.useFixture(TempDir()).path
+        self.hookrpc_handler = MockHookRPCHandler()
+        self.hookrpc_sock = os.path.join(self.root, 'hookrpc_sock')
+        self.factory = git.PackBackendFactory(
+            self.root, self.hookrpc_handler, self.hookrpc_sock)
+        self.proto = DummyPackBackendProtocol()
+        self.proto.factory = self.factory
+        self.transport = proto_helpers.StringTransportWithDisconnection()
+        self.transport.protocol = self.proto
+        self.proto.makeConnection(self.transport)
+
+    def assertKilledWith(self, message):
+        self.assertFalse(self.transport.connected)
+        self.assertEqual(
+            (b'ERR ' + message + b'\n', b''),
+            helpers.decode_packet(self.transport.value()))
+
+    def test_git_upload_pack_calls_spawnProcess(self):
+        # If the command is git-upload-pack, requestReceived calls
+        # spawnProcess with appropriate arguments.
+        self.proto.requestReceived(
+            b'git-upload-pack', b'/foo.git', {b'host': b'example.com'})
+        self.assertEqual(
+            (b'git',
+             [b'git', b'upload-pack', os.path.join(self.root, b'foo.git')],
+             {}),
+            self.proto.test_process)
+
+    def test_git_receive_pack_calls_spawnProcess(self):
+        # If the command is git-receive-pack, requestReceived calls
+        # spawnProcess with appropriate arguments.
+        repo_dir = os.path.join(self.root, 'foo.git')
+        init_repository(repo_dir, bare=True)
+        self.proto.requestReceived(
+            b'git-receive-pack', b'/foo.git', {b'host': b'example.com'})
+        self.assertThat(
+            self.proto.test_process, MatchesListwise([
+                Equals(b'git'),
+                Equals([b'git', b'receive-pack', repo_dir.encode('utf-8')]),
+                ContainsDict(
+                    {b'TURNIP_HOOK_RPC_SOCK': Equals(self.hookrpc_sock)})]))
+
+    def test_turnip_set_symbolic_ref_calls_spawnProcess(self):
+        # If the command is turnip-set-symbolic-ref, requestReceived does
+        # not spawn a process, but packetReceived calls spawnProcess with
+        # appropriate arguments.
+        repo_dir = os.path.join(self.root, 'foo.git')
+        init_repository(repo_dir, bare=True)
+        self.proto.requestReceived(b'turnip-set-symbolic-ref', b'/foo.git', {})
+        self.assertIsNone(self.proto.test_process)
+        self.proto.packetReceived(b'HEAD refs/heads/master')
+        self.assertThat(
+            self.proto.test_process, MatchesListwise([
+                Equals(b'git'),
+                Equals([
+                    b'git', b'-C', repo_dir.encode('utf-8'), b'symbolic-ref',
+                    b'HEAD', b'refs/heads/master']),
+                ContainsDict(
+                    {b'TURNIP_HOOK_RPC_SOCK': Equals(self.hookrpc_sock)})]))
+
+    def test_turnip_set_symbolic_ref_requires_valid_line(self):
+        # The turnip-set-symbolic-ref command requires a valid
+        # set-symbolic-ref-line packet.
+        self.proto.requestReceived(b'turnip-set-symbolic-ref', b'/foo.git', {})
+        self.assertIsNone(self.proto.test_process)
+        self.proto.packetReceived(b'HEAD')
+        self.assertKilledWith(b'Invalid set-symbolic-ref-line')
+
+    def test_turnip_set_symbolic_ref_name_must_be_HEAD(self):
+        # The turnip-set-symbolic-ref command's "name" parameter must be
+        # "HEAD".
+        self.proto.requestReceived(b'turnip-set-symbolic-ref', b'/foo.git', {})
+        self.assertIsNone(self.proto.test_process)
+        self.proto.packetReceived(b'another-symref refs/heads/master')
+        self.assertKilledWith(b'Symbolic ref name must be "HEAD"')
+
+    def test_turnip_set_symbolic_ref_target_not_option(self):
+        # The turnip-set-symbolic-ref command's "target" parameter may not
+        # start with "-".
+        self.proto.requestReceived(b'turnip-set-symbolic-ref', b'/foo.git', {})
+        self.assertIsNone(self.proto.test_process)
+        self.proto.packetReceived(b'HEAD --evil')
+        self.assertKilledWith(b'Symbolic ref target may not start with "-"')
+
+    def test_turnip_set_symbolic_ref_target_no_space(self):
+        # The turnip-set-symbolic-ref command's "target" parameter may not
+        # contain " ".
+        self.proto.requestReceived(b'turnip-set-symbolic-ref', b'/foo.git', {})
+        self.assertIsNone(self.proto.test_process)
+        self.proto.packetReceived(b'HEAD evil lies')
+        self.assertKilledWith(b'Symbolic ref target may not contain " "')
diff --git a/turnip/pack/tests/test_http.py b/turnip/pack/tests/test_http.py
index 45cafad..6e16b77 100644
--- a/turnip/pack/tests/test_http.py
+++ b/turnip/pack/tests/test_http.py
@@ -64,7 +64,8 @@ def render_resource(resource, request):
 
 class FakeRoot(object):
 
-    allowed_services = frozenset((b'git-upload-pack', b'git-receive-pack'))
+    allowed_services = frozenset((
+        b'git-upload-pack', b'git-receive-pack', b'turnip-set-symbolic-ref'))
 
     def __init__(self):
         self.backend_transport = None