launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

William Grant has proposed merging lp:~wgrant/launchpad/translatePath-private-404 into lp:launchpad.

Commit message:
GitAPI.translatePath now pretends that invisible private repos don't exist, and Git not found errors now say "not found".

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~wgrant/launchpad/translatePath-private-404/+merge/264375

GitAPI.translatePath now pretends that forbidden private repositories don't exist, matching the Bugs behaviour and no longer disclosing the existence of private projects. Unauthorized turns into GitRepositoryNotFound instead of PermissionDenied, and both GitRepositoryNotFound and PermissionDenied get turned into Unauthorized if further authentication is possible.

I also made the 404 error less meaningless: "Repository 'foo' not found" rather than "Could not translate 'foo'.".
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wgrant/launchpad/translatePath-private-404 into lp:launchpad.

=== modified file 'lib/lp/code/xmlrpc/git.py'
--- lib/lp/code/xmlrpc/git.py	2015-07-08 16:05:11 +0000
+++ lib/lp/code/xmlrpc/git.py	2015-07-10 06:00:22 +0000
@@ -74,7 +74,7 @@
         try:
             hosting_path = repository.getInternalPath()
         except Unauthorized:
-            raise faults.PermissionDenied()
+            return None
         writable = check_permission("launchpad.Edit", repository)
         return {
             "path": hosting_path,
@@ -236,17 +236,18 @@
                 self._createRepository(requester, path)
                 result = self._performLookup(path)
             if result is None:
-                raise faults.PathTranslationError(path)
+                raise faults.GitRepositoryNotFound(path)
             if permission != "read" and not result["writable"]:
                 raise faults.PermissionDenied()
             return result
-        except faults.PermissionDenied:
-            # Turn "permission denied" for anonymous HTTP requests into
-            # "authorisation required", so that the user-agent has a chance
-            # to try HTTP basic auth.
+        except (faults.PermissionDenied, faults.GitRepositoryNotFound):
+            # Turn lookup errors for anonymous HTTP requests into
+            # "authorisation required", so that the user-agent has a
+            # chance to try HTTP basic auth.
             if can_authenticate and requester is None:
                 raise faults.Unauthorized()
-            raise
+            else:
+                raise
 
     def translatePath(self, path, permission, requester_id, can_authenticate):
         """See `IGitAPI`."""

=== modified file 'lib/lp/code/xmlrpc/tests/test_git.py'
--- lib/lp/code/xmlrpc/tests/test_git.py	2015-07-08 16:05:11 +0000
+++ lib/lp/code/xmlrpc/tests/test_git.py	2015-07-10 06:00:22 +0000
@@ -70,14 +70,15 @@
             ZopeUtilityFixture(self.hosting_client, IGitHostingClient))
         self.repository_set = getUtility(IGitRepositorySet)
 
-    def assertPathTranslationError(self, requester, path, permission="read",
-                                   can_authenticate=False):
+    def assertGitRepositoryNotFound(self, requester, path, permission="read",
+                                    can_authenticate=False):
         """Assert that the given path cannot be translated."""
         if requester not in (LAUNCHPAD_ANONYMOUS, LAUNCHPAD_SERVICES):
             requester = requester.id
         fault = self.git_api.translatePath(
             path, permission, requester, can_authenticate)
-        self.assertEqual(faults.PathTranslationError(path.strip("/")), fault)
+        self.assertEqual(
+            faults.GitRepositoryNotFound(path.strip("/")), fault)
 
     def assertPermissionDenied(self, requester, path,
                                message="Permission denied.",
@@ -197,14 +198,14 @@
             self.factory.makeGitRepository(
                 information_type=InformationType.USERDATA))
         path = u"/%s" % repository.unique_name
-        self.assertPermissionDenied(requester, path)
+        self.assertGitRepositoryNotFound(requester, path)
 
     def test_translatePath_anonymous_cannot_see_private_repository(self):
         repository = removeSecurityProxy(
             self.factory.makeGitRepository(
                 information_type=InformationType.USERDATA))
         path = u"/%s" % repository.unique_name
-        self.assertPermissionDenied(
+        self.assertGitRepositoryNotFound(
             LAUNCHPAD_ANONYMOUS, path, can_authenticate=False)
         self.assertUnauthorized(
             LAUNCHPAD_ANONYMOUS, path, can_authenticate=True)
@@ -267,7 +268,7 @@
         # When this happens, it returns a Fault saying so, including the
         # path it couldn't translate.
         requester = self.factory.makePerson()
-        self.assertPathTranslationError(requester, u"/untranslatable")
+        self.assertGitRepositoryNotFound(requester, u"/untranslatable")
 
     def test_translatePath_repository(self):
         requester = self.factory.makePerson()
@@ -297,13 +298,13 @@
     def test_translatePath_no_such_repository(self):
         requester = self.factory.makePerson()
         path = u"/%s/+git/no-such-repository" % requester.name
-        self.assertPathTranslationError(requester, path)
+        self.assertGitRepositoryNotFound(requester, path)
 
     def test_translatePath_no_such_repository_non_ascii(self):
         requester = self.factory.makePerson()
         path = u"/%s/+git/\N{LATIN SMALL LETTER I WITH DIAERESIS}" % (
             requester.name)
-        self.assertPathTranslationError(requester, path)
+        self.assertGitRepositoryNotFound(requester, path)
 
     def test_translatePath_anonymous_public_repository(self):
         repository = self.factory.makeGitRepository()
@@ -410,10 +411,10 @@
     def test_translatePath_anonymous_cannot_create(self):
         # Anonymous users cannot create repositories.
         project = self.factory.makeProject()
-        self.assertPathTranslationError(
+        self.assertGitRepositoryNotFound(
             LAUNCHPAD_ANONYMOUS, u"/%s" % project.name,
             permission="write", can_authenticate=False)
-        self.assertPathTranslationError(
+        self.assertUnauthorized(
             LAUNCHPAD_ANONYMOUS, u"/%s" % project.name,
             permission="write", can_authenticate=True)
 

=== modified file 'lib/lp/xmlrpc/faults.py'
--- lib/lp/xmlrpc/faults.py	2015-05-04 14:56:58 +0000
+++ lib/lp/xmlrpc/faults.py	2015-07-10 06:00:22 +0000
@@ -17,6 +17,7 @@
     'CannotHaveLinkedBranch',
     'FileBugGotProductAndDistro',
     'FileBugMissingProductOrDistribution',
+    'GitRepositoryNotFound',
     'InvalidBranchIdentifier',
     'InvalidBranchName',
     'InvalidBranchUniqueName',
@@ -380,6 +381,12 @@
         LaunchpadFault.__init__(self, path=path)
 
 
+class GitRepositoryNotFound(PathTranslationError):
+    """Raised when a Git repository path lookup fails."""
+
+    msg_template = "Repository '%(path)s' not found."
+
+
 class InvalidPath(LaunchpadFault):
     """Raised when `translatePath` is passed something that's not a path."""