Merge lp:~cjwatson/launchpad/git-xmlrpc-auth-params into lp:launchpad

[Colin Watson]

Support passing GitAPI.translatePath authentication parameters as a dict.

Support passing GitAPI.translatePath authentication parameters as a dict. Once this is deployed, turnip will be able to pass additional data from the authentication stage through to translatePath; this will be needed for code imports, where we want to be able to say that a client has successfully authenticated but is only authorised to push to a certain repository.

[William Grant (wgrant)]

I don't think we use "uid" anywhere else, but it seems reasonably sensible.

review: Approve (code)

=== modified file 'lib/lp/code/xmlrpc/git.py'
--- lib/lp/code/xmlrpc/git.py	2016-10-05 09:15:05 +0000
+++ lib/lp/code/xmlrpc/git.py	2016-10-06 11:17:54 +0000
@@ -229,7 +229,7 @@
             raise
 
     @return_fault
-    def _translatePath(self, requester, path, permission, can_authenticate):
+    def _translatePath(self, requester, path, permission, auth_params):
         if requester == LAUNCHPAD_ANONYMOUS:
             requester = None
         try:
@@ -247,20 +247,28 @@
             # Turn lookup errors for anonymous HTTP requests into
             # "authorisation required", so that the user-agent has a
             # chance to try HTTP basic auth.
+            can_authenticate = auth_params.get("can-authenticate", False)
             if can_authenticate and requester is None:
                 raise faults.Unauthorized()
             else:
                 raise
 
-    def translatePath(self, path, permission, requester_id, can_authenticate):
+    def translatePath(self, path, permission, *auth_args):
         """See `IGitAPI`."""
+        if len(auth_args) == 1:
+            auth_params = auth_args[0]
+            requester_id = auth_params.get("uid")
+        else:
+            requester_id, can_authenticate = auth_args
+            auth_params = {
+                "uid": requester_id, "can-authenticate": can_authenticate}
         if requester_id is None:
             requester_id = LAUNCHPAD_ANONYMOUS
         if isinstance(path, str):
             path = path.decode('utf-8')
         return run_with_login(
             requester_id, self._translatePath,
-            path.strip("/"), permission, can_authenticate)
+            path.strip("/"), permission, auth_params)
 
     def notify(self, translated_path):
         """See `IGitAPI`."""

=== modified file 'lib/lp/code/xmlrpc/tests/test_git.py'
--- lib/lp/code/xmlrpc/tests/test_git.py	2016-10-05 09:15:05 +0000
+++ lib/lp/code/xmlrpc/tests/test_git.py	2016-10-06 11:17:54 +0000
@@ -5,16 +5,16 @@
 
 __metaclass__ = type
 
+from testscenarios import (
+    load_tests_apply_scenarios,
+    WithScenarios,
+    )
 from zope.component import getUtility
 from zope.security.proxy import removeSecurityProxy
 
 from lp.app.enums import InformationType
 from lp.code.enums import GitRepositoryType
 from lp.code.errors import GitRepositoryCreationFault
