Merge lp:~adeuring/launchpad/bug-1086043 into lp:launchpad

[Abel Deuring]

Use the permission launchpad.LimitedView for some attributes of IMilestone and IProductSeries.

This branch fixes 1086043: https://bugs.launchpad.net/~ broken
if the user subscribed to a bug with a task for a series of a
private project.

The cause of this bug is obvious: A few properties of a product
series and a milestone are displayed in bug listings, so the current
user needs to have the permission to access these properties, even
it they are allowed to view all details about the series/milestone.

In other words: These properties should be proected by the permission
lp.LimitedView, not lp.View. (Before we noicticed this bug, I would
have sworn that we already implemented this...)

Implementation details:

I added the new test class TestPersonBugListing, which adds
tests to render a person's bug page when the user is subscribed to
a bug related to a private project.

test_grant_for_bug_with_task_for_private_product() passes without further
changes, but the two other tests (where a bug task for a product series
exists, or the bug task is linked to a milestone) needed a few more
changes:

- registry/configure.zcml now requires the permission
  lp.LimitedView for some properties of IProductSeries and IMilestone;
- new interface class IProductSeriesLimitedView which defines the
  attributes that are protected by lp.LimitedView. (There no
  corresponding change for IMilestone, because the permissions
  are specified attribute-by-attribute in configure.zcml)
- new security adapters for lp.LimitedView and IProductSeries/IMilestone
- permision and access tests for IMilestone/IProductSeries updated/
  extended.

tests:

./bin/test registry -vvt test_productseries.ProductSeriesSecurityAdaperTestCase
./bin/test bugs -vvt lp.bugs.browser.tests.test_buglisting.TestPersonBugListing.test_grant
./bin/test registry -vvt test_milestone.MilestoneSecurityAdaperTestCase

- tests that a person bug page can be rendered with an AAG bug with ersies
  target and milestone
- permission lp.LimiedView for IMilestone and IProductSeries

no lint

[Richard Harding (rharding)]

#132

Shouldn't displayname also be moved to the interface like name? It seems

#221

The comment has a duped launchpad.View, I assume one is meant to be LimitedView

review: Approve

=== modified file 'lib/lp/bugs/browser/tests/test_buglisting.py'
--- lib/lp/bugs/browser/tests/test_buglisting.py	2012-10-08 07:36:45 +0000
+++ lib/lp/bugs/browser/tests/test_buglisting.py	2012-12-04 16:26:21 +0000
@@ -11,6 +11,7 @@
     )
 from zope.component import getUtility
 
+from lp.app.enums import InformationType
 from lp.app.interfaces.launchpad import ILaunchpadCelebrities
 from lp.services.features.testing import FeatureFixture
 from lp.services.webapp.publisher import canonical_url
@@ -434,3 +435,55 @@
         def _getBugTarget(self, dsp):
             """Return the current `ISourcePackage` for the dsp."""
             return dsp.development_version
+
+
+class TestPersonBugListing(BrowserTestCase):
+    layer = DatabaseFunctionalLayer
+
+    def setUp(self):
+        super(TestPersonBugListing, self).setUp()
+        self.user = self.factory.makePerson()
+        self.private_product_owner = self.factory.makePerson()
+        self.private_product = self.factory.makeProduct(
+            owner=self.private_product_owner,
+            information_type=InformationType.PROPRIETARY)
+
+    def test_grant_for_bug_with_task_for_private_product(self):
+        # A person's own bug page is correctly rendered when the person
+        # is subscribed to a bug with a task for a propritary product.
+        with person_logged_in(self.private_product_owner):
+            bug = self.factory.makeBug(
+                target=self.private_product, owner=self.private_product_owner)
+            bug.subscribe(self.user, subscribed_by=self.private_product_owner)
+        url = canonical_url(self.user, rootsite='bugs')
+        # Just ensure that no exception occurs when the page is rendered.
+        self.getUserBrowser(url, user=self.user)
+
+    def test_grant_for_bug_with_task_for_private_product_series(self):
+        # A person's own bug page is correctly rendered when the person
+        # is subscribed to a bug with a task for a propritary product series.
+        with person_logged_in(self.private_product_owner):
+            series = self.factory.makeProductSeries(
+                product=self.private_product)
+            bug = self.factory.makeBug(
+                target=self.private_product, series=series,
+                owner=self.private_product_owner)
+            bug.subscribe(self.user, subscribed_by=self.private_product_owner)
+        url = canonical_url(self.user, rootsite='bugs')
+        # Just ensure that no exception occurs when the page is rendered.
+        self.getUserBrowser(url, user=self.user)
+
+    def test_grant_for_bug_with_task_for_private_product_and_milestone(self):
+        # A person's own bug page is correctly rendered when the person
+        # is subscribed to a bug with a task for a propritary product and
+        # a milestone.
+        with person_logged_in(self.private_product_owner):
+            milestone = self.factory.makeMilestone(
+                product=self.private_product)
+            bug = self.factory.makeBug(
+                target=self.private_product, milestone=milestone,
+                owner=self.private_product_owner)
+            bug.subscribe(self.user, subscribed_by=self.private_product_owner)
+        url = canonical_url(self.user, rootsite='bugs')
+        # Just ensure that no exception occurs when the page is rendered.
+        self.getUserBrowser(url, user=self.user)

