Merge lp:~cjwatson/launchpad/fix-git-authenticateWithPassword into lp:launchpad

[Colin Watson]

Fix XML-RPC publication of IGitAPI.authenticateWithPassword.

Zope's XML-RPC publication machinery was confused by the return_fault decorator, and published a method taking zero arguments. Pushing this decorator down a layer avoids that problem.

To test this, I needed to refactor the tests to call the API under test via an XML-RPC ServerProxy (which I probably should have done from the start anyway). Since this has the effect of serialising and deserialising any fault that's raised, I also had to rearrange how they test for faults.

[Tony Simpson (tonysimpson)]

Looks good to me!

review: Approve

=== modified file 'lib/lp/code/interfaces/gitapi.py'
--- lib/lp/code/interfaces/gitapi.py	2018-10-22 19:16:09 +0000
+++ lib/lp/code/interfaces/gitapi.py	2019-09-27 15:51:36 +0000
@@ -64,8 +64,13 @@
     def authenticateWithPassword(username, password):
         """Authenticate a user by username and password.
 
-        :returns: An `Unauthorized` fault, as password authentication is
-            not yet supported.
+        This currently only works when using macaroon authentication.
+
+        :returns: An `Unauthorized` fault if the username and password do
+            not match; otherwise, a dict containing a "uid" (for a real
+            user) or "user" (for internal services) key indicating the
+            authenticated principal, and possibly "macaroon" with a macaroon
+            that requires further authorisation by other methods.
         """
 
     def checkRefPermissions(translated_paths, ref_paths, auth_params):

=== modified file 'lib/lp/code/xmlrpc/git.py'
--- lib/lp/code/xmlrpc/git.py	2019-09-10 09:58:52 +0000
+++ lib/lp/code/xmlrpc/git.py	2019-09-27 15:51:36 +0000
@@ -427,8 +427,13 @@
             removeSecurityProxy(repository))
 
     @return_fault
-    def authenticateWithPassword(self, username, password):
-        """See `IGitAPI`."""
+    def _authenticateWithPassword(self, username, password):
+        """Authenticate a user by username and password.
+
+        This is a separate method from `authenticateWithPassword` because
+        otherwise Zope's XML-RPC publication machinery gets confused by the
+        decorator and publishes a method that takes zero arguments.
+        """
         user = getUtility(IPersonSet).getByName(username) if username else None
         verified = self._verifyMacaroon(password, user=user)
         if verified:
@@ -439,7 +444,11 @@
                 auth_params["user"] = LAUNCHPAD_SERVICES
             return auth_params
         # Only macaroons are supported for password authentication.
-        return faults.Unauthorized()
+        raise faults.Unauthorized()
+
+    def authenticateWithPassword(self, username, password):
+        """See `IGitAPI`."""
+        return self._authenticateWithPassword(username, password)
 
     def _renderPermissions(self, set_of_permissions):
         """Render a set of permission strings for XML-RPC output."""

=== modified file 'lib/lp/code/xmlrpc/tests/test_git.py'
--- lib/lp/code/xmlrpc/tests/test_git.py	2019-09-05 11:29:00 +0000
+++ lib/lp/code/xmlrpc/tests/test_git.py	2019-09-27 15:51:36 +0000
@@ -6,6 +6,7 @@
 __metaclass__ = type
 
 from pymacaroons import Macaroon
+import six
 from six.moves import xmlrpc_client
 from testtools.matchers import (
     Equals,
@@ -37,7 +38,6 @@
     IGitRepositorySet,
     )
 from lp.code.tests.helpers import GitHostingFixture
-from lp.code.xmlrpc.git import GitAPI
 from lp.registry.enums import TeamMembershipPolicy
 from lp.services.config import config
 from lp.services.features.testing import FeatureFixture
@@ -62,6 +62,7 @@
     AppServerLayer,
     LaunchpadFunctionalLayer,
     )
+from lp.testing.xmlrpc import XMLRPCTestTransport
 from lp.xmlrpc import faults
 
 
@@ -108,24 +109,58 @@
         return ok
 
 
+class MatchesFault(MatchesStructure):
+    """Match an XML-RPC fault.
+
+    This can be given either::
+
+        - a subclass of LaunchpadFault (matches only the fault code)
+        - an instance of Fault (matches the fault code and the fault string
+          from this instance exactly)
+    """
+
+    def __init__(self, expected_fault):
+        fault_matchers = {}
+        if (isinstance(expected_fault, six.class_types) and
+                issubclass(expected_fault, faults.LaunchpadFault)):
+            fault_matchers["faultCode"] = Equals(expected_fault.error_code)
+        else:
+            fault_matchers["faultCode"] = Equals(expected_fault.faultCode)
+            fault_string = expected_fault.faultString
+            # XXX cjwatson 2019-09-27: InvalidBranchName.faultString is
+            # bytes, so we need this to handle that case.  Should it be?
+            if not isinstance(fault_string, six.text_type):
+                fault_string = fault_string.decode("UTF-8")
+            fault_matchers["faultString"] = Equals(fault_string)
+        super(MatchesFault, self).__init__(**fault_matchers)
+
+
 class TestGitAPIMixin:
     """Helper methods for `IGitAPI` tests, and security-relevant tests."""
 
     def setUp(self):
         super(TestGitAPIMixin, self).setUp()