-from lp.code.interfaces.codehosting import (
-    LAUNCHPAD_ANONYMOUS,
-    LAUNCHPAD_SERVICES,
-    )
 from lp.code.interfaces.gitcollection import IAllGitRepositories
 from lp.code.interfaces.gitjob import IGitRefScanJobSource
 from lp.code.interfaces.gitrepository import (
@@ -39,22 +39,36 @@
 from lp.xmlrpc import faults
 
 
-class TestGitAPIMixin:
+class TestGitAPIMixin(WithScenarios):
     """Helper methods for `IGitAPI` tests, and security-relevant tests."""
 
+    scenarios = [
+        ("auth_params_flat", {"auth_params_dict": False}),
+        ("auth_params_dict", {"auth_params_dict": True}),
+        ]
+
     def setUp(self):
         super(TestGitAPIMixin, self).setUp()
         self.git_api = GitAPI(None, None)
         self.hosting_fixture = self.useFixture(GitHostingFixture())
         self.repository_set = getUtility(IGitRepositorySet)
 
+    def _translatePath(self, path, permission, auth_params):
+        if self.auth_params_dict:
+            return self.git_api.translatePath(path, permission, auth_params)
+        else:
+            return self.git_api.translatePath(
+                path, permission, auth_params.get("uid"),
+                auth_params.get("can-authenticate", False))
+
     def assertGitRepositoryNotFound(self, requester, path, permission="read",
                                     can_authenticate=False):
         """Assert that the given path cannot be translated."""
-        if requester not in (LAUNCHPAD_ANONYMOUS, LAUNCHPAD_SERVICES):
+        if requester is not None:
             requester = requester.id
-        fault = self.git_api.translatePath(
-            path, permission, requester, can_authenticate)
+        fault = self._translatePath(
+            path, permission,
+            {"uid": requester, "can-authenticate": can_authenticate})
         self.assertEqual(
             faults.GitRepositoryNotFound(path.strip("/")), fault)
 
@@ -62,29 +76,32 @@
                                message="Permission denied.",
                                permission="read", can_authenticate=False):
         """Assert that looking at the given path returns PermissionDenied."""
-        if requester not in (LAUNCHPAD_ANONYMOUS, LAUNCHPAD_SERVICES):
+        if requester is not None:
             requester = requester.id
-        fault = self.git_api.translatePath(
-            path, permission, requester, can_authenticate)
+        fault = self._translatePath(
+            path, permission,
+            {"uid": requester, "can-authenticate": can_authenticate})
         self.assertEqual(faults.PermissionDenied(message), fault)
 
     def assertUnauthorized(self, requester, path,
                            message="Authorisation required.",
                            permission="read", can_authenticate=False):
         """Assert that looking at the given path returns Unauthorized."""
-        if requester not in (LAUNCHPAD_ANONYMOUS, LAUNCHPAD_SERVICES):
+        if requester is not None:
             requester = requester.id
-        fault = self.git_api.translatePath(
-            path, permission, requester, can_authenticate)
+        fault = self._translatePath(
+            path, permission,
+            {"uid": requester, "can-authenticate": can_authenticate})
         self.assertEqual(faults.Unauthorized(message), fault)
 
     def assertNotFound(self, requester, path, message, permission="read",
                        can_authenticate=False):
         """Assert that looking at the given path returns NotFound."""
-        if requester not in (LAUNCHPAD_ANONYMOUS, LAUNCHPAD_SERVICES):
+        if requester is not None:
             requester = requester.id
-        fault = self.git_api.translatePath(
-            path, permission, requester, can_authenticate)
+        fault = self._translatePath(
+            path, permission,
+            {"uid": requester, "can-authenticate": can_authenticate})
         self.assertEqual(faults.NotFound(message), fault)
 
     def assertInvalidSourcePackageName(self, requester, path, name,
@@ -92,28 +109,31 @@
                                        can_authenticate=False):
         """Assert that looking at the given path returns
         InvalidSourcePackageName."""
-        if requester not in (LAUNCHPAD_ANONYMOUS, LAUNCHPAD_SERVICES):
+        if requester is not None:
             requester = requester.id
-        fault = self.git_api.translatePath(
-            path, permission, requester, can_authenticate)
+        fault = self._translatePath(
+            path, permission,
+            {"uid": requester, "can-authenticate": can_authenticate})
         self.assertEqual(faults.InvalidSourcePackageName(name), fault)
 
     def assertInvalidBranchName(self, requester, path, message,
                                 permission="read", can_authenticate=False):
         """Assert that looking at the given path returns InvalidBranchName."""
-        if requester not in (LAUNCHPAD_ANONYMOUS, LAUNCHPAD_SERVICES):
+        if requester is not None:
             requester = requester.id
-        fault = self.git_api.translatePath(
-            path, permission, requester, can_authenticate)
+        fault = self._translatePath(
+            path, permission,
+            {"uid": requester, "can-authenticate": can_authenticate})
         self.assertEqual(faults.InvalidBranchName(Exception(message)), fault)
 
     def assertOopsOccurred(self, requester, path,
                            permission="read", can_authenticate=False):
         """Assert that looking at the given path OOPSes."""
