Launchpad itself

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

  1. fix-git-authenticateWithPassword
  2. Merge into devel
Proposed by Colin Watson
Status: Merged
Merged at revision: 19064
Proposed branch: lp:~cjwatson/launchpad/fix-git-authenticateWithPassword
Merge into: lp:launchpad
Diff against target: 342 lines (+116/-62)
3 files modified
lib/lp/code/interfaces/gitapi.py (+7/-2)
lib/lp/code/xmlrpc/git.py (+12/-3)
lib/lp/code/xmlrpc/tests/test_git.py (+97/-57)
To merge this branch: bzr merge lp:~cjwatson/launchpad/fix-git-authenticateWithPassword
Reviewer Review Type Date Requested Status
Tony Simpson (community) Approve
Launchpad code reviewers Pending
Review via email: mp+373332@code.launchpad.net

Commit message

Fix XML-RPC publication of IGitAPI.authenticateWithPassword.

Description of the change

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.

To post a comment you must log in.
Revision history for this message
Tony Simpson (tonysimpson) wrote :

Looks good to me!

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== 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 @@
64 def authenticateWithPassword(username, password):64 def authenticateWithPassword(username, password):
65 """Authenticate a user by username and password.65 """Authenticate a user by username and password.
6666
67 :returns: An `Unauthorized` fault, as password authentication is67 This currently only works when using macaroon authentication.
68 not yet supported.68
69 :returns: An `Unauthorized` fault if the username and password do
70 not match; otherwise, a dict containing a "uid" (for a real
71 user) or "user" (for internal services) key indicating the
72 authenticated principal, and possibly "macaroon" with a macaroon
73 that requires further authorisation by other methods.
69 """74 """
7075
71 def checkRefPermissions(translated_paths, ref_paths, auth_params):76 def checkRefPermissions(translated_paths, ref_paths, auth_params):
7277
=== 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 @@
427 removeSecurityProxy(repository))427 removeSecurityProxy(repository))
428428
429 @return_fault429 @return_fault
430 def authenticateWithPassword(self, username, password):430 def _authenticateWithPassword(self, username, password):
431 """See `IGitAPI`."""431 """Authenticate a user by username and password.
432
433 This is a separate method from `authenticateWithPassword` because
434 otherwise Zope's XML-RPC publication machinery gets confused by the
435 decorator and publishes a method that takes zero arguments.
436 """
432 user = getUtility(IPersonSet).getByName(username) if username else None437 user = getUtility(IPersonSet).getByName(username) if username else None
433 verified = self._verifyMacaroon(password, user=user)438 verified = self._verifyMacaroon(password, user=user)
434 if verified:439 if verified:
@@ -439,7 +444,11 @@
439 auth_params["user"] = LAUNCHPAD_SERVICES444 auth_params["user"] = LAUNCHPAD_SERVICES
440 return auth_params445 return auth_params
441 # Only macaroons are supported for password authentication.446 # Only macaroons are supported for password authentication.
442 return faults.Unauthorized()447 raise faults.Unauthorized()
448
449 def authenticateWithPassword(self, username, password):
450 """See `IGitAPI`."""
451 return self._authenticateWithPassword(username, password)
443452
444 def _renderPermissions(self, set_of_permissions):453 def _renderPermissions(self, set_of_permissions):
445 """Render a set of permission strings for XML-RPC output."""454 """Render a set of permission strings for XML-RPC output."""
446455
=== 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 @@
6__metaclass__ = type6__metaclass__ = type
77
8from pymacaroons import Macaroon8from pymacaroons import Macaroon
9import six
9from six.moves import xmlrpc_client10from six.moves import xmlrpc_client
10from testtools.matchers import (11from testtools.matchers import (
11 Equals,12 Equals,
@@ -37,7 +38,6 @@
37 IGitRepositorySet,38 IGitRepositorySet,
38 )39 )
39from lp.code.tests.helpers import GitHostingFixture40from lp.code.tests.helpers import GitHostingFixture
40from lp.code.xmlrpc.git import GitAPI
41from lp.registry.enums import TeamMembershipPolicy41from lp.registry.enums import TeamMembershipPolicy
42from lp.services.config import config42from lp.services.config import config
43from lp.services.features.testing import FeatureFixture43from lp.services.features.testing import FeatureFixture
@@ -62,6 +62,7 @@
62 AppServerLayer,62 AppServerLayer,
63 LaunchpadFunctionalLayer,63 LaunchpadFunctionalLayer,
64 )64 )
65from lp.testing.xmlrpc import XMLRPCTestTransport
65from lp.xmlrpc import faults66from lp.xmlrpc import faults
6667
6768
@@ -108,24 +109,58 @@
108 return ok109 return ok
109110
110111
112class MatchesFault(MatchesStructure):
113 """Match an XML-RPC fault.
114
115 This can be given either::
116
117 - a subclass of LaunchpadFault (matches only the fault code)
118 - an instance of Fault (matches the fault code and the fault string
119 from this instance exactly)
120 """
121
122 def __init__(self, expected_fault):
123 fault_matchers = {}
124 if (isinstance(expected_fault, six.class_types) and
125 issubclass(expected_fault, faults.LaunchpadFault)):
126 fault_matchers["faultCode"] = Equals(expected_fault.error_code)
127 else:
128 fault_matchers["faultCode"] = Equals(expected_fault.faultCode)
129 fault_string = expected_fault.faultString
130 # XXX cjwatson 2019-09-27: InvalidBranchName.faultString is
131 # bytes, so we need this to handle that case. Should it be?
132 if not isinstance(fault_string, six.text_type):
133 fault_string = fault_string.decode("UTF-8")
134 fault_matchers["faultString"] = Equals(fault_string)
135 super(MatchesFault, self).__init__(**fault_matchers)
136
137
111class TestGitAPIMixin:138class TestGitAPIMixin:
112 """Helper methods for `IGitAPI` tests, and security-relevant tests."""139 """Helper methods for `IGitAPI` tests, and security-relevant tests."""
113140
114 def setUp(self):141 def setUp(self):
115 super(TestGitAPIMixin, self).setUp()142 super(TestGitAPIMixin, self).setUp()
116 self.git_api = GitAPI(None, None)143 self.git_api = xmlrpc_client.ServerProxy(
144 "http://xmlrpc-private.launchpad.test:8087/git",
145 transport=XMLRPCTestTransport())
117 self.hosting_fixture = self.useFixture(GitHostingFixture())146 self.hosting_fixture = self.useFixture(GitHostingFixture())
118 self.repository_set = getUtility(IGitRepositorySet)147 self.repository_set = getUtility(IGitRepositorySet)
119148
149 def assertFault(self, expected_fault, func, *args, **kwargs):
150 """Assert that a call raises the expected fault."""
151 fault = self.assertRaises(xmlrpc_client.Fault, func, *args, **kwargs)
152 self.assertThat(fault, MatchesFault(expected_fault))
153 return fault
154
120 def assertGitRepositoryNotFound(self, requester, path, permission="read",155 def assertGitRepositoryNotFound(self, requester, path, permission="read",
121 can_authenticate=False, macaroon_raw=None):156 can_authenticate=False, macaroon_raw=None):
122 """Assert that the given path cannot be translated."""157 """Assert that the given path cannot be translated."""
123 auth_params = _make_auth_params(158 auth_params = _make_auth_params(
124 requester, can_authenticate=can_authenticate,159 requester, can_authenticate=can_authenticate,
125 macaroon_raw=macaroon_raw)160 macaroon_raw=macaroon_raw)
126 fault = self.git_api.translatePath(path, permission, auth_params)161 self.assertFault(
127 self.assertEqual(162 faults.GitRepositoryNotFound(path.strip("/")),
128 faults.GitRepositoryNotFound(path.strip("/")), fault)163 self.git_api.translatePath, path, permission, auth_params)
129164
130 def assertPermissionDenied(self, requester, path,165 def assertPermissionDenied(self, requester, path,
131 message="Permission denied.",166 message="Permission denied.",
@@ -135,8 +170,9 @@
135 auth_params = _make_auth_params(170 auth_params = _make_auth_params(
136 requester, can_authenticate=can_authenticate,171 requester, can_authenticate=can_authenticate,
137 macaroon_raw=macaroon_raw)172 macaroon_raw=macaroon_raw)
138 fault = self.git_api.translatePath(path, permission, auth_params)173 self.assertFault(
139 self.assertEqual(faults.PermissionDenied(message), fault)174 faults.PermissionDenied(message),
175 self.git_api.translatePath, path, permission, auth_params)
140176
141 def assertUnauthorized(self, requester, path,177 def assertUnauthorized(self, requester, path,
142 message="Authorisation required.",178 message="Authorisation required.",
@@ -146,16 +182,18 @@
146 auth_params = _make_auth_params(182 auth_params = _make_auth_params(
147 requester, can_authenticate=can_authenticate,183 requester, can_authenticate=can_authenticate,
148 macaroon_raw=macaroon_raw)184 macaroon_raw=macaroon_raw)
149 fault = self.git_api.translatePath(path, permission, auth_params)185 self.assertFault(
150 self.assertEqual(faults.Unauthorized(message), fault)186 faults.Unauthorized(message),
187 self.git_api.translatePath, path, permission, auth_params)
151188
152 def assertNotFound(self, requester, path, message, permission="read",189 def assertNotFound(self, requester, path, message, permission="read",
153 can_authenticate=False):190 can_authenticate=False):
154 """Assert that looking at the given path returns NotFound."""191 """Assert that looking at the given path returns NotFound."""
155 auth_params = _make_auth_params(192 auth_params = _make_auth_params(
156 requester, can_authenticate=can_authenticate)193 requester, can_authenticate=can_authenticate)
157 fault = self.git_api.translatePath(path, permission, auth_params)194 self.assertFault(
158 self.assertEqual(faults.NotFound(message), fault)195 faults.NotFound(message),
196 self.git_api.translatePath, path, permission, auth_params)
159197
160 def assertInvalidSourcePackageName(self, requester, path, name,198 def assertInvalidSourcePackageName(self, requester, path, name,
161 permission="read",199 permission="read",
@@ -164,24 +202,27 @@
164 InvalidSourcePackageName."""202 InvalidSourcePackageName."""
165 auth_params = _make_auth_params(203 auth_params = _make_auth_params(
166 requester, can_authenticate=can_authenticate)204 requester, can_authenticate=can_authenticate)
167 fault = self.git_api.translatePath(path, permission, auth_params)205 self.assertFault(
168 self.assertEqual(faults.InvalidSourcePackageName(name), fault)206 faults.InvalidSourcePackageName(name),
207 self.git_api.translatePath, path, permission, auth_params)
169208
170 def assertInvalidBranchName(self, requester, path, message,209 def assertInvalidBranchName(self, requester, path, message,
171 permission="read", can_authenticate=False):210 permission="read", can_authenticate=False):
172 """Assert that looking at the given path returns InvalidBranchName."""211 """Assert that looking at the given path returns InvalidBranchName."""
173 auth_params = _make_auth_params(212 auth_params = _make_auth_params(
174 requester, can_authenticate=can_authenticate)213 requester, can_authenticate=can_authenticate)
175 fault = self.git_api.translatePath(path, permission, auth_params)214 self.assertFault(
176 self.assertEqual(faults.InvalidBranchName(Exception(message)), fault)215 faults.InvalidBranchName(Exception(message)),
216 self.git_api.translatePath, path, permission, auth_params)
177217
178 def assertOopsOccurred(self, requester, path,218 def assertOopsOccurred(self, requester, path,
179 permission="read", can_authenticate=False):219 permission="read", can_authenticate=False):
180 """Assert that looking at the given path OOPSes."""220 """Assert that looking at the given path OOPSes."""
181 auth_params = _make_auth_params(221 auth_params = _make_auth_params(
182 requester, can_authenticate=can_authenticate)222 requester, can_authenticate=can_authenticate)
183 fault = self.git_api.translatePath(path, permission, auth_params)223 fault = self.assertFault(
184 self.assertIsInstance(fault, faults.OopsOccurred)224 faults.OopsOccurred,
225 self.git_api.translatePath, path, permission, auth_params)
185 prefix = (226 prefix = (
186 "An unexpected error has occurred while creating a Git "227 "An unexpected error has occurred while creating a Git "
187 "repository. Please report a Launchpad bug and quote: ")228 "repository. Please report a Launchpad bug and quote: ")
@@ -551,10 +592,10 @@
551592
552 def test_checkRefPermissions_nonexistent_repository(self):593 def test_checkRefPermissions_nonexistent_repository(self):
553 requester = self.factory.makePerson()594 requester = self.factory.makePerson()
554 self.assertEqual(595 self.assertFault(
555 faults.GitRepositoryNotFound("nonexistent"),596 faults.GitRepositoryNotFound("nonexistent"),
556 self.git_api.checkRefPermissions(597 self.git_api.checkRefPermissions,
557 "nonexistent", [], {"uid": requester.id}))598 "nonexistent", [], {"uid": requester.id})
558599
559600
560class TestGitAPI(TestGitAPIMixin, TestCaseWithFactory):601class TestGitAPI(TestGitAPIMixin, TestCaseWithFactory):
@@ -1136,8 +1177,7 @@
1136 def test_notify_missing_repository(self):1177 def test_notify_missing_repository(self):
1137 # A notify call on a non-existent repository returns a fault and1178 # A notify call on a non-existent repository returns a fault and
1138 # does not create a job.1179 # does not create a job.
1139 fault = self.git_api.notify("10000")1180 self.assertFault(faults.NotFound, self.git_api.notify, "10000")
1140 self.assertIsInstance(fault, faults.NotFound)
1141 job_source = getUtility(IGitRefScanJobSource)1181 job_source = getUtility(IGitRefScanJobSource)
1142 self.assertEqual([], list(job_source.iterReady()))1182 self.assertEqual([], list(job_source.iterReady()))
11431183
@@ -1153,9 +1193,9 @@
1153 self.assertEqual(repository, job.repository)1193 self.assertEqual(repository, job.repository)
11541194
1155 def test_authenticateWithPassword(self):1195 def test_authenticateWithPassword(self):
1156 self.assertIsInstance(1196 self.assertFault(
1157 self.git_api.authenticateWithPassword('foo', 'bar'),1197 faults.Unauthorized,
1158 faults.Unauthorized)1198 self.git_api.authenticateWithPassword, "foo", "bar")
11591199
1160 def test_authenticateWithPassword_code_import(self):1200 def test_authenticateWithPassword_code_import(self):
1161 self.pushConfig(1201 self.pushConfig(
@@ -1170,13 +1210,13 @@
1170 {"macaroon": macaroon.serialize(), "user": "+launchpad-services"},1210 {"macaroon": macaroon.serialize(), "user": "+launchpad-services"},
1171 self.git_api.authenticateWithPassword("", macaroon.serialize()))1211 self.git_api.authenticateWithPassword("", macaroon.serialize()))
1172 other_macaroon = Macaroon(identifier="another", key="another-secret")1212 other_macaroon = Macaroon(identifier="another", key="another-secret")
1173 self.assertIsInstance(1213 self.assertFault(
1174 self.git_api.authenticateWithPassword(1214 faults.Unauthorized,
1175 "", other_macaroon.serialize()),1215 self.git_api.authenticateWithPassword,
1176 faults.Unauthorized)1216 "", other_macaroon.serialize())
1177 self.assertIsInstance(1217 self.assertFault(
1178 self.git_api.authenticateWithPassword("", "nonsense"),1218 faults.Unauthorized,
1179 faults.Unauthorized)1219 self.git_api.authenticateWithPassword, "", "nonsense")
11801220
1181 def test_authenticateWithPassword_private_snap_build(self):1221 def test_authenticateWithPassword_private_snap_build(self):
1182 self.useFixture(FeatureFixture(SNAP_TESTING_FLAGS))1222 self.useFixture(FeatureFixture(SNAP_TESTING_FLAGS))
@@ -1193,13 +1233,13 @@
1193 {"macaroon": macaroon.serialize(), "user": "+launchpad-services"},1233 {"macaroon": macaroon.serialize(), "user": "+launchpad-services"},
1194 self.git_api.authenticateWithPassword("", macaroon.serialize()))1234 self.git_api.authenticateWithPassword("", macaroon.serialize()))
1195 other_macaroon = Macaroon(identifier="another", key="another-secret")1235 other_macaroon = Macaroon(identifier="another", key="another-secret")
1196 self.assertIsInstance(1236 self.assertFault(
1197 self.git_api.authenticateWithPassword(1237 faults.Unauthorized,
1198 "", other_macaroon.serialize()),1238 self.git_api.authenticateWithPassword,
1199 faults.Unauthorized)1239 "", other_macaroon.serialize())
1200 self.assertIsInstance(1240 self.assertFault(
1201 self.git_api.authenticateWithPassword("", "nonsense"),1241 faults.Unauthorized,
1202 faults.Unauthorized)1242 self.git_api.authenticateWithPassword, "", "nonsense")
12031243
1204 def test_authenticateWithPassword_user_macaroon(self):1244 def test_authenticateWithPassword_user_macaroon(self):
1205 # A user with a suitable macaroon can authenticate using it, in1245 # A user with a suitable macaroon can authenticate using it, in
@@ -1214,21 +1254,21 @@
1214 {"macaroon": macaroon.serialize(), "uid": requester.id},1254 {"macaroon": macaroon.serialize(), "uid": requester.id},
1215 self.git_api.authenticateWithPassword(1255 self.git_api.authenticateWithPassword(
1216 requester.name, macaroon.serialize()))1256 requester.name, macaroon.serialize()))
1217 self.assertIsInstance(1257 self.assertFault(
1218 self.git_api.authenticateWithPassword("", macaroon.serialize()),1258 faults.Unauthorized,
1219 faults.Unauthorized)1259 self.git_api.authenticateWithPassword, "", macaroon.serialize())
1220 self.assertIsInstance(1260 self.assertFault(
1221 self.git_api.authenticateWithPassword(1261 faults.Unauthorized,
1222 "nonexistent", macaroon.serialize()),1262 self.git_api.authenticateWithPassword,
1223 faults.Unauthorized)1263 "nonexistent", macaroon.serialize())
1224 other_macaroon = Macaroon(identifier="another", key="another-secret")1264 other_macaroon = Macaroon(identifier="another", key="another-secret")
1225 self.assertIsInstance(1265 self.assertFault(
1226 self.git_api.authenticateWithPassword(1266 faults.Unauthorized,
1227 requester.name, other_macaroon.serialize()),1267 self.git_api.authenticateWithPassword,
1228 faults.Unauthorized)1268 requester.name, other_macaroon.serialize())
1229 self.assertIsInstance(1269 self.assertFault(
1230 self.git_api.authenticateWithPassword(requester.name, "nonsense"),1270 faults.Unauthorized,
1231 faults.Unauthorized)1271 self.git_api.authenticateWithPassword, requester.name, "nonsense")
12321272
1233 def test_authenticateWithPassword_user_mismatch(self):1273 def test_authenticateWithPassword_user_mismatch(self):
1234 # authenticateWithPassword refuses macaroons in the case where the1274 # authenticateWithPassword refuses macaroons in the case where the
@@ -1267,10 +1307,10 @@
1267 name = (1307 name = (
1268 requester if requester == LAUNCHPAD_SERVICES1308 requester if requester == LAUNCHPAD_SERVICES
1269 else requester.name)1309 else requester.name)
1270 self.assertIsInstance(1310 self.assertFault(
1271 self.git_api.authenticateWithPassword(1311 faults.Unauthorized,
1272 name, macaroon.serialize()),1312 self.git_api.authenticateWithPassword,
1273 faults.Unauthorized)1313 name, macaroon.serialize())
12741314
1275 def test_checkRefPermissions_code_import(self):1315 def test_checkRefPermissions_code_import(self):
1276 # A code import worker with a suitable macaroon has repository owner1316 # A code import worker with a suitable macaroon has repository owner