Merge lp:~adeuring/launchpad/product-lp-limitedview into lp:launchpad

[Abel Deuring]

split IProductLimitedView into IProductView and IProductLimitedView; require the permission launchpad.View for IProductView and launchapd.LimietdView for IProductLimitedView

This branch splits the existing interface IProductLimitedView into
IProductView and IProductLimitedView. IProductView now requires the
permission lauchpad.View, and IProductLimitedView requires the
permission launchpad.LimitedView.

We want to make it possible to use artifact grants on bugs/branches/
blueprints for prioprietary/embargoed products, so that grantees
can see the artifacts but not all details about the product.

Even a call of canonical_url(artifact) requires access to
artifact.target.name, so people with artifact grants must be able
to see target.name, as well as several other attributes. I don't know
yet which attributes or even how many attributes need to be moved from
IProductView to IProductLimitedView, but I suspect that it will be
quite a few. Doing this all in one branch has the risk of a
"diff length explosion", so I started with just one attribute.

The new security adapter LimitedViewProduct allows access
not only for people with grants on the product itself, but it also
checks if the current user has any artifact grants related to the
product.

The new check is implementd in SharingService.userHasGrantsOnPillar(),
which is just a modified variant of the existing method
getSharedArtifacts(): While the latter method returns all artifacts
a user may access, the former method just checks, if the user has
any artifact grants. The common SQL query is generated in the new
method getArtifactGrantsForPersonOnPillar().

test:

bin/test -vvt lp.registry.tests.test_product.TestProduct.test_access_LimitedView
bin/test -vvt lp.registry.tests.test_product.TestProduct.test_get_permissions

no lint

[Francesco Banconi (frankban)]

This branch looks good Abel, some minors follow.

28 +class IProductLimitedView(Interface):
29 + """Attributes that must be visible for person with artifact grants
30 + on bugs, branches or specifications for the product,
31 + """

The docstring ends with a comma.

203 + def test_access_LimitedView_proprietary_product(self):

Feel free to ignore this suggestion if you want: maybe this test could be
splitted in two parts: test unauthorized users and then test users with
access grant on something.

review: Approve

=== modified file 'lib/lp/registry/configure.zcml'
--- lib/lp/registry/configure.zcml	2012-10-25 14:36:30 +0000
+++ lib/lp/registry/configure.zcml	2012-11-06 14:20:25 +0000
@@ -1248,10 +1248,13 @@
         <allow
             interface="lp.registry.interfaces.pillar.IPillar"/>
         <require
-            permission="launchpad.View"
+            permission="launchpad.LimitedView"
             interface="lp.registry.interfaces.product.IProductLimitedView"/>
         <require
             permission="launchpad.View"
+            interface="lp.registry.interfaces.product.IProductView"/>
+        <require
+            permission="launchpad.View"
             interface="lp.bugs.interfaces.bugsummary.IBugSummaryDimension"/>
         <require
             permission="launchpad.View"

=== modified file 'lib/lp/registry/interfaces/product.py'
--- lib/lp/registry/interfaces/product.py	2012-10-24 14:21:58 +0000
+++ lib/lp/registry/interfaces/product.py	2012-11-06 14:20:25 +0000
@@ -428,7 +428,22 @@
         """True if the given user has access to this product."""
 
 