-        self.git_api = GitAPI(None, None)
+        self.git_api = xmlrpc_client.ServerProxy(
+            "http://xmlrpc-private.launchpad.test:8087/git",
+            transport=XMLRPCTestTransport())
         self.hosting_fixture = self.useFixture(GitHostingFixture())
         self.repository_set = getUtility(IGitRepositorySet)
 
+    def assertFault(self, expected_fault, func, *args, **kwargs):
+        """Assert that a call raises the expected fault."""
+        fault = self.assertRaises(xmlrpc_client.Fault, func, *args, **kwargs)
+        self.assertThat(fault, MatchesFault(expected_fault))
+        return fault
+
     def assertGitRepositoryNotFound(self, requester, path, permission="read",
                                     can_authenticate=False, macaroon_raw=None):
         """Assert that the given path cannot be translated."""
         auth_params = _make_auth_params(
             requester, can_authenticate=can_authenticate,
             macaroon_raw=macaroon_raw)
-        fault = self.git_api.translatePath(path, permission, auth_params)
-        self.assertEqual(
-            faults.GitRepositoryNotFound(path.strip("/")), fault)
+        self.assertFault(
+            faults.GitRepositoryNotFound(path.strip("/")),
+            self.git_api.translatePath, path, permission, auth_params)
 
     def assertPermissionDenied(self, requester, path,
                                message="Permission denied.",
@@ -135,8 +170,9 @@
         auth_params = _make_auth_params(
             requester, can_authenticate=can_authenticate,
             macaroon_raw=macaroon_raw)
-        fault = self.git_api.translatePath(path, permission, auth_params)
-        self.assertEqual(faults.PermissionDenied(message), fault)
+        self.assertFault(
+            faults.PermissionDenied(message),
+            self.git_api.translatePath, path, permission, auth_params)
 
     def assertUnauthorized(self, requester, path,
                            message="Authorisation required.",
@@ -146,16 +182,18 @@
         auth_params = _make_auth_params(
             requester, can_authenticate=can_authenticate,
             macaroon_raw=macaroon_raw)
-        fault = self.git_api.translatePath(path, permission, auth_params)
-        self.assertEqual(faults.Unauthorized(message), fault)
+        self.assertFault(
+            faults.Unauthorized(message),
+            self.git_api.translatePath, path, permission, auth_params)
 
     def assertNotFound(self, requester, path, message, permission="read",
                        can_authenticate=False):
         """Assert that looking at the given path returns NotFound."""
         auth_params = _make_auth_params(
             requester, can_authenticate=can_authenticate)
-        fault = self.git_api.translatePath(path, permission, auth_params)
-        self.assertEqual(faults.NotFound(message), fault)
+        self.assertFault(
+            faults.NotFound(message),
+            self.git_api.translatePath, path, permission, auth_params)
 
     def assertInvalidSourcePackageName(self, requester, path, name,
                                        permission="read",
@@ -164,24 +202,27 @@
         InvalidSourcePackageName."""
         auth_params = _make_auth_params(
             requester, can_authenticate=can_authenticate)
-        fault = self.git_api.translatePath(path, permission, auth_params)
-        self.assertEqual(faults.InvalidSourcePackageName(name), fault)
+        self.assertFault(
+            faults.InvalidSourcePackageName(name),
+            self.git_api.translatePath, path, permission, auth_params)
 
     def assertInvalidBranchName(self, requester, path, message,
                                 permission="read", can_authenticate=False):
         """Assert that looking at the given path returns InvalidBranchName."""
         auth_params = _make_auth_params(
             requester, can_authenticate=can_authenticate)
-        fault = self.git_api.translatePath(path, permission, auth_params)
-        self.assertEqual(faults.InvalidBranchName(Exception(message)), fault)
+        self.assertFault(
+            faults.InvalidBranchName(Exception(message)),
+            self.git_api.translatePath, path, permission, auth_params)
 
     def assertOopsOccurred(self, requester, path,
                            permission="read", can_authenticate=False):
         """Assert that looking at the given path OOPSes."""
         auth_params = _make_auth_params(
             requester, can_authenticate=can_authenticate)
-        fault = self.git_api.translatePath(path, permission, auth_params)
-        self.assertIsInstance(fault, faults.OopsOccurred)
+        fault = self.assertFault(
+            faults.OopsOccurred,
+            self.git_api.translatePath, path, permission, auth_params)
         prefix = (
             "An unexpected error has occurred while creating a Git "
             "repository. Please report a Launchpad bug and quote: ")
@@ -551,10 +592,10 @@
 
     def test_checkRefPermissions_nonexistent_repository(self):
         requester = self.factory.makePerson()
-        self.assertEqual(
+        self.assertFault(
             faults.GitRepositoryNotFound("nonexistent"),
-            self.git_api.checkRefPermissions(
-                "nonexistent", [], {"uid": requester.id}))
+            self.git_api.checkRefPermissions,
+            "nonexistent", [], {"uid": requester.id})
 
 
 class TestGitAPI(TestGitAPIMixin, TestCaseWithFactory):
@@ -1136,8 +1177,7 @@
     def test_notify_missing_repository(self):
         # A notify call on a non-existent repository returns a fault and
         # does not create a job.
-        fault = self.git_api.notify("10000")
-        self.assertIsInstance(fault, faults.NotFound)
+        self.assertFault(faults.NotFound, self.git_api.notify, "10000")
         job_source = getUtility(IGitRefScanJobSource)
         self.assertEqual([], list(job_source.iterReady()))
 
@@ -1153,9 +1193,9 @@
         self.assertEqual(repository, job.repository)
 
     def test_authenticateWithPassword(self):
-        self.assertIsInstance(
-            self.git_api.authenticateWithPassword('foo', 'bar'),
-            faults.Unauthorized)
+        self.assertFault(
+            faults.Unauthorized,
+            self.git_api.authenticateWithPassword, "foo", "bar")
 
     def test_authenticateWithPassword_code_import(self):
         self.pushConfig(
@@ -1170,13 +1210,13 @@
             {"macaroon": macaroon.serialize(), "user": "+launchpad-services"},
             self.git_api.authenticateWithPassword("", macaroon.serialize()))
         other_macaroon = Macaroon(identifier="another", key="another-secret")
-        self.assertIsInstance(
-            self.git_api.authenticateWithPassword(
-                "", other_macaroon.serialize()),
-            faults.Unauthorized)
-        self.assertIsInstance(
-            self.git_api.authenticateWithPassword("", "nonsense"),
-            faults.Unauthorized)
+        self.assertFault(
+            faults.Unauthorized,
+            self.git_api.authenticateWithPassword,
+            "", other_macaroon.serialize())
+        self.assertFault(
+            faults.Unauthorized,
+            self.git_api.authenticateWithPassword, "", "nonsense")
 
     def test_authenticateWithPassword_private_snap_build(self):
         self.useFixture(FeatureFixture(SNAP_TESTING_FLAGS))
@@ -1193,13 +1233,13 @@
             {"macaroon": macaroon.serialize(), "user": "+launchpad-services"},
             self.git_api.authenticateWithPassword("", macaroon.serialize()))
         other_macaroon = Macaroon(identifier="another", key="another-secret")
-        self.assertIsInstance(
-            self.git_api.authenticateWithPassword(
-                "", other_macaroon.serialize()),
-            faults.Unauthorized)
-        self.assertIsInstance(
-            self.git_api.authenticateWithPassword("", "nonsense"),
-            faults.Unauthorized)
+        self.assertFault(
+            faults.Unauthorized,
+            self.git_api.authenticateWithPassword,
+            "", other_macaroon.serialize())
+        self.assertFault(
+            faults.Unauthorized,
+            self.git_api.authenticateWithPassword, "", "nonsense")
 
     def test_authenticateWithPassword_user_macaroon(self):
         # A user with a suitable macaroon can authenticate using it, in
@@ -1214,21 +1254,21 @@
             {"macaroon": macaroon.serialize(), "uid": requester.id},
             self.git_api.authenticateWithPassword(
                 requester.name, macaroon.serialize()))
-        self.assertIsInstance(
-            self.git_api.authenticateWithPassword("", macaroon.serialize()),
-            faults.Unauthorized)
-        self.assertIsInstance(
-            self.git_api.authenticateWithPassword(
-                "nonexistent", macaroon.serialize()),
-            faults.Unauthorized)
+        self.assertFault(
+            faults.Unauthorized,
+            self.git_api.authenticateWithPassword, "", macaroon.serialize())
+        self.assertFault(
+            faults.Unauthorized,
+            self.git_api.authenticateWithPassword,
+            "nonexistent", macaroon.serialize())
         other_macaroon = Macaroon(identifier="another", key="another-secret")
-        self.assertIsInstance(
-            self.git_api.authenticateWithPassword(
-                requester.name, other_macaroon.serialize()),
-            faults.Unauthorized)
-        self.assertIsInstance(
-            self.git_api.authenticateWithPassword(requester.name, "nonsense"),
-            faults.Unauthorized)
+        self.assertFault(
+            faults.Unauthorized,
+            self.git_api.authenticateWithPassword,
+            requester.name, other_macaroon.serialize())
+        self.assertFault(
+            faults.Unauthorized,
+            self.git_api.authenticateWithPassword, requester.name, "nonsense")
 
     def test_authenticateWithPassword_user_mismatch(self):
         # authenticateWithPassword refuses macaroons in the case where the
@@ -1267,10 +1307,10 @@
                 name = (
                     requester if requester == LAUNCHPAD_SERVICES
                     else requester.name)
-                self.assertIsInstance(
-                    self.git_api.authenticateWithPassword(
-                        name, macaroon.serialize()),
-                    faults.Unauthorized)
+                self.assertFault(
+                    faults.Unauthorized,
+                    self.git_api.authenticateWithPassword,
+                    name, macaroon.serialize())
 
     def test_checkRefPermissions_code_import(self):
         # A code import worker with a suitable macaroon has repository owner