launchpad-reviewers team mailing list archive

[Abel Deuring <abel.deuring@xxxxxxxxxxxxx>]

Abel Deuring has proposed merging lp:~adeuring/launchpad/bug-1056881 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1056881 in Launchpad itself: "Specifications privacy: subscribers can't see private blueprints"
  https://bugs.launchpad.net/launchpad/+bug/1056881

For more details, see:
https://code.launchpad.net/~adeuring/launchpad/bug-1056881/+merge/135374

This branch fixes bug 1056881: Specifications privacy: subscribers can't see private blueprints

The fix follows the pattern already used for private bugs and branches: When users are subscribed to a private branch or bug and if they do not have a policy grant for the pillar, an artifact grant is created.

Automatically adding grants when somebody is subscribed does not help if a public specification is made proprietary or embargoed, so I changed transitionToInformationType() also: Subscribers who would lose access to the specification get automatically an artifact grant.

Another change: SPecification owners and admins can now unsubscribe other subscribers; before, only the subscribers themselves could do this. This limitation is obviously bad when the owner of a private spec inadvertently subscribes the wrong peron or team, or when an access grant should be revoked after some time.

tests:

./bin/test blueprints -vvt lp.blueprints.model.tests.test_specification.TestSpecificationInformationType.test_transitionToInformationType_adds_grants_for_subscribers
./bin/test blueprints -vvt tests.test_specification.*subscribe
./bin/test blueprints -vvt tests.test_subscription.SpecificationTests

= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/blueprints/model/specification.py
  lib/lp/blueprints/model/specificationsubscription.py
  lib/lp/blueprints/model/tests/test_specification.py
  lib/lp/blueprints/model/tests/test_subscription.py
  lib/lp/blueprints/tests/test_specification.py

./lib/lp/blueprints/model/tests/test_specification.py
     625: E251 no spaces around keyword / parameter equals
     659: E251 no spaces around keyword / parameter equals

the errors indicate that we use too long names:

    624         product = self.factory.makeProduct(
    625             specification_sharing_policy=
    626                 SpecificationSharingPolicy.PUBLIC_OR_PROPRIETARY)

-- 
https://code.launchpad.net/~adeuring/launchpad/bug-1056881/+merge/135374
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~adeuring/launchpad/bug-1056881 into lp:launchpad.

=== modified file 'lib/lp/blueprints/model/specification.py'
--- lib/lp/blueprints/model/specification.py	2012-11-16 20:30:12 +0000
+++ lib/lp/blueprints/model/specification.py	2012-11-21 11:31:25 +0000
@@ -50,8 +50,10 @@
     InformationType,
     PUBLIC_INFORMATION_TYPES,
     )
+from lp.app.enums import PRIVATE_INFORMATION_TYPES
 from lp.app.errors import UserCannotUnsubscribePerson
 from lp.app.interfaces.informationtype import IInformationType
+from lp.app.interfaces.services import IService
 from lp.app.model.launchpad import InformationTypeMixin
 from lp.blueprints.adapters import SpecificationDelta
 from lp.blueprints.enums import (
@@ -86,6 +88,10 @@
 from lp.bugs.model.buglinktarget import BugLinkTargetMixin
 from lp.registry.enums import SpecificationSharingPolicy
 from lp.registry.errors import CannotChangeInformationType
+from lp.registry.interfaces.accesspolicy import (
+    IAccessArtifactGrantSource,
+    IAccessArtifactSource,
+    )
 from lp.registry.interfaces.distribution import IDistribution
 from lp.registry.interfaces.distroseries import IDistroSeries
 from lp.registry.interfaces.person import validate_public_person
@@ -728,6 +734,15 @@
             property_cache.subscriptions.append(sub)
             property_cache.subscriptions.sort(
                 key=lambda sub: person_sort_key(sub.person))
+        if self.information_type in PRIVATE_INFORMATION_TYPES:
+            # Grant the subscriber access if they can't see the
+            # specification.
+            service = getUtility(IService, 'sharing')
+            ignored, ignored, shared_specs = service.getVisibleArtifacts(
+                person, specifications=[self], ignore_permissions=True)
+            if not shared_specs:
+                service.ensureAccessGrants(
+                    [person], subscribed_by, specifications=[self])
         notify(ObjectCreatedEvent(sub, user=subscribed_by))
         return sub
 
@@ -746,6 +761,10 @@
                             person.displayname))
                 get_property_cache(self).subscriptions.remove(sub)
                 SpecificationSubscription.delete(sub.id)