-class IProductLimitedView(
+class IProductLimitedView(Interface):
+    """Attributes that must be visible for person with artifact grants
+    on bugs, branches or specifications for the product.
+    """
+
+    name = exported(
+        ProductNameField(
+            title=_('Name'),
+            constraint=name_validator,
+            description=_(
+                "At least one lowercase letter or number, followed by "
+                "letters, numbers, dots, hyphens or pluses. "
+                "Keep this name short; it is used in URLs as shown above.")))
+
+
+class IProductView(
     IBugTarget, ICanGetMilestonesDirectly, IHasAppointedDriver, IHasBranches,
     IHasDrivers, IHasExternalBugTracker, IHasIcon,
     IHasLogo, IHasMergeProposals, IHasMilestones, IHasExpirableBugs,
@@ -488,15 +503,6 @@
         "required because there might be a project driver and also a "
         "driver appointed in the overarching project group.")
 
-    name = exported(
-        ProductNameField(
-            title=_('Name'),
-            constraint=name_validator,
-            description=_(
-                "At least one lowercase letter or number, followed by "
-                "letters, numbers, dots, hyphens or pluses. "
-                "Keep this name short; it is used in URLs as shown above.")))
-
     displayname = exported(
         TextLine(
             title=_('Display Name'),
@@ -874,9 +880,9 @@
 class IProductEditRestricted(IOfficialBugTagTargetRestricted):
     """`IProduct` properties which require launchpad.Edit permission."""
 
-    @mutator_for(IProductLimitedView['bug_sharing_policy'])
+    @mutator_for(IProductView['bug_sharing_policy'])
     @operation_parameters(bug_sharing_policy=copy_field(
-        IProductLimitedView['bug_sharing_policy']))
+        IProductView['bug_sharing_policy']))
     @export_write_operation()
     @operation_for_version("devel")
     def setBugSharingPolicy(bug_sharing_policy):
@@ -885,10 +891,10 @@
         Checks authorization and entitlement.
         """
 
-    @mutator_for(IProductLimitedView['branch_sharing_policy'])
+    @mutator_for(IProductView['branch_sharing_policy'])
     @operation_parameters(
         branch_sharing_policy=copy_field(
-            IProductLimitedView['branch_sharing_policy']))
+            IProductView['branch_sharing_policy']))
     @export_write_operation()
     @operation_for_version("devel")
     def setBranchSharingPolicy(branch_sharing_policy):
@@ -897,10 +903,10 @@
         Checks authorization and entitlement.
         """
 
-    @mutator_for(IProductLimitedView['specification_sharing_policy'])
+    @mutator_for(IProductView['specification_sharing_policy'])
     @operation_parameters(
         specification_sharing_policy=copy_field(
-            IProductLimitedView['specification_sharing_policy']))
+            IProductView['specification_sharing_policy']))
     @export_write_operation()
     @operation_for_version("devel")
     def setSpecificationSharingPolicy(specification_sharing_policy):
@@ -912,7 +918,7 @@
 
 class IProduct(
     IHasBugSupervisor, IProductEditRestricted,
-    IProductModerateRestricted, IProductDriverRestricted,
+    IProductModerateRestricted, IProductDriverRestricted, IProductView,
     IProductLimitedView, IProductPublic, IQuestionTarget, IRootContext,
     IStructuralSubscriptionTarget, IInformationType, IPillar):
     """A Product.

=== modified file 'lib/lp/registry/interfaces/sharingservice.py'
--- lib/lp/registry/interfaces/sharingservice.py	2012-10-10 03:20:17 +0000
+++ lib/lp/registry/interfaces/sharingservice.py	2012-11-06 14:20:25 +0000
@@ -117,6 +117,9 @@
         :return: a (bugtasks, branches, specifications) tuple
         """
 
