launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #23989
[Merge] lp:~cjwatson/launchpad/fix-git-authenticateWithPassword into lp:launchpad
Colin Watson has proposed merging lp:~cjwatson/launchpad/fix-git-authenticateWithPassword into lp:launchpad.
Commit message:
Fix XML-RPC publication of IGitAPI.authenticateWithPassword.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #1845630 in Launchpad itself: "github failing to import"
https://bugs.launchpad.net/launchpad/+bug/1845630
For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/fix-git-authenticateWithPassword/+merge/373332
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.
--
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/fix-git-authenticateWithPassword into lp:launchpad.
=== 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
Follow ups