=== modified file 'lib/lp/registry/configure.zcml'
--- lib/lp/registry/configure.zcml	2012-11-27 22:02:34 +0000
+++ lib/lp/registry/configure.zcml	2012-12-04 16:26:21 +0000
@@ -1035,20 +1035,24 @@
                 bugtasks
                 code_name
                 dateexpected
-                displayname
                 distribution
                 distroseries
                 getSpecifications
                 getTags
                 getTagsData
-                name
                 product
                 product_release
                 productseries
                 series_target
                 summary
+                title
+                "/>
+        <require
+            permission="launchpad.LimitedView"
+            attributes="
+                displayname
+                name
                 target
-                title
                 "/>
         <require
             permission="launchpad.Edit"
@@ -1565,6 +1569,9 @@
         <allow
             interface="lp.registry.interfaces.productseries.IProductSeriesPublic"/>
         <require
+            permission="launchpad.LimitedView"
+            interface="lp.registry.interfaces.productseries.IProductSeriesLimitedView"/>
+        <require
             permission="launchpad.View"
             interface="lp.registry.interfaces.productseries.IProductSeriesView"/>
         <require

=== modified file 'lib/lp/registry/interfaces/productseries.py'
--- lib/lp/registry/interfaces/productseries.py	2012-10-18 14:05:41 +0000
+++ lib/lp/registry/interfaces/productseries.py	2012-12-04 16:26:21 +0000
@@ -10,6 +10,7 @@
 __all__ = [
     'IProductSeries',
     'IProductSeriesEditRestricted',
+    'IProductSeriesLimitedView',
     'IProductSeriesPublic',
     'IProductSeriesSet',
     'IProductSeriesView',
@@ -138,16 +139,29 @@
         """True if the given user has access to this product."""
 
 
-class IProductSeriesView(
-    ISeriesMixin, IHasAppointedDriver, IHasOwner, IBugTarget,
-    ISpecificationGoal, IHasMilestones, IHasOfficialBugTags, IHasExpirableBugs,
-    IHasTranslationImports, IHasTranslationTemplates, IServiceUsage):
+class IProductSeriesLimitedView(Interface):
+
+    name = exported(
+        ProductSeriesNameField(
+            title=_('Name'),
+            description=_(
+                "The name of the series is a short, unique name "
+                "that identifies it, being used in URLs. It must be all "
+                "lowercase, with no special characters. For example, '2.0' "
+                "or 'trunk'."),
+            constraint=name_validator))
+
     product = exported(
         ReferenceChoice(title=_('Project'), required=True,
             vocabulary='Product', schema=Interface),  # really IProduct
         exported_as='project')
     productID = Attribute('The product ID.')
 
+
+class IProductSeriesView(
+    ISeriesMixin, IHasAppointedDriver, IHasOwner, IBugTarget,
+    ISpecificationGoal, IHasMilestones, IHasOfficialBugTags, IHasExpirableBugs,
+    IHasTranslationImports, IHasTranslationTemplates, IServiceUsage):
     status = exported(
         Choice(
             title=_('Status'), required=True, vocabulary=SeriesStatus,
@@ -155,16 +169,6 @@
 
     parent = Attribute('The structural parent of this series - the product')
 
-    name = exported(
-        ProductSeriesNameField(
-            title=_('Name'),
-            description=_(
-                "The name of the series is a short, unique name "
-                "that identifies it, being used in URLs. It must be all "
-                "lowercase, with no special characters. For example, '2.0' "
-                "or 'trunk'."),
-            constraint=name_validator))
-
     datecreated = exported(
         Datetime(title=_('Date Registered'),
                  required=True,
@@ -336,7 +340,8 @@
 
 
 class IProductSeries(IProductSeriesEditRestricted, IProductSeriesPublic,
-                     IProductSeriesView, IStructuralSubscriptionTarget):
+                     IProductSeriesView, IProductSeriesLimitedView,
+                     IStructuralSubscriptionTarget):
     """A series of releases. For example '2.0' or '1.3' or 'dev'."""
     export_as_webservice_entry('project_series')
 

=== modified file 'lib/lp/registry/tests/test_milestone.py'
--- lib/lp/registry/tests/test_milestone.py	2012-11-26 08:33:03 +0000
+++ lib/lp/registry/tests/test_milestone.py	2012-12-04 16:26:21 +0000
@@ -21,7 +21,6 @@
 from lp.app.interfaces.informationtype import IInformationType
 from lp.app.interfaces.services import IService
 from lp.registry.enums import (
-    BugSharingPolicy,
     SharingPermission,
     SpecificationSharingPolicy,
     )
@@ -137,16 +136,17 @@
             'id', 'checkAuthenticated', 'checkUnauthenticated',
             'userCanView',
             )),
+        'launchpad.LimitedView': set(('displayname', 'name', 'target', )),
         'launchpad.View': set((
             'active', 'bug_subscriptions', 'bugtasks', 'code_name',
-            'dateexpected', 'displayname', 'distribution', 'distroseries',
+            'dateexpected', 'distribution', 'distroseries',
             '_getOfficialTagClause', 'getBugSummaryContextWhereClause',
             'getBugTaskWeightFunction', 'getSpecifications',
             'getSubscription', 'getSubscriptions', 'getTags', 'getTagsData',
-            'getUsedBugTagsWithOpenCounts', 'name', 'official_bug_tags',
+            'getUsedBugTagsWithOpenCounts', 'official_bug_tags',
             'parent_subscription_target', 'product', 'product_release',
             'productseries', 'searchTasks', 'series_target',
-            'summary', 'target', 'target_type_display', 'title',
+            'summary', 'target_type_display', 'title',
             'userCanAlterBugSubscription', 'userCanAlterSubscription',
             'userHasBugSubscriptions',
             )),
@@ -232,20 +232,28 @@
                 self.proprietary_milestone)
 
             # They have access to attributes requiring the permission
-            # launchpad.View of milestones for public products...
+            # launchpad.View or launchpad.LimitedView of milestones for
+            # public products...
             self.assertAccessAuthorized(
                 self.expected_get_permissions['launchpad.View'],
                 self.public_milestone)
+            self.assertAccessAuthorized(
+                self.expected_get_permissions['launchpad.LimitedView'],
+                self.public_milestone)
 
             # ...but not to the same attributes of milestones for private
             # products.
             self.assertAccessUnauthorized(
                 self.expected_get_permissions['launchpad.View'],
                 self.proprietary_milestone)
+            self.assertAccessUnauthorized(
+                self.expected_get_permissions['launchpad.LimitedView'],
+                self.proprietary_milestone)
 
             # They cannot access other attributes.
             for permission, names in self.expected_get_permissions.items():
-                if permission in (CheckerPublic, 'launchpad.View'):
+                if permission in (CheckerPublic, 'launchpad.View',
+                                  'launchpad.LimitedView'):
                     continue
                 self.assertAccessUnauthorized(names, self.public_milestone)
                 self.assertAccessUnauthorized(
@@ -270,12 +278,16 @@
                 self.proprietary_milestone)
 
             # They have access to attributes requiring the permission
-            # launchpad.View or launchpad.AnyAllowedPerson of milestones
-            # for public products...
+            # launchpad.View, launchpad.LimitedView or
+            # launchpad.AnyAllowedPerson of milestones for public
+            # products...
             self.assertAccessAuthorized(
                 self.expected_get_permissions['launchpad.View'],
                 self.public_milestone)
             self.assertAccessAuthorized(
+                self.expected_get_permissions['launchpad.LimitedView'],
+                self.public_milestone)
+            self.assertAccessAuthorized(
                 self.expected_get_permissions['launchpad.AnyAllowedPerson'],
                 self.public_milestone)
 
@@ -285,13 +297,16 @@
                 self.expected_get_permissions['launchpad.View'],
                 self.proprietary_milestone)
             self.assertAccessUnauthorized(
+                self.expected_get_permissions['launchpad.LimitedView'],
+                self.proprietary_milestone)
+            self.assertAccessUnauthorized(
                 self.expected_get_permissions['launchpad.AnyAllowedPerson'],
                 self.proprietary_milestone)
 
             # They cannot access other attributes.
             for permission, names in self.expected_get_permissions.items():
                 if permission in (
-                    CheckerPublic, 'launchpad.View',
+                    CheckerPublic, 'launchpad.View', 'launchpad.LimitedView',
                     'launchpad.AnyAllowedPerson'):
                     continue
                 self.assertAccessUnauthorized(names, self.public_milestone)