-        if requester not in (LAUNCHPAD_ANONYMOUS, LAUNCHPAD_SERVICES):
+        if requester is not None:
             requester = requester.id
-        fault = self.git_api.translatePath(
-            path, permission, requester, can_authenticate)
+        fault = self._translatePath(
+            path, permission,
+            {"uid": requester, "can-authenticate": can_authenticate})
         self.assertIsInstance(fault, faults.OopsOccurred)
         prefix = (
             "An unexpected error has occurred while creating a Git "
@@ -124,10 +144,11 @@
     def assertTranslates(self, requester, path, repository, writable,
                          permission="read", can_authenticate=False,
                          trailing="", private=False):
-        if requester not in (LAUNCHPAD_ANONYMOUS, LAUNCHPAD_SERVICES):
+        if requester is not None:
             requester = requester.id
-        translation = self.git_api.translatePath(
-            path, permission, requester, can_authenticate)
+        translation = self._translatePath(
+            path, permission,
+            {"uid": requester, "can-authenticate": can_authenticate})
         login(ANONYMOUS)
         self.assertEqual(
             {"path": repository.getInternalPath(), "writable": writable,
@@ -136,12 +157,13 @@
 
     def assertCreates(self, requester, path, can_authenticate=False,
                       private=False):
-        if requester in (LAUNCHPAD_ANONYMOUS, LAUNCHPAD_SERVICES):
+        if requester is None:
             requester_id = requester
         else:
             requester_id = requester.id
-        translation = self.git_api.translatePath(
-            path, "write", requester_id, can_authenticate)
+        translation = self._translatePath(
+            path, "write",
+            {"uid": requester_id, "can-authenticate": can_authenticate})
         login(ANONYMOUS)
         repository = getUtility(IGitRepositorySet).getByPath(
             requester, path.lstrip("/"))
@@ -185,10 +207,8 @@
             self.factory.makeGitRepository(
                 information_type=InformationType.USERDATA))
         path = u"/%s" % repository.unique_name
-        self.assertGitRepositoryNotFound(
-            LAUNCHPAD_ANONYMOUS, path, can_authenticate=False)
-        self.assertUnauthorized(
-            LAUNCHPAD_ANONYMOUS, path, can_authenticate=True)
+        self.assertGitRepositoryNotFound(None, path, can_authenticate=False)
+        self.assertUnauthorized(None, path, can_authenticate=True)
 
     def test_translatePath_team_unowned(self):
         requester = self.factory.makePerson()
@@ -298,11 +318,9 @@
         repository = self.factory.makeGitRepository()
         path = u"/%s" % repository.unique_name
         self.assertTranslates(
-            LAUNCHPAD_ANONYMOUS, path, repository, False,
-            can_authenticate=False)
+            None, path, repository, False, can_authenticate=False)
         self.assertTranslates(
-            LAUNCHPAD_ANONYMOUS, path, repository, False,
-            can_authenticate=True)
+            None, path, repository, False, can_authenticate=True)
 
     def test_translatePath_owned(self):
         requester = self.factory.makePerson()
@@ -400,11 +418,11 @@
         # Anonymous users cannot create repositories.
         project = self.factory.makeProject()
         self.assertGitRepositoryNotFound(
-            LAUNCHPAD_ANONYMOUS, u"/%s" % project.name,
-            permission="write", can_authenticate=False)
+            None, u"/%s" % project.name, permission="write",
+            can_authenticate=False)
         self.assertUnauthorized(
-            LAUNCHPAD_ANONYMOUS, u"/%s" % project.name,
-            permission="write", can_authenticate=True)
+            None, u"/%s" % project.name, permission="write",
+            can_authenticate=True)
 
     def test_translatePath_create_invalid_namespace(self):
         # Trying to create a repository at a path that isn't valid for Git
@@ -657,3 +675,6 @@
     """
 
     layer = AppServerLayer
+
+
+load_tests = load_tests_apply_scenarios