Merge lp:~adeuring/launchpad/specification-grants into lp:launchpad

[Abel Deuring]

new parameter "specifications" for SharingService.ensureAccessGrants()

This branch makes it possible to grant access to a single specification that
is not publicly visible. (The security adapter for ISpecification does not
use yet these grants -- the MP would become overly long with this change...)

= Details =

lib/lp/blueprints/configure.zcml,
lib/lp/blueprints/model/specification.py,
lib/lp/blueprints/tests/test_specification.py:

The property Specification.information_type should not be set directly
because instances that are not publicly visible need a record in
the table AccessPolicyArtifact that links the specification to an
AccessPolicy. These links are maintained by calling
reconcile_access_for_artifact() in Specifcation.transitionToInformationType().

This change leads to a number of changes in
lib/lp/blueprints/tests/test_specification.py, where severeal tests
changed information_type with a simple assignment.

Furthermore, reconcile_access_for_artifact() obviously fails if no
matching AccessPolicy record exists; these records are created by
calling speicifcaton.target._ensurePolicies(). This is a bit hackish,
but the class Product does not yet have a method
setSpecifcationSharingPolicy() which would manage AccessPolicy records
"officially".

lib/lp/registry/interfaces/accesspolicy.py:

The attribute specification_id was simply missing in the interface class;
it is already used in the model.

lib/lp/registry/model/accesspolicy.py,
lib/lp/registry/services/sharingservice.py,
lib/lp/registry/services/tests/test_sharingservice.py:

The core of the change: AccessArtifact.ensure() and
SharingService.ensureAccessGrants() must be able to deal
not only with bugs and branches, but also with specifications.

= tests =

./bin/test -vvt lp.registry.services.tests.test_sharingservice.*test_ensureAccessGrants
./bin/test -vvt lp.blueprints.tests.test_specification.SpecificationTests.test.*access
./bin/test -vvt lp.registry.tests.test_accesspolicy.TestAccessArtifactSpecification

no lint

[j.c.sackett (jcsackett)]

Abel--

Looks good, thanks.

review: Approve

=== modified file 'lib/lp/blueprints/browser/tests/test_specification.py'
--- lib/lp/blueprints/browser/tests/test_specification.py	2012-09-06 00:01:38 +0000
+++ lib/lp/blueprints/browser/tests/test_specification.py	2012-09-10 10:23:19 +0000
@@ -227,6 +227,8 @@
         """Setting the value via '+secrecy' works."""
         owner = self.factory.makePerson()
         spec = self.factory.makeSpecification(owner=owner)
