launchpad-reviewers team mailing list archive

["Thiago F. Pappacena" <pappacena@xxxxxxxxx>]

Thiago F. Pappacena has proposed merging ~pappacena/launchpad:getCodebrowseUrl-with-username-and-password into launchpad:master with ~pappacena/launchpad:ocirecipebuild-macaroon-issuer as a prerequisite.

Commit message:
Refactoring authenticated git repository URL creation

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~pappacena/launchpad/+git/launchpad/+merge/396948
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~pappacena/launchpad:getCodebrowseUrl-with-username-and-password into launchpad:master.

diff --git a/lib/lp/code/interfaces/gitrepository.py b/lib/lp/code/interfaces/gitrepository.py
index 8c90059..cdb9b27 100644
--- a/lib/lp/code/interfaces/gitrepository.py
+++ b/lib/lp/code/interfaces/gitrepository.py
@@ -1,4 +1,4 @@
-# Copyright 2015-2020 Canonical Ltd.  This software is licensed under the
+# Copyright 2015-2021 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Git repository interfaces."""
@@ -376,8 +376,12 @@ class IGitRepositoryView(IHasRecipes):
             otherwise False.
         """
 
-    def getCodebrowseUrl():
-        """Construct a browsing URL for this Git repository."""
+    def getCodebrowseUrl(username=None, password=None):
+        """Construct a browsing URL for this Git repository.
+
+        :param username: Include the given username in the URL (optional).
+        :param password: Include the given password in the URL (optional).
+        """
 
     def getCodebrowseUrlForRevision(commit):
         """The URL to the commit of the merge to the target branch"""
diff --git a/lib/lp/code/model/gitrepository.py b/lib/lp/code/model/gitrepository.py
index 7edd906..91c7264 100644
--- a/lib/lp/code/model/gitrepository.py
+++ b/lib/lp/code/model/gitrepository.py
@@ -1,4 +1,4 @@
-# Copyright 2015-2020 Canonical Ltd.  This software is licensed under the
+# Copyright 2015-2021 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 __metaclass__ = type
@@ -33,7 +33,11 @@ from lazr.lifecycle.event import ObjectModifiedEvent
 from lazr.lifecycle.snapshot import Snapshot
 import pytz
 import six
-from six.moves.urllib.parse import quote_plus
+from six.moves.urllib.parse import (
+    quote_plus,
+    urlsplit,
+    urlunsplit,
+    )
 from storm.databases.postgres import Returning
 from storm.expr import (
     And,
@@ -534,10 +538,22 @@ class GitRepository(StormBase, WebhookTargetMixin, GitIdentityMixin):
         # See also `IGitLookup.getByHostingPath`.
         return str(self.id)
 
-    def getCodebrowseUrl(self):
+    def getCodebrowseUrl(self, username=None, password=None):
         """See `IGitRepository`."""
-        return urlutils.join(
+        url = urlutils.join(
             config.codehosting.git_browse_root, self.shortened_path)
+        if username is None and password is None:
+            return url
+        # XXX cjwatson 2019-03-07: This is ugly and needs
+        # refactoring once we support more general HTTPS
+        # authentication; see also comment in
+        # GitRepository.git_https_url.
+        split = urlsplit(url)
+        netloc = "%s:%s@%s" % (username or "", password or "", split.hostname)
+        if split.port:
+            netloc += ":%s" % split.port
+        return urlunsplit([
+            split.scheme, netloc, split.path, "", ""])
 
     def getCodebrowseUrlForRevision(self, commit):
         return "%s/commit/?id=%s" % (
diff --git a/lib/lp/code/model/tests/test_gitrepository.py b/lib/lp/code/model/tests/test_gitrepository.py
index 5ae164b..ce52929 100644
--- a/lib/lp/code/model/tests/test_gitrepository.py
+++ b/lib/lp/code/model/tests/test_gitrepository.py
@@ -2,7 +2,7 @@
 # NOTE: The first line above must stay first; do not move the copyright
 # notice to the top.  See http://www.python.org/dev/peps/pep-0263/.
 #
-# Copyright 2015-2020 Canonical Ltd.  This software is licensed under the
+# Copyright 2015-2021 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Tests for Git repositories."""
@@ -1335,6 +1335,23 @@ class TestGitRepositoryURLs(TestCaseWithFactory):
             config.codehosting.git_browse_root, repository.shortened_path)
         self.assertEqual(expected_url, repository.getCodebrowseUrl())
 
+    def test_codebrowser_url_with_username_and_password(self):
+        self.pushConfig(
+            "codehosting", git_browse_root="http://git.launchpad.test:99";)
+        repository = self.factory.makeGitRepository()
+        expected_url = lambda usr, passw: urlutils.join(
+            "http://%s:%s@xxxxxxxxxxxxxxxxxx:99"; % (usr, passw),
+            repository.shortened_path)
+        self.assertEqual(
+            expected_url("foo", "bar"),
+            repository.getCodebrowseUrl("foo", "bar"))
+        self.assertEqual(
+            expected_url("", "bar"),
+            repository.getCodebrowseUrl(None, "bar"))
+        self.assertEqual(
+            expected_url("foo", ""),
+            repository.getCodebrowseUrl("foo", ""))
+
     def test_codebrowse_url_for_default(self):
         # The codebrowse URL for the default repository for a target is an
         # 'https' URL based on the repository's shortened path.
diff --git a/lib/lp/snappy/model/snapbuildbehaviour.py b/lib/lp/snappy/model/snapbuildbehaviour.py
index f9bd56c..cdb374f 100644
--- a/lib/lp/snappy/model/snapbuildbehaviour.py
+++ b/lib/lp/snappy/model/snapbuildbehaviour.py
@@ -1,4 +1,4 @@
-# Copyright 2015-2020 Canonical Ltd.  This software is licensed under the
+# Copyright 2015-2021 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """An `IBuildFarmJobBehaviour` for `SnapBuild`.
@@ -17,10 +17,6 @@ __all__ = [
 import base64
 import time
 
-from six.moves.urllib.parse import (
-    urlsplit,
-    urlunsplit,
-    )
 from twisted.internet import defer
 from zope.component import adapter
 from zope.interface import implementer
@@ -190,16 +186,9 @@ class SnapBuildBehaviour(SnapProxyMixin, BuildFarmJobBehaviourBase):
                     self._authserver.callRemote(
                         "issueMacaroon", "snap-build", "SnapBuild", build.id),
                     config.builddmaster.authentication_timeout)
-                # XXX cjwatson 2019-03-07: This is ugly and needs
-                # refactoring once we support more general HTTPS
-                # authentication; see also comment in
-                # GitRepository.git_https_url.
-                split = urlsplit(build.snap.git_repository.getCodebrowseUrl())
-                netloc = ":%s@%s" % (macaroon_raw, split.hostname)
-                if split.port:
-                    netloc += ":%s" % split.port
-                args["git_repository"] = urlunsplit([
-                    split.scheme, netloc, split.path, "", ""])
+                url = build.snap.git_repository.getCodebrowseUrl(
+                    username=None, password=macaroon_raw)
+                args["git_repository"] = url
             else:
                 args["git_repository"] = (
                     build.snap.git_repository.git_https_url)