+                artifacts_to_delete = getUtility(
+                    IAccessArtifactSource).find([self])
+                getUtility(IAccessArtifactGrantSource).revokeByArtifact(
+                    artifacts_to_delete, [person])
                 return
 
     def isSubscribed(self, person):
@@ -875,6 +894,16 @@
             raise CannotChangeInformationType("Forbidden by project policy.")
         self.information_type = information_type
         reconcile_access_for_artifact(self, information_type, [self.target])
+        if information_type in PRIVATE_INFORMATION_TYPES and self.subscribers:
+            # Grant the subscribers access if they do not have a
+            # policy grant.
+            service = getUtility(IService, 'sharing')
+            blind_subscribers = service.getPeopleWithoutAccess(
+                self, self.subscribers)
+            if len(blind_subscribers):
+                service.ensureAccessGrants(
+                    blind_subscribers, who, specifications=[self],
+                    ignore_permissions=True)
         return True
 
     @cachedproperty

=== modified file 'lib/lp/blueprints/model/specificationsubscription.py'
--- lib/lp/blueprints/model/specificationsubscription.py	2011-12-30 06:14:56 +0000
+++ lib/lp/blueprints/model/specificationsubscription.py	2012-11-21 11:31:25 +0000
@@ -11,11 +11,17 @@
     BoolCol,
     ForeignKey,
     )
+from zope.component import getUtility
 from zope.interface import implements
 
 from lp.blueprints.interfaces.specificationsubscription import (
     ISpecificationSubscription,
     )
+from lp.registry.interfaces.accesspolicy import (
+    IAccessArtifactGrantSource,
+    IAccessArtifactSource,
+    )
+from lp.registry.interfaces.role import IPersonRoles
 from lp.registry.interfaces.person import validate_person
 from lp.services.database.sqlbase import SQLBase
 
@@ -37,6 +43,26 @@
         """See `ISpecificationSubscription`."""
         if user is None:
             return False
-        if self.person.is_team:
-            return user.inTeam(self.person)
-        return user == self.person
+        if not IPersonRoles.providedBy(user):
+            user = IPersonRoles(user)
+        if (
+            user.inTeam(self.specification.owner) or
+            user.inTeam(self.person) or
+            user.in_admin):
+            return True
+        # People who subscribed users should be able to unsubscribe
+        # them again, similar to branch subscriptions. This is
+        # essential if somebody was erroneuosly subscribed to a
+        # proprietary or embargoed specification. Unfortunately,
+        # SpecificationSubscription does not record who subscribed
+        # somebody else, but if the specification is private, we can
+        # check who issued the artifact grant.
+        artifacts = getUtility(IAccessArtifactSource).find(
+            [self.specification])
+        wanted = [(artifact, self.person) for artifact in artifacts]
+        if len(wanted) == 0:
+            return False
+        for grant in getUtility(IAccessArtifactGrantSource).find(wanted):
+            if user.inTeam(grant.grantor):
+                return True
+        return False

=== modified file 'lib/lp/blueprints/model/tests/test_specification.py'
--- lib/lp/blueprints/model/tests/test_specification.py	2012-09-17 15:19:10 +0000
+++ lib/lp/blueprints/model/tests/test_specification.py	2012-11-21 11:31:25 +0000
@@ -5,6 +5,8 @@
 
 __metaclass__ = type
 