+        removeSecurityProxy(spec.target)._ensurePolicies(
+            [InformationType.PROPRIETARY])
         self.set_secrecy(spec, owner)
         with person_logged_in(owner):
             self.assertEqual(InformationType.PROPRIETARY,

=== modified file 'lib/lp/blueprints/configure.zcml'
--- lib/lp/blueprints/configure.zcml	2012-08-20 16:38:10 +0000
+++ lib/lp/blueprints/configure.zcml	2012-09-10 10:23:19 +0000
@@ -150,10 +150,6 @@
   <class class="lp.blueprints.model.specification.Specification">
     <allow interface="lp.blueprints.interfaces.specification.ISpecificationPublic"/>
     <require
-        permission="launchpad.Edit"
-        set_attributes="information_type" />
-
-    <require
         permission="launchpad.LimitedView"
         interface="lp.blueprints.interfaces.specification.ISpecificationView" />
 

=== modified file 'lib/lp/blueprints/model/specification.py'
--- lib/lp/blueprints/model/specification.py	2012-09-06 19:05:05 +0000
+++ lib/lp/blueprints/model/specification.py	2012-09-10 10:23:19 +0000
@@ -821,12 +821,17 @@
         return set(PUBLIC_PROPRIETARY_INFORMATION_TYPES)
 
     def transitionToInformationType(self, information_type, who):
-        """See `IBug`."""
+        """See ISpecification."""
+        # avoid circular imports.
+        from lp.registry.model.accesspolicy import (
+            reconcile_access_for_artifact,
+            )
         if self.information_type == information_type:
             return False
         if information_type not in self.getAllowedInformationTypes(who):
             raise CannotChangeInformationType("Forbidden by project policy.")
         self.information_type = information_type
+        reconcile_access_for_artifact(self, information_type, [self.target])
         return True
 
     @property

=== modified file 'lib/lp/blueprints/model/tests/test_specification.py'
--- lib/lp/blueprints/model/tests/test_specification.py	2012-09-06 19:06:11 +0000
+++ lib/lp/blueprints/model/tests/test_specification.py	2012-09-10 10:23:19 +0000
@@ -16,6 +16,7 @@
 from zope.event import notify
 from zope.interface import providedBy
 from zope.security.interfaces import Unauthorized
+from zope.security.proxy import removeSecurityProxy
 
 from lp.app.validators import LaunchpadValidationError
 from lp.blueprints.interfaces.specification import ISpecification
@@ -619,6 +620,8 @@
         """Ensure transitionToInformationType works."""
         spec = self.factory.makeSpecification()
         self.assertEqual(InformationType.PUBLIC, spec.information_type)
+        removeSecurityProxy(spec.target)._ensurePolicies(
+            [InformationType.EMBARGOED])
         with person_logged_in(spec.owner):
             result = spec.transitionToInformationType(
                 InformationType.EMBARGOED, spec.owner)

=== modified file 'lib/lp/blueprints/tests/test_specification.py'
--- lib/lp/blueprints/tests/test_specification.py	2012-09-06 00:01:38 +0000
+++ lib/lp/blueprints/tests/test_specification.py	2012-09-10 10:23:19 +0000
@@ -193,9 +193,9 @@
             'launchpad.AnyAllowedPerson': set(('whiteboard', )),
             'launchpad.Edit': set((
                 'approver', 'assignee', 'definition_status', 'distribution',
-                'drafter', 'implementation_status', 'information_type',
-                'man_days', 'milestone', 'name', 'product', 'specurl',
-                'summary', 'superseded_by', 'title')),
+                'drafter', 'implementation_status', 'man_days', 'milestone',
+                'name', 'product', 'specurl', 'summary', 'superseded_by',
+                'title')),
             }
         specification = self.factory.makeSpecification()
         checker = getChecker(specification)
@@ -249,27 +249,30 @@
                 setattr(specification, attribute, value)
 
     def test_anon_read_access(self):
-        # Anonymous users have access to public specifications...
+        # Anonymous users have access to public specifications but not
+        # to private specifications.
         specification = self.factory.makeSpecification()
-        for information_type in PUBLIC_INFORMATION_TYPES:
-            with person_logged_in(specification.owner):
-                specification.information_type = information_type
-            self.read_access_to_ISpecificationView(
-                ANONYMOUS, specification, error_expected=False)
-        # ...but not to private specifications.
-        for information_type in PRIVATE_INFORMATION_TYPES:
-            with person_logged_in(specification.owner):
-                specification.information_type = information_type
-            self.read_access_to_ISpecificationView(
-                ANONYMOUS, specification, error_expected=True)
+        removeSecurityProxy(specification.target)._ensurePolicies(
+            PRIVATE_INFORMATION_TYPES)
+        all_types = specification.getAllowedInformationTypes(ANONYMOUS)
+        for information_type in all_types:
+            with person_logged_in(specification.owner):
+                specification.transitionToInformationType(
+                    information_type, specification.owner)
+            error_expected = information_type not in PUBLIC_INFORMATION_TYPES
+            self.read_access_to_ISpecificationView(
+                ANONYMOUS, specification, error_expected)
 
     def test_anon_write_access(self):
         # Anonymous users do not have write access to specifications.
         specification = self.factory.makeSpecification()
