launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging lp:~cjwatson/launchpad/builders-visibility into lp:launchpad.

Commit message:
Render links to public builds in private archives owned by private teams as "private job".

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #760303 in Launchpad itself: "builders page inaccessible if a private recipe build is building"
  https://bugs.launchpad.net/launchpad/+bug/760303
  Bug #1193057 in Launchpad itself: "Copies of public source to private archives owned by private teams causes /builders to 403"
  https://bugs.launchpad.net/launchpad/+bug/1193057

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/builders-visibility/+merge/170818

== Summary ==

There are a couple of cases where builds can be otherwise public (copies of sources already published in public archives; recipe builds from public branches) but be for archives owned by private teams.  This causes PackageBuildFormatterAPI to fail to fetch the archive owner name, which in turn makes /builders return 403.

This has been a long-standing problem, and at least two frequent causes are known, but as far as I can tell there is just one underlying cause.  Note that the original traceback in bug 760303 was addressed by the fix for bug 728673, but the case of recipe builds from public branches in archives owned by private teams still remained.

== Proposed fix ==

Explicitly check visibility of the archive, and if that fails render the link to the build as "private job".  It's close enough and much safer.

== LOC Rationale ==

+34

I think this is tolerable to address a frequent operational issue which causes a good deal of complaint on LP-related channels.

== Tests ==

bin/test -vvct lp.app.tests.test_tales.TestPackageBuildFormatterAPI

== Demo and Q/A ==

Copy a public SPPH to a private archive owned by a private team on DF and set it building; check that /builders is still visible when logged out.
-- 
https://code.launchpad.net/~cjwatson/launchpad/builders-visibility/+merge/170818
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/builders-visibility into lp:launchpad.

=== modified file 'lib/lp/app/browser/tales.py'
--- lib/lp/app/browser/tales.py	2013-05-02 00:40:14 +0000
+++ lib/lp/app/browser/tales.py	2013-06-21 13:54:27 +0000
@@ -1749,7 +1749,8 @@
 
     def link(self, view_name, rootsite=None):
         build = self._context
-        if not check_permission('launchpad.View', build):
+        if (not check_permission('launchpad.View', build) or
+            not check_permission('launchpad.View', build.archive)):
             return 'private job'
 
         url = self.url(view_name=view_name, rootsite=rootsite)

=== modified file 'lib/lp/app/tests/test_tales.py'
--- lib/lp/app/tests/test_tales.py	2012-11-14 18:02:13 +0000
+++ lib/lp/app/tests/test_tales.py	2013-06-21 13:54:27 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2010 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2013 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """tales.py doctests."""
@@ -28,6 +28,7 @@
 from lp.registry.interfaces.irc import IIrcIDSet
 from lp.registry.interfaces.person import PersonVisibility
 from lp.services.webapp.authorization import (
+    check_permission,
     clear_cache,
     precache_permission_for_objects,
     )
@@ -474,3 +475,35 @@
         """Values in seconds are reported as "less than a minute."""
         self.assertEqual('less than a minute',
             self.getDurationsince(timedelta(0, 59)))
+
+
+class TestPackageBuildFormatterAPI(TestCaseWithFactory):
+    """Tests for PackageBuildFormatterAPI."""
+
+    layer = LaunchpadFunctionalLayer
+
+    def _make_public_build_for_private_team(self):
+        spph = self.factory.makeSourcePackagePublishingHistory()
+        team_owner = self.factory.makePerson()
+        private_team = self.factory.makeTeam(
+            owner=team_owner, visibility=PersonVisibility.PRIVATE)
+        p3a = self.factory.makeArchive(owner=private_team, private=True)
+        build = self.factory.makeBinaryPackageBuild(
+            source_package_release=spph.sourcepackagerelease, archive=p3a)
+        return build, p3a, team_owner
+
+    def test_public_build_private_team_no_permission(self):
+        # A `PackageBuild` for a public `SourcePackageRelease` in an archive
+        # for a private team is rendered gracefully when the user has no
+        # permission.
+        build, _, _ = self._make_public_build_for_private_team()
+        # Make sure this is a valid test; the build itself must be public.
+        self.assertTrue(check_permission('launchpad.View', build))
+        self.assertEqual('private job', format_link(build))
+
+    def test_public_build_private_team_with_permission(self):
+        # Members of a private team can see their builds.
+        build, p3a, team_owner = self._make_public_build_for_private_team()
+        login_person(team_owner)
+        self.assertIn(
+            "[%s/%s]" % (p3a.owner.name, p3a.name), format_link(build))