@@ -309,6 +324,42 @@
         # to most attributes of the private product.
         user = self.factory.makePerson()
         with person_logged_in(self.proprietary_product_owner):
+            bug = self.factory.makeBug(
+                target=self.proprietary_product,
+                owner=self.proprietary_product_owner)
+            bug.subscribe(user, subscribed_by=self.proprietary_product_owner)
+
+        with person_logged_in(user):
+            self.assertAccessAuthorized(
+                self.expected_get_permissions[CheckerPublic],
+                self.proprietary_milestone)
+
+            # They have access to attributes requiring the permission
+            # launchpad.LimitedView of milestones for the private
+            # product.
+            self.assertAccessAuthorized(
+                self.expected_get_permissions['launchpad.LimitedView'],
+                self.proprietary_milestone)
+
+            # They cannot access other attributes.
+            for permission, names in self.expected_get_permissions.items():
+                if permission in (
+                    CheckerPublic, 'launchpad.LimitedView'):
+                    continue
+                self.assertAccessUnauthorized(
+                    names, self.proprietary_milestone)
+
+            # They cannot change attributes.
+            for names in self.expected_set_permissions.values():
+                self.assertChangeUnauthorized(
+                    names, self.proprietary_milestone)
+
+    def test_access_for_user_with_artifact_grant_for_private_product(self):
+        # Users with an artifact grant for a private product have access
+        # to attributes requiring the permission launchpad.LimitedView of
+        # milestones for the private product.
+        user = self.factory.makePerson()
+        with person_logged_in(self.proprietary_product_owner):
             getUtility(IService, 'sharing').sharePillarInformation(
                 self.proprietary_product, user, self.proprietary_product_owner,
                 {InformationType.PROPRIETARY: SharingPermission.ALL})