-        for information_type in (PUBLIC_INFORMATION_TYPES +
-                                 PRIVATE_INFORMATION_TYPES):
+        removeSecurityProxy(specification.target)._ensurePolicies(
+            PRIVATE_INFORMATION_TYPES)
+        all_types = specification.getAllowedInformationTypes(ANONYMOUS)
+        for information_type in all_types:
             with person_logged_in(specification.owner):
-                specification.information_type = information_type
+                specification.transitionToInformationType(
+                    information_type, specification.owner)
             self.write_access_to_ISpecificationView(
                 ANONYMOUS, specification, error_expected=True,
                 attribute='whiteboard', value='foo')
@@ -278,41 +281,37 @@
                 attribute='name', value='foo')
 
     def test_ordinary_user_read_access(self):
-        # Oridnary users have access to public specifications...
+        # Oridnary users have access to public specifications but not
+        # to private specifications.
         specification = self.factory.makeSpecification()
+        removeSecurityProxy(specification.target)._ensurePolicies(
+            PRIVATE_INFORMATION_TYPES)
         user = self.factory.makePerson()
-        for information_type in PUBLIC_INFORMATION_TYPES:
-            with person_logged_in(specification.owner):
-                specification.information_type = information_type
-            self.read_access_to_ISpecificationView(
-                user, specification, error_expected=False)
-        # ...but not to private specifications.
-        for information_type in PRIVATE_INFORMATION_TYPES:
-            with person_logged_in(specification.owner):
-                specification.information_type = information_type
-            self.read_access_to_ISpecificationView(
-                user, specification, error_expected=True)
+        all_types = specification.getAllowedInformationTypes(user)
+        for information_type in all_types:
+            with person_logged_in(specification.owner):
+                specification.transitionToInformationType(
+                    information_type, specification.owner)
+            error_expected = information_type not in PUBLIC_INFORMATION_TYPES
+            self.read_access_to_ISpecificationView(
+                user, specification, error_expected)
 
     def test_ordinary_user_write_access(self):
         # Oridnary users can change the whiteborad of public specifications.
-        # They cannot change other attributes.
+        # They cannot change other attributes of public speicifcaitons and
+        # no attributes of private specifications.
         specification = self.factory.makeSpecification()
+        removeSecurityProxy(specification.target)._ensurePolicies(
+            PRIVATE_INFORMATION_TYPES)
         user = self.factory.makePerson()
