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

[Ioana Lasc]

Add MP URL to git push

[Ioana Lasc (ilasc)]

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

d9bdd8f... by Ioana Lasc on 2020-01-08

Addressed review comments and added unit tests.

[Ioana Lasc (ilasc)]

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)]

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)]

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)]

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)]

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)]

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 on 2020-03-31

Remove commented out code

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

diff --git a/lib/lp/code/interfaces/gitapi.py b/lib/lp/code/interfaces/gitapi.py
index c7db491..c289dee 100644
--- a/lib/lp/code/interfaces/gitapi.py
+++ b/lib/lp/code/interfaces/gitapi.py
@@ -79,3 +79,16 @@ class IGitAPI(Interface):
 
         :returns: A list of rules for the user in the specified repository
         """
+
+    def getMergeProposalURL(translated_path, branch, auth_params):
+        """Return the URL for a merge proposal.
+
+        When a `branch` that is not the default branch in a repository
+        is pushed, the URL where the merge proposal for that branch can
+        be opened will be generated and returned.
+
+        :returns: The URL to register a merge proposal for the branch in the
+            specified repository. A `GitRepositoryNotFound` fault is returned
+            if no repository can be found for 'translated_path',
+            or an `Unauthorized` fault for unauthorized push attempts.
+        """
diff --git a/lib/lp/code/xmlrpc/git.py b/lib/lp/code/xmlrpc/git.py
index cbf9205..b34307a 100644
--- a/lib/lp/code/xmlrpc/git.py
+++ b/lib/lp/code/xmlrpc/git.py
@@ -15,6 +15,7 @@ import uuid
 from pymacaroons import Macaroon
 import six
 from six.moves import xmlrpc_client
+from six.moves.urllib.parse import quote
 import transaction
 from zope.component import (
     ComponentLookupError,
@@ -72,7 +73,10 @@ from lp.services.macaroons.interfaces import (
     IMacaroonIssuer,
     NO_USER,
     )
-from lp.services.webapp import LaunchpadXMLRPCView
+from lp.services.webapp import (
+    canonical_url,
+    LaunchpadXMLRPCView,
+    )
 from lp.services.webapp.authorization import check_permission
 from lp.services.webapp.errorlog import ScriptRequest
 from lp.xmlrpc import faults
@@ -427,6 +431,47 @@ class GitAPI(LaunchpadXMLRPCView):
         logger.info("notify succeeded")
 
     @return_fault
+    def _getMergeProposalURL(self, requester, translated_path, branch,
+                             auth_params):
+        if requester == LAUNCHPAD_ANONYMOUS:
+            requester = None
+        repository = removeSecurityProxy(
+            getUtility(IGitLookup).getByHostingPath(translated_path))
+        if repository is None:
+            raise faults.GitRepositoryNotFound(translated_path)
+
+        verified = self._verifyAuthParams(requester, repository, auth_params)
+        if verified is not None and verified.user is NO_USER:
+            # Showing a merge proposal URL may be useful to ordinary users,
+            # but it doesn't make sense in the context of an internal service.
+            return None
+
+        # We assemble the URL this way here because the ref may not exist yet.
+        base_url = canonical_url(repository, rootsite='code')
+        mp_url = "%s/+ref/%s/+register-merge" % (
+            base_url, quote(branch))
+        return mp_url
+
+    def getMergeProposalURL(self, translated_path, branch, auth_params):
+        """See `IGitAPI`."""
+        logger = self._getLogger(auth_params.get("request-id"))
+        requester_id = _get_requester_id(auth_params)
+        logger.info(
+            "Request received: getMergeProposalURL('%s, %s') for %s",
+            translated_path, branch, requester_id)
+        result = run_with_login(
+            requester_id, self._getMergeProposalURL,
+            translated_path, branch, auth_params)
+        if isinstance(result, xmlrpc_client.Fault):
+            logger.error("getMergeProposalURL failed: %r", result)
+        else:
+            # The result of getMergeProposalURL is not sensitive for logging
+            # purposes (it may refer to private artifacts, but contains no
+            # credentials, only the merge proposal URL).
+            logger.info("getMergeProposalURL succeeded: %s" % result)
+        return result
+
+    @return_fault
     def _authenticateWithPassword(self, username, password):
         """Authenticate a user by username and password.
 