@@ -319,19 +370,23 @@
                 self.proprietary_milestone)
 
             # They have access to attributes requiring the permission
-            # launchpad.View or launchpad.AnyAllowedPerson of milestones
-            # for the private product.
+            # launchpad.View, launchpad.LimitedView or
+            # launchpad.AnyAllowedPerson of milestones for the private
+            # product.
             self.assertAccessAuthorized(
                 self.expected_get_permissions['launchpad.View'],
                 self.proprietary_milestone)
             self.assertAccessAuthorized(
+                self.expected_get_permissions['launchpad.LimitedView'],
+                self.proprietary_milestone)
+            self.assertAccessAuthorized(
                 self.expected_get_permissions['launchpad.AnyAllowedPerson'],
                 self.proprietary_milestone)
 
             # They cannot access other attributes.
             for permission, names in self.expected_get_permissions.items():
                 if permission in (
-                    CheckerPublic, 'launchpad.View',
+                    CheckerPublic, 'launchpad.View', 'launchpad.LimitedView',
                     'launchpad.AnyAllowedPerson'):
                     continue
                 self.assertAccessUnauthorized(

=== modified file 'lib/lp/registry/tests/test_productseries.py'
--- lib/lp/registry/tests/test_productseries.py	2012-11-30 20:52:15 +0000
+++ lib/lp/registry/tests/test_productseries.py	2012-12-04 16:26:21 +0000
@@ -622,6 +622,7 @@
             'addSubscription', 'removeBugSubscription',
             )),
         'launchpad.Edit': set(('newMilestone', )),