-        for information_type in PUBLIC_INFORMATION_TYPES:
-            with person_logged_in(specification.owner):
-                specification.information_type = information_type
-            self.write_access_to_ISpecificationView(
-                user, specification, error_expected=False,
-                attribute='whiteboard', value='foo')
-            self.write_access_to_ISpecificationView(
-                user, specification, error_expected=True,
-                attribute='name', value='foo')
-        # The cannot change any attribute of private specifcations.
-        for information_type in PRIVATE_INFORMATION_TYPES:
-            with person_logged_in(specification.owner):
-                specification.information_type = information_type
-            self.write_access_to_ISpecificationView(
-                user, specification, error_expected=True,
+        all_types = specification.getAllowedInformationTypes(user)
+        for information_type in all_types:
+            with person_logged_in(specification.owner):
+                specification.transitionToInformationType(
+                    information_type, specification.owner)
+            error_expected = information_type not in PUBLIC_INFORMATION_TYPES
+            self.write_access_to_ISpecificationView(
+                user, specification, error_expected,
                 attribute='whiteboard', value='foo')
             self.write_access_to_ISpecificationView(
                 user, specification, error_expected=True,
@@ -322,10 +321,14 @@
         # Users with special privileges can aceess the attributes
         # of public and private specifcations.
         specification = self.factory.makeSpecification()
-        for information_type in (PUBLIC_INFORMATION_TYPES +
-                                 PRIVATE_INFORMATION_TYPES):
+        removeSecurityProxy(specification.target)._ensurePolicies(
+            PRIVATE_INFORMATION_TYPES)
+        all_types = specification.getAllowedInformationTypes(
+            specification.owner)
+        for information_type in all_types:
             with person_logged_in(specification.owner):
-                specification.information_type = information_type
+                specification.transitionToInformationType(
+                    information_type, specification.owner)
             self.read_access_to_ISpecificationView(
                 specification.owner, specification, error_expected=False)
 
@@ -333,10 +336,14 @@
         # Users with special privileges can change the attributes
         # of public and private specifcations.
         specification = self.factory.makeSpecification()
-        for information_type in (PUBLIC_INFORMATION_TYPES +
-                                 PRIVATE_INFORMATION_TYPES):
+        removeSecurityProxy(specification.target)._ensurePolicies(
+            PRIVATE_INFORMATION_TYPES)
+        all_types = specification.getAllowedInformationTypes(
+            specification.owner)
+        for information_type in all_types:
             with person_logged_in(specification.owner):
-                specification.information_type = information_type
+                specification.transitionToInformationType(
+                    information_type, specification.owner)
             self.write_access_to_ISpecificationView(
                 specification.owner, specification, error_expected=False,
                 attribute='whiteboard', value='foo')

=== modified file 'lib/lp/registry/interfaces/accesspolicy.py'
--- lib/lp/registry/interfaces/accesspolicy.py	2012-09-03 11:22:25 +0000
+++ lib/lp/registry/interfaces/accesspolicy.py	2012-09-10 10:23:19 +0000
@@ -35,6 +35,7 @@
     concrete_artifact = Attribute("Concrete artifact")
     bug_id = Attribute("bug_id")
     branch_id = Attribute("branch_id")
+    specification_id = Attribute("specification_id")
 
 
 class IAccessArtifactGrant(Interface):

=== modified file 'lib/lp/registry/interfaces/sharingservice.py'
--- lib/lp/registry/interfaces/sharingservice.py	2012-09-05 03:39:50 +0000
+++ lib/lp/registry/interfaces/sharingservice.py	2012-09-10 10:23:19 +0000
@@ -247,13 +247,15 @@
         branches=List(
             Reference(schema=IBranch), title=_('Branches'), required=False))
     @operation_for_version('devel')
-    def ensureAccessGrants(grantees, user, branches=None, bugs=None):
+    def ensureAccessGrants(grantees, user, branches=None, bugs=None,
+                           specifications=None):
         """Ensure a grantee has an access grant to the specified artifacts.
 
         :param grantees: the people or teams for whom to grant access
         :param user: the user making the request
         :param bugs: the bugs for which to grant access
         :param branches: the branches for which to grant access
+        :param specifications: the specifications for which to grant access
         """
 
     @export_write_operation()

=== modified file 'lib/lp/registry/model/accesspolicy.py'
--- lib/lp/registry/model/accesspolicy.py	2012-09-03 11:22:25 +0000
+++ lib/lp/registry/model/accesspolicy.py	2012-09-10 10:23:19 +0000
@@ -70,7 +70,7 @@
         (pillar, information_type) for pillar in pillars)
     missing_pillars = set(pillars) - set([ap.pillar for ap in aps])
     if len(missing_pillars):
-        pillar_str =  ', '.join([p.name for p in missing_pillars])
+        pillar_str = ', '.join([p.name for p in missing_pillars])
         raise AssertionError(
             "Pillar(s) %s require an access policy for information type "
             "%s." % (pillar_str, information_type.title))
@@ -97,20 +97,25 @@
     bug = Reference(bug_id, 'Bug.id')
     branch_id = Int(name='branch')
     branch = Reference(branch_id, 'Branch.id')
+    specification_id = Int(name='specification')
+    specification = Reference(specification_id, 'Specification.id')
 
     @property
     def concrete_artifact(self):
-        artifact = self.bug or self.branch
+        artifact = self.bug or self.branch or self.specification
         return artifact
 
     @classmethod
     def _constraintForConcrete(cls, concrete_artifact):