diff --git a/lib/lp/code/xmlrpc/tests/test_git.py b/lib/lp/code/xmlrpc/tests/test_git.py
index df65f0e..6f12f1f 100644
--- a/lib/lp/code/xmlrpc/tests/test_git.py
+++ b/lib/lp/code/xmlrpc/tests/test_git.py
@@ -1,4 +1,4 @@
-# Copyright 2015-2019 Canonical Ltd.  This software is licensed under the
+# Copyright 2015-2020 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Tests for the internal Git API."""
@@ -11,6 +11,7 @@ from fixtures import FakeLogger
 from pymacaroons import Macaroon
 import six
 from six.moves import xmlrpc_client
+from six.moves.urllib.parse import quote
 from testtools.matchers import (
     Equals,
     IsInstance,
@@ -51,6 +52,7 @@ from lp.services.macaroons.interfaces import (
     NO_USER,
     )
 from lp.services.macaroons.model import MacaroonIssuerBase
+from lp.services.webapp import canonical_url
 from lp.services.webapp.escaping import html_escape
 from lp.snappy.interfaces.snap import SNAP_TESTING_FLAGS
 from lp.testing import (
@@ -326,6 +328,15 @@ class TestGitAPIMixin:
                 ])
             for ref_path, ref_permissions in permissions.items())))
 
+    def assertHasMergeProposalURL(self, repository,
+                                  pushed_branch, auth_params):
+        base_url = canonical_url(repository, rootsite='code')
+        expected_mp_url = "%s/+ref/%s/+register-merge" % (
+            base_url, quote(pushed_branch))
+        result = self.git_api.getMergeProposalURL(
+            repository.getInternalPath(), pushed_branch, auth_params)
+        self.assertEqual(expected_mp_url, result)
+
     def test_translatePath_private_repository(self):
         requester = self.factory.makePerson()
         repository = removeSecurityProxy(
@@ -636,6 +647,13 @@ class TestGitAPIMixin:
             faults.GitRepositoryNotFound("nonexistent"), None,
             "checkRefPermissions", "nonexistent", [], {"uid": requester.id})
 
+    def test_getMergeProposalURL__nonexistent_repository(self):
+        requester = self.factory.makePerson()
+        self.assertFault(
+            faults.GitRepositoryNotFound("nonexistent"), None,
+            "getMergeProposalURL", "nonexistent", 'branch1',
+            {"uid": requester.id})
+
 
 class TestGitAPI(TestGitAPIMixin, TestCaseWithFactory):
     """Tests for the implementation of `IGitAPI`."""
@@ -1265,6 +1283,236 @@ class TestGitAPI(TestGitAPIMixin, TestCaseWithFactory):
                     requester, path, permission="read",
                     macaroon_raw=macaroon.serialize())
 