+        'launchpad.LimitedView': set(('name', 'product', 'productID')),
         'launchpad.View': set((
             '_all_specifications', '_getOfficialTagClause',
             '_valid_specifications', 'active', 'all_milestones',
@@ -650,10 +651,10 @@
             'has_obsolete_translation_templates',
             'has_sharing_translation_templates', 'has_translation_files',
             'has_translation_templates', 'is_development_focus', 'milestones',
-            'name', 'official_bug_tags', 'owner', 'packagings', 'parent',
+            'official_bug_tags', 'owner', 'packagings', 'parent',
             'parent_subscription_target',
-            'personHasDriverRights', 'pillar', 'potemplate_count', 'product',
-            'productID', 'productserieslanguages', 'release_files',
+            'personHasDriverRights', 'pillar', 'potemplate_count',
+            'productserieslanguages', 'release_files',
             'releasefileglob', 'releases', 'releaseverstyle', 'searchTasks',
             'series', 'setPackaging', 'sourcepackages', 'specifications',
             'status', 'summary', 'target_type_display', 'title',
@@ -733,21 +734,29 @@
                 self.proprietary_series)
 
             # They have access to attributes requiring the permission
-            # launchpad.View of a series for a public product...
+            # launchpad.Viewand launchpad.LimitedView of a series for a
+            # public product...
             self.assertAccessAuthorized(
                 self.expected_get_permissions['launchpad.View'],
                 self.public_series)
+            self.assertAccessAuthorized(
+                self.expected_get_permissions['launchpad.LimitedView'],
+                self.public_series)
 
             # ...but not to the same attributes of a series for s private
             # product.
             self.assertAccessUnauthorized(
                 self.expected_get_permissions['launchpad.View'],
                 self.proprietary_series)
+            self.assertAccessUnauthorized(
+                self.expected_get_permissions['launchpad.LimitedView'],
+                self.proprietary_series)
 
             # The cannot access other attributes of a series for
             # public and private products.
             for permission, names in self.expected_get_permissions.items():
-                if permission in (CheckerPublic, 'launchpad.View'):
+                if permission in (CheckerPublic, 'launchpad.LimitedView',
+                                  'launchpad.View'):
                     continue
                 self.assertAccessUnauthorized(names, self.public_series)
                 self.assertAccessUnauthorized(names, self.proprietary_series)
@@ -770,12 +779,15 @@
                 self.proprietary_series)
 
             # They have access to attributes requiring the permissions
-            # launchpad.View and launchpadAnyAllowedPerson of a series
-            # for a public product...
+            # launchpad.View, launchpad.LimitedView and
+            # launchpadAnyAllowedPerson of a series for a public product...
             self.assertAccessAuthorized(
                 self.expected_get_permissions['launchpad.View'],
                 self.public_series)
             self.assertAccessAuthorized(
+                self.expected_get_permissions['launchpad.LimitedView'],
+                self.public_series)
+            self.assertAccessAuthorized(
                 self.expected_get_permissions['launchpad.AnyAllowedPerson'],
                 self.public_series)
 
@@ -785,13 +797,17 @@
                 self.expected_get_permissions['launchpad.View'],
                 self.proprietary_series)
             self.assertAccessUnauthorized(
+                self.expected_get_permissions['launchpad.LimitedView'],
+                self.proprietary_series)
+            self.assertAccessUnauthorized(
                 self.expected_get_permissions['launchpad.AnyAllowedPerson'],
                 self.proprietary_series)
 
             # The cannot access other attributes of a series for
             # public and private products.
             for permission, names in self.expected_get_permissions.items():