+        from lp.blueprints.interfaces.specification import ISpecification
         from lp.bugs.interfaces.bug import IBug
         from lp.code.interfaces.branch import IBranch
         if IBug.providedBy(concrete_artifact):
             col = cls.bug
         elif IBranch.providedBy(concrete_artifact):
             col = cls.branch
+        elif ISpecification.providedBy(concrete_artifact):
+            col = cls.specification
         else:
             raise ValueError(
                 "%r is not a valid artifact" % concrete_artifact)
@@ -128,6 +133,7 @@
     @classmethod
     def ensure(cls, concrete_artifacts):
         """See `IAccessArtifactSource`."""
+        from lp.blueprints.interfaces.specification import ISpecification
         from lp.bugs.interfaces.bug import IBug
         from lp.code.interfaces.branch import IBranch
 
@@ -143,12 +149,16 @@
         insert_values = []
         for concrete in needed:
             if IBug.providedBy(concrete):
-                insert_values.append((concrete, None))
+                insert_values.append((concrete, None, None))
             elif IBranch.providedBy(concrete):
-                insert_values.append((None, concrete))
+                insert_values.append((None, concrete, None))
+            elif ISpecification.providedBy(concrete):
+                insert_values.append((None, None, concrete))
             else:
                 raise ValueError("%r is not a supported artifact" % concrete)
-        new = create((cls.bug, cls.branch), insert_values, get_objects=True)
+        new = create(
+            (cls.bug, cls.branch, cls.specification),
+            insert_values, get_objects=True)
         return list(existing) + new
 
     @classmethod

=== modified file 'lib/lp/registry/services/sharingservice.py'
--- lib/lp/registry/services/sharingservice.py	2012-09-05 22:02:39 +0000
+++ lib/lp/registry/services/sharingservice.py	2012-09-10 10:23:19 +0000
@@ -523,7 +523,7 @@
             user, artifacts, grantee=grantee, pillar=pillar)
 
     def ensureAccessGrants(self, grantees, user, branches=None, bugs=None,
-                           ignore_permissions=False):
+                           specifications=None, ignore_permissions=False):
         """See `ISharingService`."""
 
         artifacts = []
@@ -531,6 +531,8 @@
             artifacts.extend(branches)
         if bugs:
             artifacts.extend(bugs)
+        if specifications:
+            artifacts.extend(specifications)
         if not ignore_permissions:
             # The user needs to have launchpad.Edit permission on all supplied
             # bugs and branches or else we raise an Unauthorized exception.

=== modified file 'lib/lp/registry/services/tests/test_sharingservice.py'
--- lib/lp/registry/services/tests/test_sharingservice.py	2012-09-05 22:02:39 +0000
+++ lib/lp/registry/services/tests/test_sharingservice.py	2012-09-10 10:23:19 +0000
@@ -10,9 +10,11 @@
 import transaction
 from zope.component import getUtility
 from zope.security.interfaces import Unauthorized
+from zope.security.proxy import removeSecurityProxy
 from zope.traversing.browser.absoluteurl import absoluteURL
 
 from lp.app.interfaces.services import IService
+from lp.blueprints.interfaces.specification import ISpecification
 from lp.bugs.interfaces.bug import IBug
 from lp.code.enums import (
     BranchSubscriptionNotificationLevel,
@@ -43,6 +45,7 @@
     admin_logged_in,
     login,
     login_person,
+    person_logged_in,
     StormStatementRecorder,
     TestCaseWithFactory,
     WebServiceTestCase,
@@ -1011,22 +1014,26 @@
             ValueError, self.service.revokeAccessGrants,
             product, grantee, product.owner)
 
-    def _assert_ensureAccessGrants(self, user, bugs, branches,
+    def _assert_ensureAccessGrants(self, user, bugs, branches, specifications,
                                    grantee=None):
         # Creating access grants works as expected.
         if not grantee:
             grantee = self.factory.makePerson()
         self.service.ensureAccessGrants(
-            [grantee], user, bugs=bugs, branches=branches)
+            [grantee], user, bugs=bugs, branches=branches,
+            specifications=specifications)
 
         # Check that grantee has expected access grants.
         shared_bugs = []
         shared_branches = []