+    def test_getMergeProposalURL_user(self):
+        # A merge proposal URL is returned by LP for a non-default branch
+        # pushed by a user that has their ordinary privileges on the
+        # corresponding repository.
+        requester_owner = self.factory.makePerson()
+        repository = self.factory.makeGitRepository(owner=requester_owner)
+        self.factory.makeGitRefs(
+            repository=repository, paths=[u'refs/heads/master'])
+        removeSecurityProxy(repository).default_branch = u'refs/heads/master'
+        pushed_branch = 'branch1'
+        self.assertHasMergeProposalURL(repository, pushed_branch,
+                                       {"uid": requester_owner.id})
+
+        # Turnip verifies if the pushed branch is the default branch
+        # in a repository and calls LP only for non default branches.
+        # Consequently LP will process any incoming branch from Turnip
+        # as being non default and produce a merge proposal URL for it.
+        self.factory.makeGitRefs(
+            repository=repository, paths=[u'refs/heads/%s' % pushed_branch])
+        removeSecurityProxy(repository).default_branch = (
+                u'refs/heads/%s' % pushed_branch)
+        self.assertHasMergeProposalURL(repository, pushed_branch,
+                                       {"uid": requester_owner.id})
+
+        requester_non_owner = self.factory.makePerson()
+        self.assertHasMergeProposalURL(repository, pushed_branch,
+                                       {"uid": requester_non_owner.id})
+
+    def test_getMergeProposalURL_user_macaroon(self):
+        # The merge proposal URL is returned by LP for a non-default branch
+        # pushed by a user with a suitable macaroon that
+        # has their ordinary privileges on the corresponding repository.
+
+        self.pushConfig("codehosting", git_macaroon_secret_key="some-secret")
+        requester = self.factory.makePerson()
+        repository = self.factory.makeGitRepository(owner=requester)
+        issuer = getUtility(IMacaroonIssuer, "git-repository")
+        self.factory.makeGitRefs(
+            repository=repository, paths=[u'refs/heads/master'])
+        removeSecurityProxy(repository).default_branch = u'refs/heads/master'
+
+        pushed_branch = 'branch1'
+        with person_logged_in(requester):
+            macaroon = removeSecurityProxy(issuer).issueMacaroon(
+                    repository, user=requester)
+        auth_params = _make_auth_params(requester,
+                                        macaroon_raw=macaroon.serialize())
+        self.assertHasMergeProposalURL(repository, pushed_branch, auth_params)
+
+    def test_getMergeProposalURL_user_mismatch(self):
+        # getMergeProposalURL refuses macaroons in the case where the
+        # user doesn't match what the issuer claims was verified.  (We use a
+        # dummy issuer for this, since this is a stopgap check to defend
+        # against issuer bugs)
+
+        issuer = DummyMacaroonIssuer()
+        # Claim to be the code-import-job issuer.  This is a bit weird, but
+        # it gets us past the difficulty that only certain named issuers are
+        # allowed to confer write permissions.
+        issuer.identifier = "code-import-job"
+        self.useFixture(ZopeUtilityFixture(
+            issuer, IMacaroonIssuer, name="code-import-job"))
+        requesters = [self.factory.makePerson() for _ in range(2)]
+        owner = self.factory.makeTeam(members=requesters)
+        repository = self.factory.makeGitRepository(owner=owner)
+        self.factory.makeGitRefs(
+            repository=repository, paths=[u'refs/heads/master'])
+        removeSecurityProxy(repository).default_branch = u'refs/heads/master'
+        pushed_branch = 'branch1'
+        macaroon = issuer.issueMacaroon(repository)
+
+        for verified_user, authorized, unauthorized in (
+                (requesters[0], [requesters[0]],
+                 [LAUNCHPAD_SERVICES, requesters[1], None]),
+                ([None, NO_USER], [],
+                 [LAUNCHPAD_SERVICES] + requesters + [None]),
+        ):
+            issuer._verified_user = verified_user
+            for requester in authorized:
+                login(ANONYMOUS)
+                auth_params = _make_auth_params(
+                     requester, macaroon_raw=macaroon.serialize())
+                self.assertHasMergeProposalURL(repository,
+                                               pushed_branch, auth_params)
+            for requester in unauthorized:
+                login(ANONYMOUS)
+                auth_params = _make_auth_params(
+                    requester, macaroon_raw=macaroon.serialize())
+
+                self.assertFault(
+                    faults.Unauthorized, None,
+                    "getMergeProposalURL", repository.getInternalPath(),
+                    pushed_branch, auth_params)
+
+    def test_getMergeProposalURL_code_import(self):
+        # A merge proposal URL from LP to Turnip is not returned for
+        # code import job as there is no User at the other end.
+
+        issuer = DummyMacaroonIssuer()
+        # Claim to be the code-import-job issuer.  This is a bit weird, but
+        # it gets us past the difficulty that only certain named issuers are
+        # allowed to confer write permissions.
+        self.pushConfig(
+            "launchpad", internal_macaroon_secret_key="some-secret")
+        machine = self.factory.makeCodeImportMachine(set_online=True)
+        code_imports = [
+            self.factory.makeCodeImport(
+                target_rcs_type=TargetRevisionControlSystems.GIT)
+            for _ in range(2)]
+        with celebrity_logged_in("vcs_imports"):
+            jobs = [
+                self.factory.makeCodeImportJob(code_import=code_import)
+                for code_import in code_imports]
+        issuer = getUtility(IMacaroonIssuer, "code-import-job")
+        macaroons = [
+            removeSecurityProxy(issuer).issueMacaroon(job) for job in jobs]
+        repository = code_imports[0].git_repository
+        auth_params = _make_auth_params(
+            LAUNCHPAD_SERVICES, macaroons[0].serialize())
+        pushed_branch = 'branch1'
+        self.assertFault(
+            faults.Unauthorized, None,
+            "getMergeProposalURL", repository.getInternalPath(),
+            pushed_branch, auth_params)
+
+        with celebrity_logged_in("vcs_imports"):
+            getUtility(ICodeImportJobWorkflow).startJob(jobs[0], machine)
+        auth_params = _make_auth_params(
+            LAUNCHPAD_SERVICES, macaroons[0].serialize())
+
+        self.assertFault(
+            faults.Unauthorized, None,
+            "getMergeProposalURL",
+            removeSecurityProxy(repository).getInternalPath(),
+            pushed_branch, auth_params)
+
+        auth_params = _make_auth_params(
+            LAUNCHPAD_SERVICES, macaroons[1].serialize())
+
+        self.assertFault(
+            faults.Unauthorized, None,
+            "getMergeProposalURL",
+            removeSecurityProxy(repository).getInternalPath(),
+            pushed_branch, auth_params)
+
+        auth_params = _make_auth_params(
+            LAUNCHPAD_SERVICES,
+            macaroon_raw=Macaroon(
+                location=config.vhost.mainsite.hostname, identifier="another",
+                key="another-secret").serialize())
+        self.assertFault(
+            faults.Unauthorized, None,
+            "getMergeProposalURL",
+            removeSecurityProxy(repository).getInternalPath(),
+            pushed_branch, auth_params)
+
+        auth_params = _make_auth_params(
+            LAUNCHPAD_SERVICES, macaroon_raw="nonsense")
+
+        self.assertFault(
+            faults.Unauthorized, None,
+            "getMergeProposalURL",
+            removeSecurityProxy(repository).getInternalPath(),
+            pushed_branch, auth_params)
+
+    def test_getMergeProposalURL_private_code_import(self):
+        # We do not send the Merge Proposal URL back
+        # to a Code Import Job.
+
+        self.pushConfig(
+            "launchpad", internal_macaroon_secret_key="some-secret")
+        machine = self.factory.makeCodeImportMachine(set_online=True)
+        code_imports = [
+            self.factory.makeCodeImport(
+                target_rcs_type=TargetRevisionControlSystems.GIT)
+            for _ in range(2)]
+        private_repository = code_imports[0].git_repository
+        removeSecurityProxy(private_repository).transitionToInformationType(
+            InformationType.PRIVATESECURITY, private_repository.owner)
+        with celebrity_logged_in("vcs_imports"):
+            jobs = [
+                self.factory.makeCodeImportJob(code_import=code_import)
+                for code_import in code_imports]
+        issuer = getUtility(IMacaroonIssuer, "code-import-job")
+        macaroons = [
+            removeSecurityProxy(issuer).issueMacaroon(job) for job in jobs]
+        repository = code_imports[0].git_repository
+        auth_params = _make_auth_params(
+            LAUNCHPAD_SERVICES, macaroons[0].serialize())
+        pushed_branch = 'branch1'
+
+        self.assertFault(
+            faults.Unauthorized, None,
+            "getMergeProposalURL",
+            removeSecurityProxy(repository).getInternalPath(),
+            pushed_branch, auth_params)
+
+        with celebrity_logged_in("vcs_imports"):
+            getUtility(ICodeImportJobWorkflow).startJob(jobs[0], machine)
+
+        auth_params = _make_auth_params(
+            LAUNCHPAD_SERVICES, macaroons[1].serialize())
+
+        self.assertFault(
+            faults.Unauthorized, None,
+            "getMergeProposalURL",
+            removeSecurityProxy(repository).getInternalPath(),
+            pushed_branch, auth_params)
+
+        auth_params = _make_auth_params(
+            LAUNCHPAD_SERVICES, macaroon_raw=Macaroon(
+                 location=config.vhost.mainsite.hostname,
+                 identifier="another",
+                 key="another-secret").serialize())
+
+        self.assertFault(
+            faults.Unauthorized, None,
+            "getMergeProposalURL",
+            removeSecurityProxy(repository).getInternalPath(),
+            pushed_branch, auth_params)
+
+        auth_params = _make_auth_params(
+            LAUNCHPAD_SERVICES, macaroon_raw="nonsense")
+
+        self.assertFault(
+            faults.Unauthorized, None,
+            "getMergeProposalURL",
+            removeSecurityProxy(repository).getInternalPath(),
+            pushed_branch, auth_params)
+
     def test_notify(self):
         # The notify call creates a GitRefScanJob.
         repository = self.factory.makeGitRepository()