launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging ~cjwatson/turnip:more-flexible-auth into turnip:master.

Commit message:
Make HTTPS authentication more flexible

Expect authenticateWithPassword to return a parameters dict rather than
a username/uid tuple, and pass that whole dictionary through to
translatePath using its new API.  This makes it possible to authenticate
using a macaroon and have the macaroon passed through to translatePath
for additional per-repository authorisation.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~cjwatson/turnip/+git/turnip/+merge/307938
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/turnip:more-flexible-auth into turnip:master.

diff --git a/turnip/pack/git.py b/turnip/pack/git.py
index 0c15b30..04d6770 100644
--- a/turnip/pack/git.py
+++ b/turnip/pack/git.py
@@ -457,14 +457,19 @@ class PackVirtServerProtocol(PackProxyServerProtocol):
         permission = b'read' if command == b'git-upload-pack' else b'write'
         proxy = xmlrpc.Proxy(self.factory.virtinfo_endpoint, allowNone=True)
         try:
-            can_authenticate = (
-                params.get(b'turnip-can-authenticate') == b'yes')
-            auth_uid = params.get(b'turnip-authenticated-uid')
+            auth_params = {}
+            for key, value in params.items():
+                if key.startswith(b'turnip-authenticated-'):
+                    decoded_key = key[len(b'turnip-authenticated-'):].decode(
+                        'utf-8')
+                    auth_params[decoded_key] = value
+            if 'uid' in auth_params:
+                auth_params['uid'] = int(auth_params['uid'])
+            if params.get(b'turnip-can-authenticate') == b'yes':
+                auth_params['can-authenticate'] = True
             self.log.info("Translating request.")
             translated = yield proxy.callRemote(
-                b'translatePath', pathname, permission,
-                int(auth_uid) if auth_uid is not None else None,
-                can_authenticate)
+                b'translatePath', pathname, permission, auth_params)
             self.log.info(
                 "Translation result: {translated}", translated=translated)
             if 'trailing' in translated and translated['trailing']:
diff --git a/turnip/pack/http.py b/turnip/pack/http.py
index a2d6d97..7f654fc 100644
--- a/turnip/pack/http.py
+++ b/turnip/pack/http.py
@@ -250,28 +250,27 @@ class BaseSmartHTTPResource(resource.Resource):
     @defer.inlineCallbacks
     def authenticateUser(self, request):
         """Attempt authentication of the request with the virt service."""
