Merge ~ilasc/launchpad:add-mp-url-to-git-push into launchpad:master

Proposed by Ioana Lasc on 2019-12-20
Status: Merged
Approved by: Ioana Lasc 7 hours ago
Approved revision: 9d74d487fa58d8c3cea4d78768aad92bc2ffd2f6
Merge reported by: Otto Co-Pilot
Merged at revision: not available
Proposed branch: ~ilasc/launchpad:add-mp-url-to-git-push
Merge into: launchpad:master
Diff against target: 386 lines (+308/-2)
3 files modified
lib/lp/code/interfaces/gitapi.py (+13/-0)
lib/lp/code/xmlrpc/git.py (+46/-1)
lib/lp/code/xmlrpc/tests/test_git.py (+249/-1)
Reviewer Review Type Date Requested Status
Colin Watson 2019-12-20 Approve 7 hours ago
Review via email: mp+377044@code.launchpad.net

Commit message

Add MP URL to git push

To post a comment you must log in.
Ioana Lasc (ilasc) wrote :

This needs Unit Tests and imports re-arranging - the rest is definitely in need a of a critical eye.

Colin Watson (cjwatson) :
d9bdd8f... by Ioana Lasc on 2020-01-08

Addressed review comments and added unit tests.

Ioana Lasc (ilasc) wrote :

Addressed code review comments and added a couple of Unit Tests.

Still working on moving the check for the default branch to the turnip side.

f76a6d6... by Ioana Lasc on 2020-01-23

Move check for default branch from LP to turnip

Ioana Lasc (ilasc) wrote :

Moved the check for default branch to Turnip per our conversation.

Goes hand in hand with this on the Turnip side: https://code.launchpad.net/~ilasc/turnip/+git/turnip/+merge/377045

Both now ready for review.

Colin Watson (cjwatson) :
review: Needs Fixing
d36f50c... by Ioana Lasc on 2020-01-29

Add authentication for getMergeProposalURL

Split up the implementation into `getMergeProposalURL` and
`_getMergeProposalURL` to add authetication for the
getMergeProposalURL endpoint on the Git API. As LP returns
data back to Turnip this needs to be an authenticated
operation. Added unit tests for several scenarios.
Needs more comments and more tests for other scenarios.

c0ec754... by Ioana Lasc on 2020-01-31

Add unit tests for getMergeProposalURL.

After changing to an authenticated getMergeProposalURL
more unit tests were required to cover the authenticated
user and code imports scenarios.

Ioana Lasc (ilasc) wrote :

Added more Unit Tests and addressed almost all comments apart from

from six.moves.urllib.parse import quote instead of just importing urllib

can't get the code to compile with it - which is a known PyCharm issue, tried running cli tests but code doesn't run in cli either and make lint fails.

I believe there is also an issue with Unauthorized scenario in getMergeProposalURL. I went by below comment and instead of raising Unauthorized I'm returning '', but now I'm not sure if that's not meant for timeouts only?

Comment I'm referring to in git.py line 564:
            # XXX cjwatson 2019-05-09: It would be simpler to just raise
            # this directly, but turnip won't handle it very gracefully at
            # the moment. It's possible to reach this by being very unlucky
            # about the timing of a push.

Colin Watson (cjwatson) wrote :

Not being able to use six.moves.urllib.parse is a serious problem; I expect to do a bulk conversion of all of Launchpad to that form soon. What exactly was the compilation failure, and can you give a link to the known PyCharm issue?

review: Needs Fixing
6f934f1... by Ioana Lasc on 2020-02-06

Refactor Unit Tests for GIT API getMergeProposalURL

Ioana Lasc (ilasc) wrote :

Thank you Colin. MP is now ready for review in tandem with: https://code.launchpad.net/~ilasc/turnip/+git/turnip/+merge/377045

Colin Watson (cjwatson) :
review: Approve
678cfce... by Ioana Lasc on 2020-03-30

Address code review comments

Added minor changes to unit tests suggested in code review
for the add-mp-url-to-git-push branch.

Ioana Lasc (ilasc) wrote :

Thanks Colin, last round of updates done and noted that we don't land 377045 until this makes it to Prod.

Colin Watson (cjwatson) :
review: Approve
9d74d48... by Ioana Lasc 7 hours ago

Remove commented out code

