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):
_______________________________________________
Mailing list: https://launchpad.net/~launchpad-reviewers
Post to : launchpad-reviewers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~launchpad-reviewers
More help : https://help.launchpad.net/ListHelp