+from zope.component import getUtility
+
 from lazr.lifecycle.event import ObjectModifiedEvent
 from lazr.lifecycle.snapshot import Snapshot
 from testtools.matchers import (
@@ -19,13 +21,17 @@
 from zope.security.proxy import removeSecurityProxy
 
 from lp.app.enums import InformationType
+from lp.app.interfaces.services import IService
 from lp.app.validators import LaunchpadValidationError
 from lp.blueprints.interfaces.specification import ISpecification
 from lp.blueprints.interfaces.specificationworkitem import (
     SpecificationWorkItemStatus,
     )
 from lp.blueprints.model.specificationworkitem import SpecificationWorkItem
-from lp.registry.enums import SpecificationSharingPolicy
+from lp.registry.enums import (
+    SharingPermission,
+    SpecificationSharingPolicy,
+    )
 from lp.registry.errors import CannotChangeInformationType
 from lp.registry.model.milestone import Milestone
 from lp.services.mail import stub
@@ -643,3 +649,45 @@
         with person_logged_in(spec.owner):
             with ExpectedException(CannotChangeInformationType, '.*'):
                 spec.transitionToInformationType(None, spec.owner)
+
+    def test_transitionToInformationType_adds_grants_for_subscribers(self):
+        # Subscribers are automatically granted access when the
+        # new information type requires a grant.
+        owner = self.factory.makePerson()
+        product = self.factory.makeProduct(
+            owner=owner,
+            specification_sharing_policy=
+                SpecificationSharingPolicy.PUBLIC_OR_PROPRIETARY)
+        spec = self.factory.makeSpecification(product=product)
+        subscriber_with_policy_grant = self.factory.makePerson()
+        subscriber_without_policy_grant = self.factory.makePerson()
+        service = getUtility(IService, 'sharing')
+        with person_logged_in(owner):
+            service.sharePillarInformation(
+                product, subscriber_with_policy_grant, owner,
+                permissions={
+            InformationType.PROPRIETARY: SharingPermission.ALL,
+            })
+            spec.subscribe(subscriber_with_policy_grant, owner)
+            spec.subscribe(subscriber_without_policy_grant, owner)
+
+            # The specification is public, hence subscribers do not need
+            #  and do not have access grants.
+            self.assertEqual(
+                [], service.getSharedSpecifications(
+                    product, subscriber_without_policy_grant, owner))
+            self.assertEqual(
+                [], service.getSharedSpecifications(
+                    product, subscriber_with_policy_grant, owner))
+
+            spec.transitionToInformationType(
+                InformationType.PROPRIETARY, owner)
+            # transitionToInformationType() added an artifact grant for
+            # subscriber_without_policy_grant.
+            self.assertEqual(
+                [spec], service.getSharedSpecifications(
+                    product, subscriber_without_policy_grant, owner))
+            # No access grant was created for subscriber_with_policy_grant.
+            self.assertEqual(
+                [], service.getSharedSpecifications(
+                    product, subscriber_with_policy_grant, owner))

=== modified file 'lib/lp/blueprints/model/tests/test_subscription.py'
--- lib/lp/blueprints/model/tests/test_subscription.py	2012-01-01 02:58:52 +0000
+++ lib/lp/blueprints/model/tests/test_subscription.py	2012-11-21 11:31:25 +0000
@@ -3,7 +3,15 @@
 
 __metaclass__ = type
 
+from zope.component import getUtility
+
+from lp.app.enums import InformationType
 from lp.app.errors import UserCannotUnsubscribePerson
+from lp.app.interfaces.services import IService
+from lp.registry.enums import (
+    SharingPermission,
+    SpecificationSharingPolicy,
+    )
 from lp.testing import (
     person_logged_in,
     TestCaseWithFactory,
@@ -21,11 +29,35 @@
 
     layer = DatabaseFunctionalLayer
 
-    def _make_subscription(self):
-        spec = self.factory.makeSpecification()
+    def _make_subscription(self, proprietary_subscription=False):
         subscriber = self.factory.makePerson()
         subscribed_by = self.factory.makePerson()
-        subscription = spec.subscribe(subscriber, subscribed_by)
+        policy = SpecificationSharingPolicy.PUBLIC_OR_PROPRIETARY
+        product_owner = self.factory.makePerson()
+        product = self.factory.makeProduct(
+            specification_sharing_policy=policy, owner=product_owner)
+        if proprietary_subscription:
+            info_type = InformationType.PROPRIETARY
+            with person_logged_in(product_owner):
+                permissions = {
+                    InformationType.PROPRIETARY: SharingPermission.ALL,
+                    }
+                getUtility(IService, 'sharing').sharePillarInformation(
+                    product, subscribed_by, product_owner, permissions)
+        else:
+            info_type = InformationType.PUBLIC
+        if proprietary_subscription:
+            # If the spec is proprietary, subscribed_by must have the
+            # permission launchpad.Edit on the spec in order to
+            # subscribe someone. This permission requires to have a
+            # special role for the specificaiton, like the assignee.
+            assignee = subscribed_by
+        else:
+            assignee = None
+        spec = self.factory.makeSpecification(
+            product=product, information_type=info_type, assignee=assignee)
+        with person_logged_in(subscribed_by):
+            subscription = spec.subscribe(subscriber, subscribed_by)
         return spec, subscriber, subscribed_by, subscription
 
     def test_can_unsubscribe_self(self):
@@ -35,13 +67,20 @@
             subscribed_by, subscription) = self._make_subscription()
         self.assertTrue(subscription.canBeUnsubscribedByUser(subscriber))
 
-    def test_subscriber_cannot_unsubscribe_user(self):
-        # The one who subscribed the subscriber doesn't have permission to
-        # unsubscribe him.
+    def test_subscriber_cannot_unsubscribe_user_from_public_spec(self):
+        # For public specifications, the one who subscribed the
+        # subscriber doesn't have permission to unsubscribe him.
         (spec, subscriber,
             subscribed_by, subscription) = self._make_subscription()
         self.assertFalse(subscription.canBeUnsubscribedByUser(subscribed_by))
 
+    def test_subscriber_can_unsubscribe_user_from_private_spec(self):
+        # For private specifications, the one who subscribed the
+        # subscriber has permission to unsubscribe him.
+        (spec, subscriber,
+            subscribed_by, subscription) = self._make_subscription(True)
+        self.assertTrue(subscription.canBeUnsubscribedByUser(subscribed_by))
+
     def test_anonymous_cannot_unsubscribe(self):
         # The anonymous user (represented by None) can't unsubscribe anyone.
         (spec, subscriber,
@@ -84,3 +123,16 @@
         person = self.factory.makePerson()
         self.assertRaises(
             UserCannotUnsubscribePerson, spec.unsubscribe, subscriber, person)
+
+    def test_spec_owner_can_unsubscribe(self):
+        # The owner of a specification can unsubscribe any subscriber.
+        (spec, subscriber,
+            subscribed_by, subscription) = self._make_subscription()
+        self.assertTrue(subscription.canBeUnsubscribedByUser(spec.owner))
+
+    def test_admin_can_unsubscribe(self):
+        # LP admins can unsubscribe any subscriber.
+        (spec, subscriber,
+            subscribed_by, subscription) = self._make_subscription()
+        admin = self.factory.makeAdministrator()
+        self.assertTrue(subscription.canBeUnsubscribedByUser(admin))

=== modified file 'lib/lp/blueprints/tests/test_specification.py'
--- lib/lp/blueprints/tests/test_specification.py	2012-11-15 22:02:33 +0000
+++ lib/lp/blueprints/tests/test_specification.py	2012-11-21 11:31:25 +0000
@@ -442,6 +442,62 @@
         self.assertContentEqual(
             [public_spec, proprietary_spec_1], specs_for_other_user)
 
+    def test_subscribe_to_proprietary_spec(self):
+        # If users are subscribed to a proprietary specification,
+        # they are automatically granted access to the specification.
+        owner = self.factory.makePerson()
+        spec_sharing_policy = SpecificationSharingPolicy.PROPRIETARY_OR_PUBLIC
+        product = self.factory.makeProduct(
+            owner=owner, specification_sharing_policy=spec_sharing_policy)
+        with person_logged_in(owner):
+            user = self.factory.makePerson()
+            spec = self.factory.makeSpecification(
+                product=product,
+                information_type=InformationType.PROPRIETARY)
+            spec.subscribe(user, subscribed_by=owner)
+            service = getUtility(IService, 'sharing')
+            ignored, ignored, shared_specs = service.getVisibleArtifacts(
+                user, specifications=[spec])
+            self.assertEqual([spec], shared_specs)
+            # The spec is also returned by getSharedSpecifications(),
+            # which lists only specifications for which the use has
+            # an artifact grant.
+            self.assertEqual(
+                [spec], service.getSharedSpecifications(product, user, owner))
+            # Users which have a policy grants for the spec's target
+            # do not get an additional artifact grant...
+            user_2 = self.factory.makePerson()
+            permissions = {
+                InformationType.PROPRIETARY: SharingPermission.ALL,
+                }
+            service.sharePillarInformation(
+                product, user_2, owner, permissions)
+            spec.subscribe(user_2, subscribed_by=owner)
+            ignored, ignored, shared_specs = service.getVisibleArtifacts(
+                user_2, specifications=[spec])
+            self.assertEqual([spec], shared_specs)
+            self.assertEqual(
+                [], service.getSharedSpecifications(product, user_2, owner))
+
+    def test_unsubscribe_from_proprietary_spec(self):
+        # If users are unsubscribed from a proprietary specification,
+        # a related artifact grant is deleted too.
+        owner = self.factory.makePerson()
+        spec_sharing_policy = SpecificationSharingPolicy.PROPRIETARY_OR_PUBLIC
+        product = self.factory.makeProduct(
+            owner=owner, specification_sharing_policy=spec_sharing_policy)
+        with person_logged_in(owner):
+            user = self.factory.makePerson()
+            spec = self.factory.makeSpecification(
+                product=product,
+                information_type=InformationType.PROPRIETARY)
+            spec.subscribe(user, subscribed_by=owner)
+            spec.unsubscribe(user, unsubscribed_by=owner)
+            service = getUtility(IService, 'sharing')
+            ignored, ignored, shared_specs = service.getVisibleArtifacts(
+                user, specifications=[spec])
+            self.assertEqual([], shared_specs)
+
 
 class TestSpecificationSet(TestCaseWithFactory):