Removed commented code left in lp/code/xmlrpc/tests/test_git.py.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1diff --git a/lib/lp/code/interfaces/gitapi.py b/lib/lp/code/interfaces/gitapi.py
2index c7db491..c289dee 100644
3--- a/lib/lp/code/interfaces/gitapi.py
4+++ b/lib/lp/code/interfaces/gitapi.py
5@@ -79,3 +79,16 @@ class IGitAPI(Interface):
6
7 :returns: A list of rules for the user in the specified repository
8 """
9+
10+ def getMergeProposalURL(translated_path, branch, auth_params):
11+ """Return the URL for a merge proposal.
12+
13+ When a `branch` that is not the default branch in a repository
14+ is pushed, the URL where the merge proposal for that branch can
15+ be opened will be generated and returned.
16+
17+ :returns: The URL to register a merge proposal for the branch in the
18+ specified repository. A `GitRepositoryNotFound` fault is returned
19+ if no repository can be found for 'translated_path',
20+ or an `Unauthorized` fault for unauthorized push attempts.
21+ """
22diff --git a/lib/lp/code/xmlrpc/git.py b/lib/lp/code/xmlrpc/git.py
23index cbf9205..b34307a 100644
24--- a/lib/lp/code/xmlrpc/git.py
25+++ b/lib/lp/code/xmlrpc/git.py
26@@ -15,6 +15,7 @@ import uuid
27 from pymacaroons import Macaroon
28 import six
29 from six.moves import xmlrpc_client
30+from six.moves.urllib.parse import quote
31 import transaction
32 from zope.component import (
33 ComponentLookupError,
34@@ -72,7 +73,10 @@ from lp.services.macaroons.interfaces import (
35 IMacaroonIssuer,
36 NO_USER,
37 )
38-from lp.services.webapp import LaunchpadXMLRPCView
39+from lp.services.webapp import (
40+ canonical_url,
41+ LaunchpadXMLRPCView,
42+ )
43 from lp.services.webapp.authorization import check_permission
44 from lp.services.webapp.errorlog import ScriptRequest
45 from lp.xmlrpc import faults
46@@ -427,6 +431,47 @@ class GitAPI(LaunchpadXMLRPCView):
47 logger.info("notify succeeded")
48
49 @return_fault
50+ def _getMergeProposalURL(self, requester, translated_path, branch,
51+ auth_params):
52+ if requester == LAUNCHPAD_ANONYMOUS:
53+ requester = None
54+ repository = removeSecurityProxy(
55+ getUtility(IGitLookup).getByHostingPath(translated_path))
56+ if repository is None:
57+ raise faults.GitRepositoryNotFound(translated_path)
58+
59+ verified = self._verifyAuthParams(requester, repository, auth_params)
60+ if verified is not None and verified.user is NO_USER:
61+ # Showing a merge proposal URL may be useful to ordinary users,
62+ # but it doesn't make sense in the context of an internal service.
63+ return None
64+
65+ # We assemble the URL this way here because the ref may not exist yet.
66+ base_url = canonical_url(repository, rootsite='code')
67+ mp_url = "%s/+ref/%s/+register-merge" % (
68+ base_url, quote(branch))
69+ return mp_url
70+
71+ def getMergeProposalURL(self, translated_path, branch, auth_params):
72+ """See `IGitAPI`."""
73+ logger = self._getLogger(auth_params.get("request-id"))
74+ requester_id = _get_requester_id(auth_params)
75+ logger.info(
76+ "Request received: getMergeProposalURL('%s, %s') for %s",
77+ translated_path, branch, requester_id)
78+ result = run_with_login(
79+ requester_id, self._getMergeProposalURL,
80+ translated_path, branch, auth_params)
81+ if isinstance(result, xmlrpc_client.Fault):
82+ logger.error("getMergeProposalURL failed: %r", result)
83+ else:
84+ # The result of getMergeProposalURL is not sensitive for logging
85+ # purposes (it may refer to private artifacts, but contains no
86+ # credentials, only the merge proposal URL).
87+ logger.info("getMergeProposalURL succeeded: %s" % result)
88+ return result
89+
90+ @return_fault
91 def _authenticateWithPassword(self, username, password):
92 """Authenticate a user by username and password.
93
94diff --git a/lib/lp/code/xmlrpc/tests/test_git.py b/lib/lp/code/xmlrpc/tests/test_git.py
95index df65f0e..6f12f1f 100644
96--- a/lib/lp/code/xmlrpc/tests/test_git.py
97+++ b/lib/lp/code/xmlrpc/tests/test_git.py
98@@ -1,4 +1,4 @@
99-# Copyright 2015-2019 Canonical Ltd. This software is licensed under the
100+# Copyright 2015-2020 Canonical Ltd. This software is licensed under the
101 # GNU Affero General Public License version 3 (see the file LICENSE).
102
103 """Tests for the internal Git API."""
104@@ -11,6 +11,7 @@ from fixtures import FakeLogger
105 from pymacaroons import Macaroon
106 import six
107 from six.moves import xmlrpc_client
108+from six.moves.urllib.parse import quote
109 from testtools.matchers import (
110 Equals,
111 IsInstance,
112@@ -51,6 +52,7 @@ from lp.services.macaroons.interfaces import (
113 NO_USER,
114 )
115 from lp.services.macaroons.model import MacaroonIssuerBase
116+from lp.services.webapp import canonical_url
117 from lp.services.webapp.escaping import html_escape
118 from lp.snappy.interfaces.snap import SNAP_TESTING_FLAGS
119 from lp.testing import (
120@@ -326,6 +328,15 @@ class TestGitAPIMixin:
121 ])
122 for ref_path, ref_permissions in permissions.items())))
123
124+ def assertHasMergeProposalURL(self, repository,
125+ pushed_branch, auth_params):
126+ base_url = canonical_url(repository, rootsite='code')
127+ expected_mp_url = "%s/+ref/%s/+register-merge" % (
128+ base_url, quote(pushed_branch))
129+ result = self.git_api.getMergeProposalURL(
130+ repository.getInternalPath(), pushed_branch, auth_params)
131+ self.assertEqual(expected_mp_url, result)
132+
133 def test_translatePath_private_repository(self):
134 requester = self.factory.makePerson()
135 repository = removeSecurityProxy(
136@@ -636,6 +647,13 @@ class TestGitAPIMixin:
137 faults.GitRepositoryNotFound("nonexistent"), None,
138 "checkRefPermissions", "nonexistent", [], {"uid": requester.id})
139
140+ def test_getMergeProposalURL__nonexistent_repository(self):
141+ requester = self.factory.makePerson()
142+ self.assertFault(
143+ faults.GitRepositoryNotFound("nonexistent"), None,
144+ "getMergeProposalURL", "nonexistent", 'branch1',
145+ {"uid": requester.id})
146+
147
148 class TestGitAPI(TestGitAPIMixin, TestCaseWithFactory):
149 """Tests for the implementation of `IGitAPI`."""
150@@ -1265,6 +1283,236 @@ class TestGitAPI(TestGitAPIMixin, TestCaseWithFactory):
151 requester, path, permission="read",
152 macaroon_raw=macaroon.serialize())
153
154+ def test_getMergeProposalURL_user(self):
155+ # A merge proposal URL is returned by LP for a non-default branch
156+ # pushed by a user that has their ordinary privileges on the
157+ # corresponding repository.
158+ requester_owner = self.factory.makePerson()
159+ repository = self.factory.makeGitRepository(owner=requester_owner)
160+ self.factory.makeGitRefs(
161+ repository=repository, paths=[u'refs/heads/master'])
162+ removeSecurityProxy(repository).default_branch = u'refs/heads/master'
163+ pushed_branch = 'branch1'
164+ self.assertHasMergeProposalURL(repository, pushed_branch,
165+ {"uid": requester_owner.id})
166+
167+ # Turnip verifies if the pushed branch is the default branch
168+ # in a repository and calls LP only for non default branches.
169+ # Consequently LP will process any incoming branch from Turnip
170+ # as being non default and produce a merge proposal URL for it.
171+ self.factory.makeGitRefs(
172+ repository=repository, paths=[u'refs/heads/%s' % pushed_branch])
173+ removeSecurityProxy(repository).default_branch = (
174+ u'refs/heads/%s' % pushed_branch)
175+ self.assertHasMergeProposalURL(repository, pushed_branch,
176+ {"uid": requester_owner.id})
177+
178+ requester_non_owner = self.factory.makePerson()
179+ self.assertHasMergeProposalURL(repository, pushed_branch,
180+ {"uid": requester_non_owner.id})
181+
182+ def test_getMergeProposalURL_user_macaroon(self):
183+ # The merge proposal URL is returned by LP for a non-default branch
184+ # pushed by a user with a suitable macaroon that
185+ # has their ordinary privileges on the corresponding repository.
186+
187+ self.pushConfig("codehosting", git_macaroon_secret_key="some-secret")
188+ requester = self.factory.makePerson()
189+ repository = self.factory.makeGitRepository(owner=requester)
190+ issuer = getUtility(IMacaroonIssuer, "git-repository")
191+ self.factory.makeGitRefs(
192+ repository=repository, paths=[u'refs/heads/master'])
193+ removeSecurityProxy(repository).default_branch = u'refs/heads/master'
194+
195+ pushed_branch = 'branch1'
196+ with person_logged_in(requester):
197+ macaroon = removeSecurityProxy(issuer).issueMacaroon(
198+ repository, user=requester)
199+ auth_params = _make_auth_params(requester,
200+ macaroon_raw=macaroon.serialize())
201+ self.assertHasMergeProposalURL(repository, pushed_branch, auth_params)
202+
203+ def test_getMergeProposalURL_user_mismatch(self):
204+ # getMergeProposalURL refuses macaroons in the case where the
205+ # user doesn't match what the issuer claims was verified. (We use a
206+ # dummy issuer for this, since this is a stopgap check to defend
207+ # against issuer bugs)
208+
209+ issuer = DummyMacaroonIssuer()
210+ # Claim to be the code-import-job issuer. This is a bit weird, but
211+ # it gets us past the difficulty that only certain named issuers are
212+ # allowed to confer write permissions.
213+ issuer.identifier = "code-import-job"
214+ self.useFixture(ZopeUtilityFixture(
215+ issuer, IMacaroonIssuer, name="code-import-job"))
216+ requesters = [self.factory.makePerson() for _ in range(2)]
217+ owner = self.factory.makeTeam(members=requesters)
218+ repository = self.factory.makeGitRepository(owner=owner)
219+ self.factory.makeGitRefs(
220+ repository=repository, paths=[u'refs/heads/master'])
221+ removeSecurityProxy(repository).default_branch = u'refs/heads/master'
222+ pushed_branch = 'branch1'
223+ macaroon = issuer.issueMacaroon(repository)
224+
225+ for verified_user, authorized, unauthorized in (
226+ (requesters[0], [requesters[0]],
227+ [LAUNCHPAD_SERVICES, requesters[1], None]),
228+ ([None, NO_USER], [],
229+ [LAUNCHPAD_SERVICES] + requesters + [None]),
230+ ):
231+ issuer._verified_user = verified_user
232+ for requester in authorized:
233+ login(ANONYMOUS)
234+ auth_params = _make_auth_params(
235+ requester, macaroon_raw=macaroon.serialize())
236+ self.assertHasMergeProposalURL(repository,
237+ pushed_branch, auth_params)
238+ for requester in unauthorized:
239+ login(ANONYMOUS)
240+ auth_params = _make_auth_params(
241+ requester, macaroon_raw=macaroon.serialize())
242+
243+ self.assertFault(
244+ faults.Unauthorized, None,
245+ "getMergeProposalURL", repository.getInternalPath(),
246+ pushed_branch, auth_params)
247+
248+ def test_getMergeProposalURL_code_import(self):
249+ # A merge proposal URL from LP to Turnip is not returned for
250+ # code import job as there is no User at the other end.
251+
252+ issuer = DummyMacaroonIssuer()
253+ # Claim to be the code-import-job issuer. This is a bit weird, but
254+ # it gets us past the difficulty that only certain named issuers are
255+ # allowed to confer write permissions.
256+ self.pushConfig(
257+ "launchpad", internal_macaroon_secret_key="some-secret")
258+ machine = self.factory.makeCodeImportMachine(set_online=True)
259+ code_imports = [
260+ self.factory.makeCodeImport(
261+ target_rcs_type=TargetRevisionControlSystems.GIT)
262+ for _ in range(2)]
263+ with celebrity_logged_in("vcs_imports"):
264+ jobs = [
265+ self.factory.makeCodeImportJob(code_import=code_import)
266+ for code_import in code_imports]
267+ issuer = getUtility(IMacaroonIssuer, "code-import-job")
268+ macaroons = [
269+ removeSecurityProxy(issuer).issueMacaroon(job) for job in jobs]
270+ repository = code_imports[0].git_repository
271+ auth_params = _make_auth_params(
272+ LAUNCHPAD_SERVICES, macaroons[0].serialize())
273+ pushed_branch = 'branch1'
274+ self.assertFault(
275+ faults.Unauthorized, None,
276+ "getMergeProposalURL", repository.getInternalPath(),
277+ pushed_branch, auth_params)
278+
279+ with celebrity_logged_in("vcs_imports"):
280+ getUtility(ICodeImportJobWorkflow).startJob(jobs[0], machine)
281+ auth_params = _make_auth_params(
282+ LAUNCHPAD_SERVICES, macaroons[0].serialize())
283+
284+ self.assertFault(
285+ faults.Unauthorized, None,
286+ "getMergeProposalURL",
287+ removeSecurityProxy(repository).getInternalPath(),
288+ pushed_branch, auth_params)
289+
290+ auth_params = _make_auth_params(
291+ LAUNCHPAD_SERVICES, macaroons[1].serialize())
292+
293+ self.assertFault(
294+ faults.Unauthorized, None,
295+ "getMergeProposalURL",
296+ removeSecurityProxy(repository).getInternalPath(),
297+ pushed_branch, auth_params)
298+
299+ auth_params = _make_auth_params(
300+ LAUNCHPAD_SERVICES,
301+ macaroon_raw=Macaroon(
302+ location=config.vhost.mainsite.hostname, identifier="another",
303+ key="another-secret").serialize())
304+ self.assertFault(
305+ faults.Unauthorized, None,
306+ "getMergeProposalURL",
307+ removeSecurityProxy(repository).getInternalPath(),
308+ pushed_branch, auth_params)
309+
310+ auth_params = _make_auth_params(
311+ LAUNCHPAD_SERVICES, macaroon_raw="nonsense")
312+
313+ self.assertFault(
314+ faults.Unauthorized, None,
315+ "getMergeProposalURL",
316+ removeSecurityProxy(repository).getInternalPath(),
317+ pushed_branch, auth_params)
318+
319+ def test_getMergeProposalURL_private_code_import(self):
320+ # We do not send the Merge Proposal URL back
321+ # to a Code Import Job.
322+
323+ self.pushConfig(
324+ "launchpad", internal_macaroon_secret_key="some-secret")
325+ machine = self.factory.makeCodeImportMachine(set_online=True)
326+ code_imports = [
327+ self.factory.makeCodeImport(
328+ target_rcs_type=TargetRevisionControlSystems.GIT)
329+ for _ in range(2)]
330+ private_repository = code_imports[0].git_repository
331+ removeSecurityProxy(private_repository).transitionToInformationType(
332+ InformationType.PRIVATESECURITY, private_repository.owner)
333+ with celebrity_logged_in("vcs_imports"):
334+ jobs = [
335+ self.factory.makeCodeImportJob(code_import=code_import)
336+ for code_import in code_imports]
337+ issuer = getUtility(IMacaroonIssuer, "code-import-job")
338+ macaroons = [
339+ removeSecurityProxy(issuer).issueMacaroon(job) for job in jobs]
340+ repository = code_imports[0].git_repository
341+ auth_params = _make_auth_params(
342+ LAUNCHPAD_SERVICES, macaroons[0].serialize())
343+ pushed_branch = 'branch1'
344+
345+ self.assertFault(
346+ faults.Unauthorized, None,
347+ "getMergeProposalURL",
348+ removeSecurityProxy(repository).getInternalPath(),
349+ pushed_branch, auth_params)
350+
351+ with celebrity_logged_in("vcs_imports"):
352+ getUtility(ICodeImportJobWorkflow).startJob(jobs[0], machine)
353+
354+ auth_params = _make_auth_params(
355+ LAUNCHPAD_SERVICES, macaroons[1].serialize())
356+
357+ self.assertFault(
358+ faults.Unauthorized, None,
359+ "getMergeProposalURL",
360+ removeSecurityProxy(repository).getInternalPath(),
361+ pushed_branch, auth_params)
362+
363+ auth_params = _make_auth_params(
364+ LAUNCHPAD_SERVICES, macaroon_raw=Macaroon(
365+ location=config.vhost.mainsite.hostname,
366+ identifier="another",
367+ key="another-secret").serialize())
368+
369+ self.assertFault(
370+ faults.Unauthorized, None,
371+ "getMergeProposalURL",
372+ removeSecurityProxy(repository).getInternalPath(),
373+ pushed_branch, auth_params)
374+
375+ auth_params = _make_auth_params(
376+ LAUNCHPAD_SERVICES, macaroon_raw="nonsense")
377+
378+ self.assertFault(
379+ faults.Unauthorized, None,
380+ "getMergeProposalURL",
381+ removeSecurityProxy(repository).getInternalPath(),
382+ pushed_branch, auth_params)
383+
384 def test_notify(self):
385 # The notify call creates a GitRefScanJob.
386 repository = self.factory.makeGitRepository()

Subscribers

People subscribed via source and target branches