Merge lp:~stevenk/launchpad/subscribers-can-view-p3as into lp:launchpad

[Steve Kowalik]

Allow subscribers to view P3As, but do not allow them to view +packages. Add tests that check that the links they receive are disabled, and that they receive 403 errors when they browse there directly.

[Robert Collins (lifeless)]

Just a small note on the vertical whitespace mid function: PEP8 says
to use that sparingly to separate logically distinct areas of a single
function; the 'list of things to check' is one big area IMO - I would
delete the vertical white space there.

-Rob

[Jelmer Vernooij (jelmer)]

Thanks, good stuff.

There is only one empty line above TestPPAPackages, there should be two.

The test methods in TestP3APackages also contain the phrase p3a_packages again, this seems a bit superfluous given the test case name.

The comments on the first line of each test method explaining what the tests do could perhaps be docstrings.

review: Approve (code)

=== modified file 'lib/canonical/launchpad/security.py'
--- lib/canonical/launchpad/security.py	2010-06-16 18:49:47 +0000
+++ lib/canonical/launchpad/security.py	2010-06-24 14:19:31 +0000
@@ -19,7 +19,7 @@
     IArchivePermissionSet)
 from lp.soyuz.interfaces.archiveauthtoken import IArchiveAuthToken
 from lp.soyuz.interfaces.archivesubscriber import (
-    IArchiveSubscriber, IPersonalArchiveSubscription)
+    IArchiveSubscriber, IArchiveSubscriberSet, IPersonalArchiveSubscription)
 from lp.code.interfaces.branch import (
     IBranch, user_has_special_branch_access)
 from lp.code.interfaces.branchmergeproposal import (
@@ -2040,6 +2040,13 @@
         if self.obj.is_ppa and self.obj.checkArchivePermission(user.person):
             return True
 
+        # Subscribers can view private PPAs.
+        if self.obj.is_ppa and self.obj.private:
+            archive_subs = getUtility(IArchiveSubscriberSet).getBySubscriber(
+                user.person, self.obj).any()
+            if archive_subs:
+                return True
+
         return False
 
     def checkUnauthenticated(self):

=== modified file 'lib/lp/soyuz/browser/archive.py'
--- lib/lp/soyuz/browser/archive.py	2010-06-14 09:31:26 +0000
+++ lib/lp/soyuz/browser/archive.py	2010-06-24 14:19:31 +0000
@@ -35,6 +35,7 @@
 from zope.component import getUtility
 from zope.formlib import form
 from zope.interface import implements, Interface
+from zope.security.interfaces import Unauthorized
 from zope.security.proxy import removeSecurityProxy
 from zope.schema import Choice, List, TextLine
 from zope.schema.interfaces import IContextSourceBinder
@@ -426,7 +427,12 @@
 
     def packages(self):
         text = 'View package details'
-        return Link('+packages', text, icon='info')
+        link = Link('+packages', text, icon='info')
+        # Disable the link for P3As if they don't have upload rights.
+        if self.context.private:
+            if not check_permission('launchpad.Append', self.context):
+                link.enabled = False
+        return link
 
     @enabled_with_permission('launchpad.Edit')
     def delete(self):
@@ -500,6 +506,10 @@
     """Common features for Archive view classes."""
 
     @cachedproperty
+    def private(self):
+        return self.context.private
+
+    @cachedproperty
     def has_sources(self):
         """Whether or not this PPA has any sources for the view.
 
@@ -960,6 +970,12 @@
     """Detailed packages view for an archive."""
     implements(IArchivePackagesActionMenu)
 
+    def initialize(self):
+        super(ArchivePackagesView, self).initialize()
+        if self.context.private:
+            if not check_permission('launchpad.Append', self.context):
+                raise Unauthorized
+
     @property
     def page_title(self):
         return smartquote('Packages in "%s"' % self.context.displayname)

=== added file 'lib/lp/soyuz/browser/tests/test_archive_packages.py'
--- lib/lp/soyuz/browser/tests/test_archive_packages.py	1970-01-01 00:00:00 +0000
+++ lib/lp/soyuz/browser/tests/test_archive_packages.py	2010-06-24 14:19:31 +0000
@@ -0,0 +1,101 @@
+# Copyright 2010 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+# pylint: disable-msg=F0401
+
+"""Unit tests for TestP3APackages."""
+
+__metaclass__ = type
+__all__ = [
+    'TestP3APackages',
+    'TestPPAPackages',
+    'test_suite',
+    ]
+
+import unittest
+
+from zope.security.interfaces import Unauthorized
+
+from canonical.testing import LaunchpadFunctionalLayer
+from lp.soyuz.browser.archive import ArchiveNavigationMenu
+from lp.testing import login, login_person, TestCaseWithFactory
+from lp.testing.views import create_initialized_view
+
+
+class TestP3APackages(TestCaseWithFactory):
+    """P3A archive pages are rendered correctly."""
+
+    layer = LaunchpadFunctionalLayer
+
+    def setUp(self):
+        super(TestP3APackages, self).setUp()
+        self.private_ppa = self.factory.makeArchive(description='Foo')
+        login('[e-mail]')
+        self.private_ppa.buildd_secret = 'blah'
+        self.private_ppa.private = True
+        self.joe = self.factory.makePerson(name='joe')
+        self.fred = self.factory.makePerson(name='fred')
+        self.mary = self.factory.makePerson(name='mary')
+        login_person(self.private_ppa.owner)
+        self.private_ppa.newSubscription(self.joe, self.private_ppa.owner)
+        self.private_ppa.newComponentUploader(self.mary, 'main')
+
+    def test_packages_unauthorized(self):
+        """A person with no subscription will not be able to view +packages
+        """
+        login_person(self.fred)
+        self.assertRaises(
+            Unauthorized, create_initialized_view, self.private_ppa,
+            "+packages")
+
+    def test_packages_unauthorized_subscriber(self):
+        """A person with a subscription will not be able to view +packages
+        """
+        login_person(self.joe)
+        self.assertRaises(
+            Unauthorized, create_initialized_view, self.private_ppa,
+            "+packages")
+
+    def test_packages_authorized(self):
+        """A person with launchpad.{Append,Edit} will be able to do so"""
+        login_person(self.private_ppa.owner)
+        view = create_initialized_view(self.private_ppa, "+packages")
+        menu = ArchiveNavigationMenu(view)
+        self.assertTrue(menu.packages().enabled)
+
+    def test_packages_uploader(self):
+        """A person with launchpad.Append will also be able to do so"""
+        login_person(self.mary)
+        view = create_initialized_view(self.private_ppa, "+packages")
+        menu = ArchiveNavigationMenu(view)
+        self.assertTrue(menu.packages().enabled)
+
+    def test_packages_link_unauthorized(self):
+        login_person(self.fred)
+        view = create_initialized_view(self.private_ppa, "+index")
+        menu = ArchiveNavigationMenu(view)
+        self.assertFalse(menu.packages().enabled)
+
+    def test_packages_link_subscriber(self):
+        login_person(self.joe)
+        view = create_initialized_view(self.private_ppa, "+index")
+        menu = ArchiveNavigationMenu(view)
+        self.assertFalse(menu.packages().enabled)
+
+
+class TestPPAPackages(TestCaseWithFactory):
+    layer = LaunchpadFunctionalLayer
+
+    def setUp(self):
+        super(TestPPAPackages, self).setUp()
+        self.joe = self.factory.makePerson(name='joe')
+        self.ppa = self.factory.makeArchive()
+
+    def test_ppa_packages(self):
+        login_person(self.joe)
+        view = create_initialized_view(self.ppa, "+index")
+        menu = ArchiveNavigationMenu(view)
+        self.assertTrue(menu.packages().enabled)
+
+def test_suite():
+    return unittest.TestLoader().loadTestsFromName(__name__)

=== added file 'lib/lp/soyuz/tests/test_archive_privacy.py'
--- lib/lp/soyuz/tests/test_archive_privacy.py	1970-01-01 00:00:00 +0000
+++ lib/lp/soyuz/tests/test_archive_privacy.py	2010-06-24 14:19:31 +0000
@@ -0,0 +1,40 @@
+# Copyright 2010 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Test Archive privacy features."""
+
+from zope.component import getUtility
+from zope.security.interfaces import Unauthorized
+from lp.soyuz.interfaces.archive import IArchiveSet
+
+from canonical.testing import LaunchpadFunctionalLayer
+from lp.testing import login, login_person, TestCaseWithFactory
+
+
+class TestArchivePrivacy(TestCaseWithFactory):
+    layer = LaunchpadFunctionalLayer
+
+    def setUp(self):
+        super(TestArchivePrivacy, self).setUp()
+        self.private_ppa = self.factory.makeArchive(description='Foo')
+        login('[e-mail]')
+        self.private_ppa.buildd_secret = 'blah'
+        self.private_ppa.private = True
+        self.joe = self.factory.makePerson(name='joe')
+        self.fred = self.factory.makePerson(name='fred')
+        login_person(self.private_ppa.owner)
+        self.private_ppa.newSubscription(self.joe, self.private_ppa.owner)
+
+    def _getDescription(self, p3a):
+        return p3a.description
+
+    def test_no_subscription(self):
+        login_person(self.fred)
+        p3a = getUtility(IArchiveSet).get(self.private_ppa.id)
+        self.assertRaises(Unauthorized, self._getDescription, p3a)
+
+    def test_subscription(self):
+        login_person(self.joe)
+        p3a = getUtility(IArchiveSet).get(self.private_ppa.id)
+        self.assertEqual(self._getDescription(p3a), "Foo")
+