+    def userHasGrantsOnPillar(pillar, user):
+        """Return True if user has any grants on pillar else return False."""
+
     @export_read_operation()
     @call_with(user=REQUEST_USER)
     @operation_parameters(

=== modified file 'lib/lp/registry/services/sharingservice.py'
--- lib/lp/registry/services/sharingservice.py	2012-10-10 02:45:36 +0000
+++ lib/lp/registry/services/sharingservice.py	2012-11-06 14:20:25 +0000
@@ -186,16 +186,21 @@
         """See `ISharingService`."""
         return self._getSharedPillars(person, user, Distribution)
 
+    def getArtifactGrantsForPersonOnPillar(self, pillar, person):
+        """Return the artifact grants for the given person and pillar."""
+        policies = getUtility(IAccessPolicySource).findByPillar([pillar])
+        flat_source = getUtility(IAccessPolicyGrantFlatSource)
+        return flat_source.findArtifactsByGrantee(person, policies)
+
     @available_with_permission('launchpad.Driver', 'pillar')
     def getSharedArtifacts(self, pillar, person, user, include_bugs=True,
                            include_branches=True, include_specifications=True):
         """See `ISharingService`."""
-        policies = getUtility(IAccessPolicySource).findByPillar([pillar])
-        flat_source = getUtility(IAccessPolicyGrantFlatSource)
         bug_ids = set()
         branch_ids = set()
         specification_ids = set()
-        for artifact in flat_source.findArtifactsByGrantee(person, policies):
+        for artifact in self.getArtifactGrantsForPersonOnPillar(
+            pillar, person):
             if artifact.bug_id and include_bugs:
                 bug_ids.add(artifact.bug_id)
             elif artifact.branch_id and include_branches:
@@ -222,6 +227,11 @@
 
         return bugtasks, branches, specifications
 
+    def userHasGrantsOnPillar(self, pillar, user):
+        """See `ISharingService`."""
+        return not self.getArtifactGrantsForPersonOnPillar(
+            pillar, user).is_empty()
+
     @available_with_permission('launchpad.Driver', 'pillar')
     def getSharedBugs(self, pillar, person, user):
         """See `ISharingService`."""

=== modified file 'lib/lp/registry/tests/test_product.py'
--- lib/lp/registry/tests/test_product.py	2012-11-02 20:08:55 +0000
+++ lib/lp/registry/tests/test_product.py	2012-11-06 14:20:25 +0000
@@ -551,6 +551,7 @@
         CheckerPublic: set((
             'active', 'id', 'information_type', 'pillar_category', 'private',
             'userCanView',)),
+        'launchpad.LimitedView': set(('name', )),
         'launchpad.View': set((
             '_getOfficialTagClause', '_all_specifications',
             '_valid_specifications', 'active_or_packaged_series',
@@ -594,7 +595,7 @@
             'homepageurl', 'icon', 'invitesTranslationEdits',
             'invitesTranslationSuggestions',
             'license_info', 'license_status', 'licenses', 'logo', 'milestones',
-            'mugshot', 'name', 'name_with_project', 'newCodeImport',
+            'mugshot', 'name_with_project', 'newCodeImport',
             'obsolete_translatable_series', 'official_answers',
             'official_anything', 'official_blueprints', 'official_bug_tags',
             'official_codehosting', 'official_malone', 'owner',
@@ -766,6 +767,57 @@
             for attribute_name in names:
                 getattr(product, attribute_name)
 
+    def test_access_LimitedView_public_product(self):
+        # Everybody can access attributes of public products that
+        # require the permission launchpad.LimitedView.
+        product = self.factory.makeProduct()
+        names = self.expected_get_permissions['launchpad.LimitedView']
+        with person_logged_in(None):
+            for attribute_name in names:
+                getattr(product, attribute_name)
+        ordinary_user = self.factory.makePerson()
+        with person_logged_in(ordinary_user):
+            for attribute_name in names:
+                getattr(product, attribute_name)
+
+    def test_access_LimitedView_proprietary_product(self):
+        # Anonymous users and ordinary logged in users cannot access
+        # attributes of private products that require the permission
+        # launchpad.LimitedView.
+        owner = self.factory.makePerson()
+        product = self.factory.makeProduct(
+            owner=owner,
+            information_type=InformationType.PROPRIETARY)
+        names = self.expected_get_permissions['launchpad.LimitedView']
+        with person_logged_in(None):
+            for attribute_name in names:
+                self.assertRaises(
+                    Unauthorized, getattr, product, attribute_name)
+        user = self.factory.makePerson()
+        with person_logged_in(user):
+            for attribute_name in names:
+                self.assertRaises(
+                    Unauthorized, getattr, product, attribute_name)
+        # Users with a grant on an artifact related to the product
+        # can access the attributes.
+        with person_logged_in(owner):
+            bug = self.factory.makeBug(
+                target=product, information_type=InformationType.PROPRIETARY)
+            getUtility(IService, 'sharing').ensureAccessGrants(
+                [user], owner, bugs=[bug])
+        with person_logged_in(user):
+            for attribute_name in names:
+                getattr(product, attribute_name)
+        # Users with a policy grant for the product also have access.
+        user2 = self.factory.makePerson()
+        with person_logged_in(owner):
+            getUtility(IService, 'sharing').sharePillarInformation(
+                product, user2, owner,
+                {InformationType.PROPRIETARY: SharingPermission.ALL})
+        with person_logged_in(user2):
+            for attribute_name in names:
+                getattr(product, attribute_name)
+
     def test_access_launchpad_AnyAllowedPerson_public_product(self):
         # Only logged in persons have access to properties of public products
         # that require the permission launchpad.AnyAllowedPerson.

=== modified file 'lib/lp/security.py'
--- lib/lp/security.py	2012-11-03 13:49:04 +0000
+++ lib/lp/security.py	2012-11-06 14:20:25 +0000
@@ -37,6 +37,7 @@
     AuthorizationBase,
     DelegatedAuthorization,
     )
+from lp.app.interfaces.services import IService
 from lp.archivepublisher.interfaces.publisherconfig import IPublisherConfig
 from lp.blueprints.interfaces.specification import (
     ISpecification,
@@ -441,6 +442,17 @@
         return self.obj.userCanView(None)
 
 
+class LimitedViewProduct(ViewProduct):
+    permission = 'launchpad.LimitedView'
+    usedfor = IProduct
+
+    def checkAuthenticated(self, user):
+        return (
+            super(LimitedViewProduct, self).checkAuthenticated(user) or
+            getUtility(IService, 'sharing').userHasGrantsOnPillar(
+                self.obj, user))
+
+
 class ChangeProduct(ViewProduct):
     """Used for attributes of IProduct that are accessible to any logged
     in user for public product but only to persons with access grants