+        shared_specifications = []
         all_pillars = []
         for bug in bugs or []:
             all_pillars.extend(bug.affected_pillars)
         for branch in branches or []:
             all_pillars.append(branch.target.context)
+        for specification in specifications or []:
+            all_pillars.append(specification.target)
         policies = getUtility(IAccessPolicySource).findByPillar(all_pillars)
 
         apgfs = getUtility(IAccessPolicyGrantFlatSource)
@@ -1036,8 +1043,11 @@
                 shared_bugs.append(a.concrete_artifact)
             elif IBranch.providedBy(a.concrete_artifact):
                 shared_branches.append(a.concrete_artifact)
+            elif ISpecification.providedBy(a.concrete_artifact):
+                shared_specifications.append(a.concrete_artifact)
         self.assertContentEqual(bugs or [], shared_bugs)
         self.assertContentEqual(branches or [], shared_branches)
+        self.assertContentEqual(specifications or [], shared_specifications)
 
     def test_ensureAccessGrantsBugs(self):
         # Access grants can be created for bugs.
@@ -1047,7 +1057,7 @@
         bug = self.factory.makeBug(
             target=distro, owner=owner,
             information_type=InformationType.USERDATA)
-        self._assert_ensureAccessGrants(owner, [bug], None)
+        self._assert_ensureAccessGrants(owner, [bug], None, None)
 
     def test_ensureAccessGrantsBranches(self):
         # Access grants can be created for branches.
@@ -1057,7 +1067,21 @@
         branch = self.factory.makeBranch(
             product=product, owner=owner,
             information_type=InformationType.USERDATA)
-        self._assert_ensureAccessGrants(owner, None, [branch])
+        self._assert_ensureAccessGrants(owner, None, [branch], None)
+
+    def test_ensureAccessGrantsSpecifications(self):
+        # Access grants can be created for branches.
+        owner = self.factory.makePerson()
+        product = self.factory.makeProduct(owner=owner)
+        login_person(owner)
+        specification = self.factory.makeSpecification(
+            product=product, owner=owner)
+        removeSecurityProxy(specification.target)._ensurePolicies(
+             [InformationType.EMBARGOED])
+        with person_logged_in(owner):
+            specification.transitionToInformationType(
+                InformationType.EMBARGOED, owner)
+        self._assert_ensureAccessGrants(owner, None, None, [specification])
 
     def test_ensureAccessGrantsExisting(self):
         # Any existing access grants are retained and new ones created.
@@ -1075,7 +1099,8 @@
         self.service.ensureAccessGrants([grantee], owner, bugs=[bug])
         # Test with a new bug as well as the one for which access is already
         # granted.
-        self._assert_ensureAccessGrants(owner, [bug, bug2], None, grantee)
+        self._assert_ensureAccessGrants(
+            owner, [bug, bug2], None, None, grantee)
 
     def _assert_ensureAccessGrantsUnauthorized(self, user):
         # ensureAccessGrants raises an Unauthorized exception if the user

=== modified file 'lib/lp/registry/tests/test_accesspolicy.py'
--- lib/lp/registry/tests/test_accesspolicy.py	2012-09-03 11:22:25 +0000
+++ lib/lp/registry/tests/test_accesspolicy.py	2012-09-10 10:23:19 +0000
@@ -289,6 +289,13 @@
         return self.factory.makeBug()
 
 
+class TestAccessArtifactSpecification(BaseAccessArtifactTests,
+                            TestCaseWithFactory):
+
+    def getConcreteArtifact(self):
+        return self.factory.makeSpecification()
+
+
 class TestAccessArtifactGrant(TestCaseWithFactory):
     layer = DatabaseFunctionalLayer