-        if request.getUser():
-            user, uid = yield self.root.authenticateWithPassword(
+        if request.getUser() or request.getPassword():
+            params = yield self.root.authenticateWithPassword(
                 request.getUser(), request.getPassword())
-            defer.returnValue((user, uid))
-        defer.returnValue((None, None))
+            defer.returnValue(params)
+        defer.returnValue({})
 
     @defer.inlineCallbacks
     def connectToBackend(self, factory, service, path, content, request):
         """Establish a pack connection to the backend.
 
-        The turnip-authenticated-user parameter is set to the username
-        returned by the virt service, if any.
+        The turnip-authenticated-* parameters are set to the values returned
+        by the virt service, if any.
         """
         params = {
             b'turnip-can-authenticate': b'yes',
             b'turnip-request-id': str(uuid.uuid4()),
             }
-        authenticated_user, authenticated_uid = yield self.authenticateUser(
-            request)
-        if authenticated_user:
-            params[b'turnip-authenticated-user'] = authenticated_user
-            params[b'turnip-authenticated-uid'] = str(authenticated_uid)
+        authenticated_params = yield self.authenticateUser(request)
+        for key, value in authenticated_params.items():
+            encoded_key = ('turnip-authenticated-' + key).encode('utf-8')
+            params[encoded_key] = str(value)
         params.update(self.extra_params)
         d = defer.Deferred()
         client_factory = factory(service, path, params, content, request, d)
@@ -759,4 +758,4 @@ class SmartHTTPFrontendResource(resource.Resource):
                 defer.returnValue((None, None))
             else:
                 raise
-        defer.returnValue((translated['user'], translated['uid']))
+        defer.returnValue(translated)
diff --git a/turnip/pack/tests/test_functional.py b/turnip/pack/tests/test_functional.py
index 9ded8ff..e30b59f 100644
--- a/turnip/pack/tests/test_functional.py
+++ b/turnip/pack/tests/test_functional.py
@@ -14,6 +14,16 @@ import os
 import shutil
 import stat
 import tempfile
+try:
+    from urllib.parse import (
+        urlsplit,
+        urlunsplit,
+        )
+except ImportError:
+    from urlparse import (
+        urlsplit,
+        urlunsplit,
+        )
 
 from fixtures import (
     EnvironmentVariable,
@@ -91,10 +101,16 @@ class FakeVirtInfoService(xmlrpc.XMLRPC):
 
     def __init__(self, *args, **kwargs):
         xmlrpc.XMLRPC.__init__(self, *args, **kwargs)
+        self.require_auth = False
+        self.translations = []
+        self.authentications = []
         self.push_notifications = []
 
-    def xmlrpc_translatePath(self, pathname, permission, authenticated_uid,
-                             can_authenticate):
+    def xmlrpc_translatePath(self, pathname, permission, auth_params):
+        if self.require_auth and 'user' not in auth_params:
+            raise xmlrpc.Fault(3, "Unauthorized")
+
+        self.translations.append((pathname, permission, auth_params))
         writable = False
         if pathname.startswith('/+rw'):
             writable = True
@@ -105,6 +121,7 @@ class FakeVirtInfoService(xmlrpc.XMLRPC):
         return {'path': hashlib.sha256(pathname).hexdigest()}
 
     def xmlrpc_authenticateWithPassword(self, username, password):
+        self.authentications.append((username, password))
         return {'user': username}
 
     def xmlrpc_notify(self, path):
@@ -241,9 +258,11 @@ class FunctionalTestMixin(object):
     @defer.inlineCallbacks
     def test_no_repo(self):
         test_root = self.useFixture(TempDir()).path
+        parsed_url = list(urlsplit(self.url))
+        parsed_url[2] = b'/fail'
+        fail_url = urlunsplit(parsed_url)
         output = yield utils.getProcessOutput(
-            b'git',
-            (b'clone', b'%s://localhost:%d/fail' % (self.scheme, self.port)),
+            b'git', (b'clone', fail_url),
             env=os.environ, path=test_root, errortoo=True)
         self.assertIn(
             b"Cloning into 'fail'...\n" + self.early_error + b'fatal: ',
@@ -350,8 +369,7 @@ class FrontendFunctionalTestMixin(FunctionalTestMixin):
 
     @defer.inlineCallbacks
     def test_unicode_fault(self):
-        def fake_translatePath(pathname, permission, authenticated_uid,
-                               can_authenticate):
+        def fake_translatePath(pathname, permission, auth_params):
             raise xmlrpc.Fault(2, u"홍길동 is not a member of Project Team.")
 
         test_root = self.useFixture(TempDir()).path
@@ -433,6 +451,32 @@ class TestSmartHTTPFrontendFunctional(FrontendFunctionalTestMixin, TestCase):
             factory.response_headers[b'x-turnip-revision'])
 
 
+class TestSmartHTTPFrontendWithAuthFunctional(TestSmartHTTPFrontendFunctional):
+
+    @defer.inlineCallbacks
+    def setUp(self):
+        yield super(TestSmartHTTPFrontendWithAuthFunctional, self).setUp()
+
+        self.virtinfo.require_auth = True
+        self.url = (
+            b'http://test-user:test-password@localhost:%d/+rw/test' %
+            self.port)
+        self.ro_url = (
+            b'http://test-user:test-password@localhost:%d/test' % self.port)
+
+    @defer.inlineCallbacks
+    def test_authenticated(self):
+        test_root = self.useFixture(TempDir()).path
+        clone = os.path.join(test_root, 'clone')
+        yield self.assertCommandSuccess((b'git', b'clone', self.ro_url, clone))
+        self.assertEqual(
+            [(b'test-user', b'test-password')], self.virtinfo.authentications)
+        self.assertEqual(
+            [(b'/test', b'read',
+              {b'can-authenticate': True, b'user': b'test-user'})],
+            self.virtinfo.translations)
+
+
 class TestSmartSSHServiceFunctional(FrontendFunctionalTestMixin, TestCase):
 
     scheme = b'ssh'
diff --git a/turnip/pack/tests/test_http.py b/turnip/pack/tests/test_http.py
index bc3bf10..8e0418f 100644
--- a/turnip/pack/tests/test_http.py
+++ b/turnip/pack/tests/test_http.py
@@ -71,7 +71,7 @@ class FakeRoot(object):
         self.backend_connected = defer.Deferred()
 
     def authenticateWithPassword(self, user, password):
-        return None, None
+        return {}
 
     def connectToBackend(self, client_factory):
         self.backend_transport = (