-                if permission in (CheckerPublic, 'launchpad.View',
+                if permission in (CheckerPublic, 'launchpad.LimitedView',
+                                  'launchpad.View',
                                   'launchpad.AnyAllowedPerson'):
                     continue
                 self.assertAccessUnauthorized(names, self.public_series)
@@ -817,8 +833,8 @@
 
     def test_access_for_user_with_policy_grant(self):
         # Users with a policy grant for the parent product can access
-        # properties requring the permission lanchpad.View or
-        # launchpad.ANyALlowedPerson of a series.
+        # properties requring the permission launchpad.LimitedView,
+        # launchpad.View or launchpad.AnyALlowedPerson of a series.
         user = self.factory.makePerson()
         with person_logged_in(self.proprietary_product_owner):
             getUtility(IService, 'sharing').sharePillarInformation(
@@ -826,6 +842,9 @@
                 {InformationType.PROPRIETARY: SharingPermission.ALL})
         with person_logged_in(user):
             self.assertAccessAuthorized(
+                self.expected_get_permissions['launchpad.LimitedView'],
+                self.proprietary_series)
+            self.assertAccessAuthorized(
                 self.expected_get_permissions['launchpad.View'],
                 self.proprietary_series)
             self.assertAccessAuthorized(
@@ -836,6 +855,7 @@
             # private products.
             for permission, names in self.expected_get_permissions.items():
                 if permission in (CheckerPublic, 'launchpad.View',
+                                  'launchpad.LimitedView',
                                   'launchpad.AnyAllowedPerson'):
                     continue
                 self.assertAccessUnauthorized(names, self.proprietary_series)
@@ -853,6 +873,33 @@
                     continue
                 self.assertChangeUnauthorized(names, self.proprietary_series)
 
+    def test_access_for_user_with_artifact_grant(self):
+        # Users with an artifact grant related to the parent product
+        # can access properties requring the permission launchpad.LimitedView
+        # of a series.
+        user = self.factory.makePerson()
+        with person_logged_in(self.proprietary_product_owner):
+            bug = self.factory.makeBug(
+                target=self.proprietary_product,
+                owner=self.proprietary_product_owner)
+            bug.subscribe(user, subscribed_by=self.proprietary_product_owner)
+
+        with person_logged_in(user):
+            self.assertAccessAuthorized(
+                self.expected_get_permissions['launchpad.LimitedView'],
+                self.proprietary_series)
+
+            # The cannot access other attributes of a series for
+            # private products.
+            for permission, names in self.expected_get_permissions.items():
+                if permission in (CheckerPublic, 'launchpad.LimitedView'):
+                    continue
+                self.assertAccessUnauthorized(names, self.proprietary_series)
+
+            # They cannot change any attributes.
+            for permission, names in self.expected_set_permissions.items():
+                self.assertChangeUnauthorized(names, self.proprietary_series)
+
     def test_access_for_product_owner(self):
         # The owner of a project has access to all attributes of
         # a product series.

=== modified file 'lib/lp/security.py'
--- lib/lp/security.py	2012-11-27 22:02:34 +0000
+++ lib/lp/security.py	2012-12-04 16:26:21 +0000
@@ -156,6 +156,7 @@
     )
 from lp.registry.interfaces.productseries import (
     IProductSeries,
+    IProductSeriesLimitedView,
     IProductSeriesView,
     ITimelineProductSeries,
     )
@@ -760,6 +761,15 @@
         return False
 
 
+class LimitedViewMilestone(DelegatedAuthorization):
+    permission = 'launchpad.LimitedView'
+    usedfor = IMilestone
+
+    def __init__(self, obj):
+        super(LimitedViewMilestone, self).__init__(
+            obj, obj.target, 'launchpad.LimitedView')
+
+
 class ViewMilestone(AuthorizationBase):
     permission = 'launchpad.View'
     usedfor = IMilestone
@@ -1319,6 +1329,15 @@
             or False)
 
 
+class LimitedViewProductSeries(DelegatedAuthorization):
+    permission = 'launchpad.LimitedView'
+    usedfor = IProductSeriesLimitedView
+
+    def __init__(self, obj):
+        super(LimitedViewProductSeries, self).__init__(
+            obj, obj.product, 'launchpad.LimitedView')
+
+
 class ViewProductSeries(AuthorizationBase):
     permission = 'launchpad.View'
     usedfor = IProductSeriesView